fix: resolve setup wizard flashing on auth screen#1888
fix: resolve setup wizard flashing on auth screen#1888
Conversation
The OAuthStep component had an infinite re-render loop caused by loadClaudeProfiles being defined as a plain async function without useCallback. On every render, a new function reference was created, which triggered the useEffect (which listed it as a dependency), which called setIsLoadingProfiles(true) causing another re-render, creating an endless cycle that manifested as screen flashing. Fix: wrap loadClaudeProfiles in useCallback with an empty dependency array so the function reference is stable across renders. Co-Authored-By: Claude Opus 4.6 <[email protected]>
github-actions
bot
added
area/frontend
Summary of ChangesHello @AndyMik90, 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 UI bug in the setup wizard where the Claude authentication screen experienced persistent flickering. The fix involves optimizing a function's reference stability to prevent an unintended infinite re-render cycle, ensuring a smooth user experience during the onboarding process. 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
|
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
This pull request effectively resolves an infinite re-render loop in the setup wizard's authentication screen. The root cause was correctly identified as an async function being redefined on each render and used as a useEffect dependency. Wrapping loadClaudeProfiles in useCallback with an empty dependency array is the correct solution to memoize the function and provide a stable reference, thus breaking the re-render cycle. The change is well-implemented and directly addresses the reported issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx (1)
233-242:⚠️ Potential issue | 🟠 Major
console.warnlogs a raw email address (PII) to the console.In Electron, console output is typically captured in persistent log files (e.g., via
electron-logor the default Chromium log). Logging🛡️ Proposed fix — redact or drop the PII value
const handleAuthTerminalSuccess = useCallback(async (email?: string) => { - console.warn('[OAuthStep] Auth success:', email); + console.warn('[OAuthStep] Auth success'); // omit email to avoid PII in logs // Close terminal immediatelyOr, if the email is genuinely needed for diagnostics, route it to Sentry with appropriate PII scrubbing configured, rather than the console.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx` around lines 233 - 242, The console.warn in handleAuthTerminalSuccess currently logs the raw email (PII) which may be persisted in Electron logs; remove the direct logging of the email or redact it (e.g., replace with "<redacted-email>") before logging, or instead send any needed diagnostic info to your error/telemetry system (Sentry) with PII scrubbing enabled; update the function that references handleAuthTerminalSuccess (and keep the existing setAuthTerminal, setAuthenticatingProfileId, loadClaudeProfiles calls) so no raw email value is written to console output.
🤖 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/renderer/components/onboarding/OAuthStep.tsx`:
- Around line 233-242: The console.warn in handleAuthTerminalSuccess currently
logs the raw email (PII) which may be persisted in Electron logs; remove the
direct logging of the email or redact it (e.g., replace with "<redacted-email>")
before logging, or instead send any needed diagnostic info to your
error/telemetry system (Sentry) with PII scrubbing enabled; update the function
that references handleAuthTerminalSuccess (and keep the existing
setAuthTerminal, setAuthenticatingProfileId, loadClaudeProfiles calls) so no raw
email value is written to console output.
AndyMik90
left a comment
AndyMik90
left a comment
There was a problem hiding this comment.
🤖 Auto Claude PR Review
PR #1888 reviewed: 6 findings (0 structural issues). Verdict: approve.
Findings (6 selected of 6 total)
🟡 [QLT-1] [MEDIUM] useCallback with empty deps closes over stale state/props
📁 apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx:82
The useCallback wrapping loadClaudeProfiles uses an empty dependency array [], but the function body calls setClaudeProfiles, setError, and setIsLoadingProfiles — which are stable setState references and therefore safe — but it also likely references other variables from the component scope (e.g., any values used inside the try/catch that aren't shown in the diff). With [] deps, if the function ever needs to read props or non-setState closures, it will capture stale values. While this fixes the infinite loop, the correct long-term fix would be to audit the full function body and list any non-setState dependencies, or move the async logic outside the component / into a custom hook.
Suggested fix:
Audit the full body of `loadClaudeProfiles` for any references to props or state variables. If none exist beyond setState calls, `[]` is correct. If any exist, add them to the dependency array. Consider adding an ESLint `react-hooks/exhaustive-deps` check to validate.
🔵 [QLT-2] [LOW] Generic error handling swallows error details
📁 apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx:90
The catch block (lines ~90-94, partially visible) sets an error state but it's unclear whether it logs the actual exception or preserves its type. If the catch simply does setError(someGenericMessage) without logging, debugging production issues will be difficult. This is a pre-existing issue but worth flagging since the function is being refactored.
Suggested fix:
Ensure the catch block logs the original error (e.g., `console.error('Failed to load Claude profiles:', err)`) in addition to setting user-facing error state.
🔵 [QLT-3] [LOW] Missing test coverage noted in PR checklist
📁 apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx:99
The test plan checkbox for manual verification ('Launch app fresh → Setup Wizard → auth screen should render stably without flashing') is unchecked. Given that this bug is a user-visible infinite re-render loop, it would be valuable to add a unit/integration test that asserts loadClaudeProfiles is only called once on mount (e.g., by mocking the API and asserting call count), preventing regression.
Suggested fix:
Add a test that renders <OAuthStep />, mocks the profile-loading API, and asserts it is called exactly once, not in a loop.
🟡 [DEEP-1] [MEDIUM] useCallback with empty deps captures stale closure over state setters only - safe but potentially fragile
📁 apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx:82
The useCallback with [] dependencies is safe for React state setters (which are stable), but the function body at lines 80-96 may reference other variables from the component scope that could become stale. Without seeing the full component, if loadClaudeProfiles ever needs to reference props or other state values (not just setters), the empty dependency array would cause stale closure bugs. However, based on the visible code, this appears safe for now since it only calls setters and presumably stable API functions.
Suggested fix:
Verify that the function body only references stable values (React state setters, module-level imports). If it ever needs to read props or state values, add them to the useCallback dependency array. Consider using useRef for values that need to be read without triggering re-creation.
🟡 [DEEP-2] [MEDIUM] Missing cleanup/cancellation for async effect - potential state update on unmounted component
📁 apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx:82
The loadClaudeProfiles function performs async work and then calls setIsLoadingProfiles(false) in a finally block. If the component unmounts while the async operation is in-flight (e.g., user navigates away via onBack/onSkip during loading), state updates will be attempted on an unmounted component. While React 18+ suppresses the warning, this is still a logic issue that could cause unexpected behavior - for example, if the component remounts, the stale promise resolution could interfere with a fresh load.
Suggested fix:
Add an AbortController or a cleanup flag via useEffect's cleanup function to cancel or ignore the result of the async operation when the component unmounts. For example: useEffect(() => { let cancelled = false; loadClaudeProfiles(cancelled); return () => { cancelled = true; }; }, [loadClaudeProfiles]);
🔵 [DEEP-3] [LOW] Race condition if loadClaudeProfiles is called multiple times concurrently
📁 apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx:82
The function is described as 'reusable' and is both called on mount (via useEffect) and presumably callable from other UI interactions. If invoked concurrently (e.g., rapid user interaction plus mount effect), two concurrent executions would both set setIsLoadingProfiles(true), and the first to complete would set it to false while the second is still in-flight, causing incorrect loading state. The results from the first call could also briefly flash before being overwritten by the second.
Suggested fix:
Add a guard (e.g., check `isLoadingProfiles` before proceeding, or use an AbortController to cancel previous in-flight requests) to prevent concurrent executions from creating inconsistent state.
This review was generated by Auto Claude.
Remove raw email address from console.warn output to prevent PII from being written to persistent Electron log files, addressing GDPR/CCPA risk. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
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 (1)
apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx (1)
233-242:⚠️ Potential issue | 🟡 MinorRemove the unused
handleAuthTerminalSuccess.The parameter is never referenced in the function body and can be safely removed since the
onAuthSuccessprop type inAuthTerminaldefines it as optional.♻️ Proposed fix
- const handleAuthTerminalSuccess = useCallback(async (email?: string) => { + const handleAuthTerminalSuccess = useCallback(async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/frontend/src/renderer/components/onboarding/OAuthStep.tsx` around lines 233 - 242, Remove the unused email parameter from the handleAuthTerminalSuccess callback: update the function signature in the component so it no longer accepts the unused email argument (handleAuthTerminalSuccess) and ensure any place passing it (e.g., AuthTerminal's onAuthSuccess) still works since the prop defines the parameter as optional; keep the existing body (resetting setAuthTerminal, setAuthenticatingProfileId, and awaiting loadClaudeProfiles) and update the useCallback dependency array as needed.
🤖 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/renderer/components/onboarding/OAuthStep.tsx`:
- Around line 233-242: Remove the unused email parameter from the
handleAuthTerminalSuccess callback: update the function signature in the
component so it no longer accepts the unused email argument
(handleAuthTerminalSuccess) and ensure any place passing it (e.g.,
AuthTerminal's onAuthSuccess) still works since the prop defines the parameter
as optional; keep the existing body (resetting setAuthTerminal,
setAuthenticatingProfileId, and awaiting loadClaudeProfiles) and update the
useCallback dependency array as needed.
1 participant
Summary
loadClaudeProfilesinOAuthStep.tsxwas defined as a plainasyncfunction, creating a new reference every render. Since it was auseEffectdependency, this caused an infinite re-render loop (effect fires → state update → re-render → new function ref → effect fires again)loadClaudeProfilesinuseCallbackwith[]deps to stabilize the function referenceFixes #1882
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit