Skip to content

Commit

Permalink
fix(app): fix excessive /runs network requests (#14783)
Browse files Browse the repository at this point in the history
Closes EXEC-255

UseNotifyService utilizes hostname instead of the host object, preventing a pass by reference issue causing excessive requests sent to /runs that often occurs on the RobotDetails page. Let's also ensure that if a robot loses connection while a component has passed a callback to appShellListener, we ensure we remove the correct callback from the store.
  • Loading branch information
mjhuff authored and Carlos-fernandez committed May 20, 2024
1 parent 30cb7b1 commit 61be566
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 2 deletions.
16 changes: 16 additions & 0 deletions app/src/resources/__tests__/useNotifyService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,4 +165,20 @@ describe('useNotifyService', () => {
unmount()
expect(mockAppShellListener).toHaveBeenCalled()
})

it('should still clean up the listener if the hostname changes to null after subscribing', () => {
const { unmount, rerender } = renderHook(() =>
useNotifyService({
hostOverride: MOCK_HOST_CONFIG,
topic: MOCK_TOPIC,
setRefetchUsingHTTP: mockHTTPRefetch,
options: MOCK_OPTIONS,
})
)
rerender({ hostOverride: null })
unmount()
expect(appShellListener).toHaveBeenCalledWith(
expect.objectContaining({ hostname: MOCK_HOST_CONFIG.hostname })
)
})
})
6 changes: 4 additions & 2 deletions app/src/resources/useNotifyService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export function useNotifyService<TData, TError = Error>({
const doTrackEvent = useTrackEvent()
const isFlex = useIsFlex(host?.robotName ?? '')
const hasUsedNotifyService = React.useRef(false)
const seenHostname = React.useRef<string | null>(null)
const { enabled, staleTime, forceHttpPolling } = options

const shouldUseNotifications =
Expand All @@ -62,21 +63,22 @@ export function useNotifyService<TData, TError = Error>({
})
dispatch(notifySubscribeAction(hostname, topic))
hasUsedNotifyService.current = true
seenHostname.current = hostname
} else {
setRefetchUsingHTTP('always')
}

return () => {
if (hasUsedNotifyService.current) {
appShellListener({
hostname: hostname as string,
hostname: seenHostname.current as string,
topic,
callback: onDataEvent,
isDismounting: true,
})
}
}
}, [topic, host, shouldUseNotifications])
}, [topic, hostname, shouldUseNotifications])

function onDataEvent(data: NotifyResponseData): void {
if (data === 'ECONNFAILED' || data === 'ECONNREFUSED') {
Expand Down

0 comments on commit 61be566

Please sign in to comment.