File sidebar cleanup #469
File sidebar cleanup #469
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR significantly refactors file handling and sidebar functionality in the Reor application. Here's a focused analysis of the key changes:
- Improves error handling in WindowManager by attempting direct error delivery to active windows before queueing, though potential race conditions need attention
- Consolidates file movement operations through
renameFile, removing the separatemoveFileOrDirectoryfunctionality which could impact drag-and-drop performance - Implements platform-specific file rename handling with proper watcher management, particularly for Windows vs other OS differences
- Introduces directory selection state management in FileSidebar with proper path validation needed for drag-and-drop security
- Changes new note creation behavior to use selected directory instead of current file's directory, affecting user workflow expectations
The changes appear to streamline file operations but require careful validation of path handling and error cases.
11 file(s) reviewed, 12 comment(s)
Edit PR Review Bot Settings | Greptile
| appendNewErrorToDisplayInWindow(errorString: string) { | ||
| this.errorStringsToSendWindow.push(errorString) | ||
| let errorSent = false | ||
| const activeWindows = BrowserWindow.getAllWindows() | ||
| activeWindows.forEach((window) => { | ||
| if (!window.webContents.isLoading()) { | ||
| window.webContents.send('error-to-display-in-window', errorString) | ||
| errorSent = true | ||
| } | ||
| }) | ||
|
|
||
| if (!errorSent) { | ||
| this.errorStringsToSendWindow.push(errorString) | ||
| } | ||
| } |
There was a problem hiding this comment.
logic: Potential memory leak: no limit on errorStringsToSendWindow array size if errors keep occurring while windows are loading
| @@ -27,6 +30,58 @@ const convertFileTypeToDBType = async (file: FileInfo): Promise<DBEntry[]> => { | |||
| return entries | |||
| } | |||
|
|
|||
| export const handleFileRename = async ( | |||
| windowsManager: WindowsManager, | |||
| windowInfo: { vaultDirectoryForWindow: string; dbTableClient: any }, | |||
There was a problem hiding this comment.
style: dbTableClient is typed as 'any' - should be properly typed with LanceDBTableWrapper
| renameFileProps: RenameFileProps, | ||
| sender: Electron.WebContents, | ||
| ): Promise<void> => { | ||
| windowsManager.watcher?.unwatch(windowInfo.vaultDirectoryForWindow) |
There was a problem hiding this comment.
logic: watcher.unwatch() call not wrapped in try/catch - could throw if watcher is undefined
| } | ||
| } | ||
|
|
||
| if (process.platform === 'win32') { |
There was a problem hiding this comment.
logic: Windows-specific code path doesn't restore watcher if rename fails, could leave system in inconsistent state
| }) | ||
| } | ||
|
|
||
| await windowInfo.dbTableClient.updateDBItemsWithNewFilePath(renameFileProps.oldFilePath, renameFileProps.newFilePath) |
There was a problem hiding this comment.
logic: Database update should happen before file rename to maintain atomic operations - current order risks data inconsistency if rename fails
| const destinationDirectory = await window.electronStore.getVaultDirectoryForWindow() | ||
| const destinationPath = await window.path.join(destinationDirectory, await window.path.basename(sourcePath)) | ||
| renameFile(sourcePath, destinationPath) |
There was a problem hiding this comment.
logic: Multiple awaits on path operations are unnecessary and could be chained. Also missing validation of sourcePath before attempting rename operation.
| let filesAndIndexes: { file: FileInfoNode; indentation: number }[] = [] | ||
| files.forEach((file) => { | ||
| filesAndIndexes.push({ file, indentation }) | ||
| if (isFileNodeDirectory(file) && expandedDirectories.has(file.path) && expandedDirectories.get(file.path)) { |
There was a problem hiding this comment.
style: Redundant check - expandedDirectories.has(file.path) is unnecessary since get() will return undefined if key doesn't exist
| setCurrentlyChangingFilePath(false) | ||
| const parentDirectory = await window.path.dirname(filePath) | ||
| setSelectedDirectory(parentDirectory) |
There was a problem hiding this comment.
logic: setSelectedDirectory is called after setCurrentlyChangingFilePath=false which could cause race condition. Move before the flag reset.
| const pathSep = await window.path.pathSep() | ||
| const isAbsolute = await window.path.isAbsolute(filePath) | ||
| const basePath = isAbsolute ? '' : '.' |
There was a problem hiding this comment.
logic: Potential path handling issue - absolute paths on Windows may start with a drive letter, which this logic doesn't account for
| // eslint-disable-next-line no-restricted-syntax | ||
| for (const segment of pathSegments) { | ||
| // eslint-disable-next-line no-await-in-loop | ||
| currentPath = await window.path.join(currentPath, segment) | ||
| newExpandedDirectories.set(currentPath, true) |
There was a problem hiding this comment.
style: Using await in a loop could be slow for deeply nested paths. Consider using Promise.all() with path segments
No description provided.