Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
📝 WalkthroughWalkthroughUpdates the ErrorDisplay component to accept a width parameter for text wrapping. The View method signature changes to accept maxWidth, enabling multi-line rendering with proper text wrapping. All callers are updated to pass the content width. Changes
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
| for _, word := range words { | ||
| if current.Len() == 0 { | ||
| current.WriteString(word) | ||
| continue | ||
| } | ||
| if current.Len()+1+len(word) > maxWidth { | ||
| lines = append(lines, current.String()) | ||
| current.Reset() | ||
| current.WriteString(word) | ||
| } else { | ||
| current.WriteByte(' ') | ||
| current.WriteString(word) | ||
| } | ||
| } |
There was a problem hiding this comment.
softWrap still emits overlong words, so width limits can be violated.
Overlong tokens are appended as-is when they exceed maxWidth, so narrow views can still overflow/truncate paths and socket strings. That conflicts with the helper contract and the intent of width-constrained rendering.
Proposed fix
func softWrap(text string, maxWidth int) []string {
if maxWidth <= 0 || len(text) <= maxWidth {
return []string{text}
}
words := strings.Fields(text)
if len(words) == 0 {
return []string{text}
}
var lines []string
var current strings.Builder
+ flushCurrent := func() {
+ if current.Len() > 0 {
+ lines = append(lines, current.String())
+ current.Reset()
+ }
+ }
for _, word := range words {
+ for len(word) > maxWidth {
+ flushCurrent()
+ lines = append(lines, word[:maxWidth])
+ word = word[maxWidth:]
+ }
+
if current.Len() == 0 {
current.WriteString(word)
continue
}
if current.Len()+1+len(word) > maxWidth {
- lines = append(lines, current.String())
- current.Reset()
+ flushCurrent()
current.WriteString(word)
} else {
current.WriteByte(' ')
current.WriteString(word)
}
}
- if current.Len() > 0 {
- lines = append(lines, current.String())
- }
+ flushCurrent()
return lines
}📝 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.
| for _, word := range words { | |
| if current.Len() == 0 { | |
| current.WriteString(word) | |
| continue | |
| } | |
| if current.Len()+1+len(word) > maxWidth { | |
| lines = append(lines, current.String()) | |
| current.Reset() | |
| current.WriteString(word) | |
| } else { | |
| current.WriteByte(' ') | |
| current.WriteString(word) | |
| } | |
| } | |
| func softWrap(text string, maxWidth int) []string { | |
| if maxWidth <= 0 || len(text) <= maxWidth { | |
| return []string{text} | |
| } | |
| words := strings.Fields(text) | |
| if len(words) == 0 { | |
| return []string{text} | |
| } | |
| var lines []string | |
| var current strings.Builder | |
| flushCurrent := func() { | |
| if current.Len() > 0 { | |
| lines = append(lines, current.String()) | |
| current.Reset() | |
| } | |
| } | |
| for _, word := range words { | |
| for len(word) > maxWidth { | |
| flushCurrent() | |
| lines = append(lines, word[:maxWidth]) | |
| word = word[maxWidth:] | |
| } | |
| if current.Len() == 0 { | |
| current.WriteString(word) | |
| continue | |
| } | |
| if current.Len()+1+len(word) > maxWidth { | |
| flushCurrent() | |
| current.WriteString(word) | |
| } else { | |
| current.WriteByte(' ') | |
| current.WriteString(word) | |
| } | |
| } | |
| flushCurrent() | |
| return lines | |
| } |
🤖 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 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.
| } | ||
|
|
||
| // softWrap breaks text into lines at word boundaries, falling back to hard | ||
| // breaks for words longer than maxWidth. |
There was a problem hiding this comment.
falling back to hard breaks for words longer than maxWidth.
issue: Is this really implemented?
| } | ||
|
|
||
| return lines | ||
| } |
There was a problem hiding this comment.
suggestion: Can we move softWrap together with hardWrap in internal/ui/app.go to a dedicated internal/ui/wrap.go and unit-test them?
| sb.WriteString(styles.Message.Bold(true).Render(action.Value)) | ||
| for i, action := range e.event.Actions { | ||
| if i > 0 { | ||
| sb.WriteString(styles.SecondaryMessage.Render("⇒ " + action.Label + " " + action.Value)) |
There was a problem hiding this comment.
nit: I know it was already in there like this, but I think it would be nice if we defined the symbols we're using somewhere central. Maybe as a const in ui/styles/symbols.go.
const (
ArrowRight = "→"
ArrowBold = "⇒"
Check = "✓"
Cross = "✗"
)Something like this?
Could also just be something for a follow-up PR
There was a problem hiding this comment.
Could also allow us to add an option for using nerd fonts in the future :)
3 participants
See DES-152 for more context.