diff --git a/apps/mobile/src/components/agents/session-detail-content.tsx b/apps/mobile/src/components/agents/session-detail-content.tsx index 9cb4fb8627..0427874d5f 100644 --- a/apps/mobile/src/components/agents/session-detail-content.tsx +++ b/apps/mobile/src/components/agents/session-detail-content.tsx @@ -46,10 +46,12 @@ import { import { useInteractionHandlers } from '@/components/agents/use-interaction-handlers'; import { useSessionAutoScroll } from '@/components/agents/use-session-auto-scroll'; import { useSessionConfigSync } from '@/components/agents/use-session-config-sync'; +import { useSessionDetailRename } from '@/components/agents/use-session-detail-rename'; import { WorkingIndicator } from '@/components/agents/working-indicator'; import { ChildSessionSheet } from '@/components/agents/child-session-sheet'; import { PartRenderer } from '@/components/agents/part-renderer'; import { QueryError } from '@/components/query-error'; +import { RenameModal } from '@/components/rename-modal'; import { ScreenHeader } from '@/components/screen-header'; import { Button } from '@/components/ui/button'; import { Skeleton } from '@/components/ui/skeleton'; @@ -452,8 +454,16 @@ export function SessionDetailContent({ const emptyStateText = statusIndicator ? null : 'No messages yet'; - const title = - fetchedData?.kiloSessionId === sessionId ? (fetchedData.title ?? 'Session') : 'Session'; + const isSessionLoaded = fetchedData?.kiloSessionId === sessionId; + const serverTitle = isSessionLoaded ? (fetchedData.title ?? undefined) : undefined; + const rename = useSessionDetailRename({ + sessionId, + isLoaded: isSessionLoaded, + serverTitle, + fallbackTitle: 'Session', + }); + const handleRenameSave = rename.submit; + const handleRenameClose = rename.closeModal; const requiresModel = Boolean(fetchedData?.cloudAgentSessionId); const blockingInteraction = getBlockingInteraction({ activeQuestion, activePermission }); const hasBlockingInteraction = blockingInteraction !== 'none'; @@ -507,7 +517,16 @@ export function SessionDetailContent({ return ( - + {!isConnected && } @@ -552,6 +571,16 @@ export function SessionDetailContent({ }} /> ) : null} + + {rename.isTitleInteractive && rename.isModalOpen ? ( + + ) : null} ); diff --git a/apps/mobile/src/components/agents/session-detail-rename-state.test.ts b/apps/mobile/src/components/agents/session-detail-rename-state.test.ts new file mode 100644 index 0000000000..712e0a4f5f --- /dev/null +++ b/apps/mobile/src/components/agents/session-detail-rename-state.test.ts @@ -0,0 +1,200 @@ +import { describe, expect, it } from 'vitest'; + +import { + getSessionDetailRenameState, + initialRenameState, + type RenameState, + renameStateReducer, +} from './session-detail-rename-state'; + +describe('getSessionDetailRenameState', () => { + const fallbackTitle = 'Session'; + + it('returns a non-interactive title while the session record is still loading', () => { + expect( + getSessionDetailRenameState({ + fallbackTitle, + isLoaded: false, + serverTitle: undefined, + renameState: initialRenameState(), + }) + ).toEqual({ + title: fallbackTitle, + isTitleInteractive: false, + modalInitialValue: null, + isModalOpen: false, + }); + }); + + it('exposes the title as interactive once the current session record is loaded', () => { + expect( + getSessionDetailRenameState({ + fallbackTitle, + isLoaded: true, + serverTitle: 'Original', + renameState: initialRenameState(), + }) + ).toEqual({ + title: 'Original', + isTitleInteractive: true, + modalInitialValue: null, + isModalOpen: false, + }); + }); + + it('hides interactivity when fetched data belongs to a different session', () => { + expect( + getSessionDetailRenameState({ + fallbackTitle, + isLoaded: false, + serverTitle: undefined, + renameState: initialRenameState(), + }) + ).toEqual({ + title: fallbackTitle, + isTitleInteractive: false, + modalInitialValue: null, + isModalOpen: false, + }); + }); + + it('shows the optimistic override in the header when one is pending', () => { + expect( + getSessionDetailRenameState({ + fallbackTitle, + isLoaded: true, + serverTitle: 'Original', + renameState: { ...initialRenameState(), optimisticTitle: 'Pending' }, + }) + ).toEqual({ + title: 'Pending', + isTitleInteractive: true, + modalInitialValue: null, + isModalOpen: false, + }); + }); + + it('seeds the modal with the current title only while it is open', () => { + const open = getSessionDetailRenameState({ + fallbackTitle, + isLoaded: true, + serverTitle: 'Original', + renameState: { ...initialRenameState(), isModalOpen: true }, + }); + expect(open.modalInitialValue).toBe('Original'); + expect(open.isTitleInteractive).toBe(true); + expect(open.isModalOpen).toBe(true); + + const closed = getSessionDetailRenameState({ + fallbackTitle, + isLoaded: true, + serverTitle: 'Original', + renameState: initialRenameState(), + }); + expect(closed.modalInitialValue).toBeNull(); + expect(closed.isModalOpen).toBe(false); + }); + + it('seeds the modal with the optimistic override when one is pending', () => { + // The modal should always start from whatever the header is currently + // showing so re-opening after a prior optimistic update still presents + // the live value. + expect( + getSessionDetailRenameState({ + fallbackTitle, + isLoaded: true, + serverTitle: 'Original', + renameState: { ...initialRenameState(), isModalOpen: true, optimisticTitle: 'Pending' }, + }).modalInitialValue + ).toBe('Pending'); + }); +}); + +describe('renameStateReducer', () => { + it('opens and closes the modal', () => { + const opened = renameStateReducer(initialRenameState(), { type: 'openModal' }); + expect(opened.isModalOpen).toBe(true); + + const closed = renameStateReducer(opened, { type: 'closeModal' }); + expect(closed.isModalOpen).toBe(false); + expect(closed).toEqual(initialRenameState()); + }); + + it('sets the optimistic title while keeping the modal open on submit', () => { + const modalOpen = renameStateReducer(initialRenameState(), { type: 'openModal' }); + const next = renameStateReducer(modalOpen, { + type: 'submit', + nextTitle: 'Renamed', + }); + // RenameModal awaits onSave and only calls onClose after success. The + // reducer must keep the modal mounted so a rejection can display the + // inline error and allow retry. + expect(next.isModalOpen).toBe(true); + expect(next.optimisticTitle).toBe('Renamed'); + }); + + it('restores the previously displayed title on submit failure while keeping the modal open', () => { + const modalOpen = renameStateReducer(initialRenameState(), { type: 'openModal' }); + const submitted = renameStateReducer(modalOpen, { + type: 'submit', + nextTitle: 'Renamed', + }); + const failed = renameStateReducer(submitted, { + type: 'submitFailure', + previousTitle: 'Original', + }); + expect(failed.optimisticTitle).toBe('Original'); + expect(failed.isModalOpen).toBe(true); + }); + + it('restores the previously displayed title on a second failure after a successful rename', () => { + // The authoritative server title stays A because the detail manager's + // fetchedData is not refreshed by list-query invalidation. After the + // first rename A→B succeeds, the displayed title is optimistic B. A + // second rename B→C must fail back to B, not to stale A. + const getDisplayedTitle = (state: RenameState) => + getSessionDetailRenameState({ + fallbackTitle: 'Session', + isLoaded: true, + serverTitle: 'A', + renameState: state, + }).title; + + let state = initialRenameState(); + state = renameStateReducer(state, { type: 'openModal' }); + state = renameStateReducer(state, { type: 'submit', nextTitle: 'B' }); + expect(getDisplayedTitle(state)).toBe('B'); + + state = renameStateReducer(state, { type: 'openModal' }); + const displayedBeforeSecondSubmit = getDisplayedTitle(state); + expect(displayedBeforeSecondSubmit).toBe('B'); + + state = renameStateReducer(state, { type: 'submit', nextTitle: 'C' }); + state = renameStateReducer(state, { + type: 'submitFailure', + previousTitle: displayedBeforeSecondSubmit, + }); + expect(getDisplayedTitle(state)).toBe('B'); + }); + + it('clears the optimistic override when the authoritative server title changes', () => { + const submitted = renameStateReducer(initialRenameState(), { + type: 'submit', + nextTitle: 'Renamed', + }); + const updated = renameStateReducer(submitted, { type: 'serverTitleChanged' }); + expect(updated.optimisticTitle).toBeNull(); + expect(updated.isModalOpen).toBe(false); + }); + + it('closes the modal and clears the optimistic override when the session changes', () => { + const modalOpen = renameStateReducer(initialRenameState(), { type: 'openModal' }); + const submitted = renameStateReducer(modalOpen, { + type: 'submit', + nextTitle: 'Renamed', + }); + const changed = renameStateReducer(submitted, { type: 'sessionChanged' }); + expect(changed.isModalOpen).toBe(false); + expect(changed.optimisticTitle).toBeNull(); + }); +}); diff --git a/apps/mobile/src/components/agents/session-detail-rename-state.ts b/apps/mobile/src/components/agents/session-detail-rename-state.ts new file mode 100644 index 0000000000..da7695eced --- /dev/null +++ b/apps/mobile/src/components/agents/session-detail-rename-state.ts @@ -0,0 +1,74 @@ +export type RenameState = { + isModalOpen: boolean; + optimisticTitle: string | null; +}; + +type RenameEvent = + | { type: 'openModal' } + | { type: 'closeModal' } + | { type: 'submit'; nextTitle: string } + | { type: 'submitFailure'; previousTitle: string } + | { type: 'serverTitleChanged' } + | { type: 'sessionChanged' }; + +export function initialRenameState(): RenameState { + return { isModalOpen: false, optimisticTitle: null }; +} + +/** + * Pure reducer that owns the rename modal and optimistic header title + * transitions. It is extracted so the full lifecycle stays unit-testable + * without rendering React Native. + */ +export function renameStateReducer(state: RenameState, event: RenameEvent): RenameState { + switch (event.type) { + case 'openModal': { + return { ...state, isModalOpen: true }; + } + case 'closeModal': { + return { ...state, isModalOpen: false }; + } + case 'submit': { + return { ...state, optimisticTitle: event.nextTitle }; + } + case 'submitFailure': { + return { ...state, optimisticTitle: event.previousTitle }; + } + case 'serverTitleChanged': + case 'sessionChanged': { + return { ...state, isModalOpen: false, optimisticTitle: null }; + } + default: { + return state; + } + } +} + +type SessionDetailRenameState = { + title: string; + isTitleInteractive: boolean; + modalInitialValue: string | null; + isModalOpen: boolean; +}; + +/** + * Pure helper that derives the session-detail header display state from the + * authoritative server title and the reducer state. + */ +export function getSessionDetailRenameState(input: { + fallbackTitle: string; + isLoaded: boolean; + serverTitle: string | undefined; + renameState: RenameState; +}): SessionDetailRenameState { + const baseTitle = input.isLoaded + ? (input.serverTitle ?? input.fallbackTitle) + : input.fallbackTitle; + const title = input.renameState.optimisticTitle ?? baseTitle; + return { + title, + isTitleInteractive: input.isLoaded, + modalInitialValue: input.renameState.isModalOpen ? title : null, + isModalOpen: input.renameState.isModalOpen, + }; +} diff --git a/apps/mobile/src/components/agents/use-session-detail-rename.ts b/apps/mobile/src/components/agents/use-session-detail-rename.ts new file mode 100644 index 0000000000..8bde6cfd19 --- /dev/null +++ b/apps/mobile/src/components/agents/use-session-detail-rename.ts @@ -0,0 +1,135 @@ +import { type KiloSessionId } from 'cloud-agent-sdk'; +import { useCallback, useEffect, useReducer, useRef } from 'react'; + +import { useSessionMutations } from '@/lib/hooks/use-session-mutations'; + +import { + getSessionDetailRenameState, + initialRenameState, + renameStateReducer, +} from './session-detail-rename-state'; + +type SessionDetailRenameApi = { + title: string; + isTitleInteractive: boolean; + isModalOpen: boolean; + modalInitialValue: string; + openModal: () => void; + closeModal: () => void; + /** + * Persist a new title. Awaits the shared `renameSessionAsync` so a + * rejection propagates back to the caller (typically `RenameModal`, + * which keeps the modal open and shows the inline error). The shared + * mutation hook owns the user-visible error toast and stored-list + * optimistic update/rollback/invalidation. + */ + submit: (next: string) => Promise; +}; + +type SessionDetailRenameInput = { + sessionId: KiloSessionId; + isLoaded: boolean; + serverTitle: string | undefined; + fallbackTitle: string; +}; + +/** + * Owns the rename modal and the optimistic header title for the session + * detail screen. The shared `useSessionMutations` hook continues to own + * the list-cache optimistic update/rollback, the user-visible error toast, + * per-session serialization, and list invalidation — this hook only + * mirrors the rename into the local header until the authoritative server + * title (or route change) catches up. + */ +export function useSessionDetailRename({ + sessionId, + isLoaded, + serverTitle, + fallbackTitle, +}: Readonly): SessionDetailRenameApi { + const { renameSessionAsync } = useSessionMutations(); + const [renameState, dispatch] = useReducer(renameStateReducer, initialRenameState()); + const lastSeenServerTitleRef = useRef(serverTitle); + + // Drop the optimistic override when the route's session changes so a + // previous screen's pending rename can't leak onto the next one. The + // component is keyed on the session in the parent, so a route change + // remounts this hook with a fresh ref and fresh state — this effect + // exists as a defensive reset for callers that reuse the instance. + useEffect(() => { + dispatch({ type: 'sessionChanged' }); + }, [sessionId]); + + // Sync the optimistic override only when the authoritative server title + // actually changes (e.g. the parent refetches and pushes a new prop). A + // stable but unrelated prop (failure case: server title stays the same) + // is intentionally ignored — failure is handled explicitly in `submit`. + useEffect(() => { + if (serverTitle === undefined) { + return; + } + if (lastSeenServerTitleRef.current === serverTitle) { + return; + } + lastSeenServerTitleRef.current = serverTitle; + dispatch({ type: 'serverTitleChanged' }); + }, [serverTitle]); + + const openModal = useCallback(() => { + dispatch({ type: 'openModal' }); + }, []); + + const closeModal = useCallback(() => { + dispatch({ type: 'closeModal' }); + }, []); + + const submit = useCallback( + async (next: string) => { + // RenameModal already enforces trim + non-empty + changed-input, so + // whatever reaches here is a real attempted rename. Snapshot the + // title currently shown in the header (optimistic override if present, + // otherwise the authoritative/fallback title) so a failure reverts to + // what the user actually saw, not a stale server title that may lag + // behind after a prior successful rename. + const previousTitle = getSessionDetailRenameState({ + fallbackTitle, + isLoaded, + serverTitle, + renameState, + }).title; + dispatch({ type: 'submit', nextTitle: next }); + try { + await renameSessionAsync(sessionId, next); + // Success: the optimistic title already set by the `submit` event + // stays in the header until the authoritative server title (or a + // route change) catches up. + } catch (error) { + // Restore the previously displayed title so the header visibly + // reverts. The mutation's own onError has already toasted and + // rolled back the list cache; we rethrow so RenameModal keeps the + // modal open and shows its inline error with the real failure + // message. + dispatch({ type: 'submitFailure', previousTitle }); + throw error; + } + }, + [fallbackTitle, isLoaded, renameSessionAsync, renameState, serverTitle, sessionId] + ); + + const state = getSessionDetailRenameState({ + fallbackTitle, + isLoaded, + serverTitle, + renameState, + }); + + return { + title: state.title, + isTitleInteractive: state.isTitleInteractive, + isModalOpen: state.isModalOpen, + modalInitialValue: state.modalInitialValue ?? state.title, + openModal, + closeModal, + submit, + }; +} diff --git a/apps/mobile/src/components/screen-header.tsx b/apps/mobile/src/components/screen-header.tsx index 18d911a461..40bb7684d4 100644 --- a/apps/mobile/src/components/screen-header.tsx +++ b/apps/mobile/src/components/screen-header.tsx @@ -20,6 +20,13 @@ type ScreenHeaderProps = { showBackButton?: boolean; onBack?: () => void; onTitlePress?: () => void; + /** + * Accessibility label for the pressable title. Defaults to a generic + * "Open menu" so list callers don't have to supply one. Detail screens + * (e.g. session rename) should override with a verb that describes the + * action, not "open menu". + */ + onTitlePressAccessibilityLabel?: string; backIcon?: 'back' | 'close'; /** Extra classes on the outer header container. Overrides the default `px-4` for screens that need a different horizontal inset. */ className?: string; @@ -34,6 +41,7 @@ export function ScreenHeader({ showBackButton, onBack, onTitlePress, + onTitlePressAccessibilityLabel, backIcon, className, }: Readonly) { @@ -66,7 +74,9 @@ export function ScreenHeader({ onPress={onTitlePress} hitSlop={13} accessibilityRole="button" - accessibilityLabel={title ? `Open menu for ${title}` : 'Open menu'} + accessibilityLabel={ + onTitlePressAccessibilityLabel ?? (title ? `Open menu for ${title}` : 'Open menu') + } className="flex-row items-center gap-1 active:opacity-70" > {titleText} diff --git a/apps/mobile/src/lib/hooks/use-session-mutations.test.ts b/apps/mobile/src/lib/hooks/use-session-mutations.test.ts new file mode 100644 index 0000000000..5f7e8f5660 --- /dev/null +++ b/apps/mobile/src/lib/hooks/use-session-mutations.test.ts @@ -0,0 +1,122 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useSessionMutations } from './use-session-mutations'; + +type MutationOptions = Record; +type TrpcMock = { + cliSessionsV2: { + list: { infiniteQueryKey: () => readonly unknown[] }; + recentRepositories: unknown; + rename: { mutationOptions: (opts: MutationOptions) => MutationOptions }; + delete: { mutationOptions: (opts: MutationOptions) => MutationOptions }; + }; +}; + +const mutationOptionsSpy = vi.fn<(opts: MutationOptions) => MutationOptions>(); +const capturedOptions: { current: MutationOptions | null } = { current: null }; +const mutateAsyncMock = vi.fn(); +const cancelQueriesMock = vi.fn(); +const getQueriesDataMock = vi.fn(); +const setQueriesDataMock = vi.fn(); +const setQueryDataMock = vi.fn(); +const invalidateQueriesMock = vi.fn(); +const invalidateAgentSessionsMock = vi.fn(); +const toastErrorMock = vi.fn(); +// eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule +const chainSaveMock = vi.fn((_id: string, op: () => Promise) => op()); + +const makeMutationOptions = (opts: MutationOptions) => { + mutationOptionsSpy(opts); + return opts; +}; + +vi.mock('@tanstack/react-query', () => ({ + useMutation: (opts: MutationOptions) => { + capturedOptions.current = opts; + return { mutateAsync: mutateAsyncMock }; + }, + useQueryClient: () => ({ + cancelQueries: cancelQueriesMock, + getQueriesData: getQueriesDataMock, + setQueriesData: setQueriesDataMock, + setQueryData: setQueryDataMock, + invalidateQueries: invalidateQueriesMock, + }), +})); + +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => + ({ + cliSessionsV2: { + list: { infiniteQueryKey: () => ['cliSessionsV2', 'list'] }, + recentRepositories: {}, + rename: { mutationOptions: makeMutationOptions }, + delete: { mutationOptions: makeMutationOptions }, + }, + }) satisfies TrpcMock, +})); + +vi.mock('@/lib/agent-session-cache', () => ({ + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + invalidateAgentSessionQueries: (...args: unknown[]) => { + invalidateAgentSessionsMock(...args); + return Promise.resolve(); + }, +})); + +vi.mock('sonner-native', () => ({ + toast: { error: (msg: string) => toastErrorMock(msg) }, +})); + +vi.mock('@/lib/hooks/save-chain', () => ({ + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + chainSave: (id: string, op: () => Promise) => chainSaveMock(id, op), +})); + +describe('useSessionMutations.renameSessionAsync', () => { + beforeEach(() => { + mutationOptionsSpy.mockClear(); + capturedOptions.current = null; + mutateAsyncMock.mockReset(); + cancelQueriesMock.mockReset(); + getQueriesDataMock.mockReset(); + setQueriesDataMock.mockReset(); + setQueryDataMock.mockReset(); + invalidateQueriesMock.mockReset(); + invalidateAgentSessionsMock.mockReset(); + toastErrorMock.mockReset(); + chainSaveMock.mockClear(); + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + chainSaveMock.mockImplementation((_id, op) => op()); + }); + + afterEach(() => { + vi.clearAllMocks(); + }); + + it('rejects after the mutation onError has toasted and rolled back list cache', async () => { + const error = new Error('rename failed'); + mutateAsyncMock.mockRejectedValueOnce(error); + getQueriesDataMock.mockReturnValue([]); + + const { renameSessionAsync } = useSessionMutations(); + await expect(renameSessionAsync('s1', 'New title')).rejects.toBe(error); + + expect(chainSaveMock).toHaveBeenCalledWith('s1', expect.any(Function)); + // The mutation's onError must run before the rejection propagates so the + // existing list-cache rollback and user-visible toast still fire. + const options = capturedOptions.current as { onError?: (err: unknown) => void } | null; + expect(options?.onError).toBeDefined(); + options?.onError?.(error); + expect(toastErrorMock).toHaveBeenCalledWith('rename failed'); + }); + + it('reuses the same rename mutation options as renameSession', () => { + // The detail hook relies on the async variant being backed by the exact + // same mutation (and therefore the same onError/onSettled wiring) as + // the list's fire-and-forget variant. + const { renameSessionAsync } = useSessionMutations(); + void renameSessionAsync; + expect(mutationOptionsSpy).toHaveBeenCalled(); + }); +}); diff --git a/apps/mobile/src/lib/hooks/use-session-mutations.ts b/apps/mobile/src/lib/hooks/use-session-mutations.ts index 075040c224..16459cff77 100644 --- a/apps/mobile/src/lib/hooks/use-session-mutations.ts +++ b/apps/mobile/src/lib/hooks/use-session-mutations.ts @@ -75,6 +75,12 @@ export function useSessionMutations() { // overwrite a newer one's result. Rename goes through a modal confirm and // delete through Alert.alert, so an adjacent double-fire of the same op // is already impossible — no dedupe needed here. + // + // `renameSession` is the list's fire-and-forget caller. Detail callers + // (e.g. the session detail header) use `renameSessionAsync`, which awaits + // the same mutation + chain so a rejection surfaces the existing toast, + // rolls back the list cache, and lets the caller keep its modal open for + // retry. return { deleteSession: (sessionId: string) => { void (async () => { @@ -100,5 +106,14 @@ export function useSessionMutations() { } })(); }, + renameSessionAsync: async (sessionId: string, title: string) => { + // The mutation's onError toasts and rolls back the list cache before + // this rejection propagates, so callers can rethrow to keep their + // modal open without duplicating user-visible error handling. + // eslint-disable-next-line typescript-eslint/promise-function-async -- conflicting require-await rule + await chainSave(sessionId, () => + renameSessionMutation.mutateAsync({ session_id: sessionId, title }) + ); + }, }; }