fix: avoid remounting layout tree on every render#248
Merged
arbrandes merged 9 commits intoopenedx:mainfrom Apr 27, 2026
Merged
fix: avoid remounting layout tree on every render#248arbrandes merged 9 commits intoopenedx:mainfrom
arbrandes merged 9 commits intoopenedx:mainfrom
Conversation
combineProviders defined a fresh CombinedProvider component inside its reduce on every call, and CombinedAppProvider invoked it on every render. React identifies components by reference, so a new function each render means "different component" - which forces unmount of the previous instance and mount of a new one. Because this provider sits high in the tree, the entire layout below it churned on every parent re-render, producing a visible layout shift on navigation. Replace the dynamic component composition with a direct element-tree composition. Provider component references come from apps[i].providers which are stable, so React reconciles the tree in place. Co-Authored-By: Claude <noreply@anthropic.com>
Replaces lodash.camelcase, lodash.keyby, lodash.memoize, lodash.merge, and lodash.snakecase with a single lodash dependency, accessed via subpath imports (lodash/camelCase, lodash/keyBy, lodash/merge, etc.). Most of the per-function packages are deprecated upstream. One dep instead of nine, with no test config changes needed. Co-Authored-By: Claude <noreply@anthropic.com>
useSlotOperations, useWidgetOperations, useSortedWidgetOperations,
useLayoutForSlotId, useLayoutOptionsForId, and useWidgetOptionsForId
all used a useState(initial) + useEffect(setState(derived)) pattern
as a stand-in for memoization. Two costs:
- Empty first render: every Slot returned [] / null / {} until its
effect ran. Render N showed nothing useful; render N+1 showed the
real output.
- Cascading commits per navigation: each downstream hook had its own
state and its own effect, so per Slot per navigation produced 3-4
commits.
Replace with useMemo throughout. Side benefit: the in-place .sort()
mutation in useSortedWidgetOperations becomes an out-of-place
[...operations].sort().
The original useState/useEffect setup was load-bearing in a way that
isn't obvious: the location-dep effect, by calling setState on
navigation, was the only thing that scheduled the re-render after
useActiveRouteRoleWatcher's effect updated the active-roles global.
With useMemo, condition filters now read globals synchronously during
render, before the role-watcher effect fires.
To preserve correctness without re-introducing the location dep,
isSlotOperationConditionSatisfied now takes activeRoles as a required
parameter. The three filter hooks (useWidgetOperations,
useLayoutForSlotId, useLayoutOptionsForId) subscribe via
useActiveRoles() and pass the result through, which makes activeRoles
a real memo dep that triggers re-evaluation when roles change.
Co-Authored-By: Claude <noreply@anthropic.com>
createWidgets is only called by useWidgetsForId, whose input comes from useWidgetOperations - which has already filtered out operations whose conditions don't apply. The duplicate isSlotOperationConditionSatisfied check inside createWidgets was dead code. Removing it lets us drop the getActiveRoles + isSlotOperationConditionSatisfied imports from widget/utils.tsx, and removes the corresponding mock from widget/utils.test.tsx (which was masking the redundancy). Co-Authored-By: Claude <noreply@anthropic.com>
useSiteEvent's effect re-runs whenever its callback identity changes. The previous inline arrow created a new function every render, so every render of useActiveRoles caused an unsubscribe + subscribe pair. Every Slot in the tree consumes this hook, so the churn was tree-wide. Wrapping the callback in useCallback removes it. Co-Authored-By: Claude <noreply@anthropic.com>
Now that every Slot subscribes via useActiveRoles(), an unconditional publish on every navigation re-renders every Slot once for nothing. Guard all three role mutators so they publish only on actual transitions: setActiveRouteRoles uses lodash.isEqual for the array comparison; the two widget role mutators publish only on the 0->1 and 1->0 transitions. Includes tests for the publish-on-transition contract. Co-Authored-By: Claude <noreply@anthropic.com>
The hook had a useCallback wrapper around findActiveRouteRoles whose comment claimed it was used "to populate the default state value" - but the hook never had local state to populate. Vestigial from a draft. Folding the loop into the effect drops the redundant dependency and removes the misleading comment. Co-Authored-By: Claude <noreply@anthropic.com>
mergeSiteConfig and mergeAppConfig used lodash.merge with the live config
as the target, so the global reference never changed across publishes.
React consumers that did setState(getSiteConfig()) on CONFIG_CHANGED were
handed the same object and skipped the update via Object.is, so post-init
config changes silently failed to propagate.
Switch the top-level merges to merge({}, ...) so each call produces a
fresh deep-cloned reference. The apps full-merge path gets {} as its
keyBy target, and the limitAppMergeToConfig path replaces its mutating
for-loop with a map that rebuilds each affected app via spread + a
non-mutating merge of its config.
Three initialize tests were captureing getSiteConfig() before init and
asserting against the captured ref, which only passed because of this
bug; switched them to read getSiteConfig() after init.
Co-Authored-By: Claude <noreply@anthropic.com>
Slot renders from frequently-re-rendering parents with mostly stable props. memo with the default shallow comparator stops the cascade, skipping calls into SlotRenderer and DefaultSlotLayout when props are unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
diana-villalvazo-wgu
approved these changes
Apr 27, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Fixes #180.
On every navigation (including tab-to-tab navigation inside an app with relative routes), the entire layout tree below
CombinedAppProviderwas unmounting and remounting. TheHeaderandFooterre-mounted across several later commits, so for ~50-70ms the page rendered without a header above the content - that is the visible layout shift / header reload.The root cause is in
CombinedAppProvider:combineProvidersdefines a fresh React component reference on every render. React identifies components by reference, so a new function each render means "different component" and forces an unmount of the previous instance plus a fresh mount of the entire subtree. Replacing the dynamic component composition with a direct element-tree composition fixes the primary bug.The remaining commits address perf issues discovered along the way: the slot hook
setState-in-effect cascade is collapsed intouseMemo,ACTIVE_ROLES_CHANGEDno longer publishes on every navigation when the roles haven't actually changed, and theuseActiveRolessubscription callback is stabilized so it doesn't unsubscribe and resubscribe on every render of every Slot. The lodash per-function packages (most of which are deprecated upstream) are consolidated into a singlelodashdependency with subpath imports.A separate latent bug surfaced while investigating the above:
mergeSiteConfigandmergeAppConfigwere callinglodash.mergewith the live config as the target, mutating it in place. Consumers that didsetState(getSiteConfig())onCONFIG_CHANGEDwere handed the same object reference and silently skipped the update via React'sObject.ischeck, so post-init config changes never propagated. The merges now produce a fresh top-level reference (merge({}, ...)) and the apps paths build new arrays/objects instead of mutating existing entries.No public API changes. No consumer changes required.
Before and After
As can be seen from the render debug (warmer/more permanent highlights == more renders == worse), there's potentially more we can do to optimize performance, but that's left as a future endeavor. For now, the header's performance is already much "bluer", and at least nothing's unmounting/mounting that shouldn't:
Before:
before.mp4
After:
after.mp4
LLM usage notice
Built with assistance from Claude.