Fix move & copy for prefix filesystems #1998
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 16 minutes and 35 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 (14)
WalkthroughThis pull request introduces multiple updates across the codebase. In the frontend, the function signature for renaming items now includes an additional boolean flag to indicate if the item is a directory. This change affects the URI construction and context menu actions. In the 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: 2
🧹 Nitpick comments (7)
pkg/remote/fileshare/fsutil/fsutil.go (5)
61-62: Return value mismatch on directory listing failure
When directory listing fails, the code returns(false, error)even thoughsrcInfo.IsDir == true. While this likely doesn’t impact error handling, consider using(true, error)to be consistent.
90-91: Clarify role of destPathPrefix
A comment explains this prefix for a potential directory destination. Ensure the prefix usage is consistent with actual directory semantics.
95-95: Appending trailing slash to destPathPrefix
Adding a slash is appropriate when interpreting the destination as a directory. Watch out for potential double slashes if the prefix already ends with one.
113-115: Single-file path adjustment
IfsingleFileis true,fileNamebecomes the exactdestPath. This ensures a correct final name but double-check for trailing slash scenarios.
130-183: Comprehensive path determination logic
This function centralizes checks for merge, overwrite, and destination existence. Highlights include:
- Immediate error if both
mergeandoverwriteare set.- Recomputing destination path if the source is a directory or if an existing directory is detected.
- Deleting an existing destination when
overwriteis true.All these steps are aligned with the updated copy semantics, though you might consider splitting the logic into smaller helpers for readability.
pkg/remote/fileshare/s3fs/s3fs.go (2)
210-210: Informative log statement for tar path prefix
Logging the scenarios wheresingleFileorincludeDirtoggles the tar path prefix is useful. Consider adopting a structured logger for clarity and filtering.
675-675: Delegating to fsutil.PrefixCopyRemote
ReusingPrefixCopyRemotecentralizes copy logic. Verify any S3-specific constraints (like SSE encryption or region handling) are addressed before delegation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
frontend/app/store/wshclientapi.ts(1 hunks)frontend/app/view/preview/directorypreview.tsx(5 hunks)go.mod(1 hunks)pkg/remote/fileshare/fileshare.go(2 hunks)pkg/remote/fileshare/fstype/fstype.go(1 hunks)pkg/remote/fileshare/fsutil/fsutil.go(7 hunks)pkg/remote/fileshare/pathtree/pathtree.go(3 hunks)pkg/remote/fileshare/s3fs/s3fs.go(5 hunks)pkg/remote/fileshare/wavefs/wavefs.go(2 hunks)pkg/remote/fileshare/wshfs/wshfs.go(1 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshremote/wshremote.go(9 hunks)pkg/wshrpc/wshrpctypes.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
pkg/remote/fileshare/wavefs/wavefs.go (1)
Learnt from: esimkowitz
PR: wavetermdev/waveterm#1725
File: pkg/remote/fileshare/wavefs/wavefs.go:441-494
Timestamp: 2025-01-29T04:21:11.649Z
Learning: The `CopyRemote` function in WaveTerm's file operations has proper error handling that prevents partial writes by failing fast and using context cancellation. Each step (path cleaning, file operations, tar reading/writing) is guarded by error checks that prevent proceeding with writes on error.
⏰ 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 (36)
pkg/remote/fileshare/fsutil/fsutil.go (10)
31-31: Clarify root parent path behavior
Returning an empty string for the root path might cause confusion when building parent paths. Confirm that your logic handles top-level paths correctly in all use cases.
43-43: Edge case handling for slash in path
Consider verifying the behavior whenhostAndPathcontains only a leading slash. IflastSlashis 0, slicing may yield an empty path, which could be unintended.
46-46: New return signature for PrefixCopyInternal
The function now returns a boolean flagging whether the source is a directory. This follows the updated contract, so ensure all callers properly handle the additional return.
49-52: Check error return path in DetermineCopyDestPath usage
If there's an error fromDetermineCopyDestPath, the function emits(false, err). Verify that the(false, ...)part won’t cause downstream confusion when the source is actually a directory.
76-77: Properly indicating directory copy
Returning(true, ...)here correctly conveys that the source being copied is a directory. Good job matching the new interface requirements.
85-86: Return value for single-file copy
Returning(false, copyFunc(...))clearly denotes a non-directory source. Just confirm thatcopyFuncapplies any necessary overwrite or merge logic.
89-89: PrefixCopyRemote now returns (bool, error)
Returning a boolean to indicate directory status aligns with the updated interface.
93-93: Error handling for invalid destination path
Skipping the copy with(false, fmt.Errorf(...))is a clear approach. Confirm logs or user feedback reflect that the path was invalid.
97-99: Bubbling up DetermineCopyDestPath error
Returning(false, err)is consistent with the contract. Make sure the caller can distinguish between errors versus a non-directory response if needed.
125-128: Final return clarifies directory status
The code returns(false, err)on failure and(srcInfo.IsDir, nil)otherwise, precisely reflecting the new interface requirement.pkg/remote/fileshare/s3fs/s3fs.go (6)
293-293: Aggregated error handling
Usingerrors.Join(errs...)consolidates multiple concurrent errors for better debugging. This is a solid approach to surfacing parallel fetch issues.
648-653: MoveInternal: capturing directory status
ObtainingisDirfromCopyInternalis necessary to correctly invokeDelete(..., isDir). Ensure you don’t attempt a delete when the copy fails.
656-656: CopyRemote signature updated to (bool, error)
The boolean return clarifies directory status in line with the revised interface. Confirm callers either use or safely ignore the returned boolean.
662-662: Destination validation in CopyRemote
Short-circuiting with(false, fmt.Errorf(...))is consistent. Just ensure user-facing layers or logs clarify that the required bucket field was blank.
679-679: S3Client CopyInternal now returns (bool, error)
This supports indicating source directory status. Consider partial-copy scenarios, ensuring you handle them gracefully.
756-779: New listFilesPrefix pagination logic
TheListObjectsV2Paginatorusage handles large S3 prefixes well. Make sure:
- The loop terminates properly if
fileCallbackreturnsfalse.- Errors for missing or inaccessible buckets are propagated.
- Partial listings and concurrency are considered in the overall flow.
Overall, a strong approach for fetching multiple pages.
pkg/remote/fileshare/fstype/fstype.go (1)
43-46: Interface updates: CopyInternal and CopyRemote now return directory status
Exposing a boolean to indicate if the source was a directory helps higher-level logic (e.g., subsequent delete operations). Great consistency with the rest of the PR.pkg/remote/fileshare/pathtree/pathtree.go (4)
38-38: LGTM! Helpful warning message.The added log statement provides clear visibility into path formatting issues.
64-64: LGTM! Useful debug information.The log statement helps track path processing by showing the calculated relative path.
73-73: LGTM! Good duplicate path logging.The log statement helps identify when paths are skipped due to already being in the tree.
81-81: LGTM! Comprehensive path processing logging.The added log statements provide excellent visibility into the path processing workflow:
- Invalid path components are logged for debugging
- Path additions to existing parents are tracked
- New path additions are recorded
Also applies to: 88-88, 92-92
pkg/remote/fileshare/wshfs/wshfs.go (1)
117-119: LGTM! Enhanced return types for better operation feedback.The methods now return a boolean indicating whether the source is a directory, which improves the handling of file operations by providing explicit information about the source type.
Also applies to: 121-130
pkg/remote/fileshare/fileshare.go (2)
132-136: LGTM! Improved directory handling in Move operation.The code now correctly uses the source's directory status to determine whether to perform a recursive deletion, making the move operation more robust.
153-158: LGTM! Consistent error handling in Copy operation.The code properly handles the enhanced return types from copy operations while maintaining clean error propagation.
frontend/app/store/wshclientapi.ts (1)
296-296: LGTM! Updated return type to match backend changes.The RemoteFileCopyCommand's return type now correctly reflects the enhanced feedback from the backend regarding the source's directory status.
pkg/remote/fileshare/wavefs/wavefs.go (3)
421-433: LGTM! Clean implementation of the move operation.The changes correctly propagate the directory status from
CopyInternaltoDelete, maintaining proper error handling throughout the operation.
435-468: LGTM! Well-structured implementation of the copy operation.The function properly implements the new return type and maintains robust error handling through the
fsutil.PrefixCopyInternalhelper.
470-477: LGTM! Proper error handling in the remote copy operation.The error handling is consistent with the retrieved learning about preventing partial writes and using proper error checks.
pkg/wshrpc/wshclient/wshclient.go (1)
359-362: LGTM! Clean implementation of the updated RPC client method.The changes correctly implement the new return type while maintaining the existing RPC helper pattern.
pkg/wshrpc/wshremote/wshremote.go (3)
309-322: LGTM! Proper initialization and error handling.The function maintains robust error handling while adding the new boolean return value.
420-432: LGTM! Clean implementation of directory status tracking.The
srcIsDirflag is properly initialized and updated based on the source file's status.
493-494: LGTM! Proper directory status handling in tar operations.The
srcIsDirflag is correctly set based on thesingleFileparameter from the tar operation.pkg/wshrpc/wshrpctypes.go (1)
231-231: LGTM! Clean interface update.The interface method signature has been properly updated to include the boolean return value.
frontend/app/view/preview/directorypreview.tsx (3)
294-310: LGTM! Proper handling of directory paths.The implementation correctly appends a trailing slash for directories, which is essential for prefix filesystems to properly handle directory moves.
861-861: LGTM! Improved type safety.The explicit boolean type declaration for
isDirenhances code clarity and type safety.
51-51:❓ Verification inconclusive
LGTM! Verify implementations of the updated interface.
The addition of the
isDirparameter helps distinguish between files and directories, which is crucial for proper path handling.Run the following script to verify all implementations of the
updateNamemethod:
🏁 Script executed:
#!/bin/bash # Description: Find all implementations of the updateName method to ensure they've been updated with the new signature. rg -A 5 'updateName.*\(.*\).*{' --type ts --type tsxLength of output: 160
Revised Verification Required – Please Re-run the Updated Check
The change to include the
isDirparameter is correct and essential for distinguishing file types. However, the initial verification script produced a regex error. Please verify all implementations ofupdateNamewith the revised script below:#!/bin/bash # Revised regex search: Locate all updateName implementations using the updated signature. rg "updateName\s*:\s*\(.*\)\s*=>" --type ts --type tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
cmd/wsh/cmd/wshcmd-file.go (1)
106-108:⚠️ Potential issueRemove unused recursive flag from fileMvCmd.
The recursive flag is defined but not used in the implementation. According to the changes, recursive functionality has been removed from the move operation.
Apply this diff to remove the unused flag:
-fileMvCmd.Flags().BoolP("recursive", "r", false, "move directories recursively") fileMvCmd.Flags().BoolP("force", "f", false, "force overwrite of existing files")
🧹 Nitpick comments (5)
pkg/wshrpc/wshremote/wshremote.go (5)
420-421: Consider initializingsrcIsDircloser to its first use.The
srcIsDirvariable is initialized at line 420 but not used until line 493. Consider moving the initialization closer to its first usage to improve code readability and maintain variable scope closer to its usage.- srcIsDir := false - if srcConn.Host == destConn.Host { srcPathCleaned := filepath.Clean(wavebase.ExpandHomeDirSafe(srcConn.Path))Also applies to: 493-493
476-479: Consider extracting timeout calculation to a helper function.The timeout calculation logic appears in multiple places in the codebase. Consider extracting it to a helper function to improve code reusability and maintainability.
+func getTimeout(opts *wshrpc.FileCopyOpts) time.Duration { + if opts != nil && opts.Timeout > 0 { + return time.Duration(opts.Timeout) * time.Millisecond + } + return fstype.DefaultTimeout +} + func (impl *ServerImpl) RemoteFileCopyCommand(ctx context.Context, data wshrpc.CommandFileCopyData) (bool, error) { // ... - timeout := fstype.DefaultTimeout - if opts.Timeout > 0 { - timeout = time.Duration(opts.Timeout) * time.Millisecond - } + timeout := getTimeout(opts)
480-482: Consider using a defer for the timeout cancellation immediately after creation.The timeout cancellation should be deferred right after the context creation to ensure proper cleanup.
readCtx, cancel := context.WithCancelCause(ctx) readCtx, timeoutCancel := context.WithTimeoutCause(readCtx, timeout, fmt.Errorf("timeout copying file %q to %q", srcUri, destUri)) - defer timeoutCancel() + defer func() { + timeoutCancel() + cancel() + }() - defer cancel()
489-505: Consider extracting the file copy callback to a separate method.The anonymous function passed to
TarCopyDestis complex enough to warrant its own method. This would improve readability and testability.+func (impl *ServerImpl) handleTarCopyFile(destPathCleaned string, destHasSlash bool) func(*tar.Header, *tar.Reader, bool) error { + return func(next *tar.Header, reader *tar.Reader, singleFile bool) error { + numFiles++ + nextpath := filepath.Join(destPathCleaned, next.Name) + log.Printf("RemoteFileCopyCommand: copying %q to %q\n", next.Name, nextpath) + srcIsDir = !singleFile + if singleFile && !destHasSlash { + nextpath = destPathCleaned + } + finfo := next.FileInfo() + n, err := copyFileFunc(nextpath, finfo, reader) + if err != nil { + return fmt.Errorf("cannot copy file %q: %w", next.Name, err) + } + totalBytes += n + return nil + } +}
509-515: Consider using structured logging for better observability.The performance metrics logging could benefit from structured logging to make it easier to parse and analyze the logs.
- log.Printf("RemoteFileCopyCommand: done; %d files copied in %.3fs, total of %.4f MB, %.2f MB/s, %d files skipped\n", numFiles, totalTime, totalMegaBytes, rate, numSkipped) + log.Printf("RemoteFileCopyCommand: done; files_copied=%d duration=%.3fs total_mb=%.4f rate_mbps=%.2f files_skipped=%d\n", + numFiles, totalTime, totalMegaBytes, rate, numSkipped)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/wsh/cmd/wshcmd-file.go(1 hunks)frontend/types/gotypes.d.ts(0 hunks)pkg/wshrpc/wshremote/wshremote.go(9 hunks)pkg/wshrpc/wshrpctypes.go(2 hunks)
💤 Files with no reviewable changes (1)
- frontend/types/gotypes.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/wshrpc/wshrpctypes.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (2)
cmd/wsh/cmd/wshcmd-file.go (1)
434-436: LGTM! Clean implementation of the move operation.The changes correctly implement the move operation without recursive functionality while maintaining proper error handling and force flag support.
pkg/wshrpc/wshremote/wshremote.go (1)
309-309: LGTM! Method signature change enhances functionality.The return type change from
errorto(bool, error)is a good improvement, providing explicit information about the source file type.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/remote/fileshare/fsutil/fsutil.go (2)
95-99: Consider renaming variable for clarity.The variable
destPathPrefixcould be renamed todestDirPathto better reflect its purpose as the path when copying to a directory.
134-185: Consider improving error messages and breaking down the function.While the implementation is solid, consider these improvements:
- Make error messages more descriptive, e.g., include the conflicting options' values.
- Break out the destination path determination logic (lines 159-171) into a separate function for better maintainability.
Otherwise, great job centralizing the path determination logic with comprehensive error handling!
Example error message improvement:
- return "", "", nil, fmt.Errorf("cannot specify both overwrite and merge") + return "", "", nil, fmt.Errorf("conflicting options: cannot specify both overwrite=%v and merge=%v", overwrite, merge)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/app/view/preview/directorypreview.tsx(5 hunks)pkg/remote/fileshare/fileshare.go(3 hunks)pkg/remote/fileshare/fstype/fstype.go(2 hunks)pkg/remote/fileshare/fsutil/fsutil.go(5 hunks)pkg/remote/fileshare/s3fs/s3fs.go(6 hunks)pkg/remote/fileshare/wavefs/wavefs.go(1 hunks)pkg/wshrpc/wshremote/wshremote.go(4 hunks)pkg/wshrpc/wshrpctypes.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkg/wshrpc/wshrpctypes.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/remote/fileshare/fileshare.go
- pkg/remote/fileshare/fstype/fstype.go
- pkg/remote/fileshare/wavefs/wavefs.go
- pkg/remote/fileshare/s3fs/s3fs.go
- pkg/wshrpc/wshremote/wshremote.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 (8)
frontend/app/view/preview/directorypreview.tsx (3)
49-55: LGTM! Interface change improves type safety.The addition of the
isDirparameter toupdateNamemethod signature enhances type safety by explicitly declaring whether the path refers to a directory.
294-331: LGTM! Implementation correctly handles directory URIs.The function now properly handles directory paths by appending a trailing slash to the source URI when moving directories, which is essential for prefix filesystem operations.
861-884: LGTM! Enhanced error handling for copy operations.The error handling has been improved to:
- Distinguish between file and directory copy failures
- Provide appropriate retry options based on the error type
- Show clear error messages to users
pkg/remote/fileshare/fsutil/fsutil.go (5)
29-44: LGTM! Improved path handling consistency.The changes make the function more consistent by returning empty string for both empty input and root path cases.
53-58: LGTM! Improved error handling for directory operations.The addition of the recursive flag check with a specific error message improves the user experience when attempting to copy directories.
188-191: LGTM! Improved empty path handling.The change improves consistency by treating empty path as a valid case returning an empty string.
46-52:✅ Verification successful
Verify function signature.
The AI summary indicates this function should return
(bool, error), but the implementation returns onlyerror. Please verify the correct signature.
🏁 Script executed:
#!/bin/bash # Search for any other implementations or interfaces requiring this function rg "PrefixCopyInternal.*\(" --type goLength of output: 852
Function signature verified and improvements approved.
After reviewing the code and usage inwavefsands3fs, it's clear that the actual implementation ofPrefixCopyInternalreturns only anerror—which is aligned with how the function is invoked elsewhere. The AI summary's mention of a(bool, error)return appears to be outdated or incorrect. The improvements made (including centralized path determination and enhanced error handling) are therefore validated.
- File:
pkg/remote/fileshare/fsutil/fsutil.go(Lines 46-52)- Action: No further changes required regarding the function signature.
93-104:✅ Verification successful
Verify function signature.
The AI summary indicates this function should return
(bool, error), but the implementation returns onlyerror. Please verify the correct signature.
🏁 Script executed:
#!/bin/bash # Search for any other implementations or interfaces requiring this function rg "PrefixCopyRemote.*\(" --type goLength of output: 660
Function Signature Verified & Variable Naming Suggestion
- Verified that the repository consistently uses the signature returning only an
error(as seen infsutil.go,wavefs.go, ands3fs.go). The AI summary suggesting a(bool, error)return is outdated and should be updated.- Consider renaming
destPathPrefixto something more descriptive if it clarifies its role (e.g.,dirPathPrefix).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if !overwrite { | ||
| return 0, fmt.Errorf(fstype.OverwriteCopyError, path) | ||
| } else if finfo.IsDir() { | ||
| err := os.RemoveAll(path) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
To fix the problem, we need to validate the user input before using it to construct a file path. Specifically, we should ensure that the path does not contain any path separators or parent directory references if it is expected to be a single file name. If the input can be a path with multiple components, we should verify that the path is within a specific directory that is considered safe.
In this case, we will implement a validation function to ensure that the path is within a specific safe directory. We will then use this function to validate the path variable before performing any file operations.
Also makes recursive the default for copy, adds better error for move without recursive