You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
We reviewed changes in 1d27050...65451e0 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.
Use the checkboxes below for quick actions:
▶️ Resume reviews
🔍 Trigger review
📝 Walkthrough
Walkthrough
Adds 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.
Registers new JS asset formidable_forms_list (js/admin/forms-list.js) with dependency formidable_dom and enqueues it on the forms listing page (ensures jquery-ui-autocomplete first).
Adds head(), print_forms_list_templates(), and add_screen_options() in FrmFormsController; registers screen_settings filter and admin_footer action in FrmHooksController to inject screen options and render templates.
Forms List Columns & Persistence classes/controllers/FrmFormsController.php, classes/helpers/FrmFormsListHelper.php
Adds a settings column when not trash; save_per_page() now persists frm_forms_show_desc; single_row() sanitizes data-colname for settings, renders the new settings column, and appends form descriptions when the user option is enabled.
Admin JS Interaction js/admin/forms-list.js
Adds an IIFE that toggles/moves the settings dropdown, applies settings by copying values into the advanced screen-options form and submitting it, and toggles collapsible sections with icon swaps.
Adds SCSS for forms-list UI (settings button, dropdown, per-page control, collapsible boxes, narrow settings column) and imports it into frm_admin.scss.
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
Loading
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
"I’m a rabbit in the admin glade,
I nudged the columns, toggled the shade.
Per-page hops and descriptions unfurled,
A tiny dropdown brightens the world.
🥕✨ — cheers from the forms-list burrow."
🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (2 warnings)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 73.33% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check
⚠️ Warning
The title 'Rock: Redesigned forms list' does not accurately reflect the changeset. The 'Rock:' prefix appears to be a branch or context label unrelated to the PR changes, and 'forms list' differs from the actual scope (forms table/listing page redesign).
Revise the title to remove the 'Rock:' prefix and use a clearer descriptor such as 'Redesign forms table UI with screen options and settings column' to accurately reflect the scope of changes.
✅ Passed checks (1 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch redesigned-forms-table
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.
The reason will be displayed to describe this comment to others. Learn more.
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 | 🟡 Minor
Documentation placeholder needs version number.
The docblock uses @since x.x which 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 no max attribute. While unlikely to cause issues, allowing extremely large values could impact performance when rendering the forms list.
The reason will be displayed to describe this comment to others. Learn more.
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_desc is updated on every screen option save across all admin pages.
The update_user_option call on Line 1315 executes whenever the set-screen-option filter fires for any option that isn't formidable_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 to false.
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 click listener on document (or a similar approach) that hides #frm-forms-list-settings when the click target is outside the dropdown and the trigger button.
classes/controllers/FrmFormsController.php (1)
1243-1251: Duplicate form_type check 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.
The reason will be displayed to describe this comment to others. Learn more.
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 | 🟠 Major
Bug: frm_forms_show_desc is reset on every screen-option save across all admin pages.
The update_user_option call on line 1315 runs for every invocation of the set-screen-option filter, 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 to false, silently disabling the form description preference.
Guard this call so it only runs on the relevant page:
51-62: Docblock says "This adds screen options" but the method body doesn't.
The @since x.x note says "This adds screen options," yet head() only enqueues jquery-touch-punch. Screen options are wired via add_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 duplicate simple_get calls.
FrmAppHelper::simple_get( 'form_type' ) is called twice with the same argument (lines 1237 and 1243). A local variable would reduce duplication.
The reason will be displayed to describe this comment to others. Learn more.
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.
@Crabcyborg Do you have any ideas to skip and DeepSource and PHPCS on the same line? To separate that method, it will create some methods with many params, so I decided to ignore the error.
For the Oxlint errors, if you haven't been working on it, I can fix them.
The reason will be displayed to describe this comment to others. Learn more.
Do we even want the full long description? I feel like the design wasn't intending on a new setting - just a misunderstood redesign of the existing setting.
Maybe we can just name it "Show Excerpt" instead. I can ask Mike Dalton.
Razvan has confirmed that this is meant to replace the excerpt view toggle. It isn't intended to be a new feature, and we're not trying to just ignore the old excerpt view setting for any one who was already using it.
For now, we can go with naming the setting "Description Excerpt" instead.
Everything should work based on the previous setting.
The reason will be displayed to describe this comment to others. Learn more.
Also, Truong. Looking at my screenshot, the number for "Items per page" seems really close to the up/down arrows. This is on Firefox. I'm not sure if it's different in other browsers, but if we can add some )(just a tiny bit) extra padding between the number and the arrows I think it would look nicer.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
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