updated the new workspace stuff and added clone from url#82
updated the new workspace stuff and added clone from url#82
Conversation
WalkthroughAdds Git clone support via new IPC handlers and a WorktreeManager clone method, introduces a CloneFromUrlDialog and workspace-selection UI changes, extends workspace IPC channel types, and applies widespread formatting/whitespace cleanups across many files. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CloneDialog as CloneFromUrlDialog
participant Renderer
participant IPC as Main Process (ipcMain)
participant Worktree as WorktreeManager
participant WorkspaceStore as Workspace creation
User->>CloneDialog: Enter URL + destination, submit
CloneDialog->>Renderer: validate inputs
CloneDialog->>IPC: invoke "workspace-clone-from-url" with {url,destinationPath}
IPC->>Worktree: call cloneRepository(url,destinationPath)
alt clone success
Worktree->>WorkspaceStore: create or reuse workspace
WorkspaceStore-->>IPC: workspace data
IPC-->>CloneDialog: { success: true, data: Workspace }
CloneDialog->>User: close, select workspace
else clone failure
Worktree-->>IPC: { success: false, error }
IPC-->>CloneDialog: { success: false, error }
CloneDialog->>User: display error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas to focus review on:
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 (7)
apps/desktop/src/renderer/screens/main/utils.ts (2)
7-27: Tab lookup helper matches current tab model
findTabRecursivecorrectly:
- guards against
undefinedtab arrays, and- searches top-level tabs plus one level of
type === "group"children, returning the parent for child matches.Given the current
Tabmodel explicitly disallows nested group tabs, this implementation is sufficient and straightforward.If you ever relax that constraint and allow deeper nesting, this would be a natural spot to refactor into a truly recursive search to avoid duplicating the child-loop pattern elsewhere.
30-87: Worktree enrichment logic is sound; consider stabilizing pending metadataThe enrichment flow looks good:
- Pending worktrees are projected into
WorktreeWithTaskwithisPending: true, emptytabs, and optionaltaskderived frompending.taskData.- Real worktrees are matched by
branchagainstMOCK_TASKS, and enriched with ataskthat includes assignee metadata when found.- The final merged array preserves type consistency by returning both pending and real worktrees as
WorktreeWithTask.Two minor, optional considerations:
createdAtfor pending worktrees is recomputed withnew Date().toISOString()on every call, so if you ever surface this timestamp in the UI or use it for stable sorting, you may want to store it onPendingWorktreeor pass it in rather than regenerating it.- If branches can collide across environments, you might eventually want a more robust task–worktree association (e.g., explicit task ID on the worktree), but for MOCK_TASK-based enrichment this is fine.
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/CloneFromUrlDialog.tsx (1)
31-55: Tighten input handling (trim values and clear errors on edit)The overall dialog looks solid and the IPC integration is straightforward. A couple of small tweaks would make the UX more robust and avoid subtle failures with copied URLs/paths that contain trailing spaces:
- Trim
urlanddestinationNamebefore validating and passing them toonClone, and use the trimmed values when deciding if the Clone button should be enabled.- Clear
errornot only when the URL changes, but also when the destination path is edited, so the user sees immediate feedback when correcting mistakes.Concretely, something like this would help:
const handleUrlChange = (newUrl: string) => { - setUrl(newUrl); + const trimmedUrl = newUrl.trim(); + setUrl(trimmedUrl); setError(null); // Auto-populate destination name from URL - const repoName = extractRepoName(newUrl); + const repoName = extractRepoName(trimmedUrl); if (repoName && !destinationName) { setDestinationName(repoName); } }; @@ <div className="flex gap-2"> <Input id="destination" @@ - value={destinationName} - onChange={(e) => setDestinationName(e.target.value)} + value={destinationName} + onChange={(e) => { + setDestinationName(e.target.value); + setError(null); + }} @@ - const handleClone = async () => { - if (!url || !destinationName) { + const handleClone = async () => { + const trimmedUrl = url.trim(); + const trimmedDestination = destinationName.trim(); + + if (!trimmedUrl || !trimmedDestination) { setError("Please provide both URL and destination name"); return; } @@ - try { - await onClone(url, destinationName); + try { + await onClone(trimmedUrl, trimmedDestination); @@ <Button onClick={handleClone} - disabled={isLoading || !url || !destinationName} + disabled={ + isLoading || !url.trim() || !destinationName.trim() + }This keeps the current behavior but avoids treating whitespace-only inputs as valid and provides clearer feedback as the user fixes input errors.
Also applies to: 56-74, 76-99, 117-124, 132-139, 174-179
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx (2)
30-53: IPC response handling inhandleCloneFromUrllooks correct, but consider typing the responseThe success/error branching and rethrowing here works well with
CloneFromUrlDialog’sonClonecontract (errors bubble up to the dialog for display, success selects the workspace). To avoidany-shaped IPC responses getting out of sync with the main process, consider adding a shared TS type for the"workspace-clone-from-url"response and asserting/castingresultto it at the call site, e.g. a{ success: boolean; data?: Workspace; error?: string }type reused from the IPC channel definition.
59-151: Modal is now effectively non-dismissible; confirm this UX is intentionalWith
open={isOpen},onOpenChange={() => {}}, andshowCloseButton={false}, the workspace selection modal can’t be closed via Esc, backdrop click, or a close button; only the parent can changeisOpen. If users should be able to dismiss this screen, wireonOpenChangeto a parent close handler instead (or accept anonOpenChangeprop and pass it through). If it’s intentionally “blocking until workspace chosen”, this is fine as-is.apps/desktop/src/main/lib/workspace-ipcs.ts (2)
84-104: Directory-select IPC matches renderer contract; watch out for downstream path joiningThe shape
{ canceled: boolean; filePath?: string }aligns with howCloneFromUrlDialoguses the result. However, the renderer currently constructs the final destination with string concatenation (${result.filePath}/${repoName}), which can produce mixed separators on Windows. Consider moving the “append repo name to directory” logic into this handler (using Node’spath.join) or introducing a shared helper, so all path composition happens on the main process with proper cross-platform handling.
106-179: Clone-from-URL flow is solid; improve repo name derivation and reuse existing logicThe sequence—clone, validate git repo, determine current branch, reuse existing workspace if any, otherwise create and return one—is sound and returns a clean
{ success, data, error }envelope expected by the renderer.Two improvement points:
Cross-platform repo name
const repoName = repoPath.split("/").pop() || "Repository";will not behave well on Windows paths. Prefer Node’spath.basename(and ideally reuse the same helper wherever you derive repo names). For example:import path from "path"; // at top of file const repoName = path.basename(repoPath) || "Repository";Share typing with IPC definition
If you have a shared IPC response type for"workspace-clone-from-url", return that type explicitly here and in the renderer, rather than an ad-hoc object, to avoid future drift between main and renderer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (74)
apps/desktop/src/main/lib/window-manager.ts(4 hunks)apps/desktop/src/main/lib/window-state-manager.ts(1 hunks)apps/desktop/src/main/lib/workspace-ipcs.ts(3 hunks)apps/desktop/src/main/lib/worktree-manager.ts(2 hunks)apps/desktop/src/renderer/components/ui/dialog.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/AppFrame/AppFrame.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/DiffView/DiffContent.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/DiffView/DiffContentArea.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/CreatingView.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/TaskForm.tsx(4 hunks)apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/TaskList.tsx(4 hunks)apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/types.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/useTaskData.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/useTaskForm.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/utils.ts(2 hunks)apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx(8 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/AddTaskButton.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/ModeToggle.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx(4 hunks)apps/desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/MainContentArea/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/ModeCarousel.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/AnimatedBackground.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeHeader/ModeHeader.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeHeader/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/constants.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useModeDetection.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useScrollProgress.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useScrollSnap.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/index.ts(1 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeSwitcher/ModeSwitcher.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeSwitcher/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/WorktreeList.tsx(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/NewTabButton/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx(5 hunks)apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/components/TabItem/TabItem.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/SidebarOverlay/SidebarOverlay.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/SidebarOverlay/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/CloneFromUrlDialog.tsx(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx(3 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/index.ts(0 hunks)apps/desktop/src/renderer/screens/main/constants.ts(1 hunks)apps/desktop/src/renderer/screens/main/hooks/useDiffData.ts(2 hunks)apps/desktop/src/renderer/screens/main/hooks/useSidebar.ts(0 hunks)apps/desktop/src/renderer/screens/main/hooks/useTabs.ts(0 hunks)apps/desktop/src/renderer/screens/main/hooks/useTasks.ts(6 hunks)apps/desktop/src/renderer/screens/main/hooks/useWorkspace.ts(1 hunks)apps/desktop/src/renderer/screens/main/hooks/useWorktrees.ts(1 hunks)apps/desktop/src/renderer/screens/main/types.ts(1 hunks)apps/desktop/src/renderer/screens/main/utils.ts(1 hunks)apps/desktop/src/shared/ipc-channels.ts(0 hunks)apps/desktop/src/shared/ipc-channels/deep-link.ts(0 hunks)apps/desktop/src/shared/ipc-channels/external.ts(0 hunks)apps/desktop/src/shared/ipc-channels/index.ts(1 hunks)apps/desktop/src/shared/ipc-channels/proxy.ts(0 hunks)apps/desktop/src/shared/ipc-channels/tab.ts(1 hunks)apps/desktop/src/shared/ipc-channels/terminal.ts(0 hunks)apps/desktop/src/shared/ipc-channels/types.ts(0 hunks)apps/desktop/src/shared/ipc-channels/window.ts(0 hunks)apps/desktop/src/shared/ipc-channels/workspace.ts(1 hunks)apps/desktop/src/shared/ipc-channels/worktree.ts(0 hunks)apps/website/src/app/components/HeroSection/HeroSection.tsx(1 hunks)apps/website/src/app/components/HeroSection/components/HeroCanvas/components/LitBackground/LitBackground.tsx(1 hunks)apps/website/src/app/page.tsx(1 hunks)packages/ui/src/components/dropdown-menu.tsx(1 hunks)
💤 Files with no reviewable changes (27)
- apps/desktop/src/renderer/screens/main/hooks/useTabs.ts
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/WorktreeList.tsx
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts
- apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/index.ts
- apps/desktop/src/renderer/screens/main/hooks/useSidebar.ts
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeHeader/index.ts
- apps/desktop/src/renderer/screens/main/components/MainContentArea/index.ts
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/index.ts
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/index.ts
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/index.ts
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/NewTabButton/index.ts
- apps/desktop/src/shared/ipc-channels/deep-link.ts
- apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/useTaskData.ts
- apps/desktop/src/shared/ipc-channels/window.ts
- apps/desktop/src/shared/ipc-channels/external.ts
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useScrollProgress.ts
- apps/desktop/src/shared/ipc-channels/worktree.ts
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeHeader/ModeHeader.tsx
- apps/desktop/src/shared/ipc-channels.ts
- apps/desktop/src/shared/ipc-channels/types.ts
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/constants.ts
- apps/desktop/src/shared/ipc-channels/proxy.ts
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeSwitcher/ModeSwitcher.tsx
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useScrollSnap.ts
- apps/desktop/src/shared/ipc-channels/terminal.ts
- apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeSwitcher/index.ts
- apps/desktop/src/renderer/screens/main/components/SidebarOverlay/index.ts
🧰 Additional context used
🧬 Code graph analysis (8)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/ModeCarousel.tsx (1)
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeContent/ModeContent.tsx (1)
ModeContent(11-24)
apps/desktop/src/renderer/screens/main/utils.ts (3)
apps/desktop/src/shared/types.ts (2)
Tab(44-56)Worktree(58-68)apps/desktop/src/renderer/screens/main/types.ts (1)
PendingWorktree(17-28)apps/desktop/src/renderer/screens/main/constants.ts (1)
MOCK_TASKS(4-178)
apps/desktop/src/shared/ipc-channels/workspace.ts (2)
apps/desktop/src/shared/ipc-channels/types.ts (2)
IpcResponse(8-12)NoRequest(17-17)apps/desktop/src/shared/types.ts (1)
Workspace(70-82)
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/CloneFromUrlDialog.tsx (1)
CloneFromUrlDialog(21-184)
apps/desktop/src/renderer/components/ui/dialog.tsx (1)
apps/desktop/src/renderer/lib/utils.ts (1)
cn(4-6)
apps/desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx (6)
apps/desktop/src/renderer/screens/main/types.ts (1)
AppMode(30-30)apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/types.ts (1)
SidebarMode(4-4)apps/desktop/src/renderer/screens/main/hooks/useDiffData.ts (1)
useDiffData(12-199)apps/desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx (1)
Sidebar(25-411)apps/desktop/src/renderer/screens/main/components/PlaceholderState.tsx (1)
PlaceholderState(7-44)apps/desktop/src/renderer/screens/main/components/DiffView/DiffContentArea.tsx (1)
DiffContentArea(27-373)
apps/desktop/src/main/lib/workspace-ipcs.ts (1)
apps/desktop/src/main/lib/workspace-manager.ts (1)
workspaceManager(481-481)
apps/desktop/src/renderer/screens/main/hooks/useTasks.ts (1)
apps/desktop/src/renderer/screens/main/types.ts (1)
PendingWorktree(17-28)
🔇 Additional comments (54)
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/AddTaskButton.tsx (1)
18-23: Clean formatting restructuring with no functional changes.The Button component props are now distributed across lines for improved readability. The change is purely stylistic—all props and event handlers remain identical, and component behavior is unaffected.
apps/website/src/app/components/HeroSection/HeroSection.tsx (1)
1-3: Import reordering is unrelated to PR objectives.The import statement for
HeroParallaxon line 3 has been reordered, but this change appears unrelated to the PR's stated goal of adding workspace cloning and Git functionality. While the import reordering itself has no functional impact, it introduces minor scope creep. Consider reverting this change to keep the PR focused on its core objectives, or clarify in the PR description if import organization is part of the intended changes.apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal.tsx (1)
230-230: LGTM!The formatting adjustments to the ternary conditional and prop multiline presentation are purely cosmetic with no functional impact. The component logic, state management, and prop passing remain unchanged and correct.
Also applies to: 252-254
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/WorktreeItem.tsx (1)
15-15: Formatting and indentation improvements look good.The reformatted imports and adjusted multi-line template strings for classNames improve code readability and consistency. These purely cosmetic changes maintain the same runtime behavior and logic.
Also applies to: 198-204, 263-265, 747-747, 893-893
apps/desktop/src/renderer/screens/main/components/AppFrame/AppFrame.tsx (1)
6-6: Consider the purpose and scope of this formatting change.Line 6 converts the multi-line JSX return to a single line. While the output is functionally identical, this is purely cosmetic and unrelated to the PR's stated objective of adding Git cloning and workspace features.
Verify:
- Whether your project's style guide mandates single-line JSX for simple components, or if multi-line is preferred for readability.
- Whether the resulting line length complies with your linter's line-length threshold (the className attribute is non-trivial).
If there is no style rationale, consider reverting this to reduce formatting noise and keep the diff focused on feature work.
apps/desktop/src/renderer/screens/main/constants.ts (1)
5-177: Formatting change: tabs instead of spaces.This is a whitespace-only reformatting of the MOCK_TASKS array with no functional impact.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/WorktreeList/components/WorktreeItem/components/TabItem/TabItem.tsx (1)
141-147: Formatting-only refactor; behavior and styles preservedThe ternary logic and the emitted class strings are unchanged; only the string interpolation is wrapped for readability. No issues from a behavior or styling standpoint.
apps/desktop/src/renderer/components/ui/dialog.tsx (1)
47-81: LGTM! Clean implementation of overlay customization.The addition of the
overlayClassNameprop enables customization of the dialog overlay styling while maintaining backward compatibility. The implementation correctly:
- Types the prop as optional
- Passes it through to
DialogOverlay- Leverages the existing
cn()utility for class mergingapps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/useTaskForm.ts (2)
3-3: LGTM!Formatting adjustment to the import statement with no functional impact.
48-50: LGTM!The multi-line formatting of the
findcall improves readability without affecting functionality.apps/website/src/app/components/HeroSection/components/HeroCanvas/components/LitBackground/LitBackground.tsx (2)
10-10: Import changes look good.The addition of
GEOMETRY_CONFIGis appropriate for the usage in the planeGeometry configuration (lines 170-173). The reordering of the shader imports is purely cosmetic and has no functional impact.Also applies to: 16-17
169-175: Usage of GEOMETRY_CONFIG is correct, but note PR scope concern.The implementation correctly uses
GEOMETRY_CONFIGproperties to configure the planeGeometry. Extracting these magic numbers into a centralized configuration is a good practice.However, this file appears unrelated to the PR objectives ("updated the new workspace stuff and added clone from url"). This website hero section code doesn't align with the described workspace cloning functionality. Consider verifying whether this file was intentionally included in this PR.
apps/website/src/app/page.tsx (1)
1-31: Implementation verified—confirm this change belongs in this PR.The component prop contracts are satisfied correctly:
FeaturesSectionproperly receives and usesonOpenWaitlistcallbackWaitlistModalproperly receives and usesisOpenandonCloseprops- State management flow is correct
However, this file still implements waitlist modal functionality for a website landing page, which appears unrelated to the PR's stated objectives about "workspace stuff and clone from url". Before approving, verify that bundling this website change into the same PR is intentional. If unrelated, separate it into a distinct PR to improve reviewability and reduce merge complexity.
apps/desktop/src/shared/ipc-channels/index.ts (1)
14-31: Import-order tweak is safeReordering the
WorktreeChannelsimport keeps theIpcChannelsextension list unchanged and has no behavioral impact. All channel types remain correctly combined intoIpcChannels.packages/ui/src/components/dropdown-menu.tsx (1)
7-260: Dropdown menu wrappers remain thin and consistentAll wrappers still transparently forward Radix props, add
data-slothooks, and compose Tailwind classes consistently. No prop/behavior changes jump out, and the defaulted props (modal,sideOffset, etc.) look intentional.apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/CreatingView.tsx (1)
2-2: ScrollArea import cleanup looks goodUsing the single
ScrollAreaimport from@superset/ui/scroll-areaand removing the duplicate keeps the module tidy without changing runtime behavior.apps/desktop/src/renderer/screens/main/hooks/useWorkspace.ts (1)
95-97: Window restore check formatting-only changeThe
isRestoredassignment now reads on one line but still awaitswindow-is-restoredbefore deciding whether to show the workspace selection modal. No behavioral change introduced.apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/ModeNavigation/ModeNavigation.tsx (1)
36-43: ClassName rewrap preserves behaviorThe template literal for the mode button keeps the same classes and active/inactive branches; only line breaks changed. Hover/active styling and click handling are unaffected.
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx (1)
53-55: Tab width calculation unchanged
const availableWidth = containerWidth - gap * (numTabs - 1);relies on standard operator precedence and is equivalent to the parenthesized form. The min/max clamping and resize handling remain intact.apps/desktop/src/renderer/screens/main/hooks/useWorktrees.ts (1)
165-180: Worktree selection fallback condition remains correctThe reflowed condition around
refreshedWorkspace.worktreespreserves the original semantics and still guards against empty/undefined worktree arrays before selecting the first entry. No issues here.apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/ModeToggle.tsx (1)
14-18: Conditional className formatting is fineThe reformatted template literals keep the
mode === "plan" | "edit"checks and resulting classes intact while improving readability. No behavioral change.Also applies to: 25-29
apps/desktop/src/renderer/screens/main/components/DiffView/DiffContent.tsx (1)
158-166: Diff status badge ternary unchanged in behaviorThe multi-line ternary for
file.statusnow reads more clearly but still maps each status to the same classes as before. Looks good.apps/desktop/src/main/lib/window-state-manager.ts (1)
135-138: Error logging change is cosmetic onlyWrapping the
console.errorcall and passingerroras a separate argument preserves behavior while improving readability. No further changes needed.apps/desktop/src/renderer/screens/main/components/MainContentArea/MainContentArea.tsx (1)
2-5: Diff vs tabs layout wiring looks consistent; confirmautoSaveIdchange is intentional
- The integration of
sidebarModeandselectedFilewithSidebarandDiffContentAreais coherent:
useDiffDatais gated viaenabled: sidebarMode === "changes" && !!selectedWorktreeId, so diff data isn’t fetched unnecessarily.- The main render branches correctly between:
- Diff mode (loading placeholder →
DiffContentArea→ empty placeholder), and- Workspace/tab mode (placeholder vs
TabGroup/TabContent), with appropriate guards oncurrentWorkspace,selectedWorktree, andselectedTab.- The early
if (mode === "plan")return toPlanViewkeeps plan mode isolated and avoids rendering the resizable layout, which is sensible.One behavioral detail to double-check:
ResizablePanelGroupnow usesautoSaveId="new-layout-panels". If this differs from the previous ID, it will reset any persisted panel sizes for existing users. That’s not wrong, but worth confirming it’s deliberate (e.g., to clear out legacy layout state after structural changes).Also applies to: 19-40, 42-220
apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/TaskList.tsx (4)
13-29: Task type import from./typesis appropriateImporting
Taskfrom the local./typesmodule to back the existingTaskListPropsusage keeps this component strongly typed and localized. No issues here.
91-98: Loading-state UI reformat is behaviorally identicalThe Loader2 block is just re-indented; size and animation classes are the same, so users will see the same “Loading tasks…” experience.
106-108: Empty-state message remains clearThe
"No tasks match your search"vs"No tasks found"messages are unchanged in content; only the ternary formatting moved. Readability is fine and behavior is preserved.
146-149: Open-task button behavior unchanged and sensibleThe primary footer button still triggers
onOpenTaskand is disabled when there’s noselectedTaskor while tasks are loading. The JSX reflow doesn’t alter behavior and keeps UX safeguards in place.apps/desktop/src/shared/ipc-channels/tab.ts (1)
5-10: Import formatting change onlyReformatting the type import to multi-line is fine and keeps the TabChannels surface unchanged; no issues here.
apps/desktop/src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx (1)
26-41: JSX and className formatting; behavior unchangedThe updated formatting for the wrapper div, button className conditionals, and tab label ternary preserves existing behavior (selected styling, pending state, and branch/task title display). Looks good.
Also applies to: 59-61, 77-80
apps/desktop/src/renderer/screens/main/hooks/useDiffData.ts (1)
109-112: Diff loading guards and updates remain correctThe multi-line guard for already-loaded/loading file IDs and the ternary when updating file changes are semantically identical to the prior logic and remain correct for preventing duplicate loads and safely defaulting
changesto an array.Also applies to: 153-154
apps/desktop/src/renderer/screens/main/components/SidebarOverlay/SidebarOverlay.tsx (1)
12-15: onUpdateWorktree prop typing reformat is safeReformatting the
onUpdateWorktreetype and using the inlineimport("shared/types").Worktreekeeps the signature and usage the same; no issues here.apps/desktop/src/renderer/screens/main/components/DiffView/DiffContentArea.tsx (1)
52-57: File auto-load and IntersectionObserver guards unchangedThe reformatted conditions for eagerly loading the selected file and lazily loading via IntersectionObserver keep the original semantics (avoid reloading already loaded/loading files and only operate in “files” view). No behavioral changes or issues.
Also applies to: 64-65, 72-76
apps/desktop/src/renderer/screens/main/components/Layout/NewLayoutMain.tsx (6)
243-246: LGTM!Formatting change only—the type annotation remains unchanged.
531-534: LGTM!Formatting improvement for readability—logic remains correct.
723-726: LGTM!Event listener cleanup formatting improved—no behavioral change.
Also applies to: 773-776
759-762: LGTM!Logging statement reformatted—no functional change.
589-589: LGTM!Whitespace adjustment for readability.
1051-1055: Verify that removingloadingfrom the gating condition is safe.The loading state no longer prevents the main content panel from rendering. While the remaining guards (
!currentWorkspace,!selectedTab,!selectedWorktree) should protect against incomplete data, ensure there are no race conditions where these values are truthy while data is still being populated during initial load.apps/desktop/src/main/lib/window-manager.ts (1)
10-13: LGTM!All changes are formatting-only adjustments to improve code readability—no functional changes.
Also applies to: 59-59, 81-84, 116-119
apps/desktop/src/main/lib/worktree-manager.ts (1)
857-860: LGTM!Formatting improvement for readability—logic unchanged.
apps/desktop/src/renderer/screens/main/components/Sidebar/Sidebar.tsx (1)
8-8: LGTM!Import statement consolidated to a single line—no functional change.
apps/desktop/src/renderer/screens/main/types.ts (1)
5-27: LGTM!Type definitions reformatted for consistency—no changes to field names or types.
apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/types.ts (1)
1-2: LGTM!Import order adjusted and trailing whitespace removed—no functional changes.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/components/AnimatedBackground/AnimatedBackground.tsx (1)
22-22: LGTM!Added trailing comma (best practice) and removed trailing blank line—no functional changes.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/ModeCarousel.tsx (1)
94-94: LGTM!Component usage consolidated to a single line for consistency—no prop or rendering changes.
apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/hooks/useModeDetection.ts (1)
39-53: Consolidated bounds check remains correctThe combined condition still properly guards against out-of-range indices and missing modes before accessing
modes[finalIndex], so behavior is unchanged and safe.apps/desktop/src/renderer/screens/main/components/Sidebar/components/ModeCarousel/index.ts (1)
2-2: Type export reordering is harmlessReordering
ModeCarouselPropsandSidebarModein the type export does not change the public API shape; named imports continue to work as before.apps/desktop/src/renderer/screens/main/hooks/useTasks.ts (1)
5-5: Formatting and type annotations look goodThese changes (type import order, multiline generics, and reflowed
ipcRenderer.removeListener/ progress-guard code) are formatting-only and preserve the existing listener lifecycle and task creation behavior.Also applies to: 24-26, 34-36, 43-46, 176-180, 190-193, 266-269
apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/TaskForm.tsx (1)
83-85: UI/text formatting changes are non-functionalWrapping
(Optional)in a span and reflowing handlers/props just improves readability and styling; component behavior and form wiring remain unchanged.Also applies to: 125-127, 171-175, 211-215
apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/index.ts (1)
5-5: Type export reorder is safeReordering
APITaskandTaskin the exported type list does not affect consumers; the same types are still available from this barrel.apps/desktop/src/renderer/screens/main/components/Layout/AddTaskModal/utils.ts (1)
10-14: Formatting-only changes; logic unchangedLine breaks in
formatRelativeTimeand the multiline function signature forgenerateBranchNameWithCollisionAvoidancedon’t alter behavior; both utilities continue to work as before.Also applies to: 33-35
apps/desktop/src/shared/ipc-channels/workspace.ts (1)
114-122: New workspace IPC channels are well-shapedThe
workspace-clone-from-urlandworkspace-select-directorychannel definitions follow the existing IPC conventions (IpcResponse<Workspace>for workspace mutations and a simple result object for selection dialogs) and provide clear request/response shapes for the new clone-from-URL flow.apps/desktop/src/main/lib/workspace-ipcs.ts (1)
727-729:detectMainBranchcall reformatting is harmlessThis is a formatting-only change; the call semantics to
worktreeManager.detectMainBranchare unchanged and remain correct.
| /** | ||
| * Clone a git repository from a URL | ||
| */ | ||
| async cloneRepository( | ||
| url: string, | ||
| destinationPath: string, | ||
| ): Promise<{ success: boolean; path?: string; error?: string }> { | ||
| try { | ||
| // Validate URL format (basic check) | ||
| if ( | ||
| !url.includes("github.com") && | ||
| !url.includes("gitlab.com") && | ||
| !url.includes("bitbucket.org") | ||
| ) { | ||
| // Accept common git hosting services or any URL with .git | ||
| if (!url.endsWith(".git") && !url.startsWith("git@")) { | ||
| return { | ||
| success: false, | ||
| error: | ||
| "Invalid repository URL. Please provide a valid git repository URL.", | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| // Clone the repository | ||
| await execAsync(`git clone "${url}" "${destinationPath}"`, { | ||
| encoding: "utf-8", | ||
| }); | ||
|
|
||
| return { | ||
| success: true, | ||
| path: destinationPath, | ||
| }; | ||
| } catch (error) { | ||
| console.error("Failed to clone repository:", error); | ||
|
|
||
| // Extract a cleaner error message from git output | ||
| let errorMessage = error instanceof Error ? error.message : String(error); | ||
|
|
||
| // Try to extract the fatal/error line from stderr for a cleaner message | ||
| if (typeof error === "object" && error !== null && "stderr" in error) { | ||
| const stderr = String((error as any).stderr); | ||
| const fatalMatch = stderr.match(/fatal: (.+)/); | ||
| if (fatalMatch) { | ||
| errorMessage = fatalMatch[1]; | ||
| } | ||
| } | ||
|
|
||
| return { | ||
| success: false, | ||
| error: errorMessage, | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Potential shell injection vulnerability in git clone command.
The URL and destination path are passed directly to a shell command using string interpolation with double quotes. If either parameter contains special characters or malicious input (e.g., "; rm -rf /"), this could lead to command injection.
Consider using Node's execFile or spawn with argument arrays instead of shell command strings, or at minimum, validate/sanitize inputs more rigorously.
Apply this approach for safer execution:
- await execAsync(`git clone "${url}" "${destinationPath}"`, {
- encoding: "utf-8",
- });
+ const { spawn } = require("node:child_process");
+ await new Promise((resolve, reject) => {
+ const proc = spawn("git", ["clone", url, destinationPath]);
+ proc.on("close", (code) => {
+ if (code === 0) resolve(undefined);
+ else reject(new Error(`git clone failed with code ${code}`));
+ });
+ });Alternatively, verify that the URL and destinationPath are properly validated/sanitized before use.
I'll verify the shell injection concerns by examining the actual code implementation and how execAsync handles arguments.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the cloneRepository method in worktree-manager.ts
fd worktree-manager.ts --type f -x cat {} | head -1500 | tail -200
Length of output: 5074
🏁 Script executed:
#!/bin/bash
# Search for execAsync definition and usage patterns
rg -n "execAsync" -B 3 -A 8 --type tsLength of output: 19534
🏁 Script executed:
#!/bin/bash
# Check how execAsync is imported and what library provides it
rg -n "import.*execAsync|from.*execAsync" --type ts -C 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for other instances where shell commands are executed with user input
rg -n "spawn|exec|execFile" -B 2 -A 5 --type ts | head -100Length of output: 6081
🏁 Script executed:
#!/bin/bash
# Check if there are any shell injection prevention utilities in the codebase
rg -n "escape|sanitize|quote" --type ts | head -50Length of output: 1470
🏁 Script executed:
#!/bin/bash
# Examine the full cloneRepository method implementation
fd worktree-manager.ts --type f -x grep -n "cloneRepository" -A 60 {}Length of output: 1676
---
🌐 Web query:
```web
Node.js child_process exec shell injection vulnerability double quotes
💡 Result:
Short answer: wrapping untrusted input in double quotes when using child_process.exec is not a safe protection against shell injection. exec spawns a shell and shell metacharacters (e.g. ; & | && || $() ... redirections, etc.) can still be interpreted or can break quoting; there have also been real-world failures where libraries meant to quote/escape input allowed metacharacters through. [1][2][3]
What to do instead (concise):
- Avoid exec. Use execFile or spawn and pass arguments as an array (no shell interpretation). Example: child_process.spawn('ping', ['-c','1', destination]) or execFile('prog', [arg1, arg2]). This is the recommended safe pattern. [1][2]
- If you must run a shell, either whitelist/validate allowed values strictly, or use a proven shell-escaping library and keep it up-to-date (beware past vulnerabilities such as shell-quote CVE-2021-42740). Even then prefer avoiding a shell. [2][3]
- On Windows be careful: .bat/.cmd semantics require a shell; still validate/whitelist inputs there. [1]
Sources:
- Node.js child_process docs (exec vs execFile/spawn, warning about unsanitized input). [1]
- Practical guidance / OWASP-style writeups on Node command-injection prevention (advocates spawn/execFile and warns that quoting isn't sufficient). [2]
- CVE-2021-42740 (shell-quote command-injection example showing how quoting/escaping tooling can fail). [3]
🌐 Web query:
Node.js exec vs spawn security difference
💡 Result:
Short answer: exec is riskier. exec runs a shell (via /bin/sh -c) and interprets metacharacters, so passing untrusted strings to it allows command injection. spawn and execFile invoke a program directly with an argument array (no shell) by default, which prevents shell metacharacter injection. However spawn/execFile can still be unsafe if you enable shell:true, pass unsanitized args, leak env vars, or run with excessive privileges — so always validate/whitelist input, prefer argument arrays, drop privileges, and avoid shells when possible. [1][2][3][4][5]
Key points and recommendations
- Why exec is dangerous: it spawns a shell and concatenates the command string, so things like ;, &&, |, $(), backticks let an attacker run extra commands. Use only with fully trusted/static commands. [1][2][5]
- Why spawn/execFile are safer: they call execve on the target program and accept an args array, so user input is passed as a single argument (no shell parsing). Prefer these when dealing with untrusted input. [1][3][4]
- Caveats:
- spawn(..., { shell: true }) or execFile with shell enabled reintroduces shell parsing and the risks of exec. [1][3]
- exec buffers entire output (maxBuffer), which can cause other issues; spawn streams stdout/stderr. [4]
- Environment variables and working directory can leak secrets—set a safe env and cwd. [3]
- Always validate/whitelist inputs, escape or avoid calling the shell, and run with least privilege. [2][3][5]
Sources: [1][2][3][4][5]
References
[1] SecureFlag / OS Command Injection in NodeJS.
[2] Built In — Node.js app security (child_process guidance).
[3] NodeJS security blog — avoid shell, use execFile/spawn and sanitize.
[4] Stack Overflow — technical difference: exec uses shell vs spawn execve.
[5] Datadog static-analysis guidance on child_process.exec risks.
🌐 Web query:
Can command substitution happen inside double quotes shell
💡 Result:
Yes. Command substitution ( $(...) or ... ) is recognized and performed when it appears inside double quotes. The substitution's output then replaces the $(...) text; because it was inside double quotes, the shell does NOT perform word-splitting or pathname (globbing) on that output. Also, a command substitution begins its own quoting context, so quotes inside the substitution don't pair with quotes outside. Examples and standards: Bash manual and the POSIX shell spec. [1][2][3]
Example:
- echo "$(printf 'a b')"
prints: a b - but the result won't be split into separate words by the shell because it was double-quoted. [1][2]
Sources:
[1] Bash Reference Manual — Command Substitution.
[2] POSIX Shell Command Language — Command Substitution.
[3] Discussion / examples (Unix & Linux / StackOverflow).
Shell injection vulnerability confirmed in cloneRepository method.
The use of exec() with double-quoted parameters allows command substitution and shell metacharacters to be interpreted, bypassing the URL validation. A malicious URL like "https://github.com/user/repo.git$(malicious-command)" would execute arbitrary code.
The suggested fix using spawn() with an argument array is the correct approach. spawn and execFile invoke a program directly with an argument array (no shell) by default, which prevents shell metacharacter injection.
Apply the safer pattern shown in the original review comment or use similar approach for destinationPath validation.
🤖 Prompt for AI Agents
apps/desktop/src/main/lib/worktree-manager.ts around lines 1335-1388: the
current implementation uses execAsync with a single shell command string which
allows shell metacharacter/command-substitution injection via a crafted URL or
path; replace this with a non-shell child process invocation
(child_process.spawn or execFile) that passes the git command and its arguments
as an array (e.g., ['clone', url, destinationPath]) so the shell is not used,
validate/sanitize destinationPath (no shell metacharacters, path traversal, and
ensure it is a valid filesystem path), run the process and await its exit via a
Promise that resolves on exit code 0 and rejects on non-zero code while
collecting stderr/stdout for diagnostics, and propagate a safe, extracted error
message on failure.
Resolved merge conflicts by accepting main branch changes: - src/renderer/screens/main/components/Layout/AddTaskModal/utils.ts - src/renderer/screens/main/components/Layout/TaskTabs/TaskTabs.tsx - src/renderer/screens/main/components/Layout/TaskTabs/WorktreeTabButton.tsx - src/renderer/screens/main/hooks/useTasks.ts - src/renderer/screens/main/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx (2)
20-24: MemoizerecentWorkspacesto prevent unnecessary effect re-runs.The
recentWorkspacesarray is recalculated on every render, creating a new reference even when the underlying data hasn't changed. This causes the keyboard navigation effect (line 38) to re-run unnecessarily.Apply this diff to memoize the array:
+import { useEffect, useMemo, useState } from "react"; -import { useEffect, useState } from "react";- // Show recent workspaces (limit to 5 for display) - const recentWorkspaces = workspaces.slice(0, 5); + // Show recent workspaces (limit to 5 for display) + const recentWorkspaces = useMemo(() => workspaces.slice(0, 5), [workspaces]);
189-191: Consider using a named constant for the button offset.The magic number
2(line 190) represents the number of action buttons before the recent workspaces list. While the comment explains this, using a named constant would make the code more maintainable and reduce the risk of errors if buttons are added or removed.Apply this diff:
+ // Number of action buttons before the recent workspaces list + const ACTION_BUTTON_COUNT = 2; + // Total navigable items: 2 action buttons + recent workspaces // (excluding disabled SSH button) - const totalItems = 2 + recentWorkspaces.length; + const totalItems = ACTION_BUTTON_COUNT + recentWorkspaces.length;{recentWorkspaces.map((workspace, index) => { - const itemIndex = index + 2; // Offset by 2 for the action buttons + const itemIndex = index + ACTION_BUTTON_COUNT; return (
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/renderer/globals.css(1 hunks)apps/desktop/src/renderer/index.html(1 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/CloneFromUrlDialog.tsx (1)
CloneFromUrlDialog(21-184)
🔇 Additional comments (3)
apps/desktop/src/renderer/index.html (1)
14-17: Google Fonts integration follows best practices and the.font-micro5class is actively used.The preconnect optimization and
display=swapparameter ensure efficient font loading and prevent layout shifts. The class is defined inglobals.css:139and actively used inWorkspaceSelectionModal.tsx:124. CSP policy already permits external resources.apps/desktop/src/renderer/globals.css (1)
138-141: The AI summary was inaccurate; no duplication exists.Verification shows
.font-micro5is defined only once in the file (line 139). There is no duplicate definition at top-level or elsewhere. The single definition is correct and requires no changes.Likely an incorrect or invalid review comment.
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx (1)
90-113: No changes needed. IPC contract is properly typed and validated.The IPC channel
"workspace-clone-from-url"is properly defined with response typeIpcResponse<Workspace>in the shared types. TheWorkspaceinterface guarantees anid: stringfield as required, so whenresult.datapasses the truthiness check at line 100, it is guaranteed to have anidproperty. The existing validation (result.success && result.data) already provides sufficient type-safe protection through TypeScript's type system. The suggested additional check forresult.data.idis unnecessary.
| useEffect(() => { | ||
| if (!isOpen || showCloneDialog) return; | ||
|
|
||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| switch (e.key) { | ||
| case "ArrowDown": | ||
| case "ArrowRight": | ||
| e.preventDefault(); | ||
| setSelectedIndex((prev) => | ||
| prev < totalItems - 1 ? prev + 1 : prev, | ||
| ); | ||
| break; | ||
| case "ArrowUp": | ||
| case "ArrowLeft": | ||
| e.preventDefault(); | ||
| setSelectedIndex((prev) => (prev > 0 ? prev - 1 : prev)); | ||
| break; | ||
| case "Enter": | ||
| e.preventDefault(); | ||
| handleSelectItem(selectedIndex); | ||
| break; | ||
| } | ||
| }; | ||
|
|
||
| window.addEventListener("keydown", handleKeyDown); | ||
| return () => window.removeEventListener("keydown", handleKeyDown); | ||
| }, [isOpen, showCloneDialog, selectedIndex, totalItems, recentWorkspaces]); |
There was a problem hiding this comment.
Include handleSelectItem in the effect's dependency array.
The keyboard navigation effect calls handleSelectItem (line 57) but doesn't include it in the dependency array (line 64). This creates a stale closure risk where the effect might reference an outdated version of the function.
Apply this diff:
window.addEventListener("keydown", handleKeyDown);
return () => window.removeEventListener("keydown", handleKeyDown);
- }, [isOpen, showCloneDialog, selectedIndex, totalItems, recentWorkspaces]);
+ }, [isOpen, showCloneDialog, selectedIndex, totalItems, recentWorkspaces, handleSelectItem]);Note: If ESLint warnings about exhaustive-deps appear after this change, consider wrapping handleSelectItem in useCallback to stabilize its reference.
📝 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.
| useEffect(() => { | |
| if (!isOpen || showCloneDialog) return; | |
| const handleKeyDown = (e: KeyboardEvent) => { | |
| switch (e.key) { | |
| case "ArrowDown": | |
| case "ArrowRight": | |
| e.preventDefault(); | |
| setSelectedIndex((prev) => | |
| prev < totalItems - 1 ? prev + 1 : prev, | |
| ); | |
| break; | |
| case "ArrowUp": | |
| case "ArrowLeft": | |
| e.preventDefault(); | |
| setSelectedIndex((prev) => (prev > 0 ? prev - 1 : prev)); | |
| break; | |
| case "Enter": | |
| e.preventDefault(); | |
| handleSelectItem(selectedIndex); | |
| break; | |
| } | |
| }; | |
| window.addEventListener("keydown", handleKeyDown); | |
| return () => window.removeEventListener("keydown", handleKeyDown); | |
| }, [isOpen, showCloneDialog, selectedIndex, totalItems, recentWorkspaces]); | |
| useEffect(() => { | |
| if (!isOpen || showCloneDialog) return; | |
| const handleKeyDown = (e: KeyboardEvent) => { | |
| switch (e.key) { | |
| case "ArrowDown": | |
| case "ArrowRight": | |
| e.preventDefault(); | |
| setSelectedIndex((prev) => | |
| prev < totalItems - 1 ? prev + 1 : prev, | |
| ); | |
| break; | |
| case "ArrowUp": | |
| case "ArrowLeft": | |
| e.preventDefault(); | |
| setSelectedIndex((prev) => (prev > 0 ? prev - 1 : prev)); | |
| break; | |
| case "Enter": | |
| e.preventDefault(); | |
| handleSelectItem(selectedIndex); | |
| break; | |
| } | |
| }; | |
| window.addEventListener("keydown", handleKeyDown); | |
| return () => window.removeEventListener("keydown", handleKeyDown); | |
| }, [isOpen, showCloneDialog, selectedIndex, totalItems, recentWorkspaces, handleSelectItem]); |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx
around lines 38 to 64, the useEffect that registers the keyboard handler calls
handleSelectItem but does not list it in the dependency array, creating a
stale-closure risk; update the effect dependencies to include handleSelectItem,
and if ESLint flags exhaustive-deps or you want to avoid the handler
re-registering unnecessarily, wrap handleSelectItem in useCallback with stable
dependencies so the effect can safely depend on it.
|
|
||
| return ( | ||
| <Dialog open={isOpen} onOpenChange={() => { }}> | ||
| <Dialog open={isOpen} onOpenChange={() => {}}> |
There was a problem hiding this comment.
Dialog cannot be closed via overlay or ESC key.
The onOpenChange prop is set to an empty function, which prevents users from closing the modal via the overlay click or ESC key. Users must use one of the action buttons.
If this is intentional to enforce a specific user flow, consider adding an explicit close button or documenting this behavior. Otherwise, provide a proper close handler.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx
around line 116, the Dialog component's onOpenChange is currently an empty
function preventing closing via overlay or ESC; replace the no-op with a proper
handler that updates the modal open state (or calls the provided close
callback/prop) so overlay clicks and ESC close the dialog, or if intentional,
add an explicit close button and document the behavior; ensure the handler
toggles/sets isOpen false and performs any cleanup (e.g., resetting selection)
consistently.
| <button | ||
| type="button" | ||
| onClick={handleOpenProject} | ||
| className="group flex flex-col items-center justify-center w-full md:w-52 h-32 px-3 py-4 rounded-lg border border-neutral-700 bg-neutral-800/50 hover:bg-neutral-800 hover:border-neutral-600 transition-all opacity-50 cursor-not-allowed" | ||
| disabled | ||
| title="Coming soon" | ||
| > | ||
| <div className="flex items-center justify-center w-10 h-10 rounded-lg bg-neutral-700/50 mb-2"> | ||
| <Terminal className="w-5 h-5 text-neutral-300" /> | ||
| </div> | ||
| <div className="text-sm font-medium text-white"> | ||
| Connect via SSH | ||
| </div> | ||
| </button> |
There was a problem hiding this comment.
Remove or correct the onClick handler for the disabled SSH button.
The "Connect via SSH" button is disabled (line 167) but has onClick={handleOpenProject} (line 165), which is incorrect. Even though disabled buttons don't fire onClick events, having the wrong handler is confusing for maintainers.
Apply this diff:
<button
type="button"
- onClick={handleOpenProject}
className="group flex flex-col items-center justify-center w-full md:w-52 h-32 px-3 py-4 rounded-lg border border-neutral-700 bg-neutral-800/50 hover:bg-neutral-800 hover:border-neutral-600 transition-all opacity-50 cursor-not-allowed"
disabled
title="Coming soon"
>📝 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.
| <button | |
| type="button" | |
| onClick={handleOpenProject} | |
| className="group flex flex-col items-center justify-center w-full md:w-52 h-32 px-3 py-4 rounded-lg border border-neutral-700 bg-neutral-800/50 hover:bg-neutral-800 hover:border-neutral-600 transition-all opacity-50 cursor-not-allowed" | |
| disabled | |
| title="Coming soon" | |
| > | |
| <div className="flex items-center justify-center w-10 h-10 rounded-lg bg-neutral-700/50 mb-2"> | |
| <Terminal className="w-5 h-5 text-neutral-300" /> | |
| </div> | |
| <div className="text-sm font-medium text-white"> | |
| Connect via SSH | |
| </div> | |
| </button> | |
| <button | |
| type="button" | |
| className="group flex flex-col items-center justify-center w-full md:w-52 h-32 px-3 py-4 rounded-lg border border-neutral-700 bg-neutral-800/50 hover:bg-neutral-800 hover:border-neutral-600 transition-all opacity-50 cursor-not-allowed" | |
| disabled | |
| title="Coming soon" | |
| > | |
| <div className="flex items-center justify-center w-10 h-10 rounded-lg bg-neutral-700/50 mb-2"> | |
| <Terminal className="w-5 h-5 text-neutral-300" /> | |
| </div> | |
| <div className="text-sm font-medium text-white"> | |
| Connect via SSH | |
| </div> | |
| </button> |
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceSelectionModal/WorkspaceSelectionModal.tsx
around lines 163 to 176, the disabled "Connect via SSH" button includes an
incorrect onClick={handleOpenProject} handler; remove the onClick prop (or set
it to undefined) so the disabled control has no click handler and update props
accordingly to avoid misleading maintainers, keeping the disabled and
title="Coming soon" as-is.
2 participants
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
UI/UX Improvements
Refactor