fix(ios): remove main.sync from HybridMapView and async camera APIs#45
Conversation
|
React Doctor found 7 issues in 6 files · 2 errors & 5 warnings · score 55 / 100 (Critical) · full project Errors
5 warnings
Reviewed by React Doctor for commit |
|
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 (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughCamera methods now return promises across the public TypeScript surface, the JS wrapper, Android, and iOS. iOS also adds locked state backing, main-thread dispatch helpers, and lifecycle-aware promise execution. ChangesPromise-based camera API
Estimated code review effort: 4 (Complex) | ~50 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (5 passed)
Comment |
68720d0 to
24e11cf
Compare
Replace blocking DispatchQueue.main.sync hops in property accessors and imperative methods with main-thread-owned state and non-blocking dispatch. Camera commands now return Promise<void> to avoid JS/main deadlocks.
24e11cf to
ba8156c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/ios/HybridMapView.swift (1)
50-67: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep backing ivars on one thread
runOnMainmoves setter writes for_provider,_camera,_region, etc. onto the main queue, but the getters still read those same plainvars on the caller/JS thread. Nitro property access is synchronous on the current runtime thread, so this is a cross-thread read/write race on non-atomic storage and can return stale values. Either make both access paths use the same queue/actor or add proper synchronization.🤖 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/HybridMapView.swift` around lines 50 - 67, The HybridMapView backing storage is being written on the main queue via runOnMain while the property getters still read plain ivars like _provider synchronously on the caller thread, creating a cross-thread race. Update the provider access pattern so both get and set go through the same synchronization mechanism used by HybridMapView (for example, a shared main-thread/actor boundary or explicit locking), and apply the same approach to the other backing properties such as _camera and _region.
🧹 Nitpick comments (2)
package/ios/HybridMapView.swift (1)
763-790: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe helper is correct; the copy-paste tax on the setters is not.
promiseOnMainVoiditself is fine — weak-self reject, lifecycle snapshot, sync/async dispatch parity withpromiseOnMain, all good. But zoom out: you've stamped the identicalrunOnMain { [weak self] in guard let self else { return }; _x = newValue; adapter?.x = newValue }block roughly two dozen times (lines 69–535). That's a wall of near-duplicate code where a single typo in one setter (wrong ivar, wrong adapter key) would be nearly invisible in review. A small generic helper — e.g.setOnMain(_ store: ReferenceWritableKeyPath, _ adapterKey: ReferenceWritableKeyPath, _ value:)— would collapse each setter to one line and make divergence impossible. Optional given Swift's keypath ergonomics, but the current form is a maintenance liability.🤖 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/HybridMapView.swift` around lines 763 - 790, The repeated setter blocks in HybridMapView are duplicating the same main-thread state update and adapter sync logic, which makes copy-paste mistakes easy to miss. Refactor the setter methods that currently use the repeated runOnMain pattern into a shared helper (for example, a generic setOnMain-style method) so each setter only passes the stored property and matching adapter property/keypath. Keep promiseOnMainVoid as-is; the change should target the duplicated setter implementations and preserve the same weak-self and main-thread behavior.package/src/components/MapView.tsx (1)
170-199: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the repeated "not mounted" guard.
The
hybridRef.current == null ? reject : ...block is duplicated 5 times now (getCamera, setCamera, animateCamera, getVisibleRegion, fitToCoordinates). A small helper (e.g.withHybridRef(fn)) would remove the repetition and centralize the error message.♻️ Sketch of a shared helper
function withHybridRef<T>( hybridRef: React.RefObject<NativeMapViewHybrid | null>, fn: (hybrid: NativeMapViewHybrid) => Promise<T>, ): Promise<T> { if (hybridRef.current == null) { return Promise.reject(new Error('MapView is not mounted')); } return fn(hybridRef.current); }🤖 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/components/MapView.tsx` around lines 170 - 199, The mounted-check logic is duplicated across MapView methods and should be centralized. Extract the repeated `hybridRef.current == null` guard in the `MapView` imperative handle methods (`getCamera`, `setCamera`, `animateCamera`, `getVisibleRegion`, `fitToCoordinates`) into a shared helper such as `withHybridRef`, and have each method call it so the “MapView is not mounted” error message stays in one place.
🤖 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.
Outside diff comments:
In `@package/ios/HybridMapView.swift`:
- Around line 50-67: The HybridMapView backing storage is being written on the
main queue via runOnMain while the property getters still read plain ivars like
_provider synchronously on the caller thread, creating a cross-thread race.
Update the provider access pattern so both get and set go through the same
synchronization mechanism used by HybridMapView (for example, a shared
main-thread/actor boundary or explicit locking), and apply the same approach to
the other backing properties such as _camera and _region.
---
Nitpick comments:
In `@package/ios/HybridMapView.swift`:
- Around line 763-790: The repeated setter blocks in HybridMapView are
duplicating the same main-thread state update and adapter sync logic, which
makes copy-paste mistakes easy to miss. Refactor the setter methods that
currently use the repeated runOnMain pattern into a shared helper (for example,
a generic setOnMain-style method) so each setter only passes the stored property
and matching adapter property/keypath. Keep promiseOnMainVoid as-is; the change
should target the duplicated setter implementations and preserve the same
weak-self and main-thread behavior.
In `@package/src/components/MapView.tsx`:
- Around line 170-199: The mounted-check logic is duplicated across MapView
methods and should be centralized. Extract the repeated `hybridRef.current ==
null` guard in the `MapView` imperative handle methods (`getCamera`,
`setCamera`, `animateCamera`, `getVisibleRegion`, `fitToCoordinates`) into a
shared helper such as `withHybridRef`, and have each method call it so the
“MapView is not mounted” error message stays in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4dd470a-e579-42ae-b6ad-db6194ddfa04
📒 Files selected for processing (5)
package/android/src/main/java/com/margelo/nitro/nitromaps/HybridMapView.ktpackage/ios/HybridMapView.swiftpackage/src/components/MapView.tsxpackage/src/native/specs/MapView.nitro.tspackage/src/types/ref.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package/ios/MapViewState.swift (1)
3-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffHand-mirrored struct — one day it'll bite you.
You've cloned every single property of
MapProviderAdapterinto this struct by hand. Nothing links them at compile time, so the moment someone adds a property to the protocol (and they will) and forgets to mirror it here and inapply(to:), that field silently never reaches the adapter. No error, no warning, just a quietly broken map feature and a very confused engineer three weeks from now.Consider at least leaving a comment on both
MapProviderAdapterand this struct pointing at each other so the next person doesn't have to discover this the hard way.🤖 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/MapViewState.swift` around lines 3 - 37, The MapViewState struct is manually mirroring MapProviderAdapter properties, so updates can silently drift. Add clear cross-reference comments in MapViewState and MapProviderAdapter pointing to each other, and ensure apply(to:) stays in sync with the same symbols so future property additions are handled deliberately.
🤖 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.
Nitpick comments:
In `@package/ios/MapViewState.swift`:
- Around line 3-37: The MapViewState struct is manually mirroring
MapProviderAdapter properties, so updates can silently drift. Add clear
cross-reference comments in MapViewState and MapProviderAdapter pointing to each
other, and ensure apply(to:) stays in sync with the same symbols so future
property additions are handled deliberately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa9219b2-82d8-4397-9f4e-cefa509073d8
📒 Files selected for processing (3)
package/ios/HybridMapView.swiftpackage/ios/MapViewState.swiftpackage/src/components/MapView.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- package/src/components/MapView.tsx
- package/ios/HybridMapView.swift
…dRef and updating prop types
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
example/App.tsx (1)
670-683: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winYou made every camera method a rejectable promise and then blindly ignored the rejections. Bold.
The whole point of this PR is that
animateCamera/fitToCoordinates/getCameranow returnPromise<void>that rejects when the view isn't mounted. YourmapRef.current?.only guards anullcurrent — it does nothing for a promise that resolves the?.chain and then rejects because the native view was recycled. Result: unhandled promise rejection warnings (or a redbox) in the exact "documented usage pattern" you're shipping in the README.Same rot lives in
handleMapReadyat Line 788 (fitToCoordinates) andhandleGetCameraat Line 685 (awaitwith notry/catch). Attach a.catch(or wrap theawait) at each fire-and-forget site.🧯 Proposed fix for the fire-and-forget call sites
const handleAnimateCamera = useCallback(() => { - mapRef.current?.animateCamera( - { - center: { - latitude: scenario.region.latitude, - longitude: scenario.region.longitude, - }, - zoom: 13, - heading: 0, - pitch: 0, - }, - 1, - ); + mapRef.current + ?.animateCamera( + { + center: { + latitude: scenario.region.latitude, + longitude: scenario.region.longitude, + }, + zoom: 13, + heading: 0, + pitch: 0, + }, + 1, + ) + .catch(() => { + // view unmounted before the camera update landed + }); }, [scenario]);Apply the equivalent
.catch(...)to thefitToCoordinatescall inhandleMapReady(Line 788) and atry/catcharound theawaitinhandleGetCamera(Line 685).🤖 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 670 - 683, The fire-and-forget camera calls in handleAnimateCamera, handleMapReady, and handleGetCamera are not handling rejected Promise results from animateCamera, fitToCoordinates, and getCamera. Update each call site to catch rejections explicitly: attach a .catch handler for the non-awaited calls in handleAnimateCamera and handleMapReady, and wrap the awaited getCamera call in handleGetCamera with try/catch so rejected promises from an unmounted view do not surface as unhandled rejections.
🤖 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.
Outside diff comments:
In `@example/App.tsx`:
- Around line 670-683: The fire-and-forget camera calls in handleAnimateCamera,
handleMapReady, and handleGetCamera are not handling rejected Promise results
from animateCamera, fitToCoordinates, and getCamera. Update each call site to
catch rejections explicitly: attach a .catch handler for the non-awaited calls
in handleAnimateCamera and handleMapReady, and wrap the awaited getCamera call
in handleGetCamera with try/catch so rejected promises from an unmounted view do
not surface as unhandled rejections.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d04b4a41-5141-47bb-80f9-89fd85383cc2
📒 Files selected for processing (2)
example/App.tsxpackage/src/components/MapView.tsx
2 participants
Summary
DispatchQueue.main.syncfromHybridMapView— property getters read backing fields directly; setters and lifecycle hooks use non-blockingrunOnMain(asyncdispatch).applyCamera,animateCamera, andfitToCoordinatestoPromise<void>in the Nitro spec and native implementations, matching the existingpromiseOnMainpattern used byfetchCamera/getVisibleRegion.MapViewRefand the React wrapper to propagate promises and reject when the view is not mounted.This addresses deadlock risk when imperative camera calls from the JS thread block on main while main is waiting on JS/Fabric, and removes unnecessary per-getter main round-trips.
Follow-up to #43 — that PR did not remove the
onMain/.syncpattern.Test plan
bun run typecheckinpackage/bun run nitrogeninpackage/(codegen required after spec change)animateCamera,setCamera,fitToCoordinatesfrom ref work without UI freezePromise.resolvedon UI thread)