Open
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/graph/graphConfig.ts">
<violation number="1" location="src/graph/graphConfig.ts:4">
P2: Changing the exported default direction to `LR` silently changes `GraphViewer`'s public default layout for every consumer that does not pass `direction`. Keep `TB` here unless this breaking change is being rolled out intentionally with matching docs/release updates.</violation>
<violation number="2" location="src/graph/graphConfig.ts:32">
P1: These new palette keys are too generic for a component that writes its theme variables onto `document.documentElement`; mounting the graph can now override the host app's global `--color-bg`, `--font-*`, and `--shadow-*` tokens.</violation>
</file>
<file name="src/graph/react/graph-core.css">
<violation number="1" location="src/graph/react/graph-core.css:2">
P2: Making the exported GraphViewer root `position: absolute` is a breaking layout change. Existing callers that mount it in a normal container will now need a positioned parent, or the graph will size/anchor against the wrong containing block.</violation>
</file>
<file name="src/graph/react/index.ts">
<violation number="1" location="src/graph/react/index.ts:1">
P2: This client directive is too broad for the `graph/react` barrel. It makes every export from this entrypoint client-only, including the helper/type-converter APIs re-exported below.</violation>
</file>
<file name="src/graph/react/GraphViewer.tsx">
<violation number="1" location="src/graph/react/GraphViewer.tsx:56">
P2: Restore root CSS variables in this effect; otherwise one GraphViewer instance can leave a stale palette applied globally after unmount or palette changes.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:20">
P2: Add `shiki` back to the peer-dependency table. The README still documents the `/shiki` entrypoint, and `package.json` still declares `shiki` as a peer dependency.</violation>
<violation number="2" location="README.md:64">
P2: The documented default direction is incorrect. `GraphViewer` and `DEFAULT_GRAPH_CONFIG` default to `"LR"`, so readers who omit `direction` will not get a top-to-bottom layout.</violation>
<violation number="3" location="README.md:119">
P3: Use `GraphSpecEdgeKind` here. `EdgeKind` is not exported, so this snippet does not compile when copied into a TypeScript project.</violation>
<violation number="4" location="README.md:153">
P2: This example GraphSpec is incompatible with `buildGraph()`. The renderer creates stuff nodes from `node.io.inputs/outputs` digests, not from standalone `"Stuff"` nodes and `kind: "data"` edges, so a copy-pasted example will render empty.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "--color-accent": "#8BE9FD", | ||
| "--color-warning": "#FFB86C", | ||
| // Base theme vars used by graph-core.css | ||
| "--color-bg": "#1e1e1e", |
There was a problem hiding this comment.
P1: These new palette keys are too generic for a component that writes its theme variables onto document.documentElement; mounting the graph can now override the host app's global --color-bg, --font-*, and --shadow-* tokens.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/graph/graphConfig.ts, line 32:
<comment>These new palette keys are too generic for a component that writes its theme variables onto `document.documentElement`; mounting the graph can now override the host app's global `--color-bg`, `--font-*`, and `--shadow-*` tokens.</comment>
<file context>
@@ -27,6 +28,14 @@ export const DEFAULT_GRAPH_CONFIG: GraphConfig = {
"--color-accent": "#8BE9FD",
"--color-warning": "#FFB86C",
+ // Base theme vars used by graph-core.css
+ "--color-bg": "#1e1e1e",
+ "--color-bg-dots": "#334155",
+ "--color-text-muted": "#94a3b8",
</file context>
|
|
||
| export const DEFAULT_GRAPH_CONFIG: GraphConfig = { | ||
| direction: "TB", | ||
| direction: "LR", |
There was a problem hiding this comment.
P2: Changing the exported default direction to LR silently changes GraphViewer's public default layout for every consumer that does not pass direction. Keep TB here unless this breaking change is being rolled out intentionally with matching docs/release updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/graph/graphConfig.ts, line 4:
<comment>Changing the exported default direction to `LR` silently changes `GraphViewer`'s public default layout for every consumer that does not pass `direction`. Keep `TB` here unless this breaking change is being rolled out intentionally with matching docs/release updates.</comment>
<file context>
@@ -1,14 +1,15 @@
export const DEFAULT_GRAPH_CONFIG: GraphConfig = {
- direction: "TB",
+ direction: "LR",
showControllers: false,
nodesep: 50,
</file context>
Suggested change
| direction: "LR", | |
| direction: "TB", |
| .react-flow-container { | ||
| width: 100%; | ||
| height: 100%; | ||
| position: absolute; |
There was a problem hiding this comment.
P2: Making the exported GraphViewer root position: absolute is a breaking layout change. Existing callers that mount it in a normal container will now need a positioned parent, or the graph will size/anchor against the wrong containing block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/graph/react/graph-core.css, line 2:
<comment>Making the exported GraphViewer root `position: absolute` is a breaking layout change. Existing callers that mount it in a normal container will now need a positioned parent, or the graph will size/anchor against the wrong containing block.</comment>
<file context>
@@ -1,6 +1,6 @@
.react-flow-container {
- width: 100%;
- height: 100%;
+ position: absolute;
+ inset: 0;
background: var(--color-bg);
</file context>
| @@ -1,3 +1,8 @@ | |||
| "use client"; | |||
There was a problem hiding this comment.
P2: This client directive is too broad for the graph/react barrel. It makes every export from this entrypoint client-only, including the helper/type-converter APIs re-exported below.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/graph/react/index.ts, line 1:
<comment>This client directive is too broad for the `graph/react` barrel. It makes every export from this entrypoint client-only, including the helper/type-converter APIs re-exported below.</comment>
<file context>
@@ -1,3 +1,8 @@
+"use client";
+
+import "@xyflow/react/dist/style.css";
</file context>
Comment on lines
+56
to
+62
| React.useEffect(() => { | ||
| const palette = config.paletteColors ?? DEFAULT_GRAPH_CONFIG.paletteColors; | ||
| if (palette) { | ||
| for (const [cssVar, value] of Object.entries(palette)) { | ||
| document.documentElement.style.setProperty(cssVar, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
P2: Restore root CSS variables in this effect; otherwise one GraphViewer instance can leave a stale palette applied globally after unmount or palette changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/graph/react/GraphViewer.tsx, line 56:
<comment>Restore root CSS variables in this effect; otherwise one GraphViewer instance can leave a stale palette applied globally after unmount or palette changes.</comment>
<file context>
@@ -42,8 +43,24 @@ function cloneCachedNodes(nodes: GraphNode[]): GraphNode[] {
+ } = props;
+
+ // Apply palette CSS vars on mount (so consumers don't have to)
+ React.useEffect(() => {
+ const palette = config.paletteColors ?? DEFAULT_GRAPH_CONFIG.paletteColors;
+ if (palette) {
</file context>
Suggested change
| React.useEffect(() => { | |
| const palette = config.paletteColors ?? DEFAULT_GRAPH_CONFIG.paletteColors; | |
| if (palette) { | |
| for (const [cssVar, value] of Object.entries(palette)) { | |
| document.documentElement.style.setProperty(cssVar, value); | |
| } | |
| } | |
| React.useEffect(() => { | |
| const rootStyle = document.documentElement.style; | |
| const palette = config.paletteColors ?? DEFAULT_GRAPH_CONFIG.paletteColors; | |
| if (!palette) return; | |
| const previousValues = Object.fromEntries( | |
| Object.keys(palette).map((cssVar) => [cssVar, rootStyle.getPropertyValue(cssVar)]), | |
| ); | |
| for (const [cssVar, value] of Object.entries(palette)) { | |
| rootStyle.setProperty(cssVar, value); | |
| } | |
| return () => { | |
| for (const [cssVar, value] of Object.entries(previousValues)) { | |
| if (value) { | |
| rootStyle.setProperty(cssVar, value); | |
| } else { | |
| rootStyle.removeProperty(cssVar); | |
| } | |
| } | |
| }; | |
| }, [config.paletteColors]); |
| { "id": "summary", "pipe_type": "Stuff" } | ||
| ], | ||
| "edges": [ | ||
| { "source": "input_doc", "target": "extract_text", "kind": "data" }, |
There was a problem hiding this comment.
P2: This example GraphSpec is incompatible with buildGraph(). The renderer creates stuff nodes from node.io.inputs/outputs digests, not from standalone "Stuff" nodes and kind: "data" edges, so a copy-pasted example will render empty.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 153:
<comment>This example GraphSpec is incompatible with `buildGraph()`. The renderer creates stuff nodes from `node.io.inputs/outputs` digests, not from standalone `"Stuff"` nodes and `kind: "data"` edges, so a copy-pasted example will render empty.</comment>
<file context>
@@ -1,153 +1,275 @@
+ { "id": "summary", "pipe_type": "Stuff" }
+ ],
+ "edges": [
+ { "source": "input_doc", "target": "extract_text", "kind": "data" },
+ { "source": "extract_text", "target": "pages", "kind": "data" },
+ { "source": "pages", "target": "summarize", "kind": "data" },
</file context>
| | ------------------- | ----------------------------------- | ---------------------------- | ------------------------------------------ | | ||
| | `graphspec` | `GraphSpec \| null` | — | Graph data (nodes + edges) | | ||
| | `config` | `GraphConfig` | `DEFAULT_GRAPH_CONFIG` | Layout and visual configuration | | ||
| | `direction` | `GraphDirection` | `"TB"` | Layout direction: `TB`, `LR`, `RL`, `BT` | |
There was a problem hiding this comment.
P2: The documented default direction is incorrect. GraphViewer and DEFAULT_GRAPH_CONFIG default to "LR", so readers who omit direction will not get a top-to-bottom layout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 64:
<comment>The documented default direction is incorrect. `GraphViewer` and `DEFAULT_GRAPH_CONFIG` default to `"LR"`, so readers who omit `direction` will not get a top-to-bottom layout.</comment>
<file context>
@@ -1,153 +1,275 @@
+| ------------------- | ----------------------------------- | ---------------------------- | ------------------------------------------ |
+| `graphspec` | `GraphSpec \| null` | — | Graph data (nodes + edges) |
+| `config` | `GraphConfig` | `DEFAULT_GRAPH_CONFIG` | Layout and visual configuration |
+| `direction` | `GraphDirection` | `"TB"` | Layout direction: `TB`, `LR`, `RL`, `BT` |
+| `showControllers` | `boolean` | `false` | Show controller group outlines |
+| `onNavigateToPipe` | `(pipeCode: string) => void` | — | Callback when a pipe node is clicked |
</file context>
| | `dagre` | **yes** | Graph layout engine | | ||
| | `@types/dagre` | no (TS only) | Type definitions for dagre | | ||
| | `react`, `react-dom` | no | React layer (`graph/react`) | | ||
| | `@xyflow/react` | no | React layer (`graph/react`) | |
There was a problem hiding this comment.
P2: Add shiki back to the peer-dependency table. The README still documents the /shiki entrypoint, and package.json still declares shiki as a peer dependency.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 20:
<comment>Add `shiki` back to the peer-dependency table. The README still documents the `/shiki` entrypoint, and `package.json` still declares `shiki` as a peer dependency.</comment>
<file context>
@@ -1,153 +1,275 @@
+| `dagre` | **yes** | Graph layout engine |
+| `@types/dagre` | no (TS only) | Type definitions for dagre |
+| `react`, `react-dom` | no | React layer (`graph/react`) |
+| `@xyflow/react` | no | React layer (`graph/react`) |
-## Entry points
</file context>
Suggested change
| | `@xyflow/react` | no | React layer (`graph/react`) | | |
| | `@xyflow/react` | no | React layer (`graph/react`) | | |
| | `shiki` | no | Syntax highlighting (`shiki`) | |
| id?: string; | ||
| source: string; // Source node ID | ||
| target: string; // Target node ID | ||
| kind: EdgeKind; // "data", "contains", "batch_item", etc. |
There was a problem hiding this comment.
P3: Use GraphSpecEdgeKind here. EdgeKind is not exported, so this snippet does not compile when copied into a TypeScript project.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 119:
<comment>Use `GraphSpecEdgeKind` here. `EdgeKind` is not exported, so this snippet does not compile when copied into a TypeScript project.</comment>
<file context>
@@ -1,153 +1,275 @@
+ id?: string;
+ source: string; // Source node ID
+ target: string; // Target node ID
+ kind: EdgeKind; // "data", "contains", "batch_item", etc.
+ label?: string;
}
</file context>
Suggested change
| kind: EdgeKind; // "data", "contains", "batch_item", etc. | |
| kind: GraphSpecEdgeKind; // "data", "contains", "batch_item", etc. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Simplified the
GraphViewerinto a drop-in React component with sane defaults and built-in styling, plus clearer docs and Next.js guidance. Also updated defaults (layout nowLR) and improved theming by auto-applying palette CSS variables.New Features
GraphViewerprops are now optional; usesDEFAULT_GRAPH_CONFIGand infersdirection/showControllers.paletteColorsCSS vars on mount; no manual theming setup.@xyflow/reactstyles andgraph-core.css; no extra CSS imports needed.dynamic(..., { ssr: false }), GraphSpec, and config docs.Migration
position: absolute; inset: 0; ensure the parent hasposition: relativeand a set height.LR(wasTB); passdirection="TB"if you prefer top-to-bottom.@xyflow/react/dist/style.cssandgraph-core.csswhen using@pipelex/mthds-ui/graph/react.npm install @pipelex/mthds-ui(package version bumped to0.1.4).Written for commit 91b8f05. Summary will update on new commits.