feat: vindral composer device#464
Open
Julusian wants to merge 23 commits into
Open
Conversation
WalkthroughThis PR introduces a complete Vindral Composer device integration into ChangesVindral Composer Integration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 6
🧹 Nitpick comments (4)
packages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/mediaPlayers.spec.ts (1)
51-75: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert the default
playing: truebranch whenplayingis omitted.This test validates selector/source URL but does not lock in the default-playing behavior from
buildVindralState.Suggested assertion update
expect(result.mediaPlayers['mpLayer']).toMatchObject({ selector: { target: 'player-guid', targetName: 'ClipPlayer1' }, autoPlayOnMediaChange: true, sourceUrl: 'http://cdn.example.com/other.mp4', + playing: true, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/mediaPlayers.spec.ts` around lines 51 - 75, The test for media player mapping does not assert the default playing behavior when the playing property is omitted from the input. In the expect statement where result.mediaPlayers is checked with toMatchObject, add an assertion for playing: true to the expected object to verify that the default playing state is correctly applied by buildVindralState when no explicit playing value is provided.packages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/switchers.spec.ts (1)
53-79: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression test for
transition: nullclear behavior.The suite currently validates
transition: undefinedbut not the explicit clear path (null) that the builder handles specially.Suggested test addition
describe('stateBuilder — switchers', () => { + test('switcher mapping — explicit null transition clears transition', () => { + const result = buildVindralState( + { + time: 0, + objects: [ + makeDeviceTimelineStateObject({ + enable: { start: 0 }, + id: 'obj0', + layer: 'swLayer', + content: { + deviceType: DeviceType.VINDRAL_COMPOSER, + type: TimelineContentTypeVindralComposer.SWITCHER, + switcher: { transition: null }, + }, + }), + ], + }, + MAPPINGS + ) + expect(result.switchers['abc-123']).toMatchObject({ transition: null }) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/switchers.spec.ts` around lines 53 - 79, Add a regression test case to validate the explicit null clear path for the transition property in the switcher mapping. After the existing test "switcher mapping — partial fields", create a new test that passes transition: null in the switcher content and verify that the builder correctly handles this explicit null value the same way it handles undefined. Include an assertion in the buildVindralState result that confirms the transition property is handled appropriately when explicitly set to null.packages/timeline-state-resolver/src/__mocks__/vindral-composer-connection.ts (1)
6-6: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winKeep
removeAllListenersbehavior aligned withEventEmitterin the mock.Line 6 currently makes
removeAllListenersa no-op, which can hide listener-lifecycle bugs in tests. Preserve base behavior and still track invocations.Suggested fix
- removeAllListeners = jest.fn() + removeAllListeners = jest.fn((event?: string | symbol) => { + super.removeAllListeners(event) + return this + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/timeline-state-resolver/src/__mocks__/vindral-composer-connection.ts` at line 6, The `removeAllListeners` property in the mock is currently only a no-op jest.fn() without actual implementation, which can mask listener-related bugs in tests. Modify the `removeAllListeners` mock to preserve the actual EventEmitter behavior by using jest.fn().mockImplementation() to call the parent class's removeAllListeners method while still tracking invocations for test assertions. This ensures listeners are actually removed during tests while maintaining the ability to spy on and verify the method was called.packages/timeline-state-resolver/src/integrations/vindral-composer/diffState.ts (1)
175-283: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftSplit
diffMediaPlayersinto focused helpers before this grows further.Line 175 currently combines script-engine flow, direct HTTP flow, and ordering rules in one high-complexity block. This is already tripping static complexity gates and makes sequencing regressions harder to catch.
♻️ Refactor outline
function diffMediaPlayers( oldState: VindralComposerDeviceState, newState: VindralComposerDeviceState, useScriptEngine: boolean ): VindralCommandWithContext[] { const commands: VindralCommandWithContext[] = [] for (const key of allKeys(oldState.mediaPlayers, newState.mediaPlayers)) { const old = oldState.mediaPlayers[key] const next = newState.mediaPlayers[key] if (!next) continue - - // script + direct flow logic interleaved here + commands.push( + ...(useScriptEngine + ? diffMediaPlayerScriptEngine(old, next, newState.stateTime, key) + : diffMediaPlayerDirect(old, next, key)) + ) } return commands }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/timeline-state-resolver/src/integrations/vindral-composer/diffState.ts` around lines 175 - 283, The diffMediaPlayers function combines three distinct concerns: script-engine flow logic, direct HTTP flow logic, and play/pause command handling in a single 100+ line block. Extract the script-engine flow handling (the logic inside the useScriptEngine conditional starting around the scriptInTime variable) into a separate helper function that takes the old and new media player states and returns an array of commands. Similarly, extract the direct HTTP flow handling (the logic after the continue statement, handling endBehaviour, autoPlayOnMediaChange, sourceUrl changes, and play/pause) into its own helper function. Then refactor the main diffMediaPlayers loop to call these focused helpers, passing the necessary parameters and collecting their returned commands. This separation will reduce the cyclomatic complexity and make sequencing logic easier to verify and maintain.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/timeline-state-resolver-types/src/integrations/vindral-composer/timeline.ts`:
- Around line 30-43: The connector property in the
TimelineContentVindralComposerConnector interface currently allows both name and
value to be optional, which violates the documented mutual exclusivity
constraint and allows invalid states where both fields or neither field are
present. Replace the optional name and value fields with a union type that
enforces compile-time exclusivity, such as a discriminated union pattern where
one variant contains only name with value excluded, and another variant contains
only value with name excluded. This ensures only one of the two fields can be
present and required at a time in the connector object.
In
`@packages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/basics.spec.ts`:
- Around line 5-11: The test cases 'undefined old state → empty new state: no
commands' and 'empty → empty: no commands' are flagged as having no assertions
because the expect statements are wrapped inside the compareStates function
call. Add expect.hasAssertions() at the beginning of each test function body to
explicitly signal to the linter that assertions are present, which will suppress
the warning and keep the lint signal clean.
In
`@packages/timeline-state-resolver/src/integrations/vindral-composer/`$schemas/actions.json:
- Around line 8-26: The triggerConnector action payload schema does not enforce
that name and value are mutually exclusive as documented. Add a oneOf constraint
to the payload object that defines two schemas: one requiring the name property
and one requiring the value property. In each schema variant, ensure params
remains optional and additionalProperties is set to false. This will enforce
that exactly one of name or value must be provided while still allowing the
optional params field in both cases.
In
`@packages/timeline-state-resolver/src/integrations/vindral-composer/`$schemas/mappings.json:
- Around line 46-63: The switcher schema declares mutual exclusivity between
switcherId and switcherName in the descriptions, but does not enforce this
constraint in the JSON schema itself, allowing both, neither, or just one to be
present. Add a JSON Schema constraint using oneOf to enforce that exactly one of
the two fields (switcherId or switcherName) must be provided, eliminating the
non-deterministic targeting issue caused by ambiguous selector fallback
behavior.
In `@packages/timeline-state-resolver/src/integrations/vindral-composer/index.ts`:
- Around line 69-75: The terminate() method clears the _connection reference
before the disconnected callback can update state, leaving _connected as true
after shutdown, which causes connected and getStatus() to report incorrect
status. To fix this, explicitly set the _connected flag to false at the
beginning of the terminate() method before clearing the connection reference.
This ensures the connection state is immediately marked as disconnected and
prevents stale connected status from being reported after termination.
In
`@packages/timeline-state-resolver/src/integrations/vindral-composer/stateBuilder.ts`:
- Around line 92-247: The buildVindralState function exceeds the cognitive
complexity limit of 30 (currently at 53) due to its large switch statement
handling multiple mapping types. Refactor by extracting each case block in the
switch statement (MappingVindralComposerType.Connector, SceneLayer,
ScriptEngine, Switcher, SwitcherOverlay, MediaPlayer, AudioSource, and Html)
into separate dedicated handler functions. These handlers should accept the
relevant parameters (obj, mapping, content, existing state if needed) and return
the constructed state object. Keep buildVindralState as a thin orchestration
function that iterates through objects, validates mapping and content types, and
dispatches to the appropriate handler function based on
mapping.options.mappingType.
---
Nitpick comments:
In
`@packages/timeline-state-resolver/src/__mocks__/vindral-composer-connection.ts`:
- Line 6: The `removeAllListeners` property in the mock is currently only a
no-op jest.fn() without actual implementation, which can mask listener-related
bugs in tests. Modify the `removeAllListeners` mock to preserve the actual
EventEmitter behavior by using jest.fn().mockImplementation() to call the parent
class's removeAllListeners method while still tracking invocations for test
assertions. This ensures listeners are actually removed during tests while
maintaining the ability to spy on and verify the method was called.
In
`@packages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/mediaPlayers.spec.ts`:
- Around line 51-75: The test for media player mapping does not assert the
default playing behavior when the playing property is omitted from the input. In
the expect statement where result.mediaPlayers is checked with toMatchObject,
add an assertion for playing: true to the expected object to verify that the
default playing state is correctly applied by buildVindralState when no explicit
playing value is provided.
In
`@packages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/switchers.spec.ts`:
- Around line 53-79: Add a regression test case to validate the explicit null
clear path for the transition property in the switcher mapping. After the
existing test "switcher mapping — partial fields", create a new test that passes
transition: null in the switcher content and verify that the builder correctly
handles this explicit null value the same way it handles undefined. Include an
assertion in the buildVindralState result that confirms the transition property
is handled appropriately when explicitly set to null.
In
`@packages/timeline-state-resolver/src/integrations/vindral-composer/diffState.ts`:
- Around line 175-283: The diffMediaPlayers function combines three distinct
concerns: script-engine flow logic, direct HTTP flow logic, and play/pause
command handling in a single 100+ line block. Extract the script-engine flow
handling (the logic inside the useScriptEngine conditional starting around the
scriptInTime variable) into a separate helper function that takes the old and
new media player states and returns an array of commands. Similarly, extract the
direct HTTP flow handling (the logic after the continue statement, handling
endBehaviour, autoPlayOnMediaChange, sourceUrl changes, and play/pause) into its
own helper function. Then refactor the main diffMediaPlayers loop to call these
focused helpers, passing the necessary parameters and collecting their returned
commands. This separation will reduce the cyclomatic complexity and make
sequencing logic easier to verify and maintain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b4109a2a-82cb-4386-9131-6cec5fecb1cf
⛔ Files ignored due to path filters (6)
packages/timeline-state-resolver-types/src/__tests__/__snapshots__/index.spec.ts.snapis excluded by!**/*.snappackages/timeline-state-resolver-types/src/generated/device-options.tsis excluded by!**/generated/**packages/timeline-state-resolver-types/src/generated/index.tsis excluded by!**/generated/**packages/timeline-state-resolver-types/src/generated/vindral-composer.tsis excluded by!**/generated/**packages/timeline-state-resolver/src/__tests__/__snapshots__/index.spec.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (39)
package.jsonpackages/timeline-state-resolver-types/src/index.tspackages/timeline-state-resolver-types/src/integrations/vindral-composer/timeline.tspackages/timeline-state-resolver/jest.config.jspackages/timeline-state-resolver/package.jsonpackages/timeline-state-resolver/src/__mocks__/_setup-all-mocks.tspackages/timeline-state-resolver/src/__mocks__/vindral-composer-connection.tspackages/timeline-state-resolver/src/integrations/vindral-composer/$schemas/actions.jsonpackages/timeline-state-resolver/src/integrations/vindral-composer/$schemas/mappings.jsonpackages/timeline-state-resolver/src/integrations/vindral-composer/$schemas/options.jsonpackages/timeline-state-resolver/src/integrations/vindral-composer/SCRIPT_ENGINE.js.examplepackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/audioSources.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/basics.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/connectors.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/helpers.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/htmlRenderers.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/mediaPlayers.scriptEngine.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/mediaPlayers.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/sceneLayers.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/scriptEngineWarnings.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/scriptEngines.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/switcherOverlays.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/diffState/switchers.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/lib.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/audioSources.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/htmlRenderers.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/layers.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/mediaPlayers.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/switcherOverlays.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/__tests__/stateBuilder/switchers.spec.tspackages/timeline-state-resolver/src/integrations/vindral-composer/actions.tspackages/timeline-state-resolver/src/integrations/vindral-composer/commands.tspackages/timeline-state-resolver/src/integrations/vindral-composer/constants.tspackages/timeline-state-resolver/src/integrations/vindral-composer/diffState.tspackages/timeline-state-resolver/src/integrations/vindral-composer/index.tspackages/timeline-state-resolver/src/integrations/vindral-composer/stateBuilder.tspackages/timeline-state-resolver/src/manifest.tspackages/timeline-state-resolver/src/service/ConnectionManager.tspackages/timeline-state-resolver/src/service/devices.ts
Comment on lines
+69
to
+75
| async terminate(): Promise<void> { | ||
| const conn = this._connection | ||
| this._connection = undefined | ||
| conn?.removeAllListeners() | ||
| conn?.on('error', () => undefined) // prevent unhandled error events during async disconnect | ||
| conn?.disconnect() | ||
| } |
There was a problem hiding this comment.
Reset connection flags during termination to avoid stale connected status.
Line 71 drops _connection and Line 72 removes listeners before any disconnected callback can update state, so _connected can remain true after terminate(). That leaves connected/getStatus() reporting incorrect status post-shutdown.
Suggested fix
async terminate(): Promise<void> {
const conn = this._connection
this._connection = undefined
+ this._connected = false
+ this._stateWarnings = []
+ this._connectionChanged()
conn?.removeAllListeners()
conn?.on('error', () => undefined) // prevent unhandled error events during async disconnect
conn?.disconnect()
}📝 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.
Suggested change
| async terminate(): Promise<void> { | |
| const conn = this._connection | |
| this._connection = undefined | |
| conn?.removeAllListeners() | |
| conn?.on('error', () => undefined) // prevent unhandled error events during async disconnect | |
| conn?.disconnect() | |
| } | |
| async terminate(): Promise<void> { | |
| const conn = this._connection | |
| this._connection = undefined | |
| this._connected = false | |
| this._stateWarnings = [] | |
| this._connectionChanged() | |
| conn?.removeAllListeners() | |
| conn?.on('error', () => undefined) // prevent unhandled error events during async disconnect | |
| conn?.disconnect() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/timeline-state-resolver/src/integrations/vindral-composer/index.ts`
around lines 69 - 75, The terminate() method clears the _connection reference
before the disconnected callback can update state, leaving _connected as true
after shutdown, which causes connected and getStatus() to report incorrect
status. To fix this, explicitly set the _connected flag to false at the
beginning of the terminate() method before clearing the connection reference.
This ensures the connection state is immediately marked as disconnected and
prevents stale connected status from being reported after termination.
|
justandras
approved these changes
Jun 22, 2026
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About the Contributor
This pull request is posted on behalf of SVT
Type of Contribution
This is a: Feature
New Behavior
Adds a new device integration for controlling Vindral Composer https://vindral.com/composer/
Testing Instructions
Other Information
Status