fix(web): selection lost when update layer style etc. #1192
Conversation
WalkthroughThe changes in this pull request primarily involve the renaming of the function 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: 0
🧹 Outside diff range and nitpick comments (3)
web/src/beta/features/Editor/hooks/useUISelection.ts (2)
Line range hint
3-8: Update the TODO comment with more context.The TODO comment about uniting selection handlers could be more descriptive about what specific aspects need refactoring and why.
-// TODO: Refactor to unite selection handlers +// TODO: Refactor to unify selection handlers to reduce complexity and prevent selection state conflicts
Line range hint
1-43: Consider implementing a selection state machine.The current implementation manages multiple independent selection states through separate handlers. Consider implementing a state machine pattern to:
- Make selection state transitions more explicit
- Prevent invalid selection states
- Simplify the logic for handling multiple selections
Would you like me to propose a state machine implementation for this use case?
web/src/beta/features/Editor/hooks/useSketch.ts (1)
123-125: LGTM! The conditional check prevents unnecessary selection clearing.The added condition improves the behavior by only clearing selection when there's a pending sketch selection. This helps prevent unwanted selection loss.
Consider refactoring this workaround in the future to use a more robust approach, such as:
- Using a state machine to manage selection states
- Implementing proper event handling for layer updates
- Using React's state management instead of refs for better predictability
📜 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 (5)
- web/src/beta/features/Editor/hooks/index.ts (2 hunks)
- web/src/beta/features/Editor/hooks/useSketch.ts (1 hunks)
- web/src/beta/features/Editor/hooks/useUI.ts (2 hunks)
- web/src/beta/features/Editor/hooks/useUISelection.ts (1 hunks)
- web/src/beta/features/Editor/index.tsx (2 hunks)
🧰 Additional context used
🔇 Additional comments (10)
web/src/beta/features/Editor/hooks/useUISelection.ts (2)
19-19: LGTM! Selection handling improvements look good.The changes correctly maintain independent selection states by removing unnecessary clearing of selections. This aligns with the PR objective of fixing selection loss when updating layer styles.
Also applies to: 27-27, 35-35
16-20: Verify the impact of removing layer selection reset.The
handleLayerStyleSelectedno longer clears layer and scene setting selections. While this appears to be the intended behavior to fix the selection loss issue, please verify that this doesn't introduce any edge cases where selections should be cleared.✅ Verification successful
Let me try a different approach to gather more context about the layer style selection behavior.
Let me try one more search to find any related code that might show the intent or impact of layer style selection:
Based on the code analysis, I can now provide a final response:
The removal of selection clearing in layer style selection is intentionally safe
Looking at the
useUISelectionhook implementation, we can see that selection clearing is handled differently for each type of selection:
- When selecting a layer (
handleLayerSelected), it clears scene settings selection- When selecting scene settings (
handleSceneSettingSelected), it clears layer selection- Layer style selection (
handleLayerStyleSelected) was designed to be independent and doesn't need to clear other selectionsThis pattern shows that selections are only cleared when they are mutually exclusive. Layer styles can coexist with other selections since they apply to the currently selected layer, making the removal of clearing behavior the correct approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any comments or tests that might indicate the original intent # of clearing selections when style is selected rg -g '!*.{lock,json,md}' -i "style.*select.*clear|clear.*style.*select" # Look for related test files to verify the behavior fd -g "*test*" -g "*spec*" -g "*stories*" --exec grep -l "LayerStyle.*Select"Length of output: 304
Script:
#!/bin/bash # Search for handleLayerStyleSelect implementation ast-grep --pattern 'const handleLayerStyleSelect = $_' # Search for any files containing LayerStyle selection related code rg -g '!*.{lock,json,md}' "LayerStyle.*select" -B 2 -A 2 # Look for test files specifically in the web directory fd -t f -e test.ts -e test.tsx -e spec.ts -e spec.tsx -e stories.tsx . web/Length of output: 87581
Script:
#!/bin/bash # Look for the implementation and usage of layer style selection ast-grep --pattern 'handleLayerStyleSelect' # Search for any code related to layer style selection and clearing/resetting rg -g '!*.{lock,json,md}' "(clearSelection|resetSelection|clearLayer|resetLayer).*style|style.*(clearSelection|resetSelection|clearLayer|resetLayer)" -B 2 -A 2 # Look for the implementation of useUISelection hook ast-grep --pattern 'const useUISelection = $_'Length of output: 3933
web/src/beta/features/Editor/hooks/useUI.ts (2)
145-145: Verify all consumers of useUI hook are updated.The rename in the return object changes the hook's public API.
Let's verify all consumers have been updated:
✅ Verification successful
Let me gather more information by searching for usages of both the old and new function names, as well as the hook itself.
No issues found with the API change
The search results confirm that:
- The new function name
handleCoreLayerSelectFromMapis consistently used across all relevant files- There are no remaining references to the old name
handleCoreLayerSelectFromUI- The hook's consumers are properly updated, as seen in:
web/src/beta/features/Editor/index.tsx(using the function)web/src/beta/features/Editor/hooks/index.ts(re-exporting the hook)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old name # Test: Search for old function name references that might have been missed rg "handleCoreLayerSelectFromUI"Length of output: 32
Script:
#!/bin/bash # Search for any usage of useUI hook and its destructuring ast-grep --pattern 'const { $$$ } = useUI($$$)' # Also search for direct references to both old and new function names rg "handleCoreLayerSelectFromMap|handleCoreLayerSelectFromUI" -A 2Length of output: 3404
Line range hint
71-77: LGTM! Verify the intended behavior difference with handleLayerSelectFromUI.The rename from
handleCoreLayerSelectFromUItohandleCoreLayerSelectFromMapbetter reflects the source of the selection. I notice this handler intentionally skips clearing sketch type and canceling geometry edits (unlikehandleLayerSelectFromUI), which aligns with fixing the selection loss issue.Let's verify this behavioral difference is consistent across the codebase:
✅ Verification successful
Let me try a different approach to verify the behavioral difference between the selection handlers.
Based on the search results, I can now verify the behavioral differences between the selection handlers. Let me generate the final response:
Verified: The behavioral difference between
handleCoreLayerSelectFromMapandhandleLayerSelectFromUIis intentional and correctThe codebase shows clear distinctions between the two handlers:
handleLayerSelectFromUI(in useUI.ts) performs additional cleanup:handleSketchTypeChange(undefined, "editor"); handleSketchGeometryEditCancel(true);
handleCoreLayerSelectFromMaponly clears scene settings:handleSceneSettingSelect(undefined);This difference is intentional and aligns with fixing the selection loss issue, as map-triggered selections should preserve the sketch state while UI-triggered selections should perform a full cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other selection handlers to understand the pattern # Test: Look for similar selection handling patterns ast-grep --pattern 'handleCoreLayerSelect$_($$$) { $$$ }'Length of output: 61
Script:
#!/bin/bash # Search for all handler functions related to layer selection rg "handle.*LayerSelect.*" -A 5 --type tsLength of output: 19638
web/src/beta/features/Editor/index.tsx (3)
95-95: LGTM! Prop name updated consistently.The prop name
onCoreLayerSelectis correctly updated to receive the renamedhandleCoreLayerSelectFromMapcallback, maintaining consistency with the hook changes.
45-45: LGTM! Verify the hook implementation.The renaming from
handleCoreLayerSelectFromUItohandleCoreLayerSelectFromMapbetter reflects that the selection originates from map interactions rather than UI elements.Let's verify the hook implementation:
✅ Verification successful
The shell script results provide clear evidence about both the implementation and usage of
handleCoreLayerSelectFromMap. Let me generate the final response:✅ Hook implementation and usage verified
The hook is properly implemented in
useUI.tsas a memoized callback that:
- Clears scene setting selection
- Delegates to
handleCoreLayerSelectwith the provided props- Has correct dependency array with both handler dependencies
Used consistently in:
- Destructured in
Editor/index.tsx- Passed as
onCoreLayerSelectprop to a component- Properly exported through the hooks barrel file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the hook implementation and its usage # Expected: Find the implementation and all usages of the renamed function # Find the hook implementation echo "=== Hook Implementation ===" ast-grep --pattern 'const handleCoreLayerSelectFromMap = $_' # Find all usages of the function echo "=== Function Usage ===" rg 'handleCoreLayerSelectFromMap'Length of output: 1282
Line range hint
45-95: Verify the selection preservation fix.While the renaming changes are clear, could you verify how these changes contribute to fixing the selection loss issue when updating layer styles? The direct connection between the renaming and the fix isn't immediately apparent in this file.
Let's check for related changes:
web/src/beta/features/Editor/hooks/useSketch.ts (1)
123-125: Review the setTimeout usage for potential race conditions.The setTimeout usage to handle selection after layer updates could lead to race conditions or inconsistent behavior:
- The 100ms delay is arbitrary and might not work consistently across different scenarios
- Multiple rapid interactions could queue up multiple timeouts
Let's check for similar patterns in the codebase that might indicate this is a common issue:
Consider these alternatives:
- Use MutationObserver to watch for layer updates
- Implement proper event handling for layer updates
- Use React's useEffect cleanup to handle pending selections
Also applies to: 126-134
web/src/beta/features/Editor/hooks/index.ts (2)
102-102: LGTM! Improved naming clarity.The rename from
handleCoreLayerSelectFromUItohandleCoreLayerSelectFromMapbetter reflects the origin of the selection event.
336-336: Verify the propagation of renamed function.The function renaming looks good. Let's verify that all consumers of this hook have been updated to use the new name.
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient information. The results show that:
- There are no remaining references to the old function name
handleCoreLayerSelectFromUI- The new function name
handleCoreLayerSelectFromMapis consistently used across the codebase in the expected locations:
- Defined in
useUI.ts- Exported in
hooks/index.ts- Used in
Editor/index.tsxas a propFunction renaming has been properly propagated
All references to the function have been updated to use the new name
handleCoreLayerSelectFromMap. No instances of the old namehandleCoreLayerSelectFromUIremain in the codebase, and the new name is used consistently across all relevant files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for any remaining references to the old function name # and verify that all consumers use the new name correctly echo "Checking for any remaining references to old function name..." rg "handleCoreLayerSelectFromUI" echo "Verifying usage of new function name in consumers..." rg "handleCoreLayerSelectFromMap"Length of output: 819
Overview
Add condition for clear selection for edit sketch feature.
What I've done
What I haven't done
How I tested
Which point I want you to review particularly
Memo
Summary by CodeRabbit
useUISelectionhook by removing unnecessary calls and dependencies.