fix(desktop): avoid stale historical PR matches#2906
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis change enhances PR matching logic to consider HEAD SHA when validating pull request candidates. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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.test.ts (1)
411-456: Consider adding test coverage forCLOSEDstate andundefinedheadSha.The test suite covers the core scenarios well. Two optional additions would strengthen coverage:
- A test for
state: "CLOSED"to explicitly verify it behaves identically toMERGEDfor historical PR rejection.- A test where
headShaisundefinedto verify the fallback path (should accept any branch-matching PR regardless of state).💡 Suggested additional test cases
test("accepts historical PR matches when the head commit still matches", () => { expect( shouldAcceptPRMatch({ localBranch: "feature/my-thing", headSha: "same-head-sha", pr: { headRefName: "feature/my-thing", headRefOid: "same-head-sha", headRepositoryOwner: null, state: "MERGED", }, }), ).toBe(true); }); + + test("rejects closed PR matches when the head commit differs", () => { + expect( + shouldAcceptPRMatch({ + localBranch: "feature/my-thing", + headSha: "new-head-sha", + pr: { + headRefName: "feature/my-thing", + headRefOid: "old-pr-head-sha", + headRepositoryOwner: null, + state: "CLOSED", + }, + }), + ).toBe(false); + }); + + test("accepts historical PR when headSha is not provided", () => { + expect( + shouldAcceptPRMatch({ + localBranch: "feature/my-thing", + headSha: undefined, + pr: { + headRefName: "feature/my-thing", + headRefOid: "any-sha", + headRepositoryOwner: null, + state: "MERGED", + }, + }), + ).toBe(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/github.test.ts` around lines 411 - 456, Add two unit tests for shouldAcceptPRMatch: one that verifies PRs with state "CLOSED" are treated like "MERGED" by copying the existing "rejects historical PR matches when the head commit differs" case but using state: "CLOSED" and expecting toBe(false), and another that verifies when local headSha is undefined the function accepts branch-matching PRs regardless of PR state by adding a case with headSha: undefined, a matching headRefName, a different pr.headRefOid and state: "MERGED" (or "CLOSED") and expecting toBe(true); place both tests inside the existing describe("shouldAcceptPRMatch") block near the other cases so they follow the same pattern and naming style.
🤖 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.test.ts`:
- Around line 411-456: Add two unit tests for shouldAcceptPRMatch: one that
verifies PRs with state "CLOSED" are treated like "MERGED" by copying the
existing "rejects historical PR matches when the head commit differs" case but
using state: "CLOSED" and expecting toBe(false), and another that verifies when
local headSha is undefined the function accepts branch-matching PRs regardless
of PR state by adding a case with headSha: undefined, a matching headRefName, a
different pr.headRefOid and state: "MERGED" (or "CLOSED") and expecting
toBe(true); place both tests inside the existing describe("shouldAcceptPRMatch")
block near the other cases so they follow the same pattern and naming style.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6b84bd35-60ee-4ad3-9c03-20c07dd58a09
📒 Files selected for processing (2)
apps/desktop/src/lib/trpc/routers/workspaces/utils/github/github.test.tsapps/desktop/src/lib/trpc/routers/workspaces/utils/github/pr-resolution.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
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
Verification
Task: ab8cd32a-0770-4add-9611-eaab15df7692
Summary by cubic
Stops stale PRs from auto-attaching in Desktop when a branch name is reused by only accepting closed/merged PRs if the workspace HEAD matches the PR head commit. Open PR matching stays the same.
Written for commit ec8d470. Summary will update on new commits.
Summary by CodeRabbit
Tests
Bug Fixes