perf(ui): deduplicate docStore persona requests via shared React Query key - #31300
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 ✅ 922 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 6s ⏱️ Max setup 3m 8s · max shard execution 18m 13s · max shard-job elapsed before upload 22m 8s · reporting 8s 🌐 214.64 requests/attempt · 2.50 app boots/UI scenario · 13.27% 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 —
|
…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>
|
| Count | Rule |
|---|---|
| 7 | openmetadata-imports/no-cross-page-imports |
| 6 | sonarjs/no-duplicate-string |
| 3 | sonarjs/cyclomatic-complexity |
| 2 | jsx-a11y/click-events-have-key-events |
| 2 | jsx-a11y/no-static-element-interactions |
| 2 | react-hooks/exhaustive-deps |
| 1 | jsx-a11y/anchor-is-valid |
| 1 | sonarjs/cognitive-complexity |
| 1 | openmetadata-imports/review-sequential-api-calls |
All findings
| Location | Rule | Message | |
|---|---|---|---|
| 🟡 | 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/hooks/useResolvedAppMode.ts:194:16 |
sonarjs/cognitive-complexity |
Refactor this function to reduce its Cognitive Complexity from 22 to the 15 allowed. |
| 🟡 | src/hooks/useResolvedAppMode.ts:194:16 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 22 which is greater than 10 authorized.","cost":12,"secondaryLocations":[{"line":194,"column":15,"endLine":194,"endColu |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.test.tsx:48:13 |
jsx-a11y/click-events-have-key-events |
Visible, non-interactive elements with click handlers must have at least one keyboard listener. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.test.tsx:48:13 |
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/CustomizablePage/CustomizablePage.tsx:53:1 |
openmetadata-imports/no-cross-page-imports |
Page features must not import another page feature. Move shared code to components, hooks, interfaces, or pure utilities. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:54:1 |
openmetadata-imports/no-cross-page-imports |
Page features must not import another page feature. Move shared code to components, hooks, interfaces, or pure utilities. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:55:1 |
openmetadata-imports/no-cross-page-imports |
Page features must not import another page feature. Move shared code to components, hooks, interfaces, or pure utilities. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:56:1 |
openmetadata-imports/no-cross-page-imports |
Page features must not import another page feature. Move shared code to components, hooks, interfaces, or pure utilities. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:57:1 |
openmetadata-imports/no-cross-page-imports |
Page features must not import another page feature. Move shared code to components, hooks, interfaces, or pure utilities. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:77:36 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 31 which is greater than 10 authorized.","cost":21,"secondaryLocations":[{"line":77,"column":35,"endLine":77,"endColumn |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:138:11 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:140:17 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:141:17 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:146:11 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:148:17 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:149:17 |
sonarjs/no-duplicate-string |
Define a constant instead of duplicating this literal 4 times. |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:276:54 |
sonarjs/cyclomatic-complexity |
{"message":"Function has a complexity of 11 which is greater than 10 authorized.","cost":1,"secondaryLocations":[{"line":276,"column":53,"endLine":276,"endColum |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:348:28 |
openmetadata-imports/review-sequential-api-calls |
Review these sequential API requests. If they are independent, start them together with Promise.all/Promise.allSettled; keep sequencing only when data-dependent |
| 🟡 | src/pages/CustomizablePage/CustomizablePage.tsx:380:6 |
react-hooks/exhaustive-deps |
React Hook useEffect has a missing dependency: 'initializeCustomizeStore'. Either include it or remove the dependency array. |
| 🟡 | src/pages/DataMarketplacePage/DataMarketplacePage.component.tsx:39:1 |
openmetadata-imports/no-cross-page-imports |
Page features must not import another page feature. Move shared code to components, hooks, interfaces, or pure utilities. |
| 🟡 | src/pages/MyDataPage/MyDataPage.component.tsx:50:1 |
openmetadata-imports/no-cross-page-imports |
Page features must not import another page feature. Move shared code to components, hooks, interfaces, or pure utilities. |
| 🟡 | 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: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 |
Fix locally (fast - only checks files changed in this branch):
make ui-checkstyle-changedWith isLoading = !hasMounted, RTL flushes the mount effect synchronously so isLoading resolves to false before the async React Query fetch completes. waitFor(() => isLoading === false) passed immediately, leaving customizedPage and navigation still null when the assertions ran. Gate the waitFor on the data assertions instead, which correctly block until the mock promise resolves. Also remove a stale comment in the no-persona test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
When a persona layout, navigation, background colour, or app mode is saved via CustomizablePage, the write reaches the docStore API but the shared ['docStore', fqn] React Query cache was never updated. Consumers such as useCustomPages and MyDataPage kept serving the pre-save data for up to five minutes (PERSONA_DOC_STALE_TIME). After each successful write, call queryClient.setQueryData with the fresh API response so all cache subscribers immediately see the updated doc. Using setQueryData avoids an extra round-trip because we already hold the authoritative response from the mutation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🚦 Removed from the merge queue —
|
Code Review ✅ Approved 7 resolved / 7 findingsDeduplicates persona docStore requests across UI components using a shared React Query cache key and proper staleTime configuration, successfully resolving all redundant network calls. No issues found. ✅ 7 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
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.Greptile Summary
The PR consolidates persona DocStore reads under a shared React Query key and updates that cache after successful customization saves.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
sequenceDiagram participant UI as Persona UI consumers participant RQ as React Query cache participant API as DocStore API participant Editor as CustomizablePage UI->>RQ: "Read ["docStore", "persona.<fqn>"]" alt Cache miss RQ->>API: GET persona document API-->>RQ: Document end RQ-->>UI: Shared document Editor->>API: Create or update customization API-->>Editor: Updated document Editor->>RQ: setQueryData(shared key, response) RQ-->>UI: Updated documentReviews (9): Last reviewed commit: "addressed gitar comment" | Re-trigger Greptile
Context used: