Code Review Phase 10: Canvas System — The Heart of the App

[jitu5]

Code Review Phase 10: Canvas System — The Heart of the App

Purpose: Review the visual pipeline canvas — custom ReactFlow nodes, edges, and the 9 canvas hooks that orchestrate drag, drop, connect, select, copy/paste, and keyboard shortcuts.

Prerequisites: Phases 2-4, 8 (Constants, Domain, Redux, UI Primitives)
Estimated time: 2.5 hours (largest phase — reviewers should be well-prepared by now)
Files: 29

---

Canvas Component Hierarchy

  PipelineCanvas (orchestrator)
  ├── ReactFlow (library wrapper)
  │   ├── CustomNode ──────── Kedro function node (memo)
  │   │   └── Handle (in/out connection points)
  │   ├── DatasetNode ─────── Kedro dataset node (memo)
  │   │   └── Handle (in/out connection points)
  │   └── CustomEdge ──────── Bezier connection line (memo)
  ├── CanvasOverlay
  │   ├── EmptyState ──────── "Drop components here"
  │   └── BulkActionsToolbar ─ Multi-select actions
  ├── CanvasControls ──────── Zoom, fit-view, minimap
  ├── EdgeContextMenu ─────── Right-click on edges
  └── GhostPreview ────────── Preview during connection drag

  9 Canvas Hooks (all used by PipelineCanvas):
  ┌──────────────────────────┐
  │ useCanvasState           │ Redux ──▶ ReactFlow format
  │ useNodeHandlers          │ Drop, click, delete
  │ useConnectionHandlers    │ Connect, validate, cycle check
  │ useSelectionHandlers     │ Multi-select, bulk ops
  │ useCanvasKeyboardShorts  │ Del, Cmd+C/V, Escape, Space
  │ useCopyPaste             │ Clone with new IDs
  │ useDragToCreate          │ Drag connection → new node
  │ useGhostPreview          │ Visual drag feedback
  │ useDeleteConfirmation    │ Confirm before bulk delete
  └──────────────────────────┘

---

Files to Review (in order)

Canvas Hooks (read these first — they contain the logic)

• src/components/Canvas/hooks/useCanvasState.ts — Converts Redux state to ReactFlow nodes/edges
• src/components/Canvas/hooks/utils/cycleDetection.ts — Real-time cycle detection during connection drag
• src/components/Canvas/hooks/useConnectionHandlers.ts — Connection creation with bipartite constraint and cycle detection
• src/components/Canvas/hooks/useGhostPreview.ts — Ghost preview during connection drag
• src/components/Canvas/hooks/useNodeHandlers.ts — Node creation from palette, position sync, deletion
• src/components/Canvas/hooks/useDragToCreate.ts — Drag connection to empty space → create new component
• src/components/Canvas/hooks/useSelectionHandlers.ts — Multi-select, bulk actions, focus-on-node events
• src/components/Canvas/hooks/useDeleteConfirmation.ts — Delete confirmation state
• src/components/Canvas/hooks/useCopyPaste.ts — Copy/paste with ID regeneration
• src/components/Canvas/hooks/useCanvasKeyboardShortcuts.ts — Keyboard bindings (Del, Cmd+C/V, Escape, Space)

Custom ReactFlow Components

• src/components/Canvas/CustomNode/CustomNode.tsx + .scss
• src/components/Canvas/DatasetNode/DatasetNode.tsx + .scss
• src/components/Canvas/CustomEdge/CustomEdge.tsx + .scss

Canvas Chrome (UI around the canvas)

• src/components/Canvas/CanvasControls/CanvasControls.tsx + .scss
• src/components/Canvas/CanvasOverlay/CanvasOverlay.tsx
• src/components/Canvas/BulkActionsToolbar/BulkActionsToolbar.tsx + .scss
• src/components/Canvas/EdgeContextMenu/EdgeContextMenu.tsx + .scss
• src/components/Canvas/EmptyState/EmptyState.tsx + .scss
• src/components/Canvas/GhostPreview/GhostPreview.tsx + .scss

Orchestrator (read last — ties everything together)

• src/components/Canvas/PipelineCanvas.tsx + .scss

Tests

• src/components/Canvas/CustomNode/CustomNode.test.tsx — 3 tests: renders name, 'Unnamed Node' fallback, error CSS class
• src/components/Canvas/DatasetNode/DatasetNode.test.tsx — 3 tests: renders name + extension, 'Unnamed Dataset' fallback, error CSS class
• src/components/Canvas/hooks/useConnectionHandlers.test.tsx — 2 tests: API shape, isValidConnection rejects node-to-node
• src/components/Canvas/hooks/useSelectionHandlers.test.tsx — 2 tests: API shape (9 handlers), handlePaneClick clears selection
• src/components/Canvas/hooks/useDragToCreate.test.tsx — 2 tests: API shape, handleConnectStart tracks source node ID

---

Key Concepts

• nodeTypes and edgeTypes must be declared OUTSIDE the component to prevent recreation on every render (critical ReactFlow optimization)
• useCanvasState converts the normalized Redux state (byId/allIds) into the flat array format ReactFlow requires
• useConnectionHandlers enforces the bipartite graph constraint — connections can only go node↔dataset
• useCopyPaste must regenerate IDs for pasted nodes/datasets using generateCopyId to avoid collisions

---

Focus Areas

• Does useCanvasState correctly memoize the Redux → ReactFlow conversion? Known issue: useLayoutEffect replaces the nodes/edges arrays on every relevant Redux change, which defeats React.memo() on CustomNode/DatasetNode. This is a known architectural issue tracked for future optimization (ADR-002).
• Does useConnectionHandlers properly prevent node-to-node and dataset-to-dataset connections? It now uses useAppSelector for connection and node state (no longer uses store.getState() anti-pattern).
• Does useCopyPaste correctly regenerate all IDs AND update source/target references in pasted connections?
• Are CustomNode and DatasetNode using React.memo? (They are, but the memo is defeated by array replacement — see known issue above.)
• Does PipelineCanvas pass deleteKeyCode={null} — disabling ReactFlow's built-in delete for custom handling?
• Are keyboard shortcuts checking for input focus to avoid triggering while typing in forms?
• Is the nodeTypes/edgeTypes object declared outside the component?
• useCanvasState no longer carries unused theme from the canvas selector — verify theme-dependent functionality still works via its own selector.

---

Next: Phase 11 (Code Generation & Export)

[ravi-kumar-pilla]

Issue 1 — Escape key closes config panel when pressed inside an input field

File: src/components/Canvas/hooks/useCanvasKeyboardShortcuts.ts

Severity: High — silent data loss (unsaved form state destroyed)

Description:
Every keyboard shortcut handler in this hook (Space, Cmd+A, Cmd+C, Cmd+V, Delete) guards with !isEditableElement(event.target) to prevent firing while the user is typing in an input or textarea. The Escape handler is the only exception — it has no such guard.

// Current — no isEditableElement guard on Escape
if (event.key === 'Escape') {
  clearAllSelections();          // clears all selected nodes/datasets
  dispatch(closeConfigPanel());  // closes the form, discarding unsaved changes
}

A user editing a node name in the config panel who presses Escape (the standard way to cancel an in-progress edit in most editors and browsers) will instead close the entire config panel and lose all unsaved form state. This is a common enough action to affect nearly every user session.

Fix:

if (event.key === 'Escape') {
  if (isEditableElement(event.target)) return; // ← add this guard
  clearAllSelections();
  dispatch(closeConfigPanel());
}

---

Issue 2 — Copy/paste silently drops connections between copied components

File: src/components/Canvas/hooks/useCopyPaste.ts

Severity: High — silent data loss (connections permanently lost on paste)

Description:
The CopiedItems interface only tracks nodes and datasets — connections are never copied:

interface CopiedItems {
  nodes: KedroNode[];
  datasets: KedroDataset[];
  // ← no connections field
}

When a user selects node-A + dataset-B + the edge between them and copies (Cmd+C), handleCopy reads selectedNodeIds and selectedDatasetIds from the store but never reads selectedEdgeIds or queries connectionsSlice. handlePaste then creates new nodes and datasets with offset positions but never dispatches addConnection.

The result: every copy/paste operation that spans connected components silently loses those connections. The user has to manually re-wire their pipeline after each paste, with no warning that connections were dropped.

Fix:

1. Extend CopiedItems to include connections:

   interface CopiedItems {
     nodes: KedroNode[];
     datasets: KedroDataset[];
     connections: KedroConnection[];
   }

2. In handleCopy, collect connections whose both endpoints are in the copied set:

   const copiedIds = new Set([...nodeIds, ...datasetIds]);
   const connections = allConnections.filter(
     c => copiedIds.has(c.source) && copiedIds.has(c.target)
   );
   setCopiedItems({ nodes, datasets, connections });

3. In handlePaste, re-create connections using the oldId → newId mapping:

   for (const conn of copiedItems.connections) {
     const newSource = idMap[conn.source];
     const newTarget = idMap[conn.target];
     if (newSource && newTarget) {
       dispatch(addConnection({ source: newSource, target: newTarget }));
     }
   }

---

Issue 3 — Ghost preview performs an O(n) DOM query on every mousemove event

File: src/components/Canvas/hooks/useGhostPreview.ts

Severity: High — progressive performance regression that worsens with pipeline size

Description:
useGhostPreview attaches a mousemove listener on window for the duration of a connection drag. On every mouse move event (up to ~60 per second), it queries the entire DOM for all ReactFlow node elements:

// Called on every mousemove during connection drag
const nodeElements = document.querySelectorAll('.react-flow__node');

For a pipeline with N nodes, this is an O(N) DOM traversal executed at animation-frame rate. At 50 nodes this is noticeable lag; at 200+ nodes it causes measurable frame drops during connection drag — one of the most common and interactive operations on the canvas.

Fix:

Cache the node positions at the start of the drag (in handleConnectStart) rather than querying the DOM on every move. Use ReactFlow's getNodes() API to read positions from the already-maintained in-memory state:

// In handleConnectStart, snapshot node positions once
const { getNodes } = useReactFlow();

function handleConnectStart(...) {
  // Snapshot positions into a ref — no DOM query needed
  cachedNodePositionsRef.current = getNodes().map(n => ({
    id: n.id,
    x: n.position.x,
    y: n.position.y,
    width: n.width ?? 0,
    height: n.height ?? 0,
  }));
}

// In mousemove handler, read from the ref instead of the DOM
const nodeElements = cachedNodePositionsRef.current; // O(1) ref read

If live position updates during drag are required, use getNodes() (reads from React state, no DOM traversal) rather than querySelectorAll.

---

Issue 4 — isValidConnection doesn't check for cycles — misleading visual feedback during drag

File: src/components/Canvas/hooks/useConnectionHandlers.ts

Severity: Medium-High — confusing UX; users complete invalid connections based on false visual feedback

Description:
ReactFlow calls isValidConnection during connection drag to determine whether to color the connection line green (valid) or red (invalid). The current implementation only checks the bipartite constraint (node↔dataset only):

const isValidConnection = useCallback((connection: Connection) => {
  const isSourceNode = isNodeId(connection.source ?? '');
  const isTargetNode = isNodeId(connection.target ?? '');
  const isSourceDataset = isDatasetId(connection.source ?? '');
  const isTargetDataset = isDatasetId(connection.target ?? '');
  return (isSourceNode && isTargetDataset) || (isSourceDataset && isTargetNode);
  // ← wouldCreateCycle is never called here
}, []);

Cycle detection only happens in handleConnect (after the user releases the drag):

const handleConnect = useCallback((connection: Connection) => {
  if (wouldCreateCycle(...)) {
    toast.error('Connection would create a cycle');
    return; // too late — user already completed the drag
  }
  ...
}, [...]);

The result: during drag, the connection line turns green (indicating valid), the user releases the mouse, and then a toast error appears telling them it was invalid. The connection is not created, but the user had no indication it would fail. This is especially disorienting because the same error appears for both a genuinely valid connection that was blocked by a Redux error AND for a cycle — the user can't distinguish the cause.

Fix:

Move cycle detection into isValidConnection. The connections, nodes, and datasets are all available in scope via selectors:

const existingConnections = useAppSelector(selectAllConnections);
const allNodeIds = useAppSelector(selectAllNodeIds);
const allDatasetIds = useAppSelector(selectAllDatasetIds);

const isValidConnection = useCallback((connection: Connection) => {
  const { source, target } = connection;
  if (!source || !target) return false;

  // Bipartite constraint
  const bipartiteOk =
    (isNodeId(source) && isDatasetId(target)) ||
    (isDatasetId(source) && isNodeId(target));
  if (!bipartiteOk) return false;

  // Cycle constraint
  const allIds = [...allNodeIds, ...allDatasetIds];
  if (wouldCreateCycle(existingConnections, allIds, source, target)) return false;

  return true;
}, [existingConnections, allNodeIds, allDatasetIds]);

Note: wouldCreateCycle is already imported in this file via cycleDetection.ts. This change makes the visual feedback accurate and removes the need for the cycle check in handleConnect (or it can be kept as a safety net).