diff --git a/mobile/app/pair-confirm.tsx b/mobile/app/pair-confirm.tsx index d700ea83f42..7862154a1fb 100644 --- a/mobile/app/pair-confirm.tsx +++ b/mobile/app/pair-confirm.tsx @@ -143,7 +143,13 @@ export default function PairConfirmScreen() { return ( - + diff --git a/mobile/app/pair-scan.tsx b/mobile/app/pair-scan.tsx index 5b4c17b2501..91e5255917d 100644 --- a/mobile/app/pair-scan.tsx +++ b/mobile/app/pair-scan.tsx @@ -218,7 +218,13 @@ export default function PairScanScreen() { const canAskAgain = permission.canAskAgain !== false return ( - router.back()}> + router.back()} + hitSlop={8} + accessibilityRole="button" + accessibilityLabel="Back" + > @@ -261,7 +267,13 @@ export default function PairScanScreen() { return ( - router.back()}> + router.back()} + hitSlop={8} + accessibilityRole="button" + accessibilityLabel="Back" + > diff --git a/mobile/app/terminal-settings.tsx b/mobile/app/terminal-settings.tsx index 4720705bf91..af034d60beb 100644 --- a/mobile/app/terminal-settings.tsx +++ b/mobile/app/terminal-settings.tsx @@ -258,7 +258,13 @@ export default function TerminalSettingsScreen() { return ( - router.back()}> + router.back()} + hitSlop={8} + accessibilityRole="button" + accessibilityLabel="Back" + > Terminal diff --git a/mobile/src/accounts/MobileAccountsScreen.tsx b/mobile/src/accounts/MobileAccountsScreen.tsx index 3f2c74a865b..a09df242a6f 100644 --- a/mobile/src/accounts/MobileAccountsScreen.tsx +++ b/mobile/src/accounts/MobileAccountsScreen.tsx @@ -356,6 +356,9 @@ export function MobileAccountsScreen() { style={styles.iconButton} onPress={refresh} disabled={!client || refreshing || connState !== 'connected'} + hitSlop={8} + accessibilityRole="button" + accessibilityLabel="Refresh accounts" > {refreshing ? ( diff --git a/mobile/src/components/CustomKeyModal.tsx b/mobile/src/components/CustomKeyModal.tsx index 96b9b750d26..2a5c3c9b6aa 100644 --- a/mobile/src/components/CustomKeyModal.tsx +++ b/mobile/src/components/CustomKeyModal.tsx @@ -66,20 +66,49 @@ type Props = { onManageShortcuts?: () => void } -export async function loadCustomKeys(): Promise { - try { - const raw = await AsyncStorage.getItem(CUSTOM_ACCESSORY_KEYS_STORAGE_KEY) - return raw ? (JSON.parse(raw) as CustomKey[]) : [] - } catch { - return [] - } +// Throws on a failed read or parse, so a write never starts from an empty list it made up. +async function readStoredCustomKeys(): Promise { + const raw = await AsyncStorage.getItem(CUSTOM_ACCESSORY_KEYS_STORAGE_KEY) + return raw ? (JSON.parse(raw) as CustomKey[]) : [] } -export async function saveCustomKeys(keys: CustomKey[]): Promise { +export function loadCustomKeys(): Promise { + return readStoredCustomKeys().catch(() => []) +} + +function writeCustomKeys(keys: CustomKey[]): Promise { // Through the one write path, which notes the mirror on an accepted write and on nothing else // (ruling 35). There is no rollback here any more because there is nothing to undo: on the page // a value over the cap rejects, and a rejected write never reached the map. - await persistMirrored(CUSTOM_ACCESSORY_KEYS_STORAGE_KEY, JSON.stringify(keys)) + return persistMirrored(CUSTOM_ACCESSORY_KEYS_STORAGE_KEY, JSON.stringify(keys)) +} + +// Every write to the stored list takes a turn here, so a settings save, an add and a removal never +// land over one that started first, and an add or removal reads what the last write saved. +let customKeysWriteTurn: Promise = Promise.resolve() + +function inCustomKeysWriteTurn(write: () => Promise): Promise { + const run = customKeysWriteTurn.then(write) + customKeysWriteTurn = run.catch(() => {}) + return run +} + +export function saveCustomKeys(keys: CustomKey[]): Promise { + return inCustomKeysWriteTurn(() => writeCustomKeys(keys)) +} + +function updateStoredCustomKeys(change: (keys: CustomKey[]) => CustomKey[]): Promise { + return inCustomKeysWriteTurn(async () => { + const updated = change(await readStoredCustomKeys()) + await writeCustomKeys(updated) + return updated + }) +} + +// Like addKey: resolves with the stored list only once the store took it, so a refused write +// never drops a row from screen that the next load would bring back. +export function removeCustomKey(id: string): Promise { + return updateStoredCustomKeys((keys) => keys.filter((key) => key.id !== id)) } export function CustomKeyModal({ visible, onClose, onKeysChanged, onManageShortcuts }: Props) { @@ -107,9 +136,8 @@ export function CustomKeyModal({ visible, onClose, onKeysChanged, onManageShortc const addKey = useCallback( async (key: Omit) => { - const existing = await loadCustomKeys() const newKey: CustomKey = { ...key, id: `custom-${Date.now()}` } - const updated = [...existing, newKey] + let updated: CustomKey[] // Caught here because both callers are `void addKey(...)`, which leaves a rejection nowhere // to go. On the page this key is allowlisted and its write rejects for size — the contract // `page-async-storage` states, and the one ruling 33.6 extends to a key `init` could not @@ -117,7 +145,7 @@ export function CustomKeyModal({ visible, onClose, onKeysChanged, onManageShortc // handler, which reports a page fault and drops the generation for a key nobody could add. // Every other allowlisted writer in this closure already catches its own save. try { - await saveCustomKeys(updated) + updated = await updateStoredCustomKeys((keys) => [...keys, newKey]) } catch (error) { // Neither reported nor closed: a drawer that dismissed itself and announced the key would // put a row on the accessory bar that no store holds and the next load would not have. diff --git a/mobile/src/components/TerminalShortcutSettings.tsx b/mobile/src/components/TerminalShortcutSettings.tsx index 18d882aa3fb..55c6e066e9a 100644 --- a/mobile/src/components/TerminalShortcutSettings.tsx +++ b/mobile/src/components/TerminalShortcutSettings.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useMemo, useRef, useState } from 'react' +import { useCallback, useEffect, useMemo, useState } from 'react' import { AppState, View, @@ -14,6 +14,7 @@ import type Animated from 'react-native-reanimated' import type { AnimatedRef, SharedValue } from 'react-native-reanimated' import { CustomKeyModal, loadCustomKeys, saveCustomKeys, type CustomKey } from './CustomKeyModal' import { DragReorderList } from './DragReorderList' +import { useSerializedStoredState } from '../storage/use-serialized-stored-state' import { colors, radii, spacing, typography } from '../theme/mobile-theme' import { TERMINAL_ACCESSORY_KEYS, @@ -59,6 +60,11 @@ function ShortcutBarRow({ ) } +async function loadShortcutLayout(): Promise { + const { orderedBuiltInIds, visibleBuiltInIds } = await loadTerminalAccessoryLayout() + return { orderedBuiltInIds, visibleBuiltInIds } +} + type Props = { scrollRef: AnimatedRef scrollOffsetY: SharedValue @@ -72,67 +78,24 @@ export function TerminalShortcutSettings({ scrollContentHeight, onDragActiveChange }: Props): React.JSX.Element { - const [customKeys, setCustomKeys] = useState([]) + const { + value: customKeys, + setValue: setCustomKeys, + persist: persistCustomKeys, + refresh: refreshCustomKeys, + adoptStored: adoptStoredCustomKeys + } = useSerializedStoredState([], loadCustomKeys, saveCustomKeys) const [showCustomKeyModal, setShowCustomKeyModal] = useState(false) - const [shortcutLayout, setShortcutLayout] = useState( - getDefaultTerminalAccessoryLayout + const { + value: shortcutLayout, + setValue: setShortcutLayout, + persist: persistLayout, + refresh: refreshShortcutLayout + } = useSerializedStoredState( + getDefaultTerminalAccessoryLayout, + loadShortcutLayout, + saveTerminalAccessoryLayout ) - const layoutWriteChainRef = useRef>(Promise.resolve()) - const layoutWriteSeqRef = useRef(0) - const pendingLayoutWritesRef = useRef(0) - - const persistLayout = useCallback((next: TerminalAccessoryLayout) => { - layoutWriteSeqRef.current += 1 - pendingLayoutWritesRef.current += 1 - layoutWriteChainRef.current = layoutWriteChainRef.current - .catch(() => {}) - .then(() => saveTerminalAccessoryLayout(next)) - .catch(() => {}) - .finally(() => { - pendingLayoutWritesRef.current -= 1 - }) - }, []) - - const refreshShortcutLayout = useCallback(() => { - const refreshSeq = layoutWriteSeqRef.current - void loadTerminalAccessoryLayout().then((layout) => { - if (pendingLayoutWritesRef.current > 0 || refreshSeq !== layoutWriteSeqRef.current) { - return - } - setShortcutLayout({ - orderedBuiltInIds: layout.orderedBuiltInIds, - visibleBuiltInIds: layout.visibleBuiltInIds - }) - }) - }, []) - - const customKeysWriteChainRef = useRef>(Promise.resolve()) - const customKeysWriteSeqRef = useRef(0) - const pendingCustomKeysWritesRef = useRef(0) - - // Why: same stale-snapshot guard as persistLayout — a focus/AppState refresh - // racing an in-flight save must not overwrite the optimistic state. - const persistCustomKeys = useCallback((next: CustomKey[]) => { - customKeysWriteSeqRef.current += 1 - pendingCustomKeysWritesRef.current += 1 - customKeysWriteChainRef.current = customKeysWriteChainRef.current - .catch(() => {}) - .then(() => saveCustomKeys(next)) - .catch(() => {}) - .finally(() => { - pendingCustomKeysWritesRef.current -= 1 - }) - }, []) - - const refreshCustomKeys = useCallback(() => { - const refreshSeq = customKeysWriteSeqRef.current - void loadCustomKeys().then((keys) => { - if (pendingCustomKeysWritesRef.current > 0 || refreshSeq !== customKeysWriteSeqRef.current) { - return - } - setCustomKeys(keys) - }) - }, []) const handleDeleteCustomKey = useCallback( (key: CustomKey) => { @@ -294,6 +257,9 @@ export function TerminalShortcutSettings({ pressed && styles.deleteButtonPressed ]} onPress={() => handleDeleteCustomKey(key)} + hitSlop={8} + accessibilityRole="button" + accessibilityLabel={`Remove ${key.label}`} > @@ -316,12 +282,7 @@ export function TerminalShortcutSettings({ setShowCustomKeyModal(false)} - onKeysChanged={(keys) => { - // Why: the modal already persisted this list; bumping the sequence - // discards refreshes that read storage before its save landed. - customKeysWriteSeqRef.current += 1 - setCustomKeys(keys) - }} + onKeysChanged={adoptStoredCustomKeys} /> ) diff --git a/mobile/src/components/custom-key-modal-save-refusal.test.tsx b/mobile/src/components/custom-key-modal-save-refusal.test.tsx index c1c96edda1e..120b9d6565e 100644 --- a/mobile/src/components/custom-key-modal-save-refusal.test.tsx +++ b/mobile/src/components/custom-key-modal-save-refusal.test.tsx @@ -4,19 +4,33 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' const store = vi.hoisted(() => { // Annotated rather than asserted: the literal alone narrows `raw` to `null`. - const held: { raw: string | null; refuse: boolean } = { raw: null, refuse: false } + const held: { + raw: string | null + refuse: boolean + failRead: boolean + writes: number + holdWrites: Promise | null + } = { raw: null, refuse: false, failRead: false, writes: 0, holdWrites: null } return held }) vi.mock('@react-native-async-storage/async-storage', () => ({ default: { - getItem: async () => store.raw, - setItem: async () => { + getItem: async () => { + if (store.failRead) { + throw new Error('the store could not be read') + } + return store.raw + }, + setItem: async (_key: string, value: string) => { + await store.holdWrites + store.writes += 1 if (store.refuse) { throw new Error( 'Orca could not save orca:custom-accessory-keys: a stored value may be too large.' ) } + store.raw = value } } })) @@ -52,7 +66,7 @@ vi.mock('react-native', () => ({ vi.mock('lucide-react-native', () => ({ ChevronLeft: hosts.View })) vi.mock('./BottomDrawer', () => ({ BottomDrawer: hosts.View })) -import { CustomKeyModal } from './CustomKeyModal' +import { CustomKeyModal, removeCustomKey, saveCustomKeys } from './CustomKeyModal' import { readMirroredStorage } from '../storage/mirrored-storage-keys' const CUSTOM_KEYS = 'orca:custom-accessory-keys' @@ -110,6 +124,9 @@ function addAShortcut(renderer: ReturnType): void { beforeEach(() => { store.raw = null store.refuse = false + store.failRead = false + store.writes = 0 + store.holdWrites = null }) /** @@ -174,3 +191,71 @@ describe('adding a custom key when the store refuses the write', () => { expect(onClose).toHaveBeenCalledTimes(1) }) }) + +describe('removing a custom key', () => { + const kept = { id: 'custom-1', label: 'Ctrl+C', bytes: '\u0003', enter: false } + const removed = { id: 'custom-2', label: 'Ctrl+D', bytes: '\u0004', enter: false } + + it('rejects when the store refuses, so a caller never drops the row', async () => { + store.raw = JSON.stringify([kept, removed]) + store.refuse = true + await expect(removeCustomKey(removed.id)).rejects.toThrow() + }) + + it('answers the stored list without the key once the store takes it', async () => { + store.raw = JSON.stringify([kept, removed]) + await expect(removeCustomKey(removed.id)).resolves.toEqual([kept]) + }) + + it('lets two removals in flight together both land', async () => { + const third = { id: 'custom-3', label: 'Ctrl+Z', bytes: '\u001a', enter: false } + store.raw = JSON.stringify([kept, removed, third]) + await Promise.all([removeCustomKey(kept.id), removeCustomKey(removed.id)]) + expect(JSON.parse(store.raw ?? '[]')).toEqual([third]) + }) + + it('removes from the list a settings save in flight stored, not the list before it', async () => { + const third = { id: 'custom-3', label: 'Ctrl+Z', bytes: '\u001a', enter: false } + store.raw = JSON.stringify([kept, removed, third]) + let releaseWrites: () => void = () => {} + store.holdWrites = new Promise((resolve) => { + releaseWrites = resolve + }) + const reorder = saveCustomKeys([third, removed, kept]) + const removal = removeCustomKey(removed.id) + releaseWrites() + await Promise.all([reorder, removal]) + expect(JSON.parse(store.raw ?? '[]')).toEqual([third, kept]) + }) + + it('writes nothing when the stored list cannot be read, so a read failure erases nothing', async () => { + store.raw = JSON.stringify([kept, removed]) + store.failRead = true + await expect(removeCustomKey(removed.id)).rejects.toThrow() + expect(store.writes).toBe(0) + }) +}) + +describe('adding a custom key when the stored list cannot be read', () => { + it('writes nothing and keeps the drawer open', async () => { + store.failRead = true + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const onKeysChanged = vi.fn() + const onClose = vi.fn() + let renderer: ReturnType | null = null + act(() => { + renderer = create(createElement(CustomKeyModal, { visible: true, onClose, onKeysChanged })) + }) + if (renderer === null) { + throw new Error('the modal did not render') + } + addAShortcut(renderer) + await act(async () => { + await Promise.resolve() + await Promise.resolve() + }) + expect(store.writes).toBe(0) + expect(onKeysChanged).not.toHaveBeenCalled() + expect(onClose).not.toHaveBeenCalled() + }) +}) diff --git a/mobile/src/components/terminal-shortcut-settings-custom-keys.test.tsx b/mobile/src/components/terminal-shortcut-settings-custom-keys.test.tsx new file mode 100644 index 00000000000..da54bf9f66b --- /dev/null +++ b/mobile/src/components/terminal-shortcut-settings-custom-keys.test.tsx @@ -0,0 +1,163 @@ +import { createElement, useEffect, type ReactNode } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { CustomKey } from './CustomKeyModal' + +const CUSTOM_KEYS = 'orca:custom-accessory-keys' + +const store = vi.hoisted(() => { + const held: { + values: Map + holdWrites: Promise | null + refuseWrites: boolean + } = { values: new Map(), holdWrites: null, refuseWrites: false } + return held +}) + +vi.mock('@react-native-async-storage/async-storage', () => ({ + default: { + getItem: async (key: string) => store.values.get(key) ?? null, + setItem: async (key: string, value: string) => { + await store.holdWrites + if (store.refuseWrites) { + throw new Error('the store refused the write') + } + store.values.set(key, value) + } + } +})) +const hosts = vi.hoisted(() => { + const make = (name: string) => { + const Host = (props: { children?: ReactNode }): ReactNode => props.children ?? null + Host.displayName = name + return Host + } + return { View: make('View'), Text: make('Text'), Pressable: make('Pressable') } +}) +vi.mock('react-native', () => ({ + AppState: { addEventListener: () => ({ remove: () => {} }) }, + View: hosts.View, + Text: hosts.Text, + Pressable: hosts.Pressable, + Switch: hosts.View, + StyleSheet: { create: (styles: T) => styles, absoluteFillObject: {} }, + Platform: { OS: 'ios', select: (options: Record) => options.ios } +})) +vi.mock('expo-router', () => ({ + useFocusEffect: (effect: () => void) => useEffect(effect, [effect]) +})) +vi.mock('lucide-react-native', () => ({ + ChevronLeft: hosts.View, + ChevronRight: hosts.View, + X: hosts.View +})) +vi.mock('./BottomDrawer', () => ({ BottomDrawer: hosts.View })) +const lists = vi.hoisted(() => { + const held: { custom: { items: CustomKey[]; onReorder: (ids: string[]) => void } | null } = { + custom: null + } + return held +}) +vi.mock('./DragReorderList', () => ({ + DragReorderList: (props: { + items: { id: string }[] + renderRow: (item: { id: string }) => ReactNode + onReorder: (ids: string[]) => void + }): ReactNode => { + if (props.items.some((item) => item.id.startsWith('custom-'))) { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the custom-key list is the only one whose ids start with `custom-`. + lists.custom = props as unknown as { items: CustomKey[]; onReorder: (ids: string[]) => void } + } + return props.items.map((item) => props.renderRow(item)) + } +})) + +import { TerminalShortcutSettings } from './TerminalShortcutSettings' + +const keys: CustomKey[] = ['a', 'b', 'c'].map((name) => ({ + id: `custom-${name}`, + label: name, + bytes: name, + enter: false +})) + +function stored(): string[] { + const raw = store.values.get(CUSTOM_KEYS) + const parsed: CustomKey[] = raw ? JSON.parse(raw) : [] + return parsed.map((key) => key.id) +} + +function shown(): string[] { + return lists.custom?.items.map((key) => key.id) ?? [] +} + +let renderer: ReactTestRenderer | null = null + +async function mount(): Promise { + await act(async () => { + renderer = create( + createElement(TerminalShortcutSettings, { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mocked reorder list never reads the scroll bindings. + scrollRef: {} as never, + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: as above. + scrollOffsetY: {} as never, + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: as above. + scrollContentHeight: {} as never, + onDragActiveChange: () => {} + }) + ) + }) +} + +function pressRemove(label: string): void { + const button = renderer?.root.find( + (node) => node.type === hosts.Pressable && node.props.accessibilityLabel === `Remove ${label}` + ) + act(() => button?.props.onPress()) +} + +async function settle(): Promise { + await act(async () => { + for (let turn = 0; turn < 10; turn += 1) { + await Promise.resolve() + } + }) +} + +beforeEach(() => { + store.values = new Map([[CUSTOM_KEYS, JSON.stringify(keys)]]) + store.holdWrites = null + store.refuseWrites = false + lists.custom = null +}) + +afterEach(() => { + act(() => renderer?.unmount()) + renderer = null +}) + +describe('custom shortcuts in terminal settings', () => { + it('keeps a key removed when the list is reordered before the removal is saved', async () => { + await mount() + let releaseWrites: () => void = () => {} + store.holdWrites = new Promise((resolve) => { + releaseWrites = resolve + }) + pressRemove('b') + act(() => lists.custom?.onReorder(['custom-c', 'custom-a'])) + releaseWrites() + await settle() + expect(shown()).toEqual(['custom-c', 'custom-a']) + expect(stored()).toEqual(['custom-c', 'custom-a']) + }) + + it('shows the key again when the store refuses its removal', async () => { + await mount() + store.refuseWrites = true + pressRemove('b') + expect(shown()).toEqual(['custom-a', 'custom-c']) + await settle() + expect(shown()).toEqual(['custom-a', 'custom-b', 'custom-c']) + expect(stored()).toEqual(['custom-a', 'custom-b', 'custom-c']) + }) +}) diff --git a/mobile/src/home/MobileHomeTopBar.tsx b/mobile/src/home/MobileHomeTopBar.tsx index ac6d800c7f6..c97648fab23 100644 --- a/mobile/src/home/MobileHomeTopBar.tsx +++ b/mobile/src/home/MobileHomeTopBar.tsx @@ -15,6 +15,9 @@ export function MobileHomeTopBar({ onOpenSettings }: { onOpenSettings: () => voi [styles.iconButton, pressed && styles.iconButtonPressed]} onPress={onOpenSettings} + hitSlop={8} + accessibilityRole="button" + accessibilityLabel="Settings" > diff --git a/mobile/src/session/MobileNativeChatFloatingControls.tsx b/mobile/src/session/MobileNativeChatFloatingControls.tsx index 2ca716de860..f0ac6db0179 100644 --- a/mobile/src/session/MobileNativeChatFloatingControls.tsx +++ b/mobile/src/session/MobileNativeChatFloatingControls.tsx @@ -23,6 +23,7 @@ export function MobileNativeChatFloatingControls({ [styles.toolsPill, pressed && styles.pressed]} onPress={onToggleTools} > diff --git a/mobile/src/session/use-mobile-session-keyboard-state.test.tsx b/mobile/src/session/use-mobile-session-keyboard-state.test.tsx index 5eafbb4fad9..f15bc5297cb 100644 --- a/mobile/src/session/use-mobile-session-keyboard-state.test.tsx +++ b/mobile/src/session/use-mobile-session-keyboard-state.test.tsx @@ -4,14 +4,21 @@ import { act, create, type ReactTestRenderer } from 'react-test-renderer' import { beforeEach, describe, expect, it, vi } from 'vitest' import type { SoftKeyboardState } from '../platform/keyboard-occlusion' import type { RouteHandoff } from '../navigation/route-handoff' +import type { CustomKey } from '../components/CustomKeyModal' const harness = vi.hoisted(() => { const keyboard: SoftKeyboardState = { height: 0, visible: false } + const deleteCustomKey: { current: ((key: CustomKey) => Promise) | null } = { + current: null + } return { keyboard, + deleteCustomKey, notifyKeyboardVisibility: vi.fn<(visible: boolean) => void>(), notifyTerminalFrameHeight: vi.fn<(height: number) => void>(), - setKeyboardHeight: vi.fn>>() + setKeyboardHeight: vi.fn>>(), + setCustomKeys: vi.fn>>(), + removeCustomKey: vi.fn<(id: string) => Promise>() } }) @@ -26,7 +33,7 @@ vi.mock('../terminal/terminal-viewport-refit', () => ({ notifyTerminalFrameHeight: harness.notifyTerminalFrameHeight }) })) -vi.mock('../components/CustomKeyModal', () => ({ saveCustomKeys: () => Promise.resolve() })) +vi.mock('../components/CustomKeyModal', () => ({ removeCustomKey: harness.removeCustomKey })) vi.mock('../worktree/last-visited-worktree-repo', () => ({ writeLastVisitedWorktree: () => {} })) import { useMobileSessionKeyboardState } from './use-mobile-session-keyboard-state' @@ -49,7 +56,7 @@ const router: RouteHandoff = { } function Harness(): null { - useMobileSessionKeyboardState({ + const { handleDeleteCustomKey } = useMobileSessionKeyboardState({ hostId: 'host-1', worktreeId: 'wt-1', router, @@ -62,8 +69,7 @@ function Harness(): null { tabStripOffsetRef: { current: 0 }, tabStripViewportWidthRef: { current: 0 }, tabStripContentWidthRef: { current: 0 }, - customKeys: [], - setCustomKeys: () => {}, + setCustomKeys: harness.setCustomKeys, setShowCustomKeyModal: () => {}, setKeyboardHeight: harness.setKeyboardHeight, // Read only by the viewport refit, which is mocked above; present because the scope names them. @@ -78,6 +84,7 @@ function Harness(): null { viewportMeasuredRef: { current: false }, viewportRef: { current: null } }) + harness.deleteCustomKey.current = handleDeleteCustomKey return null } @@ -130,3 +137,32 @@ describe('what the session screen hears about the keyboard', () => { expect(harness.setKeyboardHeight.mock.calls.every(([height]) => height === 0)).toBe(true) }) }) + +describe('removing a custom shortcut from the session sheet', () => { + const key: CustomKey = { id: 'custom-1', label: 'Ctrl+C', bytes: '\u0003', enter: false } + + beforeEach(() => { + harness.setCustomKeys.mockClear() + harness.removeCustomKey.mockReset() + }) + + it('keeps the key on screen when the store refuses the removal', async () => { + harness.removeCustomKey.mockRejectedValue(new Error('refused')) + vi.spyOn(console, 'warn').mockImplementation(() => {}) + await mount() + await act(async () => { + await harness.deleteCustomKey.current?.(key) + }) + expect(harness.setCustomKeys).not.toHaveBeenCalled() + }) + + it('shows the list the store took once the removal lands', async () => { + harness.removeCustomKey.mockResolvedValue([]) + await mount() + await act(async () => { + await harness.deleteCustomKey.current?.(key) + }) + expect(harness.removeCustomKey).toHaveBeenCalledWith('custom-1') + expect(harness.setCustomKeys).toHaveBeenCalledWith([]) + }) +}) diff --git a/mobile/src/session/use-mobile-session-keyboard-state.ts b/mobile/src/session/use-mobile-session-keyboard-state.ts index c3dbbe0598e..41d459c5f00 100644 --- a/mobile/src/session/use-mobile-session-keyboard-state.ts +++ b/mobile/src/session/use-mobile-session-keyboard-state.ts @@ -1,7 +1,7 @@ import { useEffect, useCallback } from 'react' import { useSoftKeyboard } from '../platform/keyboard-occlusion' import { useTerminalViewportRefit } from '../terminal/terminal-viewport-refit' -import { saveCustomKeys, type CustomKey } from '../components/CustomKeyModal' +import { removeCustomKey, type CustomKey } from '../components/CustomKeyModal' import { writeLastVisitedWorktree } from '../worktree/last-visited-worktree-repo' import { resolveTabStripScrollOffset } from './tab-strip-scroll' import type { MobileSessionLifecycleModel } from './use-mobile-session-lifecycle' @@ -16,7 +16,6 @@ export type MobileSessionKeyboardScope = Pick< | 'activeSessionTabId' | 'clientRef' | 'connState' - | 'customKeys' | 'deviceTokenRef' | 'hostId' | 'router' @@ -54,7 +53,6 @@ export function useMobileSessionKeyboardState(scope: MobileSessionKeyboardScope) tabStripViewportWidthRef, tabStripContentWidthRef, tabLayoutsRef, - customKeys, setCustomKeys, setShowCustomKeyModal, setKeyboardHeight, @@ -135,14 +133,13 @@ export function useMobileSessionKeyboardState(scope: MobileSessionKeyboardScope) } }, [hostId, worktreeId]) - const handleDeleteCustomKey = useCallback( - async (key: CustomKey) => { - const updated = customKeys.filter((k) => k.id !== key.id) - setCustomKeys(updated) - await saveCustomKeys(updated) - }, - [customKeys] - ) + const handleDeleteCustomKey = useCallback(async (key: CustomKey) => { + try { + setCustomKeys(await removeCustomKey(key.id)) + } catch (error) { + console.warn('[custom-keys] the store would not take this removal', error) + } + }, []) const handleManageShortcuts = useCallback(() => { setShowCustomKeyModal(false) diff --git a/mobile/src/source-control/MobileSourceControlContent.test.tsx b/mobile/src/source-control/MobileSourceControlContent.test.tsx index f0ec2fd93ba..560b561d33b 100644 --- a/mobile/src/source-control/MobileSourceControlContent.test.tsx +++ b/mobile/src/source-control/MobileSourceControlContent.test.tsx @@ -365,3 +365,24 @@ describe('committed branch files during commit-message typing', () => { expect(format).not.toHaveBeenCalled() }) }) + +describe('AI commit message button', () => { + function generateButton() { + return renderer?.root.find( + (node) => + String(node.type) === 'Pressable' && + (node.props.accessibilityLabel === 'Generate commit message with AI' || + node.props.accessibilityLabel === 'Cancel commit message generation') + ) + } + + it('matches desktop: a typed message blocks regenerating, and cancel stays available', () => { + render() + expect(generateButton()?.props.disabled).toBe(false) + patch({ commitMessage: 'my own message' }) + expect(generateButton()?.props.disabled).toBe(true) + expect(generateButton()?.props.accessibilityHint).toBe('Clear the message to regenerate.') + patch({ generatingMessage: true }) + expect(generateButton()?.props.disabled).toBe(false) + }) +}) diff --git a/mobile/src/source-control/MobileSourceControlContent.tsx b/mobile/src/source-control/MobileSourceControlContent.tsx index 320e44ff8cc..7a4ab882285 100644 --- a/mobile/src/source-control/MobileSourceControlContent.tsx +++ b/mobile/src/source-control/MobileSourceControlContent.tsx @@ -52,6 +52,9 @@ export function MobileSourceControlContent({ state }: Props) { // Why: a capped listing can hide unstaged rows behind an all-staged prefix. const canStageAll = stageablePaths.length > 0 || status?.didHitLimit === true const shouldShowGenerateButton = stagedCount > 0 || generatingMessage + // Why: as on desktop, a typed message blocks regenerating; while generating the button cancels. + const generateBlockedByMessage = !generatingMessage && commitMessage.trim().length > 0 + const generateDisabled = busyAction !== null || generateBlockedByMessage const createPrHeroActive = createPrAction.visible && !createPrAction.disabled && !createPrAction.pushFirst const fileList = useMemo( @@ -209,12 +212,16 @@ export function MobileSourceControlContent({ state }: Props) { [ styles.generateButton, - busyAction !== null && styles.commitButtonDisabled, + generateDisabled && styles.commitButtonDisabled, pressed && styles.commitButtonPressed ]} // Why: commit-message AI belongs to the commit path; hiding it // during Stage All keeps the quick action visually unambiguous. - disabled={busyAction !== null} + disabled={generateDisabled} + accessibilityState={{ disabled: generateDisabled }} + accessibilityHint={ + generateBlockedByMessage ? 'Clear the message to regenerate.' : undefined + } onPress={() => generatingMessage ? cancelGenerateCommitMessage() : void generateCommitMessage() } diff --git a/mobile/src/source-control/mobile-commit-during-generation.test.tsx b/mobile/src/source-control/mobile-commit-during-generation.test.tsx new file mode 100644 index 00000000000..7fb8a351e41 --- /dev/null +++ b/mobile/src/source-control/mobile-commit-during-generation.test.tsx @@ -0,0 +1,123 @@ +import { createElement, useRef, useState } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, describe, expect, it, vi, type Mock } from 'vitest' +import type { RpcClient } from '../transport/rpc-client' +import type { MobileGenerateCommitMessageResult } from './mobile-commit-message-ai' + +// Why: react-native is Flow source vitest will not parse; haptics reaches expo-haptics through it. +vi.mock('react-native', () => ({ + Platform: { OS: 'ios', select: (options: Record) => options.ios }, + StyleSheet: { create: (value: unknown) => value, hairlineWidth: 1 } +})) +vi.mock('expo-router', () => ({ + useRouter: () => ({ + back: () => undefined, + push: () => undefined, + replace: () => undefined, + navigate: () => undefined, + dismissTo: () => undefined, + prefetch: () => undefined, + canGoBack: () => false, + setParams: () => undefined + }) +})) +vi.mock('../platform/haptics', () => ({ triggerError: vi.fn(), triggerSuccess: vi.fn() })) +const ai = vi.hoisted(() => { + const held: { + resolve: ((result: MobileGenerateCommitMessageResult) => void) | null + cancel: Mock<() => void> + } = { resolve: null, cancel: vi.fn<() => void>() } + return held +}) +vi.mock('./mobile-commit-message-ai', () => ({ + requestMobileCommitMessage: () => + new Promise((resolve) => { + ai.resolve = resolve + }), + cancelMobileCommitMessage: async () => ai.cancel() +})) + +import { useRouteHandoff } from '../navigation/route-handoff' +import { useMobileSourceControlRunners } from './use-mobile-source-control-runners' + +// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mocked AI module and the injected senders are the only things that would reach the client. +const client = {} as RpcClient +type Runners = ReturnType +const view: { + message: string + type: ((next: string) => void) | null + runners: Runners | null +} = { message: '', type: null, runners: null } + +function Harness(): null { + const router = useRouteHandoff() + const [commitMessage, setCommitMessage] = useState('') + const [generatingMessage, setGeneratingMessage] = useState(false) + const mountedRef = useRef(true) + const busyActionRef = useRef(null) + view.message = commitMessage + view.type = setCommitMessage + view.runners = useMobileSourceControlRunners({ + client, + hostId: 'host-a', + worktreeId: 'wt-1', + status: null, + branchLabel: 'main', + commitMessage, + stagedEntries: [], + generatingMessage, + stageablePaths: [], + unstageablePaths: [], + router, + sendGitRequest: vi.fn(), + sendCommitRequest: vi.fn().mockResolvedValue({ success: true }), + runGitSyncSteps: vi.fn(), + loadStatus: vi.fn().mockResolvedValue(true), + mountedRef, + busyActionRef, + setBusyAction: vi.fn(), + setActionError: vi.fn(), + setCommitMessage, + setGeneratingMessage, + setShowActionSheet: vi.fn(), + setLocalBranches: vi.fn(), + setShowBranchPicker: vi.fn(), + setCreatedPrUrl: vi.fn(), + setCreatedPrWarning: vi.fn(), + recordCommitFailure: vi.fn() + }) + return null +} + +let renderer: ReactTestRenderer | null = null + +afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + ai.resolve = null + ai.cancel.mockClear() +}) + +describe('committing while the AI is still writing a message', () => { + it('cancels the generation so its late reply does not refill the emptied box', async () => { + act(() => { + renderer = create(createElement(Harness)) + }) + let generating: Promise | undefined + act(() => { + generating = view.runners?.generateCommitMessage() + }) + act(() => view.type?.('fix: my own words')) + await act(async () => { + await view.runners?.commit() + }) + expect(ai.cancel).toHaveBeenCalledTimes(1) + expect(view.message).toBe('') + + await act(async () => { + ai.resolve?.({ success: true, message: 'feat: generated for the old diff' }) + await generating + }) + expect(view.message).toBe('') + }) +}) diff --git a/mobile/src/source-control/use-mobile-commit-message-generation.test.ts b/mobile/src/source-control/use-mobile-commit-message-generation.test.ts new file mode 100644 index 00000000000..ca183cbc494 --- /dev/null +++ b/mobile/src/source-control/use-mobile-commit-message-generation.test.ts @@ -0,0 +1,120 @@ +import { createElement, useRef, useState } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { RpcClient } from '../transport/rpc-client' +import type { MobileGenerateCommitMessageResult } from './mobile-commit-message-ai' + +vi.mock('../platform/haptics', () => ({ triggerError: vi.fn(), triggerSuccess: vi.fn() })) +const reply = vi.hoisted(() => { + const held: { + resolve: ((result: MobileGenerateCommitMessageResult) => void) | null + reject: ((error: Error) => void) | null + cancel: () => Promise + } = { resolve: null, reject: null, cancel: async () => {} } + return held +}) +vi.mock('./mobile-commit-message-ai', () => ({ + requestMobileCommitMessage: () => + new Promise((resolve, reject) => { + reply.resolve = resolve + reply.reject = reject + }), + cancelMobileCommitMessage: () => reply.cancel() +})) + +import { useMobileCommitMessageGeneration } from './use-mobile-commit-message-generation' + +// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the mocked request module never reaches the client. +const client = {} as RpcClient +const view: { + message: string + error: string | null + type: ((next: string) => void) | null + generate: (() => Promise) | null + cancel: (() => void) | null +} = { message: '', error: null, type: null, generate: null, cancel: null } + +function Harness(): null { + const [message, setMessage] = useState('') + const [generating, setGenerating] = useState(false) + const [error, setError] = useState(null) + const mountedRef = useRef(true) + const busyActionRef = useRef(null) + const { generateCommitMessage, cancelGenerateCommitMessage } = useMobileCommitMessageGeneration({ + client, + worktreeId: 'wt-1', + generatingMessage: generating, + mountedRef, + busyActionRef, + setGeneratingMessage: setGenerating, + setCommitMessage: setMessage, + setActionError: setError + }) + view.message = message + view.error = error + view.cancel = cancelGenerateCommitMessage + view.type = setMessage + view.generate = generateCommitMessage + return null +} + +let renderer: ReactTestRenderer | null = null + +afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + reply.resolve = null + reply.reject = null + reply.cancel = async () => {} +}) + +async function generateThen(typed: string | null): Promise { + act(() => { + renderer = create(createElement(Harness)) + }) + let pending: Promise | undefined + act(() => { + pending = view.generate?.() + }) + if (typed !== null) { + act(() => view.type?.(typed)) + } + await act(async () => { + reply.resolve?.({ success: true, message: 'feat: generated' }) + await pending + }) +} + +describe('useMobileCommitMessageGeneration', () => { + it('keeps what the user typed while the agent ran, as desktop does', async () => { + await generateThen('fix: my own words') + expect(view.message).toBe('fix: my own words') + }) + + it('fills an empty box with the generated message', async () => { + await generateThen(null) + expect(view.message).toBe('feat: generated') + }) + + it('shows no error from a run that was stopped and then failed', async () => { + reply.cancel = () => Promise.reject(new Error('transport closed')) + const unhandled = vi.fn() + process.on('unhandledRejection', unhandled) + act(() => { + renderer = create(createElement(Harness)) + }) + let pending: Promise | undefined + act(() => { + pending = view.generate?.() + }) + act(() => view.cancel?.()) + await act(async () => { + reply.reject?.(new Error('transport closed')) + await pending + await new Promise((resolve) => setTimeout(resolve, 0)) + }) + process.off('unhandledRejection', unhandled) + expect(view.error).toBeNull() + expect(unhandled).not.toHaveBeenCalled() + }) +}) diff --git a/mobile/src/source-control/use-mobile-commit-message-generation.ts b/mobile/src/source-control/use-mobile-commit-message-generation.ts index b697541d333..c23ff3c1894 100644 --- a/mobile/src/source-control/use-mobile-commit-message-generation.ts +++ b/mobile/src/source-control/use-mobile-commit-message-generation.ts @@ -1,4 +1,10 @@ -import { useCallback, type MutableRefObject } from 'react' +import { + useCallback, + useRef, + type Dispatch, + type MutableRefObject, + type SetStateAction +} from 'react' import type { RpcClient } from '../transport/rpc-client' import { triggerError, triggerSuccess } from '../platform/haptics' import { cancelMobileCommitMessage, requestMobileCommitMessage } from './mobile-commit-message-ai' @@ -10,7 +16,7 @@ type Params = { mountedRef: MutableRefObject busyActionRef: MutableRefObject setGeneratingMessage: (next: boolean) => void - setCommitMessage: (next: string) => void + setCommitMessage: Dispatch> setActionError: (next: string | null) => void } @@ -27,6 +33,7 @@ export function useMobileCommitMessageGeneration(params: Params) { setCommitMessage, setActionError } = params + const runRef = useRef(0) // AI-generate a commit message from the staged diff. Matches desktop: the // button is always available; a missing model surfaces as a toast. @@ -34,15 +41,18 @@ export function useMobileCommitMessageGeneration(params: Params) { if (!client || generatingMessage || busyActionRef.current) { return } + runRef.current += 1 + const run = runRef.current setGeneratingMessage(true) setActionError(null) try { const result = await requestMobileCommitMessage(client, worktreeId) - if (!mountedRef.current) { + if (!mountedRef.current || run !== runRef.current) { return } if (result.success) { - setCommitMessage(result.message) + // Why: same rule as desktop — text typed while the agent ran wins over the generated message. + setCommitMessage((current) => (current.length > 0 ? current : result.message)) triggerSuccess() } else if (!result.canceled) { triggerError() @@ -51,7 +61,7 @@ export function useMobileCommitMessageGeneration(params: Params) { } catch (err) { // Why: a transport drop rejects the RPC; without this the error haptic + // message are skipped and the rejection escapes the void-called handler. - if (mountedRef.current) { + if (mountedRef.current && run === runRef.current) { triggerError() setActionError(err instanceof Error ? err.message : 'Failed to generate commit message') } @@ -72,10 +82,15 @@ export function useMobileCommitMessageGeneration(params: Params) { ]) const cancelGenerateCommitMessage = useCallback(() => { - if (client) { - void cancelMobileCommitMessage(client, worktreeId) + if (!client || !generatingMessage) { + return } - }, [client, worktreeId]) + // Why: like desktop's canceled record — a run the host still finishes must not fill the box. + runRef.current += 1 + // Why: commit and Create PR call this without awaiting; a dropped transport must not surface + // as an unhandled rejection, and the run's own reply still clears the spinner. + cancelMobileCommitMessage(client, worktreeId).catch(() => {}) + }, [client, generatingMessage, worktreeId]) return { generateCommitMessage, cancelGenerateCommitMessage } } diff --git a/mobile/src/source-control/use-mobile-create-pr-runner.ts b/mobile/src/source-control/use-mobile-create-pr-runner.ts index b7fb9ce4145..47e5e00ebd2 100644 --- a/mobile/src/source-control/use-mobile-create-pr-runner.ts +++ b/mobile/src/source-control/use-mobile-create-pr-runner.ts @@ -31,6 +31,7 @@ type Params = { mountedRef: MutableRefObject runGitWorkflow: RunGitWorkflow loadStatus: LoadStatus + cancelGenerateCommitMessage: () => void setActionError: (next: string | null) => void setCommitMessage: (next: string) => void setShowActionSheet: (next: boolean) => void @@ -49,6 +50,7 @@ export function useMobileCreatePrRunner({ mountedRef, runGitWorkflow, loadStatus, + cancelGenerateCommitMessage, setActionError, setCommitMessage, setShowActionSheet, @@ -70,6 +72,8 @@ export function useMobileCreatePrRunner({ } let progress: MobileHostedReviewCreateIntentProgress | null = null const ran = await runGitWorkflow(pushFirst ? 'push-create-pr' : 'create-pr', async () => { + // Why: this run commits the box's message, so it supersedes a running generation too. + cancelGenerateCommitMessage() created.current = await runMobileHostedReviewCreateIntent(client, worktreeId, { branch, title: branchLabel, @@ -112,6 +116,7 @@ export function useMobileCreatePrRunner({ }, [ branchLabel, + cancelGenerateCommitMessage, client, commitMessage, loadStatus, diff --git a/mobile/src/source-control/use-mobile-source-control-commit-runners.ts b/mobile/src/source-control/use-mobile-source-control-commit-runners.ts index 79253e536ea..3439013c01a 100644 --- a/mobile/src/source-control/use-mobile-source-control-commit-runners.ts +++ b/mobile/src/source-control/use-mobile-source-control-commit-runners.ts @@ -19,6 +19,7 @@ type Params = { stagedEntries: MobileCommitFailureRecovery['stagedEntries'] sendGitRequest: SendGitRequest sendCommitRequest: (message: string) => Promise + cancelGenerateCommitMessage: () => void runGitSyncSteps: () => Promise runGitWorkflow: RunGitWorkflow loadStatus: (options?: LoadStatusOptions) => Promise @@ -37,7 +38,8 @@ export function useMobileSourceControlCommitRunners(params: Params) { commitMessage, stagedEntries, sendGitRequest, - sendCommitRequest, + sendCommitRequest: sendCommitRequestOnly, + cancelGenerateCommitMessage, runGitSyncSteps, runGitWorkflow, loadStatus, @@ -48,6 +50,15 @@ export function useMobileSourceControlCommitRunners(params: Params) { setCommitMessage, recordCommitFailure } = params + // Why: as on desktop, committing the box's message cancels a running generation so its late + // reply cannot refill the box the commit empties. + const sendCommitRequest = useCallback( + (message: string) => { + cancelGenerateCommitMessage() + return sendCommitRequestOnly(message) + }, + [cancelGenerateCommitMessage, sendCommitRequestOnly] + ) const commit = useCallback(async () => { const message = commitMessage.trim() diff --git a/mobile/src/source-control/use-mobile-source-control-runners.ts b/mobile/src/source-control/use-mobile-source-control-runners.ts index b1d5f2dded1..e0be41bc351 100644 --- a/mobile/src/source-control/use-mobile-source-control-runners.ts +++ b/mobile/src/source-control/use-mobile-source-control-runners.ts @@ -1,4 +1,4 @@ -import { useCallback, type MutableRefObject } from 'react' +import { useCallback, type Dispatch, type MutableRefObject, type SetStateAction } from 'react' import type { RouteHandoff } from '../navigation/route-handoff' import type { RpcClient } from '../transport/rpc-client' import { triggerError, triggerSuccess } from '../platform/haptics' @@ -38,7 +38,7 @@ type Params = { busyActionRef: MutableRefObject setBusyAction: (next: string | null) => void setActionError: (next: string | null) => void - setCommitMessage: (next: string) => void + setCommitMessage: Dispatch> setGeneratingMessage: (next: boolean) => void setShowActionSheet: (next: boolean) => void setLocalBranches: (next: RuntimeGitLocalBranches | null) => void @@ -179,22 +179,6 @@ export function useMobileSourceControlRunners(params: Params) { await runGitAction('unstage-all', 'git.bulkUnstage', { filePaths: unstageablePaths }) }, [runGitAction, unstageablePaths]) - const { commit, runCommitSequence, runCommitSyncSequence } = useMobileSourceControlCommitRunners({ - commitMessage, - stagedEntries, - sendGitRequest, - sendCommitRequest, - runGitSyncSteps, - runGitWorkflow, - loadStatus, - mountedRef, - busyActionRef, - setBusyAction, - setActionError, - setCommitMessage, - recordCommitFailure - }) - const { generateCommitMessage, cancelGenerateCommitMessage } = useMobileCommitMessageGeneration({ client, worktreeId, @@ -206,6 +190,23 @@ export function useMobileSourceControlRunners(params: Params) { setActionError }) + const { commit, runCommitSequence, runCommitSyncSequence } = useMobileSourceControlCommitRunners({ + commitMessage, + stagedEntries, + sendGitRequest, + sendCommitRequest, + cancelGenerateCommitMessage, + runGitSyncSteps, + runGitWorkflow, + loadStatus, + mountedRef, + busyActionRef, + setBusyAction, + setActionError, + setCommitMessage, + recordCommitFailure + }) + const createPr = useMobileCreatePrRunner({ client, worktreeId, @@ -216,6 +217,7 @@ export function useMobileSourceControlRunners(params: Params) { mountedRef, runGitWorkflow, loadStatus, + cancelGenerateCommitMessage, setActionError, setCommitMessage, setShowActionSheet, diff --git a/mobile/src/storage/use-serialized-stored-state.test.ts b/mobile/src/storage/use-serialized-stored-state.test.ts new file mode 100644 index 00000000000..a2f93126913 --- /dev/null +++ b/mobile/src/storage/use-serialized-stored-state.test.ts @@ -0,0 +1,69 @@ +import { createElement } from 'react' +import { act, create, type ReactTestRenderer } from 'react-test-renderer' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { useSerializedStoredState } from './use-serialized-stored-state' + +type Hook = ReturnType> +let latest: Hook | null = null +let renderer: ReactTestRenderer | null = null + +function mount(load: () => Promise, save: (value: string) => Promise): void { + function Harness(): null { + latest = useSerializedStoredState('a', load, save) + return null + } + act(() => { + renderer = create(createElement(Harness)) + }) +} + +afterEach(() => { + act(() => renderer?.unmount()) + renderer = null + latest = null +}) + +describe('useSerializedStoredState', () => { + it('shows the stored value again when a save is refused', async () => { + mount( + async () => 'a', + async () => { + throw new Error('refused') + } + ) + await act(async () => { + latest?.setValue('b') + latest?.persist('b') + }) + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)) + }) + expect(latest?.value).toBe('a') + }) + + it('leaves a later queued write in charge when an earlier save is refused', async () => { + const load = vi.fn(async () => 'a') + let landLater: () => void = () => {} + const save = vi.fn((value: string) => + value === 'b' + ? Promise.reject(new Error('refused')) + : new Promise((resolve) => { + landLater = resolve + }) + ) + mount(load, save) + await act(async () => { + latest?.persist('b') + latest?.setValue('c') + latest?.persist('c') + await new Promise((resolve) => setTimeout(resolve, 0)) + }) + expect(load).not.toHaveBeenCalled() + await act(async () => { + landLater() + await new Promise((resolve) => setTimeout(resolve, 0)) + }) + expect(load).not.toHaveBeenCalled() + expect(latest?.value).toBe('c') + }) +}) diff --git a/mobile/src/storage/use-serialized-stored-state.ts b/mobile/src/storage/use-serialized-stored-state.ts new file mode 100644 index 00000000000..9507ee24cc8 --- /dev/null +++ b/mobile/src/storage/use-serialized-stored-state.ts @@ -0,0 +1,58 @@ +import { useCallback, useRef, useState } from 'react' + +/** + * Screen state over a stored value, changed optimistically and saved one write at a time. + * + * A focus or AppState refresh that races an in-flight save must not overwrite the optimistic + * state, and a refused save must not leave the screen showing a change the store never took. + */ +export function useSerializedStoredState( + initial: T | (() => T), + load: () => Promise, + save: (value: T) => Promise +) { + const [value, setValue] = useState(initial) + const writeChainRef = useRef>(Promise.resolve()) + const writeSeqRef = useRef(0) + const pendingWritesRef = useRef(0) + + const refresh = useCallback(() => { + const refreshSeq = writeSeqRef.current + void load().then((stored) => { + if (pendingWritesRef.current > 0 || refreshSeq !== writeSeqRef.current) { + return + } + setValue(stored) + }) + }, [load]) + + const persist = useCallback( + (next: T) => { + writeSeqRef.current += 1 + pendingWritesRef.current += 1 + writeChainRef.current = writeChainRef.current + .then(() => save(next)) + .then( + () => false, + () => true + ) + .then((refused) => { + pendingWritesRef.current -= 1 + // Why: a refused write left the store as it was; show what it holds. A write still + // queued owns the screen, and a read started now could land after it and undo it. + if (refused && pendingWritesRef.current === 0) { + refresh() + } + }) + }, + [refresh, save] + ) + + /** A value another writer already stored; refreshes that read before it landed are dropped. */ + const adoptStored = useCallback((stored: T) => { + writeSeqRef.current += 1 + setValue(stored) + }, []) + + return { value, setValue, persist, refresh, adoptStored } +} diff --git a/mobile/src/tasks/mobile-tasks-screen-chrome.tsx b/mobile/src/tasks/mobile-tasks-screen-chrome.tsx index 4d6c583ea04..b2d964ed917 100644 --- a/mobile/src/tasks/mobile-tasks-screen-chrome.tsx +++ b/mobile/src/tasks/mobile-tasks-screen-chrome.tsx @@ -14,6 +14,14 @@ import { styles } from './mobile-tasks-legacy-styles' import { renderMobileTasksProviderControls } from './mobile-tasks-provider-controls' import { renderMobileTasksSearchControl } from './mobile-tasks-search-control' +// Same wording as the desktop task page's icon buttons. +const REFRESH_LABELS = { + github: 'Refresh GitHub work', + gitlab: 'Refresh GitLab work items', + linear: 'Refresh Linear', + jira: 'Refresh Jira issues' +} as const + export function renderMobileTasksChrome(model: ConnectionPresentationModel) { const { setTaskCopyFeedbackRootRef } = model return ( @@ -72,6 +80,9 @@ export function renderMobileTasksStatusBar(model: ConnectionPresentationModel) { { if (!taskUiReady) { return @@ -89,6 +100,15 @@ export function renderMobileTasksStatusBar(model: ConnectionPresentationModel) { { if (!taskUiReady) { return diff --git a/src/renderer/src/components/right-sidebar/source-control/commit/use-commit-flows.ts b/src/renderer/src/components/right-sidebar/source-control/commit/use-commit-flows.ts index c2628b3d9dc..a98685f3549 100644 --- a/src/renderer/src/components/right-sidebar/source-control/commit/use-commit-flows.ts +++ b/src/renderer/src/components/right-sidebar/source-control/commit/use-commit-flows.ts @@ -1,3 +1,4 @@ +import { useCallback } from 'react' import type { SourceControlPanelFoundation } from '../panel/use-panel-foundation' import { useSourceControlCreatePrIntentCommitMessage } from '../review/use-create-pr-intent-commit-message' import { useSourceControlConflictAbort } from '../sync/use-conflict-abort' @@ -97,6 +98,19 @@ export function useSourceControlCommitFlows(foundation: SourceControlPanelFounda updateCommitMessageGenerationRecord, worktreePath }) + const { handleCommit: commitStagedChanges } = commitAction + const { handleCancelGenerate } = commitMessageGeneration + // Why: committing the box's own message makes a running generation moot; cancel it so its late + // reply cannot refill the box the commit empties. Create PR commits carry their own target. + const handleCommit = useCallback( + (messageOverride, options) => { + if (!options?.target) { + handleCancelGenerate() + } + return commitStagedChanges(messageOverride, options) + }, + [commitStagedChanges, handleCancelGenerate] + ) const createPrIntentCommitMessage = useSourceControlCreatePrIntentCommitMessage({ activeRepo, generateInFlightRef, @@ -115,7 +129,7 @@ export function useSourceControlCommitFlows(foundation: SourceControlPanelFounda fastForwardBranch, fetchBranch, grouped, - handleCommit: commitAction.handleCommit, + handleCommit, pullBranch, pushBranch, rebaseFromBase, @@ -143,7 +157,7 @@ export function useSourceControlCommitFlows(foundation: SourceControlPanelFounda }) return { - ...commitAction, + handleCommit, ...commitMessageGeneration, ...createPrIntentCommitMessage, ...remoteActionRunner, diff --git a/src/renderer/src/components/right-sidebar/source-control/commit/use-commit-message-generation.cancel.test.ts b/src/renderer/src/components/right-sidebar/source-control/commit/use-commit-message-generation.cancel.test.ts new file mode 100644 index 00000000000..293c2fe96dd --- /dev/null +++ b/src/renderer/src/components/right-sidebar/source-control/commit/use-commit-message-generation.cancel.test.ts @@ -0,0 +1,111 @@ +// @vitest-environment happy-dom + +import { act, createElement } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest' +import { useAppStore } from '@/store' +import type { CommitDraftsByWorktree } from './commit-drafts' + +const host = vi.hoisted(() => { + const held: { + succeed: ((message: string) => void) | null + cancel: Mock<() => void> + } = { succeed: null, cancel: vi.fn<() => void>() } + return held +}) +vi.mock('@/runtime/runtime-git-generation-client', () => ({ + generateRuntimeCommitMessage: () => + new Promise((resolve) => { + host.succeed = (message) => resolve({ success: true, message }) + }), + cancelRuntimeGenerateCommitMessage: () => host.cancel() +})) +vi.mock('@/lib/connection-context', () => ({ getConnectionId: () => null })) + +import { useSourceControlCommitMessageGeneration } from './use-commit-message-generation' + +const WORKTREE_ID = 'repo-1::/repo' +const initialAppState = useAppStore.getInitialState() +let drafts: CommitDraftsByWorktree = {} +const generateInFlightRef: { current: Record } = { current: {} } +let latest: ReturnType | null = null +let root: Root | null = null + +function Probe(): null { + const records = useAppStore((s) => s.commitMessageGenerationRecords) + const state = useAppStore.getState() + latest = useSourceControlCommitMessageGeneration({ + activeRepo: null, + activeRepoTarget: { kind: 'local' }, + activeWorktreeId: WORKTREE_ID, + allocateCommitMessageGenerationRequestId: state.allocateCommitMessageGenerationRequestId, + commitMessageGenerationRecords: records, + generateErrors: {}, + generateInFlightByWorktree: {}, + generateInFlightRef, + openCommitGenerationDialog: () => {}, + resolvedCommitMessageAi: null, + setCommitMessageGenerationRecord: state.setCommitMessageGenerationRecord, + setGenerateErrors: () => {}, + setGenerateInFlightByWorktree: () => {}, + settings: null, + sourceControlAiActionsVisible: true, + updateCommitDrafts: (update) => { + drafts = typeof update === 'function' ? update(drafts) : update + }, + updateCommitMessageGenerationRecord: state.updateCommitMessageGenerationRecord, + worktreePath: '/repo' + }) + return null +} + +async function startGenerating(): Promise<{ pending: Promise | undefined }> { + await act(async () => { + root = createRoot(document.createElement('div')) + root.render(createElement(Probe)) + }) + let pending: Promise | undefined + await act(async () => { + pending = latest?.handleGenerate({ + sourceControlAiResolvedParams: { agentId: 'claude', model: 'sonnet' } + }) + }) + return { pending } +} + +beforeEach(() => { + useAppStore.setState({ recordFeatureInteraction: async () => {} }) + drafts = {} + generateInFlightRef.current = {} +}) + +afterEach(() => { + act(() => root?.unmount()) + root = null + latest = null + host.succeed = null + host.cancel.mockClear() + useAppStore.setState(initialAppState, true) +}) + +describe('a commit message generation that was canceled', () => { + it('fills the empty box when nothing canceled the run', async () => { + const { pending } = await startGenerating() + await act(async () => { + host.succeed?.('feat: generated') + await pending + }) + expect(drafts[WORKTREE_ID]).toBe('feat: generated') + }) + + it('drops the late reply after a cancel, so a commit that emptied the box stays empty', async () => { + const { pending } = await startGenerating() + act(() => latest?.handleCancelGenerate()) + expect(host.cancel).toHaveBeenCalledTimes(1) + await act(async () => { + host.succeed?.('feat: generated for the old diff') + await pending + }) + expect(drafts[WORKTREE_ID]).toBeUndefined() + }) +}) diff --git a/src/renderer/src/components/right-sidebar/source-control/commit/use-commit-message-generation.ts b/src/renderer/src/components/right-sidebar/source-control/commit/use-commit-message-generation.ts index d7bc77ed887..aae588e58ba 100644 --- a/src/renderer/src/components/right-sidebar/source-control/commit/use-commit-message-generation.ts +++ b/src/renderer/src/components/right-sidebar/source-control/commit/use-commit-message-generation.ts @@ -169,20 +169,23 @@ export function useSourceControlCommitMessageGeneration({ return } - updateCommitMessageGenerationRecord(activeCommitMessageGenerationKey, (record) => - resolveCommitMessageGenerationSuccess({ - record, - requestId, - message: result.message - }) - ) + const succeeded = resolveCommitMessageGenerationSuccess({ + record: + useAppStore.getState().commitMessageGenerationRecords[activeCommitMessageGenerationKey], + requestId, + message: result.message + }) + if (succeeded) { + setCommitMessageGenerationRecord(activeCommitMessageGenerationKey, succeeded) + } // Why: race protection — drop the generated message if the user typed into the textarea while the agent ran, rather than overwrite their edits. + // A run canceled (by Stop or by a commit) may still succeed on the host; its message + // describes changes the box no longer holds. updateCommitDrafts((prev) => { const current = prev[activeWorktreeId] - if (current && current.length > 0) { - return prev - } - return writeCommitDraftForWorktree(prev, activeWorktreeId, result.message) + return !succeeded || (current && current.length > 0) + ? prev + : writeCommitDraftForWorktree(prev, activeWorktreeId, result.message) }) useAppStore.getState().recordFeatureInteraction('ai-commit-generation') setGenerateErrors((prev) => ({ ...prev, [activeWorktreeId]: null }))