diff --git a/apps/mobile/src/components/agents/new-session-repository-section.test.ts b/apps/mobile/src/components/agents/new-session-repository-section.test.ts index f803cfce94..4006fb6be0 100644 --- a/apps/mobile/src/components/agents/new-session-repository-section.test.ts +++ b/apps/mobile/src/components/agents/new-session-repository-section.test.ts @@ -109,6 +109,33 @@ beforeEach(() => { collapseState.setConnectCtaCollapsed.mockClear(); }); +describe('NewSessionRepositorySection repository picker', () => { + it('renders the picker under the Repository heading when no provider has rows yet', () => { + // Findings 1 and 2: with both providers settled-empty but no rows, the + // picker was gated off (`hasRepos || anyLoading`), leaving only the + // `Repository` heading above the Start session button — a blank void with + // no control under it. The picker trigger reserves its height and already + // covers loading, empty and disabled, so it must always render. + const renderer = mountSection({ + repositories: [], + groups: [group('github', 'repos'), group('gitlab', 'repos')], + }); + + expect(renderer.root.findAllByType('RepoSelector' as never)).toHaveLength(1); + }); + + it('renders the picker in its loading state while every provider is unsettled', () => { + const renderer = mountSection({ + repositories: [], + groups: [group('github', 'loading'), group('gitlab', 'loading')], + }); + + const picker = renderer.root.findAllByType('RepoSelector' as never)[0]; + expect(picker).toBeDefined(); + expect(picker?.props.isLoading).toBe(true); + }); +}); + describe('NewSessionRepositorySection branch row', () => { it.each([ ['github:owner/repo', githubRow], diff --git a/apps/mobile/src/components/agents/new-session-repository-section.tsx b/apps/mobile/src/components/agents/new-session-repository-section.tsx index 532653abf0..7c1c41072a 100644 --- a/apps/mobile/src/components/agents/new-session-repository-section.tsx +++ b/apps/mobile/src/components/agents/new-session-repository-section.tsx @@ -95,11 +95,11 @@ function connectNoteKey(platform: RepositoryPlatform): string | undefined { /** * Provider-aware repository section. One group per provider renders its own - * empty/error state independently, and the picker trigger lists every - * repository plus the Recently used rows when any provider has rows. A - * provider's expanded connect prompt renders only before a repository is - * selected. Afterwards, compact actions keep other providers reachable without - * contradicting the completed selection or requiring it to be cleared. + * empty/error state independently, and the picker trigger always renders, + * listing every repository plus the Recently used rows. A provider's expanded + * connect prompt renders only before a repository is selected. Afterwards, + * compact actions keep other providers reachable without contradicting the + * completed selection or requiring it to be cleared. */ export function NewSessionRepositorySection({ disabled, @@ -135,17 +135,25 @@ export function NewSessionRepositorySection({ {t('common.repository')} - {(hasRepos || anyLoading) && ( - - )} + {/* + The picker renders unconditionally. Its trigger always reserves its + height and already covers loading ("Loading..."), empty ("Select + repository") and disabled states, so gating it off (the old + `hasRepos || anyLoading`) could leave the `Repository` heading as the + section's only child — a blank void above Start session — while a + provider query was paused (`isLoading === false`, no data) or a + connect card was waiting on the persisted-collapse read. The web panel + renders its repository trigger unconditionally too. + */} + {isCloneEntry ? null : ( { ).toBe('loading'); }); + it('returns loading while the provider query has produced no data yet', () => { + // A paused query (iOS boot / NetInfo probe not settled) reports + // `isLoading === false` and leaves `data` undefined, so + // `integrationInstalled` is undefined. That provider's list is not known + // yet and must never read as settled (`'repos'`), or the section renders a + // heading with no control under it. + expect( + resolveProviderStatus({ + isLoading: false, + isError: false, + integrationInstalled: undefined, + repositoryCount: 0, + }) + ).toBe('loading'); + }); + it('returns error when the query failed with no cached repos', () => { expect( resolveProviderStatus({ @@ -39,11 +55,14 @@ describe('resolveProviderStatus', () => { }); it('keeps cached repos visible after a background refetch error', () => { + // Cached rows and the integration flag come from the same query data, so a + // background refetch error that keeps its rows keeps a defined + // `integrationInstalled` too; `undefined` is reserved for "no data yet". expect( resolveProviderStatus({ isLoading: false, isError: true, - integrationInstalled: undefined, + integrationInstalled: true, repositoryCount: 3, }) ).toBe('repos'); diff --git a/apps/mobile/src/components/agents/new-session-repository-state.ts b/apps/mobile/src/components/agents/new-session-repository-state.ts index 91bd1429d7..5a4476d8f4 100644 --- a/apps/mobile/src/components/agents/new-session-repository-state.ts +++ b/apps/mobile/src/components/agents/new-session-repository-state.ts @@ -60,10 +60,18 @@ export function resolveProviderStatus({ if (isError && repositoryCount === 0) { return 'error'; } - if (integrationInstalled === false) { + // `undefined` means the query has produced no data at all, not "not + // installed": a paused query (iOS boot / the NetInfo probe not settled yet) + // reports `isLoading === false` and leaves `data` undefined, so the flag is + // undefined too. A provider whose list is not known yet must never read as + // settled, or the section renders its heading with no control under it. + if (integrationInstalled === undefined) { + return 'loading'; + } + if (!integrationInstalled) { return 'connect'; } - if (integrationInstalled === true && repositoryCount === 0) { + if (repositoryCount === 0) { return 'connected-empty'; } return 'repos'; diff --git a/apps/mobile/src/lib/auth/auth-browser.ts b/apps/mobile/src/lib/auth/auth-browser.ts new file mode 100644 index 0000000000..cd051ba54e --- /dev/null +++ b/apps/mobile/src/lib/auth/auth-browser.ts @@ -0,0 +1,57 @@ +import * as WebBrowser from 'expo-web-browser'; +import { Platform } from 'react-native'; + +import { PRODUCTION_HOSTS } from '@/lib/url-contract'; + +/** + * Which browser API presents the device-auth page. The flow ends on the poll's + * approval rather than on the page's redirect, so the page must be closed from + * here, and only the API that opened it can close it: `dismissAuthSession` does + * nothing to a plain browser. + */ +export type AuthBrowserKind = 'auth-session' | 'plain-browser'; + +// iOS's ASWebAuthenticationSession raises a consent alert naming the auth URL's +// host ("" Wants to Use "" to Sign In). On a non-product host — a +// dev or preview stack — that alert shows the user a raw developer address +// instead of a domain they can recognize as Kilo, so only open the native auth +// session when the host is a product host. Otherwise present +// SFSafariViewController via openBrowserAsync: it shares Safari's cookies (an +// existing web session still applies) and raises no consent alert. The flow +// polls the server for approval and never consumes the session redirect, so +// nothing else changes. Android keeps openBrowserAsync because +// expo-web-browser's openAuthSessionAsync polyfill can get stuck (KILO-APP-22). +function isProductAuthHost(url: string): boolean { + try { + return PRODUCTION_HOSTS.includes(new URL(url).hostname); + } catch { + // An unparseable URL cannot be recognized as a product host, so avoid the + // consent alert and fall back to the plain browser. + return false; + } +} + +export function resolveAuthBrowserKind(url: string): AuthBrowserKind { + return Platform.OS === 'android' || !isProductAuthHost(url) ? 'plain-browser' : 'auth-session'; +} + +export async function openAuthBrowser(url: string): Promise { + await (resolveAuthBrowserKind(url) === 'plain-browser' + ? WebBrowser.openBrowserAsync(url) + : WebBrowser.openAuthSessionAsync(url)); +} + +/** + * Close the page the flow opened, with the API that matches it. Android opened + * a Chrome custom tab, which the user returns from with the back gesture, and + * the flow has never dismissed it from the app. + */ +export function dismissAuthBrowser(kind: AuthBrowserKind): void { + if (kind === 'auth-session') { + WebBrowser.dismissAuthSession(); + return; + } + if (Platform.OS === 'ios') { + void WebBrowser.dismissBrowser(); + } +} diff --git a/apps/mobile/src/lib/auth/device-auth-poll.test.ts b/apps/mobile/src/lib/auth/device-auth-poll.test.ts index 0065675f49..6939f8c3ad 100644 --- a/apps/mobile/src/lib/auth/device-auth-poll.test.ts +++ b/apps/mobile/src/lib/auth/device-auth-poll.test.ts @@ -1,10 +1,17 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { dismissAuthSession, dismissBrowser } from 'expo-web-browser'; import { startDeviceAuthPoll } from '@/lib/auth/device-auth-poll'; import { type DeviceAuthState } from '@/lib/auth/device-auth-state'; +const platformMock = vi.hoisted(() => ({ os: 'ios' as 'ios' | 'android' })); + vi.mock('react-native', () => ({ - Platform: { OS: 'android' }, + Platform: { + get OS() { + return platformMock.os; + }, + }, })); vi.mock('expo-application', () => ({ @@ -13,6 +20,7 @@ vi.mock('expo-application', () => ({ vi.mock('expo-web-browser', () => ({ dismissAuthSession: vi.fn(), + dismissBrowser: vi.fn(), })); vi.mock('@/lib/config', () => ({ @@ -21,11 +29,17 @@ vi.mock('@/lib/config', () => ({ const fetchMock = vi.fn(); +function approvedResponse() { + return Response.json({ status: 'approved', token: 'access', expiresIn: 3600 }); +} + function pendingResponse() { return Response.json({ status: 'pending' }, { status: 202 }); } -function makePoll(overrides: { startedAt?: number } = {}) { +function makePoll( + overrides: { startedAt?: number; browserKind?: 'auth-session' | 'plain-browser' } = {} +) { const setState = vi.fn<(updater: (prev: DeviceAuthState) => DeviceAuthState) => void>(); const cleanup = vi.fn<() => void>(); const poll = startDeviceAuthPoll({ @@ -34,6 +48,7 @@ function makePoll(overrides: { startedAt?: number } = {}) { signal: new AbortController().signal, setState, cleanup, + browserKind: 'auth-session', ...overrides, }); return { poll, setState, cleanup }; @@ -43,6 +58,8 @@ beforeEach(() => { vi.useFakeTimers(); vi.setSystemTime(new Date('2026-01-01T00:00:00Z')); fetchMock.mockReset(); + vi.mocked(dismissAuthSession).mockClear(); + vi.mocked(dismissBrowser).mockClear(); globalThis.fetch = fetchMock; }); @@ -150,4 +167,43 @@ describe('startDeviceAuthPoll', () => { expect(cleanup).toHaveBeenCalled(); expect(setState).toHaveBeenCalled(); }); + + it('dismisses the plain browser it opened when approval lands', async () => { + // A non-product auth host opens SFSafariViewController, which + // `dismissAuthSession` cannot close: without the matching dismissal the page + // stays over the approved app. + fetchMock.mockResolvedValue(approvedResponse()); + const { setState } = makePoll({ browserKind: 'plain-browser' }); + + await vi.advanceTimersByTimeAsync(3000); + + expect(vi.mocked(dismissBrowser)).toHaveBeenCalledTimes(1); + expect(vi.mocked(dismissAuthSession)).not.toHaveBeenCalled(); + expect(setState).toHaveBeenCalled(); + }); + + it('dismisses the native auth session it opened when approval lands', async () => { + fetchMock.mockResolvedValue(approvedResponse()); + makePoll({ browserKind: 'auth-session' }); + + await vi.advanceTimersByTimeAsync(3000); + + expect(vi.mocked(dismissAuthSession)).toHaveBeenCalledTimes(1); + expect(vi.mocked(dismissBrowser)).not.toHaveBeenCalled(); + }); + + it('leaves the Android custom tab to the user when approval lands', async () => { + platformMock.os = 'android'; + try { + fetchMock.mockResolvedValue(approvedResponse()); + makePoll({ browserKind: 'plain-browser' }); + + await vi.advanceTimersByTimeAsync(3000); + + expect(vi.mocked(dismissBrowser)).not.toHaveBeenCalled(); + expect(vi.mocked(dismissAuthSession)).not.toHaveBeenCalled(); + } finally { + platformMock.os = 'ios'; + } + }); }); diff --git a/apps/mobile/src/lib/auth/device-auth-poll.ts b/apps/mobile/src/lib/auth/device-auth-poll.ts index 81d83f7da7..dcfc045c05 100644 --- a/apps/mobile/src/lib/auth/device-auth-poll.ts +++ b/apps/mobile/src/lib/auth/device-auth-poll.ts @@ -1,8 +1,6 @@ -import { Platform } from 'react-native'; -import * as WebBrowser from 'expo-web-browser'; - import { i18n } from '@/i18n'; import { API_BASE_URL } from '@/lib/config'; +import { type AuthBrowserKind, dismissAuthBrowser } from '@/lib/auth/auth-browser'; import { classifyPollResponse } from '@/lib/auth/poll-response'; import { buildClientMetadataHeaders } from '@/lib/client-metadata'; import { @@ -31,9 +29,14 @@ export function startDeviceAuthPoll(params: { signal: AbortSignal; setState: (updater: (prev: DeviceAuthState) => DeviceAuthState) => void; cleanup: () => void; + /** + * The API that opened the verification page. Approval ends the flow without + * waiting for the page, so the poll closes it with the matching dismissal. + */ + browserKind: AuthBrowserKind; startedAt?: number; }): DeviceAuthPollHandle { - const { code, deviceCode, signal, setState, cleanup } = params; + const { code, deviceCode, signal, setState, cleanup, browserKind } = params; // A resumed transaction reuses the original start clock so its overall // budget does not restart from `Date.now()` and outlive the server code. @@ -94,9 +97,7 @@ export function startDeviceAuthPoll(params: { if (parsed?.status === 'approved') { cleanup(); - if (Platform.OS !== 'android') { - WebBrowser.dismissAuthSession(); - } + dismissAuthBrowser(browserKind); setState(previous => approvedDeviceAuthState({ code, diff --git a/apps/mobile/src/lib/auth/use-device-auth.test.ts b/apps/mobile/src/lib/auth/use-device-auth.test.ts index 7c24a1296d..4d0ae49464 100644 --- a/apps/mobile/src/lib/auth/use-device-auth.test.ts +++ b/apps/mobile/src/lib/auth/use-device-auth.test.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines -- one device-auth suite: the poll-request and token-parse contract cases plus the hook mount/state cases share one module-mock scaffold */ /* eslint-disable import/first -- vi.mock must precede the hook import so the native modules are stubbed */ import * as React from 'react'; import { act, TestRenderer } from '@/test/renderer'; @@ -25,9 +26,10 @@ vi.mock('expo-web-browser', () => ({ openAuthSessionAsync: vi.fn(), })); +// A product WEB_BASE_URL keeps the SSO case on the native auth session path. vi.mock('@/lib/config', () => ({ API_BASE_URL: 'http://localhost:3000', - WEB_BASE_URL: 'http://localhost:3001', + WEB_BASE_URL: 'https://app.kilo.ai', })); vi.mock('@/lib/auth/device-auth-poll', () => ({ @@ -43,7 +45,7 @@ vi.mock('@/lib/auth/pending-external-auth', () => ({ import { useDeviceAuth } from '@/lib/auth/use-device-auth'; import { startDeviceAuthPoll } from '@/lib/auth/device-auth-poll'; import { pendingDeviceAuthState } from '@/lib/auth/device-auth-state'; -import { openAuthSessionAsync } from 'expo-web-browser'; +import { openAuthSessionAsync, openBrowserAsync } from 'expo-web-browser'; import { clearPendingExternalAuth, readPendingExternalAuth, @@ -252,6 +254,19 @@ async function mountSettled(): Promise<{ current: DeviceAuthResult | null }> { return resultRef; } +/** Runs a fresh sign-in whose server response carries `verificationUrl`, so the + * test observes which browser API the auth host selects. */ +async function startSignin(verificationUrl: string): Promise { + vi.mocked(openAuthSessionAsync).mockClear(); + vi.mocked(openBrowserAsync).mockClear(); + vi.mocked(readPendingExternalAuth).mockResolvedValue({ kind: 'none' }); + fetchMock.mockResolvedValue(Response.json({ code: 'UC', verificationUrl })); + const result = requireResult(await mountSettled()); + await act(async () => { + await result.start('signin'); + }); +} + describe('useDeviceAuth hook', () => { beforeEach(() => { vi.mocked(readPendingExternalAuth).mockReset(); @@ -285,6 +300,8 @@ describe('useDeviceAuth hook', () => { expect(resultRef.current?.code).toBe('UC-1234'); expect(resultRef.current?.resumed).toBe(true); expect(startDeviceAuthPoll).toHaveBeenCalledTimes(1); + // The resumed poll closes whatever the flow opened for this URL. + expect(vi.mocked(startDeviceAuthPoll).mock.calls[0]?.[0].browserKind).toBe('plain-browser'); }); it('lets a start() that runs during the restore read win without a second poll', async () => { @@ -362,4 +379,22 @@ describe('useDeviceAuth hook', () => { expect(url.searchParams.get('callbackPath')).toBe('/device-auth?code=USER-123&app=1'); expect(url.searchParams.has('ssoOrganizationId')).toBe(false); }); + + it('opens a plain browser for a non-product auth host (no consent alert)', async () => { + const url = 'http://127.0.0.1:3000/device-auth?code=UC-1'; + await startSignin(url); + expect(vi.mocked(openBrowserAsync)).toHaveBeenCalledExactlyOnceWith(url); + expect(vi.mocked(openAuthSessionAsync)).not.toHaveBeenCalled(); + // Approval ends the flow from the poll, so it must know which page to close: + // `dismissAuthSession` does nothing to this one. + expect(vi.mocked(startDeviceAuthPoll).mock.calls[0]?.[0].browserKind).toBe('plain-browser'); + }); + + it('keeps the native auth session for a product auth host', async () => { + const url = 'https://app.kilo.ai/device-auth?code=UC-2'; + await startSignin(url); + expect(vi.mocked(openAuthSessionAsync)).toHaveBeenCalledExactlyOnceWith(url); + expect(vi.mocked(openBrowserAsync)).not.toHaveBeenCalled(); + expect(vi.mocked(startDeviceAuthPoll).mock.calls[0]?.[0].browserKind).toBe('auth-session'); + }); }); diff --git a/apps/mobile/src/lib/auth/use-device-auth.ts b/apps/mobile/src/lib/auth/use-device-auth.ts index a4ebc89355..8e22244107 100644 --- a/apps/mobile/src/lib/auth/use-device-auth.ts +++ b/apps/mobile/src/lib/auth/use-device-auth.ts @@ -1,9 +1,9 @@ -import * as WebBrowser from 'expo-web-browser'; import { useCallback, useEffect, useRef, useState } from 'react'; -import { AppState, type AppStateStatus, Platform } from 'react-native'; +import { AppState, type AppStateStatus } from 'react-native'; import { i18n } from '@/i18n'; import { API_BASE_URL, WEB_BASE_URL } from '@/lib/config'; +import { openAuthBrowser, resolveAuthBrowserKind } from '@/lib/auth/auth-browser'; import { getDeviceAuth429Message } from '@/lib/auth/poll-response'; import { parseDeviceAuthCodeResponse } from '@/lib/auth/native-auth-contract'; import { buildClientMetadataHeaders } from '@/lib/client-metadata'; @@ -28,16 +28,6 @@ type DeviceAuthResult = DeviceAuthState & { const START_TIMEOUT_MS = 15_000; -// Android has no native auth session; expo-web-browser's polyfill keeps -// module-level state that can get stuck and reject every future call -// (KILO-APP-22). We poll the server for approval instead of relying on a -// redirect, so a plain browser open is all Android needs. -async function openAuthBrowser(url: string) { - await (Platform.OS === 'android' - ? WebBrowser.openBrowserAsync(url) - : WebBrowser.openAuthSessionAsync(url)); -} - export function useDeviceAuth(): DeviceAuthResult { const [state, setState] = useState(idleDeviceAuthState()); @@ -123,6 +113,7 @@ export function useDeviceAuth(): DeviceAuthResult { signal: abort.signal, setState, cleanup, + browserKind: resolveAuthBrowserKind(record.verificationUrl), startedAt: record.startedAt, }); } @@ -230,6 +221,10 @@ export function useDeviceAuth(): DeviceAuthResult { signal: abort.signal, setState, cleanup, + // Approval dismisses the page the flow opened, so the poll has to know + // which API opened it: the plain browser on a non-product host (or on + // Android) needs its own dismissal. + browserKind: resolveAuthBrowserKind(browserUrl), }); await openAuthBrowser(browserUrl);