fix(image): carry the caret offset in a WeakMap, not a DOM stamp

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<HTMLElement, number>();
+
 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;
   }
 

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 <div class='quoll-block quoll-image-block'> wrapping a live <img>", () => {
     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", () => {