From 4df6a15e94bfe239ca7d8746dd07ea556195f5e9 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 4 Aug 2026 03:23:06 -0700 Subject: [PATCH] fix(mobile): close four session-option picker defects found in review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit D1 — a picker apply could interleave with a composer send. The composer already blocks a text send while an apply is dispatching, but not the reverse: the host spaces a send's body and its Enter ~500ms apart, so an apply tapped inside that window was submitted as part of the user's prompt, and the pill then claimed a model change that never ran as a command. The pickers render inside the composer, so they now take its in-flight state directly — the same guard, mirrored. D2 — an option was filed under the wrong model. `setTrackedSessionOption` resolves the owning model when it commits, not when the command was built, and the report effect mutates the same record off-queue. A report landing mid-dispatch therefore recorded `/effort low` against the model it switched TO. Ports desktop's supersession guard, which skips the commit when the baseline moved. D3 — a command template's prefix also matches prose that starts with it, so "/model is a weird word" tracked that prose as the current model, rendered it as the pill label, and matched no catalog model, dropping every per-model option. Parsed values are now canonicalized against the catalog; a typed value containing whitespace is treated as a prompt rather than a command. Perf — `/` on a Codex tab returned all 45 commands into a non-virtualized ScrollView showing ~5, re-reconciled on every streaming tick above the transcript. Capped at 12. Also splits the row primitives out of MobileNativeChatSessionOptionPickers.tsx, which the D1 guard pushed to 402 effective lines against a 400 cap. --- .../session/MobileNativeChatComposer.test.ts | 68 +++++ .../src/session/MobileNativeChatComposer.tsx | 12 +- .../MobileNativeChatSessionOptionPickers.tsx | 253 +----------------- .../MobileNativeChatSessionOptionRows.tsx | 252 +++++++++++++++++ ...mobile-native-chat-session-options.test.ts | 22 ++ .../use-mobile-native-chat-session-options.ts | 18 ++ ...ative-chat-session-option-commands.test.ts | 55 ++++ .../native-chat-session-option-commands.ts | 28 +- 8 files changed, 464 insertions(+), 244 deletions(-) create mode 100644 mobile/src/session/MobileNativeChatSessionOptionRows.tsx diff --git a/mobile/src/session/MobileNativeChatComposer.test.ts b/mobile/src/session/MobileNativeChatComposer.test.ts index 897d4dc945d..4ed2ffb60dc 100644 --- a/mobile/src/session/MobileNativeChatComposer.test.ts +++ b/mobile/src/session/MobileNativeChatComposer.test.ts @@ -116,6 +116,74 @@ describe('MobileNativeChatComposer', () => { expect(onSend).toHaveBeenCalledWith(' /clear is prose') }) + it('locks the option pickers while a composer send is in flight', async () => { + // The reverse of the test below. The host spaces a send's body and its Enter + // ~500ms apart, so an apply tapped inside that window would be submitted as + // part of the user's prompt instead of running as its own command. + let releaseSend: ((accepted: boolean) => void) | undefined + const onSend = vi.fn( + () => + new Promise((resolve) => { + releaseSend = resolve + }) + ) + const controller = { + snapshot: [ + { + id: 'model', + label: 'Model', + category: 'model' as const, + kind: { + type: 'select' as const, + choices: [ + { value: 'sonnet', label: 'Sonnet 5' }, + { value: 'opus', label: 'Opus 4.8' } + ] + }, + valueSource: 'unknown' as const, + settable: true + } + ], + pendingId: null, + setOption: vi.fn(), + invokeAction: vi.fn(), + recordCommand: vi.fn() + } + const restore = suppressRendererWarning() + try { + await act(async () => { + renderer = create( + createElement(MobileNativeChatComposer, { + value: 'run the tests', + onChangeText: vi.fn(), + onSend, + sessionOptions: { isWorking: false, controller } + }) + ) + }) + const modelPill = (): { props: { accessibilityState: { disabled: boolean } } } => + renderer!.root.find( + (node) => node.type === 'Pressable' && node.props.accessibilityLabel === 'Model, Model' + ) as { props: { accessibilityState: { disabled: boolean } } } + expect(modelPill().props.accessibilityState).toMatchObject({ disabled: false }) + // Start the send but don't await it — it stays in flight on purpose. + let pressed!: Promise + await act(async () => { + pressed = sendButton().props.onPress() + await Promise.resolve() + }) + expect(onSend).toHaveBeenCalled() + expect(modelPill().props.accessibilityState).toMatchObject({ disabled: true }) + await act(async () => { + releaseSend?.(true) + await pressed + }) + expect(modelPill().props.accessibilityState).toMatchObject({ disabled: false }) + } finally { + restore() + } + }) + it('blocks composer submission while a session-option command is pending', async () => { const onSend = vi.fn().mockResolvedValue(true) const restore = suppressRendererWarning() diff --git a/mobile/src/session/MobileNativeChatComposer.tsx b/mobile/src/session/MobileNativeChatComposer.tsx index 8097ab45c70..ea8eb6e1d4b 100644 --- a/mobile/src/session/MobileNativeChatComposer.tsx +++ b/mobile/src/session/MobileNativeChatComposer.tsx @@ -106,7 +106,10 @@ export function MobileNativeChatComposer({ } if (trigger.kind === 'slash') { const commands = agent ? getVerifiedNativeChatCommands(agent) : [] - return rankSlashCommandSuggestions(commands, trigger.query).map((command) => ({ + // Why: Codex's catalog is 45 commands and this list is a plain ScrollView + // (~5 rows visible), so an uncapped `/` would mount every row and + // re-reconcile them on each streaming tick right above the transcript. + return rankSlashCommandSuggestions(commands, trigger.query, 12).map((command) => ({ kind: 'command' as const, command })) @@ -192,7 +195,12 @@ export function MobileNativeChatComposer({ ))} ) : null} - {sessionOptions ? : null} + {sessionOptions ? ( + + ) : null} {onAttachImage ? ( void -}): React.JSX.Element { - return ( - [styles.pill, pressed && !disabled && styles.pressed]} - onPress={onPress} - disabled={disabled} - hitSlop={6} - > - - {label} - - - - ) -} - -function ChoiceRow({ - label, - description, - selected, - disabled, - onPress -}: { - label: string - description?: string - selected: boolean - disabled: boolean - onPress: () => void -}): React.JSX.Element { - return ( - - - {selected ? : null} - - - {label} - {description ? ( - - {description} - - ) : null} - - - ) -} - -function ActionRow({ - label, - disabled, - onPress -}: { - label: string - disabled: boolean - onPress: () => void -}): React.JSX.Element { - return ( - - - {label} - - - ) -} - -function DescriptorRows({ - descriptor, - disabled, - onSetOption, - onInvokeAction -}: { - descriptor: SessionOptionDescriptor - disabled: boolean - onSetOption: (value: SessionOptionValue) => void - onInvokeAction: () => void -}): React.JSX.Element { - const locked = disabled || !descriptor.settable - // Why: flip-only without a baseline is an action — never claim On/Off. - if (descriptor.action?.type === 'toggle-command') { - return ( - - ) - } - // Why: agent-picker opens the TUI; it is not a set of radio choices. - if (descriptor.action?.type === 'agent-picker') { - return - } - // Unknown booleans leave both radios unselected instead of inventing truth. - if (descriptor.kind.type === 'boolean') { - const current = descriptor.kind.currentValue - return ( - <> - {current === undefined ? ( - Current value unknown — pick On or Off - ) : null} - onSetOption(true)} - /> - onSetOption(false)} - /> - - ) - } - const { currentValue, choices } = descriptor.kind - return ( - <> - {choices.map((choice) => ( - onSetOption(choice.value)} - /> - ))} - - ) + /** A composer send owns the TUI input line until it settles. The host spaces a + * send's body and its Enter ~500ms apart, so an apply dispatched inside that + * window would be submitted as part of the user's prompt. The composer blocks + * the reverse direction on `pendingId`; this is the same guard mirrored. */ + sendInFlight?: boolean } /** Model + session-option pills for the chat composer, opening an inline card * of choices (the Ask-card pattern) instead of desktop's dropdown menus. */ export function MobileNativeChatSessionOptionPickers({ controller, - isWorking + isWorking, + sendInFlight = false }: MobileNativeChatSessionOptionPickersProps): React.JSX.Element | null { const [open, setOpen] = useState<'model' | 'options' | null>(null) const { snapshot, pendingId } = controller @@ -194,7 +42,7 @@ export function MobileNativeChatSessionOptionPickers({ if (!model) { return null } - const disabled = isWorking || pendingId !== null + const disabled = isWorking || pendingId !== null || sendInFlight const sheetDescriptors = open === 'model' ? [model] : options const sheetTitle = open === 'model' ? 'Model' : mobileOptionsPillTitle(options) const dispatched = sheetDescriptors.some((descriptor) => descriptor.valueSource === 'dispatched') @@ -249,9 +97,9 @@ export function MobileNativeChatSessionOptionPickers({ {dispatched ? ( - Sent to the agent — not confirmed + Sent to the agent — not confirmed ) : null} - {reason ? {reason} : null} + {reason ? {reason} : null} {sheetDescriptors.map((descriptor, index) => ( @@ -300,31 +148,6 @@ const styles = StyleSheet.create({ paddingTop: spacing.xs, backgroundColor: colors.bgPanel }, - pill: { - flexDirection: 'row', - alignItems: 'center', - gap: 4, - maxWidth: 180, - minHeight: 28, - paddingHorizontal: spacing.sm, - paddingVertical: 4, - borderRadius: radii.button, - borderWidth: StyleSheet.hairlineWidth, - borderColor: colors.borderSubtle, - backgroundColor: colors.bgRaised - }, - pillText: { - color: colors.textSecondary, - fontSize: typography.metaSize, - fontWeight: '600', - flexShrink: 1 - }, - pillTextDisabled: { - color: colors.textMuted - }, - pressed: { - opacity: 0.7 - }, sheet: { maxHeight: 320, backgroundColor: colors.bgPanel, @@ -363,55 +186,5 @@ const styles = StyleSheet.create({ }, sectionLabelSpaced: { marginTop: spacing.sm - }, - caption: { - color: colors.textMuted, - fontSize: typography.metaSize, - paddingHorizontal: spacing.md, - paddingBottom: spacing.xs - }, - row: { - flexDirection: 'row', - gap: spacing.sm, - padding: spacing.sm, - minHeight: 44, - alignItems: 'center', - borderRadius: radii.card, - backgroundColor: colors.bgRaised, - borderWidth: 1, - borderColor: colors.borderSubtle, - marginBottom: spacing.xs - }, - rowSelected: { - borderColor: colors.statusGreen - }, - rowDisabled: { - opacity: 0.5 - }, - radio: { - width: 18, - height: 18, - borderRadius: 9, - borderWidth: 1.5, - borderColor: colors.textMuted, - alignItems: 'center', - justifyContent: 'center' - }, - radioOn: { - backgroundColor: colors.statusGreen, - borderColor: colors.statusGreen - }, - rowBody: { - flex: 1, - gap: 2 - }, - rowLabel: { - color: colors.textPrimary, - fontSize: typography.bodySize, - fontWeight: '600' - }, - rowDescription: { - color: colors.textSecondary, - fontSize: typography.metaSize } }) diff --git a/mobile/src/session/MobileNativeChatSessionOptionRows.tsx b/mobile/src/session/MobileNativeChatSessionOptionRows.tsx new file mode 100644 index 00000000000..f7d03a9db2a --- /dev/null +++ b/mobile/src/session/MobileNativeChatSessionOptionRows.tsx @@ -0,0 +1,252 @@ +// The pill and choice-row primitives the session-option card is built from, kept +// beside it so the card file stays about layout and apply wiring. + +import { Pressable, StyleSheet, Text, View } from 'react-native' +import { Check, ChevronDown } from 'lucide-react-native' +import { colors, radii, spacing, typography } from '../theme/mobile-theme' +import type { + SessionOptionDescriptor, + SessionOptionValue +} from '../../../src/shared/native-chat-session-options' + +/** Muted one-liner above a group — dispatch state, or why a row is locked. */ +export function SessionOptionCaption({ children }: { children: string }): React.JSX.Element { + return {children} +} + +export function Pill({ + label, + accessibleName, + disabled, + onPress +}: { + label: string + accessibleName: string + disabled: boolean + onPress: () => void +}): React.JSX.Element { + return ( + [styles.pill, pressed && !disabled && styles.pressed]} + onPress={onPress} + disabled={disabled} + hitSlop={6} + > + + {label} + + + + ) +} + +function ChoiceRow({ + label, + description, + selected, + disabled, + onPress +}: { + label: string + description?: string + selected: boolean + disabled: boolean + onPress: () => void +}): React.JSX.Element { + return ( + + + {selected ? : null} + + + {label} + {description ? ( + + {description} + + ) : null} + + + ) +} + +function ActionRow({ + label, + disabled, + onPress +}: { + label: string + disabled: boolean + onPress: () => void +}): React.JSX.Element { + return ( + + + {label} + + + ) +} + +export function DescriptorRows({ + descriptor, + disabled, + onSetOption, + onInvokeAction +}: { + descriptor: SessionOptionDescriptor + disabled: boolean + onSetOption: (value: SessionOptionValue) => void + onInvokeAction: () => void +}): React.JSX.Element { + const locked = disabled || !descriptor.settable + // Why: flip-only without a baseline is an action — never claim On/Off. + if (descriptor.action?.type === 'toggle-command') { + return ( + + ) + } + // Why: agent-picker opens the TUI; it is not a set of radio choices. + if (descriptor.action?.type === 'agent-picker') { + return + } + // Unknown booleans leave both radios unselected instead of inventing truth. + if (descriptor.kind.type === 'boolean') { + const current = descriptor.kind.currentValue + return ( + <> + {current === undefined ? ( + Current value unknown — pick On or Off + ) : null} + onSetOption(true)} + /> + onSetOption(false)} + /> + + ) + } + const { currentValue, choices } = descriptor.kind + return ( + <> + {choices.map((choice) => ( + onSetOption(choice.value)} + /> + ))} + + ) +} + +const styles = StyleSheet.create({ + pill: { + flexDirection: 'row', + alignItems: 'center', + gap: 4, + maxWidth: 180, + minHeight: 28, + paddingHorizontal: spacing.sm, + paddingVertical: 4, + borderRadius: radii.button, + borderWidth: StyleSheet.hairlineWidth, + borderColor: colors.borderSubtle, + backgroundColor: colors.bgRaised + }, + pillText: { + color: colors.textSecondary, + fontSize: typography.metaSize, + fontWeight: '600', + flexShrink: 1 + }, + pillTextDisabled: { + color: colors.textMuted + }, + pressed: { + opacity: 0.7 + }, + caption: { + color: colors.textMuted, + fontSize: typography.metaSize, + paddingHorizontal: spacing.md, + paddingBottom: spacing.xs + }, + row: { + flexDirection: 'row', + gap: spacing.sm, + padding: spacing.sm, + minHeight: 44, + alignItems: 'center', + borderRadius: radii.card, + backgroundColor: colors.bgRaised, + borderWidth: 1, + borderColor: colors.borderSubtle, + marginBottom: spacing.xs + }, + rowSelected: { + borderColor: colors.statusGreen + }, + rowDisabled: { + opacity: 0.5 + }, + radio: { + width: 18, + height: 18, + borderRadius: 9, + borderWidth: 1.5, + borderColor: colors.textMuted, + alignItems: 'center', + justifyContent: 'center' + }, + radioOn: { + backgroundColor: colors.statusGreen, + borderColor: colors.statusGreen + }, + rowBody: { + flex: 1, + gap: 2 + }, + rowLabel: { + color: colors.textPrimary, + fontSize: typography.bodySize, + fontWeight: '600' + }, + rowDescription: { + color: colors.textSecondary, + fontSize: typography.metaSize + } +}) diff --git a/mobile/src/session/use-mobile-native-chat-session-options.test.ts b/mobile/src/session/use-mobile-native-chat-session-options.test.ts index f8e811ca1e6..7cf5b2a683b 100644 --- a/mobile/src/session/use-mobile-native-chat-session-options.test.ts +++ b/mobile/src/session/use-mobile-native-chat-session-options.test.ts @@ -135,6 +135,28 @@ describe('useMobileNativeChatSessionOptions', () => { expect(effort!.kind).toMatchObject({ currentValue: 'low' }) }) + it('does not file an option under a model that changed mid-dispatch', async () => { + const resolvers: ((outcome: MobileNativeChatSendOutcome) => void)[] = [] + dispatchCommand.mockImplementation( + () => new Promise((resolve) => resolvers.push(resolve)) + ) + mount({ reportedModel: 'claude-sonnet-5' }) + let applied!: Promise + await act(async () => { + applied = api!.setOption('effort', 'low') + await Promise.resolve() + }) + // A report lands while `/effort low` is still in flight and moves the model. + update({ reportedModel: 'claude-opus-5' }) + await act(async () => { + resolvers[0]!('accepted') + await applied + }) + // The effort must not be recorded against Opus — it was sent for Sonnet. + const effort = api!.snapshot.find((descriptor) => descriptor.id === 'effort') + expect(effort?.kind).not.toMatchObject({ currentValue: 'low' }) + }) + it('does not revive a stale session-start report over a newer local pick', async () => { mount({ reportedModel: 'claude-sonnet-5' }) await act(async () => { diff --git a/mobile/src/session/use-mobile-native-chat-session-options.ts b/mobile/src/session/use-mobile-native-chat-session-options.ts index 7bfcd0467b0..ef9cdfd2640 100644 --- a/mobile/src/session/use-mobile-native-chat-session-options.ts +++ b/mobile/src/session/use-mobile-native-chat-session-options.ts @@ -222,10 +222,28 @@ export function useMobileNativeChatSessionOptions(args: { if (!command) { return false } + // Baseline for detecting a hook report or typed command that lands while + // the dispatch is in flight — the record is shared mutable state and the + // report effect is not on this queue. + const trackedBeforeDispatch = + id === 'model' ? undefined : getTrackedSessionOption(record, previousModelId, id) const outcome = await dispatchCommand(command) if (outcome === 'rejected') { return false } + if (id !== 'model') { + // Why (desktop parity): `setTrackedSessionOption` resolves the owning + // model when it commits, not when the command was built. If the model + // moved during the dispatch, committing now would file this value under + // the NEW model — claiming an effort the agent was never asked for. + const modelStill = typeof record.model?.value === 'string' ? record.model.value : null + if ( + modelStill !== previousModelId || + getTrackedSessionOption(record, previousModelId, id) !== trackedBeforeDispatch + ) { + return true + } + } if (id === 'model') { if (typeof value === 'string' && previousModelId !== value) { // Why: switching models can reset effort/toggles for the destination model. diff --git a/src/shared/native-chat-session-option-commands.test.ts b/src/shared/native-chat-session-option-commands.test.ts index 7c33ecf0835..6be6e2525ce 100644 --- a/src/shared/native-chat-session-option-commands.test.ts +++ b/src/shared/native-chat-session-option-commands.test.ts @@ -148,6 +148,61 @@ describe('recordNativeChatSessionOptionCommand', () => { expect(record.valuesByModel.opus?.fastMode).toBeUndefined() }) + it('rejects prose that merely starts with a command template', () => { + // The `/model ` prefix matches this, so an unvalidated parse tracked + // "is a weird word" as the current model — which then rendered as the pill + // label and, matching no catalog model, dropped every per-model option. + const record = claudeRecord('sonnet') + expect( + recordNativeChatSessionOptionCommand({ + catalog: CLAUDE_SESSION_OPTION_CATALOG, + models: CLAUDE_SESSION_OPTION_CATALOG.models, + record, + command: '/model is a weird word' + }) + ).toEqual({ changed: false, opensAgentPicker: false }) + expect(record.model).toEqual({ value: 'sonnet', source: 'dispatched' }) + }) + + it('rejects an option value outside the catalog choices', () => { + const record = claudeRecord('sonnet') + recordNativeChatSessionOptionCommand({ + catalog: CLAUDE_SESSION_OPTION_CATALOG, + models: CLAUDE_SESSION_OPTION_CATALOG.models, + record, + command: '/effort of will is required' + }) + expect(record.valuesByModel.sonnet?.effort).toBeUndefined() + }) + + it('tracks nothing for a multi-line paste whose first line looks like a command', () => { + const record = claudeRecord('opus') + recordNativeChatSessionOptionCommand({ + catalog: CLAUDE_SESSION_OPTION_CATALOG, + models: CLAUDE_SESSION_OPTION_CATALOG.models, + record, + command: '/model sonnet\nplease review this' + }) + expect(record.model).toEqual({ value: 'opus', source: 'dispatched' }) + }) + + it('still accepts an alias, a full provider id, and extra spacing', () => { + for (const [command, expected] of [ + ['/model opus', 'opus'], + ['/model claude-sonnet-5', 'sonnet'], + ['/model sonnet', 'sonnet'] + ] as const) { + const record = claudeRecord() + recordNativeChatSessionOptionCommand({ + catalog: CLAUDE_SESSION_OPTION_CATALOG, + models: CLAUDE_SESSION_OPTION_CATALOG.models, + record, + command + }) + expect(record.model).toEqual({ value: expected, source: 'dispatched' }) + } + }) + it('ignores unrelated commands', () => { const record = claudeRecord('sonnet') expect( diff --git a/src/shared/native-chat-session-option-commands.ts b/src/shared/native-chat-session-option-commands.ts index a6eb28fa47b..9a6598cf462 100644 --- a/src/shared/native-chat-session-option-commands.ts +++ b/src/shared/native-chat-session-option-commands.ts @@ -10,6 +10,7 @@ import { clearTrackedSessionOption, flattenNativeChatSessionOptionRecord, isFlipOnlyMidSession, + matchNativeChatCatalogModelId, type NativeChatSessionOptionRecord } from './native-chat-session-option-state' @@ -84,9 +85,14 @@ function recordCommandApply(args: { optionId: string midSession: CatalogMidSessionApply | undefined command: string + /** Canonicalize a parsed value, or reject it. A template's prefix also matches + * prose that merely starts with it (`/model` parses `is a weird word` out of + * "/model is a weird word"), and tracking that renders raw prose as the + * current value — and, for the model, drops every option the model owns. */ + canonicalize: (value: string) => string | null persist?: PersistSessionOption }): boolean { - const { record, optionId, midSession, command, persist } = args + const { record, optionId, midSession, command, canonicalize, persist } = args if (!midSession || midSession.kind === 'unsupported') { return false } @@ -102,7 +108,11 @@ function recordCommandApply(args: { if (midSession.kind !== 'command') { return false } - const value = parseBuiltSessionOptionCommand(midSession.build, command) + const parsed = parseBuiltSessionOptionCommand(midSession.build, command) + if (!parsed) { + return false + } + const value = canonicalize(parsed) if (!value) { return false } @@ -143,6 +153,15 @@ export function recordNativeChatSessionOptionCommand(args: { optionId: 'model', midSession: catalog.modelApply.midSession, command, + // A typed model can be an alias or a full provider id, so reuse the same + // matcher the hook reports go through rather than demanding a catalog id. + // Whitespace means this was never a bare command — a multi-line paste whose + // first line happens to read `/model sonnet` is a prompt as far as we can + // tell, so track nothing rather than guess. Model ids never contain spaces. + canonicalize: (value) => + /\s/.test(value) + ? null + : matchNativeChatCatalogModelId({ ...catalog, models: [...models] }, value), persist }) const modelId = typeof record.model?.value === 'string' ? record.model.value : null @@ -156,6 +175,11 @@ export function recordNativeChatSessionOptionCommand(args: { optionId: option.id, midSession: option.apply.midSession, command, + canonicalize: (value) => + option.kind.type === 'select' && + !option.kind.choices.some((choice) => choice.value === value) + ? null + : value, persist }) || changed }