From 2d1efb1b3ae518cd8a0ab44f546be35c4755316a Mon Sep 17 00:00:00 2001 From: Joey Yakimowich-Payne Date: Tue, 21 Apr 2026 19:46:37 -0600 Subject: [PATCH] fix(chess/ui): visual-builder drag/nesting/editing UX gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses four user-reported gaps in the visual mode authoring surface: 1. × button and expand chevron triggered a drag instead of their own action. dnd-kit listeners were spread on the outer SortableBlockItem wrapper, so any pointerdown on a descendant started a sort. Fixed by routing only `listeners` to a new dedicated grip-handle icon on the card header; the rest of the card (× / expand / inspector / body) no longer competes with drag. `attributes` still go on the wrapper so keyboard drag + screen-reader announcements keep working. 2. No way to add primitives INSIDE a trigger — palette clicks always appended at the top level. Fixed by computing an addTargetInfo when the selected block is a container (has childPrimitives + a params .primitives array): the palette now shows an "Adding inside: {label}" banner with an "Add at top level instead" escape button, and handleAddPrimitive routes the new node into the parent params. The parent auto-expands and selection stays on the parent so repeated palette clicks stack children under it. 3. Inspector was read-only — ParamField.onChange was a documented no-op. Fixed by adding onParamsChange through the wire (VisualBuilderPane.handleParamsChange → BlockList.onParamsChange → SortableBlockItem.onParamsChange → BlockCard.onParamsChange → ParamField.onChange). Editing a number/string/enum field now immediately updates descriptor.primitives[index].params. 4. Nested children could not be removed — nested BlockList was given onRemove={() => {}}. Fixed by adding onNestedRemove to BlockListProps; VisualBuilderPane supplies handleNestedRemove which filters the matching parent params.primitives[] without mutating siblings. Additional polish: - Inspector now opens when a block is selected (previously needed selected AND expanded), so children and docs show up on first click. - Child block list renders whenever the parent is expanded OR selected for the same first-click visibility. - Expand button gains aria-label="Expand|Collapse" so accessibility tooling (and Playwright getByRole) can target it by name. --- .../chess/src/ui/visual-builder/BlockCard.tsx | 82 +++++++++--- .../chess/src/ui/visual-builder/BlockList.tsx | 49 ++++++-- .../ui/visual-builder/VisualBuilderPane.tsx | 119 ++++++++++++++++++ 3 files changed, 228 insertions(+), 22 deletions(-) diff --git a/packages/chess/src/ui/visual-builder/BlockCard.tsx b/packages/chess/src/ui/visual-builder/BlockCard.tsx index 07a9dfd..d486c5a 100644 --- a/packages/chess/src/ui/visual-builder/BlockCard.tsx +++ b/packages/chess/src/ui/visual-builder/BlockCard.tsx @@ -3,6 +3,14 @@ import type { EffectPrimitiveNode, PrimitiveKind } from '../../modifiers/primiti import { PRIMITIVE_REGISTRY } from '../../modifiers/primitives/registry.js'; import { ParamField } from '../ParamField.js'; +/** + * Props that a dnd-kit `useSortable` caller hands to the drag-handle + * button. Intentionally opaque — we spread them onto the grip icon + * without caring about exact shape so dnd-kit's internal event keys + * stay encapsulated. + */ +export type DragHandleProps = Record; + export interface BlockCardProps { node: EffectPrimitiveNode; index: number; @@ -11,8 +19,23 @@ export interface BlockCardProps { onSelect: () => void; onToggleExpand: () => void; onRemove: () => void; + /** + * Called when the inspector edits the primitive's params. When + * omitted, the inspector is read-only. The caller (VisualBuilderPane) + * is responsible for producing a new descriptor with the updated + * params at this block's position in the tree. + */ + onParamsChange?: (params: unknown) => void; depth: number; // 0..3 (visually clamped at 3) childBlocks?: React.ReactNode; + /** + * Props from dnd-kit's `useSortable` that enable drag initiation. + * Passed through to the grip-handle button so ONLY that element + * starts a drag — clicks on the × / expand / body never do. + * Omitted when the card isn't inside a sortable context (e.g. the + * DragOverlay ghost render). + */ + dragHandleProps?: DragHandleProps; } const CATEGORIES: Record = { @@ -61,8 +84,10 @@ export default function BlockCard({ onSelect, onToggleExpand, onRemove, + onParamsChange, depth, - childBlocks + childBlocks, + dragHandleProps, }: BlockCardProps) { const primitive = PRIMITIVE_REGISTRY.get(node.kind); const label = primitive?.label ?? node.kind; @@ -137,6 +162,32 @@ export default function BlockCard({ {/* Header Row */}
+ {dragHandleProps !== undefined && ( + + )}
- {/* Inspector Overlay (When selected and expanded) */} - {isSelected && isExpanded && primitive && ( -
e.stopPropagation()} // Prevent inspector clicks from triggering card select + {/* Inspector — shown whenever this block is selected so users + can always see/edit params of the primitive they picked. */} + {isSelected && primitive && ( +
e.stopPropagation()} > { - // The visual builder itself will handle the node updates, BlockCard just renders the inspector - // Since this is read-only from the perspective of BlockCard props, we just provide a no-op - // for now until the visual builder state management (T18/T22) handles it. - // Note: For now we'll just not support direct editing via ParamField onChange within BlockCard - // to keep the API surface exactly as requested (pure presentational). + allPrimitives={[node]} + onChange={(params) => { + if (onParamsChange) onParamsChange(params); }} />
)} - {/* Nested Children Area */} - {isExpanded && childBlocks && ( + {/* Nested children — shown when the block is expanded OR selected, + so the user sees the inside of the trigger they just picked + from the palette without needing an extra click. */} + {(isExpanded || isSelected) && childBlocks && (
{childBlocks}
diff --git a/packages/chess/src/ui/visual-builder/BlockList.tsx b/packages/chess/src/ui/visual-builder/BlockList.tsx index 89e492d..afad74e 100644 --- a/packages/chess/src/ui/visual-builder/BlockList.tsx +++ b/packages/chess/src/ui/visual-builder/BlockList.tsx @@ -31,7 +31,16 @@ export interface BlockListProps { onSelect: (index: number) => void; onToggleExpand: (index: number) => void; onRemove: (index: number) => void; + onParamsChange?: (index: number, params: unknown) => void; onNestedReorder?: (parentIndex: number, fromChildIndex: number, toChildIndex: number) => void; + /** + * Called when the user clicks × on a child block inside a trigger. + * `parentIndex` is the child's parent in THIS list; `childIndex` is + * the child's position within `parent.params.primitives`. When + * omitted, the × button on nested blocks is a no-op (present for + * backward compat with existing callers). + */ + onNestedRemove?: (parentIndex: number, childIndex: number) => void; depth?: number; } @@ -44,6 +53,7 @@ interface SortableBlockItemProps { onSelect: () => void; onToggleExpand: () => void; onRemove: () => void; + onParamsChange?: (params: unknown) => void; depth: number; childBlocks?: React.ReactNode; } @@ -66,8 +76,16 @@ function SortableBlockItem(props: SortableBlockItemProps) { zIndex: isDragging ? 1 : 0, }; + // Pointer-event listeners go ONLY to the grip handle rendered by + // BlockCard. `attributes` stay on the wrapper so dnd-kit's keyboard + // and screen-reader integration keeps working (role, tabIndex, + // aria-describedby, aria-pressed), but pointerdown on the card body, + // × button, expand toggle, or inspector fields no longer starts a + // drag. + const dragHandleProps = { ...listeners }; + return ( -
+
); @@ -91,7 +111,9 @@ export function BlockList({ onSelect, onToggleExpand, onRemove, + onParamsChange, onNestedReorder, + onNestedRemove, depth = 0, }: BlockListProps) { const [activeId, setActiveId] = React.useState(null); @@ -183,25 +205,37 @@ export function BlockList({ let childBlocks: React.ReactNode = null; if (hasChildren && isExpanded && typeof node.params === 'object' && node.params !== null && 'primitives' in node.params && Array.isArray((node.params as Record).primitives)) { - // Render nested block list + const childNodes = (node.params as Record).primitives as EffectPrimitiveNode[]; + // Render nested block list. Selection and expansion + // are intentionally scoped to the top level for now — + // multi-level selection would need a richer path-based + // selector than the current flat number. Removal of + // individual children IS supported via onNestedRemove. childBlocks = ( ).primitives as EffectPrimitiveNode[]} - selectedIndex={null} // Nested selection not yet fully scoped, keep null for now or manage differently - expandedIndices={new Set()} // Same for nested expansion + nodes={childNodes} + selectedIndex={null} + expandedIndices={new Set()} onReorder={(fromIdx, toIdx) => { if (onNestedReorder) { onNestedReorder(index, fromIdx, toIdx); } }} - onSelect={() => {}} + onSelect={() => {}} onToggleExpand={() => {}} - onRemove={() => {}} + onRemove={(childIdx) => { + if (onNestedRemove) { + onNestedRemove(index, childIdx); + } + }} depth={depth + 1} /> ); } + const paramsChangeProp = onParamsChange + ? { onParamsChange: (params: unknown) => onParamsChange(index, params) } + : {}; return ( onSelect(index)} onToggleExpand={() => onToggleExpand(index)} onRemove={() => onRemove(index)} + {...paramsChangeProp} depth={depth} childBlocks={childBlocks} /> diff --git a/packages/chess/src/ui/visual-builder/VisualBuilderPane.tsx b/packages/chess/src/ui/visual-builder/VisualBuilderPane.tsx index 406d944..d247ce4 100644 --- a/packages/chess/src/ui/visual-builder/VisualBuilderPane.tsx +++ b/packages/chess/src/ui/visual-builder/VisualBuilderPane.tsx @@ -86,6 +86,35 @@ export function VisualBuilderPane({ descriptor, onChange, validationResult }: Vi const [selectedIndex, setSelectedIndex] = useState(null); const [expandedIndices, setExpandedIndices] = useState>(new Set()); + /** + * If the user has a trigger/container primitive selected (e.g. the + * user just clicked "On Turn End"), a new primitive from the palette + * lands INSIDE that trigger's `params.primitives` — not at the top + * level. Otherwise it's appended to the descriptor root. + * + * Determined by checking whether the selected primitive's registry + * entry exposes `childPrimitives` (all triggers + conditional do). + */ + const addTargetInfo = (() => { + if (selectedIndex === null) return null; + const parent = descriptor.primitives[selectedIndex]; + if (!parent) return null; + const registryEntry = PRIMITIVE_REGISTRY.get(parent.kind); + if (registryEntry?.childPrimitives === undefined) return null; + if ( + typeof parent.params !== 'object' || + parent.params === null || + !('primitives' in parent.params) || + !Array.isArray((parent.params as Record).primitives) + ) { + return null; + } + return { + parentIndex: selectedIndex, + parentLabel: registryEntry.label ?? parent.kind, + }; + })(); + const handleAddPrimitive = (kind: PrimitiveKind) => { const primitive = PRIMITIVE_REGISTRY.get(kind); if (!primitive) return; @@ -95,6 +124,38 @@ export function VisualBuilderPane({ descriptor, onChange, validationResult }: Vi params: generateDefaultParams(primitive.paramsSchema), }; + // Nested add: append to the selected container's params.primitives. + if (addTargetInfo !== null) { + const { parentIndex } = addTargetInfo; + const parent = descriptor.primitives[parentIndex]; + if (!parent) return; + const parentParams = parent.params as Record; + const existingChildren = + (parentParams.primitives as EffectPrimitiveNode[] | undefined) ?? []; + + const newPrimitives = [...descriptor.primitives]; + newPrimitives[parentIndex] = { + ...parent, + params: { + ...parentParams, + primitives: [...existingChildren, newNode], + }, + }; + + onChange({ ...descriptor, primitives: newPrimitives }); + + // Auto-expand the parent so the new child is visible immediately. + setExpandedIndices((prev) => { + const next = new Set(prev); + next.add(parentIndex); + return next; + }); + // Keep selection on the parent so successive palette clicks keep + // adding children — matches how most block editors behave. + return; + } + + // Top-level add. const newPrimitives = [...descriptor.primitives, newNode]; onChange({ ...descriptor, primitives: newPrimitives }); setSelectedIndex(newPrimitives.length - 1); @@ -170,6 +231,40 @@ export function VisualBuilderPane({ descriptor, onChange, validationResult }: Vi onChange({ ...descriptor, primitives: newPrimitives }); }; + const handleNestedRemove = (parentIdx: number, childIdx: number) => { + const parent = descriptor.primitives[parentIdx]; + if ( + !parent || + typeof parent.params !== 'object' || + parent.params === null || + !('primitives' in parent.params) + ) { + return; + } + const childPrimitives = + ((parent.params as Record).primitives as EffectPrimitiveNode[] | undefined) ?? []; + const filtered = childPrimitives.filter((_, i) => i !== childIdx); + + const newPrimitives = [...descriptor.primitives]; + newPrimitives[parentIdx] = { + ...parent, + params: { + ...(parent.params as Record), + primitives: filtered, + }, + }; + + onChange({ ...descriptor, primitives: newPrimitives }); + }; + + const handleParamsChange = (index: number, params: unknown) => { + const target = descriptor.primitives[index]; + if (!target) return; + const newPrimitives = [...descriptor.primitives]; + newPrimitives[index] = { ...target, params }; + onChange({ ...descriptor, primitives: newPrimitives }); + }; + const handleToggleExpand = (index: number) => { const newExpanded = new Set(expandedIndices); if (newExpanded.has(index)) { @@ -239,6 +334,28 @@ export function VisualBuilderPane({ descriptor, onChange, validationResult }: Vi
Primitives
+ + {addTargetInfo !== null && ( +
+
+ Adding inside: +
+
+ {addTargetInfo.parentLabel} +
+ +
+ )} + {allKinds.map(renderPaletteButton)}
@@ -258,7 +375,9 @@ export function VisualBuilderPane({ descriptor, onChange, validationResult }: Vi onSelect={setSelectedIndex} onToggleExpand={handleToggleExpand} onRemove={handleRemove} + onParamsChange={handleParamsChange} onNestedReorder={handleNestedReorder} + onNestedRemove={handleNestedRemove} depth={0} /> )}