oficina / sheets Public

§Review pass — REQUEST CHANGES, and the response

Reviewed by Fable, which did not write this code (AGENTS.md §Review: "not the model that happened to write it"). Verdict was REQUEST CHANGES on one blocker and eleven findings under it. All of the blocker and ten of the eleven are fixed in b6853b6; two findings are accepted and deliberately not fixed, below.

The blocker, and why our own gate missed it

Alignment did nothing. Jspreadsheet writes text-align as an INLINE style on every data cell. An inline declaration outranks any selector at any specificity, so the three toolbar buttons and the Format submenu set their attribute, reflected aria-pressed, and moved no text at all. The control looked like it worked.

chrome.mjs did not catch it because it checked bold and fill — both of which paint properties the library never touches. That is the third defect tonight of one shape: a probe that returns the same answer whether or not the thing works. The other two were a CSS rule that had matched no element for the life of core (and was hiding a second defect behind it — the warning flag it should have drawn was itself invisible on the very fills it flags), and a menu bar that tested perfectly from the keyboard and did not work with a mouse at all. The drive now asserts the COMPUTED alignment, because the attribute was always right.

Two things fell out of fixing it, and both are improvements the review did not ask for:

• The grid centred everything. Meanwhile formatpatch.go carried a comment saying the engine right-aligns numbers and left-aligns text — true of the table view and false here. The server now sends whether a cell's COMPUTED value is a number (only the engine knows: =1+1 is a number, 1a is text) and the stylesheet right-aligns those. A spreadsheet that centres its numbers does not look like one, and every screenshot on this branch until now had that defect in plain sight.
• defaultColAlign is a WORKSHEET option and I had set it at spreadsheet level, where it is accepted and never read — the exact mirror of the event handlers, which are spreadsheet-level and silently ignored inside a worksheet. Two options, two levels, both failing identically and silently. Found by the drive, not by reading.

Also fixed

• The anchor cursor vanished on filled cells. The rule marking it carries a comment about coming last so the cursor survives a tie; the new fill rules land later at the same specificity, so the failure that comment guards against arrived by a new route. Now an inset ring, drawn over any fill, no layout cost.
• Delete wrote every cell twice. The library already clears a selection through its own setValue, which reaches write(). Backspace — which the library also handles — went once. Inconsistent either way.
• The number format was 128 bytes of free text, stored verbatim. An Excel pattern admits quoted literals ("Overdue: "0), so this was an arbitrary plaintext write into the formats table through a public API, in the family §2.1 calls "user data living inside a format-adjacent record". It now validates against the five patterns the UI offers, and the UI renders from that same Go list, so the two cannot drift. The importer is deliberately not bound by it: a file's own custom format is the document's, retained as authored the same way its fills are.
• Resize is drag-only and AGENTS.md requires a menu equivalent. The Format menu now carries column-width and row-height fields. Real form controls rather than a prompt(), which is unstyleable, untranslatable and blocked outright in some embedding contexts.
• The toolbar's tooltips never rendered. rastrillo's is [rst-tip]::after { content: attr(data-tip) } and only data-tip was written — so the sheets.css comment about positioning the host for that pseudo-element described one that never matched. Another rule with no element.
• A plain viewer got an empty Insert menu. The list row lost its thumbnail column below 800px, where rastrillo sets grid-template-columns directly rather than through --rst-cols. A toolbar dropdown near the right edge overflowed a 320px viewport. A colour picked from a toolbar menu dropped focus to <body>.
• Export sends RFC 6266's two-parameter Content-Disposition. The ASCII form drops every non-ASCII rune, so a suite called Oficina would have mangled the first Spanish title anybody exported.
• schema.sql was not regenerated for migration 0008 — I hand-wrote the migration rather than using rastrillo migration new, and the check reads the migrations, not the summary.
• The colour-literal guard did not know about oklch(), which this branch introduced. A literal oklch(0.92 0.08 145) would have passed the test whose entire job is to catch one.
• Nothing exercised WRITING an ink: migration 0008's columns and applyInk were reachable only by clearing them.
• Deterministic export ordering, dead code removed, three comments that had stopped being true.

Accepted and NOT fixed

Both want a change to the store's locking, which is the most delicate code in the repo, and neither is worth doing at the end of a long session:

1. applyFormat's read-modify-write is not under the handle lock. It reads every format once, then merges per cell. Two concurrent patches to the same cell can therefore lose each other's fields, and a value edit racing a format write can publish the cell with the old format key after the format frame — the client then shows the older format until a reload. Same family as last-write-wins, which this design already accepts for values. The fix is a PatchFormat(rowID, colID, func(Format) Format) applied under the lock.
2. A max-size patch is roughly 12,000 statements in autocommit — internFormat's digest lookup plus assignFormat's read and write, per cell, 4,000 cells. A range usually resolves to one or two distinct formats, so memoising the interned id per distinct result and wrapping the loop in one scoped tx would take it to about three.

Also standing, from the review's own CONSIDER list and worth knowing: a LibreOffice file writes <col min="1" max="1025"> spanning the whole sheet, so the import's default-width probe lands inside it and a uniformly widened LibreOffice file imports with no widths. That is the safe direction to be wrong in — nothing invented, our own default used — and the alternative is coupling to a library version rather than to a format. Named in the code rather than papered over.

State

b6853b6, ten commits. CI green including -race. tools/drive/chrome.mjs — 29 checks green in a real browser, now including alignment, the numeric default, the anchor on a filled cell, and the whole read-only viewer page. Not yet deployed to a canary; that is the remaining gate before this can merge, along with a human's approval.

Two guards added after the review, prompted by the Docs session shipping the same shape and losing four menu items to it: data-cmd in the markup and the dispatch table in commands.js are two halves in two files with nothing tying them, so internal/guardstest/commands_test.go now ties them in both directions — every command in the markup is implemented, every implementation is reachable, and a third guard asserts the regex that reads the table still finds names it must find, because the first two are vacuous the moment it stops matching. All three mutation-checked.

Two of their warnings were checked and do not apply: Sheets uses no position: sticky at all, so the four chrome strips cannot collapse into a shared sticky band; and rastrillo already bounds [rst-dropdown-menu] with max-block-size and overflow-y — measured at a 420px-tall viewport, not read off a grep.

Also re-ran the dead-hook audit after the review's fixes added attributes: 48 hooks in sheets.css, all live.

c47aeea. Still not deployed to a canary — that and a human's approval are the two gates left.

Merged and deployed — d7cea4f is live

Paul approved, which was the gate this was waiting on. core fast-forwarded onto the branch and deployed to https://sheets.bds.oncarlos.com.

core had moved, and the rebase was not a formality

Four commits landed on core while this branch was in flight — 20977af, a0ece74, 8ca7d2c, 4af9e0d — all of them about grants, sheet visibility and the import lifecycle, all of them in files this branch also touched. Deploying without rebasing would have silently reverted every one, and nothing in the deploy path warns about that.

The rebase applied with no textual conflicts, which is precisely when to look harder. Two things needed hands:

1. Two migrations numbered 0008. core added 0008_alter_sheets.sql; this branch had 0008_alter_formats_ink_intent.sql. Git merged them cleanly because they are different filenames touching no common line, and rastrillo migration check reported "models and migrations agree" with both present. migrate.Apply is not harmed — its ledger keys on the ID — but the number stops meaning "when", and the order between them falls out of filename sort rather than out of anyone's intent. Renumbered to 0009, which was free only because this branch had never been deployed: apply.go's own error says migrations are immutable once applied. The window in which this is cheap to fix is exactly the window in which nothing tells you about it. Filed upstream as rastrillo/rastrillo discussion 13, asking that migration check refuse a duplicate number.
2. A comment here named createSheet, which core renamed to newSheet when it taught that function about half-imported sheets.

Checked and clean: visibleSheets now excludes filling_since IS NOT NULL, so the new list thumbnails never see a mid-import sheet — no interaction between the two branches' work.

Verification, on the rebased tree

• Full CI on core itself after the fast-forward, not on the branch before it: vet, fmt, build, migrations, go test ./..., and -race. All pass.
• tools/drive/chrome.mjs — 29 checks green in a real browser on the rebased build.
• An XLSX import driven end to end, because core had rewritten that lifecycle: column widths arrive (159px, 187px from the file), the three low-contrast cells are flagged, no console errors.

The deploy

make release stamped d7cea4f. carlos deploy --app sheets --account bds --channel edge.

The promote landed immediately and the process did not cycle for 128 seconds — during which /api/version still reported 4af9e0d while X-Carlos-Version had already flipped. Polled rather than sampled, exactly as this repo's notes say to; a single early read would have looked like a failed deploy and a single read of the header would have looked like a successful one.

Verified live three ways, none of which is a 200:

• GET /api/version → d7cea4f — the process, not the router's adopted release.
• /healthz, /, /signin all 200.
• The served stylesheet carries rules that exist only on this branch: --sheet-fill-l (6), [data-menubar] (12), [data-a="right"], [data-preview-slot], [data-toolbar], [data-size-form], and the contrast flag's drop-shadow halo. That proves the new embedded FS is live and not merely that something answered.

Still true, and still owed

The two findings accepted-not-fixed above stand: applyFormat's read-modify-write is not under the handle lock, and a max-size patch is ~12,000 statements in autocommit. Both want a change to the store's locking. So does the spec's §14 debt — formatting is not reachable from the scriptless table view, and neither a format nor a resize is undoable.

Closing this: the branch is merged, deployed and verified. New work gets its own thread.