fix(cmd-j): focus the destination workspace's own terminal after a jump (#10695)
Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
parent
24706ccff0
commit
2bb3276a35
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
})
|
||||
})
|
||||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
||||
|
|
@ -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
|
||||
}
|
||||
|
|
@ -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
|
||||
})
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in New Issue