diff --git a/src/editor-launch.js b/src/editor-launch.js new file mode 100644 index 0000000..4683f74 --- /dev/null +++ b/src/editor-launch.js @@ -0,0 +1,365 @@ +// 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. +// +// 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 []; + + 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, 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` 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 = await 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 && stats.isExecutable === 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 (!await 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 }); + + let child; + try { + child = spawn(command, args, { + detached: true, + stdio: 'ignore', + shell: false, + windowsHide: 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 = { + REFUSAL_REASONS, + editorCandidates, + detectEditors, + knownEditorName, + isLaunchableEditorPath, + resolveLaunch, + openSiteInEditor +}; 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/main.js b/src/main.js index 1045ede..2d6f050 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, knownEditorName, isLaunchableEditorPath, openSiteInEditor } = require('./editor-launch'); const WORDPRESS_GIT_URL = 'https://github.com/WordPress/wordpress-develop.git'; @@ -957,6 +959,137 @@ 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. + +// 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 { + 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 }); + +async function getChosenEditor() { + const s = await getStore(); + const chosen = (s.get('preferences') || {}).editor; + return chosen && typeof chosen.path === 'string' ? chosen : null; +} + +// 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: await detectEditors({ + platform: process.platform, + env: process.env, + exists: async (p) => (await statPath(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 +// 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 (!await 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, + // 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 }; +}); + +// 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..52504fb 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), @@ -42,6 +46,17 @@ 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') +, + // 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/renderer/index.jsx b/src/renderer/index.jsx index a63305a..b48260e 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'; @@ -60,6 +63,65 @@ 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); }) + // 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; }; + }, []); + + const loadDetected = useCallback(async () => { + try { + const result = await window.api.listEditors(); + setDetected(result?.detected || []); + setChosen(result?.chosen || null); + } 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) => { + 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; + }, []); + + return { chosen, detected, loadDetected, remember }; +} + function useSites() { const [sites, setSites] = useState([]); const [siteMeta, setSiteMeta] = useState({}); @@ -74,6 +136,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) @@ -677,6 +741,7 @@ function App() { onForget={onForget} onDelete={onDelete} onRename={onRename} + editor={editorChoice} isPending={pendingSites.includes(s)} setupLogs={setupLogsBySite[s] || ''} isActive={activeSite === s} @@ -759,7 +824,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); @@ -926,6 +991,96 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit } }, [sitePath]); + // --- opening the code --------------------------------------------------- + // + // 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 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.'; + } + 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 askToOpen(); + if (result?.ok) { + setEditorNotice(''); + return; + } + // Nothing chosen yet is not an error, it is the first use. Anything else is + // worth saying out loud — but both end at the same place: the picker. + setEditorNotice(result?.reason === 'no-editor' ? '' : describeOpenFailure(result)); + await loadDetected(); + setEditorPickerOpen(true); + }, [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; + } + const opened = await askToOpen(); + setEditorNotice(opened?.ok ? '' : describeOpenFailure(opened)); + // 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 () => { + 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]); + 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 +2279,25 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit isSmall /> +
+ + + {chosenEditor ? ( + + ) : null} +
+ {editorNotice ? ( +
+ {editorNotice} + +
+ ) : null}
+ {editorPickerOpen ? ( + 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.' + : '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 ? ( `; + + 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/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 6557f26..a0823a7 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 @@ -70,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..19e8382 --- /dev/null +++ b/test/editor-launch.test.cjs @@ -0,0 +1,416 @@ +'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 { EventEmitter } = require('node:events'); + +const { + REFUSAL_REASONS, + editorCandidates, + detectEditors, + knownEditorName, + 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, + // 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); + }, + async statPath(p) { + asked.push(p); + const kind = map.get(p); + if (!kind) return null; + // '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' + }; + } + }; +} + +// 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) => { + 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 }; +} + +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', async () => { + const fs = fakeFs({ '/Applications/Visual Studio Code.app': 'dir' }); + + 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' } + ]); + 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', async () => { + const installed = { 'C:\\Users\\dev\\AppData\\Local\\Programs\\Microsoft VS Code\\Code.exe': 'exe' }; + + const withPath = await detectEditors({ + platform: 'win32', + env: { ...WIN_ENV, PATH: 'C:\\Windows\\System32' }, + exists: fakeFs(installed).exists + }); + const withoutPath = await 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', async () => { + const fs = fakeFs({}); + 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', async () => { + const exists = (p) => { + if (p === '/Applications/Visual Studio Code.app') throw new Error('EACCES'); + return p === '/Applications/Cursor.app'; + }; + + 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', async () => { + const fs = fakeFs({ + '/Applications/Cursor.app': 'dir', + '/Users/dev/Applications/Cursor.app': 'dir' + }); + + const found = await 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', async () => { + const fs = fakeFs({ 'C:\\Users\\dev\\AppData\\Local\\Programs\\cursor\\Cursor.exe': 'exe' }); + + const found = await 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', async () => { + const fs = fakeFs({ code: 'file' }); + + 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', async () => { + const fs = fakeFs({ + '/Applications/Cursor.app': 'dir', + '/Applications/notes.txt': 'file', + 'C:\\Program Files\\Sublime Text\\sublime_text.exe': 'exe', + 'C:\\Program Files\\Sublime Text\\readme.md': 'file' + }); + + 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(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(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); +}); + +// 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(await isLaunchableEditorPath(value, { platform: 'darwin', statPath: fs.statPath }), false); + } + assert.equal(await 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 = {}, outcome = { event: 'ok' }) { + const fs = fakeFs({ [EDITOR]: 'dir' }); + const { calls, spawn } = recordingSpawn(outcome); + 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 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, /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': '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. + 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 68f2620..8a8bad0 100644 --- a/test/ipc-wiring.test.cjs +++ b/test/ipc-wiring.test.cjs @@ -1170,6 +1170,147 @@ 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'); +}); + +// 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({ + 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); +}); + +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 +1364,12 @@ const WIRED = new Set([ 'git:apply-patch', 'git:fetch-pr-diff', 'git:list-ticket-patches', - 'trac:fetch-attachment' + 'trac:fetch-attachment', + 'editor:get', + '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'); +});