fix(server): zone-to-gap transition updates entity zone#74
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Walkthrough클라이언트의 GameScene에서 previousZone nullable 처리를 명확히 하고, 서버의 EntitySchema에 zone 타입·메서드 변경(nullable 지원 및 clearZone 추가)을 도입하며 ZoneSystem에서 엔티티가 존을 벗길 때 clearZone 호출을 일관되게 적용했습니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
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)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
Summary of ChangesHello @ComBba, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug that caused an entity's zone information to become outdated when it moved from a designated zone into an undefined, non-zoned area. The changes ensure that the server-side Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/src/zone/ZoneSystem.ts (1)
132-134:⚠️ Potential issue | 🟠 Major
updateEntityZone에서도 동일한 갭 전환 버그가 존재합니다.
update()메서드에서는entity.clearZone()을 호출하도록 수정했지만,updateEntityZone()메서드의else분기(Line 132-134)에서는entity가 전달되어도clearZone()을 호출하지 않습니다. 동일한 패턴의 버그가 남아있습니다.🐛 제안된 수정
} else { + if (entity) { + entity.clearZone(); + } this.entityZones.delete(entityId); }
🧹 Nitpick comments (3)
packages/server/src/schemas/EntitySchema.ts (1)
97-98:currentZone타입이string으로 약화되어 타입 안전성이 저하됩니다.이전에는
ZoneId타입이었지만 이제string으로 변경되어 임의의 문자열이 할당 가능합니다. "존 없음" 상태를 표현하면서 타입 안전성을 유지하려면ZoneId | ''를 사용하는 것이 좋습니다.♻️ 제안된 수정
`@type`('string') - currentZone: string = ''; + currentZone: ZoneId | '' = '';packages/client/src/game/scenes/GameScene.ts (2)
753-756:currentZone추출 시 타입 캐스팅이 취약합니다.서버에서
currentZone은 이제string이고 빈 문자열('')로 "존 없음"을 나타냅니다. 하지만as unknown as { currentZone?: ZoneId }캐스팅과?? null은undefined만null로 변환하고''는 변환하지 않습니다. 현재는''가 falsy이기 때문에 아래 조건문에서 우연히 올바르게 동작하지만, 의도를 명확히 하면 유지보수성이 높아집니다.♻️ 제안된 수정
- const currentZone = (entity as unknown as { currentZone?: ZoneId }).currentZone ?? null; + const rawZone = (entity as unknown as { currentZone?: string }).currentZone; + const currentZone: ZoneId | null = rawZone ? (rawZone as ZoneId) : null;이렇게 하면 빈 문자열도 명시적으로
null로 변환되어, 타입과 런타임 동작이 일치합니다.
787-787:previousZone에null할당 시 타입 불일치 가능성이 있습니다.
previousZone는ZoneId | undefined로 선언되어 있지만(Line 56),currentZone이null일 때this.previousZone = currentZone로null이 할당됩니다.null은undefined와 다르므로 strict TypeScript에서는 타입 에러가 될 수 있습니다.previousZone타입을ZoneId | null로 변경하거나, 할당 시currentZone ?? undefined를 사용하세요.♻️ 옵션 1: 타입 변경
- private previousZone?: ZoneId; + private previousZone: ZoneId | null = null;
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively fixes a bug where an entity's zone information was not updated when moving from a zoned area to a non-zoned area. The changes to EntitySchema to use an empty string for 'no zone' and the corresponding updates in ZoneSystem are well-implemented. The client-side logic in GameScene is also correctly updated to handle these changes and reflect them in the UI. I've included one suggestion to refactor a part of the client-side code for improved clarity and maintainability. Overall, this is a solid fix.
- EntitySchema: change currentZone type to string, default empty - EntitySchema: add clearZone() method for explicit zone clearing - EntitySchema: setZone() now accepts null (converts to empty string) - ZoneSystem: call clearZone() when entity leaves zone to gap area Previously, when a player moved from a zone to a gap (non-zone area), the currentZone property was never updated, causing the client to display stale zone information in the UI.
Address CodeRabbit review feedback - updateEntityZone had the same gap transition bug where entity.clearZone() was not called when moving to a non-zone area.
- Change previousZone type from ZoneId | undefined to ZoneId | null - Convert empty string currentZone to null for proper zone detection - Cast valid zone strings back to ZoneId type for UI components Addresses CodeRabbit review feedback on type safety.
ComBba
force-pushed
the
fix/zone-to-gap-transition
branch
from
4f7c43d to
3aa8bef
Compare
1 participant
Summary
currentZonewas not updated when moving from a zone to a gap (non-zone area)Changes
currentZonetype fromZoneIdtostringwith empty defaultclearZone()method for explicit zone clearingsetZone()to acceptnull(converts to empty string)clearZone()when entity leaves zone to gap areaRoot Cause
In
ZoneSystem.update(), the entity zone was only updated when entering a new zone:When moving to a gap (
currentZone === null), the entity's zone was never cleared.Testing
Summary by CodeRabbit
릴리스 노트