Merge branch 'main' of https://github.com/decentraland/js-sdk-toolchain into feat/event-system-helpers-improvements

Author: pravusjif

AGENTS.md

@@ -12,6 +12,10 @@ Before doing non-trivial work, read:
 
 For deeper context on a specific subsystem (component serialization, CRDT suppression, world transform behavior, etc.), use `docs/REFERENCES.md` as the entry point.
 
+## Reviewing PRs
+
+When reviewing a PR (or authoring one), read **[REVIEW.md](REVIEW.md)** first and apply every rule in it. It defines blocking reject conditions: pervasive narration comments, AI-filler PR descriptions without a human-readable "how to test", and bug fixes shipped without a test scene or failing-test repro.
+
 ## Packages
 
 Six packages under `packages/@dcl/` (a monorepo layout, but **not** npm workspaces):

REVIEW.md

@@ -0,0 +1,56 @@
+# Review guidelines
+
+Rules every reviewer (human or agent) must apply to every PR against this repo. These are not
+style suggestions — the **Reject** conditions are blocking.
+
+## 1. Code comments
+
+Comments are for non-obvious *why* only: a gotcha, a constraint, a workaround, an invariant
+the code can't express. Not for narrating what the code does, why the change is correct, or
+what the author was thinking.
+
+Flag and request removal of:
+
+- Comments on every (or almost every) changed hunk. That's the author talking to the
+  reviewer, not to the next reader — the PR description is the place for that.
+- Comments that restate the code ("increment the counter", "same guard as above").
+- Justification comments ("this cannot fire today", "belt-and-braces", "unreachable from
+  the CRDT path today"). If it can't fire, it doesn't need defending in a comment; if the
+  reasoning matters, it belongs in the PR description or a test.
+- Multi-line comment blocks where one short line would do.
+
+**Reject** when comment noise is pervasive — i.e. removing the narration comments would
+touch most hunks of the diff.
+
+## 2. PR description
+
+Written for humans, in few words. A reviewer should understand the PR from the description
+in under a minute. It must answer, plainly:
+
+- **What / why** — what is changing and why, in a short paragraph. No essay, no
+  "Mechanism" deep-dives, no tables of internals. If the bug analysis is genuinely worth
+  keeping, link to an issue or put it in a collapsed section — don't make it the description.
+- **How to test** — concrete steps a reviewer can run.
+- **Proof** — a test scene (or equivalent runnable repro) that demonstrates the bug before
+  the change and the fix after it.
+
+**Reject** when:
+
+- The description reads as generated filler: long, exhaustive, structured like a report,
+  and you still can't tell in a minute what to test.
+- There is no "how to test" section a human can actually follow.
+
+## 3. Proof of fix
+
+A fix PR must ship something that proves it: a test scene exercising the broken behavior,
+or a test that fails on `main` and passes on the branch. "The unit tests pass" is not
+proof that the reported scenario is fixed.
+
+**Reject** any bug-fix PR that has no test scene or failing-test repro attached.
+
+## 4. Posting review findings
+
+- Post one change request per violation, anchored to the file/line where it applies
+  (description-level issues go on the PR conversation).
+- Quote the rule number you're applying (e.g. "REVIEW.md §1: narration comment").
+- Don't pad the review: if the PR passes all rules, say so in one line.

docs/REFERENCES.md

@@ -9,6 +9,7 @@ Curated map of every doc in this repo. Use this as the entry point when looking
 | [ai-agent-context.md](ai-agent-context.md) | **Canonical project briefing** — every package's purpose, key concepts (ECS, CRDT, composites, React reconciler, transports), tech stack, full CLI command reference, deployment environments (`.zone` / `peer-testing` / `.org`), project layout, and what's out of scope. Start here. |
 | [../README.md](../README.md) | Public-facing repo overview, package list, quick-start (clone → `make install` → `make build`). |
 | [../AGENTS.md](../AGENTS.md) | Agent-facing instructions: dev commands, conventions, spec workflow, shell safety. |
+| [../REVIEW.md](../REVIEW.md) | PR review rules every reviewer (human or agent) must apply: code-comment policy, PR description requirements, proof-of-fix requirement, and the blocking reject conditions. |
 
 ## Architecture & runtime internals
 

package-lock.json

@@ -9,7 +9,7 @@
     "@babel/plugin-transform-modules-commonjs": "^7.28.5",
     "@babel/plugin-transform-react-jsx": "^7.28.5",
     "@babel/preset-typescript": "^7.28.5",
-    "@dcl/protocol": "1.0.0-31211128817.commit-215d09c",
+    "@dcl/protocol": "1.0.0-31617402096.commit-86c4613",
     "@dcl/quickjs-emscripten": "^0.21.0-3680274614.commit-1808aa1",
     "@dcl/ts-proto": "1.153.0",
     "@types/fs-extra": "^9.0.12",

package.json

@@ -101,6 +101,17 @@ export type IEntityContainer = {
   updateUsedEntity(entity: Entity): boolean
 }
 
+/**
+ * True when `entity`'s NUMBER falls in the renderer-reserved range, at any version.
+ *
+ * Masks rather than calling `EntityUtils.fromEntityId`, which allocates a tuple per call to
+ * read one number; this runs on every inbound CRDT message. `entity & MAX_U16` already lands
+ * in [0, 65535], so the `>>> 0` fromEntityId applies is a no-op here.
+ */
+function isReservedEntity(entity: Entity, reservedStaticEntities: number): boolean {
+  return (entity & MAX_U16) < reservedStaticEntities
+}
+
 /**
  * @public
  */
@@ -143,7 +154,11 @@ export function createEntityContainer(opts?: { reservedStaticEntities: number })
     }
 
     for (const [number, version] of removedEntities.getMap()) {
-      if (version < MAX_U16) {
+      // Never recycle a renderer-reserved number: the renderer reissues those slots as
+      // toEntityId(number, version + 1) too, from the same stored version, so it would hand
+      // the scene an id belonging to a live remote player. Belt-and-braces — every writer
+      // into `removedEntities` refuses reserved numbers, so this cannot fire today.
+      if (number >= reservedStaticEntities && version < MAX_U16) {
         const entity = EntityUtils.toEntityId(number, version + 1)
         // If the entity is not being used, we can re-use it
         // If the entity was removed in this tick, we're not counting for the usedEntities, but we have it in the toRemoveEntityArray
@@ -158,7 +173,7 @@ export function createEntityContainer(opts?: { reservedStaticEntities: number })
   }
 
   function removeEntity(entity: Entity) {
-    if (entity < reservedStaticEntities) return false
+    if (isReservedEntity(entity, reservedStaticEntities)) return false
 
     if (usedEntities.has(entity)) {
       usedEntities.delete(entity)
@@ -185,6 +200,12 @@ export function createEntityContainer(opts?: { reservedStaticEntities: number })
   }
 
   function updateRemovedEntity(entity: Entity) {
+    // Called for EVERY inbound DELETE_ENTITY, including the renderer's tombstones for
+    // departed remote players — so this is the door reserved numbers would otherwise enter
+    // the free list through. They need no tombstone: getEntityState reports them Reserved
+    // before it consults `removedEntities`, so `Removed` is unreachable for them anyway.
+    if (isReservedEntity(entity, reservedStaticEntities)) return false
+
     const [n, v] = EntityUtils.fromEntityId(entity)
 
     // Update the removed entities map
@@ -199,6 +220,11 @@ export function createEntityContainer(opts?: { reservedStaticEntities: number })
   }
 
   function updateUsedEntity(entity: Entity) {
+    // Same invariant as updateRemovedEntity. Unreachable from the CRDT path today
+    // (getEntityState returns `Reserved`, never `Unknown`), but the `v > 0` branch below
+    // would seed `removedEntities` with a reserved number.
+    if (isReservedEntity(entity, reservedStaticEntities)) return false
+
     const [n, v] = EntityUtils.fromEntityId(entity)
 
     // if the entity was removed then abort fast
@@ -216,11 +242,14 @@ export function createEntityContainer(opts?: { reservedStaticEntities: number })
   }
 
   function getEntityState(entity: Entity): EntityState {
-    const [n, v] = EntityUtils.fromEntityId(entity)
-    if (n < reservedStaticEntities) {
+    // Same guard as the three above, so `Engine.removeEntity` — which classifies via
+    // getEntityState — cannot disagree with whether this container releases the id.
+    if (isReservedEntity(entity, reservedStaticEntities)) {
       return EntityState.Reserved
     }
 
+    const [n, v] = EntityUtils.fromEntityId(entity)
+
     if (usedEntities.has(entity)) {
       return EntityState.UsedEntity
     }

packages/@dcl/ecs/src/engine/entity.ts

@@ -8,7 +8,7 @@ import { ByteBuffer } from '../serialization/ByteBuffer'
 import { crdtSceneSystem, OnChangeFunction } from '../systems/crdt'
 import { ComponentDefinition } from './component'
 import { createComponentDefinitionFromSchema } from './lww-element-set-component-definition'
-import { Entity, createEntityContainer } from './entity'
+import { Entity, EntityState, EntityUtils, createEntityContainer } from './entity'
 import { ReadonlyComponentSchema } from './readonly'
 import { SystemItem, SystemContainer, SystemFn, SYSTEMS_REGULAR_PRIORITY } from './systems'
 import type {
@@ -29,6 +29,9 @@ export * from './readonly'
 export * from './types'
 export { Entity, ByteBuffer, SystemItem, OnChangeFunction }
 
+/** RootEntity 0, PlayerEntity 1, CameraEntity 2 — see the engineInstance fields below. */
+const NAMED_STATIC_ENTITIES = 3
+
 function preEngine(options?: IEngineOptions): PreEngine {
   const entityContainer = options?.entityContainer ?? createEntityContainer()
   const componentsDefinition = new Map<number, ComponentDefinition<unknown>>()
@@ -49,6 +52,20 @@ function preEngine(options?: IEngineOptions): PreEngine {
     return entity
   }
   function removeEntity(entity: Entity) {
+    // The renderer streams the avatar range and drops the scene's deletes there, so purging
+    // locally is permanent: a one-shot component like PlayerIdentityData is never re-sent, and
+    // the entity is left as a moving Transform with no identity. The three named static
+    // entities are reserved too, but the renderer DOES apply scene deletes on them — that is
+    // how InputModifier.deleteFrom(engine.PlayerEntity) clears an input lock — so they must
+    // still be purged. Asking the container keeps this in step with whether it releases the
+    // id, including for a custom container with a different reserved range.
+    const [entityNumber] = EntityUtils.fromEntityId(entity)
+    const isAvatarEntity =
+      entityNumber >= NAMED_STATIC_ENTITIES && entityContainer.getEntityState(entity) === EntityState.Reserved
+
+    const released = entityContainer.removeEntity(entity)
+    if (isAvatarEntity) return released
+
     for (const [, component] of componentsDefinition) {
       // TODO: hack for the moment.
       // We still need the NetworkEntity to forward this message to the SyncTransport.
@@ -57,7 +74,7 @@ function preEngine(options?: IEngineOptions): PreEngine {
       component.entityDeleted(entity, true)
     }
 
-    return entityContainer.removeEntity(entity)
+    return released
   }
 
   function removeEntityWithChildren(entity: Entity) {
@@ -337,7 +354,12 @@ export function Engine(options?: IEngineOptions): IEngine {
     PlayerEntity: 1 as Entity,
     CameraEntity: 2 as Entity,
 
-    getEntityState: partialEngine.entityContainer.getEntityState,
+    // Delegated, not aliased. A detached `entityContainer.getEntityState` reference binds
+    // `this` to the engine, so a custom IEntityContainer that uses `this` silently reports the
+    // wrong state through the public API while the engine itself, which calls the container
+    // directly, reports the right one. Harmless for the built-in closure-based container, but
+    // removeEntity now classifies through getEntityState, so the two must never diverge.
+    getEntityState: (entity: Entity) => partialEngine.entityContainer.getEntityState(entity),
     addTransport: crdtSystem.addTransport,
 
     entityContainer: partialEngine.entityContainer

packages/@dcl/ecs/src/engine/index.ts

@@ -92,12 +92,19 @@ export interface IEngine {
    * @public
    * Remove all components of an entity
    * @param entity - entity
+   * @returns whether the entity id was released for reuse. Ids in the renderer-reserved range
+   *   are never released, at any version. Components are still purged for
+   *   RootEntity/PlayerEntity/CameraEntity, but not for the avatar range.
    */
-  removeEntity(entity: Entity): void
+  removeEntity(entity: Entity): boolean
 
   /**
    * Remove all components of each entity in the tree made with Transform parenting
    * @param entity - the root entity of the tree
+   *
+   * May complete only partially and does not report it: nodes in the renderer-reserved range
+   * are refused by `removeEntity`, so a reserved node anywhere in the tree survives while its
+   * descendants are removed — leaving its `Transform.parent` pointing at a removed entity.
    */
   removeEntityWithChildren(entity: Entity): void
 

packages/@dcl/ecs/src/engine/types.ts

@@ -43,8 +43,7 @@ export function createAudioEventsSystem(engine: IEngine): AudioEventsSystem {
   function registerAudioEventsEntity(entity: Entity, callback: AudioEventsSystemCallback) {
     // audio event component is not added here because the renderer adds it
     // to every entity with an AudioSource or AudioStream component
-    const existing = entitiesCallbackAudioStateMap.get(entity)
-    entitiesCallbackAudioStateMap.set(entity, { callback: callback, lastAudioState: existing?.lastAudioState })
+    entitiesCallbackAudioStateMap.set(entity, { callback: callback })
   }
 
   function removeAudioEventsEntity(entity: Entity) {