cloud-hosting-platform/site-builder
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(builder): duplicate inserts+selects after source; select-parent/styles-sheet/canvas-pad mobile fixes

Browse Source 
Ship-blocking fix (Fable consult): useNodeActions.duplicate() appended the
regenerated tree at the end of the parent while leaving the ORIGINAL
selected, so duplicating a top-level section landed the copy off-screen at
the bottom of the page with no visible change -- shared by both the mobile
selection toolbar and the desktop right-click ContextMenu. Now inserts the
copy immediately after the source (actions.addNodeTree(tree, parentId,
sourceIndex + 1)) and selects it (actions.selectNode(tree.rootNodeId)),
falling back to append-at-end if the source's index can't be resolved.
Mobile also scrolls the new node into view.

Three cheap fast-follows:
- canSelectParent on useNodeActions (false when the node's parent is ROOT
  or missing); MobileSelectionToolbar disables "Select Parent" instead of
  dead-ending on a page-wide ROOT outline with no toolbar of its own.
- Opening the Styles sheet on mobile now scrolls the selected node above
  the 65dvh sheet; a temporary generous bottom-padding class handles the
  case where the node is the last thing on the page and there'd otherwise
  be no room left to scroll it into view.
- Canvas gets bottom padding equal to the fixed selection toolbar's height
  while it's visible, so the last section of a short page isn't stuck
  permanently underneath it.

Desktop duplicate behavior improves (inserts after + selects) via the
shared hook; no toHtml changes. Verified live via Playwright at 375px and
1280px (screenshots in craft/scratchpad/mobileB2/).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Josh Knapp
2026-07-13 08:41:33 -07:00
parent 2c8425ffb0
commit 77f35c4e9e
5 changed files with 202 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
craft/src/hooks/useNodeActions.test.tsx
+31 -5
View File
@@ -14,12 +14,19 @@ import { useNodeActions, type NodeActions } from './useNodeActions';
* - moveUp/moveDown call `actions.move` with the right target index, and
* are no-ops at the respective boundary
* - canMoveUp/canMoveDown reflect the node's position among its siblings
* - duplicate regenerates ids before calling `actions.addNodeTree`
* - duplicate regenerates ids, inserts the copy IMMEDIATELY AFTER the
* source node (`actions.addNodeTree(tree, parentId, sourceIndex + 1)`),
* and selects the new copy (`actions.selectNode(tree.rootNodeId)`) --
* Phase B fast-follow: the previous append-at-end behavior left the copy
* off-screen with the ORIGINAL still selected, so users couldn't see
* what "Duplicate" had just done.
* - selectParent calls `actions.selectNode` with the parent id
* - canSelectParent reflects whether the node's parent is itself real
* (not ROOT/missing) -- false at a top-level section, true one level in
* - deleteNode routes through `findDeletableTarget` (also covering the
* "no deletable target" no-op)
* - ROOT/null nodeId disables every action safely (no-ops, all
* canMoveUp/canMoveDown/canDelete false)
* canMoveUp/canMoveDown/canDelete/canSelectParent false)
*/
const moveMock = vi.fn();
@@ -180,15 +187,24 @@ describe('useNodeActions', () => {
expect(moveMock).not.toHaveBeenCalled();
});
test('duplicate regenerates ids before calling actions.addNodeTree with the parent id', () => {
test('duplicate inserts the copy immediately after the source (parentId, sourceIndex + 1) and selects it', () => {
render('child-1');
act(() => captured!.duplicate());
let returned: string | null | undefined;
act(() => {
returned = captured!.duplicate();
});
expect(regenerateTreeIdsMock).toHaveBeenCalledTimes(1);
expect(addNodeTreeMock).toHaveBeenCalledTimes(1);
const [tree, parentId] = addNodeTreeMock.mock.calls[0];
const [tree, parentId, index] = addNodeTreeMock.mock.calls[0];
// child-1 is at index 1 among ['child-0', 'child-1', 'child-2'] -> insert at 2.
expect(parentId).toBe('parent-1');
expect(index).toBe(2);
expect(tree.rootNodeId).not.toBe(COPIED_TREE.rootNodeId);
expect(selectNodeMock).toHaveBeenCalledTimes(1);
expect(selectNodeMock).toHaveBeenCalledWith(tree.rootNodeId);
expect(returned).toBe(tree.rootNodeId);
});
test('selectParent calls actions.selectNode with the parent id', () => {
@@ -197,6 +213,14 @@ describe('useNodeActions', () => {
expect(selectNodeMock).toHaveBeenCalledWith('parent-1');
});
test('canSelectParent is false at a top-level node (parent is ROOT), true one level deeper', () => {
render('parent-1'); // parent-1's own parent is 'ROOT'
expect(captured!.canSelectParent).toBe(false);
rerender('child-1'); // child-1's parent is 'parent-1', a real node
expect(captured!.canSelectParent).toBe(true);
});
test('deleteNode calls actions.delete with the node id when deletable', () => {
render('child-1');
expect(captured!.canDelete).toBe(true);
@@ -209,6 +233,7 @@ describe('useNodeActions', () => {
expect(captured!.canMoveUp).toBe(false);
expect(captured!.canMoveDown).toBe(false);
expect(captured!.canDelete).toBe(false);
expect(captured!.canSelectParent).toBe(false);
act(() => {
captured!.moveUp();
@@ -228,5 +253,6 @@ describe('useNodeActions', () => {
expect(captured!.canMoveUp).toBe(false);
expect(captured!.canMoveDown).toBe(false);
expect(captured!.canDelete).toBe(false);
expect(captured!.canSelectParent).toBe(false);
});
});
craft/src/hooks/useNodeActions.ts
+58 -6
View File
@@ -5,7 +5,12 @@ import { findDeletableTarget } from '../utils/craft-helpers';
export interface NodeActions {
moveUp: () => void;
moveDown: () => void;
duplicate: () => void;
/** Duplicates `nodeId`, inserting the copy immediately after the source in
* the parent's children and selecting it. Returns the new node's id (so
* callers -- e.g. `MobileSelectionToolbar` -- can scroll it into view),
* or null if the duplicate could not be performed (no-op for ROOT/null,
* or a caught error). */
duplicate: () => string | null;
deleteNode: () => void;
selectParent: () => void;
/** True if `nodeId` has an earlier sibling under the same parent (so
@@ -18,6 +23,12 @@ export interface NodeActions {
* `nodeId` itself, or an ancestor when `nodeId` is an empty linked-node
* slot whose siblings are all also empty -- see `craft-helpers.ts`). */
canDelete: boolean;
/** True if `nodeId` has a real parent to select -- i.e. the parent is
* neither ROOT nor missing. `selectParent` on a top-level section (whose
* parent IS 'ROOT') would only select the page-wide ROOT node, which has
* no on-canvas outline and no toolbar of its own -- a dead end for a
* mobile user with no way back. False for ROOT/null/no-parent too. */
canSelectParent: boolean;
}
/**
@@ -74,11 +85,13 @@ export function useNodeActions(nodeId: string | null | undefined): NodeActions {
let canMoveUp = false;
let canMoveDown = false;
let canSelectParent = false;
if (!isRootOrNull) {
try {
const node = query.node(nodeId).get();
const parentId: string | null | undefined = node?.data?.parent;
if (parentId) {
canSelectParent = parentId !== 'ROOT';
const siblings: string[] = query.node(parentId).get()?.data?.nodes || [];
const idx = siblings.indexOf(nodeId);
canMoveUp = idx > 0;
@@ -102,16 +115,45 @@ export function useNodeActions(nodeId: string | null | undefined): NodeActions {
}
};
const duplicate = () => {
if (!nodeId || nodeId === 'ROOT') return;
const duplicate = (): string | null => {
if (!nodeId || nodeId === 'ROOT') return null;
try {
const parentId = getParentId();
if (!parentId) return;
if (!parentId) return null;
const tree = regenerateTreeIds(query.node(nodeId).toNodeTree());
actions.addNodeTree(tree, parentId);
// Insert the copy IMMEDIATELY AFTER the source node, not appended at
// the end of the parent -- on a top-level section, "append at end"
// landed the copy at the bottom of the page, off-screen, with the
// ORIGINAL still selected, so a duplicate looked like nothing had
// happened at all. Resolve the source's index among its siblings so
// the copy lands right next to what it was copied from; if that can't
// be resolved for any reason, fall back to the old append behavior
// rather than guessing at an index (or throwing).
let insertIndex: number | undefined;
try {
const siblings: string[] = query.node(parentId).get()?.data?.nodes || [];
const sourceIndex = siblings.indexOf(nodeId);
if (sourceIndex !== -1) insertIndex = sourceIndex + 1;
} catch {
// Leave insertIndex undefined -- addNodeTree appends when omitted.
}
if (insertIndex !== undefined) {
actions.addNodeTree(tree, parentId, insertIndex);
} else {
actions.addNodeTree(tree, parentId);
}
// Select the new copy (not the original) so the toolbar/context menu
// and any on-canvas outline immediately reflect what was just created.
actions.selectNode(tree.rootNodeId);
return tree.rootNodeId;
} catch (e) {
console.error('Duplicate failed:', e);
return null;
}
};
@@ -165,5 +207,15 @@ export function useNodeActions(nodeId: string | null | undefined): NodeActions {
}
};
return { moveUp, moveDown, duplicate, deleteNode, selectParent, canMoveUp, canMoveDown, canDelete };
return {
moveUp,
moveDown,
duplicate,
deleteNode,
selectParent,
canMoveUp,
canMoveDown,
canDelete,
canSelectParent,
};
}