Update atem-state#445
Merged
Merged
Conversation
mint-dewit
added
the
contribution from BBC
|
WalkthroughThe changes refactor ATEM integration type definitions and state handling. A new AtemTransitionSelection enum with bit-flag values is introduced. TimelineContentAtemME.me is restructured from a discriminated union to an explicit object with transition-related fields including transition style, selection, position, and preview behavior. Implementation logic is updated to use ProgramInputCommand instead of separate PreviewInputCommand/CutCommand sequences, and programInput derivation uses nullish coalescing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/timeline-state-resolver-types/src/integrations/atem.ts (1)
161-190:⚠️ Potential issue | 🟠 MajorKeep the M/E input shapes mutually exclusive.
previewInputbeing “cuts only” and theinput/programInputsplit are now comment-level rules only. The runtime still treats them as discriminated branches:packages/timeline-state-resolver/src/integrations/atem/state.tssuppressespreviewInputonly wheninputexists, soprogramInput + previewInputandinput + previewInputnow both type-check but are serialized differently. Please keep the old XOR/discriminated shape for the input fields and layer the new transition properties on top of it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver-types/src/integrations/atem.ts` around lines 161 - 190, The me input shape must be a discriminated (mutually exclusive) union so previewInput cannot coexist with programInput/input; change the me property/type in the ATEM integration types to express an XOR between (a) { input?: number } or { programInput?: number } and (b) { previewInput?: number } so that previewInput is only valid when no program/input is present, then compose/extend that union with the existing transition-related fields (transition, inTransition, transitionPreview, transitionPosition, transitionSettings, transitionSelection) so the transition properties remain layered on top; ensure this matches the runtime handling in packages/timeline-state-resolver/src/integrations/atem/state.ts (which currently suppresses previewInput only when input exists) by making the type reflect the same discriminated branches.packages/timeline-state-resolver/src/integrations/atem/stateBuilder.ts (1)
153-173:⚠️ Potential issue | 🟠 MajorStamp the
.basecontrolValue whentransitionSelectionchanges.A selection-only M/E object now updates
transitionProperties.nextSelection, but Line 484 still only records the.baseaddress forpreviewInputortransition. That meansvideo.mixEffects.{index}.basecan change without ever getting a fresh controlValue, which breaks granular tracking for selection-only layers.🩹 Minimal fix
- if ('previewInput' in content.me || 'transition' in content.me) { + if ('previewInput' in content.me || 'transition' in content.me || 'transitionSelection' in content.me) { addresses.push('video.mixEffects.' + mapping.index + '.base') }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/integrations/atem/stateBuilder.ts` around lines 153 - 173, When handling selection-only updates you currently set stateMixEffect.transitionProperties.nextSelection from content.me.transitionSelection but don’t update the corresponding controlValue address, so video.mixEffects.{index}.base can be stale; when content.me.transitionSelection exists (and has length) also stamp the mix effect base controlValue the same way you do for previewInput/transition updates — i.e., update this.#deviceState.video.mixEffects[mapping.index].base (the controlValue/address derived from content.me.base or the same previewInput/transition path you use elsewhere) so selection-only M/E changes get a fresh .base controlValue alongside stateMixEffect.transitionProperties.nextSelection.
🧹 Nitpick comments (1)
packages/timeline-state-resolver/src/integrations/atem/__tests__/atem.spec.ts (1)
165-166: Cover the newprogramInputpath in at least one integration test.These updated assertions still drive the M/E through deprecated
me.input, so the new serialization path inpackages/timeline-state-resolver/src/integrations/atem/state.tsis still untested. Swapping one of these happy-path cases toprogramInputwould give this upgrade a regression test for the behavior it is actually introducing.Also applies to: 200-201, 281-282
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/integrations/atem/__tests__/atem.spec.ts` around lines 165 - 166, Update one (or more) integration tests so a happy-path case drives the M/E via the new programInput field instead of the deprecated me.input; replace the old expectation that asserted the deprecated path with an assertion that the resolver emits the new AtemConnection.Commands.ProgramInputCommand (use compareAtemCommands(allCommands[0], new AtemConnection.Commands.ProgramInputCommand(...)) to assert payload), and similarly adjust the other two test locations mentioned (the assertions around the other cases at the ranges referenced) so at least one test explicitly exercises the programInput serialization path implemented in packages/timeline-state-resolver/src/integrations/atem/state.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/timeline-state-resolver-types/src/integrations/atem.ts`:
- Around line 161-190: The me input shape must be a discriminated (mutually
exclusive) union so previewInput cannot coexist with programInput/input; change
the me property/type in the ATEM integration types to express an XOR between (a)
{ input?: number } or { programInput?: number } and (b) { previewInput?: number
} so that previewInput is only valid when no program/input is present, then
compose/extend that union with the existing transition-related fields
(transition, inTransition, transitionPreview, transitionPosition,
transitionSettings, transitionSelection) so the transition properties remain
layered on top; ensure this matches the runtime handling in
packages/timeline-state-resolver/src/integrations/atem/state.ts (which currently
suppresses previewInput only when input exists) by making the type reflect the
same discriminated branches.
In `@packages/timeline-state-resolver/src/integrations/atem/stateBuilder.ts`:
- Around line 153-173: When handling selection-only updates you currently set
stateMixEffect.transitionProperties.nextSelection from
content.me.transitionSelection but don’t update the corresponding controlValue
address, so video.mixEffects.{index}.base can be stale; when
content.me.transitionSelection exists (and has length) also stamp the mix effect
base controlValue the same way you do for previewInput/transition updates —
i.e., update this.#deviceState.video.mixEffects[mapping.index].base (the
controlValue/address derived from content.me.base or the same
previewInput/transition path you use elsewhere) so selection-only M/E changes
get a fresh .base controlValue alongside
stateMixEffect.transitionProperties.nextSelection.
---
Nitpick comments:
In
`@packages/timeline-state-resolver/src/integrations/atem/__tests__/atem.spec.ts`:
- Around line 165-166: Update one (or more) integration tests so a happy-path
case drives the M/E via the new programInput field instead of the deprecated
me.input; replace the old expectation that asserted the deprecated path with an
assertion that the resolver emits the new
AtemConnection.Commands.ProgramInputCommand (use
compareAtemCommands(allCommands[0], new
AtemConnection.Commands.ProgramInputCommand(...)) to assert payload), and
similarly adjust the other two test locations mentioned (the assertions around
the other cases at the ranges referenced) so at least one test explicitly
exercises the programInput serialization path implemented in
packages/timeline-state-resolver/src/integrations/atem/state.ts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c6347af-52c7-4760-9b9c-cd7300fe2bfc
⛔ Files ignored due to path filters (3)
packages/timeline-state-resolver-types/src/__tests__/__snapshots__/index.spec.ts.snapis excluded by!**/*.snappackages/timeline-state-resolver/src/__tests__/__snapshots__/index.spec.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/timeline-state-resolver-types/src/integrations/atem.tspackages/timeline-state-resolver/package.jsonpackages/timeline-state-resolver/src/integrations/atem/__tests__/atem.spec.tspackages/timeline-state-resolver/src/integrations/atem/__tests__/diffStates.spec.tspackages/timeline-state-resolver/src/integrations/atem/state.tspackages/timeline-state-resolver/src/integrations/atem/stateBuilder.ts
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
1 participant
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 the BBC.
Type of Contribution
Updates for Sofie-Automation/sofie-atem-state#46