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(dashboard): Fix stale data on test workflow page #7245

Conversation

desiprisg
Copy link
Contributor

What changed? Why was the change needed?

Screenshots

Expand for optional sections

Related enterprise PR

Special notes for your reviewer

Copy link

linear bot commented Dec 9, 2024

Copy link

netlify bot commented Dec 9, 2024

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit 143fb81
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/6757f71d95bc0f00080899fd
😎 Deploy Preview https://deploy-preview-7245.dashboard.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 9, 2024

Deploy Preview for dashboard-v2-novu-staging ready!

Name Link
🔨 Latest commit 143fb81
🔍 Latest deploy log https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/6757f71d8795560008d3e34c
😎 Deploy Preview https://deploy-preview-7245.dashboard-v2.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@desiprisg desiprisg force-pushed the nv-4847-step-editor-refresh-need-to-get-the-new-payload-variables branch from d437c11 to 5b19e17 Compare December 9, 2024 10:41
Copy link
Contributor

@SokratisVidros SokratisVidros left a comment

Choose a reason for hiding this comment

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

I approved it but please see my comment about caching.

queryKey: [
QueryKeys.fetchWorkflowTestData,
currentEnvironment?._id,
getEncodedId({ slug: workflowSlug, divider: WORKFLOW_DIVIDER }),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit-picky one:

Suggested change
getEncodedId({ slug: workflowSlug, divider: WORKFLOW_DIVIDER }),
getWorkflowInternalId(workflowSlug),

const { currentEnvironment } = useEnvironment();
const { data, isPending, error } = useQuery<WorkflowTestDataResponseDto>({
queryKey: [QueryKeys.fetchWorkflowTestData, currentEnvironment?._id, workflowSlug],
queryKey: [
Copy link
Contributor

Choose a reason for hiding this comment

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

@desiprisg I think the test data page is better if we don't cache it at all. It's a separate tab, so I don't see the value of caching. We'd rather fetch data every time we visit the tab instead of caching and invalidating.

Copy link
Contributor Author

@desiprisg desiprisg Dec 10, 2024

Choose a reason for hiding this comment

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

Sure we can set gcTime to 0 to achieve this. queryKey remains required though, in order to trigger a refetch if an invalidation were to happen while the query is still used.

@desiprisg desiprisg force-pushed the nv-4847-step-editor-refresh-need-to-get-the-new-payload-variables branch from 5b19e17 to 731e1a0 Compare December 10, 2024 08:07
@desiprisg desiprisg force-pushed the nv-4847-step-editor-refresh-need-to-get-the-new-payload-variables branch from 731e1a0 to d153eac Compare December 10, 2024 08:07
@desiprisg desiprisg force-pushed the nv-4847-step-editor-refresh-need-to-get-the-new-payload-variables branch from d153eac to 143fb81 Compare December 10, 2024 08:09
@desiprisg desiprisg merged commit 1bd05bb into next Dec 10, 2024
34 checks passed
@desiprisg desiprisg deleted the nv-4847-step-editor-refresh-need-to-get-the-new-payload-variables branch December 10, 2024 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants