fix(cli): enable VP scroll at idle prompt and fix viewport height#4959
Merged
Conversation
Two fixes that unblock flipping `ui.useTerminalBuffer` to default-on: 1. **Scroll/input coexistence** (#4942): VP scroll keys (Shift+Up/Down, PgUp/PgDn, Ctrl+Home/End) and mouse wheel were gated on `!isInputActive`, which is `false` at the idle prompt — exactly when users most want to scroll history. Fix: gate scroll on `!dialogsVisible` instead, so scroll works at idle but is still suppressed when a dialog (Help, Settings, etc.) has its own key handlers. 2. **Key binding disambiguation**: NAVIGATION_UP/DOWN, SELECTION_UP/DOWN, and COMPLETION_UP/DOWN used bare `{ key: 'up' }` without `shift` constraint, so Shift+Up matched both SCROLL_UP and NAVIGATION_UP. Fix: add `shift: false` to all non-scroll arrow bindings. 3. **Viewport height overflow** (#4921): `containerHeight` was set to `availableTerminalHeight` without subtracting the VP header block (AppHeader + DebugModeNotification + Notifications), making the scroll region ~5 rows taller than the physical screen. Fix: measure the VP header with `useBoxMetrics` and subtract dynamically. Closes #4942 Closes #4921 Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
requested changes
Jun 10, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the PR @chiga0 — the content and analysis here are genuinely excellent, but the PR body doesn't follow our PR template. A few things are missing that we need for review:
## What this PR does/## Why it's needed— the template separates these; "Summary" combines them but drops the motivation section## Reviewer Test Planwith the### How to verify,### Evidence (Before & After), and### Tested onsubsections — your "Test plan" covers some of this but lacks the OS table and before/after evidence## Risk & Scope— tradeoffs, what's out of scope, breaking changes## Linked Issues— you reference #4942 and #4921 in the body but not in the required section with closing keywords<details>Chinese translation — the template requires a full Chinese translation of the PR body
Would you mind reformatting to match the template? The substance is all here — just needs the right structure so reviewers (and our automated triage) can process it properly.
中文说明
感谢 @chiga0 的贡献——PR 内容和分析质量很高，但正文没有按照 PR 模板 的格式。缺少以下内容：
## What this PR does/## Why it's needed— 模板将两者分开；"Summary" 合并了但缺少动机部分## Reviewer Test Plan及### How to verify、### Evidence (Before & After)、### Tested on子章节## Risk & Scope— 权衡、范围外内容、破坏性变更## Linked Issues— 正文提到了 #4942 和 #4921 但未在规定的章节中使用关闭关键词<details>中文翻译 — 模板要求 PR 正文的完整中文翻译
麻烦按照模板重新排版，内容都在，只需要调整结构。
— Qwen Code · qwen3.7-max
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. |
matchKeyBinding compared binding.shift === false against key.shift which could be undefined in test mocks (partial Key objects). This caused MemoryDialog, Help, and other tests to fail when NAVIGATION_UP, SELECTION_UP, and COMPLETION_UP gained shift: false constraints. Fix: use !!key.X to normalize undefined to false before comparison, so a binding requiring shift:false matches keys where shift is either false or omitted. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
wenshao
reviewed
Jun 10, 2026
| <OverflowProvider> | ||
| <ScrollableList | ||
| hasFocus={!uiState.isInputActive && !uiState.isEditorDialogOpen} | ||
| hasFocus={!uiState.isEditorDialogOpen && !uiState.dialogsVisible} |
Collaborator
There was a problem hiding this comment.
[Suggestion] !isEditorDialogOpen is redundant here — dialogsVisible (computed in AppContainer.tsx:2336) already includes isEditorDialogOpen in its OR chain. So !isEditorDialogOpen && !dialogsVisible is logically equivalent to just !dialogsVisible.
Suggested change
| hasFocus={!uiState.isEditorDialogOpen && !uiState.dialogsVisible} | |
| hasFocus={!uiState.dialogsVisible} |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in 0304479b0. Removed the redundant !isEditorDialogOpen check — dialogsVisible already includes it in its OR chain.
…rDialogOpen Address wenshao review: dialogsVisible (AppContainer.tsx:2336) already OR-chains isEditorDialogOpen, so the redundant check can be removed. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
chiga0
commented
Jun 10, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
Author
There was a problem hiding this comment.
Review Overview
PR: #4959 — fix(cli): enable VP scroll at idle prompt and fix viewport height
Author: chiga0 (self-review)
HEAD: 0304479b (3 commits, 4 files, +56/-28)
Type: Bug Fix (closes #4942, #4921)
Method: Two-phase blind review (v1.2.0)
Phase 1 — Blind Review (0 findings)
Reviewed all 4 files at full content (not just diffs), plus caller/sibling analysis.
Change 1: Key binding shift-disambiguation (3 files)
shift: falseadded to NAVIGATION/COMPLETION/SELECTION arrow bindings — correctly prevents Shift+Arrow collision with SCROLL commands!!key.Xnormalization inmatchKeyBinding— makesundefined→false, ensuring test mocks with partial Key objects still match correctly- Test matchers updated in parallel, negative test cases added for all 6 affected commands
Change 2: Virtual viewport layout (1 file)
useBoxMetrics(vpHeaderRef)measures actual header height;scrollContainerHeight = Math.max(0, availableTerminalHeight - vpHeaderHeight)fixes overflowhasFocuschanged from!isInputActive && !isEditorDialogOpento!dialogsVisible— removes incorrect scroll suppression at idle prompt, correctly gates on dialog presence only
Phase 2 — Cross-Validation
| Source | Type | Finding | Status | My Verdict |
|---|---|---|---|---|
| wenshao | Suggestion | !isEditorDialogOpen redundant — dialogsVisible OR-chain already includes it |
Fixed (commit 3 0304479b) |
Valid, correctly simplified |
| ci-bot | CR | PR body template format | N/A (format) | Not a code concern |
Additional Audit Coverage
!!normalization correctness: All 5 modifier comparisons use!!key.X. When binding modifier isundefined(don't-care), the!== undefinedcheck skips the comparison entirely, so!!only applies when the binding explicitly setstrue/false. No behavioral regression for existing bindings. ✓useBoxMetricsfirst-render transient: Returns0before Ink measures the header, makingscrollContainerHeighttemporarily equal to fullavailableTerminalHeight. This is a standard Ink pattern — one-frame layout shift, imperceptible in practice. ✓Math.max(0, ...)guard: Handles edge case wherevpHeaderHeight > availableTerminalHeight(e.g., very small terminal + many notifications). Returns0instead of negative. ✓- Shift+Arrow mutual exclusion: SCROLL_UP =
shift: true + up, NAVIGATION_UP =shift: false + up. Mutually exclusive by!!key.shiftcomparison. No overlap possible. ✓ dialogsVisibleOR-chain: Verified in AppContainer.tsx thatdialogsVisibleincludesisEditorDialogOpen, so removing the explicit check is logically equivalent. ✓- Test coverage: 6 commands × positive cases (bare arrow + Ctrl+Arrow) + negative cases (Shift+Arrow + wrong key + wrong modifier). All
createKeycalls properly typed. ✓ - Commit ordering: Commit 1 (core fix) → Commit 2 (
!!normalization, fixes test failures from commit 1) → Commit 3 (address wenshao review). Clean, logical progression. ✓ - Caller impact:
matchKeyBindingis private tokeyMatchers.ts, only called bymatchCommand→createKeyMatchers. No external consumers affected.ScrollableListonly consumed byMainContent.tsx. ✓
Final Verdict — LGTM, Recommend Merge
Clean, focused bug fix with comprehensive test coverage. Three well-structured commits addressing the core issues and review feedback. Zero independent findings from blind review. All cross-validation items resolved.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
| if (useVirtualScroll) { | ||
| const scrollContainerHeight = Math.max( | ||
| 0, | ||
| (uiState.availableTerminalHeight ?? 0) - vpHeaderHeight, |
Collaborator
There was a problem hiding this comment.
[Suggestion] availableTerminalHeight is computed in AppContainer.tsx:2400-2406 as terminalHeight - controlsHeight - staticExtraHeight(3) - MAIN_CONTENT_HEIGHT_RESERVATION(2) - tabBarHeight. The staticExtraHeight = 3 hardcoded at AppContainer.tsx:481 may already be reserving space for the same VP header region (AppHeader + DebugModeNotification + Notifications) that vpHeaderHeight now measures dynamically. If so, subtracting vpHeaderHeight here double-counts the header, making the scroll viewport ~3 rows shorter than intended.
Could you confirm whether staticExtraHeight accounts for the VP header? If it does, consider either:
- Zeroing
staticExtraHeightwhen VP mode is active (pass the VP mode flag to the computation), or - Adding
staticExtraHeightback before subtracting the measured height:
const scrollContainerHeight = Math.max(
0,
(uiState.availableTerminalHeight ?? 0) + uiState.staticExtraHeight - vpHeaderHeight,
);— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
No double-counting. staticExtraHeight=3 and MAIN_CONTENT_HEIGHT_RESERVATION=2 cover input area decorations (separator lines, padding, tips) — they are subtracted for both Static and VP modes. They don't account for the AppHeader.
In Static mode, AppHeader is inside <Static> (scrolls up as new items append), so no height subtraction is needed.
In VP mode, AppHeader is outside the scroll area (<Box flexShrink={0}> above ScrollableList), so vpHeaderHeight correctly subtracts just the VP header block. The two subtractions cover different regions:
Terminal height
- controlsHeight (measured footer: Composer + ExitWarning + LiveAgentPanel)
- staticExtraHeight (3) (input area decorations — applies to both modes)
- RESERVATION (2) (layout padding — applies to both modes)
- tabBarHeight (agent tab bar if active)
= availableTerminalHeight
- vpHeaderHeight (VP only: AppHeader + DebugMode + Notifications)
= scrollContainerHeight
Collaborator
Local E2E verification report (Linux, tmux-driven real CLI)TL;DR: The two target bugs are genuinely fixed — scroll at idle works precisely (#4942) and the viewport height overflow is gone (#4921). However, two new VP-mode blockers surfaced during testing, both reproducible and both bisected to this PR's changes: (1) opening any dialog (e.g. Method
✅ Confirmed fixed
Height overflow (#4921), same 100×30 pane:
Dialog focus gating works as designed: opening a dialog drops ❌ Blocker 1 — first dialog open freezes all input (VP only, introduced by this PR)Repro (100% deterministic): VP on → start CLI with no startup dialogs → type Bisect (dist-level file swap, fresh session each):
⇒ trigger is exactly Root cause chain:
On the baseline this can't happen: Suggested fix (either works):
❌ Blocker 2 — mouse wheel types
|
This was referenced Jun 10, 2026
Collaborator
|
@chiga0 The two blockers from the verification report above are now tracked as standalone issues with full repro + root cause + suggested fixes:
Both have minimal suggested fixes that can land inside this PR (always-acquire in KeypressContext / always-active |
Two pre-existing KeypressContext bugs exposed by the hasFocus change: 1. Raw-mode refcount (#4973): KeypressContext skipped setRawMode(true) when stdin was already raw (wasRaw guard), so it held no ink refcount. When the last ink useInput deactivated (e.g. opening a dialog flipped hasFocus to false), the refcount hit 0 and stdin dropped to cooked mode — all input dead until Enter. Fix: always call setRawMode(true) on mount and setRawMode(false) on cleanup. ink refcounts, so double-acquire is safe. 2. SGR mouse text leak (#4974): SGR mouse sequences (\x1b[<64;50;15M) reached readline, whose CSI parser treated < as a final byte and emitted the trailing 64;50;15M as individual character events — which appeared as typed text in the input box. Fix: detect the \x1b[< CSI in handleKeypress, set a swallowing flag, and discard subsequent events until M/m. Fixes #4973 Fixes #4974 Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
Both fixed in
Both are pre-existing KeypressContext bugs (not VP-specific) — exposed by the 174/174 tests pass. Ready for re-verification. |
KeypressContext now always calls setRawMode(true) on mount (wasRaw guard removed in the previous commit). The useFocus test's mock useStdin lacked setRawMode, causing "setRawMode is not a function". Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
qwen-code-ci-bot
requested changes
Jun 11, 2026
| // sequence into a CSI event (\x1b[<) followed by individual character | ||
| // events (digits, semicolons, M/m). Without this filter, those | ||
| // trailing characters would appear as typed text in the input box. | ||
| if (swallowingSgrMouse) { |
Collaborator
There was a problem hiding this comment.
[Critical] SGR mouse swallowing blocks Ctrl+C and has no timeout recovery.
The swallowingSgrMouse early-return here sits above the Ctrl+C escape hatch (line ~697). When the flag is true, every keypress — including Ctrl+C — is silently dropped. There is no timeout to recover from a stuck state, unlike the paste handler (PASTE_IDLE_TIMEOUT_MS = 1000ms) and kitty handler (KITTY_SEQUENCE_TIMEOUT_MS = 200ms) which both have timeouts and explicit Ctrl+C clearing.
If a terminal delivers a truncated SGR sequence without the M/m terminator (buggy emulator, flaky SSH, buffer overflow), all user input is permanently lost with no recovery path short of killing the terminal. The code's own comment 10 lines later explicitly warns against this exact anti-pattern for the paste handler.
Suggested change
| if (swallowingSgrMouse) { | |
| if (swallowingSgrMouse) { | |
| if (key.ctrl && key.name === 'c') { | |
| swallowingSgrMouse = false; | |
| // fall through to Ctrl+C handler below | |
| } else { | |
| if (key.name === 'm' || key.sequence === 'M') { | |
| swallowingSgrMouse = false; | |
| } | |
| return; | |
| } | |
| } |
Additionally, consider adding a safety timeout (e.g., 500ms) that resets swallowingSgrMouse = false, consistent with the kitty/paste timeouts already in this file.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Fixed in fed78a675. Three changes:
- Ctrl+C escape hatch: when
swallowingSgrMouseis true, Ctrl+C clears the flag and falls through to the Ctrl+C handler below — consistent with paste and kitty handlers. - Safety timeout: reuses
KITTY_SEQUENCE_TIMEOUT_MS(200ms). If noM/mterminator arrives within 200ms (truncated sequence from buggy emulator/SSH), the flag auto-clears. - Cleanup: timeout is cleared in the effect cleanup alongside kitty/paste timeouts.
| let waitingForEnterAfterBackslash = false; | ||
| let rawDataBuffer = Buffer.alloc(0); | ||
| let rawFlushTimeout: NodeJS.Timeout | null = null; | ||
| let swallowingSgrMouse = false; |
Collaborator
There was a problem hiding this comment.
[Suggestion] SGR mouse filtering has no test coverage.
The new state machine (enter via \x1b[<, stay for digits/semicolons, exit on M/m) is non-trivial logic with edge cases — truncated sequences, multi-digit parameters, button press (M) vs release (m) terminators — but has zero tests. No test file exists for KeypressContext at all.
Suggested test cases:
- Full SGR sequence is swallowed (enter → digits → terminator → next key delivered normally)
- Truncated sequence (no terminator) — verify recovery behavior
- Ctrl+C during swallow state — verify escape hatch works
— qwen3.7-max via Qwen Code /review
Collaborator
Author
There was a problem hiding this comment.
Acknowledged. KeypressContext currently has no dedicated test file — the SGR filter, paste handling, kitty buffering, and raw-mode management are all tested indirectly through integration tests (MemoryDialog, Help, ScrollableList, useFocus, etc.).
Adding a dedicated KeypressContext.test.tsx with the suggested SGR cases is a good follow-up but would significantly expand this PR's scope. Tracking for a separate PR.
Address review: the swallowingSgrMouse flag sat above the Ctrl+C handler, blocking all input including Ctrl+C during a stuck state. Fix: - Ctrl+C during SGR swallow clears the flag and falls through to the Ctrl+C handler (consistent with paste and kitty handlers) - Add a safety timeout (reuses KITTY_SEQUENCE_TIMEOUT_MS = 200ms) that auto-clears the flag if no M/m terminator arrives - Clean up timeout in the effect cleanup Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Collaborator
Author
|
@wenshao Update on the fixes since my last reply:
All 180 local tests pass. The SGR filter now:
Ready for re-verification when you have a chance. |
qwen-code-ci-bot
approved these changes
Jun 11, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — qwen3.7-max via Qwen Code /review
DragonnZhang
approved these changes
Jun 11, 2026
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! The six fixes are well-targeted and the code is clean. Key highlights: (1) The shift: false disambiguation is a safe data-only change with comprehensive test coverage. (2) The !!key.X normalization is correct — ink always provides boolean modifiers in production, so this only affects partial Key mocks in tests. (3) The SGR mouse state machine properly handles Ctrl+C escape and has a 200ms timeout safety net. (4) The raw-mode refcount fix is sound since ink's setRawMode is refcounted. — claude-opus-4-6 via Qwen Code /review
chiga0
commented
Jun 11, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
Author
There was a problem hiding this comment.
Re-Review at HEAD fed78a67 (3 new commits since last review)
New Commits
| # | Commit | Description |
|---|---|---|
| 1 | 9d262f1 |
setRawMode refcount fix + SGR mouse filter |
| 2 | 0aecec4 |
Test mock update for unconditional setRawMode |
| 3 | fed78a6 |
Ctrl+C escape hatch + timeout for SGR filter |
ci-bot Finding Audit
| ci-bot Finding | Status | Notes |
|---|---|---|
| Critical: SGR mouse swallowing blocks Ctrl+C and has no timeout | FIXED (commit 3) | Ctrl+C clears swallowingSgrMouse and falls through to normal Ctrl+C handler; sgrMouseTimeout auto-resets after KITTY_SEQUENCE_TIMEOUT_MS |
| Suggestion: SGR filtering has no test coverage | Acknowledged | No dedicated KeypressContext test file yet — noted as tech debt |
Suggestion: availableTerminalHeight reservation overlap |
FP | staticExtraHeight=3 and MAIN_CONTENT_HEIGHT_RESERVATION=2 cover different areas |
Independent Verification (3 new commits)
1. setRawMode refcount fix — Ink internally tracks raw-mode subscribers via a Set. The old wasRaw conditional caused asymmetric inc/dec under StrictMode (first cleanup would dec, second mount wouldn't re-inc). Unconditional setRawMode(true)/setRawMode(false) always balances. ✓
2. SGR mouse state machine — readline's CSI parser treats < as a final byte, splitting \x1b[<params;...M into fragments (\x1b[<, digits, M). The swallowingSgrMouse flag correctly suppresses all fragments. ✓
3. Ctrl+C escape hatch — When swallowingSgrMouse is true, Ctrl+C clears the flag and falls through (no return) to the existing Ctrl+C handler below. User can always escape. ✓
4. Timeout — Reuses KITTY_SEQUENCE_TIMEOUT_MS (already battle-tested for kitty sequences). Timer cleared on M/m terminator or on Ctrl+C. ✓
5. Cleanup on unmount — sgrMouseTimeout cleared, swallowingSgrMouse reset. ✓
6. Test mock — Minimal fixture update adds setRawMode: vi.fn() + isRawModeSupported: true. ✓
Cross-Validation Table
| Reviewer | Finding | Status at HEAD fed78a6 |
|---|---|---|
| ci-bot | SGR Ctrl+C block + no timeout | FIXED (commit 3) |
| ci-bot | No SGR test coverage | Acknowledged (tech debt) |
| ci-bot | Height reservation overlap | FP |
| DragonnZhang | APPROVED at HEAD | — |
Final Verdict — LGTM
All 3 new commits are well-targeted fixes. The SGR mouse state machine is correct, the Ctrl+C escape hatch and timeout provide proper recovery paths, and cleanup is complete. ci-bot and DragonnZhang both APPROVED at HEAD. Recommend merge.
This re-review was generated by QoderWork AI
Collaborator
Re-verification report — head
|
| Step | Pre-fix KeypressContext (A/B swap) |
Head fed78a67 |
|---|---|---|
/help opens |
✅ (and mouse_any_flag 1→0, focus handed to dialog) |
✅ same |
Tab / Esc inside dialog |
❌ dead — dialog never closes | ✅ Esc closes |
| Type immediately after close | ❌ text echoes raw at pane bottom (cooked-mode line buffer), input box dead, process alive | ✅ appears in input box instantly |
| 3× open/close cycles | — (wedged after first) | ✅ all alive, mouse_any_flag 1→0→1 each cycle, no refcount drift |
/settings, ↑/↓ |
— | ✅ navigates items (not conversation), input alive after close |
The pre-fix frozen state is byte-for-byte the signature from my original report: frame static, typed text echoed below the app frame, process alive (debug heartbeat ticking). The wasRaw guard removal fixes it for the reason stated in the PR body — KeypressContext now always holds its own ink refcount, so the count can no longer hit 0 mid-session.
❌→✅ Blocker 2 (#4974) — SGR mouse text leak
Byte-exact wheel events injected at the idle prompt (tmux send-keys -l $'\x1b[<64;50;15M'):
| Pre-fix (A/B swap) | Head fed78a67 |
|
|---|---|---|
| Input box after 2 wheel ticks | ❌ * 64;50;15M65;50;15M |
✅ clean, placeholder untouched |
| List scrolls | ✅ 3 lines/tick | ✅ 3 lines/tick (filter doesn't break the ink mouse pipeline) |
Edge paths added in fed78a675, both verified:
- Timeout rescue: partial
\x1b[<with no terminator → typing 600 ms later lands in the input box normally (200 msKITTY_SEQUENCE_TIMEOUT_MSfired and cleared the swallow flag). - Ctrl+C fall-through: pre-filled input
zz, sent\x1b[<64;50+Ctrl+Cwithin the timeout window → input box cleared (Ctrl+C executed its normal semantics, not swallowed) and typing works immediately after. No leak in either path.
Regression battery (previous ✅ set, re-run at head)
All behaviors from the original report hold:
Shift+Up/Shift+Down: exactly ±1 line per press in steady state; input box never receives recalled history (theshift: falsedisambiguation).PgUp/PgDn: exactly ±1 page (10 rows);Ctrl+Home/Ctrl+Endjump to top/bottom.- Bare
↑still recalls input history (unchanged behavior preserved). - Height (bug(cli): When active "Virtualized History" in /settings, viewport height is taller than expected #4921):
history_sizestays 0 through startup, a full model turn, dialog open/close, wheel/key scrolling, and a 30→20→30 row resize cycle; frame bottom at row 26/30. No scrollback push, no native scrollbar. - One cosmetic observation, not a bug: the first scroll-up press while bottom-anchored moves the visible window ~3 rows (bottom UI slack collapses as the view leaves the anchor); every subsequent press is exactly ±1.
Unit tests
- Changed files:
keyMatchers45,useFocus6,KeypressContext93 — all pass. - Component battery from previous report (
MainContent,ScrollableList,BaseSelectionList,InputPrompt,Help): 206 pass / 2 pre-existing skips. - CI is now fully green including CodeQL and the coverage comment (the two failures noted last time were fork-token infra issues at the old head).
Non-blocking note
The SGR swallow state machine has no unit coverage (KeypressContext.test.tsx has no SGR-mouse cases) — it's currently only covered by this kind of E2E. A small follow-up test (feed \x1b[<64;50;15M through the keypress pipeline, assert no character events escape + flag resets on M/m/Ctrl+C/timeout) would lock the behavior in. Fine as a follow-up; doesn't need to block this PR.
Verdict
All four linked issues (#4942, #4921, #4973, #4974) verified fixed at fed78a67 on Linux, with the failure modes re-demonstrated on the pre-fix build in the same environment. The Linux row of the test matrix can be flipped to ✅. Ready to merge from my perspective.
wenshao
approved these changes
Jun 12, 2026
This was referenced Jun 12, 2026
doudouOUC
pushed a commit
that referenced
this pull request
Jun 15, 2026
) * fix(cli): enable VP scroll at idle prompt and fix viewport height Two fixes that unblock flipping `ui.useTerminalBuffer` to default-on: 1. **Scroll/input coexistence** (#4942): VP scroll keys (Shift+Up/Down, PgUp/PgDn, Ctrl+Home/End) and mouse wheel were gated on `!isInputActive`, which is `false` at the idle prompt — exactly when users most want to scroll history. Fix: gate scroll on `!dialogsVisible` instead, so scroll works at idle but is still suppressed when a dialog (Help, Settings, etc.) has its own key handlers. 2. **Key binding disambiguation**: NAVIGATION_UP/DOWN, SELECTION_UP/DOWN, and COMPLETION_UP/DOWN used bare `{ key: 'up' }` without `shift` constraint, so Shift+Up matched both SCROLL_UP and NAVIGATION_UP. Fix: add `shift: false` to all non-scroll arrow bindings. 3. **Viewport height overflow** (#4921): `containerHeight` was set to `availableTerminalHeight` without subtracting the VP header block (AppHeader + DebugModeNotification + Notifications), making the scroll region ~5 rows taller than the physical screen. Fix: measure the VP header with `useBoxMetrics` and subtract dynamically. Closes #4942 Closes #4921 Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): normalize undefined modifiers to false in key matching matchKeyBinding compared binding.shift === false against key.shift which could be undefined in test mocks (partial Key objects). This caused MemoryDialog, Help, and other tests to fail when NAVIGATION_UP, SELECTION_UP, and COMPLETION_UP gained shift: false constraints. Fix: use !!key.X to normalize undefined to false before comparison, so a binding requiring shift:false matches keys where shift is either false or omitted. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): simplify hasFocus — dialogsVisible already includes isEditorDialogOpen Address wenshao review: dialogsVisible (AppContainer.tsx:2336) already OR-chains isEditorDialogOpen, so the redundant check can be removed. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): fix raw-mode refcount and SGR mouse leak in KeypressContext Two pre-existing KeypressContext bugs exposed by the hasFocus change: 1. Raw-mode refcount (#4973): KeypressContext skipped setRawMode(true) when stdin was already raw (wasRaw guard), so it held no ink refcount. When the last ink useInput deactivated (e.g. opening a dialog flipped hasFocus to false), the refcount hit 0 and stdin dropped to cooked mode — all input dead until Enter. Fix: always call setRawMode(true) on mount and setRawMode(false) on cleanup. ink refcounts, so double-acquire is safe. 2. SGR mouse text leak (#4974): SGR mouse sequences (\x1b[<64;50;15M) reached readline, whose CSI parser treated < as a final byte and emitted the trailing 64;50;15M as individual character events — which appeared as typed text in the input box. Fix: detect the \x1b[< CSI in handleKeypress, set a swallowing flag, and discard subsequent events until M/m. Fixes #4973 Fixes #4974 Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * test(cli): add setRawMode mock to useFocus test fixture KeypressContext now always calls setRawMode(true) on mount (wasRaw guard removed in the previous commit). The useFocus test's mock useStdin lacked setRawMode, causing "setRawMode is not a function". Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> * fix(cli): add Ctrl+C escape hatch and timeout to SGR mouse filter Address review: the swallowingSgrMouse flag sat above the Ctrl+C handler, blocking all input including Ctrl+C during a stuck state. Fix: - Ctrl+C during SGR swallow clears the flag and falls through to the Ctrl+C handler (consistent with paste and kitty handlers) - Add a safety timeout (reuses KITTY_SEQUENCE_TIMEOUT_MS = 200ms) that auto-clears the flag if no M/m terminator arrives - Clean up timeout in the effect cleanup Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com> --------- Co-authored-by: 秦奇 <gary.gq@alibaba-inc.com> Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
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
4 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.
What this PR does
Fixes five issues that block flipping
ui.useTerminalBuffer(Virtual Viewport) to default-on:shift: falsetoNAVIGATION_UP/DOWN,SELECTION_UP/DOWN, andCOMPLETION_UP/DOWNso Shift+Up/Down only triggersSCROLL_UP/DOWN, never input history navigation or dialog selection.ScrollableList'shasFocusfrom!isInputActive && !isEditorDialogOpento!dialogsVisible, so VP scroll keys and mouse wheel work at the idle prompt while dialogs (Help, Settings, etc.) still own their own PgUp/PgDn handlers.useBoxMetricsand subtracts it fromcontainerHeight, eliminating the ~5 extra rows that caused blank lines and a native terminal scrollbar.!!key.Xto normalizeundefined→falsein modifier comparison, soshift: falseconstraints work correctly with partial Key objects in tests.wasRawguard in KeypressContext so it always holds its own ink refcount. Previously, when the last inkuseInputdeactivated (e.g. opening a dialog), the refcount hit 0 and stdin dropped to cooked mode — all input dead until Enter.64;50;15M) into the input box — readline treats<as CSI final byte #4974) — filters SGR mouse sequences (\x1b[<...M/m) in handleKeypress before they reach readline. readline's CSI parser treated<as a final byte, emitting trailing64;50;15Mas typed text in the input box.Why it's needed
VP mode (
ui.useTerminalBuffer) was merged in #4146 as opt-in. Multiple bugs block flipping it to default-on:64;50;15M) into the input box — readline treats<as CSI final byte #4974: mouse wheel scrolling types64;50;15Minto the input box — readline misparses SGR mouse CSI sequences#4973 and #4974 are pre-existing latent bugs in KeypressContext, not VP-specific. They are exposed by this PR's
hasFocuschange which enables SGR mouse mode at the idle prompt (previously it was only active during tool processing, when the user wasn't typing).Design alternatives considered
Evaluated 5 approaches for scroll/input coexistence — click to expand
Approach A:
useBoxMetricsmeasurement +dialogsVisiblegating (chosen)Measure VP header height dynamically with
useBoxMetrics(vpHeaderRef), subtract fromcontainerHeight. Gate scroll activation on!dialogsVisibleinstead of!isInputActive.vpHeaderHeight=0(useBoxMetrics returns 0 before layout)First-frame zero-height is handled by VirtualizedList's
isReadyguard (containerHeight > 0). SGR mouse mode at idle is documented inkeyboard-shortcuts.md.Approach B:
flexGrowlayout — ❌ not viableUse
<Box flexGrow={1}>on the ScrollableList wrapper so it automatically claims remaining vertical space after the VP header.Why it fails:
DefaultAppLayout's root<Box>only setswidth={terminalWidth}with noheightconstraint. Without a known total height, yoga'sflexGrowcannot compute vertical space allocation. Addingheight={terminalHeight}breaks ink'suseInputfocus chain — Composer stops receiving Enter and other keys correctly. Additionally,MainContentreturns a React Fragment (<>), not aBox, soflexGrowcannot be applied without restructuring the component tree.Attempted and reported as broken by the #4942 author.
Approach C: VP-specific
availableTerminalHeightin AppContainerCompute a separate height in
AppContainerbased onuseTerminalBufferstate, using a differentstaticExtraHeightvalue for VP mode.Feasible but less clean than localized measurement in MainContent.
Approach D: Event consumption in KeypressContext
Add
return truesupport to thebroadcast()loop inKeypressContextso handlers can consume events and stop propagation. This would let ScrollableList consume Shift+Up without needing key binding disambiguation.useKeypresscall sitesCorrect long-term direction, but too heavy as a fix for these specific issues.
Approach E: Hardcoded header height subtraction
Subtract a fixed value (e.g. 6 rows for AppHeader) from
containerHeightinstead of measuring.Too fragile for a dynamic UI.
Decision
Approach A — minimal, accurate, localized. The
useBoxMetricsfirst-frame edge case is already handled by VirtualizedList's existingisReadyguard. The key binding disambiguation is a safe data-only change that also fixes a latent bug (Shift+arrow matching non-scroll commands).Reviewer Test Plan
How to verify
jq '.ui.useTerminalBuffer = true' ~/.qwen/settings.json > /tmp/s.json && mv /tmp/s.json ~/.qwen/settings.json)Shift+Up/Shift+Down→ should scroll history by 1 linePgUp/PgDn→ should scroll by one page64;50;15Min input box)↑/↓→ should navigate input history (not scroll)/helpdialog →PgUp/PgDnshould scroll the help list, not the conversation behind it. All keys should work (no cooked-mode freeze)./help→ input should immediately work (no dead keys until Enter)/settingsdialog →↑/↓should navigate settings, not scroll conversationEvidence (Before & After)
Before (main): At idle prompt, Shift+Up/Down, PgUp/PgDn, and mouse wheel do nothing. Bottom of VP has ~5 rows of blank space with a native scrollbar.
After (this PR): All scroll inputs work at idle. Viewport height matches physical terminal exactly. Opening/closing dialogs does not freeze input.
Tested on
Risk & Scope
shift: falseconstraint changes behavior for existing arrow-key bindings. All 6 affected commands (NAVIGATION, SELECTION, COMPLETION) were verified to never needShift+Arrow— their consumers use bare arrows only.wasRawguard removal makes KeypressContext always hold a raw-mode refcount. This is safe because ink'ssetRawModeis refcounted — double-acquire increments the count, cleanup decrements it. The only behavioral change: KeypressContext now owns its refcount for its entire lifetime instead of depending on childuseInputconsumers to keep raw mode alive.swallowingSgrMouseflag). Edge case: if a malformed sequence arrives (e.g.\x1b[<followed by non-digit, non-M/m), the flag stays true until the nextM/m. This is acceptable because malformed SGR sequences only occur from terminal bugs, and the worst case is silently swallowing a few characters.Linked Issues
Closes #4942
Closes #4921
Fixes #4973
Fixes #4974
中文说明
本 PR 做了什么
修复了五个阻塞虚拟滚动（
ui.useTerminalBuffer）默认开启的问题：NAVIGATION_UP/DOWN、SELECTION_UP/DOWN、COMPLETION_UP/DOWN添加shift: false约束，使 Shift+Up/Down 只触发SCROLL_UP/DOWN。ScrollableList的hasFocus改为!dialogsVisible，使空闲时滚动可用。useBoxMetrics动态测量 VP 头部区域高度并扣除。!!key.X归一化修饰键比较。wasRaw守卫，始终持有自己的 ink refcount。之前打开 dialog 时最后一个useInput失活会导致 stdin 退回 cooked mode，所有输入冻结。64;50;15M) into the input box — readline treats<as CSI final byte #4974) — 在 handleKeypress 中过滤 SGR 鼠标序列。readline 的 CSI 解析器把<当终止符，将后续的64;50;15M当成普通文本输入到输入框。#4973 和 #4974 是 KeypressContext 的预存缺陷，不是 VP 特有的。它们被本 PR 的
hasFocus变更暴露（之前 SGR 鼠标模式在用户可输入时从未启用过）。设计方案对比
评估了 5 种方案（详见英文 Design alternatives），最终采用方案 A：
useBoxMetrics动态测量 +dialogsVisible门控 — 最小改动，准确，局部化。风险与范围
shift: false约束：已验证所有受影响命令都不需要 Shift+方向键wasRaw去除：ink 的setRawMode是引用计数的，重复获取安全