Repository navigation
feat(visual-builder): show fields disabled while the entry is locked by another session (DRFT-925) - #664
Conversation
…by another session (DRFT-925) Adds the entryLocked restriction and its message, so the canvas blocks inline edits after a collaborator saves until the entry is reloaded.
✅ 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 a fourth entry-wide edit restriction, entryLocked, so the canvas can disable a field when the parent reports that another session changed the entry underneath the editor. The union, the runtime allowlist that narrows values arriving from the parent, and the message map are all updated, plus one test assertion. Every consumer reads a restriction as an opaque message or boolean, so no call site needed a new branch, and I confirmed none was missed.
Business impact: Inline editing on the canvas. Working as intended, a user editing an entry that someone else has already saved sees the field go disabled with the new message instead of typing into state that is about to be thrown away. The failure mode worth guarding is the quiet one: if the value does not survive the parent-to-SDK boundary, toEntryEditRestriction returns null, no restriction is recorded, and the field stays editable on a stale entry with no visible sign anything is wrong. That single boundary line is the one thing this change does not test, which is the should-fix below.
The editor side that emits this value lives in another repository and is not reviewed here, so the string literal matching on both sides is taken on trust. Worth confirming the two ship in an order where the canvas is never the half that is behind.
Security: Nothing beyond what the scanners cover. The guard that matters, the allowlist in toEntryEditRestriction, is intact and still rejects arbitrary parent strings before they can reach a message lookup; this change widens it by exactly one known literal. The message itself is a static constant with no user or entry content interpolated into it.
Flow
sequenceDiagram
participant P as Entry editor (parent)
participant H as useEntryEditRestrictionUpdateEvent
participant S as fieldLockStore
participant F as Canvas field
P->>H: ENTRY_EDIT_RESTRICTION_UPDATE {restriction: "entryLocked"}
H->>S: toEntryEditRestriction(value)
Note over S: changed here: "entryLocked" added to RESTRICTIONS
S->>S: setEntryEditRestriction(scope, "entryLocked")
S-->>F: lock listeners fire
F->>S: getEntryEditRestrictionForField(meta)
S-->>F: restriction message
Note over F: field renders disabled
Findings: 0 blocker, 1 should fix, 1 nit. Both are inline.
Reviewer candidates: None. All three changed files are solo-authored by the PR author, so commit history offers no second reader and no review was requested from it. Since this touches canvas inline editing, @karancs06 is the nearest owner of the surrounding src/visualBuilder/utils area by commit count and would be a sensible person to walk through it before merge. That is a suggestion, not a request.
Not covered: I did not run the test suite, build, or open the app; the testing claims in the description are taken at face value. The security-sca check is failing on this head and its log is not reachable from here, but the diff changes no dependency manifest, so it is not attributable to this change. The parent-side emitter was not reviewed.
Automated review by Claude Code. A human review is still required.
Generated by Claude Code
| setEntryEditRestriction(scope, "entryLocked"); | ||
| expect(getEntryEditRestrictionForField(meta())).toBe( | ||
| ENTRY_RESTRICTION_MESSAGES.entryLocked | ||
| ); |
There was a problem hiding this comment.
should fix — This exercises the union member and the message, but not the line that makes the feature work across the parent boundary.
setEntryEditRestriction takes an already-typed EntryEditRestriction and writes it straight to the store, so nothing here passes through toEntryEditRestriction. The "entryLocked" entry you added to RESTRICTIONS in fieldLockStore.ts is the only thing that lets the parent's string survive, and it has no coverage: delete that one line and this suite still passes while a locked entry stays fully editable on a real canvas.
Worth a case on the parent path, alongside the existing ENTRY_EDIT_RESTRICTION_UPDATE test in the restriction updates block:
it("accepts entryLocked from the parent", () => {
useEntryEditRestrictionUpdateEvent();
const handler = mockPostMessage.on.mock.calls[0][1] as any;
handler({ data: { ...scope, restriction: "entryLocked" } });
expect(getEntryEditRestriction(scope)).toBe("entryLocked");
});Generated by Claude Code
|
|
||
| const RESTRICTIONS: ReadonlySet<string> = new Set([ | ||
| "olderVersion", | ||
| "entryLocked", |
There was a problem hiding this comment.
nit — This set repeats the EntryEditRestriction union with nothing tying the two together, so they can drift silently.
ENTRY_RESTRICTION_MESSAGES is typed Record<EntryEditRestriction, string>, so a missing message breaks the build. This set is a ReadonlySet<string>, so the next restriction added to the union compiles clean while toEntryEditRestriction quietly rejects it at runtime, which fails in the direction nobody checks. You updated both correctly here; nothing makes the next person do the same.
Keying it by the union turns that into a compile error:
const RESTRICTIONS: Record<EntryEditRestriction, true> = {
olderVersion: true,
entryLocked: true,
unlocalized: true,
unsavedVariant: true,
};toEntryEditRestriction would then test with Object.prototype.hasOwnProperty.call(RESTRICTIONS, value). Deriving the set from the messages record would be the other option, but it would import fieldLockIndicator back into this module and close an import cycle, so I would not go that way.
Generated by Claude Code
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
Shows a canvas field as disabled when the editor reports that another session saved or discarded its entry.
Adds the
entryLockedrestriction to the allowed values, with the message "This entry was updated by someone else. Reload the entry to keep editing." Variant fields fall back to the base entry's restriction as before, so a base-entry lock covers its variants.Ticket: DRFT-925. Editor side: https://github.com/contentstack/visual-builder/pull/2796
Testing: unit test for the new restriction and its message. Full suite: 936 passed. Checked on dev11 with a local build, where the locked entry's fields showed disabled with the message.