QueryClientProvider refactor #544

jpggvilaca · 2025-06-25T11:26:29Z

📝 Description

Why did this come up? I wanted to decouple notifications from a few places, which let me to update the mutation cache, which led to tests failing, which led to realising we were using different clients, which might be dangerous since then we'll be testing something different from what our main app uses. And here we are.
QueryClient now triggers notifications instead of each hook doing it. Which means the tests need the correct query client to test the existence of those notifications.

Note: I will wait for the release to merge this.

RequiredProvidersRender will now use the actual query client we use for the geti app
Extract query client creation to its own function
Add mutationCache to our query client to inherit the same behavior we already have with queryCache (to trigger a notification onError)

For the next PRs:

Leverage the query and mutation cache and get rid of all addNotification (non-custom ones) from all consumers.

✨ Type of Change

Select the type of change your PR introduces:

🐞 Bug fix – Non-breaking change which fixes an issue
🚀 New feature – Non-breaking change which adds functionality
🔨 Refactor – Non-breaking change which refactors the code base
💥 Breaking change – Changes that break existing functionality
📚 Documentation update
🔒 Security update
🧪 Tests

🧪 Testing Scenarios

Describe how the changes were tested and how reviewers can test them too:

✅ Tested manually
🤖 Run automated end-to-end tests

✅ Checklist

Before submitting the PR, ensure the following:

🔍 PR title is clear and descriptive
📝 For internal contributors: If applicable, include the JIRA ticket number (e.g., ITEP-123456) in the PR title. Do not include full URLs
💬 I have commented my code, especially in hard-to-understand areas
📄 I have made corresponding changes to the documentation
✅ I have added tests that prove my fix is effective or my feature works

jpggvilaca · 2025-06-25T11:27:16Z

web_ui/src/core/jobs/hooks/utils.test.tsx

        const { rerender } = renderHookWithProviders(
-            ({ jobs }) => useInvalidateBalanceOnNewJob(workspaceIdentifier, getMockedResponse(jobs), {}),
+            ({ jobs }) => {
+                const queryClient = useQueryClient();


we call useQueryClient inside the wrapper to get access to the client from the tree mounted by this test

jpggvilaca · 2025-06-25T11:27:35Z

web_ui/src/notification/notification.component.tsx

 };

-interface addNotificationProps {
+export interface AddNotificationProps {


no big changes here, i just capitalised this, OCD

Copilot

Pull Request Overview

This PR refactors how the React Query client is created and wired through the app’s providers, adding mutation cache support for error notifications, and updates several tests to use the new useQueryClient pattern.

Extract createGetiQueryClient into its own function and add a MutationCache that triggers error notifications.
Refactor RequiredProviders and testing utilities to use PrefilledQueryClientProvider with the new client creation API.
Update tests to use useQueryClient instead of manually instantiating QueryClient, and rename notification prop types.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
web_ui/src/test-utils/required-providers-render.tsx	Use `PrefilledQueryClientProvider` and extracted query client code
web_ui/src/routes/organizations/organizations-context.test.tsx	Replace `waitForElementToBeRemoved` with `waitFor`, simplify setup
web_ui/src/providers/query-client-provider/query-client-provider.component.tsx	Extract `createGetiQueryClient`, add `mutationCache`, refactor provider
web_ui/src/pages/media/hooks/media-delete/media-delete.hook.test.tsx	Switch to `useQueryClient` hook, remove direct `QueryClient` usage
web_ui/src/notification/notification.component.tsx	Rename and export `AddNotificationProps`
web_ui/src/core/jobs/hooks/utils.test.tsx	Update tests to call `useQueryClient` for invalidation testing

Comments suppressed due to low confidence (1)

web_ui/src/core/jobs/hooks/utils.test.tsx:39

[nitpick] The variable queryClient here holds the hook result, not the client directly. Consider renaming it (e.g., hookResult) to avoid confusion.

        const queryClient = renderHookWithProviders(useQueryClient);

web_ui/src/providers/query-client-provider/query-client-provider.component.tsx

web_ui/src/test-utils/required-providers-render.tsx

web_ui/src/pages/media/hooks/media-delete/media-delete.hook.test.tsx

jpggvilaca · 2025-06-25T11:28:40Z

web_ui/src/test-utils/required-providers-render.tsx

-                            <ThemeProvider theme={defaultTheme}>
+                <NotificationProvider>
+                    <Notifications />
+                    <PrefilledQueryClientProvider featureFlags={featureFlags} profile={profile}>


this is the main change

MarkRedeman

I'm not sure if I fully understand the intend of this PR. I'll try to come back to this later

MarkRedeman · 2025-06-25T11:39:47Z

web_ui/src/providers/query-client-provider/query-client-provider.component.tsx

+        if (customQueryClient) {
+            return customQueryClient;
+        }


This feels a bit odd to me, if a user needs a different query client then they can render the provider manually,

<TanstackQueryClientProvider client={customQueryClient}> // ... </TanstackQueryClientProvider>

instead of relying on this wrapper.

True, previous implementation quirks. Got rid of it now.

MarkRedeman · 2025-06-25T11:41:56Z

web_ui/src/test-utils/required-providers-render.tsx

+    featureFlags?: CustomFeatureFlags;
+    profile?: OnboardingProfile | null;
+}) => {
+    const { addNotification } = useNotification();


Do we need this notification behaviour in our tests? If we verify that createGetiQueryClient works as expected with a single unit test then I don't think we really need it here.

we can also just pass the ref. But what you mean here is that we dont need notification stuff on the tests, but i dont think that is true. We do need it and we test a lot of notifications. Once we remove the "addNotification(...)" from the consumers (since our QC will trigger them by default) we will still be able to test the notification presence.

jpggvilaca · 2025-06-25T11:55:03Z

web_ui/src/test-utils/required-providers-render.tsx

+                    <Notifications />
+                    <PrefilledQueryClientProvider featureFlags={featureFlags} profile={profile}>
+                        <ThemeProvider theme={defaultTheme}>
+                            <Suspense fallback={<IntelBrandedLoading />}>


Why the extra suspense you might ask? Good question:

First, it's just for the tests so no danger there, secondly:

From google/copilot/chatgpt/testing:
"Sometimes, due to React’s rendering order or test environment quirks, the outer isn’t "ready" to catch suspensions from providers/components deep in the tree.
An inner ensures that any suspension in that subtree is always caught, preventing errors and showing the fallback as intended."

jpggvilaca · 2025-06-25T12:10:07Z

web_ui/src/core/jobs/hooks/utils.test.tsx

    it('Should invalidate balance if feature flag is enabled', async () => {
-        const queryClient = new QueryClient();
-        queryClient.invalidateQueries = jest.fn();
+        let invalidateSpy: jest.SpyInstance | undefined;


Discussed offline about these. I just "refactored" the tests to make it work with the new approach, but fundamentally they are not great tests in the sense that they test if react-query works instead of testing our app behavior AFTER react-query acts. So we should check if , once we do X, then Y happens, not that react-query called its internals correctly.

web_ui/src/providers/query-client-provider/query-client-provider.component.tsx

…he actual client used in the app

dwesolow · 2025-07-11T11:48:52Z

web_ui/src/core/jobs/hooks/utils.test.tsx

+queryClient.invalidateQueries = mockSetInvalidateQueries;
+
+const wrapper = ({ children }: { children: ReactNode }) => (
+    <QueryClientProvider client={queryClient}>{children}</QueryClientProvider>


I think instead of creating second QueryClientProvider instance we could just leave the previous implementation of the RequiredProvidersProps which accepts optionally the queryClient. In case we want to mock queryClient, we create it using createGetiQueryClient and pass it to the hook/provider. For instance: useInvalidateBalanceOnNewJob(..., { providerProps: { queryClient }}).

Copilot AI review requested due to automatic review settings June 25, 2025 11:26

jpggvilaca added UI ready for review labels Jun 25, 2025

jpggvilaca requested review from ActiveChooN, MarkRedeman, camiloHimura, dwesolow, pplaskie and romanowska as code owners June 25, 2025 11:26

jpggvilaca commented Jun 25, 2025

View reviewed changes

Copilot AI reviewed Jun 25, 2025

View reviewed changes

web_ui/src/providers/query-client-provider/query-client-provider.component.tsx Show resolved Hide resolved

web_ui/src/test-utils/required-providers-render.tsx Show resolved Hide resolved

web_ui/src/pages/media/hooks/media-delete/media-delete.hook.test.tsx Outdated Show resolved Hide resolved

jpggvilaca commented Jun 25, 2025

View reviewed changes

MarkRedeman reviewed Jun 25, 2025

View reviewed changes

jpggvilaca commented Jun 25, 2025

View reviewed changes

pplaskie previously approved these changes Jun 26, 2025

View reviewed changes

dwesolow reviewed Jul 9, 2025

View reviewed changes

web_ui/src/providers/query-client-provider/query-client-provider.component.tsx Show resolved Hide resolved

jpggvilaca added 5 commits July 9, 2025 13:35

Refactor query-client-provider and required-providers render to use t…

01a3732

…he actual client used in the app

Fix unit tests

69655aa

Fix remaining test

d8f71e9

Get rid of unnecessary customQueryClient prop

e1cc979

Simplify unit tests

2631381

jpggvilaca dismissed pplaskie’s stale review via 2631381 July 9, 2025 12:01

jpggvilaca force-pushed the jvilaca/qc-provider-refactor branch from 8fab795 to 2631381 Compare July 9, 2025 12:01

Fix addNotification reference

2d67398

dwesolow reviewed Jul 11, 2025

View reviewed changes

QueryClientProvider refactor #544

Are you sure you want to change the base?

QueryClientProvider refactor #544

Uh oh!

Conversation

jpggvilaca commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📝 Description

✨ Type of Change

🧪 Testing Scenarios

✅ Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarkRedeman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jpggvilaca commented Jun 25, 2025 •

edited

Loading