diff --git a/src/webview/cm/image/image-widget.ts b/src/webview/cm/image/image-widget.ts index 970e48f0..0f12efb2 100644 --- a/src/webview/cm/image/image-widget.ts +++ b/src/webview/cm/image/image-widget.ts @@ -35,6 +35,26 @@ import { imageDimensionCache } from "./image-dimension-cache.js"; // with image-field.ts's `warnedUnresolvableImage` once-per-session latch. let warnedImageLoadError = false; +// The block's CURRENT first-byte offset, keyed on the widget's root element. +// +// Keyed on the element rather than held in the `toDOM` closure because +// `updateDOM` reuses that element across widget instances: after a distant edit +// shifts this block, CodeMirror builds a NEW widget, `eq()` returns false, and +// `updateDOM` re-points the reused DOM — but it cannot re-bind the click +// listener, whose captured `this` is the OLD instance. So the new instance +// needs a channel to hand the current offset to the existing listener, and the +// channel has to be updatable exactly when the position moves, which +// `updateDOM` can do and the closure cannot. A WeakMap so a discarded root +// takes its entry with it. Same pattern, same reason, as table-widget.ts's +// `pendingDrag`. +// +// A `number` end to end: nothing is stringified, parsed, or read back from the +// DOM, so there is no malformed-value state to validate against. (`checkSelection` +// in @codemirror/state only rejects `range.to > doc.length`, so a `NaN` / +// negative / fractional anchor would otherwise install a silently broken +// selection that no try/catch can observe.) +const blockStart = new WeakMap(); + export class ImageBlockWidget extends WidgetType { constructor( /** CommonMark-normalized image alt text (backslash/entity decode + emphasis @@ -66,10 +86,12 @@ export class ImageBlockWidget extends WidgetType { // with the visible DOM; breathing room comes from padding on the wrapper. const root = document.createElement("div"); root.className = "quoll-block quoll-image-block"; - // Caret target stored on the DOM so a reused element (updateDOM) reflects - // the CURRENT docFrom, not a stale toDOM-time closure (mirrors the table - // widget's data-doc-from margin fallback). + // The caret target travels through `blockStart`, NOT through this + // attribute: `data-doc-from` is written for DOM inspection (and read by + // tests that pin the re-stamp) and is NEVER read back — see `blockStart` + // above for why a position must not be parsed back out of the DOM. root.dataset.docFrom = String(this.docFrom); + blockStart.set(root, this.docFrom); if (this.safeUrl !== null) { const src = this.safeUrl; @@ -129,10 +151,38 @@ export class ImageBlockWidget extends WidgetType { // widget, so (unlike the table widget) there is no modifier-click // navigation exception to guard. root.addEventListener("click", () => { - const stamped = root.dataset.docFrom; - view.dispatch({ - selection: { anchor: stamped !== undefined ? Number(stamped) : this.docFrom }, - }); + // Falling back to `this.docFrom` totalizes the `number | undefined` read; + // it is not the stale-closure hazard coming back. The entry is set above, + // in the same breath as attaching this listener, and at toDOM time the + // closure value IS the current one — so a miss is unreachable by + // construction. Logged, not silently trusted, so a future regression of + // that invariant is observable instead of silently reintroducing the + // stale-caret bug this WeakMap exists to fix. + let anchor = blockStart.get(root); + if (anchor === undefined) { + // `slice` identifies WHICH widget tripped it — a document can hold many + // images, and `fallback` alone would not say which one. Matches the + // source-identifying payload of this file's other breadcrumb + // (`{ src }` on a failed load). + console.error("[quoll] image widget blockStart miss — invariant violated", { + slice: this.slice, + fallback: this.docFrom, + }); + anchor = this.docFrom; + } + // A `number` anchor does not make the dispatch infallible — see + // table-widget.ts's `dispatchSelection` for the enumeration of what still + // throws (out-of-range after a shrinking edit, CodeMirror's re-entrancy + // error, a throwing transaction filter). The range bound is deliberately + // NOT re-checked against `view.state.doc.length`: CodeMirror owns that + // invariant and enforces it by throwing, and a second copy of the rule + // here could drift from it. The throw must not escape into a DOM listener + // unlogged — the gesture is lost, the editor keeps running. + try { + view.dispatch({ selection: { anchor } }); + } catch (err) { + console.error("[quoll] image widget selection dispatch failed", { anchor, err }); + } }); return root; @@ -153,7 +203,12 @@ export class ImageBlockWidget extends WidgetType { if (from.slice !== this.slice) { return false; } + // Re-point the caret channel the click listener actually reads. The + // attribute beside it is inspection-only (see toDOM) — dropping THIS line + // would leave the reused listener dispatching the old offset while the DOM + // still looked correct. dom.dataset.docFrom = String(this.docFrom); + blockStart.set(dom, this.docFrom); return true; } diff --git a/test/webview/image/cm-image-widget.test.ts b/test/webview/image/cm-image-widget.test.ts index 7bacd619..b7ee151b 100644 --- a/test/webview/image/cm-image-widget.test.ts +++ b/test/webview/image/cm-image-widget.test.ts @@ -13,6 +13,20 @@ const url = (s: string): AllowlistedUrl => s as AllowlistedUrl; const mockView = { dispatch: () => {} } as unknown as EditorView; +// View stub that records every dispatched selection, for the click tests that +// assert WHICH caret offset the widget aimed at. +const recordingView = (): { view: EditorView; dispatched: Array<{ anchor: number }> } => { + const dispatched: Array<{ anchor: number }> = []; + const view = { + dispatch: (tr: { selection?: { anchor: number } }) => { + if (tr.selection) { + dispatched.push(tr.selection); + } + }, + } as unknown as EditorView; + return { view, dispatched }; +}; + describe("ImageBlockWidget.toDOM (allowlisted)", () => { it("renders
wrapping a live ", () => { const dom = new ImageBlockWidget( @@ -165,37 +179,88 @@ describe("ImageBlockWidget identity + events", () => { }); it("click on the widget dispatches a selection to docFrom (reveal trigger)", () => { - const dispatched: Array<{ anchor: number }> = []; - const stub = { - dispatch: (tr: { selection?: { anchor: number } }) => { - if (tr.selection) { - dispatched.push(tr.selection); - } - }, - } as unknown as EditorView; + const { view, dispatched } = recordingView(); const dom = new ImageBlockWidget( "a", url("https://x.test/a.png"), "![a](https://x.test/a.png)", 42 - ).toDOM(stub); + ).toDOM(view); dom.click(); expect(dispatched).toEqual([{ anchor: 42 }]); }); it("click on the blocked placeholder also dispatches caret to docFrom", () => { - const dispatched: Array<{ anchor: number }> = []; - const stub = { - dispatch: (tr: { selection?: { anchor: number } }) => { - if (tr.selection) { - dispatched.push(tr.selection); - } - }, - } as unknown as EditorView; - const dom = new ImageBlockWidget("a", null, "![a](javascript:alert(1))", 5).toDOM(stub); + const { view, dispatched } = recordingView(); + const dom = new ImageBlockWidget("a", null, "![a](javascript:alert(1))", 5).toDOM(view); dom.click(); expect(dispatched).toEqual([{ anchor: 5 }]); }); + + // The DOM is NOT an input to the caret. `data-doc-from` is still written (DOM + // inspection, plus the re-stamp assertions in the updateDOM block below), but + // the listener reads the module-private WeakMap, so a value written onto the + // element cannot steer the dispatch. + // + // Both rows go red if the listener reverts to `Number(root.dataset.docFrom)`: + // "abc" would dispatch `NaN`, which CodeMirror ACCEPTS — `checkSelection` + // only rejects `range.to > doc.length` — installing a silently broken + // selection that no try/catch can observe; "999" is the case a format gate + // would also have missed, dispatching a caret into an unrelated block. + it.each([ + ["malformed", "abc"], + ["well-formed but wrong", "999"], + ])("ignores a %s data-doc-from written onto the widget root", (_label, raw) => { + const { view, dispatched } = recordingView(); + const dom = new ImageBlockWidget( + "a", + url("https://x.test/a.png"), + "![a](https://x.test/a.png)", + 42 + ).toDOM(view); + dom.dataset.docFrom = raw; + dom.click(); + expect(dispatched).toEqual([{ anchor: 42 }]); + }); + + // `view.dispatch` can still throw with the anchor a `number` by construction: + // an offset that outlived a shrinking edit (RangeError from checkSelection), + // CodeMirror's "update in progress" re-entrancy error — this listener runs on + // a DOM event, which an in-progress update can deliver — or a throwing + // transaction filter. None can be staged from a display-only widget fixture, + // so the pin uses a throwing stub; without it the try/catch could be deleted + // and the suite would stay green. + // + // Only console.error is asserted, not a `dom.click()` "did not throw" + // wrapper: in this vitest+happy-dom suite an uncaught throw from a DOM + // listener propagates synchronously out of click() (real browsers instead + // report it via window.onerror), so that wrapper would be non-vacuous here + // too — asserting console.error is still the stronger pin, since it also + // catches a mutation that empties the catch body. + it("logs and swallows a throwing dispatch instead of letting it escape the listener", () => { + const errSpy = vi.spyOn(console, "error").mockImplementation(() => {}); + try { + const boom = new RangeError("Selection points outside of document"); + const stub = { + dispatch: () => { + throw boom; + }, + } as unknown as EditorView; + const dom = new ImageBlockWidget( + "a", + url("https://x.test/a.png"), + "![a](https://x.test/a.png)", + 7 + ).toDOM(stub); + dom.click(); + expect(errSpy).toHaveBeenCalledWith("[quoll] image widget selection dispatch failed", { + anchor: 7, + err: boom, + }); + } finally { + errSpy.mockRestore(); + } + }); }); describe("ImageBlockWidget.toDOM — dimension cache", () => {