fix: empty folder selection reverts in DirectoryPicker + add create-folder button#1466
Merged
Conversation
…older button Fix bug where navigating into an empty folder in the DirectoryPicker would automatically revert to the previous folder. The useEffect that auto-fetches on open was using the stale 'value' prop instead of 'browser.currentPath' after navigation. Also adds a 'New folder' button to the DirectoryPicker toolbar so users can create folders directly when setting up a project. - DirectoryPicker.tsx: fix useEffect to use browser.currentPath || value - DirectoryPicker.tsx: add create folder UI state and handlers - DirectoryPicker.css: styles for create folder input and actions - routes.ts: add POST /api/create-directory endpoint - legacy.ts: add createDirectory API client - DirectoryPicker.test.tsx: add tests for empty folder nav and create folder
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 backend POST /api/create-directory, a client helper createDirectory, DirectoryPicker create-folder UI/state/handlers, tests for navigation and folder creation, documentation of the empty-folder navigation fix, localization updates, and a release changeset. ChangesCreate Directory Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/app/components/DirectoryPicker.tsx (1)
105-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncomplete useEffect dependency array.
The effect calls
fetchEntriesbut doesn't include it in the dependency array. IffetchEntrieschanges (e.g., whennodeIdorlocalNodeIdprops change), the effect will continue using the stale function reference.🔧 Recommended fix
useEffect(() => { if (browser.isOpen && browser.currentPath) { fetchEntries(browser.currentPath, browser.showHidden); } -}, [browser.showHidden]); +}, [browser.showHidden, fetchEntries]);Note: ESLint may warn about missing
browser.isOpenandbrowser.currentPath. These are intentionally omitted to avoid unnecessary re-runs (we only want to refetch whenshowHiddenchanges, not when the user navigates). If ESLint complains, add an inline disable with justification.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/DirectoryPicker.tsx` around lines 105 - 109, The effect in DirectoryPicker.tsx uses fetchEntries but omits it from the dependency array causing a stale closure; update the useEffect dependency array to include fetchEntries so it re-runs when that function changes (e.g., due to nodeId/localNodeId changes), and keep browser.showHidden there; if ESLint warns about including browser.isOpen or browser.currentPath, add an inline eslint-disable-next-line comment with a brief justification that those fields are intentionally omitted to avoid re-fetching on navigation.
🧹 Nitpick comments (1)
packages/dashboard/app/components/DirectoryPicker.tsx (1)
120-144: ⚡ Quick winConsider frontend validation for folder name.
While the backend validates the path and prevents
..traversal, the frontend could validate thatnewFolderNamedoesn't contain path separators (/or\) or path traversal components. This would provide immediate feedback to users rather than a server error.✨ Suggested validation
const handleCreateFolder = useCallback(async () => { if (!newFolderName.trim() || !browser.currentPath) return; + + // Validate folder name doesn't contain path separators or traversal + if (newFolderName.includes("/") || newFolderName.includes("\\") || newFolderName.includes("..")) { + setBrowser((prev) => ({ + ...prev, + createFolderError: "Folder name cannot contain path separators or '..'", + })); + return; + } // Normalize path separator for the current platform by using the same🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/DirectoryPicker.tsx` around lines 120 - 144, Add client-side validation in handleCreateFolder to reject folder names containing path separators or traversal components before calling createDirectory: check newFolderName.trim() for '/' or '\' or sequences like '..' (and empty string), and if invalid setBrowser to set createFolderError with a clear message and stop (do not call createDirectory or fetchEntries). Ensure you still clear newFolderName and close createFolderOpen only on success; keep existing error handling for createDirectory failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dashboard/src/routes.ts`:
- Around line 4483-4516: The mkdir call can fail due to races and should map
known errno values to 4xx API errors instead of letting them bubble as 500s:
wrap the await mkdir(resolvedPath) in a try/catch, inspect the caught error
(e.g. as { code?: string }), and convert EEXIST to a badRequest("Directory
already exists") (or similar client-facing 4xx), ENOENT and ENOTDIR to
badRequest("Parent directory does not exist") / badRequest("Parent path is not a
directory") as appropriate, and rethrow any other errors; reference the existing
symbols resolvedPath, mkdir, stat, parentPath, parentStat and preserve current
behavior for non-errno errors.
- Around line 4469-4481: The handler currently calls resolve(rawPath) before
validating type and absoluteness, which lets non-strings throw and relative
inputs be turned into cwd-relative paths; instead validate rawPath is a string,
then use path.isAbsolute(rawPath) (or rawPath.startsWith('/')) to require an
absolute path, check for ".." traversal on the original rawPath, and only then
call resolve(rawPath) to normalize; update the checks around rawPath,
resolvedPath, resolve(), badRequest() and remove the ineffective resolvedPath
!== resolve(resolvedPath) branch so invalid or non-string inputs return
badRequest rather than causing a 500 or creating paths under cwd.
---
Outside diff comments:
In `@packages/dashboard/app/components/DirectoryPicker.tsx`:
- Around line 105-109: The effect in DirectoryPicker.tsx uses fetchEntries but
omits it from the dependency array causing a stale closure; update the useEffect
dependency array to include fetchEntries so it re-runs when that function
changes (e.g., due to nodeId/localNodeId changes), and keep browser.showHidden
there; if ESLint warns about including browser.isOpen or browser.currentPath,
add an inline eslint-disable-next-line comment with a brief justification that
those fields are intentionally omitted to avoid re-fetching on navigation.
---
Nitpick comments:
In `@packages/dashboard/app/components/DirectoryPicker.tsx`:
- Around line 120-144: Add client-side validation in handleCreateFolder to
reject folder names containing path separators or traversal components before
calling createDirectory: check newFolderName.trim() for '/' or '\' or sequences
like '..' (and empty string), and if invalid setBrowser to set createFolderError
with a clear message and stop (do not call createDirectory or fetchEntries).
Ensure you still clear newFolderName and close createFolderOpen only on success;
keep existing error handling for createDirectory failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ca5f6813-6be7-485f-903b-4f9906dbd61c
📒 Files selected for processing (5)
packages/dashboard/app/api/legacy.tspackages/dashboard/app/components/DirectoryPicker.csspackages/dashboard/app/components/DirectoryPicker.tsxpackages/dashboard/app/components/__tests__/DirectoryPicker.test.tsxpackages/dashboard/src/routes.ts
Contributor
Greptile SummaryThis PR fixes a
Confidence Score: 5/5Safe to merge — the useEffect guard change is a narrow, well-tested one-liner and the new endpoint has solid server-side validation. The core fix (replacing entries.length === 0 with !browser.currentPath) is correct and the create-folder feature is well-guarded on both client and server. All blocking issues raised in earlier review rounds have been resolved. The only open item is a test-coverage gap for the remote-node surface in the regression test, which does not affect correctness. The regression test in DirectoryPicker.test.tsx only covers the local (no nodeId) surface for the empty-folder revert fix; a complementary test with nodeId set would close the FN-5893 gap. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant DirectoryPicker
participant API as /api/browse-directory
participant CreateAPI as /api/create-directory
User->>DirectoryPicker: Click Browse
DirectoryPicker->>API: browseDirectory(value)
API-->>DirectoryPicker: "{currentPath, entries}"
Note over DirectoryPicker: browser.currentPath set
User->>DirectoryPicker: Click empty-folder entry
DirectoryPicker->>API: browseDirectory("/empty-folder")
API-->>DirectoryPicker: "{currentPath: "/empty-folder", entries: []}"
Note over DirectoryPicker: useEffect: !browser.currentPath = false → no re-fetch
User->>DirectoryPicker: Click "New folder"
DirectoryPicker->>DirectoryPicker: "createFolderOpen = true"
User->>DirectoryPicker: Type name + click Create
DirectoryPicker->>CreateAPI: "POST {path: "/empty-folder/new-dir"}"
CreateAPI->>CreateAPI: isAbsolute + no-.. check\nparent exists check\nmkdir
CreateAPI-->>DirectoryPicker: "{success: true, path}"
DirectoryPicker->>API: browseDirectory(browser.currentPath)
API-->>DirectoryPicker: refreshed entries
Reviews (6): Last reviewed commit: "fix: disable Create button when currentP..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
- routes.ts: use isAbsolute() before resolve() for proper path validation - routes.ts: wrap mkdir() in try/catch to map EEXIST/ENOENT/ENOTDIR to 4xx - DirectoryPicker.tsx: use !browser.currentPath guard to prevent re-fetch loop - DirectoryPicker.tsx: disable Create button during in-flight requests - DirectoryPicker.tsx: add fetchEntries to useEffect dependency array - DirectoryPicker.tsx: client-side validation for folder names - Add changeset for the new create-folder feature
Collaborator
Author
|
@coderabbitai All actionable review comments have been addressed:
|
Collaborator
Author
|
@greptile-apps All feedback items have been addressed:
The empty-folder regression test now deterministically asserts the loop never fires (no mock exhaustion needed). |
Contributor
|
(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧ 🧠 Learnings used✅ Action performedReview finished.
|
Contributor
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)
packages/dashboard/app/components/DirectoryPicker.tsx (1)
67-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReopening after a browse error cannot recover.
Line 79 blocks initial fetch when
browser.erroris set, and Line 71 doesn’t clear that error when reopening. After one failure, closing/reopening the picker won’t trigger a retry.Suggested fix
const handleToggleBrowser = useCallback(() => { setBrowser((prev) => { if (!prev.isOpen) { // Opening — fetch entries - return { ...prev, isOpen: true }; + return { ...prev, isOpen: true, error: null }; } return { ...prev, isOpen: false }; }); }, []);Also applies to: 79-82
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/app/components/DirectoryPicker.tsx` around lines 67 - 74, The component never clears browser.error when reopening, so after one failed fetch the picker won’t retry; update handleToggleBrowser to clear browser.error (via setBrowser) when opening so the fetch-on-open path can run, and/or adjust the initial-fetch logic (the effect or function that triggers fetching when isOpen becomes true) to ignore a stale browser.error and attempt a retry; reference handleToggleBrowser, setBrowser, and browser.error (and the fetch-on-open effect/fetch function) when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/dashboard/app/components/DirectoryPicker.tsx`:
- Around line 123-129: The validation error string for new folder names is
hard-coded instead of localized; replace the literal message passed into
setBrowser(..., createFolderError: "...") with a call to the translation
function used elsewhere (e.g., t("...")) so the error is localizable — update
the block that checks trimmedName.includes(...) / trimmedName.includes("\\") /
trimmedName.includes("..") to set createFolderError using t(...) (reference
symbols: trimmedName, newFolderName, setBrowser, createFolderError, and the
component's t function).
- Line 108: The file contains an invalid ESLint suppression comment "//
eslint-disable-next-line react-hooks/exhaustive-deps" that references a rule the
project doesn't register; remove that suppression and either add the correct
dependencies to the related useEffect (in the DirectoryPicker component) or, if
a deliberate omission is required, replace it with a project-registered
suppression (e.g., remove the line and ensure the useEffect's dependency array
on the useEffect call in DirectoryPicker includes all referenced variables or
add a short comment explaining why a missing dependency is intentional). Locate
the offending comment near the useEffect inside the DirectoryPicker component
and remove it or fix the dependency array there so ESLint no longer needs the
undefined rule suppression.
---
Outside diff comments:
In `@packages/dashboard/app/components/DirectoryPicker.tsx`:
- Around line 67-74: The component never clears browser.error when reopening, so
after one failed fetch the picker won’t retry; update handleToggleBrowser to
clear browser.error (via setBrowser) when opening so the fetch-on-open path can
run, and/or adjust the initial-fetch logic (the effect or function that triggers
fetching when isOpen becomes true) to ignore a stale browser.error and attempt a
retry; reference handleToggleBrowser, setBrowser, and browser.error (and the
fetch-on-open effect/fetch function) when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a6d7594-d700-45c4-aadb-d0c882d6312c
📒 Files selected for processing (3)
.changeset/directory-picker-create-folder.mdpackages/dashboard/app/components/DirectoryPicker.tsxpackages/dashboard/src/routes.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/directory-picker-create-folder.md
- Add dirPicker.createFolderError to all locale files (en, es, fr, ko, zh-CN, zh-TW) - Use t() for the folder name validation error in DirectoryPicker - Add missing dirPicker keys: cancel, createFolder, createFolderAria, createFolderConfirm, createFolderTitle, newFolderPlaceholder
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.
Problem
When selecting a folder path in "Manage project" (Setup Wizard or Project Overview), if the selected folder was empty (contained no subdirectories), the DirectoryPicker would automatically revert to the previous folder after navigation.
Root Cause
The
useEffectinDirectoryPickerthat auto-fetches directory entries when the browser panel opens used thevalueprop to determine which path to fetch:After the user navigated into an empty folder:
entries: []entries.length === 0valueprop (the original path) instead ofbrowser.currentPath(the navigated path)Fix
Changed the effect to prioritize
browser.currentPathwhen available:New Feature: Create Folder Button
As requested, added a "New folder" button to the DirectoryPicker toolbar. Users can now create folders directly within the directory picker when setting up a project, allowing precise control over where and how to name the new project directory.
Changes
DirectoryPicker.tsxDirectoryPicker.cssroutes.tsPOST /api/create-directoryendpointapi/legacy.tscreateDirectory()API client functionDirectoryPicker.test.tsxTest Results
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Localization