feat: add custom marker images with anchor, rotation, and flat mode - #25
Merged
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:
📝 WalkthroughWalkthroughAdds full custom bitmap marker image support ( ChangesCustom Marker Images
Sequence Diagram(s)sequenceDiagram
participant App as App (JS)
participant resolveMarkerImage
participant LruCache
participant MapOverlayController as MapOverlayController (native)
participant MarkerIconFactory as MarkerIconFactory (Android)
participant NitroImageAnnotationView as NitroImageAnnotationView (iOS)
participant MarkerImageLoader as MarkerImageLoader (iOS)
App->>resolveMarkerImage: MarkerImageSource (require/URI/MarkerImage)
resolveMarkerImage->>LruCache: lookup by cache key
LruCache-->>resolveMarkerImage: hit or miss
resolveMarkerImage-->>App: MarkerImage | undefined
App->>MapOverlayController: setMarkers(descriptors with image)
MapOverlayController->>MapOverlayController: markersFingerprint() — skip if unchanged
rect rgba(100, 149, 237, 0.5)
note over MapOverlayController, MarkerIconFactory: Android path
MapOverlayController->>MarkerIconFactory: applyVisualProps(descriptor, marker, key)
MarkerIconFactory->>MarkerIconFactory: decode or fetch bitmap (async HTTP)
MarkerIconFactory-->>MapOverlayController: BitmapDescriptor applied to Marker
end
rect rgba(60, 179, 113, 0.5)
note over MapOverlayController, MarkerImageLoader: iOS path
MapOverlayController->>NitroImageAnnotationView: configure(for: marker)
NitroImageAnnotationView->>MarkerImageLoader: load(image, completion)
MarkerImageLoader-->>NitroImageAnnotationView: UIImage (cached or fetched)
NitroImageAnnotationView->>NitroImageAnnotationView: applyLayout (centerOffset + rotation)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/MapOverlayController.kt`:
- Around line 297-312: The marker sync path is exposing internal `s:`-prefixed
keys to consumer callbacks. Update the marker tagging and callback flow in
`MapOverlayController` so the sync key stays internal while click/drag listeners
receive the raw descriptor ID, or strip the prefix consistently before
forwarding `marker.tag` in every non-cluster callback path. Use the `update` and
marker callback code around `marker.tag`, `applyVisualProps`, and the
non-cluster listeners to keep the external API free of internal sync prefixes.
In
`@package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerDescriptor`+MarkerOptions.kt:
- Around line 13-25: `effectiveGoogleMapsAnchor` is mixing dp and px when
applying `centerOffset`, which can shift markers incorrectly on high-density
screens. Update this helper (or its caller) so `centerOffset` is converted to px
using density before subtracting it from `anchorX`/`anchorY`, and keep the unit
space consistent in `MarkerDescriptor.effectiveGoogleMapsAnchor` and
`MarkerOptions`.
In
`@package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt`:
- Around line 77-96: In MarkerIconFactory, the async icon-loading path leaves
pendingIconKeys stuck when loadIconAsync returns null, and repeated same-key
updates can keep the first onIconApplied callback with stale descriptor data.
Update the icon application flow around loadIconAsync, setPending, setApplied,
and isIconKeyCurrent so pending state is always cleared or replaced on
completion/failure, and ensure newer descriptor/anchor data is used when the
bitmap resolves instead of reusing the original closure.
- Around line 173-180: `loadRemoteIcon()`, the asset/file/resource decode paths,
and `resizeBitmap()` currently decode full-size bitmaps before downscaling,
which can spike memory on large images. Update `MarkerIconFactory` to use
bounds-first decoding (`inJustDecodeBounds`) and sampled decoding
(`inSampleSize`) for every source before creating the final bitmap, and add a
max pixel budget guard so oversized images are rejected or decoded at a smaller
sample. Keep the changes localized around `loadRemoteIcon`, the other decoder
helpers, and `resizeBitmap` so all image-loading paths share the same safe
decode strategy.
In `@package/ios/MapMarkerAnnotation.swift`:
- Around line 37-72: `update(from:)` in `MapMarkerAnnotation` is missing
interaction-state updates, so retained marker views can keep stale callout/drag
behavior. Update the method to assign `draggable` from `descriptor.draggable`
and include it in the change detection. Also make title/subtitle nil ↔ non-nil
transitions count as changes by comparing the existing and new values before
reassigning `title` and `subtitle`, so `MapOverlayController` will refresh the
view when these props change.
In `@package/ios/NitroImageAnnotationView.swift`:
- Around line 35-55: The load path in NitroImageAnnotationView.configure(for:)
leaves loadToken set even when MarkerImageLoader.load returns nil, so a failed
fetch is treated like a cache hit on the next call. Update the completion
handling to clear or invalidate loadToken when uiImage is nil, and only keep the
token on successful image loads so the same imageDescriptor will retry instead
of short-circuiting. Keep the fix localized around the loadToken fast path and
the MarkerImageLoader.load callback.
In `@package/src/overlays/markerImageFromResolvedAsset.ts`:
- Around line 13-14: The marker image validation is inconsistent:
markerImageFromResolvedAsset() treats an empty uri as missing, but
isMarkerImage() still accepts { uri: '' }, allowing invalid images to bypass
normalization. Update isMarkerImage() to reject empty uri values using the same
rule as markerImageFromResolvedAsset(), and keep the check aligned with the
marker image handling path so empty strings fall back to the default marker
behavior.
In `@package/src/types/overlays.ts`:
- Around line 33-34: MarkerProps.image is advertising a broader React Native
image union than the marker pipeline can actually preserve. Update the type in
overlays.ts so the MarkerProps.image/MarkerImage contract only accepts the
serializable subset that the resolver forwards (uri, width, height, scale), or
explicitly extend the native serialization path to support the full
ImageSourcePropType shape. Keep the fix aligned with the existing MarkerImage
and resolver code so the type matches runtime behavior.
🪄 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: 441c34bf-f4e5-45ff-8b7f-f0f42176940b
⛔ Files ignored due to path filters (19)
bun.lockis excluded by!**/*.lockexample/assets/markerblue.pngis excluded by!**/*.pngexample/assets/markerblue@2x.pngis excluded by!**/*.pngexample/assets/markergold.pngis excluded by!**/*.pngexample/assets/markergold@2x.pngis excluded by!**/*.pngexample/assets/markergreen.pngis excluded by!**/*.pngexample/assets/markergreen@2x.pngis excluded by!**/*.pngexample/assets/markerorange.pngis excluded by!**/*.pngexample/assets/markerorange@2x.pngis excluded by!**/*.pngexample/assets/markerred.pngis excluded by!**/*.pngexample/assets/markerred@2x.pngis excluded by!**/*.pngexample/assets/markerviolet.pngis excluded by!**/*.pngexample/assets/markerviolet@2x.pngis excluded by!**/*.pngexample/assets/poiNavigation.pngis excluded by!**/*.pngexample/assets/poiNavigation@2x.pngis excluded by!**/*.pngexample/assets/poiRestaurant.pngis excluded by!**/*.pngexample/assets/poiRestaurant@2x.pngis excluded by!**/*.pngexample/assets/poiStar.pngis excluded by!**/*.pngexample/assets/poiStar@2x.pngis excluded by!**/*.png
📒 Files selected for processing (38)
README.mdexample/App.tsxexample/examples/customMarkerImages.tsexample/examples/index.tsexample/index.jsexample/package.jsonpackage/android/src/main/java/com/margelo/nitro/nitromaps/GoogleMapProviderAdapter.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerDescriptor+Fingerprint.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerDescriptor+MarkerOptions.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerViewportFilter.ktpackage/ios/AppleMapProviderAdapter.swiftpackage/ios/HybridMapViewDelegate.swiftpackage/ios/MapMarkerAnnotation.swiftpackage/ios/MapOverlayController.swiftpackage/ios/MarkerDescriptor+Fingerprint.swiftpackage/ios/MarkerImageLoader.swiftpackage/ios/MarkerViewportFilter.swiftpackage/ios/NitroImageAnnotationView.swiftpackage/ios/NitroPinAnnotationView.swiftpackage/src/components/MapView.tsxpackage/src/hooks/useCollectedOverlays.tspackage/src/index.tspackage/src/native/specs/overlays.tspackage/src/overlays/__tests__/lruCache.test.tspackage/src/overlays/__tests__/markerImageCacheKey.test.tspackage/src/overlays/__tests__/normalizeMarkerDescriptors.test.tspackage/src/overlays/__tests__/resolveMarkerImage.test.tspackage/src/overlays/assetSourceResolver.tspackage/src/overlays/lruCache.tspackage/src/overlays/markerImageCacheKey.tspackage/src/overlays/markerImageFromResolvedAsset.tspackage/src/overlays/normalizeMarkerDescriptors.tspackage/src/overlays/resolveMarkerImage.tspackage/src/types/index.tspackage/src/types/overlays.tspackage/tsconfig.json
💤 Files with no reviewable changes (2)
- package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerViewportFilter.kt
- package/ios/MarkerViewportFilter.swift
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
package/src/overlays/resolveMarkerImage.ts (1)
23-30: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStop caching the caller’s object.
Line 29/Line 30 store and return
sourceverbatim. That makes the cache mutable from the outside, so one latersource.uri = ...leaves a stale entry under the old key and can feed the wrong image back on later lookups. It also preserves any extra runtime fields instead of normalizing to the Nitro-safe shape this PR is trying to guarantee. Clone theMarkerImagebefore caching/returning it.Suggested fix
if (isMarkerImage(source)) { const key = markerImageCacheKey(source); const cached = resolvedImageCache.get(key); if (cached != null) { return cached; } - resolvedImageCache.set(key, source); - return source; + const normalized = markerImageFromResolvedAsset(source)!; + resolvedImageCache.set(key, normalized); + return normalized; }🤖 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/overlays/resolveMarkerImage.ts` around lines 23 - 30, The cached MarkerImage path in resolveMarkerImage is storing and returning the caller’s source object directly, which leaves the cache mutable and preserves non-normalized runtime fields. Update the isMarkerImage branch to clone the MarkerImage before putting it into resolvedImageCache and before returning it, so the cached value is isolated from later caller mutations and matches the Nitro-safe shape; keep the existing markerImageCacheKey and resolvedImageCache logic, just replace the verbatim source reuse.package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt (2)
123-153: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftDon’t decode local marker images on the UI thread.
applyVisualProps()callsapplyAnchor(), which can hitdisplaySizePx()and decode local files/resources/assets synchronously. Then uncached local icons are decoded again throughicon(image)on the main thread at Line 174. In bulk marker mode this can visibly block map interactions.Move local decode work onto the executor and avoid decoding only to compute size; let
cacheBitmap()populatesizeCache, then re-apply the anchor through the existing callback.Also applies to: 174-174
🤖 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/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt` around lines 123 - 153, Local marker images are being decoded synchronously on the UI thread in MarkerIconFactory through displaySizePx(), applyAnchor(), and icon(), which can block interactions in bulk marker mode. Move the local bitmap loading/decoding work onto the existing executor path used by loadLocalBitmap() and cacheBitmap(), and avoid decoding just to determine size; rely on cacheBitmap() to fill sizeCache instead. Then re-apply the anchor via the existing callback after the bitmap is cached so applyVisualProps() no longer needs a synchronous decode.
73-86: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCancel pending loads before returning for an already-applied icon.
Right now a stale in-flight load can still win. Example: default icon is applied, image
Astarts loading, then props switch back to default. Line 74 returns because default is already applied, but the pendingAload is not invalidated, so it can later set the wrong custom icon.Proposed fix
if (image == null) { if (isIconApplied(marker, DEFAULT_ICON_KEY)) { + invalidatePendingLoad(marker) return } marker.setIcon(BitmapDescriptorFactory.defaultMarker()) setApplied(marker, DEFAULT_ICON_KEY) @@ val iconKey = cacheKey(image) if (isIconApplied(marker, iconKey)) { + invalidatePendingLoad(marker) return }Also applies to: 185-188
🤖 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/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt` around lines 73 - 86, In MarkerIconFactory’s icon-application flow, an already-applied icon can return early while leaving a stale in-flight load active, which may later overwrite the marker with the wrong image. Before any early return in the image-null/default path and the cached-icon path, cancel or invalidate any pending load associated with that marker/icon state. Update the logic around setApplied, isIconApplied, and the load callback so the currently requested icon remains the only one allowed to apply.
♻️ Duplicate comments (1)
package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt (1)
214-216: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound remote image bytes before decoding.
Bounds-first bitmap decode helps, but
stream.readBytes()still allocates the full response first. A huge remote payload can exhaust memory beforedecodeByteArray()enforcesMAX_DECODE_PIXELS.Suggested direction
- val bitmap = decodeByteArray(stream.readBytes(), image) ?: return null + val bytes = stream.readBoundedBytes(MAX_REMOTE_IMAGE_BYTES) ?: return null + val bitmap = decodeByteArray(bytes, image) ?: return null cacheBitmap(key, resizeBitmap(bitmap, image))Add a small bounded read helper and a max byte constant sized for expected marker assets.
🤖 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/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt` around lines 214 - 216, The remote image loading path in MarkerIconFactory.decodeBitmap still reads the entire response into memory before decoding, which can bypass the intended bounds check. Update the connection.getInputStream() flow to use a bounded read helper with a max byte limit for marker assets, then pass the bounded bytes into decodeByteArray so oversized payloads are rejected before full allocation. Keep the fix localized to the bitmap decode/cache path and reuse the existing MAX_DECODE_PIXELS logic for decode safety.
🤖 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/MapMarkerAnnotation.swift`:
- Line 52: The `MapMarkerAnnotation` update path is changing `isClusterable`
without reflecting that change in the refresh result, so the existing marker
view can miss clusterability flips. Update the refresh signal logic in
`MapMarkerAnnotation` (including the `isClusterable = descriptor.clusterable ??
true` assignment and the related return path in the same update method) so a
change to `isClusterable` also causes the returned `Bool` to report a refresh is
needed. Apply the same adjustment anywhere else in the `MapMarkerAnnotation`
refresh/update flow that mirrors this tracking.
---
Outside diff comments:
In
`@package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt`:
- Around line 123-153: Local marker images are being decoded synchronously on
the UI thread in MarkerIconFactory through displaySizePx(), applyAnchor(), and
icon(), which can block interactions in bulk marker mode. Move the local bitmap
loading/decoding work onto the existing executor path used by loadLocalBitmap()
and cacheBitmap(), and avoid decoding just to determine size; rely on
cacheBitmap() to fill sizeCache instead. Then re-apply the anchor via the
existing callback after the bitmap is cached so applyVisualProps() no longer
needs a synchronous decode.
- Around line 73-86: In MarkerIconFactory’s icon-application flow, an
already-applied icon can return early while leaving a stale in-flight load
active, which may later overwrite the marker with the wrong image. Before any
early return in the image-null/default path and the cached-icon path, cancel or
invalidate any pending load associated with that marker/icon state. Update the
logic around setApplied, isIconApplied, and the load callback so the currently
requested icon remains the only one allowed to apply.
In `@package/src/overlays/resolveMarkerImage.ts`:
- Around line 23-30: The cached MarkerImage path in resolveMarkerImage is
storing and returning the caller’s source object directly, which leaves the
cache mutable and preserves non-normalized runtime fields. Update the
isMarkerImage branch to clone the MarkerImage before putting it into
resolvedImageCache and before returning it, so the cached value is isolated from
later caller mutations and matches the Nitro-safe shape; keep the existing
markerImageCacheKey and resolvedImageCache logic, just replace the verbatim
source reuse.
---
Duplicate comments:
In
`@package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.kt`:
- Around line 214-216: The remote image loading path in
MarkerIconFactory.decodeBitmap still reads the entire response into memory
before decoding, which can bypass the intended bounds check. Update the
connection.getInputStream() flow to use a bounded read helper with a max byte
limit for marker assets, then pass the bounded bytes into decodeByteArray so
oversized payloads are rejected before full allocation. Keep the fix localized
to the bitmap decode/cache path and reuse the existing MAX_DECODE_PIXELS logic
for decode safety.
🪄 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: c00fcded-b0ab-4fd9-a151-f8f69cd43891
📒 Files selected for processing (15)
example/examples/customMarkerImages.tspackage/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerDescriptor+MarkerOptions.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.ktpackage/ios/MapMarkerAnnotation.swiftpackage/ios/MarkerImageLoader.swiftpackage/ios/NitroImageAnnotationView.swiftpackage/src/index.tspackage/src/native/specs/overlays.tspackage/src/overlays/__tests__/resolveMarkerImage.test.tspackage/src/overlays/assetSourceResolver.tspackage/src/overlays/markerImageFromResolvedAsset.tspackage/src/overlays/resolveMarkerImage.tspackage/src/types/index.tspackage/src/types/overlays.ts
💤 Files with no reviewable changes (1)
- package/ios/MarkerImageLoader.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- package/src/overlays/assetSourceResolver.ts
- package/src/types/index.ts
- package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerDescriptor+MarkerOptions.kt
- package/src/native/specs/overlays.ts
- package/src/index.ts
- package/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt
- package/ios/NitroImageAnnotationView.swift
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 (2)
README.md (1)
318-318: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove custom bitmap images from Planned features.
This is already documented as supported above, so leaving it in the roadmap just makes the README contradict itself. Move it to a future wishlist or delete it.
🤖 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 `@README.md` at line 318, The Planned features list in the README still includes custom bitmap images even though that capability is already documented as supported earlier, so update the roadmap entry to remove it or move it out of Planned features into a future wishlist; adjust the affected README bullet that mentions markers with callouts, drag support, and custom bitmap images so the supported feature list is no longer contradictory.package/ios/MapOverlayController.swift (1)
182-185: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon't blow away the marker state just to swap view classes.
Removing and re-adding the annotation here kills the current selection/callout and replays the enter animation through
HybridMapViewDelegate.mapView(_:didAdd:). That breaks the promisedtitle/subtitlecompatibility whenever a retained marker switches between pin and image mode. Preserve selection/callout state across this path, or you’ll get a lovely little UI regression every time the image presence flips.🤖 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/MapOverlayController.swift` around lines 182 - 185, The swap in MapOverlayController’s annotation update path currently removes and re-adds the marker, which resets selection/callout state and retriggers HybridMapViewDelegate.mapView(_:didAdd:). Update the needsImageView/hasImageView branch to preserve the existing annotation instance and its selection/callout while switching between pin and image presentation, so retained markers keep their title/subtitle behavior intact.
🧹 Nitpick comments (1)
package/src/overlays/normalizeMarkerDescriptors.ts (1)
6-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStop duplicating marker normalization.
This reimplements
normalizeMarkerDescriptorfrompackage/src/utils/enteringAnimation.tsalmost field-for-field. Keeping two JS→native mappers for the same marker contract is how one path inevitably misses the next prop addition and bulk markers start behaving differently from collected markers. Reuse the shared helper here instead.🤖 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/overlays/normalizeMarkerDescriptors.ts` around lines 6 - 23, The marker normalization logic in normalizeDescriptor is duplicating the shared JS-to-native mapping already implemented by normalizeMarkerDescriptor, which risks drift when marker props change. Update normalizeMarkerDescriptors.ts to reuse normalizeMarkerDescriptor from package/src/utils/enteringAnimation.ts instead of manually rebuilding the object, while preserving the existing image resolution behavior if needed by applying resolveMarkerImage through the shared path. Verify the overlay path and any bulk marker handling still produce the same MarkerDescriptor shape.
🤖 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/MapOverlayController.swift`:
- Around line 182-185: The swap in MapOverlayController’s annotation update path
currently removes and re-adds the marker, which resets selection/callout state
and retriggers HybridMapViewDelegate.mapView(_:didAdd:). Update the
needsImageView/hasImageView branch to preserve the existing annotation instance
and its selection/callout while switching between pin and image presentation, so
retained markers keep their title/subtitle behavior intact.
In `@README.md`:
- Line 318: The Planned features list in the README still includes custom bitmap
images even though that capability is already documented as supported earlier,
so update the roadmap entry to remove it or move it out of Planned features into
a future wishlist; adjust the affected README bullet that mentions markers with
callouts, drag support, and custom bitmap images so the supported feature list
is no longer contradictory.
---
Nitpick comments:
In `@package/src/overlays/normalizeMarkerDescriptors.ts`:
- Around line 6-23: The marker normalization logic in normalizeDescriptor is
duplicating the shared JS-to-native mapping already implemented by
normalizeMarkerDescriptor, which risks drift when marker props change. Update
normalizeMarkerDescriptors.ts to reuse normalizeMarkerDescriptor from
package/src/utils/enteringAnimation.ts instead of manually rebuilding the
object, while preserving the existing image resolution behavior if needed by
applying resolveMarkerImage through the shared path. Verify the overlay path and
any bulk marker handling still produce the same MarkerDescriptor shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ee2ccf2-35f5-41dc-9930-59a454935b85
📒 Files selected for processing (18)
README.mdexample/App.tsxpackage/android/src/main/java/com/margelo/nitro/nitromaps/GoogleMapProviderAdapter.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.ktpackage/ios/AppleMapProviderAdapter.swiftpackage/ios/HybridMapViewDelegate.swiftpackage/ios/MapMarkerAnnotation.swiftpackage/ios/MapOverlayController.swiftpackage/ios/MarkerDescriptor+Fingerprint.swiftpackage/ios/NitroPinAnnotationView.swiftpackage/src/components/MapView.tsxpackage/src/hooks/useCollectedOverlays.tspackage/src/index.tspackage/src/native/specs/overlays.tspackage/src/overlays/normalizeMarkerDescriptors.tspackage/src/types/index.tspackage/src/types/overlays.tspackage/src/utils/enteringAnimation.ts
✅ Files skipped from review due to trivial changes (3)
- package/ios/AppleMapProviderAdapter.swift
- package/src/types/index.ts
- package/ios/NitroPinAnnotationView.swift
🚧 Files skipped from review as they are similar to previous changes (9)
- package/src/index.ts
- package/src/hooks/useCollectedOverlays.ts
- package/ios/MarkerDescriptor+Fingerprint.swift
- package/src/types/overlays.ts
- package/ios/HybridMapViewDelegate.swift
- package/ios/MapMarkerAnnotation.swift
- package/src/native/specs/overlays.ts
- example/App.tsx
- package/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt
Resolves image loading on JS and native layers for bundled assets, with LRU caching, descriptor normalization, and example app demo. Closes #14. Co-authored-by: Cursor <cursoragent@cursor.com>
…pick Co-authored-by: Cursor <cursoragent@cursor.com>
jkasprzyk17
force-pushed
the
feat/custom-marker-images
branch
from
June 28, 2026 18:21
28a4a34 to
061a99e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/MarkerIconFactory.kt`:
- Around line 156-175: The local image path in MarkerIconFactory.loadIconAsync
is still decoding bitmaps on the main thread because non-http(s) URIs call
icon(image) inside deliverOnMainThread. Move the local decode work through
loadExecutor just like the remote branch, then post the BitmapDescriptor? back
via deliverOnMainThread. Also check applyAnchor/displaySizePx so it does not
re-run loadLocalBitmap on the UI thread when width/height are missing; reuse the
already loaded bitmap or compute size off-thread where possible.
- Around line 209-222: The remote marker image fetch in loadRemoteIcon currently
accepts any public MarkerImage.uri and opens it directly, so tighten the input
validation before URL(image.uri).openConnection(). Add scheme checks to allow
only the intended protocols and enforce a host allowlist before any network
access, then keep the existing cacheBitmap/decodeByteArray flow unchanged once
the URL is verified. If the URI is not permitted, fail fast from
loadIconAsync/loadRemoteIcon and log the rejection with the TAG context.
🪄 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: 6a4d49ee-95c8-46ef-bb31-a57a6daeda0c
⛔ Files ignored due to path filters (19)
bun.lockis excluded by!**/*.lockexample/assets/markerblue.pngis excluded by!**/*.pngexample/assets/markerblue@2x.pngis excluded by!**/*.pngexample/assets/markergold.pngis excluded by!**/*.pngexample/assets/markergold@2x.pngis excluded by!**/*.pngexample/assets/markergreen.pngis excluded by!**/*.pngexample/assets/markergreen@2x.pngis excluded by!**/*.pngexample/assets/markerorange.pngis excluded by!**/*.pngexample/assets/markerorange@2x.pngis excluded by!**/*.pngexample/assets/markerred.pngis excluded by!**/*.pngexample/assets/markerred@2x.pngis excluded by!**/*.pngexample/assets/markerviolet.pngis excluded by!**/*.pngexample/assets/markerviolet@2x.pngis excluded by!**/*.pngexample/assets/poiNavigation.pngis excluded by!**/*.pngexample/assets/poiNavigation@2x.pngis excluded by!**/*.pngexample/assets/poiRestaurant.pngis excluded by!**/*.pngexample/assets/poiRestaurant@2x.pngis excluded by!**/*.pngexample/assets/poiStar.pngis excluded by!**/*.pngexample/assets/poiStar@2x.pngis excluded by!**/*.png
📒 Files selected for processing (38)
README.mdexample/App.tsxexample/examples/customMarkerImages.tsexample/examples/index.tsexample/index.jsexample/package.jsonpackage/android/src/main/java/com/margelo/nitro/nitromaps/GoogleMapProviderAdapter.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerDescriptor+Fingerprint.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerDescriptor+MarkerOptions.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerIconFactory.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerViewportFilter.ktpackage/ios/AppleMapProviderAdapter.swiftpackage/ios/HybridMapViewDelegate.swiftpackage/ios/MapMarkerAnnotation.swiftpackage/ios/MapOverlayController.swiftpackage/ios/MarkerClusterEngine.swiftpackage/ios/MarkerDescriptor+Fingerprint.swiftpackage/ios/MarkerImageLoader.swiftpackage/ios/MarkerViewportFilter.swiftpackage/ios/NitroImageAnnotationView.swiftpackage/ios/NitroPinAnnotationView.swiftpackage/ios/OverlayEnteringAnimation.swiftpackage/src/components/MapView.tsxpackage/src/hooks/useCollectedOverlays.tspackage/src/index.tspackage/src/native/specs/overlays.tspackage/src/overlays/__tests__/lruCache.test.tspackage/src/overlays/__tests__/normalizeMarkerDescriptors.test.tspackage/src/overlays/__tests__/resolveMarkerImage.test.tspackage/src/overlays/assetSourceResolver.tspackage/src/overlays/lruCache.tspackage/src/overlays/markerImageFromResolvedAsset.tspackage/src/overlays/normalizeMarkerDescriptors.tspackage/src/overlays/resolveMarkerImage.tspackage/src/types/index.tspackage/src/types/overlays.tspackage/tsconfig.json
💤 Files with no reviewable changes (2)
- package/ios/MarkerViewportFilter.swift
- package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerViewportFilter.kt
✅ Files skipped from review due to trivial changes (4)
- package/src/overlays/assetSourceResolver.ts
- package/ios/OverlayEnteringAnimation.swift
- package/src/types/index.ts
- README.md
🚧 Files skipped from review as they are similar to previous changes (29)
- example/index.js
- package/src/overlays/tests/normalizeMarkerDescriptors.test.ts
- package/src/overlays/lruCache.ts
- package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerDescriptor+Fingerprint.kt
- package/ios/NitroPinAnnotationView.swift
- package/ios/MapOverlayController.swift
- package/ios/AppleMapProviderAdapter.swift
- package/src/overlays/resolveMarkerImage.ts
- example/package.json
- package/tsconfig.json
- package/ios/HybridMapViewDelegate.swift
- package/ios/MarkerDescriptor+Fingerprint.swift
- package/src/overlays/normalizeMarkerDescriptors.ts
- package/src/index.ts
- package/src/components/MapView.tsx
- package/src/hooks/useCollectedOverlays.ts
- example/examples/customMarkerImages.ts
- package/src/native/specs/overlays.ts
- example/examples/index.ts
- package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerDescriptor+MarkerOptions.kt
- package/ios/NitroImageAnnotationView.swift
- package/src/overlays/tests/resolveMarkerImage.test.ts
- package/src/overlays/tests/lruCache.test.ts
- package/src/types/overlays.ts
- package/ios/MarkerImageLoader.swift
- package/ios/MapMarkerAnnotation.swift
- package/android/src/main/java/com/margelo/nitro/nitromaps/GoogleMapProviderAdapter.kt
- example/App.tsx
- package/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt
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.
Closes #14
Summary
MarkerDescriptor/MarkerPropswithimage,anchor,centerOffset,rotation,flat, andopacityfieldsrequire()images on the JS side into stable Nitro-safeMarkerImagedescriptors with LRU cachingMarkerImageLoader,NitroImageAnnotationView, and MapKit adapter updates for custom bitmap markersMarkerIconFactory, anchor/rotation/flat viaMarkerOptions, and overlay diffing via image fingerprintsTest plan
bun testinpackage/(21 tests pass)markersprop with custom imagesimageprop still workMade with Cursor