fix(2d): fix typo in SpriteAtlas.getSprites warning message#3032
fix(2d): fix typo in SpriteAtlas.getSprites warning message#3032cptbtptpbcptdtptp wants to merge 4 commits into
Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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 (1)
WalkthroughThe ChangesSprite Lookup and Warning
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 docstrings
🧪 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 #3032 +/- ##
========================================
Coverage 79.17% 79.17%
========================================
Files 914 914
Lines 101666 101666
Branches 11209 11213 +4
========================================
+ Hits 80489 80499 +10
+ Misses 20990 20980 -10
Partials 187 187
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:
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… messages Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/src/core/SpriteAtlas.test.ts`:
- Around line 22-29: The warnSpy.mockRestore() call at the end of both test
cases (lines 22-29 and 53-62) only executes on the success path, so if any
assertion fails before reaching it, the spy cleanup is skipped and leaks into
later tests. Wrap the test logic in a try-finally block in both locations so
that warnSpy.mockRestore() is guaranteed to execute regardless of whether
assertions pass or fail. This ensures the spy is properly cleaned up even when
test assertions fail.
- Around line 5-8: The WebGLEngine instance created via WebGLEngine.create is
never destroyed, causing WebGL resource leaks. Add an afterEach hook in the
SpriteAtlas describe block that calls the appropriate destroy or dispose method
on the engine variable to ensure proper cleanup after each test runs and prevent
cross-test instability.
🪄 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: acfa9c8e-c2ee-4dc8-8e26-871003f43f50
📒 Files selected for processing (1)
tests/src/core/SpriteAtlas.test.ts
| describe("SpriteAtlas", async () => { | ||
| const canvas = document.createElement("canvas"); | ||
| const engine = await WebGLEngine.create({ canvas: canvas }); | ||
|
|
There was a problem hiding this comment.
Add engine teardown to prevent test resource leaks.
WebGLEngine is created once but never destroyed. This can leak WebGL resources and cause cross-test instability in browser runs. Add explicit teardown.
Suggested fix
-import { describe, expect, it, vi } from "vitest";
+import { afterAll, beforeAll, describe, expect, it, vi } from "vitest";
-describe("SpriteAtlas", async () => {
- const canvas = document.createElement("canvas");
- const engine = await WebGLEngine.create({ canvas: canvas });
+describe("SpriteAtlas", () => {
+ let engine: WebGLEngine;
+
+ beforeAll(async () => {
+ const canvas = document.createElement("canvas");
+ engine = await WebGLEngine.create({ canvas });
+ });
+
+ afterAll(() => {
+ engine.destroy();
+ });📝 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.
| describe("SpriteAtlas", async () => { | |
| const canvas = document.createElement("canvas"); | |
| const engine = await WebGLEngine.create({ canvas: canvas }); | |
| import { afterAll, beforeAll, describe, expect, it, vi } from "vitest"; | |
| describe("SpriteAtlas", () => { | |
| let engine: WebGLEngine; | |
| beforeAll(async () => { | |
| const canvas = document.createElement("canvas"); | |
| engine = await WebGLEngine.create({ canvas }); | |
| }); | |
| afterAll(() => { | |
| engine.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/SpriteAtlas.test.ts` around lines 5 - 8, The WebGLEngine
instance created via WebGLEngine.create is never destroyed, causing WebGL
resource leaks. Add an afterEach hook in the SpriteAtlas describe block that
calls the appropriate destroy or dispose method on the engine variable to ensure
proper cleanup after each test runs and prevent cross-test instability.
Prevents console.warn spy from leaking into subsequent tests when an assertion fails. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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.
总结
增量审查：当前 HEAD 256e3cf 与上轮我 review 所基于的 commit 相同，但上轮 P3 是基于早于本 commit 的快照写的——本 commit（"use afterEach to restore mocks"，提交于上轮 review 前 1m43s）恰好已实现该修复。复核当前代码全部到位：
- typo 修复完整闭环 ——
getSprites未命中分支（SpriteAtlas.ts:48）与getSprite（SpriteAtlas.ts:27）现在文案完全一致："There is no sprite named " + name + " in the atlas."，PR 主旨已达成。 - 上轮 P3 已修复（闭环） —— 测试改用顶层
afterEach(() => vi.restoreAllMocks())，并移除了每个it末尾的手动warnSpy.mockRestore()。断言中途抛错也不会再让console.warnmock 泄漏到后续测试。这正是上轮我建议的改法，CodeRabbit 的 "spy cleanup on assertion failure" 也由同一 commit 一并解决（其 inline comment 已标 "✅ Addressed in commit 256e3cf"）。 - codecov 绿 —— "All modified and coverable lines are covered"。
已关闭项不再重复：
- typo / 文案一致性 —— 已修复。
- engine teardown（CodeRabbit 建议加
engine.destroy()） —— 上轮判定不采纳（跟随tests/src/core/主流惯例，26 个测试文件仅 3 个 teardown），结论不变。 describe顶层await WebGLEngine.create、_addSprite同步登记、multi-match 反向收集契约 —— 上轮均已核对正确。
问题
无。上轮唯一的 P3 已在本 commit 修复，无新增问题。
简化建议
测试干净，覆盖到位，mock 清理已收口到 afterEach。LGTM，可以合并。
GuoLei1990
left a comment
GuoLei1990
left a comment
There was a problem hiding this comment.
总结
增量审查：当前 HEAD 2ca4126 是一个 merge commit（把 origin/dev/2.0 合进本分支），是相对上轮 256e3cf 的唯一新变更。本 PR 实际改动的两个文件 —— packages/core/src/2d/atlas/SpriteAtlas.ts 与 tests/src/core/SpriteAtlas.test.ts —— git diff 256e3cf..2ca4126 对这两个文件结果为空，即与上轮我 review 时字节完全一致。merge 带入的全部是 dev/2.0 的无关变更（shader compiledShaders 清理、particle/gltf 测试、shader-compiler 测试、lockfile），不属于本 PR 的 authored diff，不在本次审查范围。
复核 PR HEAD 上的修复仍在位：
- typo 修复闭环不变 ——
getSprites未命中分支（SpriteAtlas.ts:48）与getSprite（SpriteAtlas.ts:27）文案完全一致："There is no sprite named " + name + " in the atlas."。 - 测试无变化 ——
afterEach(() => vi.restoreAllMocks())的 mock 清理收口、4 个命中/未命中分支用例均与上轮一致，依旧正确。 - 无新评论 / 无新作者回复 —— 最后一次活动是我上轮
06:26:42Z的 review；CodeRabbit 两条 finding（engine teardown / spy cleanup）状态不变（前者上轮判定不采纳并跟随项目惯例，后者已在256e3cf修复并被 CodeRabbit 标注 ✅）。
已关闭项不再重复：typo / 文案一致性、codecov 误报、engine teardown 惯例判定、spy cleanup（P3 已修复）、_addSprite 同步登记与 multi-match 反向收集契约 —— 结论均不变。
问题
无。本轮唯一变更是 merge dev/2.0，未触碰本 PR 的 authored 改动，无新增问题。
简化建议
merge 后 PR 仍只是一处字符串文案修复 + 配套测试，干净、覆盖到位。LGTM，可以合并。
2 participants
Summary
SpriteAtlas.getSpriteswarning: "not exit" → "no sprite named X in the atlas"getSpriteTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit