fix(terminal): smoother host scroll + subtle TUI paint glide - #9339
fix(terminal): smoother host scroll + subtle TUI paint glide#9339gatsby74 wants to merge 3 commits into
Conversation
…tablyai#7450) Addresses Jinwoo's stablyai#7450 review: fractional translateY no longer exposes a blank strip (paint ±1 overscan row on DOM + WebGL, clip overflow:hidden), and scope is host-owned scroll only (scrollback / normal buffer) — not true 1px logical scroll inside alt-screen TUIs. - Extend @xterm/xterm patch: pixel-offset + catch-up glide, hasScrollback gate, clip/bg, DomRenderer overscan - Extend @xterm/addon-webgl patch: paint rows+2, logical layout height, canvas -cellHeight shift inside clipped screen - Default smoothScrollDuration 120ms so sub-row remainders animate
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (17)
📝 WalkthroughWalkthroughTerminal settings now support configurable TUI scroll glide with 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/patches/@xterm__xterm@6.1.0-beta.287.patch (1)
355-381: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
_sendCompositionInput's overlap-merge heuristic has no accompanying test coverage and looks fragile for edge cases (empty strings, repeated substrings, multi-byte/surrogate-pair splits).The longest-common-prefix/suffix merge between
_pendingKeypressDataand the composition candidate can pick the wrong branch when both strings share ambiguous overlaps (e.g. repeated characters), potentially sending duplicated or truncated text to the terminal on IME composition end. Given this changes what actually gets written to the PTY, please add unit tests for this merge logic (empty keypress, full duplicate, partial prefix/suffix overlap, no overlap) before merging.
🧹 Nitpick comments (1)
config/patches/@xterm__xterm@6.1.0-beta.287.patch (1)
444-462: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
renderRowsoverscan range is a superset, not a shift — wastes ~2 extra full-row DOM rebuilds on every partial refresh.Real paint index = logical index +
SCROLL_OVERSCAN_ROWS, so the tight bound should be[start + SCROLL_OVERSCAN_ROWS, end + SCROLL_OVERSCAN_ROWS]. Instead the code uses[start, end + 2*SCROLL_OVERSCAN_ROWS], which still contains the correct rows but always rebuilds ~2 extra full-width rows (e.g. every cursor-move/selection/blink refresh), not just at scroll boundaries.♻️ Proposed tightening
- const paintStart = Math.max(0, start); - const paintEnd = Math.min(this._rowElements.length - 1, end + 2 * SCROLL_OVERSCAN_ROWS); - // Also refresh overscan rows adjacent to the requested band. - const lo = Math.max(0, paintStart); - const hi = Math.min(this._rowElements.length - 1, paintEnd); + const lo = Math.max(0, start + SCROLL_OVERSCAN_ROWS); + const hi = Math.min(this._rowElements.length - 1, end + SCROLL_OVERSCAN_ROWS);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d4b0693c-5113-4e24-8e72-7da8abc19a62
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
config/patches/@xterm__addon-webgl@0.20.0-beta.286.patchconfig/patches/@xterm__xterm@6.1.0-beta.287.patchsrc/renderer/src/lib/pane-manager/pane-terminal-options.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/patches/@xterm__xterm@6.1.0-beta.287.patch (1)
355-381: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
_sendCompositionInput's overlap-merge heuristic has no accompanying test coverage and looks fragile for edge cases (empty strings, repeated substrings, multi-byte/surrogate-pair splits).The longest-common-prefix/suffix merge between
_pendingKeypressDataand the composition candidate can pick the wrong branch when both strings share ambiguous overlaps (e.g. repeated characters), potentially sending duplicated or truncated text to the terminal on IME composition end. Given this changes what actually gets written to the PTY, please add unit tests for this merge logic (empty keypress, full duplicate, partial prefix/suffix overlap, no overlap) before merging.
🧹 Nitpick comments (1)
config/patches/@xterm__xterm@6.1.0-beta.287.patch (1)
444-462: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
renderRowsoverscan range is a superset, not a shift — wastes ~2 extra full-row DOM rebuilds on every partial refresh.Real paint index = logical index +
SCROLL_OVERSCAN_ROWS, so the tight bound should be[start + SCROLL_OVERSCAN_ROWS, end + SCROLL_OVERSCAN_ROWS]. Instead the code uses[start, end + 2*SCROLL_OVERSCAN_ROWS], which still contains the correct rows but always rebuilds ~2 extra full-width rows (e.g. every cursor-move/selection/blink refresh), not just at scroll boundaries.♻️ Proposed tightening
- const paintStart = Math.max(0, start); - const paintEnd = Math.min(this._rowElements.length - 1, end + 2 * SCROLL_OVERSCAN_ROWS); - // Also refresh overscan rows adjacent to the requested band. - const lo = Math.max(0, paintStart); - const hi = Math.min(this._rowElements.length - 1, paintEnd); + const lo = Math.max(0, start + SCROLL_OVERSCAN_ROWS); + const hi = Math.min(this._rowElements.length - 1, end + SCROLL_OVERSCAN_ROWS);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d4b0693c-5113-4e24-8e72-7da8abc19a62
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
config/patches/@xterm__addon-webgl@0.20.0-beta.286.patchconfig/patches/@xterm__xterm@6.1.0-beta.287.patchsrc/renderer/src/lib/pane-manager/pane-terminal-options.ts
🛑 Comments failed to post (2)
config/patches/@xterm__addon-webgl@0.20.0-beta.286.patch (1)
121-141: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
+2overscan row count duplicated as a magic number.
SCROLL_OVERSCAN_ROWSis defined only inWebglRenderer.ts;GlyphRenderer.clear()independently hardcodesterminal.rows + 2. If the overscan row count ever changes in one file without the other,_vertices.attributeswill be sized for a different row count thanWebglRenderer._updateModel/updateCellwrite into — since JS typed-array writes pastlengthare silently dropped, this fails silently as corrupted/missing rendering rather than a crash.♻️ Suggested fix
Export
SCROLL_OVERSCAN_ROWS(or an equivalent shared constant) from a common module and import it in bothGlyphRenderer.tsandWebglRenderer.tsinstead of hardcoding2/+2*SCROLL_OVERSCAN_ROWSindependently.config/patches/@xterm__xterm@6.1.0-beta.287.patch (1)
200-215: 🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
Overscan row-alignment shift is applied twice (parent
screenElement+ childrowContainer), permanently misaligning rendered rows by one extra row height.
screenElementis the same DOM node in bothViewportandDomRenderer(passed identically at construction).DomRenderer._updateDimensions()already shiftsrowContainer(a child ofscreenElement) bytranslateY(-overscanPx)to align the overscan-painted row with the real top row — this alone is correct.Viewport._applyPixelOffset()then reads the same-overscanPxvalue back out ofscreenElement.dataset.xtermOverscanPxand applies it again toscreenElementitself. Since CSS transforms on nested elements compose additively, the resulting baseline shift is-2×overscanPxinstead of-1×overscanPx— even whenpixelOffsetandcatchUpare both0. This isn't limited to scroll animation; it's the resting/idle state too, so terminal content will always render shifted up by roughly one extra row height, cutting off/duplicating content at the viewport edges.
config/patches/@xterm__xterm@6.1.0-beta.287.patch#L200-L215: In_applyPixelOffset, drop theoverscanPxlookup/addition entirely and setel.style.transformfromoffset(pixelOffset + catchUp) alone, lettingDomRenderer/WebGL renderer own the static overscan baseline on their own paint containers.config/patches/@xterm__xterm@6.1.0-beta.287.patch#L419-L430: Keep therowContainerstatic shift as the single source of truth for overscan alignment (or, alternatively, remove it here and haveViewportown the full-overscanPx + offsetshift onscreenElement— but not both).Please pick one owner for this shift and verify visually (e.g. scroll to top/bottom, resize, focus) that rows no longer appear off-by-one after the fix.
📍 Affects 1 file
config/patches/@xterm__xterm@6.1.0-beta.287.patch#L200-L215(this comment)config/patches/@xterm__xterm@6.1.0-beta.287.patch#L419-L430
Remove incomplete Dom/WebGL overscan that blanked new terminals (double -cellHeight shift; WebGL paint height without model remapping). Keep host-viewport pixel-smooth scroll + catch-up + clip. Also expand CompositionHelper merge unit tests for empty/full/partial overlap, repeated chars, and surrogate pairs (CodeRabbit stablyai#9339).
gatsby74
changed the title
|
Addressed CodeRabbit review on
Host-viewport smooth scroll ( |
gatsby74
changed the title
Add visual-only sub-row paint lag for mouse-reporting TUIs (Claude, Codex, OpenCode, etc.) on top of host-viewport smooth scroll. Does not change ydisp or wheel-report distance — only eases the painted screen between discrete app updates. - xterm Viewport: nudgeTuiGlide + shared offset RAF with host catch-up - Setting terminalTuiScrollGlide: off | subtle | medium (default subtle) - Wire via existing TUI wheel multiplier attach path
|
Follow-up on the smoothness ladder:
Overscan / Jinwoo blank edge: overscan was the fill-glyphs fix for fractional host scroll; it blanked terminals and was removed. Clip remains; full overscan is optional later work, not required for this PR. |
gatsby74
changed the title
3 participants
Summary
Makes everyday terminal scrolling feel smoother in two separate paths:
Builds on Prince’s #7450 and addresses @Jinwoo-H’s review: scope is explicit, host path only for real pixel remainder, TUI path does not invent logical app scroll.
What users should notice
prefers-reduced-motion: reducesnaps both glides off.What we added (technical)
@xterm/xtermpatchscrollTopremainder →translateY(_pixelOffset)_catchUp(only whenhasScrollback)nudgeTuiGlide(deltaPx, maxCellFraction)→_tuiGlideoverflow: hiddenon scroll content / screen so offsets don’t paint outside the boxsmoothScrollDuration: 120in Orca terminal options (decay timebase + smooth scrollable animation)Orca
terminalTuiScrollGlide: 'off' | 'subtle' | 'medium'(default'subtle')enable-mouse-eventsgate) — only when report multiplication already runsTests
Limitations (please read)
rows+2paint). That implementation blanked new terminals (double Dom shift + incomplete WebGL) and was removed. Remaining mitigation is clip only — a thin empty edge can still show during host sub-row motion.Relation to #7450 / overscan
#7450 can be closed as superseded once this lands, or left as historical context.
Scope table
Testing
vitest: TUI glide, mouse-wheel, IME composition de-duplicationprefers-reduced-motiondisables glidesHost scrollback fill (for manual test):
seq -f '%04g | LINE %04g | scroll-bench-marker-%04g' 1 400 1 400 1 400AI agent review summary
Notes for maintainers
config/patches/@xterm__xterm@…(IME merge + viewport smooth/glide live in the same patch on this branch).ELI5
Terminal scrolling felt jumpy: history scrolled in whole-row steps and fullscreen TUIs painted with hard jumps. Host scrollback gets smoother pixel motion, and TUI updates get a short soft paint glide.