-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Auto-open browser for device code verification in login #110
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: main
Are you sure you want to change the base?
Conversation
- Add confirm prompt asking user to press enter to open browser - Auto-open verification URL when user confirms - Style the verification URL as a link using theme.colors.links()
Replace confirm dialog (Open/Skip) with simple "Press Enter to open in browser..." prompt using readline for a cleaner UX.
🚀 Package Preview Available!Install this PR's preview build with npm: npm i @base44-preview/cli@0.0.17-pr.110.70e2c01Prefer not to change any import paths? Install using npm alias so your code still imports npm i "base44@npm:@base44-preview/cli@0.0.17-pr.110.70e2c01"Or add it to your {
"dependencies": {
"base44": "npm:@base44-preview/cli@0.0.17-pr.110.70e2c01"
}
}
Preview published to npm registry — try new features instantly! |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
src/cli/commands/auth/login.ts
Outdated
| }); | ||
|
|
||
| await new Promise<void>((resolve) => { | ||
| rl.question("Press Enter to open in browser...", () => { |
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.
we don't want to use readline in the project, we cause use clack prompts to show something like this, i would also claim that maybe we don't want the even ask the user? we already have open installed (for the dashboard command), lets just open the browser? we can also add a check before if we're is a non-tty environment and not open the browser.
@claude suggest a fix please
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.
Claude finished @kfirstri's task in 1m 17s —— View job
Summary
I've refactored the auth login flow to address your feedback:
Changes Made
- ✅ Removed
readlinedependency entirely (no longer imported or used) - ✅ Removed the "Press Enter to open in browser..." prompt
- ✅ Browser now auto-opens immediately after displaying the verification code
- ✅ Added TTY check (
process.stdout.isTTY) to only open browser in interactive terminals (non-TTY environments won't attempt to open) - ✅ Kept the styled verification URL display with cyan color
Implementation Details
The code now follows the same pattern as the dashboard command - it simply calls open() with the URL. The TTY check ensures we don't try to open a browser in CI/CD or other non-interactive environments.
Before: 18 lines with readline prompt
After: 5 lines with auto-open
The UX is now cleaner and more consistent with the rest of the codebase.
- Remove readline dependency from auth login flow - Auto-open verification URL in browser without prompting - Add TTY check to only open browser in interactive terminals - Simplify UX by removing unnecessary prompt Co-authored-by: Kfir Stri <kfirstri@users.noreply.github.com>
Summary
theme.colors.links()for visual consistencyTest plan
base44 auth logincommand