Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAllows drops onto any existing tab (removes identity checks); expands drag-to-tab logic to create/reparent tabs and groups for many drop scenarios; extracts next-tab selection into findNextTab; refactors handleEmptyGroupRemoval signature to accept TabsState; adjusts tests and small UI/label behavior (no-drag wrappers, New Terminal button, default title). Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DropHook as useTabContentDrop
participant DragLogic as handleDragTabToTab
participant Store as TabsState
User->>DropHook: Drop dragged tab onto target tab
Note over DropHook: canDrop only verifies target exists
DropHook->>DragLogic: call dragTabToTab(draggedId, targetId)
alt target is same tab
DragLogic->>Store: create new tab
DragLogic->>Store: create/update group (row split 50/50)
DragLogic->>Store: set group active
else target is sibling / same group
DragLogic->>Store: create new tab in group
DragLogic->>Store: update group layout (split/nesting)
else target is different group
DragLogic->>Store: create new tab
DragLogic->>Store: reparent tab, update group layouts
end
DragLogic-->>DropHook: updated TabsState
DropHook-->>User: UI re-renders with new tabs/layout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
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 selected for processing (1)
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: 0
🧹 Nitpick comments (4)
apps/desktop/src/renderer/stores/tabs/store.test.ts (2)
18-31: Repeatedas anycasts in test setup could be localizedUsing
as constfor the fixtures and thenchildTab as any(and similar forchild1,child2, etc.) when setting state is fine for tests, but the pattern is a bit noisy and repeats in several places. Consider either:
- Introducing a small typed helper to build
Tabtest objects, or- Narrowing the fixture type once (e.g.,
const childTab: Tab = { … }) so that the latersetStatecalls don’t needas any.Not urgent, but it would make the tests slightly cleaner and less brittle to type changes.
Also applies to: 59-79, 115-127, 157-169
221-230: Simplify layout checks by narrowing once beforefindThe
newChildlookup currently repeats runtime guards inside thefindpredicate:if (typeof groupTab.layout === "string" || !groupTab.layout) return; const newChild = state.tabs.find( (t) => typeof groupTab.layout !== "string" && groupTab.layout && "second" in groupTab.layout && t.id === groupTab.layout.second && t.type === TabType.Group, );Given the early
return, those extra checks are always true at runtime. For clarity and to keep TypeScript happy without duplication, you could narrow once and capture the layout:if (!groupTab.layout || typeof groupTab.layout === "string") return; const { second } = groupTab.layout; const newChild = state.tabs.find( (t) => t.id === second && t.type === TabType.Single, );Same pattern applies to the horizontal split test. This keeps the tests readable and removes redundant conditions.
Also applies to: 316-321
apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.test.ts (1)
1-245: Good coverage of next-tab behavior; consider adding history-stack casesThis suite nicely exercises the main flows in
findNextTab(linear neighbors, grouped children, cross-workspace isolation, and the ultimate fallback). One thing you might add later is an explicit case wheretabHistoryStacks[workspaceId]is populated, to lock in the “history wins first” behavior before group/index fallbacks.apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
258-289: Consider guarding against cross-workspace drags inhandleDragTabToTabThe new same-group behaviors (child→child and child→group both creating new tabs in the group) are nicely aligned and go through
addToParentGroup, withremoveFromOldParentused only when moving between different parents. One hardening you might want:
- Early in
handleDragTabToTab, after findingdraggedTabandtargetTab, bail out if theirworkspaceIds differ:if (draggedTab.workspaceId !== targetTab.workspaceId) { return state; }Right now the logic assumes UI-level constraints prevent cross-workspace drags; adding this guard would make the store more robust to future UI changes and avoid accidentally wiring a tab into a group in another workspace.
Also applies to: 327-352, 127-143
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useTabContentDrop.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts(5 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.ts(3 hunks)apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts(7 hunks)apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.test.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts(2 hunks)apps/desktop/src/renderer/stores/tabs/store.test.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.test.ts (3)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
TabsState(27-31)apps/desktop/src/shared/types.ts (1)
TabType(23-30)apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts (1)
findNextTab(11-48)
apps/desktop/src/renderer/stores/tabs/store.test.ts (2)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useTabsStore(34-138)apps/desktop/src/shared/types.ts (1)
TabType(23-30)
apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (1)
apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts (1)
findNextTab(11-48)
apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts (3)
apps/desktop/src/shared/types.ts (1)
TabType(23-30)apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(25-25)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
handleDragTabToTab(127-467)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (3)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(10-36)apps/desktop/src/shared/types.ts (1)
TabType(23-30)apps/desktop/src/renderer/stores/tabs/types.ts (2)
TabGroup(20-23)Tab(25-25)
apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts (1)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
TabsState(27-31)
apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts (3)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
TabsState(27-31)apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts (1)
findNextTab(11-48)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
getChildTabIds(6-8)
🔇 Additional comments (17)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useTabContentDrop.ts (1)
15-22: Self-drop is now allowed; relies on store logic for safeguardsThe updated
dropandcanDrophandlers now allow dropping a tab onto itself (any non-nulltabToRender), which matches the new drag behavior. This is fine as long ashandleDragTabToTabfully defines what self-drop does (group creation, etc.), which seems to be the case. No additional validation looks necessary at the DnD layer.apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (1)
4-4: Delegating next-active-tab selection tofindNextTablooks correctUsing
findNextTab(state, id)insidehandleRemoveTabis a good centralization of “what becomes active next.” Passing the originalstate(before removing the tab and before pruning the history stack) is important so the helper can see the closing tab and its history/layout when making the decision. The resultingnextTabIdcan legitimately benull, which aligns with theactiveTabIdstype.Also applies to: 39-63
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
144-255: Self-drop behavior is consistent and preserves layout/parent invariantsThe new Rule 1 handling for
draggedTabId === targetTabIdlooks solid:
- Child tab: creates a new sibling in the same group, updates layout via
addToParentGroup, and keeps the group as active.- Group tab: adds a new child tab to the group and extends its layout, again keeping the group active.
- Standalone single: promotes the tab into a new group plus a new sibling, inserts that group at the original tab’s top-level index, and rewires
parentId/layout appropriately.Using
addToParentGroupin all three branches keeps the mosaic layout coherent, and re-partitioning intoworkspaceTabsUpdatedvsotherTabsUpdatedmaintains the expected ordering of top-level vs child tabs. I don’t see any immediate correctness issues here.apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts (5)
2-2: LGTM: Import additions support the refactored logic.The new imports for
removeTabFromLayoutandfindNextTabare correctly added to support the state-based refactor and next-tab selection logic.Also applies to: 6-7
9-60: Clean state-based refactor with improved next-tab selection.The refactored signature accepting a single
TabsStateobject is cleaner and more maintainable. The integration offindNextTabproperly delegates next-tab selection to a dedicated utility.The logic correctly handles the case where the active tab is being removed by using the fallback, then
findNextTab, then defaulting to the first workspace tab.
62-72: LGTM: Minor formatting improvement.The condensed ternary on line 69 maintains the same logic with improved readability.
96-126: LGTM: Correctly updated to use new state-based API.The call to
handleEmptyGroupRemovalcorrectly passes the current state object and properly handles the case where removing a child leaves an empty group.
128-218: LGTM: Correct state composition for nested call.Line 169 correctly composes a new state object with
updatedTabsbefore callinghandleEmptyGroupRemoval. This ensures the next-tab finder sees the ungrouped tab as a valid candidate, and the fallback correctly activates the ungrouped tab when the parent group is removed.apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts (3)
98-142: LGTM: Comprehensive test for self-drop group creation.The test thoroughly validates that dragging a tab onto itself creates a new group containing both the original tab and a newly created tab, with proper parent relationships and a 50/50 row layout. The assertions correctly verify the group becomes active.
144-192: LGTM: Well-structured test for child self-drop.The test correctly validates that dragging a child tab onto itself creates a new tab within the parent group, updates the group layout to include both tabs, and maintains the group as active. Good coverage of parent-child relationships.
712-917: LGTM: Comprehensive coverage of same-group drag scenarios.These three new tests thoroughly validate edge cases where tabs are dragged within the same group:
- Dragging a tab onto its own parent group creates a new sibling
- Dragging a group onto itself creates a new child within that group
- Dragging a child onto a sibling creates a new tab in the same group
All tests properly verify the nested layout structures, parent relationships, and active state management. The consistent layout patterns across these tests indicate predictable behavior.
apps/desktop/src/renderer/stores/tabs/helpers/next-tab-finder.ts (6)
11-48: LGTM: Well-structured priority-based next-tab selection.The main function implements a clear four-level priority system for selecting the next tab. The early returns for edge cases (tab not found, no remaining tabs) are correctly handled, and the progressive fallback strategy is logical.
53-72: LGTM: Robust history-based selection.The function correctly filters the closing tab from the history stack and validates that the selected tab still exists and belongs to the same workspace. The iteration through the history ensures stale entries don't cause issues.
77-105: LGTM: Position-based sibling selection.The function correctly finds the next sibling within the same group using position-based logic (next first, then previous). This provides predictable behavior for users closing tabs within groups.
110-154: LGTM: Correct position-based top-level tab selection.The function properly handles both grouped and top-level tabs:
- For grouped tabs, it delegates to
findNextRelativeToParentGroupto find tabs near the parent's position- For top-level tabs, it uses index arithmetic that correctly accounts for the removed tab when selecting the next or previous tab
The index calculations are subtle but correct: after filtering out the closing tab,
remainingTabs[currentIndex]gives the next tab (indices shifted down), andremainingTabs[currentIndex - 1]gives the previous tab.
159-183: LGTM: Logical parent-relative positioning.When closing a child tab with no siblings, finding the next tab relative to the parent group's position is intuitive and provides good UX. The function correctly handles edge cases where the parent is at the beginning or end of the workspace tabs.
188-211: LGTM: Sensible ultimate fallback with top-level preference.The fallback strategy correctly prioritizes top-level tabs (more accessible to users) before falling back to any remaining tab in the workspace. This ensures a tab is always selected when possible while maintaining good UX.
1 participant
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Refactoring
UI
✏️ Tip: You can customize this high-level summary in your review settings.