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..4fceb0d --- /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. + * + * 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 — 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 + * 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. + * + * @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..4f2b0df --- /dev/null +++ b/test/site-folder.test.cjs @@ -0,0 +1,101 @@ +// 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. 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 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 + // 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', () => { + // 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'); +}); + +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({}, ''), ''); +});