🐛(frontend) fix DnD hover flash and enable drop on pinned documents#2531
Open
magopian wants to merge 2 commits into
Open
🐛(frontend) fix DnD hover flash and enable drop on pinned documents#2531magopian wants to merge 2 commits into
magopian wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughAdds a shared document drag-and-drop provider, moves grid drag behavior into that provider, and enables pinned favorites as drop targets. Drag-state styling now suppresses hover flashes and displays drop hints. Confirmation handling remains for shared documents. Unit tests cover provider, hook, grid, and favorite behavior, while an end-to-end test verifies nesting onto a pinned document. Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@src/frontend/apps/impress/src/features/docs/DocDndContext.tsx`:
- Around line 169-177: Update the overlayText useMemo in DocDndContext so the
administrator error is shown only when canDrop is explicitly false, not when it
is undefined. Preserve the existing canDrag check and selected document title
fallback, allowing unknown drop-target states to use the normal overlay text.
- Around line 79-90: Update the drop handling around handleMove in DocDndContext
to normalize sourceDocumentId and targetDocumentId by removing the favorite-
prefix before comparing them. Reject the drop and clear onDragData.current when
the normalized IDs match, preventing a document from becoming its own parent
while preserving the existing validation and move flow for distinct documents.
- Around line 199-204: Add an onDragCancel handler to the DndContext in
DocDndContext, reusing the existing handleDragEnd cleanup logic so cancellation
via Escape resets the body cursor/class and selectedDoc. Keep the current
drag-end behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1b17a5be-87a2-49a5-af29-f3ed5bc11dcf
📒 Files selected for processing (11)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.tssrc/frontend/apps/impress/src/features/docs/DocDndContext.tsxsrc/frontend/apps/impress/src/features/docs/__tests__/DocDndContext.test.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/hooks/__tests__/useDragAndDrop.test.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/hooks/useDragAndDrop.tsxsrc/frontend/apps/impress/src/features/left-panel/components/LefPanelTargetFilters.tsxsrc/frontend/apps/impress/src/features/left-panel/components/LeftPanelFavorites.tsxsrc/frontend/apps/impress/src/features/left-panel/components/__tests__/LeftPanelFavorites.test.tsxsrc/frontend/apps/impress/src/pages/docs/index.tsx
magopian
force-pushed
the
fix/nesting-document-via-drag-and-drop
branch
from
July 16, 2026 17:39
7649824 to
33add4f
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/apps/impress/src/features/docs/docs-grid/hooks/useDragAndDrop.tsx (1)
55-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAuthorize the drop from the drag-end event, not asynchronously updated state.
canDropis set by droppable effects. A user releasing immediately after entering a valid target can therefore reachhandleDragEndwith stale or undefined state and lose the drop. Validateactive.data.currentandover.data.currentdirectly.Proposed fix
const handleDragEnd = (e: DragEndEvent) => { + const { active, over } = e; + const source = active.data.current as Doc | undefined; + const target = over?.data.current as Doc | undefined; resetDrag(); - if (!canDrag || !canDrop) { + + if (source?.user_role !== Role.OWNER || !target?.abilities.move) { return; } - const { active, over } = e; - if (!over?.id || active.id === over.id) { return; } onDrag({ sourceDocumentId: active.id as string, targetDocumentId: over.id as string, - source: active.data.current as Doc, - target: over.data.current as Doc, + source, + target, });🤖 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/frontend/apps/impress/src/features/docs/docs-grid/hooks/useDragAndDrop.tsx` around lines 55 - 72, Update handleDragEnd to authorize the drop using active.data.current and over.data.current from the DragEndEvent instead of the asynchronously maintained canDrop state. Validate both data objects before calling onDrag, while preserving the existing canDrag guard, self-drop check, and document ID/source/target mapping.
🤖 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.
Inline comments:
In
`@src/frontend/apps/impress/src/features/docs/__tests__/DocDndContext.test.tsx`:
- Around line 255-274: Update the regression test around capturedOnDrag to make
the droppable target ID include the favorite- prefix while sourceDocumentId
remains the underlying document ID. Keep the self-drop setup and zero-request
assertion, ensuring the test fails if favorite-prefix normalization is removed.
In `@src/frontend/apps/impress/src/features/docs/DocDndContext.tsx`:
- Around line 164-167: Update the onDragCancel handler so its announcement
states only that dragging was cancelled and does not say the document was
dropped; preserve the active document identifier in the message if appropriate.
- Around line 75-98: Update handleMoveDoc so its finally block clears
onDragData.current only if it still references the same drag data captured when
the move began. Preserve newer pending drag data and its confirmation state when
an earlier handleMove request settles.
---
Outside diff comments:
In
`@src/frontend/apps/impress/src/features/docs/docs-grid/hooks/useDragAndDrop.tsx`:
- Around line 55-72: Update handleDragEnd to authorize the drop using
active.data.current and over.data.current from the DragEndEvent instead of the
asynchronously maintained canDrop state. Validate both data objects before
calling onDrag, while preserving the existing canDrag guard, self-drop check,
and document ID/source/target mapping.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 194b36d8-adc8-46b6-999b-3cf316384895
📒 Files selected for processing (12)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/doc-grid-move.spec.tssrc/frontend/apps/impress/src/features/docs/DocDndContext.tsxsrc/frontend/apps/impress/src/features/docs/__tests__/DocDndContext.test.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/DocGridContentList.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/components/__tests__/DraggableDocGridItem.test.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/hooks/__tests__/useDragAndDrop.test.tsxsrc/frontend/apps/impress/src/features/docs/docs-grid/hooks/useDragAndDrop.tsxsrc/frontend/apps/impress/src/features/left-panel/components/LefPanelTargetFilters.tsxsrc/frontend/apps/impress/src/features/left-panel/components/LeftPanelFavorites.tsxsrc/frontend/apps/impress/src/features/left-panel/components/__tests__/LeftPanelFavorites.test.tsxsrc/frontend/apps/impress/src/pages/docs/index.tsx
The DnD context was scoped to MainContent, excluding LeftPanel from participating as a drop target. Favorites also shared the same droppable ID as grid items, causing dnd-kit to overwrite the favorites registration and silently ignore drops on them. Navigation links and favorites showed misleading hover styles during drag because pointer events fire even while dragging. - Lift DndContext into a new DocDndProvider wrapping the full page layout so LeftPanel is inside the DnD boundary - Prefix favorite droppable IDs with `favorite-` to avoid collision with grid droppable IDs in dnd-kit's internal map - Use `target.id` (from over.data.current) instead of `over.id` for the move API call so the prefixed ID doesn't leak into the request - Add `is-dnd-dragging` body class on drag start/end and suppress :hover styles in LeftPanelTargetFilters and LeftPanelFavorites during drag - Add unit tests for useDragAndDrop, DocDndProvider, and LeftPanelFavoriteItem; add e2e test for drag-to-favorites Signed-off-by: Mathieu Agopian <mathieu@agopian.info>
Signed-off-by: Mathieu Agopian <mathieu@agopian.info>
magopian
force-pushed
the
fix/nesting-document-via-drag-and-drop
branch
from
July 16, 2026 18:41
33add4f to
ffeebb0
Compare
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.
Fixes #1191
Purpose
When dragging a document over the left sidebar, over the pinned documents, there was some flashing hover background confusing the user into believing they could drop the document there.
The drop was not possible in the left sidebar on pinned documents.
This PR aims at enabling this drag and drop to nest documents (dragging them from the grid on the right to a pinned document on the left), and removing the "hover flashes".
Proposal
The DnD context was scoped to MainContent, excluding LeftPanel from participating as a drop target.
Favorites also shared the same droppable ID as grid items, causing dnd-kit to overwrite the favorites registration and silently ignore drops on them. Navigation links and favorites showed misleading hover styles during drag because pointer events fire even while dragging.
favorite-to avoid collision with grid droppable IDs in dnd-kit's internal maptarget.id(from over.data.current) instead ofover.idfor the move API call so the prefixed ID doesn't leak into the requestis-dnd-draggingbody class on drag start/end and suppress :hover styles in LeftPanelTargetFilters and LeftPanelFavorites during dragExternal contributions
Thank you for your contribution! 🎉
Please ensure the following items are checked before submitting your pull request:
General requirements
Skip the checkbox below 👇 if you're fixing an issue or adding documentation
CI requirements
git commit --signoff(DCO compliance)git commit -S)<gitmoji>(type) title description## [Unreleased]section (if noticeable change)AI requirements
Skip the checkboxes below 👇 If you didn't use AI for your contribution