feat(desktop): drag-and-drop reordering for v2 sidebar#3222
Merged
Conversation
Replace multi-container pattern with a single flat SortableContext. Sections are just headers/dividers — position determines membership. Workspaces before the first section are ungrouped, workspaces after a section header belong to that section. This eliminates all multi-container complexity: no onDragOver, no findContainer, no custom collision detection, no container key mismatches, no cross-container oscillation. Just arrayMove on a flat list + parse positions on drop to persist. Section drags move the header and all its workspaces as a group. Collapsed sections exclude their workspaces from the flat array.
When dragging a section (via grip), only section IDs are in the SortableContext. Workspaces inside sections are hidden visually. Sections reorder among each other only. On drop, the full flat list is rebuilt with workspaces in their correct positions. When dragging a workspace, the full flat list is in SortableContext. Sections are inert position references. Standard arrayMove + persist.
…st grouped ones" This reverts commit 69979a7.
Projects are sortable via DndContext at the sidebar level. On drag start, project content (sections + workspaces) animates out, leaving only project headers for clean sorting. On drop, arrayMove + persist via reorderProjects. DragOverlay shows a collapsed project preview. Uses the same pattern as section drag: project row gets drag handle via attributes/listeners spread, content collapses with AnimatePresence.
Section headers now render as: grip ── • Name (✎) ── ▸ - Horizontal rules (flex-1 bg-border) flank the name - Colored dot shows section color - Pencil appears on hover (replaces count) - Grip icon for drag handle on hover - Chevron for collapse toggle - Remove left border from SortableSectionHeader (no longer containers) - Center rename input between rules
- Section grip handle is the only drag trigger (not whole header) - Drag overlay matches ruled divider style for sections - Remove from Group in context menu (with up arrow, only for grouped workspaces) - Remove Default from color palette, sections always get random color - Color dots in Move to Section submenu with separator - Animated section collapse/expand via AnimatePresence - Predicted section color on ghost during workspace drag - field-sizing:content on section rename input
Revert section headers to the original look (name, count/pencil swap, chevron, colored left border) instead of the ruled divider style. Added grip icon that shows on hover with cursor-grab. Whole header is draggable, grip is just a visual affordance. Drag overlay matches.
- Show grip icon in section drag overlay - Remove active workspace transition animation (instant highlight) - Remove font-weight change on active workspace - Always show both workspace name and branch subtitle - Remove unused single-row layout code - Scroll active workspace into view on change - Fix Move to Section not visually updating (fingerprint now includes section membership) - Restore original section header style with colored left border
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds comprehensive drag-and-drop reordering to the dashboard sidebar: projects, sections, and workspaces are now sortable using Changes
Sequence DiagramsequenceDiagram
participant User
participant Sidebar as DashboardSidebar
participant Dnd as `@dnd-kit` (DndContext)
participant Hook as useSidebarDnd
participant State as useDashboardSidebarState
User->>Sidebar: start drag (project/section/workspace)
Sidebar->>Dnd: onDragStart
Dnd->>Hook: notify activeId/activeType via handlers
Hook->>Sidebar: provide activeItem, flatItems, sortableItems, handlers
Sidebar->>Dnd: render DragOverlay (portal) using Hook.activeItem
User->>Sidebar: hover over target
Sidebar->>Dnd: onDragOver
Dnd->>Hook: onDragOver -> compute overId & predictedColor
Hook->>Sidebar: update preview styling/position
User->>Sidebar: drop
Sidebar->>Dnd: onDragEnd
Dnd->>Hook: onDragEnd -> compute new order
Hook->>State: call reorderProjectChildren() or moveWorkspaceToSectionAtIndex()
State->>Sidebar: persisted ordering -> Sidebar re-renders
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR implements drag-and-drop reordering for workspaces, sections, and projects in the v2 desktop sidebar using @dnd-kit. The flat-list approach (sections as dividers in a single SortableContext) is a smart design that avoids multi-container complexity, and the two-mode drag system (workspace vs. section) cleanly separates concerns. Two bugs need attention before merge:
- "Remove from Group" tabOrder collision (
useDashboardSidebarState.ts): WhensectionId === null, every workspace removed from a group getsfirstSectionOrder - 1as its newtabOrder, causing ordering collisions when multiple workspaces are ungrouped in the same session. - External sync disrupts active drag (
useSidebarDnd.ts): The fingerprint-baseduseEffectthat syncsflatItemsfromprojectChildrendoes not early-exit whenactiveIdis set, so a background collection update (e.g., new workspace creation) mid-section-drag would snap the list back to server order.
Additional non-blocking improvements:
DashboardSidebar.tsxresetsprojectOrderon everygroupsreference change without a fingerprint guard (unlikeuseSidebarDnd).SortableWorkspaceItemonly spreadslistenersbut notattributesfromuseSortable, leaving the registeredKeyboardSensorunable to focus sortable workspace items.
Confidence Score: 3/5
Safety Assessment: Not safe to merge without fixing the 'Remove from Group' tabOrder collision and the mid-drag external sync reset.
Score Reasoning: Two P1 logic bugs: the tabOrder collision in moveWorkspaceToSection will cause unstable ordering after repeated 'Remove from Group' operations, and the missing activeId guard in the sync effect can disrupt in-progress drags on external data updates. The core flat-list DnD design is solid and the rest of the implementation is clean, but these two issues affect correctness of the primary user paths.
Files Needing Attention: useDashboardSidebarState.ts (moveWorkspaceToSection tabOrder logic) and useSidebarDnd.ts (external sync activeId guard)
File Analysis
| File | Confidence | Summary |
|---|---|---|
useSidebarDnd.ts |
3/5 | Core DnD state hook; flat-list design is solid but external sync doesn't guard against active drag |
useDashboardSidebarState.ts |
3/5 | New mutations look correct but 'Remove from Group' assigns a collision-prone tabOrder |
DashboardSidebar.tsx |
4/5 | Project-level DnD setup is clean; projectOrder sync effect lacks fingerprint guard |
DashboardSidebarExpandedProjectContent.tsx |
4/5 | Correctly wires useSidebarDnd into the render tree with animated show/hide for section drag mode |
SortableWorkspaceItem.tsx |
3/5 | Missing attributes spread means keyboard drag sensor cannot attach focus |
SortableSectionHeader.tsx |
4/5 | Correctly integrates drag handle with rename/collapse/context-menu state |
SidebarDragOverlay.tsx |
5/5 | Simple stateless overlay correctly renders workspace or section ghost |
DashboardSidebarWorkspaceContextMenu.tsx |
5/5 | Adds 'Remove from Group' menu item; context menu logic is clean |
DashboardSidebarExpandedWorkspaceRow.tsx |
5/5 | Adds scroll-into-view for active workspace and name+branch display; no issues |
DashboardSidebarSectionHeader.tsx |
5/5 | Added grip icon; correct forwardRef + props-spread integration with dnd-kit |
DashboardSidebarProjectSection.tsx |
5/5 | Correctly collapses project content during drag via isDraggingProject prop |
DashboardSidebarWorkspaceItem.tsx |
5/5 | Minor prop wiring for isInSection context menu; no issues |
project-colors.ts |
5/5 | Default removed from palette; PROJECT_COLOR_DEFAULT sentinel kept for null-checks |
project-colors.test.ts |
5/5 | New tests validate color uniqueness and visual distinctness; tests are correct |
useSidebarDnd/index.ts |
5/5 | Re-export barrel file |
SidebarDragOverlay/index.ts |
5/5 | Re-export barrel file |
SortableSectionHeader/index.ts |
5/5 | Re-export barrel file |
SortableWorkspaceItem/index.ts |
5/5 | Re-export barrel file |
DnD Flow Sequence Diagram
sequenceDiagram
participant User
participant DndContext
participant useSidebarDnd
participant SortableContext
participant commitToDb
participant Collections
User->>DndContext: Pointer down (distance ≥ 8px)
DndContext->>useSidebarDnd: onDragStart(active)
useSidebarDnd->>useSidebarDnd: setActiveId / clone flatItems
Note over SortableContext: Section drag → filter to sec:: IDs only<br/>Workspace drag → full flat list
User->>DndContext: Pointer move
DndContext->>useSidebarDnd: onDragOver(over)
useSidebarDnd->>useSidebarDnd: setOverId → predictedColor updates
User->>DndContext: Pointer up
DndContext->>useSidebarDnd: onDragEnd(active, over)
alt Section drag
useSidebarDnd->>useSidebarDnd: arrayMove(sectionIds)<br/>Rebuild full flatItems preserving workspace membership
else Workspace drag
useSidebarDnd->>useSidebarDnd: arrayMove(flatItems, oldIdx, overIdx)<br/>Section membership derived from position
end
useSidebarDnd->>commitToDb: parseFlatItems → topLevel + sections
commitToDb->>Collections: reorderProjectChildren(topLevel)
commitToDb->>Collections: moveWorkspaceToSectionAtIndex × n (per section workspace)
Collections-->>useSidebarDnd: projectChildren update triggers fingerprint check
Note over useSidebarDnd: If fingerprint unchanged → no reset<br/>If activeId set → no reset (missing guard)
User->>DndContext: ESC key
DndContext->>useSidebarDnd: onDragCancel
useSidebarDnd->>useSidebarDnd: restore clonedRef flatItems
Comment on lines
+261
to
+266
| let newTabOrder: number; | ||
| if (firstSectionOrder != null) { | ||
| // Place just before the first section | ||
| newTabOrder = firstSectionOrder - 1; | ||
| } else { | ||
| // No sections — append to end |
There was a problem hiding this comment.
[P1 - Logic Bug] tabOrder collision when removing workspaces from group
When sectionId === null ("Remove from Group"), all workspaces get assigned firstSectionOrder - 1 as their new tabOrder:
newTabOrder = firstSectionOrder - 1;If a user removes multiple workspaces from sections, every one of them gets the same tabOrder value. Since the display order depends on tabOrder sorting, their relative ordering becomes undefined and unstable — and may differ each time the list is re-rendered. A safer approach is to append after the last ungrouped workspace by using getNextTabOrder on ungrouped items, regardless of where sections are positioned:
Suggested change
| let newTabOrder: number; | |
| if (firstSectionOrder != null) { | |
| // Place just before the first section | |
| newTabOrder = firstSectionOrder - 1; | |
| } else { | |
| // No sections — append to end | |
| // No sections or placing before first section — append after last ungrouped workspace | |
| const ungroupedOrders = Array.from( | |
| collections.v2WorkspaceLocalState.state.values(), | |
| ) | |
| .filter( | |
| (item) => | |
| item.sidebarState.projectId === projectId && | |
| item.workspaceId !== workspaceId && | |
| item.sidebarState.sectionId === null, | |
| ) | |
| .map((item) => ({ tabOrder: item.sidebarState.tabOrder })); | |
| newTabOrder = getNextTabOrder(ungroupedOrders); |
Comment on lines
+142
to
+155
| const prevFingerprintRef = useRef(""); | ||
| useEffect(() => { | ||
| const fingerprint = projectChildren | ||
| .map((c) => | ||
| c.type === "workspace" | ||
| ? c.workspace.id | ||
| : `s:${c.section.id}:${c.section.workspaces.map((w) => w.id).join("|")}`, | ||
| ) | ||
| .join(","); | ||
| if (fingerprint !== prevFingerprintRef.current) { | ||
| prevFingerprintRef.current = fingerprint; | ||
| setFlatItems(buildFlatItems(projectChildren)); | ||
| } | ||
| }, [projectChildren]); |
There was a problem hiding this comment.
[P1 - Logic Bug] External sync can reset flatItems during active drag
The fingerprint-based effect will call setFlatItems(buildFlatItems(projectChildren)) even if a drag is currently in progress — for example, if a workspace creation event triggers a projectChildren update while the user is mid-section-drag. This would overwrite the locally-reordered flat list and snap items back to the server order, visually breaking the drag.
Add an early return when a drag is active:
Suggested change
| const prevFingerprintRef = useRef(""); | |
| useEffect(() => { | |
| const fingerprint = projectChildren | |
| .map((c) => | |
| c.type === "workspace" | |
| ? c.workspace.id | |
| : `s:${c.section.id}:${c.section.workspaces.map((w) => w.id).join("|")}`, | |
| ) | |
| .join(","); | |
| if (fingerprint !== prevFingerprintRef.current) { | |
| prevFingerprintRef.current = fingerprint; | |
| setFlatItems(buildFlatItems(projectChildren)); | |
| } | |
| }, [projectChildren]); | |
| useEffect(() => { | |
| if (activeId) return; // Don't reset during active drag | |
| const fingerprint = projectChildren | |
| .map((c) => | |
| c.type === "workspace" | |
| ? c.workspace.id | |
| : `s:${c.section.id}:${c.section.workspaces.map((w) => w.id).join("|")}`, | |
| ) | |
| .join(","); | |
| if (fingerprint !== prevFingerprintRef.current) { | |
| prevFingerprintRef.current = fingerprint; | |
| setFlatItems(buildFlatItems(projectChildren)); | |
| } | |
| }, [projectChildren, activeId]); |
Comment on lines
+106
to
+108
| useEffect(() => { | ||
| setProjectOrder(groups.map((p) => p.id)); | ||
| }, [groups]); |
There was a problem hiding this comment.
[P2 - Style] projectOrder resets unconditionally on any groups change
Unlike useSidebarDnd which uses a fingerprint check before resetting flatItems, this effect calls setProjectOrder on every groups reference change — including changes caused by workspace additions within a project that don't alter the project-level order. This creates a new array each render, triggering a redundant state update.
Consider applying the same fingerprint guard used in useSidebarDnd:
Suggested change
| useEffect(() => { | |
| setProjectOrder(groups.map((p) => p.id)); | |
| }, [groups]); | |
| const prevProjectFingerprintRef = useRef(""); | |
| useEffect(() => { | |
| const fingerprint = groups.map((p) => p.id).join(","); | |
| if (fingerprint !== prevProjectFingerprintRef.current) { | |
| prevProjectFingerprintRef.current = fingerprint; | |
| setProjectOrder(groups.map((p) => p.id)); | |
| } | |
| }, [groups]); |
Comment on lines
+23
to
+36
| const { setNodeRef, listeners, isDragging, transform, transition } = | ||
| useSortable({ id: sortableId }); | ||
|
|
||
| return ( | ||
| <div | ||
| ref={setNodeRef} | ||
| style={{ | ||
| transform: CSS.Translate.toString(transform), | ||
| transition, | ||
| opacity: isDragging ? 0.5 : undefined, | ||
| borderLeft: accentColor ? `2px solid ${accentColor}` : undefined, | ||
| }} | ||
| {...listeners} | ||
| > |
There was a problem hiding this comment.
[P2 - Style] Drag handle attributes not applied — breaks keyboard drag accessibility
useSortable returns both listeners (pointer events) and attributes (role="button", tabIndex, aria-* props needed for keyboard dragging). Only listeners is spread on the wrapper <div>, so the KeyboardSensor registered in useSidebarDnd cannot attach focus to this item and keyboard drag won't work. Either spread attributes as well, or explicitly document that keyboard drag is not intended for workspace rows.
Suggested change
| const { setNodeRef, listeners, isDragging, transform, transition } = | |
| useSortable({ id: sortableId }); | |
| return ( | |
| <div | |
| ref={setNodeRef} | |
| style={{ | |
| transform: CSS.Translate.toString(transform), | |
| transition, | |
| opacity: isDragging ? 0.5 : undefined, | |
| borderLeft: accentColor ? `2px solid ${accentColor}` : undefined, | |
| }} | |
| {...listeners} | |
| > | |
| const { setNodeRef, attributes, listeners, isDragging, transform, transition } = | |
| useSortable({ id: sortableId }); | |
| return ( | |
| <div | |
| ref={setNodeRef} | |
| style={{ | |
| transform: CSS.Translate.toString(transform), | |
| transition, | |
| opacity: isDragging ? 0.5 : undefined, | |
| borderLeft: accentColor ? `2px solid ${accentColor}` : undefined, | |
| }} | |
| {...attributes} | |
| {...listeners} | |
| > |
Greptile SummaryThis PR implements drag-and-drop reordering for workspaces, sections, and projects in the v2 desktop sidebar using
Additional non-blocking improvements:
Confidence Score: 3/5Not safe to merge without fixing the 'Remove from Group' tabOrder collision and the mid-drag external sync reset. Two P1 logic bugs: the tabOrder collision in moveWorkspaceToSection will cause unstable ordering after repeated 'Remove from Group' operations, and the missing activeId guard in the sync effect can disrupt in-progress drags on external data updates. The core flat-list DnD design is solid and the rest of the implementation is clean, but these two issues affect correctness of the primary user paths. useDashboardSidebarState.ts (moveWorkspaceToSection tabOrder logic) and useSidebarDnd.ts (external sync activeId guard) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant DndContext
participant useSidebarDnd
participant SortableContext
participant commitToDb
participant Collections
User->>DndContext: Pointer down (distance ≥ 8px)
DndContext->>useSidebarDnd: onDragStart(active)
useSidebarDnd->>useSidebarDnd: setActiveId / clone flatItems
Note over SortableContext: Section drag → filter to sec:: IDs only<br/>Workspace drag → full flat list
User->>DndContext: Pointer move
DndContext->>useSidebarDnd: onDragOver(over)
useSidebarDnd->>useSidebarDnd: setOverId → predictedColor updates
User->>DndContext: Pointer up
DndContext->>useSidebarDnd: onDragEnd(active, over)
alt Section drag
useSidebarDnd->>useSidebarDnd: arrayMove(sectionIds)<br/>Rebuild full flatItems preserving workspace membership
else Workspace drag
useSidebarDnd->>useSidebarDnd: arrayMove(flatItems, oldIdx, overIdx)<br/>Section membership derived from position
end
useSidebarDnd->>commitToDb: parseFlatItems → topLevel + sections
commitToDb->>Collections: reorderProjectChildren(topLevel)
commitToDb->>Collections: moveWorkspaceToSectionAtIndex × n (per section workspace)
Collections-->>useSidebarDnd: projectChildren update triggers fingerprint check
Note over useSidebarDnd: If fingerprint unchanged → no reset<br/>If activeId set → no reset (missing guard)
User->>DndContext: ESC key
DndContext->>useSidebarDnd: onDragCancel
useSidebarDnd->>useSidebarDnd: restore clonedRef flatItems
Reviews (1): Last reviewed commit: "feat(desktop): sidebar UX polish" | Re-trigger Greptile |
Comment on lines
+261
to
+266
| let newTabOrder: number; | ||
| if (firstSectionOrder != null) { | ||
| // Place just before the first section | ||
| newTabOrder = firstSectionOrder - 1; | ||
| } else { | ||
| // No sections — append to end |
There was a problem hiding this comment.
When sectionId === null ("Remove from Group"), all workspaces get assigned firstSectionOrder - 1 as their new tabOrder. If a user removes multiple workspaces from sections, every one of them gets the same tabOrder value, making their relative ordering undefined and unstable.
A safer approach is to append after the last ungrouped workspace using getNextTabOrder:
Suggested change
| let newTabOrder: number; | |
| if (firstSectionOrder != null) { | |
| // Place just before the first section | |
| newTabOrder = firstSectionOrder - 1; | |
| } else { | |
| // No sections — append to end | |
| const ungroupedOrders = Array.from( | |
| collections.v2WorkspaceLocalState.state.values(), | |
| ) | |
| .filter( | |
| (item) => | |
| item.sidebarState.projectId === projectId && | |
| item.workspaceId !== workspaceId && | |
| item.sidebarState.sectionId === null, | |
| ) | |
| .map((item) => ({ tabOrder: item.sidebarState.tabOrder })); | |
| newTabOrder = getNextTabOrder(ungroupedOrders); |
Comment on lines
+142
to
+155
| const prevFingerprintRef = useRef(""); | ||
| useEffect(() => { | ||
| const fingerprint = projectChildren | ||
| .map((c) => | ||
| c.type === "workspace" | ||
| ? c.workspace.id | ||
| : `s:${c.section.id}:${c.section.workspaces.map((w) => w.id).join("|")}`, | ||
| ) | ||
| .join(","); | ||
| if (fingerprint !== prevFingerprintRef.current) { | ||
| prevFingerprintRef.current = fingerprint; | ||
| setFlatItems(buildFlatItems(projectChildren)); | ||
| } | ||
| }, [projectChildren]); |
There was a problem hiding this comment.
flatItems during active drag
The fingerprint-based useEffect calls setFlatItems(buildFlatItems(projectChildren)) even if a drag is currently in progress. A background collection update (e.g., workspace creation) while the user is mid-section-drag would overwrite the locally-reordered flat list and snap items back to server order.
Add an early return when activeId is set:
Suggested change
| const prevFingerprintRef = useRef(""); | |
| useEffect(() => { | |
| const fingerprint = projectChildren | |
| .map((c) => | |
| c.type === "workspace" | |
| ? c.workspace.id | |
| : `s:${c.section.id}:${c.section.workspaces.map((w) => w.id).join("|")}`, | |
| ) | |
| .join(","); | |
| if (fingerprint !== prevFingerprintRef.current) { | |
| prevFingerprintRef.current = fingerprint; | |
| setFlatItems(buildFlatItems(projectChildren)); | |
| } | |
| }, [projectChildren]); | |
| useEffect(() => { | |
| if (activeId) return; // Don't reset during active drag | |
| const fingerprint = projectChildren | |
| .map((c) => | |
| c.type === "workspace" | |
| ? c.workspace.id | |
| : `s:${c.section.id}:${c.section.workspaces.map((w) => w.id).join("|")}` | |
| ) | |
| .join(","); | |
| if (fingerprint !== prevFingerprintRef.current) { | |
| prevFingerprintRef.current = fingerprint; | |
| setFlatItems(buildFlatItems(projectChildren)); | |
| } | |
| }, [projectChildren, activeId]); |
Comment on lines
+106
to
+108
| useEffect(() => { | ||
| setProjectOrder(groups.map((p) => p.id)); | ||
| }, [groups]); |
There was a problem hiding this comment.
projectOrder resets unconditionally on any groups change
Unlike useSidebarDnd which uses a fingerprint check before resetting flatItems, this effect calls setProjectOrder on every groups reference change — including changes caused by workspace additions within a project that don't alter project-level order. Consider applying the same fingerprint guard:
Suggested change
| useEffect(() => { | |
| setProjectOrder(groups.map((p) => p.id)); | |
| }, [groups]); | |
| const prevProjectFingerprintRef = useRef(""); | |
| useEffect(() => { | |
| const fingerprint = groups.map((p) => p.id).join(","); | |
| if (fingerprint !== prevProjectFingerprintRef.current) { | |
| prevProjectFingerprintRef.current = fingerprint; | |
| setProjectOrder(groups.map((p) => p.id)); | |
| } | |
| }, [groups]); |
Comment on lines
+23
to
+36
| const { setNodeRef, listeners, isDragging, transform, transition } = | ||
| useSortable({ id: sortableId }); | ||
|
|
||
| return ( | ||
| <div | ||
| ref={setNodeRef} | ||
| style={{ | ||
| transform: CSS.Translate.toString(transform), | ||
| transition, | ||
| opacity: isDragging ? 0.5 : undefined, | ||
| borderLeft: accentColor ? `2px solid ${accentColor}` : undefined, | ||
| }} | ||
| {...listeners} | ||
| > |
There was a problem hiding this comment.
attributes not applied — breaks keyboard drag accessibility
useSortable returns both listeners (pointer events) and attributes (role="button", tabIndex, aria-* props needed for keyboard dragging). Only listeners is spread on the wrapper <div>, so the KeyboardSensor registered in useSidebarDnd cannot attach focus to workspace items and keyboard drag won't work.
Suggested change
| const { setNodeRef, listeners, isDragging, transform, transition } = | |
| useSortable({ id: sortableId }); | |
| return ( | |
| <div | |
| ref={setNodeRef} | |
| style={{ | |
| transform: CSS.Translate.toString(transform), | |
| transition, | |
| opacity: isDragging ? 0.5 : undefined, | |
| borderLeft: accentColor ? `2px solid ${accentColor}` : undefined, | |
| }} | |
| {...listeners} | |
| > | |
| const { setNodeRef, attributes, listeners, isDragging, transform, transition } = | |
| useSortable({ id: sortableId }); | |
| return ( | |
| <div | |
| ref={setNodeRef} | |
| style={{ | |
| transform: CSS.Translate.toString(transform), | |
| transition, | |
| opacity: isDragging ? 0.5 : undefined, | |
| borderLeft: accentColor ? `2px solid ${accentColor}` : undefined, | |
| }} | |
| {...attributes} | |
| {...listeners} | |
| > |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
4 issues found across 18 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsx:106">
P1: This effect can overwrite `flatItems` during an active drag if `projectChildren` changes externally (e.g., a workspace is created while the user is mid-drag). This snaps items back to the server order and breaks the drag. Add an early return when `activeId` is set, and include `activeId` in the dependency array.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useSidebarDnd/useSidebarDnd.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useSidebarDnd/useSidebarDnd.ts:143">
P2: External data sync can overwrite local drag state mid-drag. If `projectChildren` changes while a drag is active (background sync, another window), `setFlatItems` resets the list and the in-progress drag will commit incorrect positions on drop. Guard the sync with the active drag state.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SortableWorkspaceItem/SortableWorkspaceItem.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SortableWorkspaceItem/SortableWorkspaceItem.tsx:23">
P2: `attributes` from `useSortable` is not destructured or spread on the wrapper `<div>`, so `role`, `tabIndex`, and `aria-*` props are missing. The `KeyboardSensor` registered in `useSidebarDnd` cannot focus this item, breaking keyboard-based drag. Destructure `attributes` and spread it alongside `listeners` (as already done in `SortableSectionHeader`).</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts:264">
P2: All workspaces removed from a group receive the same `tabOrder` value (`firstSectionOrder - 1`). If multiple workspaces are ungrouped, their relative ordering becomes undefined and unstable. Consider appending after the last ungrouped workspace instead (e.g., using `getNextTabOrder` on the current ungrouped items).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const [projectOrder, setProjectOrder] = useState(() => | ||
| groups.map((p) => p.id), | ||
| ); | ||
| useEffect(() => { |
Contributor
There was a problem hiding this comment.
P1: This effect can overwrite flatItems during an active drag if projectChildren changes externally (e.g., a workspace is created while the user is mid-drag). This snaps items back to the server order and breaks the drag. Add an early return when activeId is set, and include activeId in the dependency array.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsx, line 106:
<comment>This effect can overwrite `flatItems` during an active drag if `projectChildren` changes externally (e.g., a workspace is created while the user is mid-drag). This snaps items back to the server order and breaks the drag. Add an early return when `activeId` is set, and include `activeId` in the dependency array.</comment>
<file context>
@@ -1,34 +1,188 @@
+ const [projectOrder, setProjectOrder] = useState(() =>
+ groups.map((p) => p.id),
+ );
+ useEffect(() => {
+ setProjectOrder(groups.map((p) => p.id));
+ }, [groups]);
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsx (1)
63-84:⚠️ Potential issue | 🟠 MajorCollapsed project rows are missing sortable activator props.
The collapsed branch (lines 63–85) does not spread
dragHandleAttributesordragHandleListeners, preventing project reordering while the sidebar is collapsed. The expanded branch correctly spreads these props toDashboardSidebarProjectRow(lines 110–111).Suggested fix
- <div className={cn("border-b border-border last:border-b-0")}> + <div + className={cn("border-b border-border last:border-b-0")} + {...(dragHandleAttributes ?? {})} + {...(dragHandleListeners ?? {})} + >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsx` around lines 63 - 84, The collapsed branch of DashboardSidebarProjectSection is missing the sortable activator props; update the DashboardSidebarCollapsedProjectContent usage inside DashboardSidebarProjectContextMenu to pass through dragHandleAttributes and dragHandleListeners (the same props spread into DashboardSidebarProjectRow in the expanded branch) so collapsed project rows can be dragged; locate the collapsed JSX where DashboardSidebarCollapsedProjectContent is rendered and add the dragHandleAttributes and dragHandleListeners props (preserving onToggleCollapse, project.id, project.name and other existing props).
🧹 Nitpick comments (5)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx (1)
70-77: Consider guarding scroll-into-view to avoid unwanted scrolling.The effect scrolls the row into view whenever
isActivebecomes true. This may cause unwanted scroll jumps if the user has deliberately scrolled elsewhere in the sidebar and then switches workspaces via keyboard shortcut or external action. Consider adding a flag or debounce to only scroll on initial activation or user-initiated navigation.That said, if the intended UX is to always keep the active workspace visible, this is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx` around lines 70 - 77, The effect that calls localRef.current?.scrollIntoView in useEffect([isActive]) can cause unwanted scroll jumps; update DashboardSidebarExpandedWorkspaceRow to guard this behavior by tracking whether the activation is user-initiated or is the component's initial activation (e.g., add a ref/flag like hasScrolledOrInitialRef or onlyScrollOnFirstActivate and/or accept a prop such as userInitiatedNavigation), and only call scrollIntoView when that flag indicates initial activation or a user-initiated change; ensure the guard is consulted inside the existing useEffect that references isActive and localRef so you avoid always invoking scrollIntoView on every programmatic activation.apps/desktop/src/shared/constants/project-colors.test.ts (1)
31-31: Weak assertion after removing default color check.The assertion
expect(PROJECT_COLORS.length).toBeGreaterThan(0)is very weak and doesn't add meaningful coverage beyond what the uniqueness checks on lines 29-30 already provide (if colors are unique and sets match array length, the array implicitly has content).Consider either:
- Removing this line entirely since the uniqueness assertions already implicitly verify non-empty
- Adding a more meaningful assertion (e.g., minimum expected color count)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/shared/constants/project-colors.test.ts` at line 31, The test contains a weak assertion using PROJECT_COLORS.length that is redundant given the uniqueness checks; remove the line `expect(PROJECT_COLORS.length).toBeGreaterThan(0);` from apps/desktop/src/shared/constants/project-colors.test.ts, or if you prefer stricter coverage replace it with a meaningful minimum-size assertion such as `expect(PROJECT_COLORS.length).toBeGreaterThanOrEqual(<MIN_EXPECTED_COUNT>)` (using a concrete number) to enforce an expected palette size while keeping the uniqueness checks for PROJECT_COLORS intact.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useSidebarDnd/useSidebarDnd.ts (2)
141-155: Fingerprint-based sync is robust but could conflict during active drag.If
projectChildrenchanges externally while a drag is in progress (e.g., another client syncs changes), this effect will overwriteflatItemsmid-drag, potentially causing inconsistent state. Consider guarding against updates whileactiveIdis set:♻️ Suggested guard
useEffect(() => { + if (activeId) return; // Don't sync while dragging const fingerprint = projectChildren .map((c) => c.type === "workspace" ? c.workspace.id : `s:${c.section.id}:${c.section.workspaces.map((w) => w.id).join("|")}`, ) .join(","); if (fingerprint !== prevFingerprintRef.current) { prevFingerprintRef.current = fingerprint; setFlatItems(buildFlatItems(projectChildren)); } - }, [projectChildren]); + }, [projectChildren, activeId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useSidebarDnd/useSidebarDnd.ts` around lines 141 - 155, The effect that syncs flatItems from projectChildren (using prevFingerprintRef, fingerprint, buildFlatItems and setFlatItems) can overwrite UI state during an active drag; guard the update by checking the drag state (activeId) and skip syncing while activeId is non-empty so external changes don't clobber in-progress drags, i.e., in the useEffect that depends on projectChildren, return early if activeId is set before computing/updating fingerprint and setFlatItems, and still update prevFingerprintRef when appropriate after the drag ends to avoid stale diffs.
246-261: Loop causes redundant updates to sibling workspaces.As noted in the other file's review, calling
moveWorkspaceToSectionAtIndexin a loop (lines 254-257) results in O(n²) updates. Each call re-queries and updates all sibling workspaces.If you add a batch function to
useDashboardSidebarState, update this to:- for (let i = 0; i < wsIds.length; i++) { - moveWorkspaceToSectionAtIndex(wsIds[i], projectId, sectionId, i); - } + setWorkspacesInSection(wsIds, projectId, sectionId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useSidebarDnd/useSidebarDnd.ts` around lines 246 - 261, The loop in commitToDb causes redundant O(n²) updates because moveWorkspaceToSectionAtIndex is invoked per-workspace; update commitToDb to use a new batched API on useDashboardSidebarState (e.g., add a function like moveWorkspacesToSectionAtIndexes or batchMoveWorkspacesToSection) and call that for each section instead of the inner loop: keep the top-level reorderProjectChildren(projectId, parsed.topLevel) call, then for each [sectionId, wsIds] call the new batch function once with (sectionId, projectId, wsIds) so sibling workspaces are updated in a single batched operation; ensure you add and wire the batch function alongside moveWorkspaceToSectionAtIndex in the hook dependencies.apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts (1)
152-182: Quadratic update complexity when called in a loop.This function updates all sibling workspaces on each call. When called from
commitToDbinuseSidebarDnd.ts(lines 254-257), which iterates over every workspace in a section, you get O(n²) database updates for n workspaces.Consider adding a batch variant that accepts the full ordered list of workspace IDs for a section and performs a single pass of updates:
♻️ Suggested batch function
+ const setWorkspacesInSection = useCallback( + (workspaceIds: string[], projectId: string, sectionId: string) => { + workspaceIds.forEach((workspaceId, index) => { + if (!collections.v2WorkspaceLocalState.get(workspaceId)) return; + collections.v2WorkspaceLocalState.update(workspaceId, (draft) => { + draft.sidebarState.tabOrder = index + 1; + draft.sidebarState.sectionId = sectionId; + draft.sidebarState.projectId = projectId; + }); + }); + }, + [collections], + );Then in
useSidebarDnd.ts, replace the inner loop:- for (let i = 0; i < wsIds.length; i++) { - moveWorkspaceToSectionAtIndex(wsIds[i], projectId, sectionId, i); - } + setWorkspacesInSection(wsIds, projectId, sectionId);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts` around lines 152 - 182, The current moveWorkspaceToSectionAtIndex causes O(n²) updates because it re-writes all siblings on each call; add a new batch function (e.g., moveWorkspacesToSectionInOrder or batchUpdateWorkspacesSection) that accepts the sectionId, projectId and an ordered array of workspaceIds and then performs a single pass updating each workspace's sidebarState.tabOrder/sectionId/projectId once via collections.v2WorkspaceLocalState.update; replace the per-item loop in useSidebarDnd.ts (commitToDb) to call this batch function with the full ordered list for the section to eliminate repeated sibling scans and reduce updates to O(n).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsx`:
- Around line 115-117: The drag ghost never gets the computed predictedColor
because only the source row uses it; update the drag overlay propagation by
computing predictedColor where activeId/id/group?.color are available (in
DashboardSidebarExpandedProjectContent) and pass that value into
SidebarDragOverlay (in addition to activeItem); then update
SidebarDragOverlay/DragOverlay to accept a predictedColor prop and apply it to
the ghost's border/workspace styling so the cursor-following ghost matches the
destination section color (also repeat this change for the other occurrence
around lines 133-136 where the overlay is rendered).
- Around line 95-101: The current hidden calculation only hides workspaces that
belong to a section (isInSection) when activeType === "section", leaving
ungrouped workspaces visible during section-drag; update the hidden logic in
DashboardSidebarExpandedProjectContent (the block computing group via
groupInfo.get(parsed.realId), isInSection, isInCollapsedSection and hidden) so
that workspaces are also hidden whenever activeType === "section" (e.g. hidden =
isInCollapsedSection || activeType === "section"), ensuring all workspace rows
are suppressed during section-drag mode.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsx`:
- Around line 110-115: The dot is rendered whenever section.color is truthy,
which also matches the legacy sentinel value (PROJECT_COLOR_DEFAULT or the
string "default") and results in an invalid backgroundColor; in
DashboardSidebarWorkspaceContextMenu update the rendering guard to only show and
style the dot when section.color is present and not the sentinel (e.g., check
section.color && section.color !== PROJECT_COLOR_DEFAULT && section.color !==
'default' or extract an isValidSectionColor(section.color) helper) so the dot
spacing is only reserved for real colors.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SortableSectionHeader/SortableSectionHeader.tsx`:
- Around line 59-62: When starting rename from the context menu, reset the
rename input state so stale pre-trimmed values don't reappear: update the
onRename handler passed into DashboardSidebarSectionContextMenu (the callback
that currently only calls setIsRenaming(true)) to also call setRenameValue(...)
with the current section name (use section.name.trim() to match
handleSubmitRename’s trimming) before toggling isRenaming; this ensures
renameValue is in sync when the rename UI opens.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SortableWorkspaceItem/SortableWorkspaceItem.tsx`:
- Around line 23-35: The wrapper div using useSortable({ id: sortableId })
applies setNodeRef and {...listeners} but omits the accessibility props returned
by useSortable; include the attributes object returned by useSortable (spread
{...attributes} alongside {...listeners} on the same element) so setNodeRef,
listeners, attributes, transform, transition and isDragging are all applied to
the div and keyboard/ARIA-based sorting works correctly (locate useSortable,
setNodeRef, listeners, and where the div is rendered in SortableWorkspaceItem).
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts`:
- Around line 258-264: The code sets newTabOrder = firstSectionOrder - 1 which
can produce 0 or negative/duplicate orders; update the logic in
useDashboardSidebarState where firstSectionOrder/newTabOrder are computed so
newTabOrder is normalized (e.g., newTabOrder = Math.max(1, firstSectionOrder -
1)) and, after inserting, ensure no immediate collision by shifting or
normalizing surrounding tabOrder values so all tabOrder values remain 1-based
sequential (adjust neighboring tabs or run a small normalization pass to
reassign sequential orders if needed).
---
Outside diff comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsx`:
- Around line 63-84: The collapsed branch of DashboardSidebarProjectSection is
missing the sortable activator props; update the
DashboardSidebarCollapsedProjectContent usage inside
DashboardSidebarProjectContextMenu to pass through dragHandleAttributes and
dragHandleListeners (the same props spread into DashboardSidebarProjectRow in
the expanded branch) so collapsed project rows can be dragged; locate the
collapsed JSX where DashboardSidebarCollapsedProjectContent is rendered and add
the dragHandleAttributes and dragHandleListeners props (preserving
onToggleCollapse, project.id, project.name and other existing props).
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsx`:
- Around line 70-77: The effect that calls localRef.current?.scrollIntoView in
useEffect([isActive]) can cause unwanted scroll jumps; update
DashboardSidebarExpandedWorkspaceRow to guard this behavior by tracking whether
the activation is user-initiated or is the component's initial activation (e.g.,
add a ref/flag like hasScrolledOrInitialRef or onlyScrollOnFirstActivate and/or
accept a prop such as userInitiatedNavigation), and only call scrollIntoView
when that flag indicates initial activation or a user-initiated change; ensure
the guard is consulted inside the existing useEffect that references isActive
and localRef so you avoid always invoking scrollIntoView on every programmatic
activation.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useSidebarDnd/useSidebarDnd.ts`:
- Around line 141-155: The effect that syncs flatItems from projectChildren
(using prevFingerprintRef, fingerprint, buildFlatItems and setFlatItems) can
overwrite UI state during an active drag; guard the update by checking the drag
state (activeId) and skip syncing while activeId is non-empty so external
changes don't clobber in-progress drags, i.e., in the useEffect that depends on
projectChildren, return early if activeId is set before computing/updating
fingerprint and setFlatItems, and still update prevFingerprintRef when
appropriate after the drag ends to avoid stale diffs.
- Around line 246-261: The loop in commitToDb causes redundant O(n²) updates
because moveWorkspaceToSectionAtIndex is invoked per-workspace; update
commitToDb to use a new batched API on useDashboardSidebarState (e.g., add a
function like moveWorkspacesToSectionAtIndexes or batchMoveWorkspacesToSection)
and call that for each section instead of the inner loop: keep the top-level
reorderProjectChildren(projectId, parsed.topLevel) call, then for each
[sectionId, wsIds] call the new batch function once with (sectionId, projectId,
wsIds) so sibling workspaces are updated in a single batched operation; ensure
you add and wire the batch function alongside moveWorkspaceToSectionAtIndex in
the hook dependencies.
In
`@apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts`:
- Around line 152-182: The current moveWorkspaceToSectionAtIndex causes O(n²)
updates because it re-writes all siblings on each call; add a new batch function
(e.g., moveWorkspacesToSectionInOrder or batchUpdateWorkspacesSection) that
accepts the sectionId, projectId and an ordered array of workspaceIds and then
performs a single pass updating each workspace's
sidebarState.tabOrder/sectionId/projectId once via
collections.v2WorkspaceLocalState.update; replace the per-item loop in
useSidebarDnd.ts (commitToDb) to call this batch function with the full ordered
list for the section to eliminate repeated sibling scans and reduce updates to
O(n).
In `@apps/desktop/src/shared/constants/project-colors.test.ts`:
- Line 31: The test contains a weak assertion using PROJECT_COLORS.length that
is redundant given the uniqueness checks; remove the line
`expect(PROJECT_COLORS.length).toBeGreaterThan(0);` from
apps/desktop/src/shared/constants/project-colors.test.ts, or if you prefer
stricter coverage replace it with a meaningful minimum-size assertion such as
`expect(PROJECT_COLORS.length).toBeGreaterThanOrEqual(<MIN_EXPECTED_COUNT>)`
(using a concrete number) to enforce an expected palette size while keeping the
uniqueness checks for PROJECT_COLORS intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82f5d3fa-2a6b-40d1-9f47-9b808ada0c40
📒 Files selected for processing (18)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/DashboardSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/DashboardSidebarProjectSection.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarSection/components/DashboardSidebarSectionHeader/DashboardSidebarSectionHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/DashboardSidebarWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarExpandedWorkspaceRow/DashboardSidebarExpandedWorkspaceRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SidebarDragOverlay/SidebarDragOverlay.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SidebarDragOverlay/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SortableSectionHeader/SortableSectionHeader.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SortableSectionHeader/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SortableWorkspaceItem/SortableWorkspaceItem.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SortableWorkspaceItem/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useSidebarDnd/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useSidebarDnd/useSidebarDnd.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.tsapps/desktop/src/shared/constants/project-colors.test.tsapps/desktop/src/shared/constants/project-colors.ts
Comment on lines
+95
to
+101
| const group = groupInfo.get(parsed.realId); | ||
| const isInSection = !!group; | ||
| const isInCollapsedSection = | ||
| isInSection && collapsedSectionIds.has(group.sectionId); | ||
| const hidden = | ||
| isInCollapsedSection || | ||
| (activeType === "section" && isInSection); |
Contributor
There was a problem hiding this comment.
Section-drag mode still leaves ungrouped workspaces in the list.
hidden only suppresses workspaces that already belong to a section. Rows before the first section stay mounted during a section drag, so the supposed sections-only mode still has workspace entries occupying space at the top of the list.
Suggested fix
- const hidden =
- isInCollapsedSection ||
- (activeType === "section" && isInSection);
+ const hidden =
+ activeType === "section" || isInCollapsedSection;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsx`
around lines 95 - 101, The current hidden calculation only hides workspaces that
belong to a section (isInSection) when activeType === "section", leaving
ungrouped workspaces visible during section-drag; update the hidden logic in
DashboardSidebarExpandedProjectContent (the block computing group via
groupInfo.get(parsed.realId), isInSection, isInCollapsedSection and hidden) so
that workspaces are also hidden whenever activeType === "section" (e.g. hidden =
isInCollapsedSection || activeType === "section"), ensuring all workspace rows
are suppressed during section-drag mode.
Comment on lines
+115
to
+117
| accentColor={ | ||
| activeId === id ? predictedColor : group?.color | ||
| } |
Contributor
There was a problem hiding this comment.
predictedColor never reaches the drag ghost.
The computed color only styles the source row. The DragOverlay still receives just activeItem, so the cursor-following ghost never reflects the destination section color.
Suggested fix
- {activeId ? (
- <SidebarDragOverlay activeItem={activeItem} />
+ {activeId ? (
+ <SidebarDragOverlay
+ activeItem={activeItem}
+ accentColor={
+ activeType === "workspace"
+ ? predictedColor
+ : undefined
+ }
+ />
) : null}SidebarDragOverlay will need a matching prop to apply the workspace border.
Also applies to: 133-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarProjectSection/components/DashboardSidebarExpandedProjectContent/DashboardSidebarExpandedProjectContent.tsx`
around lines 115 - 117, The drag ghost never gets the computed predictedColor
because only the source row uses it; update the drag overlay propagation by
computing predictedColor where activeId/id/group?.color are available (in
DashboardSidebarExpandedProjectContent) and pass that value into
SidebarDragOverlay (in addition to activeItem); then update
SidebarDragOverlay/DragOverlay to accept a predictedColor prop and apply it to
the ghost's border/workspace styling so the cursor-following ghost matches the
destination section color (also repeat this change for the other occurrence
around lines 133-136 where the overlay is rendered).
Comment on lines
+110
to
+115
| {section.color && ( | ||
| <span | ||
| className="size-2 shrink-0 rounded-full mr-2" | ||
| style={{ backgroundColor: section.color }} | ||
| /> | ||
| )} |
Contributor
There was a problem hiding this comment.
Ignore the "default" sentinel before painting the section dot.
This truthy check also matches PROJECT_COLOR_DEFAULT, so legacy sections can produce an invalid backgroundColor here and still reserve the dot spacing.
Suggested fix
+import { PROJECT_COLOR_DEFAULT } from "shared/constants/project-colors";
@@
- {section.color && (
+ {section.color != null &&
+ section.color !== PROJECT_COLOR_DEFAULT && (
<span
className="size-2 shrink-0 rounded-full mr-2"
style={{ backgroundColor: section.color }}
/>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {section.color && ( | |
| <span | |
| className="size-2 shrink-0 rounded-full mr-2" | |
| style={{ backgroundColor: section.color }} | |
| /> | |
| )} | |
| import { PROJECT_COLOR_DEFAULT } from "shared/constants/project-colors"; | |
| // ... existing code ... | |
| {section.color != null && | |
| section.color !== PROJECT_COLOR_DEFAULT && ( | |
| <span | |
| className="size-2 shrink-0 rounded-full mr-2" | |
| style={{ backgroundColor: section.color }} | |
| /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarWorkspaceItem/components/DashboardSidebarWorkspaceContextMenu/DashboardSidebarWorkspaceContextMenu.tsx`
around lines 110 - 115, The dot is rendered whenever section.color is truthy,
which also matches the legacy sentinel value (PROJECT_COLOR_DEFAULT or the
string "default") and results in an invalid backgroundColor; in
DashboardSidebarWorkspaceContextMenu update the rendering guard to only show and
style the dot when section.color is present and not the sentinel (e.g., check
section.color && section.color !== PROJECT_COLOR_DEFAULT && section.color !==
'default' or extract an isValidSectionColor(section.color) helper) so the dot
spacing is only reserved for real colors.
Comment on lines
+59
to
+62
| <DashboardSidebarSectionContextMenu | ||
| color={section.color} | ||
| onRename={() => setIsRenaming(true)} | ||
| onSetColor={(color) => setSectionColor(section.id, color)} |
Contributor
There was a problem hiding this comment.
Reset renameValue when rename starts from the context menu.
handleSubmitRename() trims before saving, but this path only flips isRenaming. Reopening rename after "Name " will show the stale pre-trimmed value instead of the current section name.
Suggested fix
- onRename={() => setIsRenaming(true)}
+ onRename={() => {
+ setRenameValue(section.name);
+ setIsRenaming(true);
+ }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/SortableSectionHeader/SortableSectionHeader.tsx`
around lines 59 - 62, When starting rename from the context menu, reset the
rename input state so stale pre-trimmed values don't reappear: update the
onRename handler passed into DashboardSidebarSectionContextMenu (the callback
that currently only calls setIsRenaming(true)) to also call setRenameValue(...)
with the current section name (use section.name.trim() to match
handleSubmitRename’s trimming) before toggling isRenaming; this ensures
renameValue is in sync when the rename UI opens.
- Fix tabOrder collision when removing multiple workspaces from groups: use getNextTabOrder on ungrouped items instead of firstSectionOrder-1 - Guard flatItems sync effect with activeId to prevent overwriting local drag state during background collection updates - Add missing useSortable attributes to SortableWorkspaceItem for keyboard drag accessibility
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts:274">
P2: `getNextTabOrder(ungroupedOrders)` can equal `firstSectionOrder`, so the “remove from group” path assigns the same tabOrder as the first section. That breaks the “before first section” guarantee and can make ordering unstable in the flat list.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| item.sidebarState.sectionId === null, | ||
| ) | ||
| .map((item) => ({ tabOrder: item.sidebarState.tabOrder })); | ||
| newTabOrder = getNextTabOrder(ungroupedOrders); |
Contributor
There was a problem hiding this comment.
P2: getNextTabOrder(ungroupedOrders) can equal firstSectionOrder, so the “remove from group” path assigns the same tabOrder as the first section. That breaks the “before first section” guarantee and can make ordering unstable in the flat list.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/hooks/useDashboardSidebarState/useDashboardSidebarState.ts, line 274:
<comment>`getNextTabOrder(ungroupedOrders)` can equal `firstSectionOrder`, so the “remove from group” path assigns the same tabOrder as the first section. That breaks the “before first section” guarantee and can make ordering unstable in the flat list.</comment>
<file context>
@@ -260,8 +260,18 @@ export function useDashboardSidebarState() {
+ item.sidebarState.sectionId === null,
+ )
+ .map((item) => ({ tabOrder: item.sidebarState.tabOrder }));
+ newTabOrder = getNextTabOrder(ungroupedOrders);
} else {
// No sections — append to end
</file context>
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
MocA-Love
pushed a commit
to MocA-Love/superset
that referenced
this pull request
Apr 10, 2026
) * feat(desktop): rewrite sidebar DnD with flat list approach Replace multi-container pattern with a single flat SortableContext. Sections are just headers/dividers — position determines membership. Workspaces before the first section are ungrouped, workspaces after a section header belong to that section. This eliminates all multi-container complexity: no onDragOver, no findContainer, no custom collision detection, no container key mismatches, no cross-container oscillation. Just arrayMove on a flat list + parse positions on drop to persist. Section drags move the header and all its workspaces as a group. Collapsed sections exclude their workspaces from the flat array. * feat(desktop): separate section and workspace drag modes When dragging a section (via grip), only section IDs are in the SortableContext. Workspaces inside sections are hidden visually. Sections reorder among each other only. On drop, the full flat list is rebuilt with workspaces in their correct positions. When dragging a workspace, the full flat list is in SortableContext. Sections are inert position references. Standard arrayMove + persist. * feat(desktop): animate section workspace collapse on section drag start * fix(desktop): hide all workspaces during section drag, not just grouped ones * Revert "fix(desktop): hide all workspaces during section drag, not just grouped ones" This reverts commit 69979a7. * feat(desktop): add project-level drag and drop reordering Projects are sortable via DndContext at the sidebar level. On drag start, project content (sections + workspaces) animates out, leaving only project headers for clean sorting. On drop, arrayMove + persist via reorderProjects. DragOverlay shows a collapsed project preview. Uses the same pattern as section drag: project row gets drag handle via attributes/listeners spread, content collapses with AnimatePresence. * fix(desktop): disable project drop animation to avoid size mismatch * fix(desktop): disable section drop animation to match project behavior * feat(desktop): ghost shows predicted section color during workspace drag * fix(desktop): correct predicted color when hovering above a section header * feat(desktop): redesign section headers as ruled dividers Section headers now render as: grip ── • Name (✎) ── ▸ - Horizontal rules (flex-1 bg-border) flank the name - Colored dot shows section color - Pencil appears on hover (replaces count) - Grip icon for drag handle on hover - Chevron for collapse toggle - Remove left border from SortableSectionHeader (no longer containers) - Center rename input between rules * fix(desktop): auto-size section rename input to text width * fix(desktop): prevent layout shift on section rename by keeping count invisible * fix(desktop): replace pencil/count with separator line during section rename * fix(desktop): contiguous right rule during section rename * fix(desktop): tighten section rename input width for longer right rule * feat(desktop): polish sidebar DnD UI - Section grip handle is the only drag trigger (not whole header) - Drag overlay matches ruled divider style for sections - Remove from Group in context menu (with up arrow, only for grouped workspaces) - Remove Default from color palette, sections always get random color - Color dots in Move to Section submenu with separator - Animated section collapse/expand via AnimatePresence - Predicted section color on ghost during workspace drag - field-sizing:content on section rename input * feat(desktop): restore original section header style with grip icon Revert section headers to the original look (name, count/pencil swap, chevron, colored left border) instead of the ruled divider style. Added grip icon that shows on hover with cursor-grab. Whole header is draggable, grip is just a visual affordance. Drag overlay matches. * feat(desktop): sidebar UX polish - Show grip icon in section drag overlay - Remove active workspace transition animation (instant highlight) - Remove font-weight change on active workspace - Always show both workspace name and branch subtitle - Remove unused single-row layout code - Scroll active workspace into view on change - Fix Move to Section not visually updating (fingerprint now includes section membership) - Restore original section header style with colored left border * fix(desktop): address PR review comments - Fix tabOrder collision when removing multiple workspaces from groups: use getNextTabOrder on ungrouped items instead of firstSectionOrder-1 - Guard flatItems sync effect with activeId to prevent overwriting local drag state during background collection updates - Add missing useSortable attributes to SortableWorkspaceItem for keyboard drag accessibility
9 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
Key design decisions
SortableContext, not containers. This eliminates all multi-container complexity (noonDragOver, nofindContainer, no custom collision detection)Changes
useSidebarDndhook with flat list state managementSortableSectionHeader,SortableWorkspaceItem,SidebarDragOverlaycomponentsreorderProjectChildrenandmoveWorkspaceToSectionAtIndexmutationsDndContextinDashboardSidebar.tsxTest plan
Summary by cubic
Adds drag-and-drop reordering for projects, sections, and workspaces in the v2 sidebar using
@dnd-kitwith a flat list model where position defines group membership. This simplifies DnD, improves keyboard accessibility, and smooths reordering animations.New Features
Bug Fixes
useSortableattributes to workspace items for full keyboard DnD support.Written for commit 5a61525. Summary will update on new commits.
Summary by CodeRabbit
New Features
UI/UX Improvements