Skip to content

feat: virtual list for graphical editor#615

Merged
MakinoharaShoko merged 1 commit into
devfrom
vlist
Jun 6, 2026
Merged

feat: virtual list for graphical editor#615
MakinoharaShoko merged 1 commit into
devfrom
vlist

Conversation

@MakinoharaShoko

Copy link
Copy Markdown
Member

No description provided.

@MakinoharaShoko MakinoharaShoko requested a review from A-kirami May 28, 2026 18:55

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces virtualized scrolling to the GraphicalEditor component using @tanstack/react-virtual to improve performance when rendering large lists of sentences, and integrates it with @hello-pangea/dnd for drag-and-drop support. The review feedback highlights two main areas for improvement: first, ensuring that the scroll-to-target-line logic only runs after sentenceData is fully synchronized to prevent incorrect scroll offsets; second, properly hiding the original item's content during a drag operation by passing a dragging placeholder flag to prevent duplicate content from being displayed.

Comment on lines +297 to +302
useEffect(() => {
const targetLine = editorLineHolder.getSceneLine(props.targetPath);
if (targetLine > 3 && targetLine <= parsedScene.sentenceList.length) {
rowVirtualizer.scrollToIndex(targetLine - 1, { align: 'start' });
}
}, [parsedScene.sentenceList.length, props.targetPath, rowVirtualizer]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The current scroll-to-target-line logic triggers as soon as parsedScene.sentenceList is loaded. However, at this point, sentenceData might not be fully synchronized yet. If the scroll occurs while sentenceData is empty or incomplete, the virtualizer will use the default/collapsed height (48px) to calculate the scroll offset. Once sentenceData loads and items expand to their actual heights (e.g., 120px), the target line will be pushed down and out of view.

To fix this, we should use a ref to track the last scrolled path and only trigger the scroll once sentenceData is fully synchronized with parsedScene.sentenceList.

  const lastScrolledPathRef = useRef<string | null>(null);

  useEffect(() => {
    const targetLine = editorLineHolder.getSceneLine(props.targetPath);
    const isDataReady = sentenceData.length === parsedScene.sentenceList.length && sentenceData.length > 0;

    if (isDataReady && lastScrolledPathRef.current !== props.targetPath) {
      if (targetLine > 3 && targetLine <= parsedScene.sentenceList.length) {
        rowVirtualizer.scrollToIndex(targetLine - 1, { align: 'start' });
        lastScrolledPathRef.current = props.targetPath;
      }
    }
  }, [parsedScene.sentenceList.length, props.targetPath, sentenceData, rowVirtualizer]);

Comment on lines +49 to +51
interface SentenceRowContentProps extends SentenceRowProps {
provided: DraggableProvided;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Add isDraggingPlaceholder to SentenceRowContentProps to allow hiding the content of the original item in the list while it is being dragged.

Suggested change
interface SentenceRowContentProps extends SentenceRowProps {
provided: DraggableProvided;
}
interface SentenceRowContentProps extends SentenceRowProps {
provided: DraggableProvided;
isDraggingPlaceholder: boolean;
}

Comment on lines +338 to +341
const renderSentenceRow = (provided: DraggableProvided, i: number) => {
const rowProps = getSentenceRowProps(i);
return rowProps && <SentenceRowContent provided={provided} {...rowProps} />;
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When rendering the clone for the dragging item under the cursor, we should pass isDraggingPlaceholder={false} so that the full content of the card is visible while dragging.

Suggested change
const renderSentenceRow = (provided: DraggableProvided, i: number) => {
const rowProps = getSentenceRowProps(i);
return rowProps && <SentenceRowContent provided={provided} {...rowProps} />;
};
const renderSentenceRow = (provided: DraggableProvided, i: number) => {
const rowProps = getSentenceRowProps(i);
return rowProps && <SentenceRowContent provided={provided} isDraggingPlaceholder={false} {...rowProps} />;
};

Comment on lines +400 to 404
const SentenceRowContent = (props: SentenceRowContentProps) => {
const { provided, sentence, sentenceItem, index: i, linkedWithPrevious, targetPath, sceneLabels } = props;
const index = i + 1;
const sentenceConfig = sentenceEditorConfig.find((e) => e.type === sentence.command) ?? sentenceEditorDefault;
const SentenceEditor = sentenceConfig.component;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

When an item is being dragged in a virtual list, @hello-pangea/dnd renders a clone under the cursor, but the original item still remains in the list. If we don't hide its content, a duplicate of the dragging item will remain visible in its original position.

We should check isDraggingPlaceholder and render an empty placeholder container with the drag handle props to preserve the space without showing duplicate content.

const SentenceRowContent = (props: SentenceRowContentProps) => {
  const { provided, isDraggingPlaceholder, sentence, sentenceItem, index: i, linkedWithPrevious, targetPath, sceneLabels } = props;
  const index = i + 1;

  if (isDraggingPlaceholder) {
    return <div ref={provided.innerRef} {...provided.draggableProps} style={provided.draggableProps.style} />;
  }

  const sentenceConfig = sentenceEditorConfig.find((e) => e.type === sentence.command) ?? sentenceEditorDefault;
  const SentenceEditor = sentenceConfig.component;

Comment on lines +482 to 485
const SentenceRow = memo((props: SentenceRowProps) => {
return <Draggable key={props.sentenceItem.id} draggableId={props.sentenceItem.id} index={props.index}>
{(provided) => <SentenceRowContent provided={provided} {...props} />}
</Draggable>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Pass snapshot.isDragging from Draggable to SentenceRowContent as isDraggingPlaceholder so that the original item in the list can hide its content while being dragged.

Suggested change
const SentenceRow = memo((props: SentenceRowProps) => {
return <Draggable key={props.sentenceItem.id} draggableId={props.sentenceItem.id} index={props.index}>
{(provided) => <SentenceRowContent provided={provided} {...props} />}
</Draggable>;
const SentenceRow = memo((props: SentenceRowProps) => {
return <Draggable key={props.sentenceItem.id} draggableId={props.sentenceItem.id} index={props.index}>
{(provided, snapshot) => <SentenceRowContent provided={provided} isDraggingPlaceholder={snapshot.isDragging} {...props} />}
</Draggable>;

@MakinoharaShoko MakinoharaShoko merged commit c30ca8a into dev Jun 6, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant