fix(global-sections): block structural edits inside a global-section instance

syncInstances rebuilds an instance's subtree from the master, so a node
dropped, pasted, duplicated or moved into an instance vanished from every page
on the next master edit. Chosen approach: BLOCK (not preserve-on-sync):
- every resolver component's Craft canMoveIn/canMoveOut refuses a canvas inside
  an instance (canvas DnD, Layers DnD, actions.move); synthetic shell-inner
  nodes now carry the component's rules instead of hardcoded allow-all
- insertBlock (BlocksPanel / EmptyContainerAdd / palette), paste (menu +
  shortcut), duplicate (menu + shortcut) and Sitesmith patch/section replace
  redirect to after the instance or refuse; no empty-container '+' inside one
The instance root still moves/duplicates as a whole; content overrides are
unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
shadowdaoandClaude Opus 5.5 committed 2026-10-07 13:32:34 -07:00
1 parent ca500d36a9
commit 6d22aabdc0
11 files changed
+271 -17

No files matched your search

+25
View File
@@ -41,6 +41,7 @@ import { OpeningHours } from './business/OpeningHours';
import { StickyCallBar } from './business/StickyCallBar';
import { ReviewsStrip } from './business/ReviewsStrip';
import { BookingEmbed } from './business/BookingEmbed';
import { canvasInInstance } from '../utils/global-sections';
export const componentResolver = {
Container,
@@ -87,3 +88,27 @@ export const componentResolver = {
ReviewsStrip,
BookingEmbed,
};
/**
* #57 finding 4: no drop / move / reorder into, inside or out of a global
* section instance (its structure is rebuilt from the master on every sync).
* Wraps every component's Craft `canMoveIn` / `canMoveOut` -- the rules
* Craft's `isDroppable` checks for canvas DnD, Layers DnD and
* `actions.move` -- keeping each component's own rule.
*/
type RuleFn = (nodes: any, self: any, helper: any) => boolean;
for (const comp of Object.values(componentResolver) as any[]) {
const craft = (comp.craft ??= {});
const rules = craft.rules ?? {};
if (rules.__instanceGuard) continue;
const prevIn: RuleFn | undefined = rules.canMoveIn;
const prevOut: RuleFn | undefined = rules.canMoveOut;
craft.rules = {
...rules,
canMoveIn: (incoming: any, self: any, helper: any) =>
!canvasInInstance(self, helper) && (prevIn ? prevIn(incoming, self, helper) : true),
canMoveOut: (outgoing: any, self: any, helper: any) =>
!canvasInInstance(self, helper) && (prevOut ? prevOut(outgoing, self, helper) : true),
__instanceGuard: true,
};
}
+7 -1
View File
@@ -1,4 +1,5 @@
import React, { useCallback, useEffect, useLayoutEffect, useRef, useState } from 'react';
import { instanceMeta } from '../utils/global-sections';
import { useEditor } from '@craftjs/core';
import { BLOCKS, insertBlock } from '../panels/left/blocks-registry';
@@ -42,7 +43,12 @@ export const EmptyContainerAdd: React.FC = () => {
const eligible = (id: string | undefined): id is string => {
if (!id || id === 'ROOT') return false;
const n = state.nodes[id];
return !!n && !!n.data.isCanvas && n.data.nodes.length === 0;
if (!n || !n.data.isCanvas || n.data.nodes.length !== 0) return false;
// #57: no "+" inside a global-section instance (its structure belongs to the master).
for (let p: any = n, guard = 0; p && guard < 500; p = p.data.parent ? state.nodes[p.data.parent] : null, guard++) {
if (instanceMeta(p.data)) return false;
}
return true;
};
const hovered = [...state.events.hovered][0];
const selected = [...state.events.selected][0];
+7 -1
View File
@@ -1,4 +1,5 @@
import { registerCommands, type Command, type CommandContext } from './commands';
import { insertTargetOutsideInstance, isInsideInstance } from '../utils/global-sections';
import { findDeletableTarget, getAncestorIds, getParentId } from '../utils/craft-helpers';
import { regenerateTreeIds } from '../utils/craft-tree';
import { getClipboardTree, setClipboardTree } from '../hooks/clipboard';
@@ -64,6 +65,8 @@ function duplicateAll({ query, actions, selectedIds }: CommandContext): void {
const idx = sib ? sib.siblings.indexOf(id) : -1;
// Linked nodes (columns, section inner) are not in `nodes` and cannot be copied alone.
if (!sib || idx === -1) continue;
// #57: a copy inside an instance would be wiped by the next master sync.
if (isInsideInstance(query, sib.parentId)) continue;
const tree = regenerateTreeIds(query.node(id).toNodeTree());
created.push(insertTree(step(), tree, sib.parentId, idx + 1));
} catch (err) {
@@ -104,8 +107,11 @@ function pasteClipboard({ query, actions, selectedIds }: CommandContext): void {
if (idx !== -1) index = idx + 1;
}
}
// #57: pasting next to a node inside an instance lands after the instance.
const at = insertTargetOutsideInstance(query, parentId, index);
if (!at) return;
const tree = regenerateTreeIds(clip);
actions.selectNode(insertTree(actions, tree, parentId, index));
actions.selectNode(insertTree(actions, tree, at.parentId, at.index));
}
/** One step among siblings. Moving down walks the selection in reverse so a run of
+4
View File
@@ -1,4 +1,5 @@
import { useEditor } from '@craftjs/core';
import { isInsideInstance } from '../utils/global-sections';
import { regenerateTreeIds } from '../utils/craft-tree';
import { findDeletableTarget } from '../utils/craft-helpers';
@@ -121,6 +122,9 @@ export function useNodeActions(nodeId: string | null | undefined): NodeActions {
const parentId = getParentId();
if (!parentId) return null;
// #57: a copy inside an instance would be wiped by the next master sync.
if (isInsideInstance(query, parentId)) return null;
const tree = regenerateTreeIds(query.node(nodeId).toNodeTree());
// Insert the copy IMMEDIATELY AFTER the source node, not appended at
@@ -1,4 +1,5 @@
import React, { useEffect, useCallback, useRef, useState } from 'react';
import { insertTargetOutsideInstance } from '../../utils/global-sections';
import { useEditor } from '@craftjs/core';
import { useSitesmithModal } from '../../state/SitesmithContext';
import { buildSitesmithTarget } from '../../utils/sitesmith-target';
@@ -181,11 +182,14 @@ export const ContextMenu: React.FC<ContextMenuProps> = ({
}
}
// #57: pasting next to a node inside an instance lands after the instance.
const at = insertTargetOutsideInstance(query, targetParent, insertIndex);
if (!at) { onClose(); return; }
const tree = regenerateTreeIds(clip);
if (insertIndex !== undefined) {
actions.addNodeTree(tree, targetParent, insertIndex);
if (at.index !== undefined) {
actions.addNodeTree(tree, at.parentId, at.index);
} else {
actions.addNodeTree(tree, targetParent);
actions.addNodeTree(tree, at.parentId);
}
actions.selectNode(tree.rootNodeId);
} catch (e) {
+14 -4
View File
@@ -1,4 +1,5 @@
import React from 'react';
import { findInstanceRootIn, insertTargetOutsideInstance, isInsideInstance } from '../../utils/global-sections';
import { Container } from '../../components/layout/Container';
import { Section } from '../../components/layout/Section';
import { ColumnLayout } from '../../components/layout/ColumnLayout';
@@ -193,7 +194,8 @@ export function findDefaultCanvasId(query: any): string {
try {
const serialized = JSON.parse(query.serialize());
for (const id of Object.keys(serialized)) {
if (serialized[id].isCanvas && id !== 'ROOT') return id;
// #57: never an instance's inner canvas (its structure belongs to the master).
if (serialized[id].isCanvas && id !== 'ROOT' && !findInstanceRootIn(serialized, id)) return id;
}
} catch {
// Fall through to ROOT below.
@@ -227,8 +229,11 @@ export function insertBlock(query: any, actions: any, blockId: string, opts: Ins
if (parentId) {
const siblings: string[] = query.node(parentId).get()?.data?.nodes || [];
const idx = siblings.indexOf(selectedId);
if (idx !== -1) {
actions.addNodeTree(tree, parentId, idx + 1);
// #57: next to a node inside an instance -> next to the instance.
const at = idx !== -1 ? insertTargetOutsideInstance(query, parentId, idx + 1) : null;
if (at) {
if (at.index !== undefined) actions.addNodeTree(tree, at.parentId, at.index);
else actions.addNodeTree(tree, at.parentId);
inserted = true;
}
}
@@ -239,7 +244,12 @@ export function insertBlock(query: any, actions: any, blockId: string, opts: Ins
}
}
if (!inserted) actions.addNodeTree(tree, opts.parentId ?? findDefaultCanvasId(query));
if (!inserted) {
const parentId = opts.parentId ?? findDefaultCanvasId(query);
// #57: an explicit target inside an instance (EmptyContainerAdd on its inner canvas) is refused.
if (isInsideInstance(query, parentId)) return null;
actions.addNodeTree(tree, parentId);
}
if (opts.select !== false) {
try {
@@ -20,6 +20,12 @@ import { SectionLibraryModal, openSectionLibrary, closeSectionLibrary } from '..
import { Canvas } from '../editor/Canvas';
import { useApplyAiResponse } from '../utils/apply-ai-response';
import { TemplateModal } from '../panels/topbar/TemplateModal';
import { insertBlock } from '../panels/left/blocks-registry';
import { moveLayer } from '../panels/left/layers-dnd';
import { CORE_COMMANDS } from '../editor/core-commands';
import { setClipboardTree } from '../hooks/clipboard';
import { useNodeActions } from '../hooks/useNodeActions';
import { EmptyContainerAdd } from '../editor/EmptyContainerAdd';
import { HistoryModal } from '../panels/history/HistoryModal';
/**
@@ -514,6 +520,118 @@ describe('#57 global sections', () => {
expect(document.body.textContent).toContain('Click Done on the global section');
});
describe('#57 finding 4: no structural edits inside an instance (blocked, so a master sync never deletes user content)', () => {
const cmd = (id: string) => CORE_COMMANDS.find((c) => c.id === id)!;
const ctx = (ids: string[]) => ({ query: h!.query, actions: h!.actions, selectedIds: ids, setDevice: vi.fn() }) as any;
const setup = () => {
mount();
const sec = addPromo();
let libraryId = '';
h!.act(() => { libraryId = globals.makeGlobal(sec, 'Promo')!; });
// A plain section outside the instance, with a heading we can try to move in.
let outside = '';
h!.act(() => {
const tree = buildNodeTree(h!.query, { type: { resolvedName: 'Section' }, props: {}, nodes: [{ type: { resolvedName: 'Heading' }, props: { text: 'Outsider' } }] });
h!.actions.addNodeTree(tree, 'ROOT');
outside = tree.rootNodeId;
});
const nodes = live();
const [hid] = byType(nodes, 'Heading', sec);
const inner = nodes[hid].parent as string; // the instance's inner canvas
const [outsider] = byType(nodes, 'Heading', outside);
return { sec, libraryId, hid, inner, outsider };
};
const subtreeSize = (sec: string) => Object.keys(live()).filter((id) => { for (let p: string | null = id; p; p = live()[p]?.parent ?? null) if (p === sec) return true; return false; }).length;
test('canvas/layers DnD: dropping or moving a node into an instance is refused; reordering inside it too', () => {
const { sec, inner, hid, outsider } = setup();
const size = subtreeSize(sec);
expect(h!.query.node(inner).isDroppable(outsider, () => {})).toBe(false);
expect(moveLayer(h!.query, h!.actions, outsider, inner, 0).ok).toBe(false);
expect(() => h!.act(() => { h!.actions.move(outsider, inner, 0); })).toThrow();
// reorder inside the instance
expect(moveLayer(h!.query, h!.actions, hid, inner, 3).ok).toBe(false);
// moving an inner node OUT of the instance is refused as well
expect(moveLayer(h!.query, h!.actions, hid, 'ROOT', 0).ok).toBe(false);
expect(subtreeSize(sec)).toBe(size);
// the instance root itself still moves as a whole
expect(moveLayer(h!.query, h!.actions, sec, 'ROOT', 1).ok).toBe(true);
});
test('BlocksPanel / EmptyContainerAdd insert: an explicit parent inside an instance is refused; "after selection" lands after the instance', () => {
const { sec, inner, hid } = setup();
const size = subtreeSize(sec);
let r: string | null = 'x';
h!.act(() => { r = insertBlock(h!.query, h!.actions, 'text', { parentId: inner }); });
expect(r).toBeNull();
expect(subtreeSize(sec)).toBe(size);
h!.act(() => { h!.actions.selectNode(hid); });
h!.act(() => { r = insertBlock(h!.query, h!.actions, 'text', { afterSelection: true }); });
expect(r).toBeTruthy();
expect(subtreeSize(sec)).toBe(size);
const rootKids = live().ROOT.nodes;
expect(rootKids[rootKids.indexOf(sec) + 1]).toBe(r);
});
test('paste and duplicate never add nodes inside an instance', () => {
const { sec, hid, outsider } = setup();
const size = subtreeSize(sec);
setClipboardTree(h!.query.node(outsider).toNodeTree());
h!.act(() => { cmd('edit.paste').run(ctx([hid])); });
expect(subtreeSize(sec)).toBe(size);
const rootKids = live().ROOT.nodes;
expect(live()[rootKids[rootKids.indexOf(sec) + 1]].type.resolvedName).toBe('Heading');
h!.act(() => { cmd('edit.duplicate').run(ctx([hid])); });
expect(subtreeSize(sec)).toBe(size);
setClipboardTree(null);
});
test('Sitesmith section replace aimed inside an instance appends instead of restructuring it', async () => {
const { sec, hid } = setup();
const size = subtreeSize(sec);
const tree = { type: { resolvedName: 'Section' }, props: {}, nodes: [{ type: { resolvedName: 'Heading' }, props: { text: 'AI section' } }] } as any;
let r: any;
await h!.act(async () => { r = await applyAi({ type: 'replace', scope: 'section', pages: [{ name: 'x', tree }] } as any, hid); });
expect(r.ok).toBe(true);
expect(subtreeSize(sec)).toBe(size);
expect(live()[hid]).toBeTruthy();
expect(JSON.stringify(live())).toContain('AI section');
});
test('the empty-container "+" is not offered inside an instance', () => {
mount(<EmptyContainerAdd />);
let secId = '';
h!.act(() => {
const tree = buildNodeTree(h!.query, { type: { resolvedName: 'Section' }, props: {}, nodes: [] });
h!.actions.addNodeTree(tree, 'ROOT');
secId = tree.rootNodeId;
});
const inner = live()[secId].linkedNodes ? Object.values(live()[secId].linkedNodes)[0] as string : '';
expect(inner).toBeTruthy();
h!.act(() => { h!.actions.selectNode(inner); });
expect(document.body.querySelector('.empty-container-add, [aria-label^="Add"]')).not.toBeNull();
h!.act(() => { globals.makeGlobal(secId, 'Empty'); });
h!.act(() => { h!.actions.selectNode(Object.values(live()[secId].linkedNodes)[0] as string); });
expect(document.body.querySelector('.empty-container-add, [aria-label^="Add"]')).toBeNull();
});
test('context-menu duplicate of an inner node is refused; duplicating the instance root is allowed', () => {
const { sec, hid } = setup();
let dup: (() => string | null) | null = null;
let dupRoot: (() => string | null) | null = null;
const Hook: React.FC = () => { dup = useNodeActions(hid).duplicate; dupRoot = useNodeActions(sec).duplicate; return null; };
h!.mountChild(<PageProvider><GlobalSectionsProvider><Probe /><Hook /></GlobalSectionsProvider></PageProvider>);
const size = subtreeSize(sec);
let r: string | null = 'x';
h!.act(() => { r = dup!(); });
expect(r).toBeNull();
expect(subtreeSize(sec)).toBe(size);
h!.act(() => { r = dupRoot!(); });
expect(r).toBeTruthy();
expect(instanceMeta(live()[r!])).toBeTruthy();
});
});
test('remove is refused while instances exist, allowed once none are left', () => {
mount();
const sec = addPromo();
+21 -7
View File
@@ -1,4 +1,5 @@
import { useEditor } from '@craftjs/core';
import { insertTargetOutsideInstance, isInsideInstance } from './global-sections';
import { usePages } from '../state/PageContext';
import { SitesmithResponse, SerializedTreeNode, SitesmithPatchOp } from '../types/sitesmith';
import { sanitizeAiTree, buildNodeTree } from './craft-tree';
@@ -84,7 +85,9 @@ export function useApplyAiResponse() {
if (resp.scope === 'section') {
// When targeted at a specific node, the AI's tree replaces that node
// in place (vs appending a fresh section at the end of ROOT).
if (targetNodeId && resp.pages.length > 0) {
// #57: a target inside a global-section instance can't be restructured
// (the next master sync would wipe it) -> append as a new section instead.
if (targetNodeId && resp.pages.length > 0 && !targetInInstance(query, targetNodeId)) {
try {
const nodeTree = buildNodeTree(query, resp.pages[0].tree);
const parent: string = query.node(targetNodeId).get().data.parent ?? 'ROOT';
@@ -118,6 +121,10 @@ export function useApplyAiResponse() {
};
}
function targetInInstance(query: any, nodeId: string): boolean {
try { return isInsideInstance(query, query.node(nodeId).get()?.data?.parent); } catch { return false; }
}
function applyPatch(
actions: any,
query: any,
@@ -167,8 +174,12 @@ function applyPatch(
case 'replace_node': {
try {
const nodeTree = buildNodeTree(query, op.tree);
const parent: string = query.node(id).get().data.parent ?? 'ROOT';
if (isInsideInstance(query, parent)) {
console.warn('sitesmith patch: replace_node inside a global section instance, skipping', op.node_id);
break;
}
const nodeTree = buildNodeTree(query, op.tree);
const siblings: string[] = query.node(parent).childNodes();
const index = siblings.indexOf(id);
actions.delete(id);
@@ -182,12 +193,15 @@ function applyPatch(
case 'insert_after':
case 'insert_before': {
try {
const nodeTree = buildNodeTree(query, op.tree);
const parent: string = query.node(id).get().data.parent ?? 'ROOT';
const siblings: string[] = query.node(parent).childNodes();
const parent0: string = query.node(id).get().data.parent ?? 'ROOT';
const siblings: string[] = query.node(parent0).childNodes();
const index = siblings.indexOf(id);
const at = op.op === 'insert_after' ? index + 1 : index;
actions.addNodeTree(nodeTree, parent, at);
// #57: next to a node inside an instance -> next to the instance.
const at = insertTargetOutsideInstance(query, parent0, op.op === 'insert_after' ? index + 1 : index);
if (!at) break;
const nodeTree = buildNodeTree(query, op.tree);
if (at.index !== undefined) actions.addNodeTree(nodeTree, at.parentId, at.index);
else actions.addNodeTree(nodeTree, at.parentId);
} catch (e) {
console.warn(`sitesmith patch: ${op.op} failed`, e);
}
+12 -1
View File
@@ -311,7 +311,9 @@ export function buildNodeTree(query: any, tree: SerializedTreeNode): NodeTree {
id,
data: { ...flatNode, type: resolvedComponent, name: flatNode.type.resolvedName },
events: { selected: false, hovered: false, dragged: false },
rules: { canDrag: () => true, canMoveIn: () => true, canMoveOut: () => true, canDrop: () => true },
// The component's own rules (incl. the #57 instance guard installed in
// resolver.ts), as parseFreshNode would give a non-synthetic node.
rules: { canDrag: () => true, canMoveIn: () => true, canMoveOut: () => true, canDrop: () => true, ...craftRulesOf(resolvedComponent) },
};
} else {
craftNodes[id] = (query.parseFreshNode({
@@ -324,6 +326,15 @@ export function buildNodeTree(query: any, tree: SerializedTreeNode): NodeTree {
return { rootNodeId: flat.rootNodeId, nodes: craftNodes };
}
const RULE_KEYS = ['canDrag', 'canDrop', 'canMoveIn', 'canMoveOut'] as const;
/** The four Craft rule functions a component declares (Craft copies exactly these onto a node). */
function craftRulesOf(component: unknown): Record<string, unknown> {
const rules = (component as { craft?: { rules?: Record<string, unknown> } } | undefined)?.craft?.rules;
const out: Record<string, unknown> = {};
if (rules) for (const k of RULE_KEYS) if (typeof rules[k] === 'function') out[k] = rules[k];
return out;
}
/**
* Returns a deep-cloned NodeTree where every node id in the tree
* (rootNodeId, the `nodes` map keys, each `node.id`, each internal
+51
View File
@@ -236,6 +236,57 @@ export function findInstanceRoot(query: any, nodeId: string | null | undefined):
return null;
}
/**
* #57 finding 4: an instance's structure belongs to its master. Its subtree is
* rebuilt from the master on every sync, so anything added, moved or
* reordered inside it would silently vanish on the next master edit. All
* structural edits into/inside/out of an instance are therefore BLOCKED
* (content props stay editable as overrides; the instance root itself moves
* as a whole).
*/
export const isInsideInstance = (query: any, nodeId: string | null | undefined): boolean =>
findInstanceRoot(query, nodeId) !== null;
/**
* Where an insert aimed at `parentId`/`index` may land: unchanged when the
* parent is outside every instance, otherwise right after the OUTERMOST
* enclosing instance root (so "insert after the selection" next to a node
* inside an instance lands next to the instance). Null when there is no such
* place.
*/
export function insertTargetOutsideInstance(
query: any, parentId: string, index?: number,
): { parentId: string; index?: number } | null {
let target = { parentId, index };
let guard = 0;
for (let inst = findInstanceRoot(query, target.parentId); inst && guard++ < 50; inst = findInstanceRoot(query, target.parentId)) {
let p: string | null = null;
try { p = query.node(inst).get()?.data?.parent ?? null; } catch { return null; }
if (!p) return null;
let i = -1;
try { i = (query.node(p).get()?.data?.nodes || []).indexOf(inst); } catch { /* append */ }
target = { parentId: p, index: i === -1 ? undefined : i + 1 };
}
return target;
}
/**
* Craft rule guard (installed on every resolver component, see resolver.ts):
* `self` -- the canvas a node would move into or out of -- must not be inside
* an instance. `helper` is Craft's `(id) => NodeHelpers`.
*/
export function canvasInInstance(self: any, helper: (id: string) => { get(): any }): boolean {
let n = self;
let guard = 0;
while (n && guard++ < 500) {
if (instanceMeta(n.data)) return true;
const p = n.data?.parent;
if (!p) return false;
try { n = helper(p).get(); } catch { return false; }
}
return false;
}
/** Same as `findInstanceRoot`, over a flat serialized map. */
export function findInstanceRootIn(nodes: SNodes, nodeId: string): string | null {
let id: string | null = nodeId;
+5
View File
@@ -1,4 +1,5 @@
import type { SitesmithResponse, SerializedTreeNode } from '../types/sitesmith';
import { isInsideInstance } from './global-sections';
import { buildNodeTree } from './craft-tree';
/**
@@ -136,6 +137,10 @@ export function applyRegeneratedSection(
} catch (e) {
return { ok: false, message: 'That section no longer exists.' };
}
// #57: a node inside a global-section instance is rebuilt from its master on every sync.
if (isInsideInstance(query, parent)) {
return { ok: false, message: 'This is part of a global section. Edit the master (or detach it) to change its layout.' };
}
actions.delete(targetId);
try {
actions.history.merge().addNodeTree(nodeTree, parent, index);