fix(desktop): harden PR merge detection fallback#2668
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughIntroduces explicit PR resolution and owner-aware branch matching, a new merge helper with repo-context-aware merge + legacy fallback, cache-clear utilities, schema extension for PR head owner, and related tests for message patterns and branch/PR matching. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router as mergePR mutation
participant MergeHelper as mergePullRequest
participant RepoCtx as getRepoContext
participant PRResolver as getPRForBranch
participant GitCLI as gh CLI
participant Legacy as legacy branch-based gh merge
Client->>Router: request merge
Router->>MergeHelper: mergePullRequest(worktreePath,strategy)
MergeHelper->>RepoCtx: getRepoContext(worktreePath)
RepoCtx-->>MergeHelper: repoContext|null
MergeHelper->>PRResolver: getPRForBranch(worktreePath,localBranch,repoContext,headSha)
PRResolver->>GitCLI: gh pr view / gh pr list / gh pr list --search (branch/sha)
GitCLI-->>PRResolver: PR candidates / null
PRResolver-->>MergeHelper: PR data|null
alt PR found
MergeHelper->>GitCLI: gh pr merge <number> --<strategy> [--repo ...]
alt merge succeeds
GitCLI-->>MergeHelper: success
MergeHelper->>WorktreeCaches: clearWorktreeStatusCaches(path)
MergeHelper-->>Router: {success:true,mergedAt}
else "no pull request found" error
MergeHelper->>Legacy: fallback merge by branch
Legacy-->>MergeHelper: result
MergeHelper-->>Router: result
end
else PR not found or error
MergeHelper->>Legacy: fallback merge by branch
Legacy-->>MergeHelper: result
MergeHelper-->>Router: result
end
Router-->>Client: merge result or TRPC error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts:414">
P2: Do not use an empty catch in the new branch-based PR lookup; log the error context before returning null.
(Based on your team's feedback about avoiding empty catch blocks that hide failures.) [FEEDBACK_USED]</violation>
</file>
<file name="apps/desktop/src/lib/trpc/routers/changes/git-operations.ts">
<violation number="1" location="apps/desktop/src/lib/trpc/routers/changes/git-operations.ts:659">
P2: The broad fallback catch reruns legacy branch merge after explicit merge errors, which masks non-`no pull request` failures and can invoke `gh pr merge` multiple times on the same request.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts (1)
373-417: Consider parallelizing branch candidate lookups.The sequential
for...ofloop overgetPRHeadBranchCandidatesmeans two API calls when the local branch has an owner prefix (e.g.,"kitenite/feature"→ queries for both"kitenite/feature"and"feature").Parallelizing with
Promise.allcould reduce latency:♻️ Optional: Parallelize branch candidate lookups
async function findPRByHeadBranch( worktreePath: string, localBranch: string, repoContext?: RepoContext, headSha?: string, ): Promise<GitHubStatus["pr"]> { try { const matches = new Map<number, GHPRResponse>(); + const candidates = getPRHeadBranchCandidates(localBranch); - for (const branchCandidate of getPRHeadBranchCandidates(localBranch)) { + const results = await Promise.all( + candidates.map(async (branchCandidate) => { + try { - const { stdout } = await execWithShellEnv( + const { stdout } = await execWithShellEnv( - "gh", + "gh", - [ + [ - "pr", + "pr", - "list", + "list", - ...getPullRequestRepoArgs(repoContext), + ...getPullRequestRepoArgs(repoContext), - "--state", + "--state", - "all", + "all", - "--head", + "--head", - branchCandidate, + branchCandidate, - "--limit", + "--limit", - "20", + "20", - "--json", + "--json", - PR_JSON_FIELDS, + PR_JSON_FIELDS, - ], + ], - { cwd: worktreePath }, + { cwd: worktreePath }, - ); + ); + return parsePRListResponse(stdout); + } catch { + return []; + } + }), + ); - for (const candidate of parsePRListResponse(stdout)) { + for (const prList of results) { + for (const candidate of prList) { if (prMatchesLocalBranch(localBranch, candidate)) { matches.set(candidate.number, candidate); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts` around lines 373 - 417, findPRByHeadBranch currently queries gh sequentially for each branch candidate; change it to issue all execWithShellEnv calls in parallel by mapping getPRHeadBranchCandidates(localBranch) to an array of Promises and awaiting Promise.all, then iterate over the completed stdout results to call parsePRListResponse and prMatchesLocalBranch and populate the matches Map; keep existing behavior for getPullRequestRepoArgs, PR_JSON_FIELDS, error handling, and the subsequent sortPRCandidates/formatPRData flow so bestMatch selection is unchanged (refer to function names: findPRByHeadBranch, getPRHeadBranchCandidates, execWithShellEnv, parsePRListResponse, prMatchesLocalBranch, sortPRCandidates, formatPRData).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts`:
- Around line 373-417: findPRByHeadBranch currently queries gh sequentially for
each branch candidate; change it to issue all execWithShellEnv calls in parallel
by mapping getPRHeadBranchCandidates(localBranch) to an array of Promises and
awaiting Promise.all, then iterate over the completed stdout results to call
parsePRListResponse and prMatchesLocalBranch and populate the matches Map; keep
existing behavior for getPullRequestRepoArgs, PR_JSON_FIELDS, error handling,
and the subsequent sortPRCandidates/formatPRData flow so bestMatch selection is
unchanged (refer to function names: findPRByHeadBranch,
getPRHeadBranchCandidates, execWithShellEnv, parsePRListResponse,
prMatchesLocalBranch, sortPRCandidates, formatPRData).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8dc2391c-29f0-4302-8c27-8db46d56840f
📒 Files selected for processing (6)
apps/desktop/src/lib/trpc/routers/changes/git-operations.test.tsapps/desktop/src/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/lib/trpc/routers/changes/git-utils.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts
Kitenite
force-pushed
the
kitenite/pr-detection-issue
branch
from
91045f2 to
a3c4f35
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.ts`:
- Around line 72-98: The code currently falls back to runMerge(legacyMergeArgs)
for any error thrown after attempting an explicit PR merge, undoing the
hardening; fix by tracking whether the PR lookup/explicit-merge attempt actually
succeeded and only allow the legacy fallback when the failure was due to "no
pull request found" or when the PR lookup never succeeded. Concretely: introduce
a boolean flag (e.g., prAttempted) before the inner try around runMerge(args),
set prAttempted = true when getPRForBranch/explicit merge is attempted, and in
the outer catch only call runMerge(legacyMergeArgs) when prAttempted is false or
when isNoPullRequestFoundMessage(error.message) returns true; otherwise rethrow
the error (except for PR_ALREADY_MERGED_MESSAGE/PR_CLOSED_MESSAGE handling which
should remain unchanged). Ensure you reference runMerge,
isNoPullRequestFoundMessage, PR_ALREADY_MERGED_MESSAGE, and PR_CLOSED_MESSAGE
when making the change.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/repo-context.ts`:
- Around line 36-58: When data.isFork is true but data.parent is missing, do not
apply the origin vs ghUrl heuristic or construct a RepoContext; instead return
null to avoid producing a misleading repo/upstream pair. Update the branch
handling around data.isFork/data.parent in repo-context.ts (the block using
data.isFork, data.parent, getOriginUrl, normalizeGitHubUrl and constructing
context) so that if data.isFork && !data.parent the function returns null
immediately; keep the existing fork handling only for data.isFork && data.parent
and preserve the non-fork fallback for when data.isFork is false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 772852b5-d21b-4da1-923a-c71b998cc9c2
📒 Files selected for processing (11)
apps/desktop/src/lib/trpc/routers/changes/git-operations.test.tsapps/desktop/src/lib/trpc/routers/changes/git-operations.tsapps/desktop/src/lib/trpc/routers/changes/git-utils.tsapps/desktop/src/lib/trpc/routers/changes/utils/merge-pull-request.tsapps/desktop/src/lib/trpc/routers/changes/utils/worktree-status-caches.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/index.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/pr-resolution.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/repo-context.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/lib/trpc/routers/changes/git-utils.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts
- apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts
- apps/desktop/src/lib/trpc/routers/changes/git-operations.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts
Comment on lines
+36
to
+58
| if (data.isFork && data.parent) { | ||
| context = { | ||
| repoUrl: data.url, | ||
| upstreamUrl: data.parent.url, | ||
| isFork: true, | ||
| }; | ||
| } else { | ||
| const originUrl = await getOriginUrl(worktreePath); | ||
| const ghUrl = normalizeGitHubUrl(data.url); | ||
|
|
||
| if (originUrl && ghUrl && originUrl !== ghUrl) { | ||
| context = { | ||
| repoUrl: originUrl, | ||
| upstreamUrl: ghUrl, | ||
| isFork: true, | ||
| }; | ||
| } else { | ||
| context = { | ||
| repoUrl: data.url, | ||
| upstreamUrl: data.url, | ||
| isFork: false, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't reuse the origin !== ghUrl heuristic once gh has already told us this is a fork.
When data.isFork is true but parent is missing, ghUrl is still the fork URL. This fallback branch then either marks the repo as a non-fork or swaps repoUrl/upstreamUrl, which makes downstream --repo resolution query the wrong repository. Returning null here is safer than constructing a misleading RepoContext.
🛠️ Suggested guard
if (data.isFork && data.parent) {
context = {
repoUrl: data.url,
upstreamUrl: data.parent.url,
isFork: true,
};
} else {
const originUrl = await getOriginUrl(worktreePath);
const ghUrl = normalizeGitHubUrl(data.url);
+ if (data.isFork) {
+ return null;
+ }
+
if (originUrl && ghUrl && originUrl !== ghUrl) {
context = {
repoUrl: originUrl,
upstreamUrl: ghUrl,
isFork: true,
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/workspaces/utils/github/repo-context.ts`
around lines 36 - 58, When data.isFork is true but data.parent is missing, do
not apply the origin vs ghUrl heuristic or construct a RepoContext; instead
return null to avoid producing a misleading repo/upstream pair. Update the
branch handling around data.isFork/data.parent in repo-context.ts (the block
using data.isFork, data.parent, getOriginUrl, normalizeGitHubUrl and
constructing context) so that if data.isFork && !data.parent the function
returns null immediately; keep the existing fork handling only for data.isFork
&& data.parent and preserve the non-fork fallback for when data.isFork is false.
…itenite/pr-detection-issue # Conflicts: # apps/desktop/src/lib/trpc/routers/changes/git-operations.test.ts # apps/desktop/src/lib/trpc/routers/changes/git-operations.ts # apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.ts # apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts # apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts
This was referenced Mar 26, 2026
7 tasks
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
1 participant
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.
Summary
gh pr mergeto infer the current branchRepro
/Users/kietho/.superset/worktrees/superset/kitenite/update-status-after-squashgh pr viewreported no PR for the branch, while explicit branch/SHA lookup still found PR#2665Testing
bun test apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.ts apps/desktop/src/lib/trpc/routers/changes/git-operations.test.tsbunx biome check apps/desktop/src/lib/trpc/routers/workspaces/utils/github/types.ts apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.ts apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.ts apps/desktop/src/lib/trpc/routers/changes/git-utils.ts apps/desktop/src/lib/trpc/routers/changes/git-operations.ts apps/desktop/src/lib/trpc/routers/changes/git-operations.test.tsbun run --cwd apps/desktop typecheckSummary by cubic
Hardened PR merge detection in Desktop by resolving the current PR deterministically and merging by PR number. Centralized the merge flow with safe fallbacks, better fork matching, and clearer, user-facing errors.
Bug Fixes
--repowhen known; fall back to legacy branch merge if resolution fails orghreports “no pull request” (case-insensitive); surface “already merged/closed” as user errors.Refactors
merge-pull-request.tsandworktree-status-caches.ts; extracted PR matching topr-resolution.tsand repo/URL helpers torepo-context.ts.workspaces/utils/github/index.tsand trimmedgithub.tsto status/deploy logic.Written for commit 0405d15. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests