Conversation
WalkthroughBumps package version to 0.2.0-beta; constructor normalization in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Allonsh as Allonsh (constructor)
participant DOM as DOM Elements
App->>Allonsh: new Allonsh(options)
Allonsh->>DOM: query draggable elements
alt Draggables found
loop For each element
Allonsh->>DOM: add DRAGGABLE class if missing
end
else None found
Allonsh->>App: log/warn no draggables
end
note right of Allonsh: update() tests ensure listeners not duplicated
note right of DOM: addDraggable() appends element and increments collection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedReview triggered.
|
✅ Actions performedFull review triggered. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/allonsh.js (3)
183-190: Event listeners bound with ephemeral functions; unbind will not work → duplicates risk.use the cached bound handlers created in the constructor so removeEventListener matches.
Apply:
- element.addEventListener('mousedown', this._onMouseDown.bind(this)); - element.addEventListener('touchstart', this._onTouchStart.bind(this), { + element.addEventListener('mousedown', this._boundMouseDown); + element.addEventListener('touchstart', this._boundTouchStart, { passive: false, });
35-40: Persist the selector to support safe updates.Store the initial selector for later reuse in update().
Apply:
this.useGhostEffect = useGhostEffect; + this.draggableSelector = draggableSelector; + this.dropzoneSelector = dropzoneSelector;
123-147: update(): Selector misuse and unbind-in-loop cause listener loss/duplication.
- Uses newOptions.draggableSelector even when undefined → clears draggables.
- Calls _unbindEventListeners() inside the forEach → repeated unbind calls and ordering issues.
- Always re-binds on any option change → unnecessary churn and duplicate handlers if initial bind used ephemeral functions.
Apply:
- this.draggableElements = this.playAreaElement.querySelectorAll( - `.${draggableSelector}` - ); - - this.draggableElements.forEach((element) => { - this._unbindEventListeners(); - element.style.cursor = CSS_CURSORS.GRAB; - element.addEventListener('mousedown', this._boundMouseDown); - element.addEventListener('touchstart', this._boundTouchStart, { - passive: false, - }); - }); + // Rebind only if selector actually changes or play area changes + const selectorChanged = + draggableSelector !== undefined && + draggableSelector !== this.draggableSelector; + if (selectorChanged) { + this.draggableSelector = draggableSelector; + } + if (selectorChanged || !!playAreaSelector) { + // Unbind from current set before re-querying + this._unbindEventListeners(); + this.draggableElements = this.playAreaElement.querySelectorAll( + `.${this.draggableSelector || CSS_CLASSES.DRAGGABLE}` + ); + this.draggableElements.forEach((element) => { + element.style.cursor = CSS_CURSORS.GRAB; + element.addEventListener('mousedown', this._boundMouseDown); + element.addEventListener('touchstart', this._boundTouchStart, { + passive: false, + }); + }); + }
🧹 Nitpick comments (1)
src/allonsh.js (1)
212-219: Optionally make unbind target explicit to avoid order hazards.Safer when refactoring update() and re-queries.
Apply:
- _unbindEventListeners() { - if (!this.draggableElements) return; - - this.draggableElements.forEach((element) => { + _unbindEventListeners(elements = this.draggableElements) { + if (!elements) return; + elements.forEach((element) => { element.removeEventListener('mousedown', this._boundMouseDown); element.removeEventListener('touchstart', this._boundTouchStart); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
package.json(1 hunks)src/allonsh.js(1 hunks)test/allonsh.test.js(1 hunks)
🔇 Additional comments (3)
package.json (1)
3-3: Version bump looks good.No concerns with the pre-release tag.
src/allonsh.js (1)
59-64: Good normalization of initial draggables.Ensures consistent class application across pre-existing elements.
test/allonsh.test.js (1)
189-202: LGTM: addDraggable preserves existing and increments by one.Covers the regression from #13.
| it('should not duplicate event listeners on update', () => { | ||
| const el = allonsh.draggableElements[0]; | ||
| const addSpy = vi.spyOn(el, 'addEventListener'); | ||
|
|
||
| allonsh.update({ restrictToDropzones: true }); | ||
| allonsh.update({ enableStacking: false }); | ||
| allonsh.update({ useGhostEffect: false }); | ||
|
|
||
| const mousedownAdds = addSpy.mock.calls.filter( | ||
| ([event]) => event === 'mousedown' | ||
| ); | ||
| const touchstartAdds = addSpy.mock.calls.filter( | ||
| ([event]) => event === 'touchstart' | ||
| ); | ||
|
|
||
| expect(mousedownAdds.length).toBeLessThanOrEqual(1); | ||
| expect(touchstartAdds.length).toBeLessThanOrEqual(1); | ||
|
|
||
| addSpy.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
Test is a false negative; update() currently queries with undefined selector → no adds observed. Tighten the assertion and ensure the selector path is exercised.
Pass the selector (same value) and require zero new adds to prove no duplication.
Apply:
- it('should not duplicate event listeners on update', () => {
+ it('should not duplicate event listeners on update', () => {
const el = allonsh.draggableElements[0];
const addSpy = vi.spyOn(el, 'addEventListener');
- allonsh.update({ restrictToDropzones: true });
- allonsh.update({ enableStacking: false });
- allonsh.update({ useGhostEffect: false });
+ // Provide the same selector to exercise the re-selection path without rebinding
+ allonsh.update({ restrictToDropzones: true, draggableSelector: 'draggable' });
+ allonsh.update({ enableStacking: false, draggableSelector: 'draggable' });
+ allonsh.update({ useGhostEffect: false, draggableSelector: 'draggable' });
const mousedownAdds = addSpy.mock.calls.filter(
([event]) => event === 'mousedown'
);
const touchstartAdds = addSpy.mock.calls.filter(
([event]) => event === 'touchstart'
);
- expect(mousedownAdds.length).toBeLessThanOrEqual(1);
- expect(touchstartAdds.length).toBeLessThanOrEqual(1);
+ expect(mousedownAdds.length).toBe(0);
+ expect(touchstartAdds.length).toBe(0);
addSpy.mockRestore();
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('should not duplicate event listeners on update', () => { | |
| const el = allonsh.draggableElements[0]; | |
| const addSpy = vi.spyOn(el, 'addEventListener'); | |
| allonsh.update({ restrictToDropzones: true }); | |
| allonsh.update({ enableStacking: false }); | |
| allonsh.update({ useGhostEffect: false }); | |
| const mousedownAdds = addSpy.mock.calls.filter( | |
| ([event]) => event === 'mousedown' | |
| ); | |
| const touchstartAdds = addSpy.mock.calls.filter( | |
| ([event]) => event === 'touchstart' | |
| ); | |
| expect(mousedownAdds.length).toBeLessThanOrEqual(1); | |
| expect(touchstartAdds.length).toBeLessThanOrEqual(1); | |
| addSpy.mockRestore(); | |
| }); | |
| it('should not duplicate event listeners on update', () => { | |
| const el = allonsh.draggableElements[0]; | |
| const addSpy = vi.spyOn(el, 'addEventListener'); | |
| // Provide the same selector to exercise the re-selection path without rebinding | |
| allonsh.update({ restrictToDropzones: true, draggableSelector: 'draggable' }); | |
| allonsh.update({ enableStacking: false, draggableSelector: 'draggable' }); | |
| allonsh.update({ useGhostEffect: false, draggableSelector: 'draggable' }); | |
| const mousedownAdds = addSpy.mock.calls.filter( | |
| ([event]) => event === 'mousedown' | |
| ); | |
| const touchstartAdds = addSpy.mock.calls.filter( | |
| ([event]) => event === 'touchstart' | |
| ); | |
| expect(mousedownAdds.length).toBe(0); | |
| expect(touchstartAdds.length).toBe(0); | |
| addSpy.mockRestore(); | |
| }); |
🤖 Prompt for AI Agents
test/allonsh.test.js around lines 168 to 187: the test currently observes no
listener additions because update() is being called without the selector so the
internal query path is not exercised; modify the three allonsh.update calls to
pass the same selector used to initialize draggableElements (so the query runs),
then tighten the assertions to require zero new 'mousedown' and 'touchstart'
addEventListener calls (expect(...length).toBe(0)); keep the spy setup and
mockRestore as is.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/allonsh.js (2)
183-190: Bug: listeners are bound with new function instances; unbind will not remove them._in initialize() you call
.bind(this)inline, but_unbindEventListeners()removesthis._boundMouseDown/_boundTouchStart. These are different function references, so initial listeners remain attached and later updates can stack duplicates.Apply:
- element.addEventListener('mousedown', this._onMouseDown.bind(this)); - element.addEventListener('touchstart', this._onTouchStart.bind(this), { + element.addEventListener('mousedown', this._boundMouseDown); + element.addEventListener('touchstart', this._boundTouchStart, { passive: false, });
123-147: update(): selector handling and rebind logic cause false negatives and duplicate attempts.Issues:
- Using
draggableSelectordirectly yields'.undefined'when not provided, wipingdraggableElements._unbindEventListeners()is called inside the per-element loop and after reselecting, so the original listeners are never removed, and adds are attempted on each update.- Rebinding occurs even when nothing relevant changed.
Fix: store the initial selector, unbind before reselecting, and only rebind when selector or play area changes.
Apply:
@@ update(newOptions = {}) { const { draggableSelector, dropzoneSelector, playAreaSelector, @@ } = newOptions; + // Track whether event (re)binding is necessary + let needsRebind = false; + if (restrictToDropzones !== undefined) this.restrictToDropzones = restrictToDropzones; if (enableStacking !== undefined) this.enableStacking = enableStacking; if (stackDirection !== undefined) this.stackDirection = stackDirection; if (stackSpacing !== undefined) this.stackSpacing = stackSpacing; if (useGhostEffect !== undefined) this.useGhostEffect = useGhostEffect; if (playAreaSelector) { const newPlayArea = document.querySelector(`.${playAreaSelector}`); if (newPlayArea) { - this.playAreaElement = newPlayArea; + if (newPlayArea !== this.playAreaElement) { + this.playAreaElement = newPlayArea; + needsRebind = true; + } this.playAreaElement.style.position = CSS_POSITIONS.RELATIVE; } else { console.warn( `Allonsh Warning: Play area element with class '${playAreaSelector}' not found.` ); } } - this.draggableElements = this.playAreaElement.querySelectorAll( - `.${draggableSelector}` - ); + const nextDraggableSelector = + draggableSelector ?? this.draggableSelector; + if (nextDraggableSelector && nextDraggableSelector !== this.draggableSelector) { + this.draggableSelector = nextDraggableSelector; + needsRebind = true; + } + + if (needsRebind) { + // Unbind from current set before reselecting + this._unbindEventListeners(); + this.draggableElements = this.playAreaElement.querySelectorAll( + `.${this.draggableSelector}` + ); + } - this.draggableElements.forEach((element) => { - this._unbindEventListeners(); - element.style.cursor = CSS_CURSORS.GRAB; - element.addEventListener('mousedown', this._boundMouseDown); - element.addEventListener('touchstart', this._boundTouchStart, { - passive: false, - }); - }); + if (needsRebind) { + this.draggableElements.forEach((element) => { + element.style.cursor = CSS_CURSORS.GRAB; + element.addEventListener('mousedown', this._boundMouseDown); + element.addEventListener('touchstart', this._boundTouchStart, { + passive: false, + }); + }); + }And initialize the stored selectors in the constructor (outside this hunk):
// after options destructure in constructor this.draggableSelector = draggableSelector; this.dropzoneSelector = dropzoneSelector;
♻️ Duplicate comments (1)
test/allonsh.test.js (1)
168-187: Test currently passes as a false negative; exercise reselection and assert zero adds.Pass the selector so update() walks the query path, then require zero new addEventListener calls. This matches the intent “no duplication” when options change.
Apply:
it('should not duplicate event listeners on update', () => { const el = allonsh.draggableElements[0]; const addSpy = vi.spyOn(el, 'addEventListener'); - allonsh.update({ restrictToDropzones: true }); - allonsh.update({ enableStacking: false }); - allonsh.update({ useGhostEffect: false }); + allonsh.update({ restrictToDropzones: true, draggableSelector: 'draggable' }); + allonsh.update({ enableStacking: false, draggableSelector: 'draggable' }); + allonsh.update({ useGhostEffect: false, draggableSelector: 'draggable' }); const mousedownAdds = addSpy.mock.calls.filter( ([event]) => event === 'mousedown' ); const touchstartAdds = addSpy.mock.calls.filter( ([event]) => event === 'touchstart' ); - expect(mousedownAdds.length).toBeLessThanOrEqual(1); - expect(touchstartAdds.length).toBeLessThanOrEqual(1); + expect(mousedownAdds.length).toBe(0); + expect(touchstartAdds.length).toBe(0); addSpy.mockRestore(); });Note: With the src fixes, update() won’t rebind when only options change, so zero adds is the correct expectation.
🧹 Nitpick comments (2)
src/allonsh.js (1)
212-219: Scope _unbindEventListeners to a provided set to avoid relying on mutable state.Make
_unbindEventListeners(elements = this.draggableElements)and iterate that list. This prevents races with reassignments and lets update() cleanly unbind the “old” snapshot.Proposed change:
- _unbindEventListeners() { - if (!this.draggableElements) return; - this.draggableElements.forEach((element) => { + _unbindEventListeners(elements = this.draggableElements) { + if (!elements) return; + elements.forEach((element) => { element.removeEventListener('mousedown', this._boundMouseDown); element.removeEventListener('touchstart', this._boundTouchStart); }); }test/allonsh.test.js (1)
189-202: Extend assertions to verify class normalization and no loss of prior draggables.Also assert the new element has the library class and that original draggables remain in the collection.
Add:
it('should preserve existing draggables and increment count by 1 when addDraggable() is called', () => { const initialCount = allonsh.draggableElements.length; + const before = [...allonsh.draggableElements]; const newEl = document.createElement('div'); newEl.className = 'draggable new'; document.querySelector('.play-area').appendChild(newEl); allonsh.addDraggable(newEl); const finalCount = allonsh.draggableElements.length; expect(finalCount).toBe(initialCount + 1); expect([...allonsh.draggableElements]).toContain(newEl); + // original draggables still present + before.forEach((el) => + expect([...allonsh.draggableElements]).toContain(el) + ); + // library class applied + expect(newEl.classList.contains('allonsh-draggable')).toBe(true); });Adjust the literal 'allonsh-draggable' if CSS_CLASSES.DRAGGABLE differs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
package.json(1 hunks)src/allonsh.js(1 hunks)test/allonsh.test.js(1 hunks)
🔇 Additional comments (2)
package.json (1)
3-3: Pre-release bump to 0.2.0-beta validated. Confirmed no existing published version (npm view allonsh404s) andnpm pack --dry-runproduces the 0.2.0-beta tarball. Ensure CHANGELOG and Git tag reflect 0.2.0-beta before publishing.src/allonsh.js (1)
59-64: Good normalization: ensure base draggables carry the library class.Adding CSS_CLASSES.DRAGGABLE up front aligns existing markup with runtime expectations and keeps addDraggable() semantics consistent.
1 participant
Description
Related Issue
Type of Change
How Has This Been Tested?
Using local server and
npm run testChecklist
Additional Notes
N/A
Summary by CodeRabbit