diff --git a/Tests/StackNudgePanelCoreTests/GitHubAPITests.swift b/Tests/StackNudgePanelCoreTests/GitHubAPITests.swift index 1ffc5dd..f6318e1 100644 --- a/Tests/StackNudgePanelCoreTests/GitHubAPITests.swift +++ b/Tests/StackNudgePanelCoreTests/GitHubAPITests.swift @@ -28,54 +28,158 @@ final class GitHubAPITests: XCTestCase { XCTAssertNil(GitHubAPI.repoSlug(fromRemoteURL: "https://gitlab.com/o/r.git")) } - func test_query_carriesVariables() { - let actual = GitHubAPI.query(owner: "o", repo: "r", branch: "ENG-1/x") + func test_batchQuery_carriesEachBranchAsItsOwnVariable() { + let actual = GitHubAPI.batchQuery(owner: "o", repo: "r", branches: ["ENG-1/x", "ENG-2/y"]) XCTAssertTrue(actual.contains("headRefName")) - XCTAssertTrue(actual.contains("\"branch\":\"ENG-1\\/x\"") || actual.contains("\"branch\":\"ENG-1/x\"")) XCTAssertTrue(actual.contains("statusCheckRollup")) + // One alias + one declared variable per branch. + XCTAssertTrue(actual.contains("b0:pullRequests(headRefName:$b0")) + XCTAssertTrue(actual.contains("b1:pullRequests(headRefName:$b1")) + XCTAssertTrue(actual.contains("$b0:String!")) + XCTAssertTrue(actual.contains("$b1:String!")) + // Branch names travel as variables, never interpolated into the query, so + // a name that isn't a legal GraphQL alias (or contains a quote) is safe. + XCTAssertTrue(actual.contains(#""b0":"ENG-1\/x""#) || actual.contains(#""b0":"ENG-1/x""#)) + XCTAssertTrue(actual.contains(#""b1":"ENG-2\/y""#) || actual.contains(#""b1":"ENG-2/y""#)) + XCTAssertFalse(actual.contains("b2:")) } - private func response(state: String, isDraft: Bool = false, ci: String?) -> String { + func test_branchChunks_splitsAtTheBatchCeiling_preservingOrder() { + let branches = (0..<(GitHubAPI.maxBranchesPerQuery + 3)).map { "b/\($0)" } + let chunks = GitHubAPI.branchChunks(branches) + XCTAssertEqual(chunks.count, 2) + XCTAssertEqual(chunks.first?.count, GitHubAPI.maxBranchesPerQuery) + XCTAssertEqual(chunks.last?.count, 3) + XCTAssertEqual(chunks.flatMap { $0 }, branches) + } + + func test_branchChunks_emptyAndExactMultiple() { + XCTAssertEqual(GitHubAPI.branchChunks([]).count, 0) + let exact = (0.. String { let rollup = ci.map { "{\"state\":\"\($0)\"}" } ?? "null" return """ - {"data":{"repository":{"pullRequests":{"nodes":[{ - "number":86,"url":"https://github.com/o/r/pull/86","state":"\(state)","isDraft":\(isDraft), - "commits":{"nodes":[{"commit":{"statusCheckRollup":\(rollup)}}]}}]}}}} + {"nodes":[{"number":\(number),"url":"https://github.com/o/r/pull/\(number)", + "state":"\(state)","isDraft":\(isDraft), + "commits":{"nodes":[{"commit":{"statusCheckRollup":\(rollup)}}]}}]} """ } + private func response(state: String, isDraft: Bool = false, ci: String?) -> String { + #"{"data":{"repository":{"b0":"# + node(state: state, isDraft: isDraft, ci: ci) + "}}}" + } + + // Single-branch parse, which is what most cases below exercise. + private func parseOne(_ json: String) -> PullRequestInfo? { + GitHubAPI.parse(json, branches: ["b"])["b"] + } + func test_parse_openWithPassingCI() { - let actual = GitHubAPI.parse(response(state: "OPEN", ci: "SUCCESS")) + let actual = parseOne(response(state: "OPEN", ci: "SUCCESS")) XCTAssertEqual(actual, PullRequestInfo(number: 86, url: "https://github.com/o/r/pull/86", state: .open, isDraft: false, ci: .passing)) } func test_parse_mergedNoCI() { - let actual = GitHubAPI.parse(response(state: "MERGED", ci: nil)) + let actual = parseOne(response(state: "MERGED", ci: nil)) XCTAssertEqual(actual?.state, .merged) XCTAssertNil(actual?.ci) } func test_parse_draftPendingCI() { - let actual = GitHubAPI.parse(response(state: "OPEN", isDraft: true, ci: "PENDING")) + let actual = parseOne(response(state: "OPEN", isDraft: true, ci: "PENDING")) XCTAssertEqual(actual?.isDraft, true) XCTAssertEqual(actual?.ci, .pending) } func test_parse_failingCI() { - XCTAssertEqual(GitHubAPI.parse(response(state: "OPEN", ci: "FAILURE"))?.ci, .failing) + XCTAssertEqual(parseOne(response(state: "OPEN", ci: "FAILURE"))?.ci, .failing) } func test_parse_noPRNodes_isNil() { - XCTAssertNil(GitHubAPI.parse(#"{"data":{"repository":{"pullRequests":{"nodes":[]}}}}"#)) + XCTAssertNil(parseOne(#"{"data":{"repository":{"b0":{"nodes":[]}}}}"#)) } func test_parse_malformed_isNil() { - XCTAssertNil(GitHubAPI.parse("not json")) - XCTAssertNil(GitHubAPI.parse(#"{"data":{"repository":null}}"#)) + XCTAssertNil(parseOne("not json")) + XCTAssertNil(parseOne(#"{"data":{"repository":null}}"#)) + } + + // GraphQL reports failures as HTTP 200 with an errors array. A wholesale + // failure nulls `repository`, so nothing is read as "no PR found". + func test_parse_graphQLErrors_yieldsNothing() { + let json = #"{"data":null,"errors":[{"message":"Bad credentials"}]}"# + XCTAssertEqual(GitHubAPI.parse(json, branches: ["a", "b"]).count, 0) + } + + // A per-field error must not cost the rest of the batch its results. GraphQL + // can error on one alias and still return data for the others, and one batch + // stands in for up to maxBranchesPerQuery separate queries. + func test_parse_partialResponse_keepsTheBranchesThatResolved() { + let json = #"{"data":{"repository":{"b0":"# + + node(number: 5, state: "OPEN", ci: nil) + + #","b1":null}},"errors":[{"message":"Something went wrong"}]}"# + let actual = GitHubAPI.parse(json, branches: ["good", "bad"]) + XCTAssertEqual(actual.count, 1) + XCTAssertEqual(actual["good"]?.number, 5) + XCTAssertNil(actual["bad"]) + } + + // The aliases are positional, so a branch keeps its own PR and a branch with + // no PR drops out without shifting the others. + func test_parse_mapsEachAliasBackToItsBranch() { + let json = #"{"data":{"repository":{"b0":"# + + node(number: 11, state: "OPEN", ci: "SUCCESS") + + #","b1":{"nodes":[]},"b2":"# + + node(number: 33, state: "MERGED", ci: nil) + "}}}" + let actual = GitHubAPI.parse(json, branches: ["first", "second", "third"]) + XCTAssertEqual(actual.count, 2) + XCTAssertEqual(actual["first"]?.number, 11) + XCTAssertEqual(actual["first"]?.ci, .passing) + XCTAssertNil(actual["second"]) + XCTAssertEqual(actual["third"]?.number, 33) + XCTAssertEqual(actual["third"]?.state, .merged) + } + + func test_pullRequests_nilRunYieldsEmpty() { + XCTAssertEqual(GitHubAPI.pullRequests(owner: "o", repo: "r", branches: ["b"]) { _ in nil }.count, 0) + } + + func test_pullRequests_emptyBranchesIssuesNoRequest() { + var calls = 0 + let actual = GitHubAPI.pullRequests(owner: "o", repo: "r", branches: []) { _ in + calls += 1 + return nil + } + XCTAssertEqual(calls, 0) + XCTAssertEqual(actual.count, 0) + } + + // One request per batch, not one per branch: that ratio is the whole point of + // the batched query. + func test_pullRequests_batchesRequests() { + let branches = (0..<(GitHubAPI.maxBranchesPerQuery + 1)).map { "b/\($0)" } + var calls = 0 + _ = GitHubAPI.pullRequests(owner: "o", repo: "r", branches: branches) { _ in + calls += 1 + return nil + } + XCTAssertEqual(calls, 2) } - func test_pullRequest_nilRunYieldsNil() { - XCTAssertNil(GitHubAPI.pullRequest(owner: "o", repo: "r", branch: "b") { _ in nil }) + // A failed chunk loses only its own branches; later chunks still resolve. + func test_pullRequests_oneFailedChunkDoesNotLoseTheRest() { + let branches = (0..<(GitHubAPI.maxBranchesPerQuery + 1)).map { "b/\($0)" } + var calls = 0 + let actual = GitHubAPI.pullRequests(owner: "o", repo: "r", branches: branches) { _ in + calls += 1 + guard calls > 1 else { return nil } // first chunk fails + return #"{"data":{"repository":{"b0":"# + node(number: 7, state: "OPEN", ci: nil) + "}}}" + } + XCTAssertEqual(actual.count, 1) + XCTAssertEqual(actual["b/\(GitHubAPI.maxBranchesPerQuery)"]?.number, 7) } } diff --git a/Tests/StackNudgePanelCoreTests/OutcomeWatcherTests.swift b/Tests/StackNudgePanelCoreTests/OutcomeWatcherTests.swift index 0f3cd28..651085c 100644 --- a/Tests/StackNudgePanelCoreTests/OutcomeWatcherTests.swift +++ b/Tests/StackNudgePanelCoreTests/OutcomeWatcherTests.swift @@ -2,99 +2,235 @@ import XCTest @testable import StackNudgePanelCore -// OutcomeWatcher derives "did it ship?" from current git state via an injected -// runner. These drive each branch through the precedence ladder -// (merged > pushed > committed > needsReview > clean) with a stub git that -// answers rev-parse / merge-base for a small fake commit graph. +// OutcomeWatcher derives "did it ship?" from current git state: branch tips come +// from a RepoRefs loaded once per repo, ancestry from an injected runner. These +// drive each branch through the precedence ladder +// (merged > pushed > committed > needsReview > clean) over a small fake graph. final class OutcomeWatcherTests: XCTestCase { - // A tiny linear graph: base ← b1 ← b2. `refs` maps a ref to its tip sha; - // `ancestors[x]` is the set of shas reachable from x (x included), so - // merge-base(a, b) = a when a ∈ ancestors[b]. - private func stubGit(refs: [String: String], - ancestors: [String: Set]) -> ([String]) -> String? { + // A tiny linear graph: base ← b1 ← b2. `ancestors[x]` is the set of shas + // reachable from x (x included), so merge-base(a, b) = a when a ∈ ancestors[b]. + private func stubGit(ancestors: [String: Set]) -> ([String]) -> String? { { args in - switch args.first { - case "rev-parse": - // ["rev-parse", "--verify", "--quiet", ] - guard let ref = args.last else { return nil } - return refs[ref] - case "merge-base": - // ["merge-base", , ] → the one that's an ancestor of the other - let a = args[1], b = args[2] - if ancestors[b]?.contains(a) == true { return a } - if ancestors[a]?.contains(b) == true { return b } - return nil - default: - return nil - } + guard args.first == "merge-base" else { return nil } + // ["merge-base", , ] → the one that's an ancestor of the other + let a = args[1], b = args[2] + if ancestors[b]?.contains(a) == true { return a } + if ancestors[a]?.contains(b) == true { return b } + return nil } } + private func refs(_ shaByRef: [String: String]) -> OutcomeWatcher.RepoRefs { + OutcomeWatcher.RepoRefs(shaByRef: shaByRef) + } + func test_merged_branchTipReachableFromBase() { // base is at b2, branch tip b1 → b1 is an ancestor of base → merged. - let git = stubGit( - refs: ["origin/main": "b2", "ENG-1/x": "b1"], - ancestors: ["b2": ["base", "b1", "b2"], "b1": ["base", "b1"]]) - XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", headCommit: "b1", + let git = stubGit(ancestors: ["b2": ["base", "b1", "b2"], "b1": ["base", "b1"]]) + let repo = refs(["origin/main": "b2", "ENG-1/x": "b1"]) + XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", refs: repo, headCommit: "b1", filesChangedAtStop: 0, git: git), .merged) } func test_pushed_remoteHasAllLocalCommits() { // Not in base; remote branch tip == local tip → pushed. - let git = stubGit( - refs: ["origin/main": "base", "ENG-1/x": "b2", "origin/ENG-1/x": "b2"], - ancestors: ["base": ["base"], "b2": ["base", "b1", "b2"]]) - XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", headCommit: "b2", + let git = stubGit(ancestors: ["base": ["base"], "b2": ["base", "b1", "b2"]]) + let repo = refs(["origin/main": "base", "ENG-1/x": "b2", "origin/ENG-1/x": "b2"]) + XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", refs: repo, headCommit: "b2", filesChangedAtStop: 0, git: git), .pushed) } func test_committed_localAheadOfRemote() { // Remote at b1, local at b2 (ahead) → committed (unpushed commits). - let git = stubGit( - refs: ["origin/main": "base", "ENG-1/x": "b2", "origin/ENG-1/x": "b1"], - ancestors: ["base": ["base"], "b1": ["base", "b1"], "b2": ["base", "b1", "b2"]]) - XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", headCommit: "b2", + let git = stubGit(ancestors: ["base": ["base"], "b1": ["base", "b1"], "b2": ["base", "b1", "b2"]]) + let repo = refs(["origin/main": "base", "ENG-1/x": "b2", "origin/ENG-1/x": "b1"]) + XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", refs: repo, headCommit: "b2", filesChangedAtStop: 0, git: git), .committed) } func test_committed_noRemoteButCommitsBeyondBase() { // No remote branch; branch has commits on top of base → committed. - let git = stubGit( - refs: ["origin/main": "base", "ENG-1/x": "b1"], - ancestors: ["base": ["base"], "b1": ["base", "b1"]]) - XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", headCommit: "b1", + let git = stubGit(ancestors: ["base": ["base"], "b1": ["base", "b1"]]) + let repo = refs(["origin/main": "base", "ENG-1/x": "b1"]) + XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", refs: repo, headCommit: "b1", filesChangedAtStop: 0, git: git), .committed) } func test_needsReview_uncommittedWorkNoCommitsSinceStop() { // Branch tip == base (no new commits), no remote, dirty work at Stop. - let git = stubGit( - refs: ["origin/main": "base", "ENG-1/x": "base"], - ancestors: ["base": ["base"]]) - XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", headCommit: "base", + let git = stubGit(ancestors: ["base": ["base"]]) + let repo = refs(["origin/main": "base", "ENG-1/x": "base"]) + XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", refs: repo, headCommit: "base", filesChangedAtStop: 7, git: git), .needsReview) } func test_clean_noPendingWorkOnBase() { - let git = stubGit( - refs: ["origin/main": "base", "ENG-1/x": "base"], - ancestors: ["base": ["base"]]) - XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", headCommit: "base", + let git = stubGit(ancestors: ["base": ["base"]]) + let repo = refs(["origin/main": "base", "ENG-1/x": "base"]) + XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", refs: repo, headCommit: "base", filesChangedAtStop: 0, git: git), .clean) } func test_deletedBranch_mergedIfHeadInBase() { // Branch ref gone; recorded head is an ancestor of base → merged. - let git = stubGit( - refs: ["origin/main": "b2"], - ancestors: ["b2": ["base", "b1", "b2"]]) - XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", headCommit: "b1", + let git = stubGit(ancestors: ["b2": ["base", "b1", "b2"]]) + let repo = refs(["origin/main": "b2"]) + XCTAssertEqual(OutcomeWatcher.derive(branch: "ENG-1/x", refs: repo, headCommit: "b1", filesChangedAtStop: 0, git: git), .merged) } func test_nilBranch_isClean() { - XCTAssertEqual(OutcomeWatcher.derive(branch: nil, headCommit: nil, + XCTAssertEqual(OutcomeWatcher.derive(branch: nil, refs: refs([:]), headCommit: nil, filesChangedAtStop: 5) { _ in nil }, .clean) } + + // MARK: - RepoRefs (bulk ref loading) + + private func showRefGit(_ output: String) -> ([String]) -> String? { + { args in args.first == "show-ref" ? output : nil } + } + + func test_loadRefs_parsesHeadsAndRemotes() { + let repo = OutcomeWatcher.loadRefs(git: showRefGit(""" + aaa refs/heads/main + bbb refs/heads/ENG-1/x + aaa refs/remotes/origin/main + ccc refs/remotes/origin/ENG-1/x + ddd refs/remotes/upstream/main + """)) + XCTAssertEqual(repo.tip("main"), "aaa") + XCTAssertEqual(repo.tip("ENG-1/x"), "bbb") + XCTAssertEqual(repo.remoteTip("ENG-1/x"), "ccc") + XCTAssertEqual(repo.shaByRef["upstream/main"], "ddd") + XCTAssertNil(repo.remoteTip("nope")) + // tip() is a plain ref lookup, mirroring the `rev-parse ` it replaced, + // so a remote ref resolves through it too (as it does in real git). The + // ladder simply never asks for one that way; it goes via remoteTip. + XCTAssertEqual(repo.tip("origin/ENG-1/x"), "ccc") + XCTAssertNil(repo.remoteTip("origin/ENG-1/x")) + } + + // A detached session records its branch as literally "HEAD", so HEAD has to be + // in the map or those rows would read as deleted branches and change status. + func test_loadRefs_includesHEAD() { + let repo = OutcomeWatcher.loadRefs(git: showRefGit(""" + detached refs/heads/main + zzz HEAD + """)) + XCTAssertEqual(repo.tip("HEAD"), "zzz") + } + + // Refs the ladder never consults must not enter the map, so a tag can't + // shadow a branch of the same name. + func test_loadRefs_ignoresTagsAndOtherRemotes() { + let repo = OutcomeWatcher.loadRefs(git: showRefGit(""" + aaa refs/heads/release + bbb refs/tags/release + ccc refs/remotes/fork/release + ddd refs/notes/commits + """)) + XCTAssertEqual(repo.tip("release"), "aaa") + XCTAssertEqual(repo.shaByRef.count, 1) + } + + // Branch names contain slashes, and the sha/ref split must stop at the first + // space or a name would be truncated. + func test_loadRefs_branchNameWithSlashesIsNotTruncated() { + let repo = OutcomeWatcher.loadRefs(git: showRefGit("ddd refs/heads/feat/a/b/c")) + XCTAssertEqual(repo.tip("feat/a/b/c"), "ddd") + } + + func test_loadRefs_skipsMalformedLines() { + let repo = OutcomeWatcher.loadRefs(git: showRefGit(""" + onlyonefield + + aaa refs/heads/good + bbb refs/heads/ + """)) + XCTAssertEqual(repo.tip("good"), "aaa") + XCTAssertEqual(repo.shaByRef.count, 1) + } + + func test_loadRefs_noGitOutput_isEmptyWithNoBase() { + let repo = OutcomeWatcher.loadRefs { _ in nil } + XCTAssertNil(repo.baseSha) + XCTAssertNil(repo.tip("main")) + } + + // Base precedence: local default first, then the fork's, then upstream's. + func test_baseSha_prefersLocalDefaultThenOriginThenUpstream() { + XCTAssertEqual(refs(["main": "a", "origin/main": "b", "upstream/main": "c"]).baseSha, "a") + XCTAssertEqual(refs(["origin/main": "b", "upstream/main": "c"]).baseSha, "b") + XCTAssertEqual(refs(["upstream/main": "c"]).baseSha, "c") + XCTAssertEqual(refs(["master": "m", "origin/main": "b"]).baseSha, "m") + XCTAssertNil(refs(["ENG-1/x": "z"]).baseSha) + } + + // The spawning path (used once per Stop) and the preloaded path must agree on + // which ref is the base, or a handoff's ticket and its chip could disagree. + func test_resolveBaseSha_matchesRepoRefs() { + let shaByRef = ["master": "m", "origin/main": "b", "upstream/master": "u"] + let spawning = OutcomeWatcher.resolveBaseSha { args in + args.first == "rev-parse" ? args.last.flatMap { shaByRef[$0] } : nil + } + XCTAssertEqual(spawning, refs(shaByRef).baseSha) + } + + // Equal local and remote tips prove the branch is pushed, so the ladder must + // answer without asking git anything. + func test_pushed_equalTips_asksGitNothing() { + var calls: [[String]] = [] + let repo = refs(["origin/main": "base", "ENG-1/x": "b2", "origin/ENG-1/x": "b2"]) + let actual = OutcomeWatcher.derive( + branch: "ENG-1/x", refs: repo, headCommit: "b2", filesChangedAtStop: 0 + ) { args in + calls.append(args) + // Answer the merged check truthfully (b2 is not in base). + return args.first == "merge-base" ? "base" : nil + } + XCTAssertEqual(actual, .pushed) + // Only the merged check ran; no second merge-base for the push comparison. + XCTAssertEqual(calls.count, 1) + } + + // MARK: - Cache inputs + + // The cache is only sound if these capture everything the ladder reads. + func test_inputs_captureEveryValueTheLadderReads() { + let repo = refs(["main": "base", "ENG-1/x": "tip", "origin/ENG-1/x": "remote"]) + let actual = OutcomeWatcher.inputs(branch: "ENG-1/x", refs: repo, + headCommit: "head", filesChangedAtStop: 3) + XCTAssertEqual(actual, OutcomeInputs(branchTip: "tip", remoteTip: "remote", + baseSha: "base", headCommit: "head", + filesChanged: 3)) + } + + func test_inputs_differWhenAnyGitValueMoves() { + let base = refs(["main": "base", "ENG-1/x": "tip", "origin/ENG-1/x": "remote"]) + let reference = OutcomeWatcher.inputs(branch: "ENG-1/x", refs: base, + headCommit: "head", filesChangedAtStop: 3) + // A commit (tip moves), a push (remote moves), a pull (base moves), a new + // Stop (head or dirty count changes) must each invalidate. + let moved = [ + OutcomeWatcher.inputs(branch: "ENG-1/x", + refs: refs(["main": "base", "ENG-1/x": "tip2", "origin/ENG-1/x": "remote"]), + headCommit: "head", filesChangedAtStop: 3), + OutcomeWatcher.inputs(branch: "ENG-1/x", + refs: refs(["main": "base", "ENG-1/x": "tip", "origin/ENG-1/x": "remote2"]), + headCommit: "head", filesChangedAtStop: 3), + OutcomeWatcher.inputs(branch: "ENG-1/x", + refs: refs(["main": "base2", "ENG-1/x": "tip", "origin/ENG-1/x": "remote"]), + headCommit: "head", filesChangedAtStop: 3), + OutcomeWatcher.inputs(branch: "ENG-1/x", refs: base, + headCommit: "head2", filesChangedAtStop: 3), + OutcomeWatcher.inputs(branch: "ENG-1/x", refs: base, + headCommit: "head", filesChangedAtStop: 0), + ] + for candidate in moved { XCTAssertNotEqual(candidate, reference) } + // A branch deleted since the last pass drops its tip, so it invalidates too. + XCTAssertNotEqual(OutcomeWatcher.inputs(branch: "ENG-1/x", refs: refs(["main": "base"]), + headCommit: "head", filesChangedAtStop: 3), + reference) + } } diff --git a/Tests/StackNudgePanelCoreTests/OutcomesViewTests.swift b/Tests/StackNudgePanelCoreTests/OutcomesViewTests.swift index be5ca78..5ece159 100644 --- a/Tests/StackNudgePanelCoreTests/OutcomesViewTests.swift +++ b/Tests/StackNudgePanelCoreTests/OutcomesViewTests.swift @@ -206,4 +206,59 @@ final class OutcomesViewTests: XCTestCase { let groups = OutcomesView.groups(from: [record(id: "a", ticket: "ENG-1")]) XCTAssertEqual(groups.first?.diff.isEmpty, true) } + + // MARK: - selectedRow (keyboard selection to scroll anchor) + + // Two groups, the first with two branches, so the flat row order is + // header(ENG-1), ENG-1/a, ENG-1/b, header(ENG-2), ENG-2/a. + private func twoGroups() -> [TicketGroup] { + OutcomesView.groups(from: [ + record(id: "a", branch: "ENG-1/a", ticket: "ENG-1", tokens: 200, updated: 2), + record(id: "b", branch: "ENG-1/b", ticket: "ENG-1", tokens: 100, updated: 2), + record(id: "c", branch: "ENG-2/a", ticket: "ENG-2", tokens: 50, updated: 1), + ]) + } + + func test_selectedRow_walksHeadersThenTheirBranches() { + let groups = twoGroups() + let ids = (0..<5).map { OutcomesView.selectedRow(groups, index: $0)?.rowID } + XCTAssertEqual(ids, [ + "g:t:ENG-1", + "b:" + PanelNav.outcomeKey("/work/stack-nudge", "ENG-1/a"), + "b:" + PanelNav.outcomeKey("/work/stack-nudge", "ENG-1/b"), + "g:t:ENG-2", + "b:" + PanelNav.outcomeKey("/work/stack-nudge", "ENG-2/a"), + ]) + } + + func test_selectedRow_clampsOutOfRangeToFirstAndLastRow() { + let groups = twoGroups() + XCTAssertEqual(OutcomesView.selectedRow(groups, index: -3)?.rowID, "g:t:ENG-1") + XCTAssertEqual(OutcomesView.selectedRow(groups, index: 99)?.rowID, + "b:" + PanelNav.outcomeKey("/work/stack-nudge", "ENG-2/a")) + } + + // The group anchor is what the lazy scroll can resolve before a group has + // been built, so a branch sub-row must report its *own* group, not the first. + func test_selectedRow_groupAnchorIsTheContainingGroup() { + let groups = twoGroups() + XCTAssertEqual(OutcomesView.selectedRow(groups, index: 0)?.groupAnchor, "ga:t:ENG-1") + XCTAssertEqual(OutcomesView.selectedRow(groups, index: 2)?.groupAnchor, "ga:t:ENG-1") + XCTAssertEqual(OutcomesView.selectedRow(groups, index: 3)?.groupAnchor, "ga:t:ENG-2") + XCTAssertEqual(OutcomesView.selectedRow(groups, index: 4)?.groupAnchor, "ga:t:ENG-2") + XCTAssertEqual(OutcomesView.selectedRow(groups, index: 99)?.groupAnchor, "ga:t:ENG-2") + } + + func test_selectedRow_groupWithNoBranches_clampsToItsHeader() { + let group = TicketGroup(id: "t:ENG-1", label: "ENG-1", kind: .ticket, repos: [], + sessionCount: 1, totalTokens: 0, agents: [], + diff: DiffStat(filesChanged: 0, insertions: 0, deletions: 0), + branches: []) + XCTAssertEqual(OutcomesView.selectedRow([group], index: 0)?.rowID, "g:t:ENG-1") + XCTAssertEqual(OutcomesView.selectedRow([group], index: 7)?.rowID, "g:t:ENG-1") + } + + func test_selectedRow_nilWhenNothingToSelect() { + XCTAssertNil(OutcomesView.selectedRow([], index: 0)) + } } diff --git a/Tests/StackNudgePanelCoreTests/RefreshGateTests.swift b/Tests/StackNudgePanelCoreTests/RefreshGateTests.swift new file mode 100644 index 0000000..1a7045d --- /dev/null +++ b/Tests/StackNudgePanelCoreTests/RefreshGateTests.swift @@ -0,0 +1,128 @@ +import XCTest + +@testable import StackNudgePanelCore + +// RefreshGate rate-limits the Tickets tab's two refreshes. The contract that +// matters: a request inside the window is deferred rather than dropped (so the +// caller never has to know whether its data made the cut), repeated requests +// inside one window collapse into a single run, and force bypasses entirely. +// Driven by an injected clock so nothing here waits on a real timer. +final class RefreshGateTests: XCTestCase { + + // Captures scheduled work instead of dispatching it, so a test can fire it + // at the point it chooses. + private final class Clock { + var now = Date(timeIntervalSince1970: 1_000) + var scheduled: [(delay: TimeInterval, block: () -> Void)] = [] + + func advance(_ seconds: TimeInterval) { now.addTimeInterval(seconds) } + + // Run the pending block as if its delay had elapsed. + func fire() { + guard let next = scheduled.first else { return XCTFail("nothing scheduled") } + scheduled.removeFirst() + advance(next.delay) + next.block() + } + } + + private func gate(interval: TimeInterval = 60) -> (RefreshGate, Clock, () -> Int) { + let clock = Clock() + var runs = 0 + let gate = RefreshGate( + interval: interval, + now: { clock.now }, + after: { delay, block in clock.scheduled.append((delay, block)) }, + work: { runs += 1 }) + return (gate, clock, { runs }) + } + + func test_coldGate_runsImmediately() { + let (gate, clock, runs) = self.gate() + gate.request() + XCTAssertEqual(runs(), 1) + XCTAssertTrue(clock.scheduled.isEmpty) + } + + func test_requestAfterWindowElapsed_runsImmediately() { + let (gate, clock, runs) = self.gate(interval: 60) + gate.request() + clock.advance(60) + gate.request() + XCTAssertEqual(runs(), 2) + XCTAssertTrue(clock.scheduled.isEmpty) + } + + // The point of the gate: a second visit to the tab inside the window doesn't + // re-pay the fetch. + func test_requestInsideWindow_doesNotRunYet() { + let (gate, clock, runs) = self.gate(interval: 60) + gate.request() + clock.advance(10) + gate.request() + XCTAssertEqual(runs(), 1) + XCTAssertEqual(clock.scheduled.count, 1) + } + + // Deferred, not dropped: the request is still served when the window closes. + func test_deferredRequest_runsWhenWindowCloses() { + let (gate, clock, runs) = self.gate(interval: 60) + gate.request() + clock.advance(10) + gate.request() + XCTAssertEqual(clock.scheduled.first?.delay, 50) // remainder of the window + clock.fire() + XCTAssertEqual(runs(), 2) + } + + // A burst (several Stops, or flipping tabs repeatedly) costs one extra run. + func test_manyRequestsInsideWindow_collapseToOneRun() { + let (gate, clock, runs) = self.gate(interval: 60) + gate.request() + for _ in 0..<20 { + clock.advance(1) + gate.request() + } + XCTAssertEqual(runs(), 1) + XCTAssertEqual(clock.scheduled.count, 1) + clock.fire() + XCTAssertEqual(runs(), 2) + XCTAssertTrue(clock.scheduled.isEmpty) + } + + // After a deferred run completes, the gate is armed again rather than stuck. + func test_gateRearmsAfterDeferredRun() { + let (gate, clock, runs) = self.gate(interval: 60) + gate.request() + clock.advance(10) + gate.request() + clock.fire() + XCTAssertEqual(runs(), 2) + clock.advance(5) + gate.request() + XCTAssertEqual(runs(), 2) // still inside the new window + XCTAssertEqual(clock.scheduled.count, 1) + clock.fire() + XCTAssertEqual(runs(), 3) + } + + func test_force_bypassesTheWindow() { + let (gate, clock, runs) = self.gate(interval: 60) + gate.request() + clock.advance(1) + gate.force() + XCTAssertEqual(runs(), 2) + XCTAssertTrue(clock.scheduled.isEmpty) + } + + // force also restarts the window, so it can't be used to defeat the limit by + // alternating with request(). + func test_force_restartsTheWindow() { + let (gate, clock, runs) = self.gate(interval: 60) + gate.force() + clock.advance(10) + gate.request() + XCTAssertEqual(runs(), 1) + XCTAssertEqual(clock.scheduled.count, 1) + } +} diff --git a/panel/GitHubAPI.swift b/panel/GitHubAPI.swift index 2cb781d..0768d30 100644 --- a/panel/GitHubAPI.swift +++ b/panel/GitHubAPI.swift @@ -45,44 +45,93 @@ enum GitHubAPI { return (String(parts[0]), String(parts[1])) } - // GraphQL request body (JSON string) for the newest PR on `branch`. - static func query(owner: String, repo: String, branch: String) -> String { - let graphql = """ - query($owner:String!,$repo:String!,$branch:String!){\ - repository(owner:$owner,name:$repo){\ - pullRequests(headRefName:$branch,first:1,orderBy:{field:CREATED_AT,direction:DESC}){\ + // Branches per batched request. GraphQL is charged by query complexity rather + // than by request, so the ceiling here is about keeping any one failed + // round-trip cheap to lose, not about cost. + static let maxBranchesPerQuery = 25 + + // Split a repo's branches into batch-sized groups, order preserved. + static func branchChunks(_ branches: [String]) -> [[String]] { + guard !branches.isEmpty else { return [] } + return stride(from: 0, to: branches.count, by: maxBranchesPerQuery).map { + Array(branches[$0.. String { + let selection = """ nodes{number url state isDraft \ - commits(last:1){nodes{commit{statusCheckRollup{state}}}}}}}} + commits(last:1){nodes{commit{statusCheckRollup{state}}}}} """ - let body: [String: Any] = [ - "query": graphql, - "variables": ["owner": owner, "repo": repo, "branch": branch], - ] + let declarations = branches.indices.map { ",$b\($0):String!" }.joined() + let fields = branches.indices.map { index in + "b\(index):pullRequests(headRefName:$b\(index),first:1," + + "orderBy:{field:CREATED_AT,direction:DESC}){\(selection)}" + }.joined() + let graphql = "query($owner:String!,$repo:String!\(declarations))" + + "{repository(owner:$owner,name:$repo){\(fields)}}" + + var variables: [String: Any] = ["owner": owner, "repo": repo] + for (index, branch) in branches.enumerated() { variables["b\(index)"] = branch } + let body: [String: Any] = ["query": graphql, "variables": variables] let data = (try? JSONSerialization.data(withJSONObject: body)) ?? Data() return String(data: data, encoding: .utf8) ?? "" } - // Fetch a branch's PR via the injected runner (`run(graphQLBody) -> json?`). - static func pullRequest(owner: String, repo: String, branch: String, - run: (String) -> String?) -> PullRequestInfo? { - guard let response = run(query(owner: owner, repo: repo, branch: branch)) else { return nil } - return parse(response) + // Fetch every branch's newest PR through the injected runner + // (`run(graphQLBody) -> json?`), batching to keep the round-trip count down. + // Returns branch -> info for the branches that have a PR; a branch with none, + // or a chunk whose request failed, is simply absent. Requests are issued + // serially on purpose: GitHub's secondary rate limits penalise concurrency. + static func pullRequests(owner: String, repo: String, branches: [String], + run: (String) -> String?) -> [String: PullRequestInfo] { + var result: [String: PullRequestInfo] = [:] + for chunk in branchChunks(branches) { + guard let response = run(batchQuery(owner: owner, repo: repo, branches: chunk)) + else { continue } + result.merge(parse(response, branches: chunk)) { _, new in new } + } + return result } - static func parse(_ json: String) -> PullRequestInfo? { + // Unpack an aliased batch response. `branches` must be the same slice, in the + // same order, that built the query: the b aliases are what map a + // response field back to its branch. + static func parse(_ json: String, branches: [String]) -> [String: PullRequestInfo] { guard let data = json.data(using: .utf8), let root = try? JSONSerialization.jsonObject(with: data) as? [String: Any] - else { return nil } - // GraphQL returns HTTP 200 with {"data":null,"errors":[…]} on failures - // (bad token, rate limit, field errors). Surface them instead of - // silently treating the response as "no PR found". + else { return [:] } + // GraphQL returns HTTP 200 with {"errors":[…]} on failures (unreadable + // repo, rate limit, field errors). Surface them instead of silently + // treating the response as "no PR found", but keep reading: GraphQL can + // report an error for one field and still return data for the others, and + // a batch covers up to maxBranchesPerQuery branches. Bailing out here + // would throw away every good node because of one bad one, which the + // per-branch queries this replaced could never do. A wholesale failure + // (unreadable repo) nulls `repository` and falls out below anyway. if let errors = root["errors"] as? [[String: Any]], !errors.isEmpty { let messages = errors.compactMap { $0["message"] as? String }.joined(separator: "; ") FileHandle.standardError.write(Data("stack-nudge: GitHub GraphQL errors: \(messages)\n".utf8)) - return nil } - guard let node = firstPRNode(root), - let number = node["number"] as? Int, + guard let repository = (root["data"] as? [String: Any])?["repository"] as? [String: Any] + else { return [:] } + var result: [String: PullRequestInfo] = [:] + for (index, branch) in branches.enumerated() { + guard let node = firstPRNode(repository, alias: "b\(index)"), + let info = info(fromNode: node) + else { continue } + result[branch] = info + } + return result + } + + static func info(fromNode node: [String: Any]) -> PullRequestInfo? { + guard let number = node["number"] as? Int, let url = node["url"] as? String, let stateRaw = node["state"] as? String, let state = PRState(rawValue: stateRaw) @@ -95,10 +144,8 @@ enum GitHubAPI { ci: ciStatus(fromNode: node)) } - private static func firstPRNode(_ root: [String: Any]) -> [String: Any]? { - let data = root["data"] as? [String: Any] - let repository = data?["repository"] as? [String: Any] - let pullRequests = repository?["pullRequests"] as? [String: Any] + private static func firstPRNode(_ repository: [String: Any], alias: String) -> [String: Any]? { + let pullRequests = repository[alias] as? [String: Any] let nodes = pullRequests?["nodes"] as? [[String: Any]] return nodes?.first } diff --git a/panel/OutcomeWatcher.swift b/panel/OutcomeWatcher.swift index 7d690bb..1ce433c 100644 --- a/panel/OutcomeWatcher.swift +++ b/panel/OutcomeWatcher.swift @@ -10,6 +10,20 @@ enum OutcomeStatus: String, Equatable { case clean // nothing pending (on base / no net change / can't determine) } +// Every value OutcomeWatcher.derive reads for one branch. Ancestry between two +// fixed shas cannot change (rewriting a commit gives it a new sha), so identical +// inputs guarantee an identical status; that makes this a sound cache key and +// lets a repeat refresh skip the merge-base spawns entirely. Build it with +// `OutcomeWatcher.inputs(...)` so a new input can't be added to the ladder +// without being added here. +struct OutcomeInputs: Equatable { + let branchTip: String? + let remoteTip: String? + let baseSha: String? + let headCommit: String? + let filesChanged: Int +} + // Derives the outcome from *current* git state, read through an injected runner // (`git(args) -> stdout?`, bound to the repo by the caller; nil on failure). // Ref-based only — never checks out — so it's safe for any historical branch and @@ -26,7 +40,77 @@ enum OutcomeWatcher { "upstream/main", "upstream/master", ] + // Every ref one repo's branches need, read in a single spawn. `derive` used to + // resolve these itself: two `rev-parse`s per branch (the branch and its + // origin counterpart) plus a walk of baseCandidates per branch, which on a + // 90-day ledger meant ~6 subprocesses per branch and over 1,500 per refresh. + // Resolving the whole repo once turns all of that into one `show-ref`. + struct RepoRefs: Equatable { + // Short ref name to sha, across heads and the origin / upstream remotes. + // Keyed exactly as the old `rev-parse --verify ` was called, so a + // lookup here is a drop-in for that spawn. + let shaByRef: [String: String] + let baseSha: String? + + func tip(_ branch: String) -> String? { shaByRef[branch] } + func remoteTip(_ branch: String) -> String? { shaByRef["origin/\(branch)"] } + + init(shaByRef: [String: String]) { + self.shaByRef = shaByRef + self.baseSha = OutcomeWatcher.baseSha { shaByRef[$0] } + } + } + + // `show-ref --head` rather than `for-each-ref`, because it is the one that + // also reports HEAD: a session run detached records its branch as literally + // "HEAD" (that's what `rev-parse --abbrev-ref HEAD` gives), and for-each-ref + // only matches patterns under refs/, so those rows would look like deleted + // branches and change status. Output is ` `; shortening is done + // here so only the namespaces the ladder names can enter the map, which keeps + // a tag from shadowing a same-named branch. + static func loadRefs(git: ([String]) -> String?) -> RepoRefs { + let output = git(["show-ref", "--head"]) ?? "" + var shaByRef: [String: String] = [:] + for line in output.split(separator: "\n") { + let parts = line.split(separator: " ", maxSplits: 1, omittingEmptySubsequences: true) + guard parts.count == 2 else { continue } + let sha = String(parts[0]).trimmingCharacters(in: .whitespacesAndNewlines) + let ref = String(parts[1]).trimmingCharacters(in: .whitespacesAndNewlines) + guard !sha.isEmpty, let short = shortRef(ref) else { continue } + shaByRef[short] = sha + } + return RepoRefs(shaByRef: shaByRef) + } + + // Full ref to the name the ladder and baseCandidates use, or nil for refs we + // never consult (tags, notes, other remotes). + private static func shortRef(_ ref: String) -> String? { + if ref == "HEAD" { return ref } + for (prefix, keep) in [("refs/heads/", ""), + ("refs/remotes/origin/", "origin/"), + ("refs/remotes/upstream/", "upstream/")] + where ref.hasPrefix(prefix) { + let name = String(ref.dropFirst(prefix.count)) + return name.isEmpty ? nil : keep + name + } + return nil + } + + // The inputs `derive` will read for this branch, for cache comparison. + static func inputs(branch: String?, + refs: RepoRefs, + headCommit: String?, + filesChangedAtStop: Int) -> OutcomeInputs { + OutcomeInputs( + branchTip: branch.flatMap(refs.tip), + remoteTip: branch.flatMap(refs.remoteTip), + baseSha: refs.baseSha, + headCommit: headCommit, + filesChanged: filesChangedAtStop) + } + static func derive(branch: String?, + refs: RepoRefs, headCommit: String?, filesChangedAtStop: Int, git: ([String]) -> String?) -> OutcomeStatus { @@ -34,8 +118,8 @@ enum OutcomeWatcher { // Branch ref is gone (deleted): shipped if the recorded head merged into // the base, otherwise we can't tell — treat as clean. - guard let branchTip = revParse(branch, git) else { - if let head = headCommit, let base = resolveBaseSha(git), isAncestor(head, base, git) { + guard let branchTip = refs.tip(branch) else { + if let head = headCommit, let base = refs.baseSha, isAncestor(head, base, git) { return .merged } return .clean @@ -50,13 +134,16 @@ enum OutcomeWatcher { // Merged: the branch's tip is contained in the base *and* differs from // it. The `!= base` guard stops a branch sitting exactly at base (no // commits of its own) from reading as merged — it's clean/pending. - if let base = resolveBaseSha(git), branchTip != base, isAncestor(branchTip, base, git) { + if let base = refs.baseSha, branchTip != base, isAncestor(branchTip, base, git) { return .merged } - if let remoteTip = revParse("origin/\(branch)", git) { + if let remoteTip = refs.remoteTip(branch) { + // Equal shas already prove every local commit is on the remote; only + // a divergence needs git to settle which side is ahead. + if remoteTip == branchTip { return .pushed } return isAncestor(branchTip, remoteTip, git) ? .pushed : .committed } - if let base = resolveBaseSha(git), base != branchTip, isAncestor(base, branchTip, git) { + if let base = refs.baseSha, base != branchTip, isAncestor(base, branchTip, git) { return .committed // commits beyond base, no remote branch } return pendingOrClean(branchTip: branchTip, headCommit: headCommit, filesChangedAtStop: filesChangedAtStop) @@ -68,10 +155,18 @@ enum OutcomeWatcher { // Resolve the base branch's sha (local default first; see baseCandidates). // Shared with handoff capture, which uses it to read only branch-local - // commits when deriving the ticket. + // commits when deriving the ticket. One spawn per candidate tried, so it's + // for the once-per-Stop path; RepoRefs resolves the same ladder off its + // already-loaded map. static func resolveBaseSha(_ git: ([String]) -> String?) -> String? { + baseSha { revParse($0, git) } + } + + // The baseCandidates ladder over any ref-to-sha lookup, so the spawning and + // preloaded paths can't drift on which ref counts as the base. + static func baseSha(lookup: (String) -> String?) -> String? { for candidate in baseCandidates { - if let sha = revParse(candidate, git) { return sha } + if let sha = lookup(candidate) { return sha } } return nil } diff --git a/panel/OutcomesView.swift b/panel/OutcomesView.swift index 70c5c36..63dcfd9 100644 --- a/panel/OutcomesView.swift +++ b/panel/OutcomesView.swift @@ -27,6 +27,14 @@ struct BranchBreakdown: Identifiable, Equatable { let diff: DiffStat } +// Where the keyboard selection should scroll to. Two anchors because the list is +// lazy: `groupAnchor` is on the LazyVStack's direct child (resolvable before that +// group has been built), `rowID` is the precise header / branch row inside it. +struct OutcomeRowTarget: Equatable { + let groupAnchor: String + let rowID: String +} + // How a group is keyed. Ticket groups carry a real Linear/Jira key and deep-link // to the tracker; repo groups are the bucket for unticketed work, gathering every // branch that ran in a repo so loose work-streams nest under the repo instead of @@ -72,7 +80,7 @@ struct OutcomesView: View { var body: some View { VStack(alignment: .leading, spacing: 0) { let groups = nav.visibleOutcomeGroups() - let selectedRowID = Self.selectedRowID(groups, index: nav.outcomeSelectedIndex) + let target = Self.selectedRow(groups, index: nav.outcomeSelectedIndex) if nav.githubLinkingEnabled, !nav.githubSignedIn { connectGithubCard Divider().opacity(0.4) @@ -82,9 +90,14 @@ struct OutcomesView: View { } else { ScrollViewReader { proxy in ScrollView { - VStack(alignment: .leading, spacing: 8) { + // Lazy, like the Sessions and Events lists. A plain VStack + // built every group *and* every branch sub-row on every + // render; on a 90-day ledger that's a few hundred rows + // rebuilt per arrow keypress, since the selection index + // is published state this view reads. + LazyVStack(alignment: .leading, spacing: 8) { ForEach(groups) { group in - groupRow(group, selectedRowID: selectedRowID) + groupRow(group, selectedRowID: target?.rowID) } } .padding(.horizontal, 14) @@ -94,21 +107,32 @@ struct OutcomesView: View { } .frame(maxWidth: .infinity, maxHeight: .infinity) .scrollIndicators(.visible) - // Watch the resolved row id, not the raw index, and scroll to - // the id onChange hands us. Reading a separately-derived - // `selectedRowID` capture here lagged the viewport one keypress - // behind the selection — invisible when stepping ±1 (the row - // stays near the viewport) but obvious on a ⌘↑/↓ jump, where the - // pane didn't move until the next press. Mirrors the Sessions + // Watch the resolved target, not the raw index, and scroll to + // what onChange hands us. Reading a separately-derived + // capture here lagged the viewport one keypress behind the + // selection; invisible when stepping ±1 (the row stays near + // the viewport) but obvious on a ⌘↑/↓ jump, where the pane + // didn't move until the next press. Mirrors the Sessions // tab's onChange(of: selectedPID) pattern. - .onChange(of: selectedRowID) { newID in - guard let newID else { return } + .onChange(of: target) { newTarget in + guard let newTarget else { return } + // Two hops, because the list is lazy: a row's own anchor + // sits *inside* its group's body, and scrollTo cannot + // resolve an id in a child the LazyVStack hasn't built yet + // (measured: it moves nothing at all). The group anchor is + // on the stack's direct child, so it resolves unrealized; + // that hop builds the group, then the precise row anchor + // resolves on the next tick. + // // Snap, don't animate. With key-repeat on a long list the // 0.15s animations piled up, the scroll lagged the - // selection, then settled with an upward snap — the + // selection, then settled with an upward snap: the // "bounce". Instant scroll keeps the selection pinned and // the viewport tracking it smoothly. - proxy.scrollTo(newID, anchor: .center) + proxy.scrollTo(newTarget.groupAnchor, anchor: .center) + DispatchQueue.main.async { + proxy.scrollTo(newTarget.rowID, anchor: .center) + } } } } @@ -206,6 +230,10 @@ struct OutcomesView: View { .background(RoundedRectangle(cornerRadius: 6) .fill(Color.primary.opacity(0.04))) .contentShape(Rectangle()) + // Coarse scroll anchor on the lazy stack's direct child, so it resolves + // before this group has been built. The precise per-row anchors below it + // only exist once it has; see the two-hop scroll in `body`. + .id(Self.groupAnchorID(group)) .onTapGesture { if linkable { openTicket(group) } } .help(linkable ? "Open \(group.label) in your tracker" : "") } @@ -246,14 +274,32 @@ struct OutcomesView: View { "b:\(PanelNav.outcomeKey(branch.repoRoot, branch.branch))" } - private static func selectedRowID(_ groups: [TicketGroup], index: Int) -> String? { - var ids: [String] = [] + // Coarse anchor for the whole group card, distinct from headerID: that one is + // on the header row inside the card (a card-wide selection highlight read as + // an upward jump), which makes it invisible to a lazy scroll. + static func groupAnchorID(_ group: TicketGroup) -> String { "ga:\(group.id)" } + + // Walks the flat row order (header, then that group's branch sub-rows) to the + // requested index. Deliberately allocation-free: materialising the whole id + // list meant building a string per row on every render, and the caller only + // ever wants one of them. Out-of-range clamps to the first / last row, and an + // empty ledger yields nil. + static func selectedRow(_ groups: [TicketGroup], index: Int) -> OutcomeRowTarget? { + guard let last = groups.last else { return nil } + var remaining = max(0, index) for group in groups { - ids.append(headerID(group)) - ids.append(contentsOf: group.branches.map(branchID)) + if remaining == 0 { return target(group, headerID(group)) } + remaining -= 1 + if remaining < group.branches.count { + return target(group, branchID(group.branches[remaining])) + } + remaining -= group.branches.count } - guard !ids.isEmpty else { return nil } - return ids[min(max(0, index), ids.count - 1)] + return target(last, last.branches.last.map(branchID) ?? headerID(last)) + } + + private static func target(_ group: TicketGroup, _ rowID: String) -> OutcomeRowTarget { + OutcomeRowTarget(groupAnchor: groupAnchorID(group), rowID: rowID) } // "3 sessions · 218K tokens · 23 files · +1.8k/−400 · Claude, Codex" — diff --git a/panel/Panel.swift b/panel/Panel.swift index 7e4f9b7..f1de567 100644 --- a/panel/Panel.swift +++ b/panel/Panel.swift @@ -149,12 +149,15 @@ struct PanelContentView: View { } } - // Distinct ticket/branch groups in the ledger — the badge on the Tickets - // tab. Reads nav.handoffsRevision so the count refreshes when a Stop adds - // a session while the panel is open. + // The badge on the Tickets tab. Counts exactly the groups that tab renders, + // via nav's memoized rollup. Re-reading the ledger here meant sorting every + // record and building a set on each render of *any* tab (this sits above the + // mode switch), and it keyed by `ticket ?? branch` while the tab groups by + // `ticket ?? repoRoot`, so the badge over-counted. Reads nav.handoffsRevision + // so the count refreshes when a Stop adds a session while the panel is open. private var ticketGroupCount: Int { _ = nav.handoffsRevision - return Set(HandoffLedger.shared.all().map { $0.ticket ?? $0.branch ?? "—" }).count + return nav.visibleOutcomeGroups().count } private var tabStrip: some View { @@ -577,6 +580,19 @@ final class PanelController: NSObject, NSApplicationDelegate, PanelKeyDelegate, private let codexQuotaProbe = CodexQuotaProbe() private let antigravityUsageProbe = AntigravityUsageProbe() private var quotaTimer: Timer? + // Last outcome derived per repo+branch, alongside the git values it was + // derived from, so refreshOutcomes can skip re-deriving what hasn't moved. + // Main-thread only (see refreshOutcomes for the snapshot/write-back). + private var outcomeCache: [String: (inputs: OutcomeInputs, status: OutcomeStatus)] = [:] + // Both Tickets-tab refreshes are requested on every appearance of the tab, so + // they are gated. Outcomes are local git and cheap once warm; the PR fetch is + // network against a rate-limited API, hence the wider window. + private lazy var outcomeGate = RefreshGate(interval: 30) { [weak self] in + self?.performRefreshOutcomes() + } + private lazy var pullRequestGate = RefreshGate(interval: 120) { [weak self] in + self?.performRefreshPullRequests() + } private struct TranscriptRefreshKey: Equatable { let pid: Int let agent: String @@ -735,6 +751,7 @@ final class PanelController: NSObject, NSApplicationDelegate, PanelKeyDelegate, } nav.refreshOutcomes = { [weak self] in self?.refreshOutcomes() } nav.refreshPullRequests = { [weak self] in self?.refreshPullRequests() } + nav.refreshPullRequestsNow = { [weak self] in self?.refreshPullRequestsNow() } nav.startGithubSignIn = { [weak self] in self?.startGithubSignIn() } nav.cancelGithubSignIn = { [weak self] in self?.cancelGithubSignIn() } @@ -1669,53 +1686,110 @@ final class PanelController: NSObject, NSApplicationDelegate, PanelKeyDelegate, // off-main (git is slow), then publish to nav for the Tickets tab. Uses the // newest record per branch (ledger is newest-first) for the headCommit / // uncommitted-at-Stop inputs to the derivation. - func refreshOutcomes() { - var pairs: [(repo: String, branch: String, head: String?, files: Int)] = [] + // + // Grouped by repo so each repo loads its refs once (one `for-each-ref`) + // instead of every branch resolving its own tips and re-walking the base + // candidates. What remains per branch is at most a `merge-base`, and the + // cache below skips even that when nothing the derivation reads has moved. + func refreshOutcomes() { outcomeGate.request() } + + private func performRefreshOutcomes() { + var repoOrder: [String] = [] + var branchesByRepo: [String: [(branch: String, head: String?, files: Int)]] = [:] var seen = Set() for record in HandoffLedger.shared.all() { guard let repo = record.repoRoot, let branch = record.branch else { continue } - if seen.insert(PanelNav.outcomeKey(repo, branch)).inserted { - pairs.append((repo, branch, record.headCommit, record.filesChanged ?? 0)) - } - } - guard !pairs.isEmpty else { return } + guard seen.insert(PanelNav.outcomeKey(repo, branch)).inserted else { continue } + if branchesByRepo[repo] == nil { repoOrder.append(repo) } + branchesByRepo[repo, default: []] + .append((branch, record.headCommit, record.filesChanged ?? 0)) + } + guard !repoOrder.isEmpty else { return } + // Snapshot on main, use off-main, write back on main: keeps the cache + // single-writer without a lock. Rebuilt rather than mutated, so branches + // that leave the ledger don't accumulate. + let cache = outcomeCache DispatchQueue.global(qos: .utility).async { [weak self] in var result: [String: OutcomeStatus] = [:] - for pair in pairs { - result[PanelNav.outcomeKey(pair.repo, pair.branch)] = OutcomeWatcher.derive( - branch: pair.branch, headCommit: pair.head, filesChangedAtStop: pair.files - ) { Self.gitValue(pair.repo, $0) } + var nextCache: [String: (inputs: OutcomeInputs, status: OutcomeStatus)] = [:] + for repoRoot in repoOrder { + let refs = OutcomeWatcher.loadRefs { Self.gitValue(repoRoot, $0) } + for entry in branchesByRepo[repoRoot] ?? [] { + let key = PanelNav.outcomeKey(repoRoot, entry.branch) + let inputs = OutcomeWatcher.inputs( + branch: entry.branch, refs: refs, + headCommit: entry.head, filesChangedAtStop: entry.files) + // Every value the ladder reads is in the key, so an unchanged + // entry cannot have changed status. A pull, push, commit or + // branch delete moves one of them and invalidates it. + if let hit = cache[key], hit.inputs == inputs { + result[key] = hit.status + nextCache[key] = hit + continue + } + let status = OutcomeWatcher.derive( + branch: entry.branch, refs: refs, headCommit: entry.head, + filesChangedAtStop: entry.files) { Self.gitValue(repoRoot, $0) } + result[key] = status + nextCache[key] = (inputs, status) + } + } + DispatchQueue.main.async { + self?.outcomeCache = nextCache + self?.nav.outcomeByBranch = result } - DispatchQueue.main.async { self?.nav.outcomeByBranch = result } } } // Opt-in (STACKNUDGE_GITHUB): fetch the PR + CI state for each distinct // repo+branch via the GitHub GraphQL API with our stored token, off-main, - // publishing incrementally so chips appear as each branch resolves. A PR's - // MERGED state is what closes the squash gap the local OutcomeWatcher can't - // see. No-op when disabled or not signed in. - func refreshPullRequests() { + // publishing per repo so chips appear as the work resolves. A PR's MERGED + // state is what closes the squash gap the local OutcomeWatcher can't see. + // No-op when disabled or not signed in. + // + // Grouped by repo rather than walked branch-by-branch: the old shape spawned + // two `git remote get-url` processes and issued one blocking GraphQL request + // *per branch*, then wrote a single dictionary key back on main each time. At + // 249 branches across 37 repos that was ~500 subprocesses, 249 round-trips, + // and 249 whole-panel re-renders for one refresh. Per repo it's one slug + // resolution, one batched query per 25 branches, and one publish. + func refreshPullRequests() { pullRequestGate.request() } + + // Explicit user action (enabling the feature, completing sign-in): skip the + // window so the chips appear straight away. + func refreshPullRequestsNow() { pullRequestGate.force() } + + private func performRefreshPullRequests() { guard nav.githubLinkingEnabled, let token = GitHubAuth.token() else { return } - var pairs: [(repo: String, branch: String)] = [] + // Ledger order is newest-first, so repoOrder puts the most recently active + // repo first and its chips resolve first. + var repoOrder: [String] = [] + var branchesByRepo: [String: [String]] = [:] var seen = Set() for record in HandoffLedger.shared.all() { guard let repo = record.repoRoot, let branch = record.branch else { continue } - if seen.insert(PanelNav.outcomeKey(repo, branch)).inserted { - pairs.append((repo, branch)) - } + guard seen.insert(PanelNav.outcomeKey(repo, branch)).inserted else { continue } + if branchesByRepo[repo] == nil { repoOrder.append(repo) } + branchesByRepo[repo, default: []].append(branch) } - guard !pairs.isEmpty else { return } + guard !repoOrder.isEmpty else { return } DispatchQueue.global(qos: .utility).async { [weak self] in - for pair in pairs { - guard let slug = Self.repoSlug(forRepo: pair.repo) else { continue } - let info = GitHubAPI.pullRequest(owner: slug.owner, repo: slug.repo, - branch: pair.branch) { body in + for repoRoot in repoOrder { + guard let branches = branchesByRepo[repoRoot], + let slug = Self.repoSlug(forRepo: repoRoot) + else { continue } + let byBranch = GitHubAPI.pullRequests( + owner: slug.owner, repo: slug.repo, branches: branches + ) { body in Self.graphQLPOST(body, token: token) } - guard let info else { continue } - let key = PanelNav.outcomeKey(pair.repo, pair.branch) - DispatchQueue.main.async { self?.nav.pullRequestByBranch[key] = info } + guard !byBranch.isEmpty else { continue } + let keyed = Dictionary(uniqueKeysWithValues: byBranch.map { + (PanelNav.outcomeKey(repoRoot, $0.key), $0.value) + }) + DispatchQueue.main.async { + self?.nav.pullRequestByBranch.merge(keyed) { _, new in new } + } } } } @@ -1798,7 +1872,7 @@ final class PanelController: NSObject, NSApplicationDelegate, PanelKeyDelegate, GitHubAuth.store(token: token) self.nav.githubSignedIn = true self.nav.githubSignIn = .idle - self.refreshPullRequests() + self.refreshPullRequestsNow() case .pending: self.pollGithubSignIn(clientID: clientID, deviceCode: deviceCode, interval: interval, deadline: deadline) case .slowDown(let slower): diff --git a/panel/PanelNav.swift b/panel/PanelNav.swift index 2dda85a..780d64f 100644 --- a/panel/PanelNav.swift +++ b/panel/PanelNav.swift @@ -315,7 +315,10 @@ final class PanelNav: ObservableObject { // outcome when present — a PR reporting MERGED closes the squash gap. Keyed // by `outcomeKey`. Empty when the feature is off or `gh` is absent. @Published var pullRequestByBranch: [String: PullRequestInfo] = [:] + // Rate-limited: safe to call on every appearance of the tab, may be deferred. var refreshPullRequests: (() -> Void)? + // Same fetch, bypassing the rate limit. For explicit user actions only. + var refreshPullRequestsNow: (() -> Void)? // Mirror of STACKNUDGE_GITHUB; gates the PR fetch. Off by default — local // git tracking stays fully functional without a GitHub token. @Published var githubLinkingEnabled: Bool = false @@ -1180,7 +1183,7 @@ final class PanelNav: ObservableObject { ConfigFile.write(key: "STACKNUDGE_GITHUB", value: githubLinkingEnabled ? "true" : "false") if githubLinkingEnabled { - if githubSignedIn { refreshPullRequests?() } else { startGithubSignIn?() } + if githubSignedIn { refreshPullRequestsNow?() } else { startGithubSignIn?() } } else { pullRequestByBranch = [:] githubSignIn = .idle diff --git a/panel/RefreshGate.swift b/panel/RefreshGate.swift new file mode 100644 index 0000000..92551dc --- /dev/null +++ b/panel/RefreshGate.swift @@ -0,0 +1,59 @@ +import Foundation + +// Rate limiter for work that recomputes everything from scratch, and is +// therefore safe to defer or collapse. The Tickets tab asks for both of its +// refreshes on every appearance, so flipping to it re-paid the full cost each +// time: for the PR fetch that is dozens of network round-trips against a +// rate-limited API, and a Stop landing mid-burst used to kick another pass. +// +// A request inside the window is deferred rather than dropped, so the last +// request always gets served and the caller never has to know whether its data +// made it in. Repeated requests inside one window collapse into a single run. +// +// Main-thread only. `now` and `after` are injected so the timing behaviour can be +// tested without waiting on real timers. +final class RefreshGate { + + private let interval: TimeInterval + private let now: () -> Date + private let after: (TimeInterval, @escaping () -> Void) -> Void + private let work: () -> Void + + // nil until the first run, so a cold gate always fires immediately. + private var lastRun: Date? + private var deferredRun = false + + init(interval: TimeInterval, + now: @escaping () -> Date = Date.init, + after: @escaping (TimeInterval, @escaping () -> Void) -> Void = { delay, block in + DispatchQueue.main.asyncAfter(deadline: .now() + delay, execute: block) + }, + work: @escaping () -> Void) { + self.interval = interval + self.now = now + self.after = after + self.work = work + } + + // Run now if the window has elapsed, otherwise once when it does. + func request() { + guard let lastRun else { return run() } + let remaining = interval - now().timeIntervalSince(lastRun) + guard remaining > 0 else { return run() } + guard !deferredRun else { return } + deferredRun = true + after(remaining) { [weak self] in + self?.deferredRun = false + self?.run() + } + } + + // Bypass the window for an explicit user action, where waiting would read as + // the click having done nothing. + func force() { run() } + + private func run() { + lastRun = now() + work() + } +} diff --git a/panel/Sessions.swift b/panel/Sessions.swift index 2a0f7e1..f64c43a 100644 --- a/panel/Sessions.swift +++ b/panel/Sessions.swift @@ -1,6 +1,16 @@ import AppKit import SwiftUI +// A session's slice of the event store: how many nudges match it and when the +// newest one fired. `.empty` is what unselected rows get: they don't render the +// nudge line, so they never scan the event list. +struct NudgeSummary: Equatable { + let count: Int + let lastAt: Date? + + static let empty = NudgeSummary(count: 0, lastAt: nil) +} + struct SessionsView: View { @ObservedObject var store: SessionStore @@ -44,13 +54,20 @@ struct SessionsView: View { ScrollView { LazyVStack(spacing: 2) { ForEach(store.sessions) { session in + let selected = store.selectedPID == session.pid + // Only the selected row renders the nudge line, so only + // the selected row pays for the event scan. Every row + // scanning the list (twice: once for the count, once for + // the timestamp) also made every row's inputs change + // whenever any nudge arrived, forcing a rebuild. + let nudges = selected ? nudgeSummary(for: session) : .empty SessionRow( session: session, - selected: store.selectedPID == session.pid, + selected: selected, isEditing: store.renamingPID == session.pid, renameBuffer: $store.renameBuffer, - activeNudgeCount: nudgeCount(for: session), - lastNudgeAt: lastNudgeAt(for: session), + activeNudgeCount: nudges.count, + lastNudgeAt: nudges.lastAt, transcriptStats: transcriptStats(for: session), isMuted: nav.isMuted(session), onCommit: { store.commitRename() }, @@ -92,18 +109,19 @@ struct SessionsView: View { } // Count of currently-active (undismissed, unsnoozed-or-elapsed-snooze) - // nudges that match this session's (agent, projectPath). The plan - // deliberately scopes this to "in the store right now" — lifetime - // totals across restarts would be fuzzy and aren't asked for. - private func nudgeCount(for session: Session) -> Int { - events.events.filter { matches(event: $0, session: session) }.count - } - - private func lastNudgeAt(for session: Session) -> Date? { - events.events - .filter { matches(event: $0, session: session) } - .map(\.timestamp) - .max() + // nudges that match this session's (agent, projectPath), plus when the most + // recent one fired. The plan deliberately scopes this to "in the store right + // now"; lifetime totals across restarts would be fuzzy and aren't asked for. + // One pass: count and newest-timestamp come off the same walk. + private func nudgeSummary(for session: Session) -> NudgeSummary { + var count = 0 + var lastAt: Date? + for event in events.events where matches(event: event, session: session) { + count += 1 + if let current = lastAt, current >= event.timestamp { continue } + lastAt = event.timestamp + } + return NudgeSummary(count: count, lastAt: lastAt) } // Find the most recent NudgeEvent matching this session that carries a @@ -126,8 +144,9 @@ struct SessionsView: View { } // Fallback for sessions whose sidecar isn't readable (e.g. pre-2.1 // Claude Code): infer from the most recent matching event that - // carried a claudeSessionID. events array is newest-first. - guard let id = events.events + // carried a claudeSessionID. events array is newest-first. Lazy so the + // walk stops at the first hit instead of filtering the whole list. + guard let id = events.events.lazy .filter({ matches(event: $0, session: session) }) .compactMap(\.claudeSessionID) .first