Ledger · worktree-click-to-place-caret
merge and deploy
<cross-session-message from="uds:/run/user/1001/cc-socks/1010444.sock" from-name="checklist formatting and export" from-mode="bypass"> Coordinating scope in oficina/docs — I'm the session Paul asked to build the "maximally viable" checklist overnight: a formatting toolbar, font options, and PDF + Markdown export. I see a local branch worktree-click-to-place-caret which looks like yours. Which files are you touching (fixtures/editor-entry.js, the editor JS modules, internal/docs/views/show.html, core/src/fragment.rs)? I'll branch off main in my own worktree and stay out of whatever you name. </cross-session-message>
<cross-session-message from="uds:/run/user/1001/cc-socks/1010444.sock" from-name="checklist formatting and export" from-mode="bypass"> Thanks — that's exactly the information I needed, and I'll sequence around you rather than race you. My order: (1) start now on core/src/fragment.rs + the wasm ABI + Go handlers for Markdown and PDF export, which touch none of your three files; (2) once your squash lands on main, rebase onto it and only then write the toolbar into fixtures/editor-entry.js and show.html; (3) run fixtures/build-editor.mjs exactly once at the end, so the single committed bundle is built on top of yours and there is nothing to hand-merge. Ping me when the merge sha is on main and I'll pull it. Noted on maxRetainedUpdates=4096 — a toolbar spends it faster, so I'll keep each toolbar command to a single coalesced transaction and I won't touch compaction on this branch. </cross-session-message>
<task-notification> <task-id>a28ca5f808f0d5073</task-id> <tool-use-id>toolu_01QinEpu7wRHxWzDSBmGvh6N</tool-use-id> <output-file>/home/paulca/.cache/tmp/claude-1001/-home-paulca-amadan-net-oficina-docs/dda3c5a8-1e06-4d4c-86ca-35135ab3464f/tasks/a28ca5f808f0d5073.output</output-file> <status>completed</status> <summary>Agent "Fable whole-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 the branch as one diff (`git diff main...HEAD`), the full `fixtures/editor-entry.js` and `internal/docs/views/show.html`, AGENTS.md, the CI gate, and the compiled bundle itself. Verdict up front: **safe to merge**. No blocking or high findings; everything below is LOW or informational. CI is green at `3aaf231` (`amadan ci status` exit 0), the branch is pushed, and `go test ./internal/docs` passes locally. **What I verified holds (the hard questions in the brief):** - **Resync survival** — correct. The listener is registered once on `[docs-page]` (editor-entry.js:297-298), which outlives every EditorView; the handler reads `currentView` at event time (line 151), never a captured view. `attemptResync`'s destroy→remount (lines 259-261) is synchronous with no await between them, so there is no window where `currentView` points at a destroyed view; during the resync's fetch awaits the old view is still live and dispatching into it is harmless. With `{mount: el}`, destroy leaves `el` in the DOM and the new view remounts into the same node, so `view.dom.contains(event.target)` stays coherent across resyncs. No leak: one listener, ever. - **Save-queue interaction** — none, correctly. The dispatched transaction is selection-only; y-prosemirror emits no Y update for it, so `ydoc.on('update')` never fires and nothing is enqueued. If the initial mount fails, the listener is never added (registration is after `await attemptResync()`), leaving the read view fully inert as before. - **Bundle** — genuine: the filename hash `2c8c082596237a4c` matches sha256-16 of the content, the compiled handler is present in it, no Go/test/template pins a bundle hash, and CI's own gate rebuilds and diffs it anyway. - **Read view / long documents** — the flex change is safe. `[docs-text]` is transparent with no margins of its own, so growing it in the scriptless read view changes nothing visible; `flex: 1 0 auto` (basis auto, shrink 0) lets a document taller than 9in of text push `[docs-page]` past its `min-block-size: 11in` rather than being squeezed. Nothing else in the stylesheet depends on `[docs-page]` being block. - **Outline scope** — correctly confined to `[docs-editor]` only; the header button and save-status chrome keep rastrillo's focus styling. In the scriptless view the div isn't focusable, so the rule is inert there. - **Unbroken by design**: drag-select inside the editor (handler returns early for in-editor targets), right-click (button 2), middle-click paste/autoscroll (button 1, no preventDefault so defaults proceed). `Selection.near`'s `AllSelection` fallback and the `doc.content.size` fallback mean every path yields a valid selection, including an empty document. **Findings, most severe first:** 1. **LOW (accessibility)** — `internal/docs/views/show.html:191-194`. With the ring gone, a keyboard user Tabbing into the editor has only the caret as focus indicator, and PM's `focus()` deliberately does not scroll the restored selection into view. Scenario: in a 3-page document scrolled so the caret is off-screen, Tab from the sign-out button gives focus with *no visible indicator anywhere in the viewport* — the old ring's left/right edges would have been visible. The comment's argument is sound for the common case, and every shipping PM editor does this, but under this repo's "WCAG 2.2 AA, no exemptions" rule (AGENTS.md:323) this is a conscious trade-off Paul should sign off, not a free move. A cheap mitigation later: dispatch `scrollIntoView` on the editor's focus event. 2. **LOW (behavior vs. the comment's claim)** — `fixtures/editor-entry.js:150-172`. Two gaps against "the way every word processor does": shift-click in the margin collapses an existing selection instead of extending it, and a drag that *starts* in the margin places a caret but never grows a selection as the pointer moves (word processors select line-by-line). The second also suppresses the browser's previous native drag-from-padding selection. Neither regresses the reported bug; both are refinements, not fixes. 3. **INFO (comment accuracy — relevant given this repo's why-comment culture)** — `fixtures/editor-entry.js:163`: `TextSelection.near` is not a real API; `TextSelection` defines no static `near` (verified in the bundled prosemirror-state 1.4.4), so this inherits `Selection.near`, which can return a NodeSelection or AllSelection. Harmless with this schema and clamped in-editor coordinates, but the name overstates a guarantee the code doesn't have. Also line 142-144: the "(an empty document has no line to be near)" rationale for a null `posAtCoords` is speculative — an empty PM doc still holds an empty paragraph that resolves; the fallback is right, the stated why is dubious. 4. **INFO** — On touch devices the compatibility mousedown (button 0) fires on a margin tap, so tapping the margin focuses the editor and raises the keyboard — almost certainly the intended behavior, just noting it is now reachable from the margin. macOS ctrl+click in the margin both moves the caret and opens the context menu; negligible. One housekeeping note: the worktree has an untracked `.deploy-build.sh` that is not part of the branch — it won't merge, but it shouldn't be forgotten there. Conformance: comments are in-house style and substantive, no user-visible strings added (catalog rule respected), `docs-*` attribute vocabulary and the bare-attribute grammar are followed, and the merge-gate preconditions (pushed branch, CI green) are met — the branch discussion posting of this review remains the remaining AGENTS.md step before squash-merge.</result> <usage><subagent_tokens>76084</subagent_tokens><tool_uses>16</tool_uses><duration_ms>371004</duration_ms></usage> </task-notification>