-
Notifications
You must be signed in to change notification settings - Fork 1.3k
refactor(auth): save authType after successfully authenticated #1036
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
Conversation
📋 Review SummaryThis PR successfully refactors the authentication flow to fix the stuck auth dialog issue when users fail to complete the OAuth process. The core change is ensuring that 🔍 General Feedback
🎯 Specific Feedback🔴 CriticalNo critical issues identified in this review. 🟡 HighNo high priority issues identified in this review. 🟢 Medium
🔵 Low
✅ Highlights
|
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
| model: modelSettings?.name || process.env['OPENAI_MODEL'] || '', | ||
| }; | ||
| }; | ||
|
|
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.
This will cause some confusion:
qwen-code/packages/cli/src/config/config.ts
Line 758 in f0bbeac
| generationConfig: { |
The default value is obtained from settings, but the actual value that takes effect is the environment variable.
We should align the precedence of the values.
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.
Env variables are not editable, while the editable values won't take effect, if both env variables and auth from settings exist.
TLDR
Fixes the stuck auth dialog issue when users fail to complete the OAuth process. The CLI now only saves
authTypeafter successful authentication, ensuring that when authentication is terminated before completion and the CLI is restarted, it will prompt the auth dialog again instead of getting stuck.Dive Deeper
Previously, the
authTypemight have been saved before authentication completed successfully. When users failed to complete the OAuth process (e.g., closing the browser, network issues, Ctrl+C), the CLI would get stuck because it thought authentication was already in progress.The fix ensures that
authTypeis only saved to settings after successful authentication via thehandleAuthSuccesscallback. This way, if authentication is terminated before success and the CLI is restarted, it will detect that no authType is saved and prompt the auth dialog again.The code was refactored to:
handleAuthSuccessandhandleAuthFailurecallbacks for better organizationuseEffectthat triggered on settings changespendingAuthTypestate to track authentication in progressuseQwenAuthhook to manage Qwen OAuth stateAuthDialogto use Context API instead of propsOpenAIKeyPrompttoDialogManagerfor better flow controlReviewer Test Plan
security.auth.selectedTypeis saved in settings after successful authenticationTesting Matrix
Linked issues / bugs
Fixes #1029