refactor(web): infobox's customized property list. #1317
Conversation
WalkthroughThis pull request introduces enhanced editing capabilities for the Property List feature in the Visualizer's Infobox. The changes span across three files ( Changes
Sequence DiagramsequenceDiagram
participant User
participant ListEditor
participant EditorItem
User->>ListEditor: Double-click property
ListEditor->>EditorItem: Set edit mode
EditorItem->>ListEditor: Update property value
ListEditor->>User: Reflect changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 2
🧹 Nitpick comments (5)
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/hooks.ts (2)
53-55: Consider adding performance optimizationsWhile the simplified useEffect is more maintainable, consider adding debouncing if
propertyListField.valueupdates frequently to prevent unnecessary re-renders.useEffect(() => { + const timeoutId = setTimeout(() => { setCurrentPropertyList(propertyListField?.value ?? []); + }, 300); + return () => clearTimeout(timeoutId); }, [propertyListField?.value]);
154-162: Add cleanup for edit statesConsider adding a cleanup effect to reset edit states when the component unmounts or when switching between properties.
+useEffect(() => { + return () => { + setEditKeyIndex(null); + setEditValueIndex(null); + }; +}, []); const handleDoubleClick = useCallback((idx: number, field: string) => { if (field === "key") { setEditKeyIndex(idx); setEditValueIndex(null); } else if (field === "value") { setEditValueIndex(idx); setEditKeyIndex(null); } }, []);web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/Item.tsx (1)
29-33: Consider using useEffect for state synchronizationWhile the current state initialization is functional, consider adding a useEffect to keep the state in sync with prop changes:
+ useEffect(() => { + setCurrentKeyItem(item.key); + setCurrentValueItem(item.value); + }, [item.key, item.value]);This ensures the local state stays synchronized if the parent component updates the item prop.
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/index.tsx (2)
81-83: Consider adding error handling for the property list updateWhile the new handlers enhance the editing functionality, consider adding error handling for the update operation to improve user feedback and reliability.
- handlePropertyListUpdate + async handlePropertyListUpdate(propertyList: PropertyListItem[] | undefined) { + try { + await onPropertyUpdate?.(propertyId, /* other required params */); + // Show success feedback + } catch (error) { + // Show error feedback + console.error('Failed to update property list:', error); + } + }
136-151: Consider enhancing button accessibilityWhile the buttons are well-implemented, consider adding ARIA labels and keyboard shortcuts for better accessibility.
<Button title={t("Save")} icon="floppyDisk" size="small" + aria-label={t("Save property list")} + accessKey="s" onClick={() => handlePropertyListUpdate(currentPropertyList)} extendWidth />
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/Item.tsx(4 hunks)web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/hooks.ts(6 hunks)web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/index.tsx(4 hunks)
🔇 Additional comments (11)
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/hooks.ts (2)
33-34: LGTM: Well-typed state variables for edit tracking
The addition of editKeyIndex and editValueIndex state variables provides clear tracking of which property is being edited.
Line range hint 118-136: Verify state consistency after removing async updates
The removal of async updates in handleItemDrop could lead to state inconsistencies with the backend. Consider implementing a state reconciliation mechanism.
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/Item.tsx (4)
3-3: LGTM: Type definitions and imports are well-structured
The new props (isEditKey, isEditValue) and onDoubleClick handler are properly typed and align with the enhanced editing functionality requirements.
Also applies to: 10-11, 16-16
Line range hint 35-55: LGTM: Event handlers are well-implemented
The event handlers are properly memoized and follow React best practices. The separation of concerns between local state management and parent callbacks is clear.
121-132: LGTM: Styled components are well-implemented
The styled components make good use of the theme system and provide appropriate styling for both layout and text display. The overflow handling in TextWrapper is particularly well-considered.
Line range hint 1-132: Verify integration with save button functionality
While this component handles the editing UI well, let's verify its integration with the save button mentioned in the PR objectives.
✅ Verification successful
Integration with save functionality is properly implemented
The component is well-integrated with the save functionality:
- Individual edits in
EditorItemare handled throughonKeyBlurandonValueBlurcallbacks - These callbacks are connected to parent's
handleKeyBlurandhandleValueBlurfunctions - Changes are collected in
currentPropertyListstate - A save button with "Save" text and "floppyDisk" icon triggers
handlePropertyListUpdatewith the current list state - The entire flow maintains proper state management between individual item edits and final list updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for save button implementation in parent components
rg -l "saveButton|handleSave" "web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/"
# Check for event handler connections
ast-grep --pattern 'onBlur={$_}' "web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/"
Length of output: 216
Script:
#!/bin/bash
# Let's try a broader search for save-related functionality in the PropertyList directory
rg -A 5 "save|Save" "web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/"
# Search for components that might be importing or using this EditorItem
fd . "web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/" -t f -e tsx -e ts --exec rg -l "EditorItem"
# Look for state management or update handlers in the parent components
rg -A 5 "onChange|onUpdate|setState" "web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/"
Length of output: 3567
Script:
#!/bin/bash
# Let's examine the parent component's implementation to understand how updates are handled
rg -A 10 "handlePropertyListUpdate" "web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/index.tsx"
# Check how the current property list state is managed
rg -A 10 "currentPropertyList" "web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/index.tsx"
# Look for the connection between item updates and list updates
rg -A 5 "onKeyBlur|onValueBlur" "web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/index.tsx"
Length of output: 2884
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/index.tsx (5)
73-74: LGTM: Well-structured state management for edit modes
The addition of separate indices for key and value editing provides clear state management and enables independent editing of fields.
99-104: LGTM: Well-implemented editing controls
The addition of edit state props and double-click functionality provides a smooth and intuitive editing experience.
109-109: LGTM: Comprehensive dependency array
The useMemo dependency array correctly includes all required state variables and handlers.
172-180: LGTM: Clean styling implementation
The styling updates are well-structured and follow the theme system consistently. The boxSizing property is correctly implemented.
144-150: Verify save functionality integration
Let's verify the integration of the save functionality with similar implementations in the codebase.
✅ Verification successful
Based on the gathered information, I can now provide a final response. The code shows a complete chain of property update functionality:
Save functionality is properly integrated
The save button is correctly wired to a complete update chain:
handlePropertyListUpdatecallshandlePropertyValueUpdatewith the current listhandlePropertyValueUpdateinvokes theonPropertyUpdatecallback with proper parameters- The update is ultimately handled by the
updatePropertyValueGraphQL mutation
The implementation follows the established pattern used across multiple components in the codebase, with proper error handling and type safety.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar save button implementations and property update patterns
rg -A 3 "handlePropertyListUpdate|onPropertyUpdate" --type tsx
Length of output: 92
Script:
#!/bin/bash
# Search for similar save button implementations and property update patterns with TypeScript/React files
rg -t ts -t tsx -A 3 "handlePropertyListUpdate|onPropertyUpdate"
# Also search for similar patterns with different naming conventions
rg -t ts -t tsx -A 3 "updateProperty|saveProperty|handleSave.*Property"
Length of output: 196
Script:
#!/bin/bash
# Search for the file containing the handlePropertyListUpdate function
fd . -e ts -e tsx --exec grep -l "handlePropertyListUpdate"
# Search for the implementation of handlePropertyListUpdate
ast-grep --pattern 'const handlePropertyListUpdate = $_'
# Look for similar property update patterns in the codebase
rg "handlePropertyListUpdate|updateProperty|saveProperty" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts
Length of output: 53477
Script:
#!/bin/bash
# Let's check the implementation of handlePropertyValueUpdate which is called by handlePropertyListUpdate
ast-grep --pattern 'const handlePropertyValueUpdate = $_'
# Also check for the mutation usage
rg "useMutation.*UPDATE_PROPERTY_VALUE" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts -A 2
Length of output: 18097
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/hooks.ts
Show resolved
Hide resolved
web/src/beta/features/Visualizer/Crust/Infobox/Block/builtin/PropertyList/ListEditor/Item.tsx
Show resolved
Hide resolved
…actor/inforbox-block
Overview
Related story
https://www.notion.so/eukarya/visualizer-reearth-io-User-Stories-V2-13a16e0fb165800ba026f7569c8dd4b2?p=14316e0fb16580c381ccc123fa58ea07&pm=s
https://www.notion.so/eukarya/visualizer-reearth-io-User-Stories-V2-13a16e0fb165800ba026f7569c8dd4b2?p=13b16e0fb16580aaa5d5dec5b1b87fb5&pm=s
What I've done
Before
After
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Bug Fixes
Style