Improve event system structure and add reusable TUI components#50
Open
Improve event system structure and add reusable TUI components#50
Conversation
📝 WalkthroughWalkthroughReplaces LogEvent/WarningEvent with MessageEvent (with severities), adds SpinnerEvent and ErrorEvent, updates formatters, sinks, tests, and auth/container emitters, and introduces Spinner and ErrorDisplay UI components and related styles; also bumps charmbracelet deps in go.mod. Changes
Sequence DiagramsequenceDiagram
participant Emitter as Event Emitter
participant Sink as Event Sink
participant Formatter as Formatter
participant UIApp as UI App
participant Component as UI Component
Emitter->>Sink: EmitInfo/EmitSuccess/EmitNote/EmitWarning/EmitError/EmitSpinnerStart/Stop
Sink->>Formatter: deliver Event (MessageEvent/SpinnerEvent/ErrorEvent)
Formatter->>Formatter: formatMessageEvent() / formatErrorEvent()
Formatter->>UIApp: forward formatted event
UIApp->>UIApp: Update() processes event
alt SpinnerEvent
UIApp->>Component: spinner.Start(text) / spinner.Stop()
else ErrorEvent
UIApp->>Component: errorDisplay.Show(event)
UIApp->>Component: spinner.Stop()
else MessageEvent
UIApp->>UIApp: append formatted line to history
end
UIApp->>Component: View()
Component->>UIApp: rendered string
UIApp->>UIApp: compose final output
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
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
1 participant
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