fix(google-maps): avoid recentering on inline options#747
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
📝 WalkthroughWalkthroughThis pull request refactors the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 Nitpick comments (3)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue (2)
506-531: 💤 Low valueTwo-stage clear → re-center flow depends on watcher ordering — verify under
flushsemantics.The Line 506 watcher clears
centerOverrideand the Line 509 watcher (sourced oncontrolledCenterKey) is responsible for the resultingsetCenter. Both are registered insideonMountedin order, so under defaultflush: 'pre'they should batch and run in registration order in the same tick, with Line 509 reading the latestcontrolledCenterKeyafter the override is cleared.Worth keeping in mind if either watcher is ever moved to
flush: 'post'or'sync', or split into a separate composable — the ordering invariant becomes load-bearing. A short comment on Line 506 noting “relies on Line 509 watcher firing after this in the same tick to apply the new center” would help future maintainers.🤖 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 `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue` around lines 506 - 531, The two related watchers— the one that clears centerOverride (watch([...], () => { centerOverride.value = undefined })) and the watcher on controlledCenterKey/isMapReady/map that calls map.value!.setCenter(...) — rely on execution ordering within the same tick (default flush: 'pre') so the clear happens before the re-center; add a short inline comment above the first watcher (the centerOverride clear) stating this ordering dependency and warning future maintainers that changing watcher flush mode or moving/splitting these watchers into another composable could break the two-stage clear → re-center flow, referencing the controlledCenterKey/isMapReady/map watcher and getCenterWatchKey for clarity.
292-316: 💤 Low valueClass-instance values short-circuit to reference equality.
isSameOptionValueonly walks plain objects and arrays; anything else (includinggoogle.maps.LatLngBounds,MVCArray,Date, etc.) falls through toObject.is. If a consumer passes such an instance viamapOptions(e.g.restriction.latLngBoundsas aLatLngBoundsinstance) and re-creates it inline on each render, the guard at Line 448 will keep returning false andsetOptionswill still fire on every unrelated re-render — not the originalsetCenterbug, but the same class of regression for non-position fields.This is acceptable given the excluded fields cover the most common cases, but it’s worth either documenting the limitation in the helper’s comment or at least narrowing the guarantee in the JSDoc so users know which option shapes are de-duped.
🤖 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 `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue` around lines 292 - 316, isSameOptionValue short-circuits to Object.is for non-plain objects (e.g., google.maps.LatLngBounds, MVCArray, Date), so recreating class-instance map option values will always appear changed and trigger setOptions; update the helper's JSDoc/comment on isSameOptionValue (and/or isSameMapOptions) to explicitly state it only performs deep equality for plain objects and arrays and does not compare class instances or built‑in types by structural value, or alternatively extend the function to handle specific classes if desired (mentioning isPlainObject, isSameOptionValue, and isSameMapOptions to locate the logic).test/unit/google-maps-regressions.test.ts (1)
340-371: 💤 Low valueEquality helpers duplicated from the component.
isPlainObject/isSameOptionValueare copy-pasted fromScriptGoogleMaps.vue(Lines 292-312). That’s fine for a unit test of the behavior, but it means the test will keep passing if the component’s implementation drifts (e.g. someone tightens or weakens the deep-eq rules) while the test reimplements the old logic.Consider either:
- exporting the helpers from the component (or a sibling module) and importing them here, or
- replacing the local copy with
expect(map.setOptions).not.toHaveBeenCalled()plus an explicit reference-stability test, so the regression spec asserts the externally observable contract instead of re-implementing internals.🤖 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 `@test/unit/google-maps-regressions.test.ts` around lines 340 - 371, The test duplicates deep-equality helpers (isPlainObject, isSameOptionValue, applyOptionsWithStableGuard) from ScriptGoogleMaps.vue which risks drift; either export and import the real helpers from the component (move/ export isPlainObject and isSameOptionValue from ScriptGoogleMaps.vue and import them into google-maps-regressions.test.ts and use them in applyOptionsWithStableGuard), or remove the duplicated logic and change the spec to assert only observable behavior (use expect(map.setOptions).not.toHaveBeenCalled() for the stability case and add a reference-stability test that mutates options and verifies setOptions is/ is not called) so the test no longer re-implements internal equality logic.
🤖 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.
Nitpick comments:
In `@packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue`:
- Around line 506-531: The two related watchers— the one that clears
centerOverride (watch([...], () => { centerOverride.value = undefined })) and
the watcher on controlledCenterKey/isMapReady/map that calls
map.value!.setCenter(...) — rely on execution ordering within the same tick
(default flush: 'pre') so the clear happens before the re-center; add a short
inline comment above the first watcher (the centerOverride clear) stating this
ordering dependency and warning future maintainers that changing watcher flush
mode or moving/splitting these watchers into another composable could break the
two-stage clear → re-center flow, referencing the
controlledCenterKey/isMapReady/map watcher and getCenterWatchKey for clarity.
- Around line 292-316: isSameOptionValue short-circuits to Object.is for
non-plain objects (e.g., google.maps.LatLngBounds, MVCArray, Date), so
recreating class-instance map option values will always appear changed and
trigger setOptions; update the helper's JSDoc/comment on isSameOptionValue
(and/or isSameMapOptions) to explicitly state it only performs deep equality for
plain objects and arrays and does not compare class instances or built‑in types
by structural value, or alternatively extend the function to handle specific
classes if desired (mentioning isPlainObject, isSameOptionValue, and
isSameMapOptions to locate the logic).
In `@test/unit/google-maps-regressions.test.ts`:
- Around line 340-371: The test duplicates deep-equality helpers (isPlainObject,
isSameOptionValue, applyOptionsWithStableGuard) from ScriptGoogleMaps.vue which
risks drift; either export and import the real helpers from the component (move/
export isPlainObject and isSameOptionValue from ScriptGoogleMaps.vue and import
them into google-maps-regressions.test.ts and use them in
applyOptionsWithStableGuard), or remove the duplicated logic and change the spec
to assert only observable behavior (use
expect(map.setOptions).not.toHaveBeenCalled() for the stability case and add a
reference-stability test that mutates options and verifies setOptions is/ is not
called) so the test no longer re-implements internal equality logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7cf7e131-cf75-4a94-8c69-35576f24a509
📒 Files selected for processing (3)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vueplayground/pages/third-parties/google-maps/sfcs.vuetest/unit/google-maps-regressions.test.ts
1 participant
Summary
mapOptionsare recreated by unrelated renderssetCentermapOptionsto cover the migration path from deprecated top-levelzoomFixes #737.
Testing
pnpm vitest run test/unit/google-maps-regressions.test.ts --project unitpnpm eslint packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vue packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMapsRectangle.vue test/unit/google-maps-regressions.test.ts