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)); + } +});