fix: suppress OAuth re-auth UI for API profiles#1887
fix: suppress OAuth re-auth UI for API profiles#1887alexx-ftw wants to merge 13 commits intoAndyMik90:developfrom
Conversation
Ensure UsageIndicator only shows re-authentication prompts in OAuth mode and add regression tests for API and OAuth behavior.
|
|
Summary of ChangesHello @alexx-ftw, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the user experience for the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUsageIndicator now distinguishes API-profile vs OAuth modes by reading Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UsageIndicator
participant SettingsStore
participant ElectronAPI
User->>UsageIndicator: mount / trigger refresh
UsageIndicator->>SettingsStore: read activeProfileId & profiles
SettingsStore-->>UsageIndicator: activeProfileId, profile metadata
UsageIndicator->>ElectronAPI: request all-profiles usage
ElectronAPI-->>UsageIndicator: all-profiles usage payload
UsageIndicator->>UsageIndicator: compute showReauthForActiveAccount / showUnavailableReauth
UsageIndicator-->>User: render badge/icons/account list (reauth/unavailable as appropriate)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request effectively addresses the issue of suppressing OAuth re-authentication UI for API profiles and custom endpoint modes. The changes correctly gate the re-auth UI to OAuth mode only and prevent warnings when usage data is unavailable for API profiles. The addition of regression tests for both API and OAuth modes is a good practice, ensuring the new logic works as expected and prevents future regressions. The code is well-structured and the logic for determining showReauthForActiveAccount and showUnavailableReauth is clear.
| // 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)); |
There was a problem hiding this comment.
The comment "Track if active profile needs re-auth (OAuth mode only)" is good, but the ternary operator isUsingApiProfile ? false : (...) could be simplified. If isUsingApiProfile is true, activeProfileNeedsReauth will always be false due to the new logic, making the isUsingApiProfile ? false : redundant. The activeProfile?.needsReauthentication ?? false already handles the case where activeProfile is undefined.
| // 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); |
| // 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)); |
There was a problem hiding this comment.
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); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/frontend/src/renderer/components/UsageIndicator.test.tsx`:
- Around line 88-93: Add a test that captures and invokes the
onAllProfilesUsageUpdated event callback so the component's event-driven branch
(the code that calls setActiveProfileNeedsReauth around lines 321–335) is
exercised rather than relying only on the initial requestAllProfilesUsage
promise. Modify the test setup of (window as any).electronAPI to implement
onAllProfilesUsageUpdated as a vi.fn(cb => { store cb in a local variable;
return vi.fn(); }) and then, after rendering the component, call that stored
callback with buildAllProfilesUsageResponse(true).data (wrap the call in act())
to assert that setActiveProfileNeedsReauth/state changes occur; keep
onUsageUpdated unchanged or mirror the same pattern if needed.
- Line 47: The test suite uses a shared variable mockActiveProfileId that can
leak state between tests; add a beforeEach in the same describe block that
resets mockActiveProfileId = null before each test so every test starts with a
clean value (update the describe's setup to include a beforeEach that sets
mockActiveProfileId to null and ensure any existing per-test assignments remain
intact for tests that overwrite it).
- Around line 83-86: Replace the unsafe cast in the test mock by typing the
minimal store slice instead of using "as any": update the
vi.mocked(useSettingsStore).mockImplementation to accept the selector and pass a
properly typed object containing only activeProfileId (use the store's selector
type or create a small interface like { activeProfileId: typeof
mockActiveProfileId }) so the selector invocation in useSettingsStore receives a
correctly typed state; this removes the as any cast while keeping the mock
focused on the minimal shape required for the test.
In `@apps/frontend/src/renderer/components/UsageIndicator.tsx`:
- Line 25: Replace the relative import in UsageIndicator.tsx with the tsconfig
path alias: change the import that references useSettingsStore from
'../stores/settings-store' to use the renderer alias (e.g., import {
useSettingsStore } from '@/stores/settings-store'), ensuring the symbol
useSettingsStore and the UsageIndicator component continue to compile with the
new path.
| vi.mocked(useSettingsStore).mockImplementation((selector) => { | ||
| const state = { activeProfileId: mockActiveProfileId }; | ||
| return selector(state as any); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
as any cast masks store type mismatches — consider typing the minimal state slice.
If the activeProfileId field is renamed or its type changes in the store, TypeScript won't catch the mismatch here.
♻️ 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 as any but documenting the fields the mock exposes:
- 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/src/renderer/components/UsageIndicator.test.tsx` around lines
83 - 86, Replace the unsafe cast in the test mock by typing the minimal store
slice instead of using "as any": update the
vi.mocked(useSettingsStore).mockImplementation to accept the selector and pass a
properly typed object containing only activeProfileId (use the store's selector
type or create a small interface like { activeProfileId: typeof
mockActiveProfileId }) so the selector invocation in useSettingsStore receives a
correctly typed state; this removes the as any cast while keeping the mock
focused on the minimal shape required for the test.
Capture existing package-lock version metadata update so the branch is fully synced for review and CI.
Keep OAuth re-auth state explicitly disabled in API profile mode across both fetch and event update paths, and add regression coverage for the event listener branch. Align Vitest's @ alias with renderer path aliases so renderer imports resolve the same way in tests and runtime.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/frontend/src/renderer/components/UsageIndicator.tsx (1)
319-369: 🧹 Nitpick | 🔵 Trivial
isUsingApiProfilein the effect dependency array causes full re-subscribe on mode change — verify this is intentional.When
isUsingApiProfilechanges (e.g., user activates/deactivates an API profile), the entire effect tears down and re-runs: old event listeners are unsubscribed, new ones are created, and bothrequestUsageUpdateandrequestAllProfilesUsagefire again. This is functionally correct — the new closures capture the updatedisUsingApiProfilevalue — but it does trigger extra IPC round-trips on every mode toggle.This is likely acceptable since mode toggles are infrequent user actions, but worth confirming it doesn't cause a visible flicker (loading state briefly showing).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/renderer/components/UsageIndicator.tsx` around lines 319 - 369, The effect currently depends on isUsingApiProfile causing full teardown and re-subscribe when the mode toggles; to avoid extra IPC calls, remove isUsingApiProfile from the dependency array and instead create a ref (e.g., isUsingApiProfileRef) that you update in a small separate useEffect when isUsingApiProfile changes, then inside this main useEffect read isUsingApiProfileRef.current when calling setActiveProfileNeedsReauth (in both the onAllProfilesUsage handler and the requestAllProfilesUsage.then block); keep the same unsubscribe/unsubscribeAllProfiles cleanup logic and leave requestUsageUpdate/requestAllProfilesUsage calls in the main effect so they run only once on mount.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/frontend/src/renderer/components/UsageIndicator.test.tsx`:
- Around line 47-49: The type alias AllProfilesUsagePayload currently depends on
buildAllProfilesUsageResponse always returning a non-null 'data' property;
update the file to either add a short explanatory comment above the alias
mentioning this coupling to buildAllProfilesUsageResponse or replace the alias
with an independent type definition that matches the structure (e.g., define an
explicit interface/type for the payload) so future maintainers won't be
surprised by the implicit dependency on buildAllProfilesUsageResponse.
- Around line 10-14: The test imports and mocks the settings store using a
relative path but the component under test (UsageIndicator.tsx) uses the
'@/stores/settings-store' alias; change both the import statement and the
vi.mock call in UsageIndicator.test.tsx to import from '@/stores/settings-store'
(keep the same symbol useSettingsStore) so the test and component use the same
path alias per tsconfig and project guidelines.
In `@apps/frontend/vitest.config.ts`:
- Around line 27-30: The alias mapping defines both '@' and '@renderer' to
resolve(__dirname, 'src/renderer'), making '@renderer' redundant; either remove
the duplicate '@renderer' entry from the alias object or keep it but add an
inline comment explaining the redundancy/intent so future maintainers understand
that '@' and '@renderer' intentionally resolve to the same path (update the
aliases object where '@' and '@renderer' are defined).
---
Outside diff comments:
In `@apps/frontend/src/renderer/components/UsageIndicator.tsx`:
- Around line 319-369: The effect currently depends on isUsingApiProfile causing
full teardown and re-subscribe when the mode toggles; to avoid extra IPC calls,
remove isUsingApiProfile from the dependency array and instead create a ref
(e.g., isUsingApiProfileRef) that you update in a small separate useEffect when
isUsingApiProfile changes, then inside this main useEffect read
isUsingApiProfileRef.current when calling setActiveProfileNeedsReauth (in both
the onAllProfilesUsage handler and the requestAllProfilesUsage.then block); keep
the same unsubscribe/unsubscribeAllProfiles cleanup logic and leave
requestUsageUpdate/requestAllProfilesUsage calls in the main effect so they run
only once on mount.
| '@': resolve(__dirname, 'src/renderer'), | ||
| '@main': resolve(__dirname, 'src/main'), | ||
| '@renderer': resolve(__dirname, 'src/renderer'), | ||
| '@shared': resolve(__dirname, 'src/shared') |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
@ and @renderer now resolve to the same path.
Both aliases point to src/renderer. This is intentional (per commit message) and not a problem, but worth noting for future maintainers that @renderer is now fully redundant with @.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/vitest.config.ts` around lines 27 - 30, The alias mapping
defines both '@' and '@renderer' to resolve(__dirname, 'src/renderer'), making
'@renderer' redundant; either remove the duplicate '@renderer' entry from the
alias object or keep it but add an inline comment explaining the
redundancy/intent so future maintainers understand that '@' and '@renderer'
intentionally resolve to the same path (update the aliases object where '@' and
'@renderer' are defined).
Use the renderer alias for the settings store mock and document the payload coupling/minimal store slice to match review feedback and improve test clarity.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/frontend/src/renderer/components/UsageIndicator.test.tsx`:
- Around line 82-146: Add a new test in the same describe block that verifies
OAuth mode reacts to onAllProfilesUsageUpdated: mock
window.electronAPI.requestAllProfilesUsage to return
buildAllProfilesUsageResponse(false) so initial render of UsageIndicator shows
no "Re-authentication required", render <UsageIndicator />, wait for absence of
re-auth UI, then invoke onAllProfilesUsageUpdatedCallback with
buildAllProfilesUsageResponse(true).data and assert the "Re-authentication
required" button/text appears; reference the existing
onAllProfilesUsageUpdatedCallback, requestAllProfilesUsage, UsageIndicator, and
buildAllProfilesUsageResponse helpers to implement this.
- Around line 117-125: In the "shows re-auth UI in OAuth mode when active
profile needs re-authentication" test for UsageIndicator, remove the redundant
expect(screen.getByText('Re-authentication required')) assertion (it duplicates
the earlier expect on the button via getByRole) or, if you intended to assert a
second separate element with that text (e.g., tooltip/body), replace it with
expect(screen.getAllByText('Re-authentication
required')).toHaveLength(<expectedCount>) to make the intent explicit; update
the test accordingly so only the meaningful assertion remains.
| describe('UsageIndicator re-auth handling by auth mode', () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mockActiveProfileId = null; | ||
| onAllProfilesUsageUpdatedCallback = undefined; | ||
|
|
||
| vi.mocked(useSettingsStore).mockImplementation((selector) => { | ||
| // Minimal store slice required by UsageIndicator. | ||
| const state = { activeProfileId: mockActiveProfileId } satisfies { activeProfileId: string | null }; | ||
| return selector(state as any); | ||
| }); | ||
|
|
||
| (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)) | ||
| }; | ||
| }); | ||
|
|
||
| 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(); | ||
| }); | ||
|
|
||
| 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(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding the symmetrical OAuth-mode event-driven test.
Test 3 validates that API profile mode ignores a re-auth update from onAllProfilesUsageUpdated. The complementary case — that OAuth mode does react to the same event and flips to show re-auth UI — is not covered. Without it, a regression in the event-handler branch for OAuth mode (e.g. the callback being no-op'd accidentally) could pass the existing suite undetected.
♻️ Sketch of the missing test
it('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
Verify each finding against the current code and only fix it if needed.
In `@apps/frontend/src/renderer/components/UsageIndicator.test.tsx` around lines
82 - 146, Add a new test in the same describe block that verifies OAuth mode
reacts to onAllProfilesUsageUpdated: mock
window.electronAPI.requestAllProfilesUsage to return
buildAllProfilesUsageResponse(false) so initial render of UsageIndicator shows
no "Re-authentication required", render <UsageIndicator />, wait for absence of
re-auth UI, then invoke onAllProfilesUsageUpdatedCallback with
buildAllProfilesUsageResponse(true).data and assert the "Re-authentication
required" button/text appears; reference the existing
onAllProfilesUsageUpdatedCallback, requestAllProfilesUsage, UsageIndicator, and
buildAllProfilesUsageResponse helpers to implement this.
Drop the duplicate re-auth text assertion in the OAuth-mode test since the role-based button assertion already verifies the same rendered state.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/frontend/src/renderer/components/UsageIndicator.test.tsx`:
- Around line 82-144: Add the missing event-driven OAuth-mode test: create a
test that renders UsageIndicator with window.electronAPI.requestAllProfilesUsage
mocked to return buildAllProfilesUsageResponse(false) so no re-auth UI appears
initially, wait for onAllProfilesUsageUpdatedCallback to be defined, assert
"Re-authentication required" is not present, then call
onAllProfilesUsageUpdatedCallback with buildAllProfilesUsageResponse(true).data
inside act and waitFor the button with role 'button' name 'Re-authentication
required' to appear; reference the existing onAllProfilesUsageUpdatedCallback,
UsageIndicator, requestAllProfilesUsage, and buildAllProfilesUsageResponse
helpers to locate and implement the test.
- Around line 88-92: The mocked useSettingsStore implementation currently uses a
narrow state slice with a `satisfies { activeProfileId: string | null }` check
but still casts to `as any` for the selector; keep the `satisfies` line and
retain the `as any` cast for `selector(state as any)` to satisfy the selector's
full-store expectation, but add a short inline comment explaining why the `as
any` cast is necessary (selector expects full store shape) and reference the
identifiers `useSettingsStore`, `mockActiveProfileId`, `state`, and
`UsageIndicator` so future readers understand this is intentional.
Ensure stuck-task recovery always sends TASK_STATUS_CHANGE events even when auto-restart exits early due to missing auth/git/profile initialization. This keeps the UI in sync so Recover no longer appears to do nothing when restart cannot proceed.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts (2)
1268-1347:⚠️ Potential issue | 🟠 Major
sendStatusChange('in_progress')can fire even when the restart silently fails.
newStatus = 'in_progress'is assigned at line 1274 before thetryblock that callsagentManager.start*. Thecatchat line 1340 swallows the error and lets execution continue. If eitherstartSpecCreationorstartTaskExecutionthrows,autoRestartedstaysfalse, but by that point the plan files are already written toin_progress(lines 1281–1290), the cache is invalidated (line 1294), andsendStatusChange('in_progress')fires at line 1347. The task then appears "in progress" in the UI with no actual process running and no visible error — a harder-to-diagnose stuck state than the original one.Move
newStatus = 'in_progress'to just afterautoRestarted = true(line 1338), and push the plan-file writes + cache invalidation to the same location, so they only happen once the restart is confirmed.🐛 Proposed fix
try { // Cancel any pending fallback timer from previous process exit cancelFallbackTimer(taskId); - // Set status to in_progress for the restart - newStatus = 'in_progress'; - - // Update plan status for restart - write to ALL locations - if (plan) { - plan.status = 'in_progress'; - plan.planStatus = 'in_progress'; - const restartPlanContent = JSON.stringify(plan, null, 2); - for (const pathToUpdate of planPathsToUpdate) { - try { - writeFileAtomicSync(pathToUpdate, restartPlanContent); - } catch (writeError) { - console.error(`[Recovery] Failed to write plan file for restart at ${pathToUpdate}:`, writeError); - } - } - projectStore.invalidateTasksCache(project.id); - } - // Start the task execution // ... (file-watcher, spec-existence check, startSpecCreation / startTaskExecution) autoRestarted = true; + + // Only persist 'in_progress' after we know the agent actually started + newStatus = 'in_progress'; + if (plan) { + plan.status = 'in_progress'; + plan.planStatus = 'in_progress'; + const restartPlanContent = JSON.stringify(plan, null, 2); + for (const pathToUpdate of planPathsToUpdate) { + try { + writeFileAtomicSync(pathToUpdate, restartPlanContent); + } catch (writeError) { + console.error(`[Recovery] Failed to write plan file for restart at ${pathToUpdate}:`, writeError); + } + } + projectStore.invalidateTasksCache(project.id); + } console.warn(`[Recovery] Auto-restarted task ${taskId}`); } catch (restartError) { console.error('Failed to auto-restart task after recovery:', restartError); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts` around lines 1268 - 1347, The handler currently sets newStatus = 'in_progress' and writes plan files + invalidates cache before attempting to start the agent, which allows sendStatusChange('in_progress') to fire even if agentManager.startSpecCreation or agentManager.startTaskExecution fail; move the assignment newStatus = 'in_progress' and the block that updates plan.status/plan.planStatus, writes to each path in planPathsToUpdate, and calls projectStore.invalidateTasksCache to immediately after autoRestarted = true (i.e., only after agentManager.startSpecCreation/startTaskExecution resolve without throwing), keep cancelFallbackTimer(taskId) and the fileWatcher.start call where they are, and ensure the catch around the restart still logs the error and does not change newStatus or call sendStatusChange('in_progress') when autoRestarted is false so the UI isn't updated on failed restart; reference functions/variables: sendStatusChange, agentManager.startSpecCreation, agentManager.startTaskExecution, cancelFallbackTimer, planPathsToUpdate, projectStore.invalidateTasksCache, autoRestarted, newStatus.
1221-1255:⚠️ Potential issue | 🟠 MajorThe plan status is sent to the renderer but never written to disk in non-autoRestart paths — the next refresh will show the old status.
The
allCompletedpath correctly sequences: write plan →invalidateTasksCache→sendStatusChange.All other paths that call
sendStatusChange(the three early-exit checks at lines 1221, 1239, 1255, and the terminal line 1347) skip the write and cache invalidation:
- Lines 1063–1070:
plan.statusandplan.planStatusare updated in memory only.- Lines 1095–1140:
resetStuckSubtasksreads the original plan file from disk, resets subtask statuses within it, and writes the file back—but since the plan file on disk still has the old status fields, the written file retains the old status.- No write of the updated
planobject occurs; the in-memory status changes are lost.- Lines 1221, 1239, 1255, 1347:
sendStatusChange(newStatus)fires without having persistedplan.statusto disk or invalidated the cache.Result: The renderer receives the IPC notification, re-reads task data via
getTasks(), and immediately displays the old status again from the file.Fix: After the
resetStuckSubtasksloop and attempt-history cleanup (before the early-exit paths and line 1347), write the updatedplanobject to allplanPathsToUpdatelocations, callprojectStore.invalidateTasksCache(project.id), and then callsendStatusChange—mirroring the correct sequence in theallCompletedbranch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts` around lines 1221 - 1255, The plan status changes are only sent via sendStatusChange(newStatus) but not persisted causing UI to reload the old status; after performing resetStuckSubtasks and attempt-history cleanup (i.e., the logic around resetStuckSubtasks and the in-memory plan mutations that follow), persist the updated plan to each path in planPathsToUpdate, call projectStore.invalidateTasksCache(project.id), and only then call sendStatusChange(newStatus) for the early-exit branches (the recovery/auth/initResult checks and the terminal branch) — mirror the write → invalidate → send sequence used in the allCompleted branch so the renderer reads the updated status from disk.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts`:
- Around line 1268-1347: The handler currently sets newStatus = 'in_progress'
and writes plan files + invalidates cache before attempting to start the agent,
which allows sendStatusChange('in_progress') to fire even if
agentManager.startSpecCreation or agentManager.startTaskExecution fail; move the
assignment newStatus = 'in_progress' and the block that updates
plan.status/plan.planStatus, writes to each path in planPathsToUpdate, and calls
projectStore.invalidateTasksCache to immediately after autoRestarted = true
(i.e., only after agentManager.startSpecCreation/startTaskExecution resolve
without throwing), keep cancelFallbackTimer(taskId) and the fileWatcher.start
call where they are, and ensure the catch around the restart still logs the
error and does not change newStatus or call sendStatusChange('in_progress') when
autoRestarted is false so the UI isn't updated on failed restart; reference
functions/variables: sendStatusChange, agentManager.startSpecCreation,
agentManager.startTaskExecution, cancelFallbackTimer, planPathsToUpdate,
projectStore.invalidateTasksCache, autoRestarted, newStatus.
- Around line 1221-1255: The plan status changes are only sent via
sendStatusChange(newStatus) but not persisted causing UI to reload the old
status; after performing resetStuckSubtasks and attempt-history cleanup (i.e.,
the logic around resetStuckSubtasks and the in-memory plan mutations that
follow), persist the updated plan to each path in planPathsToUpdate, call
projectStore.invalidateTasksCache(project.id), and only then call
sendStatusChange(newStatus) for the early-exit branches (the
recovery/auth/initResult checks and the terminal branch) — mirror the write →
invalidate → send sequence used in the allCompleted branch so the renderer reads
the updated status from disk.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
PR #1887 reviewed: 14 findings (3 structural issues). Verdict: needs_review.
Findings (14 selected of 14 total)
🟠 [QLT-1] [HIGH] Vitest alias change may break other tests
📁 apps/frontend/vitest.config.ts:27
Changing the @ alias from src to src/renderer is a global config change that will break any existing test importing main-process or shared code via @/. For example, @/shared/utils/... or @/main/... imports would now resolve to src/renderer/shared/utils/... which doesn't exist. This change appears to have been made to satisfy a single test file's import (@/stores/settings-store) rather than fixing the import in that test.
Suggested fix:
Revert the alias to `'@': resolve(__dirname, 'src')` and update the test import to use `@renderer/stores/settings-store` or `@/renderer/stores/settings-store` instead.
🟡 [QLT-2] [MEDIUM] Stale closure risk in useEffect with partial dependency array
📁 apps/frontend/src/renderer/components/UsageIndicator.tsx:366
The useEffect sets up IPC listeners that capture isUsingApiProfile in their closures, but the dependency array only includes [isUsingApiProfile]. When isUsingApiProfile changes, the effect re-runs, tearing down old listeners and creating new ones. However, the requestAllProfilesUsage fetch inside the effect also re-fires on every toggle, which may cause unnecessary network requests. More critically, the onUsageUpdated listener callback doesn't use isUsingApiProfile but gets torn down and recreated anyway, potentially losing in-flight events.
Suggested fix:
Consider splitting into separate useEffects: one for the IPC listeners (stable, runs once) and one that reacts to `isUsingApiProfile` changes to recompute derived state, or use a ref for `isUsingApiProfile` inside the listener callbacks.
🟡 [QLT-3] [MEDIUM] Duplicated profile filtering and re-auth logic
📁 apps/frontend/src/renderer/components/UsageIndicator.tsx:329
The logic for filtering non-active profiles, finding the active profile, and computing setActiveProfileNeedsReauth(!isUsingApiProfile && Boolean(activeProfile?.needsReauthentication)) is duplicated in three places: the onAllProfilesUsageUpdated callback, the initial requestAllProfilesUsage promise handler, and conceptually in the derived showUnavailableReauth/showReauthForActiveAccount variables. This violates DRY and increases the risk of inconsistency.
Suggested fix:
Extract a helper function like `processAllProfilesData(data, isUsingApiProfile)` that returns `{ otherProfiles, activeProfileNeedsReauth }` and call it from both the callback and the initial fetch handler.
🟡 [QLT-4] [MEDIUM] Redundant re-auth suppression at multiple layers
📁 apps/frontend/src/renderer/components/UsageIndicator.tsx:314
Re-auth is suppressed both when setting state (setActiveProfileNeedsReauth(!isUsingApiProfile && ...)) AND when reading it (showUnavailableReauth = !isUsingApiProfile && activeProfileNeedsReauth). The double-gating is unnecessary and confusing — if the state is already guaranteed to be false for API profiles, the derived variable doesn't need the check, or vice versa. This creates a maintenance trap where future developers may only update one layer.
Suggested fix:
Choose one place to enforce the `isUsingApiProfile` check: either in the state setter or in the derived variables, not both.
🔵 [QLT-5] [LOW] Naming convention: isUsingApiProfile could be misleading
📁 apps/frontend/src/renderer/components/UsageIndicator.tsx:85
The variable isUsingApiProfile is derived from Boolean(activeProfileId). The name suggests it specifically detects API profile mode, but activeProfileId being non-null may not exclusively mean 'API profile' — it depends on the store semantics. If OAuth profiles can also have an activeProfileId, this logic would be incorrect.
Suggested fix:
Verify that `activeProfileId` being truthy always means 'API profile mode'. If so, add a comment clarifying this invariant. If not, use a more specific check like `activeProfile?.type === 'api'`.
🔵 [QLT-6] [LOW] Test uses tightly-coupled type derived from helper function
📁 apps/frontend/src/renderer/components/UsageIndicator.test.tsx:54
The test defines AllProfilesUsagePayload as ReturnType<typeof buildAllProfilesUsageResponse>['data'], coupling the type to the test helper's implementation. If the helper shape changes, the type silently changes too. This is fragile and makes the test harder to understand.
Suggested fix:
Import or define the actual payload type from the source code (e.g., from shared types) rather than deriving it from a test helper.
🔵 [QLT-7] [LOW] Missing test cleanup of window.electronAPI
📁 apps/frontend/src/renderer/components/UsageIndicator.test.tsx:96
The test sets (window as any).electronAPI in beforeEach but never cleans it up in an afterEach. This can leak state between test suites and cause hard-to-debug failures when tests run in a different order.
Suggested fix:
Add an `afterEach` block that deletes or resets `(window as any).electronAPI`.
🔵 [QLT-8] [LOW] sendStatusChange helper not called in all early-return paths
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:977
The sendStatusChange helper is added to some early-return recovery paths but there may be other early returns in the handler (e.g., the initial if (!task) check) where status notification is also missing. A systematic review of all early returns would be warranted to ensure consistency.
Suggested fix:
Audit all early-return paths in the handler to determine if `sendStatusChange` should be called, and document intentional omissions.
🟠 [DEEP-1] [HIGH] Vitest alias change from @ → src/renderer breaks all non-renderer imports project-wide
📁 apps/frontend/vitest.config.ts:27
The alias @ was changed from resolve(__dirname, 'src') to resolve(__dirname, 'src/renderer'). This means any existing test that imports from @/main/... or @/shared/... (or any non-renderer path) will now resolve incorrectly, as @ no longer points to the src root. The change was made to satisfy the new test file's @/stores/settings-store import (which lives under src/renderer/stores), but it silently breaks all other tests that rely on @ pointing to src. This is a data consistency / correctness problem for the entire test suite.
Suggested fix:
Keep `@` pointing to `resolve(__dirname, 'src')` and instead use the existing `@renderer` alias in the test file, or add a separate alias specifically for renderer stores. Alternatively, ensure all production code also uses `@` as `src/renderer` (unlikely given existing conventions).
🟡 [DEEP-2] [MEDIUM] Race condition: isUsingApiProfile in useEffect dependency can cause stale closures and event handler leaks
📁 apps/frontend/src/renderer/components/UsageIndicator.tsx:366
The useEffect that registers IPC listeners (onUsageUpdated, onAllProfilesUsageUpdated) and fires initial fetches now depends on [isUsingApiProfile]. Every time the user toggles between API and OAuth profiles, the effect tears down and re-registers listeners and re-fetches data. However, the async .then() callbacks from requestAllProfilesUsage capture the isUsingApiProfile value at the time the effect ran. If the profile changes while the promise is in flight, the callback will use a stale isUsingApiProfile value and may set incorrect state. Additionally, repeated rapid profile switches will cause multiple concurrent fetches whose results may arrive out of order.
Suggested fix:
Add an `isCancelled` flag (set to `true` in the cleanup function) and check it before calling setState in the `.then()` callback. This is a standard pattern to avoid stale closure updates after effect cleanup.
🟡 [DEEP-3] [MEDIUM] Derived state showReauthForActiveAccount and showUnavailableReauth computed outside useMemo risks stale values
📁 apps/frontend/src/renderer/components/UsageIndicator.tsx:314
showReauthForActiveAccount and showUnavailableReauth are computed as plain variables in the render body using both isUsingApiProfile and state variables (usage, activeProfileNeedsReauth). While these will re-compute on each render, the activeProfileNeedsReauth state itself is set inside IPC callbacks that also gate on isUsingApiProfile. If the profile changes between a state update and the next render, there's a brief window where activeProfileNeedsReauth is true (set during OAuth mode) but isUsingApiProfile has flipped to true, meaning showUnavailableReauth correctly becomes false. However, the activeProfileNeedsReauth state is never explicitly cleared when switching to API mode — it retains whatever value was last set. This means if the user switches from OAuth (with reauth=true) to API and then back to OAuth before a new fetch completes, the stale true could flash briefly.
Suggested fix:
Reset `activeProfileNeedsReauth` to `false` at the start of the useEffect when `isUsingApiProfile` changes, or derive it entirely from the latest fetch rather than persisting as separate state.
🟡 [DEEP-4] [MEDIUM] sendStatusChange helper is defined inside a scope but newStatus may not be set on all code paths where it's called
📁 apps/frontend/src/main/ipc-handlers/task/execution-handlers.ts:1218
The sendStatusChange helper is defined early in the handler (line ~977), but newStatus is passed as a parameter. At lines 1218, 1236, and 1252, sendStatusChange(newStatus) is called in early-return paths. The variable newStatus must have been assigned before these calls. Looking at the diff context, newStatus appears to be computed earlier in the recovery flow. However, the helper sends the notification before the function returns the success response. If the renderer processes the status change notification and immediately re-fetches task data, the cache might not yet be invalidated (since invalidateTasksCache is not called in all these early-return paths), leading to the renderer seeing stale task data.
Suggested fix:
Call `projectStore.invalidateTasksCache(project.id)` before `sendStatusChange(newStatus)` in the early-return paths at lines 1218, 1236, and 1252, similar to how it's done at line 1105.
🔵 [DEEP-5] [LOW] Test mock for requestAllProfilesUsage always returns needsReauthentication: true which conflates test scenarios
📁 apps/frontend/src/renderer/components/UsageIndicator.test.tsx:100
The default mock for requestAllProfilesUsage in beforeEach always returns buildAllProfilesUsageResponse(true) (needs re-auth). The test 'does not show OAuth re-auth UI in API profile mode' relies on the component ignoring this because isUsingApiProfile is true. However, this test doesn't verify the component correctly handles the case where the API returns needsReauthentication: false — it only tests the gating logic. A missing test for the negative case (OAuth mode with needsReauthentication: false should NOT show re-auth) reduces confidence.
Suggested fix:
Add a test case: OAuth mode (no activeProfileId) with `needsReauthentication: false` should NOT show re-auth UI, confirming both conditions must be true.
🔵 [DEEP-6] [LOW] Test determines API vs OAuth mode solely by activeProfileId presence, not by profile type
📁 apps/frontend/src/renderer/components/UsageIndicator.tsx:84
The component determines isUsingApiProfile = Boolean(activeApiProfileId) where activeApiProfileId comes from state.activeProfileId. The naming activeProfileId in the settings store is ambiguous — it could refer to any profile type (OAuth or API). If an OAuth profile also sets activeProfileId, the gating logic would incorrectly treat it as an API profile and suppress re-auth UI. The test uses 'api-profile-1' as the ID, but the component has no way to distinguish profile types from the ID alone.
Suggested fix:
Verify that `activeProfileId` in the settings store is only set for API/custom-endpoint profiles and is null/undefined for OAuth profiles. If both profile types can set this field, add explicit profile type checking rather than relying on truthiness.
This review was generated by Auto Claude.
Treat active API profile credentials as valid task auth across start/update/recovery and agent execution paths so Done→In Progress and recover flows no longer fail with OAuth-only auth warnings in custom endpoint mode.
Ensure quick spec validates and auto-fixes generated plan output, pass phase context into agent profile resolution, and update quick-spec prompt/contracts and regression tests so simple-task planning no longer fails on legacy output shapes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Capture SDK process stderr via callback and use it when ProcessError only reports a placeholder message so planning failures include actionable diagnostics.
| import pytest | ||
| import sys | ||
| from pathlib import Path | ||
| from unittest.mock import MagicMock, patch |
Check notice
Code scanning / CodeQL
Unused import Note test
Prevent spec pipeline agent runs from failing inside Claude Code sessions by clearing CLAUDECODE in SDK subprocess env, and add regression coverage for the exported env map.
Tasks can receive QA events before state restoration catches up, which left some runs stuck in backlog. Add backlog fallback transitions and regression tests so late QA events continue the workflow.
Add regression coverage for late QA_STARTED events arriving while actor state is backlog so task progression to qa_review is enforced.
Clear CLAUDECODE for insights subprocesses so API profile sessions don't fail as nested Claude Code runs, and block updater check/install behavior in non-packaged builds while logging quit-path attribution for unexpected closures.
3 participants
Summary
Test plan
Summary by CodeRabbit
Tests
Bug Fixes
New Features