Optimizations: Generate CSS Files, disable deprecated code, other speed stuff #3570
Conversation
🤖 Pull request artifacts
|
bfintal
changed the title
# Conflicts: # src/design-library/init.php
WalkthroughAdds a pluggable CSS file generation/caching system, integrates it into initialization and asset loading, introduces settings and REST route to toggle/invalidate CSS files, adjusts typography and breakpoint option handling, scopes a filter to execution time, and updates the admin UI to manage CSS files and trigger invalidation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant WP as WordPress
participant Init as Stackable_Init
participant GenG as Global_CSS_Generator
participant GenB as Block_Inheritance_Generator
Note over WP,Init: Frontend load
WP->>Init: enqueue_scripts
alt Use CSS files enabled AND not admin
Init->>GenG: enqueue_global_css_file()
GenG->>GenG: get_cached_css_file_name()
alt File exists
GenG-->>WP: wp_enqueue_style(file)
else Missing/empty
GenG->>GenG: generate_css_content() and write file
alt Write success
GenG-->>WP: wp_enqueue_style(new file)
else Write fail
GenG-->>WP: wp_add_inline_style()
end
end
Init->>GenB: enqueue_block_inheritance_css_file()
GenB->>GenB: similar existence/generate/inline flow
else Inline fallback
Init->>GenG: enqueue_global_css_inline()
Init->>GenB: enqueue_block_inheritance_css_inline()
end
sequenceDiagram
autonumber
actor Admin
participant UI as Admin UI (welcome/admin.js)
participant REST as /stackable/v3/invalidate-css-files
participant Svc as Stackable_CSS_File_Generator
participant GenG as Global_CSS_Generator
participant GenB as Block_Inheritance_Generator
Admin->>UI: Click "Invalidate CSS Files"
UI->>REST: POST invalidate-css-files
REST->>Svc: invalidate_all_css_files()
Svc->>GenG: invalidate_css_file()
Svc->>GenB: invalidate_css_file()
Svc-->>REST: { success: true }
REST-->>UI: 200 OK
UI-->>Admin: Spinner stops
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/plugins/theme-block-style-inheritance/index.php (2)
78-81: Always return a string from the filter; avoid returning null.Returning early with
return;yieldsnull, which will bubble up toStackable_Block_Style_Inheritance_CSS_Generator::generate_css_content()wheretrim($css_content)expects a string. In PHP 8+,trim(null)throws a TypeError. Return$current_cssinstead.Apply this diff:
- if ( ! isset( $raw_data[ 'styles' ] ) ) { - return; - } + if ( ! isset( $raw_data[ 'styles' ] ) ) { + return $current_css; + }
325-339: Scope the safecss allowlist filter to this generation only (add once, then remove).Registering
safecss_filter_attr_allow_cssinside this method is correct, but it’s never removed. That broadens sanitization for the remainder of the request and can be added multiple times across calls. Guard withhas_filter()andremove_filter()after generation.Apply this diff:
- // Allow some CSS functions like color-mix() to be included in the generated CSS. - add_filter( 'safecss_filter_attr_allow_css', array( $this, 'allow_css' ), 10, 2 ); + // Allow some CSS functions like color-mix() to be included in the generated CSS. + if ( false === has_filter( 'safecss_filter_attr_allow_css', array( $this, 'allow_css' ) ) ) { + add_filter( 'safecss_filter_attr_allow_css', array( $this, 'allow_css' ), 10, 2 ); + } $generated_css = wp_style_engine_get_stylesheet_from_css_rules( $styles ); if ( $generated_css ) { $generated_css = "/* Block style inheritance */\n" . $generated_css; } // Keep the hash and block style inheritance CSS in the cache. set_transient( 'stackable_block_style_inheritance_hash', $hash, 60 * 60 * 24 ); set_transient( 'stackable_block_style_inheritance_css', $generated_css, 60 * 60 * 24 ); $current_css .= $generated_css; - return apply_filters( 'stackable_frontend_css' , $current_css ); + // Remove the temporary filter to avoid side effects on other sanitization runs. + remove_filter( 'safecss_filter_attr_allow_css', array( $this, 'allow_css' ), 10 ); + return apply_filters( 'stackable_frontend_css', $current_css );src/init.php (1)
256-265: Guard admin enqueues & remove deprecated style handleThe legacy style handle
ugb-style-css-nodepis no longer registered and will trigger “not registered” notices if enqueued. Likewise, the frontend scriptugb-block-frontend-jsis only registered when! is_admin(), so unguarded enqueues in the admin will error out.Please update src/init.php in the
block_enqueue_frontend_assets()method:• Remove the deprecated style enqueue on line 262.
• Wrap the frontend‐only script enqueue on line 263 in an admin check and registration guard.Proposed diff:
public function block_enqueue_frontend_assets() { $this->register_frontend_assets(); wp_enqueue_style( 'ugb-style-css' ); if ( is_frontend() ) { $this->enqueue_inline_block_style_inheritance(); } - wp_enqueue_style( 'ugb-style-css-nodep' ); - wp_enqueue_script( 'ugb-block-frontend-js' ); + // Remove deprecated handle to avoid “not registered” notices. + // Only enqueue frontend script when registered and not in the admin. + if ( ! is_admin() && wp_script_is( 'ugb-block-frontend-js', 'registered' ) ) { + wp_enqueue_script( 'ugb-block-frontend-js' ); + } do_action( 'stackable_block_enqueue_frontend_assets' ); }Optional BC fallback: if you must support
ugb-style-css-nodepin the short term, add a no‐opwp_register_style( 'ugb-style-css-nodep', false );before this enqueue, then deprecate it in the next minor release.
🧹 Nitpick comments (6)
src/css-file-generator.php (3)
60-68: Consider adding error handling for timestamp generation.While the timestamp-based cache busting is effective, consider what happens if the system time is incorrect or if multiple rapid generations occur within the same second.
Consider adding microseconds for more granular timestamps:
public static function get_css_file_name() { $css_content = static::generate_css_content(); $content_hash = md5( $css_content ); // Add a timestamp as cache buster to ensure unique file names - $timestamp = time(); + $timestamp = microtime( true ) * 10000; return static::get_css_file_prefix() . $content_hash . '-' . $timestamp . '.css'; }
218-218: Potential race condition when deleting files.The
unlink()operation could fail if the file is being accessed. Consider adding error suppression or checking the result.if ( file_exists( $old_file_path ) ) { - unlink( $old_file_path ); + @unlink( $old_file_path ); }
428-428: Unused parameter can be removed.The static analysis correctly identifies that
$new_versionis unused. Since this is a callback, you could simplify it.-public function register_use_css_files_setting_upgraded( $old_version, $new_version ) { +public function register_use_css_files_setting_upgraded( $old_version ) { if ( ! empty( $old_version ) && version_compare( $old_version, "3.19.0", "<" ) ) { update_option( 'stackable_use_css_files', '', 'no' ); } }plugin.php (1)
336-336: Consider documenting the constant in the main documentation.The
STACKABLE_DISABLE_DEPRECATED_CODEconstant should be documented for users who want to optimize performance.Would you like me to help create documentation for this performance optimization constant, explaining when and how users should define it?
src/plugins/theme-block-style-inheritance/index.php (1)
32-37: Use a boolean sanitizer for a boolean option.The setting is declared as
type => 'boolean'but usessanitize_text_field. Preferrest_sanitize_boolean(orwp_validate_boolean) and a boolean default for correctness and REST parity.Apply this diff:
- 'type' => 'boolean', + 'type' => 'boolean', 'description' => __( 'Inherit block style from theme.json', STACKABLE_I18N ), - 'sanitize_callback' => 'sanitize_text_field', + 'sanitize_callback' => 'rest_sanitize_boolean', 'show_in_rest' => true, - 'default' => '', + 'default' => false,Follow-up (optional): In
block_style_inheritance_set_default(), store a real boolean:- update_option( 'stackable_disable_block_style_inheritance', 'true' ); + update_option( 'stackable_disable_block_style_inheritance', true );src/init.php (1)
115-117: Consider registering a script handle without a null src.Registering with
nullcan lead to an emptysrcattribute on some setups. If a placeholder handle is needed, considerfalse(no script tag) or a tiny internal stub.Apply this diff:
- wp_register_script( 'ugb-block-frontend-js', null, [], STACKABLE_VERSION ); + // Register a placeholder handle without emitting a <script src>. + wp_register_script( 'ugb-block-frontend-js', false, [], STACKABLE_VERSION, true );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
plugin.php(2 hunks)src/css-file-generator.php(1 hunks)src/dynamic-breakpoints.php(1 hunks)src/global-settings.php(2 hunks)src/init.php(6 hunks)src/plugins/theme-block-style-inheritance/index.php(1 hunks)src/welcome/admin.js(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/css-file-generator.php (2)
src/init.php (1)
__construct(29-68)src/components/__experimental-multi-post-picker-control/index.php (1)
register_rest_route(29-42)
src/init.php (1)
src/css-file-generator.php (6)
Stackable_Global_Design_System_CSS_Generator(233-305)enqueue_global_css_file(295-297)enqueue_global_css_inline(302-304)Stackable_Block_Style_Inheritance_CSS_Generator(315-383)enqueue_block_inheritance_css_file(373-375)enqueue_block_inheritance_css_inline(380-382)
src/welcome/admin.js (2)
src/components/admin-toggle-setting/index.js (1)
AdminToggleSetting(7-43)src/deprecated/v2/components/font-awesome-icon/index.js (1)
Spinner(10-12)
🪛 PHPMD (2.15.0)
src/css-file-generator.php
428-428: Avoid unused parameters such as '$new_version'. (Unused Code Rules)
(UnusedFormalParameter)
460-460: Avoid unused parameters such as '$request'. (Unused Code Rules)
(UnusedFormalParameter)
⏰ 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). (6)
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 7.3 and WP 6.5.5
🔇 Additional comments (15)
src/dynamic-breakpoints.php (1)
70-70: LGTM! Providing explicit default enhances predictability.The change to explicitly return
falsewhen the option is absent improves code clarity and aligns with the pattern used throughout the PR for option retrieval.src/global-settings.php (4)
73-73: Good optimization with the guard clause.Using
has_global_typography()as an early guard prevents unnecessary processing when global typography isn't configured.
77-81: Smart optimization to avoid redundant filter registration.Caching
get_apply_typography_to()result and conditionally registering therender_blockfilter only when needed reduces unnecessary hook calls.
565-581: Well-structured helper method for typography check.The
has_global_typography()method provides a clean, centralized way to check for global typography presence with proper validation of the nested array structure.
589-589: Consistent with the explicit default pattern.Using
get_option('stackable_global_typography', false)maintains consistency with the option retrieval pattern established across the PR.src/css-file-generator.php (2)
23-75: Well-designed abstract base class for CSS generation.The abstract class provides a solid foundation with proper separation of concerns through abstract methods and helpful utility methods for file operations.
460-478: LGTM! Clean REST endpoint implementation.The invalidation endpoint is well-structured with proper error handling and appropriate permission checks.
src/welcome/admin.js (2)
1159-1159: Good state management for async operation.The
isInvalidatingCssFilesstate properly tracks the invalidation process.
1193-1226: Well-implemented CSS file generation toggle with clear help text.The UI implementation is intuitive with the toggle control, helpful description, and invalidation button. The spinner provides good visual feedback during the operation.
plugin.php (2)
277-277: Good placement of CSS generator initialization.Loading the CSS file generator after kses.php ensures proper sanitization is available when needed.
336-353: Excellent use of conditional loading for deprecated code.The
STACKABLE_DISABLE_DEPRECATED_CODEconstant provides a clean way to exclude deprecated code for performance gains. This is particularly useful for sites that don't need backward compatibility.src/init.php (4)
119-126: LGTM: CSS file generation with inline fallback is wired correctly.The new global CSS file path respects the
stackable_use_css_filesoption and falls back to inline for admin or when disabled.
235-237: LGTM: Global one-time enqueue action guarded with did_action.This avoids repeated execution across multiple blocks while keeping the original
$block_content, $blockpayload.
241-249: Good encapsulation of theme.json inheritance enqueue.Centralizing block style inheritance enqueue behind a single method that respects the CSS-files toggle is a clean design and improves maintainability.
160-181: Verified single enqueue path for frontend assetsI’ve confirmed that
block_enqueue_frontend_assets()is only ever invoked in the following guarded locations, each checking and then setting$this->is_main_script_loadedto prevent duplicate loads:
- maybe_force_css_load() (uses
apply_filters('stackable_force_css_load', false))- load_frontend_scripts_conditionally_head()
- load_frontend_scripts_conditionally()
- load_frontend_scripts_conditionally_footer()
Additionally, there are no other direct calls to
wp_enqueue_style('ugb-style-css')elsewhere in the codebase. Assets will only be enqueued once.No further changes are required. You may optionally remove or disable the force-load filter after its first run if you ever encounter unexpected duplicates.
| $file_path = static::get_css_file_path() . $file_name; | ||
|
|
||
| // Write the CSS file | ||
| $result = file_put_contents( $file_path, $css_content ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for file write operations.
The file write operation should handle potential failures more gracefully and provide error logging.
// Write the CSS file
$result = file_put_contents( $file_path, $css_content );
+
+if ( $result === false ) {
+ error_log( 'Stackable: Failed to write CSS file to ' . $file_path );
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $result = file_put_contents( $file_path, $css_content ); | |
| // Write the CSS file | |
| $result = file_put_contents( $file_path, $css_content ); | |
| if ( $result === false ) { | |
| error_log( 'Stackable: Failed to write CSS file to ' . $file_path ); | |
| } |
🤖 Prompt for AI Agents
In src/css-file-generator.php around line 113, the file_put_contents call does
not handle write failures; modify the code to check the return value of
file_put_contents (it returns false on failure), and when it fails obtain error
details (use error_get_last() or wrap in try/catch if using exceptions), log a
clear error message including the $file_path and the error message, and return
or throw an appropriate error/boolean so callers can react; also consider
ensuring the directory is writable and create it if missing before writing.
| // Enqueue the block script once. | ||
| if ( ! isset( $this->scripts_loaded[ $block['blockName'] ] ) ) { | ||
| $stackable_block = substr( $block['blockName'], 10 ); | ||
| do_action( 'stackable/' . $stackable_block . '/enqueue_scripts' ); | ||
| $this->scripts_loaded[ $block['blockName'] ] = true; | ||
| if ( did_action( 'stackable/' . $block['blockName'] . '/enqueue_scripts' ) === 0 ) { | ||
| do_action( 'stackable/' . $block['blockName'] . '/enqueue_scripts' ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Incorrect action name for per-block enqueue; double “stackable/” prefix.
$block['blockName'] already includes the stackable/ namespace (e.g., stackable/columns). Prepending another stackable/ yields stackable/stackable/columns/enqueue_scripts, breaking any listeners expecting stackable/columns/enqueue_scripts.
Apply this diff:
- if ( did_action( 'stackable/' . $block['blockName'] . '/enqueue_scripts' ) === 0 ) {
- do_action( 'stackable/' . $block['blockName'] . '/enqueue_scripts' );
+ // $block['blockName'] is like 'stackable/columns'
+ $per_block_action = $block['blockName'] . '/enqueue_scripts';
+ if ( did_action( $per_block_action ) === 0 ) {
+ do_action( $per_block_action );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Enqueue the block script once. | |
| if ( ! isset( $this->scripts_loaded[ $block['blockName'] ] ) ) { | |
| $stackable_block = substr( $block['blockName'], 10 ); | |
| do_action( 'stackable/' . $stackable_block . '/enqueue_scripts' ); | |
| $this->scripts_loaded[ $block['blockName'] ] = true; | |
| if ( did_action( 'stackable/' . $block['blockName'] . '/enqueue_scripts' ) === 0 ) { | |
| do_action( 'stackable/' . $block['blockName'] . '/enqueue_scripts' ); | |
| } | |
| // Enqueue the block script once. | |
| // $block['blockName'] is like 'stackable/columns' | |
| $per_block_action = $block['blockName'] . '/enqueue_scripts'; | |
| if ( did_action( $per_block_action ) === 0 ) { | |
| do_action( $per_block_action ); | |
| } |
🤖 Prompt for AI Agents
In src/init.php around lines 227 to 231, the code prepends "stackable/" to
$block['blockName'] when building the action name, causing a double
"stackable/stackable/..." prefix; update both did_action and do_action calls to
use $block['blockName'] . '/enqueue_scripts' ( i.e. remove the extra
"stackable/" prefix ) so the action name matches blocks like
"stackable/columns/enqueue_scripts".
| // Show a spinner while the API call is in progress | ||
| setIsInvalidatingCssFiles( true ) | ||
| apiFetch( { | ||
| path: '/stackable/v3/invalidate-css-files', | ||
| method: 'POST', | ||
| } ).finally( () => { | ||
| setIsInvalidatingCssFiles( false ) | ||
| } ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the API call.
The API call should handle errors gracefully and provide user feedback.
apiFetch( {
path: '/stackable/v3/invalidate-css-files',
method: 'POST',
-} ).finally( () => {
+} ).then( () => {
+ // Optionally show success message
+} ).catch( ( error ) => {
+ console.error( 'Failed to invalidate CSS files:', error );
+ // Optionally show error message to user
+} ).finally( () => {
setIsInvalidatingCssFiles( false )
} )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Show a spinner while the API call is in progress | |
| setIsInvalidatingCssFiles( true ) | |
| apiFetch( { | |
| path: '/stackable/v3/invalidate-css-files', | |
| method: 'POST', | |
| } ).finally( () => { | |
| setIsInvalidatingCssFiles( false ) | |
| } ) | |
| // Show a spinner while the API call is in progress | |
| setIsInvalidatingCssFiles( true ) | |
| apiFetch( { | |
| path: '/stackable/v3/invalidate-css-files', | |
| method: 'POST', | |
| } ).then( () => { | |
| // Optionally show success message | |
| } ).catch( ( error ) => { | |
| console.error( 'Failed to invalidate CSS files:', error ) | |
| // Optionally show error message to user | |
| } ).finally( () => { | |
| setIsInvalidatingCssFiles( false ) | |
| } ) |
🤖 Prompt for AI Agents
In src/welcome/admin.js around lines 1206 to 1213, the API call to
'/stackable/v3/invalidate-css-files' lacks error handling and only toggles the
spinner; update the call to handle rejections by adding a .catch (or convert to
async/await with try/catch) that captures the error, logs it (console.error or
process logger), and surfaces user feedback (set an error state or call the
existing UI notification/toast function with a clear message); keep the .finally
to clear setIsInvalidatingCssFiles(false) so the spinner always stops.
|
Block errors in editor: |
This Pull Request does the following optimizations
typography_detect_native_blocksif not using global typographySTACKABLE_DISABLE_DEPRECATED_CODEto disable all backward compatibility code in-lieu for speeddid_actiontoload_frontend_scripts_conditionallyto lessen action callssafecss_filter_attr_allow_cssonly during style generation CC @mxkaestackable_block_style_inheritance_inline_styles_nodepgeneration of block style inheritance only when neededstackable_use_css_filesdefaults to checked (yes)Remaining work to be done
Changethis was slow!load_frontend_scripts_conditionally, remove old checker, and instead of hooking torender_block, hook to allrender_block_{name}and then remove hook when completed each.See ifwp_get_global_stylescan be used instead ofWP_Theme_JSON_Resolveras recommended by WP docs CC @mxkaeSummary by CodeRabbit
New Features
Performance
Bug Fixes