From ae5bce043cdfcdce1755c950c6459a5dd3bf1c9b Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Wed, 19 Nov 2025 12:28:46 +0200 Subject: [PATCH 1/3] Fix version switch rendering with store batching Add isSyncing flag to WorkflowStore to batch observer notifications during initial sync, reducing notifications from 8 to 1. This ensures all workflow data (jobs, triggers, edges, positions) arrives together in a single update instead of triggering 8 progressive React renders with incomplete data. The batching mechanism: - Sets isSyncing flag before running all observers - Suppresses notify() calls during sync - Sends single 'connected' notification after all data loaded This is the first part of fixing version switch visual artifacts. The second part (diagram rendering guard) prevents the diagram from updating until ReactFlow is ready. --- .../stores/createWorkflowStore.ts | 22 ++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/assets/js/collaborative-editor/stores/createWorkflowStore.ts b/assets/js/collaborative-editor/stores/createWorkflowStore.ts index 8e641d329d..7478d25b7a 100644 --- a/assets/js/collaborative-editor/stores/createWorkflowStore.ts +++ b/assets/js/collaborative-editor/stores/createWorkflowStore.ts @@ -261,6 +261,10 @@ export const createWorkflowStore = () => { let observerCleanups: (() => void)[] = []; let provider: (PhoenixChannelProvider & { channel: Channel }) | null = null; + // Flag to suppress notifications during initial sync + // This prevents multiple React renders when connect() populates state + let isSyncing = false; + let state = produceInitialState(); const listeners = new Set<() => void>(); @@ -332,7 +336,12 @@ export const createWorkflowStore = () => { if (nextState !== lastState) { lastState = nextState; state = nextState; - notify(actionName); + // Skip notification if we're in the middle of a batch sync + if (!isSyncing) { + notify(actionName); + } else { + logger.debug('skipping notify due to isSyncing', { actionName }); + } } }; @@ -600,23 +609,26 @@ export const createWorkflowStore = () => { updateDerivedState(draft); }); - // Initial sync from Y.Doc to state + // Initial sync from Y.Doc to state - batch all updates + // This prevents 7 intermediate React renders with incomplete data + isSyncing = true; workflowObserver(); jobsObserver(); triggersObserver(); edgesObserver(); positionsObserver(); - errorsObserver(); // NEW: Initial sync + errorsObserver(); - // Update state with undoManager + // Update state with undoManager (still batched) updateState(draft => { draft.undoManager = undoManager; }, 'undoManager/initialized'); + isSyncing = false; // Initialize DevTools connection devtools.connect(); - // Send initial state + // Send initial state - this is the ONLY notification/render notify('connected'); }; From 552a2a537c8f12d364da7da94928939f3f12c8b9 Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Wed, 19 Nov 2025 12:29:01 +0200 Subject: [PATCH 2/3] Fix diagram rendering and improve resize with debounce Add early return guard in WorkflowDiagram to prevent model updates until ReactFlow is initialized. This eliminates visual artifacts during version switches where nodes would flash at position (0,0) before layout runs. Replace throttle with promise-aware debounce for resize handling: - Waits 200ms after resize completes before fitting - Won't start new fit if previous promise is pending - Uses AbortController for clean cancellation - Validates bounds before calling fitBounds - Proper error handling with logging Remove cacheTimeout logic that maintained bounds across resize sessions. With debounce ensuring only one fit per resize, caching is unnecessary and could cause unexpected jumps if user panned between resizes. Together with store batching, version switching now shows smooth transitions with no flashing, jumping, or recentering artifacts. --- .../components/diagram/WorkflowDiagram.tsx | 137 ++++++++++++++---- .../js/collaborative-editor/utils/debounce.ts | 77 ++++++++++ 2 files changed, 184 insertions(+), 30 deletions(-) create mode 100644 assets/js/collaborative-editor/utils/debounce.ts diff --git a/assets/js/collaborative-editor/components/diagram/WorkflowDiagram.tsx b/assets/js/collaborative-editor/components/diagram/WorkflowDiagram.tsx index ef6fa4b17b..171c0b7d26 100644 --- a/assets/js/collaborative-editor/components/diagram/WorkflowDiagram.tsx +++ b/assets/js/collaborative-editor/components/diagram/WorkflowDiagram.tsx @@ -7,7 +7,6 @@ import { type NodeChange, ReactFlow, ReactFlowProvider, - type Rect, useReactFlow, } from '@xyflow/react'; import React, { useCallback, useEffect, useRef, useState } from 'react'; @@ -23,6 +22,7 @@ import { } from '#/collaborative-editor/hooks/useWorkflow'; import type { Workflow } from '#/collaborative-editor/types/workflow'; import { getAdaptorDisplayName } from '#/collaborative-editor/utils/adaptorUtils'; +import debounce from '#/collaborative-editor/utils/debounce'; import { isSourceNodeJob } from '#/collaborative-editor/utils/workflowGraph'; import { randomUUID } from '#/common'; import _logger from '#/utils/logger'; @@ -36,7 +36,6 @@ import usePlaceholders from '#/workflow-diagram/usePlaceholders'; import { ensureNodePosition } from '#/workflow-diagram/util/ensure-node-position'; import fromWorkflow from '#/workflow-diagram/util/from-workflow'; import shouldLayout from '#/workflow-diagram/util/should-layout'; -import throttle from '#/workflow-diagram/util/throttle'; import updateSelectionStyles from '#/workflow-diagram/util/update-selection'; import { getVisibleRect, @@ -112,7 +111,6 @@ export default function WorkflowDiagram(props: WorkflowDiagramProps) { // one in props is always set on initial render. (helps with refresh) const { selection, onSelectionChange, containerEl: el, runSteps } = props; - // Get Y.Doc workflow store for placeholder operations const workflowStore = useWorkflowStoreContext(); // Get workflow actions including position updates @@ -357,6 +355,24 @@ export default function WorkflowDiagram(props: WorkflowDiagramProps) { // This usually means the workflow has changed or its the first load, so we don't want to animate // Later, if responding to changes from other users live, we may want to animate useEffect(() => { + logger.debug('main useEffect triggered', { + jobCount: workflow.jobs.length, + triggerCount: workflow.triggers.length, + edgeCount: workflow.edges.length, + workflowPositionsCount: Object.keys(workflowPositions).length, + cachedPositionsCount: Object.keys(chartCache.current.positions).length, + isManualLayout, + hasFlow: !!flow, + }); + + // Don't update model until ReactFlow is initialized + // This prevents visual artifacts during version switches where nodes + // would flash at position (0,0) before ReactFlow is ready + if (!flow) { + logger.debug('flow not initialized yet, skipping model update'); + return; + } + // Clear cache if positions were cleared (e.g., after reset workflow) // This prevents stale cached positions from being used when Y.Doc positions are empty // Also clear lastLayout so shouldLayout() will trigger a new layout @@ -364,6 +380,7 @@ export default function WorkflowDiagram(props: WorkflowDiagramProps) { Object.keys(workflowPositions).length === 0 && Object.keys(chartCache.current.positions).length > 0 ) { + logger.debug('clearing cached positions'); chartCache.current.positions = {}; chartCache.current.lastLayout = undefined; } @@ -377,8 +394,6 @@ export default function WorkflowDiagram(props: WorkflowDiagramProps) { } // create model from workflow and also apply selection styling to the model. - logger.log('calling fromWorkflow'); - const newModel = updateSelectionStyles( fromWorkflow( workflow, @@ -391,6 +406,16 @@ export default function WorkflowDiagram(props: WorkflowDiagramProps) { ), selection ); + + logger.debug('fromWorkflow result', { + nodeCount: newModel.nodes.length, + edgeCount: newModel.edges.length, + nodeIds: newModel.nodes.map(n => n.id), + hasPositions: newModel.nodes.map(n => ({ + id: n.id, + hasPos: !!n.position, + })), + }); if (newModel.nodes.length > 0) { // If defaulting positions for multiple nodes, // try to offset them a bit @@ -405,12 +430,26 @@ export default function WorkflowDiagram(props: WorkflowDiagramProps) { chartCache.current.lastLayout ); + logger.debug('shouldLayout decision', { + layoutId, + lastLayout: chartCache.current.lastLayout, + isManualLayout, + willLayout: !!layoutId, + }); + if (layoutId) { chartCache.current.lastLayout = layoutId; const viewBounds = { width: workflowDiagramRef.current?.clientWidth ?? 0, height: workflowDiagramRef.current?.clientHeight ?? 0, }; + logger.debug('layout triggered', { + layoutId, + isManualLayout, + viewBounds, + nodeCount: newModel.nodes.length, + }); + if (isManualLayout) { // give nodes positions const nodesWPos = newModel.nodes.map(node => { @@ -435,17 +474,36 @@ export default function WorkflowDiagram(props: WorkflowDiagramProps) { }); setModel({ ...newModel, nodes: nodesWPos }); chartCache.current.positions = workflowPositions; + logger.debug('manual layout: applied positions from store', { + positionCount: Object.keys(workflowPositions).length, + }); } else { + logger.debug('auto layout: calling layout()', { + nodePositions: newModel.nodes.map(n => ({ + id: n.id, + pos: n.position, + })), + viewBounds, + }); void layout(newModel, setModel, flow, viewBounds, { duration: props.layoutDuration ?? LAYOUT_DURATION, forceFit: props.forceFit ?? false, }).then(positions => { // Note we don't update positions until animation has finished chartCache.current.positions = positions; + logger.debug('auto layout: completed', { + positionCount: Object.keys(positions).length, + positions, + }); return positions; }); } } else { + logger.debug('no layout needed: using cached/stored positions', { + isManualLayout, + cachedPositionCount: Object.keys(positions).length, + workflowPositionCount: Object.keys(workflowPositions).length, + }); // if isManualLayout, then we use values from store instead newModel.nodes.forEach(n => { if (isManualLayout && n.type !== 'placeholder') { @@ -465,6 +523,7 @@ export default function WorkflowDiagram(props: WorkflowDiagramProps) { setModel(newModel); } } else { + logger.debug('flow not initialized: setting positions on newModel'); // Flow not initialized yet, but we have nodes - ensure positions first newModel.nodes.forEach(n => { if (isManualLayout && n.type !== 'placeholder') { @@ -482,6 +541,7 @@ export default function WorkflowDiagram(props: WorkflowDiagramProps) { setModel(newModel); } } else if (workflow.jobs.length === 0 && placeholders.nodes.length === 0) { + logger.debug('empty workflow: clearing canvas'); // Explicitly empty workflow - show empty state // Only clear canvas when BOTH workflow.jobs and placeholders are empty // This prevents blank canvas during race conditions where placeholder @@ -614,26 +674,20 @@ export default function WorkflowDiagram(props: WorkflowDiagramProps) { ); // Trigger a fit to bounds when the parent div changes size - // To keep the chart more stable, try and take a snapshot of the target bounds - // when a new resize starts - // This will be imperfect but stops the user completely losing context + // Debounced to wait until resize completes before fitting useEffect(() => { if (flow && el) { let isFirstCallback = true; - let cachedTargetBounds: Rect | null = null; - let cacheTimeout: NodeJS.Timeout | undefined; - - const throttledResize = throttle(() => { - if (cacheTimeout) clearTimeout(cacheTimeout); - - // After 3 seconds, clear the timeout and take a new cache snapshot - cacheTimeout = setTimeout(() => { - cachedTargetBounds = null; - }, 3000); + const debouncedResize = debounce( + async (signal: AbortSignal) => { + // Caller responsibility: only called when flow exists + if (!flow) { + logger.warn('fitBounds called without flow instance'); + return; + } - if (!cachedTargetBounds) { - // Take a snapshot of what bounds to try and maintain throughout the resize + // Compute bounds based on current viewport const viewBounds = { width: el.clientWidth || 0, height: el.clientHeight || 0, @@ -642,27 +696,50 @@ export default function WorkflowDiagram(props: WorkflowDiagramProps) { const visible = model.nodes.filter(n => isPointInRect(n.position, rect) ); - cachedTargetBounds = flow.getNodesBounds(visible); - } + const targetBounds = flow.getNodesBounds(visible); + + // Validate rect has finite numbers (borrowed from safeFitBoundsRect logic) + const isValidRect = + targetBounds && + Number.isFinite(targetBounds.x) && + Number.isFinite(targetBounds.y) && + Number.isFinite(targetBounds.width) && + Number.isFinite(targetBounds.height); + + if (!isValidRect) { + logger.warn('fitBounds called with invalid rect', targetBounds); + return; + } - // Run an animated fit - flow.fitBounds(cachedTargetBounds, { - duration: FIT_DURATION, - padding: FIT_PADDING, - }); - }, FIT_DURATION * 2); + // Check if aborted before async operation + if (signal.aborted) { + return; + } + + try { + // Wait for fitBounds animation to complete + await flow.fitBounds(targetBounds, { + duration: FIT_DURATION, + padding: FIT_PADDING, + }); + } catch (err) { + logger.error('fitBounds failed', err); + } + }, + 200 // ~200ms after resize stops + ); const resizeOb = new ResizeObserver(_entries => { if (!isFirstCallback) { // Don't fit when the listener attaches (it callsback immediately) - throttledResize(); + debouncedResize(); } isFirstCallback = false; }); resizeOb.observe(el); return () => { - throttledResize.cancel(); + debouncedResize.cancel(); resizeOb.unobserve(el); }; } diff --git a/assets/js/collaborative-editor/utils/debounce.ts b/assets/js/collaborative-editor/utils/debounce.ts new file mode 100644 index 0000000000..c7ca7855bb --- /dev/null +++ b/assets/js/collaborative-editor/utils/debounce.ts @@ -0,0 +1,77 @@ +export type DebouncedFunction = { + (...args: T): void; + cancel: () => void; +}; + +/** + * Creates a debounced function that delays invoking func until after delay + * milliseconds have elapsed since the last time the debounced function was invoked. + * + * Features: + * - Promise-aware: Waits for previous invocation to complete before allowing next + * - Cancellable: Uses AbortController for cancellation + * + * @param fn - The function to debounce (receives AbortSignal as first argument) + * @param delay - The number of milliseconds to delay + * @returns The debounced function with a cancel method + */ +export default function debounce( + fn: (signal: AbortSignal, ...args: T) => void | Promise, + delay: number +): DebouncedFunction { + let timeoutId: NodeJS.Timeout | undefined; + let pendingPromise: Promise | null = null; + let abortController: AbortController | null = null; + + const debounced = (...args: T) => { + // Cancel any pending debounce timeout + clearTimeout(timeoutId); + + // If there's a pending promise, don't start a new timeout + // (Wait for current operation to complete first) + if (pendingPromise) { + return; + } + + timeoutId = setTimeout(() => { + void (async () => { + // Cancel previous abort controller if exists + if (abortController) { + abortController.abort(); + } + + abortController = new AbortController(); + + try { + const result = fn(abortController.signal, ...args); + if (result instanceof Promise) { + pendingPromise = result; + await result; + } + } catch (err) { + // Ignore abort errors, re-throw others + if (err instanceof Error && err.name !== 'AbortError') { + throw err; + } + } finally { + pendingPromise = null; + abortController = null; + } + })(); + }, delay); + }; + + debounced.cancel = () => { + clearTimeout(timeoutId); + timeoutId = undefined; + + if (abortController) { + abortController.abort(); + abortController = null; + } + + pendingPromise = null; + }; + + return debounced; +} From 802d066731e2e9ed033ebbd16bd594cbea65728c Mon Sep 17 00:00:00 2001 From: Stuart Corbishley Date: Wed, 19 Nov 2025 12:33:26 +0200 Subject: [PATCH 3/3] Rearrange import statements --- .../hooks/useSessionContext.ts | 8 +- .../stores/createSessionStore.ts | 90 +++++++++---------- 2 files changed, 49 insertions(+), 49 deletions(-) diff --git a/assets/js/collaborative-editor/hooks/useSessionContext.ts b/assets/js/collaborative-editor/hooks/useSessionContext.ts index d206e20720..4068793fc5 100644 --- a/assets/js/collaborative-editor/hooks/useSessionContext.ts +++ b/assets/js/collaborative-editor/hooks/useSessionContext.ts @@ -32,16 +32,16 @@ * ``` */ -import { useSyncExternalStore, useContext } from 'react'; +import { useContext, useSyncExternalStore } from 'react'; import { StoreContext } from '../contexts/StoreProvider'; import type { SessionContextStoreInstance } from '../stores/createSessionContextStore'; import type { - UserContext, - ProjectContext, - ProjectRepoConnection, AppConfig, Permissions, + ProjectContext, + ProjectRepoConnection, + UserContext, } from '../types/sessionContext'; /** diff --git a/assets/js/collaborative-editor/stores/createSessionStore.ts b/assets/js/collaborative-editor/stores/createSessionStore.ts index 783a57b45e..955884f83a 100644 --- a/assets/js/collaborative-editor/stores/createSessionStore.ts +++ b/assets/js/collaborative-editor/stores/createSessionStore.ts @@ -29,21 +29,21 @@ * Y.Doc, Provider, Awareness (too large/circular) */ -import { produce } from "immer"; -import { Socket as PhoenixSocket } from "phoenix"; -import { PhoenixChannelProvider } from "y-phoenix-channel"; -import type * as awarenessProtocol from "y-protocols/awareness"; -import { Awareness } from "y-protocols/awareness"; -import { Doc as YDoc } from "yjs"; +import { produce } from 'immer'; +import type { Socket as PhoenixSocket } from 'phoenix'; +import { PhoenixChannelProvider } from 'y-phoenix-channel'; +import type * as awarenessProtocol from 'y-protocols/awareness'; +import { Awareness } from 'y-protocols/awareness'; +import { Doc as YDoc } from 'yjs'; -import _logger from "#/utils/logger"; +import _logger from '#/utils/logger'; -import type { LocalUserData } from "../types/awareness"; +import type { LocalUserData } from '../types/awareness'; -import { createWithSelector, type WithSelector } from "./common"; -import { wrapStoreWithDevTools } from "./devtools"; +import { createWithSelector, type WithSelector } from './common'; +import { wrapStoreWithDevTools } from './devtools'; -const logger = _logger.ns("SessionStore").seal(); +const logger = _logger.ns('SessionStore').seal(); export interface SessionState { ydoc: YDoc | null; @@ -110,8 +110,8 @@ export const createSessionStore = (): SessionStore => { // Redux DevTools integration const devtools = wrapStoreWithDevTools({ - name: "SessionStore", - excludeKeys: ["ydoc", "provider", "awareness"], // Exclude large Y.js objects + name: 'SessionStore', + excludeKeys: ['ydoc', 'provider', 'awareness'], // Exclude large Y.js objects maxAge: 100, }); @@ -122,7 +122,7 @@ export const createSessionStore = (): SessionStore => { // Helper to update state const updateState: UpdateFn = ( updater, - actionName = "updateState" + actionName = 'updateState' ) => { const nextState = produce(state, updater); // const changedKeys = Object.keys(nextState).filter(key => { @@ -153,7 +153,7 @@ export const createSessionStore = (): SessionStore => { const ydoc = new YDoc(); updateState(draft => { draft.ydoc = ydoc; - }, "initializeYDoc"); + }, 'initializeYDoc'); return ydoc; }; @@ -171,7 +171,7 @@ export const createSessionStore = (): SessionStore => { draft.ydoc = null; draft.awareness = null; draft.userData = null; - }, "destroyYDoc"); + }, 'destroyYDoc'); }; const initializeSession = ( @@ -185,7 +185,7 @@ export const createSessionStore = (): SessionStore => { ) => { // Atomic initialization to prevent partial states if (!socket) { - throw new Error("Socket must be connected before initializing session"); + throw new Error('Socket must be connected before initializing session'); } // Step 1: Use existing YDoc or create new one @@ -212,7 +212,7 @@ export const createSessionStore = (): SessionStore => { draft.provider = provider; draft.userData = userData; draft.isSynced = provider.synced; - }, "initializeSession"); + }, 'initializeSession'); // Step 5: Attach provider event handlers and store cleanup function cleanupProviderHandlers?.(); // Clean up any existing handlers @@ -239,7 +239,7 @@ export const createSessionStore = (): SessionStore => { * - Resets all state to null */ const destroy = () => { - logger.debug("Destroying session"); + logger.debug('Destroying session'); // Clean up all event handlers first cleanupProviderHandlers?.(); cleanupProviderHandlers = null; @@ -264,7 +264,7 @@ export const createSessionStore = (): SessionStore => { draft.isSynced = false; draft.settled = false; draft.lastStatus = null; - }, "destroy"); + }, 'destroy'); }; // Queries (CQS) @@ -327,39 +327,39 @@ function attachProvider( const next = event.status; if (next) { - const nowConnected = next === "connected"; + const nowConnected = next === 'connected'; updateState(draft => { draft.isConnected = nowConnected; draft.lastStatus = next; - }, "connectionStatusChange"); + }, 'connectionStatusChange'); } }; const syncHandler = (synced: boolean) => { updateState(draft => { draft.isSynced = synced; - }, "syncStatusChange"); + }, 'syncStatusChange'); }; - provider.on("status", statusHandler); - provider.on("sync", syncHandler); + provider.on('status', statusHandler); + provider.on('sync', syncHandler); return () => { - provider.off("status", statusHandler); - provider.off("sync", syncHandler); + provider.off('status', statusHandler); + provider.off('sync', syncHandler); }; } function createSettlingSubscription( - subscribe: SessionStore["subscribe"], - getSnapshot: SessionStore["getSnapshot"], + subscribe: SessionStore['subscribe'], + getSnapshot: SessionStore['getSnapshot'], updateState: UpdateFn ) { const state = getSnapshot(); if (!state.provider || !state.ydoc) { throw new Error( - "Provider and YDoc must be initialized before creating settling subscription" + 'Provider and YDoc must be initialized before creating settling subscription' ); } @@ -368,8 +368,8 @@ function createSettlingSubscription( // Type-cast state since we've already verified provider and ydoc exist const stateWithProviderAndYdoc = state as SessionState & { - provider: NonNullable; - ydoc: NonNullable; + provider: NonNullable; + ydoc: NonNullable; }; const startSettling = () => { @@ -380,7 +380,7 @@ function createSettlingSubscription( // Reset settled state updateState(draft => { draft.settled = false; - }, "settlingStarted"); + }, 'settlingStarted'); // Start waiting for settled state Promise.all([ @@ -390,9 +390,9 @@ function createSettlingSubscription( .then(() => { if (!currentController?.signal.aborted) { updateState(draft => { - logger.debug("Settled"); + logger.debug('Settled'); draft.settled = true; - }, "settledStatusChange"); + }, 'settledStatusChange'); } return undefined; }) @@ -417,7 +417,7 @@ function createSettlingSubscription( currentController?.abort(); updateState(draft => { draft.settled = false; - }, "disconnected"); + }, 'disconnected'); } }; @@ -438,7 +438,7 @@ function createSettlingSubscription( function waitForChannelSynced( controller: AbortController, state: SessionState & { - provider: NonNullable; + provider: NonNullable; } ) { const { provider } = state; @@ -449,7 +449,7 @@ function waitForChannelSynced( // } const cleanup = () => { - provider.off("sync", handler); + provider.off('sync', handler); }; const handler = (synced: boolean) => { @@ -459,9 +459,9 @@ function waitForChannelSynced( } }; - provider.on("sync", handler); + provider.on('sync', handler); - controller.signal.addEventListener("abort", () => { + controller.signal.addEventListener('abort', () => { cleanup(); resolve(); }); @@ -471,15 +471,15 @@ function waitForChannelSynced( function waitForFirstUpdate( controller: AbortController, state: SessionState & { - ydoc: NonNullable; - provider: NonNullable; + ydoc: NonNullable; + provider: NonNullable; } ) { const { ydoc, provider } = state; return new Promise(resolve => { const cleanup = () => { - ydoc.off("update", handler); + ydoc.off('update', handler); }; const handler = (_update: Uint8Array, origin: unknown) => { @@ -489,11 +489,11 @@ function waitForFirstUpdate( } }; - controller.signal.addEventListener("abort", () => { + controller.signal.addEventListener('abort', () => { cleanup(); resolve(); }); - ydoc.on("update", handler); + ydoc.on('update', handler); }); }