feat(desktop): add New Project page with empty, clone, and template tabs#1623
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a dedicated New Project onboarding page with "Empty" and "Clone" tabs, new UI components (PathSelector, EmptyRepoTab, CloneRepoTab), a shared project-creation hook, TRPC procedures for directory selection and empty-repo creation, and replaces in-place clone dialogs with navigation to the onboarding page. Changes
sequenceDiagram
participant User as User
participant UI as NewProjectPage<br/>(EmptyRepoTab / CloneRepoTab / PathSelector)
participant TRPC as TRPC Client
participant Backend as projects Router
participant Hook as useProjectCreationHandler
participant Workspace as createWorkspace Mutation
User->>UI: Fill name or URL + parentDir, submit
UI->>TRPC: call mutation (createEmptyRepo / cloneRepo)
TRPC->>Backend: projects.createEmptyRepo / projects.cloneRepo
Backend->>Backend: validate input, create dir, init git, upsert project
Backend-->>TRPC: return { success, canceled?, project?, error? }
TRPC-->>UI: deliver result
alt success & project
UI->>Hook: handleResult(result)
Hook->>Hook: invalidate recents cache
Hook->>Workspace: createWorkspace({ projectId })
Workspace-->>Hook: workspace created
Hook-->>UI: success -> reset UI state
UI->>User: navigate to workspace
else error or canceled
UI->>Hook: handleError / display error
Hook-->>UI: forward message
UI->>User: show error banner
end
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarFooter.tsx (1)
100-100: Inconsistent Tailwind size shorthand onLuFolderPlus.The collapsed view uses
size-4(line 66), but the expanded view usesw-4 h-4(line 100) for the same icon.size-4is the idiomatic shorthand.✏️ Proposed fix
- <LuFolderPlus className="w-4 h-4" strokeWidth={STROKE_WIDTH} /> + <LuFolderPlus className="size-4" strokeWidth={STROKE_WIDTH} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarFooter.tsx` at line 100, The icon usage for LuFolderPlus is inconsistent: the collapsed view uses the Tailwind shorthand class "size-4" while the expanded view (in WorkspaceSidebarFooter, symbol LuFolderPlus with STROKE_WIDTH) uses "w-4 h-4"; replace the "w-4 h-4" usage on LuFolderPlus in the expanded state with the "size-4" shorthand to match the collapsed view and keep icon sizing consistent across the component.apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/components/EmptyRepoTab/EmptyRepoTab.tsx (1)
58-62: Consider adding anaria-liveregion for the loading state transition.When the button text switches from "Create Repository" to "Creating…", screen-reader users won't be notified of the change unless there is a live region. This is a minor accessibility gap.
♿ Optional enhancement
+ <span className="sr-only" aria-live="polite" aria-atomic="true"> + {isLoading ? "Creating repository, please wait…" : ""} + </span> <div className="flex justify-end gap-2"> <Button onClick={handleCreate} disabled={isLoading} size="sm"> {isLoading ? "Creating..." : "Create Repository"} </Button> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/components/EmptyRepoTab/EmptyRepoTab.tsx` around lines 58 - 62, The Create Repository button in EmptyRepoTab toggles text via isLoading but lacks an aria-live update for screen readers; update the Button rendering (and/or surrounding div) to expose the loading state: add an aria-live="polite" (or role="status") visually-hidden element that mirrors the current label (using isLoading ? "Creating…" : "Create Repository"), and also set aria-busy={isLoading} on the Button component (or add aria-disabled when appropriate) so screen readers are notified of the transition; touch only the Button render in EmptyRepoTab and the handleCreate/isLoading plumbing as needed.apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx (1)
44-63: Custom tab bar is missing ARIA tab semantics.The tab strip uses plain
<button>elements with norole="tablist"/role="tab"/aria-selected/aria-controls, so assistive technologies cannot identify the pattern as a tab widget.♿ Proposed fix
-<div className="flex p-0.5 bg-muted rounded-md"> +<div role="tablist" className="flex p-0.5 bg-muted rounded-md"> {TABS.map((tab) => ( <button key={tab.mode} type="button" + role="tab" + aria-selected={mode === tab.mode} onClick={() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx` around lines 44 - 63, The custom tab strip built from TABS needs ARIA tab semantics: wrap the buttons container (where TABS.map runs) with role="tablist" and give each button role="tab", aria-selected={mode === tab.mode}, id={`tab-${tab.mode}`} and aria-controls={`tabpanel-${tab.mode}`}; ensure the corresponding tab panel elements use role="tabpanel", id={`tabpanel-${tab.mode}`} and aria-labelledby={`tab-${tab.mode}`}; keep the existing state updates via setMode and setError unchanged but add these attributes so assistive tech recognizes the widget.apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/components/TemplateRepoTab/TemplateRepoTab.tsx (1)
52-77: Template selection buttons should exposearia-pressedfor toggle state.The selected/deselected state is conveyed only through CSS classes; screen readers cannot determine which template is active.
♿ Proposed fix
<button key={template.id} type="button" + aria-pressed={selectedTemplate?.id === template.id} onClick={() => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/components/TemplateRepoTab/TemplateRepoTab.tsx` around lines 52 - 77, The template buttons currently toggle selection via CSS only; update the button elements in TemplateRepoTab (the map over PROJECT_TEMPLATES) to expose the toggle state by adding aria-pressed={selectedTemplate?.id === template.id} so screen readers can detect which template is active; ensure the value is a boolean and remains synced with setSelectedTemplate/selectedTemplate changes so the accessible state always reflects the visual state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 984-995: The createFromTemplate flow can leave a partially-cloned
repo on disk if rm(join(repoPath, ".git")...) or initGitRepo(repoPath, ...)
throws; update createFromTemplate to ensure repoPath is removed on any error
after a successful clone: after calling simpleGit().clone(...) mark that the
clone completed (or wrap clone+post steps in a try/catch) and in the catch (or
finally on error) call a safe cleanup to remove the entire repoPath directory
(e.g., await rm(repoPath, { recursive: true, force: true })) so retries aren’t
blocked; reference the symbols simpleGit().clone, repoPath, rm(join(repoPath,
".git"), ...), and initGitRepo to locate where to add the cleanup and ensure
errors from rm or initGitRepo still trigger repository removal.
- Around line 238-281: Remove the stale duplicate async function initGitRepo
that returns Promise<string> (the second declaration shown in the diff) so there
is only one initGitRepo implementation in the module; keep the intended
refactored version that returns Promise<{ defaultBranch: string }>, ensuring all
call sites that do const { defaultBranch } = await initGitRepo(...) continue to
work and compile without TS2393 duplicate-function errors.
- Around line 936-940: The createFromTemplate handler currently accepts an
optional trimmed name without validating it, then uses join(parentDir,
input.name), allowing path traversal; update createFromTemplate to validate the
user-supplied name using the same SAFE_REPO_NAME_REGEX used by createEmptyRepo
(e.g., add .refine(SAFE_REPO_NAME_REGEX) to the z.string() schema or perform an
equivalent runtime check) and reject or normalize invalid names before calling
join(parentDir, input.name) so input.name cannot contain `..` or other unsafe
segments.
- Around line 861-916: The handler creates repoPath with mkdir before calling
initGitRepo, so if initGitRepo fails an empty directory is left behind; wrap the
mkdir + initGitRepo sequence in its own try/catch (or try/finally) surrounding
initGitRepo and on failure remove the created repoPath (use fs.rm or fs.rmdir
with recursive/force) before returning the error; refer to the repoPath
variable, the mkdir call, and the initGitRepo call and ensure cleanup runs only
when the directory was created (and remove before returning or rethrowing the
caught error so upsertProject/ensureMainWorkspace are not invoked).
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/constants.ts`:
- Around line 12-41: The three template entries with ids "nextjs", "vite-react",
and "astro" use GitHub web UI subdirectory URLs that git.clone in
createFromTemplate cannot clone; update their url fields to point to cloneable
repository roots or dedicated template repos (e.g., a create-next-app or
official Vite/Astro template repo) instead of /tree/... paths so
git.clone(input.templateUrl, repoPath, ["--depth","1"]) works; change the url
values in the objects for ids "nextjs", "vite-react", and "astro" accordingly
while leaving "express" as-is.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/hooks/useProjectCreationHandler/useProjectCreationHandler.ts`:
- Around line 23-26: The createWorkspace.mutate call is invoked without error
handlers and resetState is run immediately, which drops the spinner and clears
the form even if workspace creation fails; update the createWorkspace.mutate
invocation (in the block that checks result.success && result.project) to pass
onSuccess/onError/onSettled callbacks: move resetState?.() into onSuccess so the
form only clears on success, add an onError handler that surfaces the failure
(e.g., call your error/toast handler or set an error state) and ensure onSettled
invalidates utils.projects.getRecents (or clears pending state) so spinner state
is handled correctly; reference createWorkspace.mutate, resetState,
utils.projects.getRecents.invalidate and result.project.id when making the
change.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx`:
- Around line 34-40: The back button is icon-only and lacks accessible text;
update the button element that calls navigate (the button wrapping LuArrowLeft)
to include an appropriate aria-label (e.g., "Back" or "Go back") so screen
readers have a meaningful description; you can also add a title attribute for
tooltip support but ensure aria-label is present on the button that uses
navigate({ to: "/", replace: true }).
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarFooter.tsx`:
- Line 64: The "New project" button is being disabled by the combined isLoading
flag (isOpenPending || createBranchWorkspace.isPending) even though navigate({
to: "/new-project" }) is pure client navigation; remove isLoading from the
disabled prop for the "New project" menu item and ensure only the "Open project"
item uses the loading condition (isOpenPending ||
createBranchWorkspace.isPending). Apply this change in both the collapsed and
expanded branches of WorkspaceSidebarFooter where the menu items are rendered so
"New project" is always enabled while "Open project" remains disabled during
open/create pending states.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/components/EmptyRepoTab/EmptyRepoTab.tsx`:
- Around line 58-62: The Create Repository button in EmptyRepoTab toggles text
via isLoading but lacks an aria-live update for screen readers; update the
Button rendering (and/or surrounding div) to expose the loading state: add an
aria-live="polite" (or role="status") visually-hidden element that mirrors the
current label (using isLoading ? "Creating…" : "Create Repository"), and also
set aria-busy={isLoading} on the Button component (or add aria-disabled when
appropriate) so screen readers are notified of the transition; touch only the
Button render in EmptyRepoTab and the handleCreate/isLoading plumbing as needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/components/TemplateRepoTab/TemplateRepoTab.tsx`:
- Around line 52-77: The template buttons currently toggle selection via CSS
only; update the button elements in TemplateRepoTab (the map over
PROJECT_TEMPLATES) to expose the toggle state by adding
aria-pressed={selectedTemplate?.id === template.id} so screen readers can detect
which template is active; ensure the value is a boolean and remains synced with
setSelectedTemplate/selectedTemplate changes so the accessible state always
reflects the visual state.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx`:
- Around line 44-63: The custom tab strip built from TABS needs ARIA tab
semantics: wrap the buttons container (where TABS.map runs) with role="tablist"
and give each button role="tab", aria-selected={mode === tab.mode},
id={`tab-${tab.mode}`} and aria-controls={`tabpanel-${tab.mode}`}; ensure the
corresponding tab panel elements use role="tabpanel",
id={`tabpanel-${tab.mode}`} and aria-labelledby={`tab-${tab.mode}`}; keep the
existing state updates via setMode and setError unchanged but add these
attributes so assistive tech recognizes the widget.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarFooter.tsx`:
- Line 100: The icon usage for LuFolderPlus is inconsistent: the collapsed view
uses the Tailwind shorthand class "size-4" while the expanded view (in
WorkspaceSidebarFooter, symbol LuFolderPlus with STROKE_WIDTH) uses "w-4 h-4";
replace the "w-4 h-4" usage on LuFolderPlus in the expanded state with the
"size-4" shorthand to match the collapsed view and keep icon sizing consistent
across the component.
Comment on lines
+12
to
+41
| { | ||
| id: "nextjs", | ||
| name: "Next.js", | ||
| description: | ||
| "React framework with App Router, TypeScript, and Tailwind CSS", | ||
| url: "https://github.com/vercel/next.js/tree/canary/examples/hello-world", | ||
| tags: ["react", "typescript"], | ||
| }, | ||
| { | ||
| id: "vite-react", | ||
| name: "Vite + React", | ||
| description: "Fast React development with Vite, TypeScript, and ESLint", | ||
| url: "https://github.com/vitejs/vite/tree/main/packages/create-vite/template-react-ts", | ||
| tags: ["react", "typescript"], | ||
| }, | ||
| { | ||
| id: "astro", | ||
| name: "Astro", | ||
| description: | ||
| "Content-focused static site generator with island architecture", | ||
| url: "https://github.com/withastro/astro/tree/main/examples/minimal", | ||
| tags: ["static", "typescript"], | ||
| }, | ||
| { | ||
| id: "express", | ||
| name: "Express", | ||
| description: "Minimal Node.js web framework for APIs and web apps", | ||
| url: "https://github.com/expressjs/generator", | ||
| tags: ["node", "api"], | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can you git clone a GitHub URL with /tree/<branch>/<path> subdirectory?
💡 Result:
No. A GitHub URL like https://github.com/<owner>/<repo>/tree/<branch>/<path> is a web UI URL, not a Git remote URL, so git clone can’t clone “just that subdirectory”.
If you want only that subdirectory in your working tree, do a normal clone (optionally partial) and use sparse checkout:
git clone --filter=blob:none --no-checkout --branch <branch> https://github.com/<owner>/<repo>.git
cd <repo>
git sparse-checkout init --cone
git sparse-checkout set <path>
git checkout <branch>git sparse-checkout is the built-in feature for restricting your working tree to specific directories. [1][2] The --filter=blob:none option makes the clone “blobless” (downloads file contents on demand) to reduce transfer. [3]
If you only need a “download/export” of a folder (not a real Git clone), you can use GitHub’s SVN bridge, e.g. svn export .../trunk/<path> (or under /branches/<branch>/...). [4]
Sources: git-sparse-checkout docs [1], GitHub sparse-checkout guide [2], git-clone --filter docs [3], GitHub SVN export example [4].
🏁 Script executed:
# Find the createFromTemplate function to verify how templates are actually cloned
fd -t f "\.ts" -o -t f "\.tsx" apps/desktop/src | xargs rg -l "createFromTemplate" | head -5Repository: superset-sh/superset
Length of output: 371
🏁 Script executed:
# Search for git.clone usage to understand the template cloning mechanism
rg "git\.clone|git clone" apps/desktop/src -A 3 -B 1Repository: superset-sh/superset
Length of output: 938
🏁 Script executed:
# Read the createFromTemplate function to see full context
cat -n apps/desktop/src/lib/trpc/routers/projects/projects.ts | head -200Repository: superset-sh/superset
Length of output: 7143
🏁 Script executed:
# Read the constants file to confirm the URLs
cat -n apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/constants.tsRepository: superset-sh/superset
Length of output: 1496
🏁 Script executed:
# Find the createFromTemplate function in projects.ts
rg -A 40 "createFromTemplate|template.*procedure" apps/desktop/src/lib/trpc/routers/projects/projects.ts | head -100Repository: superset-sh/superset
Length of output: 1078
🏁 Script executed:
# Look for where PROJECT_TEMPLATES is imported and used
rg "PROJECT_TEMPLATES" apps/desktop/src -B 2 -A 5Repository: superset-sh/superset
Length of output: 3418
Three template URLs are GitHub web browser UI URLs that cannot be git cloned.
createFromTemplate calls git.clone(input.templateUrl, repoPath, ["--depth", "1"]). GitHub only serves git smart-HTTP at repository roots (https://github.com/user/repo); the /tree/<branch>/path/to/dir variant is a web-only URL that git cannot clone. Attempting to clone these will fail with a repository not found error, making the Next.js, Vite+React, and Astro templates non-functional.
| Template | URL pattern | Works? |
|---|---|---|
| Next.js | .../tree/canary/examples/hello-world |
❌ |
| Vite + React | .../tree/main/packages/create-vite/template-react-ts |
❌ |
| Astro | .../tree/main/examples/minimal |
❌ |
| Express | https://github.com/expressjs/generator |
✅ |
Replace each URL with the repository root, or use dedicated template repositories. For example: point to create-next-app's template repository instead of a subdirectory within the main next.js repo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/constants.ts`
around lines 12 - 41, The three template entries with ids "nextjs",
"vite-react", and "astro" use GitHub web UI subdirectory URLs that git.clone in
createFromTemplate cannot clone; update their url fields to point to cloneable
repository roots or dedicated template repos (e.g., a create-next-app or
official Vite/Astro template repo) instead of /tree/... paths so
git.clone(input.templateUrl, repoPath, ["--depth","1"]) works; change the url
values in the objects for ids "nextjs", "vite-react", and "astro" accordingly
while leaving "express" as-is.
Comment on lines
+23
to
+26
| if (result.success && result.project) { | ||
| utils.projects.getRecents.invalidate(); | ||
| createWorkspace.mutate({ projectId: result.project.id }); | ||
| resetState?.(); |
Contributor
There was a problem hiding this comment.
createWorkspace.mutate() has no onError callback — workspace creation failures are silently lost.
If the createWorkspace mutation rejects, isPending drops to false (hiding the spinner) but onError is never invoked, so the user receives no feedback. resetState?.() is also called unconditionally immediately after .mutate(), clearing the form even on failure.
🐛 Proposed fix
- utils.projects.getRecents.invalidate();
- createWorkspace.mutate({ projectId: result.project.id });
- resetState?.();
+ utils.projects.getRecents.invalidate();
+ createWorkspace.mutate(
+ { projectId: result.project.id },
+ {
+ onSuccess: () => resetState?.(),
+ onError: (err) =>
+ onError(err.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.
Suggested change
| if (result.success && result.project) { | |
| utils.projects.getRecents.invalidate(); | |
| createWorkspace.mutate({ projectId: result.project.id }); | |
| resetState?.(); | |
| if (result.success && result.project) { | |
| utils.projects.getRecents.invalidate(); | |
| createWorkspace.mutate( | |
| { projectId: result.project.id }, | |
| { | |
| onSuccess: () => resetState?.(), | |
| onError: (err) => | |
| onError(err.message || "Failed to open project"), | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/hooks/useProjectCreationHandler/useProjectCreationHandler.ts`
around lines 23 - 26, The createWorkspace.mutate call is invoked without error
handlers and resetState is run immediately, which drops the spinner and clears
the form even if workspace creation fails; update the createWorkspace.mutate
invocation (in the block that checks result.success && result.project) to pass
onSuccess/onError/onSettled callbacks: move resetState?.() into onSuccess so the
form only clears on success, add an onError handler that surfaces the failure
(e.g., call your error/toast handler or set an error state) and ensure onSettled
invalidates utils.projects.getRecents (or clears pending state) so spinner state
is handled correctly; reference createWorkspace.mutate, resetState,
utils.projects.getRecents.invalidate and result.project.id when making the
change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/desktop/src/lib/trpc/routers/settings/index.ts (1)
558-571:setDefaultProjectPath:pathshould enforce.min(1)to prevent persisting an empty string.
z.string()alone allows an empty string. While the client-side hook guards against this, the tRPC procedure is a public API surface. Consistent withcreateEmptyRepo'sparentDir: z.string().min(1)inprojects.ts:♻️ Proposed fix
setDefaultProjectPath: publicProcedure - .input(z.object({ path: z.string() })) + .input(z.object({ path: z.string().min(1) })) .mutation(({ input }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/settings/index.ts` around lines 558 - 571, The procedure setDefaultProjectPath currently accepts an empty string because it uses input(z.object({ path: z.string() })); change the input schema to require a non-empty string (e.g., z.string().min(1)) so the tRPC surface validates and rejects empty paths before calling localDb.insert/update; update the schema in the setDefaultProjectPath procedure (the input for the publicProcedure) accordingly and keep the rest of the mutation (localDb.insert(...).onConflictDoUpdate(...).run()) unchanged.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
672-682:.refine({ message: ... })is deprecated in Zod v4 — prefer{ error: ... }.The project uses Zod v4.3.5. Zod v4 replaces the
messageparam witherror; the oldmessageparameter is still supported but deprecated. Three.refine()call sites use the old form:
- Lines 672–682:
cloneRepoURL validation- Lines 828–831:
createEmptyReponame validation- Lines 904–913:
createFromTemplateURL validation♻️ Proposed updates (shown for one site; apply the same pattern to all three)
.refine( (val) => { ... }, - { message: "Must be a valid Git URL (HTTPS or SSH)" }, + { error: "Must be a valid Git URL (HTTPS or SSH)" }, ),.refine((val) => SAFE_REPO_NAME_REGEX.test(val), { - message: "Name can only contain letters, numbers, dots, underscores, hyphens, and spaces", + error: "Name can only contain letters, numbers, dots, underscores, hyphens, and spaces", }),Also applies to: 828-831, 904-913
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts` around lines 672 - 682, Replace the deprecated { message: ... } option passed to zod .refine(...) with the new { error: { message: ... } } shape at each refine site: update the cloneRepo URL validation (the refine block around ALLOWED_URL_PROTOCOLS/SSH_GIT_URL_REGEX), the createEmptyRepo name validation, and the createFromTemplate URL validation to use { error: { message: "..." } } instead of { message: "..." } so Zod v4's new API is used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/components/PathSelector/PathSelector.tsx`:
- Around line 44-53: The icon-only browse Button (component Button with
onClick={handleBrowse}, disabled={disabled || selectDirectory.isPending} and
child <LuFolderOpen/>) lacks an accessible label; update the Button to include a
descriptive aria-label (e.g., "Browse for project folder" or similar) so screen
readers announce its purpose, making sure the aria-label string accurately
reflects the button's action and remains present even when disabled or pending.
- Around line 29-43: PathSelector currently hardcodes id="project-path" on the
label and Input which can create duplicate IDs when multiple PathSelector
instances are mounted; update the PathSelector component signature to accept an
optional id prop (e.g., id?: string) and use that prop for label htmlFor and
Input id, and if no id is passed generate a stable unique id inside the
component (use React's useId or a simple uniqueId helper) so each instance (and
the label→input association) remains unique; update any call sites that pass an
id if needed and ensure the prop is forwarded to the Input element rather than
the hardcoded "project-path".
---
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 915-919: The createFromTemplate handler currently accepts a
user-supplied name (zod field transformed to undefined for empty) but does not
enforce SAFE_REPO_NAME_REGEX, allowing path traversal via values like
"../../etc"; update the zod schema for name in createFromTemplate to validate
against SAFE_REPO_NAME_REGEX (same check used in createEmptyRepo) or add a
refinement step that rejects names not matching SAFE_REPO_NAME_REGEX (and
specifically disallow "." or ".." segments) before using it in join(parentDir,
repoName) so repoName cannot escape parentDir.
- Around line 969-979: The createFromTemplate flow can leave repoPath on disk if
rm(join(repoPath, ".git")) or initGitRepo(repoPath, ...) throws after git.clone;
wrap the post-clone steps so any exception after a successful clone cleans up
the partially-created repo directory (repoPath) before rethrowing/returning the
error. Specifically, after calling simpleGit().clone(...) and before calling
initGitRepo, ensure you catch errors from rm(...) and initGitRepo(...) and
remove repoPath (e.g., rm(repoPath, { recursive: true, force: true })) in that
error path or use a try/finally that deletes repoPath on failure; update
createFromTemplate to reference git.clone, rm, initGitRepo and repoPath
accordingly.
- Around line 871-895: The createEmptyRepo flow currently mkdirs repoPath then
calls initGitRepo; if initGitRepo throws the new empty directory is left on
disk. Modify createEmptyRepo so that immediately after mkdir(repoPath, {
recursive: true }) you either wrap initGitRepo in its own try/catch or add a
cleanup path in the outer catch that, when initGitRepo fails (i.e., before
upsertProject/ensureMainWorkspace/track run), removes the repoPath directory
(use fs.rm or fs.rmdir with recursive:true or equivalent) before returning the
error; ensure removal errors are swallowed/logged but don't mask the original
init error. Reference: initGitRepo, upsertProject, ensureMainWorkspace,
repoPath.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 672-682: Replace the deprecated { message: ... } option passed to
zod .refine(...) with the new { error: { message: ... } } shape at each refine
site: update the cloneRepo URL validation (the refine block around
ALLOWED_URL_PROTOCOLS/SSH_GIT_URL_REGEX), the createEmptyRepo name validation,
and the createFromTemplate URL validation to use { error: { message: "..." } }
instead of { message: "..." } so Zod v4's new API is used.
In `@apps/desktop/src/lib/trpc/routers/settings/index.ts`:
- Around line 558-571: The procedure setDefaultProjectPath currently accepts an
empty string because it uses input(z.object({ path: z.string() })); change the
input schema to require a non-empty string (e.g., z.string().min(1)) so the tRPC
surface validates and rejects empty paths before calling localDb.insert/update;
update the schema in the setDefaultProjectPath procedure (the input for the
publicProcedure) accordingly and keep the rest of the mutation
(localDb.insert(...).onConflictDoUpdate(...).run()) unchanged.
Comment on lines
+29
to
+43
| <div> | ||
| <label | ||
| htmlFor="project-path" | ||
| className="block text-sm font-medium text-foreground mb-2" | ||
| > | ||
| Location | ||
| </label> | ||
| <div className="flex gap-2"> | ||
| <Input | ||
| id="project-path" | ||
| value={value} | ||
| onChange={(e) => onChange(e.target.value)} | ||
| disabled={disabled} | ||
| className="flex-1 font-mono text-xs" | ||
| /> |
Contributor
There was a problem hiding this comment.
Hardcoded id="project-path" will produce duplicate IDs if multiple PathSelector instances are in the DOM simultaneously.
All three tabs (Empty, Clone, Template) are likely to use PathSelector. If the tab implementation keeps all panels mounted (e.g., CSS-based visibility), the browser ends up with multiple elements sharing id="project-path", which breaks the label→input association and fails HTML uniqueness requirements.
Accept an optional id prop (or derive it from a passed label prop) to guarantee uniqueness per instance:
🐛 Proposed fix
interface PathSelectorProps {
value: string;
onChange: (path: string) => void;
disabled?: boolean;
+ id?: string;
+ label?: string;
}
-export function PathSelector({ value, onChange, disabled }: PathSelectorProps) {
+export function PathSelector({ value, onChange, disabled, id = "project-path", label = "Location" }: PathSelectorProps) {
...
return (
<div>
<label
- htmlFor="project-path"
+ htmlFor={id}
...
>
- Location
+ {label}
</label>
<div className="flex gap-2">
<Input
- id="project-path"
+ id={id}
...
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/components/PathSelector/PathSelector.tsx`
around lines 29 - 43, PathSelector currently hardcodes id="project-path" on the
label and Input which can create duplicate IDs when multiple PathSelector
instances are mounted; update the PathSelector component signature to accept an
optional id prop (e.g., id?: string) and use that prop for label htmlFor and
Input id, and if no id is passed generate a stable unique id inside the
component (use React's useId or a simple uniqueId helper) so each instance (and
the label→input association) remains unique; update any call sites that pass an
id if needed and ensure the prop is forwarded to the Input element rather than
the hardcoded "project-path".
Replace the CloneRepoDialog with a dedicated /new-project route that supports creating empty repos, cloning, and creating from templates. Adds a shared path selector defaulting to ~/.superset/projects.
Kitenite
force-pushed
the
kitenite/create-new-repo
branch
from
3e4696d to
d1420ed
Compare
Kitenite
changed the title
Kitenite
force-pushed
the
kitenite/create-new-repo
branch
from
d1420ed to
c47125b
Compare
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/constants.ts (1)
10-10: Consider adding aTODOannotation toPROJECT_TEMPLATES.Since this is intentionally stubbed for now, a brief comment would signal to future contributors where to populate it and what format is expected.
✏️ Suggested annotation
-export const PROJECT_TEMPLATES: ProjectTemplate[] = []; +// TODO: Populate with built-in templates once the template library is defined. +export const PROJECT_TEMPLATES: ProjectTemplate[] = [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/constants.ts` at line 10, The exported constant PROJECT_TEMPLATES is intentionally left empty but lacks guidance; add a concise TODO comment immediately above the PROJECT_TEMPLATES declaration explaining it’s a stub, describe expected shape/types (ProjectTemplate[]), indicate where/how to populate it (e.g., list of template objects with fields X/Y/Z or reference the ProjectTemplate interface), and optionally link to any docs or an issue number for follow-up so future contributors know the format and intent.apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx (1)
32-35:useEffectresets a user-cleared path back to the default.Because
parentDiris in the dependency array, the effect re-runs whenever the user empties the path input. SinceparentDir === ""is falsy, the guardif (parentDir || !homeDir) return;does not fire and the path is silently restored to${homeDir}/.superset/projects. A user who intentionally clears the field to type an entirely different path starting from scratch can't actually produce an empty intermediate state without the field immediately snapping back.A ref-based initialization flag avoids this:
♻️ Proposed refactor
- const { data: homeDir } = electronTrpc.window.getHomeDir.useQuery(); + const { data: homeDir } = electronTrpc.window.getHomeDir.useQuery(); + const dirInitialized = useRef(false); useEffect(() => { - if (parentDir || !homeDir) return; - setParentDir(`${homeDir}/.superset/projects`); - }, [homeDir, parentDir]); + if (dirInitialized.current || !homeDir) return; + dirInitialized.current = true; + setParentDir(`${homeDir}/.superset/projects`); + }, [homeDir]);Also add
useRefto the React import:-import { useEffect, useState } from "react"; +import { useEffect, useRef, useState } from "react";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx` around lines 32 - 35, The effect that sets parentDir resets a user-cleared value because it depends on parentDir; change it to run only once after homeDir becomes available by adding a ref initialization flag: import useRef, create e.g. initializedRef = useRef(false), in the effect check if (!homeDir || initializedRef.current) return; then setParentDir(`${homeDir}/.superset/projects`) and set initializedRef.current = true; also remove parentDir from the dependency array so the effect depends only on homeDir.apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/components/TemplateRepoTab/TemplateRepoTab.tsx (1)
57-112:autoFocuson the custom URL input will conflict with the template grid once templates are populated.Currently harmless since
PROJECT_TEMPLATESis empty and the custom URL input is always the first interactive element. When templates are added,autoFocuswill scroll focus past the template grid to the URL input below it, which is likely not the intended UX. Worth tracking this before the template list goes live.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/components/TemplateRepoTab/TemplateRepoTab.tsx` around lines 57 - 112, The custom URL Input in TemplateRepoTab uses autoFocus which will steal focus from the template buttons when PROJECT_TEMPLATES is non-empty; remove the unconditional autoFocus or make it conditional so the input only auto-focuses when there are no templates (e.g., autoFocus={PROJECT_TEMPLATES.length === 0}). Update the Input usage (id "custom-template-url") and related logic that sets customUrl/setSelectedTemplate accordingly to avoid forcing focus away from the template grid.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 849-873: The createEmptyRepo flow currently mkdirs repoPath, then
calls initGitRepo and on error returns without removing the created directory;
update the catch block in the createEmptyRepo handler to delete the orphaned
repoPath (use fs.rm or fs.rmdir with recursive/force options) when initGitRepo
or subsequent steps fail before upsertProject completes, ensuring you only
attempt removal when repoPath exists and before returning the error; reference
the mkdir(repoPath, { recursive: true }), initGitRepo(repoPath),
upsertProject(repoPath, defaultBranch) and the catch block to locate where to
add the cleanup.
- Around line 893-898: The createFromTemplate flow allows a user-supplied name
to bypass SAFE_REPO_NAME_REGEX validation: ensure the input `name` (schema field
in createFromTemplate) is validated/normalized the same way as names derived by
`extractRepoName` by applying `SAFE_REPO_NAME_REGEX` (or reusing the same
validation function) after the trim/transform step and rejecting or sanitizing
invalid values; update the validation logic where `name` is defined/used (the
z.string().trim().optional().transform(...) and the createFromTemplate handler)
to either run the same regex check or call the same helper that
`extractRepoName` uses so both paths enforce identical, safe repo-name
constraints.
- Around line 923-958: In createFromTemplate, if rm(join(repoPath, ".git")) or
initGitRepo(repoPath, ...) throws after simpleGit().clone, the function returns
an error but leaves the cloned repo directory in place (blocking future runs via
the existsSync check); wrap the post-clone steps (rm(...), initGitRepo,
upsertProject, ensureMainWorkspace, track, return success) in their own
try/catch and on any exception perform cleanup by recursively deleting repoPath
(the cloned directory) before re-throwing or returning the failure payload;
ensure you reference the repoPath variable and the calls to rm, initGitRepo,
upsertProject and simpleGit().clone so cleanup only runs for partially-completed
clones and do not call upsertProject unless all post-clone steps succeed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx`:
- Around line 44-50: The icon-only back button lacks an accessible name; in the
JSX for the button element that renders LuArrowLeft (the button with onClick={()
=> navigate({ to: "/", replace: true })}), add an appropriate accessible label
(e.g., aria-label="Go back" or aria-label="Back to projects") so screen readers
can announce its purpose; optionally also add a title attribute for hover
tooltips but ensure aria-label is present for accessibility.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/components/TemplateRepoTab/TemplateRepoTab.tsx`:
- Around line 57-112: The custom URL Input in TemplateRepoTab uses autoFocus
which will steal focus from the template buttons when PROJECT_TEMPLATES is
non-empty; remove the unconditional autoFocus or make it conditional so the
input only auto-focuses when there are no templates (e.g.,
autoFocus={PROJECT_TEMPLATES.length === 0}). Update the Input usage (id
"custom-template-url") and related logic that sets customUrl/setSelectedTemplate
accordingly to avoid forcing focus away from the template grid.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/constants.ts`:
- Line 10: The exported constant PROJECT_TEMPLATES is intentionally left empty
but lacks guidance; add a concise TODO comment immediately above the
PROJECT_TEMPLATES declaration explaining it’s a stub, describe expected
shape/types (ProjectTemplate[]), indicate where/how to populate it (e.g., list
of template objects with fields X/Y/Z or reference the ProjectTemplate
interface), and optionally link to any docs or an issue number for follow-up so
future contributors know the format and intent.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx`:
- Around line 32-35: The effect that sets parentDir resets a user-cleared value
because it depends on parentDir; change it to run only once after homeDir
becomes available by adding a ref initialization flag: import useRef, create
e.g. initializedRef = useRef(false), in the effect check if (!homeDir ||
initializedRef.current) return; then
setParentDir(`${homeDir}/.superset/projects`) and set initializedRef.current =
true; also remove parentDir from the dependency array so the effect depends only
on homeDir.
Replace the CloneRepoDialog with a dedicated /new-project route that supports creating empty repos and cloning. Adds a shared path selector defaulting to ~/.superset/projects.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 819-823: The current validation allows names like "." and ".."
because SAFE_REPO_NAME_REGEX (/^[a-zA-Z0-9._\- ]+$/) permits lone dots; update
the refine on the schema (the .refine(...) chained after .min(1)) to
additionally reject names that consist only of dots (e.g., /^\.+$/ or test like
val.trim().replace(/[^.]/g,"") !== val), so inputs like "." and ".." fail
validation, and update the nearby regex comment to accurately state that dots
are allowed but dot-only names are disallowed to prevent path traversal when
functions like path.join(parentDir, name) are used.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx`:
- Around line 54-72: The tab strip rendered from TABS lacks ARIA semantics: add
role="tablist" to the container div and update each mapped button (the elements
using tab.mode, setMode, setError, and comparing against mode) to include
role="tab", aria-selected={mode === tab.mode}, and a tabindex (0 when selected,
-1 otherwise) and aria-controls pointing to the corresponding panel id; also
ensure each content wrapper rendered for a tab has role="tabpanel" and an id
that matches the button's aria-controls so assistive tech can associate tabs
with their panels and selection state is updated when setMode is called.
---
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 839-841: The code creates the directory with mkdir(repoPath, {
recursive: true }) then calls initGitRepo(repoPath) and if initGitRepo throws
the created repoPath is left on disk causing existsSync-based conflicts; update
the block around initGitRepo to catch errors from initGitRepo, perform cleanup
by removing repoPath (e.g., fs.rm or rmdir recursive) before returning/throwing,
and ensure any cleanup failure is logged but does not mask the original error;
reference the mkdir call, initGitRepo(repoPath), repoPath variable, and the
existsSync guard when adding the cleanup logic.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx`:
- Around line 42-48: The back button using the LuArrowLeft icon is missing an
accessible label; update the button element (the one with onClick={() =>
navigate({ to: "/", replace: true })}) to include an aria-label (e.g.,
aria-label="Back" or "Go back") so screen readers get a meaningful name, and
optionally set aria-hidden="true" on the LuArrowLeft icon if you keep the icon
purely decorative; ensure the change is applied to the button wrapper that
handles navigation.
Comment on lines
+54
to
+72
| <div className="flex p-0.5 bg-muted rounded-md"> | ||
| {TABS.map((tab) => ( | ||
| <button | ||
| key={tab.mode} | ||
| type="button" | ||
| onClick={() => { | ||
| setMode(tab.mode); | ||
| setError(null); | ||
| }} | ||
| className={`flex-1 px-3 py-1 text-xs font-medium rounded-sm transition-colors ${ | ||
| mode === tab.mode | ||
| ? "bg-background text-foreground shadow-sm" | ||
| : "text-muted-foreground hover:text-foreground" | ||
| }`} | ||
| > | ||
| {tab.label} | ||
| </button> | ||
| ))} | ||
| </div> |
Contributor
There was a problem hiding this comment.
Tab buttons are missing ARIA role semantics.
The container <div> and the mapped <button> elements are styled and behave as a tab strip, but have no role="tablist" / role="tab" / aria-selected attributes. Without them, assistive technologies announce these as generic buttons rather than a navigable tab group.
♿ Proposed fix
-<div className="flex p-0.5 bg-muted rounded-md">
+<div role="tablist" className="flex p-0.5 bg-muted rounded-md">
{TABS.map((tab) => (
<button
key={tab.mode}
type="button"
+ role="tab"
+ aria-selected={mode === tab.mode}
+ aria-controls={`tabpanel-${tab.mode}`}
onClick={() => {
setMode(tab.mode);
setError(null);
}}And add role="tabpanel" + id to each content wrapper:
-{mode === "empty" && (
- <EmptyRepoTab onError={setError} parentDir={parentDir} />
-)}
-{mode === "clone" && (
- <CloneRepoTab onError={setError} parentDir={parentDir} />
-)}
+<div role="tabpanel" id="tabpanel-empty" hidden={mode !== "empty"}>
+ <EmptyRepoTab onError={setError} parentDir={parentDir} />
+</div>
+<div role="tabpanel" id="tabpanel-clone" hidden={mode !== "clone"}>
+ <CloneRepoTab onError={setError} parentDir={parentDir} />
+</div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx`
around lines 54 - 72, The tab strip rendered from TABS lacks ARIA semantics: add
role="tablist" to the container div and update each mapped button (the elements
using tab.mode, setMode, setError, and comparing against mode) to include
role="tab", aria-selected={mode === tab.mode}, and a tabindex (0 when selected,
-1 otherwise) and aria-controls pointing to the corresponding panel id; also
ensure each content wrapper rendered for a tab has role="tabpanel" and an id
that matches the button's aria-controls so assistive tech can associate tabs
with their panels and selection state is updated when setMode is called.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx (1)
17-20: "Template" tab from the PR test plan is absent.The PR checklist includes "Template tab renders with URL input (no preset templates yet)", but
TABSonly defines"empty"and"clone", andNewProjectModeinconstants.tshas no"template"variant. If the tab is intentionally deferred, a TODO comment here and inconstants.tswould make that explicit and prevent it from being forgotten.Do you want me to scaffold the
TemplateRepoTabcomponent, extendNewProjectMode, and add the tab entry so this test-plan item is covered?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx` around lines 17 - 20, TABS currently lacks the "template" entry and NewProjectMode in constants.ts has no "template" variant, so either add the missing mode and tab or mark it as intentionally deferred: add a "template" variant to NewProjectMode in constants.ts and add { mode: "template", label: "Template" } to the TABS array in page.tsx and wire up the TemplateRepoTab component, or if you don't want to implement it now, add a clear TODO comment in both constants.ts (next to NewProjectMode) and apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx (next to TABS) stating that the Template tab is intentionally deferred and reference the PR/test-plan item ID so it won't be forgotten.apps/desktop/src/lib/trpc/routers/projects/projects.ts (1)
304-308:selectDirectory: "no window" state is indistinguishable from user-cancel on the client.When
getWindow()returnsnull, the procedure returns{ canceled: true, path: null }— identical to a legitimate user dismissal. The caller cannot distinguish a system fault (degenerate app state) from intentional user action, which can silently swallow the error.♻️ Proposed fix
const window = getWindow(); if (!window) { - return { canceled: true as const, path: null }; + return { canceled: false as const, path: null, error: "No window available" }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts` around lines 304 - 308, The current selectDirectory mutation treats getWindow() === null the same as a user cancel by returning { canceled: true, path: null }, making system/state errors indistinguishable from user action; instead detect the missing window in the mutation and surface a distinct failure: either throw a trpc TRPCError (e.g., new TRPCError({ code: 'INTERNAL', message: 'no window available' })) or return a shaped error object (e.g., { canceled: false, path: null, error: 'no window' }) so callers can differentiate; update the code path that checks getWindow() (in the mutation where getWindow() is called) to implement one of these approaches and adjust any callers/tests accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 813-814: createEmptyRepo currently leaves the initialized git
folder (repoPath) and possibly a partial DB record if upsertProject or
ensureMainWorkspace throws; wrap the calls to upsertProject and
ensureMainWorkspace in their own try/catch (or a try/finally) so that on any
error you remove the repoPath directory (fs.rmSync or fs.promises.rm with
recursive) and, if a project object was returned, delete the created project
record (e.g. call whatever deleteProject/deleteProjectById or
prisma.project.delete using project.id) before rethrowing/returning the error;
reference the createEmptyRepo function, the repoPath variable, the upsertProject
call (which returns project), and the ensureMainWorkspace call when making the
changes.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx`:
- Around line 30-33: The effect that defaults parentDir runs whenever parentDir
changes, so clearing the input triggers it and immediately resets the value;
update the useEffect around setParentDir so it only responds to homeDir initial
resolution (remove parentDir from the dependency array) and/or guard against
overwriting user edits by using an initial-mount ref or initialize parentDir via
the query onSuccess instead of reacting to parentDir changes; target the
useEffect that references parentDir, homeDir and calls setParentDir to make this
change.
---
Duplicate comments:
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 248-252: extractRepoName currently allows names composed solely of
dots (e.g., "..") which can enable path traversal in cloneRepo; update
extractRepoName to reject dot-only values the same way createEmptyRepo's schema
does by adding a guard like !/^\.+$/.test(repoName) alongside the existing
SAFE_REPO_NAME_REGEX check so that extractRepoName returns null for inputs like
"git@github.com:.." and prevents join(targetDir, repoName) from resolving
outside the intended directory (references: extractRepoName,
SAFE_REPO_NAME_REGEX, cloneRepo, createEmptyRepo).
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx`:
- Around line 55-73: The tab strip lacks ARIA semantics: add role="tablist" to
the container div and for each mapped tab button (inside the TABS.map) add
role="tab", aria-selected={mode===tab.mode}, id and aria-controls attributes
(use a stable id pattern based on tab.mode) and ensure the two conditional
renders that show tab content include role="tabpanel" with matching id and
aria-labelledby pointing back to the tab id; update any references to mode,
setMode, and setError in the mapped button to preserve behavior while adding
these ARIA attributes so assistive tech recognizes the tabs and panels.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceSidebar/WorkspaceSidebarFooter.tsx`:
- Line 64: Remove the isLoading guard from the DropdownMenuTrigger Buttons so
the dropdown can open while createBranchWorkspace.isPending or isOpenPending is
true; keep the disabled={isLoading} on the "Open project" DropdownMenuItem only.
Concretely, in WorkspaceSidebarFooter's JSX remove disabled={isLoading} from the
DropdownMenuTrigger/ Button elements (both collapsed and expanded triggers) and
ensure the "Open project" DropdownMenuItem(s) still use disabled={isLoading} so
only that action remains blocked.
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts`:
- Around line 304-308: The current selectDirectory mutation treats getWindow()
=== null the same as a user cancel by returning { canceled: true, path: null },
making system/state errors indistinguishable from user action; instead detect
the missing window in the mutation and surface a distinct failure: either throw
a trpc TRPCError (e.g., new TRPCError({ code: 'INTERNAL', message: 'no window
available' })) or return a shaped error object (e.g., { canceled: false, path:
null, error: 'no window' }) so callers can differentiate; update the code path
that checks getWindow() (in the mutation where getWindow() is called) to
implement one of these approaches and adjust any callers/tests accordingly.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx`:
- Around line 17-20: TABS currently lacks the "template" entry and
NewProjectMode in constants.ts has no "template" variant, so either add the
missing mode and tab or mark it as intentionally deferred: add a "template"
variant to NewProjectMode in constants.ts and add { mode: "template", label:
"Template" } to the TABS array in page.tsx and wire up the TemplateRepoTab
component, or if you don't want to implement it now, add a clear TODO comment in
both constants.ts (next to NewProjectMode) and
apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx
(next to TABS) stating that the Template tab is intentionally deferred and
reference the PR/test-plan item ID so it won't be forgotten.
Comment on lines
+813
to
+814
| const project = upsertProject(repoPath, defaultBranch); | ||
| await ensureMainWorkspace(project); |
Contributor
There was a problem hiding this comment.
createEmptyRepo: no cleanup if upsertProject or ensureMainWorkspace throws after a successful initGitRepo.
The previous review addressed cleanup for initGitRepo failure (inner try-catch + rm at lines 807–812). However, if upsertProject (line 813) or ensureMainWorkspace (line 814) throw a DB error, the outer catch at line 826 returns an error response but leaves the git-initialized directory on disk (and potentially a partial project record). On retry with the same name, existsSync(repoPath) returns true → the user sees "A folder already exists" and is blocked until they manually delete the directory.
🐛 Proposed fix — extend cleanup scope
+ let project: Project;
try {
({ defaultBranch } = await initGitRepo(repoPath));
} catch (gitErr) {
await rm(repoPath, { recursive: true, force: true });
throw gitErr;
}
- const project = upsertProject(repoPath, defaultBranch);
- await ensureMainWorkspace(project);
+ try {
+ project = upsertProject(repoPath, defaultBranch);
+ await ensureMainWorkspace(project);
+ } catch (dbErr) {
+ await rm(repoPath, { recursive: true, force: true });
+ throw dbErr;
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/lib/trpc/routers/projects/projects.ts` around lines 813 -
814, createEmptyRepo currently leaves the initialized git folder (repoPath) and
possibly a partial DB record if upsertProject or ensureMainWorkspace throws;
wrap the calls to upsertProject and ensureMainWorkspace in their own try/catch
(or a try/finally) so that on any error you remove the repoPath directory
(fs.rmSync or fs.promises.rm with recursive) and, if a project object was
returned, delete the created project record (e.g. call whatever
deleteProject/deleteProjectById or prisma.project.delete using project.id)
before rethrowing/returning the error; reference the createEmptyRepo function,
the repoPath variable, the upsertProject call (which returns project), and the
ensureMainWorkspace call when making the changes.
Comment on lines
+30
to
+33
| useEffect(() => { | ||
| if (parentDir || !homeDir) return; | ||
| setParentDir(`${homeDir}/.superset/projects`); | ||
| }, [homeDir, parentDir]); |
Contributor
There was a problem hiding this comment.
Clearing the path input will silently reset it to the default.
The useEffect dependency list includes parentDir, so the effect re-runs whenever the user edits the field. The guard if (parentDir || !homeDir) return prevents re-setting while the field is non-empty, but as soon as the user clears it (parentDir → ""), the condition becomes false and the path is immediately reset to ${homeDir}/.superset/projects — with no indication to the user.
🛠️ Proposed fix — initialize once, don't react to user edits
-useEffect(() => {
- if (parentDir || !homeDir) return;
- setParentDir(`${homeDir}/.superset/projects`);
-}, [homeDir, parentDir]);
+useEffect(() => {
+ if (!homeDir) return;
+ setParentDir(`${homeDir}/.superset/projects`);
+}, [homeDir]);Removing parentDir from the deps means the effect only fires when homeDir first resolves, which is the intended behaviour. If you also want to avoid overwriting a value the user set before the query resolves, guard with the initial-mount pattern (a ref flag) or initialize the state lazily via the query's onSuccess.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_onboarding/new-project/page.tsx`
around lines 30 - 33, The effect that defaults parentDir runs whenever parentDir
changes, so clearing the input triggers it and immediately resets the value;
update the useEffect around setParentDir so it only responds to homeDir initial
resolution (remove parentDir from the dependency array) and/or guard against
overwriting user edits by using an initial-mount ref or initialize parentDir via
the query onSuccess instead of reacting to parentDir changes; target the
useEffect that references parentDir, homeDir and calls setParentDir to make this
change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CloneRepoDialogwith a dedicated/new-projectroute featuring three tabs: Empty, Clone, and Template~/.superset/projectsselectDirectory,createEmptyRepo,createFromTemplate/new-projectinstead of opening the old clone dialogconstants.ts) for nowTest plan
/new-projectfrom welcome page "New Project" button/new-projectfrom sidebar footer dropdown~/.superset/projects, browse button opens native pickerSummary by CodeRabbit
New Features
Refactor