feat(canvas): one substrate, node-kind registry, and the edge language - #145
Conversation
Issue #144 A and B. Extracts the canvas shell into @commonplace/canvas-substrate so the model ERD and the program graph become one system. - one generic NodeShell (header, id badge, badges, ports, bypass/mute, collapse, advanced-port section) driven entirely by NodeShellModel, so registering a kind never edits the shell - node-kind registry compiles kinds into React Flow nodeTypes, cached per widget renderer because React Flow rebuilds every node when that identity moves - one edge language: cubic bezier only, 1.5px round-cap, dasharray 0.1 6 for true round dots, tightening to 4 3 under attention rather than jumping to solid. Program edges take the source port's shape-class family; model relation edges stay neutral ink and carry information on the chip and cardinality glyph instead - the running-edge march is declared in the console motion register, not in the package, so the motion gate stays the authority on it; the substrate only consumes the class. Budget caps concurrent marching edges at 40 and reduced motion drops it entirely - satisfaction dimming during a drag resolves in CSS off one attribute write on the flow root, so starting a wire does not re-render every node - the canvas layout document holds positions, frame membership, reroute waypoints, collapse and port visibility, and round-trips through the layout blob the program canvas already persists. Frame membership is derived from geometry, never written as a relationship - built-in kinds: group-frame, note-markdown (host injects the markdown renderer), and the four JSON Canvas kinds 16 tests; tsc clean.
Issue 144 A, C and D wiring. Both product node types now register as kinds from outside @commonplace/canvas-substrate, which is what proves the shell never needs editing to gain one. - model-card kind (packages/model-canvas) keeps the fork's ERD rows as its body and moves header, badges, ports and ghost state to the shared shell; relation edges ride the substrate edge with the model palette, keeping the key chip, the cardinality glyph and relation direction - program-node kind (apps/console) carries the id badge, bypass/mute affordance with dimmed render, and the advanced-port collapse - widgets-on-node follow ComfyUI's duality: an unconnected input is a parameter widget, a connected one is a plain port. Widget values write to the existing per-node `tweaks` object, so no backend change was needed, and they render through the records surface's own FieldEditor rather than a second editor map - reroute waypoints, collapse, advanced-port state and positions all live in the canvas layout document and round-trip through the layout blob the server already stores opaquely - double-click the pane for quick add, double-click an edge to drop a reroute dot, drag preview in the source family with live satisfaction dimming - provider facet reaches the card badge: parsed at the harness boundary, typed in @commonplace/data-model-contracts, defaulting to declared-record on a registry that predates it Fixes a pre-existing bug where toggling bypass or mute saved a positions-only layout and silently discarded collapsed nodes and grouping. Aligns packages/model-canvas onto react 19.2.3, the version the console and the substrate already resolve; it had drifted to 19.2.7, and two React copies in one tree null the hook context. model-canvas 52 tests, canvas-substrate 16, console canvas surfaces 34, data-model-contracts 4; nine console design gates pass; tsc clean.
Issue 144 E, the "now" half. The ERD canvas joins the reactive system instead of sitting beside it. The existing object-seam subscription only sees changes this client made. A declaration from another head -- an agent, the MCP door, a restore -- moves the registry without touching a local object, and the canvas never noticed. PR 385's versioned projections are the signal: every declare, replace, restore and retire mints a DeclaredSchemaVersion with a monotonic sequence and a content anchor. - registrySignal reads the head version, preferring the one marked declared and falling back to the highest sequence - registryMoved treats an unresolvable signal as no movement on either side, so a failing endpoint cannot drive an endless rehydrate loop - the canvas rehydrates only when the anchor actually moved; an unchanged registry costs one comparison, not a re-render - SchemaVersion carries contentAnchor, parsed at the harness boundary theorem-canvas-compile stays out of this path. It compiles semantic CanvasDoc and Graph Lisp state and exposes no registry-ERD projection consumer, so routing the canvas through it today would be pretending. The producer that closes MF7 properly is specified in docs/plans/canvas/SPEC-REGISTRY-ERD-PROJECTION-1.0 in the Theorem repo; when it lands, this file's heartbeat is deleted and the version comparison moves inside that subscription. 8 tests.
The console design gates only walk apps/console/src, so a workspace package is outside all thirteen of them. Without an explicit step the shell both canvases render through would ship unchecked. Adds the substrate, model-canvas and data-model-contracts suites to the console workflow and their paths to its trigger list.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (58)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a shared canvas substrate package that both the model ERD canvas (@commonplace/model-canvas) and the console program canvas can render through, unifying node chrome and edge styling while keeping node-kind specifics in a registry. It also adds the “provider facet” plumbing and a version/anchor-based registry change signal to keep the model canvas reactive to external registry changes.
Changes:
- Added new
@commonplace/canvas-substratepackage (node-kind registry,NodeShell, shared edge language, layout document + tests). - Migrated model-canvas ERD card + relation edges and the console program canvas nodes/edges to render via the substrate.
- Extended model contracts + harness normalization for provider facet and schema version content anchors; added console CI coverage for the new package.
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds workspace link for @commonplace/canvas-substrate and normalizes React versions to avoid duplicate React copies. |
| packages/model-canvas/tsconfig.json | Adds path mappings for substrate/json-canvas and includes new kind + positions files. |
| packages/model-canvas/src/ModelCanvasShell.tsx | Switches ERD nodes/edges to substrate registry + SubstrateEdge with backward-compatible type mapping. |
| packages/model-canvas/src/kinds/modelCardKind.tsx | Registers the ERD card as a substrate kind (shell projection + body). |
| packages/model-canvas/src/index.ts | Exposes the new model-card kind exports and ErdFieldRows. |
| packages/model-canvas/src/components/canvas/MartNode.tsx | Extracts/export ERD field rows as the kind body content. |
| packages/model-canvas/src/components/canvas/edges.ts | Projects model relations into SubstrateEdgeData (palette/label/cardinality/arrow). |
| packages/model-canvas/src/components/canvas/edges.test.ts | Updates tests to assert resolved chip/glyph output rather than old plumbing. |
| packages/model-canvas/package.json | Adds substrate dependency and pins React dev deps to 19.2.3. |
| packages/data-model-contracts/src/index.ts | Adds provider facet types/helpers and schema-version contentAnchor. |
| packages/canvas-substrate/vitest.config.ts | Configures vitest for the new package. |
| packages/canvas-substrate/tsconfig.json | Adds TS config for substrate package compilation. |
| packages/canvas-substrate/src/test/setup.ts | Sets up jest-dom matchers for vitest. |
| packages/canvas-substrate/src/substrate.css | Adds shared CSS rules (satisfaction dimming + waypoint cursor + hit-path constraints). |
| packages/canvas-substrate/src/shell/shortNodeBadge.ts | Implements short, stable node id badge hashing (FNV-1a base36). |
| packages/canvas-substrate/src/shell/PortRow.tsx | Renders ports + optional inline widgets using host-injected widget renderer. |
| packages/canvas-substrate/src/shell/NodeShell.tsx | Implements the generic node shell (badges, ports, flags, collapse/advanced sections). |
| packages/canvas-substrate/src/layout/document.ts | Adds layout document model + wire round-trip helpers (nodes/frames/waypoints/hidden ports). |
| packages/canvas-substrate/src/layout/document.test.ts | Tests frame membership + layout wire round-tripping and malformed input handling. |
| packages/canvas-substrate/src/kinds/types.ts | Defines substrate contracts for kinds, shell model, ports/widgets, and flags/status. |
| packages/canvas-substrate/src/kinds/registry.tsx | Implements kind registry that compiles entries into stable React Flow nodeTypes. |
| packages/canvas-substrate/src/kinds/index.ts | Barrel exports for registry/types and builtin kinds. |
| packages/canvas-substrate/src/kinds/builtin/note.tsx | Adds a markdown-note kind with host-injected markdown renderer. |
| packages/canvas-substrate/src/kinds/builtin/jsonCanvas.tsx | Adds JSON Canvas node kinds seated on the substrate shell. |
| packages/canvas-substrate/src/kinds/builtin/frame.tsx | Adds titled group-frame kind (frame semantics). |
| packages/canvas-substrate/src/index.ts | Public barrel for substrate (kinds/edges/shell/layout APIs). |
| packages/canvas-substrate/src/edges/waypoints.ts | Adds Catmull–Rom routed path helpers and waypoint insertion/mutation helpers. |
| packages/canvas-substrate/src/edges/SubstrateEdge.tsx | New shared edge renderer (dash law, palette, waypoints, chip rendering). |
| packages/canvas-substrate/src/edges/SubstrateConnectionLine.tsx | Connection preview line matching committed geometry + dash law. |
| packages/canvas-substrate/src/edges/law.ts | Central “edge language” constants/functions (dash, palette, animation cap). |
| packages/canvas-substrate/src/edges/law.test.ts | Tests edge-law invariants (dash, palettes, animation budget). |
| packages/canvas-substrate/src/edges/index.ts | Barrel exports for edge language + components/utilities. |
| packages/canvas-substrate/src/edges/ConnectionSatisfaction.tsx | Implements satisfaction dimming via a single attribute on the flow root. |
| packages/canvas-substrate/package.json | Declares the new workspace package exports/scripts/deps. |
| apps/console/src/views/program/ProgramWidget.tsx | Adapts records-surface FieldEditor into the substrate widget renderer contract. |
| apps/console/src/views/program/ProgramView.tsx | Migrates program canvas to substrate kinds/edges + layout doc + widgets-on-node + binding-station tray hooks. |
| apps/console/src/views/program/programNodeKind.tsx | Registers program nodes as a substrate kind (ports/widgets/badges/flags/status). |
| apps/console/src/views/model/registrySignal.ts | Adds anchor/sequence-based “registry moved” signal derivation. |
| apps/console/src/views/model/registrySignal.test.ts | Tests signal derivation and movement detection rules. |
| apps/console/src/views/model/ModelView.tsx | Hooks registry signal polling into model view (subscription tick + slow heartbeat). |
| apps/console/src/motion/motion.css | Adds keyframes/class for running substrate edges in the motion register. |
| apps/console/src/motion/motion-tokens.ts | Documents new substrate interactions in the interaction inventory. |
| apps/console/src/lib/server/observed-model-harness.ts | Normalizes provider facet + schema version contentAnchor from harness responses. |
| apps/console/package.json | Adds @commonplace/canvas-substrate dependency to console. |
| .github/workflows/console-ci.yml | Adds CI step to typecheck/test the substrate and related packages explicitly. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Comments suppressed due to low confidence (1)
packages/canvas-substrate/src/edges/SubstrateEdge.tsx:143
- Waypoints are rendered as circles with a grab cursor and the edge data type includes
onWaypointMove, but there’s no pointer-drag handling here to ever callonWaypointMove. That makes reroute dots effectively non-draggable (only removable via double-click), which breaks the intended reroute interaction.
onPointerLeave={() => setHovered(false)}
onDoubleClick={(event) => {
event.stopPropagation();
data.onWaypointRemove?.(index);
}}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <BaseEdge | ||
| id={props.id} | ||
| path={path} | ||
| className={data.running && data.marching ? 'substrate-edge-running' : undefined} | ||
| style={{ stroke, ...strokeStyle }} | ||
| data-substrate-edge={palette} | ||
| data-running={data.running ? 'true' : undefined} | ||
| /> |
| useEffect(() => { | ||
| const root = document.querySelector<HTMLElement>('.react-flow'); | ||
| if (!root) return; | ||
| if (!inProgress || !fromNodeId) { | ||
| root.removeAttribute('data-connecting-family'); | ||
| return; | ||
| } | ||
| const family = familyForHandle(fromNodeId, fromHandleId); | ||
| if (family) { | ||
| root.setAttribute('data-connecting-family', family); | ||
| } else { | ||
| root.removeAttribute('data-connecting-family'); | ||
| } | ||
| return () => root.removeAttribute('data-connecting-family'); | ||
| }, [familyForHandle, fromHandleId, fromNodeId, inProgress]); |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
CommonPlace/apps/console/src/views/program/ProgramView.tsx
Lines 513 to 516 in f2626e6
Recompute connected inputs when adding an edge
After a successful connection, this updates only the edge state; the target node's connectedInputs remains the snapshot created by definitionToFlow. Consequently, the newly connected input continues rendering as an editable parameter widget until the program is reloaded, contrary to the widget-to-port behavior, and a stale widget tweak can still be submitted for that wired input. Update the affected node data whenever connections change.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (const field of node.schema ?? ([] as SchemaField[])) { | ||
| ports.push( | ||
| { id: `fl:${field.name}`, side: 'source', connectable: false }, | ||
| { id: `fr:${field.name}`, side: 'source', connectable: false }, | ||
| ); |
There was a problem hiding this comment.
In ERD mode, portsFor adds two shell ports for every schema field, but ErdFieldRows also renders FieldAnchors with the same fl:* and fr:* handle IDs. Since NodeShell renders its port rows before the body, every model card now displays duplicate field-related rows and registers duplicate React Flow handles, making field-edge attachment ambiguous. Keep the field anchors in the body and expose only the node-level left and right ports through the shell.
Useful? React with 👍 / 👎.
| <BaseEdge | ||
| id={props.id} | ||
| path={path} | ||
| className={data.running && data.marching ? 'substrate-edge-running' : undefined} | ||
| style={{ stroke, ...strokeStyle }} |
There was a problem hiding this comment.
For every model relation, marker definitions are created above, but neither markerEnd nor markerStart is passed to this BaseEdge. As a result, all ERD relations lose their direction arrow, and bidirectional relations render identically to one-way relations despite relationEdgeData setting arrow: "both". Apply the generated marker URLs to the rendered edge.
Useful? React with 👍 / 👎.
| for (const [nodeId, ports] of Object.entries(widgetTweaks)) { | ||
| const set = Object.entries(ports).filter(([, value]) => value !== undefined && value !== ''); | ||
| if (set.length === 0) continue; | ||
| tweaks[nodeId] = Object.fromEntries(set) as JsonValue; |
There was a problem hiding this comment.
These widget values are included in every subsequent run, but resetRunState clears only the raw tweak text and never clears widgetTweaks. When a user opens another program or starts a fork whose nodes reuse an ID, values entered in the previous program are silently applied to the new program and shown in its inline widgets. Reset this state alongside tweaksByNode, pins, and liveness.
Useful? React with 👍 / 👎.
| onPointerEnter={() => setHovered(true)} | ||
| onPointerLeave={() => setHovered(false)} | ||
| onDoubleClick={(event) => { | ||
| event.stopPropagation(); | ||
| data.onWaypointRemove?.(index); | ||
| }} |
There was a problem hiding this comment.
When an edge has a reroute dot, the circle handles hover and double-click removal only; no pointer-down or pointer-move path invokes data.onWaypointMove. The host supplies that callback and persists moved coordinates, but it is unreachable, so readers cannot reposition a waypoint after placing it and must instead delete and recreate it.
Useful? React with 👍 / 👎.
| setProgramId(id); | ||
| setDefinition(loaded.definition); | ||
| const flow = definitionToFlow(loaded.definition, catalogById, loaded.layout ?? { nodes: {} }, toggleNodeCollapsed); | ||
| const loadedLayout = fromLayoutWire(loaded.layout as never); |
There was a problem hiding this comment.
fromLayoutWire is called as though loaded.layout contains the complete canvas document, but loadProgram rebuilds that value with only nodes and node_metadata, discarding the newly persisted edge_metadata and frames keys in programClient.ts:109-115. Therefore every saved reroute waypoint disappears after reopening the program, even though the save path successfully wrote it. Return the complete layout wire from the client before decoding it here.
AGENTS.md reference: apps/console/AGENTS.md:L180-L180
Useful? React with 👍 / 👎.
| } else if (node._provider) { | ||
| // The provider facet supersedes the raw inputSource chip: it says where the | ||
| // rows actually come from rather than which importer produced the card. | ||
| badges.push({ | ||
| id: 'provider', | ||
| text: node._provider.text, | ||
| title: node._provider.title, |
There was a problem hiding this comment.
The new card badge is conditional on _provider, but no graph adapter supplies that property: both registryToModelGraph and declaredToModelGraph still construct nodes without mapping ObjectTypeMetadata.provider, and providerBadgeText has no caller anywhere in the repository. Consequently, native views, derived programs, and connectors all continue showing the fallback TABLE source instead of the provider badge this change introduces.
Useful? React with 👍 / 👎.
| useEffect(() => { | ||
| const root = document.querySelector<HTMLElement>('.react-flow'); | ||
| if (!root) return; |
There was a problem hiding this comment.
When more than one canvas is mounted, document.querySelector('.react-flow') always selects the first React Flow root in the document rather than the root containing this component. A connection started in a later Program canvas can therefore dim an unrelated model or program canvas while leaving its own targets unchanged, and cleanup from one instance can remove another instance's attribute. Resolve the owning flow element locally instead of querying the global document.
Useful? React with 👍 / 👎.
| const target: Point = { x: props.targetX, y: props.targetY }; | ||
| const waypoints = data.waypoints ?? []; | ||
|
|
There was a problem hiding this comment.
For a running edge that is not hovered or selected, strokeStyle is still computed solely from attention, so it retains the normal 1.5px resting width. This is especially visible under reduced motion and after the 40-edge animation cap, where marching is disabled and the documented width bump is supposed to be the only remaining execution signal; instead those running edges become indistinguishable from idle edges. Include data.running when selecting the stroke width.
Useful? React with 👍 / 👎.
| position={port.side === 'target' ? Position.Left : Position.Right} | ||
| id={port.id} | ||
| isConnectable={port.connectable !== false} | ||
| style={{ | ||
| top: 14, |
There was a problem hiding this comment.
When a node with multiple inputs or outputs is collapsed, every handle on a given side is rendered at exactly top: 14. The handles therefore stack on top of one another, so only the topmost port can be selected for a new connection and readers cannot distinguish which existing wire belongs to which port. Assign distinct offsets or expose a port-selection affordance while preserving the collapsed shape.
Useful? React with 👍 / 👎.
| const savedNodeId = typeof saved.node_id === 'string' | ||
| ? saved.node_id | ||
| : programIdRef.current; | ||
| if (!savedNodeId) { | ||
| throw new Error('binding_station_requires_saved_program'); |
There was a problem hiding this comment.
When applying a binding station to an unsaved draft, this recognizes only saved.node_id; unlike scheduleSave, it ignores valid id and program_id response shapes. If the save endpoint returns either of those supported variants, the save succeeds but savedNodeId remains null and the binding operation fails with binding_station_requires_saved_program, potentially leaving an orphaned draft. Use the same receipt-ID normalization as the normal save path.
Useful? React with 👍 / 👎.
|
Review from the planning side. Read: both PR bodies, 1. The advanced-section rule inverts the pattern it adapts (design defect)
Apply that to the KSampler in the reference screenshots: The rule conflates two different questions: "does this input have a wire" and "is this input advanced." The widget duality this PR correctly introduces makes them independent. Suggested rule: primary = connected ports plus every widgetized input; advanced = unconnected inputs that expect a wire and have no widget. Better still, let Related: is there a user override? Moving from "port visibility in the layout doc" to a purely derived rule silently drops that checklist item. Derived default plus an optional per-node override in the layout doc keeps both. 2. The hue map exists twice
Fix: I went looking for a worse version of this bug and did not find it, worth recording: I expected an unknown shape to fall through 3. Silent defaults hide new backend shapesThe same Since 4. Unverified by eye, ranked by riskThe PR is honest that the dev server did not run. The highest-risk item is not the dash values but their behavior under zoom: 5. Smaller
6. Merge choreographyFour stacked PRs across two repos, all currently unstable: Theorem#385, then Theorem#392, then #143, then #145, with What is better than the specRecorded so it survives: the widget duality (an unconnected input is the widget, connecting turns it back into a port) is a cleaner unification than the "render parameter rows from field types" I wrote, and it lands with no backend change because Also noted: |
Planning review plus Codex and Copilot findings on PR 145. Correctness (P1): - the running-edge treatment animated `stroke-dashoffset`, which the console motion constitution forbids: it permits transform and opacity, and repainting the whole path every frame is the cost that rule exists to prevent. Flow direction now rides a staggered opacity cascade across three pips sampled along the wire, measured once per geometry change rather than per frame - arrow markers were defined but never referenced, so every ERD relation lost its direction and bidirectional relations looked one-way. Attached - the model card declared per-field ports that `ErdFieldRows` already renders, registering each `fl:`/`fr:` handle twice and drawing a rowless second copy of every field. The shell now exposes only the node-level ports - `loadProgram` rebuilt the layout from `nodes` and `node_metadata` alone, so every saved reroute waypoint and frame vanished on reopen. The whole document round-trips, with `fromLayoutWire` the single validator Design: - the advanced-port rule inverted the pattern it adapts. "Has a wire" and "is advanced" are different questions, and the widget duality makes them independent: primary is now every connected port plus every input a reader can actually type into, so a node's tuning tail shows at rest. Advanced holds only unconnected inputs that expect a wire and offer no control - editability gets its own total map over the generated shape union rather than being derived from the hue family. Deriving it would have made every declared shape unwidgetizable, silently disabling widgets-on-node; a test now asserts both sides of that line stay populated - structured planes no longer offer a JSON textarea standing in for a table - the per-node advanced override is back in the layout document, restoring the port-visibility item the derived rule had quietly dropped - the shape-class hue map existed twice. `shapeHue.ts` now imports the family and its tokens from the substrate and keeps only the projection, which is the genuine console-side knowledge - both shape maps are total records over the generated union, so a new Rust shape is a console build failure rather than a silent scalar wire Also: - widget values survived a program switch and were reapplied to whatever node reused an id; cleared with the rest of the run state - reroute dots could be placed and deleted but never moved - connection dimming resolved `.react-flow` globally, so a drag in one canvas dimmed another. Scoped to its own instance - running edges kept the resting width, so past the animation cap and under reduced motion they were indistinguishable from idle ones - collapsed nodes stacked every handle at one offset, leaving only the topmost reachable - dash and width are user units, so a dot aliased away at 0.5x and swelled at 2x. `vector-effect: non-scaling-stroke` holds the rhythm across zoom - card widths are named constants; ports carry their catalog display label - the registry refuses a kind-id mismatch by name instead of exploding inside a body handed the wrong data shape, and kinds declare their own legacy type aliases so saved graphs keep rendering - the frame-membership rule is stated and total: full containment, smallest area wins, ties broken by id, absent when no frame contains the node - removed ProgramEdgeView, which this work orphaned canvas-substrate 16, model-canvas 52, data-model-contracts 4, console canvas surfaces 49; eleven console design gates pass; tsc clean.
Four stacked PRs across two repos, with a drift gate that compares a CommonPlace file against a Rust binary in the Theorem repo. Merging out of order fails as contract drift, which reads like a code bug. Records the order, the reason for each edge, and the regenerate-do-not-hand-edit rule.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
Findings raised on #143 that live in files this branch already rewrites, so they land here rather than conflicting with that branch. Correctness: - program saves, loads and edge validations could complete after the reader opened a different program, writing one draft's graph under another's id. One draft generation now guards every async completion; opening, forking, starting from a starter, materializing a proposal and resetting all bump it - validate_edge returns an externally tagged enum, so a real mismatch arrived as `{ mismatch: ... }` under `status`, not the string `'mismatch'` with a sibling. Every genuine mismatch degraded to "unknown schema status" and lost the column and shape detail. Both shapes are now read - the edges array is controlled with no `onEdgesChange`, so a wire could not be deleted or reconnected at all; the only way to remove one was to delete a node it joined. Edge changes now route through the layout persist, which also drops the reroute waypoints of a deleted edge - the field editor's state initialised only at mount, so the first selection opened empty and moving from field A to B kept A's key, label, type and required flag. Submitting could redeclare B with A's values - the layout debounce was cancelled on unmount without writing, losing the reader's last drag against the canvas durability contract. It flushes first - clearing an optional integer or number wrote a real 0 over an absent value, moving aggregates. Blank stays absent - observed ingest events were presented as record counts. They are separate fields with separate badges now: several events can touch one record, so an unlabelled number overstated the corpus - a pipe or newline in an OKF field description broke the Markdown table its own parser reads back, truncating field metadata on reimport. Cells escape - a relationship that changed cardinality or direction while keeping its endpoints and join columns compared equal, so review dialogs reported "No structural changes" over a material contract change - webkitdirectory made the OKF import a directory picker in Chromium, so the single .okf.json this same screen exports could not be selected Not changed, with reason: - the finding that human answers should key by `ask_id` is incorrect. The scheduler reads `options.human_answers.get(&node_id)` (rustyred-thg-programmable-graph/src/execute.rs:332), so `node_id` is right - the two CI findings were already addressed by the substrate CI commit console views 126 tests; canvas-substrate 16, model-canvas 52, okf 57, data-model-contracts 4; ten design gates pass.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4d141109e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| clearTimeout(layoutPersistTimer.current); | ||
| layoutPersistTimer.current = null; | ||
| const pending = pendingLayoutRef.current; | ||
| if (pending) void persistLayoutRef.current?.(pending); |
There was a problem hiding this comment.
When the topic changes within 400 ms of a node drag, render has already replaced persistLayoutRef.current with a persistLayout closure for the new topic before this cleanup runs. The pending positions from the old topic are therefore written into the new topic's canvas, leaving the old layout unsaved and corrupting the new layout. Capture the writer or topic alongside the pending positions instead of dereferencing the latest writer during cleanup.
AGENTS.md reference: apps/console/AGENTS.md:L180-L180
Useful? React with 👍 / 👎.
| } satisfies SubstrateEdgeData, | ||
| }, | ||
| ]; | ||
| persistFromFlow(nodes, next); |
There was a problem hiding this comment.
When a reader connects a wire to a widgetizable input, this persists the new edge but leaves the target node's data.connectedInputs unchanged. portsFor consequently continues treating the input as unconnected and renders an editable widget, whose committed value is then submitted as a tweak alongside the incoming edge. Recompute the affected nodes' connected-input sets whenever edges are added or removed.
Useful? React with 👍 / 👎.
| function escapeTableCell(value: string): string { | ||
| return value.replace(/\|/g, "\\|").replace(/\r?\n/g, " "); |
There was a problem hiding this comment.
For any schema type or description containing |, this emits \|, but parseSchema still uses ln.split("|") in packages/okf/src/parse.ts:184 and never unescapes cells. A value such as A | B therefore reimports as A \ with the remainder discarded, so the claimed round trip remains corrupt. Make the parser escape-aware or use an encoding it already understands.
Useful? React with 👍 / 👎.
| .substrate-edge-running { | ||
| animation: none; | ||
| } |
There was a problem hiding this comment.
If a reader enables reduced motion while program edges are already running, renderedEdges does not subscribe to media-query changes, so CSS must stop the existing animation. This rule targets .substrate-edge-running, which is never rendered; the animated circles use .substrate-edge-pip, so they continue pulsing indefinitely until some dependency happens to recompute the edges. Target the actual pip class in this media query.
AGENTS.md reference: apps/console/AGENTS.md:L162-L167
Useful? React with 👍 / 👎.
| { id: 'left', side: 'source', label: 'left' }, | ||
| { id: 'right', side: 'source', label: 'right' }, |
There was a problem hiding this comment.
For keyless relations or relations whose field anchor cannot be resolved, buildRfEdges uses the node-level left handle when the neighboring model is to the left. Both ports here are declared as side: 'source', however, and PortRow places every source port at Position.Right, so the handle named left is physically rendered on the right edge and these relations route across or around the card incorrectly. Give physical placement its own property or otherwise render this handle at Position.Left.
Useful? React with 👍 / 👎.
| const inputs = node.inputs.map((port): SubstratePort => { | ||
| const isConnected = connected.has(port.id); | ||
| const widgetized = !isConnected && isWidgetizableShape(port.shape) && Boolean(node.onTweakChange); | ||
| const derivedAdvanced = !isConnected && !widgetized; |
There was a problem hiding this comment.
For every unconnected input whose shape is classified as widgetizable, this replaces the typed input with a tweak widget and sends its value through ProgramRunOptions.tweaks. The canonical program-canvas contract instead defines widgets from catalog parameter declarations and requires promotion to create a real input port and change content_id (docs/plans/console/SPEC-PROGRAM-CANVAS-1.0.md:72-76); ProgramPort currently carries only id and shape_id, so disconnection is not evidence that a port is a parameter. This can make an ordinary upstream-data input appear locally configurable without changing the program definition or cache identity.
Useful? React with 👍 / 👎.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
…ut flush CI caught two real hazards in the previous commit, both in ModelView. The field-editor reset synced state inside an effect, which paints the stale draft once and then cascades a second render. It now adjusts during render, guarded by the field identity, which is React's documented answer for resetting state when a prop changes. The layout flush assigned persistLayoutRef during render. That is a genuine hazard, not just a lint rule: a ref written mid-render is not guaranteed to match what the committed tree closed over. It moves to an effect that runs after every render, so the unmount flush calls the latest closure. console views 127 tests; eslint clean on the files this branch owns.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
# Conflicts: # .github/workflows/console-ci.yml # apps/console/e2e/model-program-canvas.live.spec.ts # apps/console/package.json # apps/console/src/lib/server/observed-model-harness.ts # apps/console/src/views/model/ModelView.tsx # apps/console/src/views/model/diagram/ForkDiagramCanvas.tsx # apps/console/src/views/program/ProgramView.tsx # apps/console/src/views/program/connection.ts # apps/console/src/views/program/programClient.test.ts # apps/console/src/views/program/programClient.ts # apps/console/src/views/program/shapeHue.ts # apps/console/src/views/records/editors.tsx # packages/data-model-contracts/src/index.ts # packages/model-canvas/package.json # packages/model-canvas/src/ModelCanvasShell.tsx # packages/model-canvas/src/components/canvas/MartNode.tsx # packages/model-canvas/src/components/canvas/edges.test.ts # packages/model-canvas/src/components/canvas/edges.ts # packages/model-canvas/src/index.ts # packages/model-canvas/src/lib/diff.ts # packages/model-canvas/tsconfig.json # packages/okf/src/serialize.ts # packages/program-contracts/src/program.generated.ts # pnpm-lock.yaml
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
Merged to Findings addressedDesign (planning review 1). The advanced-port rule inverted the pattern it adapts. Fixed, and fixing it surfaced a second defect underneath: if every unconnected input is widgetized, "primary = connected ∪ widgetized" makes the advanced section unreachable. Editability now has its own total map over the generated shape union, separate from the hue family, and a test asserts both sides of that line stay populated. Structured planes no longer offer a JSON textarea standing in for a table. The per-node override is back in the layout document. Duplication and exhaustiveness (2, 3). Zoom (4). Smaller (5). Card widths are named constants; ports carry their catalog label; the registry refuses a kind-id mismatch by name and kinds declare their own legacy aliases; the frame rule is stated and total (full containment, smallest area, ties by id, absent when none contains). Codex P1/P2. Arrow markers attached; duplicate ERD field anchors removed; the layout wire round-trips whole (waypoints and frames were being dropped on reopen); widget values cleared on program switch; waypoints draggable; provider facet wired through both adapters; connection dimming scoped to its own flow instance; running edges keep the width bump; collapsed handles get distinct offsets. The banned animation. Codex was right and it changed the design: Two findings not applied
The two CI findings were already addressed by the substrate CI commit. Findings from #143 fixed hereBecause this branch already rewrote those files, fixing them here avoided conflicting with #143: stale save/load/validation completions writing one draft over another (one draft generation now guards all three), the Remaining #143 findings are untouched, mostly in Verificationcanvas-substrate 16, model-canvas 52, okf 57, data-model-contracts 4, console 532; eslint and all console design gates pass; Not verified: nothing was run in a browser, so the dash rendering, pip cadence and satisfaction dimming rest on unit tests and token references. The new Rust |
Implements #144. Stacked on #143; backend half is Theorem#392 (stacked on Theorem#385).
ComfyUI law observed: read-and-reimplement, zero code movement.
A. One substrate, many node kinds
New package
@commonplace/canvas-substrate. A package rather than an internal module because the two canvases live in different homes —packages/model-canvasandapps/console/src/views/program— so neither can host the other's shell without a dependency inversion.One generic
NodeShell(header, id badge, badges, ports, bypass/mute, collapse, advanced-port section) driven entirely byNodeShellModel. A kind contributes a projection onto that contract plus an optional body, and nothing else.The law that adding a kind never touches the shell is enforced by construction: both product kinds are registered from outside the package —
model-cardfrompackages/model-canvas,program-nodefromapps/console.Also registered:
group-frame,note-markdown(host injects the markdown renderer, so this never becomes a second markdown implementation), and the four JSON Canvas kinds, so the inspector rail's Z-layer can converge here later.B. The edge language
One geometry, one dash system, two palettes.
stroke-linecap: round,stroke-dasharray: 0.1 6→ true round dots4 3— solidifying under attention rather than jumping to a different markstroke-dashoffsetonly on running edges, one 6.1-unit dash cycle per loop so it seams. Capped at 40 concurrent; past the cap and underprefers-reduced-motiona running edge keeps the width bump and stands stillThe march
@keyframeslives in the console motion register, not the package — the design gates only walkapps/console/src, and putting the motion law inside a package would move it out of the design system's sight. The substrate consumes the class, as it consumes--ij-*tokens. AnINTERACTION_INVENTORYrow documents it.Satisfaction dimming resolves in CSS off one attribute write on the flow root, so starting a wire does not re-render every node.
C. ComfyUI adaptations
group-framekind; membership derived from geometry in the layout doc#66)Widgets write to the existing per-node
tweaksobject, so no backend change was needed, and they render through the records surface's ownFieldEditorrather than a second editor map.D/E
Provider facet reaches the card badge (parsed at the harness boundary, typed in
data-model-contracts, defaults todeclared-record). The ERD subscribes to the registry change signal from #385's versioned projections — seeregistrySignal.ts.theorem-canvas-compilestays out of that path; the producer that closes MF7 properly is specified in Theorem#392.Along the way
packages/model-canvashad drifted to react 19.2.7 while console and the substrate run 19.2.3; two React copies in one tree null the hook context. Pinned to 19.2.3.apps/console/src, so a package is outside all thirteen. Added an explicit CI step rather than let the shared shell ship unchecked.Validation
@commonplace/canvas-substrate— 16 tests, tsc clean@commonplace/model-canvas— 52 tests, tsc clean@commonplace/data-model-contracts— 4 teststsc --noEmitclean for every file in this PRNot verified in a browser: the dev server was not run in this session, so the dash rendering, march cadence and satisfaction dimming are asserted by unit test and token reference, not by eye.
🤖 Generated with Claude Code
Summary by CodeRabbit