refactor(galacean): replace @ts-ignore with @ts-expect-error and modernize loop#3059
refactor(galacean): replace @ts-ignore with @ts-expect-error and modernize loop#3059luo2430 wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR updates TypeScript suppression comments from ChangesTS Suppression and Registration Cleanup
Estimated code review effort: 1 (Trivial) | ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## dev/2.0 #3059 +/- ##
===========================================
+ Coverage 79.37% 79.98% +0.61%
===========================================
Files 903 903
Lines 100632 100762 +130
Branches 11260 11250 -10
===========================================
+ Hits 79879 80597 +718
+ Misses 20569 19987 -582
+ Partials 184 178 -6
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:
|
|
变基变错了 |
luo2430
force-pushed
the
refactor/galacean
branch
from
bc87faf to
2124c78
Compare
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
纯机械化现代化改动，packages/galacean/src/ 两文件：① @ts-ignore → @ts-expect-error（3 处，目标均为已确认 @internal 成员：Shader._createFromPrecompiled / ShaderPass._feedbackVaryings / SystemInfo._initialize）；② 删除 version 上方的死 //@ts-ignore；③ for (let key in CoreObjects) → for (const [key, value] of Object.entries(CoreObjects))。方向正确，是净改进。LGTM，无 P0/P1/P2。
注：本 PR 的 CodeRabbit 自动摘要（谈 canvas sizing）是早前一次错误变基的残留（作者亦评论「变基变错了」）。当前 tip
2124c783父提交 =721b42a8c（dev/2.0 tip），单 commit 只改 2 文件、共+6 -6，已干净重基，与摘要描述无关，按实际 diff 审查。
问题
无阻塞问题。逐条已核：
-
@ts-expect-error迁移（3 处） — 严格优于@ts-ignore：@ts-ignore静默吞掉该行任何未来错误（包括无关错误），@ts-expect-error自校验——若该行不再报错，tsc会以 "Unused directive" 失败。三处目标成员均带/** @internal */，跨包（packages/galacean引@galacean/engine-core的 stripped.d.ts）访问确实报错 → 三条 directive 均为 used。CIbuild（macOS/ubuntu/windows 三平台tsc）+lint+e2e 4/4全绿，实证无死 directive。 -
删
version上的//@ts-ignore— 正确。export const version =+ 反引号__buildVersion反引号 是合法 template-literal string（rollup.config.js:80构建期 replace 成pkgJson.version），该行本就无 TS 错误，原//@ts-ignore是死 directive，删除干净。 -
for...in→Object.entries— 语义等价。CoreObjects是 module namespace object（[[Prototype]]为 null、无继承可枚举属性、key 顺序 spec 固定），且Loader.registerClass是顺序无关的注册填充。与本仓 #3058 同作者同款已验证改动一致。
简化建议
无。改动本身即是简化，代码更干净。
![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
🧹 Nitpick comments (4)
tests/vitest.config.ts (1)
17-34: 🩺 Stability & Availability | 🔵 TrivialConfirm intent of disabling screenshot capture on failures.
screenshotFailures: falseremoves a useful diagnostic artifact for debugging failed browser tests in CI. If this was purely to reduce CI I/O/storage rather than fix a workflow bug, it's worth flagging that future failures (including flaky UI/canvas ones introduced by this refactor) will be harder to triage without screenshots.🤖 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/vitest.config.ts` around lines 17 - 34, The browser test config currently disables failure screenshots in the vitest browser setup, which removes a useful debug artifact. Review the browser config in vitest.config.ts and either restore screenshot capture for failures or make the disablement conditional/documented if it is intentional for CI cost reasons. Keep the change scoped to the browser test settings under test.browser, including screenshotFailures and the Playwright instance config.tests/src/rhi-webgl/WebGLEngine.test.ts (2)
105-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider splitting this large multi-assertion test.
This single
itblock exercises default observer attachment, scale reuse, pending-resize pump (both applied and skipped cases),setResolutionlocking + validation, and destroy cleanup. Splitting into focused tests would make failures easier to localize.🤖 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/rhi-webgl/WebGLEngine.test.ts` around lines 105 - 150, The single WebGLEngine.test.ts case is doing too many unrelated assertions, making failures hard to pinpoint. Split the current “canvas auto resize” test in WebGLEngine.test into smaller focused tests around WebGLEngine.create, setAutoResolution, _pumpPendingResize, setResolution, and destroy so each behavior (observer attachment, resize application/skip, fixed resolution, validation, cleanup) is validated independently.
160-176: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueWeak assertion for
disableAutoResizeeffect.
expect(webCanvas.width).toBeGreaterThan(0)(Line 172-173) passes trivially since the canvas's default width/height are already non-zero, regardless of whetherdisableAutoResizecorrectly locked the current size. Asserting an exact expected value (e.g. matching a stubbedclientWidth/clientHeightafter a pump, or the canvas's known default dimensions) would better validate the forwarding behavior described inWebGLEngine.disableAutoResize(canvas.setResolution(canvas.width, canvas.height)).🤖 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/rhi-webgl/WebGLEngine.test.ts` around lines 160 - 176, The WebGLEngine test for disableAutoResize is too weak because checking that webCanvas.width/height are greater than zero does not prove the forwarded resize behavior. Update the assertion in WebGLEngine.test around WebGLEngine.enableAutoResize and WebGLEngine.disableAutoResize to verify the canvas was locked to the expected current size after disabling auto resize, using a concrete expected value derived from the canvas state or stubbed clientWidth/clientHeight rather than a non-zero check. This should still confirm the _resizeObserver is removed while validating the setResolution(current size) path.e2e/case/gltf-blendshape.ts (1)
22-25: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNon-integer resolution passed to
setResolution.
window.innerWidth * SystemInfo.devicePixelRatiocan yield a fractional value, unlike the auto-resize path inWebCanvas._pumpPendingResizewhich explicitly rounds pixel dimensions before calling_setSize. Passing a float here is accepted bysetResolution's validation (only checks finiteness and positivity), but the resulting fractional value will be silently truncated by the browser's canvas width/height setters, causing a slight inconsistency with the rounding behavior used elsewhere.🔧 Suggested fix for consistency
engine.canvas.setResolution( - window.innerWidth * SystemInfo.devicePixelRatio, - window.innerHeight * SystemInfo.devicePixelRatio + Math.round(window.innerWidth * SystemInfo.devicePixelRatio), + Math.round(window.innerHeight * SystemInfo.devicePixelRatio) );🤖 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 `@e2e/case/gltf-blendshape.ts` around lines 22 - 25, The explicit canvas sizing in gltf-blendshape.ts is passing fractional pixel dimensions into engine.canvas.setResolution, which differs from the rounded sizing used in WebCanvas._pumpPendingResize. Update the call site to round the computed width and height before invoking setResolution, using the same pixel-dimension handling as the auto-resize path so the resolution stays consistent across both code paths.
🤖 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 `@examples/src/device-restore.ts`:
- Line 28: Guard the optional `GLTFResource.animations` before accessing the
first clip in `device-restore.ts`; `animator?.play(...)` already handles a
missing animator, but `animations![0]` can still crash when there are no clips.
Update the `animator?.play` call site to check that `animations` exists and has
at least one entry before indexing, using the existing `animations` and
`animator` symbols to keep the logic safe.
In `@packages/core/src/Canvas.ts`:
- Around line 35-41: Canvas.setResolution currently accepts finite positive
decimals, but the backing canvas size used by WebCanvas._onSizeChanged must be
integer pixels. Update setResolution in Canvas to either reject non-integer
width/height or normalize them before calling _setSize, and keep the
validation/error message aligned with the new rule.
In `@packages/rhi-webgl/src/WebCanvas.ts`:
- Around line 49-61: The auto-resolution path in setAutoResolution currently
accepts invalid or tiny scale values and can pass 0, NaN, or sub-1 dimensions
through to _pumpPendingResize/_setSize without the validation that
Canvas.setResolution normally provides. Add validation for the scale argument in
setAutoResolution and ensure the computed width/height are rounded and clamped
to a minimum of 1 before they reach _setSize, including the same safeguard in
the resize application path used by _pumpPendingResize.
In `@packages/rhi-webgl/src/WebGLEngine.ts`:
- Around line 51-53: The enableAutoResize method in WebGLEngine currently
accepts pixelRatio but ignores it, breaking existing callers that pass a value
like 0.5. Update enableAutoResize(pixelRatio?) to preserve the deprecated
argument by translating it into the new DPR-based resolution behavior when
present, while keeping the default auto-resize path for undefined values. Make
the fix in WebGLEngine.enableAutoResize and ensure the canvas resolution setup
still works correctly with both old pixelRatio callers and the new automatic
model.
---
Nitpick comments:
In `@e2e/case/gltf-blendshape.ts`:
- Around line 22-25: The explicit canvas sizing in gltf-blendshape.ts is passing
fractional pixel dimensions into engine.canvas.setResolution, which differs from
the rounded sizing used in WebCanvas._pumpPendingResize. Update the call site to
round the computed width and height before invoking setResolution, using the
same pixel-dimension handling as the auto-resize path so the resolution stays
consistent across both code paths.
In `@tests/src/rhi-webgl/WebGLEngine.test.ts`:
- Around line 105-150: The single WebGLEngine.test.ts case is doing too many
unrelated assertions, making failures hard to pinpoint. Split the current
“canvas auto resize” test in WebGLEngine.test into smaller focused tests around
WebGLEngine.create, setAutoResolution, _pumpPendingResize, setResolution, and
destroy so each behavior (observer attachment, resize application/skip, fixed
resolution, validation, cleanup) is validated independently.
- Around line 160-176: The WebGLEngine test for disableAutoResize is too weak
because checking that webCanvas.width/height are greater than zero does not
prove the forwarded resize behavior. Update the assertion in WebGLEngine.test
around WebGLEngine.enableAutoResize and WebGLEngine.disableAutoResize to verify
the canvas was locked to the expected current size after disabling auto resize,
using a concrete expected value derived from the canvas state or stubbed
clientWidth/clientHeight rather than a non-zero check. This should still confirm
the _resizeObserver is removed while validating the setResolution(current size)
path.
In `@tests/vitest.config.ts`:
- Around line 17-34: The browser test config currently disables failure
screenshots in the vitest browser setup, which removes a useful debug artifact.
Review the browser config in vitest.config.ts and either restore screenshot
capture for failures or make the disablement conditional/documented if it is
intentional for CI cost reasons. Keep the change scoped to the browser test
settings under test.browser, including screenshotFailures and the Playwright
instance config.
🪄 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: fb7f47a0-2b91-4536-b7f2-38bcf351119e
⛔ Files ignored due to path filters (7)
tests/src/core/2d/text/__screenshots__/TextRenderer.test.ts/TextRenderer-bounds-1.pngis excluded by!**/*.pngtests/src/core/__screenshots__/Sprite.test.ts/Sprite-get-set-size-1.pngis excluded by!**/*.pngtests/src/core/__screenshots__/SpriteMask.test.ts/SpriteMask--render-1.pngis excluded by!**/*.pngtests/src/core/__screenshots__/SpriteMask.test.ts/SpriteMask-get-set-size-1.pngis excluded by!**/*.pngtests/src/core/__screenshots__/SpriteRenderer.test.ts/SpriteRenderer--render-1.pngis excluded by!**/*.pngtests/src/core/__screenshots__/SpriteRenderer.test.ts/SpriteRenderer-get-set-size-1.pngis excluded by!**/*.pngtests/src/core/audio/__screenshots__/AudioSource.test.ts/AudioSource-load-1.pngis excluded by!**/*.png
📒 Files selected for processing (122)
e2e/case/.initPostProcessEnv.tse2e/case/animator-additive.tse2e/case/animator-blendShape-quantization.tse2e/case/animator-blendShape.tse2e/case/animator-crossfade.tse2e/case/animator-customAnimationClip.tse2e/case/animator-customBlendShape.tse2e/case/animator-event.tse2e/case/animator-multiSubMeshBlendShape.tse2e/case/animator-play-backwards.tse2e/case/animator-play-beforeActive.tse2e/case/animator-play.tse2e/case/animator-reuse.tse2e/case/animator-stateMachine.tse2e/case/animator-stateMachineScript.tse2e/case/camera-fxaa.tse2e/case/camera-opaque-texture.tse2e/case/camera-ssao.tse2e/case/canvas-transparency.tse2e/case/gltf-blendshape.tse2e/case/gltf-meshopt.tse2e/case/gpu-instancing-auto-batch.tse2e/case/gpu-instancing-custom-data.tse2e/case/material-LUT.tse2e/case/material-blendMode.tse2e/case/material-blinn-phong.tse2e/case/material-pbr-clearcoat.tse2e/case/material-pbr-specular.tse2e/case/material-pbr.tse2e/case/material-shader.tse2e/case/material-shaderReplacement.tse2e/case/material-unlit.tse2e/case/material-white-furnace.tse2e/case/multi-camera-no-clear.tse2e/case/multi-scene-clear.tse2e/case/multi-scene-no-clear.tse2e/case/particleRenderer-burst-cycles.tse2e/case/particleRenderer-customShader.tse2e/case/particleRenderer-dream.tse2e/case/particleRenderer-emissive.tse2e/case/particleRenderer-emit-billboard-stretched.tse2e/case/particleRenderer-emit-mesh-cone-scale-3d-rotation-life-seperate.tse2e/case/particleRenderer-emit-mesh-cone-scale-3d-rotation.tse2e/case/particleRenderer-emit-mesh-cone-scale-rotation-life-seperate.tse2e/case/particleRenderer-emit-mesh-cone-scale-rotation-life.tse2e/case/particleRenderer-emit-mesh-cone-scale-rotation-world.tse2e/case/particleRenderer-emit-mesh-cone-scale-rotation.tse2e/case/particleRenderer-emit-mesh-cone.tse2e/case/particleRenderer-emit-mesh-no-shape-world.tse2e/case/particleRenderer-emit-mesh-no-shape.tse2e/case/particleRenderer-emit-mesh-rotation-life-curve.tse2e/case/particleRenderer-fire.tse2e/case/particleRenderer-force.tse2e/case/particleRenderer-horizontal-billboard.tse2e/case/particleRenderer-limitVelocity.tse2e/case/particleRenderer-noise.tse2e/case/particleRenderer-rateOverDistance.tse2e/case/particleRenderer-shape-mesh.tse2e/case/particleRenderer-shape-transform.tse2e/case/particleRenderer-sub-emitter.tse2e/case/particleRenderer-textureSheetAnimation.tse2e/case/physx-collision-group.tse2e/case/physx-collision.tse2e/case/physx-customUrl.tse2e/case/physx-deferred-contact.tse2e/case/physx-mesh-collider-data.tse2e/case/physx-mesh-collider.tse2e/case/primitive-capsule.tse2e/case/primitive-cone.tse2e/case/primitive-cuboid.tse2e/case/primitive-cylinder.tse2e/case/primitive-plane.tse2e/case/primitive-sphere.tse2e/case/primitive-torus.tse2e/case/shader-mrt.tse2e/case/shader-renderState.tse2e/case/shadow-basic.tse2e/case/shadow-transparent.tse2e/case/spriteMask-customStencil.tse2e/case/text-character-spacing.tse2e/case/text-typed.tse2e/case/texture-R8G8.tse2e/case/texture-hdr-ktx2.tse2e/case/texture-hdr.tse2e/case/texture-sRGB-KTX2.tse2e/case/trailRenderer-basic.tse2e/case/ui-batch-order.tsexamples/src/CSS-DOM.tsexamples/src/buffer-mesh-independent.tsexamples/src/buffer-mesh-instance.tsexamples/src/buffer-mesh-interleaved.tsexamples/src/device-restore.tsexamples/src/gltf-loader.tsexamples/src/gpu-instancing-auto-batch.tsexamples/src/gpu-instancing-custom-data.tsexamples/src/paricle-emit-mesh.tsexamples/src/project-loader.tsexamples/src/screenshot.tsexamples/src/shader-01-basic-shader.tsexamples/src/shader-02-render-state.tsexamples/src/shader-03-ui-script.tsexamples/src/shader-04-multi-pass.tsexamples/src/shader-05-advance.tsexamples/src/ui-batch-massive.tspackages/core/src/Canvas.tspackages/galacean/src/ShaderPool.tspackages/galacean/src/index.tspackages/rhi-webgl/src/WebCanvas.tspackages/rhi-webgl/src/WebGLEngine.tspackages/rhi-webgl/src/WebGLGraphicDevice.tstests/src/core/Camera.test.tstests/src/core/input/InputManager.test.tstests/src/rhi-webgl/WebCanvas.test.tstests/src/rhi-webgl/WebGLEngine.test.tstests/src/ui/Image.test.tstests/src/ui/Text.test.tstests/src/ui/UICanvas.test.tstests/src/ui/UIEvent.test.tstests/src/ui/UIGroup.test.tstests/src/ui/UIInteractive.test.tstests/src/ui/UITransform.test.tstests/vitest.config.ts
💤 Files with no reviewable changes (15)
- examples/src/ui-batch-massive.ts
- examples/src/CSS-DOM.ts
- examples/src/screenshot.ts
- examples/src/shader-02-render-state.ts
- examples/src/buffer-mesh-independent.ts
- examples/src/shader-05-advance.ts
- examples/src/shader-04-multi-pass.ts
- examples/src/gpu-instancing-custom-data.ts
- examples/src/shader-01-basic-shader.ts
- examples/src/gpu-instancing-auto-batch.ts
- examples/src/gltf-loader.ts
- examples/src/shader-03-ui-script.ts
- examples/src/buffer-mesh-interleaved.ts
- examples/src/project-loader.ts
- examples/src/buffer-mesh-instance.ts
2 participants
Summary by CodeRabbit
Refactor
Chores