diff --git a/apps/mobile/src/components/code-reviewer/provider-connect-card.tsx b/apps/mobile/src/components/code-reviewer/provider-connect-card.tsx index 0fbfc35d80..6998f2a05d 100644 --- a/apps/mobile/src/components/code-reviewer/provider-connect-card.tsx +++ b/apps/mobile/src/components/code-reviewer/provider-connect-card.tsx @@ -1,7 +1,7 @@ import { GitBranch, GitMerge } from '@/components/ui/icons'; -import { useCallback, useState } from 'react'; +import { useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { Platform, View } from 'react-native'; +import { View } from 'react-native'; import { ActivityIndicator } from '@/components/ui/activity-indicator'; import { toast } from 'sonner-native'; @@ -9,7 +9,6 @@ import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; import { getGitHubIntegrationUrl } from '@/lib/agent-github-integration'; import { WEB_BASE_URL } from '@/lib/config'; -import { useExternalAuthReturn } from '@/lib/external-auth/use-external-auth-return'; import { PERSONAL_SCOPE } from '@/lib/hooks/use-code-reviewer'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { getGitLabIntegrationUrl } from '@/lib/integration-urls'; @@ -47,11 +46,6 @@ export function ProviderConnectCard({ const [connecting, setConnecting] = useState(false); const { icon: Icon, label, buttonLabel, getUrl, errorMessage } = PLATFORM_CONFIG[platform]; - const handleConnected = useCallback(() => { - void onConnected(); - }, [onConnected]); - const { markLaunched, clearLaunch } = useExternalAuthReturn(handleConnected); - const connect = async () => { setConnecting(true); try { @@ -64,17 +58,9 @@ export function ProviderConnectCard({ }); url = getGitHubIntegrationUrl(WEB_BASE_URL, orgId, result.token); } - markLaunched(); - const trigger = await openAuthorizationAndWaitForReturn(Platform.OS, url); - if (trigger === 'sheet-close') { - // iOS: the auth session resolves on sheet close — refresh right here. - clearLaunch(); - await onConnected(); - } - // Android: onConnected runs from the foreground handler once the app - // returns from the plain browser. + await openAuthorizationAndWaitForReturn(url); + await onConnected(); } catch { - clearLaunch(); toast.error(t(errorMessage)); } finally { setConnecting(false); diff --git a/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.mounted.test.tsx b/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.mounted.test.tsx new file mode 100644 index 0000000000..3b521b66c9 --- /dev/null +++ b/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.mounted.test.tsx @@ -0,0 +1,330 @@ +import * as React from 'react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type * as ReactI18next from 'react-i18next'; +import * as WebBrowser from 'expo-web-browser'; +import { toast } from 'sonner-native'; + +import '@/i18n'; +import { act, TestRenderer } from '@/test/renderer'; +import { type ProviderPrPlatform } from '@/lib/pr-review/provider-pr-ref'; +import { PrReviewConnectGate } from './pr-review-connect-gate'; + +const platform = vi.hoisted(() => ({ OS: 'ios' })); +const appState = vi.hoisted(() => ({ + subscriptions: [] as { + remove: ReturnType; + listener: (state: string) => void; + }[], + addEventListener: vi.fn(), +})); +const providers: ProviderPrPlatform[] = ['github', 'gitlab', 'bitbucket']; +let queryResult = { + data: { connected: false, revoked: false }, + isPending: false, + isLoading: false, + isError: false, + isFetching: false, + refetch: vi.fn(), +}; +const connectMutateAsync = vi.fn<() => Promise<{ authorizationUrl: string }>>(); + +vi.mock('react-i18next', async importOriginal => { + const actual = await importOriginal(); + return { + ...actual, + useTranslation: () => { + const i18n = actual.getI18n(); + return { t: i18n.t.bind(i18n), i18n }; + }, + }; +}); +vi.mock('@tanstack/react-query', () => ({ + useQuery: () => queryResult, + useMutation: () => ({ mutateAsync: connectMutateAsync, isError: false }), + useQueryClient: () => ({ invalidateQueries: vi.fn() }), +})); +vi.mock('@/lib/trpc', () => ({ + useTRPC: () => ({ + githubApps: { + getUserAuthorization: { queryOptions: () => ({}), queryKey: () => [] }, + connectUserAuthorization: { mutationOptions: () => ({}) }, + }, + organizations: { + reviewAgent: { + getGitLabStatus: { queryOptions: () => ({}) }, + getBitbucketReadiness: { queryOptions: () => ({}) }, + }, + }, + personalReviewAgent: { getGitLabStatus: { queryOptions: () => ({}) } }, + }), +})); +vi.mock('@/lib/config', () => ({ WEB_BASE_URL: 'https://web.example' })); +vi.mock('expo-router', () => ({ usePathname: () => '/pr-review/github/owner/repo/1' })); +vi.mock('@/lib/hooks/use-theme-colors', () => ({ + useThemeColors: () => ({ mutedForeground: '#6F6A61', primaryForeground: '#FFFFFF' }), +})); +vi.mock('sonner-native', () => ({ toast: { error: vi.fn() } })); +vi.mock('expo-web-browser', () => ({ + openAuthSessionAsync: vi.fn(), + openBrowserAsync: vi.fn(), + WebBrowserResultType: { OPENED: 'opened', DISMISS: 'dismiss', CANCEL: 'cancel' }, +})); +vi.mock('@/components/ui/icons', () => ({ + PlugZap: 'PlugZap', + RefreshCcw: 'RefreshCcw', + ShieldAlert: 'ShieldAlert', +})); +vi.mock('@/components/icons/github-icon', () => ({ GitHubIcon: 'GitHubIcon' })); +vi.mock('@/components/centered-state', () => ({ CenteredState: 'CenteredState' })); +vi.mock('@/components/empty-state', () => ({ + EmptyState: (props: { action?: React.ReactNode }) => + React.createElement('EmptyState', props, props.action), +})); +vi.mock('@/components/query-error', () => ({ QueryError: 'QueryError' })); +vi.mock('@/components/screen-header', () => ({ ScreenHeader: 'ScreenHeader' })); +vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); +vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); +vi.mock('@/components/ui/activity-indicator', () => ({ ActivityIndicator: 'ActivityIndicator' })); +vi.mock('react-native', () => ({ + AppState: { addEventListener: appState.addEventListener }, + Platform: platform, + View: 'View', +})); + +function emitForeground() { + for (const subscription of appState.subscriptions) { + subscription.listener('active'); + } +} + +/** Begins a launch that stays open until the test settles it. */ +function beginPendingLaunch() { + if (platform.OS === 'android') { + const pending = Promise.withResolvers(); + vi.mocked(WebBrowser.openBrowserAsync).mockReturnValueOnce(pending.promise); + return { + settle: () => { + pending.resolve({ type: WebBrowser.WebBrowserResultType.OPENED }); + }, + }; + } + const pending = Promise.withResolvers(); + vi.mocked(WebBrowser.openAuthSessionAsync).mockReturnValueOnce(pending.promise); + return { + settle: () => { + pending.resolve({ type: WebBrowser.WebBrowserResultType.CANCEL }); + }, + }; +} + +/** Settles the launch as a successful return: sheet close (iOS) / foreground (Android). */ +async function settleLaunch(pending: ReturnType) { + await act(async () => { + pending.settle(); + await Promise.resolve(); + if (platform.OS === 'android') { + emitForeground(); + } + await Promise.resolve(); + }); +} + +function failLaunch(error: Error) { + if (platform.OS === 'android') { + vi.mocked(WebBrowser.openBrowserAsync).mockRejectedValueOnce(error); + } else { + vi.mocked(WebBrowser.openAuthSessionAsync).mockRejectedValueOnce(error); + } +} + +let mounted: TestRenderer.ReactTestRenderer | undefined = undefined; +function gate(provider: ProviderPrPlatform, organizationId: string | null = 'org-1') { + return ( + + {React.createElement('ReviewContent')} + + ); +} +function mount(provider: ProviderPrPlatform, organizationId: string | null = 'org-1') { + act(() => { + mounted = TestRenderer.create(gate(provider, organizationId)); + }); + if (!mounted) { + throw new Error('gate did not mount'); + } + return mounted; +} + +beforeEach(() => { + vi.resetAllMocks(); + appState.subscriptions.length = 0; + appState.addEventListener.mockImplementation( + (_event: string, listener: (state: string) => void) => { + const subscription = { remove: vi.fn(), listener }; + appState.subscriptions.push(subscription); + return { remove: subscription.remove }; + } + ); + queryResult = { + data: { connected: false, revoked: false }, + isPending: false, + isLoading: false, + isError: false, + isFetching: false, + refetch: vi.fn().mockResolvedValue(undefined), + }; + connectMutateAsync.mockResolvedValue({ + authorizationUrl: 'https://github.com/login/oauth/authorize', + }); +}); +afterEach(() => { + mounted?.unmount(); + mounted = undefined; +}); + +describe('Android connect gate unmount', () => { + it.each(providers)('%s drops its foreground listener and pending callbacks', async provider => { + platform.OS = 'android'; + vi.mocked(WebBrowser.openBrowserAsync).mockResolvedValue({ + type: WebBrowser.WebBrowserResultType.OPENED, + }); + const renderer = mount(provider); + await act(async () => { + (renderer.root.findByType('Button').props.onPress as () => void)(); + await Promise.resolve(); + }); + expect(appState.subscriptions).toHaveLength(1); + + await act(async () => { + renderer.unmount(); + await Promise.resolve(); + }); + mounted = undefined; + expect(appState.subscriptions[0]?.remove).toHaveBeenCalledOnce(); + await act(async () => { + emitForeground(); + await Promise.resolve(); + }); + expect(queryResult.refetch).not.toHaveBeenCalled(); + expect(toast.error).not.toHaveBeenCalled(); + }); +}); + +describe.each(['ios', 'android'])('PR-review connect gate on %s', os => { + beforeEach(() => { + platform.OS = os; + }); + + it.each(providers)('%s waits for the launch to finish before refreshing', async provider => { + const pending = beginPendingLaunch(); + const renderer = mount(provider); + const button = renderer.root.findByType('Button'); + expect(button.props.disabled).toBe(false); + + await act(async () => { + (button.props.onPress as () => void)(); + await Promise.resolve(); + }); + expect(button.props.disabled).toBe(true); + expect(renderer.root.findAllByType('ActivityIndicator')).toHaveLength(1); + expect(queryResult.refetch).not.toHaveBeenCalled(); + + await settleLaunch(pending); + expect(queryResult.refetch).toHaveBeenCalledOnce(); + expect(button.props.disabled).toBe(false); + expect(toast.error).not.toHaveBeenCalled(); + }); + + it.each(providers)( + '%s reports a failed launch, re-enables Connect, and refreshes once after retry', + async provider => { + failLaunch(new Error('no browser')); + const renderer = mount(provider); + const button = renderer.root.findByType('Button'); + + await act(async () => { + (button.props.onPress as () => void)(); + await Promise.resolve(); + }); + expect(toast.error).toHaveBeenCalledExactlyOnceWith( + 'Could not open browser. Please try again.' + ); + expect(button.props.disabled).toBe(false); + expect(renderer.root.findAllByType('ActivityIndicator')).toHaveLength(0); + expect(queryResult.refetch).not.toHaveBeenCalled(); + + // The failed launch must leave no listener that could stray-refetch, so + // the retry below is the only thing that refreshes. + emitForeground(); + expect(queryResult.refetch).not.toHaveBeenCalled(); + + const retry = beginPendingLaunch(); + await act(async () => { + (button.props.onPress as () => void)(); + await Promise.resolve(); + }); + expect(button.props.disabled).toBe(true); + + await settleLaunch(retry); + expect(queryResult.refetch).toHaveBeenCalledOnce(); + expect(toast.error).toHaveBeenCalledOnce(); + expect(button.props.disabled).toBe(false); + expect(renderer.root.findAllByType('EmptyState')).toHaveLength(1); + + queryResult.data.connected = true; + act(() => { + renderer.update(gate(provider)); + }); + expect(renderer.root.findAllByType('ReviewContent')).toHaveLength(1); + expect(renderer.root.findAllByType('Button')).toHaveLength(0); + } + ); + + it.each(providers)('%s shows loading, not Connect, for a paused status query', provider => { + queryResult.isPending = true; + const renderer = mount(provider); + expect(renderer.root.findAllByType('ActivityIndicator')).toHaveLength(1); + expect(renderer.root.findAllByType('EmptyState')).toHaveLength(0); + }); + + it.each(providers)('%s offers a working Retry for status failures', async provider => { + queryResult.isError = true; + const renderer = mount(provider); + await act(async () => { + (renderer.root.findByType('QueryError').props.onRetry as () => void)(); + await Promise.resolve(); + }); + expect(queryResult.refetch).toHaveBeenCalledOnce(); + }); + + it.each(providers)( + '%s handles a rejected return refetch without a browser error', + async provider => { + queryResult.refetch.mockRejectedValue(new Error('status refetch failed')); + const pending = beginPendingLaunch(); + const renderer = mount(provider); + const button = renderer.root.findByType('Button'); + await act(async () => { + (button.props.onPress as () => void)(); + await Promise.resolve(); + }); + await settleLaunch(pending); + expect(queryResult.refetch).toHaveBeenCalledOnce(); + expect(button.props.disabled).toBe(false); + expect(toast.error).not.toHaveBeenCalled(); + } + ); + + it('explains the personal Bitbucket restriction without a Connect CTA', () => { + const renderer = mount('bitbucket', null); + expect(renderer.root.findByType('EmptyState').props.description).toBeTruthy(); + expect(renderer.root.findAllByType('Button')).toHaveLength(0); + expect(WebBrowser.openAuthSessionAsync).not.toHaveBeenCalled(); + }); + + it('shows the GitHub icon in the Connect GitHub action', () => { + const renderer = mount('github'); + expect(renderer.root.findAllByType('GitHubIcon')).toHaveLength(1); + expect(renderer.root.findAllByType('RefreshCcw')).toHaveLength(0); + }); +}); diff --git a/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.test.ts b/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.test.ts deleted file mode 100644 index b3aa79454d..0000000000 --- a/apps/mobile/src/components/pr-review/pr-review-connect-gate-view.test.ts +++ /dev/null @@ -1,170 +0,0 @@ -import * as React from 'react'; -import { describe, expect, it, vi } from 'vitest'; - -import type * as ReactI18next from 'react-i18next'; -import '@/i18n'; -import { PrReviewConnectGate } from './pr-review-connect-gate'; - -vi.mock('react-i18next', async importOriginal => { - const actual = await importOriginal(); - return { - ...actual, - useTranslation: () => { - const i18n = actual.getI18n(); - return { t: i18n.t.bind(i18n), i18n }; - }, - }; -}); - -// The gate passes `authorization.isPending` (no data yet) to the view -// selector, not `isLoading` (isPending && isFetching). A paused query -// (offline/unknown connectivity, empty cache) is pending but not fetching, -// so `isLoading` is false and the gate would otherwise fall through to -// Connect on a cold launch before NetInfo settles. This pins that wiring: -// a revert to `isLoading` would make the paused query render Connect and -// fail the assertions below. -// -// The view selector itself lives in `@/lib/pr-review/pr-review-connect-gate-view` -// with its decision table beside it. -// -// Rendered as a plain function call (same pattern as pr-review-screen.test.tsx) -// with hooks and child components stubbed so the tree walk stays deterministic. - -let authorizationQueryResult = { - data: undefined as unknown, - isPending: true, - isLoading: false, - isError: false, - isFetching: false, - refetch: vi.fn(), -}; - -vi.mock('react', async () => { - const actual = await vi.importActual('react'); - return { - ...actual, - useCallback: vi.fn( unknown>(fn: T) => fn), - useState: vi.fn((initial: T) => [initial, vi.fn() as () => void] as [T, (value: T) => void]), - useRef: vi.fn((initial: T) => ({ current: initial })), - useEffect: vi.fn(), - }; -}); - -vi.mock('@tanstack/react-query', () => ({ - useQuery: () => authorizationQueryResult, - useMutation: () => ({ mutateAsync: vi.fn() }), - useQueryClient: () => ({ invalidateQueries: vi.fn() }), -})); - -vi.mock('@/lib/trpc', () => ({ - useTRPC: () => ({ - githubApps: { - getUserAuthorization: { queryOptions: () => ({}), queryKey: () => [] }, - connectUserAuthorization: { mutationOptions: () => ({}) }, - }, - }), -})); - -vi.mock('@/lib/config', () => ({ WEB_BASE_URL: 'https://web.example' })); - -vi.mock('expo-router', () => ({ - // Any non-entry pathname reaches the GitHub arm. - usePathname: () => '/pr-review/github/owner/repo/1', -})); - -vi.mock('@/lib/hooks/use-theme-colors', () => ({ - useThemeColors: () => ({ mutedForeground: '#6F6A61', primaryForeground: '#FFFFFF' }), -})); - -vi.mock('react-native-safe-area-context', () => ({ - useSafeAreaInsets: () => ({ bottom: 0 }), -})); - -vi.mock('sonner-native', () => ({ toast: { error: vi.fn() } })); - -vi.mock('@/lib/pr-review/connect-gate-platform', () => ({ - openAuthorizationAndWaitForReturn: vi.fn(), -})); - -vi.mock('@/components/ui/icons', () => ({ - PlugZap: 'PlugZap', - RefreshCcw: 'RefreshCcw', - ShieldAlert: 'ShieldAlert', -})); - -vi.mock('@/components/icons/github-icon', () => ({ GitHubIcon: 'GitHubIcon' })); - -vi.mock('@/components/centered-state', () => ({ CenteredState: 'CenteredState' })); -vi.mock('@/components/empty-state', () => ({ EmptyState: 'EmptyState' })); -vi.mock('@/components/query-error', () => ({ QueryError: 'QueryError' })); -vi.mock('@/components/screen-header', () => ({ ScreenHeader: 'ScreenHeader' })); -vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); -vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); - -vi.mock('@/components/ui/activity-indicator', () => ({ ActivityIndicator: 'ActivityIndicator' })); -vi.mock('react-native', () => ({ - ActivityIndicator: 'ActivityIndicator', - AppState: { addEventListener: vi.fn(() => ({ remove: vi.fn() })) }, - Platform: { OS: 'ios' }, - View: 'View', -})); - -function containsType(node: unknown, type: string): boolean { - if (Array.isArray(node)) { - return node.some(child => containsType(child, type)); - } - if (React.isValidElement(node)) { - const element = node; - if (element.type === type) { - return true; - } - // A function component element (the gate dispatches to GitHubConnectGate) - // is walked by calling it: hooks are stubbed, so a plain call renders. - if ( - typeof element.type === 'function' && - containsType((element.type as (props: unknown) => unknown)(element.props), type) - ) { - return true; - } - return Object.values(element.props as Record).some(value => - containsType(value, type) - ); - } - return false; -} - -describe('PrReviewConnectGate wiring', () => { - it('shows loading, not Connect, for a paused authorization query with no data', () => { - authorizationQueryResult = { - data: undefined, - isPending: true, - isLoading: false, - isError: false, - isFetching: false, - refetch: vi.fn(), - }; - - // eslint-disable-next-line new-cap - const tree = PrReviewConnectGate({ children: null }); - - expect(containsType(tree, 'ActivityIndicator')).toBe(true); - expect(containsType(tree, 'EmptyState')).toBe(false); - }); - - it('shows the GitHub icon in the Connect GitHub action', () => { - authorizationQueryResult = { - data: { connected: false, revoked: false }, - isPending: false, - isLoading: false, - isError: false, - isFetching: false, - refetch: vi.fn(), - }; - - // eslint-disable-next-line new-cap - const tree = PrReviewConnectGate({ children: null }); - - expect(containsType(tree, 'GitHubIcon')).toBe(true); - expect(containsType(tree, 'RefreshCcw')).toBe(false); - }); -}); diff --git a/apps/mobile/src/components/pr-review/pr-review-connect-gate.tsx b/apps/mobile/src/components/pr-review/pr-review-connect-gate.tsx index f0bbabdcb3..dc0b5467fa 100644 --- a/apps/mobile/src/components/pr-review/pr-review-connect-gate.tsx +++ b/apps/mobile/src/components/pr-review/pr-review-connect-gate.tsx @@ -1,8 +1,8 @@ import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { PlugZap, RefreshCcw, ShieldAlert } from '@/components/ui/icons'; -import { type ReactNode, useCallback, useMemo, useState } from 'react'; +import { type ReactNode, useEffect, useMemo, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; -import { Platform, View } from 'react-native'; +import { View } from 'react-native'; import { ActivityIndicator } from '@/components/ui/activity-indicator'; import { usePathname } from 'expo-router'; import { CenteredState } from '@/components/centered-state'; @@ -17,8 +17,7 @@ import { Text } from '@/components/ui/text'; import { WEB_BASE_URL } from '@/lib/config'; import { getBitbucketIntegrationUrl, getGitLabIntegrationUrl } from '@/lib/integration-urls'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; -import { useExternalAuthReturn } from '@/lib/external-auth/use-external-auth-return'; -import { openAuthorizationAndWaitForReturn } from '@/lib/pr-review/connect-gate-platform'; +import { launchConnectGateBrowser } from '@/lib/pr-review/connect-gate-platform'; import { selectPrReviewGateView } from '@/lib/pr-review/pr-review-connect-gate-view'; import { type ProviderPrPlatform } from '@/lib/pr-review/provider-pr-ref'; import { useTRPC } from '@/lib/trpc'; @@ -65,9 +64,7 @@ type PrReviewConnectGateProps = { * pass-through there; the provider gates protect each detail route. * * The GitHub CTA calls `githubApps.connectUserAuthorization` and opens the - * returned URL with the platform-appropriate browser launcher (iOS native - * auth session that resolves on sheet close; Android custom tab that - * resolves on app-foreground via AppState). Cancellation on either platform + * returned URL with the shared auth-session launcher. Cancellation on either platform * simply leaves the gate showing — there's nothing to roll back because the * auth flow is server-driven. */ @@ -107,45 +104,32 @@ function GitHubConnectGate({ children }: Readonly<{ children: ReactNode }>) { }) ); - // Track the in-flight launch so a stale AppState 'active' transition - // (from the user backgrounding the app before tapping Connect) doesn't - // trigger a refetch on its own. iOS: openAuthSessionAsync already resolves - // on sheet close, so we await it and refetch right there. Android: - // openBrowserAsync is fire-and-forget, so the hook refetches on AppState - // returning to 'active'. - const refetchAuthorization = useCallback(() => { - void authorization.refetch(); - }, [authorization]); - const { markLaunched, clearLaunch } = useExternalAuthReturn(refetchAuthorization); const [connecting, setConnecting] = useState(false); + const connectAbort = useRef(null); + useEffect(() => () => connectAbort.current?.abort(), []); const handleConnect = async () => { + const controller = new AbortController(); + connectAbort.current = controller; setConnecting(true); try { const result = await connect.mutateAsync(); - markLaunched(); - const trigger = await openAuthorizationAndWaitForReturn(Platform.OS, result.authorizationUrl); - if (trigger === 'sheet-close') { - // iOS: refetch immediately. Clear the launch sentinel so the - // AppState handler (if it ever fires) doesn't double-refetch. - clearLaunch(); - await authorization.refetch(); - await queryClient.invalidateQueries({ - queryKey: trpc.githubApps.getUserAuthorization.queryKey(), - }); - } - // Android: refetch is handled by the AppState listener when the app - // returns to foreground. `openBrowserAsync` resolves as soon as the - // browser is launched, so we must NOT clear the sentinel here — the - // foreground handler clears it once it has consumed it. + await launchConnectGateBrowser(result.authorizationUrl, { + signal: controller.signal, + onOpenFailure: () => void toast.error(t('authErrors.couldNotOpenBrowser')), + onReturn: async () => { + await authorization.refetch(); + await queryClient.invalidateQueries({ + queryKey: trpc.githubApps.getUserAuthorization.queryKey(), + }); + }, + }); } catch { - // mutateAsync already toasted; the openAuthorizationAndWaitForReturn - // rejection means the browser failed to open — clear the sentinel so - // a later unrelated foreground doesn't trigger a stray refetch, and - // keep the gate showing. - clearLaunch(); + // mutateAsync toasted the server error; keep the gate showing. } finally { - setConnecting(false); + if (!controller.signal.aborted) { + setConnecting(false); + } } }; @@ -276,37 +260,38 @@ function ProviderConnectGate({ }); const status = platform === 'gitlab' ? gitlabStatus : bitbucketStatus; - const refetchStatus = useCallback(() => { - void status.refetch(); - }, [status]); - const { markLaunched, clearLaunch } = useExternalAuthReturn(refetchStatus); const [connecting, setConnecting] = useState(false); + const connectAbort = useRef(null); + useEffect(() => () => connectAbort.current?.abort(), []); const handleConnect = async () => { + const controller = new AbortController(); + connectAbort.current = controller; setConnecting(true); try { // The provider connections are web-side integrations: open the // existing integration page and re-check the status when the app - // returns (pattern: `openAuthorizationAndWaitForReturn`). - markLaunched(); + // returns (pattern: `launchConnectGateBrowser`). const integrationUrl = platform === 'gitlab' ? getGitLabIntegrationUrl(WEB_BASE_URL, organizationId ?? undefined) : getBitbucketIntegrationUrl(WEB_BASE_URL, organizationId ?? ''); - const trigger = await openAuthorizationAndWaitForReturn(Platform.OS, integrationUrl); - if (trigger === 'sheet-close') { - clearLaunch(); - await status.refetch(); - } - // Android: the AppState listener in `useExternalAuthReturn` refetches - // when the app returns to foreground; the sentinel stays set until it - // consumes the launch. + await launchConnectGateBrowser(integrationUrl, { + signal: controller.signal, + onOpenFailure: () => void toast.error(t('authErrors.couldNotOpenBrowser')), + onReturn: async () => { + await status.refetch(); + }, + }); } catch { - // The browser failed to open — clear the sentinel so a later unrelated - // foreground doesn't trigger a stray refetch, and keep the gate showing. - clearLaunch(); + // The helper reports a failed launch itself, so a rejected `onReturn` + // refetch is the only error that reaches here. Keep the gate showing: + // the query's error state renders the retryable + // QueryError instead of letting `void handleConnect()` reject. } finally { - setConnecting(false); + if (!controller.signal.aborted) { + setConnecting(false); + } } }; diff --git a/apps/mobile/src/components/security-agent/security-agent-setup.mounted.test.tsx b/apps/mobile/src/components/security-agent/security-agent-setup.mounted.test.tsx index 18669c810c..bf3e705a54 100644 --- a/apps/mobile/src/components/security-agent/security-agent-setup.mounted.test.tsx +++ b/apps/mobile/src/components/security-agent/security-agent-setup.mounted.test.tsx @@ -20,9 +20,6 @@ vi.mock('@/components/centered-state', () => ({ CenteredState: 'CenteredState' } vi.mock('@/components/ui/button', () => ({ Button: 'Button' })); vi.mock('@/components/ui/text', () => ({ Text: 'Text' })); vi.mock('@/lib/hooks/use-theme-colors', () => ({ useThemeColors: () => ({}) })); -vi.mock('@/lib/external-auth/use-external-auth-return', () => ({ - useExternalAuthReturn: () => ({ markLaunched: vi.fn(), clearLaunch: vi.fn() }), -})); vi.mock('@/lib/pr-review/connect-gate-platform', () => ({ openAuthorizationAndWaitForReturn: authorization, })); @@ -30,7 +27,7 @@ vi.mock('@/lib/pr-review/connect-gate-platform', () => ({ let mounted: Awaited> | undefined = undefined; beforeEach(() => { vi.stubGlobal('IS_REACT_ACT_ENVIRONMENT', true); - authorization.mockReset().mockResolvedValue('sheet-close'); + authorization.mockReset().mockResolvedValue(undefined); }); afterEach(() => { mounted?.unmount(); @@ -39,7 +36,7 @@ afterEach(() => { }); it('centers setup, disables Connect while authorizing, and refreshes on return', async () => { - const result = Promise.withResolvers<'sheet-close'>(); + const result = Promise.withResolvers(); authorization.mockReturnValueOnce(result.promise); const onConnected = vi.fn().mockResolvedValue(undefined); mounted = await renderWithProviders( @@ -56,10 +53,10 @@ it('centers setup, disables Connect while authorizing, and refreshes on return', expect(button.props.disabled).toBe(false); act(button.props.onPress as () => void); expect(button.props.disabled).toBe(true); - expect(authorization).toHaveBeenCalledWith('ios', 'https://github.com/apps/kilo'); + expect(authorization).toHaveBeenCalledWith('https://github.com/apps/kilo'); expect(onConnected).not.toHaveBeenCalled(); await act(async () => { - result.resolve('sheet-close'); + result.resolve(undefined); await result.promise; }); expect(onConnected).toHaveBeenCalledOnce(); diff --git a/apps/mobile/src/components/security-agent/security-agent-setup.tsx b/apps/mobile/src/components/security-agent/security-agent-setup.tsx index 95befb6fe6..57ac2d55a9 100644 --- a/apps/mobile/src/components/security-agent/security-agent-setup.tsx +++ b/apps/mobile/src/components/security-agent/security-agent-setup.tsx @@ -1,6 +1,6 @@ import { ShieldCheck } from '@/components/ui/icons'; -import { useCallback, useState } from 'react'; -import { Platform, View } from 'react-native'; +import { useState } from 'react'; +import { View } from 'react-native'; import { ActivityIndicator } from '@/components/ui/activity-indicator'; import { useTranslation } from 'react-i18next'; import { toast } from 'sonner-native'; @@ -8,7 +8,6 @@ import { toast } from 'sonner-native'; import { Button } from '@/components/ui/button'; import { Text } from '@/components/ui/text'; import { CenteredState } from '@/components/centered-state'; -import { useExternalAuthReturn } from '@/lib/external-auth/use-external-auth-return'; import { useThemeColors } from '@/lib/hooks/use-theme-colors'; import { openAuthorizationAndWaitForReturn } from '@/lib/pr-review/connect-gate-platform'; @@ -17,7 +16,7 @@ type SecurityAgentSetupProps = { description: string; buttonLabel: string; url: string; - /** Awaited in `finally` so permission/config/repository queries refresh after the browser closes. */ + /** Refreshes permission/config/repository queries after the browser closes. */ onConnected: () => Promise; }; @@ -32,25 +31,12 @@ export function SecurityAgentSetup({ const [connecting, setConnecting] = useState(false); const { t } = useTranslation(); - const handleConnected = useCallback(() => { - void onConnected(); - }, [onConnected]); - const { markLaunched, clearLaunch } = useExternalAuthReturn(handleConnected); - const connect = async () => { setConnecting(true); try { - markLaunched(); - const trigger = await openAuthorizationAndWaitForReturn(Platform.OS, url); - if (trigger === 'sheet-close') { - // iOS: the auth session resolves on sheet close — refresh right here. - clearLaunch(); - await onConnected(); - } - // Android: onConnected runs from the foreground handler once the app - // returns from the plain browser. + await openAuthorizationAndWaitForReturn(url); + await onConnected(); } catch { - clearLaunch(); toast.error(t('securityAgent.setup.couldNotOpenGithub')); } finally { setConnecting(false); diff --git a/apps/mobile/src/lib/auth/passkey-client.test.ts b/apps/mobile/src/lib/auth/passkey-client.test.ts index d606524c7e..1f0b933a60 100644 --- a/apps/mobile/src/lib/auth/passkey-client.test.ts +++ b/apps/mobile/src/lib/auth/passkey-client.test.ts @@ -115,6 +115,17 @@ describe('classifyPasskeyError', () => { expect(classifyPasskeyError({ message: 'nocredentials' })).toBe('no-passkey'); }); + it.each(['name', 'message', 'code'])( + 'classifies ASCII protocol errors in %s independently of the locale', + field => { + expect(classifyPasskeyError({ [field]: 'USERCANCELLED' })).toBe('cancelled'); + expect(classifyPasskeyError({ [field]: 'NOCREDENTIALS' })).toBe('no-passkey'); + expect(classifyPasskeyError({ [field]: 'NOTALLOWEDERROR' })).toBe('no-passkey'); + expect(classifyPasskeyError({ [field]: 'NOTSUPPORTEDEXCEPTION' })).toBe('unsupported'); + expect(classifyPasskeyError({ [field]: 'NOTCONFIGUREDEXCEPTION' })).toBe('unsupported'); + } + ); + it('falls back to the generic failure for anything else', () => { expect(classifyPasskeyError(new Error('socket closed'))).toBe('failed'); expect(classifyPasskeyError(undefined)).toBe('failed'); diff --git a/apps/mobile/src/lib/case-guard.test.ts b/apps/mobile/src/lib/case-guard.test.ts index 5f44d134b1..f65e465c6f 100644 --- a/apps/mobile/src/lib/case-guard.test.ts +++ b/apps/mobile/src/lib/case-guard.test.ts @@ -40,7 +40,7 @@ const ALLOWED_NON_DISPLAY: Readonly> = { 'lib/use-new-session-repos.ts': 'repo key normalization', 'lib/organization-invoice-download.ts': 'filename comparison', 'lib/agent-attachments/validate.ts': 'file-extension normalization', - 'lib/auth/passkey-client.ts': 'credential-error classification key', + 'lib/auth/passkey-client.ts': 'native credential-API error classification, not display text', 'lib/auth/use-native-auth.ts': 'email normalization', 'lib/telemetry/install-error-reporting.ts': 'hostname comparison', 'lib/pr-review/diff/highlight.ts': 'file-extension normalization', diff --git a/apps/mobile/src/lib/external-auth/use-external-auth-return.ts b/apps/mobile/src/lib/external-auth/use-external-auth-return.ts deleted file mode 100644 index 66e2ed3581..0000000000 --- a/apps/mobile/src/lib/external-auth/use-external-auth-return.ts +++ /dev/null @@ -1,51 +0,0 @@ -import { useCallback, useEffect, useRef } from 'react'; -import { AppState, type AppStateStatus, Platform } from 'react-native'; - -/** - * Tracks an in-flight external-auth launch and invokes `onReturn` once the - * app returns to the foreground on Android. iOS uses `openAuthSessionAsync`, - * which resolves on sheet close, so no foreground listener is needed there — - * callers refetch directly on `'sheet-close'` instead. - * - * Extracted from `pr-review-connect-gate.tsx` so every external-auth flow - * (PR review, security-agent setup, provider connect) shares one - * implementation of the launch sentinel + AppState listener. - */ -export type UseExternalAuthReturn = { - markLaunched: () => void; - clearLaunch: () => void; -}; - -export function useExternalAuthReturn(onReturn: () => void): UseExternalAuthReturn { - const launchedAt = useRef(null); - - useEffect(() => { - if (Platform.OS !== 'android') { - return undefined; - } - const handleChange = (nextState: AppStateStatus) => { - if (nextState !== 'active') { - return; - } - if (launchedAt.current === null) { - return; - } - launchedAt.current = null; - onReturn(); - }; - const subscription = AppState.addEventListener('change', handleChange); - return () => { - subscription.remove(); - }; - }, [onReturn]); - - const markLaunched = useCallback(() => { - launchedAt.current = Date.now(); - }, []); - - const clearLaunch = useCallback(() => { - launchedAt.current = null; - }, []); - - return { markLaunched, clearLaunch }; -} diff --git a/apps/mobile/src/lib/pr-review/connect-gate-platform.test.ts b/apps/mobile/src/lib/pr-review/connect-gate-platform.test.ts index 0a007aae0a..0f3d746a03 100644 --- a/apps/mobile/src/lib/pr-review/connect-gate-platform.test.ts +++ b/apps/mobile/src/lib/pr-review/connect-gate-platform.test.ts @@ -1,71 +1,289 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { - getConnectGatePlatformPlan, + launchConnectGateBrowser, openAuthorizationAndWaitForReturn, } from './connect-gate-platform'; -const webBrowserMocks = vi.hoisted(() => ({ - openAuthSessionAsync: vi.fn<(url: string) => Promise>(), +const mocks = vi.hoisted(() => ({ + platform: { OS: 'ios' }, + openAuthSessionAsync: vi.fn<(url: string) => Promise<{ type: string }>>(), openBrowserAsync: vi.fn<(url: string) => Promise>(), + addAppStateListener: vi.fn(), + subscriptions: [] as { + remove: ReturnType; + listener: (state: string) => void; + }[], })); +vi.mock('react-native', () => ({ + Platform: mocks.platform, + AppState: { addEventListener: mocks.addAppStateListener }, +})); vi.mock('expo-web-browser', () => ({ - openAuthSessionAsync: webBrowserMocks.openAuthSessionAsync, - openBrowserAsync: webBrowserMocks.openBrowserAsync, + openAuthSessionAsync: mocks.openAuthSessionAsync, + openBrowserAsync: mocks.openBrowserAsync, + WebBrowserResultType: { OPENED: 'opened', CANCEL: 'cancel', DISMISS: 'dismiss' }, })); +function emitAppState(state: string) { + for (const subscription of mocks.subscriptions) { + subscription.listener(state); + } +} + beforeEach(() => { - webBrowserMocks.openAuthSessionAsync.mockReset(); - webBrowserMocks.openBrowserAsync.mockReset(); + vi.resetAllMocks(); + mocks.subscriptions.length = 0; + mocks.addAppStateListener.mockImplementation( + (_event: string, listener: (state: string) => void) => { + const subscription = { remove: vi.fn(), listener }; + mocks.subscriptions.push(subscription); + return { remove: subscription.remove }; + } + ); }); -describe('getConnectGatePlatformPlan', () => { - it('uses the native auth session and refetches on sheet close for iOS', () => { - expect(getConnectGatePlatformPlan('ios')).toEqual({ - launcher: 'openAuthSession', - refetchTrigger: 'sheet-close', - }); +describe('iOS launch', () => { + beforeEach(() => { + mocks.platform.OS = 'ios'; }); - it('uses a plain custom-tab browser and refetches on app-foreground for Android', () => { - expect(getConnectGatePlatformPlan('android')).toEqual({ - launcher: 'openBrowser', - refetchTrigger: 'app-foreground', - }); + it('opens the native auth session and refetches when it resolves', async () => { + mocks.openAuthSessionAsync.mockResolvedValue({ type: 'cancel' }); + const handlers = { onReturn: vi.fn().mockResolvedValue(undefined), onOpenFailure: vi.fn() }; + + await launchConnectGateBrowser('https://example.com/connect', handlers); + + expect(mocks.openAuthSessionAsync).toHaveBeenCalledExactlyOnceWith( + 'https://example.com/connect' + ); + expect(mocks.openBrowserAsync).not.toHaveBeenCalled(); + // The foreground subscription is cross-platform: iOS registers it too, and + // the native auth session's own resolution means it is only awaited when + // the browser cannot report its dismissal. + expect(mocks.addAppStateListener).toHaveBeenCalledOnce(); + expect(mocks.subscriptions[0]?.remove).toHaveBeenCalledOnce(); + expect(handlers.onReturn).toHaveBeenCalledOnce(); + expect(handlers.onOpenFailure).not.toHaveBeenCalled(); }); +}); + +describe('Android launch', () => { + beforeEach(() => { + mocks.platform.OS = 'android'; + }); + + it('opens a plain browser and refetches when the app returns to the foreground', async () => { + mocks.openBrowserAsync.mockResolvedValue({ type: 'opened' }); + const handlers = { onReturn: vi.fn().mockResolvedValue(undefined), onOpenFailure: vi.fn() }; + + const launch = launchConnectGateBrowser('https://example.com/connect', handlers); + await Promise.resolve(); + + expect(mocks.openBrowserAsync).toHaveBeenCalledExactlyOnceWith('https://example.com/connect'); + expect(mocks.openAuthSessionAsync).not.toHaveBeenCalled(); + expect(mocks.subscriptions).toHaveLength(1); + expect(handlers.onReturn).not.toHaveBeenCalled(); + + emitAppState('background'); + expect(handlers.onReturn).not.toHaveBeenCalled(); + emitAppState('active'); + await launch; - it('falls back to the Android plan for unknown platforms (web, etc.)', () => { - expect(getConnectGatePlatformPlan('web')).toEqual({ - launcher: 'openBrowser', - refetchTrigger: 'app-foreground', - }); - expect(getConnectGatePlatformPlan('')).toEqual({ - launcher: 'openBrowser', - refetchTrigger: 'app-foreground', - }); + expect(handlers.onReturn).toHaveBeenCalledOnce(); + expect(handlers.onOpenFailure).not.toHaveBeenCalled(); + expect(mocks.subscriptions[0]?.remove).toHaveBeenCalledOnce(); + }); + + it('drops the foreground listener when the browser fails to open', async () => { + mocks.openBrowserAsync.mockRejectedValue(new Error('no browser')); + const handlers = { onReturn: vi.fn(), onOpenFailure: vi.fn() }; + + await expect( + launchConnectGateBrowser('https://example.com/connect', handlers) + ).resolves.toBeUndefined(); + + expect(handlers.onOpenFailure).toHaveBeenCalledOnce(); + expect(handlers.onReturn).not.toHaveBeenCalled(); + expect(mocks.subscriptions[0]?.remove).toHaveBeenCalledOnce(); + + // A later foreground cannot stray-refetch: the listener is gone and the + // launch has already settled. + emitAppState('active'); + expect(handlers.onReturn).not.toHaveBeenCalled(); + }); + + it('recovers a later launch after one fails to open', async () => { + mocks.openBrowserAsync + .mockRejectedValueOnce(new Error('no browser')) + .mockResolvedValueOnce({ type: 'opened' }); + const handlers = { onReturn: vi.fn().mockResolvedValue(undefined), onOpenFailure: vi.fn() }; + + await launchConnectGateBrowser('https://example.com/connect', handlers); + expect(handlers.onOpenFailure).toHaveBeenCalledOnce(); + expect(handlers.onReturn).not.toHaveBeenCalled(); + + // The retry must not inherit the failed launch's stuck state (KILO-APP-22). + const retry = launchConnectGateBrowser('https://example.com/connect', handlers); + await Promise.resolve(); + emitAppState('active'); + await retry; + + expect(handlers.onReturn).toHaveBeenCalledOnce(); + expect(handlers.onOpenFailure).toHaveBeenCalledOnce(); + }); + + it('aborts a foreground wait without refetching and allows a fresh launch', async () => { + mocks.openBrowserAsync.mockResolvedValue({ type: 'opened' }); + const controller = new AbortController(); + const handlers = { + signal: controller.signal, + onReturn: vi.fn(), + onOpenFailure: vi.fn(), + }; + const launch = launchConnectGateBrowser('https://example.com/connect', handlers); + await Promise.resolve(); + controller.abort(); + await launch; + + expect(mocks.subscriptions[0]?.remove).toHaveBeenCalledOnce(); + const retryHandlers = { onReturn: vi.fn(), onOpenFailure: vi.fn() }; + const retry = launchConnectGateBrowser('https://example.com/connect', retryHandlers); + emitAppState('active'); + await retry; + expect(handlers.onReturn).not.toHaveBeenCalled(); + expect(handlers.onOpenFailure).not.toHaveBeenCalled(); + expect(retryHandlers.onReturn).toHaveBeenCalledOnce(); + expect(mocks.subscriptions[1]?.remove).toHaveBeenCalledOnce(); }); }); -describe('openAuthorizationAndWaitForReturn', () => { - it('uses openAuthSessionAsync on iOS and reports sheet-close', async () => { - webBrowserMocks.openAuthSessionAsync.mockResolvedValue('done'); - const trigger = await openAuthorizationAndWaitForReturn('ios', 'https://example.com/connect'); - expect(webBrowserMocks.openAuthSessionAsync).toHaveBeenCalledWith( - 'https://example.com/connect' +describe.each(['ios', 'android'])('shared launch contract on %s', os => { + beforeEach(() => { + mocks.platform.OS = os; + }); + + it('reports a failed launch through onOpenFailure and never onReturn', async () => { + if (os === 'android') { + mocks.openBrowserAsync.mockRejectedValue(new Error('no browser')); + } else { + mocks.openAuthSessionAsync.mockRejectedValue(new Error('no browser')); + } + const handlers = { onReturn: vi.fn(), onOpenFailure: vi.fn() }; + + await expect( + launchConnectGateBrowser('https://example.com/connect', handlers) + ).resolves.toBeUndefined(); + expect(handlers.onOpenFailure).toHaveBeenCalledOnce(); + expect(handlers.onReturn).not.toHaveBeenCalled(); + }); + + it('does not launch or notify a caller whose signal is already aborted', async () => { + const controller = new AbortController(); + controller.abort(); + const handlers = { + signal: controller.signal, + onReturn: vi.fn(), + onOpenFailure: vi.fn(), + }; + + await launchConnectGateBrowser('https://example.com/connect', handlers); + expect(mocks.openBrowserAsync).not.toHaveBeenCalled(); + expect(mocks.openAuthSessionAsync).not.toHaveBeenCalled(); + expect(mocks.addAppStateListener).not.toHaveBeenCalled(); + expect(handlers.onReturn).not.toHaveBeenCalled(); + expect(handlers.onOpenFailure).not.toHaveBeenCalled(); + }); + + it.each(['resolve', 'reject'])( + 'ends a pending launch on abort before a late %s', + async outcome => { + const pending = Promise.withResolvers<{ type: string }>(); + mocks.openBrowserAsync.mockReturnValue(pending.promise); + mocks.openAuthSessionAsync.mockReturnValue(pending.promise); + const controller = new AbortController(); + const removeAbortListener = vi.spyOn(controller.signal, 'removeEventListener'); + const handlers = { + signal: controller.signal, + onReturn: vi.fn(), + onOpenFailure: vi.fn(), + }; + + const launch = launchConnectGateBrowser('https://example.com/connect', handlers); + controller.abort(); + await launch; + // The foreground subscription is registered on both platforms, and abort + // drops it on both. + expect(mocks.subscriptions[0]?.remove).toHaveBeenCalledOnce(); + expect(removeAbortListener).toHaveBeenCalledExactlyOnceWith('abort', expect.any(Function)); + + if (outcome === 'resolve') { + pending.resolve({ type: 'cancel' }); + } else { + pending.reject(new Error('late launch failure')); + } + emitAppState('active'); + await Promise.resolve(); + expect(handlers.onReturn).not.toHaveBeenCalled(); + expect(handlers.onOpenFailure).not.toHaveBeenCalled(); + } + ); + + it.each(['return', 'failure'])('removes the abort listener after normal %s', async outcome => { + const controller = new AbortController(); + const addAbortListener = vi.spyOn(controller.signal, 'addEventListener'); + const removeAbortListener = vi.spyOn(controller.signal, 'removeEventListener'); + const handlers = { + signal: controller.signal, + onReturn: vi.fn(), + onOpenFailure: vi.fn(), + }; + if (outcome === 'failure') { + const open = os === 'android' ? mocks.openBrowserAsync : mocks.openAuthSessionAsync; + open.mockRejectedValue(new Error('no browser')); + } else { + mocks.openBrowserAsync.mockResolvedValue({ type: 'opened' }); + mocks.openAuthSessionAsync.mockResolvedValue({ type: 'cancel' }); + } + + const launch = launchConnectGateBrowser('https://example.com/connect', handlers); + emitAppState('active'); + await launch; + expect(removeAbortListener).toHaveBeenCalledExactlyOnceWith( + 'abort', + addAbortListener.mock.calls[0]?.[1] ); - expect(webBrowserMocks.openBrowserAsync).not.toHaveBeenCalled(); - expect(trigger).toBe('sheet-close'); + expect(handlers.onReturn).toHaveBeenCalledTimes(outcome === 'return' ? 1 : 0); + expect(handlers.onOpenFailure).toHaveBeenCalledTimes(outcome === 'failure' ? 1 : 0); + controller.abort(); + // Registered on both platforms, removed on both. + expect(mocks.subscriptions[0]?.remove).toHaveBeenCalledOnce(); }); +}); - it('uses openBrowserAsync on Android and reports app-foreground', async () => { - webBrowserMocks.openBrowserAsync.mockResolvedValue(undefined); - const trigger = await openAuthorizationAndWaitForReturn( - 'android', - 'https://example.com/connect' +describe('launchConnectGateBrowser', () => { + it('propagates refetch errors without misreporting a browser-launch failure', async () => { + mocks.platform.OS = 'ios'; + mocks.openAuthSessionAsync.mockResolvedValue({ type: 'dismiss' }); + const error = new Error('status refetch failed'); + const handlers = { onReturn: vi.fn().mockRejectedValue(error), onOpenFailure: vi.fn() }; + + await expect(launchConnectGateBrowser('https://example.com/connect', handlers)).rejects.toBe( + error + ); + expect(handlers.onOpenFailure).not.toHaveBeenCalled(); + }); +}); + +describe('openAuthorizationAndWaitForReturn', () => { + it('propagates a launch failure to other authorization callers', async () => { + mocks.platform.OS = 'ios'; + const error = new Error('no browser'); + mocks.openAuthSessionAsync.mockRejectedValue(error); + + await expect(openAuthorizationAndWaitForReturn('https://example.com/connect')).rejects.toBe( + error ); - expect(webBrowserMocks.openBrowserAsync).toHaveBeenCalledWith('https://example.com/connect'); - expect(webBrowserMocks.openAuthSessionAsync).not.toHaveBeenCalled(); - expect(trigger).toBe('app-foreground'); }); }); diff --git a/apps/mobile/src/lib/pr-review/connect-gate-platform.ts b/apps/mobile/src/lib/pr-review/connect-gate-platform.ts index 725fa4c31b..b749952481 100644 --- a/apps/mobile/src/lib/pr-review/connect-gate-platform.ts +++ b/apps/mobile/src/lib/pr-review/connect-gate-platform.ts @@ -1,49 +1,97 @@ -// Pure/hook selection for every external-auth flow's platform branch (PR -// review connect gate, security-agent setup, provider connect card). Extracted -// so the platform choice (which browser launcher + which refetch trigger) can -// be unit-tested without pulling in the full React component tree. - import * as WebBrowser from 'expo-web-browser'; +import { AppState, type AppStateStatus, Platform } from 'react-native'; -type AuthLauncher = 'openAuthSession' | 'openBrowser'; - -type GateRefetchTrigger = 'sheet-close' | 'app-foreground'; - -type ConnectGatePlatformPlan = { - launcher: AuthLauncher; - refetchTrigger: GateRefetchTrigger; -}; +/** + * Subscribes to the next foreground return. The caller owns cleanup, including + * when the launch fails or its wait is cancelled before the app returns. + */ +function waitForForeground() { + let resolveReturn: (() => void) | undefined = undefined; + const returned = new Promise(resolve => { + resolveReturn = resolve; + }); + const subscription = AppState.addEventListener('change', (state: AppStateStatus) => { + if (state !== 'active') { + return; + } + resolveReturn?.(); + }); + return { returned, subscription }; +} /** - * Maps a React Native platform to the browser launcher and refetch trigger - * the connect gate should use after the auth session ends. + * Opens the authorization URL and resolves once the user is back in the app. + * + * The foreground subscription is registered on both platforms; it completes the + * wait wherever the browser cannot report its own dismissal. A plain browser + * resolves `opened` as soon as it launches, while the native iOS auth session + * only resolves when the sheet closes. * - * - iOS: `openAuthSessionAsync` returns when the sheet closes, so we - * refetch on `sheet-close`. No foreground listener needed. - * - Android: `openBrowserAsync` is fire-and-forget (no callback when the - * user finishes), so we wait for the app to return to foreground and - * refetch then. Same pattern as `use-device-auth.ts` ~:34-42. + * Android is the one platform branch left, and it exists because the platform + * is missing a capability: it has no native auth-session completion callback. + * expo-web-browser's Android `openAuthSessionAsync` fallback is a polyfill that + * keeps module-level state which can get stuck and reject every later call + * (KILO-APP-22), so Android opens a plain browser instead. Callers await the + * same promise on both platforms, and aborting ends the wait without closing + * the browser or auth session. */ -export function getConnectGatePlatformPlan(platform: string): ConnectGatePlatformPlan { - if (platform === 'ios') { - return { launcher: 'openAuthSession', refetchTrigger: 'sheet-close' }; +export async function openAuthorizationAndWaitForReturn( + authorizationUrl: string, + signal?: AbortSignal +): Promise { + if (signal?.aborted) { + return; + } + let resolveCancellation: (() => void) | undefined = undefined; + const cancelled = new Promise(resolve => { + resolveCancellation = resolve; + }); + const onAbort = () => resolveCancellation?.(); + signal?.addEventListener('abort', onAbort); + const foreground = waitForForeground(); + try { + const opened = + Platform.OS === 'android' + ? WebBrowser.openBrowserAsync(authorizationUrl) + : WebBrowser.openAuthSessionAsync(authorizationUrl); + await Promise.race([opened, cancelled]); + if (!signal?.aborted) { + const result = await opened; + if (result.type === WebBrowser.WebBrowserResultType.OPENED) { + await Promise.race([foreground.returned, cancelled]); + } + } + } finally { + foreground.subscription.remove(); + signal?.removeEventListener('abort', onAbort); } - return { launcher: 'openBrowser', refetchTrigger: 'app-foreground' }; } +type ConnectGateLaunchHandlers = { + signal?: AbortSignal; + onReturn: () => Promise; + /** The browser failed to open: tell the user instead of leaving the CTA inert. */ + onOpenFailure: () => void; +}; + /** - * Opens the authorization URL with the platform-appropriate launcher and - * resolves with the trigger the caller should use to refetch the - * authorization query. Kept as a single helper so the gate component - * doesn't have to know which platform maps to which API. + * Launch failures are reported separately from refetch failures. Closing the + * browser still refetches: the server-driven connection may have completed. + * Aborting on unmount instead skips callbacks belonging to the stale caller. */ -export async function openAuthorizationAndWaitForReturn( - platform: string, - authorizationUrl: string -): Promise { - const plan = getConnectGatePlatformPlan(platform); - await (plan.launcher === 'openAuthSession' - ? WebBrowser.openAuthSessionAsync(authorizationUrl) - : WebBrowser.openBrowserAsync(authorizationUrl)); - return plan.refetchTrigger; +export async function launchConnectGateBrowser( + authorizationUrl: string, + handlers: ConnectGateLaunchHandlers +): Promise { + try { + await openAuthorizationAndWaitForReturn(authorizationUrl, handlers.signal); + } catch { + if (!handlers.signal?.aborted) { + handlers.onOpenFailure(); + } + return; + } + if (!handlers.signal?.aborted) { + await handlers.onReturn(); + } } diff --git a/apps/mobile/src/lib/use-github-repos-refresh-helpers.ts b/apps/mobile/src/lib/use-github-repos-refresh-helpers.ts index 81299bd40d..9cf77d0cf2 100644 --- a/apps/mobile/src/lib/use-github-repos-refresh-helpers.ts +++ b/apps/mobile/src/lib/use-github-repos-refresh-helpers.ts @@ -1,19 +1,6 @@ // Pure decision helpers for useGitHubReposRefresh, extracted so the test // doesn't pull in react-native (Flow-syntax) via the hook module. -export type RefreshTrigger = 'sheet-close' | 'app-foreground'; - -/** - * Maps a platform to the expected refetch trigger after the auth session - * ends. Mirrors the connect-gate pattern. - */ -export function resolveRefreshTrigger(platform: string): RefreshTrigger { - if (platform === 'ios') { - return 'sheet-close'; - } - return 'app-foreground'; -} - /** * Whether `connectCheckFailed` should be set after a return-triggered * force-fresh. Only set when the browser-return payload says diff --git a/apps/mobile/src/lib/use-github-repos-refresh.test.ts b/apps/mobile/src/lib/use-github-repos-refresh.test.ts index 64e0545b24..b46821bc5f 100644 --- a/apps/mobile/src/lib/use-github-repos-refresh.test.ts +++ b/apps/mobile/src/lib/use-github-repos-refresh.test.ts @@ -1,26 +1,10 @@ import { describe, expect, it } from 'vitest'; import { - resolveRefreshTrigger, shouldClearConnectCheckFailed, shouldSetConnectCheckFailed, } from './use-github-repos-refresh-helpers'; -describe('resolveRefreshTrigger', () => { - it('returns sheet-close for iOS', () => { - expect(resolveRefreshTrigger('ios')).toBe('sheet-close'); - }); - - it('returns app-foreground for Android', () => { - expect(resolveRefreshTrigger('android')).toBe('app-foreground'); - }); - - it('falls back to app-foreground for unknown platforms', () => { - expect(resolveRefreshTrigger('web')).toBe('app-foreground'); - expect(resolveRefreshTrigger('')).toBe('app-foreground'); - }); -}); - describe('shouldSetConnectCheckFailed', () => { it('sets when return-triggered AND integration not installed', () => { expect( diff --git a/apps/mobile/src/lib/use-github-repos-refresh.ts b/apps/mobile/src/lib/use-github-repos-refresh.ts index fdf89408ca..7e757c6f89 100644 --- a/apps/mobile/src/lib/use-github-repos-refresh.ts +++ b/apps/mobile/src/lib/use-github-repos-refresh.ts @@ -1,5 +1,4 @@ -import { useCallback, useEffect, useRef, useState } from 'react'; -import { AppState, type AppStateStatus, Platform } from 'react-native'; +import { useCallback, useEffect, useState } from 'react'; import { useQueryClient } from '@tanstack/react-query'; import { toast } from 'sonner-native'; @@ -36,10 +35,6 @@ export function useGitHubReposRefresh({ const [isRefreshingRepos, setIsRefreshingRepos] = useState(false); const [connectCheckFailed, setConnectCheckFailed] = useState(false); - // Sentinel: set before browser launch on Android, cleared on - // consume or error. Prevents stale AppState from triggering refetch. - const launchedAt = useRef(null); - // ── connectCheckFailed clear-on-input effect ────────────────────── useEffect(() => { if (integrationInstalled === true && connectCheckFailed) { @@ -96,58 +91,19 @@ export function useGitHubReposRefresh({ [organizationId, trpc, queryClient] ); - // Always-correct ref so Android AppState listener calls the latest - // performForceFresh after organization/context changes. - const performForceFreshRef = useRef(performForceFresh); - performForceFreshRef.current = performForceFresh; - - // ── Android foreground listener ─────────────────────────────────── - useEffect(() => { - if (Platform.OS !== 'android') { - return undefined; - } - const handleChange = (nextState: AppStateStatus) => { - if (nextState !== 'active') { - return; - } - if (launchedAt.current === null) { - return; - } - launchedAt.current = null; - void performForceFreshRef.current(true); - }; - const subscription = AppState.addEventListener('change', handleChange); - return () => { - subscription.remove(); - }; - }, []); - // ── Open GitHub integration ────────────────────────────────────── const openGitHubIntegration = useCallback(() => { void (async () => { try { - launchedAt.current = Date.now(); const { token } = await trpcClient.githubApps.mintInstallState.mutate({ organizationId: organizationId ?? undefined, returnTo: '/cloud/sessions', }); - const trigger = await openAuthorizationAndWaitForReturn( - Platform.OS, + await openAuthorizationAndWaitForReturn( getGitHubIntegrationUrl(WEB_BASE_URL, organizationId, token) ); - if (trigger === 'sheet-close') { - // iOS: refetch immediately. Clear the sentinel so the AppState - // handler (if it ever fires on iOS) doesn't double-refetch. - launchedAt.current = null; - await performForceFresh(true); - } - // Android: refetch is handled by the AppState listener when the - // app returns to foreground. Do NOT clear the sentinel here — the - // foreground handler clears it when consumed. + await performForceFresh(true); } catch { - // Browser failed to open — clear the sentinel so a later - // unrelated foreground doesn't trigger a stray refetch. - launchedAt.current = null; toast.error(i18n.t('codeReviewer.providerConnect.githubError')); } })(); diff --git a/apps/mobile/src/lib/use-new-session-repos.test.ts b/apps/mobile/src/lib/use-new-session-repos.test.ts index 0fb2c32c75..d67808ef53 100644 --- a/apps/mobile/src/lib/use-new-session-repos.test.ts +++ b/apps/mobile/src/lib/use-new-session-repos.test.ts @@ -69,11 +69,7 @@ vi.mock('@/lib/integration-urls', () => ({ })); vi.mock('@/lib/pr-review/connect-gate-platform', () => ({ - openAuthorizationAndWaitForReturn: vi.fn(async () => 'sheet-close'), -})); - -vi.mock('@/lib/external-auth/use-external-auth-return', () => ({ - useExternalAuthReturn: () => ({ markLaunched: vi.fn(), clearLaunch: vi.fn() }), + openAuthorizationAndWaitForReturn: vi.fn(async () => undefined), })); vi.mock('@/lib/use-github-repos-refresh', () => ({ diff --git a/apps/mobile/src/lib/use-new-session-repos.ts b/apps/mobile/src/lib/use-new-session-repos.ts index 78645a562f..9b8598120b 100644 --- a/apps/mobile/src/lib/use-new-session-repos.ts +++ b/apps/mobile/src/lib/use-new-session-repos.ts @@ -1,6 +1,5 @@ /* eslint-disable max-lines -- One hook wires the GitHub, GitLab, and Bitbucket provider queries, recents resolution, and connect/refresh flows end-to-end. */ import { useCallback, useMemo, useState } from 'react'; -import { Platform } from 'react-native'; import { useQuery, useQueryClient } from '@tanstack/react-query'; import { toast } from 'sonner-native'; @@ -22,7 +21,6 @@ import { WEB_BASE_URL } from '@/lib/config'; import { useRecentAgentRepositories } from '@/lib/hooks/use-agent-sessions'; import { getBitbucketIntegrationUrl, getGitLabIntegrationUrl } from '@/lib/integration-urls'; import { openAuthorizationAndWaitForReturn } from '@/lib/pr-review/connect-gate-platform'; -import { useExternalAuthReturn } from '@/lib/external-auth/use-external-auth-return'; import { classifyProviderErrorCode } from '@/lib/code-reviewer-status'; import { useGitHubReposRefresh } from '@/lib/use-github-repos-refresh'; import { trpcClient, useTRPC } from '@/lib/trpc'; @@ -282,36 +280,18 @@ export function useNewSessionRepos({ }, [refreshGitHubForceFresh, forceFreshGitLab, forceFreshBitbucket]); // ── Per-provider connect ────────────────────────────────────────── - // Android: `openAuthorizationAndWaitForReturn` returns `'app-foreground'` - // (the browser launch is fire-and-forget), so each provider's refresh runs - // from a shared foreground listener when the app returns. - const { markLaunched: markGitLabLaunched, clearLaunch: clearGitLabLaunch } = - useExternalAuthReturn(() => { - void forceFreshGitLab(); - }); - const { markLaunched: markBitbucketLaunched, clearLaunch: clearBitbucketLaunch } = - useExternalAuthReturn(() => { - void forceFreshBitbucket(); - }); - const openGitLabIntegration = useCallback(() => { void (async () => { try { - markGitLabLaunched(); - const trigger = await openAuthorizationAndWaitForReturn( - Platform.OS, + await openAuthorizationAndWaitForReturn( getGitLabIntegrationUrl(WEB_BASE_URL, organizationId) ); - if (trigger === 'sheet-close') { - clearGitLabLaunch(); - await forceFreshGitLab(); - } + await forceFreshGitLab(); } catch { - clearGitLabLaunch(); toast.error(i18n.t('codeReviewer.providerConnect.gitlabError')); } })(); - }, [organizationId, forceFreshGitLab, markGitLabLaunched, clearGitLabLaunch]); + }, [organizationId, forceFreshGitLab]); const openBitbucketIntegration = useCallback(() => { if (!organizationId) { @@ -319,21 +299,15 @@ export function useNewSessionRepos({ } void (async () => { try { - markBitbucketLaunched(); - const trigger = await openAuthorizationAndWaitForReturn( - Platform.OS, + await openAuthorizationAndWaitForReturn( getBitbucketIntegrationUrl(WEB_BASE_URL, organizationId) ); - if (trigger === 'sheet-close') { - clearBitbucketLaunch(); - await forceFreshBitbucket(); - } + await forceFreshBitbucket(); } catch { - clearBitbucketLaunch(); toast.error(i18n.t('codeReviewer.providerConnect.bitbucketError')); } })(); - }, [organizationId, forceFreshBitbucket, markBitbucketLaunched, clearBitbucketLaunch]); + }, [organizationId, forceFreshBitbucket]); const openIntegration = useCallback( (platform: RepositoryPlatform) => {