fix: basePropsGroup use deepclone prevent clone array itself #975
Conversation
basePropsGroup 使用深拷贝方式复制自身
WalkthroughThe pull request introduces the Changes
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (1)
packages/plugins/materials/src/composable/useMaterial.js (1)
117-122: LGTM: Proper implementation of deep cloning for base propertiesThe changes correctly implement deep cloning at all insertion points:
- When inserting at the start of the group
- When appending at the end of the group
- When adding a new base property group
This prevents unintended mutations of the original data structures and ensures proper isolation of property groups.
Consider monitoring the performance impact if dealing with large property groups, as deep cloning can be computationally expensive. In such cases, you might want to:
- Add performance metrics to track cloning operations
- Consider implementing a more selective cloning strategy for large property groups
- Cache cloned results if the same property groups are frequently reused
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/plugins/materials/src/composable/useMaterial.js(2 hunks)
🔇 Additional comments (2)
packages/plugins/materials/src/composable/useMaterial.js (2)
28-28: LGTM: Appropriate addition of deepClone utility
The addition of deepClone from the utils module aligns with the PR's objective of preventing array self-cloning issues.
117-122: Verify the usage and impact of deep cloning
Let's verify the implementation and its impact across the codebase.
✅ Verification successful
Let me analyze the deep cloning impact by checking its usage patterns.
Based on the analysis of the deep cloning implementation and usage, I can provide a final assessment:
Deep cloning implementation is safe and optimized
The implementation and usage of deep cloning appears to be well-designed and safe for the following reasons:
- The
deepCloneimplementation uses aWeakMapto handle circular references, preventing infinite recursion - The cloning is done selectively based on type with specific handlers for Arrays, Maps, Sets and Objects
- The implementation includes comprehensive test coverage for various scenarios including primitive values, nested objects, and circular references
- Usage is limited to specific necessary cases:
- Material property group cloning
- Canvas state management
- Drag and drop operations
The code shows no concerning patterns:
- No recursive loops with deep cloning
- No performance-critical loops using deep cloning
- Usage is isolated to initialization and state management operations rather than frequent updates
The current implementation in useMaterial.js using deepClone for property groups is appropriate since:
- It ensures proper isolation of property definitions
- It's used only during initialization/setup rather than in frequent operations
- The property groups being cloned are relatively small objects
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the usage of patchBaseProps and potential deep cloning issues
# Check for all calls to patchBaseProps
echo "Checking patchBaseProps usage patterns..."
rg -A 3 "patchBaseProps\("
# Look for potential memory-sensitive contexts
echo "Checking for potential performance-sensitive loops..."
ast-grep --pattern 'for ($loop) {
$$$
patchBaseProps($$$)
$$$
}'
# Check for any TODO/FIXME comments related to performance
echo "Checking for known performance concerns..."
rg -i "todo|fixme|hack|xxx|performance" --type js
Length of output: 70273
Script:
#!/bin/bash
# Check for deepClone usage and implementation
echo "Checking deepClone implementation and usage..."
ast-grep --pattern 'deepClone($$$)'
# Look for any performance-related comments around deepClone
echo "Checking for performance comments around deepClone..."
rg -B2 -A2 "deepClone"
# Check for any loops or recursive patterns with deepClone
echo "Looking for potential recursive or loop patterns with deepClone..."
ast-grep --pattern 'for ($loop) {
$$$
deepClone($$$)
$$$
}'
Length of output: 12346
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
【问题描述】
点开物料面板，切换到区块Tab，切换到具体的区块分组（如无，需创建）
点击添加区块
选中其中一个区块，点击保存。
重复 1-3 步骤。
选中页面中的组件。问题：选中组件卡死（或者很卡）
选中组件后，发现右侧面板重复了很多很多的基础属性。
【问题分析】
其他的 props 属性。basePropGroup。basePropGroup。注意：这里直接插入的是引用。patchBaseProps方法的时候，此时会进入 2.1 的逻辑，即往原分组插入basePropGroup，但是，此时的原分组与basePropGroup相等，相当于自增长。随着物料的迭代，该数组会迅速增长，不断触发 vue 的 proxy 监听，直至栈溢出。【解决方案】
在 2.1 和 2.2 步骤插入
basePropGroup的时候，使用深拷贝插入。What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Refactor