Skip to content

fix(desktop): stop at-rest scroll jump-up during code-block highlight - #38352

Closed
teknium1 wants to merge 1 commit into
mainfrom
hermes/hermes-aba10f53
Closed

fix(desktop): stop at-rest scroll jump-up during code-block highlight#38352
teknium1 wants to merge 1 commit into
mainfrom
hermes/hermes-aba10f53

Conversation

@teknium1

@teknium1 teknium1 commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Desktop chat no longer jumps up and snaps back while reading at the bottom — the symptom that survived #38221. Follow-up fixing a third, independent scroll-anchor bug on the armed-at-bottom path that #38221's two disarm fixes didn't cover.

Root cause (verified live in headless Chromium over CDP, not just modeled): while parked at the bottom, Streamdown/Shiki re-tokenizing a code/patch block briefly replaces laid-out DOM, so for one frame the content is shorter. The browser clamps scrollTop upward the instant content shrinks below the scroll position (1400 → 1100 with no pin involved); the next frame content regrows and the rAF pin snaps it back down. That's the "jumps quickly upward then returns" bounce, reported on Win11/macOS after updating, and one reporter pinned it to code blocks.

A pin cannot prevent it — the clamp happens at layout, before any pin runs (confirmed: both deferred and synchronous re-pins still show 1400 → 1100 → 1480).

Fix

Keep the content's height monotonic within a turn. The ResizeObserver raises a high-water-mark of the content height and reserves it as min-height on the content before pinning, so a transient shrink never shrinks the scroller and the browser never clamps. scrollHeight still grows under the viewport, so streaming tokens follow normally. The high-water-mark is reset in jumpToBottom (new turn / session / first content) and on user disarm, so an old tall thread can't pad a new short one and a finished turn can't leave a dead gap.

Changes

  • thread-virtualizer.tsx: contentHwmRef + min-height reservation in the RO callback; reset in jumpToBottom and disarm.
  • streaming.test.tsx: regression that models the browser clamp-on-shrink (scrollHeight = max(measured, reserved min-height), scrollTop clamps on shrink). Armed at bottom, a shrink RO frame must keep scrollTop at 1400 and reserve min-height: 2000px.

Why #38221's tests missed it

Both #38221 regression tests exercise the disarmed / scrolled-up path. This bug is on the armed / parked-at-bottom path — a disjoint code path — and the root cause is the browser's own clamp-on-shrink, which jsdom doesn't model.

Validation

Before (main) After
live CDP, shrink frame while armed scrollTop 1400 → 1100 (up-jump) → 1480 (snap back) 1400 → 1400 → 1400 (no bounce)
streaming follows growth yes yes (unchanged)
streaming.test.tsx new test RED (min-height stays empty) 11/11 pass
tsc -b 0 errors
eslint (changed files) 0 errors (only pre-existing padding-line / react-compiler warnings)

Live proof was a headless google-chrome + CDP harness driving a real scroller through the shrink→grow frames with native scrollTop clamping — the honest "real browser" bar for a renderer scroll bug, since jsdom doesn't clamp.

Known tradeoff

A legitimate mid-turn shrink (a tool-call panel collapsing, the loading indicator removed at turn end) is held at the high-water-mark until the next jumpToBottom/disarm, which can briefly reserve a few px of empty space at the very bottom. This resets on the next user message / thread.runStart / session change. Far less disruptive than the bounce, and only while armed at bottom.

Infographic

stop-the-scroll-bounce

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

🔎 Lint report: hermes/hermes-aba10f53 vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 9736 on HEAD, 9736 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 5045 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have labels Jun 3, 2026
Follow-up to #38221. Users still saw the chat viewport jump up then snap
back while reading at the bottom — pinned by one reporter to code/patch
blocks being highlighted. PR #38221 fixed the disarm (scrolled-up) path;
this is the armed-at-bottom path.

Root cause, verified live in headless Chromium (CDP), NOT just modeled:
while parked at bottom, Streamdown/Shiki re-tokenizing a code block briefly
REPLACES laid-out DOM, so for one frame the content is SHORTER. The browser
clamps scrollTop upward the instant content shrinks below the scroll
position (1400 -> 1100 with NO pin involved); the next frame content
regrows and the rAF pin snaps it back down. A pin cannot prevent the
up-jump because the clamp happens at layout, before any pin runs (confirmed:
both deferred and synchronous re-pins still show 1400 -> 1100 -> 1480).

Fix: keep the content's height MONOTONIC within a turn. The ResizeObserver
raises a high-water-mark and reserves it as min-height on the content BEFORE
pinning, so a transient shrink never shrinks the scroller and the browser
never clamps. scrollHeight still grows under the viewport so streaming
tokens follow. Reset the high-water-mark in jumpToBottom (new turn / session
/ first content) and on user disarm so an old tall thread can't pad a new
short one and a finished turn can't leave a dead gap.

Live CDP proof (real Chromium native clamping):
  current main:  parked 1400 -> shrink 1100 -> grow 1480   (bounce)
  this fix:      parked 1400 -> shrink 1400 -> grow 1400    (no bounce)

Adds a streaming.test.tsx regression that models the browser clamp-on-shrink
(scrollHeight = max(measured, reserved min-height); scrollTop clamps on
shrink). Armed at bottom, a shrink RO frame must keep scrollTop at 1400 and
reserve min-height 2000px. RED on pre-fix main (min-height stays empty).
@teknium1
teknium1 force-pushed the hermes/hermes-aba10f53 branch from 648be49 to a5ff28b Compare June 3, 2026 16:38

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

The desktop scroll-anchor logic now defends against the Shiki/Streamdown transient-shrink bug (the code-block highlight phase briefly replaces laid-out DOM with shorter content, causing the browser to clamp scrollTop upward). The fix:

  1. Tracks a contentHwmRef (high-water mark) reset per turn
  2. Reserves min-height = high-water-mark during the armed state via ResizeObserver
  3. Clears min-height on disarm and jumpToBottom so prior thread content doesn't pad new sessions
  4. Guards the virtualizer's scrollToFn against fighting the pinToBottom loop (sticky-bottom ref)
  5. Key fix for IME: event.nativeEvent.isComposing guard on Enter

✅ Looks Good

  • Fixes the root cause (monotonic height) rather than masking symptoms
  • Tests are thorough: transient-shrink, idle layout shift, viewport resize during streaming, IME guard
  • IME composition guard is essential — pressing Enter to confirm a composed word must not submit

💡 Suggestion

  • stickyBottomRef is a shared mutable ref between useThreadScrollAnchor and the VirtualizedThread virtualizer config. This coupling is correct but worth documenting in a comment near the virtualizer's scrollToFn since it crosses two abstraction boundaries.

Reviewed by Hermes Agent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants