feat(desktop): support multi-folder selection in Open Project dialog #1248
Conversation
Add multiSelections to the native file picker so users can select multiple folders at once. Each folder is processed independently with per-folder success/error/needsGitInit outcomes. - Add FolderOutcome and OpenNewMultiResult types for multi-select results - Update openNew tRPC mutation to iterate all selected paths - Update all 4 UI call sites (StartView, WorkspaceSidebarFooter, NewWorkspaceModal, SidebarDropZone) to handle multi-results - Update InitGitDialog to display and process multiple non-git folders - Show summary toast (e.g. '3 projects opened') after multi-select - Navigate to first successfully opened project from StartView - Cache getGitHubUsername to avoid GitHub API rate limit spam - Fix SKIP_ENV_VALIDATION dev bypass not working (isPending guard and sign-in page were redirecting before bypass could take effect)
feat(desktop): support multi-folder selection in Open Project dialog
📝 WalkthroughWalkthroughMulti-select support is introduced for opening projects. The openNew mutation now handles multiple folder paths, returning per-folder outcomes (success, needsGitInit, or error). UI components are updated to process multi-result data, showing aggregate toasts and handling per-item errors. GitHub username caching is added, and sign-in bypass via environment flag is implemented. Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as UI (StartView)
participant Router as Backend (openNew)
participant GitCheck as Git Operations
participant Workspace as Workspace Creation
participant Dialog as InitGitDialog
User->>UI: Select multiple folders
UI->>Router: Call openNew with multiple paths
Router->>GitCheck: Check each path for git root
alt Path has git repository
GitCheck-->>Router: Return git root
Router->>Workspace: Upsert project, setup workspace
Workspace-->>Router: Success with Project object
Router->>Router: Record FolderOutcome (success)
else Path is not a git repository
GitCheck-->>Router: Error/no git root
Router->>Router: Record FolderOutcome (needsGitInit)
else Unexpected error
Router->>Router: Record FolderOutcome (error)
end
Router-->>UI: Return OpenNewMultiResult with outcomes array
UI->>UI: Process results - separate success/needsGitInit/error
alt Has successful projects
UI->>User: Show success toast with count
UI->>UI: Navigate to first successful project
end
alt Has error outcomes
UI->>User: Show per-item error toasts
end
alt Has needsGitInit outcomes
UI->>Dialog: Open with selected paths requiring init
Dialog->>GitCheck: Initialize git for each path
Dialog->>Workspace: Create workspace for each path
Dialog-->>UI: Complete
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/renderer/screens/main/components/StartView/InitGitDialog.tsx (1)
118-161:⚠️ Potential issue | 🟡 MinorMulti-path init flow is solid; minor UX concern with error visibility.
The sequential processing with per-path error collection is the right approach for git init + DB operations. One note:
onClose()on Line 155 always fires (even when errors occurred), and inStartViewthe resulting error banner auto-dismisses after 5 seconds. For a batch of failures, the aggregated error message on Line 151 could be long and disappear before the user reads it. Consider showing errors viatoast.error(which persists until dismissed) instead of the auto-dismissing banner, or letting the caller decide.
🤖 Fix all issues with AI agents
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 509-520: The catch block for the open-new-project flow logs the
real error but pushes a generic message into the outcomes array; update the
outcomes.push call in the catch to include the actual error text (e.g., derive
message via error instanceof Error ? error.message : String(error)) instead of
the static "Failed to open project" string so selectedPath and the real error
message are returned; locate the catch handling around the selectedPath variable
in the function (the section that logs "[projects/openNew] Failed to open
project") and replace the pushed error value accordingly.
In `@apps/desktop/src/renderer/screens/main/components/StartView/index.tsx`:
- Around line 60-108: The code currently calls navigate(...) when there are
successes and also sets setInitGitDialog(...) when needsGitInit exists, causing
StartView (which renders InitGitDialog) to unmount and the dialog to be lost;
fix by deferring navigation whenever needsGitInit.length > 0: detect
needsGitInit first, call setInitGitDialog({ isOpen: true, selectedPath: ...,
selectedPaths: ... }) and store the intended navigation target (e.g., in a
pendingNavigate state or include it on the dialog state), then only call
navigate(...) after the dialog is dismissed/handled; update InitGitDialog’s
confirm/close handlers to consume that pendingNavigate and perform navigate(...)
(use the existing setInitGitDialog, InitGitDialog component, and navigate
function names to locate and implement the change).
🧹 Nitpick comments (4)
apps/desktop/src/renderer/routes/sign-in/page.tsx (1)
21-24:SKIP_ENV_VALIDATIONis semantically misleading when used to bypass authentication.This variable typically controls Zod schema validation of env vars (common in t3-stack setups). Repurposing it to skip the entire sign-in flow is a naming mismatch that makes it easy to overlook the security implication — if this env var leaks into a production build, authentication is silently bypassed.
Consider using a dedicated flag (e.g.,
DEV_SKIP_AUTH) and guarding it behind aprocess.env.NODE_ENV !== 'production'check at build time.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
56-71: Type discrimination relies on fragile string-based property checks.
OpenNewMultiResultuses overlapping shapes (OpenNewErrorvsOpenNewMultiSuccess) discriminated only by the presence/absence of"multi". All UI call sites use"error" in result && !("multi" in result)which is brittle and error-prone.A proper discriminated union with an explicit
typefield would make consumption safer:♻️ Suggested approach
-type OpenNewCanceled = { canceled: true }; -type OpenNewMultiSuccess = { - canceled: false; - multi: true; - results: FolderOutcome[]; -}; -export type OpenNewMultiResult = - | OpenNewCanceled - | OpenNewMultiSuccess - | OpenNewError; +type OpenNewCanceled = { type: "canceled" }; +type OpenNewMultiSuccess = { + type: "multi"; + results: FolderOutcome[]; +}; +type OpenNewError = { type: "error"; error: string }; +export type OpenNewMultiResult = + | OpenNewCanceled + | OpenNewMultiSuccess + | OpenNewError;Then UI can use
switch (result.type)instead of"error" in result && !("multi" in result).apps/desktop/src/renderer/components/NewWorkspaceModal/NewWorkspaceModal.tsx (1)
209-244: Duplicated multi-result handling logic across three components.This exact pattern — filter successes/errors/needsGitInit, show summary toast, show per-error toasts, show git-init warnings — is repeated verbatim in
StartView/index.tsx,WorkspaceSidebarFooter.tsx, and here. Consider extracting a shared helper (e.g.,processOpenNewMultiResult) that takes the results and callbacks, to keep the three call sites DRY.Also,
err.selectedPath.split("/").pop()won't extract the folder name correctly on Windows (backslash separators). ThegetBasenamehelper inInitGitDialog.tsxalready handles both — consider reusing it.apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarFooter.tsx (1)
80-93: Same cross-platform path and code duplication notes apply here.
selectedPath.split("/").pop()doesn't handle Windows backslash paths. This is the third copy of the same multi-result processing pattern (also inStartView/index.tsxandNewWorkspaceModal.tsx). A shared utility would reduce the maintenance burden.
| } catch (error) { | ||
| console.error( | ||
| "[projects/openNew] Failed to open project:", | ||
| selectedPath, | ||
| error, | ||
| ); | ||
| outcomes.push({ | ||
| status: "error", | ||
| selectedPath, | ||
| error: "Failed to open project", | ||
| }); | ||
| } |
There was a problem hiding this comment.
Error outcome discards the actual error message.
The catch block logs the real error but pushes a generic "Failed to open project" string to the user. Include the actual message so users (and support) can diagnose the issue.
🐛 Proposed fix
} catch (error) {
console.error(
"[projects/openNew] Failed to open project:",
selectedPath,
error,
);
outcomes.push({
status: "error",
selectedPath,
- error: "Failed to open project",
+ error: error instanceof Error ? error.message : "Failed to open project",
});
}📝 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.
| } catch (error) { | |
| console.error( | |
| "[projects/openNew] Failed to open project:", | |
| selectedPath, | |
| error, | |
| ); | |
| outcomes.push({ | |
| status: "error", | |
| selectedPath, | |
| error: "Failed to open project", | |
| }); | |
| } | |
| } catch (error) { | |
| console.error( | |
| "[projects/openNew] Failed to open project:", | |
| selectedPath, | |
| error, | |
| ); | |
| outcomes.push({ | |
| status: "error", | |
| selectedPath, | |
| error: error instanceof Error ? error.message : "Failed to open project", | |
| }); | |
| } |
🤖 Prompt for AI Agents
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts` around lines 509 -
520, The catch block for the open-new-project flow logs the real error but
pushes a generic message into the outcomes array; update the outcomes.push call
in the catch to include the actual error text (e.g., derive message via error
instanceof Error ? error.message : String(error)) instead of the static "Failed
to open project" string so selectedPath and the real error message are returned;
locate the catch handling around the selectedPath variable in the function (the
section that logs "[projects/openNew] Failed to open project") and replace the
pushed error value accordingly.
| if ("multi" in result) { | ||
| const successes = result.results.filter( | ||
| (r) => r.status === "success", | ||
| ); | ||
| const needsGitInit = result.results.filter( | ||
| (r) => r.status === "needsGitInit", | ||
| ); | ||
| const errors = result.results.filter((r) => r.status === "error"); | ||
|
|
||
| // Show summary toast for opened projects | ||
| if (successes.length > 0) { | ||
| toast.success( | ||
| successes.length === 1 | ||
| ? "Project opened" | ||
| : `${successes.length} projects opened`, | ||
| ); | ||
|
|
||
| // Navigate to the first successfully opened project | ||
| navigate({ | ||
| to: "/project/$projectId", | ||
| params: { projectId: successes[0].project.id }, | ||
| replace: true, | ||
| }); | ||
| } | ||
|
|
||
| // Show errors | ||
| if (errors.length > 0) { | ||
| for (const err of errors) { | ||
| toast.error( | ||
| `Failed to open ${err.selectedPath.split("/").pop()}`, | ||
| { | ||
| description: err.error, | ||
| }, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Prompt for git init if needed | ||
| if (needsGitInit.length > 0) { | ||
| const paths = needsGitInit.map((r) => r.selectedPath); | ||
| setInitGitDialog({ | ||
| isOpen: true, | ||
| selectedPath: paths[0], | ||
| selectedPaths: paths, | ||
| }); | ||
| } | ||
|
|
||
| if ("project" in result && result.project) { | ||
| navigate({ | ||
| to: "/project/$projectId", | ||
| params: { projectId: result.project.id }, | ||
| replace: true, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Bug: InitGitDialog is lost when both successes and needsGitInit results exist.
When successes.length > 0 and needsGitInit.length > 0, the code navigates to the first project (Line 78, which unmounts StartView) and sets the InitGitDialog state (Line 100). Since InitGitDialog is rendered inside StartView (Line 304), navigation causes it to unmount before the user can see or interact with the git-init prompt.
Consider either:
- Deferring navigation until after the dialog is dismissed, or
- Lifting
InitGitDialogto a layout-level component that persists across routes.
🤖 Prompt for AI Agents
In `@apps/desktop/src/renderer/screens/main/components/StartView/index.tsx` around
lines 60 - 108, The code currently calls navigate(...) when there are successes
and also sets setInitGitDialog(...) when needsGitInit exists, causing StartView
(which renders InitGitDialog) to unmount and the dialog to be lost; fix by
deferring navigation whenever needsGitInit.length > 0: detect needsGitInit
first, call setInitGitDialog({ isOpen: true, selectedPath: ..., selectedPaths:
... }) and store the intended navigation target (e.g., in a pendingNavigate
state or include it on the dialog state), then only call navigate(...) after the
dialog is dismissed/handled; update InitGitDialog’s confirm/close handlers to
consume that pendingNavigate and perform navigate(...) (use the existing
setInitGitDialog, InitGitDialog component, and navigate function names to locate
and implement the change).
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
PR Review: feat(desktop): support multi-folder selection in Open Project dialog
Thanks for the contribution @quuu! Multi-select for project import is a solid UX improvement. I've reviewed all the changes and have feedback organized by severity.
Issues to Address
1. Duplicated result-handling logic across 3 components
The same filtering + toast pattern is copy-pasted nearly identically in StartView/index.tsx, WorkspaceSidebarFooter.tsx, and NewWorkspaceModal.tsx:
const successes = result.results.filter((r) => r.status === "success");
const needsGitInit = result.results.filter((r) => r.status === "needsGitInit");
const errors = result.results.filter((r) => r.status === "error");
// ... show toasts, handle each caseThis violates DRY and makes it easy for the three call sites to drift. Consider extracting a shared helper (e.g. processMultiOpenResult(results) returning { successes, needsGitInit, errors }) and a showMultiOpenToasts(...) utility, or a custom hook like useHandleMultiOpenResult().
2. selectedPath.split("/").pop() used as a basename utility in 3 places
InitGitDialog.tsx already defines a proper getBasename() helper that handles both Unix and Windows paths. The other components use the naive split("/").pop() which won't work on Windows. Either extract getBasename to a shared util or import it.
3. Sequential processing where parallel would be better
In projects.ts, the for...of loop processes folders sequentially:
for (const selectedPath of result.filePaths) {
// ... await getGitRoot, getDefaultBranch, etc
}Same in WorkspaceSidebarFooter.tsx for workspace creation and InitGitDialog.tsx for git init. If a user selects 10 folders, this could be noticeably slow. Consider using Promise.allSettled for the independent operations (each folder's processing doesn't depend on the others).
4. Loss of loading feedback in WorkspaceSidebarFooter
The original code used toast.promise() which showed a loading spinner toast while the workspace was being created:
// Before
toast.promise(createBranchWorkspace.mutateAsync({ projectId: result.project.id }), {
loading: "Opening project...",
success: "Project opened",
error: (err) => ...
});The new code silently creates workspaces and only shows a success/error toast after. For multi-select with sequential processing, the user gets no visual feedback during what could be a long operation.
5. SKIP_ENV_VALIDATION used as auth bypass (scope creep)
The changes to layout.tsx and sign-in/page.tsx use SKIP_ENV_VALIDATION to bypass authentication. This flag is semantically about environment variable validation, not authentication. Using it for auth bypass is confusing and potentially risky — a dev who enables SKIP_ENV_VALIDATION to skip env checks may not realize they're also bypassing auth.
These changes are also unrelated to the multi-folder selection feature and should be in a separate PR. If an auth bypass is needed for dev, consider a dedicated SKIP_AUTH flag.
6. Unnecessary toast for single project open
The new code shows toast.success("Project opened") even when only one folder was selected — the common case. The original code didn't show a toast for single opens, which was cleaner (the navigation itself was the feedback). Consider only showing the summary toast when successes.length > 1.
This is already handled correctly in StartView but WorkspaceSidebarFooter and NewWorkspaceModal show it for single opens too. The StartView pattern is the right one:
if (successes.length > 1) {
toast.success(`${successes.length} projects opened`);
}Wait, actually StartView shows "Project opened" for single too. But the original WorkspaceSidebarFooter already showed "Project opened" via toast.promise, so the single-toast behavior is consistent there. I'd still recommend not toasting for single opens from StartView since it didn't before and the navigation is sufficient feedback.
7. InitGitDialog API could be cleaner
The component now has both selectedPath: string and selectedPaths?: string[] with internal normalization logic. Since this is a new feature, consider just using paths: string[] as the sole prop (callers can pass [singlePath]). The current dual-prop approach adds complexity:
const allPaths =
selectedPaths && selectedPaths.length > 0
? selectedPaths
: selectedPath
? [selectedPath]
: [];This also means selectedPath becomes a dead prop when selectedPaths is provided, which is confusing.
Minor Notes
- The GitHub username caching in
git.tsis a nice improvement. Clean implementation with TTL. - The
"error" in result && !("multi" in result)discriminant check works but is fragile — it relies on the absence of amultifield onOpenNewError. Consider adding atypeorkinddiscriminant field to the result types for clearer pattern matching. - In
InitGitDialog,onClose()is called even when there are errors (errors.length > 0). This means the dialog closes and the user sees the error viaonErrorcallback. This seems intentional but worth confirming — should the dialog stay open if some folders failed?
Summary
The core feature (multi-folder selection) is well-implemented and the UX flow is thoughtful (per-folder error handling, InitGitDialog for non-git folders, summary toasts). The main concerns are:
- Extract shared result-handling logic to reduce duplication
- Separate the auth bypass changes into their own PR
- Consider parallel processing for better performance with many folders
- Use
getBasenameutility consistently (Windows compat)
Nice work overall — looking forward to a revised version!
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
PR Review: feat(desktop): support multi-folder selection in Open Project dialog
Thanks for the contribution @quuu! Multi-select for project import is a useful feature. I reviewed the diff carefully against our codebase conventions (AGENTS.md) and spotted a few issues — some structural, some correctness.
1. Duplicated multi-result handling across 3 call sites (Shotgun Surgery)
The filtering + toast logic for successes, needsGitInit, and errors is copy-pasted across StartView/index.tsx, WorkspaceSidebarFooter.tsx, and NewWorkspaceModal.tsx. From AGENTS.md:
| Shotgun surgery | One logical change requires edits in 5+ files | Co-locate related code; reconsider boundaries |
Consider extracting a shared helper, e.g.:
function categorizeMultiResult(results: FolderOutcome[]) {
return {
successes: results.filter(r => r.status === "success"),
needsGitInit: results.filter(r => r.status === "needsGitInit"),
errors: results.filter(r => r.status === "error"),
};
}Similarly, the folder name extraction via err.selectedPath.split("/").pop() is repeated in all three components — InitGitDialog already has a getBasename() utility for this. Extract and share it.
2. Race condition: navigate() + setInitGitDialog() in StartView
In StartView/index.tsx, when a multi-select contains both successful projects and folders needing git init, the handler does:
// Navigate away...
navigate({ to: "/project/$projectId", params: { projectId: successes[0].project.id }, replace: true });
// ...then try to open a dialog on the now-unmounted component
if (needsGitInit.length > 0) {
setInitGitDialog({ isOpen: true, selectedPath: paths[0], selectedPaths: paths });
}The navigation will unmount StartView, so the InitGitDialog will never appear. This means non-git folders are silently dropped when mixed with valid git repos. The git-init prompt should either happen before navigation, or be deferred to the target view.
3. SKIP_ENV_VALIDATION misused as auth bypass
The two "dev experience" changes repurpose SKIP_ENV_VALIDATION to skip authentication entirely:
sign-in/page.tsx:if (env.SKIP_ENV_VALIDATION) return <Navigate to="/workspace" replace />layout.tsx:if (isPending && !env.SKIP_ENV_VALIDATION)
This is semantically misleading — SKIP_ENV_VALIDATION is for skipping environment variable validation, not for bypassing authentication. If a dev auth bypass is needed, it should use a dedicated flag (e.g., DEV_SKIP_AUTH). Mixing these concerns is confusing and potentially risky if the env var is accidentally set in production. I'd recommend dropping these two changes from this PR and handling them separately if needed.
4. UX regression in WorkspaceSidebarFooter
The original code used toast.promise() to show loading → success → error feedback during workspace creation:
toast.promise(createBranchWorkspace.mutateAsync({ projectId: result.project.id }), {
loading: "Opening project...",
success: "Project opened",
error: (err) => err instanceof Error ? err.message : "Failed to open project",
});The new code replaces this with a for...of loop that processes all projects sequentially with no loading indicator. For multi-select this makes the UI appear frozen during the loop. Consider showing some loading state (at least keeping toast.promise for single-project opens, or adding a loading toast for multi).
5. Generic error message in backend hides real errors
In projects.ts, the catch block pushes a generic message:
outcomes.push({
status: "error",
selectedPath,
error: "Failed to open project", // ← real error is only logged
});The actual error is logged to console but never shown to the user. This makes debugging difficult. Consider passing the actual error message:
error: error instanceof Error ? error.message : "Failed to open project",6. InitGitDialog — redundant selectedPath + selectedPaths props
The component accepts both selectedPath: string and selectedPaths?: string[], then normalizes internally. This dual-prop interface is confusing. Since you're updating all call sites anyway, consider just changing the interface to paths: string[] and keeping it clean.
7. GitHub username caching — good addition, minor note
The in-memory cache with 5-minute TTL for getGitHubUsername is a solid improvement. One minor note: caching a null result (auth failure) for 5 minutes means if a user fixes their gh auth mid-session, they'll still see failures for up to 5 minutes. Consider using a shorter TTL for error cases (e.g., 30 seconds).
Summary
The core feature (multi-select in the file picker) is welcome and the per-folder error handling approach is sound. The main areas to address:
- Bug: Navigate-before-dialog-open race condition in StartView (non-git folders silently lost)
- Scope: Drop the
SKIP_ENV_VALIDATIONauth bypass changes — they're unrelated and semantically wrong - Code quality: Extract duplicated multi-result handling into a shared helper
- UX: Restore loading feedback in
WorkspaceSidebarFooterfor workspace creation - DX: Surface real error messages from the backend instead of generic strings
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
PR Review: feat(desktop): support multi-folder selection in Open Project dialog
Thanks for the contribution @quuu! The core feature is useful — importing projects one-by-one is definitely friction. Here are my findings:
Type safety: fragile discriminated union
The OpenNewMultiResult union relies on structural "duck typing" to discriminate variants:
if ("error" in result && !("multi" in result)) { ... }This pattern appears in all 3 UI call sites and is fragile — if someone later adds a multi field to OpenNewError, it silently breaks. A proper discriminated union would be safer:
type OpenNewMultiResult =
| { type: "canceled" }
| { type: "success"; results: FolderOutcome[] }
| { type: "error"; error: string }Or simpler: just make results always present and check result.canceled first, then result.results.
Duplicated result handling across 3 call sites
The same filtering + toast pattern is copy-pasted across StartView, WorkspaceSidebarFooter, and NewWorkspaceModal:
const successes = result.results.filter((r) => r.status === "success");
const needsGitInit = result.results.filter((r) => r.status === "needsGitInit");
const errors = result.results.filter((r) => r.status === "error");
// ... toast logic ...Per the project's AGENTS.md "three instances" heuristic, this has hit the threshold for extraction. Consider a shared helper like:
function categorizeOutcomes(results: FolderOutcome[]) {
return {
successes: results.filter((r) => r.status === "success"),
needsGitInit: results.filter((r) => r.status === "needsGitInit"),
errors: results.filter((r) => r.status === "error"),
};
}InitGitDialog prop API is redundant
The component now accepts both selectedPath: string (required) and selectedPaths?: string[] (optional), then normalizes internally:
const allPaths =
selectedPaths && selectedPaths.length > 0
? selectedPaths
: selectedPath ? [selectedPath] : [];This is confusing — selectedPath becomes dead weight when selectedPaths is provided. Cleaner to just change the prop to paths: string[] and update the one call site.
InitGitDialog now closes on partial failure (behavior change)
Original code: if any error occurred during git init, the function returned early and the dialog stayed open.
New code:
if (errors.length > 0) {
onError(`Failed to initialize ${errors.length} folder(s): ...`);
}
onClose(); // Always called — even with errorsThe dialog now closes even on partial failure. Is this intentional? The UX flow changes from "fix the problem and retry" to "dialog disappears and you see an error banner."
Single-project toast is unnecessary noise
When the user selects exactly 1 folder, they now see a "Project opened" toast. The old behavior just navigated directly. A single selection doesn't need toast confirmation — the navigation itself is the feedback. Consider suppressing the toast for successes.length === 1 cases, or at least in StartView where navigation is the primary action.
WorkspaceSidebarFooter lost its loading feedback
Old code:
toast.promise(createBranchWorkspace.mutateAsync(...), {
loading: "Opening project...",
success: "Project opened",
error: (err) => ...
});New code creates workspaces in a silent loop with no loading indicator. For multiple projects this could be noticeable.
Scope creep: unrelated changes
Two changes are unrelated to multi-folder selection and would be better as a separate PR:
SKIP_ENV_VALIDATIONbypass insign-in/page.tsxandlayout.tsx— modifies authentication flowgetGitHubUsernamecaching ingit.ts— adds a 5-minute TTL cache
These are fine changes on their own, but mixing auth flow modifications with a UI feature makes the PR harder to review and bisect.
Minor nits
selectedPath.split("/").pop()appears 4 times across the codebase for getting the folder basename. There's already agetBasename()utility inInitGitDialog.tsx— reuse it or extract it to a shared location.- The
getGitHubUsernamecache is a module-level mutable singleton. Consider that this cache never invalidates on logout/user-switch. Anullvalue cached for 5 minutes means failed auth will be sticky.
Summary
The core feature works and the approach of collecting per-folder outcomes with independent error handling is solid. The main issues to address are:
- Type safety — use a proper discriminated union instead of structural checks
- DRY — extract the duplicated outcome handling
InitGitDialogAPI — simplify the redundant prop interface- Scope — split unrelated changes into a separate PR
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
PR Review: feat(desktop): support multi-folder selection in Open Project dialog
Thanks for contributing this @quuu! Multi-folder selection is a genuinely useful quality-of-life improvement. Here's a detailed review:
Overall Assessment
The core feature — enabling multiSelections in the native dialog and processing each folder independently — is well-conceived. The per-folder FolderOutcome type system is clean. However, there are several issues ranging from behavioral regressions to code quality concerns that should be addressed before merging.
Issues
1. InitGitDialog closes on errors (behavioral regression)
In InitGitDialog.tsx, after the loop processes all paths, onClose() is always called — even if every git init failed:
if (errors.length > 0) {
onError(`Failed to initialize ${errors.length} folder(s): ${errors.join("; ")}`);
}
onClose(); // ← always closes, even on total failureThe old behavior would return early on error, keeping the dialog open so the user could retry. Now, if all initializations fail, the dialog dismisses itself and the user just sees an error toast. Consider only calling onClose() when at least one path succeeded, or when there are no errors.
2. InitGitDialog prop API is confusing — dual selectedPath + selectedPaths
The component now accepts both selectedPath: string (required) and selectedPaths?: string[] (optional), with internal normalization logic:
const allPaths =
selectedPaths && selectedPaths.length > 0
? selectedPaths
: selectedPath
? [selectedPath]
: [];This creates a confusing contract where callers must always pass selectedPath even when using selectedPaths. Consider simplifying to a single paths: string[] prop (breaking change is fine since this is an internal component).
3. Duplicated multi-result handling across 3 components
The same pattern — filter successes/needsGitInit/errors from result.results, then show toasts — is copy-pasted across:
StartView/index.tsxWorkspaceSidebarFooter.tsxNewWorkspaceModal.tsx
Per AGENTS.md's "three instances" heuristic, this has hit the extraction threshold. Consider a shared utility like:
function processMultiOpenResult(results: FolderOutcome[]) {
return {
successes: results.filter(r => r.status === "success"),
needsGitInit: results.filter(r => r.status === "needsGitInit"),
errors: results.filter(r => r.status === "error"),
};
}Or a custom hook that also handles the toast logic.
4. WorkspaceSidebarFooter lost toast.promise loading state
The original code gave users a loading indicator via toast.promise:
toast.promise(createBranchWorkspace.mutateAsync({ projectId: result.project.id }), {
loading: "Opening project...",
success: "Project opened",
error: (err) => ...
});The new code fires createBranchWorkspace.mutateAsync in a sequential loop with only error toasts. For single-project selection (the common case), the user no longer sees the "Opening project..." loading toast. This is a UX regression.
5. Sequential createBranchWorkspace calls in WorkspaceSidebarFooter
for (const s of successes) {
try {
await createBranchWorkspace.mutateAsync({ projectId: s.project.id });
} catch (err) { ... }
}This processes workspace creation sequentially. For 5+ projects, this could feel slow. Consider Promise.allSettled for parallelism (if the backend supports concurrent workspace creation).
6. Unrelated changes bundled in — SKIP_ENV_VALIDATION auth bypass
The layout.tsx and sign-in/page.tsx changes are unrelated to multi-folder selection:
// layout.tsx
if (isPending && !env.SKIP_ENV_VALIDATION) { ... }
// sign-in/page.tsx
if (env.SKIP_ENV_VALIDATION) {
return <Navigate to="/workspace" replace />;
}These should ideally be in a separate PR to keep the review scope focused. Additionally, SKIP_ENV_VALIDATION semantically means "skip Zod env schema validation" — using it to bypass authentication is a semantic mismatch that could confuse other developers. Consider a dedicated SKIP_AUTH env var if this is intentional.
7. Sequential git init in InitGitDialog.handleInitGit
Similar to point 5, the for (const path of allPaths) loop processes git inits sequentially. Promise.allSettled would speed up batch initialization.
Minor / Nits
-
GitHub username caching — The 5-minute in-memory cache is reasonable for dev experience. Note that it caches
null(failed lookups) for 5 minutes too, so if a user runsgh auth loginduring a session, they'd need to wait up to 5 min. A shorter TTL for failures (e.g. 30s) might be friendlier. -
"error" in result && !("multi" in result)check — SinceOpenNewMultiSuccessdoesn't have anerrorfield, the!("multi" in result)guard is technically redundant. Not a bug, but it suggests the discriminated union could use atypediscriminant field for cleaner narrowing (e.g.type: "multi" | "error" | "canceled"). -
openFromPathstill returnsOpenNewResult— The drag-and-drop handler inStartView(handleDrop) still uses the old single-select response format fromopenFromPath. This is correct since onlyopenNewwas changed, but worth noting that the two related procedures now return different shapes, which increases cognitive overhead.
What Looks Good
- Clean
FolderOutcomeunion type design - Error handling per-folder in the backend (instead of failing the entire batch)
- Summary toasts with proper singular/plural messaging
- The
getGitHubUsernamecaching addresses a real dev pain point - Good test coverage description in the PR body
Suggested Priority
- Fix
InitGitDialogclosing on errors (regression) - Extract duplicated multi-result handling
- Simplify
InitGitDialogprops - Restore loading toast in WorkspaceSidebarFooter
- Split out
SKIP_ENV_VALIDATIONchanges to a separate PR
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
PR Review: feat(desktop): support multi-folder selection in Open Project dialog
Nice feature — multi-folder selection removes real friction from the onboarding flow. The per-folder error handling approach is solid. Here are my findings:
Bug: Navigate + InitGitDialog race condition (StartView/index.tsx)
When the user selects a mix of git repos and non-git folders, the code:
- Navigates to the first successful project (line ~76-82)
- Opens InitGitDialog for the non-git folders (line ~93-100)
But navigating away from StartView unmounts it, which unmounts InitGitDialog. The dialog will flash briefly or never appear at all. The navigation should be deferred until after the InitGitDialog is dismissed, or the dialog should live higher in the component tree.
Design: Fragile type discrimination
All 3 UI call sites use this pattern:
if ("error" in result && !("multi" in result)) { ... }This is brittle — adding a new field to any variant could break it. A proper discriminated union would be much safer:
type OpenNewMultiResult =
| { type: "canceled" }
| { type: "multi"; results: FolderOutcome[] }
| { type: "error"; error: string };Then consumers use switch (result.type) — no guessing about property existence.
Design: Duplicated multi-result handling across 3 components
The exact same pattern appears in StartView/index.tsx, WorkspaceSidebarFooter.tsx, and NewWorkspaceModal.tsx:
- Filter
successes/needsGitInit/errors - Show summary toast
- Show per-error toasts
- Show git-init warnings
This should be extracted to a shared helper (e.g., processOpenNewMultiResult(results, callbacks)) to keep things DRY. Right now each call site is ~30 lines of nearly identical logic.
Bug: selectedPath.split("/").pop() is cross-platform unsafe
Used in 3 places but doesn't handle Windows backslash paths. InitGitDialog.tsx already has a getBasename helper that handles both separators — reuse it or promote it to a shared utility.
Nit: InitGitDialog prop API is awkward
The dialog now takes both selectedPath: string and selectedPaths?: string[], with normalization logic inside:
const allPaths =
selectedPaths && selectedPaths.length > 0
? selectedPaths
: selectedPath ? [selectedPath] : [];Simpler to just accept selectedPaths: string[] and update callers. The dual-prop API adds confusion about which one takes precedence.
Nit: Generic error message in projects.ts catch block
outcomes.push({
status: "error",
selectedPath,
error: "Failed to open project", // <-- real error is logged but not returned
});The actual error message is logged to console but the user sees a generic string. Consider including the real error message (like the pattern used elsewhere: error instanceof Error ? error.message : String(error)).
Nit: WorkspaceSidebarFooter lost loading UX
The old code used toast.promise(createBranchWorkspace.mutateAsync(...)) which showed a "Opening project..." loading state. The new code runs createBranchWorkspace in a loop with no loading indicator per-project. For multiple projects this could feel unresponsive.
Scope: SKIP_ENV_VALIDATION changes are unrelated
The SKIP_ENV_VALIDATION bypass in layout.tsx and sign-in/page.tsx is a dev experience fix unrelated to multi-folder selection. As CodeRabbit also noted, repurposing SKIP_ENV_VALIDATION (which semantically means "skip Zod env schema validation") to bypass authentication entirely is a naming mismatch with security implications. Consider:
- Splitting this into a separate PR
- Using a dedicated flag like
DEV_SKIP_AUTHwith an explicitNODE_ENV !== 'production'guard
Similarly, the GitHub username caching change in git.ts is a good improvement but should be its own PR.
Summary
The core multi-select feature is well-implemented. The main items to address before merging:
- Fix the navigate + InitGitDialog race (bug — dialog gets unmounted)
- Extract shared multi-result handling (3x duplication)
- Use proper discriminated union types (fragile
"error" in result && !("multi" in result)pattern) - Split unrelated changes (SKIP_ENV_VALIDATION bypass, GitHub username caching) into separate PRs
|
Hey sorry claude code dropped a comment on everything as my review process. im disabling that. |
|
Hey I had some changes but could not push to your branch. Will merge in a quick follow up PR |
2 participants
Submitting this PR because I was trying out superset but ran into friction when I had to import my projects 1 by 1 via the native file selector
This PR enables me to select multiple folders to all upload as projects
Video below
CleanShot.2026-02-05.at.20.55.37.mp4
Note: All of this was written by Opencode Opus4.6, but I reviewed and tested the changes
Summary
Changes
Core: Multi-select backend (
projects.ts)multiSelectionstodialog.showOpenDialogpropertiesFolderOutcomeandOpenNewMultiResulttypes for per-folder resultsopenNewmutation now iterates all selected paths, catching errors per-folder instead of failing entirelyUI: All 4 call sites updated
Dev experience fixes (incidental)
SKIP_ENV_VALIDATIONbypass — was broken becauseisPendingguard and sign-in page redirected before the bypass could take effectgetGitHubUsernamecaching — added 5-minute in-memory cache to avoid GitHub API rate limit spam during devTesting
Summary by CodeRabbit
Release Notes
New Features
Improvements