fix(google-maps): close races in resolveQueryToLatLng#693
Merged
Conversation
resolveQueryToLatLng could hang forever or throw, depending on timing: 1. The wait pattern after `await load()` used a non-immediate watcher. When load() populated mapsApi synchronously the watcher missed the change and the promise never resolved. 2. PlacesService construction dereferenced `map.value!`, but the function is publicly exposed and could be called before onLoaded populated the map ref, throwing on the bang. Extract a `waitForMapsReady` helper into useGoogleMapsResource that mirrors importLibrary's correct pattern: short-circuit when both refs are already set, re-check after `await load()`, then install an immediate watcher inside a detached effect scope. The watcher waits for both mapsApi and map and rejects if status enters 'error'.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR extracts inline readiness logic from ScriptGoogleMaps.vue into a new async helper waitForMapsReady in useGoogleMapsResource.ts. Components now await this helper to ensure both the Google Maps API ref and the map ref are populated (or reject on error) instead of using a manual watch-based promise. waitForMapsReady uses effectScope and watch to observe mapsApi, map, and status, calls load() when appropriate, and ensures cleanup. Unit tests were added to cover immediate/synchronous/asynchronous resolution and error cases for the new helper. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/google-maps-lifecycle.test.ts (1)
404-410: Avoid wall-clock timeouts in the pending-state assertions.The 30ms/100ms
setTimeoutraces can flake on slow CI even when the behavior is correct. A settlement flag or fake timers gives the same coverage without depending on scheduler latency.♻️ Possible rewrite without real timers
it('does not hang when load() resolves synchronously', async () => { @@ - await expect( - Promise.race([ - waitForMapsReady({ mapsApi, map, status, load }), - new Promise((_, reject) => setTimeout(() => reject(new Error('hang')), 100)), - ]), - ).resolves.toBeUndefined() + await expect(waitForMapsReady({ mapsApi, map, status, load })).resolves.toBeUndefined() @@ it('waits for map even when mapsApi is set first', async () => { @@ - const racedEarly = await Promise.race([ - promise.then(() => 'resolved'), - new Promise(resolve => setTimeout(resolve, 30, 'pending')), - ]) - expect(racedEarly).toBe('pending') + let settled = false + promise.then(() => { + settled = true + }) + await nextTick() + expect(settled).toBe(false)Also applies to: 470-475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/google-maps-lifecycle.test.ts` around lines 404 - 410, The test currently races waitForMapsReady(...) against a real setTimeout which causes flaky CI; update the test to avoid wall-clock timers by either (a) making waitForMapsReady return a settlement signal you can await (e.g., a promise or resolved flag) and assert that it resolves without using setTimeout, or (b) use Jest fake timers (jest.useFakeTimers()/advanceTimersByTime) and replace the new Promise timeout with a controlled timer advance; target the call site using waitForMapsReady({ mapsApi, map, status, load }) (also update the similar pattern at lines ~470-475) so the assertion no longer depends on real scheduler latency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/google-maps-lifecycle.test.ts`:
- Around line 404-410: The test currently races waitForMapsReady(...) against a
real setTimeout which causes flaky CI; update the test to avoid wall-clock
timers by either (a) making waitForMapsReady return a settlement signal you can
await (e.g., a promise or resolved flag) and assert that it resolves without
using setTimeout, or (b) use Jest fake timers
(jest.useFakeTimers()/advanceTimersByTime) and replace the new Promise timeout
with a controlled timer advance; target the call site using waitForMapsReady({
mapsApi, map, status, load }) (also update the similar pattern at lines
~470-475) so the assertion no longer depends on real scheduler latency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: acec8b6a-0252-4c70-bd64-185e5f2d643f
📒 Files selected for processing (3)
packages/script/src/runtime/components/GoogleMaps/ScriptGoogleMaps.vuepackages/script/src/runtime/components/GoogleMaps/useGoogleMapsResource.tstest/unit/google-maps-lifecycle.test.ts
CodeRabbit flagged the setTimeout-based race patterns as potentially flaky on slow CI. Replace with deterministic alternatives: - "does not hang" test now relies on vitest's per-test timeout as the backstop. The fix prevents the hang, so the await resolves immediately on the happy path. - "waits for map" test uses a settled flag plus nextTick flushes instead of a wall-clock setTimeout race.
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 #689 (split out of the umbrella PR; surfaced by CodeRabbit's review of #692).
❓ Type of change
📚 Description
resolveQueryToLatLng(the publicly exposed map helper) had two pre-existing bugs that could leave it hung or throw, depending on timing.await load(). The wait pattern installed a non-immediate watcher onmapsApi. Whenload()populatedmapsApisynchronously, the watcher missed the change and the promise hung forever.map.valuewhen constructingPlacesService. The function is publicly exposed and can be called beforeonLoadedpopulatesmap.value. Thenew mapsApi.value!.places.PlacesService(map.value!)line would throw on the bang.Extracted a
waitForMapsReadyhelper intouseGoogleMapsResourcethat mirrorsimportLibrary's correct pattern: short-circuit when both refs are already set, re-check afterawait load(), then install an{ immediate: true }watcher inside a detachedeffectScopeso it's safe to call from any context (component setup, exposed methods, tests). The watcher waits for bothmapsApiandmap, and rejects ifstatusenters'error'.Pure bug fix — no API changes. All existing protected behaviours (skip-if-equal center watcher, setOptions exclusion of zoom/center, InfoWindow group close, overlay
data-state) remain intact.🧪 Tests
Added 6 unit tests for
waitForMapsReadyintest/unit/google-maps-lifecycle.test.ts:load()call)load()resolution does not hang (the original bug)'error'status rejects synchronously without callingload()mapeven whenmapsApiis set firstAll 63 google-maps-* unit tests pass; lint, typecheck, build clean.