diff --git a/apps/mobile/src/components/agents/new-session-configure-form.tsx b/apps/mobile/src/components/agents/new-session-configure-form.tsx index a339f3dcb3..595d50e2d9 100644 --- a/apps/mobile/src/components/agents/new-session-configure-form.tsx +++ b/apps/mobile/src/components/agents/new-session-configure-form.tsx @@ -333,8 +333,12 @@ export function NewSessionConfigureForm({ ); return ( + // The root reserves the navigation-bar inset, so the keyboard-lift view + // pads from its own bottom edge and must not add the inset again. - {body} + + {body} + ); } diff --git a/apps/mobile/src/components/agents/session-detail-content.tsx b/apps/mobile/src/components/agents/session-detail-content.tsx index a80bf99318..0fc3f3d79f 100644 --- a/apps/mobile/src/components/agents/session-detail-content.tsx +++ b/apps/mobile/src/components/agents/session-detail-content.tsx @@ -1921,7 +1921,9 @@ export function SessionDetailContent({ {keepScreenAwake ? : null} {keyboardContainerKind === 'app-aware-padding' ? ( - + // The trailing bottom-chrome spacer below reserves the navigation- + // bar inset outside this view, so the view must not add it again. + {renderKeyboardBody()} ) : ( diff --git a/apps/mobile/src/components/app-root-providers.toaster-a11y.mounted.test.tsx b/apps/mobile/src/components/app-root-providers.toaster-a11y.mounted.test.tsx index ca7d460ad0..57bb972769 100644 --- a/apps/mobile/src/components/app-root-providers.toaster-a11y.mounted.test.tsx +++ b/apps/mobile/src/components/app-root-providers.toaster-a11y.mounted.test.tsx @@ -1,12 +1,35 @@ import { type ElementType } from 'react'; -import { afterEach, beforeEach, expect, it } from 'vitest'; +import type * as AppAwareKeyboardPadding from '@/components/kilo-chat/app-aware-keyboard-padding'; +import { act } from '@/test/renderer'; +import { afterEach, beforeEach, expect, it, vi } from 'vitest'; import { + keyboard, mount, + platform, resetUnlockMocks, + route, unlockRoot, unmountUnlock, } from '@/components/app-unlock-screen.test-helpers'; +import { getEffectiveTabBarHeight } from '@/lib/tab-bar-layout'; +import { MIN_BOTTOM_CHROME_HEIGHT, TOAST_BOTTOM_GAP } from '@/lib/toast-offset'; + +// One keyboard read for the whole app: the Toaster must import the hook the +// screens reserve padding with, not run its own listener pair, or the toast's +// height can drift from theirs. The counter is a plain object so +// `resetUnlockMocks` (which resets every `vi.fn`) cannot clear it. +const sharedKeyboardHook = vi.hoisted(() => ({ calls: 0 })); +vi.mock('@/components/kilo-chat/app-aware-keyboard-padding', async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + useAppAwareKeyboardPadding: () => { + sharedKeyboardHook.calls += 1; + return actual.useAppAwareKeyboardPadding(); + }, + }; +}); beforeEach(resetUnlockMocks); afterEach(unmountUnlock); @@ -31,3 +54,169 @@ it('anchors the toast container to the window so toasts reach the accessibility expect(toasters).toHaveLength(1); expect(toasters[0]?.props.positionerStyle).toEqual({ top: 0 }); }); + +/** + * The safe-area inset is not a reliable floor for the bottom chrome: on Android + * it is only `navigationBars()`, it does not grow while the IME's navigation + * row is on screen, and it can be reported as `0`. A toast anchored to the + * inset alone had its last line clipped under that chrome (2026-09-18 device + * finding), so the offset is floored at one shared bottom-chrome height for + * both platforms — no platform branch. + */ +it('floors the toast offset at the shared bottom-chrome height', async () => { + platform.OS = 'android'; + // Default route: a screen pushed over the tabs, so no tab bar is on screen + // and the bottom-chrome floor decides the offset. + await mount(); + + const toasters = unlockRoot().findAllByType('Toaster' as ElementType); + + // The mocked inset (12) is below the floor, so the floor decides the offset. + expect(toasters[0]?.props.offset).toBe(MIN_BOTTOM_CHROME_HEIGHT + TOAST_BOTTOM_GAP); +}); + +/** + * The floating tab bar is an absolute overlay over the screen bottom: the + * reported bottom inset does not include it, and a toast anchored to the + * inset landed over the tab icons (2026-09-19 visual spot check, p1 — the + * manual-review error toast covered the navigation row). While a tab screen + * is on top, the offset must clear the bar's full rendered height. + */ +it('clears the floating tab bar while a tab screen is on top', async () => { + platform.OS = 'android'; + route.segments = ['(app)', '(tabs)', '(3_profile)', 'code-reviewer', 'personal', 'manual-review']; + route.pathname = '/code-reviewer/personal/manual-review'; + await mount(); + + const toasters = unlockRoot().findAllByType('Toaster' as ElementType); + + // Same predicate and height source as the tab bar's own layout + // (`(tabs)/_layout.tsx`), so the toast clears whatever the bar renders. + const tabOverlayHeight = getEffectiveTabBarHeight({ + bottomInset: 12, + platform: 'android', + fontScale: 1, + }); + expect(toasters[0]?.props.offset).toBe(tabOverlayHeight + TOAST_BOTTOM_GAP); +}); + +/** + * The bar's own layout hides it on two in-tab routes (`shouldHideTabBar`); + * there the toast must fall back to the resting offset instead of floating + * over a bar that is not there. + */ +it('keeps the resting offset when the route hides the tab bar', async () => { + platform.OS = 'android'; + route.segments = ['(app)', '(tabs)', '(1_kiloclaw)', 'chat', 'sandbox', 'conversation']; + route.pathname = '/chat/sandbox/conversation'; + await mount(); + + const toasters = unlockRoot().findAllByType('Toaster' as ElementType); + + expect(toasters[0]?.props.offset).toBe(MIN_BOTTOM_CHROME_HEIGHT + TOAST_BOTTOM_GAP); +}); + +/** + * The keyboard read lives in one hook (`useAppAwareKeyboardPadding`) so the + * padding view and the reveal hook cannot disagree with the toast. The Toaster + * imports that hook instead of running its own `Keyboard`/`AppState` listener + * pair; this test fails if a second implementation grows back here. + */ +it('reads the keyboard height through the shared app-aware hook', async () => { + sharedKeyboardHook.calls = 0; + + await mount(); + + expect(sharedKeyboardHook.calls).toBeGreaterThan(0); +}); + +/** + * Android's `endCoordinates.height` stops at the navigation bar + * (`ReactRootView` sends `imeInsets.bottom − barInsets.bottom`), so the raw + * height sits below the IME's true top edge by the bar inset. The toast is + * anchored to the screen bottom, so the Toaster resolves the occlusion through + * the same rule the screens reserve padding with (`resolveKeyboardBottomPadding`) + * — a raw height left the toast's last line behind the IME's navigation row + * (2026-09-20 review finding). The mocked bottom inset is 12. + */ +it('clears the Android IME navigation row by adding the bottom inset', async () => { + platform.OS = 'android'; + await mount(); + + act(() => { + keyboard.show({ endCoordinates: { height: 300 } }); + }); + + const toasters = unlockRoot().findAllByType('Toaster' as ElementType); + + expect(toasters[0]?.props.offset).toBe(300 + 12 + TOAST_BOTTOM_GAP); +}); + +/** + * iOS reports the keyboard window frame, which reaches the screen bottom and + * so already includes the home-indicator inset. Adding the bottom inset there + * would float the toast above the keyboard, so the iOS height passes through + * unchanged — the platform-parity half of the keyboard rule. + */ +it('keeps the iOS keyboard height, which already reaches the screen bottom', async () => { + platform.OS = 'ios'; + await mount(); + + act(() => { + keyboard.show({ endCoordinates: { height: 300 } }); + }); + + const toasters = unlockRoot().findAllByType('Toaster' as ElementType); + + expect(toasters[0]?.props.offset).toBe(300 + TOAST_BOTTOM_GAP); +}); + +/** + * The harness keeps every keyboard subscriber, one set per direction (see the + * Keyboard mock in the test helpers). A second consumer — here an extra + * listener standing in for a screen on top of the Toaster — must not shadow the + * Toaster's listener, and disposing it must not detach the Toaster's. A single + * slot per direction failed both halves. + */ +it('delivers a keyboard event to every subscriber and detaches only the disposed one', async () => { + platform.OS = 'android'; + await mount(); + + const extra = vi.fn((_event: { endCoordinates: { height: number } }) => undefined); + const subscription = keyboard.addListener('keyboardDidShow', extra); + + act(() => { + keyboard.show({ endCoordinates: { height: 300 } }); + }); + // Both the Toaster's hook and the extra subscriber received the height. + expect(extra).toHaveBeenCalledWith({ endCoordinates: { height: 300 } }); + expect(unlockRoot().findAllByType('Toaster' as ElementType)[0]?.props.offset).toBe( + 300 + 12 + TOAST_BOTTOM_GAP + ); + + subscription.remove(); + act(() => { + keyboard.show({ endCoordinates: { height: 240 } }); + }); + // The disposed subscriber is gone; the Toaster's listener is still attached. + expect(extra).toHaveBeenCalledTimes(1); + expect(unlockRoot().findAllByType('Toaster' as ElementType)[0]?.props.offset).toBe( + 240 + 12 + TOAST_BOTTOM_GAP + ); +}); + +/** + * The same rule runs on iOS: the offset module reads no platform, so the + * resting offset is the shared bottom-chrome floor plus the standard gap, not + * the raw iOS inset (12 here). This is the platform-parity assertion for the + * toast path — if a per-platform branch grows back, one of the two platforms + * stops matching the shared floor. + */ +it('uses the same bottom-chrome floor on iOS', async () => { + platform.OS = 'ios'; + await mount(); + + const toasters = unlockRoot().findAllByType('Toaster' as ElementType); + + expect(toasters[0]?.props.offset).toBe(MIN_BOTTOM_CHROME_HEIGHT + TOAST_BOTTOM_GAP); +}); diff --git a/apps/mobile/src/components/app-root-providers.tsx b/apps/mobile/src/components/app-root-providers.tsx index 146598dcbc..973ae8e748 100644 --- a/apps/mobile/src/components/app-root-providers.tsx +++ b/apps/mobile/src/components/app-root-providers.tsx @@ -1,13 +1,18 @@ import { ActionSheetProvider } from '@expo/react-native-action-sheet'; import { PortalHost } from '@rn-primitives/portal'; import { QueryClientProvider } from '@tanstack/react-query'; +import { usePathname, useSegments } from 'expo-router'; import { CheckCircle2, Info, Loader, TriangleAlert, XCircle } from '@/components/ui/icons'; import { type ReactNode } from 'react'; +import { Platform, useWindowDimensions } from 'react-native'; import { GestureHandlerRootView } from 'react-native-gesture-handler'; +import { useSafeAreaInsets } from 'react-native-safe-area-context'; import { Toaster } from 'sonner-native'; import { useTranslation } from 'react-i18next'; import { AppUnlockAnnouncements } from '@/components/app-unlock-screen'; +import { useAppAwareKeyboardPadding } from '@/components/kilo-chat/app-aware-keyboard-padding'; +import { resolveKeyboardBottomPadding } from '@/components/login-screen-state'; import { OfflineBanner } from '@/components/offline-banner'; import { AppUnlockProvider } from '@/lib/app-unlock-context'; import { AuthProvider } from '@/lib/auth/auth-context'; @@ -16,6 +21,8 @@ import { OrganizationProvider } from '@/lib/organization-context'; import { queryClient } from '@/lib/query-client'; import { QueryClientNativeLifecycle } from '@/lib/query-client-lifecycle'; import { ToolSummaryTranslationRuntimeBootstrap } from '@/lib/tool-summary-translation/tool-summary-translation-preference'; +import { getEffectiveTabBarHeight, shouldHideTabBar } from '@/lib/tab-bar-layout'; +import { getToastBottomOffset } from '@/lib/toast-offset'; import { trpcClient, TRPCProvider } from '@/lib/trpc'; /** @@ -44,7 +51,6 @@ export function AppRootProviders({ readonly children: ReactNode; readonly languageReady: boolean; }) { - const colors = useThemeColors(); const { t } = useTranslation(); return ( @@ -79,26 +85,7 @@ export function AppRootProviders({ lifetime (spot check e4-end). Bottom is the transient-message convention: a toast may cover the composer briefly, never the navigation. */} - , - error: , - warning: , - info: , - loading: , - }} - toastOptions={{ - style: { - backgroundColor: colors.card, - borderColor: colors.border, - borderWidth: 1, - }, - titleStyle: { color: colors.foreground }, - descriptionStyle: { color: colors.mutedForeground }, - }} - /> + @@ -109,3 +96,91 @@ export function AppRootProviders({ ); } + +/** + * The Toaster reads the keyboard through the shared + * `useAppAwareKeyboardPadding` hook, so its height cannot drift from the one + * the screens reserve. It is called here, in a child of `AppRootProviders`, + * so a keyboard show/hide re-renders only the Toaster, never the app tree the + * provider wraps. + * + * Android needs this even though the app is edge-to-edge: under API 35+ the + * window never resizes for the IME (`login-screen.tsx`), and + * `react-native-safe-area-context` does not report IME insets, so a + * bottom-anchored overlay has no other way to clear the keyboard and its + * navigation row. + * + * The offset is one platform-free rule (`lib/toast-offset.ts`): iOS and Android + * run the same math, and the platform enters only through the values resolved + * here for it — the tab bar's own rendered height, which the bar's helper owns, + * and the keyboard occlusion's origin (`resolveKeyboardBottomPadding`). + */ +function AppToaster() { + const colors = useThemeColors(); + const { bottom } = useSafeAreaInsets(); + const { fontScale } = useWindowDimensions(); + const keyboardHeight = useAppAwareKeyboardPadding(); + // The hook's height is the platform's own keyboard metric, and the two + // platforms measure it from different origins: Android's stops at the + // navigation bar (`ReactRootView` reports `imeInsets.bottom − barInsets.bottom`), + // while iOS reports the keyboard frame, which reaches the screen bottom. The + // offset is anchored to the screen bottom, so the occlusion is resolved here + // with the same rule the screens reserve padding with + // (`resolveKeyboardBottomPadding`); passing the raw Android height left the + // toast's last line behind the IME's navigation row (2026-09-20 review + // finding). `lib/toast-offset.ts` stays platform-free. + const keyboardOcclusion = + keyboardHeight > 0 + ? resolveKeyboardBottomPadding({ keyboardHeight, bottomInset: bottom, platform: Platform.OS }) + : 0; + const segments = useSegments(); + const pathname = usePathname(); + // The floating tab bar is an absolute overlay over the screen bottom, so it + // never appears in the reported bottom inset and a toast anchored to the + // inset landed over the tab icons (2026-09-19 visual spot check, p1). It + // renders exactly when the focused route sits inside the tabs navigator and + // `shouldHideTabBar` does not hide it — the same predicate the bar's own + // layout uses — so the toast clears it only while it is actually on screen; + // a screen pushed over the tabs (agent-chat, pr-review) or the auth flow + // keeps the toast at its resting offset. `getEffectiveTabBarHeight` is the + // bar's own measurement (it carries the bar's small Android-only extra + // padding), so the toast cannot disagree with what the bar renders. See + // `lib/toast-offset.ts`. + const tabBarHeight = + (segments as readonly string[]).includes('(tabs)') && !shouldHideTabBar(pathname) + ? getEffectiveTabBarHeight({ bottomInset: bottom, platform: Platform.OS, fontScale }) + : 0; + + return ( + , + error: , + warning: , + info: , + loading: , + }} + toastOptions={{ + style: { + backgroundColor: colors.card, + borderColor: colors.border, + borderWidth: 1, + }, + titleStyle: { color: colors.foreground }, + descriptionStyle: { color: colors.mutedForeground }, + }} + /> + ); +} diff --git a/apps/mobile/src/components/app-unlock-screen.mounted.test.tsx b/apps/mobile/src/components/app-unlock-screen.mounted.test.tsx index a7bf6a2c31..2c3d8d0b7e 100644 --- a/apps/mobile/src/components/app-unlock-screen.mounted.test.tsx +++ b/apps/mobile/src/components/app-unlock-screen.mounted.test.tsx @@ -91,9 +91,9 @@ it.each([false, true])( native.authenticateAsync.mockResolvedValueOnce({ success: false, error: 'user_cancel' }); const now = vi.spyOn(Date, 'now').mockReturnValue(0); await flush(() => { - lifecycle.change?.('background'); + lifecycle.change('background'); now.mockReturnValue(300_000); - lifecycle.change?.('active'); + lifecycle.change('active'); }); expectHidden(root(), true); await flush(retry()?.props.onPress as () => void); @@ -263,9 +263,9 @@ describe.each(['ios', 'android'])('%s shared unlock announcements', os => { if (locked) { const now = vi.spyOn(Date, 'now').mockReturnValue(0); await flush(() => { - lifecycle.change?.('background'); + lifecycle.change('background'); now.mockReturnValue(300_000); - lifecycle.change?.('active'); + lifecycle.change('active'); }); expect(retry()?.props.disabled).toBe(true); } diff --git a/apps/mobile/src/components/app-unlock-screen.test-helpers.tsx b/apps/mobile/src/components/app-unlock-screen.test-helpers.tsx index 35bbbfe515..2b218f9219 100644 --- a/apps/mobile/src/components/app-unlock-screen.test-helpers.tsx +++ b/apps/mobile/src/components/app-unlock-screen.test-helpers.tsx @@ -53,10 +53,69 @@ const storage = vi.hoisted(() => ({ getItemAsync: vi.fn(), setItemAsync: vi.fn() const catalogs = vi.hoisted(() => ({ fr: vi.fn() })); const announcements = vi.hoisted(() => vi.fn()); const platform = vi.hoisted(() => ({ OS: 'ios' })); -const lifecycle = vi.hoisted(() => ({ - change: undefined as ((state: AppStateStatus) => void) | undefined, -})); -export { announcements, catalogs, lifecycle, native, platform, storage }; +// The route the mocked expo-router reports, shaped like production: segments +// carry the group names, the pathname strips them. Defaults to a screen +// pushed over the tabs, where no tab bar is on screen. +const route = vi.hoisted(() => ({ + segments: ['(app)', 'agent-chat'] as string[], + pathname: '/agent-chat', +})); +// AppState has more than one subscriber on this screen (the query client +// lifecycle and the keyboard-lift hooks), so the harness keeps every listener +// and broadcasts to all of them. A single slot let the last registration +// shadow the earlier ones, which hid the stale-lift defect these mounted tests +// exist to catch. Ported from the closed #6392. +const lifecycle = vi.hoisted(() => { + const listeners = new Set<(state: AppStateStatus) => void>(); + return { + listeners, + change: (state: AppStateStatus) => { + for (const listener of listeners) { + listener(state); + } + }, + }; +}); +const keyboard = vi.hoisted(() => { + // Keyboard has more than one subscriber in production: `AppRootProviders` + // mounts the Toaster's shared keyboard hook alongside a screen's, so the mock + // keeps every listener in a set per direction. One slot per direction let the + // last registration shadow the earlier ones, and a `remove()` that cleared + // both slots detached a listener it did not own. The AppState mock below is a + // set for the same reason. Same signature for both directions; `hide` is + // dispatched with an empty payload. + const showListeners = new Set<(event: { endCoordinates: { height: number } }) => void>(); + const hideListeners = new Set<(event: { endCoordinates: { height: number } }) => void>(); + const addListener = ( + event: string, + listener: (event: { endCoordinates: { height: number } }) => void + ) => { + const listeners = + event === 'keyboardDidShow' || event === 'keyboardWillShow' ? showListeners : hideListeners; + listeners.add(listener); + return { + remove: () => { + listeners.delete(listener); + }, + }; + }; + return { + addListener, + showListeners, + hideListeners, + show: (event: { endCoordinates: { height: number } }) => { + for (const listener of showListeners) { + listener(event); + } + }, + hide: () => { + for (const listener of hideListeners) { + listener({ endCoordinates: { height: 0 } }); + } + }, + }; +}); +export { announcements, catalogs, keyboard, lifecycle, native, platform, route, storage }; vi.mock('@/i18n/catalogs', () => ({ CATALOG_LOADERS: catalogs })); vi.mock('expo-local-authentication', () => native); vi.mock('expo-secure-store', () => storage); @@ -72,16 +131,20 @@ vi.mock('react-native', () => ({ Switch: 'Switch', ActivityIndicator: 'ActivityIndicator', Platform: platform, + useWindowDimensions: () => ({ fontScale: 1, width: 390, height: 844, scale: 3 }), StatusBar: { currentHeight: 0 }, I18nManager: { isRTL: false }, AccessibilityInfo: { announceForAccessibility: announcements }, + Keyboard: { + addListener: keyboard.addListener, + }, AppState: { currentState: 'active', addEventListener: (_event: string, listener: (state: AppStateStatus) => void) => { - lifecycle.change = listener; + lifecycle.listeners.add(listener); return { remove: () => { - lifecycle.change = undefined; + lifecycle.listeners.delete(listener); }, }; }, @@ -135,9 +198,9 @@ vi.mock('expo-router', () => ({ { Screen: 'StackScreen' } ), useRouter: () => ({ push: vi.fn() }), - usePathname: () => '/(app)/(tabs)/(0_home)', + usePathname: () => route.pathname, useLocalSearchParams: () => ({ owner: 'owner', repo: 'repo', number: '1', scope: 'personal' }), - useSegments: () => ['(app)', '(tabs)', '(3_profile)', 'organization'], + useSegments: () => route.segments, })); vi.mock('@expo/react-native-action-sheet', () => ({ ActionSheetProvider: 'ActionSheetProvider' })); vi.mock('@rn-primitives/portal', () => ({ PortalHost: 'PortalHost' })); @@ -290,6 +353,11 @@ export function resetUnlockMocks() { vi.stubGlobal('__DEV__', true); vi.resetAllMocks(); platform.OS = 'ios'; + route.segments = ['(app)', 'agent-chat']; + route.pathname = '/agent-chat'; + lifecycle.listeners.clear(); + keyboard.showListeners.clear(); + keyboard.hideListeners.clear(); storage.getItemAsync.mockResolvedValue('enabled'); native.hasHardwareAsync.mockResolvedValue(true); native.isEnrolledAsync.mockResolvedValue(true); diff --git a/apps/mobile/src/components/code-reviewer/manual-review-screen.tsx b/apps/mobile/src/components/code-reviewer/manual-review-screen.tsx index 570ae045ef..990eae333f 100644 --- a/apps/mobile/src/components/code-reviewer/manual-review-screen.tsx +++ b/apps/mobile/src/components/code-reviewer/manual-review-screen.tsx @@ -8,6 +8,8 @@ import { Pressable, TextInput, View } from 'react-native'; import { matchesCodeReviewUrlSuffix } from '@kilocode/app-shared/code-review'; import { ModelSelector } from '@/components/agents/model-selector'; import { EmptyState } from '@/components/empty-state'; +import { AppAwareKeyboardPaddingView } from '@/components/kilo-chat/app-aware-keyboard-padding'; +import { useRevealEndOnKeyboard } from '@/components/kilo-chat/use-reveal-end-on-keyboard'; import { QueryError } from '@/components/query-error'; import { ScreenHeader } from '@/components/screen-header'; import { Button } from '@/components/ui/button'; @@ -83,6 +85,11 @@ export function ManualReviewScreen({ scope }: Readonly<{ scope: string }>) { modelSlug: config.data?.modelSlug ?? '', thinkingEffort: config.data?.thinkingEffort ?? null, }; + // Start review is the form's last child, and the open keyboard covers it on + // Android (edge-to-edge: the window never resizes for the IME), so the + // review could not be started at all. The padding view reserves the + // keyboard's height and the hook reveals the button above it. + const scrollRef = useRevealEndOnKeyboard(); const onSubmit = () => { const url = urlRef.current.trim(); @@ -175,143 +182,146 @@ export function ManualReviewScreen({ scope }: Readonly<{ scope: string }>) { title={t('codeReviewer.manualReview.title')} eyebrow={t('common.codeReviewer')} /> - - - - {t('common.platform')} - - {statusesLoading ? ( - - - - - ) : ( - - {MANUAL_REVIEW_PLATFORMS.map((option, index) => { - const connected = isConnected(option); - return ( - { - void Haptics.selectionAsync(); - urlRef.current = ''; - setUrlError(null); - setPlatformChoice(option); - }} - {...radioItemA11y({ - label: PLATFORM_CAPABILITIES[option].label, - checked: connected && platform === option, - disabled: !connected, - })} - > - - - {PLATFORM_CAPABILITIES[option].label} - - {!connected && ( - - {t('common.notConnected')} - + + + + + {t('common.platform')} + + {statusesLoading ? ( + + + + + ) : ( + + {MANUAL_REVIEW_PLATFORMS.map((option, index) => { + const connected = isConnected(option); + return ( + - - - ); - })} - - )} - - - - - {t('codeReviewer.manualReview.pullRequestUrl')} - - { - urlRef.current = value; - if (urlError) { - setUrlError(null); - } - }} - /> - {urlError ? {urlError} : null} - + onPress={() => { + void Haptics.selectionAsync(); + urlRef.current = ''; + setUrlError(null); + setPlatformChoice(option); + }} + {...radioItemA11y({ + label: PLATFORM_CAPABILITIES[option].label, + checked: connected && platform === option, + disabled: !connected, + })} + > + + + {PLATFORM_CAPABILITIES[option].label} + + {!connected && ( + + {t('common.notConnected')} + + )} + + + + ); + })} + + )} + - - - {t('codeReviewer.manualReview.instructions')} - - { - instructionsRef.current = value; - }} - /> - + + + {t('codeReviewer.manualReview.pullRequestUrl')} + + { + urlRef.current = value; + if (urlError) { + setUrlError(null); + } + }} + /> + {urlError ? {urlError} : null} + - - - {t('common.model')} - - {/* flex-row so the pill hugs its content instead of stretching to column width */} - - { - setModelChoice({ modelSlug: modelId, thinkingEffort: variant || null }); + + + {t('codeReviewer.manualReview.instructions')} + + { + instructionsRef.current = value; }} /> - - - + + + {t('common.model')} + + {/* flex-row so the pill hugs its content instead of stretching to column width */} + + { + setModelChoice({ modelSlug: modelId, thinkingEffort: variant || null }); + }} + /> + + + + + + ); } diff --git a/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding-state.test.ts b/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding-state.test.ts index 4825e02e7c..d2a373f624 100644 --- a/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding-state.test.ts +++ b/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding-state.test.ts @@ -1,10 +1,39 @@ import { describe, expect, it } from 'vitest'; +import { resolveKeyboardBottomPadding } from '@/components/login-screen-state'; import { resolveAppAwareKeyboardPadding, resolveKeyboardPaddingEventsForPlatform, } from './app-aware-keyboard-padding-state'; +// Ported from the closed #6380, whose cases covered the platform-aware bottom +// occlusion the keeper #6388 resolves in `resolveKeyboardBottomPadding`: the +// reported geometry is the platform capability that differs, so Android adds +// the system-bar inset to reach the keyboard's top edge while iOS keeps the +// keyboard frame height, which already reaches the window bottom. +describe('platform-aware keyboard bottom occlusion', () => { + it('reaches the keyboard top edge on Android by adding the system-bar inset', () => { + expect( + resolveKeyboardBottomPadding({ platform: 'android', keyboardHeight: 704, bottomInset: 63 }) + ).toBe(767); + }); + + it('keeps the iOS height, which already reaches the window bottom', () => { + expect( + resolveKeyboardBottomPadding({ platform: 'ios', keyboardHeight: 300, bottomInset: 34 }) + ).toBe(300); + }); + + it('reserves the system-bar inset alone while the keyboard is hidden', () => { + expect( + resolveKeyboardBottomPadding({ platform: 'android', keyboardHeight: 0, bottomInset: 63 }) + ).toBe(63); + expect( + resolveKeyboardBottomPadding({ platform: 'ios', keyboardHeight: 0, bottomInset: 34 }) + ).toBe(34); + }); +}); + describe('app-aware keyboard padding state', () => { it('resolves Android keyboard events from did-show and did-hide notifications', () => { expect(resolveKeyboardPaddingEventsForPlatform('android')).toEqual({ @@ -20,7 +49,7 @@ describe('app-aware keyboard padding state', () => { }); }); - it('clears keyboard padding when the keyboard hides or the app leaves active state', () => { + it('clears keyboard padding when the keyboard hides or the app leaves the foreground', () => { expect( resolveAppAwareKeyboardPadding({ currentPadding: 0, @@ -40,4 +69,23 @@ describe('app-aware keyboard padding state', () => { }) ).toBe(0); }); + + it('keeps keyboard padding through a transient iOS inactive state', () => { + // iOS reports `inactive` for Control Center, the app switcher, a call + // banner, or a system alert while the keyboard stays up, and fires no new + // `keyboardWillShow` when it returns to `active`. Collapsing the padding + // there left the login action under an open keyboard. + expect( + resolveAppAwareKeyboardPadding({ + currentPadding: 320, + event: { type: 'app-state-change', appState: 'inactive' }, + }) + ).toBe(320); + expect( + resolveAppAwareKeyboardPadding({ + currentPadding: 320, + event: { type: 'app-state-change', appState: 'active' }, + }) + ).toBe(320); + }); }); diff --git a/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding-state.ts b/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding-state.ts index f45a6fa40a..464b0a0e42 100644 --- a/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding-state.ts +++ b/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding-state.ts @@ -10,6 +10,9 @@ type KeyboardPaddingPlatformEvents = { hide: 'keyboardDidHide' | 'keyboardWillHide'; }; +// The one keyboard-event difference the platforms keep: Android has no +// `keyboardWillShow`/`keyboardWillHide`, so it reports the did-show pair while +// iOS reports the will-show pair that lands with the keyboard animation. export function resolveKeyboardPaddingEventsForPlatform( platform: string ): KeyboardPaddingPlatformEvents | null { @@ -35,7 +38,13 @@ export function resolveAppAwareKeyboardPadding({ if (event.type === 'keyboard-hidden') { return 0; } - if (event.appState !== 'active') { + // iOS reports `inactive` for transient interruptions the keyboard survives — + // Control Center, the app-switcher preview, a call banner, a system + // permission alert — and fires no fresh `keyboardWillShow` on the way back to + // `active`. Dropping the padding there left the resolved occlusion stuck at 0 + // under an open keyboard, so only a real backgrounding (which dismisses the + // keyboard) clears it. + if (event.appState === 'background') { return 0; } return currentPadding; diff --git a/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding.mounted.test.tsx b/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding.mounted.test.tsx new file mode 100644 index 0000000000..f939cf9c6f --- /dev/null +++ b/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding.mounted.test.tsx @@ -0,0 +1,213 @@ +// Mounted coverage for the shared keyboard-lift view. When the view's bottom +// edge sits at the screen bottom, the reserved space is anchored there, so the +// view must resolve the platform's own keyboard metric through +// `resolveKeyboardBottomPadding` — the same rule the login screen and the +// Toaster use — instead of padding by the raw height. Android's raw height +// stops at the navigation bar, so reserving it left the bottom `bottomInset` +// of the content (the manual review form's Start button) behind the IME's +// navigation row (2026-09-20). +// +// Callers whose own container already reserves the bottom inset above the view +// (the session screen's trailing chrome spacer, the new-session form's parent +// padding) pass `containerReservesBottomInset`, so the inset is subtracted and +// the space is resolved once per screen instead of twice. Callers whose wrapped +// content pads the inset itself (the session composer, the discussion CTA bar) +// pass `contentReservesBottomInset`, so the screen-bottom-anchored occlusion +// does not add it a second time either. + +import { createElement } from 'react'; +import { act, TestRenderer } from '@/test/renderer'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import { AppAwareKeyboardPaddingView } from './app-aware-keyboard-padding'; + +const platform = vi.hoisted(() => ({ OS: 'android' })); +const insets = vi.hoisted(() => ({ bottom: 0 })); +const keyboard = vi.hoisted(() => ({ + show: null as ((event: { endCoordinates: { height: number } }) => void) | null, + hide: null as (() => void) | null, + appState: null as ((state: string) => void) | null, +})); + +vi.mock('react-native', () => ({ + View: 'View', + Platform: platform, + Keyboard: { + addListener: vi.fn((event: string, listener: (event?: unknown) => void) => { + if (event === 'keyboardDidShow' || event === 'keyboardWillShow') { + keyboard.show = listener as (event: { endCoordinates: { height: number } }) => void; + } + if (event === 'keyboardDidHide' || event === 'keyboardWillHide') { + keyboard.hide = listener as () => void; + } + return { remove: vi.fn() }; + }), + }, + AppState: { + addEventListener: vi.fn((_event: string, listener: (state: string) => void) => { + keyboard.appState = listener; + return { remove: vi.fn() }; + }), + }, +})); + +vi.mock('react-native-safe-area-context', () => ({ + useSafeAreaInsets: () => insets, +})); + +type MountProps = { containerReservesBottomInset?: boolean; contentReservesBottomInset?: boolean }; + +function mount(props: MountProps = {}) { + const ref: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined }; + act(() => { + ref.current = TestRenderer.create( + createElement(AppAwareKeyboardPaddingView, props, createElement('Child', null)) + ); + }); + const renderer = ref.current; + if (!renderer) { + throw new Error('padding view was not mounted'); + } + return renderer; +} + +function paddingBottom(renderer: TestRenderer.ReactTestRenderer): number { + const view = renderer.root.find(node => String(node.type) === 'View'); + const style = view.props.style as (Record | undefined)[]; + const padding = style.find(part => part != null && 'paddingBottom' in part); + if (!padding) { + throw new Error('padding view carries no paddingBottom'); + } + return padding.paddingBottom as number; +} + +describe('AppAwareKeyboardPaddingView', () => { + beforeEach(() => { + platform.OS = 'android'; + insets.bottom = 0; + keyboard.show = null; + keyboard.hide = null; + }); + + it('reserves nothing while the keyboard is down', () => { + insets.bottom = 63; + const renderer = mount(); + + expect(paddingBottom(renderer)).toBe(0); + renderer.unmount(); + }); + + it('adds the navigation-bar inset on Android, whose metric stops at the bar', () => { + platform.OS = 'android'; + insets.bottom = 63; + const renderer = mount(); + + act(() => { + keyboard.show?.({ endCoordinates: { height: 704 } }); + }); + expect(paddingBottom(renderer)).toBe(767); + + act(() => { + keyboard.hide?.(); + }); + expect(paddingBottom(renderer)).toBe(0); + + renderer.unmount(); + }); + + it('passes the iOS frame height through, which already reaches the screen bottom', () => { + platform.OS = 'ios'; + insets.bottom = 34; + const renderer = mount(); + + act(() => { + keyboard.show?.({ endCoordinates: { height: 300 } }); + }); + expect(paddingBottom(renderer)).toBe(300); + + renderer.unmount(); + }); + + it('subtracts the container-reserved inset on Android, leaving the raw metric', () => { + // The session screen's trailing spacer and the new-session form's parent + // padding already lift the view's bottom edge `bottomInset` above the + // screen bottom; Android's metric is measured down to the navigation bar, + // so the raw height is exactly the distance from the view's bottom edge to + // the IME top. Adding the inset again floated the composer / Start button + // a nav-bar height above the keyboard (2026-09-20 review finding). + platform.OS = 'android'; + insets.bottom = 63; + const renderer = mount({ containerReservesBottomInset: true }); + + act(() => { + keyboard.show?.({ endCoordinates: { height: 704 } }); + }); + expect(paddingBottom(renderer)).toBe(704); + + renderer.unmount(); + }); + + it('subtracts the container-reserved inset on iOS, whose frame reaches the screen bottom', () => { + platform.OS = 'ios'; + insets.bottom = 34; + const renderer = mount({ containerReservesBottomInset: true }); + + act(() => { + keyboard.show?.({ endCoordinates: { height: 300 } }); + }); + expect(paddingBottom(renderer)).toBe(266); + + renderer.unmount(); + }); + + it('still reserves nothing at rest when the container reserves the inset', () => { + insets.bottom = 63; + const renderer = mount({ containerReservesBottomInset: true }); + + expect(paddingBottom(renderer)).toBe(0); + renderer.unmount(); + }); + + it('leaves the content-reserved inset to the content on Android', () => { + // The chat composer and the discussion CTA bar pad the bottom inset inside + // the view themselves, so the screen-bottom-anchored occlusion must not add + // it again — adding it floated the composer a navigation-bar height above + // the keyboard (2026-09-21 review finding). + platform.OS = 'android'; + insets.bottom = 63; + const renderer = mount({ contentReservesBottomInset: true }); + + act(() => { + keyboard.show?.({ endCoordinates: { height: 704 } }); + }); + expect(paddingBottom(renderer)).toBe(704); + + act(() => { + keyboard.hide?.(); + }); + expect(paddingBottom(renderer)).toBe(0); + + renderer.unmount(); + }); + + it('keeps the iOS frame height for content that pads the inset itself', () => { + platform.OS = 'ios'; + insets.bottom = 34; + const renderer = mount({ contentReservesBottomInset: true }); + + act(() => { + keyboard.show?.({ endCoordinates: { height: 300 } }); + }); + expect(paddingBottom(renderer)).toBe(300); + + renderer.unmount(); + }); + + it('still reserves nothing at rest when the content reserves the inset', () => { + insets.bottom = 63; + const renderer = mount({ contentReservesBottomInset: true }); + + expect(paddingBottom(renderer)).toBe(0); + renderer.unmount(); + }); +}); diff --git a/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding.test.ts b/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding.test.ts index 78cc1adbba..27e6040268 100644 --- a/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding.test.ts +++ b/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding.test.ts @@ -21,6 +21,18 @@ describe('app-aware keyboard padding', () => { ).toBe(0); }); + it('keeps the padded height through a transient inactive state', () => { + // A keyboard that stays up across Control Center or a system alert must not + // have its reserved padding collapsed: iOS fires no `keyboardWillShow` + // again on the way back to `active`. + expect( + resolveAppAwareKeyboardPadding({ + currentPadding: 320, + event: { type: 'app-state-change', appState: 'inactive' }, + }) + ).toBe(320); + }); + it('keeps padding reset on foreground until a fresh keyboard event arrives', () => { expect( resolveAppAwareKeyboardPadding({ diff --git a/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding.tsx b/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding.tsx index 264b38527a..aa8049e86f 100644 --- a/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding.tsx +++ b/apps/mobile/src/components/kilo-chat/app-aware-keyboard-padding.tsx @@ -1,6 +1,8 @@ import { type ComponentProps, useEffect, useState } from 'react'; import { AppState, Keyboard, type KeyboardEvent, Platform, View } from 'react-native'; +import { useSafeAreaInsets } from 'react-native-safe-area-context'; +import { resolveKeyboardBottomPadding } from '@/components/login-screen-state'; import { resolveAppAwareKeyboardPadding, resolveKeyboardPaddingEventsForPlatform, @@ -10,11 +12,16 @@ function keyboardPaddingFromEvent(event: KeyboardEvent): number { return event.endCoordinates.height; } -export function AppAwareKeyboardPaddingView({ - style, - keyboardOffset = 0, - ...props -}: ComponentProps & { keyboardOffset?: number }) { +/** + * Height of the software keyboard while it is up, `0` otherwise, resolved for + * the current platform (`keyboardWillShow` on iOS, `keyboardDidShow` on + * Android, where the window never resizes for the IME under API 35+). + * + * A screen that must react to the keyboard beyond reserving its height (e.g. + * revealing a call-to-action the IME covers) reads it from here instead of + * adding a second listener. + */ +export function useAppAwareKeyboardPadding(): number { const [keyboardPadding, setKeyboardPadding] = useState(0); useEffect(() => { @@ -59,6 +66,71 @@ export function AppAwareKeyboardPaddingView({ }; }, []); + return keyboardPadding; +} + +export function AppAwareKeyboardPaddingView({ + style, + keyboardOffset = 0, + containerReservesBottomInset = false, + contentReservesBottomInset = false, + ...props +}: ComponentProps & { + keyboardOffset?: number; + /** + * The caller's own container reserves the platform's bottom inset above this + * view (a trailing spacer, or a `paddingBottom` on the parent), so the + * view's bottom edge sits `bottomInset` above the screen bottom. The + * resolved occlusion is anchored to the screen bottom, so the inset the + * container already reserved is subtracted here — on Android that reduces to + * the platform's raw metric, whose origin stops at the navigation bar. + * Counting the inset twice floated the session composer and the new-session + * Start button a nav-bar height above the keyboard (2026-09-20). + */ + containerReservesBottomInset?: boolean; + /** + * The wrapped content pads the platform's bottom inset itself: the session + * composer adds `MESSAGE_INPUT_BOTTOM_CLEARANCE + bottomInset` and the + * discussion CTA bar adds `useDetailScreenBottomPadding()`. The occlusion + * resolved above is anchored to the screen bottom, so it counts that inset on + * top of the content's own padding and floats the composer / CTA a + * navigation-bar height above the keyboard. Such a caller adds the platform's + * raw keyboard metric instead: on Android the content's inset padding + * completes it, and on iOS the metric already reaches the screen bottom, so + * the lift those callers shipped with is unchanged (2026-09-21 review + * finding). + */ + contentReservesBottomInset?: boolean; +}) { + const keyboardHeight = useAppAwareKeyboardPadding(); + const { bottom } = useSafeAreaInsets(); + // The hook reports the platform's own keyboard metric, and the two platforms + // measure it from different origins: Android's stops at the navigation bar + // (`ReactRootView` reports `imeInsets.bottom − barInsets.bottom`) while iOS's + // frame reaches the screen bottom. The reserved space is anchored to the + // screen bottom, so resolve it with the same rule the login screen and the + // Toaster use; padding by the raw Android height left the bottom + // `bottomInset` of the content — the manual review form's Start button — + // behind the IME's navigation row (2026-09-20). + const keyboardOcclusion = + keyboardHeight > 0 + ? resolveKeyboardBottomPadding({ + keyboardHeight, + bottomInset: bottom, + platform: Platform.OS, + }) + : 0; + // One inset per screen: where a container outside this view (a trailing + // spacer, a parent `paddingBottom`) or the wrapped content's own bottom + // padding already reserved the bottom inset, the screen-bottom-anchored + // occlusion must not count it a second time. + let keyboardPadding = keyboardOcclusion; + if (containerReservesBottomInset) { + keyboardPadding = Math.max(keyboardOcclusion - bottom, 0); + } else if (contentReservesBottomInset) { + keyboardPadding = keyboardHeight; + } + const resolvedKeyboardPadding = keyboardPadding > 0 ? keyboardPadding + keyboardOffset : 0; return ; diff --git a/apps/mobile/src/components/kilo-chat/conversation-screen.composer-keyboard.mounted.test.tsx b/apps/mobile/src/components/kilo-chat/conversation-screen.composer-keyboard.mounted.test.tsx new file mode 100644 index 0000000000..5a3a3b66a4 --- /dev/null +++ b/apps/mobile/src/components/kilo-chat/conversation-screen.composer-keyboard.mounted.test.tsx @@ -0,0 +1,235 @@ +// Mounted coverage for the chat screen's keyboard lift. The composer's own +// bottom padding already includes the platform's safe-area inset +// (`resolveMessageInputBottomPadding`), so the screen's +// `AppAwareKeyboardPaddingView` must not count that inset a second time: doing +// so floated the composer a navigation-bar height above the keyboard on Android +// (2026-09-21 review finding). Both platforms are asserted against the metric +// the composer completes, so a caller that drops the opt-in fails here. + +import { createElement } from 'react'; +import { act, TestRenderer } from '@/test/renderer'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +import '@/i18n'; +import type * as ReactI18next from 'react-i18next'; +import { ConversationScreen } from './conversation-screen'; + +const platform = vi.hoisted(() => ({ OS: 'android' })); +const insets = vi.hoisted(() => ({ bottom: 0 })); +const keyboard = vi.hoisted(() => ({ + show: null as ((event: { endCoordinates: { height: number } }) => void) | null, + hide: null as (() => void) | null, +})); + +vi.mock('react-native', () => ({ + View: 'View', + Platform: platform, + Keyboard: { + addListener: vi.fn((event: string, listener: (event?: unknown) => void) => { + if (event === 'keyboardDidShow' || event === 'keyboardWillShow') { + keyboard.show = listener as (event: { endCoordinates: { height: number } }) => void; + } + if (event === 'keyboardDidHide' || event === 'keyboardWillHide') { + keyboard.hide = listener as () => void; + } + return { remove: vi.fn() }; + }), + }, + AppState: { + addEventListener: vi.fn(() => ({ remove: vi.fn() })), + }, +})); + +vi.mock('react-native-safe-area-context', () => ({ + useSafeAreaInsets: () => insets, +})); + +vi.mock('@kilocode/kilo-chat-hooks', () => ({ + useBotStatus: () => null, + useEventServiceClient: () => ({}), +})); + +vi.mock('@kilocode/kilo-chat', () => ({ CONVERSATION_TITLE_MAX_CHARS: 100 })); + +vi.mock('expo-router', () => ({ + useFocusEffect: vi.fn(), + useRouter: () => ({ push: vi.fn() }), +})); + +vi.mock('sonner-native', () => ({ toast: { error: vi.fn() } })); +vi.mock('react-i18next', async importOriginal => { + const actual = (await importOriginal()) as typeof ReactI18next; + return { + ...actual, + useTranslation: () => { + const i18n = actual.getI18n(); + return { t: i18n.t.bind(i18n), i18n }; + }, + }; +}); + +vi.mock('@/components/rename-modal', () => ({ RenameModal: 'RenameModal' })); +vi.mock('@/lib/notifications', () => ({ setActiveChatLocation: vi.fn() })); +vi.mock('@/lib/kilo-chat-routes', () => ({ chatInstancePickerPath: () => '/instances' })); +vi.mock('@/lib/kiloclaw-display', () => ({ kiloclawConversationEyebrow: () => undefined })); +vi.mock('@/lib/hooks/use-instance-context', () => ({ + instanceOrgId: () => 'org-1', + useInstanceContext: () => ({ status: 'ready' }), + useAllKiloClawInstances: () => ({ data: undefined }), +})); +vi.mock('@/lib/hooks/use-kiloclaw-queries', () => ({ useKiloClawStatus: () => ({ data: null }) })); + +// Mocked like the repo's other mounted screens: the loading/error views and the +// heavy children are stubbed down to their element type, so the mounted tree is +// the screen's own keyboard-lift view and the composer's slot in it. This test +// keeps the history content at `ready`, so none of those views render. +vi.mock('./conversation-history-state-views', () => ({ + ConversationHistoryErrorView: 'ConversationHistoryErrorView', + ConversationHistoryLoadingView: 'ConversationHistoryLoadingView', + ConversationInlineRetryBanner: 'ConversationInlineRetryBanner', +})); +vi.mock('./conversation-header', () => ({ ConversationHeader: 'ConversationHeader' })); +vi.mock('./message-list', () => ({ MessageList: 'MessageList' })); +vi.mock('./message-input', () => ({ MessageInput: 'MessageInput' })); +vi.mock('./message-reaction-picker-sheet', () => ({ + MessageReactionPickerSheet: 'MessageReactionPickerSheet', +})); + +vi.mock('./kilo-chat-provider', () => ({ + useKiloChatTokenError: () => ({ hasError: false, retry: vi.fn() }), +})); +vi.mock('./hooks/use-app-active-and-focused', () => ({ useAppActiveAndFocused: () => true })); +vi.mock('./hooks/use-current-user-id', () => ({ useCurrentUserId: () => 'user-1' })); +vi.mock('./hooks/use-now-ticker', () => ({ useNowTicker: () => 1_800_000_000_000 })); +vi.mock('./hooks/use-kilo-chat-client', () => ({ useKiloChatClient: () => ({}) })); +vi.mock('./hooks/use-conversation-presence', () => ({ useConversationPresence: vi.fn() })); +vi.mock('./hooks/use-conversation-event-subscription', () => ({ + useConversationEventSubscription: vi.fn(), +})); +vi.mock('./hooks/use-conversation-mark-read', () => ({ useConversationMarkRead: vi.fn() })); +vi.mock('./hooks/use-messages', () => ({ + useMessageCacheUpdater: vi.fn(), + useMessages: () => ({ + data: { messages: [] }, + isPending: false, + isError: false, + hasNextPage: false, + isFetchingNextPage: false, + fetchNextPage: vi.fn(), + }), +})); +vi.mock('./hooks/use-typing', () => ({ + useMobileTypingState: () => ({ typingMembers: [], clearTypingForMember: vi.fn() }), + useTypingSender: () => vi.fn(), +})); +vi.mock('./hooks/use-conversation-options-sheet', () => ({ + useConversationOptionsSheet: () => ({ + openOptions: vi.fn(), + renaming: false, + closeRename: vi.fn(), + saveRename: vi.fn(), + }), +})); +vi.mock('./hooks/use-conversation-message-controller', () => ({ + useConversationMessageController: () => ({ + editingMessage: null, + editingText: '', + visibleEditingAttachments: [], + inputAvailability: { + disabled: false, + submitDisabled: false, + disabledReason: undefined, + showInstanceCta: false, + }, + pendingAction: null, + reactionPickerMessage: null, + recentReactions: [], + replyingTo: null, + scrollToNewestRequest: 0, + handleExecuteAction: vi.fn(), + handleLongPressMessage: vi.fn(), + handleReactionPress: vi.fn(), + handleSend: vi.fn(), + handleSwipeReplyMessage: vi.fn(), + setEditingMessage: vi.fn(), + setRemovedEditAttachmentIds: vi.fn(), + setReactionPickerMessage: vi.fn(), + setReplyingTo: vi.fn(), + }), +})); + +function mount() { + const ref: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined }; + act(() => { + ref.current = TestRenderer.create( + createElement(ConversationScreen, { + sandboxId: 'instance-1', + conversationId: 'conversation-1', + conversationTitle: 'Title', + conversationRenameTitle: 'Title', + conversationMembers: [], + }) + ); + }); + const renderer = ref.current; + if (!renderer) { + throw new Error('conversation screen was not mounted'); + } + return renderer; +} + +/** Padding the screen's keyboard-lift view reserves (its own style slot). */ +function keyboardPadding(renderer: TestRenderer.ReactTestRenderer): number { + const view = renderer.root.find( + node => String(node.type) === 'View' && Array.isArray(node.props.style) + ); + const parts = view.props.style as (Record | undefined)[]; + const padded = parts.find(part => part != null && 'paddingBottom' in part); + return padded?.paddingBottom ?? -1; +} + +describe('ConversationScreen composer keyboard lift', () => { + beforeEach(() => { + platform.OS = 'android'; + insets.bottom = 0; + keyboard.show = null; + keyboard.hide = null; + }); + + it("adds only the raw Android metric on top of the composer's own inset padding", () => { + platform.OS = 'android'; + insets.bottom = 63; + const renderer = mount(); + + act(() => { + keyboard.show?.({ endCoordinates: { height: 704 } }); + }); + // 767 would be the screen-bottom-anchored occlusion counting the inset the + // composer already pads by a second time. + expect(keyboardPadding(renderer)).toBe(704); + + renderer.unmount(); + }); + + it('keeps the iOS frame height, which the composer completes', () => { + platform.OS = 'ios'; + insets.bottom = 34; + const renderer = mount(); + + act(() => { + keyboard.show?.({ endCoordinates: { height: 300 } }); + }); + expect(keyboardPadding(renderer)).toBe(300); + + renderer.unmount(); + }); + + it('reserves nothing while the keyboard is down', () => { + insets.bottom = 63; + const renderer = mount(); + + expect(keyboardPadding(renderer)).toBe(0); + + renderer.unmount(); + }); +}); diff --git a/apps/mobile/src/components/kilo-chat/conversation-screen.tsx b/apps/mobile/src/components/kilo-chat/conversation-screen.tsx index 173863c364..65cca84613 100644 --- a/apps/mobile/src/components/kilo-chat/conversation-screen.tsx +++ b/apps/mobile/src/components/kilo-chat/conversation-screen.tsx @@ -199,7 +199,10 @@ export function ConversationScreen({ }} /> ) : null} - + {/* The composer below already pads the platform's bottom inset inside + this view (message-input-layout), so the keyboard lift must not add it + a second time and float the composer above the keyboard. */} + ({ padding: 0 })); + +vi.mock('./app-aware-keyboard-padding', () => ({ + useAppAwareKeyboardPadding: () => keyboard.padding, +})); + +const slots = { + refs: [] as { current: unknown }[], + refCursor: 0, + cleanups: [] as (() => void)[], +}; + +vi.mock('react', () => ({ + useRef: (initial: unknown) => { + if (slots.refs.length <= slots.refCursor) { + slots.refs.push({ current: initial }); + } + const slot = slots.refs[slots.refCursor]; + slots.refCursor += 1; + return slot; + }, + useEffect: (effect: () => unknown) => { + const cleanup = effect(); + if (typeof cleanup === 'function') { + slots.cleanups.push(cleanup as () => void); + } + }, +})); + +type Mounted = { + scrollToEnd: ReturnType; + unmount: () => void; +}; + +function mountHook(): Mounted { + const scrollToEnd = vi.fn(); + slots.refs = [{ current: { scrollToEnd } }]; + slots.refCursor = 0; + slots.cleanups = []; + function Harness(): null { + useRevealEndOnKeyboard(); + return null; + } + // eslint-disable-next-line new-cap -- plain-function mount of the hook harness + Harness(); + return { + scrollToEnd, + unmount: () => { + for (const cleanup of slots.cleanups.splice(0)) { + cleanup(); + } + }, + }; +} + +describe('useRevealEndOnKeyboard', () => { + beforeEach(() => { + keyboard.padding = 0; + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('does not scroll while the keyboard is closed', () => { + const { scrollToEnd } = mountHook(); + + vi.advanceTimersByTime(KEYBOARD_REVEAL_RETRY_MS * 4); + + expect(scrollToEnd).not.toHaveBeenCalled(); + }); + + it('scrolls the call-to-action into view while the keyboard is open', () => { + keyboard.padding = 300; + + const { scrollToEnd } = mountHook(); + expect(scrollToEnd).toHaveBeenCalledTimes(1); + expect(scrollToEnd).toHaveBeenLastCalledWith({ animated: false }); + + vi.advanceTimersByTime(KEYBOARD_REVEAL_RETRY_MS); + expect(scrollToEnd).toHaveBeenCalledTimes(2); + }); + + it('drops the pending retry when the screen unmounts', () => { + keyboard.padding = 300; + + const { scrollToEnd, unmount } = mountHook(); + unmount(); + vi.advanceTimersByTime(KEYBOARD_REVEAL_RETRY_MS * 4); + + expect(scrollToEnd).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/mobile/src/components/kilo-chat/use-reveal-end-on-keyboard.ts b/apps/mobile/src/components/kilo-chat/use-reveal-end-on-keyboard.ts new file mode 100644 index 0000000000..9d8b0a3c7d --- /dev/null +++ b/apps/mobile/src/components/kilo-chat/use-reveal-end-on-keyboard.ts @@ -0,0 +1,44 @@ +import { type RefObject, useEffect, useRef } from 'react'; +import { type ScrollView } from 'react-native'; + +import { useAppAwareKeyboardPadding } from './app-aware-keyboard-padding'; + +/** One frame's grace for the keyboard padding to reach the scroll view. */ +export const KEYBOARD_REVEAL_RETRY_MS = 80; + +/** + * Keeps a scroll view's trailing call-to-action reachable while the software + * keyboard is open, returning the ref to attach to that scroll view. + * + * Android is the exposed case: under API 35+ edge-to-edge the window never + * resizes for the IME and `automaticallyAdjustKeyboardInsets` is iOS-only, so + * a form whose submit button is its last child keeps that button under the + * keyboard. A screen that reserves the keyboard's height (e.g. with + * `AppAwareKeyboardPaddingView`) makes the form scrollable, but nothing scrolls + * it; this reveals the end, where the call-to-action lives, as the keyboard + * comes up. + * + * The retry covers the frame the reserved padding lands on, when the first + * scroll is still a no-op against the pre-lift viewport (the same one-frame + * race the chat message list handles with its own scheduler). + */ +export function useRevealEndOnKeyboard(): RefObject { + const scrollRef = useRef(null); + const keyboardPadding = useAppAwareKeyboardPadding(); + + useEffect(() => { + if (keyboardPadding === 0) { + return undefined; + } + const revealCallToAction = () => { + scrollRef.current?.scrollToEnd({ animated: false }); + }; + revealCallToAction(); + const retry = setTimeout(revealCallToAction, KEYBOARD_REVEAL_RETRY_MS); + return () => { + clearTimeout(retry); + }; + }, [keyboardPadding]); + + return scrollRef; +} diff --git a/apps/mobile/src/components/login-screen-state.ts b/apps/mobile/src/components/login-screen-state.ts index 47331c715a..913ae6052e 100644 --- a/apps/mobile/src/components/login-screen-state.ts +++ b/apps/mobile/src/components/login-screen-state.ts @@ -1,5 +1,35 @@ import { i18n } from '@/i18n'; +/** + * Bottom padding that keeps the login content clear of both the keyboard and + * the device's bottom bar. + * + * Android and iOS report the IME from different origins, and React Native has + * no cross-platform keyboard metric measured from the screen bottom: Android's + * `endCoordinates.height` stops at the navigation bar (`ReactRootView` reports + * `imeInsets.bottom − barInsets.bottom`), while iOS reports the keyboard window + * frame, which reaches the screen bottom and so already includes the + * home-indicator inset. Adding `bottomInset` on Android is what makes the + * reserved occlusion equal on both platforms — the capability Android lacks is + * a metric that reaches the screen bottom, and iOS has no equivalent inset to + * add (adding it there would float the form above the IME). With the keyboard + * down, the inset alone keeps the content clear of the bottom bar. + */ +export function resolveKeyboardBottomPadding({ + keyboardHeight, + bottomInset, + platform, +}: { + keyboardHeight: number; + bottomInset: number; + platform: string; +}): number { + if (keyboardHeight > 0) { + return platform === 'android' ? keyboardHeight + bottomInset : keyboardHeight; + } + return bottomInset; +} + export function errorMessage(status: string, fallback: string | undefined): string { switch (status) { case 'expired': { diff --git a/apps/mobile/src/components/login-screen.test.ts b/apps/mobile/src/components/login-screen.test.ts index 355eaa67a3..140db3db16 100644 --- a/apps/mobile/src/components/login-screen.test.ts +++ b/apps/mobile/src/components/login-screen.test.ts @@ -1,7 +1,9 @@ /* eslint-disable max-lines -- The mounted tests keep the refresh boundary, error mapping, globe, and draft-restore contracts together. */ import { createElement } from 'react'; import { act, TestRenderer } from '@/test/renderer'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { type AppStateStatus, Keyboard, type KeyboardEvent, Platform } from 'react-native'; +import { useSafeAreaInsets } from 'react-native-safe-area-context'; // login-screen.test.ts — narrow contract tests plus mounted globe tests. // The refresh boundary contract is verified through the useDeviceAuth hook's @@ -16,7 +18,7 @@ import { restoreLoginDrafts, } from '@/lib/login-draft'; import { LoginScreen } from './login-screen'; -import { errorMessage } from './login-screen-state'; +import { errorMessage, resolveKeyboardBottomPadding } from './login-screen-state'; // ── Hoisted mocks for the mounted globe tests ────────────────────────────── @@ -32,6 +34,9 @@ const deviceAuth = vi.hoisted(() => ({ })); const push = vi.hoisted(() => vi.fn()); const setLanguagePickerBridge = vi.hoisted(() => vi.fn()); +const addAppStateListener = vi.hoisted(() => + vi.fn((_event: 'change', _listener: (state: AppStateStatus) => void) => ({ remove: vi.fn() })) +); vi.mock('expo-router', () => ({ useRouter: () => ({ push }), @@ -40,25 +45,20 @@ vi.mock('expo-router', () => ({ vi.mock('@/components/ui/activity-indicator', () => ({ ActivityIndicator: 'ActivityIndicator' })); vi.mock('react-native', () => ({ ActivityIndicator: 'ActivityIndicator', - AppState: { addEventListener: vi.fn(() => ({ remove: vi.fn() })) }, + AppState: { addEventListener: addAppStateListener }, I18nManager: { isRTL: false }, Keyboard: { addListener: vi.fn(() => ({ remove: vi.fn() })) }, - KeyboardAvoidingView: 'KeyboardAvoidingView', Platform: { OS: 'ios' }, Pressable: 'Pressable', ScrollView: 'ScrollView', View: 'View', })); vi.mock('react-native-safe-area-context', () => ({ - useSafeAreaInsets: () => ({ top: 0, bottom: 0, left: 0, right: 0 }), + useSafeAreaInsets: vi.fn(() => ({ top: 0, bottom: 0, left: 0, right: 0 })), })); vi.mock('sonner-native', () => ({ toast: vi.fn() })); vi.mock('expo-clipboard', () => ({ setStringAsync: vi.fn() })); vi.mock('@/../assets/images/logo.png', () => ({ default: 1 })); -vi.mock('@/components/kilo-chat/app-aware-keyboard-padding-state', () => ({ - resolveAppAwareKeyboardPadding: vi.fn(), - resolveKeyboardPaddingEventsForPlatform: () => null, -})); vi.mock('@/components/centered-state', () => ({ CenteredState: 'CenteredState' })); vi.mock('@/components/login/idle-auth', () => ({ IdleAuth: 'IdleAuth' })); vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); @@ -118,6 +118,12 @@ function findByType( return root.findAll(node => typeof node.type === 'string' && (node.type as string) === type); } +function keyboardEventsFor(platform: 'android' | 'ios') { + return platform === 'ios' + ? ({ show: 'keyboardWillShow', hide: 'keyboardWillHide' } as const) + : ({ show: 'keyboardDidShow', hide: 'keyboardDidHide' } as const); +} + async function mountLoginScreen(): Promise { const ref: { current: TestRenderer.ReactTestRenderer | undefined } = { current: undefined }; await act(async () => { @@ -229,6 +235,35 @@ describe('login-screen error mapping', () => { }); }); +describe('login-screen keyboard bottom padding', () => { + it.each(['android', 'ios'] as const)( + 'reserves the bottom inset alone for the %s keyboard-down state', + platform => { + expect(resolveKeyboardBottomPadding({ keyboardHeight: 0, bottomInset: 28, platform })).toBe( + 28 + ); + } + ); + + it('adds the bottom inset to Android, whose keyboard metric stops at the navigation bar', () => { + expect( + resolveKeyboardBottomPadding({ keyboardHeight: 300, bottomInset: 28, platform: 'android' }) + ).toBe(328); + }); + + it('keeps the iOS keyboard frame height, which already includes the home indicator', () => { + expect( + resolveKeyboardBottomPadding({ keyboardHeight: 300, bottomInset: 28, platform: 'ios' }) + ).toBe(300); + }); + + it('ignores a negative reported height', () => { + expect( + resolveKeyboardBottomPadding({ keyboardHeight: -1, bottomInset: 28, platform: 'android' }) + ).toBe(28); + }); +}); + describe('login-screen malformed poll boundary', () => { it('returns null for a 200 body with no token — prevents signIn call', () => { // When the server returns HTTP 200 but parse fails (no token), @@ -410,3 +445,194 @@ describe('login-screen idle skeleton', () => { renderer.unmount(); }); }); + +describe('login-screen bottom-bar clearance', () => { + beforeEach(() => { + deviceAuth.status = 'idle'; + deviceAuth.token = undefined; + deviceAuth.code = undefined; + Platform.OS = 'android'; + vi.mocked(Keyboard.addListener).mockClear(); + addAppStateListener.mockClear(); + vi.mocked(useSafeAreaInsets).mockReturnValue({ top: 24, bottom: 28, left: 0, right: 0 }); + vi.mocked(restoreLoginDrafts).mockResolvedValue({ email: '', ssoRecovery: null }); + }); + + afterEach(() => { + Platform.OS = 'ios'; + vi.mocked(useSafeAreaInsets).mockReturnValue({ top: 0, bottom: 0, left: 0, right: 0 }); + }); + + function scrollViewport(renderer: TestRenderer.ReactTestRenderer) { + const scroll = findByType(renderer.root, 'ScrollView')[0]; + if (!scroll?.parent) { + throw new Error('login scroll viewport not found'); + } + expect(scroll.props.className).toContain('flex-1'); + expect(scroll.props.contentContainerClassName).toContain('flex-grow'); + expect(scroll.props.keyboardShouldPersistTaps).toBe('handled'); + return scroll.parent; + } + + function emitKeyboard( + eventName: 'keyboardDidShow' | 'keyboardDidHide' | 'keyboardWillShow' | 'keyboardWillHide', + height = 0 + ) { + const subscription = vi + .mocked(Keyboard.addListener) + .mock.calls.find(([name]) => name === eventName); + if (!subscription) { + throw new Error(`missing ${eventName} listener`); + } + const event: KeyboardEvent = { + duration: 0, + easing: 'keyboard', + endCoordinates: { height, width: 360, screenX: 0, screenY: 640 - height }, + }; + act(() => { + subscription[1](event); + }); + } + + it.each(['android', 'ios'] as const)( + 'keeps the %s bottom bar outside the scroll viewport with a long email', + async platform => { + Platform.OS = platform; + const email = 'long.email.address@subdomain.example-very-long-domain-name.co.uk'; + vi.mocked(restoreLoginDrafts).mockResolvedValue({ email, ssoRecovery: null }); + const renderer = await mountLoginScreen(); + + expect(findByType(renderer.root, 'IdleAuth')[0]?.props.initialEmail).toBe(email); + expect(scrollViewport(renderer).props.style).toEqual({ paddingBottom: 28 }); + // One padded wrapper for both platforms; the platform-gated + // KeyboardAvoidingView is gone. + expect(findByType(renderer.root, 'KeyboardAvoidingView')).toHaveLength(0); + + renderer.unmount(); + } + ); + + it.each(['idle', 'pending', 'expired', 'error', 'denied'])( + 'reserves the bottom bar for the %s auth state', + async status => { + deviceAuth.status = status; + const renderer = await mountLoginScreen(); + + expect(scrollViewport(renderer).props.style).toEqual({ paddingBottom: 28 }); + + renderer.unmount(); + } + ); + + it('reserves the bottom bar while the draft placeholder is visible', async () => { + const draft = Promise.withResolvers>>(); + vi.mocked(restoreLoginDrafts).mockReturnValue(draft.promise); + const renderer = await mountLoginScreen(); + + expect(findByType(renderer.root, 'Skeleton')).toHaveLength(2); + expect(scrollViewport(renderer).props.style).toEqual({ paddingBottom: 28 }); + + await act(async () => { + draft.resolve({ email: '', ssoRecovery: null }); + await draft.promise; + }); + expect(findByType(renderer.root, 'Skeleton')).toHaveLength(0); + expect(scrollViewport(renderer).props.style).toEqual({ paddingBottom: 28 }); + + renderer.unmount(); + }); + + it.each(['android', 'ios'] as const)( + 'pads the %s keyboard height through the same listener pair', + async platform => { + const events = keyboardEventsFor(platform); + Platform.OS = platform; + const renderer = await mountLoginScreen(); + + // Same wrapper, one listener pair per platform: the only keyboard-event + // difference is which pair that platform fires. + expect(vi.mocked(Keyboard.addListener).mock.calls.map(([name]) => name)).toEqual([ + events.show, + events.hide, + ]); + expect(scrollViewport(renderer).props.style).toEqual({ paddingBottom: 28 }); + + // Android's reported height excludes the navigation bar, so the reserved + // inset is added on top; iOS reports the keyboard window frame, whose + // height already covers the home indicator, so the inset is not added + // twice while the keyboard is up (it would leave a gap above the IME). + emitKeyboard(events.show, 300); + expect(scrollViewport(renderer).props.style).toEqual({ + paddingBottom: platform === 'android' ? 328 : 300, + }); + + emitKeyboard(events.hide); + expect(scrollViewport(renderer).props.style).toEqual({ paddingBottom: 28 }); + + emitKeyboard(events.show, 0); + expect(scrollViewport(renderer).props.style).toEqual({ paddingBottom: 28 }); + + renderer.unmount(); + } + ); + + it.each(['android', 'ios'] as const)( + 'clears stale %s keyboard occlusion on background without dropping the bottom bar', + async platform => { + Platform.OS = platform; + const renderer = await mountLoginScreen(); + emitKeyboard(keyboardEventsFor(platform).show, 300); + const subscription = addAppStateListener.mock.calls[0]; + if (!subscription) { + throw new Error('missing app state listener'); + } + + act(() => { + subscription[1]('background'); + }); + expect(scrollViewport(renderer).props.style).toEqual({ paddingBottom: 28 }); + + renderer.unmount(); + } + ); + + it('keeps the iOS keyboard occlusion through a transient inactive state', async () => { + Platform.OS = 'ios'; + const renderer = await mountLoginScreen(); + emitKeyboard(keyboardEventsFor('ios').show, 300); + const subscription = addAppStateListener.mock.calls[0]; + if (!subscription) { + throw new Error('missing app state listener'); + } + + // Control Center, the app switcher preview, a call banner, and system + // permission alerts report `inactive` while the keyboard stays up, and no + // fresh `keyboardWillShow` follows on the way back to `active`; collapsing + // the occlusion here left the form under an open keyboard. + act(() => { + subscription[1]('inactive'); + }); + expect(scrollViewport(renderer).props.style).toEqual({ paddingBottom: 300 }); + + act(() => { + subscription[1]('active'); + }); + expect(scrollViewport(renderer).props.style).toEqual({ paddingBottom: 300 }); + + renderer.unmount(); + }); + + it('tracks bottom inset changes, including devices without a bottom bar', async () => { + const renderer = await mountLoginScreen(); + + for (const bottom of [48, 0]) { + vi.mocked(useSafeAreaInsets).mockReturnValue({ top: 24, bottom, left: 0, right: 0 }); + act(() => { + renderer.update(createElement(LoginScreen)); + }); + expect(scrollViewport(renderer).props.style).toEqual({ paddingBottom: bottom }); + } + + renderer.unmount(); + }); +}); diff --git a/apps/mobile/src/components/login-screen.tsx b/apps/mobile/src/components/login-screen.tsx index 10247c17c2..3c873314f9 100644 --- a/apps/mobile/src/components/login-screen.tsx +++ b/apps/mobile/src/components/login-screen.tsx @@ -8,7 +8,6 @@ import { AppState, I18nManager, Keyboard, - KeyboardAvoidingView, type KeyboardEvent, Platform, Pressable, @@ -26,7 +25,7 @@ import { resolveKeyboardPaddingEventsForPlatform, } from '@/components/kilo-chat/app-aware-keyboard-padding-state'; import { IdleAuth } from '@/components/login/idle-auth'; -import { errorMessage } from '@/components/login-screen-state'; +import { errorMessage, resolveKeyboardBottomPadding } from '@/components/login-screen-state'; import { Button } from '@/components/ui/button'; import { Image } from '@/components/ui/image'; import { Skeleton } from '@/components/ui/skeleton'; @@ -68,7 +67,7 @@ export function LoginScreen() { const insets = useSafeAreaInsets(); const { t } = useTranslation(); const [persistError, setPersistError] = useState(undefined); - const [androidKeyboardHeight, setAndroidKeyboardHeight] = useState(0); + const [keyboardHeight, setKeyboardHeight] = useState(0); const [authFormBusy, setAuthFormBusy] = useState(false); const [draft, setDraft] = useState<{ email: string; @@ -123,23 +122,22 @@ export function LoginScreen() { // eslint-disable-next-line react-hooks/exhaustive-deps -- persistToken is stable except for signIn identity; only re-run on a newly approved token }, [status, token]); - // Android shell keyboard pad: under API 35+ EDGE_TO_EDGE_ENFORCED the window - // never resizes for the IME, so KeyboardAvoidingView is inert. keyboardDidShow - // still fires with real heights; consume them here (r0b: zero layout shift for - // the email IME when only KAV was present). + // The login screen owns keyboard occlusion on both platforms, so the layout is + // one implementation: KeyboardAvoidingView used to handle iOS alone and left + // Android — whose window never resizes for the IME under API 35+ + // EDGE_TO_EDGE_ENFORCED — to this listener. The only platform difference left + // is which keyboard events exist: Android fires no `keyboardWillShow`/ + // `keyboardWillHide`, so `resolveKeyboardPaddingEventsForPlatform` names the + // pair each platform reports. useEffect(() => { - if (Platform.OS !== 'android') { - return undefined; - } - const keyboardEvents = resolveKeyboardPaddingEventsForPlatform(Platform.OS); if (keyboardEvents === null) { - setAndroidKeyboardHeight(0); + setKeyboardHeight(0); return undefined; } const keyboardShowSubscription = Keyboard.addListener(keyboardEvents.show, event => { - setAndroidKeyboardHeight(current => + setKeyboardHeight(current => resolveAppAwareKeyboardPadding({ currentPadding: current, event: { @@ -150,7 +148,7 @@ export function LoginScreen() { ); }); const keyboardHideSubscription = Keyboard.addListener(keyboardEvents.hide, () => { - setAndroidKeyboardHeight(current => + setKeyboardHeight(current => resolveAppAwareKeyboardPadding({ currentPadding: current, event: { type: 'keyboard-hidden' }, @@ -158,7 +156,7 @@ export function LoginScreen() { ); }); const appStateSubscription = AppState.addEventListener('change', appState => { - setAndroidKeyboardHeight(current => + setKeyboardHeight(current => resolveAppAwareKeyboardPadding({ currentPadding: current, event: { type: 'app-state-change', appState }, @@ -200,13 +198,17 @@ export function LoginScreen() { ); } - // RN 0.86 Android (ReactRootView.java) reports endCoordinates.height = - // imeInsets.bottom − barInsets.bottom (excludes the nav bar). endCoordinates.screenY - // is NOT the IME top under adjustResize, so full occlusion is - // endCoordinates.height + useSafeAreaInsets().bottom (= WindowInsets.ime().bottom; - // verified 704px + 63px = 767px on pixel9). Pad only when the keyboard is up so - // the resting layout is untouched. - const androidKeyboardPad = androidKeyboardHeight > 0 ? androidKeyboardHeight + insets.bottom : 0; + // One padded wrapper for both platforms: the bottom inset is reserved at + // rest, and while the IME is up the reported keyboard occlusion is resolved + // from the platform's metric origin (see `resolveKeyboardBottomPadding` for + // the capability each platform reports). The ScrollView's centered form then + // re-centres in the space that stays above the keyboard, so "Continue" is + // never left under the keyboard, the navigation bar, or the home indicator. + const bottomPadding = resolveKeyboardBottomPadding({ + keyboardHeight, + bottomInset: insets.bottom, + platform: Platform.OS, + }); // The Globe stays enabled on idle, denied, expired, and error (those render // an interactive IdleAuth form); it is disabled while a device-auth flow // (pending/approved) or a busy auth action owns the screen. @@ -214,163 +216,146 @@ export function LoginScreen() { const globeTrailing = I18nManager.isRTL ? { left: 16 } : { right: 16 }; return ( - // iOS: automaticallyAdjustKeyboardInsets only made the ScrollView scrollable, - // it never scrolls, and iOS only auto-reveals the focused field — so the - // centered form kept "Send code" under the keyboard on shorter devices - // (verified: iPhone 17 Pro, button centre 568pt vs keyboard window top 566pt, - // taps swallowed by UIRemoteKeyboardWindow). "padding" shrinks the ScrollView - // so the whole form re-centres in the space above the keyboard. - // - // Android: on API 35+ EDGE_TO_EDGE_ENFORCED the window never resizes for the - // IME, so KeyboardAvoidingView is inert (r0b: zero layout shift for the email - // IME). keyboardDidShow fires with real heights; the shell consumes them via - // the padding wrapper below. - - + - - - - {t('login.welcome')} - + + + {t('login.welcome')} + - {/* Branch fade animations parked mid-flight on remount — e1 measured 2/2 - iOS logout→login remounts washed out at ~50% alpha for 3+ minutes, - recovering only on relaunch — so these branches render without - animation; status swaps are instant. */} - - {status === 'idle' && draft === null && ( - <> - {/* Form-sized placeholder until the SecureStore draft restore finishes. */} - - - - )} + {/* Branch fade animations parked mid-flight on remount — e1 measured 2/2 + iOS logout→login remounts washed out at ~50% alpha for 3+ minutes, + recovering only on relaunch — so these branches render without + animation; status swaps are instant. */} + + {status === 'idle' && draft === null && ( + <> + {/* Form-sized placeholder until the SecureStore draft restore finishes. */} + + + + )} - {status === 'idle' && draft !== null && ( - - )} + {status === 'idle' && draft !== null && ( + + )} - {status === 'pending' && code && ( - - {resumed && ( - - {t('login.continuingSignIn')} - - )} + {status === 'pending' && code && ( + + {resumed && ( - {t('login.signInCode')} - - - {code} + {t('login.continuingSignIn')} - {/* Stack actions full-width so labels never clip side-by-side at max text */} - - - - + )} + + {t('login.signInCode')} + + + {code} + + {/* Stack actions full-width so labels never clip side-by-side at max text */} + - - )} - - {status === 'pending' && !code && ( - - - - {t('login.startingSignIn')} - - )} + + + )} - {(status === 'denied' || status === 'expired' || status === 'error') && ( - - - {errorMessage(status, error)} - - - - )} - - - { - setLanguagePickerBridge({ beforeReload: persistLoginDrafts }); - router.push('/(auth)/language-picker' as Href); - }} - disabled={globeDisabled} - hitSlop={8} - accessibilityRole="button" - accessibilityLabel={t('common.language')} - accessibilityState={{ disabled: globeDisabled }} - className="absolute h-11 w-11 items-center justify-center rounded-full active:opacity-70 disabled:opacity-50" - // eslint-disable-next-line react-native/no-inline-styles -- safe-area + RTL-aware trailing edge - style={{ top: insets.top + 8, ...globeTrailing }} - > - - - - + {status === 'pending' && !code && ( + + + + {t('login.startingSignIn')} + + + + )} + + {(status === 'denied' || status === 'expired' || status === 'error') && ( + + + {errorMessage(status, error)} + + + + )} + + + { + setLanguagePickerBridge({ beforeReload: persistLoginDrafts }); + router.push('/(auth)/language-picker' as Href); + }} + disabled={globeDisabled} + hitSlop={8} + accessibilityRole="button" + accessibilityLabel={t('common.language')} + accessibilityState={{ disabled: globeDisabled }} + className="absolute h-11 w-11 items-center justify-center rounded-full active:opacity-70 disabled:opacity-50" + // eslint-disable-next-line react-native/no-inline-styles -- safe-area + RTL-aware trailing edge + style={{ top: insets.top + 8, ...globeTrailing }} + > + + + ); } diff --git a/apps/mobile/src/components/pr-review/discussion/pr-comment-cta.test.tsx b/apps/mobile/src/components/pr-review/discussion/pr-comment-cta.test.tsx index 64ae3be6a9..82314e6b8d 100644 --- a/apps/mobile/src/components/pr-review/discussion/pr-comment-cta.test.tsx +++ b/apps/mobile/src/components/pr-review/discussion/pr-comment-cta.test.tsx @@ -23,6 +23,7 @@ vi.mock('react-i18next', async importOriginal => { }); const insetsState = vi.hoisted(() => ({ bottom: 0 })); +const platformState = vi.hoisted(() => ({ OS: 'ios' })); const keyboardSubscribers = vi.hoisted(() => ({ show: null as ((event: { endCoordinates: { height: number } }) => void) | null, hide: null as (() => void) | null, @@ -30,15 +31,15 @@ const keyboardSubscribers = vi.hoisted(() => ({ vi.mock('react-native', () => ({ View: 'View', - Platform: { OS: 'ios' }, + Platform: platformState, Keyboard: { addListener: vi.fn((event: string, listener: (event?: unknown) => void) => { - if (event === 'keyboardWillShow') { + if (event === 'keyboardWillShow' || event === 'keyboardDidShow') { keyboardSubscribers.show = listener as (event: { endCoordinates: { height: number }; }) => void; } - if (event === 'keyboardWillHide') { + if (event === 'keyboardWillHide' || event === 'keyboardDidHide') { keyboardSubscribers.hide = listener as () => void; } return { remove: vi.fn() }; @@ -110,6 +111,7 @@ function paddingValues(renderer: TestRenderer.ReactTestRenderer): number[] { describe('PrCommentCta', () => { beforeEach(() => { + platformState.OS = 'ios'; insetsState.bottom = 0; keyboardSubscribers.show = null; keyboardSubscribers.hide = null; @@ -156,6 +158,24 @@ describe('PrCommentCta', () => { expect(paddingValues(renderer)).toContain(336); }); + it("lifts by the raw Android metric, which the bar's own inset padding completes", () => { + // The bar's inner padding already includes the platform's bottom inset + // (`useDetailScreenBottomPadding`), so the lift must not add it a second + // time and float the button a navigation-bar height above the keyboard + // (2026-09-21 review finding). + platformState.OS = 'android'; + insetsState.bottom = 63; + const renderer = mountCta(); + if (!keyboardSubscribers.show) { + throw new Error('keyboard show listener was not registered'); + } + act(() => { + keyboardSubscribers.show?.({ endCoordinates: { height: 704 } }); + }); + expect(paddingValues(renderer)).toContain(704); + expect(paddingValues(renderer)).not.toContain(767); + }); + it('does not react to keyboard events at all while the lift is gated off', () => { // The host passes keyboardLift=false when another surface owns the // keyboard (the conversation-comment formSheet): the bar must not even diff --git a/apps/mobile/src/components/pr-review/discussion/pr-comment-cta.tsx b/apps/mobile/src/components/pr-review/discussion/pr-comment-cta.tsx index 67149e46e9..0edc9e967c 100644 --- a/apps/mobile/src/components/pr-review/discussion/pr-comment-cta.tsx +++ b/apps/mobile/src/components/pr-review/discussion/pr-comment-cta.tsx @@ -52,5 +52,13 @@ export function PrCommentCta({ onPress, keyboardLift }: PrCommentCtaProps) { ); // Unmounted (not just un-padded) while unfocused: the padding view's own // keyboard listener must not react to another surface's keyboard at all. - return keyboardLift ? {bar} : bar; + // The bar's inner padding already includes the platform's bottom inset + // (`useDetailScreenBottomPadding`), so `contentReservesBottomInset` keeps the + // lift from counting that inset a second time and floating the button a + // navigation-bar height above the keyboard. + return keyboardLift ? ( + {bar} + ) : ( + bar + ); } diff --git a/apps/mobile/src/components/tab-screen.tsx b/apps/mobile/src/components/tab-screen.tsx index ad0d4a5394..eb4cd291f5 100644 --- a/apps/mobile/src/components/tab-screen.tsx +++ b/apps/mobile/src/components/tab-screen.tsx @@ -1,3 +1,4 @@ +import { type Ref } from 'react'; import { Platform, ScrollView, type ScrollViewProps, useWindowDimensions } from 'react-native'; import { useSafeAreaInsets } from 'react-native-safe-area-context'; @@ -19,8 +20,9 @@ export function TabScreenScrollView({ children, style, refreshControl, + ref, ...props -}: ScrollViewProps) { +}: ScrollViewProps & { ref?: Ref }) { const paddingBottom = useTabBarBottomPadding(); // Reserve the tab bar's space in the layout: the bar is an absolute blur // overlay, and rows parked behind it read as clipped (b911 vr1 spot check, @@ -32,6 +34,7 @@ export function TabScreenScrollView({ return ( diff --git a/apps/mobile/src/lib/toast-offset.test.ts b/apps/mobile/src/lib/toast-offset.test.ts new file mode 100644 index 0000000000..c2665f67dc --- /dev/null +++ b/apps/mobile/src/lib/toast-offset.test.ts @@ -0,0 +1,93 @@ +// eslint-disable-next-line import/no-nodejs-modules -- vitest-only parity check, runs in node, never bundled into the app +import { readFileSync } from 'node:fs'; +// eslint-disable-next-line import/no-nodejs-modules -- vitest-only parity check, runs in node, never bundled into the app +import { fileURLToPath } from 'node:url'; + +import { describe, expect, it } from 'vitest'; + +import { + getToastBottomOffset, + MIN_BOTTOM_CHROME_HEIGHT, + TOAST_BOTTOM_GAP, +} from '@/lib/toast-offset'; + +const SOURCE = readFileSync(fileURLToPath(new URL('toast-offset.ts', import.meta.url)), 'utf8'); + +describe('getToastBottomOffset', () => { + it('clears the bottom chrome when the reported inset is smaller', () => { + // One rule for both platforms: a gesture-navigation inset (~24dp) is + // floored so the toast still clears the ~48dp of bottom chrome. + expect(getToastBottomOffset({ safeAreaBottom: 24, keyboardHeight: 0 })).toBe( + MIN_BOTTOM_CHROME_HEIGHT + TOAST_BOTTOM_GAP + ); + }); + + it('clears the bottom chrome when no inset is reported', () => { + expect(getToastBottomOffset({ safeAreaBottom: 0, keyboardHeight: 0 })).toBe( + MIN_BOTTOM_CHROME_HEIGHT + TOAST_BOTTOM_GAP + ); + }); + + it('keeps a larger reported inset, such as a taskbar', () => { + expect(getToastBottomOffset({ safeAreaBottom: 60, keyboardHeight: 0 })).toBe( + 60 + TOAST_BOTTOM_GAP + ); + }); + + it('ignores negative insets', () => { + expect(getToastBottomOffset({ safeAreaBottom: -10, keyboardHeight: 0 })).toBe( + MIN_BOTTOM_CHROME_HEIGHT + TOAST_BOTTOM_GAP + ); + }); + + it('raises the toast above the software keyboard', () => { + // `keyboardHeight` is the occlusion measured from the screen bottom, which + // the caller resolves with `resolveKeyboardBottomPadding`: Android's raw + // height stops at the navigation bar and must not reach this math. + expect(getToastBottomOffset({ safeAreaBottom: 24, keyboardHeight: 300 })).toBe( + 300 + TOAST_BOTTOM_GAP + ); + }); + + it('clears the floating tab bar while one is on screen', () => { + // The tab bar floats as an absolute overlay over the screen bottom, so the + // reported inset does not include it: the toast must clear the bar's full + // rendered height (2026-09-19 visual spot check, p1 — the error toast sat + // over the tab icons). + expect(getToastBottomOffset({ safeAreaBottom: 0, keyboardHeight: 0, tabBarHeight: 75 })).toBe( + 75 + TOAST_BOTTOM_GAP + ); + }); + + it('lets a tab bar taller than the bottom-chrome floor win', () => { + expect(getToastBottomOffset({ safeAreaBottom: 24, keyboardHeight: 0, tabBarHeight: 75 })).toBe( + 75 + TOAST_BOTTOM_GAP + ); + }); + + it('lets the software keyboard win over the tab bar', () => { + expect( + getToastBottomOffset({ safeAreaBottom: 24, keyboardHeight: 300, tabBarHeight: 75 }) + ).toBe(300 + TOAST_BOTTOM_GAP); + }); + + it('keeps the resting offset when no tab bar is on screen', () => { + expect(getToastBottomOffset({ safeAreaBottom: 24, keyboardHeight: 0, tabBarHeight: 0 })).toBe( + MIN_BOTTOM_CHROME_HEIGHT + TOAST_BOTTOM_GAP + ); + }); +}); + +describe('one implementation for both platforms', () => { + /** + * The offset decides every bottom-center toast, so it must not branch on the + * platform: iOS and Android run the same rule (the reported inset is floored, + * never replaced). A `Platform.OS`/`Platform.select` check, a platform-suffixed + * import, or a `'android'`/`'ios'` literal reaching the math fails here. + */ + it('keeps no per-platform branch in the shared offset module', () => { + expect(SOURCE).not.toMatch(/\bPlatform\.(?:OS|select|Version)\b/); + expect(SOURCE).not.toMatch(/from '[^']+\.(?:ios|android)'/); + expect(SOURCE).not.toMatch(/['"](?:android|ios)['"]/); + }); +}); diff --git a/apps/mobile/src/lib/toast-offset.ts b/apps/mobile/src/lib/toast-offset.ts new file mode 100644 index 0000000000..6be0241ab1 --- /dev/null +++ b/apps/mobile/src/lib/toast-offset.ts @@ -0,0 +1,68 @@ +/** + * Bottom-center toasts are anchored to the safe-area inset, but the reported + * inset is not a reliable floor for the chrome the platform draws over the + * app's own content. On Android it is only + * `WindowInsets.Type.navigationBars()`: an inset of `0` is reported whenever + * the window does not inset for the bar, and while a text input holds focus the + * IME's navigation row (hide-keyboard chevron / show-keyboard control) sits + * above the gesture bar without growing that inset. A toast anchored to the + * inset alone then lands with its last line under that chrome, which is how the + * manual-review error toast was captured with its final glyph row clipped at + * the bottom edge (2026-09-18 device finding). + * + * One floor serves both platforms rather than an Android-only branch: the + * tallest bottom chrome either platform draws is Android's navigation bar / IME + * navigation row (48dp) and iOS's home indicator (34pt), so flooring the + * reported inset at the taller of the two clears both with a single rule. A + * platform that reports a larger inset (taskbar, landscape) still wins, because + * the inset is floored, never replaced. + * + * The in-app tab bar is bottom chrome too, and taller than the navigation bar: + * it floats as an absolute overlay over the screen bottom, so the reported + * inset does not include it and a toast anchored to the inset landed over the + * tab icons (2026-09-19 visual spot check, p1). When a tab bar is on screen it + * wins as the tallest chrome; the keyboard still wins while it is up. + */ +export const MIN_BOTTOM_CHROME_HEIGHT = 48; + +/** sonner-native's own gap above the safe-area inset. */ +export const TOAST_BOTTOM_GAP = 8; + +/** + * Bottom offset, in logical pixels, for the bottom-center toast container. + * The keyboard occlusion wins while the software keyboard is up so the toast + * cannot hide behind it; otherwise the tallest bottom chrome decides: the + * floating tab bar when one is on screen, else the reported safe-area inset + * floored at `MIN_BOTTOM_CHROME_HEIGHT`. The standard gap always separates the + * toast from the chrome. + * + * This module reads no platform, so `keyboardHeight` is the keyboard's + * occlusion measured from the screen bottom, not a platform's raw metric: the + * caller resolves it with `resolveKeyboardBottomPadding`, because Android's + * reported height stops at the navigation bar while iOS's keyboard frame + * reaches the screen bottom. The container is anchored to the screen bottom, so + * a raw Android height would leave the toast's last line behind the IME's + * navigation row. + */ +export function getToastBottomOffset({ + safeAreaBottom, + keyboardHeight, + tabBarHeight = 0, +}: { + safeAreaBottom: number; + /** + * Keyboard occlusion measured from the screen bottom, `0` while the keyboard + * is down (see `resolveKeyboardBottomPadding`). + */ + keyboardHeight: number; + /** Rendered height of the floating tab bar while one is on screen, `0` otherwise. */ + tabBarHeight?: number; +}): number { + const bottomInset = Math.max(safeAreaBottom, 0); + const chrome = Math.max(bottomInset, MIN_BOTTOM_CHROME_HEIGHT); + const resting = chrome + TOAST_BOTTOM_GAP; + const overTabBar = tabBarHeight > 0 ? tabBarHeight + TOAST_BOTTOM_GAP : Number.NEGATIVE_INFINITY; + const withKeyboard = + keyboardHeight > 0 ? keyboardHeight + TOAST_BOTTOM_GAP : Number.NEGATIVE_INFINITY; + return Math.max(resting, overTabBar, withKeyboard); +} diff --git a/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts b/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts new file mode 100644 index 0000000000..4a2449800a --- /dev/null +++ b/apps/web/src/lib/code-reviews/manual-code-review-jobs.test.ts @@ -0,0 +1,230 @@ +import { TRPCError } from '@trpc/server'; +import { getHTTPStatusCodeFromError } from '@trpc/server/http'; + +// Ported from the closed #6405 (its `manual-code-review-jobs.test.ts`) onto the +// implementation kept in #6325, which maps every public-provider round-trip +// failure to a client error instead of letting tRPC answer 500. +const mockIsLocalCodeReviewDevelopmentEnabled = jest.fn(); +const mockGetAgentConfigForOwner = jest.fn(); +const mockAssertCouncilCreationAllowed = jest.fn(); +const mockCreateCodeReview = jest.fn(); +const mockTryDispatchPendingReviews = jest.fn(); + +jest.mock('@/lib/config.server', () => ({ + isLocalCodeReviewDevelopmentEnabled: () => mockIsLocalCodeReviewDevelopmentEnabled(), +})); + +jest.mock('@/lib/agent-config/db/agent-configs', () => ({ + getAgentConfigForOwner: (...args: unknown[]) => mockGetAgentConfigForOwner(...args), +})); + +jest.mock('./core/council-entitlement', () => ({ + assertCouncilCreationAllowed: (...args: unknown[]) => mockAssertCouncilCreationAllowed(...args), +})); + +jest.mock('./db/code-reviews', () => ({ + createCodeReview: (...args: unknown[]) => mockCreateCodeReview(...args), + findActiveProviderPublishingReview: jest.fn(), +})); + +jest.mock('./dispatch/dispatch-pending-reviews', () => ({ + tryDispatchPendingReviews: (...args: unknown[]) => mockTryDispatchPendingReviews(...args), +})); + +import { createManualCodeReviewJob } from './manual-code-review-jobs'; + +const OWNER = { type: 'user' as const, id: 'user-1', userId: 'user-1' }; + +const GITHUB_PR_URL = 'https://github.com/owner/repo/pull/123'; +const GITLAB_MR_URL = 'https://gitlab.com/group/project/-/merge_requests/123'; + +function providerResponse(status: number, body: unknown): Response { + return { + ok: status >= 200 && status < 300, + status, + text: async () => JSON.stringify(body), + json: async () => body, + } as unknown as Response; +} + +function taskInput(overrides: Record = {}) { + return { + platform: 'github' as const, + url: GITHUB_PR_URL, + modelSlug: 'test-model', + ...overrides, + }; +} + +async function captureError(overrides: Record = {}): Promise { + try { + await createManualCodeReviewJob({ owner: OWNER, input: taskInput(overrides) }); + return null; + } catch (error) { + return error; + } +} + +beforeEach(() => { + jest.clearAllMocks(); + mockIsLocalCodeReviewDevelopmentEnabled.mockReturnValue(true); + mockGetAgentConfigForOwner.mockResolvedValue(null); + mockAssertCouncilCreationAllowed.mockResolvedValue(undefined); +}); + +describe('createManualCodeReviewJob provider failures', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + // Every provider round-trip failure must surface as a mapped tRPC error. + // Before the mapping landed these escaped as raw + // ProviderFetchError/TypeError/ZodError and tRPC answered + // INTERNAL_SERVER_ERROR (HTTP 500) — the finding's defect. A provider that is + // genuinely unreachable maps to 502, which is the correct gateway status and + // not the reported internal error. + const cases: Array<{ + name: string; + fetch: () => void; + code: TRPCError['code']; + /** Input overrides for a case whose name names a provider other than GitHub. */ + input?: Record; + /** Copy that proves the case ran the provider its name names. */ + message?: string; + }> = [ + { + name: 'a missing public pull request maps to NOT_FOUND', + fetch: () => + void jest + .spyOn(global, 'fetch') + .mockResolvedValue(providerResponse(404, { message: 'Not Found' })), + code: 'NOT_FOUND', + }, + { + name: 'a rate-limited GitHub maps to TOO_MANY_REQUESTS', + fetch: () => + void jest + .spyOn(global, 'fetch') + .mockResolvedValue(providerResponse(403, { message: 'API rate limit exceeded' })), + code: 'TOO_MANY_REQUESTS', + }, + { + name: 'a rate-limited GitLab maps to TOO_MANY_REQUESTS', + // GitLab reports a rate limit with 429, so the case must run the GitLab + // path — the default input is a GitHub pull request. + input: { platform: 'gitlab', url: GITLAB_MR_URL }, + message: 'GitLab rate-limited', + fetch: () => + void jest + .spyOn(global, 'fetch') + .mockResolvedValue(providerResponse(429, { message: 'Too Many Requests' })), + code: 'TOO_MANY_REQUESTS', + }, + { + name: 'an unreachable provider maps to BAD_GATEWAY', + fetch: () => + void jest.spyOn(global, 'fetch').mockRejectedValue(new TypeError('fetch failed')), + code: 'BAD_GATEWAY', + }, + { + name: 'an unexpected provider shape maps to BAD_GATEWAY', + fetch: () => + void jest.spyOn(global, 'fetch').mockResolvedValue(providerResponse(200, { nope: true })), + code: 'BAD_GATEWAY', + }, + { + name: 'a provider error status maps to BAD_GATEWAY', + fetch: () => + void jest + .spyOn(global, 'fetch') + .mockResolvedValue(providerResponse(500, { message: 'Internal Server Error' })), + code: 'BAD_GATEWAY', + }, + ]; + + it.each(cases)('$name', async ({ fetch, code, input, message }) => { + fetch(); + + const error = await captureError(input); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe(code); + if (message) { + expect((error as TRPCError).message).toContain(message); + } + // The finding's defect was tRPC's unmapped INTERNAL_SERVER_ERROR / HTTP 500. + expect((error as TRPCError).code).not.toBe('INTERNAL_SERVER_ERROR'); + expect(getHTTPStatusCodeFromError(error as TRPCError)).not.toBe(500); + }); + + it('reports an unparseable provider response as unexpected, not unreachable', async () => { + jest.spyOn(global, 'fetch').mockResolvedValue({ + ok: true, + status: 200, + text: async () => 'not json', + json: async () => JSON.parse('not json'), + } as unknown as Response); + + const error = await captureError(); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe('BAD_GATEWAY'); + expect((error as TRPCError).message).toContain('unexpected response'); + expect((error as TRPCError).message).not.toContain('Could not reach'); + // The original parse error is kept as the cause rather than dropped. + expect(((error as TRPCError).cause as Error | undefined)?.message).toContain('JSON'); + }); + + it('names the GitLab merge request, never a pull request, in a GitLab failure', async () => { + jest.spyOn(global, 'fetch').mockResolvedValue(providerResponse(404, { message: 'Not Found' })); + + const error = await captureError({ platform: 'gitlab', url: GITLAB_MR_URL }); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe('NOT_FOUND'); + expect((error as TRPCError).message).toContain('GitLab'); + expect((error as TRPCError).message).toContain('merge request'); + expect((error as TRPCError).message).not.toContain('pull request'); + }); + + // GitLab returns 429 for rate limits; a 403 is a permission error and must not + // be reported to the user as a rate limit. + it('does not map a public GitLab 403 to a rate-limit error', async () => { + jest.spyOn(global, 'fetch').mockResolvedValue(providerResponse(403, { message: 'Forbidden' })); + + const error = await captureError({ platform: 'gitlab', url: GITLAB_MR_URL }); + + expect(error).toBeInstanceOf(TRPCError); + expect((error as TRPCError).code).toBe('BAD_GATEWAY'); + expect((error as TRPCError).message).toContain('unexpected response'); + expect((error as TRPCError).message).not.toContain('rate-limited'); + }); +}); + +describe('createManualCodeReviewJob happy path', () => { + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('creates the job from a public pull request and dispatches pending reviews', async () => { + jest.spyOn(global, 'fetch').mockResolvedValue( + providerResponse(200, { + number: 123, + html_url: GITHUB_PR_URL, + title: 'Fix the thing', + state: 'open', + draft: false, + user: { login: 'octocat', id: 1 }, + base: { ref: 'main', repo: { full_name: 'owner/repo' } }, + head: { ref: 'feature', sha: 'abc123' }, + }) + ); + mockCreateCodeReview.mockResolvedValue('review-1'); + + await expect(createManualCodeReviewJob({ owner: OWNER, input: taskInput() })).resolves.toEqual({ + reviewId: 'review-1', + outputMode: 'kilo', + }); + expect(mockTryDispatchPendingReviews).toHaveBeenCalledWith(OWNER); + }); +}); diff --git a/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts b/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts index 0636d765f2..ccade75136 100644 --- a/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts +++ b/apps/web/src/lib/code-reviews/manual-code-review-jobs.ts @@ -352,28 +352,100 @@ async function resolveLocalPublicSource( platform: CodeReviewPlatform, url: string ): Promise { - if (platform === PLATFORM.GITHUB) { - const parsed = parseGitHubPullRequestUrl(url); - const pullRequest = await fetchPublicGitHubPullRequest(parsed); - validateOpenGitHubPullRequest(pullRequest); - return buildGitHubSource(pullRequest, undefined); + try { + if (platform === PLATFORM.GITHUB) { + const parsed = parseGitHubPullRequestUrl(url); + const pullRequest = await fetchPublicGitHubPullRequest(parsed); + validateOpenGitHubPullRequest(pullRequest); + return buildGitHubSource(pullRequest, undefined); + } + + const parsed = parseGitLabMergeRequestUrl(url); + if (new URL(parsed.origin).hostname !== 'gitlab.com') { + throw new TRPCError({ + code: 'BAD_REQUEST', + message: 'Local Code Reviewer jobs only support public gitlab.com merge requests.', + }); + } + + const mergeRequest = await fetchPublicGitLabMergeRequest(parsed); + validateOpenGitLabMergeRequest(mergeRequest); + return buildGitLabSource({ + mergeRequest, + projectPath: parsed.projectPath, + integrationId: undefined, + platformProjectId: mergeRequest.target_project_id ?? mergeRequest.project_id, + }); + } catch (error) { + // The local (DEBUG_SHOW_DEV_UI) path reads the pull request from the provider's + // public API instead of a connected integration. A provider failure there used + // to escape as a raw Error, so tRPC answered 500 and the client showed a generic + // failure. Translate it into an actionable client error; the connected path + // already returns typed TRPCErrors. Existing TRPCErrors (invalid URL, closed or + // draft pull request) pass through unchanged. + throw toLocalSourceError(platform, error); } +} - const parsed = parseGitLabMergeRequestUrl(url); - if (new URL(parsed.origin).hostname !== 'gitlab.com') { - throw new TRPCError({ - code: 'BAD_REQUEST', - message: 'Local Code Reviewer jobs only support public gitlab.com merge requests.', +// `Response.json()` rejects with a SyntaxError built outside this realm, so +// `instanceof SyntaxError` misses it; match the error name instead. A ZodError +// from the response schema is built in this realm and matches directly. +function isProviderResponseParseError(error: unknown): boolean { + if (error instanceof z.ZodError) return true; + return ( + typeof error === 'object' && + error !== null && + 'name' in error && + Reflect.get(error, 'name') === 'SyntaxError' + ); +} + +function toLocalSourceError(platform: CodeReviewPlatform, error: unknown): TRPCError { + if (error instanceof TRPCError) return error; + + const provider = platform === PLATFORM.GITHUB ? 'GitHub' : 'GitLab'; + // GitHub calls these pull requests; GitLab calls them merge requests. + const requestNoun = platform === PLATFORM.GITHUB ? 'pull request' : 'merge request'; + if (error instanceof ProviderFetchError) { + if (error.status === 404) { + return new TRPCError({ + code: 'NOT_FOUND', + message: `${provider} could not find that ${requestNoun}. Check the URL, or make sure the repository is public.`, + cause: error, + }); + } + // GitHub signals primary and secondary rate limits with 403; GitLab uses 429. + // A GitLab 403 is a permission error, not a rate limit, so it falls through. + if (error.status === 429 || (error.status === 403 && platform === PLATFORM.GITHUB)) { + return new TRPCError({ + code: 'TOO_MANY_REQUESTS', + message: `${provider} rate-limited the request. Try again in a few minutes.`, + cause: error, + }); + } + return new TRPCError({ + code: 'BAD_GATEWAY', + message: `${provider} returned an unexpected response for that ${requestNoun}.`, + cause: error, + }); + } + + // The provider answered, but with a body that is not valid JSON or does not + // match its documented shape. We reached it, so "could not reach" would + // misdescribe what happened. + if (isProviderResponseParseError(error)) { + return new TRPCError({ + code: 'BAD_GATEWAY', + message: `${provider} returned an unexpected response for that ${requestNoun}.`, + cause: error, }); } - const mergeRequest = await fetchPublicGitLabMergeRequest(parsed); - validateOpenGitLabMergeRequest(mergeRequest); - return buildGitLabSource({ - mergeRequest, - projectPath: parsed.projectPath, - integrationId: undefined, - platformProjectId: mergeRequest.target_project_id ?? mergeRequest.project_id, + // Network failure, timeout, or redirect. + return new TRPCError({ + code: 'BAD_GATEWAY', + message: `Could not reach ${provider} to read that ${requestNoun}. Try again.`, + cause: error, }); } diff --git a/apps/web/src/routers/code-reviews-router.test.ts b/apps/web/src/routers/code-reviews-router.test.ts index c7d052524d..c849546f1c 100644 --- a/apps/web/src/routers/code-reviews-router.test.ts +++ b/apps/web/src/routers/code-reviews-router.test.ts @@ -691,6 +691,196 @@ describe('personalReviewAgent.createManualReviewJob', () => { userId: testUser.id, }); }); + + // The local (DEBUG_SHOW_DEV_UI) path reads the pull request from the provider's + // public API. A provider failure there used to escape as a raw Error, so tRPC + // answered 500 and the client showed a generic failure. Each case below must + // resolve to an actionable client error code instead of INTERNAL_SERVER_ERROR. + it.each([ + { status: 404, expectedCode: 'NOT_FOUND', expectedMessage: 'could not find that pull request' }, + { + status: 403, + expectedCode: 'TOO_MANY_REQUESTS', + expectedMessage: 'rate-limited the request', + }, + { status: 500, expectedCode: 'BAD_GATEWAY', expectedMessage: 'unexpected response' }, + ])( + 'maps a public GitHub pull request fetch that fails with $status to $expectedCode', + async ({ status, expectedCode, expectedMessage }) => { + fetchSpy?.mockImplementation(async () => new Response('provider failure', { status })); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'github', + url: prUrl, + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string } + ); + + expect(rejection.code).toBe(expectedCode); + expect(rejection.code).not.toBe('INTERNAL_SERVER_ERROR'); + expect(rejection.message).toContain(expectedMessage); + } + ); + + it('maps a missing public GitLab merge request to an actionable GitLab error', async () => { + fetchSpy?.mockImplementation(async () => new Response('Not Found', { status: 404 })); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'gitlab', + url: 'https://gitlab.com/group/project/-/merge_requests/1', + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string } + ); + + expect(rejection.code).toBe('NOT_FOUND'); + expect(rejection.message).toContain('GitLab'); + expect(rejection.message).toContain('merge request'); + expect(rejection.message).not.toContain('pull request'); + }); + + it('maps a public GitLab rate limit to TOO_MANY_REQUESTS', async () => { + fetchSpy?.mockImplementation(async () => new Response('Too Many Requests', { status: 429 })); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'gitlab', + url: 'https://gitlab.com/group/project/-/merge_requests/1', + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string } + ); + + expect(rejection.code).toBe('TOO_MANY_REQUESTS'); + expect(rejection.message).toContain('rate-limited the request'); + }); + + // GitLab returns 429 for rate limits; a 403 is a permission error and must not + // be reported to the user as a rate limit. + it('does not map a public GitLab 403 to a rate-limit error', async () => { + fetchSpy?.mockImplementation(async () => new Response('Forbidden', { status: 403 })); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'gitlab', + url: 'https://gitlab.com/group/project/-/merge_requests/1', + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string } + ); + + expect(rejection.code).toBe('BAD_GATEWAY'); + expect(rejection.message).toContain('unexpected response'); + expect(rejection.message).not.toContain('rate-limited'); + }); + + // A provider that answers with an unparseable body was reached; the error must + // say so rather than claiming the provider was unreachable, and keep the cause. + it('reports an unparseable provider response as unexpected, not unreachable', async () => { + fetchSpy?.mockImplementation( + async () => + new Response('not json', { status: 200, headers: { 'Content-Type': 'application/json' } }) + ); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'github', + url: prUrl, + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string; cause?: unknown } + ); + + expect(rejection.code).toBe('BAD_GATEWAY'); + expect(rejection.message).toContain('unexpected response'); + expect(rejection.message).not.toContain('Could not reach'); + // The original parse error is kept as the cause rather than dropped. + expect((rejection.cause as Error | undefined)?.message).toContain('JSON'); + }); + + // A well-formed JSON body that does not match the provider's documented shape + // is also a reached-but-unusable response, not an unreachable provider. + it('reports a provider response that fails schema validation as unexpected', async () => { + fetchSpy?.mockImplementation( + async () => + new Response(JSON.stringify({ unexpected: true }), { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }) + ); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'github', + url: prUrl, + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string; cause?: unknown } + ); + + expect(rejection.code).toBe('BAD_GATEWAY'); + expect(rejection.message).toContain('unexpected response'); + expect(rejection.message).not.toContain('Could not reach'); + expect(rejection.cause).toBeDefined(); + }); + + it('maps a provider network failure to a client error instead of a 500', async () => { + const networkError = new TypeError('fetch failed'); + fetchSpy?.mockImplementation(async () => { + throw networkError; + }); + const caller = await createCallerForUser(testUser.id); + + const rejection = await caller.personalReviewAgent + .createManualReviewJob({ + platform: 'github', + url: prUrl, + modelSlug: 'test-model', + }) + .then( + () => { + throw new Error('Expected createManualReviewJob to reject'); + }, + error => error as { code?: string; message?: string; cause?: unknown } + ); + + expect(rejection.code).toBe('BAD_GATEWAY'); + expect(rejection.message).toContain('Could not reach GitHub'); + expect(rejection.cause).toBe(networkError); + }); }); describe('review agent config REVIEW.md setting', () => {