Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only reset errors if there are errors #4282

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Only reset errors if there are errors #4282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions ...es/app/src/cli/services/app-logs/logs-command/ui/components/hooks/usePollAppLogs.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,14 +374,14 @@ describe('usePollAppLogs', () => {
// needed to await the render
await vi.advanceTimersByTimeAsync(0)

// Initial invocation, 429 returned
// Initial invocation, 422 returned
expect(mockedPollAppLogs).toHaveBeenCalledTimes(1)

expect(hook.lastResult?.appLogOutputs).toHaveLength(0)
expect(hook.lastResult?.errors[0]).toEqual('Error while polling app logs')
expect(hook.lastResult?.errors[1]).toEqual('Retrying in 5s')

expect(setTimeout).toHaveBeenCalledWith(expect.any(Function), POLLING_ERROR_RETRY_INTERVAL_MS)
expect(timeoutSpy).toHaveBeenCalledWith(expect.any(Function), POLLING_ERROR_RETRY_INTERVAL_MS)

await vi.advanceTimersToNextTimerAsync()
expect(hook.lastResult?.appLogOutputs).toHaveLength(6)
Expand All @@ -391,6 +391,30 @@ describe('usePollAppLogs', () => {
expect(vi.getTimerCount()).toEqual(1)
timeoutSpy.mockRestore()
})

test('clears error on success', async () => {
const mockedPollAppLogs = vi
.fn()
.mockResolvedValueOnce(POLL_APP_LOGS_FOR_LOGS_429_RESPONSE)
.mockResolvedValueOnce(POLL_APP_LOGS_FOR_LOGS_RESPONSE)
vi.mocked(pollAppLogs).mockImplementation(mockedPollAppLogs)

const hook = renderHook(() =>
usePollAppLogs({
initialJwt: MOCKED_JWT_TOKEN,
filters: EMPTY_FILTERS,
resubscribeCallback: vi.fn().mockResolvedValue(MOCKED_JWT_TOKEN),
}),
)

// initial poll with errors
await vi.advanceTimersByTimeAsync(0)
expect(hook.lastResult?.errors).toHaveLength(2)

// second poll with no errors
await vi.advanceTimersToNextTimerAsync()
expect(hook.lastResult?.errors).toHaveLength(0)
})
})

function renderHook<THookResult>(renderHookCallback: () => THookResult) {
Expand Down
2 changes: 1 addition & 1 deletion packages/app/src/cli/services/app-logs/logs-command/ui/components/hooks/usePollAppLogs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export function usePollAppLogs({initialJwt, filters, resubscribeCallback}: UsePo
}
retryIntervalMs = result.retryIntervalMs
} else {
setErrors([])
setErrors((errors) => (errors.length ? [] : errors))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really get how this fixed the issue?
Before, we were always setting the state to [] which caused the refresh.
Now, we're either setting the state to [] or errors, either of which should cause the refresh again?

I did some searching of the useState docs, but it's not clear to me if providing a "updater" function also gets us a behaviour of "if the state hasn't changed, then don't update it".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React re-renders if a state changes, and the way it determines if the state changed is by reference. When we setState([]), we're setting the state with a new array. Although the contents are the same, the reference is not, so React re-renders, causing the flickering.

To avoid this, we want to reset the errors only if there are already errors. Ideally, we'd do something like this:

if (errors.length) {
  setErrors([])
}

But the problem with that is that errors inside this function actually refers to the initial errors array, not the current value! So we can't simply reset the errors based on the current errors value.

Luckily, React exposes the current value through the setter updater function. I use that current value to decide what we should do. If there are errors, then we set the state to [], triggering a re-render. If there aren't errors, we set the state to the exact same reference, which does not trigger a re-render.

}

const {cursor: nextCursor, appLogs} = response as SuccessResponse
Expand Down
Loading