Map layers refactoring #93
Conversation
vlach1989
requested review from
KrystofVavra,
LukasDufek,
barluq,
Copilot and
mbabic84
There was a problem hiding this comment.
Pull request overview
This PR refactors the map layer management system by introducing a modular architecture that separates layer creation logic into specialized source components, improving code organization and maintainability. The changes replace direct layer parsing in SingleMap with a LayerManager that delegates to type-specific layer sources.
Key Changes:
- Introduced
LayerManagerand specialized layer source components (XYZ, COG, GeoJSON, WMS) for modular layer handling - Refactored
SingleMapto use a layer registry pattern with the newLayerManager - Consolidated
TooltipAttributeinterface into a shared model file and updated all imports
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/client/shared/models/models.tooltip.ts |
Created new shared model file for TooltipAttribute interface |
src/client/story/utils/getTooltipAttributes.ts |
Replaced local TooltipAttribute definition with import from shared models |
src/client/map/MapSet/MapTooltip/MapTooltip.tsx |
Updated TooltipAttribute import to use shared model |
src/client/map/MapSet/MapTooltip/getMapTooltip.ts |
Updated TooltipAttribute import to use shared model |
src/client/map/MapSet/handleMapHover.ts |
Updated TooltipAttribute import to use shared model |
src/client/map/MapSet/SingleMap.tsx |
Refactored to use LayerManager with layer registry pattern instead of direct parsing |
src/client/map/components/layers/LayerManager.tsx |
Created new component to route layer rendering based on datasource labels |
src/client/map/components/layers/XYZLayerSource.tsx |
Created XYZ tile layer source component |
src/client/map/components/layers/COGLayerSource.tsx |
Created COG layer source component |
src/client/map/components/layers/GeojsonLayerSource.tsx |
Created GeoJSON layer source component |
src/client/map/components/layers/WMSLayerSource.tsx |
Created WMS layer source component |
src/client/shared/models/models.layers.ts |
Made level property optional in RenderingLayer interface |
src/client/map/components/RenderingMap.tsx |
Added deprecation notice |
src/client/map/logic/validate.layers.ts |
Added deprecation notice |
src/client/map/logic/parsing.layers.ts |
Added deprecation notice |
src/client/map/logic/map.layers.wms.ts |
Added deprecation notice |
src/client/map/logic/map.layers.tile.ts |
Added deprecation notice |
src/client/map/logic/map.layers.mvt.ts |
Added deprecation notice |
src/client/map/logic/map.layers.geojson.ts |
Removed route parameter and data fetching logic; added deprecation notice |
src/client/map/logic/map.layers.cog.ts |
Added deprecation notice |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* TODO: Since sublayers are derived from configuration, which originally is a string | ||
| (from ptr-be-core model HasConfiguration) and later parsed to an object, | ||
| we need to stringify it here to avoid infinite render loops due to object reference changes. */ | ||
| }, [url, isActive, key, opacity, JSON.stringify(sublayers)]); |
There was a problem hiding this comment.
Using JSON.stringify(sublayers) in the dependency array causes the useMemo to recompute on every render when sublayers is an object, defeating its purpose. Consider using a more stable approach such as storing the stringified value in a separate useMemo, or redesigning the data flow to avoid object reference changes.
| /* TODO: Since cogBitmapOptions is derived from configuration, which originally is a string | ||
| (from ptr-be-core model HasConfiguration) and later parsed to an object, | ||
| we need to stringify it here to avoid infinite render loops due to object reference changes. */ | ||
| }, [url, isActive, key, opacity, JSON.stringify(cogBitmapOptions)]); |
There was a problem hiding this comment.
Using JSON.stringify(cogBitmapOptions) in the dependency array causes the useMemo to recompute on every render when cogBitmapOptions is an object, defeating its purpose. Consider using a more stable approach such as storing the stringified value in a separate useMemo, or redesigning the data flow to avoid object reference changes.
barluq
left a comment
barluq
left a comment
There was a problem hiding this comment.
Than sou for edits. I have found few minors that seems to be a bit problematic for general NPM. Primary default routes or NPM methods.
| error, | ||
| isLoading, | ||
| } = useAxios( | ||
| { fetchUrl: route ?? '/api/features' }, // if no route is provided, use default |
There was a problem hiding this comment.
As this is NPM, there should be no default to route.
|
|
||
| // Log an error if data fetching fails | ||
| if (error) { | ||
| // just warning for now due to backward compatibility for apps which not using route for geojson fetching |
There was a problem hiding this comment.
For this there sould be an issue
| { fetchUrl: route ?? '/api/features' }, // if no route is provided, use default | ||
| undefined, | ||
| { documentId: documentId, validIntervalIso, url }, | ||
| { method: 'POST' } |
This pull request introduces a significant refactor of how map layers are managed and rendered in the mapping client, enabling to fetch data directly from layer component. This step is needed before implementing MVT layers. Instead of parsing and constructing Deck.gl layer instances directly in the
SingleMapcomponent, the new approach delegates this responsibility to a modularLayerManagercomponent and a set of specialized layer source components (XYZLayerSource,COGLayerSource,GeojsonLayerSource,WMSLayerSource). This change improves code modularity, maintainability, and extensibility.Additionally, there are improvements to tooltip attribute imports and a deprecation notice for the old map rendering component.
Layer Management Refactor:
LayerManagercomponent that dynamically renders the appropriate layer source component based on datasource labels, and handles the lifecycle and updates of Deck.gl layer instances. (src/client/map/components/layers/LayerManager.tsx)src/client/map/components/layers/COGLayerSource.tsx,GeojsonLayerSource.tsx,WMSLayerSource.tsx) [1] [2] [3]SingleMapto use theLayerManagerfor layer instantiation and management, maintaining a registry of current layer instances and passing only active layers to DeckGL. (src/client/map/MapSet/SingleMap.tsx) [1] [2] [3] [4] [5]Tooltip and Model Imports:
TooltipAttributeto consistently use the shared model definition, improving code clarity and maintainability. (src/client/map/MapSet/MapTooltip/MapTooltip.tsx,getMapTooltip.ts,handleMapHover.ts,SingleMap.tsx) [1] [2] F6740cb3L1)Deprecation and Cleanup:
RenderingMapcomponent as deprecated in favor of the newMapSetand related components, guiding future development. (src/client/map/components/RenderingMap.tsx)Compatibility
It shouldn't break LUISA application.
Updates for fe-core: https://github.com/Gisat/fe-core/tree/refactoring/map-layers
Updates for GG: https://github.com/Gisat/app-euHorizonGreengage/tree/refactoring/map-layers