Release YH (v0.51.678): suppress post-render scroll artifact across all intent modalities (#4970) - #4989
Merged
Conversation
…sses The new movedUp suppression branch called _recentMessageRenderArtifactWindow() and the intent helpers in an unguarded position. The #4295 unit harness injects the scroll-listener body via new Function() without those helpers, so the branch threw ReferenceError on a movedUp sample and crashed the node subprocess. Reorder the && chain with typeof-function guards first; production evaluates the real helpers, the harness short-circuits before any call. Behavior unchanged.
…4970 review) Maintainer dual-gate (Codex) found the suppression could swallow a genuine low-delta trackpad wheel scroll-up for ~1.4s after a render: _recordNonMessageScrollIntent() only recorded message-pane wheel intent at deltaY<-30, so a gentle deltaY:-5 left both intent helpers false and the post-render branch returned before movedUp set _messageUserUnpinned. - Track recent low-delta upward message-pane wheel intent separately (_lastMessageWheelIntentMs / _recentMessageWheelIntent), recorded for any upward wheel (deltaY<0). The decisive deltaY<-30 sticky-unpin is unchanged. - Require !_recentMessageWheelIntent() before suppressing the artifact, so a real gentle scroll-up inside the window still unpins. - Add behavioral node-harness regressions: gentle wheel inside the window unpins; no-intent artifact inside the window stays suppressed; outside the window unpins. Plus a source lock that low-delta intent is tracked.
…sion on scrollbar drag (#4970 review) Maintainer dual-gate found two stale-state leaks in the new _lastMessageWheelIntentMs and one adjacent pre-existing gap; all three fixed: MUST-FIX 1 — _resetScrollDirectionTracker() (session switch) did not clear _lastMessageWheelIntentMs, so a gentle wheel in chat A left _recentMessageWheelIntent() true into chat B's first post-render window, under-suppressing the artifact and falsely unpinning. Now reset to -Infinity. MUST-FIX 2 — _resetStreamScrollFollow() (fresh stream) had the same leak: a gentle upward wheel within 1200ms of a new stream could silently disable live follow. Now reset to -Infinity. SHOULD-FIX 3 — the suppression branch ignored _scrollbarDragActive, so a manual scrollbar-drag upward scroll inside the 1400ms window was swallowed. Gate the branch on (typeof _scrollbarDragActive==='undefined' || !_scrollbarDragActive); typeof guard keeps the #4295 node harness inert. Tests: scrollbar-drag-inside-window-still-unpins behavioral regression (harness extended with injected _scrollbarDragActive), plus source locks for both resets and the scrollbar-drag gate. test_4856 + test_4295 green (17 passed), node --check clean.
…ile P1) Keyboard scrolling of the message pane (PageUp/PageDown, Arrow keys, Space, Home/End) fires a native scroll event with no wheel/touch/scrollbar/non-message intent. Inside the 1400ms post-render artifact window the suppression branch then returned before movedUp could unpin, so a keyboard scroll-up was swallowed and live-follow snapped the reader back to the bottom. - Add _lastMessageKeyScrollIntentMs + _recentMessageKeyScrollIntent(), stamped by a capture-phase keydown listener on the scroll keys, gated to when the message pane is the scroll target (focused/contains focus/hovered) and not an editable field (composer/input/contenteditable). - Gate the post-render suppression on !_recentMessageKeyScrollIntent() (typeof guard keeps the #4295 node harness inert). - Clear the stamp in both _resetScrollDirectionTracker() and _resetStreamScrollFollow() (same stale-state hygiene as the wheel stamp). Tests: keyboard-scroll-inside-window-still-unpins behavioral regression (harness extended with injected _recentMessageKeyScrollIntent), plus a source lock for the helper/keydown-stamp/suppression-gate/both-resets. test_4856 + test_4295 green (19 passed), node --check clean.
…#4970 review) Maintainer/Codex found one narrow edge in the keyboard intent stamp: because the listener runs capture-phase and accepts any focused descendant of #messages, Space/Spacebar on an in-transcript control (tool-card toggles, copy buttons, role buttons, links/tabs) stamped _lastMessageKeyScrollIntentMs before the control handler could preventDefault/stopPropagation. That made a button activation look like a scroll intent and could mask a legitimate post-render artifact. Fix by excluding Space/Spacebar when the event target or active element is an interactive transcript control: button, a[href], select, summary, role=button, role=tab, role=menuitem, or contenteditable. Keep PageUp/PageDown/arrows/Home/End stamping unchanged, and keep the existing INPUT/TEXTAREA/contenteditable composer exclusion. Add a behavioral node-harness test: Space on a focused transcript button leaves the key-scroll stamp at -Infinity (serialized null), while PageUp on the pane still stamps 1234. test_4856 + test_4295 green (20 passed), node --check clean.
…ll intent modalities (#4970)
Contributor
|
| Filename | Overview |
|---|---|
| static/ui.js | Adds post-render scroll artifact suppression with per-modality intent tracking (wheel, touch, keyboard, scrollbar); a logic gap lets downward-direction keys stamp upward scroll intent, which can bypass artifact suppression and re-break live-follow after a streaming render. |
| tests/test_issue4856_android_scroll_regression.py | Adds 20 behavioral regression tests (static assertion + Node harness) covering wheel intent, scrollbar drag, keyboard scroll, and session-reset hygiene; all tests pass through a generic key_scroll boolean stub that does not distinguish upward vs. downward keys, leaving the over-broad key set untested. |
| CHANGELOG.md | Adds release entry for v0.51.678 describing the scroll artifact fix; changelog update is expected here as this is a release commit. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["scroll event on #messages"] --> B{"_programmaticScroll?"}
B -- yes --> Z["return — ignored"]
B -- no --> C["rAF: compute movedUp / movedDown / grew"]
C --> E{"movedUp?"}
E -- no --> F["normal pin/unpin logic"]
E -- yes --> G{"all typeof guards pass?"}
G -- no --> F
G -- yes --> H{"_recentMessageRenderArtifactWindow(1400)?"}
H -- no --> F
H -- yes --> I{"recent intent — touch / wheel / non-msg / scrollbar / keyboard?"}
I -- any present --> F
I -- none --> J["SUPPRESS artifact\nupdate _lastScrollTop, return"]
F --> K{"movedUp branch reached?"}
K -- yes --> L["_messageUserUnpinned=true\n_scrollPinned=false\nLive-follow OFF"]
K -- no --> M["re-pin if movedDown + nearBottom"]
W["wheel deltaY<0\n→ _lastMessageWheelIntentMs"] --> I
T["touchmove\n→ _messageTouchScrollActive"] --> I
SD["scrollbar drag\n→ _scrollbarDragActive"] --> I
KB["keydown scroll keys\n→ _lastMessageKeyScrollIntentMs"] --> I
style J fill:#d4edda,stroke:#28a745
style L fill:#f8d7da,stroke:#dc3545
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["scroll event on #messages"] --> B{"_programmaticScroll?"}
B -- yes --> Z["return — ignored"]
B -- no --> C["rAF: compute movedUp / movedDown / grew"]
C --> E{"movedUp?"}
E -- no --> F["normal pin/unpin logic"]
E -- yes --> G{"all typeof guards pass?"}
G -- no --> F
G -- yes --> H{"_recentMessageRenderArtifactWindow(1400)?"}
H -- no --> F
H -- yes --> I{"recent intent — touch / wheel / non-msg / scrollbar / keyboard?"}
I -- any present --> F
I -- none --> J["SUPPRESS artifact\nupdate _lastScrollTop, return"]
F --> K{"movedUp branch reached?"}
K -- yes --> L["_messageUserUnpinned=true\n_scrollPinned=false\nLive-follow OFF"]
K -- no --> M["re-pin if movedDown + nearBottom"]
W["wheel deltaY<0\n→ _lastMessageWheelIntentMs"] --> I
T["touchmove\n→ _messageTouchScrollActive"] --> I
SD["scrollbar drag\n→ _scrollbarDragActive"] --> I
KB["keydown scroll keys\n→ _lastMessageKeyScrollIntentMs"] --> I
style J fill:#d4edda,stroke:#28a745
style L fill:#f8d7da,stroke:#dc3545
Reviews (1): Last reviewed commit: "Release YH (v0.51.678): suppress post-re..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+4018
to
+4020
| const _MESSAGE_SCROLL_KEYS=new Set([ | ||
| 'PageUp','PageDown','ArrowUp','ArrowDown','Home','End','Spacebar',' ', | ||
| ]); |
Contributor
There was a problem hiding this comment.
PageDown, ArrowDown, End, Spacebar/' ' without Shift) are in _MESSAGE_SCROLL_KEYS and will stamp _lastMessageKeyScrollIntentMs just like upward keys do. Because the artifact suppression skips when _recentMessageKeyScrollIntent() is true, a user pressing ArrowDown at the bottom of a live-streaming chat (a common "is there more?" gesture) stamps the intent — then when the next streaming render fires and the browser emits its phantom upward scroll, suppression is bypassed, movedUp is treated as real, and live-follow breaks exactly as #4970 set out to prevent. Only keys that produce an upward scroll should protect against the phantom-upward-scroll artifact path.
Suggested change
| const _MESSAGE_SCROLL_KEYS=new Set([ | |
| 'PageUp','PageDown','ArrowUp','ArrowDown','Home','End','Spacebar',' ', | |
| ]); | |
| // Only upward-direction keys: these are the only keys that produce a real | |
| // movedUp scroll event, so they're the only ones that should bypass the | |
| // post-render artifact suppression. Downward keys (PageDown, ArrowDown, End, | |
| // Space) scroll toward the bottom and must NOT stamp intent that would | |
| // prevent suppression of a subsequent phantom upward-scroll artifact. | |
| const _MESSAGE_SCROLL_KEYS=new Set([ | |
| 'PageUp','ArrowUp','Home', | |
| ]); |
| document.addEventListener('visibilitychange',()=>{ | ||
| if(document.visibilityState==='hidden') _scrollbarDragActive=false; | ||
| },{passive:true}); | ||
| // #4970 review (greptile P1): record keyboard-driven message-pane scrolling as |
Contributor
There was a problem hiding this comment.
Comments throughout this block use the form // #4970 review (greptile P1): — a dozen or so instances in the added code. These are internal review-cycle annotations that ended up committed to the production file. They don't affect runtime behaviour but they are confusing to future maintainers who would have no context for what "greptile P1" means in a source comment. Consider rewriting them as plain engineering rationale comments before the next release.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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
2 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.
Release YH (v0.51.678) — chat no longer jumps to the bottom after a reply renders
Ships #4970 (@allenliang2022) — follow-up to the shipped #4934 DOM-wipe clamp.
What it fixes
After
renderMessages()the browser could emit a phantom upward scroll with no user intent, setting the "user scrolled up" flag and breaking live-follow (transcript snaps back to the bottom). The post-render window now suppresses that artifact when there's genuinely no recent user scroll intent.Gate (converged after 5 rounds, each catching a real edge)
Across 5 dual-gate rounds the intent detection was hardened to cover every modality without ever swallowing a real scroll: wheel (incl gentle low-delta trackpad below the unpin threshold), touch, scrollbar drag, keyboard (PageUp/PageDown/arrows/Space/Home/End), the intent state cleared on session-switch + stream-reset (no cross-conversation leak), and Space-on-a-focused-transcript-control excluded (a control activation, not a scroll).
Credit @allenliang2022 — persistent work through 5 rounds on the crown-jewel scroll-follow listener.