feat(web): support edit sketch geometry on editor #1153
Conversation
WalkthroughThe pull request introduces updates to the Changes
Possibly related PRs
Suggested reviewers
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: 10
🧹 Outside diff range and nitpick comments (19)
web/src/beta/features/Visualizer/Crust/Plugins/pluginAPI/types/sketch.ts (1)
41-41: LGTM! Consider adding documentation for the new event type.The addition of the 'update' event to
SketchEventTypeis a good enhancement that aligns well with the PR objective of supporting edit sketch geometry. This non-breaking change extends the functionality of theSketchinterface in a consistent manner.To improve this further:
- Consider adding a brief comment explaining the purpose and usage of the 'update' event.
- Ensure that other parts of the codebase that use
SketchEventTypeare updated to handle this new event type if necessary.Would you like me to suggest a documentation comment for the
SketchEventType?web/src/beta/features/Visualizer/Crust/Plugins/hooks/useSketch.ts (1)
57-68: LGTM: New useEffect hook for sketch feature update event.The implementation of the new useEffect hook for handling the sketch plugin feature update event is well-structured and consistent with existing patterns. It correctly prevents multiple bindings and emits the event as expected.
Consider extracting the common logic for event binding into a custom hook to reduce code duplication. This would make the code more DRY and easier to maintain. For example:
const useEventBinding = (eventName, callback, emit) => { const eventBinded = useRef(false); useEffect(() => { if (!eventBinded.current && callback) { callback((e) => { emit(eventName, e); }); eventBinded.current = true; } }, [emit, callback, eventName]); }; // Usage useEventBinding("create", onSketchPluginFeatureCreate, emit); useEventBinding("update", onSketchPluginFeatureUpdate, emit); useEventBinding("toolChange", onSketchTypeChange, emit);This refactoring would simplify the code and make it easier to add new event bindings in the future.
web/src/beta/features/Editor/Map/context.tsx (1)
51-54: LGTM: New properties for sketch geometry editingThe additions to the
MapPageContextTypeinterface align well with the PR's objective of supporting sketch geometry editing. The new properties provide a clear structure for managing the editing feature and its associated actions.A minor suggestion for improvement:
Consider adding JSDoc comments to these new properties to provide more context about their usage. For example:
/** The currently active sketch editing feature, if any */ sketchEditingFeature?: SketchEditingFeature; /** Handler to initiate sketch geometry editing */ handleSketchGeometryEditStart: () => void; /** Handler to cancel the current sketch geometry edit */ handleSketchGeometryEditCancel: () => void; /** Handler to apply the current sketch geometry edit */ handleSketchGeometryEditApply: () => void;This would enhance the code's self-documentation and make it easier for other developers to understand and use these new features.
web/src/beta/features/Editor/hooks/useUI.ts (3)
16-20: LGTM! Consider adding JSDoc comments for new handlers.The new handlers
handleSketchTypeChangeandhandleSketchGeometryEditCancelare well-typed and align with the PR objective of supporting edit sketch geometry.To improve code maintainability, consider adding JSDoc comments for these new handlers. For example:
/** * Handles changes in the sketch type. * @param type - The new sketch type or undefined. * @param from - The source of the change, either "editor" or "plugin". */ handleSketchTypeChange: (type: SketchType | undefined, from: "editor" | "plugin") => void; /** * Handles cancellation of sketch geometry editing. * @param ignoreAutoReSelect - Optional flag to ignore auto re-selection. */ handleSketchGeometryEditCancel: (ignoreAutoReSelect?: boolean) => void;
59-68: LGTM! Consider extracting sketch reset logic.The integration of
handleSketchTypeChangeandhandleSketchGeometryEditCancelinhandleLayerSelectFromUIis appropriate. The dependency array is correctly updated to include these new handlers.To improve code readability and reusability, consider extracting the sketch reset logic into a separate function:
const resetSketchState = useCallback(() => { handleSketchTypeChange(undefined, "editor"); handleSketchGeometryEditCancel(true); }, [handleSketchTypeChange, handleSketchGeometryEditCancel]); const handleLayerSelectFromUI = useCallback( (layerId?: string) => { handleSceneSettingSelect(undefined); resetSketchState(); handleLayerSelect({ layerId }); }, [handleLayerSelect, handleSceneSettingSelect, resetSketchState] );This approach would make the code more modular and easier to maintain, especially if you need to reset the sketch state in other parts of the component.
82-94: LGTM! Consider applying the same refactoring as suggested forhandleLayerSelectFromUI.The integration of
handleSketchTypeChangeandhandleSketchGeometryEditCancelinhandleSceneSettingSelectFromUIis appropriate and consistent with the changes made tohandleLayerSelectFromUI. The dependency array is correctly updated to include these new handlers.To maintain consistency and improve code organization, consider applying the same refactoring suggested for
handleLayerSelectFromUI:const resetSketchState = useCallback(() => { handleSketchTypeChange(undefined, "editor"); handleSketchGeometryEditCancel(true); }, [handleSketchTypeChange, handleSketchGeometryEditCancel]); const handleSceneSettingSelectFromUI = useCallback( (collection?: string) => { handleLayerSelect(undefined); resetSketchState(); // change to select layer could effect the scene setting selection requestAnimationFrame(() => { handleSceneSettingSelect(collection); }); }, [handleLayerSelect, handleSceneSettingSelect, resetSketchState] );This approach would make the code more consistent, modular, and easier to maintain.
web/src/beta/features/Editor/Visualizer/index.tsx (2)
35-35: LGTM! Consider adding JSDoc for the new prop.The addition of
onSketchFeatureUpdateprop is well-implemented and aligns with the PR objective. It follows the existing patterns for sketch-related props.Consider adding a JSDoc comment for the
onSketchFeatureUpdateprop in thePropstype to improve code documentation:/** Callback function triggered when a sketch feature is updated */ onSketchFeatureUpdate?: (feature: SketchFeature | null) => void;Also applies to: 133-133
62-62: LGTM! Consider consistent naming for handler props.The new props are correctly added to the component and passed to the
Visualizer.For consistency, consider renaming
handleCoreAPIReadytoonCoreAPIReadywhen passing it to theVisualizercomponent, similar to howonVisualizerReadyis handled:onCoreAPIReady={onCoreAPIReady}This would maintain a consistent naming pattern across all callback props.
Also applies to: 65-65, 133-133, 135-135
web/package.json (1)
Line range hint
192-192: Approve addition of packageManager field.The addition of the packageManager field specifying "[email protected]" is a good practice. It ensures consistency across development environments and indicates that the project is keeping up with modern JavaScript tooling.
Consider updating the project's README or development setup documentation to mention the required Yarn version (4.4.0) for consistency across all development environments.
web/src/beta/features/Visualizer/Crust/Plugins/hooks/index.ts (2)
Line range hint
163-315: Potential issue: value object not updated with new parameterThe
valueobject created usinguseMemodoesn't seem to include the newonSketchPluginFeatureUpdateparameter. This might lead to the new functionality not being accessible through the context.Consider updating the
valueobject to include the new parameter:const value = useMemo<Context>( () => ({ reearth: commonReearth({ // ... existing properties ... sketchEventsOn, sketchEventsOff, + onSketchPluginFeatureUpdate, // ... rest of the properties ... }), // ... rest of the value object ... }), [ // ... existing dependencies ... sketchEventsOn, sketchEventsOff, + onSketchPluginFeatureUpdate, // ... rest of the dependencies ... ] );
Line range hint
1-315: Overall review: Good addition with a minor oversightThe addition of the
onSketchPluginFeatureUpdateparameter is well-integrated into the existing code structure. It's correctly added to the function props and passed to theuseSketchhook. However, there's a potential issue with thevalueobject in theuseMemohook not being updated to include this new parameter, which could prevent the new functionality from being accessible through the context.Please address the issue with the
valueobject as mentioned in the previous comment to ensure full functionality of the new feature.web/src/beta/features/Visualizer/index.tsx (2)
78-78: LGTM: New property for handling core API readiness.The addition of
handleCoreAPIReadyproperty is a good way to allow the parent component to react when the core API is ready. The property name and type definition are appropriate.Consider adding a brief JSDoc comment to explain when this callback is invoked, which could help other developers understand its purpose more quickly.
You could add a comment like this:
/** Callback function invoked when the core API is ready for use. */ handleCoreAPIReady?: () => void;
177-179: LGTM: Proper usage of new properties in the Visualizer component.The new properties
handleSketchFeatureUpdateandhandleCoreAPIReadyare correctly destructured from the props and properly assigned to theCoreVisualizercomponent. This implementation is consistent with the changes made to theVisualizerPropstype.To improve code organization, consider grouping related props together. For example, you could move
handleSketchFeatureUpdatenext tohandleSketchFeatureCreatein both the destructuring and theCoreVisualizerprops.Here's a suggestion for grouping related props:
const Visualizer: FC<VisualizerProps> = ({ // ... other props handleSketchTypeChange, handleSketchFeatureCreate, handleSketchFeatureUpdate, handleMount, handleCoreAPIReady, // ... rest of the props }) => { // ... component logic return ( // ... other JSX <CoreVisualizer // ... other props onSketchTypeChangeProp={handleSketchTypeChange} onSketchFeatureCreate={handleSketchFeatureCreate} onSketchFeatureUpdate={handleSketchFeatureUpdate} onMount={handleMount} onAPIReady={handleCoreAPIReady} // ... rest of the props > {/* ... child components */} </CoreVisualizer> ); };Also applies to: 242-242, 244-244
web/src/beta/features/Visualizer/Crust/index.tsx (2)
210-210: Consider adding an explicit type foronSketchPluginFeatureUpdate.While the addition of
onSketchPluginFeatureUpdateis consistent with the existing code style, it would be beneficial to add an explicit type definition for this prop. This would improve type safety and make the expected function signature clear to other developers.Consider updating the line as follows:
onSketchPluginFeatureUpdate?: (featureId: string, properties: unknown) => void;Please adjust the parameter types according to the actual requirements of the function.
210-210: Overall impact: New functionality added successfully.The changes successfully add support for updating sketch plugin features, aligning with the PR objective of supporting edit sketch geometry on the editor. The implementation follows established patterns in the codebase and introduces minimal changes.
Consider updating the component's documentation or adding inline comments to explain the purpose and usage of the new
onSketchPluginFeatureUpdatefunctionality. This will help other developers understand how to use this new feature in the future.Also applies to: 297-297
web/src/beta/features/Editor/hooks/useSketch.ts (2)
97-102: Check for unnecessary null checksIn the condition within
handleSketchFeatureCreate, you check both!featureand!feature?.properties?.id. Since you're already checking for!feature, the additional check forfeaturemay be redundant.Consider simplifying the condition:
if ( !feature || !selectedLayer?.layer?.id || !selectedLayer?.layer?.isSketch ) return;
Line range hint
122-134: Avoid including refs in dependency arraysIn the
useEffecthook, you includependingSketchSelectionRefandignoreCoreLayerUnselectin the dependency array. Since refs do not trigger re-renders and their.currentvalue changes do not cause the effect to re-run, they can be omitted from the dependencies.Consider removing them from the dependency array to prevent unnecessary effect executions:
}, [ nlsLayers, - pendingSketchSelectionRef, visualizerRef, - ignoreCoreLayerUnselect ]);web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx (2)
156-157: Simplify boolean expression by removing unnecessary double negationThe use of
!!is unnecessary here sincelayer?.isSketchis already a boolean value. Removing!!improves readability.Apply this diff:
- {!!layer?.isSketch && sketchFeature?.id && ( + {layer?.isSketch && sketchFeature?.id && (
186-190: Ensure accessibility by adding descriptive titles to buttonsTo improve accessibility for users relying on assistive technologies, add
titleprops to buttons with icon-only content.Apply this diff:
<Button onClick={handleDeleteSketchFeature} size="small" icon="trash" + title={t("Delete Feature")} extendWidth />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
web/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
📒 Files selected for processing (16)
- web/package.json (1 hunks)
- web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx (5 hunks)
- web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/index.tsx (5 hunks)
- web/src/beta/features/Editor/Map/InspectorPanel/index.tsx (2 hunks)
- web/src/beta/features/Editor/Map/context.tsx (2 hunks)
- web/src/beta/features/Editor/Visualizer/index.tsx (3 hunks)
- web/src/beta/features/Editor/hooks/index.ts (8 hunks)
- web/src/beta/features/Editor/hooks/useSketch.ts (6 hunks)
- web/src/beta/features/Editor/hooks/useUI.ts (4 hunks)
- web/src/beta/features/Editor/index.tsx (3 hunks)
- web/src/beta/features/Visualizer/Crust/Plugins/hooks/index.ts (2 hunks)
- web/src/beta/features/Visualizer/Crust/Plugins/hooks/useSketch.ts (2 hunks)
- web/src/beta/features/Visualizer/Crust/Plugins/pluginAPI/types/sketch.ts (1 hunks)
- web/src/beta/features/Visualizer/Crust/Plugins/types.ts (1 hunks)
- web/src/beta/features/Visualizer/Crust/index.tsx (2 hunks)
- web/src/beta/features/Visualizer/index.tsx (3 hunks)
🔇 Additional comments (37)
web/src/beta/features/Visualizer/Crust/Plugins/types.ts (1)
64-64: LGTM: New callback property added correctlyThe new
onSketchPluginFeatureUpdateproperty has been added correctly to thePropstype. It follows the existing pattern for callback properties in this type, such asonSketchPluginFeatureCreate. The type definition is consistent and appropriate for handling sketch feature updates.To ensure consistency and completeness, let's verify if there are any other related properties or usages we should consider:
This will help us confirm if there are any other sketch-related properties that might need similar updates or if this new property is used consistently across the codebase.
✅ Verification successful
LGTM: Callback property added consistently
The
onSketchPluginFeatureUpdateproperty has been correctly added to thePropstype. It aligns with existing callback patterns such asonSketchPluginFeatureCreateand maintains consistency across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar properties and their usages rg --type typescript 'onSketch.*?: \(cb: SketchEventCallback\) => void;?' -g '!*.test.ts'Length of output: 126
Script:
#!/bin/bash # Search for similar properties and their usages using correct type identifiers rg --type ts 'onSketch.*?: \(cb: SketchEventCallback\) => void;?' -g '!*.test.ts' -A 2 rg --type tsx 'onSketch.*?: \(cb: SketchEventCallback\) => void;?' -g '!*.test.tsx' -A 2Length of output: 713
Script:
#!/bin/bash # Search for similar properties and their usages using correct type identifiers rg --type ts 'onSketch.*?: \(cb: SketchEventCallback\) => void;?' -g '!*.test.ts' -A 2 rg 'onSketch.*?: \(cb: SketchEventCallback\) => void;?' -g '*.tsx' -g '!*.test.tsx' -A 2Length of output: 685
web/src/beta/features/Visualizer/Crust/Plugins/hooks/useSketch.ts (2)
11-11: LGTM: New parameter and type update for sketch feature update support.The addition of
onSketchPluginFeatureUpdateparameter and its inclusion in the Pick type is well-implemented. This change aligns with the PR objective of supporting edit sketch geometry on the editor.Also applies to: 15-18
Line range hint
1-114: Verify complete implementation of the sketch geometry editing feature.The changes in this file are well-implemented and focused on adding support for the sketch feature update event. This aligns with the PR objective of supporting edit sketch geometry on the editor.
To ensure the feature is fully implemented, please verify the following:
- Check if the
onSketchPluginFeatureUpdatecallback is properly passed down from parent components.- Confirm that the sketch geometry editing functionality is correctly implemented in the core system that this hook interacts with.
- Verify that the event emitted by this hook is properly handled by the plugin system.
Run the following script to check for the usage of
onSketchPluginFeatureUpdate:This will help ensure that the new functionality is properly integrated throughout the codebase.
web/src/beta/features/Editor/Map/context.tsx (2)
2-2: LGTM: New import for sketch editing featureThe addition of
SketchEditingFeatureto the imports is consistent with the PR's objective of supporting sketch geometry editing in the editor.
Line range hint
1-54: Summary: Changes align well with PR objectiveThe modifications to this file effectively support the PR's goal of enabling sketch geometry editing in the editor. The new imports and interface properties provide a clear and focused implementation for this feature. The changes are non-disruptive to existing code and maintain good structure within the file.
Some key points:
- The new
SketchEditingFeatureimport supports the added functionality.- The
sketchEditingFeatureproperty allows for managing the current editing state.- The three new handler functions (
handleSketchGeometryEditStart,handleSketchGeometryEditCancel,handleSketchGeometryEditApply) provide a comprehensive set of actions for sketch editing.These changes lay a solid foundation for implementing sketch geometry editing in the editor. However, as noted in the PR description, this feature is not yet exposed to the plugin API.
To ensure consistency across the codebase, let's verify the usage of these new properties:
This script will help identify where these new properties are being used or implemented, ensuring that the changes are properly integrated into the codebase.
✅ Verification successful
Verification Successful: Changes are Properly Integrated
The usage of the new properties and handlers (
sketchEditingFeature,handleSketchGeometryEditStart,handleSketchGeometryEditCancel,handleSketchGeometryEditApply) has been confirmed across the codebase. These additions are consistently integrated within relevant hooks and context files, aligning well with the PR's objective of enabling sketch geometry editing in the editor. No issues or inconsistencies were found during the verification process.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of new sketch editing properties and handlers # Search for usage of sketchEditingFeature echo "Searching for sketchEditingFeature usage:" rg --type typescript --type tsx 'sketchEditingFeature' # Search for usage of handleSketchGeometryEditStart echo "\nSearching for handleSketchGeometryEditStart usage:" rg --type typescript --type tsx 'handleSketchGeometryEditStart' # Search for usage of handleSketchGeometryEditCancel echo "\nSearching for handleSketchGeometryEditCancel usage:" rg --type typescript --type tsx 'handleSketchGeometryEditCancel' # Search for usage of handleSketchGeometryEditApply echo "\nSearching for handleSketchGeometryEditApply usage:" rg --type typescript --type tsx 'handleSketchGeometryEditApply'Length of output: 827
Script:
#!/bin/bash # Description: Check for usage of new sketch editing properties and handlers # Search for usage of sketchEditingFeature in .ts and .tsx files echo "Searching for sketchEditingFeature usage:" rg 'sketchEditingFeature' -g '*.ts' -g '*.tsx' # Search for usage of handleSketchGeometryEditStart in .ts and .tsx files echo "\nSearching for handleSketchGeometryEditStart usage:" rg 'handleSketchGeometryEditStart' -g '*.ts' -g '*.tsx' # Search for usage of handleSketchGeometryEditCancel in .ts and .tsx files echo "\nSearching for handleSketchGeometryEditCancel usage:" rg 'handleSketchGeometryEditCancel' -g '*.ts' -g '*.tsx' # Search for usage of handleSketchGeometryEditApply in .ts and .tsx files echo "\nSearching for handleSketchGeometryEditApply usage:" rg 'handleSketchGeometryEditApply' -g '*.ts' -g '*.tsx'Length of output: 4390
web/src/beta/features/Editor/hooks/useUI.ts (2)
27-29: LGTM! Proper integration of new handlers.The new handlers
handleSketchTypeChangeandhandleSketchGeometryEditCancelare correctly destructured from the props, ensuring they are available for use within the component.
Line range hint
1-158: Overall, the changes effectively support editing sketch geometry.The modifications to
useUI.tsalign well with the PR objective of supporting edit sketch geometry in the editor. The new handlers for sketch type changes and geometry edit cancellations are properly integrated into the existing UI management logic.Key points:
- The
Propstype is correctly extended with new handlers.- The new handlers are properly destructured and used in the component.
- Sketch state is consistently reset when selecting layers or scene settings.
The suggested refactorings and documentation improvements would further enhance the code's maintainability and readability. Consider implementing these changes to make the codebase more robust for future development.
Great job on implementing this feature!
web/src/beta/features/Editor/Visualizer/index.tsx (2)
Line range hint
1-164: Overall, the changes look good and align with the PR objectives.The additions to support editing sketch geometry are well-implemented. The new props and their integration into the
EditorVisualizercomponent follow existing patterns and conventions.A few minor suggestions have been made:
- Adding JSDoc for the new
onSketchFeatureUpdateprop.- Clarifying the purpose of the
onCoreAPIReadyprop.- Ensuring consistent naming conventions for handler props.
These changes enhance the component's functionality while maintaining code quality and consistency.
38-38: LGTM! Please clarify the purpose ofonCoreAPIReady.The addition of
onCoreAPIReadyprop is well-implemented. However, its purpose is not immediately clear from the PR objectives.Could you please provide more context on the need for this new prop? How does it relate to the main objective of supporting edit sketch geometry?
To help verify the usage of this new prop, I've prepared a script to search for its occurrences:
Also applies to: 135-135
✅ Verification successful
Verified: The
onCoreAPIReadyprop is correctly implemented and utilized within theVisualizercomponent.
- Declared as an optional callback in
Visualizer/index.tsx.- Passed from
Editor/index.tsxtoVisualizer.- Consistently used within the
Visualizercomponent.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of onCoreAPIReady prop # Expected result: Occurrences of onCoreAPIReady in component props or function calls rg --type typescript --type tsx 'onCoreAPIReady'Length of output: 83
Script:
#!/bin/bash # Description: Search for usages of onCoreAPIReady prop # Expected result: Occurrences of onCoreAPIReady in component props or function calls rg --type ts --type tsx 'onCoreAPIReady'Length of output: 68
Script:
#!/bin/bash # Description: Search for usages of onCoreAPIReady prop # Expected result: Occurrences of onCoreAPIReady in .ts and .tsx files rg 'onCoreAPIReady' --glob '*.ts' --glob '*.tsx'Length of output: 378
web/src/beta/features/Editor/index.tsx (4)
43-43: LGTM: Addition ofhandleSketchFeatureUpdateThe addition of
handleSketchFeatureUpdateto the destructured values fromuseHooksis consistent with the PR's objective of supporting sketch geometry editing. The naming convention follows the established pattern in the codebase.
Line range hint
1-180: Summary: Changes align with PR objective, minor clarifications neededThe changes in this file successfully implement the necessary handlers and props to support editing sketch geometry in the editor. The code follows established patterns and naming conventions. However, a few points require attention:
- Clarification on the purpose of
handleCoreAPIReadywould be helpful.- Verification of prop types for EditorVisualizer is recommended to ensure type safety.
Overall, the implementation looks solid and achieves the PR's objective.
93-93: LGTM: New props added to EditorVisualizerThe addition of
onSketchFeatureUpdateandonCoreAPIReadyprops to the EditorVisualizer component corresponds to the new handlers destructured fromuseHooks. The naming convention follows the established pattern for event handler props.To ensure type safety, let's verify the prop types for EditorVisualizer:
#!/bin/bash # Search for the EditorVisualizer component definition and its prop types rg --type typescript 'type Props = \{' -A 20 $(fd -t f 'EditorVisualizer.(ts|tsx)' | head -n 1)Also applies to: 96-96
62-63: LGTM: Addition ofhandleCoreAPIReady, but clarification neededThe addition of
handleCoreAPIReadyto the destructured values fromuseHooksfollows the established naming convention. However, could you please clarify the purpose of this new handler? It's not immediately clear from the PR objectives why this is needed for editing sketch geometry.To better understand the usage of
handleCoreAPIReady, let's check its implementation:✅ Verification successful
LGTM: Addition of
handleCoreAPIReadyconfirmedThe
handleCoreAPIReadyfunction is implemented inhooks/index.tsand is used appropriately within theEditorandVisualizercomponents to manage the core API's readiness. This addition aligns with the project's established patterns.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for the implementation of handleCoreAPIReady rg --type typescript 'const handleCoreAPIReady' -A 10Length of output: 191
Script:
#!/bin/bash # Search for any occurrences of handleCoreAPIReady in .ts and .tsx files fd -e ts -e tsx | xargs rg 'handleCoreAPIReady'Length of output: 676
web/package.json (3)
127-127: Approve @reearth/core dependency update.The update of @reearth/core from 0.0.7-alpha.14 to 0.0.7-alpha.15 aligns with the PR objective of utilizing a new version of the core system. This minor version update likely contains new features or bug fixes necessary for the new sketch geometry editing functionality.
Please ensure that this update is compatible with the rest of the codebase and doesn't introduce any breaking changes. Consider running the test suite and manually testing the new sketch geometry editing feature to verify compatibility.
Line range hint
1-3: Approve package version increment.The increment of the package version from 1.0.0-alpha.5 to 1.0.0-alpha.6 is appropriate for the introduction of the new sketch geometry editing feature. This version update correctly reflects the ongoing development of the project.
Line range hint
1-192: Summary of package.json changes.The changes in this file are well-aligned with the PR objectives:
- The @reearth/core dependency update supports the new sketch geometry editing feature.
- The package version increment correctly reflects the addition of this new feature.
- The specification of the package manager version promotes consistency in the development environment.
These changes follow good practices and contribute to the overall improvement of the project. Please ensure to address the suggestions regarding compatibility verification and documentation updates.
web/src/beta/features/Visualizer/Crust/Plugins/hooks/index.ts (2)
36-36: LGTM: New parameter added correctlyThe addition of the
onSketchPluginFeatureUpdateparameter to the function's props is correct and consistent with the existing code structure.
146-146: LGTM: Parameter passed to useSketch hookThe
onSketchPluginFeatureUpdateparameter is correctly passed to theuseSketchhook.web/src/beta/features/Visualizer/index.tsx (2)
76-76: LGTM: New property for handling sketch feature updates.The addition of
handleSketchFeatureUpdateproperty is consistent with the PR's objective of supporting sketch geometry editing. The property name and type definition are appropriate and follow the existing conventions in the component.
Line range hint
1-324: Summary: Successfully implemented sketch geometry editing supportThe changes in this file successfully implement support for editing sketch feature geometry, aligning with the PR objectives. The new properties and their usage in the
Visualizercomponent are well-implemented and consistent with existing code patterns.Key points:
- Added
handleSketchFeatureUpdateto handle sketch feature updates.- Introduced
handleCoreAPIReadyto react when the core API is ready.- Properly integrated these new handlers into the
CoreVisualizercomponent.The code is clean, well-structured, and follows existing conventions. Minor suggestions for improvement include adding JSDoc comments and grouping related props for better organization.
Overall, this implementation effectively supports the new feature while maintaining code quality and consistency.
web/src/beta/features/Visualizer/Crust/index.tsx (1)
297-297: LGTM! VerifyPluginscomponent update.The addition of
onSketchPluginFeatureUpdateto thePluginscomponent is consistent with the existing code structure and how other similar props are handled.To ensure full compatibility, please verify that the
Pluginscomponent has been updated to accept and properly utilize this new prop. You can run the following command to check thePluginscomponent definition:This will help confirm that the
Pluginscomponent's prop types have been updated accordingly.✅ Verification successful
Verified
Pluginscomponent properly handlesonSketchPluginFeatureUpdate.The
Pluginscomponent receives theonSketchPluginFeatureUpdateprop and correctly utilizes it within the hooks.
- File:
web/src/beta/features/Visualizer/Crust/Plugins/index.tsx- Hook implementation:
web/src/beta/features/Visualizer/Crust/Plugins/hooks/useSketch.ts🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
rg -t typescript 'export default function Plugins' --context 10Length of output: 100
Script:
#!/bin/bash # Searching for 'export default function Plugins' in TypeScript files rg 'export default function Plugins' --type ts --type tsx --context 10Length of output: 100
Script:
#!/bin/bash # Searching for 'export default function Plugins' in .ts and .tsx files rg 'export default function Plugins' -g '*.ts' -g '*.tsx' --context 10Length of output: 1249
Script:
#!/bin/bash # Searching for 'onSketchPluginFeatureUpdate' in Props type definition rg 'type Props' -g '*.ts' -g '*.tsx' --context 10 web/src/beta/features/Visualizer/Crust/Plugins/index.tsx # Searching for usage of 'onSketchPluginFeatureUpdate' within Plugins component rg 'onSketchPluginFeatureUpdate' -g '*.ts' -g '*.tsx' --context 10 web/src/beta/features/Visualizer/Crust/Plugins/Length of output: 11531
web/src/beta/features/Editor/Map/InspectorPanel/index.tsx (1)
21-28: LGTM: Sketch editing functionality integrated correctlyThe addition of
sketchEditingFeatureand the associated handlers fromuseMapPageis consistent and aligns with the intended sketch editing feature enhancement.web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/index.tsx (3)
30-33: Confirmed addition of new props for sketch editing featuresThe new props
sketchEditingFeature,onSketchGeometryEditStart,onSketchGeometryEditCancel, andonSketchGeometryEditApplyhave been correctly added to thePropstype to support sketch geometry editing functionality.
99-102: Ensure IDs compared inisEditingGeometryare of the same typeWhen comparing IDs using the strict equality operator
===inisEditingGeometry, it's important to ensure thatselectedSketchFeature?.properties?.idandsketchEditingFeature?.feature?.idare of the same type (e.g., both strings or both numbers) to avoid unintended comparison results due to type differences.
99-107: VerifyFeatureInspectorcomponent handles optional props appropriatelyThe
FeatureInspectorcomponent is receiving new optional props:isEditingGeometry,onSketchGeometryEditStart,onSketchGeometryEditApply, andonSketchGeometryEditCancel. Please confirm thatFeatureInspectorhandles these optional props correctly, including cases where they might be undefined.web/src/beta/features/Editor/hooks/useSketch.ts (2)
239-239: Confirm callback signature compatibilityPassing the state setter
setSketchEditingFeaturedirectly toonEditFeatureChangeassumes that the callback receives a single argument matching the state value. Ensure thatonEditFeatureChangeexpects a function with the signature(feature: SketchEditingFeature | undefined) => void.If the callback signature differs, wrap the setter in a function to match the expected signature:
- visualizerRef.current?.sketch.onEditFeatureChange(setSketchEditingFeature); + visualizerRef.current?.sketch.onEditFeatureChange(feature => setSketchEditingFeature(feature));
83-83: EnsureselectedLayer.layer.idis defined before usageIn the
handleSketchLayerAddfunction, you check forselectedLayer?.layer?.id, but you might also need to ensure thatinp.layerIdcorresponds to it.Confirm that
inp.layerIdis correctly set and consistent withselectedLayer.layer.id. If not, consider usingselectedLayer.layer.iddirectly:if (!selectedLayer?.layer?.id) return; await useAddGeoJsonFeature({ - layerId: inp.layerId, + layerId: selectedLayer.layer.id, geometry: inp.geometry, type: inp.type, properties: inp.properties });web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx (1)
Line range hint
121-125: Ensure properties are properly updated inhandleSubmitThe
handleSubmitfunction correctly updates the feature's properties. Ensure that custom properties are validated as needed before submission to prevent invalid data.web/src/beta/features/Editor/hooks/index.ts (9)
1-1: ImportinguseCallbackis AppropriateThe addition of
useCallbackto the import statement is necessary for the new callbacks defined in the code.
62-70: Destructuring New Handlers fromuseSketchThe inclusion of new handlers and properties (
handleSketchFeatureUpdate,handleGeoJsonFeatureDelete,sketchEditingFeature,handleSketchGeometryEditStart,handleSketchGeometryEditCancel,handleSketchGeometryEditApply,initSketch) fromuseSketchenhances the sketch editing functionality.
74-74: PassingselectedLayertouseSketchBy passing
selectedLayertouseSketch, you ensure that the sketch functionalities are aware of the currently selected layer.
119-121: VerifyuseUIReceives New Handlers CorrectlyEnsure that
useUIis updated to accept and utilize the new handlers (handleSceneSettingSelect,handleSketchTypeChange,handleSketchGeometryEditCancel) appropriately, enhancing UI interactions.
173-176: Including Sketch Editing Properties inmapPageValueAdding
sketchEditingFeatureand associated handlers tomapPageValuecontext allows map components to access sketch editing functionalities, which is beneficial for feature integration.
213-216: Dependency Inclusion inuseMemoformapPageValueIncluding
sketchEditingFeatureand related handlers in the dependency array ofuseMemoensures that changes to these variables correctly trigger re-computation, preventing stale state issues.
316-318: Confirm Dependencies inhandleCoreAPIReadyThe
handleCoreAPIReadycallback depends oninitSketch. Verify thatinitSketchand its dependencies are stable or include necessary dependencies to prevent issues with stale closures.
334-334: ExposinghandleSketchFeatureUpdateBy returning
handleSketchFeatureUpdate, the component allows for external updates to sketch features, facilitating better control from parent components or other hooks.
353-354: ReturninghandleCoreAPIReadyfor Core API IntegrationExposing
handleCoreAPIReadyenables parent components or systems to initialize the sketch functionalities once the core API is ready, ensuring proper sequencing in the application flow.
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx
Outdated
Show resolved
Hide resolved
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx
Show resolved
Hide resolved
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
web/src/beta/features/Editor/hooks/useSketch.ts (1)
211-222: Ensure visualizer reference is valid before invokingeditFeatureWhile optional chaining is used, it's important to ensure that
visualizerRef.currentandvisualizerRef.current.sketchare initialized before callingeditFeatureto prevent potential runtime errors.Consider adding a safety check:
const handleEditSketchFeature = useCallback(() => { if ( !selectedLayer?.layer?.id || !selectedLayer?.computedFeature?.id || !selectedLayer?.layer?.isSketch ) return; + if (!visualizerRef.current?.sketch) return; visualizerRef.current?.sketch.editFeature({ layerId: selectedLayer.layer.id, feature: selectedLayer.computedFeature }); }, [visualizerRef, selectedLayer]);
📜 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 (5 hunks)
- web/src/beta/features/Editor/hooks/useSketch.ts (6 hunks)
- web/src/services/i18n/translations/en.yml (0 hunks)
- web/src/services/i18n/translations/ja.yml (0 hunks)
💤 Files with no reviewable changes (2)
- web/src/services/i18n/translations/en.yml
- web/src/services/i18n/translations/ja.yml
🔇 Additional comments (10)
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx (4)
Line range hint
121-129: Approve geometry inclusion in feature update.The inclusion of the
geometryproperty in theonGeoJsonFeatureUpdatecall is a good addition, ensuring that both properties and geometry are updated when submitting changes.For improved clarity, consider adding a comment explaining the purpose of including the geometry:
onGeoJsonFeatureUpdate?.({ layerId: layer.id, featureId: sketchFeature.id, geometry: selectedFeature.geometry, // Include geometry to support editing properties: newProperties });
156-191: Approve new sketch feature buttons with a suggestion.The addition of sketch feature buttons enhances the user interface, providing clear actions for editing and deleting sketch features. The conditional rendering based on the
isEditingGeometrystate is well-implemented.As suggested in a previous review, consider adding a confirmation dialog before deleting a sketch feature to prevent accidental deletions and improve user experience.
Example implementation:
const handleDeleteSketchFeature = useCallback(() => { if (!layer?.id || !sketchFeature?.id) return; if (window.confirm(t("Are you sure you want to delete this sketch feature?"))) { onGeoJsonFeatureDelete?.({ layerId: layer.id, featureId: sketchFeature.id }); } }, [layer?.id, sketchFeature?.id, onGeoJsonFeatureDelete, t]);
284-288: Approve styling with suggestion for potential refactor.The
SketchFeatureButtonsstyled component provides a clean and consistent layout for the sketch feature buttons.As suggested in a previous review, consider refactoring styled components to reduce potential duplication. If there are similar button group styles elsewhere in the codebase, you could create a shared base component or use styled-component inheritance:
const ButtonGroup = styled("div")(({ theme }) => ({ display: "flex", flexDirection: "row", gap: theme.spacing.small })); const SketchFeatureButtons = styled(ButtonGroup)` // Add any specific styles for SketchFeatureButtons here `;This approach would promote consistency and reduce code duplication across the application.
28-31: 🛠️ Refactor suggestionConsider grouping geometry editing props for improved API.
The addition of geometry editing props enhances the component's functionality. However, to improve the component's API and reduce prop drilling, consider grouping these related props into a single
geometryEditobject as suggested in a previous review.Apply this diff to refactor the props:
type Props = { // ... other props - isEditingGeometry?: boolean; - onSketchGeometryEditStart?: () => void; - onSketchGeometryEditApply?: () => void; - onSketchGeometryEditCancel?: () => void; + geometryEdit?: { + isEditing: boolean; + onStart: () => void; + onApply: () => void; + onCancel: () => void; + }; };This change will make the component's API cleaner and more maintainable.
Likely invalid or redundant comment.
web/src/beta/features/Editor/hooks/useSketch.ts (6)
2-7: Imports are well-organized and necessaryThe new imports from
@reearth/coreinclude required types for the added functionality.
97-106: Conditional checks are correctly structuredThe conditional statement properly checks for necessary conditions before proceeding with adding a sketch feature.
207-209: State declaration forsketchEditingFeatureis appropriateThe state hook correctly initializes
sketchEditingFeaturewith the appropriate type.
235-240: Initialize sketch options and handlers correctlyThe
initSketchfunction sets up the sketch options and registers the editing feature change handler appropriately.
250-252: 🛠️ Refactor suggestionInconsistent naming of handler functions
The exported function names differ from the internal handler function names, which may cause confusion:
handleSketchGeometryEditStartmaps tohandleEditSketchFeaturehandleSketchGeometryEditCancelmaps tohandleCancelEditSketchFeaturehandleSketchGeometryEditApplymaps tohandleApplyEditSketchFeatureConsider aligning the naming conventions for consistency and readability. For example, rename the exported functions to match the internal handler names:
Apply this diff to rename the exported functions:
return { sketchType, sketchEditingFeature, handleSketchTypeChange, handleSketchFeatureCreate, handleSketchFeatureUpdate, handleGeoJsonFeatureUpdate, handleGeoJsonFeatureDelete, - handleSketchGeometryEditStart: handleEditSketchFeature, - handleSketchGeometryEditCancel: handleCancelEditSketchFeature, - handleSketchGeometryEditApply: handleApplyEditSketchFeature, + handleEditSketchFeature, + handleCancelEditSketchFeature, + handleApplyEditSketchFeature, initSketch };Likely invalid or redundant comment.
20-25: Ensure consistent type usage forselectedLayerThe
selectedLayertype inPropshas been updated toSelectedLayer | undefined. Verify that all references toselectedLayerwithin the code are compatible with the new type, and that any properties accessed are valid forSelectedLayer.To check for potential type inconsistencies, run:
✅ Verification successful
Consistent type usage for
selectedLayeris confirmedAll references to
selectedLayerutilize optional chaining, ensuring compatibility with the updated typeSelectedLayer | undefined. No type inconsistencies found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of `selectedLayer` and check for type compatibility. # Test: Search for `selectedLayer` references in TypeScript files. rg --type ts --word-regexp 'selectedLayer\.\w+' -A 2 -B 2Length of output: 11030
web/src/beta/features/Editor/Map/InspectorPanel/LayerInspector/FeatureInspector/index.tsx
Show resolved
Hide resolved
Overview
This PR adds the support for edit sketch feature geometry using new version of core.
What I've done
What I haven't done
Didn't expost to plugin API.
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
These updates enhance user experience by providing more robust editing tools and improving interactivity within the application.