fix: looping behavior and timing calculations in QuickLoopService for looping Rundown Playlists#1704
Merged
jstarpl merged 4 commits intoSofie-Automation:mainfrom Apr 10, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPropagates an "entire playlist looping" flag through timing and multiple UI components and updates QuickLoopService to preserve locked quick-loop props (returns cloned quickLoop with Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Web UI
participant Resolver as RundownResolver
participant Timing as RundownTiming
participant Model as Playout Model
participant Service as QuickLoopService
User->>UI: open playlist / interact
UI->>Resolver: getIsLoopRunning(playlist)
UI->>Resolver: getIsEntirePlaylistLooping(playlist)
UI->>Timing: request countdowns (pass entirePlaylistIsLooping)
Timing-->>UI: countdowns (nullify/skip only if !entirePlaylistIsLooping)
UI->>Model: activate/reset/deactivate playlist
Model->>Service: getUpdatedPropsByClearingMarkers(...)
Service-->>Model: return cloned quickLoop (if locked -> running=false)
Model-->>UI: updated playlist state
UI->>Resolver: recompute loop flags
UI->>UI: render parts with isEntirePlaylistLooping affecting CSS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jstarpl
added
Contribution
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
jstarpl
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/webui/src/client/lib/rundownTiming.ts (1)
139-140: Consider centralizing the whole-playlist-loop special case.
findPartInstancesInQuickLoop()still modelsPLAYLIST -> PLAYLISTlooping as an empty membership map, soupdateDurations()now needs an extra side-channel to recover the right behavior. A shared helper, or normalizing thepartsInQuickLoopcontract in one place, would make the timing math and the three UI views much less likely to drift again.Also applies to: 510-564
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/lib/rundownTiming.ts` around lines 139 - 140, The special-case for an entire-playlist loop is handled inconsistently: findPartInstancesInQuickLoop models PLAYLIST->PLAYLIST looping as an empty membership map while updateDurations uses the side-channel entirePlaylistIsLooping; centralize this by normalizing the partsInQuickLoop contract in one place (either inside findPartInstancesInQuickLoop or a new helper like normalizePartsInQuickLoop) so that PLAYLIST->PLAYLIST returns an explicit mapping instead of an empty map, then remove the extra entirePlaylistIsLooping/side-channel usage in updateDurations and callers (including where partsInQuickLoop is read) so all timing math and UI views consume the unified representation consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/webui/src/client/lib/rundownTiming.ts`:
- Around line 139-140: The special-case for an entire-playlist loop is handled
inconsistently: findPartInstancesInQuickLoop models PLAYLIST->PLAYLIST looping
as an empty membership map while updateDurations uses the side-channel
entirePlaylistIsLooping; centralize this by normalizing the partsInQuickLoop
contract in one place (either inside findPartInstancesInQuickLoop or a new
helper like normalizePartsInQuickLoop) so that PLAYLIST->PLAYLIST returns an
explicit mapping instead of an empty map, then remove the extra
entirePlaylistIsLooping/side-channel usage in updateDurations and callers
(including where partsInQuickLoop is read) so all timing math and UI views
consume the unified representation consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59a378b5-a634-4cc1-9d2b-85424a354a86
📒 Files selected for processing (7)
packages/job-worker/src/playout/model/services/QuickLoopService.tspackages/webui/src/client/lib/rundownTiming.tspackages/webui/src/client/ui/SegmentList/LinePart.tsxpackages/webui/src/client/ui/SegmentList/SegmentList.tsxpackages/webui/src/client/ui/SegmentStoryboard/SegmentStoryboard.tsxpackages/webui/src/client/ui/SegmentStoryboard/StoryboardPart.tsxpackages/webui/src/client/ui/SegmentTimeline/Parts/SegmentTimelinePart.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/job-worker/src/playout/model/implementation/__tests__/PlayoutModelImpl.spec.ts (1)
242-255: Consider assertingquickLoop.runningremainsfalsethrough lifecycle calls.The test already validates
lockedand marker types. Addingrunning === falseassertions afteractivatePlaylist,resetPlaylist, anddeactivatePlaylistwould better pin the intended locked-loop behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/playout/model/implementation/__tests__/PlayoutModelImpl.spec.ts` around lines 242 - 255, Add assertions that quickLoop.running stays false after the lifecycle calls: after calling model.activatePlaylist(false), model.resetPlaylist(true), and model.deactivatePlaylist() assert model.playlist.quickLoop?.running === false in each place; update the test block that currently checks quickLoop.locked and marker types (and references QuickLoopMarkerType.PLAYLIST) to include these running === false checks so the intended locked-but-not-running behavior is explicitly validated.
🤖 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/model/implementation/__tests__/PlayoutModelImpl.spec.ts`:
- Line 227: The test calls jest.setSystemTime(TIME_NOW) inside the "quickLoop
for locked playlist loops" describe block but never enables fake timers; update
that describe block's beforeEach to call jest.useFakeTimers() before calling
jest.setSystemTime(TIME_NOW) (and ensure the corresponding afterEach restores
timers with jest.useRealTimers()); look for the describe titled "quickLoop for
locked playlist loops", the beforeEach that references TIME_NOW and
jest.setSystemTime, and add the jest.useFakeTimers() call there.
---
Nitpick comments:
In
`@packages/job-worker/src/playout/model/implementation/__tests__/PlayoutModelImpl.spec.ts`:
- Around line 242-255: Add assertions that quickLoop.running stays false after
the lifecycle calls: after calling model.activatePlaylist(false),
model.resetPlaylist(true), and model.deactivatePlaylist() assert
model.playlist.quickLoop?.running === false in each place; update the test block
that currently checks quickLoop.locked and marker types (and references
QuickLoopMarkerType.PLAYLIST) to include these running === false checks so the
intended locked-but-not-running behavior is explicitly validated.
🪄 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: 2e0ce276-15f7-4809-84f5-492c6a489c96
📒 Files selected for processing (1)
packages/job-worker/src/playout/model/implementation/__tests__/PlayoutModelImpl.spec.ts
jstarpl
force-pushed
the
contrib/fix/looping-rundown
branch
2 times, most recently
from
9cdd590 to
321d21e
Compare
jstarpl
force-pushed
the
contrib/fix/looping-rundown
branch
from
321d21e to
174c24f
Compare
…opsByClearingMarkers
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/webui/src/client/lib/rundownTiming.ts`:
- Line 139: The flag entirePlaylistIsLooping should reflect an active loop, not
just marker coverage: replace the current use of
isEntirePlaylistLooping(playlist) with a conjunction of isLoopRunning(playlist)
&& isEntirePlaylistLooping(playlist) where the variable entirePlaylistIsLooping
is defined/used so that loop behavior is gated by both the markers spanning the
playlist and the loop actually running (use the isLoopRunning and
isEntirePlaylistLooping helpers and update any downstream logic that relies on
entirePlaylistIsLooping accordingly).
🪄 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: 528581cc-d01e-4595-942c-65923c2b81ca
📒 Files selected for processing (8)
packages/job-worker/src/playout/model/implementation/__tests__/PlayoutModelImpl.spec.tspackages/job-worker/src/playout/model/services/QuickLoopService.tspackages/webui/src/client/lib/rundownTiming.tspackages/webui/src/client/ui/SegmentList/LinePart.tsxpackages/webui/src/client/ui/SegmentList/SegmentList.tsxpackages/webui/src/client/ui/SegmentStoryboard/SegmentStoryboard.tsxpackages/webui/src/client/ui/SegmentStoryboard/StoryboardPart.tsxpackages/webui/src/client/ui/SegmentTimeline/Parts/SegmentTimelinePart.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/job-worker/src/playout/model/implementation/tests/PlayoutModelImpl.spec.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/webui/src/client/ui/SegmentStoryboard/StoryboardPart.tsx
- packages/webui/src/client/ui/SegmentStoryboard/SegmentStoryboard.tsx
- packages/webui/src/client/ui/SegmentList/SegmentList.tsx
| let lastSegmentIds: { segmentId: SegmentId; segmentPlayoutId: SegmentPlayoutId } | undefined = undefined | ||
| let nextRundownAnchor: number | undefined = undefined | ||
|
|
||
| const entirePlaylistIsLooping = isEntirePlaylistLooping(playlist) |
There was a problem hiding this comment.
Gate the full-playlist loop flag on running, not just marker coverage.
isEntirePlaylistLooping() only tells you that the loop markers span the playlist. After this PR preserves locked quick-loop props with running: false, Line 510, Line 542, and Line 564 will still keep pre-current parts countdownable during reset/deactivation, even though the loop is no longer active. Please derive this flag from both isLoopRunning(playlist) and isEntirePlaylistLooping(playlist).
Suggested fix
- const entirePlaylistIsLooping = isEntirePlaylistLooping(playlist)
+ const entirePlaylistLoopIsRunning =
+ isLoopRunning(playlist) && isEntirePlaylistLooping(playlist)
...
- if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistIsLooping) {
+ if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistLoopIsRunning) {
...
- if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistIsLooping) {
+ if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistLoopIsRunning) {
...
- if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistIsLooping) continue
+ if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistLoopIsRunning) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/lib/rundownTiming.ts` at line 139, The flag
entirePlaylistIsLooping should reflect an active loop, not just marker coverage:
replace the current use of isEntirePlaylistLooping(playlist) with a conjunction
of isLoopRunning(playlist) && isEntirePlaylistLooping(playlist) where the
variable entirePlaylistIsLooping is defined/used so that loop behavior is gated
by both the markers spanning the playlist and the loop actually running (use the
isLoopRunning and isEntirePlaylistLooping helpers and update any downstream
logic that relies on entirePlaylistIsLooping accordingly).
174c24f to
f0201af
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/webui/src/client/lib/rundownTiming.ts (1)
143-143:⚠️ Potential issue | 🟠 MajorGate full-playlist behavior on active loop, not marker coverage alone.
At Line 143,
entirePlaylistIsLoopingis derived without checking whether looping is currently running. That leaks into Lines 514/546/568 and can keep pre-current countdown logic active when markers exist but loop runtime state is off (e.g., after reset/deactivate with locked markers).Suggested fix
- const entirePlaylistIsLooping = isEntirePlaylistLooping(playlist) + const loopIsRunning = isLoopRunning(playlist) + const entirePlaylistLoopIsRunning = loopIsRunning && isEntirePlaylistLooping(playlist) ... - if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistIsLooping) { + if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistLoopIsRunning) { ... - if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistIsLooping) { + if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistLoopIsRunning) { ... - if (isLoopRunning(playlist)) { + if (loopIsRunning) { ... - if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistIsLooping) continue + if (!partsInQuickLoop[unprotectString(this.linearParts[i][0])] && !entirePlaylistLoopIsRunning) continue#!/bin/bash set -euo pipefail echo "Locate stateCacheResolver implementation:" fd -i 'stateCacheResolver.*' echo echo "Inspect helper definitions:" rg -n -C3 'isEntirePlaylistLooping|isLoopRunning' $(fd -i 'stateCacheResolver.*') echo echo "Inspect current rundownTiming call sites:" rg -n -C2 'entirePlaylistIsLooping|entirePlaylistLoopIsRunning|isLoopRunning\(playlist\)' packages/webui/src/client/lib/rundownTiming.tsAlso applies to: 514-515, 546-548, 568-568
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/lib/rundownTiming.ts` at line 143, entirePlaylistIsLooping is currently set from isEntirePlaylistLooping(playlist) without checking runtime loop state, which lets pre-current countdown logic remain active when markers exist but looping is not running; change the assignment to include the run-state check (e.g., set entirePlaylistIsLooping = isEntirePlaylistLooping(playlist) && isLoopRunning(playlist) or use the resolver-provided entirePlaylistLoopIsRunning flag), and update the dependent uses (the places that reference entirePlaylistIsLooping in this file) so they are gated by the loop-running condition rather than marker coverage alone.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/webui/src/client/lib/rundownTiming.ts`:
- Line 143: entirePlaylistIsLooping is currently set from
isEntirePlaylistLooping(playlist) without checking runtime loop state, which
lets pre-current countdown logic remain active when markers exist but looping is
not running; change the assignment to include the run-state check (e.g., set
entirePlaylistIsLooping = isEntirePlaylistLooping(playlist) &&
isLoopRunning(playlist) or use the resolver-provided entirePlaylistLoopIsRunning
flag), and update the dependent uses (the places that reference
entirePlaylistIsLooping in this file) so they are gated by the loop-running
condition rather than marker coverage alone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4045cc2b-def9-4336-a299-8e26783f1dc4
📒 Files selected for processing (8)
packages/job-worker/src/playout/model/implementation/__tests__/PlayoutModelImpl.spec.tspackages/job-worker/src/playout/model/services/QuickLoopService.tspackages/webui/src/client/lib/rundownTiming.tspackages/webui/src/client/ui/SegmentList/LinePart.tsxpackages/webui/src/client/ui/SegmentList/SegmentList.tsxpackages/webui/src/client/ui/SegmentStoryboard/SegmentStoryboard.tsxpackages/webui/src/client/ui/SegmentStoryboard/StoryboardPart.tsxpackages/webui/src/client/ui/SegmentTimeline/Parts/SegmentTimelinePart.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/webui/src/client/ui/SegmentStoryboard/StoryboardPart.tsx
- packages/webui/src/client/ui/SegmentList/SegmentList.tsx
- packages/webui/src/client/ui/SegmentList/LinePart.tsx
- packages/webui/src/client/ui/SegmentStoryboard/SegmentStoryboard.tsx
- packages/job-worker/src/playout/model/implementation/tests/PlayoutModelImpl.spec.ts
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 NRK.
Type of Contribution
This is a:
Bug fix
Current Behavior
Countdowns for Parts before the current playing part don't show up if the entire playlist is looping. Looping playlist state is cleared when a Playlist is reset or deactivated.
New Behavior
Countdowns for Parts before the current playing Part are correctly shown and calculated for entire looping playlists. Looping playlist state is kept across resets and deactivations.
Testing
Affected areas
Time Frame
This Bug Fix is critical for us, please review and merge it as soon as possible.
Other Information
Status