-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: suppress OAuth re-auth UI for API profiles #1887
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from 1 commit
57b8722
07e6ec2
c53f316
ae0d1a5
bd225a6
65f830f
ae40ad8
b52b3c0
3e78e30
ba2ebd1
f048162
30f75c6
bbb0631
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,119 @@ | ||||||||||||||||||||
| /** | ||||||||||||||||||||
| * @vitest-environment jsdom | ||||||||||||||||||||
| */ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||||||||||||||||||||
| import '@testing-library/jest-dom/vitest'; | ||||||||||||||||||||
| import { render, screen, waitFor } from '@testing-library/react'; | ||||||||||||||||||||
| import type { ReactNode } from 'react'; | ||||||||||||||||||||
| import { UsageIndicator } from './UsageIndicator'; | ||||||||||||||||||||
| import { useSettingsStore } from '../stores/settings-store'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| vi.mock('../stores/settings-store', () => ({ | ||||||||||||||||||||
| useSettingsStore: vi.fn() | ||||||||||||||||||||
| })); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| vi.mock('./ui/tooltip', () => ({ | ||||||||||||||||||||
| TooltipProvider: ({ children }: { children: ReactNode }) => <>{children}</>, | ||||||||||||||||||||
| Tooltip: ({ children }: { children: ReactNode }) => <>{children}</>, | ||||||||||||||||||||
| TooltipTrigger: ({ children }: { children: ReactNode }) => <>{children}</>, | ||||||||||||||||||||
| TooltipContent: ({ children }: { children: ReactNode }) => <>{children}</> | ||||||||||||||||||||
| })); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| vi.mock('./ui/popover', () => ({ | ||||||||||||||||||||
| Popover: ({ children }: { children: ReactNode }) => <>{children}</>, | ||||||||||||||||||||
| PopoverTrigger: ({ children }: { children: ReactNode }) => <>{children}</>, | ||||||||||||||||||||
| PopoverContent: ({ children }: { children: ReactNode }) => <>{children}</> | ||||||||||||||||||||
| })); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| vi.mock('react-i18next', () => ({ | ||||||||||||||||||||
| useTranslation: vi.fn(() => ({ | ||||||||||||||||||||
| t: (key: string) => { | ||||||||||||||||||||
| const translations: Record<string, string> = { | ||||||||||||||||||||
| 'common:usage.loading': 'Loading usage', | ||||||||||||||||||||
| 'common:usage.reauthRequired': 'Re-authentication required', | ||||||||||||||||||||
| 'common:usage.reauthRequiredDescription': 'Your session has expired. Re-authenticate to continue.', | ||||||||||||||||||||
| 'common:usage.clickToOpenSettings': 'Open settings', | ||||||||||||||||||||
| 'common:usage.dataUnavailable': 'Usage data unavailable', | ||||||||||||||||||||
| 'common:usage.dataUnavailableDescription': 'Usage data is not currently available.', | ||||||||||||||||||||
| 'common:usage.notAvailable': 'N/A' | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| return translations[key] || key; | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| i18n: { language: 'en' } | ||||||||||||||||||||
| })) | ||||||||||||||||||||
| })); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| let mockActiveProfileId: string | null = null; | ||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| function buildAllProfilesUsageResponse(needsReauthentication: boolean) { | ||||||||||||||||||||
| return { | ||||||||||||||||||||
| success: true, | ||||||||||||||||||||
| data: { | ||||||||||||||||||||
| activeProfile: { | ||||||||||||||||||||
| sessionPercent: 0, | ||||||||||||||||||||
| weeklyPercent: 0, | ||||||||||||||||||||
| profileId: 'oauth-profile-1', | ||||||||||||||||||||
| profileName: 'OAuth Profile 1', | ||||||||||||||||||||
| fetchedAt: new Date(), | ||||||||||||||||||||
| needsReauthentication | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| allProfiles: [ | ||||||||||||||||||||
| { | ||||||||||||||||||||
| profileId: 'oauth-profile-1', | ||||||||||||||||||||
| profileName: 'OAuth Profile 1', | ||||||||||||||||||||
| sessionPercent: 0, | ||||||||||||||||||||
| weeklyPercent: 0, | ||||||||||||||||||||
| isAuthenticated: true, | ||||||||||||||||||||
| isRateLimited: false, | ||||||||||||||||||||
| availabilityScore: 100, | ||||||||||||||||||||
| isActive: true, | ||||||||||||||||||||
| needsReauthentication | ||||||||||||||||||||
| } | ||||||||||||||||||||
| ], | ||||||||||||||||||||
| fetchedAt: new Date() | ||||||||||||||||||||
| } | ||||||||||||||||||||
| }; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| describe('UsageIndicator re-auth handling by auth mode', () => { | ||||||||||||||||||||
| beforeEach(() => { | ||||||||||||||||||||
| vi.clearAllMocks(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| vi.mocked(useSettingsStore).mockImplementation((selector) => { | ||||||||||||||||||||
| const state = { activeProfileId: mockActiveProfileId }; | ||||||||||||||||||||
| return selector(state as any); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
Comment on lines
+88
to
+92
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial
If the ♻️ Proposed fix- vi.mocked(useSettingsStore).mockImplementation((selector) => {
- const state = { activeProfileId: mockActiveProfileId };
- return selector(state as any);
- });
+ vi.mocked(useSettingsStore).mockImplementation((selector) => {
+ const state: Pick<Parameters<typeof useSettingsStore>[0] extends (s: infer S) => unknown ? S : never, 'activeProfileId'> = {
+ activeProfileId: mockActiveProfileId
+ };
+ return selector(state as any);
+ });Or, more simply: - const state = { activeProfileId: mockActiveProfileId };
- return selector(state as any);
+ return selector({ activeProfileId: mockActiveProfileId } as Parameters<typeof useSettingsStore>[0] extends (s: infer S) => unknown ? S : never);The simplest acceptable middle ground is keeping - vi.mocked(useSettingsStore).mockImplementation((selector) => {
- const state = { activeProfileId: mockActiveProfileId };
- return selector(state as any);
+ vi.mocked(useSettingsStore).mockImplementation((selector) => {
+ // Minimal store slice required by UsageIndicator
+ const state = { activeProfileId: mockActiveProfileId } satisfies { activeProfileId: string | null };
+ return selector(state as any);
});📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
|
|
||||||||||||||||||||
| (window as any).electronAPI = { | ||||||||||||||||||||
| onUsageUpdated: vi.fn(() => vi.fn()), | ||||||||||||||||||||
| onAllProfilesUsageUpdated: vi.fn(() => vi.fn()), | ||||||||||||||||||||
| requestUsageUpdate: vi.fn().mockResolvedValue({ success: false, data: null }), | ||||||||||||||||||||
| requestAllProfilesUsage: vi.fn().mockResolvedValue(buildAllProfilesUsageResponse(true)) | ||||||||||||||||||||
| }; | ||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('does not show OAuth re-auth UI in API profile mode', async () => { | ||||||||||||||||||||
| mockActiveProfileId = 'api-profile-1'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| render(<UsageIndicator />); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| await waitFor(() => { | ||||||||||||||||||||
| expect(screen.getByRole('button', { name: 'Usage data unavailable' })).toBeInTheDocument(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(screen.queryByText('Re-authentication required')).not.toBeInTheDocument(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| it('shows re-auth UI in OAuth mode when active profile needs re-authentication', async () => { | ||||||||||||||||||||
| mockActiveProfileId = null; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| render(<UsageIndicator />); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| await waitFor(() => { | ||||||||||||||||||||
| expect(screen.getByRole('button', { name: 'Re-authentication required' })).toBeInTheDocument(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(screen.getByText('Re-authentication required')).toBeInTheDocument(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
| }); | ||||||||||||||||||||
|
Comment on lines
+82
to
+144
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Consider adding the symmetrical OAuth-mode event-driven test. Test 3 validates that API profile mode ignores a re-auth update from ♻️ Sketch of the missing testit('shows re-auth UI after onAllProfilesUsageUpdated fires in OAuth mode', async () => {
// Start with no re-auth needed so the initial fetch doesn't trigger re-auth UI.
(window as any).electronAPI.requestAllProfilesUsage = vi
.fn()
.mockResolvedValue(buildAllProfilesUsageResponse(false));
render(<UsageIndicator />);
// Confirm initial state has no re-auth UI.
await waitFor(() => {
expect(screen.queryByText('Re-authentication required')).not.toBeInTheDocument();
});
// Simulate a real-time update where the active profile now needs re-auth.
await act(async () => {
onAllProfilesUsageUpdatedCallback?.(buildAllProfilesUsageResponse(true).data);
});
await waitFor(() => {
expect(
screen.getByRole('button', { name: 'Re-authentication required' })
).toBeInTheDocument();
});
});🤖 Prompt for AI Agents |
||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -22,6 +22,7 @@ import { | |||||||||||||||||||
| import { useTranslation } from 'react-i18next'; | ||||||||||||||||||||
| import { formatTimeRemaining, localizeUsageWindowLabel, hasHardcodedText } from '../../shared/utils/format-time'; | ||||||||||||||||||||
| import type { ClaudeUsageSnapshot, ProfileUsageSummary } from '../../shared/types/agent'; | ||||||||||||||||||||
| import { useSettingsStore } from '../stores/settings-store'; | ||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||
| import type { AppSection } from './settings/AppSettings'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
|
|
@@ -82,6 +83,8 @@ export function UsageIndicator() { | |||||||||||||||||||
| const [isOpen, setIsOpen] = useState(false); | ||||||||||||||||||||
| const [isPinned, setIsPinned] = useState(false); | ||||||||||||||||||||
| const hoverTimeoutRef = useRef<NodeJS.Timeout | null>(null); | ||||||||||||||||||||
| const activeApiProfileId = useSettingsStore((state) => state.activeProfileId); | ||||||||||||||||||||
| const isUsingApiProfile = Boolean(activeApiProfileId); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Helper function to get initials from a profile name | ||||||||||||||||||||
|
|
@@ -309,6 +312,10 @@ export function UsageIndicator() { | |||||||||||||||||||
| (hasHardcodedText(usage?.weeklyResetTime) ? undefined : usage?.weeklyResetTime)) | ||||||||||||||||||||
| : (hasHardcodedText(usage?.weeklyResetTime) ? undefined : usage?.weeklyResetTime); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Re-authentication is only relevant for OAuth mode, never for API profiles | ||||||||||||||||||||
| const showReauthForActiveAccount = !isUsingApiProfile && Boolean(usage?.needsReauthentication); | ||||||||||||||||||||
| const showUnavailableReauth = !isUsingApiProfile && activeProfileNeedsReauth; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| useEffect(() => { | ||||||||||||||||||||
| // Listen for usage updates from main process | ||||||||||||||||||||
| const unsubscribe = window.electronAPI.onUsageUpdated((snapshot: ClaudeUsageSnapshot) => { | ||||||||||||||||||||
|
|
@@ -322,9 +329,9 @@ export function UsageIndicator() { | |||||||||||||||||||
| // Filter out the active profile - we only want to show "other" profiles | ||||||||||||||||||||
| const nonActiveProfiles = allProfilesUsage.allProfiles.filter(p => !p.isActive); | ||||||||||||||||||||
| setOtherProfiles(nonActiveProfiles); | ||||||||||||||||||||
| // Track if active profile needs re-auth | ||||||||||||||||||||
| // Track if active profile needs re-auth (OAuth mode only) | ||||||||||||||||||||
| const activeProfile = allProfilesUsage.allProfiles.find(p => p.isActive); | ||||||||||||||||||||
| setActiveProfileNeedsReauth(activeProfile?.needsReauthentication ?? false); | ||||||||||||||||||||
| setActiveProfileNeedsReauth(isUsingApiProfile ? false : (activeProfile?.needsReauthentication ?? false)); | ||||||||||||||||||||
|
||||||||||||||||||||
| // Track if active profile needs re-auth (OAuth mode only) | |
| const activeProfile = allProfilesUsage.allProfiles.find(p => p.isActive); | |
| setActiveProfileNeedsReauth(activeProfile?.needsReauthentication ?? false); | |
| setActiveProfileNeedsReauth(isUsingApiProfile ? false : (activeProfile?.needsReauthentication ?? false)); | |
| // Track if active profile needs re-auth (OAuth mode only) | |
| const activeProfile = allProfilesUsage.allProfiles.find(p => p.isActive); | |
| setActiveProfileNeedsReauth(activeProfile?.needsReauthentication ?? false); |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the previous comment, the ternary operator isUsingApiProfile ? false : (...) is redundant here. The activeProfile?.needsReauthentication ?? false already handles the case where activeProfile is undefined, and isUsingApiProfile will implicitly make activeProfileNeedsReauth false.
| // Track if active profile needs re-auth (OAuth mode only) | |
| const activeProfile = result.data.allProfiles.find(p => p.isActive); | |
| if (activeProfile?.needsReauthentication) { | |
| setActiveProfileNeedsReauth(true); | |
| } | |
| setActiveProfileNeedsReauth(isUsingApiProfile ? false : (activeProfile?.needsReauthentication ?? false)); | |
| // Track if active profile needs re-auth (OAuth mode only) | |
| const activeProfile = result.data.allProfiles.find(p => p.isActive); | |
| setActiveProfileNeedsReauth(activeProfile?.needsReauthentication ?? false); |
Uh oh!
There was an error while loading. Please reload this page.