Skip to content

Fix infinite loop in form widget with circular dependsOn declarations#1478

Open
austinderrick wants to merge 2 commits intowintercms:developfrom
austinderrick:fix/form-widget-circular-dependency-loop
Open

Fix infinite loop in form widget with circular dependsOn declarations#1478
austinderrick wants to merge 2 commits intowintercms:developfrom
austinderrick:fix/form-widget-circular-dependency-loop

Conversation

@austinderrick
Copy link
Copy Markdown

@austinderrick austinderrick commented Apr 7, 2026

Summary

When two or more form fields have circular dependsOn declarations (e.g., field A depends on B and B depends on A), the onRefreshDependants method triggers change events that cascade infinitely, freezing the browser tab.

This fix adds cascade chain tracking through the jQuery event object. Each time a field triggers its dependants, the current field name is appended to a cascadeChain array on the event. Before refreshing, the method checks whether the triggering field already appears in the chain. If it does, a cycle has been detected and the cascade stops.

This preserves legitimate transitive cascading (A → B → C) while preventing circular loops (A → B → A) of any depth.

How it works

The event parameter was already being passed by jQuery as the third argument to the handler (after the two preset arguments from $.proxy), so no changes to the event binding in bindDependants were needed. The only changes are in onRefreshDependants:

  1. Accept the event parameter and extract cascadeChain (defaults to [] for user-initiated changes)
  2. If fieldName is already in cascadeChain, return early (cycle detected)
  3. In the success callback, append fieldName to the chain and pass it through the triggered change events

Tests

Added 4 Jest tests covering:

  • Basic dependency refresh (A → B)
  • Circular dependency detection (A → B → A, stops at 2 requests)
  • Transitive cascading preservation (A → B → C, both refresh correctly)
  • Three-field circular chain detection (A → B → C → A, stops at 3 requests)

All 44 tests pass (7 suites including the new one).

PASS tests/js/cases/framework/FormWidgetDependants.test.js
  Form Widget dependsOn
    ✓ refreshes dependent fields when a field changes (556 ms)
    ✓ prevents infinite loop with circular dependsOn declarations (2015 ms)
    ✓ allows transitive cascading (A -> B -> C) without blocking (1514 ms)
    ✓ stops cycle in a three-field circular chain (A -> B -> C -> A) (2016 ms)

Test Suites: 7 passed, 7 total
Tests:       44 passed, 44 total

Fixes #421

Summary by CodeRabbit

  • Bug Fixes

    • Prevented infinite refresh loops for dependent form fields by tracking cascade chains and detecting circular dependencies.
  • Tests

    • Added comprehensive Jest tests for dependency cascades covering circular prevention, transitive chains, and request behavior.
    • Included test fixtures/stubs to simulate the widget environment so tests run reliably in isolation.

When two or more form fields have circular dependsOn declarations
(e.g., field A depends on B and B depends on A), the onRefreshDependants
method triggers change events that cascade infinitely, freezing the
browser tab.

This fix adds cascade chain tracking through the jQuery event object.
Each time a field triggers its dependants, the current field name is
appended to a cascadeChain array on the event. Before refreshing, the
method checks whether the triggering field already appears in the chain.
If it does, a cycle has been detected and the cascade stops.

This preserves legitimate transitive cascading (A -> B -> C) while
preventing circular loops (A -> B -> A) of any depth.

The event parameter was already being passed by jQuery as the third
argument to the handler (after the two preset arguments from $.proxy),
so no changes to the event binding are needed.

Fixes wintercms#421
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 7, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3f645e2d-37b4-44c5-8958-00da161c8282

📥 Commits

Reviewing files that changed from the base of the PR and between a8d6b93 and c55e92a.

📒 Files selected for processing (1)
  • modules/system/tests/js/fixtures/formWidget/FormWidgetStubs.js
✅ Files skipped from review due to trivial changes (1)
  • modules/system/tests/js/fixtures/formWidget/FormWidgetStubs.js

Walkthrough

onRefreshDependants in modules/backend/widgets/form/assets/js/winter.form.js was changed to accept a third event argument and propagate a cascadeChain array via triggered change events to detect and stop circular refreshes. The method now reads event.cascadeChain, avoids refreshing if the field is already in the chain, and triggers dependant change events with an updated cascadeChain. New Jest tests and a fixture stub file were added to validate cascade, transitive, and circular dependency behaviors.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main fix: preventing infinite loops in the form widget when fields have circular dependsOn declarations.
Linked Issues check ✅ Passed The PR fully addresses issue #421 by implementing circular dependency detection via cascadeChain tracking, preventing infinite loops while preserving normal cascading behavior.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the circular dependency issue: the core fix in winter.form.js, comprehensive Jest tests, and necessary test fixtures.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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 and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@modules/system/tests/js/fixtures/formWidget/FormWidgetStubs.js`:
- Around line 18-26: The jQuery.fn.request stub in FormWidgetStubs.js mismatches
the real contract (production $.fn.request returns a jqXHR with
.done/.fail/.always and is async) and currently uses a synchronous .success()
call; update the stub so it returns a jQuery.Deferred-based object exposing
.done(), .fail(), and .always() (or make .success() an alias to .done()) and
ensure callbacks are invoked asynchronously (e.g., resolve/reject via setTimeout
or Deferred.resolve()/reject() so handlers run async) so tests mirror real
$.ajax/jqXHR behavior and surface race/ordering bugs; target the
jQuery.fn.request implementation in this file when making the change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fe7242e1-cb2d-4a35-a506-b8dfcec2b8a2

📥 Commits

Reviewing files that changed from the base of the PR and between 0758670 and a8d6b93.

📒 Files selected for processing (3)
  • modules/backend/widgets/form/assets/js/winter.form.js
  • modules/system/tests/js/cases/framework/FormWidgetDependants.test.js
  • modules/system/tests/js/fixtures/formWidget/FormWidgetStubs.js

Updated the $.fn.request test stub to use jQuery.Deferred properly:
- .success() is now an alias for .done() (matching the WinterCMS
  framework.js Request class contract)
- Deferred is resolved immediately via deferred.resolve()
- Exposes .done(), .fail(), .always() from the jQuery Deferred API
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dependsOn triggers infinite loop

1 participant