Open
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
Comment on lines
+91
to
+104
| 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.
Suggested change
| 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.
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
1 participant
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.