fix(clone): preserve target instance + Map/Set deep clone + cycle detection#2992
fix(clone): preserve target instance + Map/Set deep clone + cycle detection#2992cptbtptpbcptdtptp wants to merge 2 commits into
Conversation
…ep clone Multiple improvements to CloneManager: 1. **Same-type instance preservation**: When source/target properties are distinct instances of the same class (e.g., Vector4 fields), upgrade undecorated clone mode to Deep instead of overwriting with the source reference. Prevents prefab templates from leaking shared references into clone instances. 2. **Map/Set deep clone support**: Native Map and Set instances are now deep-cloned element-by-element instead of falling back to reference copy. 3. **Cycle detection**: Default Object branch now checks deepInstanceMap before creating a new instance, preserving identity for cyclic / shared references within a clone graph. 4. **Restructured control flow** with explicit numbered phases (remap → ignore → primitive → effective mode → type-specific clone) for clarity. 5. **Assignment mode is never upgraded** — explicit @assignmentClone is respected even when target has a same-type instance. Also: ModelMesh throw string → throw Error for proper stack traces. Originated from commits on fix/shaderlab branch: a6156ba (luzhuang) — main fix ca5252a — cycle detection 5c2edee / 40006d9 / f5be424 — restructure + Assignment carve-out Squashed into one PR for clean review against current dev/2.0.
WalkthroughCloneManager restructures cloneProperty to remap references first, short-circuit Ignore, infer or upgrade clone modes for undecorated properties, and handle Map/Set/Array/Object cloning with updated target reuse and cycle registration. ModelMesh now throws Error objects for unreadable vertex buffers. ChangesCloneManager Property Cloning Refactor
ModelMesh Error Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adds test coverage for the new CloneManager behavior: - Undecorated `Entity[]` field — entities remapped via root - Undecorated nested object containing entity refs - Undecorated nested array-of-arrays of entities - Map/Set deep clone Authored alongside the CloneManager fix on fix/shaderlab branch.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/clone/CloneManager.ts`:
- Around line 166-170: The code currently reuses any truthy target[k] (e.g.,
targetPropertyM) without verifying its runtime type; update the CloneManager
cloning paths that handle Map/Set/array/cloneable objects so they only reuse a
preinitialized target when its constructor/type matches the source (use
instanceof Map / instanceof Set / Array.isArray or compare constructors, and for
cloneable objects verify the presence and prototype of copyFrom or matching
constructor); if the existing target[k] does not match, allocate a fresh
instance (new Map(), new Set(), [] or new source.constructor()) before clearing
or copying into it; apply the same guard and instantiation logic to the other
occurrences noted (the Map/Set/array/cloneable branches that use
targetPropertyM, targetPropertyS, and similar variables).
- Around line 165-180: The Map/Set branches in CloneManager are only remapping
_remap refs and then copying entries directly, which causes shared nested
objects to be aliased and bypasses deepInstanceMap; fix by first registering the
source container in deepInstanceMap (deepInstanceMap.set(sourceProperty,
targetPropertyM/targetPropertyS)) immediately after creating/clearing the target
container, then iterate and clone each key/value/element via cloneProperty (pass
the same srcRoot/targetRoot/mode/context used elsewhere) instead of assigning
them directly so nested objects/arrays/class instances get deep-cloned and
self-references are preserved; apply the same change to the Set branch (lines
~182-194) and preserve any _remap handling by invoking cloneProperty which
should internally handle _remap.
- Around line 269-270: The ArrayBuffer.isView check in CloneManager currently
returns CloneMode.Deep for all views but the switch in determineCloneMode only
handles a subset (Uint8Array, Uint16Array, Uint32Array, Int8Array, Int16Array,
Int32Array, Float32Array, Float64Array), causing DataView, Uint8ClampedArray,
BigInt64Array, BigUint64Array to fall through and be recreated empty; update
determineCloneMode (or the surrounding logic in CloneManager) to either
explicitly handle those missing view types with copy logic like the existing
TypedArray branches (ensuring buffer contents are copied) or narrow the
ArrayBuffer.isView inference to only return Deep for the specific constructors
the switch supports, referencing ArrayBuffer.isView, determineCloneMode,
CloneMode, and the listed missing types so the behavior is deterministic and
buffer contents are preserved.
- Around line 165-166: The switch in CloneManager uses let bindings across cases
causing shared lexical-scope issues; wrap each case body in its own block (e.g.,
case Map: { ... break } ), doing this for the TypedArray cases, the Map case
where targetPropertyM is declared, the Set case (targetPropertyS), the Array
case (targetPropertyA/array handling), and the default case so each let/const
has its own scope and the noSwitchDeclarations lint error is resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 46eb3fcd-2d3a-4540-8783-e5109c986971
📒 Files selected for processing (2)
packages/core/src/clone/CloneManager.tspackages/core/src/mesh/ModelMesh.ts
| case Map: | ||
| let targetPropertyM = <Map<any, any>>target[k]; | ||
| if (targetPropertyM == null) { | ||
| target[k] = targetPropertyM = new Map<any, any>(); | ||
| } else { | ||
| targetPropertyM.clear(); | ||
| } | ||
| (<Map<any, any>>sourceProperty).forEach((value, key) => { | ||
| if (key instanceof Object && (<ICustomClone>key)._remap) { | ||
| key = (<ICustomClone>key)._remap(srcRoot, targetRoot); | ||
| } | ||
| if (value instanceof Object && (<ICustomClone>value)._remap) { | ||
| value = (<ICustomClone>value)._remap(srcRoot, targetRoot); | ||
| } | ||
| targetPropertyM.set(key, value); | ||
| }); |
There was a problem hiding this comment.
Map/Set “deep clone” still aliases nested entries.
These branches only remap _remap references, then write keys/values/elements straight into the new container. In CloneMode.Deep, nested arrays/objects/class instances are still shared with the source, and deepInstanceMap never sees the container or its entries, so self-referential/shared structures inside a Map/Set break. Route each key/value/element through cloneProperty and register the source container in deepInstanceMap before iterating.
Also applies to: 182-194
🧰 Tools
🪛 Biome (2.4.14)
[error] 166-166: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clone/CloneManager.ts` around lines 165 - 180, The Map/Set
branches in CloneManager are only remapping _remap refs and then copying entries
directly, which causes shared nested objects to be aliased and bypasses
deepInstanceMap; fix by first registering the source container in
deepInstanceMap (deepInstanceMap.set(sourceProperty,
targetPropertyM/targetPropertyS)) immediately after creating/clearing the target
container, then iterate and clone each key/value/element via cloneProperty (pass
the same srcRoot/targetRoot/mode/context used elsewhere) instead of assigning
them directly so nested objects/arrays/class instances get deep-cloned and
self-references are preserved; apply the same change to the Set branch (lines
~182-194) and preserve any _remap handling by invoking cloneProperty which
should internally handle _remap.
| case Map: | ||
| let targetPropertyM = <Map<any, any>>target[k]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant lines
cat -n packages/core/src/clone/CloneManager.ts | sed -n '160,230p'Repository: galacean/engine
Length of output: 3134
🏁 Script executed:
# Check for Biome configuration
find . -type f -name "biome.json" -o -name ".biomerc*" -o -name "biome.jsonc" | head -20Repository: galacean/engine
Length of output: 72
🏁 Script executed:
# Let's also check the complete file size to understand what we're dealing with
wc -l packages/core/src/clone/CloneManager.tsRepository: galacean/engine
Length of output: 103
🏁 Script executed:
# Read the Biome configuration
cat biome.jsonRepository: galacean/engine
Length of output: 1318
🏁 Script executed:
# Get a broader view of the switch statement to ensure no cases are missed
cat -n packages/core/src/clone/CloneManager.ts | sed -n '140,260p'Repository: galacean/engine
Length of output: 5233
Wrap all case declarations with block scope.
The let bindings in multiple switch cases live in the switch's shared lexical scope, which is why Biome flags noSwitchDeclarations. Bracing each case creates isolated scopes and fixes the lint error. This applies to the TypedArray cases (lines 150–164), Map case (line 165), Set case (line 182), Array case (line 196), and default case (line 216).
Suggested fix
case Uint8Array:
case Uint16Array:
case Uint32Array:
case Int8Array:
case Int16Array:
case Int32Array:
case Float32Array:
case Float64Array: {
let targetPropertyT = <TypedArray>target[k];
if (targetPropertyT == null || targetPropertyT.length !== (<TypedArray>sourceProperty).length) {
target[k] = (<TypedArray>sourceProperty).slice();
} else {
targetPropertyT.set(<TypedArray>sourceProperty);
}
break;
+ }
case Map: {
let targetPropertyM = <Map<any, any>>target[k];
if (targetPropertyM == null) {
target[k] = targetPropertyM = new Map<any, any>();
} else {
targetPropertyM.clear();
}
(<Map<any, any>>sourceProperty).forEach((value, key) => {
if (key instanceof Object && (<ICustomClone>key)._remap) {
key = (<ICustomClone>key)._remap(srcRoot, targetRoot);
}
if (value instanceof Object && (<ICustomClone>value)._remap) {
value = (<ICustomClone>value)._remap(srcRoot, targetRoot);
}
targetPropertyM.set(key, value);
});
break;
}
case Set: {
let targetPropertyS = <Set<any>>target[k];
if (targetPropertyS == null) {
target[k] = targetPropertyS = new Set<any>();
} else {
targetPropertyS.clear();
}
(<Set<any>>sourceProperty).forEach((value) => {
if (value instanceof Object && (<ICustomClone>value)._remap) {
value = (<ICustomClone>value)._remap(srcRoot, targetRoot);
}
targetPropertyS.add(value);
});
break;
}
case Array: {
let targetPropertyA = <Array<any>>target[k];
const length = (<Array<any>>sourceProperty).length;
if (targetPropertyA == null) {
target[k] = targetPropertyA = new Array<any>(length);
} else {
targetPropertyA.length = length;
}
for (let i = 0; i < length; i++) {
CloneManager.cloneProperty(
<Array<any>>sourceProperty,
targetPropertyA,
i,
cloneMode,
srcRoot,
targetRoot,
deepInstanceMap
);
}
break;
}
default: {
// Check if we've already visited this source object (cycle detection)
if (deepInstanceMap.has(sourceProperty)) {
target[k] = deepInstanceMap.get(sourceProperty);
return;
}
let targetPropertyD = <Object>target[k];
if (!targetPropertyD) {
targetPropertyD = new sourceProperty.constructor();
target[k] = targetPropertyD;
}
deepInstanceMap.set(sourceProperty, targetPropertyD);
if ((<ICustomClone>sourceProperty).copyFrom) {
(<ICustomClone>targetPropertyD).copyFrom(<ICustomClone>sourceProperty);
} else {
const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor);
for (let k in sourceProperty) {
CloneManager.cloneProperty(
<Object>sourceProperty,
targetPropertyD,
k,
cloneModes[k],
srcRoot,
targetRoot,
deepInstanceMap
);
}
(<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetPropertyD, srcRoot, targetRoot);
}
break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case Map: | |
| let targetPropertyM = <Map<any, any>>target[k]; | |
| case Uint8Array: | |
| case Uint16Array: | |
| case Uint32Array: | |
| case Int8Array: | |
| case Int16Array: | |
| case Int32Array: | |
| case Float32Array: | |
| case Float64Array: { | |
| let targetPropertyT = <TypedArray>target[k]; | |
| if (targetPropertyT == null || targetPropertyT.length !== (<TypedArray>sourceProperty).length) { | |
| target[k] = (<TypedArray>sourceProperty).slice(); | |
| } else { | |
| targetPropertyT.set(<TypedArray>sourceProperty); | |
| } | |
| break; | |
| } | |
| case Map: { | |
| let targetPropertyM = <Map<any, any>>target[k]; | |
| if (targetPropertyM == null) { | |
| target[k] = targetPropertyM = new Map<any, any>(); | |
| } else { | |
| targetPropertyM.clear(); | |
| } | |
| (<Map<any, any>>sourceProperty).forEach((value, key) => { | |
| if (key instanceof Object && (<ICustomClone>key)._remap) { | |
| key = (<ICustomClone>key)._remap(srcRoot, targetRoot); | |
| } | |
| if (value instanceof Object && (<ICustomClone>value)._remap) { | |
| value = (<ICustomClone>value)._remap(srcRoot, targetRoot); | |
| } | |
| targetPropertyM.set(key, value); | |
| }); | |
| break; | |
| } | |
| case Set: { | |
| let targetPropertyS = <Set<any>>target[k]; | |
| if (targetPropertyS == null) { | |
| target[k] = targetPropertyS = new Set<any>(); | |
| } else { | |
| targetPropertyS.clear(); | |
| } | |
| (<Set<any>>sourceProperty).forEach((value) => { | |
| if (value instanceof Object && (<ICustomClone>value)._remap) { | |
| value = (<ICustomClone>value)._remap(srcRoot, targetRoot); | |
| } | |
| targetPropertyS.add(value); | |
| }); | |
| break; | |
| } | |
| case Array: { | |
| let targetPropertyA = <Array<any>>target[k]; | |
| const length = (<Array<any>>sourceProperty).length; | |
| if (targetPropertyA == null) { | |
| target[k] = targetPropertyA = new Array<any>(length); | |
| } else { | |
| targetPropertyA.length = length; | |
| } | |
| for (let i = 0; i < length; i++) { | |
| CloneManager.cloneProperty( | |
| <Array<any>>sourceProperty, | |
| targetPropertyA, | |
| i, | |
| cloneMode, | |
| srcRoot, | |
| targetRoot, | |
| deepInstanceMap | |
| ); | |
| } | |
| break; | |
| } | |
| default: { | |
| // Check if we've already visited this source object (cycle detection) | |
| if (deepInstanceMap.has(sourceProperty)) { | |
| target[k] = deepInstanceMap.get(sourceProperty); | |
| return; | |
| } | |
| let targetPropertyD = <Object>target[k]; | |
| if (!targetPropertyD) { | |
| targetPropertyD = new sourceProperty.constructor(); | |
| target[k] = targetPropertyD; | |
| } | |
| deepInstanceMap.set(sourceProperty, targetPropertyD); | |
| if ((<ICustomClone>sourceProperty).copyFrom) { | |
| (<ICustomClone>targetPropertyD).copyFrom(<ICustomClone>sourceProperty); | |
| } else { | |
| const cloneModes = CloneManager.getCloneMode(sourceProperty.constructor); | |
| for (let k in sourceProperty) { | |
| CloneManager.cloneProperty( | |
| <Object>sourceProperty, | |
| targetPropertyD, | |
| k, | |
| cloneModes[k], | |
| srcRoot, | |
| targetRoot, | |
| deepInstanceMap | |
| ); | |
| } | |
| (<ICustomClone>sourceProperty)._cloneTo?.(<ICustomClone>targetPropertyD, srcRoot, targetRoot); | |
| } | |
| break; | |
| } |
🧰 Tools
🪛 Biome (2.4.14)
[error] 166-166: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clone/CloneManager.ts` around lines 165 - 166, The switch
in CloneManager uses let bindings across cases causing shared lexical-scope
issues; wrap each case body in its own block (e.g., case Map: { ... break } ),
doing this for the TypedArray cases, the Map case where targetPropertyM is
declared, the Set case (targetPropertyS), the Array case (targetPropertyA/array
handling), and the default case so each let/const has its own scope and the
noSwitchDeclarations lint error is resolved.
| let targetPropertyM = <Map<any, any>>target[k]; | ||
| if (targetPropertyM == null) { | ||
| target[k] = targetPropertyM = new Map<any, any>(); | ||
| } else { | ||
| targetPropertyM.clear(); |
There was a problem hiding this comment.
Only preserve a preinitialized target when its type matches the source.
These paths currently reuse any truthy target[k]. If a field was preinitialized with a different runtime type, this can call clear() on a non-Map/Set or run copyFrom/field assignment against the wrong prototype. Gate reuse by instanceof Map / instanceof Set / matching constructor; otherwise allocate a fresh instance.
Suggested fix
- let targetPropertyM = <Map<any, any>>target[k];
+ let targetPropertyM = target[k] instanceof Map ? <Map<any, any>>target[k] : null;
if (targetPropertyM == null) {
target[k] = targetPropertyM = new Map<any, any>();
} else {
targetPropertyM.clear();
}
- let targetPropertyS = <Set<any>>target[k];
+ let targetPropertyS = target[k] instanceof Set ? <Set<any>>target[k] : null;
if (targetPropertyS == null) {
target[k] = targetPropertyS = new Set<any>();
} else {
targetPropertyS.clear();
}
let targetPropertyD = <Object>target[k];
- if (!targetPropertyD) {
+ if (!targetPropertyD || targetPropertyD.constructor !== sourceProperty.constructor) {
targetPropertyD = new sourceProperty.constructor();
target[k] = targetPropertyD;
}Also applies to: 183-187, 223-228
🧰 Tools
🪛 Biome (2.4.14)
[error] 166-166: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clone/CloneManager.ts` around lines 166 - 170, The code
currently reuses any truthy target[k] (e.g., targetPropertyM) without verifying
its runtime type; update the CloneManager cloning paths that handle
Map/Set/array/cloneable objects so they only reuse a preinitialized target when
its constructor/type matches the source (use instanceof Map / instanceof Set /
Array.isArray or compare constructors, and for cloneable objects verify the
presence and prototype of copyFrom or matching constructor); if the existing
target[k] does not match, allocate a fresh instance (new Map(), new Set(), [] or
new source.constructor()) before clearing or copying into it; apply the same
guard and instantiation logic to the other occurrences noted (the
Map/Set/array/cloneable branches that use targetPropertyM, targetPropertyS, and
similar variables).
| // TypedArrays - copy data | ||
| if (ArrayBuffer.isView(sourceProperty)) return CloneMode.Deep; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript, does ArrayBuffer.isView()return true forDataView, Uint8ClampedArray, BigInt64Array, and BigUint64Array?
💡 Result:
Yes. ArrayBuffer.isView(value) returns true for each of these, because it returns true when the argument has the internal slot [[ViewedArrayBuffer]], which is used by ArrayBuffer views such as typed array objects and DataView [1][2]. Specifically: - DataView: true [2] - Uint8ClampedArray: true (it’s a typed array view) [2][3] - BigInt64Array: true (it’s a typed array view) [2][3] - BigUint64Array: true (it’s a typed array view) [2][3] If you want to validate quickly in code: ArrayBuffer.isView(new DataView(new ArrayBuffer(1))) // true ArrayBuffer.isView(new Uint8ClampedArray(1)) // true ArrayBuffer.isView(new BigInt64Array(1)) // true ArrayBuffer.isView(new BigUint64Array(1)) // true
Citations:
- 1: https://tc39.es/ecma262/2022/multipage/structured-data.html
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer/isView
- 3: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray
🏁 Script executed:
find . -name "CloneManager.ts" -type fRepository: galacean/engine
Length of output: 101
🏁 Script executed:
cat -n ./packages/core/src/clone/CloneManager.ts | head -350 | tail -100Repository: galacean/engine
Length of output: 4429
🏁 Script executed:
cat -n ./packages/core/src/clone/CloneManager.ts | head -250 | tail -100Repository: galacean/engine
Length of output: 4288
🏁 Script executed:
cat -n ./packages/core/src/clone/CloneManager.ts | sed -n '100,160p'Repository: galacean/engine
Length of output: 2635
Handle all ArrayBuffer views explicitly in the switch statement, or narrow the ArrayBuffer.isView inference.
The ArrayBuffer.isView check at line 270 infers Deep clone for DataView, Uint8ClampedArray, BigInt64Array, and BigUint64Array, but the switch statement only handles Uint8Array, Uint16Array, Uint32Array, Int8Array, Int16Array, Int32Array, Float32Array, and Float64Array. The unhandled types fall through to the default case, which executes new sourceProperty.constructor(), creating empty instances that lose all buffer contents. Add explicit copy logic for the missing views (similar to lines 158–163), or restrict the inference to only the types the switch actually handles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/clone/CloneManager.ts` around lines 269 - 270, The
ArrayBuffer.isView check in CloneManager currently returns CloneMode.Deep for
all views but the switch in determineCloneMode only handles a subset
(Uint8Array, Uint16Array, Uint32Array, Int8Array, Int16Array, Int32Array,
Float32Array, Float64Array), causing DataView, Uint8ClampedArray, BigInt64Array,
BigUint64Array to fall through and be recreated empty; update determineCloneMode
(or the surrounding logic in CloneManager) to either explicitly handle those
missing view types with copy logic like the existing TypedArray branches
(ensuring buffer contents are copied) or narrow the ArrayBuffer.isView inference
to only return Deep for the specific constructors the switch supports,
referencing ArrayBuffer.isView, determineCloneMode, CloneMode, and the listed
missing types so the behavior is deterministic and buffer contents are
preserved.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/src/core/CloneUtils.test.ts (1)
1015-1035: ⚡ Quick winAdd
Setclone/remap coverage to match the PR scope.Line 1015 introduces Map coverage, but Set is still untested in this suite even though this PR claims Map/Set deep clone support. Adding one Set case (internal + external entity refs) will close that regression gap.
Proposed test addition
+/** Script with UNDECORATED Set containing entity values */ +class SetRefScript extends Script { + entitySet: Set<Entity> = new Set(); +} + +describe("Set with entity values (type inference)", () => { + it("undecorated Set should create new Set and remap internal entity values", () => { + const rootEntity = scene.createRootEntity("root"); + const parent = rootEntity.createChild("parent"); + const child = parent.createChild("child"); + const external = rootEntity.createChild("external"); + const script = parent.addComponent(SetRefScript); + script.entitySet.add(child); + script.entitySet.add(external); + + const cloned = parent.clone(); + const cs = cloned.getComponent(SetRefScript); + + expect(cs.entitySet).not.eq(script.entitySet); + expect(cs.entitySet.size).eq(2); + expect(cs.entitySet.has(cloned.children[0])).eq(true); + expect(cs.entitySet.has(external)).eq(true); + + rootEntity.destroy(); + }); +});🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/src/core/CloneUtils.test.ts` around lines 1015 - 1035, Add a parallel test for Set cloning/remapping to the Map case: create rootEntity/parent/child/external, attach the component that holds a Set (e.g., SetRefScript or the component that defines entitySet), add the internal child and external entity to script.entitySet, clone parent (parent.clone()), retrieve the cloned component, and assert the cloned entitySet is a different Set instance, has size 2, contains the remapped internal reference (cloned.children[0]) and still contains the original external reference; then destroy rootEntity. Use the same test structure/location as the existing "Map with entity values (type inference)" test so Set coverage matches Map.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/src/core/CloneUtils.test.ts`:
- Around line 1015-1035: Add a parallel test for Set cloning/remapping to the
Map case: create rootEntity/parent/child/external, attach the component that
holds a Set (e.g., SetRefScript or the component that defines entitySet), add
the internal child and external entity to script.entitySet, clone parent
(parent.clone()), retrieve the cloned component, and assert the cloned entitySet
is a different Set instance, has size 2, contains the remapped internal
reference (cloned.children[0]) and still contains the original external
reference; then destroy rootEntity. Use the same test structure/location as the
existing "Map with entity values (type inference)" test so Set coverage matches
Map.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31e2c0b6-0160-453f-a1e7-a094c123b42a
📒 Files selected for processing (1)
tests/src/core/CloneUtils.test.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖（预制件 Vector4 共享引用） | ✅ 已修复（same-type instance 升级 Deep clone） |
| Map/Set 无深克隆支持 | ✅ 已修复（element-by-element clone + Entity/Component remap） |
| 循环引用导致无限递归 | ✅ 已修复（deepInstanceMap 提前占位，在递归子字段前 set） |
| throw string 未包装 Error | ✅ 已修复 |
总结
CloneManager.cloneProperty 全面升级：same-type target 实例保护、Map/Set 深克隆（含 Entity/Component remap）、循环引用检测。控制流分 6 个编号阶段，可读性显著提升。_inferCloneMode 对 undecorated 属性的类型推断覆盖了 Array/TypedArray/Map/Set/copyFrom/plain object 五类，设计合理。测试扩展完整（undecorated array/object/Map remap 全场景覆盖）。
[P2] CloneManager.ts — Map/Set 分支中普通对象 value 仅 remap，不深拷贝
Map clone 中对每个 value 执行 _remap 检查，若 value 是普通对象（Vector3、{}、不实现 _remap 的自定义类）会直接把原始引用写入目标 Map，未进入 deepInstanceMap 路径，仍然共享引用。当前引擎无 Map<K, Vector3> 类用例，不阻塞合并。建议在注释中说明 Map/Set 深克隆目前只保证 Entity/Component 的 remap，普通对象 value 为浅引用，留后续迭代完整实现。
无新 P0/P1，LGTM，可合入。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖（预制件 Vector4 共享引用） | ✅ 已修复（same-type instance 升级 Deep clone） |
| Map/Set 无深克隆支持 | ✅ 已修复（element-by-element clone + Entity/Component remap） |
| 循环引用导致无限递归 | ✅ 已修复（deepInstanceMap 提前占位） |
| throw string 未包装 Error | ✅ 已修复 |
总结
CloneManager.cloneProperty 全面升级：same-type target 实例保护、Map/Set 深克隆（含 Entity/Component remap）、循环引用检测。控制流分 6 个编号阶段，可读性显著提升。_inferCloneMode 对 undecorated 属性的类型推断覆盖了 Array/TypedArray/Map/Set/copyFrom/plain object 五类，设计合理。测试扩展完整（undecorated array/object/Map remap 全场景覆盖）。
问题
[P2] CloneManager.ts — Map/Set 分支中普通对象 value 仅 remap，不深拷贝
Map clone 中对每个 value 执行 _remap 检查，若 value 是普通对象（Vector3、{}、不实现 _remap 的自定义类）会直接把原始引用写入目标 Map，仍然共享引用。当前引擎无 Map<K, Vector3> 类用例，不阻塞合并。建议在注释中说明 Map/Set 深克隆目前只保证 Entity/Component 的 remap，普通对象 value 为浅引用，留后续迭代完整实现。
无新 P0/P1，LGTM，可合入。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查（2026-05-15）
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖（预制件 Vector4 共享引用） | ✅ 已修复：same-type instance 升级 Deep clone |
| Map/Set 无深克隆支持 | ✅ 已修复（element-by-element clone + Entity/Component remap） |
| 循环引用导致无限递归 | ✅ 已修复（deepInstanceMap 检测） |
总结
CloneManager.cloneProperty 三项改进方向均正确：same-type instance 保留、Map/Set 深克隆、循环/共享引用检测。控制流重构成 6 个编号阶段也提升了可读性。ModelMesh throw string → throw new Error 顺带修复。测试覆盖全面。
问题
[P2] Map clone 中对 key 做了 _remap，但 Set clone 中只对 value 做 remap
Map 支持 key 是 Entity/Component 引用（如 HashMap<Entity, data> 场景），Set 目前只处理 value。对称性上 Set 的 remap 已正确（Set 没有 key）。但 Map 的 key remap 在当前版本实际上是必要的，而 Set 因为 value 本身就是 key 所以已对称——P3 级，不影响合入。
整体 LGTM，可以合入。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
审查（2026-05-15）
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖（预制件 Vector4 共享引用） | ✅ 已修复：same-type instance 升级 Deep clone |
| Map/Set 无深克隆支持 | ✅ 已修复（element-by-element clone + Entity/Component remap） |
| 循环引用导致无限递归 | ✅ 已修复（deepInstanceMap 在递归前 set，阻断环） |
Array 子元素克隆：传 original cloneMode 而非 effectiveCloneMode |
✅ 注释说明了理由（decorated 子元素继承 decorator 声明的模式，undecorated 子元素独立推断） |
总结
CloneManager 三项扩展全部正确且实现质量高：
- same-type instance 保留：
@assignmentClone永不升级，其他 mode + same-type target 时升级为 Deep，精确保护了组件构造函数创建的独立实例。 - Map/Set 深克隆：element-by-element 处理 + Entity/Component remap，与 Array 路径对称。
- 循环检测：
deepInstanceMap.set在递归前调用（Object default 分支），时序正确。 _inferCloneMode：将推断逻辑从主路径中提取为私有 helper，控制流清晰。
ModelMesh throw string → throw new Error 是顺带的好修复。
问题
[P2] CloneManager.ts:155-175（Map 分支）— Map key 的 remap 语义存疑
Map key 被 remap 了（Entity/Component 类型的 key 替换为克隆后的实例），但 Set 的 value 也被 remap 了。这两种行为是正确的，但文档缺失：当 Map 的 key 是场景内 Entity 时，克隆出来的新 Map 的 key 会指向克隆层级内的对应 Entity（remap）还是原始 Entity（reference）？当前实现是 remap，但如果 key 来自场景外部 Entity（无法 remap），_remap 返回的是什么？建议在 Map/Set 分支旁加注释说明 remap 行为，和"external ref 保持不变"的保证。
这是文档缺失问题，不阻塞合入。
可合入
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
CloneManager.cloneProperty 三项改进全部正确且实现质量高：
- same-type instance 保留：
@assignmentClone永不升级，其他 mode + same-type target 时升级为 Deep，精确保护了组件构造函数创建的独立实例（修复预制件 Vector4 共享引用 bug）。 - Map/Set 深克隆：element-by-element 处理 + Entity/Component remap，覆盖 Map key 也能 remap 的场景。
- 循环引用检测：
deepInstanceMap.set在递归子字段之前完成，正确阻断环。
控制流重构成 6 个编号阶段，可读性显著提升。ModelMesh throw string → throw new Error 顺带修复。测试扩展全面（undecorated array/object/Map remap 全场景覆盖）。
所有历史问题均已修复。
问题
[P2] CloneManager.ts:206 — Array 子元素克隆传 cloneMode（原始 decorator mode）而非 effectiveCloneMode
注释已说明理由（"decorated → children inherit, undecorated → children infer independently"），逻辑正确。但这意味着如果用 @shallowClone 装饰了一个 Array 字段，Array 内部的 Object 元素会走 Shallow 而非继续推断 Deep，这可能与用户期望不符。建议在 JSDoc 中说明 Array 元素的 clone mode 传播规则，避免使用者混淆。
[P2] _inferCloneMode 中 Array.isArray 检查早于 copyFrom 检查，两者无重叠，顺序无影响，但如果未来有实现了 copyFrom 的 Array 子类，行为会有意外。属于防御性建议，可忽略。
无阻塞问题，LGTM，可以合并。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
已关闭问题清单
| 问题 | 状态 |
|---|---|
| target 实例被 source 引用覆盖（Vector4 预制件 shared reference） | ✅ 已修复：same-type instance 升级 Deep clone |
| Map/Set 无深克隆支持 | ✅ 已修复（element-by-element clone + Entity/Component remap） |
| 循环引用导致无限递归 | ✅ 已修复（deepInstanceMap.set 在递归前完成，正确阻断环） |
总结
CloneManager.cloneProperty 三项改进全部正确且实现质量高：
- same-type instance 保留：
@assignmentClone永不升级，其他 mode + same-type target 时升级为 Deep，精确保护了组件构造函数创建的独立实例 - Map/Set 深克隆：element-by-element 处理 + Entity/Component remap，Map key remap 也覆盖
- 循环引用检测：
deepInstanceMap.set在递归子字段之前完成，正确阻断环
ModelMesh throw string → throw new Error 顺带修复。测试扩展全面（undecorated array/object/Map remap 全场景覆盖）。
问题
[P2] CloneManager.ts:206 — Array 子元素克隆传 cloneMode（原始 decorator mode）而非 effectiveCloneMode
注释已说明理由（"decorated → children inherit, undecorated → children infer independently"），逻辑正确。但这意味着如果用 @shallowClone 装饰了一个 Array 字段，Array 内部的 Object 元素会走 Shallow 而非继续推断 Deep，这可能与用户期望不符。建议在 JSDoc 中说明 Array 元素的 clone mode 继承规则，便于后续维护。
无阻塞问题，可以合并。
2 participants
Summary
Comprehensive improvements to
CloneManager.clonePropertyto fix prefab clone leaking shared references and to broaden type support:Same-type instance preservation
When a target field already has its own instance (e.g.,
Vector4created by a component constructor) and the source has a distinct instance of the same class, the undecorated clone mode is upgraded toDeepso the target's independent instance is preserved (filled viacopyFrom) instead of being overwritten with the prefab's shared reference.Map / Set deep clone support
Native
MapandSetproperties are now cloned element-by-element instead of being skipped.Cycle / shared-reference detection
The default Object branch checks
deepInstanceMapfirst, so cyclic and shared references within a single clone graph preserve identity.CloneMode.Assignmentis never upgradedAn explicit
@assignmentCloneis respected even when target has a same-type instance — user-declared reference semantics win over the auto-upgrade.Restructured control flow
The dispatch is now organized into explicit numbered phases (remap → ignore → primitive → effective mode → type-specific clone) for clarity.
Also:
ModelMeshthrow string→throw Errorfor usable stack traces.Origin
Squashed from commits on
fix/shaderlabbranch:a6156ba9f(luzhuang) — main fixca5252a9a— cycle detection5c2edee1e/40006d952/f5be42400— restructure + Assignment carve-outSubmitted as one PR because the original chain had misleading commit titles ("raycast and clone" / "feat: supported filled" etc.) — net result is what matters for review.
Test Plan
CloneUtils.test.tsandShaderData.test.tspass against the new implementationVector4member field: clone does not share reference with template@assignmentClonefield with same-type target instance: still gets reference assignment (not upgraded)Summary by CodeRabbit
Bug Fixes
Refactor