feat: Allow actions to recue the next part#1797
Draft
PeterC89 wants to merge 3 commits into
Draft
Conversation
WalkthroughThe change adds snapshot-based recue support for the next playout part, exposes it through action contexts, restores the snapshot during execution, and reselects the part afterward so next-part callbacks run again. ChangesRecue Next Part
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/job-worker/src/blueprints/context/adlibActions.ts`:
- Around line 239-249: Update recueNextPart in
packages/job-worker/src/blueprints/context/adlibActions.ts at lines 239-249 to
set forceRegenerateTimeline to true after restoring the next part state. In
packages/job-worker/src/blueprints/__tests__/context-adlibActions.test.ts at
lines 146-158, add an assertion that context.forceRegenerateTimeline is true.
In `@packages/job-worker/src/playout/adlibAction.ts`:
- Around line 321-330: Replace setNextPartFromPart with setNextPart in the
recueAfterExecute branch of
packages/job-worker/src/playout/adlibAction.ts#L321-L330, passing
nextPartInstance directly, and update the import at
packages/job-worker/src/playout/adlibAction.ts#L36-L36. In
packages/job-worker/src/playout/__tests__/playout-executeAction.test.ts#L206-L227,
assert setNextPartMock is called once; at `#L13-L17` mock setNextPart from
SetNextApi, and at `#L49` clear setNextPartMock in beforeEach.
In
`@packages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.ts`:
- Around line 237-252: Update snapshotRestore to preserve tracking for current
piece instances absent from the snapshot: before restoring snapshot entries,
mark those existing IDs in pieceInstancesImpl as null instead of clearing the
map, then apply the snapshot entries as currently done. Also set
this.#partInstanceHasChanges = true so the reverted state is persisted.
- Around line 42-44: The rescue snapshot map in PlayoutPartInstanceModelImpl
must not remain process-local without lifecycle management. Persist each rescue
snapshot with its associated part instance, or add guaranteed cleanup whenever
the next-part state changes so stale entries are evicted; ensure rescue handling
can restore state after worker handoff or restart.
🪄 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: 5446f1e8-7a5d-4524-ab77-8604411a626c
📒 Files selected for processing (9)
packages/blueprints-integration/src/context/playoutActionContext.tspackages/job-worker/src/blueprints/__tests__/context-adlibActions.test.tspackages/job-worker/src/blueprints/context/adlibActions.tspackages/job-worker/src/playout/__tests__/playout-executeAction.test.tspackages/job-worker/src/playout/adlibAction.tspackages/job-worker/src/playout/model/PlayoutPartInstanceModel.tspackages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.tspackages/job-worker/src/playout/model/implementation/__tests__/PlayoutPartInstanceModelImpl.spec.tspackages/job-worker/src/playout/setNext.ts
Comment on lines
+239
to
+249
| async recueNextPart(): Promise<void> { | ||
| const nextPartInstance = this._playoutModel.nextPartInstance | ||
| if (!nextPartInstance) { | ||
| throw new Error('Cannot recue next part when no next part instance is set') | ||
| } | ||
|
|
||
| nextPartInstance.recueNextPart() | ||
| this.partAndPieceInstanceService.nextPartState = ActionPartChange.NONE | ||
| this.recueAfterExecute = true | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Force timeline regeneration to apply the recued state.
Since recueNextPart sets nextPartState to NONE, applyAnyExecutionSideEffects will skip generating the timeline unless explicitly forced. The timeline must be regenerated so that the playout gateway receives the restored piece states.
packages/job-worker/src/blueprints/context/adlibActions.ts#L239-L249: Setthis.forceRegenerateTimeline = trueat the end of the method.packages/job-worker/src/blueprints/__tests__/context-adlibActions.test.ts#L146-L158: Addexpect(context.forceRegenerateTimeline).toBe(true)to the test assertions.
📍 Affects 2 files
packages/job-worker/src/blueprints/context/adlibActions.ts#L239-L249(this comment)packages/job-worker/src/blueprints/__tests__/context-adlibActions.test.ts#L146-L158
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/job-worker/src/blueprints/context/adlibActions.ts` around lines 239
- 249, Update recueNextPart in
packages/job-worker/src/blueprints/context/adlibActions.ts at lines 239-249 to
set forceRegenerateTimeline to true after restoring the next part state. In
packages/job-worker/src/blueprints/__tests__/context-adlibActions.test.ts at
lines 146-158, add an assertion that context.forceRegenerateTimeline is true.
Comment on lines
+321
to
+330
| if (actionContext.recueAfterExecute) { | ||
| actionContext.recueAfterExecute = false | ||
| const nextPartInstance = playoutModel.nextPartInstance | ||
| if (!nextPartInstance) { | ||
| throw new Error('Cannot recue next part when no next part instance is set') | ||
| } | ||
|
|
||
| await setNextPartFromPart(context, playoutModel, nextPartInstance.partInstance, true) | ||
| } | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
Pass nextPartInstance directly to setNextPart to preserve in-memory changes.
setNextPartFromPart handles DBPartInstance by querying PieceInstances directly from the database. Because the playoutModel is not yet saved during action execution, this fetches stale pre-action data, entirely discarding the snapshot restoration performed by recueNextPart(). Passing the PlayoutPartInstanceModel directly to setNextPart preserves the in-memory changes.
packages/job-worker/src/playout/adlibAction.ts#L321-L330: Useawait setNextPart(context, playoutModel, nextPartInstance, true)instead ofsetNextPartFromPart.packages/job-worker/src/playout/adlibAction.ts#L36-L36: ImportsetNextPartinstead ofsetNextPartFromPart.packages/job-worker/src/playout/__tests__/playout-executeAction.test.ts#L206-L227: Update the test to assertexpect(setNextPartMock).toHaveBeenCalledTimes(1).packages/job-worker/src/playout/__tests__/playout-executeAction.test.ts#L13-L17: Change the spy to mocksetNextPartfromSetNextApiinstead ofsetNextPartFromPart.packages/job-worker/src/playout/__tests__/playout-executeAction.test.ts#L49-L49: ClearsetNextPartMockinstead ofsetNextPartFromPartMockin thebeforeEachblock.
📍 Affects 2 files
packages/job-worker/src/playout/adlibAction.ts#L321-L330(this comment)packages/job-worker/src/playout/adlibAction.ts#L36-L36packages/job-worker/src/playout/__tests__/playout-executeAction.test.ts#L206-L227packages/job-worker/src/playout/__tests__/playout-executeAction.test.ts#L13-L17packages/job-worker/src/playout/__tests__/playout-executeAction.test.ts#L49-L49
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/job-worker/src/playout/adlibAction.ts` around lines 321 - 330,
Replace setNextPartFromPart with setNextPart in the recueAfterExecute branch of
packages/job-worker/src/playout/adlibAction.ts#L321-L330, passing
nextPartInstance directly, and update the import at
packages/job-worker/src/playout/adlibAction.ts#L36-L36. In
packages/job-worker/src/playout/__tests__/playout-executeAction.test.ts#L206-L227,
assert setNextPartMock is called once; at `#L13-L17` mock setNextPart from
SetNextApi, and at `#L49` clear setNextPartMock in beforeEach.
Comment on lines
+42
to
+44
|
|
||
| const recueNextPartSnapshots = new Map<PartInstanceId, PlayoutPartInstanceModelSnapshot>() | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if recueNextPartSnapshot is ever explicitly cleared (set to undefined).
# Test: Search for assignments clearing the snapshot. Expect: Usage where it's cleared when a part is no longer next.
rg -A 2 'recueNextPartSnapshot\s*=\s*(undefined|null)' packages/job-worker/src/Repository: Sofie-Automation/sofie-core
Length of output: 165
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation around the map and its usages.
git ls-files packages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.ts
wc -l packages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.ts
ast-grep outline packages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.ts --view expanded || true
printf '\n--- file excerpt ---\n'
sed -n '1,220p' packages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.ts
printf '\n--- repo-wide search for snapshot identifiers ---\n'
rg -n "recueNextPartSnapshots|recueNextPartSnapshot|rescueNextPartSnapshot|nextPartSnapshots" packages/job-worker/src packages -S || trueRepository: Sofie-Automation/sofie-core
Length of output: 16420
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for lifecycle hooks that add/remove the snapshot from the map.
rg -n "Map<PartInstanceId, PlayoutPartInstanceModelSnapshot>|delete\\(|clear\\(|set\\(" packages/job-worker/src/playout -S || true
printf '\n--- related classes/functions around rescue/next-part logic ---\n'
rg -n "rescue|next part|nexted|nextPart" packages/job-worker/src/playout -S || trueRepository: Sofie-Automation/sofie-core
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the target file and find the exact snapshot map definition and all references.
sed -n '1,220p' packages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.ts
printf '\n--- references ---\n'
rg -n "recueNextPartSnapshots|recueNextPartSnapshot|snapshot" packages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.ts packages/job-worker/src/playout -SRepository: Sofie-Automation/sofie-core
Length of output: 28964
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Narrow search to the exact file and nearby code paths, plus references from the job-worker package.
file=packages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.ts
printf '%s\n' '--- file outline ---'
ast-grep outline "$file" --view expanded || true
printf '\n%s\n' '--- excerpt ---'
sed -n '1,220p' "$file"
printf '\n%s\n' '--- references in package ---'
rg -n "recueNextPartSnapshots|recueNextPartSnapshot|rescueNextPartSnapshot|clear\\(|delete\\(" packages/job-worker/src -S || trueRepository: Sofie-Automation/sofie-core
Length of output: 18646
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where recueNextPart() is actually invoked and how the next-part snapshot is managed.
rg -n "\.recueNextPart\(" packages/job-worker/src -S || true
printf '\n--- setNext excerpt around snapshot assignment ---\n'
sed -n '150,230p' packages/job-worker/src/playout/setNext.ts
printf '\n--- PlayoutPartInstanceModel interface excerpt ---\n'
sed -n '1,120p' packages/job-worker/src/playout/model/PlayoutPartInstanceModel.tsRepository: Sofie-Automation/sofie-core
Length of output: 8405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the next-part lifecycle and any cleanup paths that could clear rescue snapshots.
sed -n '520,700p' packages/job-worker/src/playout/model/implementation/PlayoutModelImpl.ts
printf '\n---\n'
sed -n '700,820p' packages/job-worker/src/playout/model/implementation/PlayoutModelImpl.ts
printf '\n--- search for explicit snapshot clearing ---\n'
rg -n "recueNextPartSnapshot\s*=\s*undefined|recueNextPartSnapshots\.delete|recueNextPartSnapshots\.clear|setNextPartSnapshot|setPartInstanceAsNext" packages/job-worker/src -S || trueRepository: Sofie-Automation/sofie-core
Length of output: 11856
Persist or evict the rescue snapshot
recueNextPartSnapshots is process-local and only shrinks on an explicit undefined assignment. If a part stops being nexted without clearing it, stale snapshots accumulate, and a rescue handled by another worker or after a restart can’t restore state. Persist this with the part instance or add a guaranteed cleanup path when next changes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.ts`
around lines 42 - 44, The rescue snapshot map in PlayoutPartInstanceModelImpl
must not remain process-local without lifecycle management. Persist each rescue
snapshot with its associated part instance, or add guaranteed cleanup whenever
the next-part state changes so stale entries are evicted; ensure rescue handling
can restore state after worker handoff or restart.
Comment on lines
237
to
252
| this.pieceInstancesImpl.clear() | ||
| for (const [pieceInstanceId, pieceInstance] of snapshot.pieceInstances) { | ||
| const pieceInstancesEntries = | ||
| snapshotImpl.pieceInstances instanceof Map | ||
| ? snapshotImpl.pieceInstances.entries() | ||
| : Object.entries(snapshotImpl.pieceInstances) | ||
|
|
||
| for (const [pieceInstanceId, pieceInstance] of pieceInstancesEntries) { | ||
| if (pieceInstance) { | ||
| this.pieceInstancesImpl.set( | ||
| pieceInstanceId, | ||
| pieceInstanceId as PieceInstanceId, | ||
| new PlayoutPieceInstanceModelImpl(pieceInstance.PieceInstance, pieceInstance.HasChanges) | ||
| ) | ||
| } else { | ||
| this.pieceInstancesImpl.set(pieceInstanceId, null) | ||
| this.pieceInstancesImpl.set(pieceInstanceId as PieceInstanceId, null) | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
snapshotRestore will orphan newly inserted piece instances in the database.
Clearing the pieceInstancesImpl map completely drops tracking for any PieceInstance that was inserted after the snapshot was taken. Because the deleted piece is not marked with null in the map, changedPieceInstanceIds() will not report it, and the database sync will fail to delete it from the database. This will cause "rescued" parts to retain pieces that were supposed to be reverted.
Instead of .clear(), you must identify pieces that exist currently but are absent in the snapshot, and explicitly set them to null to ensure the database layer deletes them. Also, restoring the snapshot should ideally force this.#partInstanceHasChanges = true to guarantee the reverted state is persisted.
🐛 Proposed fix for snapshot restoration
- this.#partInstanceHasChanges = snapshotImpl.partInstanceHasChanges
- this.pieceInstancesImpl.clear()
+ this.#partInstanceHasChanges = true
+
+ // Mark all current pieces as null (deleted) first, so that pieces not in the snapshot are deleted from the DB
+ for (const id of this.pieceInstancesImpl.keys()) {
+ this.pieceInstancesImpl.set(id, null)
+ }
+
const pieceInstancesEntries =
snapshotImpl.pieceInstances instanceof Map
? snapshotImpl.pieceInstances.entries()
: Object.entries(snapshotImpl.pieceInstances)
for (const [pieceInstanceId, pieceInstance] of pieceInstancesEntries) {
if (pieceInstance) {
this.pieceInstancesImpl.set(
pieceInstanceId as PieceInstanceId,
- new PlayoutPieceInstanceModelImpl(pieceInstance.PieceInstance, pieceInstance.HasChanges)
+ // Force HasChanges to true so the restored piece is saved to the DB
+ new PlayoutPieceInstanceModelImpl(pieceInstance.PieceInstance, true)
)
} else {
this.pieceInstancesImpl.set(pieceInstanceId as PieceInstanceId, null)
}
}📝 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
| this.pieceInstancesImpl.clear() | |
| for (const [pieceInstanceId, pieceInstance] of snapshot.pieceInstances) { | |
| const pieceInstancesEntries = | |
| snapshotImpl.pieceInstances instanceof Map | |
| ? snapshotImpl.pieceInstances.entries() | |
| : Object.entries(snapshotImpl.pieceInstances) | |
| for (const [pieceInstanceId, pieceInstance] of pieceInstancesEntries) { | |
| if (pieceInstance) { | |
| this.pieceInstancesImpl.set( | |
| pieceInstanceId, | |
| pieceInstanceId as PieceInstanceId, | |
| new PlayoutPieceInstanceModelImpl(pieceInstance.PieceInstance, pieceInstance.HasChanges) | |
| ) | |
| } else { | |
| this.pieceInstancesImpl.set(pieceInstanceId, null) | |
| this.pieceInstancesImpl.set(pieceInstanceId as PieceInstanceId, null) | |
| } | |
| } | |
| this.#partInstanceHasChanges = true | |
| // Mark all current pieces as null (deleted) first, so that pieces not in the snapshot are deleted from the DB | |
| for (const id of this.pieceInstancesImpl.keys()) { | |
| this.pieceInstancesImpl.set(id, null) | |
| } | |
| const pieceInstancesEntries = | |
| snapshotImpl.pieceInstances instanceof Map | |
| ? snapshotImpl.pieceInstances.entries() | |
| : Object.entries(snapshotImpl.pieceInstances) | |
| for (const [pieceInstanceId, pieceInstance] of pieceInstancesEntries) { | |
| if (pieceInstance) { | |
| this.pieceInstancesImpl.set( | |
| pieceInstanceId as PieceInstanceId, | |
| // Force HasChanges to true so the restored piece is saved to the DB | |
| new PlayoutPieceInstanceModelImpl(pieceInstance.PieceInstance, true) | |
| ) | |
| } else { | |
| this.pieceInstancesImpl.set(pieceInstanceId as PieceInstanceId, null) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@packages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.ts`
around lines 237 - 252, Update snapshotRestore to preserve tracking for current
piece instances absent from the snapshot: before restoring snapshot entries,
mark those existing IDs in pieceInstancesImpl as null instead of clearing the
map, then apply the snapshot entries as currently done. Also set
this.#partInstanceHasChanges = true so the reverted state is persisted.
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
This is a: Feature
Current Behavior
You can't recue a part via an action to clean any state changes easily.
New Behavior
Actions can request that the next part be recued.
Testing
Affected areas
This PR affects blueprints actions
Time Frame
Not urgent, but we would like to get this merged into the in-development release.
Other Information
Status