Skip to content

Fix UI Suspense loader fallbacks#29746

Open
shah-harshit wants to merge 12 commits into
mainfrom
codex/fix-ui-suspense-loaders
Open

Fix UI Suspense loader fallbacks#29746
shah-harshit wants to merge 12 commits into
mainfrom
codex/fix-ui-suspense-loaders

Conversation

@shah-harshit

@shah-harshit shah-harshit commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Describe your changes:

Fixes excessive loading indicators caused by lazy chunk fallbacks on already-rendered UI.

  • Default shared Suspense wrapper fallback to null so background lazy chunks do not show page-level loaders.
  • Add LazyTabContent to keep inactive entity tabs unmounted while preserving one active-tab loader.
  • Apply active-tab lazy handling across lineage and knowledge graph tabs for entity detail pages.
  • Remove redundant secondary loaders from activity feed, landing widgets, and service form/doc lazy sections.

Type of change:

  • Bug fix

High-level design:

Use a silent default Suspense fallback for generic lazy components, and opt into a visible loader only where the active content is actually waiting on a chunk. Entity detail lineage and knowledge graph tabs use LazyTabContent so inactive tabs do not render fallback UI.

Tests:

Use cases covered

  • Entity detail tabs do not show loaders for inactive lazy tab content.
  • Active lineage and knowledge graph tabs still show a contained loader while loading their chunk.
  • Service add/edit forms and service doc panels do not show extra Suspense loaders while the main page is already visible.
  • Activity feed no longer renders duplicate follow-up loaders in the center and right panels.

Unit tests

  • I added unit tests for the new/changed logic.
  • Files added/updated:
    • src/components/AppRouter/withSuspenseFallback.test.tsx
    • src/components/common/LazyTabContent/LazyTabContent.test.tsx

Backend integration tests

  • Not applicable (no backend API changes).

Ingestion integration tests

  • Not applicable (no ingestion changes).

Playwright (UI) tests

  • Not applicable (covered by focused unit tests for Suspense fallback behavior).

Manual testing performed

  • ./node_modules/.bin/organize-imports-cli <changed UI files>
  • ./node_modules/.bin/eslint --no-error-on-unmatched-pattern --fix <changed UI files>
  • ./node_modules/.bin/prettier --write <changed UI files>
  • ./node_modules/.bin/jest src/components/AppRouter/withSuspenseFallback.test.tsx src/components/common/LazyTabContent/LazyTabContent.test.tsx --runInBand
  • git diff --check

UI screen recording / screenshots:

Not attached.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
  • For UI changes: I attached a screen recording and/or screenshots above.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.

Greptile Summary

This PR adjusts Suspense loading behavior for lazy UI chunks. The main changes are:

  • Silent default fallback for shared embedded lazy components.
  • Page-level fallback helper for route chunks that block navigation.
  • Active-tab lazy rendering for entity lineage and knowledge graph tabs.
  • Removed duplicate loaders from secondary panels and widget areas.
  • Unit coverage for the Suspense helper and lazy tab content.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.

Important Files Changed

Filename Overview
openmetadata-ui/src/main/resources/ui/src/components/AppRouter/withSuspenseFallback.tsx Splits generic silent Suspense fallback behavior from explicit page-level loading.
openmetadata-ui/src/main/resources/ui/src/components/common/LazyTabContent/LazyTabContent.tsx Adds active-tab gated Suspense rendering for lazy entity tab content.
openmetadata-ui/src/main/resources/ui/src/utils/EntityDetailComponentUtils.tsx Keeps a fullscreen loader visible while lazy entity detail pages resolve.
openmetadata-ui/src/main/resources/ui/src/components/AppRouter/AppRouter.tsx Moves top-level route chunks to the page-level Suspense fallback.
openmetadata-ui/src/main/resources/ui/src/components/AppRouter/AuthenticatedAppRouter.tsx Moves authenticated page route chunks to the page-level Suspense fallback.

Reviews (13): Last reviewed commit: "address comments" | Re-trigger Greptile

Context used (3)

  • Context used - CLAUDE.md (source)
  • Context used - openmetadata-ui-core-components/CLAUDE.md (source)
  • Context used - AGENTS.md (source)

@shah-harshit shah-harshit requested a review from a team as a code owner July 4, 2026 08:04
@shah-harshit shah-harshit added UI UI specific issues safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check labels Jul 4, 2026
@shah-harshit shah-harshit self-assigned this Jul 4, 2026
Comment thread openmetadata-ui/src/main/resources/ui/src/utils/CustomizableLandingPageUtils.tsx Outdated
@gitar-bot

gitar-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 6 resolved / 6 findings

Refactors UI Suspense boundaries to use silent fallbacks for background lazy chunks and introduces LazyTabContent to prevent redundant loaders. All initial concerns regarding route-level loading gaps and missing dependencies were successfully addressed.

✅ 6 resolved
Bug: Global null Suspense default removes loaders on full-page route loads

📄 openmetadata-ui/src/main/resources/ui/src/components/AppRouter/withSuspenseFallback.tsx:14-20
withSuspenseFallback now defaults fallback to null instead of the page-level DEFAULT_FALLBACK Loader (withSuspenseFallback.tsx:19). This default is shared by ~153 call sites, including top-level route/page components that rely on it as their only Suspense boundary during first load — e.g. AuthenticatedAppRouter (ApplicationRoutesClassBase.ts:19-21), and every lazy page wrapped in AppRouter.tsx / AuthenticatedAppRouter.tsx (DataQualityPage, TagPage, CustomizablePage, etc.).

There is no enclosing parent Suspense with a visible fallback: in AppRouter.tsx the Loader fullScreen at line 92-94 only covers the auth/bootstrap phase, and each wrapped page provides its own (now silent) Suspense. As a result, navigating to a route whose JS chunk has not yet been downloaded will render a blank screen for the entire download duration instead of the previous page loader. On slow networks this looks like a hang/broken navigation, which is a regression beyond the PR's stated goal (suppressing loaders for background chunks on already-rendered UI).

Recommendation: don't flip the global default. Keep the visible Loader default for genuine route/page-level wrappers and opt into the silent (null) fallback only at the specific embedded/background call sites this PR targets — e.g. add a withSilentSuspenseFallback helper (or pass fallback={null} explicitly at those sites) while leaving the route wrappers on the loader default.

Quality: breadcrumb useMemo omits testCaseFQN/activeTab deps

📄 openmetadata-ui/src/main/resources/ui/src/pages/IncidentManager/IncidentManagerDetailPage/IncidentManagerDetailPage.tsx:156-170
The breadcrumb useMemo in IncidentManagerDetailPage.tsx reads testCaseFQN (line 162) and activeTab (line 163) when building the intermediate link for dimension pages, but its dependency array is [testCase, isDimensionPage, dimensionKey, t] (line 183). On a dimension page, activeTab can change via tab navigation without testCase/dimensionKey changing, so the memo will not recompute and the parent test-case breadcrumb link will point at a stale tab URL. Add testCaseFQN and activeTab to the dependency array.

Edge Case: ApplicationsProvider renders children before app list loads

📄 openmetadata-ui/src/main/resources/ui/src/components/Settings/Applications/ApplicationsProvider/ApplicationsProvider.tsx:36-40 📄 openmetadata-ui/src/main/resources/ui/src/components/Settings/Applications/ApplicationsProvider/ApplicationsProvider.tsx:120-125
In ApplicationsProvider.tsx the loading gate {loading ? <Loader /> : children} was replaced with an unconditional {children}, and the loading state was removed entirely. Previously the entire subtree wrapped by this provider was blocked until getInstalledApplicationList() resolved. Now children mount immediately with applications still [] and installedPluginInstances empty.

Any descendant that reads applications/plugins from ApplicationsContext on first render will now observe an empty list instead of waiting for the fetch. Consumers that infer "no applications installed" from an empty array (rather than gating on the setApplicationsLoaded store flag) can render incorrect UI, then flicker once data arrives. This is a behavioral change that goes beyond suppressing a loader flash.

Please confirm every consumer of this context gates on the applicationsLoaded store signal rather than on applications.length, or restore a gate that defers rendering (or the dependent branches) until the initial fetch settles.

Edge Case: Feed right panel shows empty-state placeholder during load

📄 openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:885-897
The right-panel rendering removed the !loading guard. Previously, when nothing was selected, the "No conversations" placeholder only rendered once loading was false (!loading && (...)). Now it renders unconditionally whenever selectedThread || selectedTask || selectedActivity is falsy.

This means during the initial/refresh load (while feed data is still being fetched), the right panel now shows the "No conversations" empty-state placeholder even though data may be arriving. This can produce a brief incorrect empty-state flash before content loads. If the intent is only to remove the duplicate loader, consider still suppressing the placeholder while loading is true (e.g., render null during loading) so the empty state is not shown prematurely.

Bug: Top-level auth route chunks now render blank while loading

📄 openmetadata-ui/src/main/resources/ui/src/components/AppRouter/AppRouter.tsx:29-39 📄 openmetadata-ui/src/main/resources/ui/src/components/AppRouter/AppRouter.tsx:95-106 📄 openmetadata-ui/src/main/resources/ui/src/utils/ApplicationRoutesClassBase.ts:19-21
This commit switches the top-level authenticated shell/router wrappers from withPageSuspenseFallback (visible <PageLoader />) to withSuspenseFallback (silent null fallback):

  • AppRouter.tsx: AuthenticatedApp and AuthenticatedRoutes
  • ApplicationRoutesClassBase.ts: AuthenticatedAppRouter

These are large, top-level route chunks. In AppRouter, the full-screen loader is only shown while isApplicationLoading || isAuthenticating is true (line 95-97). Once that gate passes, AppRouter renders <AuthenticatedApp><AuthenticatedRoutesComponent /></AuthenticatedApp> — and the lazy import for these chunks only begins at that point. On a first load or slow network, the chunk is still downloading, and because the fallback is now null, the user sees a blank screen instead of a page loader until the chunk resolves. No page-level route loader can appear until that router chunk has resolved.

The silent fallback is appropriate for embedded/background lazy chunks (the stated intent of the PR), but the top-level authenticated router tree is the blocking path that gates the entire visible page. Consider keeping withPageSuspenseFallback for these three top-level wrappers so a loader is shown while the main chunk downloads, and reserve the silent fallback for secondary/embedded chunks.

...and 1 more resolved from earlier reviews

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Jest test Coverage

UI tests summary

Lines Statements Branches Functions
Coverage: 63%
63.79% (72751/114046) 46.92% (42312/90169) 48.21% (13017/26996)

@gitar-bot

gitar-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown
Code Review ✅ Approved 6 resolved / 6 findings

Refactors UI Suspense boundaries to use silent fallbacks for background lazy chunks and introduces LazyTabContent to prevent redundant loaders. All initial concerns regarding route-level loading gaps and missing dependencies were successfully addressed.

✅ 6 resolved
Bug: Global null Suspense default removes loaders on full-page route loads

📄 openmetadata-ui/src/main/resources/ui/src/components/AppRouter/withSuspenseFallback.tsx:14-20
withSuspenseFallback now defaults fallback to null instead of the page-level DEFAULT_FALLBACK Loader (withSuspenseFallback.tsx:19). This default is shared by ~153 call sites, including top-level route/page components that rely on it as their only Suspense boundary during first load — e.g. AuthenticatedAppRouter (ApplicationRoutesClassBase.ts:19-21), and every lazy page wrapped in AppRouter.tsx / AuthenticatedAppRouter.tsx (DataQualityPage, TagPage, CustomizablePage, etc.).

There is no enclosing parent Suspense with a visible fallback: in AppRouter.tsx the Loader fullScreen at line 92-94 only covers the auth/bootstrap phase, and each wrapped page provides its own (now silent) Suspense. As a result, navigating to a route whose JS chunk has not yet been downloaded will render a blank screen for the entire download duration instead of the previous page loader. On slow networks this looks like a hang/broken navigation, which is a regression beyond the PR's stated goal (suppressing loaders for background chunks on already-rendered UI).

Recommendation: don't flip the global default. Keep the visible Loader default for genuine route/page-level wrappers and opt into the silent (null) fallback only at the specific embedded/background call sites this PR targets — e.g. add a withSilentSuspenseFallback helper (or pass fallback={null} explicitly at those sites) while leaving the route wrappers on the loader default.

Quality: breadcrumb useMemo omits testCaseFQN/activeTab deps

📄 openmetadata-ui/src/main/resources/ui/src/pages/IncidentManager/IncidentManagerDetailPage/IncidentManagerDetailPage.tsx:156-170
The breadcrumb useMemo in IncidentManagerDetailPage.tsx reads testCaseFQN (line 162) and activeTab (line 163) when building the intermediate link for dimension pages, but its dependency array is [testCase, isDimensionPage, dimensionKey, t] (line 183). On a dimension page, activeTab can change via tab navigation without testCase/dimensionKey changing, so the memo will not recompute and the parent test-case breadcrumb link will point at a stale tab URL. Add testCaseFQN and activeTab to the dependency array.

Edge Case: ApplicationsProvider renders children before app list loads

📄 openmetadata-ui/src/main/resources/ui/src/components/Settings/Applications/ApplicationsProvider/ApplicationsProvider.tsx:36-40 📄 openmetadata-ui/src/main/resources/ui/src/components/Settings/Applications/ApplicationsProvider/ApplicationsProvider.tsx:120-125
In ApplicationsProvider.tsx the loading gate {loading ? <Loader /> : children} was replaced with an unconditional {children}, and the loading state was removed entirely. Previously the entire subtree wrapped by this provider was blocked until getInstalledApplicationList() resolved. Now children mount immediately with applications still [] and installedPluginInstances empty.

Any descendant that reads applications/plugins from ApplicationsContext on first render will now observe an empty list instead of waiting for the fetch. Consumers that infer "no applications installed" from an empty array (rather than gating on the setApplicationsLoaded store flag) can render incorrect UI, then flicker once data arrives. This is a behavioral change that goes beyond suppressing a loader flash.

Please confirm every consumer of this context gates on the applicationsLoaded store signal rather than on applications.length, or restore a gate that defers rendering (or the dependent branches) until the initial fetch settles.

Edge Case: Feed right panel shows empty-state placeholder during load

📄 openmetadata-ui/src/main/resources/ui/src/components/ActivityFeed/ActivityFeedTab/ActivityFeedTab.component.tsx:885-897
The right-panel rendering removed the !loading guard. Previously, when nothing was selected, the "No conversations" placeholder only rendered once loading was false (!loading && (...)). Now it renders unconditionally whenever selectedThread || selectedTask || selectedActivity is falsy.

This means during the initial/refresh load (while feed data is still being fetched), the right panel now shows the "No conversations" empty-state placeholder even though data may be arriving. This can produce a brief incorrect empty-state flash before content loads. If the intent is only to remove the duplicate loader, consider still suppressing the placeholder while loading is true (e.g., render null during loading) so the empty state is not shown prematurely.

Bug: Top-level auth route chunks now render blank while loading

📄 openmetadata-ui/src/main/resources/ui/src/components/AppRouter/AppRouter.tsx:29-39 📄 openmetadata-ui/src/main/resources/ui/src/components/AppRouter/AppRouter.tsx:95-106 📄 openmetadata-ui/src/main/resources/ui/src/utils/ApplicationRoutesClassBase.ts:19-21
This commit switches the top-level authenticated shell/router wrappers from withPageSuspenseFallback (visible <PageLoader />) to withSuspenseFallback (silent null fallback):

  • AppRouter.tsx: AuthenticatedApp and AuthenticatedRoutes
  • ApplicationRoutesClassBase.ts: AuthenticatedAppRouter

These are large, top-level route chunks. In AppRouter, the full-screen loader is only shown while isApplicationLoading || isAuthenticating is true (line 95-97). Once that gate passes, AppRouter renders <AuthenticatedApp><AuthenticatedRoutesComponent /></AuthenticatedApp> — and the lazy import for these chunks only begins at that point. On a first load or slow network, the chunk is still downloading, and because the fallback is now null, the user sees a blank screen instead of a page loader until the chunk resolves. No page-level route loader can appear until that router chunk has resolved.

The silent fallback is appropriate for embedded/background lazy chunks (the stated intent of the PR), but the top-level authenticated router tree is the blocking path that gates the entire visible page. Consider keeping withPageSuspenseFallback for these three top-level wrappers so a loader is shown while the main chunk downloads, and reserve the silent fallback for secondary/embedded chunks.

...and 1 more resolved from earlier reviews

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

@sonarqubecloud

sonarqubecloud Bot commented Jul 6, 2026

Copy link
Copy Markdown

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

Labels

safe to test Add this label to run secure Github workflows on PRs skip-pr-checks Bypass PR metadata validation check UI UI specific issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant