Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthrough📝 WalkthroughRenames the browser-based login provider to a generic login provider, replaces the callback-server device flow with an interactive console/browser-driven flow, updates tests to match the new flow, and removes the integration test that exercised the old browser callback flow. Token storage behavior remains exercised by tests. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/auth/login.go`:
- Around line 49-61: The code ignores errors from reader.ReadString and
browser.OpenURL; update the interactive flow in the function that constructs
reader and uses authURL (the reader variable, browser.OpenURL call, and
subsequent reader.ReadString before output.EmitLog) to check and handle errors:
capture and handle the first ReadString error (treat io.EOF as empty response
and return or log a user-facing message for non-interactive shells), check the
error returned by browser.OpenURL and emit a warning via output.EmitLog (using
l.sink) if it fails, and also handle the second ReadString error before polling
for auth status so the function fails fast with a clear log rather than silently
continuing.
🧹 Nitpick comments (1)
test/integration/login_test.go (1)
99-108: Consider removing fixed sleeps; stdin buffering already handles ordering.Fixed sleeps add flakiness and latency. Writing to stdin immediately should still work and speeds the test.
♻️ Suggested cleanup
- // Wait for instructions - time.Sleep(100 * time.Millisecond) @@ - // Wait a bit for browser to open - time.Sleep(100 * time.Millisecond) @@ - // Wait for instructions to be printed - time.Sleep(100 * time.Millisecond) @@ - // Wait a bit for browser to open - time.Sleep(100 * time.Millisecond)Also applies to: 167-176
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/integration/login_test.go (1)
116-121: Remove duplicate assertions.Lines 119-120 are redundant:
"Auth request confirmed"is a substring of"Auth request confirmed, exchanging for token"(already asserted on line 117)"Fetching license token"is asserted twice (lines 118 and 120)♻️ Proposed fix to remove duplicates
// Should complete authentication successfully assert.Contains(t, output, "Checking if auth request is confirmed") assert.Contains(t, output, "Auth request confirmed, exchanging for token") assert.Contains(t, output, "Fetching license token") - assert.Contains(t, output, "Auth request confirmed") - assert.Contains(t, output, "Fetching license token") assert.Contains(t, output, "Login successful")
carole-lavillonniere
force-pushed
the
carole/drg-530
branch
from
19c5650 to
c80da5d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/integration/login_test.go (1)
67-130: Trim self-explanatory comments in the new interaction steps.The new inline comments mostly restate the code; consider removing them or replacing with rationale that isn’t obvious from the code itself.
🧹 Suggested cleanup
- // Simulate answering "Y" to open browser _, err = stdinPipe.Write([]byte("Y\n")) require.NoError(t, err) - // Simulate pressing ENTER after completing auth in browser _, err = stdinPipe.Write([]byte("\n")) require.NoError(t, err) select { case out := <-outputCh: output := string(out) - // Should show instructions assert.Contains(t, output, "Verification code:") assert.Contains(t, output, "TEST123") assert.Contains(t, output, "Open browser now? [Y/n]") assert.Contains(t, output, "Waiting for authentication (Press [ENTER] when complete)") - // Should complete authentication successfully assert.Contains(t, output, "Checking if auth request is confirmed") assert.Contains(t, output, "Auth request confirmed, exchanging for token") assert.Contains(t, output, "Fetching license token") assert.Contains(t, output, "Login successful")- // Simulate answering "Y" to open browser _, err = stdinPipe.Write([]byte("Y\n")) require.NoError(t, err) - // Simulate pressing ENTER after "completing" auth in browser _, err = stdinPipe.Write([]byte("\n")) require.NoError(t, err) select { case out := <-outputCh: output := string(out) assert.Contains(t, output, "Verification code:") assert.Contains(t, output, "Open browser now? [Y/n]") assert.Contains(t, output, "Waiting for authentication (Press [ENTER] when complete)") - // Should attempt authentication but fail because request not confirmed assert.Contains(t, output, "Checking if auth request is confirmed") assert.Contains(t, output, "auth request not confirmed")As per coding guidelines, "Avoid adding comments for self-explanatory code; only comment when the 'why' isn't obvious from the code itself".
Also applies to: 132-185
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/login_test.go` around lines 67 - 130, In TestLoginSuccess, remove or shorten the self-explanatory inline comments that merely restate the code (e.g., comments around licenseToken/env check, mockServer creation, stdin handling, output assertions) and replace any that remain with brief rationale explaining non-obvious intent; focus on keeping clarifying comments for why we manipulate stdinPipe, the purpose of envWithout("LOCALSTACK_AUTH_TOKEN")/createMockAPIServer, or timing choices for context.WithTimeout, but delete comments that repeat what functions like cleanup(), createMockAPIServer, exec.CommandContext, stdinPipe.Write, and the assert.Contains checks already express.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/integration/login_test.go`:
- Around line 67-130: In TestLoginSuccess, remove or shorten the
self-explanatory inline comments that merely restate the code (e.g., comments
around licenseToken/env check, mockServer creation, stdin handling, output
assertions) and replace any that remain with brief rationale explaining
non-obvious intent; focus on keeping clarifying comments for why we manipulate
stdinPipe, the purpose of
envWithout("LOCALSTACK_AUTH_TOKEN")/createMockAPIServer, or timing choices for
context.WithTimeout, but delete comments that repeat what functions like
cleanup(), createMockAPIServer, exec.CommandContext, stdinPipe.Write, and the
assert.Contains checks already express.
1 participant
Reasons for removing browser flow:
Output (still no TUI):