fix: validate API graph shape and add cycle guards + error boundary a…#143
Conversation
📝 WalkthroughWalkthroughAdds cycle-safe ChangesCycle protection, ErrorBoundary, and API validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@src/components/ui/CodeWorkspace.tsx`:
- Around line 302-306: The graph validation in CodeWorkspace is still too
permissive because graphSchema uses a loose shape and the result is force-cast
to Graph, allowing invalid node/edge fields like parent and layout metadata to
slip through. Tighten graphSchema so it fully matches the GraphNode and
GraphEdge contract used by Diagram, remove the .loose() behavior for fields that
must be validated, and update the setGraph(parsed.data ...) path so parsed.data
is accepted by TypeScript without any cast.
In `@src/components/ui/Diagram.tsx`:
- Around line 680-684: The Diagram error boundary currently keeps its fallback
state across different inputs, so a failure in DiagramFlow can persist even when
CodeWorkspace passes a new valid graph. Update the ErrorBoundary usage in
Diagram by tying it to the current graph input with a key derived from graph (or
a stable graph id/hash if available) so the boundary remounts and resets when
the graph changes.
In `@src/components/ui/ErrorBoundary.tsx`:
- Around line 29-71: The ErrorBoundary fallback in ErrorBoundary should reset
when the wrapped diagram data changes, not only when the user clicks Try Again.
Add a reset mechanism such as a resetKeys prop and clear hasError/error when
those keys change in componentDidUpdate or via getDerivedStateFromProps, so new
valid input can recover automatically. Update the ErrorBoundary usage in Diagram
to pass a stable key derived from the graph/input data, and keep the existing
fallback render path and render() behavior intact.
- Around line 53-55: The heading in ErrorBoundary’s render fallback is using a
nonstandard Tailwind class, so update the h3 styling from text-md to text-base
in the ErrorBoundary component to match the default Tailwind scale and ensure
the intended font size is applied.
🪄 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 Plus
Run ID: f65acc97-6040-4afd-b3e4-ebadffd8c049
📒 Files selected for processing (7)
src/components/ui/CodeWorkspace.tsxsrc/components/ui/Diagram.test.tssrc/components/ui/Diagram.tsxsrc/components/ui/ErrorBoundary.tsxsrc/test/mocks/styleMock.csssrc/test/mocks/styleMock.jsvitest.config.ts
| const parsed = graphSchema.safeParse(data); | ||
| if (!parsed.success) { | ||
| throw new Error("Invalid graph data returned by the API."); | ||
| } | ||
| setGraph(parsed.data as Graph); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
The new validation still lets malformed graph shapes cross the API boundary.
graphSchema only checks a minimal subset of node/edge fields and allows everything else through with .loose(), so invalid parent/layout fields can still reach Diagram. Line 306 then hides that drift again with as Graph. Please align the schema with the full GraphNode/GraphEdge contract and let parsed.data type-check without a cast.
🤖 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 `@src/components/ui/CodeWorkspace.tsx` around lines 302 - 306, The graph
validation in CodeWorkspace is still too permissive because graphSchema uses a
loose shape and the result is force-cast to Graph, allowing invalid node/edge
fields like parent and layout metadata to slip through. Tighten graphSchema so
it fully matches the GraphNode and GraphEdge contract used by Diagram, remove
the .loose() behavior for fields that must be validated, and update the
setGraph(parsed.data ...) path so parsed.data is accepted by TypeScript without
any cast.
| <ErrorBoundary> | ||
| <ReactFlowProvider> | ||
| <DiagramFlow {...props} /> | ||
| </ReactFlowProvider> | ||
| </ErrorBoundary> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Tie the boundary to graph so it recovers on new input.
Because ErrorBoundary does not reset on prop changes (see ErrorBoundary.tsx), a render failure for one graph will keep showing the fallback even after CodeWorkspace passes a different valid graph. Adding a key derived from the graph forces a remount (and fresh boundary state) whenever the input changes — a minimal fix until/unless resetKeys is added.
Proposed fix
export default function Diagram(props: { graph: Graph; emptyLabel: string }) {
return (
- <ErrorBoundary>
+ <ErrorBoundary key={props.graph.nodes.length + ":" + props.graph.edges.length}>
<ReactFlowProvider>
<DiagramFlow {...props} />
</ReactFlowProvider>
</ErrorBoundary>
);
}A more robust key would be a stable hash/id of the graph if one is available.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <ErrorBoundary> | |
| <ReactFlowProvider> | |
| <DiagramFlow {...props} /> | |
| </ReactFlowProvider> | |
| </ErrorBoundary> | |
| <ErrorBoundary key={props.graph.nodes.length + ":" + props.graph.edges.length}> | |
| <ReactFlowProvider> | |
| <DiagramFlow {...props} /> | |
| </ReactFlowProvider> | |
| </ErrorBoundary> |
🤖 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 `@src/components/ui/Diagram.tsx` around lines 680 - 684, The Diagram error
boundary currently keeps its fallback state across different inputs, so a
failure in DiagramFlow can persist even when CodeWorkspace passes a new valid
graph. Update the ErrorBoundary usage in Diagram by tying it to the current
graph input with a key derived from graph (or a stable graph id/hash if
available) so the boundary remounts and resets when the graph changes.
| public render() { | ||
| if (this.state.hasError) { | ||
| if (this.props.fallback) { | ||
| return this.props.fallback; | ||
| } | ||
| return ( | ||
| <div className="flex h-full w-full flex-col items-center justify-center p-6 text-center bg-[#f6f7f9] dark:bg-[#0b0d12]"> | ||
| <div className="max-w-md rounded-xl border border-red-100 dark:border-red-950 bg-white dark:bg-[#12151c] p-6 shadow-sm"> | ||
| <div className="mx-auto mb-3 flex h-10 w-10 items-center justify-center rounded-full bg-red-100 dark:bg-red-950 text-red-600 dark:text-red-400"> | ||
| <svg | ||
| width="20" | ||
| height="20" | ||
| viewBox="0 0 24 24" | ||
| fill="none" | ||
| stroke="currentColor" | ||
| strokeWidth="2" | ||
| strokeLinecap="round" | ||
| strokeLinejoin="round" | ||
| > | ||
| <circle cx="12" cy="12" r="10" /> | ||
| <line x1="12" y1="8" x2="12" y2="12" /> | ||
| <line x1="12" y1="16" x2="12.01" y2="16" /> | ||
| </svg> | ||
| </div> | ||
| <h3 className="text-md font-semibold text-[#0f172a] dark:text-[#e6e9ef] mb-1.5"> | ||
| Failed to render diagram | ||
| </h3> | ||
| <p className="text-[13px] leading-relaxed text-[#475569] dark:text-[#9aa6b8] mb-4"> | ||
| The diagram contains malformed or cyclic data and could not be displayed correctly. | ||
| </p> | ||
| <button | ||
| onClick={() => this.setState({ hasError: false, error: null })} | ||
| className="rounded-lg bg-[#4f46e5] px-3.5 py-2 text-xs font-medium text-white hover:bg-[#4338ca] transition-colors" | ||
| > | ||
| Try Again | ||
| </button> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return this.props.children; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Boundary never resets when the failing input changes.
hasError only clears via the "Try Again" button. There is no getDerivedStateFromProps/resetKeys mechanism, so:
- After an error, supplying a new (valid)
graphto the wrappedDiagramkeeps the fallback UI shown until the user clicks "Try Again". - "Try Again" merely re-renders the same
children; if the underlying error is deterministic, it immediately re-throws and returns to the fallback.
Consider accepting a resetKeys prop and clearing error state in componentDidUpdate when a key changes, so the boundary recovers automatically when the offending data is replaced. The wrap site in Diagram.tsx (Lines 680-684) is the consumer affected by this.
🤖 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 `@src/components/ui/ErrorBoundary.tsx` around lines 29 - 71, The ErrorBoundary
fallback in ErrorBoundary should reset when the wrapped diagram data changes,
not only when the user clicks Try Again. Add a reset mechanism such as a
resetKeys prop and clear hasError/error when those keys change in
componentDidUpdate or via getDerivedStateFromProps, so new valid input can
recover automatically. Update the ErrorBoundary usage in Diagram to pass a
stable key derived from the graph/input data, and keep the existing fallback
render path and render() behavior intact.
| <h3 className="text-md font-semibold text-[#0f172a] dark:text-[#e6e9ef] mb-1.5"> | ||
| Failed to render diagram | ||
| </h3> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Tailwind CSS v4 include a text-md font-size utility by default?
💡 Result:
No, Tailwind CSS v4 does not include a text-md font-size utility class by default [1][2]. In Tailwind CSS, the standard utility for the base (medium) font size is text-base [1][3]. While other utilities follow an -md suffix convention (such as rounded-md or shadow-md), the font-size utility uses text-base to represent the base, default text size [3][4]. You can add a text-md utility if desired by defining it in your project's CSS using the @theme directive [5][6]: @theme { --text-md: 1rem; --text-md--line-height: 1.5rem; }
Citations:
- 1: https://tailwindcss.com/docs/font-size
- 2: https://www.layout.software/blog/tailwindcss-font-size
- 3: Why is there no text-md utility class? tailwindlabs/tailwindcss#2067
- 4: https://www.reddit.com/r/tailwindcss/comments/1df2nez/textmd/
- 5: https://tailwindlabs-tailwindcss.mintlify.app/customization/theme
- 6: https://tailwindlabs-tailwindcss.mintlify.app/advanced/theming
Use text-base instead of text-md. Tailwind doesn’t define text-md by default, so this heading won’t get the intended font size unless a custom token is added.
🤖 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 `@src/components/ui/ErrorBoundary.tsx` around lines 53 - 55, The heading in
ErrorBoundary’s render fallback is using a nonstandard Tailwind class, so update
the h3 styling from text-md to text-base in the ErrorBoundary component to match
the default Tailwind scale and ensure the intended font size is applied.
2 participants
Title fix: validate API graph shape and add cycle guards + error boundary around diagram
Description
This PR resolves Issue #45 by implementing robust API payload validation and visual guards around diagram rendering. Previously, malformed or cyclic graph structures returned by the backend would throw or cause unbounded recursion, freezing or crashing the entire client application.
Key Changes
API Validation (Schema Guard):
graphSchemavalidation inside CodeWorkspace.tsx for/api/analyzeresponses.Hierarchical Cycle & Infinite Loop Guards:
sizeOf,ancestorIn,topAncestor, anddepthOf) in Diagram.tsx.React Error Boundary:
<Diagram>component to catch any layout-calculation or render-time crash, presenting a clean fallback error card with a "Try Again" option without breaking the rest of the application.Testing & Environment Configurations:
Verification / Testing Done
pnpm test) pass cleanly.pnpm typecheck) succeeds with no warnings.Summary by CodeRabbit
New Features
Bug Fixes
Chores