oficina / sheets Public

<task-notification> <task-id>ab699c25459d68e8f</task-id> <tool-use-id>toolu_01BcrZGQPCPKNJTd1zZvRirN</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-sheets-core/b0c2ce68-caa9-4b4c-b618-0f3f336f01cb/tasks/ab699c25459d68e8f.output</output-file> <status>completed</status> <summary>Agent "Fable branch review" finished</summary> <note>A task-notification fires each time this agent stops with no live background children of its own. The user can send it another message and resume it, so the same task-id may notify more than once.</note> <result>Review complete. I read AGENTS.md, the supplied diff, the final tree, the rastrillo framework CSS the claims depend on, and ran the full test suite. One important note first: **the supplied review.diff was stale** — the worktree was mid-rebase of `design-fixes` onto current `origin/core` when I started, and the rebase completed during review. The final commit (f2fb2f8, now one commit atop origin/core b79c311) differs from review.diff in two ways: `link.html` (new on core) gained the `data-kind` hunk with `&amp;nbsp;` deliberately retained, and the alignment CSS became a two-selector rule covering both table variants. I reviewed the final tree, all tests green (`go test ./internal/guardstest/ ./internal/sheets/...`). ## Findings 1. **should-fix — `internal/sheets/templates/share.html:54` and `internal/sheets/templates/history.html:18` — the defect class fix #3 removes is still live on two screens.** Both render prose as `&lt;p rst-help&gt;…&lt;/p&gt;`; tokens.css:1245 defines `rst-help` as a fixed 28x28px inline-flex badge (`height: 28px; width: 28px; border; border-radius: 7px`). By the branch's own diagnosis for signin.html, these two paragraphs render one-to-two words per line with the badge border struck through them. They arrived on core after `design-fixes` was cut, so the branch didn't introduce them — but post-rebase they are on this branch's final tree, and a branch titled "the four design defects" that fixes one of three instances of a defect while the rationale comment names the exact failure is half-done. Neither is inside an `rst-field`, so the fix is not mechanical `rst-field-help`; they likely want a plain muted paragraph or an appropriate prose token. The recurrence within days also argues for a guard test on `&lt;p rst-help&gt;`/`rst-help` wrapping text content — the branch guards two of its four fixes; this defect class demonstrably regenerates and has no guard. 2. **should-fix — `internal/sheets/templates/table.html:10-11` — the rewritten subtitle asserts a falsehood to viewers and commenters.** "Every cell here is editable" is shown unconditionally, but `handleTable` gates on `sheetFromURL` (read access, any role), while `handleTableWrite` (handlers_table.go:130) 404s a viewer or commenter — the code comment there says so explicitly. A viewer gets the sentence, the per-cell edit anchors, the `?edit=` form, types a value, submits, and gets a 404. This was true of the old subtitle too, but the branch rewrote exactly this sentence on the argument that a subtitle must not "argue the opposite" of the truth — and kept a claim that is false for two of the four roles. `historyView` already models `MayWrite`; `tableView` should too, gating the sentence (and ideally the anchors/edit form, though that is a larger change). 3. **should-fix — `internal/sheets/handlers_share_test.go:151-174` — the markup test has two holes.** (a) It anchors on the *first* `name="role"` in the whole body, and there are two selects with `name="role"` on that page (the person-share select `id="role"` and the link select `id="link-role"`, share.html:23 and :62). Today the person form renders first; if the forms are ever reordered the test silently retargets the link select — whose first option is *also* "viewer" (`linkRoles`, sharelink.go:29) — and keeps passing while the person-select default regresses to owner. Anchor on `id="role"` instead. (b) Neither test catches the other way the default moves: an `&lt;option … selected&gt;` added to a later option overrides first-option default; the test's own comment claims it catches the bug "however the slice is ordered", and it doesn't for that vector. Add an assertion that the select's markup contains no `selected`. The slice-order unit test itself (line 135) is genuinely load-bearing against the realistic regression (a tidy-looking reorder). 4. **nit — `internal/guardstest/shellmenu_test.go:47,58` — the guard fails closed but is brittle.** The literal `&lt;div rst-shell-tail&gt;` match breaks if any attribute is ever added to the tail div (test fatals — a false failure, not a silent pass, so acceptable); `strings.Contains(between, "&lt;")` would flag an HTML comment between menu and tail, which does *not* break the `+` combinator (comments aren't elements); and the comment-strip regex misses trim-marker comments (`{{- /* … */ -}}`), though the gap assertion still catches actual removal. All error toward failing, so no action required — noting for when it fires spuriously. 5. **nit — `internal/sheets/static/sheets.css:212` — the comment overstates "the anchor still fills the cell".** `display:block; min-block-size:1.25rem` fills the width and one line of height. In a row made taller by a wrapped neighbour cell (`vertical-align: top` on tds), the lower portion of other cells is dead space, not click target. Not a regression versus `&amp;nbsp;` (which had the same geometry), so the *fix* is fine; the comment's claim is not quite. 6. **nit — `internal/sheets/handlers_table.go:257` — `KindErr` and `KindBool` left-align.** Spreadsheet convention (and Excel) centres `TRUE`/`#DIV/0!`; this view left-aligns them. `KindNum` as the sole right-align signal is otherwise correct, and critically **safe**: `KindEmpty` is the iota-zero of `ValueKind` (calc/eval.go:17), so blank cells can never read as numeric. A number formatted as text evaluates as `KindStr` and left-aligns, matching convention. Acceptable as shipped. ## Claims verified correct (I looked for these to fail and they don't) - **The `&lt;details&gt;` grouping reasoning is right, and it's the framework's own documented rule.** tokens.css:1391-1397 says verbatim that `&lt;details name&gt;` exclusivity is document-wide and a shell menu sharing `rst-menus` "would be closed by the very account menu it reveals". Omitting `name` is correct. - **No stranding on resize across 800px.** The ≥800px block (tokens.css:1420-1434) hides the menu *and* re-declares `[rst-shell-menu][open] + [rst-shell-tail] { display: contents }` at matching specificity, explicitly so an open menu can't hold the tail in column layout when widened. Closing the menu with the account dropdown open merely hides everything below 800px; reopening restores it. No bad state found. - **CSS tokens exist and the colour pair passes AA in both themes.** `--rst-radius-sm` (theme.css:138), `--rst-accent` (:161), `--rst-accent-soft` (:163, `light-dark()` pairs). `--rst-accent` on `--rst-accent-soft` is in rastrillo's audited contrast table at 4.75:1 light / 6.59:1 dark (theme.css:59,87) — it is the theme's documented tightest passing pair. - **Specificity is fine.** rastrillo has no bare-`a` rule at all (every anchor rule is component-scoped), so browser defaults were the only competition; `[data-cell-table] td &gt; a` (0,1,2) wins, and `td[data-kind="number"]` (0,2,1) beats the base td's `text-align: start` (0,1,1). `text-align: end` on the block anchor works (it's a block container). The focus ring is not suppressed — `:focus-visible` adds background on top of rastrillo's outline. - **Empty-cell anchor is clickable and focusable** (href present; full-width block, 20px min height, inside the pre-existing `overflow-x:auto` wrapper at table.html:18), and removing `&amp;nbsp;` changes nothing for AT — `aria-label="Edit {{.Ref}}"` already supplied the accessible name. - **`data-kind` leaks nothing.** The same markup already carries `{{.Display}}` and the full raw input in `title="{{.Raw}}"` to the same authorized viewer; one derived bit adds no disclosure, and no log/error/metric is involved. - **The link view works.** `handleLinkView` reuses `a.tableRows` (handlers_link.go:95), so `link.html`'s new `{{if .Number}}` resolves; keeping `&amp;nbsp;` there (no anchor to hold the cell open) is correct. - **Nothing else broken.** Share grant/revoke validation untouched (`role.Valid()` still gates the POST); table write round-trip unchanged; no JS added (scriptless guarantee intact); no migrations, scoping, or binding rule implicated; attribute grammar used consistently per screen. **Verdict: REQUEST CHANGES** — findings 1 and 2 are cheap, squarely within the branch's own stated purpose, and finding 3 closes real holes in a test whose comments claim more than it enforces. The four fixes themselves are sound and their mechanisms verified.</result> <usage><subagent_tokens>88505</subagent_tokens><tool_uses>29</tool_uses><duration_ms>413399</duration_ms></usage> </task-notification>