feat(LSG): new resolvedPlaylistTopic#1718
feat(LSG): new resolvedPlaylistTopic#1718justandras wants to merge 11 commits intoSofie-Automation:mainfrom
Conversation
justandras
added
Contribution from BBC
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 51 minutes and 9 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdded a new AsyncAPI websocket channel Changes
Sequence Diagram(s)sequenceDiagram
participant Client as WebSocket Client
participant Topic as ResolvedPlaylistTopic
participant Handlers as Collection Handlers
participant Context as ConversionContext
participant Converters as Conversion Helpers
Handlers->>Topic: push updates (playlist, rundowns, segments, parts, pieces, timers)
Topic->>Topic: cache updates & throttle
Topic->>Context: createResolvedPlaylistConversionContext(with cached state)
Context-->>Topic: normalized lookups (rundownsById, segmentsById, partsById, instances)
Topic->>Converters: toResolvedPlaylistStatus(context)
Converters->>Converters: toResolvedRundownStatus → toResolvedSegmentStatus → toResolvedPartStatus → toResolvedPieceStatus
Converters-->>Topic: ResolvedPlaylistEvent
Topic->>Client: broadcast resolvedPlaylist message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
justandras
force-pushed
the
justandras/lsg-full-status
branch
from
1490d4d to
8253eeb
Compare
Saftret
left a comment
Saftret
left a comment
There was a problem hiding this comment.
Functionality has been tested.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/live-status-gateway/src/topics/helpers/notification/toNotificationStatus.ts (1)
19-27:⚠️ Potential issue | 🔴 CriticalIncorrect mapping for
NoteSeverity.INFO— returnsERRORinstead ofINFO.Now that this helper is exported and reused by the new resolved-playlist conversion path (e.g.
toResolvedPartStatus.tsmappinginvalidReason), this pre-existing mapping bug becomes observable on a new externally-visible channel: anyINFO-severity input will surface asERRORto API consumers.🐛 Proposed fix
case NoteSeverity.INFO: - return NotificationSeverity.ERROR + return NotificationSeverity.INFO }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/notification/toNotificationStatus.ts` around lines 19 - 27, The function toNotificationSeverity incorrectly maps NoteSeverity.INFO to NotificationSeverity.ERROR; update the mapping in toNotificationSeverity so that the case for NoteSeverity.INFO returns NotificationSeverity.INFO (and consider adding a default/fallback branch or explicit exhaustive check for NoteSeverity to avoid silent mis-mapping in the future); locate and fix the switch in toNotificationSeverity that references NoteSeverity and NotificationSeverity to correct the INFO->INFO mapping.
🧹 Nitpick comments (22)
packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/resolvedPlaylistConversionTestUtils.ts (2)
96-123:makePieceInstancelackspartInstanceId.Most consumers of
PieceInstancefilter or group bypartInstanceId; fixtures without it may let conversion code appear to work in tests while missing real-world joins. Consider accepting/defaulting apartInstanceIdparameter so piece instances can be associated with the part instances constructed bymakePartInstance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/resolvedPlaylistConversionTestUtils.ts` around lines 96 - 123, makePieceInstance currently creates PieceInstance fixtures without a partInstanceId, which breaks tests that filter/group by partInstanceId; update makePieceInstance to accept an optional partInstanceId parameter (or default to the id of the PartInstance created by makePartInstance) and set it on the returned object as partInstanceId so consumers and conversion logic that join/group by partInstanceId behave like real data (refer to makePieceInstance, makePartInstance, and the PieceInstance.partInstanceId field).
20-123: Consider typing factory return values instead ofany.Returning
anyfrom these factories means test suites won't catch drift when the core/API models change (added/renamed required fields). UsingPartial<DBRundownPlaylist>-shaped builders (orDeepPartial) with a singleas unknown as DBRundownPlaylistcast at the boundary would restore most of the type guarantees while keeping fixture ergonomics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/resolvedPlaylistConversionTestUtils.ts` around lines 20 - 123, The factories (makePlaylist, makeRundown, makeSegment, makePart, makePartInstance, makePieceInstance) currently return any which loses type-safety; change their return types to Partial or DeepPartial of the correct DB model types (e.g. DBRundownPlaylist, DBRundown, DBSegment, DBPart, DBPartInstance, DBPieceInstance) and type the overrides parameter accordingly (overrides: Partial<DBRundownPlaylist> etc.), then at the function boundary cast the final object with a single "as unknown as DBX" if needed; import the DB types and/or a DeepPartial helper and replace the any return types and override params for each factory.packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.ts (1)
169-178: Minor: string-selector branch forpiecesFind/pieceInstancesFindmay not match core semantics.
piecesFind(selector)in corelib typically expects a Mongo-style selector object, not a bare id string. If no caller passes a string, this branch is dead; if callers do, compare againstunprotectString(p._id)as done insegmentsFindOne(Line 151) for consistency with ID normalization elsewhere in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.ts` around lines 169 - 178, The string-selector branches in piecesFind and pieceInstancesFind currently compare raw _id values and may not match core semantics; update both branches to normalize IDs using unprotectString like segmentsFindOne does: when selector is a string, filter piecesInPlaylistState and pieceInstancesInPlaylistState by comparing unprotectString(p._id) (or unprotectString(pi._id)) to the selector, preserving the existing behavior for object selectors and the empty-selector case.packages/live-status-gateway-api/api/components/timing/resolvedPartTiming/resolvedPartTiming.yaml (1)
1-31: LGTM — schema aligns with converter output.Required set (
startMs,durationMs) matches non-nullable fields intoResolvedPartStatus, and optional timestamp fields are consistently populated (with 0 fallback) by the converter.One consideration for a future iteration: since the converter coerces unset timing values to
0, consumers cannot distinguish "not yet occurred" from "occurred at epoch 0". If that distinction matters, consider making the optional timestamp properties nullable in the schema and propagatingnullfrom the converter. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway-api/api/components/timing/resolvedPartTiming/resolvedPartTiming.yaml` around lines 1 - 31, The schema's optional timestamp properties (plannedStartedPlayback, reportedStartedPlayback, playOffset, setAsNext, take) are currently non-nullable numbers while the converter toResolvedPartStatus coerces unset values to 0, losing "not yet occurred" vs epoch-0 distinction; update the $defs.resolvedPartTiming property types for those optional timestamps to allow null (e.g., type: [number, "null"]) and then modify the converter function toResolvedPartStatus to propagate null for truly unset timing values instead of 0 so consumers can distinguish absent timestamps; also update the example reference (resolvedPartTiming-example.yaml) if needed to reflect nulls.packages/live-status-gateway-api/api/components/part/partInvalidReason/partInvalidReason-example.yaml (1)
1-3: Consider verifying the color choice for "warning" severity.The example uses red (
#ff0000) for awarningseverity level. Typically, red is associated with errors while warnings use orange or yellow. If this color mapping is intentional and aligns with your UI design system, this is fine.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway-api/api/components/part/partInvalidReason/partInvalidReason-example.yaml` around lines 1 - 3, The example uses the color key alongside message and severity (message: 'Invalid clip reference', severity: warning, color: '#ff0000'); update the color to a warning-appropriate hex (e.g., '#FFA500' or '#FFCC00') or confirm and document that red is intentional in the partInvalidReason example by replacing the color value accordingly and/or adding a comment that this maps to your design system.packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/serializeResolvedSegment.spec.ts (1)
3-47: Consider also asserting the stripped fields are absent.
serializePieceshould not only produce the expected object but also guaranteeoutputLayeris absent (not just equal viatoEqual, which in Jest does treat missing vs. undefined as equivalent for own enumerable properties). Consider anot.toHaveProperty('outputLayer')assertion to make the intent (transport-safety / cycle-stripping) explicit and robust against future shape changes.Also consider adding a negative test for
serializeSegmentExtendedverifying that an unrelated top-level field (e.g._id) is preserved, to lock in the "only layer graph is rewritten" contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/serializeResolvedSegment.spec.ts` around lines 3 - 47, Update the tests to explicitly assert removed fields are absent and preserved fields remain: in the serializePiece test (function serializePiece) add expect(result).not.toHaveProperty('outputLayer') to ensure outputLayer is removed rather than undefined, and keep the existing equality check for transport-safe fields; in the serializePart test ensure nested piece fields are properly serialized (serializePart and the pieces assertion is fine); and add a negative/preservation assertion to serializeSegmentExtended: assert expect(result).toHaveProperty('_id', 'segment0') (or similar) to confirm top-level fields like _id are preserved while outputLayers.ol1.sourceLayers is stripped to [] and sourceLayers.sl1.pieces becomes ['pieceA','pieceB'].packages/live-status-gateway-api/api/components/layers/sourceLayer/sourceLayer.yaml (1)
19-24: Considerinteger+enumfortype, andintegerforrank.
typeis described as a "numeric enum" but typed asnumberwith no constraints; downstream clients can't generate a proper enum. TheSourceLayerTypeenum defines a specific integer set (0, 1, 2, 3, 4, 5, 6, 7, 10, 11, 13, 14, 15, 16, 17), so prefertype: integerplus anenumlist. Similarly,rankshould usetype: integerinstead ofnumber. Neither change is blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway-api/api/components/layers/sourceLayer/sourceLayer.yaml` around lines 19 - 24, Change the OpenAPI schema for the source layer so the "type" property uses integer with an explicit enum list matching the SourceLayerType values and the "rank" property uses type: integer instead of number; specifically, update the properties named type and rank in sourceLayer.yaml to use type: integer, and add an enum array for type containing the SourceLayerType integer values (0,1,2,3,4,5,6,7,10,11,13,14,15,16,17) so downstream clients can generate a proper enum.packages/live-status-gateway-api/api/components/timing/resolvedSegmentTiming/resolvedSegmentTiming.yaml (1)
7-15: Considertype: integerfor millisecond fields.
startMs,endMs, andbudgetDurationMsare declared asnumber, which permits fractional values. Millisecond timing values elsewhere in the LSG API are typically integers; usingintegerhere would catch accidental float emissions in generated consumers and better match the wire format. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway-api/api/components/timing/resolvedSegmentTiming/resolvedSegmentTiming.yaml` around lines 7 - 15, Change the three timing fields to use integer types instead of number: update startMs, endMs, and budgetDurationMs in resolvedSegmentTiming.yaml so their schema entries use "type: integer" (preserving their descriptions) to ensure millisecond values are validated as whole milliseconds and catch accidental fractional values in generated consumers.packages/live-status-gateway-api/api/components/part/partInvalidReason/partInvalidReason.yaml (1)
13-15: Consider constrainingcolorformat.
coloris currently an unbounded string. If this is meant to be a CSS color (as the example#ff0000suggests), consider adding apatternor at least documenting the accepted formats (hex, rgb(), named) so client renderers can rely on it. Safe to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway-api/api/components/part/partInvalidReason/partInvalidReason.yaml` around lines 13 - 15, The `color` property in the PartInvalidReason schema is an unconstrained string; update the `color` field definition in partInvalidReason.yaml to constrain or document accepted formats (e.g., add a `pattern` that accepts hex colors like `#RRGGBB` and `#RGB` and/or rgb(a) functions, or add a `oneOf`/`enum` or descriptive `description` listing supported formats) so client renderers can reliably parse values; target the `color` property in the PartInvalidReason component when making this change.packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/toResolvedSegmentStatus.spec.ts (1)
15-17: Missingjest.resetAllMocks()/mockClearbetween tests.Only one test exists today, so this is not a real issue, but the mocked
getResolvedSegmentwill retain call history and themockReturnValueacross future test additions in this file. Consider addingbeforeEach(() => jest.clearAllMocks())now to avoid surprises when the suite grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/toResolvedSegmentStatus.spec.ts` around lines 15 - 17, The test file currently mocks getResolvedSegment (jest.mock(... getResolvedSegment ...)) but doesn't reset or clear mocks between tests; add a beforeEach hook (e.g., beforeEach(() => jest.clearAllMocks()) or jest.resetAllMocks()) at the top of the spec so getResolvedSegment's call history and mockReturnValue are cleared before each test to avoid cross-test leakage.packages/live-status-gateway-api/api/components/segment/resolvedSegment/resolvedSegment.yaml (1)
7-10: Remove the duplicatedtitleanddescriptionfrom the innerallOfbranch.Lines 8-9 repeat the outer
title: ResolvedSegmentanddescription: A segment within a resolved rundownfrom lines 3-4. Per-branch titles insideallOfcan confuse schema-to-type generators. Removing them will keep the schema clean and prevent potential issues.Proposed cleanup
allOf: - $ref: '../segmentBase/segmentBase.yaml#/$defs/segmentBase' - type: object - title: ResolvedSegment - description: A segment within a resolved rundown properties:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway-api/api/components/segment/resolvedSegment/resolvedSegment.yaml` around lines 7 - 10, The inner allOf branch of the ResolvedSegment schema contains duplicated metadata; remove the redundant "title: ResolvedSegment" and "description: A segment within a resolved rundown" from the nested allOf entry so only the outer object retains title/description (look for the ResolvedSegment schema and its inner allOf block) to avoid confusing schema-to-type generators; leave all other properties in the inner branch intact.packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/rundowns/toResolvedRundownStatus.ts (1)
7-7: RedundantString(rundownId)coercion.
rundownIdis already typed asstring, soString(...)is a no-op on both line 7 and line 9. Can drop the wrapping for readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/rundowns/toResolvedRundownStatus.ts` at line 7, Remove the redundant String(...) coercions around rundownId in toResolvedRundownStatus: change ctx.rundownsById.get(String(rundownId)) and any other occurrences (e.g., the similar call on line 9) to use ctx.rundownsById.get(rundownId) (and .has(rundownId) if present), since rundownId is already typed as string; this improves readability without changing behavior.packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/conversionContext.spec.ts (1)
72-85: Consider also asserting the missingshowStyleBaseExtpath.The test title says "throws when required playlist/showStyle is missing" but only exercises the missing
playlistStatecase. A second assertion withshowStyleBaseExtState: undefinedwould fully cover the intended contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/conversionContext.spec.ts` around lines 72 - 85, The test currently only checks the missing playlist case for createResolvedPlaylistConversionContext; add a second assertion that keeps playlistState defined (e.g., use makeTestRundownPlaylist or the existing valid playlist fixture) and sets showStyleBaseExtState: undefined to ensure it also throws the same 'Missing playlist or showStyleBaseExt' error; update the spec 'throws when required playlist/showStyle is missing' to include both expectations so both failure paths (playlistState missing and showStyleBaseExtState missing) are verified.packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/toResolvedPieceStatus.spec.ts (1)
58-62: Minor duplication:makePieceInstance('inst3')is invoked twice.Consider extracting to a local const for clarity:
♻️ Suggested refactor
- it('filters invalid abSessions entries', () => { - const pieceExtended = { - instance: { - ...makePieceInstance('inst3'), - piece: { - ...makePieceInstance('inst3').piece, - abSessions: [{ poolName: 'ok', sessionName: 'ok', playerId: 'p0' }, { poolName: 'bad' }], - }, - }, - } as any + it('filters invalid abSessions entries', () => { + const baseInstance = makePieceInstance('inst3') + const pieceExtended = { + instance: { + ...baseInstance, + piece: { + ...baseInstance.piece, + abSessions: [{ poolName: 'ok', sessionName: 'ok', playerId: 'p0' }, { poolName: 'bad' }], + }, + }, + } as any🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/toResolvedPieceStatus.spec.ts` around lines 58 - 62, Duplicate calls to makePieceInstance('inst3') should be collapsed: create a local const (e.g., const inst3 = makePieceInstance('inst3')) and reuse inst3 and inst3.piece in the test data instead of calling makePieceInstance('inst3') twice; update the array entries that currently spread makePieceInstance('inst3') and makePieceInstance('inst3').piece to use the new inst3 variable.packages/live-status-gateway/src/topics/resolvedPlaylistTopic.ts (1)
40-48: Consider narrower types on the cached arrays.
_partInstancesInPlaylistusesDBPartInstancewhile the send path casts toPartInstance. If the downstream pipeline expectsPartInstance, updating the field type to match what the handler actually emits would eliminate theas PartInstance[]cast insendStatusand let the compiler catch future shape drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/resolvedPlaylistTopic.ts` around lines 40 - 48, The cached array _partInstancesInPlaylist is typed as DBPartInstance[] but sendStatus casts it to PartInstance[]; change the field declaration of _partInstancesInPlaylist to PartInstance[] (or convert/mapper when populating it) so the stored type matches what sendStatus emits and remove the unnecessary as PartInstance[] cast; update any places that push/pop into _partInstancesInPlaylist (e.g., assignment sites in ResolvedPlaylistTopic methods) to produce/convert DBPartInstance -> PartInstance if needed so the compiler enforces shape compatibility.packages/live-status-gateway-api/api/components/resolvedPlaylist/resolvedPlaylistEvent/resolvedPlaylistEvent.yaml (1)
26-29:playoutStateandpublicDatahave no type declaration.Unlike the other properties these are declared with only a
description, so anything (including arrays/strings) validates. If the intent is "arbitrary object blob" (as the runtime populates them with{}), consider pinning them totype: objectwithadditionalProperties: truefor consistency with other components. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway-api/api/components/resolvedPlaylist/resolvedPlaylistEvent/resolvedPlaylistEvent.yaml` around lines 26 - 29, The properties playoutState and publicData lack type declarations; update their schema entries (playoutState and publicData) to be explicit object blobs by adding "type: object" and "additionalProperties: true" so they validate consistently with other components and allow arbitrary keys/values.packages/live-status-gateway-api/api/components/part/resolvedPart/resolvedPart.yaml (1)
1-56: Schema matches the converter output; minor consistency nit onpublicData.
requiredlist covers the newinvalid/floated/untimedflags, andstateis correctly left optional (only set for the current/next part). LikeresolvedPlaylistEvent.yaml,publicDatais declared with only a description; considertype: objectwithadditionalProperties: truefor consistency across components. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway-api/api/components/part/resolvedPart/resolvedPart.yaml` around lines 1 - 56, The publicData property in the resolvedPart schema is only documented with a description; update the resolvedPart properties block (property name: publicData) to declare it as an open object by adding "type: object" and "additionalProperties: true" so it matches other components (e.g., resolvedPlaylistEvent) and accepts arbitrary key/value pairs.packages/live-status-gateway/src/collections/partInstancesInPlaylistHandler.ts (1)
83-86: DirectupdateAndNotify()bypasses the throttle.
changed()routes through_throttledUpdateAndNotify, but this branch callsupdateAndNotify()synchronously. With frequent playlist updates (e.g. activation id churn on startup), this can emit redundant notifications that would otherwise be coalesced. Consider going through the throttled path for consistency:Proposed fix
- } else if (this._subscriptionId) { - this.updateAndNotify() + } else if (this._subscriptionId) { + this._throttledUpdateAndNotify()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/collections/partInstancesInPlaylistHandler.ts` around lines 83 - 86, The branch that currently calls updateAndNotify() directly bypasses the throttling used elsewhere; replace the direct call with this._throttledUpdateAndNotify() so the changed() path consistently coalesces frequent updates (ensure _throttledUpdateAndNotify is called on this and that its binding/initialization matches other usages of the throttled updater).packages/live-status-gateway/src/collections/pieceInstancesInPlaylistHandler.ts (2)
56-60:anycast obscuresDBPartInstanceshape from upstream handler.
data.allis already typed asDBPartInstance[]viaPartInstancesInPlaylist. The(pi: any)cast is unnecessary and loses type-checking on_id. Small cleanup:Proposed refactor
- this._partInstanceIds = _.compact((data?.all ?? []).map((pi: any) => pi._id)).sort() + this._partInstanceIds = _.compact((data?.all ?? []).map((pi) => pi._id)).sort()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/collections/pieceInstancesInPlaylistHandler.ts` around lines 56 - 60, The onPartInstancesInPlaylistUpdate handler uses an unnecessary (pi: any) cast which hides the DBPartInstance shape; remove the any cast so TypeScript uses PartInstancesInPlaylist typing (or explicitly type as DBPartInstance) and map to pi._id directly (e.g. (data?.all ?? []).map(pi => pi._id)), preserving the rest of the logic that assigns this._partInstanceIds and calls resubscribe(this._currentRundownIds, prevPartInstanceIds); ensure the mapped type matches this._partInstanceIds (string | undefined) and keep the _.compact(...).sort() pipeline intact.
62-92:resubscribenever re-evaluates when only_partInstanceIdsshrunk to empty after a prior active subscription — consider explicit ordering.The two early-return branches (Lines 67-71 and 73-77) both call
stopSubscription()+clearAndNotify(), which is correct. However, the following branch at Line 84 also callsstopSubscription()beforesetupSubscription(). Note thatsetupSubscriptionin the base already callsstopSubscription()(see the base class), so the explicit one here is redundant. Keeping it isn't wrong — just worth noting for consistency withpiecesInPlaylistHandler/partInstancesInPlaylistHandler, which omit it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/collections/pieceInstancesInPlaylistHandler.ts` around lines 62 - 92, Move the emptiness checks for this._currentRundownIds and this._partInstanceIds to occur before computing sameSubscription so shrinking to empty re-evaluates correctly; then when recreating the observer in resubscribe use setupSubscription(this._currentRundownIds, this._partInstanceIds, {}) without the redundant stopSubscription() (setupSubscription already stops existing subscriptions). Reference: resubscribe, this._currentRundownIds, this._partInstanceIds, sameSubscription, stopSubscription, setupSubscription, updateAndNotify, clearAndNotify (align behavior with piecesInPlaylistHandler/partInstancesInPlaylistHandler).packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/events/toResolvedPlaylistStatus.ts (1)
73-82:segmentsById/partsByIdbuilt but only forwarded totoQuickLoopStatus.These maps are re-derived every event emission, duplicating work already done inside
createResolvedPlaylistConversionContext(which presumably buildssegmentsByRundownId/rundown/segment/part lookups). If the context already exposes per-id maps, reuse them to avoid the extra O(N) allocations on every publish, or havetoQuickLoopStatusaccept the context directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/events/toResolvedPlaylistStatus.ts` around lines 73 - 82, The code in toResolvedPlaylistStatus builds segmentsById and partsById on every event but createResolvedPlaylistConversionContext already creates per-id lookups; remove the O(N) reconstructions and reuse the context-provided maps (or change toQuickLoopStatus to accept the context directly). Specifically, stop mapping (segmentsState ?? []).map(...) and (partsState ?? []).map(...), and instead reference the lookup objects from the conversion context returned by createResolvedPlaylistConversionContext (or pass that context into toQuickLoopStatus and read its segmentsById/partsById there) so the existing precomputed maps are reused.packages/live-status-gateway/src/collections/piecesInPlaylistHandler.ts (1)
49-56:sameSubscriptionmixes boolean andSubscriptionId; hard to read.
_.isEqual(prevRundownIds, rundownIds) && this._subscriptionIdyields eitherfalseor aSubscriptionIdstring. It works under truthiness, but the intent is clearer as an explicit boolean, and avoids confusing the subsequentelse if (this._subscriptionId)which then becomes dead code (reachable only when bothisEqual===trueand_subscriptionIdis falsy, in which case!sameSubscriptionis true and the earlier branch already handles it).Proposed refactor
- if (rundownIds && rundownIds.length) { - const sameSubscription = _.isEqual(prevRundownIds, rundownIds) && this._subscriptionId - if (!sameSubscription) { - this.setupSubscription(rundownIds, null) - } else if (this._subscriptionId) { - this.updateAndNotify() - } else { - this.clearAndNotify() - } - } else { - this.clearAndNotify() - } + if (rundownIds && rundownIds.length) { + const sameScope = _.isEqual(prevRundownIds, rundownIds) + if (!sameScope || !this._subscriptionId) { + this.setupSubscription(rundownIds, null) + } else { + this._throttledUpdateAndNotify() + } + } else { + this.stopSubscription() + this.clearAndNotify() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/collections/piecesInPlaylistHandler.ts` around lines 49 - 56, Replace the mixed-typed sameSubscription expression with two explicit booleans (e.g. hasSameIds = _.isEqual(prevRundownIds, rundownIds) and hasSubscription = !!this._subscriptionId) and then branch using those: if !hasSameIds call setupSubscription(rundownIds, null), else if hasSubscription call updateAndNotify(), else call clearAndNotify(); reference symbols: prevRundownIds, rundownIds, _subscriptionId, setupSubscription, updateAndNotify, clearAndNotify.
🤖 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/live-status-gateway-api/api/components/part/resolvedPart/resolvedPart-example.yaml`:
- Around line 1-17: The example file resolvedPart-example.yaml currently uses a
root-level $ref to partBase-example.yaml with sibling properties which violates
JSON Schema composition semantics; update the example to match the schema's
allOf composition by replacing the root $ref with an allOf array that includes a
$ref to '../partBase/partBase-example.yaml' and a second object containing the
sibling properties (instanceId, externalId, rank, invalid, floated, untimed,
invalidReason ref, state, createdByAdLib, publicData, timing ref, pieces refs),
or alternatively inline all fields from partBase into this file so there is no
root $ref with siblings; ensure references to
'../partInvalidReason/partInvalidReason-example.yaml',
'../../timing/resolvedPartTiming/resolvedPartTiming-example.yaml', and
'../../piece/resolvedPiece/resolvedPiece-example.yaml' remain intact.
In
`@packages/live-status-gateway-api/api/components/rundown/resolvedRundown/resolvedRundown.yaml`:
- Around line 19-29: The schema lists the "description" property as "Optional
description" but also includes it in the required array; decide which is correct
and make the schema consistent: either remove "description" from the required
list (update required: [id, externalId, name, rank, segments]) if it's optional,
or change the property description text for "description" to indicate it is
always present (e.g., "Description (may be empty)") if it must remain required;
update the resolvedRundown.yaml required array and the "description" property's
description accordingly and leave publicData as-is.
In
`@packages/live-status-gateway-api/api/components/tTimers/timerStateRunning-example.yaml`:
- Around line 1-3: The example for the "running" timer has an inconsistent
state: the fields paused and pauseTime conflict (paused: false but pauseTime:
1706371900000). Update the timerStateRunning example so that when paused is
false the pauseTime is null (or remove the pauseTime field) to match the
countdownRunning example and represent an active/running timer; change the
pauseTime value in timerStateRunning-example.yaml accordingly.
In
`@packages/live-status-gateway/src/collections/partInstancesInPlaylistHandler.ts`:
- Around line 77-91: When the playlist deactivates or _rundownIds becomes empty
the code calls clearAndNotify() but never stops the server observer; modify the
else branch (the branch where !(this._currentPlaylist && this._rundownIds.length
&& this._activationId)) to call stopSubscription() before clearAndNotify() (or
at least when there was an active subscription previously, e.g., when
prevActivationId and prevRundownIds exist or this._subscriptionId is set) so
that setupSubscription/stopSubscription usage matches the logic in
PieceInstancesInPlaylistHandler.resubscribe and avoids leaking observers.
In `@packages/live-status-gateway/src/collections/piecesInPlaylistHandler.ts`:
- Around line 43-60: onPlaylistUpdate currently leaks subscriptions when
playlist becomes empty and calls updateAndNotify directly on rapid document
events; ensure you call stopSubscription() before clearing the subscription
scope (i.e. before or inside the branch that previously only called
clearAndNotify()) so the server-side pieces observer is stopped when
rundownIdsInOrder is empty/undefined, and replace direct calls to
updateAndNotify() with a throttled invocation using throttleToNextTick (same
utility used in PartInstancesInPlaylistHandler) so onDocumentEvent bursts don’t
emit repeatedly; refer to onPlaylistUpdate, setupSubscription, stopSubscription,
clearAndNotify, updateAndNotify and _subscriptionId to locate where to add
stopSubscription() and where to wrap updateAndNotify with throttleToNextTick.
In
`@packages/live-status-gateway/src/topics/helpers/activePlaylistConversion/timers/toTTimers.ts`:
- Around line 30-37: toTTimers currently assumes tTimers has three positional
entries and calls toTTimer on tTimers[0..2], which crashes when length is 1–2
and misplaces timers when entries aren’t in index order; change toTTimers to
build a fixed-length array for indexes 1..3 by finding a RundownTTimer with
matching index for each slot (use the index field on RundownTTimer), call
toTTimer only on the found element, and fall back to emptyTimer(slotIndex) when
no matching entry exists; ensure toTTimer and emptyTimer are used as helpers and
that extra entries with indexes >3 are ignored or logged but not placed by
position.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.ts`:
- Around line 107-111: getOrderedPartsInRundown sorts all parts in a rundown by
DBPart._rank which is scoped per segment, causing interleaving across segments;
change the implementation to iterate the rundown's segments from
ctx.segmentsByRundownId (in rundown order), for each segment gather parts from
ctx.partsByRundownId that match the segment._id, sort those by _rank, and append
them to the result array so parts are returned in proper rundown (segment)
order; update the function getOrderedPartsInRundown to use segmentsByRundownId,
filter by segmentId, sort per-segment, and concatenate.
- Around line 164-168: getActivePartInstances currently returns
partInstancesInPlaylistState without excluding reset instances; update the
implementation of getActivePartInstances to merge a reset: { $ne: true }
condition into the incoming selector (or use it alone when selector is
undefined) and then pass the merged selector to mongoWhereFilter so reset
instances are excluded, matching the StateCacheResolverDataAccess contract and
the webui reference behavior.
- Around line 58-60: The map rundownsToShowStyles currently assigns every
rundown ID to showStyleBaseExt._id; instead iterate playlist.rundownIdsInOrder
and for each rundown id look up its DBRundown and use that rundown's
showStyleBaseId (DBRundown.showStyleBaseId) when creating the Map (fall back to
showStyleBaseExt._id only if the DBRundown is missing), i.e. replace the current
(playlist.rundownIdsInOrder ?? []).map((r) => [r, showStyleBaseExt._id]) with
logic that resolves each r to its own rundown.showStyleBaseId.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/events/toResolvedPlaylistStatus.ts`:
- Around line 99-100: The fields playoutState and publicData may be undefined in
the non-fallback branch; update the object construction in
toResolvedPlaylistStatus (where playoutState:
ctx.playlist.publicPlayoutPersistentState and publicData:
ctx.playlist.publicData are assigned) to coalesce undefined to an empty object
(e.g. use `?? {}`) so the serialized shape always includes those keys and
matches the fallback shape; ensure both playoutState and publicData use the same
nullish-coalescing pattern.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/parts/toResolvedPartStatus.ts`:
- Around line 30-54: The id can become the literal "undefined" when both
part.partId and basePart._id are missing; update the id expression in
toResolvedPartStatus to provide a safe fallback before calling unprotectString
(e.g., use unprotectString(part.partId ?? basePart._id ?? '') ) so
unprotectString never receives undefined; locate the id creation in the returned
object (the line using unprotectString and references to part.partId and
basePart._id) and replace it with the fallback expression.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/pieces/toResolvedPieceStatus.ts`:
- Around line 15-17: The instanceId currently calls
unprotectString(instance?._id) which can receive undefined and produce the
literal "undefined"; update the returned instanceId so it only calls
unprotectString when instance exists (mirror how id is handled): check instance
(or instance._id) and return unprotectString(instance._id) when present,
otherwise return undefined (or null if the surrounding code expects that).
Modify the return in toResolvedPieceStatus (the instanceId field) accordingly.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/rundowns/toResolvedRundownStatus.ts`:
- Line 9: The code currently computes rank with Math.max(0,
ctx.orderedRundownIds.indexOf(String(rundownId))) which collapses a missing
rundown (index -1) to 0; change this to explicitly handle the -1 case in
toResolvedRundownStatus (or the enclosing function) by checking const idx =
ctx.orderedRundownIds.indexOf(String(rundownId)); if (idx === -1) set rank to a
sentinel (e.g. ctx.orderedRundownIds.length) or undefined and propagate that
value (or return early) so missing rundowns are distinguishable from the first
rundown; update any downstream logic that reads rank accordingly.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/segments/serializeResolvedSegment.ts`:
- Around line 15-25: serializePart dereferences partExtended directly which can
throw if a falsy value is passed; mirror serializePiece's guard by defaulting
partExtended to an empty object before accessing fields (e.g., treat part =
partExtended ?? {}), then use optional chaining (part.partId, part.instance,
etc.) and keep the pieces mapping as is to ensure consistent null-safe behavior
with serializePiece and other serializers.
In `@packages/live-status-gateway/src/topics/resolvedPlaylistTopic.ts`:
- Around line 64-77: The sendStatus method currently calls
toResolvedPlaylistStatus with this._playlist and this._showStyleBaseExt even
when they may be undefined; add an early-return guard at the top of sendStatus
that checks required state (e.g. this._playlist and this._showStyleBaseExt) and
returns immediately if either is missing to avoid calling
toResolvedPlaylistStatus (which triggers createResolvedPlaylistConversionContext
and can throw). Locate sendStatus in resolvedPlaylistTopic.ts and mirror the
existing pattern used in other topics (early-return when required inputs are
absent) so throttledSendStatusToAll won't propagate exceptions.
- Line 45: _field _partInstancesInPlaylist is typed as DBPartInstance[] but
later cast to PartInstance[] (which adds isTemporary), creating an unsafe
mismatch; fix by either changing the storage to real PartInstance objects or
stop the cast and update consumers to accept DBPartInstance[]. Concretely,
update the declaration of _partInstancesInPlaylist to PartInstance[] and ensure
wherever items are inserted into _partInstancesInPlaylist (and any
factory/mapper functions) they are given an explicit isTemporary boolean, or
alternatively remove the cast to PartInstance[] where used and change
function/method signatures that consume it to accept DBPartInstance[]
(referencing _partInstancesInPlaylist, DBPartInstance, PartInstance, and the
place where the cast currently occurs) so types align without unsafe casts.
---
Outside diff comments:
In
`@packages/live-status-gateway/src/topics/helpers/notification/toNotificationStatus.ts`:
- Around line 19-27: The function toNotificationSeverity incorrectly maps
NoteSeverity.INFO to NotificationSeverity.ERROR; update the mapping in
toNotificationSeverity so that the case for NoteSeverity.INFO returns
NotificationSeverity.INFO (and consider adding a default/fallback branch or
explicit exhaustive check for NoteSeverity to avoid silent mis-mapping in the
future); locate and fix the switch in toNotificationSeverity that references
NoteSeverity and NotificationSeverity to correct the INFO->INFO mapping.
---
Nitpick comments:
In
`@packages/live-status-gateway-api/api/components/layers/sourceLayer/sourceLayer.yaml`:
- Around line 19-24: Change the OpenAPI schema for the source layer so the
"type" property uses integer with an explicit enum list matching the
SourceLayerType values and the "rank" property uses type: integer instead of
number; specifically, update the properties named type and rank in
sourceLayer.yaml to use type: integer, and add an enum array for type containing
the SourceLayerType integer values (0,1,2,3,4,5,6,7,10,11,13,14,15,16,17) so
downstream clients can generate a proper enum.
In
`@packages/live-status-gateway-api/api/components/part/partInvalidReason/partInvalidReason-example.yaml`:
- Around line 1-3: The example uses the color key alongside message and severity
(message: 'Invalid clip reference', severity: warning, color: '#ff0000'); update
the color to a warning-appropriate hex (e.g., '#FFA500' or '#FFCC00') or confirm
and document that red is intentional in the partInvalidReason example by
replacing the color value accordingly and/or adding a comment that this maps to
your design system.
In
`@packages/live-status-gateway-api/api/components/part/partInvalidReason/partInvalidReason.yaml`:
- Around line 13-15: The `color` property in the PartInvalidReason schema is an
unconstrained string; update the `color` field definition in
partInvalidReason.yaml to constrain or document accepted formats (e.g., add a
`pattern` that accepts hex colors like `#RRGGBB` and `#RGB` and/or rgb(a)
functions, or add a `oneOf`/`enum` or descriptive `description` listing
supported formats) so client renderers can reliably parse values; target the
`color` property in the PartInvalidReason component when making this change.
In
`@packages/live-status-gateway-api/api/components/part/resolvedPart/resolvedPart.yaml`:
- Around line 1-56: The publicData property in the resolvedPart schema is only
documented with a description; update the resolvedPart properties block
(property name: publicData) to declare it as an open object by adding "type:
object" and "additionalProperties: true" so it matches other components (e.g.,
resolvedPlaylistEvent) and accepts arbitrary key/value pairs.
In
`@packages/live-status-gateway-api/api/components/resolvedPlaylist/resolvedPlaylistEvent/resolvedPlaylistEvent.yaml`:
- Around line 26-29: The properties playoutState and publicData lack type
declarations; update their schema entries (playoutState and publicData) to be
explicit object blobs by adding "type: object" and "additionalProperties: true"
so they validate consistently with other components and allow arbitrary
keys/values.
In
`@packages/live-status-gateway-api/api/components/segment/resolvedSegment/resolvedSegment.yaml`:
- Around line 7-10: The inner allOf branch of the ResolvedSegment schema
contains duplicated metadata; remove the redundant "title: ResolvedSegment" and
"description: A segment within a resolved rundown" from the nested allOf entry
so only the outer object retains title/description (look for the ResolvedSegment
schema and its inner allOf block) to avoid confusing schema-to-type generators;
leave all other properties in the inner branch intact.
In
`@packages/live-status-gateway-api/api/components/timing/resolvedPartTiming/resolvedPartTiming.yaml`:
- Around line 1-31: The schema's optional timestamp properties
(plannedStartedPlayback, reportedStartedPlayback, playOffset, setAsNext, take)
are currently non-nullable numbers while the converter toResolvedPartStatus
coerces unset values to 0, losing "not yet occurred" vs epoch-0 distinction;
update the $defs.resolvedPartTiming property types for those optional timestamps
to allow null (e.g., type: [number, "null"]) and then modify the converter
function toResolvedPartStatus to propagate null for truly unset timing values
instead of 0 so consumers can distinguish absent timestamps; also update the
example reference (resolvedPartTiming-example.yaml) if needed to reflect nulls.
In
`@packages/live-status-gateway-api/api/components/timing/resolvedSegmentTiming/resolvedSegmentTiming.yaml`:
- Around line 7-15: Change the three timing fields to use integer types instead
of number: update startMs, endMs, and budgetDurationMs in
resolvedSegmentTiming.yaml so their schema entries use "type: integer"
(preserving their descriptions) to ensure millisecond values are validated as
whole milliseconds and catch accidental fractional values in generated
consumers.
In
`@packages/live-status-gateway/src/collections/partInstancesInPlaylistHandler.ts`:
- Around line 83-86: The branch that currently calls updateAndNotify() directly
bypasses the throttling used elsewhere; replace the direct call with
this._throttledUpdateAndNotify() so the changed() path consistently coalesces
frequent updates (ensure _throttledUpdateAndNotify is called on this and that
its binding/initialization matches other usages of the throttled updater).
In
`@packages/live-status-gateway/src/collections/pieceInstancesInPlaylistHandler.ts`:
- Around line 56-60: The onPartInstancesInPlaylistUpdate handler uses an
unnecessary (pi: any) cast which hides the DBPartInstance shape; remove the any
cast so TypeScript uses PartInstancesInPlaylist typing (or explicitly type as
DBPartInstance) and map to pi._id directly (e.g. (data?.all ?? []).map(pi =>
pi._id)), preserving the rest of the logic that assigns this._partInstanceIds
and calls resubscribe(this._currentRundownIds, prevPartInstanceIds); ensure the
mapped type matches this._partInstanceIds (string | undefined) and keep the
_.compact(...).sort() pipeline intact.
- Around line 62-92: Move the emptiness checks for this._currentRundownIds and
this._partInstanceIds to occur before computing sameSubscription so shrinking to
empty re-evaluates correctly; then when recreating the observer in resubscribe
use setupSubscription(this._currentRundownIds, this._partInstanceIds, {})
without the redundant stopSubscription() (setupSubscription already stops
existing subscriptions). Reference: resubscribe, this._currentRundownIds,
this._partInstanceIds, sameSubscription, stopSubscription, setupSubscription,
updateAndNotify, clearAndNotify (align behavior with
piecesInPlaylistHandler/partInstancesInPlaylistHandler).
In `@packages/live-status-gateway/src/collections/piecesInPlaylistHandler.ts`:
- Around line 49-56: Replace the mixed-typed sameSubscription expression with
two explicit booleans (e.g. hasSameIds = _.isEqual(prevRundownIds, rundownIds)
and hasSubscription = !!this._subscriptionId) and then branch using those: if
!hasSameIds call setupSubscription(rundownIds, null), else if hasSubscription
call updateAndNotify(), else call clearAndNotify(); reference symbols:
prevRundownIds, rundownIds, _subscriptionId, setupSubscription, updateAndNotify,
clearAndNotify.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/conversionContext.spec.ts`:
- Around line 72-85: The test currently only checks the missing playlist case
for createResolvedPlaylistConversionContext; add a second assertion that keeps
playlistState defined (e.g., use makeTestRundownPlaylist or the existing valid
playlist fixture) and sets showStyleBaseExtState: undefined to ensure it also
throws the same 'Missing playlist or showStyleBaseExt' error; update the spec
'throws when required playlist/showStyle is missing' to include both
expectations so both failure paths (playlistState missing and
showStyleBaseExtState missing) are verified.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/resolvedPlaylistConversionTestUtils.ts`:
- Around line 96-123: makePieceInstance currently creates PieceInstance fixtures
without a partInstanceId, which breaks tests that filter/group by
partInstanceId; update makePieceInstance to accept an optional partInstanceId
parameter (or default to the id of the PartInstance created by makePartInstance)
and set it on the returned object as partInstanceId so consumers and conversion
logic that join/group by partInstanceId behave like real data (refer to
makePieceInstance, makePartInstance, and the PieceInstance.partInstanceId
field).
- Around line 20-123: The factories (makePlaylist, makeRundown, makeSegment,
makePart, makePartInstance, makePieceInstance) currently return any which loses
type-safety; change their return types to Partial or DeepPartial of the correct
DB model types (e.g. DBRundownPlaylist, DBRundown, DBSegment, DBPart,
DBPartInstance, DBPieceInstance) and type the overrides parameter accordingly
(overrides: Partial<DBRundownPlaylist> etc.), then at the function boundary cast
the final object with a single "as unknown as DBX" if needed; import the DB
types and/or a DeepPartial helper and replace the any return types and override
params for each factory.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/serializeResolvedSegment.spec.ts`:
- Around line 3-47: Update the tests to explicitly assert removed fields are
absent and preserved fields remain: in the serializePiece test (function
serializePiece) add expect(result).not.toHaveProperty('outputLayer') to ensure
outputLayer is removed rather than undefined, and keep the existing equality
check for transport-safe fields; in the serializePart test ensure nested piece
fields are properly serialized (serializePart and the pieces assertion is fine);
and add a negative/preservation assertion to serializeSegmentExtended: assert
expect(result).toHaveProperty('_id', 'segment0') (or similar) to confirm
top-level fields like _id are preserved while outputLayers.ol1.sourceLayers is
stripped to [] and sourceLayers.sl1.pieces becomes ['pieceA','pieceB'].
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/toResolvedPieceStatus.spec.ts`:
- Around line 58-62: Duplicate calls to makePieceInstance('inst3') should be
collapsed: create a local const (e.g., const inst3 = makePieceInstance('inst3'))
and reuse inst3 and inst3.piece in the test data instead of calling
makePieceInstance('inst3') twice; update the array entries that currently spread
makePieceInstance('inst3') and makePieceInstance('inst3').piece to use the new
inst3 variable.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/toResolvedSegmentStatus.spec.ts`:
- Around line 15-17: The test file currently mocks getResolvedSegment
(jest.mock(... getResolvedSegment ...)) but doesn't reset or clear mocks between
tests; add a beforeEach hook (e.g., beforeEach(() => jest.clearAllMocks()) or
jest.resetAllMocks()) at the top of the spec so getResolvedSegment's call
history and mockReturnValue are cleared before each test to avoid cross-test
leakage.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.ts`:
- Around line 169-178: The string-selector branches in piecesFind and
pieceInstancesFind currently compare raw _id values and may not match core
semantics; update both branches to normalize IDs using unprotectString like
segmentsFindOne does: when selector is a string, filter piecesInPlaylistState
and pieceInstancesInPlaylistState by comparing unprotectString(p._id) (or
unprotectString(pi._id)) to the selector, preserving the existing behavior for
object selectors and the empty-selector case.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/events/toResolvedPlaylistStatus.ts`:
- Around line 73-82: The code in toResolvedPlaylistStatus builds segmentsById
and partsById on every event but createResolvedPlaylistConversionContext already
creates per-id lookups; remove the O(N) reconstructions and reuse the
context-provided maps (or change toQuickLoopStatus to accept the context
directly). Specifically, stop mapping (segmentsState ?? []).map(...) and
(partsState ?? []).map(...), and instead reference the lookup objects from the
conversion context returned by createResolvedPlaylistConversionContext (or pass
that context into toQuickLoopStatus and read its segmentsById/partsById there)
so the existing precomputed maps are reused.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/rundowns/toResolvedRundownStatus.ts`:
- Line 7: Remove the redundant String(...) coercions around rundownId in
toResolvedRundownStatus: change ctx.rundownsById.get(String(rundownId)) and any
other occurrences (e.g., the similar call on line 9) to use
ctx.rundownsById.get(rundownId) (and .has(rundownId) if present), since
rundownId is already typed as string; this improves readability without changing
behavior.
In `@packages/live-status-gateway/src/topics/resolvedPlaylistTopic.ts`:
- Around line 40-48: The cached array _partInstancesInPlaylist is typed as
DBPartInstance[] but sendStatus casts it to PartInstance[]; change the field
declaration of _partInstancesInPlaylist to PartInstance[] (or convert/mapper
when populating it) so the stored type matches what sendStatus emits and remove
the unnecessary as PartInstance[] cast; update any places that push/pop into
_partInstancesInPlaylist (e.g., assignment sites in ResolvedPlaylistTopic
methods) to produce/convert DBPartInstance -> PartInstance if needed so the
compiler enforces shape compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| $ref: '../partBase/partBase-example.yaml' | ||
| instanceId: 'partInstance_current_1' | ||
| externalId: 'ext_part_1' | ||
| rank: 10 | ||
| invalid: false | ||
| floated: false | ||
| untimed: false | ||
| invalidReason: | ||
| $ref: '../partInvalidReason/partInvalidReason-example.yaml' | ||
| state: current | ||
| createdByAdLib: false | ||
| publicData: | ||
| partType: 'intro' | ||
| timing: | ||
| $ref: '../../timing/resolvedPartTiming/resolvedPartTiming-example.yaml' | ||
| pieces: | ||
| - $ref: '../../piece/resolvedPiece/resolvedPiece-example.yaml' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'package.json' packages/live-status-gateway-api | head -5 | xargs -I{} cat {}
fd -t f 'generate-schema' packages/live-status-gateway-apiRepository: Sofie-Automation/sofie-core
Length of output: 2243
🏁 Script executed:
cat packages/live-status-gateway-api/scripts/generate-schema-types.mjsRepository: Sofie-Automation/sofie-core
Length of output: 4474
🏁 Script executed:
cat packages/live-status-gateway-api/scripts/merge-schemas.mjsRepository: Sofie-Automation/sofie-core
Length of output: 1156
🏁 Script executed:
fd -t f 'schema.ts' packages/live-status-gateway-api/src | head -1 | xargs wc -l
fd -t f 'asyncapi.yaml' packages/live-status-gateway-api/src | xargs ls -lhRepository: Sofie-Automation/sofie-core
Length of output: 242
🏁 Script executed:
grep -A 30 'ResolvedPart:' packages/live-status-gateway-api/src/generated/asyncapi.yaml | head -40Repository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
rg -i 'resolvedpart' packages/live-status-gateway-api/src/generated/asyncapi.yaml -A 5 | head -50Repository: Sofie-Automation/sofie-core
Length of output: 1668
🏁 Script executed:
cat packages/live-status-gateway-api/api/components/part/resolvedPart/resolvedPart.yamlRepository: Sofie-Automation/sofie-core
Length of output: 2133
🏁 Script executed:
cat packages/live-status-gateway-api/api/components/part/resolvedPart/resolvedPart-example.yamlRepository: Sofie-Automation/sofie-core
Length of output: 529
🏁 Script executed:
rg -A 20 'resolvedPart-example' packages/live-status-gateway-api/src/generated/asyncapi.yaml | head -40Repository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
npm info `@apidevtools/json-schema-ref-parser` versions --json | head -20Repository: Sofie-Automation/sofie-core
Length of output: 286
🏁 Script executed:
fd -t f 'README' packages/live-status-gateway-api | xargs catRepository: Sofie-Automation/sofie-core
Length of output: 771
🏁 Script executed:
rg -B 5 -A 50 'examples:' packages/live-status-gateway-api/src/generated/asyncapi.yaml | grep -A 50 'ResolvedPart' | head -70Repository: Sofie-Automation/sofie-core
Length of output: 4977
🏁 Script executed:
cat > /tmp/check_ref_behavior.js << 'EOF'
// Check json-schema-ref-parser behavior with $ref and siblings
const content = `
$ref: '../partBase/partBase-example.yaml'
instanceId: 'partInstance_current_1'
externalId: 'ext_part_1'
rank: 10
`;
console.log("According to JSON Schema spec (section 8.2.4):");
console.log("When processing a $ref keyword, sibling keywords should be ignored in strict compliance.");
console.log("\nHowever, `@apidevtools/json-schema-ref-parser.dereference`() is a convenience tool");
console.log("that MERGES sibling keywords with the referenced content (non-standard behavior).");
console.log("\nThis creates portability issues with strict validators and other tooling.");
EOF
node /tmp/check_ref_behavior.jsRepository: Sofie-Automation/sofie-core
Length of output: 445
🏁 Script executed:
rg -t yaml 'allOf' packages/live-status-gateway-api/api/components/part/resolvedPart/ -B 5 -A 10Repository: Sofie-Automation/sofie-core
Length of output: 1860
Fix the example file to match the schema's composition pattern.
The schema definition in resolvedPart.yaml correctly uses allOf to compose partBase with sibling properties. However, the example file resolvedPart-example.yaml uses a $ref at the root with sibling keys, which violates strict JSON Schema semantics—though your current tooling (@apidevtools/json-schema-ref-parser) happens to merge them.
For portability and spec compliance, align the example with the schema's allOf pattern by either inlining the base example fields or using allOf syntax in the example as well.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/live-status-gateway-api/api/components/part/resolvedPart/resolvedPart-example.yaml`
around lines 1 - 17, The example file resolvedPart-example.yaml currently uses a
root-level $ref to partBase-example.yaml with sibling properties which violates
JSON Schema composition semantics; update the example to match the schema's
allOf composition by replacing the root $ref with an allOf array that includes a
$ref to '../partBase/partBase-example.yaml' and a second object containing the
sibling properties (instanceId, externalId, rank, invalid, floated, untimed,
invalidReason ref, state, createdByAdLib, publicData, timing ref, pieces refs),
or alternatively inline all fields from partBase into this file so there is no
root $ref with siblings; ensure references to
'../partInvalidReason/partInvalidReason-example.yaml',
'../../timing/resolvedPartTiming/resolvedPartTiming-example.yaml', and
'../../piece/resolvedPiece/resolvedPiece-example.yaml' remain intact.
| paused: false | ||
| zeroTime: 1706371920000 | ||
| pauseTime: 1706371900000 |
There was a problem hiding this comment.
pauseTime set while paused: false is inconsistent for the “running” variant.
The sibling example tTimerStatus-countdownRunning-example.yaml uses pauseTime: null when paused: false in its projected block, which matches typical timer semantics (a running timer has no active pause timestamp). Here pauseTime: 1706371900000 alongside paused: false conflates the running and paused cases and may mislead consumers reading this as the canonical “running” example.
✏️ Proposed fix
paused: false
zeroTime: 1706371920000
-pauseTime: 1706371900000
+pauseTime: null📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| paused: false | |
| zeroTime: 1706371920000 | |
| pauseTime: 1706371900000 | |
| paused: false | |
| zeroTime: 1706371920000 | |
| pauseTime: null |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/live-status-gateway-api/api/components/tTimers/timerStateRunning-example.yaml`
around lines 1 - 3, The example for the "running" timer has an inconsistent
state: the fields paused and pauseTime conflict (paused: false but pauseTime:
1706371900000). Update the timerStateRunning example so that when paused is
false the pauseTime is null (or remove the pauseTime field) to match the
countdownRunning example and represent an active/running timer; change the
pauseTime value in timerStateRunning-example.yaml accordingly.
| if (this._currentPlaylist && this._rundownIds.length && this._activationId) { | ||
| const sameSubscription = | ||
| areElementsShallowEqual(this._rundownIds, prevRundownIds) && prevActivationId === this._activationId | ||
| if (!sameSubscription) { | ||
| this.stopSubscription() | ||
| this.setupSubscription(this._rundownIds, this._activationId) | ||
| } else if (this._subscriptionId) { | ||
| this.updateAndNotify() | ||
| } else { | ||
| this.clearAndNotify() | ||
| } | ||
| } else { | ||
| this.clearAndNotify() | ||
| } | ||
| } |
There was a problem hiding this comment.
Subscription leak when playlist deactivates or rundownIdsInOrder becomes empty.
The else branch at Line 88 calls clearAndNotify() but never stopSubscription(). If we were previously subscribed (activation id present, rundowns populated) and the playlist later deactivates or is cleared, the server-side observer stays alive indefinitely — still receiving updates for the old scope. Over time this accumulates stale observers and also causes onDocumentEvent → changed() → updateCollectionData() to overwrite all with whatever the previous collection still contains.
Compare with PieceInstancesInPlaylistHandler.resubscribe which explicitly calls stopSubscription() in both guard branches.
Proposed fix
} else {
+ this.stopSubscription()
this.clearAndNotify()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this._currentPlaylist && this._rundownIds.length && this._activationId) { | |
| const sameSubscription = | |
| areElementsShallowEqual(this._rundownIds, prevRundownIds) && prevActivationId === this._activationId | |
| if (!sameSubscription) { | |
| this.stopSubscription() | |
| this.setupSubscription(this._rundownIds, this._activationId) | |
| } else if (this._subscriptionId) { | |
| this.updateAndNotify() | |
| } else { | |
| this.clearAndNotify() | |
| } | |
| } else { | |
| this.clearAndNotify() | |
| } | |
| } | |
| if (this._currentPlaylist && this._rundownIds.length && this._activationId) { | |
| const sameSubscription = | |
| areElementsShallowEqual(this._rundownIds, prevRundownIds) && prevActivationId === this._activationId | |
| if (!sameSubscription) { | |
| this.stopSubscription() | |
| this.setupSubscription(this._rundownIds, this._activationId) | |
| } else if (this._subscriptionId) { | |
| this.updateAndNotify() | |
| } else { | |
| this.clearAndNotify() | |
| } | |
| } else { | |
| this.stopSubscription() | |
| this.clearAndNotify() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/live-status-gateway/src/collections/partInstancesInPlaylistHandler.ts`
around lines 77 - 91, When the playlist deactivates or _rundownIds becomes empty
the code calls clearAndNotify() but never stops the server observer; modify the
else branch (the branch where !(this._currentPlaylist && this._rundownIds.length
&& this._activationId)) to call stopSubscription() before clearAndNotify() (or
at least when there was an active subscription previously, e.g., when
prevActivationId and prevRundownIds exist or this._subscriptionId is set) so
that setupSubscription/stopSubscription usage matches the logic in
PieceInstancesInPlaylistHandler.resubscribe and avoids leaking observers.
| return { | ||
| id: unprotectString(basePiece._id), | ||
| instanceId: unprotectString(instance?._id), |
There was a problem hiding this comment.
unprotectString(instance?._id) can receive undefined.
If instance is missing, instance?._id is undefined and unprotectString is typically typed to accept a protected-string, producing the literal "undefined" as instanceId. Guard this the same way the id field is handled elsewhere.
🛡️ Suggested fix
- instanceId: unprotectString(instance?._id),
+ instanceId: instance?._id ? unprotectString(instance._id) : '',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return { | |
| id: unprotectString(basePiece._id), | |
| instanceId: unprotectString(instance?._id), | |
| return { | |
| id: unprotectString(basePiece._id), | |
| instanceId: instance?._id ? unprotectString(instance._id) : '', |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/pieces/toResolvedPieceStatus.ts`
around lines 15 - 17, The instanceId currently calls
unprotectString(instance?._id) which can receive undefined and produce the
literal "undefined"; update the returned instanceId so it only calls
unprotectString when instance exists (mirror how id is handled): check instance
(or instance._id) and return unprotectString(instance._id) when present,
otherwise return undefined (or null if the surrounding code expects that).
Modify the return in toResolvedPieceStatus (the instanceId field) accordingly.
| export function toResolvedRundownStatus(ctx: ResolvedPlaylistConversionContext, rundownId: string): ResolvedRundown { | ||
| const rundown = ctx.rundownsById.get(String(rundownId)) | ||
| const orderedSegments = getOrderedSegmentsInRundown(ctx, rundownId) | ||
| const rank = Math.max(0, ctx.orderedRundownIds.indexOf(String(rundownId))) |
There was a problem hiding this comment.
rank silently collapses to 0 when the rundown is not in orderedRundownIds.
If rundownId is absent from ctx.orderedRundownIds, indexOf returns -1 and the Math.max(0, …) clamp produces rank: 0, making this indistinguishable from the legitimate first rundown. Since the "missing rundown" case already returns segments: [] with empty defaulted fields, you may want to handle the -1 case explicitly (e.g. fall back to a sentinel like the current orderedRundownIds.length, or let the caller ensure only known ids are passed). Low severity as this likely doesn't occur under normal control flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/rundowns/toResolvedRundownStatus.ts`
at line 9, The code currently computes rank with Math.max(0,
ctx.orderedRundownIds.indexOf(String(rundownId))) which collapses a missing
rundown (index -1) to 0; change this to explicitly handle the -1 case in
toResolvedRundownStatus (or the enclosing function) by checking const idx =
ctx.orderedRundownIds.indexOf(String(rundownId)); if (idx === -1) set rank to a
sentinel (e.g. ctx.orderedRundownIds.length) or undefined and propagate that
value (or return early) so missing rundowns are distinguishable from the first
rundown; update any downstream logic that reads rank accordingly.
| export function serializePart(partExtended: unknown): any { | ||
| const part = partExtended as any | ||
| return { | ||
| partId: part.partId, | ||
| instance: part.instance, | ||
| renderedDuration: part.renderedDuration, | ||
| startsAt: part.startsAt, | ||
| willProbablyAutoNext: part.willProbablyAutoNext, | ||
| pieces: part.pieces?.map(serializePiece) ?? [], | ||
| } | ||
| } |
There was a problem hiding this comment.
Inconsistent null handling between serializePart and serializePiece.
serializePiece guards with piece ?? {} (Line 6), but serializePart dereferences part.partId directly. If a falsy partExtended is ever passed in, this will throw, which is inconsistent with both sibling functions in this module. Consider mirroring the guard for consistency:
Proposed fix
export function serializePart(partExtended: unknown): any {
- const part = partExtended as any
+ const part = (partExtended as any) ?? {}
return {
partId: part.partId,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function serializePart(partExtended: unknown): any { | |
| const part = partExtended as any | |
| return { | |
| partId: part.partId, | |
| instance: part.instance, | |
| renderedDuration: part.renderedDuration, | |
| startsAt: part.startsAt, | |
| willProbablyAutoNext: part.willProbablyAutoNext, | |
| pieces: part.pieces?.map(serializePiece) ?? [], | |
| } | |
| } | |
| export function serializePart(partExtended: unknown): any { | |
| const part = (partExtended as any) ?? {} | |
| return { | |
| partId: part.partId, | |
| instance: part.instance, | |
| renderedDuration: part.renderedDuration, | |
| startsAt: part.startsAt, | |
| willProbablyAutoNext: part.willProbablyAutoNext, | |
| pieces: part.pieces?.map(serializePiece) ?? [], | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/segments/serializeResolvedSegment.ts`
around lines 15 - 25, serializePart dereferences partExtended directly which can
throw if a falsy value is passed; mirror serializePiece's guard by defaulting
partExtended to an empty object before accessing fields (e.g., treat part =
partExtended ?? {}), then use optional chaining (part.partId, part.instance,
etc.) and keep the pieces mapping as is to ensure consistent null-safe behavior
with serializePiece and other serializers.
| private _rundowns: DBRundown[] = [] | ||
| private _segments: DBSegment[] = [] | ||
| private _parts: DBPart[] = [] | ||
| private _partInstancesInPlaylist: DBPartInstance[] = [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare DBPartInstance and PartInstance to see what fields differ.
rg -nP --type=ts -C2 '^\s*(export\s+)?(interface|class|type)\s+(DBPartInstance|PartInstance)\b'
# Also inspect what toResolvedPlaylistStatus / conversionContext actually consume from PartInstance.
fd -t f 'conversionContext.ts' --exec rg -nP -C2 'PartInstance' {}Repository: Sofie-Automation/sofie-core
Length of output: 3153
🏁 Script executed:
cat -n packages/live-status-gateway/src/topics/resolvedPlaylistTopic.ts | head -80Repository: Sofie-Automation/sofie-core
Length of output: 4328
🏁 Script executed:
rg -nA 10 'export.*function toResolvedPlaylistStatus' packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/events/toResolvedPlaylistStatus.ts | head -30Repository: Sofie-Automation/sofie-core
Length of output: 446
🏁 Script executed:
cat -n packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/events/toResolvedPlaylistStatus.ts | head -60Repository: Sofie-Automation/sofie-core
Length of output: 2517
🏁 Script executed:
cat -n packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.ts | head -150Repository: Sofie-Automation/sofie-core
Length of output: 7116
🏁 Script executed:
cat -n packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.ts | tail -n +150 | head -50Repository: Sofie-Automation/sofie-core
Length of output: 1760
🏁 Script executed:
rg -rn 'isTemporary' packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/Repository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
rg -rn 'isTemporary' packages/live-status-gateway/src/topics/ packages/live-status-gateway/src/collections/partInstancesInPlaylistHandler.tsRepository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
cat -n packages/live-status-gateway/src/collections/partInstancesInPlaylistHandler.ts | head -80Repository: Sofie-Automation/sofie-core
Length of output: 3732
🏁 Script executed:
rg -rn 'isTemporary' packages/live-status-gateway/Repository: Sofie-Automation/sofie-core
Length of output: 53
🏁 Script executed:
rg -rn 'partInstancesInPlaylistState' packages/live-status-gateway/src/Repository: Sofie-Automation/sofie-core
Length of output: 3637
🏁 Script executed:
cat -n packages/live-status-gateway/src/collections/partInstancesInPlaylistHandler.ts | tail -n +50 | head -80Repository: Sofie-Automation/sofie-core
Length of output: 2051
🏁 Script executed:
rg -rn 'makePartInstance' packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/ -A 5 | head -40Repository: Sofie-Automation/sofie-core
Length of output: 5093
🏁 Script executed:
rg -n 'function.*makePartInstance|export.*function.*n\s*\(' packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/resolvedPlaylistConversionTestUtils.ts -A 10Repository: Sofie-Automation/sofie-core
Length of output: 784
🏁 Script executed:
cat -n packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/resolvedPlaylistConversionTestUtils.ts | sed -n '73,100p'Repository: Sofie-Automation/sofie-core
Length of output: 930
🏁 Script executed:
rg -rn 'isTemporary' packages/corelib/src/dataModel/PartInstance.tsRepository: Sofie-Automation/sofie-core
Length of output: 82
🏁 Script executed:
cat -n packages/corelib/src/dataModel/PartInstance.ts | head -50Repository: Sofie-Automation/sofie-core
Length of output: 2370
Remove the unsafe type cast of DBPartInstance[] to PartInstance[].
_partInstancesInPlaylist stores DBPartInstance objects (line 45), but at line 71 it is cast to PartInstance[]. Since PartInstance extends DBPartInstance by adding only isTemporary: boolean, the cast creates a type mismatch: the runtime data lacks the isTemporary property that the type declares. While current conversion functions don't access this field, the cast masks the type gap and could lead to runtime errors if downstream code is later modified to rely on isTemporary.
Either store and pass the correct type (PartInstance[]), or remove the cast and update type signatures to accept DBPartInstance[].
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/live-status-gateway/src/topics/resolvedPlaylistTopic.ts` at line 45,
_field _partInstancesInPlaylist is typed as DBPartInstance[] but later cast to
PartInstance[] (which adds isTemporary), creating an unsafe mismatch; fix by
either changing the storage to real PartInstance objects or stop the cast and
update consumers to accept DBPartInstance[]. Concretely, update the declaration
of _partInstancesInPlaylist to PartInstance[] and ensure wherever items are
inserted into _partInstancesInPlaylist (and any factory/mapper functions) they
are given an explicit isTemporary boolean, or alternatively remove the cast to
PartInstance[] where used and change function/method signatures that consume it
to accept DBPartInstance[] (referencing _partInstancesInPlaylist,
DBPartInstance, PartInstance, and the place where the cast currently occurs) so
types align without unsafe casts.
| sendStatus(subscribers: Iterable<WebSocket>): void { | ||
| const message = toResolvedPlaylistStatus({ | ||
| playlistState: this._playlist as ToResolvedPlaylistStatusProps['playlistState'], | ||
| rundownsState: this._rundowns, | ||
| showStyleBaseExtState: this._showStyleBaseExt, | ||
| segmentsState: this._segments, | ||
| partsState: this._parts, | ||
| partInstancesInPlaylistState: this._partInstancesInPlaylist as PartInstance[], | ||
| piecesInPlaylistState: this._piecesInPlaylist, | ||
| pieceInstancesInPlaylistState: this._pieceInstancesInPlaylist, | ||
| }) | ||
|
|
||
| this.sendMessage(subscribers, message) | ||
| } |
There was a problem hiding this comment.
Guard sendStatus against undefined required state.
sendStatus unconditionally forwards _playlist and _showStyleBaseExt to toResolvedPlaylistStatus, but createResolvedPlaylistConversionContext throws Missing playlist or showStyleBaseExt when either is undefined (see conversionContext.spec.ts). Because each handler callback triggers throttledSendStatusToAll(), the first updates arriving before playlist/showStyleBase are loaded (or after deactivation) will cause the throttled send to throw inside the underscore throttle wrapper, swallowing the next scheduled send and producing confusing error logs.
Add an early return while required inputs are missing, similar to how other topics guard their required state:
🛡️ Suggested guard
sendStatus(subscribers: Iterable<WebSocket>): void {
+ if (!this._playlist || !this._showStyleBaseExt) {
+ return
+ }
const message = toResolvedPlaylistStatus({
- playlistState: this._playlist as ToResolvedPlaylistStatusProps['playlistState'],
+ playlistState: this._playlist,
rundownsState: this._rundowns,
showStyleBaseExtState: this._showStyleBaseExt,
segmentsState: this._segments,
partsState: this._parts,
partInstancesInPlaylistState: this._partInstancesInPlaylist as PartInstance[],
piecesInPlaylistState: this._piecesInPlaylist,
pieceInstancesInPlaylistState: this._pieceInstancesInPlaylist,
})
this.sendMessage(subscribers, message)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sendStatus(subscribers: Iterable<WebSocket>): void { | |
| const message = toResolvedPlaylistStatus({ | |
| playlistState: this._playlist as ToResolvedPlaylistStatusProps['playlistState'], | |
| rundownsState: this._rundowns, | |
| showStyleBaseExtState: this._showStyleBaseExt, | |
| segmentsState: this._segments, | |
| partsState: this._parts, | |
| partInstancesInPlaylistState: this._partInstancesInPlaylist as PartInstance[], | |
| piecesInPlaylistState: this._piecesInPlaylist, | |
| pieceInstancesInPlaylistState: this._pieceInstancesInPlaylist, | |
| }) | |
| this.sendMessage(subscribers, message) | |
| } | |
| sendStatus(subscribers: Iterable<WebSocket>): void { | |
| if (!this._playlist || !this._showStyleBaseExt) { | |
| return | |
| } | |
| const message = toResolvedPlaylistStatus({ | |
| playlistState: this._playlist, | |
| rundownsState: this._rundowns, | |
| showStyleBaseExtState: this._showStyleBaseExt, | |
| segmentsState: this._segments, | |
| partsState: this._parts, | |
| partInstancesInPlaylistState: this._partInstancesInPlaylist as PartInstance[], | |
| piecesInPlaylistState: this._piecesInPlaylist, | |
| pieceInstancesInPlaylistState: this._pieceInstancesInPlaylist, | |
| }) | |
| this.sendMessage(subscribers, message) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/live-status-gateway/src/topics/resolvedPlaylistTopic.ts` around
lines 64 - 77, The sendStatus method currently calls toResolvedPlaylistStatus
with this._playlist and this._showStyleBaseExt even when they may be undefined;
add an early-return guard at the top of sendStatus that checks required state
(e.g. this._playlist and this._showStyleBaseExt) and returns immediately if
either is missing to avoid calling toResolvedPlaylistStatus (which triggers
createResolvedPlaylistConversionContext and can throw). Locate sendStatus in
resolvedPlaylistTopic.ts and mirror the existing pattern used in other topics
(early-return when required inputs are absent) so throttledSendStatusToAll won't
propagate exceptions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
packages/live-status-gateway/src/topics/helpers/activePlaylistConversion/timers/__tests__/toTTimers.spec.ts (2)
23-27: Optional: strengthen the "empty" assertion.
toMatchObject([{ index: 1 }, ...])only validates theindexfield. Consider also assertingconfigured: falseandlabel: ''(or snapshotting the full empty shape) so regressions toemptyTimer()are caught here rather than only in downstream consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/activePlaylistConversion/timers/__tests__/toTTimers.spec.ts` around lines 23 - 27, The test for toTTimers should assert the full empty-timer shape rather than only index; update the assertions in the 'returns 3 empty timers for nullish/empty input' test to also check configured: false and label: '' (or replace the three expect(...).toMatchObject(...) calls with a toStrictEqual/toMatchObject against the complete empty timer shape produced by emptyTimer()) so regressions to emptyTimer() are caught; reference the toTTimers function and emptyTimer shape in the update.
3-10: Nit:mode/state/projectedState/anchorPartIdtyped asanyforcesas anyat every call site.Typing these as
unknown(or the realRundownTTimerfield types) and dropping theas anycasts on Lines 30, 31, 33, 34, 38, 43, 48 would make the tests closer to real usage and catch shape regressions. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/activePlaylistConversion/timers/__tests__/toTTimers.spec.ts` around lines 3 - 10, The TimerInput type currently types mode/state/projectedState/anchorPartId as any; change those properties to unknown (or the concrete RundownTTimer field types) on the TimerInput type and then remove the unnecessary as any casts at the test call sites (the places where TimerInput objects are passed into the toTTimers tests) so the tests use stronger typing and will catch shape regressions; update any assertions or helper usages accordingly to satisfy the new types (e.g., narrow or cast only where truly needed).packages/live-status-gateway/src/topics/helpers/activePlaylistConversion/timers/toTTimers.ts (1)
17-28: Minor:mode/stateare passed through even whenconfiguredis false.If only one of
mode/stateis set on the input,configuredisfalsebut the non-null side is still surfaced onTTimerStatus. The schema describes these fields as "null if not configured", so consumers may reasonably expect both to benulltogether. Consider normalizing both tonullwhenconfiguredis false (or asserting the invariant upstream).♻️ Optional tweak
- const { index, label, mode, state, projectedState, anchorPartId } = timer - return { - index: index as TTimerIndex, - label, - configured: !!(mode && state), - mode: mode as TTimerStatus['mode'], - state: state as TTimerStatus['state'], + const { index, label, mode, state, projectedState, anchorPartId } = timer + const configured = !!(mode && state) + return { + index: index as TTimerIndex, + label, + configured, + mode: configured ? (mode as TTimerStatus['mode']) : null, + state: configured ? (state as TTimerStatus['state']) : null, projected: projectedState ? (projectedState as NonNullable<TTimerStatus['projected']>) : null, anchorPartId: anchorPartId ? unprotectString(anchorPartId) : null, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/activePlaylistConversion/timers/toTTimers.ts` around lines 17 - 28, The toTTimer function currently sets TTimerStatus.mode and .state even when configured is false, violating the schema that both should be null if not configured; update toTTimer (which accepts RundownTTimer and returns TTimerStatus) to compute configured = !!(mode && state) and then set mode and state to configured ? (mode as TTimerStatus['mode']) / (state as TTimerStatus['state']) : null so both are normalized to null when configured is false (leave projected and anchorPartId handling unchanged).packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.ts (2)
189-192:resetexclusion applied — consider tightening the cast.The
reset: { $ne: true }merge now aligns with thegetActivePartInstancescontract. Two small notes:
...(selector as any)discards type information;selector as Record<string, unknown>(or a narrowerMongoQuery<PartInstance>type) would preserve safety.- If a caller ever supplies its own
resetfield in the selector it would be silently overwritten; unlikely in current callers but worth a brief comment to document the precedence.♻️ Suggestion
- getActivePartInstances: (_playlistPick: Pick<DBRundownPlaylist, '_id'>, selector?: unknown) => { - const mergedSelector = selector ? { ...(selector as any), reset: { $ne: true } } : { reset: { $ne: true } } - return mongoWhereFilter(partInstancesInPlaylistState, mergedSelector as never) - }, + getActivePartInstances: (_playlistPick: Pick<DBRundownPlaylist, '_id'>, selector?: unknown) => { + // Always exclude reset instances; any caller-supplied `reset` is intentionally overridden. + const mergedSelector = { + ...((selector as Record<string, unknown> | undefined) ?? {}), + reset: { $ne: true }, + } + return mongoWhereFilter(partInstancesInPlaylistState, mergedSelector as never) + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.ts` around lines 189 - 192, In getActivePartInstances, tighten the unsafe cast and document precedence: change the selector cast from "any" to a safer type such as Record<string, unknown> (or the specific MongoQuery<PartInstance> type used in your codebase) when spreading into mergedSelector, and add a brief inline comment noting that the explicit reset: { $ne: true } will override any reset provided by the caller; keep the same mergedSelector construction and return mongoWhereFilter(partInstancesInPlaylistState, mergedSelector as never).
52-54: Optional: split the missing-input error for easier diagnosis.A single combined message hides which input was missing, which can slow down triage when one handler stops publishing. Consider separate throws or include names in the message.
♻️ Suggestion
- if (!playlist || !showStyleBaseExt) { - throw new Error('Missing playlist or showStyleBaseExt') - } + if (!playlist) throw new Error('createResolvedPlaylistConversionContext: missing playlistState') + if (!showStyleBaseExt) throw new Error('createResolvedPlaylistConversionContext: missing showStyleBaseExtState')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.ts` around lines 52 - 54, The current guard in conversionContext.ts throws a combined error ('Missing playlist or showStyleBaseExt') which obscures which input is absent; update the check around playlist and showStyleBaseExt in the conversion context to throw separate, explicit errors (or a single error that names the missing variable) so handlers can quickly identify the missing input—e.g., check playlist and showStyleBaseExt individually and throw 'Missing playlist' or 'Missing showStyleBaseExt' (referencing the playlist and showStyleBaseExt symbols in conversionContext.ts).
🤖 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/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.ts`:
- Around line 169-177: The segmentsFindOne implementation assumes selector._id
is a plain id and passes objects to unprotectString, causing failures for
MongoQuery forms; update segmentsFindOne to use the existing mongoWhereFilter
(already imported) to evaluate the selector against each segment in
segmentsState (falling back to comparing unprotected id when selector is a raw
id/primitive) so it correctly supports operator queries like { _id: { $in: [...]
} } and { _id: { $eq: ... } } and returns the first matching DBSegment.
---
Nitpick comments:
In
`@packages/live-status-gateway/src/topics/helpers/activePlaylistConversion/timers/__tests__/toTTimers.spec.ts`:
- Around line 23-27: The test for toTTimers should assert the full empty-timer
shape rather than only index; update the assertions in the 'returns 3 empty
timers for nullish/empty input' test to also check configured: false and label:
'' (or replace the three expect(...).toMatchObject(...) calls with a
toStrictEqual/toMatchObject against the complete empty timer shape produced by
emptyTimer()) so regressions to emptyTimer() are caught; reference the toTTimers
function and emptyTimer shape in the update.
- Around line 3-10: The TimerInput type currently types
mode/state/projectedState/anchorPartId as any; change those properties to
unknown (or the concrete RundownTTimer field types) on the TimerInput type and
then remove the unnecessary as any casts at the test call sites (the places
where TimerInput objects are passed into the toTTimers tests) so the tests use
stronger typing and will catch shape regressions; update any assertions or
helper usages accordingly to satisfy the new types (e.g., narrow or cast only
where truly needed).
In
`@packages/live-status-gateway/src/topics/helpers/activePlaylistConversion/timers/toTTimers.ts`:
- Around line 17-28: The toTTimer function currently sets TTimerStatus.mode and
.state even when configured is false, violating the schema that both should be
null if not configured; update toTTimer (which accepts RundownTTimer and returns
TTimerStatus) to compute configured = !!(mode && state) and then set mode and
state to configured ? (mode as TTimerStatus['mode']) / (state as
TTimerStatus['state']) : null so both are normalized to null when configured is
false (leave projected and anchorPartId handling unchanged).
In
`@packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.ts`:
- Around line 189-192: In getActivePartInstances, tighten the unsafe cast and
document precedence: change the selector cast from "any" to a safer type such as
Record<string, unknown> (or the specific MongoQuery<PartInstance> type used in
your codebase) when spreading into mergedSelector, and add a brief inline
comment noting that the explicit reset: { $ne: true } will override any reset
provided by the caller; keep the same mergedSelector construction and return
mongoWhereFilter(partInstancesInPlaylistState, mergedSelector as never).
- Around line 52-54: The current guard in conversionContext.ts throws a combined
error ('Missing playlist or showStyleBaseExt') which obscures which input is
absent; update the check around playlist and showStyleBaseExt in the conversion
context to throw separate, explicit errors (or a single error that names the
missing variable) so handlers can quickly identify the missing input—e.g., check
playlist and showStyleBaseExt individually and throw 'Missing playlist' or
'Missing showStyleBaseExt' (referencing the playlist and showStyleBaseExt
symbols in conversionContext.ts).
🪄 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: 0fae5cc2-34e3-488f-aed7-849813b5a1f2
⛔ Files ignored due to path filters (2)
packages/live-status-gateway-api/src/generated/asyncapi.yamlis excluded by!**/generated/**packages/live-status-gateway-api/src/generated/schema.tsis excluded by!**/generated/**
📒 Files selected for processing (8)
packages/live-status-gateway-api/api/components/rundown/resolvedRundown/resolvedRundown.yamlpackages/live-status-gateway/src/collections/piecesInPlaylistHandler.tspackages/live-status-gateway/src/topics/helpers/activePlaylistConversion/timers/__tests__/toTTimers.spec.tspackages/live-status-gateway/src/topics/helpers/activePlaylistConversion/timers/toTTimers.tspackages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/conversionContext.spec.tspackages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/__tests__/resolvedPlaylistConversionTestUtils.tspackages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/context/conversionContext.tspackages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/events/toResolvedPlaylistStatus.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/live-status-gateway-api/api/components/rundown/resolvedRundown/resolvedRundown.yaml
- packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/tests/conversionContext.spec.ts
- packages/live-status-gateway/src/collections/piecesInPlaylistHandler.ts
- packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/tests/resolvedPlaylistConversionTestUtils.ts
- packages/live-status-gateway/src/topics/helpers/resolvedPlaylistConversion/events/toResolvedPlaylistStatus.ts
3 participants
About the Contributor
This pull request is posted on behalf of the BBC and CBC.
Type of Contribution
This is a Feature
Current Behavior
The Live Status Gateway primarily exposes:
activePlaylist: current/next context and playlist-level timing state for the active playlistactivePieces: only currently active (on-air) piecesThere was no single subscription that provided a resolved, playlist-scoped hierarchy of all rundowns → segments → parts → pieces, and some part/piece validity metadata was not exposed in the resolved structures.
New Behavior
Adds a new websocket subscription/topic,
resolvedPlaylist, that publishes the fully resolved active playlist aligned with the webui.Key changes include:
resolvedPlaylisttopic: includes playlist-level fields and a full resolved structure: rundowns → segments → parts → pieces, withpublicDatasurfaced through the hierarchy.ResolvedPart.floatedResolvedPart.untimedResolvedPart.invalidReasonResolvedPiece.invalidTesting
Affected areas
Time Frame
This Feature is critical for us, please review and merge it as soon as possible.
Status