Conversation
| */ | ||
| export const PARAM_THROTTLE_MS = 33; |
There was a problem hiding this comment.
🚩 PARAM_THROTTLE_MS changed from 100ms to 33ms affects all slider controls
The default throttle for parameter updates was changed from 100ms to 33ms (~30 updates/sec) via the new PARAM_THROTTLE_MS constant. This affects both the compositor layer updates (useCompositorLayers.ts:125) and all generic numeric sliders (useNumericSlider.ts:77). While the constant is well-documented as balancing responsiveness vs network load, the 3x increase in update frequency could increase WebSocket traffic and server load, especially with multiple concurrent slider interactions. The perf-baselines.json shows dramatically improved render counts (22→2), suggesting the zero-render optimization compensates for the higher frequency on the client side. However, the server-side impact should be validated under load.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
🟡 commitLayerAppearance overwrites concurrent React state changes made during slider drag
When sliderActiveRef is true during a zero-render opacity/rotation drag, the layersRef sync from React state is skipped (useCompositorLayers.ts:168). If any concurrent operation calls setLayers() during this window — such as arrow-key nudges (compositorKeyboard.ts:108-111), visibility toggles (compositorOverlays.ts:204), mirror toggles (compositorOverlays.ts:245), or reorder (compositorOverlays.ts:499) — those changes update React state but not layersRef. When the slider is released, commitLayerAppearance() calls setLayers([...layersRef.current]) with a direct value (not a functional updater), overwriting the concurrent changes.
Reproduction steps
- Select a compositor layer (wrapper gets keyboard focus)
- Start dragging the opacity slider (sliderActiveRef = true)
- While holding the mouse on the slider, press an arrow key to nudge
- The nudge calls setLayers(prev => ...), updating React state
- But layersRef is NOT synced (guarded by sliderActiveRef)
- Release the slider → commitLayerAppearance sets layers from stale layersRef
- The arrow-key position change is lost
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| "opacity-slider-20-ticks": { | ||
| "name": "opacity-slider-20-ticks", | ||
| "meanRenderCount": 22, | ||
| "meanRenderCount": 2, |
There was a problem hiding this comment.
🚩 Perf baselines show dramatic render count reduction (22→2) — verify test correctness
The perf-baselines.json changes show meanRenderCount dropping from 22 to 2 for opacity, rotation, and mixed slider tests. This 11x improvement is consistent with the zero-render optimization (skipping setLayers during drags means the measured component doesn't re-render). However, a count of 2 for 20 slider ticks means the component renders only on initial mount + final commit, not during the drag at all. If the perf test's measureRenders is counting the component under test (not the slider), this validates that the memoization barriers are working. Worth confirming the test still exercises the same code path and that the dramatic improvement isn't from accidentally skipping the interaction.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| function layerArrayGeometryEqual(a: readonly LayerState[], b: readonly LayerState[]): boolean { | ||
| if (a.length !== b.length) return false; | ||
| for (let i = 0; i < a.length; i++) { | ||
| const la = a[i]; | ||
| const lb = b[i]; | ||
| if ( | ||
| la.id !== lb.id || | ||
| la.x !== lb.x || | ||
| la.y !== lb.y || | ||
| la.width !== lb.width || | ||
| la.height !== lb.height || | ||
| la.zIndex !== lb.zIndex || | ||
| la.visible !== lb.visible || | ||
| la.mirrorHorizontal !== lb.mirrorHorizontal || | ||
| la.mirrorVertical !== lb.mirrorVertical | ||
| ) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🚩 areCanvasPropsEqual intentionally skips opacity/rotation but this affects server-pushed updates too
The custom memo comparator areCanvasPropsEqual (CompositorCanvas.tsx:72-100) and layerArrayGeometryEqual (CompositorCanvas.tsx:107-127) intentionally skip opacity and rotationDegrees for video layers to avoid re-renders during slider drags. This is correct for the local slider path (DOM is already updated directly). However, it also prevents re-rendering when the server pushes opacity/rotation changes via mapServerLayers in compositorServerSync.ts:33-73. If the server independently modifies these values (e.g., a multi-client scenario or server-side automation), the canvas would display stale opacity/rotation until the next geometry change triggers a re-render. In the typical single-client case, echo-backs carry the same values the client sent, so this is unlikely to be visible. Worth considering whether a rare server-push edge case justifies adding these fields back to the comparator.
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
1 participant
Summary
Trying to improve perf of the compositor UI which is pretty terrible atm.