fix(cli): keep sticky todo panel compact#3647
Conversation
|
Thanks for taking this on, and thanks for the Windows PowerShell before/after repro. This is very helpful for confirming the actual flicker behavior, not just the unit-level behavior. I reviewed #3647 locally. Overall, the direction looks good to me: keeping the sticky todo panel compact, avoiding duplicate inline/recent A few small points I noticed that may be worth considering:
Extra validation I ran on this PR:
Happy to let this PR carry the fix path. Once you decide whether the optional |
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
Thanks for the detailed review and the extra Windows PowerShell validation. I took your suggestion from #3646 and added a new commit: The update keeps the compact sticky todo behavior from this PR, and also incorporates the AppContainer stabilization idea:
I also checked the import paths after moving the max-visible-items helper into |
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
This PR is a clear superset of #3646: commit a184fd8c ("fix(cli): stabilize sticky todo redraws") matches yiliang114's stabilization commit verbatim — git diff pr-3646 pr-3647 -- AppContainer.tsx is empty. On top of that, this PR adds:
- A recent-history visibility guard that hides the sticky panel for the first 2 history items after a
TodoWritecommits. This directly addresses the gap I raised on #3646: an inlineTodoWritethat has just committed but is still visible at the top of the viewport. - Defensive
clampVisibleTodoCount/getStickyTodoMaxVisibleItemsfor non-finite inputs. - Deterministic
width={contentColumnWidth}(vsflexGrow={1}in #3646) — avoids extra Ink measure passes that can re-trigger the flicker we're trying to fix. height={1}(vsminHeight={1}) + explicit width +wrap="truncate-end"→ guaranteed single-line rows, no edge-case wrap.- An
AppContainerintegration test assertingmeasureElementis not called for status-only updates. This is the test that actually proves the optimization works — unit-level layoutKey assertions don't.
I think the right call is to land this PR and close #3646. Leaving a note there.
A few items to address before merging:
Should fix
1. numberColumnWidth should be derived from visibleTodoCount, not todos.length.
const numberColumnWidth = String(todos.length).length + 2;With todos.length = 10 and visibleTodoCount = 5, only 1.–5. are ever rendered, so width 2 suffices, but the current code allocates 4 — wasting 2 columns of content space cosmetically. Suggested:
const numberColumnWidth = String(visibleTodoCount).length + 2;2. Document the 6 in contentColumnWidth. Magic number that will trip future maintainers:
// 6 = 2 (status icon column) + 2 (border × 2) + 2 (paddingX × 2)
const contentColumnWidth = Math.max(1, width - numberColumnWidth - 6);3. Cover the defensive paths with tests. The branches exist but aren't pinned by any test:
clampVisibleTodoCount(NaN)/clampVisibleTodoCount(Infinity)getStickyTodoMaxVisibleItems(NaN)/getStickyTodoMaxVisibleItems(-1)/getStickyTodoMaxVisibleItems(0)
Without tests, a future refactor can silently regress these.
Worth discussing (non-blocking)
4. The recent-history guard is more aggressive than #3646. Combined with hide-while-pending, the sticky panel never appears for short turns (a TodoWrite followed by a 1-paragraph response). On long turns it eventually catches up.
A cleaner replacement is compact-sticky: instead of hiding while pending/recent, render a single fixed-height progress line (e.g. In progress: Run cli tests (3/7)):
- Avoids the duplicate-render problem (compact ≠ inline full render).
- Keeps the bottom anchor visible during exactly the moments users care about progress.
- Eliminates the item-count heuristic entirely.
Concretely: getStickyTodos returns { mode: 'compact' | 'full', todos }; StickyTodoList renders a one-liner in compact; layout key is stable inside compact mode, so no extra measure churn.
Not blocking on this — landing the flicker fix first is right. Worth a follow-up issue.
5. Surface the heuristic's limitation in code, not just in the PR description. Important context for future maintainers:
// The 2-item threshold is item-count, not line-count. A single long
// gemini response can fill the viewport while still counting as one
// item, so the sticky panel may stay hidden longer than strictly
// necessary. This is acceptable — overshowing risks the duplicate-
// render bug we are fixing.
const MIN_HISTORY_ITEMS_AFTER_TODO_BEFORE_STICKY = 2;
Thanks for the detailed review. I agree with the should-fix items. I will add a small follow-up commit to:
I also agree that compact-sticky is a cleaner possible follow-up, but I would prefer to keep this PR focused on landing the flicker fix first. |
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅ — gpt-5.5 via Qwen Code /review
* fix(cli): keep sticky todo panel compact * fix(cli): stabilize sticky todo redraws * fix(cli): address sticky todo review feedback * fix(cli): size sticky todo number column correctly * fix(cli): address sticky todo review feedback
Summary
What changed:
Current taskspanel compact by limiting visible todo items based on terminal height.... and N more.TodoWriteresult is pending, and also suppress it for very recent committedTodoWriteresults that are still likely visible in the conversation.Why it changed:
TodoWriteoutput is still visible.Reviewer focus:
TodoWriteresult.<Static>does not expose a reliable per-item viewport API.Validation
Commands run:
Prompts / inputs used:
Expected result:
Current taskspanel should stay compact.TodoWriteresult.Observed result:
npm run typecheckpassed.npx prettier --check ...passed.git diff --checkpassed.npm run buildpassed.Quickest reviewer verification path:
TodoWriteresult with many long todo items.Current taskspanel shows only a compact set of one-line rows.... and N more.TodoWriteoutput is still visible.Evidence (output, logs, screenshots, video, JSON, before/after, etc.):
Scope / Risk
Main risk or tradeoff:
TodoWriteitem is currently visible in the terminal viewport. Ink<Static>writes committed history to scrollback and does not expose a reliable per-item viewport API, so this PR uses a conservative recent-history guard instead.Not covered / not validated:
Breaking changes / migration notes:
Testing Matrix
Testing matrix notes:
npx vitest,npm run typecheck, rootnpm run build, and a manual Windows PowerShell before/after repro.Linked Issues / Bugs
Fixes #3638.
Follow-up to #3507.
