Skip to content
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.

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

fix(visual-editor): handle circular pattern dependencies in the tree [SPA-2511] #946

Merged
merged 2 commits into from
Jan 23, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ type DropzoneProps = {
className?: string;
WrapperComponent?: ElementType | string;
dragProps?: DragWrapperProps;
wrappingPatternIds?: Set<string>;
};

export function Dropzone({
Expand All @@ -40,6 +41,7 @@ export function Dropzone({
className,
WrapperComponent = 'div',
dragProps,
wrappingPatternIds: parentWrappingPatternIds = new Set(),
...rest
}: DropzoneProps) {
const userIsDragging = useDraggedItemStore((state) => state.isDraggingOnCanvas);
Expand All @@ -66,6 +68,19 @@ export function Dropzone({
const isRootAssembly = node?.type === ASSEMBLY_NODE_TYPE;
const htmlDraggableProps = getHtmlDragProps(dragProps);
const htmlProps = getHtmlComponentProps(rest);

const wrappingPatternIds = useMemo(() => {
// On the top level, the node is not defined. If the root blockId is not the default string,
// we assume that it is the entry ID of the experience/ pattern to properly detect circular dependencies
if (!node && tree.root.data.blockId && tree.root.data.blockId !== ROOT_ID) {
return new Set([tree.root.data.blockId, ...parentWrappingPatternIds]);
}
if (isRootAssembly && node?.data.blockId) {
return new Set([node.data.blockId, ...parentWrappingPatternIds]);
}
return parentWrappingPatternIds;
}, [isRootAssembly, node, parentWrappingPatternIds, tree.root.data.blockId]);

// To avoid a circular dependency, we create the recursive rendering function here and trickle it down
const renderDropzone: RenderDropzoneFunction = useCallback(
(node, props) => {
Expand All @@ -74,11 +89,12 @@ export function Dropzone({
zoneId={node.data.id}
node={node}
resolveDesignValue={resolveDesignValue}
wrappingPatternIds={wrappingPatternIds}
{...props}
/>
);
},
[resolveDesignValue],
[wrappingPatternIds, resolveDesignValue],
);

const renderClonedDropzone: RenderDropzoneFunction = useCallback(
Expand All @@ -89,11 +105,12 @@ export function Dropzone({
node={node}
resolveDesignValue={resolveDesignValue}
renderDropzone={renderClonedDropzone}
wrappingPatternIds={wrappingPatternIds}
{...props}
/>
);
},
[resolveDesignValue],
[resolveDesignValue, wrappingPatternIds],
);

const isDropzoneEnabled = useMemo(() => {
Expand Down Expand Up @@ -152,6 +169,7 @@ export function Dropzone({
provided={provided}
snapshot={snapshot}
renderDropzone={renderClonedDropzone}
wrappingPatternIds={wrappingPatternIds}
/>
)}>
{(provided, snapshot) => {
Expand Down Expand Up @@ -200,6 +218,7 @@ export function Dropzone({
node={item}
resolveDesignValue={resolveDesignValue}
renderDropzone={renderDropzone}
wrappingPatternIds={wrappingPatternIds}
/>
))
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type DropzoneProps = {
WrapperComponent?: ElementType | string;
renderDropzone: RenderDropzoneFunction;
dragProps?: DragWrapperProps;
wrappingPatternIds: Set<string>;
};

export function DropzoneClone({
Expand All @@ -27,6 +28,7 @@ export function DropzoneClone({
WrapperComponent = 'div',
renderDropzone,
dragProps,
wrappingPatternIds,
...rest
}: DropzoneProps) {
const tree = useTreeStore((state) => state.tree);
Expand Down Expand Up @@ -72,6 +74,7 @@ export function DropzoneClone({
node={item}
resolveDesignValue={resolveDesignValue}
renderDropzone={renderDropzone}
wrappingPatternIds={wrappingPatternIds}
/>
);
})}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ type EditorBlockProps = {
resolveDesignValue: ResolveDesignValueType;
renderDropzone: RenderDropzoneFunction;
zoneId: string;
wrappingPatternIds: Set<string>;
};

export const EditorBlock: React.FC<EditorBlockProps> = ({
Expand All @@ -51,6 +52,7 @@ export const EditorBlock: React.FC<EditorBlockProps> = ({
zoneId,
userIsDragging,
placeholder,
wrappingPatternIds,
}) => {
const { slotId } = parseZoneId(zoneId);
const ref = useRef<HTMLElement | null>(null);
Expand All @@ -69,6 +71,7 @@ export const EditorBlock: React.FC<EditorBlockProps> = ({
resolveDesignValue,
renderDropzone,
userIsDragging,
wrappingPatternIds,
});
const { isSingleColumn, isWrapped } = useSingleColumn(node, resolveDesignValue);
const setDomRect = useDraggedItemStore((state) => state.setDomRect);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type EditorBlockCloneProps = {
provided?: DraggableProvided;
snapshot?: DraggableStateSnapshot;
renderDropzone: RenderDropzoneFunction;
wrappingPatternIds: Set<string>;
};

export const EditorBlockClone: React.FC<EditorBlockCloneProps> = ({
Expand All @@ -37,6 +38,7 @@ export const EditorBlockClone: React.FC<EditorBlockCloneProps> = ({
snapshot,
provided,
renderDropzone,
wrappingPatternIds,
}) => {
const userIsDragging = useDraggedItemStore((state) => state.isDraggingOnCanvas);

Expand All @@ -45,6 +47,7 @@ export const EditorBlockClone: React.FC<EditorBlockCloneProps> = ({
resolveDesignValue,
renderDropzone,
userIsDragging,
wrappingPatternIds,
});

const isAssemblyBlock = node.type === ASSEMBLY_BLOCK_NODE_TYPE;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { useEntityStore } from '@/store/entityStore';
import React, { forwardRef, HTMLAttributes } from 'react';

type CircularDependencyErrorPlaceholderProperties = HTMLAttributes<HTMLDivElement> & {
wrappingPatternIds: Set<string>;
};

export const CircularDependencyErrorPlaceholder = forwardRef<
HTMLDivElement,
CircularDependencyErrorPlaceholderProperties
>(({ wrappingPatternIds, ...props }, ref) => {
const entityStore = useEntityStore((state) => state.entityStore);

return (
<div
{...props}
// Pass through ref to avoid DND errors being logged
ref={ref}
data-cf-node-error="circular-pattern-dependency"
style={{
border: '1px solid red',
background: 'rgba(255, 0, 0, 0.1)',
padding: '1rem 1rem 0 1rem',
width: '100%',
height: '100%',
}}>
Circular usage of patterns detected:
<ul>
{Array.from(wrappingPatternIds).map((patternId) => {
const entryLink = { sys: { type: 'Link', linkType: 'Entry', id: patternId } } as const;
const entry = entityStore.getEntityFromLink(entryLink);
const entryTitle = entry?.fields?.title;
const text = entryTitle ? `${entryTitle} (${patternId})` : patternId;
return <li key={patternId}>{text}</li>;
})}
</ul>
</div>
);
});

CircularDependencyErrorPlaceholder.displayName = 'CircularDependencyErrorPlaceholder';
1 change: 1 addition & 0 deletions packages/visual-editor/src/hooks/useComponent.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ describe('useComponent', () => {
resolveDesignValue,
renderDropzone,
userIsDragging,
wrappingPatternIds: new Set(),
}),
);

Expand Down
37 changes: 26 additions & 11 deletions packages/visual-editor/src/hooks/useComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,22 @@ import { isContentfulStructureComponent } from '@contentful/experiences-core';
import { MissingComponentPlaceholder } from '@components/DraggableHelpers/MissingComponentPlaceholder';
import { useTreeStore } from '@/store/tree';
import { getItem } from '@/utils/getItem';
import { CircularDependencyErrorPlaceholder } from '@components/DraggableHelpers/CircularDependencyErrorPlaceholder';

type UseComponentProps = {
node: ExperienceTreeNode;
resolveDesignValue: ResolveDesignValueType;
renderDropzone: RenderDropzoneFunction;
userIsDragging: boolean;
wrappingPatternIds: Set<string>;
};

export const useComponent = ({
node,
resolveDesignValue,
renderDropzone,
userIsDragging,
wrappingPatternIds,
}: UseComponentProps) => {
const areEntitiesFetched = useEntityStore((state) => state.areEntitiesFetched);
const tree = useTreeStore((state) => state.tree);
Expand Down Expand Up @@ -79,11 +82,32 @@ export const useComponent = ({
});

const elementToRender = (props?: { dragProps?: DragWrapperProps; rest?: unknown }) => {
const { dragProps = {} } = props || {};
const { children, innerRef, Tag = 'div', ToolTipAndPlaceholder, style, ...rest } = dragProps;
const {
'data-cf-node-block-id': dataCfNodeBlockId,
'data-cf-node-block-type': dataCfNodeBlockType,
'data-cf-node-id': dataCfNodeId,
} = componentProps;
const refCallback = (refNode: HTMLElement | null) => {
if (innerRef && refNode) innerRef(refNode);
};

if (!componentRegistration) {
return <MissingComponentPlaceholder blockId={node.data.blockId} />;
}

const { dragProps = {} } = props || {};
if (node.data.blockId && wrappingPatternIds.has(node.data.blockId)) {
return (
<CircularDependencyErrorPlaceholder
ref={refCallback}
data-cf-node-id={dataCfNodeId}
data-cf-node-block-id={dataCfNodeBlockId}
data-cf-node-block-type={dataCfNodeBlockType}
wrappingPatternIds={wrappingPatternIds}
/>
);
}

const element = React.createElement(
ImportedComponentErrorBoundary,
Expand All @@ -98,20 +122,11 @@ export const useComponent = ({
return element;
}

const { children, innerRef, Tag = 'div', ToolTipAndPlaceholder, style, ...rest } = dragProps;
const {
'data-cf-node-block-id': dataCfNodeBlockId,
'data-cf-node-block-type': dataCfNodeBlockType,
'data-cf-node-id': dataCfNodeId,
} = componentProps;

return (
<Tag
{...rest}
style={{ ...style, ...wrapperStyles }}
ref={(refNode: HTMLElement | null) => {
if (innerRef && refNode) innerRef(refNode);
}}
ref={refCallback}
data-cf-node-id={dataCfNodeId}
data-cf-node-block-id={dataCfNodeBlockId}
data-cf-node-block-type={dataCfNodeBlockType}>
Expand Down
5 changes: 2 additions & 3 deletions packages/visual-editor/src/utils/getTreeDiff.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { ExperienceTreeNode, ExperienceTree } from '@contentful/experiences-core

import { getItem } from './getItem';
import { isEqual } from 'lodash-es';
import { ROOT_ID, TreeAction } from '@/types/constants';
import { TreeAction } from '@/types/constants';
import { TreeDiff } from '@/types/treeActions';

interface MissingNodeActionParams {
Expand Down Expand Up @@ -101,13 +101,12 @@ function compareNodes({
const nodeRemoved = currentNodeCount > updatedNodeCount;
const nodeAdded = currentNodeCount < updatedNodeCount;
const parentNodeId = updatedNode.data.id;
const isRoot = currentNode.data.id === ROOT_ID;

/**
* The data of the current node has changed, we need to update
* this node to reflect the data change. (design, content, unbound values)
*/
if (!isRoot && !isEqual(currentNode.data, updatedNode.data)) {
if (!isEqual(currentNode.data, updatedNode.data)) {
differences.push({
type: TreeAction.UPDATE_NODE,
nodeId: currentNode.data.id,
Expand Down
Loading