amadan

oficina / sheets Public

fix: the grid's event handlers were one level too deep to ever fire

Jspreadsheet v5 takes onchange, onselection and oneditionstart on the
ROOT config, beside parseFormulas — not inside a worksheet. Nested one
level too deep they are accepted in silence and never called: the grid
rendered, the console stayed clean, every Go test passed, and not one
edit was ever sent to the server. Two real browser tabs found it in
about a minute.

The same run found rst-sr-only written as an attribute. It is one of
AGENTS.md's seven class-only utilities, so it has no attribute twin and
matched nothing — a screen-reader label rendered in the middle of the
toolbar. That one is now a guard test, verified to fail on the exact
markup that caused it: AGENTS.md says a rule you rely on remembering
should be a test instead.

The drive script is committed as a manual gate rather than folded into
`make test`, because it needs a browser and a running instance and CI
has neither. Its header says what it caught, so the next person knows
why it is worth running.

Verified: two tabs on one sheet, type 6 and =A1*7 in the first, and the
second shows 6 and 42 without a reload. Its formula bar shows =A1*7 for
the cell displaying 42.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018RN4HhHR4rHH3Fxh4LMds5
Paul Campbell pushed by paul@keymail.dev f7f63e9d450156cfb8f372b696c7486e6fdf2395 parent 26b1254
6 files changed, +315 −29
  • internal/guardstest/markup_test.go +114 −0
  • internal/sheets/grid.go +9 −6
  • internal/sheets/static/grid.js +29 −21
  • internal/sheets/static/sheets.css +10 −1
  • internal/sheets/templates/sheet.html +1 −1
  • tools/drive/drive.mjs +152 −0

CI passed — run details

diff --git a/internal/guardstest/markup_test.go b/internal/guardstest/markup_test.go
new file mode 100644
index 0000000..c4bf893
--- /dev/null
+++ b/internal/guardstest/markup_test.go
@@ -0,0 +1,114 @@
+package guards
+
+import (
+ "io/fs"
+ "os"
+ "path/filepath"
+ "regexp"
+ "strings"
+ "testing"
+)
+
+// classOnlyUtilities are the seven rastrillo names that are classes by
+// design, listed in AGENTS.md's Frontend section: "An attribute says
+// what a thing *is*; a class says what is being applied to it."
+//
+// They have no attribute twin in tokens.css, so writing one as an
+// attribute produces markup that matches nothing — and fails silently.
+// That is exactly what happened here: `rst-sr-only` was written as an
+// attribute on the formula bar's label, and the label rendered visibly
+// in the middle of the toolbar. Nothing errored; it just looked wrong,
+// and only a screenshot caught it. Hence this test.
+var classOnlyUtilities = []string{
+ "rst-grow", "rst-mono", "rst-sr-only", "rst-m-hide",
+ "rst-danger", "rst-nm", "rst-cell-mut",
+}
+
+// TestUtilitiesAreNeverWrittenAsAttributes walks the templates for a
+// class-only name used in attribute position.
+//
+// The match is deliberately narrow: `rst-sr-only` immediately after a
+// `<`-tag's whitespace and followed by `>`, `=`, or more whitespace.
+// `class="rst-sr-only"` does not match, because there the name is
+// inside a quoted value.
+func TestUtilitiesAreNeverWrittenAsAttributes(t *testing.T) {
+ root := repoRoot(t)
+ files := templateFiles(t, root)
+ if len(files) == 0 {
+ t.Fatal("guards: found no templates; this test is not checking anything")
+ }
+
+ for _, name := range classOnlyUtilities {
+ // The name in attribute position: preceded by whitespace, and
+ // followed by whitespace, '>', '/' or '=' — never by a quote,
+ // which is what a class value looks like.
+ re := regexp.MustCompile(`\s` + regexp.QuoteMeta(name) + `[\s>/=]`)
+ for _, rel := range files {
+ body, err := os.ReadFile(filepath.Join(root, rel))
+ if err != nil {
+ t.Fatalf("guards: read %s: %v", rel, err)
+ }
+ for _, line := range strings.Split(string(body), "\n") {
+ // A class list is the correct spelling and must not trip
+ // this: only look at the parts of the line outside quotes.
+ if re.MatchString(stripQuoted(line)) {
+ t.Errorf("%s: %s is written as an attribute; it is one of the seven "+
+ "class-only utilities (AGENTS.md Frontend) and has no attribute "+
+ "twin, so it matches nothing:\n %s",
+ rel, name, strings.TrimSpace(line))
+ }
+ }
+ }
+ }
+}
+
+// stripQuoted blanks the contents of double- and single-quoted runs, so
+// a name inside class="..." is invisible to the attribute check while
+// the same name written bare is not.
+func stripQuoted(line string) string {
+ var b strings.Builder
+ var quote byte
+ for i := 0; i < len(line); i++ {
+ c := line[i]
+ switch {
+ case quote != 0:
+ if c == quote {
+ quote = 0
+ b.WriteByte(c)
+ } else {
+ b.WriteByte(' ')
+ }
+ case c == '"' || c == '\'':
+ quote = c
+ b.WriteByte(c)
+ default:
+ b.WriteByte(c)
+ }
+ }
+ return b.String()
+}
+
+// templateFiles returns every .html file under the repo.
+func templateFiles(t *testing.T, root string) []string {
+ t.Helper()
+ var out []string
+ err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error {
+ if err != nil {
+ return err
+ }
+ if d.IsDir() {
+ if strings.HasPrefix(d.Name(), ".") || d.Name() == "node_modules" {
+ return filepath.SkipDir
+ }
+ return nil
+ }
+ if strings.HasSuffix(d.Name(), ".html") {
+ out = append(out, relPath(t, root, path))
+ }
+ return nil
+ })
+ if err != nil {
+ t.Fatalf("guards: walk: %v", err)
+ }
+ return out
+}
diff --git a/internal/sheets/grid.go b/internal/sheets/grid.go
index 21a1376..082a445 100644
--- a/internal/sheets/grid.go
+++ b/internal/sheets/grid.go
@@ -41,12 +41,15 @@ type gridCell struct {
// own edit arriving over the stream. Without it, the tab you typed in
// repaints the cell you are still editing.
type gridResponse struct {
- Version uint64 `json:"version"`
- Rows int `json:"rows"`
- Cols int `json:"cols"`
- Cells []gridCell `json:"cells"`
- Resync bool `json:"resync,omitempty"`
- Client string `json:"client,omitempty"`
+ Version uint64 `json:"version"`
+ // omitempty: a write response describes changed cells, not a
+ // window, and "rows":0,"cols":0 on every edit is a claim about a
+ // window nobody asked for.
+ Rows int `json:"rows,omitempty"`
+ Cols int `json:"cols,omitempty"`
+ Cells []gridCell `json:"cells"`
+ Resync bool `json:"resync,omitempty"`
+ Client string `json:"client,omitempty"`
}
// display turns a computed value into the string the grid paints.
diff --git a/internal/sheets/static/grid.js b/internal/sheets/static/grid.js
index 3406dda..d2c182b 100644
--- a/internal/sheets/static/grid.js
+++ b/internal/sheets/static/grid.js
@@ -126,6 +126,34 @@
var sheet = jspreadsheet(surface.querySelector('[data-sheet-grid]'), {
/* Our engine computes; the client paints. See the file comment. */
parseFormulas: false,
+
+ /* Every handler below is a SPREADSHEET-level option, not a
+ * worksheet one. Nested inside worksheets[0] they are accepted
+ * without complaint and never called — the grid renders, the
+ * console stays clean, and no edit is ever sent anywhere. That
+ * is how they were first written here, and only driving two real
+ * browser tabs found it. */
+ onchange: function (instance, cell, c, r, value) {
+ if (applying) return;
+ write(parseInt(r, 10), parseInt(c, 10), value === null ? '' : String(value));
+ },
+ onselection: function (instance, x1, y1) {
+ selection = { r: y1, c: x1 };
+ showFormula();
+ },
+ /* The in-cell editor should show the formula, not its result.
+ * The input does not exist yet when this fires, so it is filled
+ * on the next frame — best effort, because the formula bar is
+ * the reliable path and this is the convenience. */
+ oneditionstart: function (instance, td, c, r) {
+ var text = raw[key(parseInt(r, 10), parseInt(c, 10))];
+ if (!text) return;
+ requestAnimationFrame(function () {
+ var input = td.querySelector('input, textarea');
+ if (input) { input.value = text; input.select(); }
+ });
+ },
+
worksheets: [{
data: data,
minDimensions: [cols, rows],
@@ -133,27 +161,7 @@
columnSorting: false, /* sorting is a later slice; a half-done sort is worse than none */
columnDrag: false,
allowInsertRow: false,
- allowInsertColumn: false,
- onchange: function (instance, cell, c, r, value) {
- if (applying) return;
- write(parseInt(r, 10), parseInt(c, 10), value === null ? '' : String(value));
- },
- onselection: function (instance, x1, y1) {
- selection = { r: y1, c: x1 };
- showFormula();
- },
- /* The in-cell editor should show the formula, not its result.
- * The input does not exist yet when this fires, so it is filled
- * on the next frame — best effort, because the formula bar is
- * the reliable path and this is the convenience. */
- oneditionstart: function (instance, td, c, r) {
- var text = raw[key(parseInt(r, 10), parseInt(c, 10))];
- if (!text) return;
- requestAnimationFrame(function () {
- var input = td.querySelector('input, textarea');
- if (input) { input.value = text; input.select(); }
- });
- }
+ allowInsertColumn: false
}]
});
ws = sheet.worksheets ? sheet.worksheets[0] : sheet[0];
diff --git a/internal/sheets/static/sheets.css b/internal/sheets/static/sheets.css
index 70adf65..e44b522 100644
--- a/internal/sheets/static/sheets.css
+++ b/internal/sheets/static/sheets.css
@@ -29,13 +29,22 @@
[data-sheet-title] {
font-weight: 600;
- margin-inline-end: auto;
+}
+
+/* The formula bar takes the slack, up to a point. Full width pushes
+ * every control off to the right on a wide screen; a fixed width
+ * truncates a long formula on a narrow one. */
+[data-formula-input] {
+ flex: 1 1 12rem;
+ max-inline-size: 40rem;
+ font-family: var(--rst-font-mono, ui-monospace, monospace);
}
/* The live indicator. It says connected or reconnecting and nothing
* else — presence is a later slice, and a dot that claims more than it
* knows is worse than no dot. */
[data-sheet-live] {
+ margin-inline-start: auto;
font-size: var(--rst-fs-sm, 0.875rem);
color: var(--rst-text-mut);
}
diff --git a/internal/sheets/templates/sheet.html b/internal/sheets/templates/sheet.html
index d8b015c..0982227 100644
--- a/internal/sheets/templates/sheet.html
+++ b/internal/sheets/templates/sheet.html
@@ -31,7 +31,7 @@
shows its computed value, which is the difference between a
spreadsheet and a table of numbers. It is also the reliable way to
see a formula: the in-cell editor swap is best effort. */}}
-<label rst-sr-only for="formula">Selected cell</label>
+<label class="rst-sr-only" for="formula">Selected cell</label>
<input rst-input id="formula" data-formula-input type="text" autocomplete="off"
placeholder="Select a cell">
{{/* Every drag-only affordance has a menu equivalent (AGENTS.md).
diff --git a/tools/drive/drive.mjs b/tools/drive/drive.mjs
new file mode 100644
index 0000000..3946dfb
--- /dev/null
+++ b/tools/drive/drive.mjs
@@ -0,0 +1,152 @@
+/* Drive the grid in two real browser tabs.
+ *
+ * This is NOT part of `make test`, and deliberately so: it needs a
+ * browser and a running instance, neither of which CI has. It is a
+ * manual gate, run before a canary is called good.
+ *
+ * It earns its place. Two bugs got through the entire Go suite and
+ * showed up only here:
+ *
+ * 1. Jspreadsheet v5 takes its event handlers on the ROOT config,
+ * not on a worksheet. Nested one level too deep they are accepted
+ * in silence and never called: the grid rendered, the console was
+ * clean, and not one edit was ever sent to the server.
+ * 2. `rst-sr-only` written as an attribute instead of a class. It is
+ * one of the seven class-only utilities, so it matched nothing and
+ * a screen-reader label rendered in the middle of the toolbar.
+ *
+ * The second is now caught by a guard test. The first cannot be — no
+ * static check knows which options a third-party library reads.
+ *
+ * Usage, from the repository root:
+ *
+ * go build -o /tmp/sheets ./cmd/sheets
+ * SHEETS_ORIGIN=http://127.0.0.1:8799 \
+ * SHEETS_INSTANCE_KEY=dev SHEETS_OWNERS=you@example.com \
+ * SHEETS_DEK="$(head -c 32 /dev/urandom | base64)" \
+ * /tmp/sheets --db /tmp/s.db --socket /tmp/s.sock > /tmp/s.log 2>&1 &
+ * SHEETS_SOCK=/tmp/s.sock SHEETS_LOG=/tmp/s.log node tools/drive/drive.mjs
+ *
+ * It reads the magic link out of the log, which only works because a
+ * loopback origin with no SMTP logs links instead of mailing them —
+ * see internal/sheets/auth.go. A deployed instance never does this.
+ *
+ * PLAYWRIGHT is wherever playwright is installed; there is no
+ * package.json here, because this app has no build step and is not
+ * about to grow one for a manual script.
+ */
+import net from 'node:net';
+import fs from 'node:fs';
+const PLAYWRIGHT = process.env.PLAYWRIGHT || 'playwright';
+const { chromium } = await import(PLAYWRIGHT);
+
+const SOCK = process.env.SHEETS_SOCK || '/tmp/s.sock';
+const LOG = process.env.SHEETS_LOG || '/tmp/s.log';
+const PORT = parseInt(process.env.SHEETS_PORT || '8799', 10);
+
+// TCP -> unix socket, so the browser has a real origin to use. The app
+// speaks the platform's activation contract (a socket path, no $PORT),
+// so this stands in for the edge that terminates HTTP in production.
+const proxy = net.createServer((c) => {
+ const u = net.connect(SOCK);
+ c.pipe(u); u.pipe(c);
+ c.on('error', () => u.destroy());
+ u.on('error', () => c.destroy());
+});
+await new Promise((r) => proxy.listen(PORT, '127.0.0.1', r));
+
+const base = `http://127.0.0.1:${PORT}`;
+const browser = await chromium.launch();
+const ctx = await browser.newContext();
+const page = await ctx.newPage();
+
+const errors = [];
+ctx.on('weberror', (e) => errors.push('weberror: ' + e.error().message));
+page.on('console', (m) => { if (m.type() === 'error') errors.push('console: ' + m.text()); });
+page.on('pageerror', (e) => errors.push('pageerror: ' + e.message));
+
+// 1. Sign in, the way a person does.
+const before = fs.readFileSync(LOG, 'utf8').length;
+await page.goto(base + '/signin');
+await page.fill('#address', 'paul@example.com');
+await page.click('form[action="/signin"] button[type=submit]');
+await page.waitForURL(/signin\?sent=1/, { timeout: 15000 });
+
+const fresh = fs.readFileSync(LOG, 'utf8').slice(before);
+const m = fresh.match(/auth\/verify\?token=([A-Za-z0-9_-]+)/);
+if (!m) { console.log('FAIL: no magic link in the log'); process.exit(1); }
+await page.goto(`${base}/auth/verify?token=${m[1]}`);
+console.log('signed in as:', (await page.textContent('body')).includes('Signed in as') ? 'yes' : 'NO');
+
+// 2. Make a sheet.
+await page.goto(base + '/sheets');
+await page.fill('#title', 'Browser drive');
+// Not networkidle: the SSE stream is a long-lived connection, so the
+// network on this page is never idle by design.
+await page.click('form[action="/sheets"] button[type=submit]');
+await page.waitForURL(/\/sheets\/\d+$/, { timeout: 15000 });
+const url = page.url();
+console.log('sheet url:', url);
+
+// 3. The grid must actually render.
+await page.waitForSelector('.jexcel, .jss_worksheet, table', { timeout: 15000 });
+console.log('grid rendered:', await page.locator('[data-sheet-grid] table').count() > 0);
+await page.waitForFunction(
+ () => document.querySelector('[data-sheet-live]')?.dataset.state === 'on',
+ null, { timeout: 15000 }).catch(() => {});
+console.log('live indicator:', await page.getAttribute('[data-sheet-live]', 'data-state'),
+ JSON.stringify(await page.textContent('[data-sheet-live]')));
+
+// 4. A SECOND tab on the same sheet.
+const page2 = await ctx.newPage();
+page2.on('pageerror', (e) => errors.push('tab2 pageerror: ' + e.message));
+await page2.goto(url);
+await page2.waitForSelector('[data-sheet-grid] table', { timeout: 15000 });
+await page2.waitForFunction(
+ () => document.querySelector('[data-sheet-live]')?.dataset.state === 'on',
+ null, { timeout: 15000 }).catch(() => {});
+console.log('tab2 live:', await page2.getAttribute('[data-sheet-live]', 'data-state'));
+
+// 5. Type into tab 1.
+const cellA1 = page.locator('[data-sheet-grid] td[data-x="0"][data-y="0"]');
+await cellA1.dblclick();
+await page.keyboard.type('6');
+await page.keyboard.press('Enter');
+
+const cellA2 = page.locator('[data-sheet-grid] td[data-x="0"][data-y="1"]');
+await cellA2.dblclick();
+await page.keyboard.type('=A1*7');
+await page.keyboard.press('Enter');
+await page.waitForTimeout(1500);
+
+console.log('tab1 A1:', JSON.stringify(await cellA1.textContent()));
+console.log('tab1 A2:', JSON.stringify(await cellA2.textContent()));
+
+// 6. THE TEST: does tab 2 show it without a reload?
+const t2a1 = page2.locator('[data-sheet-grid] td[data-x="0"][data-y="0"]');
+const t2a2 = page2.locator('[data-sheet-grid] td[data-x="0"][data-y="1"]');
+let ok = false;
+for (let i = 0; i < 40; i++) {
+ const a1 = (await t2a1.textContent())?.trim();
+ const a2 = (await t2a2.textContent())?.trim();
+ if (a1 === '6' && a2 === '42') { ok = true; break; }
+ await page2.waitForTimeout(250);
+}
+console.log('tab2 A1:', JSON.stringify((await t2a1.textContent())?.trim()));
+console.log('tab2 A2:', JSON.stringify((await t2a2.textContent())?.trim()));
+console.log(ok ? 'LIVE-ACROSS-TABS: YES' : 'LIVE-ACROSS-TABS: NO');
+
+// 7. The formula bar shows the input, not the result.
+await t2a2.click();
+await page2.waitForTimeout(300);
+console.log('tab2 formula bar for A2:', JSON.stringify(await page2.inputValue('[data-formula-input]')));
+
+const shots = process.env.SHEETS_SHOTS;
+if (shots) {
+ await page.screenshot({ path: shots + '/tab1.png' });
+ await page2.screenshot({ path: shots + '/tab2.png' });
+}
+
+console.log('js errors:', errors.length ? JSON.stringify(errors.slice(0, 8), null, 1) : 'none');
+await browser.close();
+proxy.close();