feat: kairos connection SOFIE-4032 #390
Conversation
Julusian
force-pushed
the
feat/kairos-connection
branch
2 times, most recently
from
dc65983 to
dba4c46
Compare
Julusian
force-pushed
the
feat/kairos-connection
branch
from
dba4c46 to
84b7bda
Compare
…connection # Conflicts: # packages/timeline-state-resolver-types/package.json # packages/timeline-state-resolver-types/src/generated/kairos.ts # packages/timeline-state-resolver/package.json # packages/timeline-state-resolver/src/integrations/kairos/$schemas/actions.json # packages/timeline-state-resolver/src/integrations/kairos/index.ts # yarn.lock
Co-authored-by: julusian <[email protected]>
Julusian
force-pushed
the
feat/kairos-connection
branch
from
52a3974 to
29b72c3
Compare
…D it and try again
It turns out that we _dont_ get an Error in response when trying to use a non-RAM-loaded ramrec/still
There was a problem hiding this comment.
I think the naming of every object defined in this file needs to be prefixed with Kairos or something to make it clear they are kairos specific.
I am worried that dumping a RefPath object loose in tsr-types makes the exports confusing, and could well lead to some conflicts if other integrations want to use similar naming
Alternatively, maybe we can do something to not generate these types as they are clones of what are used in kairos-lib, and to re-export kairos-lib under a KairosLib namespace from tsr-types (I think I did try and do that re-export earlier, but our version of typescript is not happy with doing that)
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
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 WalkthroughTypeScript version upgraded to 5.1 with module resolution updates. Kairos device integration added with comprehensive type system, state building, command diffing, and action dispatch. Import statements refactored to use default exports. New setModifiedState method added to DeviceContextAPI for device state mutations. Changes
Sequence Diagram(s)sequenceDiagram
actor Timeline
participant StateBuilder
participant DiffState
participant Commands
participant KairosConnection
Timeline->>StateBuilder: Timeline state + Mappings
activate StateBuilder
StateBuilder->>StateBuilder: Build KairosDeviceState<br/>(scenes, layers, macros, players)
deactivate StateBuilder
StateBuilder-->>DiffState: KairosDeviceState
activate DiffState
DiffState->>DiffState: Compare old vs new state<br/>across all components
DiffState->>DiffState: Generate diffs for:<br/>- Scenes/Snapshots<br/>- Scene Layers<br/>- Aux/Macros<br/>- Media Players
deactivate DiffState
DiffState-->>Commands: KairosCommandWithContext[]
activate Commands
Commands->>Commands: Process each command type<br/>(scene, layer, player, etc.)
Commands->>Commands: Handle media loading<br/>via KairosRamLoader
deactivate Commands
Commands-->>KairosConnection: Execute Commands
activate KairosConnection
KairosConnection->>KairosConnection: Send to Kairos server
KairosConnection-->>Timeline: State Updated
deactivate KairosConnection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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
♻️ Duplicate comments (1)
packages/timeline-state-resolver/src/integrations/kairos/$schemas/lib/refs.json (1)
1-7: Naming collision concern already raised.As noted in the past review, these schema names (e.g.,
RefPath,SceneRef) should be prefixed withKairosto avoid conflicts with other integrations. This applies to all definitions in this file.
🧹 Nitpick comments (12)
packages/timeline-state-resolver-api/src/device.ts (1)
246-252: Minor JSDoc inconsistency.The JSDoc on line 247 says "Reset the tracked device state to 'state'" which appears to be copied from
resetToState. This method modifies rather than resets. Consider updating to something like "Modify the tracked device state and notify the conductor to reset the resolver"./** - * Reset the tracked device state to "state" and notify the conductor to reset the resolver + * Modify the tracked device state and notify the conductor to reset the resolver * @param cb A callback that receives the current state, and should return the modified state. * Note: The `currentState` argument is a clone, so it is safe to modify it directly. * If no changes have been made, return false. */ setModifiedState: (cb: (currentState: DeviceState) => DeviceState | false) => Promise<void>packages/timeline-state-resolver/src/service/DeviceInstance.ts (1)
323-332: Consider handling undefined current state explicitly.When
getCurrentState()returnsundefined(no state has been set yet),cloneDeep(undefined)returnsundefined, and the callback receivesundefinedascurrentState. This may be unexpected for callers who assume they'll receive a validDeviceState.Consider either:
- Documenting this behavior in the API JSDoc
- Early-returning when there's no current state (similar to a no-op)
Option 2 implementation:
setModifiedState: async (cb: (currentState: DeviceState) => DeviceState | false) => { - const currentState = cloneDeep(this._stateHandler.getCurrentState()) + const rawCurrentState = this._stateHandler.getCurrentState() + if (rawCurrentState === undefined) return // No state to modify + + const currentState = cloneDeep(rawCurrentState) const newState = cb(currentState) if (newState === false) return // false means no changes were made, and no resyncStates is necessary await this._stateHandler.setCurrentState(newState) await this._stateHandler.clearFutureStates() this.emit('resyncStates') },packages/timeline-state-resolver/src/integrations/kairos/diffState/media-players.ts (1)
193-224: Copy-paste artifact: Comments refer to "ClipPlayer" instead of "ImageStore".Several comments in this function still reference "ClipPlayer" from what appears to be code copied from
diffMediaPlayers. Consider updating for clarity:const cpRef = newImageStore?.ref || oldImageStore?.ref - if (!cpRef) continue // No ClipPlayer to diff + if (!cpRef) continue // No ImageStore to diff if (!newImageStore && oldImageStore) { - // ClipPlayer obj was removed, stop it: + // ImageStore obj was removed, clear it: ... } else if (newImageStore) { - /** The properties to update on the ClipPlayer */ + /** The properties to update on the ImageStore */ const updateCmd: KairosImageStoreCommand = {packages/timeline-state-resolver/src/integrations/kairos/diffState/lib.ts (2)
9-27: Consider clarifying the "shallow comparison" terminology.The function is documented as performing a "shallow comparison" but uses
isEqualfrom underscore, which performs deep equality checks on values. While the iteration is shallow (top-level keys only), the value comparison is deep. Consider updating the JSDoc to clarify this behavior to avoid confusion.Example clarification:
/** - * Does a shallow comparision of two objects, returning an object with only the changed keys. + * Iterates over top-level keys and returns an object with only the changed keys. + * Uses deep equality comparison for each value. * @param oldObj * @param newObj * @returns */
36-54: Consider clarifying the "shallow comparison" terminology.Similar to
diffObject, this function is documented as performing a "shallow comparison" but uses deep equality checks. Consider updating the JSDoc for consistency.packages/timeline-state-resolver/src/integrations/kairos/__tests__/diffState.spec.ts (1)
494-496: Address or remove commented test.The commented test for temporal order concerns is a placeholder. Given that the PR is marked as not fully built out, this is acceptable for now.
Would you like me to open a GitHub issue to track implementing this test case for temporal order when cutting to/from a clip player?
packages/timeline-state-resolver/src/integrations/kairos/diffState.ts (4)
8-8: Remove redundant self-alias.The import
diffMediaImageStore as diffMediaImageStoreis a no-op rename.-import { diffMediaPlayers, diffMediaImageStore as diffMediaImageStore } from './diffState/media-players' +import { diffMediaPlayers, diffMediaImageStore } from './diffState/media-players'
29-29: Address temporal ordering concern for media players.The TODO raises a valid concern: cutting to/from a clip player before it has started/stopped could cause timing issues. Consider tracking this as an issue if not addressed in this PR.
Would you like me to open an issue to track this temporal ordering concern?
150-151: Remove redundant nullish coalescing.The
?? undefinedis redundant since accessing a property onundefinedalready yieldsundefined.- const oldActive = oldSceneSnapshot?.state.active ?? undefined - const newActive = newSceneSnapshot?.state.active ?? undefined + const oldActive = oldSceneSnapshot?.state.active + const newActive = newSceneSnapshot?.state.active
63-65: Consider removing commented-out code.The commented
SceneDefaultscode and its usage in lines 81-82, 114-115 appear to be leftover scaffolding. If it's intended as a reference for future implementation, consider adding a TODO or removing it entirely.packages/timeline-state-resolver/src/integrations/kairos/stateBuilder.ts (1)
143-146: Consider using typedMappings<SomeMappingKairos>parameter.The
mappingsparameter uses the untypedMappingsinterface, requiring a cast on line 153. UsingMappings<SomeMappingKairos>would provide better type safety.public static fromTimeline( timelineState: DeviceTimelineState<TSRTimelineContent>, - mappings: Mappings + mappings: Mappings<SomeMappingKairos> ): KairosDeviceState {packages/timeline-state-resolver-types/src/integrations/kairos.ts (1)
34-39: Remove or document commented experimental code.The
DELETE_IT/PartialOrNullpattern appears to be experimental scaffolding. If it's planned for future use, add a TODO; otherwise, remove it.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ 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/kairos.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 (45)
package.json(1 hunks)packages/timeline-state-resolver-api/src/device.ts(1 hunks)packages/timeline-state-resolver-api/tsconfig.build.json(1 hunks)packages/timeline-state-resolver-types/.eslintrc.json(1 hunks)packages/timeline-state-resolver-types/package.json(1 hunks)packages/timeline-state-resolver-types/src/index.ts(2 hunks)packages/timeline-state-resolver-types/src/integrations/kairos.ts(1 hunks)packages/timeline-state-resolver-types/tsconfig.build.json(1 hunks)packages/timeline-state-resolver/.eslintrc.json(1 hunks)packages/timeline-state-resolver/package.json(1 hunks)packages/timeline-state-resolver/src/__mocks__/ws.ts(1 hunks)packages/timeline-state-resolver/src/conductor.ts(1 hunks)packages/timeline-state-resolver/src/integrations/__tests__/testlib.ts(1 hunks)packages/timeline-state-resolver/src/integrations/httpSend/__tests__/httpsend.spec.ts(1 hunks)packages/timeline-state-resolver/src/integrations/httpWatcher/__tests__/httpwatcher.spec.ts(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/$schemas/actions.json(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/$schemas/lib/refs.json(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/$schemas/mappings.json(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/$schemas/options.json(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/__tests__/diffState.spec.ts(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/__tests__/lib.ts(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/__tests__/stateBuilder.spec.ts(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/actions.ts(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/commands.ts(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/diffState.ts(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/diffState/lib.ts(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/diffState/media-players.ts(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/index.ts(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/lib/kairosRamLoader.ts(1 hunks)packages/timeline-state-resolver/src/integrations/kairos/stateBuilder.ts(1 hunks)packages/timeline-state-resolver/src/integrations/pharos/__tests__/pharosAPI.spec.ts(1 hunks)packages/timeline-state-resolver/src/integrations/pharos/connection.ts(1 hunks)packages/timeline-state-resolver/src/integrations/sofieChef/__tests__/sofieChef.spec.ts(1 hunks)packages/timeline-state-resolver/src/integrations/sofieChef/index.ts(1 hunks)packages/timeline-state-resolver/src/integrations/tricaster/__tests__/index.spec.ts(2 hunks)packages/timeline-state-resolver/src/integrations/tricaster/__tests__/triCasterTimelineStateConverter.spec.ts(3 hunks)packages/timeline-state-resolver/src/integrations/vmix/vMixTimelineStateConverter.ts(1 hunks)packages/timeline-state-resolver/src/integrations/websocketClient/connection.ts(1 hunks)packages/timeline-state-resolver/src/lib.ts(1 hunks)packages/timeline-state-resolver/src/manifest.ts(2 hunks)packages/timeline-state-resolver/src/service/ConnectionManager.ts(1 hunks)packages/timeline-state-resolver/src/service/DeviceInstance.ts(2 hunks)packages/timeline-state-resolver/src/service/devices.ts(3 hunks)packages/timeline-state-resolver/src/service/stateHandler.ts(1 hunks)packages/timeline-state-resolver/tsconfig.build.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (14)
packages/timeline-state-resolver/src/integrations/tricaster/__tests__/index.spec.ts (1)
packages/timeline-state-resolver-types/src/integrations/tricaster.ts (1)
TriCasterMixEffect(87-90)
packages/timeline-state-resolver-types/src/index.ts (1)
packages/timeline-state-resolver-types/src/integrations/kairos.ts (1)
TimelineContentKairosAny(41-49)
packages/timeline-state-resolver/src/integrations/kairos/actions.ts (1)
packages/timeline-state-resolver-types/src/generated/kairos.ts (1)
KairosActionMethods(355-377)
packages/timeline-state-resolver/src/integrations/kairos/__tests__/diffState.spec.ts (5)
packages/timeline-state-resolver-types/src/mapping.ts (1)
Mappings(4-6)packages/timeline-state-resolver-types/src/generated/kairos.ts (1)
SomeMappingKairos(78-78)packages/timeline-state-resolver/src/__mocks__/objects.ts (1)
makeDeviceTimelineStateObject(6-38)packages/timeline-state-resolver/src/integrations/kairos/index.ts (1)
KairosCommandWithContext(20-20)packages/timeline-state-resolver/src/integrations/kairos/diffState.ts (1)
diffKairosStates(11-61)
packages/timeline-state-resolver/src/integrations/kairos/index.ts (7)
packages/timeline-state-resolver-api/src/device.ts (3)
CommandWithContext(21-35)Device(73-113)DeviceContextAPI(205-258)packages/timeline-state-resolver/src/integrations/kairos/commands.ts (2)
KairosCommandAny(22-32)sendCommand(128-401)packages/timeline-state-resolver-types/src/generated/kairos.ts (4)
KairosDeviceTypes(379-383)KairosActionMethods(355-377)KairosOptions(9-18)SomeMappingKairos(78-78)packages/timeline-state-resolver/src/integrations/kairos/stateBuilder.ts (2)
KairosDeviceState(53-126)KairosStateBuilder(128-409)packages/timeline-state-resolver/src/integrations/kairos/lib/kairosRamLoader.ts (1)
KairosRamLoader(14-111)packages/timeline-state-resolver/src/integrations/kairos/actions.ts (1)
getActions(14-182)packages/timeline-state-resolver/src/integrations/kairos/diffState.ts (1)
diffKairosStates(11-61)
packages/timeline-state-resolver/src/integrations/kairos/diffState.ts (5)
packages/timeline-state-resolver/src/integrations/kairos/stateBuilder.ts (1)
KairosDeviceState(53-126)packages/timeline-state-resolver-types/src/generated/kairos.ts (1)
SomeMappingKairos(78-78)packages/timeline-state-resolver/src/integrations/kairos/index.ts (1)
KairosCommandWithContext(20-20)packages/timeline-state-resolver/src/integrations/kairos/diffState/media-players.ts (2)
diffMediaPlayers(15-173)diffMediaImageStore(175-276)packages/timeline-state-resolver/src/integrations/kairos/diffState/lib.ts (2)
getAllKeysString(61-69)diffObject(9-27)
packages/timeline-state-resolver/src/integrations/kairos/diffState/media-players.ts (7)
packages/timeline-state-resolver/src/integrations/kairos/commands.ts (5)
KairosClipPlayerCommand(76-82)KairosRamRecPlayerCommand(104-110)KairosSoundPlayerCommand(120-126)KairosPlayerCommandMethod(83-102)KairosImageStoreCommand(112-118)packages/timeline-state-resolver/src/integrations/kairos/index.ts (1)
KairosCommandWithContext(20-20)packages/timeline-state-resolver/src/integrations/kairos/diffState/lib.ts (2)
getAllKeysString(61-69)diffObjectBoolean(36-54)packages/timeline-state-resolver/src/integrations/kairos/stateBuilder.ts (2)
KairosDeviceState(53-126)MappingOptions(410-413)packages/timeline-state-resolver-types/src/integrations/kairos.ts (1)
TimelineContentKairosPlayerState(166-206)packages/timeline-state-resolver-types/src/generated/kairos.ts (3)
MediaClipRef(181-184)MediaRamRecRef(203-206)MediaSoundRef(225-228)packages/timeline-state-resolver-types/src/superfly-timeline/api/resolvedTimeline.ts (1)
TimelineObjectInstance(84-106)
packages/timeline-state-resolver/src/integrations/kairos/lib/kairosRamLoader.ts (3)
packages/timeline-state-resolver-api/src/device.ts (1)
DeviceContextAPI(205-258)packages/timeline-state-resolver/src/integrations/kairos/stateBuilder.ts (1)
KairosDeviceState(53-126)packages/timeline-state-resolver-types/src/generated/kairos.ts (2)
MediaRamRecRef(203-206)MediaStillRef(192-195)
packages/timeline-state-resolver/src/service/DeviceInstance.ts (1)
packages/timeline-state-resolver/src/lib.ts (1)
cloneDeep(73-75)
packages/timeline-state-resolver/src/integrations/kairos/diffState/lib.ts (1)
packages/timeline-state-resolver/src/integrations/tricaster/triCasterStateDiffer.ts (1)
isEqual(541-546)
packages/timeline-state-resolver/src/integrations/tricaster/__tests__/triCasterTimelineStateConverter.spec.ts (1)
packages/timeline-state-resolver-types/src/integrations/tricaster.ts (1)
TriCasterMixEffect(87-90)
packages/timeline-state-resolver/src/integrations/kairos/__tests__/lib.ts (1)
packages/timeline-state-resolver/src/integrations/kairos/stateBuilder.ts (1)
KairosDeviceState(53-126)
packages/timeline-state-resolver-types/src/integrations/kairos.ts (1)
packages/timeline-state-resolver-types/src/generated/kairos.ts (5)
MediaClipRef(181-184)MediaRamRecRef(203-206)MediaStillRef(192-195)MediaImageRef(214-217)MediaSoundRef(225-228)
packages/timeline-state-resolver/src/integrations/kairos/commands.ts (4)
packages/timeline-state-resolver/src/integrations/kairos/index.ts (1)
sendCommand(125-136)packages/timeline-state-resolver/src/integrations/kairos/lib/kairosRamLoader.ts (1)
KairosRamLoader(14-111)packages/timeline-state-resolver/src/lib.ts (1)
assertNever(60-62)packages/timeline-state-resolver/src/integrations/tricaster/triCasterStateDiffer.ts (1)
isEqual(541-546)
🪛 GitHub Check: Lint (timeline-state-resolver)
packages/timeline-state-resolver/src/integrations/kairos/__tests__/diffState.spec.ts
[warning] 494-494:
Some tests seem to be commented
[warning] 46-46:
Test has no assertions
🔇 Additional comments (53)
packages/timeline-state-resolver/src/integrations/httpSend/__tests__/httpsend.spec.ts (1)
17-20: LGTM! Mock correctly updated for Got v11.The mock structure now correctly exports HTTP methods directly at the top level, aligning with Got v11's named export API. This change is consistent with the library upgrade and removes the unnecessary default wrapper.
packages/timeline-state-resolver/src/integrations/httpWatcher/__tests__/httpwatcher.spec.ts (1)
11-14: LGTM! Mock correctly updated for Got v11.The mock structure now correctly exports HTTP methods directly at the top level, consistent with the change in httpsend.spec.ts and aligned with Got v11's API.
packages/timeline-state-resolver/src/conductor.ts (1)
12-12: LGTM! Import updated for TypeScript 5.1 compatibility.The change from a namespace import to a default import aligns with TypeScript 5.1's improved ESM module resolution. The usage at line 368 remains correct.
Optionally verify that the installed
p-allversion supports default exports:#!/bin/bash # Check the installed p-all version and verify it exports a default function cat package.json | jq -r '.dependencies["p-all"] // .devDependencies["p-all"] // "not found"'packages/timeline-state-resolver/src/integrations/sofieChef/index.ts (1)
13-13: Verify that the default import works withWebSocket.Dataaccess.The import style changed from namespace import to default import. Line 274 accesses
WebSocket.Data, which requires the default export to include namespace properties. Ensure this works correctly with the current 'ws' library version and TypeScript 5.1.Run the following script to verify the 'ws' library version:
#!/bin/bash # Check the version of 'ws' library in package.json files fd -t f 'package.json' --exec jq -r 'select(.dependencies.ws != null or .devDependencies.ws != null) | "File: \(input_filename)\nws version: \(.dependencies.ws // .devDependencies.ws)"'Additionally, search for documentation on whether the 'ws' library version in use supports default imports with namespace property access:
Does the ws npm package support default import with namespace property access like WebSocket.Data in TypeScript?packages/timeline-state-resolver/src/integrations/sofieChef/__tests__/sofieChef.spec.ts (1)
4-4: LGTM! Import style updated consistently with mock.The WebSocket import has been updated from a namespace import to a default import, aligning with the mock's new default export style.
packages/timeline-state-resolver/src/integrations/websocketClient/connection.ts (1)
1-1: LGTM! Import style updated consistently.The WebSocket import has been updated from a namespace import to a default import, consistent with the broader refactoring across the codebase.
packages/timeline-state-resolver/src/__mocks__/ws.ts (1)
135-135: LGTM! Mock export updated to support default imports.The mock export has been changed from CommonJS export-equals to an ES module default export, enabling test files to use default import syntax consistently.
packages/timeline-state-resolver/src/integrations/pharos/__tests__/pharosAPI.spec.ts (1)
21-21: LGTM! Test import updated consistently with mock.The WebSocket import has been updated to use default import syntax, aligning with the mock's new default export pattern.
packages/timeline-state-resolver/src/integrations/pharos/connection.ts (1)
1-1: TypeScript configuration already supports this default import.The
esModuleInterop: truesetting in tsconfig.build.json properly enables default imports from CommonJS modules like ws v8.18.0. No configuration changes are required.packages/timeline-state-resolver/.eslintrc.json (1)
14-18: LGTM with temporary workaround.The ESLint override for
kairos-connectionis well-documented as a temporary measure. The inline comments clearly explain the issue and expected resolution path.packages/timeline-state-resolver-types/.eslintrc.json (1)
7-11: LGTM with temporary workaround.The ESLint override for
kairos-libfollows the same well-documented pattern as thekairos-connectionoverride. The temporary nature and rationale are clearly explained.packages/timeline-state-resolver/src/service/stateHandler.ts (1)
163-165: LGTM!The
getCurrentState()getter provides clean, read-only access to the current device state. The implementation is straightforward and uses safe optional chaining.package.json (1)
56-56: Verify TypeScript 5.1 upgrade compatibility.The upgrade from TypeScript ~4.9.5 to ~5.1 is a major version jump. Ensure that:
- All TypeScript-related dependencies are compatible with TS 5.1, including
@sofie-automation/code-standard-preset- The codebase compiles without new type errors after building with TS 5.1
- The CI/CD pipeline (lint, build, and test jobs) passes successfully on all branches
- Any breaking changes in TS 5.x (decorator metadata, module resolution behavior) have been addressed, though the codebase already sets
moduleResolution: "node16"which is appropriate for TS5packages/timeline-state-resolver/package.json (1)
106-106: [email protected] is a valid, secure dependency.The package is published on npm with proper versioning, active maintenance (released November 2025), MIT license, and includes integrity signatures and SLSA provenance attestations. No known vulnerabilities detected in the registry.
packages/timeline-state-resolver-types/tsconfig.build.json (1)
12-15: LGTM!The
moduleResolution: "node16"andesModuleInterop: truesettings are appropriate for modern Node.js module resolution and proper interop with CommonJS modules like the newkairos-connectiondependency.packages/timeline-state-resolver/src/lib.ts (1)
11-17: LGTM!The
Complete<T>utility type is a useful pattern for ensuring exhaustive property handling during type conversions. The implementation correctly distinguishes between originally-required properties and originally-optional properties (which becomeT[P] | undefined).packages/timeline-state-resolver/src/integrations/kairos/diffState/media-players.ts (3)
15-173: Well-structured media player diffing logic.The implementation handles the key scenarios correctly:
- Player removal with configurable clear-on-stop behavior
- Seek position tolerance (100ms) to avoid unnecessary jitter
- Preliminary command timing for play operations
- Proper state defaulting and frame-based position calculation
278-287: LGTM!The helper types cleanly separate the outer state wrapper from the inner computed state used for diffing.
309-338: LGTM!The
getClipPlayerStatehelper correctly derives the computed state for diffing:
- Computes
absoluteStartTimefrom instance start and seek offset- Properly handles the
repeatmode where seek position is irrelevant- Clears
absoluteStartTimewhen paused since it's not applicablepackages/timeline-state-resolver/src/integrations/__tests__/testlib.ts (1)
23-23: LGTM!The addition of the
setModifiedStatemock aligns with the new DeviceContextAPI method and properly supports the Kairos integration testing requirements.packages/timeline-state-resolver/src/integrations/tricaster/__tests__/index.spec.ts (2)
10-10: LGTM!Adding the
TriCasterMixEffectimport improves type safety in the test file.
86-91: LGTM!Replacing the
as anycast withas TriCasterMixEffectprovides better type safety without changing test behavior.packages/timeline-state-resolver/src/integrations/tricaster/__tests__/triCasterTimelineStateConverter.spec.ts (2)
11-11: LGTM!Adding the
TriCasterMixEffectimport improves type safety in the test file.
120-120: LGTM!Replacing the
as anycasts withas TriCasterMixEffectprovides better type safety without changing test behavior.Also applies to: 349-349
packages/timeline-state-resolver-types/src/index.ts (2)
26-26: LGTM!The Kairos integration is properly imported and exported, following the established pattern for device integrations.
Also applies to: 34-34
126-126: LGTM!The
TimelineContentKairosAnytype is correctly added to theTSRTimelineContentunion, making Kairos content available throughout the type system.packages/timeline-state-resolver/src/integrations/kairos/$schemas/options.json (1)
6-17: LGTM!The host and port configuration properties are properly defined with appropriate descriptions, UI titles, and default values.
packages/timeline-state-resolver/src/integrations/kairos/__tests__/lib.ts (1)
1-13: LGTM!The
EMPTY_STATEtest fixture is well-structured and provides a clean baseline for Kairos integration tests.packages/timeline-state-resolver/src/integrations/kairos/diffState/lib.ts (1)
61-69: LGTM!The
getAllKeysStringfunction correctly aggregates unique keys from both objects while filtering out invalid entries.packages/timeline-state-resolver/src/service/ConnectionManager.ts (1)
452-452: LGTM!The Kairos device type is properly routed through the service-implemented device path, consistent with similar device integrations.
packages/timeline-state-resolver/src/integrations/kairos/__tests__/stateBuilder.spec.ts (1)
1-87: LGTM!The test suite is well-structured and clearly validates the KairosStateBuilder behavior for empty timelines and scene layer assignments.
packages/timeline-state-resolver/src/manifest.ts (2)
29-31: LGTM!The Kairos schema imports follow the established pattern for device integrations.
116-121: LGTM!The Kairos device manifest entry is correctly structured and consistent with other device types in the manifest.
packages/timeline-state-resolver/src/service/devices.ts (2)
23-23: LGTM!The KairosDevice import is properly placed with other device imports.
46-46: LGTM!The Kairos device registration is correctly configured with sequential execution mode, which is appropriate for the Kairos device type.
Also applies to: 80-85
packages/timeline-state-resolver/src/integrations/kairos/actions.ts (2)
154-157: Verify validation logic for macroPath.The validation checks if
macroPathis falsy or not an Array, but doesn't reject empty arrays. If an empty array is invalid (which seems likely given a macro needs a path), add a length check.-if (!payload.macroPath || !Array.isArray(payload.macroPath)) { - return { result: ActionExecutionResultCode.Error, message: 'Invalid payload: macroPath is not an Array' } +if (!payload.macroPath || !Array.isArray(payload.macroPath) || payload.macroPath.length === 0) { + return { result: ActionExecutionResultCode.Error, message: 'Invalid payload: macroPath must be a non-empty Array' } }Additionally, verify whether
MacroStop(Line 165) should have similar validation.
14-181: LGTM overall!The action handlers are well-structured and consistently return standardized results. The use of
satisfies KairosActionMethodsprovides good type safety.packages/timeline-state-resolver/src/integrations/kairos/__tests__/diffState.spec.ts (2)
46-48: Test has assertions (false positive).The static analysis warning about no assertions is incorrect—the
compareStateshelper function contains the assertion at Line 507. The test is valid.
1-493: LGTM!The test suite comprehensively covers diffState functionality for scene layers and clip player lifecycle (play, pause, resume, stop, late start with seek). The tests are well-structured and use proper fixtures.
packages/timeline-state-resolver/src/integrations/kairos/index.ts (1)
1-141: LGTM!The KairosDevice implementation properly manages the device lifecycle, connection state, and command flow. The async connection initialization (Line 61) and error handling throughout are appropriate.
packages/timeline-state-resolver/src/integrations/kairos/$schemas/mappings.json (1)
1-218: LGTM!The mapping schemas are comprehensive and well-structured with appropriate UI hints, validation constraints, and defaults. The schema correctly omits the framerate field for image-store mappings, as stills don't have frame rates.
packages/timeline-state-resolver/src/integrations/kairos/lib/kairosRamLoader.ts (3)
28-35: Verify error handling strategy.When the clip reference is invalid, an error is logged but the method returns normally without throwing. This means callers won't know the RAM load failed. Verify this is the intended behavior.
If callers need to know about failures, consider throwing the error instead:
if (!isRef(clipRef) || (clipRef.realm !== 'media-still' && clipRef.realm !== 'media-ramrec')) { - this.context.logger.error( - 'KairosRamLoader', - new Error(`KairosRamLoader: Unsupported clip reference for RAM loading: ${JSON.stringify(source)}`) - ) - return + throw new Error(`KairosRamLoader: Unsupported clip reference for RAM loading: ${JSON.stringify(source)}`) }
19-89: LGTM overall!The RAM loading logic is well-designed with proper debouncing, async loading to avoid blocking commands, and state modification callbacks to trigger re-diff after load completion.
91-111: LGTM!The polling logic for load status is straightforward with appropriate timeout and retry interval.
packages/timeline-state-resolver/src/integrations/kairos/$schemas/actions.json (1)
1-691: Well-structured action definitions.The schema comprehensively covers Kairos operations with consistent patterns. The
additionalProperties: falseissue noted above applies to all result schemas using theallOfcomposition pattern (approximately 15 occurrences).packages/timeline-state-resolver/src/integrations/kairos/stateBuilder.ts (2)
128-213: Well-designed state builder pattern.The
KairosStateBuildercleanly separates state construction with per-type handlers and uses a consistent merge strategy. The early returns for invalid/empty names provide good defensive coding.
415-428: LGTM!The lookahead patching correctly pauses playback and ensures a consistent seek position for preloading.
packages/timeline-state-resolver-types/src/integrations/kairos.ts (1)
165-206: Well-documented generic player state interface.The
TimelineContentKairosPlayerState<TClip>interface has excellent JSDoc documentation explaining each property's behavior. The generic approach enables type-safe reuse across clip, ram-rec, and sound players.packages/timeline-state-resolver/src/integrations/kairos/$schemas/lib/refs.json (2)
359-385: Hardcoded player/recorder limits may need future updates.The enums for
ClipPlayerRef(CP1-CP2),RamRecorderRef(RR1-RR8), andImageStoreRef(IS1-IS8) appear to reflect current Kairos hardware limits. If future hardware revisions support more, these schemas will need updates.Is there documentation or a way to programmatically determine the available players/recorders from a Kairos device to make this more flexible?
16-33: Well-structured reference schemas.The schemas follow a consistent pattern with
realmconst discriminators and appropriate required fields. The use of$reffor path types promotes reuse.packages/timeline-state-resolver/src/integrations/kairos/commands.ts (3)
1-32: Imports and type union look good.The command union type is well-structured and includes all defined command interfaces. The use of
assertNeverfrom the local lib ensures exhaustive type checking in switch statements.
149-204: Scene-layer RAM loading logic is well-structured.The pattern of extracting source properties, sending them first, then ensuring RAM is loaded with appropriate state callbacks is consistent and handles the async loading workflow correctly.
397-400: Good exhaustive error handling.The combination of
assertNever(command)for type-level exhaustiveness and the explicitthrowensures both compile-time safety and runtime protection against unhandled command types.
packages/timeline-state-resolver/src/integrations/kairos/$schemas/actions.json
Show resolved
Hide resolved
packages/timeline-state-resolver/src/integrations/kairos/$schemas/options.json
Outdated
Show resolved
Hide resolved
Julusian
force-pushed
the
feat/kairos-connection
branch
from
4257c76 to
24c78cd
Compare
About the Contributor
This pull request is posted on behalf of NRK.
Type of Contribution
This is a: Feature
Current Behavior
This is a minimal kairos device type. It is not fully built out, but it is enough to do some basic usage
New Behavior
Testing Instructions
Other Information
Status
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.