Improve event system structure and add reusable TUI components#50
Merged
Improve event system structure and add reusable TUI components#50
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors the event system to replace Changes
Sequence Diagram(s)sequenceDiagram
participant App as App<br/>(TUI)
participant Sink as Output<br/>Sink
participant Formatter as Formatter
participant Display as Display<br/>Components
rect rgba(100, 150, 200, 0.5)
note over App,Display: MessageEvent Flow
App->>Sink: EmitInfo/Success/Warning/Note
Sink->>App: MessageEvent
App->>Formatter: FormatEventLine(MessageEvent)
Formatter->>Display: styled text with prefix
Display->>App: rendered line
end
rect rgba(100, 200, 150, 0.5)
note over App,Display: SpinnerEvent Flow
App->>Sink: EmitSpinnerStart/Stop
Sink->>App: SpinnerEvent (Active)
App->>App: spinner.Start(text)
App->>Display: spinner.View()
Display->>App: "Loading..."
end
rect rgba(200, 150, 100, 0.5)
note over App,Display: ErrorEvent Flow
App->>Sink: EmitError(ErrorEvent)
Sink->>App: ErrorEvent
App->>App: errorDisplay.Show(event)
App->>App: spinner.Stop()
App->>Display: errorDisplay.View()
Display->>App: formatted error with title/summary/actions
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (1)
internal/ui/styles/styles.go (1)
31-57: Consider removing self-explanatory section comments.The grouping comments here restate what the code already makes obvious; keeping just semantic variable names is cleaner.
As per coding guidelines
**/*.go: Don't add comments for self-explanatory code. Only comment when the "why" isn't obvious from the code itself.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/styles/styles.go` around lines 31 - 57, Remove the self-explanatory grouping comments and leave the semantic style variables as-is; specifically delete the comment lines "Message severity styles", "Secondary/muted style for prefixes", "Error styles" and "Spinner style" while keeping the style declarations (Success, Note, Warning, Secondary, ErrorTitle, ErrorDetail, ErrorAction, SpinnerStyle) and their formatting intact so only non-actionable, redundant comments are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/output/events.go`:
- Around line 126-128: The deprecation comment on EmitWarning is
self-referential; update the doc comment to point users to the non-deprecated
alternative by instructing them to call Emit with a MessageEvent (e.g., use
Emit(sink, MessageEvent{Severity: SeverityWarning, Text: ...})); modify the
comment above func EmitWarning to mention Emit and MessageEvent/SeverityWarning
explicitly instead of referencing EmitWarning itself.
In `@internal/ui/components/message.go`:
- Around line 8-19: RenderMessage currently duplicates severity label/prefix
logic; replace that logic by calling the shared formatter
output.FormatEventLine(e) to get the canonical line semantics and then apply TUI
styling. Concretely, remove the switch in RenderMessage, call formatted :=
output.FormatEventLine(e), and return the TUI prefix (styles.Secondary.Render(">
")) plus the styled formatted string (e.g. styles.Message.Render(formatted)) so
the same semantics are produced by FormatEventLine while keeping TUI-specific
styling in RenderMessage.
In `@internal/ui/components/spinner_test.go`:
- Around line 51-54: The test assigns the updated spinner to variable s from
spinner.Update but never uses it, causing SA4006; change the assignment to use
the blank identifier so the linter is satisfied—replace the left-hand variable
`s` with `_` in the call to `spinner.Update` (e.g., use `_, cmd :=
spinner.Update(spinner.TickMsg{})`) so `cmd` is still captured and `s` is not
unused.
---
Nitpick comments:
In `@internal/ui/styles/styles.go`:
- Around line 31-57: Remove the self-explanatory grouping comments and leave the
semantic style variables as-is; specifically delete the comment lines "Message
severity styles", "Secondary/muted style for prefixes", "Error styles" and
"Spinner style" while keeping the style declarations (Success, Note, Warning,
Secondary, ErrorTitle, ErrorDetail, ErrorAction, SpinnerStyle) and their
formatting intact so only non-actionable, redundant comments are removed.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
go.modinternal/auth/auth_test.gointernal/output/events.gointernal/output/format.gointernal/output/format_test.gointernal/output/plain_sink_test.gointernal/output/tui_sink_test.gointernal/ui/app.gointernal/ui/app_test.gointernal/ui/components/error_display.gointernal/ui/components/error_display_test.gointernal/ui/components/message.gointernal/ui/components/spinner.gointernal/ui/components/spinner_test.gointernal/ui/styles/styles.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/container/start.go (1)
216-223: Consider migrating the remainingEmitWarningcalls in this file.Lines 216 and 222 still use the deprecated
EmitWarning. While these are backward-compatible (they forward toMessageEventinternally), migrating them now keeps this file consistent with the changes introduced in this PR and reduces future churn.♻️ Proposed migration of EmitWarning calls
- output.EmitWarning(sink, fmt.Sprintf("failed to close response body: %v", err)) + output.EmitWarning(sink, fmt.Sprintf("failed to close response body: %v", err)) // or EmitWarning equivalent via MessageEvent if an EmitWarning helper existsVerify what the new severity-tagged equivalent of
EmitWarningis (e.g.,output.EmitWarningalready forwarding, or a newEmitWarning/EmitNote), then apply consistently to both occurrences.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/container/start.go` around lines 216 - 223, Replace the deprecated output.EmitWarning(sink, ...) calls with the new severity-tagged message API used elsewhere in the PR: call output.MessageEvent(sink, output.SeverityWarning, fmt.Sprintf("failed to close response body: %v", err)) (or the project's equivalent helper that accepts a severity and message) for both occurrences around resp.Body.Close and the early-return block; keep the same fmt.Sprintf text and the same sink and error variable names so behavior and context remain unchanged.internal/auth/login.go (1)
87-103: ConsiderSpinnerEventfor the three transient progress steps incompleteAuth.Lines 87, 95, and 102 are intermediate loading states ("Checking…", "exchanging…", "Fetching…") that are immediately superseded by the next step or by an error. They're semantically SpinnerStart/Stop pairs rather than persistent Info messages. The PR already notes this migration as a follow-up ("Use EmitSpinnerStart/EmitSpinnerStop in relevant commands"), so tagging for awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/login.go` around lines 87 - 103, Replace the three transient output.EmitInfo calls in completeAuth (the calls emitting "Checking if auth request is confirmed...", "Auth request confirmed, exchanging for token...", and "Fetching license token...") with spinner events: call output.EmitSpinnerStart(l.sink, "<message>") before each async operation (CheckAuthRequestConfirmed, ExchangeAuthRequest, GetLicenseToken) and ensure matching output.EmitSpinnerStop(l.sink) on both success paths and in error returns so spinners are stopped when an error occurs; keep the same messages and leave non-transient final messages as EmitInfo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/auth/login.go`:
- Around line 87-103: Replace the three transient output.EmitInfo calls in
completeAuth (the calls emitting "Checking if auth request is confirmed...",
"Auth request confirmed, exchanging for token...", and "Fetching license
token...") with spinner events: call output.EmitSpinnerStart(l.sink,
"<message>") before each async operation (CheckAuthRequestConfirmed,
ExchangeAuthRequest, GetLicenseToken) and ensure matching
output.EmitSpinnerStop(l.sink) on both success paths and in error returns so
spinners are stopped when an error occurs; keep the same messages and leave
non-transient final messages as EmitInfo.
In `@internal/container/start.go`:
- Around line 216-223: Replace the deprecated output.EmitWarning(sink, ...)
calls with the new severity-tagged message API used elsewhere in the PR: call
output.MessageEvent(sink, output.SeverityWarning, fmt.Sprintf("failed to close
response body: %v", err)) (or the project's equivalent helper that accepts a
severity and message) for both occurrences around resp.Body.Close and the
early-return block; keep the same fmt.Sprintf text and the same sink and error
variable names so behavior and context remain unchanged.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/auth/auth.gointernal/auth/login.gointernal/container/start.gointernal/output/events.gointernal/ui/components/spinner_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/ui/components/spinner_test.go
- internal/output/events.go
silv-io
force-pushed
the
silv-io/flc-369
branch
from
5836aba to
7b4557c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/output/plain_sink_test.go (1)
172-184: Consider addingDetailassertion in this plain-sink error test.This case verifies title/summary/actions, but not
ErrorEvent.Detail. Adding oneDetailcase here would strengthen sink-level parity for multiline error rendering.Based on learnings, "When adding a new event type, update all of:
internal/output/events.go(event type + union + emit helper),internal/output/format.go(line formatting fallback), tests ininternal/output/*_test.gofor formatter/sink behavior parity".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_sink_test.go` around lines 172 - 184, Add an ErrorEvent.Detail to the TestPlainSink_EmitsErrorEvent case and update the expected output to include the detail line(s) rendered by the plain sink; specifically, in the TestPlainSink_EmitsErrorEvent test where Emit(sink, ErrorEvent{...}) is called (and NewPlainSink, ErrorAction, out.String() are used), add a Detail field to the ErrorEvent and extend the expected string to include the plain-sink formatting for Detail so the test asserts title, summary, detail, and actions parity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/output/plain_sink_test.go`:
- Around line 172-184: Add an ErrorEvent.Detail to the
TestPlainSink_EmitsErrorEvent case and update the expected output to include the
detail line(s) rendered by the plain sink; specifically, in the
TestPlainSink_EmitsErrorEvent test where Emit(sink, ErrorEvent{...}) is called
(and NewPlainSink, ErrorAction, out.String() are used), add a Detail field to
the ErrorEvent and extend the expected string to include the plain-sink
formatting for Detail so the test asserts title, summary, detail, and actions
parity.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
go.modinternal/auth/auth.gointernal/auth/auth_test.gointernal/auth/login.gointernal/container/start.gointernal/output/events.gointernal/output/format.gointernal/output/format_test.gointernal/output/plain_sink_test.gointernal/output/tui_sink_test.gointernal/ui/app.gointernal/ui/app_test.gointernal/ui/components/error_display.gointernal/ui/components/error_display_test.gointernal/ui/components/message.gointernal/ui/components/spinner.gointernal/ui/components/spinner_test.gointernal/ui/styles/styles.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/output/tui_sink_test.go
- go.mod
- internal/auth/auth.go
- internal/ui/app_test.go
- internal/ui/components/spinner.go
- internal/auth/login.go
- internal/ui/components/error_display_test.go
- internal/ui/components/spinner_test.go
- internal/ui/components/error_display.go
silv-io
force-pushed
the
silv-io/flc-369
branch
from
7b4557c to
1154ca6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/output/events.go (1)
126-128: Add deprecation notice toEmitWarning.
EmitLoghas a deprecation comment directing toEmitInfo, butEmitWarninglacks similar guidance. Consider adding a deprecation notice or clarifying its status for consistency.+// Deprecated: Use Emit(sink, MessageEvent{Severity: SeverityWarning, Text: message}) for direct emission func EmitWarning(sink Sink, message string) { Emit(sink, MessageEvent{Severity: SeverityWarning, Text: message}) }Alternatively, if
EmitWarningis intended to remain as a convenience helper (not deprecated), that's fine—just ensure the intent is clear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/events.go` around lines 126 - 128, The EmitWarning helper lacks any deprecation or intent comment; update the declaration for EmitWarning to either add a deprecation doc comment pointing users to the preferred API (e.g., "Deprecated: use EmitInfo or Emit with SeverityWarning") or add a clear comment stating it is an intentional convenience helper that wraps Emit/MessageEvent with SeverityWarning; reference EmitWarning, Emit, MessageEvent, SeverityWarning and EmitInfo/EmitLog in the comment so readers know the recommended alternative or that it is intentionally retained.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 29: Move the module github.com/charmbracelet/bubbles from the indirect
list into the main require block of go.mod because it is directly imported (see
imports of github.com/charmbracelet/bubbles in your code). Edit the require
block to include github.com/charmbracelet/bubbles with the current version
(v1.0.0) and remove its indirect entry, then re-run go mod tidy to update the
module graph so imports in internal/ui (where bubbles is used) resolve as a
direct dependency.
---
Nitpick comments:
In `@internal/output/events.go`:
- Around line 126-128: The EmitWarning helper lacks any deprecation or intent
comment; update the declaration for EmitWarning to either add a deprecation doc
comment pointing users to the preferred API (e.g., "Deprecated: use EmitInfo or
Emit with SeverityWarning") or add a clear comment stating it is an intentional
convenience helper that wraps Emit/MessageEvent with SeverityWarning; reference
EmitWarning, Emit, MessageEvent, SeverityWarning and EmitInfo/EmitLog in the
comment so readers know the recommended alternative or that it is intentionally
retained.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
go.modinternal/auth/auth.gointernal/auth/auth_test.gointernal/auth/login.gointernal/container/start.gointernal/output/events.gointernal/output/format.gointernal/output/format_test.gointernal/output/plain_sink_test.gointernal/output/tui_sink_test.gointernal/ui/app.gointernal/ui/app_test.gointernal/ui/components/error_display.gointernal/ui/components/error_display_test.gointernal/ui/components/message.gointernal/ui/components/spinner.gointernal/ui/components/spinner_test.gointernal/ui/styles/styles.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/ui/components/error_display_test.go
- internal/output/tui_sink_test.go
- internal/auth/login.go
- internal/ui/styles/styles.go
- internal/ui/components/error_display.go
- internal/ui/components/spinner_test.go
- internal/output/format_test.go
- internal/container/start.go
- internal/ui/components/message.go
| github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect | ||
| github.com/aymanbagabas/go-udiff v0.3.1 // indirect | ||
| github.com/cespare/xxhash/v2 v2.3.0 // indirect | ||
| github.com/charmbracelet/bubbles v1.0.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether bubbles is directly imported by project source.
echo "=== Non-test files importing bubbles ==="
rg -n --type=go '"github.com/charmbracelet/bubbles' -g '!**/*_test.go'
echo ""
echo "=== Test files importing bubbles ==="
rg -n --type=go '"github.com/charmbracelet/bubbles' -g '**/*_test.go'Repository: localstack/lstk
Length of output: 367
🏁 Script executed:
#!/bin/bash
# Check the current go.mod structure for bubbles
echo "=== go.mod direct requires section ==="
head -40 go.mod | cat -n
echo ""
echo "=== Search for bubbles in entire go.mod ==="
rg -n 'charmbracelet/bubbles' go.modRepository: localstack/lstk
Length of output: 2097
Move bubbles to direct dependencies in the require block
github.com/charmbracelet/bubbles is directly imported in internal/ui/app.go:7 and internal/ui/components/spinner.go:4, so it should be listed as a direct dependency for clearer ownership and stable go mod tidy behavior. Move it from the indirect section to the main require block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 29, Move the module github.com/charmbracelet/bubbles from the
indirect list into the main require block of go.mod because it is directly
imported (see imports of github.com/charmbracelet/bubbles in your code). Edit
the require block to include github.com/charmbracelet/bubbles with the current
version (v1.0.0) and remove its indirect entry, then re-run go mod tidy to
update the module graph so imports in internal/ui (where bubbles is used)
resolve as a direct dependency.
carole-lavillonniere
approved these changes
Mar 2, 2026
Collaborator
carole-lavillonniere
left a comment
carole-lavillonniere
left a comment
There was a problem hiding this comment.
Added a few minor questions/suggestions but overall I approve the direction!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/output/plain_format.go (1)
105-109: Avoid unconditional space between error action label and value.If
ErrorAction.Valueis empty, this emits a trailing space. Consider appending the value conditionally.Suggested patch
for _, action := range e.Actions { sb.WriteString("\n → ") sb.WriteString(action.Label) - sb.WriteString(" ") - sb.WriteString(action.Value) + if action.Value != "" { + sb.WriteString(" ") + sb.WriteString(action.Value) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_format.go` around lines 105 - 109, The loop over e.Actions currently always writes a space before action.Value which produces a trailing space when action.Value is empty; update the loop that iterates e.Actions to only append the space and action.Value when action.Value is non-empty (e.g., check if action.Value != "" then sb.WriteString(" "); sb.WriteString(action.Value)), leaving sb.WriteString(action.Label) unchanged so labels are still printed without an extra trailing space.internal/output/plain_format_test.go (1)
14-37: Add one case for unknownMessageSeverityfallback.A guard case for an out-of-range severity would lock in current default behavior and catch future enum-expansion regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_format_test.go` around lines 14 - 37, Add a test case to the table-driven tests in plain_format_test.go to exercise an out-of-range MessageSeverity fallback: create a MessageEvent with a severity value cast from an invalid integer (e.g., MessageSeverity(999) or MessageSeverity(-1)) and Text set to something like "unknown", then assert the formatter still returns the current default output (plain text) and wantOK true. Reference the existing table entries using MessageEvent and the Severity constants (SeverityInfo, SeveritySuccess, SeverityNote, SeverityWarning) and add the new case alongside them so future enum expansions/regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/output/events.go`:
- Around line 109-123: Add back the deprecated EmitLog helper to restore
backwards compatibility: implement a function EmitLog(sink Sink, text string)
that simply calls Emit(sink, MessageEvent{Severity: SeverityLog, Text: text})
(matching the pattern of EmitInfo/EmitSuccess/EmitNote/EmitWarning) so existing
call sites compile; keep the name EmitLog and the same signature and mark it as
deprecated in comments if desired.
---
Nitpick comments:
In `@internal/output/plain_format_test.go`:
- Around line 14-37: Add a test case to the table-driven tests in
plain_format_test.go to exercise an out-of-range MessageSeverity fallback:
create a MessageEvent with a severity value cast from an invalid integer (e.g.,
MessageSeverity(999) or MessageSeverity(-1)) and Text set to something like
"unknown", then assert the formatter still returns the current default output
(plain text) and wantOK true. Reference the existing table entries using
MessageEvent and the Severity constants (SeverityInfo, SeveritySuccess,
SeverityNote, SeverityWarning) and add the new case alongside them so future
enum expansions/regressions are caught.
In `@internal/output/plain_format.go`:
- Around line 105-109: The loop over e.Actions currently always writes a space
before action.Value which produces a trailing space when action.Value is empty;
update the loop that iterates e.Actions to only append the space and
action.Value when action.Value is non-empty (e.g., check if action.Value != ""
then sb.WriteString(" "); sb.WriteString(action.Value)), leaving
sb.WriteString(action.Label) unchanged so labels are still printed without an
extra trailing space.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/output/events.gointernal/output/plain_format.gointernal/output/plain_format_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The current event system has fragmented types (
LogEvent,WarningEvent) that don't scale well as we add more UI feedback patterns. We also lack reusable TUI components for common patterns like spinners and structured error display.This PR introduces a unified event system that makes it easier to:
Changes
New event types in
internal/output/:MessageEventwith severity levels (Info, Success, Note, Warning) — replacesLogEvent/WarningEventSpinnerEventfor explicit spinner start/stop controlErrorEventfor structured errors with title, summary, detail, and suggested actionsNew TUI components in
internal/ui/components/:Spinner— wraps Bubble Tea's spinner with simple Start/Stop APIRenderMessage()— styled message rendering by severityErrorDisplay— rich error panel with actionsBackward compatible:
EmitLog()andEmitWarning()still work (marked deprecated, forward toMessageEvent).Tests
go test ./internal/output/... ./internal/ui/...Follow-ups
EmitSpinnerStart/EmitSpinnerStopinstartcommand during image pull (or introduce a loading bar -> we'll see what's better)EmitErrorfor Docker connection failures and auth errorsRelated