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

[juanmaguitar]

Why

#216 asks for a decision, not a patch: src/renderer/index.jsx is 4163 lines, mounts itself at module scope, and cannot be loaded without a DOM — so every decision made inside it is untestable by construction. Thirteen pure modules have been factored out of it, each with good tests, and nothing proves the component still reaches them. test/ipc-wiring.test.cjs exists because that gap became #180 on the main-process side.

The issue weighs two directions: a DOM harness, or continuing to push decisions into the modules. It asks for that to be settled deliberately rather than one PR at a time. This settles it, and lands the first extraction under the new rule.

What changes

The decision is extraction, recorded as an invariant in §1 of the review standard with a pointer from §5, because that file is the only copy of the standard and the one Copilot reads natively.

The rule states its own limit rather than overselling it. Two things came out of looking at the current state:

• All 23 functions imported from src/renderer/*.cjs have a live call site, and no-unused-vars already fails the build when the last one goes. The "wire cut by deletion" case is largely covered already.
• The uncovered case is "Show in Explorer" and "Open in editor" fail with "unknown error" while a new site is still being set up #180's: the module is still called on one path while another path answers inline. Neither lint nor a shallow wiring test catches that, so the rule is enforced by review and nowhere else. It names the trigger for reopening the harness question.

src/renderer/site-folder.cjs is the first extraction: sanitizeSiteFolder, resolveTargetDir and directoryFromFileEntry, the Create site modal's path arithmetic. Chosen because it was pure, untested, and picked a path separator by inspecting the string — the kind of thing only visible by creating a site by hand on Windows.

Deliberately not in this PR: executeTerminalCommand (index.jsx:1616), the largest decision left in the file and the only one with a security surface — it holds the TERMINAL_ALLOWED_SCRIPTS allow-list and has no tests. It wants the parse-to-a-plan shape dev-server-command.cjs already uses, and its own PR.

How to test this

Platforms: any for the suite; the Windows separator branch is covered from any machine, since nothing reads process.platform.

There is no behaviour change to observe, so the demonstration is the suite plus one path through the UI that must look identical to before.

npm run lint          # clean
npm test              # 621/621
npm run test:electron # 621/621 on Electron's bundled Node
npm run build:once    # the new module has to bundle

Starting state: the app open, on any machine, with no site selected.

1. Click Create site. The modal opens.
2. Type a site name containing characters no folder may have — trac:45678/fix will do.
3. Click Choose files under Site location and pick a parent folder in the native dialog. The folder appears next to the button.
4. Click Create site.

Expected: the site is created inside the folder chosen in step 3, in a directory named trac-45678-fix — the illegal characters became dashes, and nothing else changed about the name.

What must not have happened:

• The site created one level up from the chosen folder, or directly inside it with no directory of its own.
• A directory named wordpress-site when the name given was usable — that fallback is only for a name that sanitises down to nothing.
• On Windows, a path built with a forward slash where the chosen root used backslashes.

Driven on macOS at 5cdf06a. Name trac:45678/fix in ~/CONTRIBUTIONS produced
/Users/…/CONTRIBUTIONS/trac-45678-fix — inside the chosen folder, both illegal characters
replaced, no fallback — and the sidebar kept trac:45678/fix as the label, so siteName and
siteLabel still reach the handler as the two different values they are. The clone completed and
setup moved on to npm install. The Windows separator branch was not driven by hand; it is covered
by test/site-folder.test.cjs from any machine, and a signed Windows artifact is available from
Buildkite for anyone who wants to.

Risks and limitations

Testing this by hand surfaced #228, closed as not planned and not addressed here. directoryFromFileEntry only runs when a folder is dropped on the location control — the click and keyboard handlers both go to the native dialog — and that route is not one the app supports. It is half-present rather than absent: the drop populates the input, and what comes back is '' or the literal C:\fakepath. Supporting drops is a feature, not a gap, and #228 records the decision not to take it up.

The function was extracted exactly as it stood, dead branch included, because changing what it answers was never this PR's to do. Its tests pin where the unsupported route currently ends rather than the shapes the function was written for — see the implementation notes.

The rule this PR adds is enforced by review only. That is stated in the rule itself, and it is the direct consequence of choosing extraction over a harness.

#216 is closed as decided, but not because the gap it names is gone. The proof that index.jsx still calls its modules does not exist and, under this decision, will not — what changes is how little is left in the component for a competing answer to hide in.

Related

Settles #216 (closed with the reasoning). Surfaced #228, closed as not planned. Follow-up to #180, #211, #213.

---

Design decisions and alternatives considered

Why not the DOM harness. It needs App exported separately from the createRoot bootstrap, a stub for the 77 window.api call sites, a jsdom devDependency whose behaviour under @wordpress/components and xterm is unproven, and CI wiring. Against a 4163-line component that buys coverage of the paths the harness happens to drive. The cost is real and the coverage is partial, which is what tipped it.

Why not a static wiring test. The obvious cheap analogue of ipc-wiring.test.cjs — parse index.jsx, assert each module's exports are referenced — was measured and dropped. Every import already has a call site, and no-unused-vars fails the build when the last one goes, so such a test would duplicate the linter while catching nothing the linter misses. It would also not catch #180, which is the failure that matters.

Why this module first. site-folder.cjs is pure, has no IPC, was completely untested, and is cross-platform path handling — a dimension the review standard already treats separately. It makes the rule concrete without the PR turning into a rewrite.

What was left in the component. handleCreateDirInputChange keeps the DOM work — reading files, clearing value, calling the setters — and one conditional state assignment. The rule as written does not carve that out; if it should, that clause is worth adding once there is a second example of it.

Review outcome (required — see AGENTS.md)

/self-review run against the working tree before this branch had any commits, with the judgement pass dispatched to a subagent given only the diff and the instructions file.

1 [fix here] · 2 [follow-up] — the [fix here] is fixed; one follow-up filed and closed as not planned, one recorded.

• 🟡 Tests, [fix here] — fixed. Six assertions in test/site-folder.test.cjs were built on { path: '…' } fixtures. File.path does not exist in this app: Electron removed the augmentation in v32 in favour of webUtils.getPathForFile, this repo pins Electron 43, and src/preload.js bridges no webUtils. The whole if (rawPath) branch is unreachable, so those tests were green while proving nothing — one of the shapes §5 names. Two of the fixtures also omitted the leading segment webkitRelativePath actually carries, which is the only reason they passed. Rewritten to cover what reaches the function today, with the C:\fakepath outcome recorded as where an unsupported route ends and pointed at Dropping a folder on the Create site location button is half-supported #228.
• 🟡 Cross-platform, [follow-up] — filed as Dropping a folder on the Create site location button is half-supported #228, closed as not planned. The consequence on the one live route: click and keyboard are intercepted to the native dialog, but a drop still populates the input, and the fallback yields '' (silently clearing an already-chosen folder) or C:\fakepath (handed to wordpress:setup, where fse.ensureDir would create it). Weighed and declined: the modal's intended route — text field for the name, button for the location — is clear as it stands, and supporting drops is a feature rather than a gap. Note the drop behaviour is read from the code and Chromium's documented handling of webkitdirectory; it has not been reproduced by hand.
• 🔵 Architecture, [follow-up] — recorded, not filed. sanitizeSiteFolder now has a canonical home but its twin is still inline in the wordpress:setup handler in src/main.js — the same three replacements in the same order, differing only in fallback. It is literally the "second code path answering the same question inline" the new §1 text says nothing catches. Harmless today because main returns createdPath and the row adopts it; worth collapsing when either regex next changes. site-folder.cjs is dependency-free CJS and main.js could require it directly.

The extraction itself was checked line by line and holds. The removed finalize helper was a typeof guard over a value that was already a string plus a trailing-separator strip the module now does; setCreateSiteDir(resolved) with if (resolved) setCreateSiteError('') is the old two-branch body exactly. The useCallback dependency array was updated.

Implementation notes

On testing directoryFromFileEntry honestly. Three options were on the table: keep the path fixtures with a note that they cover the legacy Electron <32 shape, delete the dead branch outright, or leave the function untouched and test what actually reaches it. The first is the "green while proving nothing" §5 forbids. The second is a behaviour change inside a PR that advertises none. The third is what shipped: the module's header documents the dead branch and why it cannot run, and the tests assert '' for a real Electron 43 entry and C:\fakepath for the fallback, recorded rather than endorsed and pointing at #228. A file input's value is empty or that literal prefix by specification, on every platform, so there is no realistic input for which the old assertions were reachable. If the decision in #228 is ever revisited, these are the tests it has to rewrite.

On resolveTargetDir at a drive root. C:\ normalises to C:, which has no backslash left to detect, so it takes the forward-slash branch and produces C:/my-site. Still absolute on Windows, so not a bug — but it was invisible before and now has a test saying what it does and why that is acceptable, rather than leaving the next reader to re-derive it.

Files. src/renderer/site-folder.cjs (new, 3 exports + the fallback constant), test/site-folder.test.cjs (new, 11 tests), src/renderer/index.jsx (44 lines out, 11 in), .github/instructions/code-review.instructions.md (the invariant in §1, one pointer in §5).

Screenshots or recording

Nothing on screen changed. The Create site modal renders and behaves exactly as before; the extraction is invisible to the contributor, which is the point of the manual steps above.

[Copilot]

Pull request overview

Extracts Create site path decisions into a testable renderer module and establishes this pattern as a review invariant.

Changes:

• Adds tested site-folder sanitization and path-resolution helpers.
• Replaces inline renderer logic with module calls.
• Documents the module-extraction review standard.

No findings across the five review dimensions.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
src/renderer/site-folder.cjs	Adds pure site-folder and path helpers.
src/renderer/index.jsx	Uses the extracted helpers.
test/site-folder.test.cjs	Covers sanitization and cross-platform paths.
.github/instructions/code-review.instructions.md	Establishes renderer-module review guidance.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.