Skip to content

fix: compensate header size changes for MVCP on native - #523

Open
giaBaoJS wants to merge 1 commit into
LegendApp:mainfrom
giaBaoJS:fix/native-header-mvcp-compensation
Open

fix: compensate header size changes for MVCP on native#523
giaBaoJS wants to merge 1 commit into
LegendApp:mainfrom
giaBaoJS:fix/native-header-mvcp-compensation

Conversation

@giaBaoJS

Copy link
Copy Markdown

Fixes #515.

User-facing impact

With maintainVisibleContentPosition, a ListHeaderComponent that changes size while the user is scrolled below it was compensated on web only. On iOS and Android the viewport jumped by the header's size delta. The reporter hit this in a chat-style thread that swaps a small "load previous" spinner for a full post card on the last upward page.

Root cause

shouldAdjustForHeaderSizeChange in src/core/updateContentMetrics.ts opened with Platform.OS === "web" &&, so setHeaderSize never reached requestAdjust on native.

Nothing else covers this on native. MVCP anchors on state.positions[] (prepareMVCP, src/core/mvcp.ts), and those positions exclude headerSize — the header is folded in separately as topPad in calculateItemsInView.ts and via getContentSize. A header-only size change therefore produces no item position diff for MVCP to act on.

Why removing the gate is safe

The gate came from f46e6dd ("fix: preserve web MVCP for changing headers #468"), whose message says it tracks header baselines "on web". #468 was reported and tested on web only, so the web scoping looks like scope of testing rather than a deliberate native exclusion.

Removing the clause routes native onto the same requestAdjust path already used for item size changes, which is native-aware in its own right (src/utils/requestAdjust.ts): the Android old-architecture doScrollTo workaround and the ignoreScrollFromMVCP threshold window are native-only branches.

On double-compensation against native RN MVCP: the RN maintainVisibleContentPosition prop is passed to the ScrollView with minIndexForVisible: 0 (src/components/ListComponent.tsx), and the first content subview is the ScrollAdjust sentinel — a 0x0 absolutely positioned View at top: scrollAdjust + 10_000_000 (src/components/ScrollAdjust.native.tsx). Because that sentinel's origin always exceeds the content offset, RN's "first visible view" search picks it every time, so native MVCP only ever tracks the scrollAdjust signal. Routing the header delta through requestAdjust is the single intended path, not a second one. The measured behavior below confirms it: the offset moves by exactly the header delta, not twice.

The remaining guards are untouched, so the initial header measurement and changes made while the header is still visible stay uncompensated on every platform.

Verification

bun test 1590 pass / 0 fail, bun run lint, bun run tsc:src, bun run tsc:example, bun run build all clean.

Unit (__tests__/core/contentMetrics.test.ts): the header compensation test and the header-still-visible test are now parameterised over web / ios / android, plus a new test per platform that drives the real ScrollAdjustHandler (no requestAdjust spy) and asserts the resulting settle position, not just that an adjustment was requested.

Counterfactual — with the source change reverted and only the tests applied, the 4 native cases fail:

(fail) compensates MVCP on ios when a measured header changes above the viewport
(fail) settles the scroll position on ios when a measured header grows above the viewport
(fail) compensates MVCP on android when a measured header changes above the viewport
(fail) settles the scroll position on android when a measured header grows above the viewport
 17 pass, 4 fail

Web stayed green throughout. With the fix restored: 21 pass / 0 fail in that file.

iOS simulator (iPhone 17 Pro, iOS 26.3, New Architecture, example fixtures) using the new header-mvcp fixture, starting at initialScrollIndex={20} with scrollOffset: 1536 and Row 20 at the top of the viewport:

Action Before After
Grow header 96 → 176 offset stays 1536, content jumps — Row 19 takes Row 20's place offset 1536 → 1616, Row 20 stays put
Shrink header 176 → 96 offset 1616 → 1536, Row 20 stays put
Grow while scrolled to top (header visible) offset stays 0, header grows in place (guard still holds)
Scroll back to top after several grow/shrink cycles offset 0, header renders at its current height, no accumulated drift

The before column is the same build with only the Platform.OS === "web" && clause restored, verified back-to-back on the same simulator.

Android was not exercised on a device; it is covered by the unit tests only.

Fixture

example/screens/fixtures/header-mvcp.tsx mirrors the existing example-web/src/fixtures/HeaderMvcpExample.tsx added for #468, so the same scenario can be checked manually on native. It shows the live scrollOffset so the settle position is directly observable.

Out of scope

The issue also mentions a one-frame flash that remains after this fix — the grown header paints at the old offset for a frame before the adjustment lands, because native applies it after the layout commit while web applies it synchronously. That is a separate problem (it needs the adjustment to be applied synchronously with the layout commit, or an estimatedHeaderSize seed) and is deliberately left as follow-up. This PR only fixes the settle position.

`shouldAdjustForHeaderSizeChange` opened with `Platform.OS === "web"`, so
`setHeaderSize` only requested an MVCP scroll adjustment on web. On iOS and
Android a `ListHeaderComponent` that changed size while the user was scrolled
below it went uncompensated and the viewport jumped by the header's size delta.

Nothing else covered this on native: MVCP anchors on `positions[]`, which
exclude `headerSize` (the header is folded in separately as `topPad` in
`calculateItemsInView` and via `getContentSize`), so a header-only size change
produces no item position diff for `prepareMVCP` to act on.

The web gate came from LegendApp#468, which was reported and tested on web only, so it
was scope of testing rather than a deliberate native exclusion. Removing the
clause routes native through the same `requestAdjust` path already used for
item size changes, which has native-specific handling of its own (the Android
old-architecture `doScrollTo` workaround and the `ignoreScrollFromMVCP`
threshold window). The remaining guards are unchanged, so the initial header
measurement and changes made while the header is still visible stay
uncompensated on every platform.

Covers the core behavior across web/ios/android, including the resulting settle
position through the real `ScrollAdjustHandler`, and adds a native `header-mvcp`
fixture mirroring the existing web `HeaderMvcpExample`.

Fixes LegendApp#515

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b93c6de0a5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines 27 to 29
props.maintainVisibleContentPosition.size &&
didContainersLayout &&
didFinishInitialScroll &&

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid jumping when a zero-baseline header appears at top

When ListHeaderComponent is initially absent (or first measures to 0), setHeaderSize still marks didMeasureHeader; if that header later grows while the user is at offset 0, previousHeaderEnd is also 0, so this now calls requestAdjust on iOS/Android and advances the native scroll by the header height even though the header is visible rather than above the viewport. The native expansion should keep this top/zero-baseline case uncompensated (for example, require scroll to be strictly past the previous header end) before broadening the old web-only behavior.

AGENTS.md reference: AGENTS.md:L20-L22

Useful? React with 👍 / 👎.

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.

ListHeaderComponent size changes aren't scroll-compensated on native (only web) with maintainVisibleContentPosition

1 participant