Not found paths in prefix fs always treated as dir #2002
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces changes to the handling of non-existent objects and empty directories across various file system implementations. In the S3 file system, the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/remote/fileshare/wavefs/wavefs.go (1)
108-129: LGTM! The changes align well with the PR objective.The implementation correctly handles non-existent paths by representing them as empty directories, including a parent directory entry. This provides a consistent interface for directory navigation.
Consider extracting the parent directory entry creation into a helper function since this pattern might be reused elsewhere:
+func createParentDirEntry(dirPath string) *wshrpc.FileInfo { + return &wshrpc.FileInfo{ + Path: dirPath, + Dir: fspath.Dir(dirPath), + Name: "..", + IsDir: true, + Size: 0, + ModTime: time.Now().Unix(), + MimeType: "directory", + } +} if len(list) == 0 { dirPath := fspath.Dir(fileName) return &wshrpc.FileData{ Info: &wshrpc.FileInfo{ Name: fspath.Base(fileName), Path: fileName, Dir: fspath.Dir(fileName), NotFound: true, IsDir: true, }, - Entries: []*wshrpc.FileInfo{ - { - Path: dirPath, - Dir: fspath.Dir(dirPath), - Name: "..", - IsDir: true, - Size: 0, - ModTime: time.Now().Unix(), - MimeType: "directory", - }, - }}, nil + Entries: []*wshrpc.FileInfo{createParentDirEntry(dirPath)}}, nilpkg/remote/fileshare/s3fs/s3fs.go (1)
65-78: LGTM! Consistent implementation with wavefs.The changes handle non-existent paths similarly to wavefs, maintaining consistency across different file system implementations.
Consider using the same helper function suggested for wavefs to maintain consistency:
+func createParentDirEntry(dirPath string) *wshrpc.FileInfo { + return &wshrpc.FileInfo{ + Path: dirPath, + Dir: fspath.Dir(dirPath), + Name: "..", + IsDir: true, + Size: 0, + ModTime: time.Now().Unix(), + MimeType: "directory", + } +} if finfo.NotFound { - rtn <- wshrpc.RespOrErrorUnion[wshrpc.FileData]{Response: wshrpc.FileData{Entries: []*wshrpc.FileInfo{ - { - Path: finfo.Dir, - Dir: fspath.Dir(finfo.Dir), - Name: "..", - IsDir: true, - Size: 0, - ModTime: time.Now().Unix(), - MimeType: "directory", - }, - }}} + rtn <- wshrpc.RespOrErrorUnion[wshrpc.FileData]{Response: wshrpc.FileData{Entries: []*wshrpc.FileInfo{createParentDirEntry(finfo.Dir)}}} return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/remote/fileshare/s3fs/s3fs.go(2 hunks)pkg/remote/fileshare/wavefs/wavefs.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
pkg/remote/fileshare/s3fs/s3fs.go (1)
592-592: LGTM! Consistent with PR objective.Setting
IsDirto true for non-existent paths ensures consistent behavior with the ReadStream changes and aligns with the PR's goal of treating non-existent paths as directories.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/app/view/preview/preview.tsx (1)
1080-1085: Consider removing the debug log statement.The console log statement appears to be for debugging purposes and should be removed before merging to production.
- console.log("fileInfo or connection changed", fileInfo, connection);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/app/view/preview/directorypreview.tsx(1 hunks)frontend/app/view/preview/preview.tsx(3 hunks)pkg/remote/fileshare/s3fs/s3fs.go(2 hunks)pkg/remote/fileshare/wavefs/wavefs.go(1 hunks)pkg/wshrpc/wshremote/wshremote.go(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/wshrpc/wshremote/wshremote.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/remote/fileshare/s3fs/s3fs.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (3)
pkg/remote/fileshare/wavefs/wavefs.go (1)
108-117: LGTM! The changes improve the handling of non-existent paths.The implementation correctly represents non-existent paths as directories by returning a
FileDataobject with appropriate flags (NotFoundandIsDirset to true). This enhancement allows for better management and escaping of such paths.frontend/app/view/preview/directorypreview.tsx (2)
778-812: LGTM! The data fetching logic is well-implemented.The changes improve the directory content fetching by:
- Using
fireAndForgetfor clean async operation handling- Ensuring the parent directory ("..") is always included in the displayed data
- Properly handling and displaying errors to the user
814-827: LGTM! The filtering logic is optimized.The changes improve performance by:
- Moving filtering logic into a
useMemohook- Eliminating unnecessary state management
- Maintaining the same filtering functionality (hidden files and search)
Gracefully handle prefix paths that don't exist, representing them as directories so they can be escaped from.
Also removes the ".." file info from the backend, instead only creating it on the frontend