Merged
Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces ad-hoc hard-wrap logic with a new Changes
Sequence Diagram(s)sequenceDiagram
participant App as App (renderer)
participant Error as ErrorDisplay
participant Wrap as wrap
App->>Error: errorDisplay.View(contentWidth)
Error->>Wrap: SoftWrap(summary, contentWidth - prefixLen)
Wrap-->>Error: []lines
Error-->>App: rendered error block (multi-line, styled)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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: 1
🧹 Nitpick comments (1)
internal/ui/components/error_display_test.go (1)
35-50: Add a narrow-width regression test for wrapping behavior.These assertions validate content presence, but they don’t guard the new width-constrained behavior (especially long-token wrapping). A focused test for small widths would prevent regressions in this PR’s core behavior.
Suggested test addition
+func TestSoftWrap_HardBreaksLongWord(t *testing.T) { + t.Parallel() + + lines := softWrap("unix:///var/run/docker.sock", 8) + for _, line := range lines { + if len(line) > 8 { + t.Fatalf("expected hard-wrapped segment length <= 8, got %q", line) + } + } +}Based on learnings: “Ensure tests verify the separate rendering paths and avoid unintended coupling to the plain-output formatting.”
🤖 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 35 - 50, Add a focused unit test in error_display_test.go that constructs the same error display (the existing e variable/instance) but calls View with a small width (e.g., 20 or similar) and asserts that the output preserves the title, summary and detail while also verifying long-token wrapping behavior (e.g., a very long word is broken across lines) and that action lines (labels like "Start Docker:" and values like "open -a Docker") render on separate lines or wrap as expected; target the View(width int) method and the same title/summary/detail/action strings to ensure the width-constrained rendering path is exercised separately from the plain-output formatting.
🤖 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/error_display.go`:
- Around line 91-104: The softWrap loop currently appends overlong tokens
(variable word) directly and can exceed maxWidth; update the logic in the
softWrap routine (the for _, word := range words loop using current, maxWidth,
lines) to detect when len(word) > maxWidth and instead split that token into
chunks that fit maxWidth (using rune-aware slicing) appending each chunk as its
own line (flushing current first if non-empty), and when a chunk exactly fills a
line start a new line; ensure remaining partial chunk is written into current
rather than appended raw so no produced line exceeds maxWidth.
---
Nitpick comments:
In `@internal/ui/components/error_display_test.go`:
- Around line 35-50: Add a focused unit test in error_display_test.go that
constructs the same error display (the existing e variable/instance) but calls
View with a small width (e.g., 20 or similar) and asserts that the output
preserves the title, summary and detail while also verifying long-token wrapping
behavior (e.g., a very long word is broken across lines) and that action lines
(labels like "Start Docker:" and values like "open -a Docker") render on
separate lines or wrap as expected; target the View(width int) method and the
same title/summary/detail/action strings to ensure the width-constrained
rendering path is exercised separately from the plain-output formatting.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/ui/app.gointernal/ui/components/error_display.gointernal/ui/components/error_display_test.go
| } | ||
|
|
||
| // softWrap breaks text into lines at word boundaries, falling back to hard | ||
| // breaks for words longer than maxWidth. |
Collaborator
There was a problem hiding this comment.
falling back to hard breaks for words longer than maxWidth.
issue: Is this really implemented?
silv-io
reviewed
Mar 4, 2026
gtsiolis
force-pushed
the
george/improve-container-engine-error
branch
from
c840618 to
46f368e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/ui/wrap/wrap_test.go (1)
83-94: Add a UTF-8 multi-word regression case forSoftWrap.Current UTF-8 coverage only exercises single-token splitting. Please add a spaced UTF-8 case to lock in rune-aware behavior on the
current + space + wordpath.✅ Suggested test addition
{ name: "utf8 rune aware split", input: "🙂🙂🙂🙂🙂", width: 2, expected: []string{"🙂🙂", "🙂🙂", "🙂"}, }, + { + name: "utf8 multi-word keeps rune width", + input: "🙂 🙂", + width: 3, + expected: []string{"🙂 🙂"}, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/wrap/wrap_test.go` around lines 83 - 94, Add a regression test case to the existing table in wrap_test.go that ensures SoftWrap handles multi-word UTF-8 strings (rune-aware on the "current + space + word" path): add an entry with name like "utf8 multi-word rune aware split", input containing spaced emoji or other multi-byte runes (e.g., "🙂 🙂 🙂"), a small width that forces splitting across a space boundary, and the expected slices showing correct split positions; locate the test table used by the SoftWrap tests in internal/ui/wrap/wrap_test.go and append this case next to the existing utf8 cases so the SoftWrap behavior is locked in.
🤖 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/wrap/wrap.go`:
- Around line 62-67: SoftWrap currently mixes byte counts (current.Len(),
len(s)) with rune-based maxWidth causing incorrect wrapping for multi-byte UTF-8
characters; add a rune-count tracker (e.g., currentRunes int) to track the
number of runes in the current line and use rune counts everywhere instead of
current.Len(): update the early-exit check to compare utf8.RuneCountInString(s)
against maxWidth, replace uses of current.Len() in SoftWrap with currentRunes
when deciding to append or break, and increment/decrement currentRunes by the
rune length of appended words (use utf8.RuneCountInString(word) or len(runes)
from the existing rune slice) so all comparisons (the checks at the sites
indicated by current.Len() usage) are rune-consistent.
---
Nitpick comments:
In `@internal/ui/wrap/wrap_test.go`:
- Around line 83-94: Add a regression test case to the existing table in
wrap_test.go that ensures SoftWrap handles multi-word UTF-8 strings (rune-aware
on the "current + space + word" path): add an entry with name like "utf8
multi-word rune aware split", input containing spaced emoji or other multi-byte
runes (e.g., "🙂 🙂 🙂"), a small width that forces splitting across a space
boundary, and the expected slices showing correct split positions; locate the
test table used by the SoftWrap tests in internal/ui/wrap/wrap_test.go and
append this case next to the existing utf8 cases so the SoftWrap behavior is
locked in.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 9f4a80fa-d63f-4b61-894f-7851b3261f8a
📒 Files selected for processing (6)
internal/ui/app.gointernal/ui/app_test.gointernal/ui/components/error_display.gointernal/ui/components/error_display_test.gointernal/ui/wrap/wrap.gointernal/ui/wrap/wrap_test.go
💤 Files with no reviewable changes (1)
- internal/ui/app_test.go
carole-lavillonniere
approved these changes
Mar 5, 2026
silv-io
approved these changes
Mar 5, 2026
Member
Author
|
Thanks @carole-lavillonniere for taking another look, and @silv-io for merging! 🏀 |
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
3 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.
See DES-152 for more context.