From 1b8ab24b087dbcf4d159ee27f013eca85bf6ed14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Igor=20=C5=A0=C4=87eki=C4=87?= Date: Wed, 16 Sep 2026 01:08:28 +0200 Subject: [PATCH] fix(mobile): make the session context circle tappable in every state https://github.com/Kilo-Org/cloud/pull/6190 --- .../agents/context-sheet-mount-state.test.ts | 31 +++--- .../agents/context-usage-display.ts | 34 +++++-- .../agents/session-auto-approve.test.ts | 33 ++++++- .../components/agents/session-auto-approve.ts | 29 +++++- .../agents/session-context-metrics.test.ts | 96 +++++++++++++++---- .../agents/session-context-metrics.tsx | 28 ++++-- .../agents/session-detail-content.test.ts | 58 +++++++++++ .../agents/session-detail-content.tsx | 32 ++++--- 8 files changed, 274 insertions(+), 67 deletions(-) diff --git a/apps/mobile/src/components/agents/context-sheet-mount-state.test.ts b/apps/mobile/src/components/agents/context-sheet-mount-state.test.ts index 76bc9d76ee1..da3dbdb2c95 100644 --- a/apps/mobile/src/components/agents/context-sheet-mount-state.test.ts +++ b/apps/mobile/src/components/agents/context-sheet-mount-state.test.ts @@ -40,33 +40,42 @@ describe('getContextSheetMountState', () => { ).toEqual({ mounted: true, visible: false, info: undefined }); }); - it('unmounts the no-usage sheet when permission controls become unavailable', () => { + it('keeps an opened no-usage sheet mounted for a session without permission controls', () => { expect( getContextSheetMountState( undefined, { sessionId: 'current-session' }, { sessionId: 'current-session', autoApproveAvailable: false } ) - ).toEqual({ mounted: false }); + ).toEqual({ mounted: true, visible: true, info: undefined }); }); - it('unmounts when there is no context info regardless of open state', () => { - expect(getContextSheetMountState(undefined, null, { sessionId: 'current-session' })).toEqual({ - mounted: false, - }); + it('stays open when usage arrives after opening a session without permission controls', () => { + const session = { sessionId: 'current-session', autoApproveAvailable: false }; + expect(getContextSheetMountState(undefined, { sessionId: 'current-session' }, session)).toEqual( + { mounted: true, visible: true, info: undefined } + ); + expect( + getContextSheetMountState(currentInfo, { sessionId: 'current-session' }, session) + ).toEqual({ mounted: true, visible: true, info: currentInfo }); + }); + + it('does not open an unmounted sheet for another session', () => { expect( getContextSheetMountState( undefined, - { - sessionId: 'current-session', - providerID: currentInfo.providerID, - modelID: currentInfo.modelID, - }, + { sessionId: 'previous-session' }, { sessionId: 'current-session' } ) ).toEqual({ mounted: false }); }); + it('unmounts when there is no context info and the sheet was never opened', () => { + expect(getContextSheetMountState(undefined, null, { sessionId: 'current-session' })).toEqual({ + mounted: false, + }); + }); + it('mounts visible when context info exists and its identity is open', () => { const result = getContextSheetMountState( currentInfo, diff --git a/apps/mobile/src/components/agents/context-usage-display.ts b/apps/mobile/src/components/agents/context-usage-display.ts index 05cb696c67b..9cdc9b741f9 100644 --- a/apps/mobile/src/components/agents/context-usage-display.ts +++ b/apps/mobile/src/components/agents/context-usage-display.ts @@ -227,10 +227,33 @@ export type ContextSheetIdentity = { modelID?: string; }; +/** + * Whether an open request still matches the model usage currently reports. + * + * An open request recorded before the first usage report carries no model, and + * must keep matching whatever model usage later names — otherwise the sheet + * dismisses itself on arrival of the very report it was opened to read. An + * open request that did record a model stops matching once usage names another. + */ +function isOpenIdentityForModel( + info: SessionContextInfo | undefined, + openIdentity: ContextSheetIdentity | null +): boolean { + const providerID = openIdentity?.providerID; + const modelID = openIdentity?.modelID; + if (providerID === undefined || modelID === undefined) { + return true; + } + return providerID === info?.providerID && modelID === info.modelID; +} + /** * Controls when the native Modal is mounted and when it is visible. Keeping - * the sheet mounted while usage or permission controls are available lets - * `visible` transition from true → false for native dismissal. Permission + * the sheet mounted after it has been opened lets `visible` transition from + * true → false for native dismissal. The sheet is the session's own + * context/permission surface — it always has the session identity and the + * auto-approve row to show — so an open request mounts it even before usage is + * reported or permission controls are known to be available. Permission * controls belong to the session, not the model reporting the latest usage. */ export function getContextSheetMountState( @@ -238,12 +261,11 @@ export function getContextSheetMountState( openIdentity: ContextSheetIdentity | null, { sessionId, autoApproveAvailable = false }: { sessionId: string; autoApproveAvailable?: boolean } ): SheetMountState { - if (!info && !autoApproveAvailable) { + const openedForSession = openIdentity?.sessionId === sessionId; + if (!info && !autoApproveAvailable && !openedForSession) { return { mounted: false }; } const visible = - openIdentity?.sessionId === sessionId && - (autoApproveAvailable || - (openIdentity.providerID === info?.providerID && openIdentity.modelID === info?.modelID)); + openedForSession && (autoApproveAvailable || isOpenIdentityForModel(info, openIdentity)); return { mounted: true, visible, info }; } diff --git a/apps/mobile/src/components/agents/session-auto-approve.test.ts b/apps/mobile/src/components/agents/session-auto-approve.test.ts index cfc7ac7e808..2f0ee32034e 100644 --- a/apps/mobile/src/components/agents/session-auto-approve.test.ts +++ b/apps/mobile/src/components/agents/session-auto-approve.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from 'vitest'; import { canAutoApprovePermissions, + canAutoApproveReply, clearSessionAutoApprove, getSessionAutoApproveEnabled, MAX_REMEMBERED_REQUEST_IDS, @@ -23,11 +24,18 @@ describe('canAutoApprovePermissions', () => { ); }); - it('rejects the read-only transport and an unresolved transport', () => { + it('rejects the read-only transport', () => { expect(canAutoApprovePermissions({ activeSessionType: 'read-only', isReadOnly: false })).toBe( false ); - expect(canAutoApprovePermissions({ activeSessionType: null, isReadOnly: false })).toBe(false); + }); + + // The toggle is a client-side per-session preference: the transport resolves + // only after the metadata and transcript, so the settings must stay usable + // while the session is still opening (or has failed to open). Only a session + // known to be read-only is excluded. + it('allows an unresolved transport so the session settings stay reachable', () => { + expect(canAutoApprovePermissions({ activeSessionType: null, isReadOnly: false })).toBe(true); }); it('rejects every transport when the session is read-only', () => { @@ -44,6 +52,27 @@ describe('canAutoApprovePermissions', () => { }); }); +describe('canAutoApproveReply', () => { + it('allows a resolved remote or cloud-agent transport', () => { + expect(canAutoApproveReply({ activeSessionType: 'remote', isReadOnly: false })).toBe(true); + expect(canAutoApproveReply({ activeSessionType: 'cloud-agent', isReadOnly: false })).toBe(true); + }); + + // The settings row stays reachable while the transport is unresolved, but an + // unresolved transport cannot deliver a permission ask, so it must not make + // an auto-reply eligible. + it('rejects an unresolved transport even though its settings stay reachable', () => { + expect(canAutoApprovePermissions({ activeSessionType: null, isReadOnly: false })).toBe(true); + expect(canAutoApproveReply({ activeSessionType: null, isReadOnly: false })).toBe(false); + }); + + it('rejects a read-only session', () => { + expect(canAutoApproveReply({ activeSessionType: 'read-only', isReadOnly: false })).toBe(false); + expect(canAutoApproveReply({ activeSessionType: 'remote', isReadOnly: true })).toBe(false); + expect(canAutoApproveReply({ activeSessionType: null, isReadOnly: true })).toBe(false); + }); +}); + describe('resolveSessionAutoApproveState', () => { it('returns unavailable whenever the session is not available', () => { expect(resolveSessionAutoApproveState({ enabled: false, available: false })).toBe( diff --git a/apps/mobile/src/components/agents/session-auto-approve.ts b/apps/mobile/src/components/agents/session-auto-approve.ts index bb345e14c6a..707256b9364 100644 --- a/apps/mobile/src/components/agents/session-auto-approve.ts +++ b/apps/mobile/src/components/agents/session-auto-approve.ts @@ -20,9 +20,15 @@ export type SessionAutoApproveState = 'on' | 'off' | 'unavailable'; export type SessionAutoApproveSessionType = 'remote' | 'cloud-agent' | 'read-only' | null; /** - * A session can auto-approve permissions only when its transport can answer a - * permission ask. The read-only transport cannot send, and an unresolved - * transport (`null`) is not assumed answerable. + * Whether the session's permission settings may be used. + * + * The auto-approve toggle is a client-side, per-session preference, so it must + * be usable while the session is still opening: the transport resolves only + * after the session metadata and transcript, and a session whose transcript is + * still loading (or failed to load) still owns its settings. Treating the + * unresolved transport (`null`) as unavailable hid the only way into the + * context sheet for exactly that phase. A session known to be read-only can + * never receive a permission ask, so it stays unavailable. */ export function canAutoApprovePermissions(input: { activeSessionType: SessionAutoApproveSessionType; @@ -31,7 +37,22 @@ export function canAutoApprovePermissions(input: { if (input.isReadOnly) { return false; } - return input.activeSessionType === 'remote' || input.activeSessionType === 'cloud-agent'; + return input.activeSessionType !== 'read-only'; +} + +/** + * Whether the session can actually answer a permission ask, which is what + * makes an auto-reply eligible. + * + * Distinct from {@link canAutoApprovePermissions}: the settings row must stay + * reachable while the transport is unresolved, but an unresolved transport + * cannot deliver an ask, so it must never make an auto-reply eligible. + */ +export function canAutoApproveReply(input: { + activeSessionType: SessionAutoApproveSessionType; + isReadOnly: boolean; +}): boolean { + return input.activeSessionType !== null && canAutoApprovePermissions(input); } /** Resolve the row state: unavailable wins over the stored enabled flag. */ diff --git a/apps/mobile/src/components/agents/session-context-metrics.test.ts b/apps/mobile/src/components/agents/session-context-metrics.test.ts index 8c15f52b7a9..4ce147fa3ef 100644 --- a/apps/mobile/src/components/agents/session-context-metrics.test.ts +++ b/apps/mobile/src/components/agents/session-context-metrics.test.ts @@ -114,15 +114,86 @@ describe('SessionContextMetrics', () => { expect(onPress).toHaveBeenCalledOnce(); }); - it('does not expose permission controls while the session is loading', () => { + // The header control is the only way to the session's permission settings, + // so it must register a tap while the page is still loading instead of + // rendering a dead, invisible reserved box. + it('keeps the context control tappable while the session page loads', () => { + const onPress = vi.fn<() => void>(); + const root = render({ + info: undefined, + totalCostMicrodollars: null, + hasMessages: false, + autoApproveAvailable: true, + loading: true, + onPress, + }); + expect(root.type).toBe('Pressable'); + const props = root.props as { + accessibilityRole?: string; + onPress?: () => void; + className?: string; + }; + expect(props.accessibilityRole).toBe('button'); + expect(props.className ?? '').not.toContain('opacity-0'); + props.onPress?.(); + expect(onPress).toHaveBeenCalledOnce(); + }); + + // Usage is already known, so the control must be live instead of an invisible + // zero-size hit area. + it('keeps the context control tappable for an empty session that reports usage', () => { + const onPress = vi.fn<() => void>(); + const root = render({ + info: info(), + totalCostMicrodollars: 150_000, + hasMessages: false, + onPress, + }); + expect(root.type).toBe('Pressable'); + const props = root.props as { + accessibilityRole?: string; + onPress?: () => void; + className?: string; + }; + expect(props.accessibilityRole).toBe('button'); + expect(props.className ?? '').not.toContain('opacity-0'); + props.onPress?.(); + expect(onPress).toHaveBeenCalledOnce(); + }); + + // Usage and permission controls are both still unknown while a session + // opens (or after a failed open), but the parent can still open the sheet, + // so the control must register a tap instead of a dead reserved box. + it('keeps the context control tappable with no usage and no permission controls', () => { + const onPress = vi.fn<() => void>(); + const root = render({ + info: undefined, + totalCostMicrodollars: null, + hasMessages: false, + autoApproveAvailable: false, + onPress, + }); + expect(root.type).toBe('Pressable'); + const props = root.props as { + accessibilityRole?: string; + accessibilityLabel?: string; + onPress?: () => void; + className?: string; + }; + expect(props.accessibilityRole).toBe('button'); + expect(props.accessibilityLabel).toContain('Tap to view context details.'); + expect(props.className ?? '').not.toContain('opacity-0'); + props.onPress?.(); + expect(onPress).toHaveBeenCalledOnce(); + }); + + it('does not expose permission controls while the session is loading and no sheet can open', () => { expectHiddenReservedBox( render({ info: undefined, totalCostMicrodollars: null, - hasMessages: true, - autoApproveAvailable: true, + hasMessages: false, loading: true, - onPress: vi.fn<() => void>(), }) ); }); @@ -250,21 +321,4 @@ describe('SessionContextMetrics', () => { } expect((ring.props as { arcFraction?: number }).arcFraction).toBe(0); }); - - it('empty session with interactive info and onPress stays a hidden reserved view', () => { - const onPress = vi.fn(() => undefined); - const root = render({ - hasMessages: false, - info: info(), - totalCostMicrodollars: 150_000, - onPress, - }); - expectHiddenReservedBox(root); - const props = root.props as { - onPress?: () => void; - accessibilityRole?: string; - }; - expect(props.onPress).toBeUndefined(); - expect(props.accessibilityRole).not.toBe('button'); - }); }); diff --git a/apps/mobile/src/components/agents/session-context-metrics.tsx b/apps/mobile/src/components/agents/session-context-metrics.tsx index daf8b705172..82282171b69 100644 --- a/apps/mobile/src/components/agents/session-context-metrics.tsx +++ b/apps/mobile/src/components/agents/session-context-metrics.tsx @@ -14,13 +14,18 @@ import { type SessionContextMetricsProps = { info: SessionContextInfo | undefined; totalCostMicrodollars: number | null; - /** Empty sessions reserve the pill's layout unless auto-approve is available. */ + /** + * Empty sessions reserve the pill's layout box. `onPress` decides whether the + * pill is live — the parent passes it whenever the context sheet can open — + * so the reserved box never swallows a tap. + */ hasMessages: boolean; autoApproveAvailable?: boolean; onPress?: () => void; /** - * Hide the pill while the session page loads; the layout box stays reserved so the header does not - * shift. + * Marks the unresolved-session placeholder. The layout box stays reserved so + * the header does not shift; the pill itself stays hidden only while there is + * nothing to open. */ loading?: boolean; }; @@ -48,12 +53,17 @@ export function SessionContextMetrics({ loading = false, }: Readonly) { const content = getHeaderPillContent({ info, totalCostMicrodollars, hasMessages }); - // Permission controls must be reachable before the first tool finishes and - // reports usage. Other empty sessions keep their reserved, hidden layout box. - const hidden = loading || (!hasMessages && !autoApproveAvailable); - // Single source for element kind and a11y affordance wording so a future - // caller with interactive content but no onPress cannot advertise a tap. - const pressable = !hidden && (content.interactive || autoApproveAvailable) && onPress != null; + // This pill is the only way into the session's context sheet, which owns the + // session's permission settings (auto-approve). The parent provides `onPress` + // whenever the sheet can open for this session, so pressability follows that + // and never the page's load phase or whether usage has arrived: a session + // whose transcript is still loading — or failed to load — must not leave its + // settings behind a control that looks dead or an invisible tap target. + const pressable = onPress != null; + // Without an `onPress` this is the unresolved-session placeholder: the layout + // box keeps its space in every state, and stays invisible while there is + // nothing to open. + const hidden = !pressable && (loading || (!hasMessages && !autoApproveAvailable)); const accessibilityLabel = getMetricsAccessibilityLabel({ info, totalCostMicrodollars, diff --git a/apps/mobile/src/components/agents/session-detail-content.test.ts b/apps/mobile/src/components/agents/session-detail-content.test.ts index 9291adb2720..6311ffaef14 100644 --- a/apps/mobile/src/components/agents/session-detail-content.test.ts +++ b/apps/mobile/src/components/agents/session-detail-content.test.ts @@ -1042,6 +1042,64 @@ describe('session detail per-session auto-approve', () => { expect(hapticsSelection).toHaveBeenCalledTimes(2); }); + it('opens the header sheet while an answerable session is still loading', async () => { + const view = await mountDetails([]); + act(() => { + view.store.set(view.manager.atoms.activeSessionType, 'remote'); + view.store.set(view.manager.atoms.isReadOnly, false); + // No message has landed yet: the header control is the only way to the + // session's permission settings, so it must still register a tap. + view.store.set(view.manager.atoms.isLoading, true); + }); + + const metrics = view.renderer.root.findByProps({ testID: 'session-context-metrics' }); + const metricsProps = metrics.props as { + accessibilityRole?: string; + onPress?: () => void; + }; + expect(metricsProps.accessibilityRole).toBe('button'); + act(() => { + metricsProps.onPress?.(); + }); + const contextSheet = view.renderer.root.findByType(SessionContextSheet); + expect(contextSheet.props.visible).toBe(true); + expect(view.renderer.root.findByProps({ testID: 'session-auto-approve-switch' })).toBeDefined(); + }); + + it('opens the header sheet with usable settings after a failed open left the transport unresolved', async () => { + const view = await mountDetails([]); + act(() => { + // A failed session open leaves the transport unresolved and puts the + // screen on its terminal error. The settings still live behind the + // header control, so it must open the sheet instead of going dead. + view.store.set< + 'cloud-agent' | 'read-only' | 'remote' | null, + ['cloud-agent' | 'read-only' | 'remote' | null], + unknown + >(view.manager.atoms.activeSessionType, null); + view.store.set(view.manager.atoms.isReadOnly, false); + view.store.set(view.manager.atoms.isLoading, false); + view.store.set( + view.manager.atoms.error, + 'connect ECONNREFUSED 127.0.0.1:12000' + ); + }); + + const metrics = view.renderer.root.findByProps({ testID: 'session-context-metrics' }); + const metricsProps = metrics.props as { + accessibilityRole?: string; + onPress?: () => void; + }; + expect(metricsProps.accessibilityRole).toBe('button'); + act(() => { + metricsProps.onPress?.(); + }); + const contextSheet = view.renderer.root.findByType(SessionContextSheet); + expect(contextSheet.props.visible).toBe(true); + const toggle = view.renderer.root.findByProps({ testID: 'session-auto-approve-switch' }); + expect((toggle.props as { disabled?: boolean }).disabled).toBe(false); + }); + it('keeps the composer mounted, visible, and enabled while the auto-reply is in flight', async () => { const view = await mountDetails([]); makeSessionAnswerable(view); diff --git a/apps/mobile/src/components/agents/session-detail-content.tsx b/apps/mobile/src/components/agents/session-detail-content.tsx index d1edeb04774..747fc5e24bb 100644 --- a/apps/mobile/src/components/agents/session-detail-content.tsx +++ b/apps/mobile/src/components/agents/session-detail-content.tsx @@ -65,6 +65,7 @@ import { SessionContextMetrics } from '@/components/agents/session-context-metri import { SessionContextSheet } from '@/components/agents/session-context-sheet'; import { canAutoApprovePermissions, + canAutoApproveReply, resolveSessionAutoApproveState, setSessionAutoApproveEnabled, useSessionAutoApproveEnabled, @@ -271,10 +272,13 @@ export function SessionDetailContent({ const pendingMessages = useAtomValue(manager.atoms.pendingMessages); const activeSessionType = useAtomValue(manager.atoms.activeSessionType); // Per-session auto-approve lives in an in-memory store keyed by session id. - // Availability follows the transport (and the read-only flag); an unresolved - // transport is unavailable, so the row shows disabled without a fetch. + // The setting stays reachable while the transport is unresolved (metadata and + // the transcript resolve after it), so only a session known to be read-only + // shows it unavailable. Auto-reply eligibility is separate: an unresolved + // transport cannot deliver a permission ask. const autoApproveEnabled = useSessionAutoApproveEnabled(sessionId); const autoApproveAvailable = canAutoApprovePermissions({ activeSessionType, isReadOnly }); + const autoApproveReplyAvailable = canAutoApproveReply({ activeSessionType, isReadOnly }); const remoteModelState = useAtomValue(manager.atoms.remoteModelState); const observedModel = useAtomValue(manager.atoms.observedModel); const remoteModelOverride = useAtomValue(manager.atoms.remoteModelOverride); @@ -358,7 +362,7 @@ export function SessionDetailContent({ // reach the hook, so a clarification question is never auto-answered. const { suppressedRequestId } = useSessionAutoApprove({ enabled: autoApproveState === 'on', - available: autoApproveAvailable, + available: autoApproveReplyAvailable, requestId: activePermission?.requestId ?? null, respond: async () => { const outcome = await handleRespondToPermission('once'); @@ -1369,17 +1373,17 @@ export function SessionDetailContent({ hasMessages={messages.length > 0} autoApproveAvailable={autoApproveAvailable} loading={shouldShowLoading} - onPress={ - contextInfo || autoApproveAvailable - ? () => { - setOpenContextSheetIdentity({ - sessionId, - providerID: contextInfo?.providerID, - modelID: contextInfo?.modelID, - }); - } - : undefined - } + // The sheet is the session's own context/permission surface, so the + // control opens it in every state of this screen — including while the + // transcript is still loading and after a failed open. A state that + // hides the control locks the user out of the settings behind it. + onPress={() => { + setOpenContextSheetIdentity({ + sessionId, + providerID: contextInfo?.providerID, + modelID: contextInfo?.modelID, + }); + }} /> );