Skip to content

feat(client/libs): implemented DND reordering for notebook cells#1061

Merged
johbaxter merged 6 commits intodevfrom
709-DND-cells-in-notebooks
May 8, 2025
Merged

feat(client/libs): implemented DND reordering for notebook cells#1061
johbaxter merged 6 commits intodevfrom
709-DND-cells-in-notebooks

Conversation

@Paulson-Robert
Copy link
Copy Markdown
Contributor

Description

Changes Made

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes

@Paulson-Robert Paulson-Robert requested a review from johbaxter May 8, 2025 14:11
@Paulson-Robert Paulson-Robert self-assigned this May 8, 2025
@Paulson-Robert Paulson-Robert requested a review from a team as a code owner May 8, 2025 14:11
@github-actions
Copy link
Copy Markdown

github-actions bot commented May 8, 2025

@CodiumAI-Agent /describe

@QodoAI-Agent
Copy link
Copy Markdown

Title

feat(client/libs): implemented DND reordering for notebook cells


User description

Description

Changes Made

How to Test

  1. Steps to reproduce/test the behavior
  2. Expected outcomes

Notes


PR Type

Enhancement


Description

  • Add drag-and-drop cell reordering.

  • Implement MOVE_CELL action and handler.

  • Extend QueryState with _moveCell method.

  • Integrate dnd-kit in Notebook component.


Changes walkthrough 📝

Relevant files
Enhancement
query.state.ts
Add internal method for cell moving                                           

libs/renderer/src/store/state/query.state.ts

  • Add _moveCell method for cell reordering.
  • Include null safety before moving cells.
  • +13/-0   
    state.actions.ts
    Define MOVE_CELL action interface                                               

    libs/renderer/src/store/state/state.actions.ts

  • Introduce MOVE_CELL in ActionMessages.
  • Define MoveCellAction interface.
  • Include MoveCellAction in Actions union.
  • +11/-0   
    state.store.ts
    Implement MOVE_CELL in StateStore                                               

    libs/renderer/src/store/state/state.store.ts

  • Handle MOVE_CELL message in dispatcher.
  • Add moveCell private handler method.
  • Call QueryState._moveCell for logic.
  • +18/-0   
    Notebook.tsx
    Integrate drag-and-drop UI in Notebook                                     

    packages/client/src/components/notebook/Notebook.tsx

  • Import dnd-kit and DragIndicator.
  • Create SortableItems draggable wrapper.
  • Add handleDragEnd to dispatch MOVE_CELL.
  • Wrap cells in DndContext and SortableContext.
  • +103/-14

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented May 8, 2025

    @CodiumAI-Agent /review

    @Paulson-Robert Paulson-Robert linked an issue May 8, 2025 that may be closed by this pull request
    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Safety

    The handleDragEnd callback parameters are untyped, which may cause TypeScript errors or unexpected runtime issues. Consider typing the event (e.g., DragEndEvent) and its properties.

    const handleDragEnd = ({ active, over }) => {
        // If the active item is over the same item, do nothing
        if (!active || !over) {
            console.error('Invalid item!');
            return;
        }
    
        // If the active item is over a different item, swap them
        if (over && active.id !== over.id) {
            const oldIndex = Number(active.id);
            const newIndex = Number(over.id);
            state.dispatch({
                message: ActionMessages.MOVE_CELL,
                payload: {
                    queryId: id,
                    activeCellId: active.id,
                    overCellId: over.id,
                },
            });
        }
    };
    Unused Variables

    The oldIndex and newIndex variables in handleDragEnd are calculated but never used. They can be removed to clean up the code.

            const oldIndex = Number(active.id);
            const newIndex = Number(over.id);
            state.dispatch({
                message: ActionMessages.MOVE_CELL,
                payload: {
                    queryId: id,
                    activeCellId: active.id,
                    overCellId: over.id,
                },
            });
        }
    };

    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented May 8, 2025

    @CodiumAI-Agent /improve

    @QodoAI-Agent
    Copy link
    Copy Markdown

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Guard and adjust indices

    Add guards against invalid indices to avoid removing or inserting at -1, and adjust
    the insertion index when moving an item downward since removal shifts the list. This
    prevents unexpected behavior when one of the IDs is missing or when activeIdx <
    overIdx.

    libs/renderer/src/store/state/query.state.ts [340-343]

     const activeIdx = this._store.list.indexOf(active);
     const overIdx = this._store.list.indexOf(over);
    +if (activeIdx < 0 || overIdx < 0) return;
     this._store.list.splice(activeIdx, 1);
    -this._store.list.splice(overIdx, 0, active);
    +const targetIdx = activeIdx < overIdx ? overIdx - 1 : overIdx;
    +this._store.list.splice(targetIdx, 0, active);
    Suggestion importance[1-10]: 8

    __

    Why: Adding index guards and adjusting the target index prevents runtime errors when indexOf returns -1 or when removing an earlier element shifts the array.

    Medium
    General
    Remove redundant key prop

    Remove the internal key on the wrapping

    since React keys should be attached at the
    list rendering level (SortableItems usage) and not on the component's root element.

    packages/client/src/components/notebook/Notebook.tsx [83-91]

     return (
    -    <div key={`action-${id}`} ref={setNodeRef} style={style}>
    +    <div ref={setNodeRef} style={style}>
             <Box
                 {...attributes}
                 {...listeners}
                 sx={{ cursor: 'grab', alignSelf: 'baseline', paddingTop: 2 }}
             >
                 <DragIndicator color="disabled" />
             </Box>
             {children}
         </div>
     );
    Suggestion importance[1-10]: 4

    __

    Why: Removing the internal key on the wrapping <div> avoids duplicate keys and minorly improves React list rendering correctness.

    Low

    @johbaxter johbaxter merged commit 2aca07b into dev May 8, 2025
    3 checks passed
    @johbaxter johbaxter deleted the 709-DND-cells-in-notebooks branch May 8, 2025 19:44
    @github-actions
    Copy link
    Copy Markdown

    github-actions bot commented May 8, 2025

    @CodiumAI-Agent /update_changelog

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    None yet

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    Add ability to move cells in drag and drop notebook

    3 participants