Show the directory the app created, from the moment it creates it - #223
Merged
Conversation
The window has to draw a site's row before it can know where the site will be: the contributor picked a parent directory and typed a name, and the main process is the one that turns those into a directory. So the row started on a guess, built by joining the two with a separator sniffed out of the parent path. The guess is wrong whenever the folder name is already taken, because `findAvailableDirName` appends `-2`. That was cosmetic while the path was only something to display — it read wrong under the site title for the length of the clone and then corrected itself. It stopped being cosmetic once the guards started keying on the directory the app actually created: the row hands its path to `dir:show` and `editor:open`, so on a collision it was asking about a folder the app had never made and being refused. When the guessed name belongs to a different registered site, it was asking about that one. Main already reports the real directory on its first status event, minutes before the clone ends, so the row adopts it there and the guess stops existing. The selection follows it; otherwise the panel points at a path no longer in the list and the checklist vanishes mid-clone. Which event to believe turned out to be the whole problem. A first version moved the row for any status naming a different directory, which 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 and its log across. `cloning` is the event that announces a directory and arrives once per setup, so it is the only one that moves a row — and that decision is a tested function rather than a condition in a subscription, because it is the part that was wrong. Two setups cannot run at once anyway, and now the app says so. This flow has always been single-file — one pending card, one terminal, one `clearPendingSites()` that clears them all — and the create button was the only door left open on a second one. Adopting early is also what makes the failure branch dangerous. It discarded the guessed path, which was safe only because the swap could not have happened yet; now it would strand a row for a directory whose setup just failed. So the three moves that were three divergent copies inline are one tested reducer, and discarding takes whatever path the row currently has. `sites` and `siteMeta` become one piece of state, because 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 nothing else changed. Closes #180: the collision case was the last part still refused. 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
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
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 —
findAvailableDirNameappends-2when 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:showandeditor: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
donewould drag the other one's row onto its own path, carrying its label, its created date and its log across.cloningis 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 nowrowPathAfterStatus, 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.sitesandsiteMetabecome 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.setSiteMetakeeps its signature, so no other caller changed;setSiteshad 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 calleddemo— main will makedemo-2.…/demo-2from the first moment — not…/demo.demo-2, the folder being created. Before this change it was refused, and ifdemowas a registered site it would have opened that one.What must not have happened:
demoanddemo-2appear for the same setup. The adoption replaces the row rather than adding one.createdAtsorts 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:
rowPathAfterStatuswas 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.
Design decisions and alternatives considered
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
setupWordPressto 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.
Review outcome (required — see AGENTS.md)
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.doneevent satisfied the adoption condition and moved the second setup's row onto the first's directory, merging its label andcreatedAtover 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 — "Show in Explorer" and "Open in editor" fail with "unknown error" while a new site is still being set up #180 reopening for the second site. Fixed two ways, because one was not enough: adoption is restricted tocloning, 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.rowPathAfterStatuswas 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.Confirmed clean by the reviewer, by reading: no remaining
setSitescallers; the threesetSiteMetacallers unaffected; the merge removes an intermediate render rather than adding one;activeSitecannot race thesortedSitesfallback because the IPC callback is auto-batched; bothadoptSetupPathguards return the same object so React bails out;moveSetupLogis idempotent and its alias chain handles the double hop.Screenshots or recording
The visible change is the path under the site title during a clone when the folder name was taken —
…/demo-2from the start instead of…/demountil the clone ends — and the create button being disabled while a setup runs. No layout or styling changed.