Commit 1e17346377
Verified · cmc
Layout: unified · split
docs/superpowers/plans/2026-09-06-mr06-notifications.md added +345
| @@ -0,0 +1,345 @@ | |||
| 1 | # MR 6: Notification inbox 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 `notification inbox` parity row — read the inbox, mark things read, and carry the unread count on the Dashboard. | ||
| 6 | |||
| 7 | **Architecture:** `notifications list` is paginated, so it goes through the existing `PagedListModel` like every other list. `notifications read` marks one or all. The unread badge costs no extra request: `dashboard` already emits `unread` for exactly this purpose and iOS simply does not decode it yet. | ||
| 8 | |||
| 9 | **Spec:** `docs/superpowers/specs/2026-09-06-ios-parity-design.md` | ||
| 10 | |||
| 11 | ## Global Constraints | ||
| 12 | |||
| 13 | - Swift 6 language mode, default `MainActor` isolation. Wire models are `nonisolated struct`s. | ||
| 14 | - Swift Testing only — never XCTest. | ||
| 15 | - `gitbayTests` is hermetic and offline; network goes through `StubProtocol`. | ||
| 16 | - New files under `gitbay/` and `gitbayTests/` need **no** `project.pbxproj` edit. | ||
| 17 | - The label model is `IssueLabel`, never `Label`. | ||
| 18 | - Never mention Claude, LLMs or AI in commits, comments, or the merge request. No `Co-Authored-By` trailer. | ||
| 19 | - Never commit to `main`. | ||
| 20 | |||
| 21 | **The commands, verbatim from the registry:** | ||
| 22 | |||
| 23 | ``` | ||
| 24 | notifications list [--all] [--limit <n>] [--cursor <c>] | ||
| 25 | notifications read <id>... | --all | ||
| 26 | ``` | ||
| 27 | |||
| 28 | `notifications list` **is paginated** — unlike `search` in MR 3 — so it uses `PagedListModel`, which appends `--limit`/`--cursor` itself. The default listing is **unread only**; `--all` includes read ones. | ||
| 29 | |||
| 30 | **The item shape** (`internal/control/notifications.go:117-125`): | ||
| 31 | |||
| 32 | ```json | ||
| 33 | {"id":15,"repo":"krz/gitbay","kind":"build","actor":"ci", | ||
| 34 | "summary":"build 966 failed: test on runner-podman-exec-144", | ||
| 35 | "path":"krz/gitbay/builds/966","created_at":"2026-09-06T22:14:46.912Z", | ||
| 36 | "read_at":"2026-09-06T22:20:24.040Z"} | ||
| 37 | ``` | ||
| 38 | |||
| 39 | Everything is always present except `read_at`, which is `omitempty` — **absent means unread**, and that is the common case for the default listing. | ||
| 40 | |||
| 41 | **Counting tests reliably:** | ||
| 42 | |||
| 43 | ```bash | ||
| 44 | grep -cE "^Test case '[^']*' passed" /tmp/out.txt | ||
| 45 | grep -cE "^Test case '[^']*' failed" /tmp/out.txt | ||
| 46 | grep -cE "^Test case '[^']*' skipped" /tmp/out.txt | ||
| 47 | grep -rhoE "@Test(\([^)]*\))? func" gitbayTests/*.swift | wc -l | ||
| 48 | ``` | ||
| 49 | |||
| 50 | `declared - skipped == passed`. One test is always skipped (`LiveInstanceTests`). | ||
| 51 | |||
| 52 | Baseline: **285 declared, 284 passed, 1 skipped, 0 failed.** | ||
| 53 | |||
| 54 | --- | ||
| 55 | |||
| 56 | ## File Structure | ||
| 57 | |||
| 58 | | File | Responsibility | | ||
| 59 | |------|----------------| | ||
| 60 | | `gitbay/Account/NotificationsViewModel.swift` (create) | `Notification` wire model, routing, and the paged view model | | ||
| 61 | | `gitbayTests/NotificationTests.swift` (create) | Every test in this plan | | ||
| 62 | | `gitbay/Views/Account/NotificationsView.swift` (create) | The inbox screen | | ||
| 63 | | `gitbay/Dashboard/DashboardModels.swift` (modify) | Decode `unread` | | ||
| 64 | | `gitbay/Views/Dashboard/DashboardView.swift` (modify) | Badge + entry point | | ||
| 65 | | `gitbay/Views/Repos/RepoRoute.swift` (modify) | `notifications` case | | ||
| 66 | | `gitbay/ContentView.swift` (modify) | Route to `NotificationsView` | | ||
| 67 | |||
| 68 | --- | ||
| 69 | |||
| 70 | ### Task 1: The model, its routing, and the view model | ||
| 71 | |||
| 72 | **Files:** Create `gitbay/Account/NotificationsViewModel.swift`; test in `gitbayTests/NotificationTests.swift` | ||
| 73 | |||
| 74 | **Interfaces produced:** | ||
| 75 | - `nonisolated struct Notification: Decodable, Sendable, Hashable, Identifiable` — `id: Int64`, `repo: String`, `kind: String`, `actor: String`, `summary: String`, `path: String`, `createdAt: Date`, `readAt: Date?`; `var isUnread: Bool`; `var destination: NotificationDestination?` | ||
| 76 | - `nonisolated enum NotificationDestination: Hashable, Sendable` — `.issue(repo:number:)`, `.mr(repo:number:)`, `.build(repo:number:)` | ||
| 77 | - `@Observable @MainActor final class NotificationsViewModel` — `list: PagedListModel<Notification>`, `showAll: Bool`, `state`, `actionError`, `working`, `load()`, `markRead(_:)`, `markAllRead()` | ||
| 78 | |||
| 79 | **The routing table**, derived from the server (`internal/control/issue.go:153,279,349`, `mr.go:322,702,778,832,1173,1198,1382`, `diffcomment.go:111`, `build.go:522`, `deps/worker.go:249`) and confirmed against live data. `path` is a **web-style route**, `<owner>/<repo>/<section>/<n>`: | ||
| 80 | |||
| 81 | | `kind` | `path` | destination | | ||
| 82 | |---------|------------------------------|-------------| | ||
| 83 | | `issue` | `<owner>/<repo>/issues/<n>` | `.issue` | | ||
| 84 | | `mr` | `<owner>/<repo>/mrs/<n>` | `.mr` | | ||
| 85 | | `build` | `<owner>/<repo>/builds/<n>` | `.build` | | ||
| 86 | |||
| 87 | **The merge request segment is `mrs`, not `merge_requests`.** Matching the wrong spelling produces a silently untappable row — no error, no crash, just a row that does nothing. | ||
| 88 | |||
| 89 | Parse defensively: split on `/`, require exactly four components, require the last to be an integer, and return `nil` for anything else. A `nil` destination renders an untappable row rather than crashing — the same rule the search results screen follows for a missing number. | ||
| 90 | |||
| 91 | - [ ] **Step 1: Write the failing tests** | ||
| 92 | |||
| 93 | Create `gitbayTests/NotificationTests.swift`: | ||
| 94 | |||
| 95 | ```swift | ||
| 96 | import Foundation | ||
| 97 | import Testing | ||
| 98 | @testable import gitbay | ||
| 99 | |||
| 100 | private func makeClient() throws -> (GitbayClient, StubProtocol.Box) { | ||
| 101 | let box = StubProtocol.box() | ||
| 102 | let client = GitbayClient( | ||
| 103 | instance: try GitbayInstance(url: "https://gitbay.org"), | ||
| 104 | token: "test-token", | ||
| 105 | session: box.session() | ||
| 106 | ) | ||
| 107 | return (client, box) | ||
| 108 | } | ||
| 109 | |||
| 110 | private func argvFrom(_ url: URL) -> [String] { | ||
| 111 | URLComponents(url: url, resolvingAgainstBaseURL: false)? | ||
| 112 | .queryItems?.filter { $0.name == "argv" }.compactMap(\.value) ?? [] | ||
| 113 | } | ||
| 114 | |||
| 115 | private func argvOf(_ seen: StubProtocol.Seen) throws -> [String] { | ||
| 116 | let body = try #require(try JSONSerialization.jsonObject(with: seen.body) as? [String: Any]) | ||
| 117 | return try #require(body["argv"] as? [String]) | ||
| 118 | } | ||
| 119 | |||
| 120 | private func decode(_ json: String) throws -> Notification { | ||
| 121 | let decoder = JSONDecoder() | ||
| 122 | decoder.dateDecodingStrategy = .iso8601 | ||
| 123 | return try decoder.decode(Notification.self, from: Data(json.utf8)) | ||
| 124 | } | ||
| 125 | |||
| 126 | private let unreadBuild = """ | ||
| 127 | {"id":15,"repo":"krz/gitbay","kind":"build","actor":"ci",\ | ||
| 128 | "summary":"build 966 failed","path":"krz/gitbay/builds/966",\ | ||
| 129 | "created_at":"2026-09-06T22:14:46Z"} | ||
| 130 | """ | ||
| 131 | |||
| 132 | struct NotificationDecodingTests { | ||
| 133 | |||
| 134 | /// read_at is omitempty; absent means unread, and that is the common | ||
| 135 | /// case for the default listing. | ||
| 136 | @Test func anAbsentReadAtMeansUnread() throws { | ||
| 137 | let n = try decode(unreadBuild) | ||
| 138 | #expect(n.readAt == nil) | ||
| 139 | #expect(n.isUnread) | ||
| 140 | } | ||
| 141 | |||
| 142 | @Test func aPresentReadAtMeansRead() throws { | ||
| 143 | let n = try decode(""" | ||
| 144 | {"id":15,"repo":"krz/gitbay","kind":"build","actor":"ci",\ | ||
| 145 | "summary":"x","path":"krz/gitbay/builds/966",\ | ||
| 146 | "created_at":"2026-09-06T22:14:46Z","read_at":"2026-09-06T22:20:24Z"} | ||
| 147 | """) | ||
| 148 | #expect(n.readAt != nil) | ||
| 149 | #expect(n.isUnread == false) | ||
| 150 | } | ||
| 151 | |||
| 152 | @Test func everyKindRoutesToItsScreen() throws { | ||
| 153 | let issue = try decode(""" | ||
| 154 | {"id":1,"repo":"krz/gitbay","kind":"issue","actor":"cmc","summary":"x",\ | ||
| 155 | "path":"krz/gitbay/issues/168","created_at":"2026-09-06T22:14:46Z"} | ||
| 156 | """) | ||
| 157 | #expect(issue.destination == .issue(repo: "krz/gitbay", number: 168)) | ||
| 158 | |||
| 159 | // The segment is "mrs", not "merge_requests". | ||
| 160 | let mr = try decode(""" | ||
| 161 | {"id":2,"repo":"krz/gitbay","kind":"mr","actor":"cmc","summary":"x",\ | ||
| 162 | "path":"krz/gitbay/mrs/282","created_at":"2026-09-06T22:14:46Z"} | ||
| 163 | """) | ||
| 164 | #expect(mr.destination == .mr(repo: "krz/gitbay", number: 282)) | ||
| 165 | |||
| 166 | let build = try decode(unreadBuild) | ||
| 167 | #expect(build.destination == .build(repo: "krz/gitbay", number: 966)) | ||
| 168 | } | ||
| 169 | |||
| 170 | /// A path the app cannot route must yield nil, not a crash and not a | ||
| 171 | /// wrong destination. | ||
| 172 | @Test func anUnroutablePathIsNil() throws { | ||
| 173 | for path in ["krz/gitbay/wiki/Home", "krz/gitbay/issues", "krz/gitbay/issues/notanumber", | ||
| 174 | "too/short", "krz/gitbay/issues/1/extra", ""] { | ||
| 175 | let n = try decode(""" | ||
| 176 | {"id":1,"repo":"krz/gitbay","kind":"issue","actor":"cmc","summary":"x",\ | ||
| 177 | "path":"\(path)","created_at":"2026-09-06T22:14:46Z"} | ||
| 178 | """) | ||
| 179 | #expect(n.destination == nil, "\(path) should not route") | ||
| 180 | } | ||
| 181 | } | ||
| 182 | |||
| 183 | /// The repo comes from the path, not the `repo` field, so a | ||
| 184 | /// mismatch cannot send the user to the wrong repository. | ||
| 185 | @Test func theDestinationRepoComesFromThePath() throws { | ||
| 186 | let n = try decode(""" | ||
| 187 | {"id":1,"repo":"other/repo","kind":"issue","actor":"cmc","summary":"x",\ | ||
| 188 | "path":"krz/gitbay/issues/7","created_at":"2026-09-06T22:14:46Z"} | ||
| 189 | """) | ||
| 190 | #expect(n.destination == .issue(repo: "krz/gitbay", number: 7)) | ||
| 191 | } | ||
| 192 | } | ||
| 193 | |||
| 194 | @MainActor | ||
| 195 | struct NotificationsViewModelTests { | ||
| 196 | |||
| 197 | private let page = """ | ||
| 198 | {"protocol_version":1,"data":{"items":[\(unreadBuild)]},"exit_code":0} | ||
| 199 | """ | ||
| 200 | |||
| 201 | @Test func theDefaultListingIsUnreadOnly() async throws { | ||
| 202 | let (client, stub) = try makeClient() | ||
| 203 | stub.enqueue(.init(status: 200, json: page)) | ||
| 204 | let model = NotificationsViewModel(client: client) | ||
| 205 | await model.load() | ||
| 206 | |||
| 207 | let argv = argvFrom(try #require(stub.seen.last).url) | ||
| 208 | #expect(argv.prefix(2) == ["notifications", "list"]) | ||
| 209 | #expect(argv.contains("--all") == false) | ||
| 210 | #expect(model.state.value?.count == 1) | ||
| 211 | } | ||
| 212 | |||
| 213 | @Test func showingAllAddsTheFlag() async throws { | ||
| 214 | let (client, stub) = try makeClient() | ||
| 215 | stub.enqueue(.init(status: 200, json: page)) | ||
| 216 | let model = NotificationsViewModel(client: client) | ||
| 217 | await model.load() | ||
| 218 | |||
| 219 | stub.enqueue(.init(status: 200, json: page)) | ||
| 220 | model.showAll = true | ||
| 221 | try await Task.sleep(for: .milliseconds(150)) | ||
| 222 | |||
| 223 | #expect(argvFrom(try #require(stub.seen.last).url).contains("--all")) | ||
| 224 | } | ||
| 225 | |||
| 226 | @Test func markingOneReadSendsItsId() async throws { | ||
| 227 | let (client, stub) = try makeClient() | ||
| 228 | stub.enqueue(.init(status: 200, json: page)) | ||
| 229 | let model = NotificationsViewModel(client: client) | ||
| 230 | await model.load() | ||
| 231 | |||
| 232 | stub.enqueue(.init(status: 200, json: """ | ||
| 233 | {"protocol_version":1,"exit_code":0} | ||
| 234 | """)) | ||
| 235 | stub.enqueue(.init(status: 200, json: page)) | ||
| 236 | await model.markRead(15) | ||
| 237 | |||
| 238 | let write = try #require(stub.seen.first { $0.method == "POST" }) | ||
| 239 | #expect(try argvOf(write) == ["notifications", "read", "15"]) | ||
| 240 | } | ||
| 241 | |||
| 242 | @Test func markingAllReadUsesTheFlagNotAList() async throws { | ||
| 243 | let (client, stub) = try makeClient() | ||
| 244 | stub.enqueue(.init(status: 200, json: page)) | ||
| 245 | let model = NotificationsViewModel(client: client) | ||
| 246 | await model.load() | ||
| 247 | |||
| 248 | stub.enqueue(.init(status: 200, json: """ | ||
| 249 | {"protocol_version":1,"exit_code":0} | ||
| 250 | """)) | ||
| 251 | stub.enqueue(.init(status: 200, json: page)) | ||
| 252 | await model.markAllRead() | ||
| 253 | |||
| 254 | let write = try #require(stub.seen.first { $0.method == "POST" }) | ||
| 255 | #expect(try argvOf(write) == ["notifications", "read", "--all"]) | ||
| 256 | } | ||
| 257 | |||
| 258 | @Test func anEmptyInboxIsAnEmptyStateNotAFailure() async throws { | ||
| 259 | let (client, stub) = try makeClient() | ||
| 260 | stub.enqueue(.init(status: 200, json: """ | ||
| 261 | {"protocol_version":1,"data":{"items":[]},"exit_code":0} | ||
| 262 | """)) | ||
| 263 | let model = NotificationsViewModel(client: client) | ||
| 264 | await model.load() | ||
| 265 | |||
| 266 | guard case .empty = model.state else { | ||
| 267 | Testing.Issue.record("expected empty, got \(model.state)") | ||
| 268 | return | ||
| 269 | } | ||
| 270 | } | ||
| 271 | |||
| 272 | @Test func aRefusalSurfacesAndDoesNotReload() async throws { | ||
| 273 | let (client, stub) = try makeClient() | ||
| 274 | stub.enqueue(.init(status: 200, json: page)) | ||
| 275 | let model = NotificationsViewModel(client: client) | ||
| 276 | await model.load() | ||
| 277 | let before = stub.seen.count | ||
| 278 | |||
| 279 | stub.enqueue(.init(status: 200, json: """ | ||
| 280 | {"protocol_version":1,"error":"denied","exit_code":4} | ||
| 281 | """)) | ||
| 282 | await model.markRead(15) | ||
| 283 | |||
| 284 | #expect(model.actionError?.isEmpty == false) | ||
| 285 | #expect(model.working == false) | ||
| 286 | // One POST, no reload after a refusal. | ||
| 287 | #expect(stub.seen.count == before + 1) | ||
| 288 | } | ||
| 289 | } | ||
| 290 | ``` | ||
| 291 | |||
| 292 | - [ ] **Step 2: Run to verify failure.** | ||
| 293 | - [ ] **Step 3: Implement.** Follow `IssueListViewModel` for the `PagedListModel` shape and `RepoSettingsViewModel.perform(argv:)` for the write half (copy it; do not extract a shared helper — that duplication is a standing decision in this codebase). | ||
| 294 | - [ ] **Step 4: Run the tests.** | ||
| 295 | - [ ] **Step 5: Commit** — `git commit -m "Notification inbox model and view model"` | ||
| 296 | |||
| 297 | --- | ||
| 298 | |||
| 299 | ### Task 2: The screen, the badge and the route | ||
| 300 | |||
| 301 | **Files:** Create `gitbay/Views/Account/NotificationsView.swift`; modify `DashboardModels.swift`, `DashboardView.swift`, `RepoRoute.swift`, `ContentView.swift` | ||
| 302 | |||
| 303 | **The badge is free.** `dashboard` already emits `unread: int` — the server comments it as existing "so a client showing one does not need a second read to fill it" (`internal/control/dashboard.go:78`). iOS does not decode it yet. | ||
| 304 | |||
| 305 | **`DashboardData` has a hand-written `init(from:)`** (`DashboardModels.swift:27-37`) that decodes each field with `decodeIfPresent ?? []`. Adding `unread` means editing that initialiser — check field by field that you drop nothing, the same care MR 4's `MRDetail` initialiser needed. Decode it as `decodeIfPresent(Int.self, forKey: .unread) ?? 0`, since a server predating the field would omit it. | ||
| 306 | |||
| 307 | **The screen:** a `List` over the paged notifications with `PageFooter`, a toggle for unread-only versus all, a "Mark all read" action, and per-row swipe-to-mark-read. An unread row should read as unread — the web uses the count, so weight or a dot is enough; do not invent a colour that means nothing elsewhere in the app. | ||
| 308 | |||
| 309 | Each row shows `actor`, `summary` and the relative `createdAt`, and navigates to `destination` when it is non-nil. A row whose destination is `nil` renders plainly and is not tappable. | ||
| 310 | |||
| 311 | **Entry point:** a toolbar bell on the Dashboard carrying the unread count, matching where the web puts it (its rail). `DashboardView` already has a `.toolbar` with a `.topBarTrailing` item — add beside it. | ||
| 312 | |||
| 313 | Add `RepoRoute.notifications` and its `ContentView` destination. | ||
| 314 | |||
| 315 | - [ ] **Step 1: Decode `unread` in `DashboardData`** | ||
| 316 | - [ ] **Step 2: `NotificationsView`** | ||
| 317 | - [ ] **Step 3: Route case + `ContentView` destination** | ||
| 318 | - [ ] **Step 4: Dashboard bell with the count** | ||
| 319 | - [ ] **Step 5: Build and run the full suite.** No drop. | ||
| 320 | - [ ] **Step 6: Commit** — `git commit -m "Notification inbox screen and dashboard badge"` | ||
| 321 | |||
| 322 | --- | ||
| 323 | |||
| 324 | ### Task 3: Flip the parity row and open the merge request | ||
| 325 | |||
| 326 | - [ ] **Step 1:** In the Accounts table, `notification inbox` goes to `yes` for iOS. Touch no other row. | ||
| 327 | |||
| 328 | 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. | ||
| 329 | |||
| 330 | - [ ] **Step 2:** Run the full suite; record the real number. | ||
| 331 | - [ ] **Step 3:** Open the merge request. | ||
| 332 | |||
| 333 | --- | ||
| 334 | |||
| 335 | ## Notes for whoever executes this | ||
| 336 | |||
| 337 | **`notifications list` IS paginated.** Use `PagedListModel`. This differs from `search` in MR 3, which is not. | ||
| 338 | |||
| 339 | **The merge request path segment is `mrs`.** Not `merge_requests`. The wrong spelling gives a silently untappable row. | ||
| 340 | |||
| 341 | **Absent `read_at` means unread**, and it is the common case. Do not require the key. | ||
| 342 | |||
| 343 | **Take the repo from the path, not the `repo` field** — a test pins this, so a row can never send the user to a different repository than the one its link names. | ||
| 344 | |||
| 345 | **The badge needs no extra request.** If you find yourself adding a `notifications list` call to fill the Dashboard count, stop: `dashboard` already carries it. | ||