Show login options when not signed in with ChatGPT - #2440
Conversation
|
Summary
Review
Suggestions
Overall, changes are cohesive and in the right crates; just ensure tests/snapshots are updated and consider the extra tests noted. |
bolinfest
left a comment
There was a problem hiding this comment.
Looks good overall but please make the suggested enum-related changes!
| load_auth(codex_home, true) | ||
| pub fn from_codex_home( | ||
| codex_home: &Path, | ||
| always_use_api_key_signing: bool, |
There was a problem hiding this comment.
Seeing this passed in as a raw bool value is not great. Please introduce an enum with two variants like:
enum AuthCredentialStrategy {
ApiKeyOnly,
PreferChatGpt,
}or whatever names you think are appropriate. Then update this function to take the new enum type instead of a bool.
| pub enum LoginStatus { | ||
| ChatGPT, | ||
| ApiKey { always_use_api_key_signing: bool }, | ||
| None, |
There was a problem hiding this comment.
Maybe NotAuthenticated?
|
|
||
| impl OnboardingScreen { | ||
| pub(crate) fn new(args: OnboardingScreenArgs) -> Self { | ||
| let show_login_screen = matches!(args.login_status, LoginStatus::None) |
There was a problem hiding this comment.
If you use a match does Clippy pressure you to use matches!() instead? I generally prefer match so that if a new enum variant is introduced, you are forced to deal with it.
Motivation: we have users who uses their API key although they want to use ChatGPT account. We want to give them the chance to always login with their account.
This PR displays login options when the user is not signed in with ChatGPT. Even if you have set an OpenAI API key as an environment variable, you will still be prompted to log in with ChatGPT.
We’ve also added a new config,
preferred_auth_methodwhich ischatgptby default, which ensures you are never asked to log in with ChatGPT and always defaults to using your API key.Screen.Recording.2025-08-18.at.4.17.32.PM.mov
After ChatGPT sign in:
Screen.Recording.2025-08-18.at.4.18.37.PM.mov