fix(cli): improve markdown table rendering in terminal#2914
Open
fix(cli): improve markdown table rendering in terminal#2914
Conversation
wenshao
requested review from
DennisYu07,
DragonnZhang,
LaZzyMan,
Mingholy,
gwinthis,
pomelo-nwu and
tanzhenxin
as code owners
Contributor
📋 Review SummaryThis PR significantly improves markdown table rendering in the terminal CLI by addressing ANSI escape sequence handling, CJK character width calculation, column alignment parsing, and content wrapping. The implementation is comprehensive with extensive test coverage for edge cases. The changes transform the table renderer from a truncation-based approach to a wrapping-based approach with proper alignment support. 🔍 General Feedback
🎯 Specific Feedback🟡 High Priority Issues
🟢 Medium Priority Issues
🔵 Low Priority Suggestions
✅ Highlights
|
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
Improvements over previous commit: - Restore theme.border.default color for table borders - Restore theme.text.link color + bold for table headers - Add renderMarkdownToAnsi() to render **bold**, `code`, *italic*, ~~strikethrough~~, <u>underline</u>, [links](url), and bare URLs as ANSI-styled text in table cells (mirrors RenderInline behavior) - Use raw ANSI escape codes instead of chalk (chalk.level=0 in tests) - Remove dead code: INLINE_MARKDOWN_REGEX, hasInlineMarkdown, ANSI_BOLD_START/END constants, unused vi/beforeEach in tests - Update 8 snapshots to reflect themed output Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves CLI terminal rendering of Markdown tables by making table layout ANSI/CJK-aware, wrapping long cell content, and honoring Markdown alignment markers, with added regression coverage.
Changes:
- Rewrote
TableRendererto render tables as a single ANSI string block (wrapping + alignment + vertical fallback + safety checks). - Updated
MarkdownDisplayto parse separator alignment markers and pass per-column alignment through to the renderer. - Added/updated tests and snapshots covering width correctness, wrapping behavior, alignment, and edge cases.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cli/src/ui/utils/TableRenderer.tsx | New string-based renderer with ANSI/CJK-aware width, wrapping, alignment, vertical fallback, and safety width checks. |
| packages/cli/src/ui/utils/TableRenderer.test.tsx | New regression test suite for rendering stability across ANSI/CJK/wrapping/alignment/edge cases. |
| packages/cli/src/ui/utils/MarkdownDisplay.tsx | Parses table separator alignment markers and forwards alignments to TableRenderer; updated separator regex for single-column tables. |
| packages/cli/src/ui/utils/MarkdownDisplay.test.tsx | Adds tests for single-column tables, escaped pipes, table termination, and false-positive avoidance. |
| packages/cli/src/ui/utils/snapshots/MarkdownDisplay.test.tsx.snap | Snapshot updates reflecting new ANSI-styled table output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- renderRowLines: normalize cells to exactly colCount (pad/truncate) to prevent undefined access when row has fewer cells than headers - calculateMaxRowLines: iterate colCount instead of row.length to prevent undefined columnWidths access for extra cells - tableSeparatorRegex: add (?=.*\|) lookahead to require at least one pipe character, preventing `---` (horizontal rule) from being mis-parsed as a table separator - Add test: horizontal rule after pipe line is not a table separator Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- idealWidths: use getRenderedWidth() (markdown→ANSI→stripAnsi→stringWidth) instead of getPlainTextLength() so link URLs are accounted for in column width calculation - calculateMaxRowLines: use getFormattedCellText() (same as renderRowLines) so vertical fallback decision matches actual rendered row height - renderVerticalFormat: normalize row to colCount (pad/truncate) for consistency with horizontal format - renderVerticalFormat: render markdown in labels via renderMarkdownToAnsi() instead of showing raw syntax - Remove unused getCellPlainText helper and getPlainTextLength import Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Early return empty <Box /> when headers is empty (colCount === 0) to prevent malformed border output - Always apply theme.text.link color to header cells regardless of ANSI content, matching original Ink implementation behavior - Validate separator column count matches header column count before entering table mode, preventing mismatched separators like `| A | B |` followed by `|---|` from creating invalid tables - Add test for column count mismatch detection - Update 2 snapshots for consistent header link color Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- getMinWordWidth: use renderMarkdownToAnsi output so link URLs are included as unbreakable tokens in minimum column width calculation - Remove now-unused stripInlineMarkdown function - Header alignment: respect explicit alignment markers from separator; only default to center when no alignment is specified for the column - Header color nesting: re-apply theme.text.link color after inner foreground resets (from inline code/links) to match Ink's nested color behavior where parent color is restored after child resets - Add getColorCode() helper for extracting raw ANSI color escape Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add recolorAfterResets() helper that handles both \x1b[39m (foreground reset) and \x1b[0m (full SGR reset). Applies to both header and body cells so mixed ANSI content keeps consistent theme coloring. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Vertical fallback labels with inline markdown (code, URLs) now re-apply link color after SGR resets, consistent with horizontal header/body cell behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Vertical fallback values now get theme.text.primary color with recolorAfterResets, consistent with horizontal body cell styling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/cli/src/ui/utils/MarkdownDisplay.tsx:167
- Inside an active table, any line that matches
tableSeparatorRegexis treated as a separator and not added as a data row. This can drop legitimate rows like|---|---|that appear after the header separator (common for showing literal dashes). Only treat the separator line specially immediately after the header row (e.g., whentableRows.length === 0/ aseenSeparatorflag); otherwise, handle it as a normaltableRowMatch.
} else if (inTable && tableSeparatorMatch) {
// Parse alignment from separator line
tableAligns = parseTableAligns(line);
} else if (inTable && tableRowMatch) {
// Add table row
const cells = tableRowMatch[1]
.split(/(?<!\\)\|/)
.map((cell) => cell.trim().replaceAll('\\|', '|'));
// Ensure row has same column count as headers
while (cells.length < tableHeaders.length) {
cells.push('');
}
if (cells.length > tableHeaders.length) {
cells.length = tableHeaders.length;
}
tableRows.push(cells);
} else if (inTable && !tableRowMatch) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wrapText now only trims trailing empty lines (wrap-ansi artifacts) instead of filtering all empty lines, preserving intentional blank lines within multi-paragraph cell content. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add HEX_COLOR_RE validation; invalid hex like #ff00 or #gg0000 now returns unchanged text instead of producing NaN in ANSI escapes - Refactor applyColor to delegate to getColorCode, eliminating duplicated hex parsing logic Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Precompute per-cell rendered text, visible width, and min word width once via computeMetrics(), eliminating repeated renderMarkdownToAnsi calls across width calculation, max-row-lines check, and rendering - Add post-pass in totalMin > availableWidth branch: shave wider columns until sum(columnWidths) <= availableWidth, preventing MIN_COLUMN_WIDTH floor from causing unnecessary vertical fallback - Remove now-unused getMinWordWidth standalone function Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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.
Problem
Terminal markdown table rendering had several correctness issues:
:---,:---:,---:) ignored|---|not recognizedComparison with Claude Code (upstream)
stringWidthfor correct double-width...:---:,---:)<Text>, can break mid-celltheme.border.default+theme.text.link<RenderInline>componentrenderMarkdownToAnsi()Before / After
Before (Claude Code upstream):
After (this PR):
What changed
TableRenderer.tsx(rewrite)stringWidth+getCachedStringWidthwrap-ansi)theme.border.defaultfor borders,theme.text.link+ bold for headersrenderMarkdownToAnsi(): converts bold,code, link, italic,strike, underline, bare URLs to ANSI escape codes (mirrorsRenderInlinebehavior including italic boundary checks)MarkdownDisplay.tsxalignsprop toTableRendererTests
TableRenderertests: CJK, ANSI colors, mixed content, alignment, edge cases (empty headers, width 0/1, NaN check)MarkdownDisplaytests: single-column, center alignment, escaped pipes, invalid separator, blank line terminationVerification
npx vitest run packages/cli/src/ui/utils/TableRenderer.test.tsx packages/cli/src/ui/utils/MarkdownDisplay.test.tsx # 90 passed (90)Full suite: no new failures (pre-existing integration test failures only).