From 7c9059c4d4565a70b160cdef821ec79eb7e2de25 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 3 Jul 2026 01:32:22 -0700 Subject: [PATCH] Restore desktop terminal URL modifier clicks Restore desktop terminal URL and OSC link activation to require Cmd/Ctrl while preserving mobile tap routing. --- .../terminal-pane/terminal-link-activation.ts | 6 ++ .../terminal-link-handlers.test.ts | 80 +++++++++++++++++-- .../terminal-pane/terminal-link-handlers.ts | 12 +-- .../terminal-pane/terminal-link-open-hints.ts | 4 +- .../terminal-osc-link-routing.ts | 26 +++--- .../terminal-url-link-hit-testing.ts | 16 ++-- .../use-terminal-pane-lifecycle.ts | 12 ++- .../pane-manager/pane-dom-creation.test.ts | 44 +++++++++- .../src/lib/pane-manager/pane-dom-creation.ts | 4 +- 9 files changed, 162 insertions(+), 42 deletions(-) create mode 100644 src/renderer/src/components/terminal-pane/terminal-link-activation.ts diff --git a/src/renderer/src/components/terminal-pane/terminal-link-activation.ts b/src/renderer/src/components/terminal-pane/terminal-link-activation.ts new file mode 100644 index 00000000000..e85da903224 --- /dev/null +++ b/src/renderer/src/components/terminal-pane/terminal-link-activation.ts @@ -0,0 +1,6 @@ +export function isTerminalLinkActivation( + event: Pick | undefined +): boolean { + const isMac = navigator.userAgent.includes('Mac') + return isMac ? Boolean(event?.metaKey) : Boolean(event?.ctrlKey) +} diff --git a/src/renderer/src/components/terminal-pane/terminal-link-handlers.test.ts b/src/renderer/src/components/terminal-pane/terminal-link-handlers.test.ts index 5cbc01a0903..c75dd3943e7 100644 --- a/src/renderer/src/components/terminal-pane/terminal-link-handlers.test.ts +++ b/src/renderer/src/components/terminal-pane/terminal-link-handlers.test.ts @@ -7,6 +7,7 @@ import { createFilePathLinkProvider, getTerminalFileOpenHint, getTerminalHtmlFileOpenHint, + getTerminalUrlOpenHint, installFilePathLinkClickFallback, isTerminalLinkActivation, openFilePathLinkAtBufferPosition, @@ -156,12 +157,28 @@ describe('isTerminalLinkActivation', () => { }) describe('handleOscLink', () => { - it('routes http links on ordinary click', () => { + it('ignores http links without the platform modifier on desktop', () => { setPlatform('Macintosh') storeState.settings = { openLinksInApp: true } const preventDefault = vi.fn() - handleOscLink('https://example.com', { metaKey: false, ctrlKey: false, preventDefault }, deps) + expect( + handleOscLink('https://example.com', { metaKey: false, ctrlKey: false, preventDefault }, deps) + ).toBe(false) + + expect(openUrlMock).not.toHaveBeenCalled() + expect(createBrowserTabMock).not.toHaveBeenCalled() + expect(preventDefault).not.toHaveBeenCalled() + }) + + it('routes http links with the platform modifier on desktop', () => { + setPlatform('Macintosh') + storeState.settings = { openLinksInApp: true } + const preventDefault = vi.fn() + + expect( + handleOscLink('https://example.com', { metaKey: true, ctrlKey: false, preventDefault }, deps) + ).toBe(true) expect(openUrlMock).not.toHaveBeenCalled() expect(createBrowserTabMock).toHaveBeenCalledWith('wt-1', 'https://example.com/', { @@ -438,18 +455,38 @@ describe('handleOscLink', () => { setPlatform('Macintosh') expect(getTerminalFileOpenHint()).toBe('⌘+click to open or ⇧⌘+click for default app') expect(getTerminalHtmlFileOpenHint()).toBe('⌘+click to open or ⇧⌘+click for default browser') + expect(getTerminalUrlOpenHint()).toBe('⌘+click to open or ⇧⌘+click for system browser') setPlatform('Windows') expect(getTerminalFileOpenHint()).toBe('Ctrl+click to open or Shift+Ctrl+click for default app') expect(getTerminalHtmlFileOpenHint()).toBe( 'Ctrl+click to open or Shift+Ctrl+click for default browser' ) + expect(getTerminalUrlOpenHint()).toBe( + 'Ctrl+click to open or Shift+Ctrl+click for system browser' + ) }) - it('opens local file URL links in Orca on ordinary click', async () => { + it('ignores local file URL links without the platform modifier on desktop', async () => { setPlatform('Windows') - handleOscLink('file:///tmp/test.txt', { metaKey: false, ctrlKey: false }, deps) + expect(handleOscLink('file:///tmp/test.txt', { metaKey: false, ctrlKey: false }, deps)).toBe( + false + ) + + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(authorizeExternalPathMock).not.toHaveBeenCalled() + expect(openFileMock).not.toHaveBeenCalled() + expect(openFilePathMock).not.toHaveBeenCalled() + }) + + it('opens local file URL links in Orca with the platform modifier on desktop', async () => { + setPlatform('Windows') + + expect(handleOscLink('file:///tmp/test.txt', { metaKey: false, ctrlKey: true }, deps)).toBe( + true + ) // openDetectedFilePath is async (fire-and-forget), so flush the microtask queue // before asserting on positive behavior. @@ -1708,7 +1745,7 @@ describe('createFilePathLinkProvider range bounds', () => { disposable.dispose() }) - it('opens regular URLs from a direct ordinary-click fallback when xterm did not handle them', async () => { + it('ignores regular URLs from a direct ordinary-click fallback on desktop', async () => { setPlatform('Macintosh') storeState.settings = { openLinksInApp: false } const rows = [ @@ -1732,6 +1769,39 @@ describe('createFilePathLinkProvider range bounds', () => { stopPropagation } as unknown as MouseEvent) + expect(openUrlMock).not.toHaveBeenCalled() + expect(preventDefault).not.toHaveBeenCalled() + expect(stopPropagation).not.toHaveBeenCalled() + expect(terminal.clearSelection).not.toHaveBeenCalled() + + disposable.dispose() + expect(element.removeEventListener).toHaveBeenCalledWith('mouseup', mouseUp) + }) + + it('opens regular URLs from a direct modifier-click fallback when xterm did not handle them', async () => { + setPlatform('Macintosh') + storeState.settings = { openLinksInApp: false } + const rows = [ + makeBufferLine('PR opened: https://github.com/stablyai/orca-marketing-website/pull/82') + ] + const { terminal, element } = makeFallbackTerminal(rows) + const disposable = installHttpLinkClickFallback(terminal, { worktreeId: 'wt-1' }) + const mouseUp = getRegisteredBubbleMouseUpHandler(element) + const preventDefault = vi.fn() + const stopPropagation = vi.fn() + + mouseUp({ + button: 0, + metaKey: true, + ctrlKey: false, + shiftKey: false, + defaultPrevented: false, + clientX: 230, + clientY: 25, + preventDefault, + stopPropagation + } as unknown as MouseEvent) + expect(openUrlMock).toHaveBeenCalledWith( 'https://github.com/stablyai/orca-marketing-website/pull/82' ) diff --git a/src/renderer/src/components/terminal-pane/terminal-link-handlers.ts b/src/renderer/src/components/terminal-pane/terminal-link-handlers.ts index 0e111027e37..462dbdeeedc 100644 --- a/src/renderer/src/components/terminal-pane/terminal-link-handlers.ts +++ b/src/renderer/src/components/terminal-pane/terminal-link-handlers.ts @@ -33,14 +33,15 @@ import { getTerminalOrcaFileOpenHint, getTerminalWorktreePathOpenHint, getTerminalFileOpenHint, - getTerminalUrlOpenHint, - isMacPlatform + getTerminalUrlOpenHint } from './terminal-link-open-hints' import { resolveKnownWorktreeRootPathLink } from './terminal-worktree-path-link' +import { isTerminalLinkActivation } from './terminal-link-activation' export { openDetectedFilePath } from './terminal-file-open-routing' export { openFilePathLinkAtBufferPosition } from './terminal-file-link-hit-testing' export { getTerminalFileOpenHint, getTerminalHtmlFileOpenHint, getTerminalUrlOpenHint } +export { isTerminalLinkActivation } from './terminal-link-activation' export type LinkHandlerDeps = { worktreeId: string @@ -315,10 +316,3 @@ export function installFilePathLinkClickFallback( } } } - -export function isTerminalLinkActivation( - event: Pick | undefined -): boolean { - const isMac = isMacPlatform() - return isMac ? Boolean(event?.metaKey) : Boolean(event?.ctrlKey) -} diff --git a/src/renderer/src/components/terminal-pane/terminal-link-open-hints.ts b/src/renderer/src/components/terminal-pane/terminal-link-open-hints.ts index 246333c7cbd..0d51669a6c6 100644 --- a/src/renderer/src/components/terminal-pane/terminal-link-open-hints.ts +++ b/src/renderer/src/components/terminal-pane/terminal-link-open-hints.ts @@ -22,8 +22,8 @@ export function getTerminalHtmlFileOpenHint(): string { export function getTerminalUrlOpenHint(): string { return isMacPlatform() - ? 'click to open or ⇧+click for system browser' - : 'click to open or Shift+click for system browser' + ? '⌘+click to open or ⇧⌘+click for system browser' + : 'Ctrl+click to open or Shift+Ctrl+click for system browser' } export function getTerminalUrlSystemBrowserHint(): string { diff --git a/src/renderer/src/components/terminal-pane/terminal-osc-link-routing.ts b/src/renderer/src/components/terminal-pane/terminal-osc-link-routing.ts index 9d588b7857a..2b043d648cf 100644 --- a/src/renderer/src/components/terminal-pane/terminal-osc-link-routing.ts +++ b/src/renderer/src/components/terminal-pane/terminal-osc-link-routing.ts @@ -3,6 +3,7 @@ import { isWindowsAbsolutePathLike } from '../../../../shared/cross-platform-pat import type { LinkHandlerDeps } from './terminal-link-handlers' import { resolveTerminalFileUrlTarget } from './terminal-file-url-target' import { openDetectedFilePath } from './terminal-file-open-routing' +import { isTerminalLinkActivation } from './terminal-link-activation' import { openTerminalHttpLink, type TerminalLinkRoutingPreferenceRequester @@ -11,16 +12,16 @@ import { type TerminalLinkEvent = Pick & Partial> -function isPrimaryOscLinkActivation(event: TerminalLinkEvent | undefined): boolean { +function isDesktopOscLinkActivation(event: TerminalLinkEvent | undefined): boolean { if (!event) { return false } if ('button' in event && event.button !== undefined && event.button !== 0) { return false } - // Why: macOS Ctrl-click is a context-menu gesture even when Chromium reports - // it as button 0; ordinary OSC links should not steal that secondary action. - return !(navigator.userAgent.includes('Mac') && event.ctrlKey && !event.metaKey) + // Why: desktop xterm links must not open while the user is just placing the + // cursor or selecting text. Mobile URL taps use a separate WebView path. + return isTerminalLinkActivation(event) } export function handleOscLink( @@ -30,9 +31,9 @@ export function handleOscLink( Partial> & { requestOpenLinksInAppPreference?: TerminalLinkRoutingPreferenceRequester } -): void { - if (!isPrimaryOscLinkActivation(event)) { - return +): boolean { + if (!isDesktopOscLinkActivation(event)) { + return false } // Why: xterm renders OSC 8 links as clickable anchors. Orca must suppress // default anchor navigation so link-routing settings can choose the target. @@ -67,15 +68,14 @@ export function handleOscLink( ) { // Why: `new URL("C:\\path\\file.ts")` succeeds with protocol `c:`; // Windows OSC links need file-path routing before generic URL parsing. - return + return true } let parsed: URL try { parsed = new URL(rawText) } catch { - openDetectedPathLink() - return + return openDetectedPathLink() } if (parsed.protocol === 'http:' || parsed.protocol === 'https:') { @@ -84,7 +84,7 @@ export function handleOscLink( forceSystemBrowser: Boolean(event?.shiftKey), requestOpenLinksInAppPreference: deps.requestOpenLinksInAppPreference }) - return + return true } if (parsed.protocol === 'file:') { @@ -99,11 +99,13 @@ export function handleOscLink( !deps.runtimeEnvironmentId const resolved = resolveTerminalFileUrlTarget(parsed, { allowUncHost }) if (!resolved) { - return + return false } openDetectedFilePath(resolved.filePath, resolved.line, resolved.column, { ...deps, openWithSystemDefault: Boolean(event?.shiftKey) }) + return true } + return false } diff --git a/src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.ts b/src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.ts index 96ceafcce38..67505d5f622 100644 --- a/src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.ts +++ b/src/renderer/src/components/terminal-pane/terminal-url-link-hit-testing.ts @@ -1,6 +1,7 @@ import type { IBufferLine, IBufferRange, IDisposable, Terminal } from '@xterm/xterm' import { openHttpLink } from '@/lib/http-link-routing' import { buildCandidateLogicalLinesForBufferPosition } from './terminal-file-link-hit-testing' +import { isTerminalLinkActivation } from './terminal-link-activation' import { rangeForParsedFileLink } from './wrapped-terminal-link-ranges' type UrlLinkHitTestDeps = { @@ -48,13 +49,14 @@ export function extractTerminalHttpLinks(lineText: string): ParsedTerminalHttpLi return links } -function isPrimaryHttpLinkFallbackActivation(event: MouseEvent): boolean { +function isDesktopHttpLinkFallbackActivation(event: MouseEvent): boolean { if (event.defaultPrevented || event.button !== 0) { return false } - // Why: URL links now open on ordinary clicks, but macOS Ctrl-click must stay - // available for context menus even when Chromium reports it as button 0. - return !(navigator.userAgent.includes('Mac') && event.ctrlKey && !event.metaKey) + // Why: desktop terminal links require an intentional Cmd/Ctrl gesture so + // plain clicks remain available for cursor placement and selection. Mobile + // tap routing is handled separately under mobile/src/terminal. + return isTerminalLinkActivation(event) } function* iterateTerminalHttpUrlCandidates( @@ -220,7 +222,7 @@ export function installHttpLinkClickFallback( deps: UrlLinkClickFallbackDeps ): IDisposable { const handleMouseUp = (event: MouseEvent): void => { - if (!isPrimaryHttpLinkFallbackActivation(event)) { + if (!isDesktopHttpLinkFallbackActivation(event)) { return } @@ -230,8 +232,8 @@ export function installHttpLinkClickFallback( } // Why: xterm's WebLinksAddon only activates after hover state exists. This - // direct mouseup fallback preserves ordinary link clicks when the hover link - // was never established, while defaultPrevented avoids duplicate opens. + // direct mouseup fallback preserves modifier-clicks when the hover link was + // never established, while defaultPrevented avoids duplicate opens. const opened = openHttpLinkAtBufferPosition(terminal.buffer.active, position, terminal.cols, { worktreeId: deps.worktreeId, forceSystemBrowser: event.shiftKey, diff --git a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts index a5d9172855b..1674e150fcd 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-pane-lifecycle.ts @@ -1017,7 +1017,7 @@ export function useTerminalPaneLifecycle({ pane.terminal.options.linkHandler = { allowNonHttpProtocols: true, activate: (event, text) => { - handleOscLink(text, event as MouseEvent | undefined, { + const handled = handleOscLink(text, event as MouseEvent | undefined, { ...linkDeps, startupCwd: getPaneLinkCwd(pane.id), runtimeEnvironmentId: linkDeps.getRuntimeEnvironmentIdForPane?.(pane.id) ?? null, @@ -1031,7 +1031,9 @@ export function useTerminalPaneLifecycle({ // moving the mouse extends a selection until the next click/Esc. // clearSelection() explicitly detaches those listeners (see // SelectionService._removeMouseDownListeners). - pane.terminal.clearSelection() + if (handled) { + pane.terminal.clearSelection() + } }, // Show bottom-left tooltip on hover for OSC 8 hyperlinks (e.g. // GitHub owner/repo#issue references emitted by CLI tools) — same @@ -1344,7 +1346,7 @@ export function useTerminalPaneLifecycle({ return } const activePane = managerRef.current?.getActivePane() - void handleOscLink(url, event, { + const handled = handleOscLink(url, event, { ...linkDeps, startupCwd: activePane ? getPaneLinkCwd(activePane.id) : startupCwd, runtimeEnvironmentId: activePane @@ -1359,7 +1361,9 @@ export function useTerminalPaneLifecycle({ // phantom selection until the next click/Esc. Explicitly clearing the // selection also detaches those listeners (see // SelectionService._removeMouseDownListeners). - managerRef.current?.getActivePane()?.terminal.clearSelection() + if (handled) { + managerRef.current?.getActivePane()?.terminal.clearSelection() + } }, formatLinkTooltip: (url, openLinkHint) => formatTerminalUrlTooltip(url, openLinkHint), // Why: TerminalPane instances stay mounted for hidden visited worktrees diff --git a/src/renderer/src/lib/pane-manager/pane-dom-creation.test.ts b/src/renderer/src/lib/pane-manager/pane-dom-creation.test.ts index ec1783654d5..7f256d4304a 100644 --- a/src/renderer/src/lib/pane-manager/pane-dom-creation.test.ts +++ b/src/renderer/src/lib/pane-manager/pane-dom-creation.test.ts @@ -1,5 +1,5 @@ // @vitest-environment happy-dom -import { describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import type { TerminalLeafId } from '../../../../shared/stable-pane-id' import { createPaneDOM } from './pane-dom-creation' @@ -48,7 +48,49 @@ vi.mock('@xterm/xterm', () => ({ }) })) +function setPlatform(userAgent: string): void { + vi.stubGlobal('navigator', { userAgent }) +} + +afterEach(() => { + vi.unstubAllGlobals() +}) + describe('createPaneDOM link tooltips', () => { + it('uses desktop modifier-click text for WebLinks hover hints', () => { + const leafId = '11111111-1111-4111-8111-111111111111' as TerminalLeafId + + setPlatform('Macintosh') + const macPane = createPaneDOM( + 1, + leafId, + {}, + { active: null } as never, + {} as never, + vi.fn(), + vi.fn() + ) + webLinksAddonMock.options?.hover?.({} as MouseEvent, 'http://localhost:5180/') + expect(macPane.linkTooltip.textContent).toBe( + 'http://localhost:5180/ (⌘+click to open or ⇧⌘+click for system browser)' + ) + + setPlatform('Windows') + const windowsPane = createPaneDOM( + 2, + leafId, + {}, + { active: null } as never, + {} as never, + vi.fn(), + vi.fn() + ) + webLinksAddonMock.options?.hover?.({} as MouseEvent, 'http://localhost:5180/') + expect(windowsPane.linkTooltip.textContent).toBe( + 'http://localhost:5180/ (Ctrl+click to open or Shift+Ctrl+click for system browser)' + ) + }) + it('lets callers replace WebLinks hover text for display-only labels', async () => { const labeledText = 'http://main.orca.localhost:60016/ (localhost:5180; click to open)' const leafId = '11111111-1111-4111-8111-111111111111' as TerminalLeafId diff --git a/src/renderer/src/lib/pane-manager/pane-dom-creation.ts b/src/renderer/src/lib/pane-manager/pane-dom-creation.ts index 8c1c1e289ff..2e556f44400 100644 --- a/src/renderer/src/lib/pane-manager/pane-dom-creation.ts +++ b/src/renderer/src/lib/pane-manager/pane-dom-creation.ts @@ -16,8 +16,8 @@ import { installGuardedLinkProviderRegistration } from './terminal-link-provider function getTerminalUrlOpenHint(): string { return navigator.userAgent.includes('Mac') - ? 'click to open or ⇧+click for system browser' - : 'click to open or Shift+click for system browser' + ? '⌘+click to open or ⇧⌘+click for system browser' + : 'Ctrl+click to open or Shift+Ctrl+click for system browser' } function defaultLinkTooltipText(uri: string, openLinkHint: string): string {