feat: enhance project creation wizard - folder browser fixes and git clone improvements #320
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds frontend folder-creation UI/state and SSE-driven Git clone progress; new POST /api/create-folder endpoint; SSE GET /clone-progress with token-in-query fallback for authentication; server-side cloning into repo-named subfolders with improved validation, error mapping, and cleanup. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Git as "git clone (proc)"
Client->>Server: POST /create-workspace (start clone) or GET /clone-progress?repo=...&path=...&token=... (EventSource)
Server->>Server: validate token (Authorization header or req.query.token)
Server->>Git: spawn `git clone` to destination subdir (token-embedded URL if provided)
Git-->>Server: stdout/stderr (progress lines)
Server-->>Client: SSE event: progress (data)
alt clone succeeds
Git-->>Server: exit 0
Server->>Server: add cloned subpath as project
Server-->>Client: SSE event: complete (project info)
else clone fails
Git-->>Server: exit != 0
Server->>Server: cleanup partial clone
Server-->>Client: SSE event: error (message)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
blackmammoth
left a comment
blackmammoth
left a comment
There was a problem hiding this comment.
Here are some issues @EricBlanquer:
- If there are no subfolders, there is not a way to navigate back
- Clicking the ".." icon should bring us to the parent directory. However, it always brings us to "C:" on Windows
|
Fixed in c1586e3:
|
|
@EricBlanquer Awesome! This fixes the issues. However, if the user clicked a "New Workspace", then they should have an option to create a path by entering their desired folder name. Right now, it always shows the following message: 
|
EricBlanquer
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@server/index.js`:
- Around line 530-556: The create-folder POST handler (route
'/api/create-folder') lacks workspace/path validation; update it to reuse or
import an exported validateWorkspacePath (from server/routes/projects.js) and
run the provided folderPath through that validator before accessing or creating
directories, returning a 400/403 when validation fails; additionally ensure
forbidden system roots (e.g., /etc, /usr) are checked via the same validator or
a small allowlist check and only allow creation when validateWorkspacePath
approves to prevent creating folders outside the allowed workspace root.
In `@server/routes/projects.js`:
- Around line 236-238: The repo name extraction can return an empty string when
githubUrl ends with a slash; update the logic around githubUrl -> repoName (used
when creating clonePath) to trim trailing slashes first and then strip a
trailing ".git" (or alternatively split on '/' and pick the last non-empty
segment) so repoName is never empty; adjust the code that computes repoName and
clonePath (referencing variables githubUrl, repoName, clonePath in
server/routes/projects.js) to use the trimmed/validated segment and fall back to
a clear error or default name if no segment is found.
- Around line 352-353: The repo name extraction using githubUrl, repoName, and
clonePath can fail when githubUrl ends with a trailing slash; normalize the URL
first by trimming any trailing slashes (e.g., remove trailing '/' characters)
before doing .replace(/\.git$/, '') and .split('/').pop() so repoName is correct
for inputs like "https://.../repo/" and "https://.../repo.git/"; update the
expression that computes repoName to operate on the trimmed URL and leave
clonePath logic using repoName unchanged.
In `@src/components/ProjectCreationWizard.jsx`:
- Around line 236-255: The createNewFolder function builds paths by
concatenating browserCurrentPath and newFolderName with a forward slash
(producing invalid Windows paths) and uses alert() for errors; change the API
contract and UI handling: have createNewFolder call api.createFolder({ parent:
browserCurrentPath, name: newFolderName.trim() }) (update api.createFolder
implementation/server endpoint accordingly) instead of string-joining paths,
call loadBrowserFolders(browserCurrentPath) to refresh the current folder, and
replace alert(...) with the component's error state updater (use
setError(error.message || 'Failed to create folder')) so errors render
consistently via the existing error UI.
🧹 Nitpick comments (3)
server/routes/projects.js (2)
394-415: Directory cleanup missing on clone failure.When the clone fails, the created directories (
absolutePathand potentially partialclonePath) are not cleaned up, unlike thecreate-workspaceendpoint which removes the directory on failure. This could leave orphaned directories.♻️ Suggested cleanup on failure
} else { let errorMessage = 'Git clone failed'; if (lastError.includes('Authentication failed') || lastError.includes('could not read Username')) { errorMessage = 'Authentication failed. Please check your credentials.'; } else if (lastError.includes('Repository not found')) { errorMessage = 'Repository not found. Please check the URL and ensure you have access.'; } else if (lastError.includes('already exists')) { errorMessage = 'Directory already exists'; } else if (lastError) { errorMessage = lastError; } + // Cleanup partial clone directory on failure + try { + await fs.rm(clonePath, { recursive: true, force: true }); + } catch (cleanupError) { + console.error('Failed to clean up after clone failure:', cleanupError); + } sendEvent('error', { message: errorMessage }); }
267-273: Dead code: unreachable message branch.The ternary at line 270-272 checks
githubUrl, but this code path is only reached whengithubUrlis falsy (line 262 returns early when cloning). The "cloned successfully" message branch can never execute.♻️ Simplify to remove dead branch
return res.json({ success: true, project, - message: githubUrl - ? 'New workspace created and repository cloned successfully' - : 'New workspace created successfully' + message: 'New workspace created successfully' });src/components/ProjectCreationWizard.jsx (1)
450-451: Good SSH URL detection, could be extended.The check for
git@prefix correctly identifies SSH URLs. Note that SSH URLs can also be in the formatssh://git@github.com/..., which wouldn't be caught bystartsWith('git@').- {githubUrl && !githubUrl.startsWith('git@') && ( + {githubUrl && !githubUrl.startsWith('git@') && !githubUrl.startsWith('ssh://') && (
EricBlanquer
force-pushed
the
fix/new-project-folder-selection
branch
from
ff94c3c to
6dd270a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/index.js`:
- Around line 530-570: The TOCTOU between fs.promises.access and
fs.promises.mkdir in the Express handler for app.post('/api/create-folder') can
cause mkdir to throw EEXIST and currently returns 500; modify the code around
the mkdir call (referencing targetPath and the mkdir invocation) to catch errors
from fs.promises.mkdir, inspect err.code === 'EEXIST' and return
res.status(409).json({ error: 'Folder already exists' }) for that case, and only
treat other errors as 500.
In `@server/routes/projects.js`:
- Around line 236-262: The clone flow can delete an existing directory if git
fails; before calling cloneGitHubRepository(check normalizedUrl, repoName,
clonePath), check whether clonePath already exists (fs.stat/fs.existsSync) and
if it does, bail out with a clear error (or pick a unique temp subfolder)
instead of attempting to overwrite it; when you proceed to create/clone into a
path, set a local flag (e.g., createdByThisOperation = true) or create a
uniquely named temp folder and only perform fs.rm(cleanup) in the catch block if
that flag is true (i.e., the directory was created by this operation), leaving
pre-existing user directories untouched; apply the same guard to the other clone
usage referenced and ensure addProjectManually is only called after a successful
clone.
I've tested the "New Workspace" option and add new feature for it |
@EricBlanquer Clicking "Create" gives an error. The response from the API is: <!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8">
<title>Error</title>
</head>
<body>
<pre>Cannot POST /api/create-folder</pre>
</body>
</html>Note: I'm using Windows |
In addition, if you are in "New Workspace", you shouldn't see "Select" and "Use this folder". The user should only navigate to their desire directory and then they should be asked for a folder name they want to create. |
fixed for Windows path |
When selecting home folder in New Project wizard, ~ was being used instead of /home/<user>, causing "Workspace path does not exist" error. Now uses the resolved absolute path returned by the browse-filesystem API.
- Show parent directory (..) button even when folder has no subfolders - Handle Windows backslash paths when calculating parent directory - Prevent navigation to Windows drive root (C:\) from breaking
…one progress - Add "+" button to create new folders directly from folder browser - Add SSE endpoint for git clone with real-time progress display - Show clone progress (receiving objects, resolving deltas) in UI - Detect SSH URLs and display "SSH Key" instead of "No authentication" - Hide token section for SSH URLs (tokens only work with HTTPS) - Fix auto-advance behavior: only auto-advance for "Existing Workspace" - Fix various misleading UI messages - Support auth token via query param for SSE endpoints
- Add path validation to /api/create-folder endpoint (forbidden system dirs) - Fix repo name extraction to handle trailing slashes in URLs - Add cleanup of partial clone directory on SSE clone failure - Remove dead code branch (unreachable message) - Fix Windows path separator detection in createNewFolder - Replace alert() with setError() for consistent error handling - Detect ssh:// URLs in addition to git@ for SSH key display - Show create folder button for both workspace types
EricBlanquer
force-pushed
the
fix/new-project-folder-selection
branch
from
8853623 to
5cfb01b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/routes/projects.js`:
- Around line 354-362: Optional chaining on req.user is unnecessary because
authenticateToken middleware guarantees req.user; update the call to
getGithubTokenById to use req.user.id instead of req.user?.id. Locate the block
handling githubToken (variables githubTokenId, newGithubToken and the call to
getGithubTokenById) and replace the optional chain with a direct property access
(req.user.id); ensure any related logic remains unchanged and that
authenticateToken and projectsRoutes references are left intact.
In `@src/components/ProjectCreationWizard.jsx`:
- Around line 818-833: The "Select"/"Use this folder" action should be hidden in
New Workspace mode; update the JSX rendering in ProjectCreationWizard so the
Button that calls selectFolder(...) is only rendered when workspaceType ===
'existing' (or inversely, not rendered when workspaceType === 'new'); locate the
folder list block that uses navigateToFolder(folder.path) and conditionally
render the Button (and the similar button instance later in the file) so
selection is only available for existing workspaces.
🧹 Nitpick comments (2)
server/index.js (1)
553-553: Consider extractingFORBIDDEN_PATHSto a shared module.This constant is duplicated from
server/routes/projects.js(lines 14-31). To maintain consistency and reduce duplication, consider extracting it to a shared utilities module.♻️ Suggested refactor
Create a shared module (e.g.,
server/utils/paths.js):// server/utils/paths.js export const FORBIDDEN_PATHS = [ '/', '/etc', '/bin', '/sbin', '/usr', '/dev', '/proc', '/sys', '/var', '/boot', '/root', '/lib', '/lib64', '/opt', '/tmp', '/run' ];Then import in both files:
-const FORBIDDEN_PATHS = ['/', '/etc', '/bin', ...]; +import { FORBIDDEN_PATHS } from './utils/paths.js';server/routes/projects.js (1)
409-415: Minor: Git stderr may expose credential hints on auth failure.Git's stderr output is forwarded directly to the client. While git typically doesn't echo embedded credentials, error messages on authentication failure might reveal partial URL information. Consider sanitizing stderr before sending to the client, or at minimum, avoid forwarding raw error output when authentication fails.
♻️ Optional sanitization
gitProcess.stderr.on('data', (data) => { const message = data.toString().trim(); lastError = message; if (message) { + // Sanitize potential credential leaks from URLs + const sanitized = message.replace(/https?:\/\/[^@]+@/g, 'https://***@'); sendEvent('progress', { message: sanitized }); } });
oh, labels have been changed... before they were: now they are: they were better before, why a new step as we can now create folders from the select? |
The regex only replaced forward slashes, causing Windows paths like C:\Users\Eric\my_project to remain unchanged instead of being encoded to C--Users-Eric-my-project. This caused API routes to fail.
EricBlanquer
force-pushed
the
fix/new-project-folder-selection
branch
from
5cfb01b to
5fa8bed
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@server/routes/projects.js`:
- Around line 354-362: When githubTokenId is provided the code calls
getGithubTokenById but silently proceeds if it returns null, which will cause
auth failures; update the handler that uses githubTokenId to check the result of
getGithubTokenById (the token variable) and if it's null return a 404 response
(e.g., res.status(404).json({ error: "GitHub token not found" })) instead of
falling through with githubToken = null; ensure you still allow the
newGithubToken branch to work and reference the githubTokenId,
getGithubTokenById, githubToken, and newGithubToken variables to locate the fix.
- Around line 378-388: The code embeds githubToken into cloneUrl (cloneUrl,
githubUrl, githubToken) which can leak the token if that URL is later included
in lastError or server logs; add a redaction step (e.g., a helper like
redactTokenFromUrl or sanitizeUrl) that replaces the token/password in any URL
string with a placeholder before assigning to lastError or passing to
SSE/logging, and call this sanitizer wherever lastError is set or the error is
logged (references: lastError, the SSE send path, and any process/server logging
calls) so no embedded token is ever emitted.
🧹 Nitpick comments (3)
server/routes/projects.js (3)
215-216: Empty workspace directory left behind on clone failure.When a GitHub URL is provided and clone fails, only
clonePathis cleaned up (lines 256-264), but the parentabsolutePathdirectory created here remains. This could leave empty workspace directories.Consider cleaning up
absolutePathif it was newly created and remains empty after clone failure, or defer directory creation until after clone validation succeeds.
324-325: Security consideration: GitHub token in query parameters.The
newGithubTokenin query params may be logged by reverse proxies, load balancers, or server access logs. While necessary for EventSource (which doesn't support custom headers), consider:
- Ensuring server access logs redact query parameters containing tokens
- Using short-lived tokens when possible
- Preferring
githubTokenId(stored token reference) overnewGithubToken(actual token value)
455-457: Client disconnect may leave partial clone data.When the client disconnects,
gitProcess.kill()is called, but the process may have already created partial data. Thecloseevent handler (line 417) should fire with a non-zero exit code and trigger cleanup, but this depends on the kill signal being processed correctly.Consider ensuring cleanup runs even on SIGTERM:
🔧 Suggested enhancement
req.on('close', () => { - gitProcess.kill(); + if (!gitProcess.killed) { + gitProcess.kill('SIGTERM'); + } });The current implementation should work in most cases since the
closeevent will still fire afterkill().
EricBlanquer
force-pushed
the
fix/new-project-folder-selection
branch
3 times, most recently
from
c77b3c2 to
140f6ad
Compare
EricBlanquer
force-pushed
the
fix/new-project-folder-selection
branch
from
140f6ad to
00e5aee
Compare
2 participants
Summary
Bug Fixes
New Features
Technical Changes
/api/projects/clone-progressfor streaming clone progress/api/create-folderfor folder creationSummary by CodeRabbit
New Features
Bug Fixes
Chores
Localization
✏️ Tip: You can customize this high-level summary in your review settings.