-
-
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 3 commits
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,144 @@ | ||||||||||||||||||||
| /** | ||||||||||||||||||||
| * @vitest-environment jsdom | ||||||||||||||||||||
| */ | ||||||||||||||||||||
|
|
||||||||||||||||||||
| import { describe, it, expect, vi, beforeEach } from 'vitest'; | ||||||||||||||||||||
| import '@testing-library/jest-dom/vitest'; | ||||||||||||||||||||
| import { act, 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
|
||||||||||||||||||||
| let onAllProfilesUsageUpdatedCallback: ((allProfilesUsage: AllProfilesUsagePayload) => void) | undefined; | ||||||||||||||||||||
| type AllProfilesUsagePayload = ReturnType<typeof buildAllProfilesUsageResponse>['data']; | ||||||||||||||||||||
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(); | ||||||||||||||||||||
| mockActiveProfileId = null; | ||||||||||||||||||||
| onAllProfilesUsageUpdatedCallback = undefined; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| vi.mocked(useSettingsStore).mockImplementation((selector) => { | ||||||||||||||||||||
| const state = { activeProfileId: mockActiveProfileId } satisfies { activeProfileId: string | null }; | ||||||||||||||||||||
| 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((callback: (allProfilesUsage: AllProfilesUsagePayload) => void) => { | ||||||||||||||||||||
| onAllProfilesUsageUpdatedCallback = callback; | ||||||||||||||||||||
| return 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 () => { | ||||||||||||||||||||
| 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
|
||||||||||||||||||||
|
|
||||||||||||||||||||
| it('ignores re-auth updates from usage events in API profile mode', async () => { | ||||||||||||||||||||
| mockActiveProfileId = 'api-profile-1'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| render(<UsageIndicator />); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| await waitFor(() => { | ||||||||||||||||||||
| expect(onAllProfilesUsageUpdatedCallback).toBeDefined(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| await act(async () => { | ||||||||||||||||||||
| onAllProfilesUsageUpdatedCallback?.(buildAllProfilesUsageResponse(true).data); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| await waitFor(() => { | ||||||||||||||||||||
| expect(screen.getByRole('button', { name: 'Usage data unavailable' })).toBeInTheDocument(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| expect(screen.queryByText('Re-authentication required')).not.toBeInTheDocument(); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| }); | ||||||||||||||||||||
|
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 |
|---|---|---|
|
|
@@ -24,7 +24,7 @@ export default defineConfig({ | |
| }, | ||
| resolve: { | ||
| alias: { | ||
| '@': resolve(__dirname, 'src'), | ||
| '@': resolve(__dirname, 'src/renderer'), | ||
| '@main': resolve(__dirname, 'src/main'), | ||
| '@renderer': resolve(__dirname, 'src/renderer'), | ||
| '@shared': resolve(__dirname, 'src/shared') | ||
|
Comment on lines
+27
to
30
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
Both aliases point to 🤖 Prompt for AI Agents |
||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Uh oh!
There was an error while loading. Please reload this page.