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

Merged
merged 6 commits into from
Oct 2, 2024

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.

Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Looks good to me @abhi12299! But not sure why CI's red, definitely worth getting to green

@abhi12299
Copy link
Contributor Author

@Twixes working on it!

@abhi12299
Copy link
Contributor Author

abhi12299 commented Sep 20, 2024

@Twixes only the visual regression tests fail. I don't think I have access to the artifacts uploaded by the tests to see what the difference is.

Copy link
Collaborator

@Twixes Twixes left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@posthog-bot
Copy link
Contributor

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week. If you want to permanentely keep it open, use the waiting label.

@Twixes Twixes merged commit 0d01d50 into PostHog:master Oct 2, 2024
93 of 94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants