Conversation
WalkthroughReplace the flat tab model with a workspace-centered architecture: remove legacy TabView and single-file tabs store, add a Workspaces store and WorkspaceView, introduce per-workspace tab modules (types/store/drag-logic), DnD hooks and sidebar helpers, mosaic-based group rendering, many UI component renames, and numerous import/order-only edits. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WorkspaceView
participant WorkspacesStore
participant Sidebar
participant ContentView
participant TabsContent
User->>WorkspaceView: mount
WorkspaceView->>WorkspacesStore: read workspaces, activeWorkspaceId
alt activeWorkspace?.isNew
WorkspaceView->>WorkspaceView: render NewWorkspaceView
else
WorkspaceView->>Sidebar: render Sidebar
WorkspaceView->>ContentView: render ContentView
ContentView->>WorkspacesStore: read sidebar mode
alt mode = Changes
ContentView->>ContentView: render ChangesContent
else
ContentView->>TabsContent: render TabsContent
TabsContent->>TabsContent: determine tabToRender
alt no active tab
TabsContent->>TabsContent: render EmptyTabView
else tab.type = Single
TabsContent->>TabsContent: render SingleTabView (drop target)
else
TabsContent->>TabsContent: render GroupTabView (mosaic)
end
end
end
sequenceDiagram
participant Dragger as User (drag)
participant TabItem
participant DnD as DragDropEngine
participant TabsStore
Dragger->>TabItem: start drag
TabItem->>DnD: emit DragItem (tabId)
Dragger->>TabItem: drop onto TabContent (active)
TabContent->>DnD: drop -> canDrop/isOver true
DnD->>TabsStore: dragTabToTab(draggedId, targetId)
TabsStore->>TabsStore: handleDragTabToTab rules (attach/group/reorder)
TabsStore-->>TabItem: state updated
TabItem-->>Dragger: UI updates
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Points to prioritize during review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 (1)
apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/AddTabButton.tsx (1)
13-13: Margin removal looks good; consider omitting the empty className prop.The removal of top margin is appropriate for the layout adjustment. The button component will render correctly with an empty className.
For slightly cleaner code, you can omit the
classNameprop entirely when it's empty:<Button variant="ghost" size="icon" onClick={addTab} aria-label="Add new tab" - className="" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/ModeHeader.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/AddTabButton.tsx(1 hunks)
🔇 Additional comments (1)
apps/desktop/src/renderer/screens/main/components/TabView/Sidebar/ModeCarousel/ModeHeader.tsx (1)
11-11: LGTM! Typography improvement enhances readability.The text size increase from
text-xstotext-smis appropriate for a header component and improves visual hierarchy.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/AddWorkspaceButton.tsx (1)
1-18: Minor cleanup and store subscription optimization for AddWorkspaceButtonBehavior is fine, but you can avoid unnecessary re-renders and simplify props:
- Select just the
addWorkspaceaction so this button doesn’t re-render whenever other workspace state changes.- Drop the empty
className.For example:
-import { useWorkspacesStore } from "renderer/stores/workspaces"; +import { useWorkspacesStore } from "renderer/stores/workspaces"; export function AddWorkspaceButton() { - const { addWorkspace } = useWorkspacesStore(); + const addWorkspace = useWorkspacesStore((state) => state.addWorkspace); return ( <Button variant="ghost" size="icon" onClick={addWorkspace} aria-label="Add new workspace" - className="" > <span className="text-lg">+</span> </Button> ); }apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent.tsx (1)
1-18: ChangesContent placeholder layout is straightforward and fineThe centered placeholder with background/scroll behavior looks good as a stub. Since TabsContent uses the same structure, you could later extract a small shared “placeholder content” component that just takes title/description to avoid duplication, but it’s not urgent.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent.tsx (1)
1-18: TabsContent placeholder mirrors ChangesContent cleanlyThis is a fine placeholder implementation. Given it’s structurally identical to ChangesContent aside from labels, you could later factor both through a tiny shared component that takes
titleandsubtitleprops, if you want to keep things DRY as more modes are added.apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (1)
6-25: Clarify lifecycle ofisNewso workspaces don’t stay permanently in NewWorkspaceView
WorkspaceViewsends any workspace withisNew === trueintoNewWorkspaceViewindefinitely. From the provided code, it’s not obvious wheremarkWorkspaceAsUsedis ever called, so a workspace created viaaddWorkspacemight never transition to the normalSidebar+ContentViewlayout.If that’s not intentional, consider:
- Calling
markWorkspaceAsUsedwhen the user takes an action inNewWorkspaceView, or- Clearing
isNewwhen a workspace is first activated.Otherwise new workspaces may be stuck in the onboarding view.
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
7-61: Width/hover logic is solid; consider guarding againstworkspaces.length === 0The dynamic width calculation, hover-aware separator opacity, and scroll fade effects all look good and should behave well as workspaces are added/removed/reordered.
One small robustness tweak: in
updateWorkspaceWidth, you divide byworkspaces.length. With the current store implementation you never end up with 0 workspaces, but if that invariant ever changes (or via devtools time-travel),availableWidth / 0would yieldInfinitybefore clamping. An early return like:if (workspaces.length === 0) return;would make this component safer against future changes without affecting current behavior.
Also applies to: 74-121
apps/desktop/src/renderer/stores/workspaces.ts (1)
21-89: Workspace store behavior is coherent; add a defensive check inremoveWorkspaceThe state shape and actions align well with the new workspace UX (default “Home”, ensuring at least one workspace, picking a sensible next active on close, etc.).
One edge case: in
removeWorkspace, if an unknownidwere ever passed,findIndexwould return-1, so:const closedIndex = state.workspaces.findIndex((workspace) => workspace.id === id); const nextWorkspace = workspaces[closedIndex] || workspaces[closedIndex - 1]; return { workspaces, activeWorkspaceId: nextWorkspace.id };would try to access
nextWorkspace.idwherenextWorkspaceisundefined.It can’t happen with the current UI, but adding a guard like:
if (id === state.activeWorkspaceId) { const closedIndex = state.workspaces.findIndex((workspace) => workspace.id === id); if (closedIndex === -1) { return { workspaces, activeWorkspaceId: workspaces[0]?.id ?? null }; } // existing logic... }would make this reducer safer against future callers or devtools manipulation.
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
5-30: Workspace store wiring is consistent; consider narrowing the selector for perfThe mapping to
useWorkspacesStoreactions (setActiveWorkspace,removeWorkspace,reorderWorkspaces) matches the store API and looks correct. One improvement:useWorkspacesStore()without a selector will re-render this component on any workspace state change; since this component only needs the actions, consider:const { setActiveWorkspace, removeWorkspace, reorderWorkspaces } = useWorkspacesStore((state) => ({ setActiveWorkspace: state.setActiveWorkspace, removeWorkspace: state.removeWorkspace, reorderWorkspaces: state.reorderWorkspaces, }));to avoid unnecessary re-renders when the workspace list or active id changes elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
apps/desktop/src/renderer/routes.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/TabView/CenterView.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/TabView/index.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/AddTabButton.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/AddWorkspaceButton.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx(5 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx(4 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ModeCarousel/ModeContent.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ModeCarousel/ModeHeader.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/index.tsx(2 hunks)apps/desktop/src/renderer/stores/index.ts(1 hunks)apps/desktop/src/renderer/stores/tabs.ts(0 hunks)apps/desktop/src/renderer/stores/workspaces.ts(1 hunks)
💤 Files with no reviewable changes (6)
- apps/desktop/src/renderer/screens/main/index.ts
- apps/desktop/src/renderer/screens/main/components/TabView/CenterView.tsx
- apps/desktop/src/renderer/stores/tabs.ts
- apps/desktop/src/renderer/routes.tsx
- apps/desktop/src/renderer/screens/main/components/TabView/index.tsx
- apps/desktop/src/renderer/screens/main/components/TopBar/Tabs/AddTabButton.tsx
🧰 Additional context used
🧬 Code graph analysis (8)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsx (3)
apps/desktop/src/renderer/stores/sidebar-state.ts (1)
useSidebarStore(17-37)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent.tsx (1)
ChangesContent(1-18)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent.tsx (1)
TabsContent(1-18)
apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
WorkspacesTabs(11-123)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/AddWorkspaceButton.tsx (1)
apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (4)
apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView.tsx (1)
NewWorkspaceView(1-50)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx (1)
Sidebar(8-55)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsx (1)
ContentView(5-13)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView.tsx (1)
ChangesView(1-7)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView.tsx (1)
TabsView(1-20)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (3)
apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (1)
WorkspaceItem(19-103)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/AddWorkspaceButton.tsx (1)
AddWorkspaceButton(4-18)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (2)
apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)packages/ui/src/lib/utils.ts (1)
cn(4-6)
apps/desktop/src/renderer/screens/main/index.tsx (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx (1)
WorkspaceView(6-26)
🔇 Additional comments (12)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView.tsx (1)
1-7: Placeholder ChangesView looks fine for nowStateless placeholder is simple and consistent; nothing blocking here. Just remember to replace this with the real changes UI when it’s ready.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView.tsx (1)
1-7: Rename to NewWorkspaceView and updated heading are consistentThe component name and H1 text now align with the workspace terminology; no issues from this change.
apps/desktop/src/renderer/screens/main/index.tsx (1)
5-16: MainScreen now correctly renders WorkspaceViewWiring
WorkspaceViewinto the main layout in place of the old tab view is straightforward and keeps the existing DnD and frame structure intact. Looks good.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ModeCarousel/ModeContent.tsx (1)
2-2: Colocating SidebarMode types is fineUpdating the
SidebarModeimport to./typeskeeps the typing local to this ModeCarousel group and doesn’t affect runtime behavior.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ModeCarousel/ModeHeader.tsx (1)
11-13: Mode header text size change is safeBumping the mode label to
text-smis a simple visual adjustment and doesn’t affect behavior.apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx (1)
3-21: TopBar now correctly hosts WorkspacesTabsReplacing the old
TabswithWorkspacesTabskeeps the existing top-bar structure intact while aligning with the new workspace model. Theno-dragregion and overflow handling around the tabs are preserved correctly.apps/desktop/src/renderer/stores/index.ts (1)
7-8: Barrel export updated correctly to include workspacesRe-exporting from
"./workspaces"alongside"./sidebar-state"keeps store imports centralized and matches the new workspace-centric architecture; no issues here.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx (1)
4-6: Sidebar mode switching between TabsView and ChangesView looks correctUsing
SidebarMode.Changesfor the branch and defaulting toTabsViewfor other modes keeps sidebar content aligned with the store’scurrentMode; I don’t see behavioral issues with this wiring.Also applies to: 45-50
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/index.tsx (1)
1-13: ContentView’s mode-based switch is consistent with the sidebarReturning
ChangesContentonly whencurrentMode === SidebarMode.ChangesandTabsContentotherwise keeps the main content pane in sync with the sidebar mode; implementation is clear and adequate for the two current modes.apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceItem.tsx (3)
31-50: Drag-and-drop configuration for workspace reordering looks soundThe DnD setup uses a dedicated
WORKSPACE_TYPE, passesidandindexin the drag item, and updates order viareorderWorkspaces(item.index, index)while mutatingitem.indexto prevent redundant calls. This is a standard, correct pattern for index-based reordering withreact-dnd.
57-83: Active workspace behavior and main button interactions are correctThe active-state styling, drag handle ref (
drag(drop(node))), andonClick={() => setActiveWorkspace(id)}correctly align the visual selection with store state, while preserving hover callbacks. The opacity change onisDraggingis a clear feedback mechanism and doesn’t interfere with click or drag behavior.
85-100: Close button interaction and accessibility are handled wellStopping propagation before
removeWorkspace(id)ensures closing doesn’t also activate the workspace, and thearia-label="Close workspace"provides a clear description for assistive tech. The hover/active opacity behavior tied togroup-hoveris a reasonable UX pattern for a secondary control.
| export function TabsView() { | ||
| return ( | ||
| <nav className="space-y-2"> | ||
| <div className="text-sm text-sidebar-foreground"> | ||
| <p className="font-medium mb-2">Navigation</p> | ||
| <ul className="space-y-1"> | ||
| <li className="px-3 py-2 rounded-md hover:bg-sidebar-accent cursor-pointer"> | ||
| Dashboard | ||
| </li> | ||
| <li className="px-3 py-2 rounded-md hover:bg-sidebar-accent cursor-pointer"> | ||
| Projects | ||
| </li> | ||
| <li className="px-3 py-2 rounded-md hover:bg-sidebar-accent cursor-pointer"> | ||
| Settings | ||
| </li> | ||
| </ul> | ||
| </div> | ||
| </nav> | ||
| ); | ||
| } |
There was a problem hiding this comment.
List items are styled as interactive but not actually clickable or accessible
<li> elements with cursor-pointer and hover styles but no onClick or semantic role will confuse users and assistive tech. If this is more than a pure visual mock, consider making them real buttons/links:
- <li className="px-3 py-2 rounded-md hover:bg-sidebar-accent cursor-pointer">
- Dashboard
- </li>
+ <li>
+ <button
+ type="button"
+ className="w-full text-left px-3 py-2 rounded-md hover:bg-sidebar-accent cursor-pointer"
+ // onClick={...}
+ >
+ Dashboard
+ </button>
+ </li>Same pattern can apply to the other items.
📝 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.
| export function TabsView() { | |
| return ( | |
| <nav className="space-y-2"> | |
| <div className="text-sm text-sidebar-foreground"> | |
| <p className="font-medium mb-2">Navigation</p> | |
| <ul className="space-y-1"> | |
| <li className="px-3 py-2 rounded-md hover:bg-sidebar-accent cursor-pointer"> | |
| Dashboard | |
| </li> | |
| <li className="px-3 py-2 rounded-md hover:bg-sidebar-accent cursor-pointer"> | |
| Projects | |
| </li> | |
| <li className="px-3 py-2 rounded-md hover:bg-sidebar-accent cursor-pointer"> | |
| Settings | |
| </li> | |
| </ul> | |
| </div> | |
| </nav> | |
| ); | |
| } | |
| export function TabsView() { | |
| return ( | |
| <nav className="space-y-2"> | |
| <div className="text-sm text-sidebar-foreground"> | |
| <p className="font-medium mb-2">Navigation</p> | |
| <ul className="space-y-1"> | |
| <li> | |
| <button | |
| type="button" | |
| className="w-full text-left px-3 py-2 rounded-md hover:bg-sidebar-accent cursor-pointer" | |
| // onClick={...} | |
| > | |
| Dashboard | |
| </button> | |
| </li> | |
| <li> | |
| <button | |
| type="button" | |
| className="w-full text-left px-3 py-2 rounded-md hover:bg-sidebar-accent cursor-pointer" | |
| // onClick={...} | |
| > | |
| Projects | |
| </button> | |
| </li> | |
| <li> | |
| <button | |
| type="button" | |
| className="w-full text-left px-3 py-2 rounded-md hover:bg-sidebar-accent cursor-pointer" | |
| // onClick={...} | |
| > | |
| Settings | |
| </button> | |
| </li> | |
| </ul> | |
| </div> | |
| </nav> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView.tsx
around lines 1 to 20, the <li> items are visually interactive (cursor-pointer,
hover styles) but lack semantic interactivity and keyboard/assistive support;
replace each purely-styled <li> with a focusable semantic control (preferably a
<button> or an <a> if it navigates) or wrap the content in a <button>, move the
interactive classes to that control, add an onClick handler (or href) for the
action, ensure keyboard activation (Enter/Space) is handled if using a
non-native element, include appropriate ARIA attributes/labels if needed, and
keep visible focus styles so the items are accessible to keyboard and assistive
technologies.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/old-desktop/src/renderer/screens/main/components/PlanView/PlanView.tsx (1)
178-224: Critical: Undefined variables passed to KanbanColumn components.Multiple
KanbanColumncomponents reference undefined variablesonTabSelectandonTabCreated(lines 178-179, 189-190, 200-201, 212-213, 223-224). These variables are not defined in the component scope—the correct variables arehandleTabSelectandhandleTabCreated, which are destructured fromuseTabContext()on lines 18-19.This will cause runtime errors when the child components attempt to invoke these undefined callbacks.
Apply this diff to fix all occurrences:
onTaskClick={setViewingTask} statusColor="bg-neutral-500" currentWorkspace={currentWorkspace} selectedWorktreeId={selectedWorktreeId} - onTabSelect={onTabSelect} - onTabCreated={onTabCreated} + onTabSelect={handleTabSelect} + onTabCreated={handleTabCreated} onUpdateTask={handleUpdateTask} />Repeat this fix for all five
KanbanColumninstances (lines 171-225).apps/old-desktop/src/main/lib/desktop-stores.ts (1)
52-62: Error handling silently swallows initialization failures.The catch block only logs errors without re-throwing or providing any failure indication to callers. If storage directory creation or migration fails, the application continues with potentially inconsistent state.
Consider one of these approaches:
private async initializeAsync(): Promise<void> { - try { - // Ensure storage directories exist first - await ensureDesktopStorageDirs(); - - // Then run migration if needed - await this.runMigrationIfNeeded(); - } catch (error) { - console.error("Failed to initialize Desktop stores:", error); - } + // Ensure storage directories exist first + await ensureDesktopStorageDirs(); + + // Then run migration if needed + await this.runMigrationIfNeeded(); }Or, if you need to handle errors gracefully:
private async initializeAsync(): Promise<void> { try { // Ensure storage directories exist first await ensureDesktopStorageDirs(); // Then run migration if needed await this.runMigrationIfNeeded(); } catch (error) { console.error("Failed to initialize Desktop stores:", error); + throw new Error(`Desktop store initialization failed: ${error}`); } }apps/old-desktop/src/renderer/screens/main/hooks/useTasks.ts (1)
171-216: Use a ref to guard modal-open state check after async operationsThe
if (isAddTaskModalOpen)check at line 195 captures state value at handler invocation time. AftersetIsCreatingWorktree(false)(line 188), the modal can be closed by the user, but the stale closure still sees the oldtruevalue. If the user closes the modal during the 50ms delay or before this check runs, the code will still callsetSelectedWorktreeIdandhandleTabSelectdespite the modal being closed—violating the "only if modal is still open" intent.Track modal state in a ref (kept in sync via
useEffect) and check that ref instead of the render-time state value, matching the pattern already used forisHandlingProgressRef.
♻️ Duplicate comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx (1)
26-57:tab.panesusage depends on fixing TabGroup shapeThis component assumes
TabGrouphas apanesmap keyed by the mosaic leaf IDs. Right nowTabGroupintabs/types.tsdoesn’t declarepanes, and its comment says layout uses tab IDs. Once you reconcile the TabGroup model there (see comment inapps/desktop/src/renderer/stores/tabs/types.ts), update this component to match the final shape.apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
40-115: Group creation/join paths rely on the incomplete TabGroup modelBoth the “join existing group” and “create new group” rules construct
TabGroupobjects with apanesmap but don’t setchildTabIds, whileTabGroupintabs/types.tscurrently doesn’t declarepanesand does requirechildTabIds.Once you fix the TabGroup shape in
tabs/types.ts, make sure these branches:
- Build
layoutusing the agreed leaf IDs (pane or tab).- Populate all required fields on
TabGroup(including anypanes/child relationship fields you settle on).Otherwise these branches will keep returning structurally inconsistent group tabs.
🧹 Nitpick comments (14)
apps/old-desktop/src/main/lib/storage/index.ts (1)
1-5: Export order tweak is fine and keeps the storage barrel coherentReordering to export
./typesafter./orchestratorsdoesn’t change the public surface, and it aligns with the orchestrators index ordering. No issues from a correctness or API perspective assuming these modules are side‑effect free.apps/old-desktop/src/main/lib/storage/orchestrators/index.ts (1)
1-4: Barrel export reordering and deduplication look correctMoving
change-orchestratorto the top andworkspace-orchestratorto the end, while keeping environment/process in place, preserves the exported symbols and removes the duplicatechange-orchestratorre-export. This is a safe, cleanliness-only change.apps/old-desktop/src/renderer/screens/main/utils.ts (1)
2-2: Import cleanup is correct; consider promotingformatRelativeTimeto a shared util.The deduped, top-level import looks good and keeps this file tidy. If
formatRelativeTimeis (or becomes) widely used beyondAddTaskModal, consider moving it into a more generic shared/date‑utils module to avoid couplingmain/utilsto a specific layout component directory.apps/old-desktop/src/main/lib/workspace-ipcs.ts (1)
15-15: Standardize worktreeManager import pattern.The code uses both static import (line 15) and dynamic imports (lines 36, 242, 797, 870) for
worktreeManager. This mixed pattern creates confusion about which approach to follow. Consider either:
- Use only the static import (line 15) throughout the file, or
- Remove the static import and use dynamic imports consistently everywhere
The static import is currently used directly at lines 487, 541, 580, 634, 676, and 738, while dynamic imports appear in IPC handlers at lines 36, 242, 797, and 870.
Also applies to: 36-36, 242-242, 797-797, 870-870
apps/old-desktop/src/main/lib/types/cli-types.ts (2)
17-25: Clarify whetherAgentTypeandProcessTypehave runtime valuesYou’re exporting
AgentTypeandProcessTypetwice with different forms:
- As types:
export type { AgentType, ProcessType } from "@superset/cli/types/process";- As values:
export { AgentType, ProcessType } from "@superset/cli/types/process";This is only valid if
@superset/cli/types/processactually exposes runtime symbols namedAgentTypeandProcessType(e.g., enums oras constobjects). If they are type-only (aliases/interfaces), the value export will fail underisolatedModules/ emit.If these are intended to be types only, consider removing the value re-export; if both type + runtime are intended, a short comment here explaining the dual export would help future readers.
If they are type-only, you can simplify to:
-export type { - Agent, - AgentType, - Process, - ProcessOrchestrator, - ProcessType, - Terminal, -} from "@superset/cli/types/process"; -export { AgentType, ProcessType } from "@superset/cli/types/process"; +export type { + Agent, + AgentType, + Process, + ProcessOrchestrator, + ProcessType, + Terminal, +} from "@superset/cli/types/process";
26-32: Same concern forWorkspaceTypetype vs value exportHere you similarly export
WorkspaceTypeas both a type and a value:
export type { ... WorkspaceType } from "@superset/cli/types/workspace";export { WorkspaceType } from "@superset/cli/types/workspace";If
WorkspaceTypeis type-only, the value export will cause issues; if it is an enum/constant, dual export is fine but a bit surprising in a “types” façade.Also, you might not need both lines if consumers only ever use one of type or value forms. Consider either:
- Keeping both but adding a brief comment that
WorkspaceTypeis both a type and runtime enum; or- Dropping the value export if it isn’t actually used as a runtime symbol by the desktop app.
A possible simplification if
WorkspaceTypeis type-only:-export type { - LocalWorkspace, - Workspace, - WorkspaceOrchestrator, - WorkspaceType, -} from "@superset/cli/types/workspace"; -export { WorkspaceType } from "@superset/cli/types/workspace"; +export type { + LocalWorkspace, + Workspace, + WorkspaceOrchestrator, + WorkspaceType, +} from "@superset/cli/types/workspace";apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (1)
13-13: Address the type assertion pattern.The
as unknown as React.Ref<HTMLDivElement>double type assertion is a code smell that typically indicates a type mismatch. This pattern bypasses TypeScript's type safety and could hide underlying type issues.Consider addressing the type incompatibility properly. The
useDrophook from react-dnd returns a connector function that should work with ref callbacks. You might be able to fix this by:
- Using a ref callback pattern:
+ const setDropRef = (el: HTMLDivElement | null) => { + drop(el); + }; + return ( <div - ref={drop as unknown as React.Ref<HTMLDivElement>} + ref={setDropRef}
- Or checking if the react-dnd types need to be updated to properly align with React's ref types.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
93-104: Add a clarifying comment in TabItem explaining the single-level nesting design.The one-level nesting constraint is intentional and well-documented in the shared types file as "cannot contain nested group tabs". However, the TabItem component doesn't explain why
childTabs={[]}is hardcoded in the recursive call. Adding a brief comment linking to this architectural decision would improve code clarity for future maintainers:// Intentional: Group tabs can only have direct children; nested groups are not supported // See apps/desktop/src/shared/types.ts for the architectural constraint <TabItem tab={childTab} isActive={isActive} childTabs={[]} />This prevents future developers from mistakenly trying to pass actual childTabs to recursive children or questioning whether this is an oversight.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts (1)
1-14: Duplicate DragItem/TAB_DND_TYPE definitions should be centralized
DragItemandTAB_DND_TYPEare also defined inTabsContent/types.ts. Consider moving these to a shared DnD types module and importing from there to avoid future divergence.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx (1)
18-23: Avoid noisyconsole.login layout change handler
handleLayoutChangelogs on every mosaic change, which will get spammy in normal use. Once persistence is wired, drop the log or guard it behind a dev flag.apps/desktop/src/renderer/stores/tabs/types.ts (1)
3-7: Consider renaming this TabType to avoid confusion with shared TabTypeThere’s already a
TabTypestring union inapps/desktop/src/shared/types.ts. Introducing anotherTabTypeenum here increases the chance of importing the wrong one. Consider a more specific name (e.g.WorkspaceTabType) or re‑exporting the shared type instead if they are meant to represent the same concept.apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx (2)
153-209: Drag handler blocks moves within the same parent
handleMovereturns early wheneversourceParentTabId === targetParentTabId, which prevents any drag that keeps a tab under the same parent (e.g., reordering siblings) from reaching the"tab-move"IPC call. If you intend to support intra-group reordering, this guard should also take the original vs target index into account, or be removed so the backend can decide whether a no-op move is needed.
257-279: Ungroup operation ignores IPC failures and can leave inconsistent stateIn
handleUngroupTab, each"tab-move"and the final"tab-delete"IPC call are awaited but their results are ignored, unlikehandleMove/handleTabRemove/handleTabRenamewhich checkresult.successand log errors. If any of these calls fails, you may end up with some children detached while the group still exists (or vice versa) without feedback to the user.Consider mirroring the pattern used in
handleMove:
- Wrap the loop and delete in a
try/catch.- Check
result.successfor each IPC call and bail or show an error dialog if something goes wrong, instead of always callingonReload().apps/desktop/src/renderer/stores/tabs/store.ts (1)
41-99: Initial test tabs use a different group shape than dynamically created tabs
createInitialTabsconstructs a group tab withchildTabIdsand alayoutthat refers directly to child tab IDs, but doesn’t define the same structure thatcreateNewTabproduces for group tabs (which includes its own internal layout and pane model, perutils.ts). That means group tabs seeded here may not behave the same way as ones created at runtime, which can lead to subtle bugs in any code that assumes a uniformTab/group shape.Consider either:
- Building the example group via
createNewTaband then adjusting it, or- Manually mirroring the fields that
createNewTabsets for a group tab so both code paths produce the same structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (71)
apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/mosaic-theme.css(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/types.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useDropTabTarget.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ModeCarousel/ModeContent.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ModeCarousel/ModeNavigation.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ModeCarousel/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/index.tsx(2 hunks)apps/desktop/src/renderer/stores/index.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/index.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/utils.ts(1 hunks)apps/old-desktop/src/main/index.ts(1 hunks)apps/old-desktop/src/main/lib/desktop-stores.ts(1 hunks)apps/old-desktop/src/main/lib/migration/migrator.ts(1 hunks)apps/old-desktop/src/main/lib/storage/index.ts(1 hunks)apps/old-desktop/src/main/lib/storage/lowdb-adapter.ts(1 hunks)apps/old-desktop/src/main/lib/storage/orchestrators/index.ts(1 hunks)apps/old-desktop/src/main/lib/types/cli-types.ts(1 hunks)apps/old-desktop/src/main/lib/ui-store/index.ts(1 hunks)apps/old-desktop/src/main/lib/ui-store/store.ts(1 hunks)apps/old-desktop/src/main/lib/ui-store/types.ts(1 hunks)apps/old-desktop/src/main/lib/workspace-composition/composer.ts(1 hunks)apps/old-desktop/src/main/lib/workspace-composition/index.ts(1 hunks)apps/old-desktop/src/main/lib/workspace-composition/types.ts(1 hunks)apps/old-desktop/src/main/lib/workspace-ipcs.ts(1 hunks)apps/old-desktop/src/renderer/contexts/AppProviders.tsx(1 hunks)apps/old-desktop/src/renderer/contexts/TabContext.tsx(1 hunks)apps/old-desktop/src/renderer/contexts/TaskContext.tsx(1 hunks)apps/old-desktop/src/renderer/contexts/WorktreeOperationsContext.tsx(1 hunks)apps/old-desktop/src/renderer/contexts/index.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/MainScreen.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/CreatingView.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/TaskList.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/index.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/types.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/useTaskForm.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/utils.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/components/MainContent/TabContent.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/PlanView/PlanView.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/index.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx(1 hunks)apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItemArborist.tsx(2 hunks)apps/old-desktop/src/renderer/screens/main/hooks/useTasks.ts(1 hunks)apps/old-desktop/src/renderer/screens/main/utils.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/index.ts(1 hunks)apps/old-desktop/src/shared/ipc-channels/ui.ts(1 hunks)apps/website/src/app/components/HeroSection/HeroSection.tsx(1 hunks)apps/website/src/app/components/HeroSection/components/HeroCanvas/components/LitBackground/LitBackground.tsx(1 hunks)apps/website/src/app/page.tsx(1 hunks)packages/ui/src/components/context-menu.tsx(1 hunks)packages/ui/src/components/dialog.tsx(1 hunks)packages/ui/src/components/dropdown-menu.tsx(1 hunks)packages/ui/src/components/resizable.tsx(1 hunks)packages/ui/src/components/select.tsx(1 hunks)packages/ui/src/components/separator.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (21)
- apps/old-desktop/src/main/lib/workspace-composition/types.ts
- packages/ui/src/components/dropdown-menu.tsx
- apps/old-desktop/src/renderer/contexts/WorktreeOperationsContext.tsx
- apps/old-desktop/src/main/lib/migration/migrator.ts
- apps/old-desktop/src/renderer/screens/main/components/MainContent/TabContent.tsx
- packages/ui/src/components/context-menu.tsx
- apps/old-desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx
- apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/index.ts
- apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/index.ts
- apps/old-desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx
- apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/types.ts
- apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/utils.ts
- apps/website/src/app/components/HeroSection/HeroSection.tsx
- apps/old-desktop/src/main/lib/ui-store/store.ts
- apps/old-desktop/src/shared/ipc-channels/index.ts
- apps/old-desktop/src/renderer/contexts/TabContext.tsx
- apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/TaskList.tsx
- apps/old-desktop/src/main/index.ts
- apps/old-desktop/src/shared/ipc-channels/ui.ts
- packages/ui/src/components/dialog.tsx
- apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/CreatingView.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/desktop/src/renderer/stores/index.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ModeCarousel/ModeContent.tsx
- apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent.tsx
- apps/desktop/src/renderer/screens/main/index.tsx
🧰 Additional context used
🧬 Code graph analysis (14)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/types.ts (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts (2)
DragItem(3-6)TAB_DND_TYPE(8-8)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx (3)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
TabGroup(24-31)apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)apps/desktop/src/renderer/lib/dnd.ts (1)
dragDropManager(5-5)
apps/desktop/src/renderer/stores/tabs/utils.ts (2)
apps/desktop/src/shared/types.ts (1)
TabType(23-30)apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(34-34)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (3)
apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)apps/desktop/src/renderer/stores/tabs/store.ts (3)
useTabs(310-310)useActiveTabIds(311-312)useAddTab(315-315)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
TabItem(13-107)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useDropTabTarget.ts (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts (2)
DragItem(3-6)TAB_DND_TYPE(8-8)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/types.ts (2)
DragItem(1-4)TAB_DND_TYPE(6-6)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts (1)
TabItemProps(10-14)apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)apps/desktop/src/renderer/stores/tabs/store.ts (2)
useRemoveTab(316-316)useSetActiveTab(317-318)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts (1)
useDragTab(5-42)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
apps/desktop/src/shared/types.ts (2)
TabType(23-30)MosaicNode(35-35)apps/old-desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx (1)
TabGroup(18-302)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (6)
apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)apps/desktop/src/renderer/stores/tabs/store.ts (2)
useTabs(310-310)useActiveTabIds(311-312)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsx (1)
EmptyTabView(1-18)apps/desktop/src/shared/types.ts (1)
TabType(23-30)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (1)
SingleTabView(8-48)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx (1)
GroupTabView(17-82)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (2)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
SingleTab(19-21)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useDropTabTarget.ts (1)
useDropTabTarget(6-34)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts (2)
apps/desktop/src/renderer/stores/tabs/store.ts (1)
useTabsStore(101-307)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts (2)
DragItem(3-6)TAB_DND_TYPE(8-8)
apps/old-desktop/src/renderer/screens/main/components/PlanView/PlanView.tsx (2)
apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/types.ts (1)
Task(4-14)packages/api/src/index.ts (1)
RouterOutputs(9-9)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (3)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
Tab(34-34)TabGroup(24-31)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(3-40)apps/desktop/src/shared/types.ts (1)
TabType(23-30)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/types.ts (2)
DragItem(1-4)TAB_DND_TYPE(6-6)apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(34-34)
apps/desktop/src/renderer/stores/tabs/store.ts (4)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(34-34)apps/desktop/src/shared/types.ts (2)
TabType(23-30)MosaicNode(35-35)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(3-40)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
handleDragTabToTab(10-118)
🔇 Additional comments (36)
apps/old-desktop/src/main/lib/storage/lowdb-adapter.ts (1)
6-6: Import reorder is fine, no behavioral impactReordering the named imports from
"./config"is purely cosmetic here and does not affect runtime behavior. Looks good to keep as-is.apps/old-desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx (1)
21-21: Shared drag‑and‑drop manager wiring looks goodImporting
dragDropManagerfromrenderer/lib/dndand passing it asdndManagertoTreekeeps the DnD configuration centralized and consistent. As long as this is the same singleton used elsewhere, this is a clean, non‑behavioral change.If you haven’t already, please double‑check that
dragDropManagermatches the expectedreact-arboristdndManagerinterface and is reused across allTreeinstances, per their docs.Also applies to: 1067-1072
packages/ui/src/components/select.tsx (1)
4-4: LGTM! Clean import reordering.The React type import has been moved to follow the Radix UI import, improving consistency. This is a pure organizational change with no functional impact.
apps/old-desktop/src/main/lib/workspace-composition/index.ts (1)
1-2: LGTM!The reordering of export declarations has no functional impact and aligns with the broader workspace-related refactoring in this PR.
packages/ui/src/components/resizable.tsx (1)
2-2: LGTM! Import reordering with no functional impact.This is a cosmetic change that reorders the import statement without affecting component behavior or functionality.
apps/website/src/app/components/HeroSection/components/HeroCanvas/components/LitBackground/LitBackground.tsx (1)
10-10: LGTM! Clean import reorganization.The import consolidation groups related config imports together and maintains a logical order. No functional changes to the component.
Also applies to: 16-17
apps/website/src/app/page.tsx (2)
4-5: LGTM!The imports are correctly added and used in the render tree. The reordering improves organization without affecting functionality.
Also applies to: 10-10
20-21: LGTM!The new sections are correctly integrated into the landing page. The
onOpenWaitlistprop properly wires the features section to the existing waitlist modal state management.packages/ui/src/components/separator.tsx (1)
2-10: Type‑only React import matches usage and keeps runtime output clean
Reactis used only for typing (React.ComponentProps), so switching toimport typeis correct and avoids emitting an unnecessary runtime binding. No behavior change, and this is consistent with modern TS/React best practices.If any legacy tooling or older TS versions are still in use in this repo, please confirm they handle
import typecorrectly; otherwise this change is fully safe.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView.tsx (1)
1-7: LGTM! Clean placeholder implementation.The component is well-structured and appropriate for a feature under development. The centered layout and styling are correct.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ModeCarousel/ModeNavigation.tsx (1)
2-2: LGTM! Import consolidation.The import reordering consolidates
AnimatedBackgroundwith other local imports, improving organization with no functional impact.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ModeCarousel/index.tsx (1)
5-7: LGTM! Improved import grouping.The reordering groups component imports together after hooks, creating a clearer logical structure with no functional changes.
apps/old-desktop/src/main/lib/workspace-ipcs.ts (1)
13-13: LGTM!The
windowManagerimport is correctly added and properly used in the IPC handlers at lines 213 and 224 for window-workspace association management.apps/old-desktop/src/main/lib/types/cli-types.ts (1)
6-11: Change-related type re-exports look consistentRe-exporting
AgentSummary,Change,ChangeOrchestrator, andFileDifffrom@superset/cli/types/changekeeps this file as a clean façade over the CLI type surface and matches the stated intent in the header comment. No issues here.apps/old-desktop/src/renderer/screens/main/components/PlanView/PlanView.tsx (2)
6-6: LGTM: Import reordering is stylistic.The import order change has no functional impact and appears to be part of the broader codebase formatting cleanup mentioned in the PR objectives.
14-14: LGTM: Type alias change is stylistic.Converting from
interfacetotypefor an empty, non-exported props definition has no functional impact.apps/old-desktop/src/main/lib/desktop-stores.ts (2)
1-1: LGTM: Import additions support migration flow.The addition of
LegacyMigratorandDesktopChangeOrchestratorto the imports is consistent with the new migration orchestration introduced in this PR.Also applies to: 5-5, 8-8
64-90: The review comment is incorrect; the code handles migration failure and version initialization properly.The
falseparameter is well-defined asdryRun: boolean = false(migrator.ts:46), meaning normal migration proceeds—not an issue.Version initialization is actually conditional on migration success: version writes in
migrate()(migrator.ts:133–134) execute only on the success path before returning. If migration fails, the catch block (migrator.ts:147–154) returnssuccess: falsewithout writing versions. The subsequent version checks indesktop-stores.ts(lines 84–89) act as a safety net, writing versions only if not already set (conditional onread() === 0). This design correctly handles both successful migrations and failure scenarios.Likely an incorrect or invalid review comment.
apps/old-desktop/src/renderer/screens/main/hooks/useTasks.ts (1)
2-5: Type-only imports align with usageSwitching
TaskStatus,PendingWorktree, andUITaskto type-only imports matches their usage here and avoids introducing runtime dependencies from these modules.If you haven’t already, double-check that your TypeScript version in this repo supports
import type(TS ≥ 3.8) viatsconfig.json/package.jsonto prevent build surprises.apps/old-desktop/src/main/lib/ui-store/types.ts (1)
1-1: LGTM: Import reordering only.This is purely a cosmetic change to import ordering with no functional impact.
apps/old-desktop/src/main/lib/ui-store/index.ts (1)
1-2: LGTM: Export reordering only.This is purely a cosmetic change to export ordering with no functional impact.
apps/old-desktop/src/main/lib/workspace-composition/composer.ts (1)
1-11: LGTM: Import additions and reordering.The added
WorktreeUiMetadataimport is correctly used throughout the file (e.g., line 191 return type). The import reordering has no functional impact.apps/old-desktop/src/renderer/contexts/AppProviders.tsx (1)
3-9: LGTM: Import reordering only.This is purely a cosmetic change to import ordering with no functional impact.
apps/old-desktop/src/renderer/screens/main/components/Layout/AddTaskModal/useTaskForm.ts (1)
1-4: LGTM: Import reordering only.This is purely a cosmetic change to import ordering with no functional impact.
apps/old-desktop/src/renderer/contexts/TaskContext.tsx (1)
5-8: LGTM: Improved type imports.Changing from value imports to type-only imports for
PendingWorktreeandUITaskis a best practice that improves tree-shaking and clarifies that these are only used for type annotations.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/mosaic-theme.css (1)
1-65: LGTM: Well-structured mosaic theme.The CSS is well-organized and follows good practices:
- Uses CSS variables for theming consistency
- Includes smooth transitions for better UX
- Provides proper focus states for accessibility
- Split dividers with hover/dragging states enhance the mosaic layout interaction
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsx (1)
1-18: LGTM: Clean empty state component.This is a straightforward presentational component with proper centering and clear messaging. The implementation is clean and follows good practices.
apps/desktop/src/renderer/stores/tabs/index.ts (1)
1-4: LGTM!Clean barrel export pattern for organizing tab-related modules.
apps/old-desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx (1)
1-11: LGTM!Import reordering with no functional changes to the component.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useDropTabTarget.ts (1)
6-34: LGTM!The drop target logic is well-structured with appropriate guards to prevent dropping a tab onto itself. The hook correctly collects drag state and computes the drop zone status.
apps/old-desktop/src/renderer/screens/main/MainScreen.tsx (1)
1-12: LGTM!Import reordering with no functional changes to the component.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts (1)
5-42: LGTM!The drag-and-drop hook is well-implemented with both drag source and drop target functionality. The logic correctly prevents a tab from being dropped onto itself and properly collects drag state for visual feedback.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (1)
12-62: TabsView workspace/tab wiring looks correctFiltering by
workspaceIdand!parentId, resolving activeTabId, and guardingaddTabonactiveWorkspaceIdare all consistent and straightforward. LGTM.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
12-34: Active tab resolution and view selection look solidThe memoized lookup of the active tab per workspace and the type‑based switch between
SingleTabViewandGroupTabVieware clear and correct.apps/old-desktop/src/renderer/contexts/index.ts (1)
3-9: Context barrel changes are safeExport set looks consistent (providers + hooks), and reordering named exports doesn’t affect consumers. No issues here.
apps/desktop/src/renderer/stores/tabs/store.ts (1)
185-211: Active-tab history handling looks solidThe
setActiveTabimplementation maintains a per-workspace history stack as an ordered, de-duplicated list, pushing the previous active tab to the front when switching. This matches the intended “last active tab” semantics forgetLastActiveTabIdand avoids duplicate entries.
| export interface DragItem { | ||
| type: "TAB"; | ||
| tabId: string; | ||
| } | ||
|
|
||
| export const TAB_DND_TYPE = "TAB"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Consolidate duplicated type definitions.
The DragItem interface and TAB_DND_TYPE constant are duplicated in apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts. This violates the DRY principle and creates a maintenance burden where changes must be synchronized across both locations.
Consider consolidating these shared types into a common location, such as:
// apps/desktop/src/renderer/stores/tabs/types.ts or a shared types file
export interface DragItem {
type: "TAB";
tabId: string;
}
export const TAB_DND_TYPE = "TAB";Then import from the single source in both locations.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/types.ts
lines 1-6, the DragItem interface and TAB_DND_TYPE constant are duplicated in
Sidebar/TabsView/TabItem/types.ts; extract these shared definitions into a
single central module (e.g. apps/desktop/src/renderer/stores/tabs/types.ts or a
shared types file), export DragItem and TAB_DND_TYPE from that new file, then
replace the local definitions in both TabsContent/types.ts and
Sidebar/TabsView/TabItem/types.ts with imports from the new central module.
| const workspaceId = draggedTab.workspaceId; | ||
| const historyStack = state.tabHistoryStacks[workspaceId] || []; | ||
|
|
||
| // Rule 1: Dragging into itself - create new tab |
There was a problem hiding this comment.
Guard against dragging tabs across different workspaces
workspaceId is taken from the dragged tab, but nothing enforces that targetTab.workspaceId === draggedTab.workspaceId. If a cross‑workspace drop ever slips through the UI, this will:
- Remove
targetTabfrom its original workspace. - Update
activeTabIdsandtabHistoryStacksonly for the dragged tab’s workspace.
Add an explicit workspace check (and early‑return no‑op or a safer behavior) to keep workspace state consistent.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/drag-logic.ts around lines 24 to 27,
the code derives workspaceId from the dragged tab but does not verify the
targetTab is in the same workspace; add an explicit check comparing
draggedTab.workspaceId and targetTab.workspaceId and if they differ perform an
early return (no-op) or handle with a safe behavior (e.g., deny the drop) to
avoid removing/updating tabs in the wrong workspace, ensuring activeTabIds and
tabHistoryStacks are only modified for the correct workspace.
| // Tab group - split view using react-mosaic | ||
| export interface TabGroup extends BaseTab { | ||
| type: TabType.Group; | ||
| // MosaicNode describes the layout structure (split direction and children) | ||
| // Now uses tab IDs instead of pane IDs | ||
| layout: MosaicNode<string>; | ||
| // Array of child tab IDs | ||
| childTabIds: string[]; | ||
| } |
There was a problem hiding this comment.
TabGroup type is inconsistent with all current usages
TabGroup only exposes layout and childTabIds, but:
GroupTabViewrelies ontab.panes[...].createNewTabandhandleDragTabToTabconstructTabGroupobjects with apanesmap and never populatechildTabIds.- Comments say layout “now uses tab IDs instead of pane IDs”, but the actual creators use pane IDs as mosaic leaves.
This mismatch will either break type‑checking or produce a misleading data model.
I’d strongly recommend:
- Decide whether mosaic leaves represent pane IDs or tab IDs and update the comment and all creators accordingly.
- Update
TabGroupto match the real model (e.g. add a typedpanes: Record<string, { title: string; /* maybe tabId? */ }>and remove or properly maintainchildTabIds). - Fix all constructors (
createNewTab,handleDragTabToTab) and consumers (GroupTabView, any others) to use the same shape.
Until this is aligned, Group tabs are effectively ill‑typed.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/types.ts around lines 23–31, TabGroup's
shape (layout: MosaicNode<string>; childTabIds: string[]) does not match current
code which creates/uses a panes map and uses mosaic leaves as pane IDs; decide
which identity mosaic leaves represent (paneId or tabId), then update the
TabGroup interface to match that model (e.g., replace/augment childTabIds with
panes: Record<string, { title: string; tabId?: string }>, or keep childTabIds
and remove panes), ensure MosaicNode<string> leaf type is documented and matches
the chosen ID type, and update all creators (createNewTab, handleDragTabToTab)
to populate the new fields consistently and all consumers (GroupTabView, etc.)
to read the new shape; run TypeScript to fix remaining call sites and remove the
stale comment or update it to reflect the chosen representation.
| export const createNewTab = ( | ||
| workspaceId: string, | ||
| type: TabType = TabType.Single, | ||
| ): Tab => { | ||
| const id = `tab-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`; | ||
| const baseTab = { | ||
| id, | ||
| title: type === TabType.Single ? "New Tab" : "New Split View", | ||
| workspaceId, | ||
| isNew: true, | ||
| }; | ||
|
|
||
| if (type === TabType.Single) { | ||
| return { | ||
| ...baseTab, | ||
| type: TabType.Single, | ||
| }; | ||
| } | ||
|
|
||
| // Create a default split view with two panes | ||
| const pane1 = `pane-${Date.now()}-1`; | ||
| const pane2 = `pane-${Date.now()}-2`; | ||
|
|
||
| return { | ||
| ...baseTab, | ||
| type: TabType.Group, | ||
| layout: { | ||
| direction: "row", | ||
| first: pane1, | ||
| second: pane2, | ||
| splitPercentage: 50, | ||
| }, | ||
| panes: { | ||
| [pane1]: { title: "Pane 1" }, | ||
| [pane2]: { title: "Pane 2" }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Group tab shape returned here doesn’t match TabGroup type
The Group branch builds { layout, panes } but doesn’t populate childTabIds, while TabGroup currently requires childTabIds and doesn’t declare panes. That will either fail TS type‑checking or make the Tab model internally inconsistent, especially together with GroupTabView and handleDragTabToTab which also expect tab.panes.
Align TabGroup and all creators/consumers on a single shape (e.g. add a typed panes map to TabGroup, decide whether mosaic leaves are pane IDs or tab IDs, and either remove or correctly populate childTabIds).
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/utils.ts around lines 3 to 39, the
Group branch returns an object with layout and panes but omits childTabIds while
the TabGroup type currently requires childTabIds and doesn't declare panes; fix
by aligning types and creator: update the TabGroup type to include a typed panes
map (Record<PaneId, Pane>) and make childTabIds optional OR required as an array
of the pane IDs, then modify createNewTab’s Group return to populate both panes
(with titles/metadata) and childTabIds = [pane1, pane2]; ensure consumers
(GroupTabView, handleDragTabToTab) use pane IDs consistently and adjust any
mosaic/leaf expectations to treat leaves as pane IDs not tab IDs.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
24-26: Missing workspace validation can corrupt cross-workspace state.The code derives
workspaceIdfromdraggedTabbut never verifies thattargetTab.workspaceIdmatches. If tabs from different workspaces are dragged together:
- Rule 2: The target group's workspace won't have
activeTabIdsortabHistoryStacksupdated- Rule 3: The new group will be created in the dragged tab's workspace, orphaning the target tab from its original workspace
Add an explicit check at the start:
if (draggedTab.workspaceId !== targetTab.workspaceId) { return state; // Deny cross-workspace drops }
🧹 Nitpick comments (2)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
139-139: Document or handle group-to-single and group-to-group drag scenarios.The current implementation only handles:
- Single → Self (Rule 1)
- Single → Group (Rule 2)
- Single → Single (Rule 3)
Unhandled scenarios silently return the original state (no-op):
- Group → Single
- Group → Group
If these scenarios should be disallowed, consider adding explicit checks and logging/warnings. If they should be supported, implement the logic.
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
3-7: Consider using a UUID library for ID generation.The current ID generation using
Date.now()andMath.random()is acceptable for UI purposes but has a small collision risk if multiple tabs are created in rapid succession. Consider using a standard UUID library (e.g.,uuidornanoid) for better collision resistance and entropy.Example using nanoid:
+import { nanoid } from 'nanoid'; + export const createNewTab = ( workspaceId: string, type: TabType = TabType.Single, ): Tab => { - const id = `tab-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`; + const id = `tab-${nanoid()}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx(1 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/stores/tabs/types.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
apps/desktop/src/renderer/stores/tabs/utils.ts (2)
apps/desktop/src/shared/types.ts (1)
TabType(23-30)apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(35-35)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (3)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
Tab(35-35)TabGroup(24-32)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(3-30)apps/desktop/src/shared/types.ts (1)
TabType(23-30)
🔇 Additional comments (6)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (4)
1-8: LGTM!The imports and interface definition are clear and well-structured.
10-22: LGTM!Good defensive programming with the early return when tabs aren't found.
28-38: LGTM!Rule 1 correctly creates a new tab when dragging onto itself and activates it.
41-81: parentId property verified—no issues found.The
parentIdproperty is correctly defined in theBaseTabinterface as an optional string field. The assignment in line 45 (parentId: targetTabId) is type-safe and valid. The code change is sound from a typing perspective.apps/desktop/src/renderer/stores/tabs/utils.ts (2)
15-20: LGTM!The Single tab creation logic is clean and correctly typed.
22-29: Implementation correctly matches TabGroup type definition.The code returns a valid
TabGroupstructure with all required properties:
type: TabType.Group— matches interface requirementlayout: null— valid perMosaicNode<string> | nulltypechildTabIds: []— valid perstring[]type- All
BaseTabproperties provided via spread operatorThe interface confirms no
panesproperty exists, and the emptychildTabIdsarray is the correct initial state for skeleton structures created before children are added viaaddChildTabToGroup. The previous review concern has been fully addressed.
| const groupId = `tab-${Date.now()}-group`; | ||
| const childTab1Id = `tab-${Date.now()}-child-1`; | ||
| const childTab2Id = `tab-${Date.now()}-child-2`; |
There was a problem hiding this comment.
Strengthen ID generation to prevent collisions.
The ID generation relies solely on Date.now() without a random suffix. If multiple drag operations occur in quick succession (within the same millisecond), IDs could collide. The createNewTab utility (used in Rule 1) includes Math.random() for uniqueness.
Apply this diff to align with the pattern in createNewTab:
- const groupId = `tab-${Date.now()}-group`;
- const childTab1Id = `tab-${Date.now()}-child-1`;
- const childTab2Id = `tab-${Date.now()}-child-2`;
+ const timestamp = Date.now();
+ const random = Math.random().toString(36).substring(2, 11);
+ const groupId = `tab-${timestamp}-${random}-group`;
+ const childTab1Id = `tab-${timestamp}-${random}-child-1`;
+ const childTab2Id = `tab-${timestamp}-${random}-child-2`;📝 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 groupId = `tab-${Date.now()}-group`; | |
| const childTab1Id = `tab-${Date.now()}-child-1`; | |
| const childTab2Id = `tab-${Date.now()}-child-2`; | |
| const timestamp = Date.now(); | |
| const random = Math.random().toString(36).substring(2, 11); | |
| const groupId = `tab-${timestamp}-${random}-group`; | |
| const childTab1Id = `tab-${timestamp}-${random}-child-1`; | |
| const childTab2Id = `tab-${timestamp}-${random}-child-2`; |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/drag-logic.ts around lines 85 to 87,
the group and child tab IDs use only Date.now(), which can collide if created
within the same millisecond; change the ID construction to include a randomness
suffix (e.g., append Math.random().toString(36).slice(2) or follow the
createNewTab pattern) so each ID becomes something like
`tab-${Date.now()}-${random}-group` / `tab-${Date.now()}-${random}-child-1` /
`tab-${Date.now()}-${random}-child-2`, ensuring unique IDs across rapid
sequential operations.
| @@ -0,0 +1,30 @@ | |||
| import { type Tab, type TabGroup, TabType } from "./types"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
Verify TabType definition consistency across the codebase.
The code uses TabType.Single and TabType.Group (enum-style access), but shared/types.ts defines TabType as a string union: "terminal" | "editor" | "browser" | "preview" | "group" | "port" | "diff". This union doesn't include "single", and the usage pattern (dot notation) suggests an enum rather than string literals.
Ensure that the local ./types.ts definition is consistent with shared/types.ts, or clarify whether these are intentionally different types.
🏁 Script executed:
#!/bin/bash
# Description: Check TabType definitions and usage across the codebase
# Check local types.ts definition
echo "=== Local TabType definition ==="
cat apps/desktop/src/renderer/stores/tabs/types.ts | rg -A 5 "export.*TabType"
# Check shared types.ts definition
echo -e "\n=== Shared TabType definition ==="
cat apps/desktop/src/shared/types.ts | rg -A 5 "export.*TabType"
# Find all usages of TabType.Single and TabType.Group
echo -e "\n=== Usage of TabType.Single ==="
rg -n "TabType\.Single" apps/desktop/src/renderer/
echo -e "\n=== Usage of TabType.Group ==="
rg -n "TabType\.Group" apps/desktop/src/renderer/Length of output: 2565
Critical type mismatch between local and shared TabType definitions.
Verification reveals a significant type inconsistency:
- Local definition (
apps/desktop/src/renderer/stores/tabs/types.ts):TabTypeis an enum withSingle = "single"andGroup = "group" - Shared definition (
apps/desktop/src/shared/types.ts):TabTypeis a union type with values"terminal" | "editor" | "browser" | "preview" | "group"(no"single")
The renderer code creates tab objects with type: "single" (via TabType.Single), but this value is not part of the shared TabType union. This breaks type safety and will cause type errors when shared code encounters these values.
Action required: Align the shared TabType union to include "single", or refactor the local enum to match the shared definition exactly.
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/utils.ts around lines 1 to 1, the code
imports a local TabType enum that defines "single" and "group" but the shared
types declare TabType as a union that lacks "single", causing type mismatches
when renderer produces type: "single"; fix by making the definitions consistent
— either add "single" to the shared TabType union in apps/shared/types.ts or
change the local enum to exactly mirror the shared union (e.g., replace
TabType.Single usages with the appropriate shared union value such as "terminal"
or another matching literal), update all usages and imports to the unified type,
and run TypeScript checks to ensure no remaining mismatches.
| const baseTab = { | ||
| id, | ||
| title: type === TabType.Single ? "New Tab" : "New Split View", | ||
| workspaceId, | ||
| isNew: true, | ||
| }; |
There was a problem hiding this comment.
Title assignment assumes only two tab types.
Line 10 uses a ternary that assigns "New Split View" for any non-Single type. While currently only Single and Group types are handled in the function, this could lead to incorrect titles if additional tab types are added in the future.
Consider making the title assignment more explicit:
const baseTab = {
id,
- title: type === TabType.Single ? "New Tab" : "New Split View",
+ title: type === TabType.Single ? "New Tab" : type === TabType.Group ? "New Split View" : "New Tab",
workspaceId,
isNew: true,
};📝 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 baseTab = { | |
| id, | |
| title: type === TabType.Single ? "New Tab" : "New Split View", | |
| workspaceId, | |
| isNew: true, | |
| }; | |
| const baseTab = { | |
| id, | |
| title: type === TabType.Single ? "New Tab" : type === TabType.Group ? "New Split View" : "New Tab", | |
| workspaceId, | |
| isNew: true, | |
| }; |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/stores/tabs/utils.ts around lines 8 to 13, the
title assignment uses a ternary that assumes only Single vs non-Single types;
update it to map each TabType explicitly (e.g., switch or a lookup object) so
TabType.Single => "New Tab", TabType.Group => "New Split View", and provide a
safe default title for unknown/future types (e.g., "New Tab" or a localized
fallback). Ensure the implementation is exhaustive or includes a default branch
so adding new TabType values won't silently assign the wrong title.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (2)
54-61: Guard against dragging tabs across different workspaces.
workspaceIdis derived fromdraggedTabonly, but the code never validates thattargetTab.workspaceIdmatches. If a cross-workspace drop occurs, Rules 2, 3, and 4 will updateactiveTabIdsandtabHistoryStacksonly for the dragged tab's workspace, potentially removing the target tab from its original workspace and corrupting state.Apply this diff to add workspace validation:
const workspaceId = draggedTab.workspaceId; + + // Prevent cross-workspace drags + if (draggedTab.workspaceId !== targetTab.workspaceId) { + return state; + } + const historyStack = state.tabHistoryStacks[workspaceId] || [];
178-180: Strengthen ID generation to prevent collisions.The ID generation uses only
Date.now()without a random suffix. Multiple drag operations within the same millisecond will produce colliding IDs. ThecreateNewTabutility addsMath.random()for uniqueness.Apply this diff to align with
createNewTab:- const groupId = `tab-${Date.now()}-group`; - const childTab1Id = `tab-${Date.now()}-child-1`; - const childTab2Id = `tab-${Date.now()}-child-2`; + const timestamp = Date.now(); + const random = Math.random().toString(36).substring(2, 11); + const groupId = `tab-${timestamp}-${random}-group`; + const childTab1Id = `tab-${timestamp}-${random}-child-1`; + const childTab2Id = `tab-${timestamp}-${random}-child-2`;apps/desktop/src/renderer/stores/tabs/store.ts (2)
228-246: Guard against undefinedchildTabIdsin group tabs.Line 234 spreads
tab.childTabIdswithout checking if it's defined. AlthoughcreateNewTabinitializeschildTabIds: [], there's no TypeScript guarantee that allTabGroupinstances have this field populated, and future code paths could create groups without it.Apply this diff to add defensive checks:
addChildTabToGroup: (groupId, childTabId) => { set((state) => ({ tabs: state.tabs.map((tab) => { if (tab.id === groupId && tab.type === TabType.Group) { + const childTabIds = tab.childTabIds ?? []; + // Prevent duplicate additions + if (childTabIds.includes(childTabId)) return tab; return { ...tab, - childTabIds: [...tab.childTabIds, childTabId], + childTabIds: [...childTabIds, childTabId], }; } if (tab.id === childTabId) { return { ...tab, parentId: groupId, }; } return tab; }), })); },
248-315: Guard against undefinedchildTabIdswhen removing children.Line 255 calls
group.childTabIds.filter(...)without verifyingchildTabIdsis defined. While there's a type guard at lines 250-253, TypeScript doesn't guarantee the field exists at runtime.Apply this diff to add defensive handling:
const group = state.tabs.find( (tab) => tab.id === groupId && tab.type === TabType.Group, ); if (!group || group.type !== TabType.Group) return state; - const updatedChildTabIds = group.childTabIds.filter( + const updatedChildTabIds = (group.childTabIds ?? []).filter( (id) => id !== childTabId, );
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView.tsx (1)
11-46: Consider implementing button handlers in a follow-up.The action buttons are currently non-functional placeholders. While this is fine for an initial UI implementation, consider adding onClick handlers or tracking this as follow-up work to make the new workspace onboarding fully interactive.
Would you like me to open an issue to track implementing these action handlers?
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx (1)
44-50: Consider explicit mode handling for better type safety.The current implementation uses a fallback pattern where any mode that isn't
SidebarMode.ChangesrendersTabsView. While this works given the two-mode constraint on line 11, a more explicit switch or exhaustive check would improve maintainability if additional modes are added in the future.Consider this more explicit pattern:
{(mode) => { if (mode === SidebarMode.Changes) { return <ChangesView />; } - - return <TabsView />; + if (mode === SidebarMode.Tabs) { + return <TabsView />; + } + // Exhaustive check - TypeScript will error if new modes are added + const _exhaustive: never = mode; + return _exhaustive; }}apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
42-46: Add explicit type check for GroupTabView.Line 46 implicitly assumes any non-
Singletab is aGroup, relying on the exhaustiveness of theTabunion. While currently safe, an explicit check improves clarity and guards against future type expansion.Apply this diff to add explicit type checking:
if (tabToRender.type === TabType.Single) { return <SingleTabView tab={tabToRender} />; } - return <GroupTabView tab={tabToRender} />; + if (tabToRender.type === TabType.Group) { + return <GroupTabView tab={tabToRender} focusedChildId={focusedChildId} />; + } + + // Exhaustive check: should never reach here + return <EmptyTabView />;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/mosaic-theme.css(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx(1 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/mosaic-theme.css
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (2)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
SingleTab(16-18)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useDropTabTarget.ts (1)
useDropTabTarget(6-34)
apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
WorkspacesTabs(11-123)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
apps/desktop/src/shared/types.ts (2)
TabType(23-30)MosaicNode(35-35)apps/old-desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx (1)
TabGroup(18-302)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ModeCarousel/types.ts (1)
SidebarMode(5-5)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView.tsx (1)
ChangesView(1-7)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (1)
TabsView(11-55)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (3)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
Tab(26-26)TabGroup(20-24)apps/desktop/src/shared/types.ts (1)
TabType(23-30)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(3-30)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (6)
apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)apps/desktop/src/renderer/stores/tabs/store.ts (2)
useTabs(340-340)useActiveTabIds(341-342)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsx (1)
EmptyTabView(1-18)apps/desktop/src/shared/types.ts (1)
TabType(23-30)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (1)
SingleTabView(8-48)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx (1)
GroupTabView(19-82)
apps/desktop/src/renderer/stores/tabs/store.ts (4)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(26-26)apps/desktop/src/shared/types.ts (2)
TabType(23-30)MosaicNode(35-35)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(3-30)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
handleDragTabToTab(45-233)
🪛 GitHub Actions: CI
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx
[error] 1-1: Command failed: bun run typecheck (exit code 2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView.tsx
[error] 1-1: Command failed: bun run typecheck (exit code 2)
apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx
[error] 1-1: Command failed: bun run typecheck (exit code 2)
apps/desktop/src/renderer/stores/tabs/types.ts
[error] 1-1: Command failed: bun run typecheck (exit code 2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx
[error] 1-1: Command failed: bun run typecheck (exit code 2)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts
[error] 1-1: Command failed: bun run typecheck (exit code 2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
[error] 1-1: Command failed: bun run typecheck (exit code 2)
apps/desktop/src/renderer/stores/tabs/store.ts
[error] 1-1: Command failed: bun run typecheck (exit code 2)
🔇 Additional comments (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/NewWorkspaceView.tsx (2)
3-3: Verify the removed background class is intentional.The
bg-backgroundclass was removed from the outer wrapper. Ensure this is intentional and doesn't cause visual issues when the workspace view inherits a different background.
1-1: Rename migration is complete; all imports properly updated.Verification confirms:
- No remaining references to the old
NewTabViewname in the codebaseNewWorkspaceViewis correctly imported inapps/desktop/src/renderer/screens/main/components/WorkspaceView/index.tsx(line 3)- Component is properly exported and used as
<NewWorkspaceView />(line 15)The import verification concern from the original review is resolved.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx (2)
23-23: LGTM!The className update removes border and background classes as intended, keeping only the essential layout classes. The Tailwind CSS classes are valid and appropriate for the sidebar container.
3-3: Ignore this review comment — the import path is correct.The analysis in this review is incorrect. The import on line 3 (
import { SidebarMode } from "renderer/stores/sidebar-state";) is correct. SidebarMode is properly defined as an enum inapps/desktop/src/renderer/stores/sidebar-state.ts, not in ModeCarousel/types.ts. The original review conflated the current codebase structure (apps/desktop) with the legacy folder (apps/old-desktop), where ModeCarousel/types.ts exists as a type definition. The import path is accurate and usage throughout the codebase is correct.Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/screens/main/components/TopBar/index.tsx (1)
1-27: Typecheck structure validates correctly; unable to confirm reported failures in sandbox environment.The code structure is sound:
- WorkspacesTabs component is properly defined in
WorkspaceTabs/index.tsxwith correct export:export function WorkspacesTabs()- Import path
"./WorkspaceTabs"correctly resolves to the directory's index file- All referenced subcomponents (AddWorkspaceButton, WorkspaceItem) exist
- TopBar imports and component usage are syntactically valid
However, I cannot execute the full typecheck pipeline in the sandbox environment (bun/turbo not available). If a typecheck failure is reported by your CI pipeline, it likely stems from:
- Type issues in imported dependencies (useWorkspacesStore, trpc, or UI components)
- Store or hook type definitions
- Not from the TopBar or WorkspacesTabs structure itself
Verify the actual typecheck output on your system to identify the specific type error location.
apps/desktop/src/renderer/stores/tabs/types.ts (1)
3-6: Review comment is inaccurate—no actual TabType naming collision exists.The codebase defines two separate
TabTypeentities serving different architectural purposes: the local enum inrenderer/stores/tabs/types.ts("single" | "group") and the shared type inshared/types.ts(string union of terminal types). However, no file importsTabTypefromshared/types, and no file imports both definitions simultaneously. The types are never in scope together, making the claimed "ambiguity" and "typecheck failures" unfounded. These are distinct type systems for separate concerns with no actual conflict.Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/stores/tabs/store.ts (1)
1-350: Review comment is factually incorrect—the alleged root causes do not match the actual codebase.The investigation reveals:
TabType "naming collision": While two different
TabTypedefinitions exist (stores/tabs/types.tsenum vsshared/types.tsstring union), they are never imported together in the same file. Zero cross-import conflicts found.Type assertion in SingleTabView.tsx line 13: The pattern
ref={drop as unknown as React.Ref<HTMLDivElement>}is standard react-dnd practice for drop targets, not a problem."Missing type guards" in TabsContent/index.tsx: Type guards are present via discriminated union:
if (tabToRender.type === TabType.Single) { return <SingleTabView /> } return <GroupTabView />correctly narrows the union type without explicit assertions.The alleged root causes do not explain any actual typecheck failures in this codebase section.
Likely an incorrect or invalid review comment.
| if (activeTab.parentId) { | ||
| const parentGroup = allTabs.find((tab) => tab.id === activeTab.parentId); | ||
| return { | ||
| tabToRender: parentGroup || null, | ||
| focusedChildId: activeTabId, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Missing type guard when finding parent group.
Line 28 finds the parent group by ID but doesn't verify it's actually a TabGroup type. If parentId incorrectly points to a SingleTab, the GroupTabView will receive an invalid tab type, potentially causing runtime errors.
Apply this diff to add type validation:
if (activeTab.parentId) {
const parentGroup = allTabs.find((tab) => tab.id === activeTab.parentId);
+ if (parentGroup && parentGroup.type === TabType.Group) {
return {
tabToRender: parentGroup || null,
focusedChildId: activeTabId,
};
+ }
+ // Parent ID is invalid or not a group
+ return { tabToRender: null, focusedChildId: null };
}📝 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.
| if (activeTab.parentId) { | |
| const parentGroup = allTabs.find((tab) => tab.id === activeTab.parentId); | |
| return { | |
| tabToRender: parentGroup || null, | |
| focusedChildId: activeTabId, | |
| }; | |
| } | |
| if (activeTab.parentId) { | |
| const parentGroup = allTabs.find((tab) => tab.id === activeTab.parentId); | |
| if (parentGroup && parentGroup.type === TabType.Group) { | |
| return { | |
| tabToRender: parentGroup || null, | |
| focusedChildId: activeTabId, | |
| }; | |
| } | |
| // Parent ID is invalid or not a group | |
| return { tabToRender: null, focusedChildId: null }; | |
| } |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
around lines 27 to 33, the code finds a parent tab by ID but doesn't verify it's
a TabGroup, risking passing a SingleTab to GroupTabView; modify the lookup to
perform a type guard (e.g., use an existing isTabGroup(tab) helper or check a
distinguishing property like tab.type === 'group' or presence of children) and
only set tabToRender to the parent if the guard confirms it's a TabGroup;
otherwise set tabToRender to null (and keep focusedChildId as before) so
GroupTabView never receives an invalid tab type.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (2)
168-172: Align group/child tab ID generation withcreateNewTabto avoid collisions.Group and child tab IDs for Rule 4 are based solely on
Date.now(), which can collide if multiple split‑group creations occur within the same millisecond.createNewTabinapps/desktop/src/renderer/stores/tabs/utils.tsalready uses a timestamp + random suffix pattern for uniqueness.Reuse that approach here:
- if (targetTab.type === TabType.Single && draggedTab.type === TabType.Single) { - const groupId = `tab-${Date.now()}-group`; - const childTab1Id = `tab-${Date.now()}-child-1`; - const childTab2Id = `tab-${Date.now()}-child-2`; + if (targetTab.type === TabType.Single && draggedTab.type === TabType.Single) { + const timestamp = Date.now(); + const random = Math.random().toString(36).substring(2, 11); + const groupId = `tab-${timestamp}-${random}-group`; + const childTab1Id = `tab-${timestamp}-${random}-child-1`; + const childTab2Id = `tab-${timestamp}-${random}-child-2`;
56-63: Guard against cross‑workspace drops before mutating per‑workspace state.
workspaceIdis derived fromdraggedTab.workspaceId, but there is no check thattargetTab.workspaceIdmatches. If a cross‑workspace drop slips through the UI, this will updateactiveTabIdsandtabHistoryStacksfor the dragged tab’s workspace while conceptually acting on a tab from another workspace.Add an explicit workspace equality check and bail out on mismatch:
const draggedTab = state.tabs.find((tab) => tab.id === draggedTabId); const targetTab = state.tabs.find((tab) => tab.id === targetTabId); if (!draggedTab || !targetTab) return state; + + if (draggedTab.workspaceId !== targetTab.workspaceId) { + return state; + } + const workspaceId = draggedTab.workspaceId; const historyStack = state.tabHistoryStacks[workspaceId] || [];
🧹 Nitpick comments (2)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (2)
64-78: Clarify behavior for self‑drops of non‑single tabs.Rule 1 duplicates a tab when it’s dropped onto itself, but only blocks when
draggedTab.parentIdis truthy. If group tabs (or other non‑single tab types) can ever be drag sources/targets, dropping a group onto itself will create a new single tab viacreateNewTab, which may not be intended.If group tabs are not meant to support this behavior, consider tightening the condition:
- if (draggedTabId === targetTabId) { + if (draggedTabId === targetTabId && draggedTab.type === TabType.Single) {Or explicitly document that self‑dropping a group creates a fresh single tab so future maintainers understand this behavior.
4-8: Reduce duplication betweenDragTabToTabResultand thestateparameter type.
DragTabToTabResultand the inlinestateparameter type share the same shape (tabs,activeTabIds,tabHistoryStacks). To keep types in sync and ease future refactors, consider extracting a shared type alias and reusing it for both the param and return type.For example:
type TabsSlice = { tabs: Tab[]; activeTabIds: Record<string, string | null>; tabHistoryStacks: Record<string, string[]>; }; export interface DragTabToTabResult extends TabsSlice {} export const handleDragTabToTab = ( draggedTabId: string, targetTabId: string, state: TabsSlice, ): DragTabToTabResult => { /* ... */ }Also applies to: 47-55
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (2)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
Tab(26-26)TabGroup(20-24)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(3-30)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (3)
10-24: Keep group layout in sync when removing a child tab
removeFromOldParentonly filterschildTabIdsand leaves the group'slayoutuntouched. After moving a tab between groups, the old group's layout can still reference the removed tab, causing inconsistent state / duplicate rendering.Update this helper to also strip the tab ID from the group's
layout, collapsing empty branches where needed:const removeFromOldParent = ( tabs: Tab[], tabId: string, oldParentId: string, ): Tab[] => { - return tabs.map((tab) => { - if (tab.id === oldParentId && tab.type === TabType.Group) { - return { - ...tab, - childTabIds: tab.childTabIds.filter((id) => id !== tabId), - }; - } - return tab; - }); + const removeFromLayout = ( + layout: TabGroup["layout"], + idToRemove: string, + ): TabGroup["layout"] => { + if (!layout) return layout; + if (typeof layout === "string") { + return layout === idToRemove ? null : layout; + } + + const first = removeFromLayout(layout.first, idToRemove); + const second = removeFromLayout(layout.second, idToRemove); + + if (!first && !second) return null; + if (!first) return second; + if (!second) return first; + + return { + ...layout, + first, + second, + }; + }; + + return tabs.map((tab) => { + if (tab.id === oldParentId && tab.type === TabType.Group) { + return { + ...tab, + childTabIds: tab.childTabIds.filter((id) => id !== tabId), + layout: removeFromLayout(tab.layout, tabId), + }; + } + return tab; + }); };
56-63: Guard against dragging tabs across different workspaces
workspaceIdis derived from the dragged tab and used to updateactiveTabIdsandtabHistoryStacks, but there's no check thattargetTab.workspaceIdmatches. If a cross‑workspace drop ever slips through, you'll remove/update tabs in one workspace while only adjusting metadata in another.Add an explicit workspace equality check and no‑op early return:
const draggedTab = state.tabs.find((tab) => tab.id === draggedTabId); const targetTab = state.tabs.find((tab) => tab.id === targetTabId); if (!draggedTab || !targetTab) return state; - const workspaceId = draggedTab.workspaceId; + if (draggedTab.workspaceId !== targetTab.workspaceId) { + return state; + } + + const workspaceId = draggedTab.workspaceId;
168-217: Fix Rule 4 reparenting (cleanup old parents) and harden group ID generationTwo issues in Rule 4:
- If either single tab already has a
parentId, the old group'schildTabIds/layoutare never updated, so those groups still reference tabs that have been reparented into the new group.groupIduses onlyDate.now(), which can collide; elsewhere (createNewTab) you include a random suffix.You can re-use
removeFromOldParentfor both tabs and strengthen ID generation in one pass:// Rule 4: Dragging single tab into another single tab - creates new group container for split view if (targetTab.type === TabType.Single && draggedTab.type === TabType.Single) { - const groupId = `tab-${Date.now()}-group`; + const timestamp = Date.now(); + const random = Math.random().toString(36).substring(2, 11); + const groupId = `tab-${timestamp}-${random}-group`; // Keep original tab IDs stable - just update their parentId const updatedTargetTab: Tab = { ...targetTab, parentId: groupId, @@ -197,13 +210,30 @@ childTabIds: [targetTab.id, draggedTab.id], // Use original IDs }; - return { - ...state, - tabs: [ - ...state.tabs.map((tab) => { - if (tab.id === targetTab.id) return updatedTargetTab; - if (tab.id === draggedTab.id) return updatedDraggedTab; - return tab; - }), - newGroupTab, - ], + let updatedTabs = state.tabs.map((tab) => { + if (tab.id === targetTab.id) return updatedTargetTab; + if (tab.id === draggedTab.id) return updatedDraggedTab; + return tab; + }); + + if (targetTab.parentId) { + updatedTabs = removeFromOldParent( + updatedTabs, + targetTab.id, + targetTab.parentId, + ); + } + + if (draggedTab.parentId) { + updatedTabs = removeFromOldParent( + updatedTabs, + draggedTab.id, + draggedTab.parentId, + ); + } + + return { + ...state, + tabs: [...updatedTabs, newGroupTab], activeTabIds: { ...state.activeTabIds, [workspaceId]: newGroupTab.id,
🧹 Nitpick comments (1)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
64-78: Clarify self‑drop behavior vs. commentThe Rule 1 comment says "duplicates standalone tabs", but the implementation calls
createNewTab, which creates a fresh tab (default title,isNew: true, etc.), not a semantic duplicate ofdraggedTab.Either adjust the comment to describe the current behavior ("opens a new blank tab when dropping onto itself") or copy relevant properties from
draggedTabinto the new tab so it behaves like a true duplicate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx(1 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (2)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
Tab(26-26)TabGroup(20-24)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(3-30)
🔇 Additional comments (1)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
47-221: Overall structure of drag rules looks goodThe decomposition into explicit rules with small helpers, immutable updates of
tabs/activeTabIds/tabHistoryStacks, and centralizedhandleDragTabToTabentrypoint is clean and easy to reason about. Once the edge cases noted in other comments are addressed, this should be a solid foundation for DnD behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (2)
235-261: Harden group ID generation to avoidDate.now()collisions
groupIdis built fromDate.now()only, so extremely fast consecutive drags could collide. SincecreateNewTabalready uses a timestamp + random suffix, it’s cheap to align with that pattern here.- if (targetTab.type === TabType.Single && draggedTab.type === TabType.Single) { - const groupId = `tab-${Date.now()}-group`; + if (targetTab.type === TabType.Single && draggedTab.type === TabType.Single) { + const groupId = `tab-${Date.now()}-${Math.random() + .toString(36) + .substring(2, 11)}-group`;
122-129: Guard against cross‑workspace drags inhandleDragTabToTab
workspaceIdis derived from the dragged tab, but there’s no check thattargetTab.workspaceIdmatches. If a cross‑workspace drop slips through the UI, you can end up with a group and children spanning different workspaces while only updatingactiveTabIds/tabHistoryStacksfor the dragged tab’s workspace.Add an explicit workspace equality check and no‑op when they differ.
const draggedTab = state.tabs.find((tab) => tab.id === draggedTabId); const targetTab = state.tabs.find((tab) => tab.id === targetTabId); if (!draggedTab || !targetTab) return state; + + // Prevent cross-workspace drags from corrupting tab/group state + if (draggedTab.workspaceId !== targetTab.workspaceId) { + return state; + }apps/desktop/src/renderer/stores/tabs/utils.ts (1)
1-29: ClarifyTabTypeusage and make title mapping future‑proofThis helper works, but there are two small maintainability nits:
- There’s a local
TabTypein./typesand a differentTabTypestring union inapps/desktop/src/shared/types.ts. Using the same name for distinct concepts is easy to confuse; consider renaming the local one (e.g.,TabsStoreTabType) or explicitly documenting that it’s separate.- The title ternary treats “non‑Single” as “New Split View”. If you ever extend the local
TabType, a small switch/lookup mapping per type (with a default) will be safer than the binary check.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts (1)
6-343: Solid coverage of drag rules; consider a cross‑workspace regression testThese tests exercise the main cleanLayout and drag scenarios well. Once you add a guard in
handleDragTabToTabto no‑op whendraggedTab.workspaceId !== targetTab.workspaceId, it would be worth adding a test that sets different workspaceIds for dragged/target tabs and asserts the state is unchanged.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (1)
7-48: Workspace-scoped tab list looks correct; child resolution can stay simpleThe TabsView correctly scopes top-level tabs to the active workspace and disables “New Tab” when there’s no active workspace. The per-parent
getChildTabsfilter overallTabsis fine given expected tab counts; if this ever grows large, you could memoize a parent→children map fromallTabs, but that’s not necessary now.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/mosaic-theme.css(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(1 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts (2)
apps/desktop/src/shared/types.ts (2)
MosaicNode(35-35)TabType(23-30)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (2)
cleanLayout(48-70)handleDragTabToTab(113-287)
apps/desktop/src/renderer/stores/tabs/drag-logic.ts (3)
apps/desktop/src/renderer/stores/tabs/types.ts (2)
Tab(26-26)TabGroup(20-24)apps/desktop/src/shared/types.ts (2)
MosaicNode(35-35)TabType(23-30)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(3-30)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (3)
apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)apps/desktop/src/renderer/stores/tabs/store.ts (2)
useTabs(374-374)useAddTab(378-378)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
TabItem(14-114)
apps/desktop/src/renderer/stores/tabs/utils.ts (2)
apps/desktop/src/shared/types.ts (1)
TabType(23-30)apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(26-26)
apps/desktop/src/renderer/stores/tabs/store.ts (4)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(26-26)apps/desktop/src/shared/types.ts (2)
TabType(23-30)MosaicNode(35-35)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (2)
cleanLayout(48-70)handleDragTabToTab(113-287)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
createNewTab(3-30)
🔇 Additional comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/mosaic-theme.css (1)
28-35: Nice:focus-visibletreatment for mosaic contentThe explicit focus outline and inset shadow on
.mosaic-window-contentgive good keyboard focus affordance without relying on default browser styles.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
10-36: Critical type mismatch between local and shared TabType definitions.The code uses
TabType.Single(which resolves to"single"), but the sharedTabTypeunion inapps/desktop/src/shared/types.tsdoesn't include"single"as a valid value. This breaks type safety when tabs are passed to shared code.As noted in previous reviews, align the type definitions:
- Either add
"single"to the sharedTabTypeunion, or- Map
TabType.Singleto an existing shared value like"terminal"See the existing review comment for detailed remediation steps.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx (1)
131-131: Consider making the theme configurable.The
"mosaic-theme-dark"class is hardcoded, which prevents supporting light themes or user preferences in the future.Consider accepting a theme prop or deriving it from a theme context:
+ const theme = useTheme(); // or accept as prop return ( <div className="w-full h-full mosaic-container"> <Mosaic<string> renderTile={renderPane} value={cleanedLayout} onChange={handleLayoutChange} - className="mosaic-theme-dark" + className={`mosaic-theme-${theme}`} dragAndDropManager={dragDropManager} /> </div> );apps/desktop/src/renderer/stores/tabs/store.ts (1)
255-274: Consider makingaddChildTabToGroupupdate the layout automatically.The comment on line 267 notes that "caller is responsible for layout updates," which creates a fragile two-step API where callers must remember to call both
addChildTabToGroupandupdateTabGroupLayout. This could lead to inconsistent state if a caller forgets the second step.Consider accepting an optional layout parameter or automatically adding the child to the layout:
- addChildTabToGroup: (groupId, childTabId) => { + addChildTabToGroup: (groupId, childTabId, layoutPosition?: 'first' | 'second') => { set((state) => { const updatedTabs = state.tabs.map((tab) => { if (tab.id === childTabId) { return { ...tab, parentId: groupId, }; } + if (tab.id === groupId && tab.type === TabType.Group && layoutPosition) { + // Update layout to include the new child + const newLayout = layoutPosition === 'first' + ? { direction: 'row', first: childTabId, second: tab.layout, splitPercentage: 50 } + : { direction: 'row', first: tab.layout, second: childTabId, splitPercentage: 50 }; + return { ...tab, layout: newLayout }; + } return tab; }); - // Note: This doesn't update layout - caller is responsible for layout updates - // This is typically used in conjunction with updateTabGroupLayout return { tabs: updatedTabs, }; }); },This would reduce the API surface and prevent inconsistent states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.test.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/mosaic-theme.css(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(1 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/types.ts(1 hunks)apps/desktop/src/renderer/stores/tabs/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/renderer/stores/tabs/types.ts
- apps/desktop/src/renderer/stores/tabs/drag-logic.ts
- apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts
🧰 Additional context used
🧬 Code graph analysis (6)
apps/desktop/src/renderer/stores/tabs/utils.ts (2)
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/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx (6)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
TabGroup(20-23)apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)apps/desktop/src/renderer/stores/tabs/store.ts (2)
useTabs(477-477)useTabsStore(116-475)apps/desktop/src/renderer/stores/tabs/utils.ts (1)
getChildTabIds(6-8)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
cleanLayout(48-70)apps/desktop/src/renderer/lib/dnd.ts (1)
dragDropManager(5-5)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.test.tsx (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-70)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx (1)
apps/desktop/src/renderer/stores/tabs/store.ts (2)
useUngroupTab(488-488)useTabs(477-477)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (4)
apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)apps/desktop/src/renderer/stores/tabs/store.ts (2)
useTabs(477-477)useAddTab(481-481)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx (1)
UngroupDropZone(16-94)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
TabItem(14-114)
apps/desktop/src/renderer/stores/tabs/store.ts (4)
apps/desktop/src/renderer/stores/tabs/types.ts (1)
Tab(25-25)apps/desktop/src/shared/types.ts (2)
TabType(23-30)MosaicNode(35-35)apps/desktop/src/renderer/stores/tabs/utils.ts (2)
getChildTabIds(6-8)createNewTab(10-36)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (3)
cleanLayout(48-70)handleDragTabToTab(110-283)removeTabFromLayout(15-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Unit Test
🔇 Additional comments (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.test.tsx (1)
1-169: LGTM! Comprehensive test coverage for group tab logic.The test suite thoroughly covers the critical scenarios for layout cleaning and child tab management, including edge cases like empty layouts and nested structures.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView.tsx (1)
45-135: LGTM! Well-structured mosaic group tab implementation.The component correctly handles layout changes, tab removal, and empty states. The integration with react-mosaic-component and the drag-drop manager is clean.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/mosaic-theme.css (1)
1-48: LGTM! Well-structured dark theme with good accessibility.The CSS uses custom properties effectively, includes appropriate transitions, and provides focus-visible states for accessibility.
apps/desktop/src/renderer/stores/tabs/utils.ts (1)
1-8: LGTM! Clean helper for deriving child tabs.The
getChildTabIdsfunction correctly filters tabs byparentIdand returns their IDs.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (1)
9-97: LGTM! Clean tabs view with drag-and-drop preview support.The component correctly filters workspace tabs, integrates with the drag-drop system to show previews, and handles the empty workspace state appropriately.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx (1)
16-94: LGTM! Robust drag-and-drop zone implementation.The component correctly handles drop events with proper defensive checks (
monitor.didDrop(),draggedTab?.parentId), computes drop indices accurately during hover, and exposes the right drag state through the render prop.apps/desktop/src/renderer/stores/tabs/store.ts (1)
116-475: LGTM! Comprehensive tab management store with proper state synchronization.The store correctly manages tabs, groups, active states, and history stacks across workspaces. The complex operations like
removeChildTabFromGroupandungroupTabhandle edge cases appropriately (empty groups, active tab updates, reordering).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (2)
52-53: Inconsistent drop indicator styling.The drop line indicator uses
bg-primaryat Line 53 butbg-sidebar-accentat Line 62. This inconsistency could confuse users about where the tab will be dropped.For consistency, use the same color for both indicators:
{/* Drop line indicator before this tab */} {isDragOver && draggedTab && index === dropIndex && ( <div className="absolute -top-0.5 left-0 right-0 h-0.5 bg-primary rounded-full z-10" /> )} <div data-tab-item> <TabItem tab={tab} childTabs={getChildTabs(tab.id)} /> </div> </motion.div> ))} {/* Drop line indicator at the end */} {isDragOver && draggedTab && dropIndex >= tabs.length && ( - <div className="h-0.5 bg-sidebar-accent rounded-full" /> + <div className="h-0.5 bg-primary rounded-full" /> )}Also applies to: 61-62
64-72: Fix "New Tab" button layout.The
justify-betweenclass on Line 67 will push the icon and text to opposite ends of the button, which is likely unintended for an icon-label button combination.Apply this diff to group the icon and text together:
<Button variant="ghost" onClick={handleAddTab} - className="w-full text-start group px-3 py-2 rounded-md cursor-pointer flex items-center justify-between" + className="w-full text-start group px-3 py-2 rounded-md cursor-pointer flex items-center gap-2" disabled={!activeWorkspaceId} > <HiMiniPlus className="size-4" /> <span className="truncate flex-1">New Tab</span> </Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx(1 hunks)apps/desktop/src/renderer/stores/tabs/store.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx
- apps/desktop/src/renderer/stores/tabs/store.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (5)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts (1)
TabItemProps(10-13)apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)apps/desktop/src/renderer/stores/tabs/store.ts (3)
useActiveTabIds(502-503)useRemoveTab(506-506)useSetActiveTab(507-508)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts (1)
useDragTab(5-42)apps/desktop/src/shared/types.ts (1)
TabType(23-30)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/index.tsx (4)
apps/desktop/src/renderer/stores/workspaces.ts (1)
useWorkspacesStore(27-90)apps/desktop/src/renderer/stores/tabs/store.ts (2)
useTabs(501-501)useAddTab(505-505)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/UngroupDropZone/index.tsx (1)
UngroupDropZone(16-101)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
TabItem(14-114)
🔇 Additional comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
99-111: Verify if nested group tabs (groups within groups) should be supported.Line 105 passes an empty array for
childTabs, which means if a child tab is itself a group, its children won't be rendered. If nested groups are expected in your design, this will silently fail to display them.If nested groups should be supported, apply this diff:
{isGroupTab && hasChildren && isExpanded && ( <div className="ml-4 mt-1 space-y-1"> {childTabs.map((childTab) => { return ( <div key={childTab.id} className="flex items-start gap-1"> <div className="flex-1"> - <TabItem tab={childTab} childTabs={[]} /> + <TabItem tab={childTab} childTabs={getChildTabs(childTab.id)} /> </div> </div> ); })} </div> )}Where
getChildTabsis passed as a prop or imported from the parent component. Alternatively, if nested groups are not part of the design, consider adding a comment to clarify this limitation.
1 participant
Summary by CodeRabbit
New Features
Enhancements
Refactor
Removed
Tests
✏️ Tip: You can customize this high-level summary in your review settings.