Document Editing: Fix property variation change breaking document save via Infinite Editing (closes #21195)#21221
Conversation
…inite Editing (closes #21195) When changing a property's variation setting (Shared/Invariant ↔ Variant) via Infinite Editing, the document would fail to save with a 404 error. This fix: - Adds value migration fallback logic in UmbPropertyValuePresetVariantBuilderController to find values when culture/segment doesn't match exactly - Overrides reload() in UmbContentDetailWorkspaceContextBase to process incoming data through _processIncomingData() for proper value transformation - Detects property variation changes and triggers document reload to migrate values 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical issue where changing a property's variation setting (Shared/Invariant ↔ Variant) via Infinite Editing would cause document saves to fail with 404 errors. The solution implements value migration fallback logic and ensures proper data transformation during reload operations.
Key Changes
- Value Migration Logic: Added
#findExistingValue()method that implements intelligent fallback logic to handle variation setting changes, allowing values to migrate between invariant and variant states - Property Variation Tracking: Implemented a cache-based observer system to detect when property variation settings change and trigger automatic reloads to get properly migrated values
- Reload Override: Overrode the base class
reload()method to ensure incoming data is processed through the value migration pipeline
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
property-value-preset-variant-builder.controller.ts |
Adds #findExistingValue() method with fallback logic to find values when culture/segment doesn't match exactly, supporting both invariant→variant and variant→invariant migrations |
property-value-preset-variant-builder.controller.test.ts |
Adds comprehensive test coverage for the new migration scenarios with 2 new test cases validating both migration directions |
content-detail-workspace-base.ts |
Implements property variation change detection using a cache and observer, overrides reload() to process data through _processIncomingData() for proper value transformation |
...mbraco.Web.UI.Client/src/packages/content/content/workspace/content-detail-workspace-base.ts
Show resolved
Hide resolved
AndyButland
left a comment
There was a problem hiding this comment.
I've tested before and after this PR, and can verify that I can replicate the issue before the fix, but not after. The solution code-wise looks good to me to, so will approve so we can have this in for the 17.1 RC.
|
|
||
| if (data) { | ||
| // Process the data through _processIncomingData to handle value migration | ||
| const processedData = await this._processIncomingData(data); |
There was a problem hiding this comment.
This seems like we should correct it in the entity-detail-workspace-base.ts instead of just fixing it here.
|
|
||
| // If variation settings changed, reload to get properly migrated values | ||
| if (hasChanges) { | ||
| this.reload(); |
There was a problem hiding this comment.
This is a solution, but maybe also a unwished solution.
When we integrate signal-r it may be a change coming from a different computer, so would we then just want the Editor to lose their work?
| return exactMatch.value; | ||
| } | ||
|
|
||
| // No exact match - try fallback for variation setting changes |
There was a problem hiding this comment.
This new logic feels unsafe to me. There are a few things to be aware of here.
First of all, I can't seem to make it do any difference, if I remove lines 124 to 138, I still get the same experience.
I think its because in the described case, the reload is triggered, so I'm unsure why this is implemented?
And it feels good to do a reload, but as mentioned above maybe thats not what the Editor wanted. So we may need to look into ways to migrate current changes unto a new version from the server. where the changes to fields that got their variant-configs changed might need to be lost.
But back to this code:
If it ever was to actually have an effect, I would be afraid it could lead to unwished value transfers.
It seems from reading the code that changing from invariant to variant would lead in the invariant value being transferred to all new variants. This is not how the server would handle it, so that would be wrong if it took place. (well, which I can't make happen so.. :-/ )
Also if there was an old invariant value, is that want we want to fallback to. That should not happen on Documents, but how about Blocks? We are not that good at cleaning up values here.
— Resulting in new culture variants getting the old invariant value, instead of the proper preset value.
So I feel this is a very sensitive spot, so let's make sure we feel 100% sure about this with good test coverage before we make this happen. And let's make sure we do not have a different data handling than the one happening on the server.
…inite Editing (closes #21195) (#21221) When changing a property's variation setting (Shared/Invariant ↔ Variant) via Infinite Editing, the document would fail to save with a 404 error. This fix: - Adds value migration fallback logic in UmbPropertyValuePresetVariantBuilderController to find values when culture/segment doesn't match exactly - Overrides reload() in UmbContentDetailWorkspaceContextBase to process incoming data through _processIncomingData() for proper value transformation - Detects property variation changes and triggers document reload to migrate values 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Summary
reload()to process incoming data through_processIncomingData()for proper value transformationFixes #21195
Root Cause
When a property's variation setting was changed via Infinite Editing, the document's values in memory still had the old culture/segment format. The base class
reload()method was fetching raw data without processing it through_processIncomingData(), which contains the value migration logic.Changes
property-value-preset-variant-builder.controller.ts: Added#findExistingValue()method with fallback logic:content-detail-workspace-base.ts:reload()to call_processIncomingData()for proper value transformationTest plan
🤖 Generated with Claude Code