Skip to content

Commit db8e344

Browse files
fix(cli): smoother streaming table rendering (#6345)
* fix(cli): smoother streaming table rendering Follow-up to the streaming table hold-back, on its own branch so the cue-removal PR (#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> * fix(cli): guard the two remaining zero-row / non-table edge cases 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> * fix(cli): hold a multi-column header mid-type without hiding pipe text 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> * fix(cli): make table format decision consistent, not streaming-biased 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> * docs(cli): note the redraw-on-wider format-oscillation trade-off 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> * fix(cli): count held-back header columns like the table detector 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> * fix(cli): release multi-cell non-table pipe content during streaming 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> * fix(cli): anchor the vertical-format decision to the first row (no flip) 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> * fix(cli): release a dash-led data row from the streaming hold-back 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> * fix(cli): 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), 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> * fix(cli): address review — code-fence tracking, held-back edge cases, 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> * fix(cli): don't flip a completed mid-content table's format at commit A table closed by a following line is complete even while the message keeps streaming, but it was still rendered with the first-row-only format anchor — so a short-first-row + tall-later-row mid-content table showed horizontal and then flipped to vertical the moment the message committed. Split the two concerns that were both riding on `isPending`: - the height clamp still tracks whether the MESSAGE is streaming (so a mid-content table stays bounded and the estimator's clamped cost can't under-estimate the render); - the format anchor now tracks whether THIS TABLE is the streaming frontier. The mid-content flush passes isFrontier={false} → all rows measured → final format now; only the end-of-content (frontier) table anchors to the first row. Renamed TableRenderer's format-anchor prop to `isStreaming` (it is not the message-level pending flag). Added mid-content and tilde-fence tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): don't hold a pipe line inside an open $$ math block The streaming table hold-back tracked code fences so a `| A | B |` code line would render, but not display-math (`$$ … $$`) blocks. The main parser pushes math content verbatim (never as a table), so a `| a | b |` norm/matrix line at the frontier of an open math block was treated as a forming table and blanked until the block closed. Track math fences in the trim's fence scan too, mirroring the main parser's precedence (code block wins, then math), and skip the hold-back while inside one. Addresses the low-confidence review observation on #6345. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 6863268 commit db8e344

4 files changed

Lines changed: 669 additions & 40 deletions

File tree

packages/cli/src/ui/utils/MarkdownDisplay.test.tsx

Lines changed: 294 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
*/
66

77
import { beforeEach, describe, expect, it, vi } from 'vitest';
8+
import stripAnsi from 'strip-ansi';
89
import { MarkdownDisplay } from './MarkdownDisplay.js';
910
import { LoadedSettings } from '../../config/settings.js';
1011
import { renderWithProviders } from '../../test-utils/render.js';
@@ -329,6 +330,24 @@ Some text before.
329330
expect(output).toContain('│');
330331
});
331332

333+
it('holds back a frontier row that is closed but missing columns', () => {
334+
// `| four | five |` on a 3-column table is an intermediate state of
335+
// `| four | five | six |` still being typed: it matches the row regex but
336+
// has too few cells, so it must not render (and fill in cell by cell)
337+
// until every column has arrived.
338+
const text = `| A | B | C |
339+
|---|---|---|
340+
| one | two | three |
341+
| four | five |`.replace(/\n/g, eol);
342+
const { lastFrame } = renderWithProviders(
343+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
344+
);
345+
const output = lastFrame() ?? '';
346+
expect(output).toContain('one');
347+
expect(output).toContain('three');
348+
expect(output).not.toContain('four');
349+
});
350+
332351
it('renders the previously-held frontier row once it terminates', () => {
333352
const text = `| A | B |
334353
|---|---|
@@ -366,6 +385,39 @@ Some text before.
366385
expect(out).toContain('two');
367386
});
368387

388+
it('renders a tall-wrapping table the same way streaming and committed (no flip)', () => {
389+
// The horizontal-vs-vertical decision is identical while pending and once
390+
// committed, so a table never flips format mid-stream (which reads as a
391+
// jump). A tall cell trips the vertical fallback in BOTH renders.
392+
const tallCell = Array.from({ length: 80 }, (_, i) => `w${i}`).join(' ');
393+
const text = `| Col |
394+
|---|
395+
| ${tallCell} |`.replace(/\n/g, eol);
396+
397+
const streaming =
398+
renderWithProviders(
399+
<MarkdownDisplay
400+
{...baseProps}
401+
text={text}
402+
isPending={true}
403+
contentWidth={60}
404+
/>,
405+
).lastFrame() ?? '';
406+
const committed =
407+
renderWithProviders(
408+
<MarkdownDisplay
409+
{...baseProps}
410+
text={text}
411+
isPending={false}
412+
contentWidth={60}
413+
/>,
414+
).lastFrame() ?? '';
415+
416+
// Both vertical (no box border) — same format, no flip between the two.
417+
expect(stripAnsi(streaming)).not.toContain('┌');
418+
expect(stripAnsi(committed)).not.toContain('┌');
419+
});
420+
369421
it('does not hold back a partial row in committed (non-pending) output', () => {
370422
const text = `| A | B |
371423
|---|---|
@@ -391,6 +443,248 @@ Done.`.replace(/\n/g, eol);
391443
expect(output).toContain('Done');
392444
});
393445

446+
it('holds back a forming table header until its separator arrives', () => {
447+
// Header present but no separator yet — must not flash as raw `| a | b |`
448+
// text (streaming in char by char) before the table box appears.
449+
const text = `intro line
450+
| Alpha | Beta |`.replace(/\n/g, eol);
451+
const { lastFrame } = renderWithProviders(
452+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
453+
);
454+
const output = lastFrame() ?? '';
455+
expect(output).toContain('intro line');
456+
expect(output).not.toContain('Alpha');
457+
});
458+
459+
it('holds back a multi-column header still being typed (no cell-by-cell flash)', () => {
460+
// Incomplete header (no closing `|` yet) but already ≥2 columns: it must
461+
// be held, not rendered as raw pipe text, so it does not flash in.
462+
const text = `intro line
463+
| Alpha | Bet`.replace(/\n/g, eol);
464+
const { lastFrame } = renderWithProviders(
465+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
466+
);
467+
const output = lastFrame() ?? '';
468+
expect(output).toContain('intro line');
469+
expect(output).not.toContain('Alpha');
470+
});
471+
472+
it('does not hold back non-table pipe-leading text', () => {
473+
// A trailing pipe-line that is not a complete table header (e.g. a shell
474+
// pipeline or pipe-prefixed log line) must still render, not vanish.
475+
const text = `run:
476+
| grep foo`.replace(/\n/g, eol);
477+
const { lastFrame } = renderWithProviders(
478+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
479+
);
480+
expect(lastFrame() ?? '').toContain('grep foo');
481+
});
482+
483+
it('releases multi-cell non-table pipe content once a non-separator line follows', () => {
484+
// A log excerpt / multi-pipe shell output has ≥2 cells per line, so the
485+
// header heuristic alone would hold it for the whole stream. But the line
486+
// after the "header" is not a separator, so it is not a forming table and
487+
// must render live, not vanish until commit.
488+
const text = `logs:
489+
| 200 | OK | GET /a
490+
| 500 | ERR | GET /b`.replace(/\n/g, eol);
491+
const { lastFrame } = renderWithProviders(
492+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
493+
);
494+
const output = lastFrame() ?? '';
495+
expect(output).toContain('200');
496+
expect(output).toContain('500');
497+
});
498+
499+
it('releases an options table whose first data cell starts with a dash', () => {
500+
// `| --verbose | … |` after a header looks separator-ish to a naive
501+
// "starts with a dash" check and would be held all stream. It is not a
502+
// real separator (trailing letters), so it must render live.
503+
const text = `flags:
504+
| Flag | Description |
505+
| --verbose | Enable verbose output |`.replace(/\n/g, eol);
506+
const { lastFrame } = renderWithProviders(
507+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
508+
);
509+
expect(lastFrame() ?? '').toContain('--verbose');
510+
});
511+
512+
it('does not hold back a header with an empty-named column once its separator matches', () => {
513+
// `| A || B |` is a 3-column table to the renderer (the empty middle cell
514+
// counts). The hold-back must count columns the same way, or it never
515+
// finds the matching 3-column separator and hides the table all stream.
516+
const text = `intro line
517+
| A || B |
518+
| - | - | - |
519+
| x || y |`.replace(/\n/g, eol);
520+
const { lastFrame } = renderWithProviders(
521+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
522+
);
523+
const output = lastFrame() ?? '';
524+
expect(output).toContain('x');
525+
expect(output).toContain('y');
526+
});
527+
528+
it('keeps holding the header while its separator is still being typed', () => {
529+
// A partial separator whose column count does not yet match the header is
530+
// not enough to recognize the table, so the header stays held.
531+
const text = `intro line
532+
| Alpha | Beta |
533+
|--`.replace(/\n/g, eol);
534+
const { lastFrame } = renderWithProviders(
535+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
536+
);
537+
expect((lastFrame() ?? '').includes('Alpha')).toBe(false);
538+
});
539+
540+
it('releases a complete separator whose column count differs from the header', () => {
541+
// Header has 3 columns, the separator is complete (ends with `|`) but has
542+
// only 2 — it can never become a matching separator, and the main parser
543+
// treats it as plain text. So it must render, not be held for the stream.
544+
const text = `| A | B | C |
545+
| --- | --- |`.replace(/\n/g, eol);
546+
const { lastFrame } = renderWithProviders(
547+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
548+
);
549+
expect(lastFrame() ?? '').toContain('A');
550+
});
551+
552+
it('does not hold a pipe line inside a nested (longer) code fence', () => {
553+
// A ```` fence is still open; an inner ``` (shorter) does NOT close it, so a
554+
// `| … |` line after it is code content and must render. A naive fence
555+
// toggle would treat the inner ``` as a close and hold the pipe line back.
556+
const text = `\`\`\`\`
557+
| code example |
558+
\`\`\`
559+
| ZZZ | YYY |`.replace(/\n/g, eol);
560+
const { lastFrame } = renderWithProviders(
561+
<MarkdownDisplay
562+
{...baseProps}
563+
text={text}
564+
isPending={true}
565+
availableTerminalHeight={20}
566+
/>,
567+
);
568+
expect(stripAnsi(lastFrame() ?? '')).toContain('ZZZ');
569+
});
570+
571+
it('does not let a backtick fence close an open tilde fence', () => {
572+
// An open ~~~ fence must not be closed by an inner ``` (different char), so
573+
// the `| … |` line after it stays code content. Guards the fence-char check
574+
// for tilde fences (existing tests only cover backticks).
575+
const text = `~~~
576+
| code |
577+
\`\`\`
578+
| ZZZ | YYY |`.replace(/\n/g, eol);
579+
const { lastFrame } = renderWithProviders(
580+
<MarkdownDisplay
581+
{...baseProps}
582+
text={text}
583+
isPending={true}
584+
availableTerminalHeight={20}
585+
/>,
586+
);
587+
expect(stripAnsi(lastFrame() ?? '')).toContain('ZZZ');
588+
});
589+
590+
it('does not hold a pipe line inside an open display-math block', () => {
591+
// Inside a `$$ … $$` block the main parser pushes every line verbatim as
592+
// math content, never as a table. The hold-back must mirror that: a `| … |`
593+
// line (a norm/matrix row) while the math block is still open is NOT a
594+
// forming table and must render, not be blanked until the block closes.
595+
const text = `$$
596+
| ZZZ | YYY |`.replace(/\n/g, eol);
597+
const { lastFrame } = renderWithProviders(
598+
<MarkdownDisplay
599+
{...baseProps}
600+
text={text}
601+
isPending={true}
602+
availableTerminalHeight={20}
603+
/>,
604+
);
605+
expect(stripAnsi(lastFrame() ?? '')).toContain('ZZZ');
606+
});
607+
608+
it('renders the table once the separator matches the header columns', () => {
609+
const text = `| Alpha | Beta |
610+
|---|---|
611+
| 1 | 2 |`.replace(/\n/g, eol);
612+
const { lastFrame } = renderWithProviders(
613+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
614+
);
615+
const output = lastFrame() ?? '';
616+
expect(output).toContain('Alpha');
617+
expect(output).toContain('│'); // drawn as a table box, not raw text
618+
});
619+
620+
it('defers the table while it has no complete data row (no empty header box)', () => {
621+
// Header + separator recognized but the first row is still being typed. A
622+
// zero-row table can only render horizontally, so drawing the empty box now
623+
// and flipping to vertical once a long first row lands is a visible format
624+
// change. Defer instead: nothing is drawn until the first row completes.
625+
const text = `| Alpha | Beta |
626+
|---|---|
627+
| 1`.replace(/\n/g, eol);
628+
const { lastFrame } = renderWithProviders(
629+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
630+
);
631+
const output = lastFrame() ?? '';
632+
expect(output).not.toContain('Alpha'); // table not drawn yet
633+
expect(output).not.toContain('│');
634+
});
635+
636+
it('draws the table once its first row completes', () => {
637+
// The deferred table appears — already in its final format — as soon as the
638+
// first data row terminates.
639+
const text = `| Alpha | Beta |
640+
|---|---|
641+
| 1 | 2 |`.replace(/\n/g, eol);
642+
const { lastFrame } = renderWithProviders(
643+
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
644+
);
645+
const output = lastFrame() ?? '';
646+
expect(output).toContain('Alpha');
647+
expect(output).toContain('│');
648+
});
649+
650+
it('uses the final (all-rows) format for a completed mid-content table while streaming', () => {
651+
// A table CLOSED by a following line is complete even while the message
652+
// streams on, so its format is decided from all rows now — it must not
653+
// render horizontal and then flip to vertical at commit. Short first row +
654+
// a tall later row → vertical (no box border), not a horizontal grid.
655+
const tall = Array.from({ length: 80 }, (_, i) => `w${i}`).join(' ');
656+
const text = `| A | B |
657+
|---|---|
658+
| x | y |
659+
| ${tall} | y |
660+
trailing text`.replace(/\n/g, eol);
661+
const { lastFrame } = renderWithProviders(
662+
<MarkdownDisplay
663+
{...baseProps}
664+
text={text}
665+
isPending={true}
666+
availableTerminalHeight={40}
667+
/>,
668+
);
669+
const output = stripAnsi(lastFrame() ?? '');
670+
expect(output).toContain('trailing text'); // the table is mid-content
671+
expect(output).not.toContain('┌'); // vertical, no horizontal box
672+
});
673+
674+
it('does not hold back pipe lines inside a pending code block', () => {
675+
// A `|`-leading line that is fenced code-block content must still render.
676+
const text = '```\n| Alpha | Beta |'.replace(/\n/g, eol);
677+
const { lastFrame } = renderWithProviders(
678+
<MarkdownDisplay
679+
{...baseProps}
680+
text={text}
681+
isPending={true}
682+
availableTerminalHeight={20}
683+
/>,
684+
);
685+
expect(stripAnsi(lastFrame() ?? '')).toContain('Alpha');
686+
});
687+
394688
it('renders a single-column table', () => {
395689
const text = `
396690
| Name |

0 commit comments

Comments
 (0)