Move the tabs store to the tabs router#128
Conversation
WalkthroughAdds a new tabs TRPC router and layout utilities, migrates tab storage into DB with a discriminated Tab union, switches workspace activation to isActive/activeTabId, introduces projects.getAllWithWorkspaces aggregation, and rewires renderer components/hooks to use TRPC-driven tab/workspace flows and new react-query tab hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Renderer UI
participant RQ as React Query
participant Tabs as Tabs Router (TRPC)
participant DB as In-Memory DB
participant TM as TerminalManager
UI->>RQ: call mutation / query (e.g., tabs.create / tabs.getByWorkspace)
RQ->>Tabs: TRPC request
Tabs->>DB: read/update tabs & workspaces (isActive/activeTabId)
Tabs->>TM: create/kill terminals (when needed)
TM-->>Tabs: ack
Tabs-->>DB: persist changes (layout updates, deletions)
Tabs-->>RQ: return result (success/warning)
RQ-->>UI: invalidate caches (tabs, workspaces) and provide updated data
sequenceDiagram
participant UI as TopBar UI
participant RQ as React Query
participant Projects as Projects Router
participant Utils as getAllWithWorkspaces
participant DB as In-Memory DB
UI->>RQ: query projects.getAllWithWorkspaces
RQ->>Projects: getAllWithWorkspaces()
Projects->>DB: read projects & workspaces
Projects->>Utils: assemble sorted projects with workspaces
Utils-->>Projects: ProjectWithWorkspaces[]
Projects-->>RQ: return data
RQ-->>UI: render nested projects → workspaces (active via isActive)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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: 1
🧹 Nitpick comments (6)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (3)
9-51: Share Mosaic layout helpers instead of duplicating logic
extractTabIdsFromLayoutandremoveTabFromLayoutduplicate the layout logic already present inapps/desktop/src/renderer/stores/tabs/drag-logic.ts. Keeping two copies risks subtle divergence if one is updated but not the other.Consider moving these helpers into a shared module (e.g., a
shared/tabs-layoututility) and importing them in both the renderer and router.
244-282: ClamptargetIndexinreorderfor more defensive behavior
targetIndexis accepted as an unconstrainednumber; passing a negative value or one larger thanworkspaceTabs.lengthwill still “work” viaArray.prototype.splicebut with surprising semantics.Consider clamping the target index into the valid range before reordering:
const currentIndex = workspaceTabs.findIndex( (t) => t.id === input.tabId, ); if (currentIndex === -1) return; - // Reorder - const [moved] = workspaceTabs.splice(currentIndex, 1); - workspaceTabs.splice(input.targetIndex, 0, moved); + // Reorder with bounded target index + const boundedTargetIndex = Math.max( + 0, + Math.min(input.targetIndex, workspaceTabs.length - 1), + ); + const [moved] = workspaceTabs.splice(currentIndex, 1); + workspaceTabs.splice(boundedTargetIndex, 0, moved);This makes the mutation robust against bad inputs while preserving intended behavior for valid indices.
78-118: Consider whethercreateshould also mark the workspace active
createalways setsworkspace.activeTabIdto the new tab but does not touchworkspace.isActiveorlastOpenedAt. Ifcreatecan ever be called for an inactive workspace (e.g., via a context menu), you may want to mirror part ofsetActive’s behavior so that “create tab” also activates that workspace and updates its recency.If the UI already guarantees you only call
createon the active workspace and you rely solely onsetActivefor activation, this is fine as-is.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
108-111: UsingisActiveas the single source of truth for active workspace
getActivenow derives the active workspace directly viaw.isActive, andsetActivefirst clears allisActiveflags, then marks the target workspace active while updating its timestamps. This centralizes active state in the workspace record and is consistent with the new renderer logic.If
getRecentsis intended to reflect “last selected project” rather than just “last opened via dialog/create”, consider also bumping the owning project’slastOpenedAtinsidesetActivewhen switching between projects; otherwise the current behavior is also acceptable.Also applies to: 301-319
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
13-44: Tabs now correctly consume projects-with-workspaces and useisActivefor navigationSwitching to
trpc.projects.getAllWithWorkspaces.useQuery(), flatteninggroupsintoallWorkspaces, and derivingactiveWorkspacefromw.isActivekeeps the top‑bar tabs in sync with the new server data shape. The hotkey handlers and per‑project rendering viaWorkspaceGroupare consistent with the global workspace ordering.If you want to slightly reduce effect/hotkey re‑runs, you could memoize
allWorkspacesor depend only onallWorkspaces.lengthwhere applicable, but the current implementation is functionally sound.Also applies to: 93-103
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsx (1)
44-47: Consider defensive coding for edge cases.While TypeScript should enforce the
workspacesproperty exists, adding optional chaining or a fallback would improve runtime safety:<AnimatePresence initial={false}> {(isCollapsed - ? project.workspaces.filter((w) => w.isActive) - : project.workspaces + ? project.workspaces?.filter((w) => w.isActive) ?? [] + : project.workspaces ?? [] ).map((workspace, index) => (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
apps/desktop/src/lib/trpc/routers/index.ts(2 hunks)apps/desktop/src/lib/trpc/routers/projects/projects.ts(2 hunks)apps/desktop/src/lib/trpc/routers/projects/utils/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/projects/utils/utils.ts(1 hunks)apps/desktop/src/lib/trpc/routers/tabs/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/tabs/tabs.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/git/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/utils.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(6 hunks)apps/desktop/src/main/lib/db/schemas.ts(2 hunks)apps/desktop/src/renderer/react-query/projects/useReorderProjects.ts(1 hunks)apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsx(4 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
apps/desktop/src/lib/trpc/routers/projects/utils/utils.ts (1)
apps/desktop/src/main/lib/db/schemas.ts (2)
Project(3-11)Workspace(21-32)
apps/desktop/src/lib/trpc/routers/workspaces/utils/utils.ts (2)
apps/desktop/src/main/lib/db/schemas.ts (2)
Project(3-11)Workspace(21-32)apps/desktop/src/lib/trpc/routers/projects/utils/utils.ts (1)
getAllWithWorkspaces(6-24)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (5)
apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
removeTabFromLayout(15-43)apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)apps/desktop/src/main/lib/db/schemas.ts (1)
Tab(48-50)apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(399-399)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsx (1)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
ProjectWithWorkspaces(124-124)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (3)
apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)apps/desktop/src/lib/trpc/routers/workspaces/utils/utils.ts (1)
findAdjacentWorkspace(18-46)apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(399-399)
apps/desktop/src/lib/trpc/routers/index.ts (1)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (1)
createTabsRouter(53-402)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (2)
apps/desktop/src/lib/trpc/routers/projects/utils/utils.ts (1)
getAllWithWorkspaces(6-24)apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsx (1)
WorkspaceGroup(15-73)
apps/desktop/src/main/lib/db/schemas.ts (1)
apps/desktop/src/shared/types.ts (4)
MosaicNode(35-35)Tab(44-56)Worktree(58-68)Workspace(70-82)
🔇 Additional comments (18)
apps/desktop/src/lib/trpc/routers/workspaces/utils/git/index.ts (1)
1-1: Barrel export matches existing utils patternThe re-export from
"./git"is consistent with other barrels and keeps import paths tidy. No issues.apps/desktop/src/lib/trpc/routers/tabs/index.ts (1)
1-1: Tabs router barrel looks goodThe
export * from "./tabs"barrel cleanly exposes the tabs router and aligns with the other router barrels.apps/desktop/src/lib/trpc/routers/index.ts (1)
5-19: Tabs router is correctly wired into AppRouterThe
createTabsRouterimport andtabs: createTabsRouter()entry follow the existing router composition pattern; theAppRoutertype now correctly exposes atabsbranch.apps/desktop/src/lib/trpc/routers/projects/utils/utils.ts (1)
1-24:getAllWithWorkspacesimplementation matches documented orderingFiltering by
tabOrder !== nulland then sorting projects and their workspaces bytabOrderis straightforward and correct; the non-null assertion ontabOrderis safe after the filter.apps/desktop/src/main/lib/db/schemas.ts (1)
1-64: Tab and workspace schema evolution looks consistent with router usageThe
Workspaceadditions (activeTabIdandisActive) and the newBaseTab/Tabunion align with how the tabs router reads and writes these fields;Database.tabsanddefaultDatabase.tabsare coherent with that model, and usingMosaicNode<string>as a type keeps layout typing precise without adding a runtime dependency.apps/desktop/src/renderer/react-query/projects/useReorderProjects.ts (1)
6-9: Updated cache invalidation aligns with newgetAllWithWorkspacesqueryInvalidating
utils.projects.getAllWithWorkspacesafter a reorder matches the new data shape used by the UI and should properly refresh both project order and their associated workspaces.apps/desktop/src/lib/trpc/routers/workspaces/utils/index.ts (1)
1-1: Workspaces utils barrel is consistent and correctThe
export * from "./utils"barrel follows the same pattern as other utility modules and simplifies imports without changing behavior.apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.ts (1)
14-18: New projects.getAllWithWorkspaces invalidation is appropriateInvalidating
utils.projects.getAllWithWorkspacesafter a workspace reorder matches the new data source used by the tabs UI; the mutation’s onSuccess flow looks consistent and safe.apps/desktop/src/lib/trpc/routers/projects/utils/index.ts (1)
1-1: Barrel re‑export for project utils looks goodRe‑exporting from
"./utils"is a simple, consistent way to expose the helper(s) (e.g.getAllWithWorkspaces) and aligns with how other routers consume this module.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
11-11: getAllWithWorkspaces router and type alias are consistent with the shared utilWiring
getAllWithWorkspacesthrough a publicProcedure that passesdb.data.projectsanddb.data.workspacesis straightforward, and derivingProjectWithWorkspacesfromReturnType<typeof getAllWithWorkspaces>[number]keeps the public type aligned with the helper’s implementation.Also applies to: 116-118, 124-124
apps/desktop/src/lib/trpc/routers/workspaces/utils/utils.ts (1)
1-46: Adjacent-workspace selection utility matches documented behavior
findAdjacentWorkspacecorrectly reusesgetAllWithWorkspacesfor global ordering, then returns the previous workspace, falling back to the next, orundefinedwhen none remain. This aligns with the comment and the top‑bar navigation semantics.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (3)
3-13: ConfirmfindAdjacentWorkspaceexport from./utils
findAdjacentWorkspaceis defined inworkspaces/utils/utils.ts, while this file imports it from"./utils". That assumes the./utilsbarrel (or index) re‑exportsfindAdjacentWorkspace; otherwise this import will fail and should instead target the concrete module that does export it.
36-92: Create mutation’s active-workspace behavior is coherentNew workspaces initialize with
isActive: true, and thedb.updateblock deactivates all existing workspaces before inserting the new one, ensuring there is exactly one active workspace globally. ProjecttabOrderbootstrapping also remains intact. This matches the UI’s single‑active‑workspace model.
211-299: Delete flow correctly handles worktree, tabs, active workspace, and terminalsThe delete mutation now:
- Guards against missing workspaces and worktree‑removal failures.
- Computes
deletedTabIdsand, when deleting the active workspace, selects an adjacent workspace usingfindAdjacentWorkspace.- Within
db.update, removes all tabs for the workspace, deletes the workspace and worktree records, clearsproject.tabOrderwhen it loses its last workspace, and activates the chosen adjacent workspace if present.- After DB mutation, iterates
deletedTabIdsand callsterminalManager.kill({ tabId })to clean up terminals.This sequence keeps DB state and terminal lifecycle in sync and preserves a single active workspace where possible.
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsx (4)
3-13: LGTM! Clean refactoring of props interface.The consolidation from multiple separate props to a single
project: ProjectWithWorkspacesobject simplifies the component API and reduces prop drilling.
28-30: LGTM! Correct property access.The project properties are correctly accessed from the unified
projectobject.Also applies to: 40-40
61-61: LGTM! Improved active state management.Passing
workspace.isActivedirectly is cleaner than the previous comparison-based approach.
44-46: No issues found. Code is correct.The verification confirms the implementation is safe. The
setActivemutation (lines 301-319 inapps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts) enforces a global constraint by deactivating all workspaces and then activating only the target one. This ensures exactly one workspace hasisActive: trueacross the entire application, even though the data model technically permits multiple. The filtering logic in WorkspaceGroup.tsx will therefore show at most one workspace when collapsed, making the behavior change fromactiveWorkspaceIdto per-workspaceisActiveflags both intentional and correct.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
224-245: Avoid deactivating all workspaces when setActive receives an invalid idIn
setActive, you currently clearisActiveon all workspaces before checking whether the target workspace exists. If an invalid/stale id is passed (e.g., race with deletion), you’ll throw but still leave the system with no active workspace, which is hard to recover from in the UI.You can first look up the workspace, then adjust
isActiveflags atomically, e.g.:- await db.update((data) => { - // Deactivate all workspaces - for (const ws of data.workspaces) { - ws.isActive = false; - } - - // Activate target workspace - const workspace = data.workspaces.find((w) => w.id === input.id); - if (!workspace) { - throw new Error(`Workspace ${input.id} not found`); - } - - workspace.isActive = true; - workspace.lastOpenedAt = Date.now(); - workspace.updatedAt = Date.now(); - }); + await db.update((data) => { + const workspace = data.workspaces.find((w) => w.id === input.id); + if (!workspace) { + throw new Error(`Workspace ${input.id} not found`); + } + + // Ensure exactly one active workspace + for (const ws of data.workspaces) { + ws.isActive = ws.id === workspace.id; + } + + workspace.lastOpenedAt = Date.now(); + workspace.updatedAt = Date.now(); + });This preserves the “single active workspace” invariant and avoids corrupting state when the id is invalid.
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (1)
133-150: Implement user-facing notification for worktree removal failures and audit warning display.The warning message for failed worktree removal is never shown to the user. In
DeleteWorkspaceDialog.tsx(line 38-39), there's a TODO noting "Show toast notification with warning" but it's unimplemented—onlyconsole.warn()is called, which most users won't see. Combined with the lack of any cleanup or retry mechanism for orphaned worktrees, users can unknowingly leave orphaned directories on disk with no way to know they need manual cleanup.Required fixes:
- Implement the TODO in
DeleteWorkspaceDialog.tsxto display the warning via toast/notification (not just console)- Add a mechanism to track or clean up orphaned worktrees (retry logic, background job, or health check)
- Update the generic dialog description to acknowledge that worktree removal may fail and require manual cleanup
♻️ Duplicate comments (2)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (2)
80-138: Critical: StaleactiveTabIdwhen removing group children or child tabsThe current implementation only resets
workspace.activeTabIdwhen it equals the top-leveltab.idbeing removed (line 129). This creates two critical scenarios whereactiveTabIdbecomes stale:
- Removing a group tab: Children are deleted (lines 95-96), but if
workspace.activeTabIdpoints to one of those child IDs, it won't be updated.- Removing a child tab that triggers parent deletion: When a child is removed and the parent becomes empty (lines 110-112), if
workspace.activeTabIdequals the parent group ID, it won't be updated.This causes
getActiveto returnnulleven when other valid tabs exist, breaking the UI state.Track all removed tab IDs and update
activeTabIdwhen it's among them:remove: publicProcedure .input(z.object({ id: z.string() })) .mutation(async ({ input }) => { const tab = db.data.tabs.find((t) => t.id === input.id); if (!tab) { return { success: false, error: "Tab not found" }; } await db.update((data) => { + const removedTabIds = new Set<string>([tab.id]); + // If tab has children (is a group), delete them too if (tab.type === "group") { const childIds = data.tabs .filter((t) => t.parentId === tab.id) .map((t) => t.id); + + for (const childId of childIds) { + removedTabIds.add(childId); + } data.tabs = data.tabs.filter( (t) => t.id !== tab.id && !childIds.includes(t.id), ); // Kill terminals for children for (const childId of childIds) { terminalManager.kill({ tabId: childId }); } } else { // If tab is a child, remove from parent layout if (tab.parentId) { const parent = data.tabs.find((t) => t.id === tab.parentId); if (parent && parent.type === "group" && parent.layout) { parent.layout = removeTabFromLayout(parent.layout, tab.id); // If parent becomes empty, delete it if (!parent.layout) { + removedTabIds.add(parent.id); data.tabs = data.tabs.filter((t) => t.id !== parent.id); } } } data.tabs = data.tabs.filter((t) => t.id !== tab.id); // Kill terminal if terminal type if (tab.type === "terminal") { terminalManager.kill({ tabId: tab.id }); } } // Update active tab if needed const workspace = data.workspaces.find( (w) => w.id === tab.workspaceId, ); - if (workspace?.activeTabId === tab.id) { + if ( + workspace && + workspace.activeTabId && + removedTabIds.has(workspace.activeTabId) + ) { const remainingTabs = data.tabs .filter((t) => t.workspaceId === tab.workspaceId && !t.parentId) .sort((a, b) => a.position - b.position); workspace.activeTabId = remainingTabs[0]?.id; } }); return { success: true }; }),
245-286: Critical: StaleactiveTabIdwhen layout removes the active tabThe procedure computes
removedTabIds(line 260-262) and deletes those tabs (line 277), but never checks ifworkspace.activeTabIdis among the removed tabs. This causes two critical issues:
- Layout-driven child removal: When a tab is removed from the mosaic layout and deleted, if it was the active tab,
activeTabIdbecomes stale.- Group deletion (line 271-272): When
layoutisnull, the group is deleted, but ifworkspace.activeTabIdequals the group ID, it won't be updated.Update
activeTabIdwhen it's among the removed tabs:updateLayout: publicProcedure .input( z.object({ groupId: z.string(), layout: z.any(), // MosaicNode<string> | null }), ) .mutation(async ({ input }) => { const group = db.data.tabs.find((t) => t.id === input.groupId); if (!group || group.type !== "group") { throw new Error("Group not found"); } const oldTabIds = extractTabIdsFromLayout(group.layout || null); const newTabIds = extractTabIdsFromLayout(input.layout); const removedTabIds = Array.from(oldTabIds).filter( (id) => !newTabIds.has(id), ); + // If the group is being deleted (layout null), include it in removed IDs + if (!input.layout) { + removedTabIds.push(input.groupId); + } await db.update((data) => { const g = data.tabs.find((t) => t.id === input.groupId); if (g && g.type === "group") { g.layout = input.layout; g.updatedAt = Date.now(); // If layout is null, delete the group if (!input.layout) { data.tabs = data.tabs.filter((t) => t.id !== input.groupId); } } // Delete removed tabs data.tabs = data.tabs.filter((t) => !removedTabIds.includes(t.id)); + + // If the active tab was removed, select a new one + const workspace = data.workspaces.find( + (w) => w.id === group.workspaceId, + ); + if ( + workspace && + workspace.activeTabId && + removedTabIds.includes(workspace.activeTabId) + ) { + const remainingTabs = data.tabs + .filter( + (t) => t.workspaceId === group.workspaceId && !t.parentId, + ) + .sort((a, b) => a.position - b.position); + workspace.activeTabId = remainingTabs[0]?.id; + } }); // Kill terminals for removed tabs for (const tabId of removedTabIds) { terminalManager.kill({ tabId }); } return { success: true }; }),
🧹 Nitpick comments (3)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
155-221: Align delete‑time activation with setActive (timestamps + slight cleanup suggestion)The delete flow is robust overall: it tolerates worktree removal failures via
worktreeWarning, cleans up tabs, workspaces, worktrees, and activates an adjacent workspace when the deleted one was active. Two small improvements:
- When you activate
adjacentWorkspace, you don’t updatelastOpenedAt/updatedAtlikesetActivedoes, so the same “activate workspace” action has different side effects depending on the code path.deletedTabIdsis derived directly fromdb.data.tabsoutsidedb.update. Ifdb.updateis the only mutation entry point this is fine, but moving this derivation into thedb.updatecallback would make the delete more atomic and less sensitive to concurrent changes.A possible adjustment inside the
db.updateblock (focusing on the activation path) could be:- // Activate adjacent workspace if one was found - if (adjacentWorkspace) { - const ws = data.workspaces.find( - (w) => w.id === adjacentWorkspace.id, - ); - if (ws) { - ws.isActive = true; - } - } + // Activate adjacent workspace if one was found + if (adjacentWorkspace) { + const ws = data.workspaces.find( + (w) => w.id === adjacentWorkspace.id, + ); + if (ws) { + ws.isActive = true; + ws.lastOpenedAt = Date.now(); + ws.updatedAt = Date.now(); + } + }And, if you decide to tighten atomicity, you could also move the
deletedTabIdsderivation into this samedb.updateand return it from there.apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (2)
246-251: Replacez.any()with a proper schema for type safetyUsing
z.any()for the layout bypasses Zod's runtime validation, which defeats the purpose of using Zod for input validation. WhileMosaicNode<string>has a recursive structure that's complex to fully validate, you can at least validate the basic shape.Define a proper schema for the mosaic layout:
+// Add near the top of the file or in a shared schemas file +const mosaicNodeSchema: z.ZodType<MosaicNode<string> | null> = z.lazy(() => + z.union([ + z.string(), + z.object({ + direction: z.enum(["row", "column"]), + first: mosaicNodeSchema, + second: mosaicNodeSchema, + splitPercentage: z.number().optional(), + }), + z.null(), + ]) +); updateLayout: publicProcedure .input( z.object({ groupId: z.string(), - layout: z.any(), // MosaicNode<string> | null + layout: mosaicNodeSchema, }), )This provides runtime validation while maintaining type safety.
320-360: Consider handlingactiveTabIdedge case when ungroupingWhile less common, if
workspace.activeTabIdequals the group being ungrouped, it will reference a deleted tab after line 356. Most UIs would have the active tab be a child rather than the group itself, but for completeness, you could updateactiveTabIdto the first ungrouped child.Add
activeTabIdreassignment before deleting the group:// Recompute positions for all workspace tabs const allTabs = data.tabs .filter( (t) => t.workspaceId === group.workspaceId && !t.parentId, ) .filter((t) => t.id !== input.groupId) .sort((a, b) => a.position - b.position); for (let i = 0; i < allTabs.length; i++) { allTabs[i].position = i; } + // If the group was active, activate the first ungrouped child + const workspace = data.workspaces.find( + (w) => w.id === group.workspaceId, + ); + if (workspace?.activeTabId === input.groupId && children.length > 0) { + workspace.activeTabId = children[0].id; + } + // Delete group data.tabs = data.tabs.filter((t) => t.id !== input.groupId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/desktop/src/lib/trpc/routers/notifications/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/notifications/notifications.ts(1 hunks)apps/desktop/src/lib/trpc/routers/tabs/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/tabs/tabs.ts(1 hunks)apps/desktop/src/lib/trpc/routers/tabs/utils/layout/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/tabs/utils/layout/layout.ts(1 hunks)apps/desktop/src/lib/trpc/routers/terminal/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/terminal/terminal.ts(1 hunks)apps/desktop/src/lib/trpc/routers/window/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/utils.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts(7 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(6 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/lib/trpc/routers/tabs/index.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/utils.ts
🧰 Additional context used
🧬 Code graph analysis (5)
apps/desktop/src/lib/trpc/routers/notifications/notifications.ts (1)
apps/desktop/src/lib/trpc/routers/notifications/index.ts (2)
NotificationsRouter(1-1)createNotificationsRouter(2-2)
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
apps/desktop/src/lib/trpc/routers/terminal/index.ts (2)
TerminalRouter(1-1)createTerminalRouter(2-2)
apps/desktop/src/lib/trpc/routers/tabs/utils/layout/layout.ts (1)
apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (4)
apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)apps/desktop/src/lib/trpc/routers/workspaces/utils/git/git.ts (1)
removeWorktree(68-81)apps/desktop/src/lib/trpc/routers/workspaces/utils/utils.ts (1)
findAdjacentWorkspace(12-35)apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(399-399)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (4)
apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)apps/desktop/src/main/lib/db/schemas.ts (1)
Tab(48-50)apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(399-399)apps/desktop/src/lib/trpc/routers/tabs/utils/layout/layout.ts (2)
removeTabFromLayout(27-49)extractTabIdsFromLayout(6-22)
🔇 Additional comments (26)
apps/desktop/src/lib/trpc/routers/window/index.ts (1)
1-2: Re-exports look correct and match the router barrel patternType-only export for
WindowRouterand value export forcreateWindowRouterare idiomatic and align with centralizing TRPC router access; no changes needed.apps/desktop/src/lib/trpc/routers/notifications/notifications.ts (1)
39-39: NotificationsRouter type alias looks correct and idiomaticUsing
ReturnType<typeof createNotificationsRouter>to expose the router type is a clean pattern and matches typical TRPC usage. No issues from a typing or runtime perspective.apps/desktop/src/lib/trpc/routers/notifications/index.ts (1)
1-2: Barrel re-exports are consistent and correctRe‑exporting both
NotificationsRouter(type) andcreateNotificationsRouter(factory) from./notificationscleanly exposes the notifications router API and aligns with typical router index patterns.apps/desktop/src/lib/trpc/routers/terminal/index.ts (1)
1-1: LGTM! Type export follows standard barrel pattern.The type re-export is correctly structured and consistent with the value export pattern.
apps/desktop/src/lib/trpc/routers/terminal/terminal.ts (1)
163-163: LGTM! Type alias correctly exposes router type.The
ReturnTypeutility type is properly used to extract the router's type signature, enabling type-safe consumption.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (4)
3-14: New terminal and workspace utils wiring looks appropriateImporting
terminalManagerandfindAdjacentWorkspaceand using them in the delete flow is coherent with the responsibilities of this router (terminal lifecycle + next-active workspace resolution). No issues from a routing/API perspective.
52-60: Active workspace invariants on create look consistent; confirm global vs per‑project semanticsSetting
isActive: trueon the new workspace and clearingisActiveon all existing workspaces inside thedb.updateensures at most one active workspace at a time. This matches the newgetActivebehavior and keeps the invariant simple.One question to verify: this currently makes the “active workspace” global across all projects, not per‑project. If the UX expects per‑project active workspaces, you’d want to restrict the deactivation loop to
ws.projectId === input.projectId.Also applies to: 66-88
107-110: getActive matches new isActive modelSwitching
getActivetofind((w) => w.isActive)and returningnullwhen none are active aligns with the newisActiveflag and simplifies callers compared to a separatelastActiveWorkspaceId.
214-217: Terminal cleanup on deleted tabs is a good additionIterating over
deletedTabIdsand callingterminalManager.kill({ tabId })ensures terminals don’t leak when a workspace is deleted. That’s the right place to couple terminal lifecycle to tab lifetime.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (3)
14-15: LGTM! Mock data aligns with new workspace schema.The addition of
activeTabId,isActive, and thetabsarray correctly reflects the refactored workspace state management mentioned in the PR objectives.Also applies to: 41-41
67-72: LGTM! Terminal manager mock properly isolates test environment.The mock correctly stubs out the terminal lifecycle interactions needed during workspace deletion tests.
88-89: LGTM! Test setup properly resets new workspace fields.The
beforeEachblock correctly initializes the newactiveTabId,isActive, andtabsfields to ensure clean test state.Also applies to: 115-115
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (2)
28-28: LGTM! Error handling is well-implemented.The error state is properly initialized, cleared before operations, safely extracted from caught errors with a fallback message, and displayed to users with appropriate styling.
Also applies to: 33-33, 43-45, 63-65
58-62: Backend properly handles all edge cases for workspace deletion.Verification of the delete mutation in
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(lines 139-212) confirms:
- Non-existent workspace: Returns error "Workspace not found" (line 144-146)
- Active workspace deletion: Finds and activates an adjacent workspace before deletion (lines 173-176, 198-210)
- Last workspace deletion: Allowed, system can have no active workspace
The UI simplification is safe because the backend mutation properly validates these scenarios. Git worktree removal failures are gracefully handled with warnings, allowing the workspace to be cleaned up from the database even if the worktree removal fails.
apps/desktop/src/lib/trpc/routers/tabs/utils/layout/index.ts (1)
1-1: LGTM!Standard barrel export pattern for exposing layout utilities through a centralized public API surface.
apps/desktop/src/lib/trpc/routers/tabs/utils/layout/layout.ts (2)
6-22: LGTM!The recursive traversal correctly handles all three cases: null layouts, leaf nodes (strings), and internal nodes. The merging logic properly aggregates tab IDs from both subtrees.
27-49: LGTM!The recursive removal and tree collapsing logic is correct. The function properly:
- Removes matching leaf nodes
- Recursively prunes both subtrees
- Collapses parent nodes when one branch becomes null
- Preserves layout properties (direction, splitPercentage) using the spread operator
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (9)
1-14: LGTM!Imports are well-organized and the router factory pattern allows for proper dependency injection and testing.
16-22: LGTM!Simple query that correctly filters and sorts tabs for a workspace.
24-36: LGTM!Properly handles the case where a workspace has no active tab or the active tab ID is stale (returns null in both cases).
39-78: LGTM!The position calculation correctly handles both empty workspaces and existing tabs, and the new tab is appropriately set as active.
140-168: LGTM!The patch operation correctly handles optional fields and maintains the
updatedAttimestamp.
170-202: LGTM!The activation logic correctly ensures only one workspace is active at a time and properly maintains the
lastOpenedAttimestamp and attention flags.
204-242: LGTM!The reordering logic correctly uses splice to move the tab and updates all position indices to maintain consistency.
288-318: LGTM!The child tab creation correctly sets the
parentIdand defers layout management to the frontend via a subsequentupdateLayoutcall.
364-364: LGTM!The exported type alias enables proper typing for consumers of the tabs router.
| if (result.warning) { | ||
| // Show warning to user but still close dialog | ||
| console.warn("Workspace deleted with warning:", result.warning); | ||
| // TODO: Show toast notification with warning | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Complete the warning notification implementation.
The TODO comment indicates that warnings (e.g., failed worktree removal) are not being communicated to users. Currently, warnings are only logged to the console, and the dialog closes immediately, leaving users unaware of potential issues like orphaned worktrees on disk.
Consider implementing a toast notification system to display warnings before closing the dialog. Would you like me to open an issue to track this work, or would you prefer to implement it in this PR?
Example implementation pattern:
if (result.warning) {
toast.warning("Workspace deleted", {
description: result.warning,
});
}
onOpenChange(false);🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx
around lines 36 to 40, replace the console.warn + TODO with a user-facing toast
so warnings are visible: import and use the project's toast utility (e.g.,
toast.warning) to display a concise title and the result.warning as the
description, call the toast before closing the dialog, and then call
onOpenChange(false); ensure the toast import is added at the top of the file and
that the toast API matches the project's pattern.
saddlepaddle
force-pushed
the
set-up-worktree-setup-scripts
branch
from
115a42b to
9921f96
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (1)
36-40: TODO comment for toast notification still present.The warning notification implementation remains incomplete, as noted in the previous review.
🧹 Nitpick comments (8)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (1)
28-28: Add cleanup to reset error state when dialog opens.The error state is not cleared when the dialog is reopened after closing. If a user encounters an error, closes the dialog, and reopens it later, the stale error message will still be displayed.
Add a
useEffectto reset the error when the dialog opens:const [error, setError] = useState<string | null>(null); const deleteWorkspace = useDeleteWorkspace(); + + useEffect(() => { + if (open) { + setError(null); + } + }, [open]);Import
useEffectat the top:-import { useState } from "react"; +import { useEffect, useState } from "react";apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useGroupDrop.ts (1)
14-21: Track the TODO: Drag-to-group functionality not yet implemented.The actual drag-to-group mutation has been replaced with a console.log placeholder as part of the TRPC migration.
Do you want me to generate an implementation or open a new issue to track this task?
apps/desktop/src/main/lib/db/schemas.ts (1)
34-50: Consider moving type-specific fields to discriminated branches.The discriminated union approach is good for type safety. However,
layoutinBaseTabis semantically specific to group tabs. Consider restructuring to:-export interface BaseTab { +interface SharedTabFields { id: string; workspaceId: string; title: string; position: number; parentId?: string; - layout?: MosaicNode<string>; needsAttention?: boolean; createdAt: number; updatedAt: number; } export type Tab = - | (BaseTab & { type: "terminal" }) - | (BaseTab & { type: "group" }); + | (SharedTabFields & { type: "terminal" }) + | (SharedTabFields & { type: "group"; layout?: MosaicNode<string> });This ensures terminal tabs don't have a
layoutfield at the type level.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx (2)
20-43: Extract cleanLayout to a shared utility.The
cleanLayoutfunction duplicates logic fromapps/desktop/src/renderer/stores/tabs/drag-logic.ts(lines 47-74). Consider extracting this to a shared utility module to avoid duplication and ensure consistency.Create a new file (e.g.,
apps/desktop/src/lib/trpc/routers/tabs/utils/layout/cleanLayout.ts):import type { MosaicNode } from "react-mosaic-component"; export function cleanLayout( layout: MosaicNode<string> | null | undefined, validTabIds: Set<string>, ): MosaicNode<string> | null { if (!layout) return null; if (typeof layout === "string") { return validTabIds.has(layout) ? layout : null; } const cleanedFirst = cleanLayout(layout.first, validTabIds); const cleanedSecond = cleanLayout(layout.second, validTabIds); if (!cleanedFirst && !cleanedSecond) return null; if (!cleanedFirst) return cleanedSecond; if (!cleanedSecond) return cleanedFirst; return { ...layout, first: cleanedFirst, second: cleanedSecond, }; }Then import and use it in both locations.
72-90: Track TODOs: Split and remove handlers are incomplete.The handlers for
splitHorizontal,splitVertical, andremoveChildare placeholder implementations with console.log statements. These need to be implemented to support the full group tab functionality.Do you want me to generate implementations for these handlers or open new issues to track these tasks?
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
13-36: Active workspace derivation and keyboard navigation look correct; consider tightening dependencies and zero-length handling
- Using
projects.getAllWithWorkspaces+allWorkspaces.find(w => w.isActive)and navigation viafindIndexis consistent and avoids a separate “active id” source of truth.updateWorkspaceWidthcurrently divides byallWorkspaces.length; with 0 workspaces this becomesInfinitybut is safely clamped byMath.min. If you want to avoid relying on that, you could guard withif (!allWorkspaces.length) return;or treat length asMath.max(1, allWorkspaces.length)before dividing.useEffectand hotkey hooks depend onallWorkspaces, which is a new array each render. If this becomes a hot path, memoizingallWorkspacesor depending onallWorkspaces.length(for resize) could reduce re-subscribing, but it’s not critical given expected sizes.Also applies to: 60-85
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
139-167: Workspace deletion flow is solid; consider guarding terminal cleanup ifkillcan throw
- Collecting
deletedTabIdsbefore the mutation, deleting all tabs/workspaces/worktrees in a singledb.update, and then killing terminals pertabIdis a good separation of data and side effects.- Using
findAdjacentWorkspaceonly when the deleted workspace was active, and then re-activating that neighbor inside the same update, preserves “some workspace stays active” without extra queries.- You already treat
removeWorktreefailures as non-fatal and surface a warning. IfterminalManager.killcan throw, a single bad tab could cause the whole mutation to fail after the DB is updated. If that’s a concern, wrapping thefor (const tabId of deletedTabIds)loop in a try/catch (logging but not rethrowing) would make deletion more resilient.Also applies to: 168-221
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (1)
1-8: Tab-driven terminal view and remove mutation look good; optional cleanups
- Switching
SingleTabViewto accepttab: Tab & { type: "terminal" }and passing the whole tab object into<Terminal tab={tab} />matches the new tabs router shape and Terminal API.- Using
useRemoveTab().mutate({ id: tab.id })forhandleClosePaneis consistent with the new server-driven tabs lifecycle and react-query cache invalidation.Two small optional follow-ups:
isDropZoneis inSingleTabViewPropsbut unused in the component body; if it’s no longer needed, consider removing it or wiring it into the UI.- The split handlers are currently TODOs with
console.log; if this ships to users, you may want either to hide these actions until implemented or add a clear TODO comment where they’re triggered.Also applies to: 11-25, 34-35
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
apps/desktop/src/lib/trpc/routers/index.ts(2 hunks)apps/desktop/src/lib/trpc/routers/notifications/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/notifications/notifications.ts(1 hunks)apps/desktop/src/lib/trpc/routers/projects/projects.ts(2 hunks)apps/desktop/src/lib/trpc/routers/projects/utils/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/projects/utils/utils.ts(1 hunks)apps/desktop/src/lib/trpc/routers/tabs/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/tabs/tabs.test.ts(1 hunks)apps/desktop/src/lib/trpc/routers/tabs/tabs.ts(1 hunks)apps/desktop/src/lib/trpc/routers/tabs/utils/layout/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/tabs/utils/layout/layout.ts(1 hunks)apps/desktop/src/lib/trpc/routers/terminal/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/terminal/terminal.ts(1 hunks)apps/desktop/src/lib/trpc/routers/window/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/git/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/index.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/utils/utils.ts(1 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts(7 hunks)apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts(6 hunks)apps/desktop/src/main/lib/db/schemas.ts(2 hunks)apps/desktop/src/renderer/react-query/projects/useReorderProjects.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/index.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/useCreateTab.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/useRemoveTab.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/useSetActiveTab.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/useUpdateLayout.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/useUpdateTab.ts(1 hunks)apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsx(4 hunks)apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useTabContentDrop.ts(2 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/TabItem/useGroupDrop.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (18)
- apps/desktop/src/lib/trpc/routers/terminal/index.ts
- apps/desktop/src/renderer/react-query/tabs/useCreateTab.ts
- apps/desktop/src/lib/trpc/routers/notifications/index.ts
- apps/desktop/src/lib/trpc/routers/workspaces/utils/index.ts
- apps/desktop/src/renderer/react-query/tabs/useRemoveTab.ts
- apps/desktop/src/renderer/react-query/workspaces/useReorderWorkspaces.ts
- apps/desktop/src/lib/trpc/routers/terminal/terminal.ts
- apps/desktop/src/lib/trpc/routers/notifications/notifications.ts
- apps/desktop/src/renderer/react-query/tabs/useSetActiveTab.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx
- apps/desktop/src/renderer/react-query/tabs/index.ts
- apps/desktop/src/lib/trpc/routers/tabs/index.ts
- apps/desktop/src/lib/trpc/routers/index.ts
- apps/desktop/src/lib/trpc/routers/projects/utils/index.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx
- apps/desktop/src/renderer/react-query/tabs/useUpdateLayout.ts
🧰 Additional context used
🧬 Code graph analysis (16)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (2)
apps/desktop/src/lib/trpc/routers/projects/utils/utils.ts (1)
getAllWithWorkspaces(6-24)apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts (1)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (1)
Tab(387-387)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (1)
packages/ui/src/components/alert-dialog.tsx (5)
AlertDialog(144-144)AlertDialogContent(148-148)AlertDialogHeader(149-149)AlertDialogTitle(151-151)AlertDialogDescription(152-152)
apps/desktop/src/lib/trpc/routers/workspaces/utils/utils.ts (2)
apps/desktop/src/main/lib/db/schemas.ts (2)
Project(3-11)Workspace(21-32)apps/desktop/src/lib/trpc/routers/projects/utils/utils.ts (1)
getAllWithWorkspaces(6-24)
apps/desktop/src/renderer/react-query/tabs/useUpdateTab.ts (1)
apps/desktop/src/renderer/react-query/tabs/index.ts (1)
useUpdateTab(3-3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx (3)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (1)
Tab(387-387)apps/desktop/src/renderer/react-query/tabs/useSetActiveTab.ts (1)
useSetActiveTab(7-23)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
Terminal(14-201)
apps/desktop/src/lib/trpc/routers/tabs/utils/layout/layout.ts (1)
apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx (4)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (1)
Tab(387-387)apps/desktop/src/renderer/stores/tabs/drag-logic.ts (1)
cleanLayout(48-75)apps/desktop/src/renderer/react-query/tabs/useUpdateLayout.ts (1)
useUpdateLayout(7-24)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx (1)
GroupTabPane(20-68)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (4)
apps/desktop/src/lib/trpc/index.ts (1)
router(15-15)apps/desktop/src/main/lib/db/schemas.ts (1)
Tab(48-50)apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(398-398)apps/desktop/src/lib/trpc/routers/tabs/utils/layout/layout.ts (2)
removeTabFromLayout(27-49)extractTabIdsFromLayout(6-22)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (4)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (1)
Tab(387-387)apps/desktop/src/renderer/react-query/tabs/useRemoveTab.ts (1)
useRemoveTab(7-23)apps/desktop/src/renderer/stores/tabs/hooks.ts (1)
useRemoveTab(8-8)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
Terminal(14-201)
apps/desktop/src/main/lib/db/schemas.ts (2)
apps/desktop/src/shared/types.ts (4)
MosaicNode(35-35)Tab(44-56)Worktree(58-68)Workspace(70-82)apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (1)
Tab(387-387)
apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (4)
apps/desktop/src/main/lib/db/index.ts (1)
db(19-26)apps/desktop/src/lib/trpc/routers/workspaces/utils/git/git.ts (1)
removeWorktree(68-81)apps/desktop/src/lib/trpc/routers/workspaces/utils/utils.ts (1)
findAdjacentWorkspace(12-35)apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(398-398)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsx (1)
apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
ProjectWithWorkspaces(124-124)
apps/desktop/src/lib/trpc/routers/tabs/tabs.test.ts (2)
apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (1)
createTabsRouter(10-382)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsx (1)
WorkspaceGroup(15-73)
apps/desktop/src/lib/trpc/routers/projects/utils/utils.ts (1)
apps/desktop/src/main/lib/db/schemas.ts (2)
Project(3-11)Workspace(21-32)
🔇 Additional comments (27)
apps/desktop/src/lib/trpc/routers/window/index.ts (1)
1-2: LGTM! Clean barrel file implementation.The barrel file correctly re-exports the
WindowRoutertype andcreateWindowRouterfactory, both of which exist in the target module (window.ts). This aligns with the architectural pattern applied to other router modules in this PR.apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/DeleteWorkspaceDialog.tsx (3)
31-50: Error handling logic is well-structured.The delete operation properly:
- Clears previous errors before attempting deletion
- Awaits the async operation
- Extracts meaningful error messages for both Error instances and other error types
- Ensures cleanup in the finally block
58-65: LGTM! Clear user messaging and error display.The dialog description clearly communicates the deletion consequences, and the inline error display provides immediate feedback to users when deletion fails.
75-75: LGTM! Simplified disable logic is correct.The button correctly disables only during active deletion, preventing concurrent operations while keeping the logic simple.
apps/desktop/src/lib/trpc/routers/workspaces/utils/git/index.ts (1)
1-1: LGTM!Standard barrel export pattern for the git utilities module.
apps/desktop/src/lib/trpc/routers/tabs/utils/layout/index.ts (1)
1-1: LGTM!Standard barrel export pattern for layout utilities.
apps/desktop/src/lib/trpc/routers/projects/utils/utils.ts (1)
6-24: LGTM!The
getAllWithWorkspacesutility correctly filters projects with non-nulltabOrder, sorts them, and attaches their associated workspaces. The non-null assertion on line 12 is safe because of the filter on line 11.apps/desktop/src/main/lib/db/schemas.ts (3)
1-1: LGTM!MosaicNode import added to support the new tab layout structure.
27-28: LGTM!The addition of
activeTabIdandisActiveto the Workspace interface aligns with the new tab management approach.
56-63: LGTM!Database schema correctly updated to use the new
tabscollection instead ofsettings.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx (2)
57-70: LGTM!The
handleLayoutChangecallback correctly delegates all layout management to the backend mutation, which handles orphaned tabs, terminal cleanup, and activeTabId updates.
92-115: LGTM!The
renderPanefunction correctly handles the backend-driven state, checks for terminal tabs, and provides appropriate fallback UI for missing or invalid tabs.apps/desktop/src/lib/trpc/routers/tabs/tabs.test.ts (1)
1-652: LGTM! Excellent test coverage.The test suite comprehensively covers the tabs router functionality:
- Tab creation (terminal and group) with positioning
- Removal with cascade effects (children, active tab fallback, empty parent deletion)
- Updates (title, needsAttention)
- Active tab management (workspace activation, needsAttention clearing)
- Reordering
- Layout management (updates, orphan cleanup, group deletion)
- Group operations (addChildTab, ungroup)
- Query operations
The tests are well-structured, use proper mocking, and cover edge cases thoroughly.
apps/desktop/src/renderer/react-query/projects/useReorderProjects.ts (1)
8-8: LGTM!The query invalidation correctly updated to use
projects.getAllWithWorkspacesinstead ofworkspaces.getAllGrouped, aligning with the new data structure introduced in this PR.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
11-11: LGTM! Clean aggregation pattern.The new
getAllWithWorkspacesprocedure and corresponding type export provide a type-safe way to fetch projects with their nested workspaces. The implementation correctly delegates to the utility function and exposes the aggregated data through TRPC.Also applies to: 116-118, 124-124
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/types.ts (1)
1-5: LGTM! Improved type safety with discriminated union.Consolidating
tabIdandworkspaceIdinto a singletabobject with a discriminated union type (Tab & { type: "terminal" }) improves type safety and reduces prop drilling. This aligns well with the TRPC-based data flow.apps/desktop/src/renderer/react-query/tabs/useUpdateTab.ts (1)
1-22: LGTM! Follows established mutation hook pattern.The implementation correctly wraps the TRPC mutation, invalidates tabs queries on success, and properly chains the user's
onSuccesscallback. This follows the same pattern as other mutation hooks in the codebase (e.g.,useCreateTab,useRemoveTab).apps/desktop/src/lib/trpc/routers/workspaces/workspaces.test.ts (2)
14-15: LGTM! Tests updated to match new workspace schema.The test updates correctly reflect the new workspace state model with
isActiveandactiveTabIdfields, add necessary terminal manager mocking, and initialize the tabs collection. These changes align well with the broader architectural shift to TRPC-based state management.Also applies to: 41-41, 67-72, 88-89, 115-115
133-150: Verify the graceful degradation behavior is intentional.The test now expects workspace deletion to succeed with a warning when worktree removal fails, rather than failing entirely. This is a behavior change that provides better UX by not blocking deletion on filesystem errors.
Please confirm this graceful degradation is the intended behavior. The change allows the workspace to be removed from the database even if the git worktree can't be cleaned up from the filesystem, which seems reasonable for handling edge cases like permission issues or missing directories.
apps/desktop/src/lib/trpc/routers/workspaces/utils/utils.ts (1)
1-35: LGTM! Correct adjacent workspace selection logic.The
findAdjacentWorkspacefunction correctly:
- Builds an ordered list of workspaces using
getAllWithWorkspaces- Prefers the previous workspace (left) before trying the next (right)
- Handles all edge cases: workspace not found, empty list, and single-workspace scenarios
The logic ensures a sensible fallback when a workspace is deleted.
apps/desktop/src/lib/trpc/routers/tabs/utils/layout/layout.ts (2)
6-22: LGTM! Correct recursive traversal for ID extraction.The
extractTabIdsFromLayoutfunction correctly traverses the mosaic tree and collects all tab IDs into a Set. The recursive approach properly handles both leaf nodes (strings) and parent nodes with first/second branches.
27-49: LGTM! Proper tree pruning and collapsing logic.The
removeTabFromLayoutfunction correctly:
- Removes the target tab ID from leaf nodes
- Recursively processes parent nodes
- Collapses branches when one child becomes null
- Returns null when both children are removed
This ensures the layout tree remains valid after tab removal.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx (2)
4-5: LGTM! Consistent refactor to TRPC-based tab management.The component correctly:
- Consolidates
tabIdandworkspaceIdinto a singlechildTabobject- Replaces prop-based
setActiveTabwith theuseSetActiveTabmutation hook- Uses
childTab.idconsistently throughout for tab identificationThis aligns well with the broader migration to TRPC-based state management.
Also applies to: 12-12, 29-33
15-16: LGTM! Proper integration with split/close operations.The split and close operations correctly:
- Pass
childTab.idas the source tab for split operations- Include the
pathparameter for mosaic positioning- Pass the full
tabobject to the Terminal componentAll callback signatures and usages are consistent with the updated TRPC-based architecture.
Also applies to: 58-60, 63-63
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/index.tsx (1)
93-104: Project-based grouping and separator rendering are consistent with the new data shapeMapping
groupsasprojectand passing the fullprojectobject intoWorkspaceGroup(plus usingprojectIndexfor separators) keeps the tabs UI aligned withProjectWithWorkspaces. Keys and separator conditions (projectIndex < groups.length - 1) look correct.apps/desktop/src/lib/trpc/routers/workspaces/workspaces.ts (1)
52-64: NewisActive/activeTabIdmodel for workspaces is coherent and matches router usage
- Creating a workspace with
isActive: trueand clearingisActiveon all existing workspaces insidedb.updateensures there’s at most one active workspace at a time.getActiveswitching tofind((w) => w.isActive)is compatible with the tabs router’ssetActivelogic and decouples you from any previous “last active id” fields.setActivedeactivates all workspaces then activates the target and bumps timestamps, which is consistent with a single globally active workspace.No correctness issues spotted here; behavior just relies on the invariant that only one workspace should be active at any time, which the mutations now enforce.
Also applies to: 65-88, 107-110, 224-245
apps/desktop/src/renderer/screens/main/components/TopBar/WorkspaceTabs/WorkspaceGroup.tsx (1)
3-9: ProjectWithWorkspaces integration and isActive-driven rendering are consistent
- Taking a full
project: ProjectWithWorkspacesprop and derivingprojectId,projectName, andprojectColorfrom it keeps this component aligned with the newgetAllWithWorkspacesshape.- Collapsed mode showing only
project.workspaces.filter((w) => w.isActive)and expanded mode showing allproject.workspacesmatches the intended “show only the active workspace when collapsed” behavior.- Passing
workspace.isActiveintoWorkspaceItemremoves the need for an external active workspace id and ties directly into theisActivefield maintained by the workspaces/tabs routers.No issues spotted here.
Also applies to: 15-21, 28-31, 40-47, 58-62
| import type { MosaicNode } from "react-mosaic-component"; | ||
| import { db } from "main/lib/db"; | ||
| import type { Tab } from "main/lib/db/schemas"; | ||
| import { terminalManager } from "main/lib/terminal-manager"; | ||
| import { nanoid } from "nanoid"; | ||
| import { z } from "zod"; | ||
| import { publicProcedure, router } from "../.."; | ||
| import { extractTabIdsFromLayout, removeTabFromLayout } from "./utils/layout"; |
There was a problem hiding this comment.
🧩 Analysis chain
Resolve Tab type name collision between DB schema import and exported router type
You currently:
- Import
type { Tab }from"main/lib/db/schemas", and - Later export
type Tab = TabsRouter["getByWorkspace"]["_def"]["_output_out"][number];
This reuses the same identifier Tab for two different type meanings in the same module, which is confusing and likely to cause type errors or at least IDE ambiguity. It also makes it harder to distinguish “DB shape” vs “public TRPC output” in this file.
A straightforward fix is to alias the imported DB type and keep the exported Tab name for the public TRPC type:
-import type { Tab } from "main/lib/db/schemas";
+import type { Tab as DbTab } from "main/lib/db/schemas";
@@
- const newTab: Tab = {
+ const newTab: DbTab = {
@@
- const newTab: Tab = {
+ const newTab: DbTab = {The bottom export can remain:
export type TabsRouter = ReturnType<typeof createTabsRouter>;
// Export Tab type for use in components
export type Tab = TabsRouter["getByWorkspace"]["_def"]["_output_out"][number];This keeps a clear distinction between the internal DB model and the router’s inferred output type.
Also applies to: 50-58, 321-330, 384-387
I'll verify the Tab type collision claim by examining the actual file content.
🏁 Script executed:
#!/bin/bash
# Read the tabs router file to verify the type collision
find . -path "*/apps/desktop/src/lib/trpc/routers/tabs/tabs.ts" -type f -exec cat {} \;Length of output: 10636
🏁 Script executed:
#!/bin/bash
# Search for all Tab type references to understand usage patterns
rg -n "type Tab" apps/desktop/src/lib/trpc/routers/tabs/tabs.tsLength of output: 148
🏁 Script executed:
#!/bin/bash
# Get context around the mentioned line ranges
rg -n . apps/desktop/src/lib/trpc/routers/tabs/tabs.ts | head -70Length of output: 2403
🏁 Script executed:
#!/bin/bash
# Check if there are other files in the same directory
fd . "apps/desktop/src/lib/trpc/routers/tabs/" -type fLength of output: 170
🏁 Script executed:
#!/bin/bash
# Look for where Tab type is actually imported
rg -n "from.*db/schemas" apps/desktop/src/lib/trpc/routers/tabs/tabs.tsLength of output: 49
🏁 Script executed:
#!/bin/bash
# Search for Tab type exports
rg -n "export.*type Tab" apps/desktop/src/lib/trpc/routers/tabs/tabs.tsLength of output: 148
🏁 Script executed:
#!/bin/bash
# Check usages of Tab at the specific line ranges
sed -n '1,60p;50,60p;321,330p;384,387p' apps/desktop/src/lib/trpc/routers/tabs/tabs.ts 2>/dev/null || echo "File reading with sed"Length of output: 2559
🏁 Script executed:
#!/bin/bash
# Get complete file size and structure
wc -l apps/desktop/src/lib/trpc/routers/tabs/tabs.tsLength of output: 50
🏁 Script executed:
#!/bin/bash
# Find all Tab references with context
rg -n -B2 -A2 ":\s*Tab(?:\s|[,\]<}])" apps/desktop/src/lib/trpc/routers/tabs/tabs.tsLength of output: 221
🏁 Script executed:
#!/bin/bash
# Show the entire tabs.ts file to analyze the structure
cat apps/desktop/src/lib/trpc/routers/tabs/tabs.tsLength of output: 10636
Alias imported DB Tab type to DbTab to resolve naming collision
The code imports type { Tab } from the DB schemas at line 3, then re-exports a different Tab type derived from the TRPC router output at line 387. This creates a naming collision where the same identifier refers to different types depending on scope—DB schema inside the router, TRPC output for external consumers.
Update the import and usages:
-import type { Tab } from "main/lib/db/schemas";
+import type { Tab as DbTab } from "main/lib/db/schemas";
@@
- const newTab: Tab = {
+ const newTab: DbTab = {
@@
- const newTab: Tab = {
+ const newTab: DbTab = {Lines: 3, 50, 321
🤖 Prompt for AI Agents
In apps/desktop/src/lib/trpc/routers/tabs/tabs.ts lines 1–8 (and usages at lines
50 and 321), the DB schema type Tab conflicts with the router-exported Tab type;
change the import to alias the DB type (import type { Tab as DbTab } from
"main/lib/db/schemas") and update all local usages that refer to the database
schema (at the mentioned lines and any other occurrences) to use DbTab so the
router’s exported Tab name remains unambiguous for external consumers.
| remove: publicProcedure | ||
| .input(z.object({ id: z.string() })) | ||
| .mutation(async ({ input }) => { | ||
| const tab = db.data.tabs.find((t) => t.id === input.id); | ||
| if (!tab) { | ||
| return { success: false, error: "Tab not found" }; | ||
| } | ||
|
|
||
| await db.update((data) => { | ||
| // Collect all removed tab IDs | ||
| const removedTabIds: string[] = []; | ||
|
|
||
| // If tab has children (is a group), delete them too | ||
| if (tab.type === "group") { | ||
| const childIds = data.tabs | ||
| .filter((t) => t.parentId === tab.id) | ||
| .map((t) => t.id); | ||
|
|
||
| removedTabIds.push(tab.id, ...childIds); | ||
|
|
||
| data.tabs = data.tabs.filter( | ||
| (t) => t.id !== tab.id && !childIds.includes(t.id), | ||
| ); | ||
|
|
||
| // Kill terminals for children | ||
| for (const childId of childIds) { | ||
| terminalManager.kill({ tabId: childId }); | ||
| } | ||
| } else { | ||
| // If tab is a child, remove from parent layout | ||
| if (tab.parentId) { | ||
| const parent = data.tabs.find((t) => t.id === tab.parentId); | ||
| if (parent && parent.type === "group" && parent.layout) { | ||
| parent.layout = removeTabFromLayout(parent.layout, tab.id); | ||
|
|
||
| // If parent becomes empty, delete it | ||
| if (!parent.layout) { | ||
| removedTabIds.push(parent.id); | ||
| data.tabs = data.tabs.filter((t) => t.id !== parent.id); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| removedTabIds.push(tab.id); | ||
| data.tabs = data.tabs.filter((t) => t.id !== tab.id); | ||
|
|
||
| // Kill terminal if terminal type | ||
| if (tab.type === "terminal") { | ||
| terminalManager.kill({ tabId: tab.id }); | ||
| } | ||
| } | ||
|
|
||
| // Update active tab if needed | ||
| const workspace = data.workspaces.find( | ||
| (w) => w.id === tab.workspaceId, | ||
| ); | ||
| if ( | ||
| workspace?.activeTabId && | ||
| removedTabIds.includes(workspace.activeTabId) | ||
| ) { | ||
| const remainingTabs = data.tabs | ||
| .filter((t) => t.workspaceId === tab.workspaceId && !t.parentId) | ||
| .sort((a, b) => a.position - b.position); | ||
| workspace.activeTabId = remainingTabs[0]?.id; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Good handling of removed tab IDs; adjust ungroup to keep workspace.activeTabId in sync
- The
removeandupdateLayoutmutations now correctly:- Track all deleted IDs (group + children + layout removals) via
removedTabIds, and - Reset
workspace.activeTabIdwhen it points at any of those IDs, choosing the first remaining top‑level tab byposition.
- Track all deleted IDs (group + children + layout removals) via
- In
ungroup, you:- Reparent children to top level, assign positions, recompute positions for all top‑level tabs, and then delete the group.
However, ungroup never updates workspace.activeTabId. If the group being ungrouped was the active tab, you’ll end up with workspace.activeTabId still pointing at the deleted groupId, and subsequent getActive calls will return null even though the workspace still has tabs.
You can mirror the pattern used in remove/updateLayout by updating the active tab after deleting the group:
- // Recompute positions for all workspace tabs
- const allTabs = data.tabs
- .filter((t) => t.workspaceId === group.workspaceId && !t.parentId)
- .filter((t) => t.id !== input.groupId)
- .sort((a, b) => a.position - b.position);
-
- for (let i = 0; i < allTabs.length; i++) {
- allTabs[i].position = i;
- }
-
- // Delete group
- data.tabs = data.tabs.filter((t) => t.id !== input.groupId);
+ // Recompute positions for all workspace tabs (excluding the group)
+ const allTabs = data.tabs
+ .filter((t) => t.workspaceId === group.workspaceId && !t.parentId)
+ .filter((t) => t.id !== input.groupId)
+ .sort((a, b) => a.position - b.position);
+
+ for (let i = 0; i < allTabs.length; i++) {
+ allTabs[i].position = i;
+ }
+
+ // Delete group
+ data.tabs = data.tabs.filter((t) => t.id !== input.groupId);
+
+ // If this group was the active tab, move focus to the first remaining top-level tab
+ const workspace = data.workspaces.find(
+ (w) => w.id === group.workspaceId,
+ );
+ if (workspace?.activeTabId === input.groupId) {
+ const remainingTabs = data.tabs
+ .filter(
+ (t) =>
+ t.workspaceId === group.workspaceId && !t.parentId,
+ )
+ .sort((a, b) => a.position - b.position);
+ workspace.activeTabId = remainingTabs[0]?.id;
+ }This keeps workspace.activeTabId consistent across all tab removal paths, including ungrouping.
Also applies to: 249-305, 340-380
| // TODO: Implement drag-to-tab logic with tRPC mutations | ||
| // This should handle: | ||
| // 1. Dragging tab into itself - creates new tab and makes a group | ||
| // 2. Dragging into child tab - redirects to parent group | ||
| // 3. Dragging into group tab - adds to existing split view | ||
| // 4. Dragging single tab into another single tab - creates new group container | ||
| console.log("Drag tab to tab not yet implemented", { | ||
| draggedTabId: item.tabId, | ||
| targetTabId: tabToRender.id, | ||
| }); |
There was a problem hiding this comment.
Incomplete: Drag-to-tab logic needs implementation.
The drop handler currently only logs the drag event without performing any mutations. The TODO comment outlines the required scenarios, but this functionality is not yet implemented.
If this feature is needed for the PR, I can help generate the implementation for the four drag-to-tab scenarios outlined in the TODO. Otherwise, would you like me to open a tracking issue for this work?
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useTabContentDrop
around lines 14–23, the drop handler only logs the event; implement the four
drag-to-tab behaviors with tRPC mutations: (1) if item.tabId === tabToRender.id,
call a mutation to duplicate the tab and create a new group containing both
tabs; (2) if the target tab is a child of a group, call a mutation to
move/redirect the dragged tab into the parent group; (3) if the target tab is
already a group, call a mutation to add the dragged tab into that group's split
view (preserving split orientation/index); (4) if both are single tabs, call a
mutation to create a new group container combining them; ensure you derive
parent/group relationships from current store/state, perform optimistic UI
updates (update local state/store to reflect grouping/split immediately), handle
mutation errors by rolling back and logging, and emit appropriate
analytics/events after success.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
apps/desktop/src/renderer/react-query/tabs/useSplit.ts (1)
3-15: useSplit hook looks correct; consider parallel cache invalidationThe wrapper correctly forwards options and augments
onSuccessto invalidate tabs and workspaces, then calls the user callback. If you want a tiny perf win, you could invalidate both caches in parallel:- onSuccess: async (...args) => { - await utils.tabs.invalidate(); - await utils.workspaces.invalidate(); - await options?.onSuccess?.(...args); - }, + onSuccess: async (...args) => { + await Promise.all([ + utils.tabs.invalidate(), + utils.workspaces.invalidate(), + ]); + await options?.onSuccess?.(...args); + },apps/desktop/src/renderer/react-query/tabs/useUngroup.ts (1)
3-15: useUngroup implementation is solid; potential to DRY invalidation patternThe hook correctly wraps
trpc.tabs.ungroup.useMutationand centralizes tabs/workspaces invalidation before delegating tooptions?.onSuccess. Given this same pattern appears in multiple tab hooks, you might later factor a small helper (e.g.,withTabsWorkspaceInvalidation) to reduce duplication, but it’s not urgent.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (1)
18-28: Active workspace and workspace tabs query usage looks correct; consider lifting query if neededDeriving
activeWorkspaceviatrpc.workspaces.getActive.useQuery()and then callingtabs.getByWorkspacewith{ enabled: !!activeWorkspace?.id }is a reasonable pattern; the optional chaining plusenabledguard avoids unnecessary fetches, andisActivefromactiveWorkspace?.activeTabIdis straightforward.Note that using
getByWorkspaceinside everyTabItemmeans many components subscribe to the same React Query cache key; the fetch is still deduped, but if tab lists grow very large you might eventually want to lift this query up to the parent tabs view and passallTabsdown via props to reduce per-item hook overhead.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
9-12: Avoid non-null assertion when value can be undefined.The non-null assertion on line 10 (
activeWorkspaceId!) is unnecessary since the query is already guarded by theenabledflag. Consider using a default value or accepting that the parameter can be undefined:const { data: allTabs = [] } = trpc.tabs.getByWorkspace.useQuery( - { workspaceId: activeWorkspaceId! }, + { workspaceId: activeWorkspaceId ?? "" }, { enabled: !!activeWorkspaceId }, );Or update the TRPC procedure to accept optional workspaceId and handle it server-side.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx (1)
96-103: Remove debug console.log statement.The
console.logon line 99 should be removed before merging to production.const handleRemoveChild = (groupId: string, tabId: string) => { // The Mosaic onChange will handle this automatically when user closes a pane // But we can also call updateLayout directly if needed - console.log("Remove child will be handled by Mosaic onChange", { - groupId, - tabId, - }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/index.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/useMoveOutOfGroup.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/useReorder.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/useSetParent.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/useSplit.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/useSplitActiveTab.ts(1 hunks)apps/desktop/src/renderer/react-query/tabs/useUngroup.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/DropOverlay.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx(2 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/useTabContentDrop.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx(4 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useGroupDrop.ts(0 hunks)apps/desktop/src/renderer/screens/main/index.tsx(2 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts(0 hunks)apps/desktop/src/renderer/stores/tabs/drag-logic.ts(0 hunks)
💤 Files with no reviewable changes (5)
- apps/desktop/src/renderer/stores/tabs/drag-logic.test.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useGroupDrop.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/useTabContentDrop.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/DropOverlay.tsx
- apps/desktop/src/renderer/stores/tabs/drag-logic.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/renderer/react-query/tabs/index.ts
🧰 Additional context used
🧬 Code graph analysis (12)
apps/desktop/src/renderer/react-query/tabs/useMoveOutOfGroup.ts (1)
apps/desktop/src/renderer/react-query/tabs/index.ts (1)
useMoveOutOfGroup(11-11)
apps/desktop/src/renderer/react-query/tabs/useSplit.ts (1)
apps/desktop/src/renderer/react-query/tabs/index.ts (1)
useSplit(7-7)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/EmptyTabView.tsx (1)
EmptyTabView(1-18)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (1)
SingleTabView(10-43)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx (1)
GroupTabView(45-154)
apps/desktop/src/renderer/react-query/tabs/useUngroup.ts (1)
apps/desktop/src/renderer/react-query/tabs/index.ts (1)
useUngroup(9-9)
apps/desktop/src/renderer/react-query/tabs/useReorder.ts (1)
apps/desktop/src/renderer/react-query/tabs/index.ts (1)
useReorder(10-10)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx (5)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (1)
Tab(670-670)apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)apps/desktop/src/renderer/react-query/tabs/useUpdateLayout.ts (1)
useUpdateLayout(7-24)apps/desktop/src/renderer/react-query/tabs/useSplit.ts (1)
useSplit(3-16)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/GroupTabPane.tsx (1)
GroupTabPane(20-68)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (3)
apps/desktop/src/main/lib/db/schemas.ts (1)
Tab(48-50)apps/desktop/src/main/lib/terminal-manager.ts (1)
terminalManager(398-398)apps/desktop/src/lib/trpc/routers/tabs/utils/layout/layout.ts (2)
removeTabFromLayout(27-49)extractTabIdsFromLayout(6-22)
apps/desktop/src/renderer/screens/main/index.tsx (2)
apps/desktop/src/renderer/react-query/tabs/useSplitActiveTab.ts (1)
useSplitActiveTab(3-16)apps/desktop/src/renderer/stores/tabs/useAgentHookListener.ts (1)
useAgentHookListener(9-41)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (4)
apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (1)
Tab(670-670)apps/desktop/src/renderer/react-query/tabs/useRemoveTab.ts (1)
useRemoveTab(7-23)apps/desktop/src/renderer/react-query/tabs/useSplit.ts (1)
useSplit(3-16)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/Terminal.tsx (1)
Terminal(14-201)
apps/desktop/src/renderer/react-query/tabs/useSplitActiveTab.ts (1)
apps/desktop/src/renderer/react-query/tabs/index.ts (1)
useSplitActiveTab(8-8)
apps/desktop/src/renderer/react-query/tabs/useSetParent.ts (1)
apps/desktop/src/renderer/react-query/tabs/index.ts (1)
useSetParent(6-6)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (7)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/types.ts (1)
TabItemProps(10-13)apps/desktop/src/renderer/react-query/tabs/index.ts (4)
useRemoveTab(2-2)useSetActiveTab(4-4)useUngroup(9-9)useMoveOutOfGroup(11-11)apps/desktop/src/renderer/stores/tabs/hooks.ts (2)
useRemoveTab(8-8)useSetActiveTab(10-11)apps/desktop/src/renderer/react-query/tabs/useUngroup.ts (1)
useUngroup(3-16)apps/desktop/src/renderer/react-query/tabs/useMoveOutOfGroup.ts (1)
useMoveOutOfGroup(3-16)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useDragTab.ts (1)
useDragTab(4-43)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/useTabRename.ts (1)
useTabRename(4-61)
🔇 Additional comments (15)
apps/desktop/src/renderer/react-query/tabs/useMoveOutOfGroup.ts (1)
3-15: useMoveOutOfGroup follows the established mutation patternThe hook cleanly wraps
trpc.tabs.moveOutOfGroup.useMutation, performs tabs/workspaces invalidation, and then invokes any useronSuccesshandler. This keeps the behavior consistent with the rest of the tab mutations.apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx (3)
4-10: Tabs actions now sourced from React-Query hooks—looks aligned with the PR directionSwitching
TabItemto consumeuseRemoveTab,useSetActiveTab,useUngroup, anduseMoveOutOfGroupfromrenderer/react-query/tabscleanly decouples it from the old tabs store and matches the router-driven design.
34-42: Remove and activate handlers correctly delegate to mutationsStopping propagation in
handleRemoveTaband then invokingremoveTabMutation.mutate({ id: tab.id }), plus guarding rename state inhandleTabClickbeforesetActiveTabMutation.mutate({ tabId: tab.id }), is clean and avoids accidental activation/removal during rename.
78-80: Group detection and children rendering remain clear and self‑containedUsing
const isGroupTab = tab.type === "group";keeps the group check simple and feeds nicely into both the context menu (onUngroup) and the conditional child rendering. The updated children container (ml-4 mt-1 space-y-1) is straightforward and the recursiveTabItemusage forchildTabspreserves the existing mental model for nested groups.Also applies to: 179-190
apps/desktop/src/renderer/react-query/tabs/useSetParent.ts (1)
3-15: useSetParent correctly wraps the TRPC mutation with cache invalidationThe hook cleanly delegates to
trpc.tabs.setParent.useMutation, invalidates tabs and workspaces on success, then calls any user‑suppliedonSuccesscallback. This keeps parent/child relationships in sync with the rest of the tabs/workspaces data model and matches the pattern used by the other tab hooks.apps/desktop/src/renderer/react-query/tabs/useReorder.ts (1)
3-15: LGTM!The hook follows the established pattern for tab mutation hooks with proper cache invalidation for both tabs and workspaces.
apps/desktop/src/renderer/react-query/tabs/useSplitActiveTab.ts (1)
3-15: LGTM!The hook correctly implements the splitActiveTab mutation with proper cache invalidation, following the same pattern as other tab mutation hooks.
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/index.tsx (1)
14-47: LGTM!The tab resolution logic correctly handles both terminal and group tabs, properly resolving parent groups when the active tab is a child. The simplified rendering without drop overlay improves maintainability.
apps/desktop/src/renderer/screens/main/index.tsx (1)
25-49: LGTM!The hotkey handlers correctly use the
useSplitActiveTabmutation with proper direction values consistent with Mosaic terminology (rowfor vertical splits,columnfor horizontal splits).apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/GroupTabView/index.tsx (3)
21-43: LGTM!The
cleanLayouthelper correctly removes invalid tab IDs from the Mosaic layout tree, handling both leaf nodes and branch nodes with proper edge case handling.
46-56: LGTM!The TRPC queries and child tab derivation logic correctly fetch and filter tabs for the group view.
105-153: LGTM!The
renderPanefunction correctly handles missing tabs with appropriate error messages, and the conditional rendering provides a good user experience for empty groups.apps/desktop/src/lib/trpc/routers/tabs/tabs.ts (2)
76-141: Previous issues successfully addressed.The
removemutation now correctly tracks all removed tab IDs (including group children) and updatesworkspace.activeTabIdwhen it points to any removed tab, preventing stale references.
250-307: LGTM!The
updateLayoutmutation properly handles removed tab IDs and updatesworkspace.activeTabIdwhen necessary, including when the group itself is deleted (layout becomes null).apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/SingleTabView.tsx (1)
1-43: LGTM!The refactoring successfully migrates from store-based to mutation-based tab management with proper type safety using the
Tabtype from the TRPC router. The split direction mappings are correct and consistent with Mosaic terminology.
| const handleUngroup = () => { | ||
| ungroupTabs(tab.id); | ||
| ungroupMutation.mutate({ groupId: tab.id }); | ||
| }; | ||
|
|
||
| const handleMoveOutOfGroup = () => { | ||
| if (!tab.parentId) return; | ||
|
|
||
| // Find the parent group's index in the workspace tabs | ||
| const workspaceTabs = tabs.filter( | ||
| const workspaceTabs = allTabs.filter( | ||
| (t) => t.workspaceId === tab.workspaceId && !t.parentId, | ||
| ); | ||
| const parentIndex = workspaceTabs.findIndex((t) => t.id === tab.parentId); | ||
|
|
||
| // Place after the parent (parentIndex + 1) | ||
| if (parentIndex !== -1) { | ||
| ungroupTab(tab.id, parentIndex + 1); | ||
| moveOutOfGroupMutation.mutate({ | ||
| tabId: tab.id, | ||
| targetIndex: parentIndex + 1, | ||
| }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Ungroup and moveOutOfGroup wiring look reasonable; please confirm index semantics
Using ungroupMutation.mutate({ groupId: tab.id }) for group tabs matches the intent, and handleMoveOutOfGroup correctly early‑returns when tab.parentId is missing. Computing workspaceTabs by filtering allTabs to the same workspaceId and !parentId and then placing the tab at parentIndex + 1 is a sensible “after the parent group” behavior.
Two points to double‑check:
- That
tabs.getByWorkspacereturns tabs in the same canonical order that themoveOutOfGroupmutation expects fortargetIndex(i.e., that filtering still preserves the correct positional basis). - What behavior you want when
parentIndex === -1(e.g., if data is briefly stale); right now the handler silently no‑ops, which is probably acceptable but could mask edge‑case inconsistencies.
You can inspect the server‑side definition to confirm the ordering assumptions with a quick search like:
🏁 Script executed:
#!/bin/bash
rg -n "moveOutOfGroup" apps/desktop/src/lib/trpc -C3Length of output: 478
I need to see the full implementation of the moveOutOfGroup mutation to understand how targetIndex is used and what ordering semantics are expected.
I need to see the complete implementation of the moveOutOfGroup mutation to verify the index semantics.
---
🏁 Script executed:
```shell
#!/bin/bash
# Extract the full moveOutOfGroup procedure implementation
sed -n '383,450p' apps/desktop/src/lib/trpc/routers/tabs/tabs.ts
Length of output: 89
Let me retrieve the full implementation using a broader context search:
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for moveOutOfGroup with expanded context to see full implementation
rg -n "moveOutOfGroup" apps/desktop/src/lib/trpc -C15
Length of output: 2420
I need to see more of the mutation implementation to understand how targetIndex is used:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full moveOutOfGroup implementation
rg -n "moveOutOfGroup" apps/desktop/src/lib/trpc -A50 | head -80
Length of output: 3932
I found a critical issue. Let me verify the exact client-side code to confirm:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the exact filter code from the client-side handleMoveOutOfGroup
rg -n "const workspaceTabs = allTabs" apps/desktop/src/renderer/screens -A5
Length of output: 840
Server applies targetIndex to a sorted array; client calculates it from an unsorted array—this causes misplacement.
The server-side moveOutOfGroup mutation (lines 404-406 in tabs.ts) sorts workspace tabs by position before inserting: .sort((a, b) => a.position - b.position). The client-side code (lines 59–62 in TabItem) filters the same tabs but does not sort them before calculating parentIndex. This index mismatch will place tabs at incorrect positions unless they already happen to be in position order.
To fix:
- Sort the client-side
workspaceTabsarray by position before findingparentIndex:.sort((a, b) => a.position - b.position)after the filter.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/TabsView/TabItem/index.tsx
around lines 51 to 71, the client computes parentIndex from an unsorted
workspaceTabs array which mismatches the server that sorts by position; update
the client to sort the filtered workspaceTabs by position (ascending) before
calling findIndex so targetIndex aligns with server-side insertion logic.
2 participants
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.