Commit 2b5b408a5c
2b5b408a5cfbfe39c3d7d11436022dfba2c744a1
parent: c6150bdc21
Verified · cmc
cmc <hello@cleberg.net> · 2026-09-06 22:17 UTC
Fix compare-refs loading overlay and stale-result review findings
The loading overlay covered the ref pickers on empty/failed states,
leaving no way to pick different refs or retry. Initialize state to
.empty instead of .loading, drop the hasCompared/isComparing view flags,
and render .empty/.failed inline as GBNotice so the overlay only covers
.loading.
Also reset state/mergeBase/truncated at the start of compare() and on
any base/head change, so a second compare or a ref change mid-flight
can't pair stale results with a new state. Clear refs alongside
refsError in loadRefs() for the same reason.
Add tests pinning the exit-3-is-empty-not-failed mapping, the reset
behavior across a failing re-compare, and the missing patch assertion
in decodesEveryField.
Layout: unified · split
gitbay/Repos/CompareViewModel.swift
+17 −3
| @@ -22,9 +22,11 @@ nonisolated struct RepoDiff: Decodable, Sendable, Hashable { |
| 22 | 22 | @MainActor |
| 23 | 23 | final class CompareViewModel { |
| 24 | 24 | |
| 25 | | var base = "" |
| 26 | | var head = "" |
| 27 | | private(set) var state: LoadState<UnifiedDiff> = .loading |
| 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) |
| 28 | 30 | private(set) var truncated = false |
| 29 | 31 | private(set) var mergeBase: String? |
| 30 | 32 | |
| @@ -44,6 +46,7 @@ final class CompareViewModel { |
| 44 | 46 | |
| 45 | 47 | func loadRefs() async { |
| 46 | 48 | refsError = nil |
| 49 | refs = nil |
| 47 | 50 | do { |
| 48 | 51 | refs = try await client.read(["repo", "refs", repoPath], as: RepoRefs.self) |
| 49 | 52 | } catch let error as GitbayError { |
| @@ -53,12 +56,23 @@ final class CompareViewModel { |
| 53 | 56 | } |
| 54 | 57 | } |
| 55 | 58 | |
| 59 | /// Clears whatever the previous compare left behind, so a re-compare |
| 60 | /// (or a ref changed mid-flight) can never pair a stale merge base or |
| 61 | /// truncation notice with the new state. |
| 62 | private func resetResults() { |
| 63 | state = .empty(Self.notComparedYet) |
| 64 | mergeBase = nil |
| 65 | truncated = false |
| 66 | } |
| 67 | |
| 56 | 68 | func compare() async { |
| 57 | 69 | let base = base.trimmingCharacters(in: .whitespaces) |
| 58 | 70 | let head = head.trimmingCharacters(in: .whitespaces) |
| 59 | 71 | guard !base.isEmpty, !head.isEmpty else { return } |
| 60 | 72 | |
| 61 | 73 | state = .loading |
| 74 | mergeBase = nil |
| 75 | truncated = false |
| 62 | 76 | do { |
| 63 | 77 | let diff = try await client.read(["repo", "diff", repoPath, base, head], as: RepoDiff.self) |
| 64 | 78 | truncated = diff.truncated |
gitbay/Views/Repos/CompareView.swift
+19 −10
| @@ -6,12 +6,6 @@ import SwiftUI |
| 6 | 6 | struct CompareView: View { |
| 7 | 7 | |
| 8 | 8 | @State private var model: CompareViewModel |
| 9 | | /// `model.state` defaults to `.loading` before any comparison is |
| 10 | | /// requested, since `compare()` is manually triggered rather than |
| 11 | | /// run from `.task`. This tracks whether that default is a real |
| 12 | | /// request in flight, so the overlay doesn't spin before the user |
| 13 | | /// has picked anything. |
| 14 | | @State private var hasCompared = false |
| 15 | 9 | |
| 16 | 10 | init(client: GitbayClient, repo: String, defaultBranch: String? = nil) { |
| 17 | 11 | let model = CompareViewModel(client: client, repoPath: repo) |
| @@ -22,8 +16,8 @@ struct CompareView: View { |
| 22 | 16 | } |
| 23 | 17 | |
| 24 | 18 | private var isComparing: Bool { |
| 25 | | guard hasCompared, case .loading = model.state else { return false } |
| 26 | | return true |
| 19 | if case .loading = model.state { return true } |
| 20 | return false |
| 27 | 21 | } |
| 28 | 22 | |
| 29 | 23 | var body: some View { |
| @@ -32,7 +26,6 @@ struct CompareView: View { |
| 32 | 26 | |
| 33 | 27 | Section { |
| 34 | 28 | Button("Compare") { |
| 35 | | hasCompared = true |
| 36 | 29 | Task { await model.compare() } |
| 37 | 30 | } |
| 38 | 31 | .disabled(isComparing |
| @@ -59,6 +52,22 @@ struct CompareView: View { |
| 59 | 52 | } |
| 60 | 53 | } |
| 61 | 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 | |
| 62 | 71 | if let diff = model.state.value, !diff.files.isEmpty { |
| 63 | 72 | ForEach(diff.files) { file in |
| 64 | 73 | DiffFileSection(file: file) |
| @@ -66,7 +75,7 @@ struct CompareView: View { |
| 66 | 75 | } |
| 67 | 76 | } |
| 68 | 77 | .overlay { |
| 69 | | if hasCompared { |
| 78 | if isComparing { |
| 70 | 79 | LoadStateOverlay(state: model.state) |
| 71 | 80 | } |
| 72 | 81 | } |
gitbayTests/CompareTests.swift
+46
| @@ -34,6 +34,7 @@ struct RepoDiffDecodingTests { |
| 34 | 34 | #expect(diff.base == "aaa111") |
| 35 | 35 | #expect(diff.head == "bbb222") |
| 36 | 36 | #expect(diff.mergeBase == "ccc333") |
| 37 | #expect(diff.patch == "x") |
| 37 | 38 | #expect(diff.truncated) |
| 38 | 39 | } |
| 39 | 40 | } |
| @@ -154,4 +155,49 @@ struct CompareViewModelTests { |
| 154 | 155 | return |
| 155 | 156 | } |
| 156 | 157 | } |
| 158 | |
| 159 | /// The realistic failure from `repo diff` on an unresolvable ref is |
| 160 | /// exit 3, which `GitbayError.isEmptyState` maps to `.empty`, not |
| 161 | /// `.failed` — pinning that mapping rather than assuming it. |
| 162 | @Test func anUnresolvableRefIsEmptyNotFailed() async throws { |
| 163 | let (client, stub) = try makeClient() |
| 164 | stub.enqueue(.init(status: 200, json: """ |
| 165 | {"protocol_version":1,"error":"unknown revision","exit_code":3} |
| 166 | """)) |
| 167 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") |
| 168 | model.base = "main" |
| 169 | model.head = "nope" |
| 170 | await model.compare() |
| 171 | |
| 172 | guard case .empty = model.state else { |
| 173 | Testing.Issue.record("expected empty, got \(model.state)") |
| 174 | return |
| 175 | } |
| 176 | } |
| 177 | |
| 178 | /// A second compare that fails must not show the first compare's |
| 179 | /// merge base or truncation notice beside the failed state. |
| 180 | @Test func aFailedRecompareDropsTheStaleResult() async throws { |
| 181 | let (client, stub) = try makeClient() |
| 182 | stub.enqueue(.init(status: 200, json: patchJSON)) |
| 183 | let model = CompareViewModel(client: client, repoPath: "krz/gitbay") |
| 184 | model.base = "main" |
| 185 | model.head = "feature" |
| 186 | await model.compare() |
| 187 | |
| 188 | #expect(model.mergeBase == "ccc333") |
| 189 | #expect(model.state.value != nil) |
| 190 | |
| 191 | stub.enqueue(.init(status: 200, json: """ |
| 192 | {"protocol_version":1,"error":"unknown revision","exit_code":1} |
| 193 | """)) |
| 194 | await model.compare() |
| 195 | |
| 196 | guard case .failed = model.state else { |
| 197 | Testing.Issue.record("expected failed, got \(model.state)") |
| 198 | return |
| 199 | } |
| 200 | #expect(model.mergeBase == nil) |
| 201 | #expect(model.truncated == false) |
| 202 | } |
| 157 | 203 | } |