Skip to content

fix(streaming): show new-message cue when preserving scroll position (#3545)#3631

Closed
rodboev wants to merge 1 commit into
nesquena:masterfrom
rodboev:pr/streaming-scroll-cue
Closed

fix(streaming): show new-message cue when preserving scroll position (#3545)#3631
rodboev wants to merge 1 commit into
nesquena:masterfrom
rodboev:pr/streaming-scroll-cue

Conversation

@rodboev

@rodboev rodboev commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Thinking Path

  • The scroll system now intentionally honors sticky unpinning when a user scrolls up during streaming, so final settlement should not always snap the user back to the bottom.
  • The broken part of Streaming scroll position drifts to no-mans-land after response completes when user scrolled up mid-stream #3545 is that preserving the reader's viewport after the response completes gives no clear signal that new content settled below it.
  • The existing floating End button already owns the jump-to-latest affordance, so the narrow fix is to turn it into a temporary new-message cue when a preserve-scroll render leaves new content below an unpinned viewport.

What Changed

  • static/ui.js: captures scroll height in the preserve-scroll snapshot, detects unpinned renders that add content below the restored viewport, shows a temporary new-message cue, and clears it when the reader jumps to or scrolls back near the bottom.
  • static/style.css: adds a stable visual cue state for the existing scroll-to-bottom pill.
  • static/i18n.js: adds localized text and aria/title keys for the cue.
  • tests/: adds static regression coverage for the preserve-scroll branch, cue clearing, i18n keys, and CSS hook.

Why It Matters

Users can keep reading history while a long answer finishes, but they no longer land in an ambiguous state with no indication that the final response is available.

Verification

  • $env:PYTHONUTF8 = '1'; $env:BROWSER = 'echo'; C:\Apps\hermes\hermes-agent\venv\Scripts\python.exe -m pytest tests/test_issue3545_streaming_scroll_cue.py tests/test_issue1690_scroll_completion.py tests/test_session_jump_buttons.py tests/test_tars_scroll_reset_regressions.py tests/test_issue677.py -v --timeout=60 --timeout-method=thread
  • node --check static/ui.js
  • pytest tests/ -v --timeout=60
  • Manual: send a long streaming response, scroll upward mid-stream, wait for completion, confirm the new-message cue appears and jumps to the final response without changing pinned-user behavior.

Risks / Follow-ups

The cue uses a scroll-height growth heuristic rather than a full browser-driven runtime test because the current suite primarily pins these scroll invariants with static tests. A future Playwright regression could exercise the exact mid-stream wheel-scroll flow.

Model Used

GPT-5.5 via Codex CLI

@greptile-apps

greptile-apps Bot commented Jun 5, 2026

Copy link
Copy Markdown

Greptile Summary

This PR implements a "new message" cue for the scroll-to-bottom button when a streaming response completes while the user has scrolled up (preserveScroll path). After DOM rebuild, the restored viewport now detects that new content settled below it and transforms the floating End pill into an accented "New message" affordance, clearing automatically when the reader scrolls back near the bottom or clicks through.

  • static/ui.js: replaces the _followMessagesAfterDomReplace() call in _scrollAfterMessageRender with an explicit _scrollPinned branch, adds _maybeShowNewMessageScrollCue / _syncScrollToBottomCue helpers, and captures scrollHeight in the pre-render snapshot so the post-render growth check has a reference baseline.
  • static/i18n.js / static/style.css: adds translated keys and a pill-shaped modifier class for the cue state across all 12 locales.
  • tests/: extends static regression coverage to pin the new preserve-scroll / cue-clear / i18n invariants.

Confidence Score: 5/5

Safe to merge — the cue helpers are well-scoped, the pinned path is unchanged, and the unpinned settle path is correctly guarded by both a scroll-height growth threshold and a distance check.

All 12 locales received the new i18n keys with full parity, the existing scroll-pinned path is untouched, and the new cue state is cleared on every natural dismissal path (scroll near-bottom, button click, session switch, stream reset). No runtime errors or data-loss paths were found in the changed code.

CHANGELOG.md was not updated; all other files look correct.

Important Files Changed

Filename Overview
static/ui.js Core scroll logic: adds cue helpers, captures scrollHeight in snapshot, replaces _followMessagesAfterDomReplace with explicit _scrollPinned branch — logic is correct and well-guarded
static/i18n.js Adds session_new_message / session_new_message_label keys; all 12 locales updated with count parity
static/style.css Adds --new-message modifier class with pill shape and accent colours; scoped override for .session-jump-btn__text display looks correct
tests/test_issue3545_streaming_scroll_cue.py New static regression test covering cue show/hide invariants, i18n keys, CSS hook — thorough for a static test suite
tests/test_issue1690_scroll_completion.py Updated assertion to match new _scrollAfterMessageRender branch structure; change is mechanically correct
tests/test_tars_scroll_reset_regressions.py Assertions updated to reflect new unpinned-branch inline check; mechanically correct
tests/test_issue677.py Widens search window and updates assertion from btn.style.display to _syncScrollToBottomCue call; correct adaptation to the refactored scroll listener
tests/test_session_jump_buttons.py Updates assertion to match new _syncScrollToBottomCue call in scroll listener; mechanically correct

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[renderMessages preserveScroll=true] --> B[_captureMessageScrollSnapshot\ncaptures top, bottom, scrollHeight, pinned]
    B --> C[DOM rebuild]
    C --> D[_scrollAfterMessageRender\npreserveScroll=true]
    D --> E{_scrollPinned?}
    E -- yes --> F[scrollIfPinned\nscroll to bottom]
    E -- no --> G[_restoreMessageScrollSnapshot\nrestore scrollTop]
    G --> H[_maybeShowNewMessageScrollCue]
    H --> I{scrollHeight grew >24px\nAND distance >80px?}
    I -- yes --> J[_showNewMessageScrollCue\nshow accented New message pill]
    I -- no --> K[_syncScrollToBottomCue\nshow/hide plain End button\nbased on distance]
    J --> L[User scrolls near bottom\nor clicks pill]
    L --> M[_clearNewMessageScrollCue\nhide cue, restore End button label]
Loading

Reviews (2): Last reviewed commit: "fix(streaming): show new-message cue whe..." | Re-trigger Greptile

Comment thread static/ui.js
Comment thread static/ui.js
Comment thread static/ui.js
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Heads-up: #3401 (live-to-final assistant reply redesign) shipped in v0.51.294, and we're now at v0.51.301. This PR (#3545 (new-message scroll cue)) is on a stale base and no longer applies cleanly onto current master — the conflict is in static/ui.js scroll/render path, which #3401 rewrote.

The scroll-to-bottom cue functions are largely additive, but the call-site integration conflicts with the rewritten scroll-preservation flow.

Could you rebase onto current master and re-push? Once rebased it'll gate cleanly and I'll fast-track it. I held off rebasing it myself because it touches the freshly-redesigned surface and you're best placed to re-express the intent against the new architecture (rather than me risk reconstructing it wrong). The underlying fix is wanted — it's purely that the base moved out from under it when #3401 landed. Thanks @rodboev!

@nesquena-hermes nesquena-hermes added hold changes-requested Maintainer left detailed feedback requesting changes; PR is waiting on author to address labels Jun 6, 2026
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Cluster review re-confirm (#3400 live-to-final) — rework (rebase), NOT superseded

Re-checked this against current master in the live-to-final cluster sweep (post #3401 / #3763 / v0.51.309). Confirming the disposition explicitly:

  • Not superseded. The new-message cue when scroll position is preserved is not implemented anywhere on master today — I grepped the shipped messages.js/ui.js for the concept and found nothing equivalent. This is a genuine, distinct improvement we want.
  • Needs a rework = rebase. Still hold + changes-requested for the same reason as my earlier comment: it's branched pre-Redesign live-to-final assistant replies #3401 and conflicts in the exact regions Redesign live-to-final assistant replies #3401 rewrote, so it's contributor-rebase territory (re-expressing the intent against the new live-turn architecture is your call, not a force-rebase from us).

No action needed beyond a rebase onto current master; ping me when it's rebased and I'll re-gate fresh (full suite + Codex + Opus). The concept stays wanted. Thanks @rodboev.

…esquena#3545)

# Conflicts:
#	static/ui.js
#	tests/test_issue1690_scroll_completion.py
#	tests/test_tars_scroll_reset_regressions.py
@rodboev rodboev force-pushed the pr/streaming-scroll-cue branch from 9d1a3d6 to b041caa Compare June 7, 2026 06:38
@rodboev

rodboev commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current master and re-expressed this against the current preserve-scroll / live-to-final path. Force-pushed at b041caad.

  1. Wired the new-message cue into the current _scrollAfterMessageRender() preserve-scroll branch instead of the pre-Redesign live-to-final assistant replies #3401 scroll helper shape.
  2. Kept the cue reset / pill-state plumbing in the current scroll listener, scrollToBottom(), and stream/session reset helpers.
  3. Updated the static regression expectations to match the current helper layout while keeping the existing follow-scroll guards intact.

Focused verification I ran for this localized rework:

  • npx eslint --no-config-lookup -c eslint.runtime-guard.config.mjs "static/**/*.js"
  • python "C:\Apps\hermes\run-tests.py" tests\test_issue3545_streaming_scroll_cue.py tests\test_issue1690_scroll_completion.py tests\test_issue677.py tests\test_session_jump_buttons.py tests\test_tars_scroll_reset_regressions.py tests\test_bugfix_sweep.py -v --timeout=60

This should be ready for the fresh re-gate you mentioned.

nesquena-hermes added a commit that referenced this pull request Jun 8, 2026
…) (#3849)

* fix(streaming): show new-message cue when preserving scroll position (#3545)

# Conflicts:
#	static/ui.js
#	tests/test_issue1690_scroll_completion.py
#	tests/test_tars_scroll_reset_regressions.py

* i18n: add missing Polish (pl) translation for session_new_message keys

The PR added session_new_message / session_new_message_label to 12 of 13
locales; Polish was missing both, which fails the per-locale parity test.

Co-authored-by: rodboev <rodboev@users.noreply.github.com>

* fix(streaming): keep forced follow path for pinned users in preserve-scroll branch

Codex CORE catch: the PR's preserve-scroll branch used
'if(_scrollPinned) scrollIfPinned()' which skips the synchronous bottom
write unless distance>500 and can have its delayed settles cancelled by the
DOM-rebuild scroll event — leaving a pinned reader a few lines above the
settled final response. Restore master's _followMessagesAfterDomReplace()
forced-scrollToBottom() path for pinned/near-bottom users; only genuinely
scrolled-up (unpinned, not near bottom) users restore their viewport and
get the new-message cue. Updated the 3 structure-pinning tests to assert the
corrected (safer) shape while preserving their behavioral intent.

Co-authored-by: rodboev <rodboev@users.noreply.github.com>

* Release v0.51.334 — Release KX (new-message cue when scrolled up, #3631)

New-message cue on the jump-to-bottom button when the user has scrolled up
during a live turn (#3545/#3631, @rodboev). Deep-reviewed (Opus+Codex);
maintainer fixes during re-gate: (1) restored master's forced follow path
for pinned/near-bottom users (Codex CORE: scrollIfPinned could leave a pinned
reader short of the settled response) + updated 3 structure-pinning tests to
the corrected shape; (2) added missing Polish (pl) i18n keys (PR had 12/13).
Full suite 8308, ESLint/scope-undef CLEAN, Opus SHIP-safe, Codex SAFE-TO-SHIP.

Co-authored-by: rodboev <rodboev@users.noreply.github.com>

---------

Co-authored-by: Rod Boev <rod.boev@gmail.com>
Co-authored-by: Hermes Agent <hermes-agent@nesquena-hermes.local>
Co-authored-by: rodboev <rodboev@users.noreply.github.com>
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Shipped in v0.51.334 (Release KX) via #3849 — thanks @rodboev! 🎉

I deep-reviewed this (Opus + Codex), rebased it onto current master, and shipped it with two fixes.

What landed: when you scroll up to read during a live turn, a new message no longer silently lands off-screen nor yanks you to the bottom — the jump-to-bottom button shows a "New message" cue you click to catch up. Pinned/at-bottom readers still auto-follow as before.

Fix #1 (Codex CORE catch): the preserve-scroll branch used if(_scrollPinned) scrollIfPinned(), but scrollIfPinned() skips the synchronous bottom write unless distance > 500 and its delayed settles can be cancelled by the DOM-rebuild scroll event — so a pinned reader could be left a few lines above the settled final response. I restored master's forced follow path (if(_followMessagesAfterDomReplace()) return; → forced scrollToBottom()) for pinned/near-bottom users, so only genuinely scrolled-up users restore their viewport and get the cue. Updated the 3 structure-pinning tests to the corrected shape.

Fix #2 (i18n): the two new session_new_message* keys were translated into 12 of 13 locales — Polish (pl) was missing both. Added the Polish translations → all 13/13.

I also independently confirmed the snapshot-shape is contained to a single producer (no spurious-cue path) and the cue state resets on every session/stream boundary.

Gate: full suite 8308 passed, ESLint runtime + scope-undef + ruff clean, Opus SHIP-safe, Codex SAFE-TO-SHIP (after fix #1), CI 11/11 green. Co-authored to you.

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

Labels

changes-requested Maintainer left detailed feedback requesting changes; PR is waiting on author to address hold

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants