From f82d5c3e6cf61c352ae7ed0cef63b9614524f6fe Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 5 Aug 2026 23:48:44 -0700 Subject: [PATCH] fix(terminal): clear stranded link hover tooltip (#12786) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(terminal): clear stranded link hover tooltip * fix(terminal): declare the tooltip reserve var where it resolves --orca-terminal-link-tooltip-height was declared on .pane-manager-root, a class no live element carries, so both .xterm-container height calc()s were invalid at computed-value time and collapsed to height:auto — the element FitAddon measures, making rows a fixed point. Also isolate _clearCurrentLink() so a throwing provider leave() cannot skip the cache invalidation, and bound the e2e gap assertion on both sides. Co-authored-by: Orca --------- Co-authored-by: Orca --- src/renderer/src/assets/terminal.css | 16 +- .../src/lib/pane-manager/pane-lifecycle.ts | 16 +- .../lib/pane-manager/pane-manager-types.ts | 3 + ...inkifier-hover-reset-on-mouseleave.test.ts | 77 +++++++- ...nal-linkifier-hover-reset-on-mouseleave.ts | 31 ++- .../terminal-linkifier-hover-reset.ts | 25 ++- .../issue-12656-terminal-link-tooltip.spec.ts | 185 ++++++++++++++++++ 7 files changed, 334 insertions(+), 19 deletions(-) create mode 100644 tests/e2e/issue-12656-terminal-link-tooltip.spec.ts diff --git a/src/renderer/src/assets/terminal.css b/src/renderer/src/assets/terminal.css index ab45df2cc..3d9a5df21 100644 --- a/src/renderer/src/assets/terminal.css +++ b/src/renderer/src/assets/terminal.css @@ -6,6 +6,12 @@ position: relative; } +/* Why: .pane-manager-root matches no live element, so a var declared there + never resolves and every calc() using it is dropped. Keep it on :root. */ +:root { + --orca-terminal-link-tooltip-height: 25px; +} + /* Ensure pane manager root fills its absolutely-positioned container */ .pane-manager-root { width: 100% !important; @@ -497,9 +503,10 @@ so Ghostty-imported window-padding-x/y can be applied without inline styles. The fallback 4px preserves the legacy default. */ .xterm-container { + box-sizing: border-box; position: relative; width: calc(100% - var(--pane-padding-x, 4px)); - height: calc(100% - var(--pane-padding-y, 4px)); + height: calc(100% - var(--pane-padding-y, 4px) - var(--orca-terminal-link-tooltip-height, 25px)); margin-top: var(--pane-padding-y, 4px); margin-left: var(--pane-padding-x, 4px); } @@ -510,12 +517,15 @@ same amount to prevent overflow/clipping. */ .pane[data-has-title] .xterm-container { margin-top: var(--orca-pane-title-height); - height: calc(100% - var(--orca-pane-title-height)); /* match margin-top */ + height: calc( + 100% - var(--orca-pane-title-height) - var(--orca-terminal-link-tooltip-height, 25px) + ); /* match margin-top and the tooltip reserve */ } /* Ghostty-style URL hover: glued to the pane's true bottom-left corner. */ .pane-link-tooltip { position: absolute; + box-sizing: border-box; bottom: 0; left: 0; z-index: 40; @@ -526,6 +536,8 @@ border-bottom: none; border-left: none; padding: 4px 8px; + height: var(--orca-terminal-link-tooltip-height); + line-height: 15px; max-width: 80%; overflow: hidden; text-overflow: ellipsis; diff --git a/src/renderer/src/lib/pane-manager/pane-lifecycle.ts b/src/renderer/src/lib/pane-manager/pane-lifecycle.ts index dd6f41e43..e5695207a 100644 --- a/src/renderer/src/lib/pane-manager/pane-lifecycle.ts +++ b/src/renderer/src/lib/pane-manager/pane-lifecycle.ts @@ -9,7 +9,10 @@ import { cancelDeferredScrollRestore } from './pane-scroll' import { activateOrcaTerminalUnicodeProvider } from '../../../../shared/terminal-unicode-provider' import { attachTerminalMouseWheelMultiplier } from './pane-terminal-mouse-wheel' import { attachTerminalScrollIntentTracking } from './terminal-scroll-intent-dom-tracking' -import { installTerminalLinkifierHoverResetOnMouseLeave } from './terminal-linkifier-hover-reset-on-mouseleave' +import { + installTerminalLinkifierHoverResetOnMouseLeave, + installTerminalLinkifierHoverResetOnWindowBlur +} from './terminal-linkifier-hover-reset-on-mouseleave' import { installTerminalLinkifierHoverResetOnWrite } from './terminal-linkifier-hover-reset-on-write' import { attachDomRendererFocusClassSync } from './pane-dom-focus-class-sync' import { attachWebgl, cancelPendingWebglRefresh, disposeWebgl } from './pane-webgl-renderer' @@ -63,7 +66,14 @@ export function openTerminal(pane: ManagedPaneInternal): void { // line; invalidate the linkifier hover cache when output lands so the next // pointer move re-linkifies it. pane.linkifierHoverResetDisposable = installTerminalLinkifierHoverResetOnWrite(terminal) - pane.linkifierMouseLeaveResetDisposable = installTerminalLinkifierHoverResetOnMouseLeave(terminal) + pane.linkifierMouseLeaveResetDisposable = installTerminalLinkifierHoverResetOnMouseLeave( + terminal, + linkTooltip + ) + pane.linkifierWindowBlurResetDisposable = installTerminalLinkifierHoverResetOnWindowBlur( + terminal, + linkTooltip + ) // Activate Orca's Unicode 11 width shim *before* any caller-driven write. CJK / emoji / // ZWJ codepoints get baked into the buffer at the active unicode version on @@ -187,6 +197,8 @@ export function disposePane( pane.linkifierHoverResetDisposable = null pane.linkifierMouseLeaveResetDisposable?.dispose() pane.linkifierMouseLeaveResetDisposable = null + pane.linkifierWindowBlurResetDisposable?.dispose() + pane.linkifierWindowBlurResetDisposable = null // Deregister the RTL shaping joiner: terminal.dispose() below does not. try { pane.arabicShapingJoinerCleanup?.() diff --git a/src/renderer/src/lib/pane-manager/pane-manager-types.ts b/src/renderer/src/lib/pane-manager/pane-manager-types.ts index 3b82e0a92..7f285d177 100644 --- a/src/renderer/src/lib/pane-manager/pane-manager-types.ts +++ b/src/renderer/src/lib/pane-manager/pane-manager-types.ts @@ -176,6 +176,9 @@ export type ManagedPaneInternal = { linkifierHoverResetDisposable?: IDisposable | null // Stored because mouseleave does not bubble from xterm's screen. linkifierMouseLeaveResetDisposable?: IDisposable | null + // Stored because a window blur may strand xterm's active link without a + // follow-up mouse event. + linkifierWindowBlurResetDisposable?: IDisposable | null // Stored so disposePane() can deregister the joiner; terminal.dispose() // does not remove registered character joiners. arabicShapingJoinerCleanup?: (() => void) | null diff --git a/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset-on-mouseleave.test.ts b/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset-on-mouseleave.test.ts index ab4b9fcf8..d1748395d 100644 --- a/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset-on-mouseleave.test.ts +++ b/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset-on-mouseleave.test.ts @@ -1,43 +1,73 @@ import type { Terminal } from '@xterm/xterm' import { describe, expect, it, vi } from 'vitest' -import { installTerminalLinkifierHoverResetOnMouseLeave } from './terminal-linkifier-hover-reset-on-mouseleave' +import { + installTerminalLinkifierHoverResetOnMouseLeave, + installTerminalLinkifierHoverResetOnWindowBlur +} from './terminal-linkifier-hover-reset-on-mouseleave' -type FakeLinkifier = { _lastBufferCell?: unknown; _activeLine?: number } +type FakeLinkifier = { + _lastBufferCell?: unknown + _activeLine?: number + _clearCurrentLink?: () => void + _currentLink?: unknown +} function createHarness(hasScreen = true) { let mouseLeaveHandler: (() => void) | null = null + let blurHandler: (() => void) | null = null const addEventListener = vi.fn((_event: string, handler: () => void) => { - mouseLeaveHandler = handler + if (_event === 'mouseleave') { + mouseLeaveHandler = handler + } }) const removeEventListener = vi.fn((_event: string, handler: () => void) => { - if (mouseLeaveHandler === handler) { + if (_event === 'mouseleave' && mouseLeaveHandler === handler) { mouseLeaveHandler = null } }) - const screen = { addEventListener, removeEventListener } + const screen = { addEventListener, removeEventListener, classList: { remove: vi.fn() } } const querySelector = vi.fn(() => (hasScreen ? screen : null)) + const rendererWindow = { + addEventListener: vi.fn((_event: string, handler: () => void) => { + blurHandler = handler + }), + removeEventListener: vi.fn((_event: string, handler: () => void) => { + if (blurHandler === handler) { + blurHandler = null + } + }) + } const linkifier: FakeLinkifier = { _lastBufferCell: { x: 2, y: 3 }, - _activeLine: 3 + _activeLine: 3, + _clearCurrentLink: vi.fn(), + _currentLink: { link: 'https://example.com' } } const terminal = { - element: { querySelector }, + element: { querySelector, ownerDocument: { defaultView: rendererWindow } }, _core: { linkifier } } as unknown as Terminal + const linkTooltip = { + ownerDocument: { defaultView: rendererWindow }, + style: { display: '' } + } as unknown as HTMLElement return { terminal, linkifier, + linkTooltip, + rendererWindow, querySelector, addEventListener, removeEventListener, - dispatchMouseLeave: () => mouseLeaveHandler?.() + dispatchMouseLeave: () => mouseLeaveHandler?.(), + dispatchBlur: () => blurHandler?.() } } describe('installTerminalLinkifierHoverResetOnMouseLeave', () => { it('resets the hover cache when the terminal surface loses the pointer', () => { const harness = createHarness() - installTerminalLinkifierHoverResetOnMouseLeave(harness.terminal) + installTerminalLinkifierHoverResetOnMouseLeave(harness.terminal, harness.linkTooltip) expect(harness.querySelector).toHaveBeenCalledWith('.xterm-screen') expect(harness.addEventListener).toHaveBeenCalledWith('mouseleave', expect.any(Function)) @@ -45,6 +75,9 @@ describe('installTerminalLinkifierHoverResetOnMouseLeave', () => { expect(harness.linkifier._lastBufferCell).toBeUndefined() expect(harness.linkifier._activeLine).toBe(-1) + expect(harness.linkifier._clearCurrentLink).toHaveBeenCalledTimes(1) + expect(harness.linkifier._currentLink).toBeUndefined() + expect(harness.linkTooltip.style.display).toBe('none') }) it('removes the listener on dispose', () => { @@ -79,4 +112,30 @@ describe('installTerminalLinkifierHoverResetOnMouseLeave', () => { expect(harness.dispatchMouseLeave).not.toThrow() }) + + it('clears the active link and tooltip when the renderer window blurs', () => { + const harness = createHarness() + const disposable = installTerminalLinkifierHoverResetOnWindowBlur( + harness.terminal, + harness.linkTooltip + ) + + harness.dispatchBlur() + + expect(harness.rendererWindow.addEventListener).toHaveBeenCalledWith( + 'blur', + expect.any(Function) + ) + expect(harness.linkifier._clearCurrentLink).toHaveBeenCalledTimes(1) + expect(harness.linkifier._currentLink).toBeUndefined() + expect(harness.linkifier._lastBufferCell).toBeUndefined() + expect(harness.linkifier._activeLine).toBe(-1) + expect(harness.linkTooltip.style.display).toBe('none') + + disposable.dispose() + expect(harness.rendererWindow.removeEventListener).toHaveBeenCalledWith( + 'blur', + expect.any(Function) + ) + }) }) diff --git a/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset-on-mouseleave.ts b/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset-on-mouseleave.ts index 0a88c74cf..dd71b3568 100644 --- a/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset-on-mouseleave.ts +++ b/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset-on-mouseleave.ts @@ -1,16 +1,43 @@ import type { IDisposable, Terminal } from '@xterm/xterm' import { resetTerminalLinkifierHoverState } from './terminal-linkifier-hover-reset' -export function installTerminalLinkifierHoverResetOnMouseLeave(terminal: Terminal): IDisposable { +export function installTerminalLinkifierHoverResetOnMouseLeave( + terminal: Terminal, + linkTooltip?: HTMLElement +): IDisposable { const screen = terminal.element?.querySelector('.xterm-screen') if (!screen) { return { dispose: () => undefined } } - const resetHover = (): void => resetTerminalLinkifierHoverState(terminal) + const resetHover = (): void => { + if (linkTooltip) { + linkTooltip.style.display = 'none' + } + resetTerminalLinkifierHoverState(terminal) + } // Why: xterm clears its active link but keeps the cell cache on mouseleave. screen.addEventListener('mouseleave', resetHover) return { dispose: () => screen.removeEventListener('mouseleave', resetHover) } } + +export function installTerminalLinkifierHoverResetOnWindowBlur( + terminal: Terminal, + linkTooltip: HTMLElement +): IDisposable { + const ownerWindow = linkTooltip.ownerDocument?.defaultView + if (!ownerWindow) { + return { dispose: () => undefined } + } + + const resetHover = (): void => { + linkTooltip.style.display = 'none' + resetTerminalLinkifierHoverState(terminal) + } + ownerWindow.addEventListener('blur', resetHover) + return { + dispose: () => ownerWindow.removeEventListener('blur', resetHover) + } +} diff --git a/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset.ts b/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset.ts index d8c40e4b5..6b9d46f19 100644 --- a/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset.ts +++ b/src/renderer/src/lib/pane-manager/terminal-linkifier-hover-reset.ts @@ -3,6 +3,9 @@ import type { Terminal } from '@xterm/xterm' type LinkifierHoverCache = { _lastBufferCell?: unknown _activeLine?: number + // Private xterm cleanup that invokes the active provider's leave callback + // and removes its pointer cursor decoration. + _clearCurrentLink?: () => void // Set while xterm is showing a hovered link; cleared on mouseleave / when the // pointer moves off the link (Linkifier `_clearCurrentLink`). _currentLink?: unknown @@ -34,15 +37,29 @@ type TerminalCoreWithLinkifier = { export function resetTerminalLinkifierHoverState(terminal: Terminal): void { try { const linkifier = (terminal as unknown as TerminalCoreWithLinkifier)._core?.linkifier - if (!linkifier) { - return + // Why: window blur can strand xterm's active link without another mouse + // event, so invoke its own leave path before invalidating the cache. Its + // own try: this runs provider leave() callbacks, and a throwing one must + // not skip the cache invalidation below. + try { + linkifier?._clearCurrentLink?.() + } catch { + /* provider leave() threw — cache invalidation below still applies */ } - if ('_lastBufferCell' in linkifier) { + if (linkifier && '_currentLink' in linkifier) { + linkifier._currentLink = undefined + } + if (linkifier && '_lastBufferCell' in linkifier) { linkifier._lastBufferCell = undefined } - if ('_activeLine' in linkifier) { + if (linkifier && '_activeLine' in linkifier) { linkifier._activeLine = -1 } + // Why: keep the cursor recoverable if a future xterm build omits the + // private cleanup method or has no last mouse event for it to use. + terminal.element + ?.querySelector('.xterm-screen') + ?.classList.remove('xterm-cursor-pointer') } catch { /* linkifier internals unavailable — link recovers on the next cell change */ } diff --git a/tests/e2e/issue-12656-terminal-link-tooltip.spec.ts b/tests/e2e/issue-12656-terminal-link-tooltip.spec.ts new file mode 100644 index 000000000..56c27dc19 --- /dev/null +++ b/tests/e2e/issue-12656-terminal-link-tooltip.spec.ts @@ -0,0 +1,185 @@ +import { randomUUID } from 'node:crypto' +import type { Page, TestInfo } from '@stablyai/playwright-test' +import { expect, test } from './helpers/orca-app' +import { ensureTerminalVisible, waitForSessionReady } from './helpers/store' +import { + getTerminalContent, + sendToTerminal, + waitForActivePanePtyId, + waitForActiveTerminalManager, + waitForTerminalOutput +} from './helpers/terminal' +import { waitForPtyShellEcho } from './terminal-pty-readiness' + +type LinkProbe = { + tabId: string + col: number + row: number +} + +type TooltipState = { + display: string + text: string + currentLinkText: string | null + cursor: string + paneBottom: number + terminalBottom: number + tooltipTop: number + tooltipHeight: number + reserveHeight: number +} + +async function locateUrl(page: Page, url: string): Promise { + return page.evaluate((url) => { + const state = window.__store?.getState() + const worktreeId = state?.activeWorktreeId + const tabId = + state?.activeTabType === 'terminal' + ? (state.activeTabId ?? null) + : worktreeId + ? (state.activeTabIdByWorktree?.[worktreeId] ?? null) + : null + const manager = tabId ? window.__paneManagers?.get(tabId) : null + const pane = manager?.getActivePane?.() ?? manager?.getPanes?.()[0] ?? null + if (!tabId || !pane) { + return null + } + + const buffer = pane.terminal.buffer.active + for (let row = 0; row < pane.terminal.rows; row += 1) { + const line = buffer.getLine(buffer.viewportY + row) + const col = line?.translateToString(true).indexOf(url) ?? -1 + if (col >= 0) { + return { + tabId, + col: col + Math.floor(url.length / 2), + row + } + } + } + return null + }, url) +} + +async function moveToLink(page: Page, probe: LinkProbe): Promise { + await page.evaluate(({ col, row, tabId }) => { + const manager = window.__paneManagers?.get(tabId) + const pane = manager?.getActivePane?.() ?? manager?.getPanes?.()[0] ?? null + const screen = pane?.terminal.element?.querySelector('.xterm-screen') + if (!pane || !screen) { + throw new Error('xterm-screen element unavailable') + } + const rect = screen.getBoundingClientRect() + screen.dispatchEvent( + new MouseEvent('mousemove', { + bubbles: true, + cancelable: true, + clientX: rect.left + (col + 0.5) * (rect.width / pane.terminal.cols), + clientY: rect.top + (row + 0.5) * (rect.height / pane.terminal.rows) + }) + ) + }, probe) +} + +async function readTooltipState(page: Page, tabId: string): Promise { + return page.evaluate((tabId) => { + const manager = window.__paneManagers?.get(tabId) + const pane = manager?.getActivePane?.() ?? manager?.getPanes?.()[0] ?? null + const screen = pane?.terminal.element?.querySelector('.xterm-screen') + if (!pane || !screen) { + throw new Error('terminal pane unavailable') + } + + const linkifier = ( + pane.terminal as unknown as { + _core?: { linkifier?: { currentLink?: { link?: { text?: string } } } } + } + )._core?.linkifier + const paneRect = pane.container.getBoundingClientRect() + const terminalRect = pane.terminal.element?.parentElement?.getBoundingClientRect() + const tooltipRect = pane.linkTooltip.getBoundingClientRect() + const reserveHeight = tooltipRect.height + + return { + display: pane.linkTooltip.style.display, + text: pane.linkTooltip.textContent ?? '', + currentLinkText: linkifier?.currentLink?.link?.text ?? null, + cursor: getComputedStyle(screen).cursor, + paneBottom: paneRect.bottom, + terminalBottom: terminalRect?.bottom ?? 0, + tooltipTop: tooltipRect.top, + tooltipHeight: tooltipRect.height, + reserveHeight + } + }, tabId) +} + +async function captureProof(page: Page, testInfo: TestInfo, name: string): Promise { + await page.screenshot({ path: testInfo.outputPath(name), animations: 'disabled' }) +} + +test.describe('Issue #12656 terminal link tooltip', () => { + test('clears hover state on window blur and reserves the tooltip strip', async ({ + orcaPage + }, testInfo) => { + await waitForSessionReady(orcaPage) + await ensureTerminalVisible(orcaPage) + await waitForActiveTerminalManager(orcaPage) + + const ptyId = await waitForActivePanePtyId(orcaPage) + await waitForPtyShellEcho(orcaPage, ptyId, 15_000) + + const url = `https://example.com/orca-issue-12656-${randomUUID().slice(0, 8)}` + await sendToTerminal( + orcaPage, + ptyId, + `printf 'issue-12656-output-%02d\\n' $(seq 1 64); printf '${url}\\n'\r` + ) + await waitForTerminalOutput(orcaPage, url) + + let probe: LinkProbe | null = null + await expect + .poll( + async () => { + probe = await locateUrl(orcaPage, url) + return probe + }, + { timeout: 5_000, message: 'URL did not become visible in the terminal viewport' } + ) + .not.toBeNull() + if (!probe) { + throw new Error('URL probe disappeared before hover') + } + await expect + .poll(async () => { + await moveToLink(orcaPage, probe) + return readTooltipState(orcaPage, probe.tabId) + }) + .toMatchObject({ display: '', currentLinkText: url }) + + const hovered = await readTooltipState(orcaPage, probe.tabId) + expect(hovered.text).toContain(url) + expect(hovered.tooltipHeight).toBeGreaterThan(0) + expect(hovered.tooltipTop).toBeGreaterThanOrEqual(hovered.terminalBottom - 1) + // Why: bound the gap on both sides. A lower bound alone also passes when the + // reserve var fails to resolve and .xterm-container collapses to height:auto, + // which leaves a huge gap and an undersized terminal. + expect(hovered.paneBottom - hovered.terminalBottom).toBeGreaterThanOrEqual( + hovered.reserveHeight - 1 + ) + expect(hovered.paneBottom - hovered.terminalBottom).toBeLessThanOrEqual( + hovered.reserveHeight + 1 + ) + await captureProof(orcaPage, testInfo, 'issue-12656-fixed-hover.png') + + await orcaPage.evaluate(() => window.dispatchEvent(new Event('blur'))) + await expect + .poll(() => readTooltipState(orcaPage, probe.tabId)) + .toMatchObject({ display: 'none', currentLinkText: null, cursor: 'text' }) + await captureProof(orcaPage, testInfo, 'issue-12656-fixed-after-blur.png') + + // Keep the output assertion adjacent to the visual state checks so the + // reserved strip cannot hide the final terminal line without detection. + await expect.poll(() => getTerminalContent(orcaPage)).toContain(url) + }) +})