fix: create correct defaults for packageContainerSettingsWithOverrides

[ianshade]

About the Contributor

This pull request is posted on behalf of TV 2 Norge.

Type of Contribution

This is a:

Bug fix

Current Behavior

Migration might create an incorrect packageContainerSettingsWithOverrides object, which causes Package Manager and its settings section in Sofie to crash

New Behavior

Object created by the migration is type-correct

Testing

• I have added one or more unit tests for this PR
• I have updated the relevant unit tests
• No unit test changes are needed for this PR

Affected areas

Migrations, Package Manager

Time Frame

Other Information

Status

• PR is ready to be reviewed.
• The functionality has been tested by the author.
• Relevant unit tests has been added / updated.
• Relevant documentation (code comments, system documentation) has been added / updated.

[coderabbitai]

Walkthrough

A new flatObjectToOverrides factory function is introduced to create ObjectWithOverrides instances from defaults and changed values. The migration step is refactored to use this function with conditional field population based on presence of non-empty container ID arrays. Test assertions are updated to expect explicit empty array defaults.

Changes

Cohort / File(s)	Summary
ObjectWithOverrides Factory packages/corelib/src/settings/objectWithOverrides.ts	New exported flatObjectToOverrides<T> function that creates an ObjectWithOverrides instance from provided defaults and a shallow map of changed values, emitting 'set' override operations only for defined changed values.
Migration Implementation meteor/server/migration/steps/X_X_X/ContainerIdsToObjectWithOverridesMigrationStep.ts	Refactored to use flatObjectToOverrides instead of convertObjectIntoOverrides + casting. Now conditionally populates changedValues only for non-empty container ID arrays, with base object providing both arrays as empty defaults.
Migration Tests meteor/server/migration/steps/X_X_X/__tests__/ContainerIdsToObjectWithOverridesMigrationStep.test.ts	Updated assertions to expect defaults with explicit empty previewContainerIds and thumbnailContainerIds arrays. When old fields are absent, overrides should be empty; when present, overrides apply container ID changes alongside the new explicit defaults.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: correcting how defaults are created for packageContainerSettingsWithOverrides in the migration.
Description check	✅ Passed	The description clearly relates to the changeset, explaining the bug (incorrect object creation causing crashes) and the fix (type-correct object).
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 20 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ckages/corelib/src/settings/objectWithOverrides.ts	16.66%	20 Missing ⚠️

📢 Thoughts on this report? Let us know!

[coderabbitai]

🧹 Nitpick comments (1)

meteor/server/migration/steps/X_X_X/__tests__/ContainerIdsToObjectWithOverridesMigrationStep.test.ts (1)

49-75: Consider adding a test for partially-present / empty legacy arrays.

The two cases covered (both fields present and non-empty; both fields absent) exercise the endpoints of the new conditional logic in ContainerIdsToObjectWithOverridesMigrationStep.migrate(), but they don't cover the "mixed" branches that the if (…length > 0) guards introduce:

• Only one of previewContainerIds / thumbnailContainerIds is present.
• One of them is present but is an empty array [] (should be treated the same as missing — no override emitted).

These branches are exactly the ones that distinguish the new implementation from the previous buggy one, so a small extra test would lock the fix in.

🧪 Example additional test

test('migration handles one old field empty and one populated', async () => {
    await setupMockStudio({
        _id: protectString('studio2'),
        // `@ts-expect-error`
        previewContainerIds: [],
        thumbnailContainerIds: ['thumb1'],
        packageContainerSettingsWithOverrides: undefined as any,
    })

    const step = new ContainerIdsToObjectWithOverridesMigrationStep()
    await step.migrate()

    const studio = await Studios.findOneAsync(protectString('studio2'))
    expect(studio?.packageContainerSettingsWithOverrides).toMatchObject({
        defaults: {
            previewContainerIds: [],
            thumbnailContainerIds: [],
        },
        overrides: [
            { op: 'set', path: 'thumbnailContainerIds', value: ['thumb1'] },
        ],
    })
})

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In
`@meteor/server/migration/steps/X_X_X/__tests__/ContainerIdsToObjectWithOverridesMigrationStep.test.ts`
around lines 49 - 75, Add a unit test covering the mixed legacy-array cases so
the migration's conditional branches are exercised: create a new test in
ContainerIdsToObjectWithOverridesMigrationStep.test.ts that uses setupMockStudio
to seed a studio with one legacy field empty (e.g. previewContainerIds: []) and
the other populated (e.g. thumbnailContainerIds: ['thumb1']), run new
ContainerIdsToObjectWithOverridesMigrationStep().migrate(), then assert the
resulting studio.packageContainerSettingsWithOverrides has defaults with empty
arrays and an overrides array containing only the non-empty field (an op:'set'
for thumbnailContainerIds). Ensure you also call validate() before/after if
desired to mirror existing tests.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@meteor/server/migration/steps/X_X_X/__tests__/ContainerIdsToObjectWithOverridesMigrationStep.test.ts`:
- Around line 49-75: Add a unit test covering the mixed legacy-array cases so
the migration's conditional branches are exercised: create a new test in
ContainerIdsToObjectWithOverridesMigrationStep.test.ts that uses setupMockStudio
to seed a studio with one legacy field empty (e.g. previewContainerIds: []) and
the other populated (e.g. thumbnailContainerIds: ['thumb1']), run new
ContainerIdsToObjectWithOverridesMigrationStep().migrate(), then assert the
resulting studio.packageContainerSettingsWithOverrides has defaults with empty
arrays and an overrides array containing only the non-empty field (an op:'set'
for thumbnailContainerIds). Ensure you also call validate() before/after if
desired to mirror existing tests.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5c064b1f-7310-4ef7-bcae-ff2f95bbe3fa

📥 Commits

Reviewing files that changed from the base of the PR and between 38fae25 and 6e004ff.

📒 Files selected for processing (3)

• meteor/server/migration/steps/X_X_X/ContainerIdsToObjectWithOverridesMigrationStep.ts
• meteor/server/migration/steps/X_X_X/__tests__/ContainerIdsToObjectWithOverridesMigrationStep.test.ts
• packages/corelib/src/settings/objectWithOverrides.ts