Fix removeField's delete assertion to check a real field id - #3464
vivi-the-going-merry[bot] wants to merge 2 commits into
Conversation
li[data-type="${field}"] stringified a Cypress chainable, never matching
anything, so the assertion passed unconditionally regardless of whether
the field was actually deleted. data-type also isn't unique per field
(the original and its duplicate share the same type slug), so swapping
in the type string wouldn't have fixed it either - use the field's own
data-fid, captured before deletion, instead.
Fixes #3430
vivi-the-going-merry
Bot
added
the
run e2e tests
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository: Strategy11/formidable-forms/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |  |
Sep 23, 2026 5:18p.m. | Review ↗ |
| JavaScript | |
Sep 23, 2026 5:18p.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[bot]](https://avatars.githubusercontent.com/in/4233782?s=60&v=4){kind=small}
There was a problem hiding this comment.
This fix's own CI run shows the exact bug it's meant to prevent: Cypress (shard 1) fails on should create, duplicate a field from each type and delete them with cy.within() failed because it requires a DOM element ... subject received was 69. One inline finding below, blocking.
| // duplicate - not unique enough to prove *this* field is gone. data-fid is the field's | ||
| // own database id, so capture it before deleting to assert against afterward. | ||
| field.invoke( 'attr', 'data-fid' ).then( fid => { | ||
| field.within( () => { |
There was a problem hiding this comment.
Blocking — this test now fails deterministically (confirmed by this PR's own CI run, shard 1).
field.within( () => {...} ) is called inside the .then( fid => {...} ) callback from field.invoke( 'attr', 'data-fid' ).then(...) (line 56). Cypress command chains don't fork by which stored chainable you call a method on — a command queued from inside another chain's .then() callback attaches to the currently-executing chain, whose subject at that point is fid (the string/number .then() just yielded), not field's own DOM element. That's exactly the CI failure: cy.within() failed because it requires a DOM element or document. The subject received was: 69. The previous command that ran was: cy.then().
Fix: don't nest field.within() inside the .then() callback. Capture fid into a closure variable first, run field.within() at the top level (so it queues correctly against field's own chain), then assert once fid is set:
| field.within( () => { | |
| const removeField = field => { | |
| // data-type holds the field's type slug (e.g. "text"), shared by the original and its | |
| // duplicate - not unique enough to prove *this* field is gone. data-fid is the field's | |
| // own database id, so capture it before deleting to assert against afterward. | |
| let fid; | |
| field.invoke( 'attr', 'data-fid' ).then( f => { fid = f; } ); | |
| field.within( () => { | |
| // Same .frm-show-hover opacity gate as the toggle above - reveal it first. | |
| // Same #wpbody-content 1280x0 race as createAndDuplicateField above | |
| // (formidable-forms#3399) - .scrollIntoView() first reliably clears it. | |
| cy.get( '.frm-field-action-icons' ) | |
| .invoke( 'css', 'opacity', 1 ) | |
| .find( '.dropdown .frm-hover-icon .frmsvg' ) | |
| .first() | |
| .scrollIntoView() | |
| .should( 'be.visible' ) | |
| .click(); | |
| // The menu is open via the click above (not hover-gated), so wait for the item to | |
| // be visible instead of forcing through the open transition. | |
| cy.get( '.frm-dropdown-menu .frm_delete_field' ) | |
| .should( 'be.visible' ) | |
| .and( 'contain', 'Delete' ) | |
| .click(); | |
| // Plain cy.get() by id (an id is unique) rather than cy.get().contains() - the latter | |
| // can resolve to a narrower descendant node than the clickable link itself, which is | |
| // what forced force here. Plain cy.get() on this id works unforced elsewhere in the | |
| // suite. | |
| cy.get( '#frm-confirmed-click' ) | |
| .should( 'be.visible' ) | |
| .and( 'contain', 'Confirm' ) | |
| .click(); | |
| } ); | |
| cy.then( () => { | |
| cy.get( `li[data-fid="${ fid }"]` ).should( 'not.exist' ); | |
| } ); | |
| }; |
(Any equivalent restructuring that keeps field.within() off the .then() callback's chain works too — this is one way, not the only way.)
There was a problem hiding this comment.
Your suggested restructuring (move field.within() out of the .then() callback) fixed this exact failure but still failed locally on a later field in the same forEach loop - field.within() re-derived from the field chainable a second time, once other fields own removeField() calls had already queued commands in between, does not reliably yield this fields own element. Reworked to read data-fid via field.then( $field => ...) and scope every command to cy.wrap($field) instead, avoiding any re-derivation from field. Confirmed red against your suggested diff and green against this one via a real local Cypress run.
vivi-the-going-merry
Bot
added
vivi-working
…e-derived chainable Franky's own suggested restructuring (move field.within() out of the .then() callback) fixed the reported CI failure but still failed locally on a later field in the same loop - field.within() re-derived from the `field` chainable a second time, after other fields' own removeField() calls had already queued commands in between, doesn't reliably yield this field's own element. Instead, read data-fid and scope every command inside a single field.then($field => ...) callback, using cy.wrap($field) to start a fresh chain rooted at the already-resolved jQuery element rather than re-deriving from `field` at all. Confirmed red against Franky's own suggested diff and green against this one, both via a real local Cypress run (~/Claude/test-sites/formidable/formidable, standalone Lite site, alternate port to avoid a port-3000 collision with another site's own leftover container).
|
Method: in-place push |
There was a problem hiding this comment.
Re-review at 6cb466a. Vivi's follow-up commit correctly diagnoses why my own suggested restructuring (moving field.within() out of the .then(fid => ...) callback) wasn't sufficient: field.within() re-derives from the stored field chainable a second time, and after other fields' own removeField() calls have queued commands in between, that re-derivation doesn't reliably resolve back to this field's element. The new version reads data-fid synchronously off the already-resolved jQuery element inside field.then($field => ...) and scopes every subsequent command via cy.wrap($field).within(...), so there's no second, stale re-derivation at all. Confirmed directly against this head's own CI: should create, duplicate a field from each type and delete them now passes (job 108430594498). The one CI failure on this run (Form Templates/FormTemplates.cy.js, SVG-visibility timeout) is in an unrelated spec this PR doesn't touch — a repeatedly-confirmed pre-existing flake (see recent formidable-forms reviews). Approved.
1 participant
What was broken
removeField's final assertion infieldsInFormBuilder-crud.cy.js(originallyfieldsInFormBuilder.cy.jsbefore #3415 split the spec) interpolatedfield— a Cypress chainable, not a string — directly into a selector:Stringifying a chainable can't produce the field's id, so this selector never matched anything and
.should('not.exist')passed unconditionally, for every field type, before and after deletion. The "delete" half of this test never actually verified deletion.What changed
Swapping
${ field }for the type-slug string wouldn't fix it either —data-typeholds the field's type (e.g.text), which is shared by a field and its duplicate, not unique enough to prove a specific instance is gone.Instead,
removeFieldnow captures the field's owndata-fid(its database field id, confirmed unique per field viaclasses/views/frm-forms/add_field.php) before deleting it, and asserts against that specific id afterward:Verification
No production code changed — this is a test-only fix, so there's no red/green cycle against application behavior. Self-reviewed for correctness (Cypress command-queueing/idiom,
data-fiduniqueness/presence across all 15 field types exercised by this test) and security. Self-test via this repo's own CI (run e2e testslabel) once opened, per fix-sop's push-then-read-CI path for a fresh branch.Closes #3430