Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 14 additions & 2 deletions src/visualBuilder/components/FieldToolbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ import { EntryPermissions } from "../utils/getEntryPermissions";
import { FieldLocationAppList } from "./FieldLocationAppList";
import { FieldLocationIcon } from "./FieldLocationIcon";
import { WorkflowStageDetails } from "../utils/getWorkflowStageDetails";
import { getEntryEditRestrictionForField } from "../utils/fieldLockIndicator";
import { subscribeEntryFieldLockInfo } from "../utils/fieldLockStore";
import { ResolvedVariantPermissions } from "../utils/getResolvedVariantPermissions";
import { isCustomFieldMultipleInstance as checkIsCustomFieldMultipleInstance } from "../utils/isCustomFieldMultipleInstance";

Expand Down Expand Up @@ -153,7 +155,17 @@ function FieldToolbarComponent(
let isWholeMultipleField = false;
const APP_LIST_MIN_WIDTH = 230;

let disableFieldActions = false;
const [restrictionReason, setRestrictionReason] = useState(() =>
getEntryEditRestrictionForField(fieldMetadata)
);
useEffect(() => {
const update = () =>
setRestrictionReason(getEntryEditRestrictionForField(fieldMetadata));
update();
return subscribeEntryFieldLockInfo(update);

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.

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

}, [fieldMetadata]);

let disableFieldActions = Boolean(restrictionReason);

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.

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

let isCustomFieldMultipleInstance = false;
let isCustomFieldWholeMultiple = false;
if (fieldSchema) {
Expand All @@ -167,7 +179,7 @@ function FieldToolbarComponent(
entryPermissions,
entryWorkflowStageDetails,
);
disableFieldActions = isDisabled;
disableFieldActions = isDisabled || Boolean(restrictionReason);

fieldType = getFieldType(fieldSchema);

Expand Down
12 changes: 5 additions & 7 deletions src/visualBuilder/components/__test__/emptyBlock.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ 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";
import { isFieldBlockedByAutoDraft } from "../../utils/fieldLockIndicator";

vi.mock("../../utils/visualBuilderPostMessage", () => ({
default: {
Expand All @@ -20,7 +20,7 @@ vi.mock("../../utils/multipleElementAddButton", () => ({
}));

vi.mock("../../utils/fieldLockIndicator", () => ({
getPeerLockForField: vi.fn(() => null),
isFieldBlockedByAutoDraft: vi.fn(() => false),
}));

describe("EmptyBlock", () => {
Expand Down Expand Up @@ -129,15 +129,13 @@ describe("EmptyBlock", () => {
);
});

test("adds nothing when a peer holds the field", async () => {
(getPeerLockForField as any).mockReturnValueOnce({
user: { uid: "peer" },
});
test("adds nothing when a peer holds the field or the entry is restricted", async () => {
(isFieldBlockedByAutoDraft as any).mockReturnValueOnce(true);

const { getByTestId } = render(<EmptyBlock details={mockDetails} />);
fireEvent.click(getByTestId("visual-builder__empty-block-add-button"));

await waitFor(() => expect(getPeerLockForField).toHaveBeenCalled());
await waitFor(() => expect(isFieldBlockedByAutoDraft).toHaveBeenCalled());
expect((visualBuilderPostMessage as any).send).not.toHaveBeenCalled();
expect(observeParentAndFocusNewInstance).not.toHaveBeenCalled();
});
Expand Down
52 changes: 52 additions & 0 deletions src/visualBuilder/components/__test__/fieldToolbar.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ import React from "preact/compat";
import visualBuilderPostMessage from "../../utils/visualBuilderPostMessage";
import { FieldLocationIcon } from "../FieldLocationIcon";
import { VisualBuilderPostMessageEvents } from "../../utils/types/postMessage.types";
import {
clearAllEntryFieldLockInfo,
setEntryEditRestriction,
} from "../../utils/fieldLockStore";

vi.mock("../../utils/instanceHandlers", () => ({
handleMoveInstance: vi.fn(),
Expand Down Expand Up @@ -629,5 +633,53 @@ describe("FieldToolbarComponent", () => {
expect(replaceButton).toBeDisabled();
}
});

describe("on an entry the parent has restricted", () => {
const scope = { entryUid: "e1", locale: "en-us" };
const renderToolbar = async () => {
const { container } = render(
<FieldToolbarComponent
eventDetails={{
...mockEventDetails,
fieldMetadata: {
...mockMultipleFieldMetadata,
entry_uid: "e1",
locale: "en-us",
},
}}
hideOverlay={vi.fn()}
/>
);
await findByTestId(
container as HTMLElement,
"visual-builder__focused-toolbar__multiple-field-toolbar",
{},
{ timeout: 1000 }
);
return () =>
container.querySelector(
'[data-testid="visual-builder__focused-toolbar__multiple-field-toolbar__delete-button"]'
);
};

afterEach(() => clearAllEntryFieldLockInfo());

test("disables the field actions", async () => {
setEntryEditRestriction(scope, "olderVersion");

const deleteButton = await renderToolbar();

await waitFor(() => expect(deleteButton()).toBeDisabled());
});

test("disables the field actions when the restriction arrives after the toolbar opened", async () => {
const deleteButton = await renderToolbar();
await waitFor(() => expect(deleteButton()).not.toBeDisabled());

act(() => setEntryEditRestriction(scope, "olderVersion"));

await waitFor(() => expect(deleteButton()).toBeDisabled());
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
} from "./fieldLabelWrapper.mocks";
import {
setEntryFieldLockInfo,
setEntryEditRestriction,
clearAllEntryFieldLockInfo,
} from "../../../utils/fieldLockStore";

Expand Down Expand Up @@ -383,4 +384,83 @@ describe("FieldLabelWrapperComponent - Disabled Class", () => {
"visual-builder__focused-toolbar--field-disabled"
);
});

const OLDER_VERSION_MESSAGE =
"You're viewing an older version of this entry. Switch to the latest version to edit.";

const renderLabel = async () => {
const { container } = render(
<FieldLabelWrapperComponent
fieldMetadata={mockFieldMetadata}
eventDetails={mockEventDetails}
parentPaths={[]}
getParentEditableElement={mockGetParentEditable}
/>
);
await act(async () => {
await new Promise<void>((resolve) =>
queueMicrotask(() => resolve())
);
});
return (await findByTestId(
container as HTMLElement,
"visual-builder__focused-toolbar__field-label-wrapper",
{},
{ timeout: 1000 }
)) as HTMLElement;
};

test("shows a restricted entry as disabled with the restriction's message", async () => {
(isFieldDisabled as any).mockReturnValue({
isDisabled: false,
reason: "",
});
setEntryEditRestriction(
{
entryUid: mockFieldMetadata.entry_uid,
locale: mockFieldMetadata.locale,
},
"olderVersion"
);

const fieldLabel = await renderLabel();

expect(fieldLabel).toHaveClass(
"visual-builder__focused-toolbar--field-disabled"
);
expect(
fieldLabel.querySelector(
`[data-tooltip="${OLDER_VERSION_MESSAGE}"]`
)
).not.toBeNull();
});

test("prefers the entry restriction's message over a peer lock", async () => {
(isFieldDisabled as any).mockReturnValue({
isDisabled: false,
reason: "",
});
const scope = {
entryUid: mockFieldMetadata.entry_uid,
locale: mockFieldMetadata.locale,
};
setEntryFieldLockInfo(scope, {
[mockFieldMetadata.fieldPath]: {
user: { uid: "u1", name: "Ada Lovelace" },
ttl: "2030-01-01",
isLocked: true,
isOwn: false,
},
});
setEntryEditRestriction(scope, "olderVersion");

const fieldLabel = await renderLabel();

expect(
fieldLabel.querySelector(
`[data-tooltip="${OLDER_VERSION_MESSAGE}"]`
)
).not.toBeNull();
expect(fieldLabel.textContent).not.toContain("Ada Lovelace");
});
});
8 changes: 4 additions & 4 deletions src/visualBuilder/components/emptyBlock.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ 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";
import { isFieldBlockedByAutoDraft } from "../utils/fieldLockIndicator";
import { DATA_CSLP_ATTR_SELECTOR } from "../utils/constants";

interface EmptyBlockProps {
Expand All @@ -26,9 +26,9 @@ export function EmptyBlock(props: EmptyBlockProps): JSX.Element {
async function sendAddInstanceEvent(
event: JSX.TargetedMouseEvent<HTMLButtonElement>
) {
// 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;
// A peer holds this field or its entry is restricted: the same no-op a click on a
// blocked field gets in the click listener.
if (isFieldBlockedByAutoDraft(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.
Expand Down
27 changes: 19 additions & 8 deletions src/visualBuilder/components/fieldLabelWrapper.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ import { fetchEntryPermissionsAndStageDetails } from "../utils/fetchEntryPermiss
import { VariantIndicator } from "./VariantIndicator";
import { handleRevalidateFieldData } from "../eventManager/useRevalidateFieldDataPostMessageEvent";
import { RESULT_TYPES } from "../utils/constants";
import { getPeerLockForField, lockAvatarInfo } from "../utils/fieldLockIndicator";
import {
getEntryEditRestrictionForField,
getPeerLockForField,
lockAvatarInfo,
} from "../utils/fieldLockIndicator";
import { subscribeEntryFieldLockInfo } from "../utils/fieldLockStore";

interface ReferenceParentMap {
Expand Down Expand Up @@ -236,13 +240,20 @@ function FieldLabelWrapperComponent(
const base = labelBaseRef.current;
if (!base) return;
const peerLock = getPeerLockForField(props.fieldMetadata);
const effectiveDisabled = base.fieldDisabled || Boolean(peerLock);
const effectiveReason =
!base.fieldDisabled && peerLock
? `This field is locked by ${
lockAvatarInfo(peerLock).name || "another user"
}`
: base.reason;
const restrictionReason = getEntryEditRestrictionForField(
props.fieldMetadata
);
const effectiveDisabled =
base.fieldDisabled || Boolean(restrictionReason) || Boolean(peerLock);
// Permission and workflow reasons win, then the entry restriction, then a peer lock.
let effectiveReason = base.reason;
if (!base.fieldDisabled && restrictionReason) {
effectiveReason = restrictionReason;
} else if (!base.fieldDisabled && peerLock) {
effectiveReason = `This field is locked by ${
lockAvatarInfo(peerLock).name || "another user"
}`;
}
const usePlainDataTooltip =
effectiveReason &&
!effectiveReason.includes(DisableReason.CanLinkVariant) &&
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import visualBuilderPostMessage from "../utils/visualBuilderPostMessage";
import { VisualBuilderPostMessageEvents } from "../utils/types/postMessage.types";
import {
setEntryEditRestriction,
toEntryEditRestriction,
} from "../utils/fieldLockStore";

interface EntryEditRestrictionUpdateEvent {
data: {
entryUid: string;
locale: string;
variantUid?: string;
restriction: unknown;
};
}

/**
* Mirrors the parent's per-entry edit restriction (older version open, unlocalized, ...) so the
* entry's fields render disabled with the matching reason. Null clears the scope.
*/
export function useEntryEditRestrictionUpdateEvent(): void {
visualBuilderPostMessage?.on(
VisualBuilderPostMessageEvents.ENTRY_EDIT_RESTRICTION_UPDATE,
(event: EntryEditRestrictionUpdateEvent) => {
const { entryUid, locale, variantUid, restriction } = event.data;
setEntryEditRestriction(
{ entryUid, locale, variantUid },
toEntryEditRestriction(restriction)
);
}
);
}
13 changes: 9 additions & 4 deletions src/visualBuilder/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import { extractDetailsFromCslp, isValidCslp } from "../cslp";
import initUI from "./components";
import { useDraftFieldsPostMessageEvent } from "./eventManager/useDraftFieldsPostMessageEvent";
import { useEntryLockInfoUpdateEvent } from "./eventManager/useEntryLockInfoUpdateEvent";
import { useEntryEditRestrictionUpdateEvent } from "./eventManager/useEntryEditRestrictionUpdateEvent";
import { getEntryEditRestrictionForField } from "./utils/fieldLockIndicator";
import { useHideFocusOverlayPostMessageEvent } from "./eventManager/useHideFocusOverlayPostMessageEvent";
import { useScrollToField } from "./eventManager/useScrollToField";
import { debounceAddVariantFieldClass, getHighlightVariantFieldsStatus, setHighlightVariantFields, useVariantFieldsPostMessageEvent } from "./eventManager/useVariantsPostMessageEvent";
Expand Down Expand Up @@ -195,10 +197,12 @@ export class VisualBuilder {
if (!fieldSchema) {
return;
}
const { isDisabled } = isFieldDisabled(fieldSchema, {
editableElement,
fieldMetadata,
});
const isDisabled =
isFieldDisabled(fieldSchema, {
editableElement,
fieldMetadata,
}).isDisabled ||
Boolean(getEntryEditRestrictionForField(fieldMetadata));
if (isDisabled) {
addFocusOverlay(
editableElement,
Expand Down Expand Up @@ -416,6 +420,7 @@ export class VisualBuilder {
useRecalculateVariantDataCSLPValues();
useDraftFieldsPostMessageEvent();
useEntryLockInfoUpdateEvent();
useEntryEditRestrictionUpdateEvent();
useVariantFieldsPostMessageEvent({ isSSR: config.ssr ?? false });
}
})
Expand Down
10 changes: 7 additions & 3 deletions src/visualBuilder/listeners/mouseClick.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ import { VisualBuilderPostMessageEvents } from "../utils/types/postMessage.types
import { VisualBuilder } from "..";
import { FieldSchemaMap } from "../utils/fieldSchemaMap";
import { isFieldDisabled } from "../utils/isFieldDisabled";
import { getPeerLockForField } from "../utils/fieldLockIndicator";
import {
getEntryEditRestrictionForField,
getPeerLockForField,
} from "../utils/fieldLockIndicator";
import EventListenerHandlerParams from "./types";
import { toggleHighlightedCommentIconDisplay } from "../generators/generateHighlightedComment";
import { VB_EmptyBlockParentClass } from "../..";
Expand Down Expand Up @@ -187,7 +190,8 @@ export async function handleBuilderInteraction(
// gate runs before the post message so a click on a peer-locked field is a
// true no-op: posting MOUSE_CLICK with its fieldMetadata makes the host read
// it as a fresh selection and cancel the current user's own pending lock
// release.
// 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.
Comment on lines +193 to +194

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.

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

if (eventDetails && getPeerLockForField(eventDetails.fieldMetadata)) {
return;
}
Expand Down Expand Up @@ -438,7 +442,7 @@ async function handleFieldSchemaAndIndividualFields(
entryAcl,
entryWorkflowStageDetails
);
if (isDisabled) {
if (isDisabled || getEntryEditRestrictionForField(fieldMetadata)) {
addOverlay({
overlayWrapper: params.overlayWrapper,
resizeObserver: params.resizeObserver,
Expand Down
Loading
Loading