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.

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

fix(events): auto load new events checkbox issue #25039

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

abhi12299
Copy link
Contributor

@abhi12299 abhi12299 commented Sep 18, 2024

Problem

  • When the Automatically load new events checkbox is unchecked and the user cancels the ongoing query, the error message displayed is "The query was cancelled" - which is correct.
    However, after this, when the checkbox is checked, the error is about AbortSignal and it does not trigger a refetch immediately.

  • When you cancel the ongoing query when the Automatically load new events checkbox is checked, you will see the AbortSignal error.

Here's the bug:

bug-autoload.mov

Changes

  • loadNewData prioritises new response if the existing response is null.

    loadNewData: async () => {
    if (props.cachedResults) {
    return props.cachedResults
    }
    if (!values.canLoadNewData || values.dataLoading) {
    return values.response
    }
    if (isEventsQuery(props.query) && values.newQuery) {
    const now = performance.now()
    const newResponse =
    (await performQuery(
    addModifiers(values.newQuery, props.modifiers),
    undefined,
    props.alwaysRefresh
    )) ?? null
    actions.setElapsedTime(performance.now() - now)
    if (newResponse?.results) {
    actions.highlightRows(newResponse?.results)
    }
    const currentResults = ((values.response || { results: [] }) as EventsQueryResponse).results
    return {
    ...values.response,
    results: [...(newResponse?.results ?? []), ...currentResults],
    }
    }
    return values.response

    Lines 258-262 here show that the object returned from the loader will only have results key if values.response is null

  • responseError reducer also reacts to loadNewData... actions; this helps to get rid of the error state when new data is loaded.

  • When you cancel the ongoing query when the Automatically load new events checkbox is checked, you will not see any error state in the table - this is because the auto load functionality will trigger a refetch in 30s anyways, so it is not a good UX to show an error state only for it to be replaced with the table after 30s.

Here's the fix:

fix.mov

@Twixes Twixes requested a review from a team September 18, 2024 09:43
@abhi12299
Copy link
Contributor Author

Will fix the cypress tests after discussing and finalising on the desired behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant