Skip to content

fix(visual-builder): deselect the field when the parent refuses its field lock #666

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
faraazb-contentstack merged 2 commits into from
Oct 9, 2026
Merged

fix(visual-builder): deselect the field when the parent refuses its field lock #666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74d842a
fix(visual-builder): deselect the field when the parent refuses its f…
faraazb-contentstack Oct 8, 2026
bc2750d
test(visual-builder): isolate the lock-refusal tests and require onFi…
faraazb-contentstack Oct 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 65 additions & 19 deletions src/visualBuilder/components/FieldToolbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ import { getEntryEditRestrictionForField } from "../utils/fieldLockIndicator";
import { subscribeEntryFieldLockInfo } from "../utils/fieldLockStore";
import { ResolvedVariantPermissions } from "../utils/getResolvedVariantPermissions";
import { isCustomFieldMultipleInstance as checkIsCustomFieldMultipleInstance } from "../utils/isCustomFieldMultipleInstance";
import {
isFieldLockRefused,
isFieldStillSelected,
} from "../utils/fieldLockRefused";

export type FieldDetails = Pick<
VisualBuilderCslpEventDetails,
Expand All @@ -64,17 +68,50 @@ interface MultipleFieldToolbarProps {
resolvedVariantPermissions?: ResolvedVariantPermissions | undefined;
}

function handleReplaceAsset(fieldMetadata: CslpData) {
// TODO avoid sending whole fieldMetadata
visualBuilderPostMessage?.send(
VisualBuilderPostMessageEvents.OPEN_ASSET_MODAL,
{
fieldMetadata,
/**
* Sends a replace request and deselects the field if the parent refuses it for
* a collaborator's lock, so the toolbar does not linger on a field we cannot
* edit.
*/
async function sendReplaceRequest(
fieldMetadata: CslpData,
request: Promise<unknown> | undefined,
hideOverlay: () => void
): Promise<void> {
try {
const response = await request;
if (
isFieldLockRefused(response) &&
isFieldStillSelected(fieldMetadata.cslpValue)
) {
hideOverlay();
}
} catch (error) {
console.error(
"Visual Builder: Failed to open the replace modal",
error
);
}
}

function handleReplaceAsset(fieldMetadata: CslpData, hideOverlay: () => void) {
// TODO avoid sending whole fieldMetadata
return sendReplaceRequest(
fieldMetadata,
visualBuilderPostMessage?.send(
VisualBuilderPostMessageEvents.OPEN_ASSET_MODAL,
{
fieldMetadata,
}
),
hideOverlay
);
}

function handleReplaceReference(fieldMetadata: CslpData) {
function handleReplaceReference(
fieldMetadata: CslpData,
hideOverlay: () => void
) {
const isMultipleInstance =
fieldMetadata.multipleFieldMetadata.index > -1 &&
fieldMetadata.fieldPathWithIndex ===
Expand All @@ -83,16 +120,22 @@ function handleReplaceReference(fieldMetadata: CslpData) {
? fieldMetadata.instance.fieldPathWithIndex
: fieldMetadata.fieldPathWithIndex;

visualBuilderPostMessage?.send(
VisualBuilderPostMessageEvents.OPEN_REFERENCE_MODAL,
{
entry_uid: fieldMetadata.entry_uid,
content_type_uid: fieldMetadata.content_type_uid,
locale: fieldMetadata.locale,
fieldPath: fieldMetadata.fieldPath,
fieldPathWithIndex: fieldMetadata.fieldPathWithIndex,
entryPath,
}
return sendReplaceRequest(
fieldMetadata,
visualBuilderPostMessage?.send(
VisualBuilderPostMessageEvents.OPEN_REFERENCE_MODAL,
{
entry_uid: fieldMetadata.entry_uid,
content_type_uid: fieldMetadata.content_type_uid,
locale: fieldMetadata.locale,
// The parent derives the field lock path from the variant entry.
variant: fieldMetadata.variant,
fieldPath: fieldMetadata.fieldPath,
fieldPathWithIndex: fieldMetadata.fieldPathWithIndex,
entryPath,
}
),
hideOverlay
);
}

Expand Down Expand Up @@ -298,10 +341,13 @@ function FieldToolbarComponent(
e.stopPropagation();
e.preventDefault();
if (fieldType === FieldDataType.REFERENCE) {
handleReplaceReference(fieldMetadata);
void handleReplaceReference(
fieldMetadata,
props.hideOverlay
);
return;
} else if (fieldType === FieldDataType.FILE) {
handleReplaceAsset(fieldMetadata);
void handleReplaceAsset(fieldMetadata, props.hideOverlay);
return;
}
}}
Expand Down
33 changes: 33 additions & 0 deletions src/visualBuilder/components/__test__/addInstanceButton.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ describe("AddInstanceButtonComponent", () => {
fieldMetadata={{}}
index={0}
onClick={onClickCallback}
onFieldLockRefused={vi.fn()}
label="Add instance"
// @ts-expect-error mocking signal
loading={{ value: false }}
Expand Down Expand Up @@ -66,6 +67,7 @@ describe("AddInstanceButtonComponent", () => {
fieldMetadata={{}}
index={0}
onClick={onClickCallback}
onFieldLockRefused={vi.fn()}
label="Add instance"
// @ts-expect-error mocking signal
loading={{ value: false }}
Expand Down Expand Up @@ -99,6 +101,7 @@ describe("AddInstanceButtonComponent", () => {
fieldMetadata={{}}
index={0}
onClick={onClickCallback}
onFieldLockRefused={vi.fn()}
label="Add instance"
// @ts-expect-error mocking signal
loading={{ value: false }}
Expand All @@ -114,4 +117,34 @@ describe("AddInstanceButtonComponent", () => {
});
expect(onClickCallback).toHaveBeenCalled();
});
test("calls onFieldLockRefused instead of onClick when the lock is refused", async () => {
visualBuilderPostMessage!.send.mockResolvedValueOnce({
fieldLockRefused: true,
});
const onClickCallback = vi.fn();
const onFieldLockRefused = vi.fn();
await act(() => {
render(
<AddInstanceButtonComponent
value={[]}
fieldSchema={singleLineFieldSchema}
// @ts-expect-error mocking fieldMetadata
fieldMetadata={{}}
index={0}
onClick={onClickCallback}
onFieldLockRefused={onFieldLockRefused}
label="Add instance"
// @ts-expect-error mocking signal
loading={{ value: false }}
/>
);
});
await act(() => {
fireEvent.click(
getByTestId(document.body, "visual-builder-add-instance-button")
);
});
expect(onFieldLockRefused).toHaveBeenCalled();
expect(onClickCallback).not.toHaveBeenCalled();
});
});
20 changes: 20 additions & 0 deletions src/visualBuilder/components/__test__/emptyBlock.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ describe("EmptyBlock", () => {
});

afterEach(() => {
// A test's resolved value would otherwise answer `send` in the next one.
(visualBuilderPostMessage as any).send.mockReset();
// RTL only removes containers it created, so this one would outlive the test
host?.remove();
host = null;
Expand Down Expand Up @@ -85,6 +87,24 @@ describe("EmptyBlock", () => {
});
});

test("does not wait for a new instance when the lock is refused", async () => {
(visualBuilderPostMessage as any).send.mockResolvedValue({
fieldLockRefused: true,
});
const { getByTestId } = render(<EmptyBlock details={mockDetails} />);

fireEvent.click(getByTestId("visual-builder__empty-block-add-button"));

await waitFor(() => {
expect((visualBuilderPostMessage as any).send).toHaveBeenCalledWith(
VisualBuilderPostMessageEvents.ADD_INSTANCE,
expect.anything()
);
});
await Promise.resolve();
expect(observeParentAndFocusNewInstance).not.toHaveBeenCalled();
});

test("claims the field lock before adding, so a peer sees it", async () => {
host = document.createElement("div");
host.setAttribute("data-cslp", mockDetails.fieldMetadata.cslpValue);
Expand Down
147 changes: 146 additions & 1 deletion src/visualBuilder/components/__test__/fieldToolbar.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
clearAllEntryFieldLockInfo,
setEntryEditRestriction,
} from "../../utils/fieldLockStore";
import { VisualBuilder } from "../..";

vi.mock("../../utils/instanceHandlers", () => ({
handleMoveInstance: vi.fn(),
Expand Down Expand Up @@ -483,6 +484,150 @@ describe("FieldToolbarComponent", () => {
});
});

describe("replace refused by a collaborator's field lock", () => {
const referenceFieldSchema = {
uid: "refs",
data_type: "reference",
display_name: "Refs",
reference_to: ["page"],
field_metadata: { ref_multiple: true },
multiple: true,
} as unknown as ISchemaFieldMap;
const cases = [
{
name: "file",
schema: mockMultipleFileFieldSchema,
event: VisualBuilderPostMessageEvents.OPEN_ASSET_MODAL,
},
{
name: "reference",
schema: referenceFieldSchema,
event: VisualBuilderPostMessageEvents.OPEN_REFERENCE_MODAL,
},
];
const instanceMetadata: CslpData = {
...mockMultipleFieldMetadata,
cslpValue: "page.entry.en-us.refs.0",
fieldPathWithIndex: "refs",
instance: { fieldPathWithIndex: "refs.0" },
};

const selectElementWithCslp = (cslp: string) => {
const element = document.createElement("div");
element.setAttribute("data-cslp", cslp);
VisualBuilder.VisualBuilderGlobalState.value.previousSelectedEditableDOM =
element;
};

const clickReplace = async (
name: string,
schema: ISchemaFieldMap,
response: unknown,
fieldMetadata: CslpData = instanceMetadata
) => {
vi.mocked(FieldSchemaMap.getFieldSchema).mockImplementation(() =>
Promise.resolve(schema)
);
vi.mocked(visualBuilderPostMessage!.send).mockImplementation(

@hitesh-shetty-cstk hitesh-shetty-cstk Oct 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

@faraazb-contentstack faraazb-contentstack Oct 9, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(eventName: string) =>
Promise.resolve(
eventName ===
VisualBuilderPostMessageEvents.OPEN_ASSET_MODAL ||
eventName ===
VisualBuilderPostMessageEvents.OPEN_REFERENCE_MODAL
? response
: {}
) as any
);
const hideOverlay = vi.fn();
const { container } = render(
<FieldToolbarComponent
eventDetails={{
...mockEventDetails,
fieldMetadata,
}}
hideOverlay={hideOverlay}
/>
);
const replaceButton = await findByTestId(
container,
`visual-builder-replace-${name}`
);
await act(async () => {
fireEvent.click(replaceButton);
});
return hideOverlay;
};

afterEach(() => {
VisualBuilder.VisualBuilderGlobalState.value.previousSelectedEditableDOM =
null;
// clearAllMocks keeps implementations; put back the factory's `send`.
vi.mocked(visualBuilderPostMessage!.send).mockReset();
});

test.each(cases)(
"deselects the $name field when the lock is refused",
async ({ name, schema, event }) => {
selectElementWithCslp(instanceMetadata.cslpValue);

const hideOverlay = await clickReplace(name, schema, {
fieldLockRefused: true,
});

expect(visualBuilderPostMessage!.send).toHaveBeenCalledWith(
event,
expect.anything()
);
await waitFor(() => expect(hideOverlay).toHaveBeenCalled());
}
);

test.each(cases)(
"leaves a newer selection alone when the $name lock is refused",
async ({ name, schema }) => {
selectElementWithCslp("page.entry.en-us.title");

const hideOverlay = await clickReplace(name, schema, {
fieldLockRefused: true,
});

await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 0));
});
expect(hideOverlay).not.toHaveBeenCalled();
}
);

test.each(cases)(
"keeps the $name field selected when the modal opens",
async ({ name, schema }) => {
selectElementWithCslp(instanceMetadata.cslpValue);

const hideOverlay = await clickReplace(name, schema, undefined);

await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 0));
});
expect(hideOverlay).not.toHaveBeenCalled();
}
);

test("sends the variant with the reference replace request", async () => {
selectElementWithCslp(instanceMetadata.cslpValue);

await clickReplace("reference", referenceFieldSchema, undefined, {
...instanceMetadata,
variant: "variant_1",
});

expect(visualBuilderPostMessage!.send).toHaveBeenCalledWith(
VisualBuilderPostMessageEvents.OPEN_REFERENCE_MODAL,
expect.objectContaining({ variant: "variant_1" })
);
});
});

describe("'Replace button' visibility for multiple file fields", () => {
beforeEach(() => {
// Override the mock for this describe block - resolve immediately
Expand All @@ -492,7 +637,7 @@ describe("FieldToolbarComponent", () => {
});

afterEach(() => {
// Restore will happen in outer afterEach via clearAllMocks
// Nothing to restore: the outer beforeEach resets getFieldSchema per test.
});

test("'replace button' is hidden for parent wrapper of multiple file field", async () => {
Expand Down
Loading
Loading