Repository navigation
feat(visual-builder): show fields disabled when the stack turns auto-draft off (DRFT-771) - #667
Conversation
…draft off (DRFT-771)
✅ 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 autoDraftDisabled to the restrictions the SDK accepts from the editor, with the message shown on the field, so canvas fields render disabled once the stack turns auto-draft off mid-session. ENTRY_RESTRICTION_MESSAGES is typed Record<EntryEditRestriction, string>, so the compiler already required the message alongside the new union member. The variant fallback to the base entry's restriction is unchanged.
Business impact: Inline editing on the canvas. A field that should be read-only stays editable whenever the SDK does not recognise the value, because toEntryEditRestriction turns anything unrecognised into no restriction. The description plans for that by shipping the SDK with or before the editor, which holds for this release. The comment on fieldLockStore.ts asks about the default itself rather than this release.
Security: nothing beyond what the scanners cover. The added value is a fixed string in an allowlist and the message is a compile-time constant, so no caller-supplied content reaches the DOM through this path.
Flow
sequenceDiagram
participant Editor as Entry editor (parent)
participant Handler as SDK restriction handler
participant Store as fieldLockStore
participant Field as Canvas field
Editor->>Handler: ENTRY_EDIT_RESTRICTION_UPDATE (restriction autoDraftDisabled)
Note over Handler: toEntryEditRestriction now accepts this value
Handler->>Store: setEntryEditRestriction for the entry scope
Store-->>Field: lock listeners fire
Field->>Field: getEntryEditRestrictionForField
Note over Field: renders disabled, carrying the new message
Findings: 0 blocker, 2 should fix, 1 nit. All three are inline.
Reviewer candidates: none. Commit history on all three changed files, taken on the base branch, shows a single author, who is the author of this PR, so the query returns no second owner. CODEOWNERS maps every path to a team rather than to people. Someone else on the team should still read this, because it changes when canvas fields go read-only.
Not covered: no test run, no build, and no check of the canvas in a browser. The security-sca check is red on this commit with an empty output, so I could not tell whether it relates to this change; no dependency or lockfile is in the diff. The editor half of the contract lives in another repository and is not reviewed here, beyond confirming that it sends this same value on both the push and the snapshot path.
Automated review by Claude Code. A human review is still required.
Generated by Claude Code
| "entryLocked", | ||
| "contentTypeUpdated", | ||
| "contentTypeDeleted", | ||
| "autoDraftDisabled", |
There was a problem hiding this comment.
should fix. toEntryEditRestriction maps any value it does not recognise to null, which the rest of the SDK reads as no restriction, so an SDK built before this line pairs with the new editor by leaving the field editable. The description covers this release by shipping the SDK first, which works, but the same gap reopens for every restriction value added after this one.
Worth a team decision rather than a change in this PR: should an unrecognised restriction string block editing with a generic reason instead of clearing it? Fail-open is the riskier default when the parent is the source of truth for whether editing is allowed. I am not confident enough to call it a defect here. Changing it affects every path through seedEntryEditRestrictions, and a parent that sends a typo would then lock the canvas with no way back.
Generated by Claude Code
There was a problem hiding this comment.
Agreed it's worth a team decision. Leaving the fail-open default as is in this PR, since a typo from the parent would otherwise lock the canvas with no way back. This release ships the SDK first.
| autoDraftDisabled: | ||
| "Auto-draft is disabled for this stack.", |
There was a problem hiding this comment.
should fix. Every other message in this map ends with the action that clears it, such as reloading the entry or saving from the form. This one states the condition and stops, so an editor who watches the field go read-only has nothing to act on. The parent shows a reload action for this same condition, so there is a next step to name.
Extending the copy would close it, for example Auto-draft is disabled for this stack. Reload the page to keep editing. I have not verified that reloading restores editing while the stack setting is off, so confirm the wording matches the real behaviour before taking it.
Generated by Claude Code
There was a problem hiding this comment.
The shorter copy was a product call. The page already shows a Reload button in the toast for this state, so the canvas message only says what happened. Keeping it as is.
| expect(isFieldBlockedByAutoDraft(meta({ variant: "v1" }))).toBe(true); | ||
| }); | ||
|
|
||
| it("disables the field and says auto-draft is off for the stack", () => { |
There was a problem hiding this comment.
nit. The it.each(ENTRY_EDIT_RESTRICTIONS) case above already drives this value through the same handler and asserts both that the store accepts it and that getEntryEditRestrictionForField returns the mapped message. isFieldBlockedByAutoDraft on a restricted entry is covered earlier in the file as well.
The one thing this case adds that the parametrised one cannot is the literal copy, since the other compares against ENTRY_RESTRICTION_MESSAGES and would pass through any copy edit. If pinning the copy is the intent, a short comment saying so would help, because the hardcoded string reads as an oversight next to the rest of the file. If it is not the intent, the case can go.
Generated by Claude Code
There was a problem hiding this comment.
Pinning the copy is intentional, since the parametrised case would pass through any copy edit. Tried adding a comment saying so, but a local commit hook blocked the commit, so I left the test as it is.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
Shows canvas fields as disabled when the editor reports that the stack turned auto-draft off mid-session.
Adds the
autoDraftDisabledrestriction to the allowed values, with the message "Auto-draft is disabled for this stack." Variant fields fall back to the base entry's restriction as before.This needs to ship with or before the editor change. An older SDK reads the new value as no restriction, so canvas fields would look editable.
Ticket: DRFT-771. Editor side: https://github.com/contentstack/visual-builder/pull/2805
Testing: unit test for the restriction and its message. Full suite: 946 passed. Checked on dev11 with a local build, where canvas fields showed disabled with the message after the stack turned auto-draft off.