Fix threading issues on ios#43
Conversation
|
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
WalkthroughHybridMapView.swift now tracks mount state with a generation-checked lifecycle, routes adapter access through main-thread helpers, and rejects stale async camera work after recycle. Property, callback, camera, and teardown paths now use backing fields plus explicit main-thread propagation. ChangesMain-thread lifecycle and adapter routing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JS as JS caller
participant View as HybridMapView
participant Main as Main thread
participant Adapter as Map adapter
JS->>View: fetchCamera()
View->>Main: promiseOnMain(snapshot)
Main->>Main: validateActiveLifecycle(snapshot)
alt lifecycle mismatch
Main-->>JS: reject "MapView is not mounted"
else lifecycle matches
Main->>Adapter: currentAdapter(matching: snapshot)
Adapter-->>Main: camera data
Main-->>JS: resolve camera data
end
🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ios/HybridMapView.swift`:
- Around line 400-428: The synchronous camera methods in HybridMapView are
bypassing the recycle safety check and can recreate a new adapter after
prepareForRecycle(). Update applyCamera, animateCamera, and fitToCoordinates to
enforce the same recycle-generation guard used by fetchCamera and
getVisibleRegion, either by making currentAdapter() reject access once recycled
or by checking the generation before calling it so stale calls fail instead of
reattaching the map.
🪄 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: e45387cd-d86c-46cd-bd02-1400a5075cc8
📒 Files selected for processing (1)
package/ios/HybridMapView.swift
|
Great work on this PR 👏 The main-thread confinement and lifecycle guard look solid, and the fix for the CodeRabbit feedback in the third commit makes sense. One small follow-up before merge: in adapter.onRegionChange = onRegionChange
adapter.markers = markersadd unnecessary adapter.onRegionChange = _onRegionChange
adapter.markers = _markers
// ... same for the other callback/overlay fieldsOnce that’s in, I’m happy to approve. Thanks! |
oh indeed - totally missed that! fixed! |
|
Looks good with the Happy to approve. |
Summary
HybridMapViewstate access and provider adapter mutations onto the main thread.fetchCamera,getVisibleRegion) asynchronously on main to avoid blocking worklet/background callers.Why
Nitro hybrid objects can be accessed from worklet/background runtimes, but UIKit/MapKit access must happen on the main thread. This prevents off-main map mutations and cached-state races.
Observed warning before this fix: