Repository navigation
Bind the preset link once per element - #12
Merged
Merged
Conversation
Fivell
force-pushed
the
fix/init-once
branch
2 times, most recently
from
October 1, 2026 09:27
951697a to
fd2bab2
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The guard directly addresses duplicate initialization and is covered by focused regression tests.
Review effort: Balanced
Findings: None
What changed in this PR
Makes preset initialization idempotent, preventing duplicate links and handlers.
Changes:
- Tracks initialized elements and skips repeated binding.
- Adds feature specs covering duplicate initialization and input behavior.
| File | Description |
|---|---|
vendor/assets/javascripts/active_admin_date_range_preset.js |
Adds the initialization guard. |
spec/init_once_spec.rb |
Tests repeated initialization behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Fivell
force-pushed
the
fix/init-once
branch
2 times, most recently
from
October 1, 2026 10:34
a5545f5 to
bd7a412
Compare
Nothing marks an element as already wired, so a second call appends a second link and a second delegated handler. A click then runs both, and the one that fires last tears down the popup the first just opened. This was unreachable while the auto-init was dead. Hosts worked around that by calling the plugin themselves in active_admin.js, so once the auto-init works again both run; the specs stand in for that call. Two of the four examples fail here: the link count and the handler count, both reporting 2 where 1 is expected. The other two pin the semantics of the fix rather than the bug -- that re-initialising still fills the inputs, and that options passed to the second call are honoured rather than dropped. Both of those pass on this commit, because with two handlers bound the second one simply overwrites the first one's output. This commit is expected to fail CI.
Remove the existing link and unbind the namespaced handler before wiring, so a repeated call replaces the element's wiring instead of adding to it. Replacing rather than skipping is deliberate. Sprockets puts the plugin's own ready callback ahead of anything a host appends to active_admin.js, so a guard that kept the first wiring and bailed out of the second would silently discard the host's options -- worse than the duplicate link it would be fixing, because the duplicate is at least visible. The guard reads the link it guards rather than a jQuery data flag. Data lives in an expando, not the DOM, so clone() and Turbo cache restores copy the link without it: the flag and the thing it marks would drift apart in both directions. The click handler is namespaced to make the unbind targeted; it was the only unnamespaced handler in the file.
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
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.
Found while writing the first test suite (#9). Branched off that PR, because the specs need its harness.
The bug
Nothing marked an element as already wired. A second call appended a second "Set range" link and a second delegated handler, and a click ran both — the one that fired last called
unbindClickEventBlockTimerange()and tore down the popup the first had just opened.This was unreachable while the auto-init was dead, and is the normal case now that #9 revives it. The auto-init has been broken since jQuery 3 shipped, so the natural workaround — adding
$('.datetime_preset_pair').date_range_ext_preset()toactive_admin.js— is what host apps are carrying. After the upgrade that call and the auto-init both run.The fix
Mark the element on first init, return early after.
The specs
Three examples, each initialising a second time by hand first: exactly one link (
count: 1— note the existing spec in #9 would pass with two), exactly one popup per click, and the inputs still filled. All three fail on the parent commit.