fix: resolve auth race condition causing 401 errors on Electron startup #335
Conversation
Summary of ChangesHello @RayFernando1337, 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 addresses a critical race condition that caused 401 Unauthorized errors during the Electron application's startup. By implementing a robust promise-based mechanism to track and await API key initialization, all API-dependent operations, including HTTP requests and WebSocket connections, are now properly synchronized. This ensures that authentication is fully established before any network calls are made, significantly improving the stability and reliability of the application in Electron mode. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a coordination mechanism to ensure API key initialization completes before creating HttpApiClient, opening WebSocket connections, or issuing HTTP requests in Electron startup flows, preventing premature 401 responses. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Hook as Settings Hook
participant Client as HttpApiClient
participant Init as API Key Init
participant Server as Backend (HTTP/WebSocket)
rect rgba(200,230,255,0.3)
Hook->>Init: call waitForApiKeyInit()
note right of Init: ensures single init promise
Init-->>Hook: resolves when API key available
end
rect rgba(220,255,220,0.3)
Hook->>Client: instantiate HttpApiClient
Client->>Init: await waitForApiKeyInit()
Init-->>Client: resolved (or fallback)
Client->>Server: open WebSocket / send HTTP requests
Server-->>Client: responses
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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
This pull request effectively addresses a race condition during application startup by introducing a promise-based mechanism to ensure the API key is initialized before any API requests are made. The changes are well-structured and applied consistently across the HTTP client and relevant hooks. I've identified a critical issue in the implementation of the initialization logic that could lead to a persistent failure state, and a medium-severity issue regarding unhandled promise rejections for improved robustness. Once these are addressed, the fix should be solid.
| apiKeyInitPromise = (async () => { | ||
| apiKeyInitialized = true; | ||
|
|
||
| // Only Electron mode uses API key header auth | ||
| if (typeof window !== 'undefined' && window.electronAPI?.getApiKey) { | ||
| try { | ||
| cachedApiKey = await window.electronAPI.getApiKey(); | ||
| if (cachedApiKey) { | ||
| console.log('[HTTP Client] Using API key from Electron'); | ||
| return; | ||
| } | ||
| } catch (error) { | ||
| console.warn('[HTTP Client] Failed to get API key from Electron:', error); | ||
| } | ||
| } catch (error) { | ||
| console.warn('[HTTP Client] Failed to get API key from Electron:', error); | ||
| } | ||
| } | ||
|
|
||
| // In web mode, authentication is handled via HTTP-only cookies | ||
| console.log('[HTTP Client] Web mode - using cookie-based authentication'); | ||
| // In web mode, authentication is handled via HTTP-only cookies | ||
| console.log('[HTTP Client] Web mode - using cookie-based authentication'); | ||
| })(); |
There was a problem hiding this comment.
The apiKeyInitialized flag is set to true at the beginning of the async operation. If window.electronAPI.getApiKey() fails or returns a falsy value, apiKeyInitialized will still be true. Subsequent calls to waitForApiKeyInit() will then resolve immediately without a valid API key, leading to persistent 401 errors for all API requests. The flag should only be set after the initialization attempt is complete. Using a finally block is the most robust way to ensure it's always set upon completion, regardless of success or failure.
apiKeyInitPromise = (async () => {
try {
// Only Electron mode uses API key header auth
if (typeof window !== 'undefined' && window.electronAPI?.getApiKey) {
cachedApiKey = await window.electronAPI.getApiKey();
if (cachedApiKey) {
console.log('[HTTP Client] Using API key from Electron');
return;
}
}
// In web mode, or if Electron API key fetch fails/returns null
console.log('[HTTP Client] Web mode - using cookie-based authentication');
} catch (error) {
console.warn('[HTTP Client] Failed to get API key from Electron:', error);
} finally {
apiKeyInitialized = true;
}
})();
apps/ui/src/lib/http-api-client.ts
Outdated
| waitForApiKeyInit().then(() => { | ||
| this.connectWebSocket(); | ||
| }); |
There was a problem hiding this comment.
This promise chain is missing a .catch() handler. While the current implementation of waitForApiKeyInit() may not reject, it's a good practice for robustness and future maintainability to handle potential promise rejections. This prevents unhandledrejection errors if the underlying logic changes and helps in debugging if initialization fails unexpectedly.
waitForApiKeyInit()
.then(() => {
this.connectWebSocket();
})
.catch((error) => {
console.error(
'[HTTP Client] API key initialization failed, WebSocket connection aborted:',
error
);
});![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/ui/src/lib/http-api-client.ts (1)
56-122: Recommended: Add timeout to prevent indefinite blocking.The current implementation has no timeout mechanism for API key initialization. If the Electron bridge hangs or is unresponsive, all HTTP requests and WebSocket connections will block indefinitely.
🔎 Suggested timeout wrapper
// Add a helper with timeout const withTimeout = <T>(promise: Promise<T>, ms: number): Promise<T> => { return Promise.race([ promise, new Promise<T>((_, reject) => setTimeout(() => reject(new Error('API key initialization timeout')), ms) ), ]); }; export const waitForApiKeyInit = (): Promise<void> => { if (apiKeyInitPromise) return apiKeyInitPromise; if (apiKeyInitialized) return Promise.resolve(); // Add 5-second timeout to prevent indefinite blocking return withTimeout(initApiKey(), 5000).catch((error) => { console.error('[HTTP Client] API key initialization failed:', error); // Fall back to web mode on timeout apiKeyInitialized = true; }); };This ensures the app remains responsive even if Electron bridge initialization fails.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.ts
🧬 Code graph analysis (1)
apps/ui/src/hooks/use-settings-migration.ts (1)
apps/ui/src/lib/http-api-client.ts (1)
waitForApiKeyInit(60-65)
🔇 Additional comments (5)
apps/ui/src/hooks/use-settings-migration.ts (2)
21-21: LGTM!The import addition is clean and follows the existing import patterns.
102-104: LGTM - Race condition fix implemented correctly.The explicit await on
waitForApiKeyInit()ensures the API key is initialized before any HTTP requests are made, preventing 401 errors during startup. The comment clearly explains the rationale.apps/ui/src/lib/http-api-client.ts (3)
47-47: LGTM - Promise caching mechanism.The module-level promise variable enables coordination between multiple callers, ensuring only one initialization occurs.
319-326: LGTM - WebSocket connection deferred correctly.The constructor properly defers WebSocket connection until API key initialization completes, preventing 401 errors. The
.then()pattern is appropriate since constructors cannot be async.Note: This depends on the
waitForApiKeyInit()fix (lines 56-65) to function correctly.
488-531: LGTM - HTTP methods properly guarded.All four HTTP methods (
post,get,put,httpDelete) now await API key initialization before making requests. The implementation is consistent across all methods with clear comments.Note: Effectiveness depends on the
waitForApiKeyInit()fix (lines 56-65).
| export const initApiKey = async (): Promise<void> => { | ||
| // Return existing promise if already in progress | ||
| if (apiKeyInitPromise) return apiKeyInitPromise; | ||
|
|
||
| // Return immediately if already initialized | ||
| if (apiKeyInitialized) return; | ||
| apiKeyInitialized = true; | ||
|
|
||
| // Only Electron mode uses API key header auth | ||
| if (typeof window !== 'undefined' && window.electronAPI?.getApiKey) { | ||
| try { | ||
| cachedApiKey = await window.electronAPI.getApiKey(); | ||
| if (cachedApiKey) { | ||
| console.log('[HTTP Client] Using API key from Electron'); | ||
| return; | ||
| // Create and store the promise so concurrent calls wait for the same initialization | ||
| apiKeyInitPromise = (async () => { | ||
| apiKeyInitialized = true; | ||
|
|
||
| // Only Electron mode uses API key header auth | ||
| if (typeof window !== 'undefined' && window.electronAPI?.getApiKey) { | ||
| try { | ||
| cachedApiKey = await window.electronAPI.getApiKey(); | ||
| if (cachedApiKey) { | ||
| console.log('[HTTP Client] Using API key from Electron'); | ||
| return; | ||
| } | ||
| } catch (error) { | ||
| console.warn('[HTTP Client] Failed to get API key from Electron:', error); | ||
| } | ||
| } catch (error) { | ||
| console.warn('[HTTP Client] Failed to get API key from Electron:', error); | ||
| } | ||
| } | ||
|
|
||
| // In web mode, authentication is handled via HTTP-only cookies | ||
| console.log('[HTTP Client] Web mode - using cookie-based authentication'); | ||
| // In web mode, authentication is handled via HTTP-only cookies | ||
| console.log('[HTTP Client] Web mode - using cookie-based authentication'); | ||
| })(); | ||
|
|
||
| return apiKeyInitPromise; | ||
| }; |
There was a problem hiding this comment.
Major: Premature flag setting breaks coordination and prevents retry.
Setting apiKeyInitialized = true on line 102 (before the async API key fetch) creates multiple issues:
- Enables the race condition flagged in lines 56-65 by setting the flag before work completes
- Prevents retry: If Electron bridge is slow or fails temporarily,
apiKeyInitializedblocks retry attempts - Silent degradation: If
getApiKey()fails (line 107-113), the catch logs a warning but initialization "succeeds," causing all subsequent HTTP requests to fail with 401s
🔎 Recommended fix: Set flag after async work completes
Move the flag assignment outside the IIFE, after the promise resolves:
export const initApiKey = async (): Promise<void> => {
// Return existing promise if already in progress
if (apiKeyInitPromise) return apiKeyInitPromise;
// Return immediately if already initialized
if (apiKeyInitialized) return;
// Create and store the promise so concurrent calls wait for the same initialization
apiKeyInitPromise = (async () => {
- apiKeyInitialized = true;
// Only Electron mode uses API key header auth
if (typeof window !== 'undefined' && window.electronAPI?.getApiKey) {
try {
cachedApiKey = await window.electronAPI.getApiKey();
if (cachedApiKey) {
console.log('[HTTP Client] Using API key from Electron');
return;
}
} catch (error) {
console.warn('[HTTP Client] Failed to get API key from Electron:', error);
}
}
// In web mode, authentication is handled via HTTP-only cookies
console.log('[HTTP Client] Web mode - using cookie-based authentication');
})();
+ await apiKeyInitPromise;
+ apiKeyInitialized = true;
return apiKeyInitPromise;
};This ensures the flag is only set after initialization truly completes, enabling proper coordination and allowing retry on failure.
📝 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.
| export const initApiKey = async (): Promise<void> => { | |
| // Return existing promise if already in progress | |
| if (apiKeyInitPromise) return apiKeyInitPromise; | |
| // Return immediately if already initialized | |
| if (apiKeyInitialized) return; | |
| apiKeyInitialized = true; | |
| // Only Electron mode uses API key header auth | |
| if (typeof window !== 'undefined' && window.electronAPI?.getApiKey) { | |
| try { | |
| cachedApiKey = await window.electronAPI.getApiKey(); | |
| if (cachedApiKey) { | |
| console.log('[HTTP Client] Using API key from Electron'); | |
| return; | |
| // Create and store the promise so concurrent calls wait for the same initialization | |
| apiKeyInitPromise = (async () => { | |
| apiKeyInitialized = true; | |
| // Only Electron mode uses API key header auth | |
| if (typeof window !== 'undefined' && window.electronAPI?.getApiKey) { | |
| try { | |
| cachedApiKey = await window.electronAPI.getApiKey(); | |
| if (cachedApiKey) { | |
| console.log('[HTTP Client] Using API key from Electron'); | |
| return; | |
| } | |
| } catch (error) { | |
| console.warn('[HTTP Client] Failed to get API key from Electron:', error); | |
| } | |
| } catch (error) { | |
| console.warn('[HTTP Client] Failed to get API key from Electron:', error); | |
| } | |
| } | |
| // In web mode, authentication is handled via HTTP-only cookies | |
| console.log('[HTTP Client] Web mode - using cookie-based authentication'); | |
| // In web mode, authentication is handled via HTTP-only cookies | |
| console.log('[HTTP Client] Web mode - using cookie-based authentication'); | |
| })(); | |
| return apiKeyInitPromise; | |
| }; | |
| export const initApiKey = async (): Promise<void> => { | |
| // Return existing promise if already in progress | |
| if (apiKeyInitPromise) return apiKeyInitPromise; | |
| // Return immediately if already initialized | |
| if (apiKeyInitialized) return; | |
| // Create and store the promise so concurrent calls wait for the same initialization | |
| apiKeyInitPromise = (async () => { | |
| // Only Electron mode uses API key header auth | |
| if (typeof window !== 'undefined' && window.electronAPI?.getApiKey) { | |
| try { | |
| cachedApiKey = await window.electronAPI.getApiKey(); | |
| if (cachedApiKey) { | |
| console.log('[HTTP Client] Using API key from Electron'); | |
| return; | |
| } | |
| } catch (error) { | |
| console.warn('[HTTP Client] Failed to get API key from Electron:', error); | |
| } | |
| } | |
| // In web mode, authentication is handled via HTTP-only cookies | |
| console.log('[HTTP Client] Web mode - using cookie-based authentication'); | |
| })(); | |
| await apiKeyInitPromise; | |
| apiKeyInitialized = true; | |
| return apiKeyInitPromise; | |
| }; |
🤖 Prompt for AI Agents
In apps/ui/src/lib/http-api-client.ts around lines 93 to 122, the code sets
apiKeyInitialized = true before awaiting the async API key fetch, which causes
premature success signaling, blocks retries on transient failures, and enables
the race condition; move the apiKeyInitialized assignment so it only runs after
the async IIFE resolves successfully (i.e., after cachedApiKey is obtained or
web-mode path completes), and on failure ensure apiKeyInitPromise is cleared (or
apiKeyInitialized remains false) so subsequent calls can retry; keep the pattern
of storing apiKeyInitPromise for concurrent callers but only mark initialization
complete after successful completion and clear/reset the promise on errors.
API requests were being made before initApiKey() completed, causing 401 Unauthorized errors on app startup in Electron mode. Changes: - Add waitForApiKeyInit() to track and await API key initialization - Make HTTP methods (get/post/put/delete) wait for auth before requests - Defer WebSocket connection until API key is ready - Add explicit auth wait in useSettingsMigration hook Fixes race condition introduced in PR AutoMaker-Org#321
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/ui/src/lib/http-api-client.ts (1)
56-65: Reorder checks for robustness: test promise existence before flag.While the current implementation works because
apiKeyInitializedis set only after the promise completes (line 120), the check order is conceptually backwards. Testing the flag first relies on implementation details ofinitApiKey()and creates fragile coupling.Checking
apiKeyInitPromisefirst better expresses the intent: "in-progress → completed → not started".🔎 Recommended: Reorder checks to test promise existence first
export const waitForApiKeyInit = (): Promise<void> => { - if (apiKeyInitialized) return Promise.resolve(); if (apiKeyInitPromise) return apiKeyInitPromise; + if (apiKeyInitialized) return Promise.resolve(); // If not started yet, start it now return initApiKey(); };This ordering is more intuitive and prevents future modifications to
initApiKey()from accidentally introducing race conditions.Based on learnings from past review comments.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/ui/src/hooks/use-settings-migration.tsapps/ui/src/lib/http-api-client.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/ui/src/hooks/use-settings-migration.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Always import from shared packages (@automaker/*), never from old relative paths
Files:
apps/ui/src/lib/http-api-client.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
resolveModelString()from @automaker/model-resolver to convert model aliases (haiku, sonnet, opus) to full model names
Files:
apps/ui/src/lib/http-api-client.ts
🔇 Additional comments (3)
apps/ui/src/lib/http-api-client.ts (3)
93-125: Good: Flag now set after completion. Verify no-retry behavior is intentional.The
finallyblock correctly ensuresapiKeyInitializedis set only after initialization completes, addressing the race condition from the past review comments.However, the flag is set "regardless of success or failure" (line 119). If
getApiKey()fails due to a transient error (e.g., Electron bridge not ready), initialization falls back to web mode and subsequent calls won't retry. Confirm this is the intended behavior—no retry on transient Electron failures.Based on learnings from past review comments.
322-335: LGTM! Catch handler added as recommended.The catch handler correctly addresses the past review comment. Attempting WebSocket connection even after API key initialization failure is appropriate, as cookie-based auth may still succeed in web mode.
Based on learnings from past review comments.
497-540: LGTM! HTTP methods correctly wait for API key initialization.All HTTP methods now properly await
waitForApiKeyInit()before sending requests, preventing the 401 errors on startup that this PR aims to fix. The implementation is consistent across all methods with clear explanatory comments.
2 participants
API requests were being made before initApiKey() completed, causing 401 Unauthorized errors on app startup in Electron mode.
Changes:
Fixes race condition introduced in PR #321
Fix Report: Auth Race Condition (401 Errors on Startup)
Summary
Fixed the race condition where API requests were made before
initApiKey()completed, causing 401 errors on Electron app startup.Root Cause
In PR #321 (
protect-api-with-api-key), the authentication system was hardened to require API keys for all requests. However,initApiKey()was called asynchronously in auseEffecthook, while other hooks (useSettingsMigration,useRunningAgents) immediately createdHttpApiClientinstances and made API requests before the API key was fetched from Electron's main process.Timeline of the bug:
useSettingsMigrationand other hooks callgetHttpApiClient()HttpApiClientconstructor callsconnectWebSocket()which needs authgetApiKey()returningnullinitApiKey()in__root.tsxuseEffect finally completes[HTTP Client] Using API key from Electronappears in logs - too late!Changes Made
1.
apps/ui/src/lib/http-api-client.tsAdded
apiKeyInitPromiseto track initialization:Added
waitForApiKeyInit()helper function:Modified
initApiKey()to return a trackable promise:Modified
HttpApiClientconstructor to wait for auth:Modified HTTP methods (
get,post,put,httpDelete) to wait for auth:2.
apps/ui/src/hooks/use-settings-migration.tsAdded wait for auth before API calls:
Testing
After the fix:
[HTTP Client] Using API key from Electronappears BEFORE any API requestsFiles Changed
apps/ui/src/lib/http-api-client.ts- Core fix for auth initializationapps/ui/src/hooks/use-settings-migration.ts- Added explicit wait for authNotes
waitForApiKeyInit()function returns immediately if already initialized, so there's no performance penalty after first loadRelated
protect-api-with-api-key)d68de99,469ee5f,d66259bSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.