From 72516615198fadc387d1a585bbefc92c9b50e4c7 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Sun, 10 May 2026 08:12:59 -0700 Subject: [PATCH 1/7] =?UTF-8?q?feat(ui-perf):=20If-None-Match=20=E2=86=92?= =?UTF-8?q?=20304=20short-circuit=20+=20client=20ETag=20cache?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Server: ETagResponseFilter already emits ETag on entity GETs. Extended it to also (a) emit Cache-Control: must-revalidate, private and (b) short-circuit to 304 Not Modified with empty body when the request's If-None-Match matches the computed ETag (RFC 7232 weak comparison). Client: new attachEtagInterceptor() in rest/etagInterceptor.ts holds an LRU cache of (etag, response body) keyed by the canonical URL+params. On request, sends If-None-Match if a cached etag exists. On 304, returns the cached body as if it were a 200 — caller sees a normal Axios success with no awareness that no bytes crossed the wire. Cap of 200 entries keeps the cache bounded to ~10 MB worst case. clearEtagCache() is wired into AuthProvider.onLogoutHandler so a freshly-authenticated user can't pick up another principal's cached body via 304. Wins on revisits: zero body bytes on the wire, skip JSON parse, skip render. Server still computes the body (we'd need a cheap version-stamp lookup to truly skip the work — design doc tracks it as a follow-up). P1.1 of .context/perceived-latency-design.md. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../resources/filters/ETagResponseFilter.java | 65 ++++++- .../Auth/AuthProviders/AuthProvider.tsx | 5 + .../resources/ui/src/rest/etagInterceptor.ts | 183 ++++++++++++++++++ .../src/main/resources/ui/src/rest/index.ts | 7 + 4 files changed, 253 insertions(+), 7 deletions(-) create mode 100644 openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/filters/ETagResponseFilter.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/filters/ETagResponseFilter.java index c6d0805fbca7..40b49f18e986 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/filters/ETagResponseFilter.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/filters/ETagResponseFilter.java @@ -16,6 +16,7 @@ import jakarta.ws.rs.container.ContainerRequestContext; import jakarta.ws.rs.container.ContainerResponseContext; import jakarta.ws.rs.container.ContainerResponseFilter; +import jakarta.ws.rs.core.HttpHeaders; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.ext.Provider; import org.openmetadata.schema.EntityInterface; @@ -23,23 +24,73 @@ import org.openmetadata.service.util.EntityETag; /** - * JAX-RS filter that automatically adds ETag headers to GET responses - * containing EntityInterface entities. + * JAX-RS filter that adds {@code ETag} + {@code Cache-Control} headers to entity GET responses + * and short-circuits to {@code 304 Not Modified} when the client's {@code If-None-Match} matches + * the computed ETag. + * + *

The 304 path saves the response body bytes on the wire and the client-side render cost on + * revisits — the server still computes the entity body (we'd need a cheap version-stamp lookup + * to truly skip the work, see design doc), but the network and client savings are immediate. + * + *

{@code Cache-Control: must-revalidate, private}: clients (browsers, our Axios interceptor) + * may keep the body but must revalidate via {@code If-None-Match} before reusing it; private + * keeps it out of any shared/proxy cache so per-user data doesn't leak. */ @Provider public class ETagResponseFilter implements ContainerResponseFilter { + private static final String CACHE_CONTROL_VALUE = "must-revalidate, private"; + @Override public void filter( ContainerRequestContext requestContext, ContainerResponseContext responseContext) { try (var ignored = RequestLatencyContext.phase("etagGeneration")) { - if ("GET".equals(requestContext.getMethod()) - && responseContext.getStatus() == Response.Status.OK.getStatusCode() - && responseContext.getEntity() instanceof EntityInterface entity) { + if (!"GET".equals(requestContext.getMethod()) + || responseContext.getStatus() != Response.Status.OK.getStatusCode() + || !(responseContext.getEntity() instanceof EntityInterface entity)) { + return; + } + + String etag = EntityETag.generateETag(entity); + if (etag == null) { + return; + } + responseContext.getHeaders().putSingle(HttpHeaders.ETAG, etag); + responseContext.getHeaders().putSingle(HttpHeaders.CACHE_CONTROL, CACHE_CONTROL_VALUE); + + String ifNoneMatch = requestContext.getHeaderString(HttpHeaders.IF_NONE_MATCH); + if (ifNoneMatch == null) { + return; + } + if (matchesAny(ifNoneMatch, etag)) { + // RFC 7232: 304 must NOT include a message body. Drop the entity so the + // serializer emits an empty body. Headers (including ETag) are preserved. + responseContext.setStatus(Response.Status.NOT_MODIFIED.getStatusCode()); + responseContext.setEntity(null); + } + } + } - String etag = EntityETag.generateETag(entity); - responseContext.getHeaders().add("ETag", etag); + /** + * RFC 7232 §3.2: {@code If-None-Match} can be {@code *} (match any), a single ETag, or a + * comma-separated list. Weak comparison is used — we treat {@code "abc"} and {@code W/"abc"} + * as matching, which is the spec's recommendation for cache-validation use. + */ + private static boolean matchesAny(String ifNoneMatch, String currentEtag) { + String trimmed = ifNoneMatch.trim(); + if ("*".equals(trimmed)) { + return true; + } + String currentBare = stripWeakPrefix(currentEtag); + for (String candidate : trimmed.split(",")) { + if (currentBare.equals(stripWeakPrefix(candidate.trim()))) { + return true; } } + return false; + } + + private static String stripWeakPrefix(String etag) { + return etag.startsWith("W/") ? etag.substring(2) : etag; } } diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx index 74ecf1275491..f927e1d2a9da 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx @@ -54,6 +54,7 @@ import { withDomainFilter } from '../../../hoc/withDomainFilter'; import { useApplicationStore } from '../../../hooks/useApplicationStore'; import useCustomLocation from '../../../hooks/useCustomLocation/useCustomLocation'; import axiosClient from '../../../rest'; +import { clearEtagCache } from '../../../rest/etagInterceptor'; import { fetchAuthenticationConfig, fetchAuthorizerConfig, @@ -218,6 +219,10 @@ export const AuthProvider = ({ // Clear tokens properly during logout await clearOidcToken(); + // Drop the ETag interceptor's response cache so a freshly-authenticated user can't + // pick up another principal's cached body via If-None-Match → 304 mid-session. + clearEtagCache(); + setApplicationLoading(false); // Clear the refresh flag (used after refresh is complete) diff --git a/openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts b/openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts new file mode 100644 index 000000000000..90ad9a42c93b --- /dev/null +++ b/openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts @@ -0,0 +1,183 @@ +/* + * Copyright 2026 Collate. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { + AxiosHeaders, + AxiosInstance, + AxiosResponse, + InternalAxiosRequestConfig, +} from 'axios'; +import Qs from 'qs'; + +/** + * Client-side ETag / If-None-Match handling for entity GETs. + * + * Pairs with the server-side ETagResponseFilter which emits ETag + Cache-Control on entity + * GET responses and short-circuits to 304 when If-None-Match matches. The flow: + * + * 1. First GET to /tables/{fqn} → response with ETag header. We cache (etag, body) keyed + * by the canonical URL+params. + * 2. Second GET to the same URL → we attach If-None-Match. Server compares against the + * current ETag. + * - Match → 304 with empty body. Interceptor returns cached body as if it were 200. + * - No match → 200 with fresh body. Interceptor refreshes the cached entry. + * + * Wins: zero body bytes on the wire on revisit, plus skip JSON parse + render. + * + * Bounded memory: simple LRU cap at MAX_ENTRIES; oldest evicted on overflow. A typical entity + * GET response is 5-50 KB so the cap holds the cache to ~10 MB worst case. + */ + +interface CachedEntry { + etag: string; + data: unknown; +} + +const MAX_ENTRIES = 200; + +// Map preserves insertion order — re-set on hit to keep recently-used entries at the back. +const etagCache = new Map(); + +function buildKey(config: InternalAxiosRequestConfig): string { + const method = (config.method ?? 'get').toUpperCase(); + const url = config.url ?? ''; + const params = config.params + ? `?${Qs.stringify(config.params, { arrayFormat: 'comma' })}` + : ''; + + return `${method} ${url}${params}`; +} + +function touch(key: string, entry: CachedEntry): void { + etagCache.delete(key); + etagCache.set(key, entry); + + if (etagCache.size > MAX_ENTRIES) { + const oldest = etagCache.keys().next().value; + if (oldest !== undefined) { + etagCache.delete(oldest); + } + } +} + +function readEtagHeader(response: AxiosResponse): string | undefined { + const headers = response.headers; + if (!headers) { + return undefined; + } + + if (headers instanceof AxiosHeaders) { + const v = headers.get('etag'); + + return typeof v === 'string' ? v : undefined; + } + + const candidate = (headers as Record).etag; + if (typeof candidate === 'string') { + return candidate; + } + const candidateUpper = (headers as Record).ETag; + + return typeof candidateUpper === 'string' ? candidateUpper : undefined; +} + +/** + * Wire ETag handling into the axios client. Idempotent — calling twice is harmless because + * each call uses a fresh interceptor pair (callers that re-init axios should clear the cache + * via {@link clearEtagCache}). + */ +export function attachEtagInterceptor(client: AxiosInstance): void { + // Treat 304 as a success status so axios delivers it through the response interceptor + // instead of the error path. Without this, our 304-handling code would have to live in + // the error interceptor and intercepts on every error path. + const previousValidate = client.defaults.validateStatus; + client.defaults.validateStatus = (status: number) => + status === 304 || + (previousValidate + ? previousValidate(status) + : status >= 200 && status < 300); + + client.interceptors.request.use((config) => { + const method = (config.method ?? 'get').toLowerCase(); + if (method !== 'get') { + return config; + } + + const entry = etagCache.get(buildKey(config)); + if (!entry) { + return config; + } + + // Axios 1.x always populates config.headers with AxiosHeaders instance, but be + // defensive in case an upstream interceptor swapped it for a plain object. + if (config.headers instanceof AxiosHeaders) { + config.headers.set('If-None-Match', entry.etag); + } else if (config.headers) { + (config.headers as Record)['If-None-Match'] = entry.etag; + } else { + config.headers = new AxiosHeaders({ 'If-None-Match': entry.etag }); + } + + return config; + }); + + client.interceptors.response.use((response) => { + const method = (response.config.method ?? 'get').toLowerCase(); + if (method !== 'get') { + return response; + } + + const key = buildKey(response.config); + + if (response.status === 304) { + const entry = etagCache.get(key); + if (entry) { + touch(key, entry); + + return { + ...response, + status: 200, + statusText: 'OK (from ETag cache)', + data: entry.data, + }; + } + + // 304 without a cached body shouldn't happen in normal flow — a stale interceptor + // attaching If-None-Match for a key we no longer hold. Bubble through; the caller + // sees 304 and decides. Better than fabricating a fake 200. + return response; + } + + if (response.status === 200) { + const etag = readEtagHeader(response); + if (etag && response.data !== undefined) { + touch(key, { etag, data: response.data }); + } + } + + return response; + }); +} + +/** + * Drop every cached ETag entry. Call on logout / user switch so a freshly-authenticated user + * never receives another user's cached body via 304. + */ +export function clearEtagCache(): void { + etagCache.clear(); +} + +/** Test/debug helper. Returns the count of entries currently held. */ +export function etagCacheSize(): number { + return etagCache.size; +} diff --git a/openmetadata-ui/src/main/resources/ui/src/rest/index.ts b/openmetadata-ui/src/main/resources/ui/src/rest/index.ts index fd795dc40623..4110647d3577 100644 --- a/openmetadata-ui/src/main/resources/ui/src/rest/index.ts +++ b/openmetadata-ui/src/main/resources/ui/src/rest/index.ts @@ -14,10 +14,17 @@ import axios from 'axios'; import Qs from 'qs'; import { getBasePath } from '../utils/HistoryUtils'; +import { attachEtagInterceptor } from './etagInterceptor'; const axiosClient = axios.create({ baseURL: `${getBasePath()}/api/v1`, paramsSerializer: (params) => Qs.stringify(params, { arrayFormat: 'comma' }), }); +// Client-side If-None-Match support paired with the server's ETagResponseFilter. Saves the +// response body bytes + a JSON parse + a render on entity GET revisits within a session. +// Attached here (before AuthProvider's interceptors) so it sits closest to the wire and +// every other interceptor sees the resolved 304→200-with-cached-body translation. +attachEtagInterceptor(axiosClient); + export default axiosClient; From aed45a10b89deb5aa7df14a8eb5beac216c73544 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Sun, 10 May 2026 08:17:05 -0700 Subject: [PATCH 2/7] feat(ui-perf): defer Queries-tab fetch until tab activated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Most users never click into the Queries tab on a Table detail page, but TableDetailsPageV1 was firing getQueriesList() unconditionally on every page load to populate the "Queries (N)" badge. That's one wasted server round-trip per Table view for the ~95% of users who skip the tab. New useDeferredTabData hook gates a fetch on first tab activation — the badge populates when the user actually clicks into Queries, and re-arms when they navigate to a different Table FQN. Kept getTestCaseFailureCount eager: it drives the global red-alert badge in the page chrome, so deferring would mean a freshly-landed user could miss a critical "this dataset has failing tests" indicator. P1.2 of .context/perceived-latency-design.md. The "parallelize the serial chain" half of the original P1.2 was a no-op on inspection — the existing useEffects already run in parallel within their dep tracks; the chain is forced by data dependency (queryCount and DQ counts both need tableDetails.id), not by ordering choice. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../ui/src/hooks/useDeferredTabData.ts | 62 +++++++++++++++++++ .../TableDetailsPageV1/TableDetailsPageV1.tsx | 13 +++- 2 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 openmetadata-ui/src/main/resources/ui/src/hooks/useDeferredTabData.ts diff --git a/openmetadata-ui/src/main/resources/ui/src/hooks/useDeferredTabData.ts b/openmetadata-ui/src/main/resources/ui/src/hooks/useDeferredTabData.ts new file mode 100644 index 000000000000..5df7773881ee --- /dev/null +++ b/openmetadata-ui/src/main/resources/ui/src/hooks/useDeferredTabData.ts @@ -0,0 +1,62 @@ +/* + * Copyright 2026 Collate. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { useEffect, useRef } from 'react'; + +/** + * Fire {@code fetcher} the first time {@code activeTab} matches {@code tabKey}, and never + * again unless one of the {@code resetDeps} changes (in which case the hook arms itself for + * a fresh fetch on the next activation). + * + * Use case: tabs whose data drives a count badge (e.g. Queries (5), Tests (2)) AND whose + * fetch is independent of the entity-detail render. Most users never click these tabs, so + * eagerly fetching them on page load wastes a server round-trip per page view. Deferring + * the fetch until first activation moves the cost off the critical path. + * + * Caveat: the badge count won't appear before the user activates the tab. Render the tab + * label without the count until the fetch resolves; the count populates from the consumer's + * own state once the fetcher resolves. + * + * @param tabKey The tab id this hook is gated on (e.g. 'queries'). + * @param activeTab The currently-active tab id, typically from the URL or page state. + * @param fetcher Async function to run on first activation. Errors are swallowed by + * the caller's own try/catch — this hook just fires it. + * @param resetDeps Dependencies that should reset the "already fetched" flag. Typically + * includes the entity FQN so navigating to a different entity re-arms. + */ +export function useDeferredTabData( + tabKey: string, + activeTab: string | undefined, + fetcher: () => void | Promise, + resetDeps: ReadonlyArray = [] +): void { + const fetchedRef = useRef(false); + + // Reset the once-flag when any reset dep changes — typically when the user navigates to + // a different entity, even if the tab id is the same. The empty-deps default never + // re-arms; useful for ambient hooks that genuinely fire once. + useEffect(() => { + fetchedRef.current = false; + }, resetDeps); + + useEffect(() => { + if (activeTab !== tabKey || fetchedRef.current) { + return; + } + fetchedRef.current = true; + void fetcher(); + // The fetcher closure changes on every render in most callers — depending on it would + // re-fire the fetch. We deliberately depend only on the tab id so we fire exactly once + // per activation window. + }, [activeTab, tabKey]); +} diff --git a/openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx b/openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx index 966e5ead5f89..1190b0f3899e 100644 --- a/openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx @@ -56,6 +56,7 @@ import { TagLabel } from '../../generated/type/tagLabel'; import LimitWrapper from '../../hoc/LimitWrapper'; import { useApplicationStore } from '../../hooks/useApplicationStore'; import { useCustomPages } from '../../hooks/useCustomPages'; +import { useDeferredTabData } from '../../hooks/useDeferredTabData'; import { useFqn } from '../../hooks/useFqn'; import { useSub } from '../../hooks/usePubSub'; import { FeedCounts } from '../../interface/feed.interface'; @@ -785,13 +786,23 @@ const TableDetailsPageV1: React.FC = () => { } }, [tableFqn, isTourOpen, isTourPage, viewBasicPermission]); + // P1.2: getTestCaseFailureCount drives the global red-alert badge in the page chrome, + // so it must run as soon as tableDetails resolves — deferring would mean the user could + // miss a critical "this dataset has failing tests" indicator on first paint. useEffect(() => { if (tableDetails) { - fetchQueryCount(); getTestCaseFailureCount(); } }, [tableDetails?.fullyQualifiedName]); + // P1.2: queryCount only drives the "Queries (N)" tab badge — most users never click that + // tab, so eagerly fetching it on every page load wasted a server round-trip per view. + // Defer until the user actually activates the Queries tab (or any of its column-scoped + // sub-tabs); the badge then populates on first activation. + useDeferredTabData(EntityTabs.TABLE_QUERIES, activeTab, fetchQueryCount, [ + tableDetails?.fullyQualifiedName, + ]); + useSub( 'updateDetails', (suggestion: Suggestion) => { From 101a972f0c3d4dcd4b27b384baace8f8f3362312 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Sun, 10 May 2026 08:18:48 -0700 Subject: [PATCH 3/7] feat(ui-perf): lazy-mount below-fold widgets on landing page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit MyDataPage's grid renders every widget eagerly, and each widget runs its own data-fetch effect on mount — so eagerly mounting all of them on first paint pays for multiple below-fold network requests the user may never scroll to. New DeferredWidget wraps each grid item. It uses react-intersection-observer (already a dep) to defer rendering the child tree until the wrapper enters the viewport, with a 200px root-margin look-ahead so a normal scroll never reveals a placeholder. Once revealed, the widget stays mounted — no remount on scroll-out. Users with very tall screens see no behavior change (every widget is already in view on initial paint, so all mount immediately). Users on typical viewports save 2-4 widget worth of network and JS-render cost on the critical path. P1.3 of .context/perceived-latency-design.md. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../DeferredWidget.component.tsx | 88 +++++++++++++++++++ .../pages/MyDataPage/MyDataPage.component.tsx | 16 +++- 2 files changed, 100 insertions(+), 4 deletions(-) create mode 100644 openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx diff --git a/openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx b/openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx new file mode 100644 index 000000000000..fd258a8325b3 --- /dev/null +++ b/openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx @@ -0,0 +1,88 @@ +/* + * Copyright 2026 Collate. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ReactNode, useState } from 'react'; +import { useInView } from 'react-intersection-observer'; + +interface DeferredWidgetProps { + /** Content to render once the wrapper enters the viewport. */ + children: ReactNode; + + /** + * Placeholder shown while the wrapper is below the fold. Should reserve roughly the same + * height as the real widget so the page layout doesn't jump on reveal. Defaults to an + * invisible spacer — supply a skeleton if the widget is tall. + */ + placeholder?: ReactNode; + + /** + * IntersectionObserver root margin — how far ahead of the actual viewport edge to start + * loading. Default {@code "200px 0px"} pre-loads widgets that are within ~200px of being + * visible so users don't see placeholders flash during a normal scroll. + */ + rootMargin?: string; + + /** + * Threshold proportion of the wrapper that must be inside the viewport+rootMargin region + * before {@code inView} becomes true. {@code 0} fires as soon as a single pixel intersects + * — what we want for prefetch. + */ + threshold?: number; + + /** Optional class on the wrapper div — for layout grids that style by selector. */ + className?: string; +} + +/** + * Wraps a widget so its children only render once the wrapper enters the viewport (with a + * small look-ahead margin). Once revealed, it stays mounted — no remount on scroll-out. + * + * Use case: landing-page widgets that each fire their own data-fetch effect on mount. Eagerly + * mounting all of them on first paint pays for several below-fold fetches the user may never + * scroll to. Wrapping each in {@link DeferredWidget} keeps initial-paint network traffic + * proportional to what's actually visible. + * + * Caveat: if the user has very tall screens where the entire grid is above the fold, every + * widget mounts immediately and this is a no-op (correct behavior — no over-optimization for + * the rare-case). + */ +export const DeferredWidget = ({ + children, + placeholder, + rootMargin = '200px 0px', + threshold = 0, + className, +}: DeferredWidgetProps) => { + const [hasBeenVisible, setHasBeenVisible] = useState(false); + + const { ref, inView } = useInView({ + rootMargin, + threshold, + // Fire only the first crossing into view — once revealed, the widget mounts and the + // observer detaches. Re-scrolling above and back doesn't re-trigger because the child + // tree stays mounted (we drive that via {@link hasBeenVisible}). + triggerOnce: true, + }); + + if (inView && !hasBeenVisible) { + setHasBeenVisible(true); + } + + return ( +

+ {hasBeenVisible ? children : placeholder ?? null} +
+ ); +}; + +export default DeferredWidget; diff --git a/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.component.tsx b/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.component.tsx index cab5295a6956..b8d1724e6045 100644 --- a/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.component.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.component.tsx @@ -17,6 +17,7 @@ import { isEmpty } from 'lodash'; import { useCallback, useEffect, useMemo, useState } from 'react'; import RGL, { ReactGridLayoutProps, WidthProvider } from 'react-grid-layout'; import { useTranslation } from 'react-i18next'; +import DeferredWidget from '../../components/common/DeferredWidget/DeferredWidget.component'; import Loader from '../../components/common/Loader/Loader'; import { AdvanceSearchProvider } from '../../components/Explore/AdvanceSearchProvider/AdvanceSearchProvider.component'; import CustomiseLandingPageHeader from '../../components/MyData/CustomizableComponents/CustomiseLandingPageHeader/CustomiseLandingPageHeader'; @@ -160,11 +161,18 @@ const MyDataPage = () => { const widgets = useMemo( () => layout.map((widget) => ( + // P1.3: defer below-fold widget mounting until the user actually scrolls them into + // view. Each widget runs its own data-fetch effect on mount; eagerly mounting them + // all on first paint pays for several below-fold network requests the user may + // never scroll to. The 200px root margin pre-loads widgets that are about to enter + // view so a normal scroll never reveals an empty placeholder.
- {getWidgetFromKey({ - widgetConfig: widget, - currentLayout: layout, - })} + + {getWidgetFromKey({ + widgetConfig: widget, + currentLayout: layout, + })} +
)), [layout, isAnnouncementLoading, announcements] From f6654fca48238fa67f01f62a9e36262fdfbc2328 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Sun, 10 May 2026 12:49:02 -0700 Subject: [PATCH 4/7] fix(ui-perf): address PR review feedback for P1 perceived-latency MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review-driven fixes spanning all three P1 commits: DeferredWidget (lazy widget loader): - Move state update out of render — was calling `setHasBeenVisible(true)` in the component body guarded by `!hasBeenVisible`, which is a React anti-pattern (works, but trips warnings and triggers extra render cycles). Now drives state via `useInView`'s `onChange` callback so the update only fires once on the IntersectionObserver event. - Add `fallbackInView: true` so children render eagerly in environments where IntersectionObserver is unavailable (SSR, very old browsers, some Jest setups). - Add `initialInView` opt-out prop so callers (e.g. test wrappers, above-the-fold widgets) can skip the observer entirely. etagInterceptor (304 client-side cache): - Make `attachEtagInterceptor` properly idempotent. The previous "called twice is harmless" claim was wrong — each call stacked another interceptor pair and re-wrapped `validateStatus`. Now guarded by a symbol marker on the AxiosInstance so re-invocation (HMR, test bootstrap re-runs) is a true no-op. - Deep-clone cached body on read (304 hit path) via `structuredClone`. The cache stored a shared reference; consumers that mutated the entity (edit handlers, UI-local state mixing) would leak those mutations back into the cache and the next 304 would serve the mutated copy. useDeferredTabData (per-tab gated fetch): - When `resetDeps` change while the gated tab is already the active tab, fire the fetcher immediately rather than waiting for a tab toggle the user has no reason to make. Without this, navigating from table A → table B while staying on Queries left the badge showing A's count until the user clicked elsewhere and back. - Latest-fetcher captured via a ref so the reset effect always calls the up-to-date closure without re-firing on every render. TableDetailsPageV1: - Reset `queryCount` to 0 when `tableDetails?.fullyQualifiedName` changes. The badge previously kept showing the previous table's count until the deferred fetch (which also re-arms via the fix above) resolved for the new entity. Tested locally: `yarn build` green, eslint + prettier clean on all four touched files, tsc clean (the one pre-existing tsc error at TableDetailsPageV1:746 is unrelated — `findColumnByEntityLink` typing on a `string | undefined`). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../DeferredWidget.component.tsx | 36 +++++++++++++++---- .../ui/src/hooks/useDeferredTabData.ts | 30 ++++++++++++++-- .../TableDetailsPageV1/TableDetailsPageV1.tsx | 12 ++++++- .../resources/ui/src/rest/etagInterceptor.ts | 28 ++++++++++++--- 4 files changed, 91 insertions(+), 15 deletions(-) diff --git a/openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx b/openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx index fd258a8325b3..d65f9de0577f 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx @@ -11,7 +11,7 @@ * limitations under the License. */ -import { ReactNode, useState } from 'react'; +import { ReactNode, useCallback, useState } from 'react'; import { useInView } from 'react-intersection-observer'; interface DeferredWidgetProps { @@ -41,6 +41,15 @@ interface DeferredWidgetProps { /** Optional class on the wrapper div — for layout grids that style by selector. */ className?: string; + + /** + * Render children immediately, bypassing the IntersectionObserver wait. Use cases: + * - Tests where {@code IntersectionObserver} is mocked and never fires + * (the repo's Jest setup installs a no-op mock). + * - Known-above-fold widgets where the observer round-trip is wasted work. + * Defaults to {@code false} (production lazy behaviour). + */ + initialInView?: boolean; } /** @@ -62,22 +71,35 @@ export const DeferredWidget = ({ rootMargin = '200px 0px', threshold = 0, className, + initialInView = false, }: DeferredWidgetProps) => { - const [hasBeenVisible, setHasBeenVisible] = useState(false); + const [hasBeenVisible, setHasBeenVisible] = useState(initialInView); - const { ref, inView } = useInView({ + // Drive the state update through useInView's `onChange` callback rather than reading + // `inView` and calling setState during render. setState-in-render works because of the + // `!hasBeenVisible` guard but it's a React anti-pattern that can trigger extra render + // passes and dev warnings. + const handleChange = useCallback((visible: boolean) => { + if (visible) { + setHasBeenVisible(true); + } + }, []); + + const { ref } = useInView({ rootMargin, threshold, // Fire only the first crossing into view — once revealed, the widget mounts and the // observer detaches. Re-scrolling above and back doesn't re-trigger because the child // tree stays mounted (we drive that via {@link hasBeenVisible}). triggerOnce: true, + // When IntersectionObserver is unavailable (SSR, very old browsers, some test + // environments) treat the wrapper as "in view" so children render eagerly rather than + // staying invisible forever. The repo's Jest setup installs a no-op IO mock that never + // fires — combined with `initialInView` above, tests get sane defaults. + fallbackInView: true, + onChange: handleChange, }); - if (inView && !hasBeenVisible) { - setHasBeenVisible(true); - } - return (
{hasBeenVisible ? children : placeholder ?? null} diff --git a/openmetadata-ui/src/main/resources/ui/src/hooks/useDeferredTabData.ts b/openmetadata-ui/src/main/resources/ui/src/hooks/useDeferredTabData.ts index 5df7773881ee..23ca33311f8a 100644 --- a/openmetadata-ui/src/main/resources/ui/src/hooks/useDeferredTabData.ts +++ b/openmetadata-ui/src/main/resources/ui/src/hooks/useDeferredTabData.ts @@ -16,7 +16,8 @@ import { useEffect, useRef } from 'react'; /** * Fire {@code fetcher} the first time {@code activeTab} matches {@code tabKey}, and never * again unless one of the {@code resetDeps} changes (in which case the hook arms itself for - * a fresh fetch on the next activation). + * a fresh fetch on the next activation — or fires immediately if the gated tab is already + * the active one). * * Use case: tabs whose data drives a count badge (e.g. Queries (5), Tests (2)) AND whose * fetch is independent of the entity-detail render. Most users never click these tabs, so @@ -27,6 +28,12 @@ import { useEffect, useRef } from 'react'; * label without the count until the fetch resolves; the count populates from the consumer's * own state once the fetcher resolves. * + * Re-arm semantics: when any {@code resetDeps} entry changes (typically the entity FQN), the + * hook treats the situation as "new entity, stale data". If the user is already on the gated + * tab when that change happens, we fire {@code fetcher} immediately rather than waiting for + * a tab toggle the user has no reason to do — otherwise the badge would show the previous + * entity's count until something forced a re-activation. + * * @param tabKey The tab id this hook is gated on (e.g. 'queries'). * @param activeTab The currently-active tab id, typically from the URL or page state. * @param fetcher Async function to run on first activation. Errors are swallowed by @@ -41,12 +48,29 @@ export function useDeferredTabData( resetDeps: ReadonlyArray = [] ): void { const fetchedRef = useRef(false); + // Latest-fetcher ref so the resetDeps effect (which deliberately doesn't depend on + // {@code fetcher}) always calls the closure with up-to-date scope (e.g. tableFqn captured + // by the consumer). + const fetcherRef = useRef(fetcher); + fetcherRef.current = fetcher; // Reset the once-flag when any reset dep changes — typically when the user navigates to // a different entity, even if the tab id is the same. The empty-deps default never // re-arms; useful for ambient hooks that genuinely fire once. + // + // If the gated tab is the currently-active tab at reset time, fire immediately so the + // badge updates for the new entity without waiting for a tab toggle. We set the flag to + // true *before* firing so the activation effect below doesn't double-fire on the same + // render cycle. useEffect(() => { fetchedRef.current = false; + if (activeTab === tabKey) { + fetchedRef.current = true; + void fetcherRef.current(); + } + // `activeTab` and `tabKey` are read above but the intent is to fire only on resetDeps + // changes — including them in deps would cause every tab switch to also reset, which + // is the opposite of what we want. }, resetDeps); useEffect(() => { @@ -54,9 +78,9 @@ export function useDeferredTabData( return; } fetchedRef.current = true; - void fetcher(); + void fetcherRef.current(); // The fetcher closure changes on every render in most callers — depending on it would // re-fire the fetch. We deliberately depend only on the tab id so we fire exactly once - // per activation window. + // per activation window. `fetcherRef` keeps the latest closure available. }, [activeTab, tabKey]); } diff --git a/openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx b/openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx index 1190b0f3899e..2cec32ec47eb 100644 --- a/openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx @@ -798,11 +798,21 @@ const TableDetailsPageV1: React.FC = () => { // P1.2: queryCount only drives the "Queries (N)" tab badge — most users never click that // tab, so eagerly fetching it on every page load wasted a server round-trip per view. // Defer until the user actually activates the Queries tab (or any of its column-scoped - // sub-tabs); the badge then populates on first activation. + // sub-tabs); the badge then populates on first activation. {@link useDeferredTabData} + // also re-fires on FQN change if the user is already on the Queries tab, so badge counts + // never show stale data from a previous entity. useDeferredTabData(EntityTabs.TABLE_QUERIES, activeTab, fetchQueryCount, [ tableDetails?.fullyQualifiedName, ]); + // Reset the badge count to 0 when navigating to a different entity. Without this the + // badge would show the previous table's queryCount until the deferred fetch resolves, + // which is briefly misleading when navigating between tables that have differing query + // counts. + useEffect(() => { + setQueryCount(0); + }, [tableDetails?.fullyQualifiedName]); + useSub( 'updateDetails', (suggestion: Suggestion) => { diff --git a/openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts b/openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts index 90ad9a42c93b..e6bb48d349a2 100644 --- a/openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts +++ b/openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts @@ -91,12 +91,26 @@ function readEtagHeader(response: AxiosResponse): string | undefined { return typeof candidateUpper === 'string' ? candidateUpper : undefined; } +// Marker we stamp on an AxiosInstance once we've installed our interceptor pair. Lets the +// function be properly idempotent — re-invocation (HMR, test setup re-runs, callers that +// accidentally re-init) is a no-op rather than stacking another interceptor pair plus another +// `validateStatus` override on top of itself. +const ETAG_INTERCEPTOR_INSTALLED = Symbol.for( + '@openmetadata/etag-interceptor-installed' +); + /** - * Wire ETag handling into the axios client. Idempotent — calling twice is harmless because - * each call uses a fresh interceptor pair (callers that re-init axios should clear the cache - * via {@link clearEtagCache}). + * Wire ETag handling into the axios client. Idempotent — calling twice on the same client is + * a no-op (guarded via a symbol marker on the instance). Callers that re-init axios from + * scratch should also clear the cache via {@link clearEtagCache}. */ export function attachEtagInterceptor(client: AxiosInstance): void { + const marker = client as unknown as Record; + if (marker[ETAG_INTERCEPTOR_INSTALLED]) { + return; + } + marker[ETAG_INTERCEPTOR_INSTALLED] = true; + // Treat 304 as a success status so axios delivers it through the response interceptor // instead of the error path. Without this, our 304-handling code would have to live in // the error interceptor and intercepts on every error path. @@ -144,11 +158,17 @@ export function attachEtagInterceptor(client: AxiosInstance): void { if (entry) { touch(key, entry); + // Deep-clone the cached body before handing it back. Consumers (UI components, + // utilities, edit handlers) routinely mutate the entity object they receive — adding + // local UI state, normalising fields, stripping properties — and a shared reference + // would let those mutations leak back into the cache. The next 304 would then return + // the mutated copy and cross-page bugs become very hard to track. structuredClone is + // available in all supported browsers (Chrome 98+, Firefox 94+, Safari 15.4+). return { ...response, status: 200, statusText: 'OK (from ETag cache)', - data: entry.data, + data: structuredClone(entry.data), }; } From 5ab5276648f192b4269c580e8007ca071a6bd18f Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Sun, 10 May 2026 14:17:17 -0700 Subject: [PATCH 5/7] test(ui-perf): unblock tests against DeferredWidget on landing page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1.3 wrapped landing-page widgets in DeferredWidget so below-fold widgets mount only after entering the viewport. Existing tests assumed eager mount and broke: Jest (MyDataPage.test.tsx): jsdom's IntersectionObserver mock is a no-op jest.fn() that never fires, so `useInView` keeps `inView` false forever and children never render. The 4 `findByText('KnowledgePanel.*')` assertions in the test couldn't find widgets the wrapper was holding back. Fix: mock `DeferredWidget` to render children directly — same pattern as the existing LimitWrapper / DataInsightProvider mocks. Playwright (CustomizeLandingPage.spec.ts): real browser, real IO — but the 1280x720 CI viewport leaves widgets like `KnowledgePanel.KPI` below the fold. Without a scroll, those widgets are never observed, never mount, and `toBeVisible()` resolves to false. Fix: call `scrollIntoViewIfNeeded()` before each `toBeVisible` assertion in `checkAllDefaultWidgets`, plus the two inline assertions in "Add, Remove and Reset widget" and "Widget drag and drop reordering". Existing `not.toBeVisible()` checks stay as-is — a placeholder div without children is correctly "not visible", so the deferred-mount behaviour matches the assertion intent. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../e2e/Flow/CustomizeLandingPage.spec.ts | 20 ++++++++++++--- .../playwright/utils/customizeLandingPage.ts | 25 ++++++++++++++----- .../src/pages/MyDataPage/MyDataPage.test.tsx | 12 +++++++++ 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/CustomizeLandingPage.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/CustomizeLandingPage.spec.ts index f0d7ac3483ac..f8455eb0ee17 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/CustomizeLandingPage.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/CustomizeLandingPage.spec.ts @@ -213,10 +213,13 @@ test.describe( adminPage.getByTestId('KnowledgePanel.KPI') ).not.toBeVisible(); - // Check if newly added widgets are present on the landing page - await expect( - adminPage.getByTestId('KnowledgePanel.Following') - ).toBeVisible(); + // DeferredWidget only mounts widgets once they enter the viewport — scroll the + // newly-added widget into view before asserting visibility. + const followingWidget = adminPage.getByTestId( + 'KnowledgePanel.Following' + ); + await followingWidget.scrollIntoViewIfNeeded(); + await expect(followingWidget).toBeVisible(); }); await test.step('Resetting the layout flow should work properly', async () => { @@ -299,6 +302,15 @@ test.describe( await removeLandingBanner(adminPage); await waitForAllLoadersToDisappear(adminPage).catch(() => undefined); + // DeferredWidget only mounts widgets once they enter the viewport — scroll each + // into view so the assertion checks the mounted widget rather than the placeholder. + await adminPage + .getByTestId('KnowledgePanel.MyData') + .scrollIntoViewIfNeeded(); + await adminPage + .getByTestId('KnowledgePanel.Following') + .scrollIntoViewIfNeeded(); + await expect .poll( async () => ({ diff --git a/openmetadata-ui/src/main/resources/ui/playwright/utils/customizeLandingPage.ts b/openmetadata-ui/src/main/resources/ui/playwright/utils/customizeLandingPage.ts index e8cd14664f57..3061166d5517 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/utils/customizeLandingPage.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/utils/customizeLandingPage.ts @@ -173,12 +173,25 @@ export const checkAllDefaultWidgets = async (page: Page) => { await waitForAllLoadersToDisappear(page, 'entity-list-skeleton'); await expect(page.getByTestId('page-layout-v1')).toBeVisible(); - await expect(page.getByTestId('KnowledgePanel.ActivityFeed')).toBeVisible(); - await expect(page.getByTestId('KnowledgePanel.Following')).toBeVisible(); - await expect(page.getByTestId('KnowledgePanel.DataAssets')).toBeVisible(); - await expect(page.getByTestId('KnowledgePanel.MyData')).toBeVisible(); - await expect(page.getByTestId('KnowledgePanel.KPI')).toBeVisible(); - await expect(page.getByTestId('KnowledgePanel.TotalAssets')).toBeVisible(); + + // Widgets below the fold are wrapped in DeferredWidget — IntersectionObserver only mounts + // them once they enter the viewport. Scroll each into view so the test simulates a normal + // user scrolling to inspect the full layout, otherwise below-fold widgets stay placeholders + // and toBeVisible() fails on an element that never rendered. + const widgetIds = [ + 'KnowledgePanel.ActivityFeed', + 'KnowledgePanel.Following', + 'KnowledgePanel.DataAssets', + 'KnowledgePanel.MyData', + 'KnowledgePanel.KPI', + 'KnowledgePanel.TotalAssets', + ]; + + for (const widgetId of widgetIds) { + const widget = page.getByTestId(widgetId); + await widget.scrollIntoViewIfNeeded(); + await expect(widget).toBeVisible(); + } }; export const setUserDefaultPersona = async ( diff --git a/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.test.tsx b/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.test.tsx index 18c5f9519878..38d3a300c1bb 100644 --- a/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.test.tsx @@ -144,6 +144,18 @@ jest.mock('../../hoc/LimitWrapper', () => { .mockImplementation(({ children }) => <>LimitWrapper{children}); }); +jest.mock( + '../../components/common/DeferredWidget/DeferredWidget.component', + () => ({ + __esModule: true, + default: jest + .fn() + .mockImplementation(({ children }: { children: React.ReactNode }) => ( + <>{children} + )), + }) +); + jest.mock('../DataInsightPage/DataInsightProvider', async () => { return jest.fn().mockImplementation(({ children }) => <>{children}); }); From 3782a0cc626f80b380bfc30112dbdeb34a3590cd Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Sun, 10 May 2026 14:45:44 -0700 Subject: [PATCH 6/7] test(ui): force single React copy for core-components in jest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FailedTestCaseSampleData.test.tsx` (and any other test that mounts a core-components component) fails with: TypeError: Cannot read properties of null (reading 'useContext') at openmetadata-ui-core-components/.../node_modules/react/cjs/react.development.js:1618 at ... openmetadata-ui-core-components/.../dist/SnackbarContent.cjs.js The error is the classic dual-React-instance — the second React (with a null hooks dispatcher) lives at `openmetadata-ui-core-components/src/main/resources/ui/node_modules/react/`, which the core-components package installs for its own dev/test. When the consumer (`openmetadata-ui`) loads the CJS bundle from `dist/*.cjs.js`, Node's `require('react')` resolution walks up from the bundle file and finds the core-components-local React first, not the consumer's copy. Add explicit `moduleNameMapper` entries so every `require('react')` and `require('react-dom')` (and submodules) resolves to the consumer's `/node_modules/react[-dom]`. Single React instance, shared hooks dispatcher, no more null-useContext crash. Verified: failing test now passes, and a sampling of 22 unrelated suites (304 tests across Loader, Lineage, IncidentManager, AlertsListing, etc.) all stay green. Co-Authored-By: Claude Opus 4.7 (1M context) --- openmetadata-ui/src/main/resources/ui/jest.config.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/openmetadata-ui/src/main/resources/ui/jest.config.js b/openmetadata-ui/src/main/resources/ui/jest.config.js index f9b94ac41c31..a0e1f01a2149 100644 --- a/openmetadata-ui/src/main/resources/ui/jest.config.js +++ b/openmetadata-ui/src/main/resources/ui/jest.config.js @@ -75,6 +75,16 @@ module.exports = { '/src/test/unit/mocks/reactColumnResize.mock.js', '^.*/Lineage/Layout/ELKUtil/ELKUtil$': '/src/test/unit/mocks/elkLayout.mock.js', + // Force every `require('react')` / `require('react-dom')` to resolve to the consumer's + // copy. The `openmetadata-ui-core-components` package has its own `node_modules/react` + // (for its own dev/test) — without these mappings the CJS bundle loaded from + // `dist/*.cjs.js` resolves React from the core-components tree, producing a second React + // instance with a null hooks dispatcher and the classic "Invalid hook call ... reading + // 'useContext'" TypeError. + '^react$': '/node_modules/react', + '^react-dom$': '/node_modules/react-dom', + '^react/(.*)$': '/node_modules/react/$1', + '^react-dom/(.*)$': '/node_modules/react-dom/$1', }, transformIgnorePatterns: [ 'node_modules/(?!(@azure/msal-react|react-dnd|react-dnd-html5-backend|dnd-core|@react-dnd/invariant|@react-dnd/asap|@react-dnd/shallowequal|@melloware/react-logviewer|@material/material-color-utilities|@openmetadata/ui-core-components|nanoid|@rjsf/core|@rjsf/utils|@rjsf/validator-ajv8|uuid|elkjs))', From c515580468ca4c82a030098595357a6368070e62 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Sun, 10 May 2026 18:41:55 -0700 Subject: [PATCH 7/7] =?UTF-8?q?revert(ui-perf):=20drop=20P1.3=20DeferredWi?= =?UTF-8?q?dget=20=E2=80=94=20broke=20Playwright=20shards?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DeferredWidget kept its children inside an unkeyed wrapper with no data-testid and no min-height. On the home page, below-fold widgets (KnowledgePanel.KPI etc.) never entered the viewport in a 1280x720 Playwright browser, so: - The wrapper div with `ref` was 0-height and never crossed the IntersectionObserver threshold. - `page.getByTestId('KnowledgePanel.KPI')` matched nothing because the testid lives on the *child* widget that hadn't mounted yet. - `scrollIntoViewIfNeeded()` from my previous fix attempt waited forever for that non-existent element — hanging every shard for 27 minutes before the test timeout fired. All 6 Playwright shards were failing as a result. The earlier scroll-based mitigation was strictly worse than the original code: it turned a 60s timeout into a 27-minute hang. Reverting the DeferredWidget integration on MyDataPage. The component itself, its only consumer, the Jest mock, and the playwright workarounds are all removed. P1.1 (ETag/304) and P1.2 (defer Queries-tab) stand — those are clean wins. Lazy widget mounting can come back later with proper testid-on-wrapper + min-height semantics so tests can find the slot before its contents mount. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../e2e/Flow/CustomizeLandingPage.spec.ts | 20 +--- .../playwright/utils/customizeLandingPage.ts | 25 +--- .../DeferredWidget.component.tsx | 110 ------------------ .../pages/MyDataPage/MyDataPage.component.tsx | 16 +-- .../src/pages/MyDataPage/MyDataPage.test.tsx | 12 -- 5 files changed, 14 insertions(+), 169 deletions(-) delete mode 100644 openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx diff --git a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/CustomizeLandingPage.spec.ts b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/CustomizeLandingPage.spec.ts index f8455eb0ee17..f0d7ac3483ac 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/CustomizeLandingPage.spec.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/e2e/Flow/CustomizeLandingPage.spec.ts @@ -213,13 +213,10 @@ test.describe( adminPage.getByTestId('KnowledgePanel.KPI') ).not.toBeVisible(); - // DeferredWidget only mounts widgets once they enter the viewport — scroll the - // newly-added widget into view before asserting visibility. - const followingWidget = adminPage.getByTestId( - 'KnowledgePanel.Following' - ); - await followingWidget.scrollIntoViewIfNeeded(); - await expect(followingWidget).toBeVisible(); + // Check if newly added widgets are present on the landing page + await expect( + adminPage.getByTestId('KnowledgePanel.Following') + ).toBeVisible(); }); await test.step('Resetting the layout flow should work properly', async () => { @@ -302,15 +299,6 @@ test.describe( await removeLandingBanner(adminPage); await waitForAllLoadersToDisappear(adminPage).catch(() => undefined); - // DeferredWidget only mounts widgets once they enter the viewport — scroll each - // into view so the assertion checks the mounted widget rather than the placeholder. - await adminPage - .getByTestId('KnowledgePanel.MyData') - .scrollIntoViewIfNeeded(); - await adminPage - .getByTestId('KnowledgePanel.Following') - .scrollIntoViewIfNeeded(); - await expect .poll( async () => ({ diff --git a/openmetadata-ui/src/main/resources/ui/playwright/utils/customizeLandingPage.ts b/openmetadata-ui/src/main/resources/ui/playwright/utils/customizeLandingPage.ts index 3061166d5517..e8cd14664f57 100644 --- a/openmetadata-ui/src/main/resources/ui/playwright/utils/customizeLandingPage.ts +++ b/openmetadata-ui/src/main/resources/ui/playwright/utils/customizeLandingPage.ts @@ -173,25 +173,12 @@ export const checkAllDefaultWidgets = async (page: Page) => { await waitForAllLoadersToDisappear(page, 'entity-list-skeleton'); await expect(page.getByTestId('page-layout-v1')).toBeVisible(); - - // Widgets below the fold are wrapped in DeferredWidget — IntersectionObserver only mounts - // them once they enter the viewport. Scroll each into view so the test simulates a normal - // user scrolling to inspect the full layout, otherwise below-fold widgets stay placeholders - // and toBeVisible() fails on an element that never rendered. - const widgetIds = [ - 'KnowledgePanel.ActivityFeed', - 'KnowledgePanel.Following', - 'KnowledgePanel.DataAssets', - 'KnowledgePanel.MyData', - 'KnowledgePanel.KPI', - 'KnowledgePanel.TotalAssets', - ]; - - for (const widgetId of widgetIds) { - const widget = page.getByTestId(widgetId); - await widget.scrollIntoViewIfNeeded(); - await expect(widget).toBeVisible(); - } + await expect(page.getByTestId('KnowledgePanel.ActivityFeed')).toBeVisible(); + await expect(page.getByTestId('KnowledgePanel.Following')).toBeVisible(); + await expect(page.getByTestId('KnowledgePanel.DataAssets')).toBeVisible(); + await expect(page.getByTestId('KnowledgePanel.MyData')).toBeVisible(); + await expect(page.getByTestId('KnowledgePanel.KPI')).toBeVisible(); + await expect(page.getByTestId('KnowledgePanel.TotalAssets')).toBeVisible(); }; export const setUserDefaultPersona = async ( diff --git a/openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx b/openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx deleted file mode 100644 index d65f9de0577f..000000000000 --- a/openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx +++ /dev/null @@ -1,110 +0,0 @@ -/* - * Copyright 2026 Collate. - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * http://www.apache.org/licenses/LICENSE-2.0 - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -import { ReactNode, useCallback, useState } from 'react'; -import { useInView } from 'react-intersection-observer'; - -interface DeferredWidgetProps { - /** Content to render once the wrapper enters the viewport. */ - children: ReactNode; - - /** - * Placeholder shown while the wrapper is below the fold. Should reserve roughly the same - * height as the real widget so the page layout doesn't jump on reveal. Defaults to an - * invisible spacer — supply a skeleton if the widget is tall. - */ - placeholder?: ReactNode; - - /** - * IntersectionObserver root margin — how far ahead of the actual viewport edge to start - * loading. Default {@code "200px 0px"} pre-loads widgets that are within ~200px of being - * visible so users don't see placeholders flash during a normal scroll. - */ - rootMargin?: string; - - /** - * Threshold proportion of the wrapper that must be inside the viewport+rootMargin region - * before {@code inView} becomes true. {@code 0} fires as soon as a single pixel intersects - * — what we want for prefetch. - */ - threshold?: number; - - /** Optional class on the wrapper div — for layout grids that style by selector. */ - className?: string; - - /** - * Render children immediately, bypassing the IntersectionObserver wait. Use cases: - * - Tests where {@code IntersectionObserver} is mocked and never fires - * (the repo's Jest setup installs a no-op mock). - * - Known-above-fold widgets where the observer round-trip is wasted work. - * Defaults to {@code false} (production lazy behaviour). - */ - initialInView?: boolean; -} - -/** - * Wraps a widget so its children only render once the wrapper enters the viewport (with a - * small look-ahead margin). Once revealed, it stays mounted — no remount on scroll-out. - * - * Use case: landing-page widgets that each fire their own data-fetch effect on mount. Eagerly - * mounting all of them on first paint pays for several below-fold fetches the user may never - * scroll to. Wrapping each in {@link DeferredWidget} keeps initial-paint network traffic - * proportional to what's actually visible. - * - * Caveat: if the user has very tall screens where the entire grid is above the fold, every - * widget mounts immediately and this is a no-op (correct behavior — no over-optimization for - * the rare-case). - */ -export const DeferredWidget = ({ - children, - placeholder, - rootMargin = '200px 0px', - threshold = 0, - className, - initialInView = false, -}: DeferredWidgetProps) => { - const [hasBeenVisible, setHasBeenVisible] = useState(initialInView); - - // Drive the state update through useInView's `onChange` callback rather than reading - // `inView` and calling setState during render. setState-in-render works because of the - // `!hasBeenVisible` guard but it's a React anti-pattern that can trigger extra render - // passes and dev warnings. - const handleChange = useCallback((visible: boolean) => { - if (visible) { - setHasBeenVisible(true); - } - }, []); - - const { ref } = useInView({ - rootMargin, - threshold, - // Fire only the first crossing into view — once revealed, the widget mounts and the - // observer detaches. Re-scrolling above and back doesn't re-trigger because the child - // tree stays mounted (we drive that via {@link hasBeenVisible}). - triggerOnce: true, - // When IntersectionObserver is unavailable (SSR, very old browsers, some test - // environments) treat the wrapper as "in view" so children render eagerly rather than - // staying invisible forever. The repo's Jest setup installs a no-op IO mock that never - // fires — combined with `initialInView` above, tests get sane defaults. - fallbackInView: true, - onChange: handleChange, - }); - - return ( -
- {hasBeenVisible ? children : placeholder ?? null} -
- ); -}; - -export default DeferredWidget; diff --git a/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.component.tsx b/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.component.tsx index b8d1724e6045..cab5295a6956 100644 --- a/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.component.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.component.tsx @@ -17,7 +17,6 @@ import { isEmpty } from 'lodash'; import { useCallback, useEffect, useMemo, useState } from 'react'; import RGL, { ReactGridLayoutProps, WidthProvider } from 'react-grid-layout'; import { useTranslation } from 'react-i18next'; -import DeferredWidget from '../../components/common/DeferredWidget/DeferredWidget.component'; import Loader from '../../components/common/Loader/Loader'; import { AdvanceSearchProvider } from '../../components/Explore/AdvanceSearchProvider/AdvanceSearchProvider.component'; import CustomiseLandingPageHeader from '../../components/MyData/CustomizableComponents/CustomiseLandingPageHeader/CustomiseLandingPageHeader'; @@ -161,18 +160,11 @@ const MyDataPage = () => { const widgets = useMemo( () => layout.map((widget) => ( - // P1.3: defer below-fold widget mounting until the user actually scrolls them into - // view. Each widget runs its own data-fetch effect on mount; eagerly mounting them - // all on first paint pays for several below-fold network requests the user may - // never scroll to. The 200px root margin pre-loads widgets that are about to enter - // view so a normal scroll never reveals an empty placeholder.
- - {getWidgetFromKey({ - widgetConfig: widget, - currentLayout: layout, - })} - + {getWidgetFromKey({ + widgetConfig: widget, + currentLayout: layout, + })}
)), [layout, isAnnouncementLoading, announcements] diff --git a/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.test.tsx b/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.test.tsx index 38d3a300c1bb..18c5f9519878 100644 --- a/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.test.tsx @@ -144,18 +144,6 @@ jest.mock('../../hoc/LimitWrapper', () => { .mockImplementation(({ children }) => <>LimitWrapper{children}); }); -jest.mock( - '../../components/common/DeferredWidget/DeferredWidget.component', - () => ({ - __esModule: true, - default: jest - .fn() - .mockImplementation(({ children }: { children: React.ReactNode }) => ( - <>{children} - )), - }) -); - jest.mock('../DataInsightPage/DataInsightProvider', async () => { return jest.fn().mockImplementation(({ children }) => <>{children}); });