feat: audioEventSystem + avatarEmoteCommand.state - #1530
Conversation
Test this pull request
|
decentraland-bot
left a comment
decentraland-bot
left a comment
There was a problem hiding this comment.
Review: feat: audioEventSystem + avatarEmoteCommand.state
Summary
Clean, well-structured PR that adds a new AudioEventsSystem helper (mirroring the existing VideoEventsSystem) and integrates the new EmoteState field on AvatarEmoteCommand via a protocol bump. The audio system correctly adapts the video pattern to support both AudioSource and AudioStream components, including the edge case where only one of the two is removed. Tests are thorough and actually exceed videoEventsSystem.spec.ts coverage. All changes are purely additive — no breaking API changes.
CI: All checks passing ✅
ADR-6: PR title (feat: ...) and branch name (feat/...) comply ✅
Security: No issues found — no secrets, no user input handling, no network calls ✅
Consumer impact: Purely additive exports (AudioEventsSystem, audioEventsSystem, EmoteState). PBAvatarEmoteCommand.state is optional. No breaking changes. ✅
Findings
[P2] System function name "EventSystem" collision — latent footgun
File: packages/@dcl/ecs/src/systems/audioEvents.ts:58
The system is registered as function EventSystem(), which is the same name used by videoEvents.ts, events.ts, raycast.ts, and assetLoad.ts. While engine.addSystem() deduplicates by function reference (not name), so both systems run correctly, the engine.removeSystem(name) API uses findIndex by name — meaning a future engine.removeSystem('EventSystem') would silently remove only the first match. Renaming to function AudioEventSystem() (and ideally VideoEventSystem in its sibling) would cost nothing and eliminate the ambiguity.
This is a pre-existing pattern in the codebase, so not blocking.
[P2] registerAudioEventsEntity silently resets state tracking on re-registration
File: packages/@dcl/ecs/src/systems/audioEvents.ts:43-45
Calling registerAudioEventsEntity on an already-registered entity overwrites the map entry, resetting lastAudioState to undefined. This causes the callback to re-fire on the next tick with the current state even if it hasn't changed. This matches videoEvents.ts exactly (same behavior), but it's worth documenting — especially since it's now duplicated a second time. No test covers this scenario in either the audio or video test suite.
[P2] Code duplication between audio and video event systems — refactor opportunity
Files: audioEvents.ts and videoEvents.ts
The two files are ~95% structurally identical: map-based registration, per-tick state diffing, removal on entity-removed/component-missing, identical getState pattern. A generic createMediaEventsSystem<T>(engine, { hasComponent, eventComponent }) factory could back both, cutting ~180 duplicated lines. This PR was the natural moment for that refactor since it's the second copy of the pattern. Not blocking, but worth considering before a potential third "XEventsSystem" appears.
[P2] Return object methods are unnecessary wrappers
File: packages/@dcl/ecs/src/systems/audioEvents.ts:84-95
Every method in the returned object is a one-line delegation to the identically-named closure function (e.g., removeAudioEventsEntity(entity) { removeAudioEventsEntity(entity) }). Could return { removeAudioEventsEntity, registerAudioEventsEntity, hasAudioEventsEntity, getAudioState } directly. Same pattern exists in videoEvents.ts — pre-existing, not blocking.
[P2] Test assertions check call counts but not call arguments
File: test/ecs/events/audioEventsSystem.spec.ts
Callback tests use toHaveBeenCalled() / toHaveBeenCalledTimes(n) without verifying the payload via toHaveBeenCalledWith(...). A callback invoked with a wrong or stale state value would still pass. Strengthening at least the core "run callback on audio status change" test with expect(fn).toHaveBeenCalledWith(expect.objectContaining({ state: MediaState.MS_PLAYING })) would catch payload regressions.
[P2] Missing test edge cases
File: test/ecs/events/audioEventsSystem.spec.ts
A few untested but safe-by-inspection paths:
getAudioStateon an entity withAudioSourcebut noAudioEventdata yet → should returnundefined- Registering a callback before any
AudioEventvalue exists (realistic renderer flow) - Multiple rapid
addValuecalls within a single tick → only the final state fires the callback (intermediate states are silently dropped — worth documenting this behavior) - Re-registering the same entity with a different callback (resets
lastAudioState, item above) - State cycling:
LOADING → PLAYING → LOADINGto confirm dedupe is keyed on state value, not object identity
Verdict: ✅ Approved
No P0 or P1 issues. All findings are P2 suggestions — mostly inherited from the existing VideoEventsSystem pattern that this PR mirrors faithfully. The implementation is correct, the adaptation for dual audio components (AudioSource + AudioStream) is well-handled, and test coverage is solid. The EmoteState / AvatarEmoteCommand changes are clean schema additions with proper serialization round-trip tests.
Reviewed by Jarvis 🤖 · Requested by Gabriel Díaz (<@U03MGHMAJL8>) via Slack
… into feat/playback-completion-signals-main
Deploying js-sdk-toolchain with
|
| Latest commit: |
9969494
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fef0d208.js-sdk-toolchain.pages.dev |
| Branch Preview URL: | https://feat-playback-completion-sig-1ztg.js-sdk-toolchain.pages.dev |
… into feat/playback-completion-signals-main
Resolves the conflicts main brought in with #1530 (audioEventSystem, avatarEmoteCommand.state) and #1532 (jest 30, node >=24.16, dependency slimming). Every conflict sat in a generated artifact, so main's side was taken and the artifacts were regenerated instead of merged by hand. @dcl/protocol moves off the PR tarball onto the published 1.0.0-31617402096.commit-86c4613, now that decentraland/protocol#455 is merged. The development-dependency gate therefore no longer applies, and the AvatarEmoteMask entry the experimental-based tarball dragged into playground-assets.api.md is gone. Lockfiles are regenerated with the Node version CI pins (24.16.0), which drops the "libc" fields an older npm had written. Co-Authored-By: Claude <noreply@anthropic.com>
3 participants
EmoteStateto be able to read the playback state of an emote, in the scene.Related PRs: