Conversation
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughThis PR removes duplicate tab functionality from UI components and refactors the tabs store from a monolithic design to a modular architecture with specialized helper functions for different operations. Test infrastructure is established with Bun configuration and Electron environment mocking. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI Component<br/>(e.g., TabItem)
participant Hook as Custom Hook<br/>(e.g., useAddTab)
participant Store as Zustand Store
participant Helper as Helper Module<br/>(tab-crud, active-tab, etc.)
UI->>Hook: Call useAddTab()
Hook->>Store: Access state.addTab action
Store->>Helper: Delegate to handleAddTab(state, ...)
Helper->>Helper: Compute partial state<br/>(new tab, history, active)
Helper-->>Store: Return Partial<TabsState>
Store->>Store: Merge into store state
Store-->>Hook: Updated state
Hook-->>UI: New tabs/activeTabIds
UI->>UI: Re-render with new state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
Comment |
![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: 4
🧹 Nitpick comments (4)
apps/desktop/src/renderer/stores/tabs/helpers/validation.ts (1)
1-27: Layout validation logic is soundThe helper correctly:
- Restricts work to group tabs,
- Derives valid child IDs from
parentId,- Cleans the layout via
cleanLayout,- And only allocates a new tab object when the layout actually changes.
This keeps group layouts consistent with existing children and preserves immutability semantics.
If you want to shave a few cycles, you could early‑return for groups that have
!tab.layoutor no children before callingcleanLayout, but that’s purely an optimization and not required.apps/desktop/src/renderer/stores/tabs/helpers/split-operations.ts (1)
1-72: Split handlers align with existing split helpersThe vertical and horizontal handlers correctly:
- Resolve the tab to split from
sourceTabIdor the active top‑level tab for the workspace,- No‑op when the target is missing or already a group,
- Delegate to
splitPaneInGroupwhen splitting inside an existing group with apath,- Fall back to
convertTabToGroupwhen splitting a top‑level tab.This matches the behavior implied by
split.tsand keeps state updates localized viaPartial<TabsState>.You could factor the shared “find tab to split + in‑group vs top‑level” logic into a small internal helper that takes only the
direction("row" | "column") to remove duplication between vertical and horizontal functions, but the current duplication is manageable.apps/desktop/src/renderer/stores/tabs/helpers/split.ts (1)
10-54: Align split helpers with partial-state + active-history conventionsTwo things worth tightening up here:
No-op behavior in
splitPaneInGroup
Returning the fullstateon early exit is inconsistent with the rest of the helpers, which generally return{}(or a narrow partial) to indicate “no change”. This also unnecessarily spreads methods and other fields when merged byset.Consider instead:
-export const splitPaneInGroup = ( +export const splitPaneInGroup = ( state: TabsState, tabToSplit: Tab, workspaceId: string, path: MosaicBranch[], direction: "row" | "column", -) => { +) : Partial<TabsState> => { const group = state.tabs.find( (t) => t.id === tabToSplit.parentId && t.type === TabType.Group, ); - if (!group || group.type !== TabType.Group || !group.layout) return state; + if (!group || group.type !== TabType.Group || !group.layout) return {}; @@ - return { - tabs: [...updatedTabs, newTabWithParent], - }; + return { + tabs: [...updatedTabs, newTabWithParent], + };
Keep tab history in sync in
convertTabToGroup
You’re changing the active tab from the original tab to the new group but bypassinghandleSetActiveTab, sotabHistoryStacksnever records this transition. That can make “last active tab” behavior inconsistent compared to other active-tab changes.Reuse the existing helper to keep semantics uniform:
-import type { Tab, TabsState } from "../types"; -import { TabType } from "../types"; +import type { Tab, TabsState } from "../types"; +import { TabType } from "../types"; +import { handleSetActiveTab } from "./active-tab"; @@ -export const convertTabToGroup = ( +export const convertTabToGroup = ( state: TabsState, tabToSplit: Tab, workspaceId: string, direction: "row" | "column", -) => { +): Partial<TabsState> => { @@ - const newTabs = [ + const newTabs = [ ...nonWorkspaceTabs, ...otherWorkspaceTabs, updatedSourceTab, newChildTab, ]; - - return { - tabs: newTabs, - activeTabIds: { - ...state.activeTabIds, - [workspaceId]: updatedGroupTab.id, - }, - }; + + const activeUpdates = handleSetActiveTab( + state, + workspaceId, + updatedGroupTab.id, + ); + + return { + tabs: newTabs, + ...activeUpdates, + };This keeps both helpers in line with the rest of the tab-state API and reduces surprising behavior around history.
Also applies to: 59-135
apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts (1)
12-193: Minor consistency and reuse opportunities in group helpersThe group operations look logically sound, but there are two small consistency nits:
- Avoid returning full state in
handleUngroupTab(non‑empty children path)
In the path whereremainingChildren.length !== 0, the function ends with:return { ...state, tabs: validatedTabs, };Since you’re not touching
activeTabIdsortabHistoryStacksthere, returning the fullstateis unnecessary and inconsistent with the rest of the helpers that return narrow partials. You can simplify and reduce coupling to the store shape as:- return { - ...state, - tabs: validatedTabs, - }; + return { + tabs: validatedTabs, + };
Factor out duplicated reordering logic
BothhandleUngroupTab(whentargetIndexis provided) andhandleUngroupTabsreimplement the same “split into workspaceTabs/otherTabs, remove, splice at targetIndex, recombine” pattern thathandleReorderTabByIdalready uses.To keep behavior consistent and avoid future drift, consider extracting a small internal helper (or reusing
handleReorderTabByIdwhere appropriate) for “move top-level tab within workspace” and calling it from these group helpers rather than duplicating the array surgery.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/TabContextMenu.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(0 hunks)apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/helpers/group-management.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/helpers/split-operations.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/helpers/split.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/helpers/validation.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/hooks.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/index.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(2 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(2 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/TabContextMenu.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
🧰 Additional context used
🧬 Code graph analysis (11)
apps/desktop/src/renderer/stores/tabs/helpers/validation.ts (4)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(25-25)apps/desktop/src/shared/types.ts (1)
TabType(23-30)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
getChildTabIds(6-8)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
cleanLayout(48-75)
apps/desktop/src/renderer/stores/tabs/helpers/split-operations.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/split.ts (2)
splitPaneInGroup(10-54)convertTabToGroup(59-136)
apps/desktop/src/renderer/stores/tabs/helpers/group-management.ts (1)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
Tab(25-25)TabsState(27-31)
apps/desktop/src/renderer/stores/tabs/helpers/split.ts (3)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
TabsState(27-31)Tab(25-25)apps/desktop/src/shared/types.ts (2)
TabType(23-30)MosaicNode(35-35)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(10-36)
apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts (1)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
TabsState(27-31)Tab(25-25)
apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts (6)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
TabsState(27-31)Tab(25-25)apps/desktop/src/shared/types.ts (2)
MosaicNode(35-35)TabType(23-30)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
getChildTabIds(6-8)apps/desktop/src/renderer/stores/tabs/helpers/group-management.ts (1)
handleEmptyGroupRemoval(7-57)apps/desktop/src/renderer/stores/tabs/helpers/validation.ts (1)
validateGroupLayouts(9-27)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
removeTabFromLayout(15-43)
apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.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/utils.ts (1)
createNewTab(10-36)
apps/desktop/src/renderer/stores/tabs/store.ts (7)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
TabsStore(33-64)apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (4)
handleAddTab(8-31)handleRemoveTab(37-94)handleRenameTab(99-109)handleMarkTabAsUsed(114-123)apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts (4)
handleSetActiveTab(6-32)getTabsByWorkspace(37-42)getActiveTab(47-54)getLastActiveTabId(59-65)apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts (2)
handleReorderTabs(6-23)handleReorderTabById(28-51)apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts (5)
handleUpdateTabGroupLayout(12-24)handleAddChildTabToGroup(29-50)handleRemoveChildTabFromGroup(55-90)handleUngroupTab(95-194)handleUngroupTabs(199-280)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
handleDragTabToTab(127-326)apps/desktop/src/renderer/stores/tabs/helpers/split-operations.ts (2)
handleSplitTabVertical(9-38)handleSplitTabHorizontal(43-72)
apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts (1)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
TabsState(27-31)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
apps/desktop/src/shared/types.ts (2)
TabType(23-30)MosaicNode(35-35)
apps/desktop/src/renderer/stores/tabs/hooks.ts (1)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useTabsStore(34-138)
🔇 Additional comments (9)
apps/desktop/src/renderer/stores/tabs/index.ts (1)
1-4: Barrel export for hooks looks goodRe‑exporting
./hooksfrom the tabs index keeps the public surface consistent and gives consumers a single import path for all tab store APIs. No issues from a typing or tree‑shaking perspective given the current structure.apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts (2)
8-31: Add-tab behavior and history updates look correct
handleAddTabcleanly:
- Creates a new tab via
createNewTab,- Pushes the current active tab (if any) to the front of the history stack, de‑duplicating it,
- Sets the new tab as active for the given workspace,
- Updates only the relevant slices of
TabsState.This matches typical tab UX and keeps state updates immutable.
99-123: Rename and mark‑used helpers are straightforward and safeBoth
handleRenameTabandhandleMarkTabAsUsed:
- Use
mapto immutably update thetabsarray,- Only touch the targeted tab by id,
- Don’t mutate
activeTabIdsor history, which is appropriate for these operations.No issues here.
apps/desktop/src/renderer/stores/tabs/helpers/active-tab.ts (1)
6-65: Active-tab helpers look consistent and correctHistory maintenance and workspace-scoped accessors are clean and consistent with the rest of the tab CRUD helpers; no issues spotted here.
apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts (1)
28-51:handleReorderTabByIdimplementation looks solidThe ID-based reorder correctly limits itself to top‑level tabs, separates workspace vs other tabs, and cleanly re-inserts the moved tab at
targetIndexwith safe fallbacks when the tab is missing.apps/desktop/src/renderer/stores/tabs/store.ts (1)
3-129: Store refactor cleanly delegates to helpersThe wiring to the new helper modules (
tab-crud,active-tab,group-operations,tab-ordering,split-operations,drag-logic) looks consistent: arguments match, partial states are passed directly intoset, and theremoveTabdelegation toremoveChildTabFromGroupis handled safely. Overall the store surface is much clearer and easier to maintain.apps/desktop/src/renderer/stores/tabs/helpers/group-operations.ts (1)
196-280:handleUngroupTabsbehavior and state updates look correct
handleUngroupTabscleanly removes the group, repositions all ungrouped children at the original group index within the workspace, and updatesactiveTabIdsandtabHistoryStackswhen the group was active. Layout and child derivation viagetChildTabIdsare used appropriately. No functional issues spotted here.apps/desktop/src/renderer/stores/tabs/types.ts (2)
1-31: LGTM! Clean state interface definition.The import addition and TabsState interface are well-structured. The state shape is clear with proper typing for tabs array, active tab mapping, and history stacks.
33-64: Well-structured store interface.The TabsStore interface provides a comprehensive API for tab management with clear method signatures. The separation of state (TabsState) and actions (TabsStore) follows good design patterns.
Comment on lines
+15
to
+56
| const remainingTabs = tabs.filter((tab) => !idsToRemove.includes(tab.id)); | ||
| const currentActiveId = activeTabIds[workspaceId]; | ||
| const historyStack = tabHistoryStacks[workspaceId] || []; | ||
|
|
||
| const newActiveTabIds = { ...activeTabIds }; | ||
| const newHistoryStack = historyStack.filter( | ||
| (id) => !idsToRemove.includes(id), | ||
| ); | ||
|
|
||
| // Update active tab if needed | ||
| if (idsToRemove.includes(currentActiveId || "")) { | ||
| const workspaceTabs = remainingTabs.filter( | ||
| (tab) => tab.workspaceId === workspaceId, | ||
| ); | ||
|
|
||
| if (workspaceTabs.length > 0) { | ||
| // Try to use fallback (e.g., the ungrouped tab), then history, then first available | ||
| if ( | ||
| fallbackActiveTabId && | ||
| remainingTabs.some((t) => t.id === fallbackActiveTabId) | ||
| ) { | ||
| newActiveTabIds[workspaceId] = fallbackActiveTabId; | ||
| } else { | ||
| const nextTabFromHistory = newHistoryStack.find((tabId) => | ||
| workspaceTabs.some((tab) => tab.id === tabId), | ||
| ); | ||
| newActiveTabIds[workspaceId] = | ||
| nextTabFromHistory || workspaceTabs[0].id; | ||
| } | ||
| } else { | ||
| newActiveTabIds[workspaceId] = null; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| tabs: remainingTabs, | ||
| activeTabIds: newActiveTabIds, | ||
| tabHistoryStacks: { | ||
| ...tabHistoryStacks, | ||
| [workspaceId]: newHistoryStack, | ||
| }, | ||
| }; |
Contributor
There was a problem hiding this comment.
Active tab selection may pick child tabs instead of top‑level tabs
workspaceTabs currently includes all tabs in the workspace (including children), and it is used to choose the next active tab (lines 26–43). Since activeTabIds[workspaceId] is treated elsewhere as a top‑level (non‑child) tab id, this can lead to:
activeTabIds[workspaceId]being set to a child tab id, and- Follow‑up helpers that look up the active tab with
!t.parentIdfailing to find it (e.g., split operations), causing subtle “no active tab” behavior.
Example: closing a top‑level tab in a workspace that also has group children can promote a child tab to active due to the closedIndex / workspaceTabs[closedIndex] logic.
I recommend constraining candidates for the next active tab to top‑level workspace tabs and validating the fallback id against that same set:
@@
- if (idsToRemove.includes(currentActiveId || "")) {
- const workspaceTabs = remainingTabs.filter(
- (tab) => tab.workspaceId === workspaceId,
- );
+ if (idsToRemove.includes(currentActiveId || "")) {
+ // Only consider top-level tabs (no parentId) in this workspace
+ const workspaceTabs = remainingTabs.filter(
+ (tab) => tab.workspaceId === workspaceId && !tab.parentId,
+ );
@@
- // Try to use fallback (e.g., the ungrouped tab), then history, then first available
- if (
- fallbackActiveTabId &&
- remainingTabs.some((t) => t.id === fallbackActiveTabId)
- ) {
+ // Try to use fallback (e.g., the ungrouped tab), then history, then first available
+ if (
+ fallbackActiveTabId &&
+ workspaceTabs.some((t) => t.id === fallbackActiveTabId)
+ ) {
newActiveTabIds[workspaceId] = fallbackActiveTabId;
} else {
- const nextTabFromHistory = newHistoryStack.find((tabId) =>
- workspaceTabs.some((tab) => tab.id === tabId),
- );
+ const nextTabFromHistory = newHistoryStack.find((tabId) =>
+ workspaceTabs.some((tab) => tab.id === tabId),
+ );
newActiveTabIds[workspaceId] =
nextTabFromHistory || workspaceTabs[0].id;
}This keeps the activeTabIds invariant (top‑level only) explicit and aligned with how other helpers consume it.
📝 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
| const remainingTabs = tabs.filter((tab) => !idsToRemove.includes(tab.id)); | |
| const currentActiveId = activeTabIds[workspaceId]; | |
| const historyStack = tabHistoryStacks[workspaceId] || []; | |
| const newActiveTabIds = { ...activeTabIds }; | |
| const newHistoryStack = historyStack.filter( | |
| (id) => !idsToRemove.includes(id), | |
| ); | |
| // Update active tab if needed | |
| if (idsToRemove.includes(currentActiveId || "")) { | |
| const workspaceTabs = remainingTabs.filter( | |
| (tab) => tab.workspaceId === workspaceId, | |
| ); | |
| if (workspaceTabs.length > 0) { | |
| // Try to use fallback (e.g., the ungrouped tab), then history, then first available | |
| if ( | |
| fallbackActiveTabId && | |
| remainingTabs.some((t) => t.id === fallbackActiveTabId) | |
| ) { | |
| newActiveTabIds[workspaceId] = fallbackActiveTabId; | |
| } else { | |
| const nextTabFromHistory = newHistoryStack.find((tabId) => | |
| workspaceTabs.some((tab) => tab.id === tabId), | |
| ); | |
| newActiveTabIds[workspaceId] = | |
| nextTabFromHistory || workspaceTabs[0].id; | |
| } | |
| } else { | |
| newActiveTabIds[workspaceId] = null; | |
| } | |
| } | |
| return { | |
| tabs: remainingTabs, | |
| activeTabIds: newActiveTabIds, | |
| tabHistoryStacks: { | |
| ...tabHistoryStacks, | |
| [workspaceId]: newHistoryStack, | |
| }, | |
| }; | |
| const remainingTabs = tabs.filter((tab) => !idsToRemove.includes(tab.id)); | |
| const currentActiveId = activeTabIds[workspaceId]; | |
| const historyStack = tabHistoryStacks[workspaceId] || []; | |
| const newActiveTabIds = { ...activeTabIds }; | |
| const newHistoryStack = historyStack.filter( | |
| (id) => !idsToRemove.includes(id), | |
| ); | |
| // Update active tab if needed | |
| if (idsToRemove.includes(currentActiveId || "")) { | |
| // Only consider top-level tabs (no parentId) in this workspace | |
| const workspaceTabs = remainingTabs.filter( | |
| (tab) => tab.workspaceId === workspaceId && !tab.parentId, | |
| ); | |
| if (workspaceTabs.length > 0) { | |
| // Try to use fallback (e.g., the ungrouped tab), then history, then first available | |
| if ( | |
| fallbackActiveTabId && | |
| workspaceTabs.some((t) => t.id === fallbackActiveTabId) | |
| ) { | |
| newActiveTabIds[workspaceId] = fallbackActiveTabId; | |
| } else { | |
| const nextTabFromHistory = newHistoryStack.find((tabId) => | |
| workspaceTabs.some((tab) => tab.id === tabId), | |
| ); | |
| newActiveTabIds[workspaceId] = | |
| nextTabFromHistory || workspaceTabs[0].id; | |
| } | |
| } else { | |
| newActiveTabIds[workspaceId] = null; | |
| } | |
| } | |
| return { | |
| tabs: remainingTabs, | |
| activeTabIds: newActiveTabIds, | |
| tabHistoryStacks: { | |
| ...tabHistoryStacks, | |
| [workspaceId]: newHistoryStack, | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/helpers/group-management.ts around
lines 15–56, workspaceTabs is built from all tabs (including child/grouped tabs)
and is used to pick the next active tab, which can incorrectly promote a child
tab to active; restrict workspaceTabs to only top‑level tabs (e.g., filter where
!tab.parentId), validate fallbackActiveTabId against that same top‑level set
(only accept it if it exists among those top‑level workspace tabs), and when
searching newHistoryStack for a candidate ensure you only match ids that belong
to the top‑level workspaceTabs; update the logic that sets
newActiveTabIds[workspaceId] accordingly so activeTabIds always remains a
top‑level tab id (or null).
Comment on lines
+37
to
+94
| export const handleRemoveTab = ( | ||
| state: TabsState, | ||
| id: string, | ||
| ): Partial<TabsState> | null => { | ||
| const tabToRemove = state.tabs.find((tab) => tab.id === id); | ||
| if (!tabToRemove) return null; | ||
|
|
||
| // Don't allow closing group tabs directly | ||
| if (tabToRemove.type === TabType.Group) { | ||
| console.error("Cannot close group tabs directly. Ungroup the tabs first."); | ||
| return null; | ||
| } | ||
|
|
||
| // If this tab is a child of a group, return null to delegate to removeChildTabFromGroup | ||
| if (tabToRemove.parentId) { | ||
| return null; | ||
| } | ||
|
|
||
| // Otherwise, handle as a top-level tab | ||
| const workspaceId = tabToRemove.workspaceId; | ||
| const workspaceTabs = state.tabs.filter( | ||
| (tab) => tab.workspaceId === workspaceId && tab.id !== id, | ||
| ); | ||
| const tabs = state.tabs.filter((tab) => tab.id !== id); | ||
|
|
||
| const historyStack = state.tabHistoryStacks[workspaceId] || []; | ||
| const newHistoryStack = historyStack.filter((tabId) => tabId !== id); | ||
|
|
||
| const newActiveTabIds = { ...state.activeTabIds }; | ||
| if (state.activeTabIds[workspaceId] === id) { | ||
| if (workspaceTabs.length > 0) { | ||
| const nextTabFromHistory = newHistoryStack.find((tabId) => | ||
| workspaceTabs.some((tab) => tab.id === tabId), | ||
| ); | ||
| if (nextTabFromHistory) { | ||
| newActiveTabIds[workspaceId] = nextTabFromHistory; | ||
| } else { | ||
| const closedIndex = state.tabs | ||
| .filter((tab) => tab.workspaceId === workspaceId) | ||
| .findIndex((tab) => tab.id === id); | ||
| const nextTab = | ||
| workspaceTabs[closedIndex] || workspaceTabs[closedIndex - 1]; | ||
| newActiveTabIds[workspaceId] = nextTab.id; | ||
| } | ||
| } else { | ||
| newActiveTabIds[workspaceId] = null; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| tabs, | ||
| activeTabIds: newActiveTabIds, | ||
| tabHistoryStacks: { | ||
| ...state.tabHistoryStacks, | ||
| [workspaceId]: newHistoryStack, | ||
| }, | ||
| }; | ||
| }; |
Contributor
There was a problem hiding this comment.
Active tab selection on remove may target child tabs instead of top‑level
handleRemoveTab’s top‑level branch has a similar issue to handleEmptyGroupRemoval: workspaceTabs is defined as all tabs in the workspace except the one being closed (line 57–59), without excluding child tabs (parentId).
Since activeTabIds[workspaceId] is used elsewhere as a top‑level tab id (e.g., split handlers search by active id and !t.parentId), this logic can set the active id to a child tab, for example:
- Workspace has top‑level tabs and group children.
- Closing a top‑level tab can promote a child tab (e.g.,
workspaceTabs[closedIndex]) to active, breaking the invariant and confusing subsequent logic that expects a top‑level.
To keep activeTabIds consistent, restrict candidates to top‑level workspace tabs and base the “next tab” selection on that subset:
@@
- const workspaceTabs = state.tabs.filter(
- (tab) => tab.workspaceId === workspaceId && tab.id !== id,
- );
- const tabs = state.tabs.filter((tab) => tab.id !== id);
+ // Only consider top-level tabs (no parentId) when choosing the next active tab
+ const workspaceTopLevelTabs = state.tabs.filter(
+ (tab) =>
+ tab.workspaceId === workspaceId && !tab.parentId && tab.id !== id,
+ );
+ const tabs = state.tabs.filter((tab) => tab.id !== id);
@@
- if (state.activeTabIds[workspaceId] === id) {
- if (workspaceTabs.length > 0) {
- const nextTabFromHistory = newHistoryStack.find((tabId) =>
- workspaceTabs.some((tab) => tab.id === tabId),
- );
- if (nextTabFromHistory) {
- newActiveTabIds[workspaceId] = nextTabFromHistory;
- } else {
- const closedIndex = state.tabs
- .filter((tab) => tab.workspaceId === workspaceId)
- .findIndex((tab) => tab.id === id);
- const nextTab =
- workspaceTabs[closedIndex] || workspaceTabs[closedIndex - 1];
- newActiveTabIds[workspaceId] = nextTab.id;
- }
+ if (state.activeTabIds[workspaceId] === id) {
+ if (workspaceTopLevelTabs.length > 0) {
+ const nextTabFromHistory = newHistoryStack.find((tabId) =>
+ workspaceTopLevelTabs.some((tab) => tab.id === tabId),
+ );
+ if (nextTabFromHistory) {
+ newActiveTabIds[workspaceId] = nextTabFromHistory;
+ } else {
+ const closedIndex = state.tabs
+ .filter(
+ (tab) => tab.workspaceId === workspaceId && !tab.parentId,
+ )
+ .findIndex((tab) => tab.id === id);
+ const nextTab =
+ workspaceTopLevelTabs[closedIndex] ||
+ workspaceTopLevelTabs[closedIndex - 1];
+ newActiveTabIds[workspaceId] = nextTab.id;
+ }
} else {
newActiveTabIds[workspaceId] = null;
}
}This keeps active‑tab semantics coherent with the rest of the helpers and avoids hard‑to‑trace issues when closing tabs in workspaces that also have grouped children.
📝 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
| export const handleRemoveTab = ( | |
| state: TabsState, | |
| id: string, | |
| ): Partial<TabsState> | null => { | |
| const tabToRemove = state.tabs.find((tab) => tab.id === id); | |
| if (!tabToRemove) return null; | |
| // Don't allow closing group tabs directly | |
| if (tabToRemove.type === TabType.Group) { | |
| console.error("Cannot close group tabs directly. Ungroup the tabs first."); | |
| return null; | |
| } | |
| // If this tab is a child of a group, return null to delegate to removeChildTabFromGroup | |
| if (tabToRemove.parentId) { | |
| return null; | |
| } | |
| // Otherwise, handle as a top-level tab | |
| const workspaceId = tabToRemove.workspaceId; | |
| const workspaceTabs = state.tabs.filter( | |
| (tab) => tab.workspaceId === workspaceId && tab.id !== id, | |
| ); | |
| const tabs = state.tabs.filter((tab) => tab.id !== id); | |
| const historyStack = state.tabHistoryStacks[workspaceId] || []; | |
| const newHistoryStack = historyStack.filter((tabId) => tabId !== id); | |
| const newActiveTabIds = { ...state.activeTabIds }; | |
| if (state.activeTabIds[workspaceId] === id) { | |
| if (workspaceTabs.length > 0) { | |
| const nextTabFromHistory = newHistoryStack.find((tabId) => | |
| workspaceTabs.some((tab) => tab.id === tabId), | |
| ); | |
| if (nextTabFromHistory) { | |
| newActiveTabIds[workspaceId] = nextTabFromHistory; | |
| } else { | |
| const closedIndex = state.tabs | |
| .filter((tab) => tab.workspaceId === workspaceId) | |
| .findIndex((tab) => tab.id === id); | |
| const nextTab = | |
| workspaceTabs[closedIndex] || workspaceTabs[closedIndex - 1]; | |
| newActiveTabIds[workspaceId] = nextTab.id; | |
| } | |
| } else { | |
| newActiveTabIds[workspaceId] = null; | |
| } | |
| } | |
| return { | |
| tabs, | |
| activeTabIds: newActiveTabIds, | |
| tabHistoryStacks: { | |
| ...state.tabHistoryStacks, | |
| [workspaceId]: newHistoryStack, | |
| }, | |
| }; | |
| }; | |
| export const handleRemoveTab = ( | |
| state: TabsState, | |
| id: string, | |
| ): Partial<TabsState> | null => { | |
| const tabToRemove = state.tabs.find((tab) => tab.id === id); | |
| if (!tabToRemove) return null; | |
| // Don't allow closing group tabs directly | |
| if (tabToRemove.type === TabType.Group) { | |
| console.error("Cannot close group tabs directly. Ungroup the tabs first."); | |
| return null; | |
| } | |
| // If this tab is a child of a group, return null to delegate to removeChildTabFromGroup | |
| if (tabToRemove.parentId) { | |
| return null; | |
| } | |
| // Otherwise, handle as a top-level tab | |
| const workspaceId = tabToRemove.workspaceId; | |
| // Only consider top-level tabs (no parentId) when choosing the next active tab | |
| const workspaceTopLevelTabs = state.tabs.filter( | |
| (tab) => | |
| tab.workspaceId === workspaceId && !tab.parentId && tab.id !== id, | |
| ); | |
| const tabs = state.tabs.filter((tab) => tab.id !== id); | |
| const historyStack = state.tabHistoryStacks[workspaceId] || []; | |
| const newHistoryStack = historyStack.filter((tabId) => tabId !== id); | |
| const newActiveTabIds = { ...state.activeTabIds }; | |
| if (state.activeTabIds[workspaceId] === id) { | |
| if (workspaceTopLevelTabs.length > 0) { | |
| const nextTabFromHistory = newHistoryStack.find((tabId) => | |
| workspaceTopLevelTabs.some((tab) => tab.id === tabId), | |
| ); | |
| if (nextTabFromHistory) { | |
| newActiveTabIds[workspaceId] = nextTabFromHistory; | |
| } else { | |
| const closedIndex = state.tabs | |
| .filter( | |
| (tab) => tab.workspaceId === workspaceId && !tab.parentId, | |
| ) | |
| .findIndex((tab) => tab.id === id); | |
| const nextTab = | |
| workspaceTopLevelTabs[closedIndex] || | |
| workspaceTopLevelTabs[closedIndex - 1]; | |
| newActiveTabIds[workspaceId] = nextTab.id; | |
| } | |
| } else { | |
| newActiveTabIds[workspaceId] = null; | |
| } | |
| } | |
| return { | |
| tabs, | |
| activeTabIds: newActiveTabIds, | |
| tabHistoryStacks: { | |
| ...state.tabHistoryStacks, | |
| [workspaceId]: newHistoryStack, | |
| }, | |
| }; | |
| }; |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/helpers/tab-crud.ts around lines 37 to
94, the top-level removal branch computes workspaceTabs and next-tab candidates
including child (group) tabs which can cause activeTabIds[workspaceId] to be set
to a child id; change workspaceTabs (and any lookup for nextTabFromHistory) to
only include top-level tabs (filter tab.workspaceId === workspaceId &&
!tab.parentId && tab.id !== id), compute closedIndex against the original
top-level list (not the full state.tabs), pick nextTab from that top-level list
(or previous one) and fall back to null if none, and update tabHistoryStacks
filtering to use top-level ids as well so activeTabIds remains a top-level tab
id consistently.
Comment on lines
+6
to
+23
| export const handleReorderTabs = ( | ||
| state: TabsState, | ||
| workspaceId: string, | ||
| startIndex: number, | ||
| endIndex: number, | ||
| ): Partial<TabsState> => { | ||
| const workspaceTabs = state.tabs.filter( | ||
| (tab) => tab.workspaceId === workspaceId, | ||
| ); | ||
| const otherTabs = state.tabs.filter( | ||
| (tab) => tab.workspaceId !== workspaceId, | ||
| ); | ||
|
|
||
| const [removed] = workspaceTabs.splice(startIndex, 1); | ||
| workspaceTabs.splice(endIndex, 0, removed); | ||
|
|
||
| return { tabs: [...otherTabs, ...workspaceTabs] }; | ||
| }; |
Contributor
There was a problem hiding this comment.
Guard against invalid startIndex and consider top‑level consistency
In handleReorderTabs, if startIndex is out of range, [removed] becomes undefined and you end up inserting undefined into workspaceTabs, corrupting state.tabs.
You can harden this with a simple bounds check:
export const handleReorderTabs = (
state: TabsState,
workspaceId: string,
startIndex: number,
endIndex: number,
): Partial<TabsState> => {
const workspaceTabs = state.tabs.filter(
(tab) => tab.workspaceId === workspaceId,
);
const otherTabs = state.tabs.filter(
(tab) => tab.workspaceId !== workspaceId,
);
- const [removed] = workspaceTabs.splice(startIndex, 1);
- workspaceTabs.splice(endIndex, 0, removed);
+ if (startIndex < 0 || startIndex >= workspaceTabs.length) {
+ // No-op on invalid index
+ return {};
+ }
+
+ const [removed] = workspaceTabs.splice(startIndex, 1);
+ workspaceTabs.splice(endIndex, 0, removed);
return { tabs: [...otherTabs, ...workspaceTabs] };
};Also, note that this helper includes all workspace tabs (including children), whereas handleReorderTabById explicitly restricts to top‑level tabs (!t.parentId). If the indices you receive are for the top‑level tab strip, you may want to mirror that predicate here to avoid ever reordering child tabs.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/helpers/tab-ordering.ts around lines 6
to 23, handleReorderTabs currently splices without validating startIndex (which
can produce undefined and corrupt state) and operates on all workspace tabs
including children; add explicit bounds checks (ensure startIndex and endIndex
are within [0, workspaceTopLevelTabs.length - 1] and return original state or
no-op if out of range) before splicing, and mirror handleReorderTabById by
filtering workspace tabs to only top-level tabs (tab.workspaceId === workspaceId
&& !tab.parentId) so reordering only affects the top-level strip and avoids
inserting undefined or moving child tabs.
Comment on lines
+1
to
+23
| import { useTabsStore } from "./store"; | ||
|
|
||
| export const useTabs = () => useTabsStore((state) => state.tabs); | ||
| export const useActiveTabIds = () => | ||
| useTabsStore((state) => state.activeTabIds); | ||
|
|
||
| export const useAddTab = () => useTabsStore((state) => state.addTab); | ||
| export const useRemoveTab = () => useTabsStore((state) => state.removeTab); | ||
| export const useRenameTab = () => useTabsStore((state) => state.renameTab); | ||
| export const useSetActiveTab = () => | ||
| useTabsStore((state) => state.setActiveTab); | ||
| export const useReorderTabs = () => useTabsStore((state) => state.reorderTabs); | ||
| export const useReorderTabById = () => | ||
| useTabsStore((state) => state.reorderTabById); | ||
| export const useMarkTabAsUsed = () => | ||
| useTabsStore((state) => state.markTabAsUsed); | ||
| export const useUngroupTab = () => useTabsStore((state) => state.ungroupTab); | ||
| export const useUngroupTabs = () => useTabsStore((state) => state.ungroupTabs); | ||
| export const useSplitTabVertical = () => | ||
| useTabsStore((state) => state.splitTabVertical); | ||
| export const useSplitTabHorizontal = () => | ||
| useTabsStore((state) => state.splitTabHorizontal); | ||
|
|
Contributor
There was a problem hiding this comment.
Incomplete hooks module - missing 8 store members.
The hooks module is missing exports for several TabsStore members, creating an inconsistent API surface:
Missing state hook:
useTabHistoryStacksforstate.tabHistoryStacks
Missing action hooks:
useUpdateTabGroupLayoutforstate.updateTabGroupLayoutuseAddChildTabToGroupforstate.addChildTabToGroupuseRemoveChildTabFromGroupforstate.removeChildTabFromGroupuseDragTabToTabforstate.dragTabToTab
Missing getter hooks:
useGetTabsByWorkspaceforstate.getTabsByWorkspaceuseGetActiveTabforstate.getActiveTabuseGetLastActiveTabIdforstate.getLastActiveTabId
If these members are intentionally internal and not meant for component consumption, consider documenting this pattern or restructuring the TabsStore interface to separate public vs. internal APIs.
Apply this diff to add the missing hooks:
export const useTabs = () => useTabsStore((state) => state.tabs);
export const useActiveTabIds = () =>
useTabsStore((state) => state.activeTabIds);
+export const useTabHistoryStacks = () =>
+ useTabsStore((state) => state.tabHistoryStacks);
export const useAddTab = () => useTabsStore((state) => state.addTab);
export const useRemoveTab = () => useTabsStore((state) => state.removeTab);
export const useRenameTab = () => useTabsStore((state) => state.renameTab);
export const useSetActiveTab = () =>
useTabsStore((state) => state.setActiveTab);
export const useReorderTabs = () => useTabsStore((state) => state.reorderTabs);
export const useReorderTabById = () =>
useTabsStore((state) => state.reorderTabById);
export const useMarkTabAsUsed = () =>
useTabsStore((state) => state.markTabAsUsed);
+export const useUpdateTabGroupLayout = () =>
+ useTabsStore((state) => state.updateTabGroupLayout);
+export const useAddChildTabToGroup = () =>
+ useTabsStore((state) => state.addChildTabToGroup);
+export const useRemoveChildTabFromGroup = () =>
+ useTabsStore((state) => state.removeChildTabFromGroup);
+export const useDragTabToTab = () =>
+ useTabsStore((state) => state.dragTabToTab);
export const useUngroupTab = () => useTabsStore((state) => state.ungroupTab);
export const useUngroupTabs = () => useTabsStore((state) => state.ungroupTabs);
export const useSplitTabVertical = () =>
useTabsStore((state) => state.splitTabVertical);
export const useSplitTabHorizontal = () =>
useTabsStore((state) => state.splitTabHorizontal);
+export const useGetTabsByWorkspace = () =>
+ useTabsStore((state) => state.getTabsByWorkspace);
+export const useGetActiveTab = () =>
+ useTabsStore((state) => state.getActiveTab);
+export const useGetLastActiveTabId = () =>
+ useTabsStore((state) => state.getLastActiveTabId);🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/hooks.ts around lines 1 to 23, the
hooks module is missing exports for eight TabsStore members; add exports that
map to the corresponding state members: export const useTabHistoryStacks = () =>
useTabsStore((state) => state.tabHistoryStacks); export const
useUpdateTabGroupLayout = () => useTabsStore((state) =>
state.updateTabGroupLayout); export const useAddChildTabToGroup = () =>
useTabsStore((state) => state.addChildTabToGroup); export const
useRemoveChildTabFromGroup = () => useTabsStore((state) =>
state.removeChildTabFromGroup); export const useDragTabToTab = () =>
useTabsStore((state) => state.dragTabToTab); export const useGetTabsByWorkspace
= () => useTabsStore((state) => state.getTabsByWorkspace); export const
useGetActiveTab = () => useTabsStore((state) => state.getActiveTab); and export
const useGetLastActiveTabId = () => useTabsStore((state) =>
state.getLastActiveTabId); ensure naming matches the store exactly and keep them
exported alongside the existing hooks.
Merged
This was referenced Nov 21, 2025
Merged
Merged
Merged
Merged
Merged
Merged
This was referenced Nov 24, 2025
Merged
Closed
Merged
5 tasks
5 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 by CodeRabbit
Release Notes
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.