Add dnd-kit sorting with wheel scrolling support#14
Conversation
Summary of ChangesHello @0suu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the drag-and-drop reordering capabilities for both device and scene lists. By migrating from a custom implementation to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully migrates the drag-and-drop reordering functionality to dnd-kit, which is a great improvement for maintainability and user experience. The code is well-structured, particularly with the extraction of sortable item logic into new components. I've identified a few areas for improvement, including a UI bug in the drag-over handling, some unsafe type assertions that could lead to runtime errors, and a suggestion to further improve code organization by extracting large components into their own files. Overall, this is a solid update.
| const handleDragOver = (event: DragOverEvent) => { | ||
| if (!isReorderMode) return; | ||
| const overId = event.over?.id as string | undefined; | ||
| const activeId = event.active.id as string; | ||
| if (!overId || activeId === overId) return; | ||
| setDragOverId(overId); | ||
| }; |
There was a problem hiding this comment.
The current implementation of handleDragOver has a bug where the drag-over highlight (the outline) can get stuck on an item. This happens if you drag over an item and then drag away to an area with no droppable items, or if you drag over the item that is currently being dragged. To fix this and also improve type safety, I suggest replacing this handler with a more robust version.
const handleDragOver = (event: DragOverEvent) => {
if (!isReorderMode) return;
const { active, over } = event;
const overId = over ? String(over.id) : null;
// Do not highlight the item being dragged
if (String(active.id) === overId) {
setDragOverId(null);
return;
}
setDragOverId(overId);
};
| const handleDragOver = (event: DragOverEvent) => { | ||
| if (!isReorderMode) return; | ||
| const overId = event.over?.id as string | undefined; | ||
| const activeId = event.active.id as string; | ||
| if (!overId || overId === activeId) return; | ||
| setDragOverId(overId); | ||
| }; |
There was a problem hiding this comment.
The current implementation of handleDragOver has a bug where the drag-over highlight (the outline) can get stuck on an item. This happens if you drag over an item and then drag away to an area with no droppable items, or if you drag over the item that is currently being dragged. To fix this and also improve type safety, I suggest replacing this handler with a more robust version.
const handleDragOver = (event: DragOverEvent) => {
if (!isReorderMode) return;
const { active, over } = event;
const overId = over ? String(over.id) : null;
// Do not highlight the item being dragged
if (String(active.id) === overId) {
setDragOverId(null);
return;
}
setDragOverId(overId);
};
| const handleDragStart = (id: string) => (event: React.DragEvent<HTMLDivElement>) => { | ||
| const handleDragStart = (event: DragStartEvent) => { | ||
| if (!isReorderMode) return; | ||
| const id = event.active.id as string; |
There was a problem hiding this comment.
Using as string for the event ID is not type-safe, as dnd-kit's UniqueIdentifier type is string | number. If the ID were ever a number, this could cause issues with functions like moveIdInList that expect strings. It's safer to explicitly convert it to a string using String().
| const id = event.active.id as string; | |
| const id = String(event.active.id); |
| } | ||
| const { active, over } = event; | ||
| if (over && active.id !== over.id) { | ||
| setReorderList((prev) => moveIdInList(prev, active.id as string, over.id as string)); |
There was a problem hiding this comment.
Similar to handleDragStart, using as string here is not type-safe. The IDs from the dnd-kit event should be explicitly converted to strings to ensure they match the expected type for moveIdInList.
setReorderList((prev) => moveIdInList(prev, String(active.id), String(over.id)));
| } | ||
| }; | ||
|
|
||
| const SortableDeviceCard: React.FC<{ device: AnyDevice }> = ({ device }) => { |
There was a problem hiding this comment.
For better code organization and maintainability, this SortableDeviceCard component should be extracted into its own file (e.g., src/renderer/src/components/SortableDeviceCard.tsx). This component contains significant presentational and behavioral logic, and placing it in a separate file will make DeviceListScreen cleaner and the new component more reusable.
| setDraggingId(id); | ||
| const handleDragStart = (event: DragStartEvent) => { | ||
| if (!isReorderMode) return; | ||
| setDraggingId(event.active.id as string); |
There was a problem hiding this comment.
Using as string for the event ID is not type-safe, as dnd-kit's UniqueIdentifier type is string | number. If the ID were ever a number, this would cause issues. It's safer to explicitly convert it to a string using String().
| setDraggingId(event.active.id as string); | |
| setDraggingId(String(event.active.id)); |
| } | ||
| const { active, over } = event; | ||
| if (over && active.id !== over.id) { | ||
| setReorderList((prev) => moveIdInList(prev, active.id as string, over.id as string)); |
There was a problem hiding this comment.
Similar to handleDragStart, using as string here is not type-safe. The IDs from the dnd-kit event should be explicitly converted to strings to ensure they match the expected type for moveIdInList.
setReorderList((prev) => moveIdInList(prev, String(active.id), String(over.id)));
| } | ||
| }; | ||
|
|
||
| const SortableSceneCard: React.FC<{ scene: SceneSummary; isExecuting: boolean; sceneError?: string }> |
There was a problem hiding this comment.
This SortableSceneCard component is quite large (over 100 lines) and contains significant logic. For better code organization, readability, and maintainability, it should be extracted into its own file (e.g., src/renderer/src/components/SortableSceneCard.tsx). This will make the ScenesScreen component much cleaner and easier to understand.
1 participant
Summary
Testing
Codex Task