feat(ui): add file explorer toolbar and file search modal#1408
feat(ui): add file explorer toolbar and file search modal#1408yashdev9274 wants to merge 3 commits intogeneralaction:mainfrom
Conversation
… rmdir) to IPC and local/remote file systems
- Fixed Windows path separator bug in remote fs:rename, fs:mkdir, fs:rmdir handlers - On Windows, path.join produces backslashes which break POSIX remote servers - Minor cleanup: unused variable removal in LocalFileSystem - UI: added padding to delete confirmation button in FileTree
- Added FileExplorerToolbar component with Search, New File, New Folder, and Collapse buttons - Created FileSearchModal for dedicated file search functionality within projects - Added Shift+Cmd+P (Mac) / Ctrl+Shift+P (Windows) keyboard shortcut for file search - Integrated toolbar with existing FileTree component - Fixed folder collapse issue after create/delete operations by reloading expanded folders - Added folder selection on left-click for creating files/folders in selected directory
|
@yashdev9274 is attempting to deploy a commit to the General Action Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR introduces a file explorer toolbar with Search, New File, New Folder, and Collapse All buttons, a dedicated Key issues found:
Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| src/renderer/components/FileExplorer/FileExplorerToolbar.tsx | New toolbar component with Search, New File, New Folder, and Collapse buttons — missing React import (compilation error) and has an unused ChevronDown import. |
| src/renderer/components/FileExplorer/FileSearchModal.tsx | New file search modal — only indexes top-level files (missing recursive: true in fsList call) and does not reset search state between modal openings. |
| src/renderer/components/FileExplorer/FileTree.tsx | Major additions: context menu with rename/delete/new file/new folder, toolbar integration, and file search wiring — contains dead code in handleToolbarSearch and a stale closure bug in loadAllFiles where expandedPaths and loadChildren are missing from the useCallback dependency array. |
| src/main/services/fsIpc.ts | Adds IPC handlers for fs:rename, fs:mkdir, and fs:rmdir — includes proper path traversal guards (startsWith(normRoot)) and delegates correctly to local/remote filesystem implementations. |
| src/main/services/fs/LocalFileSystem.ts | Adds rename (with source/destination existence checks) and mkdir (recursive) methods — straightforward and well-guarded implementations. |
| src/renderer/hooks/useKeyboardShortcuts.ts | Adds FILE_SEARCH shortcut (Shift+Cmd/Ctrl+P) that dispatches emdash:openFileSearch custom event to open the search modal. |
Sequence Diagram
sequenceDiagram
participant User
participant KB as useKeyboardShortcuts
participant FT as FileTree
participant FSM as FileSearchModal
participant IPC as Electron IPC
participant FS as LocalFileSystem / RemoteFileSystem
Note over User, FS: File Search Flow
User->>KB: Shift+Cmd/Ctrl+P
KB->>FT: dispatchEvent("emdash:openFileSearch")
FT->>FSM: isOpen=true
FSM->>IPC: fsList(rootPath, {includeDirs:true})
IPC->>FS: list()
FS-->>IPC: items[]
IPC-->>FSM: {success, items}
User->>FSM: types query / arrows
FSM-->>User: filtered results
User->>FSM: Enter / click
FSM->>FT: onSelectFile(path) + onClose()
FT->>FT: onOpenFile(path)
Note over User, FS: Context Menu File Operations
User->>FT: right-click node
FT-->>User: ContextMenu (Rename/Delete/New File/New Folder)
User->>FT: select action → dialog input
FT->>IPC: fsRename / fsRemove / fsMkdir / fsRmdir / fsWriteFile
IPC->>FS: rename() / remove() / mkdir()
FS-->>IPC: {success}
IPC-->>FT: {success}
FT->>FT: loadAllFiles() → reloadExpandedFolders()
Last reviewed commit: 543578b
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| onCollapse: () => void; | ||
| } | ||
|
|
||
| export const FileExplorerToolbar: React.FC<FileExplorerToolbarProps> = ({ |
There was a problem hiding this comment.
Missing React import
React.FC is referenced on this line but React is never imported. Unlike JSX usage, type-level references to React.FC require React to be explicitly in scope. This will cause a TypeScript compilation error.
| export const FileExplorerToolbar: React.FC<FileExplorerToolbarProps> = ({ | |
| import React from 'react'; | |
| export const FileExplorerToolbar: React.FC<FileExplorerToolbarProps> = ({ |
Or add the import at the top of the file alongside the existing imports:
| export const FileExplorerToolbar: React.FC<FileExplorerToolbarProps> = ({ | |
| import React from 'react'; | |
| import { ChevronDown, CopyMinus, FilePlus, FolderPlus, Search } from 'lucide-react'; | |
| import { Button } from '../ui/button'; |
| const fetchFiles = async () => { | ||
| const opts: any = { includeDirs: true }; | ||
| if (connectionId && remotePath) { | ||
| opts.connectionId = connectionId; | ||
| opts.remotePath = remotePath; | ||
| } | ||
|
|
||
| const result = await window.electronAPI.fsList(rootPath, opts); |
There was a problem hiding this comment.
File search only indexes top-level files
fsList is called without recursive: true, so it only returns files directly in the root directory. Nested files in subdirectories will never appear in search results, making this essentially a root-level file picker rather than a project-wide file search.
| const fetchFiles = async () => { | |
| const opts: any = { includeDirs: true }; | |
| if (connectionId && remotePath) { | |
| opts.connectionId = connectionId; | |
| opts.remotePath = remotePath; | |
| } | |
| const result = await window.electronAPI.fsList(rootPath, opts); | |
| const opts: any = { includeDirs: false, recursive: true }; |
Setting includeDirs: false (or keeping it true if directories should also be searchable) combined with recursive: true ensures the full project tree is indexed.
| const [searchQuery, setSearchQuery] = useState(''); | ||
| const [files, setFiles] = useState<Array<{ name: string; path: string; type: string }>>([]); | ||
| const [selectedIndex, setSelectedIndex] = useState(0); |
There was a problem hiding this comment.
State not reset between modal openings
searchQuery and selectedIndex are component state that persist between opens/closes. If a user types a query, closes the modal, and reopens it, the stale search text and cursor position remain. The file list is re-fetched (via the isOpen effect) but the input and selection are not cleared.
Consider resetting these when isOpen becomes true:
useEffect(() => {
if (isOpen) {
setSearchQuery('');
setSelectedIndex(0);
}
}, [isOpen]);
| const handleToolbarSearch = () => { | ||
| // Focus the search input | ||
| const handleToolbarSearch = () => { | ||
| // Detect platform | ||
| const isMac = /Mac|iPod|iPhone|iPad/.test(navigator.platform); | ||
|
|
||
| // Create keyboard event for Shift+Cmd+P (Mac) or Ctrl+Shift+P (Windows) | ||
| const key = isMac ? 'p' : 'p'; | ||
| const event = new KeyboardEvent('keydown', { | ||
| key: key, | ||
| metaKey: isMac, | ||
| ctrlKey: !isMac, | ||
| shiftKey: true, | ||
| bubbles: true, | ||
| }); | ||
|
|
||
| // Dispatch the event to trigger CommandPalette | ||
| document.dispatchEvent(event); | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Dead code: handleToolbarSearch outer function is never called and contains an unreachable inner function
The outer handleToolbarSearch function declares an inner handleToolbarSearch function but never invokes it — the body is effectively a no-op. Additionally, the outer function itself is never used; the toolbar's onSearch prop is wired to handleFileSearch (line 914), not handleToolbarSearch.
Also, const key = isMac ? 'p' : 'p' inside the inner function is always 'p' regardless of platform — the ternary has no effect.
This entire block should be removed:
| const reloadExpandedFolders = async () => { | ||
| const expandedArray = Array.from(expandedPaths); | ||
|
|
||
| for (const path of expandedArray) { | ||
| // Find the node in the new file list | ||
| const item = validItems.find((f: any) => f.path === path); | ||
| if (item) { | ||
| // Construct a proper FileNode with required properties | ||
| const node: FileNode = { | ||
| id: item.path, | ||
| name: item.path.split('/').pop() || item.path, | ||
| path: item.path, | ||
| type: item.type === 'dir' ? 'directory' : item.type, | ||
| }; | ||
| await loadChildren(node); | ||
| } | ||
| } | ||
| }; | ||
| // Trigger reload of expanded folders | ||
| setTimeout(reloadExpandedFolders, 0); | ||
| } catch (err) { | ||
| setError(err instanceof Error ? err.message : 'Failed to load files'); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }, [rootPath, connectionId, remotePath]); // Reload when rootPath or remote info changes |
There was a problem hiding this comment.
loadAllFiles captures stale expandedPaths and loadChildren
The useCallback dependency array only includes [rootPath, connectionId, remotePath] (line 554), but the reloadExpandedFolders closure references both expandedPaths and loadChildren. Because these are omitted from the deps, loadAllFiles is never recreated when expandedPaths changes — meaning calls to loadAllFiles after rename/delete operations (lines 704, 724, 741, 758) will use whatever expandedPaths was captured at the last rootPath/remote-info change, silently skipping any folders the user has since expanded.
loadChildren should be added to the dependency array. For expandedPaths, since it's a Set stored as state, it can also be added; React's ESLint plugin (react-hooks/exhaustive-deps) would flag this as a violation.
}, [rootPath, connectionId, remotePath, expandedPaths, loadChildren]);
Note: if this creates a circular dependency (since loadChildren is defined after loadAllFiles), consider extracting reloadExpandedFolders into its own useCallback that depends on expandedPaths and loadChildren, and call it separately after loadAllFiles resolves.
Thank you! Could we also use the same icons for each file in the search component that we are using in the left sidebar, where the user can see the files? |
you mean to use same icons for these files similar to one in file trees? |
|
Yes, |
this pr is ready to merge you can also review it if you want, there is no issue in this pr. I'll add more explorer features after merging so that this pr doesn't get bulkier |
|
hey @arnestrickmann any update on this PR |
2 participants
Summary
New PR for #1379 that contain feat of #1228 and pr of #1348
following changes:
Fixes
Global Fixes : #1379
local fixe: #1228
Snapshot
Feat-1379.mp4
Type of change
Mandatory Tasks
Checklist
pnpm run format)pnpm run lint)