Skip to content

fix: stop rebuilding the web measurement probe on every render - #748

Open
giaBaoJS wants to merge 1 commit into
appandflow:mainfrom
giaBaoJS:fix/web-remeasure-on-every-render
Open

fix: stop rebuilding the web measurement probe on every render#748
giaBaoJS wants to merge 1 commit into
appandflow:mainfrom
giaBaoJS:fix/web-remeasure-on-every-render

Conversation

@giaBaoJS

Copy link
Copy Markdown

There is no issue filed for this — I found it while reading NativeSafeAreaProvider.web.tsx, so I'm leading with the reproduction rather than a report.

What happens

The measurement effect in src/NativeSafeAreaProvider.web.tsx had [onInsetsChange] in its dependency array. SafeAreaListener (src/SafeAreaContext.tsx:122) builds its onInsetsChange as an inline arrow inside its own render:

onInsetsChange={(e) => {
  onChange({ insets: e.nativeEvent.insets, frame: e.nativeEvent.frame });
}}

That arrow is a new function on every render of SafeAreaListener, so the effect's cleanup and setup ran on every render of the listener. Each cycle:

  • removes the hidden probe <div> from document.body and appends a freshly created one,
  • removes and re-adds the transitionend listener on the probe and the resize listener on window,
  • disconnects the ResizeObserver and constructs and re-observes a new one,
  • calls window.getComputedStyle(element) and providerElement.getBoundingClientRect() and fires onInsetsChange again.

Note this is driven by render count, not by anything actually changing. It fires even when onChange is a stable function, because the churn comes from SafeAreaListener's own inline arrow, not the caller's prop.

SafeAreaProvider is not affected — it already memoises its callback with React.useCallback(..., []) at src/SafeAreaContext.tsx:56. The path here is SafeAreaListener on web.

Reproduction

I added three tests to the existing src/__tests__/NativeSafeAreaProvider.web-test.tsx jsdom suite. The user-visible one renders a real SafeAreaListener with a stable onChange and re-renders it three times, then counts probe attachments, ResizeObserver constructions and measurement calls.

On main, after 3 renders:

before after
probe elements attached to document.body 3 1
ResizeObserver instances constructed 3 1
onChange calls 3 1

I have measured the setup/teardown counts above and nothing else — I am not claiming a frame-time or benchmark number.

The fix

Store the latest onInsetsChange in a ref and drop it from the effect's dependencies, so setup runs once per mount. 10 lines of source change.

I considered fixing it in SafeAreaListener instead by wrapping its callback in useCallback([onChange]), and rejected it: that only helps callers who already memoise onChange, and an inline onChange — the common usage — would still churn. Fixing it in the web provider makes the effect correct for every caller. The effect body genuinely does not depend on the callback's identity, only on being able to call the current one, so the ref is the right tool here; there are no other values in the closure that would go stale (viewRef is a ref and createContextElement is module-level).

For contrast, the native NativeSafeAreaProvider passes onInsetsChange straight through to the native view, where a changing identity is just a prop update with no teardown — the web implementation is the only one holding resources across renders, so this stays scoped to the web file.

Not regressing what the effect is for

Dropping a dependency risks a stale callback, so one of the tests guards exactly that: it mounts with callback A, re-renders with callback B (asserting B is not called just for being swapped in), then triggers a genuine resize and asserts B receives the new insets and frame while A is not called again. I verified this test fails if I take the dependency-array change without the ref, so the ref is load-bearing and not decoration. The existing tests covering window resize, ResizeObserver updates and the ResizeObserver-less fallback all still pass.

yarn test passes (prettier, eslint, tsc, jest — 24 tests, 11 snapshots). The 3 eslint no-deep-imports warnings are pre-existing on main.

Possible overlap

If an unstable_disableViewOnWeb prop for SafeAreaProvider (#637) lands around the same time it may touch this file. I have kept this change as narrow as I could — it only touches the ref plumbing and the dependency array, not the measurement logic or the rendered tree — so it should rebase cleanly either way.

The measurement effect in NativeSafeAreaProvider.web listed onInsetsChange
in its dependency array. SafeAreaListener passes a fresh inline arrow on
every render, so every render of a SafeAreaListener tore down and rebuilt
the hidden probe element, its transitionend and resize listeners and the
ResizeObserver, and re-measured.

The effect never needs the callback's identity, only the latest callback,
so read it through a ref and drop it from the dependencies. Setup now runs
once per mount while inset and frame changes still propagate to the most
recent callback.
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.

1 participant