feat: add availablePostrollDuration to parts#1779
Open
ianshade wants to merge 1 commit into
Open
Conversation
It signifies how long a Part may continue after its `expectedDuration`, when transitions/keepalive require overlap
WalkthroughAdds an optional ChangesavailablePostrollDuration keepalive capping
Sequence Diagram(s)sequenceDiagram
participant Blueprint as Blueprint/Part DB
participant convertPart as convertPartToBlueprints
participant resolvedPieces as getResolvedPiecesForPartInstancesOnTimeline
participant ext as getAutoNextExpectedDurationExtension
participant rundown as createCurrentPartGroupEnable
Blueprint->>convertPart: part.availablePostrollDuration
convertPart-->>Blueprint: IBlueprintPartDB.availablePostrollDuration
resolvedPieces->>ext: currentPartInfo, nextPartTimings (fromPartKeepalive, outTransition)
ext-->>resolvedPieces: extension capped by availablePostrollDuration
resolvedPieces->>resolvedPieces: currentPartDuration = expectedDuration + extension + toPartDelay + toPartPostroll
resolvedPieces->>resolvedPieces: cap piece durations at currentPartEnd
rundown->>ext: currentPartInfo, nextPartTimings
ext-->>rundown: expectedDurationExtension
rundown->>rundown: currentPartEnable.duration = expectedDuration + extension + toPartDelay + toPartPostroll
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested labels
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 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✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/job-worker/src/playout/timeline/rundown.ts (1)
54-70: 🧹 Nitpick | 🔵 Trivial | 🏗️ Heavy liftConsolidate duplicate extension math into one shared helper.
getAutoNextExpectedDurationExtensionnow exists in timeline code while similar logic is also used in resolved-pieces timing. Keeping two copies risks subtle divergence betweencurrentPartGroup.enable.durationand resolved piece capping behavior.Refactor direction
- function getAutoNextExpectedDurationExtension(...) { ... } // local copy + import { getAutoNextExpectedDurationExtension } from '../shared/autoNextDuration'Then reuse the same helper from both:
packages/job-worker/src/playout/timeline/rundown.tspackages/job-worker/src/playout/resolvedPieces.ts🤖 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/timeline/rundown.ts` around lines 54 - 70, Extract the duration extension calculation logic from getAutoNextExpectedDurationExtension in rundown.ts into a shared utility helper function that can be imported and reused by both the timeline code (packages/job-worker/src/playout/timeline/rundown.ts) and the resolved-pieces timing code (packages/job-worker/src/playout/resolvedPieces.ts). This consolidates the duplicate extension math into one authoritative location, preventing subtle divergence between currentPartGroup.enable.duration and resolved piece capping behavior.packages/job-worker/src/playout/__tests__/resolvedPieces.test.ts (1)
970-1358: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd an explicit
availablePostrollDuration: Infinitytest case.The new suite covers finite caps well, but it does not lock in the documented uncapped behavior for
+Infinity. A focused assertion here would protect the contract.🤖 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/__tests__/resolvedPieces.test.ts` around lines 970 - 1358, Add a new test case that explicitly sets availablePostrollDuration to Infinity to document and verify the uncapped behavior. Create a test following the same pattern as the existing availablePostrollDuration tests (like the ones capped by 0, 700, and 5000), but explicitly pass availablePostrollDuration: Infinity in the createPartInstance call for the currentPartInfo. The test should verify that when availablePostrollDuration is Infinity, the fromPartKeepalive is not capped and the resolved duration extends fully by the keepalive amount, ensuring the uncapped behavior is locked in by an explicit assertion.packages/job-worker/src/playout/timeline/__tests__/rundown.test.ts (1)
388-425: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRename the first two test titles to match their assertions.
At Line 388 and Line 427, both test names say “extends current part duration”, but expected duration stays
5000. Renaming to “does not extend … when postroll budget is unset/zero” will make intent unambiguous.Also applies to: 427-465
🤖 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/timeline/__tests__/rundown.test.ts` around lines 388 - 425, The test titles for the test blocks starting at line 388 and line 427 are misleading. Both tests are currently titled "autonext with keepalive extends current part duration" but the assertions verify that the duration remains at 5000 and does not extend. Rename both test titles (the strings passed to the it() function calls) to accurately reflect that the duration does NOT extend when postroll budget is unset or zero, such as "autonext with keepalive does not extend current part duration when postroll budget is unset" or similar wording that makes the test intent unambiguous.packages/job-worker/src/playout/resolvedPieces.ts (1)
14-26: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCentralize the auto-next extension calculation.
This helper duplicates the same timing contract used in
timeline/rundown.ts; if one copy changes later, resolved pieces and timeline group durations can diverge. Consider extracting a shared helper that both call sites use.🤖 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/resolvedPieces.ts` around lines 14 - 26, The function getAutoNextExpectedDurationExtension contains timing calculation logic that is duplicated in timeline/rundown.ts, creating a risk that changes to one copy could cause the resolved pieces and timeline group durations to diverge. Extract the shared auto-next extension calculation logic into a single centralized helper function (in a utilities or common module) and update both the resolvedPieces.ts file and the timeline/rundown.ts file to call this shared helper instead of duplicating the logic.
🤖 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/playout/resolvedPieces.ts`:
- Around line 72-83: The zero-duration guard applied to the `nextPartStarted`
calculation (checking `expectedDuration !== 0`) is not applied to the
`currentPartEnd` calculation. Add the same `expectedDuration !== 0` condition to
the `currentPartEnd` ternary expression so that it returns null when the
expected duration is zero, preventing the truncation of zero-duration auto-next
parts that occurs downstream when this value is used at line 105.
---
Nitpick comments:
In `@packages/job-worker/src/playout/__tests__/resolvedPieces.test.ts`:
- Around line 970-1358: Add a new test case that explicitly sets
availablePostrollDuration to Infinity to document and verify the uncapped
behavior. Create a test following the same pattern as the existing
availablePostrollDuration tests (like the ones capped by 0, 700, and 5000), but
explicitly pass availablePostrollDuration: Infinity in the createPartInstance
call for the currentPartInfo. The test should verify that when
availablePostrollDuration is Infinity, the fromPartKeepalive is not capped and
the resolved duration extends fully by the keepalive amount, ensuring the
uncapped behavior is locked in by an explicit assertion.
In `@packages/job-worker/src/playout/resolvedPieces.ts`:
- Around line 14-26: The function getAutoNextExpectedDurationExtension contains
timing calculation logic that is duplicated in timeline/rundown.ts, creating a
risk that changes to one copy could cause the resolved pieces and timeline group
durations to diverge. Extract the shared auto-next extension calculation logic
into a single centralized helper function (in a utilities or common module) and
update both the resolvedPieces.ts file and the timeline/rundown.ts file to call
this shared helper instead of duplicating the logic.
In `@packages/job-worker/src/playout/timeline/__tests__/rundown.test.ts`:
- Around line 388-425: The test titles for the test blocks starting at line 388
and line 427 are misleading. Both tests are currently titled "autonext with
keepalive extends current part duration" but the assertions verify that the
duration remains at 5000 and does not extend. Rename both test titles (the
strings passed to the it() function calls) to accurately reflect that the
duration does NOT extend when postroll budget is unset or zero, such as
"autonext with keepalive does not extend current part duration when postroll
budget is unset" or similar wording that makes the test intent unambiguous.
In `@packages/job-worker/src/playout/timeline/rundown.ts`:
- Around line 54-70: Extract the duration extension calculation logic from
getAutoNextExpectedDurationExtension in rundown.ts into a shared utility helper
function that can be imported and reused by both the timeline code
(packages/job-worker/src/playout/timeline/rundown.ts) and the resolved-pieces
timing code (packages/job-worker/src/playout/resolvedPieces.ts). This
consolidates the duplicate extension math into one authoritative location,
preventing subtle divergence between currentPartGroup.enable.duration and
resolved piece capping behavior.
🪄 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: d5bbad29-b0ab-47cd-8315-37a97b54e33c
📒 Files selected for processing (6)
packages/blueprints-integration/src/documents/part.tspackages/job-worker/src/blueprints/context/lib.tspackages/job-worker/src/playout/__tests__/resolvedPieces.test.tspackages/job-worker/src/playout/resolvedPieces.tspackages/job-worker/src/playout/timeline/__tests__/rundown.test.tspackages/job-worker/src/playout/timeline/rundown.ts
Comment on lines
72
to
+83
| const nextPartStarted = | ||
| partInstancesInfo.current.partInstance.part.autoNext && | ||
| partInstancesInfo.current.partInstance.part.expectedDuration !== 0 && | ||
| partInstancesInfo.current.partInstance.part.expectedDuration !== undefined | ||
| ? currentPartStarted + partInstancesInfo.current.partInstance.part.expectedDuration | ||
| currentPartDuration !== null | ||
| ? currentPartStarted + | ||
| currentPartDuration - | ||
| (partInstancesInfo.next?.calculatedTimings.fromPartRemaining ?? 0) | ||
| : null | ||
|
|
||
| const currentPartEnd = | ||
| partInstancesInfo.current.partInstance.part.autoNext && currentPartDuration !== null | ||
| ? currentPartStarted + currentPartDuration |
There was a problem hiding this comment.
Apply the zero-duration guard to currentPartEnd too.
Line 74 prevents calculating an auto-next start for expectedDuration === 0, but lines 81-83 still produce a cap that line 105 applies to current pieces. That can unexpectedly truncate zero-duration auto-next parts.
🐛 Proposed fix
+ const hasValidAutoNextDuration =
+ partInstancesInfo.current.partInstance.part.autoNext &&
+ partInstancesInfo.current.partInstance.part.expectedDuration !== 0 &&
+ currentPartDuration !== null
+
const nextPartStarted =
- partInstancesInfo.current.partInstance.part.autoNext &&
- partInstancesInfo.current.partInstance.part.expectedDuration !== 0 &&
- currentPartDuration !== null
+ hasValidAutoNextDuration
? currentPartStarted +
currentPartDuration -
(partInstancesInfo.next?.calculatedTimings.fromPartRemaining ?? 0)
: null
- const currentPartEnd =
- partInstancesInfo.current.partInstance.part.autoNext && currentPartDuration !== null
- ? currentPartStarted + currentPartDuration
- : null
+ const currentPartEnd = hasValidAutoNextDuration ? currentPartStarted + currentPartDuration : nullAlso applies to: 105-105
🤖 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/resolvedPieces.ts` around lines 72 - 83, The
zero-duration guard applied to the `nextPartStarted` calculation (checking
`expectedDuration !== 0`) is not applied to the `currentPartEnd` calculation.
Add the same `expectedDuration !== 0` condition to the `currentPartEnd` ternary
expression so that it returns null when the expected duration is zero,
preventing the truncation of zero-duration auto-next parts that occurs
downstream when this value is used at line 105.
Saftret
added
the
Contribution from BBC
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
Current Behavior
When a Part had
autoNextand anexpectedDuration, anyoutTransitionor next-Part'spreviousPartKeepaliveDurationwould overlap with the end of the Part - the next Part would begin sooner and the transition would run while the current Part's content was still presumed to be playing, consuming time from within theexpectedDuration. Completely reasonable as the default behavior for preventing freeze frames from appearing on the output - if e.g. a VT has exactlyexpectedDurationof playable content, the transition should start earlier so that the VT does not freeze while the transition executes. There are however use cases where it is not desired for a transition to eat into theexpectedDurationof the previous part.New Behavior
Blueprints can now set
availablePostrollDurationon a Part to declare how much extra time beyondexpectedDurationthe Part is allowed to run. When the next Part requires overlap (viaoutTransitionorfromPartKeepalive), Sofie extends the current Part's timeline group by up toavailablePostrollDuration, effectively delaying the autoNext - just enough to satisfy the overlap - so in a best case scenario the transition no longer has to borrow time from within the scheduled duration at all.Setting it to zero (or not setting at all) would be equivalent to the existing behavior - Sofie would execute the take sooner, making sure the part (including its transition keepalive) is gone from the air by the time
expectedDurationpasses.Setting it to +Infinity, means that the part is allowed to last past
expectedDurationhowever long the transition is.Any value in between can be used when the available postroll is known, e.g. if the VT has a defined duration of spare material that is allowed to be shown on the output while the transition executes - any transition longer than that, will start consuming the previous Part's duration.
Testing
Affected areas
Time Frame
Other Information
Status