fix(cli): smoother streaming table rendering#6345
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: real, observed UX issue. Streaming markdown tables in the interactive TUI exhibit five distinct visual artifacts — cell-by-cell fill-in, width oscillation, vertical-list-then-table flip, raw header text streaming, and a blank hang window before the first row. These are well-documented in prior PRs (#6170, #6340) and represent genuine user-facing polish gaps during live streaming. Direction: aligned. This is incremental rendering polish for the interactive TUI, a core part of the user experience. Claude Code's CHANGELOG shows a long history of similar streaming/rendering fixes (scroll position jumping, layout jitter, streaming performance improvements), so this area is well within scope. Approach: five focused refinements across four files, all in Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实的、已观察到的 UX 问题。串流中的 markdown 表格在交互式 TUI 中有五种明显的视觉瑕疵——逐格填入、宽度抖动、直式清单再翻横式、表头以纯文字逐字冒出，以及第一列完成前的空白卡住。这些问题在之前的 PR（#6170、#6340）中已有记录，确实是 live 串流过程中的用户体验问题。 方向：对齐。这是交互式 TUI 的渲染打磨，属于核心用户体验的一部分。Claude Code 的 CHANGELOG 中有大量类似的串流/渲染修复（滚动位置跳动、布局抖动、串流性能优化），这个方向在范围内。 方案：四个文件中的五项聚焦改进，全部在 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewReviewed the diff against my independent proposal. The PR's approach matches well — I'd have taken the same general strategy (hold back incomplete table state during streaming, bias toward horizontal format, show empty box early). The implementation is clean and well-scoped. What I looked at:
No critical bugs, no security concerns, no AGENTS.md violations. Code is focused — every change serves the stated goal. Unit TestsAll 211 tests pass. New tests cover all five refinements: atomic rows, width tracking, horizontal bias, header hold-back, empty box rendering, and code block safety. Tmux Real-Scenario TestBoth the installed build ( Before (installed build, v0.19.1)After (this PR via dev build)Note: Print mode ( — Qwen Code · qwen3.7-max |
ReflectionThis PR is a clean, well-scoped follow-up to #6170 and #6340 — each of the five refinements addresses a specific, documented streaming-table artifact. The implementation matches what I'd have done independently: hold back incomplete state at the frontier, bias toward horizontal format, show the empty box early to avoid the blank-hang illusion. The code is straightforward. The 211 unit tests pass, covering all five behaviors plus the code-block safety net. The tmux test in No scope creep, no unrelated changes, no over-abstraction. Every line in the diff serves the stated goal. The comments explain why without being verbose. This is the kind of focused polish PR that's easy to review, revert if needed, and maintain. Approved. ✅ 中文说明反思这个 PR 是 #6170 和 #6340 的干净、聚焦的后续——五项改进中的每一项都针对一个特定的、有记录的串流表格视觉瑕疵。实现方式与我的独立方案一致：在串流前缘压住不完整状态、偏向横式格式、提前显示空表框以避免空白卡住的错觉。 代码简洁明了。 211 个单元测试全部通过，涵盖了所有五种行为以及 code block 安全网。tmux 测试在 没有范围蔓延、没有无关改动、没有过度抽象。diff 中的每一行都服务于既定目标。注释解释了 为什么 而不冗长。这是一种聚焦的打磨 PR——易于审查、必要时易于回滚、易于维护。 已批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
| // columns, NOT because an early row wraps tall. Otherwise a table that will | ||
| // end up horizontal briefly renders as a vertical `label: value` list and | ||
| // then flips (a visible jump) once more rows arrive / it completes. | ||
| const useVerticalFormat = |
There was a problem hiding this comment.
[Suggestion] The zero-row streaming table path produces invisible output on narrow terminals (contentWidth < minHorizontalTableWidth). useVerticalFormat is unconditionally true here regardless of isStreaming, and renderVerticalFormat() iterates empty rowMetrics, producing an empty string wrapped in <Box marginY={1}> — the user sees blank space instead of the intended header box.
The isStreaming flag only suppresses the maxRowLines > MAX_ROW_LINES trigger, not the width trigger. Consider short-circuiting: when rowMetrics.length === 0, always render the horizontal header-only box (header text is typically short enough to fit even on narrow terminals).
| const useVerticalFormat = | |
| const useVerticalFormat = | |
| (rowMetrics.length > 0 && contentWidth < minHorizontalTableWidth) || | |
| (!isStreaming && maxRowLines > MAX_ROW_LINES); |
— qwen3.7-max via Qwen Code /review
Follow-up to the streaming table hold-back, on its own branch so the cue-removal PR (QwenLM#6340) can land undisturbed. Makes a live table stream predictably instead of jittering, flashing, or hanging. - Atomic rows: hold a frontier row back until it has ALL its columns. A multi-column row passes through intermediate states that are themselves valid rows with fewer cells (`| a |`, `| a | b |` toward `| a | b | c |`), so the old hold-back let it fill in cell by cell. Now the whole row (border + every cell) appears in one step. - Widths track the current rows (no freeze): a wider row redraws the whole table once; a narrower row changes nothing (widths are a max over all rows, so they only ever grow). Redraw-on-wider only, never per token. - Bias the streaming preview to the horizontal format: while a table is the live frontier it only falls back to the vertical `label: value` list when the terminal is genuinely too narrow, not because an early row wraps tall. This stops a table from briefly rendering as a vertical list and then flipping to a horizontal table (a visible jump). - Hold a forming table back until it is recognizable: a header (and any partial separator) is trimmed while pending until a separator matching the header's column count arrives, so the header no longer streams in char by char as raw `| a | b |` text before snapping into a box. Fenced code-block content is left untouched. - Draw the empty header box as soon as the table is recognized, before the first row completes, so the table area does not sit blank (no box, no cue) and look like a hang if generation stalls in that window. A zero-row box omits the header/body divider so it reads as a clean header, not an empty row. Only the live frontier table is affected; completed and committed tables use the normal logic. 211 tests pass (MarkdownDisplay + TableRenderer). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
MikeWang0316tw
force-pushed
the
fix/streaming-table-jitter
branch
from
b2b57a0 to
53db9cc
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Good catch — fixed in Zero-row header box on a narrow terminal ( Added a regression test ( |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
[Critical] The downstream safety check at TableRenderer.tsx:767 (maxLineWidth > contentWidth - SAFETY_MARGIN) is a second path to renderVerticalFormat() that is NOT guarded for zero-row tables — the rowMetrics.length > 0 guard at line 604 only protects useVerticalFormat. When the safety fires on a zero-row streaming table (6+ columns on a ~25-col terminal), renderVerticalFormat() iterates empty rowMetrics and returns ''. The header box the PR aims to draw immediately vanishes entirely.
This is the same root cause as the useVerticalFormat guard at line 604 (raised in the prior review thread there), but the fix only covered one of the two fallback paths. The safety check needs the same rowMetrics.length > 0 guard:
if (maxLineWidth > contentWidth - SAFETY_MARGIN && rowMetrics.length > 0) {| if (isPending && renderVisualBlocks && lines.length > 0) { | ||
| let start = lines.length; | ||
| while (start > 0 && /^\s*\|/.test(lines[start - 1]!)) start--; | ||
| if (start < lines.length) { |
There was a problem hiding this comment.
[Critical] The backward scan captures any trailing run of pipe-leading lines, not just table content. When the first captured line doesn't match tableRowRegex (no closing |), headerCells is 0, hasMatchingSeparator is unconditionally false (due to headerCells > 0 &&), and the entire trailing run is trimmed. Non-table text lines starting with | — un-fenced shell pipelines (| grep foo), pipe-prefixed log output, etc. — vanish from the live preview until the message commits.
| if (start < lines.length) { | |
| if (start < lines.length) { | |
| // Don't touch pipe-lines that are really fenced code-block content. | |
| let insideCodeFence = false; | |
| for (let i = 0; i < start; i++) { | |
| if (codeFenceRegex.test(lines[i]!)) insideCodeFence = !insideCodeFence; | |
| } | |
| if (!insideCodeFence) { | |
| // Only hold back when the first pipe-line is a plausible table header | |
| // (has a closing `|`). Otherwise non-table pipe-leading text (shell | |
| // pipelines, log output) would vanish from the live preview. | |
| const headerRowMatch = lines[start]!.match(tableRowRegex); | |
| if (headerRowMatch) { | |
| const headerCells = splitMarkdownTableRow(headerRowMatch[1]) | |
| .filter((c) => c.length > 0).length; | |
| const hasMatchingSeparator = lines.slice(start + 1).some((l) => { | |
| if (!tableSeparatorRegex.test(l)) return false; | |
| const cols = splitMarkdownTableRow(l).filter( | |
| (c) => c.length > 0, | |
| ).length; | |
| return cols === headerCells; | |
| }); | |
| if (!hasMatchingSeparator) { | |
| lines = lines.slice(0, start); | |
| } | |
| } | |
| } | |
| } |
— qwen3.7-max via Qwen Code /review
Review follow-up (two [Critical] findings). - TableRenderer: the maxLineWidth safety check is a second path to the vertical format, unguarded for zero-row tables. On a very narrow terminal a zero-row streaming header box would fall through it and render an empty string — the box vanishes. Skip that fallback when there are no rows so the header stays visible even if it slightly overflows. - MarkdownDisplay: the pre-loop header hold-back trimmed ANY trailing run of pipe-leading lines. When the first line is not a complete `| … |` row, headerCells was 0 and the run was trimmed anyway — so non-table pipe text (an un-fenced shell pipeline `| grep foo`, pipe-prefixed log output) would vanish from the live preview until commit. Only hold back when the first pipe-line is a plausible table header (a complete row). Tests cover both. 215 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both [Critical] findings fixed in
215 tests pass, lint clean. |
The previous commit (restricting the header hold-back to a complete `| … |` row, to stop non-table pipe text from vanishing) reintroduced the cell-by- cell header flash: while a header is typed (`| Alpha`, `| Alpha | Bet`, …) it is not yet a complete row, so it rendered as raw text. Discriminate by column count instead of closed-ness: a table header has ≥2 columns; a single-pipe line (shell pipeline `| grep foo`, pipe-prefixed log) has one cell. Count cells on the first line whether or not it is closed, and hold the run only when it has ≥2 columns and no matching separator yet. So a multi-column header held mid-type no longer flashes, while single-pipe non- table text still renders (the earlier [Critical] fix stands). A header still typing its very first cell is indistinguishable from a single-pipe line, so it shows briefly until the second column appears — the narrowest flash possible without hiding real pipe text. 217 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Heads-up / self-correction: my previous commit Fixed in
One residual by necessity: a header still typing its first cell is indistinguishable from a single-pipe line, so it shows briefly until the second column appears — the narrowest flash possible without hiding real pipe text. Added a test for the incomplete multi-column header. 217 tests pass, lint clean. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No high-confidence issues found. Three low-confidence observations for human review:
-
Multi-pipe shell commands (
| grep error | sort | uniq) outside code fences are held back during streaming because they produce ≥2 non-empty cells, passing the table-header heuristic. Impact is narrow — multi-pipe commands in LLM output are almost always inside fenced code blocks, and content reappears when streaming ends. (MarkdownDisplay.tsx:221-233) -
Pre-loop and main-loop column counting use different methods (
.filter(c => c.length > 0)on full line vs. unfiltered regex capture group). They disagree for headers with empty cells like| A | B | |. (MarkdownDisplay.tsx:220 vs :366) -
End-of-content handler removed the
tableRows.length > 0guard too broadly — a committed zero-row table at end-of-content now renders an empty header box, while the same table mid-message still requires rows. Suggested:if (inTable && tableHeaders.length > 0 && (tableRows.length > 0 || isPending)). (MarkdownDisplay.tsx:636)
Build passes. 217 tests pass (148 MarkdownDisplay + 69 TableRenderer). ESLint clean.
— qwen3.7-max via Qwen Code /review
The horizontal-vs-vertical bias (force a live table horizontal while streaming) backfired for tables that genuinely belong in the vertical `label: value` format — a wide table with many columns of long, wrapping text. It rendered horizontal (tall, clamped, looking stuck) while it was the streaming frontier, then flipped to vertical the moment it stopped being the frontier (the next block started) or committed — a visible format flip, and worse than the vertical-list flash it was meant to avoid. Drop the streaming bias: the horizontal-vs-vertical decision is now the same while pending and once committed, so a table never flips format between the two. Removes the now-unused isStreaming / isStreamingFrontier plumbing. Known residual (pre-existing, not from this change): because column widths track content (redraw-on-wider), a borderline table's wrapped-row height can still cross the vertical threshold mid-stream. Fully stabilizing that needs a content-independent format decision — a separate change. 217 tests pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Document the accepted limitation next to the horizontal-vs-vertical decision: because column widths track content (redraw-on-wider), a table with very long cell text sitting right at MAX_ROW_LINES can still oscillate format while streaming. Only extreme wide/long-text tables hit it; the alternatives (content-independent decision, or frozen widths) each cost more than the residual is worth. Comment-only; no behaviour change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Update since the earlier review (description refreshed to match):
All commits will squash on merge. |
| // narrowest flash we can allow without hiding real non-table text.) | ||
| const headerCells = insideCodeFence | ||
| ? 0 | ||
| : splitMarkdownTableRow(lines[start]!).filter((c) => c.length > 0) |
There was a problem hiding this comment.
[Suggestion] The pre-loop header column count disagrees with the main loop's counting method. Here splitMarkdownTableRow(lines[start]!).filter((c) => c.length > 0) splits the full line (including outer pipes) and filters empty strings. The main loop at line 366 uses splitMarkdownTableRow(tableRowMatch[1]) on the captured group (outer pipes stripped) without filtering.
For a header with an empty cell like | A || B | (no space between pipes), the pre-loop counts 2 columns (the "" between || is filtered out), while the main loop counts 3. The pre-loop then fails to find a matching 3-column separator, and the table is held back for the entire streaming session.
| : splitMarkdownTableRow(lines[start]!).filter((c) => c.length > 0) | |
| let hdr = lines[start]!.replace(/^\s*\|/, ''); | |
| if (/\|\s*$/.test(hdr)) hdr = hdr.replace(/\|\s*$/, ''); | |
| const headerCells = insideCodeFence | |
| ? 0 | |
| : splitMarkdownTableRow(hdr).length; |
— qwen3.7-max via Qwen Code /review
The streaming hold-back counted header columns on the full line with empty cells filtered out, while the main table detector strips the outer pipes and splits without filtering. For a header with an empty-named column like `| A || B |` the two disagreed (2 vs 3), so the hold-back never found the matching 3-column separator and hid the table for the whole stream. Count columns the same way in both places. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| if (/\|\s*$/.test(hdr)) hdr = hdr.replace(/\|\s*$/, ''); | ||
| headerCells = splitMarkdownTableRow(hdr).length; | ||
| } | ||
| if (headerCells >= 2) { |
There was a problem hiding this comment.
[Suggestion] The headerCells >= 2 threshold correctly exempts single-pipe lines like | grep foo, but multi-cell pipe-leading content — e.g. shell pipelines (| grep foo | wc -l), log excerpts (| 200 | OK | GET /api/users), or ASCII-art borders (|=====|======|) — is held back for the entire streaming duration when no matching separator ever arrives. After stripping the leading pipe, splitMarkdownTableRow("grep foo | wc -l") produces ["grep foo", "wc -l"] (2 cells), so headerCells >= 2 passes and the whole trailing run is trimmed.
Consider a line-count limit or heuristic to release non-table pipe content: if the trailing run exceeds N lines without a separator, assume it is not a forming table and stop holding it back. Alternatively, require the first line to more closely match a table header pattern (word-like cells only, no shell operators).
— qwen3.7-max via Qwen Code /review
The streaming hold-back keeps a run of pipe-lines back until a matching separator arrives, so a real multi-column header does not flash in cell by cell. But multi-cell non-table pipe content — a shell pipeline (`| grep foo | wc -l`), a log excerpt (`| 200 | OK | GET /x`), an ASCII-art border — also has >=2 cells, so it was held for the entire stream and only appeared on commit. A markdown table's separator is the line immediately after the header, so once a line follows the header and does not even begin like a separator (optional pipe, optional colon, then a dash), the run is decided: not a forming table. Release it. A lone header still being typed (no line after it yet) is still held, so the no-flash behavior is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the [Suggestion] on multi-cell non-table pipe content (commit — normal push, no force-push). Rather than a line-count limit, I keyed on the markdown rule that a table's separator is the line immediately after the header. So once a line follows the held header and doesn't even begin like a separator (optional pipe, optional colon, then a dash), the run is decided as not-a-table and released:
Added a test for the released log-excerpt case. 221 tests pass (152 MarkdownDisplay + 69 TableRenderer), lint clean. |
The horizontal-vs-vertical choice used maxRowLines measured over EVERY row, so a table that started horizontal (short first row) flipped to vertical the moment a later, taller-wrapping row streamed in — a visible mid-stream format change. Measure only the header + the first data row instead. The first row is representative for the common case, so the format is decided once and stays put as rows append. Column widths still track all rows (redraw-on-wider is unchanged); only the format choice is anchored. Trade-off: a table whose first row is short but a later row wraps very tall stays a (taller) horizontal grid rather than flipping to vertical — rare, and preferable to a visible flip. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Follow-up (commit — normal push, no force-push): the horizontal-vs-vertical format choice no longer flips mid-stream. Previously Trade-off: a table whose first row is short but a later row wraps very tall stays a (taller) horizontal grid rather than flipping — rare, and preferable to a visible flip. This tightens the earlier "format decision consistent, not streaming-biased" commit (it was consistent for a given row set, but the set grew during streaming). Added a TableRenderer test (short first row + tall later row stays horizontal). 222 tests pass (70 TableRenderer + 152 MarkdownDisplay), lint clean. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No high-confidence issues found. Downgraded from Approve to Comment: CI still running.
Summary: Well-scoped display-only improvements to streaming table rendering. 222 tests pass including 20 new test cases. The changes are thoughtfully implemented with honest documentation of accepted trade-offs.
Low-confidence observations for human review:
-
couldStillBeTableregex (MarkdownDisplay.tsx:251) is slightly too permissive — it matches data rows whose first cell starts with a dash (e.g.| --verbose | Enable verbose output |after a multi-column header). Such a table would be held back for the entire streaming session. Consider tightening withtableSeparatorRegex.test(lineAfterHeader). -
calculateMaxRowLines(TableRenderer.tsx:556) measures the first row againstcolumnWidthscomputed from all rows, so the vertical-format decision can still flip when a later wider row forces column redistribution. Acknowledged as an accepted limitation in the PR description.
— qwen3.7-max via Qwen Code /review
The "could this pipe run still become a table?" check treated any line after the header that merely started with a dash as a possible separator, so an options table whose first data cell begins with a flag — `| --verbose | … |` — was held back for the whole stream. Use tableSeparatorRegex instead: it still matches a partial separator being typed (`|--`) so a real header is held until its separator lands, but rejects a dash-led data cell (trailing letters), which now renders live. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…lip) A recognized table with no complete data row yet was drawn immediately as an empty header box. A zero-row table can only render horizontally (the vertical fallback needs rows), so once a long first row landed the box flipped to the vertical label:value format — a visible format change that cannot be avoided by looking at the header alone (column names are short; width comes from the values). Defer the table while pending until its first row completes, so it first appears already in its final format with no flip. Cost: the table area stays blank while the header + first row stream (the pre-loop trim already hid the header text, so this only extends that blank). Committed tables always have rows, so their behavior is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Two follow-up commits (normal push, no force-push): 1. Release a dash-led data row from the hold-back — addresses observation ① from the last review. The 2. Defer a streaming table until its first row (no empty-box flip) — a recognized table with no complete data row yet was drawn immediately as an empty header box. A zero-row table can only render horizontally (the vertical fallback needs rows to lay out), so once a long first row landed the box flipped to the vertical Together with the earlier first-row-anchored format decision, a streaming table no longer flips horizontal↔vertical at any point. Tests: 156 MarkdownDisplay + 70 TableRenderer pass, lint clean. |
| // Don't touch pipe-lines that are actually fenced code-block content. | ||
| let insideCodeFence = false; | ||
| for (let i = 0; i < start; i++) { | ||
| if (codeFenceRegex.test(lines[i]!)) insideCodeFence = !insideCodeFence; |
There was a problem hiding this comment.
[Critical] The code fence tracking here uses a naive boolean toggle (insideCodeFence = !insideCodeFence) that only checks whether a line matches ANY fence pattern, without validating fence character or length. The main parser (lines 67-72) correctly validates that the closing fence uses the same character and has equal or greater length (codeFenceMatch[1].startsWith(activeCodeFence[0]) && codeFenceMatch[1].length >= activeCodeFence.length).
This causes content inside nested code blocks to silently vanish during streaming. For example, streaming this with isPending=true:
| code example |
| A | B |
The toggle sees line 0 (4 backticks) → true, line 2 (3 backticks) → false. So | A | B | at line 3 is treated as a forming table header and held back — invisible to the user until the message commits.
| if (codeFenceRegex.test(lines[i]!)) insideCodeFence = !insideCodeFence; | |
| let activeCodeFence = ''; | |
| for (let i = 0; i < start; i++) { | |
| const fm = lines[i]!.match(codeFenceRegex); | |
| if (fm) { | |
| if (activeCodeFence) { | |
| if (fm[1].startsWith(activeCodeFence[0]) && fm[1].length >= activeCodeFence.length) { | |
| activeCodeFence = ''; | |
| } | |
| } else { | |
| activeCodeFence = fm[1]; | |
| } | |
| } | |
| } | |
| const insideCodeFence = activeCodeFence !== ''; |
— qwen3.7-max via Qwen Code /review
| if ( | ||
| inTable && | ||
| tableHeaders.length > 0 && | ||
| (tableRows.length > 0 || !isPending) |
There was a problem hiding this comment.
[Suggestion] This condition allows committed zero-row tables to render at end-of-content (tableRows.length > 0 || !isPending), but the mid-content end-of-table handler (line 456) still requires tableRows.length > 0 unconditionally. A committed zero-row table mid-content (followed by non-table text) is silently dropped, while the same table at end-of-content renders a header-only box.
Before this PR both branches agreed on tableRows.length > 0. Consider updating line 456 to match, or adding a test documenting the intended difference.
— qwen3.7-max via Qwen Code /review
| // yet) keep holding so a multi-column header does not flash in cell by | ||
| // cell before its separator arrives. | ||
| const lineAfterHeader = rest[0]; | ||
| const couldStillBeTable = |
There was a problem hiding this comment.
[Suggestion] When a header has 3 columns but the separator has 2, couldStillBeTable is true (the line matches tableSeparatorRegex) but hasMatchingSeparator is false (column count mismatch). Since the separator is already complete and won't gain columns, the entire pipe run is held back for the remainder of the stream.
Consider adding a check: if the separator is complete (has closing |) with wrong column count, release the content rather than holding it indefinitely:
if (lineAfterHeader !== undefined && /\|\s*$/.test(lineAfterHeader)) {
const sepCols = splitMarkdownTableRow(lineAfterHeader).filter((c) => c.length > 0).length;
if (sepCols !== headerCells) couldStillBeTable = false;
}— qwen3.7-max via Qwen Code /review
| // in. Column WIDTHS still track all rows (redraw-on-wider is unchanged); only | ||
| // the horizontal-vs-vertical CHOICE is anchored to the first row. | ||
| function calculateMaxRowLines(): number { | ||
| let maxLines = 1; |
There was a problem hiding this comment.
[Suggestion] calculateMaxRowLines was changed to measure only the header + first data row, but this applies to committed (non-streaming) tables as well — TableRenderer has no isPending/isStreaming prop. A committed table with a short first row and much taller later rows no longer triggers the vertical fallback, staying horizontal when vertical would be more readable.
The format-flip stability concern only applies during streaming. Consider passing an isStreaming flag and iterating all rows when not pending:
const rowsToMeasure = isPending ? (firstRow ? [firstRow] : []) : rowMetrics;— qwen3.7-max via Qwen Code /review
| }); | ||
| }); | ||
|
|
||
| describe('streaming table rendering (isStreaming)', () => { |
There was a problem hiding this comment.
[Suggestion] The describe block is named (isStreaming) but TableRenderer has no isStreaming prop — it has headers, rows, contentWidth, etc. The tests within don't pass any such prop either. Consider renaming to match what's actually tested:
| describe('streaming table rendering (isStreaming)', () => { | |
| describe('streaming table rendering (zero-row and format stability)', () => { |
— qwen3.7-max via Qwen Code /review
… committed format Five review findings: - [Critical] The pre-loop hold-back's code-fence check used a naive boolean toggle that ignored fence char/length, so a nested fence (```` with an inner ```) mis-closed and a real code line like `| A | B |` was held back and vanished while streaming. Track the open fence's delimiter and validate the close (same char, >= length), mirroring the main parser. - A COMPLETE separator whose column count already differs from the header can never match, so release the pipe run instead of holding it for the whole stream (the main parser treats it as text). - The end-of-content table flush now uses the same `tableRows.length > 0` guard as the mid-content handler, so a degenerate zero-row table behaves the same whichever way it ends — no EOF-vs-mid asymmetry. - TableRenderer's first-row-only maxRowLines (no-flip) applied to committed tables too; a committed short-first-row + tall-later-row table wrongly stayed horizontal. Gate on a new `isPending` prop: measure the first row only while streaming, every row once committed (most readable, no flip concern). - Renamed the test block that claimed a nonexistent `isStreaming` prop; added committed-vs-streaming format tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed all five findings from the last review (one commit, normal push):
231 tests pass (160 MarkdownDisplay + 71 TableRenderer), lint clean. |
2 participants
What this PR does
Follow-up polish to how a live (streaming) markdown table renders in the non-VP TUI, so it streams predictably instead of jittering, flashing, or briefly hanging. All changes are display-only.
| a |,| a | b |on the way to| a | b | c |), so it used to fill in cell by cell. A frontier row is now held back until it has all its columns and appears in one step. The hold-back keys on column count (a table header has ≥2 columns), so a single-pipe line — a shell pipeline| grep foo, a pipe-prefixed log line — still renders and does not vanish.label: valuefallback is the same while streaming and once committed, so a table never flips format mid-stream (which reads as a jump).| a | b |text before snapping into a box. Fenced code-block content is left untouched.Why it's needed
Building on #6170 (incremental scrollback commit) and #6340 (drop the redundant cue + hold back partial rows), a streaming table still had visible rough edges: cells trickling in one at a time, the whole table re-widening on a wider cell, the header streaming in as raw pipe text, and — after the header/first-row hold-back — a window where the table area was blank until the first row completed (which looked like a hang). Each is a display-side artifact; none changes the model output, the markdown, or the table's data.
Reviewer Test Plan
How to verify
Ask the model for a long multi-column markdown table (~60 rows) and watch the live stream at two terminal sizes (e.g. 80×20 and a large window):
| … |text — the box appears once the separator lands;| grep foo) still renders (it is not held back);label: valuelist) while streaming and once committed — it does not flip;Unit tests:
npx vitest run packages/cli/src/ui/utils/TableRenderer.test.tsx packages/cli/src/ui/utils/MarkdownDisplay.test.tsx→ all pass.Evidence (Before & After)
| a | b |text streaming in; and after the row hold-back there is a blank window (no box) until the first row completes.Tested on
Environment (optional)
Local
npm run devon macOS, streamed against a live model.Risk & Scope
isPendingwhere relevant. The rendered-height clip / safety-net is unchanged, so there is no scroll-to-top regression.Linked Issues
Relates to #6170 (incremental scrollback commit) and #6340 (cue removal + partial-row hold-back this builds on). No closing keyword.
中文说明
这个 PR 做了什么
延续对 non-VP TUI 中串流中 markdown 表格渲染的打磨，让它串流时更稳定，不再抖动、闪烁或短暂空白。全部是显示层改动。
| grep foo、pipe 开头日志）仍会显示、不会消失。label: value清单的判定，在串流与提交后一致，表格不会中途翻转（翻转看起来像跳）。| a | b |纯文字逐字冒出。Fenced code block 内容不受影响。为什么需要
在 #6170 与 #6340 的基础上，串流表格仍有可见粗糙处：储存格逐格冒、更宽储存格让整张重画、表头以纯文字逐字冒、以及行 hold-back 后第一列凑满前的一段空白（看起来像卡住）。这些都是显示层现象，不改变模型输出、markdown 或表格资料。
Reviewer 测试计划
请模型产生长的多栏 markdown 表格（约 60 列），在两种视窗大小观察串流：数值不再逐格冒、栏宽随内容（更宽重画一次、较窄不动）、表头不再纯文字逐字冒、
| grep foo类非表格文字仍显示、第一列串流时不再长时间空白（先显示空表头框）、同一张表串流与提交格式一致（不翻转）、往上滚不跳顶锁死。单元测试：npx vitest run packages/cli/src/ui/utils/TableRenderer.test.tsx packages/cli/src/ui/utils/MarkdownDisplay.test.tsx全通过。风险与范围
关联 Issue
关联 #6170 与 #6340。未使用关闭关键字。