Build placeholders with JS and optimize spinner animations by Crabcyborg · Pull Request #3531 · Strategy11/formidable-forms

Crabcyborg · 2026-10-02T20:37:39Z

Measurement	Master	Updated	Benefit
Initial field/grid HTML	598 kB	382 kB	36.1% smaller
PHP grid/placeholder rendering¹	86.7 ms	83.5 ms	3.2 ms faster
AJAX JSON, all 28 batches	43.109 MB	42.969 MB	139.5 kB smaller
AJAX response construction	96.2 ms	96.4 ms	Essentially unchanged

Summary by CodeRabbit

New Features
- Form fields can now appear as lightweight placeholders while loading in the editor, then restore their field details when ready.
Improvements
- Placeholder loading indicators respond to whether fields are visible, and stop tracking fields that have finished loading or encountered an error.

coderabbitai · 2026-10-02T20:38:44Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 3aa0644b-ef15-4015-87cd-397b486b2cd5

📥 Commits

Reviewing files that changed from the base of the PR and between e3a2cf6 and 21c4d4b.

📒 Files selected for processing (6)

classes/controllers/FrmFieldsController.php
classes/views/frm-forms/form.php
js/formidable_admin.js
js/src/admin/admin.js
js/src/admin/fieldPlaceholders.js
tests/phpunit/fields/test_FrmFieldsController.php

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

AJAX-loaded builder fields now use indexed placeholders and a JSON manifest to carry field metadata. Client-side code restores that metadata and updates placeholder spinner visibility based on viewport position.

Changes

Builder field placeholders

Layer / File(s)	Summary
Manifest generation and placeholder rendering `classes/views/frm-forms/form.php`, `classes/controllers/FrmFieldsController.php`, `tests/phpunit/fields/test_FrmFieldsController.php`	The form view initializes and emits a JSON manifest for AJAX loads. The controller records field IDs and deduplicated definition indexes while rendering indexed placeholders. Tests cover AJAX placeholders, manifest contents, and normal rendering for initial-load and non-AJAX fields.
Client-side hydration and spinner lifecycle `js/src/admin/fieldPlaceholders.js`, `js/src/admin/admin.js`	Client-side code restores field metadata from the manifest at module load and during builder initialization. The spinner observer removes spinners when placeholders leave the viewport, continues observing placeholders after entry, and stops observing a placeholder when its field load fails.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Sequence Diagram(s)

sequenceDiagram
  participant FrmFieldsController
  participant FormView
  participant AdminJS
  participant FieldPlaceholders
  FrmFieldsController->>FormView: Record placeholder fields and definitions
  FormView->>AdminJS: Emit indexed placeholders and JSON manifest
  AdminJS->>FieldPlaceholders: Call hydrateFieldPlaceholders
  FieldPlaceholders->>FieldPlaceholders: Restore field metadata and remove manifest

Suggested reviewers: truongwp

Merge Risk: ⚪ Minimal · up to 21c4d

AJAX-loaded builder fields now use indexed placeholders with a JSON manifest, and spinners follow viewport visibility. No concrete merge-blocking issue was identified in the supplied changes.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 21c4d

The inspected change preserves existing access checks and does not establish a new privilege or data-exposure path. Remaining uncertainty concerns coordinated delivery of the updated page and browser code, and recovery from incomplete or corrupted loading.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

inferred — The inspected change affects placeholder presentation and loading within the selected form-builder page. Existing field retrieval remains scoped to the requested form, so client metadata manipulation alone does not establish access to arbitrary fields or additional authority.

Trust Boundaries and Controls

observed — The inspected loading and creation endpoints retain the frm_edit_forms capability check and frm_ajax nonce verification. Loading resolves fields through the requested form and skips IDs outside that result. These controls predate the PR and are not moved into the browser.

Resilience and Maintainability Implications

observed — The normal loading lifecycle preserves failure containment: fields are claimed before requests, completion releases queue capacity, deleted placeholders are skipped, and successful replacement removes observation of the old node. Failed placeholders remain claimed and now stop spinner observation before displaying an error; viewport re-entry does not duplicate existing spinner content.

Hardening Proposals

proposed — Deliver and roll back the manifest-producing PHP and compatible browser assets together, avoiding mixed versions that leave indexed placeholders undiscoverable by the loading queue.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the two main changes: building field placeholders with JavaScript and optimizing spinner animations.

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 2

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🛠️ Fix failing CI checks 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

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

Autopilot is currently an internal CodeRabbit preview.

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

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

deepsource-io · 2026-10-02T20:39:22Z

DeepSource Code Review

We reviewed changes in e3a2cf6...21c4d4b 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 ↗

PR Report Card

Overall Grade	Security Reliability Complexity Hygiene

Code Review Summary

Analyzer	Status	Updated (UTC)	Details
PHP		Oct 2, 2026 8:38p.m.	Review ↗
JavaScript		Oct 2, 2026 8:38p.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.

deepsource-io · 2026-10-02T20:39:50Z

+			);
+			$placeholder = ob_get_clean();
+			$index       = count( $expected );
+			$this->assertSame( '<li data-frm-placeholder="' . $index . '"></li>', $placeholder );


Call to an undefined method test_FrmFieldsController::assertSame()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-10-02T20:39:50Z

+			$placeholder = ob_get_clean();
+			$index       = count( $expected );
+			$this->assertSame( '<li data-frm-placeholder="' . $index . '"></li>', $placeholder );
+			$this->assertSame( (int) $field->id, $manifest->fields[ $index ][0] );


Call to an undefined method test_FrmFieldsController::assertSame()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-10-02T20:39:50Z

+			$this->assertSame( '<li data-frm-placeholder="' . $index . '"></li>', $placeholder );
+			$this->assertSame( (int) $field->id, $manifest->fields[ $index ][0] );
+			$definition = $manifest->definitions[ $manifest->fields[ $index ][1] ];
+			$this->assertStringContainsString( 'class="' . esc_attr( $definition[0] ) . '"', $legacy_html );


Call to an undefined method test_FrmFieldsController::assertStringContainsString()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-10-02T20:39:50Z

+			$this->assertSame( (int) $field->id, $manifest->fields[ $index ][0] );
+			$definition = $manifest->definitions[ $manifest->fields[ $index ][1] ];
+			$this->assertStringContainsString( 'class="' . esc_attr( $definition[0] ) . '"', $legacy_html );
+			$this->assertStringContainsString( 'data-formid="' . esc_attr( $definition[1] ) . '"', $legacy_html );


Call to an undefined method test_FrmFieldsController::assertStringContainsString()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-10-02T20:39:50Z

+			$definition = $manifest->definitions[ $manifest->fields[ $index ][1] ];
+			$this->assertStringContainsString( 'class="' . esc_attr( $definition[0] ) . '"', $legacy_html );
+			$this->assertStringContainsString( 'data-formid="' . esc_attr( $definition[1] ) . '"', $legacy_html );
+			$this->assertStringContainsString( 'data-ftype="' . esc_attr( $definition[2] ) . '"', $legacy_html );


Call to an undefined method test_FrmFieldsController::assertStringContainsString()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-10-02T20:39:51Z

+		}
+
+		$this->assertCount( 2, $manifest->definitions );
+		$this->assertSame( $expected, array_column( $manifest->fields, 0 ) );


Call to an undefined method test_FrmFieldsController::assertSame()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-10-02T20:39:51Z

+
+		$this->assertCount( 2, $manifest->definitions );
+		$this->assertSame( $expected, array_column( $manifest->fields, 0 ) );
+		$this->assertSame( $manifest->fields[0][1], $manifest->fields[1][1] );


Call to an undefined method test_FrmFieldsController::assertSame()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-10-02T20:39:51Z

+				)
+			);
+			$html = ob_get_clean();
+			$this->assertStringContainsString( 'id="frm_field_id_' . $field->id . '"', $html );


Call to an undefined method test_FrmFieldsController::assertStringContainsString()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-10-02T20:39:51Z

+			);
+			$html = ob_get_clean();
+			$this->assertStringContainsString( 'id="frm_field_id_' . $field->id . '"', $html );
+			$this->assertStringNotContainsString( 'data-frm-placeholder=', $html );


Call to an undefined method test_FrmFieldsController::assertStringNotContainsString()

The method you are trying to call is not defined, which can result in a fatal error.

deepsource-io · 2026-10-02T20:39:51Z

+			$this->assertStringContainsString( 'id="frm_field_id_' . $field->id . '"', $html );
+			$this->assertStringNotContainsString( 'data-frm-placeholder=', $html );
+		}
+		$this->assertSame( array(), $manifest->fields );


Call to an undefined method test_FrmFieldsController::assertSame()

The method you are trying to call is not defined, which can result in a fatal error.

franky-the-going-merry

Approving. The PR also reads as an equivalent replacement for the old placeholder template, with nothing dropped.

Live-verified on this branch (preview env, 36-field form with "Load and save form builder page with AJAX" on):

The raw builder HTML has 27 data-frm-placeholder <li>s plus the frm-field-placeholders JSON script. After load the manifest is gone, all 27 are frm_field_loading with id/data-fid/data-formid/data-ftype restored, and all load to real fields. No console errors.
With field requests held, nothing is in view on landing and 0 spinners are added. Scrolled down, 7 of the 8 in-view placeholders had spinners (the 8th was probably just entering view) and 0 sat outside the viewport. Scrolled back up, 0 spinners remain. So spinners are added on entry and removed on exit, as described. Scrolled-to state:

Dropping root: postBodyContent behaves the same in practice, because the observer is still clipped by the scroll container.

Source checks: manifest JSON uses JSON_HEX_* and is applied with className/dataset (no HTML injection). The ajax_this_field guard excludes dividers, so using form_id instead of the old form_select branch is never reached. Existing nonce/capability checks are untouched. PHPUnit and all four Cypress shards pass. DeepSource's red PHP check is assertSame() flagged as undefined in the test file, which is a false positive.

Two small non-blocking notes inline.

franky-the-going-merry · 2026-10-02T20:58:12Z

 		},

 		buildInit() {
+			hydrateFieldPlaceholders();


Non-blocking: this second call looks redundant. formidable_admin is registered with in_footer = true, so it always runs after the builder markup, and the call at the top of this file already hydrates and removes the manifest. By the time buildInit() runs, this just does a getElementById that finds nothing. One call site makes it clearer which one guarantees hydration before observeFieldPlaceholders().

Suggested change

hydrateFieldPlaceholders();

franky-the-going-merry · 2026-10-02T20:58:12Z

+	const fieldsContainer = document.getElementById( 'frm-show-fields' );
+	if ( ! manifestElement || ! fieldsContainer ) {
+		return;
+	}


Non-blocking: the call at the top of admin.js runs this at module load with no guard. If the manifest script is ever empty or mangled (a CSP/minifier plugin, or wp_json_encode() returning false), JSON.parse throws and no later code in the bundle runs, including frmAdminBuildJS. Before this PR a bad placeholder only affected that one field. A try { ... } catch ( e ) { return; } around the parse would keep the failure contained. It only takes a malformed manifest to trigger, so it is a robustness nicety rather than a defect.

Build placeholders with JS and opimtize spinner animations

21c4d4b

Crabcyborg added this to the 6.36 milestone Oct 2, 2026

Crabcyborg added priority: high franky-review full automated qa Run analysis, PHPUnit, and Cypress E2E workflows labels Oct 2, 2026

Crabcyborg added full automated qa Run analysis, PHPUnit, and Cypress E2E workflows and removed full automated qa Run analysis, PHPUnit, and Cypress E2E workflows labels Oct 2, 2026

deepsource-io Bot reviewed Oct 2, 2026

View reviewed changes

Crabcyborg changed the title ~~Build placeholders with JS and opimtize spinner animations~~ Build placeholders with JS and optimize spinner animations Oct 2, 2026

franky-the-going-merry Bot added the franky-working Franky is actively reviewing this label Oct 2, 2026

franky-the-going-merry Bot approved these changes Oct 2, 2026

View reviewed changes

franky-the-going-merry Bot removed franky-review franky-working Franky is actively reviewing this labels Oct 2, 2026

Crabcyborg merged commit 0db2e17 into master Oct 2, 2026
77 of 106 checks passed

Crabcyborg deleted the build_placeholders_with_js_and_optimize_spinner_animations branch October 2, 2026 21:15

Conversation

Crabcyborg commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Security Architecture Review

❌ Failed checks (1 warning)

Uh oh!

deepsource-io Bot commented Oct 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

DeepSource Code Review

PR Report Card

Code Review Summary

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsController::assertSame()

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsController::assertSame()

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsController::assertStringContainsString()

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsController::assertStringContainsString()

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsController::assertStringContainsString()

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsController::assertSame()

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsController::assertSame()

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsController::assertStringContainsString()

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsController::assertStringNotContainsString()

Uh oh!

deepsource-io Bot Oct 2, 2026

Choose a reason for hiding this comment

Call to an undefined method test_FrmFieldsController::assertSame()

Uh oh!

franky-the-going-merry Bot left a comment

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

franky-the-going-merry Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Crabcyborg commented Oct 2, 2026 •

edited

Loading

coderabbitai Bot commented Oct 2, 2026 •

edited

Loading

deepsource-io Bot commented Oct 2, 2026 •

edited

Loading