feat(cli): virtual viewport for long conversations on ink 7#4146
Draft
chiga0 wants to merge 13 commits into
Draft
feat(cli): virtual viewport for long conversations on ink 7#4146chiga0 wants to merge 13 commits into
chiga0 wants to merge 13 commits into
Conversation
Captures the architectural analysis of how to thoroughly close the flicker / refresh-storm class of issues (#2950, #3118, #3007, #3838 UI side, #3899 follow-on) using a virtualized history viewport. - Surveys claude-code (forked ink) and gemini-cli (@jrichman/ink + ScrollableList + VirtualizedList) reference implementations. - Confirms ink 7 already exposes the primitives needed (`useBoxMetrics`, `measureElement`, `useWindowSize`, `useAnimation`) — no fork swap required. - Picks porting gemini-cli's virtualized list components to ink 7 with `ResizeObserver` -> `useBoxMetrics` and a custom `StaticRender`. - Splits the work into V.0..V.4 PRs with scope, dependencies, risk. - Lists open questions + 11-item approval checklist that must clear before V.0 implementation begins. This is a docs-only PR per the project's design-first workflow. No runtime code changes. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Port gemini-cli's VirtualizedList + ScrollableList to stock ink 7,
adapting for ink 7's available primitives:
- `overflowY="hidden"` + `marginTop={-scrollTop}` instead of ink-fork's
`overflowY="scroll"` (ink 7 has proper clip/unclip in render-node-to-output)
- `useBoxMetrics` inside each VirtualizedListItem (Option A) instead of a
single ResizeObserver WeakMap; reports height changes via onHeightChange
callback so the parent can update its heights record
- Custom `StaticRender` as `React.memo` with a reference-equality comparator,
keyed on `itemKey-static-{width}` to freeze completed conversation items
- Character scrollbar column (`│` track / `█` thumb) since ink 7 has no
native scrollbar prop
- No ScrollProvider / mouse drag (deferred to a follow-up PR)
Wire into MainContent.tsx behind `ui.useTerminalBuffer` setting (Settings
dialog → UI → Virtualized History; default false — opt-in).
Key bindings: Shift+↑/↓ (line), PgUp/PgDn (page), Ctrl+Home/End (top/bottom).
Re-render optimisations:
- renderItem wrapped in useCallback so renderedItems useMemo only recomputes
when actual deps change (not on every streaming tick)
- Completed history items passed by original object reference so
VirtualHistoryItem = memo(HistoryItemDisplay) can bail out on stable props
- estimatedItemHeight / keyExtractor / isStaticItem defined as module-level
constants with no closure deps
Generated with AI
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
… settings - keyMatchers.test.ts: 6 new test cases for SCROLL_UP/DOWN, PAGE_UP/DOWN, SCROLL_HOME/END commands (41 tests total) - settingsSchema.test.ts: assert ui.useTerminalBuffer is boolean, default false, showInDialog true, requiresRestart false Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
In VP mode, pending items are rendered inside VirtualizedList's overflowY="hidden" container, which uses ink 7's native clipping as the viewport guard. Remove the availableTerminalHeight JS- truncation bound from pending items in renderVirtualItem: - JS truncation at terminal height would silently cut off content the user could scroll to read within the virtual viewport. - ink 7 overflowY="hidden" on the VirtualizedList container is the correct clip guard — no JS line-counting workaround needed. - Remove uiState.constrainHeight from renderVirtualItem deps (no longer referenced in the VP rendering path). The legacy <Static> path is unchanged. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
Replace linear findLastIndex / findIndex scans on the offsets array with upperBound. Offsets are monotonic by construction, so the lookups inside the render body and getAnchorForScrollTop drop from O(n) to O(log n). Material for thousand-turn sessions where the lookup runs on every frame.
Two audit-found bugs in the VP path: 1. `<ShowMoreLines>` was outside the `<OverflowProvider>` that wraps `<ScrollableList>` in VP mode. `useOverflowState()` returns `undefined` outside the provider, so the component returned `null` and the "press ctrl-s to show more lines" affordance silently disappeared. Move `<ShowMoreLines>` inside the provider so the hook sees the live overflow state, matching the legacy path. 2. `refreshStatic()` and `repaintStaticViewport()` wrote `clearTerminal` / `cursorTo+eraseDown` to the host terminal unconditionally. In VP mode the React tree owns the visible region via ink 7's native `overflowY="hidden"` clipping — the physical write is a wasted flash on Ctrl+O / Alt+M / model change / resize. Guard both writes on `useTerminalBuffer === false`. The `historyRemountKey` bump still fires so the legacy `<Static>` fallback would still remount if someone toggled the setting mid- session. Extends the targeted-repaint pattern introduced in #3967 to all refreshStatic call sites, gated by the VP setting instead of by event type.
Three audit-found regressions tightened, in order of severity: 1. **Source-copy index offsets missing in VP** — legacy `<Static>` path threads per-item `sourceCopyIndexOffsets` so `/copy mermaid N` / `/copy latex N` hints stay stable across continuation messages. VP `renderVirtualItem` was not passing this prop, so the copy hints shown under each diagram drifted on every `gemini_content` chunk (the clipboard mechanism itself still worked from raw history; only the displayed number was wrong). Add two lookup tables — identity-keyed for static items, index-keyed for pending — without changing the VirtualizedList data signature, and thread offsets in both render branches. 2. **`renderVirtualItem` callback invalidated on every streaming tick** — its deps included `activePtyId` / `embeddedShellFocused` / `isEditorDialogOpen`, all of which flip mid-stream when a shell tool runs or a dialog opens. Each flip rebuilt the callback, invalidated `VirtualizedList.renderedItems`'s useMemo, and forced every static item to re-render through `<StaticRender>` — defeating the very memoization the design relies on. Move the three pending- only fields into a ref read inside the callback. Static-item closure now depends only on inputs that legitimately affect static output (terminalWidth, slashCommands, getCompactLabel, …). Pending items still re-render correctly because their item identity changes per tick, so the callback is called fresh each time and reads the latest ref. 3. **`pending` items now honour `constrainHeight`** in VP, matching the legacy path. Previously VP unconditionally passed `undefined` for `availableTerminalHeight` on pending, relying on the viewport `overflowY="hidden"` clip to limit visible size — but that hid the `<ShowMoreLines>` affordance from the user. Now that ShowMoreLines is correctly wired (previous commit), restore parity. 4. **Heights map memory leak** in `VirtualizedList` — `setHeights` only grew. Each `/clear` left orphan `h-N` keys; each pending → completed transition left orphan `p-N` keys. Add a `useLayoutEffect` that prunes entries whose keys are not in the current `data`. Runs in layout phase so the prune commits in the same paint as the data change — no stale-offsets frame.
Completion-pass artifacts driven by the multi-agent audit: - Settings description rewritten to enumerate the symptoms VP fixes so users with active flicker reports can find the toggle without reading the design doc. - `absorbedCallIds` returns a module-level constant Set when compact mode is off, instead of a fresh `new Set()` per render. Fixes a hidden cascade: `activePtyId` flip mid-stream → useMemo runs → returns a new empty Set → `isSummaryAbsorbed` rebuilds → `renderVirtualItem` rebuilds → `VirtualizedList.renderedItems` recomputes → every static item re-renders. With the constant, the cascade dies at the source. Helps both VP and legacy paths. - VP-path unit tests for MainContent (4 cases): ScrollableList mounts and Static does not when `useTerminalBuffer: true`; ShowMoreLines is reachable in VP mode (regression of the OverflowProvider mis-wrap); source-copy index offsets thread into renderItem for static items; renderItem callback identity is stable across `activePtyId` flips (proves the ref-based read keeps StaticRender memo effective).
…une + tighten ShowMoreLines test Round-2 audit follow-ups. Three real findings addressed; one flagged false positive documented separately. 1. **absorbedCallIds Set identity now content-stable when compact mode is on.** The earlier EMPTY constant only short-circuited the compactMode= false path; when compact mode is enabled (some users default-on it), activePtyId / embeddedShellFocused flips during streaming still produced fresh Sets per render even when membership was unchanged, restarting the same cascade the pendingStateRef fix was meant to avoid. Compare-and-reuse via a ref: if the new Set has identical membership to the previous one, return the previous reference. 2. **`heights` map prune in `VirtualizedList` is gated.** Previously every streaming tick rebuilt an N-key Set and walked all heights, even on the steady-state path where nothing changes. Now only fires when the heights record has clearly outpaced live data (`size > max(8, 2 × data.length)`) — covers `/clear` and accumulated pending → completed transitions, skips the 30-Hz hot path entirely. 3. **VP ShowMoreLines test now actually verifies overflow connectivity.** Previous mock unconditionally rendered "SHOW_MORE", so the test only proved the JSX mounted — it would still pass if a future refactor moved `<OverflowProvider>` out of the VP tree again. The mock now reads `useOverflowState()` and emits "OVERFLOW_DISCONNECTED" when the context is missing. The VP test asserts both presence of "SHOW_MORE" and absence of the disconnected marker, so the regression is now caught. Not addressed: - Audit P0-1 claim that `renderMode` (Alt+M) / model-change updates don't reach VP static items: false positive. `renderMode` is a React Context (`RenderModeContext`), and Context propagation traverses the tree past `memo` boundaries — MarkdownDisplay's `useRenderMode()` consumer re-renders on context change regardless of whether `StaticRender` bails out. Verified by reading `packages/cli/src/ui/contexts/RenderModeContext.tsx` and `MarkdownDisplay.tsx:172`. No code change. - Audit P1-2 pendingStateRef write-during-render race: speculative, relies on a multi-pass render path React 18+ does not currently use. Documented assumption in the existing inline comment.
…ct-mode mergedHistory stability Round-3 audit follow-ups. Three real findings; the rest verified clean. 1. **`renderItem` errors no longer crash the CLI.** Previously a throw inside a per-item render propagated through `VirtualizedList`'s useMemo into React's commit phase, tearing down the whole Ink tree — one bad history record could nuke the session. Wrap each call in a try/catch and substitute a small red `[render error] …` text box on failure. The row stays in the viewport so the user can scroll past it. 2. **Defensive height coerce in offset accumulation.** A buggy `estimatedItemHeight` returning NaN / negative / Infinity would poison every downstream offset and break the `upperBound` / `findLastLE` binary search (which assumes monotonic offsets). Clamp to `Number.isFinite(raw) && raw > 0 ? raw : 0`. No-op for the in-tree estimators that return 3; insurance against future consumers. 3. **`mergedHistory` is content-stable when compact mode is on.** The Round-2 absorbedCallIds stability fix didn't reach this path: `mergeCompactToolGroups` always allocates a fresh array, and `mergedHistory`'s useMemo lists `activePtyId` / `embeddedShellFocused` as deps, so every streaming tick mid-shell-tool produced a new array even when items aligned. Cascade went `mergedHistory` → offsets map → `renderVirtualItem` → every static item re-rendered. Pair-wise compare new vs previous and return the previous reference when items align. Restores StaticRender memo effectiveness for compact-mode users. Not addressed (audit findings deemed not worth fixing in this PR): - `scrollToItem` silently no-ops when item is not in data — no current caller checks the return value, low impact. - `allVirtualItems` array spread is O(n) per streaming tick — real but not a crash; revisit in a perf-focused follow-up. - `itemRefs.current` is dead surface (never read) — cosmetic. - StrictMode-only-in-DEBUG double-invoke paths verified safe.
… coverage + cleanups Addresses wenshao's CHANGES_REQUESTED review on PR #3941. - Add focused unit tests for `VirtualizedList` (9 cases) covering empty data, `renderStatic` full-render, `initialScrollIndex` with `SCROLL_TO_ITEM_END`, `targetScrollIndex` anchoring, imperative `scrollToEnd` / `scrollToIndex`, per-item `renderItem` error isolation, NaN/negative estimator coercion, and out-of-range `initialScrollIndex` clamping. - Add `useBatchedScroll` unit tests (4 cases) covering initial reads, pending-value reads in the same tick, post-commit pending reset, and callback identity stability across rerenders. - Remove dead `itemRefs` / `onSetRef` plumbing (declared, written, never read; `useCallback` with empty deps was also a stale-closure trap). - Remove unused `isStatic?: boolean` from `VirtualizedListProps` (only `isStaticItem` is actually consumed). - Tighten the render-phase setState block: each setter is now guarded by an equality check so React bails out of redundant updates, and a comment documents that this is the React-endorsed "adjusting state while rendering" pattern (the synchronous update avoids a one-frame flash at the previous position when `targetScrollIndex` changes). Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…wup) Declared and written in a `useLayoutEffect` on every `data` change but never read anywhere in the component. Flagged in wenshao's round-4 review of PR #3941. Generated with AI Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…on-ink7 # Conflicts: # packages/cli/src/ui/components/MainContent.test.tsx
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
1 participant
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
<Static>is append-only and feeds the entiremergedHistoryto ink on everyhistoryRemountKeychange. On a 1000-turn conversation that's 1000HistoryItemDisplayrenders + ink layout passes whenever Ctrl+O / model change / Alt+M / resize forces a remount — the root cause of every flicker / scroll-storm / repaint complaint where the trigger is "something invalidated the static block".This PR adds an opt-in virtualised history path: only the visible viewport range is mounted in React, completed items above the viewport are frozen via
StaticRendermemoization, and out-of-viewport content lives in in-app scroll state instead of the host terminal's scrollback buffer.ui.useTerminalBuffer: truein settings (Settings dialog → UI → Virtualized History). Defaultfalse— legacy<Static>path untouched.Shift+↑/↓(line) ·PgUp/PgDn(page) ·Ctrl+Home/End(top/bottom)What this PR actually delivers
We audited the implementation against the issue list and tightened the framing. The honest picture:
Direct fixes (root cause is
<Static>re-render storm on remount)Partially addresses (root cause is provider-side; VP removes the render-side amplifier)
Partially addresses (in-app workaround for a host-terminal limitation)
Shift+↑/↓/PgUp/PgDn/Ctrl+Home/Endfor in-app scrollback, which is a workaround, not a true fix.Co-fixed by this branch and existing in-flight work
progressive-replaymitigation in the<Static>path; VP supersedes it for VP users (the freeze trigger — full<Static>remount — does not exist in VP).Adjacent flicker fixes that are owned by sibling PRs
Don't expect this PR alone to close them; they're listed for context:
cursorTo + eraseDowninstead ofclearTerminalon resize. This PR extends that pattern torefreshStatic/repaintStaticViewportso Ctrl+O / model change / Alt+M no longer flash either, when VP is on.Architecture
@jrichman/inkfork)overflowY="scroll"+scrollToppropoverflowY="hidden"+marginTop={-clampedScrollTop}(ink 7 has proper clip/unclip inrender-node-to-output.js)ResizeObserverwith WeakMap for N itemsuseBoxMetricsinside eachVirtualizedListItem(Option A in design doc §6.2); reports height viaonHeightChangecallback. Safe because only the visible window mounts.StaticRenderexported from ink forkReact.memowith reference-equality comparator. Streaming-only state moved off therenderItemclosure (via refs) so the memo doesn't bail during shell-tool ticks.│track /█thumb)ScrollProvider+ mouse dragNew files
ui/hooks/useBatchedScroll.tsgetScrollTopaccessor across in-flight scroll eventsui/components/shared/StaticRender.tsxui/components/shared/VirtualizedList.tsxui/components/shared/ScrollableList.tsxdocs/design/virtual-viewport/README.mdOptimisations (real ones, not theatre)
renderItemis stable across streaming ticks —activePtyId/embeddedShellFocused/isEditorDialogOpen/constrainHeightread from refs so the callback identity doesn't flip while a shell tool runs;StaticRendermemo actually wins.upperBound) — O(log n) per frame instead of O(n).memo(HistoryItemDisplay)bails out on stable props.heightsmap is GC'd in auseLayoutEffectwhen items disappear — no unbounded growth across/clearcycles or pending → completed key transitions.renderItemso/copy mermaid N//copy latex Nhints stay stable in VP mode (matches legacy).refreshStatic/repaintStaticViewportskipclearTerminal/cursorTo+eraseDownin VP mode — the React tree owns the visible region, the physical write is a wasted flash.Test plan
npm run typecheck --workspace=@qwen-code/qwen-code— VP-touched files clean (pre-existing main errors unrelated)cd packages/cli && npx vitest run— 342 VP-related tests passui.useTerminalBuffer, start a long session, verifyShift+↑/↓/PgUp/PgDn/Ctrl+Home/Endscroll, and pending items render at the bottom during streaming<Static>path takes over without breakingDesign doc
docs/design/virtual-viewport/README.md— full analysis, alternatives, risks (LRU eviction, mouse drag deferral, etc.).🤖 Generated with Qwen Code