Add S3 fileshare implementation, improve cp behavior#1896
Conversation
|
Warning Rate limit exceeded@esimkowitz has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 13 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces extensive changes across the system’s file operations and RPC functionality. Function signatures have been updated to simplify parameters, improve timeout management (including a change from 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: 3
🧹 Nitpick comments (17)
pkg/util/utilfn/utilfn.go (1)
1021-1036: Consider enhancing the GracefulClose function.The function is well-implemented with retry logic and logging. However, consider the following improvements:
- Use exponential backoff instead of fixed delay for retries.
- Add context support for cancellation during retries.
- Include more descriptive error messages with error types.
Here's a suggested implementation:
-func GracefulClose(closer io.Closer, debugName string, closerName string, maxRetries int, retryDelay time.Duration) bool { +func GracefulClose(ctx context.Context, closer io.Closer, debugName string, closerName string, maxRetries int, initialDelay time.Duration) bool { closed := false for retries := 0; retries < maxRetries; retries++ { + select { + case <-ctx.Done(): + log.Printf("%s: context cancelled while closing %s: %v\n", debugName, closerName, ctx.Err()) + return false + default: + } + if err := closer.Close(); err != nil { - log.Printf("%s: error closing %s: %v, trying again in %dms\n", debugName, closerName, err, retryDelay.Milliseconds()) - time.Sleep(retryDelay) + backoff := initialDelay * time.Duration(1<<uint(retries)) + log.Printf("%s: error closing %s (type: %T): %v, trying again in %dms\n", debugName, closerName, err, err, backoff.Milliseconds()) + time.Sleep(backoff) continue } closed = true break } if !closed { log.Printf("%s: unable to close %s after %d retries\n", debugName, closerName, maxRetries) } return closed }pkg/remote/awsconn/awsconn.go (2)
47-67: Implement generic profile support and avoid hardcoding the AWS regionCurrently, the code hardcodes the AWS region to
"us-west-2", which reduces flexibility and may not suit all users. Reimplementing the generic profile support would allow users to specify their AWS profiles and regions dynamically.Do you want assistance in reimplementing the generic profile support to enhance configurability?
142-151: RefactorCheckAccessDeniedErrto follow idiomatic Go error handlingModifying errors through a pointer parameter (
*error) is not idiomatic in Go and can lead to confusion. Consider refactoringCheckAccessDeniedErrto return the modified error instead of altering it in place.Apply this diff to refactor the function and its usage:
-func CheckAccessDeniedErr(err *error) bool { +func CheckAccessDeniedErr(err error) error { var apiErr smithy.APIError - if err != nil && errors.As(*err, &apiErr) && apiErr.ErrorCode() == "AccessDenied" { - *err = apiErr - return true + if err != nil && errors.As(err, &apiErr) && apiErr.ErrorCode() == "AccessDenied" { + return apiErr } - return false + return err }Update the call site in
ListBuckets:-CheckAccessDeniedErr(&err) +err = CheckAccessDeniedErr(err)pkg/util/fileutil/fileutil.go (1)
209-220: Avoid usingpanicinToFsFileInfoUsing
panicto handle nil pointers inToFsFileInfois discouraged as it can cause the entire program to crash unexpectedly. Instead, consider returning an error or explicitly documenting thatfimust not benil.Modify the function to return an error:
-func ToFsFileInfo(fi *wshrpc.FileInfo) FsFileInfo { +func ToFsFileInfo(fi *wshrpc.FileInfo) (FsFileInfo, error) { if fi == nil { - panic("ToFsFileInfo: nil FileInfo") + return FsFileInfo{}, fmt.Errorf("ToFsFileInfo: nil FileInfo") } fsFileInfo := FsFileInfo{ NameInternal: fi.Name, ModeInternal: fi.Mode, SizeInternal: fi.Size, ModTimeInternal: fi.ModTime, IsDirInternal: fi.IsDir, } - return fsFileInfo + return fsFileInfo, nil }And handle the error appropriately in the calling code.
pkg/remote/fileshare/pathtree/pathtree.go (4)
8-19: Consider using a more efficient data structure for large trees.The current implementation uses a map to store all nodes, which could lead to memory overhead for large trees. Consider using a more memory-efficient data structure or implementing a cleanup mechanism for unused nodes.
47-70: Remove debug logging statements in production code.The
Addmethod contains multiple debug logging statements that should be removed or replaced with proper logging levels.- log.Printf("tree.Add: path: %s", path) - log.Printf("relativePath: %s", relativePath) - log.Printf("components: %v", components) - log.Printf("component: %s", component) - log.Printf("Adding component: %s", component) - log.Printf("curPath: %s", curPath)
71-83: Remove commented-out code.The commented-out optimization code should be either implemented or removed entirely. If it's kept for future reference, add a comment explaining why it's preserved and what conditions would make it viable.
101-109: Remove debug logging in the Walk method.The
Walkmethod contains a debug logging statement that should be removed or replaced with proper logging levels.- log.Printf("RootPath: %s", t.RootPath)pkg/remote/fileshare/fstype/fstype.go (2)
15-17: Consider making DefaultTimeout configurable.The hardcoded 30-second timeout might not be suitable for all environments or file systems. Consider making it configurable through environment variables or configuration files.
50-51: Document the GetCapability method.The new method lacks documentation explaining what capabilities are available and their implications.
- // GetCapability returns the capability of the fileshare + // GetCapability returns the capability of the fileshare, indicating supported operations + // such as file appending and directory creation. This information is used by clients + // to determine available functionality before attempting operations.pkg/remote/fileshare/pathtree/pathtree_test.go (2)
11-52: Add edge case tests for TestAdd.The current test cases don't cover important edge cases:
- Adding paths with empty components
- Adding paths with special characters
- Adding extremely long paths
- Adding paths with different delimiters
100-112: Remove debug logging from test helper.The
initializeTreefunction contains a debug logging statement that's unnecessary for tests.- log.Printf("tree: %v", tree)pkg/remote/fileshare/wshfs/wshfs.go (1)
148-150: Consider making capabilities dynamic and adding documentation.The hardcoded capabilities might not reflect the actual system capabilities. Consider:
- Making capabilities dynamic based on system checks
- Adding documentation explaining what these capabilities mean and how they affect file operations
+// GetCapability returns the file sharing capabilities of this client. +// - CanAppend: indicates whether the client supports appending to existing files +// - CanMkdir: indicates whether the client supports creating directories func (c WshClient) GetCapability() wshrpc.FileShareCapability { - return wshrpc.FileShareCapability{CanAppend: true, CanMkdir: true} + return wshrpc.FileShareCapability{ + CanAppend: c.checkAppendSupport(), + CanMkdir: c.checkMkdirSupport(), + } } + +func (c WshClient) checkAppendSupport() bool { + // TODO: Implement actual system capability check + return true +} + +func (c WshClient) checkMkdirSupport() bool { + // TODO: Implement actual system capability check + return true +}pkg/remote/fileshare/fileshare.go (1)
173-179: Consider adding context timeout and improving error message.The function could be enhanced with:
- Context timeout for client creation
- More specific error message
-func GetCapability(ctx context.Context, path string) (wshrpc.FileShareCapability, error) { +func GetCapability(ctx context.Context, path string) (wshrpc.FileShareCapability, error) { client, conn := CreateFileShareClient(ctx, path) if conn == nil || client == nil { - return wshrpc.FileShareCapability{}, fmt.Errorf(ErrorParsingConnection, path) + return wshrpc.FileShareCapability{}, fmt.Errorf("failed to create file share client for path %q: %w", path, fmt.Errorf(ErrorParsingConnection, path)) } return client.GetCapability(), nil }cmd/wsh/cmd/wshcmd-file.go (1)
204-204: Consider extracting constants and standardizing error messages.The file operations have been improved with consistent timeout handling, but there are opportunities for further enhancement:
- Extract magic numbers into named constants
- Standardize error messages for file size limits
+const ( + // BatchSize is the size of chunks for batch processing + BatchSize = 8192 // 8KB + + // MaxFileSizeExceededMsg is the standard error message for file size limit + MaxFileSizeExceededMsg = "input exceeds maximum file size of %d bytes" +) func fileWriteRun(cmd *cobra.Command, args []string) error { // ... - if int64(n) == MaxFileSize { - if _, err := WrappedStdin.Read(make([]byte, 1)); err != io.EOF { - return fmt.Errorf("input exceeds maximum file size of %d bytes", MaxFileSize) + if int64(n) == MaxFileSize { + if _, err := WrappedStdin.Read(make([]byte, 1)); err != io.EOF { + return fmt.Errorf(MaxFileSizeExceededMsg, MaxFileSize) } } // ... } func fileAppendRun(cmd *cobra.Command, args []string) error { // ... - if buf.Len() >= 8192 { // 8KB batch size + if buf.Len() >= BatchSize { fileData.Data64 = base64.StdEncoding.EncodeToString(buf.Bytes()) // ... } // ... }Also applies to: 221-221, 257-257, 274-299, 313-313, 342-342, 353-353
pkg/wshrpc/wshrpctypes.go (1)
728-731: Add documentation comments for the FileShareCapability struct.Consider adding documentation comments to describe the purpose of the struct and its fields. This will help other developers understand when and how to use these capabilities.
+// FileShareCapability represents the operations allowed on a file share type FileShareCapability struct { + // CanAppend indicates whether appending to files is allowed CanAppend bool + // CanMkdir indicates whether creating directories is allowed CanMkdir bool }frontend/types/gotypes.d.ts (1)
444-448: Convert property names to camelCase for TypeScript conventions.The property names use PascalCase, which is inconsistent with TypeScript naming conventions. Consider converting them to camelCase.
// wshrpc.FileShareCapability type FileShareCapability = { - CanAppend: boolean; - CanMkdir: boolean; + canAppend: boolean; + canMkdir: boolean; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
cmd/wsh/cmd/wshcmd-file-util.go(4 hunks)cmd/wsh/cmd/wshcmd-file.go(9 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/remote/awsconn/awsconn.go(4 hunks)pkg/remote/connparse/connparse.go(2 hunks)pkg/remote/fileshare/fileshare.go(3 hunks)pkg/remote/fileshare/fstype/fstype.go(2 hunks)pkg/remote/fileshare/pathtree/pathtree.go(1 hunks)pkg/remote/fileshare/pathtree/pathtree_test.go(1 hunks)pkg/remote/fileshare/s3fs/s3fs.go(2 hunks)pkg/remote/fileshare/wavefs/wavefs.go(6 hunks)pkg/remote/fileshare/wshfs/wshfs.go(6 hunks)pkg/util/fileutil/fileutil.go(2 hunks)pkg/util/iochan/iochan.go(2 hunks)pkg/util/tarcopy/tarcopy.go(3 hunks)pkg/util/utilfn/utilfn.go(2 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshremote/wshremote.go(4 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/util/iochan/iochan.go
🧰 Additional context used
📓 Learnings (1)
pkg/remote/fileshare/wshfs/wshfs.go (1)
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
🪛 golangci-lint (1.62.2)
pkg/util/fileutil/fileutil.go
293-293: ineffectual assignment to entries
(ineffassign)
⏰ 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 (22)
pkg/util/tarcopy/tarcopy.go (2)
39-77: LGTM!The changes to the
TarCopySrcfunction are well-structured and improve the code in several ways:
- Proper path prefix handling with slash suffix check.
- Enhanced error handling using the new
GracefulCloseutility function.- Improved debugging with additional logging statements.
92-131: LGTM!The changes to the
TarCopyDestfunction improve resource management by using the newGracefulCloseutility function while maintaining proper error handling and context cancellation.pkg/remote/connparse/connparse.go (1)
50-52: LGTM! Improved empty path handling.The new condition correctly handles empty paths by returning just the host.
cmd/wsh/cmd/wshcmd-file-util.go (2)
31-49: LGTM! Clean and consistent implementation.The function has been simplified by removing the unused parameter and uses consistent timeout handling across all RPC operations.
93-96: LGTM! Improved streaming implementation.The function has been enhanced with context support and efficient streaming capabilities, delegating to specialized functions for better maintainability.
pkg/remote/fileshare/wshfs/wshfs.go (2)
30-33: LGTM! Simplified implementation using common utilities.The function has been streamlined by delegating to
fileutil.ReadStreamToFileData, improving code reuse and maintainability.
47-47: LGTM! Standardized timeout handling.Timeout values are now consistently using
fstype.DefaultTimeout.Milliseconds()across all methods, improving configuration management.Also applies to: 112-112, 127-127
pkg/remote/fileshare/fileshare.go (1)
33-39: LGTM! Improved S3 client initialization.The S3 client creation now properly retrieves AWS configuration and includes appropriate error handling.
cmd/wsh/cmd/wshcmd-file.go (1)
85-85: LGTM! Improved timeout type and default value.The timeout handling has been enhanced with:
- int64 type for better precision
- Reasonable default value of 15 seconds
Also applies to: 90-90
pkg/remote/fileshare/wavefs/wavefs.go (4)
53-53: LGTM: Improved error handling for context cancellation.The change from
ctx.Err()tocontext.Cause(ctx)enhances error reporting by preserving the original error cause.Also applies to: 62-62
115-115: LGTM: Improved timeout configuration.Moving the timeout configuration to
fstype.DefaultTimeoutenhances maintainability and ensures consistency across the codebase.
446-457: LGTM: Enhanced file operation safety.Added proper validation for file operations with clear error messages, preventing accidental overwrites when multiple entries exist at the destination.
553-558: LGTM: Well-implemented capability reporting.The GetCapability implementation correctly reflects the actual capabilities of WaveFS, matching the behavior of the AppendFile and Mkdir methods.
frontend/app/store/wshclientapi.ts (1)
205-214: LGTM: Well-structured RPC command additions.The new commands follow the established patterns:
- FileReadStreamCommand correctly implements streaming using AsyncGenerator
- FileShareCapabilityCommand properly implements capability checking
pkg/wshrpc/wshclient/wshclient.go (1)
252-261: LGTM: Well-implemented RPC client commands.The commands use appropriate helper functions:
- FileReadStreamCommand correctly uses responseStreamHelper for streaming
- FileShareCapabilityCommand properly uses callHelper for RPC calls
pkg/wshrpc/wshremote/wshremote.go (3)
294-294: LGTM: Fixed potential resource leak.Added proper resource cleanup by ensuring file handles are closed using defer.
380-438: LGTM: Enhanced logging for file operations.Added detailed logging that provides better visibility into file operations, making debugging and monitoring easier.
671-684: LGTM: Improved error handling in RemoteFileMoveCommand.Enhanced error handling with:
- Clearer error messages using cleaned paths
- Proper null checks for destination info
pkg/wshrpc/wshrpctypes.go (2)
68-72: LGTM! Command constants follow the established pattern.The new command constants
Command_FileReadStreamandCommand_FileShareCapabilityare well-named and consistent with the existing command naming convention.
163-164: LGTM! Interface methods are well-defined.The new methods
FileReadStreamCommandandFileShareCapabilityCommandare properly integrated into the interface with appropriate return types for streaming and capability checking.Also applies to: 169-170
pkg/wshrpc/wshserver/wshserver.go (2)
337-339: LGTM! Clean implementation of FileReadStreamCommand.The implementation correctly delegates to
fileshare.ReadStreamwhile maintaining the context and data parameters.
382-384: LGTM! Clean implementation of FileShareCapabilityCommand.The implementation correctly delegates to
fileshare.GetCapabilitywhile maintaining the context and path parameters.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
pkg/remote/fileshare/s3fs/s3fs.go (3)
274-313: Consider implementing rate limiting for concurrent S3 requests.The concurrent object fetching could potentially hit S3 API limits. Consider implementing rate limiting or exponential backoff strategy.
Example implementation using a rate limiter:
+import "golang.org/x/time/rate" +// Add to S3Client struct +limiter *rate.Limiter +// Initialize in NewS3Client +limiter: rate.NewLimiter(rate.Limit(100), 1), // 100 requests per second +// Add before making S3 requests +if err := c.limiter.Wait(ctx); err != nil { + return err +}
584-600: Enhance logging for better observability.Consider adding debug logs for better troubleshooting of file operations.
Add logging for operation progress:
func (c S3Client) PutFile(ctx context.Context, conn *connparse.Connection, data wshrpc.FileData) error { + log.Printf("starting PutFile operation for %s/%s", conn.Host, conn.Path) if data.At != nil { return errors.Join(errors.ErrUnsupported, fmt.Errorf("file data offset and size not supported")) } bucket := conn.Host objectKey := conn.Path if bucket == "" || bucket == "/" || objectKey == "" || objectKey == "/" { return errors.Join(errors.ErrUnsupported, fmt.Errorf("bucket and object key must be specified")) } + log.Printf("uploading %d bytes to %s/%s", base64.StdEncoding.DecodedLen(len(data.Data64)), bucket, objectKey) _, err := c.client.PutObject(ctx, &s3.PutObjectInput{ Bucket: aws.String(bucket), Key: aws.String(objectKey), Body: base64.NewDecoder(base64.StdEncoding, strings.NewReader(data.Data64)), ContentLength: aws.Int64(int64(base64.StdEncoding.DecodedLen(len(data.Data64)))), }) + if err != nil { + log.Printf("error uploading to %s/%s: %v", bucket, objectKey, err) + } else { + log.Printf("successfully uploaded to %s/%s", bucket, objectKey) + } return err }
372-372: Optimize the regular expression for better performance.The regex for splitting paths could be compiled once and reused.
Move the regex to package level:
-var slashRe = regexp.MustCompile(`/`) +var ( + // slashRe is used for splitting paths + slashRe = regexp.MustCompile(`/`) +)pkg/util/fileutil/fileutil.go (1)
222-282: Consider enhancing error messages for better debugging.The function is well-implemented with proper error handling and cleanup. Consider making the error messages more specific by including more context.
Apply this diff to improve error messages:
- return fmt.Errorf("stream file protocol error, first pk fileinfo is empty") + return fmt.Errorf("stream file protocol error: first packet fileinfo is empty") - return fmt.Errorf("stream file protocol error, directory entry has data") + return fmt.Errorf("stream file protocol error: unexpected data in directory entry")pkg/remote/connparse/connparse_test.go (1)
403-439: Well-structured test cases for S3 bucket URIs.The new test function
TestParseURI_S3BucketOnlyeffectively:
- Tests S3 bucket URIs with and without trailing slashes
- Validates all URI components (path, host, type)
- Uses a helper function to reduce code duplication
- Aligns with the PR objective of adding S3 fileshare implementation
However, consider adding test cases for:
- Invalid bucket names
- Special characters in bucket names
- Region-specific endpoints
Example test cases to add:
func TestParseURI_S3BucketOnly(t *testing.T) { // ... existing test cases ... + t.Log("Testing with invalid bucket name") + c, err := connparse.ParseURI("profile:s3://bucket..invalid") + if err == nil { + t.Fatal("expected error for invalid bucket name") + } + + t.Log("Testing with special characters") + testUri("profile:s3://my.bucket.com", "", "my.bucket.com") + + t.Log("Testing with region endpoint") + testUri("profile:s3://bucket.s3.us-west-2.amazonaws.com", "", "bucket.s3.us-west-2.amazonaws.com") }pkg/remote/fileshare/pathtree/pathtree.go (3)
3-6: Consider using a proper logging framework instead of the standardlogpackage.The standard
logpackage is being used for debug logging. For production code, consider using a more sophisticated logging framework (e.g.,logrus,zap) that supports:
- Log levels (debug, info, error)
- Structured logging
- Better performance
21-31: Add protection against stack overflow for deep directory structures.The recursive
Walkimplementation could potentially cause stack overflow for very deep directory structures. Consider:
- Adding a maximum depth limit
- Using an iterative approach with a stack for very deep trees
func (n *Node) Walk(curPath string, walkFunc WalkFunc, delimiter string) error { + const maxDepth = 1000 // Adjust based on your needs + return n.walkWithDepth(curPath, walkFunc, delimiter, 0, maxDepth) +} + +func (n *Node) walkWithDepth(curPath string, walkFunc WalkFunc, delimiter string, depth, maxDepth int) error { + if depth > maxDepth { + return fmt.Errorf("maximum depth of %d exceeded", maxDepth) + } if err := walkFunc(curPath, len(n.Children)); err != nil { return err } for name, child := range n.Children { - if err := child.Walk(curPath+delimiter+name, walkFunc, delimiter); err != nil { + if err := child.walkWithDepth(curPath+delimiter+name, walkFunc, delimiter, depth+1, maxDepth); err != nil { return err } } return nil }
101-109: Add documentation for the Walk method.The
Walkmethod should be documented with:
- Method description
- Parameters explanation
- Return value description
- Example usage
+// Walk traverses the tree in depth-first order, calling walkFunc for each node. +// The walkFunc receives the full path to the node and the number of children. +// If walkFunc returns an error, traversal stops and the error is returned. +// +// Example: +// +// tree := pathtree.NewTree("/root", "/") +// tree.Add("/root/dir1/file1") +// err := tree.Walk(func(path string, numChildren int) error { +// fmt.Printf("Path: %s, Children: %d\n", path, numChildren) +// return nil +// }) func (t *Tree) Walk(walkFunc WalkFunc) error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/remote/connparse/connparse_test.go(15 hunks)pkg/remote/fileshare/pathtree/pathtree.go(1 hunks)pkg/remote/fileshare/s3fs/s3fs.go(2 hunks)pkg/util/fileutil/fileutil.go(2 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 (9)
pkg/remote/fileshare/s3fs/s3fs.go (2)
37-40: LGTM! Well-defined file mode constants.The file mode constants are correctly defined using standard Unix permissions.
59-159: LGTM! Robust ReadStream implementation.The implementation includes:
- Proper context cancellation handling
- Comprehensive error handling
- Memory-efficient buffer management
- Proper resource cleanup
pkg/util/fileutil/fileutil.go (4)
7-10: LGTM!The new imports are correctly added and organized to support the new streaming functionality.
209-220: LGTM! Good defensive programming.The changes improve the function by:
- Adding a nil check with a clear panic message
- Improving readability with an intermediate variable
315-323: LGTM!The function efficiently streams data directly to the writer without accumulating it in memory.
284-313: Consider adding a size limit for in-memory data.The function accumulates all file data in memory before base64 encoding. This could lead to memory issues with large files.
Consider adding a size limit check:
func ReadStreamToFileData(ctx context.Context, readCh <-chan wshrpc.RespOrErrorUnion[wshrpc.FileData]) (*wshrpc.FileData, error) { var fileData *wshrpc.FileData var dataBuf bytes.Buffer var entries []*wshrpc.FileInfo + const maxSize = 100 * 1024 * 1024 // 100MB limit + var totalSize int64 err := ReadFileStream(ctx, readCh, func(finfo wshrpc.FileInfo) { fileData = &wshrpc.FileData{ Info: &finfo, } }, func(fileEntries []*wshrpc.FileInfo) error { entries = append(entries, fileEntries...) return nil }, func(data io.Reader) error { + n, err := io.Copy(&dataBuf, io.LimitReader(data, maxSize-totalSize)) + if err != nil { + return err + } + totalSize += n + if totalSize >= maxSize { + return fmt.Errorf("file too large: exceeds %d bytes", maxSize) + } - if _, err := io.Copy(&dataBuf, data); err != nil { - return err - } return nil })pkg/remote/connparse/connparse_test.go (2)
20-20: Improved error message formatting for better readability.The changes consistently format error messages with escaped double quotes around expected and actual values, which improves readability when tests fail.
Also applies to: 24-24, 29-29, 33-33, 47-47, 51-51, 56-56, 60-60, 67-67, 82-82, 85-85, 89-89, 93-93, 104-104, 108-108, 112-112, 116-116, 127-127, 131-131, 135-135, 139-139, 154-154, 158-158, 162-162, 166-166, 177-177, 181-181, 185-185, 189-189, 201-201, 205-205, 209-209, 213-213, 225-225, 229-229, 233-233, 237-237, 250-250, 253-253, 257-257, 267-267, 270-270, 274-274, 278-278, 293-293, 297-297, 301-301, 305-305, 327-327, 331-331, 335-335, 339-339, 358-358, 362-362, 366-366, 370-370, 383-383, 387-387, 392-392, 396-396
403-439: Verify S3 bucket name validation implementation.Let's check if the S3 bucket name validation is implemented in the main package.
pkg/remote/fileshare/pathtree/pathtree.go (1)
8-19: Consider adding thread safety mechanisms.The
TreeandNodetypes are not thread-safe. If this package will be used in concurrent contexts, consider:
- Adding mutex protection for the
nodesmap andChildrenmaps- Documenting thread safety guarantees in the type documentation
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/util/utilfn/utilfn.go (1)
1028-1043: Consider enhancing error handling and documentation.The implementation is good but could benefit from some improvements:
- The function lacks documentation comments describing its purpose, parameters, and return value.
- The error handling could be more robust by capturing and aggregating errors.
Here's a suggested improvement:
+// GracefulClose attempts to close an io.Closer with retry logic. +// Parameters: +// - closer: The io.Closer to be closed +// - debugName: Name used in logging for debugging context +// - closerName: Name of the closer for logging +// - maxRetries: Maximum number of retry attempts +// - retryDelay: Duration to wait between retries +// Returns: +// - bool: true if successfully closed, false if all retries failed func GracefulClose(closer io.Closer, debugName string, closerName string, maxRetries int, retryDelay time.Duration) bool { + if closer == nil { + return true + } closed := false + var lastErr error for retries := 0; retries < maxRetries; retries++ { if err := closer.Close(); err != nil { + lastErr = err log.Printf("%s: error closing %s: %v, trying again in %dms\n", debugName, closerName, err, retryDelay.Milliseconds()) time.Sleep(retryDelay) continue @@ -1039,7 +1052,7 @@ } if !closed { - log.Printf("%s: unable to close %s after %d retries\n", debugName, closerName, maxRetries) + log.Printf("%s: unable to close %s after %d retries, last error: %v\n", debugName, closerName, maxRetries, lastErr) } return closed }cmd/wsh/cmd/wshcmd-file.go (2)
274-277: Improve error message for capability check.The error message could be more specific about what went wrong with the capability check.
- return fmt.Errorf("getting fileshare capability: %w", err) + return fmt.Errorf("failed to check if file can be appended to: %w", err)
556-556: Use configurable timeout for consistency.The function uses a hardcoded timeout of 2000ms while other functions use the configurable
fileTimeout. Consider using the configurable timeout for consistency.- filesChan := wshclient.FileListStreamCommand(RpcClient, wshrpc.FileListData{Path: path, Opts: &wshrpc.FileListOpts{All: recursive}}, &wshrpc.RpcOpts{Timeout: 2000}) + filesChan := wshclient.FileListStreamCommand(RpcClient, wshrpc.FileListData{Path: path, Opts: &wshrpc.FileListOpts{All: recursive}}, &wshrpc.RpcOpts{Timeout: fileTimeout})pkg/util/fileutil/fileutil.go (2)
245-305: Enhance error handling in the streaming protocol.The function handles basic error cases well but could benefit from additional error handling:
- Validate that
fileInfoCallbackis not nil before first packet.- Validate that
dirCallbackis not nil when handling directory entries.- Validate that
fileCallbackis not nil when handling file data.Apply this diff to add the validations:
func ReadFileStream(ctx context.Context, readCh <-chan wshrpc.RespOrErrorUnion[wshrpc.FileData], fileInfoCallback func(finfo wshrpc.FileInfo), dirCallback func(entries []*wshrpc.FileInfo) error, fileCallback func(data io.Reader) error) error { + if fileInfoCallback == nil { + return fmt.Errorf("fileInfoCallback is required") + } + if dirCallback == nil { + return fmt.Errorf("dirCallback is required") + } + if fileCallback == nil { + return fmt.Errorf("fileCallback is required") + } var fileData *wshrpc.FileData firstPk := true isDir := false
338-346: Handle directory entries in ReadFileStreamToWriter.The function silently ignores directory entries. Consider returning an error when attempting to write a directory to prevent confusion.
Apply this diff to add directory handling:
func ReadFileStreamToWriter(ctx context.Context, readCh <-chan wshrpc.RespOrErrorUnion[wshrpc.FileData], writer io.Writer) error { + var isDir bool return ReadFileStream(ctx, readCh, func(finfo wshrpc.FileInfo) { + isDir = finfo.IsDir }, func(entries []*wshrpc.FileInfo) error { + if isDir { + return fmt.Errorf("cannot write directory entries to writer") + } return nil }, func(data io.Reader) error { + if isDir { + return fmt.Errorf("cannot write directory entries to writer") + } _, err := io.Copy(writer, data) return err }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/wsh/cmd/wshcmd-file.go(9 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/remote/connparse/connparse.go(2 hunks)pkg/remote/connparse/connparse_test.go(15 hunks)pkg/util/fileutil/fileutil.go(2 hunks)pkg/util/utilfn/utilfn.go(2 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshremote/wshremote.go(3 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- frontend/types/gotypes.d.ts
- pkg/remote/connparse/connparse_test.go
- pkg/remote/connparse/connparse.go
- frontend/app/store/wshclientapi.ts
- pkg/wshrpc/wshremote/wshremote.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (go)
🔇 Additional comments (12)
pkg/util/utilfn/utilfn.go (1)
18-18: LGTM!The addition of the "log" package is appropriate for the new logging functionality.
cmd/wsh/cmd/wshcmd-file.go (6)
33-33: LGTM! Good choice using int64 for timeout values.The change from
inttoint64forfileTimeoutis a good improvement as it prevents potential integer overflow when dealing with large timeout values.Also applies to: 84-84
89-89: LGTM! Flag type correctly updated.The flag type has been correctly updated to
Int64VarPto match the variable type change, maintaining type consistency.
203-203: LGTM! Good improvement using streaming.The change to use streaming for file reading is a good improvement as it allows for efficient handling of large files without loading them entirely into memory.
220-220: LGTM! Proper timeout configuration.The function correctly uses the configurable timeout value for the RPC call.
278-299: LGTM! Good improvements in file write handling.The changes include:
- Proper capability check before writing
- Thorough input validation for file size
- Efficient handling of file data using streaming when possible
313-357: LGTM! Good implementation of buffered append with size validation.The implementation includes:
- Proper validation to prevent exceeding maximum file size
- Efficient buffered writing with reasonable batch size
- Comprehensive error handling
pkg/util/fileutil/fileutil.go (1)
307-336: LGTM! The implementation looks good.The function correctly:
- Accumulates file data using a bytes.Buffer
- Handles both directory entries and file data
- Properly encodes file data in base64
- Returns appropriate errors
pkg/wshrpc/wshclient/wshclient.go (2)
258-261: LGTM! The implementation follows established patterns.The function correctly:
- Uses the streaming helper function
- Matches the interface definition
- Uses appropriate types
263-267: LGTM! The implementation follows established patterns.The function correctly:
- Uses the RPC helper function
- Matches the interface definition
- Uses appropriate types
pkg/wshrpc/wshrpctypes.go (2)
68-68: LGTM! The constant follows established patterns.The constant is correctly:
- Named according to convention
- Placed with related commands
- Valued appropriately
72-72: LGTM! The constant follows established patterns.The constant is correctly:
- Named according to convention
- Placed with related commands
- Valued appropriately
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pkg/remote/fileshare/fspath/fspath.go (2)
8-11: Enhance constant documentation.Consider adding more context to the comment, explaining why forward slash is used and its significance in cross-platform path handling.
const ( - // Separator is the path separator + // Separator is the forward slash (/) character used as the standard path separator. + // This ensures consistent path handling across different platforms, including Windows. Separator = "/" )
13-15: Add function documentation and consider error handling.The functions look good but would benefit from:
- GoDoc comments explaining their purpose, parameters, and return values
- Error handling for edge cases (e.g., empty paths, invalid characters)
Example improvement:
+// Dir returns the directory portion of the path. +// It ensures consistent handling of path separators across platforms +// by converting backslashes to forward slashes. +// For example: +// Dir("a/b/c") returns "a/b" +// Dir("a/b/c/") returns "a/b/c" func Dir(path string) string { + if path == "" { + return "." + } return pathpkg.Dir(ToSlash(path)) }Similar documentation and error handling should be added to other functions.
Also applies to: 17-19, 21-24, 26-28
pkg/remote/fileshare/wavefs/wavefs.go (1)
4-4: Reminder: Address the TODO comment.The TODO comment indicates that tests are missing for this function. Please ensure that the additional parameter change is thoroughly tested to confirm that it behaves as expected.
Do you want me to generate the unit testing code or open a new issue to track this task?
pkg/remote/fileshare/s3fs/s3fs.go (1)
567-567: Remove redundant nil check.The nil check is redundant as
len()for[]types.Objectis defined as zero.Apply this diff to fix the issue:
- if entries.Contents != nil && len(entries.Contents) > 0 { + if len(entries.Contents) > 0 {🧰 Tools
🪛 golangci-lint (1.62.2)
567-567: S1009: should omit nil check; len() for []github.com/aws/aws-sdk-go-v2/service/s3/types.Object is defined as zero
(gosimple)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/remote/fileshare/fspath/fspath.go(1 hunks)pkg/remote/fileshare/fsutil/fsutil.go(1 hunks)pkg/remote/fileshare/s3fs/s3fs.go(2 hunks)pkg/remote/fileshare/wavefs/wavefs.go(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/remote/fileshare/fsutil/fsutil.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/remote/fileshare/s3fs/s3fs.go
567-567: S1009: should omit nil check; len() for []github.com/aws/aws-sdk-go-v2/service/s3/types.Object is defined as zero
(gosimple)
🔇 Additional comments (5)
pkg/remote/fileshare/fspath/fspath.go (1)
1-6: LGTM! Clean package structure and imports.The package name is descriptive, and the import alias
pathpkghelps avoid naming conflicts.pkg/remote/fileshare/wavefs/wavefs.go (1)
1-2: LGTM!The function logic is correct, and the implementation is accurate.
pkg/remote/fileshare/s3fs/s3fs.go (3)
53-156: LGTM!The implementation is robust and handles various edge cases correctly:
- Proper handling of both file and directory reads
- Proper handling of context cancellation
- Proper error handling
- Proper base64 encoding of data before assigning it to
Data64
158-346: LGTM!The implementation is robust and handles various edge cases correctly:
- Proper handling of both single file and directory reads
- Proper handling of context cancellation
- Proper error handling
- Proper resource cleanup
- Proper handling of concurrent operations
803-808: LGTM!The implementation correctly indicates that S3 does not support appending files or creating directories.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/remote/fileshare/fspath/fspath.go (1)
26-33: Improve documentation forFirstLevelDirfunction.The function's documentation could be enhanced to:
- Explain what constitutes a "level"
- Provide examples of input/output
- Document edge cases
Apply this diff to improve the documentation:
-// FirstLevelDir returns the first level directory of a path and a boolean indicating if the path has more than one level. +// FirstLevelDir returns the first level directory of a path and a boolean indicating if the path has more than one level. +// For example: +// - "foo/bar/baz" returns ("foo", true) +// - "foo" returns ("foo", false) +// - "" returns ("", false) +// The function splits the path by forward slashes and returns the first non-empty segment.pkg/remote/fileshare/fsutil/fsutil.go (2)
61-143: Improve error handling inPrefixCopyInternal.The function could benefit from:
- Wrapping errors with more context
- Handling edge cases for empty paths
- Adding debug logging for error cases
Apply this diff to improve error handling:
func PrefixCopyInternal(ctx context.Context, srcConn, destConn *connparse.Connection, c fstype.FileShareClient, opts *wshrpc.FileCopyOpts, listEntriesPrefix func(ctx context.Context, host string, path string) ([]string, error), copyFunc func(ctx context.Context, host string, path string) error) error { + if srcConn == nil || destConn == nil { + return fmt.Errorf("source and destination connections cannot be nil") + } log.Printf("PrefixCopyInternal: %v -> %v", srcConn.GetFullURI(), destConn.GetFullURI()) merge := opts != nil && opts.Merge overwrite := opts != nil && opts.Overwrite if overwrite && merge { - return fmt.Errorf("cannot specify both overwrite and merge") + return fmt.Errorf("invalid options: cannot specify both overwrite and merge") } srcHasSlash := strings.HasSuffix(srcConn.Path, fspath.Separator) srcPath, err := CleanPathPrefix(srcConn.Path) if err != nil { - return fmt.Errorf("error cleaning source path: %w", err) + return fmt.Errorf("failed to clean source path %q: %w", srcConn.Path, err) }
245-303: Improve concurrency handling inReadFileStream.The function could benefit from:
- Using a separate context for cleanup
- Adding timeout for channel operations
- Improving error propagation in goroutines
Apply this diff to improve concurrency handling:
func ReadFileStream(ctx context.Context, readCh <-chan wshrpc.RespOrErrorUnion[wshrpc.FileData], fileInfoCallback func(finfo wshrpc.FileInfo), dirCallback func(entries []*wshrpc.FileInfo) error, fileCallback func(data io.Reader) error) error { + cleanupCtx, cancel := context.WithCancel(context.Background()) + defer cancel() var fileData *wshrpc.FileData firstPk := true isDir := false drain := true defer func() { if drain { - utilfn.DrainChannelSafe(readCh, "ReadFileStream") + go func() { + select { + case <-cleanupCtx.Done(): + return + default: + utilfn.DrainChannelSafe(readCh, "ReadFileStream") + } + }() } }()pkg/remote/fileshare/s3fs/s3fs.go (1)
562-562: Simplify nil check inStatmethod.The
len()check onentries.Contentsimplicitly handles the nil case.Apply this diff to simplify the code:
- if entries.Contents != nil && len(entries.Contents) > 0 { + if len(entries.Contents) > 0 {🧰 Tools
🪛 golangci-lint (1.62.2)
562-562: S1009: should omit nil check; len() for []github.com/aws/aws-sdk-go-v2/service/s3/types.Object is defined as zero
(gosimple)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/remote/fileshare/fspath/fspath.go(1 hunks)pkg/remote/fileshare/fsutil/fsutil.go(1 hunks)pkg/remote/fileshare/s3fs/s3fs.go(2 hunks)pkg/remote/fileshare/wavefs/wavefs.go(14 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/remote/fileshare/s3fs/s3fs.go
562-562: S1009: should omit nil check; len() for []github.com/aws/aws-sdk-go-v2/service/s3/types.Object is defined as zero
(gosimple)
🔇 Additional comments (5)
pkg/remote/fileshare/fspath/fspath.go (1)
1-37: Consider path traversal security implications.The functions don't sanitize paths or handle "../" sequences, which could lead to path traversal vulnerabilities if not properly handled by the caller.
Let's verify if there are any path traversal vulnerabilities in the codebase:
#!/bin/bash # Search for direct usage of these path functions with user input rg -A 5 "fspath\.(Dir|Base|Join|ToSlash)\(.*user.*\)" # Search for potential path traversal patterns rg -A 5 "\.\./|\.\.\\\\|%2e%2e"pkg/remote/fileshare/wavefs/wavefs.go (2)
584-592: LGTM! Clean implementation ofJoinmethod.The implementation correctly:
- Joins path components using
fspath.Join- Cleans the path to prevent traversal
- Updates the connection path
- Returns file info via
Stat
594-599: LGTM! Clear capability declaration.The implementation correctly specifies that the Wave filesystem:
- Supports file appending
- Does not support directory creation
pkg/remote/fileshare/s3fs/s3fs.go (2)
775-792: LGTM! Clean implementation ofJoinmethod.The implementation correctly:
- Handles root paths
- Joins path components using
fspath.Join- Updates the connection path
- Returns file info via
Stat
798-803: LGTM! Clear capability declaration.The implementation correctly specifies that the S3 filesystem:
- Does not support file appending
- Does not support directory creation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/util/utilfn/utilfn.go (2)
1037-1057: Consider adding context and configurable retry parameters.The function implementation is solid, but could be enhanced with:
- A context parameter for cancellation support.
- Configurable retry parameters for different use cases.
Apply this diff to enhance the function:
-func GracefulClose(closer io.Closer, debugName, closerName string) bool { +func GracefulClose(ctx context.Context, closer io.Closer, debugName, closerName string, opts ...GracefulCloseOption) bool { + options := defaultGracefulCloseOptions() + for _, opt := range opts { + opt(options) + } closed := false - for retries := 0; retries < maxRetries; retries++ { + for retries := 0; retries < options.maxRetries; retries++ { + select { + case <-ctx.Done(): + return false + default: + } if err := closer.Close(); err != nil { - log.Printf("%s: error closing %s: %v, trying again in %dms\n", debugName, closerName, err, retryDelay.Milliseconds()) - time.Sleep(retryDelay) + log.Printf("%s: error closing %s: %v, trying again in %dms\n", debugName, closerName, err, options.retryDelay.Milliseconds()) + time.Sleep(options.retryDelay) continue } closed = true break } if !closed { - log.Printf("%s: unable to close %s after %d retries\n", debugName, closerName, maxRetries) + log.Printf("%s: unable to close %s after %d retries\n", debugName, closerName, options.maxRetries) } return closed }Add these types to support configurable options:
type gracefulCloseOptions struct { maxRetries int retryDelay time.Duration } type GracefulCloseOption func(*gracefulCloseOptions) func defaultGracefulCloseOptions() *gracefulCloseOptions { return &gracefulCloseOptions{ maxRetries: maxRetries, retryDelay: retryDelay, } } func WithMaxRetries(maxRetries int) GracefulCloseOption { return func(o *gracefulCloseOptions) { o.maxRetries = maxRetries } } func WithRetryDelay(retryDelay time.Duration) GracefulCloseOption { return func(o *gracefulCloseOptions) { o.retryDelay = retryDelay } }
1059-1076: Add documentation to explain the panic behavior.The function implementation is correct, but could benefit from more detailed documentation explaining the panic behavior and its purpose.
Apply this diff to enhance the documentation:
-// DrainChannelSafe will drain a channel until it is empty or until a timeout is reached. -// WARNING: This function will panic if the channel is not drained within the timeout. +// DrainChannelSafe drains a channel until it is empty or until a timeout is reached. +// +// This function intentionally panics if the channel is not drained within the timeout +// as a fail-fast mechanism to catch channel leaks during development. The panic is +// not meant to be caught or handled, as it indicates a programming error that should +// be fixed. +// +// Parameters: +// - ch: The channel to drain. Must be a receive-only channel. +// - debugName: A string identifier used in the panic message for debugging. +// +// Timeout: 5 seconds +// +// Example usage: +// +// ch := make(chan int) +// // ... channel operations ... +// DrainChannelSafe(ch, "myFunction") func DrainChannelSafe[T any](ch <-chan T, debugName string) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/app/block/blockframe.tsx(1 hunks)frontend/app/store/global.ts(1 hunks)frontend/app/view/preview/preview.tsx(11 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/remote/connparse/connparse.go(3 hunks)pkg/util/utilfn/utilfn.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/app/store/global.ts
- frontend/types/gotypes.d.ts
- pkg/remote/connparse/connparse.go
- frontend/app/block/blockframe.tsx
🧰 Additional context used
🧠 Learnings (2)
frontend/app/view/preview/preview.tsx (1)
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1896
File: frontend/app/view/preview/preview.tsx:1229-1241
Timestamp: 2025-02-12T00:23:25.141Z
Learning: In the Wave Terminal application, path validation and security checks should be handled by the backend, not the frontend. Empty paths are valid and should be allowed through the frontend.
pkg/util/utilfn/utilfn.go (1)
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1896
File: pkg/util/utilfn/utilfn.go:1050-1067
Timestamp: 2025-02-13T03:13:44.823Z
Learning: In the waveterm codebase, DrainChannelSafe intentionally panics after a timeout to catch channel leaks early during development. The panic is a deliberate fail-fast mechanism, not an error to be handled. Additionally, receiver channels should never be closed as that's the sender's responsibility.
⏰ 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 (4)
frontend/app/view/preview/preview.tsx (4)
384-385: LGTM! Simplified path handling.The simplification aligns with the backend-driven path validation approach.
404-404: LGTM! Consistent path formatting.Path handling has been standardized across methods using
formatRemoteUri, improving code consistency and maintainability.Also applies to: 426-426, 586-586, 609-609
1228-1238: Consider enhancing S3 path handling.The current implementation works but could be improved:
- The TODO comment suggests potential enhancements needed.
- Consider adopting standardized S3 URI formats:
- S3 Protocol URI (e.g.,
s3://bucket-name/key) for internal operations- Virtual-Hosted Style URL (e.g.,
https://bucket-name.s3.{region}.amazonaws.com/key) for HTTP access
403-403: Replace console.log with structured logging.Consider using a proper logging system for better debugging and production use.
Also applies to: 413-413, 430-430, 436-436, 589-589, 612-612, 623-623, 694-694
This implementation of fileContentAtom seems incorrect but it prevents the code editor from rerendering when the contents change. Because of this, it makes sense to use it in the meantime.
This updates the test behavior to match its description
Prior to this change, copying a directory to a directory that did not already exist resulted in the new directory containing the original one. This updates that so the new directory is the same as the original one but with a different name.
We previously only handle the case of copying the current directory to an existing directory. This adds another case for when the target directory does not exist.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/copytests/cases/test049.sh (2)
1-3: Add shebang directive and improve test description.Add a shebang directive to specify the shell interpreter. Also, consider expanding the test description to include the expected behavior.
+#!/bin/bash # copy the current directory into a non-existing directory +# this tests that wsh file copy creates the target directory # ensure the copy succeeds and the output exists🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
4-17: Consider expanding test coverage.The test case could be more comprehensive by:
- Testing with different file types (symlinks, directories with subdirectories)
- Verifying file permissions and timestamps are preserved
- Testing with special characters in filenames
Would you like me to generate an expanded version of this test case that covers these scenarios?
tests/copytests/cases/test048.sh (2)
1-3: Add shebang directive and improve test description.Add a shebang directive to specify the shell interpreter. Also, consider expanding the test description to include the expected behavior.
+#!/bin/bash # copy the current directory into an existing directory +# this tests that wsh file copy merges with existing directory # ensure the copy succeeds and the output exists🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
4-20: Consider expanding test coverage and clean up formatting.
- Remove extra blank lines at 11 and 15.
- The test case could be more comprehensive by:
- Testing with different file types (symlinks, directories with subdirectories)
- Verifying file permissions and timestamps are preserved
- Testing with special characters in filenames
- Testing that existing files in the target directory are preserved
Would you like me to generate an expanded version of this test case that covers these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/copytests/cases/test048.sh(1 hunks)tests/copytests/cases/test049.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
tests/copytests/cases/test049.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
tests/copytests/cases/test048.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
⏰ 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)
A reworked implementation switched these to use fileInfo.dir to get the parent directory; however, this does not work if the file is itself a directory. The revision uses RemoteFileJoinCommand to get around that, at least for the time being.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app/view/preview/preview.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/app/view/preview/preview.tsx (1)
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1896
File: frontend/app/view/preview/preview.tsx:1229-1241
Timestamp: 2025-02-12T00:23:25.141Z
Learning: In the Wave Terminal application, path validation and security checks should be handled by the backend, not the frontend. Empty paths are valid and should be allowed through the frontend.
⏰ 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 (3)
frontend/app/view/preview/preview.tsx (3)
383-386: LGTM! Simplified path handling.The simplified path handling aligns with the backend-driven validation approach.
403-403: Replace console.log statements with proper logging.Consider using a proper logging system instead of console.log statements.
Also applies to: 413-413, 430-430, 436-436
685-698: LGTM! Improved error handling.The error handling has been improved with better error propagation and user feedback.
| async formatRemoteUri(path: string, get: Getter): Promise<string> { | ||
| const conn = (await get(this.connection)) ?? "local"; | ||
| return `wsh://${conn}/${path}`; | ||
| return formatRemoteUri(path, await get(this.connection)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate implementation of formatRemoteUri.
The class method simply delegates to the standalone function. Consider removing this method and using the standalone formatRemoteUri function directly to avoid code duplication.
Apply this diff to remove the duplicate implementation:
- async formatRemoteUri(path: string, get: Getter): Promise<string> {
- return formatRemoteUri(path, await get(this.connection));
- }Then update the callers to use the standalone function:
-const path = await this.formatRemoteUri(fileName, get);
+const path = formatRemoteUri(fileName, await get(this.connection));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async formatRemoteUri(path: string, get: Getter): Promise<string> { | |
| const conn = (await get(this.connection)) ?? "local"; | |
| return `wsh://${conn}/${path}`; | |
| return formatRemoteUri(path, await get(this.connection)); | |
| } | |
| // Removed duplicate implementation of formatRemoteUri | |
| // (previously at lines 865-867) | |
| // async formatRemoteUri(path: string, get: Getter): Promise<string> { | |
| // return formatRemoteUri(path, await get(this.connection)); | |
| // } | |
| // Example caller update: | |
| const path = formatRemoteUri(fileName, await get(this.connection)); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
pkg/wshrpc/wshrpctypes.go (9)
54-72: Consider generating these constants directly from the interface.Maintaining manually declared command constants can become cumbersome and error-prone if not kept in sync with the interface. Since you already have a TODO at line 52, consider implementing or utilizing a code-generation step to ensure these symbolic constants remain consistent.
73-83: Add brief documentation for new file-related command constants.These newly added constants would benefit from short explanatory comments to clarify their roles (e.g., “read stream from file,” “fetch file sharing capability,” etc.). This helps other developers quickly grasp each command’s purpose.
176-176: Add doc comment for FileReadStreamCommand.The new method handles a streaming read operation. Documenting the expected lifetime and usage pattern of the returned channel (e.g., whether it must be consumed fully) would make its behavior clearer to consumers.
182-182: Add doc comment for FileJoinCommand.Briefly clarify its purpose to indicate whether it joins multiple files into one, or simply concatenates paths, etc. This fosters clearer usage and maintenance.
185-185: Add doc comment for FileShareCapabilityCommand.A short description explaining how clients can use the returned capabilities (e.g., to decide whether to enable “append” or “mkdir” features during file operations) makes the interface more self-documenting.
218-218: Consider brief documentation for ConnListAWSCommand.Adding a quick note on how this differs from ConnListCommand (e.g., querying AWS connections) clarifies its intent.
230-230: Consider clarifying RemoteFileCopyCommand usage.A concise doc string explaining whether it differs from FileCopyCommand (e.g., copying across remote boundaries) would prevent confusion.
234-234: Consider clarifying RemoteFileMoveCommand usage.A concise doc string explaining how this differs from FileMoveCommand (e.g., applies to remote context) would be helpful.
548-549: Clarify usage conditions for new fields in FileCopyOpts.Since “Recursive” is only used for move but is always true for copy (and “Merge” is only for copy), consider adding doc comments or renaming these fields to reflect their conditional use. This helps prevent confusion in future expansions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/app/block/blockframe.tsx(1 hunks)frontend/app/store/global.ts(1 hunks)frontend/app/store/wshclientapi.ts(5 hunks)frontend/app/view/preview/directorypreview.tsx(3 hunks)frontend/app/view/preview/preview.tsx(7 hunks)pkg/wshrpc/wshclient/wshclient.go(5 hunks)pkg/wshrpc/wshrpctypes.go(7 hunks)pkg/wshrpc/wshserver/wshserver.go(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/app/store/global.ts
- frontend/app/block/blockframe.tsx
- pkg/wshrpc/wshclient/wshclient.go
🧰 Additional context used
🧠 Learnings (1)
frontend/app/view/preview/preview.tsx (1)
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1896
File: frontend/app/view/preview/preview.tsx:1229-1241
Timestamp: 2025-02-12T00:23:25.141Z
Learning: In the Wave Terminal application, path validation and security checks should be handled by the backend, not the frontend. Empty paths are valid and should be allowed through the frontend.
⏰ 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 (15)
frontend/app/store/wshclientapi.ts (4)
55-58: LGTM! Clean implementation of AWS connection listing.The method follows the established RPC command pattern and integrates well with the existing codebase.
190-193: LGTM! Clean implementation of file path joining.The method follows the established RPC command pattern and correctly handles path joining.
220-223: LGTM! Well-implemented file streaming command.The method correctly uses
wshRpcStreamfor streaming file data and maintains proper typing.
225-228: LGTM! Clean implementation of file share capability check.The method follows the established RPC command pattern and provides essential capability checking.
pkg/wshrpc/wshserver/wshserver.go (4)
388-390: LGTM! Clean implementation of file streaming.The method correctly delegates to
fileshare.ReadStreamand maintains proper channel typing.
433-441: LGTM! Robust implementation of file path joining.The method handles edge cases gracefully and provides appropriate fallbacks for single paths.
443-445: LGTM! Clean implementation of file share capability check.The method correctly delegates to
fileshare.GetCapabilityfor capability checking.
573-581:Details
✅ Verification successful
Verify AWS connection handling.
The implementation checks AWS profiles correctly, but the TODO comment suggests future improvements for proper WSH connections via AWS.
🏁 Script executed:
#!/bin/bash # Description: Check for AWS profile handling in other parts of the codebase. # Search for AWS profile handling patterns rg -A 5 'aws:' | grep -v 'TODO' # Search for WSH connection handling with AWS ast-grep --pattern 'if strings.HasPrefix($_, "aws:") { $$$ }'Length of output: 5632
AWS connection handling validated.
The code correctly checks for AWS profiles using
awsconn.ParseProfiles()and returns early when a matching profile is found, consistent with similar patterns used elsewhere in the repository. The existing TODO remains a reminder for future enhancements when integrating native WSH connections via AWS.frontend/app/view/preview/directorypreview.tsx (1)
531-535: LGTM! Improved file info retrieval.The change to use
FileInfoCommandwith formatted remote URI is more direct and efficient than the previous implementation.frontend/app/view/preview/preview.tsx (2)
389-389: LGTM! Simplified path handling.The change correctly delegates path validation to the backend, aligning with the established pattern.
1304-1314: LGTM! Well-implemented remote URI formatting.The function correctly handles both AWS S3 and WSH URIs, with the intentional AWS profile prefix for S3 paths.
pkg/wshrpc/wshrpctypes.go (4)
121-121: No concerns noted; looks consistent.
168-168: No functional changes detected.
773-773: No functional changes detected.
774-780: Nice job adding struct documentation and JSON tags.This addresses the past suggestion to annotate the FileShareCapability struct with doc comments and JSON tags. It is now clear and self-descriptive.
This reverts commit 477f6e5.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/wshrpc/wshremote/wshremote.go (2)
240-240: Useimpl.Loginstead of directlog.Printf.For consistency, use the
impl.Logmethod instead of directlog.Printfcalls. This ensures logging follows the configured logging strategy.-log.Printf("RemoteTarStreamCommand: path=%s\n", path) +impl.Log("RemoteTarStreamCommand: path=%s\n", path) -log.Printf("RemoteTarStreamCommand: starting\n") +impl.Log("RemoteTarStreamCommand: starting\n") -log.Printf("RemoteTarStreamCommand: done\n") +impl.Log("RemoteTarStreamCommand: done\n") -log.Printf("RemoteTarStreamCommand: returning channel\n") +impl.Log("RemoteTarStreamCommand: returning channel\n")Also applies to: 295-295, 305-305, 307-307
704-704: Fix incorrect log message inRemoteFileMoveCommand.The log message uses "RemoteFileCopyCommand" instead of "RemoteFileMoveCommand".
-logPrintfDev("RemoteFileCopyCommand: src=%s, dest=%s\n", data.SrcUri, data.DestUri) +logPrintfDev("RemoteFileMoveCommand: src=%s, dest=%s\n", data.SrcUri, data.DestUri)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app/view/preview/preview.tsx(8 hunks)pkg/wavebase/wavebase.go(0 hunks)pkg/wshrpc/wshremote/wshremote.go(21 hunks)
💤 Files with no reviewable changes (1)
- pkg/wavebase/wavebase.go
🧰 Additional context used
🧠 Learnings (1)
frontend/app/view/preview/preview.tsx (1)
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1896
File: frontend/app/view/preview/preview.tsx:1229-1241
Timestamp: 2025-02-12T00:23:25.141Z
Learning: In the Wave Terminal application, path validation and security checks should be handled by the backend, not the frontend. Empty paths are valid and should be allowed through the frontend.
⏰ 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 (11)
frontend/app/view/preview/preview.tsx (6)
251-251: LGTM! Improved path display for home directory.The change enhances user experience by showing both the tilde and the actual path.
389-389: LGTM! Simplified path normalization.The change aligns with the learning that path validation should be handled by the backend.
407-407: Replace console.log statements with proper logging.Consider using a proper logging system instead of console.log statements for better debugging and production readiness.
Also applies to: 430-430, 434-434, 436-436
719-726: LGTM! Improved clipboard path handling.The change correctly handles both remote and local paths when copying to clipboard:
- For remote paths: Uses
formatRemoteUrito format the path- For local paths: Uses the raw file path
1304-1314: LGTM! Extracted formatRemoteUri function.The function has been extracted and made reusable. The implementation:
- Handles null/undefined connection by defaulting to "local"
- Correctly prefixes S3 paths with AWS profile as per the learnings
- Maintains consistent URI format for both S3 and WSH paths
Note: The TODO comment about S3 paths can be safely kept as it might be addressing future improvements beyond the current implementation.
1316-1316: LGTM! Added formatRemoteUri to exports.The function is now properly exported for reuse in other components.
pkg/wshrpc/wshremote/wshremote.go (5)
21-21: LGTM! Standardized timeout handling.Good improvement to centralize the timeout constant in the
fstypepackage.
146-146: LGTM! Improved file descriptor cleanup.Excellent improvement to resource management by:
- Using
utilfn.GracefulCloseconsistently across all file operations- Adding context (function name and path) for better debugging
- Ensuring proper cleanup of file descriptors
Also applies to: 288-288, 413-413, 449-449, 462-462, 620-620, 629-629, 821-821
240-242: LGTM! Improved path handling in tar streaming.Good improvements to handle trailing slashes and differentiate between single files and directories.
Also applies to: 253-258
334-346: LGTM! Improved destination path handling.Good improvements to handle destination path existence and overwrite options with clear error messages.
741-743: LGTM! Added directory operation safety check.Good addition of the recursive flag check for directory move operations.
Adds the S3 `fileshare` implementation This also updates `wsh file cp` so it behaves more like `cp` for things like copying directories and directory entries. It's not meant to align with `cp` on everything, though. Our `wsh cp` will be recursive and will create intermediate directories by default. This also adds new aliases for `wsh view`: `wsh preview` and `wsh open` --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: sawka <mike@commandline.dev> Co-authored-by: Sylvia Crowe <software@oneirocosm.com>
3 participants
Adds the S3
fileshareimplementationThis also updates
wsh file cpso it behaves more likecpfor things like copying directories and directory entries. It's not meant to align withcpon everything, though. Ourwsh cpwill be recursive and will create intermediate directories by default.This also adds new aliases for
wsh view:wsh previewandwsh open