test(cli): cover bottom-stuck virtualized list behavior#7652
Conversation
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Hi @jay666mnj — thanks for the PR! The code change itself looks interesting, but the PR body is empty, so there's nothing for a reviewer to evaluate the motivation, reproduction, or test plan against.
Could you fill in the PR template? The key sections we need:
- What this PR does — describe the change in prose
- Why it's needed — what user-visible bug does this fix? A link to an issue or a before/after reproduction goes a long way here
- Reviewer Test Plan — how can a reviewer confirm the fix works (steps, expected vs observed behavior, tmux logs or screenshots for TUI changes)
- Risk & Scope and Linked Issues
Once the description is in place, re-run with @qwen-code /triage and we'll pick it up from there. 🙏
中文说明
感谢贡献！代码改动本身看起来有意义，但 PR 描述为空，reviewer 无法评估动机、复现步骤或测试方案。
请填写 PR 模板，重点包括：
- What this PR does — 用文字描述改动内容
- Why it's needed — 修复了什么用户可见的 bug？关联 issue 或提供 before/after 复现会很有帮助
- Reviewer Test Plan — reviewer 如何确认修复有效（步骤、预期 vs 实际行为，TUI 改动请附 tmux 日志或截图）
- Risk & Scope 和 Linked Issues
填写完成后，使用 @qwen-code /triage 重新触发审查。🙏
— Qwen Code · qwen3.8-max-preview
|
@qwen-code /triage |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| expect(frame).toContain('item-0'); | ||
| expect(frame).toContain('item-19'); | ||
| expect(frame.endsWith('item-19')).toBe(true); |
There was a problem hiding this comment.
[Suggestion] The regression test passes without the PR's source changes — verified by running the full test suite against the pre-PR VirtualizedList.tsx. With 20 items, tallEstimate = () => 4, containerHeight = 20, and 10 rerenders, the natural iterative measurement convergence completes fully (~5–6 iterations), so all items render regardless of whether the renderStartIndex/measuredRenderedHeight collapse mechanism exists.
Concrete cost: a future regression in the collapse logic (e.g., an off-by-one in i - 1, or a wrong early-return condition) would ship undetected because this test provides no regression safety net for the code paths it claims to exercise.
Consider reducing the rerender count to 2–3 and asserting frame.endsWith('item-19') at that point (which requires the collapse mechanism since convergence hasn't finished), or adding a separate test with 50+ items where the virtualization window never renders all items and the collapse path is the only way to pass.
— qwen3.7-max via Qwen Code /review
| const renderStartIndex = (() => { | ||
| if (!isStickingToBottom || renderStatic === true || endIndex < 0) { | ||
| return startIndex; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The backward walk recomputes cumulative item heights using heights[key] ?? estimatedItemHeight(i) with the same coercion (Number.isFinite(raw) && raw > 0 ? raw : 0) already baked into the offsets memo above. The existing findLastLE binary-search helper could derive the same result in one call, since heightFromEnd >= viewHeightForEndIndex is equivalent to offsets[i] <= offsets[endIndex + 1] - viewHeightForEndIndex.
Concrete cost: two independent code paths compute per-item heights with identical coercion. If a future change adjusts the coercion in one place but not the other (e.g., adding a minimum-height clamp to offsets), the render range derived from renderStartIndex would disagree with the scroll math derived from offsets, producing either a blank strip or an extra row rendered off-screen.
| const renderStartIndex = (() => { | |
| if (!isStickingToBottom || renderStatic === true || endIndex < 0) { | |
| return startIndex; | |
| } | |
| const renderStartIndex = (() => { | |
| if (!isStickingToBottom || renderStatic === true || endIndex < 0) { | |
| return startIndex; | |
| } | |
| const targetOffset = | |
| (offsets[endIndex + 1] ?? totalHeight) - viewHeightForEndIndex; | |
| const backStart = findLastLE(offsets, targetOffset); | |
| return Math.min(startIndex, Math.max(0, backStart)); | |
| })(); |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: Agent 1a: Line-by-line correctness, Agent 2: Security, Agent 3: Code quality, Agent 4: Performance & efficiency, Agent 5: Test coverage, Agent 6a: Undirected audit — attacker mindset, Agent 6b: Undirected audit — 3 AM oncall mindset, Agent 6c: Undirected audit — six-months-later maintainer, Agent 1b: Removed-behavior audit, Agent 1c: Cross-file tracer, Agent 7: Build & test verification — its prompt was built, but no agent on record was launched with it. Not reviewed: verification — a verifier ran and opened its brief, but no agent was launched with the prompt the CLI built — the launch was written by hand, and the posted findings cannot be counted as verified against it.
— qwen3.7-max via Qwen Code /review
| measuredRenderedHeight !== undefined && | ||
| measuredRenderedHeight < props.containerHeight | ||
| ? measuredRenderedHeight | ||
| : totalHeight, |
There was a problem hiding this comment.
[Suggestion] The measuredRenderedHeight < props.containerHeight true-branch — the actual viewport collapse — is never exercised by any test in the suite.
In the new test, after convergence measuredRenderedHeight = 20 = containerHeight, so the strict < is false and totalHeight is used (same as pre-fix behavior). In the existing SCROLL_TO_ITEM_END tests (containerHeight=5, 20+ items at height 1), measuredRenderedHeight ≥ containerHeight. No test creates a scenario where measuredRenderedHeight is defined and strictly less than containerHeight.
Failure scenario: a bug in this branch (e.g. collapsing when it shouldn't, or computing wrong height) goes undetected — the user-visible result would be either a blank gap below short content (the original bug regressing) or content clipped too short.
Consider adding a test with few items in a large container — e.g. 5 items with estimatedItemHeight = () => 1, containerHeight = 20, SCROLL_TO_ITEM_END. After convergence measuredRenderedHeight = 5 < 20, and rootHeight should collapse to 5. Assert the frame ends immediately after the last item with no trailing blank lines.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. 3 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.
— qwen3.7-max via Qwen Code /review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
— qwen3.7-max via Qwen Code /review
| measuredRenderedHeight !== undefined && | ||
| measuredRenderedHeight < props.containerHeight | ||
| ? measuredRenderedHeight | ||
| : totalHeight, |
There was a problem hiding this comment.
[Suggestion] This PR links Fixes #7485, but that issue was closed as NOT_PLANNED (not reproducible) during this PR's lifecycle — two independent testers (@zjunothing on macOS, @ComplexSimply on Linux with 10+ configurations across both legacy and VP modes) could not reproduce the described gap between conversation content and the input prompt. The closing comment states: "Since this issue was bot-filed and we have no human runtime evidence of the symptom on current code, I'm going to close it as not reproducible." Additionally, the original report described behavior in the legacy (non-VP) rendering path, while this PR only modifies the VP-mode VirtualizedList. The two new tests also pass without the source changes (verified by running them against the pre-PR VirtualizedList.tsx), so they do not demonstrate that the fix changes observable behavior.
Concrete cost: The Fixes #7485 linkage will auto-close an issue that was already closed as not reproducible, and future readers may assume the bug was real and fixed. Consider reframing this PR as a defensive viewport-collapse improvement (the measuredRenderedHeight collapse is a sound optimization for short content) and removing the Fixes #7485 linkage, or providing a runtime reproduction on current main that shows the gap before the fix and its absence after.
— qwen3.7-max via Qwen Code /review
There was a problem hiding this comment.
[Suggestion] This PR links
Fixes #7485, but that issue was closed as NOT_PLANNED (not reproducible) during this PR's lifecycle — two independent testers (@zjunothing on macOS, @ComplexSimply on Linux with 10+ configurations across both legacy and VP modes) could not reproduce the described gap between conversation content and the input prompt. The closing comment states: "Since this issue was bot-filed and we have no human runtime evidence of the symptom on current code, I'm going to close it as not reproducible." Additionally, the original report described behavior in the legacy (non-VP) rendering path, while this PR only modifies the VP-modeVirtualizedList. The two new tests also pass without the source changes (verified by running them against the pre-PRVirtualizedList.tsx), so they do not demonstrate that the fix changes observable behavior.Concrete cost: The
Fixes #7485linkage will auto-close an issue that was already closed as not reproducible, and future readers may assume the bug was real and fixed. Consider reframing this PR as a defensive viewport-collapse improvement (themeasuredRenderedHeightcollapse is a sound optimization for short content) and removing theFixes #7485linkage, or providing a runtime reproduction on currentmainthat shows the gap before the fix and its absence after.— qwen3.7-max via Qwen Code /review
Updated the PR description based on the latest review feedback: removed the \Fixes https://github.com/QwenLM/qwen-code/issues/7485\ auto-closing linkage, reframed the change as a defensive viewport-collapse improvement for the virtualized path, and explicitly noted that this PR does not claim a current-main runtime reproduction for the originally related report.
Review + Linux verification report (real build, tmux)Verdict: the change is functionally safe, but I could not find any reachable state where it changes behavior — both new tests pass without the production change. I recommend keeping the regression tests and dropping (or re-justifying) the production diff. The decisive experimentI ran the PR's full test file against the base implementation (merge-base Both new tests pass without the production change. So the tests pin existing behavior; they do not demonstrate the defect the new code defends against. (This is consistent with the PR's own honesty that the original report #7485 was closed as not reproducible.) Why the new branch is unreachable (bottom-stuck analysis)
Verification evidence (Linux, commit 018707e)
SuggestionPer the repo's "Simplicity First" principle, ~30 lines of production logic guarding a state that appears unreachable is a maintenance cost without observable benefit. Two constructive paths: (1) keep the two new tests (they are good pins of bottom-stuck behavior) and drop the production change, or (2) provide a failing test on main that the change fixes — that would immediately upgrade this from defensive to justified. |
jay666mnj
changed the title
|
Thanks for the verification. I followed path (1): removed the production VirtualizedList.tsx diff and kept the two regression tests as bottom-stuck behavior pins. I also updated the PR title/body to make this explicitly test-only and to avoid claiming a current-main runtime reproduction for #7485. Local verification on Windows: prettier check for VirtualizedList.test.tsx, eslint for VirtualizedList.test.tsx, full VirtualizedList.test.tsx via Vitest (22/22 passed), and git diff --check. @gwinthis |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
中文说明
未发现问题。LGTM！✅
— qwen3.8-max-preview via Qwen Code /review
5 participants
What this PR does
This PR adds regression coverage for bottom-stuck
VirtualizedListlayout behavior in the CLI. The tests pin two current invariants: a bottom-stuck list with overestimated row heights still renders through the final item before excessive natural rerender convergence, and a short bottom-stuck list inside a taller container renders only the measured rows with no trailing blank lines.Why it's needed
Reviewer verification showed the previous production diff did not produce observable behavior changes because the tested states already pass on the base implementation. Following that feedback, this PR now keeps only the useful regression tests as behavior pins for the existing bottom-stuck virtualization contract and removes the unreachable defensive production branch.
Reviewer Test Plan
How to verify
Run the focused
VirtualizedList.test.tsxsuite and the formatting/lint checks for the touched test file. A reviewer should confirm that the new tests pass while the productionVirtualizedList.tsximplementation remains unchanged relative to the PR base.Evidence (Before & After)
N/A - this PR is now test-only. The latest review feedback specifically found no reachable behavior change from the previous production diff, so the production change was removed and only regression coverage remains.
Tested on
Environment (optional)
Windows local workspace, Node.js v22.13.1. Verified with
node node_modules\prettier\bin\prettier.cjs --check packages/cli/src/ui/components/shared/VirtualizedList.test.tsx,node node_modules\eslint\bin\eslint.js packages/cli/src/ui/components/shared/VirtualizedList.test.tsx,cd packages/cli; node ..\..\node_modules\vitest\vitest.mjs run src/ui/components/shared/VirtualizedList.test.tsx --coverage.enabled=false --silent=true --poolOptions.threads.minThreads=1 --poolOptions.threads.maxThreads=1, andgit diff --check.Risk & Scope
Linked Issues
Related to #7485.
中文说明
What this PR does
本 PR 为 CLI 中底部吸附的
VirtualizedList布局行为增加回归覆盖。测试固定两个现有不变量：估算行高偏大的底部吸附列表在过多自然 rerender 收敛前仍能渲染到最后一项；短列表位于更高容器中且底部吸附时，只渲染实际测量行，不在末尾留下额外空行。Why it's needed
维护者验证表明，之前的生产代码 diff 没有带来可观察行为变化，因为相关测试状态在基线实现上已经通过。根据该反馈，本 PR 现在只保留有价值的回归测试，用作现有底部吸附虚拟化契约的行为 pin，并移除不可达的防御性生产分支。
Reviewer Test Plan
How to verify
运行聚焦的
VirtualizedList.test.tsx测试套件，以及触及测试文件的格式化和 lint 检查。Reviewer 应确认新增测试通过，同时生产VirtualizedList.tsx实现相对 PR 基线保持不变。Evidence (Before & After)
不适用 - 本 PR 现在仅包含测试。最新 review 反馈明确指出之前的生产 diff 没有可达行为变化，因此已移除生产改动，只保留回归覆盖。
Tested on
Environment (optional)
Windows 本地工作区，Node.js v22.13.1。已验证命令：
node node_modules\prettier\bin\prettier.cjs --check packages/cli/src/ui/components/shared/VirtualizedList.test.tsx、node node_modules\eslint\bin\eslint.js packages/cli/src/ui/components/shared/VirtualizedList.test.tsx、cd packages/cli; node ..\..\node_modules\vitest\vitest.mjs run src/ui/components/shared/VirtualizedList.test.tsx --coverage.enabled=false --silent=true --poolOptions.threads.minThreads=1 --poolOptions.threads.maxThreads=1和git diff --check。Risk & Scope
Linked Issues
关联 #7485。