feat: enhance map region change handling and improve camera update logic - #48
feat: enhance map region change handling and improve camera update logic#48jkasprzyk17 wants to merge 5 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR replaces programmatic camera-update tracking with user-gesture state across Android and iOS, adds approximate camera and region comparisons, and updates the example app and callback documentation for region-change events. ChangesRegion-change tracking and approximate equality
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant MapView
participant ProviderAdapter
participant ReactNative
User->>MapView: Begin region gesture
MapView->>ProviderAdapter: Region movement begins
ProviderAdapter->>ReactNative: onRegionChange(region)
User->>MapView: End region gesture
MapView->>ProviderAdapter: Region becomes idle
ProviderAdapter->>ReactNative: onRegionChangeComplete(region)
Suggested labels: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Comment |
|
React Doctor could not complete this scan.
Reviewed by React Doctor for commit |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
example/App.tsx (1)
834-839: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame handler for start and complete hides the new one-shot behavior.
updateRegionStatusis wired to bothonRegionChangeandonRegionChangeComplete, producing identical status text for both events. Since the entire point of this PR is that these events now fire distinctly once (start vs. end of a gesture), the demo app doesn't actually surface that distinction to a developer poking at the example.Consider labeling the two states differently (e.g. "Region · start" vs "Region · complete") so the example demonstrates the new contract.
♻️ Suggested tweak
- const updateRegionStatus = useCallback((region: Region) => { - setStatus( - `Region · ${region.latitude.toFixed(4)}, ${region.longitude.toFixed(4)}`, - ); - }, []); + const handleRegionChange = useCallback((region: Region) => { + setStatus( + `Region start · ${region.latitude.toFixed(4)}, ${region.longitude.toFixed(4)}`, + ); + }, []); + + const handleRegionChangeComplete = useCallback((region: Region) => { + setStatus( + `Region complete · ${region.latitude.toFixed(4)}, ${region.longitude.toFixed(4)}`, + ); + }, []);- onRegionChange={updateRegionStatus} - onRegionChangeComplete={updateRegionStatus} + onRegionChange={handleRegionChange} + onRegionChangeComplete={handleRegionChangeComplete}Also applies to: 857-858
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/App.tsx` around lines 834 - 839, The demo currently uses the same updateRegionStatus handler for both onRegionChange and onRegionChangeComplete, so it hides the new one-shot distinction between start and complete. Update the App.tsx region status logic so the callbacks set different labels (for example, separate “start” and “complete” text) while still using the existing updateRegionStatus and the onRegionChange/onRegionChangeComplete wiring to make the new behavior visible in the example.package/src/native/specs/MapView.nitro.ts (1)
169-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
onRegionChangeCompleteJSDoc wasn't updated to match the new contract.The PR states
onRegionChangeCompletenow fires once when the user gesture ends and no longer fires for programmatic updates, but the doc at Line 172 still reads "Called when a region change animation completes." That's ambiguous about the once-only, gesture-only semantics — and could mislead consumers into expecting a callback aftersetCamera/fitToCoordinatesanimations finish, which this PR explicitly removes.📝 Suggested doc update
- /** Called when a region change animation completes. */ + /** Called once when a user-initiated region change ends. */ onRegionChangeComplete?: (region: Region) => void;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package/src/native/specs/MapView.nitro.ts` around lines 169 - 173, Update the JSDoc for MapView.nitro.ts’s onRegionChangeComplete to match the new user-gesture-only contract. The current comment in the MapView spec still says it is called when a region change animation completes, which is misleading. Revise the documentation near onRegionChange and onRegionChangeComplete so it clearly states that onRegionChangeComplete fires once when the user gesture ends and does not fire for programmatic camera or fitToCoordinates updates.package/src/types/map.ts (1)
105-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame doc gap as
MapView.nitro.tsforonRegionChangeComplete.The doc at Line 108 ("Called when the map region change completes.") wasn't updated alongside
onRegionChange, leaving the breaking behavior — no callback for programmatic updates, single fire on gesture end — undocumented foronRegionChangeComplete.📝 Suggested doc update
- /** Called when the map region change completes. */ + /** Called once when a user-initiated region change ends. */ onRegionChangeComplete?: (region: Region) => void;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package/src/types/map.ts` around lines 105 - 109, Update the documentation for onRegionChangeComplete in the Map types to match the updated behavior described for onRegionChange and MapView.nitro.ts. The current comment only says it fires when the region change completes; revise it to note that it does not fire for programmatic updates and only fires once when a user gesture ends, keeping the wording consistent with the related map callback docs.package/ios/Region+MKCoordinateRegion.swift (1)
13-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLongitude wraparound strikes again.
Third file, same trick: comparing
center.longitudewith plainabs()breaks near ±180°. At this point it's less "an edge case" and more "a pattern you've now shipped in triplicate." Consider extracting a single shared angle/longitude-diff helper inMapApproximateEqualityso this gets fixed once instead of independently drifting in three files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package/ios/Region`+MKCoordinateRegion.swift around lines 13 - 22, The longitude comparison in approximatelyEquals is still using a plain absolute difference, which fails across the ±180° wraparound. Update the center.longitude check to use a shared longitude/angle-difference helper from MapApproximateEquality instead of abs(), and centralize that logic in MapApproximateEquality so the same wraparound-safe comparison can be reused consistently across the similar region/shape comparison methods.package/ios/Camera+GMSCameraPosition.swift (1)
17-28: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNo wraparound handling for bearing/longitude near boundaries.
Congratulations, you've built an equality check that fails exactly where the map lies to you the most: near ±180° longitude and the 0°/360° bearing seam.
179.999999vs-179.999999differ by ~360,abs()says "not equal," so a camera sitting essentially still at the antimeridian gets treated as moved. Same story for bearing crossing 0/360. Not fatal — worst case is a spurious camera update — but it's a predictable blind spot in a function whose entire job is "don't fire updates for essentially-the-same camera."🧭 Suggested wraparound-safe comparison
func approximatelyEquals( _ other: GMSCameraPosition, coordinateEpsilon: Double = MapApproximateEquality.coordinateEpsilon, zoomEpsilon: Float = MapApproximateEquality.zoomEpsilon, angleEpsilon: CLLocationDirection = MapApproximateEquality.angleEpsilon ) -> Bool { + func angleDiff(_ a: CLLocationDirection, _ b: CLLocationDirection) -> CLLocationDirection { + let diff = abs(a - b).truncatingRemainder(dividingBy: 360) + return min(diff, 360 - diff) + } abs(target.latitude - other.target.latitude) < coordinateEpsilon && abs(target.longitude - other.target.longitude) < coordinateEpsilon && abs(zoom - other.zoom) < zoomEpsilon - && abs(bearing - other.bearing) < angleEpsilon + && angleDiff(bearing, other.bearing) < angleEpsilon && abs(viewingAngle - other.viewingAngle) < angleEpsilon }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package/ios/Camera`+GMSCameraPosition.swift around lines 17 - 28, The approximatelyEquals method on GMSCameraPosition currently compares longitude and bearing with plain abs() checks, which fails across wraparound boundaries near ±180° and 0°/360°. Update this comparison to use wraparound-aware delta logic for target.longitude and bearing (while keeping latitude, zoom, and viewingAngle as-is) so equivalent camera positions across the seam are treated as equal. Keep the change localized to approximatelyEquals and reuse MapApproximateEquality’s tolerances.package/ios/Camera+MKMapCamera.swift (1)
29-40: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSame heading-wraparound gap as its GMS sibling.
abs(heading - other.heading) < angleEpsilonhas the identical seam problem at 0°/360° described forGMSCameraPosition.approximatelyEquals. Not repeating the fix here in full — same root cause, same low-stakes consequence (a redundant camera update, not a broken map). Worth fixing once and applying consistently rather than three separate patches drifting out of sync.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package/ios/Camera`+MKMapCamera.swift around lines 29 - 40, The `approximatelyEquals` method in `Camera+MKMapCamera.swift` has the same heading wraparound seam bug as the GMS camera comparison, because `abs(heading - other.heading)` fails near 0°/360°. Update the heading comparison to use a wraparound-aware angular difference in `MKMapCamera.approximatelyEquals`, keeping the same epsilon behavior for `coordinateEpsilon`, `distanceEpsilon`, and `pitch` while matching the fix used for the sibling camera equality helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@package/android/src/main/java/com/margelo/nitro/nitromaps/GoogleMapProviderAdapter.kt`:
- Around line 679-700: The region-begin emission in GoogleMapProviderAdapter’s
handleRegionWillChange is firing multiple times for the same gesture when
onCameraMoveStarted repeats before idle. Update handleRegionWillChange so it
only emits the start event when isUserGesture is not already set, or otherwise
make the repeated-start behavior intentional and explicit; keep
handleRegionDidChange and emitRegionChange behavior unchanged except for
preserving the single start/single complete pairing.
In `@package/ios/GoogleMapProviderAdapter.swift`:
- Around line 337-356: The GoogleMapProviderAdapter region-change flow is
emitting the begin event repeatedly for a single user gesture; update
handleRegionWillChange and emitRegionChange so onRegionChange fires only once
per gesture, matching the Apple adapter’s begin-state guard. Reuse the same
one-shot tracking logic already used in the Apple adapter, and keep
onRegionChangeComplete as the end/reset point so the begin state is cleared
after idle.
In `@package/ios/MKMapView`+userInteraction.swift:
- Around line 1-13: The `MKMapView.isUserInteracting` extension currently
assumes `subviews.first` contains the active gesture recognizers, which is
fragile against UIKit internal layout changes. Update `isUserInteracting` to
avoid relying on the first subview; instead, inspect the map view’s descendant
views for gesture recognizers or use explicit gesture-state tracking tied to
`MKMapView` interactions so real pans/pinches are detected consistently.
---
Nitpick comments:
In `@example/App.tsx`:
- Around line 834-839: The demo currently uses the same updateRegionStatus
handler for both onRegionChange and onRegionChangeComplete, so it hides the new
one-shot distinction between start and complete. Update the App.tsx region
status logic so the callbacks set different labels (for example, separate
“start” and “complete” text) while still using the existing updateRegionStatus
and the onRegionChange/onRegionChangeComplete wiring to make the new behavior
visible in the example.
In `@package/ios/Camera`+GMSCameraPosition.swift:
- Around line 17-28: The approximatelyEquals method on GMSCameraPosition
currently compares longitude and bearing with plain abs() checks, which fails
across wraparound boundaries near ±180° and 0°/360°. Update this comparison to
use wraparound-aware delta logic for target.longitude and bearing (while keeping
latitude, zoom, and viewingAngle as-is) so equivalent camera positions across
the seam are treated as equal. Keep the change localized to approximatelyEquals
and reuse MapApproximateEquality’s tolerances.
In `@package/ios/Camera`+MKMapCamera.swift:
- Around line 29-40: The `approximatelyEquals` method in
`Camera+MKMapCamera.swift` has the same heading wraparound seam bug as the GMS
camera comparison, because `abs(heading - other.heading)` fails near 0°/360°.
Update the heading comparison to use a wraparound-aware angular difference in
`MKMapCamera.approximatelyEquals`, keeping the same epsilon behavior for
`coordinateEpsilon`, `distanceEpsilon`, and `pitch` while matching the fix used
for the sibling camera equality helper.
In `@package/ios/Region`+MKCoordinateRegion.swift:
- Around line 13-22: The longitude comparison in approximatelyEquals is still
using a plain absolute difference, which fails across the ±180° wraparound.
Update the center.longitude check to use a shared longitude/angle-difference
helper from MapApproximateEquality instead of abs(), and centralize that logic
in MapApproximateEquality so the same wraparound-safe comparison can be reused
consistently across the similar region/shape comparison methods.
In `@package/src/native/specs/MapView.nitro.ts`:
- Around line 169-173: Update the JSDoc for MapView.nitro.ts’s
onRegionChangeComplete to match the new user-gesture-only contract. The current
comment in the MapView spec still says it is called when a region change
animation completes, which is misleading. Revise the documentation near
onRegionChange and onRegionChangeComplete so it clearly states that
onRegionChangeComplete fires once when the user gesture ends and does not fire
for programmatic camera or fitToCoordinates updates.
In `@package/src/types/map.ts`:
- Around line 105-109: Update the documentation for onRegionChangeComplete in
the Map types to match the updated behavior described for onRegionChange and
MapView.nitro.ts. The current comment only says it fires when the region change
completes; revise it to note that it does not fire for programmatic updates and
only fires once when a user gesture ends, keeping the wording consistent with
the related map callback docs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1e78261-5956-4f4f-bb53-55c9600dc7e4
📒 Files selected for processing (15)
example/App.tsxpackage/android/src/main/java/com/margelo/nitro/nitromaps/Camera+CameraPosition.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/GoogleMapProviderAdapter.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MapApproximateEquality.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.ktpackage/ios/AppleMapProviderAdapter.swiftpackage/ios/Camera+GMSCameraPosition.swiftpackage/ios/Camera+MKMapCamera.swiftpackage/ios/GoogleMapProviderAdapter.swiftpackage/ios/HybridMapViewDelegate.swiftpackage/ios/MKMapView+userInteraction.swiftpackage/ios/MapApproximateEquality.swiftpackage/ios/Region+MKCoordinateRegion.swiftpackage/src/native/specs/MapView.nitro.tspackage/src/types/map.ts
640b9ef
2 participants
Summary
Refactors how
onRegionChangeandonRegionChangeCompleteare emitted across Apple Maps and Google Maps (iOS + Android).onRegionChangenow fires once when a user-initiated region change beginsonRegionChangeCompletefires once when the user gesture endssetCamera,setRegion,fitToCoordinates, etc.) no longer emit region change callbacksisProgrammaticUpdateflag machinery (counters, pending IDs, fallback timers) in favor of gesture-based detectionBreaking change
If consumers relied on
onRegionChange/onRegionChangeCompletefiring during programmatic camera animations, that behavior is removed. These callbacks are now user-gesture-only.Implementation details
iOS (Apple Maps)
MKMapView.isUserInteractinghelper to detect active gesture recognizershandleRegionWillChange/handleRegionDidChangereplacenotifyRegionChange+ programmatic update trackingiOS (Google Maps)
mapView(_:willMove:)gesture flagAndroid (Google Maps)
OnCameraMoveStartedListenerwithREASON_GESTUREisProgrammaticUpdatewithisUserGestureShared
MapApproximateEqualityconstants +approximatelyEqualson camera/region types (iOS + Android)Test plan
onRegionChangefires once at start,onRegionChangeCompleteonce at endsetCamera/setRegionfrom JS — no region change callbacksfitToCoordinates/animateToRegion— no region change callbackscamera/regionprops — native map does not jitter or re-applyonRegionChangewiring)