fix(engine,ui): emit nodeadded only after engine confirms creation; draft nodes, compositor stale-echo fixes#388
Conversation
In MonitorView, dropping a plugin node with required params that have no schema default (e.g. servo's url, slint's slint_file, kokoro/piper/ matcha/supertonic's model_dir) used to immediately send 'addnode' to the engine. The engine then created the node, ran the plugin's new()/validate(), got 'url must not be empty' (or similar), marked the node Failed, and dropped any pending tune/connect calls. The user saw no error feedback and a node that vanished from the canvas. Now such nodes live as UI-only 'drafts' until their required params are filled. Drafts render with a dashed border, desaturated fill, and a 'Draft - needs <fields>' banner; the inspector header shows a Draft pill. Connection attempts to or from a draft are blocked with a toast. As soon as the last required field is filled the draft is promoted via the normal addNode WS call, and the cleanup effect removes it from local state once the engine echoes the node back via nodeadded. Deleting a draft is local-only. Also fix the SDK macros native_plugin_entry! and native_source_plugin_entry! to log the actual plugin error before returning null, so future debugging surfaces 'url must not be empty' instead of a generic 'Plugin failed to create instance'. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Devin Review
|
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| const yamlString = pipeline ? generatePipelineYaml(pipeline, orderedNames) : ''; | ||
| setYamlString(yamlString); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [topoKey, defByKind, selectedSessionId, tuneNode]); |
There was a problem hiding this comment.
📝 Info: stableOnParamChange identity now changes more often due to handleDraftParamChange dep
Adding handleDraftParamChange to stableOnParamChange's useCallback deps (ui/src/views/MonitorView.tsx:1464) means it changes identity whenever nodeDefinitions, addNode, selectedSessionId, or toast change. Previously it only depended on [toast, tuneNode, tuneNodeConfigDeep]. Since the topology effect intentionally excludes stableOnParamChange from its deps (ui/src/views/MonitorView.tsx:1439), node data objects created by the topology effect can reference a stale closure.
This is mitigated because: (a) defByKind (derived from nodeDefinitions) IS in the topology effect's deps, so nodeDefinitions changes trigger a rebuild; (b) session switches clear all drafts before the stale addNode could fire; (c) draft param editing primarily happens through the inspector's handleRightPaneParamChange which has its own fresh deps. In-canvas controls on draft nodes would use the stale closure, but drafts with in-canvas controls (e.g. compositor) are an unusual workflow.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Confirmed intentional — the topoKey memo's draftFingerprint segment already changes whenever any draft's id, kind, params keys, or missingRequired list changes, so reads of draftNodes inside the effect see a consistent snapshot. stableOnParamChange / stableOnConfigChange are read via closure but their identity churn is benign here: a new identity just means we re-run the topology effect on the next render where topoKey happens to also change, and buildNodeObject simply embeds the fresh callback into data. The eslint-disable predates this PR for the same reason.
There was a problem hiding this comment.
Acknowledged. The eslint-disable on the topology effect predates this PR; stableOnParamChange/stableOnConfigChange identity is governed by topoKey (which incorporates the draft fingerprint) so the effect re-runs whenever it needs to. The added handleDraftParamChange dep on stableOnParamChange doesn't change the effect's correctness — keeping the existing pattern rather than expanding the deps array (and triggering re-runs on every addNode/nodeDefinitions change).
| stats?: NodeStats; | ||
| onParamChange?: (nodeId: string, paramName: string, value: unknown) => void; | ||
| sessionId?: string; | ||
| draft?: { missingRequired: string[] }; |
There was a problem hiding this comment.
📝 Info: Draft nodes use DraftNodeState type but ConfigurableNode uses inline type
NodeFrame.tsx exports a proper DraftNodeState type at line 173, but the node data interfaces in AudioGainNode.tsx (line 86), CompositorNode.tsx (line 51), and ConfigurableNode.tsx (line 129) all use the inline type { missingRequired: string[] } instead of importing the shared type. This is a minor consistency issue — if the draft state shape changes, these inline types would need manual updating. Not a bug, but worth noting for maintainability.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Fair point on consistency. Leaving inline for now since DraftNodeState lives in NodeFrame and the node-data interfaces have no other shared imports from there; happy to import the type if you'd prefer.
| let err_logger = logger.clone(); | ||
| match <$plugin_type as $crate::NativeProcessorNode>::new(params_json, logger) { | ||
| Ok(instance) => Box::into_raw(Box::new(instance)) as $crate::types::CPluginHandle, | ||
| Err(_) => std::ptr::null_mut(), | ||
| Err(e) => { | ||
| err_logger.error(&format!("Plugin instance creation failed: {e}")); | ||
| std::ptr::null_mut() | ||
| } |
There was a problem hiding this comment.
📝 Info: Rust logger clone is safe and well-scoped
The err_logger = logger.clone() at lines 938 and 1163 in sdks/plugin-sdk/native/src/lib.rs shallow-copies the Logger struct including its raw user_data pointer. This is safe because: (1) Logger implements Clone (line 16 of logger.rs), (2) the SAFETY comment at lines 29-36 documents the host's responsibility for thread-safe user_data, (3) both the original and clone are used within the same guard_handle scope, and (4) the original is moved into new() while the clone is only used in the Err branch. The change correctly surfaces previously-swallowed plugin creation errors.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
… fields, clarify pending-draft toast
- generateName() now considers draftNodesRef so two drops of the same
kind produce distinct ids (e.g. plugin::native::servo_1 and _2)
rather than the second silently overwriting the first in the drafts
Map.
- InspectorPane stops gating fields on schema.tunable when the node is
a draft. Drafts have no live engine instance, so the tunable check
(which reflects 'safe to change at runtime') does not apply; the
user must be able to fill required-but-not-tunable fields like
slint's slint_file to promote the draft. Tunable gating resumes
once the engine echoes the node back as a real instance.
- onConnect produces a clearer toast ('<id> is being added to the
pipeline - try again in a moment') when a draft has just been
promoted (missingRequired empty, but engine hasn't acked yet)
instead of an empty 'Configure on ...' message.
Reported by Devin Review on PR #388.
Signed-off-by: StreamKit Devin <devin@streamkit.dev>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Addresses post-merge findings on PR #388: 1. Stale-value bug: InspectorPane reads from nodeParamsAtom first then falls back to node.data.params. handleDraftParamChange only updated local draftNodes state, and the topology rebuild keys off draft fingerprint (param keys + missingRequired), not values. Subsequent keystrokes on the same key froze the input on the first character. Fix: also call writeNodeParam / writeNodeParams from handleDraftParamChange so the inspector stays in sync. 2. Dot-path correctness: handleDraftParamChange stored dotted keys verbatim ("properties.show"), bypassing buildParamUpdate. The eventual addNode payload would have been malformed. Extracted mergeDraftParam into draftNodes.ts which routes flat vs nested updates the same way dispatchParamUpdate does for live nodes. 3. Promotion timeout recovery: addNode is fire-and-forget, so a server-side rejection (unknown kind, malformed payload, transport error) left the draft pinned at missingRequired: [] forever. Added a per-draft timer (PROMOTION_TIMEOUT_MS = 8s) that recomputes missing-required from the schema, clears promotedAt, and toasts. 4. Minor cleanups: drop the 'as unknown as Parameters<...>' cast on the synthetic draft apiNode (state: null instead of 'Creating'); collapse the redundant rightPaneView if/else into a single branch; remove draftNodes from that effect's deps so it stops firing on every keystroke. Adds 5 unit tests for mergeDraftParam covering flat overwrites (regression for finding 1) and dot-path nesting (regression for finding 2). Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
There was a problem hiding this comment.
📝 Info: Right panel gating on pipeline prevents inspector for draft-only canvas
The right panel is only rendered when selectedSessionId && pipeline is truthy (ui/src/views/MonitorView.tsx:1864). If a session's pipeline were null while drafts exist, the inspector wouldn't render despite setRightPaneView('inspector') being called on drop.
After tracing the lifecycle, this scenario is impossible in practice: drops require FlowCanvas, which requires nodes.length > 0, which requires either pipeline data or existing drafts. The first draft requires FlowCanvas, which requires a pipeline. And session destruction (which could null out the pipeline) triggers draft cleanup via the prevDraftSessionIdRef layout effect. So the gating is safe.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Noted. Pre-existing pattern — the entire right pane has been gated on pipeline for the lifetime of MonitorView. The drag source (palette) is itself behind the same gate, so the race the comment describes is unreachable today. Not changing in this PR.
…ings - handleDraftParamChange: skip addNode when promotedAt is already set (BUG_0002). Without this, every keystroke during the promotion-window (between addNode WS send and the engine's nodeadded echo) re-fires addNode; the engine rejects each duplicate as 'Node already exists' and the user's edits are silently swallowed. Edits during the window now accumulate in draft.params and the atom; live-mode tuneNode takes over once the draft is cleaned up. - stableSelectedNode: also compare data.draft (BUG_0001). Without this, the inspector's DraftHint banner showed stale missing required after a promotion timeout reverted draft.missingRequired from [] back to the schema's required keys (params unchanged, so the previous memo returned the stale ref). - Promotion-timeout fallback now uses computeMissingRequired against the draft's current params instead of the schema's full required list (ANALYSIS_0005). Only fields that are actually empty are surfaced; the toast still explains what happened either way. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| const draftFinalInputs = draftBaseInputs; | ||
| const draftFinalOutputs = draftBaseOutputs; |
There was a problem hiding this comment.
📝 Info: Draft nodes don't filter dynamic template pins
At lines 1187-1188, draft nodes set draftFinalInputs = draftBaseInputs and draftFinalOutputs = draftBaseOutputs directly from the node definition, without calling resolveDynamicPins. This means dynamic template pins (with cardinality: { Dynamic: ... }) will appear in the draft's pin list. For live nodes these templates are filtered by filterRuntimePins in NodeFrame.tsx:146-150 before rendering, so they won't show as real connectable pins. The ghost/placeholder pins may still appear, which is arguably correct behavior for a draft (showing what pins the node will have). This is consistent — just different from the live-node code path.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Confirming — intentional. filterRuntimePins in NodeFrame strips template entries before render, so drafts and live nodes both end up showing the same set of pins on the canvas. The minor visual difference is that drafts won't have any concrete dynamic instances yet, which matches the user's mental model ("this node hasn't been built yet").
| // In monitor view, non-tunable params are normally disabled (they | ||
| // can't be changed at runtime). Drafts are the exception: the | ||
| // node does not exist in the engine yet, so the user must be able | ||
| // to fill required-but-not-tunable fields (e.g. slint's | ||
| // `slint_file`) to promote the draft. All fields stay editable | ||
| // until the draft is committed; tunable gating resumes once the | ||
| // engine echoes back a real node. | ||
| const isDraft = !!node.data.draft; | ||
| const isDisabled = readOnly || (isMonitorView && !isDraft && !schema.tunable); |
There was a problem hiding this comment.
📝 Info: InspectorPane enables all fields for drafts regardless of tunable status
At InspectorPane.tsx:368-369, the isDisabled computation sets isDraft = !!node.data.draft and makes all fields editable when isDraft is true, even non-tunable ones. This is intentional and well-commented (lines 361-367): drafts don't exist in the engine yet, so all fields must be editable for the user to fill required params. Once the node is committed and the engine echoes it back, draft becomes undefined and the normal tunable gating resumes. The behavior is correct for the draft lifecycle.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Confirming.
…uilds Drafts were always rendered at draft.position (the original drop coordinate). When the user dragged the draft and then started filling in a required field, the topology effect would re-run on the next draft fingerprint change (missingRequired shrinks on the first keystroke) and snap the draft back to the drop point. Fix: route draft positions through the same prevPositions / savedPositions / fallback chain that live nodes use. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…ngNodePositions - Switch the draft-clear effect on selectedSessionId change to useLayoutEffect so the clear commits before paint. Previously the topology effect could render the old session's drafts on the new session's canvas for one frame. - Don't seed pendingNodePositions for drafts in onDrop or at promotion time. Drafts already render via the draft branch in the topology effect (which reads draft.position / prevPositions directly), so the pending entry is never consumed by resolveNodePosition (prevPositions always wins after the first frame). Without this the entry leaked in the Map for the lifetime of the component. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Two rapid edits to different draft fields in the same JS task could both read 'draft.params' from the same useEffect-synced ref snapshot (the ref synced after paint, not before the next handler call) and the second setDraftNodes updater would silently overwrite the first's edit. - Add latestDraftParamsRef as a synchronous shadow of the latest computed params per draft. handleDraftParamChange now bases newParams on this ref so consecutive edits accumulate correctly even before React renders. - Switch the draftNodesRef sync from useEffect to useLayoutEffect so it commits before paint and before the next event handler. - Inside each setDraftNodes updater, spread '...c' from prev.get(id) rather than '...draft' from the closure, preserving any concurrent changes to the same draft (e.g. position from drag). - Clear latestDraftParamsRef when a draft is dropped (cleanup on promotion, on session switch, and on explicit delete) so it doesn't leak across draft lifecycles. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…-clear - Add promotionInFlightRef as a synchronous shadow of in-flight promotions. Two keystrokes on the same draft within the same JS task could both observe an unstamped promotedAt (since draftNodesRef syncs at paint, not within a tick) and both fire addNode. The new guard short-circuits the second call. - Skip the session-switch draft clear when there's nothing to clear, saving a render and a topology recompute on first mount / no-draft session navigation. - Document why handleDraftParamChange intentionally bypasses the validateParamValue path that live edits use. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [topoKey, defByKind, selectedSessionId, tuneNode]); |
There was a problem hiding this comment.
📝 Info: Topology effect dep array intentionally omits draftNodes — covered by topoKey
The topology effect at line 1124 lists [topoKey, defByKind, selectedSessionId, tuneNode] as dependencies (with eslint-disable). draftNodes is not listed but is captured in the closure. This is safe because topoKey includes a draftFingerprint that changes whenever the draft's key-set or missing-required list changes. Value-only changes (same keys, same missing list) intentionally skip the topology rebuild — the Jotai atom handles those for the inspector. Similarly, stableOnParamChange and stableOnConfigChange are omitted but use ref indirection (draftNodesRef, validateParamValueRef) to stay current.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Confirmed — topoKey carries the draftFingerprint so the lint-disable on the topology effect is correct. No change needed.
…e key onNodesDelete handled the local draftNodes Map but missed latestDraftParamsRef and promotionInFlightRef. A stale in-flight entry with the same generated id would silently block the new draft from ever being promoted. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…ilds Two UX bugs reported while testing the draft-node flow: 1. Connecting a draft works after URL is set — but only on a fresh draft. The previous code promoted on the *first* keystroke into a required field (e.g. "h" while typing "https://..."), which the engine immediately rejected as an invalid URL. The user could then keep typing the rest of the URL but it never reached the engine, so subsequent connection attempts failed. Promotion is now debounced by PROMOTION_DEBOUNCE_MS (600ms): each keystroke resets the timer, so addNode only fires after the user stops typing and the engine sees the final value. Timers are tracked per draft id and cleared on every cleanup path (pipeline arrival, session switch, explicit delete, React Flow Delete key, timeout fallback, component unmount). 2. Typing into a draft's URL field would close the inspector pane. Root cause: the topology effect calls setNodes(newNodes) when topoKey changes (which happens the first time a new param key is added to a draft). The new RFNode objects had no `selected` flag, so React Flow's selection cleared, useOnSelectionChange fired with [] and the right-pane effect snapped back to YAML. Fix: snapshot `selected` from the previous nodes array and re-apply it to the new nodes before setNodes(), so the selection survives the rebuild. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…h on promote)
Six findings from review of the previous draft-node fixes:
1. Stale Jotai atom values when a draft is deleted and a new one is
minted with the same generated id (servo_1 → deleted → next dropped
servo gets servo_1 again). The InspectorPane reads the atom first
and was showing the old draft's typed values as the "default".
Both delete paths (handleDeleteNode, onNodesDelete) now call
clearNodeParams alongside the synchronous-ref cleanup.
2. Initial draft selection had no React Flow selection ring. onDrop
calls setSelectedNodes([nodeId]) but the topology rebuild's
prevSelected snapshot was computed from the pre-drop nodes array,
so the new draft was appended with selected: false. Mirrored
selectedNodes into a ref and merged it into prevSelected so the
first paint after a drop already shows the ring.
3. Edits made between addNode and the engine's nodeadded echo were
silently clobbered: the late keystrokes mirrored to the Jotai atom,
then nodeadded arrived with the original promoted params and
overwrote the atom. The cleanup effect now diffs the accumulated
latestDraftParamsRef against the live pipeline params and flushes
the difference via tuneNodeConfigDeep, so user-typed late edits
converge with engine state.
4. topoKey rebuilt on every keystroke that introduced a new param
key, even when nothing structural changed. The draftFingerprint
no longer hashes the param key set; only id/kind/missingRequired
participate. Drafts read displayed values from the atom (mirrored
on every keystroke) so the canvas doesn't need a topology rebuild
for each new key.
5. Promotion-timeout toast ("Check the engine log") assumed shell
access to the engine — fine for self-hosted dev, awkward for
managed deployments. Reworded to "did not respond — edit a field
to retry, or remove the node."
6. Drafts now run the same validateParamValue the live path uses, so
out-of-range numbers / malformed enums are surfaced immediately
instead of accumulating in the draft and only failing at promotion
time. Required-but-empty fields are still allowed through (they
are the *expected* state of a draft and the "needs ..." banner
handles them).
Plus three style cleanups suggested in the same review:
- DraftNode type lifted to module scope so other files can reference
it.
- defaultParamsForKind wrapper deleted; the only call site now uses
draftDefaultParamsForKind directly.
- onNodesDelete narrows next.delete(...) to the actually-deleted
draft ids instead of all deleted nodes.
A state-machine vitest (drop → fill → debounce → addNode-fired;
drop → fill → addNode → no-echo → timeout → toast) would be valuable
but requires extracting the lifecycle out of MonitorView into a
testable hook to avoid mounting React Flow + ToastContext +
useSession + useTuneNode + ResizeObserver in jsdom. The existing
draftNodes.test.ts covers the pure helpers (mergeDraftParam,
computeMissingRequired); the state-machine layer remains covered
only by Playwright e2e for now.
Signed-off-by: StreamKit Devin <devin@streamkit.dev>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| export const computeMissingRequired = ( | ||
| kind: string, | ||
| params: Record<string, unknown>, | ||
| nodeDefinitions: NodeDefinition[] | ||
| ): string[] => { | ||
| const def = nodeDefinitions.find((d) => d.kind === kind); | ||
| const schema = def?.param_schema as Record<string, unknown> | undefined; | ||
| const required = schema?.['required']; | ||
| if (!Array.isArray(required)) return []; | ||
| return required.filter((k): k is string => typeof k === 'string' && isMissingValue(params[k])); |
There was a problem hiding this comment.
📝 Info: computeMissingRequired only checks top-level schema required — nested required fields are ignored
The computeMissingRequired function (draftNodes.ts:39-41) only reads the top-level required array from param_schema and checks top-level keys in params. JSON Schema allows required at nested levels (e.g., inside a nested properties object), but those are not checked. This aligns with how the StreamKit engine validates required params (top-level schema validation only), and the test suite covers this behavior explicitly. Not a bug, but worth noting for future schema extensions.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
|
|
||
| const kind = type; | ||
| const nodeId = generateName(kind); | ||
| const params = defaultParamsForKind(kind); | ||
|
|
||
| // Cache the position for when the node appears in the pipeline | ||
| pendingNodePositions.current.set(nodeId, position); | ||
|
|
||
| // Send to server immediately | ||
| addNode(nodeId, kind, params); | ||
| const params = draftDefaultParamsForKind(kind, nodeDefinitions); | ||
|
|
||
| // If any required params have no schema default, hold the node as a | ||
| // local-only draft until the user fills them in. This avoids | ||
| // round-tripping a guaranteed-to-fail `addnode` (e.g. servo without | ||
| // `url`, slint without `slint_file`, kokoro/piper/matcha without | ||
| // `model_dir`) and the cleanup churn that follows. See the topology | ||
| // effect for how drafts are merged into the React Flow graph. | ||
| // | ||
| // Drafts carry their own position (rendered directly via the draft | ||
| // branch in the topology effect) so we only seed pendingNodePositions | ||
| // for the immediate-commit path. When a draft is later promoted, | ||
| // its current rendered position is already in `prevPositions` — no | ||
| // need to round-trip through the pending map. | ||
| const missing = computeMissingRequired(kind, params, nodeDefinitions); | ||
| if (missing.length > 0) { | ||
| setDraftNodes((prev) => { | ||
| const next = new Map(prev); | ||
| next.set(nodeId, { kind, params, position, missingRequired: missing }); | ||
| return next; | ||
| }); | ||
| setSelectedNodes([nodeId]); | ||
| setRightPaneView('inspector'); | ||
| if (rightCollapsed) { | ||
| setRightCollapsed(false); | ||
| } | ||
| toast.info(`Configure ${missing.join(', ')} before this node is added to the pipeline`); | ||
| } else { | ||
| // All required params satisfied — commit immediately. | ||
| pendingNodePositions.current.set(nodeId, position); | ||
| addNode(nodeId, kind, params); | ||
| } | ||
|
|
||
| setType(null); | ||
| }; |
There was a problem hiding this comment.
📝 Info: No guard against dropping a draft when no session is selected
The onDrop handler at ui/src/views/MonitorView.tsx:1562 creates a draft without checking whether selectedSessionId is non-null. If a user somehow drops a node when no session is selected, a draft is created but can never be promoted (addNode from useSession(null) is a no-op: if (!sessionId) return at useSession.ts:109). In practice this is unreachable because the node palette and canvas are only visible when a session is selected, so this is defensive-programming territory rather than a real bug.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Without this, the per-node atomFamily accumulated entries keyed by `<oldSessionId>\0<draftNodeId>` every time the user switched sessions with drafts in flight. Functionally harmless (the new session uses different keys) but a slow leak across many session switches. Track the previous selectedSessionId in a ref and call clearNodeParams for every still-pending draft id keyed by the old session before resetting the draft state for the new one. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| const node = buildNodeObject({ | ||
| nodeName: draftId, | ||
| apiNode: { | ||
| kind: draft.kind, | ||
| params: draft.params as JsonValue, | ||
| state: null, | ||
| }, | ||
| position: draftPos, | ||
| nodeState: undefined, | ||
| finalInputs: draftFinalInputs, | ||
| finalOutputs: draftFinalOutputs, | ||
| nodeDef: draftDef, | ||
| stableOnParamChange, | ||
| stableOnConfigChange, | ||
| selectedSessionId, | ||
| draft: { missingRequired: draft.missingRequired }, | ||
| }); | ||
| newNodes.push(node); | ||
| } |
There was a problem hiding this comment.
📝 Info: Draft param atom seeding relies on fallback chain rather than explicit initialization
When a draft is created via onDrop, its params (schema defaults) are stored in draftNodes state but NOT written to the Jotai nodeParamsAtom. The atom starts empty {}. InspectorPane's fallback chain (ui/src/panes/InspectorPane.tsx:356-359) reads the atom first, then falls back to node.data.params, then schema.default. This works correctly because node.data.params carries the draft's default params from the topology effect.
However, in-canvas controls (e.g. useNumericSlider) read propValue from data.params which only updates on topology rebuilds. Editing a non-required field from an in-canvas control would update the Jotai atom but NOT trigger a topology rebuild (since missingRequired didn't change), so the control's propValue prop stays stale. This means in-canvas controls on drafts may exhibit snap-back behavior. This is a UX limitation rather than a correctness bug — drafts are designed for inspector-driven editing.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
The WebSocket addnode handler was broadcasting `NodeAdded` synchronously with the request, before the engine actor had even attempted plugin construction. For plugins that rejected construction (e.g. servo with an invalid url, slint without a slint_file) the UI saw `nodeadded`, dropped the local draft, then routed subsequent keystrokes through the live tune path, producing "Could not tune non-existent node" warnings and no clear UI feedback for the failure. Tightens the contract so `nodeadded` means what it says: the plugin's constructor and `initialize_node` both returned Ok and the node is in the pipeline. Engine ------ * New `NodeAddedNotification` and `subscribe_node_added` on the dynamic engine handle. The actor emits one notification per successful creation, after `flush_pending_connections` / `flush_pending_tunes`, never on failure (Failed surfaces via the existing `NodeStateChanged` channel). * Subscribers are unbounded — same model as `runtime_schema_subscribers`: node creations are one-per-node and low-frequency, dropping a notification leaves a client without any `nodeadded` event for that node, which is worse than the memory pressure of a dead receiver. * Documents a known gap in the actor's duplicate-id rejection path: the rejection is logged but no event reaches the originating WS client. Multi-client concurrent dups slip past the WS handler's best-effort pre-check, but the UI's auto-generated `<kind>_<n>` naming makes the race vanishingly rare in practice. Fixing this properly would need either a oneshot result channel (rejected as unidiomatic for an event-driven engine) or a new dedicated event. Server ------ * `apps/skit/src/session.rs` spawns a forwarder task that, on each `NodeAddedNotification`, inserts into `pipeline.nodes` *and* then broadcasts `EventPayload::NodeAdded`. Late `getpipeline` calls observe the inserted entry consistently with the broadcast. * `apps/skit/src/websocket_handlers.rs::handle_add_node` no longer pre-inserts into `pipeline.nodes` or pre-broadcasts. Forwards the control message and returns `Success` (request accepted). The duplicate-id check on `pipeline.nodes` stays as a best-effort guard; the actor's `node_states` check is authoritative. UI -- * Promotion is now an explicit user action: a primary "Add to pipeline" button in the canvas-side draft banner. Disabled when required fields are missing or while the request is in flight. Typing into a draft never auto-promotes — no debounce, no shadow in-flight set, no "did the last keystroke complete the required set?" branching. * Draft banner has three states: "needs <fields>" / "ready" / spinner + "creating on server". Inspector inputs are disabled while `isCreating` so post-promote keystrokes don't race the engine response — that disabling lets us drop the `latestDraftParamsRef` shadow ref and the late-flush effect entirely. * `ConfigurableNode` hides canvas-side `SchemaControls` (Toggle, Text, Slider) while the node is a draft. Those controls call `useTuneNode` directly, bypassing draft-aware routing — for a draft the engine has no entry yet and would warn "Could not tune non-existent node". Drafts are configured exclusively from the Inspector. * Deleted from `MonitorView.tsx`: the 8s promotion-timeout effect, the state-watcher / cleanup machinery that tried to reconstruct "did the node actually get created?" from out-of-band signals, the topology "prefer draft over live" hack, and all promotion shadow refs. Cleanup now keys on `pipeline.nodes[id]` (which actually means "created") for success, and a single `nodeStateAtom` Failed subscriber for the revert-and-removenode path. Tests ----- * `session_lifecycle_test::test_addnode_failure_leaves_pipeline_empty` locks in the contract: addnode for an unregistered kind returns Success on dispatch but leaves `pipeline.nodes` empty (no phantom entry). Failure is observable only via NodeStateChanged. * `test_add_and_remove_nodes` now uses `audio::gain` and a `wait_for_node_added` helper, since pipeline.nodes only populates after creation confirmation rather than synchronously with the request. AGENTS.md --------- * New "Fix Root Causes, Not Symptoms" section documenting this incident as the worked example: 13 commits of UI machinery papered over a contract bug that a small server change collapsed. Lists concrete signals that you've crossed into workaround territory.
`mapServerLayers` materializes `serverOnly: true` stub LayerStates for pins the server reports as resolved but for which `properties.layers` has no explicit config (e.g. a freshly-connected source with auto-aspect-fit). `serializeLayers` intentionally skips these stubs so the server-side compositor can keep resolving geometry rather than being pinned to explicit config. Side effect: dragging or resizing such a stub from the compositor preview updated local state but never reached the server, and the next `NodeViewDataUpdated` sync reverted the visual change. `setLayers` is the user-mutation path — drag/resize/keyboard/overlay handlers route through it, while sync-from-props and server sync write directly via `setLayersInStore`. So a write through `setLayers` is the user's signal that they want explicit config. Clear `serverOnly` on every layer in the new state; `serializeLayers` then includes the layer, the tune reaches the server, and subsequent view-data syncs preserve the user's geometry. Unrelated to the draft-promotion change; split into its own commit.
staging-devin-ai-integration
Bot
changed the title
2 participants
Summary
Root-cause fix: the WebSocket
nodeaddedevent used to fire inhandle_add_nodebefore the plugin's constructor orinitialize_nodehad even started. This meantpipeline.nodescontained entries for nodes that didn't yet exist in the engine, and the UI had to reconstruct "did the node actually get created?" from out-of-band signals — leading to 13+ commits of increasingly fragile draft-state machinery.The fix: emit
nodeaddedfrom the engine actor's success path (handle_node_created) instead of the WS handler. A session-level node-added forwarder subscribes toNodeAddedNotificationand only inserts intopipeline.nodes+ broadcastsNodeAddedonce the engine confirmsOk. This collapses the UI back to the obvious cleanup.Key changes by layer
Engine (
crates/engine/):NodeCreatedEventnow carriesparams; on success,handle_node_createdemitsNodeAddedNotificationto all subscribersSubscribeNodeAddedquery message +subscribe_node_added()handle methodSession (
apps/skit/src/session.rs):creating_nodes: Arc<Mutex<HashSet<String>>>— in-flight reservation setreserve_node_id()/release_node_id()— atomically checks pipeline + in-flight; documented lock order (pipeline → creating_nodes)pipeline.nodes, broadcastsNodeAdded, drains reservationWebSocket handlers (
apps/skit/src/websocket_handlers.rs):handle_add_nodecallsreserve_node_idbefore forwarding to engine; no longer inserts into pipeline or broadcasts NodeAddedhandle_remove_nodecallsrelease_node_idfor cleanupUI — Draft nodes (
ui/src/views/MonitorView.tsx):DraftNodetype for UI-only nodes dropped on canvasaddNode) → cleanup (onnodeaddedarrival in pipeline)NodeState::Failed)UI — Compositor stale-echo fixes:
isStaleViewData()now treats empty sender (\"\") as "ours" — fixes first-drag snap-back on auto-PiP layerspromoteEditedServerOnly()clearsserverOnlyonly on layers the user actually edited (identity check)compositorDragResizereadslayersRef.current(post-commit store) instead of closure-captured value_sender/_revinto view-data (even defaults)layer_configs_dirtyvia parameter instead of clearing sender/revresetAllConfigRevs()on WS reconnect (server resets config_rev on restart)Servo plugin (
plugins/native/servo/):handle_registerreturns immediately; loading progresses via event loop ticks inhandle_renderpost_load_doneflag gates one-shot CSS injectionwait_for_loadandnudge_frameblocking helpersPlugin SDK (
sdks/plugin-sdk/native/):null_mut())AGENTS.md: Added "Fix Root Causes, Not Symptoms" section documenting the lesson learned
Review & Testing Checklist for Human
reserve_node_id: verify that all code paths acquirepipelinebeforecreating_nodes(the joint lock is the correctness invariant preventing a node from slipping in between the two checks)transport::whip_inputwith a required URL) onto the Monitor canvas — verify the draft banner appears, filling in the URL enables the "Add to pipeline" button, clicking it creates the node, and a bogus URL surfaces the failure toast and returns the draft to editable stateNotes
gpu_tests::*) fail on environments without GPU access — this is pre-existing and unrelated to this branchtest_addnode_failure_leaves_pipeline_empty,test_addnode_rejects_duplicate_in_flight)test_add_and_remove_nodesintegration test was updated: usesaudio::gainkind (wasgain),gain_dbparam (wasgain), and waits forNodeAddedevent before querying pipelineLink to Devin session: https://staging.itsdev.in/sessions/738851b209314449971ec830d4682f53
Requested by: @streamer45