Skip to content

test(cli): cover bottom-stuck virtualized list behavior #7652

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 5 commits into from
Jul 26, 2026
+76 −0
Merged

test(cli): cover bottom-stuck virtualized list behavior #7652

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8180d13
fix(cli): collapse bottom-stuck virtualized list viewport
jay666mnj Jul 24, 2026
3c0ea5e
Merge branch 'main' into fix-virtualized-list-resume-spacing
jay666mnj Jul 24, 2026
30c5a99
fix(cli): address virtualized list review feedback
jay666mnj Jul 24, 2026
018707e
test(cli): cover short virtualized list collapse
jay666mnj Jul 24, 2026
aeba884
test(cli): keep virtualized list coverage only
jay666mnj Jul 25, 2026
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
38 changes: 38 additions & 0 deletions packages/cli/src/ui/components/shared/VirtualizedList.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,44 @@ describe('<VirtualizedList />', () => {
expect(listRef!.getScrollIndex()).toBe(19);
});

it('collapses bottom-stuck viewport to measured rows when estimates are too tall', async () => {
const tallEstimate = () => 4;

const { lastFrame, rerender } = render(
<VirtualizedList<Item>
data={makeItems(20)}
renderItem={renderItem}
estimatedItemHeight={tallEstimate}
keyExtractor={keyExtractor}
initialScrollIndex={SCROLL_TO_ITEM_END}
containerHeight={20}
width={40}
showScrollbar={false}
/>,
);

for (let i = 0; i < 10; i++) {
rerender(
<VirtualizedList<Item>
data={makeItems(20)}
renderItem={renderItem}
estimatedItemHeight={tallEstimate}
keyExtractor={keyExtractor}
initialScrollIndex={SCROLL_TO_ITEM_END}
containerHeight={20}
width={40}
showScrollbar={false}
/>,
);
await act(async () => {});
}

const frame = lastFrame() ?? '';
expect(frame).toContain('item-0');
expect(frame).toContain('item-19');
expect(frame.endsWith('item-19')).toBe(true);
Comment on lines +128 to +130

@qwen-code-ci-bot qwen-code-ci-bot Jul 24, 2026

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

});

it('targetScrollIndex anchors to that index on first usable render', () => {
type RefShape = VirtualizedListRef<Item>;
let listRef: RefShape | null = null;
Expand Down
47 changes: 44 additions & 3 deletions packages/cli/src/ui/components/shared/VirtualizedList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,9 +490,28 @@ function VirtualizedList<T>(
endIndexOffsetRaw >= offsets.length
? data.length - 1
: Math.min(data.length - 1, endIndexOffsetRaw);
const renderStartIndex = (() => {
if (!isStickingToBottom || renderStatic === true || endIndex < 0) {
return startIndex;
}

@qwen-code-ci-bot qwen-code-ci-bot Jul 24, 2026

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

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


let heightFromEnd = 0;
for (let i = endIndex; i >= 0; i--) {
const item = data[i];
const key = item ? keyExtractor(item, i) : '';
const raw = heights[key] ?? estimatedItemHeight(i);
const height = Number.isFinite(raw) && raw > 0 ? raw : 0;
heightFromEnd += height;
if (heightFromEnd >= viewHeightForEndIndex) {
return Math.min(startIndex, Math.max(0, i - 1));
}
}

return 0;
})();

const topSpacerHeight =
renderStatic === true ? 0 : (offsets[startIndex] ?? 0);
renderStatic === true ? 0 : (offsets[renderStartIndex] ?? 0);
const bottomSpacerHeight = renderStatic
? 0
: totalHeight - (offsets[endIndex + 1] ?? totalHeight);
Expand All @@ -512,8 +531,24 @@ function VirtualizedList<T>(
);
}

const renderRangeStart = renderStatic ? 0 : startIndex;
const renderRangeStart = renderStatic ? 0 : renderStartIndex;
const renderRangeEnd = renderStatic ? data.length - 1 : endIndex;
const measuredRenderedHeight = (() => {
if (!isStickingToBottom || renderStatic === true || renderRangeEnd < 0) {
return undefined;
}

let total = 0;
for (let i = renderRangeStart; i <= renderRangeEnd; i++) {
const item = data[i];
if (!item) return undefined;
const measured = heights[keyExtractor(item, i)];
if (measured === undefined) return undefined;
total += measured;
}

return total;
})();

const renderedItems = useMemo(() => {
if (!isReady) {
Expand Down Expand Up @@ -883,7 +918,13 @@ function VirtualizedList<T>(
// the full `containerHeight`, so scrolling is unaffected.
const rootHeight =
props.containerHeight !== undefined
? Math.min(props.containerHeight, totalHeight)
? Math.min(
props.containerHeight,
measuredRenderedHeight !== undefined &&
measuredRenderedHeight < props.containerHeight
? measuredRenderedHeight
: totalHeight,

@qwen-code-ci-bot qwen-code-ci-bot Jul 24, 2026

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

@doudouOUC doudouOUC Jul 24, 2026

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] 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

@jay666mnj jay666mnj Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[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

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.

)
: '100%';

return (
Expand Down
Loading