Conversation
appleboy
commented
Feb 26, 2026
appleboy
commented
- Introduce a Fallback field to FlowUpdate to distinguish soft errors (recoverable, trigger Device Code Flow) from hard errors (non-recoverable, surface to user).
- Update error handling logic to use the Fallback field instead of matching error messages.
- Add detailed documentation and comments clarifying error classes and flow control.
- Implement tests to verify correct handling and routing of soft and hard errors using the Fallback field.
- Introduce a Fallback field to FlowUpdate to distinguish soft errors (recoverable, trigger Device Code Flow) from hard errors (non-recoverable, surface to user). - Update error handling logic to use the Fallback field instead of matching error messages. - Add detailed documentation and comments clarifying error classes and flow control. - Implement tests to verify correct handling and routing of soft and hard errors using the Fallback field. Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds an explicit Fallback signal to FlowUpdate so the browser OAuth flow can distinguish recoverable (“soft”) failures that should trigger a silent fallback to Device Code Flow from non-recoverable (“hard”) failures that should be surfaced to the user.
Changes:
- Introduces
FlowUpdate.Fallbackwith documentation describing soft vs hard StepError handling. - Updates the Bubble Tea browser model to route StepError based on
Fallbackinstead of error-message matching. - Updates browser flow update emission to mark soft errors with
Fallback: trueand adds tests around the new behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tui/messages.go |
Adds Fallback to FlowUpdate and documents how it classifies StepError messages. |
tui/browser_model.go |
Switches StepError handling to use the Fallback flag instead of matching message strings. |
browser_flow.go |
Emits StepError updates for more exit paths and sets Fallback: true for soft errors (openBrowser failure, callback timeout). |
tui/manager_test.go |
Adds unit tests for the new Fallback field and for browser-model StepError routing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| t.Run("Fallback is ignored on non-error updates", func(t *testing.T) { | ||
| update := FlowUpdate{Type: StepStart, Step: 1, Fallback: false} | ||
| if update.Fallback { | ||
| t.Error("Fallback should not be set on non-error updates") | ||
| } | ||
| }) |
There was a problem hiding this comment.
The subtest "Fallback is ignored on non-error updates" is currently tautological (it sets Fallback: false and asserts it is false) and doesn’t verify any behavior. If the intent is to ensure non-error updates don’t trigger fallback handling, set Fallback: true and assert the consumer logic (e.g., handleFlowUpdate for StepStart) does not treat it as a fallback signal, or remove/rename this test.
| t.Run("Fallback is ignored on non-error updates", func(t *testing.T) { | |
| update := FlowUpdate{Type: StepStart, Step: 1, Fallback: false} | |
| if update.Fallback { | |
| t.Error("Fallback should not be set on non-error updates") | |
| } | |
| }) |
| 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 | ||
| if update.Fallback { | ||
| // Soft error — caller requested fall back to Device Code Flow. | ||
| return func() tea.Msg { | ||
| return successMsg{storage: nil, ok: false} | ||
| } | ||
| } | ||
| // Hard error — surface it to the user. | ||
| return func() tea.Msg { | ||
| return errorMsg{err: m.err} | ||
| } |
There was a problem hiding this comment.
In the StepError branch you set m.err before checking update.Fallback. For a soft error (Fallback=true) this leaves m.err non-nil, so renderBrowserComplete() will take the error path and display an authentication failure instead of the fallback-to-device-flow message. Set m.err only for hard errors, or explicitly clear it when handling a fallback StepError.
| // etc.) — surface it to the user. | ||
| updates <- tui.FlowUpdate{ | ||
| Type: tui.StepError, | ||
| Message: fmt.Sprintf("Authentication failed: %v", err), |
There was a problem hiding this comment.
This StepError update message includes the prefix "Authentication failed:", but the browser completion view also prefixes errors with "Authentication failed:". This can lead to duplicated text in the TUI (e.g., "Authentication failed: Authentication failed: …"). Prefer sending the raw error string here, or adjust the rendering to avoid double-prefixing.
| Message: fmt.Sprintf("Authentication failed: %v", err), | |
| Message: err.Error(), |
- Improve error message rendering by using only the error string. - Add tests for SimpleManager browser flow, ensuring correct fallback signaling and update handling. Signed-off-by: appleboy <appleboy.tw@gmail.com>
