Conversation
`<ScriptGoogleMapsOverlayView>` previously scheduled `panMapToFitOverlay` from `onAdd()` whenever `panOnOpen` was enabled, even when the overlay started closed or `draw()` had not yet resolved a position. That caused unexpected map panning on initial mount and remount when the overlay was hidden. Add two guards: 1. Before scheduling the rAF, check `open !== false`. This skips the rAF entirely when the overlay is mounted closed. 2. Inside the rAF callback, re-check `open` and `overlayPosition` (and the anchor element). The state may have changed during the frame: the controlled `open` could flip to false, the position may not have resolved, or the component may have unmounted. Tests cover the four pan-on-open paths: - defaultOpen=false → no rAF scheduled - :open=false (controlled) → no rAF scheduled - panOnOpen=false → no rAF scheduled - happy path (open + positioned) → rAF still scheduled
commit: |
📝 WalkthroughWalkthroughThe changes refine the Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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. Comment |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts (1)
351-408: Consider adding one test that executes the queued rAF callback.These tests cover scheduling well, but they don’t directly validate the in-callback guard. A focused case that captures the callback, flips
opentofalse, then invokes the callback and assertsmockMap.panByis not called would lock in the regression fix more tightly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts` around lines 351 - 408, Add a test that captures the queued requestAnimationFrame callback from the existing rafSpy, mounts the overlay via mountOverlay (with position set and panOnOpen default/true), then programmatically set the overlay's open prop to false (or mount with controlled open and update it) before invoking the captured rAF callback, and finally assert mocks.mockMap.panBy was not called; this will exercise the in-callback guard inside the overlay's onAdd/panMapToFitOverlay logic and lock in the regression fix.
🤖 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/ScriptGoogleMapsOverlayView.vue`:
- Around line 237-244: The guard currently treats panOnOpen falsy values (like
0) as disabled; update the conditional around panOnOpen in the block that
schedules requestAnimationFrame (the if checking panOnOpen && open.value !==
false) to explicitly check for non-null/undefined or boolean true—e.g. test
panOnOpen !== false && panOnOpen != null (or typeof panOnOpen === 'number' ||
panOnOpen === true) so numeric 0 is accepted; keep the rest of the logic
(calculating padding, calling draw(), and the requestAnimationFrame callback)
unchanged.
---
Nitpick comments:
In `@test/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts`:
- Around line 351-408: Add a test that captures the queued requestAnimationFrame
callback from the existing rafSpy, mounts the overlay via mountOverlay (with
position set and panOnOpen default/true), then programmatically set the
overlay's open prop to false (or mount with controlled open and update it)
before invoking the captured rAF callback, and finally assert
mocks.mockMap.panBy was not called; this will exercise the in-callback guard
inside the overlay's onAdd/panMapToFitOverlay logic and lock in the regression
fix.
🪄 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: e9dfed62-71af-4078-940f-9c14faa6b9cd
📒 Files selected for processing (2)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vuetest/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts
Comment on lines
+237
to
244
| if (panOnOpen && open.value !== false) { | ||
| // Wait for draw() to position the element, then pan. Re-check inside | ||
| // the rAF callback so we don't pan when: | ||
| // - the controlled `open` prop flipped to false during the frame | ||
| // - draw() never resolved a position (closed/missing position) | ||
| // - the anchor element is gone (component unmounted mid-frame) | ||
| const padding = typeof panOnOpen === 'number' ? panOnOpen : 40 | ||
| requestAnimationFrame(() => { |
There was a problem hiding this comment.
panOnOpen: 0 is currently treated as disabled.
Line 237 uses a truthy guard (if (panOnOpen && ...)), so panOnOpen={0} won’t schedule panning even though numeric values are documented as valid custom padding. Please switch to an explicit boolean check.
Suggested fix
- if (panOnOpen && open.value !== false) {
+ if (panOnOpen !== false && open.value !== false) {
// Wait for draw() to position the element, then pan. Re-check inside
// the rAF callback so we don't pan when:
// - the controlled `open` prop flipped to false during the frame
// - draw() never resolved a position (closed/missing position)
// - the anchor element is gone (component unmounted mid-frame)
const padding = typeof panOnOpen === 'number' ? panOnOpen : 40📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (panOnOpen && open.value !== false) { | |
| // Wait for draw() to position the element, then pan. Re-check inside | |
| // the rAF callback so we don't pan when: | |
| // - the controlled `open` prop flipped to false during the frame | |
| // - draw() never resolved a position (closed/missing position) | |
| // - the anchor element is gone (component unmounted mid-frame) | |
| const padding = typeof panOnOpen === 'number' ? panOnOpen : 40 | |
| requestAnimationFrame(() => { | |
| if (panOnOpen !== false && open.value !== false) { | |
| // Wait for draw() to position the element, then pan. Re-check inside | |
| // the rAF callback so we don't pan when: | |
| // - the controlled `open` prop flipped to false during the frame | |
| // - draw() never resolved a position (closed/missing position) | |
| // - the anchor element is gone (component unmounted mid-frame) | |
| const padding = typeof panOnOpen === 'number' ? panOnOpen : 40 | |
| requestAnimationFrame(() => { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue`
around lines 237 - 244, The guard currently treats panOnOpen falsy values (like
0) as disabled; update the conditional around panOnOpen in the block that
schedules requestAnimationFrame (the if checking panOnOpen && open.value !==
false) to explicitly check for non-null/undefined or boolean true—e.g. test
panOnOpen !== false && panOnOpen != null (or typeof panOnOpen === 'number' ||
panOnOpen === true) so numeric 0 is accepted; keep the rest of the logic
(calculating padding, calling draw(), and the requestAnimationFrame callback)
unchanged.
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
Follow-up to #697 (CodeRabbit nit, deferred during PR E review).
❓ Type of change
📚 Description
<ScriptGoogleMapsOverlayView>previously scheduledpanMapToFitOverlayfromonAdd()wheneverpanOnOpenwas enabled, even when the overlay started closed ordraw()had not yet resolved a position. That caused unexpected map panning on initial mount/remount whenever the overlay was hidden.This is a pre-existing bug surfaced by CodeRabbit during the PR E review. I deferred the fix from PR E to keep that PR focused on the class extraction + reactive position rendering refactor; this PR addresses it in isolation with its own regression test.
🛠 Fix
Two guards on the pan-on-open path in
onAdd():requestAnimationFrame, checkopen !== false. This skips the rAF entirely when the overlay is mounted closed.open,overlayAnchor.value, andoverlayPosition.value. State may have shifted during the frame: the controlledopencould have flipped to false, the position may still be unresolved, or the component may have unmounted.🧪 Tests
Four new regression tests in
test/nuxt-runtime/google-maps-overlay-view.nuxt.test.ts(panOnOpen guard for closed/unpositioned overlaysdescribe block) that spy onrequestAnimationFrameand assert againstmockMap.panBy:defaultOpen=false→ no rAF scheduled,panBynot called:open=false(controlled) → no rAF scheduled,panBynot calledpanOnOpen=false→ no rAF scheduled regardless of open stateopen+ positioned) → rAF still scheduled (guard does not over-block)All 104 google-maps-* tests pass; lint and typecheck clean.