perf(ui): deduplicate docStore persona requests via shared React Query key - #31300
perf(ui): deduplicate docStore persona requests via shared React Query key#31300Rohit0301 wants to merge 20 commits into
Conversation
Three identical GET /api/v1/docStore/name/persona.* requests fired on every /my-data navigation because MyDataPage and multiple useCustomPages consumers each fetched independently with no cache coordination. Introduce docStoreQuery.ts (shared queryKey + queryFn), migrate useCustomPages to useQuery, and rewrite MyDataPage's manual fetch/effect to useQuery with the same key. React Query's in-flight deduplication collapses N concurrent subscribers to one network request. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
Rohit0301
added
the
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
…lper Both useCustomPages and MyDataPage were independently building the persona docStore FQN string. Extract to personaDocFqn() in docStoreQuery.ts so the cache key derivation has a single definition and consumers can't silently drift apart. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rohit0301
left a comment
Rohit0301
left a comment
There was a problem hiding this comment.
Good call — addressed in dcb5759. Extracted the FQN construction to personaDocFqn() in docStoreQuery.ts so both useCustomPages and MyDataPage derive the cache key from a single definition. Both consumers now import and call personaDocFqn(selectedPersona) and the inline template literals are gone.
✅ Playwright Results — workflow succeededValidated commit ✅ 918 passed · ❌ 0 failed · 🟡 3 flaky · ⏭️ 1 skipped · 🧰 0 lifecycle flaky PerformanceBlocking targets: ✅ met · Optimization targets: 🟡 in progress Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting. 🕒 Full workflow signal wall (to summary) 53m 32s ⏱️ Max setup 3m 6s · max shard execution 18m 32s · max shard-job elapsed before upload 22m 1s · reporting 6s 🌐 209.07 requests/attempt · 2.49 app boots/UI scenario · 15.29% common-shard skew Optimization targets still in progress:
🟡 3 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
useCustomPages now uses useQuery internally, so components that call it require a QueryClientProvider ancestor. Fix each test with the appropriate strategy: - GlossaryV1, GlossaryDetails, LeftSidebar: add jest.mock for useCustomPages (same pattern used by 20+ other component tests) - MyDataPage: add QueryClientProvider wrapper via a renderMyDataPage() helper + fresh QueryClient per test to avoid cache pollution Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
🚦 Removed from the merge queue —
|
…right timeout Without this, a user with no persona gets isLoading=false on the very first render (React Query computes it synchronously), widgets mount immediately, and their loaders appear before waitForAllLoadersToDisappear starts polling in the data-contract Playwright test — causing a 30 s timeout. Restores the pre-React-Query invariant: useState(true) ensures the skeleton always renders on the first paint; a useEffect syncs isLoading to the actual query state after that, so widgets are only deferred by one effect flush rather than a full network round-trip. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The previous fix used useState+useEffect to mirror isQueryLoading into an isLoading state variable, which is the classic derived-state anti-pattern: extra render on every query transition and a stale window between renders. Replace with a one-shot hasMounted flag that flips true after the first paint. isLoading is now fully derived: !hasMounted forces skeleton on first render; after mount it equals the actual query loading expression with no lag. Addresses gitar-bot review comment on PR #31300. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…laywright timeout The "Data Contracts With Persona" Playwright tests all fail at waitForAllLoadersToDisappear after visitEntityPage. Every failing entity detail component (Topic, Dashboard, MlModel, Pipeline, StoredProcedure, SearchIndex, Container, APICollection, etc.) gates its loader on isLoading from useCustomPages: if (isLoading || permissionsLoading || ...) return <PageLoader />; Before this PR, isLoading = useState(true) always started true and quickly resolved. After the React Query migration, isLoading = !!fqn && isPending starts false when selectedPersona is not yet in the Zustand store, then jumps to true when the persona arrives asynchronously — after waitForAllLoadersToDisappear may have already returned count=0, leaving the test interacting with a page covered by PageLoader. Apply the same hasMounted pattern already used in MyDataPage: isLoading is true on the first render regardless of persona state, then derives from the query after the mount effect fires. This restores the well-defined always-loading-on-first-render invariant that Playwright tests relied on. Also update the no-persona unit test to await isLoading=false (hasMounted now makes the initial value true for one effect tick). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Drop `!!fqn && isPending` from `useCustomPages.isLoading`. Root cause of remaining test failures: the old `fetchDocument()` inside `useCustomPages` never called `setIsLoading(true)` — it only called `setIsLoading(false)` in the finally block. So when `selectedPersona` arrived asynchronously after the initial render, `isLoading` stayed false while the background API fetch ran. `waitForAllLoadersToDisappear` returned cleanly and the entity page stayed visible throughout. The previous hasMounted fix used `isLoading = !hasMounted || (!!fqn && isPending)`. When `selectedPersona` arrives asynchronously (`fqn` transitions null→non-null after mount), `isPending=true` makes `isLoading` jump back to true — a second loader wave AFTER `waitForAllLoadersToDisappear` returned, covering the entity page and blocking all subsequent test interactions. Fix: `isLoading = !hasMounted` only. The persona doc still fetches in the background via React Query; `customizedPage`/`navigation` update when data arrives, entity page re-renders without a loader — exactly matching old behaviour. Also drop the two sync `expect(isLoading).toBe(true)` assertions in the unit tests: RTL flushes the mount effect synchronously, so `hasMounted` is already `true` by the time the first assertion runs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
isPending was left dangling after !!fqn&&isPending was removed from the isLoading expression — dead binding that would fail no-unused-vars lint. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Code Review ✅ Approved 5 resolved / 5 findingsDeduplicates docStore persona requests on navigation by introducing a shared React Query cache layer and standardizing query keys. No issues found. ✅ 5 resolved✅ Quality: Persona FQN construction duplicated across two consumers
✅ Quality: useQuery docStore block duplicated across two hooks
✅ Performance: No staleTime means staggered mounts still refetch docStore
✅ Quality: Unused
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source
|
| Count | Rule |
|---|---|
| 16 | i18next/no-literal-string |
| 10 | sonarjs/no-duplicate-string |
| 1 | jsx-a11y/anchor-is-valid |
| 1 | react-hooks/exhaustive-deps |
| 1 | jsx-a11y/click-events-have-key-events |
| 1 | jsx-a11y/no-static-element-interactions |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | src/components/Glossary/GlossaryDetails/GlossaryDetails.test.tsx:24:39 |
i18next/no-literal-string |
disallow literal string: GlossaryTermTab.component |
| 🟡 | src/components/Glossary/GlossaryDetails/GlossaryDetails.test.tsx:27:39 |
i18next/no-literal-string |
disallow literal string: GlossaryHeader.component |
| 🟡 | src/components/Glossary/GlossaryDetails/GlossaryDetails.test.tsx:47:36 |
i18next/no-literal-string |
disallow literal string: testActivityFeedTab |
| 🟡 | src/components/Glossary/GlossaryDetails/GlossaryDetails.test.tsx:52:43 |
i18next/no-literal-string |
disallow literal string: Description |
| 🟡 | src/components/Glossary/GlossaryDetails/GlossaryDetails.test.tsx:95:55 |
i18next/no-literal-string |
disallow literal string: GenericTab |
| 🟡 | src/components/Glossary/GlossaryV1.test.tsx:77:56 |
jsx-a11y/anchor-is-valid |
The href attribute is required for an anchor to be keyboard accessible. Provide a valid, navigable address as the href value. If you cannot provide an href, but |
| 🟡 | src/components/Glossary/GlossaryV1.test.tsx:83:38 |
i18next/no-literal-string |
disallow literal string: <>Glossary-Details component</> |
| 🟡 | src/components/Glossary/GlossaryV1.test.tsx:87:38 |
i18next/no-literal-string |
disallow literal string: <>Glossary-Term component</> |
| 🟡 | src/components/Glossary/GlossaryV1.test.tsx:91:38 |
i18next/no-literal-string |
disallow literal string: <>TitleBreadcrumb</> |
| 🟡 | src/components/Glossary/GlossaryV1.test.tsx:95:34 |
i18next/no-literal-string |
disallow literal string: Breadcrumb |
| 🟡 | src/components/Glossary/GlossaryV1.test.tsx:103:39 |
i18next/no-literal-string |
disallow literal string: FeedEditor |
| 🟡 | src/hooks/useCustomPages.test.ts:48:25 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 3 times. |
| 🟡 | src/hooks/useCustomPages.test.ts:94:55 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 3 times. |
| 🟡 | src/pages/MyDataPage/MyDataPage.component.tsx:182:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has missing dependencies: 'isWelcomeVisible', 'updateWelcomeScreen', and 'usernameExistsInCookie'. Either include them or remove the depend |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:49:50 |
i18next/no-literal-string |
disallow literal string: Loader |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:61:50 |
i18next/no-literal-string |
disallow literal string: MyDataPageSkeleton |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:82:9 |
jsx-a11y/click-events-have-key-events |
Visible, non-interactive elements with click handlers must have at least one keyboard listener. |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:82:9 |
jsx-a11y/no-static-element-interactions |
Avoid non-native interactive elements. If using native HTML is not possible, add an appropriate role and support for tabbing, mouse, keyboard, and touch inputs |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:82:32 |
i18next/no-literal-string |
disallow literal string: WelcomeScreen |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:93:58 |
i18next/no-literal-string |
disallow literal string: CustomiseLandingPageHeader |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:153:11 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 7 times. |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:171:45 |
i18next/no-literal-string |
disallow literal string: <>LimitWrapper{children}</> |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:202:49 |
i18next/no-literal-string |
disallow literal string: Link |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:271:26 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 6 times. |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:288:31 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:291:31 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:298:33 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:299:33 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:300:33 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/pages/MyDataPage/MyDataPage.test.tsx:363:31 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 5 times. |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changed2 participants
Describe your changes:
I worked on eliminating three redundant identical GET requests to
/api/v1/docStore/name/persona.*that fired on every/my-datanavigation (543ms total wasted, all 404s) becauseMyDataPageand multipleuseCustomPagesconsumers each fetched independently with no cache coordination.Root cause:
useCustomPagesused a manualuseState/useCallback/useEffectpattern, andMyDataPagehad its own separatefetchDocument()+useEffect. Neither used React Query, so concurrent subscribers for the same persona FQN each fired an independent network request.Fix:
rest/queries/docStoreQuery.ts— shareddocStoreQueryKey(fqn)+docStoreQueryFn(fqn)following the existingtableQuery.tspatternuseCustomPagesfrom manual fetch touseQuerywith the shared key;pageTypefiltering moves into the return value (no longer triggers a re-fetch on pageType change — it filters from the cached doc)MyDataPage'sfetchDocument()+useEffectwithuseQueryusing the same key;layoutandpersonaPreferencesderived viauseMemoWith React Query's in-flight deduplication, all concurrent subscribers to
['docStore', 'persona.X']share exactly one network request.Type of change:
High-level design:
The existing
rest/queries/pattern (e.g.tableQuery.ts,dashboardQuery.ts) exports a canonicalqueryKey+queryFnpair so any consumer — detail page, sidebar widget, hover prefetch — hits the same normalised cache slot.docStoreQuery.tsadds the same plumbing for DocStore documents.useCustomPagespreviously re-fetched the full persona document on everypageTypechange even though the document contains all page types. The new implementation fetches once per persona FQN and filters locally, reducing N fetches to 1 per persona.MyDataPagepreviously had a duplicate fetch path independent ofuseCustomPages. Both now share the same React Query cache slot (['docStore', 'persona.X']), so on/my-datanavigation the document is fetched exactly once regardless of how many consumers are mounted.Tests:
Use cases covered
/my-datapage loads with a persona selected — one GET todocStore/name/persona.*instead of threeuseSidebarItems→useCustomPages('Navigation')) and page body share the cached response[], customizedPage resets tonull— same contract as beforepageTypebetween renders filters from the cache without a network round-tripUnit tests
useCustomPages.test.ts— wrapped withQueryClientProvider, updated "pageType changes" test to assert one fetch (not two), all other assertions preservedopenmetadata-ui/src/main/resources/ui/src/hooks/useCustomPages.test.tsBackend integration tests
Ingestion integration tests
Playwright (UI) tests
Manual testing performed
/my-datawith a persona assigneddocStore/api/v1/docStore/name/persona.*fires (was 3 before this change)UI screen recording / screenshots:
Not applicable — no visual change; the fix is a network deduplication at the data-fetching layer.
Checklist:
Fixes <issue-number>: <short explanation>Fixes #<issue-number>above.