Defer dynamic style operations until the current style is fully loaded #24
Conversation
WalkthroughThe pull request introduces several modifications across multiple components in the MapLibre library, primarily focusing on ensuring that operations related to map properties and styles are executed only after the map's style is fully loaded. Key changes include the introduction of a private Changes
Assessment against linked issues
Possibly related PRs
🪧 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 (
|
ciscorn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (11)
src/lib/maplibre/map/Sprite.svelte (2)
18-20: LGTM! Consider adding error handling.The change to use
waitForStyleLoadedfor sprite addition aligns well with the PR objectives. However, sprite loading could potentially fail.Consider adding error handling:
mapCtx.waitForStyleLoaded((map) => { - map.addSprite(id, url); + try { + map.addSprite(id, url); + } catch (error) { + console.error(`Failed to add sprite ${id}:`, error); + } });
23-25: LGTM! Consider enhancing type safety.The cleanup function correctly uses
waitForStyleLoadedto ensure the sprite is removed only after the style is loaded.Consider adding type annotations to enhance maintainability:
-mapCtx.waitForStyleLoaded((map) => { +mapCtx.waitForStyleLoaded((map: maplibregl.Map) => { map.removeSprite(id); });src/lib/maplibre/map/Light.svelte (2)
15-17: Consider adding error handling for style loading failures.The implementation correctly defers the light setting operation until the style is loaded, which aligns with the PR objectives. However, it might benefit from explicit error handling.
Consider adding error handling:
mapCtx.waitForStyleLoaded((map) => { map.setLight(mapCtx.userLight as maplibregl.LightSpecification); +}).catch((error) => { + console.error('Failed to set light properties:', error); });
15-24: Consider implementing a centralized style loading state management.While the current implementation successfully defers style operations, consider implementing a centralized loading state management system that could:
- Provide visual feedback during style loading
- Batch multiple style operations together
- Handle cleanup more gracefully during unmounting
This would improve the overall user experience and make the system more maintainable.
src/lib/maplibre/map/Terrain.svelte (3)
24-26: LGTM! Properly defers terrain operations.The implementation correctly addresses the core issue by ensuring terrain operations only execute after the style is loaded.
The
mapparameter is unused in the callback. Consider removing it:-mapCtx.waitForStyleLoaded((map) => { +mapCtx.waitForStyleLoaded(() => {🧰 Tools
🪛 eslint (1.23.1)
[error] 24-24: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
31-33: LGTM! Cleanup properly handles style loading state.The cleanup logic correctly defers resetting terrain until the style is loaded.
Similarly, the
mapparameter is unused here. Consider removing it:-mapCtx.waitForStyleLoaded((map) => { +mapCtx.waitForStyleLoaded(() => {🧰 Tools
🪛 eslint (1.23.1)
[error] 31-31: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
24-33: Consider strengthening type safety.While the current implementation works, we could improve type safety around terrain specifications.
Consider adding type guards or assertions:
mapCtx.waitForStyleLoaded(() => { - mapCtx.map?.setTerrain((mapCtx.userTerrain as maplibregl.TerrainSpecification) || null); + const terrain = mapCtx.userTerrain; + if (terrain && typeof terrain === 'object' && 'source' in terrain) { + mapCtx.map?.setTerrain(terrain); + } else { + mapCtx.map?.setTerrain(null); + } });🧰 Tools
🪛 eslint (1.23.1)
[error] 24-24: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 31-31: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/lib/maplibre/sources/RawSource.svelte (1)
Line range hint
41-119: Consider refactoring effect blocks for better maintainabilityThe current implementation has multiple effect blocks with complex conditional logic for different source types. Consider:
- Extracting source-type-specific logic into separate functions or components
- Implementing a more robust state machine for handling the initialization sequence
- Adding error boundaries for better error handling
This would improve maintainability and make the code more testable.
Would you like me to provide a detailed refactoring proposal?
🧰 Tools
🪛 eslint (1.23.1)
[error] 120-120: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
src/lib/maplibre/map/MapLibre.svelte (1)
206-232: Consider refactoring property updates to reduce repetitionWhile the move event handler is functionally correct, the property update pattern is repeated for each transform property. Consider extracting this pattern into a helper function to improve maintainability.
+ function updateTransformProperty<T>(current: T | undefined, newValue: T, setter: (value: T) => void) { + if (current !== newValue) { + setter(newValue); + } + } map.on('move', () => { if (!map) { return; } const tr = map.transform; if (center) { const _center = maplibregl.LngLat.convert(center); if (_center.lat !== tr.center.lat || _center.lng !== tr.center.lng) { center = formatLngLat(center, tr.center); } } else { center = tr.center; } - if (tr.zoom !== zoom) { - zoom = tr.zoom; - } - if (tr.bearing !== bearing) { - bearing = tr.bearing; - } - if (tr.pitch !== pitch) { - pitch = tr.pitch; - } - if (tr.roll !== roll) { - roll = tr.roll; - } - if (tr.elevation !== elevation) { - elevation = tr.elevation; - } + updateTransformProperty(zoom, tr.zoom, z => zoom = z); + updateTransformProperty(bearing, tr.bearing, b => bearing = b); + updateTransformProperty(pitch, tr.pitch, p => pitch = p); + updateTransformProperty(roll, tr.roll, r => roll = r); + updateTransformProperty(elevation, tr.elevation, e => elevation = e); });src/lib/maplibre/layers/RawLayer.svelte (2)
84-84: Consider using a more idiomatic approach instead offirstRunThe variable
firstRunis introduced and initialized totrueto control the execution of effects after the initial render. While this works, using Svelte's lifecycle methods might provide a cleaner solution.Consider using the
onMountfunction to handle initialization logic:+import { onMount } from 'svelte'; let firstRun = true; -$effect(() => { - firstRun = false; -}); + +onMount(() => { + firstRun = false; +});This approach enhances clarity by explicitly handling component initialization.
104-118: Refactor repeated$effectblocks to reduce code duplicationMultiple
$effectblocks share a similar structure for updatingpaint,layout,filter, and other properties. Refactoring these into a generic function could improve maintainability and reduce redundancy.For example, you could create a utility function:
function updateProperties(propertyName, prevProps, props, updateFunc) { if (!firstRun) { mapCtx.waitForStyleLoaded((map) => { const keysRemoved = new Set(Object.keys(prevProps)); const _props = $state.snapshot(props) ?? {}; for (const [key, value] of Object.entries(_props)) { keysRemoved.delete(key); if (prevProps[key] !== value) { updateFunc(map, id, key, value); } } for (const key of keysRemoved) { updateFunc(map, id, key, undefined); } prevProps = _props; }); } }Then use it in your
$effectblocks:$effect(() => { - paint; - if (!firstRun) { - mapCtx.waitForStyleLoaded((map) => { - // existing code - }); - } + updateProperties('paint', prevPaint, paint, (map, id, key, value) => { + map.setPaintProperty(id, key, value); + }); });This refactoring enhances code readability and reduces the potential for errors.
Also applies to: 126-139, 145-147, 154-156, 162-164
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
src/lib/maplibre/contexts.svelte.ts(2 hunks)src/lib/maplibre/layers/RawLayer.svelte(2 hunks)src/lib/maplibre/map/Light.svelte(1 hunks)src/lib/maplibre/map/MapLibre.svelte(2 hunks)src/lib/maplibre/map/Projection.svelte(1 hunks)src/lib/maplibre/map/Sky.svelte(1 hunks)src/lib/maplibre/map/Sprite.svelte(1 hunks)src/lib/maplibre/map/Terrain.svelte(1 hunks)src/lib/maplibre/sources/RawSource.svelte(3 hunks)src/routes/+layout.server.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/routes/+layout.server.ts
🧰 Additional context used
🪛 eslint (1.23.1)
src/lib/maplibre/layers/RawLayer.svelte
[error] 124-124: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
[error] 152-152: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
src/lib/maplibre/map/Projection.svelte
[error] 23-23: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/lib/maplibre/map/Terrain.svelte
[error] 24-24: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 31-31: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/lib/maplibre/sources/RawSource.svelte
[error] 120-120: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
🔇 Additional comments (17)
src/lib/maplibre/map/Light.svelte (1)
22-24: Consider immediate cleanup with style-loaded fallback.
The implementation safely resets light settings after waiting for style load. However, for cleanup operations, consider whether an immediate reset with a fallback mechanism might be more appropriate.
Let's verify if other components follow the same cleanup pattern:
src/lib/maplibre/map/Sky.svelte (2)
15-17: LGTM! Properly defers sky updates until style is loaded.
This change aligns perfectly with the PR objectives by ensuring that setSky operations only occur after the map style is fully loaded, preventing potential failures during initialization.
22-24: LGTM! Thorough cleanup implementation.
Good attention to detail in also deferring the cleanup operation. This ensures that reverting to the base sky settings is handled safely, maintaining consistency with the new style loading pattern.
src/lib/maplibre/map/Projection.svelte (1)
16-18: LGTM! Good improvement in projection handling.
The change correctly implements the deferral of projection setting until the map style is loaded, which aligns with the PR objectives and prevents potential race conditions.
src/lib/maplibre/sources/RawSource.svelte (2)
130-130: LGTM: Simplified rendering logic
The unconditional rendering of children is a good improvement. It simplifies the code and allows child components to handle their own loading states.
36-39:
Review firstRun flag initialization logic
The firstRun flag is set to true after source initialization, which might prevent the effect blocks from running their update logic. Consider setting it to false here instead, as the source is now ready for updates.
mapCtx.waitForStyleLoaded((map) => {
source = map.getSource(id);
- firstRun = true;
+ firstRun = false;
});src/lib/maplibre/map/MapLibre.svelte (3)
200-200: LGTM: Early map context initialization improves component coordination
Setting mapCtx.map immediately after map creation allows other components to access the map instance earlier and implement style loading checks.
202-204: LGTM: Simplified load event handler
The load event handler now focuses solely on setting the loaded state, as map context initialization has been moved earlier. This separation of concerns improves code clarity.
546-548: Verify child components implement style loading checks
The template now renders children as soon as the map instance is available, rather than waiting for the load event. Ensure all child components properly implement waitForStyleLoaded before performing style operations.
✅ Verification successful
Let me analyze the components that might need style loading checks more thoroughly.
Child components correctly implement style loading checks
The verification shows that all style operations are properly wrapped with waitForStyleLoaded:
- Source components use
waitForStyleLoadedbefore accessing map style - Layer components consistently use
waitForStyleLoadedfor all style operations (paint, layout, zoom, filter) - All map style operations (
setPaintProperty,setLayoutProperty,addLayer,removeLayer) are properly handled through the context which enforceswaitForStyleLoaded
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if child components properly implement style loading checks
# Look for direct style operations without waitForStyleLoaded
rg -l "(?:setPaintProperty|setLayoutProperty|addLayer|removeLayer|setStyle)" | \
xargs rg -L "waitForStyleLoaded"
# Look for components that might need style loading checks
fd -e svelte | xargs rg -l "(?:style|layer|source|paint|layout)"
Length of output: 2898
Script:
#!/bin/bash
# Check implementation details of source and layer components
# to verify they properly handle style loading
# Check source components
fd -e svelte -p "sources/" | xargs rg "waitForStyleLoaded|onMount|beforeUpdate" -C 2
# Check layer components
fd -e svelte -p "layers/" | xargs rg "waitForStyleLoaded|onMount|beforeUpdate" -C 2
# Check if there are any direct map style operations without waiting
rg "map\.(setPaintProperty|setLayoutProperty|addLayer|removeLayer|setStyle)" -C 2
Length of output: 4168
src/lib/maplibre/layers/RawLayer.svelte (2)
127-127:
Remove unused expression layout; to fix ESLint error
On line 127, the expression layout; is causing a linting error: "Expected an assignment or function call and instead saw an expression." Since the expression has no side effects, it triggers the linting rule.
Apply the following diff to correct the issue:
$effect(() => {
- layout;
if (!firstRun) {
mapCtx.waitForStyleLoaded((map) => {
// existing code
});
}
});To ensure that the effect runs when layout changes, you might consider referencing layout within the effect in a way that satisfies the linter, such as including it in a conditional or utilizing it directly.
Likely invalid or redundant comment.
105-105:
Remove unused expression paint; to fix ESLint error
The expression paint; on line 105 is causing a static analysis error: "Expected an assignment or function call and instead saw an expression." This occurs because the expression is evaluated but not used, leading to a linting issue.
Apply the following diff to remove the unused expression:
$effect(() => {
- paint;
if (!firstRun) {
mapCtx.waitForStyleLoaded((map) => {
// existing code
});
}
});If the intention is to ensure the $effect reacts to changes in paint, consider referencing paint within the effect in a way that avoids the unused expression error. For example, you could use it in a comment or restructure the effect to depend on paint.
Likely invalid or redundant comment.
src/lib/maplibre/contexts.svelte.ts (6)
47-61: Event listener management in map getter and setter is correctly implemented
The getter and setter for the map property properly handle adding and removing event listeners when the map instance is set or changed, ensuring that the _onstyledata listener is correctly registered and deregistered.
64-68: Proper error handling and deferred execution in addLayer method
The addLayer method correctly checks if the map is initialized and utilizes waitForStyleLoaded to defer the addition of the layer until the style is fully loaded, preventing potential errors.
71-75: Correct usage of waitForStyleLoaded in removeLayer method
The removeLayer method appropriately defers the layer removal until the style is loaded, ensuring safe operation.
80-82: Deferred execution in addSource method is well-implemented
The addSource method correctly adds the source after the style has loaded by using waitForStyleLoaded.
86-89: Proper deferral in removeSource method
The removeSource method ensures the source is removed only after the style is fully loaded, preventing potential issues.
22-22:
Invalid use of $state.raw(null) for class property initialization
The use of $state.raw(null) is not a standard or documented Svelte API. In Svelte, $ is used for reactive statements within components, not for initializing class properties. You should initialize _map directly to null or use Svelte stores if reactivity is required.
Apply this diff to correct the initialization:
- _map: MapLibre | null = $state.raw(null);
+ _map: MapLibre | null = null;Likely invalid or redundant comment.
ciscorn
force-pushed
the
defer-until-style-loaded
branch
from
310b42b to
d6e788b
Compare
Deploying svelte-maplibre-gl with
|
| Latest commit: |
2960500
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e376d153.svelte-maplibre-gl.pages.dev |
| Branch Preview URL: | https://defer-until-style-loaded.svelte-maplibre-gl.pages.dev |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (10)
src/lib/maplibre/map/Terrain.svelte (2)
24-26: Simplify the callback and remove redundant null checks.The implementation correctly defers terrain setting until style is loaded, but can be improved:
- The
mapparameter is unused and can be removed- The null check is redundant since we validate map existence at initialization
Consider this improvement:
- mapCtx.waitForStyleLoaded((map) => { - mapCtx.map?.setTerrain((mapCtx.userTerrain as maplibregl.TerrainSpecification) || null); - }); + mapCtx.waitForStyleLoaded(() => { + mapCtx.map.setTerrain((mapCtx.userTerrain as maplibregl.TerrainSpecification) || null); + });🧰 Tools
🪛 eslint (1.23.1)
[error] 24-24: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
31-33: Apply similar simplifications to the cleanup code.The cleanup logic is correct, but can be improved in the same way as the previous segment.
Consider this improvement:
- mapCtx.waitForStyleLoaded((map) => { - mapCtx.map?.setTerrain(mapCtx.baseTerrain ?? null); - }); + mapCtx.waitForStyleLoaded(() => { + mapCtx.map.setTerrain(mapCtx.baseTerrain ?? null); + });🧰 Tools
🪛 eslint (1.23.1)
[error] 31-31: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/lib/maplibre/layers/RawLayer.svelte (5)
84-87: Remove unused parameter in callbackThe
mapparameter in the callback is unused. SincemapCtx.addLayeris used instead of direct map operations, we can remove the parameter.-mapCtx.waitForStyleLoaded((map) => { +mapCtx.waitForStyleLoaded(() => { mapCtx.addLayer(addLayerObj, beforeId); });🧰 Tools
🪛 eslint (1.23.1)
[error] 85-85: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
106-120: Consider extracting property update logicThe property update logic is duplicated between paint and layout effects. Consider extracting this into a reusable helper function to improve maintainability and reduce code duplication.
+function updateProperties( + map: maplibregl.Map, + layerId: string, + newProps: Record<string, unknown>, + prevProps: Record<string, unknown>, + setter: (id: string, key: string, value: unknown) => void +) { + const keysRemoved = new Set(Object.keys(prevProps)); + for (const [key, value] of Object.entries(newProps)) { + keysRemoved.delete(key); + if (prevProps[key] !== value) { + setter(layerId, key, value); + } + } + for (const key of keysRemoved) { + setter(layerId, key, undefined); + } + return newProps; +} $effect(() => { paint; if (!firstRun) { mapCtx.waitForStyleLoaded((map) => { - const keysRemoved = new Set(Object.keys(prevPaint)); - const _paint = $state.snapshot(paint) ?? {}; - for (const [key, value] of Object.entries(_paint)) { - keysRemoved.delete(key); - if (prevPaint[key] !== value) { - map.setPaintProperty(id, key, value); - } - } - for (const key of keysRemoved) { - map.setPaintProperty(id, key, undefined); - } - prevPaint = _paint; + const _paint = $state.snapshot(paint) ?? {}; + prevPaint = updateProperties( + map, + id, + _paint, + prevPaint, + map.setPaintProperty.bind(map) + ); }); } });
126-126: Fix ESLint warning about unused expressionThe
layout;expression is needed for reactivity but triggers an ESLint warning. Consider using the value explicitly.$effect(() => { - layout; + const _ = layout; // Explicit usage for reactivity if (!firstRun) {🧰 Tools
🪛 eslint (1.23.1)
[error] 126-126: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
164-166: Remove unnecessary optional chainingThe optional chaining operator (
?.) onmapis unnecessary sincewaitForStyleLoadedguarantees that the map instance exists.mapCtx.waitForStyleLoaded((map) => { - map?.moveLayer(id, beforeId); + map.moveLayer(id, beforeId); });
Line range hint
84-166: Implementation successfully addresses the PR objectiveThe changes effectively implement the deferral of style operations until the map style is fully loaded. The use of
firstRunflag andwaitForStyleLoadedensures that operations are executed at the appropriate time, preventing potential failures.Consider implementing error handling within the
waitForStyleLoadedcallbacks to gracefully handle any failures during style updates.🧰 Tools
🪛 eslint (1.23.1)
[error] 105-105: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
[error] 126-126: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
[error] 154-154: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
src/lib/maplibre/contexts.svelte.ts (2)
Line range hint
22-61: LGTM! Consider adding null check in the event listener cleanup.The encapsulation of the map property with proper event listener management is well implemented. However, there's a minor potential for a null reference when cleaning up the event listener.
Consider this small safety improvement:
set map(value: maplibregl.Map | null) { if (this._listener) { - this._map?.off('styledata', this._listener); + if (this._map) { + this._map.off('styledata', this._listener); + } this._listener = undefined; }
105-113: Consider adding error handling for pending operations.While the implementation is correct, consider adding try-catch blocks to prevent a single failing operation from blocking subsequent ones.
Consider this improvement:
private _onstyledata(ev: maplibregl.MapStyleDataEvent) { const map = ev.target; if (map?.style._loaded) { for (const func of this._pending) { - // call pending tasks - func(map); + try { + func(map); + } catch (error) { + console.error('Error executing pending map operation:', error); + } } this._pending = []; } }src/lib/maplibre/map/MapLibre.svelte (1)
206-232: Consider optimizing property updates in move event handler.The move event handler is well-structured, but could be optimized to reduce property comparisons and updates.
Consider batching the property updates:
- if (tr.zoom !== zoom) { - zoom = tr.zoom; - } - if (tr.bearing !== bearing) { - bearing = tr.bearing; - } - if (tr.pitch !== pitch) { - pitch = tr.pitch; - } - if (tr.roll !== roll) { - roll = tr.roll; - } - if (tr.elevation !== elevation) { - elevation = tr.elevation; - } + const updates = { + zoom: tr.zoom, + bearing: tr.bearing, + pitch: tr.pitch, + roll: tr.roll, + elevation: tr.elevation + }; + + Object.entries(updates).forEach(([prop, value]) => { + if (value !== this[prop]) { + this[prop] = value; + } + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
src/content/examples/terrain/content.svelte.md(1 hunks)src/lib/maplibre/contexts.svelte.ts(2 hunks)src/lib/maplibre/layers/RawLayer.svelte(2 hunks)src/lib/maplibre/map/Light.svelte(1 hunks)src/lib/maplibre/map/MapLibre.svelte(2 hunks)src/lib/maplibre/map/Projection.svelte(1 hunks)src/lib/maplibre/map/Sky.svelte(1 hunks)src/lib/maplibre/map/Sprite.svelte(1 hunks)src/lib/maplibre/map/Terrain.svelte(1 hunks)src/lib/maplibre/sources/RawSource.svelte(3 hunks)src/routes/+layout.server.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/content/examples/terrain/content.svelte.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/lib/maplibre/map/Light.svelte
- src/lib/maplibre/map/Sky.svelte
- src/lib/maplibre/map/Sprite.svelte
- src/routes/+layout.server.ts
🧰 Additional context used
📓 Learnings (1)
src/lib/maplibre/contexts.svelte.ts (1)
Learnt from: ciscorn
PR: MIERUNE/svelte-maplibre-gl#24
File: src/lib/maplibre/contexts.svelte.ts:91-103
Timestamp: 2024-11-22T03:00:21.632Z
Learning: In MapLibre GL, `map.isStyleLoaded()` may not be unreliable due to known issues (e.g., https://github.com/mapbox/mapbox-gl-js/issues/8691). It's acceptable to access `map.style._loaded` directly to check if the style is loaded.
🪛 eslint (1.23.1)
src/lib/maplibre/layers/RawLayer.svelte
[error] 85-85: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 126-126: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
[error] 154-154: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
src/lib/maplibre/map/Projection.svelte
[error] 23-23: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/lib/maplibre/map/Terrain.svelte
[error] 24-24: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 31-31: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/lib/maplibre/sources/RawSource.svelte
[error] 120-120: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
🔇 Additional comments (11)
src/lib/maplibre/map/Projection.svelte (2)
16-18: LGTM! Properly defers projection update until style is loaded.
The implementation correctly uses waitForStyleLoaded to ensure the projection is set only after the style is fully loaded, aligning with the PR's objectives.
23-25:
Previous review suggestions not addressed.
The code still uses mapCtx.map?.setProjection instead of the provided map parameter, and the optional chaining is unnecessary since we have the map parameter from waitForStyleLoaded.
Apply this diff to fix the issues:
mapCtx.waitForStyleLoaded((map) => {
- mapCtx.map?.setProjection(mapCtx.baseProjection ?? { type: 'mercator' });
+ map.setProjection(mapCtx.baseProjection ?? { type: 'mercator' });
});🧰 Tools
🪛 eslint (1.23.1)
[error] 23-23: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/lib/maplibre/map/Terrain.svelte (1)
24-33: Implementation successfully addresses style loading concerns.
The changes effectively implement the deferral of terrain operations until the style is loaded, aligning perfectly with the PR objectives. The implementation is robust, handling both initial setup and cleanup appropriately.
🧰 Tools
🪛 eslint (1.23.1)
[error] 24-24: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 31-31: 'map' is defined but never used.
(@typescript-eslint/no-unused-vars)
src/lib/maplibre/sources/RawSource.svelte (3)
120-122: Previous review comment is still applicable
The current implementation has the same issues as noted in the previous review:
- The bare
sourceexpression triggers a linting error - Setting
firstRunhere might race with the initialization logic
🧰 Tools
🪛 eslint (1.23.1)
[error] 120-120: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
130-130: LGTM! Improved children rendering
The unconditional rendering of children is correct since source-dependent operations are properly handled in the effects.
Line range hint 108-114: Track MapLibre fix for clusterRadius
The current implementation uses a workaround to set clusterRadius through workerOptions due to a limitation in maplibre-gl-js. Consider:
- Adding a link to the MapLibre issue in the TODO comment
- Creating a GitHub issue to track when this can be simplified after the MapLibre fix
src/lib/maplibre/contexts.svelte.ts (3)
64-89: LGTM! Robust error handling and proper style loading checks.
The implementation correctly ensures that:
- Map instance exists before operations
- Layer/source operations are deferred until style is loaded
- Set management (add/remove tracking) happens immediately
91-103: LGTM! Correct implementation of style loading check.
The implementation correctly uses map.style._loaded instead of the unreliable isStyleLoaded() method, as documented in mapbox-gl-js#8691. The function properly handles both immediate execution and queuing of operations.
Line range hint 22-113: Verify the implementation in dependent components.
The changes successfully implement the deferral of dynamic style operations. To ensure complete coverage, let's verify the implementation in dependent components.
✅ Verification successful
Implementation is correctly used across all dependent components
The verification shows that all components using style-related operations (setTerrain, setSky, setProjection, setLight, setPaintProperty, setLayoutProperty) are already accessing these through the MapContext. These components (Terrain.svelte, Sky.svelte, Projection.svelte, Light.svelte, RawLayer.svelte) properly obtain the map context using getMapContext(), which ensures they inherit the style loading deferral mechanism implemented in the context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all components that might need to use waitForStyleLoaded
# Look for components that directly use MapContext
echo "Components using MapContext:"
rg "getMapContext|MapContext" --type svelte
# Look for components that might be setting style properties
echo -e "\nComponents potentially setting style properties:"
rg "setLayoutProperty|setPaintProperty|setLight|setTerrain|setSky|setProjection" --type svelte
Length of output: 384
Script:
#!/bin/bash
# Description: Find all components that might need to use waitForStyleLoaded
# Look for components that directly use MapContext
echo "Components using MapContext:"
rg "getMapContext|MapContext"
# Look for components that might be setting style properties
echo -e "\nComponents potentially setting style properties:"
rg "setLayoutProperty|setPaintProperty|setLight|setTerrain|setSky|setProjection"
# Look for files importing from contexts.svelte.ts
echo -e "\nFiles importing from contexts.svelte.ts:"
rg "from ['\"].*contexts\.svelte['\"]"
Length of output: 6054
src/lib/maplibre/map/MapLibre.svelte (2)
546-548: Verify that child components handle style loading state.
The condition for rendering children has been simplified to only check for map existence. While this aligns with the PR objective, we should verify that child components properly defer their style operations.
Let's check if child components use waitForStyleLoaded:
✅ Verification successful
Let me gather more information to verify if there are any other components that might interact with the map style.
Child components safely handle style loading state
Based on the analysis of the codebase:
- RawLayer component consistently uses
waitForStyleLoadedfor all style operations (paint and layout properties) - RawSource component properly uses
waitForStyleLoadedbefore adding sources - Other components (Sky, Light, Projection, etc.) don't directly interact with style operations
- The MapLibre component provides the style loading event handlers that child components can utilize
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for child components that might perform style operations
fd -e svelte | xargs rg "setPaintProperty|setLayoutProperty" -l | while read -r file; do
echo "Checking $file for waitForStyleLoaded usage:"
rg "waitForStyleLoaded" "$file" || echo "WARNING: No waitForStyleLoaded found in $file"
done
Length of output: 564
Script:
#!/bin/bash
# Search for components that use map context and might interact with styles
fd -e svelte | xargs rg "getMap\(\)|map\." -l | while read -r file; do
echo "=== $file ==="
# Show style-related operations
rg "style|layer|paint|layout|source" "$file" -A 2
done
Length of output: 7918
200-200: LGTM: Early map context initialization improves reliability.
Setting mapCtx.map immediately after map creation ensures that the map context is available as early as possible for style operations.
Let's verify that no style operations occur before this initialization:
✅ Verification successful
Let me gather more context about the map initialization and style operations to ensure a thorough verification.
Let me check one more aspect to understand how style operations are protected.
LGTM: Map initialization and style operations are properly synchronized
The early map context initialization at line 200 is safe because:
- All style operations are wrapped in
waitForStyleLoadedwhich ensures the style is loaded before execution - The implementation of
waitForStyleLoadedchecks both map existence and style load state - Style operations are queued in
_pendingif the style isn't loaded yet
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any style operations that might execute before map initialization
rg -l "setPaintProperty|setLayoutProperty" | xargs rg "map\." -B 5 -A 5
Length of output: 1276
Script:
#!/bin/bash
# Search for waitForStyleLoaded implementation and usage
ast-grep --pattern 'waitForStyleLoaded' | head -n 20
# Search for map context initialization
rg "mapCtx\.map\s*=" -B 5 -A 5
Length of output: 1212
Script:
#!/bin/bash
# Search for the implementation of waitForStyleLoaded to understand how it protects style operations
rg "waitForStyleLoaded.*=.*" -A 10
Length of output: 10939
ciscorn
force-pushed
the
defer-until-style-loaded
branch
from
d6e788b to
e1c0328
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (10)
src/content/CodeBlock.svelte (3)
Line range hint
1-10: Consider async initialization and flexible configuration.The current synchronous initialization could block the main thread. Consider these improvements:
- Use
createHighlighterCore()(async) instead ofcreateHighlighterCoreSync()- Support multiple themes/languages through component props
- Document the rationale for using the regex engine over oniguruma
Example implementation:
<script lang="ts" module> - import { createHighlighterCoreSync, createJavaScriptRegexEngine } from 'shiki'; + import { createHighlighterCore, createJavaScriptRegexEngine } from 'shiki'; import svelte from 'shiki/langs/svelte.mjs'; import dark from 'shiki/themes/github-dark-default.mjs'; - export const shiki = createHighlighterCoreSync({ + // Using regex engine for better browser compatibility and smaller bundle size + export const shiki = await createHighlighterCore({ themes: [dark], langs: [svelte], engine: createJavaScriptRegexEngine() }); </script>
Line range hint
12-20: Make language and theme configurable via props.The component could be more reusable by accepting language and theme as props instead of hard-coding them.
Example implementation:
<script lang="ts"> let { - content + content, + lang = 'svelte', + theme = 'github-dark-default' }: { content: string; + lang?: string; + theme?: string; } = $props(); const highlighted = $derived.by(() => { - return shiki.codeToHtml(content.trim(), { lang: 'svelte', theme: 'github-dark-default' }); + return shiki.codeToHtml(content.trim(), { lang, theme }); }); </script>
26-26: Make ESLint directive more specific and consider scoping styles.The ESLint disable comment is correctly used, but could be more specific. Also, consider scoping the styles to prevent conflicts.
- Make the ESLint comment more specific:
- <!-- eslint-disable-next-line svelte/no-at-html-tags --> + <!-- eslint-disable-next-line svelte/no-at-html-tags -- Safe since content is sanitized by Shiki -->
- Consider scoping the styles to prevent conflicts with other code blocks:
<style> - :global(.shiki code) { + :global(.codeblock-content .shiki code) {And update the template:
-<div class="my-6 subpixel-antialiased"> +<div class="my-6 subpixel-antialiased codeblock-content">src/lib/maplibre/controls/GeolocateControl.svelte (1)
10-10: LGTM! Consider adding JSDoc comment.The type change from
Objecttoobjectis correct and follows TypeScript best practices. Consider adding a JSDoc comment to document the purpose of this type.+/** Event type for GeolocateControl events that includes both MapLibre event properties and standard object properties */ type GeolocateEvent = Event<maplibregl.GeolocateControl> & object;src/lib/maplibre/layers/RawLayer.svelte (4)
106-120: Consider adding type safety for paint propertiesThe implementation correctly defers paint updates and handles property changes efficiently. Consider adding type safety by using MapLibre's type definitions:
-const _paint = $state.snapshot(paint) ?? {}; +const _paint = $state.snapshot(paint) ?? {} as maplibregl.LayerSpecification['paint'];This would help catch potential paint property type mismatches at compile time.
127-141: Fix unused expression warning while maintaining reactivityThe layout update implementation is correct, but there's a static analysis warning about the unused
layoutexpression.$effect(() => { - layout; + const currentLayout = layout; if (!firstRun) { mapCtx.waitForStyleLoaded((map) => { - const _layout = $state.snapshot(layout) ?? {}; + const _layout = $state.snapshot(currentLayout) ?? {}; // ... rest of the code }); } });
156-158: Fix unused expression warning in filter effectSimilar to the layout effect, there's a static analysis warning about the unused
filterexpression.$effect(() => { - filter; + const currentFilter = filter; if (!firstRun) { mapCtx.waitForStyleLoaded((map) => { - map.setFilter(id, filter); + map.setFilter(id, currentFilter); }); } });
Line range hint
84-166: Consider adding error handling for map operationsWhile the implementation successfully defers operations until the style is loaded, consider adding error handling for map operations to improve robustness:
mapCtx.waitForStyleLoaded((map) => { - map.setPaintProperty(id, key, value); + try { + map.setPaintProperty(id, key, value); + } catch (error) { + console.error(`Failed to set paint property ${key} for layer ${id}:`, error); + } });This pattern could be applied to all map operations to ensure graceful handling of potential runtime errors.
🧰 Tools
🪛 eslint (1.23.1)
[error] 105-105: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
[error] 126-126: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
[error] 154-154: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
src/lib/maplibre/contexts.svelte.ts (2)
47-61: LGTM! Well-implemented getter/setter with proper event managementThe implementation correctly manages the event listener lifecycle. One minor suggestion to improve null safety:
- this._map?.on('styledata', this._listener); + if (this._map) { + this._map.on('styledata', this._listener); + }
64-89: Standardize null checking pattern across methodsThe implementation correctly defers operations until style is loaded. However, there's an inconsistency in null checking:
addSource(id: string, source: SourceSpecification | CanvasSourceSpecification) { this.userSources.add(id); this.waitForStyleLoaded((map) => { - map?.addSource(id, source); + map.addSource(id, source); }); } removeSource(id: string) { this.userSources.delete(id); this.waitForStyleLoaded((map) => { - map?.removeSource(id); + map.removeSource(id); }); }The
mapparameter in the callback will never be null sincewaitForStyleLoadedonly executes the callback when the map is available.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (16)
src/content/CodeBlock.svelte(1 hunks)src/content/examples/terrain/content.svelte.md(1 hunks)src/lib/maplibre/contexts.svelte.ts(2 hunks)src/lib/maplibre/controls/GeolocateControl.svelte(2 hunks)src/lib/maplibre/controls/Hash.svelte(1 hunks)src/lib/maplibre/layers/RawLayer.svelte(2 hunks)src/lib/maplibre/map/Light.svelte(1 hunks)src/lib/maplibre/map/MapLibre.svelte(2 hunks)src/lib/maplibre/map/Projection.svelte(1 hunks)src/lib/maplibre/map/Sky.svelte(1 hunks)src/lib/maplibre/map/Sprite.svelte(1 hunks)src/lib/maplibre/map/Terrain.svelte(1 hunks)src/lib/maplibre/sources/FeatureState.svelte(1 hunks)src/lib/maplibre/sources/RawSource.svelte(3 hunks)src/routes/+layout.server.ts(1 hunks)src/routes/Header.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/content/examples/terrain/content.svelte.md
- src/lib/maplibre/map/Light.svelte
- src/lib/maplibre/map/MapLibre.svelte
- src/lib/maplibre/map/Projection.svelte
- src/lib/maplibre/map/Sky.svelte
- src/lib/maplibre/map/Sprite.svelte
- src/lib/maplibre/map/Terrain.svelte
- src/routes/+layout.server.ts
🧰 Additional context used
📓 Learnings (1)
src/lib/maplibre/contexts.svelte.ts (1)
Learnt from: ciscorn
PR: MIERUNE/svelte-maplibre-gl#24
File: src/lib/maplibre/contexts.svelte.ts:91-103
Timestamp: 2024-11-22T03:00:21.632Z
Learning: In MapLibre GL, `map.isStyleLoaded()` may not be unreliable due to known issues (e.g., https://github.com/mapbox/mapbox-gl-js/issues/8691). It's acceptable to access `map.style._loaded` directly to check if the style is loaded.
🪛 eslint (1.23.1)
src/lib/maplibre/layers/RawLayer.svelte
[error] 126-126: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
[error] 154-154: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
src/lib/maplibre/sources/RawSource.svelte
[error] 120-120: Expected an assignment or function call and instead saw an expression.
(@typescript-eslint/no-unused-expressions)
🪛 Gitleaks (8.21.1)
src/routes/Header.svelte
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (15)
src/lib/maplibre/controls/Hash.svelte (3)
10-10: LGTM: Clean removal of unused props destructuring
The removal of the empty props destructuring improves code cleanliness by eliminating dead code.
Line range hint 13-19: LGTM: Map initialization and Hash setup
The implementation correctly:
- Verifies map initialization before proceeding
- Sets up the Hash control
- Updates the hash if needed
Since Hash control doesn't modify map styles, it doesn't need to wait for style loading, making this implementation appropriate within the context of the PR's style loading improvements.
Line range hint 3-3: Verify SvelteKit compatibility issues
The FIXME comment suggests potential compatibility issues with SvelteKit. This should be investigated to ensure proper integration with SvelteKit's routing system.
src/content/CodeBlock.svelte (1)
Line range hint 1-43: Verify the inclusion of CodeBlock.svelte changes in this PR.
The changes to this file appear to be unrelated to the PR's main objective of "Deferring dynamic style operations until the current style is fully loaded." Please confirm if these changes should be part of a separate PR for better change tracking and review focus.
src/lib/maplibre/sources/FeatureState.svelte (2)
10-10: Good type safety improvement!
Changing from Record<string, any> to Record<string, unknown> enhances type safety by requiring explicit type checking before using state values, which helps prevent runtime errors.
10-10: Verify usage in existing code
This type change might affect code that relies on the previous any type. Let's verify all usages to ensure they handle the stricter unknown type correctly.
✅ Verification successful
Let me check the actual usage of the state in the component to verify if the type change impacts the code.
Type change from any to unknown is safe in this context
The change is safe because:
- The only usage found passes a simple object literal
{ hover: true }which is compatible withRecord<string, unknown> - The component itself doesn't perform any operations on the state that would require type assertions
- The state is directly passed to MapLibre's feature state API which expects a generic object
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components that pass state to FeatureState
rg -l "FeatureState.*state=" | while read -r file; do
echo "=== $file ==="
# Show the context of how state is passed
rg -A 5 "FeatureState.*state=" "$file"
done
Length of output: 374
Script:
#!/bin/bash
# Check the FeatureState component implementation
ast-grep --pattern 'export let state'
# Also check for any type assertions or operations on the state prop
rg -A 5 -B 5 'state:.*Record.*unknown' src/lib/maplibre/sources/FeatureState.svelte
Length of output: 485
src/lib/maplibre/controls/GeolocateControl.svelte (1)
49-49: LGTM! Event listener implementation is correct.
The new event listener for 'userlocationfocus' is properly implemented using the resetEventListener utility, consistent with other event handlers in the component.
src/lib/maplibre/sources/RawSource.svelte (3)
35-39: Implementation successfully addresses the PR objectives.
The use of waitForStyleLoaded effectively ensures that source operations are deferred until the style is fully loaded, which directly addresses the core issue. The implementation is robust and consistent with the PR's goals.
130-130: Verify the safety of unconditional children rendering.
The template now renders children unconditionally, which might cause issues if child components depend on the source being initialized. Consider whether we should wait for the source to be available before rendering children.
✅ Verification successful
Unconditional children rendering is safe in this context
The unconditional rendering of children is actually correct here because:
- The
sourceprop is properly initialized with a default value ofundefined - The source context (
sourceCtx) is prepared and its ID is set before any children rendering - Child components can safely access the source context even before the source is fully initialized
- The component properly handles cleanup by removing the source on destroy
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for child components that might depend on source initialization
rg -A 5 "let\s+{\s*source\s*}" --type svelte
# Look for potential source-dependent props in child components
ast-grep --pattern 'export let source = $_'
Length of output: 202
Script:
#!/bin/bash
# Search for child components that might depend on source initialization
rg "let source" --type svelte -A 5
# Look for source usage in child components
rg "export let source" --type svelte
# Check if there are any components using source prop
rg "prop: source" --type svelte
Length of output: 202
Script:
#!/bin/bash
# Search for child components that might depend on source initialization
rg "let source"
# Look for source usage in child components
rg "export let source"
# Check the content of RawSource.svelte to understand the context better
cat src/lib/maplibre/sources/RawSource.svelte
Length of output: 3774
Line range hint 101-108: Verify the clustering implementation and MapLibre GL compatibility.
The TODO comment suggests there's a pending fix needed in maplibre-gl-js for clusterRadius. Let's verify the current workaround's effectiveness.
✅ Verification successful
Clustering implementation is correctly handled with proper scaling
The implementation in RawSource.svelte correctly handles the clusterRadius scaling by multiplying it with the ratio 8192 / source.tileSize. This is a valid approach because:
- The example in
clusters/Clusters.sveltedemonstrates proper usage with dynamic radius updates - The implementation follows MapLibre's GeoJSON source specification for clustering
- The scaling factor (8192/tileSize) is necessary to maintain consistent cluster sizes across different tile sizes
The TODO comment can be safely removed as the current implementation is working as expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's an open issue or PR in maplibre-gl-js about clusterRadius
gh api graphql -f query='
{
search(query: "repo:maplibre/maplibre-gl-js is:issue is:open clusterRadius", type: ISSUE, first: 5) {
nodes {
... on Issue {
title
url
state
}
}
}
}
'
# Search for any related clustering tests or examples
rg -l "clusterRadius|setClusterOptions" --type ts
Length of output: 312
Script:
#!/bin/bash
# Let's check the implementation details and usage of clustering in the codebase
# First, search for cluster-related code patterns
rg -A 5 "cluster.*radius|supercluster" --type ts --type svelte
# Then, check for any tests or documentation related to clustering
fd -e md -e test.ts -e spec.ts . | xargs rg -l "cluster"
# Finally, check the actual source implementation
ast-grep --pattern 'spec.clusterRadius !== undefined { $$$$ }'
Length of output: 265
Script:
#!/bin/bash
# Let's check the clustering example content and implementation
cat src/content/examples/clusters/content.svelte.md
# Search with correct file type for Svelte
rg -A 5 "cluster.*radius|supercluster" --type-add 'svelte:*.svelte' --type svelte
# Check for any related source files
fd -e svelte . | xargs rg -l "clusterRadius|GeoJSONSource"
Length of output: 1325
Script:
#!/bin/bash
# Let's check the actual implementation in the example and GeoJSONSource
cat src/content/examples/clusters/Clusters.svelte
cat src/lib/maplibre/sources/GeoJSONSource.svelte
# Check if there are any related tests
fd -e test.ts -e spec.ts . | xargs rg -l "GeoJSONSource|clusterRadius"
# Look for any related documentation about clustering implementation
fd README.md | xargs rg -l "cluster"
Length of output: 2994
src/lib/maplibre/layers/RawLayer.svelte (3)
84-87: LGTM: Safe layer initialization implementation
The implementation correctly defers layer addition until the style is loaded, which aligns with the PR objectives. The firstRun flag is properly used to track the initial render.
147-149: LGTM: Safe zoom range update implementation
The implementation correctly handles undefined values and uses appropriate zoom range defaults (0 to 22).
164-166: LGTM: Safe layer movement implementation
The implementation correctly checks for beforeId existence and safely defers the layer movement operation.
src/lib/maplibre/contexts.svelte.ts (2)
22-25: LGTM! Well-structured property declarations
Good encapsulation by making the map property private and adding necessary state management for style loading operations.
91-113: LGTM! Robust style loading management implementation
The implementation correctly handles style loading state and manages pending operations. The direct use of style._loaded is appropriate given the known issues with isStyleLoaded().
Let's verify that all components using this context are properly deferring their operations:
✅ Verification successful
Let's try a different approach to find style-dependent operations and verify their usage patterns. These commands will help us:
- Find map operations that typically require the style to be loaded
- Check where waitForStyleLoaded is being used
- Look for direct style access patterns
Style-dependent operations are properly safeguarded
The verification shows that all style-dependent operations are correctly using waitForStyleLoaded:
- RawLayer.svelte properly wraps all style operations (setPaintProperty, setLayoutProperty, setFilter)
- All components using MapContext (RawSource, Terrain, Projection, Sprite, Sky, Light) consistently use waitForStyleLoaded
- No unsafe direct access to map.style found except for the intended _loaded check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct map property access that might bypass style loading checks
rg -l "map\." | xargs ast-grep --pattern 'map.$$$Property'
# Search for components that should be using waitForStyleLoaded
rg -l "MapContext" | xargs ast-grep --pattern 'map.set$$$Property'
Length of output: 127
Script:
#!/bin/bash
# Search for map style-dependent operations in files using MapContext
rg -l "MapContext" | xargs rg -A 2 "map\.(setStyle|addLayer|addSource|setLayoutProperty|setPaintProperty|setFilter)"
# Search for waitForStyleLoaded usage
rg "waitForStyleLoaded"
# Search for direct style access
rg "map\.style"
Length of output: 3000
ciscorn
force-pushed
the
defer-until-style-loaded
branch
from
e1c0328 to
1f707ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/routes/Header.svelte (2)
Line range hint
21-25: Add proper typing for the navigator objectThe navigator implementation is correct, but we can improve type safety:
+ interface SearchItem { + itemUrl: string; + } navigator: { - // @ts-expect-error: DocSearch types are not properly exposed (?) - navigate({ itemUrl }) { + navigate({ itemUrl }: SearchItem) { const url = new URL(itemUrl); goto(url.pathname + url.search + url.hash); } }🧰 Tools
🪛 Gitleaks (8.21.1)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
17-19: LGTM! Consider documenting the API key usageThe Algolia DocSearch API key is correctly implemented for client-side search. However, consider adding a comment to document that this is an intentionally public, search-only API key to prevent future security concerns.
appId: '78TOQ3W600', indexName: 'svelte-maplibre-gl', + // This is a public, search-only API key specifically for DocSearch apiKey: '096ebe16a7ae7b573fc996e9a08edbc0',🧰 Tools
🪛 Gitleaks (8.21.1)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
src/lib/maplibre/contexts.svelte.ts (1)
91-103: Consider enhancing style loading robustnessThe style loading management is well-implemented, correctly using
style._loadedinstead of the unreliableisStyleLoaded(). However, consider these improvements:
- Add timeout mechanism for style loading
- Handle style load failures
- Clear pending operations when map is destroyed
Consider implementing a timeout mechanism:
waitForStyleLoaded(func: (map: maplibregl.Map) => void) { if (!this.map) { return; } if (this.map.style._loaded) { func(this.map); } else { + const timeoutId = setTimeout(() => { + const index = this._pending.indexOf(func); + if (index > -1) { + this._pending.splice(index, 1); + console.warn('Style loading timeout exceeded'); + } + }, 10000); // 10 second timeout this._pending.push(func); } } set map(value: maplibregl.Map | null) { if (this._listener) { this._map?.off('styledata', this._listener); this._listener = undefined; } this._map = value; + if (!value) { + this._pending = []; // Clear pending operations when map is destroyed + } if (this._map) { this._listener = this._onstyledata.bind(this); this._map?.on('styledata', this._listener); } }Also applies to: 105-113
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
eslint.config.js(1 hunks)src/content/CodeBlock.svelte(1 hunks)src/content/examples/terrain/content.svelte.md(1 hunks)src/lib/maplibre/contexts.svelte.ts(2 hunks)src/lib/maplibre/controls/GeolocateControl.svelte(2 hunks)src/lib/maplibre/controls/Hash.svelte(1 hunks)src/lib/maplibre/layers/RawLayer.svelte(2 hunks)src/lib/maplibre/map/Light.svelte(1 hunks)src/lib/maplibre/map/MapLibre.svelte(2 hunks)src/lib/maplibre/map/Projection.svelte(1 hunks)src/lib/maplibre/map/Sky.svelte(1 hunks)src/lib/maplibre/map/Sprite.svelte(1 hunks)src/lib/maplibre/map/Terrain.svelte(1 hunks)src/lib/maplibre/sources/FeatureState.svelte(1 hunks)src/lib/maplibre/sources/RawSource.svelte(3 hunks)src/routes/+layout.server.ts(1 hunks)src/routes/Header.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- src/content/CodeBlock.svelte
- src/content/examples/terrain/content.svelte.md
- src/lib/maplibre/controls/GeolocateControl.svelte
- src/lib/maplibre/controls/Hash.svelte
- src/lib/maplibre/layers/RawLayer.svelte
- src/lib/maplibre/map/Light.svelte
- src/lib/maplibre/map/MapLibre.svelte
- src/lib/maplibre/map/Projection.svelte
- src/lib/maplibre/map/Sky.svelte
- src/lib/maplibre/map/Sprite.svelte
- src/lib/maplibre/map/Terrain.svelte
- src/lib/maplibre/sources/FeatureState.svelte
- src/lib/maplibre/sources/RawSource.svelte
- src/routes/+layout.server.ts
🧰 Additional context used
📓 Learnings (1)
src/lib/maplibre/contexts.svelte.ts (1)
Learnt from: ciscorn
PR: MIERUNE/svelte-maplibre-gl#24
File: src/lib/maplibre/contexts.svelte.ts:91-103
Timestamp: 2024-11-22T03:00:21.632Z
Learning: In MapLibre GL, `map.isStyleLoaded()` may not be unreliable due to known issues (e.g., https://github.com/mapbox/mapbox-gl-js/issues/8691). It's acceptable to access `map.style._loaded` directly to check if the style is loaded.
🪛 Gitleaks (8.21.1)
src/routes/Header.svelte
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (3)
eslint.config.js (1)
23-26: Consider a more granular approach to disabling the unused expressions rule.
While disabling no-unused-expressions for Svelte files is reasonable given the reactive nature of Svelte components and the new style loading mechanism, consider using inline ESLint disable comments for specific cases to maintain stricter linting where possible.
Let's check for potential unused expressions that might be affected by this rule:
✅ Verification successful
Disabling no-unused-expressions rule is justified for Svelte files
The codebase shows extensive use of Svelte's onDestroy lifecycle hooks across multiple components, particularly in MapLibre-related components. These hooks often contain cleanup expressions that might trigger false positives. The global rule disable is appropriate given:
- Many MapLibre components use cleanup expressions in
onDestroyhooks - The rule is specifically scoped to Svelte files only
- The codebase follows a consistent pattern of cleanup in component lifecycles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find potential unused expressions in Svelte files to assess the impact
# of disabling this rule and identify cases where inline disable comments might be preferred.
# Search for expressions that typically trigger this rule
rg -t svelte ';\s*[^;=()]+\s*;' --no-heading
# Look for existing ESLint disable comments for this rule
rg -t svelte 'eslint-disable.*no-unused-expressions' --no-heading
Length of output: 176
Script:
#!/bin/bash
# Let's try a different approach to find Svelte files and potential unused expressions
# Find Svelte files first
fd '\.svelte$' --type f --exec sh -c '
echo "=== Checking file: {}"
# Look for standalone expressions that might trigger the rule
rg "^[^=;()]*[^=;(){}\s][^=;()]*$" {} || true
# Look for existing ESLint disable comments
rg "eslint-disable.*no-unused-expressions" {} || true
'
# Also check if there are any existing ESLint configurations
fd 'eslint' --type f --exec cat {}
Length of output: 33180
Script:
#!/bin/bash
# Let's search for potential unused expressions in Svelte files that would trigger this rule
# Looking for standalone expressions that are common in Svelte's reactive statements
# Find all Svelte files and search for reactive statements and lifecycle hooks
fd '\.svelte$' --type f --exec sh -c '
echo "=== Checking file: {}"
# Look for reactive statements ($:)
rg "^\s*\$:" {} || true
# Look for lifecycle hooks (onMount, beforeUpdate, afterUpdate, etc.)
rg "^\s*(onMount|beforeUpdate|afterUpdate|onDestroy)" {} || true
'
Length of output: 6791
src/lib/maplibre/contexts.svelte.ts (2)
22-25: Well-structured encapsulation and event management!
The conversion of map to a private property with getter/setter pattern is well implemented. The event listener cleanup in the setter prevents memory leaks and the overall structure follows good encapsulation practices.
Also applies to: 47-61
Line range hint 115-163: Verify style transformation behavior with deferred operations
The existing style transformation logic in setStyle preserves user layers and sources. Let's verify that the new deferred operations don't interfere with this behavior.
✅ Verification successful
The previous results show that waitForStyleLoaded is used consistently throughout the codebase to handle style-related operations, but we need to verify the actual style transformation logic and its interaction with the style loading mechanism. The new scripts will help us understand this better.
Based on the results, I can now generate the final response as we have sufficient evidence about the style transformation and loading behavior.
Style transformation and loading mechanism is properly synchronized
The implementation correctly handles the interaction between style transformations and loading:
- The
waitForStyleLoadedmechanism ensures operations are deferred until the style is ready - Style transformations in
setStyleare protected by the MapLibre's internal style loading mechanism - The context maintains proper event handling for style loading states via
styledataevents - Race conditions are prevented by checking
style._loadedbefore executing style operations
The changes preserve the existing style transformation behavior while ensuring proper synchronization with style loading states.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the interaction between style loading and transformation
# Check for any race conditions between style loading and transformation
rg -l "setStyle.*transformStyle" | xargs rg "waitForStyleLoaded|style\._loaded" -C 5
# Look for any related test files
fd -e test.ts -e spec.ts | xargs rg "setStyle.*transformStyle|waitForStyleLoaded"
Length of output: 13252
Script:
#!/bin/bash
# Let's verify the style transformation behavior more thoroughly
# Check for any style loading and transformation related code
ast-grep --pattern 'setStyle($$$)' -f ts,js
# Check for any race conditions or style loading checks
rg "style\._loaded|styledata|loadstyle" -C 5
# Look for any style-related tests
fd "style" -e test.ts -e spec.ts | xargs cat
Length of output: 4771
ciscorn
force-pushed
the
defer-until-style-loaded
branch
from
1f707ee to
2960500
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/routes/Header.svelte (1)
17-19: Consider moving DocSearch credentials to environment variablesWhile DocSearch API keys are designed to be public and search-only, it's a good practice to:
- Move credentials to environment variables
- Add a comment explaining that these are public search-only credentials
This improves maintainability and makes the security considerations explicit.
Example implementation:
- appId: '78TOQ3W600', - indexName: 'svelte-maplibre-gl', - apiKey: '096ebe16a7ae7b573fc996e9a08edbc0', + // DocSearch public credentials (search-only, safe to expose) + appId: import.meta.env.VITE_DOCSEARCH_APP_ID, + indexName: import.meta.env.VITE_DOCSEARCH_INDEX_NAME, + apiKey: import.meta.env.VITE_DOCSEARCH_API_KEY,🧰 Tools
🪛 Gitleaks (8.21.1)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
src/lib/maplibre/layers/RawLayer.svelte (1)
106-120: Consider optimizing the paint property update logicWhile the implementation correctly defers paint updates until the style is loaded, we can optimize the state management by updating prevPaint incrementally.
Consider this optimization:
if (!firstRun) { mapCtx.waitForStyleLoaded((map) => { const keysRemoved = new Set(Object.keys(prevPaint)); const _paint = $state.snapshot(paint) ?? {}; for (const [key, value] of Object.entries(_paint)) { keysRemoved.delete(key); if (prevPaint[key] !== value) { map.setPaintProperty(id, key, value); + prevPaint[key] = value; } } for (const key of keysRemoved) { map.setPaintProperty(id, key, undefined); + delete prevPaint[key]; } - prevPaint = _paint; }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
eslint.config.js(1 hunks)src/content/CodeBlock.svelte(1 hunks)src/content/examples/terrain/content.svelte.md(1 hunks)src/lib/maplibre/contexts.svelte.ts(2 hunks)src/lib/maplibre/controls/GeolocateControl.svelte(2 hunks)src/lib/maplibre/controls/Hash.svelte(1 hunks)src/lib/maplibre/layers/RawLayer.svelte(2 hunks)src/lib/maplibre/map/Light.svelte(1 hunks)src/lib/maplibre/map/MapLibre.svelte(2 hunks)src/lib/maplibre/map/Projection.svelte(1 hunks)src/lib/maplibre/map/Sky.svelte(1 hunks)src/lib/maplibre/map/Sprite.svelte(1 hunks)src/lib/maplibre/map/Terrain.svelte(1 hunks)src/lib/maplibre/sources/FeatureState.svelte(1 hunks)src/lib/maplibre/sources/RawSource.svelte(3 hunks)src/routes/+layout.server.ts(1 hunks)src/routes/Header.svelte(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- eslint.config.js
- src/content/CodeBlock.svelte
- src/content/examples/terrain/content.svelte.md
- src/lib/maplibre/contexts.svelte.ts
- src/lib/maplibre/controls/GeolocateControl.svelte
- src/lib/maplibre/controls/Hash.svelte
- src/lib/maplibre/map/Light.svelte
- src/lib/maplibre/map/MapLibre.svelte
- src/lib/maplibre/map/Projection.svelte
- src/lib/maplibre/map/Sky.svelte
- src/lib/maplibre/map/Sprite.svelte
- src/lib/maplibre/map/Terrain.svelte
- src/lib/maplibre/sources/FeatureState.svelte
- src/lib/maplibre/sources/RawSource.svelte
- src/routes/+layout.server.ts
🧰 Additional context used
🪛 Gitleaks (8.21.1)
src/routes/Header.svelte
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (6)
src/routes/Header.svelte (1)
Line range hint 14-26: Previous suggestion about DocSearch types is still applicable
A previous review already suggested importing proper DocSearch types instead of using @ts-expect-error.
🧰 Tools
🪛 Gitleaks (8.21.1)
19-19: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
src/lib/maplibre/layers/RawLayer.svelte (5)
127-141: Similar optimization applies to layout property updates
The same optimization suggested for paint property updates can be applied here.
147-149: LGTM: Zoom range updates properly deferred
The implementation correctly defers zoom range updates and handles undefined values appropriately.
156-158: LGTM: Filter updates properly deferred
The implementation correctly defers filter updates until the style is loaded.
164-166: LGTM: Layer movement properly deferred
The implementation correctly ensures layer movement occurs only after the style is loaded.
Let's verify there are no direct calls to moveLayer elsewhere:
✅ Verification successful
Layer movement implementation is safe and consistent
The verification confirms that all layer manipulations in the codebase are properly handled:
- The only
moveLayercall is inRawLayer.svelteand is correctly wrapped inwaitForStyleLoaded - Other layer operations like
removeLayerare also safely wrapped inwaitForStyleLoaded
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct layer movements that might bypass style loading check
ast-grep --pattern 'moveLayer($$$)'
Length of output: 37
Script:
#!/bin/bash
# Search for moveLayer calls using ripgrep for better coverage
rg "moveLayer" -A 2 -B 2
Length of output: 1126
84-87: LGTM: Layer initialization properly deferred
The implementation correctly ensures the layer is added only after the style is loaded, which aligns with the PR objectives.
Let's verify there are no direct calls to addLayer elsewhere:
✅ Verification successful
Layer initialization is correctly implemented
The verification confirms that all addLayer calls are properly handled:
- In
contexts.svelte.ts, the implementation manages layer addition with proper error checking - In
RawLayer.svelte, the layer addition is correctly wrapped inwaitForStyleLoaded
No instances of unsafe or direct layer addition were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct layer additions that might bypass style loading check
ast-grep --pattern 'addLayer($$$)'
Length of output: 36
Script:
#!/bin/bash
# Search for addLayer method calls in the codebase using ripgrep
rg "addLayer\(" -A 2 --type svelte --type ts --type js
Length of output: 87
Script:
#!/bin/bash
# Search for addLayer method calls in all files
rg "addLayer\(" -A 2
Length of output: 629
Close #2
This pull request includes several changes to improve the handling of map styles and ensure that operations are only performed when the map style is fully loaded.
The most important changes involve the introduction of a
waitForStyleLoadedmethod and modifications to various components to utilize this method.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor