amadan

oficina / sheets Public

fix(grid): a cell on row 151 was not on the screen that edits cells

The grid page opened a CONSTANT window — 100 rows by 26 columns — and
never looked at the sheet first. Meanwhile the cell route accepts a
write below row 5,000, the table view lists whatever rows exist, and
the export carries all of them.

So: POST a cell at row 151, get a 200, see it in the table view, see it
in the .xlsx, and find no row 151 on the grid to scroll to. Nothing
anywhere said a window existed. Measured in Chromium on 2026-09-03,
which is also how the correction to my first reading was found — the
cell IS reachable, in the table view and through the API, so this is
invisibility and not loss. It is still the one screen most people use
being unable to show them their own cell.

The window is now sized to the sheet: one past the furthest cell that
exists, plus a screenful of room to keep typing into, with the default
as a FLOOR and the route's own maximum as the ceiling. `clamp` is
deliberately not reused for this and the comment says why — its `def`
is what a request of zero gets, so a sheet holding one cell in A1 asked
for a one-row grid and clamp handed back one row. That mistake is in
this branch's history, caught by the test that says a nearly empty
sheet must still open a grid somebody can work in.

Past the ceiling the honest answer is words, not a wider grid: the page
carries data-window-truncated and the status line reads it out. Silently
painting the first 500 rows of a longer sheet is the same quiet as the
constant window was, one order of magnitude further along.

note.js is new, and small: the status line's own module. Two callers
need it now — this and clipboard.js — and the 300-line cap caught the
second one being written inside the first, which is how a shared thing
ends up with two copies and two timers that clear each other.

Found by Fable, reviewing the shipped app for what is quietly
unfinished; confirmed independently by the spec review, which argued it
belongs ahead of new features. It does.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014od11UreNE4Ewf3oR9837B
Paul Campbell pushed by paul@keymail.dev 3109f685361d79400b06d8a1963cf5f3a1c15412 parent f2a753c
6 files changed, +287 −14
  • internal/sheets/grid.go +30 −0
  • internal/sheets/handlers_sheets.go +54 −1
  • internal/sheets/handlers_window_test.go +127 −0
  • internal/sheets/static/clipboard.js +7 −13
  • internal/sheets/static/note.js +60 −0
  • internal/sheets/templates/sheet.html +9 −0

CI passed — run details

Intent

full ledger: fix/grid-window-extent

diff --git a/internal/sheets/grid.go b/internal/sheets/grid.go
index 980b171..24f6afe 100644
--- a/internal/sheets/grid.go
+++ b/internal/sheets/grid.go
@@ -127,6 +127,36 @@ func newPosIndex(h *store.Handle) posIndex {
return idx
}
+// extent is how far a tab's content actually reaches: one past the
+// furthest row and column that hold a cell.
+//
+// It exists because the grid page opened a CONSTANT window. 100 rows is
+// the right default for a new sheet and the wrong answer for one whose
+// content runs to row 151 — the cell route accepts that write, the table
+// view lists it and the export carries it, and the grid simply had no
+// row 151 to scroll to. Nothing was lost and nothing said anything.
+//
+// Positions, not ids: a cell records the row and column it belongs to by
+// id, and only the index knows where those sit. A row record with no
+// cells in it does not count, which is deliberate — EnsureRow is called
+// on the way to a write, so counting rows rather than cells would widen
+// the window by whatever a failed write left behind.
+func extent(h *store.Handle, idx posIndex) (rows, cols int) {
+ coords, err := h.Cells()
+ if err != nil {
+ return 0, 0
+ }
+ for _, c := range coords {
+ if r, ok := idx.rowPos[c.RowID]; ok && r+1 > rows {
+ rows = r + 1
+ }
+ if col, ok := idx.colPos[c.ColID]; ok && col+1 > cols {
+ cols = col + 1
+ }
+ }
+ return rows, cols
+}
+
// snapshot reads every cell inside a window.
//
// Sparse: only cells that exist are sent. A blank cell is absent from
diff --git a/internal/sheets/handlers_sheets.go b/internal/sheets/handlers_sheets.go
index 706b906..88f122e 100644
--- a/internal/sheets/handlers_sheets.go
+++ b/internal/sheets/handlers_sheets.go
@@ -39,6 +39,11 @@ type sheetPageView struct {
Sheet SheetSummary
Rows int
Cols int
+ // Truncated is set when the sheet reaches past the widest window
+ // the grid can paint. It is rendered as an attribute the page can
+ // read rather than as prose in Go, so the wording lives with the
+ // rest of the copy.
+ Truncated bool
// MayShare drives whether the page offers a Share link. The route
// checks again — this only decides what to render, and a button
// that 404s is worse than no button.
@@ -151,12 +156,60 @@ func (a *App) handleSheetPage(w http.ResponseWriter, r *http.Request) {
// Best effort: a sheet whose handle will not open still renders its
// page, and the grid's own fetch reports the failure properly.
if h, err := a.openTabFor(s); err == nil {
- view.Warned = warnedCells(h, newPosIndex(h), defaultWindowRows, defaultWindowCols)
+ idx := newPosIndex(h)
+ // The window is sized to the sheet rather than to a constant.
+ // clamp's floor keeps a one-cell sheet opening a grid somebody
+ // can type into, and its ceiling is what the cells route would
+ // clamp the request to anyway — asking for more than the server
+ // will serve renders empty rows the grid can scroll to and never
+ // fill, which is a different lie in the same place.
+ eRows, eCols := extent(h, idx)
+ view.Rows = windowFor(eRows, defaultWindowRows, maxWindowRows)
+ view.Cols = windowFor(eCols, defaultWindowCols, maxWindowCols)
+ // Past the ceiling the honest answer is words, not a wider grid.
+ // The page says so and clipboard.js's status line reads it out;
+ // silently painting the first 500 rows of a longer sheet is the
+ // same class of quiet as the constant window was.
+ view.Truncated = eRows > maxWindowRows || eCols > maxWindowCols
+ view.Warned = warnedCells(h, idx, view.Rows, view.Cols)
h.Release()
}
a.render(w, r, "sheet", s.Title+" — Oficina Sheets", http.StatusOK, view)
}
+// windowGrowRoom is the empty space kept below and to the right of a
+// sheet's furthest cell.
+//
+// Without it the window ends exactly where the content does, and the
+// row under the last one you typed in is not on the screen — a grid you
+// cannot add a row to is a worse bug than the one this fixes. A
+// screenful is the right size: enough to keep typing into, not so much
+// that the scrollbar claims a sheet far longer than it is.
+const windowGrowRoom = 20
+
+// windowFor sizes one axis of the grid's window.
+//
+// clamp is deliberately not used, and the difference is the whole point:
+// clamp's `def` is what you get for a request of ZERO, so a sheet with a
+// single cell in A1 asked for a one-row grid and clamp handed back one
+// row. def here is a FLOOR — a nearly empty sheet still opens a grid
+// somebody can work in.
+//
+// The ceiling is what the cells route would clamp a request to anyway.
+// Opening a window wider than the server will serve paints rows that can
+// be scrolled to and never filled, which is a different lie in the same
+// place.
+func windowFor(reach, floor, ceiling int) int {
+ want := reach + windowGrowRoom
+ if want < floor {
+ want = floor
+ }
+ if want > ceiling {
+ want = ceiling
+ }
+ return want
+}
+
// sheetFromURL reads {sheetID} and loads it, answering 404 itself if it
// is not there. The bool is whether the caller should carry on.
//
diff --git a/internal/sheets/handlers_window_test.go b/internal/sheets/handlers_window_test.go
new file mode 100644
index 0000000..7aaa70a
--- /dev/null
+++ b/internal/sheets/handlers_window_test.go
@@ -0,0 +1,127 @@
+package sheets
+
+import (
+ "net/http"
+ "net/http/httptest"
+ "strconv"
+ "strings"
+ "testing"
+)
+
+// The grid page opens a WINDOW, not a whole sheet — 100 rows by 26
+// columns, because virtualising a real spreadsheet is a lot of client
+// code (sheetsvc.go says so, and it is right).
+//
+// What it did not do was look at the sheet first. Rows was the constant
+// on every render, while the cell route accepts a write far past it and
+// the table view lists whatever rows exist. Measured on 2026-09-03: a
+// cell written to row 151 came back 200, appeared in the table view and
+// in the export, and was invisible on the grid — no scrollbar reaching
+// it, no row 151 to scroll to, and no copy anywhere saying a window
+// existed at all.
+//
+// It is not lost data, which is why this is a bug and not an incident.
+// It is worse than it sounds anyway: the one screen most people will
+// ever use is the one that cannot show them the cell.
+
+func writeCellAt(t *testing.T, app *App, cookies []*http.Cookie, cells string, r, c int, raw string) {
+ t.Helper()
+ body := `{"r":` + strconv.Itoa(r) + `,"c":` + strconv.Itoa(c) + `,"raw":` + strconv.Quote(raw) + `}`
+ if rec := postCell(t, app, cookies, cells, body); rec.Code != http.StatusOK {
+ t.Fatalf("writing %d,%d = %d: %s", r, c, rec.Code, rec.Body.String())
+ }
+}
+
+// gridPage renders /sheets/{id} and returns the HTML.
+func gridPage(t *testing.T, app *App, cookies []*http.Cookie, id uint64) string {
+ t.Helper()
+ req := httptest.NewRequest(http.MethodGet, "/sheets/"+strconv.FormatUint(id, 10), nil)
+ rec := do(t, app, as(req, cookies))
+ if rec.Code != http.StatusOK {
+ t.Fatalf("grid page = %d", rec.Code)
+ }
+ return rec.Body.String()
+}
+
+func TestTheGridWindowReachesTheFurthestCell(t *testing.T) {
+ app, cookies := signIn(t)
+ s, cells := newSheet(t, app, cookies, "Deep")
+
+ if got := gridPage(t, app, cookies, s.ID); !strings.Contains(got, `data-rows="100"`) {
+ t.Fatalf("an empty sheet should still open the default window; page has no data-rows=\"100\"")
+ }
+
+ writeCellAt(t, app, cookies, cells, 150, 0, "I am on row 151")
+
+ page := gridPage(t, app, cookies, s.ID)
+ rows := attrInt(t, page, "data-rows")
+ if rows <= 150 {
+ t.Errorf("a cell on row 151 and the grid opens %d rows — it cannot be scrolled to, "+
+ "and nothing on the page says so", rows)
+ }
+}
+
+func TestTheGridWindowReachesTheFurthestColumn(t *testing.T) {
+ app, cookies := signIn(t)
+ s, cells := newSheet(t, app, cookies, "Wide")
+
+ writeCellAt(t, app, cookies, cells, 0, 40, "column AO")
+
+ page := gridPage(t, app, cookies, s.ID)
+ if cols := attrInt(t, page, "data-cols"); cols <= 40 {
+ t.Errorf("a cell in column AO and the grid opens %d columns", cols)
+ }
+}
+
+// TestTheWindowNeverShrinksBelowTheDefault: a nearly empty sheet must
+// still open a grid somebody can type into. Sizing the window to the
+// content alone would open a 1x1 grid on a sheet with one cell in it.
+func TestTheWindowNeverShrinksBelowTheDefault(t *testing.T) {
+ app, cookies := signIn(t)
+ s, cells := newSheet(t, app, cookies, "Sparse")
+ writeCellAt(t, app, cookies, cells, 0, 0, "one cell")
+
+ page := gridPage(t, app, cookies, s.ID)
+ if rows := attrInt(t, page, "data-rows"); rows != defaultWindowRows {
+ t.Errorf("data-rows = %d on a one-cell sheet, want the default %d", rows, defaultWindowRows)
+ }
+ if cols := attrInt(t, page, "data-cols"); cols != defaultWindowCols {
+ t.Errorf("data-cols = %d, want the default %d", cols, defaultWindowCols)
+ }
+}
+
+// TestTheWindowStopsAtTheMaximum: past maxWindow* the answer cannot be
+// "open a bigger window", so it has to be words. A sheet whose content
+// runs past what the grid can paint says so, in the status line the
+// clipboard already uses, rather than silently showing part of itself.
+func TestTheWindowStopsAtTheMaximum(t *testing.T) {
+ app, cookies := signIn(t)
+ s, cells := newSheet(t, app, cookies, "Enormous")
+
+ writeCellAt(t, app, cookies, cells, maxWindowRows+200, 0, "past the end")
+
+ page := gridPage(t, app, cookies, s.ID)
+ if rows := attrInt(t, page, "data-rows"); rows != maxWindowRows {
+ t.Errorf("data-rows = %d, want it clamped to %d", rows, maxWindowRows)
+ }
+ if !strings.Contains(page, "data-window-truncated") {
+ t.Error("the grid is showing part of a sheet and the page does not say so — " +
+ "the one thing worse than a window is an unannounced one")
+ }
+}
+
+// attrInt pulls an integer attribute out of the rendered page.
+func attrInt(t *testing.T, page, attr string) int {
+ t.Helper()
+ i := strings.Index(page, attr+`="`)
+ if i < 0 {
+ t.Fatalf("the page carries no %s", attr)
+ }
+ rest := page[i+len(attr)+2:]
+ j := strings.Index(rest, `"`)
+ n, err := strconv.Atoi(rest[:j])
+ if err != nil {
+ t.Fatalf("%s=%q is not a number", attr, rest[:j])
+ }
+ return n
+}
diff --git a/internal/sheets/static/clipboard.js b/internal/sheets/static/clipboard.js
index 7685175..6ca8afb 100644
--- a/internal/sheets/static/clipboard.js
+++ b/internal/sheets/static/clipboard.js
@@ -48,20 +48,14 @@
var g = window.SheetsGrid;
if (!g) return;
- var note = document.querySelector('[data-sheet-note]');
-
- /* say is the only way this module talks. It exists because the menu
- * has a Paste item and the menu cannot always paste: reading the
- * clipboard from a button is a permission the browser may refuse,
- * where Ctrl+V hands us the data with no permission at all. Told
- * that, a person presses Ctrl+V; not told, the menu item is exactly
- * the dead control this app has a rule against. */
- var clearNote = null;
+ /* note.js. This module needs to talk because the menu has a Paste
+ * item and the menu cannot always paste: reading the clipboard from
+ * a button is a permission the browser may refuse, where Ctrl+V
+ * hands us the data with no permission at all. Told that, a person
+ * presses Ctrl+V; not told, the menu item is exactly the dead
+ * control this app has a rule against. */
function say(text) {
- if (!note) return;
- note.textContent = text;
- if (clearNote) clearTimeout(clearNote);
- clearNote = setTimeout(function () { note.textContent = ''; }, 6000);
+ if (window.SheetsNote) window.SheetsNote.say(text);
}
/* lastCopy is how a paste knows where it came from.
diff --git a/internal/sheets/static/note.js b/internal/sheets/static/note.js
new file mode 100644
index 0000000..3bf2927
--- /dev/null
+++ b/internal/sheets/static/note.js
@@ -0,0 +1,60 @@
+/* The status line under the grid.
+ *
+ * One sentence, cleared after a few seconds. It is the only way this app
+ * can tell somebody that a thing they asked for did not happen, and
+ * before it existed there was no such way at all: a browser refusing the
+ * menu's clipboard read, or a sheet reaching past the widest grid this
+ * build can paint, both simply went unsaid.
+ *
+ * Its own module rather than a function inside whichever module first
+ * needed it. Two already do — clipboard.js and grid.js — and a shared
+ * thing living inside one caller is how the second caller ends up with a
+ * second copy that clears the first one's timer.
+ *
+ * role="status" is on the element in sheet.html and is right there for a
+ * reason worth writing down, because the element beside it deliberately
+ * does NOT have it: the selection size changes on every arrow key and a
+ * live region announcing it would talk over the cell a screen reader is
+ * reading. This changes only when something happened, which is exactly
+ * what should be read out.
+ */
+(function () {
+ 'use strict';
+
+ var el = document.querySelector('[data-sheet-note]');
+ if (!el) return;
+
+ var timer = null;
+
+ /* The one thing this module says on its own behalf.
+ *
+ * The server sets data-window-truncated when the sheet reaches past
+ * the widest grid this build can paint. It is said here rather than
+ * rendered as a banner because a banner over a grid is permanent and
+ * this is not news twice; and it is read from the surface's own
+ * dataset rather than from window.SheetsGrid so that a sheet whose
+ * grid failed to boot still tells you its rows exist somewhere.
+ *
+ * Deferred to the next task so the status element is not written
+ * before role="status" has been observed — a live region populated in
+ * the same tick it is parsed is not reliably announced. */
+ var surface = document.querySelector('[data-sheet-surface]');
+ if (surface && surface.dataset.windowTruncated) {
+ setTimeout(function () {
+ window.SheetsNote.say('This sheet reaches past the ' +
+ surface.dataset.rows + ' rows and ' + surface.dataset.cols +
+ ' columns the grid can show. The table view lists every row.');
+ }, 0);
+ }
+
+ window.SheetsNote = {
+ /* say replaces whatever is showing. It does not queue: two things
+ * worth saying at once is a design problem, and a queue would show
+ * the second one to somebody who has already moved on. */
+ say: function (text) {
+ el.textContent = text;
+ if (timer) clearTimeout(timer);
+ timer = setTimeout(function () { el.textContent = ''; }, 8000);
+ }
+ };
+}());
diff --git a/internal/sheets/templates/sheet.html b/internal/sheets/templates/sheet.html
index 2902824..6621016 100644
--- a/internal/sheets/templates/sheet.html
+++ b/internal/sheets/templates/sheet.html
@@ -15,6 +15,7 @@
<script defer src="{{asset "static/grid.js"}}"></script>
<script defer src="{{asset "static/menubar.js"}}"></script>
<script defer src="{{asset "static/formulabar.js"}}"></script>
+<script defer src="{{asset "static/note.js"}}"></script>
<script defer src="{{asset "static/clipboard.js"}}"></script>
<script defer src="{{asset "static/commands.js"}}"></script>
{{end}}
@@ -32,6 +33,14 @@
data-tab-id="{{.Data.Sheet.TabID}}"
data-rows="{{.Data.Rows}}"
data-cols="{{.Data.Cols}}"
+ {{/* Set when the sheet reaches past the widest grid this build
+ can paint. grid.js reads it and says so in the status line
+ rather than the page shouting at somebody who may never
+ scroll that far — but it is said, which is the point: the
+ window used to be a constant 100 rows and a cell on row 151
+ was simply not on this screen, with nothing anywhere
+ admitting a window existed. */}}
+ {{if .Data.Truncated}}data-window-truncated="1"{{end}}
{{if .Data.MayWrite}}data-may-write="1"{{end}}>
{{template "content" .}}
</main>