-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: close claude CLI window after successful /login #1926
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -252,6 +252,14 @@ export function AuthTerminal({ | |||||||||||||||||||
| debugLog('Setting status to success (no onboarding needed)', { terminalId }); | ||||||||||||||||||||
| setStatus('success'); | ||||||||||||||||||||
| onAuthSuccess?.(info.email); | ||||||||||||||||||||
| // Auto-close after a brief delay to show success UI | ||||||||||||||||||||
| successTimeoutRef.current = setTimeout(() => { | ||||||||||||||||||||
| if (isCreatedRef.current) { | ||||||||||||||||||||
| window.electronAPI.destroyTerminal(terminalId).catch(console.error); | ||||||||||||||||||||
| isCreatedRef.current = false; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| onClose(); | ||||||||||||||||||||
| }, 1500); | ||||||||||||||||||||
|
Comment on lines
+255
to
+262
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This auto-close logic is duplicated. A similar block exists in the For example, you could define these within the component: const SUCCESS_CLOSE_DELAY_MS = 1500;
const scheduleCloseOnSuccess = useCallback(() => {
// Auto-close after a brief delay to show success UI
successTimeoutRef.current = setTimeout(() => {
if (isCreatedRef.current) {
window.electronAPI.destroyTerminal(terminalId).catch(console.error);
isCreatedRef.current = false;
}
onClose();
}, SUCCESS_CLOSE_DELAY_MS);
}, [terminalId, onClose]);Then, you can call
Suggested change
|
||||||||||||||||||||
| } | ||||||||||||||||||||
| } else { | ||||||||||||||||||||
| debugLog('OAuth failed', { terminalId, message: info.message }); | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
Repository: AndyMik90/Aperant
Length of output: 2515
🏁 Script executed:
Repository: AndyMik90/Aperant
Length of output: 2534
🏁 Script executed:
Repository: AndyMik90/Aperant
Length of output: 1431
🏁 Script executed:
Repository: AndyMik90/Aperant
Length of output: 3318
🏁 Script executed:
Repository: AndyMik90/Aperant
Length of output: 3685
🏁 Script executed:
Repository: AndyMik90/Aperant
Length of output: 2887
Add a comment to
handleOnboardingCompleteclarifying its scope for auth terminals.The guard at line 704 (
if (!terminal.awaitingOnboardingComplete) { return; }) meanshandleOnboardingCompleteis effectively dead code for OAuth login terminals sinceawaitingOnboardingCompleteis never set to true for them. This is intentional—as the comment at lines 543-546 explains,claude /loginexits immediately after OAuth completes without showing an onboarding screen, so skipping the onboarding handler is correct.For future maintainers, consider adding a comment in
handleOnboardingComplete(around the guard) noting that it only activates whenawaitingOnboardingCompleteis true, which currently applies only to non-auth terminal flows, to clarify the scope and prevent confusion about why the function appears unreachable for auth terminals.🤖 Prompt for AI Agents