feat: partInstances invalid state SOFIE-317 (#69)#1732
Open
Julusian wants to merge 1 commit into
Open
Conversation
Co-authored-by: Jonas Hummelstrand <jonas@hummelstrand.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
WalkthroughThis pull request extends the PartInstance system to support runtime validation failures through a new Changes
Sequence DiagramsequenceDiagram
participant Blueprint as Blueprint<br/>(Action Handler)
participant Context as Context<br/>(updatePartInstance)
participant Service as Action Service
participant Model as PartInstance<br/>Model
participant Publication as Segment Notes<br/>Publication
participant UI as UI<br/>Component
Blueprint->>Context: updatePartInstance(part, props, instanceProps)
Context->>Service: updatePartInstance(part, props, instanceProps)
Service->>Model: setInvalidReason(reason)
Model-->>Service: invalidReason updated
Service-->>Context: IBlueprintPartInstance (updated)
Context-->>Blueprint: IBlueprintPartInstance
Note over Model: invalidReason persisted<br/>on PartInstance
Model-->>Publication: observe invalidReason change
Publication->>Publication: generateNotesForSegment()
Publication->>Publication: emit UISegmentPartNote
Publication-->>UI: notes published
UI->>UI: getEffectiveInvalidReason()
UI->>UI: render with<br/>invalid styling
UI-->>Blueprint: visual feedback of invalid state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/webui/src/client/ui/SegmentTimeline/Parts/InvalidPartCover.tsx (1)
43-45:⚠️ Potential issue | 🟡 MinorTypo in TODO comment.
Line 44 has "TODOD" instead of "TODO".
🔧 Proposed fix
return ( <div className={className} ref={element} onMouseEnter={onMouseEnter} onMouseLeave={onMouseLeave}> - {/* TODOD - add back hover with warnings */} + {/* TODO - add back hover with warnings */} </div> )🤖 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/InvalidPartCover.tsx` around lines 43 - 45, Fix the typo in the inline comment inside the InvalidPartCover component: change "TODOD - add back hover with warnings" to "TODO - add back hover with warnings" so the TODO is correctly recognized; locate the comment within the JSX block that renders the div with props className, ref={element}, onMouseEnter and onMouseLeave and update the text accordingly.packages/job-worker/src/blueprints/context/SyncIngestUpdateToPartInstanceContext.ts (1)
208-244:⚠️ Potential issue | 🟠 MajorMissing validation:
invalidReasonshould only be allowed on 'next' PartInstance.Unlike
PartAndPieceInstanceActionService.updatePartInstance(which validates thatinvalidReasoncan only be set on 'next') and unlikeremovePartInstancein the same class (which validatesplayStatus === 'next'), this method allows settinginvalidReasonfor anyplayStatus('previous', 'current', or 'next'). This inconsistency could allow blueprints to incorrectly setinvalidReasonon the current or previous part instance during ingest sync.🐛 Proposed fix to add validation
if (playoutUpdatePartInstance) { + if (this.playStatus !== 'next') { + throw new Error(`Can only set invalidReason on the next PartInstance`) + } this.#partInstance.setInvalidReason(playoutUpdatePartInstance.invalidReason) instancePropsUpdated = true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/blueprints/context/SyncIngestUpdateToPartInstanceContext.ts` around lines 208 - 244, The updatePartInstance method allows setting invalidReason for any playStatus; add a guard so invalidReason can only be set when the target PartInstance is the 'next' part. After creating playoutUpdatePartInstance (convertPartialBlueprintMutatablePartInstanceToCore) and before calling this.#partInstance.setInvalidReason(...), check playoutUpdatePartInstance.invalidReason and if present assert this.#partInstance.partInstance.playStatus === 'next' (throw an Error otherwise), mirroring the validation used in removePartInstance/PartAndPieceInstanceActionService.updatePartInstance; only call setInvalidReason when the check passes.
🧹 Nitpick comments (2)
packages/job-worker/src/playout/take.ts (1)
233-235: Consider adding an appropriate HTTP status code.Other take-blocking errors in this file use specific HTTP status codes (e.g.,
TakeBlockedDurationuses 425,TakeFromIncorrectPartuses 412). Without a status code, this will default to 500 (Internal Server Error), which doesn't accurately represent a user-correctable precondition failure.Proposed fix to add status code
if (takePartInstance.partInstance.invalidReason) { - throw UserError.create(UserErrorMessage.TakePartInstanceInvalid) + throw UserError.create(UserErrorMessage.TakePartInstanceInvalid, undefined, 412) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/job-worker/src/playout/take.ts` around lines 233 - 235, The throw for takePartInstance.partInstance.invalidReason currently creates a UserError without an HTTP status, causing a 500; update the throw to include an appropriate status (use 412 Precondition Failed to match other precondition-style take errors) by passing the status into UserError.create for UserErrorMessage.TakePartInstanceInvalid (e.g., add the status option when calling UserError.create in the block that checks takePartInstance.partInstance.invalidReason).packages/webui/src/client/styles/rundownView.scss (1)
1326-1376: Consider extracting a mixin to reduce duplication.The
.segment-timeline__part__invalid-coverand.segment-timeline__part__invalid-part-instance-coverclasses are nearly identical, differing only in stripe spacing (5px vs 6px). A mixin could reduce duplication.♻️ Optional refactor using a mixin
`@mixin` invalid-cover-base($stripe-width) { position: absolute; top: 0; left: 1px; bottom: 0; right: 1px; z-index: 10; pointer-events: all; mix-blend-mode: color-burn; background-image: repeating-linear-gradient( 45deg, var(--invalid-reason-color-transparent) 0%, var(--invalid-reason-color-transparent) $stripe-width, var(--invalid-reason-color-opaque) $stripe-width, var(--invalid-reason-color-opaque) 8px ), repeating-linear-gradient( -45deg, var(--invalid-reason-color-transparent) 0%, var(--invalid-reason-color-transparent) $stripe-width, var(--invalid-reason-color-opaque) $stripe-width, var(--invalid-reason-color-opaque) 8px ); } .segment-timeline__part__invalid-cover { `@include` invalid-cover-base(5px); } .segment-timeline__part__invalid-part-instance-cover { `@include` invalid-cover-base(6px); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/webui/src/client/styles/rundownView.scss` around lines 1326 - 1376, The two CSS classes .segment-timeline__part__invalid-cover and .segment-timeline__part__invalid-part-instance-cover duplicate most properties except stripe spacing; extract a SCSS mixin (e.g., invalid-cover-base($stripe-width)) that contains the shared positioning, z-index, pointer-events, mix-blend-mode and the two repeating-linear-gradient definitions parameterized by the stripe width, then replace each class body to include that mixin with 5px and 6px respectively (refer to the class names .segment-timeline__part__invalid-cover and .segment-timeline__part__invalid-part-instance-cover and the new mixin name invalid-cover-base).
🤖 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/ui/SegmentStoryboard/StoryboardPart.tsx`:
- Around line 130-132: The segment-end marker visibility is still using the
static flag part.instance.part.invalid instead of the runtime-aware isInvalid;
update the guard that currently checks !part.instance.part.invalid to use
!isInvalid so the segment-end marker follows the same runtime-invalid logic used
elsewhere (see getEffectiveInvalidReason and isPartInstanceInvalid usages) and
keep visuals consistent across StoryboardPart.
In `@packages/webui/src/client/ui/SegmentTimeline/Parts/SegmentTimelinePart.tsx`:
- Around line 691-693: Several UI branches in SegmentTimelinePart still check
the planned-only flag innerPart.invalid instead of the runtime-aware
isInvalid/effectiveInvalidReason; this lets runtime-invalid parts render as
next/auto-next and show end-of-segment affordances. Update all those branches to
use the computed isInvalid (and where relevant effectiveInvalidReason) instead
of innerPart.invalid — specifically replace checks of innerPart.invalid in the
conditional logic around next/auto-next rendering and inside renderEndOfSegment
with isInvalid so runtime invalidity flows through; ensure the same symbols
(isPartInstanceInvalid, getEffectiveInvalidReason, isInvalid,
effectiveInvalidReason, renderEndOfSegment, innerPart) are used to locate and
update the conditionals.
---
Outside diff comments:
In
`@packages/job-worker/src/blueprints/context/SyncIngestUpdateToPartInstanceContext.ts`:
- Around line 208-244: The updatePartInstance method allows setting
invalidReason for any playStatus; add a guard so invalidReason can only be set
when the target PartInstance is the 'next' part. After creating
playoutUpdatePartInstance (convertPartialBlueprintMutatablePartInstanceToCore)
and before calling this.#partInstance.setInvalidReason(...), check
playoutUpdatePartInstance.invalidReason and if present assert
this.#partInstance.partInstance.playStatus === 'next' (throw an Error
otherwise), mirroring the validation used in
removePartInstance/PartAndPieceInstanceActionService.updatePartInstance; only
call setInvalidReason when the check passes.
In `@packages/webui/src/client/ui/SegmentTimeline/Parts/InvalidPartCover.tsx`:
- Around line 43-45: Fix the typo in the inline comment inside the
InvalidPartCover component: change "TODOD - add back hover with warnings" to
"TODO - add back hover with warnings" so the TODO is correctly recognized;
locate the comment within the JSX block that renders the div with props
className, ref={element}, onMouseEnter and onMouseLeave and update the text
accordingly.
---
Nitpick comments:
In `@packages/job-worker/src/playout/take.ts`:
- Around line 233-235: The throw for takePartInstance.partInstance.invalidReason
currently creates a UserError without an HTTP status, causing a 500; update the
throw to include an appropriate status (use 412 Precondition Failed to match
other precondition-style take errors) by passing the status into
UserError.create for UserErrorMessage.TakePartInstanceInvalid (e.g., add the
status option when calling UserError.create in the block that checks
takePartInstance.partInstance.invalidReason).
In `@packages/webui/src/client/styles/rundownView.scss`:
- Around line 1326-1376: The two CSS classes
.segment-timeline__part__invalid-cover and
.segment-timeline__part__invalid-part-instance-cover duplicate most properties
except stripe spacing; extract a SCSS mixin (e.g.,
invalid-cover-base($stripe-width)) that contains the shared positioning,
z-index, pointer-events, mix-blend-mode and the two repeating-linear-gradient
definitions parameterized by the stripe width, then replace each class body to
include that mixin with 5px and 6px respectively (refer to the class names
.segment-timeline__part__invalid-cover and
.segment-timeline__part__invalid-part-instance-cover and the new mixin name
invalid-cover-base).
🪄 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: f92b2852-1ffe-4766-a598-81c1aa6d5e73
📒 Files selected for processing (33)
meteor/server/publications/segmentPartNotesUI/__tests__/generateNotesForSegment.test.tsmeteor/server/publications/segmentPartNotesUI/__tests__/publication.test.tsmeteor/server/publications/segmentPartNotesUI/generateNotesForSegment.tsmeteor/server/publications/segmentPartNotesUI/publication.tsmeteor/server/publications/segmentPartNotesUI/reactiveContentCache.tsmeteor/server/publications/segmentPartNotesUI/rundownContentObserver.tspackages/blueprints-integration/src/context/onSetAsNextContext.tspackages/blueprints-integration/src/context/partsAndPieceActionContext.tspackages/blueprints-integration/src/context/syncIngestChangesContext.tspackages/blueprints-integration/src/documents/partInstance.tspackages/corelib/src/dataModel/PartInstance.tspackages/corelib/src/error.tspackages/job-worker/src/blueprints/__tests__/context-OnSetAsNextContext.test.tspackages/job-worker/src/blueprints/__tests__/context-OnTakeContext.test.tspackages/job-worker/src/blueprints/__tests__/context-adlibActions.test.tspackages/job-worker/src/blueprints/context/OnSetAsNextContext.tspackages/job-worker/src/blueprints/context/OnTakeContext.tspackages/job-worker/src/blueprints/context/SyncIngestUpdateToPartInstanceContext.tspackages/job-worker/src/blueprints/context/adlibActions.tspackages/job-worker/src/blueprints/context/lib.tspackages/job-worker/src/blueprints/context/services/PartAndPieceInstanceActionService.tspackages/job-worker/src/blueprints/context/services/__tests__/PartAndPieceInstanceActionService.test.tspackages/job-worker/src/playout/model/PlayoutPartInstanceModel.tspackages/job-worker/src/playout/model/implementation/PlayoutPartInstanceModelImpl.tspackages/job-worker/src/playout/take.tspackages/webui/src/client/lib/partInstanceUtil.tspackages/webui/src/client/styles/rundownView.scsspackages/webui/src/client/ui/SegmentList/LinePart.tsxpackages/webui/src/client/ui/SegmentList/LinePartTimeline.tsxpackages/webui/src/client/ui/SegmentStoryboard/SegmentStoryboard.scsspackages/webui/src/client/ui/SegmentStoryboard/StoryboardPart.tsxpackages/webui/src/client/ui/SegmentTimeline/Parts/InvalidPartCover.tsxpackages/webui/src/client/ui/SegmentTimeline/Parts/SegmentTimelinePart.tsx
Comment on lines
+130
to
+132
| // Get effective invalidReason: planned (Part) takes precedence over runtime (PartInstance) | ||
| const effectiveInvalidReason = getEffectiveInvalidReason(part.instance) | ||
| const isInvalid = isPartInstanceInvalid(part.instance) |
There was a problem hiding this comment.
Use the runtime-aware invalid flag everywhere in this component.
After introducing isInvalid, Line 249 still gates the segment-end marker with !part.instance.part.invalid. Runtime-invalid parts will therefore keep showing the segment-end affordance even though the rest of this component treats them as invalid. Switch that guard to !isInvalid to keep the storyboard visuals consistent with the new state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/webui/src/client/ui/SegmentStoryboard/StoryboardPart.tsx` around
lines 130 - 132, The segment-end marker visibility is still using the static
flag part.instance.part.invalid instead of the runtime-aware isInvalid; update
the guard that currently checks !part.instance.part.invalid to use !isInvalid so
the segment-end marker follows the same runtime-invalid logic used elsewhere
(see getEffectiveInvalidReason and isPartInstanceInvalid usages) and keep
visuals consistent across StoryboardPart.
Comment on lines
+691
to
+693
| // Get effective invalidReason: planned (Part) takes precedence over runtime (PartInstance) | ||
| const effectiveInvalidReason = getEffectiveInvalidReason(partInstance) | ||
| const isInvalid = isPartInstanceInvalid(partInstance) |
There was a problem hiding this comment.
Replace the remaining innerPart.invalid checks with isInvalid.
This file now computes runtime-aware invalidity, but several UI branches still key off planned invalidity only. In particular, Lines 724, 781, 794, 816, 828, and the checks inside renderEndOfSegment still use innerPart.invalid, so a runtime-invalid next part can continue to render as next/auto-next and show end-of-segment affordances. Thread isInvalid through those paths as well.
🤖 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 691 - 693, Several UI branches in SegmentTimelinePart still check
the planned-only flag innerPart.invalid instead of the runtime-aware
isInvalid/effectiveInvalidReason; this lets runtime-invalid parts render as
next/auto-next and show end-of-segment affordances. Update all those branches to
use the computed isInvalid (and where relevant effectiveInvalidReason) instead
of innerPart.invalid — specifically replace checks of innerPart.invalid in the
conditional logic around next/auto-next rendering and inside renderEndOfSegment
with isInvalid so runtime invalidity flows through; ensure the same symbols
(isPartInstanceInvalid, getEffectiveInvalidReason, isInvalid,
effectiveInvalidReason, renderEndOfSegment, innerPart) are used to locate and
update the conditionals.
Julusian
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About the Contributor
This pull request is posted on behalf of the BBC
Type of Contribution
This is a: Feature
New Behavior
This allows the nexted partinstance to be marked as invalid, while preserving it as the next partinstance.
Visually it is similar to an invalid part, but not the same
It is already possible for blueprints to block a take in
onTake, this expands upon that ability by making it visible to the user that a part is unplayable right now, while keeping it as the next one to be taken.This is intended to be used when hitting resource limitations, typically one that is fixable by using an adlib.
This behaves differently to a part being invalid, as this partinstance will remain as nexted. the user is free to use adlibs, change the next and other actions as usual, but they are blocked from taking into this part.
Testing
Affected areas
Time Frame
Other Information
Status