Improve port-in-use error

[gtsiolis]

BEFORE	AFTER

[coderabbitai]

📝 Walkthrough

Walkthrough

Replaces direct port-in-use error returns with a structured user-facing error emission (emitPortInUseError) that provides actionable options and returns a SilentError. Introduces an exported ErrorActionPrefix constant and updates formatting, UI rendering, and tests to use the new prefix ("==> ").

Changes

Cohort / File(s)	Summary
Port-in-use handling internal/container/start.go	Replace formatted port-in-use error with emitPortInUseError(sink, port); emit a structured error event (mandatory "stop emulator" action, optional "use config port" action) and return a SilentError.
Output events constant internal/output/events.go	Add exported ErrorActionPrefix = "==> " constant for action prefixing.
Plain output formatting internal/output/plain_format.go, internal/output/plain_format_test.go, internal/output/plain_sink_test.go	Use ErrorActionPrefix instead of hard-coded arrow; update tests to expect "==> " prefix for action lines.
UI action rendering internal/ui/components/error_display.go, internal/ui/components/error_display_test.go	Switch action prefix to output.ErrorActionPrefix in ErrorDisplay.View; add test verifying multi-action rendering (labels and values).
Integration test expectations test/integration/start_test.go	Update expected error message capitalization to "Port 4566 already in use" and assert presence of the new summary and suggested action ("LocalStack may already be running." and "lstk stop").

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Improve container engine error #72: Related changes to internal/ui/components/error_display.go affecting action rendering and formatting.
• Handle already-running containers and port conflicts #40: Related earlier work adding port-availability check and config-file error path used by the replaced error handling.
• Better error for missing container runtime #46: Related introduction/usage of SilentError handling referenced by the new control flow.

Suggested reviewers

• silv-io
• carole-lavillonniere
• anisaoshafi

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Improve port-in-use error' directly and clearly summarizes the main change: enhancing the port-in-use error message with structured diagnostic and remediation hints.
Description check	✅ Passed	The description uses before/after screenshots to demonstrate the improvement to the port-in-use error message, showing enhanced user feedback with actionable suggestions.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch george/des-158

Tip

Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

internal/ui/components/error_display.go (1)

63-68: Add coverage for the multi-action rendering branch.

The first action and subsequent actions go through different style/composition paths here, so a single-action assertion won’t catch regressions in the i > 0 branch. A component test with at least two actions would protect the new TUI behavior.

Based on learnings, TUI components may render separately from plain output, and tests should verify those distinct rendering paths.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/ui/components/error_display.go` around lines 63 - 68, Add a
component test that covers the multi-action branch in the loop over
e.event.Actions: create an event with at least two actions and exercise the
ErrorDisplay rendering (e.g., a test named TestErrorDisplay_MultiActionRenders)
so the code path where i > 0 runs. In the test, render both the TUI/component
output and the plain/text output if your helpers separate them, then assert that
the first action uses styles.ErrorAction +
styles.Message.Bold(true).Render(action.Value) and the second action uses
styles.SecondaryMessage.Render(output.ErrorActionPrefix + action.Label + " " +
action.Value) so the SecondaryMessage branch is verified. Ensure the test
references the same symbols (e.event.Actions loop, styles.ErrorAction,
styles.Message.Bold, styles.SecondaryMessage) to catch regressions in
composition and styling.

test/integration/start_test.go (1)

91-94: Assert the new recovery hints, not just the title.

This only protects the capitalization change. The summary and the mandatory lstk stop action are the core user-facing behavior in this PR, so they should be part of the integration expectation too.

Suggested assertion expansion

 	assert.Contains(t, stderr, "Port 4566 already in use")
+	assert.Contains(t, stderr, "LocalStack may already be running.")
+	assert.Contains(t, stderr, "Stop existing emulator:")
+	assert.Contains(t, stderr, "lstk stop")

As per coding guidelines, "Prefer integration tests to cover most cases."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/integration/start_test.go` around lines 91 - 94, The test currently only
asserts the headline "Port 4566 already in use"; update the integration
expectation to assert the new recovery hints as well by checking stderr for the
human-facing summary text and the required recovery action (the suggestion to
run "lstk stop"). In the test that calls runLstk(t, testContext(t), ...,
"start") replace or complement the existing assert.Contains on "Port 4566
already in use" with assertions that stderr contains the full summary string
added by the change and that it contains the recovery instruction including the
literal 'lstk stop' command so the integration test validates both the message
and the required user action.

🤖 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/error_display.go`:
- Around line 63-68: Add a component test that covers the multi-action branch in
the loop over e.event.Actions: create an event with at least two actions and
exercise the ErrorDisplay rendering (e.g., a test named
TestErrorDisplay_MultiActionRenders) so the code path where i > 0 runs. In the
test, render both the TUI/component output and the plain/text output if your
helpers separate them, then assert that the first action uses styles.ErrorAction
+ styles.Message.Bold(true).Render(action.Value) and the second action uses
styles.SecondaryMessage.Render(output.ErrorActionPrefix + action.Label + " " +
action.Value) so the SecondaryMessage branch is verified. Ensure the test
references the same symbols (e.event.Actions loop, styles.ErrorAction,
styles.Message.Bold, styles.SecondaryMessage) to catch regressions in
composition and styling.

In `@test/integration/start_test.go`:
- Around line 91-94: The test currently only asserts the headline "Port 4566
already in use"; update the integration expectation to assert the new recovery
hints as well by checking stderr for the human-facing summary text and the
required recovery action (the suggestion to run "lstk stop"). In the test that
calls runLstk(t, testContext(t), ..., "start") replace or complement the
existing assert.Contains on "Port 4566 already in use" with assertions that
stderr contains the full summary string added by the change and that it contains
the recovery instruction including the literal 'lstk stop' command so the
integration test validates both the message and the required user action.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: cb96db8b-aa82-4885-95e5-67d2737efd15

📥 Commits

Reviewing files that changed from the base of the PR and between 3dd9206 and 30dfb98.

📒 Files selected for processing (7)

• internal/container/start.go
• internal/output/events.go
• internal/output/plain_format.go
• internal/output/plain_format_test.go
• internal/output/plain_sink_test.go
• internal/ui/components/error_display.go
• test/integration/start_test.go

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

internal/ui/components/error_display.go (1)

61-70: ⚠️ Potential issue | 🟡 Minor

Wrap remediation actions to maxWidth.

The new config-path action can easily exceed the terminal width, but these lines bypass wrap.SoftWrap, so the most useful hint may overflow or appear clipped in narrow terminals. Please wrap action text before styling/rendering, the same way the summary is handled.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/ui/components/error_display.go` around lines 61 - 70, The
remediation actions in e.event.Actions are written directly to the string
builder and can overflow terminals; wrap each action's text with wrap.SoftWrap
(using the existing maxWidth used for the summary) before applying styles so
long content is broken to terminal width. Update the loop that renders actions
(referencing e.event.Actions, styles.ErrorAction, styles.SecondaryMessage and
styles.Message) to compute a wrapped string for the combined prefix+label+value
(or prefix+label and value separately like the summary), then render the wrapped
lines with the appropriate styles instead of writing unwrapped text directly to
sb.

🧹 Nitpick comments (1)

internal/ui/components/error_display_test.go (1)

53-85: Assert the shared action prefix here too.

This test exercises the new multi-action path, but it only checks labels/values. If ErrorActionPrefix stopped rendering, the test would still pass. Adding an assertion for the prefixed action text would make this a much better guard for the behavior introduced in this PR.

Based on learnings, TUI rendering may legitimately diverge from plain output, so tests should verify the separate rendering path directly.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/ui/components/error_display_test.go` around lines 53 - 85, The test
TestErrorDisplay_MultiActionRenders currently only asserts labels/values; add an
assertion that the shared ErrorActionPrefix is rendered in the View output for
the multi-action path by checking that e.View(80) contains the ErrorActionPrefix
(e.g. prefix + action label or prefix appears before an action label), so modify
the test after calling view := e.View(80) to assert strings.Contains(view,
ErrorActionPrefix) or that ErrorActionPrefix appears immediately before one of
the action labels to ensure the prefixed rendering path used by
NewErrorDisplay/Show/output.ErrorEvent is covered.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@internal/ui/components/error_display.go`:
- Around line 61-70: The remediation actions in e.event.Actions are written
directly to the string builder and can overflow terminals; wrap each action's
text with wrap.SoftWrap (using the existing maxWidth used for the summary)
before applying styles so long content is broken to terminal width. Update the
loop that renders actions (referencing e.event.Actions, styles.ErrorAction,
styles.SecondaryMessage and styles.Message) to compute a wrapped string for the
combined prefix+label+value (or prefix+label and value separately like the
summary), then render the wrapped lines with the appropriate styles instead of
writing unwrapped text directly to sb.

---

Nitpick comments:
In `@internal/ui/components/error_display_test.go`:
- Around line 53-85: The test TestErrorDisplay_MultiActionRenders currently only
asserts labels/values; add an assertion that the shared ErrorActionPrefix is
rendered in the View output for the multi-action path by checking that
e.View(80) contains the ErrorActionPrefix (e.g. prefix + action label or prefix
appears before an action label), so modify the test after calling view :=
e.View(80) to assert strings.Contains(view, ErrorActionPrefix) or that
ErrorActionPrefix appears immediately before one of the action labels to ensure
the prefixed rendering path used by NewErrorDisplay/Show/output.ErrorEvent is
covered.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 88d30fd9-e139-4e62-9c5d-56ad2dcbcbf5

📥 Commits

Reviewing files that changed from the base of the PR and between 30dfb98 and 804bbb0.

📒 Files selected for processing (8)

• internal/container/start.go
• internal/output/events.go
• internal/output/plain_format.go
• internal/output/plain_format_test.go
• internal/output/plain_sink_test.go
• internal/ui/components/error_display.go
• internal/ui/components/error_display_test.go
• test/integration/start_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/output/events.go