Settle #216 by keeping renderer decisions in modules, and extract the first one

.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.

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;

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 `<input type="file" webkitdirectory>` 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 };

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"e<f>g|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({}, ''), '');
+});