perf: cache cursor row measurements — no more per-keypress layout sweep (#58)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: ymansurozer

src/ui/cursor-rows.test.ts

@@ -0,0 +1,41 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { mergeRows, type Row } from "./cursor-rows";
+
+// Build a measured row with the only fields mergeRows reads; `el` is carried through untouched.
+function row(side: Row["side"], line: number, top: number, change = false): Row {
+  return { el: {} as HTMLElement, side, line, top, height: 20, change };
+}
+
+test("orders rows top-to-bottom regardless of input order", () => {
+  const out = mergeRows([row("additions", 3, 40), row("additions", 1, 0), row("additions", 2, 20)]);
+  assert.deepEqual(
+    out.map((r) => r.line),
+    [1, 2, 3],
+  );
+});
+
+test("merges split-view twins at the same rounded top — additions stays primary", () => {
+  // A context line shows in both columns at (near-)identical y; the additions cell sorts first,
+  // so it becomes the primary row and the deletions coordinate is preserved as `alt`.
+  const out = mergeRows([row("deletions", 3, 10.4), row("additions", 5, 10.2)]);
+  assert.equal(out.length, 1);
+  assert.equal(out[0].side, "additions");
+  assert.equal(out[0].line, 5);
+  assert.deepEqual(out[0].alt, { side: "deletions", line: 3 });
+});
+
+test("keeps rows at distinct tops separate and merges only on rounded equality", () => {
+  const out = mergeRows([
+    row("additions", 1, 10.2),
+    row("deletions", 9, 10.4), // rounds to 10 → merged into the row above as its twin
+    row("additions", 2, 30),
+  ]);
+  assert.equal(out.length, 2);
+  assert.deepEqual(
+    out.map((r) => r.line),
+    [1, 2],
+  );
+  assert.deepEqual(out[0].alt, { side: "deletions", line: 9 });
+  assert.equal(out[1].alt, undefined);
+});

src/ui/cursor-rows.ts

@@ -0,0 +1,35 @@
+import type { Side } from "./types";
+
+export type Row = {
+  el: HTMLElement;
+  side: Side;
+  line: number;
+  top: number;
+  height: number;
+  change: boolean;
+  // The split-view twin of a context row (see mergeRows): the deletions-side coordinates of the
+  // same visual line, kept so a cursor seeded from a left-column click still matches this row.
+  alt?: { side: Side; line: number };
+};
+
+// Sort measured rows top-to-bottom (additions before deletions when they share a y) and merge the
+// split-view twins of a context line — the additions- and deletions-column cells that render at
+// the same visual line — into ONE row, keeping the additions side primary and the deletions
+// coordinate as `alt` so a cursor seeded from either column still matches. Pure over the measured
+// list; the getBoundingClientRect sweep that produces `out` lives in cursor.ts (rows()).
+export function mergeRows(out: Row[]): Row[] {
+  out.sort((a, b) => a.top - b.top || (a.side === b.side ? 0 : a.side === "additions" ? -1 : 1));
+  const seen = new Map<number, Row>();
+  const list: Row[] = [];
+  for (const r of out) {
+    const k = Math.round(r.top);
+    const kept = seen.get(k);
+    if (kept) {
+      kept.alt = { side: r.side, line: r.line };
+      continue;
+    }
+    seen.set(k, r);
+    list.push(r);
+  }
+  return list;
+}

src/ui/cursor.ts

@@ -5,6 +5,7 @@ import { acceptChange } from "./decisions";
 import { openCommentComposer } from "./selection";
 import { render } from "./render";
 import { diffShadowRoot } from "./diff-dom";
+import { type Row, mergeRows } from "./cursor-rows";
 
 // ── The diff line cursor ─────────────────────────────────────────────────────
 // Keyboard review needs a "current line" the diff doesn't otherwise have. We keep it as a
@@ -14,28 +15,32 @@ import { diffShadowRoot } from "./diff-dom";
 // pointer and keyboard share ONE highlight: a click seeds the cursor (cursorSyncTo) and the
 // arrows move the same selection from there.
 
-type Row = {
-  el: HTMLElement;
-  side: Side;
-  line: number;
-  top: number;
-  height: number;
-  change: boolean;
-  // The split-view twin of a context row (see rows()): the deletions-side coordinates of the
-  // same visual line, kept so a cursor seeded from a left-column click still matches this row.
-  alt?: { side: Side; line: number };
-};
-
 let cur: { side: Side; line: number } | null = null;
 
+// rows() runs a full-file getBoundingClientRect sweep to build its Row[]; it's called per keypress
+// (cursorMoveLine/Hunk, landAt, ensureCursor) and after every render (cursorResync), so on a large
+// expanded file each arrow press forced a synchronous whole-file layout. The list is derived purely
+// from DOM structure and content-relative tops (the stored `top` is relative to the scrolled
+// content, not the viewport), so it's stable across scroll — only a re-render, or a resize
+// reflowing line heights, actually changes it. So we cache it and rebuild only on those events:
+// invalidateCursorRows() is wired to @pierre's onPostRender (render.ts — mount / update / unmount,
+// which covers our render() AND @pierre's own expandHunk rerenders) and to window resize (main.ts).
+// Scroll deliberately does NOT invalidate: scrolling never changes the list, and arrow navigation
+// (which scrolls via scrollIntoView) stays on cache hits instead of re-sweeping the file per press.
+let cached: Row[] | null = null;
+
+export function invalidateCursorRows() {
+  cached = null;
+}
+
 // Every navigable code line in visual (top-to-bottom) order. @pierre tags each line's gutter with
 // a [data-line-number-content] span inside a [data-line-type] cell, within a [data-additions] /
 // [data-deletions] column (split) — that gives us side + number + row element. Context lines show
-// in both split columns at the same y; merge by rounded top into one row (additions side as the
-// primary, the deletions twin preserved as `alt` so either coordinate matches).
+// in both split columns at the same y; mergeRows() folds those twins into one row.
 function rows(): Row[] {
+  if (cached) return cached;
   const sh = diffShadowRoot();
-  if (!sh) return [];
+  if (!sh) return []; // shadow not mounted yet — don't cache, retry on the next call
   const diff = $("diff");
   const diffTop = diff.getBoundingClientRect().top;
   const scrollTop = diff.scrollTop;
@@ -65,20 +70,8 @@ function rows(): Row[] {
       change: type.startsWith("change-"),
     });
   });
-  out.sort((a, b) => a.top - b.top || (a.side === b.side ? 0 : a.side === "additions" ? -1 : 1));
-  const seen = new Map<number, Row>();
-  const list: Row[] = [];
-  for (const r of out) {
-    const k = Math.round(r.top);
-    const kept = seen.get(k);
-    if (kept) {
-      kept.alt = { side: r.side, line: r.line };
-      continue;
-    }
-    seen.set(k, r);
-    list.push(r);
-  }
-  return list;
+  cached = mergeRows(out);
+  return cached;
 }
 
 // A row matches on its primary coordinates or its split-view twin (`alt`).

src/ui/main.ts

@@ -36,7 +36,7 @@ import {
 } from "./guide";
 import { setBaseTitle, reviewStats } from "./progress";
 import { installKeys, helpGroups, confirmYes, confirmNo } from "./keys";
-import { cursorReset, cursorSelection } from "./cursor";
+import { cursorReset, cursorSelection, invalidateCursorRows } from "./cursor";
 import type { ReviewState, FileRow, Settings, DiffStyle } from "./types";
 
 // Close the inline composer when clicking outside it (unless it has unsaved text). The
@@ -517,4 +517,8 @@ render();
 $("diff").addEventListener("scroll", () => {
   S.diffScrolled = $("diff").scrollTop > 140;
 });
+// A resize reflows line heights (and wrap), so the cursor's cached row measurements no longer
+// hold — drop the cache so the next navigation re-measures. (Scroll doesn't: the cached tops are
+// content-relative, so scrolling leaves the row list unchanged — see cursor.ts.)
+window.addEventListener("resize", invalidateCursorRows);
 setInterval(pollState, 1500);

src/ui/render.ts

@@ -26,7 +26,7 @@ import { approveCurrentFile, resetReview } from "./decisions";
 import { blockersChip } from "./blockers";
 import { isMarkdownPath, renderMarkdownFile } from "./mdfile";
 import { renderMarkdown } from "./markdown";
-import { cursorResync, cursorReset } from "./cursor";
+import { cursorResync, cursorReset, invalidateCursorRows } from "./cursor";
 import { hasGuide, renderOverview, currentGuideEntry } from "./guide";
 import { updateProgress } from "./progress";
 import {
@@ -579,6 +579,9 @@ async function renderCenter() {
     // finds nothing; onPostRender fires when they exist. (afterRender still runs it too, for
     // the warm/cached path where rows are already present — both are idempotent.)
     onPostRender: (_node: HTMLElement, _inst: unknown, phase: string) => {
+      // The rendered rows just changed (mount, update, or @pierre's own expandHunk rerender —
+      // which never routes through our render()), so the cursor's cached row list is stale.
+      invalidateCursorRows();
       if (phase !== "unmount") applySkimCollapse();
     },
     // @pierre reserves a right-side gutter via `scrollbar-gutter: stable` on the code grid