From fe74f156769138e31f5a870dd9cd5d503307bf45 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 3 Jul 2026 10:27:55 +0800 Subject: [PATCH 1/6] Fix PR state flicker by distinguishing 'no PR' from 'not queried' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- supacode/Clients/Github/GithubCLIClient.swift | 37 +++++++++---------- supacode/Clients/Github/GithubCLIModels.swift | 4 +- .../GithubGraphQLPullRequestResponse.swift | 16 ++++---- .../PullRequestRefreshCoordinator.swift | 21 ++++++----- ...epositoriesFeature+GithubIntegration.swift | 34 ++++++++--------- ...epositoriesFeature+WorkspaceChildren.swift | 2 +- .../Reducer/RepositoriesFeature.swift | 2 +- 7 files changed, 59 insertions(+), 57 deletions(-) diff --git a/supacode/Clients/Github/GithubCLIClient.swift b/supacode/Clients/Github/GithubCLIClient.swift index 94f4dd5da..972d61c4f 100644 --- a/supacode/Clients/Github/GithubCLIClient.swift +++ b/supacode/Clients/Github/GithubCLIClient.swift @@ -177,7 +177,7 @@ struct GithubCLIClient: Sendable { var resolveRemoteInfo: @Sendable (URL) async -> GithubRemoteInfo? var latestRun: @Sendable (URL, String, GithubAccountOverride?) async throws -> GithubWorkflowRun? var batchPullRequests: - @Sendable (String, String, String, [String], GithubAccountOverride?) async throws -> [String: GithubPullRequest] + @Sendable (String, String, String, [String], GithubAccountOverride?) async throws -> [String: GithubPullRequest?] var batchPullRequestsAcrossRepositories: @Sendable (String, [CrossRepoPullRequestRequest], GithubAccountOverride?) async throws -> CrossRepoPullRequestResult var mergePullRequest: @@ -330,7 +330,7 @@ nonisolated private func latestRunFetcher( nonisolated private func batchPullRequestsFetcher( shell: ShellClient, resolver: GithubCLIExecutableResolver -) -> @Sendable (String, String, String, [String], GithubAccountOverride?) async throws -> [String: GithubPullRequest] { +) -> @Sendable (String, String, String, [String], GithubAccountOverride?) async throws -> [String: GithubPullRequest?] { { host, owner, repo, branches, accountOverride in try await withExpectedGithubAccount( shell: shell, @@ -365,7 +365,7 @@ nonisolated private let crossRepoBatchAliasLimit = 15 nonisolated private let crossRepoBatchMaxConcurrentRequests = 3 nonisolated private struct CrossRepoChunkOutcome: Sendable { - let successByRepo: [RepoKey: [String: GithubPullRequest]] + let successByRepo: [RepoKey: [String: GithubPullRequest?]] let failedRepos: [RepoKey: GithubCLIError] } @@ -492,7 +492,7 @@ nonisolated private func loadCrossRepoChunks( nonisolated private func mergeCrossRepoChunkResults( _ outcomes: [CrossRepoChunkOutcome] ) -> CrossRepoPullRequestResult { - var success: [RepoKey: [String: GithubPullRequest]] = [:] + var success: [RepoKey: [String: GithubPullRequest?]] = [:] var failed: [RepoKey: GithubCLIError] = [:] for outcome in outcomes { for (key, prs) in outcome.successByRepo { @@ -537,7 +537,7 @@ nonisolated private func fetchCrossRepoChunk( let response = try GithubCLIOutput.decode(CrossRepoPullRequestResponse.self, from: output, decoder: decoder) let errorMessagesByAlias = response.errorMessagesByAlias() - var success: [RepoKey: [String: GithubPullRequest]] = [:] + var success: [RepoKey: [String: GithubPullRequest?]] = [:] var failed: [RepoKey: GithubCLIError] = [:] for (alias, key) in plan.repoAliases { if let detail = errorMessagesByAlias[alias] { @@ -669,15 +669,14 @@ nonisolated private func rankCrossRepoPullRequests( pullRequestsByAlias: [String: GithubGraphQLPullRequestResponse.PullRequestConnection], aliasMap: [String: String], allowedHeadRepositories: Set -) -> [String: GithubPullRequest] { - var results: [String: GithubPullRequest] = [:] - for (alias, connection) in pullRequestsByAlias { - guard let branch = aliasMap[alias] else { +) -> [String: GithubPullRequest?] { + var results: [String: GithubPullRequest?] = [:] + for (alias, branch) in aliasMap { + guard let connection = pullRequestsByAlias[alias] else { + results[branch] = nil continue } - if let node = connection.bestMatchingPullRequest(allowedHeadRepositories: allowedHeadRepositories) { - results[branch] = node.pullRequest - } + results[branch] = connection.bestMatchingPullRequest(allowedHeadRepositories: allowedHeadRepositories)?.pullRequest } return results } @@ -1055,9 +1054,9 @@ nonisolated private func loadPullRequestChunks( resolver: GithubCLIExecutableResolver, request: GithubPullRequestsRequest, chunks: [[String]] -) async throws -> [Int: [String: GithubPullRequest]] { +) async throws -> [Int: [String: GithubPullRequest?]] { try await withThrowingTaskGroup( - of: (Int, [String: GithubPullRequest]).self + of: (Int, [String: GithubPullRequest?]).self ) { group in var nextChunkIndex = 0 let initialCount = min(batchPullRequestsMaxConcurrentRequests, chunks.count) @@ -1076,7 +1075,7 @@ nonisolated private func loadPullRequestChunks( nextChunkIndex += 1 } - var resultsByChunkIndex: [Int: [String: GithubPullRequest]] = [:] + var resultsByChunkIndex: [Int: [String: GithubPullRequest?]] = [:] while let (chunkIndex, prsByBranch) = try await group.next() { resultsByChunkIndex[chunkIndex] = prsByBranch if nextChunkIndex < chunks.count { @@ -1100,10 +1099,10 @@ nonisolated private func loadPullRequestChunks( } nonisolated private func mergePullRequestChunkResults( - _ chunkResults: [Int: [String: GithubPullRequest]], + _ chunkResults: [Int: [String: GithubPullRequest?]], chunkCount: Int -) -> [String: GithubPullRequest] { - var results: [String: GithubPullRequest] = [:] +) -> [String: GithubPullRequest?] { + var results: [String: GithubPullRequest?] = [:] for chunkIndex in 0.. (Int, [String: GithubPullRequest]) { +) async throws -> (Int, [String: GithubPullRequest?]) { let (query, aliasMap) = makeBatchPullRequestsQuery(branches: chunk) let output = try await runGh( shell: shell, diff --git a/supacode/Clients/Github/GithubCLIModels.swift b/supacode/Clients/Github/GithubCLIModels.swift index 2e8a122da..37e198e0b 100644 --- a/supacode/Clients/Github/GithubCLIModels.swift +++ b/supacode/Clients/Github/GithubCLIModels.swift @@ -167,11 +167,11 @@ nonisolated struct CrossRepoPullRequestRequest: Sendable, Hashable { } nonisolated struct CrossRepoPullRequestResult: Sendable { - let successByRepo: [RepoKey: [String: GithubPullRequest]] + let successByRepo: [RepoKey: [String: GithubPullRequest?]] let failedRepos: [RepoKey: GithubCLIError] init( - successByRepo: [RepoKey: [String: GithubPullRequest]] = [:], + successByRepo: [RepoKey: [String: GithubPullRequest?]] = [:], failedRepos: [RepoKey: GithubCLIError] = [:] ) { self.successByRepo = successByRepo diff --git a/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift b/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift index 4e6ceb0ae..41651d8dc 100644 --- a/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift +++ b/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift @@ -7,15 +7,17 @@ nonisolated struct GithubGraphQLPullRequestResponse: Decodable { aliasMap: [String: String], owner: String, repo: String - ) -> [String: GithubPullRequest] { - var results: [String: GithubPullRequest] = [:] - for (alias, connection) in data.repository.pullRequestsByAlias { - guard let branch = aliasMap[alias] else { + ) -> [String: GithubPullRequest?] { + var results: [String: GithubPullRequest?] = [:] + // Iterate aliasMap (not pullRequestsByAlias) so every queried branch appears + // in the result — nil means "queried but no PR found", which is semantically + // different from "branch not queried at all". + for (alias, branch) in aliasMap { + guard let connection = data.repository.pullRequestsByAlias[alias] else { + results[branch] = nil continue } - if let node = connection.bestMatchingPullRequest(owner: owner, repo: repo) { - results[branch] = node.pullRequest - } + results[branch] = connection.bestMatchingPullRequest(owner: owner, repo: repo)?.pullRequest } return results } diff --git a/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift b/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift index ca5708308..739fb1408 100644 --- a/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift +++ b/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift @@ -70,7 +70,7 @@ final class PullRequestRefreshCoordinator { repositoryID: Repository.ID, repositoryRootURL: URL, worktreeIDs: [Worktree.ID], - prsByBranch: [String: GithubPullRequest] + prsByBranch: [String: GithubPullRequest?] ) case failed( repositoryID: Repository.ID, @@ -337,7 +337,7 @@ final class PullRequestRefreshCoordinator { private func emitOutcomes( _ requests: [Request], - prsByRepo: [RepoKey: [String: GithubPullRequest]], + prsByRepo: [RepoKey: [String: GithubPullRequest?]], failedMessagesByRepo: [RepoKey: String] ) { for request in requests { @@ -369,13 +369,16 @@ final class PullRequestRefreshCoordinator { private func mergedPullRequests( for request: Request, - prsByRepo: [RepoKey: [String: GithubPullRequest]] - ) -> [String: GithubPullRequest] { - var prsByBranch: [String: GithubPullRequest] = [:] + prsByRepo: [RepoKey: [String: GithubPullRequest?]] + ) -> [String: GithubPullRequest?] { + var prsByBranch: [String: GithubPullRequest?] = [:] for branch in request.branches { for repository in request.repositories { - if let pullRequest = prsByRepo[repository.key]?[branch] { - prsByBranch[branch] = pullRequest + let repoResult = prsByRepo[repository.key] + // Check if this repo even knows about this branch — if the key exists, + // use its value (PR or nil) and stop searching. + if let prs = repoResult, prs.keys.contains(branch) { + prsByBranch[branch] = prs[branch] break } } @@ -437,12 +440,12 @@ final class PullRequestRefreshCoordinator { } private struct RepoFetchResults: Sendable { - var successByRepo: [RepoKey: [String: GithubPullRequest]] = [:] + var successByRepo: [RepoKey: [String: GithubPullRequest?]] = [:] var failedMessagesByRepo: [RepoKey: String] = [:] } private enum RepoFetchOutcome: Sendable { - case success(RepoKey, [String: GithubPullRequest]) + case success(RepoKey, [String: GithubPullRequest?]) case failed(RepoKey, String) } } diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift index a29cedf58..7f4c79d74 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift @@ -793,7 +793,7 @@ extension RepositoriesFeature { private func mergePullRequestRefreshResults( repositoryID: Repository.ID, - prsByBranch: [String: GithubPullRequest], + prsByBranch: [String: GithubPullRequest?], state: inout State ) { guard !prsByBranch.isEmpty else { @@ -805,10 +805,17 @@ extension RepositoriesFeature { // Host batches race independently. Use the returned PR URL to recover its // source repo, then compare against the original remote order before replacing. for (branch, pullRequest) in prsByBranch { - let priority = remotePriority(for: pullRequest, remotePriorities: remotePriorities) - if merged[branch] == nil || priority < (resultPriorities[branch] ?? .max) { - merged[branch] = pullRequest - resultPriorities[branch] = priority + if let pullRequest { + let priority = remotePriority(for: pullRequest, remotePriorities: remotePriorities) + if merged[branch] == nil || priority < (resultPriorities[branch] ?? .max) { + merged[branch] = pullRequest + resultPriorities[branch] = priority + } + } else if merged[branch] == nil { + // Explicitly no PR found for this branch, and we haven't seen a PR from + // another host yet — record nil so downstream consumers can clear stale state. + // If merged already has a PR, don't overwrite it with nil. + merged[branch] = nil } } state.prRefreshResultsByRepositoryID[repositoryID] = merged @@ -858,7 +865,7 @@ extension RepositoriesFeature { private func pullRequestsByWorktreeID( repository: Repository, worktreeIDs: [Worktree.ID], - prsByBranch: [String: GithubPullRequest] + prsByBranch: [String: GithubPullRequest?] ) -> [Worktree.ID: GithubPullRequest?] { var prsByWorktreeID: [Worktree.ID: GithubPullRequest?] = [:] for worktreeID in worktreeIDs { @@ -886,18 +893,9 @@ extension RepositoriesFeature { gitClient: gitClient ) guard !remoteInfos.isEmpty else { - let clearedPullRequestsByWorktreeID = Dictionary( - worktreeIDs.map { ($0, Optional.none) }, - uniquingKeysWith: { first, _ in first } - ) - await send( - .githubIntegration( - .repositoryPullRequestsLoaded( - repositoryID: repositoryID, - pullRequestsByWorktreeID: clearedPullRequestsByWorktreeID - ) - ) - ) + // No GitHub remote configured for this repository — preserve existing PR + // values rather than clearing them, which would cause a flicker during + // refresh cycles. await send(.githubIntegration(.repositoryPullRequestRefreshCompleted(repositoryID))) return } diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift index c9202e2b5..094ad3e5f 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift @@ -89,7 +89,7 @@ extension RepositoriesFeature { [branch], nil ) - return pullRequestsByBranch?[branch] + return pullRequestsByBranch?[branch] ?? nil } } diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift index 718a7c2d3..f8039dac8 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift @@ -336,7 +336,7 @@ struct RepositoriesFeature { var pendingPullRequestRefreshByRepositoryID: [Repository.ID: PendingPullRequestRefresh] = [:] var inFlightPullRequestRefreshRepositoryIDs: Set = [] var prRefreshBatchCountsByRepositoryID: [Repository.ID: Int] = [:] - var prRefreshResultsByRepositoryID: [Repository.ID: [String: GithubPullRequest]] = [:] + var prRefreshResultsByRepositoryID: [Repository.ID: [String: GithubPullRequest?]] = [:] /// Cross-host PR refresh batches complete independently; keep the intended remote /// order so same-branch collisions are resolved by priority, not arrival time. var prRefreshRemotePrioritiesByRepositoryID: [Repository.ID: [String: Int]] = [:] From dfb04fb5b6fcb11519bfbcbcf44bfbe66a7bdcb9 Mon Sep 17 00:00:00 2001 From: Alex Date: Fri, 3 Jul 2026 11:10:32 +0800 Subject: [PATCH 2/6] Fix tests for PR state optional type change 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 --- ...atchedPullRequestRefreshReducerTests.swift | 3 --- .../GithubBatchPullRequestsTests.swift | 20 +++++++++---------- supacodeTests/GithubCLIClientTests.swift | 6 +++--- .../PullRequestRefreshCoordinatorTests.swift | 20 +++++++++---------- supacodeTests/RepositoriesFeatureTests.swift | 3 --- 5 files changed, 23 insertions(+), 29 deletions(-) diff --git a/supacodeTests/BatchedPullRequestRefreshReducerTests.swift b/supacodeTests/BatchedPullRequestRefreshReducerTests.swift index 30607aab9..4ed782935 100644 --- a/supacodeTests/BatchedPullRequestRefreshReducerTests.swift +++ b/supacodeTests/BatchedPullRequestRefreshReducerTests.swift @@ -315,9 +315,6 @@ struct BatchedPullRequestRefreshReducerTests { await store.receive(\.githubIntegration.repositoryPullRequestRefreshRequested) { $0.inFlightPullRequestRefreshRepositoryIDs = [context.repository.id] } - await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) { - $0.worktreeInfoByID.removeValue(forKey: context.featureWorktree.id) - } await store.receive(\.githubIntegration.repositoryPullRequestRefreshCompleted) { $0.inFlightPullRequestRefreshRepositoryIDs = [] } diff --git a/supacodeTests/GithubBatchPullRequestsTests.swift b/supacodeTests/GithubBatchPullRequestsTests.swift index a66ebfec3..dfea93b21 100644 --- a/supacodeTests/GithubBatchPullRequestsTests.swift +++ b/supacodeTests/GithubBatchPullRequestsTests.swift @@ -61,9 +61,9 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect(prs["feature-a"]?.number == 2) - #expect(prs["feature-a"]?.title == "Primary PR") - #expect(prs["feature-b"] == nil) + #expect((prs["feature-a"] ?? nil)?.number == 2) + #expect((prs["feature-a"] ?? nil)?.title == "Primary PR") + #expect((prs["feature-b"] ?? nil) == nil) } @Test func ignoresForkOnlyMatches() throws { @@ -104,7 +104,7 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect(prs["feature-a"] == nil) + #expect((prs["feature-a"] ?? nil) == nil) } @Test func ignoresPullRequestWithUnknownHeadRepository() throws { @@ -142,7 +142,7 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect(prs["feature-a"] == nil) + #expect((prs["feature-a"] ?? nil) == nil) } @Test func ignoresForkEvenWhenBaseBranchDiffers() throws { @@ -201,7 +201,7 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect(prs["feature-a"] == nil) + #expect((prs["feature-a"] ?? nil) == nil) } @Test func prefersOpenOverMergedEvenIfOlder() throws { @@ -258,8 +258,8 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect(prs["feature-a"]?.number == 11) - #expect(prs["feature-a"]?.title == "Open PR") + #expect((prs["feature-a"] ?? nil)?.number == 11) + #expect((prs["feature-a"] ?? nil)?.title == "Open PR") } @Test func fallsBackToLatestMerged() throws { @@ -316,7 +316,7 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect(prs["feature-a"]?.number == 21) - #expect(prs["feature-a"]?.title == "Merged Newer") + #expect((prs["feature-a"] ?? nil)?.number == 21) + #expect((prs["feature-a"] ?? nil)?.title == "Merged Newer") } } diff --git a/supacodeTests/GithubCLIClientTests.swift b/supacodeTests/GithubCLIClientTests.swift index 00e3bbe54..9cb05236d 100644 --- a/supacodeTests/GithubCLIClientTests.swift +++ b/supacodeTests/GithubCLIClientTests.swift @@ -662,7 +662,7 @@ struct GithubCLIClientTests { let result = try await client.batchPullRequestsAcrossRepositories("github.com", requests, nil) let alphaPRs = try #require(result.successByRepo[RepoKey(owner: "khoi", repo: "alpha")]) - let pullRequest = try #require(alphaPRs["feat-1"]) + let pullRequest = try #require(alphaPRs["feat-1"] ?? nil) #expect(pullRequest.number == 42) } @@ -683,7 +683,7 @@ struct GithubCLIClientTests { let result = try await client.batchPullRequestsAcrossRepositories("github.com", requests, nil) let kingfisherPRs = try #require(result.successByRepo[RepoKey(owner: "onevcat", repo: "Kingfisher")]) - #expect(kingfisherPRs["master"] == nil) + #expect((kingfisherPRs["master"] ?? nil) == nil) } @Test func batchAcrossRepositoriesAllowsPullRequestFromConfiguredHeadRemote() async throws { @@ -724,7 +724,7 @@ struct GithubCLIClientTests { let result = try await client.batchPullRequestsAcrossRepositories("github.com", requests, nil) let upstreamPRs = try #require(result.successByRepo[RepoKey(owner: "supabitapp", repo: "supacode")]) - #expect(upstreamPRs["feature"]?.number == 42) + #expect((upstreamPRs["feature"] ?? nil)?.number == 42) } @Test func executableResolutionIsSingleFlightAndReused() async { diff --git a/supacodeTests/PullRequestRefreshCoordinatorTests.swift b/supacodeTests/PullRequestRefreshCoordinatorTests.swift index 2017e0762..29f646783 100644 --- a/supacodeTests/PullRequestRefreshCoordinatorTests.swift +++ b/supacodeTests/PullRequestRefreshCoordinatorTests.swift @@ -100,7 +100,7 @@ struct PullRequestRefreshCoordinatorTests { clock: clock, outcomes: outcomes, batched: { _, requests in - var success: [RepoKey: [String: GithubPullRequest]] = [:] + var success: [RepoKey: [String: GithubPullRequest?]] = [:] var failed: [RepoKey: GithubCLIError] = [:] for request in requests { let key = RepoKey(owner: request.owner, repo: request.repo) @@ -258,7 +258,7 @@ struct PullRequestRefreshCoordinatorTests { clock: clock, outcomes: outcomes, batched: { _, requests in - var dict: [RepoKey: [String: GithubPullRequest]] = [:] + var dict: [RepoKey: [String: GithubPullRequest?]] = [:] for request in requests { dict[request.key] = [ "feat-1": makeFixturePullRequest(repo: request.repo), @@ -308,7 +308,7 @@ struct PullRequestRefreshCoordinatorTests { clock: clock, outcomes: outcomes, batched: { _, requests in - var dict: [RepoKey: [String: GithubPullRequest]] = [:] + var dict: [RepoKey: [String: GithubPullRequest?]] = [:] for request in requests { if request.repo == "upstream" { dict[request.key] = ["feat-1": makeFixturePullRequest(repo: "upstream")] @@ -335,14 +335,14 @@ struct PullRequestRefreshCoordinatorTests { ] #expect(calls.first?.requests.allSatisfy { $0.allowedHeadRepositories == expectedAllowedHeadRepositories } == true) - let refreshed = await outcomes.snapshot().compactMap { outcome -> [String: GithubPullRequest]? in + let refreshed = await outcomes.snapshot().compactMap { outcome -> [String: GithubPullRequest?]? in if case .refreshed("local", _, _, let prsByBranch) = outcome { return prsByBranch } return nil } #expect(refreshed.count == 1) - #expect(refreshed.first?["feat-1"]?.title == "PR-upstream") + #expect((refreshed.first?["feat-1"] ?? nil)?.title == "PR-upstream") } @Test func duplicateRepoKeysFallbackOnceAndFanOutToEachRepository() async throws { @@ -459,7 +459,7 @@ struct PullRequestRefreshCoordinatorTests { clock: clock, outcomes: outcomes, batched: { _, requests in - var dict: [RepoKey: [String: GithubPullRequest]] = [:] + var dict: [RepoKey: [String: GithubPullRequest?]] = [:] for request in requests { let pullRequest = makeFixturePullRequest(repo: request.repo) dict[RepoKey(owner: request.owner, repo: request.repo)] = ["feat-1": pullRequest] @@ -475,7 +475,7 @@ struct PullRequestRefreshCoordinatorTests { let snapshots = await outcomes.snapshot() let refresh = try #require( - snapshots.compactMap { snapshot -> (String, [String: GithubPullRequest])? in + snapshots.compactMap { snapshot -> (String, [String: GithubPullRequest?])? in if case .refreshed(let id, _, _, let prs) = snapshot { return (id, prs) } @@ -484,7 +484,7 @@ struct PullRequestRefreshCoordinatorTests { .first ) #expect(refresh.0 == "alpha") - #expect(refresh.1["feat-1"]?.title == "PR-alpha") + #expect((refresh.1["feat-1"] ?? nil)?.title == "PR-alpha") } @Test func enqueueAfterFlushStartsNewDebounceWindow() async throws { @@ -560,7 +560,7 @@ private func makeCoordinator( CrossRepoPullRequestResult, legacy: @escaping @Sendable (String, String, String, [String]) async throws -> - [String: GithubPullRequest] = { _, _, _, _ in [:] } + [String: GithubPullRequest?] = { _, _, _, _ in [:] } ) -> PullRequestRefreshCoordinator { var client = GithubCLIClient.testValue client.batchPullRequestsAcrossRepositories = { host, requests, accountOverride in @@ -629,7 +629,7 @@ nonisolated private func request( nonisolated private func successResult( for requests: [CrossRepoPullRequestRequest] ) -> CrossRepoPullRequestResult { - var dict: [RepoKey: [String: GithubPullRequest]] = [:] + var dict: [RepoKey: [String: GithubPullRequest?]] = [:] for request in requests { dict[RepoKey(owner: request.owner, repo: request.repo)] = [:] } diff --git a/supacodeTests/RepositoriesFeatureTests.swift b/supacodeTests/RepositoriesFeatureTests.swift index 04f230f6b..ae7b4b2dd 100644 --- a/supacodeTests/RepositoriesFeatureTests.swift +++ b/supacodeTests/RepositoriesFeatureTests.swift @@ -5933,7 +5933,6 @@ struct RepositoriesFeatureTests { await store.receive(\.githubIntegration.repositoryPullRequestRefreshRequested) { $0.inFlightPullRequestRefreshRepositoryIDs = [repository.id] } - await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) await store.receive(\.githubIntegration.repositoryPullRequestRefreshCompleted) { $0.inFlightPullRequestRefreshRepositoryIDs = [] } @@ -6062,7 +6061,6 @@ struct RepositoriesFeatureTests { await store.receive(\.githubIntegration.repositoryPullRequestRefreshRequested) { $0.inFlightPullRequestRefreshRepositoryIDs = [repository.id] } - await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) await store.receive(\.githubIntegration.repositoryPullRequestRefreshCompleted) { $0.inFlightPullRequestRefreshRepositoryIDs = [] } @@ -6166,7 +6164,6 @@ struct RepositoriesFeatureTests { await store.receive(\.githubIntegration.repositoryPullRequestRefreshRequested) { $0.inFlightPullRequestRefreshRepositoryIDs = [repository.id] } - await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) await store.receive(\.githubIntegration.repositoryPullRequestRefreshCompleted) { $0.inFlightPullRequestRefreshRepositoryIDs = [] } From f92651eadbc83bf74fc3729dd89128cac43e7e86 Mon Sep 17 00:00:00 2001 From: Alex Date: Sat, 4 Jul 2026 10:30:05 +0800 Subject: [PATCH 3/6] Fix PR state flicker with Set-based tri-state semantics Revert the [String: GithubPullRequest?] approach (Swift dict[key]=nil deletes the key, not stores .some(nil)) and implement tri-state using a separate Set 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 - 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 --- supacode/Clients/Github/GithubCLIClient.swift | 37 ++++++------ supacode/Clients/Github/GithubCLIModels.swift | 4 +- .../GithubGraphQLPullRequestResponse.swift | 16 +++-- .../PullRequestRefreshCoordinator.swift | 33 ++++++---- ...epositoriesFeature+GithubIntegration.swift | 60 ++++++++++++------- ...epositoriesFeature+WorkspaceChildren.swift | 2 +- .../Reducer/RepositoriesFeature.swift | 5 +- ...atchedPullRequestRefreshReducerTests.swift | 21 +++++-- .../GithubBatchPullRequestsTests.swift | 20 +++---- supacodeTests/GithubCLIClientTests.swift | 6 +- .../PullRequestRefreshCoordinatorTests.swift | 28 ++++----- 11 files changed, 133 insertions(+), 99 deletions(-) diff --git a/supacode/Clients/Github/GithubCLIClient.swift b/supacode/Clients/Github/GithubCLIClient.swift index 972d61c4f..94f4dd5da 100644 --- a/supacode/Clients/Github/GithubCLIClient.swift +++ b/supacode/Clients/Github/GithubCLIClient.swift @@ -177,7 +177,7 @@ struct GithubCLIClient: Sendable { var resolveRemoteInfo: @Sendable (URL) async -> GithubRemoteInfo? var latestRun: @Sendable (URL, String, GithubAccountOverride?) async throws -> GithubWorkflowRun? var batchPullRequests: - @Sendable (String, String, String, [String], GithubAccountOverride?) async throws -> [String: GithubPullRequest?] + @Sendable (String, String, String, [String], GithubAccountOverride?) async throws -> [String: GithubPullRequest] var batchPullRequestsAcrossRepositories: @Sendable (String, [CrossRepoPullRequestRequest], GithubAccountOverride?) async throws -> CrossRepoPullRequestResult var mergePullRequest: @@ -330,7 +330,7 @@ nonisolated private func latestRunFetcher( nonisolated private func batchPullRequestsFetcher( shell: ShellClient, resolver: GithubCLIExecutableResolver -) -> @Sendable (String, String, String, [String], GithubAccountOverride?) async throws -> [String: GithubPullRequest?] { +) -> @Sendable (String, String, String, [String], GithubAccountOverride?) async throws -> [String: GithubPullRequest] { { host, owner, repo, branches, accountOverride in try await withExpectedGithubAccount( shell: shell, @@ -365,7 +365,7 @@ nonisolated private let crossRepoBatchAliasLimit = 15 nonisolated private let crossRepoBatchMaxConcurrentRequests = 3 nonisolated private struct CrossRepoChunkOutcome: Sendable { - let successByRepo: [RepoKey: [String: GithubPullRequest?]] + let successByRepo: [RepoKey: [String: GithubPullRequest]] let failedRepos: [RepoKey: GithubCLIError] } @@ -492,7 +492,7 @@ nonisolated private func loadCrossRepoChunks( nonisolated private func mergeCrossRepoChunkResults( _ outcomes: [CrossRepoChunkOutcome] ) -> CrossRepoPullRequestResult { - var success: [RepoKey: [String: GithubPullRequest?]] = [:] + var success: [RepoKey: [String: GithubPullRequest]] = [:] var failed: [RepoKey: GithubCLIError] = [:] for outcome in outcomes { for (key, prs) in outcome.successByRepo { @@ -537,7 +537,7 @@ nonisolated private func fetchCrossRepoChunk( let response = try GithubCLIOutput.decode(CrossRepoPullRequestResponse.self, from: output, decoder: decoder) let errorMessagesByAlias = response.errorMessagesByAlias() - var success: [RepoKey: [String: GithubPullRequest?]] = [:] + var success: [RepoKey: [String: GithubPullRequest]] = [:] var failed: [RepoKey: GithubCLIError] = [:] for (alias, key) in plan.repoAliases { if let detail = errorMessagesByAlias[alias] { @@ -669,14 +669,15 @@ nonisolated private func rankCrossRepoPullRequests( pullRequestsByAlias: [String: GithubGraphQLPullRequestResponse.PullRequestConnection], aliasMap: [String: String], allowedHeadRepositories: Set -) -> [String: GithubPullRequest?] { - var results: [String: GithubPullRequest?] = [:] - for (alias, branch) in aliasMap { - guard let connection = pullRequestsByAlias[alias] else { - results[branch] = nil +) -> [String: GithubPullRequest] { + var results: [String: GithubPullRequest] = [:] + for (alias, connection) in pullRequestsByAlias { + guard let branch = aliasMap[alias] else { continue } - results[branch] = connection.bestMatchingPullRequest(allowedHeadRepositories: allowedHeadRepositories)?.pullRequest + if let node = connection.bestMatchingPullRequest(allowedHeadRepositories: allowedHeadRepositories) { + results[branch] = node.pullRequest + } } return results } @@ -1054,9 +1055,9 @@ nonisolated private func loadPullRequestChunks( resolver: GithubCLIExecutableResolver, request: GithubPullRequestsRequest, chunks: [[String]] -) async throws -> [Int: [String: GithubPullRequest?]] { +) async throws -> [Int: [String: GithubPullRequest]] { try await withThrowingTaskGroup( - of: (Int, [String: GithubPullRequest?]).self + of: (Int, [String: GithubPullRequest]).self ) { group in var nextChunkIndex = 0 let initialCount = min(batchPullRequestsMaxConcurrentRequests, chunks.count) @@ -1075,7 +1076,7 @@ nonisolated private func loadPullRequestChunks( nextChunkIndex += 1 } - var resultsByChunkIndex: [Int: [String: GithubPullRequest?]] = [:] + var resultsByChunkIndex: [Int: [String: GithubPullRequest]] = [:] while let (chunkIndex, prsByBranch) = try await group.next() { resultsByChunkIndex[chunkIndex] = prsByBranch if nextChunkIndex < chunks.count { @@ -1099,10 +1100,10 @@ nonisolated private func loadPullRequestChunks( } nonisolated private func mergePullRequestChunkResults( - _ chunkResults: [Int: [String: GithubPullRequest?]], + _ chunkResults: [Int: [String: GithubPullRequest]], chunkCount: Int -) -> [String: GithubPullRequest?] { - var results: [String: GithubPullRequest?] = [:] +) -> [String: GithubPullRequest] { + var results: [String: GithubPullRequest] = [:] for chunkIndex in 0.. (Int, [String: GithubPullRequest?]) { +) async throws -> (Int, [String: GithubPullRequest]) { let (query, aliasMap) = makeBatchPullRequestsQuery(branches: chunk) let output = try await runGh( shell: shell, diff --git a/supacode/Clients/Github/GithubCLIModels.swift b/supacode/Clients/Github/GithubCLIModels.swift index 37e198e0b..2e8a122da 100644 --- a/supacode/Clients/Github/GithubCLIModels.swift +++ b/supacode/Clients/Github/GithubCLIModels.swift @@ -167,11 +167,11 @@ nonisolated struct CrossRepoPullRequestRequest: Sendable, Hashable { } nonisolated struct CrossRepoPullRequestResult: Sendable { - let successByRepo: [RepoKey: [String: GithubPullRequest?]] + let successByRepo: [RepoKey: [String: GithubPullRequest]] let failedRepos: [RepoKey: GithubCLIError] init( - successByRepo: [RepoKey: [String: GithubPullRequest?]] = [:], + successByRepo: [RepoKey: [String: GithubPullRequest]] = [:], failedRepos: [RepoKey: GithubCLIError] = [:] ) { self.successByRepo = successByRepo diff --git a/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift b/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift index 41651d8dc..4e6ceb0ae 100644 --- a/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift +++ b/supacode/Clients/Github/GithubGraphQLPullRequestResponse.swift @@ -7,17 +7,15 @@ nonisolated struct GithubGraphQLPullRequestResponse: Decodable { aliasMap: [String: String], owner: String, repo: String - ) -> [String: GithubPullRequest?] { - var results: [String: GithubPullRequest?] = [:] - // Iterate aliasMap (not pullRequestsByAlias) so every queried branch appears - // in the result — nil means "queried but no PR found", which is semantically - // different from "branch not queried at all". - for (alias, branch) in aliasMap { - guard let connection = data.repository.pullRequestsByAlias[alias] else { - results[branch] = nil + ) -> [String: GithubPullRequest] { + var results: [String: GithubPullRequest] = [:] + for (alias, connection) in data.repository.pullRequestsByAlias { + guard let branch = aliasMap[alias] else { continue } - results[branch] = connection.bestMatchingPullRequest(owner: owner, repo: repo)?.pullRequest + if let node = connection.bestMatchingPullRequest(owner: owner, repo: repo) { + results[branch] = node.pullRequest + } } return results } diff --git a/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift b/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift index 739fb1408..6446388fb 100644 --- a/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift +++ b/supacode/Features/Repositories/BusinessLogic/PullRequestRefreshCoordinator.swift @@ -70,7 +70,8 @@ final class PullRequestRefreshCoordinator { repositoryID: Repository.ID, repositoryRootURL: URL, worktreeIDs: [Worktree.ID], - prsByBranch: [String: GithubPullRequest?] + prsByBranch: [String: GithubPullRequest], + confirmedNoPrBranches: Set ) case failed( repositoryID: Repository.ID, @@ -337,7 +338,7 @@ final class PullRequestRefreshCoordinator { private func emitOutcomes( _ requests: [Request], - prsByRepo: [RepoKey: [String: GithubPullRequest?]], + prsByRepo: [RepoKey: [String: GithubPullRequest]], failedMessagesByRepo: [RepoKey: String] ) { for request in requests { @@ -355,12 +356,21 @@ final class PullRequestRefreshCoordinator { ) ) } else { + // Only mark branches as "confirmed no PR" when all candidate repos + // succeeded — if any repo failed, the branch status is unknown and + // the reducer should preserve existing PR state. + let allCandidatesSucceeded = + !candidateKeys.isEmpty + && candidateKeys.allSatisfy { prsByRepo[$0] != nil && failedMessagesByRepo[$0] == nil } + let confirmedNoPrBranches: Set = + allCandidatesSucceeded ? Set(request.branches).subtracting(prsByBranch.keys) : [] resultHandler( .refreshed( repositoryID: request.repositoryID, repositoryRootURL: request.repositoryRootURL, worktreeIDs: request.worktreeIDs, - prsByBranch: prsByBranch + prsByBranch: prsByBranch, + confirmedNoPrBranches: confirmedNoPrBranches ) ) } @@ -369,16 +379,13 @@ final class PullRequestRefreshCoordinator { private func mergedPullRequests( for request: Request, - prsByRepo: [RepoKey: [String: GithubPullRequest?]] - ) -> [String: GithubPullRequest?] { - var prsByBranch: [String: GithubPullRequest?] = [:] + prsByRepo: [RepoKey: [String: GithubPullRequest]] + ) -> [String: GithubPullRequest] { + var prsByBranch: [String: GithubPullRequest] = [:] for branch in request.branches { for repository in request.repositories { - let repoResult = prsByRepo[repository.key] - // Check if this repo even knows about this branch — if the key exists, - // use its value (PR or nil) and stop searching. - if let prs = repoResult, prs.keys.contains(branch) { - prsByBranch[branch] = prs[branch] + if let pullRequest = prsByRepo[repository.key]?[branch] { + prsByBranch[branch] = pullRequest break } } @@ -440,12 +447,12 @@ final class PullRequestRefreshCoordinator { } private struct RepoFetchResults: Sendable { - var successByRepo: [RepoKey: [String: GithubPullRequest?]] = [:] + var successByRepo: [RepoKey: [String: GithubPullRequest]] = [:] var failedMessagesByRepo: [RepoKey: String] = [:] } private enum RepoFetchOutcome: Sendable { - case success(RepoKey, [String: GithubPullRequest?]) + case success(RepoKey, [String: GithubPullRequest]) case failed(RepoKey, String) } } diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift index 7f4c79d74..3e3062574 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift @@ -724,7 +724,7 @@ extension RepositoriesFeature { outcome: PullRequestRefreshCoordinator.Outcome ) -> Effect { switch outcome { - case .refreshed(let repositoryID, _, let worktreeIDs, let prsByBranch): + case .refreshed(let repositoryID, _, let worktreeIDs, let prsByBranch, let confirmedNoPrBranches): guard let repository = state.repositories[id: repositoryID] else { state.inFlightPullRequestRefreshRepositoryIDs.remove(repositoryID) clearPullRequestRefreshTracking(repositoryID: repositoryID, state: &state) @@ -733,6 +733,7 @@ extension RepositoriesFeature { mergePullRequestRefreshResults( repositoryID: repositoryID, prsByBranch: prsByBranch, + confirmedNoPrBranches: confirmedNoPrBranches, state: &state ) guard consumePullRequestRefreshBatch(repositoryID: repositoryID, state: &state) else { @@ -742,11 +743,16 @@ extension RepositoriesFeature { state.prRefreshResultsByRepositoryID.removeValue( forKey: repositoryID ) ?? [:] + let confirmedNoPrBranches = + state.prRefreshConfirmedNoPrBranchesByRepositoryID.removeValue( + forKey: repositoryID + ) ?? [] state.prRefreshResultPrioritiesByRepositoryID.removeValue(forKey: repositoryID) let prsByWorktreeID = pullRequestsByWorktreeID( repository: repository, worktreeIDs: worktreeIDs, - prsByBranch: mergedPRsByBranch + prsByBranch: mergedPRsByBranch, + confirmedNoPrBranches: confirmedNoPrBranches ) return .merge( .send( @@ -767,6 +773,7 @@ extension RepositoriesFeature { state.prRefreshResultsByRepositoryID.removeValue( forKey: repositoryID ) ?? [:] + let _ = state.prRefreshConfirmedNoPrBranchesByRepositoryID.removeValue(forKey: repositoryID) state.prRefreshResultPrioritiesByRepositoryID.removeValue(forKey: repositoryID) guard !mergedPRsByBranch.isEmpty, let repository = state.repositories[id: repositoryID] @@ -781,7 +788,8 @@ extension RepositoriesFeature { pullRequestsByWorktreeID: pullRequestsByWorktreeID( repository: repository, worktreeIDs: worktreeIDs, - prsByBranch: mergedPRsByBranch + prsByBranch: mergedPRsByBranch, + confirmedNoPrBranches: [] ) ) ) @@ -793,31 +801,31 @@ extension RepositoriesFeature { private func mergePullRequestRefreshResults( repositoryID: Repository.ID, - prsByBranch: [String: GithubPullRequest?], + prsByBranch: [String: GithubPullRequest], + confirmedNoPrBranches: Set, state: inout State ) { - guard !prsByBranch.isEmpty else { - return - } var merged = state.prRefreshResultsByRepositoryID[repositoryID] ?? [:] var resultPriorities = state.prRefreshResultPrioritiesByRepositoryID[repositoryID] ?? [:] let remotePriorities = state.prRefreshRemotePrioritiesByRepositoryID[repositoryID] ?? [:] // Host batches race independently. Use the returned PR URL to recover its // source repo, then compare against the original remote order before replacing. for (branch, pullRequest) in prsByBranch { - if let pullRequest { - let priority = remotePriority(for: pullRequest, remotePriorities: remotePriorities) - if merged[branch] == nil || priority < (resultPriorities[branch] ?? .max) { - merged[branch] = pullRequest - resultPriorities[branch] = priority - } - } else if merged[branch] == nil { - // Explicitly no PR found for this branch, and we haven't seen a PR from - // another host yet — record nil so downstream consumers can clear stale state. - // If merged already has a PR, don't overwrite it with nil. - merged[branch] = nil + let priority = remotePriority(for: pullRequest, remotePriorities: remotePriorities) + if merged[branch] == nil || priority < (resultPriorities[branch] ?? .max) { + merged[branch] = pullRequest + resultPriorities[branch] = priority } } + // Accumulate confirmed-no-PR branches. Only clear when all repos for a + // branch succeeded and none returned a PR — partial failures leave the + // branch out of confirmedNoPrBranches so existing state is preserved. + var existingConfirmed = state.prRefreshConfirmedNoPrBranchesByRepositoryID[repositoryID] ?? [] + existingConfirmed.formUnion(confirmedNoPrBranches) + // Remove any confirmed-no-PR entries that now have a PR (priority-based + // merge may have resolved a later host's PR over an earlier "no PR"). + existingConfirmed.subtract(merged.keys) + state.prRefreshConfirmedNoPrBranchesByRepositoryID[repositoryID] = existingConfirmed state.prRefreshResultsByRepositoryID[repositoryID] = merged state.prRefreshResultPrioritiesByRepositoryID[repositoryID] = resultPriorities } @@ -838,6 +846,7 @@ extension RepositoriesFeature { ) { state.prRefreshBatchCountsByRepositoryID.removeValue(forKey: repositoryID) state.prRefreshResultsByRepositoryID.removeValue(forKey: repositoryID) + state.prRefreshConfirmedNoPrBranchesByRepositoryID.removeValue(forKey: repositoryID) state.prRefreshRemotePrioritiesByRepositoryID.removeValue(forKey: repositoryID) state.prRefreshResultPrioritiesByRepositoryID.removeValue(forKey: repositoryID) } @@ -845,6 +854,7 @@ extension RepositoriesFeature { private func clearAllPullRequestRefreshTracking(state: inout State) { state.prRefreshBatchCountsByRepositoryID.removeAll() state.prRefreshResultsByRepositoryID.removeAll() + state.prRefreshConfirmedNoPrBranchesByRepositoryID.removeAll() state.prRefreshRemotePrioritiesByRepositoryID.removeAll() state.prRefreshResultPrioritiesByRepositoryID.removeAll() } @@ -865,13 +875,19 @@ extension RepositoriesFeature { private func pullRequestsByWorktreeID( repository: Repository, worktreeIDs: [Worktree.ID], - prsByBranch: [String: GithubPullRequest?] + prsByBranch: [String: GithubPullRequest], + confirmedNoPrBranches: Set ) -> [Worktree.ID: GithubPullRequest?] { var prsByWorktreeID: [Worktree.ID: GithubPullRequest?] = [:] for worktreeID in worktreeIDs { - if let worktree = repository.worktrees[id: worktreeID] { - prsByWorktreeID[worktreeID] = prsByBranch[worktree.name] - } + guard let worktree = repository.worktrees[id: worktreeID] else { continue } + if let pr = prsByBranch[worktree.name] { + prsByWorktreeID[worktreeID] = pr + } else if confirmedNoPrBranches.contains(worktree.name) { + // All repos confirmed no PR for this branch — explicitly clear. + prsByWorktreeID[worktreeID] = nil + } + // Otherwise: unknown status (partial failure) — omit to preserve existing. } return prsByWorktreeID } diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift index 094ad3e5f..c9202e2b5 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature+WorkspaceChildren.swift @@ -89,7 +89,7 @@ extension RepositoriesFeature { [branch], nil ) - return pullRequestsByBranch?[branch] ?? nil + return pullRequestsByBranch?[branch] } } diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift index f8039dac8..78beaec34 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift @@ -336,7 +336,10 @@ struct RepositoriesFeature { var pendingPullRequestRefreshByRepositoryID: [Repository.ID: PendingPullRequestRefresh] = [:] var inFlightPullRequestRefreshRepositoryIDs: Set = [] var prRefreshBatchCountsByRepositoryID: [Repository.ID: Int] = [:] - var prRefreshResultsByRepositoryID: [Repository.ID: [String: GithubPullRequest?]] = [:] + var prRefreshResultsByRepositoryID: [Repository.ID: [String: GithubPullRequest]] = [:] + /// Branches confirmed as having no PR (all repos succeeded, none returned a PR). + /// Used to clear stale PR state without flashing when only some repos succeed. + var prRefreshConfirmedNoPrBranchesByRepositoryID: [Repository.ID: Set] = [:] /// Cross-host PR refresh batches complete independently; keep the intended remote /// order so same-branch collisions are resolved by priority, not arrival time. var prRefreshRemotePrioritiesByRepositoryID: [Repository.ID: [String: Int]] = [:] diff --git a/supacodeTests/BatchedPullRequestRefreshReducerTests.swift b/supacodeTests/BatchedPullRequestRefreshReducerTests.swift index 4ed782935..2ac7dde58 100644 --- a/supacodeTests/BatchedPullRequestRefreshReducerTests.swift +++ b/supacodeTests/BatchedPullRequestRefreshReducerTests.swift @@ -109,12 +109,14 @@ struct BatchedPullRequestRefreshReducerTests { repositoryID: context.repository.id, repositoryRootURL: context.repoRootURL, worktreeIDs: context.worktreeIDs, - prsByBranch: ["feature": githubPullRequest] + prsByBranch: ["feature": githubPullRequest], + confirmedNoPrBranches: [] ) )) ) { $0.prRefreshBatchCountsByRepositoryID[context.repository.id] = 1 $0.prRefreshResultsByRepositoryID[context.repository.id] = ["feature": githubPullRequest] + $0.prRefreshConfirmedNoPrBranchesByRepositoryID[context.repository.id] = [] $0.prRefreshResultPrioritiesByRepositoryID[context.repository.id] = ["feature": .max] } @@ -125,12 +127,14 @@ struct BatchedPullRequestRefreshReducerTests { repositoryID: context.repository.id, repositoryRootURL: context.repoRootURL, worktreeIDs: context.worktreeIDs, - prsByBranch: [:] + prsByBranch: [:], + confirmedNoPrBranches: [] ) )) ) { $0.prRefreshBatchCountsByRepositoryID = [:] $0.prRefreshResultsByRepositoryID = [:] + $0.prRefreshConfirmedNoPrBranchesByRepositoryID = [:] $0.prRefreshResultPrioritiesByRepositoryID = [:] } await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) { @@ -200,12 +204,14 @@ struct BatchedPullRequestRefreshReducerTests { repositoryID: context.repository.id, repositoryRootURL: context.repoRootURL, worktreeIDs: context.worktreeIDs, - prsByBranch: ["feature": enterprisePullRequest] + prsByBranch: ["feature": enterprisePullRequest], + confirmedNoPrBranches: [] ) )) ) { $0.prRefreshBatchCountsByRepositoryID[context.repository.id] = 1 $0.prRefreshResultsByRepositoryID[context.repository.id] = ["feature": enterprisePullRequest] + $0.prRefreshConfirmedNoPrBranchesByRepositoryID[context.repository.id] = [] $0.prRefreshResultPrioritiesByRepositoryID[context.repository.id] = ["feature": 1] } @@ -216,12 +222,14 @@ struct BatchedPullRequestRefreshReducerTests { repositoryID: context.repository.id, repositoryRootURL: context.repoRootURL, worktreeIDs: context.worktreeIDs, - prsByBranch: ["feature": originPullRequest] + prsByBranch: ["feature": originPullRequest], + confirmedNoPrBranches: [] ) )) ) { $0.prRefreshBatchCountsByRepositoryID = [:] $0.prRefreshResultsByRepositoryID = [:] + $0.prRefreshConfirmedNoPrBranchesByRepositoryID = [:] $0.prRefreshResultPrioritiesByRepositoryID = [:] } await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) { @@ -281,7 +289,7 @@ struct BatchedPullRequestRefreshReducerTests { #expect(enqueued.value.count == 1) } - @Test func refreshClearsStalePullRequestsWhenGithubRemotesDisappear() async { + @Test func refreshPreservesPullRequestsWhenGithubRemotesUnavailable() async { let context = makeContext() let enqueued = LockIsolated<[PullRequestRefreshCoordinator.Request]>([]) let stalePullRequest = makePullRequestFixture(url: "https://github.com/khoi/alpha/pull/7") @@ -339,7 +347,8 @@ struct BatchedPullRequestRefreshReducerTests { repositoryID: context.repository.id, repositoryRootURL: context.repoRootURL, worktreeIDs: context.worktreeIDs, - prsByBranch: ["feature": pullRequest] + prsByBranch: ["feature": pullRequest], + confirmedNoPrBranches: [] ) await store.send(.githubIntegration(.pullRequestRefreshBatchOutcome(outcome))) diff --git a/supacodeTests/GithubBatchPullRequestsTests.swift b/supacodeTests/GithubBatchPullRequestsTests.swift index dfea93b21..a66ebfec3 100644 --- a/supacodeTests/GithubBatchPullRequestsTests.swift +++ b/supacodeTests/GithubBatchPullRequestsTests.swift @@ -61,9 +61,9 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect((prs["feature-a"] ?? nil)?.number == 2) - #expect((prs["feature-a"] ?? nil)?.title == "Primary PR") - #expect((prs["feature-b"] ?? nil) == nil) + #expect(prs["feature-a"]?.number == 2) + #expect(prs["feature-a"]?.title == "Primary PR") + #expect(prs["feature-b"] == nil) } @Test func ignoresForkOnlyMatches() throws { @@ -104,7 +104,7 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect((prs["feature-a"] ?? nil) == nil) + #expect(prs["feature-a"] == nil) } @Test func ignoresPullRequestWithUnknownHeadRepository() throws { @@ -142,7 +142,7 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect((prs["feature-a"] ?? nil) == nil) + #expect(prs["feature-a"] == nil) } @Test func ignoresForkEvenWhenBaseBranchDiffers() throws { @@ -201,7 +201,7 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect((prs["feature-a"] ?? nil) == nil) + #expect(prs["feature-a"] == nil) } @Test func prefersOpenOverMergedEvenIfOlder() throws { @@ -258,8 +258,8 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect((prs["feature-a"] ?? nil)?.number == 11) - #expect((prs["feature-a"] ?? nil)?.title == "Open PR") + #expect(prs["feature-a"]?.number == 11) + #expect(prs["feature-a"]?.title == "Open PR") } @Test func fallsBackToLatestMerged() throws { @@ -316,7 +316,7 @@ struct GithubBatchPullRequestsTests { owner: "octo", repo: "repo" ) - #expect((prs["feature-a"] ?? nil)?.number == 21) - #expect((prs["feature-a"] ?? nil)?.title == "Merged Newer") + #expect(prs["feature-a"]?.number == 21) + #expect(prs["feature-a"]?.title == "Merged Newer") } } diff --git a/supacodeTests/GithubCLIClientTests.swift b/supacodeTests/GithubCLIClientTests.swift index 9cb05236d..00e3bbe54 100644 --- a/supacodeTests/GithubCLIClientTests.swift +++ b/supacodeTests/GithubCLIClientTests.swift @@ -662,7 +662,7 @@ struct GithubCLIClientTests { let result = try await client.batchPullRequestsAcrossRepositories("github.com", requests, nil) let alphaPRs = try #require(result.successByRepo[RepoKey(owner: "khoi", repo: "alpha")]) - let pullRequest = try #require(alphaPRs["feat-1"] ?? nil) + let pullRequest = try #require(alphaPRs["feat-1"]) #expect(pullRequest.number == 42) } @@ -683,7 +683,7 @@ struct GithubCLIClientTests { let result = try await client.batchPullRequestsAcrossRepositories("github.com", requests, nil) let kingfisherPRs = try #require(result.successByRepo[RepoKey(owner: "onevcat", repo: "Kingfisher")]) - #expect((kingfisherPRs["master"] ?? nil) == nil) + #expect(kingfisherPRs["master"] == nil) } @Test func batchAcrossRepositoriesAllowsPullRequestFromConfiguredHeadRemote() async throws { @@ -724,7 +724,7 @@ struct GithubCLIClientTests { let result = try await client.batchPullRequestsAcrossRepositories("github.com", requests, nil) let upstreamPRs = try #require(result.successByRepo[RepoKey(owner: "supabitapp", repo: "supacode")]) - #expect((upstreamPRs["feature"] ?? nil)?.number == 42) + #expect(upstreamPRs["feature"]?.number == 42) } @Test func executableResolutionIsSingleFlightAndReused() async { diff --git a/supacodeTests/PullRequestRefreshCoordinatorTests.swift b/supacodeTests/PullRequestRefreshCoordinatorTests.swift index 29f646783..ef13cd6ba 100644 --- a/supacodeTests/PullRequestRefreshCoordinatorTests.swift +++ b/supacodeTests/PullRequestRefreshCoordinatorTests.swift @@ -100,7 +100,7 @@ struct PullRequestRefreshCoordinatorTests { clock: clock, outcomes: outcomes, batched: { _, requests in - var success: [RepoKey: [String: GithubPullRequest?]] = [:] + var success: [RepoKey: [String: GithubPullRequest]] = [:] var failed: [RepoKey: GithubCLIError] = [:] for request in requests { let key = RepoKey(owner: request.owner, repo: request.repo) @@ -258,7 +258,7 @@ struct PullRequestRefreshCoordinatorTests { clock: clock, outcomes: outcomes, batched: { _, requests in - var dict: [RepoKey: [String: GithubPullRequest?]] = [:] + var dict: [RepoKey: [String: GithubPullRequest]] = [:] for request in requests { dict[request.key] = [ "feat-1": makeFixturePullRequest(repo: request.repo), @@ -289,7 +289,7 @@ struct PullRequestRefreshCoordinatorTests { let snapshots = await outcomes.snapshot() let refreshed = snapshots.compactMap { outcome -> (Repository.ID, [String])? in - if case .refreshed(let id, _, _, let prs) = outcome { + if case .refreshed(let id, _, _, let prs, _) = outcome { return (id, Array(prs.keys)) } return nil @@ -308,7 +308,7 @@ struct PullRequestRefreshCoordinatorTests { clock: clock, outcomes: outcomes, batched: { _, requests in - var dict: [RepoKey: [String: GithubPullRequest?]] = [:] + var dict: [RepoKey: [String: GithubPullRequest]] = [:] for request in requests { if request.repo == "upstream" { dict[request.key] = ["feat-1": makeFixturePullRequest(repo: "upstream")] @@ -335,14 +335,14 @@ struct PullRequestRefreshCoordinatorTests { ] #expect(calls.first?.requests.allSatisfy { $0.allowedHeadRepositories == expectedAllowedHeadRepositories } == true) - let refreshed = await outcomes.snapshot().compactMap { outcome -> [String: GithubPullRequest?]? in - if case .refreshed("local", _, _, let prsByBranch) = outcome { + let refreshed = await outcomes.snapshot().compactMap { outcome -> [String: GithubPullRequest]? in + if case .refreshed("local", _, _, let prsByBranch, _) = outcome { return prsByBranch } return nil } #expect(refreshed.count == 1) - #expect((refreshed.first?["feat-1"] ?? nil)?.title == "PR-upstream") + #expect(refreshed.first?["feat-1"]?.title == "PR-upstream") } @Test func duplicateRepoKeysFallbackOnceAndFanOutToEachRepository() async throws { @@ -459,7 +459,7 @@ struct PullRequestRefreshCoordinatorTests { clock: clock, outcomes: outcomes, batched: { _, requests in - var dict: [RepoKey: [String: GithubPullRequest?]] = [:] + var dict: [RepoKey: [String: GithubPullRequest]] = [:] for request in requests { let pullRequest = makeFixturePullRequest(repo: request.repo) dict[RepoKey(owner: request.owner, repo: request.repo)] = ["feat-1": pullRequest] @@ -475,8 +475,8 @@ struct PullRequestRefreshCoordinatorTests { let snapshots = await outcomes.snapshot() let refresh = try #require( - snapshots.compactMap { snapshot -> (String, [String: GithubPullRequest?])? in - if case .refreshed(let id, _, _, let prs) = snapshot { + snapshots.compactMap { snapshot -> (String, [String: GithubPullRequest])? in + if case .refreshed(let id, _, _, let prs, _) = snapshot { return (id, prs) } return nil @@ -484,7 +484,7 @@ struct PullRequestRefreshCoordinatorTests { .first ) #expect(refresh.0 == "alpha") - #expect((refresh.1["feat-1"] ?? nil)?.title == "PR-alpha") + #expect(refresh.1["feat-1"]?.title == "PR-alpha") } @Test func enqueueAfterFlushStartsNewDebounceWindow() async throws { @@ -560,7 +560,7 @@ private func makeCoordinator( CrossRepoPullRequestResult, legacy: @escaping @Sendable (String, String, String, [String]) async throws -> - [String: GithubPullRequest?] = { _, _, _, _ in [:] } + [String: GithubPullRequest] = { _, _, _, _ in [:] } ) -> PullRequestRefreshCoordinator { var client = GithubCLIClient.testValue client.batchPullRequestsAcrossRepositories = { host, requests, accountOverride in @@ -629,7 +629,7 @@ nonisolated private func request( nonisolated private func successResult( for requests: [CrossRepoPullRequestRequest] ) -> CrossRepoPullRequestResult { - var dict: [RepoKey: [String: GithubPullRequest?]] = [:] + var dict: [RepoKey: [String: GithubPullRequest]] = [:] for request in requests { dict[RepoKey(owner: request.owner, repo: request.repo)] = [:] } @@ -708,7 +708,7 @@ actor OutcomeCollector { func refreshedRepositories() -> [String] { outcomes.compactMap { - if case .refreshed(let id, _, _, _) = $0 { + if case .refreshed(let id, _, _, _, _) = $0 { return id } return nil From 8968d8519ae09e2abb0908124bdd1bd0d5de437b Mon Sep 17 00:00:00 2001 From: Alex Date: Sat, 4 Jul 2026 10:37:01 +0800 Subject: [PATCH 4/6] Fix lint: redundant discardable let + variable name length Signed-off-by: Alex --- .../RepositoriesFeature+GithubIntegration.swift | 12 ++++++------ .../Repositories/Reducer/RepositoriesFeature.swift | 2 +- .../BatchedPullRequestRefreshReducerTests.swift | 8 ++++---- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift index 3e3062574..67b40d618 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift @@ -744,7 +744,7 @@ extension RepositoriesFeature { forKey: repositoryID ) ?? [:] let confirmedNoPrBranches = - state.prRefreshConfirmedNoPrBranchesByRepositoryID.removeValue( + state.prRefreshNoPrBranchesByID.removeValue( forKey: repositoryID ) ?? [] state.prRefreshResultPrioritiesByRepositoryID.removeValue(forKey: repositoryID) @@ -773,7 +773,7 @@ extension RepositoriesFeature { state.prRefreshResultsByRepositoryID.removeValue( forKey: repositoryID ) ?? [:] - let _ = state.prRefreshConfirmedNoPrBranchesByRepositoryID.removeValue(forKey: repositoryID) + _ = state.prRefreshNoPrBranchesByID.removeValue(forKey: repositoryID) state.prRefreshResultPrioritiesByRepositoryID.removeValue(forKey: repositoryID) guard !mergedPRsByBranch.isEmpty, let repository = state.repositories[id: repositoryID] @@ -820,12 +820,12 @@ extension RepositoriesFeature { // Accumulate confirmed-no-PR branches. Only clear when all repos for a // branch succeeded and none returned a PR — partial failures leave the // branch out of confirmedNoPrBranches so existing state is preserved. - var existingConfirmed = state.prRefreshConfirmedNoPrBranchesByRepositoryID[repositoryID] ?? [] + var existingConfirmed = state.prRefreshNoPrBranchesByID[repositoryID] ?? [] existingConfirmed.formUnion(confirmedNoPrBranches) // Remove any confirmed-no-PR entries that now have a PR (priority-based // merge may have resolved a later host's PR over an earlier "no PR"). existingConfirmed.subtract(merged.keys) - state.prRefreshConfirmedNoPrBranchesByRepositoryID[repositoryID] = existingConfirmed + state.prRefreshNoPrBranchesByID[repositoryID] = existingConfirmed state.prRefreshResultsByRepositoryID[repositoryID] = merged state.prRefreshResultPrioritiesByRepositoryID[repositoryID] = resultPriorities } @@ -846,7 +846,7 @@ extension RepositoriesFeature { ) { state.prRefreshBatchCountsByRepositoryID.removeValue(forKey: repositoryID) state.prRefreshResultsByRepositoryID.removeValue(forKey: repositoryID) - state.prRefreshConfirmedNoPrBranchesByRepositoryID.removeValue(forKey: repositoryID) + state.prRefreshNoPrBranchesByID.removeValue(forKey: repositoryID) state.prRefreshRemotePrioritiesByRepositoryID.removeValue(forKey: repositoryID) state.prRefreshResultPrioritiesByRepositoryID.removeValue(forKey: repositoryID) } @@ -854,7 +854,7 @@ extension RepositoriesFeature { private func clearAllPullRequestRefreshTracking(state: inout State) { state.prRefreshBatchCountsByRepositoryID.removeAll() state.prRefreshResultsByRepositoryID.removeAll() - state.prRefreshConfirmedNoPrBranchesByRepositoryID.removeAll() + state.prRefreshNoPrBranchesByID.removeAll() state.prRefreshRemotePrioritiesByRepositoryID.removeAll() state.prRefreshResultPrioritiesByRepositoryID.removeAll() } diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift index 78beaec34..8680e9154 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift @@ -339,7 +339,7 @@ struct RepositoriesFeature { var prRefreshResultsByRepositoryID: [Repository.ID: [String: GithubPullRequest]] = [:] /// Branches confirmed as having no PR (all repos succeeded, none returned a PR). /// Used to clear stale PR state without flashing when only some repos succeed. - var prRefreshConfirmedNoPrBranchesByRepositoryID: [Repository.ID: Set] = [:] + var prRefreshNoPrBranchesByID: [Repository.ID: Set] = [:] /// Cross-host PR refresh batches complete independently; keep the intended remote /// order so same-branch collisions are resolved by priority, not arrival time. var prRefreshRemotePrioritiesByRepositoryID: [Repository.ID: [String: Int]] = [:] diff --git a/supacodeTests/BatchedPullRequestRefreshReducerTests.swift b/supacodeTests/BatchedPullRequestRefreshReducerTests.swift index 2ac7dde58..8e52d73e6 100644 --- a/supacodeTests/BatchedPullRequestRefreshReducerTests.swift +++ b/supacodeTests/BatchedPullRequestRefreshReducerTests.swift @@ -116,7 +116,7 @@ struct BatchedPullRequestRefreshReducerTests { ) { $0.prRefreshBatchCountsByRepositoryID[context.repository.id] = 1 $0.prRefreshResultsByRepositoryID[context.repository.id] = ["feature": githubPullRequest] - $0.prRefreshConfirmedNoPrBranchesByRepositoryID[context.repository.id] = [] + $0.prRefreshNoPrBranchesByID[context.repository.id] = [] $0.prRefreshResultPrioritiesByRepositoryID[context.repository.id] = ["feature": .max] } @@ -134,7 +134,7 @@ struct BatchedPullRequestRefreshReducerTests { ) { $0.prRefreshBatchCountsByRepositoryID = [:] $0.prRefreshResultsByRepositoryID = [:] - $0.prRefreshConfirmedNoPrBranchesByRepositoryID = [:] + $0.prRefreshNoPrBranchesByID = [:] $0.prRefreshResultPrioritiesByRepositoryID = [:] } await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) { @@ -211,7 +211,7 @@ struct BatchedPullRequestRefreshReducerTests { ) { $0.prRefreshBatchCountsByRepositoryID[context.repository.id] = 1 $0.prRefreshResultsByRepositoryID[context.repository.id] = ["feature": enterprisePullRequest] - $0.prRefreshConfirmedNoPrBranchesByRepositoryID[context.repository.id] = [] + $0.prRefreshNoPrBranchesByID[context.repository.id] = [] $0.prRefreshResultPrioritiesByRepositoryID[context.repository.id] = ["feature": 1] } @@ -229,7 +229,7 @@ struct BatchedPullRequestRefreshReducerTests { ) { $0.prRefreshBatchCountsByRepositoryID = [:] $0.prRefreshResultsByRepositoryID = [:] - $0.prRefreshConfirmedNoPrBranchesByRepositoryID = [:] + $0.prRefreshNoPrBranchesByID = [:] $0.prRefreshResultPrioritiesByRepositoryID = [:] } await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) { From be0e66b5bab1af021bfc34545ce90f6d2db1251a Mon Sep 17 00:00:00 2001 From: Alex Date: Sat, 4 Jul 2026 11:01:20 +0800 Subject: [PATCH 5/6] Fix lint: variable name 'pr' too short Signed-off-by: Alex --- .../Reducer/RepositoriesFeature+GithubIntegration.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift index 67b40d618..36e4a33dd 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift @@ -881,8 +881,8 @@ extension RepositoriesFeature { var prsByWorktreeID: [Worktree.ID: GithubPullRequest?] = [:] for worktreeID in worktreeIDs { guard let worktree = repository.worktrees[id: worktreeID] else { continue } - if let pr = prsByBranch[worktree.name] { - prsByWorktreeID[worktreeID] = pr + if let pullRequest = prsByBranch[worktree.name] { + prsByWorktreeID[worktreeID] = pullRequest } else if confirmedNoPrBranches.contains(worktree.name) { // All repos confirmed no PR for this branch — explicitly clear. prsByWorktreeID[worktreeID] = nil From 8def49c5d4e6a49d2a29ae4b2ef5be08a2519eee Mon Sep 17 00:00:00 2001 From: onevcat Date: Sat, 4 Jul 2026 23:35:06 +0900 Subject: [PATCH 6/6] Fix confirmed-no-PR clearing and cross-host race in PR state tri-state - 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). --- ...epositoriesFeature+GithubIntegration.swift | 16 +- .../Reducer/RepositoriesFeature.swift | 4 + ...atchedPullRequestRefreshReducerTests.swift | 236 ++++++++++++++++++ .../PullRequestRefreshCoordinatorTests.swift | 84 +++++++ 4 files changed, 337 insertions(+), 3 deletions(-) diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift index 36e4a33dd..a5705bbb3 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature+GithubIntegration.swift @@ -743,10 +743,14 @@ extension RepositoriesFeature { state.prRefreshResultsByRepositoryID.removeValue( forKey: repositoryID ) ?? [:] - let confirmedNoPrBranches = + let hadFailedBatch = state.prRefreshFailedBatchRepositoryIDs.remove(repositoryID) != nil + let accumulatedConfirmedNoPrBranches = state.prRefreshNoPrBranchesByID.removeValue( forKey: repositoryID ) ?? [] + // A failed host batch means branch status on that host is unknown, even when + // it arrived before this final refreshed outcome — suppress confirmed clears. + let confirmedNoPrBranches = hadFailedBatch ? [] : accumulatedConfirmedNoPrBranches state.prRefreshResultPrioritiesByRepositoryID.removeValue(forKey: repositoryID) let prsByWorktreeID = pullRequestsByWorktreeID( repository: repository, @@ -766,6 +770,7 @@ extension RepositoriesFeature { .send(.githubIntegration(.repositoryPullRequestRefreshCompleted(repositoryID))) ) case .failed(let repositoryID, let worktreeIDs, _): + state.prRefreshFailedBatchRepositoryIDs.insert(repositoryID) guard consumePullRequestRefreshBatch(repositoryID: repositoryID, state: &state) else { return .none } @@ -773,6 +778,7 @@ extension RepositoriesFeature { state.prRefreshResultsByRepositoryID.removeValue( forKey: repositoryID ) ?? [:] + state.prRefreshFailedBatchRepositoryIDs.remove(repositoryID) _ = state.prRefreshNoPrBranchesByID.removeValue(forKey: repositoryID) state.prRefreshResultPrioritiesByRepositoryID.removeValue(forKey: repositoryID) guard !mergedPRsByBranch.isEmpty, @@ -847,6 +853,7 @@ extension RepositoriesFeature { state.prRefreshBatchCountsByRepositoryID.removeValue(forKey: repositoryID) state.prRefreshResultsByRepositoryID.removeValue(forKey: repositoryID) state.prRefreshNoPrBranchesByID.removeValue(forKey: repositoryID) + state.prRefreshFailedBatchRepositoryIDs.remove(repositoryID) state.prRefreshRemotePrioritiesByRepositoryID.removeValue(forKey: repositoryID) state.prRefreshResultPrioritiesByRepositoryID.removeValue(forKey: repositoryID) } @@ -855,6 +862,7 @@ extension RepositoriesFeature { state.prRefreshBatchCountsByRepositoryID.removeAll() state.prRefreshResultsByRepositoryID.removeAll() state.prRefreshNoPrBranchesByID.removeAll() + state.prRefreshFailedBatchRepositoryIDs.removeAll() state.prRefreshRemotePrioritiesByRepositoryID.removeAll() state.prRefreshResultPrioritiesByRepositoryID.removeAll() } @@ -884,8 +892,10 @@ extension RepositoriesFeature { if let pullRequest = prsByBranch[worktree.name] { prsByWorktreeID[worktreeID] = pullRequest } else if confirmedNoPrBranches.contains(worktree.name) { - // All repos confirmed no PR for this branch — explicitly clear. - prsByWorktreeID[worktreeID] = nil + // All repos confirmed no PR for this branch — explicitly clear. A nil + // literal through the subscript would remove the key instead of storing + // an explicit nil, so downstream would never see the clear. + prsByWorktreeID.updateValue(nil, forKey: worktreeID) } // Otherwise: unknown status (partial failure) — omit to preserve existing. } diff --git a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift index 8680e9154..b913f5915 100644 --- a/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift +++ b/supacode/Features/Repositories/Reducer/RepositoriesFeature.swift @@ -340,6 +340,10 @@ struct RepositoriesFeature { /// Branches confirmed as having no PR (all repos succeeded, none returned a PR). /// Used to clear stale PR state without flashing when only some repos succeed. var prRefreshNoPrBranchesByID: [Repository.ID: Set] = [:] + /// Repositories with at least one failed host batch in the current refresh cycle. + /// A failure means branch status on that host is unknown, so confirmed-no-PR + /// clears from the healthy hosts must be suppressed regardless of arrival order. + var prRefreshFailedBatchRepositoryIDs: Set = [] /// Cross-host PR refresh batches complete independently; keep the intended remote /// order so same-branch collisions are resolved by priority, not arrival time. var prRefreshRemotePrioritiesByRepositoryID: [Repository.ID: [String: Int]] = [:] diff --git a/supacodeTests/BatchedPullRequestRefreshReducerTests.swift b/supacodeTests/BatchedPullRequestRefreshReducerTests.swift index 8e52d73e6..c957fc823 100644 --- a/supacodeTests/BatchedPullRequestRefreshReducerTests.swift +++ b/supacodeTests/BatchedPullRequestRefreshReducerTests.swift @@ -387,6 +387,242 @@ struct BatchedPullRequestRefreshReducerTests { await store.finish() } + @Test func coordinatorOutcomeConfirmedNoPrClearsStalePullRequest() async { + let context = makeContext() + let stalePullRequest = makePullRequestFixture(url: "https://github.com/khoi/alpha/pull/7") + var initialState = context.state + initialState.inFlightPullRequestRefreshRepositoryIDs = [context.repository.id] + var staleEntry = WorktreeInfoEntry() + staleEntry.pullRequest = stalePullRequest + initialState.worktreeInfoByID[context.featureWorktree.id] = staleEntry + + let store = TestStore(initialState: initialState) { + RepositoriesFeature() + } withDependencies: { + $0.pullRequestRefreshCoordinator = .unimplemented + } + + let outcome = PullRequestRefreshCoordinator.Outcome.refreshed( + repositoryID: context.repository.id, + repositoryRootURL: context.repoRootURL, + worktreeIDs: context.worktreeIDs, + prsByBranch: [:], + confirmedNoPrBranches: ["feature"] + ) + + await store.send(.githubIntegration(.pullRequestRefreshBatchOutcome(outcome))) + await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) { + $0.worktreeInfoByID.removeValue(forKey: context.featureWorktree.id) + } + await store.receive(\.githubIntegration.repositoryPullRequestRefreshCompleted) { + $0.inFlightPullRequestRefreshRepositoryIDs = [] + } + await store.finish() + } + + @Test func coordinatorOutcomeUnknownBranchStatusPreservesPullRequest() async { + let context = makeContext() + let stalePullRequest = makePullRequestFixture(url: "https://github.com/khoi/alpha/pull/7") + var initialState = context.state + initialState.inFlightPullRequestRefreshRepositoryIDs = [context.repository.id] + var staleEntry = WorktreeInfoEntry() + staleEntry.pullRequest = stalePullRequest + initialState.worktreeInfoByID[context.featureWorktree.id] = staleEntry + + let store = TestStore(initialState: initialState) { + RepositoriesFeature() + } withDependencies: { + $0.pullRequestRefreshCoordinator = .unimplemented + } + + let outcome = PullRequestRefreshCoordinator.Outcome.refreshed( + repositoryID: context.repository.id, + repositoryRootURL: context.repoRootURL, + worktreeIDs: context.worktreeIDs, + prsByBranch: [:], + confirmedNoPrBranches: [] + ) + + await store.send(.githubIntegration(.pullRequestRefreshBatchOutcome(outcome))) + await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) + await store.receive(\.githubIntegration.repositoryPullRequestRefreshCompleted) { + $0.inFlightPullRequestRefreshRepositoryIDs = [] + } + await store.finish() + + #expect(store.state.worktreeInfoByID[context.featureWorktree.id]?.pullRequest == stalePullRequest) + } + + @Test func confirmedNoPrClearIsSuppressedWhenAnotherHostBatchFailedFirst() async { + let context = makeContext() + let stalePullRequest = makePullRequestFixture(url: "https://ghe.example/khoi/alpha/pull/9") + var initialState = context.state + initialState.inFlightPullRequestRefreshRepositoryIDs = [context.repository.id] + initialState.prRefreshBatchCountsByRepositoryID[context.repository.id] = 2 + var staleEntry = WorktreeInfoEntry() + staleEntry.pullRequest = stalePullRequest + initialState.worktreeInfoByID[context.featureWorktree.id] = staleEntry + + let store = TestStore(initialState: initialState) { + RepositoriesFeature() + } withDependencies: { + $0.pullRequestRefreshCoordinator = .unimplemented + } + + await store.send( + .githubIntegration( + .pullRequestRefreshBatchOutcome( + .failed( + repositoryID: context.repository.id, + worktreeIDs: context.worktreeIDs, + message: "enterprise host down" + ) + )) + ) { + $0.prRefreshFailedBatchRepositoryIDs = [context.repository.id] + $0.prRefreshBatchCountsByRepositoryID[context.repository.id] = 1 + } + + await store.send( + .githubIntegration( + .pullRequestRefreshBatchOutcome( + .refreshed( + repositoryID: context.repository.id, + repositoryRootURL: context.repoRootURL, + worktreeIDs: context.worktreeIDs, + prsByBranch: [:], + confirmedNoPrBranches: ["feature"] + ) + )) + ) { + $0.prRefreshBatchCountsByRepositoryID = [:] + $0.prRefreshFailedBatchRepositoryIDs = [] + } + await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) + await store.receive(\.githubIntegration.repositoryPullRequestRefreshCompleted) { + $0.inFlightPullRequestRefreshRepositoryIDs = [] + } + await store.finish() + + #expect(store.state.worktreeInfoByID[context.featureWorktree.id]?.pullRequest == stalePullRequest) + } + + @Test func confirmedNoPrClearIsDiscardedWhenFinalHostBatchFails() async { + let context = makeContext() + let stalePullRequest = makePullRequestFixture(url: "https://ghe.example/khoi/alpha/pull/9") + var initialState = context.state + initialState.inFlightPullRequestRefreshRepositoryIDs = [context.repository.id] + initialState.prRefreshBatchCountsByRepositoryID[context.repository.id] = 2 + var staleEntry = WorktreeInfoEntry() + staleEntry.pullRequest = stalePullRequest + initialState.worktreeInfoByID[context.featureWorktree.id] = staleEntry + + let store = TestStore(initialState: initialState) { + RepositoriesFeature() + } withDependencies: { + $0.pullRequestRefreshCoordinator = .unimplemented + } + + await store.send( + .githubIntegration( + .pullRequestRefreshBatchOutcome( + .refreshed( + repositoryID: context.repository.id, + repositoryRootURL: context.repoRootURL, + worktreeIDs: context.worktreeIDs, + prsByBranch: [:], + confirmedNoPrBranches: ["feature"] + ) + )) + ) { + $0.prRefreshBatchCountsByRepositoryID[context.repository.id] = 1 + $0.prRefreshResultsByRepositoryID[context.repository.id] = [:] + $0.prRefreshNoPrBranchesByID[context.repository.id] = ["feature"] + $0.prRefreshResultPrioritiesByRepositoryID[context.repository.id] = [:] + } + + await store.send( + .githubIntegration( + .pullRequestRefreshBatchOutcome( + .failed( + repositoryID: context.repository.id, + worktreeIDs: context.worktreeIDs, + message: "enterprise host down" + ) + )) + ) { + $0.prRefreshBatchCountsByRepositoryID = [:] + $0.prRefreshResultsByRepositoryID = [:] + $0.prRefreshNoPrBranchesByID = [:] + $0.prRefreshResultPrioritiesByRepositoryID = [:] + } + await store.receive(\.githubIntegration.repositoryPullRequestRefreshCompleted) { + $0.inFlightPullRequestRefreshRepositoryIDs = [] + } + await store.finish() + + #expect(store.state.worktreeInfoByID[context.featureWorktree.id]?.pullRequest == stalePullRequest) + } + + @Test func pullRequestFromLaterHostBatchOverridesEarlierConfirmedNoPr() async { + let context = makeContext() + let pullRequest = makePullRequestFixture() + var initialState = context.state + initialState.inFlightPullRequestRefreshRepositoryIDs = [context.repository.id] + initialState.prRefreshBatchCountsByRepositoryID[context.repository.id] = 2 + + let store = TestStore(initialState: initialState) { + RepositoriesFeature() + } withDependencies: { + $0.pullRequestRefreshCoordinator = .unimplemented + } + + await store.send( + .githubIntegration( + .pullRequestRefreshBatchOutcome( + .refreshed( + repositoryID: context.repository.id, + repositoryRootURL: context.repoRootURL, + worktreeIDs: context.worktreeIDs, + prsByBranch: [:], + confirmedNoPrBranches: ["feature"] + ) + )) + ) { + $0.prRefreshBatchCountsByRepositoryID[context.repository.id] = 1 + $0.prRefreshResultsByRepositoryID[context.repository.id] = [:] + $0.prRefreshNoPrBranchesByID[context.repository.id] = ["feature"] + $0.prRefreshResultPrioritiesByRepositoryID[context.repository.id] = [:] + } + + await store.send( + .githubIntegration( + .pullRequestRefreshBatchOutcome( + .refreshed( + repositoryID: context.repository.id, + repositoryRootURL: context.repoRootURL, + worktreeIDs: context.worktreeIDs, + prsByBranch: ["feature": pullRequest], + confirmedNoPrBranches: [] + ) + )) + ) { + $0.prRefreshBatchCountsByRepositoryID = [:] + $0.prRefreshResultsByRepositoryID = [:] + $0.prRefreshNoPrBranchesByID = [:] + $0.prRefreshResultPrioritiesByRepositoryID = [:] + } + await store.receive(\.githubIntegration.repositoryPullRequestsLoaded) { + var entry = WorktreeInfoEntry() + entry.pullRequest = pullRequest + $0.worktreeInfoByID[context.featureWorktree.id] = entry + } + await store.receive(\.githubIntegration.repositoryPullRequestRefreshCompleted) { + $0.inFlightPullRequestRefreshRepositoryIDs = [] + } + await store.finish() + } + @Test(.dependencies) func refreshSkippedWhenPullRequestStateFetchDisabled() async { let context = makeContext() let enqueued = LockIsolated<[PullRequestRefreshCoordinator.Request]>([]) diff --git a/supacodeTests/PullRequestRefreshCoordinatorTests.swift b/supacodeTests/PullRequestRefreshCoordinatorTests.swift index ef13cd6ba..3a786186a 100644 --- a/supacodeTests/PullRequestRefreshCoordinatorTests.swift +++ b/supacodeTests/PullRequestRefreshCoordinatorTests.swift @@ -345,6 +345,90 @@ struct PullRequestRefreshCoordinatorTests { #expect(refreshed.first?["feat-1"]?.title == "PR-upstream") } + @Test func allCandidateReposSucceedingConfirmsBranchesWithoutPullRequests() async throws { + let clock = TestClock() + let probe = CoordinatorProbe() + let outcomes = OutcomeCollector() + let coordinator = makeCoordinator( + probe: probe, + clock: clock, + outcomes: outcomes, + batched: { _, requests in + var dict: [RepoKey: [String: GithubPullRequest]] = [:] + for request in requests { + if request.repo == "upstream" { + dict[request.key] = ["feat-1": makeFixturePullRequest(repo: "upstream")] + } else { + dict[request.key] = [:] + } + } + return CrossRepoPullRequestResult(successByRepo: dict) + } + ) + + coordinator.enqueue(request(repo: "fork", repositoryID: "local", branches: ["feat-1", "feat-2"])) + coordinator.enqueue(request(repo: "upstream", repositoryID: "local", branches: ["feat-1", "feat-2"])) + await advanceCoordinatorClock(clock, by: .milliseconds(250)) + await Task.yield() + await Task.yield() + + let refreshed = await outcomes.snapshot().compactMap { + outcome -> ([String: GithubPullRequest], Set)? in + if case .refreshed("local", _, _, let prsByBranch, let confirmedNoPrBranches) = outcome { + return (prsByBranch, confirmedNoPrBranches) + } + return nil + } + let result = try #require(refreshed.first) + #expect(refreshed.count == 1) + #expect(result.0["feat-1"]?.title == "PR-upstream") + #expect(result.1 == ["feat-2"]) + } + + @Test func partialCandidateRepoFailureLeavesBranchesUnconfirmed() async throws { + let clock = TestClock() + let probe = CoordinatorProbe() + let outcomes = OutcomeCollector() + let coordinator = makeCoordinator( + probe: probe, + clock: clock, + outcomes: outcomes, + batched: { _, requests in + var success: [RepoKey: [String: GithubPullRequest]] = [:] + var failed: [RepoKey: GithubCLIError] = [:] + for request in requests { + if request.repo == "upstream" { + failed[request.key] = .commandFailed("boom") + } else { + success[request.key] = [:] + } + } + return CrossRepoPullRequestResult(successByRepo: success, failedRepos: failed) + }, + legacy: { _, _, _, _ in + throw GithubCLIError.commandFailed("fallback down too") + } + ) + + coordinator.enqueue(request(repo: "fork", repositoryID: "local", branches: ["feat-1", "feat-2"])) + coordinator.enqueue(request(repo: "upstream", repositoryID: "local", branches: ["feat-1", "feat-2"])) + await advanceCoordinatorClock(clock, by: .milliseconds(250)) + await Task.yield() + await Task.yield() + + let refreshed = await outcomes.snapshot().compactMap { + outcome -> ([String: GithubPullRequest], Set)? in + if case .refreshed("local", _, _, let prsByBranch, let confirmedNoPrBranches) = outcome { + return (prsByBranch, confirmedNoPrBranches) + } + return nil + } + let result = try #require(refreshed.first) + #expect(refreshed.count == 1) + #expect(result.0.isEmpty) + #expect(result.1.isEmpty) + } + @Test func duplicateRepoKeysFallbackOnceAndFanOutToEachRepository() async throws { let clock = TestClock() let probe = CoordinatorProbe()