fix(web): sketch layer custom property in infobox #1248
Conversation
WalkthroughThe changes introduce enhancements to the feature management in the application. A new Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-web ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/useEvaluateProperties.ts (2)
Line range hint
11-63: Consider optimizing the useEffect implementation.The current implementation has several areas that could be improved:
- Including
evaluatedPropertiesin the dependency array could cause unnecessary re-renders since it's also being set within the effect.- The comment about running the effect on each render suggests potential performance issues.
Consider this refactoring:
- useEffect(() => { + useEffect(() => { if (!isReady) { setIsReady(true); return; } if (currentValue !== properties) { setCurrentValue(properties); setEvaluatedResult(undefined); return; } if (selectedFeature) { - const simpleFeature: Feature = { - id: selectedFeature.id, - type: "feature", - geometry: selectedFeature.geometry, - interval: selectedFeature.interval, - properties: selectedFeature.properties, - metaData: selectedFeature.metaData, - range: selectedFeature.range - }; + const simpleFeature = transformToFeature(selectedFeature); const es = currentValue?.map((v) => { const ev = evalExpression( { expression: v.value }, undefined, simpleFeature ); return ev ? { ...v, value: ev } : undefined; }); - if (!isEqual(es, evaluatedProperties)) { setEvaluatedResult(es as PropertyListItem[]); - } } - }, [isReady, currentValue, properties, evaluatedProperties, selectedFeature]); + }, [isReady, currentValue, properties, selectedFeature]); + // Helper function to transform ComputedFeature to Feature + const transformToFeature = (cf: ComputedFeature): Feature => ({ + id: cf.id, + type: "feature", + geometry: cf.geometry, + interval: cf.interval, + properties: cf.properties, + metaData: cf.metaData, + range: cf.range + });
Line range hint
1-65: Consider memoizing the feature transformation.The feature transformation logic runs on every render when properties change. For better performance, especially with large feature objects, consider memoizing the transformation.
Consider using
useMemo:const simpleFeature = useMemo( () => selectedFeature ? transformToFeature(selectedFeature) : undefined, [selectedFeature] );web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/Content.tsx (3)
Line range hint
31-32: Good architectural improvement using React ContextThe switch to using
selectedComputedFeaturefrom coreContext is a better approach than directly accessing the visualizer instance. This change ensures better reactivity and follows React's recommended patterns for state management.
76-79: Consider adding error boundaries for robust property handlingWhile the property passing is implemented correctly, consider wrapping the CustomFields component with an error boundary to gracefully handle any potential runtime errors in property computation or rendering.
Example implementation:
class PropertyErrorBoundary extends React.Component<{children: React.ReactNode}> { static getDerivedStateFromError(error: Error) { return { hasError: true }; } render() { if (this.state?.hasError) { return <Template icon={block?.extensionId} height={120} />; } return this.props.children; } } // Usage: <PropertyErrorBoundary> <CustomFields selectedFeature={selectedComputedFeature} properties={properties} extensionId={block?.extensionId} /> </PropertyErrorBoundary>
Line range hint
108-142: Enhance type safety in helper functionsWhile these functions work correctly, they could benefit from stronger TypeScript typing to prevent potential runtime errors.
Consider this typed implementation:
interface PropertyValue { [key: string]: unknown; } function filterChildObjectsToEnd(inputObject?: Record<string, unknown>): PropertyValue[] { if (!inputObject) return []; const arrayResult: PropertyValue[] = []; const childObjects: [string, unknown][] = []; Object.entries(inputObject).forEach(([key, value]) => { if (value && typeof value === "object") { childObjects.push([key, value]); } else { arrayResult.push({ [key]: value }); } }); childObjects.forEach(([key, value]) => { arrayResult.push({ [key]: value }); }); return arrayResult; }web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx (1)
116-139: Consider adding error handling and optimizing dependenciesA few suggestions to improve the robustness of the field removal logic:
- Add error handling for the update operation
- Consider memoizing complex objects in the dependency array to prevent unnecessary re-renders
useEffect(() => { + try { const removedFields = previousFields.current.filter( (field) => !fields.some((f) => f.id === field.id) ); removedFields.forEach((removedField) => { if (removedField.id && selectedFeature?.properties) { + if (!selectedFeature || !sketchFeature?.id || !layer?.id) return; const { [removedField.id]: removed, ...updatedProperties } = selectedFeature.properties; - if (!selectedFeature || !sketchFeature?.id || !layer?.id) return; if (removed !== undefined) { onGeoJsonFeatureUpdate?.({ layerId: layer.id, featureId: sketchFeature.id, geometry: selectedFeature.geometry, properties: updatedProperties }); } } }); previousFields.current = fields; + } catch (error) { + console.error('Error updating properties:', error); + } }, [ fields, - selectedFeature, + selectedFeature?.properties, + selectedFeature?.geometry, onGeoJsonFeatureUpdate, - layer, + layer?.id, - sketchFeature + sketchFeature?.id ]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx(2 hunks)web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/Content.tsx(1 hunks)web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/index.tsx(2 hunks)web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/useEvaluateProperties.ts(2 hunks)
🧰 Additional context used
🪛 GitHub Check: ci-web / ci
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx
[warning] 1-1:
There should be no empty line within import group
[warning] 1-1:
react import should occur after import of @reearth/services/theme
🔇 Additional comments (9)
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/index.tsx (3)
2-2: LGTM! Type changes look good.
The addition of ComputedFeature import and the optional selectedFeature prop aligns with the PR objective and maintains backward compatibility.
Also applies to: 12-12
16-20: LGTM! Component signature is properly updated.
The component signature has been correctly updated to include the new selectedFeature prop while maintaining a clean parameter ordering.
21-24: Verify undefined selectedFeature handling in useEvaluateProperties hook.
The hook usage looks good, but we should ensure that useEvaluateProperties properly handles cases where selectedFeature is undefined to maintain backward compatibility.
✅ Verification successful
Hook properly handles undefined selectedFeature
The implementation of useEvaluateProperties safely handles cases where selectedFeature is undefined. The evaluation logic is wrapped in a conditional if (selectedFeature) block, and when selectedFeature is undefined, the hook will return the last evaluated properties or undefined, maintaining backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for useEvaluateProperties implementation and its tests
ast-grep --pattern 'const useEvaluateProperties = $_'
# Look for any test files related to this hook
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts useEvaluateProperties
Length of output: 128
Script:
#!/bin/bash
# Try to find the hook implementation with a broader search
rg -A 10 "useEvaluateProperties" --type ts --type tsx
# Search for any files containing useEvaluateProperties in their name
fd useEvaluateProperties
# Look for any test files in the same directory as the component
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts . web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/
Length of output: 361
Script:
#!/bin/bash
# Check the implementation of useEvaluateProperties
cat web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/useEvaluateProperties.ts
# Search for any tests in the parent directories
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts . web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/
Length of output: 2119
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/useEvaluateProperties.ts (2)
1-10: LGTM! Clean type definitions and imports.
The Props type is well-structured and the imports are appropriate for the functionality.
63-63: Verify type safety in the evaluation result.
The type assertion as PropertyListItem[] might be unsafe if any mapped values are undefined.
Let's verify the type safety:
✅ Verification successful
Let me analyze the code further to understand the type safety handling.
Let me check one more aspect to understand how the evaluation results are processed.
Type safety is properly handled in the evaluation result
The type assertion is safe because:
- The code filters out undefined values before returning (
evaluatedProperties?.filter((ep) => ep !== undefined)) - The
PropertyListItemtype is strictly defined with required fields (id,key,value) - The evaluation logic preserves the object structure by spreading the original properties (
...v) - Undefined results from
evalExpressionare explicitly handled in the map function
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns in the codebase to understand how undefined values are typically handled
rg -A 5 "PropertyListItem\[\]"
Length of output: 4463
Script:
#!/bin/bash
# Let's check the implementation of evaluateProperties to understand how undefined values are handled
ast-grep --pattern 'const evaluateProperties = $_'
# Also check where PropertyListItem is defined
rg -A 5 "type PropertyListItem"
Length of output: 832
Script:
#!/bin/bash
# Let's check the full implementation of useEvaluateProperties to understand the evaluation flow
cat "web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/CustomFields/useEvaluateProperties.ts"
Length of output: 1991
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/Content.tsx (2)
Line range hint 42-54: Well-structured property computation with proper memoization
The useMemo implementation correctly includes all necessary dependencies and handles different display types appropriately.
Line range hint 31-54: Verify the fix for property fetching
The implementation aligns with the PR objectives by using selectedComputedFeature for dynamic property updates. Let's verify the fix works as intended.
✅ Verification successful
Based on the search results, I can generate the final response:
Property fetching implementation has been properly updated
The implementation has been correctly migrated to use selectedComputedFeature. The old implementation using visualizer.current?.layers.selectedFeature is only present in useExpressionEval.ts which is a different component. The new implementation in Content.tsx properly:
- Consumes
selectedComputedFeaturefrom the core context - Uses it to filter and display properties
- Passes it down to child components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the old implementation
rg "visualizer\.current.*layers\.selectedFeature"
# Check for proper usage of selectedComputedFeature
rg "selectedComputedFeature" -A 3
Length of output: 4744
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx (2)
1-1: LGTM: Import changes are correct
The addition of useRef to the React imports is appropriate for the new functionality.
🧰 Tools
🪛 GitHub Check: ci-web / ci
[warning] 1-1:
There should be no empty line within import group
[warning] 1-1:
react import should occur after import of @reearth/services/theme
114-139: Verify the impact on feature editing workflow
The new field removal logic interacts with the existing feature editing capabilities. Let's verify that it doesn't interfere with other editing operations.
✅ Verification successful
Based on the search results, I can now provide a final response regarding the impact of the field removal logic on feature editing workflow:
Field removal logic safely integrates with feature editing workflow
The implementation correctly preserves the feature's geometry while only modifying the properties object when removing fields. This is evidenced by:
- The field removal logic only updates the
propertiesobject while keeping the originalgeometryintact in theonGeoJsonFeatureUpdatecall - The component properly separates geometry editing (handled via
onSketchGeometryEdit*handlers) from property editing - The feature update operation maintains proper structure with both geometry and properties, following the established GQL schema
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related feature editing operations
echo "Searching for feature editing operations..."
rg -l "onGeoJsonFeatureUpdate|handleSubmit|setFields" --type ts --type tsx
# Search for potential race conditions with geometry editing
echo "Checking for geometry editing interactions..."
ast-grep --pattern 'onSketchGeometryEdit$$$' --lang typescript
# Look for other components using selectedFeature
echo "Finding components using selectedFeature..."
rg "selectedFeature\??\." --type ts --type tsx
Length of output: 541
Script:
#!/bin/bash
# Search for related feature editing operations with correct file type
echo "Searching for feature editing operations..."
rg -l "onGeoJsonFeatureUpdate|handleSubmit|setFields" --type typescript
# Look for other components using selectedFeature
echo "Finding components using selectedFeature..."
rg "selectedFeature\??\." --type typescript -A 2
# Check for geometry editing interactions
echo "Checking for geometry editing interactions..."
rg "geometry" --type typescript -A 2
Length of output: 561
Script:
#!/bin/bash
# Search for related feature editing operations without file type restriction
echo "Searching for feature editing operations..."
rg -l "onGeoJsonFeatureUpdate|handleSubmit|setFields"
# Look for other components using selectedFeature with context
echo "Finding components using selectedFeature..."
rg "selectedFeature\??\." -A 2
# Check for geometry editing interactions
echo "Checking for geometry editing interactions..."
rg "onSketchGeometryEdit|geometry" -A 2
Length of output: 152075
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx
Outdated
Show resolved
Hide resolved
…/sketch-feature-inforbox
…rth-visualizer into fix/sketch-feature-inforbox
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx (1)
Learnt from: airslice
PR: reearth/reearth-visualizer#1248
File: web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx:125-125
Timestamp: 2024-11-14T01:32:53.695Z
Learning: When checking for early return conditions in useEffect hooks, group them together at the top before performing other operations, to improve code readability.
🪛 GitHub Check: ci-web / ci
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx
[warning] 1-1:
There should be no empty line within import group
[warning] 1-1:
react import should occur after import of @reearth/services/theme
🔇 Additional comments (2)
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx (2)
1-1: LGTM: Import statement updated correctly
The addition of useRef to the React imports is appropriate for the new functionality.
🧰 Tools
🪛 GitHub Check: ci-web / ci
[warning] 1-1:
There should be no empty line within import group
[warning] 1-1:
react import should occur after import of @reearth/services/theme
114-115: LGTM: Well-documented state tracking implementation
The useRef hook is appropriately used to track the previous state of fields, with clear comments explaining its purpose.
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx (2)
1-1: Reorder imports to follow project conventionsThe React import should be moved after the
@reearthimports to maintain consistency with the project's import ordering.-import { FC, useCallback, useEffect, useMemo, useRef, useState } from "react"; import "react18-json-view/src/style.css"; import "react18-json-view/src/dark.css"; import { GeoJsonFeatureDeleteProps, GeoJsonFeatureUpdateProps } from "@reearth/beta/features/Editor/hooks/useSketch"; import { Button, Collapse, Typography } from "@reearth/beta/lib/reearth-ui"; import { Geometry } from "@reearth/core"; import { NLSLayer, SketchFeature } from "@reearth/services/api/layersApi/utils"; import { useT } from "@reearth/services/i18n"; import { styled, useTheme } from "@reearth/services/theme"; +import { FC, useCallback, useEffect, useMemo, useRef, useState } from "react"; import JsonView from "react18-json-view";🧰 Tools
🪛 GitHub Check: ci-web / ci
[warning] 1-1:
There should be no empty line within import group
[warning] 1-1:
reactimport should occur after import of@reearth/services/theme
114-116: Add documentation for the field tracking mechanismAdd a comment block explaining the purpose of
previousFieldsref and how it's used to track field changes.+ // previousFields ref stores the previous state of fields to detect + // when custom properties are removed from the schema, allowing us to + // update the feature properties accordingly const previousFields = useRef<FieldProp[]>([]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx(2 hunks)
🧰 Additional context used
📓 Learnings (1)
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx (1)
Learnt from: airslice
PR: reearth/reearth-visualizer#1248
File: web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx:125-125
Timestamp: 2024-11-14T01:32:53.695Z
Learning: When checking for early return conditions in useEffect hooks, group them together at the top before performing other operations, to improve code readability.
🪛 GitHub Check: ci-web / ci
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx
[warning] 1-1:
There should be no empty line within import group
[warning] 1-1:
react import should occur after import of @reearth/services/theme
🔇 Additional comments (1)
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx (1)
117-146: Implementation looks good!
The field removal handling is well-implemented with:
- Proper early returns
- Clear field removal detection
- Efficient property updates
- Good documentation
Let's verify that this implementation correctly handles all property update scenarios.
✅ Verification successful
Let me try a different search strategy to verify the property update implementation.
Let me try one more search with more specific patterns and context lines to understand the property update implementations across the identified files.
Based on the search results, I can now provide a final response as we have sufficient evidence about the property update implementations.
Property update implementation is consistent and well-isolated
The implementation correctly handles feature property updates with proper safeguards:
- The
onGeoJsonFeatureUpdatecallback is used consistently for all property updates - Property updates are properly scoped within the FeatureInspector component
- Dependencies are correctly specified in the useEffect hook
- No conflicting property update patterns were found in parent components
The search results show that parent components (LayerInspector/index.tsx) only read properties for feature identification, while all modifications are properly channeled through the onGeoJsonFeatureUpdate callback in FeatureInspector.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the property update implementation
# Look for other places where feature properties are updated to ensure consistency
# Search for property update patterns
echo "Searching for feature property update patterns..."
rg -l "properties.*selectedFeature|onGeoJsonFeatureUpdate" --type typescript
# Search for potential race conditions in property updates
echo "Checking for potential race conditions..."
ast-grep --pattern 'useEffect($$$, [$$$selectedFeature$$$])'
Length of output: 378
Script:
#!/bin/bash
# Search for property update patterns without file type restriction
echo "Searching for feature property update patterns..."
rg -l "properties.*selectedFeature|onGeoJsonFeatureUpdate"
# Search for useEffect dependencies containing selectedFeature
echo "Checking for property-related effects..."
rg "useEffect.*selectedFeature.*\[" -A 2
# Look for other property update implementations
echo "Checking property update implementations..."
rg "onGeoJsonFeatureUpdate.*properties" -A 2
Length of output: 862
Script:
#!/bin/bash
# Check the implementation in FeatureInspector and related files
echo "Checking FeatureInspector implementation..."
rg "onGeoJsonFeatureUpdate" web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx -B 2 -A 2
# Look for similar property update patterns in other files
echo "Checking property updates in other files..."
rg "selectedFeature.*properties" web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/index.tsx -B 2 -A 2
rg "selectedFeature.*properties" web/src/beta/features/Editor/Map/InspectorPanel/index.tsx -B 2 -A 2
# Check for any other useEffect hooks handling selectedFeature
echo "Checking related useEffect implementations..."
ast-grep --pattern 'useEffect(() => {
$$$
selectedFeature$$$
$$$
}, [$$$])'
Length of output: 1762
Overview
I fixed the issue where the customize property list block in the Infobox didn't fetch properties until the page was restarted. I replaced
visualizer.current?.layers.selectedFeature()withselectedComputedFeature, which updates every time, ensuring that the properties are fetched correctly.What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
selectedFeatureprop in theCustomFieldscomponent for enhanced property evaluation.Bug Fixes
Refactor
Contentcomponent to conditionally render based on thedisplayTypeField.