From f1aba441b4161d4a970639fe5c7e317e9b6b44ca Mon Sep 17 00:00:00 2001 From: JuanMa Date: Sun, 9 Aug 2026 22:06:22 +0200 Subject: [PATCH] Show the directory the app created, from the moment it creates it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/renderer/index.jsx | 119 ++++++++++++++--------- src/renderer/pending-setup.cjs | 123 ++++++++++++++++++++++++ test/pending-setup.test.cjs | 166 +++++++++++++++++++++++++++++++++ 3 files changed, 363 insertions(+), 45 deletions(-) create mode 100644 src/renderer/pending-setup.cjs create mode 100644 test/pending-setup.test.cjs diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index 4160ef9..6b61600 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -26,6 +26,7 @@ import { planDevServerStart, formatElapsed } from './dev-server-command.cjs'; import { pathBasename } from './path-basename.cjs'; import { trunkAgeInfo, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, APPLY_STATE_TO_STEP } from './update-plan.cjs'; import { pickLatest } from '../latest-patch.cjs'; +import { beginSetup, adoptSetupPath, discardSetup, rowPathAfterStatus } from './pending-setup.cjs'; import { parsePrRef } from '../patch-sources.cjs'; import { ticketUrl, attachUrl } from './trac-ticket.cjs'; import { highlightDiff } from './diff-highlight.cjs'; @@ -190,25 +191,41 @@ function useContributorProvenance() { return { handle, event, rememberHandle, rememberEvent }; } +// The two halves are one piece of state because one change moves both: adopting +// the directory the app really created has to retire the guessed row and carry +// its metadata across, and two setters cannot do that without a render in +// between where the site has a path under one key and a label under another. +// `setSiteMeta` keeps its old signature so every other caller is untouched; +// `applySetup` is for the changes that need the pair, which is every change the +// create flow makes. function useSites() { - const [sites, setSites] = useState([]); - const [siteMeta, setSiteMeta] = useState({}); + const [state, setState] = useState({ sites: [], siteMeta: {} }); + const setSiteMeta = useCallback((update) => setState((prev) => ({ + ...prev, + siteMeta: typeof update === 'function' ? update(prev.siteMeta) : update + })), []); + const applySetup = useCallback((fn) => setState(fn), []); const refresh = useCallback(async () => { const { sites: list, siteMeta: meta } = await window.api.getSitesWithMeta(); - setSites(list); - setSiteMeta(meta || {}); + setState({ sites: list, siteMeta: meta || {} }); }, []); useEffect(() => { refresh(); }, [refresh]); - return { sites, siteMeta, refresh, setSiteMeta, setSites }; + return { sites: state.sites, siteMeta: state.siteMeta, refresh, setSiteMeta, applySetup }; } function App() { - const { sites, siteMeta, refresh, setSiteMeta, setSites } = useSites(); + const { sites, siteMeta, refresh, setSiteMeta, applySetup } = useSites(); // One answer for the window, shared by every site row: which applications this // machine has is a fact about the machine, not about a site. const detectedApplications = useDetectedEditors(); const wporg = useContributorProvenance(); const [downloadPhase, setDownloadPhase] = useState(''); + // Where the row for the setup in flight currently lives. It starts as the + // window's guess and becomes the directory the main process reports, and it + // is a ref because the status subscription below has to read it without being + // torn down and rebuilt every time it changes. Null when nothing is being + // created. + const setupRowPathRef = useRef(null); // Directories whose clone is still running. An array rather than a single // path because the main process may settle on a different (deduplicated) // directory than the one the renderer optimistically created a row for. @@ -330,6 +347,22 @@ function App() { }); const unsubStat = window.api.subscribeSetupStatus((s) => { if (!s) return; + // The first moment the window learns where the site is actually being + // made. Until now the row kept the guess it was drawn with, which differs + // whenever the folder name was taken — so it showed the wrong path and, + // once the guards started keying on the real directory, asked about a + // folder the app had never created (#180). + const guess = setupRowPathRef.current; + const adopted = rowPathAfterStatus(guess, s); + if (adopted) { + setupRowPathRef.current = adopted; + moveSetupLog(guess, adopted); + applySetup((state) => adoptSetupPath(state, { from: guess, to: adopted })); + // The selection follows the row. Without this the panel is pointed at a + // path that no longer exists in the list, and the contributor watches + // their new site's checklist disappear mid-clone. + setActiveSite((current) => (current === guess ? adopted : current)); + } if (s.target && s.phase !== 'done') addPendingSite(s.target); const key = s.sitePath || s.target; if (key) { @@ -341,14 +374,22 @@ function App() { else if (s.phase === 'done') { setDownloadPhase(''); clearPendingSites(); setTerminalMsgs(''); } }); return () => { if (unsubProg) unsubProg(); if (unsubStat) unsubStat(); }; - }, [addPendingSite, appendSetupLog, clearPendingSites]); - + }, [addPendingSite, appendSetupLog, applySetup, clearPendingSites, moveSetupLog]); + + // Refused while one is already running. Everything about this flow is + // single-file and always has been — one pending card, one terminal, one + // `clearPendingSites()` that clears them all — and `setupRowPathRef` is one + // slot for the row being created. The button was the only door left open on a + // second setup, and a second setup does not half-work: it adopts the other + // one's row. Until the flow is genuinely per-site, saying no is the honest + // shape. const chooseAndSetup = useCallback(() => { + if (createSubmitting) return; setCreateSiteName(''); setCreateSiteDir(''); setCreateSiteError(''); setCreateModalOpen(true); - }, []); + }, [createSubmitting]); const sanitizeSiteFolder = useCallback((value) => ( value @@ -431,15 +472,11 @@ function App() { let finalSitePath = targetDir; const placeholderCreatedAt = new Date().toISOString(); - setSites((prev) => (prev.includes(targetDir) ? prev : [...prev, targetDir])); - setSiteMeta((prev = {}) => ({ - ...prev, - [targetDir]: { - ...(prev[targetDir] || {}), - label: nameTrimmed, - createdAt: prev[targetDir]?.createdAt || placeholderCreatedAt, - initialized: false - } + setupRowPathRef.current = targetDir; + applySetup((state) => beginSetup(state, { + path: targetDir, + label: nameTrimmed, + createdAt: placeholderCreatedAt })); setActiveSite(targetDir); setCreateModalOpen(false); @@ -455,48 +492,38 @@ function App() { const createdPath = await window.api.setupWordPress(createSiteDir, { siteName: cleanFolder, siteLabel: nameTrimmed }); if (createdPath) { finalSitePath = createdPath; - if (createdPath !== targetDir) { + // Ordinarily already done, by the `cloning` status this handler's own + // clone sent minutes ago. Kept because the status event is not a + // guarantee — a missed one would otherwise leave the row on the guess + // for good — and adopting a path the row already has is a no-op. + const current = setupRowPathRef.current; + if (current && current !== createdPath) { addPendingSite(createdPath); - moveSetupLog(targetDir, createdPath); - setSites((prev) => { - const filtered = prev.filter((path) => path !== targetDir); - return filtered.includes(createdPath) ? filtered : [...filtered, createdPath]; - }); - setSiteMeta((prev = {}) => { - const next = { ...prev }; - const placeholder = next[targetDir] || { createdAt: placeholderCreatedAt, initialized: false }; - delete next[targetDir]; - next[createdPath] = { - ...placeholder, - label: nameTrimmed, - createdAt: placeholder.createdAt || placeholderCreatedAt, - initialized: false - }; - return next; - }); + moveSetupLog(current, createdPath); + applySetup((state) => adoptSetupPath(state, { from: current, to: createdPath })); } + setupRowPathRef.current = createdPath; } await refresh(); setActiveSite(finalSitePath); appendSetupLog(finalSitePath, 'Site setup request completed.\n'); } catch (e) { + // Whatever the row is *now*, which is not necessarily what it started as: + // once the clone reports its directory the guess no longer exists, and + // discarding the guess here would strand a row for a setup that failed. + const rowPath = setupRowPathRef.current || targetDir; setCreateSiteError(String(e)); - appendSetupLog(targetDir, `Setup failed: ${String(e)}\n`); - setSites((prev) => prev.filter((path) => path !== targetDir)); - setSiteMeta((prev = {}) => { - if (!prev[targetDir]) return prev; - const next = { ...prev }; - delete next[targetDir]; - return next; - }); + appendSetupLog(rowPath, `Setup failed: ${String(e)}\n`); + applySetup((state) => discardSetup(state, rowPath)); } finally { + setupRowPathRef.current = null; // `setupWordPress` resolving (or throwing) *is* the clone finishing, so // clearing here guarantees the checklist can never stay locked even if // the `done` status event is missed. clearPendingSites(); setCreateSubmitting(false); } - }, [addPendingSite, appendSetupLog, clearPendingSites, createSiteDir, createSiteName, moveSetupLog, refresh, resolveTargetDir, sanitizeSiteFolder, setSiteMeta, setSites]); + }, [addPendingSite, appendSetupLog, applySetup, clearPendingSites, createSiteDir, createSiteName, moveSetupLog, refresh, resolveTargetDir, sanitizeSiteFolder]); const closeCreateModal = useCallback(() => { if (createSubmitting) return; @@ -736,8 +763,10 @@ function App() { icon={plus} variant="primary" onClick={chooseAndSetup} + disabled={createSubmitting} style={{ width: '100%', justifyContent: 'center' }} aria-label="Create WordPress Core site" + label={createSubmitting ? 'Finish creating the current site first' : undefined} > {!sidebarCollapsed ? 'Create WordPress Core site' : null} diff --git a/src/renderer/pending-setup.cjs b/src/renderer/pending-setup.cjs new file mode 100644 index 0000000..f50f43f --- /dev/null +++ b/src/renderer/pending-setup.cjs @@ -0,0 +1,123 @@ +// The row for a site that is being created, while it is being created. +// +// The window has to draw one before it can know where the site will be. The +// contributor picked a parent directory and typed 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. +// +// A guess was harmless while it was only a label. It stopped being harmless +// when the guards started keying on the directory the app actually created +// (#180): 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 for it. Worse, when the guessed name belongs to a *different* +// registered site, it is asking about that one. +// +// Main reports the real path on its first status event, minutes before the +// clone ends. Adopting it there makes the row honest for the whole clone — the +// path under the title included, which until now simply read wrong. +// +// The three moves are here, and pure, because there were three divergent copies +// of them in the component and because adopting earlier is what makes the third +// one dangerous: the discard branch used to filter the guessed path, which was +// only ever safe because the swap could not have happened yet. +// +// Every function returns new state and leaves its argument alone — these feed +// React setState updaters, which may run more than once. +'use strict'; + +/** + * The optimistic row, before the main process has answered. + * + * @param {{sites: string[], siteMeta: Object}} state + * @param {{path: string, label: string, createdAt: string}} site + * @return {{sites: string[], siteMeta: Object}} + */ +function beginSetup({ sites, siteMeta }, { path, label, createdAt }) { + return { + sites: sites.includes(path) ? sites : [...sites, path], + siteMeta: { + ...siteMeta, + [path]: { + ...(siteMeta[path] || {}), + label, + createdAt: siteMeta[path]?.createdAt || createdAt, + initialized: false + } + } + }; +} + +/** + * Moves the row from the guessed path to the one the app created. + * + * Everything the contributor supplied moves with it, `createdAt` included: + * losing that would drop the row to the bottom of a sidebar sorted by it, in + * the middle of watching the site being made. + * + * @param {{sites: string[], siteMeta: Object}} state + * @param {{from: string, to: string}} move + * @return {{sites: string[], siteMeta: Object}} The same state when there is + * nothing to move — main reports the path more than once per setup. + */ +function adoptSetupPath(state, { from, to }) { + if (from === to) return state; + if (!state.sites.includes(from)) return state; + + const kept = state.siteMeta[from]; + const siteMeta = { ...state.siteMeta }; + delete siteMeta[from]; + + const withoutGuess = state.sites.filter((p) => p !== from); + return { + sites: withoutGuess.includes(to) ? withoutGuess : [...withoutGuess, to], + siteMeta: { ...siteMeta, [to]: { ...(siteMeta[to] || {}), ...kept } } + }; +} + +/** + * Drops the row for a setup that failed. + * + * The caller passes the path the row currently has, not the one it started + * with. That distinction is the whole reason this is not a one-liner at the + * call site: after an adoption the guess no longer exists, and filtering it + * would leave the real row behind for a directory whose setup just failed. + * + * @param {{sites: string[], siteMeta: Object}} state + * @param {string} path + * @return {{sites: string[], siteMeta: Object}} + */ +function discardSetup(state, path) { + const siteMeta = { ...state.siteMeta }; + delete siteMeta[path]; + return { sites: state.sites.filter((p) => p !== path), siteMeta }; +} + +/** + * The path the in-flight row should move to for a status event, or null when it + * should stay where it is. + * + * Extracted from the subscription because the decision, not the reducer, is + * where this went wrong. The reducer is new code that no old test could have + * failed on; what #180's collision case actually needed was *which* event to + * believe, and a first version that believed any event whose target differed + * let a finishing setup's `done` drag a second setup's row onto its own path. + * + * `cloning` is the one event that announces the directory and arrives exactly + * once per setup, so it is the only one that moves a row. Everything else — no + * setup in flight, a later phase, a target that is already the row's — is "stay + * put", returned as null rather than as a path equal to the current one, so the + * caller cannot accidentally treat it as a move. + * + * @param {?string} currentPath Where the in-flight row is now, or null. + * @param {?Object} status A download:status payload from the main process. + * @return {?string} + */ +function rowPathAfterStatus(currentPath, status) { + if (typeof currentPath !== 'string' || currentPath === '') return null; + if (!status || status.phase !== 'cloning') return null; + if (typeof status.target !== 'string' || status.target === '') return null; + return status.target === currentPath ? null : status.target; +} + +module.exports = { beginSetup, adoptSetupPath, discardSetup, rowPathAfterStatus }; diff --git a/test/pending-setup.test.cjs b/test/pending-setup.test.cjs new file mode 100644 index 0000000..1da320f --- /dev/null +++ b/test/pending-setup.test.cjs @@ -0,0 +1,166 @@ +'use strict'; + +// The row for a site that is being created, while it is being created. +// +// The window has to show one before it can know where the site will be: the +// contributor picked a parent directory and typed a name, and that is all there +// is until the main process answers. So it guesses — and the guess is wrong +// whenever the folder name is already taken, because main appends `-2`. +// +// That mattered once the guards started keying on the real directory (#180): +// the row was sending a path the app had never created, and being refused for +// it. Main reports the real one on its first status event, so the row adopts it +// and the guess stops existing. +// +// The three moves live here rather than inline in the component because there +// were three divergent copies of them, and the discard branch was written when +// the swap could only happen at the very end. Adopting earlier is exactly what +// makes a stale discard possible. + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { beginSetup, adoptSetupPath, discardSetup, rowPathAfterStatus } = require('../src/renderer/pending-setup.cjs'); + +const GUESS = '/Users/dev/sites/demo'; +const REAL = '/Users/dev/sites/demo-2'; +const CREATED_AT = '2026-08-09T10:00:00.000Z'; + +function started(state = { sites: [], siteMeta: {} }) { + return beginSetup(state, { path: GUESS, label: 'Demo', createdAt: CREATED_AT }); +} + +test('a site being created gets a row before there is anything on disk', () => { + const next = started(); + + assert.deepEqual(next.sites, [GUESS]); + assert.equal(next.siteMeta[GUESS].label, 'Demo'); + assert.equal(next.siteMeta[GUESS].createdAt, CREATED_AT); + assert.equal(next.siteMeta[GUESS].initialized, false); +}); + +test('starting a setup leaves the other sites alone', () => { + const other = '/Users/dev/sites/other'; + const next = beginSetup( + { sites: [other], siteMeta: { [other]: { label: 'Other' } } }, + { path: GUESS, label: 'Demo', createdAt: CREATED_AT } + ); + + assert.deepEqual(next.sites, [other, GUESS]); + assert.equal(next.siteMeta[other].label, 'Other'); +}); + +test('adopting the real path replaces the guess rather than adding a second row', () => { + const next = adoptSetupPath(started(), { from: GUESS, to: REAL }); + + assert.deepEqual(next.sites, [REAL], 'the guess must not linger beside the real one'); + assert.equal(next.siteMeta[GUESS], undefined); +}); + +test('what the contributor typed survives the adoption', () => { + const next = adoptSetupPath(started(), { from: GUESS, to: REAL }); + + assert.equal(next.siteMeta[REAL].label, 'Demo'); + assert.equal(next.siteMeta[REAL].createdAt, CREATED_AT, 'the row must not jump in the sidebar order'); + assert.equal(next.siteMeta[REAL].initialized, false); +}); + +// Main sends `cloning` and then resolves with the same path, so the swap runs +// twice for one setup. The second must be a no-op rather than a second row. +test('adopting twice is a no-op', () => { + const once = adoptSetupPath(started(), { from: GUESS, to: REAL }); + const twice = adoptSetupPath(once, { from: REAL, to: REAL }); + + assert.deepEqual(twice.sites, [REAL]); + assert.equal(twice.siteMeta[REAL].label, 'Demo'); +}); + +test('adopting a path that was never guessed leaves the state alone', () => { + const state = started(); + + assert.equal(adoptSetupPath(state, { from: '/Users/dev/sites/unrelated', to: REAL }), state); +}); + +// The trap this module exists to close. The discard branch used to filter the +// guessed path, which was safe only because the swap could not have happened +// yet. Adopting on the first status event breaks that assumption, and a discard +// that still filtered the guess would strand a row for a directory whose setup +// failed. +test('discarding after an adoption removes the adopted row, not the guess', () => { + const adopted = adoptSetupPath(started(), { from: GUESS, to: REAL }); + + const next = discardSetup(adopted, REAL); + + assert.deepEqual(next.sites, []); + assert.deepEqual(next.siteMeta, {}); +}); + +test('discarding before any adoption removes the guess', () => { + const next = discardSetup(started(), GUESS); + + assert.deepEqual(next.sites, []); + assert.deepEqual(next.siteMeta, {}); +}); + +test('discarding leaves every other site untouched', () => { + const other = '/Users/dev/sites/other'; + const state = beginSetup( + { sites: [other], siteMeta: { [other]: { label: 'Other' } } }, + { path: GUESS, label: 'Demo', createdAt: CREATED_AT } + ); + + const next = discardSetup(state, GUESS); + + assert.deepEqual(next.sites, [other]); + assert.deepEqual(Object.keys(next.siteMeta), [other]); +}); + +test('none of the three mutate what they were given', () => { + const state = { sites: [], siteMeta: {} }; + const begun = beginSetup(state, { path: GUESS, label: 'Demo', createdAt: CREATED_AT }); + assert.deepEqual(state, { sites: [], siteMeta: {} }); + + const adopted = adoptSetupPath(begun, { from: GUESS, to: REAL }); + assert.deepEqual(begun.sites, [GUESS]); + assert.equal(begun.siteMeta[GUESS].label, 'Demo'); + + discardSetup(adopted, REAL); + assert.deepEqual(adopted.sites, [REAL]); +}); + +// --- which event moves the row ------------------------------------------- +// +// The decision the subscription used to make inline, and the one that was +// actually wrong: a first version moved the row for any status whose target +// differed from it, which is fine until a second setup exists. + +const CLONING = { phase: 'cloning', target: REAL }; + +test('the cloning status is what moves the row', () => { + assert.equal(rowPathAfterStatus(GUESS, CLONING), REAL); +}); + +test('a row already on the real path does not move again', () => { + assert.equal(rowPathAfterStatus(REAL, CLONING), null); +}); + +// The concurrency failure, stated. With two setups running, the first one's +// `done` names its own directory — which is not where the second one's row +// belongs, and adopting it would drag that row onto a finished site, carrying +// its label and its log with it. +test('no other phase moves the row, however different its target', () => { + for (const phase of ['done', 'installing', undefined]) { + assert.equal(rowPathAfterStatus(GUESS, { phase, target: '/Users/dev/sites/someone-else' }), null, String(phase)); + } +}); + +test('nothing being created means nothing to move', () => { + assert.equal(rowPathAfterStatus(null, CLONING), null); + assert.equal(rowPathAfterStatus('', CLONING), null); +}); + +test('a status with nothing usable in it moves nothing', () => { + for (const status of [null, undefined, {}, { phase: 'cloning' }, { phase: 'cloning', target: '' }]) { + assert.equal(rowPathAfterStatus(GUESS, status), null, JSON.stringify(status)); + } +});