Repository navigation
feat(visual-builder): show fields disabled when another session changes or deletes their content type (DRFT-927) - #665
Conversation
…es or deletes their content type (DRFT-927) Adds the contentTypeUpdated and contentTypeDeleted restrictions with their own messages.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
hitesh-shetty-cstk
left a comment
There was a problem hiding this comment.
Automated review
What this changes: Adds two entry edit restrictions, contentTypeUpdated and contentTypeDeleted, to the SDK's mirror of parent-owned lock state. Each gets a message in ENTRY_RESTRICTION_MESSAGES and an entry in the runtime allowlist, so a field whose entry carries one renders disabled with that reason. No consumer branches on the restriction value, so both new values flow through the existing paths unchanged.
Business impact: This sits on inline editing of the canvas, specifically whether a field opens for editing. The change is additive on the SDK side: both values only take effect once the parent sends them, and an entry with no restriction behaves exactly as before. The failure a content editor would see is on the parent's side of the contract, not this diff: over-report the restriction and fields are disabled that should be editable, under-report it and an entry whose content type is gone still accepts edits that cannot be saved. The second case is the subject of the inline comment on fieldLockStore.ts.
Security: Nothing beyond what the scanners cover. The new values arrive over postMessage and go through toEntryEditRestriction, which narrows against a fixed allowlist and treats anything else as no restriction, so an unexpected payload cannot inject a value. The messages are static constants passed as a prop rather than interpolated into markup, so the new copy adds no injection path.
Flow
sequenceDiagram
participant Editor as Entry editor (parent)
participant Handler as useEntryEditRestrictionUpdateEvent
participant Snapshot as getEntryLockInfo
participant Store as fieldLockStore
participant Field as Canvas field
Note over Editor: content type changed or deleted
Editor->>Handler: ENTRY_EDIT_RESTRICTION_UPDATE
Handler->>Store: toEntryEditRestriction (allowlist)
Note over Handler,Store: changed here
Snapshot->>Editor: GET_ENTRY_LOCK_INFO (once per scope)
Editor-->>Snapshot: editRestrictions
Snapshot->>Store: seedEntryEditRestrictions
Field->>Store: getEntryEditRestrictionForField
Store-->>Field: message, field renders disabled
Findings: 0 blocker, 2 should fix, 1 nit. All three are inline.
The test changes are additive. No assertion was weakened, no case deleted or skipped, no snapshot regenerated, no mock loosened. beforeEach clears both the mocks and the store, so the new cases stay isolated. One assertion that looks redundant is not: comparing getEntryEditRestrictionForField against ENTRY_RESTRICTION_MESSAGES[restriction] passes when both are undefined, and the expect.any(String) line after it is what catches a restriction that reaches the allowlist with no message behind it.
Reviewer candidates: None. Commit history on the base branch names only the pull request author across all three changed files, and CODEOWNERS maps everything to a team rather than to individuals, so there is no second name to draw from. No review was requested.
Not covered: No build, no test run, no manual check of the canvas. The editor side of this contract is a separate change in another repository and was not read, so the question in the fieldLockStore.ts comment is open rather than confirmed. The security-sca check is red on this head; the diff touches no dependency or manifest file, so that failure does not look attributable to this change, but it was not traced to its cause.
Automated review by Claude Code. A human review is still required.
Generated by Claude Code
| | "contentTypeUpdated" | ||
| | "contentTypeDeleted" |
There was a problem hiding this comment.
Should fix: contentTypeUpdated and contentTypeDeleted describe the content type, but they are stored per entry scope (entryUid.locale[.variant]), the same as the four restrictions that genuinely are per entry.
A deleted or changed content type affects every entry of that type on the canvas, in every locale. The SDK seeds restrictions per entry, when requestEntryLockInfoOnce fetches that entry's snapshot, and it never re-requests a scope it has already cached (requestedScopes). So an entry first hovered after the deletion picks the restriction up from its own snapshot, while an entry whose snapshot was cached earlier in the session only gets it if the parent pushes an ENTRY_EDIT_RESTRICTION_UPDATE for that specific scope.
Does the editor side fan that update out to every entry scope of the affected content type, or only to the entry currently open in the form? If it is only the open entry, a page rendering several entries of that type leaves the rest editable after the type is deleted, which is the case the change is named for. I cannot read the editor change from this review, so this may already be handled there.
Generated by Claude Code
There was a problem hiding this comment.
The editor does fan it out. getEntryEditRestrictionsByScope sends the reason for every loaded entry of that content type at the current locale, plus its variant scopes, whether or not the entry was opened. The same map feeds the lock snapshot, so a scope requested later gets it too. On dev11 a shoe card that was never opened showed the lock.
| "contentTypeUpdated", | ||
| "contentTypeDeleted", |
There was a problem hiding this comment.
Should fix: this set and the EntryEditRestriction union above are two hand-maintained copies of the same list, and only one of them is checked by the compiler.
ENTRY_RESTRICTION_MESSAGES is typed Record<EntryEditRestriction, string>, so a missing message fails the build. This set is ReadonlySet<string>, so a value added to the union and to the message map but missed here still compiles, and then silently never applies: toEntryEditRestriction returns null and the field stays editable. This change had to edit both lists and edited both correctly. The next one may not.
Deriving both from one list removes the drift:
export const ENTRY_EDIT_RESTRICTIONS = [
"olderVersion",
"entryLocked",
"contentTypeUpdated",
"contentTypeDeleted",
"unlocalized",
"unsavedVariant",
] as const;
export type EntryEditRestriction = (typeof ENTRY_EDIT_RESTRICTIONS)[number];
const RESTRICTIONS: ReadonlySet<string> = new Set(ENTRY_EDIT_RESTRICTIONS);Generated by Claude Code
There was a problem hiding this comment.
Done. Both now come from ENTRY_EDIT_RESTRICTIONS, and the it.each test runs over the same list.
| setEntryEditRestriction(scope, "contentTypeUpdated"); | ||
| expect(getEntryEditRestrictionForField(meta())).toBe( | ||
| ENTRY_RESTRICTION_MESSAGES.contentTypeUpdated | ||
| ); | ||
| setEntryEditRestriction(scope, "contentTypeDeleted"); | ||
| expect(getEntryEditRestrictionForField(meta())).toBe( | ||
| ENTRY_RESTRICTION_MESSAGES.contentTypeDeleted | ||
| ); |
There was a problem hiding this comment.
Nit: these four lines are now covered by the it.each block at the end of the file, which runs the same assertion across all six restrictions. Dropping them here leaves one place to update when a restriction is added.
Generated by Claude Code
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
hitesh-shetty-cstk
left a comment
There was a problem hiding this comment.
Automated review
Re-review after new commits (pass 2 of 4).
What this changes: One commit since the last pass. The six restriction values move into a single exported ENTRY_EDIT_RESTRICTIONS tuple, and both EntryEditRestriction and the runtime RESTRICTIONS set are derived from it instead of repeating it. The parameterised test iterates that tuple rather than its own copy. The values and their order are unchanged, so toEntryEditRestriction accepts and rejects exactly what it accepted and rejected before. The commit addresses all three findings from pass 1.
Business impact: None identified. This code gates inline editing on the canvas, but the allowlist the commit rewrites is identical to the one it replaces, so no field changes its editable state as a result. The behaviour the description is named for still depends on the editor-side change it links, which is open and unmerged; until that lands nothing sends the two new restrictions.
Security: Nothing beyond what the scanners cover.
Flow
No flow change. The commit moves where a list of constants is declared. It does not alter the path a restriction takes from the editor to a disabled field.
Findings: 0 blocker, 0 should fix, 2 nit. Both are inline, and neither needs to block a merge.
Test changes: No assertion was weakened in substance, no case deleted or skipped, no snapshot regenerated, no mock loosened. The two content type cases dropped from "returns a distinct message per restriction" are the ones the it.each block already covers, which is what the pass-1 nit asked for. The one real change in test strength is that it.each now reads its cases from the list under test, and the inline note covers that.
Pass-1 follow-ups: The content type fan-out question is settled. The editor-side change this description links returns a restriction for every loaded entry of the affected content type and for its variant scopes, not only for the entry open in the form, and it carries tests for that case. I read it to confirm the reply rather than taking it on trust. The duplicated allowlist and the redundant test assertions are both resolved.
Reviewer candidates: None requested. Assignment runs on the opening pass only, and pass 1 found no candidate: commit history on the base branch names only the author across these files, and CODEOWNERS maps everything to a team rather than to individuals.
Not covered: No build, no test run, no check of the canvas. The test check was still running when this was written. security-sca is red on this head and was red on the previous one. The diff touches no manifest or dependency file and the Snyk open source, licence and code scans are green, so the failure does not look attributable to this change, but I did not trace it to its cause and the base commit carries no recorded check runs to compare against.
Automated review by Claude Code. A human review is still required.
Generated by Claude Code
| }); | ||
|
|
||
| describe("content type restrictions from the editor", () => { | ||
| it.each(ENTRY_EDIT_RESTRICTIONS)( |
There was a problem hiding this comment.
Nit: the cases now come from the list this test is checking, so the test no longer pins that list's membership on its own. Drop a value from ENTRY_EDIT_RESTRICTIONS and this block runs one case fewer and stays green, where the hardcoded array it replaced would have failed.
Most of that gap is covered elsewhere. ENTRY_RESTRICTION_MESSAGES is typed Record<EntryEditRestriction, string>, so a removal that leaves the message behind is an excess-property error at compile time. Only removing the value and its message together slips past both, and that is a deliberate edit rather than a slip.
I would take this as it stands. The duplication it removes is worth more than the oracle it gives up. Flagging it so the tradeoff is on the record rather than implied.
Generated by Claude Code
There was a problem hiding this comment.
Agreed, keeping it as is.
| "unsavedVariant", | ||
| ] as const; | ||
|
|
||
| /** Why the parent has turned editing off for a whole entry scope. */ |
There was a problem hiding this comment.
Nit: this line and the comment on ENTRY_EDIT_RESTRICTIONS nine lines up say the same thing in two tenses. Now that the type is (typeof ENTRY_EDIT_RESTRICTIONS)[number], the const's comment already describes it. Deleting this one leaves the description in one place.
Generated by Claude Code
There was a problem hiding this comment.
Fair. Leaving it for now to avoid another CI round on a comment-only change.
Shows a canvas field as disabled when the editor reports that another session changed or deleted its entry's content type.
Adds the
contentTypeUpdatedandcontentTypeDeletedrestrictions to the allowed values, with the messages "This entry's content type was changed. Reload the entry to keep editing." and "This entry's content type was deleted. It can no longer be edited here." Variant fields fall back to the base entry's restriction as before.Ticket: DRFT-927. Editor side: https://github.com/contentstack/visual-builder/pull/2800
Testing: unit tests for both restrictions, their messages and the lock snapshot. Full suite: 944 passed. Checked on dev11 with a local build, where the changed type's fields showed disabled with the message.