refactor(ui): Monitor View performance & state management improvements#170
refactor(ui): Monitor View performance & state management improvements#170staging-devin-ai-integration[bot] wants to merge 13 commits intomainfrom
Conversation
Buffer nodeparamschanged events alongside node-state and node-stats updates and flush all three data types in a single Zustand set() call per animation frame. This eliminates the drift between sessionStore.pipeline.params and nodeParamsStore — both are now updated in lockstep. The sessionStore.pipeline becomes the single source of truth for params, with nodeParamsStore retained only as an optimistic overlay for immediate UI feedback. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Extract three custom hooks from the ~1850-line MonitorViewContent component: - useMonitorSessionManager: session selection, auto-select, delete logic - useMonitorStaging: staging mode enter/exit/commit/discard lifecycle - useMonitorYaml: YAML sync, 280-line handleYamlChange callback, regeneration effect Also applies two inline performance fixes: - P1 1D: resolveDynamicPins uses nodesRef.current instead of nodes array to avoid recreating the callback on every RF interaction - P1 1E: hoist getState() call above the topology effect loop instead of calling it N times Circular dependency between session selection and auto-layout is solved via an onSessionActivatedRef callback bridge. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…r view Add a serverLayoutAppliedRef gate so the 'sync from props' effect only preserves geometry once the server has provided layout data at least once. Before the first server layout, config-parsed geometry is used as a fallback so layers are visible immediately on mount. This eliminates the race where a params echo-back (nodeparamschanged) could arrive after a server layout update (nodeviewdataupdated) and briefly apply stale config-parsed positions before being corrected. The ref resets when sessionId changes, ensuring new sessions start with config-parsed geometry until their server layout arrives. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Wrap onEdgesDelete, onNodesDelete, onDrop, and onDragOver in useCallback to prevent stale closure captures in the centerPanel useMemo. Previously these were plain functions that captured isInStagingMode and selectedSessionId by closure but were NOT in the useMemo dependency list — they could act on stale state when staging mode was toggled or sessions were switched. The callbacks now read staging mode from useStagingStore.getState() at call time (matching the pattern already used by stableOnParamChange and stableOnConfigChange), making them safe to include in the dependency list without causing extra re-renders. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| const typedParams = params as Record<string, unknown>; | ||
|
|
||
| // Optimistic overlay for immediate UI feedback (e.g. slider controls | ||
| // that read from nodeParamsStore while the sessionStore pipeline | ||
| // hasn't been updated yet). | ||
| useNodeParamsStore.getState().setParams(node_id, typedParams, session_id); | ||
|
|
||
| // Also buffer the update for the session store's pipeline so the | ||
| // canonical source of truth stays in sync. The update is flushed | ||
| // via the same RAF batch as node states/stats, so multiple param | ||
| // events within one frame produce exactly one store mutation. | ||
| let sessionMap = this.pendingNodeParams.get(session_id); | ||
| if (!sessionMap) { | ||
| sessionMap = new Map(); | ||
| this.pendingNodeParams.set(session_id, sessionMap); | ||
| } | ||
| const existing = sessionMap.get(node_id); | ||
| sessionMap.set(node_id, existing ? { ...existing, ...typedParams } : typedParams); | ||
| this.scheduleBatchFlush(); |
There was a problem hiding this comment.
🚩 Pipeline now updates on every param change — potential re-render increase
The old code had updateNodeParams commented out with a warning: "This is problematic because it causes re-renders that cause issues with react flow." This PR re-enables pipeline param updates via the RAF-batched batchUpdateSessionData path (websocket.ts:313-320), which merges params into session.pipeline.nodes.
Since useSession subscribes to pipeline via useSessionStore (useSession.ts:57-59), every batched param update now produces a new pipeline reference, triggering a re-render of MonitorViewContent. The old code avoided this entirely.
The mitigation is that RAF batching coalesces all param events within one frame into a single store mutation. Additionally, topoKey is based on node kinds/connections (not params), so the expensive topology effect won't re-run. The YAML regeneration effect will re-run but is lightweight. This should be safe, but is worth monitoring for performance regressions in pipelines with high-frequency param changes (e.g., rapid slider tuning with echo-back).
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
There was a problem hiding this comment.
Correct — this is the intentional fix for the P0 2A issue (dual param store). Previously sessionStore.pipeline.nodes[x].params was permanently stale after any tune operation because the WS handler explicitly skipped the update (with the WARNING comment). Now both stores stay in sync via the same RAF batch. The key mitigant: batchUpdateSessionData does a shallow merge ({ ...existingNode, params: { ...existingNode.params, ...update } }) inside a single set(), so selectors that depend on the pipeline fire at most once per frame — the same pattern already used for nodeStates/nodeStats.
The handleYamlChange callback sets isEditingYamlRef to true before parsing. When YAML is structurally valid but lacks a 'nodes' key, the early return skipped resetting the ref, permanently blocking the YAML regeneration effect until page reload. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Extract node interaction callbacks (drag/drop, connect, delete) into a dedicated useMonitorNodeActions hook: - Connection handlers (onConnect, onConnectEnd) - Edge and node deletion (onEdgesDelete, onNodesDelete) - Drag/drop events (onDragStart, onDrop, onDragOver) - Name generation (generateName) and default params (getDefaultParams) - Context menu actions (handleDuplicateNode, handleDeleteNode) Also documents why useMonitorTopology extraction was intentionally skipped: the topology effect has 15+ dependencies including bidirectional coupling with useNodeStatesSubscription (via topoKey/topoEffectRanRef) and cross-hook deps on useMonitorYaml, making extraction add more interface complexity than it removes. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| isLoadingPipeline, | ||
| handleStartPreview, | ||
| isPreviewConnected, | ||
| handleCommitChanges, | ||
| handleDiscardChanges, | ||
| handleEnterStagingMode, | ||
| handleDeleteModalOpen, | ||
| onNodesChangeBatched, | ||
| onEdgesChange, | ||
| onNodeDragStop, | ||
| handleNodeDoubleClick, | ||
| onConnect, | ||
| onConnectEnd, | ||
| onEdgesDelete, | ||
| onNodesDelete, | ||
| onDrop, | ||
| onDragOver, | ||
| onPaneClick, | ||
| onPaneContextMenu, | ||
| onNodeContextMenu, | ||
| ] |
There was a problem hiding this comment.
📝 Info: centerPanel useMemo now tracks many more dependencies
The old code intentionally used sparse useMemo dependencies (with eslint-disable) for centerPanel, omitting handler references like onDrop, onConnect, etc. The new code adds all handler callbacks to the dependency array (lines 848-871). Most of these are stable useCallback references, but onDrop depends on type from useDnD() which changes on every drag-start. This means the centerPanel is recreated when a drag begins from the palette.
This is MORE correct than the old code (which used stale closures for handlers not in deps) and the re-render cost is minimal since it only happens once per drag operation. The trade-off of correctness over marginal perf is appropriate here.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Remove the staging mode feature entirely from the Monitor View. All node mutations now go through WebSocket immediately (live editing). Deleted files (~1,630 lines): - stagingStore.ts (605 lines) + stagingStore.test.ts (495 lines) - useMonitorStaging.ts (156 lines) - pipelineDiff.ts (124 lines) - StagingModeIndicator.tsx (251 lines) Simplified files: - MonitorView.tsx: removed ~200 lines of staging state, branching, validation effect, isStaged sync effect, staging props - useMonitorNodeActions.ts: collapsed staging branches to direct server calls, removed 6 staging-related params - useMonitorYaml.ts: removed 280-line handleYamlChange callback, YAML is now read-only - TopControls.tsx: removed staging enter/commit/discard buttons and change summary UI - LeftPanel.tsx: removed editMode prop, Nodes Library always visible - useAutoLayout.ts: removed updateNodePosition (staging store) - useMonitorSessionManager.ts: removed staging store dependency - pipelineGraph.ts: removed isStaged from buildNodeObject params - Node components: removed isStaged from data types, simplified showLiveIndicator logic 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.
…on switch When switching between sessions with identical pipeline topology, topoKey was identical (it only encoded node names/kinds/connections). The topology effect early-returned, leaving nodes with stale stableOnParamChange callbacks bound to the old session's tuneNode. Fix: include selectedSessionId in the topoKey computation so switching sessions always forces a node rebuild. The computation is extracted into a testable computeTopoKey() utility in pipelineGraph.ts with unit tests covering the session-identity dimension and other key properties. 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.
onInit was a plain function recreated every render, defeating the centerPanel useMemo (which included it in its deps). Since baseOnInit from useReactFlowCommon is already a stable useCallback([]), wrapping onInit with useCallback([baseOnInit]) makes it stable too. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Perf baseline files should only be committed by human maintainers who have benchmarked on bare-metal idle systems. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
| defaultEdgeOptions={defaultEdgeOptions} | ||
| editMode={editMode} | ||
| onNodeDragStop={onNodeDragStop} | ||
| editMode={true} |
There was a problem hiding this comment.
🚩 editMode hardcoded to true removes the read-only canvas constraint
At MonitorView.tsx:809, editMode={true} is now hardcoded where previously it was editMode={editMode} (which was isInStagingMode). This means the canvas is always editable — users can always drag/drop nodes, create connections, and delete elements, with all mutations going directly to the server via WebSocket. Previously, in non-staging 'monitor' mode, the canvas was read-only and users had to explicitly enter staging mode to make changes. This is a significant UX change: accidental node deletions or connection changes now immediately affect the running pipeline with no undo mechanism (the staging store provided a discard option). The YAML pane is correctly set to yamlReadOnly={true} (line 901), preventing direct YAML editing.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
…callbacks - Rewrite useMonitorYaml to read params exclusively from sessionStore.pipeline (single source of truth); remove nodeParamsStore reads. Debounce param-only YAML regeneration (300ms) to avoid O(N·log N) dumps on every RAF frame. - Unify duplicated YAML generation: extract pipelineToYaml() convenience wrapper in pipelineGraph.ts, remove inlined buildYaml from useMonitorYaml. - Make onDrop ref-based for DnD type to prevent centerPanel memo invalidation on every drag-start. - Extend useNodeStatesSubscription to also trigger on pipeline param changes (not just nodeStates), ensuring RAF-batched param echo-backs flow into ReactFlow node data. - Make onSessionActivatedRef assignment idiomatic (useEffect instead of render body). - Document stale nodes closure in topology effect with rationale for the intentional eslint-disable. - Add eslint-disable comments with rationale for ref-based useCallback deps in useMonitorNodeActions. - Add perf regression tests for useMonitorYaml covering debounce behavior, structural change bypass, and pipeline-null clearing. Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
The pipeline param tracking added in useNodeStatesSubscription (A5) causes stale server echo-backs to flow into compositor RF node data.params. The compositor useEffect[params,...] then calls mergeOverlayState which, under the old policy, re-added items present in parsed but absent from current — resurrecting locally-deleted overlays. Change reconciliation policy: - Items in parsed AND current → merged (preserve visibility/geometry) - Items only in current → kept (locally-added, pending confirmation) - Items only in parsed → skipped (prevents stale echo re-add) Update unit tests to match the new policy. Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
There was a problem hiding this comment.
🚩 OVERLAY_BASE_KEYS does not include cropZoom/cropX/cropY — pre-existing, not changed by PR
The OVERLAY_BASE_KEYS set in compositorLayerParsers.ts:325-337 does not include cropZoom, cropX, or cropY. When preserveGeometry=true, pickConfigFields treats these as "config fields" and takes them from parsed (config-derived) values rather than preserving the server-resolved values. Since the server provides crop_zoom, crop_x, crop_y in mapServerLayers, this could theoretically overwrite server values with config-echoed values. However, this is pre-existing — the OVERLAY_BASE_KEYS set and pickConfigFields were not modified in this PR. The PR only changes when preserveGeometry is true (now gated behind serverLayoutAppliedRef instead of just isMonitorView).
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| useEffect(() => { | ||
| if (!selectedSessionId && !isLoadingSessions && sessions.length > 0) { | ||
| const sessionId = sessions[0].id; | ||
| setSelectedSessionId(sessionId); | ||
| notifyActivated(sessionId); | ||
| } | ||
| }, [selectedSessionId, isLoadingSessions, sessions, notifyActivated]); |
There was a problem hiding this comment.
🔴 onSessionActivatedRef not wired when initial auto-select effects fire, causing missing auto-layout
On initial page load, useMonitorSessionManager auto-selects the first session (line 96-100) and calls onSessionActivatedRef.current(sessionId, false) (line 77). However, onSessionActivatedRef is initialized as a no-op () => {} (ui/src/views/MonitorView.tsx:142) and only wired to the real handler in a separate useEffect (ui/src/views/MonitorView.tsx:367-372). React fires effects in registration order — the auto-select effects inside useMonitorSessionManager are registered before the wiring effect in MonitorViewContent, so the auto-select calls the no-op. This means setNeedsAutoLayout(true) and setNeedsFit(true) are never called for the initially auto-selected session, leaving all nodes stacked at position {x: 0, y: 0}. The same issue affects the navigation-state auto-select path (lines 83-91).
Prompt for agents
The root cause is that onSessionActivatedRef is wired in a useEffect in MonitorViewContent (ui/src/views/MonitorView.tsx:367-372), but the auto-select effects in useMonitorSessionManager (ui/src/hooks/useMonitorSessionManager.ts:83-101) fire before it. To fix this, wire the ref synchronously in the render body of MonitorViewContent instead of in a useEffect. Change ui/src/views/MonitorView.tsx around lines 364-372 from:
useEffect(() => {
onSessionActivatedRef.current = (_sessionId: string, hasPositions: boolean) => {
setNeedsAutoLayout(!hasPositions);
setNeedsFit(true);
};
}, [setNeedsAutoLayout, setNeedsFit, onSessionActivatedRef]);
to a direct assignment in the render body (before useMonitorSessionManager is called), or initialize the ref with the real handler instead of a no-op. Since setNeedsAutoLayout and setNeedsFit are stable state setters, a synchronous ref assignment is safe:
onSessionActivatedRef.current = (_sessionId: string, hasPositions: boolean) => {
setNeedsAutoLayout(!hasPositions);
setNeedsFit(true);
};
Note that this assignment must appear BEFORE the useMonitorSessionManager call at line 147. This may require reordering the hooks: move the useAutoLayout call (which creates setNeedsAutoLayout/setNeedsFit) before the ref assignment, and move the ref assignment before useMonitorSessionManager.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| const statesChanged = nodeStates !== prevNodeStates; | ||
| const pipelineChanged = pipeline !== prevPipeline; | ||
| if (!statesChanged && !pipelineChanged) return; | ||
| prevNodeStates = nodeStates; | ||
| prevPipeline = pipeline; |
There was a problem hiding this comment.
🟡 useNodeStatesSubscription reads stale pipelineRef when triggered by pipeline param changes
The subscription in useNodeStatesSubscription now triggers when pipeline reference changes (ui/src/hooks/useNodeStatesSubscription.ts:215), but applyPatch reads params from pipelineRef.current (line 80) which is a React ref updated during render — not from the pipeline that triggered the subscription. Since the Zustand subscription fires synchronously before React re-renders, pipelineRef.current still holds the previous pipeline when applyPatch runs. This means the RF node data.params (used by the inspector panel) gets patched with stale params, defeating the purpose of triggering on pipeline changes. The stale data persists until another store update triggers the subscription.
Stale ref read path
The subscription at line 207 receives the new pipeline from the store, but applyPatch at line 80 reads pipelineRef.current which hasn't been updated yet:
// subscription fires (store updated) -> pipelineRef.current is stale
const currentPipeline = pipelineRef.current; // OLD pipeline
const nextParams = apiNode.params; // OLD params
React re-render happens later, updating pipelineRef.current, but the subscription won't fire again since prevPipeline was already updated at line 218.
Prompt for agents
In ui/src/hooks/useNodeStatesSubscription.ts, the applyPatch function reads pipeline from pipelineRef.current (line 80), which is stale when the subscription fires. To fix this, either:
1. Pass the pipeline from the subscription into applyPatch. Change the applyPatch signature to accept an optional pipeline parameter, and when pipelineChanged is true, pass the pipeline from the subscription:
const applyPatch = (nodeStates: Record<string, NodeState>, pipelineOverride?: Pipeline | null) => {
const currentPipeline = pipelineOverride ?? pipelineRef.current;
...
};
Then at the call site around line 253, pass `session?.pipeline` when pipeline changed:
applyPatch(nodeStates, pipelineChanged ? pipeline : undefined);
2. Or read the pipeline directly from the store inside applyPatch using useSessionStore.getState() instead of pipelineRef.current.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| nodeDefinitions={nodeDefinitions} | ||
| readOnly={!editMode} | ||
| yamlReadOnly={!isInStagingMode} | ||
| readOnly={false} |
There was a problem hiding this comment.
🚩 YAML is now read-only in Monitor view (handleYamlChange removed)
The right pane now passes yamlReadOnly={true} (MonitorView.tsx:911) and no longer passes onYamlChange. Previously, YAML editing was supported in staging mode with a complex handler that parsed YAML, rebuilt the pipeline, and dispatched tune events. This was a significant feature removal. The YAML pane is now purely a read-only display of the pipeline state. This is consistent with the PR's goal of removing staging mode, but reviewers should confirm that the YAML editing capability is not needed.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
2 participants
Summary
Performance, state management, and simplification improvements for the Monitor View.
Core architecture changes
Single source of truth for params —
useMonitorYamlnow reads params exclusively fromsessionStore.pipeline.nodes[x].params. ThenodeParamsStoreoverlay is no longer consulted for YAML generation, eliminating the dual-read inconsistency window.Staging mode removal (~1,630 lines deleted) — All node mutations go through WebSocket immediately. Removed
stagingStore,pipelineDiff,StagingModeIndicator, and staging branches across 12 files.RAF-batched param updates —
handleNodeParamsChangedbuffers intopendingNodeParamsand flushes via the same RAF cycle as states/stats, collapsing N param events per frame into oneset()call.Performance improvements
onDrop— preventscenterPanelmemo invalidation on every drag-startuseNodeStatesSubscriptiontracks pipeline changes — RAF-batched param echo-backs now flow into ReactFlow node data without full re-renderspipelineToYaml()inpipelineGraph.ts, removed duplicatedbuildYamlfromuseMonitorYamlserverLayoutAppliedRefgate prevents stale config-parsed positions from overwriting server layoutHook extraction (from ~114 statements to ~85)
useMonitorSessionManager– session selection, auto-select, delete modalsuseMonitorYaml– debounced YAML regeneration (read-only)useMonitorNodeActions– drag/drop, connect, delete callbacksCode quality
onSessionActivatedRefassignment moved touseEffect(idiomatic)nodesclosure in topology effect with rationaleuseMonitorYaml(debounce, structural bypass, null clearing)Review & Testing Checklist for Human
centerPaneldoesn't flicker/re-mount during drag.Notes
nodeParamsStoreis intentionally kept for Design view and immediate UI controls (sliders, inspector). Only the YAML generation path was decoupled from it.useNodeStatesSubscription(edge-alert lambda) is pre-existing and out of scope for this PR.just test), lint clean (just lint), TypeScript clean.Link to Devin session: https://staging.itsdev.in/sessions/750ac353d44c4ea8b0cd6530ef50f99e
Requested by: @streamer45