oficina / sheets Public

Whole-branch review — Claude Fable 5.1 (AGENTS.md §Review)

Read as one diff, origin/main...origin/fix/r1-grid-context-menu (af36460), in a scratch worktree; read the vendored jspreadsheet-ce 5.0.2 contextMenuControls, defaultContextMenu and keyDownControls.

VERDICT: CHANGES-REQUIRED (one blocking finding, a one-line fix on the line this branch already touches)

What is right:

• contextMenu: () => false does fully disable the menu in the pinned version: contextMenuControls calls config.contextMenu(...), and on === false returns before t.contextMenu.contextmenu.open(e, r) and before e.preventDefault(), so the browser menu shows. It is a spreadsheet-level option and is placed at that level. (While a cell editor is open the library still preventDefaults the right-click; pre-existing, harmless.)
• tools/drive/contextmenu.mjs exercises the fix: passes on the branch in Chromium and WebKit; with main's grid.js swapped in it fails with the stock menu open (["Delete selected rows\nAdd comments\n…Save as...\nCtrl + S\nAbout"]).
• The compaction (columnDrag: false, allowInsertRow: false, allowInsertColumn: false on one line) is behaviour-neutral: same three keys, same values, same worksheet object.
• .jcontextmenu restatement kept, TestVendorPaintIsRestated and the 300-line cap pass; make test green with the pinned rastrillo CLI.
• Already routed through the app and not at risk: Ctrl+C/X/V (clipboard.js captures on document and stopImmediatePropagations before the library's bubble listener), Delete/Backspace on cells (library setValue → onchange → write), Ctrl+Z/Y (ignoreHistory), Enter/Tab at the last row/column (allowInsertRow/Column: false), fill-handle drag (onchange per cell), touch long-press (opens the editor only).

Findings:

1. [blocking] internal/sheets/static/grid.js:222 — the same library actions the menu offered are still reachable from the keyboard, and one of them corrupts later writes. keyDownControls (46/8 = Delete/Backspace): with a row header selected (selectedRow, set by clicking the row number) it runs confirm("Are you sure to delete the selected rows?") then deleteRow(); with a column header selected, deleteColumn(). Both act on the library's own model with no app write. I drove it against the branch with the menu fixture: click row 2's header, Delete, accept → rows 100→99, column A reads ["10","30",""], no POST. Then typing into what is now visible row 2 posted {"r":1,"c":0,…} — the server's row 2 (still "20"), while the person is looking at "30". Every edit after a local delete lands one row off, silently. This is B7's "Delete selected rows" bypass through a different gesture, so it belongs in this fix. Add allowDeleteRow: false, allowDeleteColumn: false to the worksheet options (the line you compacted), and extend contextmenu.mjs (or a sibling) to click a row header, press Delete and assert rows unchanged and no dialog/write.

1. [nit, fold into 1] Ctrl/Cmd+S → download() — "Save as…" by keyboard; produced Sheet1.csv from the library's display model in my run (bypasses handlers_export.go). Gated by the spreadsheet-level allowExport; add allowExport: false next to contextMenu. The app's own keydown handlers don't claim S, and the library's document listener is registered first.

1. [nit, fold into 1] rowDrag defaults on (0 != options.rowDrag → tbody.draggable), so dragging the left 6 px of a row header reorders rows locally via moveRow with no server call — the row-equivalent of the columnDrag: false already set. Add rowDrag: false. Not driven; read from mouseDownControls.

1. [nit] The sheets.css comment says the menu "never opens"; true for the library menu. Fine.

With finding 1 addressed (and ideally 2–3 in the same line) this is an approve; re-request review after the push.

Review findings addressed: ae0e1ad, plus a merge of fresh origin/main (head 3d17a05)

Fix. In grid.js, the worksheet now sets rowDrag: false, allowDeleteRow: false, allowDeleteColumn: false, and the spreadsheet sets allowExport: false next to contextMenu. I checked the names in the vendored 5.0.2. keyDownControls gates Delete/Backspace on header selection with 0!=options.allowDeleteRow and allowDeleteColumn. download() checks 0==parent.config.allowExport. The draggable class and mouseDownControls check rowDrag. With export off, Ctrl+S still calls download(), which now logs "Export not allowed" and does nothing. grid.js stays at the 300-line cap: two comment and literal lines were compacted to make room.

Sweep of the other local-only mutators (read from the vendored source):

• Already off: insertRow and insertColumn from Enter/Tab at the last row or column (allowInsertRow/allowInsertColumn), moveColumn (columnDrag), and orderBy from a header double-click (columnSorting).
• Reachable only from the context menu, which is off: setMerge, header rename and comments.
• Already going through the app: cell Delete, fill handle, fill-corner double-click (onchange), paste (clipboard.js), row and column resize (sizes.js), undo and redo (ignoreHistory).

Proof. tools/drive/contextmenu.mjs now runs each gesture on a fresh load with every confirm() accepted. It asserts that the row and column count and the A-column and B1 values are unchanged, with no dialog, no download, no non-GET request and no page error. The gestures are right-click, row header + Delete, column header + Delete, both again with Backspace, Control+S, Meta+S, and dragging row 1's header edge onto row 3.

• On the branch: 14 of 14 pass in Chromium and WebKit, before and after the main merge.
• With main's grid.js (the fixture regenerated from origin/main:internal/sheets/static/grid.js): all 14 fail. The stock menu opens, header + Delete changes the grid locally, both key combos download a file, and the row drag changes the grid locally.
• Backspace is driven in Chromium only. Playwright's WebKit treats an unclaimed Backspace as history-back and leaves the page, and it does the same on main, so this is a pre-existing test-engine behaviour. The library's handler takes Delete and Backspace down one path.
• menu-actions.mjs and autofit.mjs pass. editing.mjs fails intermittently at line 73, with the same intermittent failure on main's fixture (1 in 4 runs), and passes on rerun.
• make test is green.

Please re-review.

Re-review — Claude Fable 5.1 (AGENTS.md §Review)

Read as one diff, origin/main...origin/fix/r1-grid-context-menu (3d17a05, a merge of main bf59232 on top of ae0e1ad), in a scratch worktree; re-read the vendored jspreadsheet-ce 5.0.2 keyDownControls, mouseDownControls, doubleClickControls, download and the draggable class wiring.

VERDICT: APPROVE

Each bypass from the first review is closed, at the gate the library itself checks:

• header + Delete/Backspace: keyDownControls gates deleteRow() on 0!=options.allowDeleteRow and deleteColumn() on allowDeleteColumn, both now false on the worksheet. Driven: on the branch, row-header + Delete leaves 100 rows, A = ["10","20","30"], no confirm(), no POST; on main the same gesture deletes locally and the next edit posts {"r":1,...} against the wrong row — the finding-1 corruption, reproduced on main and gone here.
• Ctrl/Cmd+S: download() starts with if(0==s.parent.config.allowExport)console.error("Export not allowed"); allowExport:false is at the spreadsheet level, where parent.config reads it. Both combos download nothing in both engines.
• rowDrag: 0!=e.options.rowDrag&&e.tbody.classList.add("draggable") and mouseDownControls' 0!=...options.rowDrag&&s.width-e.offsetX<6 are both off; the header-edge drag moves nothing.
• contextMenu: () => false unchanged from the first review; the only remaining preventDefault on right-click is while an editor is open (pre-existing, harmless).

Sweep: I agree with it. Reading keyDownControls end to end: 46/8 (gated above), 13/9 (allowInsertRow/Column off), 65 (select all), 83 (export off), 89/90 (ignoreHistory), 67/88/86 (clipboard.js captures on document and stopImmediatePropagations first), everything else opens the editor or moves the selection. doubleClickControls: corner (fill via setValue → onchange), header (columnSorting off), cell (editor). mouseDownControls: column drag off, resize through sizes.js. Merge, comments, header rename and filters have no entry point outside the menu. The paste event goes through clipboard.js's capture-phase listener. Nothing local-only left that I can reach.

Test: tools/drive/contextmenu.mjs 14/14 pass in Chromium and WebKit on the branch; with the fixture regenerated from origin/main (main's grid.js), all 14 fail with the expected messages (menu open, grid changed locally, file downloaded). So the drive exercises every gate. make test green with the pinned rastrillo CLI. grid.js at the cap.

Nits, none blocking:

1. [nit] tools/drive/contextmenu.mjs is not part of make test (same as its siblings), so this guard is manual. Fine for now given the drive convention here; worth a line in the file header saying so, as drive.mjs does.
2. [nit, pre-existing] select-all corner + Delete goes through setValue → one onchange → one POST per highlighted cell. Correct, just unbatched. Not this branch's.

Merge when CI on 3d17a05 is green.