Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (2)
Tip Migrating from UI to YAML configuration.Use the WalkthroughWagonRoom is refactored to accept Changes
Sequence DiagramsequenceDiagram
participant Init as Initialization
participant WR as WagonRoom
participant WRGen as WagonRoom.generate()
participant Charge as Charge Plugin
Init->>WR: new WagonRoom({ id, chunks: 6 })
activate WR
WR->>WR: this.init()
WR->>WRGen: this.generate(chunks)
activate WRGen
WRGen->>WRGen: Create firstChunk, forestChunk, finalChunk
WRGen->>WR: Push chunk objects into this.objects
deactivate WRGen
deactivate WR
Init->>Charge: Plugin initialization
activate Charge
alt import.meta.dev === true
Charge->>Charge: Log & exit
else Production
Charge->>Charge: await initCharges()
end
deactivate Charge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/website/server/plugins/04.startCharge.ts (1)
1-11: Harden startup: catch failures from initCharges to avoid crashing prod boot.External controllers can fail (network/creds). Log and continue or rethrow based on policy.
Apply this diff:
export default defineNitroPlugin(async () => { const logger = useLogger('plugin-start-charge') // Only run in production if (import.meta.dev) { logger.info('Charge server not started in dev mode') return } - await initCharges() + try { + await initCharges() + logger.success('Charge server started') + } catch (err) { + logger.error('Failed to start charges', err) + // Option A: swallow to keep server up + // return + // Option B: rethrow to fail fast: + // throw err + } })apps/website/server/plugins/03.startWagonRoom.ts (1)
29-37: Remove unnecessary async or await the call.rebootRoom() has no awaits; either remove async or await when invoked for clarity.
Two options:
- Remove async:
-async function rebootRoom() { +function rebootRoom() {
- Or await calls:
- rebootRoom() + await rebootRoom()apps/website/server/core/rooms/wagon.ts (2)
267-282: Consistency: also assign IDs when creating chunks elsewhere.generate() now assigns ids to chunks; initFirstChunk/initNextChunk still don’t. If chunk constructors require id, those calls will break; even if optional, consistency helps.
Proposed updates (outside this hunk):
// Line 98 const newForest = new ForestChunk({ startX, endX, id: createId() }) // Line 109 const newForest = new ForestChunk({ startX: previousChunk.endX, endX: previousChunk.endX + getRandomInRange(2000, 3000), id: createId(), })If ForestChunk/VillageChunk types require id, this is mandatory. Please confirm constructor signatures.
24-29: Optional: generate before scheduling timers.Minor ordering tweak: generate(chunks) before init() avoids a brief tick with empty chunks.
Apply this diff:
- constructor({ id, chunks }: WagonRoomOptions) { + constructor({ id, chunks }: WagonRoomOptions) { super({ id, type: 'WAGON' }) - - this.init() - this.generate(chunks) + this.generate(chunks) + this.init() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/website/server/core/rooms/wagon.ts(6 hunks)apps/website/server/plugins/03.startWagonRoom.ts(1 hunks)apps/website/server/plugins/04.startCharge.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/website/server/plugins/03.startWagonRoom.ts (2)
apps/website/server/core/rooms/index.ts (1)
activeRooms(3-3)apps/website/server/core/rooms/wagon.ts (1)
WagonRoom(16-297)
apps/website/server/plugins/04.startCharge.ts (1)
apps/website/server/utils/charge.ts (1)
initCharges(7-28)
🔇 Additional comments (2)
apps/website/server/core/rooms/wagon.ts (2)
251-296: LGTM: generate() API and object population.Switch to chunksCount and pushing chunk objects into this.objects improves consistency and initial world state.
24-29: No action needed — all WagonRoom instantiations already use the correct signature.Verification confirms only one instantiation of WagonRoom exists in the codebase at
apps/website/server/plugins/03.startWagonRoom.ts:35, and it already passes{ id, chunks }in the correct format. The constructor change has been properly updated at all call sites.
| constructor({ id, chunks }: WagonRoomOptions) { | ||
| super({ id, type: 'WAGON' }) | ||
|
|
||
| this.init() | ||
| this.generate(chunks) | ||
| } |
There was a problem hiding this comment.
Clear timers on finish to avoid runaway intervals and leaks.
init() sets intervals; reboot() marks FINISHED but never clears them. Over time, each reboot adds two more active timers.
Apply this diff to track and clear timers; also clear them during reboot():
export class WagonRoom extends BaseRoom {
+ private updateTimer: ReturnType<typeof setInterval> | null = null
+ private logTimer: ReturnType<typeof setInterval> | null = null
wagon!: GameObject & GameObjectWagon
@@
constructor({ id, chunks }: WagonRoomOptions) {
super({ id, type: 'WAGON' })
this.init()
this.generate(chunks)
}
@@
async init() {
this.initWagon(200)
- setInterval(() => {
- this.update()
- }, 250)
+ this.updateTimer = setInterval(() => this.update(), 250)
- setInterval(() => {
- logger.log(`Chunks on Wagon Room: ${this.chunks.length}`, `Objects on Wagon Room: ${this.objects.length}`)
- }, 60 * 60 * 1000)
+ this.logTimer = setInterval(
+ () => logger.log(`Chunks on Wagon Room: ${this.chunks.length}`, `Objects on Wagon Room: ${this.objects.length}`),
+ 60 * 60 * 1000,
+ )
}
@@
async reboot() {
if (this.status !== 'ACTIVE') {
return
}
sendMessage({ type: 'ROOM_DESTROYED', data: { id: this.id } }, this.id)
this.status = 'FINISHED'
+ this.destroy()
}
+
+ destroy() {
+ if (this.updateTimer) {
+ clearInterval(this.updateTimer)
+ this.updateTimer = null
+ }
+ if (this.logTimer) {
+ clearInterval(this.logTimer)
+ this.logTimer = null
+ }
+ }Also applies to: 37-47, 64-71
🤖 Prompt for AI Agents
In apps/website/server/core/rooms/wagon.ts around lines 24-29 (and also apply
changes at 37-47 and 64-71), init() creates intervals but their IDs are not
stored or cleared, so reboot()/finish paths leave timers running and cause
leaks; modify the class to add properties to store the interval/timeout IDs
created in init(), clear those timers with clearInterval/clearTimeout when
reboot() marks the room FINISHED (and also when the room is disposed/finished),
and ensure init() assigns the returned IDs to these properties so subsequent
reboots/starts reuse/clear them safely.
| activeRooms.push( | ||
| new WagonRoom({ id: wagonRoomId, chunks: 6 }), | ||
| ) |
There was a problem hiding this comment.
Prevent memory leak: destroy old WagonRoom before replacing it.
The room schedules intervals in init(); replacing it without clearing timers leaves orphaned intervals.
Apply this diff to call destroy() before splice (paired with WagonRoom.destroy() in wagon.ts comment):
- if (activeRooms.find((room) => room.id === wagonRoomId)) {
- activeRooms.splice(activeRooms.findIndex((room) => room.id === wagonRoomId), 1)
- }
+ const idx = activeRooms.findIndex((room) => room.id === wagonRoomId)
+ if (idx !== -1) {
+ const existing = activeRooms[idx] as WagonRoom
+ ;(existing as any)?.destroy?.()
+ activeRooms.splice(idx, 1)
+ }
activeRooms.push(
new WagonRoom({ id: wagonRoomId, chunks: 6 }),
)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/website/server/plugins/03.startWagonRoom.ts around lines 34 to 36,
replacing an existing WagonRoom by splicing into activeRooms without calling its
destroy() causes orphaned intervals and a memory leak; before you call splice to
replace the room, lookup the old WagonRoom instance (e.g., by index or id) and
call its destroy() method to clear timers/cleanup, then perform the splice and
push the new WagonRoom as currently written.
| return | ||
| } | ||
|
|
||
| await initCharges() |
There was a problem hiding this comment.
Fix missing import for initCharges.
Without an import, this will throw at runtime.
Apply this diff:
+import { initCharges } from '~~/server/utils/charge'
export default defineNitroPlugin(async () => {📝 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.
| await initCharges() | |
| import { initCharges } from '~~/server/utils/charge' | |
| export default defineNitroPlugin(async () => { | |
| await initCharges() | |
| }) |
🤖 Prompt for AI Agents
In apps/website/server/plugins/04.startCharge.ts around line 10, the call to
await initCharges() is missing its import; add an import statement at the top of
this file importing initCharges from the module that defines it (e.g., import {
initCharges } from '<correct-relative-path>'), ensure the path and named export
match the source file, and run the TypeScript build to verify there are no
unresolved-import or export issues.
1 participant
Summary by CodeRabbit
New Features
Refactor
Chores