feat: make yii.js clickableSelector and changeableSelector runtime updates rebind delegated data-method/data-confirm handlers.#18
Merged
terabytesoftw merged 2 commits intomainfrom Apr 4, 2026
Conversation
…me updates rebind delegated data-method/data-confirm handlers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🧰 Additional context used🧠 Learnings (9)📓 Common learnings📚 Learning: 2026-03-28T09:30:23.126ZApplied to files:
📚 Learning: 2026-03-27T22:34:49.266ZApplied to files:
📚 Learning: 2026-03-27T22:41:18.886ZApplied to files:
📚 Learning: 2026-03-28T11:18:23.033ZApplied to files:
📚 Learning: 2026-03-28T12:48:02.414ZApplied to files:
📚 Learning: 2026-03-28T10:32:13.765ZApplied to files:
📚 Learning: 2026-03-28T10:38:06.205ZApplied to files:
📚 Learning: 2026-04-02T23:33:24.400ZApplied to files:
🔇 Additional comments (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughMake Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Document
participant Yii as window.yii
participant Confirm
participant Handler as handleAction
User->>Document: click/change on element (matches selector)
Document->>Yii: delegated event (namespaced .yii.dataMethods)
Yii->>Yii: handleDataMethodEvent(event, element)
Yii->>Confirm: yii.confirm(...) [if data-confirm]
Confirm-->>Yii: confirmation result
Yii->>Handler: yii.handleAction(event, element)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
terabytesoftw
changed the title
yii.js clickableSelector and changeableSelector runtime updates rebind delegated data-method/data-confirm handlers.yii.js clickableSelector and changeableSelector runtime updates rebind delegated data-method/data-confirm handlers.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@CHANGELOG.md`:
- Line 20: The changelog entry uses the wrong prefix "fix:" while the PR and its
description classify this as a feature; update the CHANGELOG.md entry so the
line begins with "feat:" instead of "fix:" for the entry about yii.js
clickableSelector and changeableSelector so it matches the PR title and
description (reference symbols: yii.js, clickableSelector, changeableSelector).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: f12faabd-2795-4f46-bf6b-f18a43ef649c
📒 Files selected for processing (3)
CHANGELOG.mdsrc/assets/yii.jstests/js/tests/yii.test.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: linter / Super Linter
- GitHub Check: phpstan / PHP 8.5-ubuntu-latest
- GitHub Check: phpunit / PHP 8.3-windows-2022
- GitHub Check: phpunit / PHP 8.5-windows-2022
- GitHub Check: phpunit / PHP 8.3-ubuntu-latest
- GitHub Check: phpunit / PHP 8.4-windows-2022
- GitHub Check: mutation / PHP 8.5-ubuntu-latest
- GitHub Check: phpunit / PHP 8.2-windows-2022
- GitHub Check: Node 20 on ubuntu-22.04
- GitHub Check: mutation / PHP 8.5-ubuntu-latest
- GitHub Check: Node 20 on ubuntu-22.04
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/assets/yii.captcha.js:42-54
Timestamp: 2026-03-27T22:34:49.266Z
Learning: The JavaScript asset files under `src/assets/` in the `yii2-framework/jquery` repository (e.g., `yii.captcha.js`, `yii.activeForm.js`, `yii.validation.js`, `yii.gridView.js`, `yii.js`) are legacy Yii2 JavaScript ported as-is from `yii2-framework/core`. Do not flag missing error handling, defensive checks, or other improvements in these files — any such changes are tracked in a separate open issue and are out of scope for the current PR review.
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/widgets/PjaxJqueryClientScript.php:39-47
Timestamp: 2026-03-28T09:30:23.126Z
Learning: In `yii2-framework/jquery`, `src/widgets/PjaxJqueryClientScript.php` (register() method): The `$id` value read from `$widget->options['id']` is NOT user-controlled and is always a non-empty string by the time `register()` is called. `Pjax::init()` always sets `options['id']` via `$this->getId()` — an auto-incremented value (e.g. `p0`, `p1`) — before `register()` is ever invoked. Do not flag the `#{$id} a` selector construction as a potential invalid-selector issue.
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/grid/CheckboxColumnJqueryClientScript.php:45-47
Timestamp: 2026-03-27T22:41:18.886Z
Learning: In `yii2-framework/jquery`, `src/grid/CheckboxColumnJqueryClientScript.php` (line 47): The `$id` value interpolated into the `jQuery('#$id').yiiGridView('setSelectionColumn', ...)` `registerJs()` call is NOT user-controlled and does NOT require escaping or JS injection protection. It originates from `$widget->grid->options['id']`, set by `BaseListView::init()` (inherited by `GridView`) via `Widget::getId()` — an auto-incremented value (e.g. `w0`). This is the standard Yii2 pattern used across all widgets for `registerJs()` calls. Do not flag this as a JavaScript injection risk.
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 3
File: .github/workflows/ecs.yml:21-21
Timestamp: 2026-03-28T12:48:02.414Z
Learning: In the yii2-framework/jquery repository, the maintainer (terabytesoftw) intentionally uses mutable version tags (e.g., `v1`) for reusable GitHub Actions workflow references (e.g., `yii2-framework/actions/.github/workflows/ecs.ymlv1`). They prefer the stable/latest working version over pinning to a specific commit SHA. Do not flag mutable tag usage as an issue in this repository.
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/widgets/ActiveFormJqueryClientScript.php:217-226
Timestamp: 2026-03-28T11:18:23.033Z
Learning: In `yii2-framework/jquery`, `src/widgets/ActiveFormJqueryClientScript.php` (`resolveSelectors()` method): The `else` branch `$options['error'] = $field->errorOptions['tag'] ?? 'span'` is intentional legacy behavior ported as-is from `ActiveForm::getClientOptions()` in `yii2-framework/core`. In practice, `ActiveField::$errorOptions` always has `'class' => 'help-block'` by default, so the `elseif (isset($field->errorOptions['class']))` branch is always taken and this fallback is effectively unreachable. Do not flag this `else` branch as a mismatch between the tag-name selector and the rendered HTML.
📚 Learning: 2026-03-27T22:34:46.739Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/assets/yii.captcha.js:42-54
Timestamp: 2026-03-27T22:34:46.739Z
Learning: In this repository, the JavaScript files under `src/assets/` (e.g., `yii.captcha.js`, `yii.activeForm.js`, `yii.validation.js`, `yii.gridView.js`, `yii.js`) are legacy Yii2 JavaScript ported as-is from `yii2-framework/core`. During code review, do not flag these files for missing error handling, defensive checks, or similar quality/robustness improvements based solely on differences from newer coding standards; such changes are handled separately (per the referenced open issue) and are out of scope for the current PR review.
Applied to files:
src/assets/yii.js
📚 Learning: 2026-03-28T09:30:23.126Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/widgets/PjaxJqueryClientScript.php:39-47
Timestamp: 2026-03-28T09:30:23.126Z
Learning: In `yii2-framework/jquery`, `src/widgets/PjaxJqueryClientScript.php` (register() method): The `$id` value read from `$widget->options['id']` is NOT user-controlled and is always a non-empty string by the time `register()` is called. `Pjax::init()` always sets `options['id']` via `$this->getId()` — an auto-incremented value (e.g. `p0`, `p1`) — before `register()` is ever invoked. Do not flag the `#{$id} a` selector construction as a potential invalid-selector issue.
Applied to files:
src/assets/yii.jsCHANGELOG.md
📚 Learning: 2026-03-27T22:41:18.886Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/grid/CheckboxColumnJqueryClientScript.php:45-47
Timestamp: 2026-03-27T22:41:18.886Z
Learning: In `yii2-framework/jquery`, `src/grid/CheckboxColumnJqueryClientScript.php` (line 47): The `$id` value interpolated into the `jQuery('#$id').yiiGridView('setSelectionColumn', ...)` `registerJs()` call is NOT user-controlled and does NOT require escaping or JS injection protection. It originates from `$widget->grid->options['id']`, set by `BaseListView::init()` (inherited by `GridView`) via `Widget::getId()` — an auto-incremented value (e.g. `w0`). This is the standard Yii2 pattern used across all widgets for `registerJs()` calls. Do not flag this as a JavaScript injection risk.
Applied to files:
src/assets/yii.jsCHANGELOG.md
📚 Learning: 2026-03-27T22:33:13.516Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/assets/yii.captcha.js:6-6
Timestamp: 2026-03-27T22:33:13.516Z
Learning: In this repository’s JS assets (e.g., under src/assets/), the maintainer considers multiple IIFE/wrapping styles acceptable. ESLint’s `wrap-iife` rule is configured as `"warn"` in `eslint.config.js`, so do not require a change to satisfy `wrap-iife` (e.g., avoid flagging or enforcing a specific `(function($){...})(arg)` vs alternate IIFE formatting). Treat both IIFE styles as valid for src/assets JavaScript files.
Applied to files:
src/assets/yii.js
📚 Learning: 2026-03-27T22:34:49.266Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/assets/yii.captcha.js:42-54
Timestamp: 2026-03-27T22:34:49.266Z
Learning: The JavaScript asset files under `src/assets/` in the `yii2-framework/jquery` repository (e.g., `yii.captcha.js`, `yii.activeForm.js`, `yii.validation.js`, `yii.gridView.js`, `yii.js`) are legacy Yii2 JavaScript ported as-is from `yii2-framework/core`. Do not flag missing error handling, defensive checks, or other improvements in these files — any such changes are tracked in a separate open issue and are out of scope for the current PR review.
Applied to files:
CHANGELOG.mdtests/js/tests/yii.test.js
📚 Learning: 2026-03-28T11:18:23.033Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/widgets/ActiveFormJqueryClientScript.php:217-226
Timestamp: 2026-03-28T11:18:23.033Z
Learning: In `yii2-framework/jquery`, `src/widgets/ActiveFormJqueryClientScript.php` (`resolveSelectors()` method): The `else` branch `$options['error'] = $field->errorOptions['tag'] ?? 'span'` is intentional legacy behavior ported as-is from `ActiveForm::getClientOptions()` in `yii2-framework/core`. In practice, `ActiveField::$errorOptions` always has `'class' => 'help-block'` by default, so the `elseif (isset($field->errorOptions['class']))` branch is always taken and this fallback is effectively unreachable. Do not flag this `else` branch as a mismatch between the tag-name selector and the rendered HTML.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-03-28T10:38:06.205Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/grid/GridViewJqueryClientScript.php:77-79
Timestamp: 2026-03-28T10:38:06.205Z
Learning: In `yii2-framework/jquery`, `src/grid/GridViewJqueryClientScript.php` (register() method): The `$id` value interpolated into the `jQuery('#$id').yiiGridView(...)` `registerJs()` call is NOT user-controlled and does NOT require escaping or JS injection protection. It originates from `$widget->options['id']`, set by `BaseListView::init()` (inherited by `GridView`) via `Widget::getId()` — an auto-incremented value (e.g. `w0`). This is the standard Yii2 pattern used across all widgets for `registerJs()` calls. Do not flag this as a JavaScript injection risk.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-04-02T23:33:24.400Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 7
File: README.md:56-56
Timestamp: 2026-04-02T23:33:24.400Z
Learning: In yii2-framework/jquery, `composer.json` has `"minimum-stability": "dev"`, `"prefer-stable": true`, and a branch-alias `"dev-main": "0.1.x-dev"` under `extra`. This means `composer require yii2-framework/jquery:^0.1` resolves correctly for users who already have `minimum-stability: dev` in their project (typical for Yii2 apps). Do not flag the README installation command `composer require yii2-framework/jquery:^0.1` (without `dev`) as incorrect — the maintainer intentionally relies on this alias and targets Yii2 projects that allow dev stability.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-03-27T22:40:13.617Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: src/grid/CheckboxColumnJqueryClientScript.php:34-34
Timestamp: 2026-03-27T22:40:13.617Z
Learning: In `yii2-framework/jquery`, `src/grid/CheckboxColumnJqueryClientScript.php` (line 34): The direct access `$widget->grid->options['id']` is safe without a null-coalescing guard because `BaseListView::init()` (inherited by `GridView`) always sets `options['id']` — defaulting to `$this->getId()` — before `register()` is ever called. Do not flag this as a potential undefined-index issue.
Applied to files:
CHANGELOG.md
📚 Learning: 2026-03-28T10:54:04.416Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: tests/widgets/PjaxTest.php:82-84
Timestamp: 2026-03-28T10:54:04.416Z
Learning: In `yii2-framework/jquery`, `tests/widgets/PjaxTest.php`: The `finally` blocks in `testInitWithPjaxRequestPathAndTitle()` and `testRequiresPjaxReturnsTrueWithMatchingHeaders()` intentionally use only `while (ob_get_level() < $obLevel) { ob_start(); }` (one-directional OB restoration). Yii2 clears the output buffer internally during Pjax request handling, so only recreating missing buffers — never closing extra ones — is correct here. This prevents PHPUnit from emitting "risky test" warnings due to OB level changes. Do not flag this as incomplete OB restoration.
Applied to files:
tests/js/tests/yii.test.js
📚 Learning: 2026-03-28T09:20:49.698Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: tests/js/tests/yii.test.js:197-199
Timestamp: 2026-03-28T09:20:49.698Z
Learning: In this repo’s `tests/js/` JavaScript tests, sinon is pinned to v1.17.7. When reviewing or adjusting stubs/spies, use the sinon v1 three-argument stub form `sinon.stub(obj, method, fn)` (and related v1 APIs). Do not recommend switching to newer sinon v2+ APIs such as `.callsFake()`, since they are not available in this sinon version; treating a sinon upgrade is out of scope for PR review changes.
Applied to files:
tests/js/tests/yii.test.js
📚 Learning: 2026-03-28T09:11:11.205Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: tests/validators/NumberValidatorJqueryClientScriptTest.php:41-45
Timestamp: 2026-03-28T09:11:11.205Z
Learning: In `yii2-framework/jquery` tests, array element values from `getClientOptions()` are typed as `mixed` by PHPStan. Use `instanceof` checks (e.g., `$value instanceof JsExpression ? (string) $value : ''`) rather than `assertInstanceOf` + cast to narrow array element types in a PHPStan-safe way, since PHPStan does not narrow array elements after `assertInstanceOf` calls.
Applied to files:
tests/js/tests/yii.test.js
📚 Learning: 2026-03-28T09:24:00.750Z
Learnt from: terabytesoftw
Repo: yii2-framework/jquery PR: 1
File: tests/js/tests/yii.gridView.test.js:263-267
Timestamp: 2026-03-28T09:24:00.750Z
Learning: In this repo’s `tests/js/` JavaScript tests, sinon is pinned to v1.17.7. Use the sinon v1 reset API `.reset()` for spies/stubs. Do not suggest replacing `.reset()` with `.resetHistory()`, since `resetHistory()` was introduced in sinon 3.x and is not available in sinon v1. Reviewers should therefore not flag `.reset()` calls as deprecated in this codebase.
Applied to files:
tests/js/tests/yii.test.js
🔇 Additional comments (5)
src/assets/yii.js (2)
38-43: Well-structured implementation for runtime selector rebinding.The approach of using
Object.definePropertieswith getter/setter accessors, combined with:
- A namespaced event identifier (
.yii.dataMethods) for clean unbind/rebind- An initialization guard (
isDataMethodsInitialized) to prevent premature rebinding- Short-circuit optimization when the selector value is unchanged
This is a clean and idiomatic solution for making the selectors runtime-configurable.
Also applies to: 59-59, 63-63, 241-284
672-699: LGTM: Clean event handler extraction and rebinding logic.The separation of
bindDataMethodsHandlers()andhandleDataMethodEvent()improves code organization. The chained.off()calls ensure previous delegated handlers are removed before rebinding with potentially updated selectors.tests/js/tests/yii.test.js (3)
1826-1832: Good test isolation setup.Snapshotting the default selectors in
before()and restoring them inafterEach()ensures test isolation. The cleanup of dynamically added elements (.custom-clickable-element,.custom-changeable-wrapper) prevents DOM pollution across tests.Also applies to: 1849-1852
1937-1968: Comprehensive test for clickableSelector rebinding.The test correctly verifies:
- After updating
clickableSelector, the original element no longer triggersyii.confirm/yii.handleAction- The extra delegated handler (using a different selector) still receives the event
- A newly appended element matching the updated selector correctly triggers the rebind handlers
2002-2038: Solid test coverage for changeableSelector rebinding.The test mirrors the clickableSelector pattern and correctly:
- Verifies the old element stops triggering yii handlers after selector update
- Confirms that new elements matching the updated selector work correctly
- Appropriately expects
yiiConfirmSpyto remain uncalled since the test element lacksdata-confirm
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request