fix: stop re-serializing overlay props on unrelated re-renders - #58
Merged
Conversation
|
React Doctor found 6 issues in 3 files · 2 errors & 4 warnings · score 64 / 100 (Needs work) · full project Errors
4 warnings
Reviewed by React Doctor for commit |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 SummarySummary by CodeRabbit
Walkthrough
ChangesMapView stability
GeoJSON documentation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Unblocks: 6 PRs Merge Risk: ⚪ Minimal · up to The reviewed changes do not show a concrete merge-blocking risk. 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 11.76% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 4 files. (1 skipped: 1 unsupported.) Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This was referenced Sep 8, 2026
jkasprzyk17
added a commit
that referenced
this pull request
Sep 8, 2026
…tic clustering Native-side fixes for work the marker and overlay pipeline was doing on every render or every gesture, plus value equality for the camera props on the JS side. Builds on #58, which stabilizes the overlay arrays and callback envelopes. - Value-compare region, camera and mapPadding before they reach native, so an inline object literal no longer re-sends the prop (and, on the Google providers, no longer moves the camera) on every render. - Keep a render version per shape overlay on MapKit, Google iOS and Android: an unchanged polyline, polygon or circle is skipped and a changed one is updated in place instead of removed and re-added. MapKit replaces the overlay at its previous z-position only when the geometry changed. - Coalesce viewport refreshes to one pending request per compute queue and check a separate dataset generation before building the spatial index, so a long gesture cannot build a backlog of stale cluster work or keep discarding the index build for a dataset that has not changed. - Skip region fits that would not move the camera on Google iOS and Android. - Bound the marker image caches by decoded bytes (iOS NSCache limits, Android LruCache sizeOf). - iOS: accumulate cluster buckets in place through the dictionary subscript; the copy-out, append, write-back pattern copied the member array on every append, O(k^2) per cell. - iOS: drop the forced layoutIfNeeded() per pin configure inside MapKit's viewFor callback.
piotr-graczyk-dev
previously approved these changes
Sep 11, 2026
Nitro diffs view props by reference identity, but MapView rebuilt every overlay array, callback envelope and entering-animation descriptor on each render. A parent state change with unchanged map data marked up to 19 props dirty, paying a full JSI conversion of the descriptor arrays on the JS thread plus a native re-apply of every one of them. Compare descriptors structurally and hand back the previous value when nothing changed, memoize the callback(...) envelopes on the handler they wrap, and create the hybridRef envelope once per mount. An unchanged re-render now reaches native with zero dirty props. normalizeMarkerDescriptors no longer tries to preserve array identity; that job now belongs to the stabilizer, which does it correctly for every field. Descriptors are treated as immutable: mutating an object already handed to MapView is not picked up, because the comparison sees the same object on both sides.
Describe what MapView memoizes on the caller's behalf, what is left for the caller to hoist, and the immutability requirement the structural comparison implies.
React can discard a render, and a ref written during one would leave useStableValue remembering a value the native view never received. Move the write into a layout effect so it only records committed renders. The effect is a no-op on the renders this hook exists for: an unchanged value leaves `stable` - and with it the dependency - untouched, so nothing re-runs. Also drop the hooks barrel additions. Nothing imports that barrel, and MapView reaches the hooks by path like it already did for useCollectedOverlays.
jkasprzyk17
force-pushed
the
fix/overlay-reserialization-on-rerender
branch
from
September 11, 2026 11:53
01629a9 to
328a997
Compare
piotr-graczyk-dev
approved these changes
Sep 11, 2026
jkasprzyk17
added a commit
that referenced
this pull request
Sep 12, 2026
…tic clustering Native-side fixes for work the marker and overlay pipeline was doing on every render or every gesture, plus value equality for the camera props on the JS side. Builds on #58, which stabilizes the overlay arrays and callback envelopes. - Value-compare region, camera and mapPadding before they reach native, so an inline object literal no longer re-sends the prop (and, on the Google providers, no longer moves the camera) on every render. - Keep a render version per shape overlay on MapKit, Google iOS and Android: an unchanged polyline, polygon or circle is skipped and a changed one is updated in place instead of removed and re-added. MapKit replaces the overlay at its previous z-position only when the geometry changed. - Coalesce viewport refreshes to one pending request per compute queue and check a separate dataset generation before building the spatial index, so a long gesture cannot build a backlog of stale cluster work or keep discarding the index build for a dataset that has not changed. - Skip region fits that would not move the camera on Google iOS and Android. - Bound the marker image caches by decoded bytes (iOS NSCache limits, Android LruCache sizeOf). - iOS: accumulate cluster buckets in place through the dictionary subscript; the copy-out, append, write-back pattern copied the member array on every append, O(k^2) per cell. - iOS: drop the forced layoutIfNeeded() per pin configure inside MapKit's viewFor callback.
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
2 participants
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.
Problem
Nitro diffs view props by reference identity (
jsi::Value::strictEquals), butMapViewrebuilt every overlay array, everycallback(...)envelope and both entering-animation descriptors on each render. A re-render of the component holding theMapView— a timer, an unrelated state tick — therefore marked props dirty even when the map data was byte-identical.Measured by rendering
MapViewtwice with identical data and diffing the props exactly as Nitro does:<Marker />children + all handlers + entering animations<Marker />children, nothing elsemarkers, caller-memoized, one handler<MapView style={STYLE} />Each dirty prop cost a full
JSIConverterpass on the JS thread. For markers that is 13getPropertycalls per descriptor plus nested reads (~27 for a marker with an image, anchor, offset and animation) and astd::stringallocation per text field — then a C++→Swift array materialisation and a native re-apply.hybridRefbeing dirty also re-invoked the JS ref callback through the dispatcher on every render, even for aMapViewwith no props at all.Approach
The whole fix is controlling when the reference changes:
useStableValueholds the last returned value in a ref and hands it back when the new one is structurally equal. One primitive, N comparators — it covers the four overlay arrays and both entering-animation descriptors.descriptorEqualityprovides the comparators: field-by-field for markers, polylines, polygons and circles, plus coordinate lists and entering animations.useNitroCallbackmemoises the{ f }envelope on the handler it wraps instead of allocating one per render;hybridRef's envelope is created once per mount.normalizeMarkerDescriptorsdrops its own identity-preservation (and the partly brokendescriptorsEqualbehind it) — that job now belongs to the stabilizer, which does it correctly for every field.When something genuinely changes, the comparator returns
false, the reference changes, and the chain runs exactly as before. Nothing is skipped or deferred.Behaviour change
Descriptors and the objects inside them are now treated as immutable. Mutating a coordinate you already handed to
MapViewis not picked up, because the comparison sees the same object on both sides — with<Marker />children that previously happened to work. Documented in the README with an example, and pinned by a test so it stays a deliberate contract.Testing
descriptorEquality.test.ts. Every field of every descriptor has its own mutation case: a field a comparator misses is a map update that silently never reaches native, so the coverage is exhaustive by construction. Also pinned: the four near-identical list wrappers each reach their own item comparator, and the shared-nested-object contract above.normalizeMarkerDescriptors.test.tsreworked from identity assertions onto value assertions (require()resolution, resolution caching, animation normalisation) — the identity behaviour it pinned no longer exists.react-test-rendererthat real changes still propagate: moving a marker, renaming a title, adding/removing a marker, editing a polyline coordinate, swapping a handler, changing an entering animation, and switching between bulk props and children.bun run lint,bun run typecheck,bun run build, 129srctests and the plugin suite all pass.Not included
Shape overlays (
polylines/polygons/circles) are still torn down and rebuilt natively on every set —MapOverlayControllerhas no equality guard for them the way markers do. That is now unreachable fromMapViewbecause this change never sets an unchanged shape prop, but it is worth fixing on its own. It should be built on therenderSignature()helper that landed in #54 rather than a bespoke fold; I have a draft that predates that commit and needs rewriting against it.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.