Conversation
* Map refactoring * Implementing reading new optional bindings * Adding changes to map component from upstream/refactor/map --------- Co-authored-by: Ole Martin Handeland <git@olemartin.org>
* Add toolbar config * Add config validation - Add config validation for geometryIsEditable & toolbar * Update useValidateGeometriesBindings.ts - Only check for geometryIsEditable if simpleBinding is not set when geometries are defined * Update map config validation - Update useValidateGeometriesBindings to check for geometryIsEditable if toolbar also is set - Update Map.tsx to only show MapSingleMarker if simpleBinding is set * Add PR fixes
- save to datamodel and load geometries to editable layer
* WIP edit geometry * working editing geometries
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Leaflet Draw support: new MapEditGeometries component and spritesheet-fix hook, toolbar config and binding (geometryIsEditable), geometry parsing/type additions (isEditable), binding validation updates, stylesheet import, dependency changes, and E2E tests for draw/delete flows. (39 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
Magnusrm
added
kind/product-feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @package.json:
- Line 177: react-leaflet-draw 0.21.0 is incompatible with react-leaflet 5.0.0;
either pin react-leaflet to a pre-v5 compatible version or replace
react-leaflet-draw with a v5-compatible fork. Update package.json to remove or
replace "react-leaflet-draw": "0.21.0" with a maintained package (e.g.,
"@dbaltor00/react-leaflet-draw") or downgrade "react-leaflet" to a version
compatible with 0.21.0, then run install and update any imports/usages of
react-leaflet-draw in your codebase to the new package name (or adjust for API
differences) and run the app/tests to confirm everything works.
In @src/layout/Map/features/geometries/editable/MapEditGeometries.tsx:
- Line 52: Remove the debug console.log in the MapEditGeometries component that
prints initialGeometries: delete the line "console.log('Loading initial editable
geometries into MapEditGeometries', initialGeometries)" inside MapEditGeometries
(or replace it with a proper logger call like logger.debug if project logging is
required), ensure no other console.* calls remain in this file, and run the
linter/static analysis to confirm the warning is resolved.
- Around line 165-173: The update logic currently uses the index from the
filtered initialGeometries array which misaligns with the original form data;
change the lookup so you find the geometry's index in the unfiltered source by
matching altinnRowId (e.g. findIndex on the complete geometries array used to
build the form data) and then build the field path using that original index
(the same pattern used in onDeletedHandler), then call setLeafValue with the
corrected reference (keep using geometryBinding.field and geometryDataPath but
replace the filtered index with the found originalIndex) so the correct record
is updated.
In @src/layout/Map/features/geometries/useValidateGeometriesBindings.ts:
- Line 13: The call to useExternalItem(baseComponentId, 'Map') may return
undefined, so avoid directly accessing .toolbar; change the code that assigns
toolbar (the const toolbar = useExternalItem(baseComponentId, 'Map').toolbar
line) to first retrieve the item into a variable, check for existence (or use
optional chaining) and handle the missing-case (e.g., return early, provide a
default toolbar, or throw a clear error) so you never dereference undefined.
🧹 Nitpick comments (6)
package.json (1)
164-164: Consider pinningleaflet-drawto an exact version for consistency.The project uses exact versioning for
leaflet(1.9.4) but uses caret versioning forleaflet-draw(^1.0.4). For consistency and reproducible builds, consider pinning to an exact version.src/layout/Map/features/geometries/fixed/MapGeometries.tsx (1)
33-36: Remove redundant optional chaining.The optional chaining on
geometries?.filteris unnecessary since the early return on lines 29-31 already guaranteesgeometriesis truthy at this point.🔎 Proposed fix
// if toolbar is defined, we want to render editable geometries separately if (toolbar) { - geometries = geometries?.filter((g) => !g.isEditable); + geometries = geometries.filter((g) => !g.isEditable); }src/layout/Map/features/geometries/editable/MapEditGeometries.tsx (4)
33-37: Consolidate repeated hook calls.
useDataModelBindingsForis called three times with identical arguments. Extract the bindings once and destructure the needed properties.🔎 Proposed fix
- const geometryBinding = useDataModelBindingsFor(baseComponentId, 'Map')?.geometries; - const geometryDataBinding = useDataModelBindingsFor(baseComponentId, 'Map')?.geometryData; - const isEditableBinding = useDataModelBindingsFor(baseComponentId, 'Map')?.geometryIsEditable; + const bindings = useDataModelBindingsFor(baseComponentId, 'Map'); + const geometryBinding = bindings?.geometries; + const geometryDataBinding = bindings?.geometryData; + const isEditableBinding = bindings?.geometryIsEditable;
155-156: Replace vague @ts-expect-error with proper typing.Using
@ts-expect-errorwith just "test" as the comment is poor practice. Either add proper typing for the layer or provide a meaningful explanation. As per coding guidelines, avoid type casting andanywhen possible.🔎 Proposed fix
e.layers.eachLayer((layer) => { - // @ts-expect-error test - const editedGeo = layer.toGeoJSON(); + const editedGeo = (layer as L.GeoJSON).toGeoJSON() as FeatureWithId;
183-184: Same @ts-expect-error issue as in onEditedHandler.🔎 Proposed fix
e.layers.eachLayer((layer) => { - // @ts-expect-error test - const deletedGeo = layer.toGeoJSON(); + const deletedGeo = (layer as L.GeoJSON).toGeoJSON() as FeatureWithId;
161-161: Use strict equality operator.Using
==instead of===can lead to unexpected type coercion.🔎 Proposed fix
- if (geometryType == 'WKT') { + if (geometryType === 'WKT') {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
package.jsonsrc/index.tsxsrc/layout/Map/Map.tsxsrc/layout/Map/config.tssrc/layout/Map/features/geometries/editable/MapEditGeometries.tsxsrc/layout/Map/features/geometries/editable/useLeafletDrawSpritesheetFix.tssrc/layout/Map/features/geometries/fixed/MapGeometries.tsxsrc/layout/Map/features/geometries/fixed/hooks.tssrc/layout/Map/features/geometries/useValidateGeometriesBindings.tssrc/layout/Map/index.tsxsrc/layout/Map/types.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganytype or type casting (as type) in TypeScript code; improve typing by avoiding casts andanys when refactoring
Use objects for managing query keys and functions, andqueryOptionsfor sharing TanStack Query patterns across the system for central management
Files:
src/layout/Map/features/geometries/fixed/MapGeometries.tsxsrc/layout/Map/features/geometries/editable/useLeafletDrawSpritesheetFix.tssrc/layout/Map/types.tssrc/layout/Map/Map.tsxsrc/layout/Map/features/geometries/editable/MapEditGeometries.tsxsrc/layout/Map/index.tsxsrc/layout/Map/config.tssrc/layout/Map/features/geometries/fixed/hooks.tssrc/index.tsxsrc/layout/Map/features/geometries/useValidateGeometriesBindings.ts
{**/*.module.css,**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and leverage Digdir Design System components when possible
Files:
src/layout/Map/features/geometries/fixed/MapGeometries.tsxsrc/layout/Map/features/geometries/editable/useLeafletDrawSpritesheetFix.tssrc/layout/Map/types.tssrc/layout/Map/Map.tsxsrc/layout/Map/features/geometries/editable/MapEditGeometries.tsxsrc/layout/Map/index.tsxsrc/layout/Map/config.tssrc/layout/Map/features/geometries/fixed/hooks.tssrc/index.tsxsrc/layout/Map/features/geometries/useValidateGeometriesBindings.ts
src/layout/*/{config,Component,index,config.generated}.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Layout components must follow standardized structure with
config.ts,Component.tsx,index.tsx, andconfig.generated.tsfiles
Files:
src/layout/Map/index.tsxsrc/layout/Map/config.ts
🧠 Learnings (2)
📚 Learning: 2025-11-25T12:53:54.399Z
Learnt from: CR
Repo: Altinn/app-frontend-react PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:53:54.399Z
Learning: Applies to src/layout/*/{config,Component,index,config.generated}.{ts,tsx} : Layout components must follow standardized structure with `config.ts`, `Component.tsx`, `index.tsx`, and `config.generated.ts` files
Applied to files:
src/layout/Map/config.tssrc/index.tsx
📚 Learning: 2025-11-25T12:53:54.399Z
Learnt from: CR
Repo: Altinn/app-frontend-react PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T12:53:54.399Z
Learning: Applies to {**/*.module.css,**/*.{ts,tsx}} : Use CSS Modules for component styling and leverage Digdir Design System components when possible
Applied to files:
src/index.tsx
🧬 Code graph analysis (7)
src/layout/Map/features/geometries/fixed/MapGeometries.tsx (2)
src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/layout/Map/features/geometries/fixed/hooks.ts (1)
useMapParsedGeometries(52-65)
src/layout/Map/Map.tsx (5)
src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/layout/Map/features/geometries/editable/MapEditGeometries.tsx (1)
MapEditGeometries(28-210)src/layout/Map/features/layers/MapLayers.tsx (1)
MapLayers(41-62)src/layout/Map/features/geometries/fixed/MapGeometries.tsx (1)
MapGeometries(25-62)src/layout/Map/features/singleMarker/MapSingleMarker.tsx (1)
MapSingleMarker(39-87)
src/layout/Map/features/geometries/editable/MapEditGeometries.tsx (7)
src/utils/layout/useNodeItem.ts (1)
useItemWhenType(15-33)src/utils/layout/hooks.ts (1)
useDataModelBindingsFor(102-112)src/layout/Map/features/geometries/fixed/hooks.ts (1)
useMapParsedGeometries(52-65)src/features/saveToGroup/useSaveToGroup.ts (1)
toRelativePath(19-24)src/features/formData/FormDataWrite.tsx (1)
FD(720-1178)src/layout/Map/features/geometries/editable/useLeafletDrawSpritesheetFix.ts (1)
useLeafletDrawSpritesheetFix(13-32)src/features/formData/types.ts (1)
ALTINN_ROW_ID(39-39)
src/layout/Map/index.tsx (1)
src/utils/layout/hooks.ts (1)
useExternalItem(16-22)
src/layout/Map/config.ts (1)
src/codegen/CG.ts (1)
CG(25-57)
src/layout/Map/features/geometries/fixed/hooks.ts (1)
src/features/saveToGroup/useSaveToGroup.ts (1)
toRelativePath(19-24)
src/layout/Map/features/geometries/useValidateGeometriesBindings.ts (3)
src/features/datamodel/DataModelsProvider.tsx (1)
DataModels(381-429)src/features/form/layout/LayoutsContext.tsx (1)
useLayoutLookups(102-102)src/utils/layout/hooks.ts (1)
useExternalItem(16-22)
🪛 GitHub Check: Type-checks, eslint, unit tests and SonarCloud
src/layout/Map/features/geometries/editable/MapEditGeometries.tsx
[warning] 52-52:
Unexpected console statement. Only these console methods are allowed: warn, error
🔇 Additional comments (13)
src/index.tsx (1)
43-44: LGTM!The leaflet-draw CSS import is correctly placed alongside the existing Leaflet CSS import, which is necessary for styling the drawing toolbar controls introduced in this PR.
src/layout/Map/types.ts (1)
3-14: LGTM!The
isEditableproperty additions to bothRawGeometryandGeometrytypes are consistent and correctly typed as optional booleans, supporting the new geometry editing functionality.src/layout/Map/index.tsx (1)
47-59: LGTM! Comprehensive validation logic.The validation rules correctly enforce the mutual constraints:
geometryIsEditableandsimpleBindingare mutually exclusivegeometryIsEditablerequires atoolbarto be definedtoolbarrequiresgeometryIsEditablebindingThe error messages are clear and actionable for developers.
src/layout/Map/features/geometries/editable/useLeafletDrawSpritesheetFix.ts (1)
13-31: LGTM! Good workaround for webpack asset path issues.The hook correctly addresses the leaflet-draw spritesheet path problem by injecting a global style override once. The deduplication check via
STYLE_IDprevents multiple injections across component remounts.Note: The style element is intentionally not cleaned up on unmount since the fix should persist globally throughout the app's lifecycle.
src/layout/Map/Map.tsx (1)
55-66: LGTM! Clean conditional rendering logic.The rendering flow correctly handles the two mutually exclusive modes:
- Toolbar mode: Renders
MapEditGeometriesfor drawing/editing geometries- Simple binding mode: Renders
MapSingleMarkerfor single marker placementThis aligns well with the validation rules enforced in
Map/index.tsx.src/layout/Map/features/geometries/useValidateGeometriesBindings.ts (1)
42-57: Validation logic for geometryIsEditable looks correct.The conditional addition of
geometryIsEditablevalidation when a toolbar is present and geometries binding exists (without simpleBinding) aligns with the feature requirements.src/layout/Map/config.ts (2)
43-50: Well-structured data model binding for geometryIsEditable.The binding is correctly marked as optional with a clear description explaining that geometries won't be editable if not specified.
176-216: Toolbar configuration is well-designed.The toolbar property uses expression-based boolean controls for each geometry type, with safe defaults (
false). The structure aligns with the codegen patterns used elsewhere in the codebase. Based on learnings, this follows the standardized layout component structure.src/layout/Map/features/geometries/editable/MapEditGeometries.tsx (1)
192-209: Render logic looks correct.The EditControl is properly configured with toolbar options and event handlers. The FeatureGroup ref setup allows managing layers programmatically.
src/layout/Map/features/geometries/fixed/hooks.ts (4)
26-27: Proper isEditable propagation in raw geometry extraction.The
isEditablePathresolution follows the same pattern aslabelPathanddataPath, with a sensible default of'isEditable'. The field is correctly picked from form data items.Also applies to: 40-40
43-49: Dependency array correctly updated.Adding
dataModelBindings?.geometryIsEditableensures the memoized value recalculates when the isEditable binding changes.
78-85: isEditable correctly propagated through parsing.Both WKT and GeoJSON parsing paths now include
isEditablein the output geometry objects, maintaining consistency with theRawGeometryandGeometrytype extensions.
3-3: Verified import migration to @terraformer/wkt.The
wktToGeoJSONfunction is the correct export from@terraformer/wktfor converting WKT geometry strings to GeoJSON objects, and its usage aligns with the package documentation.
src/layout/Map/features/geometries/editable/MapEditGeometries.tsx
Outdated
Show resolved
Hide resolved
src/layout/Map/features/geometries/useValidateGeometriesBindings.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
package.json (1)
177-177: Duplicate:react-leaflet-draw0.21.0 compatibility issue withreact-leaflet5.0.0 remains unresolved.As flagged in previous review comments,
react-leaflet-draw0.21.0 is incompatible withreact-leaflet5.0.0 due to breaking API changes in v5. The PR description mentions manual functionality testing was performed—please confirm whether this compatibility issue manifested during testing or if there's a workaround in place.If the issue was not encountered, please verify by running the following script to check the actual react-leaflet version in use and search for any runtime compatibility patches or workarounds in the codebase:
#!/bin/bash # Check react-leaflet version and search for compatibility workarounds echo "=== Checking react-leaflet version ===" grep '"react-leaflet"' package.json echo -e "\n=== Searching for react-leaflet-draw usage ===" rg -n --type=ts --type=tsx -C3 'react-leaflet-draw' echo -e "\n=== Searching for potential compatibility patches or version checks ===" rg -n --type=ts --type=tsx 'EditControl|FeatureGroup.*edit'
🧹 Nitpick comments (1)
src/layout/Map/features/geometries/useValidateGeometriesBindings.ts (1)
52-57: Consider adding a default property or explicit error forgeometryIsEditable.The
geometryIsEditablevalidation entry lacks adefaultProperty(unlikegeometryLabelandgeometryData). If the binding is not provided when the toolbar exists, the validation loop will construct a field path containing the literal string "undefined" (e.g.,geometries[0].undefined), leading to either a lookup error or a confusing error message referencing an "undefined" property.While the validation will still catch the configuration error, the developer experience could be improved by either:
- Adding
defaultProperty: 'isEditable'(if a default field name makes sense), or- Adding an explicit check before the loop that produces a clearer error like:
"geometryIsEditable binding is required when toolbar is present"🔎 Option 1: Add a default property
if (bindings?.geometries && !bindings?.simpleBinding && toolbar) { fieldsToValidate = [ ...fieldsToValidate, - { binding: geometryIsEditable, name: 'geometryIsEditable', expectedType: 'boolean' }, + { binding: geometryIsEditable, name: 'geometryIsEditable', expectedType: 'boolean', defaultProperty: 'isEditable' }, ]; }🔎 Option 2: Add explicit validation with clearer error
if (bindings?.geometries && !bindings?.simpleBinding && toolbar) { + if (!geometryIsEditable) { + errors.push('geometryIsEditable binding is required when toolbar is present'); + } fieldsToValidate = [ ...fieldsToValidate, { binding: geometryIsEditable, name: 'geometryIsEditable', expectedType: 'boolean' }, ]; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
package.jsonsrc/layout/Map/features/geometries/editable/MapEditGeometries.tsxsrc/layout/Map/features/geometries/useValidateGeometriesBindings.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/layout/Map/features/geometries/editable/MapEditGeometries.tsx
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Avoid usinganytype or type casting (as type) in TypeScript code; improve typing by avoiding casts andanys when refactoring
Use objects for managing query keys and functions, andqueryOptionsfor sharing TanStack Query patterns across the system for central management
Files:
src/layout/Map/features/geometries/useValidateGeometriesBindings.ts
{**/*.module.css,**/*.{ts,tsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
Use CSS Modules for component styling and leverage Digdir Design System components when possible
Files:
src/layout/Map/features/geometries/useValidateGeometriesBindings.ts
🧬 Code graph analysis (1)
src/layout/Map/features/geometries/useValidateGeometriesBindings.ts (4)
src/layout/layout.ts (1)
IDataModelBindings(61-64)src/features/datamodel/DataModelsProvider.tsx (1)
DataModels(381-429)src/features/form/layout/LayoutsContext.tsx (1)
useLayoutLookups(102-102)src/utils/layout/hooks.ts (1)
useExternalItem(16-22)
🔇 Additional comments (2)
package.json (1)
57-57: Supporting dependencies look good, pending resolution of Line 177.The additions of
@types/leaflet-draw,@terraformer/wkt, andleaflet-draware appropriate for the drawing functionality. The use of@terraformer/wktinstead of the deprecatedterraformer-wkt-parseris a good choice. However, these dependencies' effectiveness depends on resolving the compatibility issue withreact-leaflet-drawat Line 177.Also applies to: 145-145, 164-164
src/layout/Map/features/geometries/useValidateGeometriesBindings.ts (1)
13-13: Toolbar retrieval now safely handles missing component.The optional chaining (
?.toolbar) properly addresses the past review concern about potential runtime errors when the component is not found.
src/layout/Map/features/geometries/editable/MapEditGeometries.tsx
Outdated
Show resolved
Hide resolved
JamalAlabdullah
left a comment
JamalAlabdullah
left a comment
There was a problem hiding this comment.
Looks good and works nice! I just wonder about some things ;
- After the user delete geometries the message "
Click on feature to remove" keep showing. is it ok or maybe we need disable it ? - There are many comment lines , do we need all of them?
I think the "click on feature to remove" disappears after you click save after deleting.
Agreed! i removed some excessive comments. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/layout/Map/features/geometries/editable/MapEditGeometries.tsx`:
- Line 38: The filtered geometry array is re-created each render because
.filter() returns a new reference; import useMemo and memoize the result: call
const parsed = useMapParsedGeometries(baseComponentId) and then wrap the filter
in useMemo (e.g. const initialGeometries = useMemo(() => parsed?.filter(g =>
g.isEditable), [parsed]) so the dependency for the useEffect is stable; apply
the same pattern to other similar spots between the initialGeometries
declaration and the effect (lines ~50-104) where .filter() or other array
transforms are used.
- Around line 81-95: The code uses unsafe casts "(geoData as Feature)" when
building newFeature; replace those casts by adding a type guard like
isGeoJsonFeature(data): data is Feature and use it to narrow geoData instead of
isFeature, then remove the "as Feature" casts in the newFeature creation (keep
the existing branches for Feature vs geometry), ensuring newFeature is
constructed from the narrowed geoData in the branch that the type guard confirms
is a Feature and from geoData as geometry in the other branch; update any
parameter typing for geoData if needed so the guard can operate.
- Line 151: Change the loose equality check to a strict one: in the
MapEditGeometries component replace the conditional that compares geometryType
to 'WKT' (the expression using geometryType == 'WKT') with a strict equality
comparison (geometryType === 'WKT') so the comparison avoids unintended type
coercion.
🧹 Nitpick comments (2)
src/layout/Map/features/geometries/editable/MapEditGeometries.tsx (2)
28-46: Redundant hook calls — call each hook once and destructure.
useItemWhenType(baseComponentId, 'Map')is invoked on both Line 29 and Line 46, anduseDataModelBindingsFor(baseComponentId, 'Map')is invoked three times (Lines 33–35). Each call re-runs the hook logic. Call each once and destructure what you need.♻️ Suggested refactor
- const { geometryType } = useItemWhenType(baseComponentId, 'Map'); - - const editRef = useRef<L.FeatureGroup>(null); - - const geometryBinding = useDataModelBindingsFor(baseComponentId, 'Map')?.geometries; - const geometryDataBinding = useDataModelBindingsFor(baseComponentId, 'Map')?.geometryData; - const isEditableBinding = useDataModelBindingsFor(baseComponentId, 'Map')?.geometryIsEditable; + const { geometryType, toolbar } = useItemWhenType(baseComponentId, 'Map'); + + const editRef = useRef<L.FeatureGroup>(null); + + const { geometries: geometryBinding, geometryData: geometryDataBinding, geometryIsEditable: isEditableBinding } = + useDataModelBindingsFor(baseComponentId, 'Map') ?? {}; const geometryDataFieldName = geometryDataBinding?.field.split('.').pop(); const isEditableFieldName = isEditableBinding?.field.split('.').pop(); const initialGeometries = useMapParsedGeometries(baseComponentId)?.filter((g) => g.isEditable); const geometryDataPath = toRelativePath(geometryBinding, geometryDataBinding); const appendToList = FD.useAppendToList(); const setLeafValue = FD.useSetLeafValue(); const removeFromList = FD.useRemoveFromListCallback(); - const { toolbar } = useItemWhenType(baseComponentId, 'Map');
144-146:@ts-expect-errorsuppresses type safety — improve typing instead.Lines 145 and 173 use
@ts-expect-errorto silence Leaflet's type gaps. Consider narrowing the type explicitly (e.g., checking'feature' in layer && layer.feature?.properties) or extending Leaflet's type declarations to avoid blanket suppressions. As per coding guidelines, "Avoid usinganyor type casting (as type) in TypeScript; instead, improve typing by removing such casts andanys to maintain proper type safety".Also applies to: 172-174
| const isEditableBinding = useDataModelBindingsFor(baseComponentId, 'Map')?.geometryIsEditable; | ||
| const geometryDataFieldName = geometryDataBinding?.field.split('.').pop(); | ||
| const isEditableFieldName = isEditableBinding?.field.split('.').pop(); | ||
| const initialGeometries = useMapParsedGeometries(baseComponentId)?.filter((g) => g.isEditable); |
There was a problem hiding this comment.
initialGeometries creates a new array reference every render, causing the effect to re-run continuously.
.filter() on Line 38 returns a new array on every render. Since this array is the sole dependency of the useEffect (Line 104), the effect fires every render — clearing and re-adding all layers each time. This causes unnecessary work and potential visual flicker.
Memoize the filtered result:
🐛 Proposed fix
Add useMemo to the import on Line 1, then:
- const initialGeometries = useMapParsedGeometries(baseComponentId)?.filter((g) => g.isEditable);
+ const allParsedGeometries = useMapParsedGeometries(baseComponentId);
+ const initialGeometries = useMemo(
+ () => allParsedGeometries?.filter((g) => g.isEditable),
+ [allParsedGeometries],
+ );Also applies to: 50-104
🤖 Prompt for AI Agents
In `@src/layout/Map/features/geometries/editable/MapEditGeometries.tsx` at line
38, The filtered geometry array is re-created each render because .filter()
returns a new reference; import useMemo and memoize the result: call const
parsed = useMapParsedGeometries(baseComponentId) and then wrap the filter in
useMemo (e.g. const initialGeometries = useMemo(() => parsed?.filter(g =>
g.isEditable), [parsed]) so the dependency for the useEffect is stable; apply
the same pattern to other similar spots between the initialGeometries
declaration and the effect (lines ~50-104) where .filter() or other array
transforms are used.
| const newFeature: FeatureWithId = isFeature | ||
| ? { | ||
| ...(geoData as Feature), | ||
| properties: { | ||
| ...(geoData as Feature).properties, | ||
| altinnRowId: item.altinnRowId, | ||
| }, | ||
| } | ||
| : { | ||
| type: 'Feature', | ||
| geometry: geoData, | ||
| properties: { | ||
| altinnRowId: item.altinnRowId, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid as Feature type casts; narrow with a type guard instead.
Lines 83 and 85 use as Feature casts, which bypasses type safety. As per coding guidelines, "Avoid using any or type casting (as type) in TypeScript; instead, improve typing by removing such casts and anys to maintain proper type safety".
♻️ Suggested approach
Define a type guard function, e.g.:
function isGeoJsonFeature(data: unknown): data is Feature {
return typeof data === 'object' && data !== null && 'type' in data && (data as Record<string, unknown>).type === 'Feature';
}Then use it in place of the isFeature check and the casts will be unnecessary.
🤖 Prompt for AI Agents
In `@src/layout/Map/features/geometries/editable/MapEditGeometries.tsx` around
lines 81 - 95, The code uses unsafe casts "(geoData as Feature)" when building
newFeature; replace those casts by adding a type guard like
isGeoJsonFeature(data): data is Feature and use it to narrow geoData instead of
isFeature, then remove the "as Feature" casts in the newFeature creation (keep
the existing branches for Feature vs geometry), ensuring newFeature is
constructed from the narrowed geoData in the branch that the type guard confirms
is a Feature and from geoData as geometry in the other branch; update any
parameter typing for geoData if needed so the guard can operate.
|
|
||
| let geoString = JSON.stringify(editedGeo); | ||
|
|
||
| if (geometryType == 'WKT') { |
There was a problem hiding this comment.
Use strict equality (===) instead of loose equality (==).
Line 151 uses == which can produce unexpected coercions.
- if (geometryType == 'WKT') {
+ if (geometryType === 'WKT') {📝 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.
| if (geometryType == 'WKT') { | |
| if (geometryType === 'WKT') { |
🤖 Prompt for AI Agents
In `@src/layout/Map/features/geometries/editable/MapEditGeometries.tsx` at line
151, Change the loose equality check to a strict one: in the MapEditGeometries
component replace the conditional that compares geometryType to 'WKT' (the
expression using geometryType == 'WKT') with a strict equality comparison
(geometryType === 'WKT') so the comparison avoids unintended type coercion.
|
3 participants
Description
Adds features for drawing, editing, and deleting geometries on a map.
We have added datamodelbinding
"geometryIsEditable". This is to give the user the choice of making pre-loaded geometries editable or not, as well as making geometries editable after the user has drawn them for the first time. Non-editable geometries will be rendered in the same "old" layer, while editable geometries are rendered in the new editable map layer.In order to turn on and allowing for drawing geometries on the map you must add the
"toolbar"prop to your map config.The prop is an object and have several drawing tools you can turn on.
Available optinonal tools are:
We have implemented the npm package
"react-leaflet-draw"which contains the toolbar and the drawing capabilities.Related Issue(s)
Verification/QA
kind/*andbackport*label to this PR for proper release notes groupingSummary by CodeRabbit