From 2a37212f5156796e9c83da4630a3f3b54ef54cd3 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Fri, 7 Aug 2026 07:14:11 +0200 Subject: [PATCH 1/5] Move the refusal log formatter into src/safe-log.js MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/external-url.js | 31 +++++--------------------- src/safe-log.js | 53 ++++++++++++++++++++++++++++++++++++++++++++ src/site-registry.js | 30 +++++-------------------- 3 files changed, 63 insertions(+), 51 deletions(-) create mode 100644 src/safe-log.js diff --git a/src/external-url.js b/src/external-url.js index 5b59a28..22a6dc1 100644 --- a/src/external-url.js +++ b/src/external-url.js @@ -17,6 +17,8 @@ // Widen ALLOWED_URL_SCHEMES only for a scheme the app actually needs, and only // after asking what the OS does with it. +const { describeRefused } = require('./safe-log'); + const ALLOWED_URL_SCHEMES = ['http:', 'https:']; // Returns the address to open, or null if it is not one this app opens. @@ -53,34 +55,11 @@ function isAllowedExternalUrl(url) { return normalizeExternalUrl(url) !== null; } -// Line breaks, and everything else that would let a refused address end a log -// line and start another one. -const CONTROL_CHARACTERS = /[\x00-\x1f\x7f-\x9f\u2028\u2029]/g; - // A refused address is attacker-influenced by hypothesis, and it is about to be -// written into the file contributors attach to bug reports. Two things follow. -// -// It has to stay on one line: a newline in the address would otherwise let it -// write a second entry in the app's own timestamp-and-scope format, and a log -// that can be made to describe events that never happened is worse than no log. -// The control characters are escaped rather than dropped so the line still says -// what the caller actually sent. -// -// And it has to be bounded, so a very long address cannot flood the file. -// Truncation comes after escaping, since escaping is what decides the final -// length. +// written into the file contributors attach to bug reports, so it has to stay on +// one line and it has to be bounded. safe-log.js is where both live, and why. function describeRefusedUrl(url) { - if (typeof url !== 'string') return `<${url === null ? 'null' : typeof url}>`; - - const oneLine = url.replace(CONTROL_CHARACTERS, (c) => { - const code = c.codePointAt(0); - return code <= 0xff - ? `\\x${code.toString(16).padStart(2, '0')}` - : `\\u${code.toString(16).padStart(4, '0')}`; - }); - - if (oneLine.length <= 120) return oneLine; - return `${oneLine.slice(0, 120)}…`; + return describeRefused(url); } // The `url:open` handler's body, kept out of main.js so both sides of the guard diff --git a/src/safe-log.js b/src/safe-log.js new file mode 100644 index 0000000..324939b --- /dev/null +++ b/src/safe-log.js @@ -0,0 +1,53 @@ +// How this app writes an attacker-influenced string into its own log file. +// +// Every guard module in src/ refuses some input — a URL whose scheme the app +// does not open, a path that is not a registered site, an application the app +// will not launch — and every refusal is logged, because a guard that trips +// silently is a guard nobody finds out about. The value being logged is +// attacker-influenced by hypothesis: that is why it was refused. +// +// Two things follow, and they are the whole of this module. +// +// It has to stay on one line. A newline in the value would otherwise let it +// write a second entry in the app's own timestamp-and-scope format, and a log +// that can be made to describe events that never happened is worse than no log. +// The control characters are escaped rather than dropped so the line still says +// what the caller actually sent. +// +// And it has to be bounded, so a very long value cannot flood the file. +// Truncation comes after escaping, since escaping is what decides the final +// length. +// +// This lived twice — once in external-url.js and once in site-registry.js, the +// second with a comment saying the third caller should be the one to move it +// here. editor-launch.js is that third caller. + +// Line breaks, and everything else that would let a refused value end a log line +// and start another one. +const CONTROL_CHARACTERS = /[\x00-\x1f\x7f-\x9f\u2028\u2029]/g; + +const MAX_DESCRIPTION_LENGTH = 120; + +// A one-line, bounded rendering of a refused value, safe to concatenate into a +// log message. A non-string is described by its type rather than coerced, so a +// caller that passed the wrong thing entirely reads as that in the log instead +// of as an empty or `[object Object]` value. +function describeRefused(value) { + if (typeof value !== 'string') return `<${value === null ? 'null' : typeof value}>`; + + const oneLine = value.replace(CONTROL_CHARACTERS, (c) => { + const code = c.codePointAt(0); + return code <= 0xff + ? `\\x${code.toString(16).padStart(2, '0')}` + : `\\u${code.toString(16).padStart(4, '0')}`; + }); + + if (oneLine.length <= MAX_DESCRIPTION_LENGTH) return oneLine; + return `${oneLine.slice(0, MAX_DESCRIPTION_LENGTH)}…`; +} + +module.exports = { + CONTROL_CHARACTERS, + MAX_DESCRIPTION_LENGTH, + describeRefused +}; diff --git a/src/site-registry.js b/src/site-registry.js index 6557f26..bbfd4a9 100644 --- a/src/site-registry.js +++ b/src/site-registry.js @@ -14,6 +14,8 @@ // pure check, a safe log formatter, and a wrapper whose effects are injected so // both branches can be tested without an Electron process. +const { describeRefused } = require('./safe-log'); + // True only for a path the app has on record. Exact string match, the same // convention `sites:add`/`sites:delete` already use (`sites.includes(sitePath)`, // `filter((p) => p !== sitePath)`): the registry stores the paths verbatim, so a @@ -25,33 +27,11 @@ function isRegisteredSite(sitePath, sites) { return sites.includes(sitePath); } -// Line breaks, and everything else that would let a refused path end a log line -// and start another one. -const CONTROL_CHARACTERS = /[\x00-\x1f\x7f-\x9f\u2028\u2029]/g; - // A refused path is attacker-influenced by hypothesis, and it is about to be -// written into the file contributors attach to bug reports. It has to stay on -// one line — a newline would otherwise let it forge a second entry in the app's -// own timestamp-and-scope format — and it has to be bounded so a very long path -// cannot flood the file. Control characters are escaped rather than dropped so -// the line still says what the caller actually sent; truncation comes after -// escaping, since escaping is what decides the final length. -// -// This is the same concern, and the same escaping, as `describeRefusedUrl` in -// external-url.js. If a third caller ever needs it, the two should move into a -// shared safe-log helper rather than gain a third copy. +// written into the file contributors attach to bug reports, so it has to stay on +// one line and it has to be bounded. safe-log.js is where both live, and why. function describeRefusedSite(sitePath) { - if (typeof sitePath !== 'string') return `<${sitePath === null ? 'null' : typeof sitePath}>`; - - const oneLine = sitePath.replace(CONTROL_CHARACTERS, (c) => { - const code = c.codePointAt(0); - return code <= 0xff - ? `\\x${code.toString(16).padStart(2, '0')}` - : `\\u${code.toString(16).padStart(4, '0')}`; - }); - - if (oneLine.length <= 120) return oneLine; - return `${oneLine.slice(0, 120)}…`; + return describeRefused(sitePath); } // The `sites:delete` handler's body, kept out of main.js so both sides of the From 5f395dee70bc3734e4384fad059b12f8060f7840 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Fri, 7 Aug 2026 07:21:04 +0200 Subject: [PATCH 2/5] Find and launch the contributor's editor without a shell PATH MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/editor-launch.js | 269 ++++++++++++++++++++++++++++++++ src/main.js | 117 +++++++++++++- src/preload.js | 9 ++ src/settings-store.js | 5 +- src/site-registry.js | 19 +++ test/editor-launch.test.cjs | 298 ++++++++++++++++++++++++++++++++++++ test/ipc-wiring.test.cjs | 145 +++++++++++++++++- test/site-registry.test.cjs | 51 +++++- 8 files changed, 909 insertions(+), 4 deletions(-) create mode 100644 src/editor-launch.js create mode 100644 test/editor-launch.test.cjs diff --git a/src/editor-launch.js b/src/editor-launch.js new file mode 100644 index 0000000..9cbfd13 --- /dev/null +++ b/src/editor-launch.js @@ -0,0 +1,269 @@ +// Opening a site's folder in the contributor's editor. +// +// This existed once and was removed (#24 → #26) for a reason that decides the +// shape of everything below: detection ran `which`/`where`, and **a packaged +// Electron app does not inherit the shell's PATH**. A correctly installed VS +// Code reported as missing in the shipped build while working fine in +// `npm start`, which is the worst possible failure — it only appears in the +// artifact contributors actually download. +// +// So nothing here consults PATH, at either end: +// +// - Detection is a filesystem existence check against absolute, per-platform +// install locations. No `which`, no `where`, no spawning anything to find out +// whether something is installed. +// - Launching refuses a relative command. `spawn('code', …)` without a shell +// would resolve through PATH — the same environment that is not there — so an +// editor path that is not absolute is not one this module will run. +// +// The table below is a convenience, not the contract. It exists so the common +// case needs no configuration; the contributor pointing at their own +// application is the case that always works, and the caller must always offer +// it. An editor this table misses must never surface as "unavailable" with +// nothing to do about it. +// +// The guard is the same shape as external-url.js and site-registry.js: a pure +// check, a safe log formatter, and a wrapper whose effects (`exists`, +// `statPath`, `spawn`) are injected, so both branches are testable with no +// Electron process and no editor installed. + +const path = require('path'); +const { describeRefused } = require('./safe-log'); +const { isRegisteredSite } = require('./site-registry'); + +// Path semantics follow the platform being asked about, not the platform the +// test happens to run on: `path.isAbsolute('C:\\x')` is false under POSIX, and a +// Windows check that only holds on Windows is a check that never runs in CI. +function pathApi(platform) { + return platform === 'win32' ? path.win32 : path.posix; +} + +// Windows environment variables are case-insensitive to the OS, and the casing +// in documentation ('ProgramFiles', 'LOCALAPPDATA') is not always the casing in +// the environment. Reading them case-insensitively keeps the table from +// depending on which one a machine happens to use. +function envValue(env, name) { + if (!env) return undefined; + if (env[name] !== undefined) return env[name]; + const wanted = name.toLowerCase(); + const match = Object.keys(env).find((key) => key.toLowerCase() === wanted); + return match === undefined ? undefined : env[match]; +} + +// Every entry is `{ id, name, paths }` where `paths` are absolute locations to +// probe, most likely first. A location that cannot be built — the environment +// variable it needs is not set — is dropped rather than guessed at. +// +// Deliberately not here: anything installed under a version-numbered directory +// (JetBrains' own installer writes `PhpStorm 2024.3\bin\…`). Finding those means +// listing directories and pattern-matching versions, which is a second thing to +// be wrong about; the picker covers them exactly as well. +function editorCandidates({ platform, env = {} } = {}) { + const p = pathApi(platform); + + if (platform === 'darwin') { + const home = envValue(env, 'HOME'); + const roots = ['/Applications', home ? p.join(home, 'Applications') : null].filter(Boolean); + const bundles = [ + { id: 'vscode', name: 'Visual Studio Code', bundle: 'Visual Studio Code.app' }, + { id: 'cursor', name: 'Cursor', bundle: 'Cursor.app' }, + { id: 'phpstorm', name: 'PhpStorm', bundle: 'PhpStorm.app' }, + { id: 'sublime', name: 'Sublime Text', bundle: 'Sublime Text.app' }, + { id: 'zed', name: 'Zed', bundle: 'Zed.app' } + ]; + return bundles.map(({ id, name, bundle }) => ({ + id, + name, + paths: roots.map((root) => p.join(root, bundle)) + })); + } + + if (platform === 'win32') { + const localAppData = envValue(env, 'LOCALAPPDATA'); + const programFiles = envValue(env, 'ProgramFiles'); + const programFilesX86 = envValue(env, 'ProgramFiles(x86)'); + const under = (root, ...rest) => (root ? p.join(root, ...rest) : null); + + return [ + { + id: 'vscode', + name: 'Visual Studio Code', + paths: [ + under(localAppData, 'Programs', 'Microsoft VS Code', 'Code.exe'), + under(programFiles, 'Microsoft VS Code', 'Code.exe'), + under(programFilesX86, 'Microsoft VS Code', 'Code.exe') + ] + }, + { + id: 'cursor', + name: 'Cursor', + paths: [under(localAppData, 'Programs', 'cursor', 'Cursor.exe')] + }, + { + id: 'phpstorm', + name: 'PhpStorm', + // JetBrains Toolbox's stable launcher location. The standalone + // installer's versioned directory is the picker's job. + paths: [under(localAppData, 'Programs', 'PhpStorm', 'bin', 'phpstorm64.exe')] + }, + { + id: 'sublime', + name: 'Sublime Text', + paths: [under(programFiles, 'Sublime Text', 'sublime_text.exe')] + }, + { + id: 'zed', + name: 'Zed', + paths: [under(localAppData, 'Programs', 'Zed', 'Zed.exe')] + } + ].map((entry) => ({ ...entry, paths: entry.paths.filter(Boolean) })); + } + + // Linux packaging is too varied for a table to be authoritative — these are + // the locations the common packages use, and the picker is the real answer. + const home = envValue(env, 'HOME'); + const inHome = (...rest) => (home ? p.join(home, ...rest) : null); + return [ + { + id: 'vscode', + name: 'Visual Studio Code', + paths: ['/usr/share/code/code', '/usr/bin/code', '/snap/bin/code', '/opt/visual-studio-code/code'] + }, + { id: 'cursor', name: 'Cursor', paths: ['/usr/bin/cursor', '/snap/bin/cursor', '/opt/Cursor/cursor'] }, + { id: 'phpstorm', name: 'PhpStorm', paths: ['/snap/bin/phpstorm', '/opt/phpstorm/bin/phpstorm.sh'] }, + { id: 'sublime', name: 'Sublime Text', paths: ['/usr/bin/subl', '/snap/bin/sublime-text'] }, + { id: 'zed', name: 'Zed', paths: ['/usr/bin/zed', '/snap/bin/zed', inHome('.local', 'bin', 'zed')].filter(Boolean) } + ]; +} + +// The editors this machine has, in table order, each reduced to the first +// location that exists. `exists` is injected — it is the only thing detection +// does, and the only thing a test has to stand in for. +function detectEditors({ platform, env = {}, exists } = {}) { + if (typeof exists !== 'function') return []; + + return editorCandidates({ platform, env }) + .map(({ id, name, paths }) => { + const found = paths.find((candidate) => { + try { + return exists(candidate) === true; + } catch { + // An unreadable location is a location we do not have, not a crash + // on the way to drawing a button. + return false; + } + }); + return found ? { id, name, path: found } : null; + }) + .filter(Boolean); +} + +// Whether a path is something this app will hand to the OS as an application. +// +// Absolute, because a relative command would be resolved through PATH by spawn, +// and of the shape the platform uses for an application: a `.app` bundle +// (a directory) on macOS, an `.exe` on Windows, a regular file elsewhere. The +// same check covers both a detected path and one the contributor picked — the +// picker is a dialog, and a dialog's result is still input. +// +// `statPath` returns `{ isDirectory, isFile }` or null when there is nothing +// there; it is injected for the same reason `exists` is. +function isLaunchableEditorPath(editorPath, { platform, statPath } = {}) { + if (typeof editorPath !== 'string' || editorPath === '') return false; + if (typeof statPath !== 'function') return false; + if (!pathApi(platform).isAbsolute(editorPath)) return false; + + let stats; + try { + stats = statPath(editorPath); + } catch { + return false; + } + if (!stats) return false; + + if (platform === 'darwin') { + return stats.isDirectory === true && editorPath.toLowerCase().endsWith('.app'); + } + if (platform === 'win32') { + return stats.isFile === true && editorPath.toLowerCase().endsWith('.exe'); + } + return stats.isFile === true; +} + +// What to run, as a command and an argument vector — never a string to be +// re-parsed by a shell, and never a concatenation. +// +// macOS goes through `/usr/bin/open -a`, a fixed absolute path, because a `.app` +// bundle is a directory rather than something executable. Everywhere else the +// executable takes the folder as its argument, which is what every editor in the +// table above supports. +function resolveLaunch(editorPath, sitePath, { platform } = {}) { + if (platform === 'darwin') { + return { command: '/usr/bin/open', args: ['-a', editorPath, sitePath] }; + } + return { command: editorPath, args: [sitePath] }; +} + +const REFUSAL_REASONS = { + UNREGISTERED_SITE: 'unregistered-site', + UNLAUNCHABLE_EDITOR: 'unlaunchable-editor' +}; + +// The `editor:open` handler's body. +// +// Two gates, both of which have to pass before anything is spawned. The folder +// must be one the app has on record — `isRegisteredSite` from site-registry.js, +// the same boundary `sites:delete` uses — so "open this site" cannot become +// "open this arbitrary directory". And the application must be absolute and of +// the platform's shape, so an editor path that has been tampered with, or an +// editor that has since been uninstalled, is refused rather than resolved +// through an environment that is not there. +// +// The spawn options are the ones main.js holds everywhere else it starts a +// child: no shell, hidden on Windows, detached with no stdio so the editor +// outlives the app and cannot block on a pipe nobody reads. +async function openSiteInEditor(sitePath, editorPath, { + sites, + platform, + statPath, + spawn, + onRefused +} = {}) { + if (!isRegisteredSite(sitePath, sites)) { + if (typeof onRefused === 'function') { + onRefused(REFUSAL_REASONS.UNREGISTERED_SITE, describeRefused(sitePath)); + } + return { ok: false, reason: REFUSAL_REASONS.UNREGISTERED_SITE }; + } + + if (!isLaunchableEditorPath(editorPath, { platform, statPath })) { + if (typeof onRefused === 'function') { + onRefused(REFUSAL_REASONS.UNLAUNCHABLE_EDITOR, describeRefused(editorPath)); + } + return { ok: false, reason: REFUSAL_REASONS.UNLAUNCHABLE_EDITOR }; + } + + const { command, args } = resolveLaunch(editorPath, sitePath, { platform }); + + try { + const child = spawn(command, args, { + detached: true, + stdio: 'ignore', + shell: false, + windowsHide: true + }); + if (child && typeof child.unref === 'function') child.unref(); + return { ok: true }; + } catch (e) { + return { ok: false, reason: 'spawn-failed', error: e?.message ?? String(e) }; + } +} + +module.exports = { + REFUSAL_REASONS, + editorCandidates, + detectEditors, + isLaunchableEditorPath, + resolveLaunch, + openSiteInEditor +}; diff --git a/src/main.js b/src/main.js index 1045ede..2ff8b38 100644 --- a/src/main.js +++ b/src/main.js @@ -34,9 +34,11 @@ const { parsePatchFiles, planApply } = require('./patch-plan.cjs'); const { fetchLinkedPrs, fetchPrDiff } = require('./github-prs'); const { openAndScrape, fetchAttachment } = require('./trac-view'); const { openExternalUrl, ALLOWED_URL_SCHEMES } = require('./external-url'); -const { deleteRegisteredSite } = require('./site-registry'); +const { deleteRegisteredSite, revealRegisteredSite } = require('./site-registry'); const { getStore } = require('./settings-store'); const { parseTicketRef } = require('./renderer/trac-ticket.cjs'); +const { describeRefused } = require('./safe-log'); +const { detectEditors, isLaunchableEditorPath, openSiteInEditor } = require('./editor-launch'); const WORDPRESS_GIT_URL = 'https://github.com/WordPress/wordpress-develop.git'; @@ -957,6 +959,119 @@ ipcMain.handle('url:open', async (_e, url) => openExternalUrl(url, { onRefused: (description) => logEvent('url', `refused to open ${description} — only ${ALLOWED_URL_SCHEMES.join(', ')} are allowed`) })); +// --- opening a site's code ----------------------------------------------- +// +// See editor-launch.js for why none of this consults PATH. What is here is the +// wiring: the store holds one app-wide editor choice, and every path — detected, +// picked, or remembered from a previous run — goes through the same check before +// anything is spawned. + +function statPathSync(targetPath) { + try { + const stats = fs.statSync(targetPath); + return { isDirectory: stats.isDirectory(), isFile: stats.isFile() }; + } catch { + return null; + } +} + +const editorLaunchDeps = () => ({ platform: process.platform, statPath: statPathSync }); + +async function getChosenEditor() { + const s = await getStore(); + const chosen = (s.get('preferences') || {}).editor; + return chosen && typeof chosen.path === 'string' ? chosen : null; +} + +// The editors on this machine, plus the remembered choice. `chosenMissing` is +// the case worth naming: an editor that was chosen once and has since been +// uninstalled or moved. The renderer uses it to ask again rather than to fail on +// the next click. +ipcMain.handle('editor:list', async () => { + const chosen = await getChosenEditor(); + const stillThere = chosen ? isLaunchableEditorPath(chosen.path, editorLaunchDeps()) : false; + + return { + detected: detectEditors({ + platform: process.platform, + env: process.env, + exists: (p) => statPathSync(p) !== null + }), + chosen: stillThere ? chosen : null, + chosenMissing: Boolean(chosen) && !stillThere + }; +}); + +// Remembers an editor. With a path it is the one the contributor picked from the +// detected list; without one it opens the file dialog, which is the answer for +// every editor the detection table does not know about — the reason no editor is +// ever shown as unavailable with nothing to do about it. +// +// The dialog's result is validated exactly like a detected path. A dialog is +// still input. +ipcMain.handle('editor:choose', async (_e, editorPath) => { + let target = typeof editorPath === 'string' ? editorPath : null; + + if (!target) { + const filtersByPlatform = { + darwin: [{ name: 'Applications', extensions: ['app'] }], + win32: [{ name: 'Programs', extensions: ['exe'] }] + }; + // Everywhere else an application is just a file, so the dialog does not + // narrow what can be picked. + const filters = filtersByPlatform[process.platform] || []; + const result = await dialog.showOpenDialog({ + title: 'Choose the editor to open sites in', + properties: ['openFile'], + defaultPath: process.platform === 'darwin' ? '/Applications' : undefined, + filters + }); + if (result.canceled || result.filePaths.length === 0) return { ok: false, reason: 'cancelled' }; + target = result.filePaths[0]; + } + + if (!isLaunchableEditorPath(target, editorLaunchDeps())) { + logEvent('editor', `refused to remember ${describeRefused(target)} — not an application this app can launch`); + return { ok: false, reason: 'unlaunchable-editor' }; + } + + const s = await getStore(); + const editor = { + path: target, + name: path.basename(target, path.extname(target)) + }; + s.set('preferences', { ...(s.get('preferences') || {}), editor }); + return { ok: true, editor }; +}); + +// Only a path the app has on record is opened, and only in an application that +// is still where it was — see editor-launch.js. A refusal is logged rather than +// dropped so a caller that trips the guard shows up in the log file instead of +// just doing nothing. +ipcMain.handle('editor:open', async (_e, sitePath) => { + const chosen = await getChosenEditor(); + if (!chosen) return { ok: false, reason: 'no-editor' }; + + const s = await getStore(); + return openSiteInEditor(sitePath, chosen.path, { + ...editorLaunchDeps(), + sites: s.get('sites'), + spawn, + onRefused: (reason, description) => logEvent('editor', `refused to open ${description} — ${reason}`) + }); +}); + +// The fallback that needs no configuration at all — see site-registry.js for why +// it is behind the same boundary as `sites:delete`. +ipcMain.handle('dir:show', async (_e, sitePath) => { + const s = await getStore(); + return revealRegisteredSite(sitePath, { + sites: s.get('sites'), + reveal: (target) => shell.openPath(target), + onRefused: (description) => logEvent('sites', `refused to reveal ${description} — not a registered site`) + }); +}); + const ENGINE_RETRY_NOTICE = '\n⚠ This site requires a newer Node.js than this app bundles.\n Retrying with engine checks relaxed…\n\n'; // Spawns an npm runner, and if it fails specifically because a dependency diff --git a/src/preload.js b/src/preload.js index 669e783..fba23c3 100644 --- a/src/preload.js +++ b/src/preload.js @@ -42,6 +42,15 @@ contextBridge.exposeInMainWorld('api', { npmKill: (params) => ipcRenderer.invoke('npm:kill', params) , openExternal: (url) => ipcRenderer.invoke('url:open', url) +, + listEditors: () => ipcRenderer.invoke('editor:list') +, + // With a path, remembers that editor; without one, opens the file dialog. + chooseEditor: (editorPath) => ipcRenderer.invoke('editor:choose', editorPath) +, + openInEditor: (sitePath) => ipcRenderer.invoke('editor:open', sitePath) +, + showSiteInFileManager: (sitePath) => ipcRenderer.invoke('dir:show', sitePath) , markSiteInitialized: (sitePath) => ipcRenderer.invoke('sites:mark-initialized', sitePath) , diff --git a/src/settings-store.js b/src/settings-store.js index 6ee0585..8e5ac62 100644 --- a/src/settings-store.js +++ b/src/settings-store.js @@ -24,7 +24,10 @@ async function getStore() { const Store = m.default || m; store = new Store({ name: 'settings', - defaults: { sites: [], siteMeta: {} } + // `preferences` is app-wide rather than per-site: the editor a + // contributor uses is a fact about them, asked once, not a + // property of each checkout. + defaults: { sites: [], siteMeta: {}, preferences: {} } }); }); } diff --git a/src/site-registry.js b/src/site-registry.js index bbfd4a9..a0823a7 100644 --- a/src/site-registry.js +++ b/src/site-registry.js @@ -50,8 +50,27 @@ async function deleteRegisteredSite(sitePath, { sites, forget, remove, onRefused return true; } +// The `dir:show` handler's body. `shell.openPath` hands a local path to whatever +// the OS has registered for it, so "show this site in the file manager" gets the +// same boundary as "delete this site": only a path the app has on record. The +// reveal itself is injected, like `remove` above. +// +// `reveal` resolves to electron's own convention — the empty string on success, +// an error message otherwise — and that is passed through rather than reduced to +// a boolean, so the renderer can say what went wrong. +async function revealRegisteredSite(sitePath, { sites, reveal, onRefused } = {}) { + if (!isRegisteredSite(sitePath, sites)) { + if (typeof onRefused === 'function') onRefused(describeRefusedSite(sitePath)); + return { ok: false, reason: 'unregistered-site' }; + } + + const error = await reveal(sitePath); + return error ? { ok: false, reason: 'open-failed', error } : { ok: true }; +} + module.exports = { isRegisteredSite, describeRefusedSite, + revealRegisteredSite, deleteRegisteredSite }; diff --git a/test/editor-launch.test.cjs b/test/editor-launch.test.cjs new file mode 100644 index 0000000..7a4d7f5 --- /dev/null +++ b/test/editor-launch.test.cjs @@ -0,0 +1,298 @@ +'use strict'; + +// What this suite is really testing is the reason the first attempt was removed +// (#24 → #26): detection went through the shell's PATH, which a packaged +// Electron app does not have, so an installed editor read as missing in the +// shipped build and only there. Nothing in the module may consult PATH, and the +// tests below are written so that a change reintroducing it fails here rather +// than in someone's downloaded artifact. +// +// Everything the module touches — the filesystem, the child process — is +// injected, so these run with no editor installed, no Electron, and on a +// platform other than the one under test. + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { + REFUSAL_REASONS, + editorCandidates, + detectEditors, + isLaunchableEditorPath, + resolveLaunch, + openSiteInEditor +} = require('../src/editor-launch.js'); + +// A filesystem of exactly the paths named, and a record of everything asked +// about — the record is what lets a test assert that detection looked only at +// absolute locations. +function fakeFs(entries) { + const asked = []; + const map = new Map(Object.entries(entries)); + return { + asked, + exists(p) { + asked.push(p); + return map.has(p); + }, + statPath(p) { + asked.push(p); + const kind = map.get(p); + if (!kind) return null; + return { isDirectory: kind === 'dir', isFile: kind === 'file' }; + } + }; +} + +function recordingSpawn() { + const calls = []; + const spawn = (command, args, options) => { + calls.push({ command, args, options }); + return { unref() { calls[calls.length - 1].unrefed = true; } }; + }; + return { calls, spawn }; +} + +const MAC_ENV = { HOME: '/Users/dev' }; +const WIN_ENV = { + LOCALAPPDATA: 'C:\\Users\\dev\\AppData\\Local', + ProgramFiles: 'C:\\Program Files' +}; + +// --- detection ----------------------------------------------------------- + +test('detection asks the filesystem about absolute paths only — never PATH', () => { + const fs = fakeFs({ '/Applications/Visual Studio Code.app': 'dir' }); + + const found = detectEditors({ platform: 'darwin', env: { ...MAC_ENV, PATH: '' }, exists: fs.exists }); + + assert.deepEqual(found, [ + { id: 'vscode', name: 'Visual Studio Code', path: '/Applications/Visual Studio Code.app' } + ]); + assert.ok(fs.asked.length > 0); + for (const p of fs.asked) { + assert.ok(p.startsWith('/'), `detection probed a non-absolute location: ${p}`); + } +}); + +// The #24 regression, stated as a test: the environment a packaged app actually +// gets has no useful PATH, and detection must not care. +test('an empty PATH does not change what is detected', () => { + const installed = { 'C:\\Users\\dev\\AppData\\Local\\Programs\\Microsoft VS Code\\Code.exe': 'file' }; + + const withPath = detectEditors({ + platform: 'win32', + env: { ...WIN_ENV, PATH: 'C:\\Windows\\System32' }, + exists: fakeFs(installed).exists + }); + const withoutPath = detectEditors({ + platform: 'win32', + env: { ...WIN_ENV }, + exists: fakeFs(installed).exists + }); + + assert.deepEqual(withPath, withoutPath); + assert.equal(withPath.length, 1); + assert.equal(withPath[0].id, 'vscode'); +}); + +test('nothing installed detects nothing, and does not throw', () => { + const fs = fakeFs({}); + assert.deepEqual(detectEditors({ platform: 'darwin', env: MAC_ENV, exists: fs.exists }), []); + assert.deepEqual(detectEditors({ platform: 'win32', env: WIN_ENV, exists: fs.exists }), []); + assert.deepEqual(detectEditors({ platform: 'linux', env: {}, exists: fs.exists }), []); +}); + +test('an unreadable location is a location we do not have, not a crash', () => { + const exists = (p) => { + if (p === '/Applications/Visual Studio Code.app') throw new Error('EACCES'); + return p === '/Applications/Cursor.app'; + }; + + const found = detectEditors({ platform: 'darwin', env: MAC_ENV, exists }); + + assert.deepEqual(found.map((e) => e.id), ['cursor']); +}); + +test('an editor found in more than one location reports the first', () => { + const fs = fakeFs({ + '/Applications/Cursor.app': 'dir', + '/Users/dev/Applications/Cursor.app': 'dir' + }); + + const found = detectEditors({ platform: 'darwin', env: MAC_ENV, exists: fs.exists }); + + assert.deepEqual(found, [{ id: 'cursor', name: 'Cursor', path: '/Applications/Cursor.app' }]); +}); + +test('a location whose environment variable is unset is dropped, not guessed at', () => { + const paths = editorCandidates({ platform: 'win32', env: {} }).flatMap((e) => e.paths); + assert.deepEqual(paths, []); + + const homeless = editorCandidates({ platform: 'darwin', env: {} }).flatMap((e) => e.paths); + assert.ok(homeless.every((p) => p.startsWith('/Applications/'))); +}); + +test('Windows environment variables are read whatever their casing', () => { + const fs = fakeFs({ 'C:\\Users\\dev\\AppData\\Local\\Programs\\cursor\\Cursor.exe': 'file' }); + + const found = detectEditors({ + platform: 'win32', + env: { localappdata: 'C:\\Users\\dev\\AppData\\Local' }, + exists: fs.exists + }); + + assert.deepEqual(found.map((e) => e.id), ['cursor']); +}); + +// --- what may be launched ------------------------------------------------ + +test('a relative command is not launchable — that is how PATH would come back', () => { + const fs = fakeFs({ code: 'file' }); + + assert.equal(isLaunchableEditorPath('code', { platform: 'linux', statPath: fs.statPath }), false); + assert.equal(isLaunchableEditorPath('Code.exe', { platform: 'win32', statPath: fs.statPath }), false); +}); + +test('the shape has to match the platform', () => { + const fs = fakeFs({ + '/Applications/Cursor.app': 'dir', + '/Applications/notes.txt': 'file', + 'C:\\Program Files\\Sublime Text\\sublime_text.exe': 'file', + 'C:\\Program Files\\Sublime Text\\readme.md': 'file' + }); + + assert.equal(isLaunchableEditorPath('/Applications/Cursor.app', { platform: 'darwin', statPath: fs.statPath }), true); + // A file rather than a bundle, and a bundle name is not enough on its own. + assert.equal(isLaunchableEditorPath('/Applications/notes.txt', { platform: 'darwin', statPath: fs.statPath }), false); + assert.equal(isLaunchableEditorPath('/Applications/Missing.app', { platform: 'darwin', statPath: fs.statPath }), false); + + assert.equal(isLaunchableEditorPath('C:\\Program Files\\Sublime Text\\sublime_text.exe', { platform: 'win32', statPath: fs.statPath }), true); + assert.equal(isLaunchableEditorPath('C:\\Program Files\\Sublime Text\\readme.md', { platform: 'win32', statPath: fs.statPath }), false); +}); + +test('junk input is refused rather than thrown', () => { + const fs = fakeFs({}); + for (const value of [null, undefined, 42, '', {}]) { + assert.equal(isLaunchableEditorPath(value, { platform: 'darwin', statPath: fs.statPath }), false); + } + assert.equal(isLaunchableEditorPath('/Applications/Cursor.app', { platform: 'darwin' }), false); +}); + +// --- the command that gets run ------------------------------------------- + +test('macOS opens the bundle through a fixed absolute /usr/bin/open', () => { + const { command, args } = resolveLaunch('/Applications/Cursor.app', '/Users/dev/sites/wp', { platform: 'darwin' }); + + assert.equal(command, '/usr/bin/open'); + assert.deepEqual(args, ['-a', '/Applications/Cursor.app', '/Users/dev/sites/wp']); +}); + +test('elsewhere the executable takes the folder as an argument', () => { + assert.deepEqual( + resolveLaunch('C:\\Program Files\\Sublime Text\\sublime_text.exe', 'C:\\sites\\wp', { platform: 'win32' }), + { command: 'C:\\Program Files\\Sublime Text\\sublime_text.exe', args: ['C:\\sites\\wp'] } + ); + assert.deepEqual( + resolveLaunch('/usr/bin/code', '/home/dev/wp', { platform: 'linux' }), + { command: '/usr/bin/code', args: ['/home/dev/wp'] } + ); +}); + +// --- the guard ----------------------------------------------------------- + +const SITE = '/Users/dev/sites/wp'; +const EDITOR = '/Applications/Cursor.app'; + +function launchDeps(overrides = {}) { + const fs = fakeFs({ [EDITOR]: 'dir' }); + const { calls, spawn } = recordingSpawn(); + const refusals = []; + return { + calls, + refusals, + options: { + sites: [SITE], + platform: 'darwin', + statPath: fs.statPath, + spawn, + onRefused: (reason, description) => refusals.push({ reason, description }), + ...overrides + } + }; +} + +test('a registered site opens in the chosen editor', async () => { + const { calls, refusals, options } = launchDeps(); + + const result = await openSiteInEditor(SITE, EDITOR, options); + + assert.deepEqual(result, { ok: true }); + assert.deepEqual(refusals, []); + assert.equal(calls.length, 1); + assert.equal(calls[0].command, '/usr/bin/open'); + assert.deepEqual(calls[0].args, ['-a', EDITOR, SITE]); +}); + +// The same boundary `sites:delete` uses: the app's own record of what it created +// or adopted. "Open this site" must not become "open this arbitrary directory". +test('a path the registry does not hold is not opened', async () => { + const { calls, refusals, options } = launchDeps({ sites: ['/Users/dev/sites/other'] }); + + const result = await openSiteInEditor(SITE, EDITOR, options); + + assert.equal(result.ok, false); + assert.equal(result.reason, REFUSAL_REASONS.UNREGISTERED_SITE); + assert.deepEqual(calls, []); + assert.equal(refusals.length, 1); + assert.equal(refusals[0].reason, REFUSAL_REASONS.UNREGISTERED_SITE); +}); + +test('an editor that is gone, or was never an application, is not spawned', async () => { + for (const editorPath of ['/Applications/Uninstalled.app', 'code', '/Users/dev/notes.txt']) { + const { calls, refusals, options } = launchDeps(); + + const result = await openSiteInEditor(SITE, editorPath, options); + + assert.equal(result.ok, false, `${editorPath} should not launch`); + assert.equal(result.reason, REFUSAL_REASONS.UNLAUNCHABLE_EDITOR); + assert.deepEqual(calls, []); + assert.equal(refusals.length, 1); + } +}); + +test('a refusal is logged on one bounded line', async () => { + const { refusals, options } = launchDeps(); + + await openSiteInEditor(`/tmp/${'\n'.repeat(500)}`, EDITOR, options); + + const { description } = refusals[0]; + assert.ok(!description.includes('\n')); + assert.ok(description.length <= 121); +}); + +test('the child is detached, shell-free and hidden, and its handle released', async () => { + const { calls, options } = launchDeps(); + + await openSiteInEditor(SITE, EDITOR, options); + + assert.deepEqual(calls[0].options, { + detached: true, + stdio: 'ignore', + shell: false, + windowsHide: true + }); + assert.equal(calls[0].unrefed, true); +}); + +test('a spawn that throws is reported, not raised at the window', async () => { + const { options } = launchDeps({ + spawn: () => { throw new Error('ENOENT'); } + }); + + const result = await openSiteInEditor(SITE, EDITOR, options); + + assert.equal(result.ok, false); + assert.equal(result.reason, 'spawn-failed'); + assert.match(result.error, /ENOENT/); +}); diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 68f2620..30a1d5d 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -1170,6 +1170,145 @@ test('trac:fetch-attachment goes through trac-view', async () => { assert.deepEqual(result, { ok: true, text: 'DIFF' }); }); +// --- editor:open / dir:show -> src/editor-launch.js, src/site-registry.js - + +const SITE = '/Users/dev/sites/wp'; +const EDITOR = '/Applications/Cursor.app'; + +test('editor:open asks editor-launch to open the site, with the registry as its boundary', async () => { + const openSiteInEditor = spy(async () => ({ ok: true })); + const main = loadMain({ + stubs: { + ...silentLogging(), + ...fakeSettingsStore({ sites: [SITE], preferences: { editor: { path: EDITOR, name: 'Cursor' } } }).stubs, + './editor-launch': { openSiteInEditor } + } + }); + + assert.deepEqual(await main.invoke('editor:open', SITE), { ok: true }); + + assert.equal(openSiteInEditor.calls.length, 1); + const [sitePath, editorPath, options] = openSiteInEditor.calls[0]; + assert.equal(sitePath, SITE); + assert.equal(editorPath, EDITOR); + // Without these the module cannot decide anything: the registry is the + // boundary, `statPath` is how it checks the application is still there, and + // `spawn` is the effect it is being asked to guard. + assert.deepEqual(options.sites, [SITE]); + assert.equal(typeof options.statPath, 'function'); + assert.equal(typeof options.spawn, 'function'); + assert.equal(options.platform, process.platform); +}); + +// The end of the wire, with the real module in place. This is the assertion that +// fails if the handler ever spawns an editor itself. +test('editor:open does not spawn for a path the registry does not hold', async () => { + const cp = { spawn: spy(() => { throw new Error('a refused open must not reach spawn'); }) }; + const main = loadMain({ + stubs: { + ...silentLogging(), + ...fakeSettingsStore({ sites: [SITE], preferences: { editor: { path: EDITOR, name: 'Cursor' } } }).stubs, + child_process: cp + } + }); + + const result = await main.invoke('editor:open', '/Users/dev/somewhere-else'); + + assert.equal(result.ok, false); + assert.equal(result.reason, 'unregistered-site'); + assert.deepEqual(cp.spawn.calls, []); +}); + +test('editor:open with nothing chosen asks for a choice rather than guessing', async () => { + const openSiteInEditor = spy(async () => ({ ok: true })); + const main = loadMain({ + stubs: { ...silentLogging(), ...fakeSettingsStore({ sites: [SITE] }).stubs, './editor-launch': { openSiteInEditor } } + }); + + assert.deepEqual(await main.invoke('editor:open', SITE), { ok: false, reason: 'no-editor' }); + assert.deepEqual(openSiteInEditor.calls, []); +}); + +test('editor:choose validates what the dialog returned before remembering it', async () => { + const isLaunchableEditorPath = spy(() => false); + const main = loadMain({ + stubs: { ...silentLogging(), ...fakeSettingsStore().stubs, './editor-launch': { isLaunchableEditorPath } } + }); + + const result = await main.invoke('editor:choose', '/Users/dev/not-an-app'); + + assert.equal(result.ok, false); + assert.equal(result.reason, 'unlaunchable-editor'); + assert.equal(isLaunchableEditorPath.calls.length, 1); + assert.equal(isLaunchableEditorPath.calls[0][0], '/Users/dev/not-an-app'); +}); + +test('editor:list asks editor-launch what is installed, without a shell', async () => { + const detectEditors = spy(() => [{ id: 'vscode', name: 'Visual Studio Code', path: '/Applications/Visual Studio Code.app' }]); + const main = loadMain({ + stubs: { ...silentLogging(), ...fakeSettingsStore().stubs, './editor-launch': { detectEditors } } + }); + + const result = await main.invoke('editor:list'); + + assert.deepEqual(result.detected, [{ id: 'vscode', name: 'Visual Studio Code', path: '/Applications/Visual Studio Code.app' }]); + assert.equal(detectEditors.calls.length, 1); + // `exists` is the whole of detection — a handler that passed something else, + // or ran `which` beside this call, is what #24 was. + assert.equal(typeof detectEditors.calls[0][0].exists, 'function'); + assert.equal(detectEditors.calls[0][0].platform, process.platform); +}); + +// An editor chosen once and since uninstalled must come back as a question, not +// as a button that fails on click. +test('editor:list reports a remembered editor that is no longer there', async () => { + const main = loadMain({ + stubs: { + ...silentLogging(), + ...fakeSettingsStore({ preferences: { editor: { path: '/Applications/Gone.app', name: 'Gone' } } }).stubs, + './editor-launch': { detectEditors: () => [], isLaunchableEditorPath: () => false } + } + }); + + const result = await main.invoke('editor:list'); + + assert.equal(result.chosen, null); + assert.equal(result.chosenMissing, true); +}); + +test('dir:show asks site-registry whether the path may be revealed', async () => { + const revealRegisteredSite = spy(async () => ({ ok: true })); + const main = loadMain({ + stubs: { ...silentLogging(), ...fakeSettingsStore({ sites: [SITE] }).stubs, './site-registry': { revealRegisteredSite } } + }); + + await main.invoke('dir:show', SITE); + + assert.equal(revealRegisteredSite.calls.length, 1); + const [sitePath, options] = revealRegisteredSite.calls[0]; + assert.equal(sitePath, SITE); + assert.deepEqual(options.sites, [SITE]); + assert.equal(typeof options.reveal, 'function'); + assert.equal(typeof options.onRefused, 'function'); +}); + +test('dir:show refuses a path the registry does not hold, and logs it', async () => { + const logEvent = spy(); + const main = loadMain({ + stubs: { './logging': { ...silentLogging()['./logging'], logEvent }, ...fakeSettingsStore({ sites: [SITE] }).stubs } + }); + + const result = await main.invoke('dir:show', '/Users/dev/somewhere-else'); + + assert.equal(result.ok, false); + assert.deepEqual(main.calls.openPath, []); + assert.equal(logEvent.calls.length, 1); + assert.match(logEvent.calls[0][1], /refused to reveal \/Users\/dev\/somewhere-else/); + + assert.deepEqual(await main.invoke('dir:show', SITE), { ok: true }); + assert.deepEqual(main.calls.openPath, [SITE]); +}); + // --- the harness's own guard --------------------------------------------- // Requiring the real `electron` package is not a harmless fallback: its @@ -1223,7 +1362,11 @@ const WIRED = new Set([ 'git:apply-patch', 'git:fetch-pr-diff', 'git:list-ticket-patches', - 'trac:fetch-attachment' + 'trac:fetch-attachment', + 'editor:list', + 'editor:choose', + 'editor:open', + 'dir:show' ]); // Channels with no module to reach: they read or write electron-store, drive a diff --git a/test/site-registry.test.cjs b/test/site-registry.test.cjs index a7a3536..3db73a7 100644 --- a/test/site-registry.test.cjs +++ b/test/site-registry.test.cjs @@ -1,7 +1,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); -const { isRegisteredSite, describeRefusedSite, deleteRegisteredSite } = require('../src/site-registry.js'); +const { isRegisteredSite, describeRefusedSite, deleteRegisteredSite, revealRegisteredSite } = require('../src/site-registry.js'); // A couple of paths the app might actually hold in its registry, one per platform // shape, so the tests aren't accidentally tied to POSIX separators. @@ -112,3 +112,52 @@ test('truncation is applied to the escaped form', () => { assert.ok(description.length <= 121, `escaped description was ${description.length} characters`); assert.ok(!description.includes('\n')); }); + +// --- revealRegisteredSite ------------------------------------------------ +// +// `shell.openPath` is a smaller action than `fse.remove`, but it is the same +// kind of action — a local path handed to the OS — so it gets the same boundary. + +function revealRecorder(sites = REGISTERED, error = '') { + const revealed = []; + const refused = []; + return { + revealed, + refused, + options: { + sites, + reveal: async (p) => { revealed.push(p); return error; }, + onRefused: (description) => { refused.push(description); } + } + }; +} + +test('a registered site is revealed', async () => { + const rec = revealRecorder(); + + assert.deepEqual(await revealRegisteredSite('/Users/dev/sites/my-site', rec.options), { ok: true }); + + assert.deepEqual(rec.revealed, ['/Users/dev/sites/my-site']); + assert.deepEqual(rec.refused, []); +}); + +test('a path the registry does not hold is not revealed', async () => { + const rec = revealRecorder(); + + const result = await revealRegisteredSite('/Users/dev/somewhere-else', rec.options); + + assert.equal(result.ok, false); + assert.equal(result.reason, 'unregistered-site'); + assert.deepEqual(rec.revealed, []); + assert.deepEqual(rec.refused, ['/Users/dev/somewhere-else']); +}); + +test('a reveal the OS declines is reported rather than swallowed', async () => { + const rec = revealRecorder(REGISTERED, 'Failed to open path'); + + const result = await revealRegisteredSite('/Users/dev/sites/my-site', rec.options); + + assert.equal(result.ok, false); + assert.equal(result.reason, 'open-failed'); + assert.equal(result.error, 'Failed to open path'); +}); From 11b46c14ff739a0014a6cf6ae0e51dc6f68d2ef6 Mon Sep 17 00:00:00 2001 From: JuanMa Date: Fri, 7 Aug 2026 07:26:52 +0200 Subject: [PATCH 3/5] Offer "Open in editor" and "Show in Finder" on a site MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/preload.js | 4 ++ src/renderer/index.jsx | 127 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 131 insertions(+) diff --git a/src/preload.js b/src/preload.js index fba23c3..568b531 100644 --- a/src/preload.js +++ b/src/preload.js @@ -1,6 +1,10 @@ const { contextBridge, ipcRenderer } = require('electron'); contextBridge.exposeInMainWorld('api', { + // Only so the window can name things the way the platform does — "Show in + // Finder" against "Show in Explorer". Nothing branches on it in the main + // process, where `process.platform` is read directly. + platform: process.platform, getSites: () => ipcRenderer.invoke('sites:get'), getSitesWithMeta: () => ipcRenderer.invoke('sites:getAll'), addSite: (dir) => ipcRenderer.invoke('sites:add', dir), diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index a63305a..a873e49 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -39,6 +39,9 @@ const UPDATE_STEP_MARKS = { complete: { symbol: '✓', color: '#0f5132' }, current: { symbol: '›', color: '#0b5d95' } }; +// The file manager has a name on the two platforms that have one; everywhere +// else it is whatever the desktop provides, so it is called what it is. +const FILE_MANAGER_LABELS = { darwin: 'Show in Finder', win32: 'Show in Explorer' }; const TERMINAL_INSTALL_ALIASES = ['npm install', 'npm i', 'install']; const RENAME_INPUT_ID = 'rename-site-name-input'; const CREATE_SITE_NAME_INPUT_ID = 'create-site-name-input'; @@ -926,6 +929,83 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit } }, [sitePath]); + // --- opening the code --------------------------------------------------- + // + // The editor is remembered app-wide, so this is "ask once" rather than a + // choice per site. Every path out of here ends somewhere the contributor can + // act: a launch that fails opens the picker, the picker always offers + // "Choose application…", and the copy button above is the floor under all of + // it. + const [chosenEditor, setChosenEditor] = useState(null); + const [detectedEditors, setDetectedEditors] = useState([]); + const [editorPickerOpen, setEditorPickerOpen] = useState(false); + const [editorNotice, setEditorNotice] = useState(''); + + const fileManagerLabel = FILE_MANAGER_LABELS[window.api?.platform] || 'Show in file manager'; + + const refreshEditors = useCallback(async () => { + try { + const result = await window.api.listEditors(); + setDetectedEditors(result?.detected || []); + setChosenEditor(result?.chosen || null); + return result; + } catch { + setDetectedEditors([]); + return null; + } + }, []); + + // So the button can say "Open in Cursor" on the first render rather than after + // the first click. + useEffect(() => { refreshEditors(); }, [refreshEditors]); + + const describeOpenFailure = useCallback((result) => { + if (result?.reason === 'unlaunchable-editor') { + return 'That editor is no longer where it was. Choose it again.'; + } + if (result?.reason === 'spawn-failed') { + return `The editor would not start: ${result.error || 'unknown error'}`; + } + if (result?.reason === 'unregistered-site') { + return 'This app has no record of that folder, so it will not open it.'; + } + return 'Could not open the folder in an editor.'; + }, []); + + 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 refreshEditors(); + setEditorPickerOpen(true); + }, [describeOpenFailure, refreshEditors, sitePath]); + + // `editorPath` is a detected editor; without one the main process opens the + // file dialog, which is what covers every editor the detection table misses. + const rememberEditor = useCallback(async (editorPath) => { + const result = await window.api.chooseEditor(editorPath); + if (!result?.ok) { + if (result?.reason === 'unlaunchable-editor') { + setEditorNotice('That is not an application this app can open a folder in.'); + } + return; + } + setChosenEditor(result.editor); + setEditorPickerOpen(false); + const opened = await window.api.openInEditor(sitePath); + setEditorNotice(opened?.ok ? '' : describeOpenFailure(opened)); + }, [describeOpenFailure, sitePath]); + + const showInFileManager = useCallback(async () => { + const result = await window.api.showSiteInFileManager(sitePath); + setEditorNotice(result?.ok ? '' : `Could not open the folder: ${result?.error || 'unknown error'}`); + }, [sitePath]); + const appendNpm = useCallback((s)=>setNpmLogs((v)=>v+s),[]); const appendRuntime = useCallback((s)=>setRuntimeLogs((v)=>v + String(s ?? '')),[]); const sortEmails = useCallback((list)=>[...list].sort((a,b)=>new Date(b.sentAt||b.date||0)-new Date(a.sentAt||a.date||0)),[]); @@ -2124,6 +2204,25 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit isSmall /> +
+ + + {chosenEditor ? ( + + ) : null} +
+ {editorNotice ? ( +
+ {editorNotice} + +
+ ) : null}
+ {editorPickerOpen ? ( + setEditorPickerOpen(false)} + > +
+

+ {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.'} +

+ {detectedEditors.map((candidate) => ( + + ))} + {/* Always offered, never only as a fallback: the detection list is a + shortcut, and an editor missing from it is not an editor this app + refuses to use. */} + +
+ + ) : null} {dirtyModalOpen ? ( Date: Fri, 7 Aug 2026 07:37:16 +0200 Subject: [PATCH 4/5] Answer a launch from the child's events, and detect editors once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/editor-launch.js | 57 ++++++++++++++++++++++-- src/main.js | 35 +++++++-------- src/preload.js | 2 + src/renderer/index.jsx | 89 ++++++++++++++++++++++++------------- test/editor-launch.test.cjs | 79 +++++++++++++++++++++++++++++--- test/ipc-wiring.test.cjs | 37 ++++++++------- 6 files changed, 222 insertions(+), 77 deletions(-) diff --git a/src/editor-launch.js b/src/editor-launch.js index 9cbfd13..ca8e753 100644 --- a/src/editor-launch.js +++ b/src/editor-launch.js @@ -245,18 +245,69 @@ async function openSiteInEditor(sitePath, editorPath, { const { command, args } = resolveLaunch(editorPath, sitePath, { platform }); + let child; try { - const child = spawn(command, args, { + child = spawn(command, args, { detached: true, stdio: 'ignore', shell: false, windowsHide: true }); - if (child && typeof child.unref === 'function') child.unref(); - return { ok: true }; } catch (e) { + // A synchronous throw is the argument-shape failure only. The one that + // actually happens — the target cannot be executed — arrives as an event. return { ok: false, reason: 'spawn-failed', error: e?.message ?? String(e) }; } + + return awaitLaunch(child, { platform }); +} + +// Whether the launch worked, answered from the child's own events rather than +// from `spawn` having returned. +// +// `spawn` returns a ChildProcess before the OS has been asked to execute +// anything, so a target that cannot be run — EACCES on a file that is not +// executable, EPERM from a Windows policy, or a path deleted between the check +// and the launch — fails afterwards, on the 'error' event. Returning `{ ok: true }` +// at that point is the "the button did nothing" failure this project treats as +// an architectural bug: the contributor cannot debug it, and with no listener the +// emit becomes an uncaught exception that only reaches the log file. +// +// The two platforms need different evidence: +// +// - macOS spawns `/usr/bin/open`, which is not the editor. It exits as soon as +// it has asked Launch Services to open the bundle, so its exit code is the +// answer and waiting for it costs milliseconds. +// - Everywhere else the child *is* the editor and stays alive, so the answer is +// 'spawn' — Node's "the OS accepted this" event — and waiting for exit would +// mean waiting for the contributor to close their editor. +// +// The handle is released either way, so the editor outlives the app. +function awaitLaunch(child, { platform } = {}) { + return new Promise((resolve) => { + let settled = false; + const settle = (result) => { + if (settled) return; + settled = true; + if (typeof child.unref === 'function') child.unref(); + resolve(result); + }; + + child.on('error', (e) => { + settle({ ok: false, reason: 'spawn-failed', error: e?.message ?? String(e) }); + }); + + if (platform === 'darwin') { + child.on('close', (code) => { + settle(code === 0 + ? { ok: true } + : { ok: false, reason: 'spawn-failed', error: `the editor could not be opened (exit code ${code})` }); + }); + return; + } + + child.on('spawn', () => settle({ ok: true })); + }); } module.exports = { diff --git a/src/main.js b/src/main.js index 2ff8b38..4c2a53e 100644 --- a/src/main.js +++ b/src/main.js @@ -983,24 +983,23 @@ async function getChosenEditor() { return chosen && typeof chosen.path === 'string' ? chosen : null; } -// The editors on this machine, plus the remembered choice. `chosenMissing` is -// the case worth naming: an editor that was chosen once and has since been -// uninstalled or moved. The renderer uses it to ask again rather than to fail on -// the next click. -ipcMain.handle('editor:list', async () => { - const chosen = await getChosenEditor(); - const stillThere = chosen ? isLaunchableEditorPath(chosen.path, editorLaunchDeps()) : false; - - return { - detected: detectEditors({ - platform: process.platform, - env: process.env, - exists: (p) => statPathSync(p) !== null - }), - chosen: stillThere ? chosen : null, - chosenMissing: Boolean(chosen) && !stillThere - }; -}); +// The remembered choice, and nothing else. This is what the window asks for on +// load — just enough to name the button "Open in Cursor" — so it touches no +// filesystem at all. Whether that editor is still installed is answered by +// trying to open it, which is a question the contributor has just asked anyway. +ipcMain.handle('editor:get', async () => getChosenEditor()); + +// The editors on this machine. Detection stats a dozen or so absolute locations, +// so it runs when the contributor opens the picker rather than on every load — +// `editor:get` is the cheap one. +ipcMain.handle('editor:list', async () => ({ + detected: detectEditors({ + platform: process.platform, + env: process.env, + exists: (p) => statPathSync(p) !== null + }), + chosen: await getChosenEditor() +})); // Remembers an editor. With a path it is the one the contributor picked from the // detected list; without one it opens the file dialog, which is the answer for diff --git a/src/preload.js b/src/preload.js index 568b531..52504fb 100644 --- a/src/preload.js +++ b/src/preload.js @@ -46,6 +46,8 @@ contextBridge.exposeInMainWorld('api', { npmKill: (params) => ipcRenderer.invoke('npm:kill', params) , openExternal: (url) => ipcRenderer.invoke('url:open', url) +, + getEditor: () => ipcRenderer.invoke('editor:get') , listEditors: () => ipcRenderer.invoke('editor:list') , diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index a873e49..5796d03 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -63,6 +63,48 @@ function formatEmailDate(email) { } const FEEDBACK_FORM_URL = 'https://docs.google.com/forms/d/e/1FAIpQLScnMxicyDxZO2OoaS5ela8FArYWjCyLfC3hxRBBRSF7XLPzKg/viewform'; +// The editor is one app-wide choice, so it is held once for the window rather +// than once per site. Every site is mounted at all times (the inactive ones are +// hidden), so per-row state here would mean N copies of the same answer: N loads +// on startup, and a choice made in one row leaving every other row's button +// still saying "Open in editor". +// +// Detection is deliberately not part of the load: `editor:get` is a store read, +// and the filesystem probe behind `editor:list` waits until the picker is +// actually opened. +function useEditorChoice() { + const [chosen, setChosen] = useState(null); + const [detected, setDetected] = useState([]); + + useEffect(() => { + let cancelled = false; + window.api.getEditor() + .then((editor) => { if (!cancelled) setChosen(editor || null); }) + .catch(() => {}); + return () => { cancelled = true; }; + }, []); + + const loadDetected = useCallback(async () => { + try { + const result = await window.api.listEditors(); + setDetected(result?.detected || []); + setChosen(result?.chosen || null); + } catch { + setDetected([]); + } + }, []); + + // `editorPath` is one of the detected editors; without one the main process + // opens the file dialog, which is what covers every editor detection misses. + const remember = useCallback(async (editorPath) => { + const result = await window.api.chooseEditor(editorPath); + if (result?.ok) setChosen(result.editor); + return result; + }, []); + + return { chosen, detected, loadDetected, remember }; +} + function useSites() { const [sites, setSites] = useState([]); const [siteMeta, setSiteMeta] = useState({}); @@ -77,6 +119,8 @@ function useSites() { function App() { const { sites, siteMeta, refresh, setSiteMeta, setSites } = useSites(); + // One choice for the window, shared by every site row. + const editorChoice = useEditorChoice(); const [downloadPhase, setDownloadPhase] = useState(''); // Directories whose clone is still running. An array rather than a single // path because the main process may settle on a different (deduplicated) @@ -680,6 +724,7 @@ function App() { onForget={onForget} onDelete={onDelete} onRename={onRename} + editor={editorChoice} isPending={pendingSites.includes(s)} setupLogs={setupLogsBySite[s] || ''} isActive={activeSite === s} @@ -762,7 +807,7 @@ function App() { ); } -function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSiteMetaPatch, onForget, onDelete, onRename, isPending = false, setupLogs = '', isActive = false }) { +function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSiteMetaPatch, onForget, onDelete, onRename, editor, isPending = false, setupLogs = '', isActive = false }) { // Kept in a ref so loadStatus's dependency list stays [sitePath] — a // recreated callback prop must not retrigger the status-loading effect. const metaPatchRef = useRef(onSiteMetaPatch); @@ -931,34 +976,17 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // --- opening the code --------------------------------------------------- // - // The editor is remembered app-wide, so this is "ask once" rather than a - // choice per site. Every path out of here ends somewhere the contributor can - // act: a launch that fails opens the picker, the picker always offers - // "Choose application…", and the copy button above is the floor under all of - // it. - const [chosenEditor, setChosenEditor] = useState(null); - const [detectedEditors, setDetectedEditors] = useState([]); + // The editor itself is chosen once for the whole window (see useEditorChoice); + // what is per-site here is only the UI around it. Every path out of this ends + // somewhere the contributor can act: a launch that fails opens the picker, the + // picker always offers "Choose application…", and the copy button above is the + // floor under all of it. + const { chosen: chosenEditor, detected: detectedEditors, loadDetected, remember } = editor; const [editorPickerOpen, setEditorPickerOpen] = useState(false); const [editorNotice, setEditorNotice] = useState(''); const fileManagerLabel = FILE_MANAGER_LABELS[window.api?.platform] || 'Show in file manager'; - const refreshEditors = useCallback(async () => { - try { - const result = await window.api.listEditors(); - setDetectedEditors(result?.detected || []); - setChosenEditor(result?.chosen || null); - return result; - } catch { - setDetectedEditors([]); - return null; - } - }, []); - - // So the button can say "Open in Cursor" on the first render rather than after - // the first click. - useEffect(() => { refreshEditors(); }, [refreshEditors]); - const describeOpenFailure = useCallback((result) => { if (result?.reason === 'unlaunchable-editor') { return 'That editor is no longer where it was. Choose it again.'; @@ -981,25 +1009,22 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // 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 refreshEditors(); + await loadDetected(); setEditorPickerOpen(true); - }, [describeOpenFailure, refreshEditors, sitePath]); + }, [describeOpenFailure, loadDetected, sitePath]); - // `editorPath` is a detected editor; without one the main process opens the - // file dialog, which is what covers every editor the detection table misses. const rememberEditor = useCallback(async (editorPath) => { - const result = await window.api.chooseEditor(editorPath); + const result = await remember(editorPath); if (!result?.ok) { if (result?.reason === 'unlaunchable-editor') { setEditorNotice('That is not an application this app can open a folder in.'); } return; } - setChosenEditor(result.editor); setEditorPickerOpen(false); const opened = await window.api.openInEditor(sitePath); setEditorNotice(opened?.ok ? '' : describeOpenFailure(opened)); - }, [describeOpenFailure, sitePath]); + }, [describeOpenFailure, remember, sitePath]); const showInFileManager = useCallback(async () => { const result = await window.api.showSiteInFileManager(sitePath); @@ -2213,7 +2238,7 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit ) : null} diff --git a/test/editor-launch.test.cjs b/test/editor-launch.test.cjs index 7a4d7f5..f9cf2fa 100644 --- a/test/editor-launch.test.cjs +++ b/test/editor-launch.test.cjs @@ -13,6 +13,7 @@ const test = require('node:test'); const assert = require('node:assert/strict'); +const { EventEmitter } = require('node:events'); const { REFUSAL_REASONS, @@ -44,11 +45,29 @@ function fakeFs(entries) { }; } -function recordingSpawn() { +// A stand-in for child_process.spawn that behaves like the real one in the way +// that matters here: it returns a handle first and reports success or failure +// afterwards, on an event. `outcome` says which event, and it is emitted on a +// later turn — a fake that emitted synchronously would pass whether or not the +// code under test ever attached a listener. +function recordingSpawn(outcome = { event: 'ok' }) { const calls = []; const spawn = (command, args, options) => { - calls.push({ command, args, options }); - return { unref() { calls[calls.length - 1].unrefed = true; } }; + const call = { command, args, options, unrefed: false }; + calls.push(call); + const child = new EventEmitter(); + child.unref = () => { call.unrefed = true; }; + setImmediate(() => { + if (outcome.event === 'error') { + child.emit('error', outcome.error || new Error('EACCES: permission denied')); + return; + } + // What a working launch looks like on both shapes: the OS accepted the + // command, and on macOS `open` then exits with a code. + child.emit('spawn'); + child.emit('close', outcome.code ?? 0); + }); + return child; }; return { calls, spawn }; } @@ -204,9 +223,9 @@ test('elsewhere the executable takes the folder as an argument', () => { const SITE = '/Users/dev/sites/wp'; const EDITOR = '/Applications/Cursor.app'; -function launchDeps(overrides = {}) { +function launchDeps(overrides = {}, outcome = { event: 'ok' }) { const fs = fakeFs({ [EDITOR]: 'dir' }); - const { calls, spawn } = recordingSpawn(); + const { calls, spawn } = recordingSpawn(outcome); const refusals = []; return { calls, @@ -287,12 +306,58 @@ test('the child is detached, shell-free and hidden, and its handle released', as test('a spawn that throws is reported, not raised at the window', async () => { const { options } = launchDeps({ - spawn: () => { throw new Error('ENOENT'); } + spawn: () => { throw new TypeError('args must be an array'); } }); const result = await openSiteInEditor(SITE, EDITOR, options); assert.equal(result.ok, false); assert.equal(result.reason, 'spawn-failed'); - assert.match(result.error, /ENOENT/); + assert.match(result.error, /args must be an array/); +}); + +// The failure that actually happens. `spawn` returns a handle before the OS has +// been asked to execute anything, so a target that cannot be run reports it +// afterwards — and a caller that answered "ok" on the return value has already +// told the contributor their editor is opening. +test('a launch that fails after spawn returns is still a failure', async () => { + const { options } = launchDeps({}, { event: 'error', error: new Error('spawn EACCES') }); + + const result = await openSiteInEditor(SITE, EDITOR, options); + + assert.equal(result.ok, false); + assert.equal(result.reason, 'spawn-failed'); + assert.match(result.error, /EACCES/); +}); + +// On macOS the child is `/usr/bin/open`, not the editor: it exits as soon as +// Launch Services has been asked, so its exit code is the answer. +test('macOS reports what `open` exited with', async () => { + const failed = await openSiteInEditor(SITE, EDITOR, launchDeps({}, { event: 'ok', code: 1 }).options); + assert.equal(failed.ok, false); + assert.equal(failed.reason, 'spawn-failed'); + assert.match(failed.error, /exit code 1/); + + const succeeded = await openSiteInEditor(SITE, EDITOR, launchDeps({}, { event: 'ok', code: 0 }).options); + assert.deepEqual(succeeded, { ok: true }); +}); + +// Elsewhere the child is the editor and stays alive, so waiting for it to exit +// would mean waiting for the contributor to close it. +test('a long-lived editor answers as soon as the OS accepts it', async () => { + const fs = fakeFs({ 'C:\\Program Files\\Sublime Text\\sublime_text.exe': 'file' }); + // Exit code 1 as well, to pin that it is not being waited on: an editor that + // is still open has no exit code at all, and one that eventually exits + // non-zero must not turn a launch that worked into a failure. + const { calls, spawn } = recordingSpawn({ event: 'ok', code: 1 }); + + const result = await openSiteInEditor('C:\\sites\\wp', 'C:\\Program Files\\Sublime Text\\sublime_text.exe', { + sites: ['C:\\sites\\wp'], + platform: 'win32', + statPath: fs.statPath, + spawn + }); + + assert.deepEqual(result, { ok: true }); + assert.equal(calls[0].unrefed, true); }); diff --git a/test/ipc-wiring.test.cjs b/test/ipc-wiring.test.cjs index 30a1d5d..8a8bad0 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -1243,6 +1243,25 @@ test('editor:choose validates what the dialog returned before remembering it', a assert.equal(isLaunchableEditorPath.calls[0][0], '/Users/dev/not-an-app'); }); +// The window asks this on load, for every site it has. It must not turn into a +// filesystem sweep: detection belongs to `editor:list`, which runs when the +// picker opens. +test('editor:get reads the remembered choice and touches nothing else', async () => { + const detectEditors = spy(() => []); + const isLaunchableEditorPath = spy(() => true); + const main = loadMain({ + stubs: { + ...silentLogging(), + ...fakeSettingsStore({ preferences: { editor: { path: EDITOR, name: 'Cursor' } } }).stubs, + './editor-launch': { detectEditors, isLaunchableEditorPath } + } + }); + + assert.deepEqual(await main.invoke('editor:get'), { path: EDITOR, name: 'Cursor' }); + assert.deepEqual(detectEditors.calls, []); + assert.deepEqual(isLaunchableEditorPath.calls, []); +}); + test('editor:list asks editor-launch what is installed, without a shell', async () => { const detectEditors = spy(() => [{ id: 'vscode', name: 'Visual Studio Code', path: '/Applications/Visual Studio Code.app' }]); const main = loadMain({ @@ -1259,23 +1278,6 @@ test('editor:list asks editor-launch what is installed, without a shell', async assert.equal(detectEditors.calls[0][0].platform, process.platform); }); -// An editor chosen once and since uninstalled must come back as a question, not -// as a button that fails on click. -test('editor:list reports a remembered editor that is no longer there', async () => { - const main = loadMain({ - stubs: { - ...silentLogging(), - ...fakeSettingsStore({ preferences: { editor: { path: '/Applications/Gone.app', name: 'Gone' } } }).stubs, - './editor-launch': { detectEditors: () => [], isLaunchableEditorPath: () => false } - } - }); - - const result = await main.invoke('editor:list'); - - assert.equal(result.chosen, null); - assert.equal(result.chosenMissing, true); -}); - test('dir:show asks site-registry whether the path may be revealed', async () => { const revealRegisteredSite = spy(async () => ({ ok: true })); const main = loadMain({ @@ -1363,6 +1365,7 @@ const WIRED = new Set([ 'git:fetch-pr-diff', 'git:list-ticket-patches', 'trac:fetch-attachment', + 'editor:get', 'editor:list', 'editor:choose', 'editor:open', From 1cdfdec957e132dfde879e8bff9eb5306acbb2ce Mon Sep 17 00:00:00 2001 From: JuanMa Date: Fri, 7 Aug 2026 09:24:46 +0200 Subject: [PATCH 5/5] Stop stat-ing synchronously, and never leave the button silent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- src/editor-launch.js | 91 ++++++++++++++++++++------- src/main.js | 37 ++++++++--- src/renderer/index.jsx | 78 ++++++++++++++++++++--- test/editor-launch.test.cjs | 121 ++++++++++++++++++++++++++---------- 4 files changed, 252 insertions(+), 75 deletions(-) diff --git a/src/editor-launch.js b/src/editor-launch.js index ca8e753..4683f74 100644 --- a/src/editor-launch.js +++ b/src/editor-launch.js @@ -139,43 +139,87 @@ function editorCandidates({ platform, env = {} } = {}) { // The editors this machine has, in table order, each reduced to the first // location that exists. `exists` is injected — it is the only thing detection // does, and the only thing a test has to stand in for. -function detectEditors({ platform, env = {}, exists } = {}) { +// +// It is awaited rather than called for a return value, because the caller is +// Electron's main process: a dozen or so probes of locations that mostly do not +// exist is exactly the kind of work that reads as free on the author's machine +// and stalls the whole window behind a Windows antivirus filter driver. Nothing +// here may be synchronous filesystem access. +// +// Candidates are probed together rather than one after another: they are +// independent questions, and the answer arrives in one round rather than a dozen. +async function detectEditors({ platform, env = {}, exists } = {}) { if (typeof exists !== 'function') return []; - return editorCandidates({ platform, env }) - .map(({ id, name, paths }) => { - const found = paths.find((candidate) => { - try { - return exists(candidate) === true; - } catch { - // An unreadable location is a location we do not have, not a crash - // on the way to drawing a button. - return false; - } - }); - return found ? { id, name, path: found } : null; - }) - .filter(Boolean); + const probe = async (candidate) => { + try { + return (await exists(candidate)) === true; + } catch { + // An unreadable location is a location we do not have, not a crash on + // the way to drawing a button. + return false; + } + }; + + const found = await Promise.all(editorCandidates({ platform, env }).map(async ({ id, name, paths }) => { + const present = await Promise.all(paths.map(probe)); + const index = present.indexOf(true); + return index === -1 ? null : { id, name, path: paths[index] }; + })); + + return found.filter(Boolean); +} + +// The name the table has for an application at this path, or null for one it +// does not know. +// +// Without this the stored name is the basename, which on macOS reads well by +// accident ('Sublime Text.app' → 'Sublime Text') and on Windows does not: +// 'Code.exe' → 'Code', 'phpstorm64.exe' → 'phpstorm64'. The button promises to +// name the contributor's editor, so a known one is named the way the picker +// named it, and only a manually chosen unknown application falls back to its +// filename. +// +// The comparison is case-insensitive on Windows and macOS because their default +// filesystems are: the same application reached through a differently-cased path +// is the same application. +function knownEditorName(editorPath, { platform, env = {} } = {}) { + if (typeof editorPath !== 'string' || editorPath === '') return null; + + const insensitive = platform === 'win32' || platform === 'darwin'; + const normalize = (p) => (insensitive ? p.toLowerCase() : p); + const wanted = normalize(editorPath); + + const match = editorCandidates({ platform, env }) + .find(({ paths }) => paths.some((candidate) => normalize(candidate) === wanted)); + + return match ? match.name : null; } // Whether a path is something this app will hand to the OS as an application. // // Absolute, because a relative command would be resolved through PATH by spawn, // and of the shape the platform uses for an application: a `.app` bundle -// (a directory) on macOS, an `.exe` on Windows, a regular file elsewhere. The -// same check covers both a detected path and one the contributor picked — the +// (a directory) on macOS, an `.exe` on Windows, and elsewhere a file the OS will +// actually execute. That last one is not the same as "a regular file": the Linux +// picker cannot filter by extension, so a document passes every other check and +// then fails with EACCES at the spawn, after being remembered as the +// contributor's editor. +// +// The same check covers a detected path and one the contributor picked — the // picker is a dialog, and a dialog's result is still input. // -// `statPath` returns `{ isDirectory, isFile }` or null when there is nothing -// there; it is injected for the same reason `exists` is. -function isLaunchableEditorPath(editorPath, { platform, statPath } = {}) { +// `statPath` resolves to `{ isDirectory, isFile, isExecutable }`, or null when +// there is nothing there; it is injected and awaited for the same reasons +// `exists` is. +async function isLaunchableEditorPath(editorPath, { platform, statPath } = {}) { if (typeof editorPath !== 'string' || editorPath === '') return false; if (typeof statPath !== 'function') return false; if (!pathApi(platform).isAbsolute(editorPath)) return false; let stats; try { - stats = statPath(editorPath); + stats = await statPath(editorPath); } catch { return false; } @@ -187,7 +231,7 @@ function isLaunchableEditorPath(editorPath, { platform, statPath } = {}) { if (platform === 'win32') { return stats.isFile === true && editorPath.toLowerCase().endsWith('.exe'); } - return stats.isFile === true; + return stats.isFile === true && stats.isExecutable === true; } // What to run, as a command and an argument vector — never a string to be @@ -236,7 +280,7 @@ async function openSiteInEditor(sitePath, editorPath, { return { ok: false, reason: REFUSAL_REASONS.UNREGISTERED_SITE }; } - if (!isLaunchableEditorPath(editorPath, { platform, statPath })) { + if (!await isLaunchableEditorPath(editorPath, { platform, statPath })) { if (typeof onRefused === 'function') { onRefused(REFUSAL_REASONS.UNLAUNCHABLE_EDITOR, describeRefused(editorPath)); } @@ -314,6 +358,7 @@ module.exports = { REFUSAL_REASONS, editorCandidates, detectEditors, + knownEditorName, isLaunchableEditorPath, resolveLaunch, openSiteInEditor diff --git a/src/main.js b/src/main.js index 4c2a53e..2d6f050 100644 --- a/src/main.js +++ b/src/main.js @@ -38,7 +38,7 @@ const { deleteRegisteredSite, revealRegisteredSite } = require('./site-registry' const { getStore } = require('./settings-store'); const { parseTicketRef } = require('./renderer/trac-ticket.cjs'); const { describeRefused } = require('./safe-log'); -const { detectEditors, isLaunchableEditorPath, openSiteInEditor } = require('./editor-launch'); +const { detectEditors, knownEditorName, isLaunchableEditorPath, openSiteInEditor } = require('./editor-launch'); const WORDPRESS_GIT_URL = 'https://github.com/WordPress/wordpress-develop.git'; @@ -966,16 +966,32 @@ ipcMain.handle('url:open', async (_e, url) => openExternalUrl(url, { // picked, or remembered from a previous run — goes through the same check before // anything is spawned. -function statPathSync(targetPath) { +// Asynchronous on purpose: this runs on the process that draws the window, and +// probing a dozen locations that mostly do not exist is exactly what a slow +// volume or an antivirus filter driver turns into a frozen UI. +// +// Executability is asked of the OS with `access(X_OK)` rather than read off the +// mode bits, so the answer is about the user this app is running as, ACLs and +// mount options included. On Windows every file answers X_OK, which is why the +// `.exe` check there is the one that matters. +async function statPath(targetPath) { + let stats; try { - const stats = fs.statSync(targetPath); - return { isDirectory: stats.isDirectory(), isFile: stats.isFile() }; + stats = await fs.promises.stat(targetPath); } catch { return null; } + + let isExecutable = false; + try { + await fs.promises.access(targetPath, fs.constants.X_OK); + isExecutable = true; + } catch {} + + return { isDirectory: stats.isDirectory(), isFile: stats.isFile(), isExecutable }; } -const editorLaunchDeps = () => ({ platform: process.platform, statPath: statPathSync }); +const editorLaunchDeps = () => ({ platform: process.platform, statPath }); async function getChosenEditor() { const s = await getStore(); @@ -993,10 +1009,10 @@ ipcMain.handle('editor:get', async () => getChosenEditor()); // so it runs when the contributor opens the picker rather than on every load — // `editor:get` is the cheap one. ipcMain.handle('editor:list', async () => ({ - detected: detectEditors({ + detected: await detectEditors({ platform: process.platform, env: process.env, - exists: (p) => statPathSync(p) !== null + exists: async (p) => (await statPath(p)) !== null }), chosen: await getChosenEditor() })); @@ -1029,7 +1045,7 @@ ipcMain.handle('editor:choose', async (_e, editorPath) => { target = result.filePaths[0]; } - if (!isLaunchableEditorPath(target, editorLaunchDeps())) { + if (!await isLaunchableEditorPath(target, editorLaunchDeps())) { logEvent('editor', `refused to remember ${describeRefused(target)} — not an application this app can launch`); return { ok: false, reason: 'unlaunchable-editor' }; } @@ -1037,7 +1053,10 @@ ipcMain.handle('editor:choose', async (_e, editorPath) => { const s = await getStore(); const editor = { path: target, - name: path.basename(target, path.extname(target)) + // The table's name for a known application; a filename only for one the + // contributor pointed at that the table has never heard of. + name: knownEditorName(target, { platform: process.platform, env: process.env }) + || path.basename(target, path.extname(target)) }; s.set('preferences', { ...(s.get('preferences') || {}), editor }); return { ok: true, editor }; diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index 5796d03..b48260e 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -80,7 +80,11 @@ function useEditorChoice() { let cancelled = false; window.api.getEditor() .then((editor) => { if (!cancelled) setChosen(editor || null); }) - .catch(() => {}); + // The window carries on with no remembered editor, which is a state it + // handles — but "the store could not be read" and "nothing chosen yet" + // must not be the same event to whoever reads the log afterwards. + // eslint-disable-next-line no-console -- reaches the log file: logging.js initializes electron-log with spyRendererConsole, so this is how the renderer records a diagnostic. + .catch((err) => console.error('Could not read the remembered editor:', err)); return () => { cancelled = true; }; }, []); @@ -89,15 +93,28 @@ function useEditorChoice() { const result = await window.api.listEditors(); setDetected(result?.detected || []); setChosen(result?.chosen || null); - } catch { + } catch (err) { + // eslint-disable-next-line no-console -- see the note on the first console.error above. + console.error('Could not list the editors on this machine:', err); setDetected([]); } }, []); // `editorPath` is one of the detected editors; without one the main process // opens the file dialog, which is what covers every editor detection misses. + // + // A rejected invoke is reported as a refusal rather than raised: the caller + // draws a notice from it, and an unhandled rejection here would be a button + // that silently did nothing. const remember = useCallback(async (editorPath) => { - const result = await window.api.chooseEditor(editorPath); + let result; + try { + result = await window.api.chooseEditor(editorPath); + } catch (err) { + // eslint-disable-next-line no-console -- see the note on the first console.error above. + console.error('Could not remember that editor:', err); + return { ok: false, reason: 'unavailable', error: String(err?.message ?? err) }; + } if (result?.ok) setChosen(result.editor); return result; }, []); @@ -997,11 +1014,28 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit if (result?.reason === 'unregistered-site') { return 'This app has no record of that folder, so it will not open it.'; } + if (result?.reason === 'unavailable') { + return `Could not reach the app's main process: ${result.error || 'unknown error'}`; + } return 'Could not open the folder in an editor.'; }, []); + // The invoke itself can reject — a handler that throws, a window being torn + // down — and a rejection here would leave the notice unset and the picker + // unopened: the button would appear to do nothing, which is the one outcome + // this feature is not allowed to produce. + const askToOpen = useCallback(async () => { + try { + return await window.api.openInEditor(sitePath); + } catch (err) { + // eslint-disable-next-line no-console -- see the note on the first console.error above. + console.error('Could not open the site in an editor:', err); + return { ok: false, reason: 'unavailable', error: String(err?.message ?? err) }; + } + }, [sitePath]); + const openInEditor = useCallback(async () => { - const result = await window.api.openInEditor(sitePath); + const result = await askToOpen(); if (result?.ok) { setEditorNotice(''); return; @@ -1011,23 +1045,39 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit setEditorNotice(result?.reason === 'no-editor' ? '' : describeOpenFailure(result)); await loadDetected(); setEditorPickerOpen(true); - }, [describeOpenFailure, loadDetected, sitePath]); + }, [askToOpen, describeOpenFailure, loadDetected]); const rememberEditor = useCallback(async (editorPath) => { const result = await remember(editorPath); if (!result?.ok) { if (result?.reason === 'unlaunchable-editor') { setEditorNotice('That is not an application this app can open a folder in.'); + } else if (result?.reason === 'unavailable') { + setEditorNotice(describeOpenFailure(result)); } + // 'cancelled' is the contributor closing the dialog, which is an answer, + // not a failure. The picker stays open either way: it is where the next + // attempt starts from. return; } - setEditorPickerOpen(false); - const opened = await window.api.openInEditor(sitePath); + const opened = await askToOpen(); setEditorNotice(opened?.ok ? '' : describeOpenFailure(opened)); - }, [describeOpenFailure, remember, sitePath]); + // Only a launch that actually worked closes the picker. Closing it on the + // choice alone would leave a contributor whose editor failed looking at a + // notice with no picker, one step further from a working editor than before + // they clicked. + if (opened?.ok) setEditorPickerOpen(false); + }, [askToOpen, describeOpenFailure, remember]); const showInFileManager = useCallback(async () => { - const result = await window.api.showSiteInFileManager(sitePath); + let result; + try { + result = await window.api.showSiteInFileManager(sitePath); + } catch (err) { + // eslint-disable-next-line no-console -- see the note on the first console.error above. + console.error('Could not reveal the site folder:', err); + result = { ok: false, error: String(err?.message ?? err) }; + } setEditorNotice(result?.ok ? '' : `Could not open the folder: ${result?.error || 'unknown error'}`); }, [sitePath]); @@ -2799,6 +2849,16 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit onRequestClose={() => setEditorPickerOpen(false)} >
+ {/* Why the picker is open, inside the picker. The notice below the + path row says the same thing, but the modal takes focus, so a + contributor using the keyboard or a screen reader would otherwise + be reading generic copy with the reason left behind them. */} + {editorNotice ? ( +
{editorNotice}
+ ) : null}

{detectedEditors.length ? 'Choose the editor to open this site in. This app will remember it.' diff --git a/test/editor-launch.test.cjs b/test/editor-launch.test.cjs index f9cf2fa..19e8382 100644 --- a/test/editor-launch.test.cjs +++ b/test/editor-launch.test.cjs @@ -19,6 +19,7 @@ const { REFUSAL_REASONS, editorCandidates, detectEditors, + knownEditorName, isLaunchableEditorPath, resolveLaunch, openSiteInEditor @@ -32,15 +33,23 @@ function fakeFs(entries) { const map = new Map(Object.entries(entries)); return { asked, - exists(p) { + // Both probes are async, like the real ones: the module runs on the process + // that draws the window, so it may not stat synchronously. + async exists(p) { asked.push(p); return map.has(p); }, - statPath(p) { + async statPath(p) { asked.push(p); const kind = map.get(p); if (!kind) return null; - return { isDirectory: kind === 'dir', isFile: kind === 'file' }; + // 'file' is a document — present, not executable. 'exe' is something the + // OS will run. The distinction is what the Linux branch turns on. + return { + isDirectory: kind === 'dir', + isFile: kind === 'file' || kind === 'exe', + isExecutable: kind === 'exe' || kind === 'dir' + }; } }; } @@ -80,10 +89,10 @@ const WIN_ENV = { // --- detection ----------------------------------------------------------- -test('detection asks the filesystem about absolute paths only — never PATH', () => { +test('detection asks the filesystem about absolute paths only — never PATH', async () => { const fs = fakeFs({ '/Applications/Visual Studio Code.app': 'dir' }); - const found = detectEditors({ platform: 'darwin', env: { ...MAC_ENV, PATH: '' }, exists: fs.exists }); + const found = await detectEditors({ platform: 'darwin', env: { ...MAC_ENV, PATH: '' }, exists: fs.exists }); assert.deepEqual(found, [ { id: 'vscode', name: 'Visual Studio Code', path: '/Applications/Visual Studio Code.app' } @@ -96,15 +105,15 @@ test('detection asks the filesystem about absolute paths only — never PATH', ( // The #24 regression, stated as a test: the environment a packaged app actually // gets has no useful PATH, and detection must not care. -test('an empty PATH does not change what is detected', () => { - const installed = { 'C:\\Users\\dev\\AppData\\Local\\Programs\\Microsoft VS Code\\Code.exe': 'file' }; +test('an empty PATH does not change what is detected', async () => { + const installed = { 'C:\\Users\\dev\\AppData\\Local\\Programs\\Microsoft VS Code\\Code.exe': 'exe' }; - const withPath = detectEditors({ + const withPath = await detectEditors({ platform: 'win32', env: { ...WIN_ENV, PATH: 'C:\\Windows\\System32' }, exists: fakeFs(installed).exists }); - const withoutPath = detectEditors({ + const withoutPath = await detectEditors({ platform: 'win32', env: { ...WIN_ENV }, exists: fakeFs(installed).exists @@ -115,31 +124,31 @@ test('an empty PATH does not change what is detected', () => { assert.equal(withPath[0].id, 'vscode'); }); -test('nothing installed detects nothing, and does not throw', () => { +test('nothing installed detects nothing, and does not throw', async () => { const fs = fakeFs({}); - assert.deepEqual(detectEditors({ platform: 'darwin', env: MAC_ENV, exists: fs.exists }), []); - assert.deepEqual(detectEditors({ platform: 'win32', env: WIN_ENV, exists: fs.exists }), []); - assert.deepEqual(detectEditors({ platform: 'linux', env: {}, exists: fs.exists }), []); + assert.deepEqual(await detectEditors({ platform: 'darwin', env: MAC_ENV, exists: fs.exists }), []); + assert.deepEqual(await detectEditors({ platform: 'win32', env: WIN_ENV, exists: fs.exists }), []); + assert.deepEqual(await detectEditors({ platform: 'linux', env: {}, exists: fs.exists }), []); }); -test('an unreadable location is a location we do not have, not a crash', () => { +test('an unreadable location is a location we do not have, not a crash', async () => { const exists = (p) => { if (p === '/Applications/Visual Studio Code.app') throw new Error('EACCES'); return p === '/Applications/Cursor.app'; }; - const found = detectEditors({ platform: 'darwin', env: MAC_ENV, exists }); + const found = await detectEditors({ platform: 'darwin', env: MAC_ENV, exists }); assert.deepEqual(found.map((e) => e.id), ['cursor']); }); -test('an editor found in more than one location reports the first', () => { +test('an editor found in more than one location reports the first', async () => { const fs = fakeFs({ '/Applications/Cursor.app': 'dir', '/Users/dev/Applications/Cursor.app': 'dir' }); - const found = detectEditors({ platform: 'darwin', env: MAC_ENV, exists: fs.exists }); + const found = await detectEditors({ platform: 'darwin', env: MAC_ENV, exists: fs.exists }); assert.deepEqual(found, [{ id: 'cursor', name: 'Cursor', path: '/Applications/Cursor.app' }]); }); @@ -152,10 +161,10 @@ test('a location whose environment variable is unset is dropped, not guessed at' assert.ok(homeless.every((p) => p.startsWith('/Applications/'))); }); -test('Windows environment variables are read whatever their casing', () => { - const fs = fakeFs({ 'C:\\Users\\dev\\AppData\\Local\\Programs\\cursor\\Cursor.exe': 'file' }); +test('Windows environment variables are read whatever their casing', async () => { + const fs = fakeFs({ 'C:\\Users\\dev\\AppData\\Local\\Programs\\cursor\\Cursor.exe': 'exe' }); - const found = detectEditors({ + const found = await detectEditors({ platform: 'win32', env: { localappdata: 'C:\\Users\\dev\\AppData\\Local' }, exists: fs.exists @@ -166,36 +175,80 @@ test('Windows environment variables are read whatever their casing', () => { // --- what may be launched ------------------------------------------------ -test('a relative command is not launchable — that is how PATH would come back', () => { +test('a relative command is not launchable — that is how PATH would come back', async () => { const fs = fakeFs({ code: 'file' }); - assert.equal(isLaunchableEditorPath('code', { platform: 'linux', statPath: fs.statPath }), false); - assert.equal(isLaunchableEditorPath('Code.exe', { platform: 'win32', statPath: fs.statPath }), false); + assert.equal(await isLaunchableEditorPath('code', { platform: 'linux', statPath: fs.statPath }), false); + assert.equal(await isLaunchableEditorPath('Code.exe', { platform: 'win32', statPath: fs.statPath }), false); }); -test('the shape has to match the platform', () => { +test('the shape has to match the platform', async () => { const fs = fakeFs({ '/Applications/Cursor.app': 'dir', '/Applications/notes.txt': 'file', - 'C:\\Program Files\\Sublime Text\\sublime_text.exe': 'file', + 'C:\\Program Files\\Sublime Text\\sublime_text.exe': 'exe', 'C:\\Program Files\\Sublime Text\\readme.md': 'file' }); - assert.equal(isLaunchableEditorPath('/Applications/Cursor.app', { platform: 'darwin', statPath: fs.statPath }), true); + assert.equal(await isLaunchableEditorPath('/Applications/Cursor.app', { platform: 'darwin', statPath: fs.statPath }), true); // A file rather than a bundle, and a bundle name is not enough on its own. - assert.equal(isLaunchableEditorPath('/Applications/notes.txt', { platform: 'darwin', statPath: fs.statPath }), false); - assert.equal(isLaunchableEditorPath('/Applications/Missing.app', { platform: 'darwin', statPath: fs.statPath }), false); + assert.equal(await isLaunchableEditorPath('/Applications/notes.txt', { platform: 'darwin', statPath: fs.statPath }), false); + assert.equal(await isLaunchableEditorPath('/Applications/Missing.app', { platform: 'darwin', statPath: fs.statPath }), false); - assert.equal(isLaunchableEditorPath('C:\\Program Files\\Sublime Text\\sublime_text.exe', { platform: 'win32', statPath: fs.statPath }), true); - assert.equal(isLaunchableEditorPath('C:\\Program Files\\Sublime Text\\readme.md', { platform: 'win32', statPath: fs.statPath }), false); + assert.equal(await isLaunchableEditorPath('C:\\Program Files\\Sublime Text\\sublime_text.exe', { platform: 'win32', statPath: fs.statPath }), true); + assert.equal(await isLaunchableEditorPath('C:\\Program Files\\Sublime Text\\readme.md', { platform: 'win32', statPath: fs.statPath }), false); }); -test('junk input is refused rather than thrown', () => { +// The Linux picker cannot filter by extension — there is no extension to filter +// on — so "a regular file" is not enough: a document would be remembered as the +// contributor's editor and then fail with EACCES at the spawn. +test('on Linux a file the OS will not execute is not an application', async () => { + const fs = fakeFs({ '/home/dev/notes.txt': 'file', '/usr/bin/code': 'exe' }); + + assert.equal(await isLaunchableEditorPath('/home/dev/notes.txt', { platform: 'linux', statPath: fs.statPath }), false); + assert.equal(await isLaunchableEditorPath('/usr/bin/code', { platform: 'linux', statPath: fs.statPath }), true); +}); + +// --- what the editor is called ------------------------------------------- + +// The button promises to name the editor. On Windows the filename does not: +// 'Code.exe' is Visual Studio Code and 'phpstorm64.exe' is PhpStorm. +test('a known application is named the way the picker named it', () => { + assert.equal( + knownEditorName('C:\\Users\\dev\\AppData\\Local\\Programs\\Microsoft VS Code\\Code.exe', { platform: 'win32', env: WIN_ENV }), + 'Visual Studio Code' + ); + assert.equal( + knownEditorName('C:\\Users\\dev\\AppData\\Local\\Programs\\PhpStorm\\bin\\phpstorm64.exe', { platform: 'win32', env: WIN_ENV }), + 'PhpStorm' + ); + assert.equal(knownEditorName('/Applications/Cursor.app', { platform: 'darwin', env: MAC_ENV }), 'Cursor'); +}); + +// Windows and macOS filesystems are case-insensitive: the same application +// reached through a differently-cased path is the same application. +test('the lookup is case-insensitive where the filesystem is', () => { + assert.equal( + knownEditorName('c:\\users\\dev\\appdata\\local\\programs\\cursor\\cursor.exe', { platform: 'win32', env: WIN_ENV }), + 'Cursor' + ); + assert.equal(knownEditorName('/applications/zed.app', { platform: 'darwin', env: MAC_ENV }), 'Zed'); + // Linux is not, and two paths differing in case are two different files. + assert.equal(knownEditorName('/USR/BIN/CODE', { platform: 'linux', env: {} }), null); +}); + +test('an application the table does not know has no name to give', () => { + assert.equal(knownEditorName('/Applications/Some Editor.app', { platform: 'darwin', env: MAC_ENV }), null); + assert.equal(knownEditorName('', { platform: 'darwin', env: MAC_ENV }), null); + assert.equal(knownEditorName(null, { platform: 'darwin', env: MAC_ENV }), null); +}); + +test('junk input is refused rather than thrown', async () => { const fs = fakeFs({}); for (const value of [null, undefined, 42, '', {}]) { - assert.equal(isLaunchableEditorPath(value, { platform: 'darwin', statPath: fs.statPath }), false); + assert.equal(await isLaunchableEditorPath(value, { platform: 'darwin', statPath: fs.statPath }), false); } - assert.equal(isLaunchableEditorPath('/Applications/Cursor.app', { platform: 'darwin' }), false); + assert.equal(await isLaunchableEditorPath('/Applications/Cursor.app', { platform: 'darwin' }), false); }); // --- the command that gets run ------------------------------------------- @@ -345,7 +398,7 @@ test('macOS reports what `open` exited with', async () => { // Elsewhere the child is the editor and stays alive, so waiting for it to exit // would mean waiting for the contributor to close it. test('a long-lived editor answers as soon as the OS accepts it', async () => { - const fs = fakeFs({ 'C:\\Program Files\\Sublime Text\\sublime_text.exe': 'file' }); + const fs = fakeFs({ 'C:\\Program Files\\Sublime Text\\sublime_text.exe': 'exe' }); // Exit code 1 as well, to pin that it is not being waited on: an editor that // is still open has no exit code at all, and one that eventually exits // non-zero must not turn a launch that worked into a failure.