fix custom css prefixing class on style duplicate #2748
Conversation
|
Warning Rate limit exceeded@Liviu-p has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 1 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughStyle duplication now preserves the original style name in Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller as FrmStylesController
participant Model as FrmStyle
participant View as _quick-settings.php
User->>Controller: Request duplicate style page
Controller->>Model: clone active style
Controller->>Model: get_new() to create new style (generate sanitized post_name)
Model-->>Controller: returns new_style with sanitized post_name
Controller->>Controller: set cloned_style.post_content['old_name_before_duplicate'] = original post_name
Controller->>Controller: set cloned_style.post_name = new_style.post_name
Controller->>View: render quick-settings with cloned_style
View->>View: compute scoped_name = old_name_before_duplicate ?? post_name
View->>View: unnest/scoped CSS using "frm_style_" + scoped_name
View-->>User: rendered duplicate style page
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @classes/models/FrmStyle.php:
- Around line 127-128: The CSS scope is being computed from
sanitize_title($new_instance['post_title']) and injected into
$new_instance['post_content'] before saving, so if WP alters the slug in
wp_insert_post() (via wp_unique_post_slug()) the saved scope (e.g.,
frm_style_my-style) can mismatch the real post_name (e.g., my-style-2); after
calling wp_insert_post() and getting the returned post ID, load the saved post
(get_post($id)) and compare its post_name to the originally calculated slug, and
if they differ update the nested CSS scope inside post_content to use the actual
post_name (e.g., replace the old scope token with
"frm_style_{$post->post_name}") and persist the change with wp_update_post() so
the CSS scope matches the final post_name.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
classes/controllers/FrmStylesController.phpclasses/models/FrmStyle.phpclasses/views/styles/_quick-settings.phpjs/formidable-web-components.jsjs/formidable_admin.js
🧰 Additional context used
🧬 Code graph analysis (2)
classes/controllers/FrmStylesController.php (1)
classes/models/FrmStyle.php (1)
get_new(41-60)
classes/models/FrmStyle.php (1)
classes/helpers/FrmCssScopeHelper.php (1)
nest(17-91)
🪛 Biome (2.1.2)
js/formidable-web-components.js
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
[error] 1-1: Do not reassign a function declaration.
Reassigned here.
Use a local variable instead.
(lint/suspicious/noFunctionAssign)
🪛 GitHub Actions: Psalm Code Analysis
classes/controllers/FrmStylesController.php
[error] 636-636: Psalm: UndefinedPropertyAssignment: Instance property WP_Post::$old_name_before_duplicate is not defined (see https://psalm.dev/038)
🪛 GitHub Check: Psalm
classes/controllers/FrmStylesController.php
[failure] 636-636: UndefinedPropertyAssignment
classes/controllers/FrmStylesController.php:636:5: UndefinedPropertyAssignment: Instance property WP_Post::$old_name_before_duplicate is not defined (see https://psalm.dev/038)
🔇 Additional comments (3)
classes/views/styles/_quick-settings.php (1)
255-258: LGTM! CSS scoping logic for duplicates is correct.The introduction of
$scoped_style_nameproperly handles the style duplication flow:
- During duplication, it uses
old_name_before_duplicateto unnest CSS from the original scope- For normal edits, it falls back to
post_nameThis ensures CSS is correctly extracted for editing regardless of whether the style is being duplicated or edited.
classes/models/FrmStyle.php (1)
130-133: LGTM! CSS nesting logic complements the unnesting on display.This nesting on save works correctly with the unnesting in
_quick-settings.php:
- Display: CSS is unnested for editing
- Save: CSS is nested under the style's scope
- Storage: CSS is stored nested in the database
The flow properly handles both new styles and duplicates.
classes/controllers/FrmStylesController.php (1)
633-638: Duplication logic is correct, but Psalm error should be addressed.The duplication flow properly:
- Clones the style to preserve content
- Generates a new random post_name
- Tracks the original name for CSS scoping
However, Line 636 triggers a Psalm error because
old_name_before_duplicateis not a defined property ofWP_Post. This is also causing the pipeline failure.Consider adding a Psalm suppression comment or using a more type-safe approach:
🔧 Options to resolve the Psalm error
Option 1: Suppress the Psalm warning
case 'duplicate': $style = clone $active_style; $new_style = $frm_style->get_new(); $style->ID = $new_style->ID; + /** @psalm-suppress UndefinedPropertyAssignment */ $style->old_name_before_duplicate = $active_style->post_name; $style->post_name = $new_style->post_name;Option 2: Use stdClass instead of WP_Post for new/duplicate styles
case 'duplicate': - $style = clone $active_style; + $style = (object) (array) $active_style; $new_style = $frm_style->get_new(); $style->ID = $new_style->ID; $style->old_name_before_duplicate = $active_style->post_name; $style->post_name = $new_style->post_name;
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/views/styles/_quick-settings.php (1)
282-282: Use$scoped_style_namefor thedata-css-scopeattribute.Line 282 uses
$style->post_namedirectly while line 258 unnests CSS using$scoped_style_name. For duplicated styles, this creates a mismatch: the data attribute shows the new random key, but unnesting uses the original name stored inold_name_before_duplicate. Thedata-css-scopeattribute should use$scoped_style_nameto remain consistent with the actual CSS scope used during unnesting.
🤖 Fix all issues with AI agents
In @classes/controllers/FrmStylesController.php:
- Around line 633-637: The code clones $active_style (a WP_Post) then assigns a
dynamic property old_name_before_duplicate which triggers Psalm
UndefinedPropertyAssignment and PHP 8.2 deprecation; to fix, convert the WP_Post
to a stdClass before adding dynamic props (e.g., replace the clone $active_style
line with a conversion from WP_Post to stdClass via (object) array cast or
similar) so that $style is a plain object like the $new_style returned by
get_new(), then proceed to set $style->ID, $style->old_name_before_duplicate and
$style->post_name; ensure you reference the get_one() caller that produces
$active_style and the get_new() usage to keep behavior consistent.
In @classes/models/FrmStyle.php:
- Line 57: Move the inline comment above the array element to satisfy PHPCS:
locate the 'old_name_before_duplicate' => null entry in the FrmStyle class (the
array that defines style properties) and place the comment "Stores the original
post_name when duplicating a style to maintain custom CSS scoping
compatibility." on its own line immediately above that array key, removing the
end-of-line comment so the statement has no post-statement comment.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
classes/controllers/FrmStylesController.phpclasses/models/FrmStyle.phpclasses/views/styles/_quick-settings.php
🧰 Additional context used
🧬 Code graph analysis (2)
classes/models/FrmStyle.php (2)
classes/controllers/FrmStylesController.php (1)
FrmStylesController(6-1493)classes/helpers/FrmCssScopeHelper.php (1)
nest(17-91)
classes/controllers/FrmStylesController.php (1)
classes/models/FrmStyle.php (1)
get_new(41-61)
🪛 GitHub Actions: Inspections
classes/models/FrmStyle.php
[error] 57-57: PHPCS: Squiz.Commenting.PostStatementComment.Found - Comments may not appear after statements. PHPCBF can fix automatically. Command used: './vendor/bin/phpcs --parallel=10 ./'.
🪛 GitHub Actions: Psalm Code Analysis
classes/controllers/FrmStylesController.php
[error] 636-636: Psalm: UndefinedPropertyAssignment - Instance property WP_Post::$old_name_before_duplicate is not defined. (https://psalm.dev/038)
🪛 GitHub Check: Psalm
classes/controllers/FrmStylesController.php
[failure] 636-636: UndefinedPropertyAssignment
classes/controllers/FrmStylesController.php:636:5: UndefinedPropertyAssignment: Instance property WP_Post::$old_name_before_duplicate is not defined (see https://psalm.dev/038)
⏰ 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: Cypress
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: PHP 7.4 tests in WP 6.9
🔇 Additional comments (3)
classes/views/styles/_quick-settings.php (1)
253-258: LGTM! Correct CSS scope handling for duplicated styles.The logic correctly derives the scoped style name by checking
old_name_before_duplicatefirst (populated during duplication) and falling back topost_name. This ensures the CSS unnesting operation uses the appropriate scope identifier that matches the original CSS nesting.classes/models/FrmStyle.php (2)
127-129: LGTM! Proper sanitization of post_name for new styles.The change correctly sanitizes
post_nameusingsanitize_title()when creating a new style, ensuring URL-safe slugs that follow WordPress conventions.
131-134: LGTM! CSS nesting correctly uses determined post_name.The nesting logic appropriately scopes the custom CSS after
post_namehas been determined (sanitized for new styles, unchanged for existing). This ensures the CSS scope identifier matches what will be used during unnesting in the quick settings view.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @classes/controllers/FrmStylesController.php:
- Around line 637-638: Replace the malformed comment prefix so Psalm recognizes
the suppression: change the line starting with "//** @psalm-suppress
UndefinedPropertyAssignment */" to a proper PHPDoc suppression block "/**
@psalm-suppress UndefinedPropertyAssignment */" immediately above the assignment
to $style->old_name_before_duplicate = $active_style->post_name; ensuring the
comment uses /** ... */ syntax so Psalm will ignore the
UndefinedPropertyAssignment for that assignment.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
classes/controllers/FrmStylesController.phpclasses/models/FrmStyle.php
🚧 Files skipped from review as they are similar to previous changes (1)
- classes/models/FrmStyle.php
🧰 Additional context used
🪛 GitHub Actions: Psalm Code Analysis
classes/controllers/FrmStylesController.php
[error] 638-638: Psalm: UndefinedPropertyAssignment. Instance property WP_Post::$old_name_before_duplicate is not defined (see https://psalm.dev/038)
🪛 GitHub Check: Psalm
classes/controllers/FrmStylesController.php
[failure] 638-638: UndefinedPropertyAssignment
classes/controllers/FrmStylesController.php:638:5: UndefinedPropertyAssignment: Instance property WP_Post::$old_name_before_duplicate is not defined (see https://psalm.dev/038)
⏰ 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: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
🔇 Additional comments (1)
classes/controllers/FrmStylesController.php (1)
633-641: Good approach for preserving CSS scoping on duplication.The logic correctly preserves the original style's
post_namein the temporary propertyold_name_before_duplicatewhile assigning a new randompost_namefor the duplicated style. This maintains CSS scoping compatibility as mentioned in the comment. Once the psalm-suppress annotation syntax is fixed, this implementation should work as intended.
No description provided.