Repository navigation
fix(compat): keep marker and cluster keys stable across zoom and data updates - #59
Conversation
Single markers were rendered as cloneElement(child, { key:
`marker-${index}` }), discarding the key the app gave the Marker.
Inserting or removing a marker earlier in the list shifted every later
key, so React handed component instances (callouts, tracksViewChanges
snapshots, internal state) to other points and remounted markers that
did not change, churning native annotations. react-native-map-clustering
keeps the user's key.
Render the child as Children.toArray returned it: it keeps the user's key
(`.$key`), and an unkeyed static child keeps its position key. React
already warns about markers mapped from an array without a key, so there
is no separate warning.
Refs #28
Cluster bubbles were keyed by the engine's cluster id. A cluster gets a new id at every zoom level where it gains or loses points, and every rebuild renumbers all of them, so each split, merge or data update unmounted the bubble and mounted a new one: the native marker was removed and re-added, which blinks, most visibly with Google Maps. The engine now reports minLeafId on every node: the smallest point id among its leaves, the node's own id for a point. A cluster keeps it across zoom levels while it only gains or loses other leaves. Supercluster resolves it to the loaded feature (getClusterMinLeaf, internal), so it follows the #54 filtering and replaceLoadedFeatures swaps, and MapView keys each bubble, default or renderCluster, by that marker's own key. A cluster that splits on zoom, or survives a rebuild that inserts a marker, keeps one native marker that moves and relabels. EngineClusterNode gains a required minLeafId field, so code that builds these objects (mocks) needs it. Refs #5 Refs #28
Now that a bubble keeps its native marker while its cluster splits or merges, its count changes on a mounted marker. With tracksViewChanges off (the default), Google Maps keeps showing the bitmap it first took, so the bubble would show a stale count; the same already happened to a bubble whose colour changed through selectedClusterColor. When the count, colours or font of a mounted bubble change, call the marker's redraw(), react-native-maps' documented way to refresh a marker without tracking every view change. A moved bubble is a plain coordinate update and needs no redraw. Apple Maps updates the view live, and redraw is harmless there. Refs #5
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (15)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe cluster engine now carries the smallest leaf point ID through cluster nodes. MapView uses stable marker keys for unclustered markers and cluster bubbles. ClusterMarker redraws when its rendered appearance changes while native view tracking is disabled. ChangesStable marker identity and rendering
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains; the change is ready to merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change is narrowly scoped to marker identity and appearance updates. No new privilege or data-access path was identified, but compatibility between updated JavaScript and existing native app versions has not been confirmed. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation For Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 12 files. (3 skipped: 3 unsupported.)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
React Doctor found 9 issues in 3 files · 2 errors & 7 warnings · score 55 / 100 (Critical) · full project Errors
7 warnings
Reviewed by React Doctor for commit |
# Conflicts: # package/src/__tests__/mapview/MapView.stability.test.tsx
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Summary
Three commits.
marker-${index}) #28). Single markers were rendered ascloneElement(child, { key: marker-${index} }), which discarded the key the app gave theMarker. Inserting or removing a marker earlier in the list shifted every later key. React then handed component instances (callouts,tracksViewChangessnapshots, state) to other points, and remounted markers that hadn't changed. Markers now keep the keyChildren.toArraygives them: the user's key, or the position for unkeyed static children.minLeafIdon every node: the smallest point id among its leaves, or the node's own id for a point. A cluster keeps it across zoom levels while it only gains or loses other leaves.Supercluster.getClusterMinLeaf(@internal) resolves it to the loaded feature. It follows the fix(engine): prevent silent cluster-tree corruption from a non-finite coordinate #54 filtering andreplaceLoadedFeaturesswaps.MapViewkeys each bubble, default orrenderCluster, by that marker's own key. The bubble stays one native marker that moves and relabels.tracksViewChangesoff (the default), Google Maps keeps showing the first bitmap, soClusterMarkercalls the marker's documentedredraw()when its count, colours or font change. A moved bubble is a plain coordinate update and doesn't redraw. This also fixes a bubble that never showed itsselectedClusterColoron Google.Fixes #28
Refs #5: a candidate fix. Only a Google Maps run can confirm it (see Notes).
Test plan
bun run lintandbun run format:checkcd package && bun run typecheck && bun run test:ci: 135 passcd package && bun run build && bun run verify:pack && bun run verify:exportsbun run docs:buildcd package/cpp && c++ -std=c++20 -I. ClusterEngineCore.test.cpp -o cluster_test && ./cluster_test, with a new test thatminLeafIdequals the smallest leaf for every node at every zoombun run specsre-run. The library pod compiles for the iOS simulator (xcodebuild -scheme react-native-better-clustering, as innative.yml).marker-${index}) #28 pin is now a regular test)Supercluster.getClusterMinLeafunit tests: index alignment after a skipped non-finite point,replaceLoadedFeatures, and a cluster from another instanceClusterMarkertests:redraw()fires on a count or colour change, not on a move, and not whiletracksViewChangesis onexpo-modules-jsi(Expo SDK 56 Swift), unrelated to this PR. Android only via CI.Scope
Risk
EngineClusterNodegains a requiredminLeafId, so code that builds these objects (mocks) needs it. Treat this as a minor release.redraw()is called only for the default bubbles. Bubbles fromrenderClusterkeep a persisting key too, so the app owns refreshing them if it turnstracksViewChangesoff.Checklist
cd package && bun run specswas re-run (package/nitrogen/stays git-ignored).README.md,docs/docs/troubleshooting.md("Markers flicker on zoom") anddocs/docs/api/types.md(minLeafId) updated.Notes
provider="google",imagemarkers, 86 points, zooming in and out. Compare the default withclusterUpdateIntervalMs={0}: the audit suspects mid-gesture re-clustering also contributes, and this PR does not change that default.selectedClusterIdstill compares per-buildcluster_ids (mentioned in Marker keys are replaced by index keys (marker-${index}) #28). Spider marker keys are untouched here because Bound spiderfy: cap the number of leaves and space the spiral in screen pixels #16 rewrites that file.minLeafIdthrough superclustermap/reduce, mirroring the C++ minimum. JS tests prove the key logic. The C++ test and the iOS compile cover the native field.getClusterExpansionZoomreturns the wrong zoom for clusters that survive into lower zoom levels #17 → Spiderfy never triggers for co-located markers #22) inMapView.tsxare expected and simple; whichever lands second rebases.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.