fix(image): carry the caret offset in a WeakMap, not a DOM stamp - #356
Merged
Conversation
ImageBlockWidget dispatched its click caret from `Number(root.dataset.docFrom)` with no validation. CodeMirror does not backstop that: `checkSelection` only tests `range.to > doc.length`, so "", "-5", "78.5" and "abc" each install a silently broken selection that no try/catch can observe. The stamp existed because `updateDOM` reuses the element across widget instances and cannot re-bind the click listener, so the new instance needs a channel to reach the old closure. That channel does not have to be the DOM: a module-private WeakMap<HTMLElement, number> has the same per-element, updateDOM-writable lifetime — the pattern `pendingDrag` already uses in table-widget.ts — and keeps the value a number end to end, so there is no malformed state left to validate. Gating the DOM read instead would have meant sharing the table's `stampedOffset` from a new neutral module, which the agreed end-state (both widgets off root stamps) then orphans. `data-doc-from` is still written for DOM inspection but never read back. Also guard the dispatch itself, as the table does: with the anchor now a number, an offset that outlived a shrinking edit, CodeMirror's re-entrancy error and a throwing transaction filter can still throw, and the last two were unguarded here regardless of channel. Behaviour is unchanged in every reachable state — nothing outside Quoll writes that attribute — so this buys an unrepresentable failure state, not an incident fix.
… blockStart miss - test/webview/image/cm-image-widget.test.ts: the comment above the throwing-dispatch test claimed a DOM listener's throw is "reported, not re-thrown to the click() caller" — false in this vitest+happy-dom suite, where the throw propagates synchronously out of click(). Corrected the comment and clarified why console.error is still the stronger assertion (it also catches a mutation that empties the catch body). - src/webview/cm/image/image-widget.ts: the click listener's `blockStart.get(root) ?? this.docFrom` fallback silently trusted an invariant (the WeakMap entry is always stamped before the listener can fire) with no diagnostic if it were ever violated. Now logs on a miss before falling back, so a future regression is observable instead of silently reintroducing the stale-caret bug the WeakMap exists to fix. No test added for the miss branch: it is unreachable through the public toDOM/updateDOM/click surface without monkeypatching the global WeakMap.
… view stub Simplification pass over the PR diff. No behaviour change. - The "never parse a position back out of the DOM" argument was derived twice (module-level blockStart comment and the toDOM write site). It now lives once at blockStart; toDOM states the local fact and points at it. - Fold the WeakMap-miss log and the fallback into one branch so the two cannot drift apart. - Drop a clause of diff-history narrative from the dispatch-guard comment; the pointer to table-widget.ts's dispatchSelection and the reason the range bound is not re-derived both stay. - Extract the thrice-repeated recording view stub in the click tests into a recordingView() helper. Assertions unchanged.
The invariant-violation log carried only the fallback offset, so in a document with several images it would not say which widget tripped it. Add the source slice, matching the source-identifying payload of this file's other breadcrumb. The branch remains unreachable by construction, so no test pins this; see the revert-check note for cycle 1.
5 tasks
mtskf
added a commit
that referenced
this pull request
Aug 14, 2026
…stamp (#359) * refactor(table): carry the root caret offset in a WeakMap, not a DOM stamp TableBlockWidget's margin-click caret fallback read the block start back out of `data-doc-from`. That channel existed only because `updateDOM` reuses the root element across widget instances and cannot re-bind the click listener, whose captured `this` stays the old instance — the same problem the image widget had until #356. The channel does not have to be the DOM. A module-private `WeakMap<HTMLElement, number>` has the same per-element, updateDOM-writable lifetime — the pattern `pendingDrag` already uses two declarations above — and keeps the value a number end to end, so there is no malformed state left to gate. Both block widgets now share one root channel. The per-CELL stamps stay on the DOM: `cellPointAt` resolves an arbitrary descendant under the pointer and no closure knows which cell was clicked, so `stampedOffset` keeps gating those reads. `data-doc-from` is still written for DOM inspection but never read back. This also removes the chain's weakest arm. The old third arm — the toDOM-time `this.docFrom` closure, reachable once both stamps failed the gate — could reveal a DIFFERENT block after a positional shift. The block start now always comes from a channel `updateDOM` re-points, so the "a degraded caret still reveals this table" guarantee holds unconditionally; the remaining fallback is unreachable by construction and logs if it is ever hit. Behaviour is unchanged in every reachable state — nothing outside Quoll writes that attribute — so this buys an unrepresentable failure state, not a fix. * test(table): harden margin-click WeakMap coverage and fix kill-matrix comment - Add a test that pins the blockStart WeakMap write in toDOM: existing margin-click tests couldn't kill that deletion because the miss fallback (widget.docFrom) dispatches the same anchor value in every fixture here. The new test asserts the miss console.error breadcrumb is NOT called, which the deletion does trigger. - Correct the kill-matrix comment above the data-doc-from it.each block: it previously claimed both rows redden on any attribute-read revert, but the gated pre-refactor read (stampedOffset ?? this.docFrom) only reddens the well-formed-but-wrong row — the malformed row falls through that gate to the same value the test expects. Verified both revert flavours by temporarily restoring each expression and running the suite. * docs(table): point the caret comments at stampedOffset instead of restating it Comment-only pass over this PR's diff. No executable code, test rows, or assertions changed. The "checkSelection only rejects range.to > doc.length, so NaN lands a silently broken selection" argument was being re-derived three times here. It already lives on `stampedOffset` in cell-point.ts, whose docblock names this file's caret path as a consumer, so `blockStart` and the click listener now point there and keep only their own local claims: why the block start needs no gate, why the cell offset must stay on the DOM and go through one, and why the "degraded caret still reveals this table" guarantee is now unconditional. The two test comments lose their length without losing a fact — both kill matrices and the non-vacuity argument survive verbatim in substance — and drop two self-references to notes that exist nowhere in the repo. * docs(test): correct kill-matrix and margin-click comment claims - Fix the 'both rows are needed' overstatement on the data-doc-from kill-matrix comment: '999' alone kills both reverts; 'abc' is redundant for kill power there but isolates a separate NaN-silent failure mode against the bare read. - Narrow the blockStart margin-click comment's universal claim: only fixtures that click right after toDOM share docFrom between the WeakMap entry and the fallback closure. The updateDOM re-stamp fixture stays green for an unrelated reason (its own blockStart.set write re-fills the entry).
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ImageBlockWidgetdispatched its click caret fromNumber(root.dataset.docFrom)with no validation. CodeMirror does not backstop that —checkSelectiononly testsrange.to > doc.length, so"","-5","78.5"and"abc"each install a silently broken selection that notry/catchcan observe. Rather than validating the DOM read, this removes it: the current offset now travels in a module-privateWeakMap<HTMLElement, number>, so there is no malformed state left to validate.Behaviour is unchanged in every reachable state — nothing outside Quoll's own code writes that attribute (CSP default-deny webview, no third-party scripts). This buys an unrepresentable failure state, not an incident fix, and should not be read as a security change.
Why a WeakMap rather than the shared gate
The stamp exists because
updateDOMreuses the element across widget instances and cannot re-bind the click listener, so the new instance needs a channel to reach the old closure. That channel does not have to be the DOM: aWeakMapkeyed on the root element has the same per-element,updateDOM-writable lifetime — the patternpendingDragalready uses two declarations away intable-widget.ts— and keeps the value anumberend to end.The alternative was to gate the read with the table's
stampedOffset, moved to a neutral module shared by both widgets. That module would exist solely soimage/could import it, and the agreed end-state (both widgets off root stamps) then orphans it — "create module → repoint two imports → delete the reason it exists". Codex recommended the WeakMap twice (Conf 97) and Fable adjudicated for it (Conf 85).The table's per-CELL stamps stay on the DOM and keep
stampedOffset:cellPointAtresolves an arbitrary descendant under the pointer, and no closure knows which cell was clicked. Converging the table's ROOT arm is filed as a follow-up.Changes
image-widget.ts: module-privateblockStartWeakMap written bytoDOM/updateDOM, read by the click listener.data-doc-fromis still written for DOM inspection but never read back (comment says so, so the round-trip is not reintroduced).image-widget.ts: guard the dispatch, as the table'sdispatchSelectiondoes. With the anchor anumberby construction, an offset that outlived a shrinking edit, CodeMirror's re-entrancy error and a throwing transaction filter can still throw — and the last two were unguarded here regardless of channel."abc", and"999"— the well-formed decoy a format gate would have missed), plus the guard.Test Plan
pnpm compile/pnpm test:unit(4952 passed) /pnpm lint/pnpm build/pnpm package+ vsix auditNumber(root.dataset.docFrom)→ 2 failed (both DOM-value rows)updateDOMchannel write → 2 failed (the existingupdateDOMclick assertions at:272/:321, which already pin the new offset)try/catch→ 1 failed (the guard)source reveals; type above it, click again, confirm the caret still lands on the image's lineNotes
No CHANGELOG entry: the change is invisible to users in every reachable state, which the repo's CHANGELOG rule excludes.
LEARNING.mdgains the channel decision and a testing gotcha (an exception thrown inside a DOM listener is not re-thrown to theclick()caller, so "did not throw" cannot pin the guard — only theconsole.errorcan), and corrects the 2026-07-05 entry that told readers to route offsets throughdata-*stamps.