Implement document drag and drop #176
Closed
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 in the editor, stabilizes scroll behavior for embedded databases and panels, simplifies publish/share flows and subscription checks, and refactors various UI components and Storybook/dev tooling for cleaner behavior and logging. Sequence diagram for block drag-and-drop reordering in the editorsequenceDiagram
actor User
participant HoverControls
participant ControlActions
participant useBlockDrag
participant AtlaskitDnd as Atlaskit_DnD
participant Element
participant useBlockDrop
participant handleBlockDrop
participant executeOperations
participant YDoc
User->>HoverControls: Hover over block
HoverControls->>ControlActions: Render with blockId parentId
Note over ControlActions,useBlockDrag: Drag handle is wired via useBlockDrag
User->>ControlActions: MouseDown on dragHandle
ControlActions->>useBlockDrag: Start drag
useBlockDrag->>AtlaskitDnd: draggable(element, getInitialData)
AtlaskitDnd-->>useBlockDrag: onDragStart
useBlockDrag->>ControlActions: onDragChange(true)
ControlActions->>HoverControls: isDragging=true (hide hover UI)
Note over Element,useBlockDrop: Each block wrapper registers dropTarget via useBlockDrop
AtlaskitDnd->>useBlockDrop: onDragEnter(sourceData)
useBlockDrop->>useBlockDrop: validate type editor-block
useBlockDrop->>useBlockDrop: compute closest edge (top bottom)
useBlockDrop->>Element: isDraggingOver=true dropEdge=edge
Element->>Element: Render DropIndicator at edge
AtlaskitDnd->>useBlockDrop: onDrop(sourceData)
useBlockDrop->>handleBlockDrop: onDrop(sourceBlockId targetBlockId edge)
handleBlockDrop->>YDoc: getBlock(sourceBlockId)
handleBlockDrop->>YDoc: getBlock(targetBlockId)
handleBlockDrop->>YDoc: getBlock(targetParentId)
handleBlockDrop->>YDoc: getBlockIndex(targetBlockId)
handleBlockDrop->>handleBlockDrop: compute newIndex from edge
handleBlockDrop->>executeOperations: operations=[moveNode] origin=CollabOrigin_LocalManual
executeOperations->>YDoc: doc.transact(operations origin)
YDoc-->>executeOperations: transaction applied
executeOperations-->>handleBlockDrop: success
handleBlockDrop-->>useBlockDrop: true
AtlaskitDnd-->>useBlockDrag: onDrop
useBlockDrag->>ControlActions: onDragChange(false)
ControlActions->>HoverControls: isDragging=false
Note over HoverControls,Element: Slate and Yjs propagate new document state to re-render blocks
Class diagram for editor block drag-and-drop componentsclassDiagram
class YjsEditor {
+sharedRoot YSharedRoot
}
class useBlockDrag {
+isDragging boolean
+isDraggable boolean
}
class UseBlockDragProps {
+blockId string
+parentId string
+dragHandleRef RefObject
+disabled boolean
+onDragChange function
}
class useBlockDrop {
+isDraggingOver boolean
+dropEdge Edge
}
class UseBlockDropProps {
+blockId string
+element HTMLElement
+onDrop function
}
class handleBlockDrop {
+handleBlockDrop(editor YjsEditor, sourceBlockId string, targetBlockId string, edge Edge) boolean
}
class validation {
+canDragBlock(editor YjsEditor, blockId string) boolean
+canDropBlock(editor YjsEditor, sourceBlockId string, targetBlockId string) boolean
+wouldCreateCircularReference(sourceBlock YBlock, targetBlock YBlock, sharedRoot YSharedRoot) boolean
}
class HoverControlsHooks {
+hoveredBlockId string
+hoveredBlockParentId string
+ref HTMLDivElement
+cssProperty string
}
class HoverControls {
-openMenu boolean
-isDragging boolean
}
class ControlActions {
-blockId string
-parentId string
-dragHandleRef HTMLButtonElement
-setOpenMenu function
-onDraggingChange function
}
class ElementComponent {
-blockElement HTMLElement
-allowBlockDrop boolean
-isDraggingOver boolean
-dropEdge Edge
}
class executeOperations {
+executeOperations(sharedRoot YSharedRoot, operations function[], operationName string, origin unknown) void
}
YjsEditor --> executeOperations : uses
YjsEditor --> validation : uses
HoverControls ..> HoverControlsHooks : uses hook
HoverControls ..> ControlActions : renders
HoverControls ..> useBlockDrag : indirect via ControlActions
ControlActions --> UseBlockDragProps : config
ControlActions ..> useBlockDrag : calls hook
useBlockDrag --> YjsEditor : reads
useBlockDrag ..> validation : calls canDragBlock
ElementComponent ..> useBlockDrop : calls hook
ElementComponent --> UseBlockDropProps : config
useBlockDrop ..> handleBlockDrop : calls onDrop
handleBlockDrop ..> executeOperations : calls
handleBlockDrop ..> validation : calls wouldCreateCircularReference
validation --> YjsEditor : reads
class DropIndicator {
+edge Edge
+gap string
}
ElementComponent ..> DropIndicator : renders when isDraggingOver
Flow diagram for handling a block drop operationflowchart TD
A[Block drop event
sourceBlockId
targetBlockId
edge] --> B[Check editor.sharedRoot]
B -->|missing| Z[Return false]
B -->|exists| C[Get sourceBlock from YDoc]
C --> D[Get targetBlock from YDoc]
D --> E[Get targetParent from YDoc]
E -->|missing| Z
E --> F[Check circular reference
wouldCreateCircularReference]
F -->|true| Z
F -->|false| G[Get targetIndex
getBlockIndex]
G --> H[Compute newIndex
edge top => targetIndex
edge bottom => targetIndex+1]
H --> I[executeOperations
origin=CollabOrigin_LocalManual]
I --> J[moveNode in YDoc
sourceBlock -> targetParent at newIndex]
J --> K[Transaction commits
Yjs notifies Slate]
K --> L[Editor re-renders blocks
with new order]
L --> M[Return true]
Z --> N[Abort drop
no document change]
Flow diagram for embedded database scroll restorationflowchart TD
A[DatabaseBlock mount] --> B[Find scrollContainer
via ReactEditor.toDOMNode
or document.querySelector]
B -->|not found| X[Do nothing]
B -->|found| C[Initialize latestScrollTop
from scrollTop]
C --> D[Listen to scroll events]
D --> E[On scroll
if scrollTop>0
update latestScrollTop]
subgraph RenderCycle
F[DatabaseContent renders
Grid or Calendar]
F --> G[Child view calls onRendered]
G --> H[handleRendered]
H --> I[restore callback]
I --> J[Locate scrollContainer again]
J --> K{scrollTop<10
and latestScrollTop>50}
K -->|yes| L[Set scrollTop to latestScrollTop]
K -->|no| M[Skip restore]
L --> N[Schedule second restore
setTimeout 50ms]
N --> O[After 1000ms
reset latestScrollTop to 0]
end
E --> RenderCycle
X --> RenderCycle
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 and found some issues that need to be addressed.
- In
useBlockDrag.generateDragPreview, theoriginalImages/clonedImagesmapping probably never runs as intended because you querycontainer.querySelectorAll('img')beforecloneis appended tocontainer; the image lookup should be done on the cloned subtree (e.g.clone.querySelectorAll('img')) after it has been attached. - The
isOfficialHostimplementation was simplified to only inspectwindow.location.hostname(and Storybook’s mock) and now explicitly excludes localhost/127.0.0.1; this changes subscription gating semantics for self‑hosted and local environments and may cause unintended calls togetSubscriptionswhere they were previously short‑circuited—worth double‑checking that this behavior change is desired. - The SSR server logging in
deploy/server.tsnow always usespino-prettywith a local file destination and hard‑codedlevel: 'info', ignoringLOG_LEVELand typical production best practices (JSON to stdout/stderr); consider whether you want pretty‑printed logs and file writes in production or if this should remain environment‑dependent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `useBlockDrag.generateDragPreview`, the `originalImages` / `clonedImages` mapping probably never runs as intended because you query `container.querySelectorAll('img')` before `clone` is appended to `container`; the image lookup should be done on the cloned subtree (e.g. `clone.querySelectorAll('img')`) after it has been attached.
- The `isOfficialHost` implementation was simplified to only inspect `window.location.hostname` (and Storybook’s mock) and now explicitly excludes localhost/127.0.0.1; this changes subscription gating semantics for self‑hosted and local environments and may cause unintended calls to `getSubscriptions` where they were previously short‑circuited—worth double‑checking that this behavior change is desired.
- The SSR server logging in `deploy/server.ts` now always uses `pino-pretty` with a local file destination and hard‑coded `level: 'info'`, ignoring `LOG_LEVEL` and typical production best practices (JSON to stdout/stderr); consider whether you want pretty‑printed logs and file writes in production or if this should remain environment‑dependent.
## Individual Comments
### Comment 1
<location> `src/components/_shared/mobile-topbar/MobileTopBar.tsx:53` </location>
<code_context>
>
<MobileDrawer
- swipeAreaWidth={folderDrawerWidth}
+ swipeAreaWidth={window.innerWidth - 56}
onOpen={handleOpenFolder}
onClose={handleCloseFolder}
</code_context>
<issue_to_address>
**issue (bug_risk):** Accessing `window.innerWidth` directly in render can break SSR and initial render in non-browser environments.
This used to be guarded (e.g. via `useMemo` and `typeof window === 'undefined'`) to avoid accessing `window` during SSR or in tests. Please reintroduce a guard and compute `swipeAreaWidth` outside of render (e.g. memo/layout effect) with a safe fallback when `window` is unavailable.
</issue_to_address>
### Comment 2
<location> `src/components/editor/components/drag-drop/useBlockDrag.ts:68-70` </location>
<code_context>
+ direction: computedStyle.direction,
+ });
+
+ // Explicitly handle images to ensure they render correctly in the ghost
+ const originalImages = sourceElement.querySelectorAll('img');
+ const clonedImages = container.querySelectorAll('img');
+
+ originalImages.forEach((orig, index) => {
</code_context>
<issue_to_address>
**issue (bug_risk):** Image-specific drag-preview logic never runs because you query cloned images before appending the clone to the container.
Here `container` has no children yet, so `container.querySelectorAll('img')` returns an empty NodeList and the image-handling loop never runs. To make the image logic apply to the drag preview, either:
- Query on `clone` instead: `const clonedImages = clone.querySelectorAll('img');`, or
- Append the clone first, then query: `container.appendChild(clone); const clonedImages = container.querySelectorAll('img');`.
</issue_to_address>
### Comment 3
<location> `src/components/editor/components/blocks/database/DatabaseBlock.tsx:41` </location>
<code_context>
+ let scrollContainer: HTMLElement | null = null;
+
+ try {
+ const domNode = ReactEditor.toDOMNode(editor, editor);
+
+ scrollContainer = domNode.closest('.appflowy-scroll-container');
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `ReactEditor.toDOMNode(editor, editor)` is likely incorrect and may throw at runtime.
`toDOMNode` expects `(editor, SlateNode)`, so passing `editor` as the node is invalid and may throw (e.g., when asserting `Element.isElement(node)`). This pattern appears here and in the slash-panel DB creation handlers.
If you just need the editor root element, either use a known Slate node (e.g. `editor.children[0]`) or reuse an existing ref to the editor DOM element. It may also be worth centralizing the logic for finding the `.appflowy-scroll-container` for an editor to avoid duplicating this fragile lookup across components.
</issue_to_address>
### Comment 4
<location> `src/components/editor/components/panels/slash-panel/SlashPanel.tsx:728` </location>
<code_context>
+ let scrollContainer: HTMLElement | null = null;
+
+ try {
+ const domNode = ReactEditor.toDOMNode(editor, editor);
+
+ scrollContainer = domNode.closest('.appflowy-scroll-container');
</code_context>
<issue_to_address>
**issue (bug_risk):** The slash-panel scroll preservation logic also uses `toDOMNode(editor, editor)`, which is likely an invalid node reference.
As with `DatabaseBlock`, `ReactEditor.toDOMNode` should receive a Slate node, not the editor instance. If this throws, the logic silently falls back to `document.querySelector('.appflowy-scroll-container')`, which is less precise and may break with multiple editors.
Recommend using a valid Slate node or a dedicated ref to the editor container, and sharing a common helper so this scroll-preservation logic is implemented safely in both places.
</issue_to_address>
### Comment 5
<location> `src/utils/subscription.ts:12-14` </location>
<code_context>
+export function InvalidLink() {
const { t } = useTranslation();
return (
<LandingPage
Logo={InvalidLinkLogo}
</code_context>
<issue_to_address>
**suggestion:** The implementation no longer matches the doc comment and has dead commented-out localhost handling.
The header says `localhost` is an official host for development, but the code now only allows `beta.appflowy.cloud` and `test.appflowy.cloud`, with the localhost checks commented out. Please either update/remove the comment and dead code if localhost is no longer supported, or reinstate the localhost checks to match the documented behavior.
</issue_to_address>
### Comment 6
<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-container, selection, and scroll-into-view logic into small helper utilities to keep SlashPanel handlers linear and focused on orchestration rather than DOM details.
You can reduce the added complexity by extracting the repeated DOM/selection logic into small helpers. That keeps the handlers linear and centralizes the imperative bits.
### 1) Factor out scroll-container discovery + restoration
The Grid/Board/Calendar handlers repeat the same scroll-container lookup and restore logic. A small utility keeps this behavior consistent and easier to adjust later:
```ts
// utils/scroll.ts
import { ReactEditor } from 'slate-react';
import type { Editor } from 'slate';
export function getScrollContainer(editor: Editor): Element | null {
try {
const domNode = ReactEditor.toDOMNode(editor as any, editor);
const fromEditor = domNode.closest('.appflowy-scroll-container');
if (fromEditor) return fromEditor;
} catch {
// ignore
}
return document.querySelector('.appflowy-scroll-container');
}
export function preserveScrollAround<T>(
editor: Editor,
fn: () => Promise<T> | T,
): Promise<T> | T {
const container = getScrollContainer(editor);
const savedScrollTop = container?.scrollTop;
const run = async () => {
const result = await fn();
if (savedScrollTop !== undefined) {
setTimeout(() => {
const current = document.querySelector('.appflowy-scroll-container');
if (current) current.scrollTop = savedScrollTop;
}, 50);
}
return result;
};
return run();
}
```
Usage in each `onClick` becomes much simpler:
```ts
onClick: async () => {
if (!viewId || !addPage || !openPageModal) return;
await preserveScrollAround(editor, async () => {
const newViewId = await addPage(viewId, {
layout: ViewLayout.Grid,
name: t('document.slashMenu.name.grid'),
});
turnInto(BlockType.GridBlock, {
view_id: newViewId,
parent_id: viewId,
} as DatabaseNodeData);
openPageModal(newViewId);
});
}
```
Same pattern for Board/Calendar — only the `layout` and `BlockType` differ.
---
### 2) Extract selection logic for new embed blocks
The embed selection logic now has an inline special case for database blocks. Pulling that into a helper keeps the main flow linear:
```ts
// utils/selection.ts
import { Transforms } from 'slate';
import type { Editor } from 'slate';
import { BlockType } from '@/application/slate-yjs';
import { findSlateEntryByBlockId } from '@/application/slate-yjs/utils/editor';
const DATABASE_BLOCK_TYPES = [
BlockType.GridBlock,
BlockType.BoardBlock,
BlockType.CalendarBlock,
];
export function positionSelectionForNewBlock(
editor: Editor,
type: BlockType,
newBlockId: string,
) {
const isDatabaseBlock = DATABASE_BLOCK_TYPES.includes(type);
if (isDatabaseBlock) {
// Database blocks open a modal and should not scroll the editor
Transforms.deselect(editor);
return;
}
const entry = findSlateEntryByBlockId(editor, newBlockId);
if (!entry) return;
const [, path] = entry;
editor.select(editor.start(path));
}
```
Call site becomes:
```ts
if (newBlockId && isEmbedBlockTypes(type)) {
positionSelectionForNewBlock(editor, type, newBlockId);
}
```
---
### 3) Extract custom `scrollIntoView` math
The manual scroll calculation inside the effect can be moved into a reusable utility, clarifying intent and reducing inline math:
```ts
// utils/dom.ts
export function 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;
}
}
```
Effect 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 extractions preserve all current behavior but reduce duplication and keep `SlashPanel` focused on orchestrating behavior rather than embedding DOM/scroll/selection details inline.
</issue_to_address>
### Comment 7
<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 restoration logic into shared helpers or a hook and replacing magic numbers with named constants to make the behavior clearer and less repetitive.
You can keep the new behavior but reduce complexity and duplication by extracting the scroll logic into small, focused helpers/hooks and by lifting magic numbers into named constants.
### 1. Deduplicate scroll-container lookup
Right now the DOM lookup is duplicated in both the `useEffect` and `restore` function. Extract it to a tiny helper:
```ts
// utils/scroll.ts
import { ReactEditor } from 'slate-react';
export function getScrollContainer(editor: ReactEditor): HTMLElement | null {
try {
const domNode = ReactEditor.toDOMNode(editor, editor);
const container = domNode.closest('.appflowy-scroll-container') as HTMLElement | null;
if (container) return container;
} catch {
// ignore
}
return document.querySelector('.appflowy-scroll-container');
}
```
Then both places just call this:
```ts
// in DatabaseBlock
import { getScrollContainer } from '@/utils/scroll';
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 < SCROLL_RESET_THRESHOLD &&
latestScrollTop.current > SCROLL_RESTORE_MIN_SCROLL
) {
scrollContainer.scrollTop = latestScrollTop.current;
}
} catch {
// ignore
}
};
restore();
setTimeout(restore, SCROLL_RETRY_DELAY_MS);
setTimeout(() => {
latestScrollTop.current = 0;
}, SCROLL_CLEAR_DELAY_MS);
}, [editor]);
```
### 2. Replace magic numbers with named constants
The thresholds and timeouts are currently “magic” and make the behavior harder to reason about. Small named constants make intent clear and keep behavior exactly the same:
```ts
// utils/scroll.ts or local to the component
const SCROLL_RESET_THRESHOLD = 10; // "close to top" ⇒ treat as reset
const SCROLL_RESTORE_MIN_SCROLL = 50; // ignore tiny scrolls
const SCROLL_RETRY_DELAY_MS = 50; // handle async layout shifts
const SCROLL_CLEAR_DELAY_MS = 1000; // allow future 0-scrolls after restore
```
Used in your logic:
```ts
if (
scrollContainer &&
scrollContainer.scrollTop < SCROLL_RESET_THRESHOLD &&
latestScrollTop.current > SCROLL_RESTORE_MIN_SCROLL
) {
scrollContainer.scrollTop = latestScrollTop.current;
}
```
### 3. Optional: move scroll concern into a hook
If you want to further isolate scroll concerns from `DatabaseBlock`, you can wrap the above into a hook that exposes the single callback you need:
```ts
// hooks/useDatabaseScrollRestoration.ts
import { useCallback, useEffect, useRef } from 'react';
import type { ReactEditor } from 'slate-react';
import { getScrollContainer } from '@/utils/scroll';
export function useDatabaseScrollRestoration(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 handleRendered = useCallback(() => {
const restore = () => {
try {
const scrollContainer = getScrollContainer(editor);
if (
scrollContainer &&
scrollContainer.scrollTop < SCROLL_RESET_THRESHOLD &&
latestScrollTop.current > SCROLL_RESTORE_MIN_SCROLL
) {
scrollContainer.scrollTop = latestScrollTop.current;
}
} catch {}
};
restore();
setTimeout(restore, SCROLL_RETRY_DELAY_MS);
setTimeout(() => {
latestScrollTop.current = 0;
}, SCROLL_CLEAR_DELAY_MS);
}, [editor]);
return { handleRendered };
}
```
Usage in `DatabaseBlock`:
```ts
const editor = useSlateStatic();
const { handleRendered } = useDatabaseScrollRestoration(editor);
<DatabaseContent
// ...
onRendered={handleRendered}
/>;
```
These changes keep all behavior intact (same thresholds and timing) while making the scroll logic easier to understand, test, and reuse, and they pull non-render concerns out of the main component.
</issue_to_address>
### Comment 8
<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 extracting the drag preview DOM/image logic into a separate helper module so that `useBlockDrag` focuses only on drag wiring and state.
`useBlockDrag` is taking on a lot of responsibility, and the reviewer’s suggestion to separate preview construction from drag wiring will reduce complexity and make this easier to maintain.
You can keep the behavior but push the heavy DOM/image logic behind a small API and break it up internally:
```ts
// dragPreview.ts
interface BlockPreviewOptions {
isImageBlock: boolean;
}
export function createBlockDragPreview(
sourceElement: HTMLElement,
{ isImageBlock }: BlockPreviewOptions,
): HTMLElement {
const container = createPreviewContainer(sourceElement, isImageBlock);
const clone = cloneForPreview(sourceElement);
container.appendChild(clone);
handlePreviewImages(sourceElement, clone);
document.body.appendChild(container);
return container;
}
function createPreviewContainer(
sourceElement: HTMLElement,
isImage: boolean,
): HTMLDivElement {
const container = document.createElement('div');
const computedStyle = window.getComputedStyle(sourceElement);
let targetWidth = sourceElement.offsetWidth;
if (isImage) {
const img = sourceElement.querySelector('img');
if (img && img.offsetWidth > 0) {
targetWidth = img.offsetWidth;
}
}
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,
});
return container;
}
function cloneForPreview(sourceElement: HTMLElement): HTMLElement {
const clone = sourceElement.cloneNode(true) as HTMLElement;
clone.classList.remove('block-element--dragging');
clone.style.margin = '0';
clone.style.width = '100%';
clone.style.pointerEvents = 'none';
return clone;
}
function handlePreviewImages(source: HTMLElement, clone: HTMLElement) {
const originalImages = source.querySelectorAll('img');
const clonedImages = clone.querySelectorAll('img');
originalImages.forEach((orig, index) => {
const clonedImg = clonedImages[index];
if (!clonedImg) return;
try {
if (orig.complete && orig.naturalWidth > 0) {
const canvas = document.createElement('canvas');
canvas.width = orig.offsetWidth;
canvas.height = orig.offsetHeight;
const ctx = canvas.getContext('2d');
if (ctx) {
ctx.drawImage(orig, 0, 0, canvas.width, canvas.height);
canvas.style.maxWidth = '100%';
canvas.style.height = 'auto';
canvas.style.display = 'block';
canvas.style.opacity = '1';
canvas.style.pointerEvents = 'none';
clonedImg.parentNode?.replaceChild(canvas, clonedImg);
return;
}
}
} catch {
// fall through to img fallback
}
clonedImg.src = orig.currentSrc || orig.src;
clonedImg.loading = 'eager';
clonedImg.style.maxWidth = '100%';
clonedImg.style.height = 'auto';
clonedImg.style.opacity = '1';
clonedImg.style.display = 'block';
});
}
```
Then `useBlockDrag` becomes more linear and focused on drag wiring:
```ts
import { createBlockDragPreview } from './dragPreview';
export function useBlockDrag({...}: UseBlockDragProps) {
// ...
useEffect(() => {
const element = dragHandleRef.current;
if (!element || !blockId || !isDraggable || disabled) return;
return draggable({
element,
getInitialData: () => ({ type: 'editor-block', blockId, parentId }),
onGenerateDragPreview: ({ nativeSetDragImage }) => {
try {
const entry = findSlateEntryByBlockId(editor, blockId);
if (!entry) return;
const [node] = entry;
const blockElement = ReactEditor.toDOMNode(editor, node);
if (!blockElement) return;
const isImageBlock =
blockElement.getAttribute('data-block-type') === 'image';
const preview = createBlockDragPreview(blockElement, {
isImageBlock,
});
nativeSetDragImage?.(preview, 0, 0);
setTimeout(() => document.body.removeChild(preview), 0);
} catch (e) {
console.warn('Failed to generate drag preview:', e);
}
},
// ...
});
}, [blockId, parentId, dragHandleRef, isDraggable, disabled, onDragChange, editor]);
}
```
This keeps all current behavior (including the image/canvas handling) but:
- `useBlockDrag` only coordinates drag behavior and preview creation, making it easier to skim.
- Preview-specific DOM and styling logic is isolated and testable.
- Individual concerns (`createPreviewContainer`, `cloneForPreview`, `handlePreviewImages`) are small and easier to tweak without re-reading the entire hook.
</issue_to_address>
### Comment 9
<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 10
<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 11
<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 12
<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 13
<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 14
<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
+68
to
+70
| // Explicitly handle images to ensure they render correctly in the ghost | ||
| const originalImages = sourceElement.querySelectorAll('img'); | ||
| const clonedImages = container.querySelectorAll('img'); |
There was a problem hiding this comment.
issue (bug_risk): Image-specific drag-preview logic never runs because you query cloned images before appending the clone to the container.
Here container has no children yet, so container.querySelectorAll('img') returns an empty NodeList and the image-handling loop never runs. To make the image logic apply to the drag preview, either:
- Query on
cloneinstead:const clonedImages = clone.querySelectorAll('img');, or - Append the clone first, then query:
container.appendChild(clone); const clonedImages = container.querySelectorAll('img');.
| let scrollContainer: HTMLElement | null = null; | ||
|
|
||
| try { | ||
| const domNode = ReactEditor.toDOMNode(editor, editor); |
There was a problem hiding this comment.
issue (bug_risk): Using ReactEditor.toDOMNode(editor, editor) is likely incorrect and may throw at runtime.
toDOMNode expects (editor, SlateNode), so passing editor as the node is invalid and may throw (e.g., when asserting Element.isElement(node)). This pattern appears here and in the slash-panel DB creation handlers.
If you just need the editor root element, either use a known Slate node (e.g. editor.children[0]) or reuse an existing ref to the editor DOM element. It may also be worth centralizing the logic for finding the .appflowy-scroll-container for an editor to avoid duplicating this fragile lookup across components.
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 documents, improve scrolling behavior and visual feedback for embedded databases and panels, and simplify publish/share flows and workspace UI.
New Features:
Bug Fixes:
Enhancements:
isOfficialHostlogic and consumers.