Refactor ObjectForm and ApiServerContext for improved activity handling
All checks were successful
farmcontrol/farmcontrol-ui/pipeline/head This commit looks good

- 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.
This commit is contained in:
Tom Butcher 2026-07-26 01:38:13 +01:00
parent 0db2dff9fe
commit 9956085e44
2 changed files with 201 additions and 76 deletions

View File

@ -422,7 +422,6 @@ const ObjectForm = forwardRef(
const data = await fetchObject(id, type) const data = await fetchObject(id, type)
const initialActivities = await fetchObjectActivities(id, type) const initialActivities = await fetchObjectActivities(id, type)
setActivities(initialActivities) setActivities(initialActivities)
notifyActivityState(initialActivities)
if ( if (
isEditingRef.current && isEditingRef.current &&
@ -452,7 +451,6 @@ const ObjectForm = forwardRef(
}, [ }, [
fetchObject, fetchObject,
fetchObjectActivities, fetchObjectActivities,
notifyActivityState,
id, id,
type, type,
form, form,
@ -472,19 +470,22 @@ const ObjectForm = forwardRef(
}, []) }, [])
// Update event handler // Update event handler
const updateActivityEventHandler = useCallback( const updateActivityEventHandler = useCallback((activity, activitiesList) => {
(activity, activities) => { setActivities((prev) => {
setActivities((prev) => { return Array.isArray(activitiesList)
const next = Array.isArray(activities) ? activitiesList
? activities : applyActivityUpdate(prev, activity)
: applyActivityUpdate(prev, activity) })
notifyActivityState(next) }, [])
handleEditingConflict(next)
return next useEffect(() => {
}) if (!id) {
}, return
[notifyActivityState, handleEditingConflict] }
)
notifyActivityState(activities)
handleEditingConflict(activities)
}, [activities, id, notifyActivityState, handleEditingConflict])
useEffect(() => { useEffect(() => {
if (connected == true && initialized == false && id && token != null) { if (connected == true && initialized == false && id && token != null) {
@ -493,6 +494,22 @@ const ObjectForm = forwardRef(
} }
}, [id, initialized, handleFetchObject, token, connected]) }, [id, initialized, handleFetchObject, token, connected])
useEffect(() => {
if (!id) {
return
}
const activityUnsubscribe = subscribeToObjectActivity(
id,
type,
updateActivityEventHandler
)
return () => {
if (activityUnsubscribe) activityUnsubscribe()
}
}, [id, type, subscribeToObjectActivity, updateActivityEventHandler])
useEffect(() => { useEffect(() => {
if (id && connected == true) { if (id && connected == true) {
currentSetObjectActivityRef.current( currentSetObjectActivityRef.current(
@ -506,16 +523,9 @@ const ObjectForm = forwardRef(
type, type,
updateObjectEventHandler updateObjectEventHandler
) )
const activityUnsubscribe = subscribeToObjectActivity(
id,
type,
updateActivityEventHandler
)
return () => { return () => {
if (objectUpdatesUnsubscribe) objectUpdatesUnsubscribe() if (objectUpdatesUnsubscribe) objectUpdatesUnsubscribe()
if (activityUnsubscribe) activityUnsubscribe()
currentClearObjectActivityRef.current(id, type)
} }
} }
}, [ }, [
@ -523,9 +533,7 @@ const ObjectForm = forwardRef(
type, type,
connected, connected,
subscribeToObjectUpdates, subscribeToObjectUpdates,
subscribeToObjectActivity, updateObjectEventHandler
updateObjectEventHandler,
updateActivityEventHandler
]) ])
// Debounce objectData updates sent to parent to limit re-renders // Debounce objectData updates sent to parent to limit re-renders
@ -540,7 +548,6 @@ const ObjectForm = forwardRef(
try { try {
const latestActivities = await fetchObjectActivities(id, type) const latestActivities = await fetchObjectActivities(id, type)
setActivities(latestActivities) setActivities(latestActivities)
notifyActivityState(latestActivities)
if (getBeingEditedByOther(latestActivities, userProfile?._id)) { if (getBeingEditedByOther(latestActivities, userProfile?._id)) {
clearAction() clearAction()

View File

@ -105,6 +105,7 @@ const ApiServerProvider = ({ children }) => {
const [userSettingsLoaded, setUserSettingsLoaded] = useState(false) const [userSettingsLoaded, setUserSettingsLoaded] = useState(false)
const subscribedCallbacksRef = useRef(new Map()) const subscribedCallbacksRef = useRef(new Map())
const subscribedActivityCallbacksRef = useRef(new Map()) const subscribedActivityCallbacksRef = useRef(new Map())
const subscribedActivityServerSubscriptionsRef = useRef(new Set())
const notificationListenersRef = useRef(new Set()) const notificationListenersRef = useRef(new Set())
const completedLaunchSessionsRef = useRef(new Set()) const completedLaunchSessionsRef = useRef(new Set())
@ -119,12 +120,11 @@ const ApiServerProvider = ({ children }) => {
if ( if (
objectId && objectId &&
objectType && objectType &&
activity && (activity || Array.isArray(activities)) &&
subscribedActivityCallbacksRef.current.has(callbacksRefKey) subscribedActivityCallbacksRef.current.has(callbacksRefKey)
) { ) {
const callbacks = subscribedActivityCallbacksRef.current.get( const callbacks =
callbacksRefKey subscribedActivityCallbacksRef.current.get(callbacksRefKey)
)
logger.debug( logger.debug(
`Calling ${callbacks.length} activity callbacks for object:`, `Calling ${callbacks.length} activity callbacks for object:`,
objectId objectId
@ -141,10 +141,53 @@ const ApiServerProvider = ({ children }) => {
const clearSubscriptions = useCallback(() => { const clearSubscriptions = useCallback(() => {
subscribedCallbacksRef.current.clear() subscribedCallbacksRef.current.clear()
subscribedActivityCallbacksRef.current.clear() subscribedActivityServerSubscriptionsRef.current.clear()
notificationListenersRef.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) => { const registerNotificationListener = useCallback((callback) => {
notificationListenersRef.current.add(callback) notificationListenersRef.current.add(callback)
return () => notificationListenersRef.current.delete(callback) return () => notificationListenersRef.current.delete(callback)
@ -247,6 +290,7 @@ const ApiServerProvider = ({ children }) => {
setConnecting(false) setConnecting(false)
setConnected(true) setConnected(true)
setError(null) setError(null)
resubscribeActivityListeners(newSocket)
}) })
}) })
@ -307,7 +351,8 @@ const ApiServerProvider = ({ children }) => {
messageApi, messageApi,
handleActivityUpdate, handleActivityUpdate,
clearSubscriptions, clearSubscriptions,
getUserSettings getUserSettings,
resubscribeActivityListeners
]) ])
useEffect(() => { useEffect(() => {
@ -331,6 +376,54 @@ const ApiServerProvider = ({ children }) => {
} }
}, [token, authenticated, connectToServer]) }, [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) => { const setObjectActivity = (id, type, mode) => {
logger.debug('Setting activity for', id, mode) logger.debug('Setting activity for', id, mode)
if (socketRef.current && socketRef.current.connected) { if (socketRef.current && socketRef.current.connected) {
@ -682,25 +775,31 @@ const ApiServerProvider = ({ children }) => {
) )
const offActivityEvent = useCallback((id, objectType, callback) => { const offActivityEvent = useCallback((id, objectType, callback) => {
if (socketRef.current && socketRef.current.connected == true) { const callbacksRefKey = `${objectType}:${id}`
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)
}
}
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( const offObjectEventEvent = useCallback(
@ -846,36 +945,47 @@ const ApiServerProvider = ({ children }) => {
const subscribeToObjectActivity = useCallback( const subscribeToObjectActivity = useCallback(
(id, type, callback) => { (id, type, callback) => {
logger.debug('Subscribing to activity for object:', id, 'type:', type) logger.debug('Subscribing to activity for object:', id, 'type:', type)
if (socketRef.current && socketRef.current.connected == true) { if (!id || !type) {
const callbacksRefKey = `${type}:${id}` return undefined
if (!subscribedActivityCallbacksRef.current.has(callbacksRefKey)) { }
subscribedActivityCallbacksRef.current.set(callbacksRefKey, [])
}
const callbacksLength =
subscribedActivityCallbacksRef.current.get(callbacksRefKey).length
subscribedActivityCallbacksRef.current const callbacksRefKey = `${type}:${id}`
.get(callbacksRefKey) if (!subscribedActivityCallbacksRef.current.has(callbacksRefKey)) {
.push(callback) subscribedActivityCallbacksRef.current.set(callbacksRefKey, [])
}
if (callbacksLength <= 0) { subscribedActivityCallbacksRef.current.get(callbacksRefKey).push(callback)
socketRef.current.emit(
'subscribeToObjectActivity', const socket = socketRef.current
{ _id: id, objectType: type }, if (
(result) => { socket?.connected &&
if (result?.success && Array.isArray(result.activities)) { !subscribedActivityServerSubscriptionsRef.current.has(callbacksRefKey)
result.activities.forEach((activity) => { ) {
callback(activity) 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] [offActivityEvent]
) )
@ -1426,7 +1536,11 @@ const ApiServerProvider = ({ children }) => {
} }
const getModelPropertyValues = async (objectType, property) => { 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 { try {
const response = await axios.get( const response = await axios.get(
`${config.backendUrl}/${getObjectEndpoint(objectType)}/values`, `${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 : [] return Array.isArray(response.data) ? response.data : []
} catch (err) { } catch (err) {
console.error(err) console.error(err)