dragging #2

bart-jaskulski · 2025-11-25T22:10:25Z

No description provided.

Copilot

Pull request overview

This PR implements drag-and-drop functionality for task reordering in a hierarchical task list. The implementation introduces a new DragProvider component that manages drag state, visual feedback, and drop calculations.

Key Changes:

New DragProvider component with pointer-based drag-and-drop state management, auto-scrolling, and accessibility announcements
Integration of drag-and-drop into the task list with drop position calculation (above/below/inside) and tree validation to prevent invalid moves
Visual enhancements including drag overlays, drop indicators, and transition animations

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
src/components/DragProvider.tsx	New context provider managing drag state, pointer events, auto-scroll, and accessibility features
src/components/TasksList.tsx	Integrated DragProvider with drop handling logic and drag overlay rendering
src/components/TaskItem.tsx	Added drag handle with pointer event binding
src/components/TaskItem.css	Added drag handle styling with touch-action and user-select properties, plus unused drag-shadow class
src/components/TasksList.css	Added visual feedback for drag states, drop indicators, overlay styling, and sr-only utility class

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-25T22:17:19Z

src/components/DragProvider.tsx

+      if (y > height - HOT_ZONE) {
+        window.scrollBy(0, 6 + (y - (height - HOT_ZONE)) / 8);
+      } else if (y < HOT_ZONE) {
+        window.scrollBy(0, -(6 + (HOT_ZONE - y) / 8));


[nitpick] The auto-scroll calculation uses magic numbers 6 and 8 without explanation. The formula 6 + (y - (height - HOT_ZONE)) / 8 determines scroll speed but the constants lack clarity. Consider extracting these as named constants (e.g., BASE_SCROLL_SPEED = 6, SCROLL_ACCELERATION = 8) to improve code readability and maintainability.

Copilot · 2025-11-25T22:17:20Z

src/components/TaskItem.css

+.task-item__drag-shadow {
+  background: var(--surface, #fff);
+  border-radius: var(--radius-lg, 16px);
+  padding: 0.75rem 1rem;
+  box-shadow: 0 24px 60px -32px rgba(15, 23, 42, 0.5);
+  border: 1px solid var(--border-subtle, #d8e2ec);
+}


The .task-item__drag-shadow class is defined but never appears to be used in the codebase. The drag overlay uses .drag-overlay__card instead (defined in TasksList.css). This unused CSS should be removed to avoid confusion and reduce code bloat.

Suggested change

.task-item__drag-shadow {

background: var(--surface, #fff);

border-radius: var(--radius-lg, 16px);

padding: 0.75rem 1rem;

box-shadow: 0 24px 60px -32px rgba(15, 23, 42, 0.5);

border: 1px solid var(--border-subtle, #d8e2ec);

}

Copilot · 2025-11-25T22:17:20Z

src/components/TasksList.css

+.sr-only {
+  position: absolute;
+  width: 1px;
+  height: 1px;
+  padding: 0;
+  margin: -1px;
+  overflow: hidden;
+  clip: rect(0, 0, 0, 0);
+  white-space: nowrap;
+  border-width: 0;
+}


[nitpick] The .sr-only class is defined in TasksList.css but is used for the announcer element in DragProvider.tsx (line 248). This creates a dependency between the CSS file and the component that isn't obvious. Consider moving this utility class to a global CSS file or creating a dedicated accessibility utilities CSS file to make the dependency clearer and allow reuse across components.

Copilot · 2025-11-25T22:17:20Z

src/components/DragProvider.tsx

+    window.addEventListener("pointermove", handlePointerMove, { passive: false });
+    window.addEventListener("pointerup", finishDrag);


The finishDrag function is added as a pointerup listener but also tries to release pointer capture. However, if the user moves the pointer outside the window and releases, or if there's an interaction that causes the pointer capture to be lost (like a browser dialog), the event listeners could remain attached. Consider also listening for pointercancel events to handle these edge cases and ensure cleanup happens correctly.

Copilot · 2025-11-25T22:17:20Z

src/components/TasksList.tsx

+  const renderOverlay = (id: string | null) => {
+    if (!id) return null;
+
+    const findNode = (nodes: TreeNode[]): TreeNode | undefined => {
+      for (const node of nodes) {
+        if (node.id === id) return node;
+        const nested = findNode(node.children);
+        if (nested) return nested;
+      }
+      return undefined;
+    };
+
+    const node = findNode(tasksStore());
+    if (!node) return null;
+
+    return (
+      <div class="drag-overlay__card">
+        {node.text}
+      </div>
+    );
+  };


The renderOverlay function performs a recursive tree search on every render when state.draggedId is not null. Since this is called within the Portal's Show component, it will be re-evaluated whenever the drag state changes. This could be optimized by memoizing the result or by indexing the tree once during drag start rather than searching repeatedly.

Copilot · 2025-11-25T22:17:22Z

src/components/TasksList.tsx

+  const handleDrop = (draggedId: string, targetId: string, position: Exclude<DropPosition, null>) => {
+    const tree = tasksStore();
+
+    const nodeIndex = new Map<string, TreeNode>();
+    const parentIndex = new Map<string, string | null>();
+
+    const indexTree = (nodes: TreeNode[], parentId: string | null) => {
+      nodes.forEach(node => {
+        nodeIndex.set(node.id, node);
+        parentIndex.set(node.id, parentId);
+        indexTree(node.children, node.id);
+      });
+    };
+
+    indexTree(tree, null);


[nitpick] The handleDrop function rebuilds the node and parent indexes from scratch on every drop operation. While this ensures data freshness, the same indexing logic is duplicated in renderOverlay (lines 89-96). Consider extracting this indexing logic into a shared utility function or memoizing it to avoid code duplication and improve maintainability.

Copilot · 2025-11-25T22:17:23Z

src/components/DragProvider.tsx

+    if (state.status === "DRAGGING") {
+      setState("status", "DROPPING");
+      announce("Drop complete");
+      window.setTimeout(resetState, 220);


[nitpick] Magic number for timeout duration. The timeout value 220 ms is used without explanation and should match or relate to the transition durations elsewhere in the code. For example, the "DROPPING" animation transition is 160ms (line 204), but the reset happens after 220ms. While this provides a buffer, using a named constant like DROP_ANIMATION_DURATION + BUFFER would make the relationship and intent clearer.

Copilot · 2025-11-25T22:17:23Z

src/components/DragProvider.tsx

+const calculateDropPosition = (pointerY: number, rect: DOMRect): Exclude<DropPosition, null> => {
+  const relativeY = (pointerY - rect.top) / rect.height;
+  if (relativeY < 0.25) return "above";
+  if (relativeY > 0.75) return "below";


[nitpick] The drop position thresholds (0.25 and 0.75) are hardcoded magic numbers. These define when the drop target switches from "above" to "inside" to "below". Consider extracting these as named constants like DROP_ABOVE_THRESHOLD = 0.25 and DROP_BELOW_THRESHOLD = 0.75 to make the behavior more explicit and easier to adjust.

Suggested change

const calculateDropPosition = (pointerY: number, rect: DOMRect): Exclude<DropPosition, null> => {

const relativeY = (pointerY - rect.top) / rect.height;

if (relativeY < 0.25) return "above";

if (relativeY > 0.75) return "below";

const DROP_ABOVE_THRESHOLD = 0.25;

const DROP_BELOW_THRESHOLD = 0.75;

const calculateDropPosition = (pointerY: number, rect: DOMRect): Exclude<DropPosition, null> => {

const relativeY = (pointerY - rect.top) / rect.height;

if (relativeY < DROP_ABOVE_THRESHOLD) return "above";

if (relativeY > DROP_BELOW_THRESHOLD) return "below";

Copilot · 2025-11-25T22:17:23Z

src/components/TasksList.css

  display: flex;
  flex-direction: column;
-  gap: 0.6rem;
+  gap: 0;


[nitpick] The gap was changed from 0.6rem to 0 which removes all spacing between task nodes. This could negatively impact visual hierarchy and readability. The drop indicator positioning (lines 47-52) assumes specific spacing with absolute positioning at -6px and bottom: -6px, which may not work correctly with zero gap. Consider if this spacing change is intentional and whether the drop indicators need adjustment.

Copilot · 2025-11-25T22:17:23Z

src/components/DragProvider.tsx

+                Dragging
+              </div>
+            }>
+              {props.renderOverlay ? props.renderOverlay(state.draggedId) : null}


Redundant condition in the fallback JSX. The Show component already checks when={props.renderOverlay}, but inside the children, there's another check {props.renderOverlay ? props.renderOverlay(state.draggedId) : null}. This is redundant because the Show component's children only execute when the condition is true. The children should simply be {props.renderOverlay!(state.draggedId)} with the non-null assertion operator.

Suggested change

{props.renderOverlay ? props.renderOverlay(state.draggedId) : null}

{props.renderOverlay!(state.draggedId)}

dragging

3d2505f

bart-jaskulski requested a review from Copilot November 25, 2025 22:10

Copilot started reviewing on behalf of bart-jaskulski November 25, 2025 22:10 View session

Copilot finished reviewing on behalf of bart-jaskulski November 25, 2025 22:14

Copilot AI reviewed Nov 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

dragging #2

dragging #2

Uh oh!

bart-jaskulski commented Nov 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 25, 2025

Uh oh!

Copilot AI Nov 25, 2025

Uh oh!

Copilot AI Nov 25, 2025

Uh oh!

Copilot AI Nov 25, 2025

Uh oh!

Copilot AI Nov 25, 2025

Uh oh!

Copilot AI Nov 25, 2025

Uh oh!

Copilot AI Nov 25, 2025

Uh oh!

Copilot AI Nov 25, 2025

Uh oh!

Copilot AI Nov 25, 2025

Uh oh!

Copilot AI Nov 25, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		window.addEventListener("pointermove", handlePointerMove, { passive: false });
		window.addEventListener("pointerup", finishDrag);

	{props.renderOverlay ? props.renderOverlay(state.draggedId) : null}
	{props.renderOverlay!(state.draggedId)}

dragging #2

Are you sure you want to change the base?

dragging #2

Uh oh!

Conversation

bart-jaskulski commented Nov 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants