fix(native-chat): reveal the picker on desktop too, and harden the mobile reveal

Three fixes from review of the first commit:

- Desktop advertised `/resume` in its `/` menu but never revealed the TUI the
  picker draws in, so the new entry led to the same dead end it removes on
  mobile. The chat view — the only layer that knows it is covering the terminal
  — now owns that decision, through a hook that also keeps the local `Ran /x`
  marker. It covers both desktop dispatch paths and agents with no
  session-option catalog behind them.
- The mobile reveal toggled the view from a render-snapshot guard while the
  toggle itself reads live state. A send resolving after the user switched to
  the terminal would flip them back into chat, hiding the picker and pinning
  the tab there. Added an idempotent `showTabTerminalView` and used it.
- An ack-lost send returned before the reveal. Those usually did land, so the
  picker was open and invisible on exactly the flaky links this bug came from.
This commit is contained in:
Brennan Benson
2026-08-26 15:23:39 -07:00
parent 9aec0c74cc
commit 71a68c5528
10 changed files with 267 additions and 29 deletions
@@ -13,6 +13,7 @@ const holdUnconfirmedSend = vi.fn()
// Mutable stand-ins so the launch-draft wiring below can drive chat resolution
// and transcript state; defaults keep the send-seam tests unchanged.
const toggleTabChatView = vi.fn()
const showTabTerminalView = vi.fn()
const viewMode = { isTabChatView: (_tabId: string) => true }
const sessionState = { messages: [] as unknown[], status: 'ready', transcriptLoading: false }
const draftsArgs: Record<string, unknown>[] = []
@@ -28,7 +29,8 @@ const promptsState = {
vi.mock('./use-mobile-session-view-mode', () => ({
useMobileSessionViewMode: () => ({
isTabChatView: (tabId: string) => viewMode.isTabChatView(tabId),
toggleTabChatView
toggleTabChatView,
showTabTerminalView
})
}))
vi.mock('./use-mobile-native-chat-session', () => ({
@@ -824,6 +826,14 @@ describe('useMobileNativeChatController /resume surfaces the agent picker', () =
beforeEach(() => {
vi.clearAllMocks()
// The pre-body input clear is a real transport write (only the message send
// itself is mocked), so it has to be accepted or nothing reaches the agent.
clientStub.sendRequest.mockResolvedValue({
id: 'send',
ok: true,
result: { send: { accepted: true } },
_meta: { runtimeId: 'r' }
})
resetMobileNativeChatStaleInputForTests()
captureSendOrigin.mockReturnValue(ORIGIN)
sendWithOutcome.mockResolvedValue('accepted')
@@ -838,28 +848,45 @@ describe('useMobileNativeChatController /resume surfaces the agent picker', () =
controller = null
})
it('flips the tab to the terminal view after an accepted /resume', async () => {
it('reveals the terminal after an accepted /resume', async () => {
await act(async () => {
await controller!.handleNativeChatSend('/resume')
})
expect(toggleTabChatView).toHaveBeenCalledWith('tab-1')
expect(showTabTerminalView).toHaveBeenCalledWith('tab-1')
})
it('reveals the terminal when the ack was lost, since the send usually landed', async () => {
// An ack-lost /resume on a flaky relay is exactly when the picker is open and
// invisible — the symptom this fixes. Revealing is self-correcting.
sendWithOutcome.mockResolvedValue('unknown')
await act(async () => {
await controller!.handleNativeChatSend('/resume')
})
expect(showTabTerminalView).toHaveBeenCalledWith('tab-1')
})
it('does not reveal the terminal when the write was rejected', async () => {
sendWithOutcome.mockResolvedValue('rejected')
await act(async () => {
await controller!.handleNativeChatSend('/resume')
})
expect(showTabTerminalView).not.toHaveBeenCalled()
})
it('stays in chat view for an ordinary prompt', async () => {
await act(async () => {
await controller!.handleNativeChatSend('resume where we left off')
})
expect(toggleTabChatView).not.toHaveBeenCalled()
expect(showTabTerminalView).not.toHaveBeenCalled()
})
it('does not flip a tab already showing the terminal', async () => {
// The toggle is symmetric — firing it here would yank the user INTO chat,
// hiding the very picker the command just opened.
viewMode.isTabChatView = () => false
act(() => renderer?.update(createElement(Harness)))
it('reveals rather than toggles, so a late resolve cannot hide the picker', async () => {
// The toggle is symmetric and this call lands after an async send: a user who
// switched to the terminal meanwhile would be flipped back into chat by it.
await act(async () => {
await controller!.handleNativeChatSend('/resume')
})
expect(toggleTabChatView).not.toHaveBeenCalled()
expect(showTabTerminalView).toHaveBeenCalledWith('tab-1')
})
})
@@ -57,7 +57,10 @@ export function useMobileNativeChatController(args: {
onSendError,
onSendResolved
} = args
const { isTabChatView, toggleTabChatView } = useMobileSessionViewMode({ hostId, worktreeId })
const { isTabChatView, toggleTabChatView, showTabTerminalView } = useMobileSessionViewMode({
hostId,
worktreeId
})
const activeChatResolution =
activeSessionTab && activeSessionTabId && isTabChatView(activeSessionTabId)
@@ -212,11 +215,14 @@ export function useMobileNativeChatController(args: {
const recordSessionOptionCommandRef = useRef<(command: string) => void>(() => {})
// Bring the terminal view forward when an agent-owned picker command is used.
// Why the absolute setter and not a toggle: this runs after an async send, and
// a user who switched to the terminal meanwhile would otherwise be flipped back
// into chat by this stale caller — hiding the very picker it just revealed.
const handleAgentPicker = useCallback(() => {
if (activeSessionTabId && isTabChatView(activeSessionTabId)) {
toggleTabChatView(activeSessionTabId)
if (activeSessionTabId) {
showTabTerminalView(activeSessionTabId)
}
}, [activeSessionTabId, isTabChatView, toggleTabChatView])
}, [activeSessionTabId, showTabTerminalView])
const {
send: handleNativeChatSend,
@@ -502,6 +502,15 @@ describe('useMobileNativeChatMessageSend agent-picker commands', () => {
api = null
})
it('surfaces the picker when the ack was lost, since the send usually landed', async () => {
sendWithOutcome.mockResolvedValue('unknown')
mount('claude')
await act(async () => {
await api!.send('/resume')
})
expect(onAgentPicker).toHaveBeenCalledTimes(1)
})
it('surfaces the picker after an accepted Claude /resume', async () => {
mount('claude')
await act(async () => {
@@ -144,6 +144,12 @@ export function useMobileNativeChatMessageSend(args: {
}
const seededLaunchDraft = readSeededLaunchDraftSeed()
const classification = classifyMobileNativeChatSend(agent, text)
// Why the agent captured at send start, not the live ref: a tab switch
// during the write must not judge this command against the new tab's
// catalog, exactly as `recordCommand` is captured above. Session-option
// dispatches (`recordControlSend` false) keep their own picker decision.
const revealsAgentPicker =
recordControlSend && mobileNativeChatSendOpensAgentPicker(agent, text)
const typesCodexCommand =
agent === 'codex' &&
classification !== 'chat' &&
@@ -200,6 +206,13 @@ export function useMobileNativeChatMessageSend(args: {
// turn, so an optimistic bubble would never reconcile and the
// unconfirmed hold could never observe a landing.
if (outcome === 'unknown') {
// Why reveal on an ack-lost send: it usually WAS delivered (the comment
// above), so the picker is probably already open on the host. Revealing is
// self-correcting — the user sees the picker or their untouched prompt —
// while staying put reproduces this bug on the flakiest links.
if (revealsAgentPicker) {
onAgentPicker?.()
}
if (classification === 'chat') {
// Why: an ack-lost send usually WAS delivered (issue seen on cellular
// relay) — verify via the transcript echo instead of a false "not sent".
@@ -224,10 +237,7 @@ export function useMobileNativeChatMessageSend(args: {
// The session-option catalog can recognize controls omitted from the
// autocomplete catalog (for example Claude `/model` and `/fast`).
recordCommand(text.trim())
// Why the agent captured at send start, not the live ref: a tab switch
// during the write must not judge this command against the new tab's
// catalog, exactly as `recordCommand` is captured above.
if (mobileNativeChatSendOpensAgentPicker(agent, text)) {
if (revealsAgentPicker) {
onAgentPicker?.()
}
}
@@ -143,6 +143,32 @@ describe('useMobileSessionViewMode', () => {
expect(controller?.isTabChatView('t1')).toBe(true)
})
it('reveals the terminal from chat and persists it', async () => {
await mount({ defaultView: 'chat' })
await act(async () => {
controller?.showTabTerminalView('t1')
await Promise.resolve()
})
expect(updateSessionViewOverride).toHaveBeenLastCalledWith('h', 'w', 't1', 'terminal')
expect(controller?.isTabChatView('t1')).toBe(false)
})
it('is a no-op on a tab already showing the terminal, unlike the toggle', async () => {
// STA-4617: the reveal runs after an async send, so a caller that resolves
// late must never flip a terminal tab back into chat — nor re-persist it.
await mount({ defaultView: 'terminal' })
await act(async () => {
controller?.showTabTerminalView('t1')
await Promise.resolve()
})
expect(updateSessionViewOverride).not.toHaveBeenCalled()
expect(controller?.isTabChatView('t1')).toBe(false)
})
it('does not expose overrides from the previous host while the next scope loads', async () => {
const nextScopeLoad = deferred<Map<string, MobileSessionView>>()
vi.mocked(readSessionViewOverridesPreference).mockImplementation((hostId) =>
@@ -42,6 +42,8 @@ export type MobileSessionViewModeController = {
/** Whether a tab's effective view is chat (per-tab override, else the default). */
isTabChatView: (tabId: string) => boolean
toggleTabChatView: (tabId: string) => void
/** Reveal the tab's terminal. Idempotent — safe from a stale-snapshot caller. */
showTabTerminalView: (tabId: string) => void
}
/** Resolves each tab's terminal/chat view: a per-device default (reloaded on focus
@@ -139,8 +141,11 @@ export function useMobileSessionViewMode(args: {
[defaultView, hostId, viewOverridesState, worktreeId]
)
const toggleTabChatView = useCallback(
(tabId: string) => {
/** Write a tab's view, resolving the target from the LIVE override state (the
* ref, not a render snapshot). Callers pass how to derive it so a toggle and
* an absolute set cannot disagree about what "currently chat" means. */
const applyTabView = useCallback(
(tabId: string, resolveNext: (currentlyChat: boolean) => MobileSessionView) => {
const current = viewOverridesStateRef.current
const currentScope = isOverrideScope(current, hostId, worktreeId)
? current
@@ -155,7 +160,12 @@ export function useMobileSessionViewMode(args: {
// a tab following a chat default can still be pinned back to terminal.
const fallbackView = currentScope.loaded ? defaultViewRef.current : 'terminal'
const currentlyChat = (overrides.get(tabId) ?? fallbackView) === 'chat'
const nextView = currentlyChat ? 'terminal' : 'chat'
const nextView = resolveNext(currentlyChat)
// Why: an absolute set that already holds must not re-persist the override
// (a toggle never lands here — its target is the opposite by construction).
if (currentlyChat === (nextView === 'chat')) {
return
}
overrides.set(tabId, nextView)
const next = { ...currentScope, overrides }
viewOverridesStateRef.current = next
@@ -204,5 +214,19 @@ export function useMobileSessionViewMode(args: {
[ensureViewOverridesRuntime, hostId, worktreeId]
)
return { isTabChatView, toggleTabChatView }
const toggleTabChatView = useCallback(
(tabId: string) =>
applyTabView(tabId, (currentlyChat) => (currentlyChat ? 'terminal' : 'chat')),
[applyTabView]
)
// Idempotent, unlike the toggle: a caller holding a stale view snapshot (an
// in-flight send that resolves after the user switched views themselves)
// cannot flip a terminal tab back into chat and hide what it just revealed.
const showTabTerminalView = useCallback(
(tabId: string) => applyTabView(tabId, () => 'terminal'),
[applyTabView]
)
return { isTabChatView, toggleTabChatView, showTabTerminalView }
}
@@ -7,6 +7,10 @@ import type {
SessionOptionsSurface
} from '../../../../shared/native-chat-session-options'
import type * as nativeChatAgentProfiles from '../../../../shared/native-chat-agent-profiles'
import {
getAgentSlashCommands,
type SlashCommandSuggestion
} from '../../../../shared/native-chat-slash-commands'
import { clearNativeChatSessionOptionCacheForTests } from './native-chat-session-option-cache'
import { clearNativeChatModelEnrichmentForTests } from './native-chat-session-option-enrichment'
@@ -31,6 +35,9 @@ const mocks = vi.hoisted(() => ({
createClaudeModelSwitchConfirmationObserver: vi.fn(),
discoverCommitMessageModels: vi.fn(),
draft: 'hello',
// The catalog the composer classifies against. Empty by default (these tests
// predate the shared catalog); a test that needs real commands assigns them.
verifiedCommands: [] as readonly SlashCommandSuggestion[],
getMainBufferSnapshot: vi.fn(),
sendHandle: { cancel: vi.fn(), settleAfterMs: 500 },
sendNativeChatMessage: vi.fn(),
@@ -79,7 +86,7 @@ vi.mock('./claude-model-switch-confirmation', () => ({
}))
vi.mock('../../../../shared/native-chat-agent-profiles', async (importOriginal) => ({
...(await importOriginal<typeof nativeChatAgentProfiles>()),
getVerifiedNativeChatCommands: () => []
getVerifiedNativeChatCommands: () => mocks.verifiedCommands
}))
vi.mock('@/lib/native-chat-telemetry', () => ({
emitNativeChatMessageSent: vi.fn(),
@@ -148,6 +155,7 @@ describe('NativeChatComposer', () => {
mocks.fieldProps = null
mocks.modelSwitchOutcome = 'applied'
mocks.draft = 'hello'
mocks.verifiedCommands = []
mocks.draftScopeKeys.length = 0
mocks.confirmationObserver = null
mocks.createClaudeModelSwitchConfirmationObserver.mockImplementation(() => {
@@ -293,6 +301,31 @@ describe('NativeChatComposer', () => {
expect(mocks.sendNativeChatTypedCommand).not.toHaveBeenCalled()
})
it.each(['claude', 'openclaude'] as const)(
'reports a %s /resume send as a verified command, so the view can reveal the TUI',
(agent) => {
// STA-4617: the reveal hangs off this callback (the composer cannot know the
// chat surface is covering the terminal), so an unreported send strands it.
const onSlashCommand = vi.fn()
mocks.verifiedCommands = getAgentSlashCommands(agent)
mocks.draft = '/resume'
render(
<NativeChatComposer
terminalTabId="tab-1"
paneKey="tab-1:leaf-1"
targetPtyId="pty-1"
agent={agent}
onSlashCommand={onSlashCommand}
/>
)
act(() => mocks.fieldProps?.onSend?.())
expect(mocks.sendNativeChatMessage).toHaveBeenCalledWith({}, 'pty-1', '/resume', undefined)
expect(onSlashCommand).toHaveBeenCalledWith('/resume')
}
)
it('retires the launch-draft seed once a send clears the TUI input line', () => {
render(
<NativeChatComposer
@@ -30,7 +30,6 @@ import {
type NativeChatPendingSend
} from './native-chat-pending'
import {
appendCommandMarkerCache,
applyCommandMarkerBoundaries,
commandMarkersAsMessages,
readCommandMarkerCache,
@@ -49,6 +48,7 @@ import {
emptyNativeChatContextMenuActions,
useNativeChatContextMenu
} from './use-native-chat-context-menu'
import { useNativeChatSlashCommandDispatched } from './use-native-chat-slash-command-dispatched'
import { resolveNativeChatFileLinkContext } from './native-chat-file-link'
import { selectNativeChatRuntimeEnvironmentId } from './native-chat-runtime-owner'
import { useNativeChatPasteBridge } from './use-native-chat-paste-bridge'
@@ -257,12 +257,12 @@ function NativeChatResolvedView({
},
[pendingScope]
)
const onSlashCommand = useCallback(
(command: string) => {
setCommandMarkers(appendCommandMarkerCache(commandMarkerScope, command))
},
[commandMarkerScope]
)
const onSlashCommand = useNativeChatSlashCommandDispatched({
agent,
commandMarkerScope,
setCommandMarkers,
onSwitchToTerminal
})
const launchPromptMessage = useMemo(
() => launchPromptAsMessage(paneLaunchPrompt, session.messages),
@@ -0,0 +1,63 @@
// @vitest-environment happy-dom
import { act, renderHook } from '@testing-library/react'
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { clearCommandMarkerCacheForTests } from './native-chat-command-marker'
import { useNativeChatSlashCommandDispatched } from './use-native-chat-slash-command-dispatched'
const SCOPE = { paneKey: 'tab-1:leaf-1', agent: 'claude', sessionId: 'session-1' } as const
describe('useNativeChatSlashCommandDispatched', () => {
const onSwitchToTerminal = vi.fn()
const setCommandMarkers = vi.fn()
beforeEach(() => {
vi.clearAllMocks()
clearCommandMarkerCacheForTests()
})
const render = (agent: 'claude' | 'openclaude' | 'codex') =>
renderHook(() =>
useNativeChatSlashCommandDispatched({
agent,
commandMarkerScope: { ...SCOPE, agent },
setCommandMarkers,
onSwitchToTerminal
})
)
it.each(['claude', 'openclaude', 'codex'] as const)(
'reveals the terminal when %s dispatches /resume',
(agent) => {
// STA-4617: the agent answers it with its own picker, drawn in the TUI the
// chat view is covering — staying put makes the command look inert.
const hook = render(agent)
act(() => hook.result.current('/resume'))
expect(onSwitchToTerminal).toHaveBeenCalledTimes(1)
}
)
it('leaves the chat view in place for commands the agent answers inline', () => {
const hook = render('claude')
act(() => {
hook.result.current('/clear')
hook.result.current('/compact')
hook.result.current('resume the refactor')
})
expect(onSwitchToTerminal).not.toHaveBeenCalled()
})
it('still records the local marker for a revealing command', () => {
// The reveal is additive: `/resume` is a control action with no transcript
// turn, so it needs its `Ran /resume` line exactly like `/clear` does.
const hook = render('claude')
act(() => hook.result.current('/resume'))
expect(setCommandMarkers).toHaveBeenCalledTimes(1)
expect(setCommandMarkers.mock.calls[0]![0]).toEqual([
expect.objectContaining({ command: '/resume' })
])
})
})
@@ -0,0 +1,40 @@
import { useCallback, type Dispatch, type SetStateAction } from 'react'
import type { AgentType } from '../../../../shared/agent-status-types'
import { getVerifiedNativeChatCommands } from '../../../../shared/native-chat-agent-profiles'
import { nativeChatSlashCommandOpensAgentPicker } from '../../../../shared/native-chat-slash-commands'
import {
appendCommandMarkerCache,
type NativeChatCommandMarker,
type NativeChatCommandMarkerScope
} from './native-chat-command-marker'
/**
* Everything the chat view owes a verified slash command that just went out to
* the TUI: the local `Ran /x` marker (slash commands are never transcript
* turns), plus revealing the terminal for the ones the agent answers with its
* own picker.
*
* The reveal belongs to the view, not the composer: only the view knows the
* chat surface is currently covering the TUI the picker draws in. Without it a
* `/resume` looks like a command that did nothing at all (STA-4617). Session
* options already reveal `/model` this way; this is the half that has no
* session-option catalog behind it, so it also covers agents that have slash
* commands but no options catalog.
*/
export function useNativeChatSlashCommandDispatched(args: {
agent: AgentType
commandMarkerScope: NativeChatCommandMarkerScope
setCommandMarkers: Dispatch<SetStateAction<NativeChatCommandMarker[]>>
onSwitchToTerminal?: () => void
}): (command: string) => void {
const { agent, commandMarkerScope, setCommandMarkers, onSwitchToTerminal } = args
return useCallback(
(command: string) => {
setCommandMarkers(appendCommandMarkerCache(commandMarkerScope, command))
if (nativeChatSlashCommandOpensAgentPicker(command, getVerifiedNativeChatCommands(agent))) {
onSwitchToTerminal?.()
}
},
[agent, commandMarkerScope, onSwitchToTerminal, setCommandMarkers]
)
}