fix: improve Android marker viewport performance - #28
Merged
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds render-version tracking for markers and clusters, uses it in Android viewport diffing, caps entering animations per refresh, refactors camera-triggered refresh scheduling, and updates docs wording to remove iOS-specific framing. ChangesAndroid marker diff and animation budget
Docs generalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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: 4
🤖 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 385-388: The applyMarkersSync() function clears markerVersions and
clusterByKey but fails to invalidate in-flight viewport computations, allowing
stale diffs to be applied after sync reconciliation. After the existing clear()
calls in applyMarkersSync(), bump the refreshGeneration counter and/or cancel
any scheduled viewport refreshes to ensure that previously queued viewport
computations cannot pass their generation check and apply obsolete diffs after
the sync operation completes.
- Around line 174-181: The retained-marker update path in MapOverlayController’s
retained-element handling is missing styling refreshes, so non-positional marker
option changes are ignored. Update the retained update logic to reapply all
marker styling/options, not just position/title/snippet/draggable, and make sure
the marker rebuild path in toMarkerOptions() does not hard-code the default red
icon when a marker’s icon or other visual options have changed. Use the existing
renderVersion and displayedVersions flow to decide when a full rebuild is needed
versus when retained markers can be updated in place.
In
`@package/android/src/main/java/com/margelo/nitro/nitromaps/MarkerClusterEngine.kt`:
- Line 13: The equality gate in MarkerClusterEngine is too weak because
renderVersion is an Int and can collide, causing stale overlays to be retained.
Update the render signature contract in MarkerClusterEngine to use a
collision-resistant structural signature object, or at minimum switch
renderVersion and the related markerVersions storage and comparisons to Long.
Make the same change consistently across the affected MarkerClusterEngine
methods and any implementations that read, compute, or compare renderVersion so
equality checks remain reliable.
- Around line 38-45: The renderVersion getter in MarkerClusterEngine.Cluster is
doing work on every read by sorting memberIds, which is causing avoidable
UI-thread churn when applyDiff stores versions. Move the version/hash
computation out of the getter and compute it once when the Cluster instance is
created, then have renderVersion return the cached value; use Cluster,
renderVersion, and applyDiff to locate the change.
🪄 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: 5ab9b0bb-01a6-4d4d-843c-02445a098a28
📒 Files selected for processing (4)
README.mddocs/architecture.mdpackage/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.ktpackage/android/src/main/java/com/margelo/nitro/nitromaps/MarkerClusterEngine.kt
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/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt (1)
489-498: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winStop resetting the live-refresh throttle in cancellation
cancelLiveRefresh()should only cancel the pending runnable. ClearinglastLiveRefreshMsthere makes every idle→move turnaround look like a fresh session, so the nextonCameraMove()fires immediately and defeatsLIVE_REFRESH_THROTTLE_MS. Reset the baseline only on true full resets likeclear()orapplyMarkersSync().🤖 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/MapOverlayController.kt` around lines 489 - 498, `cancelLiveRefresh()` is resetting the live-refresh throttle state in addition to canceling the pending runnable, which makes the next `onCameraMove()` bypass `LIVE_REFRESH_THROTTLE_MS`. Update `MapOverlayController.cancelLiveRefresh()` so it only removes `liveRefreshRunnable` and clears the reference, and move any `lastLiveRefreshMs` reset to full reset paths like `clear()` or `applyMarkersSync()` where a fresh session should actually start.
🤖 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/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt`:
- Around line 489-498: `cancelLiveRefresh()` is resetting the live-refresh
throttle state in addition to canceling the pending runnable, which makes the
next `onCameraMove()` bypass `LIVE_REFRESH_THROTTLE_MS`. Update
`MapOverlayController.cancelLiveRefresh()` so it only removes
`liveRefreshRunnable` and clears the reference, and move any `lastLiveRefreshMs`
reset to full reset paths like `clear()` or `applyMarkersSync()` where a fresh
session should actually start.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 211c620f-42c9-43bc-8d48-a66d2f0eaf3a
📒 Files selected for processing (1)
package/android/src/main/java/com/margelo/nitro/nitromaps/MapOverlayController.kt
7 tasks
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
1 participant
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.
Summary
Validation