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 Jun 3, 2024
1 parent fd18a48 commit c859591
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 0 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 @@ -228,4 +228,20 @@ describe('useNotifyService', () => {
expect.objectContaining({ hostname: MOCK_HOST_CONFIG.hostname })
)
})

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 })
)
})
})
10 changes: 10 additions & 0 deletions app/src/resources/useNotifyService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ export function useNotifyService<TData, TError = Error>({
const host = hostOverride ?? hostFromProvider
const hostname = host?.hostname ?? null
const doTrackEvent = useTrackEvent()
<<<<<<< HEAD
=======
const isFlex = useIsFlex(host?.robotName ?? '')
const hasUsedNotifyService = React.useRef(false)
>>>>>>> 61be566d31 (fix(app): fix excessive /runs network requests (#14783))
const seenHostname = React.useRef<string | null>(null)
const { enabled, staleTime, forceHttpPolling } = options

Expand Down Expand Up @@ -69,6 +74,7 @@ export function useNotifyService<TData, TError = Error>({
if (seenHostname.current != null) {
=======
hasUsedNotifyService.current = true
seenHostname.current = hostname
} else {
setRefetchUsingHTTP('always')
}
Expand All @@ -77,7 +83,11 @@ export function useNotifyService<TData, TError = Error>({
if (hasUsedNotifyService.current) {
>>>>>>> 1ba616651c (refactor(app-shell-odd): Utilize robot-server unsubscribe flags (#14724))
appShellListener({
<<<<<<< HEAD
hostname: seenHostname.current,
=======
hostname: seenHostname.current as string,
>>>>>>> 61be566d31 (fix(app): fix excessive /runs network requests (#14783))
topic,
callback: onDataEvent,
isDismounting: true,
Expand Down

0 comments on commit c859591

Please sign in to comment.