feat(web): Archon Studio visual workflow builder (PR-2)#2015
Merged
Wirasm merged 13 commits intoJun 27, 2026
Conversation
…r polish (PR-2) Port the standalone archon-workflow-studio visual editor into the console builder experiment, layered on PR-1's data model (coleam00#1870): - flow/: BuilderWorkflow <-> xyflow bridge (positionless-node seam; positions stay UI-only) + local dagre layout - yaml/: hand-rolled serializeToYaml (pure, DOM-free, golden-tested) rendered by a read-only CodeMirror preview themed to console tokens - editor/: pure kernels — undo/redo history (~400ms sliding coalesce), versioned clipboard envelope (id remap + internal-edge rewiring), align/distribute/auto-arrange, smart-guide snap math, reducer, and vim-flavored keymap bindings over the console useKeymap - components/: canvas (drop/connect/marquee/grid-snap/smart guides), node renderer with per-variant --node-* stripes, palette, inspector with per-variant sub-forms + structured when: builder, validation panel (click-to-select), YAML preview, toolbar - BuilderPage: controlled assembly (initialWorkflow prop, onChange) — the PR-3 seam; no skill verbs, no store/cache.ts, no server I/O - fixture-backed /console/builder route + rail "Workflow Builder" entry with Beta pill; builder section on /console/_preview - two console-scoped node tokens (--node-script, --node-cancel) - deps: CodeMirror YAML-preview packages only (@codemirror/*, @lezer/highlight, @uiw/react-codemirror) 119 pure-logic bun:test units under builder/ (no DOM, no mock.module). Part of coleam00#1863 (PR 2 of 3). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
.gitignore excludes .agents/ entirely, but the ESLint flat config only ignored .agents/examples/** — any other local .agents content (e.g. an installed skill with .mjs scripts) crashed typed linting with "rule requires type information". Align the ESLint ignore with .gitignore. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes the 8 CONFIRMED findings from the high-effort code review (.agents/code-reviews/studio-builder-pr2-code-review.md, local-only): - remove-selection: atomic reducer action replaces the remove-edges + remove-nodes double dispatch — one history entry, one undo restores a mixed node+edge deletion. Edges are matched by CONSTRUCTING edgeId() against depends_on pairs; edge-id strings are never parsed, so no id spelling can silently no-op a removal. - rename-node validates ids against the engine grammar (NODE_ID_PATTERN); the Inspector surfaces the rejection inline. - BuilderPage keymap bindings are now referentially stable (the reducer reads selection itself), so useKeymap no longer re-registers and wipes an in-progress chord buffer on every selection change. - onChange tracks the last-reported workflow instead of a fired-once ref — no spurious initial callback under StrictMode double-effects. - align/distribute receive measured node sizes from the live canvas so toolbar alignment agrees with smart-guide snapping (constants remain the pre-measurement fallback). - YAML quoteIfAmbiguous covers scientific/hex/octal numbers, .inf/.nan, case-insensitive true/false/null/~, and YAML 1.1 yes/no/on/off. - keymap prefix contract documented in lib/keymap.ts and builder/editor/keymap.ts (builder owns the g-chord prefix; full buffer unification deferred). - removed dead clipboard exports serializeEnvelope/parseEnvelope (in-memory clipboard has no JSON codec caller — YAGNI). New editor/state.test.ts covers remove-selection atomicity, rename validation, and measured-size alignment; serialize.test.ts covers the quoting matrix. 252 web tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…udio-builder-pr2-visual-ui
… (builder)
Two issues found while running the builder UI:
1. Edges (connectors) could not be selected, so they could not be
deleted. Root cause: in React Flow v12 controlled mode,
triggerEdgeChanges/triggerNodeChanges only mutate the store directly
for uncontrolled (defaultEdges) flows — otherwise selection is
delivered *only* through onEdgesChange/onNodesChange `select` changes.
Our onEdgesChange was a no-op and onNodesChange dropped everything but
position changes, so edge selection never registered and node
click-selection only worked incidentally (you can also drag a node).
Fix: forward `select` changes from both handlers into a new
`apply-selection` reducer action that merges per-element deltas into
the canonical selection sets. Removed the onSelectionChange channel
(it can't bootstrap selection in controlled mode) and the page-level
setsEqual echo guard it required. Edge removal still flows through the
keymap Delete → remove-selection.
2. 'p' opened the project palette instead of panning. xyflow already
binds hold-Space to pan (panActivationKeyCode default 'Space'); it was
just undiscoverable. Set panActivationKeyCode explicitly and added a
"Builder · canvas" section to the ? overlay documenting Space-drag pan,
marquee select, Shift-click add, and click-edge-to-select. ('p'
belongs to the ConsoleApp global keymap — see the keymap prefix
contract; suppressing globals on the builder route is a separate
follow-up.)
New state.test.ts cases cover apply-selection merge, edge-select →
deletable, and select:false / no-op delta handling. 293 web tests pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The console builder's CodeMirror YAML preview rendered on a white background: @uiw/react-codemirror injects its default *light* theme, whose .cm-editor background overrode consoleTheme()'s dark surface, leaving near-white text on white. Pass theme="none" so only the console dark theme applies, and lift string/punctuation tiers so values read clearly against the inset background. The minimap was empty because this is a *controlled* xyflow graph whose onNodesChange forwards only select/position changes and drops the dimensions changes — so measured never lands on the nodes and the MiniMap skips every node (nodeHasDimensions === false). Seed initialWidth/initialHeight in to-flow.ts; the live DOM measurement still drives the real rendered node height on the canvas. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The builder canvas had no custom context menu, so right-click fell through to the browser's native menu. Add a dependency-free floating menu (BuilderContextMenu) wired through onPaneContextMenu / onNodeContextMenu / onEdgeContextMenu: - Pane: Add node here (submenu, places at the cursor), Paste (disabled when the clipboard is empty), Select all, Auto-arrange, Fit view. - Node: Cut, Copy, Duplicate, Delete. - Edge: Delete connector. Right-clicking an unselected element replaces the selection with just it; right-clicking inside a multi-selection keeps it. All actions route through the existing editor reducer. The menu clamps to the viewport, flips submenus near the edge, and dismisses on outside-click / Escape / scroll / resize. panOnDrag drops the right mouse button ([1,2] -> [1]) so the right button is free for the menu; middle-drag and Space+drag still pan. The right-click gesture is documented in the keymap help overlay. Also carries the edge-selection visual: a selected connector now renders with the accent stroke at 2px (the reducer already tracked the selection; the inline edge style was hiding it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The context menu opened under synthetic dispatch but not a real mouse right-click. Root cause: React Flow gates `onPaneContextMenu` through its internal `wrapHandler`, which fires ONLY when `event.target` is exactly the `.react-flow__pane` element. A real cursor right-click lands on whatever child is topmost (the background dots, the viewport, a node's inner text), so React Flow silently dropped it — while a dispatch aimed straight at the pane element always passed. Move contextmenu handling off React Flow's gated onPaneContextMenu / onNodeContextMenu / onEdgeContextMenu onto a single onContextMenu on the canvas wrapper div. It catches the bubbled event for every right-click in the canvas and classifies node / edge / pane by walking the DOM (.react-flow__node / .react-flow__edge data-id), so the target element no longer matters. Mini-map and controls keep their native menu. This matches the standalone studio's working canvas, which never let panOnDrag claim the right button; the integration's earlier panOnDrag [1,2] was the original regression (fixed to [1] in the prior commit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…click The builder canvas context menu opened and was immediately dismissed by the same right-click that opened it. React 18 flushes discrete events synchronously, so the menu mounted and registered its window dismiss listeners mid-event; the same `contextmenu` event then finished bubbling to `window`, hit the just-attached listener, and closed the menu (~11ms later). This only reproduced with real hardware input — synthetic dispatchEvent does not trigger React's synchronous discrete flush, which is why it passed automated testing but failed for real cursor clicks. Defer attaching the pointer/contextmenu/scroll/resize dismissers by a macrotask so the opening event fully settles first; keydown (Escape) still attaches immediately. Apply the same hardening to the execution graph canvas (WorkflowCanvas) for consistency. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a fixture-backed ChangesVisual Workflow Builder (PR-2)
Sequence Diagram(s)sequenceDiagram
participant BuilderRoute
participant BuilderPage
participant editorReducer
participant BuilderCanvas
participant Inspector
participant YamlPreview
participant IssueList
BuilderRoute->>BuilderPage: initialWorkflow from fixture
BuilderPage->>editorReducer: createEditorState(initialWorkflow)
BuilderPage->>BuilderCanvas: nodes, edges, callbacks
BuilderCanvas-->>BuilderPage: move, connect, select, context actions
BuilderPage->>Inspector: selected node + patch/rename callbacks
BuilderPage->>YamlPreview: yamlText
BuilderPage->>IssueList: validation issues
IssueList-->>BuilderPage: onSelectNode
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
🤖 Multi-Agent Review Summary — Archon Studio Visual Workflow Builder (PR-2)7 specialized agents · 50 files · +5,348/−39 Critical Issues (0 confirmed)Two agent-flagged "criticals" were verified and downgraded: a claimed compile error ( Important Issues (5)
Suggestions (high-value subset)
Documentation Issues (2)
Strengths
VerdictNEEDS FIXES — no merge blockers, but address the 5 Important items (especially surfacing dropped import Recommended Actions
Generated by a 7-agent review (code-reviewer, docs-impact, pr-test-analyzer, comment-analyzer, silent-failure-hunter, type-design-analyzer, code-simplifier). Advisory — all findings reviewed and the two "critical" flags verified/downgraded before posting. |
…expand tests (builder) Resolve the 5 important findings from the multi-agent review of PR coleam00#2015, implement the high-value suggestions, and correct the documentation: Important items - Surface dropped import issues: add `importWorkflowDefinition(def, label)` that console.warns when `fromWorkflowDefinition().issues` is non-empty, and use it at both fixture-backed call sites (BuilderRoute, PreviewPage) instead of discarding `.issues` — import degradation (unknown variant → empty prompt, missing script `runtime` → `bun`) is now visible. - Duplicate-id rename feedback: Inspector.commitRename now detects a collision against other node ids and shows an inline error, instead of the reducer's silent no-op. - Guard the drag-and-drop variant cast: add `isVariantId()` and validate the `dataTransfer` payload in BuilderCanvas.handleDrop before dispatching, so a foreign drag with the same MIME key can't throw in `defaultData()`. - Expand editorReducer tests: cover add-node, patch-node, remove-nodes cascade, add-edge guards, move-nodes, select-all, copy/cut/paste, distribute, auto-arrange, undo/redo, and duplicate-id rename (state.ts now 100%). - Confirm validation pure-fn coverage (graph cycles/refs, content output-ref scan) — already tested; added isVariantId tests. Suggestions - Visibility for silent position fallbacks (to-flow origin, paste {40,40}). - `wireKeys: readonly (keyof WireDagNode)[]` for compile-time drift safety. - `ReactNode` named import; `readonly` History scalars; detectVariant JSDoc "unsafe outside issue-collection"; clarifying comment on the align label/centerline-axis mapping. Docs - experiments/README.md: builder now mounts at /console/builder (was "no route mount yet"). - docs-web adapters/web.md: document the beta /console/builder builder. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
✅ Review fixes applied —
|
| # | Issue | Fix |
|---|---|---|
| 1 | fromWorkflowDefinition().issues dropped at both call sites |
New importWorkflowDefinition(def, label) console.warns non-empty import issues; BuilderRoute.tsx + PreviewPage.tsx now use it instead of discarding .issues. Unknown-variant→empty-prompt and missing-runtime→bun degradation is now visible. |
| 2 | Rename to a duplicate id silently no-ops | Inspector.commitRename now checks the id against the other node ids and shows an inline error (Another node already uses the id '…') before the reducer's silent guard. |
| 3 | Unvalidated dataTransfer cast → throws on foreign drag |
Added isVariantId() guard; BuilderCanvas.handleDrop validates the payload against the registry before dispatching — a stray drop with the same MIME key is ignored instead of crashing in defaultData(). |
| 4 | editorReducer — 14/18 action cases untested |
Added 22 tests covering add-node, patch-node, remove-nodes (dep cascade), add-edge guards (self-loop / unknown / duplicate), move-nodes, select-all, copy/cut/paste, distribute, auto-arrange, undo/redo, and duplicate-id rename. state.ts now at 100% line coverage. |
| 5 | Validation pure fns "zero coverage" | Verified already covered — validation/graph.test.ts (cycle + ref detection), content.test.ts ($nodeId.output scan), structural.test.ts, and validate.test.ts exercise these. The flagged useBuilderValidation.ts / getInstantIssues/getDebouncedIssues symbols don't exist; validation lives in tested modules under validation/ behind runValidation(). Added isVariantId tests alongside. |
Suggestions implemented
- Silent position fallbacks now
console.warn(to-flow.tsorigin fallback,state.tspaste{40,40}— the latter is genuinely reachable since the clipboard omits position-less nodes). wireKeys: readonly (keyof WireDagNode)[]for compile-time drift safety (call site widens tostring[]for the membership test).ReactNodenamed import inBuilderNodeView(matches the file's import style).readonlyHistory scalars (lastKind/lastTime) for immutability parity with the arrays.detectVariantJSDoc now warns it's unsafe outside issue-collection contexts (usedetectVariantOrNull).- Align label/mode clarifying comment — the
centerH/centerV↔ "horizontal/vertical centers" pairing reads inverted but is correct (mode = centerline axis, label = coordinate equalized); documented rather than changed (behavior is self-consistent, as the review's downgrade noted).
Deliberately not changed (with rationale)
TRIGGER_RULES"duplicated twice" — the second copy is in theexperiments/console/builderInspector, which is isolation-guarded (ESLint blocks@/components/@/stores/etc.) so it can be extracted or discarded cleanly. Extracting a shared@/libconstant and importing it into the experiment would defeat that decoupling. CLAUDE.md explicitly permits a local web constant. Kept the parallel constant.eslint.config.mjs.agents/**widening — intentional in this branch (commit ignoring gitignored.agents/local content); left as-is.
Documentation
packages/web/src/experiments/README.md— builder entry updated; it now mounts (beta) at/console/builderwith a sidebar entry (was "no route mount yet").packages/docs-web/src/content/docs/adapters/web.md— added a beta/console/buildersubsection documenting the new canvas/inspector/YAML/validation/context-menu and its fixture-backed status, alongside the existing/legacy/workflows/builder.
Validation
bun --filter @archon/web type-check # ✅ exit 0
bun x eslint <touched files> # ✅ 0 warnings
bun x prettier --check <touched> # ✅ all formatted
bun --filter @archon/web test # ✅ 489 pass / 0 fail (151 builder)
🤖 Generated with Claude Code
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the Archon Studio visual workflow builder to the experimental console (/console/builder) as a fixture-backed, controlled React Flow editor surface, plus hardens context-menu dismissal behavior to avoid the open→immediate-close race in React 18.
Changes:
- Introduces the builder route + preview surface, with palette/canvas/inspector/validation panel and YAML preview.
- Adds pure editor/flow/YAML utilities (undo/redo history, clipboard envelope, align/distribute, smart guides, dagre layout) with
bun:testcoverage. - Fixes context-menu dismissal timing (deferred listener attachment) in both the new builder and the legacy
WorkflowCanvas.
Reviewed changes
Copilot reviewed 57 out of 58 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/web/src/experiments/README.md | Updates experiments inventory to reflect the mounted builder route and PR-2 scope. |
| packages/web/src/experiments/console/theme.css | Adds console-scoped node stripe tokens for script/cancel. |
| packages/web/src/experiments/console/routes/PreviewPage.tsx | Adds fixture-backed builder preview section to /console/_preview. |
| packages/web/src/experiments/console/lib/keymap.ts | Clarifies keymap prefix ownership/reservations across mounted keymaps. |
| packages/web/src/experiments/console/ConsoleApp.tsx | Mounts /console/builder route. |
| packages/web/src/experiments/console/components/ProjectRail.tsx | Adds “Workflow Builder” nav link (beta pill) to console rail. |
| packages/web/src/experiments/console/builder/yaml/serialize.ts | Implements pure YAML serializer for live preview output. |
| packages/web/src/experiments/console/builder/yaml/serialize.test.ts | Adds golden + edge-case tests for YAML serialization. |
| packages/web/src/experiments/console/builder/yaml/index.ts | Re-exports YAML serialization API. |
| packages/web/src/experiments/console/builder/variants/registry.ts | Tightens variant wire-key typing + adds isVariantId guard. |
| packages/web/src/experiments/console/builder/variants/registry.test.ts | Tests isVariantId acceptance/rejection. |
| packages/web/src/experiments/console/builder/variants/index.ts | Re-exports isVariantId. |
| packages/web/src/experiments/console/builder/variants/detect.ts | Documents safe vs unsafe variant detection usage. |
| packages/web/src/experiments/console/builder/README.md | Updates builder README for PR-2 editor surface and dependency layering. |
| packages/web/src/experiments/console/builder/model/index.ts | Exposes importWorkflowDefinition from model entrypoint. |
| packages/web/src/experiments/console/builder/model/from-workflow.ts | Adds importWorkflowDefinition helper that logs import issues to console. |
| packages/web/src/experiments/console/builder/flow/types.ts | Adds canvas-side xyflow types + node data payload shape. |
| packages/web/src/experiments/console/builder/flow/to-flow.ts | Maps BuilderWorkflow → xyflow nodes/edges with dagre fallback layout. |
| packages/web/src/experiments/console/builder/flow/layout.ts | Adds local dagre layout helper (console-isolated). |
| packages/web/src/experiments/console/builder/flow/index.ts | Re-exports flow bridge utilities. |
| packages/web/src/experiments/console/builder/flow/from-flow.ts | Maps xyflow nodes/edges → BuilderWorkflow, preserving dangling deps. |
| packages/web/src/experiments/console/builder/flow/flow.test.ts | Tests flow mapping/round-trip + dagre layout behavior. |
| packages/web/src/experiments/console/builder/editor/state.ts | Adds editor reducer/state (positions/selection/history/clipboard). |
| packages/web/src/experiments/console/builder/editor/smart-guides.ts | Adds pure smart-guide snap math for node dragging. |
| packages/web/src/experiments/console/builder/editor/smart-guides.test.ts | Tests smart-guide snapping and guide emission. |
| packages/web/src/experiments/console/builder/editor/keymap.ts | Defines builder key bindings + help overlay groups from one source table. |
| packages/web/src/experiments/console/builder/editor/history.ts | Adds pure undo/redo history with deterministic coalescing. |
| packages/web/src/experiments/console/builder/editor/history.test.ts | Tests history coalescing/undo/redo semantics. |
| packages/web/src/experiments/console/builder/editor/clipboard.ts | Implements copy/cut/paste envelope with id remap and position offsets. |
| packages/web/src/experiments/console/builder/editor/clipboard.test.ts | Tests clipboard envelope remap/rewire/position behavior. |
| packages/web/src/experiments/console/builder/editor/align.ts | Adds pure align/distribute kernels (plus dagre auto-arrange wrapper). |
| packages/web/src/experiments/console/builder/editor/align.test.ts | Tests align/distribute kernels and auto-arrange delegation. |
| packages/web/src/experiments/console/builder/components/YamlPreview.tsx | Adds read-only CodeMirror YAML pane + copy button. |
| packages/web/src/experiments/console/builder/components/WhenBuilder.tsx | Adds structured when: editor over the PR-1 grammar. |
| packages/web/src/experiments/console/builder/components/Toolbar.tsx | Adds builder toolbar for history/clipboard/arrange/view/help actions. |
| packages/web/src/experiments/console/builder/components/SmartGuides.tsx | Renders smart-guide helper lines via xyflow portal. |
| packages/web/src/experiments/console/builder/components/NodePalette.tsx | Adds draggable/clickable node palette (variant tiles). |
| packages/web/src/experiments/console/builder/components/IssueList.tsx | Adds validation panel with severity grouping + node-select affordance. |
| packages/web/src/experiments/console/builder/components/inspector/ScriptFields.tsx | Inspector controls for script node fields. |
| packages/web/src/experiments/console/builder/components/inspector/PromptFields.tsx | Inspector controls for prompt node fields. |
| packages/web/src/experiments/console/builder/components/inspector/LoopFields.tsx | Inspector controls for loop node fields. |
| packages/web/src/experiments/console/builder/components/inspector/fields.tsx | Shared inspector form primitives (text/textarea/number/checkbox/select). |
| packages/web/src/experiments/console/builder/components/inspector/CommandFields.tsx | Inspector controls for command node fields. |
| packages/web/src/experiments/console/builder/components/inspector/CancelFields.tsx | Inspector controls for cancel node fields. |
| packages/web/src/experiments/console/builder/components/inspector/BashFields.tsx | Inspector controls for bash node fields. |
| packages/web/src/experiments/console/builder/components/inspector/ApprovalFields.tsx | Inspector controls for approval node fields (incl. on_reject). |
| packages/web/src/experiments/console/builder/components/Inspector.tsx | Adds inspector assembly (rename/id validation, base fields, per-variant fields). |
| packages/web/src/experiments/console/builder/components/BuilderNodeView.tsx | Adds custom node renderer for builder nodes (stripe/badges/pills). |
| packages/web/src/experiments/console/builder/components/BuilderContextMenu.tsx | Adds context menu with deferred outside-dismiss listeners to avoid race. |
| packages/web/src/experiments/console/builder/components/BuilderCanvas.tsx | Adds controlled ReactFlow canvas with snapping, selection deltas, context menu. |
| packages/web/src/experiments/console/builder/BuilderRoute.tsx | Adds fixture-backed /console/builder route wrapper around BuilderPage. |
| packages/web/src/experiments/console/builder/BuilderPage.tsx | Wires reducer, keymap, canvas, inspector, validation, YAML preview into one surface. |
| packages/web/src/components/workflows/WorkflowCanvas.tsx | Applies the same deferred-dismiss listener fix to legacy workflow canvas menu. |
| packages/web/package.json | Adds CodeMirror-related dependencies for YAML preview. |
| packages/docs-web/src/content/docs/adapters/web.md | Documents new console builder (beta) and its fixture-backed scope. |
| eslint.config.mjs | Broadens ignore from .agents/examples/** → .agents/**. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+69
to
+73
| const field = e.target.value.trim(); | ||
| // Editing the field through the builder normalizes to the canonical | ||
| // `$node.output.field` spelling (shorthand flag dropped). | ||
| const next: AtomNode = { nodeId: atom.nodeId, op: atom.op, value: atom.value }; | ||
| if (field.length > 0) next.field = field; |
Comment on lines
+104
to
+121
| const copy = (): void => { | ||
| navigator.clipboard.writeText(yamlText).then( | ||
| () => { | ||
| setCopied('copied'); | ||
| setTimeout(() => { | ||
| setCopied('idle'); | ||
| }, 1500); | ||
| }, | ||
| () => { | ||
| // Clipboard API can be unavailable (permissions, insecure context); | ||
| // surface the failure inline instead of swallowing it. | ||
| setCopied('failed'); | ||
| setTimeout(() => { | ||
| setCopied('idle'); | ||
| }, 1500); | ||
| } | ||
| ); | ||
| }; |
Comment on lines
+217
to
+225
| const selectedNodes = new Set(state.selectedNodes); | ||
| if (selectedNodes.delete(action.id)) selectedNodes.add(nextId); | ||
| return { | ||
| ...state, | ||
| history: remember(state, 'rename-node', action.at), | ||
| workflow: { ...state.workflow, nodes }, | ||
| positions, | ||
| selectedNodes, | ||
| }; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
packages/web/src/experiments/console/builder/editor/state.test.ts (1)
399-423: ⚡ Quick winAdd undo/redo assertions for selection validity.
Current undo/redo tests verify workflow restoration but not selection invariants. Add checks that selected ids are valid (or cleared) after undo/redo to prevent stale-selection regressions.
Proposed test extension
test('undo restores the pre-edit workflow; redo reapplies it', () => { const initial = mixedState(); const added = editorReducer(initial, { type: 'add-node', variant: 'prompt', position: { x: 0, y: 0 }, at: 1000, }); expect(added.workflow.nodes.length).toBe(4); const undone = editorReducer(added, { type: 'undo' }); expect(undone.workflow.nodes.length).toBe(3); expect(undone.workflow.nodes.map(n => n.id)).toEqual(['classify', 'fix', 'report']); + expect(undone.selectedNodes.size).toBe(0); + expect(undone.selectedEdges.size).toBe(0); const redone = editorReducer(undone, { type: 'redo' }); expect(redone.workflow.nodes.length).toBe(4); + for (const id of redone.selectedNodes) { + expect(redone.workflow.nodes.some(n => n.id === id)).toBe(true); + } });🤖 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 `@packages/web/src/experiments/console/builder/editor/state.test.ts` around lines 399 - 423, In the undo/redo integration tests, add assertions to verify that the selection state remains valid after undo/redo operations. In the first test case "undo restores the pre-edit workflow; redo reapplies it", after each editorReducer call with undo/redo actions, assert that the selectedNodeIds in the resulting state only reference node ids that actually exist in the workflow.nodes array, preventing stale selection references. In the second test case "undo and redo are no-ops at the ends of the history stack", verify that the selection state is properly preserved or cleared when undo/redo operations don't modify the workflow.
🤖 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 `@packages/web/src/experiments/console/builder/components/BuilderCanvas.tsx`:
- Around line 139-161: The snapping logic in the single-node drag branch is
guarded by the dragging state on line 139, causing the final position change
emitted by `@xyflow/react` when dragging ends to skip snapping and emit the raw
position instead. Remove the dragging condition from the if statement so
snapping is always applied regardless of dragging state, but relocate the
setGuides call that updates visual guides to only execute when actively dragging
(check dragging state separately for guide updates). This preserves the snapped
position on drag end while keeping visual guides only visible during active
dragging.
In
`@packages/web/src/experiments/console/builder/components/BuilderContextMenu.tsx`:
- Around line 154-163: Replace the hard-coded font size values in the className
attribute with brand typography tokens as required by coding guidelines. The
arbitrary Tailwind values text-[12.5px] on the main menu item and text-[10.5px]
on the hint span should be replaced with approved tokenized typography utilities
from the design system. Also apply the same changes to the other occurrences
mentioned at lines 202-203 and 230 in the same BuilderContextMenu component to
ensure consistent use of brand tokens throughout the file instead of ad-hoc CSS
values.
In `@packages/web/src/experiments/console/builder/components/BuilderNodeView.tsx`:
- Around line 58-59: The BuilderNodeView component uses ad-hoc pixel-literal
typography sizes like text-[9px] and text-[10px] in className attributes instead
of brand token typography classes. Replace all instances of inline text sizing
(text-[9px], text-[10px], etc.) in the className properties throughout the
BuilderNodeView component with the appropriate brand token typography classes
from your design system tokens.
In `@packages/web/src/experiments/console/builder/editor/state.ts`:
- Around line 430-449: When undoing or redoing state changes, the selectedNodes
and selectedEdges are not being cleared after the snapshot is restored, which
can leave selections referencing entities that no longer exist in the restored
workflow. In both the 'undo' and 'redo' cases, after spreading the restored
snapshot properties (workflow and positions), also reset selectedNodes and
selectedEdges to empty arrays or appropriate empty states to ensure the
selection is consistent with the restored workflow structure.
- Around line 197-210: The rename-node case validates the new ID (nextId) but
does not verify that the source ID (action.id) being renamed actually exists in
the workflow. Add a guard clause early in the rename-node case that returns the
state unchanged if action.id is not found in nodeIds(state.workflow), similar to
the existing guards that check nextId validity. This prevents unintended
modifications to depends_on references and history when attempting to rename
non-existent nodes.
---
Nitpick comments:
In `@packages/web/src/experiments/console/builder/editor/state.test.ts`:
- Around line 399-423: In the undo/redo integration tests, add assertions to
verify that the selection state remains valid after undo/redo operations. In the
first test case "undo restores the pre-edit workflow; redo reapplies it", after
each editorReducer call with undo/redo actions, assert that the selectedNodeIds
in the resulting state only reference node ids that actually exist in the
workflow.nodes array, preventing stale selection references. In the second test
case "undo and redo are no-ops at the ends of the history stack", verify that
the selection state is properly preserved or cleared when undo/redo operations
don't modify the workflow.
🪄 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
Run ID: ce012644-cfab-40f4-b403-3128cb7d5e3d
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (57)
eslint.config.mjspackages/docs-web/src/content/docs/adapters/web.mdpackages/web/package.jsonpackages/web/src/components/workflows/WorkflowCanvas.tsxpackages/web/src/experiments/README.mdpackages/web/src/experiments/console/ConsoleApp.tsxpackages/web/src/experiments/console/builder/BuilderPage.tsxpackages/web/src/experiments/console/builder/BuilderRoute.tsxpackages/web/src/experiments/console/builder/README.mdpackages/web/src/experiments/console/builder/components/BuilderCanvas.tsxpackages/web/src/experiments/console/builder/components/BuilderContextMenu.tsxpackages/web/src/experiments/console/builder/components/BuilderNodeView.tsxpackages/web/src/experiments/console/builder/components/Inspector.tsxpackages/web/src/experiments/console/builder/components/IssueList.tsxpackages/web/src/experiments/console/builder/components/NodePalette.tsxpackages/web/src/experiments/console/builder/components/SmartGuides.tsxpackages/web/src/experiments/console/builder/components/Toolbar.tsxpackages/web/src/experiments/console/builder/components/WhenBuilder.tsxpackages/web/src/experiments/console/builder/components/YamlPreview.tsxpackages/web/src/experiments/console/builder/components/inspector/ApprovalFields.tsxpackages/web/src/experiments/console/builder/components/inspector/BashFields.tsxpackages/web/src/experiments/console/builder/components/inspector/CancelFields.tsxpackages/web/src/experiments/console/builder/components/inspector/CommandFields.tsxpackages/web/src/experiments/console/builder/components/inspector/LoopFields.tsxpackages/web/src/experiments/console/builder/components/inspector/PromptFields.tsxpackages/web/src/experiments/console/builder/components/inspector/ScriptFields.tsxpackages/web/src/experiments/console/builder/components/inspector/fields.tsxpackages/web/src/experiments/console/builder/editor/align.test.tspackages/web/src/experiments/console/builder/editor/align.tspackages/web/src/experiments/console/builder/editor/clipboard.test.tspackages/web/src/experiments/console/builder/editor/clipboard.tspackages/web/src/experiments/console/builder/editor/history.test.tspackages/web/src/experiments/console/builder/editor/history.tspackages/web/src/experiments/console/builder/editor/keymap.tspackages/web/src/experiments/console/builder/editor/smart-guides.test.tspackages/web/src/experiments/console/builder/editor/smart-guides.tspackages/web/src/experiments/console/builder/editor/state.test.tspackages/web/src/experiments/console/builder/editor/state.tspackages/web/src/experiments/console/builder/flow/flow.test.tspackages/web/src/experiments/console/builder/flow/from-flow.tspackages/web/src/experiments/console/builder/flow/index.tspackages/web/src/experiments/console/builder/flow/layout.tspackages/web/src/experiments/console/builder/flow/to-flow.tspackages/web/src/experiments/console/builder/flow/types.tspackages/web/src/experiments/console/builder/model/from-workflow.tspackages/web/src/experiments/console/builder/model/index.tspackages/web/src/experiments/console/builder/variants/detect.tspackages/web/src/experiments/console/builder/variants/index.tspackages/web/src/experiments/console/builder/variants/registry.test.tspackages/web/src/experiments/console/builder/variants/registry.tspackages/web/src/experiments/console/builder/yaml/index.tspackages/web/src/experiments/console/builder/yaml/serialize.test.tspackages/web/src/experiments/console/builder/yaml/serialize.tspackages/web/src/experiments/console/components/ProjectRail.tsxpackages/web/src/experiments/console/lib/keymap.tspackages/web/src/experiments/console/routes/PreviewPage.tsxpackages/web/src/experiments/console/theme.css
Comment on lines
+154
to
+163
| className={`flex w-full items-center justify-between gap-6 px-3 py-1.5 text-left text-[12.5px] transition-colors disabled:cursor-not-allowed disabled:opacity-40 ${ | ||
| entry.danger === true | ||
| ? 'text-[var(--error)] hover:bg-surface-hover' | ||
| : 'text-text-secondary hover:bg-surface-hover hover:text-text-primary' | ||
| }`} | ||
| > | ||
| <span>{entry.label}</span> | ||
| {entry.hint !== undefined ? ( | ||
| <span className="font-mono text-[10.5px] text-text-tertiary">{entry.hint}</span> | ||
| ) : null} |
There was a problem hiding this comment.
Replace ad-hoc font sizes with brand typography tokens.
This introduces hard-coded typography values (text-[12.5px], text-[10.5px]) in a new UI surface. Please switch these to approved tokenized typography utilities/styles for brand consistency.
As per coding guidelines, packages/web/src/**/*.{tsx,css} must “Use brand tokens, not ad-hoc values — ... typography must come from design tokens ...”.
Also applies to: 202-203, 230-230
🤖 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
`@packages/web/src/experiments/console/builder/components/BuilderContextMenu.tsx`
around lines 154 - 163, Replace the hard-coded font size values in the className
attribute with brand typography tokens as required by coding guidelines. The
arbitrary Tailwind values text-[12.5px] on the main menu item and text-[10.5px]
on the hint span should be replaced with approved tokenized typography utilities
from the design system. Also apply the same changes to the other occurrences
mentioned at lines 202-203 and 230 in the same BuilderContextMenu component to
ensure consistent use of brand tokens throughout the file instead of ad-hoc CSS
values.
Source: Coding guidelines
Comment on lines
+58
to
+59
| className="shrink-0 rounded px-1.5 py-0.5 text-[9px] font-semibold uppercase" | ||
| style={badgeStyle} |
There was a problem hiding this comment.
Use tokenized typography classes instead of pixel-literal text sizes.
The node view currently uses ad-hoc typography sizing (text-[9px], text-[10px]) on core labels/pills. Please replace these with approved brand token typography styles.
As per coding guidelines, packages/web/src/**/*.{tsx,css} requires “Use brand tokens, not ad-hoc values — ... typography must come from design tokens ...”.
Also applies to: 69-69, 90-90
🤖 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 `@packages/web/src/experiments/console/builder/components/BuilderNodeView.tsx`
around lines 58 - 59, The BuilderNodeView component uses ad-hoc pixel-literal
typography sizes like text-[9px] and text-[10px] in className attributes instead
of brand token typography classes. Replace all instances of inline text sizing
(text-[9px], text-[10px], etc.) in the className properties throughout the
BuilderNodeView component with the appropriate brand token typography classes
from your design system tokens.
Source: Coding guidelines
Resolve the inline findings from the latest Copilot + CodeRabbit pass on PR coleam00#2015: - state.ts rename-node: guard when the source id does not exist (was recording history + potentially rewriting dangling deps), and clear the now-stale edge selection (edge ids derive from node ids, so a rename invalidates any selected `source->target` id — a later delete would silently miss). - state.ts undo/redo: clear selectedNodes/selectedEdges after restoring a snapshot so the selection can't dangle against a different graph. - BuilderCanvas: apply single-node snapping on the FINAL drag change too (xyflow emits a last `position` change with `dragging:false` on drop; in controlled mode the raw position was overwriting the snapped one). Guides still only render while actively dragging. - YamlPreview: funnel clipboard writes through a `.then` callback so a synchronous TypeError (undefined `navigator.clipboard` in an insecure context) lands in the same rejection handler as an async permission denial — the "Copy failed" state is now reliably surfaced. - WhenBuilder: correct the misleading field-edit comment — it canonicalizes the path (drops the `shorthand` flag) while preserving the `bare` RHS spelling. Tests: added reducer coverage for the rename source-id guard, stale-edge- selection clearing, and undo/redo selection-invariant assertions. Skipped (with rationale): CodeRabbit's "replace ad-hoc text-[Npx] with brand typography tokens" on BuilderContextMenu/BuilderNodeView — the codebase has no font-size design tokens (only font-family + color tokens), and the console experiment uses text-[Npx] uniformly (~407 occurrences). Converting two files in isolation would change rendered sizes and break consistency; a type-scale token system is a systemic change out of scope for this PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
✅ Round-2 review fixes (Copilot + CodeRabbit) —
|
| Source | File | Issue | Fix |
|---|---|---|---|
| CodeRabbit 🟡 | editor/state.ts rename-node |
No guard that the source id exists — a rename of a missing node still recorded history and could rewrite dangling depends_on refs |
Early if (!ids.has(action.id)) return state; (and reuse the id set for the collision check). |
| Copilot | editor/state.ts rename-node |
selectedEdges left untouched on rename; edge ids derive from node ids (source->target), so a stale edge id could make a later delete silently miss |
Clear selectedEdges on rename (chosen over remap — edge ids are never parsed elsewhere). |
| CodeRabbit 🟠 | editor/state.ts undo/redo |
Selection carried over a restored snapshot can reference nodes/edges that no longer exist | Reset selectedNodes/selectedEdges to empty on both. |
| CodeRabbit 🟠 | components/BuilderCanvas.tsx |
xyflow v12 emits a final position change with dragging:false on drop; snapping was guarded by dragging, so the drop position skipped snapping and the raw position overwrote the snapped one |
Snap on every single-node change; only render guides while dragging. |
| Copilot | components/YamlPreview.tsx |
navigator.clipboard.writeText throws synchronously when navigator.clipboard is undefined (insecure context), bypassing the .then rejection handler → "Copy failed" never shown |
Funnel the write through Promise.resolve().then(() => …writeText()) so the sync TypeError and async rejection both hit the same rejection branch. |
| Copilot | components/WhenBuilder.tsx |
Field-edit comment was misleading (mixed up shorthand vs bare) |
Rewrote: it canonicalizes the path (drops the shorthand flag) while preserving the bare RHS spelling. |
Tests added (editor/state.test.ts): rename of a non-existent source id is a no-op; rename clears a stale edge selection while rewiring the dep; undo/redo selection-invariant assertions (selection cleared / never dangling) — per CodeRabbit's nitpick.
Skipped with rationale
CodeRabbit 🟠 — "replace ad-hoc text-[Npx] with brand typography tokens" (BuilderContextMenu.tsx, BuilderNodeView.tsx).
Verified against the codebase before acting (per the AI-agent prompt's "fix only still-valid issues, skip the rest with a brief reason"):
packages/web/src/index.cssdefines font-family tokens (--font-sans,--font-mono) and color tokens (--text-primary/secondary/tertiary→ colors). There is no font-size type scale exposed as tokens or utility classes.- The console experiment uses
text-[Npx]~407 times as its established sizing convention (vs. 27 scattered Tailwind defaults). - Converting just these two files would (a) change the actual rendered sizes (
text-[9px]≠text-xs/12px), and (b) diverge from the other ~405 call sites.
There is nothing to migrate to yet — a typography token/type-scale system for the console is a systemic change, out of scope for this PR. The brand guideline is satisfied at the family/color-token level. Happy to land a dedicated type-scale pass for the whole experiment if desired.
Validation
bun --filter @archon/web type-check # ✅ exit 0
bun x eslint <touched files> # ✅ 0 warnings
bun x prettier --check <touched> # ✅ all formatted
bun test .../builder/ # ✅ 153 pass / 0 fail
🤖 Generated with Claude Code
Contributor
Author
📋 Consolidated review-fix summaryAll review feedback on this PR has been addressed across two commits. Recap of everything fixed: Round 1 — multi-agent review (
|
| Source | Fix |
|---|---|
| CodeRabbit | rename-node now guards a non-existent source id (no spurious history / dep rewrite). |
| Copilot | rename-node clears the stale edge selection (edge ids derive from node ids, so a rename invalidated source->target ids → a later delete could miss). |
| CodeRabbit | undo/redo reset selection so it can't dangle against the restored snapshot. |
| CodeRabbit | BuilderCanvas snaps on the final dragging:false change too — the drop position no longer overwrites the snapped one (guides still only show while dragging). |
| Copilot | YamlPreview clipboard write funneled through .then so a synchronous navigator.clipboard TypeError (insecure context) reliably shows "Copy failed". |
| Copilot | WhenBuilder comment corrected (canonicalizes the path / drops shorthand, preserves bare). |
Added reducer tests for the rename guard, stale-edge clearing, and undo/redo selection invariants.
Skipped (documented): CodeRabbit's "use brand typography tokens" on BuilderContextMenu/BuilderNodeView — the codebase has no font-size token/type-scale (only font-family + color tokens), and the console experiment uses text-[Npx] uniformly (~407 sites). Converting two files in isolation would change rendered sizes and break consistency; a type-scale system is a systemic change out of scope here.
Validation (both rounds)
bun --filter @archon/web type-check # ✅ exit 0
bun x eslint <touched files> # ✅ 0 warnings
bun x prettier --check <touched> # ✅ all formatted
bun --filter @archon/web test # ✅ all pass (153 builder)
🤖 Generated with Claude Code
Contributor
Author
PR #2015 Summary —
|
| Repository | coleam00/Archon |
| PR | #2015 |
| Base branch | dev |
| Head branch | feat/studio-builder-pr2-visual-ui |
| Author | seanrobertwright |
| State | OPEN |
| Diff size | +5,820 / −45 across 58 files |
Note: the request referenced
cole00am/Archon; the actual repository iscoleam00/Archon. PR #2015 there matches the described PR and was used for this summary.
1) Newly created files (not in original codebase)
41 new files.
All but a handful live under the new packages/web/src/experiments/console/builder/ tree (the PR-2 visual editor). Breakdown:
| Area | Count | Files |
|---|---|---|
| Builder page / route | 2 | builder/BuilderPage.tsx, builder/BuilderRoute.tsx |
| Canvas & top-level components | 10 | components/BuilderCanvas.tsx, BuilderContextMenu.tsx, BuilderNodeView.tsx, Inspector.tsx, IssueList.tsx, NodePalette.tsx, SmartGuides.tsx, Toolbar.tsx, WhenBuilder.tsx, YamlPreview.tsx |
| Per-variant inspector sub-forms | 8 | inspector/ApprovalFields.tsx, BashFields.tsx, CancelFields.tsx, CommandFields.tsx, LoopFields.tsx, PromptFields.tsx, ScriptFields.tsx, fields.tsx |
| Editor kernels (+ tests) | 11 | editor/align.ts (+.test.ts), clipboard.ts (+.test.ts), history.ts (+.test.ts), smart-guides.ts (+.test.ts), state.ts (+.test.ts), keymap.ts |
| Flow ↔ xyflow bridge (+ test) | 6 | flow/flow.test.ts, flow/from-flow.ts, flow/index.ts, flow/layout.ts, flow/to-flow.ts, flow/types.ts |
| YAML serializer (+ test) | 3 | yaml/index.ts, yaml/serialize.ts, yaml/serialize.test.ts |
| Variants test | 1 | variants/registry.test.ts |
2) Modified files (original code files modified)
17 modified files.
Of these, 5 are documentation/config (*.md, eslint config), 1 is an auto-generated lockfile (bun.lock), and 11 are source/manifest files. Only ~3 changes alter the behavior or type-contract of pre-existing shipped code; the rest are additive (new exports, new routes, new comments).
3) Per-file modification summary & impact
Documentation & configuration
| File | Change | Impact |
|---|---|---|
packages/docs-web/src/content/docs/adapters/web.md (+6/−0) |
Adds an "Archon Studio builder (beta)" section documenting the new /console/builder route and its beta/fixture-backed status. Pure documentation. |
LOW |
packages/web/src/experiments/README.md (+1/−1) |
Updates the console/builder/ description from "data layer (PR-1)" to "data layer + PR-2 canvas". Documentation only. |
LOW |
packages/web/src/experiments/console/builder/README.md (+65/−21) |
Substantial rewrite documenting PR-2: new directory layout (flow/, yaml/, editor/, components/), layered dependency diagram, test approach, theme tokens, keymap, and the PR-3 seam. Documentation only. |
LOW |
eslint.config.mjs (+1/−1) |
Broadens an ignore glob from .agents/examples/** to .agents/** (gitignored local agent content not in any tsconfig project). Lint-config only; no source affected. |
LOW |
Dependencies / lockfile
| File | Change | Impact |
|---|---|---|
bun.lock (+47/−0) |
Auto-generated. Adds the CodeMirror dependency tree (@codemirror/*, @uiw/react-codemirror, @lezer/*, codemirror, crelt, style-mod, w3c-keyname, etc.) used by the new YAML preview. Purely additive; no existing entries removed. |
LOW |
packages/web/package.json (+15/−8) |
Adds 7 new runtime deps (@codemirror/lang-yaml, @codemirror/language, @codemirror/search, @codemirror/state, @codemirror/view, @lezer/highlight, @uiw/react-codemirror). The −8/+8 churn is mostly alphabetical re-sorting of the existing dependency block; no existing dependency removed. New deps enlarge the bundle/build surface. |
MEDIUM |
Production component behavior
| File | Change | Impact |
|---|---|---|
packages/web/src/components/workflows/WorkflowCanvas.tsx (+9/−2) |
Bug fix to existing production code. Defers attaching the mousedown/contextmenu "click-outside" dismiss listeners to the next macrotask (setTimeout(…, 0)) instead of synchronously, and clears the timer on cleanup. Prevents the just-opened right-click context menu from instantly closing because the opening contextmenu event was still propagating when the listener was added (React 18 synchronous flush). Changes runtime behavior of a shipped component. |
MEDIUM |
Console app wiring (additive)
| File | Change | Impact |
|---|---|---|
packages/web/src/experiments/console/ConsoleApp.tsx (+2/−0) |
Imports BuilderRoute and registers a new <Route path="builder">. Additive route registration; no existing route altered. |
MEDIUM |
packages/web/src/experiments/console/components/ProjectRail.tsx (+18/−3) |
Extends RailNavLink with an optional badge pill prop (and truncate on the label), then adds a new "Workflow Builder" nav entry (PenTool icon, "beta" badge) linking to /console/builder. Backward-compatible signature extension + one new sidebar link. |
MEDIUM |
packages/web/src/experiments/console/routes/PreviewPage.tsx (+53/−1) |
Adds a BuilderPreview fixture-switcher component and a new "Workflow Builder · fixture-backed" section to the preview page, importing BuilderPage, PR-1 FIXTURES, and importWorkflowDefinition. Additive preview surface; no existing preview section changed. |
MEDIUM |
packages/web/src/experiments/console/theme.css (+7/−0) |
Adds two console-scoped CSS custom properties, --node-script and --node-cancel, completing the seven node-variant color stripes (the other five inherit from production :root). Purely additive tokens. |
LOW |
packages/web/src/experiments/console/lib/keymap.ts (+7/−3) |
Comment-only update: clarifies chord-prefix reservation rules (ConsoleApp owns p/?/,; g-chord owned by non-co-mounting route keymaps). No logic change. |
LOW |
Builder data layer — type & API changes (mostly additive, one contract tightening)
| File | Change | Impact |
|---|---|---|
builder/variants/registry.ts (+11/−2) |
Adds exported type-guard isVariantId(value) (narrows untrusted strings, e.g. drag-and-drop payloads). Tightens the VariantRegistryEntry.wireKeys type from readonly string[] to readonly (keyof WireDagNode)[] so a typo/renamed wire field fails to compile. The tightening is a contract change for consumers of wireKeys. |
MEDIUM |
builder/model/from-workflow.ts (+30/−1) |
Two changes: (a) locally re-widens wireKeys back to readonly string[] so the .includes(key) membership test accepts arbitrary wire keys (counterpart to the registry tightening above); (b) adds new exported importWorkflowDefinition(def, label) that wraps fromWorkflowDefinition and console.warns on import issues for routes with no issue panel. Mostly additive; the one changed line in the existing function is a type annotation. |
MEDIUM |
builder/variants/registry.test.ts (see new files) |
— | — |
builder/model/index.ts (+5/−1) |
Re-exports the new importWorkflowDefinition alongside the existing fromWorkflowDefinition/ImportResult. Additive barrel export. |
LOW |
builder/variants/index.ts (+1/−0) |
Re-exports the new isVariantId. Additive barrel export. |
LOW |
builder/variants/detect.ts (+10/−1) |
Adds a JSDoc warning that detectVariant is unsafe outside issue-collection contexts (its prompt default hides malformed nodes; prefer detectVariantOrNull). Comment-only; no logic change. |
LOW |
Impact roll-up
| Impact | Count | Files |
|---|---|---|
| HIGH | 0 | — |
| MEDIUM | 7 | package.json, WorkflowCanvas.tsx, ConsoleApp.tsx, ProjectRail.tsx, PreviewPage.tsx, variants/registry.ts, model/from-workflow.ts |
| LOW | 10 | bun.lock, eslint.config.mjs, adapters/web.md, experiments/README.md, builder/README.md, theme.css, lib/keymap.ts, model/index.ts, variants/index.ts, variants/detect.ts |
Overall risk: LOW. No HIGH-impact changes. The feature is almost entirely new code isolated under experiments/console/builder/ (an explicitly experimental, beta, fixture-backed surface with no server I/O). The only behavioral change to pre-existing shipped code is the WorkflowCanvas.tsx context-menu listener fix; the only cross-cutting type change is the wireKeys tightening, which is immediately and locally reconciled in from-workflow.ts.
Collaborator
|
Sean — went through this properly. Short version: the engineering is genuinely strong, I've got a couple of scope nits, and one real question about the dependencies. What's impressive
Genuinely nice work overall. Two things that escape the experiment sandbox — please pull them out
Scope / sequencingThe editor polish (smart-guides, align/distribute, auto-arrange, undo-redo coalescing, clipboard envelope) is a lot of surface for a builder that can't load or save yet — that's PR-3. You raised exactly this as an open question in #1863 ("how much editor polish vs YAGNI for a spike"), and I don't think we ever landed a decision on it. For the experiment, the bar is "does it prove a non-YAML author can produce a valid, persisted workflow" — so I'd bias core-path-first, polish-later. Not asking you to rip anything out now, but let's agree the sequence before PR-3 so we're not gold-plating ahead of the load/save path. Dependency question (the main one)This adds 7 new direct deps — So — what's the hard requirement for CodeMirror here? If the preview is going editable (line numbers, search, real YAML-grammar editing) in PR-3 and CodeMirror is the deliberate foundation for that, say so and I'm happy to carry it — it's the right tool for an actual editor. But if this is read-only highlighting, I'd rather reuse what's already in the bundle than add a second highlighting stack permanently to the web app, because deps don't revert when we delete the experiment folder. Same question in the small for anything else you reached for over an existing dep: was there a concrete blocker with the in-repo option, or was it just what the standalone Studio already had wired? Want to understand the reasoning, not litigate it. None of this is blocking — it's a strong PR. Mostly I want the two sandbox-escapes split out and your read on the CodeMirror stack. |
…ction changes Addresses Rasmus's PR-2 review (coleam00#2015): - Replace the read-only YAML preview's CodeMirror stack with the console's existing react-markdown + rehype-highlight stack (highlight.js theme already imported globally). Removes 7 direct deps (@codemirror/{lang-yaml,language, search,state,view}, @lezer/highlight, @uiw/react-codemirror) + their transitive tree so the experiment stays cleanly revertable — deps don't revert when the folder is deleted. Preview is read-only through PR-3; a real editor can reintroduce CodeMirror later as a justified dependency. - Revert the production WorkflowCanvas.tsx deferred-dismiss change (the builder keeps its own BuilderContextMenu fix); to be re-proposed separately with live verification. - Revert the repo-global eslint.config.mjs .agents/** ignore widening; to land as its own one-liner. type-check, web tests (319 pass), and web build all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Rasmus — thank you, this is the kind of review that makes the thing better. You put your finger on the one real seam (deps outliving the folder) and two genuine sandbox escapes. All three are handled in 1. The two sandbox escapes — pulled out ✅Both reverted in
2. CodeMirror — dropped, and the dependency question answered 🎯You asked the right question, so here's the straight answer: the preview is read-only today and stays read-only through PR-3 (which is load/save wiring, not an editor). There is no concrete requirement for CodeMirror's editing muscle right now — so per your steer, I pulled it.
The payoff is exactly the property you care about — and it's better than "smaller":
The 7 CodeMirror/lezer/uiw packages are gone; What we give up by dropping CodeMirror: line numbers, in-pane search, and native YAML-grammar editing — none of which a read-only mirror needs. If the pane ever goes genuinely editable (two-way YAML), CodeMirror is the right foundation and comes back then — as a deliberate, called-out dependency in that PR, not smuggled in under a read-only preview. I'll flag it explicitly when/if that happens. 3. Scope / sequencing — agreed, and PR-3 is already shaped that wayYou're right that we never landed the #1863 "how much polish vs YAGNI for a spike" decision, and I'm happy to pin it before PR-3. I'm fully on board with core-path-first. Concretely, PR-3 (connected mode) is the core path, with no new editor polish: My proposed answer to the open question: treat the polish that's already landed as banked (it's pure, DOM-free, ~290 Validation on Thanks again for the thoroughness — genuinely useful. |
Wirasm
approved these changes
Jun 26, 2026
Wirasm
left a comment
Wirasm
left a comment
Collaborator
There was a problem hiding this comment.
Verified 0020d04d against the diff — all three landed: WorkflowCanvas.tsx and eslint.config.mjs are back to base, the 7 CodeMirror/Lezer/uiw deps are gone (and bun.lock dropped out with them), and YamlPreview now rides the existing react-markdown + rehype-highlight stack with the fence-length guard. The PR is experiment-folder-only with zero net-new deps — exactly the property I cared about, and the "revert by deleting the folder" promise now holds with no asterisk.
Approving. A few wrap-ups:
- Split-outs → independent follow-ups, don't block this. Send the
WorkflowCanvasproduction fix as its own 1-file PR with its own live repro + verification — that's the whole reason we pulled it, so it should stand on its own merit rather than ride this merge. The eslint one-liner is local-convenience-only (zero CI effect, as you noted), so it's lowest priority — whenever suits. - Sequencing → agreed. Your "bank what's already landed, add nothing further until load/save proves the core path" is exactly the right call. Please write that into #1863 so it's recorded rather than re-litigated. PR-3's gate being "a non-YAML author produces a valid, persisted workflow" is the bar.
- One tiny thing before you merge: drop the
package.jsondependency reorder. It's pure diff noise now (same packages, same versions, just moved lines), and there's no reason for this PR to touchpackage.jsonat all once the deps are gone — keeps the experiment-folder-only claim literally true.
Really good turnaround on the review. Merge once the reorder's reverted.
Addresses the final pre-merge nit on PR-2 (coleam00#2015): with the 7 CodeMirror deps removed, `bun install` had re-sorted the remaining dependency block, leaving pure diff noise (same packages, same versions, reordered lines). Restore packages/web/package.json to its base ordering so this PR touches neither package.json nor bun.lock — keeping the "experiment-folder-only" claim literally true. Web build green; dependency set unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Thanks Rasmus — appreciate the careful re-verify. Last nit closed and your wrap-ups handled:
Should be green and ready to merge. Thanks again for the thorough pass — genuinely sharpened the PR. |
This was referenced Jun 30, 2026
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.
Summary
/console/builder).store/cache.ts, no server I/O, no:nameroute — the builder is fixture-backed and controlled (takes a workflow prop, reports edits viaonChange). Load/save + the live route land in PR-3. No engine, executor, or schema changes.UX Journey
Before
After
Architecture Diagram
After (modules added under
packages/web/src/experiments/console/builder/)Connection inventory:
@archon/workflows@/lib/apionlyLabel Snapshot
risk: lowsize: L(PR-2 feature set; the closing fix itself is XS)webweb:builderChange Metadata
feature(PR-2 builder) +bug(context-menu fix closing the branch)webLinked Issue
Validation Evidence (required)
FIRE → OPEN → CLOSE reason=contextmenu), then confirmed fixed by the branch author (menu opens and stays).bun run validate(repo-wide) not run here — changes are isolated to two web components; recommend running in CI.Security Impact (required)
Compatibility / Migration
Human Verification (required)
WorkflowCanvas) right-click was hardened but not exercised live in this session (requires backend + a workflow open in the editor); it used capture-phase listeners that already avoided this race.Side Effects / Blast Radius (required)
Rollback Plan (required)
experiments/console/and is reachable only at/console/builder.Risks and Mitigations
setTimeout(0));keydown/Escape still attaches synchronously.Summary by CodeRabbit
New Features
Bug Fixes