From 9956085e4433ffe0412310ef973675f4d863b4d2 Mon Sep 17 00:00:00 2001 From: Tom Butcher Date: Sun, 26 Jul 2026 01:38:13 +0100 Subject: [PATCH] Refactor ObjectForm and ApiServerContext for improved activity handling - Removed redundant calls to notifyActivityState in ObjectForm, consolidating activity state updates within useEffect hooks for better performance. - Introduced resubscribeActivityListeners in ApiServerContext to manage activity subscriptions more effectively upon socket connection and page visibility changes. - Enhanced activity event subscription management by utilizing a Set to track active subscriptions, improving clarity and reducing potential memory leaks. --- .../Dashboard/common/ObjectForm.jsx | 59 ++--- .../Dashboard/context/ApiServerContext.jsx | 218 ++++++++++++++---- 2 files changed, 201 insertions(+), 76 deletions(-) diff --git a/src/components/Dashboard/common/ObjectForm.jsx b/src/components/Dashboard/common/ObjectForm.jsx index 940aad1..3948e3b 100644 --- a/src/components/Dashboard/common/ObjectForm.jsx +++ b/src/components/Dashboard/common/ObjectForm.jsx @@ -422,7 +422,6 @@ const ObjectForm = forwardRef( const data = await fetchObject(id, type) const initialActivities = await fetchObjectActivities(id, type) setActivities(initialActivities) - notifyActivityState(initialActivities) if ( isEditingRef.current && @@ -452,7 +451,6 @@ const ObjectForm = forwardRef( }, [ fetchObject, fetchObjectActivities, - notifyActivityState, id, type, form, @@ -472,19 +470,22 @@ const ObjectForm = forwardRef( }, []) // Update event handler - const updateActivityEventHandler = useCallback( - (activity, activities) => { - setActivities((prev) => { - const next = Array.isArray(activities) - ? activities - : applyActivityUpdate(prev, activity) - notifyActivityState(next) - handleEditingConflict(next) - return next - }) - }, - [notifyActivityState, handleEditingConflict] - ) + const updateActivityEventHandler = useCallback((activity, activitiesList) => { + setActivities((prev) => { + return Array.isArray(activitiesList) + ? activitiesList + : applyActivityUpdate(prev, activity) + }) + }, []) + + useEffect(() => { + if (!id) { + return + } + + notifyActivityState(activities) + handleEditingConflict(activities) + }, [activities, id, notifyActivityState, handleEditingConflict]) useEffect(() => { if (connected == true && initialized == false && id && token != null) { @@ -493,6 +494,22 @@ const ObjectForm = forwardRef( } }, [id, initialized, handleFetchObject, token, connected]) + useEffect(() => { + if (!id) { + return + } + + const activityUnsubscribe = subscribeToObjectActivity( + id, + type, + updateActivityEventHandler + ) + + return () => { + if (activityUnsubscribe) activityUnsubscribe() + } + }, [id, type, subscribeToObjectActivity, updateActivityEventHandler]) + useEffect(() => { if (id && connected == true) { currentSetObjectActivityRef.current( @@ -506,16 +523,9 @@ const ObjectForm = forwardRef( type, updateObjectEventHandler ) - const activityUnsubscribe = subscribeToObjectActivity( - id, - type, - updateActivityEventHandler - ) return () => { if (objectUpdatesUnsubscribe) objectUpdatesUnsubscribe() - if (activityUnsubscribe) activityUnsubscribe() - currentClearObjectActivityRef.current(id, type) } } }, [ @@ -523,9 +533,7 @@ const ObjectForm = forwardRef( type, connected, subscribeToObjectUpdates, - subscribeToObjectActivity, - updateObjectEventHandler, - updateActivityEventHandler + updateObjectEventHandler ]) // Debounce objectData updates sent to parent to limit re-renders @@ -540,7 +548,6 @@ const ObjectForm = forwardRef( try { const latestActivities = await fetchObjectActivities(id, type) setActivities(latestActivities) - notifyActivityState(latestActivities) if (getBeingEditedByOther(latestActivities, userProfile?._id)) { clearAction() diff --git a/src/components/Dashboard/context/ApiServerContext.jsx b/src/components/Dashboard/context/ApiServerContext.jsx index 793d171..bcdfdf7 100644 --- a/src/components/Dashboard/context/ApiServerContext.jsx +++ b/src/components/Dashboard/context/ApiServerContext.jsx @@ -105,6 +105,7 @@ const ApiServerProvider = ({ children }) => { const [userSettingsLoaded, setUserSettingsLoaded] = useState(false) const subscribedCallbacksRef = useRef(new Map()) const subscribedActivityCallbacksRef = useRef(new Map()) + const subscribedActivityServerSubscriptionsRef = useRef(new Set()) const notificationListenersRef = useRef(new Set()) const completedLaunchSessionsRef = useRef(new Set()) @@ -119,12 +120,11 @@ const ApiServerProvider = ({ children }) => { if ( objectId && objectType && - activity && + (activity || Array.isArray(activities)) && subscribedActivityCallbacksRef.current.has(callbacksRefKey) ) { - const callbacks = subscribedActivityCallbacksRef.current.get( - callbacksRefKey - ) + const callbacks = + subscribedActivityCallbacksRef.current.get(callbacksRefKey) logger.debug( `Calling ${callbacks.length} activity callbacks for object:`, objectId @@ -141,10 +141,53 @@ const ApiServerProvider = ({ children }) => { const clearSubscriptions = useCallback(() => { subscribedCallbacksRef.current.clear() - subscribedActivityCallbacksRef.current.clear() + subscribedActivityServerSubscriptionsRef.current.clear() notificationListenersRef.current.clear() }, []) + const resubscribeActivityListeners = useCallback((socket = socketRef.current) => { + if (!socket?.connected) { + return + } + + subscribedActivityCallbacksRef.current.forEach((callbacks, callbacksRefKey) => { + if (!callbacks?.length) { + return + } + + if (subscribedActivityServerSubscriptionsRef.current.has(callbacksRefKey)) { + return + } + + const colonIndex = callbacksRefKey.indexOf(':') + if (colonIndex === -1) { + return + } + + const objectType = callbacksRefKey.slice(0, colonIndex) + const objectId = callbacksRefKey.slice(colonIndex + 1) + + socket.emit( + 'subscribeToObjectActivity', + { _id: objectId, objectType }, + (result) => { + if (result?.success) { + subscribedActivityServerSubscriptionsRef.current.add(callbacksRefKey) + if (Array.isArray(result.activities)) { + callbacks.forEach((callback) => { + try { + callback(null, result.activities) + } catch (error) { + logger.error('Error in activity resubscribe callback:', error) + } + }) + } + } + } + ) + }) + }, []) + const registerNotificationListener = useCallback((callback) => { notificationListenersRef.current.add(callback) return () => notificationListenersRef.current.delete(callback) @@ -247,6 +290,7 @@ const ApiServerProvider = ({ children }) => { setConnecting(false) setConnected(true) setError(null) + resubscribeActivityListeners(newSocket) }) }) @@ -307,7 +351,8 @@ const ApiServerProvider = ({ children }) => { messageApi, handleActivityUpdate, clearSubscriptions, - getUserSettings + getUserSettings, + resubscribeActivityListeners ]) useEffect(() => { @@ -331,6 +376,54 @@ const ApiServerProvider = ({ children }) => { } }, [token, authenticated, connectToServer]) + useEffect(() => { + const handlePageVisible = () => { + if (document.visibilityState !== 'visible') { + return + } + + const socket = socketRef.current + if (!socket || !token || authenticated !== true) { + return + } + + if (!socket.connected) { + logger.debug('Page visible with disconnected socket, reconnecting...') + socket.connect() + return + } + + if (subscribedActivityCallbacksRef.current.size > 0) { + logger.debug('Page visible, resubscribing activity listeners...') + subscribedActivityServerSubscriptionsRef.current.clear() + resubscribeActivityListeners(socket) + } + } + + const handlePageShow = (event) => { + if (!event.persisted) { + return + } + + logger.debug('Page restored from bfcache, reconnecting socket...') + subscribedActivityServerSubscriptionsRef.current.clear() + setConnected(false) + if (socketRef.current) { + socketRef.current.disconnect() + socketRef.current = null + } + connectToServer() + } + + document.addEventListener('visibilitychange', handlePageVisible) + window.addEventListener('pageshow', handlePageShow) + + return () => { + document.removeEventListener('visibilitychange', handlePageVisible) + window.removeEventListener('pageshow', handlePageShow) + } + }, [token, authenticated, connectToServer, resubscribeActivityListeners]) + const setObjectActivity = (id, type, mode) => { logger.debug('Setting activity for', id, mode) if (socketRef.current && socketRef.current.connected) { @@ -682,25 +775,31 @@ const ApiServerProvider = ({ children }) => { ) const offActivityEvent = useCallback((id, objectType, callback) => { - if (socketRef.current && socketRef.current.connected == true) { - const callbacksRefKey = `${objectType}:${id}` - if (subscribedActivityCallbacksRef.current.has(callbacksRefKey)) { - const callbacks = subscribedActivityCallbacksRef.current - .get(callbacksRefKey) - .filter((cb) => cb !== callback) - if (callbacks.length === 0) { - subscribedActivityCallbacksRef.current.delete(callbacksRefKey) - socketRef.current.emit('unsubscribeObjectActivity', { - _id: id, - objectType - }) - } else { - subscribedActivityCallbacksRef.current.set(callbacksRefKey, callbacks) - } - } + const callbacksRefKey = `${objectType}:${id}` - logger.debug('Removed activity event listener for object:', id) + if (!subscribedActivityCallbacksRef.current.has(callbacksRefKey)) { + return } + + const callbacks = subscribedActivityCallbacksRef.current + .get(callbacksRefKey) + .filter((cb) => cb !== callback) + + if (callbacks.length === 0) { + subscribedActivityCallbacksRef.current.delete(callbacksRefKey) + subscribedActivityServerSubscriptionsRef.current.delete(callbacksRefKey) + + if (socketRef.current?.connected) { + socketRef.current.emit('unsubscribeObjectActivity', { + _id: id, + objectType + }) + } + } else { + subscribedActivityCallbacksRef.current.set(callbacksRefKey, callbacks) + } + + logger.debug('Removed activity event listener for object:', id) }, []) const offObjectEventEvent = useCallback( @@ -846,36 +945,47 @@ const ApiServerProvider = ({ children }) => { const subscribeToObjectActivity = useCallback( (id, type, callback) => { logger.debug('Subscribing to activity for object:', id, 'type:', type) - if (socketRef.current && socketRef.current.connected == true) { - const callbacksRefKey = `${type}:${id}` - if (!subscribedActivityCallbacksRef.current.has(callbacksRefKey)) { - subscribedActivityCallbacksRef.current.set(callbacksRefKey, []) - } - const callbacksLength = - subscribedActivityCallbacksRef.current.get(callbacksRefKey).length + if (!id || !type) { + return undefined + } - subscribedActivityCallbacksRef.current - .get(callbacksRefKey) - .push(callback) + const callbacksRefKey = `${type}:${id}` + if (!subscribedActivityCallbacksRef.current.has(callbacksRefKey)) { + subscribedActivityCallbacksRef.current.set(callbacksRefKey, []) + } - if (callbacksLength <= 0) { - socketRef.current.emit( - 'subscribeToObjectActivity', - { _id: id, objectType: type }, - (result) => { - if (result?.success && Array.isArray(result.activities)) { - result.activities.forEach((activity) => { - callback(activity) - }) + subscribedActivityCallbacksRef.current.get(callbacksRefKey).push(callback) + + const socket = socketRef.current + if ( + socket?.connected && + !subscribedActivityServerSubscriptionsRef.current.has(callbacksRefKey) + ) { + socket.emit( + 'subscribeToObjectActivity', + { _id: id, objectType: type }, + (result) => { + if (result?.success) { + subscribedActivityServerSubscriptionsRef.current.add(callbacksRefKey) + if (Array.isArray(result.activities)) { + subscribedActivityCallbacksRef.current + .get(callbacksRefKey) + ?.forEach((activityCallback) => { + try { + activityCallback(null, result.activities) + } catch (error) { + logger.error('Error in activity subscribe callback:', error) + } + }) } } - ) - } - - logger.debug('Registered activity event listener for object:', id) - - return () => offActivityEvent(id, type, callback) + } + ) } + + logger.debug('Registered activity event listener for object:', id) + + return () => offActivityEvent(id, type, callback) }, [offActivityEvent] ) @@ -1426,7 +1536,11 @@ const ApiServerProvider = ({ children }) => { } const getModelPropertyValues = async (objectType, property) => { - logger.debug('Fetching property values for model type:', objectType, property) + logger.debug( + 'Fetching property values for model type:', + objectType, + property + ) try { const response = await axios.get( `${config.backendUrl}/${getObjectEndpoint(objectType)}/values`, @@ -1438,7 +1552,11 @@ const ApiServerProvider = ({ children }) => { } } ) - logger.debug('Fetched property values for model type:', objectType, property) + logger.debug( + 'Fetched property values for model type:', + objectType, + property + ) return Array.isArray(response.data) ? response.data : [] } catch (err) { console.error(err)