-
Notifications
You must be signed in to change notification settings - Fork 10
Cache XXL breakpoint CSS to improve breakpoint switching performance #6149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Cache XXL breakpoint CSS to improve breakpoint switching performance #6149
Conversation
📝 WalkthroughWalkthroughThis PR introduces XXL breakpoint style caching in MaxiBlockComponent to avoid redundant style regeneration. It adds internal cache fields, marks the cache as dirty when breakpoint/content changes occur, and reuses cached styles under specific conditions. Three method signatures are extended to propagate a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom Pre-merge Checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/extensions/maxi-block/maxiBlockComponent.js (1)
657-783: Consider clearing cache on unmount (optional).While not strictly necessary (strings are garbage collected with the component), you could add explicit cache cleanup in
componentWillUnmountfor consistency with the existing cleanup pattern:this.xxlStyleCache = null; this.isXxlStyleCacheDirty = false;This is purely a hygiene suggestion and has no functional impact.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/extensions/maxi-block/maxiBlockComponent.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: e2e-js
- GitHub Check: unit-js
- GitHub Check: e2e-js-playwright
🔇 Additional comments (10)
src/extensions/maxi-block/maxiBlockComponent.js (10)
100-101: LGTM!Instance properties for the XXL style cache are appropriately initialized. Using instance properties (rather than React state) avoids unnecessary re-renders for internal cache management.
564-575: LGTM!The cache invalidation logic correctly marks the XXL cache as dirty when any style-affecting change occurs: non-relation attribute changes, base breakpoint changes, or block style changes. This ensures stale styles won't be served.
580-589: LGTM!The new
isBaseBreakpointChangeparameter is correctly computed and passed, mirroring the existing pattern for detecting valid breakpoint transitions (excluding initial undefined→value cases).
1377-1381: LGTM!The updated signature maintains backward compatibility via the default value, and the new parameter is correctly threaded through to downstream methods.
1422-1432: LGTM!The new parameter is correctly passed through the
injectStylescall to enable cache-aware style generation.
1544-1554: LGTM!The signature update correctly adds
isBaseBreakpointChangein the parameter chain while maintaining the existing parameter order.
1563-1573: LGTM!The new parameter is correctly threaded through to
generateStyleContentwhere the caching logic resides.
1748-1758: LGTM!The signature correctly includes the new
isBaseBreakpointChangeparameter for cache validity decisions.
1783-1791: LGTM!The cache hit conditions are appropriately defensive: requiring a breakpoint change to XXL, no block style change, no base breakpoint change, existing cache, and clean cache state. This provides multiple layers of validation before serving cached CSS.
1839-1842: LGTM!The cache is correctly updated after style generation and processing, but only when
styleContentis truthy. The dirty flag is properly reset to ensure subsequent XXL breakpoint switches can benefit from the cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24c9105337
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| currentBreakpoint === 'xxl' && | ||
| !isBlockStyleChange && | ||
| !isBaseBreakpointChange && | ||
| this.xxlStyleCache && | ||
| !this.isXxlStyleCacheDirty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Invalidate XXL cache on inherited blockStyle changes
This cached-XXL branch will reuse the previous xxlStyleCache whenever isXxlStyleCacheDirty stays false. That flag is only set from attribute diffs, but getCurrentBlockStyle() can update attributes.blockStyle after the diff (e.g., when a parent style-card changes), so the dirty flag never flips. In that scenario, switching to the XXL breakpoint reuses CSS that still references the old --maxi-<style>- variables, so colors stay wrong until another style edit forces regeneration. Consider marking the cache dirty whenever getCurrentBlockStyle() updates the style (or when SC context changes).
Useful? React with 👍 / 👎.
Motivation
Description
xxlStyleCacheandisXxlStyleCacheDirtytoMaxiBlockComponentto avoid unnecessary regeneration of XXL CSS.isBaseBreakpointChangeparameter throughdisplayStyles,injectStyles, andgenerateStyleContentto make cache validity decisions aware of base breakpoint changes.xxlbreakpoint ifxxlStyleCacheexists andisXxlStyleCacheDirtyisfalse, and save the generated XXL CSS back toxxlStyleCachewhen computed.baseBreakpointchanges, orblockStylechanges) so the cache is invalidated and recomputed when necessary.Testing
npm test) after pulling to verify integration and performance improvements.Codex Task
Summary by CodeRabbit
Performance
✏️ Tip: You can customize this high-level summary in your review settings.