From 71a68c5528d4d85e34de4bd11f499e85096f541c Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 26 Aug 2026 15:18:35 -0700 Subject: [PATCH] fix(native-chat): reveal the picker on desktop too, and harden the mobile reveal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../use-mobile-native-chat-controller.test.ts | 45 ++++++++++--- .../use-mobile-native-chat-controller.ts | 14 +++-- ...se-mobile-native-chat-message-send.test.ts | 9 +++ .../use-mobile-native-chat-message-send.ts | 18 ++++-- .../use-mobile-session-view-mode.test.ts | 26 ++++++++ .../session/use-mobile-session-view-mode.ts | 32 ++++++++-- .../native-chat/NativeChatComposer.test.tsx | 35 ++++++++++- .../components/native-chat/NativeChatView.tsx | 14 ++--- ...ive-chat-slash-command-dispatched.test.tsx | 63 +++++++++++++++++++ ...se-native-chat-slash-command-dispatched.ts | 40 ++++++++++++ 10 files changed, 267 insertions(+), 29 deletions(-) create mode 100644 src/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.test.tsx create mode 100644 src/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.ts diff --git a/mobile/src/session/use-mobile-native-chat-controller.test.ts b/mobile/src/session/use-mobile-native-chat-controller.test.ts index 64f1ca2aab4..15d2f998059 100644 --- a/mobile/src/session/use-mobile-native-chat-controller.test.ts +++ b/mobile/src/session/use-mobile-native-chat-controller.test.ts @@ -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[] = [] @@ -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') }) }) diff --git a/mobile/src/session/use-mobile-native-chat-controller.ts b/mobile/src/session/use-mobile-native-chat-controller.ts index f8e5b146aa5..a9f920df3c5 100644 --- a/mobile/src/session/use-mobile-native-chat-controller.ts +++ b/mobile/src/session/use-mobile-native-chat-controller.ts @@ -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, diff --git a/mobile/src/session/use-mobile-native-chat-message-send.test.ts b/mobile/src/session/use-mobile-native-chat-message-send.test.ts index ca09458e3a2..1c6a5668e94 100644 --- a/mobile/src/session/use-mobile-native-chat-message-send.test.ts +++ b/mobile/src/session/use-mobile-native-chat-message-send.test.ts @@ -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 () => { diff --git a/mobile/src/session/use-mobile-native-chat-message-send.ts b/mobile/src/session/use-mobile-native-chat-message-send.ts index a542ba7ad34..ebb69f701ed 100644 --- a/mobile/src/session/use-mobile-native-chat-message-send.ts +++ b/mobile/src/session/use-mobile-native-chat-message-send.ts @@ -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?.() } } diff --git a/mobile/src/session/use-mobile-session-view-mode.test.ts b/mobile/src/session/use-mobile-session-view-mode.test.ts index 0eda383487c..b37c1bea27e 100644 --- a/mobile/src/session/use-mobile-session-view-mode.test.ts +++ b/mobile/src/session/use-mobile-session-view-mode.test.ts @@ -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>() vi.mocked(readSessionViewOverridesPreference).mockImplementation((hostId) => diff --git a/mobile/src/session/use-mobile-session-view-mode.ts b/mobile/src/session/use-mobile-session-view-mode.ts index ef1888b1332..b15e06bcedc 100644 --- a/mobile/src/session/use-mobile-session-view-mode.ts +++ b/mobile/src/session/use-mobile-session-view-mode.ts @@ -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 } } diff --git a/src/renderer/src/components/native-chat/NativeChatComposer.test.tsx b/src/renderer/src/components/native-chat/NativeChatComposer.test.tsx index e7a6269f533..2a33aba9cb1 100644 --- a/src/renderer/src/components/native-chat/NativeChatComposer.test.tsx +++ b/src/renderer/src/components/native-chat/NativeChatComposer.test.tsx @@ -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()), - 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( + + ) + + 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( { - setCommandMarkers(appendCommandMarkerCache(commandMarkerScope, command)) - }, - [commandMarkerScope] - ) + const onSlashCommand = useNativeChatSlashCommandDispatched({ + agent, + commandMarkerScope, + setCommandMarkers, + onSwitchToTerminal + }) const launchPromptMessage = useMemo( () => launchPromptAsMessage(paneLaunchPrompt, session.messages), diff --git a/src/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.test.tsx b/src/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.test.tsx new file mode 100644 index 00000000000..838f382bd4e --- /dev/null +++ b/src/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.test.tsx @@ -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' }) + ]) + }) +}) diff --git a/src/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.ts b/src/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.ts new file mode 100644 index 00000000000..7141300ad6b --- /dev/null +++ b/src/renderer/src/components/native-chat/use-native-chat-slash-command-dispatched.ts @@ -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> + 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] + ) +}