From 391c3efa14e95b1b1dafba977a97e45e19ffe49b Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 25 Jun 2025 16:46:56 -0700 Subject: [PATCH] improvement(sockets): fixed positioning of blocks recalc, hydration issue with workspaceId, and presence (#547) * fixed positioning of blocks recalc, hydration issue with workspaceId, and presence * fixed loading animation, auto-close workspace selector onClick --- .../user-avatar-stack/user-avatar-stack.tsx | 5 +- .../workspace-header/workspace-header.tsx | 27 ++--- .../w/hooks/use-registry-loading.ts | 111 +++++++----------- .../app/workspace/[workspaceId]/w/page.tsx | 23 +++- apps/sim/components/ui/loading-agent.tsx | 19 +-- apps/sim/hooks/use-collaborative-workflow.ts | 42 ++++++- apps/sim/socket-server/handlers/connection.ts | 8 -- apps/sim/socket-server/handlers/operations.ts | 78 ++++++++---- apps/sim/socket-server/handlers/workflow.ts | 2 +- 9 files changed, 185 insertions(+), 130 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/control-bar/components/user-avatar-stack/user-avatar-stack.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/control-bar/components/user-avatar-stack/user-avatar-stack.tsx index 2489fda062..3bf9259c1c 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/control-bar/components/user-avatar-stack/user-avatar-stack.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/control-bar/components/user-avatar-stack/user-avatar-stack.tsx @@ -43,8 +43,9 @@ export function UserAvatarStack({ } }, [users, maxVisible]) - // Don't render anything if there are no users - if (users.length === 0) { + // Only show presence when there are multiple users (>1) + // Don't render anything if there are no users or only 1 user + if (users.length <= 1) { return null } diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx index 3d67c0087d..4ffc0ba6de 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx @@ -239,10 +239,9 @@ WorkspaceEditModal.displayName = 'WorkspaceEditModal' export const WorkspaceHeader = React.memo( ({ onCreateWorkflow, isCollapsed, onDropdownOpenChange }) => { // Get sidebar store state to check current mode - const { mode, workspaceDropdownOpen, setAnyModalOpen } = useSidebarStore() + const { mode, workspaceDropdownOpen, setWorkspaceDropdownOpen, setAnyModalOpen } = + useSidebarStore() - // Keep local isOpen state in sync with the store (for internal component use) - const [isOpen, setIsOpen] = useState(workspaceDropdownOpen) const { data: sessionData, isPending } = useSession() const [plan, setPlan] = useState('Free Plan') // Use client-side loading instead of isPending to avoid hydration mismatch @@ -335,14 +334,14 @@ export const WorkspaceHeader = React.memo( const switchWorkspace = useCallback( (workspace: Workspace) => { - // If already on this workspace, do nothing + // If already on this workspace, close dropdown and do nothing else if (activeWorkspace?.id === workspace.id) { - setIsOpen(false) + setWorkspaceDropdownOpen(false) return } setActiveWorkspace(workspace) - setIsOpen(false) + setWorkspaceDropdownOpen(false) // Use full workspace switch which now handles localStorage automatically switchToWorkspace(workspace.id) @@ -350,7 +349,7 @@ export const WorkspaceHeader = React.memo( // Update URL to include workspace ID router.push(`/workspace/${workspace.id}/w`) }, - [activeWorkspace?.id, switchToWorkspace, router] + [activeWorkspace?.id, switchToWorkspace, router, setWorkspaceDropdownOpen] ) const handleCreateWorkspace = useCallback( @@ -472,7 +471,7 @@ export const WorkspaceHeader = React.memo( setActiveWorkspace(updatedWorkspaces[0]) } - setIsOpen(false) + setWorkspaceDropdownOpen(false) } catch (err) { logger.error('Error deleting workspace:', err) } finally { @@ -504,13 +503,13 @@ export const WorkspaceHeader = React.memo( // Notify parent component when dropdown opens/closes const handleDropdownOpenChange = useCallback( (open: boolean) => { - setIsOpen(open) + setWorkspaceDropdownOpen(open) // Inform the parent component about the dropdown state change if (onDropdownOpenChange) { onDropdownOpenChange(open) } }, - [onDropdownOpenChange] + [onDropdownOpenChange, setWorkspaceDropdownOpen] ) // Special handling for click interactions in hover mode @@ -521,10 +520,10 @@ export const WorkspaceHeader = React.memo( e.stopPropagation() e.preventDefault() // Toggle dropdown state - handleDropdownOpenChange(!isOpen) + handleDropdownOpenChange(!workspaceDropdownOpen) } }, - [mode, isOpen, handleDropdownOpenChange] + [mode, workspaceDropdownOpen, handleDropdownOpenChange] ) const handleContainerClick = useCallback( @@ -568,7 +567,7 @@ export const WorkspaceHeader = React.memo( workspace={editingWorkspace} /> - +
( href={workspaceUrl} className='group flex h-6 w-6 shrink-0 items-center justify-center rounded bg-[#802FFF]' onClick={(e) => { - if (isOpen) e.preventDefault() + if (workspaceDropdownOpen) e.preventDefault() }} > diff --git a/apps/sim/app/workspace/[workspaceId]/w/hooks/use-registry-loading.ts b/apps/sim/app/workspace/[workspaceId]/w/hooks/use-registry-loading.ts index 239a7148db..7e8196fc1c 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/hooks/use-registry-loading.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/hooks/use-registry-loading.ts @@ -1,35 +1,12 @@ 'use client' -import { useEffect } from 'react' +import { useEffect, useState } from 'react' import { useParams, usePathname, useRouter } from 'next/navigation' import { createLogger } from '@/lib/logs/console-logger' import { useWorkflowRegistry } from '@/stores/workflows/registry/store' const logger = createLogger('UseRegistryLoading') -/** - * Extract workflow ID from pathname - * @param pathname - Current pathname - * @returns workflow ID if found, null otherwise - */ -function extractWorkflowIdFromPathname(pathname: string): string | null { - try { - const pathSegments = pathname.split('/') - // Check if URL matches pattern /w/{workflowId} - if (pathSegments.length >= 3 && pathSegments[1] === 'w') { - const workflowId = pathSegments[2] - // Basic UUID validation (36 characters, contains hyphens) - if (workflowId && workflowId.length === 36 && workflowId.includes('-')) { - return workflowId - } - } - return null - } catch (error) { - logger.warn('Failed to extract workflow ID from pathname:', error) - return null - } -} - /** * Custom hook to manage workflow registry loading state and handle first-time navigation * @@ -43,35 +20,55 @@ export function useRegistryLoading() { const params = useParams() const workspaceId = params.workspaceId as string - // Load workflows for current workspace + // Track hydration state to prevent premature API calls + const [isHydrated, setIsHydrated] = useState(false) + + // Handle client-side hydration useEffect(() => { - if (workspaceId) { - loadWorkflows(workspaceId).catch((error) => { - logger.warn('Failed to load workflows for workspace:', error) - }) + setIsHydrated(true) + }, []) + + // Load workflows for current workspace only after hydration + useEffect(() => { + // Only proceed if we're hydrated and have a valid workspaceId + if ( + !isHydrated || + !workspaceId || + typeof workspaceId !== 'string' || + workspaceId.trim() === '' + ) { + return } - }, [workspaceId, loadWorkflows]) + + logger.debug('Loading workflows for workspace:', workspaceId) + loadWorkflows(workspaceId).catch((error) => { + logger.warn('Failed to load workflows for workspace:', error) + }) + }, [isHydrated, workspaceId, loadWorkflows]) // Handle first-time navigation: if we're at /w and have workflows, navigate to first one useEffect(() => { - if (!isLoading && workspaceId && Object.keys(workflows).length > 0) { - const currentWorkflowId = extractWorkflowIdFromPathname(pathname) - - // Check if we're on the workspace root and need to redirect to first workflow - if ( - (pathname === `/workspace/${workspaceId}/w` || - pathname === `/workspace/${workspaceId}/w/`) && - Object.keys(workflows).length > 0 - ) { - const firstWorkflowId = Object.keys(workflows)[0] - logger.info('First-time navigation: redirecting to first workflow:', firstWorkflowId) - router.replace(`/workspace/${workspaceId}/w/${firstWorkflowId}`) - } + // Only proceed if hydrated and we have valid data + if (!isHydrated || !workspaceId || isLoading || Object.keys(workflows).length === 0) { + return } - }, [isLoading, workspaceId, workflows, pathname, router]) - // Handle loading states + // Check if we're on the workspace root and need to redirect to first workflow + if ( + (pathname === `/workspace/${workspaceId}/w` || pathname === `/workspace/${workspaceId}/w/`) && + Object.keys(workflows).length > 0 + ) { + const firstWorkflowId = Object.keys(workflows)[0] + logger.info('First-time navigation: redirecting to first workflow:', firstWorkflowId) + router.replace(`/workspace/${workspaceId}/w/${firstWorkflowId}`) + } + }, [isHydrated, isLoading, workspaceId, workflows, pathname, router]) + + // Handle loading states - only after hydration useEffect(() => { + // Don't manage loading state until we're hydrated + if (!isHydrated) return + // Only set loading if we don't have workflows and aren't already loading if (Object.keys(workflows).length === 0 && !isLoading) { setLoading(true) @@ -83,26 +80,6 @@ export function useRegistryLoading() { return } - // Only create timeout if we're actually loading - if (!isLoading) return - - // Create a timeout to clear loading state after max time - const timeout = setTimeout(() => { - setLoading(false) - }, 3000) // 3 second maximum loading time - - // Listen for workflows to be loaded - const checkInterval = setInterval(() => { - const currentWorkflows = useWorkflowRegistry.getState().workflows - if (Object.keys(currentWorkflows).length > 0) { - setLoading(false) - clearInterval(checkInterval) - } - }, 200) - - return () => { - clearTimeout(timeout) - clearInterval(checkInterval) - } - }, [setLoading, workflows, isLoading]) + // The fetch function itself handles setting isLoading to false + }, [isHydrated, setLoading, workflows, isLoading]) } diff --git a/apps/sim/app/workspace/[workspaceId]/w/page.tsx b/apps/sim/app/workspace/[workspaceId]/w/page.tsx index 998bd9a27b..b5e069abbc 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/page.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/page.tsx @@ -1,6 +1,6 @@ 'use client' -import { useEffect } from 'react' +import { useEffect, useState } from 'react' import { useParams, useRouter } from 'next/navigation' import { LoadingAgent } from '@/components/ui/loading-agent' import { useWorkflowRegistry } from '@/stores/workflows/registry/store' @@ -11,7 +11,20 @@ export default function WorkflowsPage() { const params = useParams() const workspaceId = params.workspaceId + // Track hydration state to prevent premature redirects + const [isHydrated, setIsHydrated] = useState(false) + + // Handle client-side hydration useEffect(() => { + setIsHydrated(true) + }, []) + + useEffect(() => { + // Don't do anything until we're hydrated and have a valid workspaceId + if (!isHydrated || !workspaceId || typeof workspaceId !== 'string') { + return + } + // Wait for workflows to load if (isLoading) return @@ -23,11 +36,11 @@ export default function WorkflowsPage() { return } - // If no workflows exist, this means the workspace creation didn't work properly - // or the user doesn't have any workspaces. Redirect to home to let the system - // handle workspace/workflow creation properly. + // If no workflows exist after loading is complete, this means the workspace creation + // didn't work properly or the user doesn't have any workspaces. + // Redirect to home to let the system handle workspace/workflow creation properly. router.replace('/') - }, [workflows, isLoading, router, workspaceId]) + }, [isHydrated, workflows, isLoading, router, workspaceId]) // Show loading state while determining where to redirect return ( diff --git a/apps/sim/components/ui/loading-agent.tsx b/apps/sim/components/ui/loading-agent.tsx index 5f87b57f17..0e42d1685d 100644 --- a/apps/sim/components/ui/loading-agent.tsx +++ b/apps/sim/components/ui/loading-agent.tsx @@ -33,11 +33,10 @@ export function LoadingAgent({ size = 'md' }: LoadingAgentProps) { strokeWidth='1.8' strokeLinecap='round' strokeLinejoin='round' - // The magic: dash array & offset style={{ strokeDasharray: pathLength, strokeDashoffset: pathLength, - animation: 'dash 1.5s linear forwards', + animation: 'dashLoop 3s linear infinite', }} /> diff --git a/apps/sim/hooks/use-collaborative-workflow.ts b/apps/sim/hooks/use-collaborative-workflow.ts index 2e0c0dfa85..d4d84eb794 100644 --- a/apps/sim/hooks/use-collaborative-workflow.ts +++ b/apps/sim/hooks/use-collaborative-workflow.ts @@ -34,6 +34,9 @@ export function useCollaborativeWorkflow() { // Track if we're applying remote changes to avoid infinite loops const isApplyingRemoteChange = useRef(false) + // Track last applied position timestamps to prevent out-of-order updates + const lastPositionTimestamps = useRef>(new Map()) + // Join workflow room when active workflow changes useEffect(() => { if (activeWorkflowId && isConnected && currentWorkflowId !== activeWorkflowId) { @@ -43,6 +46,10 @@ export function useCollaborativeWorkflow() { activeWorkflowId, presenceUsers: presenceUsers.length, }) + + // Clear position timestamps when switching workflows + lastPositionTimestamps.current.clear() + joinWorkflow(activeWorkflowId) } }, [activeWorkflowId, isConnected, currentWorkflowId, joinWorkflow]) @@ -86,15 +93,44 @@ export function useCollaborativeWorkflow() { payload.extent ) break - case 'update-position': - // Apply immediate position update with smooth interpolation for other users - workflowStore.updateBlockPosition(payload.id, payload.position) + case 'update-position': { + // Apply position update only if it's newer than the last applied timestamp + // This prevents jagged movement from out-of-order position updates + const blockId = payload.id + + // Server should always provide timestamp - if missing, skip ordering check + if (!data.timestamp) { + logger.warn('Position update missing timestamp, applying without ordering check', { + blockId, + }) + workflowStore.updateBlockPosition(payload.id, payload.position) + break + } + + const updateTimestamp = data.timestamp + const lastTimestamp = lastPositionTimestamps.current.get(blockId) || 0 + + if (updateTimestamp >= lastTimestamp) { + workflowStore.updateBlockPosition(payload.id, payload.position) + lastPositionTimestamps.current.set(blockId, updateTimestamp) + } else { + // Skip out-of-order position update to prevent jagged movement + logger.debug('Skipping out-of-order position update', { + blockId, + updateTimestamp, + lastTimestamp, + position: payload.position, + }) + } break + } case 'update-name': workflowStore.updateBlockName(payload.id, payload.name) break case 'remove': workflowStore.removeBlock(payload.id) + // Clean up position timestamp tracking for removed blocks + lastPositionTimestamps.current.delete(payload.id) break case 'toggle-enabled': workflowStore.toggleBlockEnabled(payload.id) diff --git a/apps/sim/socket-server/handlers/connection.ts b/apps/sim/socket-server/handlers/connection.ts index b66511f7d9..7c1657cb32 100644 --- a/apps/sim/socket-server/handlers/connection.ts +++ b/apps/sim/socket-server/handlers/connection.ts @@ -24,17 +24,9 @@ export function setupConnectionHandlers( const workflowId = roomManager.getWorkflowIdForSocket(socket.id) const session = roomManager.getUserSession(socket.id) - logger.info(`Socket ${socket.id} disconnected: ${reason}`) - if (workflowId && session) { roomManager.cleanupUserFromRoom(socket.id, workflowId) - - // Broadcast updated presence list to all remaining users roomManager.broadcastPresenceUpdate(workflowId) - - logger.info( - `User ${session.userId} (${session.userName}) disconnected from workflow ${workflowId} - reason: ${reason}` - ) } roomManager.clearPendingOperations(socket.id) diff --git a/apps/sim/socket-server/handlers/operations.ts b/apps/sim/socket-server/handlers/operations.ts index b56bd82766..81e798d34d 100644 --- a/apps/sim/socket-server/handlers/operations.ts +++ b/apps/sim/socket-server/handlers/operations.ts @@ -9,12 +9,6 @@ import type { HandlerDependencies } from './workflow' const logger = createLogger('OperationsHandlers') -// Simplified conflict resolution - just last-write-wins since we have normalized tables -function shouldAcceptOperation(operation: any, roomLastModified: number): boolean { - // Accept all operations - with normalized tables, conflicts are very unlikely - return true -} - export function setupOperationsHandlers( socket: AuthenticatedSocket, deps: HandlerDependencies | RoomManager @@ -46,17 +40,6 @@ export function setupOperationsHandlers( const validatedOperation = WorkflowOperationSchema.parse(data) const { operation, target, payload, timestamp } = validatedOperation - if (!shouldAcceptOperation(validatedOperation, room.lastModified)) { - socket.emit('operation-rejected', { - type: 'OPERATION_REJECTED', - message: 'Operation rejected', - operation, - target, - serverTimestamp: Date.now(), - }) - return - } - // Check operation permissions const permissionCheck = await verifyOperationPermission( session.userId, @@ -82,23 +65,71 @@ export function setupOperationsHandlers( userPresence.lastActivity = Date.now() } - // Persist to database with transaction (last-write-wins) - const serverTimestamp = Date.now() + // For position updates, preserve client timestamp to maintain ordering + // For other operations, use server timestamp for consistency + const isPositionUpdate = operation === 'update-position' && target === 'block' + const operationTimestamp = isPositionUpdate ? timestamp : Date.now() + + // Broadcast first for position updates to minimize latency, then persist + // For other operations, persist first for consistency + if (isPositionUpdate) { + // Broadcast position updates immediately for smooth real-time movement + const broadcastData = { + operation, + target, + payload, + timestamp: operationTimestamp, + senderId: socket.id, + userId: session.userId, + userName: session.userName, + metadata: { + workflowId, + operationId: crypto.randomUUID(), + isPositionUpdate: true, + }, + } + + socket.to(workflowId).emit('workflow-operation', broadcastData) + + // Persist position update asynchronously to avoid blocking real-time updates + persistWorkflowOperation(workflowId, { + operation, + target, + payload, + timestamp: operationTimestamp, + userId: session.userId, + }).catch((error) => { + logger.error('Failed to persist position update:', error) + }) + + room.lastModified = Date.now() + + socket.emit('operation-confirmed', { + operation, + target, + operationId: broadcastData.metadata.operationId, + serverTimestamp: Date.now(), + }) + + return // Early return for position updates + } + + // For non-position operations, persist first then broadcast await persistWorkflowOperation(workflowId, { operation, target, payload, - timestamp: serverTimestamp, + timestamp: operationTimestamp, userId: session.userId, }) - room.lastModified = serverTimestamp + room.lastModified = Date.now() const broadcastData = { operation, target, payload, - timestamp: serverTimestamp, + timestamp: operationTimestamp, // Preserve client timestamp for position updates senderId: socket.id, userId: session.userId, userName: session.userName, @@ -106,6 +137,7 @@ export function setupOperationsHandlers( metadata: { workflowId, operationId: crypto.randomUUID(), + isPositionUpdate, // Flag to help clients handle position updates specially }, } @@ -115,7 +147,7 @@ export function setupOperationsHandlers( operation, target, operationId: broadcastData.metadata.operationId, - serverTimestamp, + serverTimestamp: Date.now(), }) } catch (error) { if (error instanceof ZodError) { diff --git a/apps/sim/socket-server/handlers/workflow.ts b/apps/sim/socket-server/handlers/workflow.ts index 0fba11ddba..e21bbc8ad4 100644 --- a/apps/sim/socket-server/handlers/workflow.ts +++ b/apps/sim/socket-server/handlers/workflow.ts @@ -94,7 +94,7 @@ export function setupWorkflowHandlers( const workflowState = await getWorkflowState(workflowId) socket.emit('workflow-state', workflowState) - // Send complete presence list to all users in the room (including the new user) + // Broadcast updated presence list to all users in the room roomManager.broadcastPresenceUpdate(workflowId) logger.info(