fix: address review findings for interactive figure

- Convert to React Flow with one persistent instance and edge labels
- Reset roving focus when focusPath changes to prevent stale tabIndex
- Export NODE_WIDTH/NODE_HEIGHT from layout module to prevent drift
- Change kindLabel to Record<FigureNodeKind, string> for compile safety
- Remove redundant role='button' from native button element
- Remove all unsafe non-null assertions in storyboard-navigation
This commit is contained in:
lda
2026-07-05 18:13:53 +07:00 Verified
parent 91748e1277
commit 196abe4b49
6 changed files with 226 additions and 98 deletions
@@ -1,4 +1,4 @@
import type { FigureNodeDefinition } from "./model.js";
import type { FigureNodeDefinition, FigureNodeKind } from "./model.js";
type FigureNodeViewProps = {
readonly node: FigureNodeDefinition;
@@ -9,7 +9,7 @@ type FigureNodeViewProps = {
readonly onFocus: (nodeId: string) => void;
};
const kindLabel: Record<string, string> = {
const kindLabel: Record<FigureNodeKind, string> = {
actor: "Actor",
operation: "Operation",
artifact: "Artifact",
@@ -39,7 +39,6 @@ export const FigureNodeView = ({
data-active={isActive}
data-expandable={expandable}
data-testid={`figure-node-${node.id}`}
role="button"
aria-label={accessibleName}
tabIndex={focusedNodeId === node.id ? 0 : -1}
onClick={() => {
@@ -1,9 +1,37 @@
import { act, cleanup, render, screen } from "@testing-library/react";
import { cleanup, fireEvent, render, screen } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { afterEach, describe, expect, it, vi } from "vitest";
import { afterEach, beforeAll, afterAll, describe, expect, it, vi } from "vitest";
import type { FigureCatalogDefinition } from "./model.js";
import { InteractiveFigure } from "./InteractiveFigure.js";
class MockResizeObserver {
observe() {}
unobserve() {}
disconnect() {}
}
beforeAll(() => {
globalThis.ResizeObserver = MockResizeObserver as unknown as typeof ResizeObserver;
globalThis.DOMRect = {
fromRect: () => ({
x: 0,
y: 0,
width: 0,
height: 0,
top: 0,
right: 0,
bottom: 0,
left: 0,
toJSON() {},
}),
} as unknown as typeof DOMRect;
});
afterAll(() => {
delete (globalThis as Record<string, unknown>).ResizeObserver;
delete (globalThis as Record<string, unknown>).DOMRect;
});
const validCatalog: FigureCatalogDefinition = {
rootFigureId: "architecture-overview",
figures: [
@@ -16,7 +44,7 @@ const validCatalog: FigureCatalogDefinition = {
{ id: "runtime", label: "Runtime & providers", summary: "WorkflowServer", kind: "runtime", childFigureId: "runtime-detail" },
{ id: "leaf", label: "Leaf node", summary: "Non-expandable", kind: "artifact" },
],
edges: [{ id: "e1", from: "client", to: "runtime" }],
edges: [{ id: "e1", from: "client", to: "runtime", label: "calls" }],
},
{
id: "runtime-detail",
@@ -26,7 +54,7 @@ const validCatalog: FigureCatalogDefinition = {
{ id: "providers", label: "Configured providers", summary: "Built-in and external", kind: "runtime", childFigureId: "provider-detail" },
{ id: "leaf2", label: "Leaf detail", summary: "Static detail", kind: "artifact" },
],
edges: [{ id: "e2", from: "providers", to: "leaf2" }],
edges: [{ id: "e2", from: "providers", to: "leaf2", label: "uses" }],
},
{
id: "provider-detail",
@@ -36,11 +64,13 @@ const validCatalog: FigureCatalogDefinition = {
{ id: "mcp", label: "MCP providers", summary: "Model Context Protocol", kind: "runtime" },
{ id: "python", label: "Python provider", summary: "Trusted in-process", kind: "runtime" },
],
edges: [{ id: "e3", from: "mcp", to: "python" }],
edges: [{ id: "e3", from: "mcp", to: "python", label: "delegates" }],
},
],
};
const figureNode = (id: string) => screen.getByTestId(`figure-node-${id}`);
const renderFigure = (overrides: Partial<React.ComponentProps<typeof InteractiveFigure>> = {}) => {
const onFocusPathChange = overrides.onFocusPathChange ?? vi.fn();
const { onFocusPathChange: _, ...restOverrides } = overrides;
@@ -63,16 +93,24 @@ afterEach(() => cleanup());
describe("InteractiveFigure", () => {
it("renders conceptual labels and hides evidence pointers by default", () => {
renderFigure({ focusPath: [] });
expect(screen.getByRole("button", { name: /runtime & providers.*expand/i })).toBeInTheDocument();
expect(figureNode("runtime")).toBeInTheDocument();
expect(figureNode("runtime")).toHaveTextContent("Runtime & providers");
expect(screen.queryByText(/docs\/source_architecture\.md/i)).not.toBeInTheDocument();
});
it("renders within a React Flow container for edge support", () => {
renderFigure({ focusPath: [] });
const rfWrapper = screen.getByTestId("rf__wrapper");
expect(rfWrapper).toBeInTheDocument();
expect(rfWrapper.querySelector("[class*='react-flow']")).toBeInTheDocument();
});
it("expands a child figure by click and Enter", async () => {
const user = userEvent.setup();
const { onFocusPathChange } = renderFigure({ focusPath: [], onFocusPathChange: vi.fn() });
await user.click(screen.getByRole("button", { name: /runtime & providers/i }));
fireEvent.click(figureNode("runtime"));
expect(onFocusPathChange).toHaveBeenCalledWith(["runtime"]);
screen.getByRole("button", { name: /runtime & providers/i }).focus();
figureNode("runtime").focus();
await user.keyboard("{Enter}");
expect(onFocusPathChange).toHaveBeenLastCalledWith(["runtime"]);
});
@@ -83,7 +121,7 @@ describe("InteractiveFigure", () => {
focusPath: ["runtime", "providers"],
onFocusPathChange: vi.fn(),
});
screen.getByRole("button", { name: /configured providers/i }).focus();
figureNode("mcp").focus();
await user.keyboard("{Escape}");
expect(onFocusPathChange).toHaveBeenCalledWith(["runtime"]);
await user.click(screen.getByRole("button", { name: /architecture/i }));
@@ -104,10 +142,10 @@ describe("InteractiveFigure", () => {
/>
</div>,
);
screen.getByRole("button", { name: /client operations/i }).focus();
figureNode("client").focus();
await user.keyboard("{ArrowDown}");
expect(outerKeyDown).not.toHaveBeenCalled();
expect(screen.getByRole("button", { name: /runtime & providers/i })).toHaveFocus();
expect(figureNode("runtime")).toHaveFocus();
});
it("retains all information when motion is disabled", () => {
@@ -115,4 +153,29 @@ describe("InteractiveFigure", () => {
expect(screen.getByRole("group", { name: /runtime detail/i })).toHaveAttribute("data-motion", "disabled");
expect(screen.getAllByRole("button").length).toBeGreaterThan(1);
});
it("resets roving focus when focusPath changes", () => {
const { rerender } = render(
<InteractiveFigure
catalog={validCatalog}
focusPath={[]}
activeNodeId={null}
onFocusPathChange={vi.fn()}
motionDisabled={false}
/>,
);
const firstFigureId = screen.getByRole("group", { name: /architecture/i }).getAttribute("data-figure-id");
rerender(
<InteractiveFigure
catalog={validCatalog}
focusPath={["runtime"]}
activeNodeId={null}
onFocusPathChange={vi.fn()}
motionDisabled={false}
/>,
);
const secondFigureId = screen.getByRole("group", { name: /runtime detail/i }).getAttribute("data-figure-id");
expect(secondFigureId).not.toBe(firstFigureId);
expect(figureNode("providers")).toBeInTheDocument();
});
});
@@ -1,12 +1,13 @@
import { useCallback, useEffect, useRef, useState, type KeyboardEvent } from "react";
import type { FigureCatalogDefinition } from "./model.js";
import { layoutFigure, type PositionedFigure } from "./layout.js";
import { useCallback, useEffect, useMemo, useRef, useState, type KeyboardEvent } from "react";
import { ReactFlow, type Node, type Edge, type NodeTypes } from "@xyflow/react";
import "@xyflow/react/dist/style.css";
import type { FigureCatalogDefinition, FigureNodeKind } from "./model.js";
import { layoutFigure, NODE_WIDTH, NODE_HEIGHT, type PositionedFigure } from "./layout.js";
import { nextFigureNodeId, type FigureDirection } from "./navigation.js";
import {
popFigureFocus,
pushFigureFocus,
resolveFigureFocus,
type FigureFocus,
} from "./focus.js";
import { FigureBreadcrumbs } from "./FigureBreadcrumbs.js";
import { FigureNodeView } from "./FigureNodeView.js";
@@ -20,6 +21,54 @@ type InteractiveFigureProps = {
readonly motionDisabled: boolean;
};
type FigureNodeData = {
readonly nodeId: string;
readonly label: string;
readonly summary: string;
readonly kind: FigureNodeKind;
readonly isActive: boolean;
readonly isExpandable: boolean;
readonly onActivate: (nodeId: string) => void;
readonly onExpand: (nodeId: string) => void;
};
const FigureFlowNode = ({ data }: { data: FigureNodeData }) => {
const expandable = data.isExpandable;
const accessibleName = expandable ? `${data.label}, expand` : data.label;
return (
<button
type="button"
className="figure-node"
data-figure-node-kind={data.kind}
data-active={data.isActive}
data-expandable={expandable}
data-testid={`figure-node-${data.nodeId}`}
aria-label={accessibleName}
onClick={() => {
data.onActivate(data.nodeId);
if (expandable) data.onExpand(data.nodeId);
}}
onKeyDown={(event) => {
if (event.key === "Enter" && expandable) {
event.preventDefault();
data.onExpand(data.nodeId);
}
}}
>
<span className="figure-node__kind">{data.kind}</span>
<strong className="figure-node__label">{data.label}</strong>
<span className="figure-node__summary">{data.summary}</span>
{expandable && <span className="figure-node__expand-affance" aria-hidden="true">&#9656;</span>}
{data.isActive && <span className="figure-node__current-marker">Current</span>}
</button>
);
};
const nodeTypes: NodeTypes = {
figure: FigureFlowNode,
};
export const InteractiveFigure = ({
catalog,
focusPath,
@@ -32,12 +81,18 @@ export const InteractiveFigure = ({
const [focusedNodeId, setFocusedNodeId] = useState<string>(
activeNodeId ?? focus.figure.nodes[0]?.id ?? "",
);
const groupRef = useRef<HTMLDivElement>(null);
const containerRef = useRef<HTMLDivElement>(null);
const prevFigureIdRef = useState(focus.figure.id);
useEffect(() => {
if (activeNodeId) setFocusedNodeId(activeNodeId);
}, [activeNodeId]);
useEffect(() => {
const firstNode = focus.figure.nodes[0];
if (firstNode) setFocusedNodeId(firstNode.id);
}, [focus.figure.id]);
const handleExpand = useCallback(
(nodeId: string) => {
const next = pushFigureFocus(catalog, focus, nodeId);
@@ -81,79 +136,77 @@ export const InteractiveFigure = ({
event.stopPropagation();
const nextId = nextFigureNodeId(layout, focusedNodeId, direction);
setFocusedNodeId(nextId);
const nextEl = groupRef.current?.querySelector(
const nextNode = containerRef.current?.querySelector(
`[data-testid="figure-node-${nextId}"]`,
) as HTMLElement | null;
nextEl?.focus();
);
if (nextNode instanceof HTMLElement) nextNode.focus();
}
},
[catalog, focus, focusedNodeId, layout, onFocusPathChange],
);
const rfNodes: Node[] = useMemo(
() =>
layout.nodes.map((node) => ({
id: node.id,
type: "figure",
position: node.position,
data: {
nodeId: node.id,
label: node.label,
summary: node.summary,
kind: node.kind,
isActive: node.id === activeNodeId,
isExpandable: node.childFigureId !== undefined,
onActivate: setFocusedNodeId,
onExpand: handleExpand,
},
})),
[layout.nodes, activeNodeId, handleExpand],
);
const rfEdges: Edge[] = useMemo(
() =>
layout.edges.map((edge) => ({
id: edge.id,
source: edge.from,
target: edge.to,
label: edge.label,
type: "default",
})),
[layout.edges],
);
return (
<div
className="interactive-figure"
role="group"
aria-label={focus.figure.title}
data-motion={motionDisabled ? "disabled" : "enabled"}
data-figure-id={focus.figure.id}
onKeyDown={handleKeyDown}
>
<FigureBreadcrumbs
breadcrumbs={focus.breadcrumbs}
onNavigate={handleBreadcrumbNavigate}
/>
<div className="interactive-figure__canvas" ref={groupRef}>
{layout.nodes.map((node) => (
<FigureNodeView
key={node.id}
node={node}
isActive={node.id === activeNodeId}
focusedNodeId={focusedNodeId}
onActivate={() => setFocusedNodeId(node.id)}
onExpand={handleExpand}
onFocus={setFocusedNodeId}
/>
))}
{layout.edges.map((edge) => (
<svg
key={edge.id}
className="interactive-figure__edge"
aria-hidden="true"
>
<line
x1={getNodeCenter(layout, edge.from)?.x ?? 0}
y1={getNodeCenter(layout, edge.from)?.y ?? 0}
x2={getNodeCenter(layout, edge.to)?.x ?? 0}
y2={getNodeCenter(layout, edge.to)?.y ?? 0}
stroke="currentColor"
strokeWidth={1.5}
markerEnd="url(#figure-arrow)"
/>
</svg>
))}
<svg className="interactive-figure__edge-defs" aria-hidden="true">
<defs>
<marker id="figure-arrow" markerWidth="8" markerHeight="6" refX="8" refY="3" orient="auto">
<polygon points="0 0, 8 3, 0 6" />
</marker>
</defs>
</svg>
<div className="interactive-figure__canvas" ref={containerRef}>
<ReactFlow
nodes={rfNodes}
edges={rfEdges}
nodeTypes={nodeTypes}
fitView
proOptions={{ hideAttribution: true }}
nodesDraggable={false}
nodesConnectable={false}
elementsSelectable={false}
panOnDrag={false}
zoomOnScroll={false}
zoomOnPinch={false}
zoomOnDoubleClick={false}
preventScrolling={false}
/>
</div>
</div>
);
};
const NODE_WIDTH = 196;
const NODE_HEIGHT = 84;
const getNodeCenter = (
layout: PositionedFigure,
nodeId: string,
): { x: number; y: number } | undefined => {
const node = layout.nodes.find((n) => n.id === nodeId);
if (!node) return undefined;
return {
x: node.position.x + NODE_WIDTH / 2,
y: node.position.y + NODE_HEIGHT / 2,
};
};
@@ -2,31 +2,36 @@
position: relative;
width: 100%;
height: 100%;
display: flex;
flex-direction: column;
}
.interactive-figure__canvas {
position: relative;
width: 100%;
height: calc(100% - 32px);
flex: 1;
min-height: 0;
}
.interactive-figure__edge {
position: absolute;
inset: 0;
width: 100%;
height: 100%;
pointer-events: none;
color: var(--color-editorial-muted, oklch(0.48 0.025 65));
.interactive-figure .react-flow__node {
cursor: default;
}
.interactive-figure__edge-defs {
position: absolute;
width: 0;
height: 0;
.interactive-figure .react-flow__edge path {
stroke: var(--color-editorial-muted, oklch(0.48 0.025 65));
stroke-width: 1.5;
}
.interactive-figure .react-flow__edge text {
font-family: var(--font-interface, sans-serif);
font-size: 11px;
fill: var(--color-editorial-muted, oklch(0.48 0.025 65));
}
.interactive-figure .react-flow__controls,
.interactive-figure .react-flow__attribution {
display: none;
}
.figure-node {
position: absolute;
display: flex;
flex-direction: column;
align-items: flex-start;
@@ -147,7 +152,7 @@
}
.figure-breadcrumbs__crumb + .figure-breadcrumbs__crumb::before {
content: "";
content: "\203A";
margin-right: 4px;
color: var(--color-editorial-muted, oklch(0.48 0.025 65));
}
@@ -15,8 +15,8 @@ export type PositionedFigure = {
readonly edges: readonly FigureEdgeDefinition[];
};
const NODE_WIDTH = 196;
const NODE_HEIGHT = 84;
export const NODE_WIDTH = 196;
export const NODE_HEIGHT = 84;
const NODESEP = 56;
const RANKSEP = 88;
@@ -35,11 +35,15 @@ export const locationFromHash = (hash: string): PresentationLocation => {
let sceneId: string;
let beatId: string;
let focusPath: string[] = [];
const sceneSegment = sceneMatch[1];
const beatSegment = sceneMatch[2];
const focusSegment = sceneMatch[3];
if (!sceneSegment || !beatSegment) return defaultMainLocation;
try {
sceneId = decodeURIComponent(sceneMatch[1]!);
beatId = decodeURIComponent(sceneMatch[2]!);
if (sceneMatch[3] !== undefined && sceneMatch[3] !== "") {
focusPath = sceneMatch[3]!.split("/").map(decodeURIComponent);
sceneId = decodeURIComponent(sceneSegment);
beatId = decodeURIComponent(beatSegment);
if (focusSegment !== undefined && focusSegment !== "") {
focusPath = focusSegment.split("/").map(decodeURIComponent);
}
} catch {
return defaultMainLocation;
@@ -52,9 +56,11 @@ export const locationFromHash = (hash: string): PresentationLocation => {
}
const discussMatch = raw.match(/^discuss\/(.+)$/);
if (discussMatch) {
const branchSegment = discussMatch[1];
if (!branchSegment) return defaultMainLocation;
let branchId: string;
try {
branchId = decodeURIComponent(discussMatch[1]!);
branchId = decodeURIComponent(branchSegment);
} catch {
return defaultMainLocation;
}
@@ -72,7 +78,8 @@ export const nextMainLocation = (current: MainLocation): MainLocation => {
(loc) => loc.sceneId === current.sceneId && loc.beatId === current.beatId,
);
if (index === -1 || index === locations.length - 1) return current;
return locations[index + 1]!;
const next = locations[index + 1];
return next ?? current;
};
export const previousMainLocation = (current: MainLocation): MainLocation => {
@@ -81,5 +88,6 @@ export const previousMainLocation = (current: MainLocation): MainLocation => {
(loc) => loc.sceneId === current.sceneId && loc.beatId === current.beatId,
);
if (index <= 0) return current;
return locations[index - 1]!;
const prev = locations[index - 1];
return prev ?? current;
};