Conversation
Auto-committed for PR creation
WalkthroughAdds balanced-mosaic construction and widespread drag‑and‑drop support: a backend buildBalancedMosaicTree, frontend mosaic helpers (insert/remove/validate), new TabDropZone and SingleTabView components, react-dnd wiring (DndProvider/drag sources), and changes to TabGroup to use balanced layouts and persist mosaic updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Renderer as Renderer (TabGroup / SingleTabView)
participant DropZone as TabDropZone
participant Helpers as mosaic-helpers
participant IPC as Main IPC (tab-move / tab-create / tab-update-mosaic-tree)
participant Store as Backend Store
User->>Renderer: drag tab -> drop on target
Renderer->>DropZone: drop event with item (tab, worktreeId, workspaceId, pos)
DropZone-->>Renderer: onDrop callback
Renderer->>Helpers: handleTabDropValidation(droppedTab, target, existingTree)
alt validation requires copy (cross-worktree)
Helpers->>IPC: request tab-create (copy)
IPC-->>Helpers: new copied tab
end
Helpers-->>Renderer: validation result (valid, tab)
Renderer->>IPC: tab-move (or tab-create+tab-move) to target group
Renderer->>Helpers: insertTabIntoMosaicTree(existingTree, tabId, position)
Helpers-->>Renderer: updated mosaicTree
Renderer->>IPC: tab-update-mosaic-tree(groupId, mosaicTree)
IPC->>Store: persist changes
Store-->>Renderer: updated workspace/group state event
Renderer->>Renderer: re-render using balanced mosaic layout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ 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 (2)
apps/desktop/src/main/lib/workspace/tab-helpers.ts (1)
131-155: Balanced mosaic helper looks correct; consider aligning empty-case semanticsThe recursive split logic and alternating row/column directions look correct and match how the tree is consumed elsewhere.
The function returns
nullfor an emptytabIdsarray whileTab.mosaicTreeitself is modeled asMosaicNode<string> | undefined. You’re already normalizing via callers, so this is fine, but longer term you might consider returningMosaicNode<string> | undefinedhere to avoid needingnull→undefinedcoercion at call sites.apps/desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx (1)
13-35: Local balanced-tree helper mirrors backend correctly; duplication is acceptable for nowThe local
buildBalancedMosaicTreeimplementation matches the backend helper’s behavior (split in halves, alternate row/column by depth) and is appropriate for constructing a stable mosaic layout in the renderer.Given the Node import in
tab-helpers.ts, duplicating this small pure helper here is a reasonable compromise. Longer term, consider moving this logic to a shared, Node-free module so both main and renderer rely on a single source of truth.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/main/lib/workspace/tab-helpers.ts(1 hunks)apps/desktop/src/main/lib/workspace/tab-operations.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/main/lib/workspace/tab-operations.ts (1)
apps/desktop/src/main/lib/workspace/tab-helpers.ts (1)
buildBalancedMosaicTree(135-155)
apps/desktop/src/main/lib/workspace/tab-helpers.ts (1)
apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)
apps/desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx (1)
apps/desktop/src/shared/types.ts (1)
MosaicNode(35-35)
🪛 GitHub Actions: CI
apps/desktop/src/main/lib/workspace/tab-operations.ts
[error] 92-92: TS2322: Type 'MosaicNode | null' is not assignable to type 'MosaicNode | undefined'. Type 'null' is not assignable to type 'MosaicNode | undefined'.
🔇 Additional comments (3)
apps/desktop/src/main/lib/workspace/tab-operations.ts (1)
15-21: ImportingbuildBalancedMosaicTreehere is appropriateThe new import is consistent with how you use the helper in
createTab; no issues with the dependency wiring.apps/desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx (2)
53-68: Mosaic state initialization matches backend semanticsInitializing
mosaicTreeby preferringgroupTab.mosaicTreeand only falling back to a freshly built balanced tree when there’s no saved layout but tabs exist is a good default. It keeps the renderer in sync with persisted state while still handling legacy/empty cases gracefully.No changes needed here.
70-79: Sync effect correctly reconciles backend changes and tab-set changesThe
useEffectthat:
- applies
groupTab.mosaicTreewhen present, and- rebuilds a balanced tree when
mosaicTreeis cleared butgroupTab.tabsstill has entrieskeeps the local
mosaicTreealigned with backend updates and avoids ending up with anulllayout when tabs exist. The dependency array[groupTab.mosaicTree, groupTab.tabs]is sufficient to cover these cases.This looks solid as-is.
| parentTab!.tabs = parentTab!.tabs || []; | ||
| parentTab!.tabs.push(tab); | ||
|
|
||
| // Rebuild the mosaic tree to include the new tab | ||
| const allTabIds = parentTab!.tabs.map((t) => t.id); | ||
| parentTab!.mosaicTree = buildBalancedMosaicTree(allTabIds); |
There was a problem hiding this comment.
🧩 Analysis chain
Fix TS2322 (null vs undefined) and confirm layout reset behavior
buildBalancedMosaicTree returns MosaicNode<string> | null, but parentTab!.mosaicTree is typed as MosaicNode<string> | undefined, which causes the CI error:
TS2322: Type 'MosaicNode | null' is not assignable to type 'MosaicNode | undefined'.
You can fix this by normalizing null to undefined:
- // Rebuild the mosaic tree to include the new tab
- const allTabIds = parentTab!.tabs.map((t) => t.id);
- parentTab!.mosaicTree = buildBalancedMosaicTree(allTabIds);
+ // Rebuild the mosaic tree to include the new tab
+ const allTabIds = parentTab!.tabs.map((t) => t.id);
+ parentTab!.mosaicTree =
+ buildBalancedMosaicTree(allTabIds) ?? undefined;This satisfies the type and matches the existing null/undefined handling elsewhere.
Note that this logic fully rebuilds the group’s mosaic tree whenever a tab is added, so any custom split layout for that group will be replaced by the balanced layout. Please verify that this behavior is intended for the UX.
I need to gather information from the codebase to verify the review comment. Let me search for the relevant type definitions and function signatures.
🏁 Script executed:
#!/bin/bash
# Find buildBalancedMosaicTree function definition and its return type
echo "=== Searching for buildBalancedMosaicTree function definition ==="
rg -nP 'buildBalancedMosaicTree|function buildBalancedMosaicTree' -A 10
echo ""
echo "=== Searching for mosaicTree property type definition ==="
rg -nP 'mosaicTree\s*[?:]' -B 2 -A 2
echo ""
echo "=== Looking at tab-operations.ts context around the issue ==="
fd 'tab-operations.ts' --exec cat {} \;Length of output: 25883
Fix TS2322 (null vs undefined) by normalizing return value to match property type
The type mismatch is confirmed: buildBalancedMosaicTree returns MosaicNode<string> | null but parentTab!.mosaicTree is typed as MosaicNode<string> | undefined. Apply the suggested fix using the nullish coalescing operator:
// Rebuild the mosaic tree to include the new tab
const allTabIds = parentTab!.tabs.map((t) => t.id);
- parentTab!.mosaicTree = buildBalancedMosaicTree(allTabIds);
+ parentTab!.mosaicTree =
+ buildBalancedMosaicTree(allTabIds) ?? undefined;This aligns with existing patterns in the codebase (e.g., deleteTab line 310, updateTabMosaicTree line 413). The behavior is correct: the tree is rebuilt with a balanced layout each time a tab is added, replacing any prior custom splits. Verify this is the intended UX before merging.
🧰 Tools
🪛 GitHub Actions: CI
[error] 92-92: TS2322: Type 'MosaicNode | null' is not assignable to type 'MosaicNode | undefined'. Type 'null' is not assignable to type 'MosaicNode | undefined'.
🤖 Prompt for AI Agents
In apps/desktop/src/main/lib/workspace/tab-operations.ts around lines 87 to 92,
buildBalancedMosaicTree returns MosaicNode<string> | null while
parentTab!.mosaicTree is typed as MosaicNode<string> | undefined; change the
assignment to normalize null to undefined by using the nullish coalescing
operator so parentTab!.mosaicTree gets (buildBalancedMosaicTree(allTabIds) ??
undefined), ensuring the types align with existing code patterns.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
apps/desktop/src/renderer/index.tsx (1)
9-14: StrictMode removal is a pragmatic workaround; plan to revisit when react-dnd catches upDisabling
React.StrictModehere is a reasonable short‑term fix to avoid double mounting with theHTML5Backendsingleton under React 19, but it also removes useful dev‑time checks for the whole app. Once the react‑dnd stack has solid React 19/StrictMode support, it would be good to either re‑enableStrictModeglobally or localize the DnD workaround to only the affected subtree.Please double‑check the current react‑dnd / React 19 guidance to confirm this is still required before merging.
apps/desktop/src/renderer/screens/main/components/MainContent/TabDropZone.tsx (1)
1-176: Multi-zone tab drop handling looks correct; minor DRY opportunityThe five
useDrophooks and overlay rendering correctly implement the top/right/bottom/left/center drop semantics for"TAB"items, and theshowOverlay+ pointer‑events setup ensures the overlay only participates while a compatible drag is active. The publiconDropsignature aligns with call sites inTabGroupandSingleTabView.If this grows further, consider extracting a small helper to create the drop spec per position to reduce repetition across the five zones, but it isn’t strictly necessary right now.
apps/desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx (1)
106-161: Tab drop handling into groups is functionally correct; consider avoiding full reloadThe
handleTabDroppipeline—validation viahandleTabDropValidation,tab-moveinto this group, updating the mosaic withinsertTabIntoMosaicTree, persisting viatab-update-mosaic-tree, then updatingmosaicTreestate—looks correct and matches the new mosaic helper semantics.Triggering
window.location.reload()at the end is a heavy but safe way to refresh workspace data; however, it resets the entire UI and may feel jarring. Longer term, you could rely on context/state updates (or a targeted data refetch) to reflect the new group layout without a full reload.apps/desktop/src/renderer/screens/main/components/MainContent/SingleTabView.tsx (2)
20-101: Avoid full window reload after successful dropThe drop handler is functionally correct, but
window.location.reload()is heavy‑handed and will reset unrelated UI state (e.g., terminals, expanded trees). Prefer updating the relevant workspace/tab context after the IPC calls instead of reloading the whole window so the UX stays smooth.
104-107: Don’t forcegroupTabIdwhen rendering a non‑group tab
TabContentis rendered withgroupTabId="". IfgroupTabIdis optional and “no group” is a valid state, it’s clearer to omit the prop (or passundefined) rather than an empty string, which can be ambiguous in downstream truthiness checks.apps/desktop/src/renderer/screens/main/components/MainContent/mosaic-helpers.ts (2)
38-99: Preserve parent node metadata when rebuilding mosaic nodesIn
removeTabFromMosaicTreeandinsertTabIntoMosaicTree, new parent nodes are created with onlydirection, which will drop any other properties present on the original node (e.g., split ratios or future metadata). It’s safer to carry existing fields forward when both branches remain.You can preserve metadata by spreading the original node and overriding just
first/second:- return { - direction: tree.direction, - first: newFirst, - second: newSecond, - } as MosaicNode<string>; + return { + ...tree, + first: newFirst, + second: newSecond, + };and similarly in
insertTabIntoMosaicTreewhen creating the split.
104-124: Balanced tree builder is correct; consider avoiding theascast
buildBalancedMosaicTree’s splitting logic is sound and will always produce a non‑empty node whentabIds.length >= 2. Theas MosaicNode<string>cast is only needed because the return type includesundefinedfor the empty input case. You can tighten this a bit by asserting non‑undefined at the recursive call sites:return { direction, first: buildBalancedMosaicTree(firstHalf, depth + 1)!, second: buildBalancedMosaicTree(secondHalf, depth + 1)!, };or by extracting a private helper that always returns
MosaicNode<string>for non‑empty arrays, so public API stays the same without casts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/desktop/src/main/lib/workspace/tab-operations.ts(2 hunks)apps/desktop/src/renderer/contexts/AppProviders.tsx(2 hunks)apps/desktop/src/renderer/index.tsx(1 hunks)apps/desktop/src/renderer/screens/main/MainScreen.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/MainContent/SingleTabView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/MainContent/TabDropZone.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx(4 hunks)apps/desktop/src/renderer/screens/main/components/MainContent/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/MainContent/mosaic-helpers.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/components/TabItem/TabItem.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/renderer/screens/main/components/MainContent/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/lib/workspace/tab-operations.ts
🧰 Additional context used
🧬 Code graph analysis (7)
apps/desktop/src/renderer/index.tsx (1)
apps/desktop/src/renderer/contexts/AppProviders.tsx (1)
AppProviders(17-44)
apps/desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx (2)
apps/desktop/src/renderer/screens/main/components/MainContent/SingleTabView.tsx (1)
SingleTabView(12-109)apps/desktop/src/renderer/screens/main/components/MainContent/index.ts (1)
SingleTabView(3-3)
apps/desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx (3)
apps/desktop/src/shared/types.ts (2)
MosaicNode(35-35)Tab(44-56)apps/desktop/src/renderer/screens/main/components/MainContent/mosaic-helpers.ts (3)
buildBalancedMosaicTree(104-124)handleTabDropValidation(147-203)insertTabIntoMosaicTree(72-99)apps/desktop/src/renderer/screens/main/components/MainContent/TabDropZone.tsx (1)
TabDropZone(27-177)
apps/desktop/src/renderer/screens/main/components/MainContent/SingleTabView.tsx (4)
apps/desktop/src/shared/types.ts (1)
Tab(44-56)apps/desktop/src/renderer/screens/main/components/MainContent/mosaic-helpers.ts (2)
handleTabDropValidation(147-203)createSimpleMosaicTree(129-142)apps/desktop/src/renderer/screens/main/components/MainContent/TabContent.tsx (1)
TabContent(24-120)apps/desktop/src/renderer/screens/main/components/MainContent/TabDropZone.tsx (1)
TabDropZone(27-177)
apps/desktop/src/renderer/contexts/AppProviders.tsx (6)
apps/desktop/src/renderer/contexts/index.ts (5)
WorkspaceProvider(2-2)TabProvider(3-3)SidebarProvider(4-4)WorktreeOperationsProvider(5-5)TaskProvider(6-6)apps/desktop/src/renderer/contexts/WorkspaceContext.tsx (1)
WorkspaceProvider(31-46)apps/desktop/src/renderer/contexts/TabContext.tsx (1)
TabProvider(30-53)apps/desktop/src/renderer/contexts/SidebarContext.tsx (1)
SidebarProvider(24-32)apps/desktop/src/renderer/contexts/WorktreeOperationsContext.tsx (1)
WorktreeOperationsProvider(25-44)apps/desktop/src/renderer/contexts/TaskContext.tsx (1)
TaskProvider(40-57)
apps/desktop/src/renderer/screens/main/components/MainContent/mosaic-helpers.ts (1)
apps/desktop/src/shared/types.ts (2)
MosaicNode(35-35)Tab(44-56)
apps/desktop/src/renderer/screens/main/components/MainContent/TabDropZone.tsx (1)
apps/desktop/src/shared/types.ts (1)
Tab(44-56)
🪛 GitHub Actions: CI
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/components/TabItem/TabItem.tsx
[error] 161-161: Type 'ConnectDragSource' is not assignable to type 'Ref | undefined'. Type 'ConnectDragSource' is not assignable to type '(instance: HTMLButtonElement | null) => void | (() => VoidOrUndefinedOnly)'.
🔇 Additional comments (9)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/components/TabItem/TabItem.tsx (1)
168-168: LGTM!The drag visual feedback is well-implemented. The opacity reduction and cursor change provide clear user feedback during drag operations.
apps/desktop/src/renderer/contexts/AppProviders.tsx (1)
20-42: DnD provider placement and lifted tab selection state look correctWrapping the entire provider tree in
DndProviderand liftingselectedWorktreeId/selectedTabIdintoAppProvidersaligns with theWorkspaceProvider/TabProvidercontracts and should give all tab components consistent drag-and-drop context. No issues from a correctness or lifecycle standpoint.apps/desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx (1)
17-18: SingleTabView integration for base-level tabs is consistentSwitching the base-tab branch to render
SingleTabView(and importing it here) cleanly centralizes single-tab rendering and drop handling, while the group-tab paths still useTabGroup. The conditions aroundselectedTab/parentGroupTablook sound.Also applies to: 147-149
apps/desktop/src/renderer/screens/main/MainScreen.tsx (1)
83-171: Removal of nested DnD provider in MainScreen is appropriateReplacing the inner
DndProviderwith a fragment keeps the MainScreen layout intact while relying on the single app‑level provider inAppProviders, which should prevent multiple backend instances.apps/desktop/src/renderer/screens/main/components/MainContent/TabGroup.tsx (3)
36-61: Balanced mosaic initialization and resync logic look solid; ensure helper tolerates empty treesInitializing
mosaicTreefromgroupTab.mosaicTreewhen present, and otherwise building a balanced tree from allgroupTab.tabs, is a nice improvement over ad‑hoc defaults. The follow‑upuseEffectthat reconstructs a balanced tree when the mosaic is cleared but tabs still exist keeps state in sync with backend changes.This relies on
buildBalancedMosaicTreeandgetTabIdsFromTreecorrectly handlingundefined/nullroots. Please double‑check thatgetTabIdsFromTreeexplicitly treatsnull/undefinedas “no tabs” so calls likegetTabIdsFromTree(mosaicTree)andgetTabIdsFromTree(newTree)can’t throw when the tree is absent.
64-104: Mosaic change persistence and tab deletion behavior are reasonable but depend on robust ID extraction
handleMosaicChange’s flow—diff old/new tab IDs, delete removed tabs via IPC, normalizenulltoundefinedfor the backend, then persist and mirror the updated tree into local state—is logically coherent and keeps client and backend aligned.The main precondition is that
getTabIdsFromTreereturns an empty set when passednull/undefinedsooldTabIds/newTabIdsare well‑defined even when the mosaic is initially unset or completely cleared. As long as that helper behaves defensively, this code should be safe.
247-256: TabDropZone overlay integration with the mosaic container is appropriateAdding
relativeto the mosaic container and placing<TabDropZone onDrop={handleTabDrop} />as an absolutely positioned child correctly overlays the drop zones across the entire group view without interfering withreact-mosaic-component’s layout.apps/desktop/src/renderer/screens/main/components/MainContent/mosaic-helpers.ts (2)
7-33: Mosaic ID collection and duplicate check look solid
getTabIdsFromTreecorrectly recurses over both branches andcanAddTabToMosaicuses it to guard against duplicates. This is a clear and safe way to enforce uniqueness per mosaic tree.
129-203: Drop validation and simple mosaic creation look correct
createSimpleMosaicTree’s orientation and ordering behavior matches the position semantics (top/bottom → column, left/right → row, new tab on the “near” side).handleTabDropValidationcorrectly prevents self‑drops, avoids duplicates when given an existing tree, and clones tabs when crossing worktrees, with clear error signaling via{ valid, reason }.This is a good centralization of the DnD checks.
| // Set up drag source | ||
| const [{ isDragging }, dragRef] = useDrag( | ||
| () => ({ | ||
| type: "TAB", | ||
| item: { tab, worktreeId, workspaceId }, | ||
| canDrag: !isEditing, // Don't allow dragging while editing | ||
| collect: (monitor) => ({ | ||
| isDragging: monitor.isDragging(), | ||
| }), | ||
| }), | ||
| [tab, worktreeId, workspaceId, isEditing], | ||
| ); |
There was a problem hiding this comment.
Fix type mismatch: ConnectDragSource is not compatible with Ref<HTMLButtonElement>.
The pipeline failure indicates that dragRef from useDrag returns a ConnectDragSource type, which cannot be directly assigned as a React ref. React DnD's connector functions need to be called on the DOM node, not passed as a ref prop.
Additionally, workspaceId is optional in the component props but is always included in the drag item payload. Ensure downstream drop handlers can handle undefined values.
Apply this diff to fix the type error using a callback ref pattern:
const [isEditing, setIsEditing] = useState(false);
const [editName, setEditName] = useState(tab.name);
const inputRef = useRef<HTMLInputElement>(null);
+ const buttonRef = useRef<HTMLButtonElement>(null);
// Set up drag source
const [{ isDragging }, dragRef] = useDrag(
() => ({
type: "TAB",
item: { tab, worktreeId, workspaceId },
canDrag: !isEditing, // Don't allow dragging while editing
collect: (monitor) => ({
isDragging: monitor.isDragging(),
}),
}),
[tab, worktreeId, workspaceId, isEditing],
);
+ // Connect drag source
+ useEffect(() => {
+ if (buttonRef.current) {
+ dragRef(buttonRef.current);
+ }
+ }, [dragRef]);
+
// Focus input when entering edit modeThen update the button ref attachment:
<ContextMenuTrigger asChild>
<button
- ref={dragRef}
+ ref={buttonRef}
type="button"Also applies to: 161-161
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/components/TabItem/TabItem.tsx
around lines 54-65 (and also at line 161), the dragRef returned by useDrag is a
connector function type (ConnectDragSource) and cannot be used directly as a
React Ref<HTMLButtonElement>; replace the direct ref assignment with a callback
ref that both forwards the local button ref and calls the drag connector (e.g.,
store the DOM node in your buttonRef via setButtonRef(node) and then call
dragRef(node) if dragRef exists), update the button element to use that callback
ref, and keep canDrag logic as-is; also ensure the drag item includes
workspaceId only if present and document that downstream drop handlers handle
undefined workspaceId (or pass workspaceId: workspaceId ?? null) so type
mismatch and potential undefined handling are resolved.
1 participant
Auto-committed for PR creation
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Bug Fixes / Improvements