fix: update lasso.js for D3 v6+ compatibility#1276
Open
tonypzy wants to merge 3 commits into
Open
Conversation
Contributor
Reviewer's GuideUpdates js/lasso.js to be compatible with D3 v6+ while maintaining support for v4/v5, adds null-safety around lasso interaction state, and applies minor ES6/style cleanups including a clearer distance calculation. Sequence diagram for lasso drag lifecycle with D3 v6 pointer fallback and null safetysequenceDiagram
actor User
participant Root as RootSVG
participant Lasso as lasso
participant Drag as d3drag
participant D3 as d3_selection
participant LassoPath as lassoPath
participant ClosePath as closePath
participant Dispatch as dispatch
User->>Root: mousedown and drag on area
Root->>Drag: drag behavior triggered
Drag->>Lasso: handleDragStart(event)
alt d3.mouse available
Lasso->>D3: mouse(this)
D3-->>Lasso: startPoint
else d3.mouse not available
Lasso->>D3: pointer(event, this)
D3-->>Lasso: startPoint
end
Lasso->>Lasso: lassoPolygon = [startPoint]
Lasso->>Root: append path as lassoPath
Lasso->>Root: append line as closePath
Lasso->>Dispatch: start(lassoPolygon)
loop while user drags
Drag->>Lasso: handleDrag(event)
Lasso->>Lasso: check lassoPolygon && lassoPath && closePath
alt lasso state missing
Lasso-->>Drag: return (no-op)
else lasso state valid
alt d3.mouse available
Lasso->>D3: mouse(this)
D3-->>Lasso: point
else d3.mouse not available
Lasso->>D3: pointer(event, this)
D3-->>Lasso: point
end
Lasso->>Lasso: push point into lassoPolygon
Lasso->>LassoPath: update d attribute with polygonToPath(lassoPolygon)
Lasso->>Lasso: compute distance(startPoint, lastPoint)
alt distance < closeDistance
Lasso->>ClosePath: update x1,y1,opacity=1
else
Lasso->>ClosePath: opacity=0
end
end
end
User->>Root: mouseup
Drag->>Lasso: handleDragEnd()
alt closePath exists
Lasso->>ClosePath: remove()
Lasso->>Lasso: closePath = null
else no closePath
Lasso-->>Drag: continue
end
Lasso->>Lasso: check lassoPolygon
alt lassoPolygon exists and distance(startPoint, lastPoint) < closeDistance
Lasso->>LassoPath: close path with Z
Lasso->>Dispatch: end(lassoPolygon)
else cancel lasso
alt lassoPath exists
Lasso->>LassoPath: remove()
Lasso->>Lasso: lassoPath = null
end
end
Lasso->>Lasso: lassoPolygon = null
User->>Lasso: calls reset()
Lasso->>LassoPath: remove if exists
Lasso->>ClosePath: remove if exists
Lasso->>Lasso: lassoPolygon = null, lassoPath = null, closePath = null
Class diagram for lasso module with updated handlers and utilitiesclassDiagram
class PolygonUtils {
+polygonToPath(polygon)
+distance(pt1, pt2)
}
class Lasso {
+hoverSelect : Boolean
+closeDistance : Number
+lassoPolygon : Array
+lassoPath : Object
+closePath : Object
+g : Object
+area : Object
+bbox : Object
+dispatch : Object
+lasso(root)
+handleDragStart(event)
+handleDrag(event)
+handleDragEnd()
+reset()
}
class D3Selection {
+mouse(target)
+pointer(event, target)
}
class D3Drag {
+drag()
}
PolygonUtils <.. Lasso : uses
D3Selection <.. Lasso : uses mouse_or_pointer
D3Drag <.. Lasso : used_for_drag_behavior
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The D3 coordinate fallback logic (
d3.mouse ? d3.mouse(...) : d3.pointer(...)) is duplicated and a bit brittle; consider extracting a small helper likegetPointer(event, node)and preferringd3.pointerwhen available (with atypeof d3.pointer === 'function'check andd3.mouseas the legacy fallback) to make the version-compat behavior clearer and easier to maintain. - In
handleDragStart, the new code assumeslassoPolygonandgare always valid; ifreset()can be called before a new drag starts, consider mirroring the null-check pattern you added inhandleDrag/handleDragEndso a partially reset lasso group cannot cause errors when a drag is initiated.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The D3 coordinate fallback logic (`d3.mouse ? d3.mouse(...) : d3.pointer(...)`) is duplicated and a bit brittle; consider extracting a small helper like `getPointer(event, node)` and preferring `d3.pointer` when available (with a `typeof d3.pointer === 'function'` check and `d3.mouse` as the legacy fallback) to make the version-compat behavior clearer and easier to maintain.
- In `handleDragStart`, the new code assumes `lassoPolygon` and `g` are always valid; if `reset()` can be called before a new drag starts, consider mirroring the null-check pattern you added in `handleDrag`/`handleDragEnd` so a partially reset lasso group cannot cause errors when a drag is initiated.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
Author
|
@Manik-Khajuria-5 @Jayantparashar10 @vedansh-5 Please review. |
8 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the lasso interaction helper to be compatible across D3 v4–v6+ and more robust during active drag interactions to address remaining CI failures from #997.
Changes:
- Adds coordinate extraction fallback between legacy
d3.mouseand modernd3.pointer. - Guards drag handling against mid-interaction resets to prevent
TypeErrorcrashes. - Refactors geometry and formatting (
Math.hypot, shorter path generation).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
93
to
118
| function handleDragEnd() { | ||
| // remove the close path | ||
| closePath.remove(); | ||
| closePath = null; | ||
| if (closePath) { | ||
| closePath.remove(); | ||
| closePath = null; | ||
| } | ||
|
|
||
| // successfully closed | ||
| if ( | ||
| distance(lassoPolygon[0], lassoPolygon[lassoPolygon.length - 1]) < | ||
| closeDistance | ||
| lassoPolygon && | ||
| distance(lassoPolygon[0], | ||
| lassoPolygon[lassoPolygon.length - 1]) < closeDistance | ||
| ) { | ||
| lassoPath.attr('d', polygonToPath(lassoPolygon) + 'Z'); | ||
| dispatch.call('end', lasso, lassoPolygon); | ||
|
|
||
| // otherwise cancel | ||
| } else { | ||
| lassoPath.remove(); | ||
| lassoPath = null; | ||
| lassoPolygon = null; | ||
| if (lassoPath) { | ||
| lassoPath.remove(); | ||
| lassoPath = null; | ||
| } | ||
| } | ||
|
|
||
| lassoPolygon = null; | ||
| } |
Comment on lines
+50
to
+51
| function handleDragStart(event) { | ||
| lassoPolygon = [d3.mouse ? d3.mouse(this) : d3.pointer(event, this)]; |
|
|
||
| function handleDrag() { | ||
| var point = d3.mouse(this); | ||
| function handleDrag(event) { |
| // If reset() was called mid-drag, bail out safely. | ||
| if (!lassoPolygon || !lassoPath || !closePath) return; | ||
|
|
||
| var point = d3.mouse ? d3.mouse(this) : d3.pointer(event, this); |
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.
Description
This PR follows up on #997 to resolve remaining CI failures and ensure compatibility across different D3 versions (v4 through v6+).
Key Changes:
d3.pointerwhere available (D3 v6+) and falls back tod3.mousefor legacy environments (v4/v5).handleDragforlassoPolygon,lassoPath, andclosePath. This preventsTypeErrorcrashes if the lasso is reset during an active interaction.Math.hypot()for better clarity.max-lenviolations by wrapping long statements and comments to meet the 80-character limit.How Has This Been Tested?
Verified via code inspection.
Types of changes
Checklist:
Summary by Sourcery
Ensure the lasso selection helper works reliably across D3 versions and is more robust during drag interactions.
Bug Fixes:
Enhancements: