Refactor Allonsh into Modular Architecture#31
Merged
satnam72 merged 6 commits intobeta.0.2.0from Aug 29, 2025
Merged
Conversation
WalkthroughReplaces the monolithic drag-and-drop implementation with a modular architecture (Allonsh orchestrator plus DragManager, DropzoneManager, EventManager, StyleManager), removes the legacy Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant DOM
participant EM as EventManager
participant DM as DragManager
participant DZ as DropzoneManager
participant SM as StyleManager
Note over EM,SM: Initialization
EM->>SM: applyPlayAreaStyles()
DZ->>DOM: initializeDropzones()
EM->>DOM: bind events on draggable elements
Note over User,DM: Drag start
User->>DOM: mousedown / touchstart
DOM->>EM: start event
EM->>DM: startDrag(event, x, y)
DM->>SM: applyDraggingStyles()
DM->>DZ: toggleHighlight(true)
alt ghost enabled
DM->>DOM: create ghost element
DM->>SM: applyGhostStyles(ghost)
end
Note over User,DM: Drag move
User->>DOM: mousemove / touchmove
DOM->>EM: move event
EM->>DM: updateDragPosition(x, y)
Note over User,DM: Drop
User->>DOM: mouseup / touchend
DOM->>EM: end event
EM->>DM: handleDrop(x, y, event)
alt over dropzone
DM->>DZ: handleDropzoneDrop(element, dropzone, event)
DZ->>SM: resetPosition(element)
else not over dropzone
DM->>DM: place in play area or return to origin
end
DM->>DZ: toggleHighlight(false)
DM->>SM: resetDraggableStyles()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml 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:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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
Status, Documentation and Community
|
Owner
Author
|
@coderabbitai full review |
✅ 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: 12
🧹 Nitpick comments (4)
src/StyleManager.js (1)
53-57: Restoring to 'relative' may shift layout; consider restoring original positioning.If an element was 'static' or 'absolute' pre-drag, forcing 'relative' can cause layout changes. Capture and restore the original computed position instead.
src/DropzoneManager.js (1)
34-44: Re-applying stacking styles without cleaning previous dropzones can leave stale styles.If setDropzones is called repeatedly, old dropzones retain stacking styles.
Consider removing stacking styles from previous dropzones before reassigning:
- Track previous this.dropzoneElements.
- Call removeStackingStyles on each when enableStacking is true.
src/DragManager.js (1)
65-88: Throttle DOM writes for smoother draggingConsider wrapping position updates in requestAnimationFrame to avoid layout thrash on fast pointermove/touchmove streams.
Example:
- updateDragPosition(clientX, clientY) { + updateDragPosition(clientX, clientY) { + if (this._rafId) cancelAnimationFrame(this._rafId); + this._pending = { clientX, clientY }; + this._rafId = requestAnimationFrame(() => this._updateDragPositionNow()); + } + + _updateDragPositionNow() { + const { clientX, clientY } = this._pending || {}; + if (clientX == null || !this.currentDraggedElement) return; // ... existing body ... }src/Allonsh.js (1)
31-39: Defaulting play area to document.body can create global side effectsApplying play-area styles to body (positioning/overflow) can affect the whole page. Consider requiring an explicit play-area selector or warning loudly when defaulting.
If you keep the default, add a guard and visible warning:
- this.playAreaElement = playAreaSelector + this.playAreaElement = playAreaSelector ? document.querySelector(this._normalizeSelector(playAreaSelector)) : document.body; + if (!playAreaSelector) { + console.warn('Allonsh Warning: Defaulting play area to <body>; this may affect page layout.'); + }
📜 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 (9)
.coderabbit.yaml(1 hunks).github/workflows/pull-request-comment.yml(0 hunks)src/Allonsh.js(1 hunks)src/DragManager.js(1 hunks)src/DropzoneManager.js(1 hunks)src/EventManager.js(1 hunks)src/StyleManager.js(1 hunks)src/allonsh.js(0 hunks)src/constants.js(1 hunks)
💤 Files with no reviewable changes (2)
- src/allonsh.js
- .github/workflows/pull-request-comment.yml
Comment on lines
+31
to
+47
| this.playAreaElement = playAreaSelector | ||
| ? document.querySelector(`.${playAreaSelector}`) | ||
| : document.body; | ||
| if (!this.playAreaElement) { | ||
| throw new Error( | ||
| `Allonsh Error: Play area element with class '${playAreaSelector}' not found.` | ||
| ); | ||
| } | ||
|
|
||
| this.draggableElements = this.playAreaElement.querySelectorAll( | ||
| `.${draggableSelector}` | ||
| ); | ||
| if (!this.draggableElements.length) { | ||
| console.warn( | ||
| `Allonsh Warning: No draggable elements found with selector '.${draggableSelector}'.` | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Accept real CSS selectors instead of forcing class names
The API calls these “selectors” but code prepends a dot, breaking valid selectors (e.g., '#id', '[data-x]'). Normalize: if the string starts with '.', '#', or '[' use as-is; otherwise treat as a class.
Apply this diff:
export default class Allonsh {
+ _normalizeSelector(sel) {
+ if (!sel) return sel;
+ const s = sel.trim();
+ return /^[.#\[]/.test(s) ? s : `.${s}`;
+ }
constructor(options = {}) {
@@
- this.playAreaElement = playAreaSelector
- ? document.querySelector(`.${playAreaSelector}`)
+ this.playAreaElement = playAreaSelector
+ ? document.querySelector(this._normalizeSelector(playAreaSelector))
: document.body;
@@
- this.draggableElements = this.playAreaElement.querySelectorAll(
- `.${draggableSelector}`
- );
+ this.draggableElements = this.playAreaElement.querySelectorAll(
+ this._normalizeSelector(draggableSelector)
+ );
@@
- const newPlayArea = document.querySelector(`.${playAreaSelector}`);
+ const newPlayArea = document.querySelector(this._normalizeSelector(playAreaSelector));
@@
- this.draggableElements = this.playAreaElement.querySelectorAll(
- `.${draggableSelector}`
- );
+ this.draggableElements = this.playAreaElement.querySelectorAll(
+ this._normalizeSelector(draggableSelector)
+ );
@@
addDraggable(element) {
this.eventManager.bindEvents([element]);
- this.draggableElements = this.playAreaElement.querySelectorAll(
- `.${CSS_CLASSES.DRAGGABLE}`
- );
+ this.draggableElements = this.playAreaElement.querySelectorAll(
+ this._normalizeSelector(this.options.draggableSelector)
+ );
}
@@
removeDraggable(element) {
this.eventManager.unbindEvents([element]);
element.classList.remove(CSS_CLASSES.DRAGGABLE);
- this.draggableElements = this.playAreaElement.querySelectorAll(
- `.${CSS_CLASSES.DRAGGABLE}`
- );
+ this.draggableElements = this.playAreaElement.querySelectorAll(
+ this._normalizeSelector(this.options.draggableSelector)
+ );
}Also applies to: 104-116, 118-124, 152-165
🤖 Prompt for AI Agents
In src/Allonsh.js around lines 31-47 (and also update the same pattern at
104-116, 118-124, and 152-165), the constructor and other methods currently
force a class selector by prepending '.' which breaks valid CSS selectors like
'#id' or '[data-x]'; change the logic to normalize incoming selector strings: if
the selector is falsy leave it (use document.body where intended), else if it
starts with '.' or '#' or '[' use it as-is, otherwise prepend '.' to treat it as
a class; then use this normalized selector for document.querySelector /
querySelectorAll and for all error/warning messages so they report the exact
selector string used. Ensure the normalization is applied consistently at the
listed line ranges and that checks for "no elements found" use the normalized
selector rather than assuming a class name.
Comment on lines
+73
to
+78
| _initialize() { | ||
| this.styleManager.applyPlayAreaStyles(this.playAreaElement); | ||
| if (!this.draggableElements.length) return; | ||
| this.eventManager.bindEvents(this.draggableElements); | ||
| this.dropzoneManager.initializeDropzones(this.options.dropzoneSelector); | ||
| } |
There was a problem hiding this comment.
Dropzones not initialized when there are zero draggables
Early return skips dropzone initialization. Initialize dropzones unconditionally; only guard event binding.
Apply this diff:
_initialize() {
this.styleManager.applyPlayAreaStyles(this.playAreaElement);
- if (!this.draggableElements.length) return;
- this.eventManager.bindEvents(this.draggableElements);
- this.dropzoneManager.initializeDropzones(this.options.dropzoneSelector);
+ if (this.draggableElements.length) {
+ this.eventManager.bindEvents(this.draggableElements);
+ }
+ this.dropzoneManager.initializeDropzones(this.options.dropzoneSelector);
}📝 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.
Suggested change
| _initialize() { | |
| this.styleManager.applyPlayAreaStyles(this.playAreaElement); | |
| if (!this.draggableElements.length) return; | |
| this.eventManager.bindEvents(this.draggableElements); | |
| this.dropzoneManager.initializeDropzones(this.options.dropzoneSelector); | |
| } | |
| _initialize() { | |
| this.styleManager.applyPlayAreaStyles(this.playAreaElement); | |
| if (this.draggableElements.length) { | |
| this.eventManager.bindEvents(this.draggableElements); | |
| } | |
| this.dropzoneManager.initializeDropzones(this.options.dropzoneSelector); | |
| } |
🤖 Prompt for AI Agents
In src/Allonsh.js around lines 73 to 78, the early return prevents dropzones
from being initialized when there are zero draggable elements; change the logic
so dropzoneManager.initializeDropzones(this.options.dropzoneSelector) is always
called and only eventManager.bindEvents(this.draggableElements) is conditional
on this.draggableElements.length. Move or remove the early return and wrap only
the event binding in an if (this.draggableElements.length) guard so dropzones
initialize unconditionally while event binding remains protected.
Comment on lines
+92
to
+129
| this.options = { | ||
| ...this.options, | ||
| draggableSelector, | ||
| dropzoneSelector, | ||
| playAreaSelector, | ||
| }; | ||
| this.restrictToDropzones = restrictToDropzones; | ||
| this.enableStacking = enableStacking; | ||
| this.stackDirection = stackDirection; | ||
| this.stackSpacing = stackSpacing; | ||
| this.useGhostEffect = useGhostEffect; | ||
|
|
||
| if (playAreaSelector !== this.options.playAreaSelector) { | ||
| const newPlayArea = document.querySelector(`.${playAreaSelector}`); | ||
| if (newPlayArea) { | ||
| this.playAreaElement = newPlayArea; | ||
| this.styleManager.applyPlayAreaStyles(this.playAreaElement); | ||
| this.dragManager.playAreaElement = this.playAreaElement; | ||
| this.dropzoneManager.playAreaElement = this.playAreaElement; | ||
| } else { | ||
| console.warn( | ||
| `Allonsh Warning: Play area element with class '${playAreaSelector}' not found.` | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| if (draggableSelector !== this.options.draggableSelector) { | ||
| this.eventManager.unbindEvents(this.draggableElements); | ||
| this.draggableElements = this.playAreaElement.querySelectorAll( | ||
| `.${draggableSelector}` | ||
| ); | ||
| this.eventManager.bindEvents(this.draggableElements); | ||
| } | ||
|
|
||
| if (dropzoneSelector !== this.options.dropzoneSelector) { | ||
| this.dropzoneManager.setDropzones(dropzoneSelector); | ||
| } | ||
|
|
There was a problem hiding this comment.
update() never applies changes due to comparing against already-mutated options
You assign this.options before comparing, so all equality checks are false negatives. Capture prevOptions first and compare against that.
Apply this diff:
update(newOptions = {}) {
const {
draggableSelector = this.options.draggableSelector,
dropzoneSelector = this.options.dropzoneSelector,
playAreaSelector = this.options.playAreaSelector,
restrictToDropzones = this.restrictToDropzones,
enableStacking = this.enableStacking,
stackDirection = this.stackDirection,
stackSpacing = this.stackSpacing,
useGhostEffect = this.useGhostEffect,
} = newOptions;
- this.options = {
- ...this.options,
- draggableSelector,
- dropzoneSelector,
- playAreaSelector,
- };
+ const prevOptions = { ...this.options };
+ this.options = { ...this.options, draggableSelector, dropzoneSelector, playAreaSelector };
this.restrictToDropzones = restrictToDropzones;
this.enableStacking = enableStacking;
this.stackDirection = stackDirection;
this.stackSpacing = stackSpacing;
this.useGhostEffect = useGhostEffect;
- if (playAreaSelector !== this.options.playAreaSelector) {
+ if (playAreaSelector !== prevOptions.playAreaSelector) {
const newPlayArea = document.querySelector(`.${playAreaSelector}`);
if (newPlayArea) {
this.playAreaElement = newPlayArea;
this.styleManager.applyPlayAreaStyles(this.playAreaElement);
this.dragManager.playAreaElement = this.playAreaElement;
this.dropzoneManager.playAreaElement = this.playAreaElement;
} else {
console.warn(
`Allonsh Warning: Play area element with class '${playAreaSelector}' not found.`
);
}
}
- if (draggableSelector !== this.options.draggableSelector) {
+ if (draggableSelector !== prevOptions.draggableSelector) {
this.eventManager.unbindEvents(this.draggableElements);
this.draggableElements = this.playAreaElement.querySelectorAll(
`.${draggableSelector}`
);
this.eventManager.bindEvents(this.draggableElements);
}
- if (dropzoneSelector !== this.options.dropzoneSelector) {
+ if (dropzoneSelector !== prevOptions.dropzoneSelector) {
this.dropzoneManager.setDropzones(dropzoneSelector);
}📝 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.
Suggested change
| this.options = { | |
| ...this.options, | |
| draggableSelector, | |
| dropzoneSelector, | |
| playAreaSelector, | |
| }; | |
| this.restrictToDropzones = restrictToDropzones; | |
| this.enableStacking = enableStacking; | |
| this.stackDirection = stackDirection; | |
| this.stackSpacing = stackSpacing; | |
| this.useGhostEffect = useGhostEffect; | |
| if (playAreaSelector !== this.options.playAreaSelector) { | |
| const newPlayArea = document.querySelector(`.${playAreaSelector}`); | |
| if (newPlayArea) { | |
| this.playAreaElement = newPlayArea; | |
| this.styleManager.applyPlayAreaStyles(this.playAreaElement); | |
| this.dragManager.playAreaElement = this.playAreaElement; | |
| this.dropzoneManager.playAreaElement = this.playAreaElement; | |
| } else { | |
| console.warn( | |
| `Allonsh Warning: Play area element with class '${playAreaSelector}' not found.` | |
| ); | |
| } | |
| } | |
| if (draggableSelector !== this.options.draggableSelector) { | |
| this.eventManager.unbindEvents(this.draggableElements); | |
| this.draggableElements = this.playAreaElement.querySelectorAll( | |
| `.${draggableSelector}` | |
| ); | |
| this.eventManager.bindEvents(this.draggableElements); | |
| } | |
| if (dropzoneSelector !== this.options.dropzoneSelector) { | |
| this.dropzoneManager.setDropzones(dropzoneSelector); | |
| } | |
| update(newOptions = {}) { | |
| const { | |
| draggableSelector = this.options.draggableSelector, | |
| dropzoneSelector = this.options.dropzoneSelector, | |
| playAreaSelector = this.options.playAreaSelector, | |
| restrictToDropzones = this.restrictToDropzones, | |
| enableStacking = this.enableStacking, | |
| stackDirection = this.stackDirection, | |
| stackSpacing = this.stackSpacing, | |
| useGhostEffect = this.useGhostEffect, | |
| } = newOptions; | |
| // Capture old options before mutating | |
| const prevOptions = { ...this.options }; | |
| this.options = { | |
| ...this.options, | |
| draggableSelector, | |
| dropzoneSelector, | |
| playAreaSelector, | |
| }; | |
| this.restrictToDropzones = restrictToDropzones; | |
| this.enableStacking = enableStacking; | |
| this.stackDirection = stackDirection; | |
| this.stackSpacing = stackSpacing; | |
| this.useGhostEffect = useGhostEffect; | |
| if (playAreaSelector !== prevOptions.playAreaSelector) { | |
| const newPlayArea = document.querySelector(`.${playAreaSelector}`); | |
| if (newPlayArea) { | |
| this.playAreaElement = newPlayArea; | |
| this.styleManager.applyPlayAreaStyles(this.playAreaElement); | |
| this.dragManager.playAreaElement = this.playAreaElement; | |
| this.dropzoneManager.playAreaElement = this.playAreaElement; | |
| } else { | |
| console.warn( | |
| `Allonsh Warning: Play area element with class '${playAreaSelector}' not found.` | |
| ); | |
| } | |
| } | |
| if (draggableSelector !== prevOptions.draggableSelector) { | |
| this.eventManager.unbindEvents(this.draggableElements); | |
| this.draggableElements = this.playAreaElement.querySelectorAll( | |
| `.${draggableSelector}` | |
| ); | |
| this.eventManager.bindEvents(this.draggableElements); | |
| } | |
| if (dropzoneSelector !== prevOptions.dropzoneSelector) { | |
| this.dropzoneManager.setDropzones(dropzoneSelector); | |
| } | |
| } |
Comment on lines
+152
to
+165
| addDraggable(element) { | ||
| this.eventManager.bindEvents([element]); | ||
| this.draggableElements = this.playAreaElement.querySelectorAll( | ||
| `.${CSS_CLASSES.DRAGGABLE}` | ||
| ); | ||
| } | ||
|
|
||
| removeDraggable(element) { | ||
| this.eventManager.unbindEvents([element]); | ||
| element.classList.remove(CSS_CLASSES.DRAGGABLE); | ||
| this.draggableElements = this.playAreaElement.querySelectorAll( | ||
| `.${CSS_CLASSES.DRAGGABLE}` | ||
| ); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Keep draggable discovery consistent with configuration
Re-querying by CSS_CLASSES.DRAGGABLE can diverge from the configured selector. The diff above switches these to use options.draggableSelector for consistency.
Run to check no hidden reliance on CSS_CLASSES.DRAGGABLE for discovery:
🏁 Script executed:
#!/bin/bash
# Expect matches only in Style/Event managers (for styling), not for querying lists in Allonsh.
rg -n --type=js 'CSS_CLASSES\.DRAGGABLE' src | sed -n '1,200p'Length of output: 313
Use configured draggableSelector instead of CSS_CLASSES.DRAGGABLE for queries
Replace the selector in addDraggable (line 155) and removeDraggable (line 163) with this.options.draggableSelector so discovery aligns with the configured selector.
🤖 Prompt for AI Agents
In src/Allonsh.js around lines 152 to 165, the querySelectorAll calls currently
use `.${CSS_CLASSES.DRAGGABLE}` which ignores a configured selector; update both
occurrences (in addDraggable and removeDraggable) to use
`this.options.draggableSelector` instead so discovery aligns with configuration,
e.g. replace the selector argument with `this.options.draggableSelector`
(preserve the rest of the logic), and ensure `this.options.draggableSelector` is
defined/available where these methods run.
Comment on lines
+55
to
+63
| _createGhostElement(rect, playAreaRect) { | ||
| this._removeGhostElement(); | ||
| this.ghostElement = this.currentDraggedElement.cloneNode(true); | ||
| this.styleManager.applyGhostStyles(this.ghostElement); | ||
| this.playAreaElement.appendChild(this.ghostElement); | ||
| this.ghostElement.style.left = `${rect.left - playAreaRect.left}px`; | ||
| this.ghostElement.style.top = `${rect.top - playAreaRect.top}px`; | ||
| this.currentDraggedElement.style.opacity = OPACITY.GHOST; | ||
| } |
There was a problem hiding this comment.
Prevent ghost from intercepting hit-tests and lock its size
The ghost can sit under the pointer and consume elementFromPoint hits, breaking dropzone detection. Also, cloning without locking width/height can cause size drift under different CSS context.
Apply this diff:
_createGhostElement(rect, playAreaRect) {
this._removeGhostElement();
this.ghostElement = this.currentDraggedElement.cloneNode(true);
this.styleManager.applyGhostStyles(this.ghostElement);
this.playAreaElement.appendChild(this.ghostElement);
this.ghostElement.style.left = `${rect.left - playAreaRect.left}px`;
this.ghostElement.style.top = `${rect.top - playAreaRect.top}px`;
+ this.ghostElement.style.width = `${rect.width}px`;
+ this.ghostElement.style.height = `${rect.height}px`;
+ this.ghostElement.style.pointerEvents = POINTER_EVENTS.NONE;
this.currentDraggedElement.style.opacity = OPACITY.GHOST;
}📝 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.
Suggested change
| _createGhostElement(rect, playAreaRect) { | |
| this._removeGhostElement(); | |
| this.ghostElement = this.currentDraggedElement.cloneNode(true); | |
| this.styleManager.applyGhostStyles(this.ghostElement); | |
| this.playAreaElement.appendChild(this.ghostElement); | |
| this.ghostElement.style.left = `${rect.left - playAreaRect.left}px`; | |
| this.ghostElement.style.top = `${rect.top - playAreaRect.top}px`; | |
| this.currentDraggedElement.style.opacity = OPACITY.GHOST; | |
| } | |
| _createGhostElement(rect, playAreaRect) { | |
| this._removeGhostElement(); | |
| this.ghostElement = this.currentDraggedElement.cloneNode(true); | |
| this.styleManager.applyGhostStyles(this.ghostElement); | |
| this.playAreaElement.appendChild(this.ghostElement); | |
| this.ghostElement.style.left = `${rect.left - playAreaRect.left}px`; | |
| this.ghostElement.style.top = `${rect.top - playAreaRect.top}px`; | |
| this.ghostElement.style.width = `${rect.width}px`; | |
| this.ghostElement.style.height = `${rect.height}px`; | |
| this.ghostElement.style.pointerEvents = POINTER_EVENTS.NONE; | |
| this.currentDraggedElement.style.opacity = OPACITY.GHOST; | |
| } |
🤖 Prompt for AI Agents
In src/DragManager.js around lines 55 to 63, the ghost element currently can
intercept hit-tests and its size can drift; after creating and styling the
cloned ghost, set its CSS pointer-events to 'none' so it won't block
elementFromPoint, and lock its size by explicitly setting style.width and
style.height from rect.width and rect.height (in px) before
appending/positioning it; keep the existing left/top positioning and ghost
opacity changes for the source element.
Comment on lines
+90
to
+129
| handleDrop(clientX, clientY, event) { | ||
| if (!this.currentDraggedElement) return; | ||
|
|
||
| this.currentDraggedElement.style.opacity = OPACITY.FULL; | ||
| const playAreaRect = this.playAreaElement.getBoundingClientRect(); | ||
| const clampedX = Math.min( | ||
| Math.max(clientX, playAreaRect.left + 1), | ||
| playAreaRect.right - 1 | ||
| ); | ||
| const clampedY = Math.min( | ||
| Math.max(clientY, playAreaRect.top + 1), | ||
| playAreaRect.bottom - 1 | ||
| ); | ||
|
|
||
| this.currentDraggedElement.style.pointerEvents = POINTER_EVENTS.NONE; | ||
| const elementBelow = document.elementFromPoint(clampedX, clampedY); | ||
| this.currentDraggedElement.style.pointerEvents = POINTER_EVENTS.AUTO; | ||
|
|
||
| if (!elementBelow && this.restrictToDropzones) { | ||
| this._returnToOrigin(); | ||
| this._resetDraggedElementState(); | ||
| return; | ||
| } | ||
|
|
||
| const dropzoneFound = | ||
| this.dropzoneManager.findClosestDropzone(elementBelow); | ||
| if (dropzoneFound) { | ||
| this.dropzoneManager.handleDropzoneDrop( | ||
| this.currentDraggedElement, | ||
| dropzoneFound, | ||
| event | ||
| ); | ||
| } else if (!this.restrictToDropzones) { | ||
| this._placeInPlayArea(clampedX, clampedY, playAreaRect); | ||
| } else { | ||
| this._returnToOrigin(); | ||
| } | ||
|
|
||
| this._resetDraggedElementState(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Emit DRAG_END/DROP events for parity with DRAG_START
Downstream listeners likely expect a matching lifecycle. Emit DROP when a drop target is resolved and DRAG_END at the end of the gesture.
Apply this diff (adjust event names if your constants differ):
if (dropzoneFound) {
this.dropzoneManager.handleDropzoneDrop(
this.currentDraggedElement,
dropzoneFound,
event
);
+ this.currentDraggedElement.dispatchEvent(
+ new CustomEvent(EVENTS.DROP, { detail: { target: dropzoneFound, originalEvent: event } })
+ );
} else if (!this.restrictToDropzones) {
this._placeInPlayArea(clampedX, clampedY, playAreaRect);
+ this.currentDraggedElement.dispatchEvent(
+ new CustomEvent(EVENTS.DROP, { detail: { target: this.playAreaElement, originalEvent: event } })
+ );
} else {
this._returnToOrigin();
}
- this._resetDraggedElementState();
+ this.currentDraggedElement.dispatchEvent(
+ new CustomEvent(EVENTS.DRAG_END, { detail: { originalEvent: event } })
+ );
+ this._resetDraggedElementState();To verify constants and usages across the repo:
🏁 Script executed:
#!/bin/bash
# Expect to see matching constants and no typos.
rg -n --type=js -C2 'DRAG_END|DROP' src constants.jsLength of output: 859
🏁 Script executed:
#!/bin/bash
# Search for any existing DRAG_END references across the source
rg -n "DRAG_END" -C2 src || true
rg -n "DRAG_END" -C2 . || trueLength of output: 61
Add DRAG_END constant and emit DROP/DRAG_END events
- In
src/constants.js, add:export const EVENTS = {
- DRAG_END: 'allonsh-dragend',
DRAG_START: 'allonsh-dragstart',
DROP: 'allonsh-drop',
…
- In `src/DragManager.js#handleDrop`, after handling a drop or placing the element:
```diff
if (dropzoneFound) {
this.dropzoneManager.handleDropzoneDrop(
this.currentDraggedElement,
dropzoneFound,
event
);
+ this.currentDraggedElement.dispatchEvent(
+ new CustomEvent(EVENTS.DROP, { detail: { target: dropzoneFound, originalEvent: event } })
+ );
} else if (!this.restrictToDropzones) {
this._placeInPlayArea(clampedX, clampedY, playAreaRect);
+ this.currentDraggedElement.dispatchEvent(
+ new CustomEvent(EVENTS.DROP, { detail: { target: this.playAreaElement, originalEvent: event } })
+ );
} else {
this._returnToOrigin();
}
- this._resetDraggedElementState();
+ this.currentDraggedElement.dispatchEvent(
+ new CustomEvent(EVENTS.DRAG_END, { detail: { originalEvent: event } })
+ );
+ this._resetDraggedElementState();
- (Optional) Emit
DRAG_ENDbefore the early return in therestrictToDropzonesbranch to signal cancellation.
📝 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.
Suggested change
| handleDrop(clientX, clientY, event) { | |
| if (!this.currentDraggedElement) return; | |
| this.currentDraggedElement.style.opacity = OPACITY.FULL; | |
| const playAreaRect = this.playAreaElement.getBoundingClientRect(); | |
| const clampedX = Math.min( | |
| Math.max(clientX, playAreaRect.left + 1), | |
| playAreaRect.right - 1 | |
| ); | |
| const clampedY = Math.min( | |
| Math.max(clientY, playAreaRect.top + 1), | |
| playAreaRect.bottom - 1 | |
| ); | |
| this.currentDraggedElement.style.pointerEvents = POINTER_EVENTS.NONE; | |
| const elementBelow = document.elementFromPoint(clampedX, clampedY); | |
| this.currentDraggedElement.style.pointerEvents = POINTER_EVENTS.AUTO; | |
| if (!elementBelow && this.restrictToDropzones) { | |
| this._returnToOrigin(); | |
| this._resetDraggedElementState(); | |
| return; | |
| } | |
| const dropzoneFound = | |
| this.dropzoneManager.findClosestDropzone(elementBelow); | |
| if (dropzoneFound) { | |
| this.dropzoneManager.handleDropzoneDrop( | |
| this.currentDraggedElement, | |
| dropzoneFound, | |
| event | |
| ); | |
| } else if (!this.restrictToDropzones) { | |
| this._placeInPlayArea(clampedX, clampedY, playAreaRect); | |
| } else { | |
| this._returnToOrigin(); | |
| } | |
| this._resetDraggedElementState(); | |
| } | |
| handleDrop(clientX, clientY, event) { | |
| if (!this.currentDraggedElement) return; | |
| this.currentDraggedElement.style.opacity = OPACITY.FULL; | |
| const playAreaRect = this.playAreaElement.getBoundingClientRect(); | |
| const clampedX = Math.min( | |
| Math.max(clientX, playAreaRect.left + 1), | |
| playAreaRect.right - 1 | |
| ); | |
| const clampedY = Math.min( | |
| Math.max(clientY, playAreaRect.top + 1), | |
| playAreaRect.bottom - 1 | |
| ); | |
| this.currentDraggedElement.style.pointerEvents = POINTER_EVENTS.NONE; | |
| const elementBelow = document.elementFromPoint(clampedX, clampedY); | |
| this.currentDraggedElement.style.pointerEvents = POINTER_EVENTS.AUTO; | |
| if (!elementBelow && this.restrictToDropzones) { | |
| this._returnToOrigin(); | |
| this._resetDraggedElementState(); | |
| return; | |
| } | |
| const dropzoneFound = | |
| this.dropzoneManager.findClosestDropzone(elementBelow); | |
| if (dropzoneFound) { | |
| this.dropzoneManager.handleDropzoneDrop( | |
| this.currentDraggedElement, | |
| dropzoneFound, | |
| event | |
| ); | |
| this.currentDraggedElement.dispatchEvent( | |
| new CustomEvent(EVENTS.DROP, { | |
| detail: { target: dropzoneFound, originalEvent: event } | |
| }) | |
| ); | |
| } else if (!this.restrictToDropzones) { | |
| this._placeInPlayArea(clampedX, clampedY, playAreaRect); | |
| this.currentDraggedElement.dispatchEvent( | |
| new CustomEvent(EVENTS.DROP, { | |
| detail: { target: this.playAreaElement, originalEvent: event } | |
| }) | |
| ); | |
| } else { | |
| this._returnToOrigin(); | |
| } | |
| this.currentDraggedElement.dispatchEvent( | |
| new CustomEvent(EVENTS.DRAG_END, { | |
| detail: { originalEvent: event } | |
| }) | |
| ); | |
| this._resetDraggedElementState(); | |
| } |
🤖 Prompt for AI Agents
In src/DragManager.js around lines 90 to 129, import the new DROP and DRAG_END
constants from src/constants.js and update handleDrop to emit a DROP event when
a drop actually happens (pass the dragged element, the dropzone when
dropzoneFound is true, or the final coordinates/playAreaRect when placed in the
play area), and always emit DRAG_END after handling the drop/placement; also
emit DRAG_END before the early return in the restrictToDropzones cancellation
branch so listeners know the drag sequence ended. Ensure the event emit calls
match the existing event bus/api used in this class (e.g. this.eventBus.emit or
this.emit) and include the original DOM event in the payload for context.
Comment on lines
+57
to
+76
| handleDropzoneDrop(draggedElement, dropzone, event) { | ||
| if (this.enableStacking) { | ||
| this.applyStackingStyles(dropzone); | ||
| } | ||
| this.styleManager.resetPosition(draggedElement); | ||
| dropzone.appendChild(draggedElement); | ||
|
|
||
| dropzone.dispatchEvent( | ||
| new CustomEvent(EVENTS.DROP, { | ||
| detail: { draggedElement, originalEvent: event }, | ||
| }) | ||
| ); | ||
| dropzone.dispatchEvent( | ||
| new CustomEvent(EVENTS.DRAG_ENTER, { detail: { draggedElement } }) | ||
| ); | ||
| dropzone.dispatchEvent( | ||
| new CustomEvent(EVENTS.DRAG_LEAVE, { detail: { draggedElement } }) | ||
| ); | ||
| dropzone.classList.remove(CSS_CLASSES.HIGHLIGHT); | ||
| } |
There was a problem hiding this comment.
Don’t emit DRAG_ENTER and DRAG_LEAVE on drop.
Emitting both immediately after DROP is misleading for listeners and duplicates lifecycle events. Only DROP should fire here.
handleDropzoneDrop(draggedElement, dropzone, event) {
if (this.enableStacking) {
this.applyStackingStyles(dropzone);
}
this.styleManager.resetPosition(draggedElement);
dropzone.appendChild(draggedElement);
dropzone.dispatchEvent(
new CustomEvent(EVENTS.DROP, {
detail: { draggedElement, originalEvent: event },
})
);
- dropzone.dispatchEvent(
- new CustomEvent(EVENTS.DRAG_ENTER, { detail: { draggedElement } })
- );
- dropzone.dispatchEvent(
- new CustomEvent(EVENTS.DRAG_LEAVE, { detail: { draggedElement } })
- );
dropzone.classList.remove(CSS_CLASSES.HIGHLIGHT);
}📝 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.
Suggested change
| handleDropzoneDrop(draggedElement, dropzone, event) { | |
| if (this.enableStacking) { | |
| this.applyStackingStyles(dropzone); | |
| } | |
| this.styleManager.resetPosition(draggedElement); | |
| dropzone.appendChild(draggedElement); | |
| dropzone.dispatchEvent( | |
| new CustomEvent(EVENTS.DROP, { | |
| detail: { draggedElement, originalEvent: event }, | |
| }) | |
| ); | |
| dropzone.dispatchEvent( | |
| new CustomEvent(EVENTS.DRAG_ENTER, { detail: { draggedElement } }) | |
| ); | |
| dropzone.dispatchEvent( | |
| new CustomEvent(EVENTS.DRAG_LEAVE, { detail: { draggedElement } }) | |
| ); | |
| dropzone.classList.remove(CSS_CLASSES.HIGHLIGHT); | |
| } | |
| handleDropzoneDrop(draggedElement, dropzone, event) { | |
| if (this.enableStacking) { | |
| this.applyStackingStyles(dropzone); | |
| } | |
| this.styleManager.resetPosition(draggedElement); | |
| dropzone.appendChild(draggedElement); | |
| dropzone.dispatchEvent( | |
| new CustomEvent(EVENTS.DROP, { | |
| detail: { draggedElement, originalEvent: event }, | |
| }) | |
| ); | |
| dropzone.classList.remove(CSS_CLASSES.HIGHLIGHT); | |
| } |
🤖 Prompt for AI Agents
In src/DropzoneManager.js around lines 57 to 76, the handler currently emits
EVENTS.DRAG_ENTER and EVENTS.DRAG_LEAVE immediately after emitting EVENTS.DROP,
which duplicates and misleads lifecycle listeners; remove the two extra
dispatches so only the DROP event is emitted (keep the DROP dispatch,
appendChild call, stacking/styles handling, and cleanup of
CSS_CLASSES.HIGHLIGHT) ensuring listeners receive a single DROP notification on
successful drop.
Comment on lines
+60
to
+64
| _onMouseUp(event) { | ||
| this.dragManager.handleDrop(event.clientX, event.clientY, event); | ||
| document.removeEventListener('mousemove', this._boundMouseMoveHandler); | ||
| document.removeEventListener('mouseup', this._boundMouseUpHandler); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Always remove listeners even if handleDrop throws (use try/finally).
Prevents leaked handlers on exceptions during drop handling.
_onMouseUp(event) {
- this.dragManager.handleDrop(event.clientX, event.clientY, event);
- document.removeEventListener('mousemove', this._boundMouseMoveHandler);
- document.removeEventListener('mouseup', this._boundMouseUpHandler);
+ try {
+ this.dragManager.handleDrop(event.clientX, event.clientY, event);
+ } finally {
+ document.removeEventListener('mousemove', this._boundMouseMoveHandler);
+ document.removeEventListener('mouseup', this._boundMouseUpHandler);
+ }
}
_onTouchEnd(event) {
- const touchPoint = event.changedTouches[0];
- this.dragManager.handleDrop(touchPoint.clientX, touchPoint.clientY, event);
- document.removeEventListener('touchmove', this._boundTouchMoveHandler);
- document.removeEventListener('touchend', this._boundTouchEndHandler);
+ const touchPoint = event.changedTouches[0];
+ try {
+ this.dragManager.handleDrop(touchPoint.clientX, touchPoint.clientY, event);
+ } finally {
+ document.removeEventListener('touchmove', this._boundTouchMoveHandler);
+ document.removeEventListener('touchend', this._boundTouchEndHandler);
+ }
}Also applies to: 66-71
🤖 Prompt for AI Agents
In src/EventManager.js around lines 60-64 (and similarly for 66-71), the event
listeners are removed only after calling this.dragManager.handleDrop, so if
handleDrop throws the mousemove/mouseup listeners remain attached; wrap the
handleDrop invocation in a try/finally block and move the
document.removeEventListener calls into the finally so listeners are always
removed regardless of errors, and mirror the same try/finally pattern in the
other occurrence at lines 66-71.
Comment on lines
+25
to
+31
| applyGhostStyles(element) { | ||
| element.style.pointerEvents = POINTER_EVENTS.NONE; | ||
| element.style.position = CSS_POSITIONS.ABSOLUTE; | ||
| element.style.zIndex = Z_INDEX.GHOST; | ||
| element.style.opacity = OPACITY.FULL; | ||
| } | ||
|
|
There was a problem hiding this comment.
Bug: Ghost uses full opacity; should use OPACITY.GHOST.
Currently the ghost appears fully opaque, ignoring the OPACITY.GHOST constant.
applyGhostStyles(element) {
element.style.pointerEvents = POINTER_EVENTS.NONE;
element.style.position = CSS_POSITIONS.ABSOLUTE;
element.style.zIndex = Z_INDEX.GHOST;
- element.style.opacity = OPACITY.FULL;
+ element.style.opacity = OPACITY.GHOST;
}📝 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.
Suggested change
| applyGhostStyles(element) { | |
| element.style.pointerEvents = POINTER_EVENTS.NONE; | |
| element.style.position = CSS_POSITIONS.ABSOLUTE; | |
| element.style.zIndex = Z_INDEX.GHOST; | |
| element.style.opacity = OPACITY.FULL; | |
| } | |
| applyGhostStyles(element) { | |
| element.style.pointerEvents = POINTER_EVENTS.NONE; | |
| element.style.position = CSS_POSITIONS.ABSOLUTE; | |
| element.style.zIndex = Z_INDEX.GHOST; | |
| element.style.opacity = OPACITY.GHOST; | |
| } |
🤖 Prompt for AI Agents
In src/StyleManager.js around lines 25 to 31, the applyGhostStyles method sets
element.style.opacity to OPACITY.FULL instead of using OPACITY.GHOST; change the
assignment to use OPACITY.GHOST so the ghost element is semi-transparent as
intended, leaving the other style assignments untouched.
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.
Description
Refactored Allonsh into modular structure for better maintainability, splitting functionality into EventManager, DragManager, DropzoneManager, StyleManager, and constants.js. Preserved all drag-and-drop features, removed redundant code, and used PascalCase naming (e.g., Allonsh.js). Test suite updates pending in follow-up PR. No breaking changes.
Changes:
Related Issue
Type of Change
How Has This Been Tested?
Checklist
Additional Notes
N/A
Summary by CodeRabbit
New Features
Refactor
Chores