fix(js): only reset the clicked field when removing its last row by faisalahammad · Pull Request #1573 · CMB2/CMB2

faisalahammad · 2026-10-11T18:49:19Z

Description

When a repeatable field has only one row, clicking its Remove button "resets" the row: removeAjaxRow() clicks the field's Add Row button and then the Remove button again. The Add Row button was looked up like this:

Before (js/cmb2.js, removeAjaxRow()):

var $parent = $this.parents('.cmb-row');
...
return cmb.resetRow( $parent.find( '.cmb-add-row-button' ), $this );

After:

var $parent = $this.closest( '.cmb-row' );
...
// Only use this field's add button. Group rows are .cmb-row too.
var tableId = $table.attr( 'id' );
var $addBtn = $table.siblings( '.cmb-add-row' ).find( '.cmb-add-row-button' );
if ( ! $addBtn.length ) {
	// Markup was changed, so find the button that targets this table.
	$addBtn = $( '.cmb-add-row-button' ).filter( function() {
		return this.getAttribute( 'data-selector' ) === tableId;
	} ).first();
}
return cmb.resetRow( $addBtn, $this );

And cmb.resetRow() now stops early:

// Without a usable add button, the remove click would call this again forever.
if ( ! $addNewBtn.length || $addNewBtn.prop( 'disabled' ) ) {
	return;
}

.parents('.cmb-row') returns every ancestor with that class. Inside a group, the group entry (.cmb-repeatable-grouping) and the group wrapper (.cmb-repeat-group-wrap) also have cmb-row, so .find() matched the Add Row button of every repeatable field in the whole group. Now we take the button next to the field's own .cmb-repeat-table (it is rendered as a sibling p.cmb-add-row in CMB2_Types::render_repeatable_field()). If a plugin or theme wraps the table so the button is no longer a sibling, we fall back to the Add Row button whose data-selector is this table's id.

resetRow() clicks Add and then Remove. If the Add button is missing or disabled, no row is added, so the Remove click calls resetRow() again and the page hits Maximum call stack size exceeded. Now resetRow() does nothing in that case.

js/cmb2.min.js is rebuilt in a separate commit (npm run build:js). The e2e env runs with SCRIPT_DEBUG off, so the new Playwright test needs the min file. Feel free to drop that commit if you rather rebuild at release.

Motivation and Context

Fixes #1515.

With a group that has two or more repeatable fields, removing the only row of one field added an empty row to all the other repeatable fields in that group, and in the other group entries too.

Risk Level

Low. Admin JS only, and only the path where a repeatable field's only row is removed.

removeAjaxRow() is only bound to .cmb-remove-row-button clicks. The group reset in removeGroupRow() also uses resetRow(), so it gets the same missing or disabled button guard. With a normal button it works the same.
Top-level repeatable fields: the old selector already found only their own button, so the result is the same. The data-selector fallback only runs when the sibling lookup finds nothing.
Behavior change: if the Add Row button is disabled (for example by a snippet that limits rows), removing the last row now does nothing. Before, it looped until the browser threw an error.
$parent is now just the clicked row. After the reset branch it is only used for the empty-row check, and only repeat rows get that class (CMB2_Types::render_repeatable_field()), so that path works the same.
Behavior change: before, the reset fired cmb2_add_row once for every repeatable field in the group. Now it fires once, for the clicked field only.
No PHP, hook, or saved data changes.

Testing procedure

Manual, in wp-admin with a group that has two repeatable text fields plus a top-level repeatable text field:

Remove the only row of the first field in a group entry. It is cleared and keeps one row. The other field and the other group entries do not change.
Add a second group entry and repeat step 1 in each entry. The other entry does not change.
Add rows to a field (a, b, c) and remove "b". Only "b" is removed.
Remove the only row of the top-level field. It is cleared, nothing else changes.
Wrap the top-level table in the console (jQuery('#top_repeat_repeat').wrap('<div></div>')) and remove its only row. It resets to its default, no error.
Reload, disable its Add Row button (jQuery('[data-selector="top_repeat_repeat"]').prop('disabled', true)) and remove its only row. Nothing happens, no error.
Save and reload. Saved values match what was entered, no extra empty rows.
No JS errors in the console.

Automated:

New Playwright tests in tests/playwright/group-repeatable.spec.js, with a new box in tests/playwright/fixtures/test-fields.php (a repeatable group with two repeatable fields, plus a top-level repeatable field top_repeat). Three tests:
- Adds a second group entry and fills the other four fields, then removes the only row of the first field. The field goes back to its default DEF-A, all five repeat tables still have one row plus the hidden empty row, the other fields keep their values, and only one cmb2_add_row and one cmb2_remove_row event fire.
- Wraps the top_repeat table so the Add Row button is not a sibling, then removes its only row. It resets to TOP-DEF with no page errors.
- Disables the top_repeat Add Row button, then removes its only row. The value stays and there are no page errors.
```
npx playwright test group-repeatable.spec.js --project=chromium
```
To see them catch the bugs, put back an old min file and run again. With git show develop:js/cmb2.min.js > js/cmb2.min.js the first and the disabled-button tests fail. With the min file from b23737f9 (before the data-selector fallback and the guard) the wrapped-table and disabled-button tests fail.
npm run phptests (wp-env, PHP 8.3, WP latest): OK, 328 tests, 1073 assertions.
npm run build:js: eslint passes. Before the rebuild I checked that a fresh build of develop matches the committed cmb2.min.js, so the min diff only has this fix.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code follows the code style of this project.
My code and pull requests meets the Contributing guidelines.

Screenshots

No visual change. The only difference is that other fields no longer get an extra empty row.

Removing the only row of a repeatable field resets it by clicking the field's Add Row button and then Remove. The button was found with .parents('.cmb-row'), which also matches the group entry and the group wrapper, since they have the cmb-row class too. So every repeatable field in the group, and in its other entries, got an extra empty row. Look up the Add Row button next to the field's own repeat table instead, and use .closest() for the row. Adds a Playwright regression test with a group fixture. Fixes CMB2#1515

The e2e test env runs with SCRIPT_DEBUG off, so it loads the minified file. Rebuilt with npm run build:js.

jtsternberg

Justin's agent here (Claude Code), reviewing on behalf of @jtsternberg.

PR #1573: only reset the clicked field when removing its last row

Head b23737f · base develop · fixes #1515

Briefing

Origin: #1515. In a group with two or more repeatable fields, removing the only row of one field adds a row to every other repeatable field in the group, in every group entry.
Root cause (confirmed): 752f0e3 (2017, "related #312") added the reset-by-Add-then-Remove path as cmb.resetRow( $parent.find( '.cmb-add-row-button' ), $this ). It reused $parent = $this.parents('.cmb-row'), which has been there since the 2014 table→div conversion (106f91e). .parents() returns every .cmb-row ancestor. Inside a group those ancestors include .cmb-repeatable-grouping and .cmb-repeat-group-wrap, so .find() matches every Add Row button in the group and resetRow() clicks all of them.
Fix: $parent becomes closest('.cmb-row'), and the Add button is looked up as $table.siblings('.cmb-add-row').find('.cmb-add-row-button'). The PR also rebuilds the min file and adds a Playwright spec plus a fixture box.
Trade-off: the new lookup depends on the button being a DOM sibling of the table. The old one only needed the button somewhere inside an ancestor row. Markup that was tolerated before now sends resetRow() into infinite recursion (F2).

Verdict: REQUEST CHANGES

The diagnosis and the fix are correct for standard markup, and that is proven both ways: the spec fails on develop and passes on head. One small change is needed before merge. Any markup where the Add button isn't a sibling of the table now throws Maximum call stack size exceeded on a remove click that works on develop (F2). The fix is about 8 lines, and I ran it against every probe below.

Findings

F1: Scope: rebuilt `js/cmb2.min.js` is in the PR · nit/question · `js/cmb2.min.js`

CONTRIBUTING.md → "What we expect" #1 asks contributors to leave rebuilt *.min.js out. The author flagged this themselves (PR body: "Feel free to drop that commit"). It's needed here because .wp-env-tests.json sets SCRIPT_DEBUG: false (includes/CMB2_JS.php:149 serves .min), so the new e2e spec can't pass in CI without it. I checked the rebuild: running terser js/cmb2.js js/cmb2-wysiwyg.js js/cmb2-char-counter.js --compress --no-rename on head gives output byte-identical to the committed min, and the same command on develop reproduces develop's min. The diff therefore contains only this fix. None of #1571, #1572 or #1574 touch JS, so there's no min conflict between them. All four PRs except #1574 add a ### Bug Fixes heading under Unreleased in CHANGELOG.md, which will give trivial merge conflicts.
Recommendation: keep the commit (the CI spec depends on it), or drop it and rebuild at merge. That's Justin's call. The CONTRIBUTING rule doesn't account for e2e specs that need the min file. Consider adding an exception there.

F2: Correctness: empty Add-button lookup now recurses until the stack overflows · medium · blocking · `js/cmb2.js:770`, `js/cmb2.js:783`

When $table.siblings('.cmb-add-row') is empty, resetRow() clicks nothing for Add. It then clicks Remove, and the count is still ≤ 2, so it re-enters resetRow() with no end. The old ancestor-scoped find() still found the button when an integration wrapped the table (for styling, tabs or conditional-display wrappers) or moved p.cmb-add-row elsewhere inside the field row. An earlier private review raised this risk; the probe confirms it.

Evidence: probe P3 table wrapped by integration (add button no longer a sibling). It runs jQuery('#probe_top_repeat').wrap('<div class="third-party-wrap">') on a top-level repeatable text field with default: 'TOP-DEF', fills "typed", then clicks Remove:

PR head:  P3 value after reset: typed     P3 events: {"add":0,"remove":0}  P3 errors: ["Maximum call stack size exceeded"]
develop:  P3 value after reset: TOP-DEF   P3 events: {"add":1,"remove":1}  P3 errors: []

This also affects top-level (non-group) fields, which the PR describes as "the same result". That is true for standard markup only. Under C3, whichever bundled copy is newest serves every box on the site, so a theme's or plugin's wrapped markup breaks as soon as any other bundled plugin ships this version.

Recommendation: keep the sibling lookup first. If it comes back empty, fall back to the button that targets this table through data-selector, which is the same contract addAjaxRow() uses to resolve its table. Then guard resetRow() so an unusable button never recurses:

if ( number <= 2 ) {
	// Only use this field's add button. Group rows are .cmb-row too.
	var tableId = $table.attr( 'id' );
	var $addBtn = $table.siblings( '.cmb-add-row' ).find( '.cmb-add-row-button' );
	if ( ! $addBtn.length ) {
		$addBtn = $( '.cmb-add-row-button' ).filter( function() {
			return this.getAttribute( 'data-selector' ) === tableId;
		} ).first();
	}
	return cmb.resetRow( $addBtn, $this );
}
// …
cmb.resetRow = function( $addNewBtn, $removeBtn ) {
	if ( ! $addNewBtn.length || $addNewBtn.prop( 'disabled' ) ) {
		return;
	}
	// …

filter() with getAttribute avoids building a selector from the ID, so there's no escaping to get wrong. I ran this exact patch (rebuilt into the min file): the PR spec and P1–P6 all pass, P3 resets to TOP-DEF with 1 add/1 remove event, and nothing errors.

F3: Correctness (pre-existing, not caused by this PR): disabled Add button also recurses · low · nit / fold into F2 · `js/cmb2.js:783`

jQuery doesn't dispatch delegated click handlers on a disabled button. .prop('disabled', true) on Add Row is a common snippet for a third-party "max rows" limit. If it's set while a field has one row, resetRow() clicks nothing and recurses. Probe P6 disabled add button:

PR head:  P6 value after: typed  P6 errors: ["Maximum call stack size exceeded"]
develop:  P6 value after: typed  P6 errors: ["Maximum call stack size exceeded"]

The prop('disabled') check in the F2 guard fixes it (patched run: P6 errors: []). The same guard also protects removeGroupRow()'s resetRow() call. Not blocking on its own. It costs nothing to fold into F2.

F4: Testing: spec checks row counts but not defaults, other fields' values, events, or the F2 shape · low · should-fix · `tests/playwright/group-repeatable.spec.js:16`, `tests/playwright/fixtures/test-fields.php:50`

The spec proves the regression. With develop's min file restored it fails with Expected: 2 / Received: 3 on tables.nth(1), and it passes on head. Gaps:

The fixture fields have no default, so "reset restores the default" (the #312 contract) isn't asserted, only ''.
Only the clicked field is filled. A regression that cleared or replaced other fields' values without changing row counts would pass.
There's no case for F2's wrapped or relocated markup.

Recommendation: give first a default and assert it after reset. Fill second (and entry 2's fields) with sentinel values first and assert they're unchanged. Add a case that wraps the table before clicking Remove and asserts no pageerror and a reset value. Event counts (1 cmb2_add_row + 1 cmb2_remove_row per reset) are an optional extra.

Behavior changes / who observes them

JS events: a field reset inside a group now fires cmb2_add_row once, on the clicked field's table. Develop fired it once per repeatable table in the group, across every entry. Probe P1 (2 entries × 2 fields; the first event is the setup "add group" click): develop add:5, addTables:[group, g0_a, g0_b, g1_a, g1_b]; head add:2, addTables:[group, g0_a]. cmb2_remove_row fires once on both. The extra events were for the spurious rows themselves, so a listener can't legitimately depend on them. This is a bug fix, not a default flip, and C3 staging doesn't apply.
Saved data: develop's spurious rows are real inputs (P1 develop: the other three tables went from 2 to 3 .cmb-row). Repeatable sanitizing drops empty values, so for blank fields the save is likely unchanged. For fields with a default, a spurious row could be saved with the default value. I didn't verify this with a save/reload.
Top-level repeatables: unchanged on standard markup (P2: both builds reset to TOP-DEF, 1 add/1 remove). On non-sibling markup the behavior changes (F2).
Not affected (checked): PHP, hooks, REST and stored-value interpretation (JS-only diff). The middle-row removal path (P4: a,b,c → remove b → ["a","c"] on both). The last-group-entry reset through removeGroupRow() (P5: resets to DEF-A, no errors, on both).
$parent narrowing: the PR says the post-reset path "works the same", and that holds through the UI. Before, $parent.prev().addClass('empty-row') ran on every .cmb-row ancestor when the clicked row had empty-row. A user can't reach that, because the empty row is .hidden. closest() removes the latent multi-element behavior.

Fences checked

$this.parents('.cmb-row') as $parent → 106f91e (2014-08-17, "Working and converting from tables"). It was used only for hasClass('empty-row')/prev() until 752f0e3 reused it for the button lookup. Neither #312 nor 752f0e3 shows any cross-field reset intent. I searched with git log -S on both strings, read the full 752f0e3 diff, and read #312's comments in full. → changed (to closest()); the need (find the clicked row) is kept.
$parent.find('.cmb-add-row-button') in the reset path → 752f0e3 (2017, related #312). Built to reset the last row instead of removing it. → changed. Reset-to-default is kept (P1 DEF-A, P2 TOP-DEF), but the lookup is now sibling-only (F2).
The visible-row reset itself (not hiding) → #312 (jtsternberg: hiding "a radio input with 3 values… will still get set in the DB, and the user will not understand why"). → kept.
cmb.resetRow.resetting confirm suppression → b15d588 (2021, fixes #1427). → kept, untouched. Only removeGroupRow() reads it, and field-row Remove buttons carry no data-confirm (includes/CMB2_Types.php:408), so this PR's path never prompts.
Add-then-Remove reset order in resetRow() → 752f0e3. → kept.
The number <= 2 threshold (one row plus the hidden empty-row) → 752f0e3. → kept.

Claims from an earlier private review, checked

#312 rationale, 752f0e3, b15d588: confirmed (above).
"Keeps reset-to-default, visible row, add/remove events, confirm suppression": confirmed (P1/P2/P5). The events now fire once instead of once per table.
"Broad selector dates to 2014, no deliberate cross-field requirement": confirmed, with one detail. The selector is from 2014, but its use for the Add-button lookup is from 2017 (752f0e3).
"Sibling selector matches standard markup": confirmed. CMB2_Types::render_repeatable_field() (includes/CMB2_Types.php:338-345) is the only emitter of .cmb-add-row-button.
"Relocated button → recursion": confirmed (P3), and it's a regression against develop. The data-selector + empty-guard suggestion was validated by the patched run. The guard should also cover disabled (F3).
"min actually rebuilt": confirmed byte-identical.

Verification run

PHPUnit, full suite on head, isolated wp-env (port 2633, PHP 8.3): OK (328 tests, 1073 assertions). That matches the PR body. No PHP changed, so the fixture ([] arrays) is the only PHP the diff adds, and it's 7.4-safe. The 7.4 floor is only proven by CI (C7).
Playwright group-repeatable.spec.js (chromium, WP_BASE_URL=http://localhost:2633 SKIP_WP_SERVER=1): passes on head and fails on develop's min file (Expected: 2, Received: 3).
Full Playwright suite: inconclusive. One full chromium run hit 9 failures, all net::ERR_ABORTED / 30s navigation timeouts with no assertion failures, at host load average 24 (other test environments running in parallel). That included group-repeatable.spec.js, which passed in three separate targeted runs. Not evidence for or against the PR.

Justin's agent here (Claude Code), reviewing on behalf of @jtsternberg.

…led button When the add row button is not a sibling of the repeat table, look it up by its data-selector. Skip the reset when the add button is missing or disabled, so the remove click cannot call resetRow again forever. Addresses PR feedback. Refs CMB2#1573

… button Check that the clicked field resets to its default, that other fields keep their values, and that only one add and one remove event fire. Add a top-level repeatable field to cover a wrapped table and a disabled add button. Addresses PR feedback. Refs CMB2#1573

# Conflicts: # CHANGELOG.md

faisalahammad · 2026-10-12T10:58:17Z

Thanks for the review. Updated in 0f6aa36 and f4430a6.

F1: I kept the rebuilt cmb2.min.js, because the e2e env runs with SCRIPT_DEBUG off and the new tests need it. Happy to drop it if you rather rebuild at release.
F2: Used your patch. If the add button is not next to the table, it now finds the button by data-selector.
F3: resetRow() now returns early when the add button is missing or disabled. This also covers removeGroupRow().
F4: The main test now checks that the field goes back to its default, the other fields keep their values, and only one add and one remove event fire. I added a top-level repeatable field and two more tests. One wraps the table, one disables the add button. Both check there is no page error.

I also merged develop to fix the CHANGELOG conflict. The PR description is updated too.

codecov · 2026-10-12T11:00:20Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.16%. Comparing base (c244c31) to head (63ef0e3).

Additional details and impacted files

@@            Coverage Diff             @@
##             develop    #1573   +/-   ##
==========================================
  Coverage      72.16%   72.16%           
  Complexity      1911     1911           
==========================================
  Files             53       53           
  Lines           5034     5034           
==========================================
  Hits            3633     3633           
  Misses          1401     1401

Flag	Coverage Δ
phpunit	`72.16% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@handle

Props/Fixes order and @handle credit like the other entries, plus the two changes integrations can see: cmb2_add_row fires once per reset, and Remove with a disabled Add Row button is now a no-op. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KpfVcxMH9VE8CRmGRvjJPS Co-Authored-By: jtsternberg-bot <me+jtbot@jtsternberg.com>

faisalahammad added 3 commits October 12, 2026 00:48

build(js): rebuild cmb2.min.js

6ccf2a1

The e2e test env runs with SCRIPT_DEBUG off, so it loads the minified file. Rebuilt with npm run build:js.

docs(changelog): link PR CMB2#1573 for the CMB2#1515 fix

b23737f

jtsternberg requested changes Oct 11, 2026

View reviewed changes

faisalahammad added 3 commits October 12, 2026 16:57

Merge remote-tracking branch 'origin/develop' into pr-1573-review

63ef0e3

# Conflicts: # CHANGELOG.md

faisalahammad requested a review from jtsternberg October 12, 2026 11:11

jtsternberg merged commit c9fe955 into CMB2:develop Oct 13, 2026
18 checks passed

Navigation Menu

Repository navigation

fix(js): only reset the clicked field when removing its last row - #1573

fix(js): only reset the clicked field when removing its last row#1573
jtsternberg merged 6 commits into
CMB2:developfrom
faisalahammad:fix/1515-group-repeatable-row-reset

faisalahammad commented Oct 11, 2026 •

edited

Loading

Uh oh!

jtsternberg left a comment

Uh oh!

faisalahammad commented Oct 12, 2026

Uh oh!

codecov Bot commented Oct 12, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

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

Uh oh!

Description

Motivation and Context

Risk Level

Testing procedure

Types of changes

Checklist:

Screenshots

Uh oh!

jtsternberg left a comment

Choose a reason for hiding this comment

PR #1573: only reset the clicked field when removing its last row

Briefing

Verdict: REQUEST CHANGES

Findings

F1: Scope: rebuilt js/cmb2.min.js is in the PR · nit/question · js/cmb2.min.js

F2: Correctness: empty Add-button lookup now recurses until the stack overflows · medium · blocking · js/cmb2.js:770, js/cmb2.js:783

F3: Correctness (pre-existing, not caused by this PR): disabled Add button also recurses · low · nit / fold into F2 · js/cmb2.js:783

F4: Testing: spec checks row counts but not defaults, other fields' values, events, or the F2 shape · low · should-fix · tests/playwright/group-repeatable.spec.js:16, tests/playwright/fixtures/test-fields.php:50

Behavior changes / who observes them

Fences checked

Claims from an earlier private review, checked

Verification run

Uh oh!

faisalahammad commented Oct 12, 2026

Uh oh!

codecov Bot commented Oct 12, 2026

Codecov Report

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

faisalahammad commented Oct 11, 2026 •

edited

Loading

F1: Scope: rebuilt `js/cmb2.min.js` is in the PR · nit/question · `js/cmb2.min.js`

F2: Correctness: empty Add-button lookup now recurses until the stack overflows · medium · blocking · `js/cmb2.js:770`, `js/cmb2.js:783`

F3: Correctness (pre-existing, not caused by this PR): disabled Add button also recurses · low · nit / fold into F2 · `js/cmb2.js:783`

F4: Testing: spec checks row counts but not defaults, other fields' values, events, or the F2 shape · low · should-fix · `tests/playwright/group-repeatable.spec.js:16`, `tests/playwright/fixtures/test-fields.php:50`