Show the tickets with work on a site, and let a contributor jump between them - #185
Open
juanmaguitar wants to merge 14 commits into
Conversation
The branch layer (#108) made a second ticket cheap, but nothing showed the branches that exist: coming back to a ticket meant remembering its number and typing it again, and "delete this ticket's work" had no UI at all. The Trac ticket panel now lists the site's ticket branches in both states — "Continue working on #59234 · edited 2 days ago" when no ticket is linked, "You also have work on #61002 — switch" when one is — with a confirmed destructive delete per row. Resuming goes through the existing sites:set-ticket write, which already switches to a branch it recognises, so the only new bridge calls are the list and the delete. Row selection, ordering and the relative-time label live in a pure module (src/renderer/ticket-branch-list.cjs) driven by node --test, following the trac-ticket.cjs convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…th long operations (self-review) Two findings from the pre-PR review pass, both in the switch path this change makes routine: - Switching refreshed the branch list but not the site status, so the previous ticket's "patch applied · Revert" banner survived onto a branch it does not describe — and Revert then reported there was nothing to revert. The status reload now rides the same await. - The new row actions blocked each other but not the panel's other writers, and nothing blocked them during an install, build or trunk update — all of which share the working directory a switch checks out. One guard now covers every ticket control in both directions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… testing) Two findings from JuanMa driving the app against real branches: - Right after a switch, the list on screen is still the one loaded before it, so its idea of the checked-out branch can be a step behind — and the panel offered "You also have work on #59234" while linked to #59234. The linked ticket is now excluded by number, independently of the list's current ref, so a stale list can no longer offer the branch you are on. - The unlinked state's "Continue working on #N" buttons become links, matching the linked state's "switch" and the panel's other actions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Loose edits on trunk cannot be parked — trunk is the diff base every ticket is measured against, so it is never committed to — and a forced checkout would overwrite them. The switch is refused for good reason, but the panel said only that and stopped there, leaving the edits in the tree and the contributor with nowhere to click. The refusal now carries its two honest ways out: save the edits as a patch, or discard them and continue. They are separate steps on purpose — saving a patch does not empty the working tree, so the panel says so and only then is discarding a safe thing to offer. The third route, typing a ticket number so the edits ride along into a new branch, already worked and is now named in the message. The panel keys this off the refusal's code rather than its wording, so a wiring test pins that contract: the message alone would have left the dead end in place with a green suite. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 8, 2026
At 11px in a pill it read as a tag on the panel rather than as the thing the panel is about. Once a ticket is linked it is what the site is for — and under #108 it is also the branch you are checked out on, so it answers "which of my tickets is this" at a glance. Sized just under the section heading, which keeps the hierarchy intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…g-the' into juanmaguitar/ticket-branches-ui # Conflicts: # src/renderer/index.jsx
…g-the' into juanmaguitar/ticket-branches-ui
…g-the' into juanmaguitar/ticket-branches-ui
## Why Creating a site clones `wordpress-develop`, which takes minutes. The window shows the site straight away and its folder exists from the first moment — but every way of opening it is refused for the whole clone. `dir:show` and `editor:open` are gated on the `sites` registry, and the registry does not hear about the site until the clone finishes. So the app spends several minutes refusing to open a folder it created itself, at exactly the moment a newcomer wants to look at the checkout arriving. That is #180. ## What changes **The obvious fix is registering the site early, and it is the wrong one.** `sites` is persisted. A half-cloned directory written into it survives the crash, the quit or the power loss that no unregister-on-failure path can catch — a phantom site for a directory that was never finished, which is the thing AGENTS.md's architecture rules single out. It would also widen the allow-list for the recursive delete in `sites:delete` to include a tree `isomorphic-git` is actively writing into, and silently change what `sites` means for every registry already on disk. A UX fix that loosens a destructive guard as a side effect is not a fix. **So the second record is liveness, not truth.** The new `src/setup-tracker.js` holds the paths this process is setting up right now, keyed by the directory main computed itself and released in a `finally` however the setup ends. Nothing half-finished reaches the store, and a restart mid-clone lands back on exactly the old behaviour instead of a new state that has to be reconciled. It sits beside the six per-site maps in `main.js` that already hold liveness this way — `playgroundServers`, `runningInstalls`, `runIdByDirectory` and the rest — and its entries are the shortest-lived of them all. **The two verbs then want opposite answers about a site being created, and get them.** `isActionableSite` widens revealing and opening to include a pending path; `deleteRegisteredSite` refuses one outright, registered or not. That asymmetry is the design: reveal gets wider, delete gets narrower. Until now the delete refusal came for free from the path not being in `sites` yet — making the folder openable is what took that accident away, so the guard is now explicit rather than incidental. The row's menu stops offering **Delete this site** mid-clone to match; main's refusal is the backstop, and not offering a control that cannot work is the actual answer. **`wordpress:setup` gets its first tests.** It was listed as `NOT_REACHABLE` in `test/ipc-wiring.test.cjs` on the grounds that it clones over the network. It does not have to: the harness resolves bare packages through `require.resolve`, so `isomorphic-git` is stubbable and the whole handler runs offline in milliseconds. That is what lets a test be *inside* the clone, which is the only place this bug is visible. **Deliberately not in this PR:** the collision case. The window still sends its own guess of the path, so when the folder name is already taken (`demo` exists → main creates `demo-2`) the guess and the real directory differ and the open is still refused. The tracker is keyed on the real path, which is the only safe choice — a key the renderer supplies is not a key. The fix is the window adopting the path main reports, which is PR 3; the contract it needs is pinned by a test here. ## How to test this **Platform: any.** Nothing here is platform-specific. **Starting state:** the app open, and somewhere to create a site. A slow network helps — the whole bug lives inside the clone, so you need it to last long enough to click. 1. Click **Create WordPress Core site**, give it a name that is not already a folder on disk, and start it. The checklist shows the clone running. 2. **While it is still cloning**, open the **Open directory in** menu under the site path and choose **Show in Finder** / **Show in Explorer**. The folder opens, showing the partial checkout. Before this change it refused. 3. Still cloning, choose a detected editor from the same menu. It opens on the partial checkout. 4. Still cloning, open the row's **More** menu. **Delete this site** is not offered. 5. Let the clone finish. **Delete this site** is offered again, and deleting works. **What must not have happened:** - **No site left behind that the app cannot see.** After step 5, and after a failed setup, the sidebar and the app's own record must agree. Quit and reopen the app mid-clone: the half-cloned site must **not** appear in the sidebar — that is the phantom this design exists to avoid, and it is what registering early would have produced. - **Nothing deleted from under the clone.** In step 4, if you reach delete another way, the directory must survive and the clone must continue. - **Creating a site still works end to end.** The label you typed, the created date and the trunk date all appear on the finished row — those are written after the clone, inside the region this PR moved into a callback. To watch the bug fail to reproduce, do step 2 on a build from `trunk`: the notice appears instead of the folder. **What I could not test by hand:** all of it. I have no machine set up to drive the app here, so the steps above are unrun and the evidence is the test suite. Someone driving a Buildkite artifact would be closing a real gap — check the build matches the head commit. ## Risks and limitations Self-review came back **3 [fix here] · 2 [follow-up]**; all three are fixed, and the detail is below. One is worth surfacing here because it changed how much this PR proves: two of the wiring lines that make the fix work were not covered by anything, so the line could be deleted with the suite still green. After the fix I mutation-tested all three by deleting each in turn and confirming the suite fails each time. The known gaps, both filed rather than fixed: - **The collision case is not closed**, as described above. #180 stays open until PR 3. - **`pendingSites` in the window is cleared wholesale**, so with two setups running the second one's row can lose its pending state when the first finishes, and the Delete control reappears mid-clone. Main still refuses, but the renderer ignores the `false`, so the contributor gets a confirmation dialog and then nothing. Pre-existing coarseness that this PR makes visible on a new path. ## Related Fixes #180 for a site whose directory name was free. Follows #211. Follow-ups filed: see below. --- <details> <summary>Design decisions and alternatives considered</summary> **Why not register early and unregister on failure?** Covered above; it is the approach this PR is an argument against. The short version: the failure path cannot catch a crash, and the check it would need — "did *this* call add it?" — is the same in-memory liveness this module provides, except load-bearing for persisted data and lost exactly when it is most needed. **Why key on the real `siteDir` rather than what the renderer sent?** A guard keyed by whoever it is guarding is not a guard. It costs the collision case for now, which PR 3 pays back. **Why refuse a second setup for the same directory rather than joining it?** Two windows can resolve the same name before either creates it. Two clones interleaving in one tree is worse than the second one refusing. **Why `paths()` returns a copy.** The guards only read it, but handing out the live Set would let a caller widen the boundary it is being checked against. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **3 [fix here] · 2 [follow-up]** — all three `[fix here]` fixed; both follow-ups described under Risks and limitations. Dispatched to a subagent with fresh context per `.claude/skills/self-review/SKILL.md`. - *[fix here] Tests 🟡* — the tracker's "a second setup is refused" test did not prove its own claim: it asserted the entry was released *after* the outer setup had already ended, so it only re-tested the outer release. The reviewer demonstrated it by building a tracker whose refusal deletes the first setup's entry — leaving a live clone deletable — and the test still passed. **Fixed**: the assertion moved inside the first setup, and I reproduced the reviewer's broken tracker to confirm the test now fails on it. - *[fix here] Tests 🟡* — `pending` was passed to `editor:open` and `sites:delete` with nothing covering it. Deleting either line left the suite green while half of #180 was broken again; the unit tests could not see it because they hand `pending` to the guard themselves. **Fixed**: all three handlers now assert it reaches the module, plus a new test that opens the folder in an editor *during* the clone and asserts the list main actually built. Verified by deleting each of the three lines in turn — the suite fails each time. - *[fix here] Security 🔵* — the module header claimed "nothing the renderer sends ever reaches `begin`", which is false: the leaf name is main's, but the parent directory comes straight off the invoke. Not exploitable — the same call registers that path minutes later anyway — but it is exactly the sentence a future reviewer would lean on to justify a further widening. **Fixed**: the header now says precisely what a key is and what it is and is not good enough for. - *[follow-up] Architecture 🔵* — the wholesale `pendingSites` clearing, described under Risks. - *[follow-up] Architecture 🔵* — the collision case, which is PR 3. Honest note on which tests reproduce the bug: the reveal-mid-clone test, the editor-mid-clone test and the collision test fail on trunk. The delete-during-clone test and the cloning-status test pass on trunk too — they are regression guards for this change, not reproductions, and the new delete behaviour is proven in `test/site-registry.test.cjs` instead. </details> <details> <summary>Screenshots or recording</summary> The only visible change inside the app is the absence of **Delete this site** from the row's **More** menu while a clone is running. Everything else that changes is off-app: whether the file manager or the editor opens. </details> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
> Reopened from #213, which GitHub closed automatically when #212 merged and its base branch was deleted. Same branch, same single commit, now rebased onto `trunk` — the review history is on #213. ## Why The window draws a site's row before it can know where the site will be. The contributor picks a parent directory and types a name; the main process is the one that turns those into a directory, and it may not use the name it was given — `findAvailableDirName` appends `-2` when the folder already exists. So the row starts on a guess. That was cosmetic while the path was only something to display: it read wrong under the site title for the length of the clone, then corrected itself. It stopped being cosmetic in #212, which made the open/reveal guards key on the directory the app actually created. The row hands its own path to `dir:show` and `editor:open`, so on a collision it asks about a folder that was never made and is refused — and if the guessed name happens to belong to a *different* registered site, it asks about that one instead. This is the last part of #180 still refused. ## What changes **The row adopts the real path when main first reports it.** `download:status {phase:'cloning', target}` arrives minutes before the clone ends, so the row is honest for essentially the whole wait — the path under the title included. The selection moves with it; otherwise the panel points at a path no longer in the list and the contributor watches their new site's checklist disappear. **Which event to believe turned out to be the whole problem.** My first version moved the row for any status naming a different directory. That is correct for one setup and wrong for two: a finishing setup's `done` would drag the other one's row onto its own path, carrying its label, its created date and its log across. `cloning` is the event that announces a directory and arrives exactly once per setup, so it is the only one that moves a row — and that decision is now `rowPathAfterStatus`, a tested function, rather than a condition inside a subscription, because it is the part that was actually wrong. **Two setups cannot run at once, and now the app says so.** This flow has always been single-file — one pending card, one terminal, one `clearPendingSites()` that clears them all, one slot for the row being created. The sidebar's create button was the only door left open on a second one, and a second one does not half-work. Disabling it while a setup runs makes the UI agree with what the code has always assumed. Real per-site setups are a bigger change and are filed separately. **Adopting early is what makes the failure branch dangerous.** It discarded the *guessed* path, which was only ever safe because the swap could not have happened yet. Now it would strand a row for a directory whose setup just failed. So the three divergent inline copies of add/swap/discard became one reducer, `src/renderer/pending-setup.cjs`, and discarding takes whatever path the row currently has. **`sites` and `siteMeta` become one piece of state.** Adopting moves both, and two setters cannot do that without a render in between where the site has a path under one key and its label under another. `setSiteMeta` keeps its signature, so no other caller changed; `setSites` had no callers left and is gone. ## How to test this **Platform: any.** **Starting state:** the app open, and a folder on disk whose name you can collide with. The easiest setup is to create a site called `demo`, let it finish, and then start a second one *also* called `demo` — main will make `demo-2`. 1. Create a site with a name that is **not** taken. While it clones, check the path under the site title: it is the folder being created, and **Open directory in → Show in Finder/Explorer** opens it. (This is #212's behaviour; it must not have regressed.) 2. Now create a second site with a name that **is** already taken. While it clones, the path under the title reads `…/demo-2` from the first moment — not `…/demo`. 3. Still cloning, choose **Show in Finder** / **Show in Explorer**. It opens `demo-2`, the folder being created. Before this change it was refused, and if `demo` was a registered site it would have opened *that* one. 4. Let it finish. The row keeps the label you typed and stays where it was in the sidebar — it does not jump to the bottom. 5. With a setup running, look at the sidebar's **Create WordPress Core site** button: it is disabled until the setup finishes. **What must not have happened:** - **No duplicate row.** At no point should both `demo` and `demo-2` appear for the same setup. The adoption replaces the row rather than adding one. - **The panel never goes blank mid-clone.** At the moment of adoption the selection follows the row; if the main panel empties for a frame, that is this change failing. - **A failed setup leaves no row behind.** Point the create flow at a directory the clone will fail on. The row disappears — and specifically the *adopted* row, not just the guess. A leftover row for a half-created directory is what this PR's reducer exists to prevent. - **The label and created date survive.** After step 4 the sidebar order is unchanged; a lost `createdAt` sorts the row to the bottom. **What I could not test by hand:** all of it. I have no machine set up to drive the app, so the evidence is the suite. A reviewer on a Buildkite artifact would be closing a real gap. ## Risks and limitations Self-review came back **1 [fix here] · 2 [follow-up]**. The `[fix here]` was the concurrency bug described above — it was found by review, not by me, and the response was both to fix the sequencing and to extract the decision so a test could hold it. Detail below. Known and not fixed here: - **Only one setup at a time**, now enforced rather than silently broken. Genuine per-site setup state — a pending card, a terminal and a row per site — is a larger change; filed. - **A failed setup leaves its log entry and its path alias behind** in the renderer's in-memory log map. Pre-existing; the adoption adds an alias to the orphan set. Filed. - The reducer's tests are new-code tests: no test here fails on old code, because the code they cover did not exist. What the old code got wrong lives in the subscription, which is why `rowPathAfterStatus` was pulled out — that one *is* covered, and I verified it by deleting its phase check and watching the test fail. The wire from the subscription into the reducer is still unproven, like every other renderer module in this repo. ## Related Closes #180. Stacked on #212, which follows #211. --- <details> <summary>Design decisions and alternatives considered</summary> **Why not key the tracker on the renderer's guess instead, so no adoption is needed?** A guard keyed by whoever it is guarding is not a guard. The real directory is the only safe key, which is what makes the row's honesty the renderer's problem to solve. **Why adopt on the status event rather than waiting for `setupWordPress` to resolve?** Resolving *is* the clone finishing. Waiting for it means the row is wrong for exactly the window the bug is about. The resolve-path swap is kept as a backstop for a missed event, and is a no-op when the event arrived. **Why a single ref rather than a map of in-flight setups?** Because the events carry no setup identity, so a map could not be keyed correctly anyway — a second setup's events are indistinguishable from the first's except by their target. Making concurrency impossible is honest; faking support for it with a heuristic correlation would not be. **Why merge the two state atoms rather than sequencing two setters?** Two setters give you a render where the row has moved but its metadata has not, or vice versa. The sidebar reads both. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **1 [fix here] · 2 [follow-up]** — the `[fix here]` is fixed; both follow-ups are described under Risks and filed. Dispatched to a subagent with fresh context per `.claude/skills/self-review/SKILL.md`. - *[fix here] Architecture 🟡* — with two setups in flight (the create button was not disabled), the first setup's `done` event satisfied the adoption condition and moved the *second* setup's row onto the first's directory, merging its label and `createdAt` over a registered site's metadata and aliasing its log. The second row then vanished until the next refresh, and in the other interleaving it never adopted at all — #180 reopening for the second site. **Fixed** two ways, because one was not enough: adoption is restricted to `cloning`, and the create button is disabled while a setup runs, since the single slot the ref provides is what the rest of the flow has always assumed. I verified the phase guard by deleting it and watching the test fail. - *[follow-up] Tests 🔵* — the reducer tests cannot fail on old code, and the corrected behaviour lived entirely in the subscription. **Acted on rather than deferred**: `rowPathAfterStatus` was extracted so the decision is testable, and it is the function the concurrency bug lived in. What remains deferred is the untested wire from subscription to reducer, which is structural across every renderer module here. - *[follow-up] Architecture 🔵* — orphan setup-log entries and path aliases after a failed setup. Pre-existing; filed. Confirmed clean by the reviewer, by reading: no remaining `setSites` callers; the three `setSiteMeta` callers unaffected; the merge removes an intermediate render rather than adding one; `activeSite` cannot race the `sortedSites` fallback because the IPC callback is auto-batched; both `adoptSetupPath` guards return the same object so React bails out; `moveSetupLog` is idempotent and its alias chain handles the double hop. </details> <details> <summary>Screenshots or recording</summary> The visible change is the path under the site title during a clone when the folder name was taken — `…/demo-2` from the start instead of `…/demo` until the clone ends — and the create button being disabled while a setup runs. No layout or styling changed. </details> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g-the' into juanmaguitar/ticket-branches-ui
Closes #202. Closes #203. Two halves of the same question, so one PR. ## The rule was written for one kind of child The Windows section of the review standard said the flag flatly: > `windowsHide: true` keeps console windows from flashing True for a console child, misleading for a GUI one — and that gap is what produced #181. The editor spawn carried the flag, the editor honored the "start hidden" it puts in STARTUPINFO, and it launched with no window while the spawn still reported success. Left as it was, the rule hands the same advice to the next GUI process someone spawns. It now turns on the child's subsystem rather than on whether anyone reads the child's output. That distinction matters for more than the editor: `src/kill-tree.js` sets the flag on `taskkill`, whose output nobody reads, and an "only if you read its output" rule would have made that line look wrong. ## Nothing stopped main from applying the patch `patchChildProcess` is deliberately blunt — every `child_process` entry point, overriding even an explicit `windowsHide: false`. That is safe today only because the four runners call it and `main.js` does not, which nothing said and nothing checked. "Hide the console flashes everywhere, do it once at startup" is the plausible-looking change that reinstates #181 in silence. `test/editor-launch.test.cjs` injects its own `spawn`, so it never sees the real module; `test/runner-wiring.test.cjs` only asserts that the runners *do* call it. So the module header now names who must not call it, and `test/ipc-wiring.test.cjs` asserts `main.js` does not. ## On the test It watches for a call that does not happen rather than reading the flag off the real `child_process`. `patchChildProcess` is a no-op off Windows, so the flag-reading version would be green on macOS whatever `main.js` did — the "green while proving nothing" shape the standard already names. Scoped to module evaluation, which is where "once at startup" lands. A call made lazily inside a handler would slip past it; that is a less likely shape, and the module header carries the rule for it. ## Testing - `npm test` — 610 pass, 0 fail. - `npm run lint` — clean. - Mutation-checked: prepending `require('./hide-child-windows').hideChildWindows();` to `src/main.js` fails the new test, and only that test. - No runtime behaviour changes — the diff is one document, one comment, one test. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… first one (#229) Records the decision from #216 — decisions move out of index.jsx into pure .cjs modules, no DOM harness — as an invariant in the review standard, and lands site-folder.cjs as the first extraction under it: the Create site modal's path arithmetic, previously untested cross-platform string work. Surfaced #228, closed as not planned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
#168 made a second ticket cheap, but invisible: the branches exist, and nothing on screen shows them. A contributor coming back to a ticket has to remember its number and type it into the link field again — and "delete this ticket's work", the destructive half of #108's model, has no UI at all, so
branches:deleteships unreachable.What changes
The Trac ticket panel lists the site's ticket branches, in both of its states:
When a switch is refused because trunk has loose edits — they cannot be parked, since trunk is never committed to — the message now carries its two ways out: save them as a patch, or discard them and continue. Separate steps on purpose: saving a patch does not empty the working tree, so the panel says so before offering the destructive one. Carrying the edits along into an existing ticket in one action is #196.
Each row carries a confirmed, destructive Delete this ticket's work — the only UI for
branches:delete, and deliberately a different gesture from switching.The decision a reviewer would otherwise reverse-engineer: resuming does not use the new
switchBranchbridge call.sites:set-ticketalready resumes a branch it recognises, with all the parking rules, and the renderer'ssaveTicketalready owns the busy state, the error alert and the sidebar sync — so "Continue working on #N" is that same write, and the new bridge surface this PR consumes is justlistBranchesanddeleteBranch.Row selection, ordering and the "edited N days ago" label live in a pure module,
src/renderer/ticket-branch-list.cjs, per thetrac-ticket.cjsconvention — the JSX renders what the module returns.Deliberately not in this PR: tickets nested under the site in the sidebar, any change to Unlink's semantics, and a per-row busy spinner distinct from the shared saving state. The first two are the rest of #108's UI; the last is cosmetic.
How to test this
Platforms: any. Stacked on #168 — its branch is the base, so this diff is only the UI. The Buildkite artifact for this branch exercises the whole stack; check it matches the head commit.
Starting state: a site that has finished its first install and build, with no ticket linked and no ticket branches yet.
62281, then Unlink. Link61002, then Unlink. → The panel now shows Your tickets on this site with Continue working on #62281 and #61002, most recently used first, each with an "edited…" note.#62281, and the list becomes Other tickets on this site — You also have work on #61002 — switch.wp-login.php, then click switch on #61002. → Seconds again. Click switch back on #62281 → your edit is back, and the "patch applied" banner (if any) always describes the ticket you are on.git -C <site> branchno longer showsticket/N.What must not have happened:
What could not be tested by hand: the mid-switch (
switch-incomplete) refusal — it needs a checkout to die part-way. Its message surfaces through the panel's existing error alert, which is exercised by the wiring tests on the base branch.Risks and limitations
window.confirmpattern. It destroys a branch and its parked WIP commit; there is no undo.branches:deletehas no mid-switch guard on the base branch (sites:set-ticketandbranches:switchboth have one). This PR is what first exposes the handler to users. Found by the review pass; it is base-branch code, so it is a follow-up there rather than a change here.SiteRow, matching the existingloadStatusprecedent — work that scales with the site registry. AnisActivegate is the follow-up shape if it ever shows up in practice.Related
Part of #108, on top of #168 (stacked; this PR's base is that branch). Follow-up: #196 (carrying loose trunk edits into an existing ticket in one action). The sidebar half of the UI remains, tracked in #108.
Review outcome (required — see AGENTS.md)
2 [fix here] · 2 [follow-up] — both fixed, judgement pass run in a fresh context per the skill. Plus one found by driving the app (below).
🟡 Switching refreshed the branch list but not the site status, so the previous ticket's per-branch "patch applied · Revert" banner survived onto a branch it does not describe — and Revert then reported nothing to revert. Fixed: the status reload rides the same await as the list reload.
🟡 The new row actions blocked each other but not Unlink/Link, and nothing blocked any ticket control during an install, build or trunk update — operations that share the working directory a switch checks out. Fixed: one
ticketActionsBlockedguard, both directions, covering the long-operation trio the panel's other destructive controls already guard on.🟡 Found in manual testing, not by the review pass: right after a switch the on-screen list can still be the one loaded before it, so its
currentref excluded nothing — and the panel offered "You also have work on #59234" while linked to #59234. The linked ticket is now excluded by its number as well, independently of the list'scurrent, with a regression test reproducing the stale-list state. The same pass turned the unlinked state's row buttons into links, matching the linked state.Deferred, with reasons: the missing mid-switch guard on
branches:delete(base-branch code — see Risks) and the per-SiteRowlist load (matches the existing pattern;isActivegate if it ever matters). Style notes from the pass (comment wording, the'trunk'literal) were applied.Implementation notes
src/renderer/ticket-branch-list.cjs—ticketBranchRows({branches, current, now})filters out foreign branches (ticketId: null) and the checked-out ref (which also excludes the linked ticket, sincesites:set-ticketalways leaves you on it), sortslastUsedAtdesc with nulls last, and attachesrelativeTimeLabel. Buckets are hand-rolled rather thanIntl.RelativeTimeFormat— with an injectednowthey are testable to the millisecond, and past a week the label switches to the absolute date.test/ticket-branch-list.test.cjs— 14 cases; the ≥7-day assertions check only the"edited on "prefix so no CI locale can break them.loadBranchesis deliberately not part ofloadStatus: that one runs after every long operation, and the branch list only changes on link, resume and delete — the three paths that refresh it themselves.Screenshots or recording
Pending a manual pass — the walkthrough above is the script. A short recording of steps 2–5 shows the thing a still frame cannot: that a switch takes seconds, not a rebuild.