Fix/detailed view scroll select#498
Conversation
renderAssistantMarkdownText embeds bold markers for prose and ANSI color sequences from chroma for fenced code blocks. stripMarkdownRenderControls only stripped the bold markers, leaking ANSI codes into clipboard text. Switch to ansi.Strip() to remove all escape sequences.
…anscriptBodyItems Extract rowRenderFn and transcriptRowDispatchFn types and refactor rendering methods into *Fn variants parameterized by the render function. Add detailed bool parameter to transcriptBodyItems to prepare for the detailed transcript viewport. All callers pass false so there is no behavior change yet. Extract transcriptRowBodyHeightCacheKeyOpts from transcriptRowBodyHeightCacheKey so the body cap can be configured per caller.
…ession Enable keyboard scrolling (PgUp/PgDown/Up/Down) in detailed transcript mode by routing through scrollChat instead of swallowing the keys. Rewrite detailedTranscriptView to use the transcriptBodyItems pipeline so the body renders through the same viewport/scroll engine as the live view. Implement detailedTranscriptFooter with copy-status and jump-to-bottom hint integration. Suppress the sidebar in detailed mode so the full chat width is available for reading uncapped tool output.
TestPgUpDownScrollsDetailedTranscript verifies that PgUp scrolls toward older content and PgDown returns to the bottom in detailed mode. TestUpDownArrowsScrollDetailedTranscript verifies that arrow keys scroll one line at a time and bottom is clamped at offset 0.
…d selectable text TestStripMarkdownRenderControlsStripsAllANSI verifies bold markers, color sequences, and combined sequences are stripped. TestSelectedTranscriptTextStripsANSIFromHighlightedCode verifies that chroma-highlighted code blocks produce clean plain text in the selectable layer.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR adds detailed transcript scrolling and selection support, updates detailed transcript viewport and hit-testing to match the rendered layout, refreshes transcript body call sites for the new signature, and switches markdown control stripping to centralized ANSI removal. ChangesDetailed transcript and clipboard rendering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant Model
participant detailedTranscriptView
participant transcriptBodyItems
User->>Model: press Ctrl+O
Model->>Model: set transcriptDetailed = true
Model->>detailedTranscriptView: render detailed transcript
detailedTranscriptView->>transcriptBodyItems: call(width, "", true)
transcriptBodyItems-->>detailedTranscriptView: detailed items
User->>Model: press PgUp / Up / PgDown / Down
Model->>Model: scrollChat(delta)
Model->>detailedTranscriptView: re-render with updated offset
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/tui/model.go (1)
1376-1395: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDetailed-mode PgUp/PgDown/Up/Down skip
clearHover().The non-detailed PgUp/PgDown paths explicitly call
m = m.clearHover()before scrolling — per the comment there, a stationary mouse over a bodyY-keyed hover target otherwise stays lit at the scrolled-away position. The new detailed-mode branches for PgUp, PgDown, Down, and Up all bypass that call even though detailed mode reuses the samehoverTranscripthighlighting path (finalizeTranscriptBodyRow/renderHoverHighlightin transcript_selection.go). Result: a hover highlight can visibly desync from its row after a keyboard scroll in the detailed view until the next mouse-motion event.🖱️ Proposed fix
case keyIs(msg, tea.KeyPgUp): - if m.transcriptDetailed { - return m.scrollChat(m.chatPageScrollLines()), nil - } - // A stationary mouse over a bodyY-keyed transcript hover target would - // otherwise stay lit at the scrolled-away bodyY until the next real - // motion event (see clearHover) — same reasoning as the wheel-scroll - // cases in mouse.go. - m = m.clearHover() - return m.scrollChat(m.chatPageScrollLines()), nil + m = m.clearHover() + return m.scrollChat(m.chatPageScrollLines()), nil case keyIs(msg, tea.KeyPgDown): - if m.transcriptDetailed { - return m.scrollChat(-m.chatPageScrollLines()), nil - } - m = m.clearHover() - return m.scrollChat(-m.chatPageScrollLines()), nil + m = m.clearHover() + return m.scrollChat(-m.chatPageScrollLines()), nil case keyIs(msg, tea.KeyDown): if m.transcriptDetailed { - return m.scrollChat(-1), nil + m = m.clearHover() + return m.scrollChat(-1), nil }and for Up:
if m.transcriptDetailed { - return m.scrollChat(1), nil + m = m.clearHover() + return m.scrollChat(1), nil }Also applies to: 1428-1437
🤖 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/tui/model.go` around lines 1376 - 1395, Detailed-mode keyboard scrolling in the key handling branch skips clearing hover state, so the hover highlight can stay stuck on the old transcript row after PgUp/PgDown/Up/Down. Update the detailed-mode paths in the key switch in model.go to call m.clearHover() before scrollChat(), matching the existing non-detailed behavior. Make sure the fix covers the PgUp, PgDown, and Up/Down handlers that affect hoverTranscript rendering via finalizeTranscriptBodyRow/renderHoverHighlight.
🧹 Nitpick comments (1)
internal/tui/transcript_view_test.go (1)
162-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRound out the arrow-key test with a down-from-nonzero assertion.
This test only exercises
KeyDownat the clamped floor (0→0, a no-op) andKeyUp(0→1). It never assertsKeyDownactually decrements from a non-zero offset back toward 0, so the decrement path itself is unverified.♻️ Suggested addition
// Arrow up scrolls one line toward older content. updated, _ = m.Update(testKey(tea.KeyUp)) m = updated.(model) if m.chatScrollOffset != 1 { t.Fatalf("KeyUp should scroll one line up, got chatScrollOffset=%d", m.chatScrollOffset) } + + // Arrow down scrolls back toward the bottom. + updated, _ = m.Update(testKey(tea.KeyDown)) + m = updated.(model) + if m.chatScrollOffset != 0 { + t.Fatalf("KeyDown should scroll one line down, got chatScrollOffset=%d", m.chatScrollOffset) + } }🤖 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/tui/transcript_view_test.go` around lines 162 - 190, The arrow-key coverage in TestUpDownArrowsScrollDetailedTranscript only verifies KeyDown at the zero clamp and KeyUp from 0 to 1, so the decrement path in transcript scrolling is still untested. Extend this test by first setting chatScrollOffset to a non-zero value in the transcriptViewTestModel, then sending a KeyDown event and asserting the offset decreases toward 0; use the existing Update method, testKey helper, and chatScrollOffset field to locate and validate the behavior.
🤖 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/tui/transcript_selection.go`:
- Around line 671-673: Remove the now-unused transcriptRowBodyHeightCacheKey
wrapper from model in transcript_selection.go, since all callers have already
been moved to transcriptRowBodyHeightCacheKeyOpts with an explicit bodyCap. Keep
the underlying transcriptRowBodyHeightCacheKeyOpts helper intact and make sure
no remaining references to transcriptRowBodyHeightCacheKey exist so
golangci-lint no longer flags dead code.
In `@internal/tui/transcript_view.go`:
- Around line 34-45: The detailed transcript footer currently hardcodes the
toggle hint as “Ctrl+O” instead of using the configured binding. Update
detailedTranscriptFooter in transcript_view.go to render the actual key from
m.keyBindings.toggleDetailed, following the same pattern used by other
user-facing hints like labelOr for toggleMouse. Keep the rest of the footer
behavior unchanged, including the jumpToBottomHint and copyStatus handling.
---
Outside diff comments:
In `@internal/tui/model.go`:
- Around line 1376-1395: Detailed-mode keyboard scrolling in the key handling
branch skips clearing hover state, so the hover highlight can stay stuck on the
old transcript row after PgUp/PgDown/Up/Down. Update the detailed-mode paths in
the key switch in model.go to call m.clearHover() before scrollChat(), matching
the existing non-detailed behavior. Make sure the fix covers the PgUp, PgDown,
and Up/Down handlers that affect hoverTranscript rendering via
finalizeTranscriptBodyRow/renderHoverHighlight.
---
Nitpick comments:
In `@internal/tui/transcript_view_test.go`:
- Around line 162-190: The arrow-key coverage in
TestUpDownArrowsScrollDetailedTranscript only verifies KeyDown at the zero clamp
and KeyUp from 0 to 1, so the decrement path in transcript scrolling is still
untested. Extend this test by first setting chatScrollOffset to a non-zero value
in the transcriptViewTestModel, then sending a KeyDown event and asserting the
offset decreases toward 0; use the existing Update method, testKey helper, and
chatScrollOffset field to locate and validate the behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f6cd8409-bef9-45aa-8b5b-b6abd175de50
📒 Files selected for processing (12)
internal/tui/assistant_markdown.gointernal/tui/file_view_test.gointernal/tui/files_panel.gointernal/tui/model.gointernal/tui/model_test.gointernal/tui/rendering_lime_test.gointernal/tui/sidebar.gointernal/tui/specialist_summary_render_test.gointernal/tui/transcript_body_test.gointernal/tui/transcript_selection.gointernal/tui/transcript_view.gointernal/tui/transcript_view_test.go
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The approach is right — routing the detailed view through the same selectable-line + viewport pipeline as the normal view is exactly what the issue described, and it fixes both halves (scroll keys and click-drag selection). I checked for a normal-view regression and the render/flush path holds up. Good to have that view not be a dead text dump anymore.
A few loose ends before it goes in, though — most of these CodeRabbit already flagged and they're still open at HEAD:
- The Up-arrow branch in the scroll handler doesn't clear hover state the way PgUp/PgDn/Down now do — looks like 3 of the 4 got it and Up got missed.
transcriptRowBodyHeightCacheKeyis dead after the refactor, nothing calls it. Drop it.- The footer still hardcodes
Ctrl+Ofor the detailed toggle. Now that keybindings are rebindable (#417), that should read fromm.keyBindings.toggleDetailedso a remapped key shows the right label. - One behavior change worth confirming is intended: the rewritten default render case now returns selectable lines instead of
nil, and that default is also hit byrowRecap, not justrowWelcome— andrc.skip()doesn't skip recap rows. So recap rows pick up selection/hover geometry in the normal view too. Probably harmless (maybe even nice), but it wasn't called out, so confirm it's on purpose.
None of this is a crash or a correctness problem — the fix works, CI's green — it's just not quite buttoned up. Tie those off and it's an easy approve.
rebindable key label
|
done and yes, recap rows getting selection/hover geometry is intentional. theyre user facing content and there's no reason to exclude them from copy/selection |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found a couple of issues that should be addressed before this is buttoned up.
Findings
-
[P2] Do not let Ctrl+B toggle the hidden sidebar while detailed mode is active
internal/tui/model.go:1310
This PR hides the context sidebar whilem.transcriptDetailedis true, but theCtrl+Bhandler still runs before the later detailed-mode key swallow andsidebarToggleAllowed()does not checkm.transcriptDetailed. As a result, pressingCtrl+Bin the detailed transcript flipsm.sidebarHiddenwith no visible feedback, and when the user exits detailed mode the normal transcript can unexpectedly come back without the sidebar. Please block the sidebar toggle while detailed mode is active, the same waysidebarAvailable()now suppresses rendering there. -
[P3] Complete the detailed-scroll down-arrow regression assertion
internal/tui/transcript_view_test.go:177
CodeRabbit's earlier arrow-key coverage request is still valid: the test only sendsKeyDownwhilechatScrollOffsetis already0, so it verifies the bottom clamp but never proves thatKeyDowndecrements from a nonzero offset back toward the bottom. Please extendTestUpDownArrowsScrollDetailedTranscriptafter the existingKeyUpassertion to sendKeyDownagain and assert the offset returns from1to0, so this new detailed-mode scroll path is actually covered.
1. the ctrl + b in detailed mode, added the guard to not be able to do it in detailed mode so it affect its state in the normal mode. 2. added a test coverage for keyDown after `keDown then keyUp` so it covers that.
done, there is what i did
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P2] Let detailed transcript bypass the active file drill-in
internal/tui/transcript_selection.go:217
Ctrl+Ocan be pressed while a file drill-in is active, andtoggleDetailedTranscriptdoes not close or suspend that file view. The new detailed renderer callstranscriptBodyItems(width, "", true), but this helper returnsm.fileViewBodyItems(width)before looking at the detailed flag, so the "Transcript detailed" view can show the selected file body instead of the full transcript. That leaves the exact detailed transcript surface from #495 still unavailable from this state. Please make detailed mode ignore or exit the file drill-in before building its body items, and add a regression test for toggling detailed mode whilefileView.activeis true. -
[P2] Stop invisible composer hit-testing in detailed mode
internal/tui/composer.go:264
The rendered detailed transcript usesdetailedTranscriptFooter, which has no composer, but the mouse path still checkshandleComposerSelectionMousebefore transcript hit-testing andcomposerPositionAtMousebuilds its rectangle from the normalfooterView()/composerBox(). The wheel path has the same problem throughmouseOverComposer. In detailed mode those normal composer rows are actually part of the detailed transcript body, so click-drag selection or wheel scrolling over the lower body can be consumed as invisible composer interaction instead of selecting or scrolling the transcript. Please block composer mouse hit-testing whilem.transcriptDetailedis true, or compute these rectangles from the same detailed frame that is actually rendered.
toggleDetailedTranscript now calls exitFileView before returning, so an active file drill-in is automatically closed when the user enters detailed transcript mode.
Adds TestDetailedTranscriptClosesFileView and TestDetailedTranscriptStaysClosedOnSecondToggle to guard against the file-view bypass bug.
composerMouseSelectionBlocked and mouseOverComposer now check m.transcriptDetailed so mouse clicks and wheel events in the composer area reach the transcript body instead of being silently consumed.
Adds TestComposerMouseSelectionBlockedInDetailedMode to guard against composer hit-test swallowing mouse events during detailed transcript viewing.
done |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-reviewing — my loose ends and jatmn's two findings are all tied off, so approving.
The rebindable footer label is done right: labelOr(m.keyBindings.toggleDetailed, "Ctrl+O") in both the footer and the hint, and the toggle itself now goes through keyMatch on the binding — so a remapped detailed key both shows and works. The Up-arrow hover clear and the dead cache-key helper are handled, and jatmn's two are in with tests: Ctrl+B no longer toggles the hidden sidebar in detailed mode (the handler gates on !m.transcriptDetailed), and entering detailed mode closes an active file drill-in.
Ran the tui suite locally on Windows (green), and CI is green across all platforms. The core is still the right fix — the detailed view goes through the same selectable-line + viewport pipeline as the normal view, so scroll keys and click-drag selection both work and it's no longer a dead text dump, which is exactly what #495 asked for.
Marked #495 issue-approved — the checklist claimed it already was, but it wasn't labeled yet. It's a real bug and this is the fix.
Good to merge — over to kevin.
kevincodex1
left a comment
kevincodex1
left a comment
There was a problem hiding this comment.
nice one! looks great thank you
4 participants
Summary
Linked issue
Fixes #495
2026-07-04T15-57-22-394Z.mp4
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Summary by CodeRabbit