feat: add interactive graph walker visualization#3
Conversation
Add browser-safe simulation engine and visual step-through UI for flowprint blueprints. Users can run simulations directly in the editor, step through execution traces, and see node highlights updating in real-time. Engine changes: - Extract pure rules evaluation core (core.ts) from evaluator - Add browser-safe AST expression interpreter (no node:vm) - Create shared walkGraph<TStep> skeleton used by both Node.js runner and browser simulator - Build browser simulator using shared skeleton - Add ./browser entry point (no Node.js imports) Editor changes: - Add SimulationContext with NodeHighlightMap for node state tracking - Add simulation CSS classes with Catppuccin Mocha palette colors and pulse animation for active nodes App changes: - Add useSimulation hook with pre-computed snapshots for O(1) step access, chained setTimeout auto-play, and error handling - Add SimulationPanel with input form, step controls, progress scrubber, rules/expression detail, and keyboard shortcuts - Wire Simulate button in Header and integrate into App.tsx Tests: 95 new engine tests (378 total), 5 editor tests (490 total), 14 app tests (83 total), 5 Playwright E2E tests
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| try { | ||
| const steps = await walkGraph(doc, handlers, options.input, results, { | ||
| maxSteps: options.maxSteps, | ||
| }) | ||
|
|
||
| const lastStep = steps[steps.length - 1] | ||
| const outcome = lastStep?.outcome | ||
| const status = outcome === 'failure' ? 'failure' : 'success' | ||
| const lastResultKey = [...results.keys()].pop() | ||
| const output = lastResultKey !== undefined ? results.get(lastResultKey) : undefined | ||
|
|
||
| return { status, steps, output } |
There was a problem hiding this comment.
simulateGraph always derives SimulationTrace.status from the last terminal outcome, so missing rules/data errors produce a trace with status: 'success' even though the first step has status: 'error'. Consumers (e.g. the UI hook) can't tell the simulation failed without scanning every step, so can we propagate any step.status === 'error' into trace.status = 'error' instead of returning success?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In packages/engine/src/simulator/simulator.ts around lines 245-256, the simulateGraph
function derives the returned trace.status only from the last step outcome, which hides
earlier step-level errors. After obtaining steps from walkGraph, scan the steps array
for any step where status === 'error' and if any exist set trace status to 'error';
otherwise keep the current logic that maps the lastStep.outcome to 'failure' or
'success'. Update the return construction so it uses this computed status (and preserve
output and steps as before).
There was a problem hiding this comment.
Commit 5123bec addressed this comment by tracking the walk results before returning and computing a hasStepError flag so any step with status === 'error' promotes the trace status to error, while keeping the existing failure/success outcome logic and propagating an explanatory error message.
There was a problem hiding this comment.
Fixed in 5123bec. simulateGraph now scans all steps via steps.some(s => s.status === 'error') and promotes step-level errors to trace status. Partial steps are also preserved when walkGraph throws (Finding 4).
|
@claude can you fix ci/cd build for this pr? |
The modal was rendering with unstyled native HTML elements (white background, browser defaults). Replace inline styles with fp-modal-* CSS classes that use the app's existing CSS variables for proper dark/light theme support, and import app.css which was never loaded.
- Wrap switch case expression evaluation in try/catch so label-style `when` values (e.g. "Approved") gracefully fall through instead of crashing the simulation - Check for `result.error` in useSimulation before setting trace to prevent "Step 1 of 0" display on simulation errors - Make node highlights more visually dramatic: add background color tinting, thicker borders, larger outer glow, and pulse animation for active/visited/error states - Update test expectation: invalid expressions now fall through to default rather than erroring the entire simulation
albertgwo
force-pushed
the
feat/interactive-simulation
branch
from
2cb121b to
c271bb7
Compare
albertgwo
left a comment
albertgwo
left a comment
There was a problem hiding this comment.
Comprehensive PR Review
5 specialized review agents analyzed this PR covering code quality, test coverage, error handling, type design, and comment accuracy. 20 findings organized by severity.
Legend: 🔴 Critical | 🟠 Important | 🟡 Suggestion
🔴 Finding 3 — Missing changeset (code-reviewer)
CLAUDE.md states: "Each PR that changes a package should include a changeset. CI automates publishing." This PR adds significant public API to engine (new browser entry point, simulator, walker exports) and editor (simulation context), but no changeset file is included.
Fix: Add a changeset for @ruminaider/flowprint-engine (minor — new API surface) and @ruminaider/flowprint-editor (minor — new exports).
See 19 inline comments below for all remaining findings.
| expressionEvaluation: { expression: c.when, result }, | ||
| } | ||
| } | ||
| } catch { |
There was a problem hiding this comment.
🔴 Finding 1 — Silent catch swallows all errors in switch expression evaluation (error-hunter)
This bare catch {} catches everything — not just parse errors for label-style when values. Security violations (forbidden identifiers, __proto__ access), MAX_AST_DEPTH exceeded, and genuine expression typos are all silently treated as "case not matching."
A user who writes inpt.amount > 100 (typo) sees the switch take the default path with no indication why.
Suggested fix: Distinguish parse errors (label-style values) from runtime evaluation errors. Surface real errors on the step record:
} catch (err: unknown) {
const msg = err instanceof Error ? err.message : String(err)
if (!msg.startsWith('Expression parse error:')) {
return {
node_id: nodeId,
type: 'switch',
status: 'error',
error: `Case expression "${c.when}" failed: ${msg}`,
}
}
// Parse error = label-style when, continue to next case
}There was a problem hiding this comment.
Fixed in 5123bec. Introduced typed ExpressionParseError class. The catch now only swallows parse errors (multi-word labels) and bare identifiers (single-word labels like "Approved"). All other errors (security violations, depth exceeded, expression typos with operators) surface as step errors.
| }, | ||
| "dependencies": { | ||
| "@ruminaider/flowprint-editor": "workspace:*", | ||
| "@ruminaider/flowprint-engine": "workspace:*", |
There was a problem hiding this comment.
🔴 Finding 2 — CLAUDE.md dependency graph violation (code-reviewer)
CLAUDE.md states app depends on editor + schema. This line adds @ruminaider/flowprint-engine as a direct dependency, changing the documented dependency graph without updating CLAUDE.md.
Fix: Update CLAUDE.md to document app → editor + schema + engine, or restructure to avoid the direct dependency.
There was a problem hiding this comment.
Fixed in 5123bec. Updated CLAUDE.md to document app → editor + schema + engine.
| return { status, steps, output } | ||
| } catch (err: unknown) { | ||
| const errorMessage = err instanceof Error ? err.message : String(err) | ||
| return { status: 'error', steps: [], error: errorMessage } |
There was a problem hiding this comment.
🟠 Finding 4 — Simulator discards accumulated steps on error (error-hunter)
This returns steps: [], discarding all steps collected before the error. The runner correctly preserves allSteps in its error path. Combined with Finding 5, error traces become useless — the user sees "Walk exceeded maximum steps (500)" with zero steps to inspect.
Fix: Restructure so steps is available in the catch block. Mirror the runner's pattern: declare allSteps outside the try/catch.
There was a problem hiding this comment.
Fixed in 5123bec. steps is now declared outside the try/catch block, mirroring the runner's pattern. Partial progress is preserved on crash.
| // Review #8: .catch() handler for unhandled rejections | ||
| simulateGraph(doc, options).then( | ||
| (result) => { | ||
| if (result.error) { |
There was a problem hiding this comment.
🟠 Finding 5 — start() drops trace when result.error is set (error-hunter)
When simulateGraph returns status: 'error', the hook sets the error message but does NOT store the trace. Even if Finding 4 were fixed (steps preserved), the partial trace would still be discarded here.
Fix: Store the trace even when it has an error:
(result) => {
if (result.error) {
setError(result.error)
}
if (result.steps.length > 0) {
setTrace(result)
setCurrentStep(0)
}
},There was a problem hiding this comment.
Fixed in 5123bec. The hook now stores the trace whenever result.steps.length > 0, even when an error is present, so users can step through partial progress.
| style={{ width: '100%', height: '100%' }} | ||
| /> | ||
| </div> | ||
| {showSimPanel && ( |
There was a problem hiding this comment.
🟠 Finding 6 — SimulationPanel has no React error boundary (error-hunter)
SimulationPanel is rendered outside the editor's <ErrorBoundary>. If it throws during rendering (e.g., JSON.stringify on a circular reference in the context viewer), the entire React tree crashes with no recovery path.
Fix: Wrap in its own error boundary:
<SimulationErrorBoundary onReset={() => { simulation.stop(); setShowSimPanel(false) }}>
<SimulationPanel simulation={...} />
</SimulationErrorBoundary>There was a problem hiding this comment.
Fixed in 43dbfd0. Added SimulationErrorBoundary component that wraps SimulationPanel in App.tsx. On crash, it shows the error message with a "Stop Simulation" button that resets state.
| results: Map<string, unknown> | ||
| steps: TStep[] | ||
| rulesData?: Record<string, RulesDocument> | ||
| } |
There was a problem hiding this comment.
🟡 Finding 16 — rulesData on WalkContext is never set by the walker (type-analyzer)
This optional field exists on the shared context type but is never populated by walkGraph. The simulator sets it via SimulationOptions, not through the walker. Having it on WalkContext creates a false promise for implementors who might expect the walker to provide it.
Fix: Remove rulesData from WalkContext — it belongs on SimulationOptions / handler closure scope where it's actually used.
There was a problem hiding this comment.
Fixed in 7231187. Removed rulesData from WalkContext — it belongs on SimulationOptions / handler closure scope where it's actually used.
packages/engine/src/runner/types.ts
Outdated
| @@ -11,21 +13,10 @@ export interface ExecutionContext { | |||
| results: Map<string, unknown> // nodeId -> output | |||
There was a problem hiding this comment.
🟡 Finding 17 — ExecutionContext should be relocated from runner/types.ts (type-analyzer)
This type is imported by the simulator (rules/core.ts) and re-exported from the browser entry point. It has outgrown its home in runner/types.ts — it's a shared engine concept, not runner-specific.
Fix: Move to walker/types.ts or a dedicated shared-types.ts in the engine package.
There was a problem hiding this comment.
Fixed in 7231187. Moved ExecutionContext to walker/types.ts as it's a shared concept used by rules, simulator, and runner. Re-exported from runner/types.ts for import path compatibility.
| * Build a frozen Math object with only allowlisted methods/constants. | ||
| * Shared with runner/evaluator.ts (Review #4). | ||
| */ | ||
| export function buildSafeMath(): Readonly<Record<string, unknown>> { |
There was a problem hiding this comment.
🟡 Finding 18 — buildSafeMath docstring says "shared with runner/evaluator.ts" — actually duplicated (comment-analyzer)
The comment says "Shared with runner/evaluator.ts (Review #4)." This is factually incorrect. The runner's evaluator.ts has its own private buildSafeMath() function. The two are duplicated implementations, not shared code.
Fix: Remove "(Review #4)" or revise to: "Builds the same safe Math subset as runner/evaluator.ts. Both read from ALLOWED_MATH_MEMBERS." Better yet, extract into a shared utility.
There was a problem hiding this comment.
Fixed in 5123bec. Updated docstring to: "Builds the same safe Math subset as runner/evaluator.ts. Both read from ALLOWED_MATH_MEMBERS."
| autoPlayRef.current = enabled | ||
| }, []) | ||
|
|
||
| // Review #22: chained setTimeout instead of setInterval for auto-play |
There was a problem hiding this comment.
🟡 Finding 19 — Review trace comments (Review #N) should be removed (comment-analyzer)
This file and others contain ~15 comments referencing PR review numbers (e.g., Review #22, Review #27, Review #19, Review #38). These are opaque references that future maintainers cannot look up. The comments should either explain the why without the review number, or be removed if the code is self-explanatory.
Examples across the PR:
// Review #22: chained setTimeout instead of setInterval→ explain WHY (prevents timer drift)// Review #27: byte-size guard→ remove (code is self-evident)// Review #13: Snapshot results before branching→ keep the explanation, drop the review number
There was a problem hiding this comment.
Fixed in 5123bec. Removed all 14 Review #N references across 6 files. Kept useful context where present, removed bare references where code is self-evident.
| } else if (isSwitchNode(node)) { | ||
| step = await handlers.onSwitch(currentNodeId, node, ctx) | ||
| } else if (isParallelNode(node)) { | ||
| const walkBranch = async (startId: string): Promise<TStep[]> => { |
There was a problem hiding this comment.
🟡 Finding 20 — walkBranch comment contradicts itself (comment-analyzer)
The comments say "Each branch gets its own steps array but shares the results map" followed by "Snapshot results before branching (Review #13)." These contradict each other — the code does new Map(results) which creates a copy, meaning branches do NOT share the results map.
Fix: Revise to: "Each branch gets its own steps array and a snapshot of the results map (so branches cannot observe each other's mutations)." Remove the (Review #13) reference.
There was a problem hiding this comment.
Fixed in 5123bec. Revised to: "Each branch gets its own steps array and a snapshot of the results map (so branches cannot observe each other's mutations)."
Error handling cluster: - Add typed ExpressionParseError class to distinguish parse errors from runtime errors in switch case evaluation (Finding 1) - Throw on unknown rule operators instead of silent false (Finding 7) - Promote step-level errors to trace status via hasStepError scan (Baz) - Preserve partial steps on walkGraph crash (Finding 4) - Store trace in hook even when error is present (Finding 5) Quick wins: - Update CLAUDE.md dependency graph to include engine (Finding 2) - Add changeset for engine + editor minor releases (Finding 3) - Fix O(n) → O(n²) docstring in buildTraceSnapshots (Finding 10) - Fix buildSafeMath "shared" → "duplicated" docstring (Finding 18) - Remove all Review #N comment references (Finding 19) - Fix contradictory walkBranch comment (Finding 20)
Deploying flowprint with
|
| Latest commit: |
7231187
|
| Status: | 🚫 Build failed. |
- Wrap SimulationPanel in SimulationErrorBoundary to prevent React tree crashes from rendering errors in the simulation UI (Finding 6) - Add StepNodeType and StepStatus strict literal unions to BaseStep, replacing plain string types for compile-time typo detection — caught missing 'compensation' type during implementation (Finding 11) - Align resolveDotPath with interpreter: throw on blocked property access instead of silently returning undefined (Finding 12) - Replace unbounded parseCache with LRU cache capped at 1000 entries, with cache-hit promotion for true LRU semantics. Export clearParseCache() for lifecycle cleanup (Finding 14) Finding 13 (member access on non-objects) intentionally left as-is — the silent undefined behavior is load-bearing in the rules fallthrough system across 5+ test cases.
walkGraph (9 tests — Finding 8): - Abort signal: pre-aborted and mid-walk cancellation - No root nodes found error path - onUnknownNodeType: undefined (throws) and provided (called) - walkBranch results isolation between parallel branches - maxSteps limit enforcement - startNodeId option (valid and invalid) SimulationPanel (13 tests — Finding 9): - 256KB input size limit enforcement - Invalid JSON input and fixtures validation - Keyboard navigation: ArrowRight, ArrowLeft, Space toggle - Show/Hide fixtures and context viewer toggles - Scrubber interaction with goToStep and auto-play pause
- Rename browser evaluateRules export to evaluateRulesCore to disambiguate from the Node.js version which has different third parameter semantics (Finding 15) - Remove unused rulesData field from WalkContext — it was never set by the walker and belongs on SimulationOptions (Finding 16) - Move ExecutionContext from runner/types.ts to walker/types.ts since it is a shared concept used by rules, simulator, and runner. Re-export from runner/types.ts for import path compatibility (Finding 17)
|
The CI/CD build failure (Cloudflare Pages) is a pre-existing issue with Vite's Rollup not resolving the |
1 participant
User description
Summary
walkGraph<TStep>skeleton powers both Node.js runner and browser simulator. Browser entry point uses tree-walk AST interpreter (nonode:vm). Pure rules evaluation core extracted for reuse.Test Plan
node:imports in browser entry pointGenerated description
Ship the interactive
SimulationPanelflow,useSimulationhook, and editor simulation context so the header can toggle simulation, node highlighting surfaces insideFlowprintEditor, and Playwright/e2e checks cover panel lifecycle, fixture/error handling, and keyboard shortcuts. Establish a shared engine walker, browser-safe simulator entry, and pure rules/expression core sosimulateGraphandrunGraphreuse the same traversal logic withoutnode:vm, keeping the browser bundle lean while syncing evaluator/resolution behavior.NewBlueprintWizardto use the new modal layout and exposing reusable modal/field/button tokens instyles/app.css, giving lane rows, inputs, and action buttons consistent spacing/variants.Modified files (2)
Latest Contributors(1)
Modified files (1)
Latest Contributors(1)
Simulatebutton, wiringApptouseSimulation, surfacing highlights viaSimulationProvider/NodeShell, and building theSimulationPanel(with fixtures, stepper, rules/expression detail, context viewer, shortcuts, and Playwright coverage) so documents can be stepped through visually.Modified files (12)
Latest Contributors(1)
walkGraphskeleton/types/index, exposing browser-friendly APIs/types inbrowser.ts, introducing the AST-based interpreter/rules core/loader, and expanding simulator, runner, and conformance tests so bothrunGraphandsimulateGraphreuse the same safe evaluation/step-tracking logic (plus dependency updates and build config).Modified files (26)
Latest Contributors(1)