fix: compositor node reads params from Jotai atom for remote sync#408
Conversation
Subscribe to nodeParamsAtom in the default session store via useParamAtomSync (non-React subscription) and merge config-driven fields (opacity, rotation, z_index, mirror, crop, text content, image asset path) into the compositor's per-instance Jotai store. Geometry (x, y, width, height) continues to come from useServerLayoutSync. Client-only fields (visible, measured text dimensions, serverOnly) are preserved from existing state. This avoids subscribing at the CompositorNode level (which would cause 27x re-render cascades) while still picking up remote param changes that arrive via WebSocket nodeparamschanged events. Closes #398 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}
| return { | ||
| ...p, | ||
| x: existing.x, | ||
| y: existing.y, | ||
| width: existing.width, | ||
| height: existing.height, | ||
| visible: existing.visible, | ||
| opacity: existing.visible ? p.opacity : existing.opacity, | ||
| serverOnly: existing.serverOnly, | ||
| }; |
There was a problem hiding this comment.
🔴 Explicit remote layer configs keep serverOnly and get skipped on later commits
When a layer that was materialized from server view data later appears in remote params, this merge keeps serverOnly: true even though the layer now has explicit config. That contradicts the existing monitor sync path, which clears serverOnly once parsed params contain the layer (ui/src/hooks/useCompositorLayers.ts:354-365). Because serializeLayers omits every serverOnly layer (ui/src/hooks/compositorLayerParsers.ts:335-340), any later full compositor commit, such as an unrelated overlay edit through commitOverlays (ui/src/hooks/compositorCommit.ts:87-90), serializes a config that drops that remotely-added layer config and can overwrite the server with the layer missing.
| return { | |
| ...p, | |
| x: existing.x, | |
| y: existing.y, | |
| width: existing.width, | |
| height: existing.height, | |
| visible: existing.visible, | |
| opacity: existing.visible ? p.opacity : existing.opacity, | |
| serverOnly: existing.serverOnly, | |
| }; | |
| return { | |
| ...p, | |
| x: existing.x, | |
| y: existing.y, | |
| width: existing.width, | |
| height: existing.height, | |
| visible: existing.visible, | |
| opacity: existing.visible ? p.opacity : existing.opacity, | |
| }; |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| if (dragStateRef.current) return; | ||
| if (activeInteractionRef?.current) return; |
There was a problem hiding this comment.
🟡 Remote param updates are permanently dropped during active interactions
If nodeParamsAtom changes while dragStateRef.current or activeInteractionRef.current is set, applyRemoteParams returns without recording that skipped value. The subscription only calls it when the atom changes (ui/src/hooks/compositorParamSync.ts:150-151), and ending an inspector interaction only mutates a ref (ui/src/nodes/CompositorNode.tsx:150-155), so it does not trigger this effect again. A remote opacity/text/image update that arrives during a local slider drag can therefore be ignored until some later unrelated param update or remount.
Prompt for agents
In ui/src/hooks/compositorParamSync.ts, avoid dropping nodeParamsAtom updates just because an interaction is currently active. The current guard returns from applyRemoteParams and there is no retry when activeInteractionRef.current flips back to false, because refs do not notify React or Jotai. Consider keeping the latest skipped params in a ref and arranging for it to be applied when the interaction ends, or moving the suppression logic to only ignore known stale self-echoes while still preserving/applying the latest remote value after the guard clears. The fix likely needs coordination with the CompositorNode interaction callbacks that set activeInteractionRef.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| import { parseLayers, parseTextOverlays, parseImageOverlays } from './compositorLayerParsers'; | ||
| import type { LayerState, TextOverlayState, ImageOverlayState } from './compositorLayerParsers'; | ||
|
|
||
| // ── Pure merge helpers ────────────────────────────────────────────────────── |
There was a problem hiding this comment.
🟡 New section divider comment violates repository comment rules
AGENTS.md explicitly lists section dividers such as // --- Public Modules --- under “Do NOT write”. This new file adds the same kind of section divider comment here, with additional instances in ui/src/hooks/compositorParamSync.ts:108, ui/src/hooks/compositorParamSync.test.ts:101, ui/src/hooks/compositorParamSync.test.ts:257, and the added ui/src/hooks/useCompositorLayers.ts:417, so the PR violates the repository’s mandatory comment guidelines.
Prompt for agents
Remove the section divider comments added by this PR and rely on code structure, blank lines, or extracted names instead. AGENTS.md explicitly disallows section dividers, and the new compositorParamSync implementation/test additions include several of them.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| describe('useParamAtomSync integration', () => { | ||
| it('remote param write propagates config to compositor store', () => { | ||
| const sessionId = 'test-session'; | ||
| const nodeId = 'compositor-1'; | ||
| const key = nodeKey(sessionId, nodeId); | ||
|
|
||
| const store = createStore(); | ||
| setLayersInStore(store, [ | ||
| makeLayer('in_0', { x: 160, y: 0, width: 960, height: 720, opacity: 1.0 }), | ||
| ]); | ||
|
|
||
| writeNodeParams( | ||
| nodeId, | ||
| { | ||
| width: 1280, | ||
| height: 720, | ||
| layers: { | ||
| in_0: { opacity: 0.5, z_index: 0, rotation_degrees: 30 }, | ||
| }, | ||
| text_overlays: [], | ||
| image_overlays: [], | ||
| }, | ||
| sessionId | ||
| ); | ||
|
|
||
| const atomParams = sessionStore.get(nodeParamsAtom(key)); | ||
| expect(atomParams).toBeDefined(); | ||
| expect((atomParams.layers as Record<string, Record<string, unknown>>)?.in_0?.opacity).toBe(0.5); | ||
|
|
||
| // Clean up | ||
| sessionStore.set(nodeParamsAtom(key), {}); | ||
| }); |
There was a problem hiding this comment.
🚩 The “integration” test does not exercise the sync hook or compositor store
This test writes to nodeParamsAtom and asserts the atom value changed, but it never mounts useParamAtomSync/useCompositorLayers and never reads the compositor store that was initialized with setLayersInStore. As a result, it would pass even if the subscription in useParamAtomSync were removed entirely; it does not verify the intended atom → compositor-store propagation described by the test name. A stronger test should mount the hook or call the sync pathway and assert that the layer in the compositor store receives the remote opacity/rotation while preserving geometry.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| const paramsAtom = nodeParamsAtom(nodeKey(sessionId, nodeId)); | ||
| const current = defaultSessionStore.get(paramsAtom); | ||
| applyRemoteParams(current); | ||
|
|
||
| const unsub = defaultSessionStore.sub(paramsAtom, () => { | ||
| applyRemoteParams(defaultSessionStore.get(paramsAtom)); | ||
| }); |
There was a problem hiding this comment.
📝 Info: Remote sync deliberately bypasses React rendering via the default Jotai store
The new hook reads and subscribes to nodeParamsAtom(nodeKey(sessionId, nodeId)) through defaultSessionStore rather than via useAtomValue. This matches the session-scoped writers in ui/src/stores/sessionAtoms.ts:82-99 and WebSocket handling in ui/src/services/websocket.ts:340-371, so the provider-less subscription itself is not a wrong-store bug; it is the intended way to reach session atoms from inside the compositor’s separate Provider-scoped store.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
…-count test - Trim module-level JSDoc to ~5 fewer lines (point 4) - Add comment explaining why merge helpers diverge from mergeOverlayState: atom writes skip pickChangedConfigFields because WS rev check deduplicates (point 1) - Add 'visible is client-only' rationale at the 3 opacity-preservation sites (point 3) - Add render-counting test proving useParamAtomSync does not re-render its host component on atom writes (10 writes → 1 mount render) (point 2) 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>
Alphabetize @/ imports and merge duplicate ./compositorParamSync imports into a single statement. The previous ordering triggered the import/order rule which crashed on CI's eslint-plugin-import version. Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
2 participants
Summary
Fixes the compositor node not picking up remote param changes from other clients. The
CompositorNodepreviously read params exclusively fromdata.params(ReactFlow data prop), which only refreshes on full pipeline/topology rebuild. Remote param changes arriving via WebSocketnodeparamschanged→writeNodeParams()→ JotainodeParamsAtomwere never consumed by the compositor.New module:
compositorParamSync.ts— subscribes tonodeParamsAtomin the default (provider-less) session store usingdefaultSessionStore.sub()(non-React subscription), then merges config-driven fields into the compositor's per-instance Jotai store. This mirrors the pattern used bycompositorServerSync.tsfor view-data geometry.What syncs from remote params: opacity, rotation, z_index, mirror flags, crop/zoom, aspect_fit, text content/font/color, image asset path.
What is preserved from existing state: geometry (x, y, width, height) — owned by
useServerLayoutSync; client-onlyvisibleflag;measuredTextWidth/measuredTextHeight;serverOnlyflag. Hidden layers preserve their stored opacity to avoid losing the user's last-set value.Stale echo gating: Two layers — the WebSocket handler's sender-nonce + rev check filters self-echoes before they reach
nodeParamsAtom, and theactiveInteractionRef/dragStateRefguards in the hook skip updates during local slider drags.Closes #398
Review & Testing Checklist for Human
just perf-ui— all 4 scenarios should report "unchanged" render counts (opacity: 2, rotation: 2, param-echo: 11, mixed: 2).Notes
useParamAtomSynchook only activates in Monitor view (whensessionIdis present). Design view continues to usedata.paramsexclusively.width/height) from remote clients are NOT handled by this hook — they still require a topology rebuild. This is intentional to keep scope focused; it could be a follow-up if needed.sync-from-propseffect inuseCompositorLayersstill runs on topology rebuilds. Both paths write to the same compositor store with field-level equality checks, so redundant writes are no-ops.UI / Lint, Test & Buildcheck is failing withTypeError: sourceCode.getTokenOrCommentBefore is not a functionin ESLint 10.2.1 — this is a pre-existing CI infrastructure issue (the first CI run on this PR passed the same check on the same code). All tests pass locally (569/569).Link to Devin session: https://staging.itsdev.in/sessions/0ddde4a9101941d38bc3d07cce13768b
Requested by: @streamer45
Devin Review
c6ac6ec(HEAD iscba15f7)