Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d08f0e183
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| }) | ||
| ); | ||
| try { | ||
| await s3Client.deleteObject({ s3Uri: sourceS3Uri }); |
There was a problem hiding this comment.
When an unversioned source is replaced after copyObject succeeds but before this deletion, the unconditional delete removes the newer object even though the destination contains the older contents. The adapter already snapshots the source ETag/version for copying, so move/rename should carry that identity through and conditionally delete that same object rather than deleting whatever is current.
Useful? React with 👍 / 👎.
| const objects = [...result.objects]; | ||
| for (const prefix of result.prefixes) { | ||
| for (const object of await crawl(prefix)) { | ||
| objects.push(object); | ||
| } |
There was a problem hiding this comment.
For a folder represented by an object whose key ends in the delimiter, s3Client.listObjects explicitly filters that object out at lines 380-387, so this inventory never copies or deletes it. Moving a nonempty marker-backed folder therefore leaves the source folder behind, while copying, moving, or renaming an empty marker-only folder reports success without creating the destination at all.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1554084ed9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| id: "move-object", | ||
| label: t("move to"), | ||
| icon: getIconUrlByName("DriveFileMove"), | ||
| onClick: () => onMove(currentS3Uri) |
There was a problem hiding this comment.
When the current-directory menu is opened while a large upload into that directory is still running, these move/rename callbacks remain enabled, unlike row actions guarded by getIsItemActionAvailable. The operation inventories only currently visible S3 objects, so an in-progress multipart upload is omitted; after the move or rename reports success, that upload can complete under the old prefix, leaving the operation incomplete and recreating the source directory. Pass the current prefix's upload-busy state into this menu and disable move/rename until those uploads finish.
Useful? React with 👍 / 👎.
| const result = await s3Client.copyObject({ | ||
| sourceS3Uri, | ||
| destinationS3Uri, | ||
| evtCancel, |
There was a problem hiding this comment.
The destination conflict decision is based on listings completed before all prompts and sequential copies, but this call carries no expected destination state. If another client creates an initially absent target, or replaces a target after confirmation, the copy silently overwrites data that was never inspected or approved. Carry the observed absence/ETag into the adapter and use destination If-None-Match/If-Match conditions for both single and multipart completion; S3 supports these destination preconditions for copy writes in the CopyObject API.
Useful? React with 👍 / 👎.
| if (result.status === "canceled" || evtCancel.postCount !== 0) { | ||
| dispatch(actions.commandLogCancelled({ cmdId })); | ||
| if (evtCancel.postCount === 0) { | ||
| dispatch(actions.fileOperationCanceled({ operationId })); | ||
| } |
There was a problem hiding this comment.
When cancellation arrives after an unabortable copy commit has started, the adapter deliberately waits and can return success, but this condition still takes the canceled branch solely because evtCancel was posted. The destination then exists while progress and command logs report cancellation; for a move, the source is retained without recording that the copy completed. Handle a successful adapter result before the cancellation flag and record the committed copy before stopping the remaining work.
Useful? React with 👍 / 👎.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f669309e6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| MetadataDirective: "COPY", | ||
| TaggingDirective: "COPY", | ||
| WebsiteRedirectLocation: head.WebsiteRedirectLocation |
There was a problem hiding this comment.
For any source using a non-STANDARD storage class, this request omits head.StorageClass, and the multipart path omits it from CreateMultipartUpload as well. AWS documents that an omitted storage class defaults to STANDARD for both CopyObject and CreateMultipartUpload. Copying—and especially moving—an IA, Intelligent-Tiering, or archive object therefore silently changes its storage characteristics and potentially its cost; pass the source storage class to both write paths.
Useful? React with 👍 / 👎.
| onMove === undefined || | ||
| !selectedItems.every(getIsItemActionAvailable) |
There was a problem hiding this comment.
When a move or rename is already running, its source remains in the listing and still satisfies getIsItemActionAvailable, because listed items carry only upload/deletion state and are not correlated with fileOperations. A user can therefore start a second move of the same source while the first is awaiting confirmation or copying; if both copies commit before deletion, S3's idempotent deletes let both operations report success and leave copies in two destinations. Disable destructive actions for objects and prefixes covered by an ongoing move or rename.
Useful? React with 👍 / 👎.
No description provided.