refactor(clone): type-driven default clone mode for container elements#3040
refactor(clone): type-driven default clone mode for container elements#3040cptbtptpbcptdtptp wants to merge 12 commits into
Conversation
…ainer elements - Add `@defaultCloneMode(mode)` class decorator for declaring how instances of a type should be cloned when encountered as field values - Change container element cloning: array elements now use `undefined` as cloneMode so each element's type-level `_defaultCloneMode` is consulted. Unknown types (no _defaultCloneMode) default to Assignment (shared reference) instead of Deep, preventing crash on DOM/native objects. - Add `_defaultCloneMode` to ICustomClone interface - Mark RenderState system classes with @defaultCloneMode(Deep): DepthState, StencilState, RasterState, RenderTargetBlendState, BlendState, RenderState Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a new ChangesType-level Default Clone Mode
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 67-69: The defaultCloneMode decorator accepts any CloneMode value,
but the runtime logic at lines 141-149 only properly handles Deep and Shallow
modes, while Ignore is checked separately earlier at line 138. To fix this,
modify the defaultCloneMode function signature to restrict the mode parameter to
only accept CloneMode.Deep or CloneMode.Shallow (using a union type or
overload), preventing callers from incorrectly decorating with CloneMode.Ignore
which would not be honored at runtime.
🪄 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: 585fad6c-db8f-4c53-9512-ff3e147e9dde
📒 Files selected for processing (8)
packages/core/src/clone/CloneManager.tspackages/core/src/clone/ComponentCloner.tspackages/core/src/shader/state/BlendState.tspackages/core/src/shader/state/DepthState.tspackages/core/src/shader/state/RasterState.tspackages/core/src/shader/state/RenderState.tspackages/core/src/shader/state/RenderTargetBlendState.tspackages/core/src/shader/state/StencilState.ts
| export function defaultCloneMode(mode: CloneMode) { | ||
| return function (target: Function): void { | ||
| Object.defineProperty(target.prototype, "_defaultCloneMode", { value: mode }); |
There was a problem hiding this comment.
@defaultCloneMode accepts modes that are not fully honored at runtime
At Line 67 the decorator accepts any CloneMode, but Lines 141-149 only promote Deep/Shallow, and the Ignore fast-path is checked earlier at Line 138. So @defaultCloneMode(CloneMode.Ignore) will not actually ignore the value.
Suggested fix
- if (cloneMode === CloneMode.Ignore) return;
-
- // If no per-field decorator, consult the value's type-level default clone mode
- if (cloneMode === undefined && sourceProperty instanceof Object) {
- const typeDefault = (<ICustomClone>sourceProperty)._defaultCloneMode;
- if (typeDefault === CloneMode.Deep) {
- cloneMode = CloneMode.Deep;
- } else if (typeDefault === CloneMode.Shallow) {
- cloneMode = CloneMode.Shallow;
- }
- // typeDefault === undefined or Assignment → fall through to assignment below
- }
+ // If no per-field decorator, consult the value's type-level default clone mode
+ if (cloneMode === undefined && sourceProperty instanceof Object) {
+ cloneMode = (<ICustomClone>sourceProperty)._defaultCloneMode;
+ }
+ if (cloneMode === CloneMode.Ignore) return;Also applies to: 138-149
🤖 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 67 - 69, The
defaultCloneMode decorator accepts any CloneMode value, but the runtime logic at
lines 141-149 only properly handles Deep and Shallow modes, while Ignore is
checked separately earlier at line 138. To fix this, modify the defaultCloneMode
function signature to restrict the mode parameter to only accept CloneMode.Deep
or CloneMode.Shallow (using a union type or overload), preventing callers from
incorrectly decorating with CloneMode.Ignore which would not be honored at
runtime.
…+ type-driven deep clone Core changes: - Clone is now opt-out: all enumerable fields are cloned unless @ignoreClone - Default clone mode for unknown types is Assignment (shared reference) — safe for DOM elements, native handles, and any unrecognized constructor - Types that need independent copies declare @defaultCloneMode(CloneMode.Deep) - Identity-map based deep clone with cycle/shared-subgraph dedup - 3-stage lifecycle: Construct → Populate (copyFrom or for...in) → Finalize (_cloneTo) - Container elements (Array/Map/Set) go through the type-driven gate individually - @deepClone/@assignmentClone/@shallowClone kept as no-op for backward compat - @ignoreClone remains functional RenderState classes marked @defaultCloneMode(Deep): DepthState, StencilState, RasterState, RenderTargetBlendState, BlendState, RenderState Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eep) Particle system: ParticleGeneratorModule (+ all subclasses via inheritance), MainModule, BaseShape (+ all shape subclasses), ParticleGenerator, ParticleCompositeCurve, ParticleCurve, CurveKey, ParticleCompositeGradient, ParticleGradient, GradientColorKey, GradientAlphaKey, Burst Post-process: PostProcessEffect (+ BloomEffect, TonemappingEffect via inheritance), PostProcessEffectParameter (+ Float/Bool/Color/Vector/Enum/Texture) Physics: JointLimits, JointMotor Other: VirtualCamera, Skin Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mark Entity/Component with @defaultCloneMode(Remap) and split Entity.clone() into a register-then-copy two-pass: pre-register every source Entity/Component to its clone in the identity map across the whole subtree, so references nested in arrays/maps/objects are remapped through the clone gate, not just top-level fields. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Re-register @deepClone/@assignmentClone/@ignoreClone as field-level modes (highest priority). - Containers (Array/Map/Set/TypedArray/plain object) default to deep clone. - Type defaults: ReferResource -> Assignment, math types -> Deep, UpdateFlagManager -> Ignore. - Thread srcRoot/targetRoot through the gate so Signal._cloneTo can remap listeners. - Add CloneMode.Ignore; drop erroneous @deepClone on Joint limits/motor _updateFlagManager. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…efaults - Remove @deepClone/@assignmentClone/@shallowClone field decorators (171) across core & ui. - Fields resolve via container default deep + type-level @defaultCloneMode + Assignment fallback. - Mark ShaderData & Signal Deep; ShaderData adds _cloneTo delegating to cloneTo. - Clean up now-unused clone decorator imports. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t-assignment # Conflicts: # packages/core/src/particle/ParticleGenerator.ts
…l run Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3040 +/- ##
===========================================
- Coverage 79.49% 79.44% -0.05%
===========================================
Files 919 919
Lines 102523 102651 +128
Branches 11364 8696 -2668
===========================================
+ Hits 81500 81555 +55
- Misses 20837 20913 +76
+ Partials 186 183 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…low) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Rework the clone system around a clear type-driven priority chain, so most objects clone correctly by their type with little to no per-field annotation.
How a field value is cloned — priority high → low
@deepClone/@assignmentClone/@ignoreClone(explicit per-field override).@defaultCloneMode.Assignment(share the reference).Primitives are copied by value; functions are skipped (the clone's constructor re-establishes bound handlers).
Type defaults (what most fields rely on)
ReferResource(Texture / Mesh / Material / Sprite / Font / …) → Assignment — assets are shared by reference.Entity/Component→ Remap — rewired to the clone within the cloned subtree.CloneManager(the math package can't depend on core's decorator).UpdateFlagManager→ Ignore — runtime notifier; the clone keeps its own.Changes
CloneModegainsIgnore.@deepClone/@assignmentClone/@shallowClonefield decorators — they're covered by the type-default network + container default; only@ignoreClone(and a few genuine overrides) remain.srcRoot/targetRootare threaded through the gate to_cloneTo, so types likeSignalcan remap entity/component references in their listeners.Test plan
🤖 Generated with Claude Code