Feat: Kairos commands priority#441
Merged
Merged
Conversation
Monitor Kairos status, to ensure that all resources mentioned in mappings and state exists on the Kairos.
# Conflicts: # packages/timeline-state-resolver/src/integrations/kairos/index.ts
WalkthroughThe PR updates Kairos dependencies, adds KairosApplicationMonitor for runtime health checks, implements temporal-priority-based command ordering, refactors Kairos device-state types into exported aliases, renames a sound command type to audio, extends schemas with temporalPriority and monitorState, and adds tests and minor type/validation tweaks. Changes
Sequence Diagram(s)sequenceDiagram
participant Timeline
participant KairosDevice
participant KairosApplicationMonitor
participant KairosConnection
participant DiffEngine
participant TemporalPrioritizer
participant CommandQueue
Timeline->>KairosDevice: convertTimelineStateToDeviceState()
KairosDevice->>KairosDevice: build device state (StateBuilder)
KairosDevice->>KairosApplicationMonitor: updateMappings & updateDeviceState
Timeline->>KairosDevice: diffStates(old, new)
KairosDevice->>DiffEngine: diffKairosStates()
DiffEngine-->>KairosDevice: commands (unordered)
KairosDevice->>TemporalPrioritizer: temporalPriorityOrderCommands(commands)
TemporalPrioritizer->>TemporalPrioritizer: buildDependencyGraph()
TemporalPrioritizer->>TemporalPrioritizer: propagate temporalPriority
TemporalPrioritizer-->>KairosDevice: ordered commands
KairosDevice->>CommandQueue: return prioritized commands
sequenceDiagram
participant KairosDevice
participant KairosApplicationMonitor
participant KairosConnection
participant StatusEmitter
KairosDevice->>KairosApplicationMonitor: init (if monitorState enabled)
KairosApplicationMonitor->>KairosApplicationMonitor: schedule checks
loop monitor interval
KairosApplicationMonitor->>KairosApplicationMonitor: collect aware refs (mappings + deviceState)
KairosApplicationMonitor->>KairosConnection: exists(ref)
KairosConnection-->>KairosApplicationMonitor: exists result / error
alt status changed
KairosApplicationMonitor->>StatusEmitter: emit statusChanged
StatusEmitter-->>KairosDevice: receive status
KairosDevice->>KairosDevice: refresh connection state
end
end
KairosDevice->>KairosApplicationMonitor: terminate()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
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. ✨ 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 |
nytamin
force-pushed
the
feat/kairos-tally-priority
branch
from
fb6e1b1 to
19b1a7a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/timeline-state-resolver/src/integrations/kairos/kairos-application-monitor.ts (1)
338-353: Consider removing stale commented interface block.This commented-out block is dead code and adds maintenance noise in a new file.
🧹 Optional cleanup
-// export interface Aware { -// sceneLayers: Set<SceneLayerRef> -// sceneSnapshot: Set<SceneSnapshotRef> -// sceneLayerEffects: Set<SceneLayerEffectRef> - -// auxes: Set<AuxRef> -// macros: Set<MacroRef> -// anySources: Set<AnySourceRef> - -// soundPlayers: Set<AudioPlayerRef> - -// mediaClips: Set<MediaClipRef> -// mediaStills: Set<MediaStillRef | MediaImageRef> -// mediaRamRecs: Set<MediaRamRecRef> -// mediaSounds: Set<MediaSoundRef> -// }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/integrations/kairos/kairos-application-monitor.ts` around lines 338 - 353, Remove the stale commented-out Aware interface block (the commented lines starting with "export interface Aware" and its properties like sceneLayers, sceneSnapshot, sceneLayerEffects, auxes, macros, anySources, soundPlayers, mediaClips, mediaStills, mediaRamRecs, mediaSounds) from kairos-application-monitor.ts to eliminate dead code and reduce maintenance noise; if the interface is needed later, restore it from VCS rather than keeping it commented in the file.packages/timeline-state-resolver/src/integrations/kairos/index.ts (1)
93-104: Side-effecting state notifications insideconvertTimelineStateToDeviceState.Calling
updateMappingsandupdateDeviceStateon the monitor inside a method whose name implies a pure conversion is a bit surprising. This is a pragmatic placement (it's the only point where both mappings and device state are available), but worth a brief inline comment noting why the monitor is updated here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/integrations/kairos/index.ts` around lines 93 - 104, The method convertTimelineStateToDeviceState currently performs side effects by calling this._kairosApplicationMonitor?.updateMappings(...) and updateDeviceState(...) even though its name suggests a pure conversion; add a brief inline comment immediately above those monitor calls (near KairosStateBuilder.fromTimeline and the two updateMappings/updateDeviceState lines) stating that this is intentionally done here because this is the only place where both mappings and the derived KairosDeviceState are available and the monitor must be notified, and mark that this method therefore performs a deliberate side-effect for clarity.packages/timeline-state-resolver/src/integrations/kairos/__tests__/temporal-priority.spec.ts (2)
33-33: Local variableMappingsshadows the imported typeMappings.The local
const Mappingson this line (and similarly at Lines 214, 411, 605) shadows theMappingstype imported on Line 5. TypeScript allows this because the import is type-only, but it can confuse readers. Consider renaming the local variables (e.g.,testMappingsorkaMappings) for clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/integrations/kairos/__tests__/temporal-priority.spec.ts` at line 33, Rename the local constant `Mappings` (e.g., to `testMappings` or `kairosMappings`) so it no longer shadows the imported type `Mappings`; update its declaration `const Mappings: Mappings<SomeMappingKairos> = {` and every use of that local variable in this test (and the other occurrences you noted at the same file: the declarations around lines 214, 411, 605) to the new name while keeping the type annotation `Mappings<SomeMappingKairos>` unchanged.
712-751: In-place mutation of sharedMappingsobject is fragile.The test mutates
Mappings.scene2Content.options.temporalPrioritydirectly and cleans up withdeleteat the end of each scoped block. This is fine today since Jest runs synchronously within a singletest(), but it's brittle if someone later restructures or adds assertions between blocks without realizing the shared mutation pattern. A safer approach would be to spread/clone the mappings per sub-case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/integrations/kairos/__tests__/temporal-priority.spec.ts` around lines 712 - 751, The test mutates the shared Mappings object via Mappings.scene2Content.options.temporalPriority and then deletes it—make a cloned per-case mapping instead to avoid fragile shared state: create a shallow/deep clone (e.g., structuredClone or JSON deep clone) into a local variable (e.g., localMappings), set localMappings.scene2Content.options.temporalPriority = 1000, call temporalPriorityOrderCommands(state, localMappings, diffKairosStates(EMPTY_STATE, state, localMappings)), run the assertions against the returned commands, and remove the in-place delete; do the same pattern for other sub-cases so each uses its own cloned Mappings instance.
🤖 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/src/integrations/kairos/__tests__/temporal-priority.spec.ts`:
- Line 787: Fix the typo in the test comment that reads "The the ones with no
temporalPriority" by changing it to "Then the ones with no temporalPriority";
locate the string in the temporal-priority.spec test file (search for the exact
phrase "The the ones with no temporalPriority") and update the comment text so
the sentence reads correctly.
In
`@packages/timeline-state-resolver/src/integrations/kairos/kairos-application-monitor.ts`:
- Around line 241-243: The issue is a malformed message string that inserts an
extra ')' into the monitor issue text; locate the code in
kairos-application-monitor.ts where the ResponseError branch pushes to the
issues array (the line using issues.push(`${description}) not found on the
Kairos.`)) and remove the stray parenthesis so the pushed string reads
`${description} not found on the Kairos.` to fix the noisy status text.
In
`@packages/timeline-state-resolver/src/integrations/kairos/temporal-priority.ts`:
- Around line 244-245: The branch handling media-player commands always maps
media-player:do to refClipPlayer, causing ram-rec-player and sound-player
actions to get the wrong temporal priority; update the branch that checks
command.command.type === 'media-player:do' to inspect command.command.playerType
and dispatch to the correct reference function (e.g., use refRamRecPlayer for
playerType 'ram-rec-player', refSoundPlayer for 'sound-player', and
refClipPlayer for clip players), ensuring the code paths that call
refClipPlayer, refRamRecPlayer, and refSoundPlayer use the correct playerId from
command.command.playerId.
---
Nitpick comments:
In
`@packages/timeline-state-resolver/src/integrations/kairos/__tests__/temporal-priority.spec.ts`:
- Line 33: Rename the local constant `Mappings` (e.g., to `testMappings` or
`kairosMappings`) so it no longer shadows the imported type `Mappings`; update
its declaration `const Mappings: Mappings<SomeMappingKairos> = {` and every use
of that local variable in this test (and the other occurrences you noted at the
same file: the declarations around lines 214, 411, 605) to the new name while
keeping the type annotation `Mappings<SomeMappingKairos>` unchanged.
- Around line 712-751: The test mutates the shared Mappings object via
Mappings.scene2Content.options.temporalPriority and then deletes it—make a
cloned per-case mapping instead to avoid fragile shared state: create a
shallow/deep clone (e.g., structuredClone or JSON deep clone) into a local
variable (e.g., localMappings), set
localMappings.scene2Content.options.temporalPriority = 1000, call
temporalPriorityOrderCommands(state, localMappings,
diffKairosStates(EMPTY_STATE, state, localMappings)), run the assertions against
the returned commands, and remove the in-place delete; do the same pattern for
other sub-cases so each uses its own cloned Mappings instance.
In `@packages/timeline-state-resolver/src/integrations/kairos/index.ts`:
- Around line 93-104: The method convertTimelineStateToDeviceState currently
performs side effects by calling
this._kairosApplicationMonitor?.updateMappings(...) and updateDeviceState(...)
even though its name suggests a pure conversion; add a brief inline comment
immediately above those monitor calls (near KairosStateBuilder.fromTimeline and
the two updateMappings/updateDeviceState lines) stating that this is
intentionally done here because this is the only place where both mappings and
the derived KairosDeviceState are available and the monitor must be notified,
and mark that this method therefore performs a deliberate side-effect for
clarity.
In
`@packages/timeline-state-resolver/src/integrations/kairos/kairos-application-monitor.ts`:
- Around line 338-353: Remove the stale commented-out Aware interface block (the
commented lines starting with "export interface Aware" and its properties like
sceneLayers, sceneSnapshot, sceneLayerEffects, auxes, macros, anySources,
soundPlayers, mediaClips, mediaStills, mediaRamRecs, mediaSounds) from
kairos-application-monitor.ts to eliminate dead code and reduce maintenance
noise; if the interface is needed later, restore it from VCS rather than keeping
it commented in the file.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/timeline-state-resolver-types/src/generated/kairos.tsis excluded by!**/generated/**yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
packages/timeline-state-resolver-types/package.jsonpackages/timeline-state-resolver/package.jsonpackages/timeline-state-resolver/src/integrations/kairos/$schemas/mappings.jsonpackages/timeline-state-resolver/src/integrations/kairos/$schemas/options.jsonpackages/timeline-state-resolver/src/integrations/kairos/__tests__/diffState.spec.tspackages/timeline-state-resolver/src/integrations/kairos/__tests__/temporal-priority.spec.tspackages/timeline-state-resolver/src/integrations/kairos/commands.tspackages/timeline-state-resolver/src/integrations/kairos/diffState.tspackages/timeline-state-resolver/src/integrations/kairos/diffState/media-players.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/kairos/stateBuilder.tspackages/timeline-state-resolver/src/integrations/kairos/temporal-priority.tspackages/timeline-state-resolver/src/service/DeviceInstance.ts
Comment on lines
+241
to
+243
| if (e instanceof ResponseError) { | ||
| issues.push(`${description}) not found on the Kairos.`) | ||
| } else { |
There was a problem hiding this comment.
Fix malformed monitor issue message formatting.
Line 242 currently adds an extra ) which produces noisy status text.
💡 Proposed fix
- issues.push(`${description}) not found on the Kairos.`)
+ issues.push(`${description} not found on the Kairos.`)📝 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
| if (e instanceof ResponseError) { | |
| issues.push(`${description}) not found on the Kairos.`) | |
| } else { | |
| if (e instanceof ResponseError) { | |
| issues.push(`${description} not found on the Kairos.`) | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/timeline-state-resolver/src/integrations/kairos/kairos-application-monitor.ts`
around lines 241 - 243, The issue is a malformed message string that inserts an
extra ')' into the monitor issue text; locate the code in
kairos-application-monitor.ts where the ResponseError branch pushes to the
issues array (the line using issues.push(`${description}) not found on the
Kairos.`)) and remove the stray parenthesis so the pushed string reads
`${description} not found on the Kairos.` to fix the noisy status text.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/timeline-state-resolver/src/integrations/kairos/__tests__/temporal-priority.spec.ts (1)
712-800: Consider isolating mapping mutations to avoid test pollution on failure.The code mutates
Mappings.scene2Content.options.temporalPrioritydirectly and relies ondeletefor cleanup. If an assertion fails before cleanup, subsequent test blocks would inherit the polluted state. Consider creating a fresh mappings copy for each block.♻️ Suggested approach using spread for isolated test blocks
{ // Test with high temporalPriority on scene2 const testMappings: Mappings<MappingKairosSceneLayer> = { ...Mappings, scene2Content: { ...Mappings.scene2Content, options: { ...Mappings.scene2Content.options, temporalPriority: 1000 }, }, } const commands = temporalPriorityOrderCommands( state, testMappings, diffKairosStates(EMPTY_STATE, state, testMappings) ) // ... assertions ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/timeline-state-resolver/src/integrations/kairos/__tests__/temporal-priority.spec.ts` around lines 712 - 800, The test mutates global Mappings (e.g. Mappings.scene2Content.options.temporalPriority / scene3Content) and relies on delete for cleanup which can leak state on assertion failures; instead create isolated copies (e.g. const testMappings = { ...Mappings, scene2Content: { ...Mappings.scene2Content, options: { ...Mappings.scene2Content.options, temporalPriority: 1000 } } } ) and pass testMappings into temporalPriorityOrderCommands and diffKairosStates (use EMPTY_STATE and state as before) so each block runs with its own mapping snapshot and no deletes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/timeline-state-resolver/src/integrations/kairos/__tests__/temporal-priority.spec.ts`:
- Around line 712-800: The test mutates global Mappings (e.g.
Mappings.scene2Content.options.temporalPriority / scene3Content) and relies on
delete for cleanup which can leak state on assertion failures; instead create
isolated copies (e.g. const testMappings = { ...Mappings, scene2Content: {
...Mappings.scene2Content, options: { ...Mappings.scene2Content.options,
temporalPriority: 1000 } } } ) and pass testMappings into
temporalPriorityOrderCommands and diffKairosStates (use EMPTY_STATE and state as
before) so each block runs with its own mapping snapshot and no deletes are
needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/timeline-state-resolver/src/integrations/kairos/__tests__/temporal-priority.spec.tspackages/timeline-state-resolver/src/integrations/kairos/temporal-priority.ts
jstarpl
approved these changes
Mar 3, 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 NRK.
Type of Contribution
This is a: Feature
Current Behavior
Commands sent to the Kairos vision mixer are sent in a "best effort" order.
In certain scenarios when updating several things at once, visual artifacts might show because "inner content" is not ready yet.
An example is when switching to a Split scene with multiple content boxes, the boxes might display the wrong content for a frame or two, because we switched to the Split before the boxes in it where ready.
New Behavior
We now build a "dependency graph" using the known state and mappings, in order to determine which commands should be sent in what order to ensure that sources are handled first.
Testing Instructions
This PR affects the Kairos device only.
Test by switching to split views on a Kairos.
Other Information
Note: This is based on #425
Status
Overview
This PR introduces command priority ordering for the Kairos vision mixer integration, eliminating transient visual artifacts that occurred when multiple commands were sent simultaneously in arbitrary order. The implementation builds a dependency graph from the current device state and mappings to ensure sources and content are configured before layout and split changes are applied.
Key Changes
Dependency Updates
kairos-libbumped from 0.2.3 to 1.0.0 inpackages/timeline-state-resolver-types/package.jsonkairos-connectionupdated from 0.2.3 to ^1.0.0 inpackages/timeline-state-resolver/package.jsonto allow 1.x releasesNew Modules
temporal-priority.ts – Core ordering logic
temporalPriorityOrderCommands(): computes dependency graph and returns commands ordered by temporal prioritybuildDependencyGraph(): constructs dependency graph from Kairos state and mappingsorderCommandsByTemporalPriority(): sorts commands using computed prioritieskairos-application-monitor.ts – Health monitoring
KairosApplicationMonitorclass: periodically verifies Kairos integration health by checking existence of referenced scenes, clips, auxiliaries, inputs, and other items via TTL-based awareness trackingstatusChangedanderroreventsSchema Changes
mappings.json
temporalPriorityproperty (integer, default 0) to scene, scene-layer, aux, clip-player, ram-rec-player, image-store, and sound-player mappings with UI metadataoptions.json
monitorStateboolean property (default false) to enable application monitoringType System Refactoring
commands.ts
KairosCommandWithContext = CommandWithContext<KairosCommandAny, string>KairosSoundPlayerCommandtoKairosAudioPlayerCommand(updated throughout codebase)KairosCommandAnyunion to reference new audio command typestateBuilder.ts
KairosDeviceStatewith new exported type aliases for each device component:KairosDeviceStateScenes,KairosDeviceStateSceneSnapshots,KairosDeviceStateSceneLayers,KairosDeviceStateAux,KairosDeviceStateMacros,KairosDeviceStateClipPlayers,KairosDeviceStateRamRecPlayers,KairosDeviceStateImageStores,KairosDeviceStateSoundPlayersfromTimeline()signature to useMappings<SomeMappingKairos>for better type specificityIntegration Updates
index.ts (KairosDevice)
KairosApplicationMonitorwhenoptions.monitorStateis enabledinit()andterminate()getStatus()to report monitor health statusdiffStates()to apply temporal priority ordering viatemporalPriorityOrderCommands()after computing diffsconvertTimelineStateToDeviceState()to notify monitor of state changesLibrary Updates
kairosRamLoader.ts
media-players.ts
KairosSoundPlayerCommandtoKairosAudioPlayerCommandTest Coverage
temporal-priority.spec.ts – New comprehensive test suite (801 lines)
temporalPrioritymappings on command sequencesdiffState.spec.ts
KairosCommandWithContextfrom commands moduleOther Changes
CommandWithContext<any, any>toCommandWithContext<unknown, unknown>Impact
monitorStateoption