Fix PR state flicker: make confirmed-no-PR clearing effective and order-independent - #538
Merged
Merged
Conversation
The sidebar PR badge flickers (disappears briefly then reappears) on every refresh cycle because the PR data dictionary only contains branches with a matching PR. Branches without a PR are absent from the dictionary, but the downstream mapping treats 'key absent' the same as 'no PR', clearing stale PR state for worktrees that weren't actually queried. Change the entire PR refresh data pipeline from [String: GithubPullRequest] to [String: GithubPullRequest?] so the three cases are distinguished: - key present with PR → update - key present with nil → confirmed no PR, clear old value - key absent → not queried this cycle, preserve old value Also add race protection so a nil result from one host does not overwrite a PR already merged from another host. Signed-off-by: Alex <alex.tech.lab@outlook.com>
Update test assertions to handle [String: GithubPullRequest?] instead of [String: GithubPullRequest]: - Use (dict[key] ?? nil)?.property to unwrap double optional - Use (dict[key] ?? nil) == nil to check for nil value - Remove .repositoryPullRequestsLoaded receives when remoteInfos is empty (new behavior preserves existing PR state instead of clearing) Signed-off-by: Alex <alex.tech.lab@outlook.com>
Revert the [String: GithubPullRequest?] approach (Swift dict[key]=nil deletes the key, not stores .some(nil)) and implement tri-state using a separate Set<String> for "confirmed no PR" branches. Three-way distinction: - prsByBranch contains branches with a PR -> update - confirmedNoPrBranches contains branches all repos confirmed as no PR -> clear - neither -> unknown (partial failure) -> preserve existing state Key changes: - Outcome.refreshed gains confirmedNoPrBranches: Set<String> - emitOutcomes computes it only when ALL candidate repos succeeded - pullRequestsByWorktreeID only clears worktrees in confirmedNoPrBranches - mergePullRequestRefreshResults accumulates Set without overwriting existing PRs - Keep no-remote path fix (remoteInfos.isEmpty no longer clears PRs) - Rename refreshClearsStalePullRequestsWhenGithubRemotesDisappear to refreshPreservesPullRequestsWhenGithubRemotesUnavailable Signed-off-by: Alex <alex.tech.lab@outlook.com>
Signed-off-by: Alex <alex.tech.lab@outlook.com>
Signed-off-by: Alex <alex.tech.lab@outlook.com>
- pullRequestsByWorktreeID assigned a nil literal through the optional-value dictionary subscript, which removes the key instead of storing an explicit nil. The confirmed-no-PR clear never reached the reducer, so stale PR badges were never cleared and the tri-state mechanism was a no-op. Use updateValue(nil, forKey:) instead. - A failed host batch arriving before the final refreshed outcome left the accumulated confirmed-no-PR set intact, so a healthy host could clear a PR that lives on the failed host. Track failed batches per repository and suppress confirmed clears when any batch failed, regardless of outcome arrival order. - Add reducer tests for explicit clear, unknown-status preserve, both failed/refreshed arrival orderings, and a later host batch overriding an earlier confirmation; add coordinator tests covering confirmedNoPrBranches computation (all-candidates-succeeded vs partial candidate failure).
There was a problem hiding this comment.
Pull request overview
This PR fixes PR badge “flicker” and stale-state issues in the Repositories feature by making the tri-state “confirmed no PR” semantics actually take effect, and by ensuring cross-host failures suppress clears regardless of batch arrival order.
Changes:
- Make “confirmed no PR” clears effective by emitting explicit nil values for worktrees (
updateValue(nil, forKey:)) and threadingconfirmedNoPrBranchesthrough the refresh pipeline. - Make confirmed clears order-independent across multiple GitHub hosts by tracking per-repository failed batches and suppressing clears whenever any host batch failed.
- Add missing semantic test coverage for non-empty
confirmedNoPrBranchesand the failure/refreshed ordering cases.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift | Implements effective explicit clears and order-independent suppression when any host batch fails. |
| supacode/Features/Repositories/Reducer/RepositoriesFeature.swift | Adds per-repository tracking for confirmed-no-PR branches and failed batches. |
| supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift | Computes confirmedNoPrBranches only when all candidate repos succeed, emitting tri-state outcomes. |
| supacodeTests/BatchedPullRequestRefreshReducerTests.swift | Adds targeted reducer tests covering clears, unknown-state preservation, ordering, and override behavior. |
| supacodeTests/PullRequestRefreshCoordinatorTests.swift | Adds coordinator tests ensuring confirmed-no-PR is only produced on full success and suppressed on partial failure. |
| supacodeTests/RepositoriesFeatureTests.swift | Updates expectations for the “no GitHub remotes” path (no longer emits a clearing payload). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Supersedes #533. Includes the original tri-state work by @Alex-ai-future (commits kept as-is) plus two fixes and the missing semantic test coverage.
Background
#533 introduced Set-based tri-state semantics (
confirmedNoPrBranches) to distinguish "queried, confirmed no PR" from "status unknown (fetch failed)", so partial refresh failures no longer clear PR badges and cause flicker. The design is sound, but the implementation had two gaps.Fixes
1. Confirmed-no-PR clearing was a no-op (dead code).
pullRequestsByWorktreeIDstored the explicit clear via a nil literal:On a dictionary with optional values, assigning a nil literal through the subscript removes the key instead of storing
.some(nil). Since the downstreamrepositoryPullRequestsLoadedhandler only iterates keys present in the dictionary, the clear never happened: stale badges (PR deleted, or worktree switched to a branch without a PR) persisted forever, and the entire tri-state mechanism had zero runtime effect. Fixed withupdateValue(nil, forKey:).2. Cross-host suppression of confirmed clears was arrival-order dependent.
With multiple GitHub hosts, a
.failedbatch arriving before the final.refreshedoutcome left the accumulated confirmed set intact, so a healthy host could clear a PR that lives on the failed host — the exact flicker this PR set out to fix, surviving as a coin flip. Only the failed-batch-arrives-last ordering discarded the set. Now failed batches are tracked per repository (prRefreshFailedBatchRepositoryIDs) and confirmed clears are suppressed whenever any batch failed, regardless of order.Test coverage
All #533 test call sites passed
confirmedNoPrBranches: [], so the non-empty path — the feature's whole point — was untested, which is why the no-op was invisible. Added:confirmedNoPrBranchesis populated only when all candidate repos succeed; a partial candidate failure (fallback failing too) leaves branches unconfirmed.Verification
make checkcleanBatchedPullRequestRefreshReducerTests+PullRequestRefreshCoordinatorTests+RepositoriesFeatureTests: 256 tests passingmake build-appsucceeds