diff --git a/src/editor-launch.js b/src/editor-launch.js index 81fa6d5..5f3c98a 100644 --- a/src/editor-launch.js +++ b/src/editor-launch.js @@ -274,10 +274,15 @@ function resolveLaunch(editorPath, sitePath, { platform } = {}) { return { command: editorPath, args: [sitePath] }; } +// Every `reason` this module can answer `editor:open` with, refusals and +// failures alike. Exported as the complete list on purpose: the renderer's +// open-failure.cjs owes each of these a sentence, and its tests check that +// against this object rather than a copy that could go stale. const REFUSAL_REASONS = { UNREGISTERED_SITE: 'unregistered-site', UNLAUNCHABLE_EDITOR: 'unlaunchable-editor', - UNKNOWN_EDITOR: 'unknown-editor' + UNKNOWN_EDITOR: 'unknown-editor', + SPAWN_FAILED: 'spawn-failed' }; // The `editor:open` handler's body. @@ -327,7 +332,7 @@ async function openSiteInEditor(sitePath, editorPath, { } 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 { ok: false, reason: REFUSAL_REASONS.SPAWN_FAILED, error: e?.message ?? String(e) }; } return awaitLaunch(child, { platform }); @@ -365,14 +370,14 @@ function awaitLaunch(child, { platform } = {}) { }; child.on('error', (e) => { - settle({ ok: false, reason: 'spawn-failed', error: e?.message ?? String(e) }); + settle({ ok: false, reason: REFUSAL_REASONS.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})` }); + : { ok: false, reason: REFUSAL_REASONS.SPAWN_FAILED, error: `the editor could not be opened (exit code ${code})` }); }); return; } diff --git a/src/renderer/index.jsx b/src/renderer/index.jsx index 20e2e85..94f3f58 100644 --- a/src/renderer/index.jsx +++ b/src/renderer/index.jsx @@ -24,6 +24,7 @@ import { computeSetupStepState } from './setup-steps.cjs'; import { shouldShowTerminalHints, computeTerminalBusy } from './terminal-hints.cjs'; import { planDevServerStart, formatElapsed } from './dev-server-command.cjs'; import { pathBasename } from './path-basename.cjs'; +import { noticeForOpenResult } from './open-failure.cjs'; import { trunkAgeInfo, planUpdateSteps, updateStepStatuses, SKIP_INSTALL_MESSAGE, planApplySteps, APPLY_STATE_TO_STEP } from './update-plan.cjs'; import { pickLatest } from '../latest-patch.cjs'; import { parsePrRef } from '../patch-sources.cjs'; @@ -1207,37 +1208,15 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit // entry is ever drawn disabled: an application this app cannot find is not one // it refuses to use, and the copy button above is the floor under all of it. const { detected: detectedEditors, loading: detectingEditors, loadDetected } = editor; - const [editorNotice, setEditorNotice] = useState(''); + // `{ message, offerPicker }` from open-failure.cjs, or null for nothing to + // say. Both what it reads and whether "Choose application…" is a way out of + // it are decided there, per reason — the two callers below deciding that + // separately is what #180 was. + const [editorNotice, setEditorNotice] = useState(null); const fileManagerLabel = FILE_MANAGER_LABELS[window.api?.platform] || 'Show in file manager'; const fileManagerName = FILE_MANAGER_NAMES[window.api?.platform] || 'File manager'; - // `picked` says which of the two failures 'unlaunchable-editor' is: an - // application detection offered that has since moved, or one the contributor - // just pointed at that is not an application at all. Main cannot tell them - // apart — the guard is the same — but the caller knows which it asked for, and - // the two need different next steps. - const describeOpenFailure = useCallback((result, { picked = false } = {}) => { - if (result?.reason === 'unlaunchable-editor') { - return picked - ? 'That is not an application this app can open a folder in.' - : 'That application is no longer where it was. Choose another.'; - } - if (result?.reason === 'unknown-editor') { - return 'That application is no longer where it was. Choose another.'; - } - if (result?.reason === 'spawn-failed') { - return `The application 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 application.'; - }, []); - // `editorPath` is one of the detected applications; null asks the main process // for the file dialog instead. // @@ -1254,19 +1233,17 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit console.error('Could not open the site directory:', err); result = { ok: false, reason: 'unavailable', error: String(err?.message ?? err) }; } - // Closing the dialog is an answer, not a failure — saying something about it - // would be the app arguing with a decision the contributor just made. - if (result?.ok || result?.reason === 'cancelled') { - setEditorNotice(''); - return; - } - setEditorNotice(describeOpenFailure(result, { picked: editorPath === null })); + const notice = noticeForOpenResult(result, { picked: editorPath === null }); + setEditorNotice(notice); // An application that was detected and then failed is one detection should be // asked about again, so the next menu does not offer it as if nothing had // happened. - if (editorPath !== null) await loadDetected(); - }, [describeOpenFailure, loadDetected, sitePath]); + if (notice && editorPath !== null) await loadDetected(); + }, [loadDetected, sitePath]); + // Through the same function as `openIn` above, deliberately: this used to + // build its own sentence out of `error` alone, so a refusal — which carries a + // `reason` and no `error` — came out as the words "unknown error" (#180). const showInFileManager = useCallback(async () => { let result; try { @@ -1274,9 +1251,9 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit } 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) }; + result = { ok: false, reason: 'unavailable', error: String(err?.message ?? err) }; } - setEditorNotice(result?.ok ? '' : `Could not open the folder: ${result?.error || 'unknown error'}`); + setEditorNotice(noticeForOpenResult(result)); }, [sitePath]); const appendNpm = useCallback((s)=>setNpmLogs((v)=>v+s),[]); @@ -2993,8 +2970,10 @@ function SiteRow({ sitePath, initialized, createdAt, label, onInitialized, onSit contributor to find the menu again. */} {editorNotice ? (
- {editorNotice} - + {editorNotice.message} + {editorNotice.offerPicker ? ( + + ) : null}
) : null} diff --git a/src/renderer/open-failure.cjs b/src/renderer/open-failure.cjs new file mode 100644 index 0000000..eb169c2 --- /dev/null +++ b/src/renderer/open-failure.cjs @@ -0,0 +1,108 @@ +// What the window says when a folder will not open. +// +// There were two of these. The editor menu grew a real one in #209 — a branch +// per reason, each saying what the contributor can do next — while revealing +// the folder in the file manager kept the string it shipped with: the `error` +// field, or the words "unknown error" when there was none. +// +// A refusal is precisely the case with no `error` field: main declined on +// purpose and returned a `reason` instead. So the failure the app understood +// best was the one it described as unknown, which is #180 as the contributor +// meets it — a button that does nothing, explained by a sentence that explains +// nothing. +// +// Pure and dependency-free for the same reason as setup-steps.cjs: the renderer +// bundle imports it, `node --test` requires it directly, and neither needs a DOM. +'use strict'; + +// An OS-supplied message is quoted rather than replaced — it is the only part of +// these failures the app did not write, and usually the only part that says +// which of a dozen things went wrong. +// +// "unknown error" as the fallback is #209's wording, kept deliberately. Here it +// is honest: the attempt failed and nothing came back to say why. What #180 was +// about is the opposite case — a refusal, where the app knows exactly why and +// has a `reason` — and that never reaches this function's fallback. +function quote(error) { + const text = typeof error === 'string' ? error.trim() : ''; + return text || 'unknown error'; +} + +/** + * The sentence for a failed attempt to open a site's folder. + * + * `picked` says which of the two situations 'unlaunchable-editor' is: an + * application detection offered that has since moved, or one the contributor + * just pointed at that is not an application at all. Main cannot tell them + * apart — the guard is the same — but the caller knows which it asked for, and + * the two need different next steps. + * + * @param {Object} result What the main process returned. + * @param {Object} [options] + * @param {boolean} [options.picked] + * @return {string} + */ +function describeOpenFailure(result, { picked = false } = {}) { + if (result?.reason === 'unlaunchable-editor') { + return picked + ? 'That is not an application this app can open a folder in.' + : 'That application is no longer where it was. Choose another.'; + } + if (result?.reason === 'unknown-editor') { + return 'That application is no longer where it was. Choose another.'; + } + if (result?.reason === 'spawn-failed') { + return `The application would not start: ${quote(result.error)}`; + } + // The file manager's own refusal, from `shell.openPath` — a different verb + // from the editor's, and the one case here that carries the OS's message. + if (result?.reason === 'open-failed') { + return `The file manager would not open the folder: ${quote(result.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: ${quote(result.error)}`; + } + // Both callers share this now, so it says nothing about an application — + // "could not open it in an application" is not what happened when the file + // manager is what failed. + return 'Could not open the folder.'; +} + +// The reasons another application is a way out of. The notice's only affordance +// is "Choose application…", and beside the other reasons it is a dead end that +// looks like a fix: `openSiteInEditor` checks the folder before it looks at the +// editor (see editor-launch.js), so answering a refused *folder* by picking a +// different application returns the identical sentence. +const PICKING_HELPS = new Set(['unlaunchable-editor', 'unknown-editor', 'spawn-failed']); + +/** + * The whole notice for an attempt to open a site's folder, or null when there + * is nothing to say. + * + * This, rather than `describeOpenFailure`, is what the window calls. The two + * callers used to decide separately whether there was a failure at all and what + * to render beside it, which is how one of them ended up printing its own + * "unknown error" for a refusal that had a perfectly good reason (#180). One + * function means one answer. + * + * A closed dialog is not a failure: saying something about it would be the app + * arguing with a decision the contributor just made. + * + * @param {Object} result What the main process returned. + * @param {Object} [options] + * @param {boolean} [options.picked] + * @return {?{message: string, offerPicker: boolean}} + */ +function noticeForOpenResult(result, { picked = false } = {}) { + if (result?.ok || result?.reason === 'cancelled') return null; + + return { + message: describeOpenFailure(result, { picked }), + offerPicker: PICKING_HELPS.has(result?.reason) + }; +} + +module.exports = { describeOpenFailure, noticeForOpenResult }; diff --git a/src/site-registry.js b/src/site-registry.js index a0823a7..ffdd756 100644 --- a/src/site-registry.js +++ b/src/site-registry.js @@ -16,6 +16,15 @@ const { describeRefused } = require('./safe-log'); +// Every `reason` this module can answer `dir:show` with, same convention as +// REFUSAL_REASONS in editor-launch.js: the renderer's open-failure.cjs owes +// each of these a sentence, and its tests read this object rather than keeping +// a copy that could go stale. +const REVEAL_REASONS = { + UNREGISTERED_SITE: 'unregistered-site', + OPEN_FAILED: 'open-failed' +}; + // 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 @@ -61,14 +70,15 @@ async function deleteRegisteredSite(sitePath, { sites, forget, remove, onRefused async function revealRegisteredSite(sitePath, { sites, reveal, onRefused } = {}) { if (!isRegisteredSite(sitePath, sites)) { if (typeof onRefused === 'function') onRefused(describeRefusedSite(sitePath)); - return { ok: false, reason: 'unregistered-site' }; + return { ok: false, reason: REVEAL_REASONS.UNREGISTERED_SITE }; } const error = await reveal(sitePath); - return error ? { ok: false, reason: 'open-failed', error } : { ok: true }; + return error ? { ok: false, reason: REVEAL_REASONS.OPEN_FAILED, error } : { ok: true }; } module.exports = { + REVEAL_REASONS, isRegisteredSite, describeRefusedSite, revealRegisteredSite, diff --git a/test/open-failure.test.cjs b/test/open-failure.test.cjs new file mode 100644 index 0000000..6bbc302 --- /dev/null +++ b/test/open-failure.test.cjs @@ -0,0 +1,146 @@ +'use strict'; + +// The sentence the window shows when a folder will not open. +// +// It exists as its own module because there were two of them. The editor menu +// grew a real one in #209 — a branch per reason, each saying what to do next — +// while revealing the folder in the file manager kept the string it shipped +// with: the `error` field, or the words "unknown error" when there was none. +// +// A refusal is exactly the case with no `error` field. So the one failure the +// app understands best — it declined on purpose, and knows why — was the one it +// described as unknown (#180). + +const test = require('node:test'); +const assert = require('node:assert/strict'); + +const { describeOpenFailure, noticeForOpenResult } = require('../src/renderer/open-failure.cjs'); +const { REFUSAL_REASONS } = require('../src/editor-launch.js'); +const { REVEAL_REASONS } = require('../src/site-registry.js'); + +// The two the guard modules cannot list: `cancelled` is the file dialog +// closing, returned by the `editor:open` handler itself (src/main.js), and +// `unavailable` is synthesised in the renderer when the invoke rejects. +const REASONS_FROM_ELSEWHERE = ['cancelled', 'unavailable']; + +// Everything `editor:open` and `dir:show` can answer with has to have a +// sentence here, and a hand-kept copy of that list is the kind that goes stale +// in the direction of the generic fallback — which is the failure this whole +// module exists to stop. So the list is the guard modules' own exports, which +// their answers are built from, plus the two above. +function reasonsInGuardModules() { + return [...new Set([ + ...Object.values(REFUSAL_REASONS), + ...Object.values(REVEAL_REASONS) + ])]; +} + +// --- the whole notice ---------------------------------------------------- +// +// `noticeForOpenResult` is the unit the window actually uses, and it is the +// unit deliberately: the sentence was never the broken part. #209 already +// described 'unregistered-site' correctly, and revealing the folder still +// printed "unknown error", because that call site built its own string instead +// of asking. Testing the describer alone would have passed on the old code. +// +// So everything the caller used to decide inline — whether there is a notice at +// all, what it says, and whether "Choose application…" is a way out of it — +// lives here, and the window is left with an assignment. + +test('a refusal to reveal says the app has no record of the folder', () => { + const notice = noticeForOpenResult({ ok: false, reason: 'unregistered-site' }); + + assert.match(notice.message, /no record/); + assert.doesNotMatch(notice.message, /unknown error/); +}); + +test('a success is not a notice', () => { + assert.equal(noticeForOpenResult({ ok: true }), null); +}); + +// Closing the dialog is an answer, not a failure — saying something about it +// would be the app arguing with a decision the contributor just made. +test('a cancelled dialog is not a notice either', () => { + assert.equal(noticeForOpenResult({ ok: false, reason: 'cancelled' }), null); +}); + +// "Choose application…" answers "that editor did not work". It is not an answer +// to a refusal the application had nothing to do with: `openSiteInEditor` +// checks the folder before it looks at the editor, so picking another one comes +// back with the identical sentence. +test('the picker is only offered where picking another application would help', () => { + const helps = ['unlaunchable-editor', 'unknown-editor', 'spawn-failed']; + const doesNot = ['unregistered-site', 'open-failed', 'unavailable']; + + for (const reason of helps) { + assert.equal(noticeForOpenResult({ ok: false, reason }).offerPicker, true, reason); + } + for (const reason of doesNot) { + assert.equal(noticeForOpenResult({ ok: false, reason }).offerPicker, false, reason); + } + + // The two lists above are a judgement per reason, so a new refusal must be + // judged rather than defaulting to "no way forward" unnoticed. + const judged = new Set([...helps, ...doesNot, 'cancelled']); + for (const reason of [...reasonsInGuardModules(), ...REASONS_FROM_ELSEWHERE]) { + assert.ok(judged.has(reason), `${reason} has no decision about the picker`); + } +}); + +// --- the sentence -------------------------------------------------------- + +// The half that is easy to lose in the move. `revealRegisteredSite` returns +// 'open-failed' *with* the OS's own message, and the string this replaces did +// surface it. A branch that fell through to the generic sentence would be a +// regression dressed as a cleanup. +test('an OS failure keeps the message the OS gave', () => { + const sentence = describeOpenFailure({ ok: false, reason: 'open-failed', error: 'no application' }); + + assert.match(sentence, /would not open/); + assert.match(sentence, /no application/); +}); + +test('an OS failure with nothing to quote still names the failure', () => { + const sentence = describeOpenFailure({ ok: false, reason: 'open-failed' }); + + assert.match(sentence, /would not open/); + assert.doesNotMatch(sentence, /undefined/); +}); + +// Read from the guard modules' own exports rather than listed here, so a +// refusal added to either one fails this instead of silently arriving as +// "Could not open the folder." — the generic sentence is a fallback, not a +// destination. +test('every reason the main process can refuse with has its own sentence', () => { + const generic = describeOpenFailure({ ok: false }); + + for (const reason of [...reasonsInGuardModules(), ...REASONS_FROM_ELSEWHERE]) { + if (reason === 'cancelled') continue; // Not a failure; noticeForOpenResult drops it. + assert.notEqual(describeOpenFailure({ ok: false, reason }), generic, reason); + } +}); + +test('a message the app could not read does not reach the window as "undefined"', () => { + for (const error of [undefined, null, ' ', 42]) { + const sentence = describeOpenFailure({ ok: false, reason: 'spawn-failed', error }); + assert.doesNotMatch(sentence, /undefined|null|42/); + } +}); + +// Carried over from #209 rather than invented here: 'unlaunchable-editor' is two +// different situations, and only the caller knows which it asked for. +test('an application that will not launch reads differently for a picked one', () => { + const detected = describeOpenFailure({ ok: false, reason: 'unlaunchable-editor' }, { picked: false }); + const picked = describeOpenFailure({ ok: false, reason: 'unlaunchable-editor' }, { picked: true }); + + assert.notEqual(detected, picked); + assert.match(detected, /no longer where it was/); + assert.match(picked, /not an application/); +}); + +test('a failure with no reason at all still says something', () => { + const sentence = describeOpenFailure({ ok: false }); + + assert.ok(sentence.length > 0); + assert.doesNotMatch(sentence, /undefined/); +});