Repository navigation
feat(visual-builder): disable fields of entries the editor has restricted (DRFT-913) - #663
Conversation
…cted (DRFT-913) Mirror per-entry edit restrictions (older version, unlocalized, unsaved variant) from the parent and show those fields disabled on hover, click, empty-block add and the field label, with a reason-specific message.
…the snapshot authoritative (DRFT-913) Entry restrictions no longer swallow the click, so the form can still open the entry (the way back to its latest version or to localize it); the field shows disabled and inline editing and add buttons stay off. The lock-info snapshot now also clears scopes it no longer lists, unknown restriction values are ignored, listeners fire only on real changes, and the restriction messages live with the indicator to avoid an import cycle.
✅ 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
hitesh-shetty-cstk
left a comment
There was a problem hiding this comment.
Automated review
What this changes: The SDK now mirrors a per-entry "editing is turned off" flag that the parent sends, either as a new entry-edit-restriction-update event or alongside the lock snapshot. fieldLockStore keeps it next to the field-lock mirror, and the hover outline, the cursor, the field label and the inline-edit gate read it so a restricted entry's fields render disabled with a reason-specific message. A click on a restricted field still reaches the parent, by design, so the form can open the entry.
Business impact: Inline editing on the canvas, and modular block structure actions. Two paths let a user still mutate an entry the editor has restricted. The field toolbar's Delete instance, Move instance and Replace actions are never gated on the restriction, so someone viewing an older version can delete a block from the canvas. Separately, the inline-edit gate reads the restriction once, synchronously, while the snapshot that populates it is deliberately not awaited, so a field can be made editable before the restriction is known and is never re-gated when it lands. Both are inline.
Security: Nothing beyond what the scanners cover. The restriction value from the parent is narrowed against a fixed allowlist before use and the messages are static constants, so no parent-supplied string reaches the label or the tooltip.
Flow
sequenceDiagram
participant Parent as Visual Builder (parent)
participant Snap as getEntryLockInfo
participant Store as fieldLockStore
participant UI as Hover, label, inline edit
Snap->>Parent: get-entry-lock-info
Parent-->>Snap: fieldLockInfo + editRestrictions
Snap->>Store: seedEntryEditRestrictions(entryUid, snapshot, seq)
Note over Snap,Store: changed here
Parent->>Store: entry-edit-restriction-update
Note over Parent,Store: new event, changed here
Store->>UI: notifyLockListeners()
UI->>Store: getEntryEditRestrictionForField(field)
Store-->>UI: message or null
Findings: 2 blocker, 2 should fix, 1 nit. All inline.
The write-ordering work in the store holds up. The per-scope sequence guard makes a late snapshot lose to a newer delta without dropping a concurrent request for a different entry, and the backward-compatibility skip when the parent omits editRestrictions is right. The test changes are genuine: the emptyBlock test follows a real rename of the function the component calls and keeps its assertions, and the new label tests assert user-observable state (the disabled class and the tooltip text) rather than internals. I found no test bent to pass.
Reviewer candidates:
- @faraazb-contentstack authored 5 of the last 30 commits on
src/visualBuilder/utils/handleIndividualFields.tsand 7 of the last 30 onsrc/visualBuilder/listeners/mouseClick.ts, the two files both blockers sit on. - @karancs06 authored 11 of the last 30 commits on
src/visualBuilder/components/fieldLabelWrapper.tsx, which this change rewires.
Since the inline-edit path is the critical journey here and @faraazb-contentstack owns it, please walk them through the gating before merging.
Not covered: No build, no test run, no app run. The editor side of this contract is in another repository and was not read, so the shape of editRestrictions and whether it covers every locale of an entry are unverified assumptions in two of the findings. Container-field propagation for restrictions was not exercised; peer locks propagate through ancestors in getPeerLockForField, and restrictions are entry-wide, so they should not need it.
Automated review by Claude Code. A human review is still required.
Generated by Claude Code
| // release. An entry-wide restriction does not stop here: the click still opens | ||
| // the form (the way back to the latest version), and the field shows disabled. |
There was a problem hiding this comment.
Blocker. Letting the click through means addOverlayAndToolbar runs for a restricted field, and FieldToolbar computes disableFieldActions from isFieldDisabled(...) alone (FieldToolbar.tsx:170); it never reads the entry restriction. On an entry the editor has restricted, the focused toolbar therefore renders with Delete instance, Move instance up/down, Replace and Edit-in-modal still enabled, and handleDeleteInstance / handleMoveInstance have no guard of their own. A user on an older version can delete a modular block instance from the canvas.
Peer locks never hit this because the gate above returns before the toolbar is built. Restrictions deliberately do not, so the toolbar needs the same treatment the inline-edit path got: fold getEntryEditRestrictionForField(fieldMetadata) into disableFieldActions in FieldToolbar.tsx.
Generated by Claude Code
| const disabled = | ||
| isFieldDisabled( | ||
| fieldSchema, | ||
| eventDetails, | ||
| resolvedVariantPermissions, | ||
| entryAcl, | ||
| entryWorkflowStageDetails | ||
| ).isDisabled || Boolean(getEntryEditRestrictionForField(fieldMetadata)); |
There was a problem hiding this comment.
Blocker. This reads the restriction synchronously, but the snapshot that populates it is fire-and-forget: fetchEntryPermissionsAndStageDetails starts it with void requestEntryLockInfoOnce(...) and explicitly does not await it (fetchEntryPermissionsAndStageDetails.ts:22). On the first interaction with an entry the restriction is usually still unknown here, so disabled is false and the field is made contenteditable with its add buttons live. Nothing re-runs this gate afterwards: the hover outline (mouseHover.ts:162) and the field label (fieldLabelWrapper.tsx:290) subscribe to the mirror, this path does not. The field then shows the disabled affordance while still accepting typing.
I can't tell from the SDK side how often that wins the race. Hover normally warms the snapshot before a click, so the ordinary desktop path is probably covered. It is not covered for a click with no prior hover (touch, keyboard focus, a fast click), or for a restriction that arrives while a field is already focused, which is the version-switch case this change exists for.
Subscribing to subscribeEntryFieldLockInfo and re-running the gate, removing contenteditable when a restriction appears, would close both.
Generated by Claude Code
| // An older parent omits the field; it cannot have restrictions, so skip the seed. | ||
| if (response.editRestrictions) { | ||
| seedEntryEditRestrictions( | ||
| scope.entryUid, | ||
| response.editRestrictions, | ||
| restrictionSeqBeforeRequest | ||
| ); | ||
| } |
There was a problem hiding this comment.
Should fix. The seed sits after the early return on line 38, which bails when response.fieldLockInfo == null. A reply that carries editRestrictions but no lock map drops the restrictions entirely and returns null, which makes requestEntryLockInfoOnce release the scope so the next hover repeats the same round trip and the same loss. While the parent's lock-status route is failing, the entry's fields stay editable even though the parent knows the entry is restricted.
The two are independent facts about the entry, so tying the restriction seed to lock-fetch success looks unintended. If the parent can ever answer with restrictions but no lock info, move the seed above that guard.
Generated by Claude Code
| for (const key of restrictions.keys()) { | ||
| if (key.startsWith(`${entryUid}.`)) next.set(key, null); | ||
| } |
There was a problem hiding this comment.
Should fix. This clears every key under the entry, which spans all of its locales and variants, but the request that produced the snapshot was for one scope (getEntryLockInfo is called with a single {entryUid, locale, variantUid}). If the parent's editRestrictions only covers the requested locale, the restrictions recorded for the entry's other locales are cleared here and those fields silently become editable.
The JSDoc reads as though the snapshot is always the whole entry across locales, and the uid:locale[:variant] key format implies the same, so the editor side probably does return all of them. Nothing fails loudly if it stops doing so. Worth stating that requirement in the comment as a contract the parent has to meet, and confirming it against the editor-side handler.
Generated by Claude Code
| } | ||
| ); | ||
| }); | ||
| clearAllEntryFieldLockInfo(); |
There was a problem hiding this comment.
Nit. clearAllEntryFieldLockInfo() runs inside the test body, so if the act block above throws, the olderVersion restriction set at the top leaks into cleanIndividualFieldResidual below and into anything added after it. Moving it to an afterEach keeps the cleanup unconditional.
Generated by Claude Code
…diting (DRFT-913) On a fresh canvas the first click could make a restricted field editable before the entry's restriction arrived; inline editing now waits (bounded) for that scope's in-flight snapshot.
Automated self-review (commit 7c5d60c)Mostly solid: the stale-snapshot guard, the parsing of unknown values and the toolbar subscription are all well done. The gaps are in what happens when a restriction lands after a field is already focused, plus two fail-open edge cases.
|
hitesh-shetty-cstk
left a comment
hitesh-shetty-cstk
left a comment
There was a problem hiding this comment.
Re-review after new commits (pass 2 of 4).
Automated review
What this changes: The one new commit makes the inline-edit gate wait for an entry's first lock snapshot when one is still in flight. requestEntryLockInfoOnce now records the pending request in a map keyed by entry, locale and variant, a new waitForEntryLockInfo races that request against a 1500ms timer, and handleIndividualFields awaits it before computing disabled. Two unit tests cover the waiting path and the case where nothing is pending.
Business impact: Inline editing on the canvas, which is the journey this commit is trying to protect. The shape of the fix is right: the wait sits after the permission fetch, so in the ordinary path the snapshot has already landed and the wait costs nothing. The blocker below is about what happens when it has not landed, where the timeout drops back to the pre-commit behaviour without saying so.
Security: nothing beyond what the scanners cover. The commit moves local module state and adds a timer. No payload validation, DOM write or credential path changes in it.
Flow
flowchart TD
A[handleIndividualFields] --> B[fetchEntryPermissionsAndStageDetails]
B --> C[requestEntryLockInfoOnce, fire and forget]
B --> D[waitForEntryLockInfo]:::changed
D --> E{snapshot in flight?}:::changed
E -->|no| H[compute disabled]
E -->|yes| F[race snapshot against 1500ms timer]:::changed
F -->|snapshot lands| H
F -->|timer wins| H
H --> I{disabled?}
I -->|yes| J[stop, no inline edit]
I -->|no| K[enableInlineEditing sets contenteditable]
classDef changed fill:#fff3cd,stroke:#d39e00
Findings: 1 blocker, 1 should fix, 2 nit. All inline.
The write path in requestEntryLockInfoOnce holds up. The in-flight map is set synchronously before the first await, so a caller reaching waitForEntryLockInfo in the same turn always sees the pending request, and the finally clears it on both outcomes. getEntryLockInfo catches and returns null rather than throwing, so the stored promise cannot reject and the new await in handleIndividualFields cannot abort field setup. The key helper is shared by both functions, so the request and the wait cannot drift apart. The two new tests are genuine additions: no assertion was weakened, nothing was skipped, and the waiting test checks the restriction is readable after the snapshot rather than checking internals.
One finding from the previous pass is outside this commit and still open: FieldToolbar.tsx computes disableFieldActions from isFieldDisabled alone, so the toolbar on a restricted entry keeps Delete instance, Move instance and Replace enabled. Noting it as status, not re-raising it here.
Reviewer candidates: naming only, since reviewers are requested on the opening pass and this is a re-review.
- @faraazb-contentstack authored 5 of the last 30 commits on
src/visualBuilder/utils/handleIndividualFields.ts, the file holding the gate both substantive findings concern. - @srinad007 authored 4 of the last 30 on the same file.
fieldLockIndicator.tsis new in this branch, so its history names only the author.
Since inline editing is the critical journey here and @faraazb-contentstack owns that file, please walk them through the timeout behaviour before merging.
Not covered: No build, no test run, no app run. Whether a restriction can arrive while a field is already focused depends on whether the parent re-renders the canvas on a version switch, which is in another repository and was not read, so the second finding is stated as a question rather than a defect. The 1500ms budget was not measured against real reply times.
Automated review by Claude Code. A human review is still required.
Generated by Claude Code
| await Promise.race([ | ||
| request, | ||
| new Promise<void>((resolve) => { | ||
| timer = setTimeout(resolve, timeoutMs); | ||
| }), | ||
| ]); | ||
| clearTimeout(timer); |
There was a problem hiding this comment.
Blocker. Promise.race resolves the same way whether the snapshot landed or the 1500ms timer fired, and the function returns void, so the caller cannot tell the two apart. When the timer wins, handleIndividualFields carries on, getEntryEditRestrictionForField reads a mirror that still has no restriction for the scope, disabled comes out false, and enableInlineEditing sets contenteditable="true" on a field of a restricted entry. Nothing re-runs the gate afterwards, so it stays editable for that focus.
The window is narrow. The request has already had the three round trips inside fetchEntryPermissionsAndStageDetails to settle, so the timer only wins when the lock reply lags those by more than 1500ms. What makes it worth fixing is that it fails silently and lands on exactly the behaviour this commit removes.
Returning whether the snapshot actually arrived would at least make the outcome visible to the caller. I am not proposing a specific policy for the timeout case: disabling the field on a slow reply would block editing on an entry that is not restricted, so the honest options are to re-run the gate when the snapshot does arrive, or to decide deliberately which way a timeout should fall and say so in the comment.
Generated by Claude Code
| await waitForEntryLockInfo({ | ||
| entryUid: entry_uid, | ||
| locale, | ||
| ...(variant ? { variantUid: variant } : {}), | ||
| }); |
There was a problem hiding this comment.
Should fix. This closes the case where the first snapshot is still in flight. The other half of the race is still open: waitForEntryLockInfo returns immediately once inFlightRequests holds nothing for the scope, so a restriction that arrives later as an update delta never re-runs this gate. A field that was already focused and made contenteditable keeps accepting typing, and the only subscribers to the mirror are the hover outline in mouseHover.ts and the label in fieldLabelWrapper.tsx, neither of which touches inline-edit state.
I cannot tell from the SDK side whether that is reachable. If switching to another version re-renders the canvas and re-runs handleIndividualFields, it is covered and this is nothing. If the parent pushes the restriction onto a canvas that stays mounted with a field focused, it is not. That is the scenario the description leads with, so it seems worth confirming against the editor side before this merges.
Generated by Claude Code
| it("does not wait when nothing is in flight", async () => { | ||
| await expect( | ||
| waitForEntryLockInfo({ entryUid: "idle", locale: "en-us" }, 5000) | ||
| ).resolves.toBeUndefined(); |
There was a problem hiding this comment.
Nit. Both new tests pass a 5000ms budget, so neither exercises the timer winning the race. That is the branch deciding whether a restricted field becomes editable, and it has no coverage. A test with a small timeoutMs and a request that never resolves would pin the behaviour down, whichever way the timeout question above is settled.
Generated by Claude Code
| }) as never | ||
| ); | ||
| const wideScope = { ...request, entryUid: "entry-wait" }; | ||
| void requestEntryLockInfoOnce(wideScope); |
There was a problem hiding this comment.
Nit. requestedScopes and inFlightRequests are module state in fieldLockIndicator, and the clearAllEntryFieldLockInfo() in beforeEach does not reset them, so entry-wait stays in requestedScopes for the rest of the file. Nothing collides today. A later test reusing that uid would skip the request, leaving this wait a no-op while still reporting a pass. An exported reset called from beforeEach would drop the ordering dependency.
Generated by Claude Code
…tricted entries (DRFT-913) The toolbar's delete, move, edit and app buttons and the focus overlay redraws only read isFieldDisabled, so a restricted entry's field showed disabled while its actions stayed live.
hitesh-shetty-cstk
left a comment
hitesh-shetty-cstk
left a comment
There was a problem hiding this comment.
Re-review after new commits (pass 3 of 4).
Automated review
What this changes: The one new commit extends the entry-wide edit restriction from the field label and the inline-edit gate to two places that still ignored it. FieldToolbar now reads the restriction into state and subscribes to the lock store, so Delete instance, Move instance, Replace, Edit, the variant revert dropdown, the comment icon and the field-location icon all disable while an entry is restricted. updateFocussedState and the resize observer in index.ts fold the restriction into the isDisabled they pass to addFocusOverlay. The form button stays enabled, which keeps the route into the form open.
This closes the blocker raised on the opening pass, where the toolbar's structure actions were never gated and a user viewing an older version could delete a block from the canvas. The gating now covers every action in that toolbar.
Business impact: Inline editing on the canvas, specifically the field toolbar. The gating itself is correct. The blocker below is about what the new subscription leaves behind: the toolbar component is never unmounted, so each toolbar created on focus adds a listener to the lock store that is never removed, and nothing in the existing teardown removes it.
Security: nothing beyond what the scanners cover. The commit adds no payload parsing, no DOM write and no credential path. The restriction value is still narrowed against the fixed allowlist in the store before it reaches any of this code.
Flow
flowchart TD
P[Parent restriction update] --> S[fieldLockStore restrictions]
S --> N[notifyLockListeners]
N --> T[FieldToolbar subscription]:::changed
T --> D[disableFieldActions]:::changed
D --> B[Delete, Move, Replace, Edit disabled]:::changed
S --> U[updateFocussedState one-shot read]:::changed
S --> R[ResizeObserver one-shot read]:::changed
U --> O[addFocusOverlay outline colour]
R --> O
T -.-> L[listener left behind on toolbar teardown]:::changed
classDef changed fill:#fff3cd,stroke:#d39e00
Findings: 1 blocker, 2 should fix, 1 nit. All inline.
On the test changes, the two new FieldToolbar cases are genuine. Nothing was weakened or skipped, they assert the user-observable disabled state rather than internals, and the second one covers the case the subscription exists for, a restriction arriving after the toolbar is already open. The new focus-overlay test is also a real assertion rather than a bent one: isFieldDisabled is mocked to return false and updateFocussedStateOnMutation never calls addFocusOverlay, so the true it checks for can only come from the restriction. Its problem is placement, which is the second finding.
One finding from the previous pass is outside this commit and still open: when the lock snapshot is still in flight, waitForEntryLockInfo gives up after 1500ms and the inline-edit gate falls back to treating the entry as unrestricted without saying so. Noting it as status, not re-raising it.
Reviewer candidates: naming only, since reviewers are requested on the opening pass and this is a re-review. @faraazb-contentstack and @karancs06 are already on the pull request.
- @srinad007 authored 5 of the last 30 commits on
src/visualBuilder/components/FieldToolbar.tsx, where the blocker sits. - @ZuhairAhmed-cs authored 7 of the last 30 on
src/visualBuilder/index.ts.
The blocker is really about generateToolbar.tsx, where the toolbar is rendered and torn down. @devAyushDubey authored 4 of the last 30 commits there. Since the canvas toolbar is the critical journey here, please walk them through the listener lifecycle before merging.
Not covered: No build, no test run and no app run. Dependencies are not installed in this environment, so the claim that the new focus-overlay test fails in isolation is read from the mock setup rather than observed. The listener growth is reasoned from the teardown path, not measured in a session. The parent side of this contract is in another repository and was not read, so how often a restriction lands after a field is already focused remains an assumption.
Automated review by Claude Code. A human review is still required.
Generated by Claude Code
| const update = () => | ||
| setRestrictionReason(getEntryEditRestrictionForField(fieldMetadata)); | ||
| update(); | ||
| return subscribeEntryFieldLockInfo(update); |
There was a problem hiding this comment.
Blocker. This subscription is never torn down, so it outlives the toolbar that created it.
appendFieldToolbar renders this component with render(<FieldToolbarComponent .../>, wrapper), and removeFieldToolbar tears it down with toolbar.innerHTML = "". That removes the DOM but never unmounts the preact tree, so effect cleanups on this component do not run. The DELETE_INSTANCE effect lower in this file shows the consequence already being worked around: its return () => event?.unregister() is dead, which is why removeFieldToolbar re-unregisters those three events by hand through @ts-expect-error calls into a private handler map.
The listener registered here has no such counterpart, so every toolbar creation adds one more entry to lockListeners that is never removed. A toolbar is built on each focus, and the hover path in appendFieldToolbar skips the "already present" guard, so the set grows across an editing session. Each dead listener retains this component's fieldMetadata and props, including the editable element, so detached DOM is retained with it, and every later restriction or lock update calls setRestrictionReason on all of them. notifyLockListeners catches and logs listener errors at debug level, so none of this surfaces.
Fix it the way the postMessage listeners are handled: keep the unsubscribe where removeFieldToolbar can reach it and call it during teardown, or move the subscription out of the component so its lifetime is not tied to an unmount that never happens. Unmounting properly in removeFieldToolbar with render(null, container) would fix this and the dead DELETE_INSTANCE cleanup together, but it needs the container reference that appendFieldToolbar currently drops.
Generated by Claude Code
| expect(focusOutlineMock.style.height).toBe("100px"); | ||
| }); | ||
|
|
||
| it("redraws the focus overlay as disabled while the entry is restricted", async () => { |
There was a problem hiding this comment.
Should fix. This test exercises updateFocussedState, but it is defined inside the updateFocussedStateOnMutation describe, and it depends on setup that only the other describe performs.
fetchEntryPermissionsAndStageDetails is mocked at the top of this file as a bare vi.fn() with no implementation. Only the updateFocussedState describe's beforeEach gives it a mockResolvedValue. vi.clearAllMocks() clears call history but keeps implementations, so by the time this test runs the resolved value left over from the earlier describe is still attached and the test passes. Run this test on its own, for example -t "redraws the focus overlay", and the mock resolves undefined, so the destructure in updateFocussedState throws before addFocusOverlay is reached.
Move it into the updateFocussedState describe, which already sets the permissions mock and clears mocks around each test.
Separately, the clearAllEntryFieldLockInfo() call sits inline between the act block and the assertion. If the act block throws, the restriction stays in the module-level store and leaks into every later test in this file. The FieldToolbar suite puts the same call in an afterEach; do that here too.
Generated by Claude Code
| resolvedVariantPermissions, | ||
| entryAcl, | ||
| entryWorkflowStageDetails | ||
| ).isDisabled || Boolean(getEntryEditRestrictionForField(fieldMetadata)); |
There was a problem hiding this comment.
Should fix. This read is a one-shot snapshot, while the toolbar subscribes to the same store.
FieldToolbar re-renders when the restriction changes because it registers a lock-store listener. This call site and the matching one in index.ts read getEntryEditRestrictionForField once, at focus time, and nothing redraws the overlay when the value changes afterwards. The previous pass established that an entry's first lock snapshot can still be in flight for up to 1500ms, so a field focused in that window gets its outline drawn in the editable colour while the toolbar beside it greys out once the restriction lands. The same gap appears when a restriction is lifted, and index.ts only calls addFocusOverlay when isDisabled is true, so a grey outline there is never repainted.
The effect is confined to the outline colour in generateOverlay.tsx, so this is an inconsistent affordance rather than a way past the gate. Worth closing anyway, since making the restriction visible on the canvas is what this commit is for. Subscribing here the way the toolbar does, or repainting the focused element when the store notifies, would cover it.
Generated by Claude Code
| return subscribeEntryFieldLockInfo(update); | ||
| }, [fieldMetadata]); | ||
|
|
||
| let disableFieldActions = Boolean(restrictionReason); |
There was a problem hiding this comment.
Nit. restrictionReason holds a user-facing sentence, such as the one explaining that an older version is being viewed, but only its truthiness is ever used. The toolbar buttons go grey with no indication why.
The other consumers of this value render it, and the buttons here already carry a data-tooltip. Passing the reason through as the tooltip when the restriction is what disabled the button would explain the state at the point the user is trying to act on it, rather than only on the field label.
Generated by Claude Code
2 participants
Shows fields of an entry the Visual Editor has restricted as disabled on the canvas, with a reason-specific message.
The editor sends each entry scope's restriction (
olderVersion,unlocalized,unsavedVariant) through a newentry-edit-restriction-updateevent and in theget-entry-lock-inforeply. The SDK keeps them next to the field-lock mirror and uses them in the hover outline, the cursor, the field label and inline editing. For example, an older version shows "You're viewing an older version of this entry. Switch to the latest version to edit."A click on a restricted field still reaches the editor, so the form can open that entry (the way back to the latest version, or to localize it). Inline editing and the add buttons stay off. Peer-locked fields keep blocking the click as before.
Other details:
isFieldDisabled, which avoids an import cycle through the VisualBuilder index.Ticket: DRFT-913. Editor side: https://github.com/contentstack/visual-builder/pull/2784
Testing: new unit tests for the store, the update event, snapshot seeding, the field helpers, the label message and inline editing on a restricted entry. Full suite: 933 passed. Checked against dev11 with a local build: Version 50 shows the field disabled with the message and no inline edit; Version 53 edits normally.