ignore stale merged/closed PRs for reused branch names#49
Open
skarim wants to merge 2 commits intoskarim/preflight-stacksfrom
Open
ignore stale merged/closed PRs for reused branch names#49skarim wants to merge 2 commits intoskarim/preflight-stacksfrom
skarim wants to merge 2 commits intoskarim/preflight-stacksfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens PR metadata discovery so reused branch names don’t accidentally pick up stale merged/closed PRs, and it updates sync logic to prefer authoritative sources (remote stack API when available, otherwise OPEN-only branch discovery).
Changes:
- Update TUI branch node loading to only adopt PR details when the PR is tracked for that branch or is currently OPEN.
- Rework
syncStackPRsto (a) use the stack API as source of truth whenStack.IDis set and (b) otherwise refresh tracked PRs by number and only adopt OPEN PRs by branch name. - Add/adjust tests across TUI + cmd packages and refine GitHub client behavior for “PR not found”.
Show a summary per file
| File | Description |
|---|---|
| internal/tui/stackview/data.go | Filter adopted PR details to avoid showing stale merged/closed PRs for reused branch names. |
| internal/tui/stackview/data_test.go | Add coverage for ignoring stale merged PR details and still showing tracked merged + OPEN PRs. |
| internal/github/github.go | Make FindPRByNumber return nil for “not found” results. |
| cmd/utils.go | Rework PR sync: remote stack API path + local OPEN-only adoption path. |
| cmd/utils_test.go | Add extensive tests for new PR sync behavior (tracked/untracked, merged/closed, remote stack). |
| cmd/view_test.go | Update mocks to use PR-by-number lookups consistent with new sync behavior. |
| cmd/submit_test.go | Update mocks/expectations for new sync behavior; add a missing pipe close. |
| cmd/merge_test.go | Ensure merge tests provide a GitHub client override since merge now always syncs PR state. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 1
759b8d0 to
89d0a64
Compare
89d0a64 to
db8f91b
Compare
9c58a54 to
1537762
Compare
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.
Ignore stale merged/closed PRs for reused branch names
When a branch name previously used in a merged or closed PR is reused in a new stack,
syncStackPRswould adopt the old PR and incorrectly mark the branch as "MERGED". This causedgh stack viewto show the wrong status,gh stack submitto skip PR creation, andgh stack pushto skip the branch entirely.Root cause:
syncStackPRscalledFindAnyPRForBranch(returns the most recent PR regardless of state) for every branch, including ones with no locally tracked PR. A reused branch name would pick up the old merged/closed PR from a previous stack.Fix: Rewrite
syncStackPRswith two sync modes:1. Remote stack exists (
s.IDset) — stack API is source of truth:ListStacks, looks up each PR by number, and matches to local branches by head ref name.2. No remote stack — local branch-name discovery:
FindPRByNumber. If the PR was closed (not merged), clear the association and fall through to open-PR discovery.FindPRForBranch(OPEN state only), preventing adoption of stale merged/closed PRs.Additional fixes:
data.go):FindPRDetailsForBranchalso returned stale merged PRs vialast: 1. Added a filter so PR details are only shown if the PR matches the tracked number or is OPEN.FindPRByNumber(github.go): Now returnsnilfor zero-value GraphQL responses.GitHubClientOverrideand silently hitting the real API. Fixed with proper mocks. Updated mock functions fromFindAnyPRForBranchFntoFindPRByNumberFnwhere branches have tracked PRs.Stack created with GitHub Stacks CLI • Give Feedback 💬