From 130b2ef425bd33849972c0138e4b3997ea262582 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 3 Oct 2026 00:51:13 -0700 Subject: [PATCH] feat(documents): open CSV and TSV files from the OS Extend existing OS document associations and delivery to CSV/TSV, preserving restoration and authorization. Co-authored-by: Neil --- config/electron-builder.config.cjs | 30 +- config/nsis/orca-installer-hooks.nsh | 44 +- ...ron-builder-markdown-associations.test.mjs | 33 +- src/main/index.ts | 26 +- .../markdown-documents-ripgrep-real.test.ts | 2 + src/main/ipc/markdown-documents.ts | 8 +- .../startup/main-process-ipc-bootstrap.ts | 10 +- src/main/startup/main-process-state.ts | 6 +- src/main/startup/main-window-controller.ts | 2 +- .../os-opened-document-arguments.test.ts | 176 ++++++++ ...ts => os-opened-document-delivery.test.ts} | 10 +- .../os-opened-document-resolution.test.ts | 91 ++++ ...t.ts => os-opened-document-wiring.test.ts} | 14 +- src/main/startup/os-opened-documents.test.ts | 179 ++++++++ ...rkdown-files.ts => os-opened-documents.ts} | 59 +-- .../startup/os-opened-markdown-files.test.ts | 387 ------------------ src/preload/api/ui-command-event-api.ts | 2 +- .../use-floating-terminal-create-actions.ts | 4 +- .../os-markdown-file-open-bridge.test.ts | 45 +- .../os-markdown-file-open-bridge.ts | 39 +- .../src/i18n/en-runtime-required.json | 15 + src/renderer/src/i18n/locales/en.json | 3 + ...en-document-in-floating-workspace.test.ts} | 41 +- ...=> open-document-in-floating-workspace.ts} | 8 +- src/shared/filesystem-entry-types.ts | 4 +- tests/e2e/helpers/orca-restart.ts | 3 +- tests/e2e/os-opened-documents.spec.ts | 110 +++++ 27 files changed, 815 insertions(+), 536 deletions(-) create mode 100644 src/main/startup/os-opened-document-arguments.test.ts rename src/main/startup/{os-opened-markdown-delivery.test.ts => os-opened-document-delivery.test.ts} (88%) create mode 100644 src/main/startup/os-opened-document-resolution.test.ts rename src/main/startup/{os-opened-markdown-wiring.test.ts => os-opened-document-wiring.test.ts} (81%) create mode 100644 src/main/startup/os-opened-documents.test.ts rename src/main/startup/{os-opened-markdown-files.ts => os-opened-documents.ts} (72%) delete mode 100644 src/main/startup/os-opened-markdown-files.test.ts rename src/renderer/src/lib/{open-markdown-in-floating-workspace.test.ts => open-document-in-floating-workspace.test.ts} (66%) rename src/renderer/src/lib/{open-markdown-in-floating-workspace.ts => open-document-in-floating-workspace.ts} (77%) create mode 100644 tests/e2e/os-opened-documents.spec.ts diff --git a/config/electron-builder.config.cjs b/config/electron-builder.config.cjs index 7c146a29171..f821319f0cd 100644 --- a/config/electron-builder.config.cjs +++ b/config/electron-builder.config.cjs @@ -164,9 +164,10 @@ const rpmElectronRuntimeDependencies = [ ] // Why mirrored, not imported: this config is CJS loaded by electron-builder outside the TS build. -// Keep in sync with isMarkdownDocumentName() in src/main/ipc/markdown-documents.ts and with +// Keep in sync with isOsOpenedDocumentName() in src/main/startup/os-opened-documents.ts and with // config/nsis/orca-installer-hooks.nsh, which registers the same set on Windows. const MARKDOWN_FILE_EXTENSIONS = ['md', 'markdown', 'mdx'] +const TABULAR_FILE_EXTENSIONS = ['csv', 'tsv'] // Why: the config must load on a host-only install without resolving unused Windows addons. // This is load-time tolerance only; beforePack enforces that the target's natives are installed. @@ -509,16 +510,25 @@ module.exports = { include: resolve(__dirname, 'nsis', 'orca-installer-hooks.nsh') }, mac: { - // Why rank Alternate: Orca joins Finder's "Open With" list for Markdown without claiming + // Why rank Alternate: Orca joins Finder's "Open With" list without claiming // LSHandlerRank ownership, so whichever editor the user already prefers stays the default. // Why one entry per extension: app-builder-lib globs `*.${ext}`, which an array would break. - fileAssociations: MARKDOWN_FILE_EXTENSIONS.map((ext) => ({ - ext, - name: 'Markdown Document', - description: 'Markdown Document', - role: 'Editor', - rank: 'Alternate' - })), + fileAssociations: [ + ...MARKDOWN_FILE_EXTENSIONS.map((ext) => ({ + ext, + name: 'Markdown Document', + description: 'Markdown Document', + role: 'Editor', + rank: 'Alternate' + })), + ...TABULAR_FILE_EXTENSIONS.map((ext) => ({ + ext, + name: `${ext.toUpperCase()} Document`, + description: `${ext.toUpperCase()} Document`, + role: 'Editor', + rank: 'Alternate' + })) + ], icon: 'resources/build/icon.icns', entitlements: 'resources/build/entitlements.mac.plist', entitlementsInherit: 'resources/build/entitlements.mac.plist', @@ -611,7 +621,7 @@ module.exports = { // override. A desktop entry's MimeType only adds a handler - mimeapps.list still owns the // default. .mdx is deliberately absent: Ubuntu 24.04's mime database maps it to // application/x-genesis-32x-rom, so claiming it here would need a glob override. - mimeTypes: ['text/markdown'], + mimeTypes: ['text/markdown', 'text/csv', 'text/tab-separated-values'], // Why: Ubuntu desktop ships GNOME Orca as the `orca` package and /usr/bin/orca. // The Linux installer should not claim those system package/file names. executableName: 'orca-ide', diff --git a/config/nsis/orca-installer-hooks.nsh b/config/nsis/orca-installer-hooks.nsh index a6fb31f9aec..6d6dd069592 100644 --- a/config/nsis/orca-installer-hooks.nsh +++ b/config/nsis/orca-installer-hooks.nsh @@ -6,7 +6,7 @@ !include "${__FILEDIR__}\orca-process-check.nsh" ; --------------------------------------------------------------------------- -; Markdown "Open with Orca" (issue #10138) +; Markdown and CSV/TSV "Open with Orca" (issues #10138, #23225) ; ; Why hand-rolled instead of electron-builder's `fileAssociations` on Windows: ; app-builder-lib emits !insertmacro APP_ASSOCIATE, whose first line is @@ -21,29 +21,36 @@ ; exactly where the user left it. Never add a `Software\Classes\.` default ; value here. ; -; MARKDOWN_PROGID must stay in sync with the extension list handled by -; isMarkdownDocumentName() in src/main/ipc/markdown-documents.ts. +; Keep the extension list in sync with isOsOpenedDocumentName(). ; --------------------------------------------------------------------------- !define MARKDOWN_PROGID "Orca.Markdown" +!define TABULAR_PROGID "Orca.Tabular" -!macro ORCA_REGISTER_MARKDOWN_OPEN_WITH EXT - WriteRegNone SHELL_CONTEXT "Software\Classes\${EXT}\OpenWithProgids" "${MARKDOWN_PROGID}" +!macro ORCA_REGISTER_DOCUMENT_OPEN_WITH EXT PROGID + WriteRegNone SHELL_CONTEXT "Software\Classes\${EXT}\OpenWithProgids" "${PROGID}" WriteRegStr SHELL_CONTEXT "Software\Classes\Applications\${APP_EXECUTABLE_FILENAME}\SupportedTypes" "${EXT}" "" !macroend -!macro ORCA_UNREGISTER_MARKDOWN_OPEN_WITH EXT - DeleteRegValue SHELL_CONTEXT "Software\Classes\${EXT}\OpenWithProgids" "${MARKDOWN_PROGID}" +!macro ORCA_UNREGISTER_DOCUMENT_OPEN_WITH EXT PROGID + DeleteRegValue SHELL_CONTEXT "Software\Classes\${EXT}\OpenWithProgids" "${PROGID}" DeleteRegValue SHELL_CONTEXT "Software\Classes\Applications\${APP_EXECUTABLE_FILENAME}\SupportedTypes" "${EXT}" !macroend +!macro ORCA_REGISTER_DOCUMENT_PROGID PROGID NAME + WriteRegStr SHELL_CONTEXT "Software\Classes\${PROGID}" "" "${NAME}" + WriteRegStr SHELL_CONTEXT "Software\Classes\${PROGID}\DefaultIcon" "" "$appExe,0" + WriteRegStr SHELL_CONTEXT "Software\Classes\${PROGID}\shell\open" "" "Open with ${PRODUCT_NAME}" + WriteRegStr SHELL_CONTEXT "Software\Classes\${PROGID}\shell\open\command" "" '"$appExe" "%1"' +!macroend + !macro customInstall - WriteRegStr SHELL_CONTEXT "Software\Classes\${MARKDOWN_PROGID}" "" "Markdown Document" - WriteRegStr SHELL_CONTEXT "Software\Classes\${MARKDOWN_PROGID}\DefaultIcon" "" "$appExe,0" - WriteRegStr SHELL_CONTEXT "Software\Classes\${MARKDOWN_PROGID}\shell\open" "" "Open with ${PRODUCT_NAME}" - WriteRegStr SHELL_CONTEXT "Software\Classes\${MARKDOWN_PROGID}\shell\open\command" "" '"$appExe" "%1"' - !insertmacro ORCA_REGISTER_MARKDOWN_OPEN_WITH ".md" - !insertmacro ORCA_REGISTER_MARKDOWN_OPEN_WITH ".markdown" - !insertmacro ORCA_REGISTER_MARKDOWN_OPEN_WITH ".mdx" + !insertmacro ORCA_REGISTER_DOCUMENT_PROGID "${MARKDOWN_PROGID}" "Markdown Document" + !insertmacro ORCA_REGISTER_DOCUMENT_PROGID "${TABULAR_PROGID}" "Tabular Document" + !insertmacro ORCA_REGISTER_DOCUMENT_OPEN_WITH ".md" "${MARKDOWN_PROGID}" + !insertmacro ORCA_REGISTER_DOCUMENT_OPEN_WITH ".markdown" "${MARKDOWN_PROGID}" + !insertmacro ORCA_REGISTER_DOCUMENT_OPEN_WITH ".mdx" "${MARKDOWN_PROGID}" + !insertmacro ORCA_REGISTER_DOCUMENT_OPEN_WITH ".csv" "${TABULAR_PROGID}" + !insertmacro ORCA_REGISTER_DOCUMENT_OPEN_WITH ".tsv" "${TABULAR_PROGID}" ; Why: Explorer caches the association list until told otherwise. System::Call "shell32::SHChangeNotify(i,i,i,i) (0x08000000, 0x1000, 0, 0)" !macroend @@ -100,8 +107,11 @@ ; Why outside the ${isUpdated} guard: customInstall rewrites these on every update, so ; dropping them during uninstallOldVersion is correct and keeps the pair symmetric. DeleteRegKey SHELL_CONTEXT "Software\Classes\${MARKDOWN_PROGID}" - !insertmacro ORCA_UNREGISTER_MARKDOWN_OPEN_WITH ".md" - !insertmacro ORCA_UNREGISTER_MARKDOWN_OPEN_WITH ".markdown" - !insertmacro ORCA_UNREGISTER_MARKDOWN_OPEN_WITH ".mdx" + DeleteRegKey SHELL_CONTEXT "Software\Classes\${TABULAR_PROGID}" + !insertmacro ORCA_UNREGISTER_DOCUMENT_OPEN_WITH ".md" "${MARKDOWN_PROGID}" + !insertmacro ORCA_UNREGISTER_DOCUMENT_OPEN_WITH ".markdown" "${MARKDOWN_PROGID}" + !insertmacro ORCA_UNREGISTER_DOCUMENT_OPEN_WITH ".mdx" "${MARKDOWN_PROGID}" + !insertmacro ORCA_UNREGISTER_DOCUMENT_OPEN_WITH ".csv" "${TABULAR_PROGID}" + !insertmacro ORCA_UNREGISTER_DOCUMENT_OPEN_WITH ".tsv" "${TABULAR_PROGID}" System::Call "shell32::SHChangeNotify(i,i,i,i) (0x08000000, 0x1000, 0, 0)" !macroend diff --git a/config/scripts/electron-builder-markdown-associations.test.mjs b/config/scripts/electron-builder-markdown-associations.test.mjs index 58f6f8d8865..654c6db246f 100644 --- a/config/scripts/electron-builder-markdown-associations.test.mjs +++ b/config/scripts/electron-builder-markdown-associations.test.mjs @@ -8,6 +8,8 @@ const require = createRequire(import.meta.url) const electronBuilderConfig = require('../electron-builder.config.cjs') const MARKDOWN_EXTENSIONS = ['md', 'markdown', 'mdx'] +const TABULAR_EXTENSIONS = ['csv', 'tsv'] +const DOCUMENT_EXTENSIONS = [...MARKDOWN_EXTENSIONS, ...TABULAR_EXTENSIONS] // The exact shape app-builder-lib's APP_ASSOCIATE emits: a write to the DEFAULT ("") // value of Software\Classes\.. Additive `WriteRegNone ...\OpenWithProgids` must not @@ -23,23 +25,23 @@ const stripNsisCommentLines = (source) => const readInstallerHooks = () => readFile(electronBuilderConfig.nsis.include, 'utf8') -describe('electron-builder markdown file associations', () => { +describe('electron-builder document file associations', () => { // Why: any top-level (or `win.`) fileAssociations entry makes app-builder-lib's NSIS // packager emit `!insertmacro APP_ASSOCIATE`, whose first line writes that DEFAULT value // — silently taking .md from whichever editor owns it, for every existing user on their // next UPDATE, with APP_UNASSOCIATE never restoring it. `rank: 'Alternate'` cannot // prevent this; it is LSHandlerRank and applies to macOS only. So the mac block must // stay under `mac.` — hoisting it up "to share it with Windows" is what this test blocks. - it('never claims the Windows default markdown handler', () => { + it('never claims the Windows default document handler', () => { expect(electronBuilderConfig.fileAssociations).toBeUndefined() expect(electronBuilderConfig.win?.fileAssociations).toBeUndefined() }) - it('joins the macOS Open With list for every markdown extension without owning it', () => { + it('joins the macOS Open With list for every supported extension without owning it', () => { const associations = electronBuilderConfig.mac.fileAssociations // One entry per extension: an array `ext` would break the Linux packager's `*.${ext}` glob. expect([...associations].map((association) => association.ext).sort()).toEqual( - [...MARKDOWN_EXTENSIONS].sort() + [...DOCUMENT_EXTENSIONS].sort() ) for (const association of associations) { expect(association).toMatchObject({ role: 'Editor', rank: 'Alternate' }) @@ -54,6 +56,14 @@ describe('electron-builder markdown file associations', () => { expect(electronBuilderConfig.linux.fileAssociations).toBeUndefined() }) + it('adds CSV and TSV handlers to the Linux desktop entry', () => { + expect(electronBuilderConfig.linux.mimeTypes).toEqual([ + 'text/markdown', + 'text/csv', + 'text/tab-separated-values' + ]) + }) + it('points the single NSIS include at the installer hooks file on disk', () => { const includePath = electronBuilderConfig.nsis.include expect(existsSync(includePath)).toBe(true) @@ -84,7 +94,7 @@ describe('electron-builder markdown file associations', () => { expect(stripped).toMatch(DEFAULT_HANDLER_WRITE) }) - it('registers Windows markdown Open With additively, never as the default', async () => { + it('registers Windows document Open With additively, never as the default', async () => { const hooks = await readInstallerHooks() expect(stripNsisCommentLines(hooks)).not.toMatch(DEFAULT_HANDLER_WRITE) @@ -92,11 +102,18 @@ describe('electron-builder markdown file associations', () => { expect(hooks).toMatch( /WriteRegNone\s+SHELL_CONTEXT\s+"Software\\Classes\\\$\{EXT\}\\OpenWithProgids"/ ) - expect(hooks).toMatch(/!macro\s+ORCA_REGISTER_MARKDOWN_OPEN_WITH\s+EXT/) + expect(hooks).toMatch(/!macro\s+ORCA_REGISTER_DOCUMENT_OPEN_WITH\s+EXT\s+PROGID/) for (const ext of MARKDOWN_EXTENSIONS) { - expect(hooks).toContain(`ORCA_REGISTER_MARKDOWN_OPEN_WITH ".${ext}"`) - expect(hooks).toContain(`ORCA_UNREGISTER_MARKDOWN_OPEN_WITH ".${ext}"`) + expect(hooks).toContain(`ORCA_REGISTER_DOCUMENT_OPEN_WITH ".${ext}" "\${MARKDOWN_PROGID}"`) + expect(hooks).toContain(`ORCA_UNREGISTER_DOCUMENT_OPEN_WITH ".${ext}" "\${MARKDOWN_PROGID}"`) } + for (const ext of TABULAR_EXTENSIONS) { + expect(hooks).toContain(`ORCA_REGISTER_DOCUMENT_OPEN_WITH ".${ext}" "\${TABULAR_PROGID}"`) + expect(hooks).toContain(`ORCA_UNREGISTER_DOCUMENT_OPEN_WITH ".${ext}" "\${TABULAR_PROGID}"`) + } + expect(hooks).toContain('!define TABULAR_PROGID "Orca.Tabular"') + expect(hooks).toContain('ORCA_REGISTER_DOCUMENT_PROGID "${TABULAR_PROGID}" "Tabular Document"') + expect(hooks).toContain('DeleteRegKey SHELL_CONTEXT "Software\\Classes\\${TABULAR_PROGID}"') expect(hooks).toMatch(/!macro\s+customInstall\b/) expect(hooks).toMatch(/!macro\s+customUnInstall\b/) }) diff --git a/src/main/index.ts b/src/main/index.ts index bd02d96abcc..4e60d1076ed 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -13,7 +13,7 @@ import { registerMainProcessIpcHandlers } from './startup/main-process-ipc-boots import { initializeMainProcessReady } from './startup/main-process-ready' import { installMainProcessQuitHandlers } from './startup/main-process-quit' import { shouldActivateDesktopForSecondInstance } from './startup/single-instance-lock' -import { resolveOpenedMarkdownDocuments } from './startup/os-opened-markdown-files' +import { resolveOsOpenedDocuments } from './startup/os-opened-documents' import { formatProfileStateStartupFailure, isDivergedProfileStateFailure, @@ -41,7 +41,7 @@ function requestDesktopActivation(argv: readonly string[] = []): void { state.skillShareDeepLinks.capture(argv, (shareId) => { state.mainWindow?.webContents.send('ui:openSkillShare', shareId) }) - state.osOpenedMarkdownFiles.capture(argv, publishOsOpenedMarkdownFiles) + state.osOpenedDocuments.capture(argv, publishOsOpenedDocuments) // Why: a duplicate `orca serve` must not drag a headless server into opening a desktop window (#11935). if (!shouldActivateDesktopForSecondInstance(argv)) { return @@ -50,26 +50,26 @@ function requestDesktopActivation(argv: readonly string[] = []): void { } /** - * Hands buffered OS-opened markdown paths to a renderer that has proven it is listening. + * Hands buffered OS-opened document paths to a renderer that has proven it is listening. * * Until that proof arrives the paths stay buffered, because `webContents.send` to a renderer * with no listener attached is dropped silently and the queue would be gone. */ -function publishOsOpenedMarkdownFiles(): void { +function publishOsOpenedDocuments(): void { const targetWindow = state.mainWindow - if (!state.markdownFileOpenListenerReady || !targetWindow || targetWindow.isDestroyed()) { + if (!state.osDocumentOpenListenerReady || !targetWindow || targetWindow.isDestroyed()) { return } // Why consumed before the await: a renderer pull racing this resolve must not take the same // batch again. The restore() calls hand it back if delivery turns out to be impossible. - const filePaths = state.osOpenedMarkdownFiles.consume() + const filePaths = state.osOpenedDocuments.consume() if (filePaths.length === 0) { return } - void resolveOpenedMarkdownDocuments(filePaths) + void resolveOsOpenedDocuments(filePaths) .then((documents) => { if (targetWindow.isDestroyed() || targetWindow.webContents.isDestroyed()) { - state.osOpenedMarkdownFiles.restore(filePaths) + state.osOpenedDocuments.restore(filePaths) return } if (documents.length > 0) { @@ -77,8 +77,8 @@ function publishOsOpenedMarkdownFiles(): void { } }) .catch((error) => { - state.osOpenedMarkdownFiles.restore(filePaths) - console.warn('[os-open] Failed to resolve OS-opened markdown files:', error) + state.osOpenedDocuments.restore(filePaths) + console.warn('[os-open] Failed to resolve OS-opened documents:', error) }) } @@ -102,9 +102,9 @@ if (preflightReady) { requestDesktopActivation([url]) }) // Why: macOS delivers "Open With" as open-file, often before `ready`, and only to a handler - // that claims the event. Non-markdown paths stay unclaimed so the OS default handler wins. + // that claims the event. Unsupported paths stay unclaimed. app.on('open-file', (event, filePath) => { - if (!state.osOpenedMarkdownFiles.captureFilePaths([filePath], publishOsOpenedMarkdownFiles)) { + if (!state.osOpenedDocuments.captureFilePaths([filePath], publishOsOpenedDocuments)) { return } event.preventDefault() @@ -116,7 +116,7 @@ if (preflightReady) { }) state.skillShareDeepLinks.capture(process.argv) // Why no publish: nothing is listening this early, so the first renderer pulls these on mount. - state.osOpenedMarkdownFiles.capture(process.argv) + state.osOpenedDocuments.capture(process.argv) registerMainProcessIpcHandlers() installMainProcessQuitHandlers() void app.whenReady().then(async () => { diff --git a/src/main/ipc/markdown-documents-ripgrep-real.test.ts b/src/main/ipc/markdown-documents-ripgrep-real.test.ts index 373a6502c81..6549035a5d3 100644 --- a/src/main/ipc/markdown-documents-ripgrep-real.test.ts +++ b/src/main/ipc/markdown-documents-ripgrep-real.test.ts @@ -43,6 +43,8 @@ describe('Markdown listing with bundled ripgrep', () => { ] const excluded = [ 'plain.txt', + 'data.csv', + 'data.tsv', '.md', 'docs/.MDX', '.git/hidden.md', diff --git a/src/main/ipc/markdown-documents.ts b/src/main/ipc/markdown-documents.ts index 3977999b9af..6695c7aa89c 100644 --- a/src/main/ipc/markdown-documents.ts +++ b/src/main/ipc/markdown-documents.ts @@ -1,5 +1,5 @@ import { basename as pathBasename, extname, isAbsolute, join, relative, resolve } from 'node:path' -import type { MarkdownDocument } from '../../shared/filesystem-entry-types' +import type { FileDocument, MarkdownDocument } from '../../shared/filesystem-entry-types' import { spawnBundledRipgrep } from '../ripgrep/bundled-ripgrep-spawn' import { parseWslPath } from '../wsl' import { @@ -39,11 +39,11 @@ function rootRelativePath(rootPath: string, filePath: string): string | null { return normalizeRelativePath(relativePath) } -export function markdownDocumentFromFilePath( +export function fileDocumentFromFilePath( rootPath: string, filePath: string, options: { outsideRootRelativePath?: 'basename' | 'relative' } = {} -): MarkdownDocument { +): FileDocument { const basename = pathBasename(filePath) const extension = extname(basename) const relativePath = @@ -59,6 +59,8 @@ export function markdownDocumentFromFilePath( } } +export const markdownDocumentFromFilePath = fileDocumentFromFilePath + export function markdownDocumentFromRelativePath( rootPath: string, relativePath: string diff --git a/src/main/startup/main-process-ipc-bootstrap.ts b/src/main/startup/main-process-ipc-bootstrap.ts index 6bf6533c57c..306f1d1f394 100644 --- a/src/main/startup/main-process-ipc-bootstrap.ts +++ b/src/main/startup/main-process-ipc-bootstrap.ts @@ -2,7 +2,7 @@ import { ipcMain } from 'electron' import { recoverLegacyWorkerTerminalsForRendererStartup } from './legacy-worker-renderer-recovery' import { logStartupMilestone } from './startup-diagnostics' import { mainProcessState as state } from './main-process-state' -import { resolveOpenedMarkdownDocuments } from './os-opened-markdown-files' +import { resolveOsOpenedDocuments } from './os-opened-documents' import { onStructuredAgentSessionsHeldChanged, structuredAgentSessionsHeld @@ -61,13 +61,13 @@ export function registerMainProcessIpcHandlers(): void { // cold-start "Open With" queued before mount still opens. The pull doubles as the proof // that the listener is live, which is what lets main start pushing. ipcMain.handle('ui:consumePendingMarkdownFileOpens', async () => { - state.markdownFileOpenListenerReady = true - const filePaths = state.osOpenedMarkdownFiles.consume() + state.osDocumentOpenListenerReady = true + const filePaths = state.osOpenedDocuments.consume() try { - return await resolveOpenedMarkdownDocuments(filePaths) + return await resolveOsOpenedDocuments(filePaths) } catch (error) { // Why restored: the renderer never received these, so a later mount must still get them. - state.osOpenedMarkdownFiles.restore(filePaths) + state.osOpenedDocuments.restore(filePaths) throw error } }) diff --git a/src/main/startup/main-process-state.ts b/src/main/startup/main-process-state.ts index f6e86423029..35508809164 100644 --- a/src/main/startup/main-process-state.ts +++ b/src/main/startup/main-process-state.ts @@ -37,7 +37,7 @@ import type { ServeOptions } from './main-process-serve' import type { HangDetectionMarker } from '../hang-watchdog/hang-detection-marker' import { ServeReadinessPublisher } from '../server/serve-readiness' import { SkillShareDeepLinkState } from './skill-share-deep-link-state' -import { OsOpenedMarkdownFileState } from './os-opened-markdown-files' +import { OsOpenedDocumentState } from './os-opened-documents' import { DEFAULT_GPU_CRASH_FALLBACK_THRESHOLD, DEFAULT_GPU_CRASH_FALLBACK_WINDOW_MS, @@ -115,11 +115,11 @@ export const mainProcessState = { pendingOpenSettings: createWebContentsTimedFlag(), skillShareDeepLinks: new SkillShareDeepLinkState(), // Why: a Finder/Explorer "Open With" can land before any window exists; the renderer pulls this buffer on mount. - osOpenedMarkdownFiles: new OsOpenedMarkdownFileState(), + osOpenedDocuments: new OsOpenedDocumentState(), // Why a latch and not just "a window exists": a window can be up while its renderer has not // attached the ui:openMarkdownFiles listener yet, and a push into that gap is dropped by // Electron with no error. Only the renderer's own pull proves the listener is live. - markdownFileOpenListenerReady: false, + osDocumentOpenListenerReady: false, firstWindowStartupServicesReady: Promise.resolve(), // Why published: the default-session proxy must be applied before the first app-owned fetcher, // but window creation has no reason to queue behind it (the request guard already fences it). diff --git a/src/main/startup/main-window-controller.ts b/src/main/startup/main-window-controller.ts index 941b3654f65..2e3ae99a4d4 100644 --- a/src/main/startup/main-window-controller.ts +++ b/src/main/startup/main-window-controller.ts @@ -186,7 +186,7 @@ export function openMainWindow(options: { revealOnDidFinishLoad?: boolean } = {} logStartupMilestone('did-finish-load') // Why cleared here: a reload drops the old ui:openMarkdownFiles listener, and the fresh // renderer re-attaches by pulling. Pushing into the gap between would be silently lost. - state.markdownFileOpenListenerReady = false + state.osDocumentOpenListenerReady = false const currentStore = state.store if (currentStore && resolveConsent(currentStore.getSettings()).effective === 'enabled') { trackAppOpenedOnce() diff --git a/src/main/startup/os-opened-document-arguments.test.ts b/src/main/startup/os-opened-document-arguments.test.ts new file mode 100644 index 00000000000..b4452c646eb --- /dev/null +++ b/src/main/startup/os-opened-document-arguments.test.ts @@ -0,0 +1,176 @@ +import { describe, expect, it } from 'vitest' +import { isMarkdownDocumentName } from '../ipc/markdown-documents' +import { documentPathsFromArguments } from './os-opened-documents' + +describe('documentPathsFromArguments', () => { + it('accepts CSV/TSV alongside Markdown without widening Markdown discovery', () => { + expect( + documentPathsFromArguments(['/notes/a.md', '/notes/b.CSV', '/notes/c.TsV'], 'darwin') + ).toEqual(['/notes/a.md', '/notes/b.CSV', '/notes/c.TsV']) + expect(isMarkdownDocumentName('b.csv')).toBe(false) + expect(isMarkdownDocumentName('c.tsv')).toBe(false) + }) + + it('accepts and dedupes Windows tabular paths and file URLs', () => { + expect( + documentPathsFromArguments( + [ + 'C:\\notes\\a.csv', + 'file:///C:/notes/A.csv', + '\\\\server\\share\\b.tsv', + 'file://server/share/b.tsv' + ], + 'win32' + ) + ).toEqual(['C:\\notes\\a.csv', '\\\\server\\share\\b.tsv']) + }) + + it('decodes tabular file URLs and excludes invalid or unsupported inputs', () => { + expect( + documentPathsFromArguments( + [ + 'file:///notes/query%20result.csv', + '/notes/query result.csv', + 'file:///notes/export.tsv', + 'relative.csv', + '../export.tsv', + 'https://example.com/export.csv', + 'file:///%zz.tsv', + 'file://server/share/a.csv', + '--output=/notes/a.csv', + '/notes/private.txt', + '/notes/program.js' + ], + 'linux' + ) + ).toEqual(['/notes/query result.csv', '/notes/export.tsv']) + }) + + it('keeps absolute markdown paths and drops other extensions', () => { + expect( + documentPathsFromArguments( + [ + '/Users/dev/notes/a.md', + '/Users/dev/notes/b.markdown', + '/Users/dev/notes/c.mdx', + '/Users/dev/notes/d.txt', + '/Users/dev/src/e.tsx', + '/Users/dev/notes/README' + ], + 'darwin' + ) + ).toEqual(['/Users/dev/notes/a.md', '/Users/dev/notes/b.markdown', '/Users/dev/notes/c.mdx']) + }) + + it('drops switches, including Chromium-style ones that would otherwise look like values', () => { + expect( + documentPathsFromArguments( + ['--serve', '-v', '--allow-file-access-from-files', '/Users/dev/notes/a.md'], + 'darwin' + ) + ).toEqual(['/Users/dev/notes/a.md']) + }) + + it('drops the executable and dev entries because none of them end in a markdown extension', () => { + const nonDocumentEntries = [ + '/Applications/Orca.app/Contents/MacOS/Orca', + '/Users/dev/orca/out/main/index.js', + '/Applications/Orca.app/Contents/Resources/app.asar' + ] + // The module documents that the extension check alone excludes these; hold it to that. + for (const entry of nonDocumentEntries) { + expect(isMarkdownDocumentName(entry), entry).toBe(false) + } + expect( + documentPathsFromArguments([...nonDocumentEntries, '/Users/dev/notes/a.md'], 'darwin') + ).toEqual(['/Users/dev/notes/a.md']) + }) + + it('drops relative paths because a second instance has no meaningful cwd', () => { + expect( + documentPathsFromArguments(['readme.md', './docs/a.md', '../up.md', ''], 'darwin') + ).toEqual([]) + }) + + it('accepts win32 drive-letter and UNC paths', () => { + expect( + documentPathsFromArguments( + ['C:\\Users\\dev\\todo.md', '\\\\server\\share\\a.md', 'C:\\Users\\dev\\todo.txt'], + 'win32' + ) + ).toEqual(['C:\\Users\\dev\\todo.md', '\\\\server\\share\\a.md']) + }) + + it('dedupes case-insensitively on win32 and keeps the first spelling', () => { + expect(documentPathsFromArguments(['C:\\notes\\A.md', 'c:\\notes\\a.md'], 'win32')).toEqual([ + 'C:\\notes\\A.md' + ]) + }) + + it('normalizes parent segments before deduping', () => { + expect( + documentPathsFromArguments(['C:\\notes\\sub\\..\\a.md', 'C:\\notes\\a.md'], 'win32') + ).toEqual(['C:\\notes\\a.md']) + expect(documentPathsFromArguments(['/docs/../notes/a.md', '/notes/a.md'], 'darwin')).toEqual([ + '/notes/a.md' + ]) + }) + + it('does not dedupe case-insensitively on posix, where casing is a different file', () => { + expect(documentPathsFromArguments(['/a/A.md', '/a/a.md'], 'linux')).toEqual([ + '/a/A.md', + '/a/a.md' + ]) + }) + + it('accepts a file:// URI, which the desktop entry %U field code permits', () => { + // Why defensive rather than load-bearing: GLib decodes a local file:// URI to a plain + // path before spawning (measured on Ubuntu 24.04), so Linux hits the plain-path branch + // today. The %U spec still allows a URI, and a launcher that passes one literally would + // otherwise be dropped without a trace. + expect( + documentPathsFromArguments( + ['file:///home/me/notes/a.md', 'file:///home/me/notes/b.txt'], + 'linux' + ) + ).toEqual(['/home/me/notes/a.md']) + }) + + it('percent-decodes a file:// URI so a path with spaces still opens', () => { + expect(documentPathsFromArguments(['file:///home/me/design%20notes.md'], 'linux')).toEqual([ + '/home/me/design notes.md' + ]) + }) + + it('decodes win32 file:// URIs, including UNC authority form', () => { + expect( + documentPathsFromArguments( + ['file:///C:/Users/me/todo.md', 'file://server/share/a.md'], + 'win32' + ) + ).toEqual(['C:\\Users\\me\\todo.md', '\\\\server\\share\\a.md']) + }) + + it('dedupes a path delivered as both a URI and a bare path', () => { + expect(documentPathsFromArguments(['file:///home/me/a.md', '/home/me/a.md'], 'linux')).toEqual([ + '/home/me/a.md' + ]) + }) + + it('drops a malformed or non-file URL instead of throwing', () => { + expect(() => + documentPathsFromArguments(['file://', 'file:///%zz.md', 'https://example.com/a.md'], 'linux') + ).not.toThrow() + expect( + documentPathsFromArguments(['file://', 'file:///%zz.md', 'https://example.com/a.md'], 'linux') + ).toEqual([]) + }) + + it('honours the platform argument rather than the host OS', () => { + const argv = ['C:\\notes\\a.md', '/notes/b.md'] + // Same argv, two platforms: a win32 path is not absolute to posix, and posix input is + // renormalized to backslashes on win32. Neither result may depend on where the suite runs. + expect(documentPathsFromArguments(argv, 'darwin')).toEqual(['/notes/b.md']) + expect(documentPathsFromArguments(argv, 'win32')).toEqual(['C:\\notes\\a.md', '\\notes\\b.md']) + }) +}) diff --git a/src/main/startup/os-opened-markdown-delivery.test.ts b/src/main/startup/os-opened-document-delivery.test.ts similarity index 88% rename from src/main/startup/os-opened-markdown-delivery.test.ts rename to src/main/startup/os-opened-document-delivery.test.ts index 0647516e3fa..283a9d3b2e4 100644 --- a/src/main/startup/os-opened-markdown-delivery.test.ts +++ b/src/main/startup/os-opened-document-delivery.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from 'vitest' -import { OsOpenedMarkdownFileState } from './os-opened-markdown-files' +import { OsOpenedDocumentState } from './os-opened-documents' /** * The two ways a queued "Open With" can be lost between main and the renderer. Both are @@ -7,7 +7,7 @@ import { OsOpenedMarkdownFileState } from './os-opened-markdown-files' */ describe('os-opened markdown delivery ownership', () => { it('keeps the batch when resolution rejects on the pull path', async () => { - const state = new OsOpenedMarkdownFileState() + const state = new OsOpenedDocumentState() state.captureFilePaths(['/notes/a.md']) const resolve = vi.fn().mockRejectedValue(new Error('floating root unavailable')) @@ -28,11 +28,11 @@ describe('os-opened markdown delivery ownership', () => { }) it('holds the batch while the renderer listener is not yet attached', () => { - const state = new OsOpenedMarkdownFileState() + const state = new OsOpenedDocumentState() const send = vi.fn() let listenerReady = false - // Mirrors publishOsOpenedMarkdownFiles()'s guard. + // Mirrors publishOsOpenedDocuments()'s guard. const publish = (): void => { if (!listenerReady) { return @@ -56,7 +56,7 @@ describe('os-opened markdown delivery ownership', () => { }) it('restores a batch the window could no longer receive', () => { - const state = new OsOpenedMarkdownFileState() + const state = new OsOpenedDocumentState() state.captureFilePaths(['/notes/a.md']) const filePaths = state.consume() diff --git a/src/main/startup/os-opened-document-resolution.test.ts b/src/main/startup/os-opened-document-resolution.test.ts new file mode 100644 index 00000000000..1267d51e5db --- /dev/null +++ b/src/main/startup/os-opened-document-resolution.test.ts @@ -0,0 +1,91 @@ +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { resolveOsOpenedDocuments } from './os-opened-documents' + +vi.mock('../ipc/filesystem-auth', () => ({ + authorizeExternalPath: vi.fn() +})) +vi.mock('../ipc/floating-workspace-directory', () => ({ + ensureDefaultFloatingWorkspacePath: vi.fn() +})) + +const { authorizeExternalPath } = await import('../ipc/filesystem-auth') +const { ensureDefaultFloatingWorkspacePath } = await import('../ipc/floating-workspace-directory') + +describe('resolveOsOpenedDocuments', () => { + let floatingRoot: string + let fileRoot: string + + beforeEach(async () => { + vi.mocked(authorizeExternalPath).mockClear() + vi.mocked(ensureDefaultFloatingWorkspacePath).mockClear() + floatingRoot = await mkdtemp(join(tmpdir(), 'orca-os-open-root-')) + fileRoot = await mkdtemp(join(tmpdir(), 'orca-os-open-files-')) + vi.mocked(ensureDefaultFloatingWorkspacePath).mockResolvedValue(floatingRoot) + }) + + afterEach(async () => { + await rm(floatingRoot, { recursive: true, force: true }) + await rm(fileRoot, { recursive: true, force: true }) + }) + + it.each(['md', 'csv', 'tsv'])( + 'resolves a real %s file outside the floating root', + async (extension) => { + const basename = `design notes.${extension}` + const filePath = join(fileRoot, basename) + await writeFile(filePath, '# hi\n', 'utf8') + + const documents = await resolveOsOpenedDocuments([filePath]) + + expect(documents).toEqual([ + { + filePath, + relativePath: basename, + basename, + name: 'design notes' + } + ]) + expect(authorizeExternalPath).toHaveBeenCalledWith(filePath) + } + ) + + it('never authorizes unsupported or relative files even when they exist', async () => { + const filePath = join(fileRoot, 'private.txt') + await writeFile(filePath, 'private') + expect(await resolveOsOpenedDocuments([filePath, 'relative.csv', 'file:///%zz.tsv'])).toEqual( + [] + ) + expect(authorizeExternalPath).not.toHaveBeenCalled() + expect(ensureDefaultFloatingWorkspacePath).not.toHaveBeenCalled() + }) + + it('drops a directory that merely looks like a markdown file', async () => { + const bundlePath = join(fileRoot, 'bundle.tsv') + await mkdir(bundlePath) + const filePath = join(fileRoot, 'real.md') + await writeFile(filePath, '# hi\n', 'utf8') + + const documents = await resolveOsOpenedDocuments([bundlePath, filePath]) + + expect(documents.map((document) => document.filePath)).toEqual([filePath]) + // Security contract: a path we never validated must never be authorized for renderer reads. + expect(authorizeExternalPath).toHaveBeenCalledTimes(1) + expect(authorizeExternalPath).toHaveBeenCalledWith(filePath) + }) + + it('drops a path that no longer exists without authorizing it', async () => { + const missingPath = join(fileRoot, 'gone.csv') + + expect(await resolveOsOpenedDocuments([missingPath])).toEqual([]) + expect(authorizeExternalPath).not.toHaveBeenCalled() + }) + + it('returns nothing for an empty input without touching the filesystem', async () => { + expect(await resolveOsOpenedDocuments([])).toEqual([]) + expect(ensureDefaultFloatingWorkspacePath).not.toHaveBeenCalled() + expect(authorizeExternalPath).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/startup/os-opened-markdown-wiring.test.ts b/src/main/startup/os-opened-document-wiring.test.ts similarity index 81% rename from src/main/startup/os-opened-markdown-wiring.test.ts rename to src/main/startup/os-opened-document-wiring.test.ts index 179ca0820ca..59972195b39 100644 --- a/src/main/startup/os-opened-markdown-wiring.test.ts +++ b/src/main/startup/os-opened-document-wiring.test.ts @@ -14,7 +14,7 @@ describe('os-opened markdown wiring', () => { it('captures argv before the serve-duplicate early return', () => { const captureIndex = index.indexOf( - 'state.osOpenedMarkdownFiles.capture(argv, publishOsOpenedMarkdownFiles)' + 'state.osOpenedDocuments.capture(argv, publishOsOpenedDocuments)' ) const serveGuardIndex = index.indexOf('if (!shouldActivateDesktopForSecondInstance(argv)) {') @@ -37,21 +37,21 @@ describe('os-opened markdown wiring', () => { }) it('captures the cold-start argv and lets the renderer pull it after mount', () => { - expect(index).toContain('state.osOpenedMarkdownFiles.capture(process.argv)') + expect(index).toContain('state.osOpenedDocuments.capture(process.argv)') expect(bootstrap).toContain("ipcMain.handle('ui:consumePendingMarkdownFileOpens'") }) // Why: `webContents.send` to a renderer that has not attached the listener is dropped with no // error, so publishing on "a window exists" alone would consume the queue into a void. it('only pushes once the renderer has proven its listener is attached', () => { - expect(index).toContain('!state.markdownFileOpenListenerReady') - expect(bootstrap).toContain('state.markdownFileOpenListenerReady = true') + expect(index).toContain('!state.osDocumentOpenListenerReady') + expect(bootstrap).toContain('state.osDocumentOpenListenerReady = true') // A reload drops the listener; the fresh renderer re-proves itself by pulling again. - expect(controller).toContain('state.markdownFileOpenListenerReady = false') + expect(controller).toContain('state.osDocumentOpenListenerReady = false') }) it('restores an undelivered batch on both the push and the pull path', () => { - expect(index).toContain('state.osOpenedMarkdownFiles.restore(filePaths)') - expect(bootstrap).toContain('state.osOpenedMarkdownFiles.restore(filePaths)') + expect(index).toContain('state.osOpenedDocuments.restore(filePaths)') + expect(bootstrap).toContain('state.osOpenedDocuments.restore(filePaths)') }) }) diff --git a/src/main/startup/os-opened-documents.test.ts b/src/main/startup/os-opened-documents.test.ts new file mode 100644 index 00000000000..bdbcdd10e9d --- /dev/null +++ b/src/main/startup/os-opened-documents.test.ts @@ -0,0 +1,179 @@ +import { resolve, sep } from 'node:path' +import { describe, expect, it, vi } from 'vitest' +import { MAX_PENDING_OS_OPENED_DOCUMENTS, OsOpenedDocumentState } from './os-opened-documents' + +// Why resolve(): the state uses the host platform by default, so fixture paths must already be +// spelled the way the host's path module normalizes them (`\n\a.md` and a drive on Windows). +const hostPath = (name: string): string => resolve(sep, 'notes', name) + +describe('OsOpenedDocumentState', () => { + it('reports no capture and does not publish when argv carries no markdown', () => { + const state = new OsOpenedDocumentState() + const publish = vi.fn() + + expect(state.capture(['/Applications/Orca.app/Contents/MacOS/Orca', '--serve'], publish)).toBe( + false + ) + expect(publish).not.toHaveBeenCalled() + expect(state.consume()).toEqual([]) + }) + + it('buffers and publishes when argv carries markdown', () => { + const state = new OsOpenedDocumentState() + const publish = vi.fn() + const filePath = hostPath('a.csv') + + expect(state.capture(['/Applications/Orca.app/Contents/MacOS/Orca', filePath], publish)).toBe( + true + ) + expect(publish).toHaveBeenCalledTimes(1) + expect(state.consume()).toEqual([filePath]) + }) + + it('captures a single macOS open-file path', () => { + const state = new OsOpenedDocumentState() + const publish = vi.fn() + const filePath = hostPath('a.tsv') + + expect(state.captureFilePaths([filePath], publish)).toBe(true) + expect(state.captureFilePaths([hostPath('a.png')], publish)).toBe(false) + expect(publish).toHaveBeenCalledTimes(1) + expect(state.consume()).toEqual([filePath]) + }) + + it('does not duplicate a path captured twice', () => { + const state = new OsOpenedDocumentState() + const filePath = hostPath('a.csv') + + state.captureFilePaths([filePath]) + state.captureFilePaths([filePath]) + state.capture(['orca', filePath]) + + expect(state.consume()).toEqual([filePath]) + }) + + it('drains the buffer on consume', () => { + const state = new OsOpenedDocumentState() + const paths = [hostPath('a.md'), hostPath('b.md')] + state.captureFilePaths(paths) + + expect(state.consume()).toEqual(paths) + expect(state.consume()).toEqual([]) + }) + + it('restores an undelivered batch at the front of the buffer', () => { + const state = new OsOpenedDocumentState() + state.captureFilePaths([hostPath('later.md')]) + + state.restore([hostPath('undelivered.md')]) + + expect(state.consume()).toEqual([hostPath('undelivered.md'), hostPath('later.md')]) + }) + + it('caps the buffer when captures overflow it', () => { + const state = new OsOpenedDocumentState() + const overflow = MAX_PENDING_OS_OPENED_DOCUMENTS + 5 + const paths = Array.from({ length: overflow }, (_, index) => + hostPath(`file-${index}.${index % 2 === 0 ? 'csv' : 'tsv'}`) + ) + + expect(state.captureFilePaths(paths)).toBe(true) + + expect(state.consume()).toEqual(paths.slice(0, MAX_PENDING_OS_OPENED_DOCUMENTS)) + }) + + it('caps the buffer when a restore overflows it', () => { + const state = new OsOpenedDocumentState() + state.captureFilePaths([hostPath('pending.md')]) + const restored = Array.from({ length: MAX_PENDING_OS_OPENED_DOCUMENTS }, (_, index) => + hostPath(`restored-${index}.md`) + ) + + state.restore(restored) + + const pending = state.consume() + expect(pending).toHaveLength(MAX_PENDING_OS_OPENED_DOCUMENTS) + expect(pending).toEqual(restored) + }) +}) + +describe('OsOpenedDocumentState delivery cap', () => { + it('stops merging an OS file batch at the pending delivery cap', () => { + const state = new OsOpenedDocumentState() + const includes = vi.spyOn(Array.prototype, 'includes') + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + let probes: number + try { + state.captureFilePaths( + Array.from({ length: 10000 }, (_, index) => resolve(`/notes/${index}.md`)) + ) + probes = includes.mock.calls.length + } finally { + includes.mockRestore() + warn.mockRestore() + } + expect(probes).toBeLessThan(100) + expect(state.consume()).toEqual( + Array.from({ length: MAX_PENDING_OS_OPENED_DOCUMENTS }, (_, index) => + resolve(`/notes/${index}.md`) + ) + ) + }) + + // Why: the cap drops files the user explicitly asked to open. Pin which end is + // dropped (the tail, in shell order) and that the loss is reported, not silent. + it('keeps the first paths in shell order and reports the dropped tail', () => { + const state = new OsOpenedDocumentState() + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const total = MAX_PENDING_OS_OPENED_DOCUMENTS + 8 + const paths = Array.from({ length: total }, (_, index) => + resolve(`/notes/${String(index).padStart(3, '0')}.md`) + ) + try { + expect(state.captureFilePaths(paths)).toBe(true) + expect(warn).toHaveBeenCalledWith( + expect.stringContaining(`Dropped 8 of ${total} OS-opened documents`) + ) + } finally { + warn.mockRestore() + } + expect(state.consume()).toEqual(paths.slice(0, MAX_PENDING_OS_OPENED_DOCUMENTS)) + }) + + it('stays silent for a batch that fits under the cap', () => { + const state = new OsOpenedDocumentState() + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + state.captureFilePaths( + Array.from({ length: MAX_PENDING_OS_OPENED_DOCUMENTS }, (_, index) => + resolve(`/notes/${index}.md`) + ) + ) + expect(warn).not.toHaveBeenCalled() + } finally { + warn.mockRestore() + } + }) + + it('reports a drop when an already-full queue rejects a later batch', () => { + const state = new OsOpenedDocumentState() + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + state.captureFilePaths( + Array.from({ length: MAX_PENDING_OS_OPENED_DOCUMENTS }, (_, index) => + resolve(`/first/${index}.md`) + ) + ) + expect(warn).not.toHaveBeenCalled() + state.captureFilePaths([resolve('/second/a.md'), resolve('/second/b.md')]) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('Dropped 2 of 2')) + } finally { + warn.mockRestore() + } + expect(state.consume()).toEqual( + Array.from({ length: MAX_PENDING_OS_OPENED_DOCUMENTS }, (_, index) => + resolve(`/first/${index}.md`) + ) + ) + }) +}) diff --git a/src/main/startup/os-opened-markdown-files.ts b/src/main/startup/os-opened-documents.ts similarity index 72% rename from src/main/startup/os-opened-markdown-files.ts rename to src/main/startup/os-opened-documents.ts index 144f59fe578..ec2b9c20e76 100644 --- a/src/main/startup/os-opened-markdown-files.ts +++ b/src/main/startup/os-opened-documents.ts @@ -1,14 +1,19 @@ import { stat } from 'node:fs/promises' import path from 'node:path' import { fileURLToPath } from 'node:url' -import type { MarkdownDocument } from '../../shared/filesystem-entry-types' +import type { FileDocument } from '../../shared/filesystem-entry-types' import { authorizeExternalPath } from '../ipc/filesystem-auth' import { ensureDefaultFloatingWorkspacePath } from '../ipc/floating-workspace-directory' -import { isMarkdownDocumentName, markdownDocumentFromFilePath } from '../ipc/markdown-documents' +import { fileDocumentFromFilePath, isMarkdownDocumentName } from '../ipc/markdown-documents' + +export function isOsOpenedDocumentName(name: string): boolean { + const extension = path.extname(name).toLowerCase() + return isMarkdownDocumentName(name) || extension === '.csv' || extension === '.tsv' +} // Why: a shell can only ever hand over the files the user selected; anything past this is a // runaway argv, and buffering it unbounded would pin the paths for the whole session. -export const MAX_PENDING_OS_OPENED_MARKDOWN_FILES = 32 +export const MAX_PENDING_OS_OPENED_DOCUMENTS = 32 /** * Resolves one argv entry to a local absolute path, or null if it is not one. @@ -35,13 +40,13 @@ function localPathFromArgument(argument: string, platform: NodeJS.Platform): str } /** - * Absolute markdown paths an OS "Open With" put on a launch or second-instance argv. + * Absolute document paths an OS "Open With" put on a launch or second-instance argv. * - * Why no executable/asar/dev-entry filtering: none of those argv entries end in a markdown + * Why no executable/asar/dev-entry filtering: none of those argv entries end in a supported * extension, so the extension check already excludes them. Relative entries are dropped * because the shell always passes absolute paths and `cwd` is meaningless for a second instance. */ -export function markdownPathsFromArguments( +export function documentPathsFromArguments( argv: readonly string[], platform: NodeJS.Platform = process.platform ): string[] { @@ -53,7 +58,7 @@ export function markdownPathsFromArguments( continue } const argument = localPathFromArgument(rawArgument, platform) - if (!argument || !isMarkdownDocumentName(argument)) { + if (!argument || !isOsOpenedDocumentName(argument)) { continue } const normalized = pathApi.normalize(argument) @@ -70,23 +75,23 @@ export function markdownPathsFromArguments( } /** - * Buffers markdown paths the OS handed us until a renderer can receive them. + * Buffers document paths the OS handed us until a renderer can receive them. * * Mirrors SkillShareDeepLinkState: main pushes when a window is already live, and the * renderer pulls the same buffer when its listener attaches, so a cold-start "Open With" * that lands before mount is not dropped. */ -export class OsOpenedMarkdownFileState { +export class OsOpenedDocumentState { private pending: string[] = [] - /** Returns true when argv carried at least one markdown path. */ + /** Returns true when argv carried at least one supported document path. */ capture(argv: readonly string[], publish?: () => void): boolean { - return this.add(markdownPathsFromArguments(argv), publish) + return this.add(documentPathsFromArguments(argv), publish) } - /** Returns true when at least one path was a markdown document. */ + /** Returns true when at least one path was a supported document. */ captureFilePaths(filePaths: readonly string[], publish?: () => void): boolean { - return this.add(markdownPathsFromArguments(filePaths), publish) + return this.add(documentPathsFromArguments(filePaths), publish) } consume(): string[] { @@ -97,7 +102,7 @@ export class OsOpenedMarkdownFileState { /** Puts an undelivered batch back at the front so the next renderer still receives it. */ restore(filePaths: readonly string[]): void { - this.pending = [...filePaths, ...this.pending].slice(0, MAX_PENDING_OS_OPENED_MARKDOWN_FILES) + this.pending = [...filePaths, ...this.pending].slice(0, MAX_PENDING_OS_OPENED_DOCUMENTS) } private add(filePaths: readonly string[], publish?: () => void): boolean { @@ -107,7 +112,7 @@ export class OsOpenedMarkdownFileState { const merged = [...this.pending] let index = 0 for (; index < filePaths.length; index++) { - if (merged.length >= MAX_PENDING_OS_OPENED_MARKDOWN_FILES) { + if (merged.length >= MAX_PENDING_OS_OPENED_DOCUMENTS) { break } const filePath = filePaths[index]! @@ -119,30 +124,34 @@ export class OsOpenedMarkdownFileState { // Why logged: the cap drops the tail of an oversized selection, and a file the // user explicitly asked to open must not vanish without leaving a trace. console.warn( - `[os-open] Dropped ${filePaths.length - index} of ${filePaths.length} OS-opened markdown files; the pending queue is capped at ${MAX_PENDING_OS_OPENED_MARKDOWN_FILES}.` + `[os-open] Dropped ${filePaths.length - index} of ${filePaths.length} OS-opened documents; the pending queue is capped at ${MAX_PENDING_OS_OPENED_DOCUMENTS}.` ) } - this.pending = merged.slice(0, MAX_PENDING_OS_OPENED_MARKDOWN_FILES) + this.pending = merged.slice(0, MAX_PENDING_OS_OPENED_DOCUMENTS) publish?.() return true } } /** - * Turns OS-handed paths into the same `MarkdownDocument` shape the floating workspace's own + * Turns OS-handed paths into the same document shape the floating workspace's own * file picker produces, authorizing each one for the renderer's later read. */ -export async function resolveOpenedMarkdownDocuments( +export async function resolveOsOpenedDocuments( filePaths: readonly string[] -): Promise { - if (filePaths.length === 0) { +): Promise { + const supportedPaths = documentPathsFromArguments(filePaths).slice( + 0, + MAX_PENDING_OS_OPENED_DOCUMENTS + ) + if (supportedPaths.length === 0) { return [] } const floatingRoot = await ensureDefaultFloatingWorkspacePath() - const documents: MarkdownDocument[] = [] - for (const filePath of filePaths) { + const documents: FileDocument[] = [] + for (const filePath of supportedPaths) { try { - // Why: the shell can hand over a bundle directory named `*.md`, or a path already + // Why: the shell can hand over a directory named like a document, or a path already // deleted by the time we resolve. Authorize only something that is really a file. if (!(await stat(filePath)).isFile()) { continue @@ -152,7 +161,7 @@ export async function resolveOpenedMarkdownDocuments( } authorizeExternalPath(filePath) documents.push( - markdownDocumentFromFilePath(floatingRoot, filePath, { + fileDocumentFromFilePath(floatingRoot, filePath, { outsideRootRelativePath: 'basename' }) ) diff --git a/src/main/startup/os-opened-markdown-files.test.ts b/src/main/startup/os-opened-markdown-files.test.ts deleted file mode 100644 index c980b6258d2..00000000000 --- a/src/main/startup/os-opened-markdown-files.test.ts +++ /dev/null @@ -1,387 +0,0 @@ -import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises' -import { tmpdir } from 'node:os' -import { join, resolve, sep } from 'node:path' -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -import { isMarkdownDocumentName } from '../ipc/markdown-documents' -import { - MAX_PENDING_OS_OPENED_MARKDOWN_FILES, - OsOpenedMarkdownFileState, - markdownPathsFromArguments, - resolveOpenedMarkdownDocuments -} from './os-opened-markdown-files' - -vi.mock('../ipc/filesystem-auth', () => ({ - authorizeExternalPath: vi.fn() -})) -vi.mock('../ipc/floating-workspace-directory', () => ({ - ensureDefaultFloatingWorkspacePath: vi.fn() -})) - -const { authorizeExternalPath } = await import('../ipc/filesystem-auth') -const { ensureDefaultFloatingWorkspacePath } = await import('../ipc/floating-workspace-directory') - -describe('markdownPathsFromArguments', () => { - it('keeps absolute markdown paths and drops other extensions', () => { - expect( - markdownPathsFromArguments( - [ - '/Users/dev/notes/a.md', - '/Users/dev/notes/b.markdown', - '/Users/dev/notes/c.mdx', - '/Users/dev/notes/d.txt', - '/Users/dev/src/e.tsx', - '/Users/dev/notes/README' - ], - 'darwin' - ) - ).toEqual(['/Users/dev/notes/a.md', '/Users/dev/notes/b.markdown', '/Users/dev/notes/c.mdx']) - }) - - it('drops switches, including Chromium-style ones that would otherwise look like values', () => { - expect( - markdownPathsFromArguments( - ['--serve', '-v', '--allow-file-access-from-files', '/Users/dev/notes/a.md'], - 'darwin' - ) - ).toEqual(['/Users/dev/notes/a.md']) - }) - - it('drops the executable and dev entries because none of them end in a markdown extension', () => { - const nonDocumentEntries = [ - '/Applications/Orca.app/Contents/MacOS/Orca', - '/Users/dev/orca/out/main/index.js', - '/Applications/Orca.app/Contents/Resources/app.asar' - ] - // The module documents that the extension check alone excludes these; hold it to that. - for (const entry of nonDocumentEntries) { - expect(isMarkdownDocumentName(entry), entry).toBe(false) - } - expect( - markdownPathsFromArguments([...nonDocumentEntries, '/Users/dev/notes/a.md'], 'darwin') - ).toEqual(['/Users/dev/notes/a.md']) - }) - - it('drops relative paths because a second instance has no meaningful cwd', () => { - expect( - markdownPathsFromArguments(['readme.md', './docs/a.md', '../up.md', ''], 'darwin') - ).toEqual([]) - }) - - it('accepts win32 drive-letter and UNC paths', () => { - expect( - markdownPathsFromArguments( - ['C:\\Users\\dev\\todo.md', '\\\\server\\share\\a.md', 'C:\\Users\\dev\\todo.txt'], - 'win32' - ) - ).toEqual(['C:\\Users\\dev\\todo.md', '\\\\server\\share\\a.md']) - }) - - it('dedupes case-insensitively on win32 and keeps the first spelling', () => { - expect(markdownPathsFromArguments(['C:\\notes\\A.md', 'c:\\notes\\a.md'], 'win32')).toEqual([ - 'C:\\notes\\A.md' - ]) - }) - - it('normalizes parent segments before deduping', () => { - expect( - markdownPathsFromArguments(['C:\\notes\\sub\\..\\a.md', 'C:\\notes\\a.md'], 'win32') - ).toEqual(['C:\\notes\\a.md']) - expect(markdownPathsFromArguments(['/docs/../notes/a.md', '/notes/a.md'], 'darwin')).toEqual([ - '/notes/a.md' - ]) - }) - - it('does not dedupe case-insensitively on posix, where casing is a different file', () => { - expect(markdownPathsFromArguments(['/a/A.md', '/a/a.md'], 'linux')).toEqual([ - '/a/A.md', - '/a/a.md' - ]) - }) - - it('accepts a file:// URI, which the desktop entry %U field code permits', () => { - // Why defensive rather than load-bearing: GLib decodes a local file:// URI to a plain - // path before spawning (measured on Ubuntu 24.04), so Linux hits the plain-path branch - // today. The %U spec still allows a URI, and a launcher that passes one literally would - // otherwise be dropped without a trace. - expect( - markdownPathsFromArguments( - ['file:///home/me/notes/a.md', 'file:///home/me/notes/b.txt'], - 'linux' - ) - ).toEqual(['/home/me/notes/a.md']) - }) - - it('percent-decodes a file:// URI so a path with spaces still opens', () => { - expect(markdownPathsFromArguments(['file:///home/me/design%20notes.md'], 'linux')).toEqual([ - '/home/me/design notes.md' - ]) - }) - - it('decodes win32 file:// URIs, including UNC authority form', () => { - expect( - markdownPathsFromArguments( - ['file:///C:/Users/me/todo.md', 'file://server/share/a.md'], - 'win32' - ) - ).toEqual(['C:\\Users\\me\\todo.md', '\\\\server\\share\\a.md']) - }) - - it('dedupes a path delivered as both a URI and a bare path', () => { - expect(markdownPathsFromArguments(['file:///home/me/a.md', '/home/me/a.md'], 'linux')).toEqual([ - '/home/me/a.md' - ]) - }) - - it('drops a malformed or non-file URL instead of throwing', () => { - expect(() => - markdownPathsFromArguments(['file://', 'file:///%zz.md', 'https://example.com/a.md'], 'linux') - ).not.toThrow() - expect( - markdownPathsFromArguments(['file://', 'file:///%zz.md', 'https://example.com/a.md'], 'linux') - ).toEqual([]) - }) - - it('honours the platform argument rather than the host OS', () => { - const argv = ['C:\\notes\\a.md', '/notes/b.md'] - // Same argv, two platforms: a win32 path is not absolute to posix, and posix input is - // renormalized to backslashes on win32. Neither result may depend on where the suite runs. - expect(markdownPathsFromArguments(argv, 'darwin')).toEqual(['/notes/b.md']) - expect(markdownPathsFromArguments(argv, 'win32')).toEqual(['C:\\notes\\a.md', '\\notes\\b.md']) - }) -}) - -// Why resolve(): the state uses the host platform by default, so fixture paths must already be -// spelled the way the host's path module normalizes them (`\n\a.md` and a drive on Windows). -const hostPath = (name: string): string => resolve(sep, 'notes', name) - -describe('OsOpenedMarkdownFileState', () => { - it('reports no capture and does not publish when argv carries no markdown', () => { - const state = new OsOpenedMarkdownFileState() - const publish = vi.fn() - - expect(state.capture(['/Applications/Orca.app/Contents/MacOS/Orca', '--serve'], publish)).toBe( - false - ) - expect(publish).not.toHaveBeenCalled() - expect(state.consume()).toEqual([]) - }) - - it('buffers and publishes when argv carries markdown', () => { - const state = new OsOpenedMarkdownFileState() - const publish = vi.fn() - const filePath = hostPath('a.md') - - expect(state.capture(['/Applications/Orca.app/Contents/MacOS/Orca', filePath], publish)).toBe( - true - ) - expect(publish).toHaveBeenCalledTimes(1) - expect(state.consume()).toEqual([filePath]) - }) - - it('captures a single macOS open-file path', () => { - const state = new OsOpenedMarkdownFileState() - const publish = vi.fn() - const filePath = hostPath('a.md') - - expect(state.captureFilePaths([filePath], publish)).toBe(true) - expect(state.captureFilePaths([hostPath('a.png')], publish)).toBe(false) - expect(publish).toHaveBeenCalledTimes(1) - expect(state.consume()).toEqual([filePath]) - }) - - it('does not duplicate a path captured twice', () => { - const state = new OsOpenedMarkdownFileState() - const filePath = hostPath('a.md') - - state.captureFilePaths([filePath]) - state.captureFilePaths([filePath]) - state.capture(['orca', filePath]) - - expect(state.consume()).toEqual([filePath]) - }) - - it('drains the buffer on consume', () => { - const state = new OsOpenedMarkdownFileState() - const paths = [hostPath('a.md'), hostPath('b.md')] - state.captureFilePaths(paths) - - expect(state.consume()).toEqual(paths) - expect(state.consume()).toEqual([]) - }) - - it('restores an undelivered batch at the front of the buffer', () => { - const state = new OsOpenedMarkdownFileState() - state.captureFilePaths([hostPath('later.md')]) - - state.restore([hostPath('undelivered.md')]) - - expect(state.consume()).toEqual([hostPath('undelivered.md'), hostPath('later.md')]) - }) - - it('caps the buffer when captures overflow it', () => { - const state = new OsOpenedMarkdownFileState() - const overflow = MAX_PENDING_OS_OPENED_MARKDOWN_FILES + 5 - const paths = Array.from({ length: overflow }, (_, index) => hostPath(`file-${index}.md`)) - - expect(state.captureFilePaths(paths)).toBe(true) - - expect(state.consume()).toEqual(paths.slice(0, MAX_PENDING_OS_OPENED_MARKDOWN_FILES)) - }) - - it('caps the buffer when a restore overflows it', () => { - const state = new OsOpenedMarkdownFileState() - state.captureFilePaths([hostPath('pending.md')]) - const restored = Array.from({ length: MAX_PENDING_OS_OPENED_MARKDOWN_FILES }, (_, index) => - hostPath(`restored-${index}.md`) - ) - - state.restore(restored) - - const pending = state.consume() - expect(pending).toHaveLength(MAX_PENDING_OS_OPENED_MARKDOWN_FILES) - expect(pending).toEqual(restored) - }) -}) - -describe('resolveOpenedMarkdownDocuments', () => { - let floatingRoot: string - let fileRoot: string - - beforeEach(async () => { - vi.mocked(authorizeExternalPath).mockClear() - vi.mocked(ensureDefaultFloatingWorkspacePath).mockClear() - floatingRoot = await mkdtemp(join(tmpdir(), 'orca-os-open-root-')) - fileRoot = await mkdtemp(join(tmpdir(), 'orca-os-open-files-')) - vi.mocked(ensureDefaultFloatingWorkspacePath).mockResolvedValue(floatingRoot) - }) - - afterEach(async () => { - await rm(floatingRoot, { recursive: true, force: true }) - await rm(fileRoot, { recursive: true, force: true }) - }) - - it('resolves a real file outside the floating root to a basename-relative document', async () => { - const filePath = join(fileRoot, 'design notes.md') - await writeFile(filePath, '# hi\n', 'utf8') - - const documents = await resolveOpenedMarkdownDocuments([filePath]) - - expect(documents).toEqual([ - { - filePath, - relativePath: 'design notes.md', - basename: 'design notes.md', - name: 'design notes' - } - ]) - expect(authorizeExternalPath).toHaveBeenCalledWith(filePath) - }) - - it('drops a directory that merely looks like a markdown file', async () => { - const bundlePath = join(fileRoot, 'bundle.md') - await mkdir(bundlePath) - const filePath = join(fileRoot, 'real.md') - await writeFile(filePath, '# hi\n', 'utf8') - - const documents = await resolveOpenedMarkdownDocuments([bundlePath, filePath]) - - expect(documents.map((document) => document.filePath)).toEqual([filePath]) - // Security contract: a path we never validated must never be authorized for renderer reads. - expect(authorizeExternalPath).toHaveBeenCalledTimes(1) - expect(authorizeExternalPath).toHaveBeenCalledWith(filePath) - }) - - it('drops a path that no longer exists without authorizing it', async () => { - const missingPath = join(fileRoot, 'gone.md') - - expect(await resolveOpenedMarkdownDocuments([missingPath])).toEqual([]) - expect(authorizeExternalPath).not.toHaveBeenCalled() - }) - - it('returns nothing for an empty input without touching the filesystem', async () => { - expect(await resolveOpenedMarkdownDocuments([])).toEqual([]) - expect(ensureDefaultFloatingWorkspacePath).not.toHaveBeenCalled() - expect(authorizeExternalPath).not.toHaveBeenCalled() - }) -}) - -describe('OsOpenedMarkdownFileState delivery cap', () => { - it('stops merging an OS file batch at the pending delivery cap', () => { - const state = new OsOpenedMarkdownFileState() - const includes = vi.spyOn(Array.prototype, 'includes') - const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) - let probes: number - try { - state.captureFilePaths( - Array.from({ length: 10000 }, (_, index) => resolve(`/notes/${index}.md`)) - ) - probes = includes.mock.calls.length - } finally { - includes.mockRestore() - warn.mockRestore() - } - expect(probes).toBeLessThan(100) - expect(state.consume()).toEqual( - Array.from({ length: MAX_PENDING_OS_OPENED_MARKDOWN_FILES }, (_, index) => - resolve(`/notes/${index}.md`) - ) - ) - }) - - // Why: the cap drops files the user explicitly asked to open. Pin which end is - // dropped (the tail, in shell order) and that the loss is reported, not silent. - it('keeps the first paths in shell order and reports the dropped tail', () => { - const state = new OsOpenedMarkdownFileState() - const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) - const total = MAX_PENDING_OS_OPENED_MARKDOWN_FILES + 8 - const paths = Array.from({ length: total }, (_, index) => - resolve(`/notes/${String(index).padStart(3, '0')}.md`) - ) - try { - expect(state.captureFilePaths(paths)).toBe(true) - expect(warn).toHaveBeenCalledWith( - expect.stringContaining(`Dropped 8 of ${total} OS-opened markdown files`) - ) - } finally { - warn.mockRestore() - } - expect(state.consume()).toEqual(paths.slice(0, MAX_PENDING_OS_OPENED_MARKDOWN_FILES)) - }) - - it('stays silent for a batch that fits under the cap', () => { - const state = new OsOpenedMarkdownFileState() - const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) - try { - state.captureFilePaths( - Array.from({ length: MAX_PENDING_OS_OPENED_MARKDOWN_FILES }, (_, index) => - resolve(`/notes/${index}.md`) - ) - ) - expect(warn).not.toHaveBeenCalled() - } finally { - warn.mockRestore() - } - }) - - it('reports a drop when an already-full queue rejects a later batch', () => { - const state = new OsOpenedMarkdownFileState() - const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) - try { - state.captureFilePaths( - Array.from({ length: MAX_PENDING_OS_OPENED_MARKDOWN_FILES }, (_, index) => - resolve(`/first/${index}.md`) - ) - ) - expect(warn).not.toHaveBeenCalled() - state.captureFilePaths([resolve('/second/a.md'), resolve('/second/b.md')]) - expect(warn).toHaveBeenCalledWith(expect.stringContaining('Dropped 2 of 2')) - } finally { - warn.mockRestore() - } - expect(state.consume()).toEqual( - Array.from({ length: MAX_PENDING_OS_OPENED_MARKDOWN_FILES }, (_, index) => - resolve(`/first/${index}.md`) - ) - ) - }) -}) diff --git a/src/preload/api/ui-command-event-api.ts b/src/preload/api/ui-command-event-api.ts index 217e5c027e5..74692c5c472 100644 --- a/src/preload/api/ui-command-event-api.ts +++ b/src/preload/api/ui-command-event-api.ts @@ -56,7 +56,7 @@ export type UiCommandEventApi = { consumePendingOpenSettings: () => Promise onOpenSkillShare: (callback: (shareId: string) => void) => () => void consumePendingSkillShare: () => Promise - /** OS "Open With" markdown paths pushed while a renderer is already listening. */ + /** OS "Open With" Markdown/CSV/TSV documents; the local IPC name is retained. */ onOpenMarkdownFiles: (callback: (documents: MarkdownDocument[]) => void) => () => void /** Drains the "Open With" paths queued before this renderer's listener attached. */ consumePendingMarkdownFileOpens: () => Promise diff --git a/src/renderer/src/components/floating-terminal/use-floating-terminal-create-actions.ts b/src/renderer/src/components/floating-terminal/use-floating-terminal-create-actions.ts index 7d2207cca9a..65b29800796 100644 --- a/src/renderer/src/components/floating-terminal/use-floating-terminal-create-actions.ts +++ b/src/renderer/src/components/floating-terminal/use-floating-terminal-create-actions.ts @@ -4,7 +4,7 @@ import { resolveGroupTabFromVisibleId } from '@/components/tab-group/tab-group-v import { getConnectionId } from '@/lib/connection-context' import { createUntitledMarkdownFileWithTemplateSelection } from '@/lib/create-untitled-markdown' import { ensureClientCreationActionAllowed } from '@/lib/client-creation-action-error' -import { openMarkdownDocumentInFloatingWorkspace } from '@/lib/open-markdown-in-floating-workspace' +import { openDocumentInFloatingWorkspace } from '@/lib/open-document-in-floating-workspace' import { extractIpcErrorMessage } from '@/lib/ipc-error' import { focusTerminalTabSurface } from '@/lib/focus-terminal-tab-surface' import { createFloatingWorkspaceTerminalTab } from '@/lib/floating-workspace-tab-creation' @@ -111,7 +111,7 @@ export function useFloatingTerminalCreateActions({ if (!document) { return } - openMarkdownDocumentInFloatingWorkspace(openFile, document, { + openDocumentInFloatingWorkspace(openFile, document, { targetGroupId: activeGroup?.id }) } catch (error) { diff --git a/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.test.ts b/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.test.ts index d4825e89206..44e627a2881 100644 --- a/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.test.ts +++ b/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.test.ts @@ -9,16 +9,20 @@ const mocks = vi.hoisted(() => ({ openFile: vi.fn(() => 'file-1'), updateSettings: vi.fn(async () => {}), isFloatingWorkspacePanelVisible: vi.fn(() => false), - toastError: vi.fn() + toastError: vi.fn(), + subscribe: vi.fn<(listener: (state: { workspaceSessionReady: boolean }) => void) => () => void>() })) let storeState: { openFile: typeof mocks.openFile updateSettings: typeof mocks.updateSettings settings: { floatingTerminalEnabled?: boolean } | undefined + workspaceSessionReady: boolean } -vi.mock('../../store', () => ({ useAppStore: { getState: () => storeState } })) +vi.mock('../../store', () => ({ + useAppStore: { getState: () => storeState, subscribe: mocks.subscribe } +})) vi.mock('@/lib/floating-workspace-terminal-actions', () => ({ isFloatingWorkspacePanelVisible: mocks.isFloatingWorkspacePanelVisible })) @@ -70,11 +74,13 @@ describe('registerOsMarkdownFileOpenBridge', () => { storeState = { openFile: mocks.openFile, updateSettings: mocks.updateSettings, - settings: { floatingTerminalEnabled: true } + settings: { floatingTerminalEnabled: true }, + workspaceSessionReady: true } mocks.openFile.mockReturnValue('file-1') mocks.updateSettings.mockResolvedValue(undefined) mocks.isFloatingWorkspacePanelVisible.mockReturnValue(false) + mocks.subscribe.mockReset().mockReturnValue(() => {}) vi.stubGlobal('requestAnimationFrame', (callback: FrameRequestCallback) => frames.push(callback) ) @@ -82,6 +88,27 @@ describe('registerOsMarkdownFileOpenBridge', () => { process.on('unhandledRejection', recordUnhandledRejection) }) + it('leaves documents in main until startup restoration has finished', async () => { + storeState.workspaceSessionReady = false + const unsubscribe = vi.fn() + mocks.subscribe.mockReturnValue(unsubscribe) + const consume = vi.fn(async () => [markdownDocument({ relativePath: 'export.csv' })]) + stubPreload({ onOpenMarkdownFiles: () => () => {}, consumePendingMarkdownFileOpens: consume }) + const unsubs: (() => void)[] = [] + registerOsMarkdownFileOpenBridge(unsubs) + await settle() + expect(consume).not.toHaveBeenCalled() + expect(mocks.openFile).not.toHaveBeenCalled() + expect(unsubs).toEqual([unsubscribe]) + + storeState.workspaceSessionReady = true + mocks.subscribe.mock.calls[0][0](storeState) + await settle() + expect(unsubscribe).toHaveBeenCalledOnce() + expect(consume).toHaveBeenCalledOnce() + expect(mocks.openFile.mock.calls[0][0].language).toBe('csv') + }) + afterEach(() => { process.off('unhandledRejection', recordUnhandledRejection) vi.unstubAllGlobals() @@ -94,7 +121,7 @@ describe('registerOsMarkdownFileOpenBridge', () => { consumePendingMarkdownFileOpens: () => Promise.resolve([ markdownDocument(), - markdownDocument({ filePath: '/Users/me/notes/plan.md', relativePath: 'plan.md' }) + markdownDocument({ filePath: '/Users/me/notes/plan.csv', relativePath: 'plan.csv' }) ]) }) @@ -104,7 +131,7 @@ describe('registerOsMarkdownFileOpenBridge', () => { expect(mocks.openFile).toHaveBeenCalledTimes(2) expect(mocks.openFile.mock.calls.map((call) => call[0].filePath)).toEqual([ '/Users/me/notes/README.md', - '/Users/me/notes/plan.md' + '/Users/me/notes/plan.csv' ]) expect(mocks.openFile.mock.calls[0][0].worktreeId).toBe(FLOATING_TERMINAL_WORKTREE_ID) }) @@ -125,12 +152,12 @@ describe('registerOsMarkdownFileOpenBridge', () => { expect(unsubs).toEqual([unsubscribe]) listeners[0]([ - markdownDocument({ filePath: '/Users/me/notes/live.md', relativePath: 'live.md' }) + markdownDocument({ filePath: '/Users/me/notes/live.tsv', relativePath: 'live.tsv' }) ]) await settle() expect(mocks.openFile).toHaveBeenCalledTimes(1) - expect(mocks.openFile.mock.calls[0][0].filePath).toBe('/Users/me/notes/live.md') + expect(mocks.openFile.mock.calls[0][0].filePath).toBe('/Users/me/notes/live.tsv') unsubs.forEach((teardown) => teardown()) expect(unsubscribe).toHaveBeenCalledOnce() @@ -216,7 +243,7 @@ describe('registerOsMarkdownFileOpenBridge', () => { registerOsMarkdownFileOpenBridge([]) await settle() - expect(mocks.toastError).toHaveBeenCalledWith('Failed to open the Markdown file.') + expect(mocks.toastError).toHaveBeenCalledWith('Failed to open the file.') // Why: App.tsx awaits hydration around this registration and treats any throw as // "session restore failed", so the bridge must swallow its own failures. expect(unhandledRejections).toEqual([]) @@ -239,7 +266,7 @@ describe('registerOsMarkdownFileOpenBridge', () => { expect(() => listeners[0]([markdownDocument()])).not.toThrow() await settle() - expect(mocks.toastError).toHaveBeenCalledWith('Failed to open the Markdown file.') + expect(mocks.toastError).toHaveBeenCalledWith('Failed to open the file.') expect(unhandledRejections).toEqual([]) expect(dispatchEvent).not.toHaveBeenCalled() }) diff --git a/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.ts b/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.ts index 3dd345da07c..11b313c5e7f 100644 --- a/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.ts +++ b/src/renderer/src/hooks/ipc-events/os-markdown-file-open-bridge.ts @@ -1,16 +1,16 @@ import { toast } from 'sonner' -import type { MarkdownDocument } from '../../../../shared/filesystem-entry-types' +import type { FileDocument } from '../../../../shared/filesystem-entry-types' import { TOGGLE_FLOATING_TERMINAL_EVENT } from '@/lib/floating-terminal' import { isFloatingWorkspacePanelVisible } from '@/lib/floating-workspace-terminal-actions' -import { openMarkdownDocumentInFloatingWorkspace } from '@/lib/open-markdown-in-floating-workspace' +import { openDocumentInFloatingWorkspace } from '@/lib/open-document-in-floating-workspace' import { translate } from '@/i18n/i18n' import { useAppStore } from '../../store' /** - * Opens markdown files the OS shell handed to Orca ("Open With" / double-click) in the + * Opens documents the OS shell handed to Orca ("Open With" / double-click) in the * floating workspace, which is the one editor surface that needs no project. */ -async function openOsRequestedMarkdownFiles(documents: MarkdownDocument[]): Promise { +async function openOsRequestedDocuments(documents: FileDocument[]): Promise { // Why the shape check: this payload crosses the preload boundary, so a stale or mismatched // preload can hand back something that is not an array. Reading .length off that throws // inside the promise chain rather than failing loudly at the boundary. @@ -23,10 +23,10 @@ async function openOsRequestedMarkdownFiles(documents: MarkdownDocument[]): Prom // Why isolated: selecting several files hands us one batch, and one unopenable file // must not cost the user the rest of the selection. try { - openMarkdownDocumentInFloatingWorkspace(store.openFile, document) + openDocumentInFloatingWorkspace(store.openFile, document) opened += 1 } catch (error) { - reportOsRequestedMarkdownFailure(error) + reportOsRequestedDocumentFailure(error) } } if (opened === 0) { @@ -46,19 +46,26 @@ async function openOsRequestedMarkdownFiles(documents: MarkdownDocument[]): Prom }) } -function reportOsRequestedMarkdownFailure(error: unknown): void { - console.error('Failed to open markdown files requested by the OS:', error) - toast.error( - translate( - 'auto.hooks.ipc.events.os.markdown.file.open.bridge.1e9a1a63c4', - 'Failed to open the Markdown file.' - ) - ) +function reportOsRequestedDocumentFailure(error: unknown): void { + console.error('Failed to open documents requested by the OS:', error) + toast.error(translate('osDocumentOpen.failed', 'Failed to open the file.')) } export function registerOsMarkdownFileOpenBridge(unsubs: (() => void)[]): void { + // Startup restoration replaces tabs; keep OS requests in main until it finishes. + if (!useAppStore.getState().workspaceSessionReady) { + const unsubscribe = useAppStore.subscribe((state) => { + if (state.workspaceSessionReady) { + unsubscribe() + registerOsMarkdownFileOpenBridge(unsubs) + } + }) + unsubs.push(unsubscribe) + return + } + // Keep the legacy local IPC names; the payload also carries CSV/TSV documents. const unsubscribe = window.api.ui.onOpenMarkdownFiles?.((documents) => { - void openOsRequestedMarkdownFiles(documents).catch(reportOsRequestedMarkdownFailure) + void openOsRequestedDocuments(documents).catch(reportOsRequestedDocumentFailure) }) if (unsubscribe) { unsubs.push(unsubscribe) @@ -67,6 +74,6 @@ export function registerOsMarkdownFileOpenBridge(unsubs: (() => void)[]): void { // Why: a cold-start "Open With" resolves before this listener attaches; drain what main queued. const pending = window.api.ui.consumePendingMarkdownFileOpens?.() if (pending && typeof pending.then === 'function') { - void pending.then(openOsRequestedMarkdownFiles).catch(reportOsRequestedMarkdownFailure) + void pending.then(openOsRequestedDocuments).catch(reportOsRequestedDocumentFailure) } } diff --git a/src/renderer/src/i18n/en-runtime-required.json b/src/renderer/src/i18n/en-runtime-required.json index cfd3ac80e0f..b20c7037d55 100644 --- a/src/renderer/src/i18n/en-runtime-required.json +++ b/src/renderer/src/i18n/en-runtime-required.json @@ -2618,6 +2618,21 @@ } }, "hooks": { + "ipc": { + "events": { + "os": { + "markdown": { + "file": { + "open": { + "bridge": { + "1e9a1a63c4": "Failed to open the Markdown file." + } + } + } + } + } + } + }, "useComposerState": { "dropPartiallyAttached_one": "{{failureCount}} of {{count}} item could not be attached.", "dropPartiallyAttached_other": "{{failureCount}} of {{count}} items could not be attached." diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 0f06c44787f..12ab4fe22e3 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -1,4 +1,7 @@ { + "osDocumentOpen": { + "failed": "Failed to open the file." + }, "sidebar": { "revealFiltered": { "title": "Reveal hidden workspace?", diff --git a/src/renderer/src/lib/open-markdown-in-floating-workspace.test.ts b/src/renderer/src/lib/open-document-in-floating-workspace.test.ts similarity index 66% rename from src/renderer/src/lib/open-markdown-in-floating-workspace.test.ts rename to src/renderer/src/lib/open-document-in-floating-workspace.test.ts index 62fc278fc7a..b36ff34654c 100644 --- a/src/renderer/src/lib/open-markdown-in-floating-workspace.test.ts +++ b/src/renderer/src/lib/open-document-in-floating-workspace.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../shared/constants' import type { MarkdownDocument } from '../../../shared/filesystem-entry-types' import type { EditorFilesSlice } from '@/store/slices/editor/types/editor-files-slice' -import { openMarkdownDocumentInFloatingWorkspace } from './open-markdown-in-floating-workspace' +import { openDocumentInFloatingWorkspace } from './open-document-in-floating-workspace' function openFileMock(): ReturnType> { return vi.fn(() => 'file-1') @@ -18,11 +18,11 @@ function markdownDocument(overrides: Partial = {}): MarkdownDo } } -describe('openMarkdownDocumentInFloatingWorkspace', () => { +describe('openDocumentInFloatingWorkspace', () => { it('opens the document as a permanent floating-workspace edit tab', () => { const openFile = openFileMock() - const fileId = openMarkdownDocumentInFloatingWorkspace(openFile, markdownDocument()) + const fileId = openDocumentInFloatingWorkspace(openFile, markdownDocument()) expect(fileId).toBe('file-1') expect(openFile).toHaveBeenCalledTimes(1) @@ -44,7 +44,7 @@ describe('openMarkdownDocumentInFloatingWorkspace', () => { it('pins the open to this machine instead of the active runtime', () => { const openFile = openFileMock() - openMarkdownDocumentInFloatingWorkspace(openFile, markdownDocument()) + openDocumentInFloatingWorkspace(openFile, markdownDocument()) // Why: the caller already resolved an absolute local path, so a null runtime plus the // fallback suppression is what keeps the read off a remote SSH host the user is focused on. @@ -53,26 +53,31 @@ describe('openMarkdownDocumentInFloatingWorkspace', () => { expect(openFile.mock.calls[0][1]?.suppressActiveRuntimeFallback).toBe(true) }) - it('derives the language from the relative path', () => { - const openFile = openFileMock() + it.each(['mdx', 'csv', 'tsv'])( + 'derives the language for %s from the relative path', + (extension) => { + const openFile = openFileMock() - openMarkdownDocumentInFloatingWorkspace( - openFile, - markdownDocument({ - filePath: '/Users/me/notes/plan.mdx', - relativePath: 'plan.mdx', - basename: 'plan.mdx', - name: 'plan' - }) - ) + openDocumentInFloatingWorkspace( + openFile, + markdownDocument({ + filePath: `/Users/me/notes/plan.${extension}`, + relativePath: `plan.${extension}`, + basename: `plan.${extension}`, + name: 'plan' + }) + ) - expect(openFile.mock.calls[0][0].language).toBe('markdown') - }) + expect(openFile.mock.calls[0][0].language).toBe(extension === 'mdx' ? 'markdown' : extension) + expect(openFile.mock.calls[0][0].runtimeEnvironmentId).toBeNull() + expect(openFile.mock.calls[0][1]?.suppressActiveRuntimeFallback).toBe(true) + } + ) it('forwards a requested target group', () => { const openFile = openFileMock() - openMarkdownDocumentInFloatingWorkspace(openFile, markdownDocument(), { + openDocumentInFloatingWorkspace(openFile, markdownDocument(), { targetGroupId: 'group-2' }) diff --git a/src/renderer/src/lib/open-markdown-in-floating-workspace.ts b/src/renderer/src/lib/open-document-in-floating-workspace.ts similarity index 77% rename from src/renderer/src/lib/open-markdown-in-floating-workspace.ts rename to src/renderer/src/lib/open-document-in-floating-workspace.ts index 3b1bd4cd08b..2236dc81e3b 100644 --- a/src/renderer/src/lib/open-markdown-in-floating-workspace.ts +++ b/src/renderer/src/lib/open-document-in-floating-workspace.ts @@ -1,17 +1,17 @@ -import type { MarkdownDocument } from '../../../shared/filesystem-entry-types' +import type { FileDocument } from '../../../shared/filesystem-entry-types' import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../shared/constants' import type { EditorFilesSlice } from '@/store/slices/editor/types/editor-files-slice' import { detectLanguage } from './language-detect' /** - * Opens a markdown file that belongs to no workspace as a floating-workspace editor tab. + * Opens a local document that belongs to no workspace as a floating-workspace editor tab. * * Why local-only: every caller resolves an absolute path on this machine (a native picker or * the OS shell), so routing it through the active runtime would read it on the wrong host. */ -export function openMarkdownDocumentInFloatingWorkspace( +export function openDocumentInFloatingWorkspace( openFile: EditorFilesSlice['openFile'], - document: MarkdownDocument, + document: FileDocument, options: { targetGroupId?: string } = {} ): string { return openFile( diff --git a/src/shared/filesystem-entry-types.ts b/src/shared/filesystem-entry-types.ts index b81a68f4c2a..ec769bc8e31 100644 --- a/src/shared/filesystem-entry-types.ts +++ b/src/shared/filesystem-entry-types.ts @@ -7,13 +7,15 @@ export type DirEntry = { isSymlink: boolean } -export type MarkdownDocument = { +export type FileDocument = { filePath: string relativePath: string basename: string name: string } +export type MarkdownDocument = FileDocument + // ─── Filesystem watcher ───────────────────────────────────── export type FsChangeEvent = { kind: 'create' | 'update' | 'delete' | 'rename' | 'overflow' diff --git a/tests/e2e/helpers/orca-restart.ts b/tests/e2e/helpers/orca-restart.ts index 8be4adb79f8..3972936e42d 100644 --- a/tests/e2e/helpers/orca-restart.ts +++ b/tests/e2e/helpers/orca-restart.ts @@ -37,6 +37,7 @@ type LaunchedOrca = { } type LaunchOptions = { + extraArgs?: string[] /** * Called for each chunk the relaunched main process writes to stderr. The * listener is attached before `firstWindow()` resolves so main-process @@ -177,7 +178,7 @@ export function createRestartSession( const launch = async (options?: LaunchOptions): Promise => { runtimeWsPort ??= await reserveRestartRuntimeWsPort() const app = await electron.launch({ - args: getOrcaElectronLaunchArgs(mainPath, headful), + args: [...getOrcaElectronLaunchArgs(mainPath, headful), ...(options?.extraArgs ?? [])], env: { ...homeIsolation.env, ...options?.extraEnv, diff --git a/tests/e2e/os-opened-documents.spec.ts b/tests/e2e/os-opened-documents.spec.ts new file mode 100644 index 00000000000..cd25dd593e4 --- /dev/null +++ b/tests/e2e/os-opened-documents.spec.ts @@ -0,0 +1,110 @@ +import { mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs' +import os from 'node:os' +import path from 'node:path' +import { pathToFileURL } from 'node:url' +import { test, expect } from './helpers/orca-app' +import { createRestartSession } from './helpers/orca-restart' + +for (const extension of ['md', 'csv', 'tsv'] as const) { + test(`opens OS-requested ${extension} documents on cold and warm launches without duplicate tabs`, async ({ + seedTestRepo + }, testInfo) => { + void seedTestRepo + const root = realpathSync(mkdtempSync(path.join(os.tmpdir(), 'orca-os-documents-'))) + const delimiter = extension === 'tsv' ? '\t' : ',' + const coldPath = path.join(root, `cold.${extension}`) + const warmPath = path.join(root, `warm.${extension}`) + writeFileSync( + coldPath, + extension === 'md' + ? '# Cold result\n\n42\n' + : `name${delimiter}amount\nCold result${delimiter}42\n` + ) + writeFileSync( + warmPath, + extension === 'md' + ? '# Warm result\n\n84\n' + : `name${delimiter}amount\nWarm result${delimiter}84\n` + ) + const unsupportedPath = path.join(root, 'unsupported.txt') + writeFileSync(unsupportedPath, 'private content') + const session = createRestartSession(testInfo, { + ORCA_BACKGROUND_LAUNCH: '1', + ORCA_E2E_ENFORCE_SINGLE_INSTANCE_LOCK: '1' + }) + let launched: Awaited> | undefined + try { + launched = await session.launch({ extraArgs: [coldPath, pathToFileURL(coldPath).href] }) + const { app, page } = launched + const table = page.getByRole('table') + const coldResult = + extension === 'md' + ? page.getByRole('heading', { name: 'Cold result' }) + : table.getByRole('cell', { name: 'Cold result', exact: true }) + const warmResult = + extension === 'md' + ? page.getByRole('heading', { name: 'Warm result' }) + : table.getByRole('cell', { name: 'Warm result', exact: true }) + await expect(coldResult).toBeVisible() + await expect(page.getByText('42', { exact: true })).toBeVisible() + await expect( + page.locator('[data-tab-id]').filter({ hasText: `cold.${extension}` }) + ).toHaveCount(1) + const coldProof = testInfo.outputPath('cold-open.png') + await page.screenshot({ path: coldProof }) + await testInfo.attach('cold-open', { path: coldProof, contentType: 'image/png' }) + + await app.evaluate(({ app }, fileUrl) => { + app.emit('second-instance', {}, ['orca', fileUrl], process.cwd()) + }, pathToFileURL(warmPath).href) + await expect(warmResult).toBeVisible() + await expect(page.getByText('84', { exact: true })).toBeVisible() + + await page + .locator('[data-tab-id]') + .filter({ hasText: `cold.${extension}` }) + .click() + await expect(coldResult).toBeVisible() + + await app.evaluate(({ app }, filePath) => { + app.emit('open-file', { preventDefault() {} }, filePath) + }, warmPath) + await expect(warmResult).toBeVisible() + await expect( + page.locator('[data-tab-id]').filter({ hasText: `warm.${extension}` }) + ).toHaveCount(1) + const warmProof = testInfo.outputPath('warm-open.png') + await page.screenshot({ path: warmProof }) + await testInfo.attach('warm-open', { path: warmProof, contentType: 'image/png' }) + + await app.evaluate(({ app }, filePath) => { + app.emit('open-file', { preventDefault() {} }, filePath) + }, unsupportedPath) + await expect( + page.locator('[data-tab-id]').filter({ hasText: 'unsupported.txt' }) + ).toHaveCount(0) + const unauthorizedRead = await page.evaluate(async (filePath) => { + try { + await window.api.fs.readFile({ filePath }) + return 'allowed' + } catch (error) { + return error instanceof Error ? error.message : String(error) + } + }, unsupportedPath) + expect(unauthorizedRead).toContain('Access denied') + expect( + await app.evaluate(({ BrowserWindow }) => + BrowserWindow.getAllWindows().every( + (window) => !window.isVisible() && !window.isFocused() + ) + ) + ).toBe(true) + } finally { + if (launched) { + await session.close(launched.app) + } + await session.dispose() + rmSync(root, { recursive: true, force: true }) + } + }) +}