FIX: Repeater required validation after row removal https://github.co…

[Gawuww]

Issue - #564

[github-actions]

🤖 AI PR Review

Risk level: medium

Review

Summary

• The only real source change in this PR is in assets/src/frontend/multi.step/PageState.js. It introduces page-level inputBindings (a Map) and new helpers: registerInput, unregisterInput and getTrackedInputs. The compiled/bundled frontend/admin/editor assets were updated accordingly (many built files have changed versions/hashes).

What the change does

• Instead of only pushing inputs into a simple array this.inputs and attaching watchers directly, PageState now records bindings in a Map and stores the watchers (clear functions) on registration. This is a good direction for being able to remove watchers when inputs are removed, which is necessary for repeater row removal where inputs disappear from the DOM.

Files changed (important)

• assets/src/frontend/multi.step/PageState.js
• Many compiled assets under assets/build/... (expected from the build)

Positive

• Registering watchers and keeping their clear callbacks in a map is the correct approach to avoid stale listeners and ensure correct validation state after dynamic DOM changes (row removal, repeaters).
• The change keeps the previous behaviour (observeInput still returns the input) but centralises registration logic.

Issues / Risks / Things to address

1. Missing unregister usage (likely the root cause still):

   • The PR adds unregisterInput(input) and registerInput(input) on PageState, but I could not find any place in this PR that actually calls unregisterInput when an input is removed. If inputs are removed (for example, when a repeater row is deleted), the watchers registered by registerInput will remain active unless something else calls unregisterInput. That can keep the removed input's restrictions/watchers active and lead to incorrect validation state or memory leaks.
   • Please ensure that input removal triggers PageState.unregisterInput for that input. Possible hook points:
     • InputData.onRemove (called from Observable.remove) — call pageState.unregisterInput(this) there if the input belongs to a PageState.
     • Observable.remove() — when a data input is removed from the observable, find the owning PageState (root.multistep) and call unregisterInput.
   • Without such cleanup the change can cause false negatives/positives in validation after rows are deleted and cause listeners to pile up on long-lived pages.

2. Repeater / nested / multi-scope cases

   • Ensure unregisterInput covers nested repeaters and inputs that move between scopes. Observers and bindings must be removed exactly when the DOM nodes are removed.
   • If a row is moved between pages/scopes, make sure to unregister from the old PageState and register on the new one.

3. Memory / lifecycle cleanup

   • PageState should make sure to clear all watchers and bindings when the multistep instance / scope is destroyed to avoid retained references to DOM nodes.
   • When a PageState is destroyed (if that ever happens), iterate inputBindings and call the stored clear watchers.

4. Performance

   • Using a Map is fine. Just ensure unregister logic is O(1) (Map.delete) which it is.
   • For sites with many inputs in a page or large repeaters, make sure observe/registration logic is not called unnecessarily.

5. Tests

   • No unit/integration tests included. This change addresses a dynamic/DOM lifecycle bug; please add an automated test (or at least a manual test doc) that covers:
     • Create repeater rows with required fields inside a multi-step page.
     • Validate that removing a row immediately removes its validation influence (i.e. the page validity updates accordingly).
     • Ensure no JS errors / no leaked watchers after repeated add/remove operations.

6. Backwards compatibility

   • This is an internal implementation change. It doesn't change public APIs but it changes lifecycle management. Ensure any third-party code relying on previous internals won't be broken. The new methods are additional so should be backwards compatible, but the missing unregister call could leave behaviour unchanged (still broken) until addressed.

7. Small suggestion: central place to add unregister call

   • Observable.remove() currently calls each input.onRemove(). Implement InputData.onRemove to call something like:
     if (this.root?.multistep) {
     const page = this.root.multistep.getCurrentPage() || this.root.multistep.getPages().find(p => p.getTrackedInputs?.()?.includes(this));
     page?.unregisterInput?.(this);
     }
     or even better: when creating inputs in PageState.registerInput, attach a small onRemove wrapper to the input that will call pageState.unregisterInput automatically when the input is removed.
   • That approach keeps registration/unregistration paired in one place and reduces chance of missing cleanup.

Other notes

• The PR updates many compiled asset files (admin, editor, frontend) — these are expected for a build but they make the diff very large. I focused on the source change.
• No security issues spotted in the changed source file (no new data being sent remotely, no nonce/capability changes). Still keep an eye on any future changes touching server requests.

Required changes before merge

• Ensure inputs are unregistered when rows/inputs are removed (call unregisterInput on removal). If this is implemented elsewhere in the codebase (not included in this PR), please point to the call or add a comment linking where cleanup happens. If not implemented, add the unregister call (either via input.onRemove or Observable.remove hooking into PageState.unregisterInput).
• Add a unit/integration or at least a QA checklist for repeater row removal + multi-step pages validation.

Optional improvements

• Add a PageState.destroy() that clears all bindings (useful for single-page-app lifecycles).
• Add tests to prevent regressions.

Overall recommendation: HOLD until unregister/cleanup is confirmed or implemented. If unregister is already called elsewhere (not in this diff), then change is acceptable and should be merged after tests are added.

Suggested changelog entry

- FIX: Repeater required validation after row removal in multi-step forms (PageState input registration/cleanup improvements)

[github-actions]

🤖 AI PR Review

Risk level: medium

Review

Summary

• This PR fixes repeater validation edge-cases after rows are removed by making the page-state track repeater inner inputs and unregister stale inputs. It also hardens ObservableRow re-observation (preserves previous values, handles native date/time inputs) and improves macro handling for repeater fields. Minor improvements to text-field signal: native date/time inputs are formatted before writing to the DOM.

What I looked at

• Frontend multi-step page state: assets/src/frontend/multi.step/PageState.js (+ build)
  • Added registerInput(), unregisterInput(), getTrackedInputs() and inputBindings Map.
  • Files: assets/src/frontend/multi.step/PageState.js and assets/build/frontend/multi.step.js
• Repeater frontend: modules/blocks-v2/repeater-field/assets/src/frontend/* (+ multiple built files)
  • ObservableRow.js — preserves previous values on reObserve, stamps inputs with _observeVersion, handles native date/time inputs, clone helpers added.
  • index.js — sync logic to register/unregister inputs with page state; avoids stale watchers; uses pageState.registerInput/unregisterInput and getTrackedInputs to cleanup.
  • signal.js — small build change (export formatting only) and built bundle updated.
  • macro resolver changes to bind listeners so repeater inner inputs trigger macro updates.
• Text-field signal: modules/blocks-v2/text-field/assets/src/frontend/field/SignalTextField.js and build changes
  • Converts numeric timestamp-like values into proper native date/time/datetime-local formatted values before updating DOM nodes.
• Many built asset/version bumps across editor/frontend bundles included.

Positive

• The approach to explicitly register/unregister inner repeater inputs in PageState is appropriate and addresses the root cause (stale watchers keeping page as invalid after removal).
• stamp/_observeVersion avoids re-binding or mis-detecting stale inputs — nice and efficient.
• Handling native date/time inputs in both ObservableRow reObserve and text-field signal prevents clobbering those nodes with raw timestamps.

Security

• No server-side or new REST endpoints introduced. Security-sensitive items (nonces, capability checks) unaffected by these client-only changes.
• Macro-binding code attaches event listeners to nodes; this is DOM-only and does not introduce new XSS vectors (rendered macro templates are still produced elsewhere). Continue to ensure any HTML inserted by macros is sanitized on server-side where used.

Performance & Scalability

• Good: unregisterInput() clears watches set by registerInput so watchers should not leak after removal.
• Good: getTrackedInputs() + stale-detection avoids N+1 or duplicate watchers in long repeaters.
• Concern: adding more watchers for every inner input can be expensive on very large repeaters (hundreds of rows). However this was already required for validation; this change reduces stale watches rather than adding unnecessary ones. Still, recommend QA on large forms.

Backward compatibility

• Changes are frontend-only and should be backward compatible for most cases.
• Potential break for custom code that relied on PageState.observeInput returning previously unwrapped behavior — new register/unregister API is internal but public addons/snippets might call pageState.observeInput directly. Consider documenting new methods or keeping observeInput as wrapper (PR keeps observeInput but it now delegates to registerInput, so existing calls keep working).
• Text-field value formatting for date/time: if external code expects raw numeric timestamp in the input DOM value, it will now see formatted date/time strings. This is intended and correct for native inputs, but it's a behavioural change to surface-date fields — please validate integrations that rely on raw numeric DOM values.

Edge cases and risks

• Event listeners bound in macro resolver (__jfbMacrosRepeaterBound) are not explicitly removed when rows are removed. This probably isn't a memory leak for the typical lifecycle (form DOM removed with page), but consider removing bound listeners in ObservableRow.onRemove or when rows are manually removed to be thorough.
• Nested repeaters: I did not see explicit tests for nested repeater scenarios. The logic uses input.root?._observeVersion and root identity checks — nested repeaters may need extra verification.
• Input binding map keys are the input objects — ensure no duplicate identity issues when inputs are re-created (reObserve now tries to preserve values and re-stamp versions, which helps). Still test cases where row nodes are cloned/templated by other code.
• Macro templating path uses innerHTML replacements in several places — ensure macros with HTML remain safe and escaped where intended.

Tests and QA

• Missing automated browser/unit tests for these important UI flows. Please add or run manual QA for:
  • Multi-step forms where a repeater is on a page that becomes hidden/visible and rows are added/removed. Verify required validation and next-page blocking/unblocking.
  • Repeater with dynamically-managed count (dynamically option), nested repeaters, and many rows (>100) to check performance.
  • Macro rendering for repeater fields after row add/remove.
  • Date/time inputs, check timezone/format correctness across browsers.

Smaller suggestions

• Consider using a WeakMap/WeakSet for tracking bound DOM nodes for macro-binding cleanup, or remove event listeners in onRemove to be explicit.
• Add a short inline comment in PageState.registerInput/unregisterInput describing the expected lifecycle and why includeInValidation flag is used (helps future maintenance).
• Add a changelog/test note about the text-field behavior change for native date/time inputs so integrators are aware.

Overall

• The changes look well thought-through and address the reported issue (Repeater Field: Remove a row causes submit button to a preventDefault state #564). The introduced mechanisms (inputBindings Map, observe-version stamping, explicit unregister) make the lifecycle explicit and should fix the “required validation remains after row removal” bug. Given the scope (critical form validation UX) and breadth of frontend changes, mark this PR as medium risk — require QA and tests as described before merge.

Files of interest (high-level)

• modules/blocks-v2/repeater-field/assets/src/frontend/ObservableRow.js — preserves values and stamps inputs
• modules/blocks-v2/repeater-field/assets/src/frontend/index.js — sync and unregister logic
• assets/src/frontend/multi.step/PageState.js — registerInput / unregisterInput additions
• modules/blocks-v2/text-field/assets/src/frontend/field/SignalTextField.js — native date/time formatting
• Many build files updated (assets/build/...), ensure built output matches src changes and no accidental omissions.

Approve after

• Add/execute automated or manual tests listed above.
• Optional: remove/better-manage macro event listeners on row removal (recommended but not blocking).

Suggested changelog entry

- FIX: Repeater required validation after row removal (multi-step forms / repeater field)