Say when loose work came along into a new ticket - #218
Merged
juanmaguitar merged 10 commits intoAug 10, 2026
Conversation
…t in silence Linking a ticket this site has not seen carries whatever is uncommitted into the new branch. That is the right behaviour and it stays — "I started editing, then realised which ticket this is" is a real sequence. What was wrong is that it happened without a word, while the same gesture on a ticket that already has a branch is refused with two ways out. Same starting state, same click, opposite outcomes, and nothing on screen to tell them apart. Reported from driving the app. Two sentences fix it. Afterwards, the panel says how many changes came along and into which ticket. Beforehand, the field says that whatever is not yet on a ticket will come with you — written so it needs no scan and holds in both cases. The count is one worktree walk, which is why it only runs on the path that moves work without announcing it, once per ticket rather than once per switch. And it degrades: a worktree it cannot walk costs the sentence, never the ticket. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…thing false (self-review) Six findings; these are the two that mattered. The count held the reply. Linking a ticket is a HEAD move and was instant; putting a full worktree walk in front of it made the most common sequence — install a site, link a ticket, then start editing — pay a scan for an answer that is always zero. It now runs after the answer, on its own channel, and is counted against the branch's own HEAD so the number says what this ticket's patch will contain. And the progress frames I wrapped it in were a lie: on trunk, with no ticket to name, "scan" renders as "Saving your work…" — about the one branch this feature refuses to write to — followed by "Ready to work on #62281" before the branch existed. A fact after the fact is not a stage of an operation, so it stopped pretending to be one. The rest: the silent catch now logs, since a worktree that cannot be walked is a state someone will need diagnosed; the second render site was unreachable, because the notice only ever has content once a ticket is linked; the up-front line claimed work would come along even for the tickets listed right above it, which are refused instead; and the notice said work was "part of that ticket now" when it is still uncommitted. The test that would have caught none of it is also here: nothing asserted where the scan does NOT run, so a refactor could have put a worktree walk on every switch with the suite still green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-work # Conflicts: # src/renderer/index.jsx
…to juanmaguitar/carried-work
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
… picked Picking a ticket while trunk had uncommitted edits did opposite things depending on whether the ticket already had a branch: carried silently for a new one, refused for a known one. Both paths now refuse with dirty-trunk and one panel asks: carry (new tickets only), save a patch then start clean, discard and start clean, or cancel. The worktree scan #218 kept off the link path now runs before the answer on the new-ticket path — deliberately, so the wait lands in front of the question instead of behind a completed move. Fixes #234. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
… picked Picking a ticket while trunk had uncommitted edits did opposite things depending on whether the ticket already had a branch: carried silently for a new one, refused for a known one. Both paths now refuse with dirty-trunk and one panel asks: carry (new tickets only), save a patch then start clean, discard and start clean, or cancel. The worktree scan #218 kept off the link path now runs before the answer on the new-ticket path — deliberately, so the wait lands in front of the question instead of behind a completed move. Fixes #234. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
…efuses it (#238) # Why Picking a ticket while trunk has uncommitted edits does two opposite things, and which one you get depends on whether that ticket already has a branch on this site — an implementation fact the contributor cannot see. A ticket new to the site takes the edits with it immediately and says so afterwards (#218); a ticket that already has a branch refuses the switch and offers a panel (#185). Same gesture, same tree, same edits — one path moves the work for you, the other asks. #234 makes the case in full. ## What changes Both paths now refuse with the same `dirty-trunk` code, and one panel asks the question once: - **Take these edits into the ticket** — the old carry, now chosen rather than assumed. Offered only for a ticket new to this site; an existing branch has its own work to restore, so loose edits cannot ride into it. - **Save them as a patch, then start clean…** — one chosen outcome: the save dialog, then the discard, then the switch. Cancelling the dialog cancels the whole option; a failed save leaves the edits in place with the question still open. Where the patch went stays on screen after the switch. - **Discard them and start clean** — behind the same confirm as before. - **Cancel** — touches nothing, records nothing. The panel previously offered three consequential actions and no way out. Mechanically: `sites:set-ticket` grows an `options.carryTrunkWork` flag. Without it, the new-ticket path scans the worktree *before* answering and refuses if trunk is dirty; with it, it performs exactly the pre-#234 carry, including the after-the-fact count on `ticket:carried-work`. The existing-branch path is unchanged in main — `switchToBranch`'s guard already refused; the renderer just stopped treating that refusal differently. The panel is rendered under both places a "Link ticket" field exists (the Trac ticket panel and the patch-destination view), and the standing help line under the field — which claimed edits "will come along", no longer unconditionally true — now says you will be asked. A worktree that cannot be scanned refuses the link instead of guessing there is nothing to carry — guessing "nothing" is how work gets moved in silence again. ## How to test this Platforms: any. **Starting state:** a site that has finished setup, on trunk with no ticket linked, and one uncommitted edit — e.g. add a line to `wp-login.php`. 1. Type a ticket number the site has never worked on and press **Link ticket**. → After a moment (the scan), a panel appears: "You have 1 uncommitted change on this site, not on any ticket yet. What should happen to them?" with four choices. The branch is **not** created yet. 2. Click **Cancel**. → The panel closes. The edit is still in `wp-login.php`, and no ticket is linked. 3. Link the same ticket again and choose **Take these edits into #\<n\>**. → The ticket links, and the blue notice says your 1 uncommitted change came along into the ticket. 4. Unlink the ticket (back to trunk, the edit stays parked on the branch), make a *new* edit on trunk, then click **Continue working on #\<n\>**. → The same panel appears, without the carry option, saying this ticket already has its own work here. 5. Choose **Save them as a patch, then start clean…** and save the file. → The switch completes, and a notice names the saved file. The patch contains the trunk edit from step 4; the working tree now shows the branch's own state from step 3. 6. Repeat step 1's setup with a third fresh edit and a new ticket, choose **Discard them and start clean**, confirm. → The ticket links and the edit is gone from the file. **What must not have happened:** - No edit moved or vanished on any path that ended in **Cancel** or a cancelled save dialog. - Step 1 must not have created `ticket/<n>` or written `tracTicket` to the registry — check "Your tickets on this site" stays empty after Cancel. - Step 5's patch must contain the step-4 edit only, not the step-3 one. - Linking a ticket from a *clean* trunk must still just work, with no panel and no notice. The mid-dialog race (clicking another panel button while the non-modal save dialog is open) is guarded by the busy flag; I could not stage a true OS-level race by hand beyond confirming the buttons are disabled while the dialog is up. ## Risks and limitations - **This deliberately rewrites an acceptance criterion of #218** ("linking a ticket does not pause"). Deciding whether to ask requires looking at the worktree before answering, which is exactly the scan #218 moved off the critical path. It is spent on purpose: the wait now lands in front of a question the contributor is about to read, rather than behind a completed move of their work. The scan runs once per link-from-trunk (the after-the-fact count now runs only when the carry is actually chosen), and resuming an existing branch pays nothing new — its scan already lived inside `switchToBranch`. - The carry option is not offered when the ticket already has a branch. Merging loose trunk edits into a branch's restored state is a merge, not a carry, and is out of scope here; the panel says why the option is absent. - Review outcome headline: 1 [fix here] · 1 [follow-up], both 🔵, both fixed — details in the collapsed section. ## Related Fixes #234. Refines #218 (the silent carry and its notice) and #185 (the refusal panel this extends). Sits on #108's ticket-as-branch model. --- <details> <summary>Design decisions and alternatives considered</summary> - **Extend the refusal panel rather than add a modal.** #234 suggests it, and the panel already carried two of the four options, the warning styling, and the saved-patch affordance. A modal would have been a second interaction for the same question. - **The choice crosses IPC as an option, not as renderer-side sequencing.** The renderer could have checked `git:worktree-dirty` before calling `sites:set-ticket`, but that puts the policy in the renderer and races the check against the action. Main owns the refusal and the carry the same way it owns the branch invariants. - **`canCarry` is main's word, not the renderer's branch list.** The renderer's list can be stale (a branch created outside the app); the handler decides from `listTicketBranches` at the moment of refusal. - **The existing-branch path adds no second scan.** The pre-answer scan is only on the new-ticket path; a refused resume still comes from `switchToBranch`'s own guard, so the clean resume path costs exactly what it did before. - **"Save as a patch" became "save, then start clean" as one action.** #185 kept them deliberately separate so "keep my work" could not quietly mean "lose my work"; the combined option keeps that property by discarding only after the dialog has really produced a file, and it removes the trap where saving still left the switch refused with the discard as a separate leap. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> Self-review ran per `.github/instructions/code-review.instructions.md`, judgement pass in a fresh subagent context. 1 [fix here] · 1 [follow-up], both 🔵 architecture — both fixed in the second commit: - *[fix here]* The "Saved to <path>" confirmation only existed inside the panel, which unmounts when the combined option's switch completes — the path flashed and was gone. Now a notice that survives the switch names the file. - *[follow-up]* The save dialog is not window-modal and the panel underneath stayed clickable while it was open (pre-existing in #185's two-step flow, but the automatic second half raised the stakes). The busy flag is now held across the dialog, disabling the panel's other actions. Style note from the review (a test comment describing the old silent-carry world) also addressed. </details> <details> <summary>Implementation notes</summary> - `src/main.js` — `sites:set-ticket` takes `options`; new-ticket-from-trunk path counts changes via `countChangesAgainst` before acting and returns `{ ok: false, code: 'dirty-trunk', canCarry: true, files, ticket }` when dirty and no choice was passed. `carryTrunkWork: true` re-enters the old path verbatim, `reportCarriedWork` after the reply included. A clean link now pays its one scan before the answer instead of after it. - `src/renderer/index.jsx` — `blockedByTrunkWork` goes from a string (the attempted ref) to `{ ref, canCarry, files, ticket }`. The panel is built once as `blockedPanel` and rendered under both link fields. `dirty-trunk` no longer sets the red error line — it is a question, not a failure. `saveTrunkWorkAsPatch` is replaced by `saveTrunkWorkThenStartClean`. - `src/preload.js` — `setSiteTicket` passes `options` through. - Tests (`test/ipc-wiring.test.cjs`): the ask (refuses, creates nothing, records nothing, no carried-work message), the chosen carry (previous carry test, now passing the option), and scan-failure-refuses. The ask test and the scan-failure test fail against the pre-change code — verified by stashing the src changes and re-running: `ℹ fail 2`, both `(issue #234)` tests. The "resume and unlink never pay for the count" guard from #218 passes unchanged, which is the proof resuming got no slower. - Suite: 681 pass on `.nvmrc` Node and on Electron's bundled Node; `npm run lint` clean. </details> <details> <summary>Screenshots or recording</summary> None yet — the branch was built and tested headlessly. The visible surface is the panel described in "How to test this"; the Buildkite artifact for this branch head can be used to drive it. Can add a recording on request. </details> 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
## Why The card's unsubmitted-changes note never appears for the work its own sentence describes. Link a ticket, make a change, switch to another ticket, come back — the card says nothing, while the patch modal shows the change exactly as it was left. For anyone using more than one ticket, that is all of their work. Observed on a real site, and the two readings disagree on disk: ``` git status -> empty git diff <baseOid>..HEAD -> src/wp-login.php, +3 lines ``` Both answers are correct. They answer different questions, and only one of them is the question a contributor is asking. `#239` has the reasoning. ## What changes **Root cause.** The note measured the worktree against `HEAD`. Under the ticket-as-branch model (#108) a ticket parks its work in a WIP commit, so `HEAD`-relative is *correctly* "clean" for every change that has survived a ticket switch — which is the work the note exists to speak about. `changes-note.cjs` was written before tickets were branches, when "not written down" and "not submitted" described the same edits. Splitting them is what opened the gap. **The fix is to make the note ask the patch's question**, measured from the ticket's branch point rather than from the last time anything was written down. That is the measurement `createMinimalPatchForDir` already makes, so this is the two agreeing rather than a third answer being invented. **Which reader wants which question** — the part worth reviewing: | Reader | Question | Why | | --- | --- | --- | | The card's note | **wide** — `git:unsubmitted-work`, from the branch point | Parked work is unsubmitted work. It is the whole subject of the sentence. | | `startTrunkUpdate`'s dirty dialog | narrow — `git:worktree-dirty` | It protects what a force checkout would overwrite. Parked work survives one; uncommitted edits do not. | | The `dirty-trunk` switch refusal (`sites:set-ticket`) | narrow — `countChangesAgainst` | Same thing: it is about edits on trunk that have nowhere to go. Trunk is never committed to, so there is no parked work there to see. | | `git:preview-patch`'s collision scan | narrow — `collectDirtyFiles` | It lists the files an incoming patch would collide with in the *working tree*. | So the narrow signal is untouched and keeps its channel; the wide one is a new channel beside it, and only the note reads it. Nothing was widened underneath a caller that did not ask for it. Two consequences that are not obvious from that summary: - **A discard now reports what it left behind.** On a ticket branch a discard rewinds to the last park — the WIP commit is not its to take, since destroying a ticket is what deleting its branch is for. The old code asserted "clean" locally after a discard, which would now hide the note over work that is still there. `git:discard-changes` replies with a recount and the card renders that. - **A branch change re-takes the measurement.** A branch-point count is *about a particular branch*, so a stale one is not an old version of the new answer — it is about different work. Linking, unlinking, resuming and deleting a ticket now invalidate the note and walk again. This was a self-review finding, not part of the original design; see the review block. **Deliberately not in this PR:** the note's wording is unchanged (it was already right — the reassurance about unlinking is about work that outlives a link, which is exactly the parked work it could not see), and #236's applied-patch record is untouched. ## How to test this **Platforms: any.** The change is git-graph logic through `isomorphic-git`, with no path, spawn or line-ending surface. Buildkite has signed artifacts for this branch — check the build matches the head commit. **Starting state:** a site set up and built, with no ticket linked and a clean tree. 1. Link ticket **12345**. Edit `src/wp-login.php` — add a comment line. Return focus to the app. → The ticket card reads **"You have 1 unsubmitted change for ticket #12345."** 2. Link ticket **54321** (a ticket this site has not worked on). The switch parks #12345's work. → The card names #54321 and shows **no** changes note. It must **not** say "1 unsubmitted change for ticket #54321" — that is the stale-count bug the second commit fixes, and it is the one thing here you have to be looking for to see. 3. Link **12345** again. → **This is the bug.** Before this PR the card is silent. Now it reads **"You have 1 unsubmitted change for ticket #12345"**, with *review and submit* and *discard your changes*. 4. Click **review and submit**. → The modal's diff shows the same single change to `wp-login.php`. The note and the modal now agree, which is the whole point. 5. Close the modal and click **discard your changes**, confirm. → The note goes away and the tree is clean. 6. Make two edits on #12345. Switch to #54321 and back, then click **discard your changes**. → The note goes away. Nothing is left claiming changes that are gone. **Then the narrow readers, which must be unchanged:** 7. On **trunk**, with an uncommitted edit, click **Update to latest trunk**. → The dirty-tree dialog still appears and lists the file. 8. On **trunk** with an uncommitted edit, link a ticket. → The "decide what happens to it" panel from #238 still appears, with the same three options. (This one shares the area with #238 — worth a look for that reason as much as this one.) 9. Delete a *different* ticket's work from the switcher while on #12345. → The note for #12345 stays on screen and does not blink out and back. Deleting the ticket you are **on** does clear it. **What must not have happened:** - **No work discarded.** Step 3 is the load-bearing one: the parked change must still be there and must still be the only thing in the patch. If the note appears but the patch is empty, or the patch gained files from another ticket, the base being diffed against is wrong. - **The note must not outlive its branch.** A count from the ticket you just left, rendered under the number of the ticket you just arrived at, is the failure mode of this design — with a live *discard your changes* link under it. Steps 2 and 9 are what catch it. - **`node_modules` must not be rebuilt** by any switch here. - **The trunk-update and ticket-switch guards must not have loosened.** If step 7 or 8 stops asking, the narrow signal was widened and a force checkout can now land on uncommitted edits. **Which tests cover it, and the check that they fail without the fix:** the 10 new tests in `test/ipc-wiring.test.cjs` and `test/changes-note.test.cjs`. Verified by resetting `src/` to the stack base with the tests held at their new state — all 10 fail, 124 pass. The end-to-end one is `git:unsubmitted-work sees the parked ticket work a clean status hides (#239)`, which builds a real repository in the parked state and asserts both readings on the same tree: `git:worktree-dirty` clean, `git:unsubmitted-work` dirty. **What could not be tested by hand:** nothing in this change; every path above is reachable in the app. What is *not* covered by the suite is the wiring in `index.jsx` — `reprobeAfterBranchChange` being called from the right places is a call, not a branch, so it is verified by steps 2 and 9 rather than by a test (see Risks). ## Risks and limitations **Review outcome: 5 [fix here] · 1 [follow-up] — all 5 fixed, 1 deferred.** Two passes; the first found a bug this PR introduced. Detail in the collapsed block. - **The focus probe costs more than it did.** `collectChangedFiles` reads both sides of every changed row into buffers and decodes them, where `collectDirtyFiles` read a blob only for CRLF candidates — and the note probes on every window focus. `statusMatrix` still dominates, and the changed-row count is small for a normal ticket, so no user-visible path was identified. Deferred rather than fixed: the cheap version needs the buffers only up to the line-ending comparison, which is a change to the shared classifier and belongs on its own. **Follow-up issue not yet filed** — happy to file it. - **`index.jsx` wiring is not test-covered.** The re-probe calls live in the component, which the suite cannot load. The *decisions* are all in `changes-note.cjs` with tests, per the repo invariant; what is untested is that they are called from the right four places. Steps 2 and 9 of the manual test are what stand in for that. - **`branches:delete` gained a reply field** (`movedToTrunk`). Additive, and no stored shape changed, so there is no migration. - **This PR sits on #238, which touches the same handlers.** Rebased onto its current tip (`0c9902c`) rather than merged, so the stack keeps one shape. ## Related Closes #239. Sits on #238 (base branch), the tip of #168 → #185 → #198 → #205 → #218 → #238. Related: #234 (the other decision that hangs on this reading of "changes"), #236 (the same shape again — one thing moved to a branch, another stayed behind), #108 (the ticket-as-branch model that split the two questions apart). --- <details> <summary>Design decisions and alternatives considered</summary> **Rejected: widen `git:worktree-dirty` so everyone gets the branch-point answer.** The smallest diff and the wrong one. It would have made the trunk-update dialog offer to save and discard *parked* work, and made the `dirty-trunk` refusal fire for a ticket that has nothing loose in its tree at all — a switch refused over work that is already safely committed. The issue calls this out directly, and it is the finding I most expected to have to defend, so: two questions, two channels, and a table in "What changes" saying which caller asks which. **Rejected: derive the note's count in the renderer from the patch text.** The modal already fetches a diff; counting its `+++` lines would have needed no new channel. But it means generating and transferring the full patch on every window focus to answer a yes/no, and it makes the note's truth depend on the patch *renderer* rather than on the walk. The count and the diff should be two readings of one walk, not one derived from the other's output. **Why the classifier was extracted rather than the count re-derived.** `collectUnsubmittedFiles` could have filtered on its own rules — "skip binaries, skip line-ending-only churn". It would have drifted from the patch's rules within one change to either. `classifyChangedFile` now holds that decision once and both read it, which is what makes "the note and the modal never disagree" a property of the code rather than a promise. It also decides the *count*: a binary change counts, because the patch names it above the diff and the contributor has to hear about it, while a CRLF-only difference does not, because nothing would be in the patch. **Why a discard reports a recount instead of the renderer re-probing.** It could have called `refreshDirty()` after the discard. The reply is better: it is one walk instead of two, it cannot race the probe already in flight, and it makes "what survived this" a fact the main process states rather than something the renderer infers from a second question. The recount is non-fatal — if it throws, the discard still succeeded and the reply just says less. **`branches:delete` gained `movedToTrunk` rather than the renderer inferring it.** `current` reports `trunk` both when the delete took the checkout with it and when the delete was simply *made* from trunk. The renderer cannot tell those apart, and it now re-walks the tree on this answer, so main states the fact instead. Pinned by a test that asserts `current` is `trunk` on a path where `movedToTrunk` is false — the exact ambiguity that made the field necessary. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **5 [fix here] · 1 [follow-up] — all 5 fixed, the follow-up deferred with a reason.** Run per `.claude/skills/self-review`, dispatching the judgement pass to a subagent with the diff and `.github/instructions/code-review.instructions.md` and nothing else from the authoring session. Run twice: the first pass found a bug the fix itself introduced, so the second pass reviewed the corrections. Deterministic layer clean throughout — `npm run lint` passes, and the suite is green on `.nvmrc`'s Node (24.18.0) and on Electron's bundled Node. **First pass — 2 [fix here] · 1 [follow-up]:** 1. **🟡 architecture, `index.jsx` — the note was never re-taken when the branch changed.** Introduced by this PR: with a `HEAD`-relative probe the post-switch answer was always "clean" and a stale value was harmless; a branch-point measurement changes with the branch. So resuming a ticket showed the *previous* ticket's count next to the *new* ticket's number, with a working discard link, until the window lost focus. **Fixed** — `reprobeAfterBranchChange` on every path that moves the checkout, clearing the note while the new walk runs so no wrong sentence is ever rendered. The probe's `inFlight` guard also silently dropped a request made mid-walk, which was safe only while the answer could not change without the window losing focus; it now queues and re-runs. 2. **🔵 architecture, `index.jsx` — an inline second answer to a question the module already answered.** `noteAfterDiscard` decided "does this reply carry a recount" inline, slightly differently from `discardOutcome`, and its three call sites disagreed about whether they passed a raw reply or a normalised one. Exactly the shape of #180, and against the §1 invariant. **Fixed** — the decision is `noteAfterDiscard` in `changes-note.cjs` with its own tests; the component holds a state assignment and a call. 3. **🔵 performance, `main.js` — the focus probe reads and decodes more than it used to.** **Deferred**, see Risks. **Second pass — findings 1 and 2 confirmed resolved; 2 new [fix here], both 🔵, both fixed:** 4. **architecture — `deleteTicketWork` re-walked unconditionally.** Deleting a ticket you are not on does not move the checkout, so the note blanked and rebuilt the identical sentence, paying a full walk for a tree that never changed; and the walk was started before `loadStatus()`, so a fast answer could render trunk's count under the ticket number the delete had just cleared. **Fixed** — `movedToTrunk` (new, tested) gates it, and it runs after the status reload. 5. **architecture — the discard behind "discard them and start clean" dropped its recount.** Harmless on the happy path, since the switch that follows re-walks; but a switch that *fails* returns without re-probing, leaving the note offering to discard trunk work that was already gone — finding 1's shape on a narrower path. **Fixed.** The second pass also verified the parts most likely to be wrong and found them sound: the `do/while` probe queue cannot spin or leak `inFlight` (the only `await` is inside the inner try, and no `await` separates the loop test from the `finally`), the `useCallback` dependency arrays are complete, `setWorktreeDirty(null)` is safe for its single reader, and `classifyChangedFile` preserves the original precedence exactly so patch output is byte-identical. </details> <details> <summary>Implementation notes</summary> - **`src/main.js`** — `classifyChangedFile` extracted from `createMinimalPatchForDir`, preserving its precedence (unreadable workdir → unreadable base → binary → line-ending-equal → text) so the patch is unchanged. `collectUnsubmittedFiles` runs `patchBaseOid` + `collectChangedFiles` and keeps everything the patch would speak about. New `git:unsubmitted-work` handler beside `git:worktree-dirty`, same reply shape. `git:discard-changes` appends the recount. - **`src/preload.js`** — `hasUnsubmittedWork`, a named bridge function; the surface is not widened. - **`src/renderer/changes-note.cjs`** — `discardOutcome` passes a recount through when there is one; new `noteAfterDiscard` decides the post-discard card state, falling back to clean when a reply carries no recount (what the old `markTreeClean` asserted unconditionally). - **`src/renderer/index.jsx`** — the probe reads the new channel; `markTreeClean` becomes `applyDiscardToNote`; `reprobeAfterBranchChange` added and called from `saveTicket` (which every link, unlink, resume and carry funnels through) and `deleteTicketWork`. - **On trunk** `patchBaseOid` returns `null` and the walk falls back to `HEAD`, so a site that never linked a ticket gets exactly the answer it always got. Pinned by `git:unsubmitted-work matches git:worktree-dirty on trunk (#239)`. </details> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
…es that predate it (#245) > [!WARNING] > **Do not merge this before the ticket-branches stack (#168 #185 #198 #205 #218 #238 #241 #242 #243).** It documents features that are not on trunk yet. Merged first, the published guide describes an app nobody can download. ## Why The user guide describes a site as one working tree and one implicit patch, because that is what the app was when the guide was written. The ticket-branches stack makes a ticket a branch on the site, which leaves several pages not merely incomplete but wrong: what a patch contains, what **Unlink** costs you, what a trunk update does to work in progress, whether the applied-patch banner describes the ticket you are looking at. The stack also adds surface nothing documents at all — the tickets card, the question asked about edits made before a ticket was picked, the note about unsubmitted work, the switch progress line, coloured pull request states. ## What changes Docs only. The diff is `docs/` — guide pages, `docs/.vitepress/config.mjs` for the sidebar, and screenshots. No `src/`, no `scripts/`. **New page: `guide/ticket-branches.md`, "Working on several tickets".** The model (a site is the expensive substrate, a ticket is a cheap branch), starting a second ticket, the tickets card in both its states, what a switch says while it runs and what to do when one fails part-way, deleting one ticket's work, the four-choice question about loose trunk edits, the unsubmitted-work note, and what a ticket's patch does and does not contain. Added to the sidebar under "Contributing changes", after "Working on a Trac ticket". **Corrections to pages that describe the old model:** | Page | What was wrong | | --- | --- | | `trac-tickets.md` | Unlink described as only forgetting an association; no mention that a site holds many tickets, that linking asks about loose edits, or that PR states are now coloured pills with a third value | | `submitting-changes.md` | "everything this site has that its copy of trunk does not" — the diff is now the ticket's own work, measured from its branch point. Also gained what a patch can and cannot carry: deletions now travel, binaries and unreadable files are named above the diff rather than dropped | | `trunk-updates.md` | Said nothing about being on a ticket — that the update parks and returns, and that the branch point deliberately does not move | | `applying-patches.md` | Applied-patch state is per ticket now; and "use Update to latest trunk to reset the checkout" does not hold on a ticket branch, where the update puts you back exactly where you were | | `creating-a-site.md` | Implied a site per piece of work | | `managing-sites.md` | **Delete this site** takes every ticket's work with it; and its dirty-tree dialog is about loose edits only, never parked ticket work | | `troubleshooting.md` | New entry for the refused-switch message, which is what someone will search for | | `getting-started.md`, `setup-wizard.md`, `submit-*.md`, `running-the-site.md` | The button is **Review & submit changes**; the guide still quoted **Submit changes** everywhere | **Deliberately not in this PR:** new entries in `scripts/screenshots/shots.cjs` for the panels the stack adds. A shot definition belongs with the code that draws the panel, and one added here would fail on trunk where the panel does not exist — see Risks. ## How to test this **Platforms:** any. This is a static site; nothing here touches the app. **Starting state:** this branch checked out. 1. `npm run docs:build` → completes. This is the real check: `ignoreDeadLinks: false`, so a broken link or a missing image fails it. 2. `npm run docs:dev` and open `/guide/ticket-branches` → the page renders, the six screenshots load, and "Working on several tickets" is in the sidebar under **Contributing changes**, between "Working on a Trac ticket" and "Applying patches and PRs". 3. Follow the internal links out of that page — `#what-a-patch-contains`, `#edits-you-made-before-picking-a-ticket`, `#deleting-a-ticket-s-work`, `trunk-updates#updating-while-you-are-on-a-ticket`, `submitting-changes#what-a-patch-can-and-cannot-carry` → each lands on the heading it names, not at the top of the page. 4. `npm run lint` → clean. **And the part a build cannot check** — drive the stack (the Buildkite artifact for `juanmaguitar/pr-state-colours` exercises all nine PRs) with the new page open, and check the prose against the app: 5. On a site with two tickets, read "Starting a second ticket" and follow it literally. → Unlink, then type the second number. The page should not describe a control that is not on screen. 6. Read the switch progress sentences in the page against the ones the panel actually shows. 7. Compare the six screenshots against the panels as they render for you. **What must not have happened:** - **No `src/` or `scripts/` file in the diff.** `git diff --stat origin/trunk...HEAD` must list only `docs/`. If a screenshot-harness change slipped in, it will fail on trunk. - **No invented UI string.** Every label quoted in these pages was read out of the stack's source or seen in the app. A plausible-sounding button that does not exist is worse than no documentation. - **No page left describing the old model.** The table above is the list; if another page still says a site carries one ticket, it was missed. ## Risks and limitations **This PR must merge after the stack.** It is the only real risk here and it is not defended by anything automated: nothing in CI knows the difference. The Pages deploy runs on trunk, so merging early publishes it immediately. - **`scripts/screenshots/shots.cjs` has no entries for the new panels**, so the six new images are not reproducible with `npm run shots` as things stand. They were taken by driving the stack through the same Playwright harness in a throwaway worktree, with fixture and shot definitions that were not committed anywhere. Follow-up, and it belongs on a branch that has the panels: the fixtures need real repositories with ticket branches, where the current ones are empty directories. **Follow-up issue not yet filed** — happy to file it. - **`site-view.png` is re-shot** and is the only existing image replaced. Its button still read **Submit changes**, which was already stale on trunk after #235 renamed it — so that image and the text corrections around it are true of trunk today, stack or no stack. - **The `MERGED` pill is documented but not screenshotted.** A repository-wide search returns no merged pull requests on `wordpress-develop`; the state is real in the API and unit-tested, and #243 says the same. - **The screenshots name a real ticket and its real pull requests** (#29798), pulled live from GitHub when the images were taken. They will age the way any screenshot of live data ages. - Review outcome: **5 [fix here] · 1 [follow-up] — all 6 addressed.** Every one was a factual claim about the app, not prose. Details in the collapsed block. ## Related Documents #168, #185, #198, #205, #218, #238, #241, #242, #243 — the ticket-branches stack, tip branch `juanmaguitar/pr-state-colours`. Part of the contribution-flow tracker #110. Follows the docs site (#230), its screenshot harness (#231) and the guide itself (#232). --- <details> <summary>Design decisions and alternatives considered</summary> **A new page rather than growing `trac-tickets.md`.** That page is about one ticket: linking it, and reading the work that already exists on it. Several tickets is a different subject with its own vocabulary — parking, switching, deleting a branch — and folding it in would have doubled the length of a page a first-time contributor reads on their first ticket, to describe something they do not do yet. `trac-tickets.md` gains pointers instead. **Corrections before additions.** A page that is wrong costs more than a page that is missing: someone acts on it. So the pass over the existing guide came first, and the new page was written to fill what was left. **Screenshots taken by hand rather than by adding shot definitions.** The harness lives on trunk and the panels live on the stack, so a shot definition committed here would fail every `npm run shots` on trunk until the stack lands. The images were taken by assembling the two in a scratch worktree — stack tip, plus trunk's `scripts/screenshots/` and the `TOOLKIT_USER_DATA_DIR` hook, plus fixtures that build real repositories with ticket branches — and only the resulting PNGs were copied here. That worktree is gone; nothing of it is in this diff. **Quoting the app rather than paraphrasing it.** Where a sentence in the app is the thing being explained — the question about loose edits, the switch stages, the confirm before a delete — the page quotes it verbatim, so someone reading with the app open can match what they see. Every quote was read out of the stack's source. </details> <details> <summary>Review outcome (5 [fix here] · 1 [follow-up] — all addressed)</summary> Run per `.github/instructions/code-review.instructions.md`. Deterministic layer inline and clean — `npm run lint` clean, `npm test` 638/638, `npm run docs:build` clean. The judgement pass went to a subagent with the diff, the instructions file and the stack's source, and nothing from the session that wrote the pages; it was asked to check every quoted string and every behavioural claim against the code rather than to read the prose. Every finding was an accuracy finding — the guide asserting something the app does not do. All five were verified against the source before being fixed. | # | What was wrong | | --- | --- | | 1 🔴 | The new page's central how-to. "Type the number into the **Trac ticket** panel" — the panel holds no input while a ticket is linked; the field is in the unlinked branch only. The route is **Unlink**, then link the new number. Unfollowable as written. | | 2 🟡 | The mid-switch advice described a recovery that does not exist: the marker is written only when a checkout throws inside a running app, so a force-quit leaves nothing behind, and `midSwitchBlock` refuses the retry along with everything else. The one action it allows is **Unlink**, which is now what the page and `troubleshooting.md` say. | | 3 🟡 | `applying-patches.md`'s "use **Update to latest trunk** to reset the checkout" escape hatch. On a ticket the update parks, resets trunk and checks the branch back out — applied patch included — so it leaves you where you started. | | 4 🟡 | "Nothing is rebuilt" was true and misleading: a switch does not rebuild, so a running dev server keeps serving the previous ticket's assets. The page now says to run `npm run build` after a switch, which is what the site view itself says. | | 5 🔵 | "If the note says two changes, the patch has two files." The note counts everything the patch speaks about, binaries included, and those are named above the diff rather than carried — so one text file and one image is two in the note and one in the diff. It also contradicted this PR's own `submitting-changes.md`. | | 6 🔵 | The by-hand replay recipe skipped the trunk update that gives the new branch a newer base, and offered **Delete this ticket's work** for the ticket you are on, which the card never lists. | **`[follow-up]`, both taken here rather than deferred** because they were two sentences each: `managing-sites.md` and `trunk-updates.md` described the same dirty-tree dialog differently, and `troubleshooting.md` had no entry for the refused-switch message. Style notes, also applied: the tickets card is additionally gated on the setup checklist being finished or skipped, and the loose-edits question can be raised from the **Attach to Trac** card's link field too. The pass also confirmed what was most likely to be wrong and was not: every other quoted label and message is verbatim, all six screenshots match their alt text, `#deleting-a-ticket-s-work` resolves under VitePress's slugifier, and the new page is in the sidebar. </details> <details> <summary>Implementation notes</summary> **How the screenshots were taken.** `git worktree add` from the stack tip, then `git checkout <trunk> -- scripts/screenshots docs/…`, `playwright-core` installed, and the `TOOLKIT_USER_DATA_DIR` hook from `src/main.js` re-applied — the stack predates all of it, so `npm run shots` does not exist on that branch. Two throwaway fixture variants were added, both building real repositories through the app's own `src/ticket-branches.js` (`startTicketBranch`, `parkCurrentWork`, `switchToBranch`) so the state is the state the app makes: one site on `ticket/29798` with parked work and a second branch, one parked back on trunk with a loose edit. Same 1200×800 window and `--force-device-scale-factor=1` as `capture.cjs`, so the new images sit beside the existing ones without a size jump. The worktree has been removed. **The new slugs**, none of which `shots.cjs` knows about: `ticket-list-card`, `ticket-list-unlinked`, `trunk-work-question`, `carried-work-notice`, `linked-pull-requests`, `site-with-tickets`. **`linked-pull-requests.png` is one image doing three jobs** — the coloured state pills (#243), the unsubmitted-changes note (#241), and the reorganised ticket card with the tickets list no longer inside it (#242). **`docs/` is its own npm package**, so `npm run docs:build` needs `npm --prefix docs ci` first on a fresh checkout. </details> <details> <summary>Screenshots or recording</summary> Six new images, and one replaced. **The tickets card, linked** — `ticket-list-card.png`:  **The tickets card, unlinked** — `ticket-list-unlinked.png`:  **The question about loose trunk edits** — `trunk-work-question.png`:  **After choosing to carry them** — `carried-work-notice.png`:  **The ticket card with coloured pull request states and the unsubmitted-work note** — `linked-pull-requests.png`:  **The three cards in their new order** — `site-with-tickets.png`:  **Replaced:** `site-view.png`, whose action bar read **Submit changes**. No stack behaviour is visible in it — it is a straight re-take of a stale image. </details> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
Found by driving the app, not by the suite.
Link a ticket while you have uncommitted changes on trunk and one of two things happens, depending on something you cannot see:
Same starting state, same click, opposite outcomes, and nothing on screen telling you which one you are in. The reporter's words were "algo nos hemos cargado por el camino" — it read as a regression, because the app had asked the last time. It had not regressed; they had linked a ticket the other site happened to already have a branch for.
Carrying the work is the right behaviour and stays. I started editing, then realised which ticket this is is a real sequence, and a confirmation dialog on it would punish the correct flow. The bug is that the app moves someone's work without saying so.
What changes
Two sentences.
Afterwards — a notice naming how much came along and where: "Your 2 uncommitted changes came along into #62281, and will go into its patch."
Beforehand — a line under the Link ticket field: "Anything you have edited so far will come along into a ticket this site has not worked on yet." Scoped on purpose: it is false for the tickets listed directly above it, which are refused instead.
The decision a reviewer would otherwise have to reconstruct: the count runs after the answer, on its own channel. Linking a ticket is a HEAD move and is instant; counting is a walk of the whole worktree. Holding the reply for it would make the most common Contributor Day sequence — install a site, link a ticket, then start editing — pay a scan for an answer that is always zero. It is counted against the new branch's own HEAD, so the number says what this ticket's patch will contain rather than what trunk happened to have.
And it is not a stage of the switch progress (#173), which was my first attempt: on trunk with no ticket to name, a
scanstage renders as "Saving your work…" — about the one branch this whole feature refuses to write to.Deliberately not in this PR: the count is
statusMatrix's idea of changed, so it includes untracked files a contributor may not think of as edits. Naming the files instead of counting them would be truer; it is also a different, larger design.How to test this
Platforms: any. Stacked on #205.
Starting state: a site with no ticket branches, sitting on trunk.
What must not have happened:
wordpress-develop, the count is back on the reply path.What could not be tested by hand: a worktree that cannot be walked (a permission, a volume that went away). Covered by a wiring test asserting the link still succeeds, no notice is sent, and the failure reaches the log.
Risks and limitations
Review outcome: 6
[fix here]· 2[follow-up]— all 6 fixed. Two are worth naming here because both were mine, introduced in the first commit: the count sat in front of the reply, and the progress frames I wrapped it in rendered as "Saving your work…" on trunk followed by "Ready to work on #N" before the branch existed.Related
Part of #108. Stacked on #205 → #198 → #185 → #168.
Design decisions and alternatives considered
Why not ask before carrying? Because the carrying case is the good one. "I started editing, then realised which ticket this is" is the sequence the behaviour was designed for, and a confirmation on every new ticket would tax the correct flow to fix what is an information problem, not an intent problem.
Why a separate channel instead of the switch progress stream? The progress stream describes an operation while it runs, and its vocabulary is stages. This is one fact after the fact. Forcing it into a stage produced literally false sentences (above) — which is the argument, rather than tidiness.
Why count against the branch's HEAD rather than trunk? They are the same commit at that moment, since
git.branch({checkout:true})only moves HEAD. But counting against the branch says "what this ticket's patch will contain", which is what the sentence claims; counting against trunk would be describing where the work came from.Review outcome (required — see AGENTS.md)
6 [fix here] · 2 [follow-up] — all 6 fixed. Judgement pass in a fresh context per the skill, on the first commit here.
.catch(() => 0)was a silent catch against the repo's explicit invariant — and.catchdoes not cover a synchronous throw, which is exactly what a partial test stub produced. Now logged.[follow-up]: the count including untracked files, and the index-vs-worktree edge where a staged-then-reverted file is not counted. Both in Risks.Worth flagging rather than burying: every one of the six was in code this PR added. The change is small; the first attempt at it still managed to put a lie on screen and a scan on the critical path.