Show PR status for branch worktrees at a merged PR head#6607
Conversation
The left worktree sidebar showed only the branch (no PR) for a worktree checked out at a merged PR's head commit, while the right Checks panel showed the PR. The merged-PR visibility check compared the PR head against the main repo's HEAD instead of the worktree's HEAD, so it hid the PR for every secondary worktree. Thread the worktree's checked-out HEAD oid into the GitHub branch lookup so the merged-at-head decision uses the worktree's own HEAD. Head-gate fallback PR numbers so a worktree that has moved past a merged PR head does not resurrect it. Also preserve the last known review on a transient lookup failure instead of caching a definitive 'no review' miss. currentHeadOid is GitHub-only; GitLab/Bitbucket/Azure/Gitea are unaffected. No new git/gh calls in the common path and no new logging. Co-authored-by: Orca <help@stably.ai>
📝 WalkthroughWalkthroughThis change threads 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/src/store/slices/github.ts (1)
1031-1046: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHead-gate the hosted-review fallback too.
The new guard only checks
cachedPR, buthostedReviewFallbackPRNumbercan still return a stale merged PR number whenprCacheis empty. That lets a moved-past worktree re-enqueue the historical PR via the hosted-review cache. Return the hosted review metadata here and suppress GitHub merged fallbacks whenheadSha !== worktree.head.Suggested direction
- const hostedReviewFallbackPRNumber = githubHostedReviewFallbackPRNumber( + const hostedReviewFallback = githubHostedReviewFallbackReview( state, repoPath ?? repo.path, repo.id, @@ - const fallbackPRNumber = - worktree.linkedPR == null && !cachedMergedPRMovedPastHead - ? (cachedFallbackPRNumber ?? hostedReviewFallbackPRNumber) + const hostedReviewMergedPRMovedPastHead = + worktree.linkedPR == null && + hostedReviewFallback?.provider === 'github' && + hostedReviewFallback.state === 'merged' && + hostedReviewFallback.headSha !== worktree.head + const fallbackPRNumber = + worktree.linkedPR == null && + !cachedMergedPRMovedPastHead && + !hostedReviewMergedPRMovedPastHead + ? (cachedFallbackPRNumber ?? hostedReviewFallback?.number ?? null) : nullAlso applies to: 1086-1105
src/renderer/src/components/right-sidebar/ChecksPanel.tsx (1)
1189-1195: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThread
currentHeadOidthrough every hosted-review refresh path.The automatic effect and entry refresh now send
activeWorktree?.head, but the manual refresh, mutation refresh, linked-PR refresh, and post-createrefreshHostedReviewCardcalls still omit it. Those secondary-worktree paths can fall back to the main repo HEAD and reproduce the merged-PR visibility bug. AddcurrentHeadOid: activeWorktree?.head ?? nullto the remainingrefreshHostedReviewCardoption objects and include it in the relevant callback dependencies.Also applies to: 1871-1881, 1925-1935, 2085-2091, 2183-2193, 2212-2222, 2863-2872, 3139-3160
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ed8a31eb-544d-403b-940c-aaff05be4607
📒 Files selected for processing (19)
src/main/github/client.tssrc/main/ipc/hosted-review.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/rpc/methods/hosted-review.test.tssrc/main/runtime/rpc/methods/hosted-review.tssrc/main/source-control/forge-provider.test.tssrc/main/source-control/forge-provider.tssrc/main/source-control/hosted-review.test.tssrc/main/source-control/hosted-review.tssrc/renderer/src/components/right-sidebar/ChecksPanel.tsxsrc/renderer/src/components/sidebar/WorktreeCard.hosted-review-refresh.test.tsxsrc/renderer/src/components/sidebar/WorktreeCard.pr-display.test.tsxsrc/renderer/src/components/sidebar/WorktreeCard.tsxsrc/renderer/src/store/slices/github.test.tssrc/renderer/src/store/slices/github.tssrc/renderer/src/store/slices/hosted-review-cache-race.test.tssrc/renderer/src/store/slices/hosted-review.test.tssrc/renderer/src/store/slices/hosted-review.tssrc/shared/hosted-review.ts
The branch-scoped hosted-review cache could return (or, on a failed refresh, preserve) a merged GitHub PR for a head the worktree has moved off of, within the cache TTL. Skip cache reuse and error-preservation when a cached merged GitHub review's headSha no longer matches the requested worktree HEAD, forcing a fresh head-aware lookup. Co-authored-by: Orca <help@stably.ai>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/renderer/src/store/slices/hosted-review.test.ts (2)
534-547: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the refetch forwards the updated
currentHeadOid.This currently proves only that a second lookup happens. Because the mock is value-only, the test still passes if the refetch sends the stale head or
null, which is the contract this PR is threading through. Add atoHaveBeenNthCalledWith(...)assertion for the first call using'aaaaaaa'and the second using'bbbbbbb'.
550-595: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLock down the cache state after the failed forced refresh.
resolves.toBeNull()verifies the failed fetch does not immediately fall back to the stale merged review, but it does not catch leaving that stale entry reusable inhostedReviewCache. A follow-up non-force read or direct cache assertion would cover the regression described in the PR objective.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1194cb55-8dee-48e5-81c8-6ff625735141
📒 Files selected for processing (2)
src/renderer/src/store/slices/hosted-review.test.tssrc/renderer/src/store/slices/hosted-review.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/renderer/src/store/slices/hosted-review.ts
1 participant
What changes
The left worktree sidebar now shows PR/review status for branch worktrees whenever it makes sense — in particular, a merged GitHub PR whose head commit equals the worktree's checked-out HEAD now shows a PR badge in the left sidebar, matching what the right Checks panel already shows.
What does not change: a worktree that has moved past a merged PR's head (checked out at a different commit) still shows branch-only — a historical merged PR is not resurrected. Detached-HEAD worktrees are out of scope here. Non-GitHub providers (GitLab/Bitbucket/Azure DevOps/Gitea) are unaffected.
Root cause
The merged-PR visibility check hid a merged PR unless the current HEAD equalled the PR head, but it read the current HEAD by running
git rev-parse HEADagainst the registered (main) repo path, not the worktree being inspected. For any secondary worktree the main repo is almost always on a different commit, so the merged PR was hidden even when the worktree was sitting exactly on it. The right Checks panel avoided this because it passes a fallback PR number that keeps the PR alive; the left sidebar passed none.Approach
currentHeadOid) end-to-end into the GitHub branch lookup (renderer → IPC/runtime RPC → forge provider →getPRForBranchOutcome). The merged-at-head decision now uses the worktree's own HEAD. When no oid is supplied, behavior is exactly as before (no new git call). The oid is supplied fromworktree.head, which the renderer already has — so no newgit rev-parseper card.currentHeadOidlives only on the GitHub lookup path; other providers ignore it. SSH/runtime supply the oid from the already-transportedworktree.head, so there is no new server-side git exec.Design review
Ran an automated design-review loop (5 iterations to convergence). It tightened the cache-interaction story: the warm-cache badge surfaces through the
prCachedisplay path (not the hint-suppressed hosted-review entry), and the head-gate must hold at every fallback-number source (sidebar refresh, hover-reveal, and the coordinator candidate) or a moved-past merged PR can be re-seeded. Both reviewers returned clean on the final pass.Implementation & deviations
Implemented as designed. The end-to-end
currentHeadOidthreading, the head-gating at all three fallback sources, and the warm-case display preference are all in place. One belt-and-suspenders backend guard was added: when an explicit head oid differs from a merged PR's head, neither the confirmed-merged-branch path noracceptMergedFallbackPRoverrides the hide — this also brings the right Checks panel into alignment (it no longer keeps a moved-past merged PR). No deviations from the design intent.Completeness verification
A read-only verification pass confirmed all 13 design requirements are implemented (Change 1 threading: 9/9; Change 2 head-gate + warm display: 4/4), with file:line evidence, and confirmed no
eslint-disable max-lineswas added.Headline behavior status
Implemented and validated in the running app. The merged-at-head PR badge appears in the left sidebar in production code paths (not fixtures/scaffolding).
Performance
Audited against the
perfchecklist. No findings. Touched hot paths: the per-visible-card hosted-review refresh effect and the main-process branch lookup. No newgit rev-parseon the common (oid-supplied) path; gh-call parity with the right Checks panel (the exact PR lookup only runs when branch lookup misses, which does not happen in the merged-at-head common case);buildPRRefreshCandidatehead-gate is a pure local comparison that only ever withholds a fallback; no new store-subscription churn, no unbounded scans, no new logging; #6203 burst/concurrency bounds untouched.Code review
review-codetwo-reviewer loop completed in one round with both reviewers clean (no actionable findings).Validation
PR: Mergedwith the merged-PR badge; the hover-card showedPR #23 · State: Merged · Checks: Failing; the right Checks panel showed#23 MERGED— full parity. A second worktree advanced past the merged head correctly showed branch-only. Console: 0 errors; no git/gh/hosted-review stack traces.pnpm run typecheckclean;pnpm run lintclean (one pre-existing unrelateduseGitStatusPolling.tswarning); broad targeted vitest sweep (renderer store/sidebar/right-sidebar + main github/source-control/ipc/runtime) — all green (4000+ tests). Existing call-shape assertions updated for the newcurrentHeadOidfield; added focused tests for the explicit-oid hide/keep, the upstream-error throw, the transient-error cache preservation, and the coordinator head-gate.Assumptions / residual risk
prCachepath remains main-repo-scoped by design (it relies on the head-gate to avoid stale merged PRs); the cold-cache badge is served by the hosted-review path.Made with Orca 🐋