Conversation
77391f5 把 useEffect 依赖从 [] 改为完整列表后，任何 useDrag 实例 重渲染时 cleanup 都会重置共享的 dragItem 单例（DragContext）， 干扰正在进行的拖拽操作。 触发链路： 1. 从 Hierarchy 拖实体进入 Assets 面板 → setHighlight(true) → 面板重渲染 2. AssetItem 的 useDrag 有内联 onCancel → 引用变化 → useEffect 重绑 3. cleanup 重置 dragItem.type → isAccepted 失败 → onLeave 不触发 4. 高亮永不消失 修复：cleanup 检查 dragItem.item === item，只有当前实例正在拖拽时 才重置共享状态。dragItem.type 初始值和重置值从 0 恢复为 -1。
📝 WalkthroughWalkthroughThe drag-drop hooks are refactored to improve cleanup logic and nested drag event handling. The dragItem type sentinel shifts from 0 to -1, cleanup now conditionally resets shared state only for matching items, and useDrop uses a useRef counter to properly track nested entry/leave events across renders. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
📚 Storybook Preview🔗 Open Storybook · 🔨 View Build
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ui/src/hooks/useDrag.tsx`:
- Line 19: The drag idle type is incorrectly set to -1 (all bits) causing
bitmask matches to always succeed and the cleanup check compares payloads
(dragItem.item === item) which isn’t instance-safe; update initial/reset
assignments in useDrag (dragItemRef and any resets at the points referenced) to
use 0 for dragItem.type instead of -1, and in useDrop (the check around the
bitmask at useDrop.ts:50) either ensure you gate by dragItem.state ===
DragState.None/Active before evaluating (accept & dragItem.type) or rely on the
corrected 0 idle value; finally, replace the payload equality cleanup guard in
useDrag (the cleanup at the referenced guard) with an instance-specific token/id
stored on the drag context so cleanup only affects the exact drag instance.
- Around line 117-121: The shared drag state reset must use per-instance
ownership instead of comparing payloads: in useDrag create a unique ownership
token (e.g. const owner = Symbol()) for each hook instance, store that token on
the shared dragItem (e.g. dragItem.owner = owner when this instance starts
dragging) and then change the cleanup/reset logic to check ownership
(dragItem.owner === owner) before setting dragItem.state = DragState.None,
dragItem.item = null, dragItem.type = -1; update any start/stop handlers in
useDrag to set/clear dragItem.owner consistently so unrelated instances with
undefined/null/identical item values cannot reset the shared state.
In `@packages/ui/src/hooks/useDrop.ts`:
- Around line 80-82: The dragleave handler currently decrements
counterRef.current unconditionally which can drive it negative and break future
enter/leave transitions; update the logic around the decrement in the function
handling dragleave (the block that modifies counterRef.current and calls
onLeave) to ensure you never underflow — e.g., only decrement if
counterRef.current > 0 or clamp the value after decrement (counterRef.current =
Math.max(0, counterRef.current - 1)), and only call onLeave(e, dragItem.item,
dropElement) when the counter actually reaches 0; keep references to counterRef,
onLeave, dragItem.item and dropElement to locate the exact spot to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 51989c65-aa80-4e59-9337-fa6bc4040e4a
📒 Files selected for processing (2)
packages/ui/src/hooks/useDrag.tsxpackages/ui/src/hooks/useDrop.ts
|
|
||
| export function DragDropContextProvider(props: PropsWithChildren) { | ||
| const dragItemRef = useRef<IDndContextValue>({ item: null, type: 0, state: DragState.None }) | ||
| const dragItemRef = useRef<IDndContextValue>({ item: null, type: -1, state: DragState.None }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify bitmask acceptance and whether state gating exists before matching.
fd useDrop.ts --exec sed -n '1,220p' {}
rg -nP --type=ts --type=tsx -C2 'accept\s*&\s*dragItem\.type|dragItem\.state\s*===\s*DragState\.Dragging'Repository: galacean/editor-ui
Length of output: 3469
🏁 Script executed:
# Retrieve useDrag.tsx to verify line 19, 100, 115-121
fd useDrag.tsx --exec cat -n {}Repository: galacean/editor-ui
Length of output: 4302
🏁 Script executed:
# Check how DragState is defined
rg -A3 'enum DragState|const DragState'Repository: galacean/editor-ui
Length of output: 286
🏁 Script executed:
# Verify if dragItem.state is checked in useDrop or useDrag before bitmask matching
rg -B2 -A2 'dragItem.state.*DragState.Dragging|if.*dragItem.state' packages/ui/src/hooks/Repository: galacean/editor-ui
Length of output: 705
Use 0 instead of -1 for idle dragItem.type to prevent unintended bitmask matches.
Lines 19, 100, and 120 initialize/reset dragItem.type to -1. In bitmask logic, -1 is all bits set (0xFFFFFFFF), so accept & -1 always equals accept, making idle state appear accepted by any non-zero mask. useDrop.ts:50 checks (accept & dragItem.type) with no prior state gate.
Additionally, the cleanup guard at line 117 (if (dragItem.item === item)) is not instance-safe; when item is undefined across multiple useDrag instances, cleanup can interfere with active drags.
Suggested fixes
Replace -1 with 0:
-const dragItemRef = useRef<IDndContextValue>({ item: null, type: -1, state: DragState.None })
+const dragItemRef = useRef<IDndContextValue>({ item: null, type: 0, state: DragState.None })
- dragItem.type = -1
+ dragItem.type = 0
- dragItem.type = -1
+ dragItem.type = 0Or, add an explicit state check before bitmask matching in useDrop:
const isAccepted = (e: DragEvent) => {
- return (accept & dragItem.type) || (acceptFiles && e.dataTransfer?.types.includes('Files'))
+ return (dragItem.state === DragState.Dragging && accept & dragItem.type) || (acceptFiles && e.dataTransfer?.types.includes('Files'))
}For the cleanup guard, consider using an instance token instead of comparing payload values.
Also applies to: 100-100, 120-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/hooks/useDrag.tsx` at line 19, The drag idle type is
incorrectly set to -1 (all bits) causing bitmask matches to always succeed and
the cleanup check compares payloads (dragItem.item === item) which isn’t
instance-safe; update initial/reset assignments in useDrag (dragItemRef and any
resets at the points referenced) to use 0 for dragItem.type instead of -1, and
in useDrop (the check around the bitmask at useDrop.ts:50) either ensure you
gate by dragItem.state === DragState.None/Active before evaluating (accept &
dragItem.type) or rely on the corrected 0 idle value; finally, replace the
payload equality cleanup guard in useDrag (the cleanup at the referenced guard)
with an instance-specific token/id stored on the drag context so cleanup only
affects the exact drag instance.
| if (dragItem.item === item) { | ||
| dragItem.state = DragState.None | ||
| dragItem.item = null | ||
| dragItem.type = -1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect useDrag callsites to verify whether `item` is omitted in options.
rg -nP --type=ts --type=tsx -C3 '\buseDrag\s*(<[^>]+>)?\s*\(\s*\{'Repository: galacean/editor-ui
Length of output: 89
🏁 Script executed:
#!/bin/bash
# First, let's read the useDrag.tsx file to understand the implementation
wc -l packages/ui/src/hooks/useDrag.tsxRepository: galacean/editor-ui
Length of output: 99
🏁 Script executed:
#!/bin/bash
# Read the useDrag.tsx file
cat -n packages/ui/src/hooks/useDrag.tsxRepository: galacean/editor-ui
Length of output: 4302
🏁 Script executed:
#!/bin/bash
# Search for useDrag usage patterns
rg -n "useDrag\s*\(" --type-add 'typescript:*.{ts,tsx}' -t typescript -A 5Repository: galacean/editor-ui
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Alternative search for useDrag calls
fd -e ts -e tsx -exec grep -l "useDrag" {} \;Repository: galacean/editor-ui
Length of output: 253
🏁 Script executed:
#!/bin/bash
# Find all TypeScript/TSX files that use useDrag
fd -e ts -e tsx | xargs grep -l "useDrag" 2>/dev/nullRepository: galacean/editor-ui
Length of output: 258
🏁 Script executed:
#!/bin/bash
# Search for useDrag usage with context
grep -r "useDrag" --include="*.ts" --include="*.tsx" -n -B 2 -A 8Repository: galacean/editor-ui
Length of output: 5704
🏁 Script executed:
#!/bin/bash
# Search specifically for useDrag calls with their parameters
grep -r "useDrag(" --include="*.ts" --include="*.tsx" -B 2 -A 10 | grep -v "export function useDrag\|import.*useDrag"Repository: galacean/editor-ui
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Read useDrop to see how it uses DragContext
cat -n packages/ui/src/hooks/useDrop.tsRepository: galacean/editor-ui
Length of output: 4229
🏁 Script executed:
#!/bin/bash
# Search more broadly for useDrag instantiation
grep -rn "useDrag\s*(" --include="*.ts" --include="*.tsx" | grep -v "export function\|IDragOptions\|useDragOptions"Repository: galacean/editor-ui
Length of output: 44
🏁 Script executed:
#!/bin/bash
# Search in node_modules or external packages for useDrag usage examples
find . -type f \( -name "*.ts" -o -name "*.tsx" \) ! -path "./node_modules/*" -exec grep -l "useDrag" {} \; | head -20Repository: galacean/editor-ui
Length of output: 268
🏁 Script executed:
#!/bin/bash
# Search for imports of useDrag
grep -rn "from.*useDrag\|import.*useDrag" --include="*.ts" --include="*.tsx"Repository: galacean/editor-ui
Length of output: 356
🏁 Script executed:
#!/bin/bash
# Search in editor packages for useDrag usage
find packages -type f \( -name "*.ts" -o -name "*.tsx" \) -exec grep -l "useDrag" {} \; | xargs -I {} sh -c 'echo "=== {} ===" && grep -n "useDrag" {}'Repository: galacean/editor-ui
Length of output: 820
🏁 Script executed:
#!/bin/bash
# Let's check if there are any test files that show useDrag usage
find . -type f \( -name "*.test.ts" -o -name "*.test.tsx" -o -name "*.spec.ts" -o -name "*.spec.tsx" \) | head -10Repository: galacean/editor-ui
Length of output: 1120
🏁 Script executed:
#!/bin/bash
# Check package.json to understand the package structure
cat packages/ui/package.json | head -30Repository: galacean/editor-ui
Length of output: 853
🏁 Script executed:
#!/bin/bash
# Search for actual useDrag usage in packages (not ui package itself)
find packages -type f \( -name "*.ts" -o -name "*.tsx" \) ! -path "*/ui/src/hooks/*" -exec grep -l "useDrag" {} \;Repository: galacean/editor-ui
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Search in src directory more broadly
find . -path ./node_modules -prune -o -type f \( -name "*.ts" -o -name "*.tsx" \) -print | xargs grep -l "useDrag" 2>/dev/null | grep -v node_modulesRepository: galacean/editor-ui
Length of output: 268
🏁 Script executed:
#!/bin/bash
# Let's look for any references mentioning AssetItem which was mentioned in the comment
grep -rn "AssetItem" --include="*.ts" --include="*.tsx" --include="*.js" --include="*.jsx"Repository: galacean/editor-ui
Length of output: 3001
🏁 Script executed:
#!/bin/bash
# Check the git history or comments for context
grep -rn "item === item\|payload.*collid\|ownership" --include="*.ts" --include="*.tsx"Repository: galacean/editor-ui
Length of output: 3018
🏁 Script executed:
#!/bin/bash
# Let's look at AssetItem to see how it uses useDrag
cat -n packages/ui/src/AssetItem/AssetItem.tsxRepository: galacean/editor-ui
Length of output: 3139
🏁 Script executed:
#!/bin/bash
# Check if there are any useDrag usages in editor packages beyond the ui package export
grep -rn "useDrag" packages --include="*.ts" --include="*.tsx" | grep -v "export\|import" | grep -v "^packages/ui"Repository: galacean/editor-ui
Length of output: 177
🏁 Script executed:
#!/bin/bash
# Let's verify the concern more directly by checking dependency scenarios
# Look for scenarios where multiple useDrag instances could coexist
# Check if DragDropContextProvider is used somewhere
grep -rn "DragDropContextProvider" packages --include="*.ts" --include="*.tsx"Repository: galacean/editor-ui
Length of output: 273
🏁 Script executed:
#!/antml:
# Confirm the theoretical collision scenario by examining the shared context
cat -n packages/ui/src/hooks/useDrag.tsx | sed -n '15,25p'Repository: galacean/editor-ui
Length of output: 526
Shared drag state can be incorrectly reset by unrelated hook instances with colliding item values.
Line 117's check dragItem.item === item cannot distinguish between multiple useDrag instances when item is undefined, null, or identical values. Since item is optional (line 32) and dragItem is a single shared ref across all hooks in the tree (line 19), a cleanup in one hook can reset an active drag in another. Use a per-instance ownership token (Symbol) instead of payload equality to ensure only the owning instance resets the shared state.
Suggested fix (instance token ownership)
interface IDndContextValue<T = any> {
type: number
item: T
state: DragState
+ ownerId?: symbol
}
export function useDrag<T>(options: IDragOptions<T>): IDragReturnType {
+ const ownerIdRef = useRef(Symbol('drag-owner'))
const dragRef = useRef<HTMLDivElement>(null)
const previewRef = useRef<HTMLDivElement>(null)
const { type, item, onEnd, onStart, onCancel, disable = false } = options
@@
dragItem.state = DragState.Dragging
dragItem.type = type
dragItem.item = item
+ dragItem.ownerId = ownerIdRef.current
@@
dragItem.state = DragState.None
dragItem.item = null
dragItem.type = -1
+ dragItem.ownerId = undefined
@@
- if (dragItem.item === item) {
+ if (dragItem.ownerId === ownerIdRef.current) {
dragItem.state = DragState.None
dragItem.item = null
dragItem.type = -1
+ dragItem.ownerId = undefined
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/hooks/useDrag.tsx` around lines 117 - 121, The shared drag
state reset must use per-instance ownership instead of comparing payloads: in
useDrag create a unique ownership token (e.g. const owner = Symbol()) for each
hook instance, store that token on the shared dragItem (e.g. dragItem.owner =
owner when this instance starts dragging) and then change the cleanup/reset
logic to check ownership (dragItem.owner === owner) before setting
dragItem.state = DragState.None, dragItem.item = null, dragItem.type = -1;
update any start/stop handlers in useDrag to set/clear dragItem.owner
consistently so unrelated instances with undefined/null/identical item values
cannot reset the shared state.
| counterRef.current-- | ||
| if (counterRef.current === 0 && onLeave) { | ||
| onLeave(e, dragItem.item, dropElement) |
There was a problem hiding this comment.
Prevent dragleave counter underflow.
Line 80 decrements unconditionally. Browser drag events can be noisy; if the counter goes negative, next enters/leaves won’t transition correctly, and consumers like packages/ui/src/Tree/InsertionIndicator.tsx:58-71 can keep stale highlight state.
🔧 Proposed fix
- counterRef.current--
- if (counterRef.current === 0 && onLeave) {
+ counterRef.current = Math.max(0, counterRef.current - 1)
+ if (counterRef.current === 0 && onLeave) {
onLeave(e, dragItem.item, dropElement)
}📝 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.
| counterRef.current-- | |
| if (counterRef.current === 0 && onLeave) { | |
| onLeave(e, dragItem.item, dropElement) | |
| counterRef.current = Math.max(0, counterRef.current - 1) | |
| if (counterRef.current === 0 && onLeave) { | |
| onLeave(e, dragItem.item, dropElement) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ui/src/hooks/useDrop.ts` around lines 80 - 82, The dragleave handler
currently decrements counterRef.current unconditionally which can drive it
negative and break future enter/leave transitions; update the logic around the
decrement in the function handling dragleave (the block that modifies
counterRef.current and calls onLeave) to ensure you never underflow — e.g., only
decrement if counterRef.current > 0 or clamp the value after decrement
(counterRef.current = Math.max(0, counterRef.current - 1)), and only call
onLeave(e, dragItem.item, dropElement) when the counter actually reaches 0; keep
references to counterRef, onLeave, dragItem.item and dropElement to locate the
exact spot to change.
1 participant
Summary
useDragcleanup 增加dragItem.item === item守卫，只有当前实例正在拖拽时才重置共享dragItemdragItem.type初始值和重置值从0恢复为-1根因
77391f5把useEffect依赖从[]改为完整列表后，任何useDrag实例重渲染时 cleanup 都会重置共享的dragItem单例（DragContext）。具体触发链路：
setHighlight(true)→ 面板重渲染useDrag有内联onCancel回调 → 引用变化 →useEffect重绑dragItem.type = 0→isAccepted失败 →onLeave不触发Test plan
Summary by CodeRabbit
Bug Fixes