fix(cli): clip live markdown to the viewport to stop non-VP scrollback replay - #6081
Conversation
|
(Re-run — current diff: 2 files, +203/−1, contiguous head-slice approach, head Thanks for the PR, @chiga0! Template looks good ✓ — all required sections present with variant headings ("Root cause" for "Why it's needed", "Risk & UX" for "Risk & Scope"). On direction: squarely in scope. The non-VP scrollback replay in terminal multiplexers is a real, severe rendering bug — ink's On approach: minimal and focused — 2 files, +203/−1, targets exactly the unbounded live content path. This version uses contiguous head-slicing ( Moving on to code review. 🔍 中文说明(重新运行 — 当前 diff:2 文件,+203/−1,连续头部切片方案,head 感谢贡献,@chiga0! 模板完整 ✓(标题使用了变体名称——"Root cause" 代替 "Why it's needed","Risk & UX" 代替 "Risk & Scope")。 方向:完全在范围内。终端复用器中的非 VP 回滚重播是一个真实且严重的渲染 bug——当 pending 帧溢出时,ink 的 方案:最小且聚焦——2 个文件,+203/−1,精确针对无界实时内容路径。这个版本使用了连续头部切片( 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
|
(Re-run on head 2a. Code reviewIndependent proposal (before reading the diff): given the root cause — pending markdown renders all lines, overflows ink's non- Comparison with the diff: the PR's approach matches this exactly — Reuse check: No critical blockers found. The code is correct, well-commented, and follows project conventions. The new 2b. Real-scenario testingUnit tests (worktree, head
|
| Verifier | Build | ESC[2J count | Method |
|---|---|---|---|
| PR author (chiga0) | before | 239 | stdout hook, cmux |
| PR author (chiga0) | after | 0 | same |
| Maintainer (wenshao) | before | 218 | stdout hook, tmux 80×30 |
| Maintainer (wenshao) | after | 0 | same |
| Maintainer (wenshao, 2nd run) | before | 77 | PTY pipe-pane |
| Maintainer (wenshao, 2nd run) | after | 0 | same |
All verifiers confirm the complete message still commits to <Static> in full. ✅
中文说明
(在 head 1cf52d80d 上重新运行 — 连续头部切片方案)
2a. 代码审查
独立方案(阅读 diff 之前):根因是 pending markdown 渲染所有行,导致 ink 的非 <Static> 帧溢出,在每个 token 上触发 shouldClearTerminalForFrame——修复应限制实时预览高度。最简洁的方案是在渲染前对源行做连续头部切片(而非用 ink 的 overflow="hidden" 裁剪,那会抽稀行)。以 isPending 为门控,确保提交的 <Static> 渲染不受影响。添加与 pending 代码块一致的 "... generating more ..." 提示。
与 diff 对比:PR 的方案与此完全吻合——allLines.slice(0, pendingLineBudget) 加 isPending 门控和提示。2 行预留空间(availableTerminalHeight - 2)是精妙之处,可防止当代码/数学块靠近边界时触发自身内部截断而出现双重提示。MIN_PENDING_CONTENT_LINES = 1 干净地处理了退化下限情况。
复用检查:早期迭代中考虑过 MaxSizedBox 但未在此使用——头部切片方案更简单且避免了行抽稀问题。不存在与现有工具的重复。
未发现关键阻塞问题。 代码正确、注释充分、符合项目规范。新增的 MIN_PENDING_CONTENT_LINES 常量作用域恰当(不与 MaxSizedBox 的下限耦合)。
2b. 真实场景测试
单元测试(worktree, head 1cf52d80d)
MarkdownDisplay.test.tsx: 115/115 通过(含 7 个新增裁剪测试)
MainContent.test.tsx: 14/14 通过
eslint: 改动文件干净
新增测试覆盖:长消息连续头部裁剪、短消息不补齐、已提交消息不裁剪、代码围栏在裁剪边界、数学块双提示防止、无预算直通、退化下限。变异检查可靠——回退修复后 "clips a long pending message" 测试会失败(符合预期)。
Tmux E2E(Linux CI — 非 VP, tmux 80×30, 伪 OpenAI SSE)
在 tmux 中用 400 行流式输出(80ms 间隔)从 worktree 运行 dev 构建。分别测试了有修复和无修复(A/B 通过将 MarkdownDisplay.tsx 换为 merge-base 版本):
修复前(main 分支 MarkdownDisplay.tsx,400 行流式):
ESC[2J(clearTerminal):0
ESC[3J(clearScrollback):0
PTY 字节:49,626
修复后(PR head,400 行流式):
ESC[2J(clearTerminal):0
ESC[3J(clearScrollback):0
PTY 字节:22,092(减少 2.2 倍——正常增量 vs 整个 transcript 重流)
说明: clearTerminal 风暴在此 Linux CI 环境中未复现(可能是 ink 渲染时序与 macOS 不同)。但 PTY 字节减少(49.6 KB → 22.1 KB)证实修复减少了输出量——与 PR 的机制一致。
PR 作者和维护者在 macOS 上的多次独立验证确认风暴已消除:
| 验证者 | 构建 | ESC[2J 次数 | 方法 |
|---|---|---|---|
| PR 作者 (chiga0) | 修复前 | 239 | stdout hook, cmux |
| PR 作者 (chiga0) | 修复后 | 0 | 同上 |
| 维护者 (wenshao) | 修复前 | 218 | stdout hook, tmux 80×30 |
| 维护者 (wenshao) | 修复后 | 0 | 同上 |
| 维护者 (wenshao, 第2次) | 修复前 | 77 | PTY pipe-pane |
| 维护者 (wenshao, 第2次) | 修复后 | 0 | 同上 |
所有验证者确认完整消息仍完整提交到 <Static>。✅
— Qwen Code · qwen3.7-max
|
(Re-run on head Stepping back to look at the whole picture: This PR fixes a real, severe bug — the scrollback replay storm in terminal multiplexers that re-streams the entire transcript on every token during long streaming messages. The root cause analysis is precise (ink's The approach has evolved well through review iterations. The earlier Three independent verifications (PR author + 2 maintainer runs on macOS) all confirm the storm drops to 0 with no data loss. The PTY byte reduction I observed on Linux CI (49.6 KB → 22.1 KB) is consistent with the mechanism even though the storm itself didn't reproduce on this platform. Verdict: safe to merge. The fix is focused, well-tested, and verifiably eliminates a severe UX bug without regressions. No concerns that would block shipping. 中文说明(在 head 退一步看全貌: 这个 PR 修复了一个真实且严重的 bug——终端复用器中的 scrollback 重放风暴,在长流式消息期间每个 token 都会重新推送整个 transcript。根因分析精确(ink 的 方案在审查迭代中不断完善。早期的 三次独立验证(PR 作者 + 维护者 2 次在 macOS 上)均确认风暴降至 0 且无数据丢失。我在 Linux CI 上观察到的 PTY 字节减少(49.6 KB → 22.1 KB)与机制一致,尽管风暴本身在此平台上未复现。 结论:可以合并。 修复聚焦、测试充分、可验证地消除了一个严重的 UX bug 且无回归。没有阻塞发布的顾虑。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
DragonnZhang
left a comment
There was a problem hiding this comment.
LGTM. Clean, minimal fix with correct guard conditions (isPending + availableTerminalHeight !== undefined), proper floor via Math.max(MINIMUM_MAX_HEIGHT, ...), and well-established overflow="hidden" pattern already used throughout the codebase. The flexDirection="column" on the wrapper is consistent with all parent layouts. Tests cover both the clipping and non-padding cases.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: CI still running.
Clean, minimal fix — the <Box maxHeight overflow="hidden"> guard correctly targets the non-Static frame overflow path that triggers ink's shouldClearTerminalForFrame. Tests cover both the clipping and no-padding cases. tsc + eslint clean, all 105 tests pass.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
1 similar comment
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Local verification report (real binaries · tmux · streaming fake-OpenAI)I verified this PR end-to-end on macOS by building two real bundled CLIs — TL;DR
1) E2E: clearTerminal "scroll replay" storm — before vs afterReal binaries in tmux, one long streaming assistant message (plain prose, no code fence):
Before-build log shows each replay re-streaming the whole transcript, with byte size growing as the message grows — a ~25 KB message caused 1.26 MB of cumulative re-writes: After: 0 such writes; the composer stays pinned and the complete message (lines 1…N) commits to scrollback identically to before. This matches your root-cause write-up exactly ( 2) Automated tests + mutation
3)
|
| 构建 | 单条流式消息期间 clearTerminal(\x1b[2J)写入次数 |
|---|---|
修复前(origin/main) |
90、87(两次) |
| 修复后(本 PR) | 0、0(两次) |
修复前的日志显示每次重放都在重新推送整段 transcript,且随消息增长字节数持续变大 —— 一条约 25 KB 的消息累计造成了 1.26 MB 的重复写入:
CLEAR #85 bytes=24330 cumBytes=1136498
CLEAR #87 bytes=24802 cumBytes=1186102
CLEAR #89 bytes=25215 cumBytes=1236119 ← 字节数随消息增长
修复后:0 次此类写入;输入框保持固定位置,完整消息(第 1…N 行)与修复前一样完整提交到 scrollback。这与你的根因分析完全吻合(node_modules/ink/build/ink.js 里的 shouldClearTerminalForFrame → wasOverflowing || (isOverflowing && hadPreviousFrame))。
2)自动化测试 + 变异测试
vitest run …/MarkdownDisplay.test.tsx→ 105 通过。- 变异(承重性检查): 只回退源码修复、保留新增测试 → "clips a long pending message" 失败(
expected 60 to be less than or equal to 10)。✔️ 证明该断言确实在把关。 - 实现正确地用了
maxHeight(而非height)—— 短的 pending 消息按自然高度渲染(1 行),不会补白。✔️(已直接验证)
3)⚠️ 发现 —— 裁剪实际是把预览抽稀(而非连续裁剪)
<Box maxHeight overflow="hidden"> 套在 N 个单行块的列上,当 N 超过预算时不会保留连续的头部/尾部 —— ink 会丢掉均匀间隔的中间行。用你自己单测的输入(60 行 → availableTerminalHeight=10)渲染出来是:
line 3, line 9, line 15, line 21, line 27, line 33, line 39, line 45, line 51, line 57
也就是在整条消息上以步长 6 抽出的 10 行 —— 是断断续续的,而不是一段窗口。已用三种独立方式复现(真实 tmux 终端的多个流速;ink-testing-library 渲染 MarkdownDisplay;裸 Box 与 MaxSizedBox 对比)。作为对照,MaxSizedBox maxHeight=16 对同样内容是连续裁剪:…first 6 lines hidden… 然后 line 7 … line 21。
为什么重要:
- 它只影响预览 / 是瞬态的(提交到
<Static>的最终渲染是完整且连续的),而且远好于修复前的风暴 —— 所以不阻塞合并。 - 但它和 PR 描述里的 UX 说法相矛盾("与 pending 代码块的截断一致" —— 代码块是显示连续的头部 +
… generating more …);而对于 400 行的消息塞进约 28 行预算时,预览会变成对整条消息稀疏、不断跳动的采样。 MaxSizedBox(这里已为MINIMUM_MAX_HEIGHT导入它)同样会限高 → 一样能修风暴,并且是连续裁剪 + 带"还有 N 行被隐藏"的提示。注意:它是逐行测量的,可能无法处理所有 markdown 块类型(表格 / 着色代码),所以直接替换需要核验;另一个方案是对contentBlocks取连续的头部切片。- 新增测试只断言
lineCount <= 10,不检查连续性,因此无法捕捉这个问题。
4)次要测试说明
does not pad a short pending message … 这条测试会剥掉结尾换行(.replace(/\n+$/, '')),因此即便把 maxHeight 改成 height(那会补 19 行空白)它也照样通过。对"不补白"这个意图来说是个偏弱的把关。(当前代码是对的,这里只针对测试本身。)
建议
为修复风暴可以合并 —— 这是一个真实且严重的 bug,本 PR 在不丢数据的前提下消除了它。建议后续(或合并前的小改动)改成连续裁剪(例如 MaxSizedBox 或头部切片),让流式预览不被抽稀;并顺手把"与 pending 代码块一致"那句措辞更正一下。
Method: built both bundles via isolated npm ci worktree; tmux 80×30; fake OpenAI SSE (one line per chunk); \x1b[2J counted via stdout --require hook. Numbers vary with stream rate/throttle — the qualitative result (storm → 0) is stable across runs.
ea7ddac
|
@qwen-code /triage |
DragonnZhang
left a comment
There was a problem hiding this comment.
No review findings. The fix correctly bounds the pending (non-Static) frame height with proper null guards (availableTerminalHeight != null in MainContent, !== undefined in MarkdownDisplay), appropriate minimum floors, and overflow="hidden" — targeting exactly the ink overflow path that triggers the scrollback replay. The latest commit properly fixes the missing null guard that would have produced NaN for maxHeight. Well-tested (105 tests) and minimal in scope.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Maintainer local verification — real build + E2EBuilt the PR head ( How it was verified
1) Primary fix — storm eliminated ✅
Both runs commit the identical complete output (lines 1–400) to 2) Tests / typecheck / lint ✅
3) Non-blocking findings(a) The clip decimates — it does not keep a contiguous tail. ink's
So the transient live preview is gappy/jumpy. This is an acceptable trade-off (strictly better than the storm, and the committed (b) The
ink does not do this — it decimates interior rows throughout; only the single last row is reliably kept. A multi-row block at the bottom loses interior rows (synthetic 4-row tail block under an over-tall reply → 2 of its 4 rows dropped, incl. its top line). So the stated guarantee ("embedded shell … stays visible") is not upheld by the mechanism. If tail-preservation is genuinely wanted, the clip must be bottom-anchored (slice the children, or a (c) Minor: the backstop ( VerdictFunctionally correct and safe to merge — the scrollback-replay storm is verifiably gone (218 → 0), tests / 中文版(完整对应)维护者本地验证 — 真实构建 + E2E从源码构建 PR head ( 验证方法
1)主修复 — storm 已消除 ✅
两次运行都把完全相同的完整输出(第 1–400 行)提交到 2)测试 / 类型检查 / lint ✅
3)非阻塞发现(a) 裁剪是「抽稀」而非保留连续尾部。 ink 的
所以实时预览会「跳空/发抖」。这是可接受的取舍(严格优于 storm,且提交到 (b)
ink 并非如此 —— 它在整段范围内抽稀内部行;只有最后一行能稳定保留。底部的多行块会丢失内部行(合成实验:一个超高回复下方的 4 行尾部块 → 4 行里丢了 2 行,含它的顶行)。因此注释承诺的「embedded shell 保持可见」并不成立。若确实想保留尾部,裁剪必须锚定底部(对子项切片,或用 (c) 次要: backstop( 结论功能正确、可安全合并 —— scrollback 重放 storm 已被可验证地消除(218 → 0),测试 / |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Also affected: AgentChatContent.tsx renders isPending={true} HistoryItemDisplay items outside <Static> with no outer maxHeight backstop — the same scroll-replay vulnerability this PR fixes in MainContent.tsx. Consider applying the same wrapper pattern there.
3ae994d to
ce22396
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)为单个提交。 |
DragonnZhang
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: No high-confidence findings.
This PR fixes the non-VP scroll replay bug by clipping pending (live/streaming) markdown to a viewport budget using a contiguous head-slice approach. The implementation is clean, minimal, and well-tested.
Correctness: The pendingLineBudget math is sound (Math.max(MIN_PENDING_CONTENT_LINES, availableTerminalHeight - 2)). The 2-row reserve correctly prevents double-cue stacking with inner code block (RESERVED_LINES=2) and math block (RESERVED_LINES=3) budgets. Guard conditions are correct: clip activates only when isPending && availableTerminalHeight !== undefined && allLines.length > pendingLineBudget. Committed messages (isPending=false) are never clipped.
Security: No new attack surface. Pure rendering optimization.
Code Quality: Clean separation of concerns with a local MIN_PENDING_CONTENT_LINES constant decoupled from MaxSizedBox. The wrap="truncate" on the cue <Text> correctly prevents wrapping in narrow terminals. Tests comprehensively cover: long pending clip, short pending no-clip, committed message full render, code fence at clip boundary, math block no double-cue, undefined budget, degenerate floor (availableTerminalHeight=1), and content-presence assertions.
LGTM.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No review findings on this revision. The contiguous head-slice approach is clean and well-targeted — proper gating (isPending + availableTerminalHeight !== undefined), correct budget floor (MIN_PENDING_CONTENT_LINES = 1), coordinated inner-block reservation (2 rows prevents double cues for both code and math blocks), and thorough test coverage (7 new tests including boundary, degenerate, and negative scenarios). Build passes locally, 115/115 tests pass.
— qwen3.7-max via Qwen Code /review
✅ Maintainer verification — real tmux E2E (head
|
| Scenario | ESC[2J (clearTerminal) |
ESC[3J |
raw PTY bytes |
|---|---|---|---|
| A — base (pre‑fix), single reply | 77 | 77 | 403,605 |
| B — fix (head), single reply | 0 | 0 | 73,567 |
| T — fix (head), long thought + reply | 0 | 0 | 122,370 |
A → B: 77 → 0 clearTerminal replays, ~5.5× less PTY output.
Mid‑stream, the fix renders a contiguous head + cue (not decimated); base renders every line, overflows, and the visible window scrolls (line 33 → 97 → …) while re‑streaming the whole transcript per token:
FIX (B), mid-stream — contiguous, bounded: BASE (A), mid-stream — overflow, scrolling window:
◆ line 1 line 33
line 2 line 34
… …
line 14 line 53 → next sample: line 97, 98, … (storm)
... generating more ... (no cue, full render past the viewport)
- Committed (non‑pending) message still renders in full (
line 400present in<Static>) ✓ - Thought path is independently bounded (
ThinkMessagetail‑clips toavailableTerminalHeight/3), so thought + reply also produced 0 storms (T) — the reverted backstop leaves no residual storm in the normal thought‑then‑reply flow.
Non‑blocking notes
- Aggregate scope: the clip is per pending item (reply head‑slice + thought's own tail‑clip). A single frame with a simultaneously maxed thought (
H/3) and maxed reply (H−2) could sum slightly over the viewport — a bounded, transient overshoot, not the unbounded per‑token storm this PR fixes. I could not reproduce it in the normal flow (T = 0). Fine to leave the aggregate backstop out. - Degenerate floor: at
availableTerminalHeight ∈ {1,2}the budget floors to 1 → 1 line + cue = 2 rows, which can't fit a 1–2 row area. Purely theoretical (the main‑content area is never that short).
Verdict: mergeable. ✅
🇨🇳 中文版(点击展开)
✅ 维护者本地验证 —— 真实 tmux 端到端(head d6bc34c9e)
我用本 PR head 从源码构建了真实 qwen 二进制,在真实终端里修复前/后复现了「滚动重放风暴」。修复有效、严格优于基线 —— 建议合并。
验证范围
相对 origin/main 的净 diff 是 3 个文件(+210 / −1):MarkdownDisplay.tsx(+45)、MarkdownDisplay.test.tsx(+159)、settings.schema.json(+7)。现在的做法是对 pending markdown 取连续的头部切片(allLines.slice(0, availableTerminalHeight − 2) + ... generating more ... 提示),并且早期那个 MainContent 聚合兜底已被回退(a10af959d)—— 净 diff 不再改动 MainContent.tsx。这解决了早期 overflow="hidden" 版本的行抽稀问题:.slice() 天然连续。
静态检查
MarkdownDisplay.test.tsx:115/115 通过(含 7 个新增裁剪用例:长裁剪、短消息不补齐、已提交不裁剪、代码围栏边界、数学块双提示、无预算、退化下限)。- 改动文件
tsc --noEmit干净、eslint 干净。 settings.schema.json重新生成与当前origin/main逐字节一致 → 干净合并(无关的toolIdleTimeoutMs,无害)。
真实端到端(在终端层测量风暴)
源码方式(tsx)跑真实二进制,tmux 80×30,非‑VP(默认),伪 OpenAI 端点流式返回 400 行回复。clearTerminal 重放从 原始 PTY 输出 用 tmux pipe-pane 统计(数 ESC[2J / ESC[3J),在启动完成之后才开始抓取,因此只测流式渲染。基线(A)=该文件回退到 merge-base;修复(B/T)=原始 head。
| 场景 | ESC[2J(clearTerminal) |
ESC[3J |
原始 PTY 字节 |
|---|---|---|---|
| A —— 基线(修复前),单条回复 | 77 | 77 | 403,605 |
| B —— 修复(head),单条回复 | 0 | 0 | 73,567 |
| T —— 修复(head),长思考 + 回复 | 0 | 0 | 122,370 |
A → B:77 → 0 次 clearTerminal 重放,PTY 输出约减少 5.5 倍。
流式过程中,修复渲染的是连续头部 + 提示(不抽稀);基线渲染全部行、溢出,可见窗口随之滚动(line 33 → 97 → …),且每个 token 都重新流式整段:
- 已提交(非 pending)的消息仍完整渲染(
<Static>中能看到line 400)✓ - 思考路径本身独立受限(
ThinkMessage尾部裁剪到availableTerminalHeight/3),所以**「思考 + 回复」同样是 0 次风暴**(T)—— 回退聚合兜底后,在正常的「先思考后回复」流程里没有残留风暴。
非阻塞备注
- 聚合范围:裁剪是按单个 pending 项做的(回复头部切片 + 思考自带尾部裁剪)。若某一帧里思考(
H/3)与回复(H−2)同时都顶满,总高可能略超视口 —— 这是有界的瞬时溢出,不是本 PR 修复的那种无界逐 token 风暴。正常流程里我复现不出(T = 0)。聚合兜底不加也可以。 - 退化下限:当
availableTerminalHeight ∈ {1,2}时预算下限为 1 → 1 行内容 + 提示 = 2 行,塞不进 1–2 行的区域。纯理论(主内容区不会这么矮)。
结论:可合并。 ✅
Method: real binary via tsx from a PR‑head worktree (npm ci + built core); fake OpenAI SSE (400 lines); tmux 80×30 non‑VP; storm counted from raw PTY (pipe-pane, ESC[2J/ESC[3J); A = MarkdownDisplay.tsx reverted to merge‑base, B/T = pristine head.
…k replay In non-VP (default) mode the committed transcript lives in ink's `<Static>` region; only the live/pending items form the dynamic frame. A long streaming assistant message rendered ALL of its lines (code blocks self-truncate, but plain prose / lists / tables had no overall cap), so the dynamic frame grew past the terminal height. Once that happens ink takes its overflow path and writes `clearTerminal + the entire static transcript + output` on EVERY repaint — i.e. it re-streams the whole conversation on every streamed token. Focused, this is continuous flicker; in a terminal multiplexer (tmux/cmux) the backgrounded tab keeps doing it, so switching back replays the transcript top→bottom for a while. Bound the pending markdown to `availableTerminalHeight` with `maxHeight` + `overflow="hidden"`, which clips only when the content is genuinely too tall (short messages render unpadded) and keeps the dynamic frame within the viewport so the overflow path never fires. The full message still renders uncapped once it commits to `<Static>`. Only applies while pending and when a budget is known (constrainHeight on, non-VP). Verified end-to-end with a stdout hook on the real built CLI in cmux: a long streaming response emitted 239 full-transcript replays/7s before, 0 after. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The per-message clip caps each pending markdown message, but the non-VP pending region renders MULTIPLE items and each is capped individually — several tall pending items (a long thought + a long reply, or a reply + a tool group) can still sum past the terminal height and re-trigger the clearTerminal + full-transcript replay. Wrap the whole pending list in a maxHeight + overflow="hidden" box (only while constrainHeight is on) so the total dynamic frame can never exceed the viewport, regardless of how many/what pending items. overflow keeps the most recent rows (the tail), so interactive prompts / the embedded shell — which live at the bottom — stay visible and only the oldest rows are dropped; ShowMoreLines (Ctrl+S) still reveals the rest. The committed transcript in <Static> is unaffected. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
… height availableTerminalHeight is typed number | undefined; Math.max(1, ...) needs a number, so tsc --build failed. Only clamp when it is a concrete number (and constrainHeight is on); otherwise render unclamped. No behaviour change when a budget exists. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…rkdown clip The MainContent-level maxHeight/overflow wrapper (added to bound the total pending height) had real defects surfaced in review: ink's overflow clips the NEWEST content rather than the oldest (so the active reply / interactive prompt would be hidden), the outer wrapper does not register with OverflowContext so ShowMoreLines' "Ctrl+S" hint is lost when it is the sole clipper, and it adds a row beyond the budget via the sibling ShowMoreLines. It also targeted a multi-item "sum" overflow that was never reproduced. Revert it. The per-item markdown clip in MarkdownDisplay — verified end-to-end to stop the replay for the reported case — remains. A correct aggregate backstop would need MaxSizedBox with overflowDirection="top" and is left as a follow-up if the sum case is confirmed. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…omment
Address review on the pending markdown clip:
- the clip tests asserted only line count; a blanked output would pass
(''.split('\n').length === 1). Assert real content — /line \d+/ for the
clipped case (clip direction is not asserted, only that it is non-blank) and
the exact text for the short case.
- add a control test that a long COMMITTED (isPending=false) message renders in
full, guarding the isPending gate.
- the clip also applies to VP pending items (both MainContent paths pass
availableTerminalHeight when constrainHeight), so correct the "non-VP" note.
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
The previous maxHeight + overflow="hidden" clip DECIMATES — ink drops interspersed rows to fit, so a clipped streaming message rendered garbled (non-contiguous) lines and could erase a code block's own "generating more" indicator (its budget was uncoordinated with the outer clip). Confirmed with the real component: a 30-line message at budget 8 rendered L01,L05,L09,... instead of a clean head. Slice the source lines to a CONTIGUOUS head that fits availableTerminalHeight (reserving one row) and append a single "… generating more …" cue. Output is now readable and contiguous; code blocks retained in the head keep their own truncation. Short/committed messages are unchanged. Strengthen the test to assert contiguous head + cue (locks against a regression back to decimation). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…ence test Address review on the head-slice: - the "generating more" cue now uses ASCII ellipses (matching the file's other truncation cues) and wrap="truncate", so it stays one row on narrow terminals (the budget reserves exactly one row for it). - decouple the pending budget floor from MaxSizedBox's MINIMUM_MAX_HEIGHT via a local MIN_PENDING_CONTENT_LINES (same numeric result, explicit intent). - add a test for a code fence spanning the clip boundary (the EOF inCodeBlock flush path). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…ngle cue) Address maintainer review: - add tests for availableTerminalHeight undefined (guard: not clipped, no cue) and a degenerate budget of 1 (Math.max floor keeps one content line + cue). - assert the short-message case emits no "generating more" cue. - assert the code-fence-boundary case emits at most ONE cue: the head-slice bounds code content to <= availableTerminalHeight - 2 (RenderCodeBlock's own inner budget), so the inner truncation never fires inside a slice — no stacked double indicator. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…ble cue RenderMathBlock reserves RESERVED_LINES=3 (vs code block's 2), so at the head slice boundary a retained math block could hit its own inner "generating more" cue on top of the outer one (confirmed: a $$-fenced pending message emitted two cues). Reserve 2 rows in the head-slice budget (1 for the outer cue + 1 for the widest inner reservation) so no retained block's inner truncation fires within the slice. Add a math-block regression test asserting a single cue, and fix two stale test comments that still referenced the old maxHeight/overflow approach. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
d6bc34c to
1cf52d8
Compare
doudouOUC
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
| // its own cue and we'd stack two. | ||
| const pendingLineBudget = | ||
| isPending && availableTerminalHeight !== undefined | ||
| ? Math.max(MIN_PENDING_CONTENT_LINES, availableTerminalHeight - 2) |
There was a problem hiding this comment.
[Suggestion] The -2 reserve correctly covers code/math block overhead (opening fence consumed by parser + inner RESERVED_LINES up to 3), but tables add 3 rendered border lines (top ┌─┬─┐, middle ├─┼─┤, bottom └─┴─┘) with no source-line counterpart — the separator line |---|---| is consumed during parsing but the 3 borders are generated by RenderTable.
A table that exactly fills the budget (e.g., 1 header + 1 separator + 6 data rows = 8 source lines for pendingLineBudget=8 with availableTerminalHeight=10) renders as: 1 top border + 1 header + 1 middle border + 6 data rows + 1 bottom border = 10 rows, plus the "... generating more ..." cue = 11 rows in a 10-row terminal. This overflows by 1 line, potentially re-triggering the shouldClearTerminalForFrame path this PR fixes.
Consider detecting a table in the slice and reducing the budget by 1 extra row:
const sliceContainsTable = pendingLineBudget !== undefined
&& allLines.slice(0, pendingLineBudget).some((l) => tableSeparatorRegex.test(l));
const effectiveBudget = sliceContainsTable
? Math.max(MIN_PENDING_CONTENT_LINES, availableTerminalHeight! - 3)
: pendingLineBudget;— qwen3.7-max via Qwen Code /review
| // Contiguous head + a "generating more" cue — NOT decimated (ink | ||
| // overflow="hidden" would drop interspersed rows) and NOT blank. | ||
| expect(output).toContain('line 1'); | ||
| expect(output).toContain('line 2'); |
There was a problem hiding this comment.
[Suggestion] All clipping tests assert upper bounds (toBeLessThanOrEqual(10), toBeLessThan(20), etc.) but none assert lower bounds. A regression that over-reserves (e.g., someone changes - 2 to - 5) would still pass all tests: the budget for availableTerminalHeight=10 would be 10 - 5 = 5 lines, which still contains "line 1" and "line 2", still excludes "line 60", and still satisfies lineCount <= 10.
Adding a lower-bound assertion would catch over-clipping regressions:
expect(lineCount).toBeGreaterThanOrEqual(availableTerminalHeight - 2);— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
There was a problem hiding this comment.
No high-confidence findings on this revision. The contiguous head-slice approach is correct and well-targeted:
pendingLineBudgetmath is sound:Math.max(1, availableTerminalHeight - 2)properly floors at 1 and reserves 2 rows (1 for the outer cue + 1 for the widest inner RESERVED_LINES=3 in math blocks).isPendinggate ensures committed messages render in full; negative test coverage confirms this.- Double-cue prevention for code blocks (RESERVED_LINES=2) and math blocks (RESERVED_LINES=3) is verified by dedicated tests.
MIN_PENDING_CONTENT_LINESdecoupled from MaxSizedBox.- ASCII ellipsis cue with
wrap=truncateis consistent and stable. availableTerminalHeight=undefinedand degenerate=1cases are tested.
The two open suggestions from @doudouOUC (table border line overhead, lower-bound assertions) are valid incremental improvements, not correctness blockers.
LGTM.
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No high-confidence review findings on this revision. The contiguous head-slice approach is clean and well-targeted — proper gating (isPending + availableTerminalHeight !== undefined), correct floor via Math.max(MIN_PENDING_CONTENT_LINES, ...), and the 7 new tests cover the key scenarios including code fence spanning, math block double-cue prevention, committed message exemption, and degenerate budgets. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
… lock (QwenLM#6170) * fix(cli): stream long responses into scrollback to stop scroll-to-top lock ## Problem In non-VP (default) mode, scrolling up while the model streams a long reply — especially one containing a markdown table — jumps the viewport to the very top and locks it there until the response finishes (issue QwenLM#5941). Root cause: when the live (below-`<Static>`) frame grows taller than the terminal, ink can no longer do its incremental cursor-up redraw and falls back to clearing + repainting the whole frame from the top on every token. A markdown table renders ~2 rows per data row (TableRenderer draws a separator between every row), so QwenLM#6081's source-line budget under-counted the rendered height and the frame still overflowed for tables / wide CJK text. ## Fix Incremental scrollback streaming + a rendered-height safety net: - useGeminiStream: commit finished chunks of the streaming reply into `<Static>` (scrollback) so the pending live item stays short. The commit is rendered-height-aware (tables count double, wide/CJK lines wrap) and bounded by the live content-area height (threaded via `availableTerminalHeightRef`), with a reserve so it fires before the render-side clip. It commits in a `while` loop and splits only at `findLastSafeSplitPoint` boundaries (never inside a fenced code block). - MarkdownDisplay: a rendered-height-aware slice of the pending preview as a last line of defence — it guarantees the live frame never exceeds the viewport regardless of how the stream is chunked (tables charged at ~2x; non-table lines charged their wrapped height). A completed table renders in full; a table still being written renders live and is clamped by TableRenderer's new `maxHeight`. Result: long replies (and tables) flow smoothly into scrollback, tables draw live, and the viewport never locks to the top. Refs QwenLM#5941, QwenLM#6081 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): address review — share rendered-height estimator + guard edges Follow-up to review feedback on the incremental-scrollback streaming fix: - Extract a shared rendered-height estimator (`pendingRenderedHeight.ts`: `fitPendingSlice` / `estimateWrappedRows` / `isTableStart`) and use it from BOTH the useGeminiStream commit and the MarkdownDisplay safety-net slice, so the two agree on table (block: 2*dataRows + chrome) and wrap accounting instead of diverging. - useGeminiStream: use a conservative content-area fallback (terminalHeight minus a composer reserve) when `availableTerminalHeightRef` is not yet populated, so a short terminal never commits with an over-large budget. - MarkdownDisplay: allow the pending slice to keep 0 lines — a single very wide / CJK line that wraps past the budget now renders only the "generating more" cue instead of one oversized row that would bypass the height bound. - Add missing `useCallback` deps (terminalWidth / terminalHeight / availableTerminalHeightRef) — fixes the CI ESLint failure. - Tests: unit tests for the shared estimator (table detection, zero/negative width, CJK wrapping, cut-before / clamp / keptLines=0 boundaries) and for TableRenderer's `maxHeight` clamp (fit, clip+cue, vertical fallback, undefined passthrough). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): rename shared util to kebab-case to satisfy check-file lint The new pendingRenderedHeight.{ts,test.ts} tripped the check-file/filename-naming-convention (KEBAB_CASE) ESLint rule on new files in packages/cli/src. Rename to pending-rendered-height.{ts,test.ts} and update imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): point imports at renamed pending-rendered-height module The previous rename commit landed the file rename but not the importer edits (a stale pathspec aborted the git add), leaving MarkdownDisplay, useGeminiStream and the test importing the old ./pendingRenderedHeight.js path — a module-not- found in CI. Update the imports to the kebab-case path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): drop unused eslint-disable directive on while(true) reportUnusedDisableDirectives + --max-warnings 0 flags the no-constant-condition disable as an unused directive (the rule doesn't flag while(true) here). Remove it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): unify table parsing in shared module + cover commit edge cases Address the second review pass: - Move splitMarkdownTableRow and the table regexes (TABLE_ROW_RE / TABLE_SEPARATOR_RE) into pending-rendered-height.ts as the single source of truth; MarkdownDisplay now imports them instead of keeping duplicate copies. - isTableStart now also checks the separator's column count matches the header (mirroring the renderer's table detection) so the height estimator and the renderer agree on what is a table. - Tests: shared-module coverage for splitMarkdownTableRow and the isTableStart column-count check; tighten the incremental-commit assertion (budget-relative, requires multiple commits); add coverage for the splitPoint<=0 loop-break guard and for the populated-availableTerminalHeightRef production path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): commit streaming chunks only at block boundaries (no split tables) The incremental scrollback commit could cut a markdown table mid-way (e.g. when the tail row was still streaming): the committed chunk kept the header+rows and rendered as a table, but the continuation started with headerless `| ... |` rows that render as raw text (visible orphaned rows below a table). Only commit at a blank-line block boundary. A table (or list / code block) has no internal blank line, so it is never split into a headerless continuation; a still-streaming table stays pending — bounded in view by MarkdownDisplay's clamp — until it is complete, then commits whole. Tests: the oversized-commit test now uses blank-line-separated content and asserts every committed chunk ends at a block boundary; add a regression test that a streaming table taller than the budget is never committed as a headerless fragment (its header stays with its rows in the pending item). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): don't treat code-fence content as a table in the height estimator fitPendingSlice called isTableStart on every line regardless of fenced-code- block state, so table-like lines inside a ``` block were charged as a table (2*dataRows + chrome) while MarkdownDisplay renders them as code (one row each). Track the code fence and charge fenced lines individually. Share CODE_FENCE_RE from the module (MarkdownDisplay now imports it too) to keep a single source of truth. Adds a unit test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): account for vertical-format table height + tilde code fences Address the third review pass: - (Critical) fitPendingSlice charged the horizontal table height (2*dataRows+5) only, but TableRenderer falls back to the vertical key-value format on a narrow terminal / when cells wrap tall, which is much taller for 3+ column tables. Charge the larger of the horizontal and vertical estimates (dataRows*colCount + separators + marginY), still capped by the clamp — under-charging could let a vertical-format table overflow the viewport and re-introduce the scroll lock. - findLastSafeSplitPoint only recognised triple-backtick fences while the estimator's CODE_FENCE_RE also matches ~~~; a ~~~ block with an internal blank line could be split mid-block. isIndexInsideCodeBlock / findEnclosingCodeBlockStart now track both fence types (matching by fence character). - Tests: vertical-format table cost, ~~~ fence tracking, inline math and multi-backtick spans in splitMarkdownTableRow, and a ~~~ split-safety case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): only charge vertical-format table height on a narrow terminal The prior fix charged the max of the horizontal and vertical table estimates unconditionally, which over-estimated a table's height on a wide terminal (where it actually renders in the shorter horizontal format) and clipped small tables early with a premature "generating more". Mirror TableRenderer's width-based vertical decision (contentWidth < max(24, 6*colCount + 5)) and charge the format it will actually render: horizontal when the terminal is wide enough, vertical only when narrow — so a narrow-terminal vertical render still can't overflow and lock, but a small table on a wide terminal is no longer clipped prematurely. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(cli): fix phantom code fences + read commit width from a live ref Address the fourth review pass: - (Critical) isIndexInsideCodeBlock / findEnclosingCodeBlockStart used indexOf('```', ...) which matches only the first three characters of a longer fence run, so a 4+ backtick/tilde fence was miscounted as two delimiters (phantom close-then-reopen). That could mark a blank line inside a code block as outside it, letting findLastSafeSplitPoint split mid-block and commit an unclosed code block to scrollback. findNextFence now returns the full run length, callers advance past the whole run, and a fence only closes a block opened with the same character and a run at least as long. - The commit loop read height live from availableTerminalHeightRef but width from the render-time closure, so a mid-stream resize handled the two inconsistently. Pair a terminalWidthRef with the height ref and read both live. Tests: a 6-backtick fenced block is not split at its internal blank line. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
What this PR does
Stops the whole transcript from re-scrolling top→bottom in non-VP (default) mode — most visibly when a long task runs in a terminal-multiplexer (tmux/cmux) tab and you switch away and back.
Bounds the live (pending) markdown message height to the viewport budget with
maxHeight+overflow="hidden".Root cause
In non-VP mode the committed transcript lives in ink's
<Static>region; only the live/pending items form the dynamic (non-<Static>) frame. A long streaming assistant message rendered all of its lines — code blocks already self-truncate while pending, but plain prose / lists / tables had no overall height cap — so the dynamic frame grew past the terminal height.Once the dynamic frame exceeds the viewport, ink's
renderInteractiveFrametakes its overflow path and writes:i.e. it clears the screen+scrollback and re-streams the entire transcript on every repaint — which during streaming is every token. While the tab is focused this is continuous flicker/scroll; in a multiplexer the backgrounded tab keeps doing it, so switching back replays the transcript top→bottom for a while before settling.
(This is the same
shouldClearTerminalForFramemechanism as #5798/#6015; #6015 windowed the agent panel — this caps the streaming message, the other unbounded live content.)The fix
MarkdownDisplay, whenisPendingand a height budget is known (constrainHeighton, non-VP), wraps the rendered content in:maxHeightclips only when the content is genuinely too tall (short messages render unpadded), keeping the dynamic frame within the viewport so the overflow path never fires. The full message still renders uncapped once it commits to<Static>.Reviewer Test Plan
End-to-end (the live symptom), verified
Preloaded a
process.stdout.writehook (logs writes containing\x1b[2J) on the real built CLI and ran it in cmux; sent a long pure-text streaming prompt (print 1..400):Output still commits in full to
<Static>; replays only ever fired while the pending frame overflowed.Automated
Adjacent suites green (
HistoryItemDisplay,ConversationMessages). eslint + tsc clean.Risk & UX
<Static>render is unchanged, and tool output (alreadyMaxSizedBox-bounded) is unaffected.Linked
Refs #5798
🤖 Generated with Qwen Code