Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions Packages/CrowCore/Sources/CrowCore/AppState.swift
Original file line number Diff line number Diff line change
Expand Up @@ -497,6 +497,19 @@ public final class AppState {
return reviewRequests.first { $0.url == prLink.url }
}

/// Authoritative lookup for "is there already an active review session
/// for this PR URL?". Cross-references `reviewSessions` (which already
/// excludes completed/archived) against `links` by `.pr` linkType +
/// exact URL match. Used by the kickoff watcher, the review-board
/// buttons, and `SessionService.createReviewSession` as a single source
/// of truth so they don't rely on the lagging `ReviewRequest.reviewSessionID`
/// cross-reference that IssueTracker populates one tick late (CROW-406).
public func existingReviewSession(forPRURL url: String) -> Session? {
reviewSessions.first { session in
links(for: session.id).contains { $0.linkType == .pr && $0.url == url }
}
}

/// Labels for a session, sourced from its linked AssignedIssue or ReviewRequest.
public func labels(forSession session: Session) -> [LabelInfo] {
if let issue = assignedIssue(for: session) {
Expand Down
13 changes: 13 additions & 0 deletions Packages/CrowCore/Sources/CrowCore/Models/Session.swift
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,19 @@ public struct Session: Identifiable, Codable, Sendable {
self.autoMergeEnabledAt = autoMergeEnabledAt
}

/// Parse a GitHub PR URL (`https://github.com/<owner>/<repo>/pull/<number>`)
/// into its components. Returns `nil` if the URL is malformed. Shared by
/// `SessionService.createReviewSession` and the kickoff-dedup helpers on
/// `AppState`; keep here so callers don't reinvent the same parser.
public static func parseReviewPR(url: String) -> (owner: String, repo: String, number: Int)? {
let components = url.split(separator: "/")
guard components.count >= 5,
let number = Int(components.last ?? "") else { return nil }
let owner = String(components[components.count - 4])
let repo = String(components[components.count - 3])
return (owner, repo, number)
}

// Backward-compatible decoding: default `kind`, `agentKind`, and
// `reviewPromptDispatched` when missing from older persisted data.
// `reviewPromptDispatched` defaults to `true` so existing review sessions
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import Foundation
import Testing
@testable import CrowCore

@MainActor
private func makeReviewSession(name: String = "review-crow-406", status: SessionStatus = .active) -> Session {
Session(
name: name,
status: status,
kind: .review,
agentKind: .claudeCode,
provider: .github
)
}

@MainActor
@Test func existingReviewSessionReturnsNilWhenNoSessionMatches() {
let appState = AppState()
let session = makeReviewSession()
appState.sessions = [session]
appState.links[session.id] = [
SessionLink(sessionID: session.id, label: "PR #1", url: "https://github.com/org/other/pull/1", linkType: .pr)
]

#expect(appState.existingReviewSession(forPRURL: "https://github.com/org/repo/pull/406") == nil)
}

@MainActor
@Test func existingReviewSessionReturnsSessionWithMatchingPRLink() {
let appState = AppState()
let session = makeReviewSession()
let prURL = "https://github.com/radiusmethod/crow/pull/406"
appState.sessions = [session]
appState.links[session.id] = [
SessionLink(sessionID: session.id, label: "PR #406", url: prURL, linkType: .pr)
]

#expect(appState.existingReviewSession(forPRURL: prURL)?.id == session.id)
}

@MainActor
@Test func existingReviewSessionIgnoresCompletedSessions() {
let appState = AppState()
let session = makeReviewSession(status: .completed)
let prURL = "https://github.com/radiusmethod/crow/pull/406"
appState.sessions = [session]
appState.links[session.id] = [
SessionLink(sessionID: session.id, label: "PR #406", url: prURL, linkType: .pr)
]

#expect(appState.existingReviewSession(forPRURL: prURL) == nil)
}

@MainActor
@Test func existingReviewSessionIgnoresArchivedSessions() {
let appState = AppState()
let session = makeReviewSession(status: .archived)
let prURL = "https://github.com/radiusmethod/crow/pull/406"
appState.sessions = [session]
appState.links[session.id] = [
SessionLink(sessionID: session.id, label: "PR #406", url: prURL, linkType: .pr)
]

#expect(appState.existingReviewSession(forPRURL: prURL) == nil)
}

@MainActor
@Test func existingReviewSessionIgnoresNonPRLinkTypes() {
let appState = AppState()
let session = makeReviewSession()
let prURL = "https://github.com/radiusmethod/crow/pull/406"
appState.sessions = [session]
// Same URL string but linked as `.repo`, not `.pr` — should not match.
appState.links[session.id] = [
SessionLink(sessionID: session.id, label: "Repo", url: prURL, linkType: .repo)
]

#expect(appState.existingReviewSession(forPRURL: prURL) == nil)
}

@MainActor
@Test func existingReviewSessionReturnsFirstMatchWhenDuplicatesExist() {
let appState = AppState()
let first = makeReviewSession(name: "review-crow-406-a")
let second = makeReviewSession(name: "review-crow-406-b")
let prURL = "https://github.com/radiusmethod/crow/pull/406"
appState.sessions = [first, second]
appState.links[first.id] = [
SessionLink(sessionID: first.id, label: "PR #406", url: prURL, linkType: .pr)
]
appState.links[second.id] = [
SessionLink(sessionID: second.id, label: "PR #406", url: prURL, linkType: .pr)
]

// We don't care which session wins — only that the helper returns
// exactly one of them so callers get a stable, non-nil answer.
let match = appState.existingReviewSession(forPRURL: prURL)
#expect(match != nil)
#expect(match?.id == first.id || match?.id == second.id)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import Foundation
import Testing
@testable import CrowCore

@Test func parseReviewPRExtractsOwnerRepoAndNumber() {
let parsed = Session.parseReviewPR(url: "https://github.com/radiusmethod/crow/pull/406")
#expect(parsed?.owner == "radiusmethod")
#expect(parsed?.repo == "crow")
#expect(parsed?.number == 406)
}

@Test func parseReviewPRReturnsNilForNonIntegerTrailingSegment() {
#expect(Session.parseReviewPR(url: "https://github.com/owner/repo/pull/abc") == nil)
}

@Test func parseReviewPRReturnsNilForTooFewPathComponents() {
#expect(Session.parseReviewPR(url: "github.com/owner/123") == nil)
}

@Test func parseReviewPRReturnsNilForEmptyString() {
#expect(Session.parseReviewPR(url: "") == nil)
}
15 changes: 12 additions & 3 deletions Packages/CrowUI/Sources/CrowUI/ReviewBoardView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,15 @@ public struct ReviewBoardView: View {
.buttonStyle(.plain)

Button {
// Pre-filter PRs that already have an active review session
// so a stale multi-select doesn't enqueue duplicate kickoffs
// (CROW-406). The dedup in createReviewSession would catch
// these too, but filtering here keeps the kickoff queue from
// accumulating no-op work.
let urls = appState.filteredReviewRequests
.filter { selectedRequestIDs.contains($0.id) }
.map(\.url)
.filter { appState.existingReviewSession(forPRURL: $0) == nil }
appState.onBatchStartReview?(urls)
selectedRequestIDs.removeAll()
isSelectionMode = false
Expand Down Expand Up @@ -306,10 +312,13 @@ struct ReviewRow: View {
private var reviewAction: some View {
if isSelectionMode {
EmptyView()
} else if let sessionID = request.reviewSessionID,
appState.sessions.contains(where: { $0.id == sessionID }) {
} else if let existing = appState.existingReviewSession(forPRURL: request.url) {
// Look up the session directly from `appState` instead of the
// lagging `request.reviewSessionID` so the button flips to
// "Go to Session" the instant the session row is appended,
// not on the next IssueTracker refresh (CROW-406).
Button {
appState.selectedSessionID = sessionID
appState.selectedSessionID = existing.id
} label: {
HStack(spacing: 4) {
Image(systemName: "arrow.right.circle")
Expand Down
9 changes: 8 additions & 1 deletion Sources/Crow/App/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,14 @@ final class AppDelegate: NSObject, NSApplicationDelegate {
let shaAdvanced = linkedSession != nil
&& request.headRefOid != nil
&& linkedSession?.lastReviewedHeadSha != request.headRefOid
guard request.reviewSessionID == nil || shaAdvanced else { continue }
// Authoritative `appState`-side check: a prior tick during an
// in-flight kickoff may have already created a session even
// though `request.reviewSessionID` hasn't been repopulated by
// the next IssueTracker refresh yet (CROW-406). Without this,
// a watcher tick during the ~10s clone window enqueues a
// duplicate kickoff for the same PR.
let existingByPR = self.appState.existingReviewSession(forPRURL: request.url)
guard (request.reviewSessionID == nil && existingByPR == nil) || shaAdvanced else { continue }

// B-fallback: tear down the stale round-1 session so the new
// session doesn't double up in `reviewSessions` for the same
Expand Down
21 changes: 16 additions & 5 deletions Sources/Crow/App/SessionService.swift
Original file line number Diff line number Diff line change
Expand Up @@ -1246,15 +1246,26 @@ final class SessionService {
/// what produced the SwiftUI reentrant-layout crash in #266.
@discardableResult
func createReviewSession(prURL: String, selectAfterCreate: Bool = false) async -> UUID? {
// Universal backstop against duplicate sessions for the same PR. The
// serial kickoff queue in AppDelegate guarantees that by the time a
// second `createReviewSession` runs, the first has already appended
// its row to `appState.sessions` — so this check is authoritative,
// not racy. Belt-and-suspenders for the auto-review watcher race
// (CROW-406) and any future caller that re-enters during a kickoff.
if let existing = appState.existingReviewSession(forPRURL: prURL) {
NSLog("[SessionService] Skipping duplicate review session for \(prURL); reusing \(existing.id)")
if selectAfterCreate { appState.selectedSessionID = existing.id }
return existing.id
}

// Parse org/repo and PR number from URL like "https://github.com/org/repo/pull/123"
let components = prURL.split(separator: "/")
guard components.count >= 5,
let prNumber = Int(components.last ?? "") else {
guard let parsed = Session.parseReviewPR(url: prURL) else {
NSLog("[SessionService] Could not parse PR URL: \(prURL)")
return nil
}
let owner = String(components[components.count - 4])
let repoName = String(components[components.count - 3])
let owner = parsed.owner
let repoName = parsed.repo
let prNumber = parsed.number
let repoSlug = "\(owner)/\(repoName)"

// Determine clone path
Expand Down
Loading