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 17 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) 36m 58s ⏱️ Max setup 3m 7s · max shard execution 18m 50s · max shard-job elapsed before upload 22m 36s · reporting 5s 🌐 210.15 requests/attempt · 2.50 app boots/UI scenario · 21.15% 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>
Code Review ✅ Approved 4 resolved / 4 findingsDeduplicates docStore persona requests by introducing a shared React Query cache key and migrating useCustomPages and MyDataPage to use it, addressing the persona FQN duplication and staleTime findings. ✅ 4 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: isLoading mirrors derived value via state+effect, adding a render
OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
|
2 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.