Let a site's folder be opened while it is still being cloned - #212
Merged
Conversation
Creating a site clones wordpress-develop, which takes minutes. The window shows the site immediately and the directory exists from the first moment, but `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 for the whole clone the app refused to open a folder it had just created, and there was no way to look at the checkout while watching it arrive (#180). Registering the site early is the obvious fix and the wrong one. `sites` is persisted, so a half-cloned directory written into it survives the crash or the quit that no unregister-on-failure path can catch — a phantom site for a directory that was never finished. It would also widen the allow-list for the recursive delete in `sites:delete` to include a tree isomorphic-git is writing into, and quietly change what `sites` means for every registry already on disk. So the second record is liveness rather than truth. `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 to reconcile. It sits beside the six per-site maps in main.js that already hold liveness this way. The two verbs then want opposite answers about a site being created, and get them: `isActionableSite` widens revealing and opening to include it, while `deleteRegisteredSite` refuses it outright, registered or not. Until now that refusal came for free from the path not being in `sites` yet; making the folder openable is what took the accident away, so the guard is now explicit. The menu stops offering "Delete this site" mid-clone to match — the refusal is the backstop, not the answer. `wordpress:setup` was listed as NOT_REACHABLE for wiring tests on the grounds that it clones over the network. It does not have to: the harness resolves bare packages, so isomorphic-git is stubbable and the handler runs offline. That is what lets a test be *inside* the clone, which is the only place this bug is visible. Fixes #180 for a site whose directory name was free. The window still sends its own guess of the path, so a name collision is refused until the follow-up that has it adopt the real one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…e-it-is-cloning # Conflicts: # src/main.js # test/ipc-wiring.test.cjs # test/site-registry.test.cjs
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
> 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>
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
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:showandeditor:openare gated on thesitesregistry, 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.
sitesis 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 insites:deleteto include a treeisomorphic-gitis actively writing into, and silently change whatsitesmeans 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.jsholds the paths this process is setting up right now, keyed by the directory main computed itself and released in afinallyhowever 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 inmain.jsthat already hold liveness this way —playgroundServers,runningInstalls,runIdByDirectoryand 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.
isActionableSitewidens revealing and opening to include a pending path;deleteRegisteredSiterefuses 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 insitesyet — 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:setupgets its first tests. It was listed asNOT_REACHABLEintest/ipc-wiring.test.cjson the grounds that it clones over the network. It does not have to: the harness resolves bare packages throughrequire.resolve, soisomorphic-gitis 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 (
demoexists → main createsdemo-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.
What must not have happened:
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:
pendingSitesin 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 thefalse, 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.
Design decisions and alternatives considered
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
siteDirrather 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.Review outcome (required — see AGENTS.md)
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.pendingwas passed toeditor:openandsites:deletewith nothing covering it. Deleting either line left the suite green while half of "Show in Explorer" and "Open in editor" fail with "unknown error" while a new site is still being set up #180 was broken again; the unit tests could not see it because they handpendingto 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.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.pendingSitesclearing, described under Risks.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.cjsinstead.Screenshots or recording
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.