Commit a179ef304c
Unsigned
Layout: unified · split
docs/superpowers/plans/2026-09-06-mr05-compare-refs.md added +300
| @@ -0,0 +1,300 @@ | ||
| 1 | # MR 5: Compare two refs Implementation Plan | |
| 2 | ||
| 3 | > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. | |
| 4 | ||
| 5 | **Goal:** Close the `compare two refs` parity row — pick two refs, see the diff between them. | |
| 6 | ||
| 7 | **Architecture:** Purely additive. A view model reads `repo diff <owner/name> <base> <head>`, parses its `patch` with the existing `UnifiedDiff.parse`, and renders it with the existing `DiffFileSection`. No refactor. | |
| 8 | ||
| 9 | **Spec:** `docs/superpowers/specs/2026-09-06-ios-parity-design.md` | |
| 10 | ||
| 11 | ## Correction to the spec | |
| 12 | ||
| 13 | The spec says this merge request must split `DiffView`, calls it "the only existing code the plan modifies rather than extends", and flags it as the one to expect a review round on. **That is wrong, and this plan supersedes it.** | |
| 14 | ||
| 15 | The seam already exists and already has a second caller: | |
| 16 | ||
| 17 | - `gitbay/Views/MRs/DiffView.swift` — `struct DiffFileSection` takes `var model: MRDetailViewModel? = nil`, commented "Review threads belong to merge requests; a commit diff has none." | |
| 18 | - `gitbay/Views/Repos/CommitView.swift:71` — already calls `DiffFileSection(file: file)` with no model. | |
| 19 | ||
| 20 | So compare reuses `DiffFileSection(file:)` exactly as the commit screen does. **Do not refactor `DiffView`, `DiffFileSection`, `HunkView` or `LineView`.** Touching them is out of scope and risks the merge request review screen, which is the app's most-used surface. | |
| 21 | ||
| 22 | ## Global Constraints | |
| 23 | ||
| 24 | - Swift 6 language mode, default `MainActor` isolation. Wire models are `nonisolated struct`s. | |
| 25 | - Swift Testing only — never XCTest. | |
| 26 | - `gitbayTests` is hermetic and offline; network goes through `StubProtocol`. | |
| 27 | - New files under `gitbay/` and `gitbayTests/` need **no** `project.pbxproj` edit. | |
| 28 | - The label model is `IssueLabel`, never `Label`. | |
| 29 | - Never mention Claude, LLMs or AI in commits, comments, or the merge request. No `Co-Authored-By` trailer. | |
| 30 | - Never commit to `main`. | |
| 31 | ||
| 32 | **The command, verbatim from the registry:** | |
| 33 | ||
| 34 | ``` | |
| 35 | repo diff <owner/name> <base> <head> | |
| 36 | ``` | |
| 37 | ||
| 38 | Both refs are **positional**, base first. It returns: | |
| 39 | ||
| 40 | ```json | |
| 41 | {"base":"f1cc68c…","head":"0f4f9b4…","merge_base":"0f4f9b4…","patch":"","truncated":false} | |
| 42 | ``` | |
| 43 | ||
| 44 | `patch` is unified-diff text — the same shape `UnifiedDiff.parse` already handles for `mr diff` and `repo show`'s commit patch. `truncated` says the server cut the patch short. | |
| 45 | ||
| 46 | **Counting tests reliably** — the naive grep undercounts because `xcodebuild` interleaves output: | |
| 47 | ||
| 48 | ```bash | |
| 49 | grep -cE "^Test case '[^']*' passed" /tmp/out.txt | |
| 50 | grep -cE "^Test case '[^']*' failed" /tmp/out.txt | |
| 51 | grep -rhoE "@Test(\([^)]*\))? func" gitbayTests/*.swift | wc -l # declared | |
| 52 | ``` | |
| 53 | ||
| 54 | Declared minus skipped should equal passed. One test is always skipped (`LiveInstanceTests`, conditionally enabled on a token). | |
| 55 | ||
| 56 | --- | |
| 57 | ||
| 58 | ## File Structure | |
| 59 | ||
| 60 | | File | Responsibility | | |
| 61 | |------|----------------| | |
| 62 | | `gitbay/Repos/CompareViewModel.swift` (create) | `RepoDiff` wire model + the view model over `repo diff` | | |
| 63 | | `gitbayTests/CompareTests.swift` (create) | Every test in this plan | | |
| 64 | | `gitbay/Views/Repos/CompareView.swift` (create) | Two ref pickers and the rendered patch | | |
| 65 | | `gitbay/Views/Repos/RepoRoute.swift` (modify) | `compare(repo:)` case | | |
| 66 | | `gitbay/ContentView.swift` (modify) | Route to `CompareView` | | |
| 67 | | `gitbay/Views/Repos/RefsView.swift` (modify) | Entry point | | |
| 68 | ||
| 69 | --- | |
| 70 | ||
| 71 | ### Task 1: The compare view model | |
| 72 | ||
| 73 | **Files:** Create `gitbay/Repos/CompareViewModel.swift`; test in `gitbayTests/CompareTests.swift` | |
| 74 | ||
| 75 | **Interfaces produced:** | |
| 76 | - `nonisolated struct RepoDiff: Decodable, Sendable, Hashable` — `base: String`, `head: String`, `mergeBase: String`, `patch: String`, `truncated: Bool` | |
| 77 | - `@Observable @MainActor final class CompareViewModel` — `base: String`, `head: String`, `state: LoadState<UnifiedDiff>`, `truncated: Bool`, `mergeBase: String?`, `refs: RepoRefs?`, `func loadRefs() async`, `func compare() async` | |
| 78 | ||
| 79 | Behaviour, each pinned by a test: | |
| 80 | 1. `compare()` sends exactly `["repo", "diff", repoPath, base, head]` — both positional, **base first**. Swapping them silently returns the opposite diff, which is why the test asserts the full array. | |
| 81 | 2. Comparing a ref with itself yields an empty patch, which is an `.empty` state, not `.failed`. | |
| 82 | 3. `truncated` is decoded and exposed. A partial patch rendered as though it were whole is a bug the user cannot see. | |
| 83 | 4. `mergeBase` is decoded and exposed, so the screen can say what the comparison is against. | |
| 84 | 5. Either ref being blank issues **no request** — there is nothing to compare yet. | |
| 85 | 6. `refs` comes from `repo refs`, reusing the existing `RepoRefs`/`RepoRef` models; a failure there is not fatal to the screen but must be distinguishable from "this repository has no branches" (the same three-state lesson MR 4 learned on its retarget picker: expose the refs as an optional plus an error string, not an empty array). | |
| 86 | ||
| 87 | - [ ] **Step 1: Write the failing tests** | |
| 88 | ||
| 89 | Create `gitbayTests/CompareTests.swift`: | |
| 90 | ||
| 91 | ```swift | |
| 92 | import Foundation | |
| 93 | import Testing | |
| 94 | @testable import gitbay | |
| 95 | ||
| 96 | private func makeClient() throws -> (GitbayClient, StubProtocol.Box) { | |
| 97 | let box = StubProtocol.box() | |
| 98 | let client = GitbayClient( | |
| 99 | instance: try GitbayInstance(url: "https://gitbay.org"), | |
| 100 | token: "test-token", | |
| 101 | session: box.session() | |
| 102 | ) | |
| 103 | return (client, box) | |
| 104 | } | |
| 105 | ||
| 106 | private func argvFrom(_ url: URL) -> [String] { | |
| 107 | URLComponents(url: url, resolvingAgainstBaseURL: false)? | |
| 108 | .queryItems?.filter { $0.name == "argv" }.compactMap(\.value) ?? [] | |
| 109 | } | |
| 110 | ||
| 111 | private let patchJSON = """ | |
| 112 | {"protocol_version":1,"data":{"base":"aaa111","head":"bbb222","merge_base":"ccc333",\ | |
| 113 | "patch":"diff --git a/x.txt b/x.txt\\n--- a/x.txt\\n+++ b/x.txt\\n@@ -1 +1 @@\\n-old\\n+new\\n",\ | |
| 114 | "truncated":false},"exit_code":0} | |
| 115 | """ | |
| 116 | ||
| 117 | struct RepoDiffDecodingTests { | |
| 118 | ||
| 119 | @Test func decodesEveryField() throws { | |
| 120 | let decoder = JSONDecoder() | |
| 121 | let json = """ | |
| 122 | {"base":"aaa111","head":"bbb222","merge_base":"ccc333","patch":"x","truncated":true} | |
| 123 | """ | |
| 124 | let diff = try decoder.decode(RepoDiff.self, from: Data(json.utf8)) | |
| 125 | #expect(diff.base == "aaa111") | |
| 126 | #expect(diff.head == "bbb222") | |
| 127 | #expect(diff.mergeBase == "ccc333") | |
| 128 | #expect(diff.truncated) | |
| 129 | } | |
| 130 | } | |
| 131 | ||
| 132 | @MainActor | |
| 133 | struct CompareViewModelTests { | |
| 134 | ||
| 135 | @Test func bothRefsArePositionalAndBaseComesFirst() async throws { | |
| 136 | let (client, stub) = try makeClient() | |
| 137 | stub.enqueue(.init(status: 200, json: patchJSON)) | |
| 138 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 139 | model.base = "main" | |
| 140 | model.head = "feature" | |
| 141 | await model.compare() | |
| 142 | ||
| 143 | #expect(argvFrom(try #require(stub.seen.last).url) | |
| 144 | == ["repo", "diff", "krz/gitbay", "main", "feature"]) | |
| 145 | } | |
| 146 | ||
| 147 | @Test func theParsedPatchReachesTheScreen() async throws { | |
| 148 | let (client, stub) = try makeClient() | |
| 149 | stub.enqueue(.init(status: 200, json: patchJSON)) | |
| 150 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 151 | model.base = "main" | |
| 152 | model.head = "feature" | |
| 153 | await model.compare() | |
| 154 | ||
| 155 | #expect(model.state.value?.files.count == 1) | |
| 156 | #expect(model.mergeBase == "ccc333") | |
| 157 | #expect(model.truncated == false) | |
| 158 | } | |
| 159 | ||
| 160 | /// A partial patch rendered as though whole is a bug the user cannot | |
| 161 | /// see, so the flag must survive to the screen. | |
| 162 | @Test func truncationIsCarriedThrough() async throws { | |
| 163 | let (client, stub) = try makeClient() | |
| 164 | stub.enqueue(.init(status: 200, json: """ | |
| 165 | {"protocol_version":1,"data":{"base":"a","head":"b","merge_base":"c",\ | |
| 166 | "patch":"diff --git a/x b/x\\n--- a/x\\n+++ b/x\\n@@ -1 +1 @@\\n-a\\n+b\\n",\ | |
| 167 | "truncated":true},"exit_code":0} | |
| 168 | """)) | |
| 169 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 170 | model.base = "main" | |
| 171 | model.head = "feature" | |
| 172 | await model.compare() | |
| 173 | ||
| 174 | #expect(model.truncated) | |
| 175 | } | |
| 176 | ||
| 177 | @Test func comparingARefWithItselfIsEmptyNotFailed() async throws { | |
| 178 | let (client, stub) = try makeClient() | |
| 179 | stub.enqueue(.init(status: 200, json: """ | |
| 180 | {"protocol_version":1,"data":{"base":"a","head":"a","merge_base":"a",\ | |
| 181 | "patch":"","truncated":false},"exit_code":0} | |
| 182 | """)) | |
| 183 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 184 | model.base = "main" | |
| 185 | model.head = "main" | |
| 186 | await model.compare() | |
| 187 | ||
| 188 | guard case .empty = model.state else { | |
| 189 | Testing.Issue.record("expected empty, got \(model.state)") | |
| 190 | return | |
| 191 | } | |
| 192 | } | |
| 193 | ||
| 194 | @Test func aBlankRefComparesNothing() async throws { | |
| 195 | let (client, stub) = try makeClient() | |
| 196 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 197 | model.base = "main" | |
| 198 | model.head = " " | |
| 199 | await model.compare() | |
| 200 | ||
| 201 | #expect(stub.seen.isEmpty) | |
| 202 | } | |
| 203 | ||
| 204 | @Test func refsComeFromRepoRefs() async throws { | |
| 205 | let (client, stub) = try makeClient() | |
| 206 | stub.enqueue(.init(status: 200, json: """ | |
| 207 | {"protocol_version":1,"data":{"branches":[{"name":"main","sha":"a"},\ | |
| 208 | {"name":"feature","sha":"b"}],"tags":[{"name":"v1","sha":"c"}]},"exit_code":0} | |
| 209 | """)) | |
| 210 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 211 | await model.loadRefs() | |
| 212 | ||
| 213 | #expect(model.refs?.branches.map(\.name) == ["main", "feature"]) | |
| 214 | #expect(model.refs?.tags.map(\.name) == ["v1"]) | |
| 215 | #expect(argvFrom(try #require(stub.seen.last).url) | |
| 216 | == ["repo", "refs", "krz/gitbay"]) | |
| 217 | } | |
| 218 | ||
| 219 | /// The lesson from the retarget picker: a failed ref load must not | |
| 220 | /// look like a repository with no branches. | |
| 221 | @Test func aFailedRefLoadIsDistinguishableFromNoBranches() async throws { | |
| 222 | let (client, stub) = try makeClient() | |
| 223 | stub.enqueue(.init(status: 200, json: """ | |
| 224 | {"protocol_version":1,"error":"denied","exit_code":4} | |
| 225 | """)) | |
| 226 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 227 | await model.loadRefs() | |
| 228 | ||
| 229 | #expect(model.refs == nil) | |
| 230 | #expect(model.refsError?.isEmpty == false) | |
| 231 | } | |
| 232 | ||
| 233 | @Test func aFailedCompareIsTheScreensState() async throws { | |
| 234 | let (client, stub) = try makeClient() | |
| 235 | stub.enqueue(.init(status: 200, json: """ | |
| 236 | {"protocol_version":1,"error":"unknown revision","exit_code":1} | |
| 237 | """)) | |
| 238 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 239 | model.base = "main" | |
| 240 | model.head = "nope" | |
| 241 | await model.compare() | |
| 242 | ||
| 243 | guard case .failed = model.state else { | |
| 244 | Testing.Issue.record("expected failed, got \(model.state)") | |
| 245 | return | |
| 246 | } | |
| 247 | } | |
| 248 | } | |
| 249 | ``` | |
| 250 | ||
| 251 | - [ ] **Step 2: Run to verify failure.** | |
| 252 | - [ ] **Step 3: Implement.** `RepoDiff` with `CodingKeys` mapping `merge_base`. `CompareViewModel` holding `client`, `repoPath`, the two ref strings, `state`, `truncated`, `mergeBase`, `refs: RepoRefs?`, `refsError: String?`. `compare()` guards on both refs being non-blank, reads, parses with `UnifiedDiff.parse(diff.patch)`, and sets `.empty` when the parsed diff has no files. | |
| 253 | - [ ] **Step 4: Run the tests.** | |
| 254 | - [ ] **Step 5: Commit** — `git commit -m "Compare two refs through repo diff"` | |
| 255 | ||
| 256 | --- | |
| 257 | ||
| 258 | ### Task 2: The compare screen | |
| 259 | ||
| 260 | **Files:** Create `gitbay/Views/Repos/CompareView.swift`; modify `RepoRoute.swift`, `ContentView.swift`, `RefsView.swift` | |
| 261 | ||
| 262 | No unit tests — UI. | |
| 263 | ||
| 264 | **Reuse `DiffFileSection(file:)` with no model**, exactly as `CommitView.swift:71` does. Do not modify it. | |
| 265 | ||
| 266 | The screen: two ref pickers (base and head) fed by `model.refs`, a Compare action, then the rendered patch. Follow `RefsView`'s idiom for presenting branches and tags together. | |
| 267 | ||
| 268 | **Three things the screen must say:** | |
| 269 | 1. **When `truncated`** — a visible notice that the patch is incomplete. `GBNotice` exists for this. Rendering a cut-short diff silently is the one way this screen misleads. | |
| 270 | 2. **The merge base**, so the user knows what the comparison is against — `repo diff` reports it and a three-dot comparison against an unstated base is ambiguous. | |
| 271 | 3. **A failed ref load is not "no branches"** — render `refsError` distinctly from an empty ref list, the same way MR 4's retarget picker does. | |
| 272 | ||
| 273 | Add `RepoRoute.compare(repo:)`, wire it in `ContentView`'s `RepoRoute` switch, and add the entry point to `RefsView` (which is the branches-and-tags screen and the natural home). | |
| 274 | ||
| 275 | - [ ] **Step 1: Route case + ContentView destination** | |
| 276 | - [ ] **Step 2: `CompareView` with the two pickers and the patch** | |
| 277 | - [ ] **Step 3: Entry point in `RefsView`** | |
| 278 | - [ ] **Step 4: Build and run the full suite.** No drop. | |
| 279 | - [ ] **Step 5: Commit** — `git commit -m "Compare screen with two ref pickers"` | |
| 280 | ||
| 281 | --- | |
| 282 | ||
| 283 | ### Task 3: Flip the parity row and open the merge request | |
| 284 | ||
| 285 | - [ ] **Step 1:** In the Repositories table, `compare two refs` goes to `yes` for iOS. Touch no other row. | |
| 286 | ||
| 287 | Land it via a worktree off `origin/main` in `krz/gitbay`, merged `--strategy ff`. That repo requires signed commits, so squash and rebase merges are refused, and its working tree usually holds unrelated work — never switch its branch. | |
| 288 | ||
| 289 | - [ ] **Step 2:** Run the full suite; record the real number. | |
| 290 | - [ ] **Step 3:** Open the merge request. | |
| 291 | ||
| 292 | --- | |
| 293 | ||
| 294 | ## Notes for whoever executes this | |
| 295 | ||
| 296 | **Do not refactor the diff views.** `DiffFileSection` is already reusable and already reused. The spec's claim that this merge request must split `DiffView` is wrong and has been superseded by this plan. | |
| 297 | ||
| 298 | **Base comes first.** `repo diff <owner/name> <base> <head>` — swapping them returns the opposite diff with no error at all. | |
| 299 | ||
| 300 | **`truncated` must reach the screen.** A partial patch that looks whole is worse than an error. | |
gitbay/ContentView.swift +2
| @@ -96,6 +96,8 @@ private struct RouteDestinations: ViewModifier { | ||
| 96 | 96 | BlameView(client: client, repo: repo, path: path, ref: ref) |
| 97 | 97 | case .refs(let repo): |
| 98 | 98 | RefsView(client: client, repo: repo) |
| 99 | case .compare(let repo, let defaultBranch): | |
| 100 | CompareView(client: client, repo: repo, defaultBranch: defaultBranch) | |
| 99 | 101 | case .milestones(let repo): |
| 100 | 102 | MilestoneListView(client: client, repo: repo) |
| 101 | 103 | case .labels(let repo): |
gitbay/Repos/CompareViewModel.swift added +100
| @@ -0,0 +1,100 @@ | ||
| 1 | import Foundation | |
| 2 | import Observation | |
| 3 | ||
| 4 | /// `repo diff <owner/name> <base> <head>` — the same unified-diff shape as | |
| 5 | /// `mr diff` and a commit patch, parsed the same way. | |
| 6 | nonisolated struct RepoDiff: Decodable, Sendable, Hashable { | |
| 7 | let base: String | |
| 8 | let head: String | |
| 9 | let mergeBase: String | |
| 10 | let patch: String | |
| 11 | let truncated: Bool | |
| 12 | ||
| 13 | enum CodingKeys: String, CodingKey { | |
| 14 | case base, head, patch, truncated | |
| 15 | case mergeBase = "merge_base" | |
| 16 | } | |
| 17 | } | |
| 18 | ||
| 19 | /// Compares two refs in a repo. Both refs are positional and base comes | |
| 20 | /// first — swapping them silently returns the opposite diff with no error. | |
| 21 | @Observable | |
| 22 | @MainActor | |
| 23 | final class CompareViewModel { | |
| 24 | ||
| 25 | private static let notComparedYet = "Pick two refs to compare." | |
| 26 | ||
| 27 | var base = "" { didSet { resetResults() } } | |
| 28 | var head = "" { didSet { resetResults() } } | |
| 29 | private(set) var state: LoadState<UnifiedDiff> = .empty(notComparedYet) | |
| 30 | private(set) var truncated = false | |
| 31 | private(set) var mergeBase: String? | |
| 32 | ||
| 33 | /// `repo refs`, for pickers. Exposed as an optional plus an error | |
| 34 | /// string rather than an empty array, so "failed to load" stays | |
| 35 | /// distinguishable from "this repository has no branches". | |
| 36 | private(set) var refs: RepoRefs? | |
| 37 | private(set) var refsError: String? | |
| 38 | ||
| 39 | /// Bumped by `resetResults()` (so on every `base`/`head` change) and | |
| 40 | /// captured by `compare()` before it awaits. A response that arrives | |
| 41 | /// after the generation has moved on belongs to a ref pairing nobody | |
| 42 | /// is looking at any more, so it's discarded rather than written — | |
| 43 | /// otherwise a slow request for an old ref pair could land after a | |
| 44 | /// fast one for the current pair and overwrite it. | |
| 45 | private var generation = 0 | |
| 46 | ||
| 47 | private let client: GitbayClient | |
| 48 | let repoPath: String | |
| 49 | ||
| 50 | init(client: GitbayClient, repoPath: String) { | |
| 51 | self.client = client | |
| 52 | self.repoPath = repoPath | |
| 53 | } | |
| 54 | ||
| 55 | func loadRefs() async { | |
| 56 | refsError = nil | |
| 57 | refs = nil | |
| 58 | do { | |
| 59 | refs = try await client.read(["repo", "refs", repoPath], as: RepoRefs.self) | |
| 60 | } catch let error as GitbayError { | |
| 61 | refsError = error.userFacingMessage | |
| 62 | } catch { | |
| 63 | refsError = GitbayError.transport(error).userFacingMessage | |
| 64 | } | |
| 65 | } | |
| 66 | ||
| 67 | /// Clears whatever the previous compare left behind, so a re-compare | |
| 68 | /// (or a ref changed mid-flight) can never pair a stale merge base or | |
| 69 | /// truncation notice with the new state. | |
| 70 | private func resetResults() { | |
| 71 | generation += 1 | |
| 72 | state = .empty(Self.notComparedYet) | |
| 73 | mergeBase = nil | |
| 74 | truncated = false | |
| 75 | } | |
| 76 | ||
| 77 | func compare() async { | |
| 78 | let base = base.trimmingCharacters(in: .whitespaces) | |
| 79 | let head = head.trimmingCharacters(in: .whitespaces) | |
| 80 | guard !base.isEmpty, !head.isEmpty else { return } | |
| 81 | ||
| 82 | let requestGeneration = generation | |
| 83 | state = .loading | |
| 84 | mergeBase = nil | |
| 85 | truncated = false | |
| 86 | do { | |
| 87 | let diff = try await client.read(["repo", "diff", repoPath, base, head], as: RepoDiff.self) | |
| 88 | guard requestGeneration == generation else { return } | |
| 89 | truncated = diff.truncated | |
| 90 | mergeBase = diff.mergeBase | |
| 91 | let parsed = UnifiedDiff.parse(diff.patch) | |
| 92 | state = parsed.files.isEmpty | |
| 93 | ? .empty("No differences between \(base) and \(head).") | |
| 94 | : .loaded(parsed) | |
| 95 | } catch { | |
| 96 | guard requestGeneration == generation else { return } | |
| 97 | state = .from(error) | |
| 98 | } | |
| 99 | } | |
| 100 | } | |
gitbay/Views/Repos/CompareView.swift added +134
| @@ -0,0 +1,134 @@ | ||
| 1 | import SwiftUI | |
| 2 | ||
| 3 | /// Two refs, one diff — `repo diff`'s three-dot comparison, rendered the | |
| 4 | /// same way a commit's patch is. `base` and `head` are picked from | |
| 5 | /// `repo refs`; the request only fires when the user taps Compare. | |
| 6 | struct CompareView: View { | |
| 7 | ||
| 8 | @State private var model: CompareViewModel | |
| 9 | ||
| 10 | init(client: GitbayClient, repo: String, defaultBranch: String? = nil) { | |
| 11 | let model = CompareViewModel(client: client, repoPath: repo) | |
| 12 | if let defaultBranch { | |
| 13 | model.base = defaultBranch | |
| 14 | } | |
| 15 | _model = State(initialValue: model) | |
| 16 | } | |
| 17 | ||
| 18 | private var isComparing: Bool { | |
| 19 | if case .loading = model.state { return true } | |
| 20 | return false | |
| 21 | } | |
| 22 | ||
| 23 | var body: some View { | |
| 24 | List { | |
| 25 | refsSection | |
| 26 | ||
| 27 | Section { | |
| 28 | Button("Compare") { | |
| 29 | Task { await model.compare() } | |
| 30 | } | |
| 31 | .disabled(isComparing | |
| 32 | || model.base.trimmingCharacters(in: .whitespaces).isEmpty | |
| 33 | || model.head.trimmingCharacters(in: .whitespaces).isEmpty) | |
| 34 | .accessibilityIdentifier("compare-submit") | |
| 35 | } | |
| 36 | ||
| 37 | if model.truncated { | |
| 38 | Section { | |
| 39 | GBNotice("The patch was too large and was cut short.", .gbWarn) | |
| 40 | } | |
| 41 | } | |
| 42 | ||
| 43 | if let mergeBase = model.mergeBase { | |
| 44 | Section { | |
| 45 | HStack { | |
| 46 | Text("Merge base") | |
| 47 | Spacer() | |
| 48 | Text(String(mergeBase.prefix(10))) | |
| 49 | .font(.gbMono(.caption)) | |
| 50 | .foregroundStyle(.secondary) | |
| 51 | } | |
| 52 | } | |
| 53 | } | |
| 54 | ||
| 55 | // `.loading` is shown by the overlay below; `.empty` and | |
| 56 | // `.failed` render inline so the pickers above stay reachable | |
| 57 | // instead of being covered by a frame-filling overlay. | |
| 58 | switch model.state { | |
| 59 | case .empty(let message): | |
| 60 | Section { | |
| 61 | GBNotice(message, .gbWarn) | |
| 62 | } | |
| 63 | case .failed(let message): | |
| 64 | Section { | |
| 65 | GBNotice(message, .gbWarn) | |
| 66 | } | |
| 67 | case .loading, .loaded: | |
| 68 | EmptyView() | |
| 69 | } | |
| 70 | ||
| 71 | if let diff = model.state.value, !diff.files.isEmpty { | |
| 72 | ForEach(diff.files) { file in | |
| 73 | DiffFileSection(file: file) | |
| 74 | } | |
| 75 | } | |
| 76 | } | |
| 77 | .overlay { | |
| 78 | if isComparing { | |
| 79 | LoadStateOverlay(state: model.state) | |
| 80 | } | |
| 81 | } | |
| 82 | .navigationTitle("Compare") | |
| 83 | .navigationBarTitleDisplayMode(.inline) | |
| 84 | .task { await model.loadRefs() } | |
| 85 | } | |
| 86 | ||
| 87 | @ViewBuilder | |
| 88 | private var refsSection: some View { | |
| 89 | Section { | |
| 90 | if let refs = model.refs { | |
| 91 | if refs.branches.isEmpty && refs.tags.isEmpty { | |
| 92 | GBNotice("This repository has no branches or tags to compare.", .gbWarn) | |
| 93 | } else { | |
| 94 | refPicker("Base", refs: refs, selection: Bindable(model).base) | |
| 95 | refPicker("Head", refs: refs, selection: Bindable(model).head) | |
| 96 | } | |
| 97 | } else if let refsError = model.refsError { | |
| 98 | GBNotice(refsError, .gbWarn) | |
| 99 | } else { | |
| 100 | ProgressView() | |
| 101 | } | |
| 102 | } | |
| 103 | } | |
| 104 | ||
| 105 | private func refPicker(_ title: String, refs: RepoRefs, selection: Binding<String>) -> some View { | |
| 106 | Menu { | |
| 107 | if !refs.branches.isEmpty { | |
| 108 | Section("Branches") { | |
| 109 | ForEach(refs.branches) { ref in | |
| 110 | Button(ref.name) { selection.wrappedValue = ref.name } | |
| 111 | } | |
| 112 | } | |
| 113 | } | |
| 114 | if !refs.tags.isEmpty { | |
| 115 | Section("Tags") { | |
| 116 | ForEach(refs.tags) { ref in | |
| 117 | Button(ref.name) { selection.wrappedValue = ref.name } | |
| 118 | } | |
| 119 | } | |
| 120 | } | |
| 121 | } label: { | |
| 122 | HStack { | |
| 123 | Text(title) | |
| 124 | Spacer() | |
| 125 | Text(selection.wrappedValue.isEmpty ? "Select a ref" : selection.wrappedValue) | |
| 126 | .foregroundStyle(selection.wrappedValue.isEmpty ? .secondary : .primary) | |
| 127 | Image(systemName: "chevron.up.chevron.down") | |
| 128 | .font(.gbSans(.caption2)) | |
| 129 | .foregroundStyle(.secondary) | |
| 130 | } | |
| 131 | } | |
| 132 | .accessibilityIdentifier("compare-\(title.lowercased())-menu") | |
| 133 | } | |
| 134 | } | |
gitbay/Views/Repos/RefsView.swift +7
| @@ -11,6 +11,13 @@ struct RefsView: View { | ||
| 11 | 11 | |
| 12 | 12 | var body: some View { |
| 13 | 13 | List { |
| 14 | Section { | |
| 15 | NavigationLink(value: RepoRoute.compare( | |
| 16 | repo: model.repoPath, defaultBranch: model.defaultBranch | |
| 17 | )) { | |
| 18 | Label("Compare", systemImage: "arrow.left.arrow.right") | |
| 19 | } | |
| 20 | } | |
| 14 | 21 | if !model.branches.isEmpty { |
| 15 | 22 | Section("Branches") { |
| 16 | 23 | ForEach(model.branches) { ref in |
gitbay/Views/Repos/RepoRoute.swift +1
| @@ -11,6 +11,7 @@ nonisolated enum RepoRoute: Hashable { | ||
| 11 | 11 | case grep(repo: String) |
| 12 | 12 | case blame(repo: String, path: String, ref: String?) |
| 13 | 13 | case refs(repo: String) |
| 14 | case compare(repo: String, defaultBranch: String?) | |
| 14 | 15 | case milestones(repo: String) |
| 15 | 16 | case labels(repo: String) |
| 16 | 17 | case wiki(repo: String) |
gitbayTests/CompareTests.swift added +355
| @@ -0,0 +1,355 @@ | ||
| 1 | import Foundation | |
| 2 | import Testing | |
| 3 | @testable import gitbay | |
| 4 | ||
| 5 | private func makeClient() throws -> (GitbayClient, StubProtocol.Box) { | |
| 6 | let box = StubProtocol.box() | |
| 7 | let client = GitbayClient( | |
| 8 | instance: try GitbayInstance(url: "https://gitbay.org"), | |
| 9 | token: "test-token", | |
| 10 | session: box.session() | |
| 11 | ) | |
| 12 | return (client, box) | |
| 13 | } | |
| 14 | ||
| 15 | private func argvFrom(_ url: URL) -> [String] { | |
| 16 | URLComponents(url: url, resolvingAgainstBaseURL: false)? | |
| 17 | .queryItems?.filter { $0.name == "argv" }.compactMap(\.value) ?? [] | |
| 18 | } | |
| 19 | ||
| 20 | nonisolated private let patchJSON = """ | |
| 21 | {"protocol_version":1,"data":{"base":"aaa111","head":"bbb222","merge_base":"ccc333",\ | |
| 22 | "patch":"diff --git a/x.txt b/x.txt\\n--- a/x.txt\\n+++ b/x.txt\\n@@ -1 +1 @@\\n-old\\n+new\\n",\ | |
| 23 | "truncated":false},"exit_code":0} | |
| 24 | """ | |
| 25 | ||
| 26 | /// `StubProtocol` answers a request as soon as it arrives, and the actual | |
| 27 | /// delivery back onto the main actor still crosses a real thread — so | |
| 28 | /// polling its request log cannot promise "the response has not landed | |
| 29 | /// yet" at the instant a test checks it. `GatedProtocol` pins that instant | |
| 30 | /// down: `startLoading` blocks the request until the test explicitly | |
| 31 | /// releases it, so a ref mutation made between arrival and release is | |
| 32 | /// provably made while the request is still outstanding. Scoped per test | |
| 33 | /// the same way `StubProtocol` is, via a marker header, so parallel tests | |
| 34 | /// don't share a gate. | |
| 35 | nonisolated private final class GatedProtocol: URLProtocol, @unchecked Sendable { | |
| 36 | ||
| 37 | final class Box: Sendable { | |
| 38 | fileprivate let id = UUID().uuidString | |
| 39 | private let gate = Mutex<(continuation: CheckedContinuation<Void, Never>?, release: DispatchSemaphore?)>((nil, nil)) | |
| 40 | ||
| 41 | func session() -> URLSession { | |
| 42 | let configuration = URLSessionConfiguration.ephemeral | |
| 43 | configuration.protocolClasses = [GatedProtocol.self] | |
| 44 | configuration.httpAdditionalHeaders = [GatedProtocol.marker: id] | |
| 45 | return URLSession(configuration: configuration) | |
| 46 | } | |
| 47 | ||
| 48 | /// Suspends until the in-flight request reaches `startLoading`, | |
| 49 | /// then returns the semaphore that must be signaled to let its | |
| 50 | /// (canned, successful) response through. | |
| 51 | func awaitArrival() async -> DispatchSemaphore { | |
| 52 | let release = DispatchSemaphore(value: 0) | |
| 53 | await withCheckedContinuation { (continuation: CheckedContinuation<Void, Never>) in | |
| 54 | gate.withLock { $0 = (continuation, release) } | |
| 55 | } | |
| 56 | return release | |
| 57 | } | |
| 58 | ||
| 59 | fileprivate func arrived() -> (CheckedContinuation<Void, Never>?, DispatchSemaphore?) { | |
| 60 | gate.withLock { state in | |
| 61 | let result = state | |
| 62 | state.continuation = nil | |
| 63 | return result | |
| 64 | } | |
| 65 | } | |
| 66 | } | |
| 67 | ||
| 68 | private static let marker = "X-Gated-Box" | |
| 69 | private static let boxes = Mutex<[String: Box]>([:]) | |
| 70 | ||
| 71 | static func box() -> Box { | |
| 72 | let box = Box() | |
| 73 | boxes.withLock { $0[box.id] = box } | |
| 74 | return box | |
| 75 | } | |
| 76 | ||
| 77 | override static func canInit(with _: URLRequest) -> Bool { true } | |
| 78 | override static func canonicalRequest(for request: URLRequest) -> URLRequest { request } | |
| 79 | ||
| 80 | override func startLoading() { | |
| 81 | guard let id = request.value(forHTTPHeaderField: Self.marker), | |
| 82 | let box = Self.boxes.withLock({ $0[id] }) else { | |
| 83 | client?.urlProtocol(self, didFailWithError: URLError(.resourceUnavailable)) | |
| 84 | return | |
| 85 | } | |
| 86 | let (continuation, release) = box.arrived() | |
| 87 | continuation?.resume() | |
| 88 | release?.wait() // blocks this background thread until the test releases it | |
| 89 | ||
| 90 | let response = HTTPURLResponse( | |
| 91 | url: request.url!, | |
| 92 | statusCode: 200, | |
| 93 | httpVersion: "HTTP/1.1", | |
| 94 | headerFields: ["Content-Type": "application/json"] | |
| 95 | )! | |
| 96 | client?.urlProtocol(self, didReceive: response, cacheStoragePolicy: .notAllowed) | |
| 97 | client?.urlProtocol(self, didLoad: Data(patchJSON.utf8)) | |
| 98 | client?.urlProtocolDidFinishLoading(self) | |
| 99 | } | |
| 100 | ||
| 101 | override func stopLoading() {} | |
| 102 | } | |
| 103 | ||
| 104 | struct RepoDiffDecodingTests { | |
| 105 | ||
| 106 | @Test func decodesEveryField() throws { | |
| 107 | let decoder = JSONDecoder() | |
| 108 | let json = """ | |
| 109 | {"base":"aaa111","head":"bbb222","merge_base":"ccc333","patch":"x","truncated":true} | |
| 110 | """ | |
| 111 | let diff = try decoder.decode(RepoDiff.self, from: Data(json.utf8)) | |
| 112 | #expect(diff.base == "aaa111") | |
| 113 | #expect(diff.head == "bbb222") | |
| 114 | #expect(diff.mergeBase == "ccc333") | |
| 115 | #expect(diff.patch == "x") | |
| 116 | #expect(diff.truncated) | |
| 117 | } | |
| 118 | } | |
| 119 | ||
| 120 | @MainActor | |
| 121 | struct CompareViewModelTests { | |
| 122 | ||
| 123 | @Test func bothRefsArePositionalAndBaseComesFirst() async throws { | |
| 124 | let (client, stub) = try makeClient() | |
| 125 | stub.enqueue(.init(status: 200, json: patchJSON)) | |
| 126 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 127 | model.base = "main" | |
| 128 | model.head = "feature" | |
| 129 | await model.compare() | |
| 130 | ||
| 131 | #expect(argvFrom(try #require(stub.seen.last).url) | |
| 132 | == ["repo", "diff", "krz/gitbay", "main", "feature"]) | |
| 133 | } | |
| 134 | ||
| 135 | @Test func theParsedPatchReachesTheScreen() async throws { | |
| 136 | let (client, stub) = try makeClient() | |
| 137 | stub.enqueue(.init(status: 200, json: patchJSON)) | |
| 138 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 139 | model.base = "main" | |
| 140 | model.head = "feature" | |
| 141 | await model.compare() | |
| 142 | ||
| 143 | #expect(model.state.value?.files.count == 1) | |
| 144 | #expect(model.mergeBase == "ccc333") | |
| 145 | #expect(model.truncated == false) | |
| 146 | } | |
| 147 | ||
| 148 | /// A partial patch rendered as though whole is a bug the user cannot | |
| 149 | /// see, so the flag must survive to the screen. | |
| 150 | @Test func truncationIsCarriedThrough() async throws { | |
| 151 | let (client, stub) = try makeClient() | |
| 152 | stub.enqueue(.init(status: 200, json: """ | |
| 153 | {"protocol_version":1,"data":{"base":"a","head":"b","merge_base":"c",\ | |
| 154 | "patch":"diff --git a/x b/x\\n--- a/x\\n+++ b/x\\n@@ -1 +1 @@\\n-a\\n+b\\n",\ | |
| 155 | "truncated":true},"exit_code":0} | |
| 156 | """)) | |
| 157 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 158 | model.base = "main" | |
| 159 | model.head = "feature" | |
| 160 | await model.compare() | |
| 161 | ||
| 162 | #expect(model.truncated) | |
| 163 | } | |
| 164 | ||
| 165 | @Test func comparingARefWithItselfIsEmptyNotFailed() async throws { | |
| 166 | let (client, stub) = try makeClient() | |
| 167 | stub.enqueue(.init(status: 200, json: """ | |
| 168 | {"protocol_version":1,"data":{"base":"a","head":"a","merge_base":"a",\ | |
| 169 | "patch":"","truncated":false},"exit_code":0} | |
| 170 | """)) | |
| 171 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 172 | model.base = "main" | |
| 173 | model.head = "main" | |
| 174 | await model.compare() | |
| 175 | ||
| 176 | guard case .empty = model.state else { | |
| 177 | Testing.Issue.record("expected empty, got \(model.state)") | |
| 178 | return | |
| 179 | } | |
| 180 | } | |
| 181 | ||
| 182 | @Test func aBlankRefComparesNothing() async throws { | |
| 183 | let (client, stub) = try makeClient() | |
| 184 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 185 | model.base = "main" | |
| 186 | model.head = " " | |
| 187 | await model.compare() | |
| 188 | ||
| 189 | #expect(stub.seen.isEmpty) | |
| 190 | } | |
| 191 | ||
| 192 | @Test func refsComeFromRepoRefs() async throws { | |
| 193 | let (client, stub) = try makeClient() | |
| 194 | stub.enqueue(.init(status: 200, json: """ | |
| 195 | {"protocol_version":1,"data":{"branches":[{"name":"main","sha":"a"},\ | |
| 196 | {"name":"feature","sha":"b"}],"tags":[{"name":"v1","sha":"c"}]},"exit_code":0} | |
| 197 | """)) | |
| 198 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 199 | await model.loadRefs() | |
| 200 | ||
| 201 | #expect(model.refs?.branches.map(\.name) == ["main", "feature"]) | |
| 202 | #expect(model.refs?.tags.map(\.name) == ["v1"]) | |
| 203 | #expect(argvFrom(try #require(stub.seen.last).url) | |
| 204 | == ["repo", "refs", "krz/gitbay"]) | |
| 205 | } | |
| 206 | ||
| 207 | /// The lesson from the retarget picker: a failed ref load must not | |
| 208 | /// look like a repository with no branches. | |
| 209 | @Test func aFailedRefLoadIsDistinguishableFromNoBranches() async throws { | |
| 210 | let (client, stub) = try makeClient() | |
| 211 | stub.enqueue(.init(status: 200, json: """ | |
| 212 | {"protocol_version":1,"error":"denied","exit_code":4} | |
| 213 | """)) | |
| 214 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 215 | await model.loadRefs() | |
| 216 | ||
| 217 | #expect(model.refs == nil) | |
| 218 | #expect(model.refsError?.isEmpty == false) | |
| 219 | } | |
| 220 | ||
| 221 | @Test func aFailedCompareIsTheScreensState() async throws { | |
| 222 | let (client, stub) = try makeClient() | |
| 223 | stub.enqueue(.init(status: 200, json: """ | |
| 224 | {"protocol_version":1,"error":"unknown revision","exit_code":1} | |
| 225 | """)) | |
| 226 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 227 | model.base = "main" | |
| 228 | model.head = "nope" | |
| 229 | await model.compare() | |
| 230 | ||
| 231 | guard case .failed = model.state else { | |
| 232 | Testing.Issue.record("expected failed, got \(model.state)") | |
| 233 | return | |
| 234 | } | |
| 235 | } | |
| 236 | ||
| 237 | /// The realistic failure from `repo diff` on an unresolvable ref is | |
| 238 | /// exit 3, which `GitbayError.isEmptyState` maps to `.empty`, not | |
| 239 | /// `.failed` — pinning that mapping rather than assuming it. | |
| 240 | @Test func anUnresolvableRefIsEmptyNotFailed() async throws { | |
| 241 | let (client, stub) = try makeClient() | |
| 242 | stub.enqueue(.init(status: 200, json: """ | |
| 243 | {"protocol_version":1,"error":"unknown revision","exit_code":3} | |
| 244 | """)) | |
| 245 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 246 | model.base = "main" | |
| 247 | model.head = "nope" | |
| 248 | await model.compare() | |
| 249 | ||
| 250 | guard case .empty = model.state else { | |
| 251 | Testing.Issue.record("expected empty, got \(model.state)") | |
| 252 | return | |
| 253 | } | |
| 254 | } | |
| 255 | ||
| 256 | /// The race from the retarget bug: a compare is outstanding, the user | |
| 257 | /// changes a ref before it resolves, and the outstanding request's | |
| 258 | /// result must not land — it belongs to a ref pairing nobody is | |
| 259 | /// looking at any more. `GatedProtocol` makes the interleaving exact | |
| 260 | /// rather than probable: the request is provably still unanswered when | |
| 261 | /// the ref changes, because nothing has let its response through yet. | |
| 262 | @Test func aStaleCompareDoesNotOverwriteANewerRef() async throws { | |
| 263 | let box = GatedProtocol.box() | |
| 264 | let client = GitbayClient( | |
| 265 | instance: try GitbayInstance(url: "https://gitbay.org"), | |
| 266 | token: "test-token", | |
| 267 | session: box.session() | |
| 268 | ) | |
| 269 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 270 | model.base = "aaa111" | |
| 271 | model.head = "v1" | |
| 272 | ||
| 273 | let task = Task { await model.compare() } | |
| 274 | let release = await box.awaitArrival() // the v1 request has arrived and is now blocked | |
| 275 | ||
| 276 | guard case .loading = model.state else { | |
| 277 | Testing.Issue.record("expected .loading while the request is outstanding, got \(model.state)") | |
| 278 | release.signal() | |
| 279 | await task.value | |
| 280 | return | |
| 281 | } | |
| 282 | ||
| 283 | model.head = "v2" // ref changed while the v1 request is still in flight | |
| 284 | release.signal() // let the v1 response through now that it's stale | |
| 285 | await task.value | |
| 286 | ||
| 287 | #expect(model.state.value == nil) | |
| 288 | #expect(model.mergeBase == nil) | |
| 289 | #expect(model.truncated == false) | |
| 290 | } | |
| 291 | ||
| 292 | /// A superseded compare must not leave the screen presenting as | |
| 293 | /// in-flight forever: the ref change has to clear `.loading` | |
| 294 | /// immediately, and the discarded response arriving afterward must | |
| 295 | /// not put it back. | |
| 296 | @Test func aSupersededCompareStopsPresentingAsInFlight() async throws { | |
| 297 | let box = GatedProtocol.box() | |
| 298 | let client = GitbayClient( | |
| 299 | instance: try GitbayInstance(url: "https://gitbay.org"), | |
| 300 | token: "test-token", | |
| 301 | session: box.session() | |
| 302 | ) | |
| 303 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 304 | model.base = "aaa111" | |
| 305 | model.head = "v1" | |
| 306 | ||
| 307 | let task = Task { await model.compare() } | |
| 308 | let release = await box.awaitArrival() | |
| 309 | ||
| 310 | guard case .loading = model.state else { | |
| 311 | Testing.Issue.record("expected .loading while the request is outstanding, got \(model.state)") | |
| 312 | release.signal() | |
| 313 | await task.value | |
| 314 | return | |
| 315 | } | |
| 316 | ||
| 317 | model.head = "v2" // ref changed while the v1 request is still in flight | |
| 318 | ||
| 319 | if case .loading = model.state { | |
| 320 | Testing.Issue.record("ref change did not clear .loading immediately") | |
| 321 | } | |
| 322 | release.signal() | |
| 323 | await task.value | |
| 324 | ||
| 325 | if case .loading = model.state { | |
| 326 | Testing.Issue.record("superseded compare's response left the screen stuck presenting as in-flight") | |
| 327 | } | |
| 328 | } | |
| 329 | ||
| 330 | /// A second compare that fails must not show the first compare's | |
| 331 | /// merge base or truncation notice beside the failed state. | |
| 332 | @Test func aFailedRecompareDropsTheStaleResult() async throws { | |
| 333 | let (client, stub) = try makeClient() | |
| 334 | stub.enqueue(.init(status: 200, json: patchJSON)) | |
| 335 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") | |
| 336 | model.base = "main" | |
| 337 | model.head = "feature" | |
| 338 | await model.compare() | |
| 339 | ||
| 340 | #expect(model.mergeBase == "ccc333") | |
| 341 | #expect(model.state.value != nil) | |
| 342 | ||
| 343 | stub.enqueue(.init(status: 200, json: """ | |
| 344 | {"protocol_version":1,"error":"unknown revision","exit_code":1} | |
| 345 | """)) | |
| 346 | await model.compare() | |
| 347 | ||
| 348 | guard case .failed = model.state else { | |
| 349 | Testing.Issue.record("expected failed, got \(model.state)") | |
| 350 | return | |
| 351 | } | |
| 352 | #expect(model.mergeBase == nil) | |
| 353 | #expect(model.truncated == false) | |
| 354 | } | |
| 355 | } | |