Implement document drag and drop3 #178
Merged
Conversation
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
Reviewer's GuideImplements block-level drag-and-drop for editor blocks using Atlaskit pragmatic-drag-and-drop, adds visual drag/drop affordances, and hardens embedded database behavior and scroll handling (including modals, virtualized grids, and panels) to avoid scroll jumps and unwanted selection when creating/opening database views. Sequence diagram for editor block drag-and-drop with Yjs movesequenceDiagram
actor User
participant HoverControls
participant ControlActions
participant useBlockDrag
participant AtlaskitDnD
participant Element as Element_useBlockDrop
participant handleBlockDrop
participant YjsEditor
participant YDoc
User->>HoverControls: Hover over block
HoverControls->>ControlActions: Render toolbar for hoveredBlockId
User->>ControlActions: MouseDown on drag handle
ControlActions->>useBlockDrag: dragHandleRef attached
useBlockDrag->>AtlaskitDnD: draggable(element, getInitialData)
User->>AtlaskitDnD: Start drag gesture
AtlaskitDnD->>useBlockDrag: onDragStart
useBlockDrag->>ControlActions: onDragChange(true)
ControlActions->>HoverControls: isDragging=true (hide toolbar)
AtlaskitDnD->>Element: Hover over block DOM
Element->>useBlockDrop: useBlockDrop({ blockId, element })
useBlockDrop->>AtlaskitDnD: dropTargetForElements(element)
AtlaskitDnD->>useBlockDrop: onDragEnter
useBlockDrop->>Element: isDraggingOver=true, dropEdge=top_or_bottom
Element->>User: Show DropIndicator at top/bottom
User->>AtlaskitDnD: Drop on target block
AtlaskitDnD->>useBlockDrop: onDrop(self, source)
useBlockDrop->>handleBlockDrop: onDrop({ sourceBlockId, targetBlockId, edge })
handleBlockDrop->>YjsEditor: access sharedRoot
handleBlockDrop->>YDoc: executeOperations(sharedRoot, [moveNode], handleBlockDrop, LocalManual)
YDoc-->>YjsEditor: Yjs transaction with CollabOrigin.LocalManual
YjsEditor-->>Element: Slate tree updated (block moved)
AtlaskitDnD->>useBlockDrag: onDrop
useBlockDrag->>ControlActions: onDragChange(false)
ControlActions->>HoverControls: isDragging=false (toolbar visible)
Element->>User: Block appears in new position
Class diagram for new editor block drag-and-drop modulesclassDiagram
class YjsEditor {
}
class useBlockDrag {
+useBlockDrag(props UseBlockDragProps) UseBlockDragResult
}
class UseBlockDragProps {
+string blockId
+string parentId
+RefObject dragHandleRef
+boolean disabled
+function onDragChange
}
class UseBlockDragResult {
+boolean isDragging
+boolean isDraggable
}
class useBlockDrop {
+useBlockDrop(props UseBlockDropProps) UseBlockDropResult
}
class UseBlockDropProps {
+string blockId
+HTMLElement element
+function onDrop
}
class UseBlockDropResult {
+boolean isDraggingOver
+Edge dropEdge
}
class validation {
+boolean canDragBlock(editor YjsEditor, blockId string)
+boolean canDropBlock(editor YjsEditor, sourceBlockId string, targetBlockId string)
+boolean wouldCreateCircularReference(sourceBlock YBlock, targetBlock YBlock, sharedRoot YSharedRoot)
}
class handleBlockDrop {
+boolean handleBlockDrop(editor YjsEditor, sourceBlockId string, targetBlockId string, edge Edge)
}
class executeOperations {
+executeOperations(sharedRoot YSharedRoot, operations function[], operationName string, origin unknown)
}
class CollabOrigin {
<<enum>>
Local
LocalManual
Remote
}
class ElementComponent {
+HTMLElement blockElement
+boolean allowBlockDrop
+boolean isDraggingOver
+Edge dropEdge
}
class ControlActionsComponent {
+string blockId
+string parentId
+boolean isDragging
}
class HoverControlsHooks {
+string hoveredBlockId
+string hoveredBlockParentId
}
%% Relationships
useBlockDrag --> UseBlockDragProps
useBlockDrag --> UseBlockDragResult
useBlockDrop --> UseBlockDropProps
useBlockDrop --> UseBlockDropResult
useBlockDrag ..> YjsEditor : uses
validation ..> YjsEditor : uses
validation ..> YBlock : uses
validation ..> YSharedRoot : uses
handleBlockDrop ..> YjsEditor : uses
handleBlockDrop ..> executeOperations : calls
handleBlockDrop ..> CollabOrigin : uses LocalManual
handleBlockDrop ..> validation : uses wouldCreateCircularReference
ElementComponent ..> useBlockDrop : uses
ElementComponent ..> handleBlockDrop : calls onDrop
ControlActionsComponent ..> useBlockDrag : uses
ControlActionsComponent ..> HoverControlsHooks : uses parentId
HoverControlsHooks ..> YjsEditor : uses
HoverControlsHooks ..> ElementComponent : locates DOM node
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}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
generateDragPreviewthe cloned image handling never runs becauseclonedImagesis queried fromcontainerbeforecloneis appended (so it’s always empty); update this logic to query images from thecloneafter appending it (e.g.const clonedImages = clone.querySelectorAll('img')) so image previews render correctly. - The logic to find the
.appflowy-scroll-containerand save/restorescrollTopis duplicated in multiple places (three slash-panel handlers andDatabaseBlock); consider extracting a small utility or hook (e.g.getScrollContainer/withSavedScroll) to centralize this behavior and avoid inconsistencies. - You’ve implemented
canDropBlock/wouldCreateCircularReferencebutuseBlockDroponly validates the source/target inhandleBlockDrop; consider callingcanDropBlockincanDrop(or before invokingonDrop) to prevent invalid drops earlier and avoid showing drop affordances for disallowed targets.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `generateDragPreview` the cloned image handling never runs because `clonedImages` is queried from `container` before `clone` is appended (so it’s always empty); update this logic to query images from the `clone` after appending it (e.g. `const clonedImages = clone.querySelectorAll('img')`) so image previews render correctly.
- The logic to find the `.appflowy-scroll-container` and save/restore `scrollTop` is duplicated in multiple places (three slash-panel handlers and `DatabaseBlock`); consider extracting a small utility or hook (e.g. `getScrollContainer` / `withSavedScroll`) to centralize this behavior and avoid inconsistencies.
- You’ve implemented `canDropBlock`/`wouldCreateCircularReference` but `useBlockDrop` only validates the source/target in `handleBlockDrop`; consider calling `canDropBlock` in `canDrop` (or before invoking `onDrop`) to prevent invalid drops earlier and avoid showing drop affordances for disallowed targets.
## Individual Comments
### Comment 1
<location> `src/components/editor/components/drag-drop/handleBlockDrop.ts:51-52` </location>
<code_context>
+ }
+
+ // Get the target's parent (source will move to same parent as target)
+ const targetParentId = targetBlock.get(YjsEditorKey.block_parent);
+ const targetParent = getBlock(targetParentId, sharedRoot);
+
+ if (!targetParent) {
</code_context>
<issue_to_address>
**issue:** Guard against missing `block_parent` on the target block to avoid errors when dropping near the root.
`targetBlock` may not always have a `block_parent` (e.g. dropping relative to a root/top-level block), so `targetParentId` could be `undefined` and `getBlock` may throw before the `!targetParent` check.
Consider guarding this case before calling `getBlock`, for example:
```ts
const targetParentId = targetBlock.get(YjsEditorKey.block_parent);
if (!targetParentId) {
console.warn('Target has no parent, skipping drop');
return false;
}
const targetParent = getBlock(targetParentId, sharedRoot);
```
This avoids crashes when dropping near root-level blocks.
</issue_to_address>
### Comment 2
<location> `src/components/editor/components/drag-drop/useBlockDrop.ts:35-42` </location>
<code_context>
+
+ return dropTargetForElements({
+ element: element,
+ canDrop: ({ source }) => {
+ const data = source.data as DragData;
+
+ // Only accept editor blocks
+ if (data.type !== 'editor-block') return false;
+ // Can't drop a block onto itself
+ if (data.blockId === blockId) return false;
+ return true;
+ },
+ getData: ({ input }) => {
</code_context>
<issue_to_address>
**suggestion:** Consider integrating structural validation into `canDrop` to avoid showing drop indicators for invalid moves.
Right now `canDrop` only checks the payload type and `blockId !== targetBlockId`. Structural rules (e.g. circular ancestry, disallowed parents) are only enforced later in `handleBlockDrop` via `wouldCreateCircularReference`, so invalid drops are rejected but still show as valid drop targets.
To avoid misleading feedback, consider reusing the structural validation here (e.g. a `canDropBlock` helper using source/target IDs) so invalid targets never show as drop zones. This doesn’t affect correctness, but it would make drag-and-drop behavior more predictable for users.
Suggested implementation:
```typescript
const [isDraggingOver, setIsDraggingOver] = useState(false);
const [dropEdge, setDropEdge] = useState<Edge | null>(null);
/**
* Centralised structural validation for block drops.
* This currently mirrors the simple checks we perform inline in `canDrop`,
* but should be extended to include structural rules (e.g. ancestry, parent constraints).
*/
const canDropBlock = (sourceData: DragData, targetBlockId: string): boolean => {
// Only accept editor blocks
if (sourceData.type !== 'editor-block') return false;
// Can't drop a block onto itself
if (sourceData.blockId === targetBlockId) return false;
// TODO: Integrate structural validation here so invalid moves never show as drop targets.
// E.g. something like:
// if (wouldCreateCircularReference({ sourceBlockId: sourceData.blockId, targetBlockId })) return false;
// if (!isAllowedParent({ sourceBlockId: sourceData.blockId, targetBlockId })) return false;
return true;
};
```
```typescript
return dropTargetForElements({
element: element,
canDrop: ({ source }) => {
const data = source.data as DragData;
return canDropBlock(data, blockId);
},
getData: ({ input }) => {
```
To fully implement the structural validation as per the review comment, you should:
1. Locate the existing structural validation used during the actual drop handling (likely in the same file, in `handleBlockDrop` or similar) where `wouldCreateCircularReference` and any "allowed parent" checks are currently performed.
2. Extend `canDropBlock` to reuse that logic, for example:
- Inject or access whatever context/state `handleBlockDrop` uses to call `wouldCreateCircularReference` and other structural guards.
- Add those checks inside `canDropBlock` (e.g. `if (wouldCreateCircularReference({ sourceBlockId: sourceData.blockId, targetBlockId })) return false;`).
3. Ensure the types for `DragData` and any helper functions (`wouldCreateCircularReference`, `isAllowedParent`, etc.) are imported into this file if they are defined elsewhere.
4. If you centralize more rules (e.g. allowed edges per block type), consider moving `canDropBlock` to a shared helper module and have both `useBlockDrop` and the drop handler reuse it, keeping validation logic in one place.
</issue_to_address>
### Comment 3
<location> `src/components/editor/components/panels/slash-panel/SlashPanel.tsx:723` </location>
<code_context>
keywords: ['grid', 'table', 'database'],
onClick: async () => {
if (!viewId || !addPage || !openPageModal) return;
+
+ let scrollContainer: Element | null = null;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the repeated scroll-preservation and menu option scrolling logic into small helper functions to simplify the handlers and effect code.
You can reduce a lot of incidental complexity by extracting the repeated scroll-preservation logic and the menu option scrolling into small helpers. That keeps the behavior intact while making each handler much easier to read and maintain.
### 1. Factor out scroll preservation for database blocks
Right now the `grid`, `board`, and `calendar` handlers all duplicate:
- DOM lookup for `.appflowy-scroll-container`
- capturing `scrollTop`
- `setTimeout` restore logic
You can centralize this into two tiny helpers:
```ts
const getScrollContainer = (editor: YjsEditor): HTMLElement | null => {
try {
const domNode = ReactEditor.toDOMNode(editor, editor);
const container = domNode.closest('.appflowy-scroll-container');
if (container instanceof HTMLElement) return container;
} catch {
// ignore
}
const fallback = document.querySelector('.appflowy-scroll-container');
return fallback instanceof HTMLElement ? fallback : null;
};
const withPreservedScroll = async (
editor: YjsEditor,
fn: () => Promise<void>,
): Promise<void> => {
const scrollContainer = getScrollContainer(editor);
const savedScrollTop = scrollContainer?.scrollTop;
await fn();
if (savedScrollTop !== undefined) {
const restoreScroll = () => {
const currentContainer = getScrollContainer(editor);
if (currentContainer) currentContainer.scrollTop = savedScrollTop;
};
setTimeout(restoreScroll, 50);
}
};
```
Then the three handlers become much simpler and share the behavior:
```ts
onClick: async () => {
if (!viewId || !addPage || !openPageModal) return;
try {
await withPreservedScroll(editor, async () => {
const newViewId = await addPage(viewId, {
layout: ViewLayout.Grid, // or Board / Calendar
name: t('document.slashMenu.name.grid'),
});
turnInto(BlockType.GridBlock, {
view_id: newViewId,
parent_id: viewId,
} as DatabaseNodeData);
openPageModal(newViewId);
});
} catch (e: any) {
notify.error(e.message);
}
},
```
This keeps all existing behavior (including the timeout), but removes the duplicated try/catch and selector logic from each handler.
### 2. Extract the option scroll logic into a helper
The custom option scrolling is fine, but putting the math into a small function makes the effect more readable and reusable:
```ts
const scrollChildIntoView = (container: HTMLElement, child: HTMLElement) => {
const elOffsetTop = child.offsetTop;
const elHeight = child.offsetHeight;
const menuScrollTop = container.scrollTop;
const menuHeight = container.clientHeight;
if (elOffsetTop < menuScrollTop) {
container.scrollTop = elOffsetTop;
} else if (elOffsetTop + elHeight > menuScrollTop + menuHeight) {
container.scrollTop = elOffsetTop + elHeight - menuHeight;
}
};
```
Then the `useEffect` becomes:
```ts
useEffect(() => {
selectedOptionRef.current = selectedOption;
if (!selectedOption) return;
const el = optionsRef.current?.querySelector(
`[data-option-key="${selectedOption}"]`,
) as HTMLButtonElement | null;
if (el && optionsRef.current) {
scrollChildIntoView(optionsRef.current, el);
}
}, [selectedOption]);
```
These two extractions should significantly reduce visual noise in an already-long panel file, while preserving all the new scroll behavior.
</issue_to_address>
### Comment 4
<location> `src/components/editor/components/blocks/database/DatabaseBlock.tsx:34` </location>
<code_context>
loadViewMeta: context?.loadViewMeta,
});
+ // Track latest valid scroll position to restore if layout shift resets it
+ const latestScrollTop = useRef<number>(0);
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the scroll container lookup and scroll persistence logic into shared helpers or a hook to keep DatabaseBlock simpler and more focused on its core responsibilities.
You can reduce the added complexity by (1) deduplicating the scroll-container lookup and (2) encapsulating the scroll tracking/restore behavior into a small hook. This keeps `DatabaseBlock` focused on data/collab while preserving the current scroll behavior.
### 1. Deduplicate scroll container lookup
Right now the DOM lookup logic is duplicated in the effect and in `handleRendered`. Extract it into a helper:
```ts
// helpers.ts (or near the component)
const getScrollContainer = (editor: ReactEditor): HTMLElement | null => {
try {
const domNode = ReactEditor.toDOMNode(editor, editor);
const closest = domNode.closest('.appflowy-scroll-container');
if (closest) return closest as HTMLElement;
} catch {
// ignore
}
return document.querySelector('.appflowy-scroll-container') as HTMLElement | null;
};
```
Then reuse:
```ts
useEffect(() => {
const scrollContainer = getScrollContainer(editor);
if (!scrollContainer) return;
if (scrollContainer.scrollTop > 0) {
latestScrollTop.current = scrollContainer.scrollTop;
}
const handleScroll = () => {
if (scrollContainer.scrollTop > 0) {
latestScrollTop.current = scrollContainer.scrollTop;
}
};
scrollContainer.addEventListener('scroll', handleScroll);
return () => scrollContainer.removeEventListener('scroll', handleScroll);
}, [editor]);
const handleRendered = useCallback(() => {
const restore = () => {
try {
const scrollContainer = getScrollContainer(editor);
if (
scrollContainer &&
scrollContainer.scrollTop < 10 &&
latestScrollTop.current > 50
) {
scrollContainer.scrollTop = latestScrollTop.current;
}
} catch {
// ignore
}
};
restore();
setTimeout(restore, 50);
setTimeout(() => {
latestScrollTop.current = 0;
}, 1000);
}, [editor]);
```
### 2. Encapsulate scroll persistence into a small hook
To further isolate the DOM/timing logic, you could move all scroll-related state and effects into a dedicated hook, keeping the same heuristics and timers:
```ts
// useEditorScrollPersistence.ts
export const useEditorScrollPersistence = (editor: ReactEditor) => {
const latestScrollTop = useRef(0);
useEffect(() => {
const scrollContainer = getScrollContainer(editor);
if (!scrollContainer) return;
if (scrollContainer.scrollTop > 0) {
latestScrollTop.current = scrollContainer.scrollTop;
}
const handleScroll = () => {
if (scrollContainer.scrollTop > 0) {
latestScrollTop.current = scrollContainer.scrollTop;
}
};
scrollContainer.addEventListener('scroll', handleScroll);
return () => scrollContainer.removeEventListener('scroll', handleScroll);
}, [editor]);
const restoreScrollIfReset = useCallback(() => {
const restore = () => {
try {
const scrollContainer = getScrollContainer(editor);
if (
scrollContainer &&
scrollContainer.scrollTop < 10 &&
latestScrollTop.current > 50
) {
scrollContainer.scrollTop = latestScrollTop.current;
}
} catch {
// ignore
}
};
restore();
setTimeout(restore, 50);
setTimeout(() => {
latestScrollTop.current = 0;
}, 1000);
}, [editor]);
return { restoreScrollIfReset };
};
```
Then `DatabaseBlock` stays simple:
```ts
const editor = useSlateStatic();
const { restoreScrollIfReset } = useEditorScrollPersistence(editor);
const handleRendered = useCallback(() => {
restoreScrollIfReset();
}, [restoreScrollIfReset]);
// ...
<DatabaseContent
// ...
onRendered={handleRendered}
/>
```
This keeps all existing behavior (DOM lookup, heuristics, double restore, delayed reset) but localizes complexity and makes the block component easier to read and test.
</issue_to_address>
### Comment 5
<location> `src/components/editor/components/drag-drop/useBlockDrag.ts:20` </location>
<code_context>
+/**
+ * Generates a custom drag preview element
+ */
+function generateDragPreview(sourceElement: HTMLElement): HTMLElement {
+ const container = document.createElement('div');
+ const clone = sourceElement.cloneNode(true) as HTMLElement;
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying `generateDragPreview` by removing the per-image canvas logic and tightening the drag-preview integration so the hook only clones/stylizes the block and lets the browser handle ghost rendering.
You can significantly reduce complexity in `generateDragPreview` without changing observable behavior for “ghost preview” dragging, and also fix a likely bug.
### 1. Simplify `generateDragPreview` and remove canvas/image processing
Right now you:
- Clone the source element
- Create a styled container
- Try to draw each `<img>` to a `<canvas>` (with CORS and maintenance concerns)
- Then replace `<img>` with `<canvas>` or reconfigure cloned `<img>`
For a drag ghost, the browser snapshot of a styled DOM clone is usually enough. You can:
- Just clone the block
- Strip transient classes
- Put it into a styled container
- Let images render naturally (no canvas)
This removes a lot of complexity and potential CORS bugs while preserving the “card-like ghost” behavior.
Example replacement (keep the same public behavior of returning a preview element appended to `body`):
```ts
function generateDragPreview(sourceElement: HTMLElement): HTMLElement {
const container = document.createElement('div');
const clone = sourceElement.cloneNode(true) as HTMLElement;
const computedStyle = window.getComputedStyle(sourceElement);
const blockType = sourceElement.getAttribute('data-block-type');
const isImage = blockType === 'image';
let targetWidth = sourceElement.offsetWidth;
if (isImage) {
const img = sourceElement.querySelector('img');
if (img && img.offsetWidth > 0) {
targetWidth = img.offsetWidth;
}
}
// Clean up the clone
clone.classList.remove('block-element--dragging');
clone.style.margin = '0';
clone.style.width = '100%';
clone.style.pointerEvents = 'none';
// Style the container like a card
Object.assign(container.style, {
width: `${targetWidth}px`,
maxWidth: '600px',
maxHeight: isImage ? '1000px' : '150px',
backgroundColor: 'var(--bg-body, #ffffff)',
borderRadius: '8px',
boxShadow: 'var(--shadows-sm, 0 4px 20px rgba(0, 0, 0, 0.1))',
overflow: 'hidden',
position: 'absolute',
top: '-1000px',
left: '-1000px',
zIndex: '9999',
pointerEvents: 'none',
border: '1px solid var(--line-divider, rgba(0, 0, 0, 0.1))',
display: 'block',
fontFamily: computedStyle.fontFamily,
color: computedStyle.color,
lineHeight: computedStyle.lineHeight,
textAlign: computedStyle.textAlign,
direction: computedStyle.direction,
});
container.appendChild(clone);
document.body.appendChild(container);
return container;
}
```
If you truly need special handling for images later, you can move that into a separate optional helper (e.g. `enhancePreviewImages(container, sourceElement)`) so the default flow stays simple.
### 2. Fix the current image handling bug (if you keep it)
If you decide to keep the advanced canvas logic, there is a bug: you call
```ts
const originalImages = sourceElement.querySelectorAll('img');
const clonedImages = container.querySelectorAll('img');
```
before you’ve appended `clone` into `container`, so `clonedImages` will always be empty. At minimum, you should append the clone first:
```ts
container.appendChild(clone);
// now query cloned images
const originalImages = sourceElement.querySelectorAll('img');
const clonedImages = container.querySelectorAll('img');
```
But this still leaves the complexity; the simplification above avoids this entire class of issues.
### 3. Keep drag preview integration minimal
Your `onGenerateDragPreview` can stay as-is, but you can keep it focused and slightly simpler:
```ts
onGenerateDragPreview: ({ nativeSetDragImage }) => {
if (!nativeSetDragImage) return;
try {
const entry = findSlateEntryByBlockId(editor, blockId);
if (!entry) return;
const [node] = entry;
const blockElement = ReactEditor.toDOMNode(editor, node);
if (!blockElement) return;
const preview = generateDragPreview(blockElement);
nativeSetDragImage(preview, 0, 0);
// Cleanup after snapshot
setTimeout(() => {
if (preview.parentNode === document.body) {
document.body.removeChild(preview);
}
}, 0);
} catch (e) {
console.warn('Failed to generate drag preview:', e);
}
},
```
This keeps the hook concerned with drag behavior and leaves layout/styling to `generateDragPreview`, with much less logic overall.
</issue_to_address>
### Comment 6
<location> `cypress/e2e/embeded/database/database-bottom-scroll-simple.cy.ts:67` </location>
<code_context>
const scrollHeight = $container[0].scrollHeight;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {scrollHeight} = $container[0];
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 7
<location> `cypress/e2e/embeded/database/database-bottom-scroll-simple.cy.ts:68` </location>
<code_context>
const clientHeight = $container[0].clientHeight;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {clientHeight} = $container[0];
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 8
<location> `cypress/e2e/embeded/database/database-bottom-scroll.cy.ts:105` </location>
<code_context>
const scrollHeight = $container[0].scrollHeight;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {scrollHeight} = $container[0];
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 9
<location> `cypress/e2e/embeded/database/database-bottom-scroll.cy.ts:106` </location>
<code_context>
const clientHeight = $container[0].clientHeight;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {clientHeight} = $container[0];
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 10
<location> `cypress/e2e/embeded/database/database-bottom-scroll.cy.ts:189` </location>
<code_context>
const scrollHeight = $container[0].scrollHeight;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {scrollHeight} = $container[0];
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>
### Comment 11
<location> `cypress/e2e/embeded/database/database-bottom-scroll.cy.ts:190` </location>
<code_context>
const clientHeight = $container[0].clientHeight;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Prefer object destructuring when accessing and using properties. ([`use-object-destructuring`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/use-object-destructuring))
```suggestion
const {clientHeight} = $container[0];
```
<br/><details><summary>Explanation</summary>Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.
From the [Airbnb Javascript Style Guide](https://airbnb.io/javascript/#destructuring--object)
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+51
to
+52
| const targetParentId = targetBlock.get(YjsEditorKey.block_parent); | ||
| const targetParent = getBlock(targetParentId, sharedRoot); |
There was a problem hiding this comment.
issue: Guard against missing block_parent on the target block to avoid errors when dropping near the root.
targetBlock may not always have a block_parent (e.g. dropping relative to a root/top-level block), so targetParentId could be undefined and getBlock may throw before the !targetParent check.
Consider guarding this case before calling getBlock, for example:
const targetParentId = targetBlock.get(YjsEditorKey.block_parent);
if (!targetParentId) {
console.warn('Target has no parent, skipping drop');
return false;
}
const targetParent = getBlock(targetParentId, sharedRoot);This avoids crashes when dropping near root-level blocks.
Comment on lines
+35
to
+42
| canDrop: ({ source }) => { | ||
| const data = source.data as DragData; | ||
|
|
||
| // Only accept editor blocks | ||
| if (data.type !== 'editor-block') return false; | ||
| // Can't drop a block onto itself | ||
| if (data.blockId === blockId) return false; | ||
| return true; |
There was a problem hiding this comment.
suggestion: Consider integrating structural validation into canDrop to avoid showing drop indicators for invalid moves.
Right now canDrop only checks the payload type and blockId !== targetBlockId. Structural rules (e.g. circular ancestry, disallowed parents) are only enforced later in handleBlockDrop via wouldCreateCircularReference, so invalid drops are rejected but still show as valid drop targets.
To avoid misleading feedback, consider reusing the structural validation here (e.g. a canDropBlock helper using source/target IDs) so invalid targets never show as drop zones. This doesn’t affect correctness, but it would make drag-and-drop behavior more predictable for users.
Suggested implementation:
const [isDraggingOver, setIsDraggingOver] = useState(false);
const [dropEdge, setDropEdge] = useState<Edge | null>(null);
/**
* Centralised structural validation for block drops.
* This currently mirrors the simple checks we perform inline in `canDrop`,
* but should be extended to include structural rules (e.g. ancestry, parent constraints).
*/
const canDropBlock = (sourceData: DragData, targetBlockId: string): boolean => {
// Only accept editor blocks
if (sourceData.type !== 'editor-block') return false;
// Can't drop a block onto itself
if (sourceData.blockId === targetBlockId) return false;
// TODO: Integrate structural validation here so invalid moves never show as drop targets.
// E.g. something like:
// if (wouldCreateCircularReference({ sourceBlockId: sourceData.blockId, targetBlockId })) return false;
// if (!isAllowedParent({ sourceBlockId: sourceData.blockId, targetBlockId })) return false;
return true;
}; return dropTargetForElements({
element: element,
canDrop: ({ source }) => {
const data = source.data as DragData;
return canDropBlock(data, blockId);
},
getData: ({ input }) => {To fully implement the structural validation as per the review comment, you should:
- Locate the existing structural validation used during the actual drop handling (likely in the same file, in
handleBlockDropor similar) wherewouldCreateCircularReferenceand any "allowed parent" checks are currently performed. - Extend
canDropBlockto reuse that logic, for example:- Inject or access whatever context/state
handleBlockDropuses to callwouldCreateCircularReferenceand other structural guards. - Add those checks inside
canDropBlock(e.g.if (wouldCreateCircularReference({ sourceBlockId: sourceData.blockId, targetBlockId })) return false;).
- Inject or access whatever context/state
- Ensure the types for
DragDataand any helper functions (wouldCreateCircularReference,isAllowedParent, etc.) are imported into this file if they are defined elsewhere. - If you centralize more rules (e.g. allowed edges per block type), consider moving
canDropBlockto a shared helper module and have bothuseBlockDropand the drop handler reuse it, keeping validation logic in one place.
appflowy
force-pushed
the
implement_document_drag_and_drop3
branch
from
9e41f0b to
c9f7275
Compare
josue693
pushed a commit
to josue693/AppFlowy-Web
that referenced
this pull request
Dec 21, 2025
* feat: implement drag and drop * chore: fix scroll to top when insert database at the bottom * chore: fix scroll * chore: lint * fix: test * fix: shift + left rigit selection * chore: add test * chore: fix test * test: drag and drop image * chore: selector * chore: update test with selector * fix: test
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
Checklist
General
Testing
Feature-Specific
Summary by Sourcery
Add block-level drag-and-drop for editor content and improve scroll and focus behavior around database blocks and inline panels.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: