fix(EAV-372): settings lost on studio update #1455
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where updating Studio (Blueprint) Config via the HTTP API caused other Studio settings (like peripheral devices) to be lost. The fix preserves existing studio data when building updated studio objects.
- Refactored the
updateOverridesfunction to properly preserve existing overrides and handle complex object structures - Modified the studio building logic to preserve existing studio properties before applying API updates
- Added comprehensive test coverage for the new override update behavior
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/corelib/src/settings/objectWithOverrides.ts | Completely rewrote updateOverrides function with improved logic for preserving overrides and handling nested objects |
| packages/corelib/src/settings/tests/objectWithOverrides.spec.ts | Added extensive test cases covering various override update scenarios |
| meteor/server/api/rest/v1/typeConversion.ts | Refactored studio creation to preserve existing studio fields using spread operator |
| meteor/server/api/rest/v1/tests/typeConversions.spec.ts | Added new test file with tests for studio field preservation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ianshade
added
the
Contribution from TV 2 Norge
ianshade
force-pushed
the
contribute/EAV-588
branch
from
725da4a to
5c235bc
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe changes introduce a new helper function to construct DBStudio objects from API data with blueprint manifest integration, refactor override handling in the object utilities to use a two-phase preservation-and-generation model, and add comprehensive test coverage for both the studio type conversion and override update logic. Changes
Sequence Diagram(s)sequenceDiagram
participant API as API Request
participant studioFrom as studioFrom()
participant buildStudio as buildStudioFromResolved()
participant manifest as Blueprint Manifest
participant result as DBStudio Result
API->>studioFrom: apiStudio, studioId
studioFrom->>studioFrom: Retrieve existingStudio from DB
studioFrom->>buildStudio: Pass apiStudio, existingStudio, manifest, IDs
buildStudio->>manifest: Access blueprintConfigFromAPI
manifest-->>buildStudio: Config function or undefined
buildStudio->>buildStudio: Compute blueprint config from API/existing
buildStudio->>buildStudio: Scaffold defaults, merge existingStudio, override with apiStudio
buildStudio-->>result: Consolidated DBStudio object
result-->>studioFrom: Complete studio with overrides
studioFrom-->>API: Final DBStudio
sequenceDiagram
participant caller as Caller
participant updateOverrides as updateOverrides()
participant preserve as getOverridesToPreserve()
participant generate as recursivelyGenerateOverrides()
participant result as Final ObjectWithOverrides
caller->>updateOverrides: curObj, rawObj, overrides
updateOverrides->>preserve: Determine overrides to keep
preserve->>preserve: Check if override still matches rawObj
preserve-->>updateOverrides: Filtered preservedOverrides
updateOverrides->>updateOverrides: Clone defaults, apply preserved overrides
updateOverrides->>generate: Find missing overrides in flattened state
generate->>generate: Traverse curObj vs rawObj recursively
generate->>generate: Detect set/delete operations on scalars and arrays
generate-->>updateOverrides: New override operations
updateOverrides-->>result: Defaults + preserved + new overrides
result-->>caller: Complete ObjectWithOverrides
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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
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 |
properties not present in apiStudio needed to be taken from the existing studio refactor(EAV-588): extract `studioFrom` logic ... and add test coverage refactor(EAV-588): small changes for clarity fix(EAV-598): make `updateOverrides` respect deep property overrides, not generate invalid overrides, and correctly preserve existing overrides fix(EAV-588): add null checks
ianshade
force-pushed
the
contribute/EAV-588
branch
from
5c235bc to
d6df5c1
Compare
|
@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: 1
🤖 Fix all issues with AI agents
In `@meteor/server/api/rest/v1/typeConversion.ts`:
- Around line 353-386: The code always resets supportedShowStyleBase to [] when
apiStudio omits it, overwriting existingStudio; change the assignment for
supportedShowStyleBase to preserve existing values when
apiStudio.supportedShowStyleBase is undefined: set supportedShowStyleBase to
apiStudio.supportedShowStyleBase !== undefined ?
apiStudio.supportedShowStyleBase.map(id => protectString<ShowStyleBaseId>(id)) :
(existingStudio?.supportedShowStyleBase ?? []), mirroring the pattern used for
settingsWithOverrides (and keep wrapDefaultObject/updateOverrides usage
unchanged).
♻️ Duplicate comments (2)
packages/corelib/src/settings/objectWithOverrides.ts (2)
2-5: Avoid deep import from type-fest’s internal path.
type-fest/source/readonly-deepisn’t part of the public API and can break on upgrades. IfReadonlyObjectDeepis exported from the package root in v4.33.0, prefer importing it fromtype-festalongsideReadonlyDeep. Otherwise considerReadonlyDeepor a local alias.♻️ Proposed refactor
-import { ReadonlyDeep } from 'type-fest' -import { ReadonlyObjectDeep } from 'type-fest/source/readonly-deep' +import { ReadonlyDeep, ReadonlyObjectDeep } from 'type-fest'Does type-fest v4.33.0 export ReadonlyObjectDeep from the package root?
153-162: Arrays are treated as atomic values, but recursion still dives into them.Line 153 handles arrays as single values, yet Line 160 recurses into arrays (they’re objects), which can emit per-index overrides and contradict the “arrays are treated as a single value” rule. Add an early guard for arrays (both
curValueandrawValue) andcontinue.🐛 Suggested fix
- if (Array.isArray(rawValue) && !_.isEqual(curValue, rawValue)) { - outOverrides.push({ - op: 'set', - path: fullKeyPathString, - value: rawValue, - }) - } - if (typeof curValue === 'object' && curValue !== null && typeof rawValue === 'object' && rawValue !== null) { + if (Array.isArray(rawValue) || Array.isArray(curValue)) { + if (!_.isEqual(curValue, rawValue)) { + outOverrides.push({ + op: 'set', + path: fullKeyPathString, + value: rawValue, + }) + } + continue + } + if (typeof curValue === 'object' && curValue !== null && typeof rawValue === 'object' && rawValue !== null) { recursivelyGenerateOverrides(curValue, rawValue, fullKeyPath, outOverrides) continue }
4 participants
About the Contributor
This pull request is posted on behalf of TV 2 Norge.
Type of Contribution
This is a:
Bug fix
Current Behavior
Updating Studio (Blueprint) Config using the HTTP API (updateStudioConfig operation) resulted in other Studio settings, e.g. peripheral devices to be lost.
New Behavior
Settings of the existing studio are preserved
Testing
Affected areas
This PR affects the HTTP API, specifically operations that update a Studio
Time Frame
Not urgent for us
Other Information
Status
Fix for Settings Loss During Studio Configuration Updates
Problem:
Updating studio configuration (Blueprint) through the HTTP API's
updateStudioConfigoperation inadvertently discarded existing studio settings such as peripheral device configuration, causing data loss.Solution:
The fix ensures that when a studio configuration is updated via the HTTP API, all existing studio properties not explicitly provided in the API request are preserved from the existing studio record, rather than being overwritten with defaults.
Key Changes:
New Helper Function (
buildStudioFromResolved)meteor/server/api/rest/v1/typeConversion.tsDBStudiofrom API dataexistingStudioparameter and merges it with the newapiStudiodatablueprintConfigFromAPIis a function or valueEnhanced Override Handling (
objectWithOverrides.ts)updateOverridesfunction with a two-phase approach:getOverridesToPreservehelper)recursivelyGenerateOverridesfunction)Comprehensive Test Coverage
meteor/server/api/rest/v1/__tests__/typeConversions.spec.tsforbuildStudioFromResolved, verifying that API-provided fields override existing studio fields while preserving ID fields and correctly applying overridespackages/corelib/src/settings/__tests__/objectWithOverrides.spec.tswith suite covering:Testing Status:
Unit tests added and verified; patch coverage at 80.59701%, with noted coverage gaps in override handling logic.
Impact:
Affected areas: HTTP API operations for studio configuration updates. Existing studio properties are now safely preserved during configuration updates, preventing unintended data loss.