Repository navigation
fix(visual-builder): deselect the field when the parent refuses its field lock - #666
Conversation
…ield lock
Replace on a file or reference field and the add-instance buttons opened a
picker in the parent even when a collaborator held the field, leaving the
field selected with its toolbar up. The parent now answers OPEN_ASSET_MODAL,
OPEN_REFERENCE_MODAL and ADD_INSTANCE with { fieldLockRefused: true } when it
cannot take the lock. On that reply the toolbar deselects the field, but only
if it is still the selected one (matched by data-cslp), so a field the user
picked during the round trip keeps its selection. The empty-block add skips
focusing a new instance, since none is coming.
OPEN_REFERENCE_MODAL now carries the field's variant, which the parent needs
to derive the lock path.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
✅ 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
Critical journey at risk: inline editing on the canvas. This changes what clears field focus, the edit overlay and the field toolbar, plus the add-instance buttons on modular and multiple fields. If the new deselect fires when it should not, an editor loses their selection and toolbar in the middle of editing a field they do own. If it fails to fire, the toolbar stays on a field a collaborator has locked and every action on it silently does nothing.
What this changes: Replace on file and reference fields, and the add-instance buttons, now await the parent's reply instead of sending and forgetting. When the parent answers { fieldLockRefused: true } because a collaborator holds the field lock, the SDK deselects the field and drops the toolbar rather than leaving it on a field the user cannot edit. A new isFieldStillSelected check keeps a field the user selected during the round trip from being deselected. OPEN_REFERENCE_MODAL also started sending the field's variant.
Business impact: as above. The window this covers is narrow, the gap before the collaborator's lock reaches the canvas, but the code path it touches runs on every Replace click and every add-instance click.
Security: nothing beyond what the scanners cover. The refusal check reads a reply to the SDK's own request and narrows it with a typed === true test, so a malformed or absent reply reads as "not refused" rather than throwing. The authorization decision itself stays on the parent and the SDK only reacts to it, which is the right split. Snyk passed on this head commit across open source, licence and code security.
Flow
sequenceDiagram
participant User
participant Button as Replace / add-instance button (SDK)
participant State as VisualBuilderGlobalState
participant Parent as Visual Builder parent window
User->>Button: click
Button->>Parent: OPEN_ASSET_MODAL / OPEN_REFERENCE_MODAL / ADD_INSTANCE
Note over Parent: field lock checked before the picker opens
Parent-->>Button: { fieldLockRefused: true }
Note over Button: changed here, the reply is now awaited
Button->>State: is data-cslp still the clicked field?
State-->>Button: yes
Button->>Button: hideOverlay, drop the selection and the toolbar
Findings: 0 blocker, 3 should fix, 2 nit. All five are inline. Three of them are test isolation, one is a type contract, one is a question about the empty-block path.
What I checked and found sound, since this sits on a critical path:
hideOverlaynullspreviousSelectedEditableDOM, so the guard and the thing it guards agree on what "selected" means.onFieldLockRefusedhas one non-test caller,multipleElementAddButton, and both the previous and next buttons get it, so the optional prop is fully wired today.- In production
previousSelectedEditableDOMis the element the toolbar'seventDetailswas built from, so comparing itsdata-cslpagainstfieldMetadata.cslpValuematches, rather than only matching because the tests set both from one value. fieldLockRefused.tsimportingVisualBuilderfrom".."closes an import cycle, but it dereferences only inside the function bodies, and four other files underutils/already do the same.- Backward compatibility holds for the reason the description gives. The current parent handler returns
undefinedon its early return, which resolves toundefined, soisFieldLockRefusedis false and nothing is deselected. - Awaiting these replies converts a previously unhandled promise rejection into a logged one.
Reviewer candidates:
@SahilCs15authored 10 of the last 30 commits onsrc/visualBuilder/components/FieldToolbar.tsxand 14 across the five most-changed files, the most of anyone. They are also the top author of the critical-path file here, so please walk them through the deselect conditions before merging.@srinad007authored 5 of the last 30 commits onFieldToolbar.tsx, second on that file.
Not covered: No tests, type check or build were run, and no app was started. This checkout has no installed dependencies, so the suite result, the clean tsc --noEmit and the pre-existing DTS error in the description are unverified here. The matching parent change is not in the checkout I read, so the refusal contract and the new variant consumer are verified from the SDK side only. I could not check what the post-message layer does when the parent registers no responder at all, as opposed to a responder that returns nothing. E2E behaviour with two live users is unverified.
One question for the summary rather than a line: the parent refuses Replace and add-instance. If it also refuses the toolbar's Edit action, OPEN_FIELD_EDIT_MODAL still sends and forgets, so that path would keep the old behaviour of leaving the toolbar up. Worth confirming the parent's refusal set matches what this PR handles.
Automated review by Claude Code. A human review is still required.
Generated by Claude Code
| vi.mocked(FieldSchemaMap.getFieldSchema).mockImplementation(() => | ||
| Promise.resolve(schema) | ||
| ); | ||
| vi.mocked(visualBuilderPostMessage!.send).mockImplementation( |
There was a problem hiding this comment.
[should fix] This mockImplementation outlives the describe block. The outer afterEach calls vi.clearAllMocks(), which clears recorded calls but leaves the implementation in place, and nothing else restores send. From here on every test in the file gets {} for every event, which replaces the two cases the file's vi.mock factory deliberately special-cases, field-location-data and get-field-variant-status.
The six tests after this block ('Replace button' visibility, the disabled-state test and the restricted-entry tests) therefore run against a send they were not written for. They pass today only because none of them asserts on that data, so this couples them to the order this block runs in. The comment at line 636 in this file, "Restore will happen in outer afterEach via clearAllMocks", relies on the same incorrect assumption.
Restoring in this block's existing afterEach is enough. On the pinned vitest@^4.1.5, mockReset() puts back the implementation that was passed to vi.fn(), which is the factory behaviour:
afterEach(() => {
VisualBuilder.VisualBuilderGlobalState.value.previousSelectedEditableDOM =
null;
vi.mocked(visualBuilderPostMessage!.send).mockReset();
});Generated by Claude Code
There was a problem hiding this comment.
Fixed in bc2750d. This describe's afterEach now calls mockReset() on send, which puts back the factory implementation. I also corrected the misleading comment on the visibility describe's afterEach: getFieldSchema is reset by the outer beforeEach, not by clearAllMocks.
| }); | ||
| await Promise.resolve(); | ||
| expect(observeParentAndFocusNewInstance).not.toHaveBeenCalled(); | ||
| (visualBuilderPostMessage as any).send.mockReset(); |
There was a problem hiding this comment.
[should fix] The cleanup is the last statement of the test body, so it is skipped whenever the assertion above it fails. mockResolvedValue is persistent and the beforeEach here only calls vi.clearAllMocks(), which does not clear a queued resolved value, so a failure in this test leaves { fieldLockRefused: true } answering send for the three tests that follow it. A single real regression then reports as four failures and the later ones point away from the cause.
Moving it into an afterEach makes it run on both paths:
afterEach(() => {
(visualBuilderPostMessage as any).send.mockReset();
host?.remove();
host = null;
});The other three refusal tests in this PR use mockResolvedValueOnce, which avoids the problem. That works here too, since this test renders with no data-cslp ancestor so the edit stack is empty and ADD_INSTANCE is the only message sent. mockResolvedValue plus an afterEach is the less fragile of the two, because it does not depend on that.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in bc2750d. The send reset now runs in afterEach, so it also happens when an assertion fails.
| } | ||
| ); | ||
| // A collaborator holds the field: no instance is coming to focus. | ||
| if (isFieldLockRefused(response)) return; |
There was a problem hiding this comment.
[should fix, question] I am not sure this is a defect, so treat it as a question rather than a finding.
Unlike the other two paths, this one claims the lock before it asks. Lines 45 to 50 send FOCUS_FIELD fire and forget, then ADD_INSTANCE goes out immediately after. On a refusal this returns without telling the parent anything, so if the FOCUS_FIELD claim can land while the refusal is in flight, the parent is left holding this user as the focused editor on a field they did not get, and nothing in the SDK will clear it because the empty-block path never selected the field locally.
If a refusal always implies the claim was rejected too, because the collaborator already held the lock, then there is nothing to release and this is correct as written. Can you confirm that is the parent's behaviour? If the claim can succeed independently, this needs a deselect on the refusal, and the comment on line 44 suggests sending an empty edit stack is how the parent reads one.
Either way, a one-line comment here saying why no release is needed would save the next reader the same trip through the parent.
Generated by Claude Code
There was a problem hiding this comment.
Confirmed, nothing to release. On the parent, FOCUS_FIELD and the ADD_INSTANCE lock check both act on the same focus claim, for the same lock path. A refusal is the answer to that claim: on a conflict the controller drops the field, and on a timeout it releases it explicitly. So the parent holds nothing for this field when the reply arrives. The SDK side never selected the field, so there's nothing to deselect either. I added a comment saying this in bc2750d.
|
|
||
| test("sends the variant with the reference replace request", async () => { | ||
| selectElementWithCslp(instanceMetadata.cslpValue); | ||
| instanceMetadata.variant = "variant_1"; |
There was a problem hiding this comment.
[nit] instanceMetadata is shared with the test.each cases above, and this mutation is undone on the last line of the test. If the assertion fails, the reset is skipped and variant stays "variant_1" for anything that runs after it.
A local copy keeps the mutation out of the shared object and drops the need to reset at all:
await clickReplace("reference", referenceFieldSchema, undefined, {
...instanceMetadata,
variant: "variant_1",
});Passing the metadata through clickReplace means a small signature change. Setting the value in a beforeEach and clearing it in an afterEach would also work if you would rather not touch the helper.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in bc2750d. clickReplace now takes the metadata as an optional argument, and the variant test passes its own copy, so the shared object isn't touched.
| value: any; | ||
| onClick: (event: MouseEvent) => void; | ||
| /** Called instead of `onClick` when a collaborator holds the field lock. */ | ||
| onFieldLockRefused?: () => void; |
There was a problem hiding this comment.
[nit] Worth considering making this required. There is one non-test caller, handleAddButtonsForMultiple, and it passes the callback for both buttons, so nothing is relying on the prop being optional.
As it stands, a future caller that omits it gets a button that does nothing at all on a refusal: props.onFieldLockRefused?.() is a no-op and the return below skips props.onClick(event) as well. That is the same class of dead control as the bug this PR fixes, and it would fail silently rather than at the type level. Making it required here and in the generateAddInstanceButton parameter type turns it into a compile error instead.
Generated by Claude Code
There was a problem hiding this comment.
Done in bc2750d. onFieldLockRefused is now required on AddInstanceButton and on generateAddInstanceButton. The test call sites pass it too.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
…eldLockRefused Reset the `send` mock after the toolbar refusal tests and after each empty block test, so a test's stubbed reply cannot answer `send` in later tests. Pass the variant test its own metadata copy instead of mutating the shared one. Make onFieldLockRefused required: a button without it would do nothing on a refusal. Note why the empty-block add has no claim to release when its request is refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
2 participants
Summary
When a toolbar action on the canvas asks the parent to open a picker (Replace on a file or reference field, or the add-instance buttons), the parent now checks the field lock first. If a collaborator holds the field, it does not open the picker and replies
{ fieldLockRefused: true }. This PR handles that reply in the SDK, so the field is deselected and its toolbar removed instead of staying up on a field the user can't edit.This only matters in the window where the canvas doesn't know about the other user's lock yet. Once the lock reaches the SDK, the existing locked-field handling already blocks the click and disables the toolbar actions.
Changes
FieldToolbar: Replace on file and reference fields now awaits the parent's reply. OnfieldLockRefusedit callshideOverlay, but only if the selected element'sdata-cslpstill matches the field that asked. A field the user selected during the round trip keeps its selection.AddInstanceButtonandmultipleElementAddButton: on a refused reply, the button calls a newonFieldLockRefusedcallback instead ofonClick. That deselects the field (with the samedata-cslpcheck) and skips waiting for a new instance.EmptyBlock: on a refused reply it skipsobserveParentAndFocusNewInstance, since no instance is coming.OPEN_REFERENCE_MODALnow sends the field'svariant. The parent derives the lock path from the variant entry.utils/fieldLockRefused.tswithisFieldLockRefusedandisFieldStillSelected.A parent that doesn't send the new reply keeps the old behaviour: the reply isn't
fieldLockRefused, so nothing is deselected.Test plan
fieldToolbar.test.tsx,addInstanceButton.test.tsx,emptyBlock.test.tsxandmultipleElementAddButton.test.ts, plusfieldLockRefused.test.ts.npx vitest run: 116 files, 962 tests pass.tsc --noEmitis clean.npm run buildshows a DTS error onemptyBlock.tsx(JSX.TargetedMouseEvent) that is already on the base branch; the JS bundle builds.🤖 Generated with Claude Code