feat(auth): auto-open browser for device flow verification URL#243
Open
meijing0114 wants to merge 2 commits intolarksuite:mainfrom
Open
feat(auth): auto-open browser for device flow verification URL#243meijing0114 wants to merge 2 commits intolarksuite:mainfrom
meijing0114 wants to merge 2 commits intolarksuite:mainfrom
Conversation
After printing the verification URL, attempt to open the user's default browser via platform command (open/xdg-open/start). On success, print a hint; on failure, silently continue — the plain-text URL is always shown. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
github-actions
bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a cross-platform helper to open URLs in the system browser and invokes it from interactive verification flows, printing a localized "BrowserOpened" message only when the browser-launch attempt reports success. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI"
participant CmdUtil as "cmdutil.OpenBrowser"
participant OS as "OS / Shell"
participant Auth as "Auth Server (polling)"
CLI->>CLI: print verification URL (and QR)
CLI->>CmdUtil: OpenBrowser(url)
CmdUtil->>OS: start platform command (open/xdg-open/start)
OS-->>CmdUtil: process started
CmdUtil-->>CLI: returns true
CLI->>CLI: print localized BrowserOpened
CLI->>Auth: begin polling for token
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds automatic browser-opening to the device-flow verification URL in both Confidence Score: 5/5Safe to merge — all previously raised issues are resolved and no new issues found. All three prior P1 concerns (Windows start title, zombie process, --no-wait headless launch) are correctly addressed in the current diff. The change is small, well-scoped, and fails silently without affecting the core auth flow. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[auth login / config init] --> B[RequestDeviceAuthorization]
B --> C{--no-wait flag?}
C -- yes --> D[Emit JSON output\nreturn early]
C -- no --> E{--json flag?}
E -- yes --> F[Emit JSON event]
E -- no --> G[Print verification URL]
G --> H[OpenBrowser url]
H --> I{Launched OK?}
I -- yes --> J[Print BrowserOpened message]
I -- no --> K[Silent fallback\nURL already printed]
J --> L[PollDeviceToken]
K --> L
F --> L
L --> M[Login / Init complete]
Reviews (2): Last reviewed commit: "fix(auth): address PR review feedback fo..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/auth/login.go (1)
240-242: Deduplicate the browser-open print block to avoid drift.The same open-and-notify logic appears twice. Extract a small helper/closure in
authLoginRunand call it from both branches.♻️ Minimal refactor sketch
+ openAndNotify := func(url string) { + if cmdutil.OpenBrowser(url) { + fmt.Fprintf(f.IOStreams.ErrOut, "%s\n", msg.BrowserOpened) + } + } @@ - if cmdutil.OpenBrowser(authResp.VerificationUriComplete) { - fmt.Fprintf(f.IOStreams.ErrOut, "%s\n", msg.BrowserOpened) - } + openAndNotify(authResp.VerificationUriComplete) @@ - if cmdutil.OpenBrowser(authResp.VerificationUriComplete) { - fmt.Fprintf(f.IOStreams.ErrOut, "%s\n", msg.BrowserOpened) - } + openAndNotify(authResp.VerificationUriComplete)Also applies to: 263-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 240 - 242, The browser-open-and-notify logic is duplicated; inside authLoginRun extract a small helper (e.g., openAndNotify or a closure) that takes the verification URL and handles cmdutil.OpenBrowser(authResp.VerificationUriComplete) plus fmt.Fprintf(f.IOStreams.ErrOut, "%s\n", msg.BrowserOpened) so both places (the current block using authResp.VerificationUriComplete and the other branch around lines 263-265) simply call that helper; reference cmdutil.OpenBrowser, authResp.VerificationUriComplete, msg.BrowserOpened and f.IOStreams.ErrOut when implementing the helper and replace the duplicated blocks with calls to it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmdutil/browser_test.go`:
- Around line 8-12: TestOpenBrowser_NoPanic currently calls
OpenBrowser("https://example.com") with no assertions and may spawn a real
browser; refactor OpenBrowser to accept an injectable command starter (e.g., a
function type like cmdStarter or an interface) so tests can supply a fake
executor, then update TestOpenBrowser_NoPanic to inject a stubbed starter that
records the command and does not spawn a process and assert the expected command
selection/arguments for each runtime.GOOS (windows, darwin, linux) by
table-driven subtests; reference the OpenBrowser function and
TestOpenBrowser_NoPanic when making these changes and ensure the production call
site uses the real starter by default.
In `@internal/cmdutil/browser.go`:
- Around line 14-23: The Windows branch in OpenBrowser incorrectly uses
exec.Command("cmd","/c","start", url) which allows cmd.exe to parse
metacharacters (e.g., & in query strings); update the Windows case inside the
OpenBrowser function to call start with an empty title placeholder so the URL is
passed safely (use the form equivalent to exec.Command("cmd", "/c", "start", "",
url)); optionally, as a defense-in-depth step, validate the URL with
url.ParseRequestURI() before launching to avoid passing malformed input to the
command.
---
Nitpick comments:
In `@cmd/auth/login.go`:
- Around line 240-242: The browser-open-and-notify logic is duplicated; inside
authLoginRun extract a small helper (e.g., openAndNotify or a closure) that
takes the verification URL and handles
cmdutil.OpenBrowser(authResp.VerificationUriComplete) plus
fmt.Fprintf(f.IOStreams.ErrOut, "%s\n", msg.BrowserOpened) so both places (the
current block using authResp.VerificationUriComplete and the other branch around
lines 263-265) simply call that helper; reference cmdutil.OpenBrowser,
authResp.VerificationUriComplete, msg.BrowserOpened and f.IOStreams.ErrOut when
implementing the helper and replace the duplicated blocks with calls to it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83cd1d5d-c79c-46d3-a7d2-d827d3bf0401
📒 Files selected for processing (6)
cmd/auth/login.gocmd/auth/login_messages.gocmd/config/init_interactive.gocmd/config/init_messages.gointernal/cmdutil/browser.gointernal/cmdutil/browser_test.go
Comment on lines
+8
to
+12
| func TestOpenBrowser_NoPanic(t *testing.T) { | ||
| // Verify OpenBrowser doesn't panic on any URL. | ||
| // On CI/headless this may return true (process spawned) or false — both are fine. | ||
| _ = OpenBrowser("https://example.com") | ||
| } |
There was a problem hiding this comment.
This test is non-hermetic and effectively assertion-free.
Line 11 may launch a real browser, and the test passes even if command selection regresses. Please make OpenBrowser testable via an injected command starter and assert expected behavior per runtime.GOOS.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmdutil/browser_test.go` around lines 8 - 12,
TestOpenBrowser_NoPanic currently calls OpenBrowser("https://example.com") with
no assertions and may spawn a real browser; refactor OpenBrowser to accept an
injectable command starter (e.g., a function type like cmdStarter or an
interface) so tests can supply a fake executor, then update
TestOpenBrowser_NoPanic to inject a stubbed starter that records the command and
does not spawn a process and assert the expected command selection/arguments for
each runtime.GOOS (windows, darwin, linux) by table-driven subtests; reference
the OpenBrowser function and TestOpenBrowser_NoPanic when making these changes
and ensure the production call site uses the real starter by default.
- Fix Windows `cmd /c start` URL handling: add empty title arg to prevent URL being treated as window title - Remove auto-open from --no-wait path (agent/script context) - Add `go cmd.Wait()` to reap child process Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
auth loginandconfig init --newflowsChanges
internal/cmdutil/browser.go—OpenBrowser(url)using platform commands (open/xdg-open/start)internal/cmdutil/browser_test.go— smoke testcmd/auth/login.go— callOpenBrowser()after printing URL (both--no-waitand normal paths)cmd/auth/login_messages.go— addBrowserOpenedmessage (zh/en)cmd/config/init_interactive.go— callOpenBrowser()after printing app registration URLcmd/config/init_messages.go— addBrowserOpenedmessage (zh/en)Test plan
./lark-cli auth login --domain calendaropens browser on macOSgo test ./cmd/auth/... ./internal/cmdutil/... ./cmd/config/...Spec:
docs/spec-auto-open-browser.md🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests