chore(global-header): Global header optimization - #4006
Conversation
|
Important This PR includes changes that affect public-facing API. Please ensure you are adding/updating documentation for new features or behavior. Changed Packages
|
PR Summary by QodoOptimize global-header federation sync via loaders and /components entry
AI Description
Diagram
High-Level Assessment
Files changed (23)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4006 +/- ##
==========================================
- Coverage 61.59% 61.56% -0.03%
==========================================
Files 2556 2558 +2
Lines 102054 102082 +28
Branches 28576 28584 +8
==========================================
- Hits 62856 62850 -6
- Misses 38599 38636 +37
+ Partials 599 596 -3
*This pull request uses carry forward flags. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Code Review by Qodo
1.
|
rhdh-qodo-merge
Bot
added
documentation
9460854 to
11f5f34
Compare
ciiay
left a comment
ciiay
left a comment
There was a problem hiding this comment.
The following files still importing GlobalHeaderMenuItem from /alpha. Let's also update them in this pr.
- workspaces/quickstart/plugins/quickstart/src/QuickstartHelpMenuItem.tsx
- workspaces/app-defaults/packages/app/src/modules/drawer-demo/DrawerDemoContent.tsx
11f5f34 to
3ef6dd4
Compare
These are different workspaces importing from the global-header package . I will update them once the new version of the global-header with these changes are released |
3ef6dd4 to
6f2d59f
Compare
|
Let's merge this one #3413 first |
6f2d59f to
62dfcba
Compare
debsmita1
changed the title
979d489 to
8ff1096
Compare
e7504e6 to
5dd3cb0
Compare
fa49611 to
f91270c
Compare
ciiay
left a comment
ciiay
left a comment
There was a problem hiding this comment.
Thanks for the NFS sync work — the loader / /components split and the trackValidity settle logic look right, and the new dropdown tests cover the empty-state race.
The architecture matches RHIDP-15555 and the ~88% sync-size drop is a real win. A few API/consumer issues should be fixed before merge:
- Breaking export move vs
minorbump. Building blocks are removed from the root and/alphaon a 2.x package, but the changeset isminor.workspaces/quickstartis on^2.0.0and still importsGlobalHeaderMenuItemfrom the root, so 2.1.0 would break it (same for app-defaults/alpha). Please use a major, keep a deprecated root re-export for one release, or update in-repo consumers in this PR. HeaderIconnow returns null for unregistered ids. Sample app + NFS docs still usemenu_book; quickstart useswaving_hand. Those icons will disappear unless registered or switched to a system-icon id / SVG / URL.- Leftover “
/alphasync chunk” comment inpackages/app/src/App.tsx.
Non-blocking: globalHeaderSystemIcons static MUI imports land back on the root module; consider wrapping LazyGlobalHeader in an ErrorBoundary.
CI looks good. Happy to re-review after the versioning + icon-id follow-ups.
| {ligature} | ||
| </MuiIcon> | ||
| ); | ||
| return null; |
There was a problem hiding this comment.
Dropping the ligature fallback is the right call for Module Federation (no host icon font), but it is a silent behavior change for existing config.
Ids that used to work via the font — menu_book, waving_hand, hub, manage_accounts, addCircleOutline, account_circle — now render nothing unless the host or IconBundleBlueprint registers them.
Default extensions in this PR were updated; sample app, NFS docs, and in-repo consumers (quickstart waving_hand) were not. Please align those, and call out the config migration in the changeset so deployers know globalHeader.menuItems[].icon / globalHeader.components[].icon values may need to change.
There was a problem hiding this comment.
I have addressed this in the changeset as a breaking change
|
|
||
| import AccountCircleOutlined from '@mui/icons-material/AccountCircleOutlined'; | ||
| import AddCircleOutline from '@mui/icons-material/AddCircleOutline'; | ||
| import ArticleOutlined from '@mui/icons-material/ArticleOutlined'; | ||
| import BugReportOutlined from '@mui/icons-material/BugReportOutlined'; | ||
| import DashboardOutlined from '@mui/icons-material/DashboardOutlined'; | ||
| import ForumOutlined from '@mui/icons-material/ForumOutlined'; | ||
| import HubOutlined from '@mui/icons-material/HubOutlined'; | ||
| import LogoutOutlined from '@mui/icons-material/LogoutOutlined'; | ||
| import ManageAccountsOutlined from '@mui/icons-material/ManageAccountsOutlined'; | ||
| import QuizOutlined from '@mui/icons-material/QuizOutlined'; | ||
| import Support from '@mui/icons-material/Support'; | ||
|
|
||
| /** | ||
| * System icons registered for global-header via `IconBundleBlueprint`. | ||
| * Host registrations override via `app.getSystemIcon`. | ||
| * | ||
| * Includes default extension ids and common `globalHeader` config icon ids, | ||
| * using outlined `@mui/icons-material` components. | ||
| */ | ||
| export const globalHeaderSystemIcons = { | ||
| account: AccountCircleOutlined, | ||
| add: AddCircleOutline, | ||
| article: ArticleOutlined, | ||
| bug_report: BugReportOutlined, | ||
| dashboard: DashboardOutlined, | ||
| developerHub: HubOutlined, | ||
| forum: ForumOutlined, | ||
| logout: LogoutOutlined, | ||
| manageAccounts: ManageAccountsOutlined, | ||
| quiz: QuizOutlined, | ||
| support: Support, |
There was a problem hiding this comment.
Non-blocking / measurement ask: this file is imported from globalHeaderModule, which is exported from the root entry, so these @mui/icons-material modules land back on the NFS sync chunk this PR is trying to empty.
Twelve outlined icons are much smaller than AppBar, so the 88% win can still be real — but the PR/changeset text says MUI stays off the root sync path, which is no longer strictly true. Worth confirming in the expose summary, or lazy-registering if IconBundleBlueprint allows it.
Also a small consistency nit: ids mix snake_case (bug_report) and camelCase (manageAccounts, developerHub), and support uses Support rather than an outlined variant.
There was a problem hiding this comment.
Yes, the PR changes doesnot eliminating all MUI from root and the changeset reflects that. Lazy icon registration is not possible for IconBundleBlueprint
On naming ids, I have fixed it in the code and captured it in the doc and the changeset
support now uses SupportAgentOutlined
341f813 to
1ef14d0
Compare
| {ligature} | ||
| </MuiIcon> | ||
| ); | ||
| return null; |
There was a problem hiding this comment.
@debsmita1 I am thinking if we should have a default icon instead. Returning null just renders that awful text like incomplete icon which we already have as a bug. It just looks like the UI is broken because of that. Maybe we can have a default icon with a tooltip that gets rendered when none of the above options are available, WDYT?
There was a problem hiding this comment.
Updated. Unknown icon ids now render HelpOutlineOutlined with an unknown id warning in the browser's console. Unknown id tooltip will otherwise overlap with the menu item tooltip
There was a problem hiding this comment.
Not sure if thats the best option for default icon since we also use the same for help dropdown.
There was a problem hiding this comment.
@ShiranHi could you help here with a fallback icon for global header menu items when the configured icon is not found
There was a problem hiding this comment.
What do you think about using a generic shapes icon outlined as the default?
| export type { GlobalHeaderDropdownProps } from './components/GlobalHeaderDropdown'; | ||
|
|
||
| /** | ||
| * Empty module so this package export is published as a Module Federation |
There was a problem hiding this comment.
Do we actually need this to be available at module federation level? These are all the components that will be imported by another plugin to build there own components which in turn should be lazy loaded. But the base components could just be exported at /component without a remote in module federation.
There was a problem hiding this comment.
This is required for quickstart’s help menu item . Without a federated ./components expose, that import would either fail at runtime or get resolved through the root remote, defeating the sync-chunk split.
There was a problem hiding this comment.
Wouldn't QuickStart import this component at build time? The component then built based on that will be lazy loaded in the blueprint registration, right?
| /** | ||
| * Default toolbar component extensions (`gh-component`) for the global header. | ||
| * | ||
| * Heavy UI uses blueprint `loader` (ExtensionBoundary.lazyComponent). |
There was a problem hiding this comment.
The async size went up by ~984 KB across 43 chunks and all these loaders fire when the header mounts. I know cutting the sync chunk is the goal, but did we check the actual first-paint/render time of the header? Are we now waterfalling a bunch of small chunks on mount? Just want to make sure that's acceptable, WDYT?
There was a problem hiding this comment.
The goal is moving MUI/AppBar UI off the root MF sync chunk. The async growth is expected as code splits into per-component chunks
There was a problem hiding this comment.
Understood on shrinking the MF sync chunk but this is more about the overall approach than this PR.
If first paint stays the same because one large chunk is now many smaller ones all loading on mount, we have not really gained anything. Multiple HTTP requests can be slower than one, especially when everything is needed immediately — request overhead, weaker compression, and waterfalls add up even with HTTP/2.
Async growth from splitting is expected, but it only pays off if those chunks are deferred or first paint is actually faster. Have we measured header first-paint before vs after?
Co-authored-by: Cursor <cursoragent@cursor.com>
4ea672e to
91472ea
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
91472ea to
b1d2356
Compare
|
4 participants
Hey, I just made a Pull Request!
Resolves:
https://redhat.atlassian.net/browse/RHIDP-15555
Solution description:
Current expose summary
GIF:
Tested on RHDH with Quickstarts plugin
Screen.Recording.2026-07-27.at.10.44.05.PM.mov
Test setup:
Update the Quickstarts plugin
In workspaces/quickstart:
File Dependency
package.json (resolutions) portal:../global-header/plugins/global-header
plugins/quickstart/package.json. portal:../../../global-header/plugins/global-header
packages/app/package.json. portal:../../../global-header/plugins/global-header
✔️ Checklist