Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds worktree deletion (UI close control, confirmation, IPC removal, workspace/worktrees refresh, selection-reset) and expands add-task/worktree creation flows (modal modes, branch fetching, live setup progress, creation state/output). Hooks and tab/modal components updated to surface new handlers and state. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as TaskTabs UI
participant Dialog as Confirm Dialog
participant Hook as useWorktrees.handleDeleteWorktree
participant IPC as IPC ("worktree-remove")
participant State as Workspace State
User->>UI: Click tab close (onClose)
UI->>Dialog: Open confirmation (may show uncommitted-changes warning)
User->>Dialog: Confirm removal
Dialog->>Hook: Call onDeleteWorktree(worktreeId)
Hook->>IPC: send "worktree-remove" request
IPC-->>Hook: success / error
alt success
Hook->>State: refresh current workspace
Hook->>State: reload all workspaces
alt deleted was selected
Hook->>State: select first remaining worktree & its first tab
else no remaining worktrees
Hook->>State: clear selection
end
Hook-->>UI: deletion complete
else error
Hook->>UI: surface error (console/alert)
end
UI->>User: update tabs view
sequenceDiagram
participant User
participant UI as AddTaskModal
participant Hook as useTasks.handleCreateTask
participant IPC as IPC ("worktree-create", "worktree-setup-progress")
participant State as Workspace State
User->>UI: Open AddTaskModal (mode="new")
UI->>Hook: handleOpenAddTaskModal(mode="new")
Hook->>IPC: fetch branches
Hook-->>UI: branches populated
User->>UI: Submit create (branch, options)
UI->>Hook: handleCreateTask(payload)
Hook->>IPC: send "worktree-create"
IPC-->>Hook: emits "worktree-setup-progress" events
loop progress updates
IPC-->>Hook: progress chunk
Hook-->>UI: update setupStatus / setupOutput
end
IPC-->>Hook: creation success / failure
alt success
Hook->>State: refresh workspace, select new worktree/tab
Hook-->>UI: optionally show setup results, close modal after delay
else failure
Hook-->>UI: show failure status/output, keep modal open
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
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: 2
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTab.tsx (1)
13-41: Prop plumb-through looks correct; consider tightening event type (optional).The new
onClose/widthprops are cleanly added and correctly forwarded toWorktreeTabButton; wiring looks good. If you want stricter typing, you could narrowonClosetoReact.MouseEvent<HTMLButtonElement>here and inWorktreeTabButton, but it’s not required.apps/desktop/src/renderer/screens/main/MainScreen.tsx (1)
75-90: Hook wiring for deletion looks good; verify active-selection sync behavior.The integration of
useWorktrees(includinghandleDeleteWorktree) withMainScreenand the newTaskTabsprops (onDeleteWorktree,workspaceId) is consistent and type-safe.One thing to double-check in the broader flow:
handleTabSelectexplicitly updates the main process viaworkspace-set-active-selection, buthandleDeleteWorktreeinuseWorktreesonly updatesselectedWorktreeId/selectedTabIdwhen auto-selecting a new worktree after deletion. If you rely on the main process’s “active selection” for later operations, you may want to ensure that code paths triggered by delete also invoke the same IPC update (either inside the hook or via a shared helper) so state stays fully in sync.Also applies to: 136-168
apps/desktop/src/renderer/screens/main/hooks/useWorktrees.ts (1)
144-188: Consider simplifying the selection reset logic.The nested conditionals for resetting selection (lines 165-178) work correctly but are complex. Consider extracting this into a helper function for clarity.
Additionally, the check
refreshedWorkspace.worktrees && refreshedWorkspace.worktrees.length > 0on line 166 should be sufficient, but ensure thatworktreesis always defined as an array in theWorkspacetype to avoid potential issues.Example refactor:
+const resetSelectionAfterDelete = ( + workspace: Workspace, + deletedId: string, + currentSelectedId: string | null +) => { + if (currentSelectedId !== deletedId) return; + + const worktrees = workspace.worktrees || []; + if (worktrees.length === 0) { + setSelectedWorktreeId(null); + setSelectedTabId(null); + return; + } + + const firstWorktree = worktrees[0]; + setSelectedWorktreeId(firstWorktree.id); + const firstTab = firstWorktree.tabs?.[0]; + setSelectedTabId(firstTab?.id || null); +}; const handleDeleteWorktree = async (worktreeId: string) => { // ... existing code ... if (refreshedWorkspace) { setCurrentWorkspace(refreshedWorkspace); await loadAllWorkspaces(); - - // If we deleted the selected worktree, select the first available one - if (selectedWorktreeId === worktreeId) { - if (refreshedWorkspace.worktrees && refreshedWorkspace.worktrees.length > 0) { - const firstWorktree = refreshedWorkspace.worktrees[0]; - setSelectedWorktreeId(firstWorktree.id); - if (firstWorktree.tabs && firstWorktree.tabs.length > 0) { - setSelectedTabId(firstWorktree.tabs[0].id); - } else { - setSelectedTabId(null); - } - } else { - setSelectedWorktreeId(null); - setSelectedTabId(null); - } - } + resetSelectionAfterDelete(refreshedWorkspace, worktreeId, selectedWorktreeId); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/desktop/src/renderer/screens/main/MainScreen.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTab.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/types.ts(1 hunks)apps/desktop/src/renderer/screens/main/hooks/useWorktrees.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx (1)
apps/desktop/src/renderer/screens/main/components/Layout/StatusIndicator.tsx (1)
StatusIndicator(55-115)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx (5)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/SidebarToggle.tsx (1)
SidebarToggle(12-42)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/ModeToggle.tsx (1)
ModeToggle(8-33)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTab.tsx (1)
WorktreeTab(21-54)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/AddTaskButton.tsx (1)
AddTaskButton(10-21)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/PRActions.tsx (1)
PRActions(14-69)
apps/desktop/src/renderer/screens/main/MainScreen.tsx (2)
apps/desktop/src/renderer/screens/main/hooks/useWorktrees.ts (1)
useWorktrees(14-197)apps/desktop/src/renderer/screens/main/hooks/index.ts (1)
useWorktrees(6-6)
🔇 Additional comments (10)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/types.ts (1)
20-34: TaskTabsProps extension is consistent with usage.The additions of
onDeleteWorktreeandworkspaceIdalign with howTaskTabsis now consumed (IPC-backed delete + workspace scoping). Types look correct.apps/desktop/src/renderer/screens/main/hooks/useWorktrees.ts (2)
9-9: LGTM!The addition of
selectedWorktreeIdto the hook props is necessary for handling selection reset after deletion.Also applies to: 19-19
195-195: LGTM!The new handler is correctly exposed in the hook's API.
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx (7)
1-11: LGTM!The new Dialog component imports and React hooks are correctly added for the deletion confirmation flow.
29-30: LGTM!The new props correctly enable workspace-scoped worktree deletion.
34-38: LGTM!State management for the deletion dialog and tab width calculation is well-structured.
40-79: LGTM!The tab width calculation with ResizeObserver is well-implemented:
- Correctly accounts for gaps between tabs
- Applies sensible min/max constraints (60-240px)
- Properly cleans up the observer
- Has correct dependencies
85-119: LGTM!The close handler is well-implemented with:
- Proper event propagation control
- Guard clauses for pending worktrees and missing dependencies
- Pre-deletion check for uncommitted changes
- Graceful error handling (still shows dialog if check fails)
121-130: Verify error handling is sufficient.The function clears state and closes the dialog before the async
onDeleteWorktreecompletes. IfonDeleteWorktreefails, the user won't see the dialog anymore. However, sincehandleDeleteWorktreeinuseWorktrees.tsshows error alerts, this might be intentional.Consider whether you want to:
- Keep the current behavior (close dialog immediately, errors shown via alerts)
- Wait for the operation to complete before closing (better UX)
Example alternative (if desired):
const confirmRemoveWorktree = async () => { if (!onDeleteWorktree || !worktreeToDelete) return; - setShowRemoveDialog(false); - setRemoveWarning(""); const worktreeId = worktreeToDelete; - setWorktreeToDelete(null); - await onDeleteWorktree(worktreeId); + try { + await onDeleteWorktree(worktreeId); + } finally { + setShowRemoveDialog(false); + setRemoveWarning(""); + setWorktreeToDelete(null); + } };
132-241: LGTM!The JSX changes are well-implemented:
- Tab width is properly passed to
WorktreeTabcomponents- Close handlers are conditionally provided
- Confirmation dialog has clear messaging and appropriate styling for warnings
- Dialog accessibility is maintained with proper semantic structure
| // Handle worktree deletion | ||
| const handleDeleteWorktree = async (worktreeId: string) => { | ||
| if (!currentWorkspace) return; | ||
|
|
||
| try { | ||
| const result = await window.ipcRenderer.invoke("worktree-remove", { | ||
| workspaceId: currentWorkspace.id, | ||
| worktreeId, | ||
| }); | ||
|
|
||
| if (result.success) { | ||
| // Reload workspace to get updated worktree list | ||
| const refreshedWorkspace = await window.ipcRenderer.invoke( | ||
| "workspace-get", | ||
| currentWorkspace.id, | ||
| ); | ||
|
|
||
| if (refreshedWorkspace) { | ||
| setCurrentWorkspace(refreshedWorkspace); | ||
| await loadAllWorkspaces(); | ||
|
|
||
| // If we deleted the selected worktree, select the first available one | ||
| if (selectedWorktreeId === worktreeId) { | ||
| if (refreshedWorkspace.worktrees && refreshedWorkspace.worktrees.length > 0) { | ||
| const firstWorktree = refreshedWorkspace.worktrees[0]; | ||
| setSelectedWorktreeId(firstWorktree.id); | ||
| if (firstWorktree.tabs && firstWorktree.tabs.length > 0) { | ||
| handleTabSelect(firstWorktree.id, firstWorktree.tabs[0].id); | ||
| } else { | ||
| setSelectedTabId(null); | ||
| } | ||
| } else { | ||
| setSelectedWorktreeId(null); | ||
| setSelectedTabId(null); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| console.error("Failed to remove worktree:", result.error); | ||
| alert(`Failed to remove worktree: ${result.error || "Unknown error"}`); | ||
| } | ||
| } catch (error) { | ||
| console.error("Error removing worktree:", error); | ||
| alert(`Error: ${error instanceof Error ? error.message : String(error)}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Worktree deletion can revert to stale workspace state due to double setCurrentWorkspace.
In handleDeleteWorktree, after a successful delete you:
- call
setCurrentWorkspace(refreshedWorkspace); - then, when the deleted worktree was selected, you eventually call
handleTabSelect(firstWorktree.id, firstWorktree.tabs[0].id);
But handleTabSelect does:
if (currentWorkspace) {
window.ipcRenderer.invoke("workspace-set-active-selection", { ... });
setCurrentWorkspace({
...currentWorkspace,
activeWorktreeId: worktreeId,
activeTabId: tabId,
});
}Because both setCurrentWorkspace calls are plain object updates in the same event, the one inside handleTabSelect will win and is based on the stale currentWorkspace snapshot (pre‑deletion). That can reintroduce the deleted worktree into the in‑memory workspace state.
A minimal, safer fix here is to keep the fresh workspace and only adjust selection IDs, similar to the useWorktrees hook:
- if (selectedWorktreeId === worktreeId) {
- if (refreshedWorkspace.worktrees && refreshedWorkspace.worktrees.length > 0) {
- const firstWorktree = refreshedWorkspace.worktrees[0];
- setSelectedWorktreeId(firstWorktree.id);
- if (firstWorktree.tabs && firstWorktree.tabs.length > 0) {
- handleTabSelect(firstWorktree.id, firstWorktree.tabs[0].id);
- } else {
- setSelectedTabId(null);
- }
- } else {
- setSelectedWorktreeId(null);
- setSelectedTabId(null);
- }
- }
+ if (selectedWorktreeId === worktreeId) {
+ if (refreshedWorkspace.worktrees && refreshedWorkspace.worktrees.length > 0) {
+ const firstWorktree = refreshedWorkspace.worktrees[0];
+ setSelectedWorktreeId(firstWorktree.id);
+ if (firstWorktree.tabs && firstWorktree.tabs.length > 0) {
+ setSelectedTabId(firstWorktree.tabs[0].id);
+ } else {
+ setSelectedTabId(null);
+ }
+ } else {
+ setSelectedWorktreeId(null);
+ setSelectedTabId(null);
+ }
+ }(Optionally, if you want the main process’s active selection kept in sync, you can inline the workspace-set-active-selection IPC call here using refreshedWorkspace.id.)
The wiring of onDeleteWorktree={handleDeleteWorktree} and workspaceId={currentWorkspace?.id} into TaskTabs looks correct and matches the new props.
Also applies to: 913-945
🏁 Script executed:
#!/bin/bash
# Locate the file and examine the handleDeleteWorktree function
find . -name "NewLayoutMain.tsx" -type f | head -1Length of output: 76
🏁 Script executed:
#!/bin/bash
# View the file to get the full context around the mentioned lines
if [ -f "apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx" ]; then
sed -n '495,555p' "apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx"
else
echo "File not found at expected path"
fiLength of output: 1809
🏁 Script executed:
#!/bin/bash
# Find handleTabSelect definition to understand what it does
if [ -f "apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx" ]; then
rg -n "handleTabSelect\s*=" "apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx" -A 20
fiLength of output: 584
🏁 Script executed:
#!/bin/bash
# Check lines 913-945 mentioned in the review
if [ -f "apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx" ]; then
sed -n '910,950p' "apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx"
fiLength of output: 1374
Replace handleTabSelect call with direct setSelectedTabId to prevent stale workspace state from overriding deletion.
The issue is confirmed. After successful worktree deletion:
setCurrentWorkspace(refreshedWorkspace)is called at line 525handleTabSelect()is called at line 533, which internally callssetCurrentWorkspace({ ...currentWorkspace, ... })using a stale closure value- Both state updates batch together in the same event handler, and the stale one wins, reintroducing deleted data
Apply the suggested minimal fix by replacing the handleTabSelect call with direct selection setters:
- if (firstWorktree.tabs && firstWorktree.tabs.length > 0) {
- handleTabSelect(firstWorktree.id, firstWorktree.tabs[0].id);
- } else {
- setSelectedTabId(null);
- }
+ if (firstWorktree.tabs && firstWorktree.tabs.length > 0) {
+ setSelectedTabId(firstWorktree.tabs[0].id);
+ } else {
+ setSelectedTabId(null);
+ }(Optionally inline the workspace-set-active-selection IPC call here using refreshedWorkspace.id if needed to keep the main process in sync.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Handle worktree deletion | |
| const handleDeleteWorktree = async (worktreeId: string) => { | |
| if (!currentWorkspace) return; | |
| try { | |
| const result = await window.ipcRenderer.invoke("worktree-remove", { | |
| workspaceId: currentWorkspace.id, | |
| worktreeId, | |
| }); | |
| if (result.success) { | |
| // Reload workspace to get updated worktree list | |
| const refreshedWorkspace = await window.ipcRenderer.invoke( | |
| "workspace-get", | |
| currentWorkspace.id, | |
| ); | |
| if (refreshedWorkspace) { | |
| setCurrentWorkspace(refreshedWorkspace); | |
| await loadAllWorkspaces(); | |
| // If we deleted the selected worktree, select the first available one | |
| if (selectedWorktreeId === worktreeId) { | |
| if (refreshedWorkspace.worktrees && refreshedWorkspace.worktrees.length > 0) { | |
| const firstWorktree = refreshedWorkspace.worktrees[0]; | |
| setSelectedWorktreeId(firstWorktree.id); | |
| if (firstWorktree.tabs && firstWorktree.tabs.length > 0) { | |
| handleTabSelect(firstWorktree.id, firstWorktree.tabs[0].id); | |
| } else { | |
| setSelectedTabId(null); | |
| } | |
| } else { | |
| setSelectedWorktreeId(null); | |
| setSelectedTabId(null); | |
| } | |
| } | |
| } | |
| } else { | |
| console.error("Failed to remove worktree:", result.error); | |
| alert(`Failed to remove worktree: ${result.error || "Unknown error"}`); | |
| } | |
| } catch (error) { | |
| console.error("Error removing worktree:", error); | |
| alert(`Error: ${error instanceof Error ? error.message : String(error)}`); | |
| } | |
| }; | |
| // Handle worktree deletion | |
| const handleDeleteWorktree = async (worktreeId: string) => { | |
| if (!currentWorkspace) return; | |
| try { | |
| const result = await window.ipcRenderer.invoke("worktree-remove", { | |
| workspaceId: currentWorkspace.id, | |
| worktreeId, | |
| }); | |
| if (result.success) { | |
| // Reload workspace to get updated worktree list | |
| const refreshedWorkspace = await window.ipcRenderer.invoke( | |
| "workspace-get", | |
| currentWorkspace.id, | |
| ); | |
| if (refreshedWorkspace) { | |
| setCurrentWorkspace(refreshedWorkspace); | |
| await loadAllWorkspaces(); | |
| // If we deleted the selected worktree, select the first available one | |
| if (selectedWorktreeId === worktreeId) { | |
| if (refreshedWorkspace.worktrees && refreshedWorkspace.worktrees.length > 0) { | |
| const firstWorktree = refreshedWorkspace.worktrees[0]; | |
| setSelectedWorktreeId(firstWorktree.id); | |
| if (firstWorktree.tabs && firstWorktree.tabs.length > 0) { | |
| setSelectedTabId(firstWorktree.tabs[0].id); | |
| } else { | |
| setSelectedTabId(null); | |
| } | |
| } else { | |
| setSelectedWorktreeId(null); | |
| setSelectedTabId(null); | |
| } | |
| } | |
| } | |
| } else { | |
| console.error("Failed to remove worktree:", result.error); | |
| alert(`Failed to remove worktree: ${result.error || "Unknown error"}`); | |
| } | |
| } catch (error) { | |
| console.error("Error removing worktree:", error); | |
| alert(`Error: ${error instanceof Error ? error.message : String(error)}`); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx
around lines 501 to 546, after setting setCurrentWorkspace(refreshedWorkspace)
the code calls handleTabSelect which uses a stale closure of currentWorkspace
and can reintroduce deleted data; replace the handleTabSelect(...) call with
direct state setters (setSelectedWorktreeId(firstWorktree.id) and
setSelectedTabId(firstWorktree.tabs[0].id)) and, if required, invoke the
workspace-set-active-selection IPC directly with refreshedWorkspace.id to keep
main-process selection in sync instead of calling handleTabSelect.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx (1)
62-83: Add focus visibility to the close button for keyboard users.The close button remains invisible when focused via keyboard because it only shows on
group-hover:opacity-100. Keyboard users navigating with Tab cannot see the button even though they can interact with it.Apply the suggested fix to reveal the button on keyboard focus:
<button type="button" onClick={(e) => { e.stopPropagation(); onClose(e); }} className={` absolute right-1 top-1/2 -translate-y-1/2 flex items-center justify-center w-4 h-4 rounded - opacity-0 group-hover:opacity-100 + opacity-0 group-hover:opacity-100 group-focus-within:opacity-100 focus:opacity-100 transition-opacity hover:bg-neutral-700 + focus:outline-none focus:ring-2 focus:ring-blue-500 text-neutral-400 hover:text-neutral-200 ${isSelected ? "text-neutral-300" : ""} `} aria-label="Close tab" > <X size={12} /> </button>
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx (1)
85-119: Consider using finally block for consistent state cleanup.The state cleanup logic is duplicated in both the success path (lines 109-111) and the error catch block (lines 115-117). This could lead to inconsistencies if the logic diverges in the future.
Apply this diff to consolidate state management:
const handleCloseClick = async (e: React.MouseEvent, worktreeId: string) => { e.stopPropagation(); if (!onDeleteWorktree || !workspaceId) return; const worktree = worktrees.find((wt) => wt.id === worktreeId); if (!worktree || worktree.isPending) return; + let warning = ""; + // Check if the worktree has uncommitted changes try { const canRemoveResult = await window.ipcRenderer.invoke( "worktree-can-remove", { workspaceId, worktreeId, }, ); // Build warning message if there are uncommitted changes - let warning = ""; if (canRemoveResult.hasUncommittedChanges) { warning = `Warning: This worktree (${worktree.branch}) has uncommitted changes. Removing it will delete these changes permanently.`; } - - setRemoveWarning(warning); - setWorktreeToDelete(worktreeId); - setShowRemoveDialog(true); } catch (error) { console.error("Error checking if worktree can be removed:", error); - // Still show dialog even if check fails - setRemoveWarning(""); - setWorktreeToDelete(worktreeId); - setShowRemoveDialog(true); + } finally { + setRemoveWarning(warning); + setWorktreeToDelete(worktreeId); + setShowRemoveDialog(true); } };apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx (1)
26-29: Consider handling overflow behavior for the wrapper.The wrapper div uses
shrink-0which prevents it from shrinking, but when combined with a fixed width, this could cause layout issues if the container is narrower than expected. Consider whetheroverflow-hiddenor similar constraints are needed.If overflow issues occur, apply this diff:
<div - className="group relative flex items-end shrink-0" + className="group relative flex items-end shrink-0 overflow-hidden" style={{ width: width ? `${width}px` : undefined }} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx (5)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/SidebarToggle.tsx (1)
SidebarToggle(12-42)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/ModeToggle.tsx (1)
ModeToggle(8-33)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTab.tsx (1)
WorktreeTab(21-54)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/AddTaskButton.tsx (1)
AddTaskButton(10-21)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/PRActions.tsx (1)
PRActions(14-69)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx (1)
apps/desktop/src/renderer/screens/main/components/Layout/StatusIndicator.tsx (1)
StatusIndicator(55-115)
🔇 Additional comments (3)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx (2)
198-238: LGTM! Dialog implementation follows accessibility best practices.The confirmation dialog uses semantic DialogHeader, DialogTitle, and DialogDescription components, providing proper context for screen readers. The warning message is clearly differentiated with visual styling and the destructive action is appropriately styled with the "destructive" variant.
41-79: ****The calculation is correct. The offsetWidth property returns the viewable width of an element including padding and border, so any padding on the container is already accounted for. The tabs container's className has no padding classes, and flex-1 allows a flex item to grow and shrink as needed—combined with min-w-0, this ensures the tabs container only receives the space available after siblings (like AddTaskButton) are sized. The flex layout automatically handles this distribution, making the offsetWidth measurement accurate for tab width calculations. The ResizeObserver setup and cleanup are properly implemented.
Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx (1)
30-61: LGTM! Dynamic padding adjustment handles the close button spacing.The conditional padding (
onClose && !isPending ? "pl-3 pr-6" : "px-3") correctly reserves space for the absolutely positioned close button, preventing text overlap. The width constraints and truncation ensure the tab label degrades gracefully when space is limited.
| const confirmRemoveWorktree = async () => { | ||
| if (!onDeleteWorktree || !worktreeToDelete) return; | ||
|
|
||
| setShowRemoveDialog(false); | ||
| setRemoveWarning(""); | ||
| const worktreeId = worktreeToDelete; | ||
| setWorktreeToDelete(null); | ||
|
|
||
| await onDeleteWorktree(worktreeId); | ||
| }; |
There was a problem hiding this comment.
Handle errors from the deletion operation.
confirmRemoveWorktree calls onDeleteWorktree but doesn't catch potential errors. If the deletion fails, the user won't be notified and the UI state will be inconsistent.
Apply this diff to add error handling:
const confirmRemoveWorktree = async () => {
if (!onDeleteWorktree || !worktreeToDelete) return;
setShowRemoveDialog(false);
- setRemoveWarning("");
const worktreeId = worktreeToDelete;
- setWorktreeToDelete(null);
- await onDeleteWorktree(worktreeId);
+ try {
+ await onDeleteWorktree(worktreeId);
+ } catch (error) {
+ console.error("Error deleting worktree:", error);
+ // TODO: Show error notification to user
+ } finally {
+ setRemoveWarning("");
+ setWorktreeToDelete(null);
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const confirmRemoveWorktree = async () => { | |
| if (!onDeleteWorktree || !worktreeToDelete) return; | |
| setShowRemoveDialog(false); | |
| setRemoveWarning(""); | |
| const worktreeId = worktreeToDelete; | |
| setWorktreeToDelete(null); | |
| await onDeleteWorktree(worktreeId); | |
| }; | |
| const confirmRemoveWorktree = async () => { | |
| if (!onDeleteWorktree || !worktreeToDelete) return; | |
| setShowRemoveDialog(false); | |
| const worktreeId = worktreeToDelete; | |
| try { | |
| await onDeleteWorktree(worktreeId); | |
| } catch (error) { | |
| console.error("Error deleting worktree:", error); | |
| // TODO: Show error notification to user | |
| } finally { | |
| setRemoveWarning(""); | |
| setWorktreeToDelete(null); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx
around lines 121-130, confirmRemoveWorktree awaits onDeleteWorktree without
handling errors; wrap the await in a try/catch, call onDeleteWorktree inside
try, and on success clear dialog state as now; in catch set an informative
user-visible error (e.g. setRemoveWarning(error.message || "Failed to delete
worktree")), restore or keep worktreeToDelete and re-open the remove dialog
(setShowRemoveDialog(true)) so UI remains consistent, and log the error for
debugging.
1 participant
Summary by CodeRabbit