perf(ui): lazy load explore and shared page components#29183
perf(ui): lazy load explore and shared page components#29183shah-harshit wants to merge 1 commit into
Conversation
shah-harshit
added
UI
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| import { | ||
| getEntityDeleteMessage, | ||
| getEntityMissingError, | ||
| } from '../../utils/EntityDisplayUtils'; | ||
| } from '../../utils/EntityDisplayPureUtils'; |
There was a problem hiding this comment.
🚨 Bug: Imports reference non-existent EntityDisplayPureUtils module
These files were changed to import from ../utils/EntityDisplayPureUtils (e.g. getEntityDeleteMessage, getEntityMissingError, getCountBadge), but no such module exists in this branch. The only file present is utils/EntityDisplayUtils.tsx, which is where these functions are actually defined (confirmed: getCountBadge, getEntityMissingError, getEntityDeleteMessage all live there). As a result the module resolution will fail and the build/typecheck will break. This rename appears to belong to a sibling PR that was not included in this split. Either restore the original EntityDisplayUtils import path, or include the renamed module file in this PR.
Point imports back to the existing EntityDisplayUtils module.:
// Revert path until EntityDisplayPureUtils is introduced
import {
getEntityDeleteMessage,
getEntityMissingError,
} from '../../utils/EntityDisplayUtils';
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
| useEffect(() => { | ||
| if ( | ||
| props.isEditView && | ||
| props.widgetKey.startsWith(GlossaryTermDetailPageWidgetKeys.TERMS_TABLE) | ||
| ) { | ||
| setGlossaryChildTerms( | ||
| customizeGlossaryTermPageClassBase.getGlossaryChildTerms() | ||
| ); | ||
| import( | ||
| '../../../utils/CustomizeGlossaryTerm/CustomizeGlossaryTermBaseClass' | ||
| ).then(({ default: customizeGlossaryTermPageClassBase }) => { | ||
| setGlossaryChildTerms( | ||
| customizeGlossaryTermPageClassBase.getGlossaryChildTerms() | ||
| ); | ||
| }); | ||
| } | ||
| }, [props.widgetKey, props.isEditView]); | ||
| }, [props.widgetKey, props.isEditView, setGlossaryChildTerms]); |
There was a problem hiding this comment.
💡 Bug: Dynamic import in useEffect can set state after unmount
The effect now performs a dynamic import(...).then(...) and calls setGlossaryChildTerms(...) in the resolved callback. If the component unmounts before the dynamic import resolves, the state setter runs on an unmounted component (harmless warning in React 17, generally a no-op in React 18, but still an unguarded async-after-unmount pattern). Consider guarding with a cancellation flag in the effect cleanup to avoid the late state update.
Guard the post-import state update with a cancellation flag.:
useEffect(() => {
let cancelled = false;
if (
props.isEditView &&
props.widgetKey.startsWith(GlossaryTermDetailPageWidgetKeys.TERMS_TABLE)
) {
import('../../../utils/CustomizeGlossaryTerm/CustomizeGlossaryTermBaseClass')
.then(({ default: customizeGlossaryTermPageClassBase }) => {
if (!cancelled) {
setGlossaryChildTerms(customizeGlossaryTermPageClassBase.getGlossaryChildTerms());
}
});
}
return () => { cancelled = true; };
}, [props.widgetKey, props.isEditView, setGlossaryChildTerms]);
- Apply fix
Check the box to apply the fix or reply for a change | Was this helpful? React with 👍 / 👎
Code Review 🚫 Blocked 0 resolved / 2 findingsImplements lazy-loading for core dashboard components and refactors imports to improve bundle tree-shaking, but is blocked by broken module references in EntityDisplayPureUtils and potential state update leaks in dynamic imports. 🚨 Bug: Imports reference non-existent EntityDisplayPureUtils module📄 openmetadata-ui/src/main/resources/ui/src/pages/TagPage/TagPage.tsx:101-104 📄 openmetadata-ui/src/main/resources/ui/src/pages/TagsPage/TagsPage.tsx:56-59 📄 openmetadata-ui/src/main/resources/ui/src/utils/DeleteWidget/DeleteWidgetClassBase.ts:14-15 These files were changed to import from Point imports back to the existing EntityDisplayUtils module.💡 Bug: Dynamic import in useEffect can set state after unmountThe effect now performs a dynamic Guard the post-import state update with a cancellation flag.🤖 Prompt for agentsOptionsDisplay: compact → Showing less information. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const EntityLineageTab = lazy(() => | ||
| import('../../Lineage/EntityLineageTab/EntityLineageTab').then((module) => ({ | ||
| default: module.EntityLineageTab, | ||
| })) | ||
| ); |
There was a problem hiding this comment.
EntityLineageTab is the only lazy component in this file without withSuspenseFallback
Every other lazily loaded component in this file (13 total) is wrapped with withSuspenseFallback, which co-locates the <Suspense> boundary with the component definition. EntityLineageTab skips this and instead relies on an inline <Suspense> wrapper at its single call site (line 444). If the component is ever moved, reused, or that wrapper is refactored away, it will suspend without a fallback, causing the nearest React error boundary — or a blank screen — to handle it rather than a graceful loader.
Consider wrapping EntityLineageTab consistently with withSuspenseFallback as done for all peers in this file.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const PropertyValue = withSuspenseFallback( | ||
| lazy(() => | ||
| import('../../components/common/CustomPropertyTable/PropertyValue').then( | ||
| (m) => ({ default: m.PropertyValue }) | ||
| ) | ||
| ) | ||
| ) as ComponentType<Record<string, unknown>>; |
There was a problem hiding this comment.
ComponentType<Record<string, unknown>> cast silences prop-type errors
Both PropertyValue and TagButton are cast to ComponentType<Record<string, unknown>>, which tells TypeScript to accept any object as valid props. Their actual interfaces likely enforce required props (e.g., PropertyValue almost certainly requires a value prop). Every call site that passes incorrect or missing props will now compile cleanly instead of failing, meaning regressions in widget rendering would only surface at runtime.
Consider using ComponentType<PropertyValueProps> / ComponentType<TagButtonProps> (importing the real prop interfaces as type-only imports) so TypeScript continues to enforce correct usage.
| import( | ||
| '../../../utils/CustomizeGlossaryTerm/CustomizeGlossaryTermBaseClass' | ||
| ).then(({ default: customizeGlossaryTermPageClassBase }) => { | ||
| setGlossaryChildTerms( | ||
| customizeGlossaryTermPageClassBase.getGlossaryChildTerms() | ||
| ); | ||
| }); |
There was a problem hiding this comment.
useEffect has no .catch() handler. If CustomizeGlossaryTermBaseClass fails to load (e.g., network error or chunk hash mismatch after a deploy), setGlossaryChildTerms is never called and the glossary terms widget silently shows no dummy data in edit/preview mode with no user feedback.
| import( | |
| '../../../utils/CustomizeGlossaryTerm/CustomizeGlossaryTermBaseClass' | |
| ).then(({ default: customizeGlossaryTermPageClassBase }) => { | |
| setGlossaryChildTerms( | |
| customizeGlossaryTermPageClassBase.getGlossaryChildTerms() | |
| ); | |
| }); | |
| import( | |
| '../../../utils/CustomizeGlossaryTerm/CustomizeGlossaryTermBaseClass' | |
| ) | |
| .then(({ default: customizeGlossaryTermPageClassBase }) => { | |
| setGlossaryChildTerms( | |
| customizeGlossaryTermPageClassBase.getGlossaryChildTerms() | |
| ); | |
| }) | |
| .catch(() => { | |
| // no-op: edit/preview dummy data won't render, page stays functional | |
| }); |
1 participant
Summary
Testing
Ref: https://github.com/open-metadata/openmetadata-collate/issues/4230
Summary by Gitar
KnowledgePages,ContainerChildren, andContainerDataModelusingwithSuspenseFallback.useMemotouseEffectinGenericWidgetto dynamically importcustomizeGlossaryTermPageClassBaseon demand.import typefor interface/enum definitions to improve bundle tree-shaking.EntityDisplayPureUtilsandTaskNavigationUtilsfollowing directory restructuring.styles.cssimport forreact-awesome-query-builderinAdvanceSearchProvider.This will update automatically on new commits.
Greptile Summary
This PR applies lazy-loading across 35 files in the explore, widget, and shared page layers, converting static imports to
lazy()+withSuspenseFallbackto defer non-critical component bundles and reduce the initial page load weight. It also relocates the@react-awesome-query-builderCSS import from the global entry stylesheet to the three components that actually consume it, and migrates several utility references fromEntityDisplayUtilsto the newEntityDisplayPureUtilsmodule.withSuspenseFallback(lazy(...)), following the project's established HOC pattern consistently in almost all cases.@react-awesome-query-builder/antd/css/styles.cssis removed fromstyles/index.tsand co-located inAdvanceSearchProvider,QueryBuilderWidget, andQueryBuilderWidgetV1so it is only loaded when the query builder mounts.getEntityDeleteMessage,getEntityMissingError,getCountBadge,requiredField,getTaskDetailPath) are re-imported from their new pure-utility counterparts, with one edge case inEditConnectionFormPagewheregetServiceLogocorrectly stays inEntityDisplayUtils.Confidence Score: 4/5
Largely mechanical and safe; the changes follow the established withSuspenseFallback(lazy(...)) pattern consistently, with a few small inconsistencies worth addressing before the pattern is extended further.
The PR is a well-scoped refactor with no data-path changes. The few issues found are stylistic or type-safety concerns: one lazy component (EntityLineageTab) skips withSuspenseFallback in favour of an inline Suspense that is functionally equivalent today but easy to break in a future refactor; two components lose their specific prop types via ComponentType<Record<string, unknown>> casts; and a dynamic import in GenericWidget has no error handler. None of these affect production behaviour today.
TopicDetails.component.tsx (inconsistent Suspense strategy for EntityLineageTab) and GenericWidgetUtils.tsx (broad prop-type casts on PropertyValue and TagButton) are the two files worth a second look before this pattern is replicated elsewhere.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Initial Page Load] -->|"styles/index.ts (entry)"| B[Core Styles + Fonts] A --> C[App Shell Components] C --> D{User navigates to...} D -->|"Explore / Search"| E["AdvanceSearchProvider\n+ QB CSS (lazy chunk)"] E --> F["QueryBuilderWidget (lazy)"] E --> G["QueryBuilderWidgetV1 (lazy)"] D -->|"Topic Detail Page"| H["TopicDetails\n(lazy components)"] H --> H1["ActivityFeedTab (lazy)"] H --> H2["GenericProvider (lazy)"] H --> H3["DataAssetsHeader (lazy)"] H --> H4["EntityLineageTab (lazy + inline Suspense)"] D -->|"Customize Page"| I["CustomizablePage\n(lazy components)"] I --> I1["CustomizeGlossaryTermDetailPage (lazy)"] I --> I2["CustomizeTabWidget → GenericWidget (lazy)"] I2 --> I3["WIDGET_COMPONENTS map\n(all lazy via GenericWidgetUtils)"] D -->|"Domain / Data Products"| J["CommonWidgets\n(9 lazy components)"] J --> J1["DescriptionV1, TierWidget, CertificationWidget..."] J --> J2["DomainExpertWidget → EditIconButton + PlusIconButton (lazy)"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A[Initial Page Load] -->|"styles/index.ts (entry)"| B[Core Styles + Fonts] A --> C[App Shell Components] C --> D{User navigates to...} D -->|"Explore / Search"| E["AdvanceSearchProvider\n+ QB CSS (lazy chunk)"] E --> F["QueryBuilderWidget (lazy)"] E --> G["QueryBuilderWidgetV1 (lazy)"] D -->|"Topic Detail Page"| H["TopicDetails\n(lazy components)"] H --> H1["ActivityFeedTab (lazy)"] H --> H2["GenericProvider (lazy)"] H --> H3["DataAssetsHeader (lazy)"] H --> H4["EntityLineageTab (lazy + inline Suspense)"] D -->|"Customize Page"| I["CustomizablePage\n(lazy components)"] I --> I1["CustomizeGlossaryTermDetailPage (lazy)"] I --> I2["CustomizeTabWidget → GenericWidget (lazy)"] I2 --> I3["WIDGET_COMPONENTS map\n(all lazy via GenericWidgetUtils)"] D -->|"Domain / Data Products"| J["CommonWidgets\n(9 lazy components)"] J --> J1["DescriptionV1, TierWidget, CertificationWidget..."] J --> J2["DomainExpertWidget → EditIconButton + PlusIconButton (lazy)"]Comments Outside Diff (1)
openmetadata-ui/src/main/resources/ui/src/components/Domain/DomainExpertsWidget/DomainExpertWidget.tsx, line 424-438 (link)lazy()chunks for the same source moduleEditIconButtonandPlusIconButtonare imported from the same file via two separatelazy()calls. Each call creates its ownLazyExoticComponentinstance with its own loading state, so when both buttons are visible simultaneously, two independent<Suspense>fallback spinners appear while the single underlying module loads. The module itself is only fetched once (browsers cache the resolved promise), but the double-spinner flash can look jarring.A single
lazy()call that returns both exports would share one loading state and eliminate the double-render.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "perf(ui): lazy load explore and shared p..." | Re-trigger Greptile