Repository navigation
feat: add React Flow Mermaid renderer - #9
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds React Flow-based Mermaid diagram interaction, collaborative node-position persistence through Yjs, identity-based edge mutations, connection-state helpers, updated canvas and session wiring, and expanded browser validation. ChangesReact Flow Integration for Interactive Diagram Layout
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DiagramCanvas
participant ReactFlow
participant SessionWorkspace
participant Yjs
User->>DiagramCanvas: Drag node or edit edge
DiagramCanvas->>ReactFlow: Render interactive graph
ReactFlow->>DiagramCanvas: Emit interaction event
DiagramCanvas->>SessionWorkspace: Report layout or mutation change
SessionWorkspace->>Yjs: Persist collaborative state
Yjs->>SessionWorkspace: Notify synchronized positions
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates @xyflow/react (React Flow) into the diagram canvas component to enable interactive flowchart rendering, node dragging, and layout resets, alongside a new toolbar for adding nodes. The E2E tests have been updated to validate these interactive features. The review feedback highlights a potential crash in getShapeClassName if the shape is undefined, potential React Flow console errors from rendering edges with missing nodes, and UX improvements to keep the new node label input empty by default instead of pre-filling it.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function getShapeClassName(shape: DiagramNodeShape): string { | ||
| return shape.replace(/[^a-z0-9_-]/gi, '').toLowerCase() || 'rect'; | ||
| } |
There was a problem hiding this comment.
The getShapeClassName function assumes that shape is always defined. However, node.shape is optional and can be undefined (as seen in other parts of the codebase using node?.shape ?? 'node'). If shape is undefined, calling shape.replace will throw a TypeError and crash the renderer. We should make the parameter optional and safely default to 'rect'.
| function getShapeClassName(shape: DiagramNodeShape): string { | |
| return shape.replace(/[^a-z0-9_-]/gi, '').toLowerCase() || 'rect'; | |
| } | |
| function getShapeClassName(shape?: DiagramNodeShape): string { | |
| return shape ? shape.replace(/[^a-z0-9_-]/gi, '').toLowerCase() || 'rect' : 'rect'; | |
| } |
| const flowEdges = useMemo<MermaidFlowEdge[]>(() => { | ||
| if (!graph) { | ||
| return []; | ||
| } | ||
|
|
||
| return graph.links.map((link, index) => ({ | ||
| id: `${link.source}-${link.target}-${index}`, | ||
| label: getLinkText(link), | ||
| markerEnd: { color: '#38bdf8', type: MarkerType.ArrowClosed }, | ||
| source: link.source, | ||
| style: { stroke: '#38bdf8', strokeWidth: 1.6 }, | ||
| target: link.target, | ||
| type: 'smoothstep', | ||
| })); | ||
| }, [graph]); |
There was a problem hiding this comment.
The flowEdges memoization maps all links from the graph directly. However, if a node is filtered out or not yet present in interactiveNodeBounds (e.g., during initial load or transitions), React Flow will log console errors about missing source/target nodes. We should filter the edges to ensure both the source and target nodes exist in interactiveNodeBounds.
const flowEdges = useMemo<MermaidFlowEdge[]>(() => {
if (!graph || !interactiveNodeBounds) {
return [];
}
const edges: MermaidFlowEdge[] = [];
graph.links.forEach((link, index) => {
if (interactiveNodeBounds.has(link.source) && interactiveNodeBounds.has(link.target)) {
edges.push({
id: `${link.source}-${link.target}-${index}`,
label: getLinkText(link),
markerEnd: { color: '#38bdf8', type: MarkerType.ArrowClosed },
source: link.source,
style: { stroke: '#38bdf8', strokeWidth: 1.6 },
target: link.target,
type: 'smoothstep',
});
}
});
return edges;
}, [graph, interactiveNodeBounds]);
| const [pendingEdgeLabel, setPendingEdgeLabel] = useState(''); | ||
| const [groupPromptValue, setGroupPromptValue] = useState(''); | ||
| const [showGroupPrompt, setShowGroupPrompt] = useState(false); | ||
| const [newNodeLabel, setNewNodeLabel] = useState(DEFAULT_NEW_NODE_LABEL); |
There was a problem hiding this comment.
Initializing newNodeLabel with DEFAULT_NEW_NODE_LABEL ('New Node') pre-fills the input field in the toolbar, which hides the placeholder and forces the user to manually clear the text before typing. Initializing it to an empty string '' provides a better user experience, as the placeholder 'label' will be visible.
| const [newNodeLabel, setNewNodeLabel] = useState(DEFAULT_NEW_NODE_LABEL); | |
| const [newNodeLabel, setNewNodeLabel] = useState(''); |
| const addNodeFromToolbar = useCallback(() => { | ||
| onAddNode?.(newNodeLabel.trim() || DEFAULT_NEW_NODE_LABEL, newNodeShape); | ||
| setNewNodeLabel(DEFAULT_NEW_NODE_LABEL); | ||
| }, [newNodeLabel, newNodeShape, onAddNode]); |
There was a problem hiding this comment.
Resetting newNodeLabel to DEFAULT_NEW_NODE_LABEL after adding a node pre-fills the input field again. We should reset it to an empty string '' to keep the input clear for the next entry, while still falling back to the default label on submission.
| const addNodeFromToolbar = useCallback(() => { | |
| onAddNode?.(newNodeLabel.trim() || DEFAULT_NEW_NODE_LABEL, newNodeShape); | |
| setNewNodeLabel(DEFAULT_NEW_NODE_LABEL); | |
| }, [newNodeLabel, newNodeShape, onAddNode]); | |
| const addNodeFromToolbar = useCallback(() => { | |
| onAddNode?.(newNodeLabel.trim() || DEFAULT_NEW_NODE_LABEL, newNodeShape); | |
| setNewNodeLabel(''); | |
| }, [newNodeLabel, newNodeShape, onAddNode]); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/components/diagram-canvas.tsx (1)
232-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFit-to-diagram ignores dragged node positions.
graphBoundsis still computed fromhitMap.nodes, so after React Flow drag updates,fitToDiagram()can zoom/pan to stale extents and miss moved nodes.Suggested fix
const graphBounds = useMemo(() => { if (!hitMap) { return null; } + const nodeBounds = interactiveNodeBounds + ? [...interactiveNodeBounds.values()] + : [...hitMap.nodes.values()]; + const allBounds = [ - ...hitMap.nodes.values(), + ...nodeBounds, ...hitMap.subgraphs.values(), ...[...hitMap.edges.values()].map((edge) => edge.bounds), ]; return getBoundsUnion(allBounds); - }, [hitMap]); + }, [hitMap, interactiveNodeBounds]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/diagram-canvas.tsx` around lines 232 - 244, graphBounds is computed only from hitMap.nodes so dragged node positions (updated by React Flow) are ignored; update the useMemo that defines graphBounds to incorporate the current node positions when available by merging hitMap data with the latest nodes from React Flow (e.g., use the nodes array or a getNodes()/nodesRef accessor), replacing bounds for node ids present in the current nodes list before calling getBoundsUnion; reference graphBounds, hitMap.nodes, fitToDiagram and getBoundsUnion so the memo uses up-to-date node positions when computing extents.e2e-validate.ts (1)
35-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid index-based node/overlay pairing in alignment checks.
Line 38 and Line 173 assume SVG node order and overlay DOM order are identical. That ordering can diverge after interaction, causing false alignment failures.
Proposed fix (nearest-neighbor matching instead of index matching)
- const matches: Array<{ nodeId: string; svgCenter: string; overlayCenter: string; offsetPx: number }> = []; - nodes.forEach((n, i) => { - const g = n as SVGGElement; - const overlay = overlays[i] as HTMLElement | undefined; - if (!overlay) return; - - const svgRect = g.getBoundingClientRect(); - const overlayRect = overlay.getBoundingClientRect(); - - const svgCx = Math.round(svgRect.x + svgRect.width / 2); - const svgCy = Math.round(svgRect.y + svgRect.height / 2); - const overlayCx = Math.round(overlayRect.x + overlayRect.width / 2); - const overlayCy = Math.round(overlayRect.y + overlayRect.height / 2); - - matches.push({ - nodeId: g.id, - svgCenter: `${svgCx},${svgCy}`, - overlayCenter: `${overlayCx},${overlayCy}`, - offsetPx: Math.round(Math.sqrt((svgCx - overlayCx) ** 2 + (svgCy - overlayCy) ** 2)), - }); - }); + const nodeCenters = Array.from(nodes).map((n) => { + const r = (n as SVGGElement).getBoundingClientRect(); + return { nodeId: (n as SVGGElement).id, x: r.x + r.width / 2, y: r.y + r.height / 2 }; + }); + const overlayCenters = Array.from(overlays).map((o) => { + const r = (o as HTMLElement).getBoundingClientRect(); + return { x: r.x + r.width / 2, y: r.y + r.height / 2 }; + }); + + const matches = overlayCenters.map((overlay, i) => { + const nearest = Math.min( + ...nodeCenters.map((node) => Math.hypot(node.x - overlay.x, node.y - overlay.y)), + ); + return { overlayIndex: i, offsetPx: Math.round(nearest) }; + });Apply the same pattern to both alignment sections.
Also applies to: 170-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e-validate.ts` around lines 35 - 57, The current code pairs SVG nodes and overlays by array index (using overlays[i] for nodes.forEach), which can be incorrect after DOM reordering; change to nearest-neighbor matching instead: compute center points for all SVG nodes (using svgRect/x/y/width/height and g.id) and for all overlays (using overlayRect), then for each overlay find the closest SVG node by Euclidean distance, pair them, mark that SVG node as used to avoid duplicates, and push the pair into matches with svgCenter, overlayCenter and offsetPx; apply the exact same nearest-neighbor pairing logic to the other alignment block referenced around lines 170-188 so both sections use identical matching instead of index-based matching (update references to overlays, nodes, matches, g.id, svgRect and overlayRect accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/app/globals.css`:
- Around line 637-677: Add a CSS rule for .mermaid-flow-node--cylinder in
globals.css to match the cylinder option used in diagram-canvas.tsx (the
"cylinder" node type referenced around the diagram-canvas component), so it no
longer falls back to a rectangle: give the selector a large horizontal
border-radius (e.g., 999px) and appropriate padding/height, and implement a
::before (and optional ::after) pseudo-element(s) positioned absolutely to
render the rounded top (and bottom) ellipses (matching the visual style of other
mermaid flow-node rules such as .mermaid-flow-node--round and
.mermaid-flow-node--subroutine) so the cylinder shape appears correctly in React
Flow mode.
In `@apps/web/src/components/diagram-canvas.tsx`:
- Around line 284-292: The edges mapping currently forces MarkerType.ArrowClosed
for every edge; instead detect each graph.links[].type (e.g., 'arrow_open',
'arrow_circle', 'arrow_cross') when building flowEdges and set
markerStart/markerEnd to reference custom SVG marker IDs (e.g., markerOpen,
markerCircle, markerCross) rather than a MarkerType enum. Add a <defs> block in
the DiagramCanvas component that defines those three custom <marker> elements
(SVG paths matching the visual styles) and ensure flowEdges uses markerEnd: {
type: 'url(`#markerOpen`)' } or similar URL refs per link.type (and fall back to
ArrowClosed for default cases); update the mapping in the function that returns
edges (the code around graph.links.map) to use link.type to pick the proper
marker id.
In `@e2e-validate.ts`:
- Around line 105-110: Wrap the Reset button click and subsequent boundingBox
check in a try/catch so a missing or timeout on
page.locator('button[aria-label="Reset dragged node layout to Mermaid"]') does
not throw and abort the script; on catch set resetPass = false, push the result
into results (using the existing results.push call), and log the failure message
(console.log) so the test continues to later validations; continue to compute
bounding box only if the click succeeds and beforeDrag/dragTarget are present,
but never rethrow the error.
---
Outside diff comments:
In `@apps/web/src/components/diagram-canvas.tsx`:
- Around line 232-244: graphBounds is computed only from hitMap.nodes so dragged
node positions (updated by React Flow) are ignored; update the useMemo that
defines graphBounds to incorporate the current node positions when available by
merging hitMap data with the latest nodes from React Flow (e.g., use the nodes
array or a getNodes()/nodesRef accessor), replacing bounds for node ids present
in the current nodes list before calling getBoundsUnion; reference graphBounds,
hitMap.nodes, fitToDiagram and getBoundsUnion so the memo uses up-to-date node
positions when computing extents.
In `@e2e-validate.ts`:
- Around line 35-57: The current code pairs SVG nodes and overlays by array
index (using overlays[i] for nodes.forEach), which can be incorrect after DOM
reordering; change to nearest-neighbor matching instead: compute center points
for all SVG nodes (using svgRect/x/y/width/height and g.id) and for all overlays
(using overlayRect), then for each overlay find the closest SVG node by
Euclidean distance, pair them, mark that SVG node as used to avoid duplicates,
and push the pair into matches with svgCenter, overlayCenter and offsetPx; apply
the exact same nearest-neighbor pairing logic to the other alignment block
referenced around lines 170-188 so both sections use identical matching instead
of index-based matching (update references to overlays, nodes, matches, g.id,
svgRect and overlayRect accordingly).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a9b8f93-d634-43c9-b2c6-b96c8dd65ac7
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/web/package.jsonapps/web/src/app/globals.cssapps/web/src/components/diagram-canvas.tsxe2e-validate.ts
Fugu review — changes requestedVerdict: Block.
Concern, not blocking by itself: Bot feedback + learning:
Evidence reviewed: exact head |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/src/components/diagram-canvas.tsx (1)
542-558: 💤 Low valueEffect dependency on
manualNodePositionscauses unnecessary re-runs during drag.Including
manualNodePositionsin the dependency array triggers this effect every time positions change (e.g., during node drag), not just whengraphchanges. The trim logic usessetNodePositions((current) => ...)which already receives the latest value through the callback parameter.Consider removing
manualNodePositionsfrom deps and handling the null-graph case differently:♻️ Proposed refactor
useEffect(() => { if (!graph) { - if (Object.keys(manualNodePositions).length > 0) { - setNodePositions(() => ({})); - } + setNodePositions((current) => Object.keys(current).length > 0 ? {} : current); return; } const currentNodeIds = new Set(graph.nodes.map((node) => node.id)); setNodePositions((current) => { const next = Object.fromEntries( Object.entries(current).filter(([nodeId]) => currentNodeIds.has(nodeId)), ); return Object.keys(next).length === Object.keys(current).length ? current : next; }); -}, [graph, manualNodePositions, setNodePositions]); +}, [graph, setNodePositions]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/components/diagram-canvas.tsx` around lines 542 - 558, The useEffect hook in the diagram-canvas.tsx file is including manualNodePositions in its dependency array, which causes the effect to re-run every time node positions change during drag operations rather than only when the graph structure changes. Remove manualNodePositions from the dependency array of this useEffect, keeping only graph and setNodePositions. The callback pattern in setNodePositions((current) => ...) already provides access to the current positions, so an explicit dependency on manualNodePositions is unnecessary and causes excessive re-runs during interactive drag operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/session-workspace.tsx`:
- Around line 181-184: The positions dictionary is created using empty object
literal syntax `{}` which is vulnerable to prototype pollution attacks since Yjs
keys are collaborator-controlled and malicious keys like `__proto__` could
poison the object's prototype. Replace the object initialization with
`Object.create(null)` when creating the `positions` variable to create a
dictionary without a prototype chain, preventing prototype-key injection and
ensuring membership checks with the `in` operator remain reliable.
---
Nitpick comments:
In `@apps/web/src/components/diagram-canvas.tsx`:
- Around line 542-558: The useEffect hook in the diagram-canvas.tsx file is
including manualNodePositions in its dependency array, which causes the effect
to re-run every time node positions change during drag operations rather than
only when the graph structure changes. Remove manualNodePositions from the
dependency array of this useEffect, keeping only graph and setNodePositions. The
callback pattern in setNodePositions((current) => ...) already provides access
to the current positions, so an explicit dependency on manualNodePositions is
unnecessary and causes excessive re-runs during interactive drag operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36bd3ea3-b73a-4b39-b4d7-17ab79a50aa6
📒 Files selected for processing (4)
apps/web/src/app/globals.cssapps/web/src/components/diagram-canvas.tsxapps/web/src/components/session-workspace.tsxe2e-validate.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/app/globals.css
There was a problem hiding this comment.
Pull request overview
This PR introduces a new React Flow–based renderer for Mermaid flowcharts in the web app, using the existing Mermaid SVG hit-map as the initial layout source, and extends the E2E validation script to cover React Flow interactions.
Changes:
- Add
@xyflow/reactand render flowchart nodes/edges via React Flow while keeping Mermaid as the canonical source of truth. - Persist and sync manual node layout positions through Yjs (
nodePositions) and expose layout reset + fixed add-node toolbar controls. - Extend
e2e-validate.tsto validate empty-canvas building, React Flow semantics, drag/reset, toolbar add, and label-edit sync.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks new React Flow dependency tree (@xyflow/react + transitive deps). |
| apps/web/package.json | Adds @xyflow/react dependency to the web app. |
| apps/web/src/app/globals.css | Imports React Flow base styles and adds styling for React Flow nodes/edges and Mermaid-like node shapes. |
| apps/web/src/components/diagram-canvas.tsx | Implements React Flow rendering layer, node/edge mapping, drag/reset behavior, keyboard interaction, and add-node UI. |
| apps/web/src/components/session-workspace.tsx | Adds Yjs-backed persistence for node positions and wires position/connected-node events to the canvas. |
| e2e-validate.ts | Updates Playwright validation coverage for React Flow mode and adds failure exit semantics. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (1)
e2e-validate.ts:55
- The E2E script uses
Control+Ato select all editor content, which won’t work on macOS (the standard shortcut isMeta/Command+A). Playwright supports a cross-platform shortcut viaControlOrMeta+A, which keeps the script portable across developer machines and CI runners.
console.log('2b. Typing flowchart...');
await editor.click();
await page.keyboard.press('Control+A');
await page.keyboard.type(`flowchart TD
A[Start] --> B{Bug?}
B -->|Yes| C[Fix]
B -->|No| D[Ship]`, { delay: 10 });
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // --- Test: canonical Mermaid edge semantics + React Flow shape fidelity --- | ||
| console.log('\n3b. Checking Mermaid semantics in React Flow mode...'); | ||
| await editor.click(); | ||
| await page.keyboard.press('Control+A'); | ||
| await page.keyboard.type(`flowchart TD | ||
| A[(Database)] --> B{Decision} | ||
| B -.-> C[Retry] | ||
| B ==>|thick| D[Done] | ||
| A --o E((Circle)) | ||
| E --x F[/Trap/]`, { delay: 10 }); |
| const handleAddConnectedNode = useCallback((source: string, label: string, shape: DiagramNodeShape, position: NodePosition, type: DiagramLinkType) => { | ||
| const queue = mutationQueueRef.current; | ||
| if (!queue || !flowchartSnapshot) { | ||
| return; | ||
| } | ||
|
|
||
| const nodeId = ensureUniqueId(flowchartSnapshot.nodeIds, createNodeId(label)); | ||
| void queue.addNode(label, { id: nodeId, shape }) | ||
| .then(() => queue.addEdge(source, nodeId, { type })) | ||
| .then(() => { | ||
| handleNodePositionsChange({ | ||
| ...nodePositions, | ||
| [nodeId]: position, | ||
| }); | ||
| setSelectedNodeIds([nodeId]); | ||
| }); | ||
| }, [flowchartSnapshot, handleNodePositionsChange, nodePositions]); |
| role="button" | ||
| tabIndex={focused ? 0 : -1} | ||
| > |
|
Pushed
Verified locally:
|
|
Pushed
Verification:
Note: exact |
Fugu review gate — blocked as staleVerdict: Block / no approval. This review gate was scoped to exact PR head I am not reviewing or approving a different head under a stale gate. Please create fresh exact-head QA/review/release gates for the current PR head before merge. Bot feedback + learning:
|
Fugu exact-head review — scoped approvalVerdict: Approve, scoped only to exact commit This is not approval for the current live PR head. At review time, PR #9 live head is Findings:
Non-blocking concerns:
Bot feedback + learning:
Evidence reviewed:
|
Fugu exact-head review — request changesVerdict: Block / changes requested for live head I confirmed PR #9 live head is exactly Blocking findings:
Non-blocking concerns:
Evidence reviewed:
Bot feedback + learning:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/lib/diagram-layout.ts (1)
29-45: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrite side skips the validation the read side enforces.
readNodePositionsfilters entries throughisNodePosition(Lines 22-24), butwriteNodePositionswritespositionsentries directly without the same check (Line 43). Invalid coordinates (e.g.NaNfrom a drag miscalculation) would be silently written into the shared Yjs map and then silently dropped again on the nextreadNodePositionscall — leaving inert junk entries in the collaborative document instead of being rejected at the source.♻️ Proposed fix to validate before writing
for (const [nodeId, position] of Object.entries(positions)) { - positionMap.set(nodeId, { x: position.x, y: position.y }); + if (!isNodePosition(position)) { + continue; + } + positionMap.set(nodeId, { x: position.x, y: position.y }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/lib/diagram-layout.ts` around lines 29 - 45, writeNodePositions currently writes every entry directly into the Y.Map without the same validation that readNodePositions applies, so invalid coordinates can be persisted as junk. Update writeNodePositions to validate each candidate position with the same isNodePosition check used by readNodePositions before calling positionMap.set, and skip or reject any invalid node positions (including NaN values) so only valid coordinates are stored.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/components/diagram-canvas.tsx`:
- Around line 218-234: The default full-replace broadcast in setNodePositions is
clearing or overwriting collaborators’ shared layout state. Update the
graph-trim effect and the “Clean layout to Mermaid” action so they do not call
setNodePositions with an unconditional replace of the full map; instead scope
the sync to only the affected node keys or local drag-owned positions, following
the merge-style pattern used by handleFlowNodeDragStop. Keep the local
persistedNodePositionsRef update, but ensure onNodePositionsChange only receives
the minimal delta rather than syncPositions ?? next for these call sites.
- Around line 363-383: The edge index in flowEdges is being taken from the
filtered map position, but onEdgeClick/onEdgeDoubleClick and selectedEdgeIndex
expect the original graph.links index. Update the flowEdges construction in
diagram-canvas.tsx to preserve each link’s original index before filtering, and
use that original index for id, data.index, and selected comparison so
getFlowEdgeIndex/getDiagramEdgeIdentity resolve the correct edge. Keep the
filtering by interactiveNodeBounds, but do not let the filtered array position
drive edge identity or selection.
---
Nitpick comments:
In `@apps/web/src/lib/diagram-layout.ts`:
- Around line 29-45: writeNodePositions currently writes every entry directly
into the Y.Map without the same validation that readNodePositions applies, so
invalid coordinates can be persisted as junk. Update writeNodePositions to
validate each candidate position with the same isNodePosition check used by
readNodePositions before calling positionMap.set, and skip or reject any invalid
node positions (including NaN values) so only valid coordinates are stored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83989a59-dbad-48a6-b06b-d18a4b610463
📒 Files selected for processing (8)
apps/web/src/app/globals.cssapps/web/src/components/diagram-canvas.tsxapps/web/src/components/session-workspace.tsxapps/web/src/lib/diagram-layout.test.tsapps/web/src/lib/diagram-layout.tsapps/web/src/lib/diagram-mutations.test.tsapps/web/src/lib/diagram-mutations.tse2e-validate.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/web/src/components/session-workspace.tsx
- e2e-validate.ts
- apps/web/src/app/globals.css
| const setNodePositions = useCallback(( | ||
| update: (current: DiagramNodePositions) => DiagramNodePositions, | ||
| mode: NodePositionsSyncMode = 'replace', | ||
| syncPositions?: DiagramNodePositions, | ||
| ) => { | ||
| const currentPositions = persistedNodePositionsRef.current; | ||
| const next = update(currentPositions); | ||
| if (next === currentPositions) { | ||
| return; | ||
| } | ||
|
|
||
| persistedNodePositionsRef.current = next; | ||
| if (nodePositions === undefined) { | ||
| setUncontrolledNodePositions(next); | ||
| } | ||
| onNodePositionsChange?.(syncPositions ?? next, mode); | ||
| }, [nodePositions, onNodePositionsChange]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
setNodePositions default full-replace broadcast can wipe collaborators' saved layout.
setNodePositions defaults mode to 'replace' and broadcasts syncPositions ?? next via onNodePositionsChange. Two call sites invoke it without a scoped syncPositions, so they broadcast the entire locally-known positions map as an unconditional replace of the shared state:
- The graph-change trim effect (lines 638-656) calls
setNodePositions((current) => {...trim...})with nosyncPositions, so any local node removal triggers a full'replace'of the shared node-positions map with whatever this client currently has computed — a race window if another collaborator's concurrently-added position hasn't yet been merged locally. - The "Clean layout to Mermaid" button (line 1621) calls
setNodePositions(() => ({})), broadcasting{}with'replace'— this clears all persisted positions for every collaborator, not just this user's local drags.
This directly contradicts the PR's own stated scope ("reset-layout button for locally dragged React Flow nodes") and matches the collaboration-safety class of bug called out in review history (capturing a local snapshot and rewriting the whole shared map). Contrast with handleFlowNodeDragStop (982-992), which correctly scopes the broadcast to the single moved node via syncPositions + 'merge' — that pattern should be applied to these two call sites as well (e.g., only sync the specific removed keys, or track and clear only positions this client itself set).
Also applies to: 638-657, 1620-1624
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/components/diagram-canvas.tsx` around lines 218 - 234, The
default full-replace broadcast in setNodePositions is clearing or overwriting
collaborators’ shared layout state. Update the graph-trim effect and the “Clean
layout to Mermaid” action so they do not call setNodePositions with an
unconditional replace of the full map; instead scope the sync to only the
affected node keys or local drag-owned positions, following the merge-style
pattern used by handleFlowNodeDragStop. Keep the local persistedNodePositionsRef
update, but ensure onNodePositionsChange only receives the minimal delta rather
than syncPositions ?? next for these call sites.
Fugu exact-head review — changes requestedVerdict: Block / changes requested for exact head The Yjs node-position fix is directionally correct: drag/add writes now call merge paths ( Blocking finding:
Non-blocking notes:
Evidence reviewed:
Bot feedback + learning:
|
Fugu exact-head review — approvedVerdict: Approve, scoped only to exact head Findings:
Verification run on local Node v22.22.3 (repo warns it wants >=24):
Bot feedback + learning:
|
Summary
@xyflow/reactand render flowchart nodes/edges through React Flow using Mermaid SVG hit-map layout as the initial layout sourceVerification
npx --yes pnpm@10.15.0 --filter @arielcharts/shared buildnpx --yes pnpm@10.15.0 typechecknpx --yes pnpm@10.15.0 testnpx --yes pnpm@10.15.0 buildnpx --yes tsx e2e-validate.tsNote: this devbox is on Node v22.22.3, so pnpm prints the repo's Node >=24 engine warning, but all checks above passed.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Tests