refactor: sweep of small review fixes — batched reset, LRU comment cache, drag side, port-safe smokes (#62)

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

Author: ymansurozer

scripts/perf-smoke.mjs

@@ -17,13 +17,46 @@ import { tmpdir } from "node:os";
 import path from "node:path";
 import assert from "node:assert/strict";
 
-const PORT = 6798,
-  ID = "perf-smoke";
+const ID = "perf-smoke";
 const CLI = path.join(process.cwd(), "dist", "cli.js");
 const UI_BUNDLE = path.join(process.cwd(), "dist", "ui.js");
-const BASE = `http://127.0.0.1:${PORT}`;
+// Bound lazily from the desk's startup line once it launches — see waitForDeskUrl. The desk
+// silently falls back to a random port when a fixed one is taken, so assuming a port here made
+// every fetch fail confusingly on a busy machine; we read the port it actually bound instead.
+let BASE;
 const FILE_COUNT = 1000;
 const sleep = (ms) => new Promise((r) => setTimeout(r, ms));
+
+// The desk prints `Galley <session>: http://127.0.0.1:<port>/` to stderr once it's listening.
+// Read that line (accumulating chunks — it can arrive split) and return the origin without the
+// trailing slash, so BASE + "/api/..." is well-formed.
+const waitForDeskUrl = (child) =>
+  new Promise((resolve, reject) => {
+    let buf = "";
+    const timer = setTimeout(() => {
+      cleanupListeners();
+      reject(new Error("timed out waiting for the desk to print its URL"));
+    }, 30_000);
+    const onData = (d) => {
+      buf += d;
+      const m = buf.match(/http:\/\/127\.0\.0\.1:\d+/);
+      if (m) {
+        cleanupListeners();
+        resolve(m[0]);
+      }
+    };
+    const onExit = () => {
+      cleanupListeners();
+      reject(new Error("desk exited before printing its URL"));
+    };
+    const cleanupListeners = () => {
+      clearTimeout(timer);
+      child.stderr.off("data", onData);
+      child.off("exit", onExit);
+    };
+    child.stderr.on("data", onData);
+    child.once("exit", onExit);
+  });
 const cli = (...args) => execFileSync("node", [CLI, ...args], { encoding: "utf8" }).trim();
 const getText = async (p) => (await fetch(BASE + p)).text();
 
@@ -101,10 +134,12 @@ try {
   writeFileSync(path.join(tmp, "generated-bundle.txt"), bigLines("edited"));
 
   const startedAt = Date.now();
-  desk = spawn("node", [CLI, "--repo", tmp, "--session", ID, "--port", String(PORT), "--no-open"], {
-    stdio: "ignore",
+  desk = spawn("node", [CLI, "--repo", tmp, "--session", ID, "--port", "0", "--no-open"], {
+    // stderr piped so we can read the bound URL; stdout ignored.
+    stdio: ["ignore", "ignore", "pipe"],
     env: { ...process.env, GALLEY_NO_UPDATE_CHECK: "1" },
   });
+  BASE = await waitForDeskUrl(desk);
   let up = false;
   for (let i = 0; i < 150; i++) {
     try {

scripts/smoke.mjs

@@ -9,11 +9,44 @@ import { tmpdir } from "node:os";
 import path from "node:path";
 import assert from "node:assert/strict";
 
-const PORT = 6799,
-  ID = "smoke";
+const ID = "smoke";
 const CLI = path.join(process.cwd(), "dist", "cli.js");
-const BASE = `http://127.0.0.1:${PORT}`;
+// Bound lazily from the desk's startup line once it launches — see waitForDeskUrl. The desk
+// silently falls back to a random port when a fixed one is taken, so assuming a port here made
+// every fetch fail confusingly on a busy machine; we read the port it actually bound instead.
+let BASE;
 const sleep = (ms) => new Promise((r) => setTimeout(r, ms));
+
+// The desk prints `Galley <session>: http://127.0.0.1:<port>/` to stderr once it's listening.
+// Read that line (accumulating chunks — it can arrive split) and return the origin without the
+// trailing slash, so BASE + "/api/..." is well-formed.
+const waitForDeskUrl = (child) =>
+  new Promise((resolve, reject) => {
+    let buf = "";
+    const timer = setTimeout(() => {
+      cleanupListeners();
+      reject(new Error("timed out waiting for the desk to print its URL"));
+    }, 30_000);
+    const onData = (d) => {
+      buf += d;
+      const m = buf.match(/http:\/\/127\.0\.0\.1:\d+/);
+      if (m) {
+        cleanupListeners();
+        resolve(m[0]);
+      }
+    };
+    const onExit = () => {
+      cleanupListeners();
+      reject(new Error("desk exited before printing its URL"));
+    };
+    const cleanupListeners = () => {
+      clearTimeout(timer);
+      child.stderr.off("data", onData);
+      child.off("exit", onExit);
+    };
+    child.stderr.on("data", onData);
+    child.once("exit", onExit);
+  });
 const cli = (...args) => execFileSync("node", [CLI, ...args], { encoding: "utf8" }).trim();
 const getJson = async (p, init) => (await fetch(BASE + p, init)).json();
 const post = (p, body) =>
@@ -58,11 +91,13 @@ try {
   writeFileSync(path.join(tmp, "huge-approved.txt"), bigLines("edited"));
   writeFileSync(path.join(tmp, "huge-rejected.txt"), bigLines("edited"));
 
-  desk = spawn("node", [CLI, "--repo", tmp, "--session", ID, "--port", String(PORT), "--no-open"], {
-    stdio: "ignore",
+  desk = spawn("node", [CLI, "--repo", tmp, "--session", ID, "--port", "0", "--no-open"], {
+    // stderr piped so we can read the bound URL; stdout ignored.
+    stdio: ["ignore", "ignore", "pipe"],
     // Keep the smoke hermetic: no update-check network call at desk start.
     env: { ...process.env, GALLEY_NO_UPDATE_CHECK: "1" },
   });
+  BASE = await waitForDeskUrl(desk);
   for (let i = 0; i < 60; i++) {
     try {
       await fetch(BASE + "/api/state");

src/server.test.ts

@@ -804,6 +804,121 @@ test("a throwing wrapped route settles the mutex without poisoning the chain (is
   });
 });
 
+test("/api/reset unstages every reviewed file in one batched restore and clears the review (issue 13)", async () => {
+  const root = await mkdtemp(path.join(tmpdir(), "galley-reset-"));
+  const oldHome = process.env.HOME;
+  process.env.HOME = root;
+  const g = (args: string[]) => execFileSync("git", args, { cwd: root }).toString();
+  g(["init", "-q"]);
+  g(["config", "user.email", "t@t.co"]);
+  g(["config", "user.name", "tester"]);
+  await writeFile(path.join(root, "a.ts"), "one\n");
+  await writeFile(path.join(root, "b.ts"), "two\n");
+  g(["add", "."]);
+  g(["commit", "-qm", "init"]);
+  // Change both files and stage them, so the index carries two paths for the reset to restore.
+  await writeFile(path.join(root, "a.ts"), "one CHANGED\n");
+  await writeFile(path.join(root, "b.ts"), "two CHANGED\n");
+  const st = await buildReviewState(root, { session: "s" });
+  assert.ok(st, "built a review state for the working diff");
+  g(["add", "."]);
+  assert.deepEqual(
+    g(["diff", "--cached", "--name-only"]).trim().split("\n").sort(),
+    ["a.ts", "b.ts"],
+    "both files staged before reset",
+  );
+  st!.comments.push({
+    id: "c1",
+    path: "a.ts",
+    side: "additions",
+    lineNumber: 1,
+    body: "x",
+    createdAt: "t",
+    updatedAt: "t",
+    status: "open",
+    role: "user",
+  });
+  st!.decisions = [
+    {
+      key: "a.ts:k",
+      status: "accepted",
+      path: "a.ts",
+      lineNumber: 1,
+      side: "additions",
+      title: "t",
+    },
+  ];
+  const handle = await startServer({ state: st!, open: false, idleTimeoutMs: 0 });
+  try {
+    const res = await fetch(`${handle.url}api/reset`, { method: "POST" });
+    assert.equal(res.status, 200);
+    // A single batched `git restore --staged -- a.ts b.ts` cleared the index for BOTH files.
+    assert.equal(
+      g(["diff", "--cached", "--name-only"]).trim(),
+      "",
+      "index restored for every file",
+    );
+    // …and the reviewer-owned slice is wiped.
+    assert.deepEqual(st!.comments, []);
+    assert.deepEqual(st!.decisions, []);
+  } finally {
+    handle.server.close();
+    process.env.HOME = oldHome;
+    await rm(root, { recursive: true, force: true });
+  }
+});
+
+test("/api/reset in pr mode clears the review without touching the git index (issue 13)", async () => {
+  const root = await mkdtemp(path.join(tmpdir(), "galley-reset-pr-"));
+  const oldHome = process.env.HOME;
+  process.env.HOME = root;
+  const g = (args: string[]) => execFileSync("git", args, { cwd: root }).toString();
+  g(["init", "-q"]);
+  g(["config", "user.email", "t@t.co"]);
+  g(["config", "user.name", "tester"]);
+  await writeFile(path.join(root, "a.ts"), "one\n");
+  g(["add", "."]);
+  g(["commit", "-qm", "init"]);
+  // A staged change sitting in the index — if reset spawned git in pr mode, it would vanish.
+  await writeFile(path.join(root, "a.ts"), "one CHANGED\n");
+  g(["add", "."]);
+  const st: ReviewState = {
+    ...state(root),
+    mode: "pr",
+    comments: [
+      {
+        id: "c1",
+        path: "a.ts",
+        side: "additions",
+        lineNumber: 1,
+        body: "x",
+        createdAt: "t",
+        updatedAt: "t",
+        status: "open",
+        role: "user",
+      },
+    ],
+  };
+  const handle = await startServer({ state: st, open: false, idleTimeoutMs: 0 });
+  try {
+    const res = await fetch(`${handle.url}api/reset`, { method: "POST" });
+    assert.equal(res.status, 200);
+    // PR mode has no working-tree index to restore — the staged change is left exactly as it was
+    // (staging is disabled in pr mode), proving the route never spawned git here.
+    assert.equal(
+      g(["diff", "--cached", "--name-only"]).trim(),
+      "a.ts",
+      "index untouched in pr mode",
+    );
+    // The reviewer-owned slice is still cleared, git or no git.
+    assert.deepEqual(st.comments, []);
+  } finally {
+    handle.server.close();
+    process.env.HOME = oldHome;
+    await rm(root, { recursive: true, force: true });
+  }
+});
+
 test("settings API round-trips editorCommand", async () => {
   await withServer(async (handle) => {
     await fetch(`${handle.url}api/settings`, {

src/server.ts

@@ -745,9 +745,13 @@ export async function startServer(options: ServerOptions): Promise<ServerHandle>
       }
       if (req.method === "POST" && url.pathname === "/api/reset") {
         return await serialize(async () => {
-          for (const file of state.files) {
-            await git(["restore", "--staged", "--", file.path], state.root).catch(async () =>
-              git(["reset", "HEAD", "--", file.path], state.root),
+          // Unstage everything the review touched in one spawn — `git restore --staged` takes many
+          // pathspecs, so we don't fork per file. PR mode has no working-tree index to restore
+          // (staging is disabled there), so skip the git work entirely and only clear review state.
+          if (state.mode !== "pr" && state.files.length) {
+            const paths = state.files.map((file) => file.path);
+            await git(["restore", "--staged", "--", ...paths], state.root).catch(async () =>
+              git(["reset", "HEAD", "--", ...paths], state.root),
             );
           }
           state.comments = [];

src/state.ts

@@ -1098,11 +1098,16 @@ export async function mergeReviewState(base: ReviewState, saved: ReviewState | n
   // file-level unanchored strip rather than crashing the whole reload.
   const openPaths = new Set(comments.filter((c) => c.status === "open").map((c) => c.path));
   const contentsByPath = new Map<string, FileContents>();
-  for (const f of base.files)
-    if (openPaths.has(f.path)) {
-      const resolved = await readFileContents(base, f).catch(() => undefined);
-      if (resolved) contentsByPath.set(f.path, resolved);
-    }
+  // Read every commented file concurrently — each is an independent `git show` spawn, so awaiting
+  // them one at a time serialized the whole set behind the slowest read on every reload.
+  await Promise.all(
+    base.files
+      .filter((f) => openPaths.has(f.path))
+      .map(async (f) => {
+        const resolved = await readFileContents(base, f).catch(() => undefined);
+        if (resolved) contentsByPath.set(f.path, resolved);
+      }),
+  );
   return {
     ...base,
     id: saved.id,

src/ui/decisions.ts

@@ -1,5 +1,5 @@
-import { S, D, toast, api, persist } from "./store";
-import { currentFile, applyDecisionToDiff, fileObjections, flowIndex } from "./changes";
+import { S, toast, api, persist } from "./store";
+import { currentFile, fileObjections, flowIndex } from "./changes";
 import { render, deferRender } from "./render";
 import { nextUnreviewedFileIndex, guideProgress } from "./guide";
 import type { ChangeState, Decision } from "./types";
@@ -137,7 +137,9 @@ export async function acceptChange(id: string, status: Decision["status"]) {
   recordDecision(change, status);
   S.state.decisionFiles = S.state.decisionFiles || [];
   if (!S.state.decisionFiles.includes(change.path)) S.state.decisionFiles.push(change.path);
-  if (D.fileDiff) D.fileDiff = applyDecisionToDiff(D.fileDiff, change, status);
+  // No need to apply the decision to D.fileDiff here: renderCenter unconditionally rebuilds
+  // D.fileDiff from the raw diff (parse → ensureChanges → replayDecisions) every render, so a
+  // pre-render mutation is thrown away — the replay below picks the new status up from the record.
   toast(status === "rejected" ? "Rejected" : "Accepted");
   render();
   persist();

src/ui/markdown.ts

@@ -28,6 +28,10 @@ let md: MarkdownIt | null = null;
 // markdown keeps the standard soft-break behavior via `md`, so hard-wrapped prose isn't shredded.
 let mdComment: MarkdownIt | null = null;
 let hl: Awaited<ReturnType<typeof createHighlighterCore>> | null = null;
+// Comment bodies re-render on every poll tick, and each edit mints a fresh id:updatedAt key —
+// so the cache would grow without bound (an orphaned entry per edit) if left uncapped. An LRU
+// keeps it bounded like the other UI caches (contents.ts, render.ts both cap at 30).
+const COMMENT_CACHE_CAP = 30;
 const cache = new Map<string, string>();
 
 // Stamp each commentable block-open token with its 1-based source line (1-based
@@ -101,9 +105,18 @@ export function renderMarkdownInline(text: string): string {
 export function renderCommentBody(c: ReviewComment): string {
   const key = `${c.id}:${c.updatedAt}`;
   const cached = cache.get(key);
-  if (cached !== undefined) return cached;
+  if (cached !== undefined) {
+    cache.delete(key); // re-insert → most-recently-used
+    cache.set(key, cached);
+    return cached;
+  }
   if (!mdComment) return `<p>${esc(c.body)}</p>`; // not ready yet — don't cache the fallback
   const html = DOMPurify.sanitize(mdComment.render(c.body || ""));
   cache.set(key, html);
+  while (cache.size > COMMENT_CACHE_CAP) {
+    const oldest = cache.keys().next().value;
+    if (oldest === undefined) break;
+    cache.delete(oldest);
+  }
   return html;
 }

src/ui/render.ts

@@ -69,7 +69,14 @@ const RENDER_INDICATOR_MIN_LINES = 400;
 // otherwise a fast cached re-open skips the badge to avoid an appear-then-vanish flash.
 export function deferRender(forceIfBig = false) {
   const f = currentFile();
-  const lc = (s?: string) => (s?.match(/\n/g)?.length ?? 0) + 1;
+  // Count newlines with a loop rather than s.match(/\n/g): match allocates a full array of every
+  // newline over what can be a multi-MB file, on every file switch — the count is all we need.
+  const lc = (s?: string) => {
+    if (!s) return 1;
+    let n = 1;
+    for (let i = 0; i < s.length; i++) if (s.charCodeAt(i) === 10) n++;
+    return n;
+  };
   // Contents now arrive via a per-file fetch (see contents.ts). When they aren't warm yet the
   // upcoming render() gates on that fetch, so show the indicator; when they are, decide on size
   // as before. peekContents never fetches, so this stays synchronous.

src/ui/selection-derive.test.ts

@@ -0,0 +1,22 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { sideFromLineType } from "./selection-derive.js";
+
+test("a drag ending on a deletion row (unified view) tags the selection deletions", () => {
+  // Stacked/unified view carries both sides in one column, so geometry can't tell the side —
+  // @pierre's data-line-type on the row does. A deletion row → "deletions".
+  assert.equal(sideFromLineType("deletion"), "deletions");
+  assert.equal(sideFromLineType("change-deletion"), "deletions");
+});
+
+test("an addition row tags the selection additions", () => {
+  assert.equal(sideFromLineType("addition"), "additions");
+  assert.equal(sideFromLineType("change-addition"), "additions");
+});
+
+test("a context or unknown row yields null so the caller can fall back to geometry", () => {
+  assert.equal(sideFromLineType("context"), null);
+  assert.equal(sideFromLineType(""), null);
+  assert.equal(sideFromLineType(null), null);
+  assert.equal(sideFromLineType(undefined), null);
+});

src/ui/selection-derive.ts

@@ -0,0 +1,13 @@
+import type { Side } from "./types";
+
+// @pierre tags each rendered row (both its gutter cell and its code cell) with a data-line-type
+// — "addition", "deletion", "change-addition", "context", etc. In Stacked (unified) view one
+// column carries both sides, so horizontal pointer geometry can't tell which side a dragged row
+// belongs to; the row's own type can. Returns the side a data-line-type names, or null when it
+// names neither (context / unknown) so the caller can fall back to geometry.
+export function sideFromLineType(lineType: string | null | undefined): Side | null {
+  if (!lineType) return null;
+  if (lineType.includes("deletion")) return "deletions";
+  if (lineType.includes("addition")) return "additions";
+  return null;
+}