Extend Style slider component's usability#3025
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThese changes add font-size customization support to email styles. A conditional check in the slider component initializes email-specific settings. The email styles controller conditionally propagates font_size from style settings to table attributes, and the table HTML generator appends corresponding CSS declarations to table cell styles. Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~12 minutes 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📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Mar 18, 2026 6:14p.m. | Review ↗ |
| JavaScript | |
Mar 18, 2026 6:14p.m. | Review ↗ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
js/formidable_dashboard.js (1)
401-403: Consider improving error handling in the request queue.The
.catch(task)pattern reruns the failing task on rejection, which means if the task fails consistently, it runs twice before propagating the error. This could cause duplicate side effects for non-idempotent operations.♻️ Suggested improvement for proper error handling
var addToRequestQueue = function addToRequestQueue(task) { - return lastPromise = lastPromise.then(task).catch(task); + return lastPromise = lastPromise.then(task).catch(function(error) { + console.error('Request queue task failed:', error); + // Continue the chain so subsequent tasks aren't blocked + return Promise.resolve(); + }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@js/formidable_dashboard.js` around lines 401 - 403, The current addToRequestQueue uses .catch(task) which re-executes the same failing task; modify addToRequestQueue (and the lastPromise chaining) to stop calling task again on rejection and instead propagate the error while resetting the queue so subsequent tasks still run: replace .catch(task) with .catch(err => { lastPromise = Promise.resolve(); throw err; }) so the failing task's error is thrown to callers but lastPromise is set to a resolved promise to allow the next queued task to start normally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@css/admin/frm-settings-components.css`:
- Around line 380-389: The native select caret is still visible under your
custom arrow; update the select rules for the selectors .frm-style-component
.frm-slider-value select and .frm-unit-input .frm-input-group-suffix select (in
_sliders.scss and _unit-input.scss) to disable the browser chrome by adding
appearance: none plus vendor-prefixed properties (-webkit-appearance: none;
-moz-appearance: none;) so the native arrow is hidden before your custom
background caret is drawn.
In `@js/formidable_dashboard.js`:
- Around line 124-127: The beforeunload handler is added with
this.cleanupObservers unbound, so inside cleanupObservers (method on
frmTabsNavigator) this becomes window and observers aren't disconnected; fix by
binding the method to the instance (e.g., in the constructor or initializer set
this.cleanupObservers = this.cleanupObservers.bind(this)) and then keep using
window.addEventListener('beforeunload', this.cleanupObservers) so the correct
this is available when cleanupObservers runs; ensure any corresponding
removeEventListener uses the same bound reference if you remove the listener
elsewhere.
- Around line 880-889: The initInbox method currently assumes DOM elements
exist; add defensive null checks around document.querySelector results for
'#frm_leave_email' and '#frm-add-my-email-address' before using them: if
subscribeButton is null, do not call addEventListener (early-return or skip
wiring); inside the click handler, ensure userEmailInput exists before reading
.value or call this.saveSubscribedEmail only when a non-null value is available;
reference the initInbox function, the subscribeButton and userEmailInput
variables, and the saveSubscribedEmail call when applying the guards so behavior
matches other components like frmTabsNavigator/frmCounter.
- Around line 916-948: Add CSRF nonce handling: include the nonce from
window.frmGlobal.nonce in the POST body of the JavaScript AJAX calls made by
saveSubscribedEmail and closeWelcomeBannerSaveCookieRequest (add a 'nonce' field
to the URLSearchParams) and on the server side add nonce validation at the start
of FrmDashboardController::ajax_requests() by calling
check_ajax_referer('frm_ajax', 'nonce') so the AJAX handler rejects requests
without a valid nonce.
In `@js/formidable_overlay.js`:
- Around line 49-60: The open(overlayData) method is prepending a new overlay
every call (via body.insertBefore(this.buildOverlay(), ...)) which allows
duplicate overlays; modify open to first detect an existing overlay wrapper (the
element returned/created by buildOverlay or marked by a unique class/id) and
either remove it or reuse it before calling bodyAddOverflowHidden(),
initCloseButton(), and initOverlayIntroAnimation(200); update the logic
referenced in open(), buildOverlay(), initCloseButton(), and
initOverlayIntroAnimation() so repeated frmOverlay.open() calls do not leave
duplicates (also apply the same guard in the analogous code block around lines
72–78).
- Around line 143-145: The code in js/formidable_overlay.js sets copy.innerHTML
= this.overlayData.copy, which is an XSS sink; replace direct innerHTML
assignment with a safe approach: either set copy.textContent (for plain text) or
run this.overlayData.copy through an explicit sanitizer before assignment (if
rich HTML is required). Update the creation path in the overlay code (refer to
the copy variable and the innerHTML assignment in the overlay rendering logic,
and mirror the change in js/src/components/class-overlay.js before rebuilding)
so that unsanitized user data is never inserted via innerHTML.
- Around line 104-125: The code assumes this.overlayData.buttons is a non-empty
array and may render an empty wrapper; first guard with
Array.isArray(this.overlayData.buttons) and produce a filtered list of valid
button entries (e.g., filter out null/undefined and entries missing url/label)
before mapping to DOM nodes with frmDom.a; compute the primary/secondary button
class using the index from the filtered list (not the original array) and only
call frmDom.div with buttonsWrapperElementOptions when the resulting
buttonElements.length > 0, otherwise return nothing (or null/empty string) to
avoid rendering an empty wrapper.
- Around line 85-88: The close control currently uses a non-semantic span
(closeButton) which is not keyboard-accessible or accessible to assistive tech;
replace the span with a semantic <button> element (retain the frm-overlay--close
class), set its type="button", provide an accessible name via aria-label or
visible text (e.g., "Close"), and keep the existing click handler (this.close)
attached and the prepend into overlayWrapper; ensure the handler is bound
correctly if it relies on instance context (e.g., use the same reference as
currently used).
In
`@js/src/web-components/frm-border-radius-component/frm-border-radius-component.css`:
- Around line 121-126: The rule currently sets background: white; then later
uses a background shorthand with the SVG which resets the background-color;
change the first declaration to background-color: white; and replace the
background shorthand (background: url(...) no-repeat center center;) with
explicit properties—background-image: url(...); background-repeat: no-repeat;
background-position: center center;—so the white fill is preserved while the SVG
is applied.
In
`@js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css`:
- Around line 153-170: The active-link rule currently splits styling between
`.frm-tabs-navs ul li.frm-active` and `.frm-style-tabs-wrapper .frm-tabs-navs ul
li.frm-active a`, causing anchors to stay grey outside the admin wrapper; change
the selector to target both the list item and its anchor globally by replacing
the wrapper-scoped selector with `.frm-tabs-navs ul li.frm-active a` so the
combined selector becomes `.frm-tabs-navs ul li.frm-active, .frm-tabs-navs ul
li.frm-active a` and ensure the rule sets `color: var(--grey-900)` for both.
In `@js/src/web-components/frm-typography-component/frm-typography-component.css`:
- Around line 127-163: The compound unit field removes native outlines on its
children (input:focus and select:focus) but lacks a replacement indicator; add a
.frm-typography-component .frm-typography-container .frm-unit-value:focus-within
rule to provide a visible focus state (e.g., change border color, add box-shadow
or ring, and/or adjust background) so keyboard users see when focus is inside
the frm-unit-value; update the CSS near the .frm-unit-value rules to include
this :focus-within style and ensure it is accessible and contrasts with the
existing border (referencing the .frm-unit-value selector and its input/select
focus suppression).
---
Nitpick comments:
In `@js/formidable_dashboard.js`:
- Around line 401-403: The current addToRequestQueue uses .catch(task) which
re-executes the same failing task; modify addToRequestQueue (and the lastPromise
chaining) to stop calling task again on rejection and instead propagate the
error while resetting the queue so subsequent tasks still run: replace
.catch(task) with .catch(err => { lastPromise = Promise.resolve(); throw err; })
so the failing task's error is thrown to callers but lastPromise is set to a
resolved promise to allow the next queued task to start normally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee91e737-de33-4f2c-af0b-49befcb842c5
📒 Files selected for processing (24)
css/admin/frm-settings-components.csscss/admin/welcome-tour.csscss/font_icons.csscss/frm_admin.csscss/frm_testing_mode.cssjs/addons-page.jsjs/form-templates.jsjs/formidable-settings-components.jsjs/formidable-web-components.jsjs/formidable_admin.jsjs/formidable_blocks.jsjs/formidable_dashboard.jsjs/formidable_overlay.jsjs/formidable_styles.jsjs/frm_testing_mode.jsjs/onboarding-wizard.jsjs/src/settings-components/components/slider-component.jsjs/src/web-components/frm-border-radius-component/frm-border-radius-component.cssjs/src/web-components/frm-colorpicker-component/frm-colorpicker-component.cssjs/src/web-components/frm-dropdown-component/frm-dropdown-component.cssjs/src/web-components/frm-range-slider-component/frm-range-slider-component.cssjs/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.cssjs/src/web-components/frm-typography-component/frm-typography-component.cssjs/welcome-tour.js
| .frm-style-component .frm-slider-value select { | ||
| text-align: right; | ||
| padding: 0; | ||
| font-size: var(--text-sm); | ||
| color: rgb(102, 112, 133); | ||
| width: 44px; | ||
| background: url("../../images/style/small-arrow.svg") no-repeat; | ||
| background-position: center right 12px; | ||
| padding-right: 24px !important; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "frm-settings-components.css"Repository: Strategy11/formidable-forms
Length of output: 108
🏁 Script executed:
wc -l css/admin/frm-settings-components.cssRepository: Strategy11/formidable-forms
Length of output: 112
🏁 Script executed:
sed -n '375,395p' css/admin/frm-settings-components.cssRepository: Strategy11/formidable-forms
Length of output: 661
🏁 Script executed:
sed -n '604,625p' css/admin/frm-settings-components.cssRepository: Strategy11/formidable-forms
Length of output: 886
🏁 Script executed:
rg -n "appearance" css/admin/frm-settings-components.cssRepository: Strategy11/formidable-forms
Length of output: 126
🏁 Script executed:
sed -n '590,605p' css/admin/frm-settings-components.cssRepository: Strategy11/formidable-forms
Length of output: 459
🏁 Script executed:
sed -n '380,395p' css/admin/frm-settings-components.css | cat -nRepository: Strategy11/formidable-forms
Length of output: 668
🏁 Script executed:
rg -g "*.scss" "frm-slider-value select" --max-count 5Repository: Strategy11/formidable-forms
Length of output: 155
🏁 Script executed:
rg -g "*.scss" "frm-unit-input.*select" --max-count 5Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
rg -g "*.scss" "frm-unit-input" | grep -i "select"Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
cat resources/scss/admin/components/settings/_sliders.scssRepository: Strategy11/formidable-forms
Length of output: 3988
🏁 Script executed:
fd -t f "*.scss" | xargs grep -l "frm-unit-input" | head -5Repository: Strategy11/formidable-forms
Length of output: 356
🏁 Script executed:
fd -t f ".scss" | xargs grep -l "frm-unit-input" | head -5Repository: Strategy11/formidable-forms
Length of output: 179
🏁 Script executed:
cat resources/scss/admin/components/settings/_unit-input.scssRepository: Strategy11/formidable-forms
Length of output: 1135
🏁 Script executed:
sed -n '380,395p' css/admin/frm-settings-components.css | grep -i "appearance"Repository: Strategy11/formidable-forms
Length of output: 53
Hide the native select chrome before drawing a custom caret.
Both .frm-style-component .frm-slider-value select and .frm-unit-input .frm-input-group-suffix select add custom arrow artwork but never disable the built-in select UI. In browsers that retain the native arrow, these unit pickers render a double caret.
Fix in the source SCSS files:
resources/scss/admin/components/settings/_sliders.scss— Addappearance: none;and vendor prefixes to the select ruleresources/scss/admin/components/settings/_unit-input.scss— Addappearance: none;and vendor prefixes to the select rule
Proposed SCSS fix
For _sliders.scss:
select {
+ appearance: none;
+ -webkit-appearance: none;
+ -moz-appearance: none;
text-align: right;
padding: 0;
font-size: var(--text-sm);
color: rgba(102, 112, 133, 1);
width: 44px;
background: url("../../images/style/small-arrow.svg") no-repeat;
background-position: center right 12px;
padding-right: 24px !important;
}For _unit-input.scss:
select {
+ appearance: none;
+ -webkit-appearance: none;
+ -moz-appearance: none;
width: fit-content;
height: 100%;
min-height: unset;
font-size: var(--text-sm);
text-align: right;
color: var(--grey-500) !important;
background: `#fff` url("data:image/svg+xml,...") no-repeat right 0 top 50%;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@css/admin/frm-settings-components.css` around lines 380 - 389, The native
select caret is still visible under your custom arrow; update the select rules
for the selectors .frm-style-component .frm-slider-value select and
.frm-unit-input .frm-input-group-suffix select (in _sliders.scss and
_unit-input.scss) to disable the browser chrome by adding appearance: none plus
vendor-prefixed properties (-webkit-appearance: none; -moz-appearance: none;) so
the native arrow is hidden before your custom background caret is drawn.
js/formidable_dashboard.js
Outdated
| this.setupScrollbarObserver(); | ||
| // Cleanup observers when page unloads to prevent memory leaks | ||
| window.addEventListener('beforeunload', this.cleanupObservers); | ||
| } |
There was a problem hiding this comment.
Missing this binding for cleanupObservers callback causes memory leak.
When this.cleanupObservers is passed directly to addEventListener, the this context inside the method will be window instead of the frmTabsNavigator instance. This causes this.resizeObserver to be undefined, and the observer won't be disconnected.
🔧 Proposed fix to bind the correct context
- window.addEventListener('beforeunload', this.cleanupObservers);
+ window.addEventListener('beforeunload', this.cleanupObservers.bind(this));📝 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.
| this.setupScrollbarObserver(); | |
| // Cleanup observers when page unloads to prevent memory leaks | |
| window.addEventListener('beforeunload', this.cleanupObservers); | |
| } | |
| this.setupScrollbarObserver(); | |
| // Cleanup observers when page unloads to prevent memory leaks | |
| window.addEventListener('beforeunload', this.cleanupObservers.bind(this)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/formidable_dashboard.js` around lines 124 - 127, The beforeunload handler
is added with this.cleanupObservers unbound, so inside cleanupObservers (method
on frmTabsNavigator) this becomes window and observers aren't disconnected; fix
by binding the method to the instance (e.g., in the constructor or initializer
set this.cleanupObservers = this.cleanupObservers.bind(this)) and then keep
using window.addEventListener('beforeunload', this.cleanupObservers) so the
correct this is available when cleanupObservers runs; ensure any corresponding
removeEventListener uses the same bound reference if you remove the listener
elsewhere.
js/formidable_dashboard.js
Outdated
| key: "initInbox", | ||
| value: function initInbox() { | ||
| var _this = this; | ||
| new _components_class_tabs_navigator__WEBPACK_IMPORTED_MODULE_1__.frmTabsNavigator('.frm-inbox-wrapper'); | ||
| var userEmailInput = document.querySelector('#frm_leave_email'); | ||
| var subscribeButton = document.querySelector('#frm-add-my-email-address'); | ||
| subscribeButton.addEventListener('click', function () { | ||
| _this.saveSubscribedEmail(userEmailInput.value).then(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing null checks before accessing DOM elements can cause runtime errors.
subscribeButton and userEmailInput are used without null checks. If these elements don't exist in the DOM (e.g., different page context or conditional rendering), calling addEventListener on null will throw a TypeError.
This is inconsistent with the defensive patterns in frmTabsNavigator and frmCounter which gracefully early-return when elements are missing.
🐛 Proposed fix to add null guards
}, {
key: "initInbox",
value: function initInbox() {
var _this = this;
new _components_class_tabs_navigator__WEBPACK_IMPORTED_MODULE_1__.frmTabsNavigator('.frm-inbox-wrapper');
var userEmailInput = document.querySelector('#frm_leave_email');
var subscribeButton = document.querySelector('#frm-add-my-email-address');
+ if (!subscribeButton || !userEmailInput) {
+ return;
+ }
subscribeButton.addEventListener('click', function () {
_this.saveSubscribedEmail(userEmailInput.value).then();
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/formidable_dashboard.js` around lines 880 - 889, The initInbox method
currently assumes DOM elements exist; add defensive null checks around
document.querySelector results for '#frm_leave_email' and
'#frm-add-my-email-address' before using them: if subscribeButton is null, do
not call addEventListener (early-return or skip wiring); inside the click
handler, ensure userEmailInput exists before reading .value or call
this.saveSubscribedEmail only when a non-null value is available; reference the
initInbox function, the subscribeButton and userEmailInput variables, and the
saveSubscribedEmail call when applying the guards so behavior matches other
components like frmTabsNavigator/frmCounter.
js/formidable_dashboard.js
Outdated
| key: "saveSubscribedEmail", | ||
| value: function saveSubscribedEmail(email) { | ||
| return fetch(window.ajaxurl, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/x-www-form-urlencoded' | ||
| }, | ||
| body: new URLSearchParams({ | ||
| action: this.options.ajax.action, | ||
| dashboard_action: this.options.ajax.dashboardActions.saveSubscribedEmail, | ||
| email: email | ||
| }) | ||
| }).then(function (result) { | ||
| return result.json(); | ||
| }); | ||
| } | ||
| }, { | ||
| key: "closeWelcomeBannerSaveCookieRequest", | ||
| value: function closeWelcomeBannerSaveCookieRequest() { | ||
| return fetch(window.ajaxurl, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/x-www-form-urlencoded' | ||
| }, | ||
| body: new URLSearchParams({ | ||
| action: this.options.ajax.action, | ||
| dashboard_action: this.options.ajax.dashboardActions.welcomeBanner, | ||
| banner_has_closed: 1 | ||
| }) | ||
| }).then(function (result) { | ||
| return result.json(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for the dashboard AJAX action handler to verify nonce validation
# Find the PHP handler for dashboard_ajax_action
rg -n -A 10 "dashboard_ajax_action" --type=php
# Check if nonce verification is implemented
rg -n "wp_verify_nonce|check_ajax_referer" --type=php -C 3 | head -50Repository: Strategy11/formidable-forms
Length of output: 5693
🏁 Script executed:
#!/bin/bash
# Get the full ajax_requests() method from FrmDashboardController
rg -n -A 100 "public static function ajax_requests" classes/controllers/FrmDashboardController.php | head -120Repository: Strategy11/formidable-forms
Length of output: 3381
🏁 Script executed:
#!/bin/bash
# Check if there's any common nonce validation in helper classes or base controllers
rg -n "check_ajax_referer|wp_verify_nonce" classes/ | grep -i "helper\|base" -A 5 -B 5
# Also check if FrmAppHelper has any AJAX validation
rg -n -A 5 "class FrmAppHelper" classes/Repository: Strategy11/formidable-forms
Length of output: 2658
🏁 Script executed:
#!/bin/bash
# Check what nonce is available in formidable_dashboard.js
rg -n "nonce|frmGlobal" js/formidable_dashboard.js | head -20
# Check how nonce is typically passed in other AJAX calls in the codebase
rg -n "frmGlobal.nonce\|_wpnonce\|frm_ajax" js/ --type=js | head -30Repository: Strategy11/formidable-forms
Length of output: 259
🏁 Script executed:
#!/bin/bash
# Check where frmGlobal.nonce is set/enqueued in PHP
rg -n "frmGlobal\|wp_localize_script.*formidable_dashboard" classes/controllers/FrmDashboardController.php
# Check enqueue_assets more thoroughly
rg -n -B 5 -A 20 "public static function enqueue_assets" classes/controllers/FrmDashboardController.phpRepository: Strategy11/formidable-forms
Length of output: 707
🏁 Script executed:
#!/bin/bash
# Search for where frmGlobal is defined/localized in the entire codebase
rg -n "frmGlobal" classes/ --type=php | head -30
# Also search for wp_localize_script calls that might set it up
rg -n "wp_localize_script" classes/ --type=php -A 5 | head -50Repository: Strategy11/formidable-forms
Length of output: 3675
🏁 Script executed:
#!/bin/bash
# Get the full context of where frmGlobal is localized and what it contains
rg -n -B 20 "wp_localize_script.*formidable_admin_global.*frmGlobal" classes/helpers/FrmAppHelper.php | head -60Repository: Strategy11/formidable-forms
Length of output: 1633
AJAX requests missing CSRF nonce protection.
Both saveSubscribedEmail and closeWelcomeBannerSaveCookieRequest make POST requests without including a nonce for CSRF verification. The nonce is available from window.frmGlobal.nonce but isn't used in these requests.
The corresponding PHP handler (FrmDashboardController::ajax_requests()) also lacks nonce validation. All other AJAX handlers in the codebase validate nonces using check_ajax_referer( 'frm_ajax', 'nonce' ) at the method entry point.
WordPress admin AJAX handlers should validate nonces to prevent cross-site request forgery attacks.
🔒 Proposed fixes
JavaScript side - Add nonce to request body:
body: new URLSearchParams({
action: this.options.ajax.action,
dashboard_action: this.options.ajax.dashboardActions.saveSubscribedEmail,
- email: email
+ email: email,
+ nonce: window.frmGlobal.nonce
})PHP side - Add nonce validation at the start of FrmDashboardController::ajax_requests():
public static function ajax_requests() {
+ check_ajax_referer( 'frm_ajax', 'nonce' );
$dashboard_action = FrmAppHelper::get_post_param( 'dashboard_action', '', 'sanitize_text_field' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/formidable_dashboard.js` around lines 916 - 948, Add CSRF nonce handling:
include the nonce from window.frmGlobal.nonce in the POST body of the JavaScript
AJAX calls made by saveSubscribedEmail and closeWelcomeBannerSaveCookieRequest
(add a 'nonce' field to the URLSearchParams) and on the server side add nonce
validation at the start of FrmDashboardController::ajax_requests() by calling
check_ajax_referer('frm_ajax', 'nonce') so the AJAX handler rejects requests
without a valid nonce.
js/formidable_overlay.js
Outdated
| value: function open(overlayData) { | ||
| this.overlayData = { | ||
| hero_image: null, | ||
| heading: null, | ||
| copy: null, | ||
| buttons: [] | ||
| }; | ||
| this.overlayData = _objectSpread(_objectSpread({}, this.overlayData), overlayData); | ||
| this.bodyAddOverflowHidden(); | ||
| this.body.insertBefore(this.buildOverlay(), this.body.firstChild); | ||
| this.initCloseButton(); | ||
| this.initOverlayIntroAnimation(200); |
There was a problem hiding this comment.
Prevent overlay stacking on repeated open() calls.
Line 58 always prepends a new wrapper, while Line 74 removes only the first wrapper. Multiple frmOverlay.open() calls can leave duplicate overlays in DOM.
Suggested fix (apply in js/src/components/class-overlay.js, then rebuild)
diff --git a/js/src/components/class-overlay.js b/js/src/components/class-overlay.js
@@
open( overlayData ) {
+ this.close(); // avoid duplicate wrappers
this.overlayData = {
hero_image: null,
heading: null,
copy: null,
buttons: []
};
@@
close() {
- const overlayWrapper = document.querySelector( '.frm-overlay--wrapper' );
- if ( overlayWrapper ) {
+ const overlayWrappers = document.querySelectorAll( '.frm-overlay--wrapper' );
+ if ( overlayWrappers.length ) {
document.body.classList.remove( 'frm-hidden-overflow' );
- overlayWrapper.remove();
+ overlayWrappers.forEach( ( el ) => el.remove() );
}
}Also applies to: 72-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/formidable_overlay.js` around lines 49 - 60, The open(overlayData) method
is prepending a new overlay every call (via
body.insertBefore(this.buildOverlay(), ...)) which allows duplicate overlays;
modify open to first detect an existing overlay wrapper (the element
returned/created by buildOverlay or marked by a unique class/id) and either
remove it or reuse it before calling bodyAddOverflowHidden(), initCloseButton(),
and initOverlayIntroAnimation(200); update the logic referenced in open(),
buildOverlay(), initCloseButton(), and initOverlayIntroAnimation() so repeated
frmOverlay.open() calls do not leave duplicates (also apply the same guard in
the analogous code block around lines 72–78).
js/formidable_overlay.js
Outdated
| var buttons = this.overlayData.buttons.map(function (button, index) { | ||
| if (!button.url) { | ||
| return ''; | ||
| } | ||
| var buttonTypeClassname = 1 === index ? 'frm-button-primary' : 'frm-button-secondary'; | ||
| var options = { | ||
| href: button.url, | ||
| text: button.label, | ||
| className: "button frm_animate_bg ".concat(buttonTypeClassname) | ||
| }; | ||
| if (button.target) { | ||
| options.target = button.target; | ||
| } | ||
| return frmDom.a(options); | ||
| }); | ||
| if (buttons) { | ||
| var buttonsWrapperElementOptions = { | ||
| className: 'frm-overlay--cta frm-flex-box', | ||
| children: buttons | ||
| }; | ||
| return frmDom.div(buttonsWrapperElementOptions); | ||
| } |
There was a problem hiding this comment.
Harden CTA button rendering against invalid payloads.
Line 104 assumes buttons is always an array, and Line 119 uses if (buttons) (always truthy for arrays), so empty wrappers can be rendered.
Suggested fix (apply in js/src/components/class-overlay.js, then rebuild)
-const buttons = this.overlayData.buttons.map( ( button, index ) => {
+const buttonList = Array.isArray( this.overlayData.buttons ) ? this.overlayData.buttons : [];
+const buttons = buttonList
+ .map( ( button, index ) => {
if ( ! button.url ) {
return '';
}
@@
- return frmDom.a( options );
- } );
-
-if ( buttons ) {
+ return frmDom.a( options );
+ } )
+ .filter( Boolean );
+
+if ( buttons.length ) {
return frmDom.div( {
className: 'frm-overlay--cta frm-flex-box',
children: buttons
} );
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/formidable_overlay.js` around lines 104 - 125, The code assumes
this.overlayData.buttons is a non-empty array and may render an empty wrapper;
first guard with Array.isArray(this.overlayData.buttons) and produce a filtered
list of valid button entries (e.g., filter out null/undefined and entries
missing url/label) before mapping to DOM nodes with frmDom.a; compute the
primary/secondary button class using the index from the filtered list (not the
original array) and only call frmDom.div with buttonsWrapperElementOptions when
the resulting buttonElements.length > 0, otherwise return nothing (or null/empty
string) to avoid rendering an empty wrapper.
js/formidable_overlay.js
Outdated
| var copy = frmDom.tag('div'); | ||
| copy.innerHTML = this.overlayData.copy; | ||
| return frmDom.div({ |
There was a problem hiding this comment.
Avoid unsanitized HTML injection in overlay copy.
Line 144 assigns runtime data directly to innerHTML, which is an XSS sink if overlayData.copy is not strictly sanitized upstream.
Safer default (apply in js/src/components/class-overlay.js, then rebuild)
- copy.innerHTML = this.overlayData.copy;
+ copy.textContent = this.overlayData.copy ?? '';If rich HTML is required, enforce explicit sanitization before assignment.
📝 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.
| var copy = frmDom.tag('div'); | |
| copy.innerHTML = this.overlayData.copy; | |
| return frmDom.div({ | |
| var copy = frmDom.tag('div'); | |
| copy.textContent = this.overlayData.copy ?? ''; | |
| return frmDom.div({ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/formidable_overlay.js` around lines 143 - 145, The code in
js/formidable_overlay.js sets copy.innerHTML = this.overlayData.copy, which is
an XSS sink; replace direct innerHTML assignment with a safe approach: either
set copy.textContent (for plain text) or run this.overlayData.copy through an
explicit sanitizer before assignment (if rich HTML is required). Update the
creation path in the overlay code (refer to the copy variable and the innerHTML
assignment in the overlay rendering logic, and mirror the change in
js/src/components/class-overlay.js before rebuilding) so that unsanitized user
data is never inserted via innerHTML.
| background: white; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| box-sizing: border-box; | ||
| background: url('data:image/svg+xml,<svg width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M3.95837 12.2915V14.3748C3.95837 15.2953 4.70457 16.0415 5.62504 16.0415H7.70837" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/><path d="M16.0416 12.2915V14.3748C16.0416 15.2953 15.2955 16.0415 14.375 16.0415H12.2916" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/><path d="M16.0416 7.7085V5.62516C16.0416 4.70469 15.2955 3.9585 14.375 3.9585H12.2916" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/><path d="M3.95837 7.7085V5.62516C3.95837 4.70469 4.70457 3.9585 5.62504 3.9585H7.70837" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/></svg>') no-repeat center center; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "frm-border-radius-component.css" 2>/dev/null | head -5Repository: Strategy11/formidable-forms
Length of output: 154
🏁 Script executed:
sed -n '113,130p' ./js/src/web-components/frm-border-radius-component/frm-border-radius-component.cssRepository: Strategy11/formidable-forms
Length of output: 1505
Keep the button fill when adding the SVG background.
Line 126 uses the background shorthand after line 121 set background: white;. The shorthand resets background-color to transparent (its initial value), so the white fill is lost. The button background should explicitly use background-color: white; paired with individual background properties for the image:
Proposed fix
- background: white;
+ background-color: white;
display: flex;
justify-content: center;
align-items: center;
box-sizing: border-box;
- background: url('data:image/svg+xml,...') no-repeat center center;
+ background-image: url('data:image/svg+xml,...');
+ background-repeat: no-repeat;
+ background-position: center center;
background-size: 20px;📝 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.
| background: white; | |
| display: flex; | |
| justify-content: center; | |
| align-items: center; | |
| box-sizing: border-box; | |
| background: url('data:image/svg+xml,<svg width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M3.95837 12.2915V14.3748C3.95837 15.2953 4.70457 16.0415 5.62504 16.0415H7.70837" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/><path d="M16.0416 12.2915V14.3748C16.0416 15.2953 15.2955 16.0415 14.375 16.0415H12.2916" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/><path d="M16.0416 7.7085V5.62516C16.0416 4.70469 15.2955 3.9585 14.375 3.9585H12.2916" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/><path d="M3.95837 7.7085V5.62516C3.95837 4.70469 4.70457 3.9585 5.62504 3.9585H7.70837" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/></svg>') no-repeat center center; | |
| background-color: white; | |
| display: flex; | |
| justify-content: center; | |
| align-items: center; | |
| box-sizing: border-box; | |
| background-image: url('data:image/svg+xml,<svg width="20" height="20" viewBox="0 0 20 20" fill="none" xmlns="http://www.w3.org/2000/svg"><path d="M3.95837 12.2915V14.3748C3.95837 15.2953 4.70457 16.0415 5.62504 16.0415H7.70837" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/><path d="M16.0416 12.2915V14.3748C16.0416 15.2953 15.2955 16.0415 14.375 16.0415H12.2916" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/><path d="M16.0416 7.7085V5.62516C16.0416 4.70469 15.2955 3.9585 14.375 3.9585H12.2916" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/><path d="M3.95837 7.7085V5.62516C3.95837 4.70469 4.70457 3.9585 5.62504 3.9585H7.70837" stroke="%23667085" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/></svg>'); | |
| background-repeat: no-repeat; | |
| background-position: center center; |
🧰 Tools
🪛 Biome (2.4.7)
[error] 126-126: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
(lint/suspicious/noDuplicateProperties)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/src/web-components/frm-border-radius-component/frm-border-radius-component.css`
around lines 121 - 126, The rule currently sets background: white; then later
uses a background shorthand with the SVG which resets the background-color;
change the first declaration to background-color: white; and replace the
background shorthand (background: url(...) no-repeat center center;) with
explicit properties—background-image: url(...); background-repeat: no-repeat;
background-position: center center;—so the white fill is preserved while the SVG
is applied.
| .frm-tabs-navs ul li, | ||
| .frm-tabs-navs ul li a { | ||
| color: var(--grey-500); | ||
| font-weight: 500; | ||
| font-size: var(--text-sm); | ||
| line-height: 28px; | ||
| } | ||
| .frm-tabs-navs ul li { | ||
| flex: 1; | ||
| height: 28px; | ||
| text-align: center; | ||
| margin-top: var(--gap-xs); | ||
| margin-bottom: 0; | ||
| cursor: pointer; | ||
| } | ||
|
|
||
| .frm-tabs-navs ul li.frm-active, .frm-style-tabs-wrapper .frm-tabs-navs ul li.frm-active a { | ||
| color: var(--grey-900); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the CSS file
find . -name "frm-tab-navigator-component.css" -type fRepository: Strategy11/formidable-forms
Length of output: 154
🏁 Script executed:
# Read the CSS file to verify the current state at lines 153-170
if [ -f "js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css" ]; then
wc -l "js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css"
echo "=== Lines 150-175 ==="
sed -n '150,175p' "js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css"
fiRepository: Strategy11/formidable-forms
Length of output: 698
🏁 Script executed:
# Search for .frm-style-tabs-wrapper in the codebase to understand its context
rg "frm-style-tabs-wrapper" --type css --type scss --type html --type ts --type tsx -B 2 -A 2Repository: Strategy11/formidable-forms
Length of output: 99
🏁 Script executed:
# Check if the component file exists and look for any imports or component structure
find . -name "*frm-tab-navigator*" -type f | head -20Repository: Strategy11/formidable-forms
Length of output: 322
🏁 Script executed:
# Search for .frm-style-tabs-wrapper usage throughout the codebase
rg "frm-style-tabs-wrapper" -B 3 -A 3Repository: Strategy11/formidable-forms
Length of output: 21168
🏁 Script executed:
# Examine the SCSS file to understand the source
cat -n "js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.scss"Repository: Strategy11/formidable-forms
Length of output: 3923
🏁 Script executed:
# Look at the component JS file to understand usage and context
head -100 "js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.js"Repository: Strategy11/formidable-forms
Length of output: 2562
🏁 Script executed:
# Search more broadly for how this component is used in the codebase
rg "frm-tab-navigator" --type html --type ts --type tsx -B 2 -A 2 | head -100Repository: Strategy11/formidable-forms
Length of output: 98
Remove the wrapper-scoped selector from the active-link rule.
The selector on line 169 splits active link styling across two rules: the list item color is promoted globally, but the anchor color is scoped to .frm-style-tabs-wrapper, which is the admin panel wrapper. Since the web component uses frm-tabs-wrapper instead, active links remain grey when this component is used standalone.
Proposed fix
-.frm-tabs-navs ul li.frm-active, .frm-style-tabs-wrapper .frm-tabs-navs ul li.frm-active a {
+.frm-tabs-navs ul li.frm-active,
+.frm-tabs-navs ul li.frm-active a {
color: var(--grey-900);
}📝 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.
| .frm-tabs-navs ul li, | |
| .frm-tabs-navs ul li a { | |
| color: var(--grey-500); | |
| font-weight: 500; | |
| font-size: var(--text-sm); | |
| line-height: 28px; | |
| } | |
| .frm-tabs-navs ul li { | |
| flex: 1; | |
| height: 28px; | |
| text-align: center; | |
| margin-top: var(--gap-xs); | |
| margin-bottom: 0; | |
| cursor: pointer; | |
| } | |
| .frm-tabs-navs ul li.frm-active, .frm-style-tabs-wrapper .frm-tabs-navs ul li.frm-active a { | |
| color: var(--grey-900); | |
| .frm-tabs-navs ul li, | |
| .frm-tabs-navs ul li a { | |
| color: var(--grey-500); | |
| font-weight: 500; | |
| font-size: var(--text-sm); | |
| line-height: 28px; | |
| } | |
| .frm-tabs-navs ul li { | |
| flex: 1; | |
| height: 28px; | |
| text-align: center; | |
| margin-top: var(--gap-xs); | |
| margin-bottom: 0; | |
| cursor: pointer; | |
| } | |
| .frm-tabs-navs ul li.frm-active, | |
| .frm-tabs-navs ul li.frm-active a { | |
| color: var(--grey-900); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@js/src/web-components/frm-tab-navigator-component/frm-tab-navigator-component.css`
around lines 153 - 170, The active-link rule currently splits styling between
`.frm-tabs-navs ul li.frm-active` and `.frm-style-tabs-wrapper .frm-tabs-navs ul
li.frm-active a`, causing anchors to stay grey outside the admin wrapper; change
the selector to target both the list item and its anchor globally by replacing
the wrapper-scoped selector with `.frm-tabs-navs ul li.frm-active a` so the
combined selector becomes `.frm-tabs-navs ul li.frm-active, .frm-tabs-navs ul
li.frm-active a` and ensure the rule sets `color: var(--grey-900)` for both.
| .frm-typography-component .frm-typography-container .frm-unit-value { | ||
| width: 28%; | ||
| height: 36px; | ||
| display: flex; | ||
| justify-content: center; | ||
| box-sizing: border-box; | ||
| background: white; | ||
| border-radius: var(--small-radius); | ||
| border: 1px solid var(--grey-300); | ||
| overflow: hidden; | ||
| } | ||
| .frm-typography-component .frm-typography-container .frm-unit-value > * { | ||
| border: none; | ||
| } | ||
| .frm-typography-component .frm-typography-container .frm-unit-value input { | ||
| width: 32px; | ||
| height: 100%; | ||
| padding: 0; | ||
| font-size: var(--text-sm); | ||
| color: rgb(16, 24, 40); | ||
| padding-left: 8px; | ||
| box-sizing: border-box; | ||
| } | ||
| .frm-typography-component .frm-typography-container .frm-unit-value input:focus { | ||
| outline: none; | ||
| } | ||
| .frm-typography-component .frm-typography-container .frm-unit-value select { | ||
| text-align: left; | ||
| padding: 0; | ||
| font-size: var(--text-sm); | ||
| color: rgb(102, 112, 133); | ||
| width: 36px; | ||
| background: url("../../images/style/small-arrow.svg") no-repeat; | ||
| background-position: center right 12px; | ||
| } | ||
| .frm-typography-component .frm-typography-container .frm-unit-value select:focus { | ||
| outline: none; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "frm-typography-component.css" -type fRepository: Strategy11/formidable-forms
Length of output: 148
🏁 Script executed:
wc -l ./js/src/web-components/frm-typography-component/frm-typography-component.cssRepository: Strategy11/formidable-forms
Length of output: 152
🏁 Script executed:
cat -n ./js/src/web-components/frm-typography-component/frm-typography-component.css | sed -n '120,165p'Repository: Strategy11/formidable-forms
Length of output: 1641
🏁 Script executed:
cat -n ./js/src/web-components/frm-typography-component/frm-typography-component.css | grep -i "focus-within\|frm-unit-value.*focus"Repository: Strategy11/formidable-forms
Length of output: 249
🏁 Script executed:
# Check if there are any related CSS files that might apply focus styles
find ./js/src/web-components/frm-typography-component -type fRepository: Strategy11/formidable-forms
Length of output: 304
🏁 Script executed:
cat -n ./js/src/web-components/frm-typography-component/frm-typography-component.scss | sed -n '120,165p'Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
wc -l ./js/src/web-components/frm-typography-component/frm-typography-component.scssRepository: Strategy11/formidable-forms
Length of output: 152
🏁 Script executed:
cat -n ./js/src/web-components/frm-typography-component/frm-typography-component.scssRepository: Strategy11/formidable-forms
Length of output: 1853
🏁 Script executed:
# Check the imported frm-web-component.scss file to see if there are any global focus styles
cat ./js/src/web-components/frm-web-component.scss | grep -i "focus" | head -20Repository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
# Search for any global focus styles that might apply to these elements
rg "focus-within" ./js/src/web-components/ -A 2 -B 2Repository: Strategy11/formidable-forms
Length of output: 53
Add a visible focus indicator to the unit field container.
The inner input:focus (lines 46–48) and select:focus (lines 59–61) both suppress the outline, but .frm-unit-value has no :focus-within replacement style. Keyboard users will see no visual cue when focus enters this compound field.
Proposed fix
.frm-unit-value {
width: 28%;
height: 36px;
display: flex;
justify-content: center;
box-sizing: border-box;
background: white;
border-radius: var(--small-radius);
border: 1px solid var(--grey-300);
overflow: hidden;
+
+ &:focus-within {
+ border-color: var(--primary-500);
+ box-shadow: 0 0 0 1px var(--primary-500);
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@js/src/web-components/frm-typography-component/frm-typography-component.css`
around lines 127 - 163, The compound unit field removes native outlines on its
children (input:focus and select:focus) but lacks a replacement indicator; add a
.frm-typography-component .frm-typography-container .frm-unit-value:focus-within
rule to provide a visible focus state (e.g., change border color, add box-shadow
or ring, and/or adjust background) so keyboard users see when focus is inside
the frm-unit-value; update the CSS near the .frm-unit-value rules to include
this :focus-within style and ensure it is accessible and contrasts with the
existing border (referencing the .frm-unit-value selector and its input/select
focus suppression).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@classes/controllers/FrmEmailStylesController.php`:
- Line 255: Guard the access to $style_settings['font_size'] to avoid
undefined-index notices by checking for its existence before assignment; in the
code path that sets $atts['font_size'] (inside FrmEmailStylesController where
get_email_style_settings() is used), change the assignment to use a safe
fallback (e.g. isset($style_settings['font_size']) ?
$style_settings['font_size'] : $atts['font_size'] or the null coalescing
equivalent) so $atts keeps a sensible default when font_size is not provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a5ee820-efde-4600-bb29-fb607220d665
📒 Files selected for processing (2)
classes/controllers/FrmEmailStylesController.phpclasses/models/FrmTableHTMLGenerator.php
1 participant
Related issue: https://github.com/Strategy11/formidable-pro/issues/6378
Related PR: https://github.com/Strategy11/formidable-pro/pull/6388
Summary by CodeRabbit
New Features
Improvements