feat: consolidate player tracking in @dcl/sdk/players

[LautaroPetaccio]

Scope changed during review. This PR originally added a separate session-tracking API under @dcl/sdk/server. Review showed almost nothing in it was actually server-specific, and that it would have been a third per-frame scan of PlayerIdentityData on the same engine. It now reworks the existing @dcl/sdk/players helper instead. @dcl/sdk/server is untouched — git diff origin/auth-server -- packages/@dcl/sdk/src/server/ is empty.

The problem

onEnterScene gates on PlayerIdentityData and AvatarBase (players/index.ts:36). The avatar profile can replicate seconds after the identity, or never for a given peer — so a player can be present, addressable and interacting while onEnterScene has never fired for them.

That is unusable for anything that needs to know a player exists: presence counts, per-player storage, authorization. So every scene running an authoritative server hand-rolled the same tracker.

Scene	Hand-rolled today
flagtag	src/server/playerTracking.ts:38 diffs PlayerIdentityData into a module-level currentlyConnected Set (:58, :119), plus nameResolverServerSystem (:191) polling AvatarBase every 3s (:183)
towerofmadness	src/server/server.ts:87-104 player-name-system, a 2s AvatarBase poll; getPlayerName() (:164) rescans every identity+profile pair per lookup; arrival is a client-announced playerJoin message (:174)
dead-surge	src/server/lobbyServer.ts:1454 diff loop with its own DISCONNECTED_PLAYER_GRACE_MS (:93) reconciler (:1481, :1504)
cozy-farm	src/server/farmServer.ts:27 getDisplayName() rescans all entities per call; cleanupDisconnectedPlayers() (:66) rebuilds the set to detect leaves
My-Dear-Pet	src/server/server.ts:14 connected Set, with first-arrival detection folded into a message handler (:52-55) because there is no hook

Across those five that is 42 getEntitiesWith(PlayerIdentityData…) call sites and 384 .toLowerCase() calls, most of them normalizing an address the SDK could have handed over usable.

The change

One options object, one diff pass, both thresholds:

onEnterScene(cb)                             // unchanged: identity + profile, name populated
onEnterScene(cb, { requireProfile: false })  // as soon as the identity exists

requireProfile defaults to true, so existing scenes are unaffected. onLeaveScene takes the same option and mirrors whichever threshold you chose.

Also new:

• onPlayerNameChanged(cb) — fires when the profile name resolves or changes. Replaces the 2s/3s polling loops above.
• getPlayers() / getPlayerCount() — the accessor that replaces the hand-rolled scan, with duplicate addresses already collapsed. Same element shape as getPlayer (both go through one private builder, so they cannot drift).
• displayName on the player payload — never empty: the resolved profile name, else a shortened address. name keeps its exact meaning (raw profile, '' until replicated) so existing readers are unaffected. nameResolved distinguishes them and is sticky, so a profile that momentarily reports empty does not downgrade a known name.
• joinedAtMs, and a second argument on onLeaveScene carrying the last known state — needed because getPlayer() legitimately returns null once the entity is gone. Adding a callback parameter is source-compatible, so existing one-argument handlers still typecheck.
• Unsubscribers from every subscription.
• async handlers are failure-isolated. TypeScript's void-return rule means an async callback satisfies a => void parameter, so a rejection after an await would otherwise escape as an unhandled rejection. Every delivery path — arrival, departure, replay, name change — now catches synchronous throws and attaches a .catch to a returned thenable (duck-typed, so transpiled/cross-realm promises count). The callback types stay => void deliberately: widening to void | Promise<void> would break ordinary handlers, because the void-return special case does not apply to a union, so onEnterScene((p) => arr.push(p)) would stop compiling.

API reference

All exported from @dcl/sdk/players, both as loose named exports and on the default-export object. Every subscription returns an unsubscribe function.

Events

Function	Arguments	Fires when	Notes
onEnterScene	cb: (player: GetPlayerDataRes) => void options?: PlayerEventOptions	Default: identity and an AvatarBase component are present. { requireProfile: false }: as soon as identity exists	Once per player, per subscription. The default threshold is the historical behaviour, so existing scenes are unaffected. avatar is set on the default threshold, but name may still be '' or an address echo — render displayName
onLeaveScene	cb: (userId: string, lastKnown: PlayerSnapshot) => void options?: PlayerEventOptions	Mirrors the threshold you chose. Default: the player left or their AvatarBase went away. { requireProfile: false }: only when identity is gone	userId is the platform-reported casing, unchanged from before. Do not rely on getPlayer() here: on the identity threshold the entity is gone and it returns null, but on the default threshold it can still return a live player, because only the profile went away. lastKnown is the only reading correct in both cases. The second parameter is additive, so existing one-argument handlers still typecheck
onPlayerNameChanged	cb: (player: GetPlayerDataRes) => void	A real profile name first resolves, or later changes	Replaces polling AvatarBase on a timer. Not fired for a name already resolved when the player was first seen. It can fire on the same tick as a default-threshold onEnterScene, since a late-arriving profile both resolves the name and satisfies that threshold — subscribe to one or the other, not both, if you want a single notification

Accessors

Function	Arguments	Returns	Notes
getPlayer	user?: GetPlayerDataReq ({ userId }; omit for the local player)	GetPlayerDataRes | null	Matching is case-insensitive, and O(1) for tracked players via an address index — the index is re-validated against the live component on every call, so a reassigned identity cannot answer a lookup with the wrong player. Returns null for an entity with avatar/wearable data but no identity — presence means identity is available
getPlayers	—	GetPlayerDataRes[]	Same element shape as getPlayer (one shared builder, so they cannot drift), with duplicate addresses already collapsed. On a client this includes the local player; filter with p.entity !== engine.PlayerEntity when you mean everyone else
getPlayerCount	—	number	Deliberately not getPlayers().length — reads the tracked size without building a payload per player, so it stays cheap for per-frame gates. Consequently it reflects the last settled tick while getPlayers validates live components, so for one frame after an entity disappears it can read one higher; use getPlayers().length when the two must agree. Counts the local player on a client, same as getPlayers
definePlayerHelper	engine: IEngine	IPlayersHelper	Memoized per engine, so repeated calls share one tracking system. Pre-existing export, unchanged signature

PlayerEventOptions

Option	Default	Meaning
requireProfile	true	true waits for the AvatarBase component. false fires on identity alone — earlier, and also for peers whose profile replicates late or never. The identity threshold is what an authoritative server wants: it is the point at which the address becomes usable as a key
replayPresent	true	Replay the callback for players already satisfying the threshold at subscription time. Defaults to on because that is the historical behaviour: before the tracker was shared per engine, each definePlayerHelper call built a cold tracker whose first tick announced everyone already present. The replay is unconditional — subscribing from inside another handler still replays everyone present at that moment, including players being announced on that very tick, and the per-tick subscriber snapshot keeps it exactly-once. Set false for arrivals-from-now-on only

GetPlayerDataRes

Field	Type	Notes
userId	string	Address in the platform-reported casing. All lookups accept any casing
entity	Entity	The avatar entity currently backing this player. Do not cache it: tracking is keyed on the address, so if one entity for an address disappears and another appears in the same tick the player never left, no event fires, and this points somewhere new. Re-read via getPlayer(userId) — anything attached to a stale entity is attached to nothing
name	string	Raw profile name — '' until the profile replicates. Meaning unchanged from before this PR
displayName	string	New. Never empty: resolved profile name, else shortened address. Render this
nameResolved	boolean	New. Whether a real name has ever been seen. Sticky — a profile that later reports empty does not clear it
joinedAtMs	number	New. Date.now() of the tick the player was first seen; 0 if not tracked yet
isGuest	boolean	Refreshed every tick (previously captured once and never updated)
avatar	PBAvatarBase | undefined	Raw live component
wearables / emotes	string[]	Raw live components
position	Vector3 | undefined	Live Transform position

PlayerSnapshot (the onLeaveScene second argument) carries userId, name, displayName, nameResolved, isGuest, joinedAtMs — the tracked values, readable after the entity is gone. name is the last raw value seen, identical to what GetPlayerDataRes.name would have reported, so the two shapes cannot drift.

Cross-cutting guarantees

Guarantee	Detail
Failure isolation	A handler that throws — or an async handler whose promise rejects — is logged and contained. It never skips sibling handlers and never kills the tracker. Applies to arrival, departure, replay and name-change delivery
Callback types stay => void	Deliberate. TypeScript's void-return rule already admits async handlers; widening to void | Promise<void> would break ordinary handlers like onEnterScene((p) => arr.push(p)), because that rule does not apply to a union
Consistent reads	All state settles before any callback runs, so every handler observes the same set even when one player joins and another leaves on the same tick
Iteration safety	Component rows and callback lists are iterated over snapshots, so a handler may add/remove subscriptions or entities without corrupting the pass
One system per engine	A single engine.addSystem named @dcl/sdk/players, registered eagerly at construction

Before / after

flagtag's tracker (~140 lines, condensed) plus a second system polling for names:

export function playerTrackingSystem(): void {
  const nowConnected = new Set<string>()
  for (const [, identity] of engine.getEntitiesWith(PlayerIdentityData)) {
    nowConnected.add(identity.address.toLowerCase())
  }
  for (const userKey of nowConnected) {
    if (!currentlyConnected.has(userKey)) { currentlyConnected.add(userKey); /* …40 lines… */ }
  }
  for (const userKey of currentlyConnected) {
    if (!nowConnected.has(userKey)) { currentlyConnected.delete(userKey); /* …30 lines… */ }
  }
}
// + nameResolverServerSystem: a second system polling AvatarBase every 3s

becomes:

onEnterScene((player) => { /* arrival work */ }, { requireProfile: false })
onLeaveScene((userId, lastKnown) => { /* departure work, name still readable */ }, { requireProfile: false })
onPlayerNameChanged((player) => { /* replaces the 3s poll */ })

Bugs fixed along the way

All of these affected client scenes too, independent of the server use case:

• players.length === playerEntities.size early return (:37) compared a count of entities-with-both-components against a map cleaned elsewhere (only by the onChange hook). The two can coincide while the sets differ, silently dropping a join.
• Leaves ran off AvatarBase.onChange (:49-54), so a peer that lost identity without a profile change never fired onLeaveScene. And onChange is push-only with no removal path (lww-element-set-component-definition.ts:419-423), so those per-entity callbacks accumulated for the life of the process — a slow leak on a long-lived server. Leaves are now detected by absence and the hook is gone.
• An identity with an empty address crashed the handler. Old code called getPlayer({userId: ''}), which fell back to the local player entity and returned null, then dereferenced it with !. Such rows are now skipped.
• getPlayer pinned to the first entity by insertion order for a duplicated address, and onEnterScene double-fired for it — both entities were tracked separately because the key was the entity, not the address.
• getPlayer compared addresses case-sensitively (:73), so passing a lowercased address silently returned null — a real contributor to all that .toLowerCase() at call sites. Matching is now case-insensitive.
• No unsubscribe and no error isolation — the forEach had no try/catch, so one throwing handler killed the tracker for the rest of the run.
• Iteration safety and ordering — component rows and callback lists are iterated over snapshots, and all state settles before any callback runs, so handlers see a consistent set even when someone joins and someone else leaves on the same tick.

Fixing the state-sync bootstrap while we are here

message-bus-sync.ts:186 subscribes to onEnterScene and calls requestState() — the CRDT state bootstrap — when it sees the local player. Memoizing the helper made that subscription order-dependent (the old per-caller tracker started empty, so it always rediscovered present players), and reviewing it turned up two pre-existing problems in the same three lines:

• It waited for requireProfile: true, delaying the bootstrap until the avatar profile replicated — seconds later, or never — when only the local identity is needed.
• It compared myProfile.userId === player.userId case-sensitively. Those come from different sources (getUserData vs CRDT PlayerIdentityData.address), so any casing difference means the bootstrap never fires.

Now { requireProfile: false, replayPresent: true } with a case-insensitive compare, and the subscription is deferred until the local profile resolves. That last part matters: firing at the identity threshold is strictly earlier than before, so comparing against myProfile.userId — populated asynchronously by fetchProfile — would have lost that race more often, not less. fetchProfile now returns its promise (which also fixes an unhandled rejection when profile data is missing), and replayPresent is what makes the late subscription safe. requestState() is guarded by requestingState (:235), so the replay cannot double-request.

One tracker instead of two

definePlayerHelper(engine) was being called twice with the global engine — players/index.ts:103 and message-bus-sync.ts:66 (both live, used at :186 and :256) — so two systems scanned the same component every frame. It is now memoized per engine, so there is one. The originally-proposed server module would have made three.

Cost

The rewrite moves SCENE_COMPILED_JS_SIZE_PROD by roughly +2.6KB on every scene bundle (~1% of a 250KB scene), because observables.ts:14 pulls this module into all of them — including scenes that never touch player events. That is the deliberate tradeoff of putting it in players/ (always loaded) rather than server/ (opt-in). The 12 snapshot goldens are regenerated accordingly, and grep -rn "ERR!" test/snapshots/ is clean per AGENTS.md.

Compatibility

• onEnterScene/onLeaveScene keep their default thresholds and payload types; onLeaveScene still receives the address in the platform-reported casing (lowercasing is used only for internal map keys).
• Return values changed from void to an unsubscribe function, and onLeaveScene gained a second parameter — both additive.
• GetPlayerDataRes gained displayName, nameResolved, joinedAtMs; existing fields unchanged.
• GetPlayerDataRes was not exported before this PR (it was a bare type), so it is newly nameable. The three added fields are required, which means code that constructs the shape structurally — typically a hand-built fake player in a test — needs the new fields. No runtime impact.
• Deliberate narrowing: getPlayer now returns null for an entity that carries avatar or wearable data but no PlayerIdentityData. Previously it returned a partial object with userId: '', which contradicts both "presence means identity is available" and the never-empty displayName. This also covers getPlayer() for the local player in the window where a host writes AvatarBase before PlayerIdentityData — verified near-theoretical, since unity-explorer and godot-explorer both write identity and avatar base together onto the player entity.
• The implicit replay is preserved. Memoizing definePlayerHelper would otherwise have removed it: a subscription registered after the shared tracker has run would see only future arrivals, where previously each call built a cold tracker that announced everyone already present. replayPresent therefore defaults to true, which reproduces the old behaviour; pass { replayPresent: false } for arrivals-from-now-on only.
• A subscription made from inside a handler takes effect from the next tick. The subscriber list is snapshotted once per pass, so it cannot receive the remainder of the tick it was created during (and unsubscribing a sibling mid-pass does not retroactively skip it).
• A same-tick reconnect now emits nothing (previously leave + enter). Less churn, and the tracker follows the new entity — but a payload captured at arrival keeps the dead one, so re-read via getPlayer(userId) rather than caching player.entity. Documented on GetPlayerDataRes.entity and on onEnterScene, and pinned by a test asserting no events fire while getPlayer returns the replacement.
• getPlayerCount() and getPlayers() can disagree for one frame when a backing entity disappears between ticks: the count reflects the last settled tick, getPlayers() reflects live data.
• Not purely additive at the type level. Three shapes now fail to compile: hand-building a GetPlayerDataRes (three new required fields); implementing or mocking the helper type (onEnterScene now returns an unsubscribe function, not void); and export * from a barrel that already exports a getPlayers. Calling code — including one-argument onLeaveScene handlers, async handlers and loose structural annotations — is unaffected.
• observables.ts is untouched, and onPlayerConnectedObservable / onPlayerDisconnectedObservable now benefit from the fixed leave detection.

Testing

test/sdk/players.spec.ts, 99 cases driving a real engine: both thresholds and that a single subscription fires exactly once at each, late/empty/address-shaped profile names, profile-lost-but-identity-kept vs identity-lost, same-tick join+leave with a consistency assertion inside the handler, duplicate address where only the newer entity has a profile, isGuest refresh, unsubscribe, throwing handlers, case-insensitive lookup, replayPresent, per-engine memoization (asserted via removeSystem returning true then false, so it pins one named system rather than just one object), rejected async handlers on every delivery path, the identity requirement, incumbency when a second entity claims a tracked address, displayName markup stripping, and the full PlayerSnapshot shape.

The suite was audited by mutation testing (42 mutants, each reintroducing one claimed fix): the gaps it found — getPlayers having no coverage at all, unsubscribe untested on the leave and name-change paths, joinedAtMs/snapshot fields unasserted, and one test that passed for the wrong reason via the local-player fallback — are all closed.

• make test — full suite
• make lint — @dcl/sdk clean
• tsc --noEmit on @dcl/sdk — clean
• make update-snapshots then grep -rn "ERR!" test/snapshots/ — no output

Deliberately not included

• PlayerIdentityData is not server-authoritative. There is no component allow-list on the inbound CRDT path and no validateBeforeChange for it (network/server/index.ts:108-141), so a crafted frame can inject phantom players — inflating counts, faking arrival/departure pairs, and shadowing a real player's name. This is pre-existing and affects the hand-rolled trackers in the scenes above identically; consolidating here at least reduces it to one place to guard. The likely fix is filtering on the reserved entity range (RESERVED_STATIC_ENTITIES = 512), but that is only correct if the headless server really receives avatars in that band, which needs verifying against a live server. Filed separately rather than guessed at here.
• IntelliSense on the loose named exports. The JSDoc lives on IPlayersHelper and survives into the emitted .d.ts, so players.onEnterScene(...) is fully documented — but the destructured export { onEnterScene, … } line emits as a bare declare const with no prose, so import { onEnterScene } shows signatures only. That is pre-existing for getPlayer/onEnterScene/onLeaveScene and unchanged here; fixing it means either explicit wrapper functions or steering creators to the object import, which is a module-wide decision.
• getPlayers() includes the local player on a client (documented in its JSDoc, with the filtering idiom). Whether that deserves an excludeSelf option is left open rather than adding a flag pre-emptively.

[cloudflare-workers-and-pages]

Deploying js-sdk-toolchain with    Cloudflare Pages

Latest commit:	0962c57
Status:	✅  Deploy successful!
Preview URL:	https://4bffb8ac.js-sdk-toolchain.pages.dev
Branch Preview URL:	https://feat-server-session.js-sdk-toolchain.pages.dev

View logs

[github-actions]

Test this pull request

• The @dcl/sdk package can be tested in scenes by running

  npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/server-session/dcl-sdk-7.25.1-30669220061.commit-6d5767d.tgz"

• The @dcl/js-runtime package can be tested in scenes by running

  npm install "https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/server-session/@dcl/js-runtime/dcl-js-runtime-7.25.1-30669220061.commit-6d5767d.tgz"

• To test with npx init

  export SDK_COMMANDS="https://sdk-team-cdn.decentraland.org/@dcl/js-sdk-toolchain/branch/feat/server-session/dcl-sdk-commands-7.25.1-30669220061.commit-6d5767d.tgz"
  npx $SDK_COMMANDS init

• The /changerealm command to test test in-world

  /changerealm https://sdk-team-cdn.decentraland.org/ipfs/feat/server-session-e2e
  

• You can preview this build entering:
  https://playground.decentraland.org/?sdk-branch=feat/server-session

[decentraland-bot]

I found one P1 issue that should be fixed before merging.

Findings

• P1 — Async session handlers are not failure-isolated even though the public example encourages them. emit() in packages/@dcl/sdk/src/server/session.ts:64-70 only catches synchronous throws. The documented onPlayerConnected(async (player) => { ... }) example is valid TypeScript for a void callback, but if it rejects after an await, the rejection bypasses this guard and can become an unhandled rejection instead of being logged/isolated. The replay path at session.ts:189-194 has the same problem. Please type handlers as void | Promise<void> (or explicitly disallow async handlers in docs/types) and attach a .catch()/Promise.resolve(...).catch(...) around every callback invocation, with tests for async rejection on normal emits and replay.

Additional notes

• P2 — Display-name heuristic is broader than the comment. isRealName() rejects every name beginning with 0x, not only names that equal the player's wallet address. If 0x... display names are allowed, pass the address into the helper and compare equality instead.
• Public API impact: this is additive server-session API surface, so I did not find a backward-incompatible consumer impact.
• Security review: no hardcoded secrets or injection issues found in the changed files.
• CI: CLI E2E is failing; build, docs, lint, and tests pass.

---

Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack

[gonpombo8]

Hey laucha, we already have something similar in the @dcl/sdk/players entrypoint.
It already resolves the name also, so i dont think we need this session layer 🤔

import { getPlayer, onEnterScene, onLeaveScene } from '@dcl/sdk/players'

[decentraland-bot]

Thanks for the rewrite. The previous server-session findings are mostly addressed by moving the surface into @dcl/sdk/players, but I found one remaining blocker in the new API.

Findings

• P1 — Async player callbacks are still not failure-isolated. emit() only catches synchronous throws around invoke(sub.cb), and the replayPresent path does the same around cb(data). The public JSDoc example now explicitly shows onEnterScene(async (player) => { ... }), which TypeScript accepts for a void callback; if that promise rejects after an await, the rejection bypasses this guard and becomes an unhandled rejection instead of being logged/isolated. The same applies to onPlayerNameChanged. Please either explicitly disallow async handlers in the public types/docs, or make the callback type void | Promise<void> and wrap every callback invocation with promise rejection handling, with tests for rejected async handlers on normal emit, replay, and name-change delivery. (packages/@dcl/sdk/src/players/index.ts:205, packages/@dcl/sdk/src/players/index.ts:449)

• P2 — buildPlayerData() can violate the new identity/display-name contract. The helper returns a player object when AvatarBase or wearables exist without PlayerIdentityData, producing userId: '' and displayName: ''. That contradicts the new docs that presence means identity is available and displayName is never empty. Consider returning null unless playerData?.address exists, and treating avatar/wearables as optional enrichment only. (packages/@dcl/sdk/src/players/index.ts:265-284)

Additional notes

• Public API impact: the runtime API additions are backward-compatible for normal callers. The exported GetPlayerDataRes shape gained required fields, which may be TypeScript-breaking only for consumers that construct that type manually; I did not find direct external SDK consumers doing that in a quick org search.
• Security review: no hardcoded secrets, injection sinks, dependency changes, or new auth/authz issues found in the changed files.
• CI: all reported checks are passing.

---

Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack

[decentraland-bot]

Thanks for the updates — the previous blockers are fixed. Async callback rejections are now isolated across enter/leave/replay/name-change paths, and buildPlayerData() now requires identity before returning player data.

Findings

• P1 — Memoizing the shared player helper can make network state sync miss the local-player enter event. This PR changes definePlayerHelper(engine) from “new tracker per caller” to “one tracker per engine”. That is good for avoiding duplicate scans, but it changes the behavior of the existing network subscriber in packages/@dcl/sdk/src/network/message-bus-sync.ts:186: if the shared tracker has already processed the local PlayerIdentityData before addSyncTransport() registers its players.onEnterScene(...) handler, there is no fresh helper state to rediscover that player, and the handler does not use replayPresent. It also keeps the default requireProfile: true, even though requestState() only needs the local identity. Please update that subscription to preserve the old “catch already-present player” behavior, e.g. players.onEnterScene(cb, { requireProfile: false, replayPresent: true }) (and compare myProfile.userId case-insensitively if casing can differ).

Additional notes

• P2 — Default requireProfile docs still overpromise populated names. The implementation treats any AvatarBase component as profile-ready, so default onEnterScene can still receive name === '' or an address-echo name. If that is the intended compatibility behavior, please adjust the JSDoc from “so name and avatar are populated” to make clear that avatar is present but name may still be unresolved and displayName/nameResolved should be used for rendering.
• Public API impact: runtime changes are backward-compatible/additive for normal callers; the only compatibility caveat is TypeScript users constructing GetPlayerDataRes mocks manually because the exported type gained required fields.
• Security review: no hardcoded secrets, injection sinks, dependency changes, or new auth/authz issues found.
• CI: all reported checks are passing.

---

Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack

[decentraland-bot]

Thanks for the updates — the previous blockers are fixed. I did another pass over the player tracker, the message-bus sync bootstrap, tests, and snapshots, and I don't see any remaining P0/P1 issues blocking merge.

Findings

• P2 — Validate the cached entity address before returning it from getPlayer. findEntity() trusts the cached trackedEntity as long as it still has PlayerIdentityData, but it does not verify that the live component address still matches the requested key. If the identity component on that entity changes before the tracker system settles the next tick, getPlayer({ userId: oldAddress }) can return the new user's payload. Please consider checking PlayerIdentityData.getOrNull(trackedEntity)?.address?.toLowerCase() === key before returning the cached entity, otherwise fall through to the live scan. (packages/@dcl/sdk/src/players/index.ts:331-335)

• P2 — Same-tick entity replacement is now address-scoped, so consumers that keep player.entity should re-read. The PR documents this tradeoff, and I don't think it should block, but it is worth being explicit in the API docs/tests: when a same-address entity disappears and another appears in the same tick, enter/leave may not fire even though the entity backing the player changed. Consumers that attach resources to the event payload's entity need to call getPlayer() again rather than caching it indefinitely. (packages/@dcl/sdk/src/players/index.ts:485-491)

Additional notes

• Public API impact: runtime changes are backward-compatible/additive for normal callers. I checked the likely public surfaces (onEnterScene, definePlayerHelper, GetPlayerDataRes) and did not find direct external consumers constructing the newly exported GetPlayerDataRes type from @dcl/sdk/players.
• Security review: no hardcoded secrets, injection sinks, dependency changes, or new auth/authz issues found.
• CI: all reported checks are passing.

---

Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack

[decentraland-bot]

Re-reviewed after the latest push. All previous P1 findings from earlier rounds are resolved — async failure isolation via runIsolated, identity-gated buildPlayerData, the deferred message-bus-sync subscription with { requireProfile: false, replayPresent: true }, and cached entity address validation in findEntity. Ran parallel sub-agent analysis (security, architecture, code quality) alongside a manual pass.

Findings

No P0 or P1 issues found.

• P2 — name semantic drift between live payload and snapshot. buildPlayerData() returns the raw avatar name (avatarData?.name ?? ''), while the TrackedPlayer.name stored by the tracker goes through resolveName() which trims whitespace. snapshotOf() reads from the tracker, so a departure snapshot's name can differ from what getPlayer() returned moments earlier for a name with leading/trailing whitespace. Practically harmless (profile names with meaningful whitespace are near-nonexistent), but a latent inconsistency. (packages/@dcl/sdk/src/players/index.ts:500-508 vs :467)

• P2 — getPlayerCount() / getPlayers() one-frame disagreement. getPlayerCount() reads tracked.size (last settled tick), while getPlayers() filters through buildPlayerData() which validates live components. An entity removed between ticks produces getPlayerCount() > getPlayers().length for one frame. Documented in the PR and JSDoc — known trade-off, not a bug.

• P2 — Late subscriber from inside a handler permanently misses current-tick players. If a handler calls onEnterScene(cb) during delivery, deliveringKeys blocks the replay for those players, and they are already tracked so the system will never re-announce them. Documented behavior ("takes effect from the next tick"), but could surprise someone who expects replayPresent: true to unconditionally replay. A note in the JSDoc for replayPresent would help.

Additional notes

• Public API impact: runtime changes are backward-compatible/additive for normal callers. Breaking only at the type level for code that constructs GetPlayerDataRes structurally (test mocks), implements IPlayersHelper, or re-exports getPlayers from a barrel. All documented.
• Security review: no hardcoded secrets, injection sinks, or dependency changes. sanitizeForDisplay strips angle brackets and caps length — appropriate for TextMeshPro rendering. PlayerIdentityData trust boundary is a known pre-existing limitation, not a regression.
• Test suite: 99 cases (984 lines), mutation-tested. Thorough coverage of both thresholds, error isolation (sync + async), case-insensitive lookup, replay, incumbency, entity replacement, and memoization.
• CI: lint passing; build, docs, test still in progress.

---

Reviewed by Jarvis 🤖 · Requested by Lautaro Petaccio (<@U025WCHLMN3>) via Slack