fix: ._engine has been deleted#346
Merged
cptbtptpbcptdtptp merged 2 commits intogalacean:dev/2.0from Mar 9, 2026
Merged
Conversation
WalkthroughReplace private Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/xr/src/trackable/XRTrackedObjectManager.ts (1)
51-57: Consider covering the XRengine.xrManagerpath with a focused test.Both changed sites are runtime-only branches, so a lightweight session/feature stub test would make this
_engine→enginemigration much safer and catch regressions before XR usage does.Also applies to: 115-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xr/src/trackable/XRTrackedObjectManager.ts` around lines 51 - 57, Add a focused unit test that stubs a lightweight XR session/feature path to cover the engine.xrManager usage in XRTrackedObjectManager: specifically exercise XRTrackedObjectManager._onSessionChanged with XRSessionState.Initialized (and the similar branch around lines 115-116) by mocking this.engine to include an xrManager with a getFeature method that returns a fake feature object, and assert the code path runs (e.g., that _initXRFeature is invoked for Initializing and that engine.xrManager.getFeature is called and handled for Initialized). Ensure the test replaces or spies on XRTrackedObjectManager._initXRFeature and supplies a minimal feature stub so the runtime-only branches are executed without a real XR runtime.packages/custom-material/src/plain-color/PlainColorMaterial.ts (1)
37-40: Add a regression test for material cloning throughengine.This accessor swap is repeated in
packages/custom-material/src/bake-pbr/BakePBRMaterial.ts:51-54andpackages/gizmo/src/icon/IconMaterial.ts:47-50. A smallclone()test that asserts the clone stays bound to the same engine and preserves copied state would protect the exact API-removal scenario this PR is fixing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/custom-material/src/plain-color/PlainColorMaterial.ts` around lines 37 - 40, Add a regression test that creates a PlainColorMaterial (using PlainColorMaterial class), sets identifiable state on it (e.g., color/flags), then clones it via the engine-backed clone path (exercise the same codepath used by PlainColorMaterial.clone and the similar implementations in BakePBRMaterial.clone and IconMaterial.clone) and assert that the resulting clone is bound to the same engine instance and that all copied state matches the original; ensure the test covers both calling material.clone() and the engine-based clone invocation so the accessor-swap regression is caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/custom-material/src/plain-color/PlainColorMaterial.ts`:
- Around line 37-40: Add a regression test that creates a PlainColorMaterial
(using PlainColorMaterial class), sets identifiable state on it (e.g.,
color/flags), then clones it via the engine-backed clone path (exercise the same
codepath used by PlainColorMaterial.clone and the similar implementations in
BakePBRMaterial.clone and IconMaterial.clone) and assert that the resulting
clone is bound to the same engine instance and that all copied state matches the
original; ensure the test covers both calling material.clone() and the
engine-based clone invocation so the accessor-swap regression is caught.
In `@packages/xr/src/trackable/XRTrackedObjectManager.ts`:
- Around line 51-57: Add a focused unit test that stubs a lightweight XR
session/feature path to cover the engine.xrManager usage in
XRTrackedObjectManager: specifically exercise
XRTrackedObjectManager._onSessionChanged with XRSessionState.Initialized (and
the similar branch around lines 115-116) by mocking this.engine to include an
xrManager with a getFeature method that returns a fake feature object, and
assert the code path runs (e.g., that _initXRFeature is invoked for Initializing
and that engine.xrManager.getFeature is called and handled for Initialized).
Ensure the test replaces or spies on XRTrackedObjectManager._initXRFeature and
supplies a minimal feature stub so the runtime-only branches are executed
without a real XR runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c79d3a3c-056e-47d7-9c01-49cdfd735b20
📒 Files selected for processing (5)
packages/custom-material/src/bake-pbr/BakePBRMaterial.tspackages/custom-material/src/plain-color/PlainColorMaterial.tspackages/gizmo/src/icon/Icon.tspackages/gizmo/src/icon/IconMaterial.tspackages/xr/src/trackable/XRTrackedObjectManager.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/stats/src/Monitor.ts (1)
68-78: Formatting improvement looks good.The multi-line array formatting improves readability.
Minor observation: the empty array assignment on line 68 is immediately overwritten and could be removed, simplifying the initialization to a single assignment.
,
♻️ Optional: Remove redundant empty array assignment
- this.items = []; - this.items = [ + this.items = [ "fps", "memory", "totalGraphicsMemory", "textureMemory", "bufferMemory", "drawCall", "triangles", "webglContext" ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stats/src/Monitor.ts` around lines 68 - 78, Remove the redundant immediate overwrite of this.items by deleting the initial this.items = []; so the property is initialized once with the multi-line array; update the constructor or the class initializer in Monitor (look for the this.items assignment in Monitor.ts) to only contain the single multi-line this.items = [ "fps", "memory", "totalGraphicsMemory", "textureMemory", "bufferMemory", "drawCall", "triangles", "webglContext" ] assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/stats/src/Monitor.ts`:
- Around line 68-78: Remove the redundant immediate overwrite of this.items by
deleting the initial this.items = []; so the property is initialized once with
the multi-line array; update the constructor or the class initializer in Monitor
(look for the this.items assignment in Monitor.ts) to only contain the single
multi-line this.items = [ "fps", "memory", "totalGraphicsMemory",
"textureMemory", "bufferMemory", "drawCall", "triangles", "webglContext" ]
assignment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: df108ed3-3017-4ab1-a1b9-eb682f72794d
📒 Files selected for processing (2)
packages/stats/src/Core.tspackages/stats/src/Monitor.ts
💤 Files with no reviewable changes (1)
- packages/stats/src/Core.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Summary by CodeRabbit