Optimize selector queries in form builder

[Crabcyborg]

No description provided.

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 28 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml
• Review profile: CHILL
• Plan: Advanced
• Run ID: 636061e1-cdfb-4025-a7fa-20fad75765f6

📥 Commits

Reviewing files that changed from the base of the PR and between 4742001 and af5f891.

📒 Files selected for processing (2)

• js/formidable_admin.js
• js/src/admin/admin.js

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[deepsource-io]

DeepSource Code Review

We reviewed changes in 4742001...af5f891 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.

See full review on DeepSource ↗

Important

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.

PR Report Card

Overall Grade

Security   Reliability   Complexity   Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Oct 3, 2026 12:21p.m.	Review ↗
JavaScript		Oct 3, 2026 12:21p.m.	Review ↗

---

Important

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.

[franky-the-going-merry]

Approve. The selector scoping is behaviour-preserving; I checked it live in the form builder at c779e3d.

Checked live (Playground, Lite at this head + Pro, seeded forms with a 3-field row, a section, and 40 fillers + a late section with ajax_load on):

• Hover on a row sets frm-field-group-hover-target, and the group controls show on a multi-field row and hide on a single one, including for a field inside a section.
• Group controls: Duplicate (5 to 6 rows, 11 to 14 fields), the layout popup opens, Break turns a 3-field row into 3 rows, and Delete shows "delete these 3 selected field(s)" and removes only that group.
• Dragging a row by its handle into the section: the nested frm-over-droppable chain resolves through the new getDroppableTarget loop and the field lands inside the section.
• Section setup: all 3 ul.frm_sorting and the section title become droppable, and the section row draggable, both on first render and when the section is hydrated later (40-filler form, after scrolling to it).
• No page errors in any of these runs. The shipped js/formidable_admin.js contains the new code (checked for the forEach(...) in the hydrate path and the scoped .frm_field_loading queries).

Source checks: fieldsContainer can be null only off the builder page, and every new use sits behind $newFields.length > 0 / buildInit(), so no new null dereference. setupSortableList matches the old jQuery(list).children(...).children(...) lookup via :scope >. .frm-field-group-hover-target is only ever added inside #frm-show-fields lists, so scoping those lookups cannot miss a target. The code-review skill's other findings are about the loading-placeholder code already merged in #3533, not lines this PR touches, so none are posted here.

Non-blocking:

• A few document.-wide lookups of the same class remain (.frm-field-group-hover-target in closest/querySelectorAll near lines 3299, 5000 and 5476). Scoping them the same way would make the change consistent; not needed for correctness.
• The PR description is empty. A line on what was measured (query count / time on a large form) would help the next reader.

Not exercised: timing or perf on a large form, rebuilding the bundle from source to compare it byte for byte, security-review and simplify (a DOM-scoping refactor with no new data handling), PHPUnit, Cypress (skipped in CI), and the failed-placeholder state.

[franky-the-going-merry]

Approve again at af5f891, plain removal. Only change since my last review (c779e3d) is af5f891 "More small improvements": ignoreLoadingFieldMouseEvent now returns early while body has frm-dragging, and mouseup is no longer in the list of events it swallows on a loading placeholder. js/formidable_admin.js was rebuilt to match (checked the minified buildInit list and the frm-dragging guard are both in it).

Live on Playground (Lite at the PR head + Pro,36-field form with ajax_load on, so the builder opens with ~27 li.frm_field_loading rows). Dispatched the five mouse events (mousedown, mouseup, mousemove, mouseover, click) at the first placeholder and recorded which reach a document-level listener:

• c779e3d: idle none, during a drag none. All swallowed, even while a field is being dragged over it.
• af5f891: idle only mouseup (the other four swallowed), during a drag all five. That is what the new comment says.

mouseup passing through while idle is safe: the mouseup listeners in admin.js are maybeHideShortcodes (line 11977, delegated on #frm_builder_page) and the checked-radio un-check helper in maybeUncheckRadio (line 4971, bound to the radio itself); neither selects a field. The synthetic idle events selected nothing and opened no field settings.

NOT run: a reliable end-to-end drag compare. My scripted mouse drag only engaged jQuery UI's helper in 1 of ~9 attempts on either commit (timing against lazy widget attach), so I am not claiming the drop itself was observed before/after; the event-level check above is the evidence. Also not run: code-review/security-review skills on this 4-line delta (they covered the rest at c779e3d), perf timing, PHPUnit/Cypress (skipped in CI). The notes from my first review (leftover document-wide hover-target lookups, empty PR body) are unchanged and non-blocking.