Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions apps/desktop/e2e/code-review-journey.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import fs from "node:fs";
import path from "node:path";
import { appDir, tid } from "./support.js";
import { baseWorld, fitWindow, launchWithGh, prRow, READ_ENDPOINT, type Harness } from "./code-review-support.js";
import { ghFile } from "../test/fixtures/code-review-gh-world.js";

/**
* CR-8 (#327): the integrated Code Review journey in real Electron, through main's production read service
Expand Down Expand Up @@ -48,6 +49,22 @@ test("inbox → R1 → R2 → R3 → R4 through the production read service, wit
body: `Journey activity ${i + 1}: keep the hold's place when moving between branches.`,
}));
world.routes["repos/example-org/lending/deployments"] = [];
// R1's sidebar shows checks for the head, and R4's Changes pane shows grouped diffs and a file tree: give the
// world lending #73's check runs and a commit status, and field-guide #91's four files (+11 −4, as its PR says),
// so the captures hold populated content for the reference comparison (#322 Step 3). All made up.
const head = `repos/example-org/lending/commits/${"c".repeat(40)}`;
const checkRun = (id: number, name: string, conclusion: string | null, minutes: number) => ({
id, name, head_sha: "c".repeat(40), status: conclusion ? "completed" : "in_progress", conclusion, app: { slug: "github-actions" },
started_at: "2026-10-18T20:31:00Z", completed_at: conclusion ? `2026-10-18T20:${31 + minutes}:00Z` : null, html_url: `https://github.com/example-org/lending/runs/${id}`,
});
world.routes[`${head}/check-runs`] = { total_count: 4, check_runs: [checkRun(8101, "unit + e2e (macOS)", "success", 6), checkRun(8102, "typecheck", "success", 2), checkRun(8103, "lint", "failure", 1), checkRun(8104, "package", null, 0)] };
world.routes[`${head}/status`] = { total_count: 1, statuses: [{ id: 8201, context: "preview/deploy", state: "pending", creator: { id: 1003, login: "deploy-bot", type: "User" }, created_at: "2026-10-18T20:32:00Z", updated_at: "2026-10-18T20:32:00Z", target_url: "https://example.test/preview/73" }] };
world.routes["repos/example-org/field-guide/pulls/91/files"] = [
ghFile("src/branches/regions.ts", "@@ -1,4 +1,7 @@\n import { Branch } from \"./branch\";\n-export const regions = [\"north\", \"south\"];\n-export function groupByRegion(branches: Branch[]) { return branches; }\n+export const regions = [\"north\", \"south\", \"east\", \"west\"] as const;\n+export type Region = (typeof regions)[number];\n+export function groupByRegion(branches: Branch[]): Map<Region, Branch[]> {\n+ return new Map(regions.map((region) => [region, branches.filter((b) => b.region === region)]));\n+}\n export const DEFAULT_REGION = \"north\";", 5, 2),
ghFile("src/branches/index.ts", "@@ -2,3 +2,5 @@\n export { Branch } from \"./branch\";\n-export { regions } from \"./regions\";\n+export { regions, groupByRegion } from \"./regions\";\n+export type { Region } from \"./regions\";\n+export { BranchList } from \"../pages/branch-list\";\n export { openingHours } from \"./hours\";", 3, 1),
ghFile("src/pages/branch-list.tsx", "@@ -8,4 +8,5 @@ export function BranchList({ branches }: Props) {\n return (\n- <ul>{branches.map((b) => <li key={b.id}>{b.name}</li>)}</ul>\n+ <>{[...groupByRegion(branches)].map(([region, list]) => <section key={region}><h2>{region}</h2>\n+ <ul>{list.map((b) => <li key={b.id}>{b.name}</li>)}</ul></section>)}</>\n );\n }", 2, 1),
ghFile("docs/branches.md", "@@ -1,2 +1,3 @@\n # Branches\n+Branch pages are grouped by region: north, south, east and west.\n Each branch page lists its opening hours.", 1, 0),
];
const deploymentRead = `repos/example-org/lending/deployments?sha=${"c".repeat(40)}&per_page=20&page=1`;
h = await launchWithGh(world);
const { page, repo } = h;
Expand Down
6 changes: 5 additions & 1 deletion docs/design/code-review.md
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,11 @@ With the overview (#323) and the remote diff (#324) in the panes, `e2e/code-revi
proves AC3 and the scroll half of AC4 at 1440×900 and 900×600: the inbox list, the overview (its main
column, or its body where the pane stacks), the diff and the file tree each scroll alone by wheel and
by PageDown, nothing else moves, the focused control keeps focus, and each position is restored on
return ([acceptance record](../reviews/2026-10-28-cr3-acceptance.md)). Original R1–R4 reference
return ([acceptance record](../reviews/2026-10-28-cr3-acceptance.md)). On 2026-10-30 the automated half
and the authenticated probe were re-run on `main` at `f51ef7e`, and the journey spec's world was given
files for field-guide #91 and checks for lending #73's head, so its R1 and R4 captures show a populated
diff, file tree and checks sidebar rather than empty panes
([resume record](../reviews/2026-10-30-cr3-acceptance-resume.md)). Original R1–R4 reference
comparison and human keyboard/VoiceOver acceptance remain open gates.


Expand Down
7 changes: 4 additions & 3 deletions docs/reviews/2026-10-22-cr8-voiceover/checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ and, since the overview (#323) and the diff and file tree (#324) landed, parts C
The review panel and instructions popover (#325) and the GitHub writes (#326) have landed too; their rows stay
listed at the end for their own issues' acceptance and are not part of #322 AC4.

**Build:** `main` at `7a01df2` or later (the CR-3 acceptance PR's head once it merges), built with `pnpm build`.
Record the SHA you used. The automated half of this gate at that head is in
[the CR-3 acceptance record](../2026-10-28-cr3-acceptance.md).
**Build:** `main` at `afec85b` (#438, the CR-3 acceptance follow-up) or later, built with `pnpm build`. On
2026-10-30 `main` was `f51ef7e`; the automated half of this gate was re-run there
([resume record](../2026-10-30-cr3-acceptance-resume.md)). Record the SHA you used. The automated half's first run
is in [the CR-3 acceptance record](../2026-10-28-cr3-acceptance.md).

## Setup (about 3 minutes)

Expand Down
117 changes: 117 additions & 0 deletions docs/reviews/2026-10-30-cr3-acceptance-resume.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
# CR-3 acceptance resumed on `main`, 2026-10-30

Evidence for [#322](https://github.com/TypeSafeAI/modex/issues/322) after the acceptance follow-up (#438,
`afec85b`) landed. It re-runs the automated half of the acceptance on current `main`, re-runs the read-only
authenticated navigation probe there, and hands Val the two gates that remain human: the R1–R4 reference
comparison and the keyboard/VoiceOver pass. It does not close the issue.

## Candidate

- `main` at `f51ef7e8caa17e79c9029be0d696f5bb40b3dcf2` (#453). The worktree is `cr3-resume`; it changes no
product code. Besides this record, the checklist and `docs/status.md`, it changes one spec,
`e2e/code-review-journey.spec.ts`, so that its R1 and R4 captures hold populated content (below).
- Evidence outside the repository: `~/Documents/Codex/artifacts/modex-cr3-acceptance/2026-10-30/` (logs, the
probe and its result, journey captures and the reference-less `compare.html`). `environment.txt` has the
versions: macOS 27.0, Node 24, pnpm as pinned, gh 2.102.0.

## What is already on `main`

| Step | Landed | Where |
| --- | --- | --- |
| 1. #349's re-review items: `mergePages` requires every page to report the same total; a `SIGN_IN_CHANGED` read is retried once by the renderer `Reader`, within the same continuation so a newer read or cancellation wins | #438 `afec85b` (2026-10-28) | `src/shared/code-review.ts`, `src/renderer/code-review/reader.ts`; tests in `code-review-contract.test.ts`, `code-review-workspace.test.ts`, `e2e/code-review-regressions.spec.ts` |
| 2. Independent scrolling and focus with the overview (#393) and the remote diff (#404) in the panes, at 1440×900 and 900×600 | #438 | `e2e/code-review-scroll.spec.ts` |
| 4. The human keyboard/VoiceOver checklist, parts A–D | #438 | [`2026-10-22-cr8-voiceover/checklist.md`](2026-10-22-cr8-voiceover/checklist.md) |

CR-4 (#393, 2026-10-24) and CR-5 (#404, 2026-10-24) have both landed, so Step 2's precondition holds.

## Re-run on `main` at `f51ef7e`

Run from the worktree on this Mac, each exiting 0:

| What | Command | Result |
| --- | --- | --- |
| Build | `pnpm install --frozen-lockfile && pnpm build` | passed |
| Step 1 unit files | `node --test --test-force-exit dist/test/code-review-{contract,workspace,read-service}.test.js` | 75 passed, 0 failed |
| Focused Code Review E2E (the resume prompt's two specs plus the scroll and regression specs) | `pnpm exec playwright test e2e/code-review-navigation.spec.ts e2e/code-review-ipc.spec.ts e2e/code-review-scroll.spec.ts e2e/code-review-regressions.spec.ts` | 24 passed, 0 failed, 1.9 min |
| Typecheck | `pnpm typecheck` | passed |
| Unit | `pnpm test` | worktree scripts 120/120; core 16/16; desktop 2,089 passed, 20 skipped, 0 failed; desktop native suites 21 passed, 6 skipped; site 10/10; store-desktop 16/16 |
| E2E | `pnpm test:e2e` | desktop 476 passed, 7 skipped (opt-in live cases), 0 failed, 40.3 min on a loaded machine; site 7/7 |

The focused run includes the #349 regressions (the one-retry read through the production read service and a
rewritten `hosts.yml`; the `gh auth switch` refusal that is not retried) and both scroll cases.

## Authenticated read-only navigation probe, re-run on `main`

The same probe Val authorized for `main` on 2026-10-22 (`A-main` in the
[acceptance matrix](2026-10-22-cr8-acceptance-matrix.md)), `live-navigation.mjs`, was run again against this
build with the Mac's own `gh` login (account BunsDev, id 68980965, gh 2.102.0), a disposable Modex home and
no `MODEX_E2E` or demo flag. It exited 0 with all six checks passed:

1. authenticated inbox and `repo:TypeSafeAI/modex` search, every row in that repository;
2. public PRs #349 and #348 resolved from their links with overview-derived totals and no overview error;
3. Back, Forward, Changes and the split through their real controls;
4. a pin retained and resolved after renderer reload;
5. an explicit resolve error for a nonexistent public PR;
6. a separate disposable home with an empty `GH_CONFIG_DIR` and no token variables shows signed-out and a
disabled search field.

Only GET reads of public PRs were made. No GitHub write, coding turn, account switch or private content
export took place; `gh auth status` showed the same active account afterwards. The sign-in retry path was
not exercised live (it would require switching the Mac's gh account), so it stays synthetic-`gh` evidence.

## Step 3: R1–R4 comparison

Still not run. The originals were confirmed gone on 2026-10-22 and were not found on this Mac on 2026-10-28,
and Val decided that day to leave the comparison open rather than re-capture or substitute. Nothing has
changed since: no reference directory has been named. No synthetic capture was substituted for a reference.

**The captures were not populated.** Reviewing the journey spec's R1 and R4 captures from the full E2E run on
this `main` (kept under `journey-captures/`), and the 2026-10-28 ones, showed the same two gaps: R4's Changes
pane read "0 of 4 files loaded" with an empty file tree, because the synthetic world serves no files for
field-guide #91, and R1's Checks section read "not found", because the world serves no check runs for
lending #73's head. The references show grouped diffs with a file tree (R4) and a checks sidebar (R1), so a
comparison from those captures would have measured empty panes. The journey spec's world now carries four
files for #91 (+11 −4, as its PR says) and four check runs and one commit status for #73's head; its
geometry assertions are unchanged and it passes. The populated R1–R4 captures at the references' CSS sizes,
plus 1440×900 and 900×600, are kept under `journey-captures-populated/` with their manifest and a
reference-less `compare.html`. The Stack section still reads "not found" (the world declares no branch
relationships; `stack` was unavailable as designed in `A-main` too). When Val names a directory holding
`R1.png`–`R4.png` (originals, or re-captures with their own calibration row), the comparison is:

```sh
cd apps/desktop && pnpm exec playwright test e2e/code-review-journey.spec.ts
pnpm run ui:compare -- --captures test-results/ui-code-review <directory with R1.png … R4.png>
```

Each reviewed deviation then gets a D-row in the design page's Workspace section.

## Step 4: the human keyboard/VoiceOver checklist

[`2026-10-22-cr8-voiceover/checklist.md`](2026-10-22-cr8-voiceover/checklist.md) now names the landed build
(`afec85b` or later; this run used `f51ef7e`). To run it, from `apps/desktop` in a worktree at the SHA under
test after `pnpm build`:

```sh
node ../../docs/reviews/2026-10-22-cr8-voiceover/seed.mjs /tmp/modex-cr8-voiceover
```

then the printed `env … npx electron .` line, `localStorage.setItem("modex.codeReview", "on")` in DevTools
and a reload. Parts A and B cover CR-3's inbox, tabs, header and dividers; C and D the populated overview,
diff and tree; A1–A6, C1–C3 and D1–D4 are repeated at about 900×600. The result row at the end waits for Val.

## Acceptance criteria, by evidence

| AC | Fixtures / synthetic `gh` (this run) | Authenticated (this run) | Human |
| --- | --- | --- | --- |
| AC1 R1/R4 arrangements through real controls | `code-review-navigation.spec.ts` (R1 test, R1/R4 captures) | probe check 3 (Changes and split) | geometry comparison with the originals: open, deferred by Val |
| AC2 stale replies, back/forward | navigation (identical numbers, slow search, account switch, back/forward) and regressions (identity ×3, revision) | probe check 3 (back/forward) | — |
| AC3 independent scrolling, restored geometry | `code-review-scroll.spec.ts` ×2; navigation keyboard test (persisted panes) | — | — |
| AC4 keyboard at 900×600 and 1440×900 | navigation keyboard test; scroll spec (focus kept) | — | keyboard/VoiceOver checklist: open |
| AC5 local Changes, no remote Discard | regressions "local Changes stay local"; navigation pin/copy test | — | — |
| AC6 fixtures and authenticated smoke reported separately | this table's first column | probe 6/6 on `f51ef7e` | — |

## Still open on #322

- The R1–R4 comparison against originals or re-captures Val names (Step 3).
- The human keyboard/VoiceOver result (Step 4).
- A live run of the sign-in retry is not claimed.
9 changes: 7 additions & 2 deletions docs/status.md
Original file line number Diff line number Diff line change
Expand Up @@ -311,8 +311,13 @@ Desktop CI memory diagnostics landed through #458 after #456’s cold transcript
cancellation remain authoritative across attempts) and proves, with the
overview and remote diff now in the panes, that the list, overview, diff and file tree scroll alone
and keep focus at 1440×900 and 900×600 ([acceptance record](reviews/2026-10-28-cr3-acceptance.md)).
Original reference comparison waits for Val's re-captured references; human keyboard/VoiceOver
acceptance waits for Val's pass with the [checklist](reviews/2026-10-22-cr8-voiceover/checklist.md).
On 2026-10-30 the automated half was re-run on `main` at `f51ef7e` and the read-only authenticated
navigation probe passed six of six there again ([resume record](reviews/2026-10-30-cr3-acceptance-resume.md));
the journey spec's R1 and R4 captures, which had shown an empty Changes pane and no checks, now carry
populated content for the comparison.
Original reference comparison waits for Val to name the references (deferred on 2026-10-28); human
keyboard/VoiceOver acceptance waits for Val's pass with the
[checklist](reviews/2026-10-22-cr8-voiceover/checklist.md).

- **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.

Expand Down
Loading