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/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); }); } 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'); }; 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; +}