Skip to content

fix: hold the scroll position when older events load above - #4007

Merged
chasers merged 4 commits into
mainfrom
fix/load-more-top-scroll-anchor
Sep 18, 2026
Merged

chasers merged 4 commits into
mainfrom
fix/load-more-top-scroll-anchor

Conversation

@chasers

@chasers chasers commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Follows #3986. Reported on staging: clicking "Load more" at the top moves the viewport, and the jump is obvious when a log line wraps to several lines.

Cause

#logs-list is flex-direction: column-reverse, so the top button inserts older events at the visual top and pushes every row below them down.

Two things moved the scroll on that load, and neither knew about the other:

  1. updated/0 ran restoreScrollAnchor/0, holding the visible row in place.
  2. put_event_page/3 pushed scroll-to-event, jumping the oldest new row to the top.

Both ran inside requestAnimationFrame, so whichever fired second won. A following diff (the URL patch, the chart, the buttons) ran the anchor restore again. updated/0 guarded the restore for scroll-to-bottom only, so scroll-to-event never got that discipline.

Both failure modes scale with the height of the inserted batch, which is why wrapped lines made it visible and single lines did not.

Fix

  • Drop push_scroll_to_oldest/2 and the scroll-to-event push. Nothing else used that event, so the hook handler goes too. The anchor restore is now the only owner of the scroll on a page load.
  • Stop falling back to window.scrollTo(0, this.scrollPosition). That offset is stale once rows land above the viewport, and it was wrong by exactly the height of those rows.
  • Keep an anchor only when the row carries an id.

Tests

The "anchor row is gone" test fails against the old fallback and passes against the fix. I verified both directions.

  • 226 Elixir tests pass across the 5 affected files
  • 70 JS tests pass
  • format, lint.all, test.slop (27/29) and test.structure pass

Please confirm on staging with a source that has wrapped log lines.

🤖 Generated with Claude Code

@chasers

chasers commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Reproduced on staging, and the fix measured at zero drift

Attached Playwright to a real Chrome over CDP against test.logflarestaging.com, source 58, t:2026-09-17T09:{26..30}:00, US/Arizona. 100 rows on screen, 100 added per click, tallest row 195px, median 20px.

As deployed

driftPx 2408    anchorTop 459 -> 2867    scrollY 0 -> 225

Timeline:

t=1151  y=4     scrollBy [0, 2628.5]      <- anchor restore, correct
t=1152  y=2633  scrollIntoView log-...    <- scroll-to-event, wins
t=1177  y=225                             <- lands 2408px off

restoreScrollAnchor/0 computes the right compensation and applies it. One millisecond later scroll-to-event runs scrollIntoView({block: "start"}) on the oldest new row and overwrites it. That is the race this PR removes.

With scroll-to-event suppressed, which is exactly what this PR does

driftPx 0    anchorTop 459 -> 459    scrollY 0 -> 2633

The row the reader was on does not move.

Note for reviewers

This does not reproduce against a local dev source. There document.getElementById(id) returns null, so the ?. swallows the call and the jump never happens. You need a source where the oldest row's dom id resolves, which staging has.

`#logs-list` is `column-reverse`, so the top button inserts older events at
the visual top and pushes every row below them down.

Two things moved the scroll on that load and neither knew about the other.
`updated/0` ran `restoreScrollAnchor/0` to hold the visible row in place,
while `put_event_page/3` pushed `scroll-to-event` to jump the oldest new row
to the top. Both ran inside `requestAnimationFrame`, so the later one won,
and a following diff ran the anchor restore again. Both errors scale with
the height of the batch, so a wrapped log line made the jump obvious.

- Drop `push_scroll_to_oldest/2` and the `scroll-to-event` push. Nothing
  else used that event, so the hook handler goes too. The anchor restore is
  now the only owner of the scroll on a page load.
- Stop falling back to `window.scrollTo(0, this.scrollPosition)`. That
  offset is stale once rows land above the viewport, and it was wrong by
  exactly the height of those rows. Leave the scroll alone instead.
- Keep an anchor only when the row carries an id.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@chasers
chasers force-pushed the fix/load-more-top-scroll-anchor branch from 0d7566b to 6426ee5 Compare September 18, 2026 18:05
The unit tests around this fix stub `getBoundingClientRect`, so they pass
whatever the real layout does. `#logs-list` is `column-reverse`, and only
a browser works out where a row lands once older rows enter above it.

The test reads a visible row's viewport position, loads an older page, and
asserts the row did not move. Messages carry a long stack-trace tail so
rows wrap, which is the case that makes the drift large.

I could not run it locally. The whole feature suite fails on this machine,
including `searches logs from the search page`, which predates this work
and renders no rows. `E2E=true` and `npm run deploy --prefix assets` did
not change that. The e2e workflow watches `test/e2e/features/**`, so the
PR run exercises it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011XYNBQPEPUvDU2ESE4Gcaa
Comment thread assets/js/source_lv_hooks.js Outdated
Comment thread assets/js/test/source_lv_hooks.test.js Outdated
Comment thread assets/js/test/source_lv_hooks.test.js
Comment thread test/logflare_web/live/search_live/logs_search_lv_test.exs
- Restore the anchor inline in `updated/0` rather than on the next frame.
  `beforeUpdate` captures synchronously, so a following diff could capture
  an already shifted top and cancel the first correction. The DOM is
  patched by the time `updated/0` runs, and `getBoundingClientRect` forces
  layout, so the inline read is correct.
- Cover that ordering: the anchor moves before any frame runs.
- Drop the `requestAnimationFrame` stub the anchor tests no longer need.
- Drop `refute_push_event(view, "scroll-to-event", %{})`. No code path
  emits that event now, and the e2e test covers the behaviour it stood in
  for.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011XYNBQPEPUvDU2ESE4Gcaa
@chasers
chasers requested a review from amokan September 18, 2026 18:34
Reinstates `refute_push_event(view, "scroll-to-event", %{})` on the
previous-page test.

The review asked to drop it as unfailable. It is not: it fails the moment
`push_scroll_to_oldest/2` comes back, which is the regression this branch
exists to prevent. The e2e test covers the same ground through the
browser, but it is one feature test away from leaving this branch with no
cheap guard at all.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011XYNBQPEPUvDU2ESE4Gcaa
@chasers
chasers merged commit 5acfd0c into main Sep 18, 2026
16 checks passed
@chasers
chasers deleted the fix/load-more-top-scroll-anchor branch September 18, 2026 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants