Focus terminal pane when notification is clicked (#2356)
* fix: address review findings * increase highlight brightness
This commit is contained in:
parent
fd24061b62
commit
e97b109aac
|
|
@ -113,6 +113,14 @@ describe('registerNotificationHandlers', () => {
|
|||
return call[1] as (event: unknown) => unknown
|
||||
}
|
||||
|
||||
function getNotificationEventHandler(eventName: string): () => void {
|
||||
const call = notificationOnMock.mock.calls.find((c: unknown[]) => c[0] === eventName)
|
||||
if (!call) {
|
||||
throw new Error(`Notification ${eventName} handler not registered`)
|
||||
}
|
||||
return call[1] as () => void
|
||||
}
|
||||
|
||||
it('registers the IPC handler', () => {
|
||||
registerNotificationHandlers({
|
||||
getSettings: () => ({
|
||||
|
|
@ -199,6 +207,54 @@ describe('registerNotificationHandlers', () => {
|
|||
expect(notificationShowMock).toHaveBeenCalledTimes(1)
|
||||
})
|
||||
|
||||
it('focuses the originating terminal pane when a notification with paneKey is clicked', () => {
|
||||
const webContentsSend = vi.fn()
|
||||
const restore = vi.fn()
|
||||
const focus = vi.fn()
|
||||
getAllWindowsMock.mockReturnValue([
|
||||
{
|
||||
isDestroyed: () => false,
|
||||
isFocused: () => false,
|
||||
isMinimized: () => true,
|
||||
restore,
|
||||
focus,
|
||||
webContents: { send: webContentsSend }
|
||||
} as never
|
||||
])
|
||||
registerNotificationHandlers({
|
||||
getSettings: () => ({
|
||||
notifications: {
|
||||
enabled: true,
|
||||
agentTaskComplete: true,
|
||||
terminalBell: true,
|
||||
suppressWhenFocused: true
|
||||
}
|
||||
})
|
||||
} as never)
|
||||
|
||||
const paneKey = 'tab-1:11111111-1111-4111-8111-111111111111'
|
||||
const handler = getDispatchHandler()
|
||||
expect(
|
||||
handler({}, { source: 'agent-task-complete', worktreeId: 'repo::wt1', paneKey })
|
||||
).toEqual({ delivered: true })
|
||||
|
||||
getNotificationEventHandler('click')()
|
||||
|
||||
expect(restore).toHaveBeenCalledTimes(1)
|
||||
expect(focus).toHaveBeenCalledTimes(1)
|
||||
expect(webContentsSend).toHaveBeenCalledWith('ui:activateWorktree', {
|
||||
repoId: 'repo',
|
||||
worktreeId: 'repo::wt1'
|
||||
})
|
||||
expect(webContentsSend).toHaveBeenCalledWith('ui:focusTerminal', {
|
||||
tabId: 'tab-1',
|
||||
worktreeId: 'repo::wt1',
|
||||
leafId: '11111111-1111-4111-8111-111111111111',
|
||||
ackPaneKeyOnSuccess: paneKey,
|
||||
flashFocusedPane: true
|
||||
})
|
||||
})
|
||||
|
||||
it('formats agent-task-complete with the agent response when a status snapshot is present', () => {
|
||||
registerNotificationHandlers({
|
||||
getSettings: () => ({
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@ import type {
|
|||
import { getRepoIdFromWorktreeId } from '../../shared/worktree-id'
|
||||
import type { OrcaRuntimeService } from '../runtime/orca-runtime'
|
||||
import { buildNotificationOptions } from './notification-options'
|
||||
import { parsePaneKey } from '../../shared/stable-pane-id'
|
||||
|
||||
const NOTIFICATION_COOLDOWN_MS = 5000
|
||||
const MAX_NOTIFICATION_SOUND_BYTES = 10 * 1024 * 1024
|
||||
|
|
@ -148,9 +149,9 @@ export function registerNotificationHandlers(store: Store, runtime?: OrcaRuntime
|
|||
setTimeout(release, 5 * 60 * 1000)
|
||||
|
||||
// Why: clicking a notification should bring Orca to the foreground and
|
||||
// switch to the worktree that triggered it. We reuse the existing
|
||||
// ui:activateWorktree IPC channel that the renderer already handles
|
||||
// (setActiveRepo, setActiveView, setActiveWorktree, revealInSidebar).
|
||||
// switch to the worktree/pane that triggered it. Worktree activation owns
|
||||
// repo/sidebar state; the optional focusTerminal follow-up uses the stable
|
||||
// pane leaf id so split-pane notifications land on the exact pane.
|
||||
// Why: worktreeId is formatted as "repoId::worktreePath". If the
|
||||
// separator is missing we cannot reliably extract a repoId, so skip
|
||||
// the click-to-navigate binding — the notification still fires but
|
||||
|
|
@ -174,6 +175,16 @@ export function registerNotificationHandlers(store: Store, runtime?: OrcaRuntime
|
|||
repoId,
|
||||
worktreeId: args.worktreeId
|
||||
})
|
||||
const paneTarget = args.paneKey ? parsePaneKey(args.paneKey) : null
|
||||
if (paneTarget) {
|
||||
win.webContents.send('ui:focusTerminal', {
|
||||
tabId: paneTarget.tabId,
|
||||
worktreeId: args.worktreeId,
|
||||
leafId: paneTarget.leafId,
|
||||
ackPaneKeyOnSuccess: args.paneKey,
|
||||
flashFocusedPane: true
|
||||
})
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1685,7 +1685,13 @@ export type PreloadApi = {
|
|||
callback: (data: { tabId: string; title: string | null }) => void
|
||||
) => () => void
|
||||
onFocusTerminal: (
|
||||
callback: (data: { tabId: string; worktreeId: string; leafId?: string | null }) => void
|
||||
callback: (data: {
|
||||
tabId: string
|
||||
worktreeId: string
|
||||
leafId?: string | null
|
||||
ackPaneKeyOnSuccess?: string
|
||||
flashFocusedPane?: boolean
|
||||
}) => void
|
||||
) => () => void
|
||||
onFocusEditorTab: (
|
||||
callback: (data: { tabId: string; worktreeId: string }) => void
|
||||
|
|
|
|||
|
|
@ -2427,11 +2427,23 @@ const api = {
|
|||
return () => ipcRenderer.removeListener('ui:renameTerminal', listener)
|
||||
},
|
||||
onFocusTerminal: (
|
||||
callback: (data: { tabId: string; worktreeId: string; leafId?: string | null }) => void
|
||||
callback: (data: {
|
||||
tabId: string
|
||||
worktreeId: string
|
||||
leafId?: string | null
|
||||
ackPaneKeyOnSuccess?: string
|
||||
flashFocusedPane?: boolean
|
||||
}) => void
|
||||
): (() => void) => {
|
||||
const listener = (
|
||||
_event: Electron.IpcRendererEvent,
|
||||
data: { tabId: string; worktreeId: string; leafId?: string | null }
|
||||
data: {
|
||||
tabId: string
|
||||
worktreeId: string
|
||||
leafId?: string | null
|
||||
ackPaneKeyOnSuccess?: string
|
||||
flashFocusedPane?: boolean
|
||||
}
|
||||
) => callback(data)
|
||||
ipcRenderer.on('ui:focusTerminal', listener)
|
||||
return () => ipcRenderer.removeListener('ui:focusTerminal', listener)
|
||||
|
|
|
|||
|
|
@ -134,13 +134,11 @@
|
|||
inset: 1px;
|
||||
z-index: 30;
|
||||
pointer-events: none;
|
||||
border: 1.5px solid color-mix(in srgb, var(--terminal-pane-locate) 52%, transparent);
|
||||
border-right-color: color-mix(in srgb, var(--terminal-pane-locate) 68%, transparent);
|
||||
border-left-color: color-mix(in srgb, var(--terminal-pane-locate) 68%, transparent);
|
||||
border: 1.5px solid color-mix(in srgb, var(--terminal-pane-locate) 68%, transparent);
|
||||
border-radius: 2px;
|
||||
box-shadow:
|
||||
0 0 0 2px color-mix(in srgb, var(--terminal-pane-locate) 34%, transparent),
|
||||
0 0 20px color-mix(in srgb, var(--terminal-pane-locate) 40%, transparent);
|
||||
0 0 18px color-mix(in srgb, var(--terminal-pane-locate) 36%, transparent);
|
||||
animation: pane-focus-rim-flash 1.5s ease-out forwards;
|
||||
}
|
||||
|
||||
|
|
@ -148,6 +146,12 @@
|
|||
0% {
|
||||
opacity: 1;
|
||||
}
|
||||
24% {
|
||||
opacity: 0.42;
|
||||
}
|
||||
46% {
|
||||
opacity: 0;
|
||||
}
|
||||
100% {
|
||||
opacity: 0;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1292,7 +1292,12 @@ describe('connectPanePty', () => {
|
|||
expect(deps.markTerminalTabUnread).toHaveBeenCalledWith('tab-1')
|
||||
expect(deps.dispatchNotification).not.toHaveBeenCalled()
|
||||
vi.advanceTimersByTime(250)
|
||||
expect(deps.dispatchNotification).toHaveBeenCalledWith({ source: 'terminal-bell' })
|
||||
expect(deps.dispatchNotification).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
source: 'terminal-bell',
|
||||
paneKey: makePaneKey('tab-1', LEAF_1)
|
||||
})
|
||||
)
|
||||
})
|
||||
|
||||
it('lets concurrent agent-complete notifications win over terminal bell notifications', async () => {
|
||||
|
|
@ -1382,7 +1387,9 @@ describe('connectPanePty', () => {
|
|||
idleHandler('* Codex done')
|
||||
vi.advanceTimersByTime(250)
|
||||
|
||||
expect(deps.dispatchNotification).toHaveBeenCalledWith({ source: 'terminal-bell' })
|
||||
expect(deps.dispatchNotification).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ source: 'terminal-bell' })
|
||||
)
|
||||
vi.advanceTimersByTime(1000)
|
||||
expect(deps.dispatchNotification).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ source: 'agent-task-complete' })
|
||||
|
|
@ -1629,7 +1636,9 @@ describe('connectPanePty', () => {
|
|||
bellHandler()
|
||||
idleHandler('* Codex done')
|
||||
vi.advanceTimersByTime(250)
|
||||
expect(deps.dispatchNotification).not.toHaveBeenCalledWith({ source: 'terminal-bell' })
|
||||
expect(deps.dispatchNotification).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ source: 'terminal-bell' })
|
||||
)
|
||||
|
||||
mockStoreState.settings = {
|
||||
...mockStoreState.settings,
|
||||
|
|
@ -1641,7 +1650,9 @@ describe('connectPanePty', () => {
|
|||
notifyStoreSubscribers()
|
||||
vi.advanceTimersByTime(250)
|
||||
|
||||
expect(deps.dispatchNotification).toHaveBeenCalledWith({ source: 'terminal-bell' })
|
||||
expect(deps.dispatchNotification).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ source: 'terminal-bell' })
|
||||
)
|
||||
})
|
||||
|
||||
it('requires fresh working evidence after notifications are disabled', async () => {
|
||||
|
|
@ -1891,7 +1902,9 @@ describe('connectPanePty', () => {
|
|||
workingHandler()
|
||||
vi.advanceTimersByTime(250)
|
||||
|
||||
expect(deps.dispatchNotification).toHaveBeenCalledWith({ source: 'terminal-bell' })
|
||||
expect(deps.dispatchNotification).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ source: 'terminal-bell' })
|
||||
)
|
||||
expect(deps.dispatchNotification).not.toHaveBeenCalledWith(
|
||||
expect.objectContaining({ source: 'agent-task-complete' })
|
||||
)
|
||||
|
|
|
|||
|
|
@ -359,7 +359,7 @@ export function connectPanePty(
|
|||
return
|
||||
}
|
||||
pendingTerminalBellNotification = false
|
||||
deps.dispatchNotification({ source: 'terminal-bell' })
|
||||
deps.dispatchNotification({ source: 'terminal-bell', paneKey: cacheKey })
|
||||
}, AGENT_TASK_COMPLETE_NOTIFICATION_GRACE_MS)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -168,6 +168,7 @@ export function dispatchTerminalNotification(
|
|||
.dispatch({
|
||||
source: event.source,
|
||||
worktreeId,
|
||||
paneKey: event.paneKey,
|
||||
repoLabel: repo?.displayName,
|
||||
worktreeLabel: worktree?.displayName || worktree?.branch || worktreeId,
|
||||
hasMultipleActiveRepos: countReposNeedingNotificationDisambiguation(state) > 1,
|
||||
|
|
|
|||
|
|
@ -141,6 +141,28 @@ describe('computeAutoAckTargets — codex retain race regression', () => {
|
|||
expect(computeAutoAckTargets(store.getState(), 'tab-codex', CODEX_LEAF_ID)).toEqual([])
|
||||
})
|
||||
|
||||
it('skips sibling panes in the same terminal tab', () => {
|
||||
const store = createTestStore()
|
||||
const activeTabId = 'tab-split'
|
||||
const activePaneKey = makePaneKey(activeTabId, CODEX_LEAF_ID)
|
||||
const siblingPaneKey = makePaneKey(activeTabId, OTHER_LEAF_ID)
|
||||
|
||||
store.getState().setAgentStatus(activePaneKey, {
|
||||
state: 'done',
|
||||
prompt: 'visible pane',
|
||||
agentType: 'codex'
|
||||
})
|
||||
store.getState().setAgentStatus(siblingPaneKey, {
|
||||
state: 'done',
|
||||
prompt: 'hidden sibling pane',
|
||||
agentType: 'claude'
|
||||
})
|
||||
|
||||
expect(computeAutoAckTargets(store.getState(), activeTabId, CODEX_LEAF_ID)).toEqual([
|
||||
activePaneKey
|
||||
])
|
||||
})
|
||||
|
||||
it('acks a paneKey present in BOTH live and retained without throwing', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.setSystemTime(new Date('2026-05-05T12:00:00.000Z'))
|
||||
|
|
|
|||
|
|
@ -45,6 +45,7 @@ import {
|
|||
import { isGitRepoKind } from '../../../shared/repo-kind'
|
||||
import { TOGGLE_FLOATING_TERMINAL_EVENT } from '@/lib/floating-terminal'
|
||||
import { focusTerminalTabSurface } from '@/lib/focus-terminal-tab-surface'
|
||||
import { activateTabAndFocusPane } from '@/lib/activate-tab-and-focus-pane'
|
||||
import { focusRuntimeTerminalSurface } from '@/runtime/sync-runtime-graph'
|
||||
import { setFitOverride, hydrateOverrides } from '@/lib/pane-manager/mobile-fit-overrides'
|
||||
import { setDriverForPty, hydrateDrivers } from '@/lib/pane-manager/mobile-driver-state'
|
||||
|
|
@ -977,19 +978,28 @@ export function useIpcEvents(): void {
|
|||
)
|
||||
|
||||
unsubs.push(
|
||||
window.api.ui.onFocusTerminal(({ tabId, worktreeId, leafId }) => {
|
||||
const store = useAppStore.getState()
|
||||
store.setActiveWorktree(worktreeId)
|
||||
// Why: CLI-driven focus is a user-initiated switch; stamp focus
|
||||
// recency for Cmd+J. See docs/cmd-j-empty-query-ordering.md.
|
||||
store.markWorktreeVisited(worktreeId)
|
||||
store.setActiveView('terminal')
|
||||
store.setActiveTab(tabId)
|
||||
store.revealWorktreeInSidebar(worktreeId)
|
||||
if (!focusRuntimeTerminalSurface(tabId, leafId)) {
|
||||
focusTerminalTabSurface(tabId, leafId)
|
||||
window.api.ui.onFocusTerminal(
|
||||
({ tabId, worktreeId, leafId, ackPaneKeyOnSuccess, flashFocusedPane }) => {
|
||||
const store = useAppStore.getState()
|
||||
store.setActiveWorktree(worktreeId)
|
||||
// Why: CLI-driven focus is a user-initiated switch; stamp focus
|
||||
// recency for Cmd+J. See docs/cmd-j-empty-query-ordering.md.
|
||||
store.markWorktreeVisited(worktreeId)
|
||||
store.setActiveView('terminal')
|
||||
store.setActiveTab(tabId)
|
||||
store.revealWorktreeInSidebar(worktreeId)
|
||||
if (ackPaneKeyOnSuccess || flashFocusedPane) {
|
||||
activateTabAndFocusPane(tabId, leafId ?? null, {
|
||||
...(ackPaneKeyOnSuccess ? { ackPaneKeyOnSuccess } : {}),
|
||||
...(flashFocusedPane ? { flashFocusedPane: true } : {})
|
||||
})
|
||||
return
|
||||
}
|
||||
if (!focusRuntimeTerminalSurface(tabId, leafId)) {
|
||||
focusTerminalTabSurface(tabId, leafId)
|
||||
}
|
||||
}
|
||||
})
|
||||
)
|
||||
)
|
||||
|
||||
unsubs.push(
|
||||
|
|
|
|||
|
|
@ -1748,6 +1748,8 @@ export type NotificationEventSource = 'agent-task-complete' | 'terminal-bell' |
|
|||
export type NotificationDispatchRequest = {
|
||||
source: NotificationEventSource
|
||||
worktreeId?: string
|
||||
/** Stable `${tabId}:${leafId}` terminal pane key for click-to-focus routing. */
|
||||
paneKey?: string
|
||||
repoLabel?: string
|
||||
worktreeLabel?: string
|
||||
hasMultipleActiveRepos?: boolean
|
||||
|
|
|
|||
Loading…
Reference in New Issue