feat(auth): handle auth flow errors gracefully (#1256)

This commit is contained in:
N. Taylor Mullen 2025-06-20 01:30:06 -07:00 committed by GitHub
parent 4d9e258a1e
commit 4e69ba3bbe
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 55 additions and 19 deletions

View File

@ -587,11 +587,6 @@ const App = ({ config, settings, startupWarnings = [] }: AppProps) => {
</Box>
) : isAuthDialogOpen ? (
<Box flexDirection="column">
{authError && (
<Box marginBottom={1}>
<Text color={Colors.AccentRed}>{authError}</Text>
</Box>
)}
<AuthDialog
onSelect={handleAuthSelect}
onHighlight={handleAuthHighlight}

View File

@ -6,7 +6,12 @@
import { useState, useCallback, useEffect } from 'react';
import { LoadedSettings, SettingScope } from '../../config/settings.js';
import { AuthType, Config, clearCachedCredentialFile } from '@gemini-cli/core';
import {
AuthType,
Config,
clearCachedCredentialFile,
getErrorMessage,
} from '@gemini-cli/core';
async function performAuthFlow(authMethod: AuthType, config: Config) {
await config.refreshAuth(authMethod);
@ -22,16 +27,36 @@ export const useAuthCommand = (
settings.merged.selectedAuthType === undefined,
);
useEffect(() => {
if (!isAuthDialogOpen) {
performAuthFlow(settings.merged.selectedAuthType as AuthType, config);
}
}, [isAuthDialogOpen, settings, config]);
const openAuthDialog = useCallback(() => {
setIsAuthDialogOpen(true);
}, []);
useEffect(() => {
const authFlow = async () => {
if (isAuthDialogOpen || !settings.merged.selectedAuthType) {
return;
}
try {
await performAuthFlow(
settings.merged.selectedAuthType as AuthType,
config,
);
} catch (e) {
const errorMessage =
settings.merged.selectedAuthType ===
AuthType.LOGIN_WITH_GOOGLE_PERSONAL
? `Failed to login. Ensure your Google account is not an enterprise account.
Message: ${getErrorMessage(e)}`
: `Failed to login. Message: ${getErrorMessage(e)}`;
setAuthError(errorMessage);
openAuthDialog();
}
};
void authFlow();
}, [isAuthDialogOpen, settings, config, setAuthError, openAuthDialog]);
const handleAuthSelect = useCallback(
async (authMethod: string | undefined, scope: SettingScope) => {
if (authMethod) {

View File

@ -63,6 +63,20 @@ describe('usePhraseCycler', () => {
});
it('should reset to a witty phrase when isActive becomes true after being false (and not waiting)', () => {
// Ensure there are at least two phrases for this test to be meaningful.
if (WITTY_LOADING_PHRASES.length < 2) {
return;
}
// Mock Math.random to make the test deterministic.
let callCount = 0;
vi.spyOn(Math, 'random').mockImplementation(() => {
// Cycle through 0, 1, 0, 1, ...
const val = callCount % 2;
callCount++;
return val / WITTY_LOADING_PHRASES.length;
});
const { result, rerender } = renderHook(
({ isActive, isWaiting }) => usePhraseCycler(isActive, isWaiting),
{ initialProps: { isActive: false, isWaiting: false } },
@ -72,25 +86,27 @@ describe('usePhraseCycler', () => {
rerender({ isActive: true, isWaiting: false });
const firstActivePhrase = result.current;
expect(WITTY_LOADING_PHRASES).toContain(firstActivePhrase);
// With our mock, this should be the first phrase.
expect(firstActivePhrase).toBe(WITTY_LOADING_PHRASES[0]);
act(() => {
vi.advanceTimersByTime(PHRASE_CHANGE_INTERVAL_MS);
});
// Phrase should change if enough phrases and interval passed
if (WITTY_LOADING_PHRASES.length > 1) {
expect(result.current).not.toBe(firstActivePhrase);
}
expect(WITTY_LOADING_PHRASES).toContain(result.current);
// Phrase should change to the second phrase.
expect(result.current).not.toBe(firstActivePhrase);
expect(result.current).toBe(WITTY_LOADING_PHRASES[1]);
// Set to inactive - should reset to the default initial phrase
rerender({ isActive: false, isWaiting: false });
expect(result.current).toBe(WITTY_LOADING_PHRASES[0]);
// Set back to active - should pick a random witty phrase
// Set back to active - should pick a random witty phrase (which our mock controls)
act(() => {
rerender({ isActive: true, isWaiting: false });
});
expect(WITTY_LOADING_PHRASES).toContain(result.current);
// The random mock will now return 0, so it should be the first phrase again.
expect(result.current).toBe(WITTY_LOADING_PHRASES[0]);
});
it('should clear phrase interval on unmount when active', () => {