fix(sidebar): honor category drop indicator contract#5
Conversation
📝 WalkthroughWalkthroughThis PR introduces keyboard drag support for the sortable categories list by adding state to distinguish keyboard-initiated drags, a new utility function to determine visible drop projections, and lifecycle management to sync projection visibility with the UI during drag operations. ChangesKeyboard Drag Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/sidebar/SortableCategoriesList.tsx (2)
791-807:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t record a parent change when the drop lands outside every droppable.
finalProjectioncan still hold the last visible child projection whenevent.overisnull. This block writes that parent intodropProjectionRef, but the laterif (over)guard skips the IPC, so the screen-reader announcement path can report a successful reparent that never happened.Suggested fix
const activeStr = String(active.id); const activeRow = baseFlatRef.current.find((it) => String(it.id) === activeStr); const oldParentId: string | null = activeRow?.parentId ?? null; let newParentId: string | null; - if (finalProjection) { + if (over && finalProjection) { newParentId = finalProjection.parentId; } else { // No projection (dwell never armed) — same-level reorder; parent // stays unchanged. newParentId = oldParentId;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/sidebar/SortableCategoriesList.tsx` around lines 791 - 807, The code sets newParentId from finalProjection.parentId even when the drop target is outside every droppable (event.over is null), causing a false reparent in dropProjectionRef; change the logic so newParentId is set to finalProjection.parentId only when the actual over target exists (e.g., over is truthy), otherwise keep newParentId = oldParentId; update the block that computes newParentId (around activeStr, baseFlatRef.current, finalProjection, oldParentId) so dropProjectionRef.current never records a parent change when over is null.
403-423:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeyboard drags must bypass pointer-geometry resolution in
getProjection.When
isKeyboardDragis true,pointerBelowOvercan become non-null duringonDragMove(since keyboard sensors emit DragMove events). Passing this pointer-derived value togetProjectionactivates position-aware mode, causing the insertion point to be determined by pointer geometry instead of the keyboard coordinate getter's X-offset logic. This contradicts the documented intent (line 390) and defeats the keyboard sensor's depth-control mechanism.The suggested fix correctly enforces the legacy path for keyboard drags by passing
undefinedinstead ofpointerBelowOver ?? undefinedwhenisKeyboardDragis true.Suggested fix
const projected = useMemo(() => { if (activeId === null || overId === null) return null; // Asymmetric gate: only ROOT-active demote requires dwell. if (!isChildActive && dwellState === 'OUT') return null; @@ // the legacy arrayMove path because there is no // pointer side; the coordinate getter supplies depth intent via X. if (!isChildActive && !isKeyboardDrag && pointerBelowOver === null) return null; + const projectionPointerSide = isKeyboardDrag ? undefined : (pointerBelowOver ?? undefined); return getProjection( displayFlat, activeId, overId, offsetLeft, INDENT_STEP_PX, - pointerBelowOver ?? undefined, + projectionPointerSide, baseFlat, activeOriginalParentId, ); }, [🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/sidebar/SortableCategoriesList.tsx` around lines 403 - 423, The projected useMemo passes pointerBelowOver (via pointerBelowOver ?? undefined) into getProjection which lets pointer geometry override keyboard drag depth intent; update the call in the projected memo so when isKeyboardDrag is true you pass undefined for the pointer side (i.e. use pointerBelowOver ?? undefined only when !isKeyboardDrag, otherwise pass undefined) so getProjection uses the keyboard X-based depth logic; locate this change in the projected useMemo that calls getProjection and adjust the pointer argument accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/components/sidebar/SortableCategoriesList.tsx`:
- Around line 791-807: The code sets newParentId from finalProjection.parentId
even when the drop target is outside every droppable (event.over is null),
causing a false reparent in dropProjectionRef; change the logic so newParentId
is set to finalProjection.parentId only when the actual over target exists
(e.g., over is truthy), otherwise keep newParentId = oldParentId; update the
block that computes newParentId (around activeStr, baseFlatRef.current,
finalProjection, oldParentId) so dropProjectionRef.current never records a
parent change when over is null.
- Around line 403-423: The projected useMemo passes pointerBelowOver (via
pointerBelowOver ?? undefined) into getProjection which lets pointer geometry
override keyboard drag depth intent; update the call in the projected memo so
when isKeyboardDrag is true you pass undefined for the pointer side (i.e. use
pointerBelowOver ?? undefined only when !isKeyboardDrag, otherwise pass
undefined) so getProjection uses the keyboard X-based depth logic; locate this
change in the projected useMemo that calls getProjection and adjust the pointer
argument accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 119dea7a-5039-4ac1-8c67-bbb45a5efedd
📒 Files selected for processing (3)
src/components/sidebar/SortableCategoriesList.tsxsrc/components/sidebar/dnd/__tests__/treeUtilities.test.tssrc/components/sidebar/dnd/treeUtilities.ts
1 participant
Summary
Fixes the category hierarchy drag/drop contract so a visible blue drop-into indicator is the single source of truth for committing a category as a child.
Changes
Test Plan
Notes
Summary by CodeRabbit
Release Notes