fix(cli): stabilize sticky todo redraws - #3646
Closed
yiliang114 wants to merge 1 commit into
Closed
Conversation
Collaborator
Author
|
cc @wenshao @shenyankm for review. This follows the root-cause analysis in #3638 and the sticky todo implementation from #3507. |
Contributor
Thanks for picking this up. This overlaps with my PR #3647. Since this PR also includes additional redraw / measurement stabilization work, I’m happy to close #3647 if maintainers prefer this as the main fix path. The main thing #3647 adds is the Windows PowerShell before/after validation GIF and local repro notes for #3638. Please let me know whether I should close #3647 now, or keep it open until reviewers confirm if any of that evidence should be carried over here. |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
approved these changes
Apr 26, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
wenshao
reviewed
Apr 26, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Two pieces of feedback after a deeper read.
1. Misleading comment in todoSnapshot.test.ts
In the new 'shows history snapshot when pending TodoWrite is executing without result' test, the leading comment contradicts the assertion (the comment closer to the assertion is the correct one):
it('shows history snapshot when pending TodoWrite is executing without result', () => {
// When TodoWrite is still executing (no result yet),
// we should hide the sticky panel to prepare for the result. // ← contradicts the assertion
...
// Should show history snapshot when TodoWrite is executing but no result yet
expect(getStickyTodos(history, pendingHistoryItems)).toEqual([...]);
});Suggested cleanup:
it('shows history snapshot when pending TodoWrite is executing without result', () => {
- // When TodoWrite is still executing (no result yet),
- // we should hide the sticky panel to prepare for the result.
+ // When TodoWrite is still executing (resultDisplay is undefined), there is
+ // no inline rendering to duplicate, so the sticky panel falls back to the
+ // latest history snapshot instead of being hidden.
const history = [makeTodoToolGroup('history task', 1)] as HistoryItem[];
...
- // Should show history snapshot when TodoWrite is executing but no result yet
expect(getStickyTodos(history, pendingHistoryItems)).toEqual([2. Behavior trade-off: the in-stream progress indicator disappears
The semantic change in getStickyTodos — returning null whenever a pending TodoWrite snapshot exists — does kill the flicker, but it hides the sticky panel during exactly the part of the turn when users care most about progress.
The PR body argues "the user still sees the todo list inline during that period." That holds while the inline render is still in view. On long turns with multiple TodoWrite updates and sizable reasoning/tool output between them, the inline TodoWrite block can scroll above the fold while the model keeps streaming, leaving no progress indicator at the bottom until the turn ends. The whole point of a sticky panel is to be the always-visible bottom anchor, and the fix removes that anchor at the very moment it's most useful.
Two asks:
a) Acknowledge the trade-off in the PR body (Scope / Risk section):
Trade-off: during streaming with long output, the inline TodoWrite render may scroll out of view, leaving no progress indicator at the bottom until the turn finishes. Acceptable as a hot-fix; a follow-up will restore an in-stream indicator without reintroducing flicker.
b) Open a follow-up issue for restoring an in-stream indicator. Two directions:
- Compact sticky (cheaper): during pending TodoWrite, render a single fixed-height line (e.g.
In progress: Run cli tests (3/7)) instead of hiding. Fixed height + throttled updates → nomeasureElementchurn → no flicker. Concretely:getStickyTodosreturns{ mode: 'compact' | 'full', todos }instead ofnull;StickyTodoListrenders a one-liner incompact; layoutKey stays stable inside compact mode. - Inline collapse + sticky takeover (cleaner): collapse the inline TodoWrite block to
Updated todos (N items)while pending, let the sticky panel be the canonical full view. Bigger change but eliminates the duplication problem at its root.
Compact-sticky is the smaller delta and probably the right next step.
Collaborator
|
After reading #3647 side-by-side with this one: #3647 has commit
I think the cleanest path is to close this PR in favor of #3647. Your stabilization work is preserved verbatim there. I've left detailed pre-merge feedback on #3647 for the remaining items. Thanks for the quick turnaround on the flicker fix. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Current taskspanel during streaming updates without removing the feature.Root Cause
The flicker is caused by a feedback loop between sticky todo rendering and Ink's dynamic redraw area:
Current taskspanel is rendered in the bottom controls area.TodoWriteupdates and pending history items can update frequently.TodoWriteresult can still be visible while the sticky panel renders the same todo snapshot again.mainControlsRefmeasurement, which affectscontrolsHeightand thenavailableTerminalHeight.Solution
This PR reduces the bottom-area layout churn while keeping the sticky todo feature:
TodoWriteresult is still visible, avoiding duplicate todo rendering in the same frame.... and N morewhen the todo list is longer than the compact panel.StickyTodoListrender path keyed by semantic todo content instead of array identity.id + content + status, for visible content updates;width + maxVisibleItems + id + content, for changes that can affect panel height or wrapping.AppContainer's footer-height measurement dependency list so status-only updates do not unnecessarily triggermeasureElement()andavailableTerminalHeightrecalculation.Validation
Commands run:
Test results:
todoSnapshot,DefaultAppLayout,ScreenReaderAppLayout,StickyTodoList: 4 test files passed, 22 tests passed.AppContainer: 1 test file passed, 50 tests passed.packages/clilint: passed.npm run typecheck: passed.npm run build: passed with existingvscode-ide-companionlint warnings only: 9 warnings, 0 errors.node scripts/check-build-status.js:Build is up-to-date.git diff --check origin/main...HEAD: passed.Prompts / inputs used:
todo_write, long todo lists, and streaming analysis responses so the sticky panel remains active while output is updating.Expected result:
TodoWriteresult.Observed result:
Quickest reviewer verification path:
cd packages/cli npx vitest run src/ui/utils/todoSnapshot.test.ts src/ui/components/StickyTodoList.test.tsx src/ui/layouts/DefaultAppLayout.test.tsx src/ui/layouts/ScreenReaderAppLayout.test.tsx src/ui/AppContainer.test.tsxScope / Risk
TodoWriteresult is visible, the sticky panel is hidden to avoid rendering the same todo list twice. The user still sees the todo list inline during that period; the sticky panel returns once the inline pending result is no longer the active pending display.Testing Matrix
Testing matrix notes:
npm runandnpx vitestcommands.Linked Issues / Bugs
Refs #3638