fix(google-maps): persist user pan and re-emit ready on color-mode re-init - #730
Closed
harlan-zw wants to merge 2 commits into
Closed
fix(google-maps): persist user pan and re-emit ready on color-mode re-init#730harlan-zw wants to merge 2 commits into
harlan-zw wants to merge 2 commits into
Conversation
…-init Two regressions surfaced after the cloud-styled mapId fix (#727): 1. Toggling color mode discarded the user's panned position. The recreate watcher captured the live center before teardown, but the standalone center watcher then re-fired against the new map instance and called `setCenter(propsInitialCenter)`. Persist the captured center into `centerOverride` so the recomputed `options.value.center` matches the new map's actual center; the watcher's lat/lng comparison guard then short-circuits. 2. Imperative resources created off the exposed `map` ref (e.g. pins added outside declarative children) silently disappeared after re-init with no signal to re-attach. Re-emit `ready` after constructing the new Map instance so consumers can re-bind, and document the contract in the map-styling guide.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
|
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)
📝 WalkthroughWalkthroughThe Vue Google Maps component now preserves user pan during map re-initialization by capturing the current map center into a runtime Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Review rate limit: 2/8 reviews remaining, refill in 38 minutes and 13 seconds.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: 1
🧹 Nitpick comments (1)
test/unit/google-maps-regressions.test.ts (1)
624-656: Strengthen this regression to assert runtime behavior, not just value precedence.This currently validates
centerOverride || propsCenter, but it doesn’t assert that the re-assigned map path actually skipssetCenterin the watcher flow. Consider asserting the watcher guard path directly (including a post-reinit explicit prop center change case).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/google-maps-regressions.test.ts` around lines 624 - 656, Update the test to assert runtime behavior of the watcher guard: use the mock map's setCenter mock (from createMockMap / map.setCenter) to verify that when you simulate re-init with centerOverride present the watcher does not call setCenter (expect setCenter not toHaveBeenCalled), then simulate a later prop-derived center change (e.g., change propsCenter to a different lat/lng and recompute optionsCenter or trigger the same watcher flow) and assert setCenter is called with the new coordinates; reference the existing mocks and variables (createMockMap, map.getCenter, map.setCenter, centerOverride, optionsCenter, propsCenter) to locate where to add these assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`:
- Around line 412-420: The current code writes the map's captured lat/lng into
centerOverride.value which then always wins in the defu merge and blocks later
external center updates; change the logic so centerOverride only temporarily
preserves the user's pan: assign centerOverride.value = { lat: ..., lng: ... }
right before teardown/re-init but immediately clear it (e.g.,
centerOverride.value = null or undefined) once the new map is initialized or
whenever props.center / mapOptions.center changes (add/extend the watcher that
handles external center updates to reset centerOverride), and ensure
centerOverride's type accepts null so external updates can again take precedence
in options.value.center.
---
Nitpick comments:
In `@test/unit/google-maps-regressions.test.ts`:
- Around line 624-656: Update the test to assert runtime behavior of the watcher
guard: use the mock map's setCenter mock (from createMockMap / map.setCenter) to
verify that when you simulate re-init with centerOverride present the watcher
does not call setCenter (expect setCenter not toHaveBeenCalled), then simulate a
later prop-derived center change (e.g., change propsCenter to a different
lat/lng and recompute optionsCenter or trigger the same watcher flow) and assert
setCenter is called with the new coordinates; reference the existing mocks and
variables (createMockMap, map.getCenter, map.setCenter, centerOverride,
optionsCenter, propsCenter) to locate where to add these assertions.
🪄 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: 47a2a39e-466b-4fe4-9b8e-594b7bdf3ce1
📒 Files selected for processing (3)
docs/content/scripts/google-maps/1.guides/2.map-styling.mdpackages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vuetest/unit/google-maps-regressions.test.ts
…center Persisting the user-panned center into `centerOverride` made the override sticky in `defu` precedence: any later `props.center` or `props.mapOptions.center` updates would silently fail to propagate because `options.value.center` kept returning the stored override. Add a watcher on the requested center that clears the override whenever the caller changes it. Strengthen the regression test to assert the watcher's setCenter call is actually skipped on re-init, and add a new test covering the post-reinit external center update path.
Collaborator
Author
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.
🔗 Linked issue
Related to #727
❓ Type of change
📚 Description
Two regressions surfaced after the cloud-styled mapId fix in #727:
User pan discarded on color-mode toggle. The recreate watcher captured the live center before teardown, but the standalone center watcher then re-fired against the new map instance and called
setCenter(propsInitialCenter). Persist the captured center intocenterOverrideso the recomputedoptions.value.centermatches the new map's actual center; the watcher's lat/lng comparison guard then short-circuits.Imperative resources silently dropped. Pins (or any objects) created off the exposed
mapref outside declarative children disappeared after re-init with no signal to re-attach them. Re-emitreadyafter constructing the new Map instance so consumers can re-bind, and document the contract in the map-styling guide.Adds regression tests covering both behaviours.