Conversation
|
View your CI Pipeline Execution ↗ for commit 1ed5565
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
hoebbelsB
force-pushed
the
fix-keep-scrolled-index
branch
from
January 2, 2026 21:07
843a285 to
feeb9e1
Compare
This fix prevents items from jumping from the anchor items current offset to it's scrollTop when scrolling upwards.
The scroll strategies only compute a rendered range once `containerRect$` emits, and that stream was fed exclusively by a `ResizeObserver`. The observer delivers its first entry only when the browser produces a rendering frame. A viewport created in a hidden or fully occluded tab (chrome classifies a covered window as invisible) therefore never received a container size and rendered nothing at all - permanently, until the tab became visible again. Measure the container synchronously in `ngAfterViewInit` and seed `containerRect$` with it. Layout reads are available regardless of frame production, and the ResizeObserver keeps the value up to date from there. Co-Authored-By: Claude Fable 5 <[email protected]>
Prepending items shifts the rendered range by exactly the amount of inserted items, which leaves the rendered slice identical. The IterableDiffer reports no changes, so rendering bailed out through `NEVER` - but every view now maps to a new index and has to be re-positioned, leaving the list drawn at stale offsets. Detect that case and re-render the views in place. Two details matter for the strategies consuming these events: - the batch handed to `renderingStart$` names every view index, because every view emits `viewRendered$` right after. An empty batch makes the strategies' position pass fast-forward its running cursor past views that still emit afterwards, stacking them at the end of the content - visible as a blank viewport with gaps between items. - `viewRendered$` carries the view container index, not the context index. The two diverge as soon as `range.start > 0`. Co-Authored-By: Claude Fable 5 <[email protected]>
`keepScrolledIndexOnPrepend` compensated the scroll position by counting how many items were inserted before the anchor. That breaks as soon as the update is not a pure insert - a chat client showing a transient "loading older messages" row performs an insert *and* a remove, leaving the list shifted by the height of that row (#1857). Locate the anchored item again instead, via `trackBy`. That nets out inserts, removals and moves ahead of the anchor in one pass, and shifts by the resulting delta so the anchor keeps its sub-item offset - scrolling to the top of the new anchor index is what produced the reported jump. Further correctness fixes found while validating the above: - when the anchored item itself disappears (the anchor sits *on* the transient row, which happens whenever the user waits at the very top), fall back to the nearest following survivor rather than bailing out. Bailing left the viewport pinned to the top, which in a reverse infinite scroller retriggers loading forever. - autosize resolves the item index of a view through the trackBy cache instead of `view.context.index`. A second emission rebuilds the size ledger while a render or ResizeObserver pass for the previous dataset is still in flight, and the stale index booked measurements onto the neighbouring item. Since sizes are cached, that corruption was permanent and showed up as overlapping items and gaps while scrolling. - `waitForScroll` is decided against the scroll target *clamped* to the scrollable bounds. A scroll the browser cannot perform emits no scroll event, so an out-of-bounds target latched `isStable` to false forever and froze rendering with a stale loading row on screen. - autosize applies the range shrink independently of the relocation, so a transient row that was both the anchor and the last item of the rendered range no longer wedges a pending `scrollToIndex`. - render and measure passes tolerate indices of a superseded dataset instead of dereferencing a removed ledger entry. Adds cypress coverage for all of the above across the fixed, dynamic and autosize strategies, including the render-without-ResizeObserver-frames and range-shift cases from the preceding two commits. Co-Authored-By: Claude Fable 5 <[email protected]>
Make the demo behave like an actual chat client so it exercises the prepend compensation the way real consumers do: - a transient "loading older messages" row inside the list while a history request is in flight, toggleable, plus a slower simulated backend so that state is actually observable - history requests are driven by user input (wheel upwards, or arriving at the very top) rather than by range emissions. Passive scroll changes - the initial scroll, prepend compensations, measurement corrections - all report a range starting at 0 and would otherwise chain-load without any interaction. Wheel events are debounced to one trigger per gesture, since trackpad momentum keeps emitting for seconds after the flick. - the batch runs dry at the beginning of the conversation instead of refetching the first page forever Co-Authored-By: Claude Fable 5 <[email protected]>
hoebbelsB
force-pushed
the
fix-keep-scrolled-index
branch
from
August 3, 2026 21:49
feeb9e1 to
1ed5565
Compare
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🎓 Learn more about Self-Healing CI on nx.dev
|
@hoebbelsB when do you plan to merge this PR? )) |
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
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.
Extension of #1859, as I couldn't push to the fork.
Fixes #1857.
Problem
keepScrolledIndexOnPrependcompensated the scroll position by counting how many items were inserted ahead of the anchored item. That only holds for a pure prepend.A real reverse infinite scroller — a chat client loading older messages — also renders a transient "loading older messages" row while the request is in flight, so the update that resolves it is an insert and a remove. Counting insertions mis-compensates by the height of that row and the list jumps.
Approach
Locate the anchored item again via
trackByinstead of counting insertions. That nets out inserts, removals and moves ahead of the anchor in a single pass.The compensation then shifts by the resulting delta rather than scrolling to the top of the new anchor index — the latter drops the anchor's sub-item offset, which is the jump reported in #1857.
Also fixed
Validating the above against a realistic demo surfaced several further defects in the same area. Each has its own regression spec.
containerRect$emits, and it was fed exclusively by aResizeObserver— which never delivers its first entry without a rendering frame (hidden or fully occluded tab). Now seeded with a synchronous measurement on init.NEVER. Now re-renders in place — and announces the batch correctly, since an empty batch made the strategies' position pass fast-forward its cursor past views that still emit afterwards.view.context.index, which goes stale when a second emission rebuilds the size ledger while a render orResizeObserverpass for the previous dataset is still in flight. Because sizes are cached, that corruption was permanent. Now resolved through thetrackBycache.waitForScrollblocks rendering until a scroll event arrives, but a target outside the scrollable bounds is clamped by the browser and emits no event — latchingisStabletofalseforever. Now decided against the clamped target.Additionally, autosize applies the range shrink independently of the relocation (a transient row that was both the anchor and the last item of the rendered range used to wedge a pending
scrollToIndex), and render/measure passes tolerate indices belonging to a superseded dataset instead of dereferencing a removed ledger entry.Testing
nx run template:component-test— 82 specs, 23 of them new, across the fixed, dynamic and autosize strategies. Each fix listed above has a spec that was verified to fail against the unfixed code.Demo
apps/demos→ Virtual Scrolling → Reverse Infinite Scroll was reworked to behave like an actual chat client, which is what surfaced most of the defects above:0and would otherwise chain-load without any interaction. Wheel events are debounced to one trigger per gesture, since trackpad momentum keeps emitting for seconds after the flick.Known limitation
Scrolling upward into items that have never been measured can still produce a visible correction: the anchor walk uses
tombstoneSizeestimates, andmaybeAdjustScrollPositionapplies the real geometry a frame later, mid-gesture. With high per-item size variance this reads as rubber-banding near the top.Setting
tombstoneSizeclose to the real mean item height reduces it substantially, and that is worth documenting for reverse infinite scrollers. Damping corrections while a gesture is in progress would address the remainder, but that is a behavioural change to the scroll strategies and is left for a separate PR.