Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request migrates the WorktreeItem component from DND-kit-based drag-and-drop to react-arborist for hierarchical tab and group rendering. Three new dependencies are added, and the MainScreen root is wrapped with DndProvider. Event handling is updated to prevent drag conflicts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Tree as React Arborist<br/>Tree Component
participant WorktreeItem as WorktreeItem<br/>Component
participant IPC as IPC Handler
participant State as Local State
User->>Tree: Drag tab/group node
Tree->>WorktreeItem: onMove(node, parent)
WorktreeItem->>WorktreeItem: convertTreeDataToTabs()
WorktreeItem->>IPC: tab-move / tab-reorder
IPC-->>WorktreeItem: Success
WorktreeItem->>WorktreeItem: onReload() / onTabSelect()
WorktreeItem->>State: Update treeData & expanded
State-->>Tree: Re-render with new structure
User->>Tree: Click group node
Tree->>WorktreeItem: onToggle(node)
WorktreeItem->>State: Update expanded IDs
State-->>Tree: Re-render (expand/collapse)
User->>Tree: Right-click group/tab
Tree->>WorktreeItem: ContextMenu action
WorktreeItem->>IPC: Rename / Ungroup / Move
IPC-->>WorktreeItem: Success
WorktreeItem->>State: Refresh tree data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
Tip 📝 Customizable high-level summaries are now available!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example:
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx (1)
1272-1279: DndContext/SortableContext wiring is mostly good, butitems={allTabIds}includes non-sortable IDs.Wrapping the tab list in:
<DndContext onDragEnd={handleDragEnd}> <SortableContext items={allTabIds} strategy={verticalListSortingStrategy}> {tabs.map((tab) => renderTab(tab, undefined, 0))} </SortableContext> </DndContext>is the right overall structure. One nuance:
allTabIdscomes fromgetAllTabs(tabs)and includes both group and non-group tabs, but only non-group tabs are wrapped inSortableTab/useSortable. That can be a bit confusing and may lead to dnd-kit warnings or unexpected behavior if it expects every id initemsto correspond to a sortable element.If groups are not themselves draggable, consider restricting
itemsto just the non-group tab ids:const allTabIds = allTabsFlat .filter(({ tab }) => tab.type !== "group") .map(({ tab }) => tab.id);This makes it explicit which nodes are sortable and keeps the items list aligned with the actual
useSortableinstances.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx(8 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/components/TabItem/TabItem.tsx(2 hunks)
🔇 Additional comments (4)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx (4)
1-5: dnd-kit imports are appropriate for the new drag-and-drop orchestration.Bringing in
DndContext,DragEndEvent, anduseDroppablehere aligns with the new cross-tab DnD behavior and keeps the concerns localized toWorktreeItem.
149-155: DroppableGroupTab: droppable wiring and hover styling look coherent.
useDroppablewithid: "group-" + tab.idanddata: { type: "group", groupTabId }matches the checks inhandleDragEnd, and theisOver-based styling branch (blue highlight + border) provides a clear affordance when dragging over a group header.No issues from a correctness standpoint; just ensure that
tab.idis unique across the worktree and consistent with any backend expectations for group identifiers.Also applies to: 210-213
260-285: DroppableGroupArea: implementation is consistent and gives good UX feedback.The group content droppable area:
- Uses a distinct id prefix (
group-area-${groupTabId}) anddata: { type: "group-area", groupTabId }that line up with thehandleDragEndlogic.- Applies a subtle background + left border when
isOverand shows an overlay “Drop here to add to group” message, which should make the target obvious.The way you wrap the nested tabs inside
<DroppableGroupArea>also keeps the droppable region scoped correctly to expanded groups. Looks solid.Also applies to: 1229-1235
97-102: Drag-to-reorder listeners may be blocked by TabItem's stopPropagation on mousedown.The concern is architecturally valid:
handleMouseDownin TabItem.tsx callse.stopPropagation()on the button, which prevents the event from bubbling to the parent<div>where the dnd-kitlistenersare attached. While the code comment indicates this was intentional to prevent unintended drags during clicks, this creates a potential fragility if dnd-kit's drag initialization depends on receivingmousedownor pointer events that should bubble.If drag-to-reorder is currently working, it may be relying on other event paths (e.g.,
pointerdownwith different propagation behavior), but the current architecture conflates two concerns: preventing UI interactions from triggering drag AND blocking drag's event flow.Consider either:
- Extracting a dedicated drag handle element without
stopPropagation- Using event delegation or pointer-specific listeners to bypass the
mousedownblock- Verifying dnd-kit's event requirements align with the current event flow
| const handleMouseDown = (e: React.MouseEvent) => { | ||
| // Stop propagation to prevent drag from starting when clicking the button | ||
| e.stopPropagation(); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Drag-start suppression may either be ineffective (PointerSensor) or disable tab drag entirely (MouseSensor).
You’re now stopping propagation for both onMouseDown and onClick on the main tab <button>. Because the dnd-kit useSortable listeners are attached to the parent wrapper <div>, this has a couple of implications:
- If your sensor stack uses
MouseSensor,onMouseDown={handleMouseDown}on the button will prevent the parent’s mouse-down handler from ever seeing the event, which can effectively disable drag start from anywhere inside the tab. - If you use
PointerSensor(the dnd-kit default), you’re only interceptingonMouseDown, notonPointerDown, so pointer-based drags may still start even though you intended to prevent them.
This is worth double-checking in the actual app configuration; otherwise you may either still see unintended drags or accidentally lose drag-to-reorder entirely.
Consider one of:
- Only stopping propagation for specific interactive sub-elements (e.g., the close button, rename input) rather than the entire tab button, or
- Explicitly wiring dnd-kit listeners to a smaller “drag handle” element that you don’t intercept.
Also applies to: 72-73, 154-154
🏁 Script executed:
#!/bin/bash
# Find and examine TabItem.tsx
fd -t f "TabItem.tsx$" --exec cat {}Length of output: 5335
🏁 Script executed:
#!/bin/bash
# Find and examine the parent WorktreeItem.tsx
fd -t f "WorktreeItem.tsx$" --exec cat {}Length of output: 38087
🏁 Script executed:
#!/bin/bash
# Search for dnd-kit sensor configuration or initialization
rg -n "Sensor|useSortable|DndContext" --type=ts --type=tsx -B2 -A5 | head -100Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for dnd-kit dependencies and setup
fd -t f "package.json" --exec grep -l "@dnd-kit" {} \;Length of output: 27
🏁 Script executed:
#!/bin/bash
# Look for any dnd-kit initialization or provider setup
rg -n "DndContext|DndProvider|useSortable" --type=ts --type=tsx --type=jsx -C3Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check dnd-kit version and dependencies
cat ./apps/desktop/package.json | grep -A5 "@dnd-kit"Length of output: 272
🏁 Script executed:
#!/bin/bash
# Search for DndContext and sensor setup across all file types
rg -n "DndContext|MouseSensor|PointerSensor|Sensor.*activate" -C3Length of output: 2899
🏁 Script executed:
#!/bin/bash
# Find where DndContext is instantiated/configured
rg -n "new DndContext|<DndContext" -C5Length of output: 1713
🌐 Web query:
dnd-kit v6 default sensor PointerSensor pointer events vs mouse events
💡 Result:
Short summary — what changes in dnd-kit v6 when using the default PointerSensor (pointer events) vs the MouseSensor (mouse events):
Key differences
- Event model: PointerSensor listens to Pointer Events (pointerdown/pointermove/pointerup) which unify mouse, touch and pen; MouseSensor listens to Mouse Events (mousedown/mousemove/mouseup) which are mouse-only. [1][3][7]
- Input coverage: PointerSensor handles mouse, touch and pen with one implementation; MouseSensor only handles mouse. Use separate TouchSensor + MouseSensor if you want touch-specific handling. [1][3]
- Activation defaults: PointerSensor applies different default activation constraints by pointerType (e.g., immediate for mouse, delay + tolerance for touch) — these defaults are customizable via configure/activationConstraints. MouseSensor has its own distance/delay constraints (mutually exclusive). [2][3]
- Scrolling / touch-action behavior: Pointer Events rely on CSS touch-action to prevent scrolling. That means preventing page scroll reliably during touch drags requires setting touch-action: none (or using a drag handle) because changing touch-action after a pointerdown is ignored; Touch events (if used directly) let you call preventDefault on touchmove. For that reason some users prefer Mouse+Touch sensors for finer control on mobile. [1][2][6]
- Bugs/edge cases on touch: The PointerSensor has historically had some mobile/touch usability issues (reported lag/behavior on some devices and some activation-distance edge bugs). If you hit touch-specific problems, maintainers/consumers have recommended using separate TouchSensor or custom activators. See issues for examples. [6][3]
When to use which
- Use PointerSensor (default) for a simpler, unified implementation covering mouse/touch/pen and for most desktop+mobile cases. Customize activationConstraints and set touch-action on drag handles if you need to allow scrolling elsewhere. [1][2]
- Use MouseSensor + TouchSensor if you need explicit, different handling for touch vs mouse (e.g., to call preventDefault on touchmove or avoid pointer-event touch-action limitations), or if you need to work around specific PointerSensor touch bugs on some devices. [3][6]
Useful links
- PointerSensor docs (behavior, activators, touch-action notes). [1][2]
- MouseSensor docs (mouse-specific activator/constraints). [3]
- dnd-kit GitHub issues discussing PointerSensor touch problems and edge cases. [6][4]
References
[1] dnd-kit Pointer sensor docs (API).
[2] dnd-kit Pointer Sensor overview / configuration.
[3] dnd-kit Mouse sensor docs.
[4] GitHub issue: dragging with PointerSensor on touch devices.
[6] GitHub issue: PointerSensor activation-distance bug.
[7] W3C Pointer Events spec (pointer vs mouse/touch semantics).
Drag-start suppression is ineffective—PointerSensor will still initiate drags from the button.
You're stopping propagation on both onMouseDown and onClick, but dnd-kit is configured with the default PointerSensor (from @dnd-kit/core v6.3.1 with no explicit sensor override in DndContext). PointerSensor listens to Pointer Events (pointerdown/pointermove/pointerup), not MouseEvent, so calling e.stopPropagation() on MouseEvent won't prevent drag activation from the parent <div> where useSortable attaches its listeners.
Result: Tabs can still be drag-reordered when clicking anywhere on the button, defeating the intended suppression.
Consider one of:
- Wiring a proper drag handle and suppressing the entire button, or
- Calling
e.preventDefault()on pointer events in addition to mouse events (or replacing mouse handlers with pointer handlers), or - Only intercepting specific sub-elements (close button, rename input) rather than the entire tab button.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/components/TabItem/TabItem.tsx
around lines 66-69, the current onMouseDown handler uses
MouseEvent.stopPropagation which does not block dnd-kit's PointerSensor; replace
or augment it with pointer event handling so pointerdown is intercepted (e.g.,
add an onPointerDown handler that calls e.stopPropagation() and
e.preventDefault() for the button/close/rename controls), or alternatively
implement a proper drag handle by adding data attributes/handleRef and
configuring useSortable to only start drag from the handle; ensure the button
elements specifically use the pointer handler (or handle-based approach) so
clicking them will no longer start a drag.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.backup.tsx (4)
646-651: Use error dialogs instead of alert() for consistency.The codebase already has a proper error dialog pattern (lines 1529-1543), but this function uses
alert()which is jarring UX.Apply this diff:
if (result.success) { // Optimistically update the local worktree data const updatedTabs = updateTabNameRecursive( worktree.tabs, groupTabId, newName, ); const updatedWorktree = { ...worktree, tabs: updatedTabs }; onUpdateWorktree(updatedWorktree); } else { - alert(`Failed to rename group: ${result.error}`); + setErrorTitle("Failed to Rename Group"); + setErrorMessage(result.error || "An unknown error occurred."); + setShowErrorDialog(true); } } catch (error) { console.error("Error renaming group:", error); - alert("Failed to rename group"); + setErrorTitle("Failed to Rename Group"); + setErrorMessage("An unexpected error occurred."); + setShowErrorDialog(true); }
1112-1117: Use error dialogs instead of alert() for consistency.Same issue as in handleRenameGroup - use the existing error dialog pattern instead of
alert().Apply this diff:
if (result.success) { // Optimistically update the local worktree data const updatedTabs = updateTabNameRecursive( worktree.tabs, tabId, newName, ); const updatedWorktree = { ...worktree, tabs: updatedTabs }; onUpdateWorktree(updatedWorktree); } else { - alert(`Failed to rename tab: ${result.error}`); + setErrorTitle("Failed to Rename Tab"); + setErrorMessage(result.error || "An unknown error occurred."); + setShowErrorDialog(true); } } catch (error) { console.error("Error renaming tab:", error); - alert("Failed to rename tab"); + setErrorTitle("Failed to Rename Tab"); + setErrorMessage("An unexpected error occurred."); + setShowErrorDialog(true); }
1356-1413: Consider memoizing renderTab for performance.The recursive
renderTabfunction is called for every tab on every render. With many tabs or deep nesting, this could cause performance issues. Consider wrapping tabs inReact.memoor memoizing the renderTab function.However, if this backup file will be removed, optimization is not necessary.
546-556: Ensure IPC handlers validate input on the main process.The code invokes IPC operations with user-controlled data (tab names, IDs, etc.). While input validation in the renderer is not strictly required in Electron's security model, ensure that the main process IPC handlers properly validate and sanitize all inputs to prevent potential exploits.
This is standard Electron architecture, but worth verifying the main process has appropriate guards.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx (3)
33-58: Tree data conversion helper is sound; consider extracting a shared NodeData type.
convertTabsToTreeDatacorrectly mapsTab[]into the{ id, name, tab, children }shape expected by the Arborist tree and supports nested groups if they ever appear. To reduce repetition and keep typings tight, consider defining aTreeNodeDatatype alias used consistently in this file (and inWorktreeItemArborist.tsxif it survives).
83-99: Expanded-group tracking and dynamic height look good; be aware of invariants.Initializing
expandedGroupTabswith all group IDs and using it intreeHeightensures groups start open and the tree height tracks visible rows. Given that the current UI never creates nested groups (only groups of non-group tabs), relying on top-level group IDs inexpandedGroupTabsis fine; if nested groups are introduced later, you’ll want to revisitonToggleand the visible-node counting to handle deeper levels consistently.Also applies to: 132-147, 470-509
259-341: Grouping / ungrouping / move-out IPC flows are consistent; double-checktargetIndexcomments.The group operations (
handleGroupTabs,handleUngroupTab,handleMoveOutOfGroup) correctly usetab-create,tab-move,tab-delete, andtab-update-mosaic-tree, and they filter out group tabs when grouping. One minor nit: comments liketargetIndex: 0 // Add to enddon’t match the usual “index 0 == start” intuition; if the backend does treat0as “top”, you may want to adjust the comment, otherwise consider passing the actual “end” index for clarity.Also applies to: 371-422
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
apps/desktop/package.json(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.backup.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx(6 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx (2)
apps/desktop/src/shared/types.ts (2)
Worktree(58-68)Tab(44-56)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.backup.tsx (1)
WorktreeItem(355-1555)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.backup.tsx (5)
apps/desktop/src/shared/types.ts (3)
Tab(44-56)Worktree(58-68)MosaicNode(35-35)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx (1)
WorktreeItem(72-1254)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx (1)
WorktreeItem(71-449)apps/desktop/src/main/lib/workspace/tab-helpers.ts (1)
removeTabFromMosaicTree(73-109)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/components/GitStatusDialog.tsx (1)
GitStatusDialog(48-406)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx (2)
apps/desktop/src/shared/types.ts (1)
Tab(44-56)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/components/TabItem/TabItem.tsx (1)
TabItem(35-207)
🔇 Additional comments (7)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.backup.tsx (1)
97-115: Good defensive pattern for drag handling.The custom mouseDown handler correctly prevents drag initiation when clicking on interactive elements (buttons, inputs), then delegates to the native @dnd-kit handler using
e.nativeEvent. This pattern ensures buttons like delete/rename work properly without triggering drags.apps/desktop/package.json (1)
51-51: Newreact-arboristruntime dependency looks appropriate; verify compatibility.Adding
react-arboristhere matches the new Arborist-basedWorktreeItemimplementation; just ensure this version is compatible with React 19 and your bundler/packaging setup.apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx (5)
210-257: Shift-click range selection is constrained to siblings, which matches typical UX.
getTabsAtSameLevelplus thehandleTabSelectlogic correctly limits shift-click range selection to non-group tabs at the same level, avoiding cross-group and cross-level ranges. This matches common editor/Explorer behavior and keeps multi-select semantics predictable.
831-875: Rename flows and local optimistic updates are wired correctly.Both
handleTabRenameandhandleRenameGroupinvoketab-update-nameand then useupdateTabNameRecursiveplusonUpdateWorktreeto keep local state in sync, with sensible error handling (console + alert). This preserves consistency between IPC results and the sidebar view.Also applies to: 877-895
897-993: ArboristonMovehandler now correctly distinguishes cross-parent moves vs same-parent reorders.The new
handleMoveimplementation:
- Rejects dragging group tabs.
- Uses
tab-movewhensourceParentTabId !== targetParentTabId, includingtargetIndexfrom the Arborist callback.- Computes a new sibling order for same-parent moves and persists it via
tab-reorder.This addresses the prior issue where intra-parent drag-to-reorder wasn’t persisted. One thing to confirm is that
args.indexfrom Arborist matches the expected index semantics for your IPC handlers (especially at root vs within groups); if those contracts ever change, this will be a sensitive integration point.
995-1083: Node renderer integrates Tree drag handle and TabItem cleanly; just ensure drag UX matches intent.
renderNodecorrectly usesNodeApifor state (level, open, parent) and passesdragHandleto the row wrapper while delegating tab interactions toTabItem. GivenTabItemnow stopsmousedownpropagation to avoid accidental drags, the practical drag handle will be the wrapper area rather than the button; just make sure this matches the intended UX and that there’s enough clickable area to initiate drags comfortably.
1094-1134: Tree configuration aligns with Arborist API; group drag is correctly disabled.The
<Tree>props (data,height={treeHeight},rowHeight,indent,onMove,onSelect,onToggle,openByDefault,initialOpenState) follow the documented API, anddisableDragcorrectly prevents dragging group nodes while allowing tab nodes to be moved and reordered. This setup should give a stable, virtualized sidebar tree matching the previous DnD behavior but with better structure.
| const [expandedGroupTabs, setExpandedGroupTabs] = useState<Set<string>>( | ||
| new Set(), | ||
| ); |
There was a problem hiding this comment.
Inconsistent initialization of expandedGroupTabs.
The expandedGroupTabs state is initialized as an empty Set, meaning all groups start collapsed. However, the main WorktreeItem implementation pre-populates this with all group tab IDs to expand groups by default, improving discoverability.
If this backup file is retained, align it with the main implementation:
- const [expandedGroupTabs, setExpandedGroupTabs] = useState<Set<string>>(
- new Set(),
- );
+ const [expandedGroupTabs, setExpandedGroupTabs] = useState<Set<string>>(() => {
+ const groupIds = new Set<string>();
+ const collectGroupIds = (tabs: Tab[]) => {
+ for (const tab of tabs) {
+ if (tab.type === "group") {
+ groupIds.add(tab.id);
+ if (tab.tabs) collectGroupIds(tab.tabs);
+ }
+ }
+ };
+ collectGroupIds(worktree.tabs);
+ return groupIds;
+ });📝 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.
| const [expandedGroupTabs, setExpandedGroupTabs] = useState<Set<string>>( | |
| new Set(), | |
| ); | |
| const [expandedGroupTabs, setExpandedGroupTabs] = useState<Set<string>>(() => { | |
| const groupIds = new Set<string>(); | |
| const collectGroupIds = (tabs: Tab[]) => { | |
| for (const tab of tabs) { | |
| if (tab.type === "group") { | |
| groupIds.add(tab.id); | |
| if (tab.tabs) collectGroupIds(tab.tabs); | |
| } | |
| } | |
| }; | |
| collectGroupIds(worktree.tabs); | |
| return groupIds; | |
| }); |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.backup.tsx
around lines 367–369, expandedGroupTabs is currently initialized to an empty Set
which collapses all groups by default; change the initialization to pre-populate
the Set with all group tab IDs so groups start expanded (for example, construct
the Set from the worktree's group tab IDs — e.g., new Set(worktree.groupTabIds)
or by mapping worktree.groups to their id/tabId — matching the main WorktreeItem
implementation).
| // Move each selected tab into the group | ||
| for (const tabId of tabIds) { | ||
| const tab = findTabById(tabs, tabId); | ||
| if (!tab || tab.type === "group") continue; // Skip group tabs | ||
|
|
||
| // Use tab-move to move the tab into the group | ||
| await window.ipcRenderer.invoke("tab-move", { | ||
| workspaceId, | ||
| worktreeId: worktree.id, | ||
| tabId, | ||
| targetParentTabId: groupTabId, | ||
| targetIndex: 0, // Add to end | ||
| }); |
There was a problem hiding this comment.
Logic error: targetIndex should be array length to add to end.
The comment says "Add to end" but targetIndex: 0 inserts at the beginning. To append to the end of a group's tabs, use the group's tab count as the index.
Apply this diff:
// Move each selected tab into the group
for (const tabId of tabIds) {
const tab = findTabById(tabs, tabId);
if (!tab || tab.type === "group") continue; // Skip group tabs
+
+ const groupTab = findTabById(tabs, groupTabId);
+ const targetIndex = groupTab?.tabs?.length || 0;
// Use tab-move to move the tab into the group
await window.ipcRenderer.invoke("tab-move", {
workspaceId,
worktreeId: worktree.id,
tabId,
targetParentTabId: groupTabId,
- targetIndex: 0, // Add to end
+ targetIndex, // Add to end
});
}🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.backup.tsx
around lines 560 to 572, the code uses targetIndex: 0 which inserts moved tabs
at the start despite the comment “Add to end”; change the logic to compute the
group's current tab count and pass that as targetIndex so tabs are appended.
Specifically, before invoking "tab-move" resolve the group tab (e.g. const
groupTab = findTabById(tabs, groupTabId)) and set targetIndex to the number of
child tabs (groupTab?.children?.length ?? 0) when calling
window.ipcRenderer.invoke.
| for (const childTab of groupTab.tabs) { | ||
| await window.ipcRenderer.invoke("tab-move", { | ||
| workspaceId, | ||
| worktreeId: worktree.id, | ||
| tabId: childTab.id, | ||
| sourceParentTabId: groupTabId, // Move from the group | ||
| targetParentTabId: undefined, // Move to worktree level | ||
| targetIndex: 0, // Add to end of worktree tabs | ||
| }); |
There was a problem hiding this comment.
Same logic error: targetIndex should be array length.
The comment says "Add to end of worktree tabs" but targetIndex: 0 inserts at the beginning.
Apply this diff:
// Move each child tab back to the worktree level
for (const childTab of groupTab.tabs) {
await window.ipcRenderer.invoke("tab-move", {
workspaceId,
worktreeId: worktree.id,
tabId: childTab.id,
sourceParentTabId: groupTabId, // Move from the group
targetParentTabId: undefined, // Move to worktree level
- targetIndex: 0, // Add to end of worktree tabs
+ targetIndex: tabs.length, // Add to end of worktree tabs
});
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.backup.tsx
around lines 602 to 610 the code moves child tabs out of a group but sets
targetIndex: 0 which inserts them at the start despite the comment "Add to end
of worktree tabs"; change targetIndex to the current worktree tabs length (e.g.,
targetIndex: worktree.tabs.length or the variable/property that holds the
destination tabs array length) so each moved tab is appended to the end,
removing the hardcoded 0.
| try { | ||
| const result = await window.ipcRenderer.invoke("tab-move", { | ||
| workspaceId, | ||
| worktreeId: worktree.id, | ||
| tabId: draggedTabId, | ||
| sourceParentTabId: sourceParentTabId || undefined, | ||
| targetParentTabId: targetGroupId, | ||
| targetIndex: 0, // Add to end of group | ||
| }); |
There was a problem hiding this comment.
Same logic error: targetIndex should be array length.
The comment says "Add to end of group" but targetIndex: 0 inserts at the beginning.
Apply this diff:
+ const targetGroup = findTabById(tabs, targetGroupId);
+ const targetIndex = targetGroup?.tabs?.length || 0;
+
// Move tab into the group
try {
const result = await window.ipcRenderer.invoke("tab-move", {
workspaceId,
worktreeId: worktree.id,
tabId: draggedTabId,
sourceParentTabId: sourceParentTabId || undefined,
targetParentTabId: targetGroupId,
- targetIndex: 0, // Add to end of group
+ targetIndex, // Add to end of group
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.backup.tsx
around lines 1209 to 1217, the call to window.ipcRenderer.invoke uses
targetIndex: 0 which inserts the tab at the start despite the comment "Add to
end of group"; change targetIndex to the length of the destination group's tab
array so the tab is appended. To fix, compute the target group's current tabs
length (e.g., find the group by targetGroupId and use its tabs.length,
defaulting to 0 if not found) and pass that value as targetIndex in the invoke
payload.
| // Move tab into the group | ||
| try { | ||
| const result = await window.ipcRenderer.invoke("tab-move", { | ||
| workspaceId, | ||
| worktreeId: worktree.id, | ||
| tabId: draggedTabId, | ||
| sourceParentTabId: sourceParentTabId || undefined, | ||
| targetParentTabId: targetGroupId, | ||
| targetIndex: 0, // Add to end of group | ||
| }); |
There was a problem hiding this comment.
Same logic error: targetIndex should be array length.
The comment says "Add to end of group" but targetIndex: 0 inserts at the beginning.
Apply this diff:
+ const targetGroup = findTabById(tabs, targetGroupId);
+ const targetIndex = targetGroup?.tabs?.length || 0;
+
// Move tab into the group
try {
const result = await window.ipcRenderer.invoke("tab-move", {
workspaceId,
worktreeId: worktree.id,
tabId: draggedTabId,
sourceParentTabId: sourceParentTabId || undefined,
targetParentTabId: targetGroupId,
- targetIndex: 0, // Add to end of group
+ targetIndex, // Add to end of group
});Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.backup.tsx
around lines 1243 to 1252, the code sets targetIndex: 0 but the intent is to
"Add to end of group"; change targetIndex to the current length of the target
group's tab array so the tab is appended. Compute the index from the target
group's tabs (e.g., use targetGroupTabs.length or targetGroup?.tabs.length ?? 0)
and pass that value as targetIndex in the invoke payload.
| // Convert Tab[] to react-arborist format | ||
| function convertTabsToTreeData(tabs: Tab[]): Array<{ id: string; name: string; tab: Tab; children?: Array<{ id: string; name: string; tab: Tab }> }> { | ||
| return tabs.map((tab) => { | ||
| const node: { id: string; name: string; tab: Tab; children?: Array<{ id: string; name: string; tab: Tab }> } = { | ||
| id: tab.id, | ||
| name: tab.name, | ||
| tab, | ||
| }; | ||
| if (tab.type === "group" && tab.tabs) { | ||
| node.children = convertTabsToTreeData(tab.tabs); | ||
| } | ||
| return node; | ||
| }); | ||
| } | ||
|
|
||
| // Convert react-arborist data back to Tab[] | ||
| function convertTreeDataToTabs(nodes: NodeApi[]): Tab[] { | ||
| return nodes.map((node) => { | ||
| const tab = node.data.tab as Tab; | ||
| if (tab.type === "group" && node.children && node.children.length > 0) { | ||
| return { | ||
| ...tab, | ||
| tabs: convertTreeDataToTabs(node.children), | ||
| }; | ||
| } | ||
| return tab; | ||
| }); | ||
| } | ||
|
|
||
| export function WorktreeItem({ | ||
| worktree, | ||
| workspaceId, | ||
| activeWorktreeId, | ||
| onTabSelect, | ||
| onReload, | ||
| onUpdateWorktree, | ||
| selectedTabId, | ||
| hasPortForwarding = false, | ||
| onCloneWorktree: _onCloneWorktree, | ||
| }: WorktreeItemProps) { | ||
| const [expandedGroupTabs, setExpandedGroupTabs] = useState<Set<string>>( | ||
| new Set(), | ||
| ); | ||
| const [selectedTabIds, setSelectedTabIds] = useState<Set<string>>(new Set()); | ||
| const [lastClickedTabId, setLastClickedTabId] = useState<string | null>(null); | ||
|
|
||
| // Dialog states | ||
| const [showRemoveDialog, setShowRemoveDialog] = useState(false); | ||
| const [showMergeDialog, setShowMergeDialog] = useState(false); | ||
| const [showErrorDialog, setShowErrorDialog] = useState(false); | ||
| const [showGitStatusDialog, setShowGitStatusDialog] = useState(false); | ||
| const [errorMessage, setErrorMessage] = useState(""); | ||
| const [errorTitle, setErrorTitle] = useState(""); | ||
| const [mergeWarning, setMergeWarning] = useState(""); | ||
| const [removeWarning, setRemoveWarning] = useState(""); | ||
|
|
||
| const isActive = activeWorktreeId === worktree.id; | ||
| const tabs = Array.isArray(worktree.tabs) ? worktree.tabs : []; | ||
| const treeData = convertTabsToTreeData(tabs); | ||
|
|
||
| // Auto-expand group tabs that contain the selected tab | ||
| useEffect(() => { | ||
| if (!selectedTabId) return; | ||
| const findParentGroup = (tabs: Tab[], tabId: string): Tab | null => { | ||
| for (const tab of tabs) { | ||
| if (tab.type === "group" && tab.tabs) { | ||
| if (tab.tabs.some((t) => t.id === tabId)) return tab; | ||
| const found = findParentGroup(tab.tabs, tabId); | ||
| if (found) return found; | ||
| } | ||
| } | ||
| return null; | ||
| }; | ||
| const parentGroup = findParentGroup(tabs, selectedTabId); | ||
| if (parentGroup) { | ||
| setExpandedGroupTabs((prev) => new Set(prev).add(parentGroup.id)); | ||
| } | ||
| }, [selectedTabId, tabs]); | ||
|
|
||
| // Handle tab selection | ||
| const handleTabSelect = ( | ||
| worktreeId: string, | ||
| tabId: string, | ||
| shiftKey: boolean, | ||
| ) => { | ||
| if (shiftKey && lastClickedTabId) { | ||
| // Shift-click: select range | ||
| const allTabs = tabs.flatMap((t) => | ||
| t.type === "group" && t.tabs ? t.tabs : [t], | ||
| ); | ||
| const lastIndex = allTabs.findIndex((t) => t.id === lastClickedTabId); | ||
| const currentIndex = allTabs.findIndex((t) => t.id === tabId); | ||
| if (lastIndex !== -1 && currentIndex !== -1) { | ||
| const start = Math.min(lastIndex, currentIndex); | ||
| const end = Math.max(lastIndex, currentIndex); | ||
| const rangeTabIds = allTabs.slice(start, end + 1).map((t) => t.id); | ||
| setSelectedTabIds(new Set(rangeTabIds)); | ||
| } | ||
| } else { | ||
| setSelectedTabIds(new Set([tabId])); | ||
| setLastClickedTabId(tabId); | ||
| } | ||
| onTabSelect(worktreeId, tabId); | ||
| }; | ||
|
|
||
| // Handle drag and drop (move) | ||
| const handleMove = async (args: { | ||
| dragIds: string[]; | ||
| dragNodes: NodeApi<{ id: string; name: string; tab: Tab; children?: Array<{ id: string; name: string; tab: Tab }> }>[]; | ||
| parentId: string | null; | ||
| parentNode: NodeApi<{ id: string; name: string; tab: Tab; children?: Array<{ id: string; name: string; tab: Tab }> }> | null; | ||
| index: number; | ||
| }) => { | ||
| if (args.dragNodes.length === 0) return; | ||
|
|
||
| const draggedNode = args.dragNodes[0]; | ||
| const draggedTab = draggedNode.data.tab as Tab; | ||
|
|
||
| if (!draggedTab || draggedTab.type === "group") return; | ||
|
|
||
| const draggedTabId = draggedTab.id; | ||
| const sourceParent = draggedNode.parent; | ||
| const sourceParentTabId = sourceParent?.data.tab?.type === "group" ? sourceParent.id : null; | ||
| const targetParentTabId = args.parentNode?.data.tab?.type === "group" ? args.parentNode.id : null; | ||
|
|
||
| // Don't move if already in the same position | ||
| if (sourceParentTabId === targetParentTabId) { | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| const result = await window.ipcRenderer.invoke("tab-move", { | ||
| workspaceId, | ||
| worktreeId: worktree.id, | ||
| tabId: draggedTabId, | ||
| sourceParentTabId: sourceParentTabId || undefined, | ||
| targetParentTabId: targetParentTabId || undefined, | ||
| targetIndex: args.index, | ||
| }); | ||
|
|
||
| if (result.success) { | ||
| onReload(); | ||
| onTabSelect(worktree.id, draggedTabId); | ||
| } else { | ||
| console.error("Failed to move tab:", result.error); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error moving tab:", error); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify role of this alternate Arborist WorktreeItem and align behavior or remove it.
This file defines a WorktreeItem with its own Arborist wiring, but:
handleMoveonly handles cross-parent moves and returns early whensourceParentTabId === targetParentTabId, so same-parent drag-to-reorder does nothing if this component is used.- There is a fair amount of unused scaffolding (dialogs,
Button,GitStatusDialog,convertTreeDataToTabs, dialog state) that never renders. - Behavior and helpers here diverge from the richer
WorktreeItem.tsximplementation in the same directory.
If the app actually uses WorktreeItem.tsx as the source of truth, consider deleting or clearly marking this file as a backup to avoid accidental imports. If this component is intended to be used, it should probably share the same drag/reorder and dialog behavior as WorktreeItem.tsx and trim the unused code.
Also applies to: 327-448
I'll verify the review comment by examining the codebase structure and comparing implementations.
🏁 Script executed:
#!/bin/bash
# Find all WorktreeItem files
echo "=== Finding WorktreeItem files ==="
fd -t f "WorktreeItem" apps/desktop/src/renderer
echo -e "\n=== Checking for imports of WorktreeItemArborist ==="
rg -n "WorktreeItemArborist" apps/desktop/src/renderer -C 3
echo -e "\n=== Checking for WorktreeItem exports/imports in main file ==="
rg -n "export.*WorktreeItem" apps/desktop/src/renderer --type=tsx -C 1Length of output: 564
🏁 Script executed:
#!/bin/bash
# Check exports from WorktreeItemArborist
echo "=== Exports from WorktreeItemArborist.tsx ==="
rg -n "^export" apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx
# Check imports at top of file
echo -e "\n=== Imports in WorktreeItemArborist.tsx ==="
head -40 apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx | grep -E "^import|^from"
# Check if WorktreeItemArborist is imported anywhere globally
echo -e "\n=== Global search for WorktreeItemArborist imports ==="
rg -n "import.*WorktreeItemArborist" apps/desktop/src
# Check main WorktreeItem.tsx handleMove logic
echo -e "\n=== handleMove in main WorktreeItem.tsx ==="
rg -n "handleMove" apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx -A 30 | head -50
# Check for unused dialog state/imports in Arborist file
echo -e "\n=== Checking unused dialog state usage in WorktreeItemArborist.tsx ==="
grep -n "showRemoveDialog\|showMergeDialog\|showErrorDialog\|showGitStatusDialog\|GitStatusDialog\|<Dialog\|<Button" apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsxLength of output: 2720
🏁 Script executed:
#!/bin/bash
# Get full handleMove from main WorktreeItem.tsx
echo "=== Full handleMove from main WorktreeItem.tsx ==="
rg -n "const handleMove = async" apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx -A 100 | head -120
# Get full handleMove from WorktreeItemArborist.tsx for direct comparison
echo -e "\n=== Full handleMove from WorktreeItemArborist.tsx ==="
sed -n '115,165p' apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsxLength of output: 5063
Delete WorktreeItemArborist.tsx—it is unused dead code with incomplete functionality.
This file is never imported anywhere in the codebase and should be removed:
- Dead code: No imports of
WorktreeItemArboristfound globally; the exportedWorktreeItemis not used. - Incomplete drag behavior:
handleMovereturns early whensourceParentTabId === targetParentTabId, meaning same-parent reordering is not implemented. The mainWorktreeItem.tsxcorrectly handles both cross-parent moves (viatab-move) and same-parent reordering (viatab-reorder). - Unused scaffolding: Dialog state (
showRemoveDialog,showMergeDialog,showErrorDialog,showGitStatusDialog), imports (Button,GitStatusDialog), and conversion function (convertTreeDataToTabs) are declared but never rendered or called.
Remove this file to avoid maintenance burden and accidental imports.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx
(lines 42-191), this file contains unused/partial implementation (unused
exported WorktreeItem, incomplete handleMove that skips same-parent reorders,
and unused dialog state and helper functions); delete this file from the repo,
remove any stale imports or references if they exist, and run the build/tests to
ensure nothing else depends on it (if there is a need to preserve functionality
instead, port the missing same-parent reorder logic from WorktreeItem.tsx and
remove unused scaffolding).
1 participant
Summary by CodeRabbit
New Features
Refactor