Open a site's code without leaving the app by juanmaguitar · Pull Request #158 · WordPress/contributor-toolkit

juanmaguitar · 2026-08-07T05:44:53Z

Closes #150.

Each site gains Open in editor and Show in Finder / Show in Explorer / Show in file manager, next to the path and its copy button — which stays exactly as it was, since it is the floor under everything else.

First use opens a picker of the editors found on the machine, with Choose application… always beside them rather than only as a fallback. After that the button names the editor ("Open in Sublime Text") and launches straight through, with Change editor next to it. A launch that fails — nothing chosen yet, or an editor that has since moved — reopens the picker and says why. Nothing here can end at a disabled button or a bare "unavailable".

The constraint this is built around

The earlier editor dropdown (#24) was removed in #26 because its detection ran which/where, and a packaged Electron app does not inherit the shell's PATH — a correctly installed VS Code read as missing in the shipped build while working fine in npm start. So src/editor-launch.js never consults PATH, at either end:

Detection stats absolute, per-platform install locations (/Applications/*.app, %LOCALAPPDATA%\Programs\…, JetBrains Toolbox, the common Linux packages). No which, no where, no spawning anything to find out what is installed.
Launching refuses a relative command, because spawn('code', …) without a shell would resolve it through that same missing environment.

Both are pinned by tests, including one that runs detection with an empty PATH and gets the same answer.

The table is a convenience, not the contract: editor:choose opens a file dialog for anything it misses and validates the result to the same standard, so an editor this app has never heard of works exactly as well.

Guards

editor:open and dir:show both refuse a folder the app has no record of — the same isRegisteredSite boundary sites:delete uses — so "open this site" cannot become "open this arbitrary directory". dir:show goes through a new revealRegisteredSite in site-registry.js rather than calling shell.openPath itself. Spawns are shell: false, detached, stdio: 'ignore', windowsHide: true. Refusals are logged, never dropped. Nothing added here goes near shell.openExternal.

Two moves that are not the feature

src/safe-log.js — external-url.js and site-registry.js each carried a copy of the escape-and-truncate step for logging a refused value, and site-registry's copy said in a comment that the third caller should move it somewhere shared. This is that third caller. Behaviour identical; both existing suites pass untouched, which is the proof.
src/settings-store.js — this branch originally extracted getStore() out of main.js for the same reason Prove the delete handler still goes through the registry gate #154 did, and the two collided on rebase. Trunk's version won: the file here is trunk's, plus preferences in its defaults. The wiring tests likewise use trunk's fakeSettingsStore rather than the near-identical helper this branch had added. The one thing that survived from this side is the consequence — the four new channels read the store before reaching their guard module, and behind that seam they are wiring tests rather than entries on the known-holes list.

Adding preferences to the store defaults is additive: sites and siteMeta keep their keys and shapes, so existing registries need no migration.

Review

Ran per AGENTS.md, with the judgement pass in a fresh context. npm run lint clean, 213 tests pass.

4 findings · 4 fixed · 0 deferred

🟡 Architecture — a launch was reported successful the moment spawn returned. spawn returns a handle before the OS has been asked to execute anything, so the failure that actually happens (EACCES on a non-executable file, a Windows policy's EPERM, a path deleted since the check) arrived afterwards on the 'error' event with nothing listening: "opening your editor", then a button that did nothing, with the uncaught emit going only to the log file. The answer now comes from the child — 'error' either way, plus open's exit code on macOS where the child is /usr/bin/open and exits in milliseconds; 'spawn' elsewhere, since waiting for exit would mean waiting for the contributor to close their editor.
🟡 Tests — the test for that path injected a spawn that throws, the one failure real spawn does not produce for an unexecutable target. The fake now returns a handle and emits on a later turn; the two new cases fail on the previous code.
🟡 Performance — detection ran per site row, and every site is mounted at once, so with N sites that was N × ~14 synchronous stats on the main process at load. The choice is now held once for the window; load asks editor:get (a store read, no filesystem) and detection waits for the picker.
🔵 Architecture — the same per-row state meant choosing an editor in one site left every other row's button reading "Open in editor". Fixed by the same hoist. chosenMissing went with it: nothing consumed it, and an editor that has moved is reported by trying to open it.

Clean on security and cross-platform. The Windows and Linux branches are exercised from macOS by injecting platform, env and the filesystem, per the house pattern in test/win-spawn-patch.test.cjs.

Testing

Driven on macOS against a real site: detection found the three editors installed, the picker remembered the choice, the second click went straight through without the picker, the other site's row agreed on the label, and both refusals (a folder the registry does not hold, a file that is not an application) came back as refusals.

Not verified locally: the packaged build — which is the only place the #24 failure ever showed up. The Buildkite artifact for this branch is the check that matters, on Windows and macOS both.

🤖 Generated with Claude Code

Copilot

Pull request overview

Adds in-app editor launching and file-manager access for registered WordPress sites.

Changes:

Detects, validates, remembers, and launches local editors.
Adds editor-picker and file-manager controls to site rows.
Introduces shared persistence/logging helpers and IPC coverage.

Review findings: 7 [fix here] · 0 [follow-up]

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
`src/editor-launch.js`	Implements editor detection, validation, and launching.
`src/external-url.js`	Uses shared safe-log formatting.
`src/main.js`	Adds editor and directory IPC handlers.
`src/preload.js`	Exposes the new IPC APIs.
`src/renderer/index.jsx`	Adds editor controls, picker, and notices.
`src/safe-log.js`	Centralizes safe log-value formatting.
`src/settings-store.js`	Extracts the electron-store initialization seam.
`src/site-registry.js`	Adds guarded site-directory revealing.
`test/editor-launch.test.cjs`	Tests detection, validation, and process launching.
`test/ipc-wiring.test.cjs`	Covers the new IPC wiring and store seam.
`test/site-registry.test.cjs`	Tests guarded directory revealing.

Suppressed comments (1)

src/main.js:683

🟡 Performance [fix here] — This synchronous stat is reached by editor:list, editor:choose, and editor:open; listing alone performs roughly a dozen of them on Electron’s main/UI process. Slow filesystem or antivirus access will freeze the whole window. Make filesystem probing asynchronous (for example with fs.promises.stat) and await async detection/validation helpers.

});

+  const openInEditor = useCallback(async () => {
+    const result = await window.api.openInEditor(sitePath);
+    if (result?.ok) {
+      setEditorNotice('');
+      return;
+    }
+    // Nothing chosen yet is not an error, it is the first use. Anything else is
+    // worth saying out loud — but both end at the same place: the picker.
+    setEditorNotice(result?.reason === 'no-editor' ? '' : describeOpenFailure(result));
+    await loadDetected();
+    setEditorPickerOpen(true);
+  }, [describeOpenFailure, loadDetected, sitePath]);


+    window.api.getEditor()
+      .then((editor) => { if (!cancelled) setChosen(editor || null); })
+      .catch(() => {});


+	const editor = {
+		path: target,
+		name: path.basename(target, path.extname(target))
+	};


+	if (platform === 'win32') {
+		return stats.isFile === true && editorPath.toLowerCase().endsWith('.exe');
+	}
+	return stats.isFile === true;


+            <p style={{ margin: 0, fontSize: 13, lineHeight: 1.5 }}>
+              {detectedEditors.length
+                ? 'Choose the editor to open this site in. This app will remember it.'
+                : 'This app could not find an editor in the usual place. Point at yours and it will remember it.'}
+            </p>


+    setEditorPickerOpen(false);
+    const opened = await window.api.openInEditor(sitePath);
+    setEditorNotice(opened?.ok ? '' : describeOpenFailure(opened));
+  }, [describeOpenFailure, remember, sitePath]);


Copilot's review of #158, all seven findings. Detection and validation stat'd synchronously on the process that draws the window — a dozen probes of locations that mostly do not exist, which is free on the author's machine and a frozen window behind a Windows antivirus filter driver. The injected probes are awaited now, and the candidates are probed together rather than one after another. On Linux the picker cannot filter by extension, and "a regular file" was enough to be remembered as the contributor's editor: a document was accepted and then failed with EACCES at the spawn. Executability is now part of the answer, asked of the OS with access(X_OK) so it is about the user this app runs as rather than about mode bits. The stored name was the basename, which reads well on macOS by accident and not at all on Windows: "Code" for Visual Studio Code, "phpstorm64" for PhpStorm. A known application is now named the way the picker named it, and only an unknown one the contributor pointed at falls back to its filename. Three renderer paths could end in a button that appeared to do nothing. A rejected invoke — a handler that throws, a window being torn down — skipped both the notice and the picker; a failed load of the remembered choice was swallowed entirely, making "the store could not be read" indistinguishable from "nothing chosen yet"; and choosing an editor that then failed to launch closed the picker anyway, leaving a notice and no way forward, which is the opposite of what this feature promises. The picker now closes only on a launch that worked, and the reason it opened is rendered inside it, where the focus is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

juanmaguitar · 2026-08-07T07:25:14Z

Copilot's review: 7 findings · 7 fixed in 7cfa97f. All of them were real; two would have shipped a broken flow on a platform I did not test on.

🟡 Performance — synchronous stats on the main process. Correct, and it is an explicit invariant in this repo's review standard. detectEditors and isLaunchableEditorPath now take awaited probes and main.js supplies fs.promises; the candidates are probed together rather than one after another, so the picker opens after one round of I/O instead of a dozen.

🟡 Cross-platform — Linux accepted any regular file as an application. The right catch. The Linux dialog cannot filter by extension, so a document passed every check and was remembered as the editor, failing with EACCES only at the spawn. isLaunchableEditorPath now requires executability, asked of the OS with access(X_OK) rather than read off mode bits, so it reflects the user this app runs as. Both branches are tested.

🔵 Architecture — the friendly name was discarded. Right, and worse on Windows than the comment suggests: the button would have read "Open in Code" and "Open in phpstorm64". A new knownEditorName looks the path up in the candidate table (case-insensitively on Windows and macOS, since their filesystems are), and the basename is now only the fallback for an application the table has never heard of.

🟡 Architecture — a rejected editor:open invoke skipped both the notice and the picker. Yes. All three renderer entry points (openInEditor, chooseEditor, showSiteInFileManager) now treat a rejection as a structured failure, so a transport error draws a notice like any other.

🟡 Architecture — the failed load of the remembered choice was swallowed. Fixed, and worth stating why the suggestion is right: src/logging.js initializes electron-log with spyRendererConsole: true, so a renderer console.error does reach the log file. It needed a justified no-console disable, since the repo lints that rule on the renderer.

🟡 Architecture — choosing an editor that then failed to launch closed the picker. This one contradicted the feature's own stated promise, which makes it the most valuable of the seven. The picker now closes only on a launch that actually worked.

🟡 Architecture — the notice was outside the focused modal. Fixed: the reason the picker reopened is rendered inside it, as a role="alert", alongside the copy that was previously the only thing a keyboard or screen-reader user would meet.

Verified after the changes: lint clean, 255 tests pass, and the flow re-driven on macOS — the picker names the editor, closes only on a successful launch, and both refusals (a file that is not an application, a folder the registry does not hold) come back as refusals.

external-url.js and site-registry.js each carried their own copy of the escape-and-truncate step that renders a refused value into the log file, and site-registry.js's copy said in a comment that the third caller should be the one to move it somewhere shared rather than add a third. The editor-launch module for #150 is that third caller, so this moves it first: safe-log.js holds `describeRefused` and the reasoning, and the two existing formatters become one-line wrappers over it so their names — and their tests, unchanged — still say which guard is refusing. Behaviour is identical; test/external-url.test.cjs and test/site-registry.test.cjs pass untouched, which is the point. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The main-process half of #150: detect installed editors, remember one, open a site's folder in it, and reveal that folder in the file manager. Detection is the part that has to be right this time. The first attempt (#24, removed in #26) ran `which`/`where`, and a packaged Electron app does not inherit the shell's PATH — so an installed VS Code read as missing in the shipped build and only there. src/editor-launch.js therefore probes absolute, per-platform install locations with a filesystem check, and refuses to launch a relative command, since spawning one would resolve it through the same PATH that is not there. Both ends are asserted in test/editor-launch.test.cjs. The table is a convenience, not the contract: `editor:choose` opens a file dialog for anything it misses, and validates what comes back the same way, so an editor this app does not know about is never a dead end. Both new effects are behind the registry boundary `sites:delete` already uses: `editor:open` will not open a folder the app has no record of, and `dir:show` goes through a new `revealRegisteredSite` in site-registry.js rather than calling shell.openPath itself. `getStore()` moves to src/settings-store.js unchanged. Its dynamic `import('electron-store')` is an ESM import, which Module._load cannot stand in for, so every handler that read the store before reaching its guard module was unreachable from test/ipc-wiring.test.cjs and recorded there as a known hole. Behind a require-able seam the four new channels are wired tests rather than a fifth hole — and site:status, which was that hole, is now wired too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The window half of #150. The site's path row keeps its copy button — it is the floor under everything else — and gains two actions above the fold: open the folder in the contributor's editor, and reveal it in the file manager, which needs no configuration on any machine. First use opens a picker listing what was found, with "Choose application…" always beside it rather than only as a fallback. After that the button names the editor ("Open in Sublime Text") and launches straight through, with "Change editor" next to it. Nothing here can end at a dead button. A launch that fails — no editor chosen yet, or one that has since been uninstalled — reopens the picker and says why, and every failure notice carries "Choose application…" as its action. Driven on macOS against a real site: detection found the three editors installed, the picker remembered the choice, the second click went straight through, and both refusals (a folder the registry does not hold, a file that is not an application) came back as refusals. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Findings from the pre-PR review pass. A launch was reported as successful the moment `spawn` returned, but `spawn` returns a handle before the OS has been asked to execute anything. The failure that actually happens — EACCES on a file that is not executable, a Windows policy's EPERM, a path deleted since the check — arrives afterwards on the 'error' event, which nothing was listening for: the contributor got "opening your editor" and a button that did nothing, while the uncaught emit went to the log file. The answer now comes from the child: 'error' either way, plus `open`'s exit code on macOS, where the child is /usr/bin/open rather than the editor and exits in milliseconds. Elsewhere it is 'spawn', since waiting for exit would mean waiting for the contributor to close their editor. The test that covered this injected a spawn that throws — the one failure real spawn does not produce for an unexecutable target. The fake now returns a handle and emits on a later turn, and the two new cases fail on the previous code. Detection also ran per site row. Every site is mounted at once, so with N sites that was N × ~14 synchronous stats on the main process at load, and a choice made in one row left every other row's button still saying "Open in editor". The choice is now held once for the window and passed down: load asks `editor:get`, a store read that touches no filesystem, and detection waits for the picker to open. `chosenMissing` goes with it — nothing consumed it, and an editor that has moved is reported by trying to open it, which the contributor has just asked for anyway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot's review of #158, all seven findings. Detection and validation stat'd synchronously on the process that draws the window — a dozen probes of locations that mostly do not exist, which is free on the author's machine and a frozen window behind a Windows antivirus filter driver. The injected probes are awaited now, and the candidates are probed together rather than one after another. On Linux the picker cannot filter by extension, and "a regular file" was enough to be remembered as the contributor's editor: a document was accepted and then failed with EACCES at the spawn. Executability is now part of the answer, asked of the OS with access(X_OK) so it is about the user this app runs as rather than about mode bits. The stored name was the basename, which reads well on macOS by accident and not at all on Windows: "Code" for Visual Studio Code, "phpstorm64" for PhpStorm. A known application is now named the way the picker named it, and only an unknown one the contributor pointed at falls back to its filename. Three renderer paths could end in a button that appeared to do nothing. A rejected invoke — a handler that throws, a window being torn down — skipped both the notice and the picker; a failed load of the remembered choice was swallowed entirely, making "the store could not be read" indistinguishable from "nothing chosen yet"; and choosing an editor that then failed to launch closed the picker anyway, leaving a notice and no way forward, which is the opposite of what this feature promises. The picker now closes only on a launch that worked, and the reason it opened is rendered inside it, where the focus is. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

## Why Opening a site's folder shipped in #158 as a *remembered choice*: an **Open in editor** button, a **Show in Finder** button, a **Change editor** button, and a modal picker on first use — four controls and a modal for one intention. This explores the alternative shape sketched in the issue thread: one menu that says what it does, listing the applications the folder can be opened in. The application is what the contributor is actually choosing, and it changes — Finder to look at the files, an editor to change them. Making it a setting configured once puts the wrong thing in front of them and then makes them go back to change it. ## What changes **One control, on the row under the path it acts on.** `Open directory in ⌄` opens a menu of the file manager, every editor detection found, and **Other application…**. Nothing is remembered: the application is an argument to `editor:open`, not a stored preference. That deletes `editor:get`, `editor:choose`, the `preferences.editor` store entry, the first-run picker modal, and the stale-remembered-editor failure path — three of the four old buttons go with them. **Deliberately not in it: the "(not installed)" rows** from the sketch. Detection in `src/editor-launch.js` is a fixed table of absolute install locations, and its own header calls it "a convenience, not the contract" — it misses JetBrains' versioned install directories and most Linux packaging. A greyed-out **VS Code (not installed)** would be a confident lie to anyone whose install the table does not probe, and #158's standing rule is that nothing here ends at a disabled button. The menu shows what was found; **Other application…** is always there, never only as a fallback. **The security shape changed, and that is the part to review hardest.** The renderer now names an executable, where the path used to come out of the main-side store. So `editor:open` re-establishes what it is allowed to launch: `matchDetectedEditor` re-runs detection and answers with the *detected* path, which is what gets spawned — checking one string and launching the caller's string would not be a check (same reason `external-url.js` hands the OS its parsed URL). A path detection did not return is refused as `unknown-editor` before `openSiteInEditor` is reached. `null` still means the native file dialog, whose result is validated exactly as it was under `editor:choose`. The path chip and its copy button are untouched — still the floor under all of it. ## How to test this Platforms: **any**, but the case-insensitivity of the new check is what differs across them and is covered by tests that run both branches from one machine (`test/editor-launch.test.cjs`). **Starting state:** the app open on any site, with at least one of VS Code / Cursor / PhpStorm / Sublime Text / Zed installed in its normal location, and ideally one editor installed somewhere unusual (a JetBrains Toolbox PhpStorm, or any `.app` outside `/Applications`). 1. Look at the site header. Under the path row there is a single **Open directory in ⌄** link. The old **Open in editor** / **Show in Finder** / **Change editor** buttons are gone, and the path chip and its copy button are unchanged. 2. Click it. The menu lists **Finder** (or **File Explorer**), then each editor found on this machine by name, then **Other application…**. No row is greyed out, and no row claims something is "not installed". 3. Choose **Finder**. The folder opens in the file manager. 4. Reopen the menu and choose an editor. That editor opens on the folder. Reopen the menu: it looks exactly as it did — nothing was remembered, no "Change editor" appeared, the chosen one is not marked. 5. Choose **Other application…** and press Escape / Cancel in the dialog. Nothing opens and **no error appears** — dismissing a dialog is an answer, not a failure. 6. Choose **Other application…** again and point it at the editor installed somewhere the menu did not list. It opens, and it is still not remembered. 7. Choose **Other application…** and point it at something that is not an application (a `.txt`, or any document). A yellow notice appears under the path saying it is not an application this app can open a folder in, with a **Choose application…** button that reopens the dialog. 8. Open the **⋯ More** menu at the top right. **Copy path** and **Show in Finder** are still there; **Open in editor** is not — that lives in the new menu now. **What must not have happened:** - No remembered editor anywhere: no button that says "Open in Cursor", no "Change editor", no picker on first use. If any of those appear, a code path from #158 survived. - No dead end. Every menu row does something, and every failure leaves a visible notice with a way forward. A row that opens nothing and says nothing is the one outcome this feature may not produce. - Nothing writes to `preferences.editor` any more. An existing user's stored editor is left inert in `settings.json` — it must not be read, and their sites and contributor details must be untouched. Not testable by hand here: the Windows and Linux branches of the launchable-path and case-sensitivity checks. Those are exercised by injection in `test/editor-launch.test.cjs` rather than skipped, and I verified both new checks fail the suite when the logic is removed. ## Risks and limitations - **One extra click per open, forever.** The remembered-editor button was one click; this is two. That is the trade this shape makes, and it is the main thing to decide before merging. - Self-review found **3 [fix here] · 1 [follow-up]** — all 3 fixed, 1 deferred. Detail in the collapsed block below. - The menu re-runs detection on each open rather than caching it, so an editor installed while the app is running appears next time. That is ~10–20 `stat`s per open, all async, off the main thread's critical path. - Not done: keyboard-driven verification of the popover on Windows. The menu is `Dropdown` + `MenuGroup`/`MenuItem` from `@wordpress/components`, the same primitives the sidebar's feedback dropdown and the ⋯ menu already use. ## Related Follow-up to #158. Explores the alternative UI proposed for it. --- <details> <summary>Design decisions and alternatives considered</summary> **Greyed-out "not installed" rows — rejected.** The sketch showed all five known editors always listed, disabled when absent. Two problems: it turns the detection table from a shortcut into a claim about the machine, and the table is knowingly incomplete (its own comment says the picker covers versioned JetBrains installs "exactly as well"). Someone with PhpStorm installed via the JetBrains installer would read **PHPStorm (not installed)** and believe the app. A third option — dimmed but clickable, opening the dialog prefilled — was also dropped: it makes a row mean two different things depending on a state the contributor cannot see, and **Other application…** already covers it honestly. **Dropping the memory entirely — chosen over "remember, last used on top".** Keeping the memory would have kept `editor:get`, `editor:choose` and the stored preference alive to save one click. Since the menu has to exist anyway, the remembered copy is a second source of truth for the same answer, plus a failure path (the remembered editor moved) that only exists because something was remembered. The one click is the price. **Where the menu goes.** First on the metadata line beside the created date, then beside the copy button, and finally on its own row under the path — the folder is what it acts on, so it reads directly beneath the folder, above everything that acts on the site itself. **`matchDetectedEditor` returns the candidate, not a boolean.** A boolean would let a caller check one string and spawn another; returning the path detection vouches for makes that mistake unwritable. It lives in `editor-launch.js` rather than the handler so the case-sensitivity split has an injected `platform` and both branches are testable from one machine — the house pattern. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **3 [fix here] · 1 [follow-up] — all 3 fixed, the follow-up deferred.** Fixed: 1. **Tests 🟡** — the new allow-list check read `process.platform` inline, so neither branch of the case-sensitivity comparison could be exercised from one machine, and no test passed a case-differing path at all: the suite would have stayed green with `normalize` deleted. Moved the check into `editor-launch.js` as `matchDetectedEditor` with an injected `platform`, and added three tests covering the macOS, Windows and Linux branches. Verified both new tests fail when the logic is removed. 2. **Security 🔵** — the handler compared `normalize(candidate.path)` but then passed `target`, the renderer's own string, to `openSiteInEditor`. On a case-sensitive volume `/applications/Cursor.app` would pass the comparison while being a different filesystem entry. Now the detected path is what is spawned. 3. **Architecture 🔵** — the comparison duplicated `knownEditorName`'s normalisation while leaving that function without a production caller. Both now live in `editor-launch.js`; the duplication is gone. Deferred: 4. **Architecture 🔵 [follow-up]** — the **Other application…** branch opens the file dialog before `sitePath` is checked against the registry, so a site forgotten in another window would show a file browser and only then refuse. The ordering predates this PR (`editor:choose` had the same dialog with no site in hand at all), and fixing it belongs with the registry-staleness handling rather than here. Verified and explicitly not findings: `preferences.editor` needs no migration (nothing reads it on either side of the upgrade, and the surviving writer spreads the existing object, so contributor provenance is preserved); the second detection sweep per open is not a user-visible cost. Style notes from the same pass, all applied: comments in `settings-store.js`, `main.js` and `index.jsx` that still described the remembered editor, the `editorChoice` variable name for a hook that no longer holds a choice, and `'unknown-editor'` as a bare literal rather than a `REFUSAL_REASONS` entry. </details> <details> <summary>Implementation notes</summary> - `src/editor-launch.js` — adds `matchDetectedEditor(editorPath, { platform, env, exists })` and `REFUSAL_REASONS.UNKNOWN_EDITOR`. Detection, launchability and the spawn are unchanged; nothing here consults `PATH`, at either end, for the reason the file's header gives. - `src/main.js` — `editor:get` and `editor:choose` are gone; `editor:list` drops `chosen`; `editor:open` takes `(sitePath, editorPath | null)`. - `src/preload.js` — `getEditor` and `chooseEditor` removed; `openInEditor` takes the second argument. - `src/renderer/index.jsx` — `useEditorChoice` becomes `useDetectedEditors` (detected + loading, no chosen, no remember); the picker modal and the three-button row are deleted; the inline notice stays and is now the only place a failure speaks, so it carries **Choose application…** with it. - Tests: `test/editor-launch.test.cjs` gains the three `matchDetectedEditor` cases; `test/ipc-wiring.test.cjs` swaps the `editor:get`/`editor:choose` wiring tests for the refusal and file-dialog paths. 419 pass, lint clean. </details> <details> <summary>Screenshots or recording</summary> **The menu, open**, on a site with four applications found. Everything the folder can be opened in, in one list, with `Other application…` always at the end — no row disabled, nothing claiming to know what is not installed: <img width="2000" height="1336" alt="1-menu-open" src="https://github.com/user-attachments/assets/f183a31f-2dd5-499e-8117-f5b4e3cf3a67" /> **Closed**, the resting state: one link directly under the path it acts on, where three buttons and a "Change editor" used to be: <img width="2000" height="1336" alt="2-menu-closed" src="https://github.com/user-attachments/assets/565aebc2-ab12-4df9-ba2d-268063032949" /> Both captured from the running app on macOS through `webContents.capturePage()`, with the menu opened by a real click rather than a mock. </details> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

## Why On Windows, choosing a detected editor from **Open directory in** appears to do nothing. No editor window, and no error notice either — the app reports success. The editor is in fact running: Task Manager shows Visual Studio Code under *Background processes*, holding ~400 MB, with no window anywhere. (Reported against the buttons #158 shipped; #209 has since replaced them with the menu, and the bug is in the spawn underneath both.) That is the failure shape this project treats as an architectural bug rather than a cosmetic one. A Contributor Day newcomer clicks a button, nothing happens, and there is nothing on screen to diagnose. ## What changes The root cause is `windowsHide: true` on the editor's spawn — the same option the app passes to its console children, where it stops Windows allocating a visible console for every npm and grunt subprocess. For a GUI application the flag means something else. Alongside `CREATE_NO_WINDOW` it sets `STARTF_USESHOWWINDOW` with `SW_HIDE` in the new process's STARTUPINFO, and an application that honors `nCmdShow` when creating its first window starts invisible. Electron apps do, so VS Code and Cursor both. The spawn itself succeeds, the child emits `spawn`, `awaitLaunch` returns ok, and the renderer correctly has nothing to report. So the editor spawn drops the flag. Nothing else does: the runners keep it through `hide-child-windows.js`, which this does not touch. That patch is applied only inside the four runner child processes and never in the main process, so nothing re-adds the flag to this call site. The comment at the call site was also wrong in a way worth correcting while here — it claimed the options match what main.js uses everywhere else, but `detached` is unconditional here and conditional in the runners, for a real reason: the runners are killed as a process group, and this child is released rather than ever signalled. ## How to test this **Platform: Windows.** The bug does not exist on macOS or Linux — `windowsHide` is a no-op there, and macOS goes through `/usr/bin/open` rather than spawning the editor directly. Buildkite builds a signed artifact for this branch; check it matches the head commit, since rebasing onto current trunk invalidated the earlier ones. **Starting state:** a Windows machine with Visual Studio Code installed by the user installer, so it lands at `%LOCALAPPDATA%\Programs\Microsoft VS Code\Code.exe`. Any site in the app will do; it does not need to be initialized. 1. Open the app, select a site, and open the **Open directory in** menu under the site path. It lists **Show in Explorer**, then **Visual Studio Code** — if the editor is not listed, detection did not find it, which is a different problem from this one. 2. Choose **Visual Studio Code**. It opens **with a visible window**, showing the site folder. 3. Repeat through **Other application…** and pick `Code.exe` by hand. It opens with a window too — that path reaches the same spawn, and it is the one a contributor with an editor detection misses will take. 4. Close VS Code, then close the app entirely, and repeat step 2 from a fresh launch. The window appears again — the editor is spawned detached, so this also confirms it still outlives the app rather than being killed with it. **What must not have happened:** - **No console window flashes** at any point during steps 2 and 3. That is what `windowsHide` was doing correctly for the runners, and the concern with removing it. Then run **Install npm dependencies** on a site and watch: still no console flashes there either, since that path is unaffected. - **No orphaned editor process.** After step 4, closing VS Code's window should leave nothing behind in Task Manager under "Visual Studio Code" — the symptom of this bug was precisely a windowless process sitting there, so the old failure is easy to recognise. To watch the bug fail to reproduce, do step 2 on a build from `trunk` first: the menu item does nothing, and Task Manager shows Visual Studio Code as a background process with no window. The regression test is `the editor is not asked to start hidden on Windows` in `test/editor-launch.test.cjs`. It drives the win32 branch from any machine by injecting `platform`, the house pattern from `win-spawn-patch.test.cjs`. I checked it fails on the old code — `windowsHide: true` was present in the recorded spawn options — and passes on the new. The existing launch-options assertion was updated in the same direction rather than left pinning the old behaviour. ## Risks and limitations Self-review came back **0 [fix here] · 2 [follow-up]**, both filed rather than deferred silently (#202, #203). I could not test this by hand myself — I have no Windows machine. The reproduction and the confirmation of the running-but-windowless process came from a maintainer's VM; the fix itself has only been verified by the suite and by reading the Win32 process-creation semantics. A reviewer on Windows driving the steps above is what would actually close that gap. The one behaviour change beyond the fix: a contributor who points the picker at a *console-mode* editor now gets its console window, where before it was hidden. That is the wanted behaviour — a terminal editor with a hidden terminal is the same bug — but it is a change, not a no-op. ## Related Fixes #181. Follow-ups filed: #202, #203. --- <details> <summary>Design decisions and alternatives considered</summary> **Why not keep the flag and add `shell: false` + a `start` wrapper, or go through `shell.openPath`?** Both would work around the flag rather than remove the thing that was wrong. `shell.openPath` in particular would hand the folder to whatever the OS has registered for a directory, which is Explorer — that is the *other* button. **Why not make it conditional — hide for console editors, show for GUI ones?** There is no reliable way to ask a `.exe` which subsystem it targets without reading its PE header, and the app has no business doing that. The simpler rule is correct: this call site launches the contributor's editor, and an editor's window is the point of the click. **Why the flag stays for the runners.** They exist to collect output that is streamed to the renderer; their console windows are pure noise, and `hide-child-windows.js` documents why the patch has to reach grandchildren. Nothing about that reasoning applies to the editor. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **0 [fix here] · 2 [follow-up]** — both follow-ups filed as issues rather than fixed here. The judgement pass ran in a subagent with fresh context, per `.claude/skills/self-review/SKILL.md`. What it verified rather than assumed: - Nothing re-applies `windowsHide` to this spawn. `hideChildWindows()` is called only in the four runner child processes; the main process never requires the module. `win-spawn-patch.js` self-applies only under `WPTK_SPAWN_PATCH=1`, never touches `windowsHide`, and its `resolveSpawnTarget` returns `null` for a `.exe` anyway. - No console-flash regression at this call site: `resolveLaunch` produces `/usr/bin/open` on macOS and the editor `.exe` elsewhere, not the `cmd.exe`/`grunt.cmd` grandchildren the patch exists for. - No kill-tree interaction: the child is `unref`'d and never registered with `killChildTree`, so `detached` here is about outliving the app, not signalling a group. - The regression test genuinely fails on the old code. **Follow-up 1 — #202** (cross-platform, 🔵): the Windows rule in `.github/instructions/code-review.instructions.md` states `windowsHide: true` flatly, without the GUI/console distinction that caused this. As written it will give the same wrong advice to the next GUI spawn someone adds. **Follow-up 2 — #203** (tests/architecture, 🔵): `patchChildProcess` deliberately overrides an explicit `windowsHide: false`, so a future call to `hideChildWindows()` in the main process would silently reinstate this bug, and no test would catch it — `test/editor-launch.test.cjs` injects its own `spawn`, and `test/runner-wiring.test.cjs` only asserts the runners *do* call the patch. One style note from the review was fixed before opening: the comment claiming parity with main.js's other spawn options, described under **What changes**. </details> <details> <summary>Screenshots or recording</summary> Nothing on screen changed inside the app — the menu, its items and the notice area are untouched. What changes is off-app: whether VS Code's own window appears after the choice. The before state is visible in #181: Task Manager filtered to "code", showing Visual Studio Code under *Background processes (1)* with *Apps (0)* — the editor running with no window. </details> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

juanmaguitar requested a balanced review from Copilot August 7, 2026 05:54

Copilot started reviewing on behalf of juanmaguitar August 7, 2026 05:54 View session

juanmaguitar force-pushed the juanmaguitar/opening-a-sites-code-means-leaving-the-app branch from 6a84860 to 1276c0a Compare August 7, 2026 06:00

Copilot AI reviewed Aug 7, 2026

View reviewed changes

juanmaguitar and others added 5 commits August 7, 2026 12:44

juanmaguitar force-pushed the juanmaguitar/opening-a-sites-code-means-leaving-the-app branch from 7cfa97f to 1cdfdec Compare August 7, 2026 10:46

juanmaguitar merged commit 0e805fa into trunk Aug 7, 2026
3 checks passed

This was referenced Aug 8, 2026

"Show in Explorer" and "Open in editor" fail with "unknown error" while a new site is still being set up #180

Closed

Open a site's folder from one menu instead of a remembered editor #209

Merged

This was referenced Aug 9, 2026

Let the editor open with a window on Windows #201

Merged

A site whose folder was moved reports that the editor would not start #219

Closed

juanmaguitar deleted the juanmaguitar/opening-a-sites-code-means-leaving-the-app branch August 11, 2026 11:29

Navigation Menu

Uh oh!

Open a site's code without leaving the app - #158

Open a site's code without leaving the app#158
juanmaguitar merged 5 commits into
trunkfrom
juanmaguitar/opening-a-sites-code-means-leaving-the-app

juanmaguitar commented Aug 7, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

juanmaguitar commented Aug 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

juanmaguitar commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The constraint this is built around

Guards

Two moves that are not the feature

Review

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

juanmaguitar commented Aug 7, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

juanmaguitar commented Aug 7, 2026 •

edited

Loading