Commit 89df1fc979

89df1fc979d23107104ae69b3afc445fc5e124c0

parent: 34e06cf562

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-06 18:35 UTC

Fix label-list findings from final review on labels-colour

Fetch label list at most once per IssueDetailViewModel/IssueListViewModel
instance instead of on every write-triggered reload, skip the label read
entirely when the primary read fails, and drop the unused
IssueDetailViewModel.labels property in favor of building colors directly.
Adds regression coverage in LabelTests.swift for all three, plus a failure
test for LabelListViewModel.load().

Layout: unified · split

gitbay/Issues/IssueDetailViewModel.swift +7 −3
@@ -12,8 +12,10 @@ final class IssueDetailViewModel {
12 private(set) var working = false 12 private(set) var working = false
13 /// Open milestones for the picker; fetched on first use. 13 /// Open milestones for the picker; fetched on first use.
14 private(set) var availableMilestones: [Milestone]? 14 private(set) var availableMilestones: [Milestone]?
15 private(set) var labels: [IssueLabel] = []
16 private(set) var colors = LabelColors() 15 private(set) var colors = LabelColors()
16 /// Labels rarely change mid-session, and every write reloads via
17 /// `perform(argv:)`: fetch them once per view model, not on every reload.
18 private var labelsLoaded = false
17 19
18 private let client: GitbayClient 20 private let client: GitbayClient
19 let repoPath: String 21 let repoPath: String
@@ -32,9 +34,11 @@ final class IssueDetailViewModel {
32 state = .loaded(try await client.read(["issue", "show"] + ref, as: IssueDetail.self)) 34 state = .loaded(try await client.read(["issue", "show"] + ref, as: IssueDetail.self))
33 } catch { 35 } catch {
34 state = .from(error) 36 state = .from(error)
37 return
35 } 38 }
36 labels = await labelList() 39 guard !labelsLoaded else { return }
37 colors = LabelColors(labels) 40 labelsLoaded = true
41 colors = LabelColors(await labelList())
38 } 42 }
39 43
40 private func labelList() async -> [IssueLabel] { 44 private func labelList() async -> [IssueLabel] {
gitbay/Issues/IssueListViewModel.swift +6
@@ -27,6 +27,9 @@ final class IssueListViewModel {
27 /// every chip then derives its colour from its name. 27 /// every chip then derives its colour from its name.
28 private(set) var labels: [IssueLabel] = [] 28 private(set) var labels: [IssueLabel] = []
29 private(set) var colors = LabelColors() 29 private(set) var colors = LabelColors()
30 /// Labels rarely change mid-session: fetch them once per view model,
31 /// not on every reload.
32 private var labelsLoaded = false
30 33
31 init(client: GitbayClient, repoPath: String) { 34 init(client: GitbayClient, repoPath: String) {
32 self.client = client 35 self.client = client
@@ -42,6 +45,9 @@ final class IssueListViewModel {
42 45
43 func load() async { 46 func load() async {
44 await list.reload() 47 await list.reload()
48 if case .failed = list.state { return }
49 guard !labelsLoaded else { return }
50 labelsLoaded = true
45 labels = await labelList() 51 labels = await labelList()
46 colors = LabelColors(labels) 52 colors = LabelColors(labels)
47 } 53 }
gitbayTests/LabelTests.swift +65
@@ -182,6 +182,56 @@ struct IssueDetailColorsTests {
182 #expect(model.state.value?.number == 7) 182 #expect(model.state.value?.number == 7)
183 #expect(model.colors.hex("bug") == "#cf222e") 183 #expect(model.colors.hex("bug") == "#cf222e")
184 } 184 }
185
186 /// Decoration must not take the screen down with it.
187 @Test func issueStillLoadsWhenTheLabelReadFails() async throws {
188 let (client, stub) = try makeClient()
189 stub.enqueue(.init(status: 200, json: """
190 {"protocol_version":1,"data":{"number":7,"title":"a bug","state":"open",\
191 "author":"cmc","labels":["bug"],"created_at":"2026-09-01T00:00:00Z"},\
192 "exit_code":0}
193 """, match: "argv=show"))
194 stub.enqueue(.init(status: 403, json: """
195 {"protocol_version":1,"error":"denied","exit_code":4}
196 """, match: "argv=label"))
197
198 let model = IssueDetailViewModel(client: client, repoPath: "krz/gitbay", number: 7)
199 await model.load()
200
201 #expect(model.state.value?.number == 7)
202 // Derived from the name, since nothing was stored.
203 #expect(model.colors.hex("bug") == "#b93a86")
204 }
205
206 /// Regression test: `perform(argv:)` used to reload labels on every
207 /// write. Labels rarely change mid-session, so the read must happen
208 /// at most once per view model instance.
209 @Test func aWriteDoesNotReissueLabelList() async throws {
210 let (client, stub) = try makeClient()
211 stub.enqueue(.init(status: 200, json: """
212 {"protocol_version":1,"data":{"number":7,"title":"a bug","state":"open",\
213 "author":"cmc","labels":["bug"],"created_at":"2026-09-01T00:00:00Z"},\
214 "exit_code":0}
215 """, match: "argv=show"))
216 stub.enqueue(.init(status: 200, json: labelListJSON, match: "argv=label"))
217
218 let model = IssueDetailViewModel(client: client, repoPath: "krz/gitbay", number: 7)
219 await model.load()
220
221 stub.enqueue(.init(status: 200, json: """
222 {"protocol_version":1,"exit_code":0}
223 """, match: "argv=close"))
224 stub.enqueue(.init(status: 200, json: """
225 {"protocol_version":1,"data":{"number":7,"title":"a bug","state":"closed",\
226 "author":"cmc","labels":["bug"],"created_at":"2026-09-01T00:00:00Z"},\
227 "exit_code":0}
228 """, match: "argv=show"))
229
230 await model.close()
231
232 let labelReads = stub.seen.filter { ($0.url.query() ?? "").contains("argv=label") }
233 #expect(labelReads.count == 1)
234 }
185} 235}
186 236
187@MainActor 237@MainActor
@@ -284,4 +334,19 @@ struct LabelListViewModelTests {
284 #expect(model.actionError?.isEmpty == false) 334 #expect(model.actionError?.isEmpty == false)
285 #expect(model.working == false) 335 #expect(model.working == false)
286 } 336 }
337
338 @Test func aServerFailureSurfacesAsFailed() async throws {
339 let (client, stub) = try makeClient()
340 stub.enqueue(.init(status: 200, json: """
341 {"protocol_version":1,"error":"boom","exit_code":1}
342 """))
343 let model = LabelListViewModel(client: client, repoPath: "krz/gitbay")
344
345 await model.load()
346
347 guard case .failed = model.state else {
348 Testing.Issue.record("expected a failed state, got \(model.state)")
349 return
350 }
351 }
287} 352}