feat(trading-signals-docs): add visual strategy builder - #1191
Open
bennycode wants to merge 3 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new /graph-builder page in trading-signals-docs that provides a visual, React Flow–based strategy builder backed by the trading-strategies graph runtime, including JSON import/export and backtest execution.
Changes:
- Introduces graph<->canvas conversion utilities and a new visual graph editor (palette, typed ports, inline node config, delete behavior).
- Adds a new docs page that validates graphs by instantiating
GraphStrategyand runs backtests (plus buy-and-hold baseline) with existing result rendering. - Adds Playwright E2E coverage for the builder flows and wires the builder entry into the docs navigation; adds
@xyflow/reactdependency + styles.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/trading-signals-docs/utils/graphIO.ts | Converts React Flow nodes/edges to/from StrategyGraphInput JSON. |
| packages/trading-signals-docs/pages/graph-builder.tsx | Implements the Strategy Builder page: validation, JSON share/import, backtest execution. |
| packages/trading-signals-docs/pages/_app.tsx | Adds React Flow global CSS and a “Builder” nav item. |
| packages/trading-signals-docs/package.json | Adds @xyflow/react dependency. |
| packages/trading-signals-docs/e2e/pages/GraphBuilderPage.ts | Adds Playwright page object for graph builder interactions. |
| packages/trading-signals-docs/e2e/graph-builder.spec.ts | Adds E2E specs for template, build-from-scratch, validation, deletion, type mismatch. |
| packages/trading-signals-docs/components/graph/nodeFields.ts | Defines per-node editable config field descriptors for inline forms. |
| packages/trading-signals-docs/components/graph/GraphEditor.tsx | Implements the React Flow editor: palette, connection validation, node creation, change propagation. |
| packages/trading-signals-docs/components/graph/BuilderNodeView.tsx | Renders node UI: ports, labels, inline config widgets, delete button. |
| package-lock.json | Updates lockfile for the new dependency and its transitive packages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+122
to
+135
| <input | ||
| data-testid={`node-${id}-${field.key}`} | ||
| className={common} | ||
| type={field.widget === 'number' ? 'number' : 'text'} | ||
| placeholder={field.placeholder} | ||
| value={value === undefined ? '' : String(value)} | ||
| onChange={event => | ||
| onConfigChange( | ||
| id, | ||
| field.key, | ||
| field.widget === 'number' ? Number(event.target.value) : event.target.value | ||
| ) | ||
| } | ||
| /> |
Comment on lines
+78
to
+83
| const loadGraph = useCallback((graph: StrategyGraphInput) => { | ||
| setLoadedGraph(graph); | ||
| setEditorKey(key => key + 1); | ||
| setResult(null); | ||
| setBaselineResult(null); | ||
| }, []); |
A node-based editor (React Flow) where users stack building blocks — candles, batchers, indicators, if-conditions, order advice — into trading strategies, n8n/Scratch style. The canvas state is the same graph JSON that GraphStrategy executes, so what users see is exactly what runs. - Palette driven by the engine's node registry (registered custom nodes appear automatically); config forms rendered inside nodes - Typed ports: mismatched connections refuse to snap; occupied inputs reject second writers - Live validation by constructing the real interpreter on every change, surfacing node-addressable errors before running - SMA crossover template, clear canvas, per-node delete, JSON share/import - Backtests run against the existing BacktestExecutor with a buy-and-hold baseline, reusing BacktestResults - Playwright coverage incl. building the full strategy from scratch via palette clicks and port drags, asserting results identical to the template
bennycode
force-pushed
the
feat/graph-builder-ui
branch
from
July 13, 2026 14:43
4bb8e29 to
e1593a8
Compare
…ilder - Clearing a number config field now unsets the key so the node's schema default applies, instead of coercing the empty string to 0 - loadGraph() syncs the current graph immediately, so validation and the run button never lag behind Clear/Template actions while the client-only canvas re-hydrates - Clear the pending 'Copied!' reset timer on unmount
bennycode
force-pushed
the
feat/graph-builder-ui
branch
from
July 13, 2026 14:44
e1593a8 to
0d7dd5b
Compare
Owner
Author
|
Addressed all three review findings in 0d7dd5b:
Also rebased onto the updated #1190 (deterministic topo sort). Full e2e suite re-run: green. |
…e and forms
- Replace the key-remount reset (Clear/Template/JSON import) with a plain
resetSignal prop the editor applies via setNodes/setEdges, so the canvas
never unmounts and keeps its viewport. A forwardRef+useImperativeHandle
version was tried first but next/dynamic hijacks the ref for its own
{retry} handle and never forwards it to the loaded component.
- Debounce the interpreter-rebuilding validation check so it doesn't
reconstruct GraphStrategy on every keystroke in a config field.
- Show each node's actual Zod schema default (via configSchema.safeParse({}))
as the config field's placeholder/selected value instead of a UI guess,
so an unset field no longer looks unrelated to what a backtest will use.
- Fix addNode: move id/position computation out of the setNodes updater so
the updater stays pure, and replace the fixed-height grid (which let tall
nodes like `advice` overlap the row below) with column shelf-packing based
on each node type's estimated height.
- Add a confirm() guard before Clear canvas, and surface clipboard write
failures in copyJson() instead of leaving them as unhandled rejections.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Node creation in GraphEditor can produce duplicate ids and incorrect placement due to using render-time nodes state rather than the latest state inside the setNodes update.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 10/11 changed files
- Comments generated: 1
- Review effort level: Lite
Comment on lines
+176
to
+195
| const addNode = useCallback( | ||
| (nodeType: string) => { | ||
| const shortName = nodeType.replace('source:', ''); | ||
| let suffix = nodes.length + 1; | ||
| let id = `${shortName}-${suffix}`; | ||
| while (nodes.some(node => node.id === id)) { | ||
| suffix += 1; | ||
| id = `${shortName}-${suffix}`; | ||
| } | ||
| // Shelf-pack into the shortest column so a tall node never overlaps the one placed after it. | ||
| const heights = columnHeights.current; | ||
| const column = heights.indexOf(Math.min(...heights)); | ||
| const position = {x: GRID_ORIGIN_X + column * COLUMN_WIDTH, y: heights[column]}; | ||
| heights[column] += estimateNodeHeight(nodeType) + NODE_GAP_Y; | ||
|
|
||
| const newNode: BuilderNode = {data: {config: {}, nodeType}, id, position, type: 'builder'}; | ||
| setNodes(current => [...current, newNode]); | ||
| }, | ||
| [nodes, setNodes] | ||
| ); |
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
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.
What
A visual, node-based strategy builder at
/graph-builder— n8n/Scratch-style: drag building blocks, connect typed ports, configure them inline, and backtest. The canvas state is the graph JSON thatGraphStrategyexecutes, so what users see is exactly what runs.Features
registerNodeType()appear automatically, no UI changesBacktestExecutorwith a buy-and-hold baseline, rendered by the existingBacktestResultsTest plan
6 Playwright specs (class-based page object per repo e2e conventions), including the flagship: build the SMA crossover from scratch — palette clicks, config edits, 10 real mouse-dragged connections — and assert the backtest matches the template run exactly (trade count + ROI). Plus: template validity, results rendering, incomplete-graph rejection, type-mismatch refusal, node deletion. Full e2e suite 9/9 green;
next buildprerenders the page statically.