Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe PR refactors worktree creation handling by introducing a return value from Changes
Sequence DiagramsequenceDiagram
participant MainScreen
participant useTasks
participant useWorktrees
participant API as IPC/Backend
rect rgb(200, 220, 240)
Note over MainScreen,API: Before: handleWorktreeCreated returns void
MainScreen->>useTasks: handleWorktreeCreated (Promise<void>)
useTasks->>useWorktrees: handleWorktreeCreated()
useWorktrees->>API: Create worktree
API-->>useWorktrees: Success
useWorktrees->>useWorktrees: Update state
useWorktrees-->>useTasks: void
useTasks->>API: Re-fetch latestWorkspace (redundant)
API-->>useTasks: Workspace
useTasks-->>MainScreen: void
end
rect rgb(220, 240, 200)
Note over MainScreen,API: After: handleWorktreeCreated returns refreshed workspace
MainScreen->>useTasks: handleWorktreeCreatedWithResult()
useTasks->>useWorktrees: handleWorktreeCreated()
useWorktrees->>API: Create worktree
API-->>useWorktrees: Success
useWorktrees->>useWorktrees: Update state
useWorktrees-->>useTasks: Workspace | null (refreshedWorkspace)
useTasks->>useTasks: Wait for state propagation
useTasks-->>MainScreen: refreshedWorkspace
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas requiring extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Comment |
Kitenite
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/desktop/src/renderer/screens/main/hooks/useTabs.ts (1)
37-61: Functional updater in handleTabCreated looks correct; outer null check is redundantThe functional
setCurrentWorkspacehere correctly appends the tab and updatesactiveWorktreeId/activeTabIdwithout risking stale closures. The outerif (!currentWorkspace) return;is now redundant since the updater already guards onprev; you can simplify if you want slightly clearer logic.- const handleTabCreated = (worktreeId: string, tab: Tab) => { - if (!currentWorkspace) return; - - // Use functional setState to avoid stale closures - setCurrentWorkspace((prev) => { - if (!prev) return prev; + const handleTabCreated = (worktreeId: string, tab: Tab) => { + // Use functional setState to avoid stale closures + setCurrentWorkspace((prev) => { + if (!prev) return prev; @@ - }); - }; + }); + };apps/desktop/src/renderer/screens/main/hooks/useTasks.ts (2)
7-15: Prop types for currentWorkspace/handleWorktreeCreated are sound but could reuse shared workspace typingThe narrowed shapes for
currentWorkspaceandhandleWorktreeCreated’s return type are sufficient for this hook and remain structurally compatible withWorkspace. If you want to keep these in sync with the shared model, consider expressing them via aPick<Workspace, "id" | "worktrees">alias instead of duplicating the inline object shape.-import type { Worktree } from "shared/types"; +import type { Worktree, Workspace } from "shared/types"; @@ -interface UseTasksProps { - currentWorkspace: { - id: string; - worktrees?: Worktree[]; - } | null; +interface UseTasksProps { + currentWorkspace: Pick<Workspace, "id" | "worktrees"> | null; @@ - handleWorktreeCreated: () => Promise<{ id: string; worktrees?: Worktree[] } | null>; + handleWorktreeCreated: () => Promise<Pick<Workspace, "id" | "worktrees"> | null>;
173-193: Avoid the extra 50ms timeout after handleWorktreeCreated; it’s unnecessary and brittleAwaiting
handleWorktreeCreated()already guarantees itssetCurrentWorkspaceupdate is enqueued before you callsetSelectedWorktreeId/handleTabSelect. The additionalsetTimeout(…, 50)only adds latency and couples correctness to timing, while the functional state updates inuseTabsare designed to compose without this delay.You can rely on the awaited call plus the returned
refreshedWorkspaceand drop the timeout.- // handleWorktreeCreated returns the refreshed workspace directly - const refreshedWorkspace = await handleWorktreeCreated(); - - // Wait for React to process the state update from handleWorktreeCreated - // This ensures handleTabSelect sees the updated workspace in its functional setState - await new Promise((resolve) => setTimeout(resolve, 50)); + // handleWorktreeCreated returns the refreshed workspace directly + const refreshedWorkspace = await handleWorktreeCreated(); @@ - // Use the refreshed workspace returned by handleWorktreeCreated - if (result.worktree && refreshedWorkspace) { + // Use the refreshed workspace returned by handleWorktreeCreated + if (result.worktree && refreshedWorkspace) { // Find the worktree by branch name to get the correct ID const newWorktree = refreshedWorkspace.worktrees?.find( (wt) => wt.branch === result.worktree?.branch, );apps/desktop/src/renderer/screens/main/MainScreen.tsx (1)
116-127: Consider memoizing WorkspaceOperationsProvider value to avoid unnecessary context recalculationsWrapping the tree in
WorkspaceOperationsProvideris a good way to centralize workspace/worktree operations. Right now thevalueobject is recreated on everyMainScreenrender, which will force alluseWorkspaceOperationsconsumers to re-render even when the underlying handlers/state haven’t changed.Not urgent, but you can optionally wrap the value in
useMemoto stabilize it.-import { useState } from "react"; +import { useMemo, useState } from "react"; @@ - const { - handleWorktreeCreated, - handleWorktreeCreatedWithResult, - handleUpdateWorktree, - handleCreatePR, - handleMergePR, - handleDeleteWorktree, - } = useWorktrees({ + const { + handleWorktreeCreated, + handleWorktreeCreatedWithResult, + handleUpdateWorktree, + handleCreatePR, + handleMergePR, + handleDeleteWorktree, + } = useWorktrees({ @@ - const { + const { @@ - } = useTasks({ + } = useTasks({ @@ - return ( - <WorkspaceOperationsProvider - value={{ - handleWorktreeCreated, - handleUpdateWorktree, - handleCreatePR, - handleMergePR, - handleDeleteWorktree, - currentWorkspace, - workspaces, - handleWorkspaceSelect, - loadAllWorkspaces, - }} - > + const workspaceOperationsValue = useMemo( + () => ({ + handleWorktreeCreated, + handleUpdateWorktree, + handleCreatePR, + handleMergePR, + handleDeleteWorktree, + currentWorkspace, + workspaces, + handleWorkspaceSelect, + loadAllWorkspaces, + }), + [ + handleWorktreeCreated, + handleUpdateWorktree, + handleCreatePR, + handleMergePR, + handleDeleteWorktree, + currentWorkspace, + workspaces, + handleWorkspaceSelect, + loadAllWorkspaces, + ], + ); + + return ( + <WorkspaceOperationsProvider value={workspaceOperationsValue}> @@ - {workspaces && ( + {workspaces && ( @@ - )} - </WorkspaceOperationsProvider> + )} + </WorkspaceOperationsProvider> );Also applies to: 243-243
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src/renderer/screens/main/MainScreen.tsx(4 hunks)apps/desktop/src/renderer/screens/main/contexts/WorkspaceOperationsContext.tsx(1 hunks)apps/desktop/src/renderer/screens/main/hooks/useTabs.ts(4 hunks)apps/desktop/src/renderer/screens/main/hooks/useTasks.ts(2 hunks)apps/desktop/src/renderer/screens/main/hooks/useWorktrees.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
apps/desktop/src/renderer/screens/main/hooks/useTabs.ts (1)
apps/desktop/src/shared/types.ts (1)
Workspace(70-82)
apps/desktop/src/renderer/screens/main/contexts/WorkspaceOperationsContext.tsx (1)
apps/desktop/src/shared/types.ts (2)
Worktree(58-68)Workspace(70-82)
apps/desktop/src/renderer/screens/main/hooks/useTasks.ts (1)
apps/desktop/src/shared/types.ts (1)
Worktree(58-68)
apps/desktop/src/renderer/screens/main/MainScreen.tsx (1)
apps/desktop/src/renderer/screens/main/contexts/WorkspaceOperationsContext.tsx (1)
WorkspaceOperationsProvider(37-43)
apps/desktop/src/renderer/screens/main/hooks/useWorktrees.ts (1)
apps/desktop/src/shared/types.ts (1)
Workspace(70-82)
🔇 Additional comments (7)
apps/desktop/src/renderer/screens/main/hooks/useTabs.ts (2)
1-8: setCurrentWorkspace typed as React.Dispatch is aligned with React state usageTyping
setCurrentWorkspaceasReact.Dispatch<React.SetStateAction<Workspace | null>>is a good change and matches how you’re now using functional updates throughout this hook. No issues from a typing or usage perspective.
65-85: Selection/focus handlers now compose correctly with concurrent workspace updatesUsing functional
setCurrentWorkspacein bothhandleTabSelectandhandleTabFocusensures that active worktree/tab updates compose safely with other callers (like workspace refresh) without clobbering concurrent changes. The IPC call still only depends on the workspace id, so this refactor is behavior‑preserving.Also applies to: 89-107
apps/desktop/src/renderer/screens/main/MainScreen.tsx (1)
75-83: Clean separation of void vs resultful worktree-creation handlersUsing
handleWorktreeCreatedfor the Promise surface andhandleWorktreeCreatedWithResultfor callers that need the refreshed workspace (useTasks) is a clear separation that keeps the context API simple while still enabling richer flows in hooks. The mappinghandleWorktreeCreated: handleWorktreeCreatedWithResultinuseTasksmatches the updated hook signatures.Also applies to: 108-113
apps/desktop/src/renderer/screens/main/hooks/useWorktrees.ts (3)
4-7: setCurrentWorkspace typing matches new functional update usageUpdating
setCurrentWorkspacetoReact.Dispatch<React.SetStateAction<Workspace | null>>is consistent with the functional updates you’re now doing inhandleWorktreeCreatedand keeps this hook aligned withuseWorkspace/useTabs. No issues here.
24-59: handleWorktreeCreated’s refreshed-workspace return and guarded functional update look solidThe new
handleWorktreeCreatedimplementation:
- Safely bails out when there’s no
currentWorkspace.- Uses a workspaceId snapshot plus a functional
setCurrentWorkspaceto avoid races when the current workspace changes.- Clones the
worktreesarray to ensure React notices the structural change.- Returns the
refreshedWorkspace(ornullon failure), which matches howuseTasksconsumes it.This is a good balance between correctness and ergonomics; no changes needed.
210-218: Wrapper/alias split between void and resultful handlers is clean
handleWorktreeCreatedVoidis a simple wrapper that preserves the existing Promise contract for components/context, while exposinghandleWorktreeCreatedWithResultfor callers that need the refreshed workspace. This keeps public APIs stable without duplicating logic.apps/desktop/src/renderer/screens/main/contexts/WorkspaceOperationsContext.tsx (1)
5-43: WorkspaceOperationsContext surface and hook/provider pattern look correctThe
WorkspaceOperationsContextValueinterface matches whatMainScreenprovides, anduseWorkspaceOperationscorrectly throws if used outsideWorkspaceOperationsProvider. The provider itself is a straightforward wrapper aroundcreateContextanduseContext, which should make workspace operations easy to consume across the tree.
1 participant
Summary by CodeRabbit