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 60.38% 60.36% -0.03%
==========================================
Files 2526 2527 +1
Lines 101503 101528 +25
Branches 28415 28420 +5
==========================================
- Hits 61297 61288 -9
- Misses 39642 39679 +37
+ Partials 564 561 -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
debsmita1
changed the title
37fe7cd to
abe6e8b
Compare
debsmita1
changed the title
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
Co-authored-by: Cursor <cursoragent@cursor.com>
45a02b6 to
341f813
Compare
341f813 to
1ef14d0
Compare
|
2 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