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
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4853 +/- ##
==========================================
+ Coverage 63.62% 63.63% +0.01%
==========================================
Files 2693 2697 +4
Lines 107215 107243 +28
Branches 29996 30006 +10
==========================================
+ Hits 68215 68244 +29
+ Misses 38494 38492 -2
- Partials 506 507 +1
*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:
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 2:15 PM UTC · Completed 2:42 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $8.95 |
57fc8af to
72be27e
Compare
|
Risk Assessment: elevated (3/5) DetailsA mid-size refactor (28 files, 1058 lines, large blast radius) with four dependency files changed and a borderline test ratio drives an elevated score, offset by no security exposure, no CI changes, and an experienced returning author. |
ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
workspaces/quickstart/plugins/quickstart/README.md(file-level): Line 36 · [low] stale identifier reference
OFS config lists QuickstartDrawerProvider, QuickstartDrawerStateExposer, and QuickstartButton as importName values. These were removed from explicit re-exports in legacyExports.ts but remain accessible via export * from './plugin'.
workspaces/quickstart/plugins/quickstart/README.md(file-level): Line 59 · [low] stale identifier reference
Static installation example imports QuickstartDrawerProvider. Still works via wildcard re-export but the explicit export path was removed.
| export const QuickstartButton: ( | ||
| input: QuickstartButtonProps, | ||
| ) => JSX_2.Element | null; | ||
| export const QuickstartButton: React.ComponentType<QuickstartButtonProps>; |
There was a problem hiding this comment.
[medium] exported type signature change
QuickstartButton's exported type changed from (input: QuickstartButtonProps) => JSX_2.Element | null to React.ComponentType because the explicit re-export was removed from legacyExports.ts; the surviving wildcard re-export from plugin.ts uses the broader type. Consumers with explicit type annotations may see TypeScript errors.
Suggested fix: Document this type-level change in the changeset.
| export const QuickstartDrawerProvider: ( | ||
| input: PropsWithChildren, | ||
| ) => JSX_2.Element; | ||
| export const QuickstartDrawerProvider: React.ComponentType<PropsWithChildren>; |
There was a problem hiding this comment.
[medium] exported type signature change
QuickstartDrawerProvider's type similarly changed from (input: PropsWithChildren) => JSX_2.Element to React.ComponentType.
Suggested fix: Document in the changeset.
| export const QuickstartCtaLink = ({ cta, onClick }: QuickstartCtaLinkProps) => { | ||
| const { t } = useTranslation(); | ||
| // If no CTA is provided, show a default "Got it!" button | ||
|
|
There was a problem hiding this comment.
[low] scope-creep
Replacing LinkButton with ButtonLink changes CTA DOM role from button to link (confirmed by test changes from getByRole('button') to getByRole('link')). Part of the NFS optimization goal of removing @backstage/core-components from the sync graph.
72be27e to
06994e0
Compare
06994e0 to
db0e9c9
Compare
db0e9c9 to
9986da4
Compare
9986da4 to
e944a1a
Compare
|
|
Two things are blocking this for me: 1. The bundle got bigger, not smaller. 2. SonarQube quality gate failed. |
|
/hold |
3 participants
Hey, I just made a Pull Request!
Resolves:
https://redhat.atlassian.net/browse/RHIDP-15553
This PR reduces the quickstart plugin’s NFS Module Federation sync footprint by deferring UI that was previously loaded eagerly at plugin registration time. The NFS entry (./) now registers only blueprint wiring and thin React.lazy boundaries; drawer UI, snackbar init, help menu content, and MUI item icons load via async chunks when actually needed.
Problem
Chunk analysis showed the quickstart NFS entry (./) at 464.8 KB across 4 sync chunks, ~294 KB larger than the OFS legacy entry (./legacy, 170.9 KB / 3 chunks). Plugin UI was pulled into the sync graph through:
Eager blueprint mounts — AppDrawerContentBlueprint and AppRootElementBlueprint used static elements (, ), sync-loading drawer UI, Snackbar/MUI, and icons even when the drawer was closed.
Sync help menu component — GlobalHeaderMenuItemBlueprint referenced QuickstartHelpMenuItem synchronously.
Static MUI icon map — eight @mui/icons-material imports in QuickstartItemIcon landed on the drawer sync path.
OFS eager legacy overrides — legacyExports.ts re-exported concrete QuickstartDrawerProvider / QuickstartButton instead of the lazy extensions already defined in plugin.ts.
Solution description:
@backstage/ui CTAs — QuickstartCtaLink migrates from @backstage/core-components LinkButton to @backstage/ui Button / ButtonLink.
Expose summary
NFS entry chunk (__federation_expose_default_export): 14 KB → 5 KB (−8 KB, −61.3%)
Sync chunk breakdown
The entry chunk shrank 61%, but total sync rose slightly because the framework sync chunk shifted from lodash (69 KB) to backstage (86 KB) — driven by the global-header 3.0 and @backstage/ui dependency updates in this commit, not by quickstart UI code staying on the sync graph.
Async growth (on-demand only)
Vendor async is dominated by highlight.js (~852 KB) and parse5 (~182 KB), reachable through the drawer/CTA dependency graph (@backstage/ui, markdown rendering). These load only when the drawer or help menu is opened, not at plugin startup.
Screenshots:
With fallback icon
✔️ Checklist