Conversation
appleboy
commented
Feb 22, 2026
appleboy
commented
- Add an interactive Terminal UI (TUI) for OAuth flows using Bubble Tea, with auto-detection of environment to choose between TUI or simple text output.
- Introduce a Manager interface to abstract UI logic, with BubbleTeaManager and SimpleManager implementations for interactive and non-interactive modes.
- Update CLI flow control so all user feedback (headers, progress, authentication state) is routed through the Manager.
- Implement progress reporting for browser and device flows using FlowUpdate messages and channels.
- Add reusable TUI components: step indicators, timers, progress bars, and info boxes.
- Provide rendering code for browser and device flow views with spinners, layout, and color-coded messages.
- Add forced UI mode selection via --simple-ui and --interactive-ui flags.
- Extend test coverage for new TUI components and manager selection (unit tests for UI helpers, CI-detection).
- Add adapter code for TokenStorage between main and TUI packages.
- Add Bubble Tea and related libraries to dependencies in go.mod.
There was a problem hiding this comment.
Pull request overview
Adds a new tui/ package to provide an interactive Bubble Tea-based terminal UI for OAuth browser/device flows, while preserving a simple printf mode for CI/non-interactive environments. The CLI now routes user-facing output through a tui.Manager selected via environment detection and new --simple-ui/--interactive-ui flags.
Changes:
- Introduce
tui.Managerabstraction withBubbleTeaManager(interactive) andSimpleManager(printf) plus selection logic. - Add Bubble Tea models/views and reusable components (step indicator, timer, progress bar, info box) for browser and device flows.
- Refactor main authentication flow to emit progress updates via
FlowUpdatechannels and render via the selected manager; update docs/tests and dependencies.
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
main.go |
Routes headers/progress/verification/demo output through a selected tui.Manager; switches flows to update-driven wrappers. |
config.go |
Adds --simple-ui / --interactive-ui flags and stores forced UI mode. |
browser_flow.go |
Adds performBrowserFlowWithUpdates that emits FlowUpdate messages (timer ticks, steps, errors). |
device_flow.go |
Adds performDeviceFlowWithUpdates + polling loop that emits FlowUpdate messages (poll count, backoff, timer ticks). |
tui_adapter.go |
Adapts token storage types between main and tui packages to avoid import cycles. |
tui/manager.go |
Defines the Manager interface, environment detection, and manager selection. |
tui/simple_manager.go |
Implements non-interactive/simple output while consuming flow updates. |
tui/bubbletea_manager.go |
Runs Bubble Tea programs for flows and falls back to simple output on TUI failure. |
tui/messages.go |
Defines FlowUpdate message types and helper accessors. |
tui/browser_model.go / tui/browser_view.go |
Bubble Tea model/view for browser flow (spinner/timer/progress). |
tui/device_model.go / tui/device_view.go |
Bubble Tea model/view for device flow (info box, polling/backoff, elapsed timer). |
tui/styles.go |
Centralized Lipgloss styles and rendering helpers. |
tui/components/* |
Reusable TUI components with unit tests. |
tui/manager_test.go |
Adds tests for selection/helpers (but currently misses key logic paths). |
README.md |
Documents the new interactive TUI and UI mode selection/flags. |
CLAUDE.md |
Documents the new TUI architecture and message-passing design. |
go.mod / go.sum |
Adds Bubble Tea ecosystem deps and bumps Go version directive. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fmt.Println("Step 1: Opening browser for authorization...") | ||
| if url := update.GetString("url"); url != "" { | ||
| fmt.Printf("\n %s\n\n", url) | ||
| } | ||
| } else if update.Step == 2 { | ||
| fmt.Println("Browser opened. Please complete authorization in your browser.") | ||
| port := update.GetInt("port") | ||
| if port == 0 { | ||
| port = 8888 // default | ||
| } | ||
| fmt.Printf("Step 2: Waiting for callback on http://localhost:%d/callback ...\n", port) | ||
| } else if update.Step == 3 { | ||
| fmt.Println("Step 3: Exchanging authorization code for tokens...") |
There was a problem hiding this comment.
handleBrowserUpdate hardcodes step text for StepStart(step=1) as "Opening browser...". The browser wrapper emits a StepStart for step 1 with message "Generating PKCE parameters", which will be printed as "Opening browser..." and then printed again when the actual open-browser update arrives. Consider using update.Message for StepStart output (or introducing a distinct update type/step for PKCE generation).
| fmt.Println("Step 1: Opening browser for authorization...") | |
| if url := update.GetString("url"); url != "" { | |
| fmt.Printf("\n %s\n\n", url) | |
| } | |
| } else if update.Step == 2 { | |
| fmt.Println("Browser opened. Please complete authorization in your browser.") | |
| port := update.GetInt("port") | |
| if port == 0 { | |
| port = 8888 // default | |
| } | |
| fmt.Printf("Step 2: Waiting for callback on http://localhost:%d/callback ...\n", port) | |
| } else if update.Step == 3 { | |
| fmt.Println("Step 3: Exchanging authorization code for tokens...") | |
| if update.Message != "" { | |
| fmt.Println(update.Message) | |
| } else { | |
| fmt.Println("Step 1: Opening browser for authorization...") | |
| } | |
| if url := update.GetString("url"); url != "" { | |
| fmt.Printf("\n %s\n\n", url) | |
| } | |
| } else if update.Step == 2 { | |
| if update.Message != "" { | |
| fmt.Println(update.Message) | |
| } else { | |
| fmt.Println("Browser opened. Please complete authorization in your browser.") | |
| } | |
| port := update.GetInt("port") | |
| if port == 0 { | |
| port = 8888 // default | |
| } | |
| fmt.Printf("Step 2: Waiting for callback on http://localhost:%d/callback ...\n", port) | |
| } else if update.Step == 3 { | |
| if update.Message != "" { | |
| fmt.Println(update.Message) | |
| } else { | |
| fmt.Println("Step 3: Exchanging authorization code for tokens...") | |
| } |
| func TestFlowUpdateHelpers(t *testing.T) { | ||
| update := FlowUpdate{ | ||
| Type: StepStart, | ||
| Step: 1, | ||
| Message: "Testing", | ||
| Data: map[string]interface{}{ | ||
| "string_val": "hello", | ||
| "int_val": 42, | ||
| "float_val": 3.14, | ||
| "duration_val": 5000000000, // 5 seconds in nanoseconds | ||
| }, | ||
| } |
There was a problem hiding this comment.
In TestFlowUpdateHelpers, duration_val is provided as an int (nanoseconds) but GetDuration expects a time.Duration value, and the test never asserts GetDuration. This means the duration helper isn’t actually covered. Consider setting duration_val to time.Duration(5*time.Second) and asserting update.GetDuration("duration_val") returns 5s.
| module github.com/go-authgate/cli | ||
|
|
||
| go 1.24.0 | ||
| go 1.24.2 |
There was a problem hiding this comment.
The go directive is set to 1.24.2. The go.mod go version is intended to be a language version and typically shouldn’t include a non-zero patch level; this may be rejected by Go tooling. Consider using go 1.24 (or 1.24.0) and, if you need to pin a toolchain patch version, use a separate toolchain go1.24.2 directive.
| go 1.24.2 | |
| go 1.24 | |
| toolchain go1.24.2 |
| - Output piped to file or another command | ||
| - Terminal too small (< 60 columns or < 20 rows) | ||
| - `TERM=dumb` or TERM unset | ||
| - SSH session without display forwarding |
There was a problem hiding this comment.
The UI auto-selection bullets include “SSH session without display forwarding”, but shouldUseSimpleUI() doesn’t check SSH/DISPLAY; that condition currently affects browser flow availability (checkBrowserAvailability), not UI mode. Consider either implementing this check in shouldUseSimpleUI or adjusting the documentation to match the actual behavior.
| - SSH session without display forwarding |
| // Channel closed | ||
| return nil |
There was a problem hiding this comment.
waitForUpdate returns nil when the updates channel is closed. In Bubble Tea, a Cmd returning nil produces no message, so the model may keep running (spinner ticks) and never quit if the OAuth flow ends without emitting a final StepComplete/StepError (e.g., ctx cancellation paths). Consider returning a dedicated "flowFinished" message (or errorMsg) on channel close so the program can terminate cleanly.
| // Channel closed | |
| return nil | |
| // Channel closed: signal an error so the program can terminate cleanly. | |
| return errorMsg{err: fmt.Errorf("authorization flow ended without a final status update")} |
config.go
Outdated
| // --device or --no-browser forces Device Code Flow unconditionally. | ||
| forceDevice = *flagDevice || *flagNoBrowser | ||
|
|
||
| // --simple-ui or --interactive-ui forces UI mode. |
There was a problem hiding this comment.
--simple-ui and --interactive-ui can both be set at once; SelectManager currently resolves this by always choosing simple mode due to ordering. Consider validating the flags in initConfig and exiting with a clear error when both are set to avoid surprising behavior.
| // --simple-ui or --interactive-ui forces UI mode. | |
| // --simple-ui or --interactive-ui forces UI mode. | |
| if *flagSimpleUI && *flagInteractiveUI { | |
| fmt.Fprintln(os.Stderr, "Error: --simple-ui and --interactive-ui cannot be used together. Please specify only one.") | |
| os.Exit(1) | |
| } |
README.md
Outdated
| # Force interactive TUI (override environment detection) | ||
| ./bin/authgate-cli --interactive-ui | ||
| ``` | ||
|
|
There was a problem hiding this comment.
This section introduces --simple-ui/--interactive-ui, but the CLI flags table later in the README doesn’t list these new flags. Consider adding them to the “CLI flags” table so users can discover them alongside the other options.
| #### CLI flags | |
| | Flag | Description | | |
| | ----------------- | ----------------------------------------------------- | | |
| | `--simple-ui` | Force simple printf-based output (no TUI). | | |
| | `--interactive-ui`| Force interactive TUI, overriding environment detection. | |
|
|
||
| // Manager is the interface for managing UI output in the CLI. | ||
| // It abstracts the presentation layer, allowing for different implementations: | ||
| // - SimplePrintManager: Uses fmt.Printf (current behavior, for CI/pipelines) |
There was a problem hiding this comment.
The Manager interface comment mentions “SimplePrintManager”, but the type implemented in this package is named SimpleManager (and NewSimpleManager). Consider updating the comment to match the actual type name to avoid confusion for readers.
| // - SimplePrintManager: Uses fmt.Printf (current behavior, for CI/pipelines) | |
| // - SimpleManager: Uses fmt.Printf (current behavior, for CI/pipelines) |
| case StepError: | ||
| m.err = fmt.Errorf("%s", update.Message) | ||
| if update.Message == "Browser authorization timed out" || update.Message == "Could not open browser: exit status 1" { | ||
| // Fallback to device flow | ||
| return func() tea.Msg { | ||
| return successMsg{storage: nil, ok: false} | ||
| } |
There was a problem hiding this comment.
StepError handling sets m.err before deciding whether the error should trigger a fallback (ok=false). This causes the UI to render the error state even when falling back is intended, and the open-browser match is overly specific (exact string "...exit status 1"). Suggest: detect fallback via structured data (or a message prefix), avoid setting m.err for fallback cases, and don’t rely on exact error strings.
| case "slow_down": | ||
| oldInterval := pollInterval | ||
| backoffMultiplier *= 1.5 | ||
| newInterval := time.Duration(float64(pollInterval) * backoffMultiplier) | ||
| if newInterval > 60*time.Second { | ||
| newInterval = 60 * time.Second | ||
| } | ||
| pollInterval = newInterval | ||
| pollTicker.Reset(pollInterval) |
There was a problem hiding this comment.
The slow_down backoff math multiplies pollInterval by an accumulating multiplier, but pollInterval is already updated each time; this results in super-exponential growth (1.5^(n(n+1)/2)). Use either pollInterval = time.Duration(float64(pollInterval)*1.5) (no separate multiplier) or follow RFC 8628’s recommendation to increase the interval by a fixed amount (commonly +5s) per slow_down.
- Add a terminal UI architecture with environment-adaptive, interactive Bubble Tea TUI and print fallback - Implement a new Manager interface abstracting all UI responsibilities and automatic UI selection - Introduce message-passing architecture and progress updates for OAuth flows via channels - Provide wrapper functions for OAuth flows that emit progress for UI consumption - Create reusable TUI components: step indicator, timer, progress bar, info box - Implement detailed browser and device flow models and views for TUI - Add and update test coverage for TUI manager behavior and UI components - Refactor main program flow to use UI Manager abstraction instead of direct fmt.Printf calls - Update authentication and token handling to report states via Manager methods - Add Bubble Tea, Lipgloss, and other TUI-related dependencies to go.mod - Increase Go version in go.mod to 1.24.2 - Improve documentation to explain interactive terminal UI and automatic environment detection Signed-off-by: appleboy <appleboy.tw@gmail.com>
- Change dependency declarations for bubbles, bubbletea, and lipgloss from indirect to direct - Add go-isatty and golang.org/x/term as direct dependencies - Remove go-isatty and golang.org/x/term as indirect dependencies Signed-off-by: appleboy <appleboy.tw@gmail.com>
…stency - Change function signatures to multi-line style for improved readability - Refactor argument passing for constructors and error handling to multi-line in TUI models and components - No logic changes, only formatting and style improvements for consistency Signed-off-by: appleboy <appleboy.tw@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 23 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| return nil, false, fmt.Errorf("failed to generate state: %w", err) | ||
| } | ||
|
|
||
| pkce, err := GeneratePKCE() | ||
| if err != nil { |
There was a problem hiding this comment.
On early failures (e.g., generateState() / GeneratePKCE()), this function returns an error without emitting a StepError update. In interactive mode, the Bubble Tea model only reacts to FlowUpdates, so the UI can hang indefinitely. Ensure all exit paths send a terminal update (StepError/StepComplete) before returning.
| if err != nil { | |
| return nil, false, fmt.Errorf("failed to generate state: %w", err) | |
| } | |
| pkce, err := GeneratePKCE() | |
| if err != nil { | |
| if err != nil { | |
| updates <- tui.FlowUpdate{ | |
| Type: tui.StepError, | |
| Message: fmt.Sprintf("Failed to generate state: %v", err), | |
| } | |
| return nil, false, fmt.Errorf("failed to generate state: %w", err) | |
| } | |
| pkce, err := GeneratePKCE() | |
| if err != nil { | |
| updates <- tui.FlowUpdate{ | |
| Type: tui.StepError, | |
| Message: fmt.Sprintf("Failed to generate PKCE: %v", err), | |
| } |
| } | ||
| resultCh := make(chan result, 1) | ||
|
|
||
| // Start OAuth flow in a goroutine | ||
| go func() { | ||
| storage, ok, err := perform(flowCtx, updates) | ||
| resultCh <- result{storage, ok, err} | ||
| close(updates) | ||
| }() | ||
|
|
||
| // Create and run Bubble Tea program | ||
| model := NewBrowserModel(updates, cancel) | ||
| p := tea.NewProgram(model) | ||
|
|
||
| finalModel, err := p.Run() | ||
| if err != nil { | ||
| // If TUI fails, cancel OAuth flow and fall back to simple mode | ||
| cancel() | ||
| simple := NewSimpleManager() | ||
| return simple.RunBrowserFlow(ctx, perform) | ||
| } | ||
|
|
||
| // Wait for OAuth flow to complete or timeout | ||
| select { | ||
| case res := <-resultCh: | ||
| // Extract final state from model | ||
| if bm, ok := finalModel.(*BrowserModel); ok { | ||
| // Check if user cancelled | ||
| if bm.userCancelled { | ||
| return nil, false, context.Canceled | ||
| } | ||
| if bm.storage != nil { | ||
| return bm.storage, bm.ok, nil | ||
| } | ||
| } | ||
| return res.storage, res.ok, res.err | ||
| case <-ctx.Done(): | ||
| // Parent context cancelled |
There was a problem hiding this comment.
The Bubble Tea program has no guaranteed quit signal when the OAuth goroutine finishes with an error (many error paths return without emitting StepError). Since the model only listens to updates, the UI can keep running after the flow completes and updates is closed. Consider waiting on resultCh and using p.Send(errorMsg/successMsg) (or a dedicated final message) so the program exits reliably and can render the terminal state.
| // Channel closed | ||
| return nil |
There was a problem hiding this comment.
When the updates channel is closed, waitForUpdate returns nil, and no new command is scheduled. If the model hasn't already quit, the TUI can get stuck. Consider returning a specific message (e.g., errorMsg{...} or a doneMsg) on channel close so the model can quit/transition deterministically.
| // Channel closed | |
| return nil | |
| // Channel closed: return an error message so the model can | |
| // handle this deterministically (e.g., quit or transition). | |
| return errorMsg{err: fmt.Errorf("flow updates channel closed")} |
| // Give a brief moment for the cancellation to propagate | ||
| time.Sleep(100 * time.Millisecond) |
There was a problem hiding this comment.
Calling time.Sleep inside Bubble Tea Update blocks the UI event loop. If you need a delay before quitting, use a tea.Tick/command-based approach, or just quit immediately after canceling the context.
| // Give a brief moment for the cancellation to propagate | |
| time.Sleep(100 * time.Millisecond) |
| var expiresAt time.Time | ||
| switch v := tuiStorage.ExpiresAt.(type) { | ||
| case time.Time: | ||
| expiresAt = v |
There was a problem hiding this comment.
fromTUITokenStorage silently falls back to time.Time{} when ExpiresAt isn't a time.Time, which can turn a valid expiry into an already-expired token without surfacing an error. If ExpiresAt remains untyped, consider handling additional expected types (e.g., *time.Time, RFC3339 string) or returning an error on unexpected types. If you switch tui.TokenStorage.ExpiresAt to time.Time, this conversion can be simplified.
| expiresAt = v | |
| expiresAt = v | |
| case *time.Time: | |
| if v != nil { | |
| expiresAt = *v | |
| } | |
| case string: | |
| if parsed, err := time.Parse(time.RFC3339, v); err == nil { | |
| expiresAt = parsed | |
| } | |
| case *string: | |
| if v != nil { | |
| if parsed, err := time.Parse(time.RFC3339, *v); err == nil { | |
| expiresAt = parsed | |
| } | |
| } |
|
|
||
| case PollingUpdate: | ||
| m.pollCount = update.GetInt("poll_count") | ||
| m.elapsed = update.GetDuration("elapsed") |
There was a problem hiding this comment.
PollingUpdate includes the current polling interval in Data["interval"], but the model never updates m.pollInterval from that value. This can display an incorrect interval when the server's initial deviceAuth.Interval isn't 5s. Consider reading interval from the update and updating m.pollInterval accordingly.
| m.elapsed = update.GetDuration("elapsed") | |
| m.elapsed = update.GetDuration("elapsed") | |
| m.pollInterval = update.GetDuration("interval") |
| titleBox := lipgloss.NewStyle(). | ||
| Border(lipgloss.RoundedBorder()). | ||
| BorderForeground(colorPrimary). | ||
| Padding(0, 2). | ||
| Width(m.width - 4). |
There was a problem hiding this comment.
Width(m.width - 4) can become negative if the terminal is resized very small after startup, which can cause rendering glitches/panics in some lipgloss layouts. Consider clamping the computed width to a minimum (e.g., max(0, m.width-4)) before passing it to lipgloss.
| titleBox := lipgloss.NewStyle(). | |
| Border(lipgloss.RoundedBorder()). | |
| BorderForeground(colorPrimary). | |
| Padding(0, 2). | |
| Width(m.width - 4). | |
| titleWidth := m.width - 4 | |
| if titleWidth < 0 { | |
| titleWidth = 0 | |
| } | |
| titleBox := lipgloss.NewStyle(). | |
| Border(lipgloss.RoundedBorder()). | |
| BorderForeground(colorPrimary). | |
| Padding(0, 2). | |
| Width(titleWidth). |
| titleBox := lipgloss.NewStyle(). | ||
| Border(lipgloss.RoundedBorder()). | ||
| BorderForeground(colorPrimary). | ||
| Padding(0, 2). | ||
| Width(m.width - 4). |
There was a problem hiding this comment.
Width(m.width - 4) can become negative if the terminal is resized very small after startup, which can cause rendering glitches/panics in some lipgloss layouts. Consider clamping the computed width to a minimum (e.g., max(0, m.width-4)) before passing it to lipgloss.
| titleBox := lipgloss.NewStyle(). | |
| Border(lipgloss.RoundedBorder()). | |
| BorderForeground(colorPrimary). | |
| Padding(0, 2). | |
| Width(m.width - 4). | |
| titleWidth := m.width - 4 | |
| if titleWidth < 0 { | |
| titleWidth = 0 | |
| } | |
| titleBox := lipgloss.NewStyle(). | |
| Border(lipgloss.RoundedBorder()). | |
| BorderForeground(colorPrimary). | |
| Padding(0, 2). | |
| Width(titleWidth). |
| // TokenStorage represents the stored OAuth tokens. | ||
| // This is a placeholder - the actual type should match the one in the main package. | ||
| type TokenStorage struct { | ||
| AccessToken string | ||
| RefreshToken string | ||
| TokenType string | ||
| ExpiresAt interface{} // time.Time, but avoiding import cycle | ||
| ClientID string | ||
| Flow string | ||
| } |
There was a problem hiding this comment.
tui.TokenStorage.ExpiresAt is declared as interface{} with a comment about avoiding an import cycle, but time.Time is in the stdlib and won't create cycles here. Using time.Time would remove runtime type assertions throughout the code and avoid silently zeroing expiry on mismatch.
| // If TUI fails, cancel OAuth flow and fall back to simple mode | ||
| cancel() |
There was a problem hiding this comment.
If p.Run() fails, this falls back by calling perform again via simple.RunBrowserFlow(...) while the first perform(flowCtx, updates) goroutine may still be running (cancel is async). That can result in duplicate auth flows/port contention. Prefer waiting for the first goroutine to exit (or reuse its resultCh) instead of starting a second flow immediately.
| // If TUI fails, cancel OAuth flow and fall back to simple mode | |
| cancel() | |
| // If TUI fails, cancel OAuth flow and fall back to simple mode. | |
| // Wait for the original flow goroutine to finish (or for the parent | |
| // context to be cancelled) before starting a new flow to avoid | |
| // overlapping OAuth flows / port contention. | |
| cancel() | |
| select { | |
| case <-resultCh: | |
| // Original flow has finished; safe to start a fallback flow. | |
| case <-ctx.Done(): | |
| // Parent context cancelled; propagate the error instead of | |
| // starting a new flow. | |
| return nil, false, ctx.Err() | |
| } |
1 participant
