Repository navigation
Fix sidebar PR badge detection for workspace branches - #1896
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR enhances GitHub PR probing in TabManager by introducing multi-repository support, implementing a new PR selection algorithm prioritized by status and recency, and improving error handling. The changes replace the single-repo slug model with a priority-ordered multi-slug system and switch from Changes
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Greptile SummaryThis PR fixes sidebar PR badge detection for workspace branches by switching from Key changes:
Issues found:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["workspacePullRequestSnapshot(dir, branch)"] --> B["githubRepositorySlugs(dir)\ngit remote -v"]
B --> C{Any slugs?}
C -- No --> D[.unsupportedRepository]
C -- Yes --> E["Loop over repoSlugs\n(upstream → origin → others)"]
E --> F["workspacePullRequestSnapshot\n(dir, branch, repoSlug)\ngh pr list --repo R --state all --head B"]
F --> G{exit code 0?}
G -- No --> H[".transientFailure\nsawTransientFailure = true"]
G -- Yes --> I["decodeJSON\n[GitHubPullRequestProbeItem]"]
I --> J{Parse OK?}
J -- No --> H
J -- Yes --> K["preferredPullRequest(from: items)\nrank: OPEN > MERGED > CLOSED\nthen updatedAt desc\nthen number desc"]
K --> L{Found?}
L -- No --> M[".notFound → continue to next slug"]
L -- Yes --> N["validate state + URL"]
N --> O{Valid?}
O -- No --> H
O -- Yes --> P[".resolved(SidebarPullRequestState)"]
P --> Q[Return immediately]
M --> E
H --> E
E -- All slugs exhausted --> R{sawTransientFailure?}
R -- Yes --> S[.transientFailure]
R -- No --> T[.notFound]
Last reviewed commit: "fix: detect sidebar ..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func isPreferred( | ||
| candidate: GitHubPullRequestProbeItem, | ||
| over current: GitHubPullRequestProbeItem | ||
| ) -> Bool { | ||
| guard let candidateStatus = pullRequestStatus(from: candidate.state), | ||
| let currentStatus = pullRequestStatus(from: current.state) else { | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
The guard let candidateStatus / currentStatus inside isPreferred can never fail in practice. The outer loop in preferredPullRequest already validates every candidate with:
guard pullRequestStatus(from: pullRequest.state) != nil,
URL(string: pullRequest.url) != nil else { continue }before calling isPreferred. And currentBest was always first accepted via the same guard, so pullRequestStatus(from: current.state) is always non-nil too.
The return false branch is unreachable dead code. If it ever fires due to a future refactor, silently keeping the current best is a subtle outcome that could be hard to debug. Consider removing the guard entirely (since callers pre-validate), or add a assertionFailure / dlog to make the unexpected case explicit.
| @@ -1486,6 +1499,16 @@ class TabManager: ObservableObject { | |||
| return .transientFailure | |||
There was a problem hiding this comment.
parseFail log when stdout is nil
output is set to "" when result.stdout is nil. decodeJSON will fail to parse "" as [GitHubPullRequestProbeItem] (not valid JSON), so this guard trips and logs parseFail. But the real cause is that stdout was absent, not a malformed JSON payload. During debugging, a parseFail entry that has output=none in the log can be indistinguishable from a genuine parse error in a non-empty payload.
Consider differentiating the two cases:
let output = result.stdout ?? ""
guard !output.isEmpty else {
#if DEBUG
dlog(
"workspace.gitProbe.pr.noOutput dir=\(directory) branch=\(branch) " +
"repo=\(repoSlug)"
)
#endif
return .transientFailure
}
guard let pullRequests = decodeJSON([GitHubPullRequestProbeItem].self, from: output) else {
#if DEBUG
dlog(
"workspace.gitProbe.pr.parseFail dir=\(directory) branch=\(branch) " +
"repo=\(repoSlug) output=\(debugLogSnippet(output) ?? "none")"
)
#endif
return .transientFailure
}| func testPreferredPullRequestIgnoresMalformedCandidates() { | ||
| let valid = TabManager.GitHubPullRequestProbeItem( | ||
| number: 1888, | ||
| state: "OPEN", | ||
| url: "https://github.com/manaflow-ai/cmux/pull/1888", | ||
| updatedAt: "2026-03-20T18:00:00Z" | ||
| ) | ||
|
|
||
| XCTAssertEqual( | ||
| TabManager.preferredPullRequest(from: [ | ||
| TabManager.GitHubPullRequestProbeItem( | ||
| number: 9999, | ||
| state: "WHATEVER", | ||
| url: "https://github.com/manaflow-ai/cmux/pull/9999", | ||
| updatedAt: "2026-03-21T18:00:00Z" | ||
| ), | ||
| TabManager.GitHubPullRequestProbeItem( | ||
| number: 10000, | ||
| state: "OPEN", | ||
| url: "not a url", | ||
| updatedAt: "2026-03-21T18:00:00Z" | ||
| ), | ||
| valid, | ||
| ]), | ||
| valid | ||
| ) | ||
| } |
There was a problem hiding this comment.
nil
testPreferredPullRequestIgnoresMalformedCandidates always includes at least one valid candidate. There is no test verifying that preferredPullRequest(from:) returns nil when every entry in the array is malformed (bad state + bad URL), and no test for the empty-input case preferredPullRequest(from: []).
Both are straightforward edge cases worth covering:
func testPreferredPullRequestReturnsNilForEmptyList() {
XCTAssertNil(TabManager.preferredPullRequest(from: []))
}
func testPreferredPullRequestReturnsNilWhenAllCandidatesMalformed() {
XCTAssertNil(
TabManager.preferredPullRequest(from: [
TabManager.GitHubPullRequestProbeItem(
number: 1,
state: "UNKNOWN",
url: "not-a-url",
updatedAt: nil
),
])
)
}![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 (1)
Sources/TabManager.swift (1)
1437-1510:⚠️ Potential issue | 🟠 MajorPR badges still stop updating after the initial probe window.
This new lookup still feeds the existing finite retry schedule, and that schedule clears on both
.notFoundand.resolved. If the PR is opened after the last retry, or an already-open PR is merged later, the sidebar badge stays stale until the branch/directory changes. Please keep a low-frequency re-probe alive while the workspace remains on the same branch.Also applies to: 1533-1541
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 1437 - 1510, The probe currently returns .notFound or .resolved which clears the finite retry schedule; change the behavior so the workspace keeps a low-frequency re-probe while on the same branch: either introduce a new enum case (e.g. .notFoundKeepProbing or .keepProbing) or map the existing .notFound/.resolved returns to a non-terminal status (e.g. .transientFailure) so the retry schedule is not cleared; update the probe return points in the GitHub PR lookup function (the block that calls runCommandResult, decodes via decodeJSON([GitHubPullRequestProbeItem].self, from:), and uses preferredPullRequest(from:)) and the equivalent logic around lines 1533-1541 to return the non-terminal status instead of .notFound/.resolved so a low-frequency background re-probe continues while the workspace remains on the same branch.
🧹 Nitpick comments (1)
cmuxTests/TabManagerUnitTests.swift (1)
120-213: Add one refresh-path regression.These helper tests will still all pass if
TabManagernever re-probes after the initial window, so the user-visible “badge updates automatically” behavior is still uncovered. A single test that drives the actual refresh scheduling/state transition would make this fix much harder to regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TabManagerUnitTests.swift` around lines 120 - 213, Add a regression test that verifies TabManager actually schedules and executes a subsequent refresh (not just the initial probe): write a new test in TabManagerUnitTests.swift that creates a TabManager with a test double/mock for the network client or probe handler (or inject a mock scheduler) that counts probe invocations, call the public method that starts probing (e.g., TabManager.startProbing() or the initializer that begins the probe cycle), advance the test scheduler or run the loop to simulate the refresh interval (or call TabManager.scheduleNextProbe()/TabManager.triggerScheduledProbe() if those exist), and assert the probe was invoked at least twice to ensure the refresh path runs; reference TabManager, the probing/start method, and the probe/network client mock when locating where to inject the test double.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TabManager.swift`:
- Around line 1786-1800: The loop that parses git remote lines only accepts
remotes where remoteKind == "(fetch)", so push-only GitHub remotes are ignored;
update the guard in the parsing block (the loop that builds slugByRemoteName) to
accept push remotes as well by allowing remoteKind == "(push)" in addition to
"(fetch)" (or accept both by testing for contains "fetch" or "push"), and ensure
you still call githubRepositorySlug(fromRemoteURL:) and populate
slugByRemoteName[remoteName] when a push remote yields a valid repoSlug.
---
Outside diff comments:
In `@Sources/TabManager.swift`:
- Around line 1437-1510: The probe currently returns .notFound or .resolved
which clears the finite retry schedule; change the behavior so the workspace
keeps a low-frequency re-probe while on the same branch: either introduce a new
enum case (e.g. .notFoundKeepProbing or .keepProbing) or map the existing
.notFound/.resolved returns to a non-terminal status (e.g. .transientFailure) so
the retry schedule is not cleared; update the probe return points in the GitHub
PR lookup function (the block that calls runCommandResult, decodes via
decodeJSON([GitHubPullRequestProbeItem].self, from:), and uses
preferredPullRequest(from:)) and the equivalent logic around lines 1533-1541 to
return the non-terminal status instead of .notFound/.resolved so a low-frequency
background re-probe continues while the workspace remains on the same branch.
---
Nitpick comments:
In `@cmuxTests/TabManagerUnitTests.swift`:
- Around line 120-213: Add a regression test that verifies TabManager actually
schedules and executes a subsequent refresh (not just the initial probe): write
a new test in TabManagerUnitTests.swift that creates a TabManager with a test
double/mock for the network client or probe handler (or inject a mock scheduler)
that counts probe invocations, call the public method that starts probing (e.g.,
TabManager.startProbing() or the initializer that begins the probe cycle),
advance the test scheduler or run the loop to simulate the refresh interval (or
call TabManager.scheduleNextProbe()/TabManager.triggerScheduledProbe() if those
exist), and assert the probe was invoked at least twice to ensure the refresh
path runs; reference TabManager, the probing/start method, and the probe/network
client mock when locating where to inject the test double.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbb20c90-2f3f-4d3c-b883-8e070fdc3d6a
📒 Files selected for processing (2)
Sources/TabManager.swiftcmuxTests/TabManagerUnitTests.swift
| for line in output.split(whereSeparator: \.isNewline) { | ||
| let parts = line.split(whereSeparator: \.isWhitespace) | ||
| guard parts.count >= 3 else { continue } | ||
|
|
||
| let remoteName = String(parts[0]) | ||
| let remoteURL = String(parts[1]) | ||
| let remoteKind = String(parts[2]) | ||
| guard remoteKind == "(fetch)", | ||
| let repoSlug = githubRepositorySlug(fromRemoteURL: remoteURL) else { | ||
| continue | ||
| } | ||
|
|
||
| if slugByRemoteName[remoteName] == nil { | ||
| slugByRemoteName[remoteName] = repoSlug | ||
| } |
There was a problem hiding this comment.
Accept GitHub push remotes here too.
This parser ignores "(push)" entries entirely. Repos that fetch from a mirror/non-GitHub remote but push to GitHub will still produce no slug here, so PR badges remain broken for that workspace configuration.
Possible fix
for line in output.split(whereSeparator: \.isNewline) {
let parts = line.split(whereSeparator: \.isWhitespace)
guard parts.count >= 3 else { continue }
let remoteName = String(parts[0])
let remoteURL = String(parts[1])
let remoteKind = String(parts[2])
- guard remoteKind == "(fetch)",
+ guard remoteKind == "(fetch)" || remoteKind == "(push)",
let repoSlug = githubRepositorySlug(fromRemoteURL: remoteURL) else {
continue
}
- if slugByRemoteName[remoteName] == nil {
+ if slugByRemoteName[remoteName] == nil || remoteKind == "(fetch)" {
slugByRemoteName[remoteName] = repoSlug
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for line in output.split(whereSeparator: \.isNewline) { | |
| let parts = line.split(whereSeparator: \.isWhitespace) | |
| guard parts.count >= 3 else { continue } | |
| let remoteName = String(parts[0]) | |
| let remoteURL = String(parts[1]) | |
| let remoteKind = String(parts[2]) | |
| guard remoteKind == "(fetch)", | |
| let repoSlug = githubRepositorySlug(fromRemoteURL: remoteURL) else { | |
| continue | |
| } | |
| if slugByRemoteName[remoteName] == nil { | |
| slugByRemoteName[remoteName] = repoSlug | |
| } | |
| for line in output.split(whereSeparator: \.isNewline) { | |
| let parts = line.split(whereSeparator: \.isWhitespace) | |
| guard parts.count >= 3 else { continue } | |
| let remoteName = String(parts[0]) | |
| let remoteURL = String(parts[1]) | |
| let remoteKind = String(parts[2]) | |
| guard remoteKind == "(fetch)" || remoteKind == "(push)", | |
| let repoSlug = githubRepositorySlug(fromRemoteURL: remoteURL) else { | |
| continue | |
| } | |
| if slugByRemoteName[remoteName] == nil || remoteKind == "(fetch)" { | |
| slugByRemoteName[remoteName] = repoSlug | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 1786 - 1800, The loop that parses git
remote lines only accepts remotes where remoteKind == "(fetch)", so push-only
GitHub remotes are ignored; update the guard in the parsing block (the loop that
builds slugByRemoteName) to accept push remotes as well by allowing remoteKind
== "(push)" in addition to "(fetch)" (or accept both by testing for contains
"fetch" or "push"), and ensure you still call
githubRepositorySlug(fromRemoteURL:) and populate slugByRemoteName[remoteName]
when a push remote yields a valid repoSlug.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 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="Sources/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:1444">
P2: Add an explicit `--limit` to the `gh pr list` probe. The CLI defaults to 30 results, so the current query can miss the relevant PR and produce the wrong sidebar badge when many PRs match the head branch.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "--head", branch, | ||
| "--json", "number,state,url,updatedAt", |
There was a problem hiding this comment.
P2: Add an explicit --limit to the gh pr list probe. The CLI defaults to 30 results, so the current query can miss the relevant PR and produce the wrong sidebar badge when many PRs match the head branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TabManager.swift, line 1444:
<comment>Add an explicit `--limit` to the `gh pr list` probe. The CLI defaults to 30 results, so the current query can miss the relevant PR and produce the wrong sidebar badge when many PRs match the head branch.</comment>
<file context>
@@ -1408,17 +1409,40 @@ class TabManager: ObservableObject {
"--repo", repoSlug,
- "--json", "number,state,url",
+ "--state", "all",
+ "--head", branch,
+ "--json", "number,state,url,updatedAt",
],
</file context>
| "--head", branch, | |
| "--json", "number,state,url,updatedAt", | |
| "--head", branch, | |
| "--limit", "200", | |
| "--json", "number,state,url,updatedAt", |
* test: cover sidebar PR probe selection * fix: detect sidebar PR badges across github remotes
This branch was successfully deployed
1 participant
Summary
gh pr list --state all --head <branch>instead ofgh pr view, which misses cross-repo headsTesting
./scripts/reload.sh --tag fix-1893-pr-badgeFixes #1893
Summary by cubic
Fixes #1893 by reliably detecting sidebar PR badges for workspace branches across multiple GitHub remotes and cross-repo heads. We now probe PRs by branch and prioritize
upstreamoveroriginfor better accuracy.gh pr list --state all --head <branch>instead ofgh pr viewto include cross-repo heads.git remote -v, prioritizeupstreamthenorigin, and dedupe slugs before probing.updatedAt, then highest number.ghexits are transient; no PRs return not found; removed fragile stderr matching.Written for commit a8cfa3b. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests