Conversation
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| PHP |  |
Mar 25, 2026 3:27p.m. | Review ↗ |
| JavaScript |  |
Mar 25, 2026 3:27p.m. | Review ↗ |
|
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:
📝 WalkthroughWalkthroughAdds an admin forms-list feature: new JS and SCSS assets, screen options UI and templates, a Settings column and optional form descriptions in the list, and persistence of the description visibility preference. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Admin Browser
participant JS as forms-list.js
participant PHP as WP Admin (FrmFormsController)
participant DB as WP User Options
Browser->>JS: click settings / toggle / Apply
JS->>JS: build settings payload
JS->>PHP: submit adv-settings form (POST)
PHP->>DB: update options (per-page, frm_forms_show_desc, columns)
PHP->>Browser: respond / reload listing
Browser->>JS: reflect updated UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/controllers/FrmFormsController.php (1)
50-61:⚠️ Potential issue | 🟡 MinorDocumentation placeholder needs version number.
The docblock uses
@since x.xwhich should be updated to the actual version number before release.
🤖 Fix all issues with AI agents
In `@classes/controllers/FrmFormsController.php`:
- Around line 1248-1256: Fix the typo in the settings wrapper class by renaming
the HTML class "frm-forms-list-settings-btn-wrappper" to
"frm-forms-list-settings-btn-wrapper" in FrmFormsController (where
$columns['settings'] is set) and update the matching selector in the SCSS/CSS so
both markup and styles use the corrected "wrapper" spelling; ensure you update
any other occurrences of the misspelled class name across the codebase to keep
markup and styles consistent.
- Around line 1319-1320: The update_user_option call in FrmFormsController
directly reads $_POST['frm_forms_show_desc'] without nonce verification; before
using that POST value (the call around update_user_option(..., ! empty(
$_POST['frm_forms_show_desc'] ))), either perform a nonce check with
wp_verify_nonce() or check_admin_referer() using the nonce name used by the
form, or if this POST access is intentionally safe, add the same phpcs ignore
comment used elsewhere (// phpcs:ignore
WordPress.Security.NonceVerification.Missing) immediately above the line; update
the method in FrmFormsController accordingly so the POST read is protected or
explicitly whitelisted.
In `@classes/helpers/FrmFormsListHelper.php`:
- Around line 304-306: The form description output in FrmFormsListHelper (the
concat that builds $val using nl2br( $item->description )) is not escaped and
can lead to XSS; update the concatenation to escape the description before
converting newlines (e.g., call esc_html() or an appropriate WP escaping
function on $item->description and then pass that result to nl2br) so the output
is safely encoded while preserving line breaks, keeping the conditional around
get_user_option( 'frm_forms_show_desc' ) and the $item->description reference
intact.
In `@js/admin/forms-list.js`:
- Around line 6-24: In handleClickFormsListSettings, guard against a null btn
(computed from event.target or its closest 'a') before accessing
btn.nextElementSibling or calling btn.after: after computing const btn = ... add
a null check (if (!btn) return;) so the handler safely no-ops when the click
target isn't an <a> or inside one; this prevents the TypeError when btn is null
while leaving existing logic for toggling/moving the dropdown (refer to btn,
event.target, and frm-forms-list-settings).
- Around line 50-64: The handler handleClickCollapsibleBtn must guard against
nulls: verify that container (result of
event.target.closest('.frm-collapsible-box')) is non-null before using it, then
query for the '.frm-collapsible-box__content' element and ensure that content
exists before toggling its class, and finally keep the existing check for svgUse
(container.querySelector('.frm-collapsible-box__btn use')) but only access
svgUse.href.baseVal after confirming svgUse is not null; update the function to
return early when container or content is missing to avoid TypeError.
In `@resources/scss/admin/components/_forms-list.scss`:
- Around line 1-3: Fix the typo in the CSS class name by renaming
.frm-forms-list-settings-btn-wrappper to .frm-forms-list-settings-btn-wrapper in
the SCSS file, and update the corresponding HTML output in the
FrmFormsController::get_columns() method to use the corrected class name so both
stylesheet and generated markup match; search for all occurrences of
"frm-forms-list-settings-btn-wrappper" (including in get_columns()) and replace
them with "frm-forms-list-settings-btn-wrapper", then verify styling still
applies.
🧹 Nitpick comments (1)
classes/controllers/FrmFormsController.php (1)
3712-3798: Consider adding a maximum value for the per-page input.The number input on Line 3789 has
min="1"but nomaxattribute. While unlikely to cause issues, allowing extremely large values could impact performance when rendering the forms list.Proposed enhancement
-<input id="frm-forms-list-per-page" type="number" value="<?php echo intval( $per_page ); ?>" min="1" data-screen-option-id="formidable_page_formidable_per_page" /> +<input id="frm-forms-list-per-page" type="number" value="<?php echo intval( $per_page ); ?>" min="1" max="999" data-screen-option-id="formidable_page_formidable_per_page" />
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/controllers/FrmFormsController.php (1)
1309-1318:⚠️ Potential issue | 🔴 Critical
frm_forms_show_descis updated on every screen option save across all admin pages.The
update_user_optioncall on Line 1315 executes whenever theset-screen-optionfilter fires for any option that isn'tformidable_page_formidable_per_page. When a user saves screen options on the Posts page, Users page, etc.,$_POST['frm_forms_show_desc']will be absent, resetting the preference tofalse.Gate this behind a page check:
Proposed fix
public static function save_per_page( $save, $option, $value ) { if ( $option === 'formidable_page_formidable_per_page' ) { + // phpcs:ignore WordPress.Security.NonceVerification.Missing + update_user_option( get_current_user_id(), 'frm_forms_show_desc', ! empty( $_POST['frm_forms_show_desc'] ) ); + return (int) $value; } - // phpcs:ignore WordPress.Security.NonceVerification.Missing - update_user_option( get_current_user_id(), 'frm_forms_show_desc', ! empty( $_POST['frm_forms_show_desc'] ) ); - return $save; }
🧹 Nitpick comments (2)
js/admin/forms-list.js (1)
66-71: No mechanism to close the settings dropdown when clicking outside.The dropdown is toggled by clicking the gear icon but there is no listener to close it when the user clicks elsewhere on the page. Consider adding a
clicklistener ondocument(or a similar approach) that hides#frm-forms-list-settingswhen the click target is outside the dropdown and the trigger button.classes/controllers/FrmFormsController.php (1)
1243-1251: Duplicateform_typecheck can be consolidated.Lines 1237 and 1243 both check
'trash' !== FrmAppHelper::simple_get( 'form_type' ). Consider extracting the result into a local variable to avoid the repeated call.Proposed refactor
+ $is_trash = 'trash' === FrmAppHelper::simple_get( 'form_type' ); - if ( 'trash' !== FrmAppHelper::simple_get( 'form_type' ) ) { + if ( ! $is_trash ) { $columns['shortcode'] = esc_html__( 'Actions', 'formidable' ); } $columns['created_at'] = esc_html__( 'Date', 'formidable' ); - if ( 'trash' !== FrmAppHelper::simple_get( 'form_type' ) ) { + if ( ! $is_trash ) { $columns['settings'] = '<div class="frm-forms-list-settings-btn-wrapper">
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
classes/controllers/FrmFormsController.php (1)
1309-1318:⚠️ Potential issue | 🟠 MajorBug:
frm_forms_show_descis reset on every screen-option save across all admin pages.The
update_user_optioncall on line 1315 runs for every invocation of theset-screen-optionfilter, not just the Formidable forms listing page. When a user saves screen options on any other admin page (e.g., Posts, Pages),$_POST['frm_forms_show_desc']won't be set, so! empty(...)evaluates tofalse, silently disabling the form description preference.Guard this call so it only runs on the relevant page:
🐛 Proposed fix
public static function save_per_page( $save, $option, $value ) { if ( $option === 'formidable_page_formidable_per_page' ) { + // phpcs:ignore WordPress.Security.NonceVerification.Missing + update_user_option( get_current_user_id(), 'frm_forms_show_desc', ! empty( $_POST['frm_forms_show_desc'] ) ); return (int) $value; } - // phpcs:ignore WordPress.Security.NonceVerification.Missing - update_user_option( get_current_user_id(), 'frm_forms_show_desc', ! empty( $_POST['frm_forms_show_desc'] ) ); - return $save; }
🧹 Nitpick comments (2)
classes/controllers/FrmFormsController.php (2)
51-62: Docblock says "This adds screen options" but the method body doesn't.The
@since x.xnote says "This adds screen options," yethead()only enqueuesjquery-touch-punch. Screen options are wired viaadd_screen_options()and hooks elsewhere. Consider updating the annotation to reflect what actually changed (e.g., "Added docblock" or referencing the hook registration that ties into this method).
1237-1250: Consider caching the trash check to avoid duplicatesimple_getcalls.
FrmAppHelper::simple_get( 'form_type' )is called twice with the same argument (lines 1237 and 1243). A local variable would reduce duplication.♻️ Proposed refactor
+ $is_trash = 'trash' === FrmAppHelper::simple_get( 'form_type' ); - if ( 'trash' !== FrmAppHelper::simple_get( 'form_type' ) ) { + if ( ! $is_trash ) { $columns['shortcode'] = esc_html__( 'Actions', 'formidable' ); } $columns['created_at'] = esc_html__( 'Date', 'formidable' ); - if ( 'trash' !== FrmAppHelper::simple_get( 'form_type' ) ) { + if ( ! $is_trash ) { $columns['settings'] = '<div class="frm-forms-list-settings-btn-wrapper">
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@classes/helpers/FrmFormsListHelper.php`:
- Around line 306-312: The code appends a description twice in excerpt mode
because get_form_name() already calls add_form_description() when $mode ===
'excerpt'; to fix, avoid appending the description again in FrmFormsListHelper
where $val is built: change the conditional that appends '<p
class="frm_form_desc">' . nl2br($item->description) . '</p>' to skip when $mode
=== 'excerpt' (or otherwise unify the logic so only add_form_description()
handles excerpts and this block only handles non-excerpt/full-description
cases), referencing get_form_name(), add_form_description(), $mode and
$item->description to locate the affected logic.
| $val = $this->get_form_name( $item, $actions, $edit_link, $mode ); | ||
|
|
||
| if ( get_user_option( 'frm_forms_show_desc' ) && ! empty( $item->description ) ) { | ||
| $val .= '<p class="frm_form_desc">' . nl2br( $item->description ) . '</p>'; | ||
| } | ||
|
|
||
| $val .= $action_links; |
There was a problem hiding this comment.
Potential double description in excerpt mode.
get_form_name() already calls add_form_description() (line 494), which appends a truncated description when $mode === 'excerpt' (line 520). Then lines 308–310 append the full description again based on the user option, regardless of mode. When both conditions are true, the description appears twice.
Consider guarding lines 308–310 to skip when $mode === 'excerpt', or unify the two description paths.
Proposed fix
- if ( get_user_option( 'frm_forms_show_desc' ) && ! empty( $item->description ) ) {
+ if ( 'excerpt' !== $mode && get_user_option( 'frm_forms_show_desc' ) && ! empty( $item->description ) ) {
$val .= '<p class="frm_form_desc">' . nl2br( esc_html( $item->description ) ) . '</p>';
}🤖 Prompt for AI Agents
In `@classes/helpers/FrmFormsListHelper.php` around lines 306 - 312, The code
appends a description twice in excerpt mode because get_form_name() already
calls add_form_description() when $mode === 'excerpt'; to fix, avoid appending
the description again in FrmFormsListHelper where $val is built: change the
conditional that appends '<p class="frm_form_desc">' . nl2br($item->description)
. '</p>' to skip when $mode === 'excerpt' (or otherwise unify the logic so only
add_form_description() handles excerpts and this block only handles
non-excerpt/full-description cases), referencing get_form_name(),
add_form_description(), $mode and $item->description to locate the affected
logic.
There was a problem hiding this comment.
@Crabcyborg If the mode is "excerpt" and our Show form description is enabled, it will show a truncated description and the full description below. Should we ignore the excerpt when our setting is enabled?
There was a problem hiding this comment.
@truongwp, that's a good approach. If the "Show form description" setting is enabled, the new <p class="frm_form_desc"> block already handles displaying the description in full (for all modes), so the old truncated-excerpt path in add_form_description() becomes redundant. Suppressing the excerpt when the setting is on avoids the double-description without needing the 'excerpt' !== $mode guard on the new block.
Here's the suggested fix in add_form_description():
private function add_form_description( $item, &$val ) {
global $mode;
- if ( 'excerpt' === $mode && ! is_null( $item->description ) ) {
+ if ( 'excerpt' === $mode && ! is_null( $item->description ) && ! get_user_option( 'frm_forms_show_desc' ) ) {
$val .= FrmAppHelper::truncate( strip_tags( $item->description ), 50 );
}
}This way:
- Setting disabled + excerpt mode → truncated description shown as before (no regression).
- Setting enabled (any mode) → full description shown via the new
<p class="frm_form_desc">block; the old excerpt path is skipped.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@truongwp I tested this feature and found a couple of issues as listed below:
Let me know if you need any further details. Thank you! |
|
@lauramekaj1 Thanks for testing this. I'm fixing them. For your questions:
|
|
@lauramekaj1 I fixed all the issues you reported and made the settings panel close when clicking outside. Please test this again. Notice: The column checkboxes now affect immediately without clicking the "Apply" button. This is the same as the WordPress behavior. |
lauramekaj1
left a comment
lauramekaj1
left a comment
There was a problem hiding this comment.
@truongwp I tested it and it's working as expected. Thank you!
| $val = $this->get_form_name( $item, $actions, $edit_link, $mode ); | ||
|
|
||
| if ( get_user_option( 'frm_forms_show_desc' ) && ! empty( $item->description ) ) { | ||
| $val .= '<p class="frm_form_desc">' . nl2br( $item->description ) . '</p>'; |
There was a problem hiding this comment.
@truongwp Can we filter this $item->description variable at all? Like through kses?
There was a problem hiding this comment.
I added FrmAppHelper::kses().
Crabcyborg
left a comment
Crabcyborg
left a comment
There was a problem hiding this comment.
@truongwp I believe with this update we're planning to also remove the screen options tab, since the options are now all covered with the new gear icon.
Ok. Let me update the code for that change. Will the Screen Options tab be removed in this PR or in the future? |
|
@Crabcyborg This button shows the form description. Will we remove it? 
|
That button shows the excerpt, not the full description, so I think we will leave it there and use our own logic. Now the forms list settings work with the screen options disabled. You can test with this hook |
3 participants
Handoff: https://linear.app/strategy11/document/feature-handoff-aa4fc7646f11
This adds a new gear icon to change the screen options more quickly.
Summary by CodeRabbit
New Features
Style
Chores