Skip to content

[CR-3][P1] Build the PR inbox and resizable Code Review workspace #322

Open

Description

@BunsDev

Outcome

Navigate from the Code Review inbox to a selected PR, its Changes tab and a split overview/diff workspace without losing selection, scroll position or keyboard access. Match the R1–R4 composition in #319 using the existing Modex shell, not a separate app or a screenshot painted over the interface.

Parent: #319. Start dependency: #320's accepted placement, source map, shared identity/state contract and fixtures. Live-integration/completion dependency: #321's read adapter. Fixture-driven UI can proceed in parallel with #321. Produces the navigation/layout host consumed by CR-4/5. Dependencies mean accepted artifacts, not merely closed issue numbers.

Context / ownership

Read current AGENTS.md, #319–#321 and the actual shell, tabs, theme tokens, persistence and bridge paths identified in #320. Audit reference: main cb33db5df00e235f68d36b5fac556dc30e0851f2; refresh current main and open PRs first. Existing apps/desktop/src/renderer/components/ChangesPanel.tsx is a LOCAL working-tree view; preserve it. One integrator owns shared shell/route/type changes.

Proposed additions, subject to the accepted source map: apps/desktop/src/renderer/components/code-review/CodeReviewWorkspace.tsx, CodeReviewInbox.tsx, and apps/desktop/e2e/code-review-navigation.spec.ts. Extend existing equivalents instead when that is smaller. New paths are proposals, not claims that they exist.

Items to do

  • Write failing navigation tests for authored/review-requested groups, search, canonical PR-link input, selection, tab switching, back/forward and return to a previous PR. Use the [CR-1][P1] Establish the Code Review reference contract and deterministic fixtures #320 fixtures and stable accessible locators.
  • Add the Code Review destination to the existing navigation. Show meaningful empty, loading, disconnected, denied, stale and failed states using existing error/retry components. Do not turn unavailable data into an empty inbox.
  • Bind selection and pending responses to account + stable repository + PR identity. Debounce search and discard obsolete replies. Preserve each PR's location independently; identical PR numbers in different repositories must not collide.
  • Implement overview, Changes and split modes within existing workspace tabs. Match the icon rail, PR list, content boundaries, compact toolbars and independent scroll containers. R4 is a description pane beside a unified-diff workspace, not necessarily an old/new split diff.
  • Support pointer and keyboard resizing with minimum usable widths. Reuse existing layout persistence for non-sensitive UI preferences; validate restored geometry after window/monitor changes. At 900×600 collapse secondary panes into discoverable views rather than compressing everything until controls disappear.
  • Wire local pin/unpin, copy canonical PR link and validated Open in GitHub actions if accepted in [CR-1][P1] Establish the Code Review reference contract and deterministic fixtures #320. Do not invent data-sync semantics for pins. Unsupported AI, voice and write controls remain explicitly unavailable until their owning slices deliver them.
  • Preserve logical focus across navigation and menu/popover dismissal; support Escape, visible focus, accessible selected states and reduced motion. Do not let the floating composer obscure the end of a document or trap keyboard navigation.
  • Connect [CR-2][P1] Add a main-owned, revision-aware GitHub PR read service #321's real main-process read path and test cancellation/account switching through it. Keep network credentials and raw transport details out of renderer state.

Acceptance criteria

  • AC1: users reach the R1 and R4 workspace arrangements through real controls, not test-only DOM substitutions; selected PR and tab identity remain correct.
  • AC2: rapid search/PR/account switching cannot reveal a stale reply from another context. Back/forward and returning to a PR restore the intended view without replaying actions.
  • AC3: list, overview, diff and file-tree scroll regions do not unintentionally scroll each other. Resizing and reopening the app do not strand controls outside the window.
  • AC4: keyboard-only operation works at 900×600 and 1440×900; primary controls remain discoverable and focus returns after dismissal. Original-reference captures use [CR-1][P1] Establish the Code Review reference contract and deterministic fixtures #320's calibrated dimensions/DPR.
  • AC5: existing local Changes, threads, tab persistence and other navigation regressions pass. No remote PR can invoke local Discard through this host.
  • AC6: fixtures and an authenticated read-only navigation smoke test are reported separately. Missing authenticated evidence remains an open gate.

Validation / handoff

Run focused Playwright navigation tests, then pnpm build && pnpm test and pnpm test:e2e as required by AGENTS.md. Record screenshots for normal/narrow/split/error states, the exact SHA and commands, and reviewed visual deviations. Save evidence outside Playwright's wiped test-results directory. Update docs/status.md; do not merge or release. Primary references: #319, Playwright snapshots, Electron security.

Long-running goal prompt

Implement CR-3 in TypeSafeAI/modex using #319's reference states and #320's accepted contract/source map. Read current issues, scoped AGENTS.md and open PRs; verify repo, main SHA and your owned worktree. Start with the smallest failing navigation test. Reuse the actual shell, tabs, theme tokens, error components and persistence. Build the inbox and overview/Changes/split host against deterministic fixtures, then integrate #321 when its read artifact is ready. Preserve account/repository/PR identity, independent scrolling and accessible keyboard resizing. Keep local Changes and its destructive actions separate from remote PRs. Do not add another app framework, auth system, fake working controls, provider calls or GitHub writes. Check normal and narrow layouts visually, run regression tests, and checkpoint exact SHA, changed files, commands/results, evidence and one next action. Stop on ownership, scope or reference blockers; finish with a small signed PR and honest fixture versus live evidence. Do not merge or deploy.

Follow-up / resume prompt

Resume CR-3 from the latest issue/PR checkpoint and actual diff. Verify worktree ownership and current source, then rerun the navigation smoke test. Check which #320/#321 artifacts are accepted; do not rebuild them or wait for unrelated tracker closure. Continue one failing acceptance item, especially selection races, restored layout and keyboard focus. Preserve other agents' shell work and reconcile shared-file ownership before editing. Update screenshots only after reviewing the visual difference, not simply to make tests pass. Record remaining live/manual gates and the next action; do not merge.

Independent review prompt

Exercise the navigation with rapid PR/account switching, a narrow window, keyboard-only input, restored tabs and delayed responses. Compare R1/R4 geometry and independent scrolling against approved references. Verify unavailable controls are honest and remote views cannot call local Discard. Inspect the actual diff and fresh test evidence; report findings by acceptance ID rather than trusting the implementer's completion summary.

Landed host and remaining acceptance — 2026-10-21

PR #367 landed the read-service-integrated inbox/workspace host at
af49881 after required PR CI and protected merge-queue validation.
Current-head local validation passed build/typecheck, 1,983 unit tests (16 skips),
387 desktop E2E cases (6 opt-in skips), and 7 site cases.

This issue remains open: original R1–R4 comparison and human keyboard/VoiceOver acceptance
are not recorded as complete. The host has explicit placeholders for overview/activity and
remote diff/file content owned by #323/#324; populated-content scroll behavior must still be
verified with those slices. Automated identity/history/layout tests and the historical
authenticated navigation smoke are distinct evidence, as documented in
docs/reviews/2026-10-21-cr3-read-integration.md.

GitHub automatically marked this issue completed at the merge because a negated closing
phrase appeared in the PR description. That phrase has been corrected; this status preserves
the outstanding acceptance work rather than claiming completion from the host merge.

Activity

  1. BunsDev commented on Oct 20, 2026

    @BunsDev
    ContributorAuthor

    The CR-1 contract is on main as 43d5b9c (#334). Bind to these files at that commit:

    • apps/desktop/src/shared/code-review.ts (the contract)
    • apps/desktop/test/fixtures/code-review.ts (synthetic fixtures)
    • docs/design/code-review.md (source map, references, inventory, open decisions)

    For CR-3:

    • Apply a reply only through acceptsReply.
    • Group the inbox with inboxGroup, and count with knownCount and isEmpty.
    • capabilities(connection) says which controls are unavailable, and why.
    • The R1 and R4 fixtures, the calibration and the control inventory (C01–C82) are in the design page.
    • E2E serves the fixtures by replacing main's code-review:read handler.

    Coven-Harness: Claude Code / claude-opus-5-5

  2. BunsDev commented on Oct 20, 2026

    @BunsDev
    ContributorAuthor

    CR-3 checkpoint: inbox and workspace host up for review as #346

    Where:

    What it delivers: the Code Review destination, the inbox and the PR workspace host:

    • PR and Changes tabs in the title bar, one pane (R1) or side by side (R4).
    • Back and forward between PRs.
    • Pin, copy link and Open in GitHub.
    • Resizable, persisted dividers.
    • Folding at 900×600.

    Every read goes through code-review:read, and replies are applied only through acceptsReply. The controls it delivers are C02, C03, C05, C06, C08, C11–C15, C17–C20, C23–C26, C62, C74, C75 and C80. The design page's new Workspace (CR-3) section maps each to its test.

    Fixture evidence on 8883428:

    • pnpm build && pnpm test and pnpm test:e2e exited 0. Desktop E2E: 372 passed, 6 skipped.
    • Code Review spec 9/9 and unit 12/12.
    • Each of 13 mutations of the pure modules failed a test.

    The built geometry is within the captures' crop offsets of R1 and R4 (the split at 663 | 473 | 290 against 659 | 470 | 290).

    Open:

    • Val's call: should the destination show before Gate A completes? Built apps hide it unless localStorage["modex.codeReview"] is "on".
    • AC6 and the live read path: the authenticated navigation smoke test, and connecting the real read path, wait for CR-2's handler.

    Next action: once CR-2 lands, merge main, run the navigation spec against the real handler, and record the authenticated smoke test separately.

    Coven-Harness: Claude Code / claude-opus-5-5

  3. BunsDev commented on Oct 20, 2026

    @BunsDev
    ContributorAuthor

    For CR-3, from #349's re-review:

    • Fix mergePages before relying on it. A PR closing between pages can offset a skipped row, so the merged list reads complete. Also require every page to report the same total.
    • Retry once at most. Retry a read refused with "sign-in changed" one time, then show the refusal.

    Coven-Harness: Claude Code / claude-opus-5-5

  4. mariscate commented on Oct 22, 2026

    @mariscate

    Refreshed goal prompts — 2026-10-22 (main at 0e674e7)

    Where it stands: the inbox and workspace host landed in #367 (af49881), integrated with CR-2's main-owned read service. The code is in apps/desktop/src/renderer/code-review/ (CodeReviewView, Inbox, PrPane, navigation, reader, pins), not the components/code-review/ path proposed above. Evidence: 2026-10-21-cr3-read-integration.md. Still open:

    Long-running goal prompt

    Finish CR-3 (#322) acceptance in TypeSafeAI/modex. The host is already on main (#367, af49881), so do not rebuild it. Read AGENTS.md, #319, this issue's checkpoints, docs/design/code-review.md and docs/reviews/2026-10-21-cr3-read-integration.md, then verify main's SHA. Work in your own worktree. Step 1: show with tests that the #349 re-review items hold on main: mergePages requires the same total on every page and doesn't call a list with a skipped row complete, and a "sign-in changed" read is retried once at most. Add a red test for any that fails, then fix it. Step 2: once #323/#324 replace PrPane's placeholders, test that list, overview, diff and file tree scroll independently and keep focus, at 900×600 and 1440×900. Step 3: run the R1–R4 geometry comparison against #320's calibrated references (off-repo, at their DPR) and record each reviewed deviation. Step 4: prepare the human keyboard/VoiceOver checklist for Val, reusing docs/reviews/2026-10-20-keyboard-voiceover's format. Run pnpm build && pnpm test && pnpm test:e2e. Record exact SHAs and evidence in docs/reviews and docs/status.md. Leave the issue open until Val records the human result. No merging without Val's go-ahead.
    

    Follow-up / resume prompt

    Resume #322 acceptance. Fetch main and check whether #323/#324 have landed; until they have, only steps 1 and 4 can proceed. Re-run e2e/code-review-navigation.spec.ts and code-review-ipc.spec.ts on current main before trusting earlier results. Pick the next unchecked AC and change the smallest thing that proves it. Don't refresh screenshots or goldens without reviewing the differences. Post a checkpoint: SHA, commands and results, which ACs are proven by fixtures and which by authenticated runs, what remains for a human, and one next action.
    
  5. BunsDev commented on Oct 23, 2026

    @BunsDev
    ContributorAuthor

    Refreshed goal prompts — 2026-10-23 (main at 203a350)

    On main already:

    Nothing for this issue has landed since 0e674e7.

    Still open:

    Long-running goal prompt

    Finish CR-3 (#322) acceptance in TypeSafeAI/modex. The host is already on main (#367, af49881), so do not rebuild it.
    
    Read AGENTS.md, #319, this issue's checkpoints, docs/design/code-review.md and docs/reviews/2026-10-21-cr3-read-integration.md, then verify main's SHA. Create your own worktree with scripts/worktree.sh new.
    
    Step 1, #349's re-review items. Write red tests:
    - mergePages: pages that report different totals must not read as complete, and a PR that closes between pages must not make a list with a skipped row complete;
    - a read refused with "sign-in changed" is retried once at most, then the refusal is shown.
    Then fix the code.
    
    Step 2: after #393 (CR-4) and CR-5 land, test that the list, overview, diff and file tree scroll independently and keep focus, at 900×600 and 1440×900.
    
    Step 3: run the R1–R4 geometry comparison with populated content, using #372's journey captures and ui:compare. It needs the original references; ask Val where they are now, and don't substitute synthetic captures. Record each reviewed deviation.
    
    Step 4: give Val the VoiceOver/keyboard checklist from #372 (docs/reviews/2026-10-22-cr8-voiceover).
    
    Run pnpm build && pnpm test && pnpm test:e2e. Record exact SHAs and evidence in docs/reviews and update docs/status.md. Open signed PRs; nothing merges without Val's go-ahead. Leave the issue open until Val records the human result.
    

    Follow-up / resume prompt

    Resume #322 acceptance.
    1. Confirm you own the worktree and fetch main.
    2. Check whether #393 and CR-5 have landed. Until they have, only Steps 1 and 4 can proceed.
    3. Re-run e2e/code-review-navigation.spec.ts and code-review-ipc.spec.ts on current main before trusting earlier results.
    4. Pick the next unchecked AC and change the smallest thing that proves it. Don't refresh screenshots or goldens without reviewing the differences.
    Post a checkpoint: SHA, commands and results, which ACs are proven by fixtures and which by authenticated runs, what remains for a human, and one next action.
    
  6. self-assigned this
    on Oct 28, 2026
  7. BunsDev commented on Oct 28, 2026

    @BunsDev
    ContributorAuthor

    CR-3 acceptance checkpoint — 2026-10-28 (main at 7a01df2)

    Where: worktree and branch cr3-acceptance, PR #437. Code 271d9f6876a379defeec176f75360e2d8454e265, docs 0cdf369 (both signed). Evidence outside the repository: ~/Documents/Codex/artifacts/modex-cr3-acceptance/2026-10-28/; record: docs/reviews/2026-10-28-cr3-acceptance.md.

    Step 1, #349's re-review items (AC2, AC3), red first:

    • mergePages: a PR closing between pages (31 → second page empty with total 30) merged as complete: true before the fix; now every page must report the same total. Also covers PRs opening between pages and a stale page.
    • "sign-in changed": no automatic retry existed; the Reader now asks once more, then shows the refusal. Fake-main unit test plus an Electron regression through the production read service (held read, hosts.yml rewritten away and back, one retry, second refusal shown, no third read until Retry).

    Step 2, populated scroll/focus (AC3, AC4): e2e/code-review-scroll.spec.ts passes at 1440×900 and 900×600 through the production read path: list, overview, diff and tree scroll alone by wheel and PageDown, nothing else moves, focus is kept, positions restore on return.

    Gates on 271d9f6: pnpm build, pnpm typecheck, pnpm test (desktop 2,018 passed, 17 skipped, 0 failed) and pnpm test:e2e (desktop 473 passed, 7 opt-in skips; site 7/7) all exited 0.

    Step 3, R1–R4: not run. The design page records the originals as gone (2026-10-22) and Spotlight finds none on this Mac. Journey captures (R1, R4, 1440×900, 900×600) and a reference-less compare.html are kept with the evidence; no synthetic capture was substituted. Val: where do the references (or re-captures) live now?

    Step 4, human gate: docs/reviews/2026-10-22-cr8-voiceover/checklist.md is updated with parts C (overview) and D (diff and file tree) and the 900×600 fold; seed.mjs still prints the launch line on this build. The result row waits for Val.

    Proven by fixtures/synthetic gh: AC2, AC3, AC4's scroll half, AC5 (existing suites). Authenticated: the earlier navigation probe only; the retry path was not exercised live. Human: AC4's keyboard/VoiceOver pass, open.

    Next action: Val names the reference location and runs the checklist; then the R1–R4 comparison and its D-rows. Nothing merges without Val's go-ahead.

  8. BunsDev commented on Oct 28, 2026

    @BunsDev
    ContributorAuthor

    Step 3 decision, 2026-10-28: the original R1–R4 references are not on this Mac (searched by name, pixel size, Trash, iCloud Drive and local snapshots; every hit at their CSS sizes is a Playwright capture). Val chose to leave the comparison open for now rather than re-capture or substitute synthetic captures. Recorded in docs/reviews/2026-10-28-cr3-acceptance.md on PR #437. The issue stays open for that comparison and the human keyboard/VoiceOver result.

  9. BunsDev commented on Oct 31, 2026

    @BunsDev
    ContributorAuthor

    CR-3 acceptance checkpoint — 2026-10-30 (main at f51ef7e)

    Where: worktree and branch cr3-resume, PR #464, head fce6ccc (signed). No product code changes. Evidence outside the repository: ~/Documents/Codex/artifacts/modex-cr3-acceptance/2026-10-30/; record: docs/reviews/2026-10-30-cr3-acceptance-resume.md.

    Already on main: Steps 1 and 2 landed in #438 (afec85b, 2026-10-28). CR-4 (#393, 6c40547) and CR-5 (#404, e3527f4) have both landed, so Step 2's precondition holds and nothing was rebuilt.

    Re-run on main f51ef7e, each exiting 0:

    • pnpm install --frozen-lockfile && pnpm build: passed.
    • Step 1 unit files (code-review-contract, -workspace, -read-service): 75 passed.
    • e2e/code-review-navigation.spec.ts, -ipc, -scroll, -regressions: 24 passed in 1.9 min (includes the one-retry sign-in regression and both populated-scroll cases).
    • pnpm typecheck: passed. pnpm test: desktop 2,089 passed, 20 skipped, 0 failed (worktree 120, core 16, native 21 + 6 skips, site 10, store-desktop 16).
    • pnpm test:e2e: desktop 476 passed, 7 opt-in skips, 0 failed, 40.3 min; site 7/7.
    • Read-only authenticated navigation probe (the A-main probe, account BunsDev, gh 2.102.0, disposable home): 6/6, exit 0; the Mac's gh login was unchanged afterwards. GET reads of public PRs Add main's Code Review read service through gh (CR-2) #349 and Preserve native question drafts across thread navigation (CF-3) #348 only.

    Found while reviewing the captures (the smallest change this round): the journey spec's R1 capture read "Checks · not found" and its R4 capture showed "0 of 4 files loaded" with an empty file tree, on this main and in the 2026-10-28 evidence alike, because the synthetic world served no files for field-guide #91 and no checks for lending #73's head. A Step 3 comparison from those captures would have measured empty panes. #464 gives the journey's world four files for #91 (+11 −4) and four check runs plus a status for #73; geometry assertions are unchanged (R4: inbox 53–333, PR pane 333–996, diff 997–1470, tree 1470–1760) and the spec passes. The populated R1–R4, 1440×900 and 900×600 captures are kept under journey-captures-populated/ with a reference-less compare.html. No synthetic capture was substituted for a reference.

    By evidence:

    • Fixtures / synthetic gh: AC1 (arrangements and captures), AC2, AC3, AC4's scroll and keyboard half, AC5, AC6's fixture half.
    • Authenticated: AC6 (probe 6/6 on f51ef7e); AC1 and AC2's navigation through the real service. The sign-in retry path was not exercised live.
    • Human, still open: AC4's keyboard/VoiceOver pass; AC1's geometry comparison with the originals.

    Step 3 — Val: the originals were confirmed gone on 2026-10-22 and you deferred on 2026-10-28. Where do the references, or re-captures, live now? With a directory holding R1.png–R4.png, the comparison is pnpm exec playwright test e2e/code-review-journey.spec.ts then pnpm run ui:compare -- --captures test-results/ui-code-review <that directory>; each reviewed deviation gets a D-row.

    Step 4 — Val: the checklist is docs/reviews/2026-10-22-cr8-voiceover/checklist.md (parts A–D, repeated at 900×600), now pinned to afec85b or later. From apps/desktop after pnpm build: node ../../docs/reviews/2026-10-22-cr8-voiceover/seed.mjs /tmp/modex-cr8-voiceover, run the printed line, set localStorage["modex.codeReview"]="on" and reload. The result row waits for you.

    Next action: Val names the reference location and runs the checklist; #464 waits for CI and Val's go-ahead. The issue stays open.

  10. reopened this on Nov 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

enhancementNew feature or request

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions