docs/superpowers/plans/2026-09-06-mr05-compare-refs.md
300 lines · 13882 bytes
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
13The 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
15The 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
20So 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```
35repo diff <owner/name> <base> <head>
36```
37
38Both 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
49grep -cE "^Test case '[^']*' passed" /tmp/out.txt
50grep -cE "^Test case '[^']*' failed" /tmp/out.txt
51grep -rhoE "@Test(\([^)]*\))? func" gitbayTests/*.swift | wc -l # declared
52```
53
54Declared 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
79Behaviour, each pinned by a test:
801. `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.
812. Comparing a ref with itself yields an empty patch, which is an `.empty` state, not `.failed`.
823. `truncated` is decoded and exposed. A partial patch rendered as though it were whole is a bug the user cannot see.
834. `mergeBase` is decoded and exposed, so the screen can say what the comparison is against.
845. Either ref being blank issues **no request** — there is nothing to compare yet.
856. `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
89Create `gitbayTests/CompareTests.swift`:
90
91```swift
92import Foundation
93import Testing
94@testable import gitbay
95
96private 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
106private func argvFrom(_ url: URL) -> [String] {
107 URLComponents(url: url, resolvingAgainstBaseURL: false)?
108 .queryItems?.filter { $0.name == "argv" }.compactMap(\.value) ?? []
109}
110
111private 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
117struct 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
133struct 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
262No unit tests — UI.
263
264**Reuse `DiffFileSection(file:)` with no model**, exactly as `CommitView.swift:71` does. Do not modify it.
265
266The 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:**
2691. **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.
2702. **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.
2713. **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
273Add `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
287Land 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.