fix loading of google maps libraries #384
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates Google Maps loading to explicitly import maps, marker, and geometry libraries via Promise.all instead of using the deprecated libraries option on the loader, ensuring all required libraries are loaded before resolving. Sequence diagram for updated Google Maps library loadingsequenceDiagram
participant Component
participant loadGMaps
participant GMapLoader
Component->>loadGMaps: loadGMaps()
activate loadGMaps
loadGMaps->>GMapLoader: setOptions(key, v)
loadGMaps->>GMapLoader: importLibrary(maps)
loadGMaps->>GMapLoader: importLibrary(marker)
loadGMaps->>GMapLoader: importLibrary(geometry)
GMapLoader-->>loadGMaps: Promise.all resolved
loadGMaps-->>Component: gMapsApiLoading Promise resolved
deactivate loadGMaps
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughRefactored Google Maps library initialization in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
Promise.all([...]).then(() => resolve())chain doesn’t handle rejections; consider propagating failures (e.g., usingcatchorasync/awaitwithtry/catch) so callers can distinguish load errors from success. - You can simplify the
thencallback by passingresolvedirectly (...).then(resolve)instead of wrapping it in an arrow function with an explicitvoidreturn type.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Promise.all([...]).then(() => resolve())` chain doesn’t handle rejections; consider propagating failures (e.g., using `catch` or `async/await` with `try/catch`) so callers can distinguish load errors from success.
- You can simplify the `then` callback by passing `resolve` directly (`...).then(resolve)` instead of wrapping it in an arrow function with an explicit `void` return type.
## Individual Comments
### Comment 1
<location> `apps/fxc-front/src/app/components/2d/map-element.ts:57-61` </location>
<code_context>
- libraries: ['geometry', 'marker'],
});
- gMapLoader.importLibrary('maps').then(() => resolve());
+ Promise.all([
+ gMapLoader.importLibrary('maps'),
+ gMapLoader.importLibrary('marker'),
+ gMapLoader.importLibrary('geometry'),
+ ]).then((): void => {
+ resolve();
+ });
</code_context>
<issue_to_address>
**issue (bug_risk):** Promise.all lacks rejection handling, which can leave gMapsApiLoading unresolved on failure
If any library fails to load, the outer promise never settles and callers can hang indefinitely. Please either propagate rejection (e.g., expose `reject` and use `.then(resolve, reject)`), or attach a `.catch` that resolves or rejects `gMapsApiLoading` with a clear failure state.
</issue_to_address>
### Comment 2
<location> `apps/fxc-front/src/app/components/2d/map-element.ts:61-62` </location>
<code_context>
+ gMapLoader.importLibrary('maps'),
+ gMapLoader.importLibrary('marker'),
+ gMapLoader.importLibrary('geometry'),
+ ]).then((): void => {
+ resolve();
+ });
};
</code_context>
<issue_to_address>
**suggestion:** The extra Promise wrapper and explicit resolve in `.then` could be simplified
Since `gMapsApiLoading` already reflects the loader’s resolution, you can simplify this to `.then(resolve)` instead of wrapping `resolve` in a function. You might also be able to return `Promise.all` directly from `loadGMaps` and drop the extra `new Promise` if no other code relies on holding an external `resolve` reference.
Suggested implementation:
```typescript
Promise.all([
gMapLoader.importLibrary('maps'),
gMapLoader.importLibrary('marker'),
gMapLoader.importLibrary('geometry'),
]).then(resolve);
```
To fully apply the second part of your suggestion (dropping the extra `new Promise` and external `resolve`), you’ll need to:
1. Change the code that sets `gMapsApiLoading` to directly use the `Promise.all` chain (or a function that returns that chain), for example:
- Replace `gMapsApiLoading = new Promise((r) => (resolve = r));` with something like:
`gMapsApiLoading = Promise.all([...imports...]).then(() => undefined);`
2. Remove the `let resolve: () => void = () => null;` declaration and any other uses of `resolve` tied to this pattern.
3. Ensure that any callers that depend on `gMapsApiLoading` being a `Promise<void>` still work correctly (you may need to map the `Promise.all` result to `void` with `.then(() => undefined)` if the type matters).
These refactors depend on the surrounding context (how `gMapsApiLoading` and `resolve` are used elsewhere), so they should be adjusted to match the rest of the file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| ]).then((): void => { | ||
| resolve(); |
There was a problem hiding this comment.
suggestion: The extra Promise wrapper and explicit resolve in .then could be simplified
Since gMapsApiLoading already reflects the loader’s resolution, you can simplify this to .then(resolve) instead of wrapping resolve in a function. You might also be able to return Promise.all directly from loadGMaps and drop the extra new Promise if no other code relies on holding an external resolve reference.
Suggested implementation:
Promise.all([
gMapLoader.importLibrary('maps'),
gMapLoader.importLibrary('marker'),
gMapLoader.importLibrary('geometry'),
]).then(resolve);To fully apply the second part of your suggestion (dropping the extra new Promise and external resolve), you’ll need to:
- Change the code that sets
gMapsApiLoadingto directly use thePromise.allchain (or a function that returns that chain), for example:- Replace
gMapsApiLoading = new Promise((r) => (resolve = r));with something like:
gMapsApiLoading = Promise.all([...imports...]).then(() => undefined);
- Replace
- Remove the
let resolve: () => void = () => null;declaration and any other uses ofresolvetied to this pattern. - Ensure that any callers that depend on
gMapsApiLoadingbeing aPromise<void>still work correctly (you may need to map thePromise.allresult tovoidwith.then(() => undefined)if the type matters).
These refactors depend on the surrounding context (how gMapsApiLoading and resolve are used elsewhere), so they should be adjusted to match the rest of the file.
![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
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/fxc-front/src/app/components/2d/map-element.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Sourcery review
- GitHub Check: build (22.x)
- GitHub Check: Analyze (javascript)
- GitHub Check: Cloudflare Pages
| Promise.all([ | ||
| gMapLoader.importLibrary('maps'), | ||
| gMapLoader.importLibrary('marker'), | ||
| gMapLoader.importLibrary('geometry'), | ||
| ]).then((): void => { | ||
| resolve(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
head -100 apps/fxc-front/src/app/components/2d/map-element.ts | cat -nRepository: vicb/flyXC
Length of output: 4082
🏁 Script executed:
sed -n '40,80p' apps/fxc-front/src/app/components/2d/map-element.ts | cat -nRepository: vicb/flyXC
Length of output: 1544
🏁 Script executed:
rg -n "gMapsApiLoading" apps/fxc-front/src/app/components/2d/map-element.ts -A 2 -B 2Repository: vicb/flyXC
Length of output: 705
🏁 Script executed:
rg "@googlemaps/js-api-loader" --type jsonRepository: vicb/flyXC
Length of output: 358
🏁 Script executed:
rg -n "loadGMaps" apps/fxc-front/src/app/components/2d/map-element.ts -B 2 -A 2Repository: vicb/flyXC
Length of output: 371
🌐 Web query:
@googlemaps/js-api-loader version 2.0.2 importLibrary supported library names
💡 Result:
Supported importLibrary names (as of the Maps JS API / js-api-loader v2.x docs):
- core
- maps
- maps3d
- places
- geocoding
- routes
- marker
- geometry
- elevation
- streetview
- journeysharing
- drawing
- visualization
Sources: Google Maps JS API top-level reference and Libraries guide. [1][2]
References
[1] Google Maps JavaScript API — google.maps (top-level reference).
[2] Google Maps JavaScript API — Libraries (dynamic importLibrary guide).
🏁 Script executed:
sed -n '46,81p' apps/fxc-front/src/app/components/2d/map-element.ts | cat -nRepository: vicb/flyXC
Length of output: 1386
Add error handling to prevent app hang on library load failure.
The parallel loading approach is efficient and correct. However, if any of the importLibrary calls fails (e.g., network error, invalid API key, quota limits), the Promise.all() will reject and resolve() will never be called. This leaves gMapsApiLoading in a pending state indefinitely, causing the app to hang at the splash screen (where loadGMaps() is awaited on line 155).
Proposed fix to add error handling
Promise.all([
gMapLoader.importLibrary('maps'),
gMapLoader.importLibrary('marker'),
gMapLoader.importLibrary('geometry'),
-]).then((): void => {
- resolve();
-});
+])
+ .then((): void => {
+ resolve();
+ })
+ .catch((error) => {
+ console.error('Failed to load Google Maps libraries:', error);
+ resolve(); // Or handle the error appropriately
+ });All three library names ('maps', 'marker', 'geometry') are supported in @googlemaps/js-api-loader v2.0.2.
📝 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.
| Promise.all([ | |
| gMapLoader.importLibrary('maps'), | |
| gMapLoader.importLibrary('marker'), | |
| gMapLoader.importLibrary('geometry'), | |
| ]).then((): void => { | |
| resolve(); | |
| }); | |
| Promise.all([ | |
| gMapLoader.importLibrary('maps'), | |
| gMapLoader.importLibrary('marker'), | |
| gMapLoader.importLibrary('geometry'), | |
| ]) | |
| .then((): void => { | |
| resolve(); | |
| }) | |
| .catch((error) => { | |
| console.error('Failed to load Google Maps libraries:', error); | |
| resolve(); // Or handle the error appropriately | |
| }); |
🤖 Prompt for AI Agents
In apps/fxc-front/src/app/components/2d/map-element.ts around lines 57-63, the
Promise.all(...) call currently only handles the success path and never handles
rejection, which can leave gMapsApiLoading pending and hang the app; add error
handling by attaching a .catch handler to the Promise.all that logs the error
and rejects the surrounding promise (or otherwise resolves with a failure state)
so callers don't wait indefinitely; ensure the surrounding Promise executor
calls either resolve() on success or reject(err) on failure and include a clear
log message with the error details to aid debugging.
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.