fix: prevent a still playing part from disappearing from the timeline#1700
Open
ianshade wants to merge 6 commits into
Open
fix: prevent a still playing part from disappearing from the timeline#1700ianshade wants to merge 6 commits into
ianshade wants to merge 6 commits into
Conversation
WalkthroughRefactors RundownPlaylist to track multiple previous part instances: Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client(UI)
participant API as API / Webserver
participant DB as DB (RundownPlaylists)
participant Playout as PlayoutModel
participant Lookahead as Lookahead/ResolvedPieces
participant Timeline as Timeline Generator
UI->>API: request playlist / trigger take
API->>DB: read `previousPartsInfo`
DB-->>API: playlist with previousPartsInfo[]
API->>Playout: loadPartInstances(previousPartsInfo[])
Playout->>Lookahead: compute lookahead using previousPartsInfo (oldest-first)
Lookahead->>Timeline: generate timeline objects (chain caps/overlap)
Timeline-->>API: timeline payload
API-->>UI: updated timeline / state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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! |
ianshade
force-pushed
the
contribute/EAV-949
branch
2 times, most recently
from
f41537d to
b544d85
Compare
ianshade
changed the title
ianshade
force-pushed
the
contribute/EAV-949
branch
from
b544d85 to
739abe8
Compare
ianshade
force-pushed
the
contribute/EAV-949
branch
2 times, most recently
from
a06d461 to
5bf5f55
Compare
… timeline when more than two parts overlap keeps track of more than one previous part, so that they are included in the timeline and ab-logic, until stopped playback; timing properties are used for pruning, but an arbitrary limit of 10 is in place, to avoid too much data piling up if blueprints did something very wrong
ianshade
force-pushed
the
contribute/EAV-949
branch
from
5bf5f55 to
e1076ed
Compare
ianshade
added
the
Contribution from TV 2 Norge
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/webui/src/client/ui/SegmentTimeline/Parts/SegmentTimelinePart.tsx (1)
158-178:⚠️ Potential issue | 🟠 MajorCheck every previous part before entering duration settling.
Line 162 only treats
previousPartsInfo[0]as previous. With multiple still-active previous parts, older entries will skipisDurationSettling, so their timeline width can stop reflecting ongoing playback before they are pruned.🐛 Proposed fix
- const isPrevious = nextProps.playlist.previousPartsInfo?.[0]?.partInstanceId === nextProps.part.instance._id + const isPrevious = + nextProps.playlist.previousPartsInfo?.some( + (previousPartInfo) => previousPartInfo.partInstanceId === nextProps.part.instance._id + ) ?? false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/SegmentTimeline/Parts/SegmentTimelinePart.tsx` around lines 158 - 178, getDerivedStateFromProps currently only checks previousPartsInfo[0] to determine if a part is previous, which misses older still-active previous parts; update the isPrevious calculation in getDerivedStateFromProps to test the entire nextProps.playlist.previousPartsInfo array (e.g. using .some(...) to compare partInstanceId to nextProps.part.instance._id) so that isDurationSettling (which relies on isPrevious) correctly applies for any previous part entry rather than only the first one.packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx (1)
122-127:⚠️ Potential issue | 🟠 MajorDisable edits for all active previous parts.
Line 126 only blocks edits for
previousPartsInfo[0]. Older previous parts can still be active after this PR, so user edit operations can become enabled on on-air previous content.🐛 Proposed fix
+ const partInstanceId = part?.instance._id + const isPartEditAble = + !!partInstanceId && isSegmentEditAble && - part?.instance._id !== playlist.currentPartInfo?.partInstanceId && - part?.instance._id !== playlist.nextPartInfo?.partInstanceId && - part?.instance._id !== playlist.previousPartsInfo?.[0]?.partInstanceId + partInstanceId !== playlist.currentPartInfo?.partInstanceId && + partInstanceId !== playlist.nextPartInfo?.partInstanceId && + !(playlist.previousPartsInfo?.some((previousPartInfo) => previousPartInfo.partInstanceId === partInstanceId) ?? false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx` around lines 122 - 127, The isPartEditAble check currently only excludes playlist.previousPartsInfo?.[0] so older previous parts remain editable; update the condition in the isPartEditAble computation to check against all entries in playlist.previousPartsInfo (e.g., ensure part?.instance._id is not equal to any previousPartsInfo[].partInstanceId by using Array.some/Array.find and negating it) so edits are disabled for any active previous part; reference the isPartEditAble variable, part?.instance._id, and playlist.previousPartsInfo to locate and modify the logic.packages/webui/src/client/ui/RundownView/RundownViewSubscriptions.ts (1)
91-113:⚠️ Potential issue | 🟠 MajorSubscribe to piece instances for every previous part.
This still includes only
previousPartsInfo[0]. With multiple previous parts alive, older previous part instances won’t have their piece instances subscribed on the client, so the rundown/timeline UI can still drop the part this PR is trying to preserve.Proposed subscription fix
meteorSubscribe( CorelibPubSub.pieceInstances, rundownIds, [ playlist.currentPartInfo?.partInstanceId, playlist.nextPartInfo?.partInstanceId, - playlist.previousPartsInfo?.[0]?.partInstanceId, - ].filter((p): p is PartInstanceId => p !== null), + ...(playlist.previousPartsInfo ?? []).map((info) => info.partInstanceId), + ].filter((p): p is PartInstanceId => p !== null && p !== undefined), {} )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/RundownView/RundownViewSubscriptions.ts` around lines 91 - 113, The subscription currently passes only previousPartsInfo[0] as the previous part instance to meteorSubscribe, so pieceInstances for older previous parts are not subscribed; update the argument construction passed to meteorSubscribe (where playlist is read and meteorSubscribe is called) to include all partInstanceId values from playlist.previousPartsInfo (e.g., map previousPartsInfo to partInstanceId and filter nulls) instead of only previousPartsInfo[0], keeping the same filtering to PartInstanceId and preserving currentPartInfo and nextPartInfo inclusion.packages/webui/src/client/ui/Shelf/AdLibPanel.tsx (1)
142-179:⚠️ Potential issue | 🟠 MajorInvalidate the adlib segment cache for the full previous-part chain.
Line 172 watches only
previousPartsInfo[0], butfetchAndFilterlater builds active part instances from the playlist. If an older previous part is added/pruned while the latest previous entry stays the same, the cached UI segments can be stale.Proposed cache-key fix
export function useFetchAndFilter( playlist: DBRundownPlaylist | undefined, showStyleBase: UIShowStyleBase | undefined, filter: RundownLayoutFilterBase | undefined, includeGlobalAdLibs: boolean | undefined ): AdLibFetchAndFilterProps { + const previousPartInstanceIdsKey = playlist?.previousPartsInfo?.map((info) => info.partInstanceId).join('\0') ?? '' + return useTracker( () => playlist && showStyleBase ? fetchAndFilter({ @@ playlist?.currentPartInfo?.partInstanceId, playlist?.nextPartInfo?.partInstanceId, - playlist?.previousPartsInfo?.[0]?.partInstanceId, + previousPartInstanceIdsKey, playlist?.rundownIdsInOrder, showStyleBase?._id,Also include the same key in the
memoizedIsolatedAutorunarguments that computeuiSegments, because that block callsgetActivePartInstancesMap(props.playlist):( currentPartInstanceId: PartInstanceId | null, nextPartInstanceId: PartInstanceId | null, + _previousPartInstanceIdsKey: string, segments: DBSegment[], rundowns: Record<string, MinimalRundown> ) => { @@ `uiSegments_${props.filter?._id || 'no_filter'}`, props.playlist.currentPartInfo?.partInstanceId ?? null, props.playlist.nextPartInfo?.partInstanceId ?? null, + props.playlist.previousPartsInfo?.map((info) => info.partInstanceId).join('\0') ?? '', segments, rundowns )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/ui/Shelf/AdLibPanel.tsx` around lines 142 - 179, The cache key only watches previousPartsInfo[0], causing stale uiSegments when deeper previous parts change; update useFetchAndFilter to include the full previousPartsInfo array (e.g. playlist?.previousPartsInfo) in the dependency list passed to useTracker so fetchAndFilter sees changes across the whole chain, and also add that same full previousPartsInfo dependency to the memoizedIsolatedAutorun (the block that computes uiSegments / calls getActivePartInstancesMap) so the memoized computation invalidates when any previous part is added/pruned; reference useFetchAndFilter, fetchAndFilter, memoizedIsolatedAutorun and uiSegments when applying this change.packages/job-worker/src/ingest/commit.ts (2)
474-485:⚠️ Potential issue | 🔴 CriticalPreserve active previous PartInstances during ingest cleanup.
Lines 479-480 only protect current/next. With this PR, a still-active previous PartInstance can be reset when its source Part is removed, which can drop the very timeline contribution this change is trying to keep alive.
Proposed fix
const instancesToReset: PartInstanceId[] = [] const instancesToOrphan: PartInstanceId[] = [] + const previousPartInstanceIds = new Set((playlist.previousPartsInfo ?? []).map((info) => info.partInstanceId)) for (const partInstance of partInstancesToOrphan) { if ( + !previousPartInstanceIds.has(partInstance._id) && playlist.currentPartInfo?.partInstanceId !== partInstance._id && playlist.nextPartInfo?.partInstanceId !== partInstance._id ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/ingest/commit.ts` around lines 474 - 485, The loop that classifies partInstancesToOrphan currently only protects playlist.currentPartInfo?.partInstanceId and playlist.nextPartInfo?.partInstanceId, so a still-active previous partInstance can be incorrectly added to instancesToReset; update the condition inside the for (const partInstance of partInstancesToOrphan) loop to also check and protect playlist.previousPartInfo?.partInstanceId (i.e., compare partInstance._id against playlist.previousPartInfo?.partInstanceId alongside current and next) so active previous PartInstances are pushed to instancesToOrphan rather than instancesToReset.
598-605:⚠️ Potential issue | 🟠 MajorCheck the full previous-part chain before clearing rundown references.
Line 602 only checks the most recent previous PartInstance. If an older
previousPartsInfoentry belongs torundownIdToForget, it remains referenced after that rundown is removed.Proposed fix
if ( rundownIdToForget && - playoutModel.previousPartInstance && - playoutModel.previousPartInstance.partInstance.rundownId === rundownIdToForget + playoutModel.previousPartInstances.some( + (previousPartInstance) => previousPartInstance.partInstance.rundownId === rundownIdToForget + ) ) { playoutModel.clearPreviousPartInstances() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/ingest/commit.ts` around lines 598 - 605, The current check only inspects playoutModel.previousPartInstance; update the logic to scan the whole previousPartsInfo chain and clear references if any entry references rundownIdToForget. Concretely, iterate playoutModel.previousPartsInfo (or equivalent array on playoutModel) and if any entry.partInstance.rundownId === rundownIdToForget then call playoutModel.clearPreviousPartInstances(); otherwise leave the chain intact; use the existing symbols previousPartInstance, previousPartsInfo, rundownIdToForget, and playoutModel.clearPreviousPartInstances() to locate and modify the code.
🧹 Nitpick comments (5)
packages/job-worker/src/blueprints/context/services/__tests__/PartAndPieceInstanceActionService.test.ts (1)
287-291: Consider letting this helper seed multiple previous parts.The helper always writes a single-element
previousPartsInfo, so tests using it can’t cover the new multi-previous-part behavior introduced by this PR.Proposed helper refactor
- previousPartInstance?: PlayoutPartInstanceModel | DBPartInstance | PieceInstance | null + previousPartInstance?: + | PlayoutPartInstanceModel + | DBPartInstance + | PieceInstance + | Array<PlayoutPartInstanceModel | DBPartInstance | PieceInstance> + | null @@ if (previousPartInstance !== undefined) { + const previousPartInstances = Array.isArray(previousPartInstance) + ? previousPartInstance + : previousPartInstance + ? [previousPartInstance] + : [] await jobContext.mockCollections.RundownPlaylists.update(playlistId, { $set: { - previousPartsInfo: previousPartInstance ? [convertInfo(previousPartInstance)!] : [], + previousPartsInfo: previousPartInstances.map((partInstance) => convertInfo(partInstance)!), }, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/blueprints/context/services/__tests__/PartAndPieceInstanceActionService.test.ts` around lines 287 - 291, The helper currently only seeds a single previous part by setting previousPartsInfo to previousPartInstance ? [convertInfo(previousPartInstance)!] : [], so update the helper signature to accept multiple previous parts (e.g., previousPartInstances or allow an array), and change the RundownPlaylists.update call (still referenced as jobContext.mockCollections.RundownPlaylists.update and playlistId) to set previousPartsInfo to previousPartInstances?.map(convertInfo).filter(Boolean) || [] so tests can seed and exercise multi-previous-part behavior; keep convertInfo usage and preserve the empty-array fallback when none supplied.packages/job-worker/src/playout/lookahead/__tests__/lookahead.test.ts (1)
296-318: Add coverage for multiple previous parts and ordering.This test only verifies a single previous entry. Since the fix depends on preserving a chain of previous parts, please add a case with at least two entries and assert that
findLookaheadForLayerreceives them in the expected oldest-first order.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/playout/lookahead/__tests__/lookahead.test.ts` around lines 296 - 318, Add a test case that creates at least two previous entries in the SelectedPartInstancesTimelineInfo (e.g. duplicate previousEntry with different partInstance._id and different partTimes.nowInPart values), push them into partInstancesInfo.previous in chronological order (oldest first), call getLookeaheadObjects via runJobWithPlayoutModel as done now, and update the expectedPrevious to be an array of the two mapped objects (using previousEntry.partInstance, partTimes.nowInPart, pieceInstances, calculatedTimings) in oldest-first order; finally assert via expectLookaheadForLayerMock (and indirectly findLookaheadForLayer) that it was invoked with previous equal to that ordered array to verify ordering and multiple-previous handling for getLookeaheadObjects.meteor/server/api/deviceTriggers/__tests__/TagsService.test.ts (1)
244-402: LGTM — coverage matches the contract that pieces from any entry ofpreviousPartsInfoare tallied as on-air. The setup duplicates portions ofcreateAndPopulateMockCache; a small shared helper acceptingpreviousPartsInfocould reduce the repetition, but it's not essential.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@meteor/server/api/deviceTriggers/__tests__/TagsService.test.ts` around lines 244 - 402, Tests duplicate cache setup between the two cases; extract the repeated setup into a small helper (e.g. createAndPopulateMockCache or buildTestCache) that accepts previousPartsInfo (array of { partInstanceId }) and returns the created testee and cache (or at least the cache and showStyleBaseId) so both tests call that helper, then call testee.updatePieceInstances(cache, showStyleBaseId) and assert via testee.getTallyStateFromTags; update references to createTestee, cache construction, ShowStyleBases insert, PieceInstances/PartInstances inserts to use the helper to remove duplication.packages/job-worker/src/playout/__tests__/resolvedPieces.test.ts (1)
972-1070: Nice coverage for the multi-previous-parts path.The two new tests lock in both the cap-at-following-part boundary and the "don't extend a naturally shorter piece" invariant, which is exactly the behavior the implementation in
resolvedPieces.tsrelies on. It might be worth adding a one-liner comment in the test file (or a JSDoc onSelectedPartInstancesTimelineInfo.previous) stating thatpreviousis most-recent-first, since that ordering is now load-bearing for the cap computation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/playout/__tests__/resolvedPieces.test.ts` around lines 972 - 1070, Tests rely on the ordering of SelectedPartInstancesTimelineInfo.previous being most-recent-first but there's no explicit comment; add a one-line clarifying comment or JSDoc stating that SelectedPartInstancesTimelineInfo.previous is ordered most-recent-first (newest first) so the cap computation in getResolvedPiecesForPartInstancesOnTimeline is unambiguous. Update either the test file near the test block or the SelectedPartInstancesTimelineInfo.previous property JSDoc (or type/interface declaration) to include that single-line clarification.packages/job-worker/src/playout/resolvedPieces.ts (1)
78-96: Ordering contract is now load-bearing; consider documenting it.The per-entry
capEndderivation relies onpartInstancesInfo.previousbeing ordered most-recent-first —previous[0]must start afterprevious[1], etc. This is consistent with the tests and the UI helper, but the contract is implicit on the type. A short JSDoc onSelectedPartInstancesTimelineInfo.previous(intimeline/generate.ts) stating the ordering would protect future callers from silently producing negative cap intervals.Two smaller notes on the same block:
- If
previous[i-1]wascontinue'd (nopartStartTime), the fallback tocurrentPartStartedcollapses the cap for the entry atito the current-part boundary, silently losing the intermediate cap. Unlikely in practice, but worth being aware of.allPreviousResolvedPieces = allPreviousResolvedPieces.concat(resolved)in a loop can be replaced withallPreviousResolvedPieces.push(...resolved)for O(n) growth. Negligible given the 10-previous cap mentioned in the PR description.♻️ Optional tweak
- let allPreviousResolvedPieces: ResolvedPieceInstance[] = [] + const allPreviousResolvedPieces: ResolvedPieceInstance[] = [] for (let i = 0; i < partInstancesInfo.previous.length; i++) { const prevInfo = partInstancesInfo.previous[i] if (!prevInfo.partTimes.partStartTime) continue const capEnd = i === 0 ? currentPartStarted : (partInstancesInfo.previous[i - 1].partTimes.partStartTime ?? currentPartStarted) const resolved = prevInfo.pieceInstances.map((instance) => resolvePrunedPieceInstance(prevInfo.partTimes, instance) ) offsetResolvedStartAndCapDuration(resolved, prevInfo.partTimes.partStartTime, capEnd) - allPreviousResolvedPieces = allPreviousResolvedPieces.concat(resolved) + allPreviousResolvedPieces.push(...resolved) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/playout/resolvedPieces.ts` around lines 78 - 96, Document the ordering contract on SelectedPartInstancesTimelineInfo.previous in timeline/generate.ts (JSDoc: array is most-recent-first so previous[0] starts after previous[1], etc.). In resolvedPieces.ts, make the cap calculation robust: when computing capEnd for index i, search backward from i-1 to find the nearest previous entry with partTimes.partStartTime and use that startTime as the cap (fallback to currentPartStarted only if none found) to avoid collapsing caps when an earlier entry was skipped; keep using resolvePrunedPieceInstance and offsetResolvedStartAndCapDuration. Also replace allPreviousResolvedPieces = allPreviousResolvedPieces.concat(resolved) with allPreviousResolvedPieces.push(...resolved) for better append performance.
🤖 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/job-worker/src/playout/__tests__/lib.ts`:
- Around line 31-34: The two thrown errors use misleading labels: when
nextPartInstance is undefined use a message that references the missing next
part instance (e.g., playlist.nextPartInfo?.partInstanceId) and when
previousPartInstance is undefined use a message that references the missing
previous part instance (e.g., playlist.previousPartsInfo?.[0]?.partInstanceId);
update the Error messages in the checks for nextPartInstance and
previousPartInstance to correctly say "Missing nextPartInstance" and "Missing
previousPartInstance" (or equivalent) so they match the variable being
validated.
In
`@packages/job-worker/src/playout/lookahead/__tests__/findForLayer/constants.ts`:
- Around line 25-32: The fixture's "previous" array in constants.ts only
contains one PartInstance, so add at least one more
PartInstanceAndPieceInstances object (use createFakePiece to create pieces) to
simulate multiple previous entries and pass a 2+ previousParts scenario through
findLookaheadForLayer/findForLayer; ensure the additional entries are ordered
most-recent-first (matching DBRundownPlaylist.previousPartsInfo) and set
appropriate onTimeline/nowInPart values to reflect distinct previous parts.
In `@packages/job-worker/src/playout/lookahead/findForLayer.ts`:
- Around line 15-20: Update the JSDoc for
PartInstanceAndPieceInstancesInfos.previous to explicitly state the invariant
that every entry has onTimeline === true (oldest-first), and then simplify
findForLayer by removing the dead defensive branch that checks
prevInfo.onTimeline for entries from previous; instead either assert
prevInfo.onTimeline or unconditionally handle them as on-timeline (push to the
same branch you currently take when onTimeline is true) in the loop that
iterates previous (referencing the symbols
PartInstanceAndPieceInstancesInfos.previous, prevInfo, and the result buckets
like res.future/res.timed used in findForLayer).
In `@packages/job-worker/src/playout/model/implementation/PlayoutModelImpl.ts`:
- Around line 562-596: prunePreviousPartInstances currently treats a reference
part as "active" when referenceStarted is undefined, which conflicts with
generatePreviousPartInstancesObjects (which skips entries lacking
plannedStartedPlayback) and causes dangling timeline group refs; in
prunePreviousPartInstances (method name) change the active check to treat
entries with missing plannedStartedPlayback as inactive (i.e. require
referenceStarted and referenceTimings to be defined and now <= referenceStarted
+ referenceTimings.fromPartRemaining), remove the forced fallback that always
keeps one entry so the previousPartsInfo array may become empty when nothing is
active, replace the length-only change detection with a direct identity/content
check (e.g. compare pruned !== current or deep-equal) to set
`#playlistHasChanged`, and hoist MAX_PREVIOUS_PARTS out of the function into a
module-level const with a brief comment explaining the cap; this will align
behavior with generatePreviousPartInstancesObjects and avoid leaving unreachable
group.start references.
In `@packages/job-worker/src/playout/timeline/generate.ts`:
- Around line 335-343: The regeneration scheduling misses previous PartInstances
— ensure previousPartInstances are included wherever regenerateTimelineAt is
built (the spots around getPartInstanceTimelineInfo usage that currently only
include current/next). For each place that constructs the list (e.g., where
currentPartInstance and nextPartInstance are considered at lines referenced),
append previousPartInstances.flatMap(pi => { const info =
getPartInstanceTimelineInfo(absolutePiecePrepareTime, targetNowTime,
showStyle.sourceLayers, pi); return info ? [info] : [] }) so previous parts that
need timeline regeneration are scheduled; make sure to merge and filter
undefined entries the same way as for current/next entries (use the same
mapping/filter pattern) so any previously skipped future absolute pieces get the
regeneration trigger.
In `@packages/job-worker/src/playout/timeline/rundown.ts`:
- Around line 505-531: The loop over previousPartsInfo can create dangling
timeline references when a skipped previous part
(previousPartInfo.partInstance.timings?.plannedStartedPlayback is falsy) is
skipped with continue; change that continue to break so once a previous part
lacks plannedStartedPlayback you stop building older groups (affects the logic
that computes nextGroupId using timingContext.currentPartGroup.id and
getPartGroupId(previousPartsInfo[i - 1].partInstance) and the end expressions
that use currentPartInstanceTimings.fromPartRemaining or previousPartsInfo[i -
1].calculatedTimings.fromPartRemaining), preventing references to non-existent
groups created by createPartGroup; also add a unit test in
__tests__/rundown.test.ts that constructs multiple previousPartsInfo entries
where an earlier entry has no plannedStartedPlayback to assert older groups are
not emitted.
In `@packages/live-status-gateway/src/collections/pieceInstancesHandler.ts`:
- Around line 120-131: The code pairs pieces from previousPartsInfo with timing
objects by array index which breaks when this._partInstances.previous has been
filtered; instead build a Map of previous PartInstance timings keyed by
partInstanceId from this._partInstances.previous and then, inside the flatMap
over this._currentPlaylist.previousPartsInfo, look up the timing object by
info.partInstanceId and pass that result to processAndPrunePieceInstanceTimings
(preserving the existing call to collection.find({ partInstanceId:
info.partInstanceId }) and the boolean flag); this ensures
processAndPrunePieceInstanceTimings receives the correct timing object even when
entries were removed from _partInstances.previous.
---
Outside diff comments:
In `@packages/job-worker/src/ingest/commit.ts`:
- Around line 474-485: The loop that classifies partInstancesToOrphan currently
only protects playlist.currentPartInfo?.partInstanceId and
playlist.nextPartInfo?.partInstanceId, so a still-active previous partInstance
can be incorrectly added to instancesToReset; update the condition inside the
for (const partInstance of partInstancesToOrphan) loop to also check and protect
playlist.previousPartInfo?.partInstanceId (i.e., compare partInstance._id
against playlist.previousPartInfo?.partInstanceId alongside current and next) so
active previous PartInstances are pushed to instancesToOrphan rather than
instancesToReset.
- Around line 598-605: The current check only inspects
playoutModel.previousPartInstance; update the logic to scan the whole
previousPartsInfo chain and clear references if any entry references
rundownIdToForget. Concretely, iterate playoutModel.previousPartsInfo (or
equivalent array on playoutModel) and if any entry.partInstance.rundownId ===
rundownIdToForget then call playoutModel.clearPreviousPartInstances(); otherwise
leave the chain intact; use the existing symbols previousPartInstance,
previousPartsInfo, rundownIdToForget, and
playoutModel.clearPreviousPartInstances() to locate and modify the code.
In `@packages/webui/src/client/ui/RundownView/RundownViewSubscriptions.ts`:
- Around line 91-113: The subscription currently passes only
previousPartsInfo[0] as the previous part instance to meteorSubscribe, so
pieceInstances for older previous parts are not subscribed; update the argument
construction passed to meteorSubscribe (where playlist is read and
meteorSubscribe is called) to include all partInstanceId values from
playlist.previousPartsInfo (e.g., map previousPartsInfo to partInstanceId and
filter nulls) instead of only previousPartsInfo[0], keeping the same filtering
to PartInstanceId and preserving currentPartInfo and nextPartInfo inclusion.
In `@packages/webui/src/client/ui/SegmentTimeline/Parts/SegmentTimelinePart.tsx`:
- Around line 158-178: getDerivedStateFromProps currently only checks
previousPartsInfo[0] to determine if a part is previous, which misses older
still-active previous parts; update the isPrevious calculation in
getDerivedStateFromProps to test the entire nextProps.playlist.previousPartsInfo
array (e.g. using .some(...) to compare partInstanceId to
nextProps.part.instance._id) so that isDurationSettling (which relies on
isPrevious) correctly applies for any previous part entry rather than only the
first one.
In `@packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx`:
- Around line 122-127: The isPartEditAble check currently only excludes
playlist.previousPartsInfo?.[0] so older previous parts remain editable; update
the condition in the isPartEditAble computation to check against all entries in
playlist.previousPartsInfo (e.g., ensure part?.instance._id is not equal to any
previousPartsInfo[].partInstanceId by using Array.some/Array.find and negating
it) so edits are disabled for any active previous part; reference the
isPartEditAble variable, part?.instance._id, and playlist.previousPartsInfo to
locate and modify the logic.
In `@packages/webui/src/client/ui/Shelf/AdLibPanel.tsx`:
- Around line 142-179: The cache key only watches previousPartsInfo[0], causing
stale uiSegments when deeper previous parts change; update useFetchAndFilter to
include the full previousPartsInfo array (e.g. playlist?.previousPartsInfo) in
the dependency list passed to useTracker so fetchAndFilter sees changes across
the whole chain, and also add that same full previousPartsInfo dependency to the
memoizedIsolatedAutorun (the block that computes uiSegments / calls
getActivePartInstancesMap) so the memoized computation invalidates when any
previous part is added/pruned; reference useFetchAndFilter, fetchAndFilter,
memoizedIsolatedAutorun and uiSegments when applying this change.
---
Nitpick comments:
In `@meteor/server/api/deviceTriggers/__tests__/TagsService.test.ts`:
- Around line 244-402: Tests duplicate cache setup between the two cases;
extract the repeated setup into a small helper (e.g. createAndPopulateMockCache
or buildTestCache) that accepts previousPartsInfo (array of { partInstanceId })
and returns the created testee and cache (or at least the cache and
showStyleBaseId) so both tests call that helper, then call
testee.updatePieceInstances(cache, showStyleBaseId) and assert via
testee.getTallyStateFromTags; update references to createTestee, cache
construction, ShowStyleBases insert, PieceInstances/PartInstances inserts to use
the helper to remove duplication.
In
`@packages/job-worker/src/blueprints/context/services/__tests__/PartAndPieceInstanceActionService.test.ts`:
- Around line 287-291: The helper currently only seeds a single previous part by
setting previousPartsInfo to previousPartInstance ?
[convertInfo(previousPartInstance)!] : [], so update the helper signature to
accept multiple previous parts (e.g., previousPartInstances or allow an array),
and change the RundownPlaylists.update call (still referenced as
jobContext.mockCollections.RundownPlaylists.update and playlistId) to set
previousPartsInfo to previousPartInstances?.map(convertInfo).filter(Boolean) ||
[] so tests can seed and exercise multi-previous-part behavior; keep convertInfo
usage and preserve the empty-array fallback when none supplied.
In `@packages/job-worker/src/playout/__tests__/resolvedPieces.test.ts`:
- Around line 972-1070: Tests rely on the ordering of
SelectedPartInstancesTimelineInfo.previous being most-recent-first but there's
no explicit comment; add a one-line clarifying comment or JSDoc stating that
SelectedPartInstancesTimelineInfo.previous is ordered most-recent-first (newest
first) so the cap computation in getResolvedPiecesForPartInstancesOnTimeline is
unambiguous. Update either the test file near the test block or the
SelectedPartInstancesTimelineInfo.previous property JSDoc (or type/interface
declaration) to include that single-line clarification.
In `@packages/job-worker/src/playout/lookahead/__tests__/lookahead.test.ts`:
- Around line 296-318: Add a test case that creates at least two previous
entries in the SelectedPartInstancesTimelineInfo (e.g. duplicate previousEntry
with different partInstance._id and different partTimes.nowInPart values), push
them into partInstancesInfo.previous in chronological order (oldest first), call
getLookeaheadObjects via runJobWithPlayoutModel as done now, and update the
expectedPrevious to be an array of the two mapped objects (using
previousEntry.partInstance, partTimes.nowInPart, pieceInstances,
calculatedTimings) in oldest-first order; finally assert via
expectLookaheadForLayerMock (and indirectly findLookaheadForLayer) that it was
invoked with previous equal to that ordered array to verify ordering and
multiple-previous handling for getLookeaheadObjects.
In `@packages/job-worker/src/playout/resolvedPieces.ts`:
- Around line 78-96: Document the ordering contract on
SelectedPartInstancesTimelineInfo.previous in timeline/generate.ts (JSDoc: array
is most-recent-first so previous[0] starts after previous[1], etc.). In
resolvedPieces.ts, make the cap calculation robust: when computing capEnd for
index i, search backward from i-1 to find the nearest previous entry with
partTimes.partStartTime and use that startTime as the cap (fallback to
currentPartStarted only if none found) to avoid collapsing caps when an earlier
entry was skipped; keep using resolvePrunedPieceInstance and
offsetResolvedStartAndCapDuration. Also replace allPreviousResolvedPieces =
allPreviousResolvedPieces.concat(resolved) with
allPreviousResolvedPieces.push(...resolved) for better append performance.
🪄 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: 0abdf53a-d511-410e-9490-c67dff22ec45
⛔ Files ignored due to path filters (2)
packages/job-worker/src/ingest/mosDevice/__tests__/__snapshots__/mosIngest.test.ts.snapis excluded by!**/*.snappackages/job-worker/src/playout/__tests__/__snapshots__/playout.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (58)
meteor/__mocks__/defaultCollectionObjects.tsmeteor/server/__tests__/cronjobs.test.tsmeteor/server/api/__tests__/externalMessageQueue.test.tsmeteor/server/api/__tests__/peripheralDevice.test.tsmeteor/server/api/deviceTriggers/TagsService.tsmeteor/server/api/deviceTriggers/__tests__/TagsService.test.tsmeteor/server/api/deviceTriggers/reactiveContentCacheForPieceInstances.tspackages/corelib/src/dataModel/RundownPlaylist/RundownPlaylist.tspackages/job-worker/src/__mocks__/defaultCollectionObjects.tspackages/job-worker/src/blueprints/context/OnTimelineGenerateContext.tspackages/job-worker/src/blueprints/context/services/__tests__/PartAndPieceInstanceActionService.test.tspackages/job-worker/src/events/__tests__/externalMessageQueue.test.tspackages/job-worker/src/ingest/__tests__/ingest.test.tspackages/job-worker/src/ingest/__tests__/syncChangesToPartInstance.test.tspackages/job-worker/src/ingest/__tests__/updateNext.test.tspackages/job-worker/src/ingest/commit.tspackages/job-worker/src/playout/__tests__/lib.tspackages/job-worker/src/playout/__tests__/resolvedPieces.test.tspackages/job-worker/src/playout/__tests__/tTimersJobs.test.tspackages/job-worker/src/playout/abPlayback/index.tspackages/job-worker/src/playout/lookahead/__tests__/findForLayer/basicBehavior.test.tspackages/job-worker/src/playout/lookahead/__tests__/findForLayer/constants.tspackages/job-worker/src/playout/lookahead/__tests__/findForLayer/orderedParts.test.tspackages/job-worker/src/playout/lookahead/__tests__/findForLayer/playoutStatePropagation.test.tspackages/job-worker/src/playout/lookahead/__tests__/findForLayer/searchDistance.test.tspackages/job-worker/src/playout/lookahead/__tests__/findForLayer/timing.test.tspackages/job-worker/src/playout/lookahead/__tests__/lookahead.test.tspackages/job-worker/src/playout/lookahead/__tests__/lookaheadOffset/lookaheadOffset.test.tspackages/job-worker/src/playout/lookahead/findForLayer.tspackages/job-worker/src/playout/lookahead/index.tspackages/job-worker/src/playout/model/PlayoutModel.tspackages/job-worker/src/playout/model/implementation/LoadPlayoutModel.tspackages/job-worker/src/playout/model/implementation/PlayoutModelImpl.tspackages/job-worker/src/playout/model/implementation/__tests__/PlayoutModelImpl.spec.tspackages/job-worker/src/playout/resolvedPieces.tspackages/job-worker/src/playout/setNext.tspackages/job-worker/src/playout/snapshot.tspackages/job-worker/src/playout/timeline/__tests__/rundown.test.tspackages/job-worker/src/playout/timeline/generate.tspackages/job-worker/src/playout/timeline/rundown.tspackages/job-worker/src/playout/timings/partPlayback.tspackages/job-worker/src/rundownPlaylists.tspackages/live-status-gateway/src/collections/partInstancesHandler.tspackages/live-status-gateway/src/collections/pieceInstancesHandler.tspackages/live-status-gateway/src/topics/__tests__/activePlaylist.spec.tspackages/live-status-gateway/src/topics/__tests__/utils.tspackages/webui/src/__mocks__/defaultCollectionObjects.tspackages/webui/src/client/lib/__tests__/rundownTiming.test.tspackages/webui/src/client/lib/rundownPlaylistUtil.tspackages/webui/src/client/ui/ClockView/DirectorScreen/DirectorScreen.tsxpackages/webui/src/client/ui/ClockView/PresenterScreen.tsxpackages/webui/src/client/ui/MediaStatus/MediaStatus.tsxpackages/webui/src/client/ui/RundownView.tsxpackages/webui/src/client/ui/RundownView/RundownTiming/RundownTimingProvider.tsxpackages/webui/src/client/ui/RundownView/RundownViewSubscriptions.tspackages/webui/src/client/ui/SegmentTimeline/Parts/SegmentTimelinePart.tsxpackages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsxpackages/webui/src/client/ui/Shelf/AdLibPanel.tsx
Comment on lines
15
to
20
| export interface PartInstanceAndPieceInstancesInfos { | ||
| previous?: PartInstanceAndPieceInstances | ||
| /** Oldest-first. Each entry is "on timeline" and contributes timed lookahead data. */ | ||
| previous: PartInstanceAndPieceInstances[] | ||
| current?: PartInstanceAndPieceInstances | ||
| next?: PartInstanceAndPieceInstances | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm caller ordering: previous should be oldest-first when passed to findLookaheadForLayer.
rg -nP --type=ts -C5 '\bfindLookaheadForLayer\s*\('
echo '---'
# Inspect how lookahead/index.ts builds the `previous` array.
fd -a 'index.ts' packages/job-worker/src/playout/lookahead
rg -nPU --type=ts -C5 '(?s)previous\s*:\s*\[' packages/job-worker/src/playout/lookahead/index.ts || true
rg -nP --type=ts -C3 '\bpreviousPartsInfo\b' packages/job-worker/src/playout/lookahead/Repository: Sofie-Automation/sofie-core
Length of output: 17596
🏁 Script executed:
# Read the relevant section of findForLayer.ts to see the branching logic
sed -n '1,80p' packages/job-worker/src/playout/lookahead/findForLayer.ts | cat -nRepository: Sofie-Automation/sofie-core
Length of output: 3235
🏁 Script executed:
# Read index.ts around the call site to see how partInstancesInfo is constructed
sed -n '180,240p' packages/job-worker/src/playout/lookahead/index.ts | cat -nRepository: Sofie-Automation/sofie-core
Length of output: 2721
🏁 Script executed:
# Search for where partInstancesInfo (specifically previous) is built
rg -n 'partInstancesInfo\s*=' packages/job-worker/src/playout/lookahead/index.ts -B 5 -A 10Repository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
# Search for previousPartsInfo to understand ordering context
rg -n 'previousPartsInfo' packages/job-worker/src/playout/lookahead/ -A 2 -B 2Repository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
# Find the function that contains the findLookaheadForLayer call and see the full context
rg -n 'partInstancesInfo' packages/job-worker/src/playout/lookahead/index.ts | head -20Repository: Sofie-Automation/sofie-core
Length of output: 1263
🏁 Script executed:
# Get more context around the partInstancesInfo0 variable to see construction
sed -n '100,220p' packages/job-worker/src/playout/lookahead/index.ts | cat -nRepository: Sofie-Automation/sofie-core
Length of output: 5686
🏁 Script executed:
# Search for previousPartsInfo with broader context to understand ordering
rg -n 'previousPartsInfo' packages/job-worker/src --type=ts -B 3 -A 3 | head -60Repository: Sofie-Automation/sofie-core
Length of output: 5560
JSDoc is accurate but the defensive branch for off-timeline entries is dead code in practice.
The interface's JSDoc assertion is correct: all entries in previous are hardcoded with onTimeline: true at index.ts line 27. However, the loop at lines 49–52 in findForLayer.ts still branches defensively on prevInfo.onTimeline, even though the else case (routing to res.future) can never occur for the previous array. This pattern makes sense for current and next (which have dynamic onTimeline values), but is dead code for previous.
Consider adding a note in the JSDoc to clarify that this field maintains the invariant that all entries are on-timeline, distinguishing it from the current and next fields:
✏️ Suggested JSDoc clarification
- /** Oldest-first. Each entry is "on timeline" and contributes timed lookahead data. */
+ /**
+ * Previous PartInstances ordered oldest-first to chain `previousPart` forward through each
+ * generated lookahead. All entries are on-timeline (never contribute future objects).
+ */Regarding ordering: this is already well-documented in index.ts lines 10–17, which explains the oldest-first requirement for timed-lookahead chain correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/job-worker/src/playout/lookahead/findForLayer.ts` around lines 15 -
20, Update the JSDoc for PartInstanceAndPieceInstancesInfos.previous to
explicitly state the invariant that every entry has onTimeline === true
(oldest-first), and then simplify findForLayer by removing the dead defensive
branch that checks prevInfo.onTimeline for entries from previous; instead either
assert prevInfo.onTimeline or unconditionally handle them as on-timeline (push
to the same branch you currently take when onTimeline is true) in the loop that
iterates previous (referencing the symbols
PartInstanceAndPieceInstancesInfos.previous, prevInfo, and the result buckets
like res.future/res.timed used in findForLayer).
Contributor
Author
There was a problem hiding this comment.
wasn't it hardcoded for the if (partInstancesInfo.current) block too?
There was a problem hiding this comment.
🧩 Analysis chain
Good question — let me check how current.onTimeline is set in index.ts.
🏁 Script executed:
#!/bin/bash
# Check how onTimeline is set for current, previous, and next in index.ts
rg -n 'onTimeline' packages/job-worker/src/playout/lookahead/index.ts -B 3 -A 2
Length of output: 1225
@ianshade, you're right — current is also hardcoded with onTimeline: true (line 138 in index.ts):
| Field | onTimeline value |
|---|---|
previous |
true (hardcoded) |
current |
true (hardcoded) |
next |
!!partInstancesInfo0.current?.partInstance?.part?.autoNext (dynamic) |
So the defensive onTimeline branch is dead code for both previous and current — only next has a meaningful dynamic value that drives the res.timed vs res.future split. My original comment was incomplete in flagging only previous. The suggested JSDoc clarification still holds, but the simplification opportunity extends to the current block as well.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/job-worker/src/playout/timeline/rundown.ts (1)
189-194:⚠️ Potential issue | 🟠 MajorLink the current first object to the first emitted previous part.
Line 510 skips previous entries that never started, but Line 193 still uses raw
previous[0]. In theprevious[0]skipped /previous[1]active case, the current first object points at a PartInstance that was not emitted into the timeline.Proposed fix
+ const previousPartForCurrentFirstObject = partInstancesInfo.previous.find( + (previousPartInfo) => previousPartInfo.partInstance.timings?.plannedStartedPlayback + )?.partInstance + const groupClasses: string[] = ['current_part'] timelineObjs.push( currentPartGroup, createPartGroupFirstObject( activePlaylist._id, partInstancesInfo.current.partInstance, currentPartGroup, - partInstancesInfo.previous[0]?.partInstance + previousPartForCurrentFirstObject ),Also applies to: 507-510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/playout/timeline/rundown.ts` around lines 189 - 194, The call to createPartGroupFirstObject currently uses partInstancesInfo.previous[0]?.partInstance which can reference a previous PartInstance that was never emitted/started; change it to locate the first previous entry that was actually emitted/started (the same selection logic used at lines ~507-510) and pass that PartInstance instead. In other words, replace the raw previous[0] usage with a search over partInstancesInfo.previous for the first emitted/started entry (same predicate as used elsewhere in this file) and supply its .partInstance to createPartGroupFirstObject while leaving partInstancesInfo.current.partInstance and currentPartGroup as-is. Ensure the same fix is applied to the other occurrences noted (around lines 507–510).
🧹 Nitpick comments (1)
packages/job-worker/src/playout/lookahead/__tests__/findForLayer/searchDistance.test.ts (1)
23-46: Consider mocking the 4th (nextFuture) call explicitly.With
mockReset()inbeforeEach, the defaultmockImplementation(() => [])set at module load is cleared between tests. The chain here only defines 3mockReturnValueOncevalues, so the 4th call (fornextFuture) returnsundefinedrather than[]. It happens to work becausesearchDistance = 0causes the result to be discarded viares.future, but adding an explicit.mockReturnValueOnce([] as any)(or a.mockReturnValue([])at the head of the chain) would make intent clearer and guard against future regressions infindLookaheadForLayerthat might dereference the returned array.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/playout/lookahead/__tests__/findForLayer/searchDistance.test.ts` around lines 23 - 46, The test for searchDistance = 0 only sets three mockReturnValueOnce values for findLookaheadObjectsForPartMock but the beforeEach mockReset clears the module-level default, so the 4th call (used for nextFuture) returns undefined; update the test to explicitly mock the 4th call by adding another .mockReturnValueOnce([] as any) (or replace the chain with a leading .mockReturnValue([]) plus specific .mockReturnValueOnce overrides) so findLookaheadObjectsForPartMock always returns an array for the nextFuture invocation when calling findLookaheadForLayer.
🤖 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/job-worker/src/playout/snapshot.ts`:
- Around line 719-735: The migration code builds migratedInfo from fullOldKey
but leaves the legacy key on snapshot.playlist, risking stale data overwriting
modern fields later; update the logic in snapshot migration (the block using
fullOldKey, snapshot.playlist, partInstanceOldRundownIdMap) to first check for
and prefer existing modern fields (previousPartsInfo, nextPartInfo,
currentPartInfo) — only migrate if the modern field is absent — and then remove
the legacy key (delete (snapshot.playlist as any)[fullOldKey]) after migrating
so the old property is not persisted.
---
Outside diff comments:
In `@packages/job-worker/src/playout/timeline/rundown.ts`:
- Around line 189-194: The call to createPartGroupFirstObject currently uses
partInstancesInfo.previous[0]?.partInstance which can reference a previous
PartInstance that was never emitted/started; change it to locate the first
previous entry that was actually emitted/started (the same selection logic used
at lines ~507-510) and pass that PartInstance instead. In other words, replace
the raw previous[0] usage with a search over partInstancesInfo.previous for the
first emitted/started entry (same predicate as used elsewhere in this file) and
supply its .partInstance to createPartGroupFirstObject while leaving
partInstancesInfo.current.partInstance and currentPartGroup as-is. Ensure the
same fix is applied to the other occurrences noted (around lines 507–510).
---
Nitpick comments:
In
`@packages/job-worker/src/playout/lookahead/__tests__/findForLayer/searchDistance.test.ts`:
- Around line 23-46: The test for searchDistance = 0 only sets three
mockReturnValueOnce values for findLookaheadObjectsForPartMock but the
beforeEach mockReset clears the module-level default, so the 4th call (used for
nextFuture) returns undefined; update the test to explicitly mock the 4th call
by adding another .mockReturnValueOnce([] as any) (or replace the chain with a
leading .mockReturnValue([]) plus specific .mockReturnValueOnce overrides) so
findLookaheadObjectsForPartMock always returns an array for the nextFuture
invocation when calling findLookaheadForLayer.
🪄 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: c3f34eff-cb32-4348-852b-457e0ddc1bf4
📒 Files selected for processing (8)
packages/job-worker/src/playout/__tests__/lib.tspackages/job-worker/src/playout/lookahead/__tests__/findForLayer/constants.tspackages/job-worker/src/playout/lookahead/__tests__/findForLayer/searchDistance.test.tspackages/job-worker/src/playout/lookahead/__tests__/findForLayer/timing.test.tspackages/job-worker/src/playout/snapshot.tspackages/job-worker/src/playout/timeline/__tests__/rundown.test.tspackages/job-worker/src/playout/timeline/rundown.tspackages/live-status-gateway/src/collections/pieceInstancesHandler.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/job-worker/src/playout/lookahead/tests/findForLayer/constants.ts
- packages/job-worker/src/playout/tests/lib.ts
- packages/live-status-gateway/src/collections/pieceInstancesHandler.ts
- packages/job-worker/src/playout/lookahead/tests/findForLayer/timing.test.ts
Comment on lines
719
to
735
| const fullOldKey = `${property}PartInstanceId` | ||
| if (fullOldKey in snapshot.playlist) { | ||
| const oldId = (snapshot.playlist as any)[fullOldKey] as PartInstanceId | ||
| snapshot.playlist.currentPartInfo = { | ||
| const migratedInfo = { | ||
| partInstanceId: oldId, | ||
| rundownId: partInstanceOldRundownIdMap.get(oldId) || protectString(''), | ||
| manuallySelected: false, | ||
| consumesQueuedSegmentId: false, | ||
| } | ||
| if (property === 'previous') { | ||
| snapshot.playlist.previousPartsInfo = [migratedInfo] | ||
| } else if (property === 'next') { | ||
| snapshot.playlist.nextPartInfo = migratedInfo | ||
| } else { | ||
| snapshot.playlist.currentPartInfo = migratedInfo | ||
| } | ||
| } |
There was a problem hiding this comment.
Drop consumed legacy selection ids before saving the restored playlist.
fullOldKey is migrated but never deleted, and snapshot.playlist is saved as-is later. A restored legacy snapshot can keep a stale previousPartInstanceId; on a later restore, Line 729 can then overwrite valid previousPartsInfo with that stale single id. Prefer existing modern fields when both shapes exist, then delete the legacy key.
Proposed fix
const fullOldKey = `${property}PartInstanceId`
if (fullOldKey in snapshot.playlist) {
const oldId = (snapshot.playlist as any)[fullOldKey] as PartInstanceId
const migratedInfo = {
partInstanceId: oldId,
rundownId: partInstanceOldRundownIdMap.get(oldId) || protectString(''),
manuallySelected: false,
consumesQueuedSegmentId: false,
}
if (property === 'previous') {
- snapshot.playlist.previousPartsInfo = [migratedInfo]
+ if (!Array.isArray(snapshot.playlist.previousPartsInfo) || snapshot.playlist.previousPartsInfo.length === 0) {
+ snapshot.playlist.previousPartsInfo = [migratedInfo]
+ }
} else if (property === 'next') {
- snapshot.playlist.nextPartInfo = migratedInfo
+ if (!snapshot.playlist.nextPartInfo) snapshot.playlist.nextPartInfo = migratedInfo
} else {
- snapshot.playlist.currentPartInfo = migratedInfo
+ if (!snapshot.playlist.currentPartInfo) snapshot.playlist.currentPartInfo = migratedInfo
}
+ delete (snapshot.playlist as any)[fullOldKey]
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/job-worker/src/playout/snapshot.ts` around lines 719 - 735, The
migration code builds migratedInfo from fullOldKey but leaves the legacy key on
snapshot.playlist, risking stale data overwriting modern fields later; update
the logic in snapshot migration (the block using fullOldKey, snapshot.playlist,
partInstanceOldRundownIdMap) to first check for and prefer existing modern
fields (previousPartsInfo, nextPartInfo, currentPartInfo) — only migrate if the
modern field is absent — and then remove the legacy key (delete
(snapshot.playlist as any)[fullOldKey]) after migrating so the old property is
not persisted.
Contributor
Author
There was a problem hiding this comment.
is this a regression?
There was a problem hiding this comment.
@ianshade — not a regression in the strict sense. The "legacy key never deleted" behaviour was already present before this PR. However, the pre-existing code had a separate, arguably worse bug: the migration block always wrote to snapshot.playlist.currentPartInfo regardless of which property was being handled, so the previous branch was essentially broken. Because previousPartsInfo didn't exist yet, there was nothing to overwrite.
This PR correctly introduces the if/else branching and populates previousPartsInfo, but because the legacy key (previousPartInstanceId) is still not removed from snapshot.playlist, a second restore of a snapshot that was itself produced by a prior restore could overwrite a valid previousPartsInfo with the migrated single-entry value. That scenario wasn't reachable before.
So in short:
- The root cause (not deleting the legacy key) is pre-existing.
- The PR makes it newly observable by finally doing something meaningful with the
previousmigration path. - It's worth fixing regardless, since the delete is cheap and prevents a subtle double-restore footgun.
🧠 Learnings used
Learnt from: Julusian
Repo: Sofie-Automation/sofie-core PR: 1698
File: packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx:266-290
Timestamp: 2026-03-24T16:58:20.370Z
Learning: In `packages/webui/src/client/ui/SegmentTimeline/SegmentContextMenu.tsx`, the `UserEditOperationMenuItems` for parts and pieces are intentionally rendered outside the `!part.instance.part.invalid` guard. This is by design (SOFIE-283): the properties editor must be accessible for invalid parts so users can edit their properties to fix the invalid state. Only the "Set part as Next" / "play from here" menu items are suppressed for invalid parts.
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 TV 2 Norge.
Type of Contribution
This is a:
Bug fix (handling an edge case that probably no one encountered before us)
Current Behavior
PartInstance expected to be kept alive using e.g. previousPartKeepaliveDuration, sometimes would prematurely disappear from the timeline, if two following PartInstances started their playback.
Previous partInstance(s) were not properly considered when generating lookaheads. When a take occurred, and a partinstance became previous while its timed piece was yet to start, there would be no timed lookahead generated from that piece.
There are times when e.g. VT playback should continue into the taken part, so that a transition can safely execute till completion. Due to our transitions being separate Parts, we end up in a scenario when at a certain point three parts need to be active. Unfortunately Sofie supports only two actively playing Parts at any given time (only previous and current + next for lookahads, and briefly during autoNext, which is not enough). In our case, during a transition, the to Part quickly becomes current, whereas the transition Part becomes previous, that means the from Part no longer is considered for timeline generation, yet it is needed for its VB contents to keep playing until the transition finishes. Currently this results in either black frames during a transition, or, in conjunction with preroll applied to some pieces, holes on the PGM layer mapping.
New Behavior
Keeps track of more than one previous part, so that they are included in the timeline and ab-logic, until stopped playback;
timing properties are used for pruning, but an arbitrary limit of 10 is in place, to avoid too much data piling up if blueprints did something very wrong
Testing
Affected areas
Time Frame
Other Information
Status