feat: rework ExpectedPackages generation/management to share packages between ingest and playout #1595
Conversation
Julusian
added
the
Contribution from BBC
WalkthroughThis pull request performs a comprehensive restructuring of the ExpectedPackages system, migrating from a piece-centric to an ingest-source-centric data model, introducing a new in-process job queue manager for workers, and refactoring how packages are generated, tracked, and prioritized across ingest and playout workflows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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! |
This is a preparation step for the documents having multiple owners. In the current iteration, each document will remain with one owner.
…s within rundown/bucket
…nces as owners to existing docs # Conflicts: # meteor/server/publications/pieceContentStatusUI/checkPieceContentStatus.ts # packages/job-worker/src/ingest/model/implementation/SaveIngestModel.ts
… between ingest and playout
Julusian
force-pushed
the
upstream/expected-packages-shared-ingest-and-playout
branch
from
b3ba362 to
03346be
Compare
nytamin
left a comment
nytamin
left a comment
There was a problem hiding this comment.
I'm not able to run a full test at this point, but the concepts and implementations look solid in my opinion.
I left a few very minor notes.
| @@ -0,0 +1,137 @@ | |||
| import type { ExpectedPackage, Time } from '@sofie-automation/blueprints-integration' | |||
There was a problem hiding this comment.
Since these types are only ever used during migration, should we move this file to under meteor\server\migration\ ?
| plannedStartedPlayback?: Time | ||
| plannedStoppedPlayback?: Time | ||
|
|
||
| neededExpectedPackageIds?: ExpectedPackageId[] |
There was a problem hiding this comment.
Could you add a comment describing this property and its intended use?
|
|
||
| export function setDefaultIdOnExpectedPackages(expectedPackages: ExpectedPackage.Any[] | undefined): void { | ||
| export function sanitiseExpectedPackages(expectedPackages: ExpectedPackage.Any[] | undefined): void { | ||
| // Fill in ids of unnamed expectedPackage |
There was a problem hiding this comment.
// Fill in ids of unnamed expectedPackage
This comment is no longer valid
|
@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: 7
🤖 Fix all issues with AI agents
In `@meteor/server/worker/jobQueue.ts`:
- Line 196: Update the inline comment in jobQueue.ts that currently reads "So we
psuh a `null` job at the start so that it interrupts immediately" to correct the
typo to "push" (e.g., "So we push a `null` job at the start so that it
interrupts immediately") near the code that enqueues the null job in the job
queue processing logic.
In `@packages/corelib/src/hash.ts`:
- Around line 14-18: The Array branch in the Array.isArray(obj) block
double-hashes elements by calling hashObj on the `"index:hash"` string; change
the mapping in that block so it produces the string `${i}:${hashObj(val)}` (like
the object branch) and then call getHash on strs.join('|') once, ensuring order
and undefined preservation; update the mapping that defines strs and leave
getHash(strs.join('|')) as the final hash call (referencing hashObj, strs,
getHash).
In
`@packages/job-worker/src/blueprints/context/__tests__/watchedPackages.test.ts`:
- Around line 207-256: The test name/assertion mismatch: update the test
description to reflect actual behavior (the test currently asserts that the
package is found and package info is returned), e.g. change the it(...) string
to "returns package info for packages with listenToPackageInfoUpdates: false"
(or similar), keeping the existing assertions that call
WatchedPackagesHelper.create, hasPackage(packageId) and
getPackageInfo('package1'); alternatively, if you intended to assert that no
package info is returned, change the expectations to
expect(helper.hasPackage(packageId)).toBe(true) and
expect(helper.getPackageInfo('package1')).toHaveLength(0) and adjust setup
accordingly, referencing the test case around WatchedPackagesHelper.create,
hasPackage, and getPackageInfo.
In `@packages/shared-lib/src/package-manager/package.ts`:
- Around line 43-44: Update the _id JSDoc to explicitly state its parent
scoping: change the comment for _id (ExpectedPackageId) to indicate it is a
unique identifier scoped to the parent context (rundown, studio, or bucket) for
which the package is defined, referencing the getExpectedPackageId(parentId)
behavior so integrators know scoping affects deduplication and matching across
ingest/playout.
In `@packages/webui/src/client/ui/Settings/DevicePackageManagerSettings.tsx`:
- Around line 59-61: The finally block unconditionally clears
reloadingNow.current which can race with concurrent requests; change the finally
to only set reloadingNow.current = null if it still equals the deviceId for that
request (i.e., in the finally check if reloadingNow.current === deviceId then
set to null) so a later-started request doesn’t get invalidated by an earlier
completion; locate the finally that currently does reloadingNow.current = null
and add this conditional guard around that assignment.
In `@packages/webui/src/client/ui/Status/package-status/index.tsx`:
- Line 149: Remove the debug console.log calls in the package-status module:
delete the console.log('p', p) call (and the other console.log found later in
the same component) so no debug logging is emitted from that component in
production; after removing them, run lint/format and a dev build/test to ensure
no leftover debug statements or unused variables remain.
♻️ Duplicate comments (3)
packages/corelib/src/dataModel/PieceInstance.ts (1)
78-79: Add documentation for this property.This field is central to the PR's goal of sharing packages between ingest and playout. A comment describing its purpose would help future maintainers understand when and how this array is populated. As noted in a previous review, please add a comment describing this property and its intended use.
📝 Suggested documentation
plannedStoppedPlayback?: Time + /** + * The ExpectedPackage IDs that this PieceInstance references. + * Used to track which packages are needed for playout, allowing + * pieceInstances to share package statuses with ingest sources. + */ neededExpectedPackageIds?: ExpectedPackageId[]packages/job-worker/src/ingest/expectedPackages.ts (1)
275-282: Stale comment - function behavior changed.The comment on line 276 says "Fill in ids of unnamed expectedPackage" but the function now sets
contentVersionHashinstead. This was flagged in a past review. Update the comment to accurately describe the current behavior.📝 Suggested fix
export function sanitiseExpectedPackages(expectedPackages: ExpectedPackage.Any[] | undefined): void { - // Fill in ids of unnamed expectedPackage + // Compute and set contentVersionHash for each expectedPackage if (expectedPackages) { for (const expectedPackage of expectedPackages) { expectedPackage.contentVersionHash = getContentVersionHash(expectedPackage) } } }packages/corelib/src/dataModel/Old/ExpectedPackagesR52.ts (1)
16-28: Consider relocating to migration directory.The warning comment clearly states this is "a snapshot...that should only be used in code performing fixup operations." As noted in a previous review, since these types are exclusively used during migration, relocating this file to
meteor/server/migration/would better reflect its purpose and keep migration-specific code co-located.
🧹 Nitpick comments (24)
packages/job-worker/src/ingest/__tests__/expectedPackages.test.ts (1)
114-137: Test name is now misleading after refactoring.The test is named "Generates ExpectedPackages for a Part" but now only verifies that
setExpectedPlayoutItemsis called with an empty array. SinceupdateExpectedMediaAndPlayoutItemsForPartModelno longer handles expected packages (per the relevant snippet showing it only callsupdateExpectedPlayoutItemsForPartModel), consider:
- Renaming the test to reflect its actual purpose, e.g., "Updates expected playout items for a Part"
- Optionally removing the unused
expectedPackagesproperty from the mock pieces if it's no longer relevantSuggested test name update
- test('Generates ExpectedPackages for a Part', async () => { + test('Updates expected playout items for a Part', async () => {packages/job-worker/src/ingest/packageInfo.ts (2)
72-75: Rundown baseline regeneration is not implemented.When
regenerateRundownBaselineis set totrue(for baseline piece types), no action is taken. This could lead to silent failures where package info updates for baseline objects don't trigger the expected regeneration.Would you like me to open an issue to track this implementation, or is this intentionally deferred? If deferred, consider adding a log statement to make it visible when baseline regeneration is skipped.
92-92: Inconsistent log level.This line uses
logger.warnfor an informational message about the number of changed segments. For consistency with line 66 (which useslogger.info), this should also beinfolevel.Suggested fix
- logger.warn(`onUpdatedPackageInfoForRundown: Changed ${result?.changedSegmentIds.length ?? 0} segments`) + logger.info(`onUpdatedPackageInfoForRundown: Changed ${result?.changedSegmentIds.length ?? 0} segments`)packages/job-worker/src/ingest/model/implementation/ExpectedPackagesStore.ts (2)
18-21: Consider narrowing the type assertion.The
as anycast bypasses type safety entirely. If the issue isReadonlyDeepnot inferring correctly with the generic, a more targeted assertion likeas ReadonlyDeep<IngestExpectedPackage<TPackageSource>[]>would preserve some type checking while satisfying the compiler.Suggested refinement
get expectedPackages(): ReadonlyDeep<IngestExpectedPackage<TPackageSource>[]> { - // Typescript is not happy because of the generic - return this.#expectedPackages as any + // ReadonlyDeep inference struggles with generics + return this.#expectedPackages as ReadonlyDeep<IngestExpectedPackage<TPackageSource>[]> }
104-107: Inconsistent change tracking compared tosetExpectedPlayoutItems.
setExpectedPlayoutItemsusesdiffAndReturnLatestObjectsto detect actual changes and only marks items as changed when they differ. In contrast,setExpectedPackagesunconditionally sets#expectedPackagesHasChanges = truewithout comparing to previous values.If this asymmetry is intentional (e.g., expected packages are always fully regenerated from ingest sources), the current approach is fine. Otherwise, consider aligning the behavior for consistency.
meteor/server/publications/pieceContentStatusUI/__tests__/checkPieceContentStatus.test.ts (1)
475-501: Consider adding test coverage for the expectedPackages path with ownership context.The existing tests exercise the MediaObject fallback (no
expectedPackageson pieces). Since the PR's core change is howpackageOwnerIdaffectsgetExpectedPackageIdlookup for pieces withexpectedPackages, consider adding tests that verify:
- A piece with
expectedPackagesuses the correctcandidatePackageIdbased onpackageOwnerId- Different
packageOwnerIdvalues (RundownId vs BucketId) resolve correctlyWould you like me to generate a test case that covers the expectedPackages path with the new ownership model?
meteor/server/publications/pieceContentStatusUI/checkPieceContentStatus.ts (1)
708-730: Minor: duplicategetSideEffectcalls.
getSideEffect(expectedPackage, studio)is called twice - once for thumbnail (line 709) and once for preview (line 721). Consider extracting to a single call.♻️ Optional refactor to avoid duplicate getSideEffect call
if (!thumbnailUrl) { const sideEffect = getSideEffect(expectedPackage, studio) thumbnailUrl = await getAssetUrlFromPackageContainerStatus( studio.packageContainers, getPackageContainerPackageStatus, candidatePackageId, sideEffect.thumbnailContainerId, sideEffect.thumbnailPackageSettings?.path ) - } - if (!previewUrl) { - const sideEffect = getSideEffect(expectedPackage, studio) - - previewUrl = await getAssetUrlFromPackageContainerStatus( - studio.packageContainers, - getPackageContainerPackageStatus, - candidatePackageId, - sideEffect.previewContainerId, - sideEffect.previewPackageSettings?.path - ) + if (!previewUrl) { + previewUrl = await getAssetUrlFromPackageContainerStatus( + studio.packageContainers, + getPackageContainerPackageStatus, + candidatePackageId, + sideEffect.previewContainerId, + sideEffect.previewPackageSettings?.path + ) + } }meteor/server/api/integration/expectedPackages.ts (1)
103-115: Consider improving the error message for multiple package IDs.The projection optimization is a good improvement. However, the error message on line 115 will display the entire array (e.g.,
ExpectedPackages "id1,id2,id3" not found), which may be misleading since only one match is needed. Consider clarifying the message.💡 Suggested improvement
if (!expPackage) - throw new Meteor.Error(404, `ExpectedPackages "${fromPackageIds}" not found`) + throw new Meteor.Error(404, `None of the ExpectedPackages "${fromPackageIds.join(', ')}" were found`)packages/corelib/src/__tests__/hash.spec.ts (1)
193-196: Consider removing the uncertain comment.The comment "This might fail - need to check if conversion is correct" is misleading since the test will pass:
hashObjconverts primitives viaobj + '', producing"false"and"0"respectively, which are distinct strings. Consider removing the comment to avoid confusion for future maintainers.Suggested fix
test('false should be different from 0', () => { - // This might fail - need to check if conversion is correct expect(hashObj(false)).not.toBe(hashObj(0)) })meteor/server/worker/jobQueue.ts (1)
374-374: Track the TODO for job timeouts.The comment indicates timeout handling is not yet implemented. Long-running jobs could hang indefinitely. Consider creating an issue to track this enhancement.
Would you like me to open an issue to track implementing job timeout handling?
packages/job-worker/src/ingest/model/implementation/__tests__/SaveIngestModel.spec.ts (1)
232-246: Consider clarifying the bulkWrite operation count assertion.Line 238's assertion
expect(...operations[1].args[0]).toBe(1)depends on the mock's internal structure. A comment explaining what this verifies (e.g., "1 bulkWrite operation for the merged package") would improve test clarity.Suggested improvement
// Verify only one insert (sources should be merged) expect(context.mockCollections.ExpectedPackages.operations).toHaveLength(3) expect(context.mockCollections.ExpectedPackages.operations[0].type).toBe('findFetch') expect(context.mockCollections.ExpectedPackages.operations[1].type).toBe('bulkWrite') - expect(context.mockCollections.ExpectedPackages.operations[1].args[0]).toBe(1) // 1 operation + // Verify bulkWrite contains exactly 1 operation (both sources merged into single package) + expect(context.mockCollections.ExpectedPackages.operations[1].args[0]).toBe(1) expect(context.mockCollections.ExpectedPackages.operations[2].type).toBe('insertOne')packages/job-worker/src/playout/__tests__/expectedPackages.test.ts (1)
460-480: Consider optimizing empty array handling.The test at lines 460-480 documents that the function updates even when
pieceInstanceIdsis already empty. This could be a minor performance optimization opportunity in the implementation to skip the update when there are no changes needed, but it's not critical.packages/job-worker/src/ingest/expectedPackages.ts (1)
256-263: Cleanup logic is correct but consider adding a comment about future playout references.The
cleanUpUnusedPackagesInBucketfunction removes packages with emptyingestSources. The comment on line 261 mentions that bucket packages can't currently be referenced byplayoutSources, but this might change. Consider adding a TODO or tracking issue if this is planned.packages/job-worker/src/playout/model/implementation/__tests__/SavePlayoutModel.spec.ts (1)
457-470: HardcodedpartInstanceIdmay cause test inaccuracies.The helper always sets
partInstanceIdto'partInstance0'regardless of the actualpartInstanceIdpassed tocreatePartInstanceWithPieceInstances. This could cause issues if tests rely on thePieceInstance.partInstanceIdmatching the actualPartInstance._id.Consider parameterizing this:
Proposed fix
function createPieceInstanceWithExpectedPackages( pieceInstanceId: string, - expectedPackages: ExpectedPackage.Base[] + expectedPackages: ExpectedPackage.Base[], + partInstanceId: string = 'partInstance0' ): PieceInstance { return { _id: protectString<PieceInstanceId>(pieceInstanceId), rundownId: rundownId, - partInstanceId: protectString<PartInstanceId>('partInstance0'), + partInstanceId: protectString<PartInstanceId>(partInstanceId), piece: { _id: protectString(`piece_${pieceInstanceId}`), expectedPackages, }, } as unknown as PieceInstance }meteor/server/worker/worker.ts (1)
229-240: Consider adding options parameter to QueueIngestJob for API consistency.
QueueStudioJobnow accepts an optionaloptions?: QueueJobOptionsparameter, butQueueIngestJob(lines 249-257) doesn't have this parameter. If there's no specific reason to exclude it, consider adding it for API consistency.♻️ Suggested change
export async function QueueIngestJob<T extends keyof IngestJobFunc>( jobName: T, studioId: StudioId, - jobParameters: Parameters<IngestJobFunc[T]>[0] + jobParameters: Parameters<IngestJobFunc[T]>[0], + options?: QueueJobOptions ): Promise<WorkerJob<ReturnType<IngestJobFunc[T]>>> { if (!studioId) throw new Meteor.Error(500, 'Missing studioId') const now = getCurrentTime() - return queueManager.queueJobAndWrapResult(getIngestQueueName(studioId), jobName, jobParameters, now) + return queueManager.queueJobAndWrapResult(getIngestQueueName(studioId), jobName, jobParameters, now, options) }meteor/server/migration/X_X_X.ts (2)
176-194: Non-atomic remove/insert could leave orphaned state on failure.If the process crashes between
removeAsync(line 176) andinsertAsync(line 179), the package will be lost. Consider using a transaction orreplaceOneoperation instead.Suggested approach using replaceOne
- await ExpectedPackages.mutableCollection.removeAsync(pkg._id) - if (ingestSource) { - await ExpectedPackages.mutableCollection.insertAsync({ + await ExpectedPackages.mutableCollection.updateAsync( + { _id: pkg._id }, + { _id: pkg._id, // Preserve the old id to ensure references aren't broken. This will be 'corrected' upon first ingest operation studioId: pkg.studioId, rundownId: rundownId, bucketId: bucketId, package: { ...(pkg as any), // Some fields should be pruned off this, but this is fine _id: pkg.blueprintPackageId, }, created: pkg.created, ingestSources: [ingestSource], playoutSources: { pieceInstanceIds: [], }, - } satisfies Complete<ExpectedPackageDB>) + } satisfies Complete<ExpectedPackageDB>, + { upsert: true } + ) + } else { + await ExpectedPackages.mutableCollection.removeAsync(pkg._id) }
184-187: Unpruned legacy fields will persist in migrated documents.The
(pkg as any)spread includes old top-level fields likefromPieceType,pieceId,segmentId, etc. in thepackageobject. While the comment acknowledges this, these stale fields will remain until re-ingest. Consider explicitly picking only the needed fields to keep the data clean.packages/job-worker/src/playout/model/implementation/SavePlayoutModel.ts (1)
237-249: Address the TODO comment for test coverage.The
$addToSetwith$eachoperator usage is correct, but the TODO comment on line 242 indicates this specific MongoDB operation may need additional test coverage. Ensure the test suite (referenced inSavePlayoutModel.spec.ts) validates this update behavior.Would you like me to help generate test cases for this
$addToSetoperation, or should I open an issue to track this?packages/job-worker/src/playout/expectedPackages.ts (1)
78-87: Clarify the purpose ofisPackageReferencedByPlayoutcheck with emptypieceInstanceIds.Looking at the
isPackageReferencedByPlayoutfunction from the snippets:export function isPackageReferencedByPlayout(expectedPackage: Pick<ExpectedPackageDB, 'playoutSources'>): boolean { return expectedPackage.playoutSources.pieceInstanceIds.length > 0 }The call on lines 80-87 passes a fake package with
pieceInstanceIds: [], which will always returnfalsefromisPackageReferencedByPlayout. Combined with theingestSources.length === 0check on line 79, this effectively means: "delete if no ingest sources exist."If this is the intended behavior, consider simplifying to just
expectedPackage.ingestSources.length === 0for clarity. IfplayoutSourcesmay have additional fields in the future that would affect this check, consider adding a comment explaining the forward-compatibility intent.meteor/server/api/ingest/packageInfo.ts (2)
56-60: Confusing error message - the guard validates correctly but the log is misleading.The error message logs
pkg.rundownIdwhich will beundefined(the very condition being checked). Consider making the message clearer:Suggested improvement
function onUpdatedPackageInfoForRundownDebounce(pkg: ExpectedPackageDB) { if (!pkg.rundownId) { - logger.error(`Updating ExpectedPackage "${pkg._id}" for Rundown "${pkg.rundownId}" not possible`) + logger.error(`Updating ExpectedPackage "${pkg._id}" not possible: missing rundownId`) return }
115-119: Same misleading error message pattern.Suggested improvement
function onUpdatedPackageInfoForBucketItemDebounce(pkg: ExpectedPackageDB, source: ExpectedPackageIngestSourceBucket) { if (!pkg.bucketId) { - logger.error(`Updating ExpectedPackage "${pkg._id}" for Bucket "${pkg.bucketId}" not possible`) + logger.error(`Updating ExpectedPackage "${pkg._id}" not possible: missing bucketId`) return }packages/job-worker/src/ingest/model/IngestExpectedPackage.ts (1)
27-65: Consider clarifying or verifying deduplication scope.The
insertedPackagesForSourceSet (line 39) is scoped to a singleaddPackagesWithSourcecall, meaning deduplication only occurs within that call. If identical packages are added via separate calls, duplicates will accumulate in#packages.This may be intentional since each call represents a different source, but if cross-call deduplication is needed, consider lifting the Set to a class field or documenting this behavior.
packages/job-worker/src/ingest/model/implementation/SaveIngestModel.ts (1)
116-129: Type cast includes unprojected field.Line 128 casts to
Pick<ExpectedPackageDB, '_id' | 'created' | 'playoutSources'>[]but the projection (lines 123-126) only fetches_idandplayoutSources, notcreated. While this doesn't cause runtime issues sincecreatedisn't accessed, the type is misleading.🛠️ Suggested fix
- )) as Pick<ExpectedPackageDB, '_id' | 'created' | 'playoutSources'>[] + )) as Pick<ExpectedPackageDB, '_id' | 'playoutSources'>[]packages/corelib/src/dataModel/ExpectedPackages.ts (1)
48-51: Mutually exclusive ownership fields need runtime validation.The comment states
rundownIdandbucketIdmust not both be set, but there's no compile-time enforcement. Consider adding a runtime assertion or using a discriminated union pattern if this invariant is critical.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (80)
meteor/server/api/__tests__/cleanup.test.tsmeteor/server/api/ingest/debug.tsmeteor/server/api/ingest/packageInfo.tsmeteor/server/api/integration/expectedPackages.tsmeteor/server/collections/packages-media.tsmeteor/server/migration/1_50_0.tsmeteor/server/migration/X_X_X.tsmeteor/server/publications/packageManager/expectedPackages/contentCache.tsmeteor/server/publications/packageManager/expectedPackages/generate.tsmeteor/server/publications/packageManager/expectedPackages/publication.tsmeteor/server/publications/pieceContentStatusUI/__tests__/checkPieceContentStatus.test.tsmeteor/server/publications/pieceContentStatusUI/bucket/regenerateForItem.tsmeteor/server/publications/pieceContentStatusUI/checkPieceContentStatus.tsmeteor/server/publications/pieceContentStatusUI/rundown/regenerateItems.tsmeteor/server/worker/__tests__/jobQueue.test.tsmeteor/server/worker/jobQueue.tsmeteor/server/worker/worker.tspackages/corelib/src/__tests__/hash.spec.tspackages/corelib/src/dataModel/ExpectedPackageWorkStatuses.tspackages/corelib/src/dataModel/ExpectedPackages.tspackages/corelib/src/dataModel/Old/ExpectedPackagesR52.tspackages/corelib/src/dataModel/PackageInfos.tspackages/corelib/src/dataModel/PieceInstance.tspackages/corelib/src/hash.tspackages/corelib/src/mongo.tspackages/corelib/src/pubsub.tspackages/corelib/src/snapshots.tspackages/corelib/src/worker/ingest.tspackages/corelib/src/worker/studio.tspackages/job-worker/src/__mocks__/collection.tspackages/job-worker/src/__mocks__/context.tspackages/job-worker/src/blueprints/context/__tests__/watchedPackages.test.tspackages/job-worker/src/blueprints/context/services/__tests__/PartAndPieceInstanceActionService.test.tspackages/job-worker/src/blueprints/context/watchedPackages.tspackages/job-worker/src/blueprints/postProcess.tspackages/job-worker/src/ingest/__tests__/expectedPackages.test.tspackages/job-worker/src/ingest/bucket/bucketAdlibs.tspackages/job-worker/src/ingest/bucket/import.tspackages/job-worker/src/ingest/commit.tspackages/job-worker/src/ingest/expectedPackages.tspackages/job-worker/src/ingest/generationRundown.tspackages/job-worker/src/ingest/generationSegment.tspackages/job-worker/src/ingest/model/IngestExpectedPackage.tspackages/job-worker/src/ingest/model/IngestModel.tspackages/job-worker/src/ingest/model/IngestPartModel.tspackages/job-worker/src/ingest/model/implementation/DocumentChangeTracker.tspackages/job-worker/src/ingest/model/implementation/ExpectedPackagesStore.tspackages/job-worker/src/ingest/model/implementation/IngestModelImpl.tspackages/job-worker/src/ingest/model/implementation/IngestPartModelImpl.tspackages/job-worker/src/ingest/model/implementation/IngestSegmentModelImpl.tspackages/job-worker/src/ingest/model/implementation/LoadIngestModel.tspackages/job-worker/src/ingest/model/implementation/SaveIngestModel.tspackages/job-worker/src/ingest/model/implementation/__tests__/SaveIngestModel.spec.tspackages/job-worker/src/ingest/packageInfo.tspackages/job-worker/src/ingest/runOperation.tspackages/job-worker/src/ipc.tspackages/job-worker/src/jobs/index.tspackages/job-worker/src/manager.tspackages/job-worker/src/playout/__tests__/expectedPackages.test.tspackages/job-worker/src/playout/adlibAction.tspackages/job-worker/src/playout/expectedPackages.tspackages/job-worker/src/playout/model/implementation/PlayoutModelImpl.tspackages/job-worker/src/playout/model/implementation/PlayoutPieceInstanceModelImpl.tspackages/job-worker/src/playout/model/implementation/SavePlayoutModel.tspackages/job-worker/src/playout/model/implementation/__tests__/SavePlayoutModel.spec.tspackages/job-worker/src/playout/snapshot.tspackages/job-worker/src/playout/timeline/generate.tspackages/job-worker/src/studio/model/StudioBaselineHelper.tspackages/job-worker/src/studio/model/StudioPlayoutModel.tspackages/job-worker/src/studio/model/StudioPlayoutModelImpl.tspackages/job-worker/src/workers/context/JobContextImpl.tspackages/job-worker/src/workers/context/util.tspackages/job-worker/src/workers/ingest/jobs.tspackages/job-worker/src/workers/studio/jobs.tspackages/meteor-lib/src/collections/ExpectedPackages.tspackages/shared-lib/src/package-manager/package.tspackages/shared-lib/src/package-manager/publications.tspackages/webui/src/client/ui/Settings/DevicePackageManagerSettings.tsxpackages/webui/src/client/ui/Status/package-status/PackageStatus.tsxpackages/webui/src/client/ui/Status/package-status/index.tsx
💤 Files with no reviewable changes (2)
- meteor/server/api/ingest/debug.ts
- packages/corelib/src/worker/ingest.ts
🧰 Additional context used
🧬 Code graph analysis (49)
packages/webui/src/client/ui/Status/package-status/PackageStatus.tsx (1)
packages/shared-lib/src/package-manager/package.ts (3)
Any(24-28)Any(395-404)Any(576-585)
packages/corelib/src/snapshots.ts (2)
packages/corelib/src/dataModel/ExpectedPackages.ts (1)
ExpectedPackageDB(42-69)packages/corelib/src/dataModel/Old/ExpectedPackagesR52.ts (1)
ExpectedPackageDB(39-43)
packages/corelib/src/pubsub.ts (2)
packages/corelib/src/dataModel/ExpectedPackages.ts (1)
ExpectedPackageDB(42-69)packages/corelib/src/dataModel/Old/ExpectedPackagesR52.ts (1)
ExpectedPackageDB(39-43)
packages/job-worker/src/workers/studio/jobs.ts (1)
packages/job-worker/src/playout/expectedPackages.ts (1)
handleCleanupOrphanedExpectedPackageReferences(12-133)
packages/job-worker/src/__mocks__/collection.ts (2)
packages/corelib/src/mongo.ts (1)
MongoQuery(60-60)packages/job-worker/src/db/collections.ts (1)
MongoQuery(48-48)
packages/webui/src/client/ui/Status/package-status/index.tsx (3)
packages/job-worker/src/ingest/model/implementation/ExpectedPackagesStore.ts (1)
expectedPackages(18-21)packages/job-worker/src/ingest/model/implementation/IngestPartModelImpl.ts (1)
expectedPackages(90-92)packages/shared-lib/src/lib/protectedString.ts (1)
unprotectString(35-37)
packages/job-worker/src/ingest/model/implementation/__tests__/SaveIngestModel.spec.ts (5)
packages/job-worker/src/ingest/model/implementation/IngestModelImpl.ts (1)
rundownId(106-108)packages/shared-lib/src/lib/protectedString.ts (1)
protectString(21-23)packages/shared-lib/src/core/model/Ids.ts (2)
RundownId(21-21)PartId(37-37)packages/corelib/src/dataModel/ExpectedPackages.ts (2)
getExpectedPackageId(158-172)ExpectedPackageDB(42-69)packages/corelib/src/dataModel/Ids.ts (2)
PieceId(55-55)SegmentId(67-67)
meteor/server/worker/__tests__/jobQueue.test.ts (1)
meteor/server/worker/jobQueue.ts (2)
queueName(65-79)queueName(247-314)
packages/job-worker/src/ingest/model/IngestPartModel.ts (1)
packages/job-worker/src/ingest/model/IngestExpectedPackage.ts (1)
IngestExpectedPackage(15-25)
packages/corelib/src/dataModel/PieceInstance.ts (1)
packages/shared-lib/src/core/model/Ids.ts (1)
ExpectedPackageId(29-29)
packages/job-worker/src/ingest/commit.ts (1)
packages/job-worker/src/ingest/model/IngestModel.ts (2)
IngestModel(141-254)IngestDatabasePersistedModel(258-263)
packages/job-worker/src/playout/adlibAction.ts (1)
packages/job-worker/src/blueprints/context/watchedPackages.ts (2)
WatchedPackagesHelper(15-189)context(134-145)
packages/job-worker/src/playout/model/implementation/__tests__/SavePlayoutModel.spec.ts (1)
packages/corelib/src/dataModel/ExpectedPackages.ts (2)
ExpectedPackageDB(42-69)getExpectedPackageId(158-172)
packages/job-worker/src/ingest/model/implementation/LoadIngestModel.ts (1)
packages/job-worker/src/ingest/model/IngestModel.ts (2)
IngestModel(141-254)IngestDatabasePersistedModel(258-263)
packages/job-worker/src/playout/expectedPackages.ts (4)
packages/corelib/src/worker/studio.ts (1)
CleanupOrphanedExpectedPackageReferencesProps(378-381)packages/corelib/src/dataModel/ExpectedPackages.ts (2)
ExpectedPackageDB(42-69)isPackageReferencedByPlayout(174-176)packages/corelib/src/dataModel/PieceInstance.ts (1)
PieceInstance(34-80)packages/shared-lib/src/core/model/Ids.ts (2)
PieceInstanceId(15-15)ExpectedPackageId(29-29)
packages/job-worker/src/jobs/index.ts (1)
packages/corelib/src/worker/studio.ts (1)
StudioJobFunc(387-441)
meteor/server/publications/pieceContentStatusUI/__tests__/checkPieceContentStatus.test.ts (4)
packages/shared-lib/src/lib/protectedString.ts (1)
protectString(21-23)packages/shared-lib/src/core/model/Ids.ts (1)
RundownId(21-21)meteor/server/publications/pieceContentStatusUI/messageFactory.ts (1)
PieceContentStatusMessageFactory(88-124)meteor/server/publications/pieceContentStatusUI/checkPieceContentStatus.ts (1)
checkPieceContentStatusAndDependencies(228-349)
packages/job-worker/src/ingest/__tests__/expectedPackages.test.ts (1)
packages/job-worker/src/ingest/expectedPackages.ts (1)
updateExpectedMediaAndPlayoutItemsForPartModel(27-29)
packages/job-worker/src/ingest/model/implementation/IngestSegmentModelImpl.ts (3)
packages/job-worker/src/ingest/model/implementation/IngestPartModelImpl.ts (6)
adLibPieces(79-81)expectedPackages(90-92)part(71-73)pieces(75-77)adLibActions(83-85)IngestPartModelImpl(21-235)packages/job-worker/src/ingest/model/IngestExpectedPackage.ts (2)
IngestExpectedPackage(15-25)ExpectedPackageCollector(27-66)packages/corelib/src/dataModel/ExpectedPackages.ts (3)
ExpectedPackageIngestSourcePart(136-136)ExpectedPackageIngestSourcePiece(94-102)ExpectedPackageIngestSourceAdlibAction(103-111)
packages/corelib/src/worker/studio.ts (1)
packages/shared-lib/src/core/model/Ids.ts (2)
RundownPlaylistId(24-24)RundownId(21-21)
packages/job-worker/src/ingest/runOperation.ts (1)
packages/job-worker/src/ingest/model/IngestModel.ts (2)
IngestModel(141-254)IngestDatabasePersistedModel(258-263)
packages/job-worker/src/ipc.ts (2)
meteor/server/worker/jobQueue.ts (5)
jobFinished(82-102)interruptJobStream(179-199)queueName(65-79)queueName(247-314)getNextJob(139-177)packages/job-worker/src/manager.ts (1)
JobManager(5-21)
packages/job-worker/src/blueprints/postProcess.ts (1)
packages/job-worker/src/ingest/expectedPackages.ts (1)
sanitiseExpectedPackages(275-282)
meteor/server/collections/packages-media.ts (2)
meteor/server/collections/indices.ts (1)
registerIndex(20-32)packages/webui/src/client/collections/index.ts (1)
ExpectedPackages(62-62)
packages/job-worker/src/workers/context/JobContextImpl.ts (4)
packages/corelib/src/worker/ingest.ts (1)
getIngestQueueName(329-331)packages/corelib/src/worker/studio.ts (2)
StudioJobFunc(387-441)getStudioQueueName(443-445)packages/job-worker/src/jobs/index.ts (1)
QueueJobOptions(27-38)packages/corelib/src/worker/events.ts (2)
EventsJobFunc(22-25)getEventsQueueName(27-29)
meteor/server/api/__tests__/cleanup.test.ts (1)
packages/webui/src/client/collections/index.ts (1)
Pieces(86-86)
packages/corelib/src/dataModel/ExpectedPackageWorkStatuses.ts (1)
packages/shared-lib/src/core/model/Ids.ts (1)
ExpectedPackageId(29-29)
meteor/server/migration/1_50_0.ts (2)
packages/corelib/src/dataModel/ExpectedPackages.ts (1)
ExpectedPackageDB(42-69)packages/corelib/src/dataModel/Old/ExpectedPackagesR52.ts (1)
ExpectedPackageDB(39-43)
meteor/server/publications/packageManager/expectedPackages/contentCache.ts (5)
packages/corelib/src/dataModel/PieceInstance.ts (1)
PieceInstance(34-80)packages/corelib/src/mongo.ts (1)
MongoFieldSpecifierOnesStrict(29-35)packages/corelib/src/dataModel/ExpectedPackages.ts (1)
ExpectedPackageDB(42-69)meteor/server/publications/lib/ReactiveCacheCollection.ts (1)
ReactiveCacheCollection(9-153)meteor/server/publications/packageManager/expectedPackages/contentObserver.ts (1)
cache(113-115)
meteor/server/api/integration/expectedPackages.ts (2)
meteor/server/collections/packages-media.ts (1)
ExpectedPackages(12-14)packages/corelib/src/dataModel/ExpectedPackages.ts (1)
ExpectedPackageDB(42-69)
meteor/server/publications/pieceContentStatusUI/checkPieceContentStatus.ts (4)
packages/shared-lib/src/core/model/Ids.ts (1)
RundownId(21-21)packages/corelib/src/dataModel/Ids.ts (1)
BucketId(17-17)packages/corelib/src/dataModel/ExpectedPackages.ts (1)
getExpectedPackageId(158-172)packages/meteor-lib/src/collections/ExpectedPackages.ts (1)
getSideEffect(58-72)
packages/corelib/src/dataModel/Old/ExpectedPackagesR52.ts (3)
packages/corelib/src/dataModel/ExpectedPackages.ts (1)
ExpectedPackageDB(42-69)packages/shared-lib/src/core/model/Ids.ts (3)
ExpectedPackageId(29-29)PartId(37-37)RundownId(21-21)packages/corelib/src/dataModel/Ids.ts (7)
PieceId(55-55)SegmentId(67-67)AdLibActionId(12-12)RundownBaselineAdLibActionId(58-58)BucketId(17-17)BucketAdLibId(18-18)BucketAdLibActionId(19-19)
packages/job-worker/src/ingest/generationSegment.ts (2)
packages/job-worker/src/ingest/expectedPackages.ts (1)
updateExpectedMediaAndPlayoutItemsForPartModel(27-29)packages/job-worker/src/blueprints/context/watchedPackages.ts (1)
context(134-145)
packages/job-worker/src/playout/model/implementation/PlayoutPieceInstanceModelImpl.ts (3)
packages/shared-lib/src/core/model/Ids.ts (2)
ExpectedPackageId(29-29)RundownId(21-21)packages/corelib/src/dataModel/PieceInstance.ts (1)
PieceInstance(34-80)packages/corelib/src/dataModel/ExpectedPackages.ts (1)
getExpectedPackageId(158-172)
packages/corelib/src/__tests__/hash.spec.ts (1)
packages/corelib/src/hash.ts (1)
hashObj(10-37)
packages/shared-lib/src/package-manager/publications.ts (1)
packages/shared-lib/src/core/model/Ids.ts (1)
ExpectedPackageId(29-29)
packages/job-worker/src/ingest/packageInfo.ts (2)
packages/corelib/src/lib.ts (1)
assertNever(27-27)meteor/server/logging.ts (1)
logger(146-146)
packages/job-worker/src/ingest/model/implementation/SaveIngestModel.ts (3)
packages/job-worker/src/ingest/model/IngestExpectedPackage.ts (1)
IngestExpectedPackage(15-25)packages/corelib/src/dataModel/ExpectedPackages.ts (2)
ExpectedPackageDB(42-69)isPackageReferencedByPlayout(174-176)packages/corelib/src/lib.ts (1)
normalizeArrayToMap(168-174)
packages/job-worker/src/ingest/generationRundown.ts (6)
packages/job-worker/src/ingest/model/implementation/ExpectedPackagesStore.ts (1)
expectedPackages(18-21)packages/job-worker/src/ingest/model/implementation/IngestPartModelImpl.ts (3)
expectedPackages(90-92)adLibPieces(79-81)adLibActions(83-85)packages/job-worker/src/ingest/expectedPackages.ts (1)
updateExpectedMediaAndPlayoutItemsForRundownBaseline(31-37)packages/job-worker/src/ingest/model/implementation/IngestModelImpl.ts (1)
rundownId(106-108)packages/job-worker/src/ingest/model/IngestExpectedPackage.ts (2)
IngestExpectedPackage(15-25)ExpectedPackageCollector(27-66)packages/corelib/src/dataModel/ExpectedPackages.ts (5)
ExpectedPackageIngestSourceRundownBaseline(142-146)ExpectedPackageIngestSourceBaselineObjects(127-129)ExpectedPackageIngestSourceBaselineAdlibPiece(117-121)ExpectedPackageIngestSourceBaselineAdlibAction(122-126)ExpectedPackageIngestSourceBaselinePiece(112-116)
packages/job-worker/src/manager.ts (1)
packages/job-worker/src/jobs/index.ts (1)
QueueJobOptions(27-38)
packages/job-worker/src/ingest/model/IngestExpectedPackage.ts (5)
packages/corelib/src/dataModel/ExpectedPackages.ts (3)
ExpectedPackageIngestSourcePart(136-136)ExpectedPackageIngestSourceRundownBaseline(142-146)getExpectedPackageId(158-172)packages/shared-lib/src/core/model/Ids.ts (2)
ExpectedPackageId(29-29)RundownId(21-21)packages/corelib/src/dataModel/Ids.ts (1)
BucketId(17-17)packages/job-worker/src/ingest/model/implementation/ExpectedPackagesStore.ts (1)
expectedPackages(18-21)packages/job-worker/src/ingest/model/implementation/IngestPartModelImpl.ts (1)
expectedPackages(90-92)
packages/job-worker/src/__mocks__/context.ts (2)
packages/corelib/src/worker/studio.ts (1)
StudioJobFunc(387-441)packages/job-worker/src/jobs/index.ts (1)
QueueJobOptions(27-38)
packages/webui/src/client/ui/Settings/DevicePackageManagerSettings.tsx (1)
packages/webui/src/client/lib/ModalDialog.tsx (1)
doModalDialog(393-399)
packages/job-worker/src/ingest/model/implementation/ExpectedPackagesStore.ts (5)
packages/corelib/src/dataModel/Ids.ts (1)
ExpectedPlayoutItemId(28-28)packages/job-worker/src/ingest/model/implementation/utils.ts (4)
DocumentChanges(76-80)getDocumentChanges(82-109)setValuesAndTrackChanges(46-68)diffAndReturnLatestObjects(13-44)packages/shared-lib/src/core/model/Ids.ts (2)
RundownId(21-21)PartId(37-37)packages/job-worker/src/ingest/model/implementation/IngestModelImpl.ts (1)
rundownId(106-108)packages/job-worker/src/ingest/model/implementation/IngestPartModelImpl.ts (2)
expectedPlayoutItems(87-89)expectedPackages(90-92)
packages/job-worker/src/blueprints/context/watchedPackages.ts (4)
packages/shared-lib/src/core/model/Ids.ts (2)
ExpectedPackageId(29-29)RundownId(21-21)packages/job-worker/src/ingest/model/IngestExpectedPackage.ts (1)
IngestExpectedPackage(15-25)packages/corelib/src/dataModel/Ids.ts (1)
BucketId(17-17)packages/shared-lib/src/lib/protectedString.ts (1)
unprotectObjectArray(68-70)
packages/job-worker/src/ingest/bucket/bucketAdlibs.ts (1)
packages/job-worker/src/ingest/expectedPackages.ts (1)
cleanUpExpectedPackagesForBucketAdLibs(226-254)
packages/job-worker/src/studio/model/StudioBaselineHelper.ts (5)
packages/shared-lib/src/core/model/Ids.ts (1)
ExpectedPackageId(29-29)packages/job-worker/src/ingest/model/IngestExpectedPackage.ts (1)
IngestExpectedPackage(15-25)packages/corelib/src/dataModel/ExpectedPackages.ts (3)
ExpectedPackageIngestSourceStudioBaseline(131-134)getExpectedPackageId(158-172)ExpectedPackageDB(42-69)packages/job-worker/src/ingest/expectedPackages.ts (1)
sanitiseExpectedPackages(275-282)packages/corelib/src/dataModel/Old/ExpectedPackagesR52.ts (1)
ExpectedPackageDB(39-43)
packages/job-worker/src/playout/model/implementation/PlayoutModelImpl.ts (2)
packages/corelib/src/lib.ts (1)
groupByToMapFunc(227-244)packages/job-worker/src/playout/model/implementation/SavePlayoutModel.ts (1)
writeExpectedPackagesForPlayoutSources(158-290)
packages/job-worker/src/ingest/bucket/import.ts (5)
packages/job-worker/src/blueprints/context/watchedPackages.ts (2)
context(134-145)WatchedPackagesHelper(15-189)packages/job-worker/src/ingest/expectedPackages.ts (1)
cleanUpExpectedPackagesForBucketAdLibs(226-254)packages/corelib/src/dataModel/Ids.ts (1)
BucketId(17-17)packages/blueprints-integration/src/documents/adlibPiece.ts (1)
IBlueprintAdLibPiece(3-27)packages/blueprints-integration/src/action.ts (1)
IBlueprintActionManifest(75-128)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| }) | ||
| } else { | ||
| // There should be a worker waiting, its `getNextJob` might not have reached us yet | ||
| // So we psuh a `null` job at the start so that it interrupts immediately |
There was a problem hiding this comment.
Typo in comment: "psuh" should be "push".
Suggested fix
- // So we psuh a `null` job at the start so that it interrupts immediately
+ // So we push a `null` job at the start so that it interrupts immediately📝 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.
| // So we psuh a `null` job at the start so that it interrupts immediately | |
| // So we push a `null` job at the start so that it interrupts immediately |
🤖 Prompt for AI Agents
In `@meteor/server/worker/jobQueue.ts` at line 196, Update the inline comment in
jobQueue.ts that currently reads "So we psuh a `null` job at the start so that
it interrupts immediately" to correct the typo to "push" (e.g., "So we push a
`null` job at the start so that it interrupts immediately") near the code that
enqueues the null job in the job queue processing logic.
| if (Array.isArray(obj)) { | ||
| // For arrays, we care about the order, and should preserve undefined | ||
| const strs = obj.map((val, i) => hashObj(`${i}:${hashObj(val)}`)) | ||
|
|
||
| return getHash(strs.join('|')) |
There was a problem hiding this comment.
Redundant nested hashObj call for array elements.
Line 16 applies hashObj twice: once to compute the element's hash, and again to hash the resulting "index:hash" string. This double-hashing is likely unintentional and differs from the object branch (line 31) which directly pushes ${key}:${hashObj(val)} without an outer hashObj call.
🔧 Suggested fix
if (Array.isArray(obj)) {
// For arrays, we care about the order, and should preserve undefined
- const strs = obj.map((val, i) => hashObj(`${i}:${hashObj(val)}`))
+ const strs = obj.map((val, i) => `${i}:${hashObj(val)}`)
return getHash(strs.join('|'))📝 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 (Array.isArray(obj)) { | |
| // For arrays, we care about the order, and should preserve undefined | |
| const strs = obj.map((val, i) => hashObj(`${i}:${hashObj(val)}`)) | |
| return getHash(strs.join('|')) | |
| if (Array.isArray(obj)) { | |
| // For arrays, we care about the order, and should preserve undefined | |
| const strs = obj.map((val, i) => `${i}:${hashObj(val)}`) | |
| return getHash(strs.join('|')) |
🤖 Prompt for AI Agents
In `@packages/corelib/src/hash.ts` around lines 14 - 18, The Array branch in the
Array.isArray(obj) block double-hashes elements by calling hashObj on the
`"index:hash"` string; change the mapping in that block so it produces the
string `${i}:${hashObj(val)}` (like the object branch) and then call getHash on
strs.join('|') once, ensuring order and undefined preservation; update the
mapping that defines strs and leave getHash(strs.join('|')) as the final hash
call (referencing hashObj, strs, getHash).
| it('does not return package info for packages with listenToPackageInfoUpdates: false', async () => { | ||
| const context = setupDefaultJobEnvironment() | ||
| const rundownId = protectString<RundownId>('rundown1') | ||
| const packageId = protectString<ExpectedPackageId>('pkg1') | ||
|
|
||
| await context.mockCollections.ExpectedPackages.insertOne( | ||
| literal<ExpectedPackageDB>({ | ||
| _id: packageId, | ||
| studioId: context.studioId, | ||
| rundownId: rundownId, | ||
| bucketId: null, | ||
| package: { _id: 'package1' } as any, | ||
| ingestSources: [ | ||
| { | ||
| fromPieceType: ExpectedPackageDBType.PIECE, | ||
| pieceId: protectString('piece1'), | ||
| blueprintPackageId: 'package1', | ||
| listenToPackageInfoUpdates: false, | ||
| } as any, | ||
| ], | ||
| playoutSources: { | ||
| pieceInstanceIds: [], | ||
| }, | ||
| created: 1000, | ||
| }) | ||
| ) | ||
|
|
||
| await context.mockCollections.PackageInfos.insertOne( | ||
| literal<PackageInfoDB>({ | ||
| _id: protectString('info1'), | ||
| studioId: context.studioId, | ||
| packageId: packageId, | ||
| deviceId: mockDeviceId, | ||
| type: PackageInfo.Type.SCAN, | ||
| expectedContentVersionHash: 'abc123', | ||
| actualContentVersionHash: 'abc123', | ||
| payload: {} as any, | ||
| }) | ||
| ) | ||
|
|
||
| const helper = await WatchedPackagesHelper.create(context, rundownId, null, { | ||
| fromPieceType: ExpectedPackageDBType.PIECE, | ||
| pieceId: protectString('piece1'), | ||
| }) | ||
|
|
||
| // Package should still be found (create doesn't filter by listenToPackageInfoUpdates) | ||
| expect(helper.hasPackage(packageId)).toBe(true) | ||
| // And package info should be available | ||
| expect(helper.getPackageInfo('package1')).toHaveLength(1) | ||
| }) |
There was a problem hiding this comment.
Test name/assertion mismatch - test proves opposite of description.
The test is named "does not return package info for packages with listenToPackageInfoUpdates: false" but the assertions at lines 253-255 show that:
- The package IS found (
hasPackagereturnstrue) - Package info IS returned (
getPackageInfohas length 1)
The comment at line 254 even states "And package info should be available". Either the test name should be updated to reflect actual behavior, or the assertions need to match the expected behavior described in the name.
Suggested fix for test name
- it('does not return package info for packages with listenToPackageInfoUpdates: false', async () => {
+ it('returns package info even when listenToPackageInfoUpdates is false in create', async () => {📝 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.
| it('does not return package info for packages with listenToPackageInfoUpdates: false', async () => { | |
| const context = setupDefaultJobEnvironment() | |
| const rundownId = protectString<RundownId>('rundown1') | |
| const packageId = protectString<ExpectedPackageId>('pkg1') | |
| await context.mockCollections.ExpectedPackages.insertOne( | |
| literal<ExpectedPackageDB>({ | |
| _id: packageId, | |
| studioId: context.studioId, | |
| rundownId: rundownId, | |
| bucketId: null, | |
| package: { _id: 'package1' } as any, | |
| ingestSources: [ | |
| { | |
| fromPieceType: ExpectedPackageDBType.PIECE, | |
| pieceId: protectString('piece1'), | |
| blueprintPackageId: 'package1', | |
| listenToPackageInfoUpdates: false, | |
| } as any, | |
| ], | |
| playoutSources: { | |
| pieceInstanceIds: [], | |
| }, | |
| created: 1000, | |
| }) | |
| ) | |
| await context.mockCollections.PackageInfos.insertOne( | |
| literal<PackageInfoDB>({ | |
| _id: protectString('info1'), | |
| studioId: context.studioId, | |
| packageId: packageId, | |
| deviceId: mockDeviceId, | |
| type: PackageInfo.Type.SCAN, | |
| expectedContentVersionHash: 'abc123', | |
| actualContentVersionHash: 'abc123', | |
| payload: {} as any, | |
| }) | |
| ) | |
| const helper = await WatchedPackagesHelper.create(context, rundownId, null, { | |
| fromPieceType: ExpectedPackageDBType.PIECE, | |
| pieceId: protectString('piece1'), | |
| }) | |
| // Package should still be found (create doesn't filter by listenToPackageInfoUpdates) | |
| expect(helper.hasPackage(packageId)).toBe(true) | |
| // And package info should be available | |
| expect(helper.getPackageInfo('package1')).toHaveLength(1) | |
| }) | |
| it('returns package info even when listenToPackageInfoUpdates is false in create', async () => { | |
| const context = setupDefaultJobEnvironment() | |
| const rundownId = protectString<RundownId>('rundown1') | |
| const packageId = protectString<ExpectedPackageId>('pkg1') | |
| await context.mockCollections.ExpectedPackages.insertOne( | |
| literal<ExpectedPackageDB>({ | |
| _id: packageId, | |
| studioId: context.studioId, | |
| rundownId: rundownId, | |
| bucketId: null, | |
| package: { _id: 'package1' } as any, | |
| ingestSources: [ | |
| { | |
| fromPieceType: ExpectedPackageDBType.PIECE, | |
| pieceId: protectString('piece1'), | |
| blueprintPackageId: 'package1', | |
| listenToPackageInfoUpdates: false, | |
| } as any, | |
| ], | |
| playoutSources: { | |
| pieceInstanceIds: [], | |
| }, | |
| created: 1000, | |
| }) | |
| ) | |
| await context.mockCollections.PackageInfos.insertOne( | |
| literal<PackageInfoDB>({ | |
| _id: protectString('info1'), | |
| studioId: context.studioId, | |
| packageId: packageId, | |
| deviceId: mockDeviceId, | |
| type: PackageInfo.Type.SCAN, | |
| expectedContentVersionHash: 'abc123', | |
| actualContentVersionHash: 'abc123', | |
| payload: {} as any, | |
| }) | |
| ) | |
| const helper = await WatchedPackagesHelper.create(context, rundownId, null, { | |
| fromPieceType: ExpectedPackageDBType.PIECE, | |
| pieceId: protectString('piece1'), | |
| }) | |
| // Package should still be found (create doesn't filter by listenToPackageInfoUpdates) | |
| expect(helper.hasPackage(packageId)).toBe(true) | |
| // And package info should be available | |
| expect(helper.getPackageInfo('package1')).toHaveLength(1) | |
| }) |
🤖 Prompt for AI Agents
In `@packages/job-worker/src/blueprints/context/__tests__/watchedPackages.test.ts`
around lines 207 - 256, The test name/assertion mismatch: update the test
description to reflect actual behavior (the test currently asserts that the
package is found and package info is returned), e.g. change the it(...) string
to "returns package info for packages with listenToPackageInfoUpdates: false"
(or similar), keeping the existing assertions that call
WatchedPackagesHelper.create, hasPackage(packageId) and
getPackageInfo('package1'); alternatively, if you intended to assert that no
package info is returned, change the expectations to
expect(helper.hasPackage(packageId)).toBe(true) and
expect(helper.getPackageInfo('package1')).toHaveLength(0) and adjust setup
accordingly, referencing the test case around WatchedPackagesHelper.create,
hasPackage, and getPackageInfo.
| /** Unique id within the blueprints for the rundown */ | ||
| _id: ExpectedPackageId |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file and understand the context better
cat -n packages/shared-lib/src/package-manager/package.ts | head -60Repository: Sofie-Automation/sofie-core
Length of output: 2738
🏁 Script executed:
# Search for ExpectedPackage interface definition
rg -n "interface ExpectedPackage|type ExpectedPackage" packages/shared-lib/src/package-manager/ -A 20Repository: Sofie-Automation/sofie-core
Length of output: 8577
🏁 Script executed:
# Search for how _id is being generated
rg -n "_id\s*[:=]" packages/shared-lib/src/package-manager/ -B 2 -A 2Repository: Sofie-Automation/sofie-core
Length of output: 3142
🏁 Script executed:
# Search for _id usage in the broader codebase
rg -n "expectedPackage\._id|_id.*expected" --type=ts -B 2 -A 2 | head -100Repository: Sofie-Automation/sofie-core
Length of output: 9800
🏁 Script executed:
# Look for any package generation logic
rg -n "ExpectedPackageId|packageId" packages/shared-lib/src/package-manager/ -B 1 -A 1 | head -50Repository: Sofie-Automation/sofie-core
Length of output: 2217
🏁 Script executed:
# Check the old documentation comment in publications.ts
cat -n packages/shared-lib/src/package-manager/publications.ts | sed -n '28,40p'Repository: Sofie-Automation/sofie-core
Length of output: 657
🏁 Script executed:
# Search for any other references to the old documentation text
rg -n "Unique id of the expectedPackage" --type=tsRepository: Sofie-Automation/sofie-core
Length of output: 170
🏁 Script executed:
# Look at how expectedPackage._id is actually generated/created
rg -n "expectedPackage._id\s*=" --type=ts -B 3 -A 1 | head -60Repository: Sofie-Automation/sofie-core
Length of output: 483
🏁 Script executed:
# Search for where getExpectedPackageId is defined to understand the scoping
rg -n "function getExpectedPackageId|const getExpectedPackageId|export.*getExpectedPackageId" --type=ts -A 10 | head -40Repository: Sofie-Automation/sofie-core
Length of output: 1294
🏁 Script executed:
# Check if there are any comments about uniqueness scope elsewhere in the codebase
rg -n "Unique id.*blueprint|Unique id.*rundown" --type=ts -B 2 -A 2Repository: Sofie-Automation/sofie-core
Length of output: 3230
🏁 Script executed:
# Check if PackageManagerExpectedPackage._id and ExpectedPackage.Base._id are related or if they should have matching docs
cat -n packages/shared-lib/src/package-manager/publications.ts | sed -n '28,42p'Repository: Sofie-Automation/sofie-core
Length of output: 734
🏁 Script executed:
# See if there are any PR notes about the documentation change
git log --oneline --all | grep -i "package\|blueprint\|document" | head -20Repository: Sofie-Automation/sofie-core
Length of output: 177
Clarify the scoping mechanism in the _id documentation.
The updated documentation "Unique id within the blueprints for the rundown" is accurate but could be more explicit. Consider clarifying that _id is scoped per parent context (rundown, studio, or bucket), as indicated by the getExpectedPackageId() function which takes a parentId parameter. A more precise wording would help prevent misunderstandings when integrating or extending this package sharing logic:
Suggested wording: "Unique id scoped to the parent (rundown, studio, or bucket) for which this package is defined"
This is especially important since the PR implements package sharing across ingest and playout contexts where ID scoping semantics directly affect how packages are deduplicated and matched.
🤖 Prompt for AI Agents
In `@packages/shared-lib/src/package-manager/package.ts` around lines 43 - 44,
Update the _id JSDoc to explicitly state its parent scoping: change the comment
for _id (ExpectedPackageId) to indicate it is a unique identifier scoped to the
parent context (rundown, studio, or bucket) for which the package is defined,
referencing the getExpectedPackageId(parentId) behavior so integrators know
scoping affects deduplication and matching across ingest/playout.
| /** Lower should be done first. If not set, the priority is considered as low priority */ | ||
| priority: number | null |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of PackageManagerExpectedPackage across the codebase
echo "=== Searching for PackageManagerExpectedPackage usages ==="
rg -n --type=ts 'PackageManagerExpectedPackage' | head -30
echo ""
echo "=== Searching for priority field access and usage ==="
rg -n --type=ts -B2 -A2 '\.priority' | grep -E '(priority|sort|compar|<|>|===|!==)' | head -40
echo ""
echo "=== Checking for null checks related to priority ==="
rg -n --type=ts 'priority\s*\?' | head -20
echo ""
echo "=== Searching priority in sorting context ==="
rg -n --type=ts 'priority.*sort|sort.*priority' | head -20Repository: Sofie-Automation/sofie-core
Length of output: 5688
🏁 Script executed:
#!/bin/bash
# Look at the actual file to understand the change
echo "=== Contents of publications.ts around lines 40-41 ==="
cat -n packages/shared-lib/src/package-manager/publications.ts | sed -n '30,50p'
echo ""
echo "=== Full type definition of PackageManagerExpectedPackage ==="
rg -n --type=ts -B20 'priority: number' packages/shared-lib/src/package-manager/publications.ts | head -60Repository: Sofie-Automation/sofie-core
Length of output: 1570
🏁 Script executed:
#!/bin/bash
# Check the context of the sorting in rundown.ts
echo "=== Context around rundown.ts:645 ==="
cat -n packages/webui/src/client/lib/rundown.ts | sed -n '638,655p'
echo ""
echo "=== Check type of 'instance' variable ==="
cat -n packages/webui/src/client/lib/rundown.ts | sed -n '630,650p'
echo ""
echo "=== Check imports in rundown.ts to see if it uses PackageManagerExpectedPackage ==="
rg -n 'import|PackageManager' packages/webui/src/client/lib/rundown.ts | head -30Repository: Sofie-Automation/sofie-core
Length of output: 4788
🏁 Script executed:
#!/bin/bash
# Check the publication.ts file where priority is compared
echo "=== Context around publication.ts:223 ==="
cat -n meteor/server/publications/packageManager/expectedPackages/publication.ts | sed -n '215,235p'
echo ""
echo "=== Check the webui PackageWorkStatus to see how it handles priority ==="
cat -n packages/webui/src/client/ui/Status/package-status/PackageWorkStatus.tsx | sed -n '140,160p'Repository: Sofie-Automation/sofie-core
Length of output: 1495
Handle nullable priority in UI display and ensure all comparisons account for null values.
The priority field is now number | null, which requires null handling in consumers. The main concern is in PackageWorkStatus.tsx:149 where priority is displayed directly without checking for null—if priority is null, this renders "null" as a string in the UI. Additionally, any arithmetic operations or sorting logic involving priority must use null-coalescing (??) or explicit null checks to avoid NaN results. The equality check in publication.ts:223 is safe, but code performing numeric operations on priority should follow the pattern used in timeline/generate.ts:572 (o.priority ?? 0).
| .finally(() => { | ||
| reloadingNow.current = null | ||
| }) |
There was a problem hiding this comment.
Race condition in finally block can discard valid responses.
When switching devices quickly, if Device A's request completes after Device B's request has started, Device A's finally block will set reloadingNow.current = null. This causes Device B's response to be incorrectly discarded because the check reloadingNow.current !== deviceId at lines 38 and 43 will evaluate to true (null !== deviceId_B).
Proposed fix
.finally(() => {
- reloadingNow.current = null
+ if (reloadingNow.current === deviceId) {
+ reloadingNow.current = 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.
| .finally(() => { | |
| reloadingNow.current = null | |
| }) | |
| .finally(() => { | |
| if (reloadingNow.current === deviceId) { | |
| reloadingNow.current = null | |
| } | |
| }) |
🤖 Prompt for AI Agents
In `@packages/webui/src/client/ui/Settings/DevicePackageManagerSettings.tsx`
around lines 59 - 61, The finally block unconditionally clears
reloadingNow.current which can race with concurrent requests; change the finally
to only set reloadingNow.current = null if it still equals the deviceId for that
request (i.e., in the finally check if reloadingNow.current === deviceId then
set to null) so a later-started request doesn’t get invalidated by an earlier
completion; locate the finally that currently does reloadingNow.current = null
and add this conditional guard around that assignment.
| return keys.map(({ packageId }) => { | ||
| const p = packagesWithWorkStatuses[packageId] | ||
|
|
||
| console.log('p', p) |
There was a problem hiding this comment.
Remove debug console.log statements before merging.
Lines 149 and 163 contain debug logging that should be removed. These will clutter the browser console in production and may expose internal data structures.
🧹 Proposed fix
return keys.map(({ packageId }) => {
const p = packagesWithWorkStatuses[packageId]
-
- console.log('p', p)
return p.package ? ( function renderPackageContainerStatuses() {
return packageContainerStatuses.map((packageContainerStatus) => {
const device = peripheralDevicesMap.get(packageContainerStatus.deviceId)
- console.log(device, packageContainerStatus.deviceId)
return (Also applies to: 163-163
🤖 Prompt for AI Agents
In `@packages/webui/src/client/ui/Status/package-status/index.tsx` at line 149,
Remove the debug console.log calls in the package-status module: delete the
console.log('p', p) call (and the other console.log found later in the same
component) so no debug logging is emitted from that component in production;
after removing them, run lint/format and a dev build/test to ensure no leftover
debug statements or unused variables remain.
2 participants
About the Contributor
This pull request is posted on behalf of the BBC
Type of Contribution
This is a: Feature
Current Behavior
New Behavior
This is a continuation of #1589
Closes #1154
This is the third stage of reimplementing #1258. I believe we do not have any plans to complete the final steps at the moment.
This continues the previous PR and reworks the package generation flow so that pieceInstances add a claim to each expectedPackage they reference. With this, ingest will not delete these packages, and it means that the pieceInstances will share the statuses of the existing packages.
This will both reduce the number of packages and statuses in the database, and minimises the amount of packages that get pushed to package manager at runtime which need to be resolved ASAP.
With this, when adlibbing a piece, we should never see temporary zebras on the adlibbed piece while waiting for package-manager to catchup
The benefit of this is that the pieceInstances instantly have package statuses available.
With a hack in place to stop pieceInstances from using the statuses of their underlying pieces, this causes the time for the ui to show states to decrease from ~5s to instantly.
While falling back to the piece has been sufficient in many cases, it left pieceinstances created from adlib-actions to suffer.
Note: No treatment has been done for buckets yet, due to the different ownership structures, it needs some further thought on how to handle those correctly.
Testing
Affected areas
Time Frame
Other Information
Status
Summary
Reworks ExpectedPackages generation and management to support shared ownership between ingest and playout subsystems. This fundamental restructuring aims to reduce database load, improve performance of package status availability for UI clients, and prevent erroneous package deletion when parts are set as next.
Key Changes
Data Model Restructuring
ExpectedPackageDB Interface: Replaced a discriminated union of package types with a single public interface that uses
rundownIdandbucketIdto distinguish ownership:package(ReadonlyDeep<Omit<ExpectedPackage.Base>>),ingestSources(array of source descriptors),playoutSources(withpieceInstanceIdsarray)_idgeneration via newgetExpectedPackageIdfunction that hashes package content with parent ownership contextNew Source Types: Introduced comprehensive ingest source abstractions:
ExpectedPackageIngestSourceunion type covering all source variants (piece, adlib action, baseline, bucket, studio baseline)fromPieceType,blueprintPackageId,listenToPackageInfoUpdates, and source-specific ownership fieldsIngestExpectedPackage<TPackageSource>wrapper type used throughout ingestion pipelinesPieceInstance Enhancement: Added optional
neededExpectedPackageIds?: ExpectedPackageId[]field to track which packages a piece instance claims.Package Ownership and Sharing
Dual Ownership Model: Packages now track both ingest and playout claims:
ingestSources[]: Who created the package (ingest, baseline, studio setup)playoutSources.pieceInstanceIds[]: Which piece instances are using the packageCleanup Strategy: New
handleCleanupOrphanedExpectedPackageReferencesfunction reconciles package references:pieceInstanceIdsfromplayoutSourcesGeneration and Publication Flows
Ingest-side Changes:
updateExpectedPackagesForPartModel→updateExpectedMediaAndPlayoutItemsForPartModelIngestExpectedPackagewrapper to separate ingest model from database representationExpectedPackageCollectorutility to deduplicate packages by composite key (package ID, blueprint package ID, listenToPackageInfoUpdates flag)writeExpectedPackagesChangesForRundownfunction handles bulk insert/update/delete with smart source mergingPlayout-side Changes:
writeExpectedPackagesForPlayoutSourcesfunction registers piece instances as claims on existing packagesneededExpectedPackageIdsfor efficient ownership trackingPackage Manager Publication:
PieceInstanceCompacttype changed from exposing nestedpiece.expectedPackagesto exposingpartInstanceIdandneededExpectedPackageIdsExpectedPackageDBCompacttype exposes only_idandpackagefields (minimal surface for UI)updatePackagePrioritiesDatabase Optimization
Collection Indexing: Updated indexes on ExpectedPackages:
fromPieceType,pieceIdindexesstudioId + bucketIdindexstudioId + rundownIdandrundownIdindexesMigration Path: New migration step (
X_X_X.ts) converts old package format to new structure:_idbut regenerates based on new hashJob Queue Infrastructure
Worker Job Queue Manager: Introduced new
WorkerJobQueueManagerclass for in-process job queueing:New Studio Job:
CleanupOrphanedExpectedPackageReferencesjob queued after playout updates to reconcile package ownership.Hash Function Enhancement
Updated
hashObjfunction to handle null/undefined consistently:Supporting Infrastructure
Mock/Test Enhancements:
insertOne,deleteOne)Type Safety: Replaced loose typing with strict source and ownership tracking; eliminated reliance on PieceInstanceId as primary package identifier.
Performance Impact
Breaking Changes
ExpectedPackageDBinterface shape completely changed (single interface vs. discriminated union)updateExpectedPackagesForPartModel,updateExpectedPackagesForRundownBaselineIngestJobs.ExpectedPackagesRegeneratejob typeOutstanding Work