Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 142 additions & 0 deletions packages/cli/src/ui/utils/MarkdownDisplay.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import { beforeEach, describe, expect, it, vi } from 'vitest';
import stripAnsi from 'strip-ansi';
import { MarkdownDisplay } from './MarkdownDisplay.js';
import { LoadedSettings } from '../../config/settings.js';
import { renderWithProviders } from '../../test-utils/render.js';
Expand Down Expand Up @@ -329,6 +330,24 @@ Some text before.
expect(output).toContain('│');
});

it('holds back a frontier row that is closed but missing columns', () => {
// `| four | five |` on a 3-column table is an intermediate state of
// `| four | five | six |` still being typed: it matches the row regex but
// has too few cells, so it must not render (and fill in cell by cell)
// until every column has arrived.
const text = `| A | B | C |
|---|---|---|
| one | two | three |
| four | five |`.replace(/\n/g, eol);
const { lastFrame } = renderWithProviders(
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
);
const output = lastFrame() ?? '';
expect(output).toContain('one');
expect(output).toContain('three');
expect(output).not.toContain('four');
});

it('renders the previously-held frontier row once it terminates', () => {
const text = `| A | B |
|---|---|
Expand Down Expand Up @@ -366,6 +385,39 @@ Some text before.
expect(out).toContain('two');
});

it('keeps a streaming frontier table horizontal instead of the vertical list', () => {
// A tall cell trips the vertical fallback, which would render the table as
// a `label: value` list and then flip to a horizontal table once it
// completes (a visible jump). The live streaming frontier table must stay
// horizontal; the committed render may fall back to vertical.
const tallCell = Array.from({ length: 80 }, (_, i) => `w${i}`).join(' ');
const text = `| Col |
|---|
| ${tallCell} |`.replace(/\n/g, eol);

const streaming =
renderWithProviders(
<MarkdownDisplay
{...baseProps}
text={text}
isPending={true}
contentWidth={60}
/>,
).lastFrame() ?? '';
const committed =
renderWithProviders(
<MarkdownDisplay
{...baseProps}
text={text}
isPending={false}
contentWidth={60}
/>,
).lastFrame() ?? '';

expect(stripAnsi(streaming)).toContain('┌');
expect(stripAnsi(committed)).not.toContain('┌');
});

it('does not hold back a partial row in committed (non-pending) output', () => {
const text = `| A | B |
|---|---|
Expand All @@ -391,6 +443,96 @@ Done.`.replace(/\n/g, eol);
expect(output).toContain('Done');
});

it('holds back a forming table header until its separator arrives', () => {
// Header present but no separator yet — must not flash as raw `| a | b |`
// text (streaming in char by char) before the table box appears.
const text = `intro line
| Alpha | Beta |`.replace(/\n/g, eol);
const { lastFrame } = renderWithProviders(
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
);
const output = lastFrame() ?? '';
expect(output).toContain('intro line');
expect(output).not.toContain('Alpha');
});

it('holds back a multi-column header still being typed (no cell-by-cell flash)', () => {
// Incomplete header (no closing `|` yet) but already ≥2 columns: it must
// be held, not rendered as raw pipe text, so it does not flash in.
const text = `intro line
| Alpha | Bet`.replace(/\n/g, eol);
const { lastFrame } = renderWithProviders(
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
);
const output = lastFrame() ?? '';
expect(output).toContain('intro line');
expect(output).not.toContain('Alpha');
});

it('does not hold back non-table pipe-leading text', () => {
// A trailing pipe-line that is not a complete table header (e.g. a shell
// pipeline or pipe-prefixed log line) must still render, not vanish.
const text = `run:
| grep foo`.replace(/\n/g, eol);
const { lastFrame } = renderWithProviders(
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
);
expect(lastFrame() ?? '').toContain('grep foo');
});

it('keeps holding the header while its separator is still being typed', () => {
// A partial separator whose column count does not yet match the header is
// not enough to recognize the table, so the header stays held.
const text = `intro line
| Alpha | Beta |
|--`.replace(/\n/g, eol);
const { lastFrame } = renderWithProviders(
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
);
expect((lastFrame() ?? '').includes('Alpha')).toBe(false);
});

it('renders the table once the separator matches the header columns', () => {
const text = `| Alpha | Beta |
|---|---|
| 1 | 2 |`.replace(/\n/g, eol);
const { lastFrame } = renderWithProviders(
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
);
const output = lastFrame() ?? '';
expect(output).toContain('Alpha');
expect(output).toContain('│'); // drawn as a table box, not raw text
});

it('draws the empty table box once recognized, before the first row completes', () => {
// Header + separator recognized but the first row is still being typed:
// show the header box immediately (empty body) rather than a long blank
// while the first row streams (a stall in that window looked like a hang).
const text = `| Alpha | Beta |
|---|---|
| 1`.replace(/\n/g, eol);
const { lastFrame } = renderWithProviders(
<MarkdownDisplay {...baseProps} text={text} isPending={true} />,
);
const output = lastFrame() ?? '';
expect(output).toContain('Alpha'); // header box drawn with zero rows
expect(output).toContain('│');
});

it('does not hold back pipe lines inside a pending code block', () => {
// A `|`-leading line that is fenced code-block content must still render.
const text = '```\n| Alpha | Beta |'.replace(/\n/g, eol);
const { lastFrame } = renderWithProviders(
<MarkdownDisplay
{...baseProps}
text={text}
isPending={true}
availableTerminalHeight={20}
/>,
);
expect(stripAnsi(lastFrame() ?? '')).toContain('Alpha');
});

it('renders a single-column table', () => {
const text = `
| Name |
Expand Down
105 changes: 86 additions & 19 deletions packages/cli/src/ui/utils/MarkdownDisplay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,51 @@ const MarkdownDisplayInternal: React.FC<MarkdownDisplayProps> = ({
}
}

// Hold back a still-forming table at the streaming frontier. A table is only
// recognized once its separator line (matching the header's column count) has
// arrived; until then the header — and any partial separator — would render as
// raw `| a | b |` text and stream in character by character before snapping
// into a table box. So while pending, trim a trailing run of pipe-lines that
// does not yet contain a matching separator: nothing renders for it until it
// can render as a table. (A run that IS a recognizable table is kept — the
// per-row hold-back below then handles its unterminated frontier row.)
if (isPending && renderVisualBlocks && lines.length > 0) {
let start = lines.length;
while (start > 0 && /^\s*\|/.test(lines[start - 1]!)) start--;
if (start < lines.length) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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

// 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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
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

}
// Only hold back a plausible forming TABLE, not arbitrary pipe text. A
// table header has ≥2 columns; a single-pipe line (an un-fenced shell
// pipeline `| grep foo`, a pipe-prefixed log line) has one cell and must
// render. Count cells on the first line whether or not it is closed yet,
// so a multi-column header held mid-type does not flash in cell by cell
// before its separator arrives. (A header still typing its very first
// cell — one cell so far — is indistinguishable from a single-pipe line,
// so it renders briefly until the second column appears; that is the
// narrowest flash we can allow without hiding real non-table text.)
const headerCells = insideCodeFence
? 0
: splitMarkdownTableRow(lines[start]!).filter((c) => c.length > 0)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
: 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

.length;
if (headerCells >= 2) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

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);
}
}
}
}

/** Parse column alignments from a markdown table separator like `|:---|:---:|---:|` */
const parseTableAligns = (line: string): ColumnAlign[] =>
splitMarkdownTableRow(line)
Expand Down Expand Up @@ -346,6 +391,26 @@ const MarkdownDisplayInternal: React.FC<MarkdownDisplayProps> = ({
} else if (inTable && tableSeparatorMatch) {
// Parse alignment from separator line
tableAligns = parseTableAligns(line);
} else if (
isPending &&
inTable &&
index === lines.length - 1 &&
tableHeaders.length > 0 &&
/^\s*\|/.test(line) &&
(!tableRowMatch ||
splitMarkdownTableRow(tableRowMatch[1]).length < tableHeaders.length)
) {
// Live streaming frontier: the final line is a row still being typed —
// either mid-cell (`| a | b` with no closing `|` yet) or closed but with
// fewer cells than the header (`| a |` while `| a | b | c |` is coming, an
// intermediate that itself matches the row regex). Both would otherwise
// render as a padded row that fills in cell by cell and flips as the
// closing `|`/columns arrive, jittering the frame. Hold the row back until
// it has all its columns: skip it so `inTable` stays set and the
// end-of-content handler renders only the COMPLETE rows as a live table.
// The whole row (border + all cells) then appears in one step. Guarded on
// `tableHeaders.length > 0` so the header + separator never blank out with
// a stray partial line beneath them while the first row is typed.
} else if (inTable && tableRowMatch) {
// Add table row
const cells = splitMarkdownTableRow(tableRowMatch[1]);
Expand All @@ -357,24 +422,6 @@ const MarkdownDisplayInternal: React.FC<MarkdownDisplayProps> = ({
cells.length = tableHeaders.length;
}
tableRows.push(cells);
} else if (
isPending &&
inTable &&
!tableRowMatch &&
index === lines.length - 1 &&
tableHeaders.length > 0 &&
/^\s*\|/.test(line)
) {
// Live streaming frontier: the final line is an unterminated table row or
// separator (`| a | b` with no closing `|` yet). Rendering it as a plain
// text line and then flipping it into the table once the closing `|`
// arrives makes the frame height and column widths oscillate on every
// token, which visibly jitters the footer/composer. Hold it back instead:
// skip it so `inTable` stays set and the end-of-content handler renders
// only the COMPLETE rows as a live table. A partial row never flips in;
// the table appears once its first row terminates and grows one complete
// row at a time. Until then the table is simply not drawn (no header +
// separator with a stray partial line beneath it).
} else if (inTable && !tableRowMatch) {
// End of table — a following line closes it, so this table is COMPLETE
// and renders in full (the rendered-aware slice guarantees a completed
Expand Down Expand Up @@ -577,7 +624,16 @@ const MarkdownDisplayInternal: React.FC<MarkdownDisplayProps> = ({
// slice above keeps preceding content within budget, so it can never overflow
// and lock the terminal. Renders in full once the message commits to <Static>
// (isPending=false → no clamp).
if (inTable && tableHeaders.length > 0 && tableRows.length > 0) {
//
// No `tableRows.length > 0` guard: once the header + separator are recognized
// draw the empty table box right away (its data rows fill in as they complete
// via the per-row hold-back). Otherwise the whole table area stays blank from
// the moment it is recognized until the first row terminates — if generation
// stalls in that window it looks like a hang (no box, no cue). The header does
// NOT flash char by char: the pre-loop trim holds it until the separator lands,
// so the box appears atomically. Committed (isPending=false) tables always have
// rows, so this only affects the live frontier.
if (inTable && tableHeaders.length > 0) {
addContentBlock(
<RenderTable
key={`table-${contentBlocks.length}`}
Expand All @@ -587,6 +643,7 @@ const MarkdownDisplayInternal: React.FC<MarkdownDisplayProps> = ({
aligns={tableAligns}
enableInlineMath={renderVisualBlocks}
isPending={isPending}
isStreamingFrontier={true}
availableTerminalHeight={availableTerminalHeight}
/>,
);
Expand Down Expand Up @@ -901,6 +958,14 @@ interface RenderTableProps {
aligns?: ColumnAlign[];
enableInlineMath?: boolean;
isPending?: boolean;
/**
* True only for the table at the live streaming frontier — the one still
* being written at the end of a pending message. A table closed earlier in
* the same (still pending) message is already COMPLETE, so it must size its
* columns from every row immediately (isStreaming false) instead of staying
* frozen to the first row until the whole message finishes.
*/
isStreamingFrontier?: boolean;
availableTerminalHeight?: number;
}

Expand All @@ -911,6 +976,7 @@ const RenderTableInternal: React.FC<RenderTableProps> = ({
aligns,
enableInlineMath = false,
isPending = false,
isStreamingFrontier = false,
availableTerminalHeight,
}) => {
const maxHeight =
Expand All @@ -925,6 +991,7 @@ const RenderTableInternal: React.FC<RenderTableProps> = ({
aligns={aligns}
enableInlineMath={enableInlineMath}
maxHeight={maxHeight}
isStreaming={isPending && isStreamingFrontier}
/>
);
};
Expand Down
Loading
Loading