Say what actually went wrong when a folder will not open - #211
Merged
Conversation
Revealing a site's folder in the file manager built its sentence out of the `error` field alone, falling back to the words "unknown error" when there was none. A refusal is exactly the case with no `error`: the main process declined on purpose and returned a `reason` saying which gate it was. So the failure the app understood best was the one it described as unknown — most visibly while a site is still being cloned, where every reveal is refused and the notice explains nothing (#180). The editor menu already grew the vocabulary in #209, a branch per reason. What it did not have is one place that decides the whole notice, so the two callers each decided separately whether there was a failure worth mentioning and what to render beside it — and one of them got it wrong. `noticeForOpenResult` is now that place, and the window is left with an assignment. Three things fall out of moving the decision rather than only the sentence: `open-failed` gets a branch. It is the file manager's own refusal, it carries the OS's message, and it would have fallen through to the generic sentence the moment the reveal path started asking. "Choose application…" is offered per reason instead of always. It answers "that editor did not work". It is not an answer to a refused folder — the folder is checked before the application is, so picking another one returns the identical sentence — and there is no second file manager to choose. The generic fallback stops naming an application, since both callers share it now and only one of them is opening one. Part of #180. The refusal itself is still there; this is the message. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 9, 2026
…eir source The exhaustiveness test scanned editor-launch.js and site-registry.js with a regex to learn what they can refuse with. A scan over source text breaks on a formatting change and goes silently vacuous when the pattern stops matching — it needed its own guard against matching nothing. Now each guard module exports the complete list it answers with: REFUSAL_REASONS grows the SPAWN_FAILED member it was already returning as a repeated literal, and site-registry.js gains REVEAL_REASONS for its two. The test reads those objects directly, so a new reason still cannot arrive without a sentence and a picker judgement — but through the module boundary instead of a grep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 10, 2026
## 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>
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
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
Revealing a site's folder in the file manager failed with
Could not open the folder: unknown error. The app knew exactly what had gone wrong — it had refused on purpose — and said the one thing that was not true.It happens most visibly while a site is still being created: the clone takes minutes, the site is not in the registry until it finishes, and every reveal in that window is refused. That is #180, and the wrong message is the half a contributor actually meets.
What changes
The root cause is not the sentence, it is that there were two places deciding it.
showInFileManagerbuilt its own string out ofresult.erroralone, and a refusal is precisely the case with noerror— it carries areasoninstead. #209 had already written the vocabulary for the editor menu; the reveal path never asked it.So the decision moves, not just the words.
noticeForOpenResultin the newsrc/renderer/open-failure.cjsanswers all three questions the two callers were answering separately — is there a notice at all, what does it say, and is "Choose application…" a way out of it — and the window is left with an assignment.Three things fall out of moving the decision rather than only the sentence:
open-failedgets a branch. It is the file manager's own refusal and it carries the OS's message, which the hand-rolled string did surface. Reusing the editor's vocabulary without adding it would have been a regression dressed as a cleanup.openSiteInEditorchecks the folder before it looks at the editor, so picking another one returns the identical sentence — and there is no second file manager to choose.This is PR 1 of 3 on #180 and it deliberately does not make the folder open; the refusal is still there. It ships first because it stands on its own: any
openPathfailure, guard or not, currently ends in "unknown error".How to test this
Platform: any. Nothing here is platform-specific — the same refusal and the same notice on macOS, Windows and Linux.
Starting state: the app open with at least one site that has finished setting up.
What must not have happened:
{ message, offerPicker }; a missed read would render an empty warning box rather than throw. Any notice you can produce must have visible text in it.To watch the old behaviour, do step 2 on a build from
trunk: the notice readsCould not open the folder: unknown error.Risks and limitations
Self-review came back 1 [fix here] · 2 [follow-up] on the first pass and 1 [fix here] · 1 [follow-up] on the re-review; everything at
[fix here]is fixed, and the detail is in the collapsed block below. The first pass caught something worth stating plainly: my original test claimed to reproduce #180 and did not, because the sentence was never the broken part. That is what drove the restructure.The honest limitation is that nothing fails if the wire is cut. These tests cover the module; reverting the call site in
index.jsxto the hand-rolled string would leave the suite green.test/ipc-wiring.test.cjsexists for exactly that class of gap on the main-process side, but there is no renderer equivalent, andsetup-steps.cjs,terminal-hints.cjsandupdate-plan.cjsall have the same unproven wire. This PR inherits the hole rather than digging it; it is filed as a follow-up rather than solved here.I could not test the OS-failure branch by hand on Windows — step 2 above was driven on macOS. The message it quotes comes from
shell.openPath, which is Electron's, not ours.Related
Part of #180. Follow-ups: #202, #203 are unrelated to this one.
Design decisions and alternatives considered
Why not just add the
reasonbranch toshowInFileManager's string? That is the two-line version, and it leaves two places deciding the same thing — which is the actual defect. The next reason added to either guard module would have to be remembered in both.Why a
.cjsmodule rather than a hook or a helper inside the component? It is the house pattern for renderer logic worth testing:setup-steps.cjs,path-basename.cjs,update-plan.cjs. The renderer bundle imports it,node --testrequires it directly, and neither needs a DOM.Why derive
offerPickerfrom the reason rather than from the caller? The first draft passedofferPicker: truefrom the editor path andfalsefrom the reveal path, which reads as correct and is not:unregistered-siteandunavailablereach the editor caller too, and the picker is a dead end for both. The reason knows; the caller only thinks it does.Review outcome (required — see AGENTS.md)
Two passes, both dispatched to a subagent with fresh context per
.claude/skills/self-review/SKILL.md.First pass — 1 [fix here] · 2 [follow-up].
describeOpenFailurealready handledunregistered-siteon trunk; the bug was at the call site. Reverting only the call site would have left the whole new suite green with "Show in Explorer" and "Open in editor" fail with "unknown error" while a new site is still being set up #180 back. Fixed by moving the mapping — not just the sentence — intonoticeForOpenResult, which is the restructure this PR is now built around.offerPickerwas unconditional on the editor path. Fixed, not deferred: derived from the reason viaPICKING_HELPS.Re-review — 1 [fix here] · 1 [follow-up]. All three above confirmed resolved by reading.
editor-launch.jsandsite-registry.js(REFUSAL_REASONSplus scanned literals) and fail if a reason is undescribed or unjudged. I verified the guard bites by adding a fake reason tosite-registry.js— two tests fail — then reverting it. The scan also asserts it matched something, so it cannot go vacuously green.Screenshots or recording
The only visible change is the text inside the existing notice, and the absence of the Choose application… button beside a file-manager failure. The notice box, its position and its styling are untouched.
Before:
Could not open the folder: unknown error, with a Choose application… button that returns the same sentence.