Add styled CLI output with spinner for logout command#37
Add styled CLI output with spinner for logout command#37
Conversation
gtsiolis
requested review from
carole-lavillonniere and
silv-io
as code owners
📝 WalkthroughWalkthroughThis change adds a spinner-based UI wrapper to the logout operation and enhances the output system to support success and note event types. The logout method now returns a boolean indicating whether a user was previously logged in, enabling differentiated messaging for successful logouts versus no-token scenarios. Changes
Sequence DiagramsequenceDiagram
participant User
participant LogoutCmd as Logout Command
participant Spinner as RunWithSpinner
participant Auth
participant Output as Output Sink
participant UI as Spinner UI
User->>LogoutCmd: Execute logout
LogoutCmd->>Spinner: RunWithSpinner(message, logout action)
Spinner->>UI: Initialize spinner
UI->>UI: Display spinning animation
Spinner->>Auth: Execute Logout()
Auth->>Auth: Check for existing token
alt Token exists
Auth->>Auth: Delete token
Auth-->>Spinner: (true, nil)
else No token
Auth-->>Spinner: (false, nil)
end
Spinner->>Output: EmitSuccess/EmitNote based on wasLoggedIn
Output->>UI: Render success/note message
UI->>User: Display completion message
Spinner-->>LogoutCmd: Return
LogoutCmd->>User: Exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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/auth.go`:
- Around line 55-67: Logout currently treats any error from
tokenStorage.GetAuthToken() as "not logged in", masking real storage failures;
add a sentinel error (e.g., var ErrTokenNotFound = errors.New("token not
found")) returned by GetAuthToken() instead of a wrapped fmt.Errorf, update
GetAuthToken's implementation to return that sentinel when the underlying
keyring.ErrKeyNotFound occurs, and modify Auth.Logout() to use errors.Is(err,
ErrTokenNotFound) to return (false, nil) only for the "not found" case while
returning actual errors for other failures; keep calls to
tokenStorage.DeleteAuthToken() and the final return behavior unchanged.
🧹 Nitpick comments (1)
internal/output/styles.go (1)
5-10: Consider usingAdaptiveColorconsistently across all styles.
WarningStyleusesAdaptiveColorfor light/dark theme adaptation, butPrimaryStyle,SecondaryStyle, andErrorStyleuse fixed colors. This may cause readability issues on light terminal backgrounds where#4DB6AC(teal) or#888888(gray) might have poor contrast.♻️ Optional: Use AdaptiveColor for all styles
var ( - PrimaryStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("#4DB6AC")) - SecondaryStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("#888888")) + PrimaryStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "#00796B", Dark: "#4DB6AC"}) + SecondaryStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "#555555", Dark: "#888888"}) WarningStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "#2E7D32", Dark: "#FFB800"}) - ErrorStyle = lipgloss.NewStyle().Foreground(lipgloss.Color("#FF5555")) + ErrorStyle = lipgloss.NewStyle().Foreground(lipgloss.AdaptiveColor{Light: "#C62828", Dark: "#FF5555"}) )
| // Logout removes the stored auth token from the keyring. | ||
| // Returns true if a token was found and deleted, false if no token existed. | ||
| func (a *Auth) Logout() (wasLoggedIn bool, err error) { | ||
| _, err = a.tokenStorage.GetAuthToken() | ||
| if err != nil { | ||
| return false, nil | ||
| } | ||
|
|
||
| err = a.tokenStorage.DeleteAuthToken() | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| return err | ||
| return true, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the GetAuthToken implementation and check what errors it returns
ast-grep --pattern 'func ($_ $_) GetAuthToken() ($_, $_) {
$$$
}'Repository: localstack/lstk
Length of output: 1179
🏁 Script executed:
# Also search for tokenStorage interface definition to understand the contract
rg -t go 'type tokenStorage|interface.*TokenStorage|type.*TokenStorage' -A 10Repository: localstack/lstk
Length of output: 2218
🏁 Script executed:
# Search for existing error types in the auth package
rg -t go 'var.*Err|type.*Error' internal/auth/Repository: localstack/lstk
Length of output: 41
🏁 Script executed:
# Check if there's a defined error for "credential not found" or if this is just a string
rg -t go 'credential not found|ErrTokenNotFound|ErrCredentialNotFound' internal/auth/Repository: localstack/lstk
Length of output: 138
🏁 Script executed:
# Check if there's a way to distinguish the errors in GetAuthToken more carefully
rg -t go 'return.*credential not found' -B 5 -A 2 internal/auth/Repository: localstack/lstk
Length of output: 546
🏁 Script executed:
# Check how Logout is tested to understand the intended behavior
rg -t go 'func.*Logout|Logout\(' internal/auth/ -A 10 | head -80Repository: localstack/lstk
Length of output: 535
🏁 Script executed:
# Check if there's any existing pattern for handling "credential not found" in the codebase
rg -t go 'errors\.Is.*credential|errors\.Is.*Err.*Token|errors\.Is.*Err.*Found' internal/auth/Repository: localstack/lstk
Length of output: 233
🏁 Script executed:
# Check if there are any tests for Logout() that show expected behavior
rg -t go 'func.*TestLogout|Logout.*test' internal/auth/ -A 20Repository: localstack/lstk
Length of output: 41
Logout() masks storage errors by treating "credential not found" and actual failures identically.
The GetAuthToken() implementation at line 67 wraps keyring.ErrKeyNotFound in fmt.Errorf("credential not found"), but Logout() at line 58-60 returns (false, nil) for all errors, including real storage failures. This hides permission errors, service unavailability, and other issues behind a "not logged in" response.
To fix this, GetAuthToken() should return a defined sentinel error (e.g., var ErrTokenNotFound = errors.New("token not found")) that can be checked with errors.Is() in Logout(), allowing it to distinguish "no token" from actual storage errors.
🤖 Prompt for AI Agents
In `@internal/auth/auth.go` around lines 55 - 67, Logout currently treats any
error from tokenStorage.GetAuthToken() as "not logged in", masking real storage
failures; add a sentinel error (e.g., var ErrTokenNotFound = errors.New("token
not found")) returned by GetAuthToken() instead of a wrapped fmt.Errorf, update
GetAuthToken's implementation to return that sentinel when the underlying
keyring.ErrKeyNotFound occurs, and modify Auth.Logout() to use errors.Is(err,
ErrTokenNotFound) to return (false, nil) only for the "not found" case while
returning actual errors for other failures; keep calls to
tokenStorage.DeleteAuthToken() and the final return behavior unchanged.
|
I'd prefer to add this after we have the actual TUI in there with #38, but looks good so far :) |
Sounds good, putting this on hold. Please, ping me when I can get back to this. 🏓 |
2 participants
This will add styled output events and spinner to output package, using also
This will ...
SuccessEventandNoteEventto the Sink system for consistent styled outputRunWithSpinnerhelper for operations with progress indicatorstyles.gowith color definitions, adaptive for light and dark themesauth.Logoutto return(wasLoggedIn, err)for better user feedbackFix DES-113