Conversation
📝 WalkthroughWalkthroughAdds a Bubble Tea-based interactive TUI and wiring: event formatting, a TUI sink, UI components and styles, runtime integration, and conditional CLI routing; also updates dependencies for terminal/Charmbracelet libs and adapts auth/start to accept an ENTER signal channel. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as cmd/root.go
participant UI as internal/ui/run.go
participant Tea as Bubble Tea Program
participant Container as container.Start
participant Sink as output.TUISink
participant App as ui.App
participant Formatter as output.FormatEventLine
User->>CLI: run "lstk start" (TTY)
CLI->>UI: ui.Run(ctx, runtime, version, platformClient)
UI->>Tea: start Bubble Tea program
UI->>Container: start runtime with TUISink (goroutine)
Container->>Sink: emit(Event)
Sink->>Tea: Send(Event)
Tea->>App: Update with Event
App->>Formatter: FormatEventLine(Event)
Formatter-->>App: (formatted, ok)
App->>Tea: Update view (append line)
Tea-->>User: Render UI
User->>Tea: press Ctrl+C
Tea->>UI: cancel()
UI->>Container: context cancelled
Container->>Tea: send runDoneMsg/runErrMsg
Tea-->>User: Exit
sequenceDiagram
participant Event
participant Formatter as output.FormatEventLine
participant Plain as PlainSink
participant TUI as TUISink
Event->>Formatter: format based on type
Formatter-->>Plain: (string, ok)
Formatter-->>TUI: (string, ok)
Plain->>Plain: print if ok
TUI->>TUI: forward Event to Bubble Tea Sender
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
🤖 Fix all issues with AI agents
In `@internal/output/tui_sink.go`:
- Around line 3-5: The comment on the Sender interface only states who
implements it but not the design intent; update the comment for the Sender
interface (and optionally its Send method) to explain the "why" — e.g., that
Sender decouples UI event emission from concrete bubbletea.Program
implementations to improve testability and allow injection of test doubles — so
readers understand the interface exists to enable loose coupling and easier
testing rather than just who implements it.
silv-io
force-pushed
the
silv-io/flc-328
branch
from
9b55778 to
f4ce0d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/auth/login.go`:
- Around line 123-141: The drainEnterSignal function can busy-loop if the input
channel is closed; update drainEnterSignal to perform a non-blocking receive
that checks the receive "ok" boolean (e.g., case _, ok := <-ch:) and return
immediately when ok is false (channel closed), otherwise continue draining any
queued values; keep using stdinEnterSignal as-is but ensure drainEnterSignal
distinguishes between a closed channel and an empty channel so it doesn't spin.
In `@internal/ui/app_test.go`:
- Around line 68-69: The test currently assigns the result of app.Update to
`model` and reassigns `app` (via model.(App)) but never uses it, causing SA4006;
change the call to discard both return values (e.g., call
app.Update(tea.KeyMsg{Type: tea.KeyEnter}) and ignore the returned model and
command) so only the `signals` counter is asserted, updating the line with the
`app.Update` invocation to not assign its results.
| func stdinEnterSignal() <-chan struct{} { | ||
| ch := make(chan struct{}, 1) | ||
| go func() { | ||
| reader := bufio.NewReader(os.Stdin) | ||
| _, _ = reader.ReadString('\n') | ||
| ch <- struct{}{} | ||
| }() | ||
| return ch | ||
| } | ||
|
|
||
| func drainEnterSignal(ch <-chan struct{}) { | ||
| for { | ||
| select { | ||
| case <-ch: | ||
| default: | ||
| return | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for enterSignal and drainEnterSignal usage patterns
echo "=== Searching for enterSignal usage ==="
rg -n --type=go 'enterSignal' -C 3
echo -e "\n=== Searching for drainEnterSignal calls ==="
rg -n --type=go 'drainEnterSignal' -C 3
echo -e "\n=== Searching for channel close operations ==="
rg -n --type=go 'close\(' -C 2 --type=goRepository: localstack/lstk
Length of output: 3270
🏁 Script executed:
#!/bin/bash
# Check the actual definition of drainEnterSignal to see current behavior
sed -n '133,141p' internal/auth/login.goRepository: localstack/lstk
Length of output: 163
Fix potential busy-loop when enterSignal is closed.
If the channel is closed (common for broadcast-style signals), drainEnterSignal will spin forever because receives are always ready on closed channels. Guard against closed channels to avoid a CPU loop.
🛠️ Suggested fix
func drainEnterSignal(ch <-chan struct{}) {
for {
select {
- case <-ch:
+ case _, ok := <-ch:
+ if !ok {
+ return
+ }
default:
return
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func stdinEnterSignal() <-chan struct{} { | |
| ch := make(chan struct{}, 1) | |
| go func() { | |
| reader := bufio.NewReader(os.Stdin) | |
| _, _ = reader.ReadString('\n') | |
| ch <- struct{}{} | |
| }() | |
| return ch | |
| } | |
| func drainEnterSignal(ch <-chan struct{}) { | |
| for { | |
| select { | |
| case <-ch: | |
| default: | |
| return | |
| } | |
| } | |
| } | |
| func stdinEnterSignal() <-chan struct{} { | |
| ch := make(chan struct{}, 1) | |
| go func() { | |
| reader := bufio.NewReader(os.Stdin) | |
| _, _ = reader.ReadString('\n') | |
| ch <- struct{}{} | |
| }() | |
| return ch | |
| } | |
| func drainEnterSignal(ch <-chan struct{}) { | |
| for { | |
| select { | |
| case _, ok := <-ch: | |
| if !ok { | |
| return | |
| } | |
| default: | |
| return | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@internal/auth/login.go` around lines 123 - 141, The drainEnterSignal function
can busy-loop if the input channel is closed; update drainEnterSignal to perform
a non-blocking receive that checks the receive "ok" boolean (e.g., case _, ok :=
<-ch:) and return immediately when ok is false (channel closed), otherwise
continue draining any queued values; keep using stdinEnterSignal as-is but
ensure drainEnterSignal distinguishes between a closed channel and an empty
channel so it doesn't spin.
| model, _ := app.Update(tea.KeyMsg{Type: tea.KeyEnter}) | ||
| app = model.(App) |
There was a problem hiding this comment.
Remove unused assignment to fix staticcheck SA4006.
The app variable assigned on line 69 is never used. Since the test only verifies the signals counter, both return values can be discarded.
🔧 Proposed fix
- model, _ := app.Update(tea.KeyMsg{Type: tea.KeyEnter})
- app = model.(App)
+ _, _ = app.Update(tea.KeyMsg{Type: tea.KeyEnter})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| model, _ := app.Update(tea.KeyMsg{Type: tea.KeyEnter}) | |
| app = model.(App) | |
| _, _ = app.Update(tea.KeyMsg{Type: tea.KeyEnter}) |
🧰 Tools
🪛 GitHub Check: Lint
[failure] 69-69:
SA4006: this value of app is never used (staticcheck)
🤖 Prompt for AI Agents
In `@internal/ui/app_test.go` around lines 68 - 69, The test currently assigns the
result of app.Update to `model` and reassigns `app` (via model.(App)) but never
uses it, causing SA4006; change the call to discard both return values (e.g.,
call app.Update(tea.KeyMsg{Type: tea.KeyEnter}) and ignore the returned model
and command) so only the `signals` counter is asserted, updating the line with
the `app.Update` invocation to not assign its results.
| sink output.Sink | ||
| } | ||
|
|
||
| func New(sink output.Sink, platformClient api.PlatformAPI) (*Auth, error) { |
There was a problem hiding this comment.
nit: This is not used anymore, let's remove it?
| if enterCh == nil { | ||
| enterCh = stdinEnterSignal() | ||
| } else { | ||
| drainEnterSignal(enterCh) |
There was a problem hiding this comment.
nit: can we add a comment to explain why drainEnterSignal is needed?
There was a problem hiding this comment.
I think I'll remove that whole part so that we don't take inputs in the internal code anymore but have this all just work through public function calls
| return "\n" + nimboLine1() + " " + title + "\n" + | ||
| nimboLine2() + " " + version + "\n" + | ||
| nimboLine3() + "\n" | ||
| } |
There was a problem hiding this comment.
suggestion: can we use lipgloss.JoinHorizontal to join the logo and text columns side by side, instead of building each line manually? Something like:
func (h Header) View() string {
logo := lipgloss.JoinVertical(lipgloss.Left,
nimboLine1(),
nimboLine2(),
nimboLine3(),
)
text := lipgloss.JoinVertical(lipgloss.Left,
styles.Title.Render("LocalStack (lstk)"),
styles.Version.Render(h.version),
"",
)
return "\n" + lipgloss.JoinHorizontal(lipgloss.Top, logo, " ", text) + "\n"
}
There was a problem hiding this comment.
you're right! I had it like this before even before I redid it to work with the event sink system
| if NimboLightColor != "#7E88EC" { | ||
| t.Fatalf("unexpected NimboLightColor: %s", NimboLightColor) | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion: what about testing the exact value of the rendered header (strings and colors) instead of tests TestNimboColorConstants and TestHeaderViewIncludesNimboAndVersion?
We could use https://pkg.go.dev/gotest.tools/v3/golden to write it to a golden file.
A failing test would show an exact diff and it would cover the full rendered layout not just substring presence.
2 participants
Motivation
We’ve already merged the generic event/sink foundation, so this PR builds a minimal TUI on top of that architecture. The goal is to keep output routing consistent across TTY and non-TTY modes, preserve the Nimbo header branding, and make future feature emitters plug into the same typed event pipeline.
Changes
TUISink) that forwards events viaProgram.Send().TUISink,PlainSink.This is how it looks like now:
We'll continuously update this to contain all the content we want in the header and also to not just print out line after line after the header. This is a first iteration :)
Tests
Added new tests for TUISink, styles, header component and app