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:
📝 WalkthroughWalkthroughAdds a Bubble Tea TUI and event-driven I/O: new UserInputRequest event types and emitters, centralized event formatting, plain/TUI sinks, UI components/styles, interactive run/login flows, auth token storage and allowLogin flag, CLI routing to launch the TUI on TTY, tests, and dependency updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as "cmd/root.go"
participant UI as "internal/ui/run.go"
participant Program as "Bubble Tea Program"
participant Container as "container.Start"
participant TUISink as "internal/output.TUISink"
participant App as "internal/ui.App"
participant Formatter as "internal/output.FormatEventLine"
User->>CLI: run command (TTY)
CLI->>UI: ui.Run(parentCtx, rt, version, platformClient)
UI->>Program: start Bubble Tea program (goroutine)
UI->>Container: start runtime with TUISink (goroutine)
Container->>TUISink: emit(Event)
TUISink->>Program: Send(Event)
Program->>App: Update with Event
App->>Formatter: FormatEventLine(Event)
Formatter-->>App: (formatted, ok)
App->>Program: update view (append line)
Program-->>User: render UI
User->>Program: input (Enter / Ctrl+C / q)
Program->>UI: send input response or runErr/runDone
UI->>Container: cancel or continue
Container->>Program: send completion/error
Program-->>User: Exit
sequenceDiagram
autonumber
participant Event as "Event"
participant Formatter as "FormatEventLine"
participant Plain as "PlainSink"
participant TUI as "TUISink"
participant Program as "Bubble Tea Program"
Event->>Formatter: format by type
Formatter-->>Plain: (string, ok)
Plain->>Plain: print if ok
Event->>TUI: forward Event
TUI->>Program: Send(Event)
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)
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.
| if NimboLightColor != "#7E88EC" { | ||
| t.Fatalf("unexpected NimboLightColor: %s", NimboLightColor) | ||
| } | ||
| } |
Collaborator
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.
Member
Author
There was a problem hiding this comment.
Done, not sure how the diffs will look like with the ANSI colors enabled though :D
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/output/events.go (1)
43-57: Consider addingUserInputRequestEventto theEventunion and reuseEmit.
This keeps the “type‑safe Emit” story consistent and avoids duplicating nil checks.♻️ Suggested refactor
type Event interface { - LogEvent | WarningEvent | ContainerStatusEvent | ProgressEvent + LogEvent | WarningEvent | ContainerStatusEvent | ProgressEvent | UserInputRequestEvent } func EmitUserInputRequest(sink Sink, event UserInputRequestEvent) { - if sink == nil { - return - } - sink.emit(event) + Emit(sink, event) }Also applies to: 89-94
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/events.go` around lines 43 - 57, Add the new UserInputRequestEvent type into the Event union/type that other events use (alongside existing event types) and replace direct uses that manually send on ResponseCh with a call to the existing Emit mechanism so nil-checks and routing are centralized; specifically, update the Event union/type to include UserInputRequestEvent, then where code currently constructs a UserInputRequestEvent and writes to ResponseCh (or does manual nil checks around it) switch to calling Emit(ctxOrBus, UserInputRequestEvent{Prompt:..., Options:..., ResponseCh:...}) so the existing Emit handling will route and guard nils consistently. Ensure types InputOption and InputResponse are left as-is and that any switch/case handling for Event in the event dispatcher is updated to handle the new UserInputRequestEvent branch.
🤖 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/events.go`:
- Around line 43-57: Add the new UserInputRequestEvent type into the Event
union/type that other events use (alongside existing event types) and replace
direct uses that manually send on ResponseCh with a call to the existing Emit
mechanism so nil-checks and routing are centralized; specifically, update the
Event union/type to include UserInputRequestEvent, then where code currently
constructs a UserInputRequestEvent and writes to ResponseCh (or does manual nil
checks around it) switch to calling Emit(ctxOrBus,
UserInputRequestEvent{Prompt:..., Options:..., ResponseCh:...}) so the existing
Emit handling will route and guard nils consistently. Ensure types InputOption
and InputResponse are left as-is and that any switch/case handling for Event in
the event dispatcher is updated to handle the new UserInputRequestEvent branch.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/ui/components/header_test.go (1)
11-15: Test structure looks good; consider cleanup for parallel-safety.The golden file approach is appropriate for UI rendering tests. One minor observation:
lipgloss.SetColorProfilemodifies global state without restoring it afterward. This works fine sincet.Parallel()isn't used, but could cause flakiness if tests are parallelized in the future.♻️ Optional: Add cleanup to restore original profile
func TestHeaderView(t *testing.T) { + original := lipgloss.DefaultRenderer().ColorProfile() lipgloss.SetColorProfile(termenv.TrueColor) + t.Cleanup(func() { lipgloss.SetColorProfile(original) }) view := NewHeader("v1.0.0").View() golden.Assert(t, view, "header.golden") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/components/header_test.go` around lines 11 - 15, TestHeaderView sets global state via lipgloss.SetColorProfile which can affect other tests; capture the current profile before calling lipgloss.SetColorProfile and register a t.Cleanup callback to restore it (e.g., original := /* capture current profile */; lipgloss.SetColorProfile(termenv.TrueColor); t.Cleanup(func(){ lipgloss.SetColorProfile(original) })) so TestHeaderView (and the NewHeader/View logic) is safe to run in parallel.
🤖 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/ui/components/header_test.go`:
- Around line 11-15: TestHeaderView sets global state via
lipgloss.SetColorProfile which can affect other tests; capture the current
profile before calling lipgloss.SetColorProfile and register a t.Cleanup
callback to restore it (e.g., original := /* capture current profile */;
lipgloss.SetColorProfile(termenv.TrueColor); t.Cleanup(func(){
lipgloss.SetColorProfile(original) })) so TestHeaderView (and the NewHeader/View
logic) is safe to run in parallel.
2cfe9ea to
cf51ff9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
internal/ui/styles/styles_test.go (1)
5-16: Confirm unit-test necessity vs integration coverage.This pins exact constants with a unit test; if an integration/golden UI test can cover this, consider moving it there or briefly justify why a unit test is the only practical option.
As per coding guidelines: "Prefer integration tests to cover most cases; use unit tests only when integration tests are not practical."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/styles/styles_test.go` around lines 5 - 16, The TestNimboColorConstants unit test is pinning exact color constants (NimboDarkColor, NimboMidColor, NimboLightColor); either replace it with an integration/golden UI test that validates rendered colors in the UI, or keep the unit test but add a brief justification comment above TestNimboColorConstants explaining why a unit test is required (e.g., these constants are public API used by multiple packages and cannot be reliably asserted in an integration test). Update TestNimboColorConstants accordingly and ensure the justification references the constant names NimboDarkColor, NimboMidColor, and NimboLightColor so reviewers can see why the unit test is retained.internal/output/tui_sink_test.go (1)
16-35: Consider whether this belongs in integration coverage.This is a unit test for sink forwarding; if an integration test can exercise the same behavior, prefer that or justify why unit coverage is necessary.
As per coding guidelines: "Prefer integration tests to cover most cases; use unit tests only when integration tests are not practical."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/tui_sink_test.go` around lines 16 - 35, TestTUISinkForwardsEvents is flagged as a unit test that may belong in integration coverage; either move or justify it: either remove this unit test and add an equivalent integration test that exercises NewTUISink and Emit via the real TUI pipeline, or keep the unit test but add a short justification comment above TestTUISinkForwardsEvents explaining why an isolated unit test using testSender and direct calls to Emit/NewTUISink is necessary (e.g., integration is flaky or too slow) and reference the symbols NewTUISink, Emit, testSender, and TestTUISinkForwardsEvents so reviewers can find the rationale.internal/auth/login.go (1)
98-106: Remove self-explanatory comments or explain the “why.”The added comments just restate the code flow; please drop them unless there’s a non-obvious rationale to capture.
As per coding guidelines: "Avoid adding 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/auth/login.go` around lines 98 - 106, Remove the self-explanatory comment lines around the user input emission and wait logic; specifically delete the comment "// Emit user input request event" before the creation of responseCh and output.EmitUserInputRequest(...) and the comment "// Wait for either browser callback, user response, or context cancellation" that follows, unless you add a short "why" comment explaining non-obvious behavior (e.g., that the flow blocks here to await an out-of-band browser OAuth callback or user-confirmation to continue). Keep the code using responseCh and output.EmitUserInputRequest(UserInputRequestEvent{...}) unchanged.internal/output/format_test.go (1)
5-76: Consider integration coverage for formatting behavior.Since this is unit-level formatting coverage, please confirm an integration/golden test isn’t practical, or move coverage up if possible.
As per coding guidelines: "Prefer integration tests to cover most cases; use unit tests only when integration tests are not practical."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/format_test.go` around lines 5 - 76, TestFormatEventLine currently provides only unit coverage for FormatEventLine and the various event structs (LogEvent, WarningEvent, ContainerStatusEvent, ProgressEvent); either replace or supplement it with an integration/golden test that exercises the actual output path (produce real container/pull events, capture output and compare against a checked-in golden file) or explicitly document why an integration/golden test is impractical. To fix: decide which approach to take, then (A) implement an integration test that triggers the real flow and validates output against a golden fixture (move test out of internal unit tests into the integration test package and name it to reflect golden comparison), or (B) keep TestFormatEventLine but add a brief comment above the test explaining why integration/golden testing cannot be used here and reference FormatEventLine and the event structs, so reviewers know the limitation was considered.internal/ui/run_login_test.go (1)
249-253: Port hardcoding and timing-based synchronization create flakiness and conflict risk.The callback server port is hardcoded to
45678ininternal/auth/login.go:18, and the test usestime.Sleep(100 * time.Millisecond)before invoking the callback. This approach has two issues:
- Flakiness: The sleep duration is arbitrary and doesn't guarantee the callback server is ready, especially under load.
- Port conflicts: The hardcoded port can be occupied by other services or parallel test runs, causing failures.
Make the port dynamic by using
net.Listen("tcp", ":0")to auto-assign an available port and pass the actual port to the browser callback, or implement a proper wait mechanism (e.g., retry loop or channel-based synchronization) instead of a fixed sleep.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/run_login_test.go` around lines 249 - 253, The test is flaky because it sleeps and hits a hardcoded callback port; change the callback server in the auth code (where the server is started in internal/auth/login.go) to bind to an ephemeral port using net.Listen("tcp", ":0") and expose the actual port (or full callback URL) to the caller, then update the test in internal/ui/run_login_test.go to use that returned port/URL instead of 45678 and replace time.Sleep with a deterministic readiness signal (e.g., return when the listener is accepted, use a channel the server closes when ready, or poll with a short retry loop until the endpoint responds) so the test reliably waits for the callback server before issuing the http.Get.
🤖 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/ui/app_test.go`:
- Around line 12-28: The reviewer asks for an integration-level test or
justification — add either an end-to-end integration test that exercises the
same UI flow instead of (or in addition to) TestAppAddsFormattedLinesInOrder:
instantiate the real application via NewApp, drive it with real events (use
output.LogEvent and output.WarningEvent as in the unit test), render via
App.View, and assert ordering/formatting; if an integration test is impractical,
add a brief comment above TestAppAddsFormattedLinesInOrder explaining why
unit-level coverage is necessary (e.g., heavy UI dependencies, flakiness, or
cost of end-to-end environment) and reference the functions App.Update and
App.View used so future readers know the scope.
In `@internal/ui/app.go`:
- Around line 43-81: The Update method currently sends to
pendingInput.ResponseCh synchronously which can block the Bubble Tea event loop;
change both the cancel/q and Enter handling in App.Update to not write directly
to ResponseCh but instead return a tea.Cmd that sends the response
asynchronously (spawn a goroutine or use tea.Batch/tea.Cmd) after guarding that
pendingInput and pendingInput.ResponseCh are non-nil, then in Update set
a.pendingInput = nil and a.inputPrompt = a.inputPrompt.Hide() immediately and
perform any cancel() and a.err = context.Canceled synchronously; update the
ctrl+c/q branch and the KeyEnter branch to construct and return the non-blocking
tea.Cmd that delivers output.InputResponse (Cancelled:true or SelectedKey) so
Update remains non-blocking while still clearing state.
In `@internal/ui/run.go`:
- Around line 35-46: The goroutine launching container.Start may exit without
writing to runErrCh (e.g., panic or early return), causing the reader on
runErrCh to block; ensure the channel always receives by writing the final error
value in a defer: add defer func(){ select{ case runErrCh <- errVal: default: }
}() or simply capture err into a local variable and defer a send to runErrCh at
the top of the goroutine, and also recover from panics (recover() then send a
wrapped error) so container.Start, runErrCh, runErrMsg, runDoneMsg and p.Send
paths always send a terminal value and avoid deadlock.
---
Nitpick comments:
In `@internal/auth/login.go`:
- Around line 98-106: Remove the self-explanatory comment lines around the user
input emission and wait logic; specifically delete the comment "// Emit user
input request event" before the creation of responseCh and
output.EmitUserInputRequest(...) and the comment "// Wait for either browser
callback, user response, or context cancellation" that follows, unless you add a
short "why" comment explaining non-obvious behavior (e.g., that the flow blocks
here to await an out-of-band browser OAuth callback or user-confirmation to
continue). Keep the code using responseCh and
output.EmitUserInputRequest(UserInputRequestEvent{...}) unchanged.
In `@internal/output/format_test.go`:
- Around line 5-76: TestFormatEventLine currently provides only unit coverage
for FormatEventLine and the various event structs (LogEvent, WarningEvent,
ContainerStatusEvent, ProgressEvent); either replace or supplement it with an
integration/golden test that exercises the actual output path (produce real
container/pull events, capture output and compare against a checked-in golden
file) or explicitly document why an integration/golden test is impractical. To
fix: decide which approach to take, then (A) implement an integration test that
triggers the real flow and validates output against a golden fixture (move test
out of internal unit tests into the integration test package and name it to
reflect golden comparison), or (B) keep TestFormatEventLine but add a brief
comment above the test explaining why integration/golden testing cannot be used
here and reference FormatEventLine and the event structs, so reviewers know the
limitation was considered.
In `@internal/output/tui_sink_test.go`:
- Around line 16-35: TestTUISinkForwardsEvents is flagged as a unit test that
may belong in integration coverage; either move or justify it: either remove
this unit test and add an equivalent integration test that exercises NewTUISink
and Emit via the real TUI pipeline, or keep the unit test but add a short
justification comment above TestTUISinkForwardsEvents explaining why an isolated
unit test using testSender and direct calls to Emit/NewTUISink is necessary
(e.g., integration is flaky or too slow) and reference the symbols NewTUISink,
Emit, testSender, and TestTUISinkForwardsEvents so reviewers can find the
rationale.
In `@internal/ui/run_login_test.go`:
- Around line 249-253: The test is flaky because it sleeps and hits a hardcoded
callback port; change the callback server in the auth code (where the server is
started in internal/auth/login.go) to bind to an ephemeral port using
net.Listen("tcp", ":0") and expose the actual port (or full callback URL) to the
caller, then update the test in internal/ui/run_login_test.go to use that
returned port/URL instead of 45678 and replace time.Sleep with a deterministic
readiness signal (e.g., return when the listener is accepted, use a channel the
server closes when ready, or poll with a short retry loop until the endpoint
responds) so the test reliably waits for the callback server before issuing the
http.Get.
In `@internal/ui/styles/styles_test.go`:
- Around line 5-16: The TestNimboColorConstants unit test is pinning exact color
constants (NimboDarkColor, NimboMidColor, NimboLightColor); either replace it
with an integration/golden UI test that validates rendered colors in the UI, or
keep the unit test but add a brief justification comment above
TestNimboColorConstants explaining why a unit test is required (e.g., these
constants are public API used by multiple packages and cannot be reliably
asserted in an integration test). Update TestNimboColorConstants accordingly and
ensure the justification references the constant names NimboDarkColor,
NimboMidColor, and NimboLightColor so reviewers can see why the unit test is
retained.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
internal/ui/run.go (1)
35-46: Consider panic recovery to prevent potential deadlock.If
container.Startpanics, the channel write at line 37 never executes, and line 57 blocks forever. Usingdeferwith panic recovery would make this more robust.🛡️ Proposed defensive fix
go func() { + defer func() { + if r := recover(); r != nil { + runErrCh <- fmt.Errorf("panic in container.Start: %v", r) + } + }() err := container.Start(ctx, rt, output.NewTUISink(programSender{p: p}), platformClient, true) runErrCh <- errThis requires adding
"fmt"to imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/run.go` around lines 35 - 46, The goroutine launching container.Start may panic and never send to runErrCh, causing the receiver to block; wrap the goroutine body with a defer that recovers panics and always sends an error (or a wrapped fmt.Errorf containing the panic) into runErrCh before returning. Specifically, in the anonymous func that calls container.Start(ctx, rt, output.NewTUISink(programSender{p: p}), platformClient, true), add a defer which checks recover(), converts a non-nil panic into an error (using fmt.Errorf) and sends it on runErrCh, then ensure normal errors from container.Start are still sent as before; add the "fmt" import for constructing the panic error message.internal/ui/run_login_test.go (1)
249-253: Potential test flakiness with hardcoded callback port.The test relies on the hardcoded
127.0.0.1:45678port fromloginCallbackURL. If this port is occupied or if tests run in parallel, the test may fail intermittently. Consider adding a retry or skip condition if the callback server fails to start.Additionally, the
time.Sleep(100 * time.Millisecond)is a timing-based synchronization that could be fragile on slow CI systems. A polling approach waiting for the server to be ready would be more robust, though acceptable for now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/run_login_test.go` around lines 249 - 253, The test is fragile because it uses a hardcoded callback port and a fixed sleep; update the test in run_login_test.go to (1) allocate a free port dynamically (e.g., bind to ":0" or use httptest.Server) instead of assuming 127.0.0.1:45678 (replace any use of loginCallbackURL literal), and (2) replace time.Sleep(100 * time.Millisecond) with a small polling/retry loop that checks server readiness (e.g., attempt a TCP dial or HTTP GET to the callback endpoint with short backoff and a timeout) before proceeding; also make the final http.Get call resilient by retrying a few times and failing only after the timeout.go.mod (1)
9-9: Consider the stability implications of using an experimental package.The
x/exp/teatestdependency is from an experimental namespace and uses a pseudo-version (commit hash) rather than a tagged release. While this is acceptable for a testing-only dependency, experimental packages may have unstable APIs that could change without notice.Consider monitoring for stable releases of
teatestin the future and migrating once available. For now, pinning to a specific commit provides reproducibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 9, The go.mod currently depends on the experimental package github.com/charmbracelet/x/exp/teatest using a pseudo-version; explicitly pin this testing-only dependency to the specific pseudo-version/commit (leave the current pseudo-version or update to the exact commit hash) and add a short comment in go.mod noting this is an experimental testing dependency to monitor for a stable tagged release; ensure any change is committed so the go.sum/go.mod reproducibly capture that exact version and consider adding a TODO to migrate to the stable release when one appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@go.mod`:
- Line 9: The go.mod currently depends on the experimental package
github.com/charmbracelet/x/exp/teatest using a pseudo-version; explicitly pin
this testing-only dependency to the specific pseudo-version/commit (leave the
current pseudo-version or update to the exact commit hash) and add a short
comment in go.mod noting this is an experimental testing dependency to monitor
for a stable tagged release; ensure any change is committed so the go.sum/go.mod
reproducibly capture that exact version and consider adding a TODO to migrate to
the stable release when one appears.
In `@internal/ui/run_login_test.go`:
- Around line 249-253: The test is fragile because it uses a hardcoded callback
port and a fixed sleep; update the test in run_login_test.go to (1) allocate a
free port dynamically (e.g., bind to ":0" or use httptest.Server) instead of
assuming 127.0.0.1:45678 (replace any use of loginCallbackURL literal), and (2)
replace time.Sleep(100 * time.Millisecond) with a small polling/retry loop that
checks server readiness (e.g., attempt a TCP dial or HTTP GET to the callback
endpoint with short backoff and a timeout) before proceeding; also make the
final http.Get call resilient by retrying a few times and failing only after the
timeout.
In `@internal/ui/run.go`:
- Around line 35-46: The goroutine launching container.Start may panic and never
send to runErrCh, causing the receiver to block; wrap the goroutine body with a
defer that recovers panics and always sends an error (or a wrapped fmt.Errorf
containing the panic) into runErrCh before returning. Specifically, in the
anonymous func that calls container.Start(ctx, rt,
output.NewTUISink(programSender{p: p}), platformClient, true), add a defer which
checks recover(), converts a non-nil panic into an error (using fmt.Errorf) and
sends it on runErrCh, then ensure normal errors from container.Start are still
sent as before; add the "fmt" import for constructing the panic error message.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/auth/auth.go (1)
28-43: Consider validating required config fields.The constructor handles the optional
TokenStoragewell, butSinkandPlatformare required for proper operation. If either is nil, callers will get a runtime panic whenGetTokenis invoked.🛡️ Optional: Add nil checks for required fields
func New(cfg AuthConfig) (*Auth, error) { + if cfg.Sink == nil { + return nil, fmt.Errorf("Sink is required") + } + if cfg.Platform == nil { + return nil, fmt.Errorf("Platform is required") + } storage := cfg.TokenStorage if storage == nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/auth.go` around lines 28 - 43, The New constructor currently skips validating required AuthConfig fields which can cause panics later (e.g., in GetToken); add nil checks for cfg.Sink and cfg.Platform at the start of New and return an error if either is nil, so callers get a clear failure instead of runtime panics; update the function that constructs browserLogin (newBrowserLogin(cfg.Sink, cfg.Platform)) only after validation and reference AuthConfig, New, newBrowserLogin and GetToken in your checks.internal/ui/run_login_test.go (1)
245-252: Reduce test flakiness by avoiding fixed sleep.The fixed
time.Sleep(100ms)can be brittle on slower CI. Prefer polling the callback endpoint until it’s ready.♻️ Suggested refactor
- time.Sleep(100 * time.Millisecond) - - resp, err := http.Get("http://127.0.0.1:45678/auth/success?token=browser-token") - require.NoError(t, err) - require.NoError(t, resp.Body.Close()) + require.Eventually(t, func() bool { + resp, err := http.Get("http://127.0.0.1:45678/auth/success?token=browser-token") + if err != nil { + return false + } + _ = resp.Body.Close() + return true + }, 2*time.Second, 50*time.Millisecond)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/run_login_test.go` around lines 245 - 252, Replace the brittle fixed time.Sleep(100 * time.Millisecond) by polling the callback endpoint until it responds; after teatest.WaitFor(... tm.Output()) returns, repeatedly attempt http.Get("http://127.0.0.1:45678/auth/success?token=browser-token") (or use teatest.WaitFor/require.Eventually helpers) with short intervals and a total timeout (e.g. 5s) and break when err==nil and resp.StatusCode indicates success; use the existing resp/err variables and assert require.NoError/require.Equal once a successful response is received to avoid flaky timing-dependent failures.
🤖 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/auth.go`:
- Around line 28-43: The New constructor currently skips validating required
AuthConfig fields which can cause panics later (e.g., in GetToken); add nil
checks for cfg.Sink and cfg.Platform at the start of New and return an error if
either is nil, so callers get a clear failure instead of runtime panics; update
the function that constructs browserLogin (newBrowserLogin(cfg.Sink,
cfg.Platform)) only after validation and reference AuthConfig, New,
newBrowserLogin and GetToken in your checks.
In `@internal/ui/run_login_test.go`:
- Around line 245-252: Replace the brittle fixed time.Sleep(100 *
time.Millisecond) by polling the callback endpoint until it responds; after
teatest.WaitFor(... tm.Output()) returns, repeatedly attempt
http.Get("http://127.0.0.1:45678/auth/success?token=browser-token") (or use
teatest.WaitFor/require.Eventually helpers) with short intervals and a total
timeout (e.g. 5s) and break when err==nil and resp.StatusCode indicates success;
use the existing resp/err variables and assert require.NoError/require.Equal
once a successful response is received to avoid flaky timing-dependent failures.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/ui/app.go (1)
75-77: Consider canceling an in-flight input request before overwriting.If multiple input requests arrive, the previous
ResponseChcan be left hanging. Canceling it before replacing makes the UI more robust.♻️ Suggested adjustment
case output.UserInputRequestEvent: - a.pendingInput = &msg - a.inputPrompt = a.inputPrompt.Show(msg.Prompt, msg.Options) + var cancelCmd tea.Cmd + if a.pendingInput != nil { + cancelCmd = sendInputResponseCmd(a.pendingInput.ResponseCh, output.InputResponse{Cancelled: true}) + } + a.pendingInput = &msg + a.inputPrompt = a.inputPrompt.Show(msg.Prompt, msg.Options) + return a, cancelCmd🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/app.go` around lines 75 - 77, When handling UserInputRequestEvent, cancel any in-flight pending request before overwriting a.pendingInput: if a.pendingInput != nil and its ResponseCh (msg.ResponseCh / a.pendingInput.ResponseCh) is non-nil, send a cancellation signal (or close the channel) to unblock the waiter, then set a.pendingInput = &msg and update the prompt with a.inputPrompt = a.inputPrompt.Show(msg.Prompt, msg.Options); ensure you perform the cancel step before replacing a.pendingInput to avoid leaving the previous ResponseCh hanging.
🤖 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/ui/app.go`:
- Around line 75-77: When handling UserInputRequestEvent, cancel any in-flight
pending request before overwriting a.pendingInput: if a.pendingInput != nil and
its ResponseCh (msg.ResponseCh / a.pendingInput.ResponseCh) is non-nil, send a
cancellation signal (or close the channel) to unblock the waiter, then set
a.pendingInput = &msg and update the prompt with a.inputPrompt =
a.inputPrompt.Show(msg.Prompt, msg.Options); ensure you perform the cancel step
before replacing a.pendingInput to avoid leaving the previous ResponseCh
hanging.
cd33a93 to
437c935
Compare
# Conflicts: # cmd/root.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
silv-io
force-pushed
the
silv-io/flc-328
branch
from
437c935 to
5e26acb
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/integration/login_browser_flow_test.go (1)
3-84:⚠️ Potential issue | 🟠 MajorGuard concurrent access to the shared output buffer.
io.Copywrites tooutputwhilerequire.Eventuallyreadsoutput.Bytes()in another goroutine.bytes.Bufferisn’t concurrency‑safe, so this can race or flake (and will fail under-race). Add a mutex or a channel-based reader.🔧 Example fix with a locked writer
import ( "bytes" "context" "encoding/json" "io" "net/http" "net/http/httptest" "os/exec" "runtime" + "sync" "testing" "time" "github.com/creack/pty" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) + +type lockedBuffer struct { + mu *sync.Mutex + buf *bytes.Buffer +} + +func (l *lockedBuffer) Write(p []byte) (int, error) { + l.mu.Lock() + defer l.mu.Unlock() + return l.buf.Write(p) +} func TestBrowserFlowStoresToken(t *testing.T) { @@ - output := &bytes.Buffer{} + output := &bytes.Buffer{} + var outputMu sync.Mutex outputCh := make(chan struct{}) go func() { - _, _ = io.Copy(output, ptmx) + _, _ = io.Copy(&lockedBuffer{mu: &outputMu, buf: output}, ptmx) close(outputCh) }() // Wait for verification code to appear (callback server should be ready) require.Eventually(t, func() bool { - return bytes.Contains(output.Bytes(), []byte("TEST123")) + outputMu.Lock() + defer outputMu.Unlock() + return bytes.Contains(output.Bytes(), []byte("TEST123")) }, 10*time.Second, 100*time.Millisecond, "verification code should appear") @@ - out := output.String() + outputMu.Lock() + out := output.String() + outputMu.Unlock()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/login_browser_flow_test.go` around lines 3 - 84, The test TestBrowserFlowStoresToken mutates and reads the shared bytes.Buffer output concurrently (io.Copy in the PTY goroutine vs require.Eventually reading output.Bytes()), causing race conditions; fix by making access to output thread-safe: add a sync.Mutex (e.g., outputMu) and lock around all writes and reads to output (wrap the io.Copy target with a small writer that locks on Write, and in the require.Eventually callback lock, copy output.Bytes() into a local slice, then unlock before checking for "TEST123"), or alternatively replace output with a channel-based reader that serializes writes and exposes a safe snapshot method; ensure all references to output in TestBrowserFlowStoresToken use the guarded access.
🧹 Nitpick comments (1)
internal/ui/run_login_test.go (1)
34-76: Consider extracting shared mock server logic.This
createMockAPIServerfunction closely mirrors the one intest/integration/login_device_flow_test.go. If both implementations need to stay in sync, consider extracting to a shared test helper package (e.g.,internal/testutil) to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/run_login_test.go` around lines 34 - 76, The createMockAPIServer function is duplicated; extract it into a shared test helper (e.g., package internal/testutil) as an exported helper (e.g., NewMockAPIServer or CreateMockAPIServer) that accepts t *testing.T plus the same configurable parameters (licenseToken, confirmed) and preserves the same handler behavior (auth request/exchange, license endpoints); then replace the local createMockAPIServer implementations in internal/ui/run_login_test.go and test/integration/login_device_flow_test.go with calls to the new shared helper so both tests use the single canonical implementation.
🤖 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/auth/auth.go`:
- Around line 30-31: The comment above GetToken in internal/auth/auth.go is
redundant because the method body already shows the token retrieval order;
remove that self‑explanatory TODO/comment and keep only rationale comments if
needed—edit the comment block immediately preceding the func (a *Auth)
GetToken(ctx context.Context) declaration to delete the line describing the
retrieval order, leaving any non‑redundant explanatory notes for the Auth type
or GetToken implementation.
In `@internal/ui/run_login_test.go`:
- Around line 217-221: Replace the brittle sleep and hardcoded port by making
startCallbackServer return the actual listening address (or accept a
net.Listener) and bind to port 0 so the OS picks an available port; then in the
test (run_login_test.go) poll the server URL (built from the returned address,
e.g. returnedAddr + "/auth/success?token=browser-token") with a short retry loop
and overall timeout instead of time.Sleep(100 * time.Millisecond), retrying
http.Get until it succeeds or the timeout elapses; update any other tests
referencing port 45678 (and the startCallbackServer signature) to use the
returned address rather than the hardcoded port.
In `@test/integration/login_device_flow_test.go`:
- Around line 161-171: The test uses a plain bytes.Buffer (output) concurrently:
a goroutine writes to it via io.Copy(ptmx -> output) while the main goroutine
reads output.Bytes() inside require.Eventually, causing a data race; replace the
bytes.Buffer with the same synchronized buffer implementation used in
TestDeviceFlowSuccess (a thread-safe wrapper that implements io.Writer and a
locked Bytes() accessor), update the output variable to that safe type, and
ensure the goroutine still closes outputCh after io.Copy; read from the buffer
using its locked Bytes() method in the require.Eventually check.
- Around line 100-110: The test currently has a data race because output
(declared as bytes.Buffer) is written by the goroutine running io.Copy(ptmx ->
output) and concurrently read by require.Eventually via output.Bytes(); replace
this with a thread-safe approach: either use io.Pipe (create pr, pw; let the
goroutine io.Copy(pw, ptmx) and in the main goroutine read from pr into a local
buffer for inspection) or implement a small mutex-guarded buffer type (replace
output := &bytes.Buffer{} with a sync-safe wrapper and change calls to
output.Write/Bytes to use the wrapper’s methods) so that io.Copy, the outputCh
goroutine, and require.Eventually access the buffer safely.
---
Outside diff comments:
In `@test/integration/login_browser_flow_test.go`:
- Around line 3-84: The test TestBrowserFlowStoresToken mutates and reads the
shared bytes.Buffer output concurrently (io.Copy in the PTY goroutine vs
require.Eventually reading output.Bytes()), causing race conditions; fix by
making access to output thread-safe: add a sync.Mutex (e.g., outputMu) and lock
around all writes and reads to output (wrap the io.Copy target with a small
writer that locks on Write, and in the require.Eventually callback lock, copy
output.Bytes() into a local slice, then unlock before checking for "TEST123"),
or alternatively replace output with a channel-based reader that serializes
writes and exposes a safe snapshot method; ensure all references to output in
TestBrowserFlowStoresToken use the guarded access.
---
Nitpick comments:
In `@internal/ui/run_login_test.go`:
- Around line 34-76: The createMockAPIServer function is duplicated; extract it
into a shared test helper (e.g., package internal/testutil) as an exported
helper (e.g., NewMockAPIServer or CreateMockAPIServer) that accepts t *testing.T
plus the same configurable parameters (licenseToken, confirmed) and preserves
the same handler behavior (auth request/exchange, license endpoints); then
replace the local createMockAPIServer implementations in
internal/ui/run_login_test.go and test/integration/login_device_flow_test.go
with calls to the new shared helper so both tests use the single canonical
implementation.
Comment on lines
100
to
110
| output := &bytes.Buffer{} | ||
| outputCh := make(chan struct{}) | ||
| go func() { | ||
| out, _ := cmd.CombinedOutput() | ||
| outputCh <- out | ||
| _, _ = io.Copy(output, ptmx) | ||
| close(outputCh) | ||
| }() | ||
|
|
||
| // Wait for device flow instructions | ||
| time.Sleep(100 * time.Millisecond) | ||
| // Wait for verification code to appear | ||
| require.Eventually(t, func() bool { | ||
| return bytes.Contains(output.Bytes(), []byte("TEST123")) | ||
| }, 10*time.Second, 100*time.Millisecond, "verification code should appear") |
There was a problem hiding this comment.
Data race: bytes.Buffer accessed concurrently without synchronization.
The goroutine writes to output via io.Copy (line 103) while the main goroutine reads via output.Bytes() (line 109) in the Eventually loop. bytes.Buffer is not thread-safe, which can cause test flakiness, panics, or corrupted reads.
Use a synchronized buffer or io.Pipe with a separate reader.
🔒 Proposed fix using sync.Mutex
+type syncBuffer struct {
+ mu sync.Mutex
+ buf bytes.Buffer
+}
+
+func (sb *syncBuffer) Write(p []byte) (int, error) {
+ sb.mu.Lock()
+ defer sb.mu.Unlock()
+ return sb.buf.Write(p)
+}
+
+func (sb *syncBuffer) Bytes() []byte {
+ sb.mu.Lock()
+ defer sb.mu.Unlock()
+ return sb.buf.Bytes()
+}
+
+func (sb *syncBuffer) String() string {
+ sb.mu.Lock()
+ defer sb.mu.Unlock()
+ return sb.buf.String()
+}Then use output := &syncBuffer{} instead of output := &bytes.Buffer{}.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration/login_device_flow_test.go` around lines 100 - 110, The test
currently has a data race because output (declared as bytes.Buffer) is written
by the goroutine running io.Copy(ptmx -> output) and concurrently read by
require.Eventually via output.Bytes(); replace this with a thread-safe approach:
either use io.Pipe (create pr, pw; let the goroutine io.Copy(pw, ptmx) and in
the main goroutine read from pr into a local buffer for inspection) or implement
a small mutex-guarded buffer type (replace output := &bytes.Buffer{} with a
sync-safe wrapper and change calls to output.Write/Bytes to use the wrapper’s
methods) so that io.Copy, the outputCh goroutine, and require.Eventually access
the buffer safely.
Comment on lines
161
to
171
| output := &bytes.Buffer{} | ||
| outputCh := make(chan struct{}) | ||
| go func() { | ||
| out, _ := cmd.CombinedOutput() | ||
| outputCh <- out | ||
| _, _ = io.Copy(output, ptmx) | ||
| close(outputCh) | ||
| }() | ||
|
|
||
| // Wait for device flow instructions to be printed | ||
| time.Sleep(1 * time.Second) | ||
| // Wait for verification code to appear | ||
| require.Eventually(t, func() bool { | ||
| return bytes.Contains(output.Bytes(), []byte("TEST123")) | ||
| }, 10*time.Second, 100*time.Millisecond, "verification code should appear") |
There was a problem hiding this comment.
Same data race issue exists here.
This test has the identical concurrent access pattern on bytes.Buffer. Apply the same synchronized buffer fix as suggested for TestDeviceFlowSuccess.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration/login_device_flow_test.go` around lines 161 - 171, The test
uses a plain bytes.Buffer (output) concurrently: a goroutine writes to it via
io.Copy(ptmx -> output) while the main goroutine reads output.Bytes() inside
require.Eventually, causing a data race; replace the bytes.Buffer with the same
synchronized buffer implementation used in TestDeviceFlowSuccess (a thread-safe
wrapper that implements io.Writer and a locked Bytes() accessor), update the
output variable to that safe type, and ensure the goroutine still closes
outputCh after io.Copy; read from the buffer using its locked Bytes() method in
the require.Eventually check.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/output/format.go (2)
55-60: Only the first option label is displayed.
formatUserInputRequestonly showsOptions[0].Label. If multiple options exist (e.g., "Yes/No"), users won't see the alternatives. This may be intentional for the current use case, but consider joining all option labels for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/format.go` around lines 55 - 60, formatUserInputRequest currently only displays Options[0].Label so multi-option prompts hide alternatives; update the function to collect all option labels from e.Options (e.g., map over e.Options to extract each .Label) and join them (using a delimiter like " / " or ", ") when building the returned string so the prompt shows all available choices; refer to the formatUserInputRequest function and the UserInputRequestEvent.Options slice to implement this change.
44-53: Edge case: Empty LayerID produces awkward output.When
e.LayerIDis empty bute.Statusis set, the output becomes" : status"with a leading colon. Consider guarding against emptyLayerID.♻️ Proposed fix
func formatProgressLine(e ProgressEvent) (string, bool) { if e.Total > 0 { pct := float64(e.Current) / float64(e.Total) * 100 + if e.LayerID == "" { + return fmt.Sprintf(" %s %.1f%%", e.Status, pct), true + } return fmt.Sprintf(" %s: %s %.1f%%", e.LayerID, e.Status, pct), true } if e.Status != "" { + if e.LayerID == "" { + return fmt.Sprintf(" %s", e.Status), true + } return fmt.Sprintf(" %s: %s", e.LayerID, e.Status), true } return "", false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/format.go` around lines 44 - 53, formatProgressLine currently prints " %s: %s" which yields an awkward " : status" when e.LayerID is empty; update formatProgressLine to detect when e.LayerID == "" and omit the "LayerID: " prefix (or use a short fallback label) so that lines render as " status" (or " <unknown>: status" if you prefer a placeholder). Locate formatProgressLine and change the branches that build the strings using e.LayerID/e.Status to conditionally format based on e.LayerID being non-empty, ensuring both the percentage and non-percentage paths handle an empty e.LayerID consistently.internal/ui/run.go (1)
49-63: Model type assertion uses wrong variable.Line 54 asserts
model.(App)into a newappvariable but then callsapp.Err(). However, the originalappfrom line 31 is a different variable. This shadowing is correct behavior, but ifmodelis not anApptype (theokis false), the code silently continues. This should be fine sinceNewAppreturns anApp, but the pattern could be clearer.♻️ Optional: rename for clarity
- if app, ok := model.(App); ok && app.Err() != nil { - return app.Err() + if finalApp, ok := model.(App); ok && finalApp.Err() != nil { + return finalApp.Err() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/run.go` around lines 49 - 63, The code shadows the outer app variable by asserting model.(App) into a new app and can be confusing; change the type assertion after p.Run() to use a distinct name (e.g., modelApp or runApp) instead of app, e.g., do modelApp, ok := model.(App) and then check modelApp.Err(); alternatively, if NewApp always returns an App, you can remove the assertion entirely and use the original app variable from NewApp. Ensure you update all references to use the new identifier (modelApp or runApp) and avoid shadowing the outer app variable.
🤖 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/ui/components/header.go`:
- Around line 12-16: The comment above the nimbo helpers is redundant; either
remove it or replace it with a brief "why" comment explaining the non-obvious
spacing rationale (e.g., why leading space and relative offsets matter for cloud
shape alignment). Update the comment associated with nimboLine1 (and the other
nimbo helper functions) to state that the leading space and specific
glyph/spacing choices are intentional for alignment across terminals, or simply
delete the comment if no extra rationale is needed.
In `@test/integration/login_browser_flow_test.go`:
- Around line 59-69: The test has a data race because the goroutine running
io.Copy writes into output (a bytes.Buffer) while require.Eventually
concurrently calls output.Bytes(); make the buffer thread-safe by protecting all
access with a mutex (or replace bytes.Buffer with a concurrent-safe wrapper) —
e.g., create a sync.Mutex and use it around writes in the goroutine that uses
io.Copy (or wrap io.Copy target to lock on Write) and around reads inside the
lambda passed to require.Eventually that calls output.Bytes() (references:
output, io.Copy goroutine, outputCh, and the require.Eventually closure).
---
Duplicate comments:
In `@internal/auth/auth.go`:
- Around line 30-31: Remove the redundant comment block above the GetToken
method on the Auth type in internal/auth/auth.go: delete the line(s) "//
GetToken tries in order: 1) keyring 2) LOCALSTACK_AUTH_TOKEN env var 3) browser
login (if allowed)" so the method signature func (a *Auth) GetToken(ctx
context.Context) (string, error) stands without the self-explanatory comment;
leave any non-redundant comments that explain non-obvious "why" logic inside
GetToken intact.
In `@internal/ui/run_login_test.go`:
- Around line 216-220: Replace the brittle fixed-port and sleep approach: stop
using time.Sleep(100 * time.Millisecond) and the hardcoded
"http://127.0.0.1:45678/..." URL; instead start the callback server on port 0 so
the OS assigns an available port (use listener.Addr() to get the actual address)
and implement a short polling/retry loop that attempts the http GET to the
derived URL until it succeeds or a timeout elapses. Update the test around the
http.Get call (and any server start routine that currently binds 45678) to
derive the request URL from the listener address and to retry with small delays
rather than relying on a single sleep.
In `@test/integration/login_device_flow_test.go`:
- Around line 100-110: The test uses a plain bytes.Buffer named output that is
written to by the goroutine (io.Copy from ptmx) and concurrently read by
require.Eventually via output.Bytes(), causing a data race; replace the unsafe
buffer with a thread-safe buffer wrapper (e.g., define a small safeBuffer type
with an embedded bytes.Buffer, a sync.Mutex, and exported Write(p []byte) (int,
error) and Bytes() []byte methods that lock/unlock) and instantiate output as
&safeBuffer{} so the goroutine calls io.Copy(output, ptmx) and the poll reads
output.Bytes() safely; apply the same replacement for the other test instance
where output/goroutine/require.Eventually are used.
---
Nitpick comments:
In `@internal/output/format.go`:
- Around line 55-60: formatUserInputRequest currently only displays
Options[0].Label so multi-option prompts hide alternatives; update the function
to collect all option labels from e.Options (e.g., map over e.Options to extract
each .Label) and join them (using a delimiter like " / " or ", ") when building
the returned string so the prompt shows all available choices; refer to the
formatUserInputRequest function and the UserInputRequestEvent.Options slice to
implement this change.
- Around line 44-53: formatProgressLine currently prints " %s: %s" which yields
an awkward " : status" when e.LayerID is empty; update formatProgressLine to
detect when e.LayerID == "" and omit the "LayerID: " prefix (or use a short
fallback label) so that lines render as " status" (or " <unknown>: status" if
you prefer a placeholder). Locate formatProgressLine and change the branches
that build the strings using e.LayerID/e.Status to conditionally format based on
e.LayerID being non-empty, ensuring both the percentage and non-percentage paths
handle an empty e.LayerID consistently.
In `@internal/ui/run.go`:
- Around line 49-63: The code shadows the outer app variable by asserting
model.(App) into a new app and can be confusing; change the type assertion after
p.Run() to use a distinct name (e.g., modelApp or runApp) instead of app, e.g.,
do modelApp, ok := model.(App) and then check modelApp.Err(); alternatively, if
NewApp always returns an App, you can remove the assertion entirely and use the
original app variable from NewApp. Ensure you update all references to use the
new identifier (modelApp or runApp) and avoid shadowing the outer app variable.
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
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, have 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.UserInputRequestEventwith included response channel for the UI to interact withThis 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 (login and general startup)
Also, I've changed the Device Flow and Browser Flow tests to use a PTY to be able to interact with BubbleTea properly