fix: shared control undefined state#419
Conversation
mint-dewit
added
bug
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughThis PR introduces optional startup synchronization control for devices, refactors address state reassertion logic to permit undefined states, enhances command report logging in quick-tsr, improves ATEM upstream-keyer state comparison using resolver utilities, and updates state management with debouncing and address cleanup mechanisms. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/timeline-state-resolver-api/src/device.ts (1)
109-112: Device.addressStateReassertsControl signature must allow undefined newState.The Device interface (line 111) declares
newState: AddressState, but BaseDeviceAPI (line 154) allowsnewState: AddressState | undefined. Real usage in stateHandler.ts line 242 passes undefined via optional chaining (addrStatefromnextState.addressStates?.[addr]), and the ATEM implementation correctly guards against it. The Device interface signature must be updated to match BaseDeviceAPI and reflect actual usage:- addressStateReassertsControl?(oldState: AddressState | undefined, newState: AddressState): boolean + addressStateReassertsControl?(oldState: AddressState | undefined, newState: AddressState | undefined): boolean
🤖 Fix all issues with AI agents
In `@packages/quick-tsr/src/tsrHandler.ts`:
- Around line 112-114: The handler registered on this.tsr.connectionManager for
the 'connectionEvent:info' event is using console.error incorrectly; change the
logging call in the listener for 'connectionEvent:info' (the callback registered
on this.tsr.connectionManager.on) to use console.info or console.log instead of
console.error so informational connection events use an appropriate log level.
- Around line 99-101: The 'info' event handler is logging with the wrong prefix;
locate the this.tsr.connectionManager.on('info', (msg: string) => { ... })
handler and change the console.log message from "Warning: connectionManager" to
a correct "Info: connectionManager" (or use console.info with
"connectionManager" + msg) so the log prefix matches the event.
In `@packages/timeline-state-resolver/src/`$schemas/common-options.json:
- Around line 31-36: The ui hint for the JSON property "syncOnStartup" contains
an extra leading space before the quoted phrase " Shared Hardware Control";
update the "ui:hint" value in the syncOnStartup schema to remove the leading
space so the quoted phrase reads "Shared Hardware Control" (reference: property
name syncOnStartup and key "ui:hint").
In `@packages/timeline-state-resolver/src/service/__tests__/stateHandler.spec.ts`:
- Line 427: Fix the typo in the test comment that currently reads "device is
ah`ead" by removing the stray backtick so it reads "device is ahead"; update the
comment in the test around the stateHandler.spec.ts assertion where that comment
appears to ensure it is correct and clear.
In `@packages/timeline-state-resolver/src/service/DeviceInstance.ts`:
- Around line 132-143: The deviceUpdated event handler is currently reading the
first argument (address) into the `ahead` variable so `ahead` is a string and
the boolean check always passes; update the handler signature used with
this._stateTracker.on('deviceUpdated', ...) to accept the address first and the
boolean second (e.g., `(address, ahead)` or `(_, ahead)`), then use that second
parameter in the setImmediate callback to conditionally call
this._stateHandler.recalcDiff(); keep the existing doRecalc debounce logic
intact.
🧹 Nitpick comments (3)
packages/timeline-state-resolver/src/integrations/atem/state.ts (1)
255-270: Use correct ME and keyer indices in upstream keyer diff comparison.Line 256 hardcodes ME index
0and creates single-element arrays, discarding the ME and keyer indices fromstate1.index. The address state structure uses[meIndex, keyerId](as seen in the address state construction), but this is ignored. For multi-ME or multi-keyer systems, the diff will compare against the wrong indices. Pass arrays indexed by keyer ID:♻️ Suggested fix
- const output = resolveUpstreamKeyerState(0, [state1.state], [state2.state], { + const meIndex = state1.index[0] as number + const keyerIndex = state1.index[1] as number + const oldKeyers: UpstreamKeyer[] = [] + const newKeyers: UpstreamKeyer[] = [] + oldKeyers[keyerIndex] = state1.state + newKeyers[keyerIndex] = state2.state + const output = resolveUpstreamKeyerState(meIndex, oldKeyers, newKeyers, { sources: true, onAir: true, type: true, mask: true, flyKeyframes: 'all', flyProperties: true, dveSettings: true, chromaSettings: false, advancedChromaSettings: false, lumaSettings: true, patternSettings: true, })packages/quick-tsr/src/tsrHandler.ts (2)
109-111: Inconsistent log level: warnings logged to console.error.Warnings are being logged via
console.error. While this may be intentional for visibility, it's inconsistent with typical logging conventions where warnings useconsole.warn.♻️ Suggested change
this.tsr.connectionManager.on('connectionEvent:warning', (deviceId: string, warning: string) => { - console.error(`Device ${deviceId} connection warning: ${warning}`) + console.warn(`Device ${deviceId} connection warning: ${warning}`) })
124-140: No-op handlers for slow command events.The
slowSentCommandandslowFulfilledCommandhandlers are empty (with commented-out logging). If these events are not needed, consider removing the handlers entirely to avoid confusion. If they're placeholders for future implementation, add a TODO comment.♻️ Suggested change: Remove or document
Option 1 - Remove the no-op handlers:
if (tsrSettings.logCommandReports) { - this.tsr.connectionManager.on( - 'connectionEvent:slowSentCommand', - (_deviceId: string, _info: SlowSentCommandInfo) => { - // console.log(`Device ${device.deviceId} slow sent command: ${_info}`) - } - ) - this.tsr.connectionManager.on( - 'connectionEvent:slowFulfilledCommand', - (_deviceId: string, _info: SlowFulfilledCommandInfo) => { - // console.log(`Device ${device.deviceId} slow fulfilled command: ${_info}`) - } - ) this.tsr.connectionManager.on('connectionEvent:commandReport', (deviceId: string, command: any) => { console.log(`Device ${deviceId} command: ${JSON.stringify(command)}`) }) }Option 2 - Add TODO if these are placeholders:
this.tsr.connectionManager.on( 'connectionEvent:slowSentCommand', (_deviceId: string, _info: SlowSentCommandInfo) => { - // console.log(`Device ${device.deviceId} slow sent command: ${_info}`) + // TODO: Implement slow command logging if needed } )
jstarpl
changed the title
|
Updated the PR to resolve issues highlighted in the generated review. |
| return false | ||
| } else if (state1.type === AddressType.UpStreamKey && state2.type === AddressType.UpStreamKey) { | ||
| const output = resolveUpstreamKeyerState(0, [state1.state], [state2.state], { | ||
| sources: true, |
There was a problem hiding this comment.
Is this diff object the same as the one being used at the point where the command diffing happens?
Perhaps it should be pulled out as a constant that both places can use to keep it in sync?
There was a problem hiding this comment.
Yes, I was thinking about that as well. I mostly just dropped this in because the device state and timeline state aren't strictly/deeply equal even though we should treat them as such. I don't think they have to be using the same options but equally I can't think of a reason why they shouldn't. I'll just pull it out so at least the next person is aware of the usage.
3 participants
About the Contributor
This pull request is posted on behalf of the BBC.
Type of Contribution
This is a: Bug Fix aimed at the Shared Control feature used with the Atem integration
Current Behavior
With no active timeline objects the Atem Integration will overwrite any changes made to Mix Effects by external actors. This is because the "old state" will have the external changes added to it but the "new state" will not get these changes (as there is no address to apply them to)
New Behavior
The Integration correctly responds to external actors even without active timeline objects (i.e. does nothing until the Expected State is changed or Control Value changes). Additionally, when a timeline object is removed from the timeline, the Expected State for the device will be removed as well. I believe this should address all cases when dealing with "undefined" states on the timeline.
The additional Device Setting lets a user override the startup behaviour when a Device reports some State before the TSR has seen Timeline Objects for this Address:
Additional changes
Testing Instructions
With shared control enabled, an empty timeline should not control the Atem. When going from an empty timeline back to a timeline object the integration should correctly reassert control.
Status
Summary
This PR fixes a bug in the Shared Control feature when used with the Atem integration, where the Atem device could incorrectly overwrite external changes when no active timeline objects exist. The fix ensures the integration properly ignores external actors until timeline objects are present, and correctly removes expected state for addresses no longer on the timeline.
Changes
Core Bug Fix - Shared Control State Management:
addressStateReassertsControlmethod signature in the Device API to acceptundefinedfor thenewStateparameter, allowing the integration to distinguish between "no timeline object" and "timeline object with specific state"unsetExpectedStatemechanism in StateTracker to remove expected state entries when addresses are not present on the timelinesyncOnStartupparameter (defaulting to true) to control whether the device syncs on initial connection before timeline objects are presentAtem Integration Improvements:
diffAddressStatesto use the state library for consistent comparison of UpStreamKey (Atem keyer) states viaresolveUpstreamKeyerStatesendCommandto include command constructor names in logged command payloads for improved debuggingLogging and Configuration Enhancements:
logCommandReportsboolean setting to TSRSettings to control command-related event loggingnoUnusedLocalscompiler option in QuickTSR to eliminate spurious unused-variable warningsSchema and Testing:
syncOnStartupproperty to device common options schema, allowing users to disable initial device sync when shared control is enabledTesting