Skip to content

fix: follow content growth from first item measurements at the end - #520

Merged
jmeistrich merged 1 commit into
LegendApp:mainfrom
chrisnojima:fix-maintain-scroll-at-end-first-measurement
Aug 10, 2026
Merged

fix: follow content growth from first item measurements at the end#520
jmeistrich merged 1 commit into
LegendApp:mainfrom
chrisnojima:fix-maintain-scroll-at-end-first-measurement

Conversation

@chrisnojima

Copy link
Copy Markdown
Contributor

Problem

A list pinned to its end drifts short after a prepend, and never recovers.

updateItemSizes asks for a re-pin from an item layout only when the row already had a known size that moved more than 5px:

// Check if we should maintain scroll at end
if (prevSizeKnown !== undefined && Math.abs(prevSizeKnown - size) > 5) {
    shouldMaintainScrollAtEnd = true;
}

Rows measured for the first time take the undefined branch, so they never request one and maintainScrollAtEnd.onItemLayout cannot fire for them.

Prepending is entirely first measurements. The sequence:

  1. Data changes → onDataChange re-pins against the content size at that instant.
  2. The newly inserted rows measure for the first time and grow the content underneath.
  3. No re-pin follows that growth.
  4. The residue ends up smaller than maintainScrollAtEndThreshold, so isWithinMaintainScrollAtEndThreshold and isAtEnd both read true and the list considers itself finished — parked a couple hundred pixels short of the last item.

Step 4 is what makes it stick: the list believes it is already at the end, so nothing later corrects it.

Evidence

Instrumented updateItemSizes and doMaintainScrollAtEnd in a running web list and ran the failing flow:

measurements:   285 total  →  first=true: 285,  first=false: 0
maintain calls: 3         →  last was the onDataChange re-pin
after it:       ~25 further first measurements moved totalSize
                scroll frozen, no maintain call
final:          gap = +57px  (pinned baseline is -116px)

Every measurement in the flow is a first measurement, so the item-layout path is unreachable end to end.

With the condition flipped, the same flow:

maintain calls: 9
trailing measurements coalesce via pendingMaintainScrollAtEnd
final:          gap = -116px   (pinned)

Fix

Treat a first measurement as maintain-worthy too. doMaintainScrollAtEnd still gates on isWithinMaintainScrollAtEndThreshold and didContainersLayout, so this only follows growth for a list already sitting at its end, and concurrent requests still coalesce through pendingMaintainScrollAtEnd.

Test

Every existing test in updateItemSize.test.ts pre-seeds sizesKnown, so none covered a first measurement — which is why this path had no coverage. The added test asserts doMaintainScrollAtEnd is called when sizesKnown has no entry for the item; it fails on main.

Verified: bun run lint, bun run tsc:src, bun test (1554 pass / 0 fail), bun run build.

Related: #518, #519 (independent 3.3.x issues).

🤖 Generated with Claude Code

maintainScrollAtEnd requested a re-pin from an item layout only when the
row already had a known size that moved more than 5px:

    if (prevSizeKnown !== undefined && Math.abs(prevSizeKnown - size) > 5)

Rows measured for the first time take the undefined branch and never
request one, so maintainScrollAtEnd.onItemLayout cannot fire for them.

That strands a list at its end after a prepend. The data change re-pins
against the content size at that instant, then the newly inserted rows
measure for the first time and grow the content underneath it. Nothing
follows that growth, and once the residue is smaller than
maintainScrollAtEndThreshold the list reports isAtEnd and stops, parked
short of the last item.

Instrumenting a running list through the failing flow recorded 285 item
measurements, every one of them a first measurement, against 3 calls to
doMaintainScrollAtEnd -- the last being the data-change re-pin. With the
condition flipped the same flow records 9 calls, the trailing measurements
coalesce through pendingMaintainScrollAtEnd, and the list settles at its
end instead of short.

doMaintainScrollAtEnd still gates on isWithinMaintainScrollAtEndThreshold
and didContainersLayout, so this only follows growth for a list already
sitting at its end.
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

chrisnojima added a commit to keybase/client that referenced this pull request Aug 10, 2026
The patch now holds exactly the three fixes filed against the library, so
each hunk has a PR to track and a clear removal trigger when it lands:

- ScheduledWork.cancel receiver bug (LegendApp/legend-list#518)
- useFreshDataTransitionVisibility pending flag outliving the transition
  (LegendApp/legend-list#519)
- maintainScrollAtEnd ignoring first measurements
  (LegendApp/legend-list#520)

#519 was not carried before, because remounting on clear sidesteps the
stuck-hidden containers. Carrying it keeps the local tree honest against
what we reported, and makes dataKey usable again whenever we want to drop
the remount.

Dropped both ResizeObserver hunks. They date from 3.0.3, when the global
observer ran callbacks in a bare loop with no batching at all; 3.3.3 added
batchItemSizeUpdates around that same loop for the same reason, so the
coalescing is now upstream. What our version still added was a macrotask
of deferral, which delays measurement past paint -- the same shape as the
bugs fixed above.

Removal is NOT yet validated at runtime. These were added against runtime
errors, so the build needs an error hunt across window resize, info panel
and search bar toggles, and hard scrolling before this is trusted.
chrisnojima added a commit to keybase/client that referenced this pull request Aug 10, 2026
* refactor(list): drop redundant LegendList ref/state types

getState() is part of the published LegendListRef as of 3.3.x, and its
LegendListState is a superset of the four fields we declared. Re-export
the library type instead of intersecting our own onto it.

* refactor(chat): drop LegendList desktop workarounds fixed upstream

Three workarounds in the desktop thread predate legend-list 3.3.x and no
longer describe how the library behaves.

Remount on clear -> dataKey. The list handles a non-empty -> empty ->
non-empty transition itself now: updateProps bumps freshDataTransitionEpoch
when the dataset identity changes or the previous data was empty, which
resets layout readiness and re-runs the initial scroll. dataKey expresses
the same intent as the key remount but keeps measurement and position
caches, so a centered load no longer re-measures the whole thread.

rAF data gate. This guarded handleInitialScrollDataChange bailing on
!queuedInitialLayout. That branch is now unreachable for us: passing
initialScrollAtEnd or initialScrollIndex selects the bootstrap path, which
returns before the check, and the desktop list always passes one of them.

maintainScrollAtEndThreshold widening. 3.3.4 changed the deferred re-pin to
run when the user has not scrolled since the request, instead of requiring
the gap to still be within the threshold, which is the case this widened
the threshold for. The settle timer and its wheel teardown go with it.

The wheel handler keeps aborting the centering loop.

* fix(chat): keep remounting the desktop thread on clear

dataKey is the library's answer to a non-empty -> empty -> non-empty
dataset swap, but it cannot be used here yet. The fresh-data reset clears
readyToRender and restores it inside one layout-effect pass, so the
containers never render in the not-ready state that
useFreshDataTransitionVisibility waits for before clearing its pending
flag, and the wrapper stays at opacity 0 with the thread fully measured
behind it. Confirmed live: readyToRender true, didContainersLayout true,
dataLen 100, completedTransitionEpoch 1 against a transition epoch of 2.

Reported upstream as LegendApp/legend-list#519. Revisit dataKey once that
lands -- it avoids throwing away measurement caches on every centered load.

The comment now records the real mechanism instead of the earlier guess
about a container layout event that never arrives.

* fix(deps): patch legend-list to follow growth from first measurements

maintainScrollAtEnd asks for a re-pin from an item layout only when the
row already had a known size that moved more than 5px:

    if (prevSizeKnown !== void 0 && Math.abs(prevSizeKnown - size) > 5)

Rows measured for the first time take the `void 0` branch and never
request one, so `maintainScrollAtEnd.onItemLayout` is unreachable for
them.

That strands the thread after loading older messages. The prepend fires
onDataChange, which re-pins against the content size at that instant, and
the freshly inserted rows then measure for the first time and grow the
content underneath it. Nothing follows that growth, and the leftover gap
lands inside maintainScrollAtEndThreshold, so the list reports isAtEnd and
considers itself done -- parked a couple hundred pixels short of the
newest message.

Instrumenting the library in a running thread showed 285 measurements in
the flow, every one of them a first measurement, against 3 maintain calls
total, the last of which was the onDataChange re-pin. With the condition
flipped, the same flow reports 9 maintain calls, the trailing measurements
coalesce through pendingMaintainScrollAtEnd, and the thread settles at the
pinned baseline instead of short.

Still gated by isWithinMaintainScrollAtEndThreshold and didContainersLayout,
so this only follows growth for a list already sitting at its end.

* refactor(deps): carry only the legend-list fixes we sent upstream

The patch now holds exactly the three fixes filed against the library, so
each hunk has a PR to track and a clear removal trigger when it lands:

- ScheduledWork.cancel receiver bug (LegendApp/legend-list#518)
- useFreshDataTransitionVisibility pending flag outliving the transition
  (LegendApp/legend-list#519)
- maintainScrollAtEnd ignoring first measurements
  (LegendApp/legend-list#520)

#519 was not carried before, because remounting on clear sidesteps the
stuck-hidden containers. Carrying it keeps the local tree honest against
what we reported, and makes dataKey usable again whenever we want to drop
the remount.

Dropped both ResizeObserver hunks. They date from 3.0.3, when the global
observer ran callbacks in a bare loop with no batching at all; 3.3.3 added
batchItemSizeUpdates around that same loop for the same reason, so the
coalescing is now upstream. What our version still added was a macrotask
of deferral, which delays measurement past paint -- the same shape as the
bugs fixed above.

Removal is NOT yet validated at runtime. These were added against runtime
errors, so the build needs an error hunt across window resize, info panel
and search bar toggles, and hard scrolling before this is trusted.
@jmeistrich
jmeistrich merged commit ac4fa07 into LegendApp:main Aug 10, 2026
chrisnojima added a commit to keybase/client that referenced this pull request Aug 10, 2026
3.3.5 ships all three fixes we were carrying locally, so the patch file
goes away entirely:

- ScheduledWork.cancel receiver bug (LegendApp/legend-list#518)
- useFreshDataTransitionVisibility pending flag outliving the transition
  (LegendApp/legend-list#519)
- maintainScrollAtEnd ignoring first measurements
  (LegendApp/legend-list#520)

Verified each fix is present in all six published entrypoints
(react/react-native/react-native.web, .js and .mjs), not just the one
the desktop build pulls.

#519 landing upstream is what the dataKey switch in the previous commit
was waiting on; it now holds without a local patch.
chrisnojima added a commit to keybase/client that referenced this pull request Aug 10, 2026
… patch (#29526)

* fix(chat): use dataKey instead of remounting the desktop thread

The centered-load remount was kept because useFreshDataTransitionVisibility
left isTransitionPending set after the fresh-data reset, leaving the wrapper
at opacity 0 with the thread measured behind it. The fix for that
(LegendApp/legend-list#519) is already carried in
patches/@LegendApp+list+3.3.4.patch, so dataKey works now.

dataKey expresses the same intent as the key remount but keeps the
measurement and position caches, so a centered load no longer re-measures
the whole thread.

Verified live across search-hit, reply-quote and pinned-message jumps and
conversation switches.

* chore(deps): legend-list 3.3.5, drop our patch

3.3.5 ships all three fixes we were carrying locally, so the patch file
goes away entirely:

- ScheduledWork.cancel receiver bug (LegendApp/legend-list#518)
- useFreshDataTransitionVisibility pending flag outliving the transition
  (LegendApp/legend-list#519)
- maintainScrollAtEnd ignoring first measurements
  (LegendApp/legend-list#520)

Verified each fix is present in all six published entrypoints
(react/react-native/react-native.web, .js and .mjs), not just the one
the desktop build pulls.

#519 landing upstream is what the dataKey switch in the previous commit
was waiting on; it now holds without a local patch.
@jmeistrich

Copy link
Copy Markdown
Member

Thanks for all the PRs!

@chrisnojima

Copy link
Copy Markdown
Contributor Author

Thanks @jmeistrich for the library and all the recent work, awesome stuff

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