fix(profiles): use unified account selection in terminal rate limit handler#1848
fix(profiles): use unified account selection in terminal rate limit handler#1848VDT-91 wants to merge 5 commits intoAndyMik90:developfrom
Conversation
…andler Replaces getBestAvailableProfile (OAuth-only) with getBestAvailableUnifiedAccount (OAuth + API) so terminals can swap to API profiles like GLM when OAuth is rate-limited. - Add ensureBestProfileActive() helper for proactive swap before invoking Claude (awaited in async path, fire-and-forget in sync) - Rewrite handleRateLimit() to use unified account selection - API profile swaps use setActiveAPIProfile(), OAuth uses existing switchProfileCallback - Add suggestedAccountType to RateLimitEvent type - Add 6 new tests for unified swap behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…-scorer Adds 8 tests for getBestAvailableUnifiedAccount() covering: - API profile selection when all OAuth profiles are rate-limited - API profile selection when OAuth is at 100% capacity - Priority order respect for manual user swaps - Exclusion of specified accounts - Null return when no profiles exist Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When a profileId is explicitly provided (e.g. via Rate Limit Modal or manual switch), skip ensureBestProfileActive() to avoid silently overriding the user's choice with an auto-selected "better" profile. Adds test verifying getBestAvailableUnifiedAccount is not called when profileId is explicitly provided to invokeClaudeAsync. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rengthen tests - Extract sendRateLimitIpc() to eliminate duplicated IPC payload construction - Extract switchToUnifiedAccount() to deduplicate API-vs-OAuth dispatch logic - Fix ID format mismatch: use toOAuthUnifiedId() so exclude filter works correctly - Add oauthToken to test factory so OAuth profiles are properly authenticated - Remove inconsistent vi.doMock in first handleRateLimit test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @VDT-91, 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 significantly enhances the terminal's Claude integration by introducing a robust unified account selection mechanism. It addresses the problem of users being unable to automatically switch to available API profiles when their OAuth profiles are rate-limited, leading to a smoother and more resilient experience. The changes include both reactive switching upon rate limit detection and proactive optimization before invocation, ensuring that users always utilize the best available credentials without manual intervention, unless explicitly overridden. 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
|
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughIntegrates unified-account selection into rate-limit handling and Claude invocation flows, adds proactive profile-swap helpers, extends rate-limit IPC payload with account type, and introduces tests covering unified account selection and rate-limit auto-switch behavior. Changes
Sequence DiagramssequenceDiagram
participant Client
participant Handler as IntegrationHandler
participant Selector as AccountSelector
participant ProfileMgr as ProfileManager
participant IPC
Client->>Handler: invokeClaude() or handleRateLimit()
alt No explicit profileId provided
Handler->>Handler: ensureBestProfileActive()
Handler->>Selector: getBestAvailableUnifiedAccount(unifiedId)
Selector-->>Handler: BestAccount (API or OAuth)
alt Account is API-type
Handler->>ProfileMgr: setActiveAPIProfile(id)
ProfileMgr-->>Handler: persisted
else Account is OAuth-type
Handler->>Handler: switchProfileCallback(id)
Handler-->>Handler: persisted
end
end
Handler->>IPC: sendRateLimitIpc({suggestedProfileId, suggestedAccountType, autoSwitchEnabled})
IPC-->>Client: RateLimitEvent
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related Issues
Possibly Related PRs
Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![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 introduces unified account selection for rate limit handling and proactive profile swapping in the terminal. This is a significant improvement, allowing fallback to API profiles when OAuth profiles are rate-limited. The changes are well-tested with new unit tests covering the unified account selection logic and proactive swap behavior. The code is generally clean and follows good practices, with clear separation of concerns into helper functions. I've identified a few areas for minor improvement related to error handling and logging consistency.
| // Mock setActiveAPIProfile for handleRateLimit API profile auto-switch | ||
| const mockSetActiveAPIProfile = vi.fn().mockResolvedValue({}); | ||
| vi.mock('../../services/profile/profile-manager', () => ({ | ||
| setActiveAPIProfile: mockSetActiveAPIProfile, | ||
| })); |
There was a problem hiding this comment.
It's good practice to group related vi.mock calls. Consider moving this mock into the profile-manager mock block if setActiveAPIProfile is part of that module, or create a separate mock file for profile-manager if it's a larger module with many exports.
| } catch (err) { | ||
| debugError('[ClaudeIntegration] Proactive swap check failed (continuing with current profile):', err); | ||
| } |
There was a problem hiding this comment.
The comment /* handled internally */ is a bit vague. While the error is caught, it's still good to know if ensureBestProfileActive failed. Consider logging the error here as well, perhaps with a debugError to provide more context during debugging, even if it's not critical enough to halt execution.
if (!profileId) {
ensureBestProfileActive().catch(err => debugError('[ClaudeIntegration] Proactive swap failed in invokeClaude (sync):', err));
}| console.warn('[ClaudeIntegration] API profile auto-switch completed:', bestAccount.name); | ||
| }).catch(err => { | ||
| console.error('[ClaudeIntegration] API profile auto-switch failed:', err); | ||
| }); |
There was a problem hiding this comment.
The error message for API profile auto-switch failure is generic. It would be more helpful to include bestAccount.name and bestAccount.id in the error log to quickly identify which API profile failed to activate.
}).catch(err => {
console.error('[ClaudeIntegration] API profile auto-switch failed for', bestAccount.name, '(', bestAccount.id, '):', err);
});| }).catch(err => { | ||
| console.error('[ClaudeIntegration] OAuth profile auto-switch failed:', err); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Similar to the API profile, it would be beneficial to include bestAccount.name and bestAccount.id in the error log for OAuth profile auto-switch failures. This provides more specific debugging information.
}).catch(err => {
console.error('[ClaudeIntegration] OAuth profile auto-switch failed for', bestAccount.name, '(', bestAccount.id, '):', err);
});| // Proactive swap only when no explicit profile was requested (i.e. not a manual switch). | ||
| // When profileId is set, the user explicitly chose a profile — respect that choice. | ||
| if (!profileId) { | ||
| ensureBestProfileActive().catch(() => {/* handled internally */}); |
There was a problem hiding this comment.
The comment /* handled internally */ is a bit vague. While the error is caught, it's still good to know if ensureBestProfileActive failed. Consider logging the error here as well, perhaps with a debugError to provide more context during debugging, even if it's not critical enough to halt execution.
if (!profileId) {
ensureBestProfileActive().catch(err => debugError('[ClaudeIntegration] Proactive swap failed in invokeClaude (async):', err));
}
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@apps/frontend/src/main/claude-profile/__tests__/unified-account-selection.test.ts`:
- Around line 12-33: The test factories createOAuthProfile and createAPIProfile
omit required timestamp fields and rely on `as` casts, so update these factories
to include sensible defaults for the missing fields (e.g., set
ClaudeProfile.createdAt to a new Date() and APIProfile.createdAt/updatedAt to
Date.now() or fixed numbers) while preserving the ability to override via the
overrides param; ensure the added fields match the types in ClaudeProfile and
APIProfile so getBestAvailableUnifiedAccount won't encounter undefined
timestamps at runtime.
- Around line 77-95: The test for getBestAvailableUnifiedAccount is too
weak—after calling getBestAvailableUnifiedAccount with healthyOAuth (id
'oauth-1') and glmProfile, add assertions that the chosen account is the OAuth
profile (e.g., assert result!.type is the OAuth type and/or result!.id ===
'oauth-1') so the test fails if scoring ever prefers the API profile; update the
test in unified-account-selection.test.ts to explicitly check the selected
account's type/identifier in addition to isAvailable.
In `@apps/frontend/src/main/terminal/claude-integration-handler.ts`:
- Around line 408-429: The bestAccount parameter in sendRateLimitIpc is declared
with type: string but is assigned to RateLimitEvent.suggestedAccountType which
expects 'oauth' | 'api', so tighten the parameter type to match RateLimitEvent
(e.g., change bestAccount?: { id: string; name: string; type: 'oauth' | 'api' }
| null or reference the RateLimitEvent type for suggestedAccountType) and
remove/avoid relying on the broad "as RateLimitEvent" mask; update any callers
to pass the narrowed union type if needed so the type system enforces only
'oauth'|'api' values for bestAccount.type instead of an arbitrary string.
- Around line 437-447: The function switchToUnifiedAccount currently accepts
account: { id: string; type: string } and treats any non-'api' value as OAuth;
change the account type to a discriminated union (e.g., { id: string; type:
'api' } | { id: string; type: 'oauth' }) or an enum so callers are constrained,
then replace the current if/else with explicit branches for 'api' (call
setActiveAPIProfile(account.id)) and 'oauth' (call
profileManager.setActiveProfile(account.id)), and add an exhaustive default that
throws an error for unknown types so unexpected values cannot silently be
treated as OAuth; update the signature of switchToUnifiedAccount and any callers
to match the tighter type.
- Around line 1179-1183: The fire-and-forget call to ensureBestProfileActive()
in the synchronous invokeClaude path causes a race because
profileManager.getActiveProfile() is read before the async swap can complete;
remove the proactive ensureBestProfileActive().catch(...) invocation from the
sync path (leave the awaited call in the async path that already uses await
ensureBestProfileActive()), and add a brief comment explaining that proactive
swaps are handled only in the async/await path so the sync path will not mutate
active profile mid-invocation; do not change invokeClaude's return type.
- Around line 515-545: Replace the duplicated inline API-switch logic in the API
branch (the dynamic import and setActiveAPIProfile calls inside the
bestAccount.type === 'api' block) with a call to the existing helper
switchToUnifiedAccount to reuse its encapsulated import/setActiveAPIProfile
behavior; call switchToUnifiedAccount(bestAccount) (or bestAccount.id if the
helper expects an id), chain the same then() and catch() handlers to preserve
the success/error console messages, and remove the now-redundant import →
setActiveAPIProfile block.
| function createOAuthProfile(overrides: Partial<ClaudeProfile> = {}): ClaudeProfile { | ||
| return { | ||
| id: 'profile-1', | ||
| name: 'Account 1', | ||
| isDefault: false, | ||
| configDir: '/tmp/config', | ||
| oauthToken: 'fake-token-for-testing', | ||
| usage: { weeklyUsagePercent: 50, sessionUsagePercent: 50 }, | ||
| rateLimitEvents: [], | ||
| ...overrides, | ||
| } as ClaudeProfile; | ||
| } | ||
|
|
||
| function createAPIProfile(overrides: Partial<APIProfile> = {}): APIProfile { | ||
| return { | ||
| id: 'glm-1', | ||
| name: 'GLM API', | ||
| baseUrl: 'https://api.z.ai/api/anthropic', | ||
| apiKey: 'sk-glm-key', | ||
| ...overrides, | ||
| } as APIProfile; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test factories omit required fields, relying on as casts to bypass type checking.
createOAuthProfile omits createdAt (required Date on ClaudeProfile) and createAPIProfile omits createdAt/updatedAt (required number on APIProfile). The as casts suppress compiler errors but if getBestAvailableUnifiedAccount ever accesses those fields, tests would pass at compile time yet produce undefined at runtime, masking real bugs.
Consider adding sensible defaults:
♻️ Proposed fix
function createOAuthProfile(overrides: Partial<ClaudeProfile> = {}): ClaudeProfile {
return {
id: 'profile-1',
name: 'Account 1',
isDefault: false,
configDir: '/tmp/config',
oauthToken: 'fake-token-for-testing',
usage: { weeklyUsagePercent: 50, sessionUsagePercent: 50 },
rateLimitEvents: [],
+ createdAt: new Date(),
...overrides,
} as ClaudeProfile;
}
function createAPIProfile(overrides: Partial<APIProfile> = {}): APIProfile {
return {
id: 'glm-1',
name: 'GLM API',
baseUrl: 'https://api.z.ai/api/anthropic',
apiKey: 'sk-glm-key',
+ createdAt: Date.now(),
+ updatedAt: Date.now(),
...overrides,
} as APIProfile;
}📝 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.
| function createOAuthProfile(overrides: Partial<ClaudeProfile> = {}): ClaudeProfile { | |
| return { | |
| id: 'profile-1', | |
| name: 'Account 1', | |
| isDefault: false, | |
| configDir: '/tmp/config', | |
| oauthToken: 'fake-token-for-testing', | |
| usage: { weeklyUsagePercent: 50, sessionUsagePercent: 50 }, | |
| rateLimitEvents: [], | |
| ...overrides, | |
| } as ClaudeProfile; | |
| } | |
| function createAPIProfile(overrides: Partial<APIProfile> = {}): APIProfile { | |
| return { | |
| id: 'glm-1', | |
| name: 'GLM API', | |
| baseUrl: 'https://api.z.ai/api/anthropic', | |
| apiKey: 'sk-glm-key', | |
| ...overrides, | |
| } as APIProfile; | |
| } | |
| function createOAuthProfile(overrides: Partial<ClaudeProfile> = {}): ClaudeProfile { | |
| return { | |
| id: 'profile-1', | |
| name: 'Account 1', | |
| isDefault: false, | |
| configDir: '/tmp/config', | |
| oauthToken: 'fake-token-for-testing', | |
| usage: { weeklyUsagePercent: 50, sessionUsagePercent: 50 }, | |
| rateLimitEvents: [], | |
| createdAt: new Date(), | |
| ...overrides, | |
| } as ClaudeProfile; | |
| } | |
| function createAPIProfile(overrides: Partial<APIProfile> = {}): APIProfile { | |
| return { | |
| id: 'glm-1', | |
| name: 'GLM API', | |
| baseUrl: 'https://api.z.ai/api/anthropic', | |
| apiKey: 'sk-glm-key', | |
| createdAt: Date.now(), | |
| updatedAt: Date.now(), | |
| ...overrides, | |
| } as APIProfile; | |
| } |
🤖 Prompt for AI Agents
In
`@apps/frontend/src/main/claude-profile/__tests__/unified-account-selection.test.ts`
around lines 12 - 33, The test factories createOAuthProfile and createAPIProfile
omit required timestamp fields and rely on `as` casts, so update these factories
to include sensible defaults for the missing fields (e.g., set
ClaudeProfile.createdAt to a new Date() and APIProfile.createdAt/updatedAt to
Date.now() or fixed numbers) while preserving the ability to override via the
overrides param; ensure the added fields match the types in ClaudeProfile and
APIProfile so getBestAvailableUnifiedAccount won't encounter undefined
timestamps at runtime.
| it('returns available OAuth profile over API when OAuth is not rate-limited', () => { | ||
| const healthyOAuth = createOAuthProfile({ | ||
| id: 'oauth-1', | ||
| name: 'Healthy Account', | ||
| }); | ||
|
|
||
| const glmProfile = createAPIProfile(); | ||
|
|
||
| const result = getBestAvailableUnifiedAccount( | ||
| [healthyOAuth], | ||
| [glmProfile], | ||
| defaultSettings, | ||
| { excludeAccountId: 'some-other-id' } | ||
| ); | ||
|
|
||
| expect(result).not.toBeNull(); | ||
| // Both are available; result depends on priority order | ||
| expect(result!.isAvailable).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test assertion is too weak to catch regressions.
This test only asserts isAvailable is true but doesn't assert which account type was selected. If scoring logic were to accidentally prefer API over healthy OAuth, this test would still pass. Consider asserting the expected type:
♻️ Proposed fix
expect(result).not.toBeNull();
- // Both are available; result depends on priority order
- expect(result!.isAvailable).toBe(true);
+ // Healthy OAuth should be preferred over API by default priority
+ expect(result!.isAvailable).toBe(true);
+ expect(result!.type).toBe('oauth');📝 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.
| it('returns available OAuth profile over API when OAuth is not rate-limited', () => { | |
| const healthyOAuth = createOAuthProfile({ | |
| id: 'oauth-1', | |
| name: 'Healthy Account', | |
| }); | |
| const glmProfile = createAPIProfile(); | |
| const result = getBestAvailableUnifiedAccount( | |
| [healthyOAuth], | |
| [glmProfile], | |
| defaultSettings, | |
| { excludeAccountId: 'some-other-id' } | |
| ); | |
| expect(result).not.toBeNull(); | |
| // Both are available; result depends on priority order | |
| expect(result!.isAvailable).toBe(true); | |
| }); | |
| it('returns available OAuth profile over API when OAuth is not rate-limited', () => { | |
| const healthyOAuth = createOAuthProfile({ | |
| id: 'oauth-1', | |
| name: 'Healthy Account', | |
| }); | |
| const glmProfile = createAPIProfile(); | |
| const result = getBestAvailableUnifiedAccount( | |
| [healthyOAuth], | |
| [glmProfile], | |
| defaultSettings, | |
| { excludeAccountId: 'some-other-id' } | |
| ); | |
| expect(result).not.toBeNull(); | |
| // Healthy OAuth should be preferred over API by default priority | |
| expect(result!.isAvailable).toBe(true); | |
| expect(result!.type).toBe('oauth'); | |
| }); |
🤖 Prompt for AI Agents
In
`@apps/frontend/src/main/claude-profile/__tests__/unified-account-selection.test.ts`
around lines 77 - 95, The test for getBestAvailableUnifiedAccount is too
weak—after calling getBestAvailableUnifiedAccount with healthyOAuth (id
'oauth-1') and glmProfile, add assertions that the chosen account is the OAuth
profile (e.g., assert result!.type is the OAuth type and/or result!.id ===
'oauth-1') so the test fails if scoring ever prefers the API profile; update the
test in unified-account-selection.test.ts to explicitly check the selected
account's type/identifier in addition to isAvailable.
| function sendRateLimitIpc( | ||
| getWindow: WindowGetter, | ||
| terminalId: string, | ||
| resetTime: string, | ||
| profileId: string, | ||
| autoSwitchEnabled: boolean, | ||
| bestAccount?: { id: string; name: string; type: string } | null | ||
| ): void { | ||
| const win = getWindow(); | ||
| if (win) { | ||
| win.webContents.send(IPC_CHANNELS.TERMINAL_RATE_LIMIT, { | ||
| terminalId, | ||
| resetTime, | ||
| detectedAt: new Date().toISOString(), | ||
| profileId, | ||
| suggestedProfileId: bestAccount?.id, | ||
| suggestedProfileName: bestAccount?.name, | ||
| suggestedAccountType: bestAccount?.type, | ||
| autoSwitchEnabled, | ||
| } as RateLimitEvent); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
bestAccount.type is typed as string but assigned to a 'oauth' | 'api' field.
The parameter bestAccount declares type: string, but it's assigned to RateLimitEvent.suggestedAccountType which is 'oauth' | 'api'. The as RateLimitEvent cast on line 427 silently masks this mismatch. If a caller ever passes an unexpected type string, it would flow through unchecked.
♻️ Proposed fix — tighten the parameter type
function sendRateLimitIpc(
getWindow: WindowGetter,
terminalId: string,
resetTime: string,
profileId: string,
autoSwitchEnabled: boolean,
- bestAccount?: { id: string; name: string; type: string } | null
+ bestAccount?: { id: string; name: string; type: 'oauth' | 'api' } | null
): void {📝 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.
| function sendRateLimitIpc( | |
| getWindow: WindowGetter, | |
| terminalId: string, | |
| resetTime: string, | |
| profileId: string, | |
| autoSwitchEnabled: boolean, | |
| bestAccount?: { id: string; name: string; type: string } | null | |
| ): void { | |
| const win = getWindow(); | |
| if (win) { | |
| win.webContents.send(IPC_CHANNELS.TERMINAL_RATE_LIMIT, { | |
| terminalId, | |
| resetTime, | |
| detectedAt: new Date().toISOString(), | |
| profileId, | |
| suggestedProfileId: bestAccount?.id, | |
| suggestedProfileName: bestAccount?.name, | |
| suggestedAccountType: bestAccount?.type, | |
| autoSwitchEnabled, | |
| } as RateLimitEvent); | |
| } | |
| } | |
| function sendRateLimitIpc( | |
| getWindow: WindowGetter, | |
| terminalId: string, | |
| resetTime: string, | |
| profileId: string, | |
| autoSwitchEnabled: boolean, | |
| bestAccount?: { id: string; name: string; type: 'oauth' | 'api' } | null | |
| ): void { | |
| const win = getWindow(); | |
| if (win) { | |
| win.webContents.send(IPC_CHANNELS.TERMINAL_RATE_LIMIT, { | |
| terminalId, | |
| resetTime, | |
| detectedAt: new Date().toISOString(), | |
| profileId, | |
| suggestedProfileId: bestAccount?.id, | |
| suggestedProfileName: bestAccount?.name, | |
| suggestedAccountType: bestAccount?.type, | |
| autoSwitchEnabled, | |
| } as RateLimitEvent); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/claude-integration-handler.ts` around lines
408 - 429, The bestAccount parameter in sendRateLimitIpc is declared with type:
string but is assigned to RateLimitEvent.suggestedAccountType which expects
'oauth' | 'api', so tighten the parameter type to match RateLimitEvent (e.g.,
change bestAccount?: { id: string; name: string; type: 'oauth' | 'api' } | null
or reference the RateLimitEvent type for suggestedAccountType) and remove/avoid
relying on the broad "as RateLimitEvent" mask; update any callers to pass the
narrowed union type if needed so the type system enforces only 'oauth'|'api'
values for bestAccount.type instead of an arbitrary string.
| async function switchToUnifiedAccount( | ||
| profileManager: ReturnType<typeof getClaudeProfileManager>, | ||
| account: { id: string; type: string } | ||
| ): Promise<void> { | ||
| if (account.type === 'api') { | ||
| const { setActiveAPIProfile } = await import('../services/profile/profile-manager'); | ||
| await setActiveAPIProfile(account.id); | ||
| } else { | ||
| profileManager.setActiveProfile(account.id); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Same type: string looseness — and the else branch silently treats any non-'api' value as OAuth.
If account.type is ever anything other than 'api', it falls into the else branch and calls profileManager.setActiveProfile. Tightening the type and adding an exhaustive check would make this safer.
♻️ Proposed fix
async function switchToUnifiedAccount(
profileManager: ReturnType<typeof getClaudeProfileManager>,
- account: { id: string; type: string }
+ account: { id: string; type: 'oauth' | 'api' }
): Promise<void> {
if (account.type === 'api') {
const { setActiveAPIProfile } = await import('../services/profile/profile-manager');
await setActiveAPIProfile(account.id);
} else {
profileManager.setActiveProfile(account.id);
}
}🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/claude-integration-handler.ts` around lines
437 - 447, The function switchToUnifiedAccount currently accepts account: { id:
string; type: string } and treats any non-'api' value as OAuth; change the
account type to a discriminated union (e.g., { id: string; type: 'api' } | { id:
string; type: 'oauth' }) or an enum so callers are constrained, then replace the
current if/else with explicit branches for 'api' (call
setActiveAPIProfile(account.id)) and 'oauth' (call
profileManager.setActiveProfile(account.id)), and add an exhaustive default that
throws an error for unknown types so unexpected values cannot silently be
treated as OAuth; update the signature of switchToUnifiedAccount and any callers
to match the tighter type.
| // Use unified account selection (OAuth + API) instead of OAuth-only | ||
| // Pass a properly-prefixed unified ID so the exclude filter works correctly | ||
| profileManager.getBestAvailableUnifiedAccount(toOAuthUnifiedId(currentProfileId)).then(bestAccount => { | ||
| sendRateLimitIpc(getWindow, terminal.id, resetTime, currentProfileId, autoSwitchSettings.autoSwitchOnRateLimit, bestAccount); | ||
|
|
||
| if (autoSwitchSettings.enabled && autoSwitchSettings.autoSwitchOnRateLimit && bestAccount) { | ||
| console.warn('[ClaudeIntegration] Auto-switching to account:', bestAccount.name, '(type:', bestAccount.type, ')'); | ||
|
|
||
| if (bestAccount.type === 'api') { | ||
| // API profile: persist globally via setActiveAPIProfile | ||
| import('../services/profile/profile-manager').then(({ setActiveAPIProfile }) => { | ||
| return setActiveAPIProfile(bestAccount.id); | ||
| }).then(() => { | ||
| console.warn('[ClaudeIntegration] API profile auto-switch completed:', bestAccount.name); | ||
| }).catch(err => { | ||
| console.error('[ClaudeIntegration] API profile auto-switch failed:', err); | ||
| }); | ||
| } else { | ||
| // OAuth profile: use existing callback (restarts terminal, not just setActiveProfile) | ||
| switchProfileCallback(terminal.id, bestAccount.id).then(() => { | ||
| console.warn('[ClaudeIntegration] OAuth profile auto-switch completed'); | ||
| }).catch(err => { | ||
| console.error('[ClaudeIntegration] OAuth profile auto-switch failed:', err); | ||
| }); | ||
| } | ||
| } | ||
| }).catch(err => { | ||
| // Fallback: send IPC event without suggested profile | ||
| console.error('[ClaudeIntegration] Unified account selection failed:', err); | ||
| sendRateLimitIpc(getWindow, terminal.id, resetTime, currentProfileId, autoSwitchSettings.autoSwitchOnRateLimit); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicated API-switch logic between handleRateLimit and switchToUnifiedAccount.
Lines 523–531 inline the same import → setActiveAPIProfile pattern already encapsulated in switchToUnifiedAccount (lines 437–447). The OAuth path intentionally differs (uses switchProfileCallback to restart the terminal), but the API path could delegate to the shared helper:
♻️ Reduce duplication for the API branch
if (bestAccount.type === 'api') {
- // API profile: persist globally via setActiveAPIProfile
- import('../services/profile/profile-manager').then(({ setActiveAPIProfile }) => {
- return setActiveAPIProfile(bestAccount.id);
- }).then(() => {
+ switchToUnifiedAccount(profileManager, bestAccount).then(() => {
console.warn('[ClaudeIntegration] API profile auto-switch completed:', bestAccount.name);
}).catch(err => {
console.error('[ClaudeIntegration] API profile auto-switch failed:', err);
});
} else {🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/claude-integration-handler.ts` around lines
515 - 545, Replace the duplicated inline API-switch logic in the API branch (the
dynamic import and setActiveAPIProfile calls inside the bestAccount.type ===
'api' block) with a call to the existing helper switchToUnifiedAccount to reuse
its encapsulated import/setActiveAPIProfile behavior; call
switchToUnifiedAccount(bestAccount) (or bestAccount.id if the helper expects an
id), chain the same then() and catch() handlers to preserve the success/error
console messages, and remove the now-redundant import → setActiveAPIProfile
block.
| // Proactive swap only when no explicit profile was requested (i.e. not a manual switch). | ||
| // When profileId is set, the user explicitly chose a profile — respect that choice. | ||
| if (!profileId) { | ||
| ensureBestProfileActive().catch(() => {/* handled internally */}); | ||
| } |
There was a problem hiding this comment.
Fire-and-forget ensureBestProfileActive() creates a race — proactive swap cannot take effect before invocation.
ensureBestProfileActive() is fully async (awaits getBestAvailableUnifiedAccount and switchToUnifiedAccount), but here it's launched without await. The synchronous code on lines 1188–1191 reads profileManager.getActiveProfile() immediately, before the swap can complete. The proactive swap therefore has no effect on this invocation — the old (potentially rate-limited) profile is used, and the swap silently completes in the background (affecting only the next invocation).
In contrast, the async path at line 1380 correctly awaits the swap.
Options:
- Convert
invokeClaudeto return aPromise(may require upstream changes). - Remove the fire-and-forget call from the sync path entirely and rely on the async path and reactive handling (clearer contract, no hidden background mutation).
- Document this as intentional "best-effort for next invocation" behavior (but this contradicts the PR description).
Option 2: Remove fire-and-forget to avoid silent no-op
- // Proactive swap only when no explicit profile was requested (i.e. not a manual switch).
- // When profileId is set, the user explicitly chose a profile — respect that choice.
- if (!profileId) {
- ensureBestProfileActive().catch(() => {/* handled internally */});
- }
-
const startTime = Date.now();📝 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.
| // Proactive swap only when no explicit profile was requested (i.e. not a manual switch). | |
| // When profileId is set, the user explicitly chose a profile — respect that choice. | |
| if (!profileId) { | |
| ensureBestProfileActive().catch(() => {/* handled internally */}); | |
| } | |
| const startTime = Date.now(); |
🤖 Prompt for AI Agents
In `@apps/frontend/src/main/terminal/claude-integration-handler.ts` around lines
1179 - 1183, The fire-and-forget call to ensureBestProfileActive() in the
synchronous invokeClaude path causes a race because
profileManager.getActiveProfile() is read before the async swap can complete;
remove the proactive ensureBestProfileActive().catch(...) invocation from the
sync path (leave the awaited call in the async path that already uses await
ensureBestProfileActive()), and add a brief comment explaining that proactive
swaps are handled only in the async/await path so the sync path will not mutate
active profile mid-invocation; do not change invokeClaude's return type.
Add CodeQL suppression comments for js/weak-crypto-hashing rule on two lines that hash filesystem paths for credential storage identifiers (not passwords). These are legitimate uses of SHA256 for creating deterministic identifiers from config directory paths. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| * @returns The 8-character hex hash suffix | ||
| */ | ||
| export function calculateConfigDirHash(configDir: string): string { | ||
| // CodeQL[js/weak-crypto-hashing] suppress False positive: hashing filesystem path for identifier, not password |
Check failure
Code scanning / CodeQL
Use of password hash with insufficient computational effort High
1 participant
Problem
When all OAuth profiles are rate-limited, terminals running Claude Code cannot automatically fall back to API profiles (GLM, etc.). Users hit a hard stop even though they have valid API credentials configured.
Solution
Add unified account selection to the terminal's rate limit handler and proactive swap logic:
handleRateLimit()now callsgetBestAvailableUnifiedAccount()which considers BOTH OAuth and API profiles. If an API profile is available, it's suggested (and auto-switched if enabled).ensureBestProfileActive()checks if a better profile (OAuth or API) is available and swaps globally.Changes
claude-integration-handler.ts: AddedensureBestProfileActive()helper, rewrotehandleRateLimit()to use unified account selectiontypes.ts: AddedsuggestedAccountTypefield toRateLimitEventIPC payloadsendRateLimitIpc()andswitchToUnifiedAccount()helpers to eliminate duplicationTesting
[ClaudeIntegration] Auto-switching to account: GLM API (type: api)when OAuth is rate-limitedThis fix addresses the core logic for unified account selection. We believe this resolves the Windows OAuth/API rate limit swap issues, but we don't have 100% confirmation that it fixes all edge cases on Windows. The logic is platform-agnostic and uses the same cross-platform abstractions as the rest of the codebase.
Community testing on Windows would be appreciated to confirm the fix works end-to-end.
Related
This is the terminal-focused portion of the unified account selection work, building on the existing
getBestAvailableUnifiedAccount()infrastructure inprofile-scorer.tsandclaude-profile-manager.ts.Summary by CodeRabbit
New Features
Tests