Contain markdown tables in chat transcripts#14
Conversation
- Wrap rendered markdown tables in a horizontal scroll container - Add regression coverage for completed diff tables
📝 WalkthroughWalkthroughThis PR adds horizontal scrolling support for markdown tables rendered in chat transcripts. CSS styling enables the scroll container, a React table component applies the wrapper class to all markdown tables, and a new test verifies the feature works end-to-end. ChangesScrollable chat markdown tables
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
github-actions
Bot
added
size:S
vouch:trusted
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/index.css (1)
457-465:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix double margin on wrapped tables.
Both
.chat-markdown-table-scroll(the wrapper) and.chat-markdown table(the inner table) are in the spacing selector list, so both receivemargin: 0.65rem 0. Since tables are now wrapped in the scroll container, this creates unwanted internal spacing—the table will have its own margin inside the wrapper that also has margin.Remove
.chat-markdown tablefrom the spacing selector and let the wrapper handle spacing, or add a reset rule.🔧 Suggested fix
Option 1: Remove table from the spacing selector (preferred)
.chat-markdown p, .chat-markdown ul, .chat-markdown ol, .chat-markdown blockquote, .chat-markdown pre, -.chat-markdown-table-scroll, -.chat-markdown table { +.chat-markdown-table-scroll { margin: 0.65rem 0; }Option 2: Add a reset rule after the wrapper styles
.chat-markdown-table-scroll > table { min-width: 100%; } + +.chat-markdown-table-scroll > table { + margin: 0; +}🤖 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 `@apps/web/src/index.css` around lines 457 - 465, The spacing selector is applying margin to both the wrapper and the inner table, causing double vertical spacing; update the CSS so only the wrapper controls spacing by removing `.chat-markdown table` from the selector list (leave `.chat-markdown-table-scroll`), or alternatively add a reset rule `margin: 0` for `.chat-markdown table` after the wrapper styles to cancel the inner table margin; locate selectors involving `.chat-markdown-table-scroll` and `.chat-markdown table` and implement one of these fixes so the wrapper handles spacing exclusively.
🧹 Nitpick comments (1)
apps/web/src/components/chat/ChatTranscriptPane.browser.tsx (1)
169-239: ⚡ Quick winConsider verifying actual scroll behavior, not just DOM presence.
The test confirms that the scroll wrapper and table elements exist in the DOM, but doesn't verify that horizontal scrolling actually works. Given the long file path in the test data, the test could additionally check that the table overflows the wrapper (e.g.,
scrollWidth > clientWidth), ensuring the CSS enables scrolling correctly.💡 Enhanced test assertion
Add after line 234:
expect(tableScroll).not.toBeNull(); expect(tableScroll!.querySelector("table")).not.toBeNull(); + + // Verify that the table content actually triggers horizontal scrolling + const table = tableScroll!.querySelector("table"); + expect(table!.scrollWidth).toBeGreaterThan(table!.clientWidth); } finally {This verifies that the table's content width exceeds its visible width, confirming that horizontal scrolling is available.
🤖 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 `@apps/web/src/components/chat/ChatTranscriptPane.browser.tsx` around lines 169 - 239, Update the test "contains completed markdown diff tables within the transcript row" to assert actual horizontal overflow rather than just DOM presence: after you locate tableScroll via ".chat-markdown-table-scroll" and the inner table via tableScroll.querySelector("table"), add an assertion that the table's scrollWidth is greater than tableScroll.clientWidth (or that innerTable.scrollWidth > tableScroll.clientWidth), making sure to guard for nulls so the test fails meaningfully if elements are missing; this ensures the CSS enables horizontal scrolling for the long file path content.
🤖 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.
Outside diff comments:
In `@apps/web/src/index.css`:
- Around line 457-465: The spacing selector is applying margin to both the
wrapper and the inner table, causing double vertical spacing; update the CSS so
only the wrapper controls spacing by removing `.chat-markdown table` from the
selector list (leave `.chat-markdown-table-scroll`), or alternatively add a
reset rule `margin: 0` for `.chat-markdown table` after the wrapper styles to
cancel the inner table margin; locate selectors involving
`.chat-markdown-table-scroll` and `.chat-markdown table` and implement one of
these fixes so the wrapper handles spacing exclusively.
---
Nitpick comments:
In `@apps/web/src/components/chat/ChatTranscriptPane.browser.tsx`:
- Around line 169-239: Update the test "contains completed markdown diff tables
within the transcript row" to assert actual horizontal overflow rather than just
DOM presence: after you locate tableScroll via ".chat-markdown-table-scroll" and
the inner table via tableScroll.querySelector("table"), add an assertion that
the table's scrollWidth is greater than tableScroll.clientWidth (or that
innerTable.scrollWidth > tableScroll.clientWidth), making sure to guard for
nulls so the test fails meaningfully if elements are missing; this ensures the
CSS enables horizontal scrolling for the long file path content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1416d56c-a0d5-4b22-9bd5-c5a56b73186b
📒 Files selected for processing (3)
apps/web/src/components/ChatMarkdown.tsxapps/web/src/components/chat/ChatTranscriptPane.browser.tsxapps/web/src/index.css
1 participant
What Changed
Why
Wide markdown tables in assistant messages can overflow chat transcript rows, especially when diff summaries include long file paths. Wrapping tables in an overflow container keeps the message layout stable while preserving the full table content through horizontal scrolling.
UI Changes
Validation
bun run --cwd apps/web test:browser src/components/chat/ChatTranscriptPane.browser.tsxChecklist
Reviewer Notes