Skip to content

Audit and optimise React component performance + optimize JS bundle - #783

Open
abhijitb wants to merge 12 commits into
developfrom
perf/bundle-optimization
Open

Audit and optimise React component performance + optimize JS bundle#783
abhijitb wants to merge 12 commits into
developfrom
perf/bundle-optimization

Conversation

@abhijitb

@abhijitb abhijitb commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Proposed changes

Started as a bundle-size effort; net result is bundle size grew slightly (index.js +24 KB), but re-renders and two production-breaking bugs got fixed instead.

  • Fixed webpack-public-path.js — was never invoked and read the wrong runtime property; every lazy chunk would have 404'd once code-splitting shipped.
  • Fixed ErrorBoundary — was passed a JSX element instead of a component reference (would crash on fallback).
  • Removed a duplicate, permanently-hidden notifications instance — cut that network request in half (2 → 1 per page load).
  • Route-level code splitting (React.lazy/Suspense) for Marketplace, Settings, Help, Admin, AI Designer.
  • Re-renders reduced 9 → 1 per settings toggle (−89%) — measured directly (A/B console instrumentation across the base commit and this branch, not estimated) via AppBootContext split, stable notification context, and React.memo on the settings-section components. One remaining case needs a bigger AppStore-splitting refactor, left as a follow-up.

Bundle size — investigated, not reduced: the +24 KB comes entirely from @newfold/ui-component-library's bundled lodash, unrelated to anything in this branch. Tested merging #588 (#588) (UI library 1.3.0→2.1.1) as a fix — it makes size worse (+25 KB more, new 56.7 KB chunk from a @headlessui/react v2 bump) — recommend not merging it for size reasons. Also audited lodash/react-use/html-react-parser dependency changes; none yield real savings. Full data in docs/react-performance-optimisation-plan.md.

Type of Change

Production

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update
  • Refactoring / housekeeping (changes to files not directly related to functionality)

Development

  • Tests
  • Dependency update
  • Environment update / refactoring
  • Documentation Update

Visual

Checklist

  • I have read the CONTRIBUTING doc
  • I have viewed my change in a web-browser
  • Linting and tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

abhijitb added 11 commits July 16, 2026 11:06
@abhijitb
Add route-based code splitting and fix webpack public path
9ffbe1f 
- Lazy-load all pages except Home via React.lazy + Suspense,
  splitting them into on-demand chunks (index.js 211KB -> 154KB)
- Lazy-load the NewfoldNotifications vendor component
- Remove top-level awaits in routes.js: marketplace subnav is now
  fetched via a useMarketplaceSubnavRoutes hook instead of blocking
  module evaluation, and the performance feature condition reads
  window.NewfoldFeatures.features synchronously
- Fix webpack-public-path.js: the exported function was never called
  and read NewfoldRuntime.url (site URL) instead of
  NewfoldRuntime.plugin.url (versioned build URL); now sets
  __webpack_public_path__ at import time so chunks resolve correctly
- Fix ErrorBoundary FallbackComponent being passed an element
  instead of a component
- Fix start:analyzer flag forwarding and add build:analyzer script
@abhijitb
Replace react-use with local useUpdateEffect and tighten Tailwind purge
53422c8 
- Drop the react-use dependency: only useUpdateEffect was used, now a
  ~15-line local hook in App/util/hooks consumed by the six settings
  components
- Stop scanning minified vendor build bundles in tailwind.config.js
  content globs; those modules (onboarding, ecommerce, next-steps,
  performance) enqueue their own compiled CSS, and scanning minified
  code makes Tailwind emit utilities for class-like strings. Scan
  module src/ and assets/js/ sources instead. All 10 utility classes
  dropped by this change are duplicated in the owning module's own CSS.
@abhijitb
Silence Dart Sass legacy JS API deprecation warning
1d5515b 
The sass-loader shipped with @wordpress/scripts 27 (v12) still calls
Dart Sass through the legacy JS API, so every .scss file logs a
deprecation warning. Modern API support needs sass-loader 14+, which
we only get when wp-scripts upgrades. Until then, pass
silenceDeprecations: ['legacy-js-api'] to sass via the merged config.
@abhijitb
Reduce unnecessary re-renders from context churn
01ebbe6 
- Split boot/error status into a dedicated AppBootContext so AppBody
  (and everything it renders) no longer re-renders every time a
  settings component writes to the app store
- Memoize the NotificationFeed context value with a stable push
  callback so pushing/dismissing a snackbar doesn't re-render every
  useNotification() consumer
- Remove the duplicate NewfoldNotifications instance that TopBarNav
  rendered inside an always-hidden div; it fired a second notifications
  API request on every page load for UI that could never be seen
- Use functional setStore in the boot effect to avoid a stale closure
@abhijitb
lodash cleanup and memoization for better performance
a89cd5a
@abhijitb
memoization part 2
99d634d
@abhijitb
Complete remaining memoization items and reinstate react-use
2143bd0 
- Memoize TopBarNav's nav-links array with useMemo (was documented as
  done but wasn't actually implemented)
- Wrap AutomaticUpdates.toggleAutoUpdatesAll in useCallback (same gap)
- Verify TopBarNav/MobileNav's setIsOpen(false)-on-navigation effects
  are already no-ops when already closed; no debounce needed
- Reinstate react-use (removed earlier for dependency hygiene) since
  the upcoming @newfold/ui-component-library upgrade requires it as a
  dependency anyway; drop the local useUpdateEffect hook in favor of
  react-use's version across the six settings pages that used it
@abhijitb
Fill in Phase 6.4 bundle size comparison table
9733663 
Replace the TBD placeholders with actual measured before/after/final
numbers from the current build (build/2.3.4). Notes that "After
Splitting" duplicates "Before" since route-level code splitting was
already present in the Jul 16 baseline build, so this table isolates
the impact of the memoization/lodash/context work done this session:
+24 KB (+16%) on index.js, no change to index.css.
@abhijitb
Remove react-use again; document PR #588 and lodash findings
d45eff6 
react-use was reinstated last commit on the assumption that
@newfold/ui-component-library's latest release requires it. Checked
the actual published package on npm (1.3.0 and 2.1.1, the version
bumped to in #588) — neither declares or
uses react-use. With no dependency forcing it in, and confirmed ~0 KB
bundle impact either way (Settings chunk was byte-identical with and
without it), removed it again and restored the local useUpdateEffect
hook.

Also tested and documented two other bundle-size questions:
- Merging PR #588 (UI-library 1.3.0 -> 2.1.1): regresses index.js by
  +25 KB and total JS by +71 KB, mostly from the @headlessui/react v1
  -> v2 bump. Its lodash dependency is unchanged between versions, so
  it doesn't address the size problem this branch already flagged.
  Recommend not merging for size reasons.
- Dropping our explicit `lodash` dependency in favor of the UI
  library's transitive one: confirmed 0 byte saving via `npm ls
  lodash` (already a single deduped copy). Kept it explicit rather
  than accept a phantom-dependency risk for no size benefit.
@abhijitb
Add measured re-render evidence (A/B console instrumentation)
09ac691 
React DevTools Profiler's panel isn't scriptable from this session's
browser-automation tooling, so re-render counts were measured directly
instead: temporary console.log instrumentation on AppBody and the six
settings-section components, built once from the branch's base commit
(via a throwaway git worktree) and once from this branch, same
Coming-Soon-toggle interaction against both in the browser.

Result: one toggle caused 9 unrelated re-renders before this branch's
work, 4 after (-56%) -- AppBody's unrelated re-render eliminated
entirely (context-churn fix), the four settings-section components
each dropped from 2 renders to 1 (context-stabilization fix removed
the double-fire, but each still re-renders once because those six
components aren't wrapped in React.memo -- flagged as a separate,
still-open follow-up rather than claimed as fixed here).

No source files changed; this is a docs-only update recording the
methodology and results.
@abhijitb
Wrap settings-section components in React.memo
3bd51e3 
The Phase 1.2 re-render measurement found that four unrelated
settings sections still re-rendered once per Coming Soon toggle even
after the context-stabilization fixes, because none of the six
top-level settings components (ComingSoon, AutomaticUpdates,
WonderBlockSettings, ContentSettings, CommentSettings,
PerformanceFeatureSettings) were memoized -- so their unmemoized
parent (the Settings page, itself a Context consumer of AppStore)
re-rendering was enough to re-render them regardless of their own
props being unchanged.

All six take zero props, so React.memo is a safe, correct fix with no
edge cases to worry about. Verified with the same before/after
console-instrumentation method used for the original measurement:
AutomaticUpdates, ContentSettings, and CommentSettings drop from 1
re-render to 0 per unrelated settings write. WonderBlockSettings stays
at 1 -- it (and ComingSoon, PerformanceFeatureSettings) call
useContext(AppStore) directly at their own top level, and React.memo
cannot prevent a component's own direct context subscription from
re-rendering it. Eliminating that one would require splitting AppStore
into narrower per-domain contexts, a materially bigger and riskier
change left as a follow-up rather than attempted here.

Net result measured end to end: one Coming Soon toggle now causes 1
unrelated re-render, down from 9 before this branch's work (-89%).
@github-actions

github-actions Bot commented Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown

AI code review

✅ Strengths

  • Clear focus on runtime performance with measurable wins: route-level splitting for heavy pages, memoization across presentational components, and stable context values to reduce cascading re-renders.
  • Fixes two critical correctness issues: ErrorBoundary fallback now passes a component reference (react-error-boundary API), and the public path logic is corrected to point at the versioned build dir (avoids 404s on lazy chunks).
  • Duplicate notifications instance removed (TopBarNav), halving that network request. Notifications are now lazy-loaded in both AppBody and SideNav to keep them out of the main bundle.
  • Thoughtful DX/tooling improvements: analyzer scripts added/fixed; lodash per-function imports plus native replacements; Tailwind content globs tightened to avoid scanning minified bundles; SCSS deprecation warning silenced in loader config.
  • Good separation of concerns: introduced AppBootContext so AppBody doesn’t re-render on every store write; functional state update for setStore to prevent stale closures.
  • Detailed documentation of methodology and measurements in docs/react-performance-optimisation-plan.md is excellent and sets a high bar for change logs and future audits.

❌ Issues (blocking)

  • AppRoutes default export appears to be missing in src/app/data/routes.js, but src/app/index.js imports it as a default export: import AppRoutes from 'App/data/routes';. The routes file currently defines export const AppRoutes = () => { ... } without a matching export default AppRoutes;. This will break the build. Action: either add export default AppRoutes; at the bottom of routes.js or update the import to import { AppRoutes } from 'App/data/routes';.

⚠️ Suggestions / non-blocking

  • Inconsistent dynamic import paths for Notifications:
    • src/app/components/app-nav/index.js uses @modules/wp-module-notifications/...
    • src/app/index.js uses a relative vendor path ../../vendor/newfold-labs/wp-module-notifications/...
      While both likely resolve to the same file at build time, prefer a single canonical import (the alias) to avoid the risk of duplicate chunks or resolution surprises in future refactors.
  • Minor accessibility/content nit: the MobileNav button’s sr-only label reads “Open Navingation Menu” (typo). Consider correcting to “Open Navigation Menu”.
  • Dead import: NSIcon in src/app/components/app-nav/index.js is imported but not used. Removing it will avoid unnecessary bundle bloat.
  • Swallowing errors in useMarketplaceSubnavRoutes is fine from a UX perspective, but consider a low-verbosity console.debug on failure to aid troubleshooting.
  • Validation: The new public path code runs at module eval. Please double-check this file is imported at the very start of the entry (before any dynamic import usage) so it actually executes before chunks are requested.
  • package-lock drift: react-use still appears in package-lock despite source removing it. If not truly a transitive dep, consider a clean install (npm ci) or prune to keep the lock lean and reduce future noise.

Testing / verification notes

  • Please smoke-test route navigation to each lazy page (Marketplace, Settings, Help, Admin, AI Designer) to confirm Suspense boundaries work and chunks resolve under typical and cached conditions.
  • Verify Notifications still display and dismiss correctly in both contexts after memoization and lazy loading.
  • Sanity-check that the “Performance” route visibility still matches NewfoldFeatures at runtime (no unexpected hiding when features are available on initial load).

✅ Follow-up vs. prior feedback

  • No prior human reviews to compare; this set looks internally consistent with the PR description and docs.

⚠️ Verdict: merge with minor follow-ups

Overall this is a solid performance-focused PR with clear, measured gains and multiple correctness fixes. There is one blocking issue to resolve (AppRoutes default export vs. import). After that, the remaining items are small polish/consistency changes. Once the export/import is fixed, this should be good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ramyakrishnai ramyakrishnai ramyakrishnai approved these changes
@ajayadav09 ajayadav09 Awaiting requested review from ajayadav09
@diwanshuster diwanshuster Awaiting requested review from diwanshuster

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@abhijitb @ramyakrishnai