feat: TSR Device Feedback#456
Open
Julusian wants to merge 14 commits into
Open
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughAdds a typed device state-event system: new event types, StateEventHandler for subscription-filtered batching, context API event reporting, device and connection wiring for subscriptions/emission, many integration constructor type updates, ATEM external-address event reporting, and updated schema generation for per-integration Events types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Conductor
participant ConnMgr as ConnectionManager
participant DeviceInst as DeviceInstanceWrapper
participant StateHandler as StateEventHandler
participant Listener
Client->>Conductor: setDeviceEventSubscriptions(deviceId, events[])
Conductor->>ConnMgr: setDeviceEventSubscriptions(deviceId, events[])
ConnMgr->>DeviceInst: setEventSubscriptions(events[])
DeviceInst->>StateHandler: setEventSubscriptions(events[])
Note over DeviceInst,StateHandler: Later, device reports an external change
DeviceInst->>StateHandler: report(eventName, payload)
StateHandler->>StateHandler: filter by allowed set, queue, schedule flush (setImmediate)
Note over StateHandler: next tick
StateHandler->>DeviceInst: onFlush([SomeTSRStateEvent])
DeviceInst->>ConnMgr: emit('connectionEvent:stateEvent', events)
ConnMgr->>Listener: connectionEvent:stateEvent
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Julusian
added
the
contribution from BBC
![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
🧹 Nitpick comments (1)
packages/quick-tsr/src/tsrHandler.ts (1)
152-156: Use defensive payload serialization in event logging.Direct
JSON.stringifyon external payloads can throw and interrupt event handling. Consider a safe fallback here.♻️ Suggested hardening
this.tsr.connectionManager.on('connectionEvent:stateEvent', (deviceId: string, events: SomeTSRStateEvent[]) => { for (const e of events) { - const payloadStr = e.payload === null ? 'null' : JSON.stringify(e.payload) + let payloadStr: string + try { + payloadStr = e.payload === null ? 'null' : JSON.stringify(e.payload) + } catch { + payloadStr = '[unserializable payload]' + } console.log(`State event [${deviceId}] ${e.event}: ${payloadStr}`) } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/quick-tsr/src/tsrHandler.ts` around lines 152 - 156, The event logging in the connectionManager handler (the callback registered on 'connectionEvent:stateEvent' within tsrHandler.ts) uses JSON.stringify(e.payload) which can throw on circular or unsafe payloads; wrap payload serialization in a defensive helper or inline try/catch: attempt JSON.stringify(e.payload) and on failure fall back to a safe representation (e.g. util.inspect, a safeStringify helper, or a fixed placeholder like '<unserializable payload>') before calling console.log so that a bad payload cannot break the for-loop or event processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/timeline-state-resolver-tools/bin/schema-types.mjs`:
- Around line 483-485: The deviceTypesMapEntries generation uses raw dirs[i]
which can become misaligned with deviceTypeEnum when some dirs are skipped; fix
by pairing each device type with its corresponding processed dir when building
deviceTypeEnum (or create a processedDirs array or map keyed by typeId) and then
use that paired dir value instead of dirs[i] inside deviceTypesMapEntries;
update the code that constructs deviceTypeEnum (and/or introduce processedDirs)
and change deviceTypesMapEntries to reference the paired/processed dir (and
still fallback to typeId) when calling capitalise to produce the correct
*DeviceTypes name.
In `@packages/timeline-state-resolver/src/__tests__/mockDeviceInstanceWrapper.ts`:
- Around line 112-114: The mock implementation of setEventSubscriptions
currently throws a "Method not implemented." error which breaks unrelated tests;
change the jest.fn in mockDeviceInstanceWrapper so setEventSubscriptions is a
no-op by default (e.g., jest.fn((_events: string[]): void => { /* no-op */ }))
so tests that only check wiring don't fail, while keeping the mock configurable
so specific tests can still assert calls or override behavior.
In
`@packages/timeline-state-resolver/src/service/__tests__/stateEventHandler.spec.ts`:
- Around line 107-125: The test's name and comments contradict its assertion:
update the test name and inline comments in the test that uses makeHandler,
handler.report, handler.setEventSubscriptions and the onFlush assertion so they
reflect the intended behavior (that an event queued before subscriptions are
cleared is still flushed and onFlush is called once); specifically rename the
test from “does not call onFlush if all pending events were filtered” to
something like “calls onFlush for events queued before subscriptions cleared”
and adjust the explanatory comments to state that pending snapshot is taken at
flush time and therefore the queued event will be flushed.
In `@packages/timeline-state-resolver/src/service/DeviceInstance.ts`:
- Around line 146-154: The integration hook calls onAddressControlRestored and
onAddressExternallyChanged are invoked directly and can throw, so wrap each
invocation in a try/catch to prevent exceptions from bubbling out of
DeviceInstance; specifically update the call site where
this._device.onAddressControlRestored?.(a) is invoked and the handler registered
in this._stateTracker.on('deviceUpdated', (addr, ahead) => { if (ahead) {
this._device.onAddressExternallyChanged?.(addr) } }) to catch any errors from
the hook, handle or log the error and continue processing so device update
handling cannot be interrupted by a faulty integration callback.
In `@packages/timeline-state-resolver/src/service/stateEventHandler.ts`:
- Around line 24-26: setEventSubscriptions currently updates this.#allowedEvents
but queued events are not re-validated so stale ones can still be emitted; fix
by either (a) purging the internal queue of events that are no longer in the
updated this.#allowedEvents inside setEventSubscriptions, or (b) adding a
whitelist check in the flush/processQueue routine (the method that
dequeues/emits events) to skip any queued events not in this.#allowedEvents
before emitting; reference the private field `#allowedEvents`, the
setEventSubscriptions method, and the queue enqueue/flush (processQueue/flush)
logic so the filter is applied at dequeue time or when subscriptions change.
---
Nitpick comments:
In `@packages/quick-tsr/src/tsrHandler.ts`:
- Around line 152-156: The event logging in the connectionManager handler (the
callback registered on 'connectionEvent:stateEvent' within tsrHandler.ts) uses
JSON.stringify(e.payload) which can throw on circular or unsafe payloads; wrap
payload serialization in a defensive helper or inline try/catch: attempt
JSON.stringify(e.payload) and on failure fall back to a safe representation
(e.g. util.inspect, a safeStringify helper, or a fixed placeholder like
'<unserializable payload>') before calling console.log so that a bad payload
cannot break the for-loop or event processing.
🪄 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: 2b9ac143-af15-4cea-a0db-ec41639c1929
⛔ Files ignored due to path filters (2)
packages/timeline-state-resolver-types/src/generated/atem.tsis excluded by!**/generated/**packages/timeline-state-resolver-types/src/generated/index.tsis excluded by!**/generated/**
📒 Files selected for processing (73)
packages/quick-tsr/input/settings.tspackages/quick-tsr/src/index.tspackages/quick-tsr/src/tsrHandler.tspackages/timeline-state-resolver-api/src/device.tspackages/timeline-state-resolver-api/src/tsr-api.tspackages/timeline-state-resolver-tools/bin/schema-types.mjspackages/timeline-state-resolver-types/src/events.tspackages/timeline-state-resolver-types/src/expectedPlayoutItems.tspackages/timeline-state-resolver-types/src/index.tspackages/timeline-state-resolver-types/src/integrations/abstract/timeline.tspackages/timeline-state-resolver-types/src/integrations/atem/events.tspackages/timeline-state-resolver-types/src/integrations/atem/timeline.tspackages/timeline-state-resolver-types/src/integrations/casparcg/timeline.tspackages/timeline-state-resolver-types/src/integrations/httpSend/timeline.tspackages/timeline-state-resolver-types/src/integrations/hyperdeck/timeline.tspackages/timeline-state-resolver-types/src/integrations/kairos/timeline.tspackages/timeline-state-resolver-types/src/integrations/lawo/timeline.tspackages/timeline-state-resolver-types/src/integrations/multiOsc/timeline.tspackages/timeline-state-resolver-types/src/integrations/obs/timeline.tspackages/timeline-state-resolver-types/src/integrations/ograf/timeline.tspackages/timeline-state-resolver-types/src/integrations/osc/timeline.tspackages/timeline-state-resolver-types/src/integrations/panasonicPTZ/timeline.tspackages/timeline-state-resolver-types/src/integrations/pharos/timeline.tspackages/timeline-state-resolver-types/src/integrations/quantel/timeline.tspackages/timeline-state-resolver-types/src/integrations/shotoku/timeline.tspackages/timeline-state-resolver-types/src/integrations/singularLive/timeline.tspackages/timeline-state-resolver-types/src/integrations/sisyfos/timeline.tspackages/timeline-state-resolver-types/src/integrations/sofieChef/timeline.tspackages/timeline-state-resolver-types/src/integrations/tcpSend/timeline.tspackages/timeline-state-resolver-types/src/integrations/telemetrics/timeline.tspackages/timeline-state-resolver-types/src/integrations/tricaster/timeline.tspackages/timeline-state-resolver-types/src/integrations/udpSend/timeline.tspackages/timeline-state-resolver-types/src/integrations/viscaOverIP/timeline.tspackages/timeline-state-resolver-types/src/integrations/vizMSE/timeline.tspackages/timeline-state-resolver-types/src/integrations/vmix/timeline.tspackages/timeline-state-resolver-types/src/integrations/websocketClient/timeline.tspackages/timeline-state-resolver/src/__tests__/mockDeviceInstanceWrapper.tspackages/timeline-state-resolver/src/conductor.tspackages/timeline-state-resolver/src/devices/device.tspackages/timeline-state-resolver/src/integrations/__tests__/testlib.tspackages/timeline-state-resolver/src/integrations/abstract/index.tspackages/timeline-state-resolver/src/integrations/atem/index.tspackages/timeline-state-resolver/src/integrations/httpSend/index.tspackages/timeline-state-resolver/src/integrations/httpWatcher/index.tspackages/timeline-state-resolver/src/integrations/hyperdeck/index.tspackages/timeline-state-resolver/src/integrations/kairos/index.tspackages/timeline-state-resolver/src/integrations/kairos/kairos-application-monitor.tspackages/timeline-state-resolver/src/integrations/kairos/lib/kairosRamLoader.tspackages/timeline-state-resolver/src/integrations/lawo/index.tspackages/timeline-state-resolver/src/integrations/multiOsc/index.tspackages/timeline-state-resolver/src/integrations/obs/index.tspackages/timeline-state-resolver/src/integrations/ograf/index.tspackages/timeline-state-resolver/src/integrations/osc/index.tspackages/timeline-state-resolver/src/integrations/panasonicPTZ/index.tspackages/timeline-state-resolver/src/integrations/pharos/index.tspackages/timeline-state-resolver/src/integrations/quantel/index.tspackages/timeline-state-resolver/src/integrations/shotoku/index.tspackages/timeline-state-resolver/src/integrations/singularLive/index.tspackages/timeline-state-resolver/src/integrations/sisyfos/__tests__/sisyfos.spec.tspackages/timeline-state-resolver/src/integrations/sisyfos/index.tspackages/timeline-state-resolver/src/integrations/sofieChef/index.tspackages/timeline-state-resolver/src/integrations/tcpSend/index.tspackages/timeline-state-resolver/src/integrations/telemetrics/index.tspackages/timeline-state-resolver/src/integrations/tricaster/index.tspackages/timeline-state-resolver/src/integrations/udpSend/index.tspackages/timeline-state-resolver/src/integrations/viscaOverIP/index.tspackages/timeline-state-resolver/src/integrations/vmix/index.tspackages/timeline-state-resolver/src/integrations/websocketClient/index.tspackages/timeline-state-resolver/src/service/ConnectionManager.tspackages/timeline-state-resolver/src/service/DeviceInstance.tspackages/timeline-state-resolver/src/service/__tests__/stateEventHandler.spec.tspackages/timeline-state-resolver/src/service/remoteDeviceInstance.tspackages/timeline-state-resolver/src/service/stateEventHandler.ts
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 (1)
packages/timeline-state-resolver/src/service/DeviceInstance.ts (1)
237-240:⚠️ Potential issue | 🟡 Minor
terminate()does not drain/cancel the pending state-event flush.
StateEventHandlerschedules asetImmediateflush onreport(...)(seestateEventHandler.ts:39-46). If a device emits a state event just beforeterminate()is invoked (e.g. during shutdown of a sub-connection), the deferred callback will run afterterminate()resolves and causeDeviceInstanceWrapperto emit'stateEvent'for an already-terminated device. Downstream (ConnectionManager→connectionEvent:stateEvent) this can deliver events for a device the consumer believes has been torn down.Consider adding a
dispose()/terminate()method toStateEventHandlerthat clears#pendingEventsand ignores the queuedsetImmediate, and calling it from thisterminate()before/after_device.terminate().♻️ Sketch of the change
In
stateEventHandler.ts:+ terminate(): void { + this.#allowedEvents.clear() + this.#pendingEvents = [] + this.#terminated = true + }…and guard the
setImmediatecallback /reportagainst#terminated. Then inDeviceInstance.ts:async terminate() { this._stateHandler.terminate() + this._stateEventHandler.terminate() return this._device.terminate() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/service/DeviceInstance.ts` around lines 237 - 240, The terminate() on DeviceInstance currently calls this._stateHandler.terminate() but does not cancel the StateEventHandler's pending setImmediate flush, allowing late stateEvent emissions after tear-down; add a disposal flag and a new dispose()/terminate() implementation on StateEventHandler (or extend its existing terminate) that clears `#pendingEvents`, marks it as terminated, and makes report(...) / the scheduled setImmediate callback no-op when terminated, then update DeviceInstance.terminate() to call the StateEventHandler disposal before (or immediately after) calling this._device.terminate() so no queued flush can emit 'stateEvent' for a torn-down device.
🧹 Nitpick comments (1)
packages/timeline-state-resolver/src/service/DeviceInstance.ts (1)
295-298: Add validation or diagnostic logging for event subscription mismatches.
setEventSubscriptionsacceptsevents: string[]with no validation against the device's known events. Misconfigured or typo'd entries silently produce no emissions becausereport(...)filters by exact membership. While the configuration at the consumer level has type safety (TSREventTypesMap), runtime validation doesn't exist here.Consider logging a warning (via the existing
'warning'channel or'debug') when a subscribed event name is not something the device would report. This would make configuration errors easier to diagnose, since currently unmatched events fail silently with no feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/service/DeviceInstance.ts` around lines 295 - 298, setEventSubscriptions currently accepts events: string[] with no runtime validation, so typos silently fail because report(...) only emits exact matches; update setEventSubscriptions to validate each entry against the device's known/reportable events (e.g., use the device definition or the same source used by report(...) to derive allowed event names) and emit a warning via this.emit('warning', ...) or this.emit('debug', ...) for any unknown/mismatched event names before delegating to this._stateEventHandler.setEventSubscriptions(events); ensure you still call setEventSubscriptions with the original array but include the diagnostic logging for mismatches to aid debugging.
🤖 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/src/service/DeviceInstance.ts`:
- Around line 237-240: The terminate() on DeviceInstance currently calls
this._stateHandler.terminate() but does not cancel the StateEventHandler's
pending setImmediate flush, allowing late stateEvent emissions after tear-down;
add a disposal flag and a new dispose()/terminate() implementation on
StateEventHandler (or extend its existing terminate) that clears `#pendingEvents`,
marks it as terminated, and makes report(...) / the scheduled setImmediate
callback no-op when terminated, then update DeviceInstance.terminate() to call
the StateEventHandler disposal before (or immediately after) calling
this._device.terminate() so no queued flush can emit 'stateEvent' for a
torn-down device.
---
Nitpick comments:
In `@packages/timeline-state-resolver/src/service/DeviceInstance.ts`:
- Around line 295-298: setEventSubscriptions currently accepts events: string[]
with no runtime validation, so typos silently fail because report(...) only
emits exact matches; update setEventSubscriptions to validate each entry against
the device's known/reportable events (e.g., use the device definition or the
same source used by report(...) to derive allowed event names) and emit a
warning via this.emit('warning', ...) or this.emit('debug', ...) for any
unknown/mismatched event names before delegating to
this._stateEventHandler.setEventSubscriptions(events); ensure you still call
setEventSubscriptions with the original array but include the diagnostic logging
for mismatches to aid debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3dc6edba-1449-4984-b08f-42aaa374566b
📒 Files selected for processing (2)
packages/timeline-state-resolver/src/service/DeviceInstance.tspackages/timeline-state-resolver/src/service/__tests__/stateEventHandler.spec.ts
5 tasks
|
nytamin
approved these changes
May 14, 2026
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 the BBC
Type of Contribution
This is a: Feature
New Behavior
RFC: Sofie-Automation/sofie-core#1670
Allow TSR devices to report events when external state changes occur. In sofie, this will trigger a new blueprints method to allow for custom handling of these state changes.
Each device type has a list of defined events it can produce (with typings that are available to consumers of the api/events). The user of TSR has to tell each device which events to emit, any other events are discarded inside of the device thread.
Some types generation has been adjusted a bit to be able to propagate the new types as needed.
Testing Instructions
This has been tested with both new logging added to quick-tsr, and the full flow into sofie blueprints.
Other Information
Status