fix(google-maps): overhaul <ScriptGoogleMaps> and <ScriptGoogleMapsOverlayView>#689
Open
DamianGlowala wants to merge 9 commits intomainfrom
Open
fix(google-maps): overhaul <ScriptGoogleMaps> and <ScriptGoogleMapsOverlayView>#689DamianGlowala wants to merge 9 commits intomainfrom
<ScriptGoogleMaps> and <ScriptGoogleMapsOverlayView>#689DamianGlowala wants to merge 9 commits intomainfrom
Conversation
…leMapsOverlayView>`
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
DamianGlowala
changed the title
<ScriptGoogleMaps> and <ScriptGoogleMapsOverlayView><ScriptGoogleMaps> and <ScriptGoogleMapsOverlayView>
commit: |
|
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:
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactored Google Maps components to export typed interfaces for props, emits, slots, and expose. ScriptGoogleMaps: replaced Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e-dev/first-party.test.ts (1)
41-41:⚠️ Potential issue | 🔴 CriticalFix type error: Map keys cannot include
undefinedwhenregistryKeyis optional.The
registryKeyproperty inRegistryScriptis optional (defined asregistryKey?: RegistryScriptKey), so the Map construction at line 41 creates typeMap<RegistryScriptKey | undefined, RegistryScript>. However,getProxyDef()at lines 44 and 46 expectsMap<string, RegistryScript>, causing a TypeCheckError.Filter scripts with undefined
registryKeybefore creating the Map:Proposed fix
-const scriptByKey = new Map(scripts.map(s => [s.registryKey, s])) +const scriptByKey = new Map( + scripts + .filter((s): s is typeof s & { registryKey: string } => s.registryKey !== undefined) + .map(s => [s.registryKey, s]) +)Alternatively, if all scripts must have a
registryKey, add a runtime assertion:-const scriptByKey = new Map(scripts.map(s => [s.registryKey, s])) +const scriptByKey = new Map(scripts.map((s) => { + if (!s.registryKey) throw new Error(`Script missing registryKey: ${JSON.stringify(s)}`) + return [s.registryKey, s] +}))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e-dev/first-party.test.ts` at line 41, scriptByKey is built from scripts where RegistryScript.registryKey is optional, causing scriptByKey to be Map<RegistryScriptKey|undefined,RegistryScript> and breaking getProxyDef which expects Map<string,RegistryScript>; fix by filtering out entries with undefined registryKey before constructing the map (e.g., scripts.filter(s => s.registryKey != null) then new Map(...)) or add a runtime assertion that every RegistryScript has registryKey before mapping; update the code that constructs scriptByKey (symbol: scriptByKey, type: RegistryScript, property: registryKey) so the resulting Map has keys of type string compatible with getProxyDef.
🧹 Nitpick comments (6)
playground/pages/third-parties/google-maps/query.vue (1)
2-2: Remove unused import.
ScriptGoogleMapsPropsis imported but never used in this file.Proposed fix
-import type { ScriptGoogleMapsProps } from '#nuxt-scripts/components/GoogleMaps/ScriptGoogleMaps.vue' import { ref } from 'vue'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/pages/third-parties/google-maps/query.vue` at line 2, Remove the unused import ScriptGoogleMapsProps from the top of the file: locate the import statement referencing ScriptGoogleMapsProps and delete it (or replace it with only needed imports) so the file no longer imports a symbol that isn't used; run the linter/TypeScript check to verify there are no other unused imports remaining.packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue (2)
184-195: Redundant condition in panOnOpen logic.Since the overlay is only created when
open.valueistrue(per thereadycondition at line 255), theif (props.open)check at line 185 will always be true whenonAdd()runs, making theelsebranch dead code.Consider simplifying or documenting why this branch exists for future maintainability.
♻️ Suggested simplification
if (props.panOnOpen) { - if (props.open) { - this.panOnOpenWatchHandle = whenever(() => props.open, () => { - requestAnimationFrame(() => this.panMapToFitOverlay()) - }, { - immediate: true, - }) - } - else { - requestAnimationFrame(() => this.panMapToFitOverlay()) - } + // Pan immediately on initial open, and on subsequent reopens + this.panOnOpenWatchHandle = whenever(() => props.open, () => { + requestAnimationFrame(() => this.panMapToFitOverlay()) + }, { + immediate: true, + }) }🤖 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 184 - 195, The if (props.open) branch inside the panOnOpen logic is redundant because the overlay is only created when open.value is true; remove the outer if/else and always attach the whenever watcher (assign to panOnOpenWatchHandle) that calls requestAnimationFrame(() => this.panMapToFitOverlay()) with immediate: true when props.panOnOpen is true, and delete the dead else branch; update references to panOnOpenWatchHandle and whenever in onAdd() accordingly or add a short comment if you intentionally keep the else for future cases.
100-103: Awkward type casting with useVModel.The
as falsecast onpassiveis a TypeScript workaround to influence the return type inference, but it's misleading since the runtime value can betrue. The subsequentas Ref<boolean>cast then overrides the entire return type anyway.Consider using a cleaner approach or adding a comment explaining the typing workaround.
♻️ Suggested improvement with explanatory comment
-const open = useVModel(props, 'open', emit, { - defaultValue: props.defaultOpen, - passive: (props.open === undefined) as false, -}) as Ref<boolean> +// useVModel in passive mode when `open` prop is undefined (uncontrolled), +// otherwise syncs with the controlled prop. Type cast needed due to VueUse's +// complex overloaded return types. +const open = useVModel(props, 'open', emit, { + defaultValue: props.defaultOpen, + passive: props.open === undefined, +}) as Ref<boolean>🤖 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 100 - 103, The current line casts the passive option to "as false" and then force-casts the whole result to Ref<boolean>, which is misleading; instead remove the misleading "as false" cast and either pass a properly typed boolean (e.g. passive: props.open === undefined) or use an explicit generic on useVModel to fix inference (e.g. useVModel<boolean>(...)), and add a short comment explaining the typing choice; update the identifier references: change the call to useVModel(props, 'open', emit, { defaultValue: props.defaultOpen, passive: props.open === undefined }) and ensure the returned value open is typed as Ref<boolean> via the generic or an explicit const annotation.packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue (1)
336-336: Uninitialized variable should have explicit type annotation.
mapElWatchHandleis declared withletbut has no initial value, making it implicitlyundefined. The type annotation should reflect this.♻️ Suggested fix
-let mapElWatchHandle: WatchHandle +let mapElWatchHandle: WatchHandle | undefined🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue` at line 336, The variable mapElWatchHandle is declared without an initial value making its type implicitly undefined; update its declaration to include an explicit type that allows undefined (e.g., WatchHandle | undefined) and initialize it to undefined so TypeScript knows the intended shape—modify the declaration of mapElWatchHandle (and keep the WatchHandle import/type) to use the union type and set it to undefined.playground/pages/third-parties/google-maps/overlay-animated.vue (2)
78-88: Non-scoped style may cause global CSS conflicts.The
<style>block is not scoped, meaning theoverlayInkeyframes will be added to the global CSS namespace. This could conflict with other animations using the same name elsewhere in the application.Consider either scoping the style or using a more unique keyframe name (e.g.,
googleMapsOverlayIn).♻️ Suggested fix
-<style> -@keyframes overlayIn { +<style scoped> +@keyframes overlayIn {Or if scoped keyframes don't work as expected with the data-state selector, use a unique name:
-@keyframes overlayIn { +@keyframes googleMapsOverlayIn {And update the class accordingly:
-class="... data-[state=open]:animate-[overlayIn_200ms_ease-out]" +class="... data-[state=open]:animate-[googleMapsOverlayIn_200ms_ease-out]"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/pages/third-parties/google-maps/overlay-animated.vue` around lines 78 - 88, The keyframes named overlayIn are declared in a non-scoped <style> and will pollute the global CSS namespace; rename the animation to a unique name (e.g., googleMapsOverlayIn) or add the scoped attribute to the <style> block, then update any uses of overlayIn (and related selectors that reference the animation or data-state) to the new name so the component's animation won't conflict with other styles.
52-71: Inconsistent indentation in template markup.The content inside
ScriptGoogleMapsOverlayViewhas inconsistent indentation (some lines start with 12 spaces, others with arbitrary spacing). This affects readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/pages/third-parties/google-maps/overlay-animated.vue` around lines 52 - 71, The template markup inside ScriptGoogleMapsOverlayView has inconsistent indentation around the place card (elements like the surrounding <div>, <h3>, <button>, the SVG, rating span and <p>), so reformat the template block to use a consistent indentation style (e.g., 2 or 4 spaces) throughout; specifically align the opening/closing tags and child elements for the container div, the header block using {{ place.name }}, the close(place.id) button and SVG, the rating/reviews line ({{ place.rating }}, {{ place.reviews }}), and the description paragraph ({{ place.desc }}) so all nested lines follow the same indent level for improved readability.
🤖 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 246-255: The Promise-wrapping watch in the block checking
mapsApi.value can hang if mapsApi is already set because the watcher lacks {
immediate: true }; update the watch call (the one created around mapsApi in the
if-block that follows await load()) to pass { immediate: true } in its options
so the callback runs immediately and the Promise resolves, and keep using the
returned stop function (currently named `_`) to teardown the watcher after
resolve; reference the mapsApi reactive, the load() call, and the watcher that
currently assigns `const _ = watch(mapsApi, () => { ... })`.
In
`@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue`:
- Line 2: The import list for Vue types in ScriptGoogleMapsOverlayView.vue is
missing Ref, which is used later as "as Ref<boolean>"; update the import
statement that currently imports ShallowRef and WatchHandle to also import Ref
from 'vue' so the type reference resolves (i.e., add Ref to the import list
alongside ShallowRef and WatchHandle).
- Line 4: Import shallowRef from 'vue' into the existing import list (alongside
computed, inject, useTemplateRef, watch) in ScriptGoogleMapsOverlayView.vue so
the shallowRef symbol used around line 155 resolves at runtime; update the
import statement to include shallowRef and keep existing imports intact.
- Around line 349-358: The template applies props.offset twice and has an
operator-precedence bug: instead of adding offsets in the style binding, rely on
overlayPosition which already includes the offset computed in draw(), so remove
(props.offset?.x ?? 0) and (props.offset?.y ?? 0) from the left/top expressions
and use left: `${overlayPosition?.x ?? 0}px` and top: `${overlayPosition?.y ??
0}px`; if you ever reintroduce manual addition, wrap the nullish-coalescing
operands in parentheses like (overlayPosition?.x ?? 0) + (props.offset?.x ?? 0)
to avoid precedence issues (references: overlayPosition, props.offset, draw(),
ANCHOR_TRANSFORMS).
In `@playground/pages/third-parties/google-maps/nuxt-scripts.vue`:
- Around line 11-15: changeQuery currently assumes googleMapsRef.value is
non-null and calls googleMapsRef.value.resolveQueryToLatLng which can throw if
invoked before mount; add a null/undefined check for googleMapsRef.value (and/or
googleMapsRef.value.resolveQueryToLatLng) at the start of changeQuery and return
early or await component readiness before calling it, then assign the returned
coords to mapOptions.value.center; also correct the query string from
"Brooklyn+Bride,New+York+NY" to "Brooklyn+Bridge,New+York+NY" to fix the typo.
In `@playground/pages/third-parties/google-maps/query.vue`:
- Line 7: The string assigned to the reactive variable query contains a typo:
"Brooklyn+Bride" should be "Brooklyn+Bridge"; update the value of the query ref
(the const query = ref(...)) and any other occurrences of "Brooklyn+Bride" in
the same file (the additional occurrences around the code block at the top and
the later examples) to "Brooklyn+Bridge" so the search query is correct.
---
Outside diff comments:
In `@test/e2e-dev/first-party.test.ts`:
- Line 41: scriptByKey is built from scripts where RegistryScript.registryKey is
optional, causing scriptByKey to be
Map<RegistryScriptKey|undefined,RegistryScript> and breaking getProxyDef which
expects Map<string,RegistryScript>; fix by filtering out entries with undefined
registryKey before constructing the map (e.g., scripts.filter(s => s.registryKey
!= null) then new Map(...)) or add a runtime assertion that every RegistryScript
has registryKey before mapping; update the code that constructs scriptByKey
(symbol: scriptByKey, type: RegistryScript, property: registryKey) so the
resulting Map has keys of type string compatible with getProxyDef.
---
Nitpick comments:
In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`:
- Line 336: The variable mapElWatchHandle is declared without an initial value
making its type implicitly undefined; update its declaration to include an
explicit type that allows undefined (e.g., WatchHandle | undefined) and
initialize it to undefined so TypeScript knows the intended shape—modify the
declaration of mapElWatchHandle (and keep the WatchHandle import/type) to use
the union type and set it to undefined.
In
`@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue`:
- Around line 184-195: The if (props.open) branch inside the panOnOpen logic is
redundant because the overlay is only created when open.value is true; remove
the outer if/else and always attach the whenever watcher (assign to
panOnOpenWatchHandle) that calls requestAnimationFrame(() =>
this.panMapToFitOverlay()) with immediate: true when props.panOnOpen is true,
and delete the dead else branch; update references to panOnOpenWatchHandle and
whenever in onAdd() accordingly or add a short comment if you intentionally keep
the else for future cases.
- Around line 100-103: The current line casts the passive option to "as false"
and then force-casts the whole result to Ref<boolean>, which is misleading;
instead remove the misleading "as false" cast and either pass a properly typed
boolean (e.g. passive: props.open === undefined) or use an explicit generic on
useVModel to fix inference (e.g. useVModel<boolean>(...)), and add a short
comment explaining the typing choice; update the identifier references: change
the call to useVModel(props, 'open', emit, { defaultValue: props.defaultOpen,
passive: props.open === undefined }) and ensure the returned value open is typed
as Ref<boolean> via the generic or an explicit const annotation.
In `@playground/pages/third-parties/google-maps/overlay-animated.vue`:
- Around line 78-88: The keyframes named overlayIn are declared in a non-scoped
<style> and will pollute the global CSS namespace; rename the animation to a
unique name (e.g., googleMapsOverlayIn) or add the scoped attribute to the
<style> block, then update any uses of overlayIn (and related selectors that
reference the animation or data-state) to the new name so the component's
animation won't conflict with other styles.
- Around line 52-71: The template markup inside ScriptGoogleMapsOverlayView has
inconsistent indentation around the place card (elements like the surrounding
<div>, <h3>, <button>, the SVG, rating span and <p>), so reformat the template
block to use a consistent indentation style (e.g., 2 or 4 spaces) throughout;
specifically align the opening/closing tags and child elements for the container
div, the header block using {{ place.name }}, the close(place.id) button and
SVG, the rating/reviews line ({{ place.rating }}, {{ place.reviews }}), and the
description paragraph ({{ place.desc }}) so all nested lines follow the same
indent level for improved readability.
In `@playground/pages/third-parties/google-maps/query.vue`:
- Line 2: Remove the unused import ScriptGoogleMapsProps from the top of the
file: locate the import statement referencing ScriptGoogleMapsProps and delete
it (or replace it with only needed imports) so the file no longer imports a
symbol that isn't used; run the linter/TypeScript check to verify there are no
other unused imports remaining.
🪄 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: 316d7327-225c-4bc6-9693-bd2decdbe7ae
📒 Files selected for processing (15)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vuepackages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vueplayground/pages/index.vueplayground/pages/third-parties/google-maps/center.vueplayground/pages/third-parties/google-maps/emit-test.vueplayground/pages/third-parties/google-maps/geojson-test.vueplayground/pages/third-parties/google-maps/markers.vueplayground/pages/third-parties/google-maps/null.vueplayground/pages/third-parties/google-maps/nuxt-scripts.vueplayground/pages/third-parties/google-maps/overlay-animated.vueplayground/pages/third-parties/google-maps/overlay-popup.vueplayground/pages/third-parties/google-maps/query.vueplayground/pages/third-parties/google-maps/sfcs.vueplayground/pages/third-parties/google-maps/styled.vuetest/e2e-dev/first-party.test.ts
💤 Files with no reviewable changes (3)
- playground/pages/third-parties/google-maps/markers.vue
- playground/pages/index.vue
- playground/pages/third-parties/google-maps/styled.vue
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue
Show resolved
Hide resolved
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue
Outdated
Show resolved
Hide resolved
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue
Show resolved
Hide resolved
Comment on lines
+11
to
15
| async function changeQuery() { | ||
| const res = await googleMapsRef.value.resolveQueryToLatLng('Brooklyn+Bride,New+York+NY') | ||
|
|
||
| mapOptions.value.center = res | ||
| } |
There was a problem hiding this comment.
Add null check before accessing googleMapsRef.value.
If changeQuery is invoked before the component mounts, googleMapsRef.value will be null, causing a runtime error when calling resolveQueryToLatLng.
Also note the query string contains "Bride" (should likely be "Bridge").
Proposed fix
async function changeQuery() {
+ if (!googleMapsRef.value) {
+ return
+ }
- const res = await googleMapsRef.value.resolveQueryToLatLng('Brooklyn+Bride,New+York+NY')
+ const res = await googleMapsRef.value.resolveQueryToLatLng('Brooklyn+Bridge,New+York+NY')
- mapOptions.value.center = res
+ if (res) {
+ mapOptions.value.center = res
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playground/pages/third-parties/google-maps/nuxt-scripts.vue` around lines 11
- 15, changeQuery currently assumes googleMapsRef.value is non-null and calls
googleMapsRef.value.resolveQueryToLatLng which can throw if invoked before
mount; add a null/undefined check for googleMapsRef.value (and/or
googleMapsRef.value.resolveQueryToLatLng) at the start of changeQuery and return
early or await component readiness before calling it, then assign the returned
coords to mapOptions.value.center; also correct the query string from
"Brooklyn+Bride,New+York+NY" to "Brooklyn+Bridge,New+York+NY" to fix the typo.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
playground/pages/third-parties/google-maps/nuxt-scripts.vue (1)
11-15:⚠️ Potential issue | 🟠 MajorGuard ref access and only assign valid coordinates.
Line 12 dereferences
googleMapsRef.valuewithout a mount check, and Line 14 assignsreswithout verifying it exists. This can throw before mount and can write an invalid center when geocoding fails.Proposed fix
async function changeQuery() { - const res = await googleMapsRef.value.resolveQueryToLatLng('Brooklyn+Bridge,New+York+NY') - - mapOptions.value.center = res + if (!googleMapsRef.value) { + return + } + + try { + const res = await googleMapsRef.value.resolveQueryToLatLng('Brooklyn+Bridge,New+York+NY') + if (res) { + mapOptions.value.center = res + } + } + catch (error) { + console.error('Failed to resolve query to coordinates:', error) + } }#!/bin/bash set -euo pipefail # Verify exposed API contract and throw behavior. fd -i "ScriptGoogleMaps.vue" | xargs -I{} rg -n "resolveQueryToLatLng|throw new Error" "{}" # Verify this file's call-site has no guard before dereference/assignment. rg -n -C3 "async function changeQuery|resolveQueryToLatLng|mapOptions\\.value\\.center" --type=vue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/pages/third-parties/google-maps/nuxt-scripts.vue` around lines 11 - 15, In changeQuery, guard dereference of googleMapsRef.value by checking the ref is mounted/defined (googleMapsRef && googleMapsRef.value) before calling resolveQueryToLatLng, wrap the call in try/catch, and only assign mapOptions.value.center if the resolved result (res) is non-null and matches expected coordinate shape (e.g., has lat/lng numeric properties or valid array); use resolveQueryToLatLng and mapOptions identifiers to locate the code and ensure you handle failures (log or noop) instead of writing an invalid center.packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue (1)
352-353:⚠️ Potential issue | 🟠 MajorRemove offset terms from template
left/topand fix??precedence pitfall.This issue is still present from earlier review feedback.
draw()already appliesprops.offset, and Lines 352-353 still use expressions where+and??precedence is error-prone.Suggested diff
- left: `${overlayPosition?.x ?? 0 + (props.offset?.x ?? 0)}px`, - top: `${overlayPosition?.y ?? 0 + (props.offset?.y ?? 0)}px`, + left: `${overlayPosition?.x ?? 0}px`, + top: `${overlayPosition?.y ?? 0}px`,🤖 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 352 - 353, The template's inline style for left/top mistakenly re-applies props.offset and misuses the + and ?? precedence; remove the offset addition (draw() already applies props.offset) and ensure nullish-coalescing wraps each operand or the whole sum so the fallback to 0 isn't bypassed—update the left/top expressions in ScriptGoogleMapsOverlayView.vue to use overlayPosition?.x ?? 0 and overlayPosition?.y ?? 0 (or parenthesize like (overlayPosition?.x ?? 0) + (props.offset?.x ?? 0) only if you intend to keep offsets) to fix the precedence pitfall and avoid double-applying offset; reference draw(), overlayPosition, and props.offset when making the change.
🧹 Nitpick comments (1)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue (1)
100-103: Avoid unsafeuseVModeltype assertions.Line 102 (
as false) and Line 103 (as ShallowRef<boolean>) force types that don’t reflect runtime behavior. This makes the open-state contract harder to reason about and can hide future typing regressions.Suggested diff
const open = useVModel(props, 'open', emit, { defaultValue: props.defaultOpen, - passive: (props.open === undefined) as false, -}) as ShallowRef<boolean> + passive: props.open === undefined, +})🤖 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 100 - 103, The code forces unsafe casts on the useVModel call — remove the "as false" and "as ShallowRef<boolean>" assertions and let types be explicit: call useVModel with an explicit generic for the boolean type (e.g., useVModel<boolean>(props, 'open', emit, {...})), and pass a proper boolean expression for passive (passive: props.open === undefined) so the passive option is typed as boolean; keep defaultValue: props.defaultOpen and assign the result to a properly typed ref (e.g., const open: ShallowRef<boolean> = useVModel<boolean>(...)) rather than using inline type 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/ScriptGoogleMapsOverlayView.vue`:
- Line 4: Reorder the named imports on the Vue import line to satisfy the
perfectionist/sort-named-imports rule: ensure shallowRef appears before watch in
the import list (i.e., import { computed, inject, useTemplateRef, shallowRef,
watch } from 'vue'), so update the import statement in
ScriptGoogleMapsOverlayView.vue accordingly.
In `@playground/pages/third-parties/google-maps/query.vue`:
- Around line 20-22: The call to
googleMapsRef.value.resolveQueryToLatLng(query.value) inside changeQuery is
unhandled and can cause unhandled promise rejections on geocode/network
failures; wrap the await in a try/catch in changeQuery, handle errors by logging
or showing a user-facing message and gracefully returning (or setting an error
state) so the click handler doesn't propagate the rejection, referencing
changeQuery, googleMapsRef, resolveQueryToLatLng, and query.value.
- Line 35: The Vue template currently hardcodes the Google Maps API key in the
api-key attribute; remove that literal and instead read the key from runtime
configuration/environment and bind it into the component (e.g., replace the
literal api-key attribute with a bound property like
:api-key="googleMapsApiKey"), sourcing googleMapsApiKey from your runtime config
or process env (use your framework's runtime config helper such as
useRuntimeConfig()/process.env) so the key is not committed, and rotate the
leaked key immediately.
---
Duplicate comments:
In
`@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue`:
- Around line 352-353: The template's inline style for left/top mistakenly
re-applies props.offset and misuses the + and ?? precedence; remove the offset
addition (draw() already applies props.offset) and ensure nullish-coalescing
wraps each operand or the whole sum so the fallback to 0 isn't bypassed—update
the left/top expressions in ScriptGoogleMapsOverlayView.vue to use
overlayPosition?.x ?? 0 and overlayPosition?.y ?? 0 (or parenthesize like
(overlayPosition?.x ?? 0) + (props.offset?.x ?? 0) only if you intend to keep
offsets) to fix the precedence pitfall and avoid double-applying offset;
reference draw(), overlayPosition, and props.offset when making the change.
In `@playground/pages/third-parties/google-maps/nuxt-scripts.vue`:
- Around line 11-15: In changeQuery, guard dereference of googleMapsRef.value by
checking the ref is mounted/defined (googleMapsRef && googleMapsRef.value)
before calling resolveQueryToLatLng, wrap the call in try/catch, and only assign
mapOptions.value.center if the resolved result (res) is non-null and matches
expected coordinate shape (e.g., has lat/lng numeric properties or valid array);
use resolveQueryToLatLng and mapOptions identifiers to locate the code and
ensure you handle failures (log or noop) instead of writing an invalid center.
---
Nitpick comments:
In
`@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue`:
- Around line 100-103: The code forces unsafe casts on the useVModel call —
remove the "as false" and "as ShallowRef<boolean>" assertions and let types be
explicit: call useVModel with an explicit generic for the boolean type (e.g.,
useVModel<boolean>(props, 'open', emit, {...})), and pass a proper boolean
expression for passive (passive: props.open === undefined) so the passive option
is typed as boolean; keep defaultValue: props.defaultOpen and assign the result
to a properly typed ref (e.g., const open: ShallowRef<boolean> =
useVModel<boolean>(...)) rather than using inline type 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: 46941885-4c83-4005-bf41-1d8ec76fbded
📒 Files selected for processing (3)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vueplayground/pages/third-parties/google-maps/nuxt-scripts.vueplayground/pages/third-parties/google-maps/query.vue
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue
Outdated
Show resolved
Hide resolved
Comment on lines
+20
to
+22
| const queryLatLng = await googleMapsRef.value.resolveQueryToLatLng( | ||
| query.value | ||
| ) |
There was a problem hiding this comment.
Handle geocode failures to avoid unhandled rejections.
changeQuery awaits a network-dependent call without error handling. A failed geocode request will reject and bubble from the click handler.
Proposed fix
async function changeQuery() {
if (!googleMapsRef.value) {
return
}
query.value = query.value.startsWith('Statue')
? 'Brooklyn+Bridge,New+York+NY'
: 'Statue+of+Liberty+National+Monument+New+York+NY'
- const queryLatLng = await googleMapsRef.value.resolveQueryToLatLng(
- query.value
- )
-
- if (queryLatLng) {
- center.value = queryLatLng
+ try {
+ const queryLatLng = await googleMapsRef.value.resolveQueryToLatLng(query.value)
+ if (queryLatLng) {
+ center.value = queryLatLng
+ }
+ }
+ catch (error) {
+ console.error('Failed to resolve map query:', error)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playground/pages/third-parties/google-maps/query.vue` around lines 20 - 22,
The call to googleMapsRef.value.resolveQueryToLatLng(query.value) inside
changeQuery is unhandled and can cause unhandled promise rejections on
geocode/network failures; wrap the await in a try/catch in changeQuery, handle
errors by logging or showing a user-facing message and gracefully returning (or
setting an error state) so the click handler doesn't propagate the rejection,
referencing changeQuery, googleMapsRef, resolveQueryToLatLng, and query.value.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue (1)
4-4:⚠️ Potential issue | 🟡 MinorFix named import ordering to unblock lint
Line 4 still violates
perfectionist/sort-named-imports(shallowRefmust come beforewatch), and CI is failing on it.Suggested diff
-import { computed, inject, useTemplateRef, watch, shallowRef } from 'vue' +import { computed, inject, shallowRef, useTemplateRef, watch } from 'vue'🤖 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` at line 4, The named imports on the Vue line are out of order and break perfectionist/sort-named-imports: move shallowRef to appear before watch in the import list (i.e., reorder the named imports so shallowRef comes earlier than watch), then save and re-run the linter/CI to confirm the rule passes; locate the import statement that includes computed, inject, useTemplateRef, watch, shallowRef and reorder accordingly.
🧹 Nitpick comments (1)
playground/pages/third-parties/google-maps/sfcs.vue (1)
68-84: Make the heatmap seed data assignment idempotent.This callback appends on every run. If
wheneverfires again, the example duplicates all points instead of rebuilding the same dataset.♻️ Suggested cleanup
whenever(() => googleMapsRef.value?.mapsApi, (mapsApi) => { - heatmapLayerData.value.push(...[ + heatmapLayerData.value = [ new mapsApi.LatLng(-33.8688, 151.2093), new mapsApi.LatLng(-33.8690, 151.2100), new mapsApi.LatLng(-33.8700, 151.2150), new mapsApi.LatLng(-33.8710, 151.2200), new mapsApi.LatLng(-33.8720, 151.2250), new mapsApi.LatLng(-33.8730, 151.2300), new mapsApi.LatLng(-33.8740, 151.2350), new mapsApi.LatLng(-33.8750, 151.2400), new mapsApi.LatLng(-33.8760, 151.2450), new mapsApi.LatLng(-33.8770, 151.2500), new mapsApi.LatLng(-33.8780, 151.2550), new mapsApi.LatLng(-33.8790, 151.2600), new mapsApi.LatLng(-33.8800, 151.2650), new mapsApi.LatLng(-33.8810, 151.2700), - ]) + ] })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playground/pages/third-parties/google-maps/sfcs.vue` around lines 68 - 84, The whenever callback is appending seed points on every run causing duplicates; change the logic in the whenever handler (the one referencing googleMapsRef, mapsApi and heatmapLayerData) to replace the heatmap data instead of pushing: build the array of new mapsApi.LatLng instances and assign it to heatmapLayerData.value (or clear heatmapLayerData.value and then push) so the dataset is rebuilt idempotently each time the handler runs.
🤖 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 307-310: The watch source is reading props.position?.lat/?.lng as
properties which breaks when position is a google.maps.LatLng (its lat/lng are
methods); update the watch to normalize position the same way
getResolvedPosition() does—call position.lat() / position.lng() when those are
functions, otherwise read the properties—so the dependency array calculates
numeric lat/lng values and triggers overlayView.value?.draw() on real coordinate
changes; keep references to the existing watch call, props.position,
getResolvedPosition(), and overlayView.value?.draw() when making this change.
---
Duplicate comments:
In
`@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue`:
- Line 4: The named imports on the Vue line are out of order and break
perfectionist/sort-named-imports: move shallowRef to appear before watch in the
import list (i.e., reorder the named imports so shallowRef comes earlier than
watch), then save and re-run the linter/CI to confirm the rule passes; locate
the import statement that includes computed, inject, useTemplateRef, watch,
shallowRef and reorder accordingly.
---
Nitpick comments:
In `@playground/pages/third-parties/google-maps/sfcs.vue`:
- Around line 68-84: The whenever callback is appending seed points on every run
causing duplicates; change the logic in the whenever handler (the one
referencing googleMapsRef, mapsApi and heatmapLayerData) to replace the heatmap
data instead of pushing: build the array of new mapsApi.LatLng instances and
assign it to heatmapLayerData.value (or clear heatmapLayerData.value and then
push) so the dataset is rebuilt idempotently each time the handler runs.
🪄 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: 2d9770c3-cbb9-4ecf-b641-6c9bed53331e
📒 Files selected for processing (8)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vueplayground/pages/third-parties/google-maps/center.vueplayground/pages/third-parties/google-maps/emit-test.vueplayground/pages/third-parties/google-maps/geojson-test.vueplayground/pages/third-parties/google-maps/markers.vueplayground/pages/third-parties/google-maps/null.vueplayground/pages/third-parties/google-maps/query.vueplayground/pages/third-parties/google-maps/sfcs.vue
💤 Files with no reviewable changes (1)
- playground/pages/third-parties/google-maps/markers.vue
🚧 Files skipped from review as they are similar to previous changes (3)
- playground/pages/third-parties/google-maps/emit-test.vue
- playground/pages/third-parties/google-maps/null.vue
- playground/pages/third-parties/google-maps/query.vue
Comment on lines
307
to
310
| watch( | ||
| () => [props.position?.lat, props.position?.lng, props.offset?.x, props.offset?.y, props.zIndex, props.anchor], | ||
| () => { overlay.value?.draw() }, | ||
| () => { overlayView.value?.draw() }, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the Google Maps JavaScript API, are google.maps.LatLng.latandgoogle.maps.LatLng.lngmethods (requiringlat()/lng()), and what does property access without parentheses return?
💡 Result:
Yes, in the Google Maps JavaScript API, google.maps.LatLng.lat and google.maps.LatLng.lng are methods, requiring parentheses as lat and lng to retrieve the latitude and longitude values. Property access without parentheses (e.g., lat or lng) does not work and returns undefined, as there are no such public properties on the LatLng object—only the getter methods exist. This design prevents direct modification and ensures immutability. Use LatLngLiteral objects {lat: number, lng: number} for plain property access where supported.
Citations:
- 1: https://developers.google.com/maps/documentation/javascript/reference/coordinates
- 2: https://developers.google.com/maps/documentation/javascript/reference/3.57
- 3: https://www.endpointdev.com/blog/2015/06/google-maps-javascript-api-latlng/
- 4: https://stackoverflow.com/questions/16748716/cannot-use-google-maps-latlng-objects-lat
🏁 Script executed:
# First, find and read the file mentioned in the review
cat -n packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue | head -350 | tail -100Repository: nuxt/scripts
Length of output: 3955
🏁 Script executed:
# Read the beginning of the file to find prop definitions
head -150 packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue | tail -100Repository: nuxt/scripts
Length of output: 3123
🏁 Script executed:
# Search for the props interface definition
grep -n "ScriptGoogleMapsOverlayViewProps" packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vue | head -5Repository: nuxt/scripts
Length of output: 191
🏁 Script executed:
# Read the props interface definition
sed -n '15,83p' packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsOverlayView.vueRepository: nuxt/scripts
Length of output: 2370
Watch source misses google.maps.LatLng coordinate updates
Lines 308-309 access props.position?.lat and props.position?.lng without checking the position type. Since props.position can be either google.maps.LatLng (where .lat/.lng are methods) or google.maps.LatLngLiteral (where they are properties), accessing them as properties when it's a LatLng instance returns the method function itself rather than the coordinate value. The watch then fails to detect position changes. Call the methods for LatLng instances and access properties for plain objects, consistent with the pattern already used in getResolvedPosition().
Suggested diff
watch(
- () => [props.position?.lat, props.position?.lng, props.offset?.x, props.offset?.y, props.zIndex, props.anchor],
+ () => {
+ const position = props.position
+ const lat = position instanceof google.maps.LatLng ? position.lat() : position?.lat
+ const lng = position instanceof google.maps.LatLng ? position.lng() : position?.lng
+ return [lat, lng, props.offset?.x, props.offset?.y, props.zIndex, props.anchor]
+ },
() => { overlayView.value?.draw() },
)📝 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
| watch( | |
| () => [props.position?.lat, props.position?.lng, props.offset?.x, props.offset?.y, props.zIndex, props.anchor], | |
| () => { overlay.value?.draw() }, | |
| () => { overlayView.value?.draw() }, | |
| ) | |
| watch( | |
| () => { | |
| const position = props.position | |
| const lat = position instanceof google.maps.LatLng ? position.lat() : position?.lat | |
| const lng = position instanceof google.maps.LatLng ? position.lng() : position?.lng | |
| return [lat, lng, props.offset?.x, props.offset?.y, props.zIndex, props.anchor] | |
| }, | |
| () => { overlayView.value?.draw() }, | |
| ) |
🤖 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 307 - 310, The watch source is reading props.position?.lat/?.lng as
properties which breaks when position is a google.maps.LatLng (its lat/lng are
methods); update the watch to normalize position the same way
getResolvedPosition() does—call position.lat() / position.lng() when those are
functions, otherwise read the properties—so the dependency array calculates
numeric lat/lng values and triggers overlayView.value?.draw() on real coordinate
changes; keep references to the existing watch call, props.position,
getResolvedPosition(), and overlayView.value?.draw() when making this change.
harlan-zw
reviewed
Apr 5, 2026
| import { useScriptTriggerElement } from '#nuxt-scripts/composables/useScriptTriggerElement' | ||
| import { useScriptGoogleMaps } from '#nuxt-scripts/registry/google-maps' | ||
| import { scriptRuntimeConfig, scriptsPrefix } from '#nuxt-scripts/utils' | ||
| import { whenever } from '@vueuse/shared' |
Collaborator
There was a problem hiding this comment.
is this a new implicit dep? We may need to make it explicit if we need it
Collaborator
|
Happy to look at getting these changes in, but we need to be very mindful of any breaking changes. We need to properly document them and have dev warnings for old usage; we prefer deprecations. It may be easier to make more targeted PRs. |
Member
Author
|
More than happy to discuss and revert any breaking changes you believe shouldn't be introduced at this stage to get this one merged 😊 |
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.
🔗 Linked issue
📚 Description
I've rewritten or tweaked majority of both mentioned components. Some of the major changes include simplified open state handling for overlay view (accounting for using either v-if or open prop) and fixing various bugs discovered while updating SFCs examples.
I've split overlay view anchor from the content. This lets users declare open animations directly on the
<ScriptGoogleMapsOverlayView>and removes the need of wrapping div inside its slot.I am also thinking a lot about whether we should let users benefit from exposed
resolveQueryToLatLngrather than increase complexity of transforming map center internally (which this PR removes and updates examples accordingly).Also, having
zoomandcenteras top-level options AND being able to set these viamap-optionswas a bit confusing, I'd rather stick to one way of declaring them. This PR goes for themap-optionsapproach.