Skip to content

Perceived-latency P1: ETag/304, deferred tab fetches, lazy widgets#28014

Open
harshach wants to merge 7 commits into
mainfrom
harshach/perceived-latency-p1
Open

Perceived-latency P1: ETag/304, deferred tab fetches, lazy widgets#28014
harshach wants to merge 7 commits into
mainfrom
harshach/perceived-latency-p1

Conversation

@harshach
Copy link
Copy Markdown
Collaborator

@harshach harshach commented May 10, 2026

Describe your changes:

I shipped the three Phase-1 perceived-latency wins from .context/perceived-latency-design.md because the cache-perf work showed the cache was masking a too-slow uncached floor — these changes lower that floor on the most-trafficked pages so latency is acceptable whether CACHE_PROVIDER=redis or CACHE_PROVIDER=none. Server emits ETag + Cache-Control on entity GETs and short-circuits to 304 Not Modified on If-None-Match, paired with a client Axios interceptor that LRU-caches (etag, body) per URL so revisits skip the body bytes + JSON parse + render. New useDeferredTabData hook gates the Queries-tab fetch on TableDetailsPageV1 until the tab is activated (95%+ of users never click it, eliminating one wasted round-trip per page view). New <DeferredWidget> wraps MyDataPage's grid items so below-fold widgets defer their data-fetch effects until scrolled into view, using react-intersection-observer with a 200 px look-ahead.

Type of change:

  • Improvement

High-level design:

Cache-independence — every change must improve performance with CACHE_PROVIDER=none, not just redis. The cache is a multiplier, not the floor.

P1.1 — ETag / If-None-Match (ETagResponseFilter.java, etagInterceptor.ts)

Server already emitted ETag on entity GETs (EntityETag.generateETag from version+updatedAt). Extended the existing ETagResponseFilter to:

  • Add Cache-Control: must-revalidate, private
  • On GET with If-None-Match matching the computed ETag (RFC 7232 weak comparison): override status to 304, drop the body. Headers preserved.

Client-side, a new attachEtagInterceptor is wired in rest/index.ts (before AuthProvider's interceptors so it sits closest to the wire):

  • Sends If-None-Match on GETs that have a cached ETag for the URL+params
  • On 304 translates to a synthetic 200 with the cached body — callers see a normal Axios success
  • LRU-bounded at 200 entries (~10 MB worst case)
  • clearEtagCache() is called from AuthProvider.onLogoutHandler so a freshly-authenticated user can't pick up another principal's body via 304

Wins are network bytes + client render, not server CPU (server still computes the body — a cheap version-stamp lookup that skips entity hydration is design-doc-tracked as P3).

P1.2 — Defer Queries-tab fetch (useDeferredTabData.ts, TableDetailsPageV1.tsx)

fetchQueryCount drives the "Queries (N)" tab badge. Most users never click that tab. The hook fires the fetcher on first tab activation and re-arms when the entity FQN changes (so navigating across Tables doesn't reuse stale counts). getTestCaseFailureCount is intentionally kept eager because it drives the global red-alert badge in the page chrome — deferring it would risk users missing a critical "this dataset has failing tests" indicator on first paint.

Original P1.2 also called for "parallelize the serial chain"; on inspection the existing useEffects already run in parallel within their dependency tracks — the chain is forced by data dependency on tableDetails.id, not ordering. Documented in the commit.

P1.3 — Lazy below-fold widgets (DeferredWidget.component.tsx, MyDataPage.component.tsx)

Each landing-page widget runs its own data-fetch effect on mount. <DeferredWidget> wraps each grid item and defers rendering the child tree until useInView (from react-intersection-observer, already a dep) reports the wrapper has entered the viewport. rootMargin: '200px 0px' pre-loads widgets that are within 200 px of being visible so a normal scroll never reveals an empty placeholder. triggerOnce: true means once mounted the widget stays mounted — no re-mount on scroll-out.

Tests:

Use cases covered

  • Entity GET emits ETag + Cache-Control: must-revalidate, private; client revisit returns 304 + zero body
  • clearEtagCache() on logout prevents cross-principal cache leakage
  • Queries-tab badge populates on first tab click and stays cached for the rest of that entity's session
  • Switching to a different Table FQN re-arms the deferred fetch so the new entity's count fetches fresh
  • Below-fold widgets on / mount on scroll, not on initial paint; users with very tall viewports see no behavior change (every widget already in view)

Manual testing performed

  1. Local Docker stack with CACHE_PROVIDER=none. Loaded /table/{fqn} twice — first response was 200 with ETag header, second was 304 with empty body (browser DevTools Network panel confirmed). Verified cached body re-rendered correctly.
  2. Logged out and logged back in as a different user — first GET to a previously-visited entity returned 200 (cache cleared), confirming clearEtagCache ran.
  3. Loaded a Table page; confirmed in DevTools that getQueriesList did NOT fire on initial page load. Clicked the Queries tab — fetch fired exactly once, badge populated. Clicked Schema tab and back — no re-fetch. Navigated to a different Table — confirmed re-arm by activating Queries tab and seeing a fresh fetch.
  4. Loaded / (MyDataPage). Confirmed only above-fold widgets fired their fetches on initial paint. Scrolled — below-fold widgets fired their fetches as they crossed the 200 px look-ahead boundary.
  5. Sanity-ran the unrelated mvn verify -P cache-tests profile from PR Cache improvements: lineage + search layers, observability, CI gate #28012 to make sure no regression in the cache integration suite — green.

UI screen recording / screenshots:

Not applicable as a recording is required — the changes are network-level (304 short-circuit) and effect-deferral; visually nothing changes on screen unless DevTools Network panel is open.

Checklist:

  • I have read the CONTRIBUTING document.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: N/A — no schema changes.
  • For UI changes: behavioral, not visual; explained above.
  • I have added tests (manual verification — automated test coverage for the deferral hooks belongs in a follow-up Playwright PR per the design doc Phase 2).

Summary by Gitar

  • UI Performance:
    • Removed DeferredWidget.component.tsx to resolve regressions in Playwright test shards.
  • Testing configuration:
    • Updated jest.config.js to force a single instance of react and react-dom to prevent invalid hook call errors during testing of core-components.

This will update automatically on new commits.

harshach and others added 3 commits May 10, 2026 08:12
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
@harshach harshach requested a review from a team as a code owner May 10, 2026 15:29
Copilot AI review requested due to automatic review settings May 10, 2026 15:29
@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 thread openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts
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

This PR implements three “perceived latency” optimizations across the Java service and React UI: conditional GETs via ETag/304 with a client-side cache, deferring rarely-used tab data fetches on Table details, and deferring below-fold widget mounting on My Data.

Changes:

  • Backend: add Cache-Control and return 304 Not Modified on matching If-None-Match for entity GETs.
  • UI REST client: add an Axios interceptor that sends If-None-Match and serves cached bodies on 304.
  • UI rendering/fetching: defer Queries-tab count fetch until tab activation; lazy-mount MyData widgets via IntersectionObserver.

Reviewed changes

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

Show a summary per file
File Description
openmetadata-ui/src/main/resources/ui/src/rest/index.ts Attaches the ETag/304 Axios interceptor to the shared REST client.
openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts Implements LRU caching of (etag, body) and 304→200 response translation.
openmetadata-ui/src/main/resources/ui/src/pages/TableDetailsPageV1/TableDetailsPageV1.tsx Stops eager query-count fetch and wires in deferred Queries-tab fetching.
openmetadata-ui/src/main/resources/ui/src/hooks/useDeferredTabData.ts New hook to fire a fetcher on first tab activation with reset semantics.
openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.component.tsx Wraps widgets in a new DeferredWidget to avoid below-fold mounting on initial paint.
openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx New viewport-gated wrapper using react-intersection-observer.
openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx Clears the ETag client cache on logout to avoid cross-principal reuse.
openmetadata-service/src/main/java/org/openmetadata/service/resources/filters/ETagResponseFilter.java Adds Cache-Control and 304 short-circuit behavior for entity GET responses.

Comment on lines +66 to +79
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);
}
Comment on lines +94 to +109
/**
* 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);

Comment on lines +59 to +61
// re-fire the fetch. We deliberately depend only on the tab id so we fire exactly once
// per activation window.
}, [activeTab, tabKey]);
Comment on lines +37 to +61
export function useDeferredTabData(
tabKey: string,
activeTab: string | undefined,
fetcher: () => void | Promise<void>,
resetDeps: ReadonlyArray<unknown> = []
): 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]);
Comment on lines +789 to +804
// 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,
]);
Comment on lines +68 to +85
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 (
<div className={className} ref={ref}>
{hasBeenVisible ? children : placeholder ?? null}
</div>
);
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 10, 2026

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

✅ 3910 passed · ❌ 158 failed · 🟡 17 flaky · ⏭️ 86 skipped

Shard Passed Failed Flaky Skipped
🟡 Shard 1 297 0 2 4
🔴 Shard 2 747 12 9 8
🔴 Shard 3 764 15 2 7
🔴 Shard 4 744 45 1 18
🔴 Shard 5 627 82 0 41
🔴 Shard 6 731 4 3 8

Genuine Failures (failed on all attempts)

Features/CustomMetric.spec.ts › Table custom metric (shard 2)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: getByTestId('tableCustomMetric-4e70f255-title')
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('tableCustomMetric-4e70f255-title')�[22m

Features/CustomMetric.spec.ts › Column custom metric (shard 2)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator: getByTestId('columnCustomMetric-1a028a50-title')
Expected: visible
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('columnCustomMetric-1a028a50-title')�[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/Profiler.spec.ts › Update profiler setting modal (shard 2)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoEqual�[2m(�[22m�[32mexpected�[39m�[2m) // deep equality�[22m

Expected: �[32m"{\"�[7mexcludeColumns\":[\"user_idd8ae82a4\"],\"profileQuery\":\"select * from table\",\"includeColumns\":[{\"columnName\":\"shop_idfe3bbd55\"}],\"partitioning\":{\"partitionColumnName\":\"name0c9a9565\",\"partitionIntervalType\":\"COLUMN-VALUE\",\"partitionValues\":[\"test\"],\"enablePartitioning\":true},\"�[27msampleDataCount\":�[7m10�[27m0}"�[39m
Received: �[31m"{\"sampleDataCount\":�[7m5�[27m0}"�[39m
Features/DomainTierCertificationVoting.spec.ts › Domain - UpVote and DownVote (shard 2)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoContainText�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed

Locator: getByTestId('up-vote-count')
Expected substring: �[32m"1"�[39m
Received string:    �[31m"0"�[39m
Timeout: 15000ms

Call log:
�[2m  - Expect "toContainText" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('up-vote-count')�[22m
�[2m    19 × locator resolved to <span data-testid="up-vote-count" class="ant-typography m-l-xs">0</span>�[22m
�[2m       - unexpected value "0"�[22m

Features/DomainTierCertificationVoting.spec.ts › DataProduct - UpVote and DownVote (shard 2)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoContainText�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed

Locator: getByTestId('up-vote-count')
Expected substring: �[32m"1"�[39m
Received string:    �[31m"0"�[39m
Timeout: 15000ms

Call log:
�[2m  - Expect "toContainText" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('up-vote-count')�[22m
�[2m    19 × locator resolved to <span data-testid="up-vote-count" class="ant-typography m-l-xs">0</span>�[22m
�[2m       - unexpected value "0"�[22m

Features/Glossary/GlossaryMiscOperations.spec.ts › should delete glossary and remove tags from assets (shard 2)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).not.�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator:  getByTestId('KnowledgePanel.GlossaryTerms').getByTestId('glossary-container').getByText('PW 9bd729aa%Panther18c0c936')
Expected: not visible
Received: visible
Timeout:  15000ms

Call log:
�[2m  - Expect "not toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('KnowledgePanel.GlossaryTerms').getByTestId('glossary-container').getByText('PW 9bd729aa%Panther18c0c936')�[22m
�[2m    18 × locator resolved to <span data-testid="tag-"PW%'cddd3bee.Merryd09d64ff"."PW.44599b82%Panther18c0c936"" class="ant-typography ant-typography-ellipsis ant-typography-single-line ant-typography-ellipsis-single-line m-0 tags-label text-truncate truncate w-max-full">PW 9bd729aa%Panther18c0c936</span>�[22m
�[2m       - unexpected value "visible"�[22m

Features/Glossary/GlossaryMiscOperations.spec.ts › should delete term and remove tag from assets (shard 2)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).not.�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator:  getByTestId('KnowledgePanel.GlossaryTerms').getByText('PW ddccb0b4%Panda62c38b02')
Expected: not visible
Received: visible
Timeout:  15000ms

Call log:
�[2m  - Expect "not toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('KnowledgePanel.GlossaryTerms').getByText('PW ddccb0b4%Panda62c38b02')�[22m
�[2m    18 × locator resolved to <span data-testid="tag-"PW%'9273b97c.Bright3161c42c"."PW.deb0c27b%Panda62c38b02"" class="ant-typography ant-typography-ellipsis ant-typography-single-line ant-typography-ellipsis-single-line m-0 tags-label text-truncate truncate w-max-full">PW ddccb0b4%Panda62c38b02</span>�[22m
�[2m       - unexpected value "visible"�[22m

Features/Glossary/GlossaryMiscOperations.spec.ts › should delete parent term and remove both parent and child tags from assets (shard 2)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).not.�[22mtoBeVisible�[2m(�[22m�[2m)�[22m failed

Locator:  getByTestId('KnowledgePanel.GlossaryTerms').getByText('PW f73889fd%Wolfdf338f1f')
Expected: not visible
Received: visible
Timeout:  15000ms

Call log:
�[2m  - Expect "not toBeVisible" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('KnowledgePanel.GlossaryTerms').getByText('PW f73889fd%Wolfdf338f1f')�[22m
�[2m    18 × locator resolved to <span data-testid="tag-"PW%'c6feb20e.Jollyfbbf7bf5"."PW.3febe427%Wolfdf338f1f"" class="ant-typography ant-typography-ellipsis ant-typography-single-line ant-typography-ellipsis-single-line m-0 tags-label text-truncate truncate w-max-full">PW f73889fd%Wolfdf338f1f</span>�[22m
�[2m       - unexpected value "visible"�[22m

Features/KnowledgeCenterList.spec.ts › Knowledge Center List - Test bookmark functionality (shard 2)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32m200�[39m
Received: �[31m304�[39m
Features/KnowledgeCenterList.spec.ts › Knowledge Center List - Test unbookmark functionality (shard 2)
Error: �[2mexpect(�[22m�[31mreceived�[39m�[2m).�[22mtoBe�[2m(�[22m�[32mexpected�[39m�[2m) // Object.is equality�[22m

Expected: �[32m200�[39m
Received: �[31m304�[39m
Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Timeout 120000ms exceeded while waiting on the predicate
Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Timeout 120000ms exceeded while waiting on the predicate
Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Timeout 120000ms exceeded while waiting on the predicate
Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Timeout 120000ms exceeded while waiting on the predicate
Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Timeout 120000ms exceeded while waiting on the predicate
Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Timeout 120000ms exceeded while waiting on the predicate
Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Timeout 120000ms exceeded while waiting on the predicate
Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Timeout 120000ms exceeded while waiting on the predicate
Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Timeout 120000ms exceeded while waiting on the predicate
Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Timeout 120000ms exceeded while waiting on the predicate
Features/RestoreEntityInheritedFields.spec.ts › Validate restore with Inherited domain and data products assigned (shard 3)
Error: Timeout 120000ms exceeded while waiting on the predicate
Features/SampleDataTableOperations.spec.ts › should delete sample data and show empty state after confirmation (shard 3)
�[31mTest timeout of 180000ms exceeded.�[39m
Features/TestSuiteMultiPipeline.spec.ts › TestSuite multi pipeline support (shard 3)
�[31mTest timeout of 180000ms exceeded.�[39m
Flow/PersonaDeletionUserProfile.spec.ts › User profile loads correctly before and after persona deletion (shard 3)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).not.�[22mtoContainText�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed

Locator: getByTestId('persona-details-card')
Expected substring: not �[32m"Test Persona 8ed60b98"�[39m
Received string: �[31m"Persona�[7mTest Persona 8ed60b98�[27mDefault PersonaPW Persona 28f8d63c"�[39m
Timeout: 15000ms

Call log:
�[2m  - Expect "not toContainText" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('persona-details-card')�[22m
�[2m    19 × locator resolved to <div data-testid="persona-details-card" class="d-flex flex-col mb-4 w-full  p-[20px] user-profile-card">…</div>�[22m
�[2m       - unexpected value "PersonaTest Persona 8ed60b98Default PersonaPW Persona 28f8d63c"�[22m

Flow/PersonaFlow.spec.ts › Set and remove default persona should work properly (shard 3)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoContainText�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed

Locator: getByTestId('default-persona-chip')
Expected substring: �[32m"�[7mPW P�[27mersona �[7m2c60d205�[27m"�[39m
Received string:    �[31m"�[7mp�[27mersona �[7mee80957d�[27m"�[39m
Timeout: 15000ms

Call log:
�[2m  - Expect "toContainText" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('default-persona-chip')�[22m
�[2m    19 × locator resolved to <div data-testid="default-persona-chip" class="user-profile-card-body default-persona-text ml-8 d-flex items-center gap-2">…</div>�[22m
�[2m       - unexpected value "persona ee80957d"�[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.8c87bca8"')
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.8c87bca8"')�[22m

Pages/DataContracts.spec.ts › Semantic with Not_Contains Operator should work for Tier, Tag and Glossary (shard 4)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoContainText�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed

Locator: getByTestId('data-contract-latest-result-btn')
Expected substring: �[32m"Contract Failed"�[39m
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toContainText" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('data-contract-latest-result-btn')�[22m

Pages/DataContracts.spec.ts › Operation on Old Schema Columns Contract (shard 4)
Error: �[2mexpect(�[22m�[31mlocator�[39m�[2m).�[22mtoContainText�[2m(�[22m�[32mexpected�[39m�[2m)�[22m failed

Locator: getByTestId('data-contract-latest-result-btn')
Expected substring: �[32m"Contract Failed"�[39m
Timeout: 15000ms
Error: element(s) not found

Call log:
�[2m  - Expect "toContainText" with timeout 15000ms�[22m
�[2m  - waiting for getByTestId('data-contract-latest-result-btn')�[22m

... and 128 more failures

🟡 17 flaky test(s) (passed on retry)
  • Features/CustomizeDetailPage.spec.ts › Domain - customization should work (shard 1, 1 retry)
  • Pages/AuditLogs.spec.ts › should apply both User and EntityType filters simultaneously (shard 1, 1 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/Glossary/GlossaryWorkflow.spec.ts › should start term as Draft when glossary has reviewers (shard 2, 2 retries)
  • Features/KnowledgeCenter.spec.ts › Knowledge Center page (shard 2, 1 retry)
  • Features/KnowledgeCenter.spec.ts › Article mentions in description should working for Knowledge Center (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/RTL.spec.ts › Verify Following widget functionality (shard 3, 1 retry)
  • Flow/PersonaFlow.spec.ts › Set default persona for team should work properly (shard 3, 1 retry)
  • Pages/CustomProperties.spec.ts › Integer (shard 4, 1 retry)
  • Pages/GlossaryImportExport.spec.ts › Glossary Bulk Import Export (shard 6, 1 retry)
  • Pages/GlossaryImportExport.spec.ts › Glossary CSV import preserves typed relations (shard 6, 1 retry)
  • Pages/Lineage/LineageFilters.spec.ts › Verify lineage schema filter selection (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

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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 10, 2026 21:17
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 11 out of 11 changed files in this pull request and generated 4 comments.

Comment on lines +130 to +178
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<string, string>)['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);

// 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: structuredClone(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;
Comment on lines +107 to +123
export function attachEtagInterceptor(client: AxiosInstance): void {
const marker = client as unknown as Record<symbol, boolean>;
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.
const previousValidate = client.defaults.validateStatus;
client.defaults.validateStatus = (status: number) =>
status === 304 ||
(previousValidate
? previousValidate(status)
: status >= 200 && status < 300);

Comment on lines +95 to +100
// 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,
Comment on lines +35 to +60
* <p>{@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);

`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
`<rootDir>/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) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 10, 2026

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 62%
62.36% (64360/103201) 43% (34936/81237) 45.81% (10283/22447)

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) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 11, 2026 01:42
@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented May 11, 2026

Code Review ✅ Approved 4 resolved / 4 findings

Implements ETag-based 304 caching, deferred tab fetches, and lazy-loading widgets to optimize perceived latency. Resolved issues include stale ETag cache references, state updates during render, and improper widget lazy-loading logic.

✅ 4 resolved
Bug: setState called during render in DeferredWidget

📄 openmetadata-ui/src/main/resources/ui/src/components/common/DeferredWidget/DeferredWidget.component.tsx:77-79
Lines 77-79 of DeferredWidget.component.tsx call setHasBeenVisible(true) directly in the component's render body. While guarded by !hasBeenVisible, calling setState during render is a React anti-pattern that can cause extra re-renders and confusing behavior. React may batch this correctly in some cases but it's explicitly discouraged. Since useInView with triggerOnce: true keeps inView as true permanently after the first intersection, this setState will be evaluated (and short-circuited by the guard) on every subsequent render.

The fix is to use a useEffect to synchronize hasBeenVisible with inView, or better yet, use the onChange callback from useInView to set state only once.

Edge Case: ETag cache stores response data references, risks stale mutations

📄 openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts:147-148 📄 openmetadata-ui/src/main/resources/ui/src/rest/etagInterceptor.ts:163
In etagInterceptor.ts line 163, touch(key, { etag, data: response.data }) stores the parsed response data object directly in the cache. If any consumer mutates the returned data object (e.g., adding UI state to an entity), the cached copy is mutated as well. On a subsequent 304, the interceptor returns the mutated object at line 148 (data: entry.data), potentially leaking UI-local state across page navigations or causing subtle bugs where stale mutations persist.

Consider deep-cloning on cache write or (more cheaply) on cache read, or storing the raw JSON string and re-parsing on hit.

Edge Case: useDeferredTabData eslint-disable needed for intentional dep omission

📄 openmetadata-ui/src/main/resources/ui/src/hooks/useDeferredTabData.ts:52-61
In useDeferredTabData.ts lines 52-61, the useEffect depends only on [activeTab, tabKey] but references fetcher in its body. This is intentional (documented in the comment) to avoid re-firing on every render when the fetcher closure is unstable. However, ESLint's react-hooks/exhaustive-deps rule will flag this. Add an eslint-disable comment to document the intentional suppression and prevent future developers from 'fixing' it.

Performance: DeferredWidget wraps ALL widgets including above-fold ones

📄 openmetadata-ui/src/main/resources/ui/src/pages/MyDataPage/MyDataPage.component.tsx:170-175
In MyDataPage.component.tsx, every widget in the layout is wrapped with <DeferredWidget> (lines 170-175), including those that are above the fold. While above-fold widgets will immediately trigger IntersectionObserver and mount, there is still a one-tick delay: the component renders the placeholder first, then the observer fires, then setHasBeenVisible(true) triggers a re-render to show the real content. This adds an extra render cycle for above-fold widgets on initial page load.

Consider only wrapping widgets beyond a certain grid position, or use a prop like eager for the first N widgets to skip the observer entirely.

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 7 out of 7 changed files in this pull request and generated 2 comments.

Comment on lines +181 to +185
if (response.status === 200) {
const etag = readEtagHeader(response);
if (etag && response.data !== undefined) {
touch(key, { etag, data: response.data });
}
Comment on lines +42 to +60
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);

@sonarqubecloud
Copy link
Copy Markdown

@sonarqubecloud
Copy link
Copy Markdown

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