refactor(table): carry the root caret offset in a WeakMap, not a DOM stamp

src/webview/cm/table/table-widget.ts

@@ -2,7 +2,8 @@
 // <table> in place of its source. Click-to-reveal: a click on any cell
 // dispatches a caret selection to the cell's absolute LF-internal source
 // offset (data-cell-from = nodeFrom + cell.from); a click on the widget
-// padding/margin (no cell) falls back to the block line-start (data-doc-from).
+// padding/margin (no cell) falls back to the block line-start, carried in the
+// module-private `blockStart` WeakMap (NOT read back out of the DOM).
 // A mousedown followed by a click that actually moved (see DRAG_THRESHOLD_PX)
 // instead dispatches a RANGE selection between the two RESOLVED source offsets;
 // an endpoint whose cell renders non-byte-aligned (inline markup) has no exact
@@ -21,9 +22,9 @@
 // caret offset (nodeFrom + cell.from). Both are LF-internal (seed.ts
 // splitToCmText strips \r). Two tables at different doc positions or with
 // different Lezer node starts are NOT eq; same (docFrom, slice, nodeFrom) on
-// a rebuild reuses the existing DOM. updateDOM re-stamps both on reuse so a
-// margin/cell click after a shift uses the new offsets, not a stale toDOM-time
-// closure.
+// a rebuild reuses the existing DOM. updateDOM re-points both channels on reuse
+// (cell stamps on the DOM, block start in `blockStart`) so a margin/cell click
+// after a shift uses the new offsets, not a stale toDOM-time closure.
 
 import { type EditorView, WidgetType } from "@codemirror/view";
 
@@ -59,6 +60,43 @@ interface PendingDrag {
 }
 const pendingDrag = new WeakMap<HTMLElement, PendingDrag>();
 
+/** The block's CURRENT first-byte offset (margin-click caret target), keyed on
+ *  the widget's root element for the same reason `pendingDrag` is: `updateDOM`
+ *  reuses that element across widget instances and cannot re-bind the click
+ *  listener, whose captured `this` stays the OLD instance — so the new instance
+ *  needs a channel to the existing listener that moves exactly when the block
+ *  moves, which `updateDOM` can write and the closure cannot.
+ *
+ *  A `number` end to end: unlike the per-CELL offsets it is never stringified,
+ *  parsed, or read back from the DOM, so there is no malformed-value state to
+ *  gate — which is why `stampedOffset` guards those stamps and not this one
+ *  (its docblock has what CodeMirror does NOT catch). Same channel, same
+ *  rationale, as image-widget.ts's `blockStart`. */
+const blockStart = new WeakMap<HTMLElement, number>();
+
+/** Margin-click caret: the block start this root currently points at.
+ *
+ *  Falling back to the toDOM-time `widget.docFrom` totalizes the
+ *  `number | undefined` read; it is not the stale-closure hazard coming back.
+ *  The entry is written in `toDOM` in the same breath as attaching the listener,
+ *  and at that moment the closure value IS the current one — so a miss is
+ *  unreachable by construction. Logged rather than trusted, so a future
+ *  regression of that invariant is observable instead of quietly reintroducing
+ *  the stale-caret bug this WeakMap exists to prevent. */
+function blockStartCaret(root: HTMLElement, widget: TableBlockWidget): number {
+  const current = blockStart.get(root);
+  if (current === undefined) {
+    // `slice` identifies WHICH widget tripped it — a document can hold many
+    // tables, and `fallback` alone would not say which one.
+    console.error("[quoll] table widget blockStart miss — invariant violated", {
+      slice: widget.slice,
+      fallback: widget.docFrom,
+    });
+    return widget.docFrom;
+  }
+  return current;
+}
+
 /** Every selection dispatch out of this widget's DOM listeners goes through
  *  here, so the throw paths are handled in ONE place rather than at each seam.
  *
@@ -177,9 +215,12 @@ export class TableBlockWidget extends WidgetType {
     // block-widget height measurement stays in lockstep with the visible DOM.
     const root = document.createElement("div");
     root.className = "quoll-block quoll-table-block";
-    // Margin-click caret fallback, stored on the DOM so a reused element
-    // (updateDOM) reflects the CURRENT docFrom, not a stale toDOM-time closure.
+    // The margin-click caret travels through `blockStart`, NOT through this
+    // attribute: `data-doc-from` is written for DOM inspection (and read by the
+    // tests that pin the re-stamp) and is NEVER read back — see `blockStart`
+    // above for why this position must not be parsed back out of the DOM.
     root.dataset.docFrom = String(this.docFrom);
+    blockStart.set(root, this.docFrom);
 
     // Resource base for relative in-cell image srcs. Static per editor
     // (resource-base.ts), so it is NOT part of eq() — reading it at
@@ -267,32 +308,24 @@ export class TableBlockWidget extends WidgetType {
         return;
       }
       const cell = (event.target as Element | null)?.closest?.("th, td") ?? null;
-      // Read the stamps through the SAME gate the drag path uses
-      // (`stampedOffset`), not a bare `Number(...)`. Both are DOM attributes and
-      // therefore both are trust boundaries, and CodeMirror will not catch a bad
-      // one for us: `checkSelection` only rejects `range.to > doc.length`, so a
-      // `NaN` anchor is accepted silently and installs a broken selection that
-      // `dispatchSelection`'s catch never sees.
+      // The CELL offset must stay on the DOM — `cellPointAt` resolves an
+      // arbitrary descendant under the pointer, so no closure knows which cell
+      // was clicked. That makes it a trust boundary, read through the SAME gate
+      // the drag path uses (`stampedOffset`) rather than a bare `Number(...)`;
+      // its docblock has the why — CodeMirror accepts a `NaN` anchor and
+      // installs a broken selection `dispatchSelection`'s catch never sees.
       //
       // A stamp that fails the gate degrades one step rather than dispatching
       // nothing: reveal-on-caret is LINE-level, so the block start reveals the
       // same table the cell offset would — only the intra-table caret precision
       // is lost, and a dead click (no reveal at all) is a worse answer for a
       // failure mode that only arises when something outside this widget wrote
-      // its DOM.
-      //
-      // ⚠️ That "same table" guarantee holds for arm 2 (`data-doc-from`) but
-      // NOT for arm 3. `this.docFrom` is the toDOM-time closure value, and
-      // `updateDOM` re-stamps the DOM without being able to re-bind this
-      // listener — the very trap this file's header warns about — so after a
-      // positional shift the closure is stale and arm 3 can reveal a DIFFERENT
-      // block, not merely a less precise position within this one. It is last
-      // for exactly that reason: it is the least trustworthy link, reachable
-      // only once both DOM stamps have already failed the gate.
+      // its DOM. That "same table" guarantee is unconditional now that the block
+      // start comes from `blockStart`, which `updateDOM` re-points: it can no
+      // longer be a stale closure value pointing at a DIFFERENT block.
       const caret =
         (cell === null ? null : stampedOffset(cell, "data-cell-from")) ??
-        stampedOffset(root, "data-doc-from") ??
-        this.docFrom;
+        blockStartCaret(root, this);
       dispatchSelection(view, dragRange(view, root, event, pending) ?? { anchor: caret });
     });
 
@@ -362,9 +395,13 @@ export class TableBlockWidget extends WidgetType {
     // would dispatch a selection over an unrelated span. Drop it; the gesture
     // degrades to the collapsed caret.
     pendingDrag.delete(dom);
-    // Re-stamp the margin fallback so a reused element tracks the new docFrom
-    // after a distant edit shifted this table without changing its bytes.
+    // Re-point the margin-click caret channel the click listener actually reads,
+    // so a reused element tracks the new docFrom after a distant edit shifted
+    // this table without changing its bytes. 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);
     // Pure positional shift: the bytes are identical (from.slice === this.slice)
     // and only the absolute offsets moved. Re-stamp data-cell-from on each cell
     // and reuse the rendered inline children verbatim — skip patchRow's

test/webview/table/cm-table-widget.test.ts

@@ -1024,12 +1024,14 @@ describe("TableBlockWidget drag-selection", () => {
 });
 
 describe("TableBlockWidget caret dispatch hardening", () => {
-  // The caret path reads `data-cell-from` / `data-doc-from` off the DOM, so it
-  // sits on the same trust boundary as the drag path and must use the same
-  // gate. A bare `Number(...)` here would not merely be untidy: CodeMirror's
-  // `checkSelection` tests `range.to > doc.length` and nothing else, so a `NaN`
-  // anchor is ACCEPTED and installs a range whose `from` is `NaN` — a silently
-  // broken selection, with no throw for `dispatchSelection`'s catch to see.
+  // The caret path reads `data-cell-from` off the DOM — the per-cell offset has
+  // to live there, since `cellPointAt` resolves whatever descendant is under the
+  // pointer — so it sits on the same trust boundary as the drag path and must
+  // use the same gate. A bare `Number(...)` here would not merely be untidy:
+  // CodeMirror's `checkSelection` tests `range.to > doc.length` and nothing
+  // else, so a `NaN` anchor is ACCEPTED and installs a range whose `from` is
+  // `NaN` — a silently broken selection, with no throw for
+  // `dispatchSelection`'s catch to see.
   // Hence the assertions below are on the exact dispatched value, not on
   // "something was dispatched".
   it.each([
@@ -1050,15 +1052,34 @@ describe("TableBlockWidget caret dispatch hardening", () => {
     expect(dispatched).toEqual([{ selection: { anchor: 7 } }]);
   });
 
-  it("falls back to the widget's own docFrom when the ROOT stamp is malformed too", () => {
+  // The ROOT position, unlike the cell stamps, is NOT an input from the DOM.
+  // `data-doc-from` is still written (DOM inspection, plus the re-stamp
+  // assertions in the updateDOM block above), but the block-start fallback
+  // reads the module-private WeakMap, so a value written onto the element
+  // cannot steer the dispatch.
+  //
+  // "999" alone kills both reverts — it dispatches 999 (≠ 7, this fixture's
+  // expected anchor) whether read BARE (`Number(root.dataset.docFrom)`) or
+  // via the pre-refactor GATED read (`stampedOffset(root, "data-doc-from")
+  // ?? this.docFrom`), since "999" also passes the gate's `/^\d+$/`. "abc"
+  // is redundant against the gated read — it fails the gate and falls
+  // through to `this.docFrom`, leaving that revert green — but earns its
+  // place against the bare read: it dispatches `NaN`, silently accepted by
+  // `checkSelection` (rejects only `range.to > doc.length`), breaking the
+  // selection silently. (Each revert applied; red rows observed.)
+  it.each([
+    ["malformed", "abc"],
+    ["well-formed but wrong", "999"],
+  ])("ignores a %s data-doc-from written onto the widget root", (_label, raw) => {
     const dispatched: unknown[] = [];
     const dom = makeWidget(SRC, 7).toDOM(stubView(dispatched));
     document.body.appendChild(dom);
     const td = dom.querySelectorAll("td")[0] as HTMLElement;
-    td.setAttribute("data-cell-from", "abc");
-    dom.setAttribute("data-doc-from", "-3");
+    td.setAttribute("data-cell-from", "abc"); // force the block-start fallback
+    dom.setAttribute("data-doc-from", raw);
     press(td, "click", 10, 10);
-    // 7 is the constructor argument, which never travelled through the DOM.
+    // 7 is the constructor argument, carried in the WeakMap — it never
+    // travelled through the DOM.
     expect(dispatched).toEqual([{ selection: { anchor: 7 } }]);
   });
 
@@ -1090,4 +1111,27 @@ describe("TableBlockWidget caret dispatch hardening", () => {
       consoleError.mockRestore();
     }
   });
+
+  // A fresh toDOM'd widget's margin click must hit the `blockStart` entry
+  // written in toDOM, not `blockStartCaret`'s miss fallback — the absent
+  // `console.error` is the only observable difference. The anchor VALUE
+  // cannot distinguish them here (nor in any other fixture in this file
+  // that clicks right after toDOM): both trace back to the same `docFrom`
+  // constructor argument, so deleting `blockStart.set(root, this.docFrom)`
+  // in `toDOM` leaves every such anchor assertion green. (The updateDOM
+  // re-stamp fixture above stays green for an unrelated reason: its OWN
+  // `blockStart.set` write re-fills the entry with the new docFrom.)
+  it("does not log a blockStart miss when a fresh toDOM'd widget's margin is clicked", () => {
+    const dispatched: unknown[] = [];
+    const dom = makeWidget(SRC, 7).toDOM(stubView(dispatched));
+    document.body.appendChild(dom);
+    const consoleError = vi.spyOn(console, "error").mockImplementation(() => undefined);
+    try {
+      dom.click(); // the root div, not a cell
+      expect(dispatched).toEqual([{ selection: { anchor: 7 } }]);
+      expect(consoleError).not.toHaveBeenCalled();
+    } finally {
+      consoleError.mockRestore();
+    }
+  });
 });