Add a Code Review setting, with accessibility, budget and regression gates

README.md

@@ -94,7 +94,7 @@ modex/
   when it is unused, or retries in place while another turn is running or opening.
   A second failure shows the card with both attempts recorded. Early send/retry rejections
   (for example, a busy thread) return to the caller without adding transcript items.
-- **Code Review development preview.** The read-only PR workspace includes remote diffs bound to the selected base/head revision, grouped files, filtering, line anchors and bounded context expansion. Missing or truncated patches remain explicit. This preview stays behind the existing Code Review development flag while reference and human acceptance are open; see [the design](docs/design/code-review.md).
+- **Code Review.** A PR inbox, overview and remote diffs bound to the selected base/head revision, through your own `gh` login; a read-only Claude review and questions; comments, reviews and merge behind an explicit preview. It is on by default from the v0.5.0 candidate. Settings → General → "Show Code Review in the rail" turns it off. Live and human acceptance are still open; see [the design](docs/design/code-review.md).
 - **Tabbed workspace.** Review working-tree changes with numbered diffs, a filterable file
   tree, and confirmed per-file discard. Open Files, Terminal, Canvas, or browser tabs beside the chat;
   use Shift + Command + B for a new tab or Shift + Command + F for full view. Escape exits

apps/desktop/e2e/code-review-a11y.spec.ts

@@ -55,44 +55,133 @@ for (const theme of Object.keys(THEMES) as Theme[]) {
   });
 }
 
+/**
+ * C76: every control CR-3 to CR-7 put on the surface, identified as a person meets it. A test id where the slice
+ * gave one, otherwise the accessible name; the numbers name the matrix rows (docs/reviews/2026-11-01-cr8-acceptance-matrix.md).
+ */
+const REQUIRED_IDS = [
+  // CR-3: inbox, tabs and header.
+  "code-review-inbox-menu", "code-review-search", "code-review-row", "code-review-tab-overview", "code-review-close-overview",
+  "code-review-split-toggle", "code-review-pin", "code-review-copy-link", "code-review-open-github",
+  // CR-3 Changes tab; CR-5 tree (C73); CR-6 review and instructions (C27, C28); CR-7 state and merge (C21, C29).
+  "code-review-tab-changes", "code-review-close-changes", "cr-tree-file", "code-review-review", "code-review-instructions", "code-review-state", "code-review-merge",
+];
+const REQUIRED_NAMES: [string, RegExp][] = [
+  // CR-4 overview: the scrollable regions, Open Changes and the activity filter (C36, C47, C48).
+  ["overview column", /^Description and activity$/], ["details column", /^Pull request details$/], ["open Changes", /^Open Changes$/], ["activity filter", /^Activity filter$/],
+  // CR-5 diff: dividers, filter, file sections, menus, line anchors and folded context (C64, C66, C67, C68, C70, C72, C74, C75).
+  ["pane divider", /^Resize the pull request pane$/], ["tree divider", /^Resize the file tree$/], ["file filter", /^Filter remote files$/],
+  ["file collapse", /^Collapse src\/holds\/move\.ts$/], ["file menu", /^File options for src\/holds\/move\.ts$/], ["line anchor", /^(Old|New) line \d+/], ["folded context", /^Expand \d+ unchanged lines/],
+  // CR-7 writes: comment, review, edit and reviewer requests (C31, C35, C38, C43, C55).
+  ["comment", /^Comment$/], ["review", /^Review$/], ["edit", /^Edit pull request$/], ["reviewers", /^Request reviewers$/],
+];
+
+/** One pane has no divider beside it to move. */
+const WIDE_ONLY = new Set(["pane divider"]);
+
 for (const size of [{ width: 1440, height: 900 }, { width: 900, height: 600 }]) {
   test(`Tab reaches every Code Review control with a visible focus ring at ${size.width}×${size.height}`, async () => {
     h = await launchWithGh(baseWorld());
     const { page } = h;
     const used = await fitWindow(h, size.width, size.height);
     await tid(page, "rail-code-review").click();
-    await prRow(page, "example-org/field-guide#91").click();
+    await prRow(page, "example-org/lending#73").click();
     await tid(page, "code-review-split-toggle").click();
-    await expect(tid(page, "code-review-tab-changes")).toContainText("+11");
-    await tid(page, "rail-code-review").focus();
+    await expect(tid(page, "code-review-tab-changes")).toContainText("+8,912");
+    await expect(page.getByRole("button", { name: "Request reviewers" })).toBeVisible();
+    await expect(page.getByRole("combobox", { name: "Activity filter" })).toBeVisible();
+    const expand = page.getByRole("button", { name: /^Expand \d+ unchanged lines/ }).first();
+    // A narrow window keeps one pane of the split (navigation.ts), so the keyboard path visits each view in turn.
+    const onePane = size.width < 1000;
+    if (!onePane) await expect(expand).toBeVisible();
     const order: { id: string; name: string; ring: boolean }[] = [];
-    // The PR and Changes tabs sit in the title bar's strip, outside the surface's region; both count.
-    for (let i = 0; i < 80; i++) {
-      await page.keyboard.press("Tab");
-      const stop = await page.evaluate(() => {
-        const el = document.activeElement as HTMLElement | null;
-        if (!el || !el.closest('[data-testid="code-review"], [data-testid="code-review-tabbar"]')) return null;
-        const style = getComputedStyle(el);
-        let ring = (style.outlineStyle !== "none" && parseFloat(style.outlineWidth) > 0) || style.boxShadow !== "none";
-        // A search field shows focus on its box: the border turns the accent colour (styles.css .sidebar-search).
-        const field = el.closest(".sidebar-search");
-        if (!ring && field) {
-          const probe = document.createElement("span");
-          probe.style.color = "var(--accent)";
-          document.body.append(probe);
-          ring = getComputedStyle(field).borderTopColor === getComputedStyle(probe).color;
-          probe.remove();
-        }
-        return { id: el.dataset.testid ?? el.getAttribute("role") ?? el.tagName.toLowerCase(), name: el.getAttribute("aria-label") ?? el.textContent?.trim().slice(0, 60) ?? "", ring };
-      });
-      if (stop) order.push(stop);
+    const walk = async () => {
+      await tid(page, "rail-code-review").focus();
+      // The PR and Changes tabs sit in the title bar's strip, outside the surface's region; both count.
+      for (let i = 0; i < 240; i++) {
+        await page.keyboard.press("Tab");
+        const stop = await page.evaluate(() => {
+          const el = document.activeElement as HTMLElement | null;
+          if (!el || !el.closest('[data-testid="code-review"], [data-testid="code-review-tabbar"]')) return null;
+          const style = getComputedStyle(el);
+          let ring = (style.outlineStyle !== "none" && parseFloat(style.outlineWidth) > 0) || style.boxShadow !== "none";
+          // A search field shows focus on its box: the border turns the accent colour (styles.css .sidebar-search).
+          const field = el.closest(".sidebar-search");
+          if (!ring && field) {
+            const probe = document.createElement("span");
+            probe.style.color = "var(--accent)";
+            document.body.append(probe);
+            ring = getComputedStyle(field).borderTopColor === getComputedStyle(probe).color;
+            probe.remove();
+          }
+          return { id: el.dataset.testid ?? el.getAttribute("role") ?? el.tagName.toLowerCase(), name: el.getAttribute("aria-label") ?? el.textContent?.trim().slice(0, 60) ?? "", ring };
+        });
+        if (stop) order.push(stop);
+      }
+    };
+    await walk();
+    if (onePane) {
+      await tid(page, "code-review-tab-changes").click();
+      await expect(expand).toBeVisible();
+      await walk();
     }
     record(`tab:${used.width}x${used.height}`, order);
     const reached = new Set(order.map((s) => s.id));
-    for (const id of ["code-review-inbox-menu", "code-review-search", "code-review-row", "code-review-tab-overview", "code-review-close-overview",
-      "code-review-split-toggle", "code-review-pin", "code-review-copy-link", "code-review-open-github", "code-review-review", "code-review-instructions", "code-review-merge"]) {
-      expect(reached.has(id), `${id} reachable by Tab at ${size.width}×${size.height}; order: ${order.map((s) => s.id).join(" → ")}`).toBe(true);
-    }
+    const path = () => order.map((s) => `${s.id}:${s.name}`).join(" → ");
+    for (const id of REQUIRED_IDS) expect(reached.has(id), `${id} reachable by Tab at ${size.width}×${size.height}; order: ${path()}`).toBe(true);
+    for (const [label, name] of REQUIRED_NAMES.filter(([label]) => !onePane || !WIDE_ONLY.has(label))) expect(order.some((s) => name.test(s.name)), `${label} reachable by Tab at ${size.width}×${size.height}; order: ${path()}`).toBe(true);
     expect(order.filter((s) => !s.ring).map((s) => s.id), "stops without a visible focus indicator").toEqual([]);
   });
 }
+
+/** The lowest WCAG contrast of any text inside every element the selector matches, skipping inactive controls (WCAG 1.4.3 exempts them), with the text that scored it. */
+const lowest = (page: Page, selector: string) => page.locator(selector).evaluateAll((roots) => {
+  const rgba = (s: string) => (s.match(/[\d.]+/g) ?? []).map(Number);
+  const lum = ([r, g, b]: number[]) => [r!, g!, b!].map((v) => { const c = v / 255; return c <= 0.03928 ? c / 12.92 : ((c + 0.055) / 1.055) ** 2.4; }).reduce((a, c, i) => a + c * [0.2126, 0.7152, 0.0722][i]!, 0);
+  let worst = { ratio: 21, text: "" };
+  for (const root of roots) for (let walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT), node = walker.nextNode(); node; node = walker.nextNode()) {
+    const el = node.parentElement!;
+    const text = node.textContent?.trim() ?? "";
+    if (!text || el.closest('[disabled], [aria-disabled="true"]') || !el.checkVisibility({ opacityProperty: true, visibilityProperty: true })) continue;
+    let bg: number[] = [0, 0, 0];
+    for (let n: Element | null = el; n; n = n.parentElement) { const c = rgba(getComputedStyle(n).backgroundColor); if (c.length === 3 || (c.length === 4 && c[3]! > 0.9)) { bg = c; break; } }
+    const fg = rgba(getComputedStyle(el).color);
+    const [a, b] = [lum(fg), lum(bg)].sort((x, y) => y - x);
+    const ratio = Math.round(((a! + 0.05) / (b! + 0.05)) * 100) / 100;
+    if (ratio < worst.ratio) worst = { ratio, text: `${text.slice(0, 40)} <${el.tagName.toLowerCase()}.${el.className}>` };
+  }
+  return worst;
+});
+
+/** The CR-4 to CR-7 text a reviewer reads, in every theme: details, activity, tree and diff, the review panel, the composers and the merge panel. */
+const SLICE_TEXT: [string, string][] = [
+  ["activity", '[data-testid="cr-activity"]'], ["checks", '[data-testid="cr-checks"]'], ["details", 'aside[aria-label="Pull request details"]'], ["details", 'aside[aria-label="Pull request details"]'],
+  ["diff", '[data-testid="code-review-diff"]'], ["tree", '.cr-tree'],
+  ["review section", '[data-testid="cr-agent"]'], ["composer", '[data-testid="cr-compose"]'],
+];
+for (const theme of Object.keys(THEMES) as Theme[]) {
+  test(`CR-4 to CR-7 text meets AA contrast in the ${THEMES[theme].name} theme`, async () => {
+    h = await launchWithGh(baseWorld());
+    const { page } = h;
+    await fitWindow(h, 1440, 900);
+    await setTheme(page, theme);
+    await tid(page, "rail-code-review").click();
+    await prRow(page, "example-org/lending#73").click();
+    await tid(page, "code-review-split-toggle").click();
+    await expect(tid(page, "cr-tree-file").first()).toBeVisible();
+    const measured: Record<string, { ratio: number; text: string }> = {};
+    for (const [name, selector] of SLICE_TEXT) {
+      await expect(page.locator(selector).first(), name).toBeAttached();
+      measured[name] = await lowest(page, selector);
+    }
+    // Panels open on a click: the review panel and the merge panel.
+    await tid(page, "code-review-review").first().click();
+    await expect(tid(page, "cr-agent-panel").first()).toBeVisible();
+    measured["review panel"] = await lowest(page, '[data-testid="cr-agent-panel"]');
+    await tid(page, "code-review-merge").first().click();
+    await expect(tid(page, "cr-merge").first()).toBeVisible();
+    measured["merge panel"] = await lowest(page, '[data-testid="cr-merge"]');
+    record(`contrast-slices:${theme}`, measured);
+    for (const [name, worst] of Object.entries(measured)) expect(worst.ratio, `${name} in ${theme}: "${worst.text}"`).toBeGreaterThanOrEqual(4.5);
+  });
+}

apps/desktop/e2e/code-review-actions.spec.ts

@@ -64,6 +64,56 @@ test("a comment stays a local draft across a reload, previews its exact destinat
   expect(h.providerStarts()).toEqual([]);
 });
 
+test("a draft stays hidden after sign-out and for another account, and comes back only for its own account (#326)", async () => {
+  const TEXT = "Private note before signing out.";
+  const ADA = { account: "1001", host: "github.com", repoId: "7301", number: 73 };
+  const REVISION = { baseRef: "main", baseSha: "b".repeat(40), headSha: "c".repeat(40) };
+  const status = (p: Page, key: typeof ADA) => p.evaluate(([k, r]) => window.modex!.invoke("code-review:write", { op: "status", requestId: `probe-${Date.now()}`, key: k, revision: r }), [key, REVISION] as const);
+  const refresh = async (p: Page) => {
+    await tid(p, "code-review-inbox-menu").click();
+    await tid(p, "code-review-refresh").click();
+  };
+  h = await launchWithGh(baseWorld());
+  const page = await openLending();
+  await writeComment(page, TEXT);
+  await expect.poll(async () => { const reply = await status(page, ADA) as any; return reply.result.state === "ok" ? reply.result.value.drafts.comment?.body ?? null : null; }).toBe(TEXT);
+
+  // Signed out: the surface says so, the PR closes, and main answers nothing about the draft, though it keeps it on disk.
+  h.world((w) => { w.user = null; });
+  h.switchHosts("nobody");
+  await refresh(page);
+  await expect(tid(page, "code-review-connection-error")).toContainText("Not signed in");
+  await expect(comment(page)).toHaveCount(0);
+  await expect(page.locator("body")).not.toContainText(TEXT);
+  const signedOut = await status(page, ADA) as any;
+  expect(signedOut.result.state).not.toBe("ok");
+  expect(JSON.stringify(signedOut)).not.toContain(TEXT);
+
+  // Another account: neither Ada's key nor the same PR under its own key shows her draft.
+  h.world((w) => { w.user = { id: 2002, login: "grace-fixture" }; });
+  h.switchHosts("grace-fixture");
+  await page.reload();
+  await tid(page, "rail-code-review").click();
+  await expect(tid(page, "code-review-inbox")).toBeVisible();
+  await prRow(page, "example-org/lending#73").click();
+  await expect(comment(page).getByRole("button", { name: "Comment", exact: true })).toBeVisible();
+  await expect(page.locator("body")).not.toContainText(TEXT);
+  const other = await status(page, ADA) as any;
+  expect(other.result.state).not.toBe("ok");
+  expect(JSON.stringify(other)).not.toContain(TEXT);
+  const own = await status(page, { ...ADA, account: "2002" }) as any;
+  expect(JSON.stringify(own)).not.toContain(TEXT);
+
+  // Ada again: the draft was hidden, never lost.
+  h.world((w) => { w.user = { id: 1001, login: "ada-fixture" }; });
+  h.switchHosts("ada-fixture");
+  await page.reload();
+  await tid(page, "rail-code-review").click();
+  await prRow(page, "example-org/lending#73").click();
+  await expect(comment(page).getByRole("textbox", { name: "Comment text" })).toHaveValue(TEXT);
+  expect(h.posts()).toEqual([]);
+});
+
 test("navigating before the draft debounce preserves the latest text", async () => {
   h = await launchWithGh(baseWorld());
   const page = await openLending();

apps/desktop/e2e/code-review-agent.spec.ts

@@ -334,3 +334,65 @@ test("integrated review: populated diff → native fixture finding → confirmed
   expect(runs()).toHaveLength(1);
   expect(h.posts()).toHaveLength(1);
 });
+
+/** The pane's scroller for its content: the main column, or the whole body where the narrow overview stacks. */
+async function overviewScroller(page: Page) {
+  const main = page.locator(".cr-overview-main");
+  const stacked = await main.evaluate((el) => getComputedStyle(el).overflowY === "visible");
+  return stacked ? tid(page, "code-review-overview-body") : main;
+}
+
+/** C61: the control is inside its scroller's visible box and nothing is painted over it. */
+async function expectClear(page: Page, control: ReturnType<Page["locator"]>, name: string): Promise<void> {
+  const scroller = await overviewScroller(page);
+  await control.focus();
+  await expect(control, `${name} keeps the focus`).toBeFocused();
+  await expect.poll(async () => control.evaluate((el, box) => {
+    const r = el.getBoundingClientRect();
+    const hit = document.elementFromPoint(r.left + r.width / 2, r.top + r.height / 2);
+    return r.height > 0 && r.top >= box.top - 0.5 && r.bottom <= box.bottom + 0.5 && !!hit && (el === hit || el.contains(hit));
+  }, await scroller.evaluate((el) => { const b = el.getBoundingClientRect(); return { top: b.top, bottom: b.bottom }; })), `${name} is on screen and uncovered`).toBe(true);
+}
+
+/**
+ * C61: brought into view, the last thing in the overview's main column ends inside the scroller, uncovered. Where the
+ * narrow overview stacks, the sidebar follows the main column, so the item is scrolled to rather than the scroller's end.
+ */
+async function expectLastItemClear(page: Page, label: string): Promise<void> {
+  const scroller = await overviewScroller(page);
+  const measure = () => scroller.evaluate((el) => {
+    const last = document.querySelector(".cr-overview-main section:last-of-type > :last-child")!;
+    last.scrollIntoView({ block: "end" });
+    const box = el.getBoundingClientRect(), r = last.getBoundingClientRect();
+    const hit = document.elementFromPoint(r.left + r.width / 2, r.bottom - 2);
+    return { scrollerHeight: Math.round(box.height), overrun: Math.round(r.bottom - box.bottom), covered: !hit || !(last === hit || last.contains(hit)), hit: hit?.className ?? null };
+  });
+  await expect.poll(async () => { const m = await measure(); return m.scrollerHeight >= 120 && m.overrun <= 0 && !m.covered ? "clear" : JSON.stringify(m); }, `${label}: the last item clears the bottom, and the pane keeps a usable height`).toBe("clear");
+}
+
+for (const [width, height] of [[900, 600], [1440, 900]] as const) {
+  test(`C61: the inline Ask section and the last activity item stay reachable at ${width}×${height}, with the review panel shut and open`, async () => {
+    h = await launchWithGh(baseWorld());
+    installClaude("reply");
+    const page = await openLending();
+    // fitWindow caps the height at the display's work area, so a hosted runner's small screen tests a shorter window.
+    await fitWindow(h, width, height);
+    const ask = tid(page, "cr-agent-ask");
+    await expect(ask).toBeVisible();
+
+    await expectClear(page, ask.getByRole("textbox"), "Ask's text box");
+    await ask.getByRole("textbox").fill("A question long enough to matter.\n".repeat(8));
+    await expectClear(page, ask.getByRole("button", { name: "Ask", exact: true }), "Ask");
+    await expectLastItemClear(page, "panel shut");
+
+    await tid(page, "code-review-review").first().click();
+    await expect(tid(page, "cr-agent-panel")).toBeVisible();
+    await expectClear(page, ask.getByRole("textbox"), "Ask's text box with the panel open");
+    await expectClear(page, ask.getByRole("button", { name: "Ask", exact: true }), "Ask with the panel open");
+    await expectLastItemClear(page, "panel open");
+    // The panel itself: its controls fit, or scroll, inside the window rather than off its bottom edge.
+    const start = tid(page, "cr-agent-start");
+    await start.scrollIntoViewIfNeeded();
+    await expect(start).toBeInViewport({ ratio: 1 });
+  });
+}