refactor(tui): deduplicate shared utilities and simplify flow runners by appleboy · Pull Request #22 · go-authgate/cli

appleboy · 2026-03-12T14:35:09Z

Summary

Consolidate 3 identical token masking functions and 4 duration formatting functions into shared utilities in tui/styles.go
Extract doTokenExchange() helper to deduplicate HTTP token exchange across exchangeCode, refreshAccessToken
Unify OAuth error code maps between browser (error_sanitizer.go) and TUI (error_parser.go) packages
Extract generic runFlow() and handleFlowUpdate() methods to deduplicate browser/device flow runners in bubbletea_manager.go
Remove 5 unused exported methods (IsHidden, ViewSimple, ViewCompact) and their tests
Fix goroutine leak risk in browser flow timer by checking context cancellation before channel send

Net result: -172 lines (356 added, 528 removed) across 18 files.

Test plan

make build — builds successfully
make test — all existing tests pass (37.9% coverage main, 14.6% tui, 45.8% components)
make lint — 0 lint issues

🤖 Generated with Claude Code

- Consolidate three identical token masking functions into shared maskTokenPreview - Extract duration formatting into shared FormatDurationCompact/Human/Interval - Extract doTokenExchange helper to deduplicate HTTP token exchange logic - Unify OAuth error code maps with browser-specific overrides - Extract generic runFlow and handleFlowUpdate to deduplicate flow runners - Remove unused exported methods IsHidden, ViewSimple, ViewCompact - Fix goroutine leak risk in browser flow timer Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

codecov-commenter · 2026-03-12T14:36:32Z

Codecov Report

❌ Patch coverage is 11.96581% with 206 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
tui/bubbletea_manager.go	0.00%	106 Missing ⚠️
tui/styles.go	0.00%	34 Missing ⚠️
tokens.go	45.23%	18 Missing and 5 partials ⚠️
browser_flow.go	0.00%	14 Missing ⚠️
main.go	36.36%	5 Missing and 2 partials ⚠️
tui/token_view.go	0.00%	7 Missing ⚠️
tui/error_parser.go	25.00%	6 Missing ⚠️
tui/device_view.go	0.00%	3 Missing ⚠️
error_sanitizer.go	60.00%	1 Missing and 1 partial ⚠️
tui/simple_manager.go	0.00%	2 Missing ⚠️
... and 2 more

📢 Thoughts on this report? Let us know!

Copilot

Pull request overview

Refactors the TUI/auth flow implementation to reduce duplicated utilities and consolidate shared flow logic, while also centralizing OAuth error messaging and token exchange HTTP handling.

Changes:

Deduplicates token masking + duration/interval formatting into shared TUI utilities (tui/styles.go).
Extracts shared flow runner logic in Bubble Tea manager (runFlow/handleFlowUpdate) to unify browser + device runners.
Centralizes OAuth token exchange HTTP logic (doTokenExchange) and unifies OAuth error-code messaging between browser and TUI paths.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tui/unified_flow_view.go	Switches expiry rendering to shared duration formatter.
tui/token_view.go	Removes local masking/duration helpers; uses shared utilities and simplifies backend string parsing.
tui/styles.go	Adds shared duration/interval formatters and hosts token preview masking helper.
tui/simple_manager.go	Uses shared token preview masking helper.
tui/flow_renderer.go	Switches expiry rendering to shared duration formatter.
tui/error_parser.go	Introduces shared OAuth error message map and uses it in TUI sanitization.
tui/device_view.go	Switches interval formatting to shared helper and removes now-unused local helper/import.
tui/components/timer.go	Removes compact timer view API.
tui/components/step_indicator.go	Removes compact step indicator view API.
tui/components/progress_bar.go	Removes compact progress bar view API.
tui/components/info_box.go	Removes simple info box rendering API.
tui/components/help_view.go	Removes `IsHidden()` API.
tui/components/components_test.go	Removes tests for deleted compact/simple component views.
tui/bubbletea_manager.go	Refactors browser/device flow runners into shared `runFlow` + `handleFlowUpdate`.
tokens.go	Adds shared `doTokenExchange` + `tokenResponseToCredstore` helpers for OAuth token POST handling.
main.go	Uses `doTokenExchange` during refresh; keeps refresh-token preservation logic.
error_sanitizer.go	Uses shared TUI OAuth error messages with browser-specific overrides.
browser_flow.go	Uses shared token exchange helper and improves timer goroutine cancellation handling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-12T14:40:48Z

tokens.go

+					return nil, hookErr
+				}
+			}
+			return nil, fmt.Errorf("%s: %s", errResp.Error, errResp.ErrorDescription)


doTokenExchange formats OAuth errors as "%s: %s". If error_description is empty (allowed by the spec / common in some servers), this returns a trailing ": ", which then shows up in user-facing errors and logs. Consider omitting the separator when the description is empty (or falling back to a generic message) to avoid awkward output.

Suggested change

return nil, fmt.Errorf("%s: %s", errResp.Error, errResp.ErrorDescription)

desc := strings.TrimSpace(errResp.ErrorDescription)

if desc == "" {

return nil, fmt.Errorf("%s", errResp.Error)

}

return nil, fmt.Errorf("%s: %s", errResp.Error, desc)

Copilot · 2026-03-12T14:40:48Z

tui/styles.go

+	d = d.Round(time.Second)
+	minutes := int(d.Minutes())
+	seconds := int(d.Seconds()) % 60
+	return fmt.Sprintf("%d:%02d", minutes, seconds)


FormatDurationCompact doesn’t handle negative durations correctly: int(d.Seconds()) % 60 is negative for negative inputs, producing outputs like -1:-05. Since this function is exported, it should behave sensibly for negative values (e.g., clamp to 0, or format with a leading - and absolute seconds).

Suggested change

d = d.Round(time.Second)

minutes := int(d.Minutes())

seconds := int(d.Seconds()) % 60

return fmt.Sprintf("%d:%02d", minutes, seconds)

sign := ""

if d < 0 {

sign = "-"

d = -d

}

d = d.Round(time.Second)

totalSeconds := int(d.Seconds())

minutes := totalSeconds / 60

seconds := totalSeconds % 60

return fmt.Sprintf("%s%d:%02d", sign, minutes, seconds)

Copilot · 2026-03-12T14:40:48Z

tui/error_parser.go

+// OAuthErrorMessages maps standard OAuth error codes to user-friendly TUI messages.
+// This is the authoritative map; other packages should delegate here.
+var OAuthErrorMessages = map[string]string{
+	"access_denied":           "Authorization was denied. You may have cancelled the request or don't have permission to access this resource.",
+	"invalid_request":         "The authorization request was invalid. Please check your configuration.",
+	"unauthorized_client":     "The client is not authorized to use this authorization method. Please verify your client ID and permissions.",
+	"invalid_client":          "Client authentication failed. Please check your client ID and secret.",
+	"invalid_grant":           "The authorization code or refresh token is invalid or expired.",
+	"unsupported_grant_type":  "The authorization grant type is not supported by this server.",
+	"invalid_scope":           "One or more requested scopes are invalid or not allowed.",
+	"server_error":            "The authorization server encountered an error. Please try again later.",
+	"temporarily_unavailable": "The authorization server is temporarily unavailable. Please try again in a few moments.",
+}


OAuthErrorMessages is exported as a mutable map[string]string. Any importing package can modify it at runtime, which can lead to hard-to-debug behavior and makes the “authoritative map” claim unenforceable. Consider keeping the map unexported and exposing a read-only accessor (e.g., OAuthErrorMessage(code) (string, bool) or SanitizeOAuthErrorForTUI(...)).

Copilot · 2026-03-12T14:40:49Z

tui/bubbletea_manager.go

 		case <-ctx.Done():
-			return nil, false, ctx.Err()
+			return nil, ctx.Err()
 		}


In runFlow, the case <-ctx.Done() branch returns immediately. That stops draining updates, so any in-flight flow goroutine that attempts to send another update can block forever (leaking the goroutine) if the buffer fills. Consider canceling flowCtx and continuing to drain/ignore updates until resultCh returns (or redesign update sends to be non-blocking on context cancellation).

- Replace direct access to the OAuth error message map with a getter function to centralize and encapsulate error handling. - Improve OAuth token exchange errors by omitting empty descriptions and trimming whitespace. - Prevent goroutine and channel leaks in the TUI flow by explicitly canceling context and draining channels on exit. - Make the OAuth error message map private and add a public accessor to enforce a single authoritative source. - Enhance duration formatting to correctly handle and display negative durations. Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>

Copilot AI review requested due to automatic review settings March 12, 2026 14:35

Copilot started reviewing on behalf of appleboy March 12, 2026 14:35 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

appleboy merged commit be34c58 into main Mar 13, 2026
14 of 16 checks passed

appleboy deleted the worktree-woolly-wibbling-fiddle branch March 13, 2026 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(tui): deduplicate shared utilities and simplify flow runners#22

refactor(tui): deduplicate shared utilities and simplify flow runners#22
appleboy merged 2 commits intomainfrom
worktree-woolly-wibbling-fiddle

appleboy commented Mar 12, 2026

Uh oh!

codecov-commenter commented Mar 12, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Copilot AI Mar 12, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

-			return nil, fmt.Errorf("%s: %s", errResp.Error, errResp.ErrorDescription)
+			desc := strings.TrimSpace(errResp.ErrorDescription)
+			if desc == "" {
+				return nil, fmt.Errorf("%s", errResp.Error)
+			}
+			return nil, fmt.Errorf("%s: %s", errResp.Error, desc)

-	d = d.Round(time.Second)
-	minutes := int(d.Minutes())
-	seconds := int(d.Seconds()) % 60
-	return fmt.Sprintf("%d:%02d", minutes, seconds)
+	sign := ""
+	if d < 0 {
+		sign = "-"
+		d = -d
+	}
+	d = d.Round(time.Second)
+	totalSeconds := int(d.Seconds())
+	minutes := totalSeconds / 60
+	seconds := totalSeconds % 60
+	return fmt.Sprintf("%s%d:%02d", sign, minutes, seconds)

Conversation

appleboy commented Mar 12, 2026

Summary

Test plan

Uh oh!

codecov-commenter commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Mar 12, 2026 •

edited

Loading