From 0d8e4ffd25026c12c86432e4ed0958350fd229b4 Mon Sep 17 00:00:00 2001 From: SahilCs15 Date: Wed, 9 Sep 2026 17:49:42 +0530 Subject: [PATCH 1/2] fix(visual-builder): claim the field lock from the empty-block add The empty-state placeholder never selects the field, so its add button claimed no lock and a peer editor was never told the field was changing. Send FOCUS_FIELD with the element's edit stack before ADD_INSTANCE, skip the send when the stack is empty (the parent reads that as a deselect and would release the lock), and refuse the add outright when a peer holds the field, matching the click listener's peer-lock gate. --- .../components/__test__/emptyBlock.test.tsx | 67 +++++++++++++++++++ src/visualBuilder/components/emptyBlock.tsx | 41 +++++++++--- 2 files changed, 99 insertions(+), 9 deletions(-) diff --git a/src/visualBuilder/components/__test__/emptyBlock.test.tsx b/src/visualBuilder/components/__test__/emptyBlock.test.tsx index d7d5b2c8..1eb93da2 100644 --- a/src/visualBuilder/components/__test__/emptyBlock.test.tsx +++ b/src/visualBuilder/components/__test__/emptyBlock.test.tsx @@ -6,6 +6,8 @@ import { observeParentAndFocusNewInstance } from "../../utils/multipleElementAdd import { CslpData } from "../../../cslp/types/cslp.types"; import { ISchemaFieldMap } from "../../utils/types/index.types"; import { VisualBuilderPostMessageEvents } from "../../utils/types/postMessage.types"; +import { getDOMEditStack } from "../../utils/getCsDataOfElement"; +import { getPeerLockForField } from "../../utils/fieldLockIndicator"; vi.mock("../../utils/visualBuilderPostMessage", () => ({ default: { @@ -17,6 +19,14 @@ vi.mock("../../utils/multipleElementAddButton", () => ({ observeParentAndFocusNewInstance: vi.fn(), })); +vi.mock("../../utils/fieldLockIndicator", () => ({ + getPeerLockForField: vi.fn(() => null), +})); + +const flushMicrotasks = async () => { + for (let i = 0; i < 10; i += 1) await Promise.resolve(); +}; + describe("EmptyBlock", () => { const mockDetails = { fieldMetadata: { @@ -70,4 +80,61 @@ describe("EmptyBlock", () => { index: 0, }); }); + + test("claims the field lock before adding, so a peer sees it", async () => { + const host = document.createElement("div"); + host.setAttribute("data-cslp", "ct.entry.en-us.blocks_field"); + document.body.appendChild(host); + + const { getByTestId } = render(, { + container: host, + }); + fireEvent.click(getByTestId("visual-builder__empty-block-add-button")); + + await waitFor(() => { + expect((visualBuilderPostMessage as any).send).toHaveBeenCalledWith( + VisualBuilderPostMessageEvents.FOCUS_FIELD, + { DOMEditStack: getDOMEditStack(host) } + ); + }); + + // the lock must be claimed first, or the parent applies the add with no lock + const events = (visualBuilderPostMessage as any).send.mock.calls.map( + (call: unknown[]) => call[0] + ); + expect(events).toEqual([ + VisualBuilderPostMessageEvents.FOCUS_FIELD, + VisualBuilderPostMessageEvents.ADD_INSTANCE, + ]); + }); + + test("does not send an empty edit stack, which the parent reads as a deselect", async () => { + // no ancestor carries data-cslp, so the stack comes back empty + const { getByTestId } = render(); + fireEvent.click(getByTestId("visual-builder__empty-block-add-button")); + + await waitFor(() => { + expect((visualBuilderPostMessage as any).send).toHaveBeenCalledWith( + VisualBuilderPostMessageEvents.ADD_INSTANCE, + { fieldMetadata: mockDetails.fieldMetadata, index: 0 } + ); + }); + expect((visualBuilderPostMessage as any).send).not.toHaveBeenCalledWith( + VisualBuilderPostMessageEvents.FOCUS_FIELD, + expect.anything() + ); + }); + + test("adds nothing when a peer holds the field", async () => { + (getPeerLockForField as any).mockReturnValueOnce({ + user: { uid: "peer" }, + }); + + const { getByTestId } = render(); + fireEvent.click(getByTestId("visual-builder__empty-block-add-button")); + await flushMicrotasks(); + + expect((visualBuilderPostMessage as any).send).not.toHaveBeenCalled(); + expect(observeParentAndFocusNewInstance).not.toHaveBeenCalled(); + }); }); diff --git a/src/visualBuilder/components/emptyBlock.tsx b/src/visualBuilder/components/emptyBlock.tsx index ad932e70..46b4444b 100644 --- a/src/visualBuilder/components/emptyBlock.tsx +++ b/src/visualBuilder/components/emptyBlock.tsx @@ -7,6 +7,8 @@ import { ISchemaFieldMap } from "../utils/types/index.types"; import { VisualBuilderPostMessageEvents } from "../utils/types/postMessage.types"; import React from "preact/compat"; import { startCase, toLower } from "lodash-es"; +import { getDOMEditStack } from "../utils/getCsDataOfElement"; +import { getPeerLockForField } from "../utils/fieldLockIndicator"; interface EmptyBlockProps { details: { @@ -20,14 +22,33 @@ export function EmptyBlock(props: EmptyBlockProps): JSX.Element { const blockParentName = details.fieldSchema.display_name; - async function sendAddInstanceEvent() { - await visualBuilderPostMessage?.send( - VisualBuilderPostMessageEvents.ADD_INSTANCE, - { - fieldMetadata: details.fieldMetadata, - index: 0, - } - ); + async function sendAddInstanceEvent(event: MouseEvent) { + // A peer holds this field: adding would edit through their lock, the same + // no-op a click on a peer-locked field gets in the click listener. + if (getPeerLockForField(details.fieldMetadata)) return; + + // The empty-state add never selects the field, so nothing else claims the + // lock. Fire and forget: the parent does not await the claim either. + const DOMEditStack = getDOMEditStack(event.currentTarget as Element); + // An empty stack reads as a deselect on the parent and would RELEASE the lock. + if (DOMEditStack.length) { + visualBuilderPostMessage?.send( + VisualBuilderPostMessageEvents.FOCUS_FIELD, + { DOMEditStack } + ); + } + + try { + await visualBuilderPostMessage?.send( + VisualBuilderPostMessageEvents.ADD_INSTANCE, + { + fieldMetadata: details.fieldMetadata, + index: 0, + } + ); + } catch (error) { + console.error("Visual Builder: Failed to add instance", error); + } observeParentAndFocusNewInstance({ parentCslp: details.fieldMetadata.cslpValue, index: 0, @@ -67,7 +88,9 @@ export function EmptyBlock(props: EmptyBlockProps): JSX.Element { "visual-builder__empty-block-add-button" ] )} - onClick={() => sendAddInstanceEvent()} + onClick={(e) => + sendAddInstanceEvent(e as unknown as MouseEvent) + } type="button" data-testid="visual-builder__empty-block-add-button" > From 07a1c652b91455f5a449efd67cc4c2f63b6b257b Mon Sep 17 00:00:00 2001 From: SahilCs15 Date: Thu, 10 Sep 2026 17:36:18 +0530 Subject: [PATCH 2/2] fix(visual-builder): address review on the empty-block lock claim - Return from the ADD_INSTANCE catch so a failed add no longer starts an observer waiting for an instance that will never appear. - Resolve the field element from the cslp rather than the button's DOM position, so a portal render cannot silently yield an empty edit stack and skip the lock claim. Falls back to the button. - Type the handler as JSX.TargetedMouseEvent, dropping the double cast through unknown and the cast on currentTarget. - Clear mocks in beforeEach rather than afterEach, remove the appended host node RTL does not clean up, and replace the fixed 10-microtask drain with a waitFor on an observable signal. --- .../components/__test__/emptyBlock.test.tsx | 26 +++++++++++-------- src/visualBuilder/components/emptyBlock.tsx | 20 ++++++++++---- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/src/visualBuilder/components/__test__/emptyBlock.test.tsx b/src/visualBuilder/components/__test__/emptyBlock.test.tsx index 1eb93da2..fb53a33d 100644 --- a/src/visualBuilder/components/__test__/emptyBlock.test.tsx +++ b/src/visualBuilder/components/__test__/emptyBlock.test.tsx @@ -23,24 +23,28 @@ vi.mock("../../utils/fieldLockIndicator", () => ({ getPeerLockForField: vi.fn(() => null), })); -const flushMicrotasks = async () => { - for (let i = 0; i < 10; i += 1) await Promise.resolve(); -}; - describe("EmptyBlock", () => { const mockDetails = { fieldMetadata: { - cslpValue: "parent.cslp.value", + cslpValue: "ct.entry.en-us.blocks_field", } as CslpData, fieldSchema: { display_name: "Test Block", } as ISchemaFieldMap, }; - afterEach(() => { + let host: HTMLElement | null = null; + + beforeEach(() => { vi.clearAllMocks(); }); + afterEach(() => { + // RTL only removes containers it created, so this one would outlive the test + host?.remove(); + host = null; + }); + test("should render correctly", () => { const { getByText, getByTestId } = render( @@ -82,19 +86,19 @@ describe("EmptyBlock", () => { }); test("claims the field lock before adding, so a peer sees it", async () => { - const host = document.createElement("div"); - host.setAttribute("data-cslp", "ct.entry.en-us.blocks_field"); + host = document.createElement("div"); + host.setAttribute("data-cslp", mockDetails.fieldMetadata.cslpValue); document.body.appendChild(host); const { getByTestId } = render(, { - container: host, + container: host as HTMLElement, }); fireEvent.click(getByTestId("visual-builder__empty-block-add-button")); await waitFor(() => { expect((visualBuilderPostMessage as any).send).toHaveBeenCalledWith( VisualBuilderPostMessageEvents.FOCUS_FIELD, - { DOMEditStack: getDOMEditStack(host) } + { DOMEditStack: getDOMEditStack(host as HTMLElement) } ); }); @@ -132,8 +136,8 @@ describe("EmptyBlock", () => { const { getByTestId } = render(); fireEvent.click(getByTestId("visual-builder__empty-block-add-button")); - await flushMicrotasks(); + await waitFor(() => expect(getPeerLockForField).toHaveBeenCalled()); expect((visualBuilderPostMessage as any).send).not.toHaveBeenCalled(); expect(observeParentAndFocusNewInstance).not.toHaveBeenCalled(); }); diff --git a/src/visualBuilder/components/emptyBlock.tsx b/src/visualBuilder/components/emptyBlock.tsx index 46b4444b..4ca4a818 100644 --- a/src/visualBuilder/components/emptyBlock.tsx +++ b/src/visualBuilder/components/emptyBlock.tsx @@ -9,6 +9,7 @@ import React from "preact/compat"; import { startCase, toLower } from "lodash-es"; import { getDOMEditStack } from "../utils/getCsDataOfElement"; import { getPeerLockForField } from "../utils/fieldLockIndicator"; +import { DATA_CSLP_ATTR_SELECTOR } from "../utils/constants"; interface EmptyBlockProps { details: { @@ -22,14 +23,23 @@ export function EmptyBlock(props: EmptyBlockProps): JSX.Element { const blockParentName = details.fieldSchema.display_name; - async function sendAddInstanceEvent(event: MouseEvent) { + async function sendAddInstanceEvent( + event: JSX.TargetedMouseEvent + ) { // A peer holds this field: adding would edit through their lock, the same // no-op a click on a peer-locked field gets in the click listener. if (getPeerLockForField(details.fieldMetadata)) return; + // Resolve the field by its cslp, not the button's DOM position: a portal + // render would yield an empty stack and silently skip the lock claim. + const fieldElement = + document.querySelector( + `[${DATA_CSLP_ATTR_SELECTOR}="${details.fieldMetadata.cslpValue}"]` + ) ?? event.currentTarget; + // The empty-state add never selects the field, so nothing else claims the // lock. Fire and forget: the parent does not await the claim either. - const DOMEditStack = getDOMEditStack(event.currentTarget as Element); + const DOMEditStack = getDOMEditStack(fieldElement); // An empty stack reads as a deselect on the parent and would RELEASE the lock. if (DOMEditStack.length) { visualBuilderPostMessage?.send( @@ -48,7 +58,9 @@ export function EmptyBlock(props: EmptyBlockProps): JSX.Element { ); } catch (error) { console.error("Visual Builder: Failed to add instance", error); + return; } + observeParentAndFocusNewInstance({ parentCslp: details.fieldMetadata.cslpValue, index: 0, @@ -88,9 +100,7 @@ export function EmptyBlock(props: EmptyBlockProps): JSX.Element { "visual-builder__empty-block-add-button" ] )} - onClick={(e) => - sendAddInstanceEvent(e as unknown as MouseEvent) - } + onClick={sendAddInstanceEvent} type="button" data-testid="visual-builder__empty-block-add-button" >