feat(flows): Workflows B5b.1 — read-only canvas view (/flows/:id)#4477
Merged
Conversation
First slice of the visual builder: view any saved flow as a node graph.
- Adds @xyflow/react ^12.11.1 (MIT; dist/style.css inlines via Vite — no CSP
change; CEF-safe SVG/DOM). Static import per repo rule.
- lib/flows/types.ts — TS mirror of tinyflows 0.3 model (verified vs crate source).
- lib/flows/graphAdapter.ts (+20 tests) — workflowGraphToXyflow / xyflowToWorkflowGraph
/ autoLayout(nodes,edges) / edgeId. Derives effective input/output ports per node
from declared ports ∪ touching edges (Rust Port is {name,label?}, output-only; a
switch's live case ports are runtime-computed), so switch/branch nodes render right.
Node positions round-trip via the graph's position field.
- components/flows/canvas/{FlowCanvas,FlowNodeComponent}.tsx + styles — ReactFlow in
read-only mode (no drag/connect/select) + minimap/controls; per-kind node cards.
- pages/FlowCanvasPage.tsx — /flows/:id: getFlow → adapter → FlowCanvas; loading/
error/not-found.
- flowsApi.getFlow; AppRoutes split /flows (list) + /flows/:id (canvas); FlowListRow
flow-name is now a View link → navigate. navigation.spec unchanged (not tab-level).
- i18n flows.canvas.* + flows.list.view + flows.nodeKind.* (12) x14 locales.
typecheck/lint/prettier clean; 75 tests pass; i18n parity clean.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughAdds a read-only Workflow Canvas feature: graph conversion to ChangesWorkflow Canvas Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
coderabbitai
Bot
added
the
feature
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd5f02bd3e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| /** Stable, collision-free xyflow edge id for one `WorkflowEdge`. */ | ||
| export function edgeId(edge: WorkflowEdge): string { | ||
| return `${edge.from_node}-${edge.from_port}-${edge.to_node}-${edge.to_port}`; |
There was a problem hiding this comment.
When a valid graph uses - in a node id or port name, different edges can produce the same React Flow id here; for example from_node: "a-b", from_port: "c", to_node: "d", to_port: "e" and from_node: "a", from_port: "b-c", to_node: "d", to_port: "e" both become a-b-c-d-e. The flow proposal schema only constrains these fields as strings, and React Flow treats edge ids as unique keys, so one of the connections can be overwritten or rendered incorrectly in the canvas. Encode the tuple (or escape delimiters) before using it as the id.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Fixed in 7bb91e4 — edgeId now uses JSON.stringify([from_node,from_port,to_node,to_port]), collision-safe when names contain -. Added tests for the colliding case + edgeId determinism.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/src/lib/flows/graphAdapter.ts (1)
50-53: 📐 Maintainability & Code Quality | 🔵 Trivial
PointduplicatesPositionfromtypes.ts.Both are
{ x: number; y: number }. ReusingPositionhere would remove the duplication.
[optional_refactor_low_reward_skip]🤖 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 `@app/src/lib/flows/graphAdapter.ts` around lines 50 - 53, `Point` in graphAdapter duplicates the existing `Position` type from types.ts, so update the graphAdapter typings to reuse `Position` instead of redefining the same `{ x: number; y: number }` shape. Remove the local Point interface and switch any graphAdapter references to the shared Position symbol to keep the type definition centralized.app/src/pages/FlowCanvasPage.tsx (2)
122-127: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract ready-state rendering and memoize the graph conversion.
The IIFE inline in JSX is a bit awkward, and
workflowGraphToXyflow(which runsautoLayout) recomputes on every render whilestate.flowis unchanged. Consider hoisting to auseMemokeyed onstate.🤖 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 `@app/src/pages/FlowCanvasPage.tsx` around lines 122 - 127, The ready-state JSX in FlowCanvasPage is using an inline IIFE and recalculates the graph layout on every render. Refactor the `state.status === 'ready'` branch by hoisting the `workflowGraphToXyflow` conversion out of JSX and memoizing it with `useMemo`, keyed on the relevant `state.flow`/graph data, then render `FlowCanvas` directly from the memoized `nodes` and `edges`.
66-73: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse the RPC kind here instead of matching the error text.
CoreRpcErroralready exposeskind: 'thread_not_found'andisThreadNotFoundCoreRpcError(...); branch on that so not-found handling doesn’t depend on backend wording.🤖 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 `@app/src/pages/FlowCanvasPage.tsx` around lines 66 - 73, The not-found branch in FlowCanvasPage’s load error handling is relying on matching error text, which is brittle. Update the catch block to branch on the RPC error type instead of message content by using CoreRpcError’s kind value and the isThreadNotFoundCoreRpcError helper, then keep the existing notFound vs error state updates in the same load logic.
🤖 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 `@app/src/components/flows/canvas/FlowNodeComponent.tsx`:
- Around line 74-81: Add a defensive fallback in FlowNodeComponent so an unknown
FlowNode kind does not break canvas rendering. In the FlowNodeComponent path
where NODE_KIND_META[data.kind] is read, guard against a missing entry before
using meta.color and derive a safe default color/label for unsupported kinds.
Keep the rest of the node rendering working by falling back in the
FlowNodeComponent logic rather than assuming every kind exists in
NODE_KIND_META.
In `@app/src/lib/flows/graphAdapter.ts`:
- Around line 36-45: `xyflowToWorkflowGraph` is dropping `type_version` on
round-trip because `FlowNodeData` does not carry it and the rebuild in
`xyflowToWorkflowGraph` omits it, causing non-default versions to be downgraded
to the default. Thread `type_version` through `FlowNodeData` and both conversion
paths in `graphAdapter.ts`, updating the
`flowGraphToXyflow`/`xyflowToWorkflowGraph` logic so `WorkflowNode` preserves
the original version when present and still defaults correctly when absent. Also
update the lossy-field doc comment near `xyflowToWorkflowGraph` to mention
`type_version` alongside `position`.
In `@app/src/pages/FlowCanvasPage.tsx`:
- Around line 54-79: The FlowCanvasPage load flow can be overwritten by stale
responses from a previous id because getFlow() is never cancelled when id
changes. Update the load/useEffect path in FlowCanvasPage so each request is
tied to the current id and any in-flight request is aborted or ignored when a
newer load starts; only call setState from load after confirming the response
still matches the latest id, and clean up the request in the useEffect that
invokes load().
---
Nitpick comments:
In `@app/src/lib/flows/graphAdapter.ts`:
- Around line 50-53: `Point` in graphAdapter duplicates the existing `Position`
type from types.ts, so update the graphAdapter typings to reuse `Position`
instead of redefining the same `{ x: number; y: number }` shape. Remove the
local Point interface and switch any graphAdapter references to the shared
Position symbol to keep the type definition centralized.
In `@app/src/pages/FlowCanvasPage.tsx`:
- Around line 122-127: The ready-state JSX in FlowCanvasPage is using an inline
IIFE and recalculates the graph layout on every render. Refactor the
`state.status === 'ready'` branch by hoisting the `workflowGraphToXyflow`
conversion out of JSX and memoizing it with `useMemo`, keyed on the relevant
`state.flow`/graph data, then render `FlowCanvas` directly from the memoized
`nodes` and `edges`.
- Around line 66-73: The not-found branch in FlowCanvasPage’s load error
handling is relying on matching error text, which is brittle. Update the catch
block to branch on the RPC error type instead of message content by using
CoreRpcError’s kind value and the isThreadNotFoundCoreRpcError helper, then keep
the existing notFound vs error state updates in the same load logic.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05b48a10-d3db-4cde-9dae-8f2b753c0f5b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (30)
app/package.jsonapp/src/AppRoutes.tsxapp/src/components/flows/FlowListRow.test.tsxapp/src/components/flows/FlowListRow.tsxapp/src/components/flows/canvas/FlowCanvas.tsxapp/src/components/flows/canvas/FlowNodeComponent.tsxapp/src/components/flows/canvas/__tests__/FlowCanvas.test.tsxapp/src/components/flows/canvas/flowCanvasStyles.cssapp/src/lib/flows/graphAdapter.test.tsapp/src/lib/flows/graphAdapter.tsapp/src/lib/flows/types.tsapp/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/pages/FlowCanvasPage.tsxapp/src/pages/FlowsPage.test.tsxapp/src/pages/FlowsPage.tsxapp/src/pages/__tests__/FlowCanvasPage.test.tsxapp/src/services/api/flowsApi.ts
…pe_version, fetch race - edgeId now JSON.stringify([from_node,from_port,to_node,to_port]) — collision-safe when node/port names contain '-' (React Flow needs unique edge ids). [Codex] - FlowNodeComponent: DEFAULT_NODE_META fallback so an unrecognized NodeKind renders as a plain node instead of throwing on COLOR_CLASSES[meta.color] and crashing the whole canvas. [CodeRabbit] - graphAdapter: carry + restore type_version through the xyflow round-trip (was silently dropped → would downgrade a non-default version once B5b.4 saves). [CodeRabbit] - FlowCanvasPage: single id-keyed effect with a cancelled flag so a slow getFlow for a superseded :id can't clobber newer state (mirrors useFlowRunPoller). [CodeRabbit] 55 tests pass; typecheck/lint/prettier/i18n clean.
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
2 participants
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
First slice of the visual builder: view any saved workflow as a node graph at
/flows/:id(read-only). De-risks the whole@xyflow/reactintegration before editing lands (B5b.2+).Changes
@xyflow/react^12.11.1 — MIT; itsdist/style.cssinlines via Vite (no CSP change — the app CSP allows'unsafe-inline'styles); CEF-safe SVG/DOM. Static import per repo rule.lib/flows/types.ts— TS mirror of the tinyflows 0.3 model (verified vs crate source).lib/flows/graphAdapter.ts(+20 tests) —workflowGraphToXyflow/xyflowToWorkflowGraph/autoLayout. Derives effective in/out ports per node from declared ports ∪ touching edges (RustPortis output-only; aswitch's live case ports are runtime-computed), so branch/switch nodes render correctly. Node positions round-trip via the graph'spositionfield.FlowCanvas+FlowNodeComponent— ReactFlow read-only (no drag/connect/select) + minimap/controls; per-kind node cards.FlowCanvasPage—/flows/:id:getFlow→ adapter → canvas; loading/error/not-found.flowsApi.getFlow;AppRoutessplit/flows(list) +/flows/:id(canvas);FlowListRowflow-name is now a View link.navigation.specunchanged (/flows/:idisn't tab-level).flows.canvas.*+flows.list.view+flows.nodeKind.*(12) × 14 locales.Slicing
B5b.1 is read-only (view). Next: B5b.2 palette + drag/connect editing · B5b.3 per-node config panels · B5b.4 save + wire "New workflow"/proposal → canvas.
Verification
pnpm typecheck+lint+prettierclean · 75 Vitest tests pass ·pnpm i18n:checkparity clean.Summary by CodeRabbit