Frontend bundle size #795
Merged
Frontend bundle size #795
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideFrontend bundle size optimizations via React lazy-loading, route and widget code-splitting, icon tree-shaking imports, Docker/runtime tweaks, and documentation updates for building images and running the frontend. Sequence diagram for lazy-loaded dashboard routesequenceDiagram
actor User
participant BrowserRouter
participant SuspenseRoute
participant DashboardChunk as Dashboard_module
participant Server
User->>BrowserRouter: Navigate to /project/:project_id/dashboard
BrowserRouter->>SuspenseRoute: Render Dashboard element
SuspenseRoute->>DashboardChunk: dynamic import('../pages/dashboard')
DashboardChunk->>Server: HTTP GET dashboard.chunk.js
Server-->>DashboardChunk: dashboard.chunk.js
DashboardChunk-->>SuspenseRoute: Promise resolved with Dashboard
Note over SuspenseRoute: While pending, render PageSpinner/ContentSpinner
SuspenseRoute-->>BrowserRouter: Render Dashboard component
BrowserRouter-->>User: Dashboard UI displayed
Sequence diagram for lazy-loaded Jenkins heatmap widgetsequenceDiagram
actor User
participant DashboardPage
participant useWidgetsHook
participant JenkinsHeatmapWrapper
participant getHeatmapTypes
participant FilterHeatmapWidget
participant FilterHeatmapModule as filter_heatmap_module
User->>DashboardPage: Open dashboard with jenkins-heatmap widget
DashboardPage->>useWidgetsHook: Render widgets
useWidgetsHook->>JenkinsHeatmapWrapper: Render widget component
JenkinsHeatmapWrapper->>JenkinsHeatmapWrapper: useEffect on mount
JenkinsHeatmapWrapper->>getHeatmapTypes: getHeatmapTypes()
getHeatmapTypes->>FilterHeatmapModule: import('../../widgets/filter-heatmap')
FilterHeatmapModule-->>getHeatmapTypes: HEATMAP_TYPES
getHeatmapTypes-->>JenkinsHeatmapWrapper: types.jenkins
JenkinsHeatmapWrapper->>JenkinsHeatmapWrapper: setHeatmapType(types.jenkins)
Note over JenkinsHeatmapWrapper: While heatmapType is null, render WidgetSpinner
JenkinsHeatmapWrapper->>FilterHeatmapWidget: Render with title, params, type, callbacks
FilterHeatmapWidget-->>User: Jenkins heatmap displayed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
use-widgets.js, you're dynamically importing../../widgets/filter-heatmaptwice (once forFilterHeatmapWidgetand once forHEATMAP_TYPES_PROMISE); consider centralizing this into a single dynamic import (e.g., a helper that exposes both the default widget andHEATMAP_TYPES) to avoid duplicate chunks and keep the exports in sync. - There are multiple very similar inline spinner components (
WidgetSpinner,PageSpinner,ContentSpinner) with minor style differences; consider extracting a shared loading/spinner component to reduce duplication and keep the loading UX consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `use-widgets.js`, you're dynamically importing `../../widgets/filter-heatmap` twice (once for `FilterHeatmapWidget` and once for `HEATMAP_TYPES_PROMISE`); consider centralizing this into a single dynamic import (e.g., a helper that exposes both the default widget and `HEATMAP_TYPES`) to avoid duplicate chunks and keep the exports in sync.
- There are multiple very similar inline spinner components (`WidgetSpinner`, `PageSpinner`, `ContentSpinner`) with minor style differences; consider extracting a shared loading/spinner component to reduce duplication and keep the loading UX consistent.
## Individual Comments
### Comment 1
<location> `frontend/src/components/hooks/use-widgets.js:23-22` </location>
<code_context>
+ default: module.FilterHeatmapWidget,
+ })),
+);
+const HEATMAP_TYPES_PROMISE = import('../../widgets/filter-heatmap').then(
+ (module) => module.HEATMAP_TYPES,
+);
+
+const GenericAreaWidget = lazy(() => import('../../widgets/generic-area'));
</code_context>
<issue_to_address>
**suggestion (performance):** Defer creating the HEATMAP_TYPES import promise to avoid an eager side-effect at module load time.
`HEATMAP_TYPES_PROMISE` is created when the module is loaded, so the dynamic import runs even if `JenkinsHeatmapWrapper` is never rendered. To keep this truly lazy, move the import into `getHeatmapTypes` and cache the result there, for example:
```js
let heatmapTypesPromise;
let heatmapTypesCache = null;
const getHeatmapTypes = async () => {
if (heatmapTypesCache) return heatmapTypesCache;
if (!heatmapTypesPromise) {
heatmapTypesPromise = import('../../widgets/filter-heatmap')
.then(m => m.HEATMAP_TYPES);
}
heatmapTypesCache = await heatmapTypesPromise;
return heatmapTypesCache;
};
```
This way the chunk only loads when needed and is reused on subsequent calls.
Suggested implementation:
```javascript
const FilterHeatmapWidget = lazy(() =>
import('../../widgets/filter-heatmap').then((module) => ({
default: module.FilterHeatmapWidget,
})),
);
let heatmapTypesPromise;
let heatmapTypesCache = null;
const getHeatmapTypes = async () => {
if (heatmapTypesCache) return heatmapTypesCache;
if (!heatmapTypesPromise) {
heatmapTypesPromise = import('../../widgets/filter-heatmap').then(
(module) => module.HEATMAP_TYPES,
);
}
heatmapTypesCache = await heatmapTypesPromise;
return heatmapTypesCache;
};
const GenericAreaWidget = lazy(() => import('../../widgets/generic-area'));
```
1. Replace any existing uses of `HEATMAP_TYPES_PROMISE` in this file (or its consumers) with calls to `getHeatmapTypes()`. For example:
- `await HEATMAP_TYPES_PROMISE` ➜ `await getHeatmapTypes()`.
2. If `HEATMAP_TYPES_PROMISE` was being exported from this module, remove that export and export `getHeatmapTypes` instead, preserving the existing export style (named or default).
</issue_to_address>
### Comment 2
<location> `frontend/docker/entrypoint.sh:10-13` </location>
<code_context>
+
+echo "Generating settings.js with runtime configuration..."
+
+cat > /opt/app-root/src/build/settings.js << EOF
+window.settings = {};
+window.settings.serverUrl='${REACT_APP_SERVER_URL:-http://localhost:8080/api}';
+window.settings.environment='${NODE_ENV:-development}';
+EOF
+
</code_context>
<issue_to_address>
**🚨 issue (security):** Escape environment variable values when embedding them into JavaScript strings.
`REACT_APP_SERVER_URL` and `NODE_ENV` are inserted directly into single-quoted JS strings. If they contain characters like `'`, `\`, or newlines, `settings.js` may become invalid, and in some cases misconfigured values could lead to code injection. Please escape these values before writing them, e.g. via JSON encoding:
```bash
cat > /opt/app-root/src/build/settings.js << EOF
window.settings = {};
window.settings.serverUrl=$(printf '%s' "${REACT_APP_SERVER_URL:-http://localhost:8080/api}" | jq -Rs .);
window.settings.environment=$(printf '%s' "${NODE_ENV:-development}" | jq -Rs .);
EOF
```
JSON-encoding avoids most escaping issues and makes the script more robust to unexpected env values.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements bundle size optimization for the frontend through code splitting and tree-shaking optimizations. The changes address the issue of a large bundle size (568.14 kB main JS after gzip) by implementing lazy loading for routes and widgets, and switching from barrel imports to direct ESM imports for PatternFly React icons.
- Replaced barrel imports for PatternFly icons with direct ESM imports to enable tree-shaking
- Implemented lazy loading with React.lazy() and Suspense for route-level components
- Added code splitting for widget components in the dashboard system
- Updated Docker configuration to use Node.js 20 base images and simplified deployment
- Aligned port configuration across package.json, Docker, and deployment scripts
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/widgets/filter-heatmap.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/views/jenkins-job.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/views/accessibility-analysis.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/utilities/tables.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/routes/ibutsu-page.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/routes/base.js | Added lazy loading for top-level routes with Suspense boundaries and loading spinner |
| frontend/src/routes/app.js | Added lazy loading for page components with per-route Suspense boundaries |
| frontend/src/routes/admin.js | Added lazy loading for admin page components with Suspense boundaries |
| frontend/src/pages/user-tokens.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/pages/user-profile.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/pages/sign-up.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/pages/run.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/pages/reset-password.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/pages/login.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/pages/admin/user-list.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/pages/admin/user-edit.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/pages/admin/project-list.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/pages/admin/project-edit.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/constants.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/components/widget-header.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/components/user-dropdown.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/components/table-states.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/components/result-view.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/components/multi-value-input.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/components/modals/new-dashboard-modal.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/components/modals/edit-widget-modal.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/components/hooks/use-widgets.js | Added lazy loading for widget components with custom async handling for HEATMAP_TYPES |
| frontend/src/components/hooks/use-table-filters.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/components/filtering/result-filter.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/components/filtering/active-filters.js | Converted icon imports from barrel to direct ESM imports |
| frontend/src/components/empty-object.js | Converted icon imports from barrel to direct ESM imports |
| frontend/package.json | Changed production server port from 3000 to 8080, added PORT=3000 to devserver script, added description field |
| frontend/docker/entrypoint.sh | New entrypoint script for runtime configuration of React app via settings.js |
| frontend/docker/Dockerfile.frontend | Migrated from nginx-based deployment to Node.js serve, updated to UBI9 Node.js 20 images, simplified build process |
| AGENTS.md | Added documentation for building container images with podman |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #795 +/- ##
==========================================
- Coverage 64.47% 63.95% -0.52%
==========================================
Files 156 157 +1
Lines 7572 7633 +61
Branches 659 659
==========================================
Hits 4882 4882
- Misses 2456 2517 +61
Partials 234 234
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
lazy route imports direct icon imports suppress css import warnings from patternfly-styles that come from code splitting with CI=false in container build
mshriver
force-pushed
the
frontend-bundle-size
branch
from
486651a to
511d996
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Unfortunate tradeoff here where we see build time warnings for css import order issues due to code splitting and patternfly-styles use. They're just warnings though, and we end up with smaller bundle with split files.
Test coverage is being modified here and I'll not include any updates to tests in this PR:
#792
Before
After