diff --git a/mobile/src/session/QuickCommandsSheet.test.ts b/mobile/src/session/QuickCommandsSheet.test.ts index bc76b2ac244..bc03965365b 100644 --- a/mobile/src/session/QuickCommandsSheet.test.ts +++ b/mobile/src/session/QuickCommandsSheet.test.ts @@ -1,10 +1,12 @@ -import { createElement, type ReactNode } from 'react' +import { createElement, Fragment, startTransition, Suspense, type ReactNode } from 'react' import { act, create, type ReactTestRenderer } from 'react-test-renderer' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { TerminalQuickCommand } from '../../../src/shared/terminal-quick-command-types' import type { RpcClient } from '../transport/rpc-client' import { MAX_QUICK_COMMANDS } from '../terminal/quick-commands' import { QuickCommandsSheet } from './QuickCommandsSheet' +import { createStableLogicalRpcClient } from '../transport/stable-logical-rpc-client' +import { FakeSession } from '../transport/mobile-endpoint-supervisor-test-fakes' const mocks = vi.hoisted(() => ({ alert: vi.fn(), @@ -13,6 +15,15 @@ const mocks = vi.hoisted(() => ({ })) const quickCommandEditorForm = 'QuickCommandEditorForm' const quickCommandsList = 'QuickCommandsList' +const suspended = new Promise(() => {}) +const suspendedRender = vi.fn() +function Suspend({ active }: { active: boolean }) { + if (active) { + suspendedRender() + throw suspended + } + return null +} const command: TerminalQuickCommand = { id: 'command', label: 'Test', @@ -135,6 +146,202 @@ describe('QuickCommandsSheet', () => { }) }) + it('keeps save ownership when a repository transition suspends before commit', async () => { + const save = deferred() + mocks.persist.mockReturnValue(save.promise) + const onClose = vi.fn() + const onLaunch = () => true + const render = (suspend = false) => + createElement( + Suspense, + { fallback: null }, + createElement( + Fragment, + null, + createElement(QuickCommandsSheet, { + visible: true, + client: null, + repoId: suspend ? 'repo-2' : 'repo-1', + repoName: 'Repo', + agentPromptMaxLength: null, + onClose, + onLaunch + }), + createElement(Suspend, { active: suspend }) + ) + ) + act(() => { + renderer = create(render()) + }) + act(() => renderer!.root.findByType(quickCommandsList).props.onAdd()) + act(() => + renderer!.root.findByType(quickCommandEditorForm).props.onChange({ + label: 'Test', + command: 'pnpm test' + }) + ) + act(() => renderer!.root.findByType(quickCommandEditorForm).props.onSave()) + suspendedRender.mockClear() + act(() => { + startTransition(() => renderer?.update(render(true))) + }) + expect(suspendedRender).toHaveBeenCalled() + await act(async () => { + save.resolve(true) + await save.promise + }) + expect(renderer!.root.findAllByType(quickCommandEditorForm)).toHaveLength(0) + expect(renderer!.root.findAllByType(quickCommandsList)).toHaveLength(1) + }) + + it.each(['new editor', 'newer edits'])( + 'preserves %s when an older save completes', + async (change) => { + const save = deferred() + mocks.persist.mockReturnValue(save.promise) + await act(async () => { + renderer = create( + createElement(QuickCommandsSheet, { + visible: true, + onClose: vi.fn(), + client: null, + repoId: 'repo-1', + repoName: 'Repo', + agentPromptMaxLength: null, + onLaunch: () => true + }) + ) + }) + act(() => renderer!.root.findByType(quickCommandsList).props.onAdd()) + act(() => + renderer!.root + .findByType(quickCommandEditorForm) + .props.onChange({ label: 'First', command: 'first' }) + ) + act(() => renderer!.root.findByType(quickCommandEditorForm).props.onSave()) + if (change === 'new editor') { + act(() => renderer!.root.findByType(quickCommandEditorForm).props.onCancel()) + act(() => renderer!.root.findByType(quickCommandsList).props.onAdd()) + } + act(() => + renderer!.root + .findByType(quickCommandEditorForm) + .props.onChange({ label: 'Second', command: 'second' }) + ) + await act(async () => { + save.resolve(true) + await save.promise + }) + expect(renderer!.root.findAllByType(quickCommandEditorForm)).toHaveLength(1) + expect(renderer!.root.findByType(quickCommandEditorForm).props.draft.label).toBe('Second') + if (change === 'newer edits') { + act(() => renderer!.root.findByType(quickCommandEditorForm).props.onSave()) + expect(mocks.persist.mock.calls[1]?.[0].command).toMatchObject({ + id: mocks.persist.mock.calls[0]?.[0].command.id, + label: 'Second', + command: 'second' + }) + } + } + ) + + it.each(['client', 'repo'])('isolates pending saves across a changed %s', async (context) => { + const first = deferred() + const second = deferred() + mocks.persist.mockReturnValueOnce(first.promise).mockReturnValueOnce(second.promise) + const props = { + visible: true, + onClose: vi.fn(), + client: null, + repoId: 'repo-1', + repoName: 'Repo', + agentPromptMaxLength: null, + onLaunch: () => true + } + act(() => { + renderer = create(createElement(QuickCommandsSheet, props)) + }) + act(() => renderer!.root.findByType(quickCommandsList).props.onAdd()) + act(() => + renderer!.root + .findByType(quickCommandEditorForm) + .props.onChange({ label: 'First', command: 'first' }) + ) + act(() => renderer!.root.findByType(quickCommandEditorForm).props.onSave()) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: The settings hook is mocked; this client is used only as an identity by the sheet. + const nextClient = {} as RpcClient + act(() => + renderer!.update( + createElement(QuickCommandsSheet, { + ...props, + client: context === 'client' ? nextClient : null, + repoId: context === 'repo' ? 'repo-2' : 'repo-1' + }) + ) + ) + act(() => renderer!.root.findByType(quickCommandsList).props.onAdd()) + act(() => + renderer!.root + .findByType(quickCommandEditorForm) + .props.onChange({ label: 'Second', command: 'second' }) + ) + act(() => renderer!.root.findByType(quickCommandEditorForm).props.onSave()) + expect(mocks.persist).toHaveBeenCalledTimes(2) + await act(async () => { + first.resolve(true) + await first.promise + }) + const editor = renderer!.root.findByType(quickCommandEditorForm) + expect(editor.props.draft.label).toBe('Second') + expect(editor.props.saving).toBe(true) + act(() => editor.props.onSave()) + expect(mocks.persist).toHaveBeenCalledTimes(2) + await act(async () => { + second.resolve(true) + await second.promise + }) + expect(renderer!.root.findAllByType(quickCommandEditorForm)).toHaveLength(0) + }) + + it('keeps newer edits and the pending save when the same computer changes connection paths', async () => { + const saved = deferred() + mocks.persist.mockReturnValue(saved.promise) + const client = createStableLogicalRpcClient(new FakeSession('connected'), 'relay') + const props = { + visible: true, + onClose: vi.fn(), + client, + repoId: 'repo-1', + repoName: 'Repo', + agentPromptMaxLength: null, + onLaunch: () => true + } + act(() => { + renderer = create(createElement(QuickCommandsSheet, props)) + }) + act(() => renderer!.root.findByType(quickCommandsList).props.onAdd()) + const editor = () => renderer!.root.findByType(quickCommandEditorForm) + act(() => editor().props.onChange({ label: 'First', command: 'first' })) + act(() => editor().props.onSave()) + act(() => editor().props.onChange({ label: 'Newer', command: 'newer' })) + await act(async () => { + await client.migrateTo(new FakeSession('connected'), 'lan') + renderer!.update(createElement(QuickCommandsSheet, props)) + }) + expect(editor().props.draft.label).toBe('Newer') + expect(editor().props.saving).toBe(true) + await act(async () => { + saved.resolve(true) + await saved.promise + }) + expect(editor().props.draft).toMatchObject({ + label: 'Newer', + id: mocks.persist.mock.calls[0]?.[0].command.id + }) + expect(editor().props.saving).toBe(false) + client.close() + }) + it('keeps creation closed when the host command limit is reached', async () => { mocks.commands = Array.from({ length: MAX_QUICK_COMMANDS }, (_, index) => ({ ...command, diff --git a/mobile/src/session/QuickCommandsSheet.tsx b/mobile/src/session/QuickCommandsSheet.tsx index 7b516b25d23..33bf538711c 100644 --- a/mobile/src/session/QuickCommandsSheet.tsx +++ b/mobile/src/session/QuickCommandsSheet.tsx @@ -1,4 +1,4 @@ -import { useMemo, useRef, useState } from 'react' +import { useLayoutEffect, useMemo, useRef, useState } from 'react' import { Alert, View, Text, Pressable, StyleSheet } from 'react-native' import { ChevronLeft } from 'lucide-react-native' import { colors, spacing } from '../theme/mobile-theme' @@ -50,17 +50,29 @@ export function QuickCommandsSheet({ const [query, setQuery] = useState('') const [draft, setDraft] = useState(null) const [saving, setSaving] = useState(false) - const savingRef = useRef(false) + const saveOwnerRef = useRef(null) + const draftRevisionRef = useRef(0) + const editorGenerationRef = useRef(0) - const [wasVisible, setWasVisible] = useState(visible) - if (visible !== wasVisible) { - setWasVisible(visible) - if (visible) { - setView('list') - setQuery('') - setDraft(null) + const previousSurface = useRef({ visible, client, repoId }) + useLayoutEffect(() => { + const contextChanged = + previousSurface.current.client !== client || previousSurface.current.repoId !== repoId + if (visible !== previousSurface.current.visible || contextChanged) { + draftRevisionRef.current += 1 + editorGenerationRef.current += 1 + previousSurface.current = { visible, client, repoId } + if (contextChanged) { + saveOwnerRef.current = null + setSaving(false) + } + if (visible || contextChanged) { + setView('list') + setQuery('') + setDraft(null) + } } - } + }, [visible, client, repoId]) // Why: prompt bodies can total megabytes. Lowercase them once per settings // update instead of allocating the same search text on every keystroke. @@ -89,6 +101,8 @@ export function QuickCommandsSheet({ if (!command && commands.length >= MAX_QUICK_COMMANDS) { return } + draftRevisionRef.current += 1 + editorGenerationRef.current += 1 setDraft( command ? quickCommandToDraft(command) @@ -123,7 +137,7 @@ export function QuickCommandsSheet({ } const handleSave = async () => { - if (!draft || savingRef.current) { + if (!draft || saveOwnerRef.current !== null) { return } const built = draftToQuickCommand(draft, agentPromptMaxLength) @@ -131,17 +145,24 @@ export function QuickCommandsSheet({ return } // Why: state cannot lock out a second tap until React commits the disabled UI. - savingRef.current = true + const saveOwner = Symbol() + saveOwnerRef.current = saveOwner setSaving(true) + const savedRevision = draftRevisionRef.current + const savedEditor = editorGenerationRef.current try { const ok = await persist({ type: 'upsert', command: built }) - if (ok) { + if (ok && draftRevisionRef.current === savedRevision) { setView('list') setDraft(null) + } else if (ok && editorGenerationRef.current === savedEditor) { + setDraft((current) => (current ? { ...current, id: built.id } : current)) } } finally { - savingRef.current = false - setSaving(false) + if (saveOwnerRef.current === saveOwner) { + saveOwnerRef.current = null + setSaving(false) + } } } @@ -162,7 +183,10 @@ export function QuickCommandsSheet({ ) : ( [styles.backButton, pressed && styles.pressed]} - onPress={() => setView(view === 'agent' ? 'editor' : 'list')} + onPress={() => { + draftRevisionRef.current += 1 + setView(view === 'agent' ? 'editor' : 'list') + }} accessibilityRole="button" accessibilityLabel="Back" > @@ -208,11 +232,16 @@ export function QuickCommandsSheet({ repoId={repoId} repoName={repoName} agentPromptMaxLength={agentPromptMaxLength} - onChange={(patch) => + onChange={(patch) => { + draftRevisionRef.current += 1 setDraft((current) => (current ? { ...current, ...patch } : current)) - } - onOpenAgentPicker={() => setView('agent')} + }} + onOpenAgentPicker={() => { + draftRevisionRef.current += 1 + setView('agent') + }} onCancel={() => { + draftRevisionRef.current += 1 setView('list') setDraft(null) }} @@ -224,6 +253,7 @@ export function QuickCommandsSheet({ { + draftRevisionRef.current += 1 setDraft((current) => (current ? { ...current, agent } : current)) setView('editor') }}