chore: Optimize paste #162
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 GuideThis PR overhauls the editor paste pipeline to use AST-based HTML/Markdown parsing with context-aware merging, adds robust sanitization and table/simple table handling, and improves database grid scroll restoration and Cypress E2E coverage for paste scenarios and page editing; it also tweaks Slate-Yjs conversions and test/build dependencies to support the new behavior. Sequence diagram for the new smart paste pipelinesequenceDiagram
actor "User" as User
participant "Browser Clipboard" as Clipboard
participant "Slate ReactEditor" as Editor
participant "withPasted plugin" as WithPasted
participant "HTML parser" as HtmlParser
participant "Markdown detector" as MarkdownDetector
participant "Markdown parser" as MarkdownParser
participant "Paste context analyzer" as PasteContext
participant "Smart paste merger" as SmartPaste
participant "Yjs editor bridge" as YjsEditor
"User"->>"Browser Clipboard": "Copy content (HTML and/or plain text)"
"User"->>"Editor": "Paste (Ctrl+V / Cmd+V)"
"Editor"->>"WithPasted": "insertTextData(DataTransfer)"
"WithPasted"->>"Browser Clipboard": "data.getData('text/html')"
"WithPasted"->>"Browser Clipboard": "data.getData('text/plain')"
alt "HTML present and non-empty"
"WithPasted"->>"HtmlParser": "parseHTML(html)"
"HtmlParser"-->>"WithPasted": "Parsed blocks[]"
"WithPasted"->>"PasteContext": "analyzePasteContext(editor)"
"PasteContext"-->>"WithPasted": "PasteContext or null"
alt "Context available and blocks not empty"
"WithPasted"->>"SmartPaste": "smartPaste(editor, blocks, context)"
"SmartPaste"->>"YjsEditor": "Insert or merge blocks via slateContentInsertToYData()"
"SmartPaste"-->>"WithPasted": "true on success"
else "Context missing or error"
"WithPasted"-->>"Editor": "false (fallback handled upstream)"
end
else "No HTML, only plain text"
"WithPasted"->>"MarkdownDetector": "detectMarkdown(text)"
"MarkdownDetector"-->>"WithPasted": "isMarkdown (true/false)"
alt "Text is Markdown"
"WithPasted"->>"MarkdownParser": "parseMarkdown(text)"
"MarkdownParser"-->>"WithPasted": "Parsed blocks[]"
"WithPasted"->>"PasteContext": "analyzePasteContext(editor)"
"PasteContext"-->>"WithPasted": "PasteContext or null"
alt "Context available"
"WithPasted"->>"SmartPaste": "smartPaste(editor, blocks, context)"
"SmartPaste"->>"YjsEditor": "Insert or merge blocks"
"SmartPaste"-->>"WithPasted": "true on success"
else "Context missing"
"WithPasted"-->>"Editor": "false"
end
else "Plain multi-line text without Markdown"
"WithPasted"->>"PasteContext": "analyzePasteContext(editor)"
"PasteContext"-->>"WithPasted": "PasteContext or null"
"WithPasted"->>"SmartPaste": "smartPaste(editor, paragraphBlocksFromLines, context)"
"SmartPaste"->>"YjsEditor": "Append paragraph blocks"
"SmartPaste"-->>"WithPasted": "true on success"
end
end
"WithPasted"-->>"Editor": "Return true or false (paste handled or not)"
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:
- The new paste handlers (e.g. handleHTMLPaste/handlePlainTextPaste) contain several console.log/console.debug statements that will run on every paste in production; consider removing or gating them behind a debug flag to avoid noisy logs and potential performance impact.
- The pasteContent Cypress helper relies on traversing React fiber internals (e.g. __reactFiber*/__reactInternalInstance) to get the Slate editor, which is brittle across React versions; it would be more robust to expose the editor via a test-only prop or data attribute instead of depending on private React internals.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new paste handlers (e.g. handleHTMLPaste/handlePlainTextPaste) contain several console.log/console.debug statements that will run on every paste in production; consider removing or gating them behind a debug flag to avoid noisy logs and potential performance impact.
- The pasteContent Cypress helper relies on traversing React fiber internals (e.g. __reactFiber*/__reactInternalInstance) to get the Slate editor, which is brittle across React versions; it would be more robust to expose the editor via a test-only prop or data attribute instead of depending on private React internals.
## Individual Comments
### Comment 1
<location> `src/components/editor/plugins/withPasted.ts:32-38` </location>
<code_context>
- const lines = text.split(/\r\n|\r|\n/);
+ // Priority 1: HTML (if available)
+ if (html && html.trim().length > 0) {
+ console.log('[AppFlowy] Handling HTML paste', html);
+ return handleHTMLPaste(editor, html, text);
+ }
- const html = data.getData('text/html');
+ // Priority 2: Plain text
+ if (text && text.trim().length > 0) {
+ console.log('[AppFlowy] Handling Plain Text paste', text);
+ return handlePlainTextPaste(editor, text);
+ }
</code_context>
<issue_to_address>
**suggestion (performance):** Avoid unconditional console logging in the paste path to prevent noisy production logs and potential performance impact.
Consider wrapping these logs in a feature/debug flag or switching to the existing logDebug utility so they can be turned off outside of debug builds.
Suggested implementation:
```typescript
// Priority 1: HTML (if available)
if (html && html.trim().length > 0) {
logDebug('[AppFlowy] Handling HTML paste', html);
return handleHTMLPaste(editor, html, text);
}
// Priority 2: Plain text
if (text && text.trim().length > 0) {
logDebug('[AppFlowy] Handling Plain Text paste', text);
return handlePlainTextPaste(editor, text);
}
```
To fully implement this change, you will also need to:
1) Import the logDebug utility at the top of src/components/editor/plugins/withPasted.ts from wherever it is defined in your codebase (for example: import { logDebug } from '@/utils/logging';), matching existing logging import conventions in the project.
2) Ensure that logDebug is configured to be disabled or no-op in production builds, if that is not already the case.
</issue_to_address>
### Comment 2
<location> `src/components/editor/plugins/withPasted.ts:111-117` </location>
<code_context>
- const point = editor.selection?.anchor as BasePoint;
- const entry = getBlockEntry(editor as YjsEditor, point);
+ // Multi-line text: Check if it's Markdown
+ if (detectMarkdown(text)) {
+ return handleMarkdownPaste(editor, text);
+ }
- if (!entry) return;
+ // Plain multi-line text: Create paragraphs
+ return handleMultiLinePlainText(editor, lines);
+}
</code_context>
<issue_to_address>
**issue:** Multi-line paste behavior inside CodeBlocks is different from the previous implementation.
With this change, multi-line plain text is always routed through smartPaste/handleMultiLinePlainText, which turns it into Paragraph blocks. That alters prior behavior where pasting into a CodeBlock inserted raw text and stayed inside the block. Unless this UX change is intentional, consider special-casing BlockType.CodeBlock in handlePlainTextPaste to skip smartPaste and preserve the old “insert as plain text inside the code block” behavior.
</issue_to_address>
### Comment 3
<location> `src/components/database/DatabaseViews.tsx:47-49` </location>
<code_context>
const viewContainerRef = useRef<HTMLDivElement | null>(null);
const [lockedHeight, setLockedHeight] = useState<number | null>(fixedHeight ?? null);
const lastScrollRef = useRef<number | null>(null);
+ const heightLockTimeoutRef = useRef<NodeJS.Timeout | null>(null);
const value = useMemo(() => {
return Math.max(
</code_context>
<issue_to_address>
**suggestion:** Use a browser-safe timeout type instead of NodeJS.Timeout in a React DOM component.
In the browser/React DOM, setTimeout returns a number, not NodeJS.Timeout. Typing this ref as NodeJS.Timeout can cause type issues, especially outside Node-aware toolchains. Prefer a portable type like `useRef<ReturnType<typeof setTimeout> | null>(null)` to match the actual return type across environments.
```suggestion
const lastScrollRef = useRef<number | null>(null);
const heightLockTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const value = useMemo(() => {
```
</issue_to_address>
### Comment 4
<location> `src/components/editor/parsers/inline-converters.ts:210-219` </location>
<code_context>
+ // Group by type
</code_context>
<issue_to_address>
**issue (bug_risk):** mergeFormats groups bgColor spans without including bgColor-specific data in the grouping key.
In mergeFormats, the grouping key is `format.type + (format.data?.href || format.data?.color || '')`, but for bgColor formats it never uses `data.bgColor`. This means overlapping/adjacent bgColor spans with different colors can be merged, losing their distinct background colors. Please include `data.bgColor` in the key when `type === 'bgColor'` so visually different segments are not combined.
</issue_to_address>
### Comment 5
<location> `cypress/support/paste-utils.ts:15` </location>
<code_context>
+ * Helper function to paste content and wait for processing
+ * Directly calls Slate editor's insertData method to bypass event system
+ */
+export const pasteContent = (html: string, plainText: string) => {
+ // Wait for editors to be available
+ cy.get('[contenteditable="true"]').should('have.length.at.least', 1);
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the React fiber introspection in pasteContent with a single explicit test hook (or standardized DOM paste) to access Slate’s insertData behavior more directly and robustly.
You can significantly reduce complexity in `pasteContent` by avoiding React fiber introspection and consolidating on a single, explicit integration point.
Instead of:
- Walking the React fiber tree to find `slateEditor`
- Conditionally calling `slateEditor.insertData` vs. triggering a paste event
Introduce a simple, test-only hook that exposes the Slate editor (or a paste helper) in a stable way, and then call that from Cypress. This keeps the behavior (direct `insertData` call) without depending on React internals.
For example, in your app code (only in test/dev builds):
```ts
// Somewhere in your editor setup component
import { Editor } from 'slate';
declare global {
interface Window {
slateTestHelpers?: {
insertPasteData?: (data: DataTransfer) => void;
};
}
}
function MySlateEditor({ editor }: { editor: Editor }) {
if (typeof window !== 'undefined') {
window.slateTestHelpers = {
insertPasteData: (data: DataTransfer) => {
editor.insertData(data);
},
};
}
// ... existing render logic
}
```
Then in your Cypress helper, simplify `pasteContent` to:
```ts
export const pasteContent = (html: string, plainText: string) => {
cy.get('[contenteditable="true"]').should('have.length.at.least', 1);
cy.window().then(win => {
const helper = win.slateTestHelpers?.insertPasteData;
if (!helper) {
throw new Error('Slate test helper not available');
}
const dt = new win.DataTransfer();
if (html) dt.setData('text/html', html);
if (plainText || (!html && plainText === '')) {
dt.setData('text/plain', plainText ?? '');
}
helper(dt);
});
cy.wait(1500);
};
```
If you prefer to test via DOM events only, you can also drop the direct `insertData` call and just standardize on the `trigger('paste')` path, but the key simplification is:
- Remove all `__reactFiber/__reactInternalInstance` access
- Remove manual fiber traversal and `memoizedProps/stateNode` checks
- Use a small, explicit test helper (`window.slateTestHelpers`) as the single integration point
This keeps the tests robust to React/Slate refactors while preserving your ability to test all paste scenarios.
</issue_to_address>
### Comment 6
<location> `src/components/database/DatabaseViews.tsx:173` </location>
<code_context>
const targetScroll = lastScrollRef.current;
let rafCount = 0;
- let rafId: number;
+ const maxRAFs = 3; // Wait 3 animation frames (≈50ms at 60fps) for layout to settle
+ const rafIds: number[] = [];
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the scroll restoration logic and extracting it into a dedicated hook to reduce incidental complexity and clarify responsibilities in DatabaseViews.
You can keep the behavior but reduce incidental complexity around the scroll restoration / height lock coordination.
Two focused improvements:
1) Avoid the array of RAF ids – track a single RAF and a “cancelled” flag
2) Extract the timing/locking logic into a small hook to isolate responsibilities
---
1) Simplify RAF management (no array needed)
You don’t need to store all RAF ids; a single “current id” plus a cancellation flag is enough and easier to reason about:
Current:
```ts
const targetScroll = lastScrollRef.current;
let rafCount = 0;
const maxRAFs = 3;
const rafIds: number[] = [];
const restoreScroll = () => {
rafCount++;
if (rafCount < maxRAFs) {
const id = requestAnimationFrame(restoreScroll);
rafIds.push(id);
return;
}
if (Math.abs(scrollElement.scrollTop - targetScroll) > 1) {
scrollElement.scrollTop = targetScroll;
logDebug('[DatabaseViews] restored scroll position after layout settled', {
target: targetScroll,
current: scrollElement.scrollTop,
rafCount,
});
}
lastScrollRef.current = null;
setViewVisible(true);
if (!fixedHeight) {
heightLockTimeoutRef.current = setTimeout(() => {
setLockedHeight(null);
heightLockTimeoutRef.current = null;
}, 100);
}
};
const firstId = requestAnimationFrame(restoreScroll);
rafIds.push(firstId);
return () => {
rafIds.forEach(id => cancelAnimationFrame(id));
if (heightLockTimeoutRef.current) {
clearTimeout(heightLockTimeoutRef.current);
heightLockTimeoutRef.current = null;
}
};
```
Simplified (same behavior, fewer moving parts):
```ts
useEffect(() => {
if (isLoading) return;
if (lastScrollRef.current === null) return;
const scrollElement = getScrollElement();
if (!scrollElement) {
lastScrollRef.current = null;
return;
}
const targetScroll = lastScrollRef.current;
let rafCount = 0;
const maxRAFs = 3;
let rafId: number | null = null;
let cancelled = false;
const restoreScroll = () => {
if (cancelled) return;
rafCount++;
if (rafCount < maxRAFs) {
rafId = requestAnimationFrame(restoreScroll);
return;
}
if (Math.abs(scrollElement.scrollTop - targetScroll) > 1) {
scrollElement.scrollTop = targetScroll;
logDebug('[DatabaseViews] restored scroll position after layout settled', {
target: targetScroll,
current: scrollElement.scrollTop,
rafCount,
});
}
lastScrollRef.current = null;
setViewVisible(true);
if (!fixedHeight) {
heightLockTimeoutRef.current = setTimeout(() => {
// guard in case effect cleaned up in the meantime
if (cancelled) return;
setLockedHeight(null);
heightLockTimeoutRef.current = null;
}, 100);
}
};
rafId = requestAnimationFrame(restoreScroll);
return () => {
cancelled = true;
if (rafId != null) cancelAnimationFrame(rafId);
if (heightLockTimeoutRef.current) {
clearTimeout(heightLockTimeoutRef.current);
heightLockTimeoutRef.current = null;
}
};
}, [isLoading, viewId, fixedHeight]);
```
This keeps the “wait N frames then restore scroll” behavior but removes the array of ids and makes cleanup much clearer.
---
2) Extract scroll restoration + height lock into a hook
Right now `DatabaseViews` is coordinating:
- capturing scroll,
- locking height,
- hiding/showing view,
- waiting for multiple frames,
- and managing a timeout.
You can keep the same logic but move the timing/locking concerns into a hook so `DatabaseViews` reads more declaratively and is easier to test.
Example of extracting the current behavior:
```ts
function useViewTransitionScrollLock({
isLoading,
fixedHeight,
viewId,
viewContainerRef,
lastScrollRef,
}: {
isLoading: boolean;
fixedHeight?: number;
viewId: string;
viewContainerRef: React.RefObject<HTMLDivElement>;
lastScrollRef: React.MutableRefObject<number | null>;
}) {
const [lockedHeight, setLockedHeight] = useState<number | null>(fixedHeight ?? null);
const [viewVisible, setViewVisible] = useState(true);
const heightLockTimeoutRef = useRef<NodeJS.Timeout | null>(null);
// expose a helper to be called on view change
const startTransition = useCallback(() => {
const scrollElement = getScrollElement();
lastScrollRef.current = scrollElement?.scrollTop ?? null;
if (heightLockTimeoutRef.current) {
clearTimeout(heightLockTimeoutRef.current);
heightLockTimeoutRef.current = null;
}
const currentHeight = viewContainerRef.current?.offsetHeight;
const heightToLock = fixedHeight ?? currentHeight ?? null;
setLockedHeight(heightToLock ?? null);
setViewVisible(false);
}, [fixedHeight]);
// reuse the simplified RAF effect from above
useEffect(() => {
// ...same effect as in snippet 1, but using setViewVisible / setLockedHeight here...
}, [isLoading, viewId, fixedHeight]);
useEffect(() => {
return () => {
if (heightLockTimeoutRef.current) {
clearTimeout(heightLockTimeoutRef.current);
}
};
}, []);
return {
lockedHeight,
viewVisible,
startTransition,
};
}
```
Then `DatabaseViews` becomes easier to read:
```ts
const lastScrollRef = useRef<number | null>(null);
const { lockedHeight, viewVisible, startTransition } = useViewTransitionScrollLock({
isLoading,
fixedHeight,
viewId,
viewContainerRef,
lastScrollRef,
});
const handleViewChange = useCallback(
(newViewId: string) => {
startTransition();
setIsLoading(true);
onChangeView(newViewId);
},
[onChangeView, startTransition],
);
// usage in JSX:
<div
style={{
...(effectiveHeight !== null
? { height: `${effectiveHeight}px`, maxHeight: `${effectiveHeight}px` }
: {}),
opacity: viewVisible && !isLoading ? 1 : 0,
visibility: viewVisible && !isLoading ? 'visible' : 'hidden',
transition: 'opacity 150ms ease-in-out',
}}
>
...
</div>
```
This keeps all your current behaviors (no unmount, scroll restored after layout settles, height locked during transition) but:
- `DatabaseViews` no longer has to coordinate RAFs, timeouts, and visibility flags directly.
- The scroll/height/visibility timing is centralized in one hook, with a single cleanup path.
</issue_to_address>
### Comment 7
<location> `src/components/database/components/grid/grid-table/useGridVirtualizer.ts:62` </location>
<code_context>
- previous: parentOffset,
- delta,
- });
+ // Use multiple RAFs during initial mount to ensure layout is stable
+ // This helps prevent scroll jumps during view transitions
+ const rafCount = isInitialMountRef.current ? 3 : 1;
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the new RAF/threshold/initial-measurement logic into a dedicated helper to keep updateParentOffset simpler and easier to reason about.
You can keep the new behavior (initial multi-RAF, thresholds, first-measurement handling) but localize the complexity by extracting the RAF/threshold/state-machine into a small helper. That makes `updateParentOffset` much easier to reason about.
For example, move the RAF/threshold logic into a dedicated helper that only exposes “give me a stable parent offset”:
```ts
function scheduleStableParentOffset({
measureParentOffset,
isInitialMountRef,
parentOffsetRef,
rafIdRef,
logDebug,
onStableOffset,
}: {
measureParentOffset: () => number | null;
isInitialMountRef: React.MutableRefObject<boolean>;
parentOffsetRef: React.MutableRefObject<number | null>;
rafIdRef: React.MutableRefObject<number | undefined>;
logDebug: (...args: any[]) => void;
onStableOffset: (offset: number) => void;
}) {
const first = measureParentOffset();
if (first === null) {
logDebug('[GridVirtualizer] skip parent offset update; missing refs');
return;
}
const rafCount = isInitialMountRef.current ? 3 : 1;
let currentRaf = 0;
const performUpdate = () => {
currentRaf++;
if (currentRaf < rafCount) {
rafIdRef.current = requestAnimationFrame(performUpdate);
return;
}
const measured = measureParentOffset();
const nextOffset = measured ?? first;
if (parentOffsetRef.current === null) {
parentOffsetRef.current = nextOffset;
isInitialMountRef.current = false;
logDebug('[GridVirtualizer] initial parent offset set', { nextOffset });
onStableOffset(nextOffset);
return;
}
const delta = Math.abs(nextOffset - parentOffsetRef.current);
const threshold = isInitialMountRef.current ? 10 : 5;
if (delta < threshold) {
logDebug('[GridVirtualizer] parent offset stable', {
current: parentOffsetRef.current,
measured: nextOffset,
delta,
threshold,
isInitialMount: isInitialMountRef.current,
});
isInitialMountRef.current = false;
return;
}
parentOffsetRef.current = nextOffset;
logDebug('[GridVirtualizer] parent offset updated', {
nextOffset,
previous: parentOffsetRef.current,
delta,
isInitialMount: isInitialMountRef.current,
});
isInitialMountRef.current = false;
onStableOffset(nextOffset);
};
rafIdRef.current = requestAnimationFrame(performUpdate);
}
```
Then `updateParentOffset` becomes a thin wrapper that focuses only on orchestration (cancel old RAF, schedule new one, update React state), while the helper encapsulates all the tricky behavior:
```ts
const updateParentOffset = useCallback(() => {
if (rafIdRef.current !== undefined) {
cancelAnimationFrame(rafIdRef.current);
}
scheduleStableParentOffset({
measureParentOffset,
isInitialMountRef,
parentOffsetRef,
rafIdRef,
logDebug,
onStableOffset: (offset) => {
setParentOffset(offset);
},
});
}, [measureParentOffset]);
```
This keeps:
- Triple RAF vs single RAF behavior.
- First-measurement “always accept” rule.
- 10px vs 5px thresholds.
- isInitialMountRef semantics across view switches.
But it separates concerns:
- `scheduleStableParentOffset` handles timing, thresholds, and state flags.
- `updateParentOffset` just triggers a new measurement cycle and updates state.
</issue_to_address>
### Comment 8
<location> `src/components/editor/parsers/html-parser.ts:44-46` </location>
<code_context>
const blocks = convertASTToAppFlowyBlocks(tree.children as HastElement[], options);
return blocks;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return convertASTToAppFlowyBlocks(tree.children as HastElement[], options);
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 9
<location> `src/components/editor/parsers/html-parser.ts:88-110` </location>
<code_context>
if (block) {
// Handle array of blocks (e.g. flattened lists)
if (Array.isArray(block)) {
// Recursively process nested structures if needed (though flattened blocks are usually leaves)
blocks.push(...block);
} else {
// For blocks with children (like lists that weren't flattened), recursively process
// Note: parseList now returns array, so this branch is less common for lists
if (block.children.length > 0) {
// Children already processed in block converter for some types
blocks.push(block);
} else {
blocks.push(block);
}
}
} else {
// If element couldn't be converted, try processing its children
if (node.children && node.children.length > 0) {
const childBlocks = convertASTToAppFlowyBlocks(node.children as HastElement[], options, depth + 1);
blocks.push(...childBlocks);
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge else clause's nested if statement into `else if` ([`merge-else-if`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/merge-else-if))
```suggestion
if (block) {
// Handle array of blocks (e.g. flattened lists)
if (Array.isArray(block)) {
// Recursively process nested structures if needed (though flattened blocks are usually leaves)
blocks.push(...block);
} else {
// For blocks with children (like lists that weren't flattened), recursively process
// Note: parseList now returns array, so this branch is less common for lists
if (block.children.length > 0) {
// Children already processed in block converter for some types
blocks.push(block);
} else {
blocks.push(block);
}
}
}
else if (node.children && node.children.length > 0) {
const childBlocks = convertASTToAppFlowyBlocks(node.children as HastElement[], options, depth + 1);
blocks.push(...childBlocks);
}
```
<br/><details><summary>Explanation</summary>Flattening if statements nested within else clauses generates code that is
easier to read and expand upon.
</details>
</issue_to_address>
### Comment 10
<location> `src/components/editor/parsers/html-parser.ts:90-102` </location>
<code_context>
if (Array.isArray(block)) {
// Recursively process nested structures if needed (though flattened blocks are usually leaves)
blocks.push(...block);
} else {
// For blocks with children (like lists that weren't flattened), recursively process
// Note: parseList now returns array, so this branch is less common for lists
if (block.children.length > 0) {
// Children already processed in block converter for some types
blocks.push(block);
} else {
blocks.push(block);
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge else clause's nested if statement into `else if` ([`merge-else-if`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/merge-else-if))
```suggestion
if (Array.isArray(block)) {
// Recursively process nested structures if needed (though flattened blocks are usually leaves)
blocks.push(...block);
}
else if (block.children.length > 0) {
// Children already processed in block converter for some types
blocks.push(block);
}
else {
blocks.push(block);
}
```
<br/><details><summary>Explanation</summary>Flattening if statements nested within else clauses generates code that is
easier to read and expand upon.
</details>
</issue_to_address>
### Comment 11
<location> `src/components/editor/parsers/html-parser.ts:145-163` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 12
<location> `src/components/editor/parsers/inline-converters.ts:35-179` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 13
<location> `src/components/editor/parsers/markdown-parser.ts:58-60` </location>
<code_context>
const blocks = convertMarkdownASTToAppFlowyBlocks(ast.children as BlockContent[]);
return blocks;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return convertMarkdownASTToAppFlowyBlocks(ast.children as BlockContent[]);
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 14
<location> `src/components/editor/parsers/mdast-utils.ts:36-118` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid function declarations, favouring function assignment expressions, inside blocks. ([`avoid-function-declarations-in-blocks`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/avoid-function-declarations-in-blocks))
<details><summary>Explanation</summary>Function declarations may be hoisted in Javascript, but the behaviour is inconsistent between browsers.
Hoisting is generally confusing and should be avoided. Rather than using function declarations inside blocks, you
should use function expressions, which create functions in-scope.
</details>
</issue_to_address>
### Comment 15
<location> `src/components/editor/parsers/sanitize.ts:80-82` </location>
<code_context>
const sanitized = DOMPurify.sanitize(html, SANITIZE_CONFIG);
return sanitized;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return DOMPurify.sanitize(html, SANITIZE_CONFIG);
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 16
<location> `src/components/editor/parsers/sanitize.ts:150-155` </location>
<code_context>
if (key && value && allowed.includes(key.toLowerCase())) {
// Basic validation of CSS values (prevent expressions)
if (!/javascript|expression|@import|url\(/i.test(value)) {
result[key.toLowerCase()] = value;
}
}
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/merge-nested-ifs))
```suggestion
if (key && value && allowed.includes(key.toLowerCase()) && !/javascript|expression|@import|url\(/i.test(value)) {
result[key.toLowerCase()] = value;
}
```
<br/><details><summary>Explanation</summary>Reading deeply nested conditional code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 17
<location> `src/components/editor/utils/markdown-detector.ts:79-81` </location>
<code_context>
const density = Math.min(totalMatches / (lines * 0.5), 1);
return density;
</code_context>
<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/TypeScript/Default-Rules/inline-immediately-returned-variable))
```suggestion
return Math.min(totalMatches / (lines * 0.5), 1);
```
<br/><details><summary>Explanation</summary>Something that we often see in people's code is assigning to a result variable
and then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
</details>
</issue_to_address>
### Comment 18
<location> `src/components/editor/utils/paste-merger.ts:184` </location>
<code_context>
const blockId = (node as { blockId?: string }).blockId;
</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 {blockId} = node as { blockId?: string };
```
<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 19
<location> `src/components/editor/utils/paste-merger.ts:268` </location>
<code_context>
const blockId = (node as { blockId?: string }).blockId;
</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 {blockId} = node as { blockId?: string };
```
<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
+111
to
+117
| // Multi-line text: Check if it's Markdown | ||
| if (detectMarkdown(text)) { | ||
| return handleMarkdownPaste(editor, text); | ||
| } | ||
|
|
||
| if (!entry) return; | ||
| // Plain multi-line text: Create paragraphs | ||
| return handleMultiLinePlainText(editor, lines); |
There was a problem hiding this comment.
issue: Multi-line paste behavior inside CodeBlocks is different from the previous implementation.
With this change, multi-line plain text is always routed through smartPaste/handleMultiLinePlainText, which turns it into Paragraph blocks. That alters prior behavior where pasting into a CodeBlock inserted raw text and stayed inside the block. Unless this UX change is intentional, consider special-casing BlockType.CodeBlock in handlePlainTextPaste to skip smartPaste and preserve the old “insert as plain text inside the code block” behavior.
Comment on lines
47
to
49
| const lastScrollRef = useRef<number | null>(null); | ||
| const heightLockTimeoutRef = useRef<NodeJS.Timeout | null>(null); | ||
| const value = useMemo(() => { |
There was a problem hiding this comment.
suggestion: Use a browser-safe timeout type instead of NodeJS.Timeout in a React DOM component.
In the browser/React DOM, setTimeout returns a number, not NodeJS.Timeout. Typing this ref as NodeJS.Timeout can cause type issues, especially outside Node-aware toolchains. Prefer a portable type like useRef<ReturnType<typeof setTimeout> | null>(null) to match the actual return type across environments.
Suggested change
| const lastScrollRef = useRef<number | null>(null); | |
| const heightLockTimeoutRef = useRef<NodeJS.Timeout | null>(null); | |
| const value = useMemo(() => { | |
| const lastScrollRef = useRef<number | null>(null); | |
| const heightLockTimeoutRef = useRef<ReturnType<typeof setTimeout> | null>(null); | |
| const value = useMemo(() => { |
Comment on lines
+210
to
+219
| // Group by type | ||
| const byType = new Map<string, InlineFormat[]>(); | ||
|
|
||
| formats.forEach((format) => { | ||
| const key = format.type + (format.data?.href || format.data?.color || ''); | ||
|
|
||
| if (!byType.has(key)) { | ||
| byType.set(key, []); | ||
| } | ||
|
|
There was a problem hiding this comment.
issue (bug_risk): mergeFormats groups bgColor spans without including bgColor-specific data in the grouping key.
In mergeFormats, the grouping key is format.type + (format.data?.href || format.data?.color || ''), but for bgColor formats it never uses data.bgColor. This means overlapping/adjacent bgColor spans with different colors can be merged, losing their distinct background colors. Please include data.bgColor in the key when type === 'bgColor' so visually different segments are not combined.
Comment on lines
+44
to
+46
| const blocks = convertASTToAppFlowyBlocks(tree.children as HastElement[], options); | ||
|
|
||
| return blocks; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| const blocks = convertASTToAppFlowyBlocks(tree.children as HastElement[], options); | |
| return blocks; | |
| return convertASTToAppFlowyBlocks(tree.children as HastElement[], options); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
Comment on lines
+88
to
+110
| if (block) { | ||
| // Handle array of blocks (e.g. flattened lists) | ||
| if (Array.isArray(block)) { | ||
| // Recursively process nested structures if needed (though flattened blocks are usually leaves) | ||
| blocks.push(...block); | ||
| } else { | ||
| // For blocks with children (like lists that weren't flattened), recursively process | ||
| // Note: parseList now returns array, so this branch is less common for lists | ||
| if (block.children.length > 0) { | ||
| // Children already processed in block converter for some types | ||
| blocks.push(block); | ||
| } else { | ||
| blocks.push(block); | ||
| } | ||
| } | ||
| } else { | ||
| // If element couldn't be converted, try processing its children | ||
| if (node.children && node.children.length > 0) { | ||
| const childBlocks = convertASTToAppFlowyBlocks(node.children as HastElement[], options, depth + 1); | ||
|
|
||
| blocks.push(...childBlocks); | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge else clause's nested if statement into else if (merge-else-if)
Suggested change
| if (block) { | |
| // Handle array of blocks (e.g. flattened lists) | |
| if (Array.isArray(block)) { | |
| // Recursively process nested structures if needed (though flattened blocks are usually leaves) | |
| blocks.push(...block); | |
| } else { | |
| // For blocks with children (like lists that weren't flattened), recursively process | |
| // Note: parseList now returns array, so this branch is less common for lists | |
| if (block.children.length > 0) { | |
| // Children already processed in block converter for some types | |
| blocks.push(block); | |
| } else { | |
| blocks.push(block); | |
| } | |
| } | |
| } else { | |
| // If element couldn't be converted, try processing its children | |
| if (node.children && node.children.length > 0) { | |
| const childBlocks = convertASTToAppFlowyBlocks(node.children as HastElement[], options, depth + 1); | |
| blocks.push(...childBlocks); | |
| } | |
| } | |
| if (block) { | |
| // Handle array of blocks (e.g. flattened lists) | |
| if (Array.isArray(block)) { | |
| // Recursively process nested structures if needed (though flattened blocks are usually leaves) | |
| blocks.push(...block); | |
| } else { | |
| // For blocks with children (like lists that weren't flattened), recursively process | |
| // Note: parseList now returns array, so this branch is less common for lists | |
| if (block.children.length > 0) { | |
| // Children already processed in block converter for some types | |
| blocks.push(block); | |
| } else { | |
| blocks.push(block); | |
| } | |
| } | |
| } | |
| else if (node.children && node.children.length > 0) { | |
| const childBlocks = convertASTToAppFlowyBlocks(node.children as HastElement[], options, depth + 1); | |
| blocks.push(...childBlocks); | |
| } | |
Explanation
Flattening if statements nested within else clauses generates code that iseasier to read and expand upon.
Comment on lines
+80
to
+82
| const sanitized = DOMPurify.sanitize(html, SANITIZE_CONFIG); | ||
|
|
||
| return sanitized; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| const sanitized = DOMPurify.sanitize(html, SANITIZE_CONFIG); | |
| return sanitized; | |
| return DOMPurify.sanitize(html, SANITIZE_CONFIG); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
Comment on lines
+150
to
+155
| if (key && value && allowed.includes(key.toLowerCase())) { | ||
| // Basic validation of CSS values (prevent expressions) | ||
| if (!/javascript|expression|@import|url\(/i.test(value)) { | ||
| result[key.toLowerCase()] = value; | ||
| } | ||
| } |
There was a problem hiding this comment.
suggestion (code-quality): Merge nested if conditions (merge-nested-ifs)
Suggested change
| if (key && value && allowed.includes(key.toLowerCase())) { | |
| // Basic validation of CSS values (prevent expressions) | |
| if (!/javascript|expression|@import|url\(/i.test(value)) { | |
| result[key.toLowerCase()] = value; | |
| } | |
| } | |
| if (key && value && allowed.includes(key.toLowerCase()) && !/javascript|expression|@import|url\(/i.test(value)) { | |
| result[key.toLowerCase()] = value; | |
| } | |
Explanation
Reading deeply nested conditional code is confusing, since you have to keep track of whichconditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two
if conditions can be combined usingand is an easy win.
Comment on lines
+79
to
+81
| const density = Math.min(totalMatches / (lines * 0.5), 1); | ||
|
|
||
| return density; |
There was a problem hiding this comment.
suggestion (code-quality): Inline variable that is immediately returned (inline-immediately-returned-variable)
Suggested change
| const density = Math.min(totalMatches / (lines * 0.5), 1); | |
| return density; | |
| return Math.min(totalMatches / (lines * 0.5), 1); | |
Explanation
Something that we often see in people's code is assigning to a result variableand then immediately returning it.
Returning the result directly shortens the code and removes an unnecessary
variable, reducing the mental load of reading the function.
Where intermediate variables can be useful is if they then get used as a
parameter or a condition, and the name can act like a comment on what the
variable represents. In the case where you're returning it from a function, the
function name is there to tell you what the result is, so the variable name
is unnecessary.
| if (!entry) return false; | ||
|
|
||
| const [node] = entry; | ||
| const blockId = (node as { blockId?: string }).blockId; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const blockId = (node as { blockId?: string }).blockId; | |
| const {blockId} = node as { blockId?: string }; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
| if (!entry) return false; | ||
|
|
||
| const [node] = entry; | ||
| const blockId = (node as { blockId?: string }).blockId; |
There was a problem hiding this comment.
suggestion (code-quality): Prefer object destructuring when accessing and using properties. (use-object-destructuring)
Suggested change
| const blockId = (node as { blockId?: string }).blockId; | |
| const {blockId} = node as { blockId?: string }; |
Explanation
Object destructuring can often remove an unnecessary temporary reference, as well as making your code more succinct.From the Airbnb Javascript Style Guide
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
Improve rich text paste handling and database view scroll stability while enhancing test coverage for paste behavior and page editing.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: