diff --git a/src/renderer/src/components/WorktreeJumpPalette.tsx b/src/renderer/src/components/WorktreeJumpPalette.tsx index c034994f1..de0259c15 100644 --- a/src/renderer/src/components/WorktreeJumpPalette.tsx +++ b/src/renderer/src/components/WorktreeJumpPalette.tsx @@ -40,6 +40,7 @@ import StatusIndicator from '@/components/sidebar/StatusIndicator' import { cn } from '@/lib/utils' import { getWorktreeStatus, getWorktreeStatusLabel } from '@/lib/worktree-status' import { activateAndRevealWorktree } from '@/lib/worktree-activation' +import { queueWorkspaceActivationTerminalFocus } from '@/lib/workspace-activation-terminal-focus' import { findWorktreeById } from '@/store/slices/worktree-helpers' import { getWorktreePaletteSearchScope, @@ -1271,12 +1272,16 @@ export default function WorktreeJumpPalette(): React.JSX.Element | null { ) return } - activateAndRevealWorktree(worktreeId) + const activation = activateAndRevealWorktree(worktreeId) recordFeatureInteraction('cmd-j-workspace-open') skipRestoreFocusRef.current = true closeModal() setSelectedItemId('') - focusFallbackSurface() + // Why: #9939 — the unscoped fallback grabs the first terminal in the document, which is + // often the worktree we just left, now hidden. Focus the destination's own tab instead. + if (!queueWorkspaceActivationTerminalFocus(worktreeId, activation)) { + focusFallbackSurface() + } }, [closeModal, focusFallbackSurface, recordFeatureInteraction] ) @@ -1445,7 +1450,9 @@ export default function WorktreeJumpPalette(): React.JSX.Element | null { return } if (previousWorktreeIdRef.current) { - focusFallbackSurface() + // Why: #9939 — sidebar reveal keeps the same worktree, so restore the exact element the + // user came from rather than the first terminal in the document. + focusFallbackSurface(previousFocusElementRef.current) } }, [ @@ -1516,7 +1523,9 @@ export default function WorktreeJumpPalette(): React.JSX.Element | null { const activeMatch = matches.find((w) => w.repoId === state.activeRepoId) ?? matches[0] if (activeMatch) { closeModal() - activateAndRevealWorktree(activeMatch.id) + // Why: #9939 — jumping to an already-open workspace must focus its own terminal. + const activation = activateAndRevealWorktree(activeMatch.id) + queueWorkspaceActivationTerminalFocus(activeMatch.id, activation) recordFeatureInteraction('cmd-j-workspace-open') return } @@ -1543,7 +1552,9 @@ export default function WorktreeJumpPalette(): React.JSX.Element | null { const activeMatch = matches.find((w) => w.repoId === state.activeRepoId) ?? matches[0] if (activeMatch) { closeModal() - activateAndRevealWorktree(activeMatch.id) + // Why: #9939 — jumping to an already-open workspace must focus its own terminal. + const activation = activateAndRevealWorktree(activeMatch.id) + queueWorkspaceActivationTerminalFocus(activeMatch.id, activation) recordFeatureInteraction('cmd-j-workspace-open') return } diff --git a/src/renderer/src/components/cmd-j/palette-activation-focus-routing.test.ts b/src/renderer/src/components/cmd-j/palette-activation-focus-routing.test.ts new file mode 100644 index 000000000..0222a0191 --- /dev/null +++ b/src/renderer/src/components/cmd-j/palette-activation-focus-routing.test.ts @@ -0,0 +1,58 @@ +import { readFileSync } from 'node:fs' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' + +// Why: happy-dom does not reproduce Chromium's display:none focus no-op, so the #9939 regression +// is invisible to behavioral tests. Pin the routing decision at the source level instead. +function paletteSource(): string { + return readFileSync(join(__dirname, '..', 'WorktreeJumpPalette.tsx'), 'utf8') +} + +function sourceBetween(source: string, startPattern: string, endPattern: string): string { + const start = source.indexOf(startPattern) + expect(start).toBeGreaterThanOrEqual(0) + const end = source.indexOf(endPattern, start + startPattern.length) + expect(end).toBeGreaterThan(start) + return source.slice(start, end) +} + +describe('Cmd+J activation focus routing (#9939)', () => { + it('routes worktree selection through the scoped helper before any unscoped fallback', () => { + const handler = sourceBetween( + paletteSource(), + 'const handleSelectWorktree = useCallback', + 'const handleSelectBrowserPage' + ) + + expect(handler).toContain('queueWorkspaceActivationTerminalFocus(worktreeId, activation)') + // The fallback must be reachable only when the helper declines the destination. + expect(handler).toMatch( + /if \(!queueWorkspaceActivationTerminalFocus\(worktreeId, activation\)\) \{\s*focusFallbackSurface\(\)\s*\}/ + ) + // An unconditional fallback is the exact shape of the original bug, so the only bare call + // allowed is the guarded one inside the if-block above. + expect(handler.match(/focusFallbackSurface\(\)/g)?.length).toBe(1) + }) + + it('restores the pre-palette element for project targets instead of the first terminal found', () => { + const handler = sourceBetween( + paletteSource(), + 'const handleSelectProjectTarget = useCallback', + 'const handleSelectItem = useCallback' + ) + + expect(handler).toContain('focusFallbackSurface(previousFocusElementRef.current)') + expect(handler).not.toMatch(/focusFallbackSurface\(\)/) + }) + + it('focuses the destination workspace when jumping to an already-open issue match', () => { + const source = paletteSource() + const activationCalls = + source.match(/const activation = activateAndRevealWorktree\(activeMatch\.id\)/g)?.length ?? 0 + // Both issue-number match paths (PR and issue lookup) must focus the destination. + expect(activationCalls).toBe(2) + expect( + source.match(/queueWorkspaceActivationTerminalFocus\(activeMatch\.id, activation\)/g)?.length + ).toBe(activationCalls) + }) +}) diff --git a/src/renderer/src/hooks/useComposerState.ts b/src/renderer/src/hooks/useComposerState.ts index 1d9a4d9d5..2411ed3b9 100644 --- a/src/renderer/src/hooks/useComposerState.ts +++ b/src/renderer/src/hooks/useComposerState.ts @@ -145,7 +145,7 @@ import { type ExecutionHostId } from '../../../shared/execution-host' import { getHostDisplayLabelOverrides } from '../../../shared/host-setting-overrides' -import { queueNewWorkspaceTerminalFocus } from '@/lib/new-workspace-terminal-focus' +import { queueWorkspaceActivationTerminalFocus } from '@/lib/workspace-activation-terminal-focus' import { getSettingsForRepoRuntimeOwner } from '@/lib/repo-runtime-owner' import { getSuggestedCreatureName } from '@/components/sidebar/worktree-name-suggestions' import type { SmartWorkspaceNameSelection } from '@/components/new-workspace/SmartWorkspaceNameField' @@ -3560,7 +3560,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS clearNewWorkspaceDraft() } onCreated?.() - queueNewWorkspaceTerminalFocus(worktree.id, activation) + queueWorkspaceActivationTerminalFocus(worktree.id, activation) } catch (error) { const formattedError = formatWorkspaceCreateError(error) setCreateError(formattedError) diff --git a/src/renderer/src/lib/new-workspace-terminal-focus.test.ts b/src/renderer/src/lib/workspace-activation-terminal-focus.test.ts similarity index 63% rename from src/renderer/src/lib/new-workspace-terminal-focus.test.ts rename to src/renderer/src/lib/workspace-activation-terminal-focus.test.ts index 62decac8b..74e95953c 100644 --- a/src/renderer/src/lib/new-workspace-terminal-focus.test.ts +++ b/src/renderer/src/lib/workspace-activation-terminal-focus.test.ts @@ -18,7 +18,7 @@ vi.mock('@/store', () => ({ } })) -import { queueNewWorkspaceTerminalFocus } from './new-workspace-terminal-focus' +import { queueWorkspaceActivationTerminalFocus } from './workspace-activation-terminal-focus' type FocusState = { activeWorktreeId: string | null @@ -39,7 +39,7 @@ function flushFrame(): void { frame?.() } -describe('queueNewWorkspaceTerminalFocus', () => { +describe('queueWorkspaceActivationTerminalFocus', () => { beforeEach(() => { vi.clearAllMocks() pendingFrame = null @@ -58,7 +58,7 @@ describe('queueNewWorkspaceTerminalFocus', () => { activeTabId: 'tab-1' }) - queueNewWorkspaceTerminalFocus('wt-1', { primaryTabId: 'tab-1' }) + queueWorkspaceActivationTerminalFocus('wt-1', { primaryTabId: 'tab-1' }) expect(focusRuntimeTerminalSurfaceMock).not.toHaveBeenCalled() flushFrame() @@ -75,7 +75,7 @@ describe('queueNewWorkspaceTerminalFocus', () => { activeTabId: 'tab-adopted' }) - queueNewWorkspaceTerminalFocus('wt-1', { primaryTabId: null }) + queueWorkspaceActivationTerminalFocus('wt-1', { primaryTabId: null }) flushFrame() expect(focusRuntimeTerminalSurfaceMock).toHaveBeenCalledWith('tab-adopted') @@ -91,13 +91,51 @@ describe('queueNewWorkspaceTerminalFocus', () => { activeTabId: 'tab-1' }) - queueNewWorkspaceTerminalFocus('wt-1', { primaryTabId: 'tab-1' }) + queueWorkspaceActivationTerminalFocus('wt-1', { primaryTabId: 'tab-1' }) flushFrame() expect(focusRuntimeTerminalSurfaceMock).toHaveBeenCalledWith('tab-1') expect(focusTerminalTabSurfaceMock).not.toHaveBeenCalled() }) + // Why: #9939 — the return value is what stops the palette from running its whole-document + // fallback, which would grab the worktree the user just left, now mounted but hidden. + it('reports that it claimed focus for a terminal destination', () => { + setFocusState({ + activeWorktreeId: 'wt-1', + activeView: 'terminal', + activeTabType: 'terminal', + activeTabId: 'tab-adopted' + }) + + expect(queueWorkspaceActivationTerminalFocus('wt-1', { primaryTabId: null })).toBe(true) + }) + + it('declines a destination whose restored surface is not a terminal', () => { + setFocusState({ + activeWorktreeId: 'wt-1', + activeView: 'terminal', + activeTabType: 'browser', + activeTabId: 'tab-adopted' + }) + + expect(queueWorkspaceActivationTerminalFocus('wt-1', { primaryTabId: null })).toBe(false) + flushFrame() + expect(focusRuntimeTerminalSurfaceMock).not.toHaveBeenCalled() + expect(focusTerminalTabSurfaceMock).not.toHaveBeenCalled() + }) + + it('declines a destination that has no terminal tab yet', () => { + setFocusState({ + activeWorktreeId: 'wt-1', + activeView: 'terminal', + activeTabType: 'terminal', + activeTabId: null + }) + + expect(queueWorkspaceActivationTerminalFocus('wt-1', { primaryTabId: null })).toBe(false) + }) + it('does not steal focus if the user leaves the created workspace first', () => { const state: FocusState = { activeWorktreeId: 'wt-1', @@ -107,7 +145,7 @@ describe('queueNewWorkspaceTerminalFocus', () => { } setFocusState(state) - queueNewWorkspaceTerminalFocus('wt-1', { primaryTabId: 'tab-1' }) + queueWorkspaceActivationTerminalFocus('wt-1', { primaryTabId: 'tab-1' }) state.activeWorktreeId = 'wt-2' flushFrame() diff --git a/src/renderer/src/lib/new-workspace-terminal-focus.ts b/src/renderer/src/lib/workspace-activation-terminal-focus.ts similarity index 76% rename from src/renderer/src/lib/new-workspace-terminal-focus.ts rename to src/renderer/src/lib/workspace-activation-terminal-focus.ts index 8a5aa6ce3..7ef6f61ec 100644 --- a/src/renderer/src/lib/new-workspace-terminal-focus.ts +++ b/src/renderer/src/lib/workspace-activation-terminal-focus.ts @@ -3,7 +3,7 @@ import { focusTerminalTabSurface } from '@/lib/focus-terminal-tab-surface' import { focusRuntimeTerminalSurface } from '@/runtime/sync-runtime-graph' import type { ActivateAndRevealResult } from '@/lib/worktree-activation' -function resolveCreatedWorkspaceTerminalTabId( +function resolveActivatedWorkspaceTerminalTabId( worktreeId: string, activation: ActivateAndRevealResult | false ): string | null { @@ -21,13 +21,15 @@ function resolveCreatedWorkspaceTerminalTabId( return state.activeTabId } -export function queueNewWorkspaceTerminalFocus( +// Why: returns false when the destination's restored surface is not a terminal, so callers can +// fall back instead of focusing an unrelated worktree's mounted-but-hidden terminal (#9939). +export function queueWorkspaceActivationTerminalFocus( worktreeId: string, activation: ActivateAndRevealResult | false -): void { - const tabId = resolveCreatedWorkspaceTerminalTabId(worktreeId, activation) +): boolean { + const tabId = resolveActivatedWorkspaceTerminalTabId(worktreeId, activation) if (!tabId) { - return + return false } requestAnimationFrame(() => { @@ -48,4 +50,5 @@ export function queueNewWorkspaceTerminalFocus( focusTerminalTabSurface(tabId) } }) + return true } diff --git a/src/renderer/src/lib/worktree-creation-flow.test.ts b/src/renderer/src/lib/worktree-creation-flow.test.ts index f614c679c..90af3fe69 100644 --- a/src/renderer/src/lib/worktree-creation-flow.test.ts +++ b/src/renderer/src/lib/worktree-creation-flow.test.ts @@ -57,8 +57,8 @@ vi.mock('@/lib/worktree-activation', () => ({ ensureWorktreeHasInitialTerminal: vi.fn() })) -vi.mock('@/lib/new-workspace-terminal-focus', () => ({ - queueNewWorkspaceTerminalFocus: vi.fn() +vi.mock('@/lib/workspace-activation-terminal-focus', () => ({ + queueWorkspaceActivationTerminalFocus: vi.fn() })) vi.mock('@/lib/new-workspace', () => ({ @@ -80,7 +80,7 @@ import { activateAndRevealWorktree, ensureWorktreeHasInitialTerminal } from '@/lib/worktree-activation' -import { queueNewWorkspaceTerminalFocus } from '@/lib/new-workspace-terminal-focus' +import { queueWorkspaceActivationTerminalFocus } from '@/lib/workspace-activation-terminal-focus' import { beginBackgroundWorktreePreparation, continueBackgroundWorktreeCreation, @@ -498,7 +498,7 @@ describe('staged background worktree creation', () => { undefined, { activateCreatedTabs: false } ) - expect(queueNewWorkspaceTerminalFocus).not.toHaveBeenCalled() + expect(queueWorkspaceActivationTerminalFocus).not.toHaveBeenCalled() expect(store.removePendingWorktreeCreation).toHaveBeenCalledWith('creation-1', { cleanupVm: false }) diff --git a/src/renderer/src/lib/worktree-creation-flow.ts b/src/renderer/src/lib/worktree-creation-flow.ts index a975405de..dcfc25a33 100644 --- a/src/renderer/src/lib/worktree-creation-flow.ts +++ b/src/renderer/src/lib/worktree-creation-flow.ts @@ -8,7 +8,7 @@ import { type WorktreeStartupPayload } from '@/lib/worktree-activation' import { ensureAgentStartupInTerminal } from '@/lib/new-workspace' -import { queueNewWorkspaceTerminalFocus } from '@/lib/new-workspace-terminal-focus' +import { queueWorkspaceActivationTerminalFocus } from '@/lib/workspace-activation-terminal-focus' import { getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' import { attachEphemeralVmRuntimeToWorkspace, @@ -273,7 +273,7 @@ async function executeWorktreeCreation( }) } if (shouldActivateOnCompletion && !preparedRequest.suppressTerminalFocusOnCompletion) { - queueNewWorkspaceTerminalFocus(worktree.id, activation) + queueWorkspaceActivationTerminalFocus(worktree.id, activation) } // Why: awaiting the note IPC before the swap would add a visible round-trip to