fix(web): select pull request on review surface#1857
Merged
Conversation
📝 WalkthroughWalkthroughAdds task-scoped multi-PR selection, stable PR-key propagation through review and diff panels, repository identity resolution, responsive review dialog mounting, and desktop/mobile/tablet tests and documentation. ChangesMulti-PR review experience
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| apps/web/components/review/review-repository-identity.ts | Resolves the selected PR to an exact branch, repository position, or unique branch slug. |
| apps/web/components/task/task-changes-panel-state.ts | Propagates PR-scoped file identity through visible diff and file-selection state. |
| apps/web/hooks/domains/github/use-review-pr-selection.ts | Provides shared task-scoped PR selection and fallback behavior. |
| apps/web/components/review/review-dialog-pr-state.tsx | Adds PR-keyed transient state and loading, retry, and auto-close handling. |
Reviews (7): Last reviewed commit: "test(web): cover PR review boundary bran..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR fixes multi-PR task review behavior in apps/web by introducing a task-scoped pull request (PR) selection, ensuring Review/Changes surfaces render the intended PR revision (instead of implicitly using the first linked PR) across desktop, docked panels, and mobile/tablet layouts, with stale-response protection and regression coverage.
Changes:
- Add a shared, task-scoped PR selection state (default primary PR; override per task; safe fallback when PR associations change).
- Thread exact PR identity (
prKey) through Review/Changes rendering and routing (dockview + mobile diff sheet) and harden PR file fetching against stale responses. - Add unit + Playwright E2E coverage and update spec/public docs to describe multi-PR Review behavior.
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/specs/tasks/multi-branch/spec.md | Spec update describing one-PR-at-a-time Review selection semantics and constraints. |
| docs/public/sessions-and-review.md | Public docs update explaining the PR selector and precedence rules. |
| docs/plans/multi-pr-review-selector/plan.md | Implementation plan record for the selector work. |
| docs/plans/multi-pr-review-selector/task-01-selection-foundation.md | Plan task record: state/keying foundation for selection. |
| docs/plans/multi-pr-review-selector/task-02-review-selector-ui.md | Plan task record: selector UI + responsive behavior. |
| docs/plans/multi-pr-review-selector/task-03-inline-review-routing.md | Plan task record: PR-qualified routing and inline review identity. |
| docs/plans/multi-pr-review-selector/task-04-e2e-verification.md | Plan task record: E2E proof across viewports. |
| apps/web/lib/state/store.ts | Adds reviewPRSelection + setReviewPRSelection to app state shape. |
| apps/web/lib/state/store.test.ts | Tests new default state + per-task override persistence. |
| apps/web/lib/state/store-overrides.ts | Wires reviewPRSelection into default override builder. |
| apps/web/lib/state/slices/ui/ui-slice.ts | Adds default reviewPRSelection and setter action implementation. |
| apps/web/lib/state/slices/ui/types.ts | Defines ReviewPRSelectionState and action signature. |
| apps/web/lib/state/dockview-store.ts | Adds optional prKey to dockview diff panel params. |
| apps/web/lib/state/dockview-panel-actions.ts | Scopes diff preview/pinned identity by prKey to avoid cross-PR reuse. |
| apps/web/lib/state/dockview-panel-actions.test.ts | Verifies same-path diffs from different PRs don’t reuse pinned panels. |
| apps/web/lib/state/default-state.ts | Adds reviewPRSelection to default/merge behavior for initial state. |
| apps/web/hooks/use-review-sidebar-resize.ts | Reattaches resize observer when Review container is re-keyed by PR/session. |
| apps/web/hooks/use-review-sidebar-resize.test.ts | Tests ResizeObserver lifecycle on keyed container replacement. |
| apps/web/hooks/domains/session/use-review-sources.ts | Adds selected PR plumbing, explicit PR key support, and PR diff error/refresh surface. |
| apps/web/hooks/domains/github/use-review-pr-selection.ts | New hook + helpers for resolving task-scoped selected PR with fallback. |
| apps/web/hooks/domains/github/use-review-pr-selection.test.ts | Unit tests for selection default/override/fallback and store usage. |
| apps/web/hooks/domains/github/use-pr-review-repository-identity.ts | New hook resolving PR repository identity from task/worktree/session metadata. |
| apps/web/hooks/domains/github/use-pr-review-repository-identity.test.tsx | Tests identity resolution for boot vs live worktree metadata. |
| apps/web/hooks/domains/github/use-pr-diff.ts | Adds sourceKey masking to prevent stale PR files flashing during selection changes. |
| apps/web/hooks/domains/github/use-pr-diff.test.ts | Tests stale-request masking and late-resolution handling. |
| apps/web/e2e/tests/review/review-multi-pr.spec.ts | New desktop E2E ensuring PR switching doesn’t show stale diff content. |
| apps/web/e2e/tests/review/mobile-review-multi-pr.spec.ts | New mobile/tablet E2E verifying touch targets, containment, and PR switching. |
| apps/web/e2e/pages/session-page.ts | Adds selectors/helpers for the new Review PR selector and diff text extraction. |
| apps/web/e2e/helpers/multi-pr-review.ts | Seeds a multi-PR task with distinct PR markers for E2E validation. |
| apps/web/e2e/helpers/api-client.ts | Adds helper to associate GitHub task PRs via API for E2E seeding. |
| apps/web/components/task/use-review-dialog.ts | Switches Review dialog data flow to selected PR + PR diff loading/error/retry. |
| apps/web/components/task/task-changes-panel.tsx | Threads explicit PR key into file-mode diff rendering and adds PR diff boundary states. |
| apps/web/components/task/task-changes-panel.test.ts | Moves/extends tests for new helper module and PR-related deferral/auto-close logic. |
| apps/web/components/task/task-changes-panel-state.ts | New extracted helpers for PR deferral, file filtering, and auto-close semantics. |
| apps/web/components/task/mobile/session-tablet-layout.tsx | Mounts shared Review dialog on tablet layout via TaskReviewDialogMount. |
| apps/web/components/task/mobile/session-mobile-layout.tsx | Replaces mobile-local Review mount with shared TaskReviewDialogMount. |
| apps/web/components/task/mobile/mobile-diff-sheet.tsx | Adds prKey propagation into TaskChangesPanel file-mode. |
| apps/web/components/task/mobile/mobile-changes-panel.tsx | Preserves prKey when opening PR file diffs from mobile changes panel. |
| apps/web/components/task/dockview-shared.tsx | Threads prKey through dockview shared diff-view routing. |
| apps/web/components/task/dockview-session-tabs.ts | Imports prTaskKey from shared utils for stable PR identity. |
| apps/web/components/task/dockview-review-dialog.tsx | Refactors to TaskReviewDialogMount (shared across desktop/mobile/tablet). |
| apps/web/components/task/dockview-panel-content.tsx | Threads prKey through dockview diff-view routing. |
| apps/web/components/task/dockview-panel-content.diff.test.tsx | Adds coverage ensuring prKey/PR source survives dockview rendering paths. |
| apps/web/components/task/dockview-desktop-layout.tsx | Uses TaskReviewDialogMount instead of duplicating Review mount + overlay wiring. |
| apps/web/components/task/dockview-add-panel-items.tsx | Consolidates PR identity imports onto pr-utils. |
| apps/web/components/task/changes-top-bar.tsx | Adds Review PR selector to the Changes top bar when multiple PRs exist. |
| apps/web/components/task/changes-panel-timeline.tsx | Adds optional prKey to PR timeline file row shape. |
| apps/web/components/task/changes-panel-pr-files.tsx | Stamps data-pr-key and propagates prKey into diff open options. |
| apps/web/components/task/changes-panel-pr-files.test.tsx | Ensures prKey is passed through when opening PR diffs. |
| apps/web/components/task/changes-panel-helpers.ts | Stamps PR file rows with the stable prKey. |
| apps/web/components/task/changes-panel-helpers.test.ts | Tests that PR rows are stamped with the exact PR key. |
| apps/web/components/task/changes-panel-data.tsx | Builds PR groups/labels while preserving stable PR identity and selected-PR progress source. |
| apps/web/components/task/changes-diff-target.ts | Extends diff routing types to carry optional prKey. |
| apps/web/components/review/review-top-bar.tsx | Adds Review PR selector to the Review toolbar with responsive wrapping. |
| apps/web/components/review/review-repository-identity.ts | New pure helper for deriving review repository identity for PRs (worktree-aware). |
| apps/web/components/review/review-repository-identity.test.ts | Tests primary vs sibling identity resolution behavior. |
| apps/web/components/review/review-pr-selector.tsx | New shared PR selector component (desktop + mobile + tablet affordances). |
| apps/web/components/review/review-dialog.tsx | Refactors Review dialog into surface + PR-keyed transient state and boundary behaviors. |
| apps/web/components/review/review-dialog.build-files.test.ts | Adds tests for PR transitions, transient state reset, and selection setter correctness. |
| apps/web/components/review/review-dialog-surface.tsx | New extracted Review dialog surface with PR boundary + sidebar resize re-keying. |
| apps/web/components/review/review-dialog-pr-state.tsx | New helpers for auto-close rules, transient state, and PR diff loading/error boundary. |
| apps/web/components/review/review-dialog-pr-state.test.tsx | Tests PR diff boundary behavior (no stale children; retry; local-only ignore). |
| apps/web/components/github/pr-utils.ts | Moves/centralizes prTaskKey into shared PR utilities. |
| apps/web/components/github/pr-topbar-button.tsx | Updates import path to shared prTaskKey. |
| apps/web/components/github/pr-detail-panel.tsx | Removes local prTaskKey definition; imports from shared utils. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Preserve keyed PR panels when the PR is missing
When a dockview panel was opened with a prKey but that PR is not in prs anymore (for example a restored pr-detail|owner/repo/2 tab after the PR association was removed), this expression falls back to the active primary PR and the keyed tab can display/act on the wrong pull request. Only use activePR for legacy panels with no params.prKey; for keyed panels, render the missing-PR placeholder until the exact key is present.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/hooks/domains/github/use-pr-review-repository-identity.ts`:
- Around line 17-41: Update normalizeReviewWorktrees so live-only entries from
liveWorktrees are converted to ReviewWorktreeIdentity objects before insertion,
explicitly mapping id, repositoryId, branch, and path while leaving branchSlug
and position unset. Preserve the existing seen check and append the normalized
identity instead of the raw Worktree.
In `@apps/web/lib/state/store.test.ts`:
- Around line 32-48: Extend the store tests around createAppStore and
reviewPRSelection to cover initialization with caller-supplied
selectedKeyByTaskId. Create the store with an initial reviewPRSelection
containing a task-to-PR mapping, then assert the mapping survives in
store.getState().reviewPRSelection after initialization.
In `@docs/plans/multi-pr-review-selector/task-01-selection-foundation.md`:
- Around line 16-40: Update Task 01’s plan to include
use-pr-review-repository-identity.ts and its corresponding test in the Files
list. Extend the focused Verification command to run that test alongside the
existing selection and store tests, keeping the scope limited to the
repository-identity foundation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f25e04d6-ae61-4d95-a6c3-0942fd947b99
📒 Files selected for processing (65)
apps/web/components/github/pr-detail-panel.tsxapps/web/components/github/pr-topbar-button.tsxapps/web/components/github/pr-utils.tsapps/web/components/review/review-dialog-pr-state.test.tsxapps/web/components/review/review-dialog-pr-state.tsxapps/web/components/review/review-dialog-surface.tsxapps/web/components/review/review-dialog.build-files.test.tsapps/web/components/review/review-dialog.tsxapps/web/components/review/review-pr-selector.tsxapps/web/components/review/review-repository-identity.test.tsapps/web/components/review/review-repository-identity.tsapps/web/components/review/review-top-bar.tsxapps/web/components/task/changes-diff-target.tsapps/web/components/task/changes-panel-data.tsxapps/web/components/task/changes-panel-helpers.test.tsapps/web/components/task/changes-panel-helpers.tsapps/web/components/task/changes-panel-pr-files.test.tsxapps/web/components/task/changes-panel-pr-files.tsxapps/web/components/task/changes-panel-timeline.tsxapps/web/components/task/changes-top-bar.tsxapps/web/components/task/dockview-add-panel-items.tsxapps/web/components/task/dockview-desktop-layout.tsxapps/web/components/task/dockview-panel-content.diff.test.tsxapps/web/components/task/dockview-panel-content.tsxapps/web/components/task/dockview-review-dialog.tsxapps/web/components/task/dockview-session-tabs.tsapps/web/components/task/dockview-shared.tsxapps/web/components/task/mobile/mobile-changes-panel.tsxapps/web/components/task/mobile/mobile-diff-sheet.tsxapps/web/components/task/mobile/session-mobile-layout.tsxapps/web/components/task/mobile/session-tablet-layout.tsxapps/web/components/task/task-changes-panel-state.tsapps/web/components/task/task-changes-panel.test.tsapps/web/components/task/task-changes-panel.tsxapps/web/components/task/use-review-dialog.tsapps/web/e2e/helpers/api-client.tsapps/web/e2e/helpers/multi-pr-review.tsapps/web/e2e/pages/session-page.tsapps/web/e2e/tests/review/mobile-review-multi-pr.spec.tsapps/web/e2e/tests/review/review-multi-pr.spec.tsapps/web/hooks/domains/github/use-pr-diff.test.tsapps/web/hooks/domains/github/use-pr-diff.tsapps/web/hooks/domains/github/use-pr-review-repository-identity.test.tsxapps/web/hooks/domains/github/use-pr-review-repository-identity.tsapps/web/hooks/domains/github/use-review-pr-selection.test.tsapps/web/hooks/domains/github/use-review-pr-selection.tsapps/web/hooks/domains/session/use-review-sources.tsapps/web/hooks/use-review-sidebar-resize.test.tsapps/web/hooks/use-review-sidebar-resize.tsapps/web/lib/state/default-state.tsapps/web/lib/state/dockview-panel-actions.test.tsapps/web/lib/state/dockview-panel-actions.tsapps/web/lib/state/dockview-store.tsapps/web/lib/state/slices/ui/types.tsapps/web/lib/state/slices/ui/ui-slice.tsapps/web/lib/state/store-overrides.tsapps/web/lib/state/store.test.tsapps/web/lib/state/store.tsdocs/plans/multi-pr-review-selector/plan.mddocs/plans/multi-pr-review-selector/task-01-selection-foundation.mddocs/plans/multi-pr-review-selector/task-02-review-selector-ui.mddocs/plans/multi-pr-review-selector/task-03-inline-review-routing.mddocs/plans/multi-pr-review-selector/task-04-e2e-verification.mddocs/public/sessions-and-review.mddocs/specs/tasks/multi-branch/spec.md
…e-multi-rv1 # Conflicts: # apps/web/components/task/dockview-session-tabs.ts # apps/web/components/task/mobile/session-mobile-layout.tsx
|
Claude finished @zeval's task in 6m 51s —— View job Claude's code review
Review posted. See full findings report below. 1 blocker — the |
Contributor
Cloudflare Pages docs previewBuilt from docs commit Stable PR alias: https://docs-pr-1857.landing-87j.pages.dev/docs |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 65 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
apps/web/components/review/review-dialog-pr-state.tsx:74
useReviewDialogTransientStatecallssetStateduring render whensourceKeychanges. This pattern can trigger React warnings (and is especially brittle under StrictMode double-invocation), and it adds a render-phase side effect that’s easy to regress into an infinite rerender if the condition ever changes.
Move the state synchronization into an effect keyed on sourceKey, and keep resolvedState as a pure derived value for the current render.
export function useReviewDialogTransientState(sourceKey: string) {
const [state, setState] = useState<ReviewTransientState>(() => ({
sourceKey,
selectedFile: null,
filter: "",
}));
const resolvedState = resolveReviewTransientState(state, sourceKey);
if (resolvedState !== state) setState(resolvedState);
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
FindingsBlocker (must fix before merge)
Suggestion (recommended, doesn't block)
Summary
Verdict: Blocked — fix blocker (#1) before merge. The
|
|
Claude Code is working…
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/components/task/task-changes-panel.test.ts (1)
196-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the new helper fallback branches.
The tests do not exercise the
"all"+ PR-only branch ofshouldBlockChangesForPR(Line 22), nor the non-PR fallback ofresolveSelectedFileRepositoryName(Line 31). Add assertions for both to prevent regressions in the newly introduced branching.As per coding guidelines, every code change must include tests for new or changed logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/components/task/task-changes-panel.test.ts` around lines 196 - 217, Extend the existing shouldBlockChangesForPR tests to assert the "all" view blocks when only PR files are present, and extend resolveSelectedFileRepositoryName tests to cover the non-PR fallback path. Use representative inputs matching each branch and assert the expected fallback results.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/web/components/task/task-changes-panel.test.ts`:
- Around line 196-217: Extend the existing shouldBlockChangesForPR tests to
assert the "all" view blocks when only PR files are present, and extend
resolveSelectedFileRepositoryName tests to cover the non-PR fallback path. Use
representative inputs matching each branch and assert the expected fallback
results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af4f45af-912f-42f0-bf0c-76808f12112a
📒 Files selected for processing (10)
apps/web/components/review/review-dialog-pr-state.test.tsxapps/web/components/review/review-dialog-pr-state.tsxapps/web/components/review/review-repository-identity.test.tsapps/web/components/review/review-repository-identity.tsapps/web/components/task/task-changes-panel-state.tsapps/web/components/task/task-changes-panel.test.tsapps/web/components/task/task-changes-panel.tsxapps/web/hooks/domains/github/use-pr-review-repository-identity.tsapps/web/lib/state/store.test.tsdocs/plans/multi-pr-review-selector/task-01-selection-foundation.md
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/lib/state/store.test.ts
- docs/plans/multi-pr-review-selector/task-01-selection-foundation.md
- apps/web/components/review/review-repository-identity.ts
- apps/web/components/review/review-dialog-pr-state.tsx
- apps/web/hooks/domains/github/use-pr-review-repository-identity.ts
- apps/web/components/task/task-changes-panel.tsx
Re-review in progress
|
|
Claude finished @zeval's task in 4m 21s —— View job Claude finished @josé Almeida's task
3 blockers — each is a missing test branch for a new pure-logic function, all ≤3-line additions to existing |
FindingsHead under review: Blocker (must fix before merge)
Summary
Verdict: Blocked — three new pure-logic functions each have one untested branch. All three are ≤3-line additions to the existing
|
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Multi-PR tasks could silently open the first associated pull request in Review, hiding sibling changes. Review now carries an exact task-scoped PR selection across desktop, docked, and mobile surfaces with stale-request protection.
Important Changes
Validation
make fmtmake typecheckTMPDIR=/tmp/kdv-pr-verify.XXXXXX make testmake lintChecklist
apps/web/), I have added or updated Playwright e2e tests inapps/web/e2e/and verified them withmake test-e2e.docs/public/**and updated them or noted why no docs change is needed.Screenshots
Desktop Review with the sibling PR selector open:
Mobile Review with the contained touch selector: