fix(#337): Fix boolean logic bug in ReactFlowEditor (use || instead of ??)
- Nullish coalescing (??) doesn't work with booleans as expected - When readOnly=false, ?? never evaluates right side (!selectedNode) - Changed to logical OR (||) for correct disabled state calculation - Added comprehensive tests verifying the fix: * readOnly=false with no selection: editing disabled * readOnly=false with selection: editing enabled * readOnly=true: editing always disabled - Removed unused eslint-disable directive Refs #337 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Jason Woltje
270
apps/web/src/components/mindmap/ReactFlowEditor.test.tsx
Normal file
270
apps/web/src/components/mindmap/ReactFlowEditor.test.tsx
Normal file
@@ -0,0 +1,270 @@
|
||||
/**
|
||||
* ReactFlowEditor Tests
|
||||
* Tests for the boolean logic in handleDeleteSelected
|
||||
* - When readOnly=false AND selectedNode=null, editing should be disabled
|
||||
* - When readOnly=false AND selectedNode exists, editing should be enabled
|
||||
* - When readOnly=true, editing should always be disabled
|
||||
*/
|
||||
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
import { render, screen, fireEvent } from "@testing-library/react";
|
||||
import { ReactFlowEditor } from "./ReactFlowEditor";
|
||||
import type { GraphData } from "./hooks/useGraphData";
|
||||
|
||||
// Mock ReactFlow since it requires DOM APIs not available in test environment
|
||||
vi.mock("@xyflow/react", () => ({
|
||||
ReactFlow: ({
|
||||
nodes,
|
||||
edges,
|
||||
children,
|
||||
onNodeClick,
|
||||
onPaneClick,
|
||||
}: {
|
||||
nodes: unknown[];
|
||||
edges: unknown[];
|
||||
children: React.ReactNode;
|
||||
onNodeClick?: (event: React.MouseEvent, node: { id: string }) => void;
|
||||
onPaneClick?: () => void;
|
||||
}): React.JSX.Element => (
|
||||
<div data-testid="react-flow">
|
||||
<div data-testid="node-count">{nodes.length}</div>
|
||||
<div data-testid="edge-count">{edges.length}</div>
|
||||
{/* Simulate node click for testing */}
|
||||
<button
|
||||
data-testid="mock-node-click"
|
||||
onClick={(e): void => {
|
||||
if (onNodeClick) {
|
||||
onNodeClick(e as unknown as React.MouseEvent, { id: "node-1" });
|
||||
}
|
||||
}}
|
||||
>
|
||||
Click Node
|
||||
</button>
|
||||
{/* Simulate pane click for deselection */}
|
||||
<button
|
||||
data-testid="mock-pane-click"
|
||||
onClick={(): void => {
|
||||
if (onPaneClick) {
|
||||
onPaneClick();
|
||||
}
|
||||
}}
|
||||
>
|
||||
Click Pane
|
||||
</button>
|
||||
{children}
|
||||
</div>
|
||||
),
|
||||
Background: (): React.JSX.Element => <div data-testid="background" />,
|
||||
Controls: (): React.JSX.Element => <div data-testid="controls" />,
|
||||
MiniMap: (): React.JSX.Element => <div data-testid="minimap" />,
|
||||
Panel: ({
|
||||
children,
|
||||
position,
|
||||
}: {
|
||||
children: React.ReactNode;
|
||||
position: string;
|
||||
}): React.JSX.Element => <div data-testid={`panel-${position}`}>{children}</div>,
|
||||
useNodesState: (initial: unknown[]): [unknown[], () => void, () => void] => [
|
||||
initial,
|
||||
vi.fn(),
|
||||
vi.fn(),
|
||||
],
|
||||
useEdgesState: (initial: unknown[]): [unknown[], () => void, () => void] => [
|
||||
initial,
|
||||
vi.fn(),
|
||||
vi.fn(),
|
||||
],
|
||||
addEdge: vi.fn(),
|
||||
MarkerType: { ArrowClosed: "arrowclosed" },
|
||||
BackgroundVariant: { Dots: "dots" },
|
||||
}));
|
||||
|
||||
const mockGraphData: GraphData = {
|
||||
nodes: [
|
||||
{
|
||||
id: "node-1",
|
||||
title: "Test Node 1",
|
||||
content: "Content 1",
|
||||
node_type: "concept",
|
||||
tags: ["test"],
|
||||
domain: "test",
|
||||
metadata: {},
|
||||
created_at: new Date().toISOString(),
|
||||
updated_at: new Date().toISOString(),
|
||||
},
|
||||
{
|
||||
id: "node-2",
|
||||
title: "Test Node 2",
|
||||
content: "Content 2",
|
||||
node_type: "task",
|
||||
tags: ["test"],
|
||||
domain: "test",
|
||||
metadata: {},
|
||||
created_at: new Date().toISOString(),
|
||||
updated_at: new Date().toISOString(),
|
||||
},
|
||||
],
|
||||
edges: [
|
||||
{
|
||||
source_id: "node-1",
|
||||
target_id: "node-2",
|
||||
relation_type: "relates_to",
|
||||
weight: 1.0,
|
||||
metadata: {},
|
||||
created_at: new Date().toISOString(),
|
||||
},
|
||||
],
|
||||
};
|
||||
|
||||
describe("ReactFlowEditor", (): void => {
|
||||
beforeEach((): void => {
|
||||
vi.clearAllMocks();
|
||||
});
|
||||
|
||||
describe("rendering", (): void => {
|
||||
it("should render the graph with nodes and edges", (): void => {
|
||||
render(<ReactFlowEditor graphData={mockGraphData} />);
|
||||
|
||||
expect(screen.getByTestId("react-flow")).toBeInTheDocument();
|
||||
expect(screen.getByTestId("node-count")).toHaveTextContent("2");
|
||||
expect(screen.getByTestId("edge-count")).toHaveTextContent("1");
|
||||
});
|
||||
|
||||
it("should render controls and minimap", (): void => {
|
||||
render(<ReactFlowEditor graphData={mockGraphData} />);
|
||||
|
||||
expect(screen.getByTestId("controls")).toBeInTheDocument();
|
||||
expect(screen.getByTestId("minimap")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should display node and edge counts in panel", (): void => {
|
||||
render(<ReactFlowEditor graphData={mockGraphData} />);
|
||||
|
||||
expect(screen.getByText("2 nodes, 1 edges")).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
||||
describe("handleDeleteSelected boolean logic (CQ-WEB-5 fix)", (): void => {
|
||||
it("should NOT show delete button when readOnly=false AND no node is selected", (): void => {
|
||||
// This tests the core bug fix: when readOnly=false but selectedNode=null,
|
||||
// the delete button should NOT appear because there's nothing to delete.
|
||||
// The bug was using ?? instead of || which would fail this case.
|
||||
render(<ReactFlowEditor graphData={mockGraphData} readOnly={false} />);
|
||||
|
||||
// No node selected initially, delete button should not appear
|
||||
expect(screen.queryByRole("button", { name: /delete node/i })).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should show delete button when readOnly=false AND a node is selected", (): void => {
|
||||
render(<ReactFlowEditor graphData={mockGraphData} readOnly={false} />);
|
||||
|
||||
// Initially no delete button
|
||||
expect(screen.queryByRole("button", { name: /delete node/i })).not.toBeInTheDocument();
|
||||
|
||||
// Select a node
|
||||
fireEvent.click(screen.getByTestId("mock-node-click"));
|
||||
|
||||
// Now delete button should appear
|
||||
expect(screen.getByRole("button", { name: /delete node/i })).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should NOT show delete button when readOnly=true even with a node selected", (): void => {
|
||||
render(<ReactFlowEditor graphData={mockGraphData} readOnly={true} />);
|
||||
|
||||
// Select a node
|
||||
fireEvent.click(screen.getByTestId("mock-node-click"));
|
||||
|
||||
// Delete button should NOT appear in readOnly mode
|
||||
expect(screen.queryByRole("button", { name: /delete node/i })).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should hide delete button when node is deselected", (): void => {
|
||||
render(<ReactFlowEditor graphData={mockGraphData} readOnly={false} />);
|
||||
|
||||
// Select a node
|
||||
fireEvent.click(screen.getByTestId("mock-node-click"));
|
||||
expect(screen.getByRole("button", { name: /delete node/i })).toBeInTheDocument();
|
||||
|
||||
// Click on pane to deselect
|
||||
fireEvent.click(screen.getByTestId("mock-pane-click"));
|
||||
|
||||
// Delete button should disappear
|
||||
expect(screen.queryByRole("button", { name: /delete node/i })).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should call onNodeDelete when delete button is clicked with valid selection", (): void => {
|
||||
const onNodeDelete = vi.fn();
|
||||
render(
|
||||
<ReactFlowEditor graphData={mockGraphData} readOnly={false} onNodeDelete={onNodeDelete} />
|
||||
);
|
||||
|
||||
// Select a node
|
||||
fireEvent.click(screen.getByTestId("mock-node-click"));
|
||||
|
||||
// Click delete button
|
||||
fireEvent.click(screen.getByRole("button", { name: /delete node/i }));
|
||||
|
||||
// onNodeDelete should be called with the node id
|
||||
expect(onNodeDelete).toHaveBeenCalledWith("node-1");
|
||||
});
|
||||
|
||||
it("should NOT call onNodeDelete in readOnly mode even if somehow triggered", (): void => {
|
||||
// This tests that the handleDeleteSelected function early-returns
|
||||
// when readOnly is true, providing defense in depth
|
||||
const onNodeDelete = vi.fn();
|
||||
render(
|
||||
<ReactFlowEditor graphData={mockGraphData} readOnly={true} onNodeDelete={onNodeDelete} />
|
||||
);
|
||||
|
||||
// Even if we try to select a node, readOnly should prevent deletion
|
||||
fireEvent.click(screen.getByTestId("mock-node-click"));
|
||||
|
||||
// No delete button should exist
|
||||
expect(screen.queryByRole("button", { name: /delete node/i })).not.toBeInTheDocument();
|
||||
|
||||
// And the callback should never have been called
|
||||
expect(onNodeDelete).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("node selection", (): void => {
|
||||
it("should call onNodeSelect when a node is clicked", (): void => {
|
||||
const onNodeSelect = vi.fn();
|
||||
render(<ReactFlowEditor graphData={mockGraphData} onNodeSelect={onNodeSelect} />);
|
||||
|
||||
fireEvent.click(screen.getByTestId("mock-node-click"));
|
||||
|
||||
expect(onNodeSelect).toHaveBeenCalledWith(mockGraphData.nodes[0]);
|
||||
});
|
||||
|
||||
it("should call onNodeSelect with null when pane is clicked", (): void => {
|
||||
const onNodeSelect = vi.fn();
|
||||
render(<ReactFlowEditor graphData={mockGraphData} onNodeSelect={onNodeSelect} />);
|
||||
|
||||
// First select a node
|
||||
fireEvent.click(screen.getByTestId("mock-node-click"));
|
||||
expect(onNodeSelect).toHaveBeenCalledWith(mockGraphData.nodes[0]);
|
||||
|
||||
// Then click on pane to deselect
|
||||
fireEvent.click(screen.getByTestId("mock-pane-click"));
|
||||
expect(onNodeSelect).toHaveBeenLastCalledWith(null);
|
||||
});
|
||||
});
|
||||
|
||||
describe("readOnly mode", (): void => {
|
||||
it("should hide interactive controls when readOnly is true", (): void => {
|
||||
render(<ReactFlowEditor graphData={mockGraphData} readOnly={true} />);
|
||||
|
||||
// The delete panel should not appear even after clicking
|
||||
fireEvent.click(screen.getByTestId("mock-node-click"));
|
||||
expect(screen.queryByText("Delete Node")).not.toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("should show interactive controls when readOnly is false and node is selected", (): void => {
|
||||
render(<ReactFlowEditor graphData={mockGraphData} readOnly={false} />);
|
||||
|
||||
fireEvent.click(screen.getByTestId("mock-node-click"));
|
||||
expect(screen.getByRole("button", { name: /delete node/i })).toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -1,4 +1,3 @@
|
||||
/* eslint-disable @typescript-eslint/no-unnecessary-condition */
|
||||
"use client";
|
||||
|
||||
import { useCallback, useEffect, useMemo, useState } from "react";
|
||||
@@ -211,7 +210,7 @@ export function ReactFlowEditor({
|
||||
);
|
||||
|
||||
const handleDeleteSelected = useCallback(() => {
|
||||
if (readOnly ?? !selectedNode) return;
|
||||
if (readOnly || !selectedNode) return;
|
||||
|
||||
if (onNodeDelete) {
|
||||
onNodeDelete(selectedNode);
|
||||
|
||||
Reference in New Issue
Block a user