Commit 51ce3d7cd4
Verified · cmc
Layout: unified · split
docs/superpowers/plans/2026-09-07-mr12-repo-state.md added +195
| @@ -0,0 +1,195 @@ | ||
| 1 | # MR 12: Render the repository state the server now reports | |
| 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:** Replace the stateless watch/mute buttons with a control that shows where you actually are, drop the extra request the bookmark state costs, and say when a repository is a fork. | |
| 6 | ||
| 7 | **Why now:** `krz/gitbay#178` — filed while building `!53`, because watch, bookmark and fork state were readable from no command — has been implemented upstream. `repo show` now carries all three. `!53` shipped a weaker affordance purely because of that gap, and the gap is gone. | |
| 8 | ||
| 9 | **No parity row moves.** All three rows are already `yes`; this improves what closed them. | |
| 10 | ||
| 11 | ## What the server now reports | |
| 12 | ||
| 13 | From `internal/control/repo.go:288-299`, all three `omitempty`: | |
| 14 | ||
| 15 | ```go | |
| 16 | // ForkOf names the parent only when the caller can read it: a | |
| 17 | // private parent is not confirmed to exist, here as anywhere. | |
| 18 | ForkOf string `json:"fork_of,omitempty"` | |
| 19 | // Watch and Bookmarked are the caller's own state, so a client | |
| 20 | // can draw a toggle rather than two stateless buttons (#178). | |
| 21 | Watch string `json:"watch,omitempty"` // watching, muted, or absent | |
| 22 | Bookmarked bool `json:"bookmarked,omitempty"` | |
| 23 | ``` | |
| 24 | ||
| 25 | Three consequences that shape the whole design: | |
| 26 | ||
| 27 | 1. **`watch` has three states, and only two are reachable.** `repo watch` sets `watching`; `repo unwatch` sets `muted` (`internal/control/notifications.go:177-181`). **No command returns you to the absent default.** So the control must show the current state and offer only the transitions that exist — never a "Default" option with no command behind it. | |
| 28 | 2. **`bookmarked` is `omitempty` on a `Bool`, so absent means false.** Decoding must default it, not require it. | |
| 29 | 3. **Absent `fork_of` does not mean "not a fork."** The server withholds it when the caller cannot read the parent — a private parent is not confirmed to exist. So the UI may say "forked from X" when present, and must say **nothing** when absent. It must never say "not a fork". | |
| 30 | ||
| 31 | ## Global Constraints | |
| 32 | ||
| 33 | - Swift 6 language mode, default `MainActor` isolation. Wire models are `nonisolated struct`s. | |
| 34 | - Swift Testing only — never XCTest. | |
| 35 | - `gitbayTests` is hermetic and offline; network goes through `StubProtocol`. | |
| 36 | - New files under `gitbay/` and `gitbayTests/` need **no** `project.pbxproj` edit. | |
| 37 | - The label model is `IssueLabel`; the notification model is `InboxNotification`. | |
| 38 | - Never mention Claude, LLMs or AI in commits, comments, or the merge request. No `Co-Authored-By` trailer. | |
| 39 | - Never commit to `main`. | |
| 40 | - **Test discipline:** use the `until(_:_:)` helper in `gitbayTests/StubProtocol.swift` rather than fixed sleeps; assert on the request identified by its argv, not `stub.seen.last`; never index `stub.seen` directly, go through `#require`; provision a stub for every request each flow makes. | |
| 41 | ||
| 42 | Baseline: **365 total, 364 passed, 1 skipped, 0 failed.** | |
| 43 | ||
| 44 | --- | |
| 45 | ||
| 46 | ## File Structure | |
| 47 | ||
| 48 | | File | Responsibility | | |
| 49 | |------|----------------| | |
| 50 | | `gitbay/Repos/RepoModels.swift` (modify) | Decode `watch`, `bookmarked`, `fork_of` on `RepoDetail` | | |
| 51 | | `gitbay/Repos/RepoActionsViewModel.swift` (modify) | Delete `loadBookmarkState`; take state from `RepoDetail` | | |
| 52 | | `gitbayTests/RepoActionTests.swift` (modify) | Rewrite the bookmark-state tests | | |
| 53 | | `gitbayTests/RepoStateTests.swift` (create) | Decoding and transition tests | | |
| 54 | | `gitbay/Views/Repos/RepoView.swift` (modify) | The watch control, the bookmark toggle, the fork line | | |
| 55 | ||
| 56 | --- | |
| 57 | ||
| 58 | ### Task 1: Decode the state, and stop fetching it separately | |
| 59 | ||
| 60 | **Files:** `RepoModels.swift`, `RepoActionsViewModel.swift`; tests in `gitbayTests/RepoStateTests.swift` and `RepoActionTests.swift` | |
| 61 | ||
| 62 | **Interfaces produced:** | |
| 63 | - `RepoDetail.watch: String?`, `.bookmarked: Bool`, `.forkOf: String?` | |
| 64 | - `nonisolated enum WatchState: Sendable` — `.watching`, `.muted`, `.default`; `init(_ raw: String?)`; `var available: [WatchAction]` giving only the transitions that exist | |
| 65 | - `RepoActionsViewModel` **loses** `isBookmarked` and `loadBookmarkState()`; the screen reads state from the loaded `RepoDetail` instead | |
| 66 | ||
| 67 | **What to delete, and why.** `!53` added `loadBookmarkState()`, which calls `repo bookmarks` and tests membership — one extra read per repository screen, plus a `.refreshable` hook and a three-state `Bool?` to cover its failure. All of that existed only because the state was unreadable. `repo show` is already fetched by that screen, so the whole mechanism goes. Delete it; do not leave it dual-sourced. | |
| 68 | ||
| 69 | Behaviour, each pinned by a test: | |
| 70 | 1. `bookmarked` is `omitempty` on a Bool — **absent means false**, and must decode, not throw. | |
| 71 | 2. `watch` absent means the default state, which is neither watching nor muted. | |
| 72 | 3. `fork_of` absent means **either** not-a-fork **or** a parent you cannot read. The model exposes it as `String?` and says nothing more. | |
| 73 | 4. `WatchState.available` returns `[.mute]` when watching, `[.watch]` when muted, and **both** when default. It never offers a transition to the default, because no command performs one. | |
| 74 | 5. `watch()` and `unwatch()` still send `repo watch` / `repo unwatch` unchanged, and still reload so the new state is read back from the server rather than assumed. | |
| 75 | ||
| 76 | - [ ] **Step 1: Write the failing tests** | |
| 77 | ||
| 78 | ```swift | |
| 79 | import Foundation | |
| 80 | import Testing | |
| 81 | @testable import gitbay | |
| 82 | ||
| 83 | struct RepoStateDecodingTests { | |
| 84 | ||
| 85 | private func decode(_ json: String) throws -> RepoDetail { | |
| 86 | try JSONDecoder().decode(RepoDetail.self, from: Data(json.utf8)) | |
| 87 | } | |
| 88 | ||
| 89 | private let base = """ | |
| 90 | "path":"krz/gitbay","visibility":"public","default_branch":"main" | |
| 91 | """ | |
| 92 | ||
| 93 | /// All three are omitempty. A repository you have expressed no | |
| 94 | /// opinion about, which is not a fork, carries none of them. | |
| 95 | @Test func aRepositoryWithNoStateDecodes() throws { | |
| 96 | let repo = try decode("{\(base)}") | |
| 97 | #expect(repo.watch == nil) | |
| 98 | #expect(repo.bookmarked == false) | |
| 99 | #expect(repo.forkOf == nil) | |
| 100 | } | |
| 101 | ||
| 102 | @Test func theCallersOwnStateDecodes() throws { | |
| 103 | let repo = try decode(""" | |
| 104 | {\(base),"watch":"watching","bookmarked":true,"fork_of":"upstream/thing"} | |
| 105 | """) | |
| 106 | #expect(repo.watch == "watching") | |
| 107 | #expect(repo.bookmarked) | |
| 108 | #expect(repo.forkOf == "upstream/thing") | |
| 109 | } | |
| 110 | ||
| 111 | @Test func mutedDecodes() throws { | |
| 112 | #expect(try decode("{\(base),\"watch\":\"muted\"}").watch == "muted") | |
| 113 | } | |
| 114 | } | |
| 115 | ||
| 116 | struct WatchStateTests { | |
| 117 | ||
| 118 | @Test func theThreeStatesMapFromTheWire() { | |
| 119 | #expect(WatchState("watching") == .watching) | |
| 120 | #expect(WatchState("muted") == .muted) | |
| 121 | #expect(WatchState(nil) == .default) | |
| 122 | // An unknown value the server might add later must not crash or | |
| 123 | // masquerade as watching. | |
| 124 | #expect(WatchState("something-new") == .default) | |
| 125 | } | |
| 126 | ||
| 127 | /// Only two transitions exist. Nothing returns you to the default, | |
| 128 | /// so nothing may offer it. | |
| 129 | @Test func onlyTheReachableTransitionsAreOffered() { | |
| 130 | #expect(WatchState.watching.available == [.mute]) | |
| 131 | #expect(WatchState.muted.available == [.watch]) | |
| 132 | #expect(WatchState.default.available == [.watch, .mute]) | |
| 133 | } | |
| 134 | ||
| 135 | @Test func noStateOffersAReturnToTheDefault() { | |
| 136 | for state in [WatchState.watching, .muted, .default] { | |
| 137 | #expect(state.available.allSatisfy { $0 == .watch || $0 == .mute }) | |
| 138 | } | |
| 139 | } | |
| 140 | } | |
| 141 | ``` | |
| 142 | ||
| 143 | Add view-model tests confirming `watch()`/`unwatch()` still send their commands and reload, and **rewrite** `RepoActionTests`'s bookmark tests: the ones covering `loadBookmarkState` and the `repo bookmarks` argv go, since that path no longer exists. `setBookmarked` still sends `repo bookmark`/`repo unbookmark` and must keep its tests. | |
| 144 | ||
| 145 | - [ ] **Step 2: Run to verify failure.** | |
| 146 | - [ ] **Step 3: Implement, deleting `loadBookmarkState` and `isBookmarked` entirely.** | |
| 147 | - [ ] **Step 4: Run the tests.** | |
| 148 | - [ ] **Step 5: Commit** — `git commit -m "Take watch, bookmark and fork state from repo show"` | |
| 149 | ||
| 150 | --- | |
| 151 | ||
| 152 | ### Task 2: The controls | |
| 153 | ||
| 154 | **Files:** `gitbay/Views/Repos/RepoView.swift` | |
| 155 | ||
| 156 | No unit tests — UI. | |
| 157 | ||
| 158 | **The watch control** replaces `!53`'s two stateless buttons. Show the current state and offer only what `WatchState.available` gives: | |
| 159 | ||
| 160 | - Watching → "Watching" with a Mute action | |
| 161 | - Muted → "Muted" with a Watch action | |
| 162 | - Default → neither label claimed, both actions offered | |
| 163 | ||
| 164 | Do **not** render a three-segment picker: selecting the default segment would have no command behind it. And drop `!53`'s caption saying the current setting is not shown — it is now. | |
| 165 | ||
| 166 | **The bookmark toggle** reads `RepoDetail.bookmarked` directly. Remove the `.task` and `.refreshable` hooks that fed `loadBookmarkState`, and the "Checking Bookmark…" state, which existed only for a read that no longer happens. `setBookmarked` still reloads, so the toggle follows the server. | |
| 167 | ||
| 168 | **The fork line:** when `forkOf` is present, show "Forked from `<path>`", linking to `RepoRoute.repo(forkOf)`. When absent, **show nothing** — absence means either not-a-fork or a parent you cannot see, and the screen must not claim the former. | |
| 169 | ||
| 170 | - [ ] **Step 1: The watch control** | |
| 171 | - [ ] **Step 2: The bookmark toggle, with the old load path removed** | |
| 172 | - [ ] **Step 3: The fork line** | |
| 173 | - [ ] **Step 4: Build and run the full suite.** | |
| 174 | - [ ] **Step 5: Commit** — `git commit -m "Show watch, bookmark and fork state on the repository screen"` | |
| 175 | ||
| 176 | --- | |
| 177 | ||
| 178 | ### Task 3: Open the merge request | |
| 179 | ||
| 180 | No parity rows move — all three are already `yes`. **Do not touch the wiki.** | |
| 181 | ||
| 182 | - [ ] **Step 1:** Run the full suite via `xcresulttool`; record the real numbers. | |
| 183 | - [ ] **Step 2:** Open the merge request, noting it closes the gap `krz/gitbay#178` described and that the issue is already closed upstream. | |
| 184 | ||
| 185 | --- | |
| 186 | ||
| 187 | ## Notes for whoever executes this | |
| 188 | ||
| 189 | **No command returns you to the neutral watch state.** Only `repo watch` and `repo unwatch` exist. Any control offering a third option would be offering something the server cannot do. | |
| 190 | ||
| 191 | **Absent `fork_of` is not "not a fork."** The server withholds it when you cannot read the parent. Say nothing rather than saying the wrong thing. | |
| 192 | ||
| 193 | **`bookmarked` absent means false** — it is `omitempty` on a Bool. | |
| 194 | ||
| 195 | **Delete the old path, do not dual-source it.** `loadBookmarkState` and `isBookmarked` existed only because the state was unreadable. Leaving both would mean two answers to one question. | |