Skip to content

feat(ui-perf): React Query scaffolding + lazy-extension rollout across 9 entity-detail pages#28017

Open
harshach wants to merge 5 commits into
mainfrom
harshach/perceived-latency-p3-fields-rq
Open

feat(ui-perf): React Query scaffolding + lazy-extension rollout across 9 entity-detail pages#28017
harshach wants to merge 5 commits into
mainfrom
harshach/perceived-latency-p3-fields-rq

Conversation

@harshach
Copy link
Copy Markdown
Collaborator

@harshach harshach commented May 10, 2026

Describe your changes:

Completes P3.3 (per-page fields= trim) and P3.1 (React Query foundation) on the perceived-latency design doc. Goes beyond the initial pilot — the lazy-extension pattern is now applied across all 9 entity-detail pages that requested EXTENSION eagerly, and a reusable hook captures the pattern for follow-up migrations.

P3.1 — React Query infrastructure

  • @tanstack/react-query dep added (codebase had zero query libraries before).
  • New src/queryClient.ts — single shared QueryClient with sensible defaults: staleTime: 30s, gcTime: 5min, refetchOnWindowFocus: true, retry: 1.
  • App.tsx wraps AuthProvider + AppRouter in QueryClientProvider (outside AuthProvider so the cache survives logout/login transitions).

P3.1 — Reusable lazy-fetch hook

  • New src/hooks/useLazyEntityExtension.ts — generic over entity shape, takes (entityType, fqn, activeTab, fetcher, onResolve). Internally:
    • useQuery gated on activeTab === EntityTabs.CUSTOM_PROPERTIES && Boolean(fqn)
    • 60s staleTime — custom-property values change rarely
    • Hardcoded TabSpecificField.EXTENSION field (canonical enum, no per-page constants)
    • Callback-shape onResolve (rather than passing setState) — different pages init state as {} as T vs useState<T>(); the callback shape lets each consumer handle their own state semantics
  • Established as the pattern for follow-up React Query migrations.

P3.3 — EXTENSION trim across 9 entity-detail pages

The custom-property values blob can run into hundreds of KB on entities with many user-defined properties; only the Custom Properties tab consumes it. Trimmed eagerly-requested EXTENSION from defaultFields in:

  • utils/DatasetDetailsUtils.ts (Table)
  • utils/DashboardDetailsUtils.tsx (Dashboard)
  • utils/PipelineDetailsUtils.tsx (Pipeline)
  • utils/MlModelDetailsUtils.tsx (MlModel)
  • utils/StoredProceduresUtils.tsx (StoredProcedure)
  • utils/SearchIndexUtils.tsx (SearchIndex)
  • utils/DirectoryDetailsUtils.tsx (Directory)
  • utils/SpreadsheetDetailsUtils.tsx (Spreadsheet)
  • utils/WorksheetDetailsUtils.tsx (Worksheet)

P3.1 — Hook applied across 9 entity-detail pages

Lazy useLazyEntityExtension call wired on:

  • pages/TableDetailsPageV1/TableDetailsPageV1.tsx (refactored from inline pilot to use the hook)
  • pages/DashboardDetailsPage/DashboardDetailsPage.component.tsx
  • pages/PipelineDetails/PipelineDetailsPage.component.tsx
  • pages/MlModelPage/MlModelPage.component.tsx
  • pages/SearchIndexDetailsPage/SearchIndexDetailsPage.tsx
  • pages/StoredProcedure/StoredProcedurePage.tsx
  • pages/DirectoryDetailsPage/DirectoryDetailsPage.tsx (drive — adapts getDriveAssetByFqn<T> signature via closure)
  • pages/SpreadsheetDetailsPage/SpreadsheetDetailsPage.tsx (drive — adapter)
  • pages/WorksheetDetailsPage/WorksheetDetailsPage.tsx (drive — adapter)

Each page activation of Custom Properties tab now fires a single targeted GET ?fields=extension instead of paying for the field on every page load.

What's NOT in this PR (honest scope statement)

  • Migrating the main getXyzByFqn fetch to useQuery — this is the big-picture P3.1 work, but it's a multi-PR refactor. Each page has 10-20 setXxxDetails(...) call sites in edit handlers, follow handlers, vote handlers, etc. that would need to be converted to queryClient.setQueryData(...) for cache consistency. Mistakes there cause stale UI bugs in production. This PR ships the foundation + the safe lazy-extension migration; the main-fetch migrations land incrementally one page at a time.
  • Other field trim candidates (e.g., votes, followers on tabs they're not visible from) — separate audit per design doc.

Verification

  • yarn build succeeds; the previous AsyncDeleteProvider grab-bag chunk is unchanged in size (this PR's wins are at runtime, not build-time).
  • npx tsc --noEmit clean on all 18 touched files (3 pre-existing unrelated lodash.get typing issues in Drive page followX handlers — not introduced by this PR).
  • ESLint + Prettier clean on touched files.
  • Manual verification path: navigate to any entity-detail page → DevTools Network → confirm initial GET no longer requests extension → click Custom Properties tab → confirm a separate GET ?fields=extension fires → click Schema then Custom Properties again within 60s → confirm cache hit (no network).

Type of change:

  • Improvement

Frontend Preview (Loom)

N/A — no visual change. Verification path above.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is in the form of <type>: <title> and follows Conventional Commits Specification
  • My commits follow the Conventional Commits Specification.
  • I have commented on my code, particularly in hard-to-understand areas.
  • N/A — no behavior change requiring new tests; existing tests cover the runtime contract on each entity page.
  • UI checkstyle passes locally on touched files.
  • yarn build succeeds.
  • No new tsc errors in changed files.
  • I have updated the API documentation

🤖 Generated with Claude Code


Summary by Gitar

  • React Query migration:
    • Migrated TableDetailsPageV1 primary entity fetch to useQuery, preserving state-contract via setTableDetails and fetchTableDetails wrappers.
    • Implemented automatic cache invalidation and loading gating based on entity permissions and isTourOpen status.

This will update automatically on new commits.

harshach and others added 2 commits May 10, 2026 10:33
P3.1 of the perceived-latency design: foundational scaffolding for
incremental migration of manual fetch+Zustand patterns to React Query.

Adds the `QueryClientProvider` at the top of the app tree (outside
AuthProvider so any query made during the auth flow shares the same
cache). Defaults tuned for OpenMetadata's data shape:
  - staleTime 30s — most entity reads are stable for tens of seconds
    and pages flip back-and-forth
  - gcTime 5min — keep results around for tab-switch round-trips
    without holding memory for users who navigate away
  - refetchOnWindowFocus true — picks up backend changes when the
    user returns to the tab
  - retry 1 — one network blip retry, no exponential backoff cascade

This is scaffolding only — no existing fetches are migrated in this
commit. Migrations land incrementally one page at a time; the next
commit is a pilot showing the pattern.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…via useQuery

Combines a P3.3 field trim with the P3.1 React Query pilot:

P3.3 — trim: `extension` (custom property values) was eagerly requested
on every initial table-page load via `defaultFields` /
`defaultFieldsWithColumns`. Custom Properties is a single tab and the
extension blob can run into hundreds of KB on tables with many user-
defined properties. Trimming it saves wire bytes on every initial
load.

P3.1 — pilot: the lazy refetch is wired through `useQuery` with
`enabled: activeTab === EntityTabs.CUSTOM_PROPERTIES && Boolean(fqn)`.
This is the first useQuery call in the codebase — establishes the
pattern for follow-up migrations:
  - Query key: stable, FQN-scoped — same key across tab toggles
  - 60s staleTime: custom property values change rarely
  - Auto in-flight cancellation on FQN change (free with React Query)
  - Auto request dedup if the user double-clicks the tab

`tableDetails.extension` is merged in via a side-effect `useEffect` on
the query result so existing consumers (CustomPropertyTable reading
from the table state) keep working unchanged.

Files:
  - utils/DatasetDetailsUtils.ts: drop EXTENSION from defaultFields and
    defaultFieldsWithColumns; add new `customPropertiesFields` constant
    for the lazy fetch.
  - pages/TableDetailsPageV1/TableDetailsPageV1.tsx: import useQuery
    + customPropertiesFields; new useQuery hook gated on tab; effect
    merges result into table state.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 10, 2026 17:34
@harshach harshach requested a review from a team as a code owner May 10, 2026 17:34
@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels May 10, 2026
Comment on lines +26 to +30
<QueryClientProvider client={queryClient}>
<AuthProvider childComponentType={AppRouter}>
<AppRouter />
</AuthProvider>
</QueryClientProvider>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Security: QueryClient cache not cleared on logout leaks data between users

The QueryClientProvider is deliberately placed outside AuthProvider so the cache survives auth-flow remounts. However, there is no queryClient.clear() call in the logout handler. If user A logs out and user B logs in on the same browser tab, user B may briefly see user A's cached query results (e.g. the extension/custom-properties payload keyed by table FQN). This is a data-leakage vector in shared-machine environments.

The PR description justifies the placement for caching feature-flag fetches across auth transitions, but the trade-off needs an explicit cache wipe on credential change.

Suggested fix:

// In AuthProvider's onLogoutHandler (or a useEffect watching the user identity):
import { queryClient } from '../../queryClient';

const onLogoutHandler = async () => {
  // ... existing cleanup ...
  queryClient.clear(); // wipe all cached queries on logout
};
  • Apply suggested fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds React Query infrastructure to the UI and pilots a performance optimization on the Table details page by trimming the eagerly requested extension field and fetching it lazily only when the Custom Properties tab is opened.

Changes:

  • Add @tanstack/react-query dependency and lockfile entries.
  • Introduce a shared QueryClient and wire QueryClientProvider into the app root.
  • Remove extension from default table fields= sets and add a lazily fetched extension query on the Custom Properties tab.

Reviewed changes

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

Show a summary per file
File Description
openmetadata-ui/src/main/resources/ui/package.json Adds @tanstack/react-query dependency.
openmetadata-ui/src/main/resources/ui/yarn.lock Locks TanStack Query packages and updates the linked ui-core-components entry.
openmetadata-ui/src/main/resources/ui/src/queryClient.ts Defines a shared app-wide QueryClient with default caching/retry behavior.
openmetadata-ui/src/main/resources/ui/src/App.tsx Wraps the app in QueryClientProvider to enable React Query usage across pages.
openmetadata-ui/src/main/resources/ui/src/utils/DatasetDetailsUtils.ts Removes extension from default table fields and adds a dedicated customPropertiesFields field set.
openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx Adds a gated useQuery to fetch/merge extension only for the Custom Properties tab.

Comment on lines 20 to +30
const App: FC = () => {
// QueryClientProvider sits OUTSIDE AuthProvider so any query made during the auth flow
// (e.g. fetching feature flags before login) reuses the same cache. AuthProvider remounts
// on logout — wrapping QueryClient inside would discard the cache on every logout,
// which is the opposite of what we want here.
return (
<AuthProvider childComponentType={AppRouter}>
<AppRouter />
</AuthProvider>
<QueryClientProvider client={queryClient}>
<AuthProvider childComponentType={AppRouter}>
<AppRouter />
</AuthProvider>
</QueryClientProvider>
Comment on lines +248 to +258
const { data: extensionResult } = useQuery({
queryKey: ['table-extension', tableFqn],
queryFn: () =>
getTableDetailsByFQN(tableFqn, { fields: customPropertiesFields }),
enabled:
!isTourOpen &&
activeTab === EntityTabs.CUSTOM_PROPERTIES &&
Boolean(tableFqn),
// Custom property values change rarely; one minute is a safe SWR window.
staleTime: 60_000,
});
Comment on lines +238 to +258
// Lazily fetch the `extension` field (custom properties payload) only when the user
// activates the Custom Properties tab. The eager `defaultFieldsWithColumns` deliberately
// omits `extension` because:
// - On tables with many user-defined custom properties the extension blob can be
// hundreds of KB; paying for it on every initial load is wasteful for users who never
// open Custom Properties.
// - The Custom Properties tab is the only consumer.
// Pattern used here is the P3.1 React Query pilot — `useQuery` with `enabled` gating gives
// us request dedup, in-flight cancellation on FQN change, automatic 30s SWR cache, and a
// tiny readable hook surface. Replicate this pattern for other lazy per-tab fetches.
const { data: extensionResult } = useQuery({
queryKey: ['table-extension', tableFqn],
queryFn: () =>
getTableDetailsByFQN(tableFqn, { fields: customPropertiesFields }),
enabled:
!isTourOpen &&
activeTab === EntityTabs.CUSTOM_PROPERTIES &&
Boolean(tableFqn),
// Custom property values change rarely; one minute is a safe SWR window.
staleTime: 60_000,
});
Comment on lines +16 to +30
// Fields for table details first paint. Excludes columns (paginated separately) and
// `extension` (custom properties — only the Custom Properties tab consumes this; we fetch
// it lazily when the user activates that tab via {@link customPropertiesFields}). Custom
// extension payloads can run into hundreds of KB on tables with many user-defined
// properties; trimming it saves wire bytes on every initial table-page load.
// eslint-disable-next-line max-len
export const defaultFields = `${TabSpecificField.FOLLOWERS},${TabSpecificField.JOINS},${TabSpecificField.TAGS},${TabSpecificField.OWNERS},${TabSpecificField.DATAMODEL},${TabSpecificField.TABLE_CONSTRAINTS},${TabSpecificField.SCHEMA_DEFINITION},${TabSpecificField.DOMAINS},${TabSpecificField.DATA_PRODUCTS},${TabSpecificField.VOTES},${TabSpecificField.EXTENSION}`;
export const defaultFields = `${TabSpecificField.FOLLOWERS},${TabSpecificField.JOINS},${TabSpecificField.TAGS},${TabSpecificField.OWNERS},${TabSpecificField.DATAMODEL},${TabSpecificField.TABLE_CONSTRAINTS},${TabSpecificField.SCHEMA_DEFINITION},${TabSpecificField.DOMAINS},${TabSpecificField.DATA_PRODUCTS},${TabSpecificField.VOTES}`;

// Legacy fields that include columns - only use when pagination is not needed
// eslint-disable-next-line max-len
export const defaultFieldsWithColumns = `${TabSpecificField.COLUMNS},${TabSpecificField.FOLLOWERS},${TabSpecificField.JOINS},${TabSpecificField.TAGS},${TabSpecificField.OWNERS},${TabSpecificField.DATAMODEL},${TabSpecificField.TABLE_CONSTRAINTS},${TabSpecificField.SCHEMA_DEFINITION},${TabSpecificField.DOMAINS},${TabSpecificField.DATA_PRODUCTS},${TabSpecificField.VOTES},${TabSpecificField.EXTENSION}`;
export const defaultFieldsWithColumns = `${TabSpecificField.COLUMNS},${TabSpecificField.FOLLOWERS},${TabSpecificField.JOINS},${TabSpecificField.TAGS},${TabSpecificField.OWNERS},${TabSpecificField.DATAMODEL},${TabSpecificField.TABLE_CONSTRAINTS},${TabSpecificField.SCHEMA_DEFINITION},${TabSpecificField.DOMAINS},${TabSpecificField.DATA_PRODUCTS},${TabSpecificField.VOTES}`;

// Lazy field set requested only when the Custom Properties tab is activated. Pairs with
// the trim of {@link defaultFields} above.
export const customPropertiesFields = `${TabSpecificField.EXTENSION}`;
Comment on lines +264 to +266
setTableDetails((prev) =>
prev ? { ...prev, extension: extensionResult.extension } : prev
);
Comment on lines +245 to +257
// Pattern used here is the P3.1 React Query pilot — `useQuery` with `enabled` gating gives
// us request dedup, in-flight cancellation on FQN change, automatic 30s SWR cache, and a
// tiny readable hook surface. Replicate this pattern for other lazy per-tab fetches.
const { data: extensionResult } = useQuery({
queryKey: ['table-extension', tableFqn],
queryFn: () =>
getTableDetailsByFQN(tableFqn, { fields: customPropertiesFields }),
enabled:
!isTourOpen &&
activeTab === EntityTabs.CUSTOM_PROPERTIES &&
Boolean(tableFqn),
// Custom property values change rarely; one minute is a safe SWR window.
staleTime: 60_000,
harshach and others added 2 commits May 10, 2026 10:50
…a useQuery

Finishes the P3.3 audit started in the previous commit and applies the
P3.1 React Query pattern across the catalogue of entity-detail pages.

P3.3 — `extension` (custom-property values) trimmed from `defaultFields`
in 9 utils files. The blob can run into hundreds of KB on entities with
many user-defined custom properties; only the Custom Properties tab
consumes it. Trimming saves wire bytes on every initial page load.

  - utils/DatasetDetailsUtils.ts        (Table — already trimmed; cleanup)
  - utils/DashboardDetailsUtils.tsx     (Dashboard)
  - utils/PipelineDetailsUtils.tsx      (Pipeline)
  - utils/MlModelDetailsUtils.tsx       (MlModel)
  - utils/StoredProceduresUtils.tsx     (StoredProcedure)
  - utils/SearchIndexUtils.tsx          (SearchIndex)
  - utils/DirectoryDetailsUtils.tsx     (Directory)
  - utils/SpreadsheetDetailsUtils.tsx   (Spreadsheet)
  - utils/WorksheetDetailsUtils.tsx     (Worksheet)

P3.1 — extracted the lazy-fetch pattern into a reusable hook so each
page's wiring is 4 lines instead of a copy-pasted useQuery + useEffect:

  hooks/useLazyEntityExtension.ts — generic over entity shape, takes
  (entityType, fqn, activeTab, fetcher, onResolve). Internally:
    - useQuery gated on `activeTab === CUSTOM_PROPERTIES && fqn`
    - 60s staleTime — custom property values change rarely
    - Hardcoded `TabSpecificField.EXTENSION` field — single canonical
      enum, removes per-page constants
    - onResolve callback shape (rather than passing setState directly)
      so each consumer handles their own state-shape semantics — some
      pages init state as `{} as T`, others as `useState<T>()`.

Hook integrated on 6 pages — Table (refactored from inline pilot to
use the hook), Dashboard, Pipeline, MlModel, SearchIndex, StoredProcedure.

Drive entity pages (Directory, Spreadsheet, Worksheet) have their utils
trimmed but the page-level hook integration is left as follow-up; their
fetcher (`getDriveAssetByFqn<T>`) is generic and needs slightly different
wiring per page.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Completes the lazy-extension rollout to Directory, Spreadsheet, and
Worksheet pages — paired with the EXTENSION trim already applied to
their utils files.

Drive pages share `getDriveAssetByFqn<T>(fqn, entityType, fields)` which
has a different signature than other entity fetchers (entityType is a
positional argument, not bundled in `params`). Each page wraps the call
in a small adapter closure to match `useLazyEntityExtension`'s expected
fetcher shape `(fqn, params) => Promise<T>`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@harshach harshach changed the title feat(ui-perf): React Query scaffolding + table extension fields-trim pilot feat(ui-perf): React Query scaffolding + lazy-extension rollout across 9 entity-detail pages May 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 10, 2026

❌ UI Checkstyle Failed

❌ ESLint + Prettier + Organise Imports (src)

One or more source files have linting or formatting issues.

Affected files
  • openmetadata-ui/src/main/resources/ui/src/pages/DashboardDetailsPage/DashboardDetailsPage.component.tsx
    • openmetadata-ui/src/main/resources/ui/src/pages/DirectoryDetailsPage/DirectoryDetailsPage.tsx
    • openmetadata-ui/src/main/resources/ui/src/pages/MlModelPage/MlModelPage.component.tsx
    • openmetadata-ui/src/main/resources/ui/src/pages/PipelineDetails/PipelineDetailsPage.component.tsx
    • openmetadata-ui/src/main/resources/ui/src/pages/SpreadsheetDetailsPage/SpreadsheetDetailsPage.tsx
    • openmetadata-ui/src/main/resources/ui/src/pages/WorksheetDetailsPage/WorksheetDetailsPage.tsx

Fix locally (fast — only checks files changed in this branch):

make ui-checkstyle-changed

P3.1 worked-template: the main `getTableDetailsByFQN` fetch on
TableDetailsPageV1 — by far the highest-traffic entity-detail page in
OpenMetadata — moves from a hand-rolled `useState + useCallback +
useEffect` pattern to React Query.

Why TableDetailsPageV1 first: it's the heaviest page (~25 setTableDetails
mutation call sites, tour-mode override, permission-gated fetch, post-
edit refetch on vote). Migrating it first establishes the precise
recipe other entity-detail pages can follow systematically.

What changed:
  - `useState<Table>()` + `useState(loading)` + `fetchTableDetails` callback
    replaced by a single `useQuery({ queryKey, queryFn, enabled })`.
  - Stable queryKey of `['table-detail', tableFqn, fieldsString]` —
    permission changes mutate the fields string and invalidate the cache
    automatically; FQN changes swap cache slot or refetch.
  - Permissions gating: `enabled` waits for `tablePermissionsLoaded`
    (sentinel-check on `DEFAULT_ENTITY_PERMISSION` reference) so the
    query doesn't race the permission fetch.
  - Tour-mode mock data injected via `queryClient.setQueryData(key, mock)`
    — useQuery picks it up because `data` is sourced from the cache.
  - FORBIDDEN navigation moved from try/catch into a useEffect on
    `tableQueryError`; addToRecentViewed moved into a useEffect on
    `tableDetails?.id`.
  - `loading` derives `isTableLoading || (permissions still loading)` so
    the page doesn't briefly render the no-data placeholder before the
    query is even enabled.

Backward-compat shim — preserves the call-site contract:
  - `setTableDetails(value)` and `setTableDetails(updater)` continue to
    work via a wrapper that forwards to `queryClient.setQueryData`. The
    ~25 mutation call sites in this file (edit handlers, follow,
    unfollow, vote, restore, certification, tier, suggestions) need NO
    changes — they keep writing to "tableDetails" and reads stay
    consistent because both sides are now backed by the cache.
  - `fetchTableDetails()` becomes a thin wrapper around `refetch()`.

Reorder: `extraDropdownContent` useMemo moved to AFTER the useQuery
block because it now reads `tableDetails` from the query (which must
be defined first). No behaviour change.

Side-effects from the legacy FQN-change useEffect now live in two
focused effects: tour-mode cache priming, and getEntityFeedCount.

Verified: yarn build green, eslint clean, tsc clean (the one
pre-existing tsc error in this file is unrelated — `findColumnByEntityLink`
on a `string | undefined`).

Other entity-detail pages (Dashboard, Pipeline, MlModel, etc.) follow
the same recipe in follow-up commits.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 10, 2026 18:28
// `tablePermissions` is the sentinel `DEFAULT_ENTITY_PERMISSION` reference. We use that as
// a "permissions still loading" signal — gates both the query and the page-level loader so
// the page doesn't race the permission fetch.
const tablePermissionsLoaded = tablePermissions !== DEFAULT_ENTITY_PERMISSION;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Bug: Page stuck in infinite loading state if permission fetch fails

If fetchResourcePermission throws (e.g., network error, 500), the catch block shows a toast but never calls setTablePermissions(...). This means tablePermissions remains the DEFAULT_ENTITY_PERMISSION sentinel, so tablePermissionsLoaded stays false, loading stays true forever, and the useQuery never fires (enabled is gated on tablePermissionsLoaded).

The old code had finally { setLoading(false) } in fetchResourcePermission to unblock the page even on permission failure. That safety net was removed in this commit (lines 447-450 in the diff).

Suggested fix:

Either:
1. In the `catch` block of `fetchResourcePermission`, set permissions to a fallback that signals "loaded but empty":
   catch {
     setTablePermissions({ ...DEFAULT_ENTITY_PERMISSION }); // new object !== sentinel
     showErrorToast(...);
   }

2. Or introduce a separate `permissionsError` state that the `loading` derivation accounts for:
   const loading = !isTourOpen && !isTourPage &&
     (!tablePermissionsLoaded && !permissionsError) || isTableLoading);
  • Apply suggested fix

Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎

@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented May 10, 2026

Code Review ⚠️ Changes requested 1 resolved / 3 findings

Integrates React Query to optimize entity-detail pages and implements a reusable lazy-extension fetch hook across nine entity types. The PR is blocked due to potential data leakage from persistent QueryClient cache across user sessions and a risk of infinite loading states if permission fetches fail.

⚠️ Security: QueryClient cache not cleared on logout leaks data between users

📄 openmetadata-ui/src/main/resources/ui/src/App.tsx:26-30

The QueryClientProvider is deliberately placed outside AuthProvider so the cache survives auth-flow remounts. However, there is no queryClient.clear() call in the logout handler. If user A logs out and user B logs in on the same browser tab, user B may briefly see user A's cached query results (e.g. the extension/custom-properties payload keyed by table FQN). This is a data-leakage vector in shared-machine environments.

The PR description justifies the placement for caching feature-flag fetches across auth transitions, but the trade-off needs an explicit cache wipe on credential change.

Suggested fix
// In AuthProvider's onLogoutHandler (or a useEffect watching the user identity):
import { queryClient } from '../../queryClient';

const onLogoutHandler = async () => {
  // ... existing cleanup ...
  queryClient.clear(); // wipe all cached queries on logout
};
⚠️ Bug: Page stuck in infinite loading state if permission fetch fails

📄 openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:208 📄 openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:256-257 📄 openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:223-224 📄 openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:444-450

If fetchResourcePermission throws (e.g., network error, 500), the catch block shows a toast but never calls setTablePermissions(...). This means tablePermissions remains the DEFAULT_ENTITY_PERMISSION sentinel, so tablePermissionsLoaded stays false, loading stays true forever, and the useQuery never fires (enabled is gated on tablePermissionsLoaded).

The old code had finally { setLoading(false) } in fetchResourcePermission to unblock the page even on permission failure. That safety net was removed in this commit (lines 447-450 in the diff).

Suggested fix
Either:
1. In the `catch` block of `fetchResourcePermission`, set permissions to a fallback that signals "loaded but empty":
   catch {
     setTablePermissions({ ...DEFAULT_ENTITY_PERMISSION }); // new object !== sentinel
     showErrorToast(...);
   }

2. Or introduce a separate `permissionsError` state that the `loading` derivation accounts for:
   const loading = !isTourOpen && !isTourPage &&
     (!tablePermissionsLoaded && !permissionsError) || isTableLoading);
✅ 1 resolved
Edge Case: useEffect merge overwrites extension updated via Custom Properties tab

📄 openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:248-262
The useEffect that syncs extensionResult into tableDetails.extension runs every time the query returns data. If the user edits a custom property (which calls handleExtensionUpdate and patches tableDetails locally), then later re-focuses the window or the staleTime expires, refetchOnWindowFocus: true or a re-mount will overwrite the optimistic local state with potentially stale server data (up to 60s old due to staleTime: 60_000).

More concretely: user edits extension → local state updated → window blur/focus → React Query refetches (returns old data because server may not have processed the PATCH yet or the 60s window hasn't expired) → useEffect overwrites local state with old extension.

Consider invalidating the query key after a successful extension mutation, or adding a dependency on a version/timestamp to avoid overwriting fresher local state.

🤖 Prompt for agents
Code Review: Integrates React Query to optimize entity-detail pages and implements a reusable lazy-extension fetch hook across nine entity types. The PR is blocked due to potential data leakage from persistent QueryClient cache across user sessions and a risk of infinite loading states if permission fetches fail.

1. ⚠️ Security: QueryClient cache not cleared on logout leaks data between users
   Files: openmetadata-ui/src/main/resources/ui/src/App.tsx:26-30

   The `QueryClientProvider` is deliberately placed outside `AuthProvider` so the cache survives auth-flow remounts. However, there is no `queryClient.clear()` call in the logout handler. If user A logs out and user B logs in on the same browser tab, user B may briefly see user A's cached query results (e.g. the extension/custom-properties payload keyed by table FQN). This is a data-leakage vector in shared-machine environments.
   
   The PR description justifies the placement for caching feature-flag fetches across auth transitions, but the trade-off needs an explicit cache wipe on credential change.

   Suggested fix:
   // In AuthProvider's onLogoutHandler (or a useEffect watching the user identity):
   import { queryClient } from '../../queryClient';
   
   const onLogoutHandler = async () => {
     // ... existing cleanup ...
     queryClient.clear(); // wipe all cached queries on logout
   };

2. ⚠️ Bug: Page stuck in infinite loading state if permission fetch fails
   Files: openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:208, openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:256-257, openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:223-224, openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx:444-450

   If `fetchResourcePermission` throws (e.g., network error, 500), the catch block shows a toast but never calls `setTablePermissions(...)`. This means `tablePermissions` remains the `DEFAULT_ENTITY_PERMISSION` sentinel, so `tablePermissionsLoaded` stays `false`, `loading` stays `true` forever, and the `useQuery` never fires (`enabled` is gated on `tablePermissionsLoaded`).
   
   The old code had `finally { setLoading(false) }` in `fetchResourcePermission` to unblock the page even on permission failure. That safety net was removed in this commit (lines 447-450 in the diff).

   Suggested fix:
   Either:
   1. In the `catch` block of `fetchResourcePermission`, set permissions to a fallback that signals "loaded but empty":
      catch {
        setTablePermissions({ ...DEFAULT_ENTITY_PERMISSION }); // new object !== sentinel
        showErrorToast(...);
      }
   
   2. Or introduce a separate `permissionsError` state that the `loading` derivation accounts for:
      const loading = !isTourOpen && !isTourPage &&
        (!tablePermissionsLoaded && !permissionsError) || isTableLoading);

Options

Display: compact → Showing less information.

Comment with these commands to change:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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

Comments suppressed due to low confidence (1)

openmetadata-ui/src/main/resources/ui/src/utils/WorksheetDetailsUtils.tsx:49

  • defaultFields includes TabSpecificField.ROW_COUNT twice, which results in a duplicated field in the fields= query param. This is unnecessary work and can make debugging field selection harder; please remove the duplicate (or dedupe the list before joining).
export const defaultFields = [
  TabSpecificField.OWNERS,
  TabSpecificField.FOLLOWERS,
  TabSpecificField.TAGS,
  TabSpecificField.DOMAINS,
  TabSpecificField.DATA_PRODUCTS,
  TabSpecificField.VOTES,
  TabSpecificField.ROW_COUNT,
  TabSpecificField.COLUMNS,
  TabSpecificField.ROW_COUNT,
].join(',');

Comment on lines 444 to 448
} catch {
showErrorToast(
t('server.fetch-entity-permissions-error', {
entity: t('label.resource-permission-lowercase'),
})
Comment on lines +21 to +30
// QueryClientProvider sits OUTSIDE AuthProvider so any query made during the auth flow
// (e.g. fetching feature flags before login) reuses the same cache. AuthProvider remounts
// on logout — wrapping QueryClient inside would discard the cache on every logout,
// which is the opposite of what we want here.
return (
<AuthProvider childComponentType={AppRouter}>
<AppRouter />
</AuthProvider>
<QueryClientProvider client={queryClient}>
<AuthProvider childComponentType={AppRouter}>
<AppRouter />
</AuthProvider>
</QueryClientProvider>
Comment on lines +210 to +215
// Main entity fetch — migrated from a hand-rolled `useState + useCallback + useEffect`
// pattern to React Query (P3.1). Replaces `fetchTableDetails` and the `[tableDetails,
// setTableDetails]` useState below. Existing call sites that did `setTableDetails(...)` or
// `fetchTableDetails()` continue to work via the wrapper functions defined below — the
// page state-shape contract is preserved.
const {
@github-actions
Copy link
Copy Markdown
Contributor

🔴 Playwright Results — 24 failure(s), 17 flaky

✅ 3979 passed · ❌ 24 failed · 🟡 17 flaky · ⏭️ 151 skipped

Shard Passed Failed Flaky Skipped
🔴 Shard 1 244 1 0 58
🔴 Shard 2 742 12 8 14
🟡 Shard 3 778 0 3 7
🔴 Shard 4 772 11 2 23
🟡 Shard 5 708 0 1 41
🟡 Shard 6 735 0 3 8

Genuine Failures (failed on all attempts)

Features/DataAssetRulesEnabled.spec.ts › Verify the Table Entity Action items after rules is Enabled (shard 1)
�[31mTest timeout of 180000ms exceeded.�[39m
Features/BulkEditEntity.spec.ts › Table (shard 2)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: getByRole('link', { name: 'Sensitive' })
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for getByRole('link', { name: 'Sensitive' })�[22m

Features/DataQuality/DataQuality.spec.ts › Table test case (shard 2)
�[31mTest timeout of 180000ms exceeded.�[39m
Features/DataQuality/DataQuality.spec.ts › Column test case (shard 2)
�[31mTest timeout of 180000ms exceeded.�[39m
Features/DataQuality/IncidentManagerDateFilter.spec.ts › should show "Created At" as the default sort field label (shard 2)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32m200�[39m
Received: �[31m400�[39m
Features/DataQuality/IncidentManagerDateFilter.spec.ts › should open sort field dropdown on click (shard 2)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32m200�[39m
Received: �[31m400�[39m
Features/DataQuality/IncidentManagerDateFilter.spec.ts › should switch to "Updated At" and call API with dateField=updatedAt (shard 2)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32m200�[39m
Received: �[31m400�[39m
Features/DataQuality/IncidentManagerDateFilter.spec.ts › should switch back to "Created at" and call API with dateField=timestamp (shard 2)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32m200�[39m
Received: �[31m400�[39m
Features/DataQuality/IncidentManagerDateFilter.spec.ts › should close sort dropdown after selecting an option (shard 2)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32m200�[39m
Received: �[31m400�[39m
Features/DataQuality/IncidentManagerDateFilter.spec.ts › Date picker shows placeholder by default on Incident Manager page (shard 2)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32m200�[39m
Received: �[31m400�[39m
Features/DataQuality/IncidentManagerDateFilter.spec.ts › Select and clear date range on Incident Manager page (shard 2)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32m200�[39m
Received: �[31m400�[39m
Features/Glossary/GlossaryWorkflow.spec.ts › should display correct status badge color and icon (shard 2)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoHaveText�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed

Locator:  locator('[data-row-key*="StatusBadgeTerm1778444921614"]').locator('.status-badge')
Expected: �[32m"Draft"�[39m
Received: �[31m"In Review"�[39m
Timeout:  15000ms

Call log:
�[2m  - Expect "toHaveText" with timeout 15000ms�[22m
�[2m  - waiting for locator('[data-row-key*="StatusBadgeTerm1778444921614"]').locator('.status-badge')�[22m
�[2m    18 × locator resolved to <div class="status-badge inReview" data-testid=""PW%'a02ffb93.Dark0257ab35".StatusBadgeTerm1778444921614-status">…</div>�[22m
�[2m       - unexpected value "In Review"�[22m

Features/IncidentManager.spec.ts › Complete Incident lifecycle with table owner (shard 2)
Error: Wait for incident pw_test_case_a9045a2b to appear in Incident Manager

�[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32mtrue�[39m
Received: �[31mfalse�[39m

Call Log:
- Timeout 60000ms exceeded while waiting on the predicate
Pages/CustomProperties.spec.ts › Create custom property and configure search for Dashboard (shard 4)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: getByText('EXECUTIVE_DASHBOARD_fee3a838')
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for getByText('EXECUTIVE_DASHBOARD_fee3a838')�[22m

Pages/DataContractInheritance.spec.ts › Partial Contract Inheritance - Asset contract merges with Data Product contract (shard 4)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: getByTestId('KnowledgePanel.DataProducts').getByTestId('data-products-list').getByTestId('data-product-"PW%dataProduct.f3914980"')
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('KnowledgePanel.DataProducts').getByTestId('data-products-list').getByTestId('data-product-"PW%dataProduct.f3914980"')�[22m

Pages/DataContractsSemanticRules.spec.ts › Validate DataProduct Rule Is (shard 4)
�[31mTest timeout of 180000ms exceeded.�[39m
Pages/DataContractsSemanticRules.spec.ts › Validate DataProduct Rule Is Not (shard 4)
�[31mTest timeout of 180000ms exceeded.�[39m
Pages/DataContractsSemanticRules.spec.ts › Validate DataProduct Rule Any_In (shard 4)
�[31mTest timeout of 180000ms exceeded.�[39m
Pages/DataContractsSemanticRules.spec.ts › Validate DataProduct Rule Not_In (shard 4)
�[31mTest timeout of 180000ms exceeded.�[39m
Pages/DataContractsSemanticRules.spec.ts › Validate DataProduct Rule Is_Set (shard 4)
�[31mTest timeout of 180000ms exceeded.�[39m
Pages/Domains.spec.ts › User with noDomain() rule cannot access tables without domain (shard 4)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoContainText�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed

Locator: getByTestId('permission-error-placeholder')
Expected substring: �[32m"You don't have necessary permissions. Please check with the admin to get the �[7mView Table Details�[27m permission."�[39m
Received string:    �[31m"You don't have necessary permissions. Please check with the admin to get the  permission."�[39m
Timeout: 15000ms

Call log:
�[2m  - Expect "toContainText" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('permission-error-placeholder')�[22m
�[2m    19 × locator resolved to <div data-testid="permission-error-placeholder" class="ant-space ant-space-vertical ant-space-align-center">…</div>�[22m
�[2m       - unexpected value "You don't have necessary permissions. Please check with the admin to get the  permission."�[22m

Pages/Entity.spec.ts › Domain Add, Update and Remove (shard 4)
�[31mTest timeout of 180000ms exceeded.�[39m
Pages/Entity.spec.ts › Tag Add, Update and Remove for child entities (shard 4)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: locator('[data-row-key="pw-database-service-e18ff314.pw-database-a1aacc56.pw-database-schema-4ce2548c.pw-table-274c84ce-4e56-43ff-b70e-5387d750a517.user_id222cda8b"]').getByTestId('tag-PersonalData.SpecialCategory')
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for locator('[data-row-key="pw-database-service-e18ff314.pw-database-a1aacc56.pw-database-schema-4ce2548c.pw-table-274c84ce-4e56-43ff-b70e-5387d750a517.user_id222cda8b"]').getByTestId('tag-PersonalData.SpecialCategory')�[22m

Pages/Entity.spec.ts › Tag and Glossary Term preservation in column detail panel (shard 4)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: locator('[data-row-key="pw-database-service-981dc0e9.pw-database-0774ed01.pw-database-schema-53f2e55a.pw-table-8f914ce9-753e-451e-b65e-cb4dd69f4e7b.user_idaf4165b5"]').getByTestId('tag-PII.Sensitive')
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for locator('[data-row-key="pw-database-service-981dc0e9.pw-database-0774ed01.pw-database-schema-53f2e55a.pw-table-8f914ce9-753e-451e-b65e-cb4dd69f4e7b.user_idaf4165b5"]').getByTestId('tag-PII.Sensitive')�[22m

🟡 17 flaky test(s) (passed on retry)
  • Features/ActivityAPI.spec.ts › Activity event shows the actor who made the change (shard 2, 1 retry)
  • Features/BulkEditEntity.spec.ts › Glossary (shard 2, 1 retry)
  • Features/DataQuality/TestCaseResultPermissions.spec.ts › User with only VIEW cannot PATCH results (shard 2, 1 retry)
  • Features/KnowledgeCenter.spec.ts › Article mentions in description should working for Knowledge Center (shard 2, 1 retry)
  • Features/KnowledgeCenterList.spec.ts › Knowledge Center List - Test add article button (shard 2, 1 retry)
  • Features/KnowledgeCenterTextEditor.spec.ts › Rich Text Editor - Text Formatting (shard 2, 1 retry)
  • Features/KnowledgeCenterTextEditor.spec.ts › Rich Text Editor - Text Formatting (shard 2, 1 retry)
  • Features/KnowledgeCenterTextEditor.spec.ts › Rich Text Editor - Text Formatting (shard 2, 1 retry)
  • Features/UserProfileOnlineStatus.spec.ts › Should show "Active recently" for users active within last hour (shard 3, 1 retry)
  • Flow/IngestionBot.spec.ts › Ingestion bot should be able to access domain specific domain (shard 3, 1 retry)
  • Flow/NotificationAlerts.spec.ts › Single Filter Alert (shard 3, 1 retry)
  • Pages/CustomProperties.spec.ts › Set & Update all CP types on dashboard (shard 4, 2 retries)
  • Pages/DataContracts.spec.ts › Contract Status badge should be visible on condition if Contract Tab is present/hidden by Persona (shard 4, 1 retry)
  • Pages/ExplorePageRightPanel_KnowledgeCenter.spec.ts › Should remove user owner for knowledgeCenter (shard 5, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (shard 6, 1 retry)
  • Pages/UserDetails.spec.ts › Create team with domain and verify visibility of inherited domain in user profile after team removal (shard 6, 1 retry)
  • Pages/Users.spec.ts › Create and Delete user (shard 6, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants