Repository navigation
Defer loading codelist icons - #3511
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 47 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughField-code rendering can defer icon markup in ChangesDeferred code-list icons
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant FrmFormsHelper
participant showShortcodeBox
participant hydrateDeferredCodeListIcons
FrmFormsHelper->>showShortcodeBox: Provides code-list items with data-frm-icon
showShortcodeBox->>hydrateDeferredCodeListIcons: Hydrates icons before displaying the panel
hydrateDeferredCodeListIcons->>showShortcodeBox: Inserts icon HTML and removes data-frm-icon
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change defers field icon rendering until the shortcode panel opens. No merge-blocking risk is evident from the supplied review. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new icon-loading path is limited to the form editor in the inspected code, and its generated markup uses escaped values. No exploitable path was established, but the new browser insertion step and incomplete production-script coverage prevent a minimal-risk assessment. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Hardening Proposals
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
Request Changes. The popup itself works. There are two blocking items: PHPCS is red on a line this PR adds, and the default tags box now loses its icons in the Views editor.
Blocking
Run PHPCS inspectionfails on a line this PR adds,classes/views/shared/mb_adv_info.php:80(multi-item array with explicit keys on one line). One-click fix is inline.defer_icon => trueis hard-coded in the shared default template, but that template is also rendered visible, not in the popup.mb_tags_box()with the default template is what Views'frm_get_cd_tags_boxAJAX returns after you pick a form. Nothing hydrates the icons there, so the Fields list loses them. Details, screenshot and a fix are inline on:76.
Non-blocking
- The
clickTabhydrate call (admin.js:1059) finds nothing to hydrate. Inline. hydrateDeferredCodeListIcons()copiesicon_by_class()'s marker parsing and deprecated-icon map into JS. Inline.- No test was added. A small Cypress check (open the popup, expect an
svgin each.frm_customize_field_list li, no[data-frm-icon]left) would pin the behaviour. Blocking item 2 lives in Pro/Views, so this wouldn't have caught it.
What I checked, and what I didn't
- Ran Lite at the PR head in Playground (WP 7.0, SQLite). Form settings > Customize HTML, opening the popup from a
.frm-show-boxicon: before opening, 5li[data-frm-icon]and 0svg. After opening, 0 deferred and 10svg(5 items x the id and key links), each<svg class="frmsvg" aria-hidden="true"><use href="#frm_text2_icon">, the same shapeicon_by_class()prints. Clicking an item still inserts its code ([3]). - Confirmed the committed
js/formidable_admin.jscontains the new code. The bundle is minified, so I checked for the code, not a byte diff. - Ran Pro and Views (local
mastercopies offormidable-proandformidable-views) for blocking item 2. Baseline: opening the Views editor with&form=1renders the list with icons (0 deferred, 10svg). After picking a form in the editor, the list has 5 deferred and 0svg. Switching to Advanced and back to Fields doesn't change that. Cypressshard 1 is red onFormTemplates.cy.js(cy.select()matched more than oneForm Template Testoption, and an svg visibility timeout). That's not this area. The same spec failed on other PRs today.- Not exercised: the form builder page's copy of the popup (same
showShortcodeBox()path, read from source), the Keys view visually, Lite's legacy Views editor (js/admin/legacy-views.js:244makes the same AJAX call, so it regresses the same way by reading), and any add-on'sfrm_field_code_tabhandler. - I didn't run the
code-review/security-review/simplifyskills on this one. I read the full diff by hand, since the change is small and has no user-input or data-handling surface (the only new output isdata-frm-icon, which goes throughesc_attr()).
| ); | ||
|
|
||
| do_action( 'frm_field_code_tab', array( 'field' => $f ) ); | ||
| do_action( 'frm_field_code_tab', array( 'field' => $f, 'defer_icon' => true ) ); |
There was a problem hiding this comment.
Blocking: PHPCS fails here (Run PHPCS inspection: "When a multi-item array is declared with explicit keys, each item should start on a new line"). This line is new in this PR.
| do_action( 'frm_field_code_tab', array( 'field' => $f, 'defer_icon' => true ) ); | |
| do_action( | |
| 'frm_field_code_tab', | |
| array( | |
| 'field' => $f, | |
| 'defer_icon' => true, | |
| ) | |
| ); |
| 'name' => $f->name, | ||
| 'type' => $f->type, | ||
| 'class' => 'frm-customize-list dropdown-item', | ||
| 'defer_icon' => true, |
There was a problem hiding this comment.
Blocking: icons go missing in the Views editor.
'defer_icon' => true here (and on line 80) applies to every render of the default template, but only the form-settings and builder popup (mb_insert_fields.php) is hidden until showShortcodeBox() runs the hydration. Formidable Views renders its own template on first load (editor/mb_adv_info.php, no deferral), then refreshes the sidebar with frm_get_cd_tags_box, which calls FrmFormsController::mb_tags_box( $form_id, 'frm_doing_ajax' ) with the default template. That HTML is put straight into the visible customization sidebar (formidable-views/js/editor.js:4760; Lite's js/admin/legacy-views.js:244 does the same). Nothing there opens the popup, so nothing hydrates.
Reproduced with Lite at this PR's head plus Pro and Views on master. After picking the form, the Fields list is visible and has 5 li[data-frm-icon] and 0 svg. Reloading the editor with the form already chosen, where Views' own template renders, has 10 svg and no deferred items. Clicking Advanced and back to Fields doesn't change that.
Fix: make deferral opt-in for the popup only, so any other caller of the default template keeps server-rendered icons.
// FrmFormsController::mb_tags_box()
public static function mb_tags_box( $form_id, $class = '', $template_path = 'default', $defer_icon = false ) {
// mb_adv_info.php, both places (this line and the do_action on :80)
'defer_icon' => $defer_icon,
// classes/views/frm-forms/mb_insert_fields.php:10, the hidden popup
FrmFormsController::mb_tags_box( $id, '', 'default', true );The new parameter is optional, so existing callers (Pro, Views, third parties) are unchanged.
| const targetEl = document.getElementById( targetId ); | ||
| if ( targetEl ) { | ||
| if ( targetId === 'frm-adv-info-tab' ) { | ||
| hydrateDeferredCodeListIcons( targetEl ); |
There was a problem hiding this comment.
Non-blocking. frm-adv-info-tab is the Advanced panel. The Fields list, the only .frm_customize_field_list in mb_adv_info.php, sits in #frm-insert-fields-box, so this call finds no [data-frm-icon] and does nothing. Opening the popup already hydrates the whole box at showShortcodeBox(). If it was meant to cover a visible copy of the list, it is aimed at the wrong panel, and it still wouldn't run for the Fields tab that starts active. Drop it, or fix the target once blocking item 2 is settled.
| * @param {HTMLElement} container The opened shortcode panel. | ||
| * @return {void} | ||
| */ | ||
| function hydrateDeferredCodeListIcons( container ) { |
There was a problem hiding this comment.
Non-blocking. This repeats FrmAppHelper::icon_by_class()'s work: the frmfont/frm_icon_font marker split and the frm_clone_solid_icon/frm_keyalt_* rename map, plus a font-icon <i> branch for icons PHP already marks deprecated. The two copies will drift. It also means the PHP _deprecated_argument() notice no longer fires for an add-on's old icon when it is deferred. Having PHP resolve the icon name and extra classes once and put them in the data attribute (for example data-frm-icon="frm_text2_icon", plus data-frm-icon-class if needed) would leave JS to just build the <svg>.
There was a problem hiding this comment.
Approve. Both blocking items from my last review are fixed, and I found nothing new that blocks.
Fixed since last review
Run PHPCS inspectionis green. Thedo_actioncall is now multi-line.- Deferral is opt-in.
mb_tags_box()takes$defer_icon = falseand onlymb_insert_fields.phppassestrue, so Views'frm_get_cd_tags_boxrefresh keeps server-rendered icons. - The stray
clickTabhydrate call is gone. Hydration now copies one icon per field type from a<template>instead of re-implementingicon_by_class()in JS. - A Cypress spec was added (
codeListIconsDeferredInit.cy.js).
Non-blocking, optional
- The opt-in default is the thing that broke last time, and nothing in Lite pins it. A small PHPUnit test would:
mb_tags_box( $form_id )output has nodata-frm-iconand does containsvg; with$defer_icon = trueit hasdata-frm-iconand atemplate.frm-code-list-icons.
What I checked, and what I didn't
- Ran Lite at
f4c1d77in Playground (WP 7.0, SQLite), with Pro and Views on their localmastercopies. Form settings > Actions > Confirmation > Redirect to URL, opening the popup from its.frm-show-boxicon:- Before opening: 5
li[data-frm-icon], 0svg. The template holds 3 icons (text, email, textarea) for 5 items. - After opening: 0 deferred, 10
svg(5 items x the id and key links). Each link got the icon for its own type (#frm_text2_icon,#frm_email2_icon,#frm_paragraph_icon). - Opening and closing the popup three times leaves the count at 10, so nothing is duplicated.
- Clicking the Email item inserted
[3 sanitize_url=1]into the redirect field. The Keys view also has an icon on each of its 5 links.
- Before opening: 5
- Views editor (the earlier regression): after picking a form in the editor, the Fields list has 5 items, 0 deferred and 10
svg, so the AJAX refresh keeps its icons. I read this from the DOM. The list sits in a hidden container in that editor, so I have no screenshot of it. - Rebuilt
js/formidable_admin.jsfrom the PR'sjs/src/admin/admin.js(npm run build, sibling checkout'snode_modules). After normalizing webpack module ids, the rebuild and the committed bundle differ only in those ids, so the committed bundle matches the source at this head. - CI: everything is green except
Cypress (shard 1). That isFormTemplates.cy.jsagain (an svg visibility timeout on the Form Templates page), the same spec that failed on other PRs today. The new spec ran in shard 3, which passed. - Popup at the PR head, Fields tab:
- Not exercised: the form builder page's copy of the popup (same
showShortcodeBox()path, read from source only), Lite's legacy Views editor (js/admin/legacy-views.js, same AJAX call, read from source), and any add-on'sfrm_field_code_tabhandler. Pro'sfield_sidebarhandler receives the newdefer_iconkey but its items keep inline icons, which is harmless. - I read the full diff by hand and didn't run the
code-review/security-review/simplifyskills. The only new output isdata-frm-icon/data-frm-icon-key, escaped withesc_attr(), and the injected icon markup comes from server-renderedicon_by_class()output, not user input.
Summary by CodeRabbit