Wait for the tiles to be loaded to display the map #341
Conversation
Reviewer's Guide by SourceryThis PR changes when the API loading state is set to false. Previously, it was set to false immediately after the map was initialized. Now, it waits for the map tiles to be loaded before setting the API loading state to false. Sequence diagram for map loading and tile initializationsequenceDiagram
participant MapElement
participant GoogleMaps
participant Store
MapElement->>GoogleMaps: Initialize map
Note over MapElement,GoogleMaps: Previously set loading=false here
GoogleMaps-->>MapElement: Map initialized
MapElement->>GoogleMaps: Add tilesloaded listener
GoogleMaps-->>MapElement: Tiles loaded
MapElement->>Store: dispatch(setApiLoading(false))
Note over MapElement,Store: Now waits for tiles to load
State diagram for map loading statesstateDiagram-v2
[*] --> Loading: Initialize map
Loading --> WaitingForTiles: Map initialized
WaitingForTiles --> Ready: Tiles loaded
Ready --> [*]: Set loading=false
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe pull request introduces modifications to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @vicb - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Deploying flyxc with
|
| Latest commit: |
9d62788
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://42727110.flyxc.pages.dev |
| Branch Preview URL: | https://vb-branch-1.flyxc.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/fxc-front/src/app/components/2d/map-element.ts (1)
242-244: Consider reordering cleanup operations.While the cleanup of map listeners is good, consider reordering the operations to ensure a more predictable cleanup sequence.
disconnectedCallback(): void { super.disconnectedCallback(); + if (this.map) { + google.maps.event.clearInstanceListeners(this.map); + } this.subscriptions.forEach((sub) => sub()); this.subscriptions.length = 0; - if (this.map) { - google.maps.event.clearInstanceListeners(this.map); - } this.map = undefined; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/fxc-front/src/app/components/2d/map-element.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: build (20.x)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
apps/fxc-front/src/app/components/2d/map-element.ts (1)
232-234: LGTM! Improved loading state management.The use of
tilesloadedevent ensures a better user experience by showing the map only when it's actually ready to be displayed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/fxc-front/src/app/components/2d/map-element.ts (2)
232-240: Good implementation of tile loading with timeout!The implementation properly handles tile loading with a timeout, which prevents the app from being stuck in a loading state if tiles fail to load. However, consider extracting the timeout duration to a constant for better maintainability.
+const TILE_LOADING_TIMEOUT_MS = 4000; + // Wait for the tiles to be loaded (or timeout) to hide the splashscreen. let splashResolve: () => void; const splashPromise = new Promise<void>((r) => (splashResolve = r)); -const to = setTimeout(() => splashResolve(), 4000); +const to = setTimeout(() => splashResolve(), TILE_LOADING_TIMEOUT_MS); google.maps.event.addListenerOnce(this.map, 'tilesloaded', () => { splashResolve(); clearTimeout(to); });🧰 Tools
🪛 Biome (1.9.4)
[error] 234-234: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
233-234: Improve Promise initialization pattern.The current implementation assigns the resolver in the Promise executor, which is flagged by static analysis. Consider using a cleaner pattern.
-let splashResolve: () => void; -const splashPromise = new Promise<void>((r) => (splashResolve = r)); +let splashResolve!: () => void; +const splashPromise = new Promise<void>((resolve) => { + splashResolve = resolve; +});🧰 Tools
🪛 Biome (1.9.4)
[error] 234-234: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/fxc-front/src/app/components/2d/map-element.ts(1 hunks)apps/fxc-front/tsconfig.json(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
apps/fxc-front/src/app/components/2d/map-element.ts
[error] 234-234: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (20.x)
- GitHub Check: Analyze (javascript)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
apps/fxc-front/tsconfig.json (1)
5-5: LGTM! Verify compatibility with dependencies.The update to ES2022 brings useful features that can enhance the codebase. However, ensure all dependencies support ES2022 features to prevent runtime issues.
Run this script to check package compatibility:
Also applies to: 8-8
✅ Verification successful
ES2022 compatibility verified across dependencies
No significant compatibility issues found that would prevent the use of ES2022. Most dependencies are modern and well-maintained, with good support for recent ECMAScript features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check package.json dependencies for ES2022 compatibility issues # Look for any reported compatibility issues in package.json # Find and check package.json files fd package.json --exec jq -r '.dependencies + .devDependencies | keys[]' {} | sort -u | while read pkg; do # Search for ES2022 compatibility issues in dependencies gh api graphql -f query=" query { search(query:\"$pkg ES2022 compatibility issue\", type:ISSUE, first:5) { nodes { ... on Issue { title url repository { nameWithOwner } labels(first:5) { nodes { name } } } } } } " doneLength of output: 64003
apps/fxc-front/src/app/components/2d/map-element.ts (1)
248-250: LGTM! Good cleanup of map event listeners.Proper cleanup of instance listeners in
disconnectedCallbackprevents memory leaks and ensures clean component unmounting.
Summary by Sourcery
Enhancements:
Summary by CodeRabbit
Bug Fixes
Performance
New Features