[Data Object Editor] Restore (inherited) changes - #4026
Open
ValeriaMaltseva wants to merge 7 commits into
Open
ValeriaMaltseva wants to merge 7 commits into
ValeriaMaltseva wants to merge 7 commits into
Conversation
Adds a per-field Restore action that undoes the change which broke a field's inheritance, so the field takes its value from the origin object again. The value a field would inherit is only known for fields that were inherited when the editor was opened - inheritanceData carries the origin id and a flag, never a value - so the action is offered for those and not for fields that already carried an own value. Where the value lives decides how it is put back: - Plain fields are held by the Ant form store, so the form resets them to the loaded value and the field is separately persisted as empty, since an auto save may already have written the own value into the draft. The empty value comes from the field type, because the block API takes a list of items and rejects null. - Inside a Form.KeyedList - object bricks and the classification store - the value is held by the list, and each of the two encodes inherited fields in its own payload (null for bricks, absence for the store), so its owner drops its own bookkeeping for the field and re-emits the payload. A synchronous marker is needed there because restoring the inheritance state only schedules a React update. Also fixes getMergedValue dropping the inherited values of a classification store group once that group holds no own values any more. filterInheritedFields encodes such a group as an empty list, which getFieldList reports as a field of its own and the merge then wrote over the values merged in for the keys of the group. Restoring the last own key of a group made that reachable. Verified: tsc, eslint and the jest suite pass, and the SDK builds. Not verified in a browser. Resolves #1045 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds per-field inheritance restoration to the Data Object Editor.
Changes:
- Adds Restore UI, translations, and inheritance-state handling.
- Supports plain fields, blocks, object bricks, and classification stores.
- Fixes classification-store merging and adds tests.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
translations/studio.sv.yaml |
Adds Swedish label. |
translations/studio.no.yaml |
Adds Norwegian label. |
translations/studio.it.yaml |
Adds Italian label. |
translations/studio.fr.yaml |
Adds French label. |
translations/studio.es.yaml |
Adds Spanish label. |
translations/studio.en.yaml |
Adds English label. |
translations/studio.de.yaml |
Adds German label. |
dynamic-type-object-data-block.tsx |
Defines block empty value. |
use-restore-inheritance.ts |
Implements restore behavior. |
use-restore-inheritance.test.tsx |
Tests restore branches. |
field-label.tsx |
Displays Restore action. |
dynamic-type-object-data-abstract.tsx |
Adds empty-value API. |
object-brick.tsx |
Restores brick fields. |
group-value.ts |
Fixes inherited group merging. |
group-value.test.ts |
Tests group merging. |
classification-store.tsx |
Restores classification keys. |
inheritance-state-provider.tsx |
Adds restore state transition. |
inheritance-state-provider.test.tsx |
Tests state restoration. |
clear-form-field.ts |
Builds cleared payloads. |
clear-form-field.test.ts |
Tests cleared payloads. |
edit-form-provider.tsx |
Exposes field-clearing support. |
restore-inheritance-button.tsx |
Adds translated Restore button. |
keyed-list-provider.tsx |
Exposes restore callback. |
keyed-list.tsx |
Routes field restoration. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ValeriaMaltseva
marked this pull request as ready for review
September 1, 2026 12:34
Adds a Restore action to the label of a field whose inheritance was broken during the editing session, so the field can be given back to its origin object without reloading the editor. The action is served through a new end-of-label slot: LabelExtraProvider offers a node, and both label implementations render it after everything the item appends to the label (locale suffix, tooltip icon, required mark) - the Ant form item via the requiredMark renderer, and the virtual item of the keyed and numbered lists directly. RestoreInheritanceLabelExtraProvider fills that slot in the object editor, so a single node decides per item whether the field offers a restore, instead of every FieldLabel carrying the empty value. Object data blocks keep their own action on the accordion header: block inheritance is all-or-nothing and the block form item carries no label the generic slot could render into. The action sits on the first text line of the label rather than being centred on the label row - the row is taller than that line, so centring left it a couple of pixels below the field name in virtual items - and it takes exactly one line box, which the row is at least that tall already, so it cannot move the field name or the row when it appears or disappears. Its divider gets the same 4px on both sides. Verified in the object editor on both label hosts: the field name and the row keep their position when the action appears, and the action shares the field name's baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ValeriaMaltseva
force-pushed
the
feat/1045-restore-inherited-changes
branch
from
September 3, 2026 11:59
c485784 to
e8218fe
Compare
An auto save issued while another save is in flight was returned from without a trace. That is safe as long as an edit follows: the payload accumulates in the edit form provider, so the next auto save carries what the dropped one would have. It does not hold for a write with no edit behind it. Restoring a field's inheritance resets the form, which Ant does not report through onValuesChange, so nothing follows to carry the cleared value - the form shows the origin value while the draft keeps the own one, and the difference only surfaces on the next reload. Such an auto save is now queued instead. It is self-coalescing, because editableData is the whole accumulated payload: running the latest once after the current save is enough, whatever collided in between. Only behind another auto save, though. Behind a save or a publish there is nothing to rescue - that task carries the same accumulated payload and resets it when it finishes, so running a snapshot of it afterwards would put a draft back on an object the user just published. The queued task is also read and claimed through a ref rather than the state. useSave is mounted in several places that share this context, and the state value can still name the task in another instance running the same effect in the same tick, which sends it twice. Clearing through setQueuedTask updates the ref synchronously, so only the first instance gets it. That ref already existed in the provider and is now exposed on the context. Covers restores inside object bricks and the classification store as well: they reach the draft through the same save. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A critical nested-block persistence defect and several moderate save, state-race, and interaction issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
assets/js/src/core/modules/data-object/components/restore-inheritance-button.tsx:35
- The new button embeds its visual rules in the component, but this codebase requires component styling to be placed in a dedicated
.styles.tsfile usingcreateStyles(). Please extract these rules and pass the generated class name instead.
style={ { fontSize: 'inherit', lineHeight: 'inherit', padding: 0, border: 0, height: 'auto' } }
assets/js/src/core/modules/data-object/components/restore-inheritance-label-extra.tsx:54
- These component styles are defined inline, contrary to the repository requirement that component styling use a dedicated
.styles.tsfile withcreateStyles(). Moving the label-action layout rules into a style hook also avoids recreating style objects on each render.
style={ {
marginInlineStart: token.marginXXS,
// Placed on the label's first line, not centered on the label row: the
// row is taller than that line — both hosts pad it, and the virtual item
// adds the descender of the inline box its label text sits in — so
// centering on the row leaves the action a couple of pixels below the
// field name. Starting at the top of the row and taking exactly one line
// box (1lh, the line height the label renders at, whatever that is) puts
// the action on the same line as the field name in a label of any size,
// and keeps it from growing the row, which is at least one line tall
// already, when it appears or disappears.
alignSelf: 'flex-start',
height: '1lh'
} }
- Files reviewed: 35/36 changed files
- Comments generated: 5
- Review effort level: Balanced
Comment on lines
+50
to
+56
| const isKeyedList = keyedList !== undefined | ||
| const isBroken = name !== undefined && | ||
| inheritanceStateContext?.getInheritanceState(name)?.inherited === 'broken' | ||
|
|
||
| const canRestore = isBroken && ( | ||
| isKeyedList | ||
| ? keyedList.onFieldRestore !== undefined |
Comment on lines
+69
to
73
| const executeTask = queuedTaskRef?.current | ||
|
|
||
| if (!isNil(executeTask)) { | ||
| setQueuedTask(undefined) | ||
| await save(executeTask.editableData, executeTask.task) |
Comment on lines
+93
to
+94
| const queuesBehindAutoSave = runningTaskRef?.current === SaveTaskType.AutoSave && | ||
| (isNil(queuedTaskRef?.current) || queuedTaskRef.current.task === SaveTaskType.AutoSave) |
| return ( | ||
| <IconTextButton | ||
| icon={ { value: 'corner-up-left' } } | ||
| onClick={ onRestore } |
Comment on lines
+118
to
+123
| startTransition(() => { | ||
| setInheritanceStates(prevStates => ({ | ||
| ...prevStates, | ||
| [key]: initialState | ||
| })) | ||
| }) |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #1045, the remaining unchecked item of #291.
Adds a per-field Restore action that undoes the change which broke a field's inheritance, so the field takes its value from the origin object again.
Scope
inheritanceDatacarries only the origin id and a flag per field, never a value, so the value a field would inherit is known only for fields that were inherited when the editor was opened. The action is therefore offered for those, and not for fields that already carried an own value — covering the latter needs a backend endpoint that returns the inherited value plus origin id, which does not exist today.Field collections, calculated values, consent, URL slugs and reverse relations never get the action, because
supportsInheritance()is false for them.How it works
useRestoreInheritancepicks the branch by where the value actually lives.Plain fields are held by the Ant form store:
form.resetFields([name])puts the loaded (inherited) value back. Ant does not report a reset throughonValuesChange, so the field is not counted as changed again.DynamicTypeObjectDataAbstract.getEmptyValue) —nullfor almost everything, but[]for blocks, whose adapter takesarrayand would throw aTypeErroronnull. I checked every data adapter;BlockAdapteris the only one that cannot takenull.Object bricks and the classification store hold their values in a
Form.KeyedList, which the Ant form store never sees, and each encodes inherited fields in its own payload —nullfor bricks, absence for the store. So the owner of the list restores the field instead: it drops its own "field was changed" bookkeeping and re-emits the payload, and the displayed value fixes itself becausegetMergedValuethen reads the field from the loaded data. A synchronous marker (restoredFieldsRef, mirroring the existingchangedFieldsRef) is needed because restoring the inheritance state only schedules a React update, which the payload built right after must not wait for.KeyedListgains one optionalonFieldRestoreprop, the counterpart of the existingonFieldChange. It is the channel the label uses to reach the owner.components/pipeline/*passes nothing and is unaffected.Blocks
Block inheritance is all-or-nothing at field level —
BlockAdapterdoes not implementDataInheritanceInterface, so the generic path yields one state for the whole block field. Restore therefore sits on the block's own label, not on its inner fields, which matches that granularity.Drive-by fix
getMergedValuedropped the inherited values of a classification store group once that group held no own values any more.filterInheritedFieldsdeliberately encodes such a group as an empty list,getFieldListreports that as a field of its own, and the merge then wrote over the values already merged in for the keys of the group. Restoring the last own key of a group made this reachable.getMergedValuehad no tests; it has three now.Verified
tsc --noEmit,eslint, and the jest suite (562 tests) pass;npm run dev-sdkbuilds.useRestoreInheritance, and thegetMergedValueregression.Follow-ups, not in here
activeGroups,groupCollectionMapping,DELETEDmarkers). Different semantics, and destructive enough to want a confirmation.getMergedValueingroup-value.tsthrows iforiginalValueholds a group that the current value does not (the siblingbrick-value.tsguards this); andDataComponentbuildsvirtualFieldNamewithout theForm.Listpath, so a field inside a block can pick up the inheritance overlay of a same-named top-level field.