diff --git a/README.md b/README.md index 2dcb2e89..0b64e0b6 100644 --- a/README.md +++ b/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 diff --git a/apps/desktop/e2e/code-review-a11y.spec.ts b/apps/desktop/e2e/code-review-a11y.spec.ts index 783975f2..57a8955e 100644 --- a/apps/desktop/e2e/code-review-a11y.spec.ts +++ b/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 = {}; + 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); + }); +} diff --git a/apps/desktop/e2e/code-review-actions.spec.ts b/apps/desktop/e2e/code-review-actions.spec.ts index 4afc5b98..2f6490e0 100644 --- a/apps/desktop/e2e/code-review-actions.spec.ts +++ b/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(); diff --git a/apps/desktop/e2e/code-review-agent.spec.ts b/apps/desktop/e2e/code-review-agent.spec.ts index 936bfb77..bea849cd 100644 --- a/apps/desktop/e2e/code-review-agent.spec.ts +++ b/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, name: string): Promise { + 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 { + 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 }); + }); +} diff --git a/apps/desktop/e2e/code-review-budgets.spec.ts b/apps/desktop/e2e/code-review-budgets.spec.ts new file mode 100644 index 00000000..97f13d11 --- /dev/null +++ b/apps/desktop/e2e/code-review-budgets.spec.ts @@ -0,0 +1,74 @@ +import { test, expect } from "@playwright/test"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { tid } from "./support.js"; +import { baseWorld, LENDING, launchWithGh, prRow, type Harness } from "./code-review-support.js"; +import { ghFile, ghPull } from "../test/fixtures/code-review-gh-world.js"; + +/** + * CR-8 (#327): regression budgets for opening the inbox, an overview and a large diff, through the production + * read service in real Electron with the synthetic gh. Each is the median of five launches, from the click to the + * content on screen. + * + * The budgets leave headroom for hosted runners, which have run the same code several times slower than this Mac + * (docs/status.md, the 500 ms transcript gate). They catch a change that makes these paths several times slower; + * they are not a claim about GitHub's latency, since the synthetic gh answers at once. + * + * MODEX_MEASURE= writes the times as JSON before anything is asserted. MODEX_BUDGET_GH_DELAY_MS slows every gh + * answer by that much, which is how each budget was shown to fail (docs/reviews/2026-11-02-v0.5.0-candidate.md). + */ +const BUDGET_MS = { inbox: 5000, overview: 1500, thousandFiles: 2500, largePatch: 3000 }; +const LAUNCHES = 5; +const median = (values: number[]) => [...values].sort((a, b) => a - b)[Math.floor(values.length / 2)]!; + +function world() { + const w = baseWorld(); + const files = Array.from({ length: 1000 }, (_, i) => ghFile(`src/generated/module-${String(i).padStart(4, "0")}.ts`, `@@ -0,0 +1 @@\n+export const value${i} = ${i};`, 1, 0, "added")); + w.routes["repos/example-org/lending/pulls/73"] = ghPull(LENDING, 73, "A thousand files", { files: 1000, additions: 1000, deletions: 0 }); + w.routes["repos/example-org/lending/pulls/73/files"] = files; + // 3 MiB of added text in one patch: 49,152 lines of 63 characters plus the newline. + const big = `@@ -0,0 +1,49152 @@\n${Array.from({ length: 49152 }, () => "+" + "x".repeat(62)).join("\n")}`; + w.routes["repos/example-org/lending/pulls/70"] = ghPull(LENDING, 70, "One very large patch", { files: 1, additions: 49152, deletions: 0 }); + w.routes["repos/example-org/lending/pulls/70/files"] = [ghFile("data/large.txt", big, 49152, 0, "added")]; + return w; +} + +/** Puts a sleep in front of the synthetic gh, for the failure proof only. */ +function slowGh(h: Harness, ms: number): void { + const gh = path.join(h.home, "bin", "gh"), real = `${gh}-real`; + fs.renameSync(gh, real); + fs.writeFileSync(gh, `#!/bin/sh\nsleep ${ms / 1000}\nexec '${real}' "$@"\n`, { mode: 0o755 }); +} + +test("opening the inbox, an overview and a large diff stay within their budgets", async () => { + const delay = Number(process.env.MODEX_BUDGET_GH_DELAY_MS ?? 0); + test.setTimeout(delay ? 900_000 : 300_000); + const data = world(); + const runs: Record = { inbox: [], overview: [], thousandFiles: [], largePatch: [] }; + for (let i = 0; i < LAUNCHES; i++) { + const h = await launchWithGh(data); + try { + const { page } = h; + if (delay) slowGh(h, delay); + const timed = async (name: keyof typeof BUDGET_MS, act: () => Promise, done: () => Promise) => { + const t = Date.now(); + await act(); + await done(); + runs[name].push(Date.now() - t); + }; + await timed("inbox", () => tid(page, "rail-code-review").click(), () => expect(prRow(page, "example-org/lending#73"), "inbox").toBeVisible({ timeout: 120_000 })); + await timed("overview", () => prRow(page, "example-org/lending#73").click(), () => expect(tid(page, "cr-description"), "overview").toBeVisible({ timeout: 120_000 })); + await timed("thousandFiles", () => tid(page, "code-review-tab-changes").click(), () => expect(page.getByRole("button", { name: "Collapse src/generated/module-0000.ts" }), "1,000-file diff").toBeVisible({ timeout: 300_000 })); + await prRow(page, "example-org/lending#70").click(); + await expect(tid(page, "code-review-pr-id").first()).toHaveText("lending #70", { timeout: 120_000 }); + await timed("largePatch", () => tid(page, "code-review-tab-changes").click(), () => expect(page.getByRole("button", { name: "Collapse data/large.txt" }), "3 MiB patch").toBeVisible({ timeout: 120_000 })); + } finally { + await h.close(); + } + } + const medians = Object.fromEntries(Object.entries(runs).map(([name, values]) => [name, median(values)])) as Record; + const file = process.env.MODEX_MEASURE; + if (file) fs.writeFileSync(file, JSON.stringify({ measuredAt: new Date().toISOString(), delay, machine: { cpu: os.cpus()[0]?.model, cores: os.cpus().length, loadAverage: os.loadavg() }, medians, runs, budgets: BUDGET_MS }, null, 2) + "\n"); + for (const name of Object.keys(BUDGET_MS) as (keyof typeof BUDGET_MS)[]) expect.soft(medians[name], `${name} median of ${runs[name].join(", ")} ms`).toBeLessThanOrEqual(BUDGET_MS[name]); +}); diff --git a/apps/desktop/e2e/code-review-diff.spec.ts b/apps/desktop/e2e/code-review-diff.spec.ts index 16a9800a..46632577 100644 --- a/apps/desktop/e2e/code-review-diff.spec.ts +++ b/apps/desktop/e2e/code-review-diff.spec.ts @@ -43,7 +43,6 @@ async function open(files?: PrFile[], hold: string | null = null) { return { requestId: req.requestId, op: req.op, scope: { account: req.key?.account ?? data.account, key: req.key ?? null, revision: req.revision ?? null }, result }; }); }, { files: files ?? R4.files.value.files, overview: R4.overview, alternate: R1.overview, connection: R1.connection, inbox: R1.inbox, at: NOW, account: R4.overview.key.account, hold }); - await page.evaluate(() => localStorage.setItem("modex.codeReview", "on")); await page.reload(); await tid(page, "rail-code-review").click(); await page.locator('[data-testid="code-review-row"][data-pr="example-org/field-guide#91"]').click(); @@ -452,3 +451,96 @@ test("context from a different commit is refused even when revision and path mat await expect(page.getByRole("alert")).toContainText("did not match the requested file"); await expect(tid(page, "code-review-diff")).not.toContainText("remote context"); }); + +test("diff controls: wrapping and highlighting change rendered code and horizontal scroll stays per file", async () => { + const files = stressFiles(2).map((file, i) => ({ ...file, path: `wide-${i}.ts`, status: "modified" as const, additions: 1, deletions: 1, + patch: { kind: "text" as const, patch: `@@ -1 +1 @@\n-old\n+const value = "${"long-content-".repeat(70)}";` } })); + await open(files); + await resizeWindow(app, page, 1440, 900); + const patches = page.locator(".cr-patch"); + await expect(patches).toHaveCount(2); + const added = patches.first().locator('[data-line="add"]'); + const code = added.locator("code"); + const wrapped = await code.boundingBox(); + expect(wrapped!.height).toBeGreaterThan(40); + const color = await added.evaluate(el => getComputedStyle(el).backgroundColor); + expect(color).not.toBe("rgba(0, 0, 0, 0)"); + await page.getByRole("checkbox", { name: "Highlight changes", exact: true }).uncheck(); + await expect.poll(() => added.evaluate(el => getComputedStyle(el).backgroundColor)).toBe("rgba(0, 0, 0, 0)"); + await page.getByRole("checkbox", { name: "Highlight changes", exact: true }).check(); + await expect.poll(() => added.evaluate(el => getComputedStyle(el).backgroundColor)).toBe(color); + await page.getByRole("checkbox", { name: "Wrap lines", exact: true }).uncheck(); + await expect.poll(() => code.evaluate(el => el.getBoundingClientRect().height), { timeout: 5_000 }).toBeLessThan(wrapped!.height); + await expect.poll(() => patches.first().evaluate(el => el.scrollWidth - el.clientWidth)).toBeGreaterThan(500); + const patchBox = await patches.first().boundingBox(); + await page.mouse.move(patchBox!.x + 120, patchBox!.y + 20); + await page.mouse.wheel(400, 0); + await expect.poll(() => patches.first().evaluate(el => el.scrollLeft)).toBeGreaterThan(0); + expect(await patches.nth(1).evaluate(el => el.scrollLeft)).toBe(0); + expect(await tid(page, "code-review-diff").evaluate(el => el.scrollLeft)).toBe(0); + await page.getByRole("checkbox", { name: "Wrap lines", exact: true }).check(); + await expect.poll(() => patches.first().evaluate(el => el.scrollWidth - el.clientWidth)).toBeLessThanOrEqual(1); +}); + +test("diff controls: file tree divider resizes with keyboard and pointer and persists on reload", async () => { + await open(); + await resizeWindow(app, page, 1440, 900); + const divider = page.getByRole("separator", { name: "Resize the file tree", exact: true }); + const tree = tid(page, "code-review-tree"); + const width = async () => (await tree.boundingBox())!.width; + await expect(divider).toBeVisible(); + const initial = await width(); + await divider.focus(); + await page.keyboard.press("ArrowLeft"); + await expect.poll(width).toBe(initial + 8); + await expect(divider).toBeFocused(); + const box = await divider.boundingBox(); + await page.mouse.move(box!.x + box!.width / 2, box!.y + 80); + await page.mouse.down(); + await page.mouse.move(box!.x + box!.width / 2 - 48, box!.y + 80, { steps: 6 }); + await page.mouse.up(); + await expect.poll(width).toBe(initial + 56); + await page.reload(); + await tid(page, "rail-code-review").click(); + await page.locator('[data-testid="code-review-row"][data-pr="example-org/field-guide#91"]').click(); + await tid(page, "code-review-tab-changes").click(); + await expect.poll(width).toBe(initial + 56); + await divider.focus(); + await page.keyboard.press("Enter"); + await expect.poll(width).toBe(initial); +}); + +test("diff controls: file menu copies the remote path and offers a GitHub link without local Discard", async () => { + await open(); + await app.evaluate(({ ipcMain }) => { + (globalThis as any).__copiedPaths = []; + ipcMain.removeHandler("clipboard:write"); + ipcMain.handle("clipboard:write", (_event, request) => { (globalThis as any).__copiedPaths.push(request.text); }); + }); + const section = tid(page, "cr-file-section").first(); + const menu = section.locator(".cr-file-menu"); + await menu.locator("summary").focus(); + await page.keyboard.press("Enter"); + await expect(menu).toHaveAttribute("open", ""); + await expect(menu.getByRole("link", { name: "Open in GitHub", exact: true })).toHaveAttribute("href", "https://github.com/example-org/field-guide/pull/91/files"); + await expect(menu.getByRole("button", { name: /Discard/ })).toHaveCount(0); + await menu.getByRole("button", { name: "Copy path", exact: true }).focus(); + await page.keyboard.press("Enter"); + await expect.poll(() => app.evaluate(() => (globalThis as any).__copiedPaths)).toEqual(["src/holds/move.ts"]); + expect(await app.evaluate(() => (globalThis as any).__diff.local)).toEqual([]); +}); + +test("diff controls: more unchanged context discloses uncertainty and loads only the selected remote file", async () => { + await open(); + const section = tid(page, "cr-file-section").first(); + const more = section.getByRole("button", { name: /More unchanged context may be available/ }); + await more.focus(); + await page.keyboard.press("Enter"); + await expect(section).toContainText("remote context"); + const requests = (await log()).filter((r: any) => r.op === "context"); + expect(requests).toHaveLength(1); + expect(requests[0].path).toBe("src/holds/move.ts"); + expect(requests[0].revision).toEqual(R4.overview.revision); + expect(requests[0].key).toEqual(R4.overview.key); + expect(await app.evaluate(() => (globalThis as any).__diff.local)).toEqual([]); +}); diff --git a/apps/desktop/e2e/code-review-enablement.spec.ts b/apps/desktop/e2e/code-review-enablement.spec.ts new file mode 100644 index 00000000..05180fd2 --- /dev/null +++ b/apps/desktop/e2e/code-review-enablement.spec.ts @@ -0,0 +1,80 @@ +import { test, expect } from "@playwright/test"; +import fs from "node:fs"; +import { tid } from "./support.js"; +import { baseWorld, launchWithGh, prRow, type Harness } from "./code-review-support.js"; + +/** + * The Code Review setting (#319, #327): on by default in this build, off as a visible rollback in Settings → General, + * and an old explicit off kept. Turning it off hides the surface; it does not touch main's records. + */ +let h: Harness; +test.afterEach(async () => { await h?.close(); }); + +async function general() { + await tid(h.page, "open-settings").click(); + await tid(h.page, "settings-nav").getByRole("button", { name: "General" }).click(); + return tid(h.page, "code-review-in-rail"); +} + +test("Code Review is on by default: the rail shows it, Settings says so, and the inbox opens", async () => { + h = await launchWithGh(baseWorld()); + const { page } = h; + await expect(tid(page, "rail-code-review")).toBeVisible(); + await tid(page, "rail-code-review").click(); + await expect(prRow(page, "example-org/lending#73")).toBeVisible(); + await expect(await general()).toBeChecked(); +}); + +test("turning it off is the rollback: the surface leaves at once, survives a relaunch, keeps a draft, and can come back", async () => { + h = await launchWithGh(baseWorld()); + let { page } = h; + await tid(page, "rail-code-review").click(); + await prRow(page, "example-org/lending#73").click(); + const region = () => h.page.getByRole("region", { name: "Comment on this pull request" }); + await region().getByRole("button", { name: "Comment", exact: true }).click(); + await region().getByRole("textbox", { name: "Comment text" }).fill("Keep me through the rollback."); + await expect.poll(() => { try { return fs.readFileSync(`${h.home}/review-actions/state.json`, "utf8").includes("Keep me through the rollback."); } catch { return false; } }).toBe(true); + + const box = await general(); + await box.uncheck(); + await page.keyboard.press("Escape"); + await expect(tid(page, "rail-code-review")).toHaveCount(0); + await expect(tid(page, "code-review")).not.toBeVisible(); + + await h.restart(); + page = h.page; + await expect(tid(page, "open-settings")).toBeVisible(); + await expect(tid(page, "rail-code-review")).toHaveCount(0); + const calls = h.calls().length; + expect(fs.readFileSync(`${h.home}/review-actions/state.json`, "utf8")).toContain("Keep me through the rollback."); + + const again = await general(); + await expect(again).not.toBeChecked(); + await again.check(); + await page.keyboard.press("Escape"); + await tid(page, "rail-code-review").click(); + await prRow(page, "example-org/lending#73").click(); + await expect(region().getByRole("textbox", { name: "Comment text" })).toHaveValue("Keep me through the rollback."); + expect(h.calls().length).toBeGreaterThan(calls); + expect(h.posts()).toEqual([]); +}); + +test("an explicit off on the old switch stays off, an old on stays on, and a later choice wins", async () => { + h = await launchWithGh(baseWorld()); + let { page } = h; + await page.evaluate(() => { localStorage.setItem("modex.codeReview", "off"); localStorage.removeItem("modex.layout"); }); + await page.reload(); + await expect(tid(page, "open-settings")).toBeVisible(); + await expect(tid(page, "rail-code-review")).toHaveCount(0); + const box = await general(); + await expect(box).not.toBeChecked(); + await box.check(); + await page.keyboard.press("Escape"); + await expect(tid(page, "rail-code-review")).toBeVisible(); + await page.reload(); + await expect(tid(page, "rail-code-review")).toBeVisible(); + + await page.evaluate(() => { localStorage.setItem("modex.codeReview", "on"); localStorage.removeItem("modex.layout"); }); + await page.reload(); + await expect(tid(page, "rail-code-review")).toBeVisible(); +}); diff --git a/apps/desktop/e2e/code-review-ipc.spec.ts b/apps/desktop/e2e/code-review-ipc.spec.ts index bd18621f..7abbff28 100644 --- a/apps/desktop/e2e/code-review-ipc.spec.ts +++ b/apps/desktop/e2e/code-review-ipc.spec.ts @@ -71,7 +71,6 @@ test("real Electron IPC reads synthetic inbox, distinct PRs and revision-bound f { path: "src/holds/move.ts", patch: { kind: "text" } }, { path: "assets/shelf.png", patch: { kind: "omitted" } }, ] } }); // CR-5: drive the populated Changes pane through the production read path too. - await page.evaluate(() => localStorage.setItem("modex.codeReview", "on")); await page.reload(); await tid(page, "rail-code-review").click(); await page.locator('[data-testid="code-review-row"][data-pr="example-org/lending#73"]').click(); diff --git a/apps/desktop/e2e/code-review-navigation.spec.ts b/apps/desktop/e2e/code-review-navigation.spec.ts index 3c471c81..d5beb43e 100644 --- a/apps/desktop/e2e/code-review-navigation.spec.ts +++ b/apps/desktop/e2e/code-review-navigation.spec.ts @@ -122,7 +122,6 @@ async function open(data: Served = served()): Promise { ({ home, repo } = seedHome()); ({ app, page } = await launch(home)); await serve(app, data); - await page.evaluate(() => localStorage.setItem("modex.codeReview", "on")); await page.reload(); await expect(tid(page, "rail-code-review")).toBeVisible(); } diff --git a/apps/desktop/e2e/code-review-overview.spec.ts b/apps/desktop/e2e/code-review-overview.spec.ts index 4b32dcf4..86311181 100644 --- a/apps/desktop/e2e/code-review-overview.spec.ts +++ b/apps/desktop/e2e/code-review-overview.spec.ts @@ -42,7 +42,6 @@ async function open(hostile = false, mode = "normal") { return { requestId: req.requestId, op: req.op, scope: { account: req.key?.account ?? "1001", key: req.key ?? null, revision: req.revision ?? null }, result }; }); }, { R1, R2, R4, NOW, hostile, mode }); - await page.evaluate(() => localStorage.setItem("modex.codeReview", "on")); await page.reload(); await tid(page, "rail-code-review").click(); await page.locator('[data-testid="code-review-row"][data-pr="example-org/lending#73"]').first().click(); @@ -67,6 +66,35 @@ test("the loaded repository identity updates header and links and survives an un await expect(id).toHaveText("lending-renamed #73"); await expect(page.getByTestId("code-review-open-github")).toHaveAttribute("href", url); }); +test("activity opening and comment link preserve their identity and keyboard focus", async () => { + await open(); + await app.evaluate(({ ipcMain }) => { + (globalThis as any).__copiedCommentLinks = []; + ipcMain.removeHandler("clipboard:write"); + ipcMain.handle("clipboard:write", (_event, request) => { + (globalThis as any).__copiedCommentLinks.push(request.text); + }); + }); + const opening = page.locator('[data-activity-id="opened:opened-73"]'); + await expect(opening.locator("strong")).toHaveText("ada-fixture"); + await expect(opening).toContainText("opened this pull request"); + await expect(opening.locator("time")).toHaveAttribute("datetime", "2026-10-18T15:44:00Z"); + const card = page.locator('[data-activity-id="comment:comment-9001"]'); + await expect(card.locator("header strong")).toHaveText("preview-bot[bot]"); + await expect(card).toContainText("Comment content"); + const actions = card.getByRole("button", { name: "Comment actions", exact: true }); + await actions.focus(); + await page.keyboard.press("Enter"); + const copy = page.getByRole("menuitem", { name: "Copy comment link", exact: true }); + await copy.focus(); + await page.keyboard.press("Enter"); + await expect.poll(() => app.evaluate(() => (globalThis as any).__copiedCommentLinks)).toEqual([ + "https://github.com/example-org/lending/pull/73#issuecomment-9001", + ]); + await expect(card.getByRole("status")).toHaveText("Comment link copied"); + await expect(actions).toBeFocused(); + await expect(page.getByRole("menu", { name: "Comment actions", exact: true })).toHaveCount(0); +}); test("R1 metadata and R2 pagination stay truthful, deduplicated and keyboard accessible", async () => { await open(); await expect(page.getByRole("heading", { name: R1.overview.title, exact: true })).toBeVisible(); diff --git a/apps/desktop/e2e/code-review-support.ts b/apps/desktop/e2e/code-review-support.ts index f1296975..eaf58e2c 100644 --- a/apps/desktop/e2e/code-review-support.ts +++ b/apps/desktop/e2e/code-review-support.ts @@ -32,6 +32,8 @@ export interface Harness { posts: () => { endpoint: string; body: Record; id: number }[]; /** Stops the app as a crash would leave it (no clean shutdown of its gh calls) and starts it again on the same home. */ relaunch: () => Promise; + /** Quits normally, so everything the app saves on the way out is saved, and starts again on the same home. */ + restart: () => Promise; close: () => Promise; } @@ -68,12 +70,11 @@ export async function launchWithGh(initial: GhWorld): Promise { const inner = await page.evaluate(() => [window.innerWidth, window.innerHeight]); return native !== null && native[0]! > 0 && native[0] === inner[0] && native[1] === inner[1]; }, { timeout: 15_000, intervals: [100] }).toBe(true); - // A built app hides the unfinished destination behind its local switch (src/renderer/code-review/feature.ts). - await page.evaluate(() => localStorage.setItem("modex.codeReview", "on")); // Row ages and captures read the renderer's clock: pin it to the fixtures' (test/fixtures/code-review.ts NOW). await page.clock.setFixedTime(new Date("2026-10-18T20:41:00Z")); await page.reload(); - await expect(tid(page, "rail-code-review")).toBeVisible(); + // Code Review's own button may be off (e2e/code-review-enablement.spec.ts); Settings is always in the rail. + await expect(tid(page, "open-settings")).toBeVisible(); return { app, page }; }; let { app, page } = await start(); @@ -93,6 +94,11 @@ export async function launchWithGh(initial: GhWorld): Promise { ({ app, page } = await start()); harness.app = app; harness.page = page; }, + restart: async () => { + await app.close().catch(() => {}); + ({ app, page } = await start()); + harness.app = app; harness.page = page; + }, close: async () => { await app.close().catch(() => {}); for (const dir of [home, repo]) fs.rmSync(dir, { recursive: true, force: true }); diff --git a/apps/desktop/e2e/human-requests.spec.ts b/apps/desktop/e2e/human-requests.spec.ts index c5035e44..3952bd52 100644 --- a/apps/desktop/e2e/human-requests.spec.ts +++ b/apps/desktop/e2e/human-requests.spec.ts @@ -229,6 +229,68 @@ test("Codex questions: the card answers by its schema, the CLI hears once, the s } }); +test("Codex questions: a draft and its attachments survive the card being shown and answered (#276)", async () => { + const { home } = seedHome({ default_backend: "codex", default_mode: "agent" }); + const logs = fs.mkdtempSync(path.join(os.tmpdir(), "modex-e2e-codex-questions-")); + const statePath = path.join(home, "app/state.json"); + const state = JSON.parse(fs.readFileSync(statePath, "utf8")); + state.settings.codex_bin = fixture(logs); + fs.writeFileSync(statePath, JSON.stringify(state)); + let app: ElectronApplication | undefined; + try { + const opened = await launch(home); app = opened.app; + const page = opened.page; + await send(page, "deploy it"); + const first = items(page, "question").first(); + await first.getByRole("radio", { name: "staging" }).check(); + await first.getByLabel("Token").fill("s3cr3t"); + await tid(first, "question-submit").click(); + await expect(currentRow(page)).toHaveAttribute("data-status", "idle"); + + // Write the next message and attach a file while nothing runs (attaching is refused during a turn). + const draft = " the follow-up I am still writing\n second line "; + const input = tid(page, "composer-input"); + await input.fill(draft); + await page.evaluate(() => { + const data = new DataTransfer(); + data.items.add(new File(["kept bytes"], "kept.txt", { type: "text/plain" })); + document.querySelector('[data-testid="composer-input"]')!.dispatchEvent(new ClipboardEvent("paste", { clipboardData: data, bubbles: true, cancelable: true })); + }); + const chips = tid(page, "composer-attachments"); + await expect(chips).toContainText("kept.txt"); + + // A turn this composer did not send, as from the iPhone, asks a question while the draft sits there. + const threadId = (await currentRow(page).getAttribute("data-thread-id"))!; + await page.evaluate((id) => window.modex!.invoke("thread:send", { threadId: id, text: "deploy again" }), threadId); + const card = items(page, "question").last(); + await expect(tid(card, "question-submit")).toBeVisible(); + await expect(currentRow(page)).toHaveAttribute("data-status", "waiting"); + await expect(input).toHaveValue(draft); + await expect(chips).toContainText("kept.txt"); + + // A refused answer and a good one both leave the composer as written. + await card.getByRole("radio", { name: "staging" }).check(); + await tid(card, "question-submit").click(); + await expect(tid(card, "question-error")).toHaveText("Token needs an answer."); + await expect(input).toHaveValue(draft); + await expect(chips).toContainText("kept.txt"); + await card.getByLabel("Token").fill("s3cr3t"); + await tid(card, "question-submit").click(); + await expect(tid(card, "question-answer")).toHaveText(["staging", "(hidden)"]); + await expect(currentRow(page)).toHaveAttribute("data-status", "idle"); + await expect(input).toHaveValue(draft); + await expect(chips).toContainText("kept.txt"); + expect(replies(logs)).toHaveLength(2); + + // The draft was never sent: the only user messages are the two that started the turns. + await expect(items(page, "user")).toHaveCount(2); + } finally { + await app?.close(); + fs.rmSync(home, { recursive: true, force: true }); + fs.rmSync(logs, { recursive: true, force: true }); + } +}); + test("with the journal on, a restart shows answered and stopped cards inert, says when an answer was never confirmed, and re-sends nothing", async () => { const { home } = seedHome({ default_backend: "codex", default_mode: "agent" }); const logs = fs.mkdtempSync(path.join(os.tmpdir(), "modex-e2e-codex-questions-")); diff --git a/apps/desktop/src/renderer/App.tsx b/apps/desktop/src/renderer/App.tsx index 2d0dabf6..02138e94 100644 --- a/apps/desktop/src/renderer/App.tsx +++ b/apps/desktop/src/renderer/App.tsx @@ -37,7 +37,6 @@ import { PANEL_WIDTHS, usePanelSizing } from "./panel-sizing"; import { useLayout } from "./layout"; import { applyTheme, restoreTheme } from "./theme"; import { applyItemEvent, type ItemEvent } from "./transcript"; -import { codeReviewEnabled } from "./code-review/feature.js"; import type { ReviewNavigation } from "./code-review/CodeReviewView"; /** ⌃` shows or hides the thread's terminal. xterm maps no byte to it, so it reaches this handler even from inside the shell. */ @@ -51,7 +50,7 @@ const PluginSurface = lazy(() => import("./plugins/PluginSurface").then(m => ({ const SpaceView = lazy(() => import("./space/SpaceView").then(m => ({ default: m.SpaceView }))); const CodeReviewView = lazy(() => import("./code-review/CodeReviewView")); -export function App({ persistPreferences = true, spaceEnabled = true, analyticsEnabled = true, suspended = false }: { persistPreferences?: boolean; spaceEnabled?: boolean; analyticsEnabled?: boolean; suspended?: boolean } = {}) { +export function App({ persistPreferences = true, spaceEnabled = true, analyticsEnabled = true, codeReviewEnabled = true, suspended = false }: { persistPreferences?: boolean; spaceEnabled?: boolean; analyticsEnabled?: boolean; codeReviewEnabled?: boolean; suspended?: boolean } = {}) { const updater = useUpdateState(); const updateBlocked = ["preparing", "staged", "uncertain", "recovery"].includes(updater.installation?.phase ?? ""); const updateBlockedRef = useRef(updateBlocked); updateBlockedRef.current = updateBlocked; @@ -89,7 +88,6 @@ export function App({ persistPreferences = true, spaceEnabled = true, analyticsE }, []); const [surface, setSurface] = useState<"chat" | "space" | "analytics" | "images" | "plugins" | "routines" | "code-review">("chat"); // Code Review (#319) stays hidden in built apps until Gate A is done; see code-review/feature.ts. - const [reviewEnabled] = useState(() => codeReviewEnabled(localStorage, import.meta.env.DEV)); const [reviewOpened, setReviewOpened] = useState(false); const [reviewNav, setReviewNav] = useState(null); const [mediaFeatures, setMediaFeatures] = useState({ images: false, plugins: false }); @@ -207,6 +205,9 @@ export function App({ persistPreferences = true, spaceEnabled = true, analyticsE const showAnalytics = analyticsEnabled && layout.analytics; // Hiding Analytics in Settings while it is open returns to Chat rather than leaving an unreachable surface. useEffect(() => { if (!showAnalytics && surface === "analytics") setSurface("chat"); }, [showAnalytics, surface]); + const reviewEnabled = codeReviewEnabled && layout.codeReview; + // The same for Code Review, which is also the rollback: the surface goes, main's drafts and records stay. + useEffect(() => { if (!reviewEnabled && surface === "code-review") setSurface("chat"); }, [reviewEnabled, surface]); useStreamerRedaction(streamerMode); const privateLabel = streamerMode ? redactSecrets : (text: string) => text; const setShowChanges = (f: (v: boolean) => boolean) => setLayout((l) => ({ changes: f(l.changes) })); @@ -1043,7 +1044,7 @@ export function App({ persistPreferences = true, spaceEnabled = true, analyticsE )} - {showSettings && setShowSettings(false)} {...(analyticsEnabled ? { analyticsInRail: layout.analytics, onAnalyticsInRail: (shown: boolean) => setLayout({ analytics: shown }) } : {})} />} + {showSettings && setShowSettings(false)} {...(analyticsEnabled ? { analyticsInRail: layout.analytics, onAnalyticsInRail: (shown: boolean) => setLayout({ analytics: shown }) } : {})} {...(codeReviewEnabled ? { codeReviewInRail: layout.codeReview, onCodeReviewInRail: (codeReview: boolean) => setLayout({ codeReview }) } : {})} />} {showCompanion && setShowCompanion(false)} />} ) : {error?.message ?? "Loading…"}; diff --git a/apps/desktop/src/renderer/code-review/code-review.css b/apps/desktop/src/renderer/code-review/code-review.css index 101e65e4..25200fb2 100644 --- a/apps/desktop/src/renderer/code-review/code-review.css +++ b/apps/desktop/src/renderer/code-review/code-review.css @@ -253,3 +253,11 @@ button.cr-state { cursor: pointer; font: inherit; } .cr-agent-finding p { margin: 0; } .cr-agent-ask { display: flex; flex-direction: column; gap: 8px; padding: 16px; border: 1px solid var(--border-pane); border-radius: 10px; } .cr-agent-ask p { margin: 0; } + +/* C76: the deletion counts on the file header and tree use the diff's text colour, which keeps AA on every theme's panel background. */ +.cr-file-heading .stat.del, .cr-tree-file .stat.del { color: var(--diff-del-text); } + +/* The diff's plain actions (Refresh files, Retry, Cancel context): without this they take the browser's button colours, which fail contrast on every dark theme. */ +.cr-diff button:not([class]) { color: var(--text-1); background: transparent; border: 1px solid var(--border-pane); border-radius: 6px; padding: 2px 8px; font: inherit; cursor: pointer; } +.cr-diff button:not([class]):hover:not(:disabled) { background: var(--bg-row-hover); } +.cr-diff button:not([class]):disabled { cursor: default; color: var(--text-3); } diff --git a/apps/desktop/src/renderer/code-review/feature.ts b/apps/desktop/src/renderer/code-review/feature.ts deleted file mode 100644 index a1557c41..00000000 --- a/apps/desktop/src/renderer/code-review/feature.ts +++ /dev/null @@ -1,14 +0,0 @@ -/** - * Whether the Code Review destination is shown. Gate A (#319) is incomplete, so a built app hides it - * unless this local switch is "on"; development builds show it. Nothing else reads the switch. - */ -export const CODE_REVIEW_SWITCH = "modex.codeReview"; - -export function codeReviewEnabled(storage: { getItem(key: string): string | null }, development: boolean): boolean { - if (development) return true; - try { - return storage.getItem(CODE_REVIEW_SWITCH) === "on"; - } catch { - return false; - } -} diff --git a/apps/desktop/src/renderer/components/SettingsDialog.tsx b/apps/desktop/src/renderer/components/SettingsDialog.tsx index d7044031..d91ef1f9 100644 --- a/apps/desktop/src/renderer/components/SettingsDialog.tsx +++ b/apps/desktop/src/renderer/components/SettingsDialog.tsx @@ -20,6 +20,8 @@ interface Props { onClose: () => void; /** Whether Analytics shows in the rail. A layout preference: it applies at once, not on Save. */ analyticsInRail?: boolean; + codeReviewInRail?: boolean; + onCodeReviewInRail?: (show: boolean) => void; onAnalyticsInRail?: (shown: boolean) => void; } @@ -47,7 +49,7 @@ const errorMessage = (err: unknown, fallback: string) => const backendHealthText = (status: BackendHealth | undefined, failed: boolean) => status ? `${status.version ? `v${status.version} · ` : ""}${status.detail}` : failed ? "Account check unavailable · retry" : "checking…"; -export function SettingsDialog({ settings, projects, currentProjectId, onSave, onClose, analyticsInRail, onAnalyticsInRail }: Props) { +export function SettingsDialog({ settings, projects, currentProjectId, onSave, onClose, analyticsInRail, onAnalyticsInRail, codeReviewInRail, onCodeReviewInRail }: Props) { const [section, setSection] = useState<"general" | "clis" | "routing" | "approvals" | "advanced">("routing"); const dialogRef = useRef(null); const contentRef = useRef(null); @@ -347,6 +349,10 @@ export function SettingsDialog({ settings, projects, currentProjectId, onSave, o onAnalyticsInRail(e.target.checked)} /> Show Analytics in the rail — token and cache use per turn, recorded on this Mac. Applies right away. } + {onCodeReviewInRail && } } {section === "clis" &&
diff --git a/apps/desktop/src/shared/layout.ts b/apps/desktop/src/shared/layout.ts index a93e47d4..e8780486 100644 --- a/apps/desktop/src/shared/layout.ts +++ b/apps/desktop/src/shared/layout.ts @@ -16,18 +16,27 @@ export type Layout = { streamerMode: boolean; /** Analytics' icon in the rail (Settings → General). */ analytics: boolean; + /** Code Review's destination in the rail (Settings → General). Turning it off is the rollback: main's records and any write already sent stay. */ + codeReview: boolean; }; export const LAYOUT_KEY = "modex.layout"; /** Pre-layout key that stored only the sidebar as "open" | "closed"; read once and migrated. */ export const LEGACY_SIDEBAR_KEY = "modex.sidebar"; -export const DEFAULT_LAYOUT: Layout = { sidebar: true, changes: true, streamerMode: false, analytics: true }; +/** The per-home switch Code Review had before this setting: "on" showed it, anything else set hid it; read until the layout saves a choice. */ +export const LEGACY_CODE_REVIEW_SWITCH = "modex.codeReview"; +export const DEFAULT_LAYOUT: Layout = { sidebar: true, changes: true, streamerMode: false, analytics: true, codeReview: true }; type Storage = { getItem(key: string): string | null; setItem(key: string, value: string): void }; /** Reads the saved layout; unknown or malformed fields fall back to their defaults, never throw. */ export function loadLayout(storage: Storage): Layout { const layout: Layout = { ...DEFAULT_LAYOUT }; + // Someone who explicitly set the old switch to anything but "on" keeps Code Review hidden. An unset switch was only ever the old default. + try { + const legacy = storage.getItem(LEGACY_CODE_REVIEW_SWITCH); + if (legacy !== null && legacy !== "on") layout.codeReview = false; + } catch { /* Storage is blocked: the default stands. */ } let saved: unknown = null; try { saved = JSON.parse(storage.getItem(LAYOUT_KEY) ?? "null"); @@ -35,7 +44,7 @@ export function loadLayout(storage: Storage): Layout { saved = null; } if (saved && typeof saved === "object") { - for (const key of Object.keys(DEFAULT_LAYOUT) as ("sidebar" | "changes" | "streamerMode" | "analytics")[]) { + for (const key of Object.keys(DEFAULT_LAYOUT) as ("sidebar" | "changes" | "streamerMode" | "analytics" | "codeReview")[]) { const v = (saved as Record)[key]; if (typeof v === "boolean") layout[key] = v; } @@ -43,8 +52,10 @@ export function loadLayout(storage: Storage): Layout { const value = (saved as Record)[key]; if (typeof value === "number" && Number.isFinite(value) && value > 0) layout[key] = Math.round(value); } - } else if (storage.getItem(LEGACY_SIDEBAR_KEY) === "closed") { - layout.sidebar = false; + } else { + try { + if (storage.getItem(LEGACY_SIDEBAR_KEY) === "closed") layout.sidebar = false; + } catch { /* Storage is blocked: the default stands. */ } } return layout; } diff --git a/apps/desktop/test/code-review-workspace.test.ts b/apps/desktop/test/code-review-workspace.test.ts index c255f45e..9a4674d6 100644 --- a/apps/desktop/test/code-review-workspace.test.ts +++ b/apps/desktop/test/code-review-workspace.test.ts @@ -4,8 +4,7 @@ import { prKeyId, scopeOf, SIGN_IN_CHANGED, type CodeReviewReply, type CodeRevie import { Reader } from "../src/renderer/code-review/reader.js"; import { back, canBack, canForward, closeChanges, current, DEFAULT_VIEW, fitSplit, fitTree, forward, panesFor, showTab, SPLIT, toggleSplit, visit, type History } from "../src/renderer/code-review/navigation.js"; import { isPinned, loadPins, PINS_KEY, togglePin } from "../src/renderer/code-review/pins.js"; -import { CODE_REVIEW_SWITCH, codeReviewEnabled } from "../src/renderer/code-review/feature.js"; -import { loadLayout, saveLayout } from "../src/shared/layout.js"; +import { LAYOUT_KEY, LEGACY_CODE_REVIEW_SWITCH, loadLayout, saveLayout } from "../src/shared/layout.js"; import { ACCOUNT_A, ACCOUNT_B, FIELD_GUIDE, key, LENDING, locator, R1, R4 } from "./fixtures/code-review.js"; // CR-3 (#322): the renderer half of the read path, and the workspace's navigation and geometry. @@ -242,13 +241,16 @@ test("pins are local, per account, validated on load and capped", () => { assert.equal(loadPins(storage, ACCOUNT_A.id).length, 50); }); -test("the destination is on in development and hidden in a built app unless its switch is set", () => { - const storage = (value: string | null) => ({ getItem: (k: string) => k === CODE_REVIEW_SWITCH ? value : null }); - assert.equal(codeReviewEnabled(storage(null), true), true); - assert.equal(codeReviewEnabled(storage(null), false), false); - assert.equal(codeReviewEnabled(storage("on"), false), true); - assert.equal(codeReviewEnabled(storage("yes"), false), false); - assert.equal(codeReviewEnabled({ getItem: () => { throw new Error("blocked"); } }, false), false); +test("the destination defaults on, keeps an explicit old off, and the saved choice wins over the old switch", () => { + const storage = (value: string | null, saved?: object) => ({ getItem: (k: string) => k === LEGACY_CODE_REVIEW_SWITCH ? value : k === LAYOUT_KEY && saved ? JSON.stringify(saved) : null, setItem: () => {} }); + assert.equal(loadLayout(storage(null)).codeReview, true); + assert.equal(loadLayout(storage("on")).codeReview, true); + assert.equal(loadLayout(storage("off")).codeReview, false); + assert.equal(loadLayout(storage("yes")).codeReview, false); + assert.equal(loadLayout(storage("off", { v: 1, codeReview: true })).codeReview, true); + assert.equal(loadLayout(storage(null, { v: 1, codeReview: false })).codeReview, false); + assert.equal(loadLayout(storage(null, { v: 1, sidebar: false })).codeReview, true); + assert.equal(loadLayout({ getItem: () => { throw new Error("blocked"); }, setItem: () => {} }).codeReview, true); }); test("the workspace's pane widths persist with the layout, and invalid saved values fall back", () => { diff --git a/apps/desktop/test/layout.test.ts b/apps/desktop/test/layout.test.ts index 23ff21ec..bd5b4ce5 100644 --- a/apps/desktop/test/layout.test.ts +++ b/apps/desktop/test/layout.test.ts @@ -14,14 +14,14 @@ const memory = (init: Record = {}) => { test("layout: defaults, round trip, and bad data falls back per field", () => { const s = memory(); assert.deepEqual(loadLayout(s), DEFAULT_LAYOUT); - saveLayout(s, { sidebar: false, changes: false, streamerMode: true, analytics: false }); - assert.deepEqual(loadLayout(s), { sidebar: false, changes: false, streamerMode: true, analytics: false }); + saveLayout(s, { sidebar: false, changes: false, streamerMode: true, analytics: false, codeReview: false }); + assert.deepEqual(loadLayout(s), { sidebar: false, changes: false, streamerMode: true, analytics: false, codeReview: false }); assert.deepEqual(loadLayout(memory({ [LAYOUT_KEY]: "{not json" })), DEFAULT_LAYOUT); - assert.deepEqual(loadLayout(memory({ [LAYOUT_KEY]: JSON.stringify({ changes: "no", sidebar: false }) })), { sidebar: false, changes: true, streamerMode: false, analytics: true }, "a layout saved before Analytics shows it"); + assert.deepEqual(loadLayout(memory({ [LAYOUT_KEY]: JSON.stringify({ changes: "no", sidebar: false }) })), { sidebar: false, changes: true, streamerMode: false, analytics: true, codeReview: true }, "a layout saved before Analytics shows it"); }); test("layout: migrates the old sidebar key, and the new key wins once written", () => { - assert.deepEqual(loadLayout(memory({ [LEGACY_SIDEBAR_KEY]: "closed" })), { sidebar: false, changes: true, streamerMode: false, analytics: true }); + assert.deepEqual(loadLayout(memory({ [LEGACY_SIDEBAR_KEY]: "closed" })), { sidebar: false, changes: true, streamerMode: false, analytics: true, codeReview: true }); assert.deepEqual(loadLayout(memory({ [LEGACY_SIDEBAR_KEY]: "open" })), DEFAULT_LAYOUT); const s = memory({ [LEGACY_SIDEBAR_KEY]: "closed" }); saveLayout(s, { ...loadLayout(s), sidebar: true }); diff --git a/apps/store-desktop/src/renderer/main.tsx b/apps/store-desktop/src/renderer/main.tsx index 50cfcb0f..a8cba425 100644 --- a/apps/store-desktop/src/renderer/main.tsx +++ b/apps/store-desktop/src/renderer/main.tsx @@ -85,7 +85,7 @@ function StoreShell() { {error && {error}}
} - + } {!connected &&
diff --git a/docs/design/code-review.md b/docs/design/code-review.md index 075d43b1..6af6042e 100644 --- a/docs/design/code-review.md +++ b/docs/design/code-review.md @@ -6,11 +6,7 @@ screenshots (R1–R4). The work is tracked in [#319](https://github.com/TypeSafe This page is CR-1's contract ([#320](https://github.com/TypeSafeAI/modex/issues/320)). Every later slice works against it. -**Where it stands (2026-11-01).** CR-1 through CR-7 are on `main` and in v0.4.0, but built apps hide -Code Review unless the local switch `modex.codeReview` is `on`, because Gate A (#319) is not accepted -([v0.4.0 review](../reviews/2026-10-24-v0.4.0-release-review.md)). What remains is acceptance: the original -reference comparisons, human keyboard/VoiceOver, and the live CR-6 smoke and CR-7 canary. Each issue's -ticked boxes and next action are in [status.md](../status.md#open-issue-checklists). +**Where it stands (2026-11-01).** The core CR-1 through CR-7 slices have landed on `main`, and Code Review is present but hidden in the v0.4 releases, whose built apps require the local switch `modex.codeReview` to be `on`. The v0.5.0 candidate replaces that switch with a default-on setting and a rollback in Settings → General, pending Val's approval. Built apps required the switch because Gate A (#319) is not accepted ([v0.4.0 review](../reviews/2026-10-24-v0.4.0-release-review.md)). The candidate also carries the CR-7 sign-out/draft regression and CR-4 through CR-7 accessibility coverage (C76). Remaining engineering work is composer-clearance verification. C58 attachments and C59 effort remain omitted without an approved deviation, alongside the unresolved composer decisions below. Original-reference comparison, human keyboard/VoiceOver, the live CR-6 smoke and the CR-7 canary retain their separate acceptance gates. Each issue's ticked boxes and next action are in [status.md](../status.md#open-issue-checklists). CR-1 itself added a pure read contract, synthetic fixtures and this page, with no IPC channel, UI or GitHub request. Gate A then followed in order: @@ -524,9 +520,11 @@ CR-3 (#322) builds the destination, the inbox and the PR workspace host, in replaces (`reader.ts`). The overview's content (#323) and the remote diff and file tree (#324) fill panes the host lays out. Until then, those panes say so. -**Visibility.** The Code Review rail button shows in development builds. A built app hides it -unless `localStorage["modex.codeReview"]` is `"on"` (`feature.ts`). There is no setting and no main -flag. Whether it shows before Gate A completes is Val's decision. +**Visibility.** The v0.4 releases hide the rail button in a built app unless `localStorage["modex.codeReview"]` +is `"on"`. The v0.5.0 candidate replaces that with the layout's `codeReview` setting, on by default, with +Settings → General → "Show Code Review in the rail" as the rollback; an old explicit off is kept. There is no main +flag. Whether it ships on before Gate A completes is Val's decision, recorded in the +[candidate review](../reviews/2026-11-02-v0.5.0-candidate.md). **Arrangements.** A PR opens with two tabs in the title bar, PR and Changes, starting where the inbox ends. They show one pane (R1), or both side by side behind the right-panel button (R4). Each PR keeps @@ -582,8 +580,8 @@ avatars and C58, C60 and C61 of the composer are not built (open decisions, #325 **Deviations.** - **D6, repository in the header.** The header shows the repository name and number, as the references - do, with `owner/name` in its tooltip. It reads them from the inbox row or the resolved link until #323 - binds `overview.locator` (C22). + do, with `owner/name` in its tooltip. It reads them from the inbox row or the resolved link until the + overview loads, then from the accepted `overview.locator` (C22, #477). - **D7, state pill.** It is the state menu once the PR has loaded and main has answered for the account; otherwise, and for a merged PR, it shows the state with no menu. - **D8, a Pinned group.** Pins appear in a Pinned group above the inbox's own, which the references diff --git a/docs/reviews/2026-10-22-cr8-voiceover/checklist.md b/docs/reviews/2026-10-22-cr8-voiceover/checklist.md index b50f4874..2a09f21d 100644 --- a/docs/reviews/2026-10-22-cr8-voiceover/checklist.md +++ b/docs/reviews/2026-10-22-cr8-voiceover/checklist.md @@ -33,7 +33,7 @@ review, all made up), and prints an `env … npx electron .` line. Since 2026-11 and `codex` executables first on PATH: they log to `provider-calls.log` and exit 1, so nothing in this home reaches your own Claude login or a provider. Run that line, then: -1. Open DevTools (View → Toggle Developer Tools) and run `localStorage.setItem("modex.codeReview", "on")`. Then reload (Command-R). This turns on the unfinished destination in a built app. +1. From the v0.5.0 candidate on, Code Review is in the rail by default. On an older SHA, open DevTools (View → Toggle Developer Tools), run `localStorage.setItem("modex.codeReview", "on")` and reload (Command-R). 2. Make the window about 1440×900. 3. Part A runs with VoiceOver off. Part B runs with VoiceOver on (Command-F5; VO = Control-Option). Parts C and D run with VoiceOver on. Repeat A1–A6, C1–C3 and D1–D4 at about 900×600 and note diff --git a/docs/reviews/2026-10-28-cr6-native-review.md b/docs/reviews/2026-10-28-cr6-native-review.md index 77c76551..7376cd67 100644 --- a/docs/reviews/2026-10-28-cr6-native-review.md +++ b/docs/reviews/2026-10-28-cr6-native-review.md @@ -176,7 +176,7 @@ env -u MODEX_E2E -u MODEX_DEMO MODEX_HOME="$SMOKE/home" MODEX_NO_LOGIN_PATH=1 \ PATH="$SMOKE/bin:$PATH" npx electron . 2> "$SMOKE/electron-stderr.log" ``` -In the app: DevTools → `localStorage.setItem("modex.codeReview", "on")` → reload (this throwaway home only); +In the app (before the v0.5.0 candidate, first: DevTools → `localStorage.setItem("modex.codeReview", "on")` → reload, in this throwaway home only): Code Review → paste the PR link Val named → **Review with Claude** → read the panel's provider disclosure → **Start** once. Wait for the reply. Then open the instructions gear and note the version shown. diff --git a/docs/reviews/2026-11-01-cr-activity-controls.md b/docs/reviews/2026-11-01-cr-activity-controls.md new file mode 100644 index 00000000..e77f1114 --- /dev/null +++ b/docs/reviews/2026-11-01-cr-activity-controls.md @@ -0,0 +1,9 @@ +# Code Review activity controls: C50 and C51 + +Candidate based on main `172dcd282704dcf3fa4d76f26d242f5c3074a294`. This adds real Electron behavioral evidence for two gaps identified in the CR-8 matrix: the opening row and the bot comment menu’s Copy comment link. No application behavior changes. + +The test opens the R1/R2 synthetic GitHub fixture, asserts the opening actor, wording and timestamp, and identifies the bot comment. It uses the keyboard to open Comment actions and activate Copy comment link. It checks the exact renderer-to-main clipboard payload (`https://github.com/example-org/lending/pull/73#issuecomment-9001`), the copied status, menu closure and focus returning to the comment actions button. Clipboard IPC is intercepted inside the fixture, so the test does not write to the person's system clipboard. This is Electron/fixture evidence, not authenticated GitHub or human accessibility acceptance. + +The focused test initially passed. Two separate negative controls modified only the built renderer bundle: incorrect opening text failed the row assertion, and an incorrect comment fragment failed the clipboard payload assertion. Each exited 1 for its intended assertion, and the original bundle was restored after each run. Logs are preserved outside the worktree under `Documents/Codex/artifacts/modex-land-and-prune/cr-activity-*`. + +The restored focused test passed. Independent review found no blockers and confirmed that intercepted clipboard IPC is fixture-local. Programmatic focus verifies keyboard activation and focus return, not Tab reachability. `pnpm build && pnpm test` passed with 2,277 unit passes and 26 skips. The full `pnpm test:e2e` run passed on its first run: 479 desktop passes, 7 skips (22.3 minutes), and 7 site passes (12.1 seconds). The new test also passed within that full run. The five-launch cold transcript median was 93.4 ms against the unchanged 500 ms gate on Apple M5 Max, macOS arm64, Node v24.22.0. Reference comparison and VoiceOver remain open; this change does not complete #327 or the full C51 human gate. diff --git a/docs/reviews/2026-11-01-cr5-diff-controls.md b/docs/reviews/2026-11-01-cr5-diff-controls.md new file mode 100644 index 00000000..2d5367ec --- /dev/null +++ b/docs/reviews/2026-11-01-cr5-diff-controls.md @@ -0,0 +1,23 @@ +# Remote diff control evidence, 2026-11-01 + +Four Electron regressions extend `apps/desktop/e2e/code-review-diff.spec.ts` on base `9ecccc9bcea42ec99cd76092b1fe3bec15d05775`. They exercise the existing remote diff UI with synthetic main-process responses. No application behavior changes and no GitHub mutations are included. + +The local environment is macOS 27.2 (26B123), arm64, Node 24.22.0, pnpm 10.34.5, Electron 44.4.5 and Playwright 1.63.0. The tests reuse `R4` from `test/fixtures/code-review.ts`, with fixed fixture time `2026-10-18T20:41:00Z`, base SHA `4d` repeated 20 times and head SHA `6f` repeated 20 times. Wrap/scroll and divider cases explicitly resize the rendered viewport to 1440 by 900. These behavioral cases do not calibrate reference-image crop, fonts or DPR and make no screenshot-fidelity claim. + +Reproduce the four focused cases with `pnpm --filter @modex/desktop exec playwright test e2e/code-review-diff.spec.ts --grep 'diff controls:'` after `pnpm build`. Canonical validation uses `pnpm build && pnpm test && pnpm test:e2e` from the worktree root, with one Playwright worker and no configured retries. + +| Matrix control | New automated evidence | Limit | +| --- | --- | --- | +| C63 view toggles | Highlight changes alters the rendered added-line background and restores it; Wrap lines changes rendered code height and overflow | Does not establish reference-image fidelity | +| C71 horizontal scroll | A mouse wheel scrolls the first unwrapped file horizontally while its sibling and outer diff container remain at zero; wrapping restores fit | Synthetic long-line content at 1440 by 900 | +| C75 divider | ArrowLeft changes width by eight pixels, pointer drag changes it by 48 more, reload preserves it after returning to Changes, Enter restores the default | Programmatic focus does not establish Tab reachability or VoiceOver acceptance | +| C66 file menu | Enter opens the menu and activates Copy path, exact remote path reaches clipboard IPC, the GitHub link has the expected PR files URL, no Discard button or local-file operation occurs | Clipboard IPC is intercepted; the system clipboard and external browser are not exercised | +| C69 context | The control discloses that more unchanged context may be available; Enter shows remote context and sends one request for the selected path, key and revision without a local-file operation | Fixture evidence does not establish authenticated GitHub behavior | + +The first two-test run passed wrapping but failed the divider test after reload because the app returned to Chat. The test now reopens Code Review, the PR and Changes before measuring persisted width. The unchanged application then passed both tests, and all four tests passed after adding file-menu and context coverage. + +Four temporary negative controls were rebuilt and removed: forcing wrapping on failed the height assertion (242 pixels remained 242); ignoring divider resize failed expected 298 versus actual 290; copying an incorrect path failed the exact clipboard request assertion; changing the uncertainty label prevented the named context control from being found. The first wrapping negative control ended with a null bounding-box diagnostic; using the rendered element height with a five-second poll produced the intended assertion failure on repetition. The restored implementation passed all four focused tests. None of these temporary product edits remains in the diff. + +Full build and unit validation passed: 2,277 passed, 26 skipped, zero failures or cancellations. The first full E2E run exited 1 after 21.3 minutes: 481 desktop cases passed, 7 were skipped and 1 failed; the site stage did not run. The failure was the existing Canvas local-HTML/full-view case: after clicking Exit full view, the main pane stayed hidden for 15 seconds (`canvas.spec.ts:97`). All four added diff-control cases passed within that run. The original Canvas trace and error context are preserved under `cr-diff-controls-canvas-first-failure/`; the original cause remains unestablished. This first run was not green. Independent review found no blocking findings in the four added tests. Logs and the original failure artifacts are retained outside the worktree under `~/Documents/Codex/artifacts/modex-land-and-prune/` with the `cr-diff-` prefix. This record supplements the CR-8 matrix; it does not close #324 or #327, authenticate Viewed, supply the original reference comparison, or complete human keyboard/VoiceOver acceptance. + +Three instrumented Canvas repetitions passed, recording host pointer delivery, full-state transitions and guest bounds without reproducing the failure. After restoring the original test exactly, five unchanged repetitions also passed. These results do not establish the original cause or clear the failed full run. The second canonical `pnpm test:e2e` run exited 0 on unchanged application and test code: 482 desktop cases passed with 7 skips in 19.7 minutes, followed by 7 site cases in 7.8 seconds. Its cold-transcript median was 79.1 ms against the unchanged 500 ms gate on Apple M5 Max. This later success does not explain the first failure; the first full-run results remain archived with SHA-256 verification for all 107 files. diff --git a/docs/reviews/2026-11-01-cr7-live-canary.md b/docs/reviews/2026-11-01-cr7-live-canary.md index d59c4128..d021dd11 100644 --- a/docs/reviews/2026-11-01-cr7-live-canary.md +++ b/docs/reviews/2026-11-01-cr7-live-canary.md @@ -59,9 +59,8 @@ env -u MODEX_E2E -u MODEX_DEMO MODEX_HOME="$CANARY/home" MODEX_NO_LOGIN_PATH=1 \ PATH="$CANARY/bin:$PATH" npx electron . 2> "$CANARY/electron-stderr.log" ``` -In the app: `localStorage["modex.codeReview"] = "on"` from the developer tools of this throwaway home only -(it is a per-home setting, so the person's real home is unchanged), then Code Review → paste the throwaway -PR's link. +In the app: Code Review → paste the throwaway PR's link. (Before the v0.5.0 candidate, first set +`localStorage["modex.codeReview"] = "on"` from the developer tools of this throwaway home only.) ## Steps diff --git a/docs/reviews/2026-11-01-cr8-acceptance-matrix.md b/docs/reviews/2026-11-01-cr8-acceptance-matrix.md index 9467fd7c..f1074f75 100644 --- a/docs/reviews/2026-11-01-cr8-acceptance-matrix.md +++ b/docs/reviews/2026-11-01-cr8-acceptance-matrix.md @@ -68,7 +68,7 @@ found that asserts it; it is not a pass. | C19 | Show more | #322 | built | contract test (`knownCount`, `mergePages`, #438 same-total rule); `code-review-navigation.spec.ts` pagination ×3; `code-review-measure.spec.ts` | — | — | open (VoiceOver) | | C20 | Independent inbox scroll | #322 | built | `code-review-navigation.spec.ts` keyboard test; `code-review-scroll.spec.ts` ×2 (#438 `afec85b`) | — | — | — | | C21 | State pill "Open ▾" | #326 (decision 2 settled 2026-10-25) | built (#416 `b3aad0d`); D7: no menu until main answers, none for merged | `code-review-actions.spec.ts` "the state menu closes and reopens a PR, and converts it to draft…", "a state change GitHub may have made stays in doubt…"; `code-review-writes.test.ts`, `code-review-write-ipc.test.ts`; `code-review-a11y.spec.ts` (pill contrast) | — | open (needs disposable repo Val names, #326) | open (VoiceOver) | -| C22 | Repository and number | #323 | deviation D6 (interim): the header still reads `pr.locator` from the row or resolved link (`PrPane.tsx` `PrHeader`), not `overview.locator` | `code-review-navigation.spec.ts` (`title` gives `owner/repo`) | — | — | open (VoiceOver) | +| C22 | Repository and number | #323 | built: #477 (`49aa449`) adopts `overview.locator` into the open PR after the account and PR-key checks, so `PrHeader`'s `pr.locator` is the accepted identity once the overview loads; D6 ended ([evidence](2026-11-01-cr4-current-locator.md)) | `code-review-overview.spec.ts` (renamed repository: header text, title and links); `code-review-navigation.spec.ts` (`title` gives `owner/repo`) | — | — | open (VoiceOver) | | C23 | Loading spinner | #322 | built | `code-review-navigation.spec.ts` (reduced motion); `code-review-regressions.spec.ts` "identity" | — | — | — | | C24 | Pin | #322 | built | `code-review-navigation.spec.ts` (local, per account, survives reload) | `A-322`, `A-main`, `A-main-2` (pin after reload) | — | open (VoiceOver) | | C25 | Copy link | #322 | built | contract test (`canonicalPrUrl`); `code-review-navigation.spec.ts` (clipboard) | — | — | open (VoiceOver) | @@ -96,8 +96,8 @@ found that asserts it; it is not a pass. | C47 | Independent sidebar scroll | #323 | built (#393 `6c40547`) | `code-review-scroll.spec.ts` (#438 `afec85b`, where the sidebar is its own scroller) | — | — | — | | C48 | Activity heading and filter | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` "refresh does not mix fresh and pending cached pages…" (filter keeps focus) | — | — | open (VoiceOver) | | C49 | Commit groups | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` ("7 commits"); `code-review-overview.test.ts` (dedup) | `A-main` (activity 36, read), `A-323` (35, complete) | — | open (VoiceOver) | -| C50 | "opened this pull request" | #323 | built (#393 `6c40547`; rendered in `PrOverview.tsx`) | unverified: no spec found that asserts the text | `A-323` (opening enrichment read) | — | open: reference comparison | -| C51 | Bot comment card (⋯ menu: Copy comment link, Add to chat) | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` (cards); the ⋯ menu's Copy comment link is unverified (no spec found); its Add to chat is covered under C54 | — | — | open (VoiceOver) | +| C50 | "opened this pull request" | #323 | built (#393 `6c40547`; rendered in `PrOverview.tsx`) | candidate `cr-v050` (#481): `code-review-overview.spec.ts` "activity opening and comment link…" asserts actor, wording and time | `A-323` (opening enrichment read) | — | open: reference comparison | +| C51 | Bot comment card (⋯ menu: Copy comment link, Add to chat) | #323 | built (#393 `6c40547`) | `code-review-overview.spec.ts` (cards); candidate `cr-v050` (#481): the same spec's activity-opening case asserts the ⋯ menu's Copy comment link by keyboard, with the exact URL; its Add to chat is covered under C54 | — | — | open (VoiceOver) | | C52 | Deployment table and preview links | #323 | built (#393 `6c40547`) | `code-review-overview.test.ts` "preview destinations reject credentials…"; `code-review-read-service.test.ts`; `code-review-overview.spec.ts` | `A-323` (one real deployment and status) | — | open (VoiceOver) | | C53 | Third-party bot button | #323 | deviation D4 (inert) | `code-review-overview.spec.ts` "hostile Markdown is inert…"; `code-review-regressions.spec.ts` "hostile content" | — | — | — | | C54 | Reaction and Add to chat | #325 (reactions D3) | built: Add to chat (#422 `b643617`); reactions deviation D3 | `code-review-agent.spec.ts` "a question is answered by a run of its own, with 'Add to chat' quoting a comment into it" | — | open (needs repo Val names, #325) | open (VoiceOver) | @@ -107,22 +107,22 @@ found that asserts it; it is not a pass. | C58 | Attach (+) | #325 | not built (left out; no numbered decision covers it) | — | — | — | — | | C59 | Model and reasoning effort | #325 | built: model select only, effort left out (#422 `b643617`) | `code-review-agent.spec.ts` (runs with `--model`); `code-review-agent.test.ts` (unknown model refused) | — | open (needs repo Val names, #325) | open (VoiceOver) | | C60 | Voice input | deferred | deviation D5 | — | — | — | — | -| C61 | Composer clearance | #323 / #324 | built at fixture (#393 `6c40547`): bottom clearance; final Comment in viewport at 900×600 | `code-review-overview.spec.ts` "hostile Markdown is inert and narrow content remains reachable"; `code-review-scroll.spec.ts`. Unverified against CR-6's inline Ask/review section | — | — | open (VoiceOver) | +| C61 | Composer clearance | #323 / #324 | built at fixture (#393 `6c40547`): bottom clearance; final Comment in viewport at 900×600 | `code-review-overview.spec.ts` "hostile Markdown is inert and narrow content remains reachable"; `code-review-scroll.spec.ts`. Candidate `cr-v050-followups`: `code-review-agent.spec.ts` "C61…" measures Ask and the last activity item beside CR-6's inline section and its open panel, at 900×600 and 1440×900; red under an overlay and under a panel that takes the pane's height | — | — | open (VoiceOver) | | C62 | Header repeated over Changes | #322 | built; now repeats the live C21 and C27–C29 | `code-review-navigation.spec.ts`; journey (R4); `code-review-a11y.spec.ts` | — | — | open (VoiceOver) | -| C63 | Changes ⋯ menu and view toggles | #324 | built in part (#404 `e3527f4`): "Wrap lines" and "Highlight changes" toggles. The ⋯ menu and the tree/image/layout toggles are left out, as the inventory allows | unverified: no spec found that toggles them (they show in the diff spec and journey captures) | — | — | open (VoiceOver); open: reference comparison | +| C63 | Changes ⋯ menu and view toggles | #324 | built in part (#404 `e3527f4`): "Wrap lines" and "Highlight changes" toggles. The ⋯ menu and the tree/image/layout toggles are left out, as the inventory allows | candidate `cr-v050` (#484): `code-review-diff.spec.ts` "wrapping and highlighting change rendered code…" | — | — | open (VoiceOver); open: reference comparison | | C64 | File header | #324 | built (#404 `e3527f4`) | `code-review-diff.spec.ts` "grouped diffs, both gutters, hierarchy, filter…"; review-flow (#441) | `A-324` (service: 15/15 files) | — | open (VoiceOver); open: reference comparison | | C65 | Viewed toggle | needs decision 5 | needs decision 5 (not built) | — | — | — | — | -| C66 | File ⋯ menu (never Discard) | #324 | built (#404 `e3527f4`): Copy path and Open in GitHub only | `code-review-regressions.spec.ts` "local Changes stay local"; `code-review-diff.spec.ts` "…use remote data only". The menu items themselves are unverified (no Code Review spec found that opens them) | — | — | open (VoiceOver) | +| C66 | File ⋯ menu (never Discard) | #324 | built (#404 `e3527f4`): Copy path and Open in GitHub only | `code-review-regressions.spec.ts` "local Changes stay local"; `code-review-diff.spec.ts` "…use remote data only". Candidate `cr-v050` (#484): the same file's "file menu copies the remote path…" opens the menu and asserts both items and no Discard | — | — | open (VoiceOver) | | C67 | Folded lines, expand | #324 | built (#404 `e3527f4`) | `code-review-diff.spec.ts` "expanding context asks the exact remote revision…", "review: context expands adjacent to both ends…", "context text budget is shared…"; `code-review-diff.test.ts` "context windows reveal both ends…" | `A-main` (context read), `A-324` (base context matched the merge base) | — | open (VoiceOver) | | C68 | Line numbers, add/remove rows | #324 | built (#404 `e3527f4`) | `code-review-diff.test.ts` ×9; `code-review-diff.spec.ts` "line anchors have one keyboard entry per file…", "old-line anchors name the merge base…"; review-flow | `A-324` (15/15 patches parsed; anchor) | — | open (VoiceOver) | -| C69 | "More unchanged context…" | #324 | built (#404 `e3527f4`; `ContextFold` when `count` is null) | unverified: no spec found that asserts the text (`code-review-diff.spec.ts` clicks `/Expand.*unchanged/`) | — | — | open (VoiceOver) | +| C69 | "More unchanged context…" | #324 | built (#404 `e3527f4`; `ContextFold` when `count` is null) | candidate `cr-v050` (#484): `code-review-diff.spec.ts` "more unchanged context discloses uncertainty…" finds the control by this text and asserts the one request it sends | — | — | open (VoiceOver) | | C70 | Collapsed file sections | #324 | built (#404 `e3527f4`) | `code-review-diff.spec.ts` (Collapse a file; "1,000 files keep… mounted patch bodies bounded") | — | — | open (VoiceOver) | -| C71 | Horizontal scroll per file | #324 | built (#404 `e3527f4`; `.cr-patch { overflow-x: auto }` with wrap off) | unverified: no spec found | — | — | — | +| C71 | Horizontal scroll per file | #324 | built (#404 `e3527f4`; `.cr-patch { overflow-x: auto }` with wrap off) | candidate `cr-v050` (#484): `code-review-diff.spec.ts` "…horizontal scroll stays per file" | — | — | — | | C72 | Filter files | #324 | built (#404 `e3527f4`) | `code-review-diff.spec.ts` (Filter remote files) | — | — | open (VoiceOver) | | C73 | Folder tree, selected file | #324 | built (#404 `e3527f4`) | `code-review-diff.spec.ts` "…hierarchy, filter and keyboard selection…", "narrow tree selection returns to the diff…"; `code-review-scroll.spec.ts`; journey R4 populated (#464 `3d72c6e`) | — | — | open (VoiceOver); open: reference comparison | | C74 | Overview / Changes divider | #322 | built | `code-review-navigation.spec.ts` (keyboard resize persists across reload) | — | — | open (VoiceOver) | -| C75 | Diff / tree divider | #322 | built; pointer and keyboard resize still not asserted | `code-review-a11y.spec.ts` (reached by Tab as a separator); `code-review-navigation.spec.ts` (tree folds when narrow) | — | — | open (VoiceOver) | -| C76 | Keyboard path, 900×600 and 1440×900 | #327 | partial: the a11y spec still requires only CR-3's 12 controls | `code-review-a11y.spec.ts`; `code-review-navigation.spec.ts` keyboard test; `code-review-scroll.spec.ts` (focus kept); per-slice keyboard cases in the overview, diff, agent and actions specs | — | — | **open: [VoiceOver checklist](2026-10-22-cr8-voiceover/checklist.md) parts A–D for Val (result row empty)** | +| C75 | Diff / tree divider | #322 | built | candidate `cr-v050` (#484): `code-review-diff.spec.ts` "file tree divider resizes with keyboard and pointer and persists on reload"; `code-review-a11y.spec.ts` (reached by Tab as a separator); `code-review-navigation.spec.ts` (tree folds when narrow) | — | — | open (VoiceOver) | +| C76 | Keyboard path, 900×600 and 1440×900 | #327 | built; candidate `cr-v050` requires the CR-3 to CR-7 controls and checks every text run in the CR-4 to CR-7 sections | `code-review-a11y.spec.ts`; `code-review-navigation.spec.ts` keyboard test; `code-review-scroll.spec.ts` (focus kept); per-slice keyboard cases in the overview, diff, agent and actions specs | — | — | **open: [VoiceOver checklist](2026-10-22-cr8-voiceover/checklist.md) parts A–D for Val (result row empty)** | | C77 | Overview column scroll | #323 | built (#393 `6c40547`) | `code-review-scroll.spec.ts` (#438 `afec85b`); journey R2 (overview moves, inbox doesn't) | — | — | — | | C78 | Diff pane scroll | #324 | built (#404 `e3527f4`) | `code-review-scroll.spec.ts`; `code-review-diff.spec.ts` "returning from Overview preserves… overall scroll position" | — | — | — | | C79 | File tree scroll | #324 | built (#404 `e3527f4`) | `code-review-scroll.spec.ts` (#438 `afec85b`) | — | — | — | @@ -155,7 +155,7 @@ Decisions 2–4 (C21, C31, C35, C38, C43's +) were settled by Val on 2026-10-25 | #323 AC2 | Pagination and refresh: no duplicates, lost scroll or cross-PR bleed; partial never zero | `code-review-overview.spec.ts` ×8 (failed pages, refresh, delayed bleed, partial first page, retry cursor); `code-review-overview.test.ts`; [cr4-overview](2026-10-23-cr4-overview.md) | `A-323` (activity complete) | — | met at fixture (#393 `6c40547`). A live account switch was not exercised | | #323 AC3 | Stale checks, inferred stack and comment-derived previews kept distinct | `code-review-overview.spec.ts` "stale checks and unavailable collections…"; `code-review-overview.test.ts` (stack, preview destinations, stale retention); `code-review-read-service.test.ts` | `A-323` (deployment; stack unavailable) | — | met at fixture (#393 `6c40547`) | | #323 AC4 | Hostile Markdown/HTML, unsafe schemes, misleading previews inert | `code-review-overview.spec.ts` "hostile Markdown is inert…"; `code-review-regressions.spec.ts` "hostile content"; `code-review-overview.test.ts` "preview destinations reject…" | — | — | met at fixture (#393 `6c40547`). No live untrusted PR content was run | -| #323 AC5 | Long titles, large descriptions, narrow windows readable; composer clearance | `code-review-overview.spec.ts` (900×600, final Comment in viewport); `code-review-scroll.spec.ts` (#438 `afec85b`) | — | — | met at fixture (#393). Clearance next to CR-6's inline review/Ask section is unverified. **Open:** VoiceOver (Val) | +| #323 AC5 | Long titles, large descriptions, narrow windows readable; composer clearance | `code-review-overview.spec.ts` (900×600, final Comment in viewport); `code-review-scroll.spec.ts` (#438 `afec85b`) | — | — | met at fixture (#393). Candidate `cr-v050-followups` measures clearance next to CR-6's inline review/Ask section (C61). **Open:** VoiceOver (Val) | | #323 AC6 | Every visible write control is connected to a verified CR-7 capability or is unavailable; no mutation by #323 | At #393 all writes were unavailable ([cr4-overview](2026-10-23-cr4-overview.md)). Since #416 `b3aad0d` they are connected through `code-review:write` (`code-review-actions.spec.ts`) | — | see #326 AC6 | met at fixture | | #324 AC1 | R4 overview-plus-diff, tree, folds, selected file through real controls; match approved reference geometry | `code-review-diff.spec.ts` "captures populated R4 diff geometry and narrow keyboard file selection"; journey R4 populated (#464 `3d72c6e`); review-flow (#441 `eca2bd9`). Evidence: [cr5-remote-diffs](2026-10-23-cr5-remote-diffs.md) | — | — | met at fixture for the controls (#404 `e3527f4`). **Open:** approved-reference geometry, since the original R4 is missing (Val) | | #324 AC2 | Old/new line references for additions, deletions, renames, no final newline; context keeps them | `code-review-diff.test.ts` ×9; `code-review-diff.spec.ts` "review: deleted ranges and renamed paths retain base anchors…", "old-line anchors name the merge base…"; [cr5-merge-base](2026-10-24-cr5-merge-base.md) | `A-324` (anchor; merge-base match) | — | met at fixture (#404 `e3527f4`), plus an authenticated service probe | @@ -190,7 +190,13 @@ Decisions 2–4 (C21, C31, C35, C38, C43's +) were settled by Val on 2026-10-25 ## Staged enablement and rollback -Today the only switch is the per-home `localStorage["modex.codeReview"]` (`renderer/code-review/feature.ts`): +**Candidate `cr-v050` (2026-11-02), awaiting Val's approval:** the switch is replaced by a persisted setting, Settings → +General → "Show Code Review in the rail", on by default. An old explicit value other than `"on"` stays off until the +person chooses (`shared/layout.ts`, `code-review-enablement.spec.ts`). This takes Stage 2's product step before Stage 1 +and 2's gates have run, so that one candidate can carry them all. It reaches nobody until Val merges and releases it; +[the Val session](2026-11-02-v0.5.0-val-session.md) lists the gates to pass or accept first. + +Before that candidate, the only switch was the per-home `localStorage["modex.codeReview"]`: development builds show the destination, packaged builds hide it unless the value is `"on"`. Nothing else reads it, and main's channels answer regardless of whether the destination is visible. @@ -200,7 +206,8 @@ it, and main's channels answer regardless of whether the destination is visible. - **Stage 2, after VoiceOver and Val's call on the references, decisions 1, 5, 6, 7, and the C58/C59 omissions:** remove the switch (or invert it) in a product PR that Val approves. That PR updates `docs/status.md` and this matrix. -**Rollback:** setting the value to anything but `"on"` (or clearing site data for that home) hides the +**Rollback:** in the candidate, clearing "Show Code Review in the rail" hides the destination at once and across a +quit. On earlier builds, setting the value to anything but `"on"` (or clearing site data for that home) hides the destination on the next launch. That hides the surface; it does not cancel a write already sent. An in-doubt write keeps its record in `/review-actions/state.json` and an unconfirmed review run in `/review-agent/state.json`; both stay until a read-back or the person settles them, whether or not @@ -225,13 +232,12 @@ the destination is visible. A product rollback (removing a capability) is a code ## Follow-ups found in this refresh (agent work, not gates) -- C22 still shows the repository and number from the inbox row or resolved link (`PrPane.tsx` reads - `pr.locator`); D6 was meant to end when #323 bound `overview.locator`. Either bind it or record D6 as permanent. -- `code-review-a11y.spec.ts` still requires only CR-3's 12 controls; the overview, diff, review and write - controls are not in its required Tab set (C76). -- No asserting spec for C50's text, C51's Copy comment link, C63's toggles, C66's menu items, C69's text, C71's - horizontal scroll or C75's resize. -- C61's clearance was planned for a floating composer; CR-6 built Ask inline and clearance was not re-measured. +- C22 is closed on `main`: #477 binds the accepted `overview.locator`, which ends D6. +- Done in candidate `cr-v050`: `code-review-a11y.spec.ts` requires the overview, diff, review and write controls + (C76). Its new per-text contrast check found the file header and tree deletion count at 4.02:1 in OpenCoven; fixed. +- Done in candidate `cr-v050`: `code-review-budgets.spec.ts` gates the agreed opening budgets (AC5), and + `code-review-actions.spec.ts` proves drafts stay hidden after sign-out (#326). +- Done in candidate `cr-v050` (#481, #484): asserting specs for C50, C51, C63, C66, C69, C71 and C75. Done in `cr-v050-followups`: C61's clearance beside CR-6's inline Ask section. - CR-4's and CR-5's authenticated evidence is service-level on pre-merge candidates (`A-323`, `A-324`); an authenticated Electron read of the overview and diff on `main` needs Val's authorization (GET only). diff --git a/docs/reviews/2026-11-01-issue-checkbox-reconciliation.md b/docs/reviews/2026-11-01-issue-checkbox-reconciliation.md index fbfc27c9..74be570a 100644 --- a/docs/reviews/2026-11-01-issue-checkbox-reconciliation.md +++ b/docs/reviews/2026-11-01-issue-checkbox-reconciliation.md @@ -191,7 +191,7 @@ Total proposed flips: 66. ### #319 Tracker -**Current state:** CR-1 (#320) and CR-2 (#321) are closed and on main. Every CR slice's code is on main: CR-3 #367/#438/#464, CR-4 #393, CR-5 #395 via #404, CR-6 #396/#422/#426, CR-7 #410/#416, CR-8 test/doc PRs #372/#440/#441/#430. What's left is acceptance gates, all waiting on Val: the R1-R4 comparison (the originals are gone; re-captures need Val's decision), the VoiceOver pass, a repo for CR-6's live smoke, a disposable repo for CR-7's live canary, decisions 1/5/6/7 (C16/C81, C65, C07, C82), and when to remove the `modex.codeReview` packaged-build switch. Next action: land #472 (matrix refresh plus the live procedures), then Val answers the batched ask ([comment](https://github.com/TypeSafeAI/modex/issues/319#issuecomment-6558837152)). +**Current state:** CR-1 (#320) and CR-2 (#321) are closed and on main. Every CR slice's code is on main: CR-3 #367/#438/#464, CR-4 #393, CR-5 #395 via #404, CR-6 #396/#422/#426, CR-7 #410/#416, CR-8 test/doc PRs #372/#440/#441/#430. Core slices have landed, but engineering work remains: the CR-7 sign-out/draft regression, CR-4 through CR-7 accessibility coverage (C76), and composer-clearance verification. C58 attachments and C59 effort remain omitted without an approved deviation. Separate gates and decisions require Val: the R1-R4 comparison (the originals are gone; re-captures need Val's decision), the VoiceOver pass, a repo for CR-6's live smoke, a disposable repo for CR-7's live canary, decisions 1/5/6/7 (C16/C81, C65, C07, C82), and when to remove the `modex.codeReview` packaged-build switch. Next action: land #472 (matrix refresh plus the live procedures), then Val answers the batched ask ([comment](https://github.com/TypeSafeAI/modex/issues/319#issuecomment-6558837152)). Proposed flips: 2 of 10 to `[x]`; no unchecks (nothing is checked today). diff --git a/docs/reviews/2026-11-02-v0.5.0-candidate.md b/docs/reviews/2026-11-02-v0.5.0-candidate.md new file mode 100644 index 00000000..172024be --- /dev/null +++ b/docs/reviews/2026-11-02-v0.5.0-candidate.md @@ -0,0 +1,84 @@ +# v0.5.0 candidate review (2026-11-02) + +**Status: a candidate. Nothing here is merged, tagged or released, and no live or human gate has run on it.** + +Branch `cr-v050`, on `main` at `b785054e88836ac9df5ca917425031ab697d4d30`. The candidate is `370db33b877fa143cdfe463c6c8dda6046c9afc0`; the commit after it adds only this page's results. + +## What it carries + +| Delivery | What | Where it came from | +| --- | --- | --- | +| Code Review on by default | A persisted setting replaces `modex.codeReview`. Settings → General → "Show Code Review in the rail" is the rollback. An old explicit off is kept. The Store build keeps it off | this branch | +| C76 accessibility | The Tab spec requires the CR-3 to CR-7 controls at 1440×900 and 900×600. A new check measures every text run in the CR-4 to CR-7 sections and panels, in all three themes | this branch | +| Diff button contrast, on `cr-v050-followups` | Review of #490 showed the check sampled only the first match of each selector. It now measures every match and the whole tree and diff regions. That found the diff's plain buttons (Refresh files, Retry, Cancel context) at 1.72:1 to 2.29:1 in all three themes, because they took the browser's colours. They now use theme colours | stacked on this branch | +| Contrast repair | That check found the file header and tree deletion count at 4.02:1 in OpenCoven. Both use the diff's text colour now (5.12:1 or better across what it measures) | this branch | +| Performance budgets | `code-review-budgets.spec.ts`, below | this branch | +| CR-7 regression | A saved draft stays hidden from the surface and from main after sign-out and for another account, and returns for its own account | this branch | +| CF-3 regression | A draft and attachment survive a card arriving, a refused answer and a good answer | this branch | +| C61 clearance, on `cr-v050-followups` | `code-review-agent.spec.ts` measures Ask and the last activity item beside CR-6's inline section and its open panel, at 900×600 and 1440×900. The layout needed no change | stacked on this branch | +| #481, #484, #485 | C50/C51 coverage; remote diff control coverage; readiness wording | rebased; their PRs are unchanged until the force-push in [the Val session](2026-11-02-v0.5.0-val-session.md) | + +Unreleased source already on `main` since v0.4.1, also in this candidate: #461, #462, #463, #465 and #467. + +#487 landed on `main` while this was being built and adds a very similar CF-3 test. Both stay: #487 also checks focus on arrival, and this branch's test adds the refused answer and its mutation proof. A reviewer may fairly fold them into one. + +## Budgets (#327 AC5) + +Median of five launches, from the click to the content on screen, through the production read service with a synthetic +`gh`. Times are on a Mac17,6 (Apple M5 Max) under other sessions' load, about 7. "Delayed" slows each `gh` answer by 5.5 s +through `MODEX_BUDGET_GH_DELAY_MS`, the failure proof. + +| Path | Run 1 (ms) | Run 2 (ms) | Budget (ms) | Delayed (ms) | +| --- | --- | --- | --- | --- | +| Open the inbox | 821 | 317 | 5,000 | 17,406, fails | +| Open an overview | 97 | 98 | 1,500 | 5,842, fails | +| Open a 1,000-file diff (first page) | 219 | 224 | 2,500 | 22,486, fails | +| Open a 3 MiB patch | 308 | 307 | 3,000 | 33,560, fails | + +Budgets leave roughly tenfold headroom. The 500 ms transcript gate failed on hosted runners from variance alone, so +these catch a change several times slower, and no longer. The inbox is bimodal here, about 310 or 820 ms; the cause was +not investigated. **Not yet seen on a hosted runner.** The synthetic `gh` answers at once, so none of this measures GitHub. + +## Mutation proofs + +| Test | Mutation | Red | Restored | +| --- | --- | --- | --- | +| CR-7 sign-out | main's write `status` trusts the requested account instead of checking who is signed in | `signedOut.result.state` was "ok" | passed | +| CF-3 question card | answering a question clears the thread's unsent text | composer value was "" | passed | +| C61 clearance, on `cr-v050-followups` | an overlay over the pane's bottom 200 px; a panel that takes the pane's height | both sizes failed on each: Ask or the last item covered, Ask's text box off screen | passed | + +The CF-3 mutation clears the text only. Attachments are asserted by the same test but were not separately mutated. + +## Limits + +- **Attachments during a turn.** Attaching is refused while a turn runs, by design. The CF-3 test therefore attaches while + idle and starts the waiting turn through `thread:send`, as a turn from another client would. +- **The hard-kill path.** The layout saves through localStorage, so a crash immediately after the toggle can lose it, as + with every layout preference. The rollback test uses a normal quit. +- **Gates still open.** Live CR-6 smoke, live CR-7 canary, VoiceOver, keyboard A rows, CF-3 D and P rows and the reference + decision. All are in [the Val session](2026-11-02-v0.5.0-val-session.md). This candidate runs the default-on step ahead of + them, which is [decision 1.1](2026-11-02-v0.5.0-val-session.md#1-decisions-no-launch-10-minutes) there. + +## Full run (one pass, no reruns) + +`pnpm build && pnpm test && pnpm test:e2e` on `370db33`, one pass, 2026-11-03 01:49–02:14 UTC, on a Mac17,6 with other sessions' load at about 10. + +| Step | Result | +| --- | --- | +| Build | exit 0 | +| Unit | exit 0, 0 failed: worktree 120; core 16; desktop 2,117 passed, 20 skipped; desktop scripts 21 passed, 6 skipped; site 10; Store 16 | +| Desktop E2E | 495 passed, 7 skipped, 0 failed, 20.9 min | +| Site E2E | 7 passed | + +**The C61 follow-up on `cr-v050-followups`, one pass on `7e45440`** (`370db33` plus the test and these docs), 01:54–02:18 UTC: build exit 0; unit exit 0, with the same counts as above; desktop E2E 497 passed, 7 skipped, 0 failed, 19.6 min; site E2E 7 passed. The two extra E2E passes are the C61 cases. The branch was then re-stacked on the commit that records the run above, which changes only documents. + +**This is the third candidate SHA. The earlier runs stay on the record:** + +| SHA | Result | What happened next | +| --- | --- | --- | +| `a249a5f` | **Failed.** Build passed. Unit had 3 failures, all from the default-on change; `pnpm test`'s chain stopped there, so the site and Store unit suites did not run. E2E: 494 and 7 passed | The default-on commit had not been through the unit suite. Two older layout tests listed exact fields, and the third found a real gap: `loadLayout` still threw on blocked storage. Fixed in the commit `370db33` carries at its tip | +| `6f1efef` | Clean: unit 0 failed, E2E 494 and 7 passed | `main` moved to `b785054` (#487, tests and docs only). The candidate was rebased, and this diff was exactly #487 | + +Before these, three unit runs in parallel on the rebased PR branches each timed out two timing-sensitive tests (the runner's unconfirmed stop, and the demo's slow turn). Both passed alone and in one-at-a-time reruns; no change was made for them. The final PR branch checks ran one at a time on `b785054` and passed. + +**Not established:** a hosted CI run (the first will come from this branch's PR), the budgets on a hosted runner, and anything live or human. diff --git a/docs/reviews/2026-11-02-v0.5.0-val-session.md b/docs/reviews/2026-11-02-v0.5.0-val-session.md new file mode 100644 index 00000000..4538d11d --- /dev/null +++ b/docs/reviews/2026-11-02-v0.5.0-val-session.md @@ -0,0 +1,132 @@ +# v0.5.0 candidate: Val's one session (2026-11-02) + +Everything left for a person on Code Review, CF-3 and CF-8, in one order, on one candidate. Agents have marked none +of these rows passed. The rows' full wording stays in the linked checklists, which remain the receipts' home. + +**Candidate:** the head of branch `cr-v050`; its tested SHA is in [the candidate review](2026-11-02-v0.5.0-candidate.md). It carries the default-on Code +Review setting, so step 1 is the decision the rest depends on. + +**One build for everything** (about 2 minutes). Every launch below starts from here: + +```sh +cd /.worktrees/cr-v050/apps/desktop # at the candidate SHA +pnpm install --frozen-lockfile && pnpm build +git rev-parse HEAD # write this SHA on every receipt +``` + +Estimated at the desk: about 2 hours for the keyboard, VoiceOver and live rows; the phone rows need the paired +iPhone and a Companion build with the native question card. + +## 1. Decisions (no launch, 10 minutes) + +Post each answer on the issue named. Modex builds nothing further until you do. + +| # | Decide | What to look for | Receipt | +| --- | --- | --- | --- | +| 1.1 | Ship Code Review on by default before every gate below has passed, or only after them | This candidate takes [Stage 2](2026-11-01-cr8-acceptance-matrix.md#staged-enablement-and-rollback) early so one build can carry them. The alternative is to flip `DEFAULT_LAYOUT.codeReview` to false and release hidden | Comment on #319 | +| 1.2 | Decision 1: avatars (C16, C81): login only, or fetch avatars | [Open decisions](../design/code-review.md#open-decisions). Fetching adds an image request per person to GitHub's avatar host | #319 | +| 1.3 | Decision 5: the Viewed toggle (C65) | Not built. Build it, or record it as left out | #324 | +| 1.4 | Decision 6: new tab (C07) and decision 7: access-mode indicator (C82) | C07 is disabled and C82 is words in the panel today | #319 | +| 1.5 | C58 attachment and C59 reasoning effort: build each, or approve the omission | Neither has a numbered decision | #325 | +| 1.6 | R1–R4 references: re-capture as a new baseline, or retire the comparison | The originals are gone. Re-capture means accepting the journey's captures as the reference, which stops being an independent check | #319 | +| 1.7 | The CF-8 candidate: this one, instead of `b643617` | This session assumes yes. `b643617` predates #450, #453, #463 and this work | #281 | + +## 2. Code Review: rollback, by hand (5 minutes) + +```sh +env -u MODEX_E2E -u MODEX_DEMO MODEX_HOME=/tmp/modex-v050-rollback npx electron . +``` + +| Row | Do | Expect | Receipt | +| --- | --- | --- | --- | +| 2.1 | Look at the rail on this fresh home | Code Review is there | Comment on #327 | +| 2.2 | Settings → General → clear "Show Code Review in the rail", then quit and reopen | It leaves at once and stays gone | #327 | + +## 3. CR-8 keyboard and VoiceOver (about 45 minutes) + +Synthetic data; your own `gh` and Claude login are never read. + +```sh +node ../../docs/reviews/2026-10-22-cr8-voiceover/seed.mjs /tmp/modex-cr8-voiceover # run the env line it prints +``` + +Follow [the checklist](2026-10-22-cr8-voiceover/checklist.md), parts A to D, at 1440×900 and again at 900×600. + +Look for: every control named and reachable, focus returning after each popover, and nothing announced as a bare +"button". **Receipt:** fill its Result row; it covers #322 AC4 and CR-8 AC5. + +## 4. CR-6 live smoke (about 10 minutes, a small provider cost) + +Name `owner/repo#n` on #325 and say go. Follow the [procedure](2026-10-28-cr6-native-review.md#live-smoke-procedure-325-ac6-not-run), which starts the app with logging wrappers around your real `claude` and read-only `gh`. + +Look for: `--safe-mode --restricted --tools Read` on the logged `-p` line, one start, a reply bound to the head you +opened, and no GitHub write. **Receipt:** `~/.claude/task-evidence/cr6-live-smoke-/receipt.md`, summarized on #325. + +## 4a. Authenticated read of the overview and diff (about 5 minutes, no write) + +`A-323` and `A-324` read through the service, on pre-merge candidates. Nothing has read a real overview and diff through Electron on this code. Name a PR you may read on #319 and say go, or do it yourself: start the app as in step 4's procedure, with its logging `gh` wrapper, and open that PR's overview and Changes. + +Look for: the real title, activity and diff at the head GitHub shows, and only `gh api` GET reads in the wrapper's log. **Receipt:** `~/.claude/task-evidence/cr-authenticated-read-/receipt.md`, summarized on #319. + +## 5. CR-7 live canary (about 20 minutes) + +Name a disposable repository on #326 and say go. Follow the [procedure](2026-11-01-cr7-live-canary.md). + +Look for: each preview matching what is sent, exactly one mutating `gh` call per step, the merge passing the +reviewed head, and no token in anything Modex wrote. **Receipt:** `$CANARY/steps.md`, summarized on #326. + +## 6. CF-8 keyboard A1–A8 (about 15 minutes) + +```sh +node ../../docs/reviews/2026-10-20-keyboard-voiceover/seed.mjs /tmp/modex-voiceover +MODEX_HOME=/tmp/modex-voiceover/home npx electron . +``` + +Follow [part A](2026-10-20-keyboard-voiceover/checklist.md#a-keyboard-only-no-mouse-or-trackpad). **Receipt:** the filled table on #281. + +## 7. CF-8 VoiceOver B1–B7 (about 15 minutes) + +Same launch as step 6, VoiceOver on (Command-F5). Follow [part B](2026-10-20-keyboard-voiceover/checklist.md#b-voiceover). +These rows still await you. Part C (C1–C5, deferred 2026-10-30) stays deferred unless you say otherwise. **Receipt:** #281. + +## 8. CF-3 desktop D1–D5 (about 15 minutes, a small provider cost) + +```sh +MODEX_EVENT_JOURNAL=1 MODEX_HOME=/tmp/modex-v050-cf3 npx electron . +``` + +Turn Plan on (⇧⌘P) in a disposable project on Codex, then send the prompt in [the setup](2026-10-22-cf3-question-acceptance.md#setup). Follow its D rows. The new automated test covers a draft surviving the card; D3 and D4 still need your eyes. + +Look for: the card reachable by Tab, an incomplete answer refused in place, and the token absent from +`grep -ra "" "$MODEX_HOME"`. **Receipt:** the [CF-3 page](2026-10-22-cf3-question-acceptance.md), Run 1, and #281. + +## 9. CF-3 phone P1–P4 and P6 (about 15 minutes) + +Same Mac session as step 8, with a Companion build that has the native question card, paired. Follow the [P rows](2026-10-22-cf3-question-acceptance.md). P6 needs Companion 0.3.0 (8) or earlier installed; record its version and build. V1–V3 and P5 stay deferred as you said on 2026-11-01. **Receipt:** the same page and #281, with the iPhone model and iOS version. + +## 10. CF-8 rows that need your call, not a test + +The [#281 table in status](../status.md#open-issue-checklists) also lists these. Say on #281 whether each is agent work to schedule, or accepted as open: + +- memory measurement; +- the ChatGPT multi-agent restriction check; +- the staged release recommendation; +- the list of remaining visual differences. + +## Agent actions blocked on you + +The automated rebases of #481, #484 and #485 could not be force-pushed: the session's safety rules denied it. The +rebased, signed commits are on `*-v050` branches, based on `b785054`. To update the PRs, from the primary checkout: + +```sh +for b in cr-activity-evidence cr-diff-controls cr-readiness-summary; do + git fetch origin "$b-v050" && git push --force-with-lease="$b" origin "origin/$b-v050:refs/heads/$b" +done +``` + +The `*-rebased` branches are an earlier rebase, on `7cfb29c`, and can be deleted. Both sets exist only because of the denial. + +## After the session + +Tell an agent "resume the Code Review v0.5.0 goal". Failing rows become issues linked from their tracker. Merging, the +release review, tagging and publishing remain yours to authorize. diff --git a/docs/status.md b/docs/status.md index 255e19d2..4b0550d4 100644 --- a/docs/status.md +++ b/docs/status.md @@ -246,8 +246,8 @@ before relying on a tick. | [#280](https://github.com/TypeSafeAI/modex/issues/280) CF-7 | 11/14 | Steps 1, 2 and 4: model-proposed relations, file hashes and selection/notes context are not built; step-level linking was retired | Val reviews the two saved Notes and records steps 1, 2 and 4 as deferred or retired | | [#281](https://github.com/TypeSafeAI/modex/issues/281) CF-8 | 8/27 | Keyboard A1–A8, VoiceOver B1–B7, CF-3 D1–D5, P1–P4 and P6; memory measurement; ChatGPT multi-agent restriction check; staged release recommendation; list of remaining visual differences. C1–C5, V1–V3 and P5 are deferred | Val chooses the candidate, then runs A1–A8 and D1–D5 | | [#319](https://github.com/TypeSafeAI/modex/issues/319) CR tracker | 2/10 | CR-3 to CR-8 acceptance; R1–R4 reconstruction comparison | Land #472 (current CR-8 matrix) | -| [#322](https://github.com/TypeSafeAI/modex/issues/322) CR-3 | 10/14 | Original-reference comparison (AC1), keyboard/VoiceOver (AC4), composer clearance with CR-6's composer | Val runs the [CR-8 VoiceOver checklist](reviews/2026-10-22-cr8-voiceover/checklist.md) | -| [#323](https://github.com/TypeSafeAI/modex/issues/323) CR-4 | 10/14 | Original R1/R2 comparison, VoiceOver, composer clearance; avatars wait on decision 1 | Re-check composer clearance; Val decides on references | +| [#322](https://github.com/TypeSafeAI/modex/issues/322) CR-3 | 10/14 | Original-reference comparison (AC1), keyboard/VoiceOver (AC4); clearance beside CR-6's inline section is measured only in candidate `cr-v050-followups`, not on `main` | Val runs the [CR-8 VoiceOver checklist](reviews/2026-10-22-cr8-voiceover/checklist.md) | +| [#323](https://github.com/TypeSafeAI/modex/issues/323) CR-4 | 10/14 | Original R1/R2 comparison, VoiceOver; clearance beside CR-6's inline section is measured only in candidate `cr-v050-followups`, not on `main`; avatars wait on decision 1 | Val decides on references | | [#324](https://github.com/TypeSafeAI/modex/issues/324) CR-5 | 12/14 | Original R4 comparison (AC1); the Viewed toggle waits on decision 5 | Val decides on R4 references or re-captures | | [#325](https://github.com/TypeSafeAI/modex/issues/325) CR-6 | 13/14 | Authorized live native smoke (AC6) | Val names `owner/repo#n` for the smoke | | [#326](https://github.com/TypeSafeAI/modex/issues/326) CR-7 | 12/14 | No on-main test of draft visibility after sign-out; live canary on a disposable repository (AC6) | Add the sign-out test; Val names a disposable repository | @@ -263,6 +263,7 @@ before relying on a tick. ## In flight +- **v0.5.0 candidate: Code Review on by default** (#319, #327; branch `cr-v050`, full run clean on `370db33`). Carries #481, #484 and #485, a11y coverage of the CR-4 to CR-7 controls (C76), performance budgets as gates, the CR-7 sign-out and CF-3 question-card regressions, and a default-on setting with a rollback in Settings → General that replaces the `modex.codeReview` switch. It stays a candidate: nothing is merged, tagged or released, and no live or human gate has run on it. [Candidate review](reviews/2026-11-02-v0.5.0-candidate.md); [Val's one session](reviews/2026-11-02-v0.5.0-val-session.md). The C61 clearance test beside CR-6's inline section is on the stacked branch `cr-v050-followups`, whose own full run passed on `7e45440`. - **Unmerged-work retirement safety repair (#486):** signed candidate `783898a` is pushed on `retire-safety-repair`; it requires matching ownership, archives index and working state separately, verifies raw working/ignored recovery, and retains the original checkout indefinitely instead of deleting files. Focused disposable-fixture results and design are in [the repair review](reviews/2026-11-02-retirement-safety-repair.md). Pinned installation, build, full unit tests (2,325 passed, 26 skipped), typecheck and independent follow-up review passed; protected CI and landing remain pending. No real worktree retirement or repair-specific Electron e2e has been exercised. - **Claude account pool with OrcaRouter keys.** Settings → Coding CLIs → Claude Code adds separate Claude Code profiles (each with its own `CLAUDE_CONFIG_DIR` and browser login), OrcaRouter gateway keys in OS-encrypted storage, an account for new conversations, and opt-in rotation when an account hits its usage limit. Threads bind to their first account; a turn that had not started work retries once on the next account. Unit and E2E coverage use a synthetic `claude`; no live OrcaRouter turn, second subscription or real limit message has been run. OmniRouter was evaluated and not integrated. [Design and limits](claude-accounts.md). @@ -273,6 +274,8 @@ Claude account pool review repairs passed build, full unit validation and the ac - **CF-3 question regressions (#276).** Local regression work adds native-question YOLO timing cases and a composer draft/attachment preservation fixture. Current-main build, typechecks, full unit validation (2,300 passes, 26 skips), four focused Electron cases and full E2E (481 desktop passes, seven skips; seven site passes) pass after host recovery and a fixture setup correction. Post-answer focus, approval interactions and human/native acceptance remain open. See the [validation receipt](reviews/2026-11-01-cf3-question-regressions.md). +Code Review C50/C51 behavioral coverage is ready for review: an Electron fixture asserts the opening row and the comment menu’s exact copied URL, status, closure and keyboard focus return. Independent review found no blockers; build and unit validation passed (2,277 passes, 26 skips), and full E2E passed (479 desktop passes, 7 skips; 7 site passes). Reference comparison and VoiceOver remain open. [Evidence](reviews/2026-11-01-cr-activity-controls.md). + - **Deferred Markdown for long threads (hosted cold-open budget).** Hosted CI's approved 500 ms cold-open gate failed on runner variance alone: 33 hosted runs on 2026-10-30 had a 377.7 ms median and a 781.9 ms failure on application code identical to a 436.1 ms pass. Val chose on 2026-10-30 to defer offscreen answers. Turns before the newest eight show their Markdown as written until it nears the view, then parse once; scroll anchoring keeps the reader's place. Locally the five-launch median fell from about 190 to 88 ms, and 3× CPU throttling cut it from 806–870 to 312–340 ms. The gate is unchanged. The first hosted run had a 129.9 ms median (481 desktop E2E passes). It landed through #463 (`32f1cb4`) on 2026-10-31 UTC and is unreleased. [Evidence](reviews/2026-10-30-deferred-markdown.md). Desktop CI memory diagnostics landed through #458 after #456’s cold transcript budget failure. The workflow reuses the existing resource sampler and retains its log as a separate CI artifact; it does not establish the cause of the earlier failure or change the 500 ms gate. The #458 queue passed desktop and site E2E and retained its memory log. See [the diagnostic review](reviews/2026-10-30-desktop-ci-memory.md). @@ -351,6 +354,8 @@ Desktop CI memory diagnostics landed through #458 after #456’s cold transcript keyboard/VoiceOver acceptance waits for Val's pass with the [checklist](reviews/2026-10-22-cr8-voiceover/checklist.md). +- **Remote diff control regression coverage** (#324, #327). Four Electron cases now cover wrap/highlight rendering, per-file horizontal scrolling, keyboard and pointer divider resizing with reload persistence, file-menu copy-path IPC and link destination, and revision-bound context requests. [Evidence and limits](reviews/2026-11-01-cr5-diff-controls.md) retain the first test-navigation failure and four negative controls. Build and unit validation passed. The first full desktop E2E run had 481 passes, 7 skips and one Canvas full-view failure; its site stage did not run. The unchanged full retry passed 482 desktop cases with 7 skips and 7 site cases. The original failure remains preserved and unexplained. Reference comparison, authenticated behavior and human accessibility remain open. + - **CR-6 agent review and questions** (#325). Phase 1's native proof ([isolation evidence](reviews/2026-10-23-cr6-isolation.md)) showed Claude Code's restricted Read-only palette rejects injected shell, write, fetch, MCP and outside reads while Codex's read-only sandbox does not. Phase 2 builds on it: main's local `code-review:agent` channel starts the installed Claude Code CLI, under the person's own login, on a snapshot of the PR's diff that main reads itself through `gh`, with the shipped argv `--safe-mode --restricted --tools Read` and no permission tool, and an environment stripped of `gh`, GitHub and Modex variables. Each run is recorded before it starts, bound to the head it read, marked outdated when the head moves, stopped explicitly, and read as unknown after a crash; a finding reaches GitHub only through CR-7's review draft. Review instructions are one versioned text per Mac whose scope the popover states, and each run records the version it used. Unit, native-fixture (opt-in, production argv) and E2E evidence over a synthetic `claude` and `gh` are in [the review evidence](reviews/2026-10-28-cr6-native-review.md); an authenticated live run and C82 remain open gates. On 2026-11-01 the unit and opt-in native denial tests were re-run on the installed Claude Code 2.1.316 and codex-cli 0.166.0 (6/6 native cases passed), and the live-smoke procedure was written down; it waits for a repository Val names. - **CR-7 Code Review writes** (#326). Val decided on 2026-10-25: writes go through the person's `gh` login, and decisions 2–4 are in scope. The foundation stores scoped drafts and revision-bound intents, consumes confirmations once after durable admission, and recovers interrupted dispatches as unknown without resend. The first write increment adds main's separate `code-review:write` channel for top-level comments and submitted reviews (line comments from Changes included). Each is previewed with its exact destination, account and head, re-checked in main before sending, sent at most once, and reconciled by read-back when its outcome is unclear ([evidence](reviews/2026-10-25-cr7-write-submit.md)). Mock and synthetic-`gh` only: the live write waits for a disposable repository Val names. A second increment adds Merge: main reads push permission, the repository's allowed methods, GitHub's merge state and the target branch's rules fresh. It offers only what `mergePreflight` ([preflight evidence](reviews/2026-10-24-cr7-merge-preflight.md)) allows, sends the reviewed head as the merge's `sha`, and reconciles an unclear merge from the PR's own record ([evidence](reviews/2026-10-25-cr7-merge.md)). A third increment adds decisions 2–4 the same way: the state menu (close, reopen, convert to draft, ready for review), title and description edits that are refused if GitHub's text changed, and reviewer requests ([evidence](reviews/2026-10-25-cr7-pr-actions.md)). CR-6 isolation and C82 remain unchanged. See [the preparation evidence](reviews/2026-10-23-cr7-intents.md). A follow-up binds the model to CR-5's shared `LineAnchor`, diff commit included, and records the check of CR-4's action slots ([evidence](reviews/2026-10-24-cr7-anchors.md)). The integration repair fences queued and in-flight submits against document invalidation, verifies inline destinations against fresh complete patch evidence, preserves pending composer edits across navigation, and shows frozen inline details before submission ([repair evidence](reviews/2026-10-26-cr7-write-repair.md)). All of it landed through #416 (`b3aad0d`, 2026-10-27). The live-write [canary procedure](reviews/2026-11-01-cr7-live-canary.md) (comment, review, state, edit, reviewer, squash-merge on a throwaway PR) is written and not run; it waits for a disposable repository Val names.