From ea6be7814881d9d28432eee74df41d631b2df117 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Mon, 10 Aug 2026 11:06:40 +0200 Subject: [PATCH 1/3] Answer #216 by moving renderer decisions into modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #216 asks for a deliberate choice between building a DOM harness for index.jsx and continuing to push decisions out into pure modules. This is the second, made explicit rather than left to drift one PR at a time. The rule goes in the review standard, where every agent and Copilot already read it, and says out loud what it does not buy: no-unused-vars catches a module whose last call site is deleted, but nothing catches a second code path answering the same question inline, which is what #180 was. site-folder.cjs is the first extraction under it — the Create site modal's path arithmetic, which was pure, untested, and chose a path separator by looking at the string. Both platforms are now exercised from one machine. directoryFromFileEntry came out as it stood, dead branch and all, and its tests pin what it actually answers today rather than the shapes it was written for. That turned out to be #228. Co-Authored-By: Claude Opus 5 (1M context) --- .../instructions/code-review.instructions.md | 18 ++++ src/renderer/index.jsx | 51 ++------- src/renderer/site-folder.cjs | 100 ++++++++++++++++++ test/site-folder.test.cjs | 96 +++++++++++++++++ 4 files changed, 221 insertions(+), 44 deletions(-) create mode 100644 src/renderer/site-folder.cjs create mode 100644 test/site-folder.test.cjs diff --git a/.github/instructions/code-review.instructions.md b/.github/instructions/code-review.instructions.md index c1ba626..7cdb0e4 100644 --- a/.github/instructions/code-review.instructions.md +++ b/.github/instructions/code-review.instructions.md @@ -125,6 +125,20 @@ spawn failure, and `runNpmWithEngineRetry` in `src/main.js` shows the expected s chair. And a setup that dies halfway must leave the site registry consistent — no phantom site in `electron-store` for a directory that was never finished. +**Renderer decisions live in modules, not in `index.jsx`.** `src/renderer/index.jsx` mounts itself +at module scope and cannot be loaded without a DOM, so nothing in the suite can reach it: a +decision made there is untestable by construction. Anything with more than one branch — a string +the user reads, a path joined, a status derived, a command parsed — belongs in a +`src/renderer/*.cjs` module with its own test, leaving the component holding JSX, state +assignments and the call. `site-folder.cjs` and `open-failure.cjs` are the shape. + +This is the direction chosen in #216 over building a DOM harness, which was judged too much setup +for the coverage it buys against a 4000-line component. The consequence is that it is enforced +here, by review, and nowhere else — `no-unused-vars` catches a module whose last call site is +deleted, but nothing catches a second code path that answers the same question inline. That is +exactly what #180 was. Reopen the harness question if a bug ever lands in the assignments the +modules cannot absorb. + **New dependencies are findings by default.** Native compilation or a host binary breaks the zero-prerequisite promise on user machines. A dependency with lifecycle scripts also needs an `allowScripts` entry in `package.json` — the mechanism already exists, and a missing entry means @@ -244,6 +258,10 @@ paths from macOS by injecting `platform`, lookup and env rather than reading `pr A new platform split tested with `it.skip` on the other OS is a coverage hole CI will never close, since the suite runs on both platforms but each skips the other's branch. +**Renderer logic is tested through its module, so check it has one.** A PR that puts a branch +inside `src/renderer/index.jsx` has written code the suite cannot reach — see the invariant in §1. +The finding is the missing module, not the missing test. + **Scope stays proportional.** Missing tests on a touched line of legacy code is `[follow-up]`, not `[fix here]` — the strong rule applies to what the PR introduces, not to everything it brushes against. diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index 5c3bfec..81c2453 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -25,6 +25,7 @@ import { shouldShowTerminalHints, computeTerminalBusy } from './terminal-hints.c import { planDevServerStart, formatElapsed } from './dev-server-command.cjs'; import { appendBounded, countLines } from './debug-log.cjs'; import { pathBasename } from './path-basename.cjs'; +import { sanitizeSiteFolder, resolveTargetDir, directoryFromFileEntry } from './site-folder.cjs'; import { noticeForOpenResult } from './open-failure.cjs'; import { trunkAgeInfo, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, APPLY_STATE_TO_STEP } from './update-plan.cjs'; import { pickLatest } from '../latest-patch.cjs'; @@ -395,21 +396,6 @@ function App() { setCreateModalOpen(true); }, [createSubmitting]); - const sanitizeSiteFolder = useCallback((value) => ( - value - .replace(/[\\/:*?"<>|]+/g, '-') - .replace(/\s+/g, '-') - .replace(/^-+|-+$/g, '') - || 'wordpress-site' - ), []); - - const resolveTargetDir = useCallback((root, folder) => { - if (!root) return folder; - const normalizedRoot = root.replace(/[\\/]+$/, ''); - const separator = /\\/.test(normalizedRoot) && !normalizedRoot.includes('/') ? '\\' : '/'; - return `${normalizedRoot}${separator}${folder}`; - }, []); - const openDirectoryPicker = useCallback(async () => { try { const dir = await window.api.chooseDirectory(); @@ -423,40 +409,17 @@ function App() { const handleCreateDirInputChange = useCallback((event) => { const inputEl = event.target; createDirInputRef.current = inputEl; - const finalize = (rawDir) => { - const normalized = typeof rawDir === 'string' ? rawDir.replace(/[\\/]+$/, '') : ''; - if (normalized) { - setCreateSiteDir(normalized); - setCreateSiteError(''); - } else { - setCreateSiteDir(''); - } - }; const files = inputEl.files; if (!files || files.length === 0) { inputEl.value = ''; return; } - const first = files[0]; - const relative = first?.webkitRelativePath || ''; - const rawPath = first?.path || ''; - let resolved = ''; - - if (rawPath) { - if (relative) { - resolved = rawPath.slice(0, rawPath.length - relative.length); - } else { - resolved = rawPath.replace(/[\\/][^\\/]*$/, ''); - } - } - - if (!resolved && inputEl.value) { - resolved = inputEl.value.replace(/[^\\/]*$/, ''); - } - - resolved = resolved.replace(/[\\/]+$/, ''); - finalize(resolved); + const resolved = directoryFromFileEntry(files[0], inputEl.value); + setCreateSiteDir(resolved); + // Clearing the error only when there is a directory: a selection that + // resolved to nothing has not fixed anything the message was about. + if (resolved) setCreateSiteError(''); inputEl.value = ''; }, [setCreateSiteDir, setCreateSiteError]); @@ -527,7 +490,7 @@ function App() { clearPendingSites(); setCreateSubmitting(false); } - }, [addPendingSite, appendSetupLog, applySetup, clearPendingSites, createSiteDir, createSiteName, moveSetupLog, refresh, resolveTargetDir, sanitizeSiteFolder]); + }, [addPendingSite, appendSetupLog, applySetup, clearPendingSites, createSiteDir, createSiteName, moveSetupLog, refresh]); const closeCreateModal = useCallback(() => { if (createSubmitting) return; diff --git a/src/renderer/site-folder.cjs b/src/renderer/site-folder.cjs new file mode 100644 index 0000000..59b0068 --- /dev/null +++ b/src/renderer/site-folder.cjs @@ -0,0 +1,100 @@ +// Where a new site goes, and what its folder is called. +// +// Three decisions the Create site modal makes before `setupWordPress` is ever +// called, all of them string work on paths the renderer cannot hand to Node's +// `path` module: it has none. The chosen root arrives in the platform's native +// form — `C:\Users\me` on Windows, `/Users/me` elsewhere — so joining a folder +// name onto it means picking the separator by looking at the string. +// +// They lived inside the component until #216, where nothing in the suite could +// reach them: `index.jsx` cannot be loaded without a DOM, so a wrong separator, +// or a name sanitised down to nothing, was visible only by creating a site by +// hand on the platform in question. +'use strict'; + +// Everything a folder name may not contain on Windows, which is the stricter of +// the two platforms. One rule everywhere keeps a name from working on macOS and +// failing on Windows. +const ILLEGAL_FOLDER_CHARS = /[\\/:*?"<>|]+/g; + +// What a name that sanitises down to nothing becomes. Any folder is better than +// the alternative, which is creating the site directly in the chosen root. +const FALLBACK_FOLDER = 'wordpress-site'; + +/** + * A site name as typed, turned into a folder name that is legal everywhere. + * + * @param {*} value + * @return {string} + */ +function sanitizeSiteFolder(value) { + return String(value ?? '') + .replace(ILLEGAL_FOLDER_CHARS, '-') + .replace(/\s+/g, '-') + .replace(/^-+|-+$/g, '') || FALLBACK_FOLDER; +} + +/** + * The chosen root joined to the folder name, using the separator the root + * already uses. + * + * A root written entirely in backslashes is Windows and gets a backslash. + * Everything else — including the mixed separators Windows itself accepts — + * gets a forward slash, which Windows also accepts. + * + * @param {*} root + * @param {*} folder + * @return {string} + */ +function resolveTargetDir(root, folder) { + if (!root) return String(folder ?? ''); + const normalizedRoot = String(root).replace(/[\\/]+$/, ''); + const separator = /\\/.test(normalizedRoot) && !normalizedRoot.includes('/') ? '\\' : '/'; + return `${normalizedRoot}${separator}${folder}`; +} + +/** + * The directory an `` selection points at. + * + * Extracted as it stood, dead branch included, because #216 is a refactor and + * this is the wrong PR to change what it answers. What it answers today is + * wrong, and #228 is where that gets decided: + * + * - `path` plus `webkitRelativePath`, and `path` alone, are the two shapes this + * was written for. Electron removed the `path` augmentation on `File` in v32 + * in favour of `webUtils.getPathForFile`; this app pins Electron 43 and + * bridges no `webUtils`, so neither branch is reachable. + * - What is left is the input's own `value`, which is a fiction: a file input's + * value is empty or the literal `C:\fakepath\` prefix on every platform. So a + * dropped folder resolves to '' or to `C:\fakepath`, and the modal presents + * the second as a real destination. + * + * The click and keyboard handlers on that input are intercepted and go to the + * native dialog, so nothing here runs unless a folder is dropped onto it. + * + * @param {*} file The first entry of the input's `files` list. + * @param {*} inputValue The input's `value`, read only when `file` has no path. + * @return {string} The directory without a trailing separator, or '' when none + * could be derived. + */ +function directoryFromFileEntry(file, inputValue) { + const relative = file?.webkitRelativePath || ''; + const rawPath = file?.path || ''; + let resolved = ''; + + if (rawPath) { + if (relative) { + resolved = rawPath.slice(0, rawPath.length - relative.length); + } else { + resolved = rawPath.replace(/[\\/][^\\/]*$/, ''); + } + } + + if (!resolved && inputValue) { + resolved = String(inputValue).replace(/[^\\/]*$/, ''); + } + + return resolved.replace(/[\\/]+$/, ''); +} + +module.exports = { sanitizeSiteFolder, resolveTargetDir, directoryFromFileEntry, FALLBACK_FOLDER }; diff --git a/test/site-folder.test.cjs b/test/site-folder.test.cjs new file mode 100644 index 0000000..e25e044 --- /dev/null +++ b/test/site-folder.test.cjs @@ -0,0 +1,96 @@ +// The Create site modal's path arithmetic, which decides where a site is +// cloned before anything is cloned. Both platforms are exercised from one +// machine: nothing here reads `process.platform`, the separator is chosen from +// the shape of the root string, so a macOS run covers the Windows branch too. +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { + sanitizeSiteFolder, + resolveTargetDir, + directoryFromFileEntry, + FALLBACK_FOLDER +} = require('../src/renderer/site-folder.cjs'); + +test('sanitizeSiteFolder replaces characters Windows refuses in a folder name', () => { + assert.equal(sanitizeSiteFolder('feature/45678'), 'feature-45678'); + assert.equal(sanitizeSiteFolder('trac:45678'), 'trac-45678'); + assert.equal(sanitizeSiteFolder('a\\b*c?d"eg|h'), 'a-b-c-d-e-f-g-h'); +}); + +test('sanitizeSiteFolder collapses whitespace and trims the dashes it created', () => { + assert.equal(sanitizeSiteFolder('My Site'), 'My-Site'); + assert.equal(sanitizeSiteFolder(' My Site '), 'My-Site'); + assert.equal(sanitizeSiteFolder('-already-dashed-'), 'already-dashed'); +}); + +test('sanitizeSiteFolder falls back rather than returning an empty folder name', () => { + // An empty result would join to the root itself, cloning WordPress straight + // into the directory the contributor picked. + assert.equal(sanitizeSiteFolder('///'), FALLBACK_FOLDER); + assert.equal(sanitizeSiteFolder(' '), FALLBACK_FOLDER); + assert.equal(sanitizeSiteFolder(''), FALLBACK_FOLDER); + assert.equal(sanitizeSiteFolder(null), FALLBACK_FOLDER); + assert.equal(sanitizeSiteFolder(undefined), FALLBACK_FOLDER); +}); + +test('resolveTargetDir keeps a Windows root on backslashes', () => { + assert.equal(resolveTargetDir('C:\\Users\\me\\sites', 'my-site'), 'C:\\Users\\me\\sites\\my-site'); + assert.equal(resolveTargetDir('C:\\Users\\me\\sites\\', 'my-site'), 'C:\\Users\\me\\sites\\my-site'); +}); + +test('resolveTargetDir keeps a POSIX root on forward slashes', () => { + assert.equal(resolveTargetDir('/Users/me/sites', 'my-site'), '/Users/me/sites/my-site'); + assert.equal(resolveTargetDir('/Users/me/sites///', 'my-site'), '/Users/me/sites/my-site'); +}); + +test('resolveTargetDir uses a forward slash for a mixed root', () => { + // Windows accepts both, so the only thing this must not do is guess wrong + // about a path that already contains a forward slash and produce neither. + assert.equal(resolveTargetDir('C:/Users/me\\sites', 'my-site'), 'C:/Users/me\\sites/my-site'); +}); + +test('resolveTargetDir at a Windows drive root produces an absolute path', () => { + // Stripping the trailing separator off `C:\` leaves `C:`, which has no + // backslash left to detect — so this takes the forward-slash branch. The + // result is still absolute on Windows, which is what matters; `C:my-site` + // would have been drive-relative and landed somewhere else entirely. + assert.equal(resolveTargetDir('C:\\', 'my-site'), 'C:/my-site'); +}); + +test('resolveTargetDir with no root is the folder name alone', () => { + // The modal blocks submitting without a directory, so this is a guard, not + // a path a contributor reaches. + assert.equal(resolveTargetDir('', 'my-site'), 'my-site'); + assert.equal(resolveTargetDir(null, 'my-site'), 'my-site'); +}); + +// What follows pins what this function does with the entries it actually gets, +// which is not the same as what it was written for. See #228: the `path` +// property it prefers was removed from `File` in Electron 32, this app pins +// Electron 43, and no `webUtils` bridge replaces it — so every real entry takes +// the fallback. Asserting the `path` shapes would be green and prove nothing. + +test('directoryFromFileEntry gets nothing from a real dropped entry', () => { + // A File in Electron 43. No `path`, and `webkitRelativePath` alone carries + // no absolute part to cut it off. + assert.equal(directoryFromFileEntry({ webkitRelativePath: 'sites/inner/file.txt' }, ''), ''); + assert.equal(directoryFromFileEntry({}, ''), ''); + assert.equal(directoryFromFileEntry(null, ''), ''); + assert.equal(directoryFromFileEntry(undefined, undefined), ''); +}); + +test('directoryFromFileEntry passes C:\\fakepath through — #228', () => { + // The bug, pinned rather than endorsed. A file input's `value` is either + // empty or this literal prefix on every platform, browsers substituting it + // for the real path, so the fallback's "typed path" is a fiction. The + // modal shows this as the chosen folder and submit hands it to setup. + assert.equal(directoryFromFileEntry({}, 'C:\\fakepath\\my-folder'), 'C:\\fakepath'); +}); + +test('directoryFromFileEntry returns nothing rather than a wrong directory', () => { + // '' is what the caller checks before it clears the chosen directory — a + // bare segment is not a directory anyone chose. + assert.equal(directoryFromFileEntry({}, 'file.txt'), ''); + assert.equal(directoryFromFileEntry({}, ''), ''); +}); From a5478414ba544f1cdcec28e2d076e4c4d7a17a88 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Mon, 10 Aug 2026 11:15:09 +0200 Subject: [PATCH 2/3] Say what #228 is: an unsupported route, not a bug in this path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comments called the C:\fakepath outcome "the bug" and said what the function answers today is wrong. Dropping a folder on the location control is a route the app does not support — the click and keyboard handlers both go to the native dialog — so what these record is where an unsupported route currently ends, and the failing tests to write against if #228 is ever taken up. Co-Authored-By: Claude Opus 5 (1M context) --- src/renderer/site-folder.cjs | 12 ++++++------ test/site-folder.test.cjs | 21 +++++++++++++-------- 2 files changed, 19 insertions(+), 14 deletions(-) diff --git a/src/renderer/site-folder.cjs b/src/renderer/site-folder.cjs index 59b0068..ba1eb15 100644 --- a/src/renderer/site-folder.cjs +++ b/src/renderer/site-folder.cjs @@ -56,9 +56,12 @@ function resolveTargetDir(root, folder) { /** * The directory an `` selection points at. * - * Extracted as it stood, dead branch included, because #216 is a refactor and - * this is the wrong PR to change what it answers. What it answers today is - * wrong, and #228 is where that gets decided: + * Nothing reaches this by the intended route: the input's click and keyboard + * handlers are intercepted and go to the native dialog. It runs only when a + * folder is dropped onto the control, which is a second route the app does not + * currently support — see #228, where supporting it or closing it off gets + * decided. Extracted as it stood, dead branch included, because #216 is a + * refactor and not the place to change what it answers: * * - `path` plus `webkitRelativePath`, and `path` alone, are the two shapes this * was written for. Electron removed the `path` augmentation on `File` in v32 @@ -69,9 +72,6 @@ function resolveTargetDir(root, folder) { * dropped folder resolves to '' or to `C:\fakepath`, and the modal presents * the second as a real destination. * - * The click and keyboard handlers on that input are intercepted and go to the - * native dialog, so nothing here runs unless a folder is dropped onto it. - * * @param {*} file The first entry of the input's `files` list. * @param {*} inputValue The input's `value`, read only when `file` has no path. * @return {string} The directory without a trailing separator, or '' when none diff --git a/test/site-folder.test.cjs b/test/site-folder.test.cjs index e25e044..42cb36e 100644 --- a/test/site-folder.test.cjs +++ b/test/site-folder.test.cjs @@ -66,10 +66,15 @@ test('resolveTargetDir with no root is the folder name alone', () => { }); // What follows pins what this function does with the entries it actually gets, -// which is not the same as what it was written for. See #228: the `path` -// property it prefers was removed from `File` in Electron 32, this app pins -// Electron 43, and no `webUtils` bridge replaces it — so every real entry takes -// the fallback. Asserting the `path` shapes would be green and prove nothing. +// which is not the same as what it was written for. The `path` property it +// prefers was removed from `File` in Electron 32, this app pins Electron 43, +// and no `webUtils` bridge replaces it — so every real entry takes the +// fallback. Asserting the `path` shapes would be green and prove nothing. +// +// The only route that reaches this at all is dropping a folder on the control, +// which the app does not support; #228 decides whether it should. So these +// record where that route currently ends, and are the failing tests to write +// against when it is taken up. test('directoryFromFileEntry gets nothing from a real dropped entry', () => { // A File in Electron 43. No `path`, and `webkitRelativePath` alone carries @@ -81,10 +86,10 @@ test('directoryFromFileEntry gets nothing from a real dropped entry', () => { }); test('directoryFromFileEntry passes C:\\fakepath through — #228', () => { - // The bug, pinned rather than endorsed. A file input's `value` is either - // empty or this literal prefix on every platform, browsers substituting it - // for the real path, so the fallback's "typed path" is a fiction. The - // modal shows this as the chosen folder and submit hands it to setup. + // Recorded, not endorsed. A file input's `value` is either empty or this + // literal prefix on every platform, browsers substituting it for the real + // path, so the fallback's "typed path" is a fiction. The modal shows this + // as the chosen folder and submit hands it to setup. assert.equal(directoryFromFileEntry({}, 'C:\\fakepath\\my-folder'), 'C:\\fakepath'); }); From 5cdf06a62c9af1a6c40085302545ebc03e552d10 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Mon, 10 Aug 2026 11:16:25 +0200 Subject: [PATCH 3/3] Record #228 as decided, not pending Closed as not planned: the modal's route is clear as it stands and drop support is a feature rather than a gap. The comments said the question was still open, which would have read as a loose end to whoever got here next. Co-Authored-By: Claude Opus 5 (1M context) --- src/renderer/site-folder.cjs | 8 ++++---- test/site-folder.test.cjs | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/renderer/site-folder.cjs b/src/renderer/site-folder.cjs index ba1eb15..4fceb0d 100644 --- a/src/renderer/site-folder.cjs +++ b/src/renderer/site-folder.cjs @@ -58,10 +58,10 @@ function resolveTargetDir(root, folder) { * * Nothing reaches this by the intended route: the input's click and keyboard * handlers are intercepted and go to the native dialog. It runs only when a - * folder is dropped onto the control, which is a second route the app does not - * currently support — see #228, where supporting it or closing it off gets - * decided. Extracted as it stood, dead branch included, because #216 is a - * refactor and not the place to change what it answers: + * folder is dropped onto the control — a route the app deliberately does not + * support, decided in #228 and closed there. Extracted as it stood, dead branch + * included, because #216 is a refactor and not the place to change what it + * answers: * * - `path` plus `webkitRelativePath`, and `path` alone, are the two shapes this * was written for. Electron removed the `path` augmentation on `File` in v32 diff --git a/test/site-folder.test.cjs b/test/site-folder.test.cjs index 42cb36e..4f2b0df 100644 --- a/test/site-folder.test.cjs +++ b/test/site-folder.test.cjs @@ -72,9 +72,9 @@ test('resolveTargetDir with no root is the folder name alone', () => { // fallback. Asserting the `path` shapes would be green and prove nothing. // // The only route that reaches this at all is dropping a folder on the control, -// which the app does not support; #228 decides whether it should. So these -// record where that route currently ends, and are the failing tests to write -// against when it is taken up. +// which the app deliberately does not support — #228, closed as not planned. +// So these record where an unsupported route ends, and are the tests a change +// of mind would have to rewrite. test('directoryFromFileEntry gets nothing from a real dropped entry', () => { // A File in Electron 43. No `path`, and `webkitRelativePath` alone carries