diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 62855a0..1b0db90 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,6 +59,8 @@ jobs: swift test --package-path "$pkg" fi done + echo "==> Testing root package (CrowTests)..." + swift test shellcheck: name: Shell Lint diff --git a/Makefile b/Makefile index 7d91005..053882c 100644 --- a/Makefile +++ b/Makefile @@ -60,13 +60,15 @@ sign: release # --- Test --- -test: +test: $(XCFW) @for pkg in Packages/*/; do \ if [ -d "$$pkg/Tests" ]; then \ echo "==> Testing $$(basename $$pkg)..."; \ swift test --package-path "$$pkg"; \ fi; \ done + @echo "==> Testing root package (CrowTests)..." + @swift test # --- Clean --- diff --git a/Package.swift b/Package.swift index 1a6834b..9d421df 100644 --- a/Package.swift +++ b/Package.swift @@ -56,5 +56,10 @@ let package = Package( ], path: "Sources/CrowCLI" ), + .testTarget( + name: "CrowTests", + dependencies: ["Crow"], + path: "Tests/CrowTests" + ), ] ) diff --git a/Sources/Crow/App/IssueTracker.swift b/Sources/Crow/App/IssueTracker.swift index 37c3b9a..b086c12 100644 --- a/Sources/Crow/App/IssueTracker.swift +++ b/Sources/Crow/App/IssueTracker.swift @@ -233,7 +233,7 @@ final class IssueTracker { let stalePRs = staleCandidateURLs.isEmpty ? [] : await fetchStalePRStates(urls: staleCandidateURLs) - let allKnownPRs = ghResult.viewerPRs + stalePRs + let allKnownPRs = Self.dedupedByURL(ghResult.viewerPRs + stalePRs) applyPRStatuses(viewerPRs: allKnownPRs) @@ -291,7 +291,7 @@ final class IssueTracker { let rateLimit: GitHubRateLimit? } - private struct ViewerPR { + struct ViewerPR: Sendable { let number: Int let url: String let state: String // OPEN / MERGED / CLOSED @@ -306,12 +306,67 @@ final class IssueTracker { let failedCheckNames: [String] let latestReviewStates: [String] - struct LinkedIssue { + struct LinkedIssue: Sendable { let number: Int let repo: String } } + // MARK: - PR Dedup + + /// State-rank precedence used when the same PR URL appears in multiple + /// source lists (viewer vs stale-PR follow-up). Higher rank wins. + nonisolated static func stateRank(_ state: String) -> Int { + switch state { + case "MERGED": return 3 + case "CLOSED": return 2 + case "OPEN": return 1 + default: return 0 + } + } + + /// Merge two `ViewerPR` records for the same URL. The record with the + /// higher state rank wins the state/isDraft/number fields; empty fields + /// on the winner are backfilled from the loser so that (e.g.) an + /// OPEN→MERGED demotion mid-refresh still carries the checks/reviews + /// from the OPEN record (the stale-PR follow-up query leaves those + /// fields empty). + nonisolated static func mergePRRecords(_ lhs: ViewerPR, _ rhs: ViewerPR) -> ViewerPR { + let (winner, loser) = stateRank(lhs.state) >= stateRank(rhs.state) + ? (lhs, rhs) : (rhs, lhs) + return ViewerPR( + number: winner.number, + url: winner.url, + state: winner.state, + mergeable: winner.mergeable != "UNKNOWN" ? winner.mergeable : loser.mergeable, + reviewDecision: winner.reviewDecision.isEmpty ? loser.reviewDecision : winner.reviewDecision, + isDraft: winner.isDraft, + headRefName: winner.headRefName.isEmpty ? loser.headRefName : winner.headRefName, + baseRefName: winner.baseRefName.isEmpty ? loser.baseRefName : winner.baseRefName, + repoNameWithOwner: winner.repoNameWithOwner.isEmpty ? loser.repoNameWithOwner : winner.repoNameWithOwner, + linkedIssueReferences: winner.linkedIssueReferences.isEmpty ? loser.linkedIssueReferences : winner.linkedIssueReferences, + checksState: winner.checksState.isEmpty ? loser.checksState : winner.checksState, + failedCheckNames: winner.failedCheckNames.isEmpty ? loser.failedCheckNames : winner.failedCheckNames, + latestReviewStates: winner.latestReviewStates.isEmpty ? loser.latestReviewStates : winner.latestReviewStates + ) + } + + /// Collapse duplicate URLs using `mergePRRecords`, preserving first-seen + /// order so downstream iteration remains deterministic. + nonisolated static func dedupedByURL(_ prs: [ViewerPR]) -> [ViewerPR] { + var byURL: [String: ViewerPR] = [:] + var order: [String] = [] + for pr in prs { + if let existing = byURL[pr.url] { + byURL[pr.url] = mergePRRecords(existing, pr) + } else { + byURL[pr.url] = pr + order.append(pr.url) + } + } + return order.compactMap { byURL[$0] } + } + private static let consolidatedQuery = """ query($openQuery: String!, $closedQuery: String!, $reviewQuery: String!) { openIssues: search(type: ISSUE, query: $openQuery, first: 100) { @@ -869,7 +924,7 @@ final class IssueTracker { /// in the viewer-PR payload. No extra gh calls. private func applyPRStatuses(viewerPRs: [ViewerPR]) { guard !viewerPRs.isEmpty else { return } - let byURL = Dictionary(uniqueKeysWithValues: viewerPRs.map { ($0.url, $0) }) + let byURL = Dictionary(viewerPRs.map { ($0.url, $0) }, uniquingKeysWith: Self.mergePRRecords) let sessionsWithPRs = appState.sessions.filter { $0.id != AppState.managerSessionID } for session in sessionsWithPRs { @@ -957,7 +1012,7 @@ final class IssueTracker { /// closed accidentally. private func autoCompleteFinishedSessions(openIssues: [AssignedIssue], viewerPRs: [ViewerPR]) { let openIssueURLs = Set(openIssues.map(\.url)) - let prsByURL = Dictionary(uniqueKeysWithValues: viewerPRs.map { ($0.url, $0) }) + let prsByURL = Dictionary(viewerPRs.map { ($0.url, $0) }, uniquingKeysWith: Self.mergePRRecords) let candidateSessions = appState.sessions.filter { $0.id != AppState.managerSessionID && diff --git a/Tests/CrowTests/IssueTrackerDedupTests.swift b/Tests/CrowTests/IssueTrackerDedupTests.swift new file mode 100644 index 0000000..88d818b --- /dev/null +++ b/Tests/CrowTests/IssueTrackerDedupTests.swift @@ -0,0 +1,131 @@ +import Foundation +import Testing +@testable import Crow + +@Suite("IssueTracker PR dedup") +struct IssueTrackerDedupTests { + + private func makeViewerPR( + url: String, + state: String, + number: Int = 1, + mergeable: String = "UNKNOWN", + reviewDecision: String = "", + isDraft: Bool = false, + headRefName: String = "", + baseRefName: String = "", + repoNameWithOwner: String = "", + linkedIssueReferences: [IssueTracker.ViewerPR.LinkedIssue] = [], + checksState: String = "", + failedCheckNames: [String] = [], + latestReviewStates: [String] = [] + ) -> IssueTracker.ViewerPR { + IssueTracker.ViewerPR( + number: number, + url: url, + state: state, + mergeable: mergeable, + reviewDecision: reviewDecision, + isDraft: isDraft, + headRefName: headRefName, + baseRefName: baseRefName, + repoNameWithOwner: repoNameWithOwner, + linkedIssueReferences: linkedIssueReferences, + checksState: checksState, + failedCheckNames: failedCheckNames, + latestReviewStates: latestReviewStates + ) + } + + @Test func collapsesExactDuplicates() { + let url = "https://github.com/radiusmethod/corveil/pull/201" + let prs = [ + makeViewerPR(url: url, state: "OPEN"), + makeViewerPR(url: url, state: "OPEN"), + ] + let deduped = IssueTracker.dedupedByURL(prs) + #expect(deduped.count == 1) + #expect(deduped[0].url == url) + } + + @Test func mergedBeatsOpenRegardlessOfOrder() { + let url = "https://github.com/radiusmethod/corveil/pull/201" + let open = makeViewerPR(url: url, state: "OPEN") + let merged = makeViewerPR(url: url, state: "MERGED") + + let openFirst = IssueTracker.dedupedByURL([open, merged]) + let mergedFirst = IssueTracker.dedupedByURL([merged, open]) + + #expect(openFirst.count == 1) + #expect(openFirst[0].state == "MERGED") + #expect(mergedFirst.count == 1) + #expect(mergedFirst[0].state == "MERGED") + } + + @Test func mergedWinnerBackfillsEmptyFieldsFromOpenLoser() { + let url = "https://github.com/radiusmethod/corveil/pull/201" + let open = makeViewerPR( + url: url, + state: "OPEN", + reviewDecision: "APPROVED", + headRefName: "feature/x", + baseRefName: "main", + repoNameWithOwner: "radiusmethod/corveil", + checksState: "SUCCESS", + failedCheckNames: ["noop"], + latestReviewStates: ["APPROVED"] + ) + let merged = makeViewerPR(url: url, state: "MERGED") + + let result = IssueTracker.dedupedByURL([open, merged]) + #expect(result.count == 1) + let pr = result[0] + #expect(pr.state == "MERGED") + #expect(pr.reviewDecision == "APPROVED") + #expect(pr.headRefName == "feature/x") + #expect(pr.baseRefName == "main") + #expect(pr.repoNameWithOwner == "radiusmethod/corveil") + #expect(pr.checksState == "SUCCESS") + #expect(pr.failedCheckNames == ["noop"]) + #expect(pr.latestReviewStates == ["APPROVED"]) + } + + @Test func mergedWinsThreeWay() { + let url = "https://github.com/radiusmethod/corveil/pull/201" + let prs = [ + makeViewerPR(url: url, state: "OPEN"), + makeViewerPR(url: url, state: "CLOSED"), + makeViewerPR(url: url, state: "MERGED"), + ] + let result = IssueTracker.dedupedByURL(prs) + #expect(result.count == 1) + #expect(result[0].state == "MERGED") + } + + @Test func preservesFirstSeenOrderAcrossDistinctURLs() { + let a = makeViewerPR(url: "https://example/a", state: "OPEN", number: 1) + let b = makeViewerPR(url: "https://example/b", state: "OPEN", number: 2) + let c = makeViewerPR(url: "https://example/c", state: "OPEN", number: 3) + + let result = IssueTracker.dedupedByURL([a, b, c]) + #expect(result.map(\.url) == ["https://example/a", "https://example/b", "https://example/c"]) + } + + @Test func dictionaryUniquingWithMergeRecordsDoesNotTrap() { + // Guard for the belt-and-suspenders call sites at + // applyPRStatuses / autoCompleteFinishedSessions. If a future + // refactor regresses dedup at the assembly site, these call sites + // must still tolerate duplicates instead of trapping. + let url = "https://github.com/radiusmethod/corveil/pull/201" + let prs = [ + makeViewerPR(url: url, state: "OPEN"), + makeViewerPR(url: url, state: "MERGED"), + ] + let byURL = Dictionary( + prs.map { ($0.url, $0) }, + uniquingKeysWith: IssueTracker.mergePRRecords + ) + #expect(byURL.count == 1) + #expect(byURL[url]?.state == "MERGED") + } +}