refactor(terminal): split message rendering#75
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors terminal message rendering into three focused pieces: cache orchestration (cache state and warm-up), role-specific rendering (per-role formatters and streaming cache), and layout/scrolling (compose static + dynamic groups and compute viewport slices). ChangesTerminal message rendering refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #75 +/- ##
==========================================
+ Coverage 65.96% 66.18% +0.21%
==========================================
Files 205 207 +2
Lines 17828 17850 +22
==========================================
+ Hits 11761 11814 +53
+ Misses 4976 4940 -36
- Partials 1091 1096 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/terminal/message_layout.go`:
- Around line 220-225: The helper visibleMessageLineGroups (used by
drawRuntimeTextBuffer and message windows) must not mutate app.scrollOffset;
remove the side-effecting assignments and clamping lines and instead only
compute and return the sliced/flattened groups. Specifically, in the block where
you check if maxRows < 0 || totalRows <= maxRows and where you compute maxOffset
and clamp app.scrollOffset, eliminate any writes to app.scrollOffset and just
return flattenStyledLineGroups(groups, totalRows) or the properly sliced result
based on maxRows/totalRows; keep all scroll bookkeeping (setting or clamping
app.scrollOffset) in the message-specific callers (e.g., the chat rendering
code) not in visibleMessageLineGroups.
In `@internal/terminal/message_render.go`:
- Around line 154-156: The loop that currently does for _, line := range
markdownLines { lines = append(lines, newStyledLine(style, line.Text)) }
discards per-line Spans and any line-level styling produced by renderMarkdown;
instead, preserve the original styledLine data by appending the existing line
(or a shallow copy) and merge the desired container style only into its
Style/Spans rather than reconstructing from Text. Locate markdownLines and
newStyledLine usage in message_render.go and replace constructing a new styled
line from line.Text with either lines = append(lines, line) or by copying line
and applying/combining the outer style onto line.Style/Spans so Spans and
line-level attributes from renderMarkdown are retained for thinking output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd3cc542-75d0-4bfb-850b-3571dae5733c
📒 Files selected for processing (3)
internal/terminal/message_layout.gointernal/terminal/message_render.gointernal/terminal/render_messages.go
💤 Files with no reviewable changes (1)
- internal/terminal/render_messages.go
omarluq
force-pushed
the
refactor/terminal-message-render-split
branch
from
629a0bb to
e0dee94
Compare
omarluq
force-pushed
the
refactor/terminal-message-render-split
branch
from
e0dee94 to
fa7cb65
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/terminal/render_test.go (1)
158-170: ⚡ Quick winConsider table-driving these rendering-variant assertions.
These tests cover closely related renderer variants with repeated setup/assert patterns. A table-driven structure would make it easier to add new role/layout variants and keep expectations aligned.
As per coding guidelines, "
**/*_test.go: Prefer table-driven tests for core behavior and regression tests for terminal rendering bugs".Also applies to: 172-181
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/terminal/render_test.go` around lines 158 - 170, This test (TestRenderQueuedMessagesRendersHeadersAndBody) repeats similar setup/assert patterns; convert it and the neighboring related test(s) into a table-driven test that iterates variants (e.g., different queuedMessages, widths, expected text slices) to reduce duplication; structure a slice of cases with name, queuedMessages, width, and expectedLines, and inside the loop call renderQueuedMessages and lineTexts and run assertions (assert.Contains or assert.Equal as appropriate) using t.Run for each case to preserve isolation; reference the existing functions renderQueuedMessages and lineTexts to locate where to adapt the assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/terminal/render_test.go`:
- Around line 62-67: The test TestRenderThinkingMessagePreservesMarkdownSpans
indexes lines[2] without verifying the slice length which can cause a panic;
before accessing lines[2] in the test that calls app.renderThinkingMessage, add
a guard assertion (e.g., require.GreaterOrEqual(t, len(lines), 3, "expected
thinking content line")) to ensure at least three lines are present, then
proceed to set content := lines[2] and assert on content.Spans.
---
Nitpick comments:
In `@internal/terminal/render_test.go`:
- Around line 158-170: This test (TestRenderQueuedMessagesRendersHeadersAndBody)
repeats similar setup/assert patterns; convert it and the neighboring related
test(s) into a table-driven test that iterates variants (e.g., different
queuedMessages, widths, expected text slices) to reduce duplication; structure a
slice of cases with name, queuedMessages, width, and expectedLines, and inside
the loop call renderQueuedMessages and lineTexts and run assertions
(assert.Contains or assert.Equal as appropriate) using t.Run for each case to
preserve isolation; reference the existing functions renderQueuedMessages and
lineTexts to locate where to adapt the assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 168629bd-4343-4e5a-9b1b-e73ba0464509
📒 Files selected for processing (3)
internal/terminal/message_layout.gointernal/terminal/message_render.gointernal/terminal/render_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/terminal/message_render.go
- internal/terminal/message_layout.go
|
This was referenced Jun 10, 2026
Merged
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.
No description provided.