Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. |
The explorer accepts a drop of files from the desktop, but nothing can be dragged out of it. Anyone who wants an object's URI somewhere else — a notebook cell, a chat box, a form field, an application embedding Onyxia — has to open the row menu, copy the URI, and paste it. Rows are now draggable. A drag publishes the S3 URIs twice: as `text/plain`, newline separated, which is what makes a drop onto an ordinary text field useful without the target knowing anything about Onyxia; and as `application/x-onyxia-s3-objects`, JSON, for a consumer that needs to tell "these are S3 objects" from "the user dropped some text". Nothing is read and nothing is modified — only the URIs travel. Three details that are easy to get wrong, so they are pure functions with tests rather than logic inside the component: - Which rows a drag carries is not "the selection". Dragging an unselected row drags that row alone; returning the selection there would carry files the user cannot see they had selected, since this list is virtualized. - Rows mid-upload or mid-delete are not draggable, and are dropped from a multi-row drag rather than refusing the whole gesture. - Detection during dragover reads `types` and never `getData()`. The DataTransfer is in protected mode while a drag is in flight, so every value reads back empty — a check written against `getData()` passes its unit test and refuses every real drag. A drag beginning on the row checkbox or a row action button is that control's gesture and does not become a row drag. Signed-off-by: Islam Eldemery <islam.eldemery@gmail.com>
…efix The explorer can list, upload, download, delete and share, but it cannot copy. There is no copy, no move and no rename anywhere in the app, and `S3Client` had no method for it — so getting a file from one prefix to another means downloading it and uploading it again, through the browser, at the size of the object. Objects and prefixes dragged out of an explorer can now be dropped onto a prefix row, or onto the surface to land in the listed prefix. It is CopyObject, server side: no bytes reach the browser, so a 40 GB object costs the client one request. Most of what makes this feel like a file manager is key arithmetic, so it is in a pure module with tests rather than inside the thunk: - Dragging a folder keeps its name and its internal shape. The part of the key below the dragged item's parent is what gets rebuilt under the destination — taking the basename instead would flatten every object side by side and let same-named files in different folders overwrite each other. - A prefix dropped into itself or into its own descendant is refused: the crawl would keep finding the objects it had just written. The check is on path segments, so `reports/` may still be copied into the sibling `reports-archive/`, which a string-prefix check would forbid. - Dropping something back where it already is is refused rather than performed; on a versioned bucket it would be a new version for nothing. `CopySource` is encoded per segment, which the AWS SDK does not do for this parameter — it is a header value, not a path parameter. An unencoded key containing a space, a `+` or a `#` either fails the request or copies a different object than the one asked for. Deliberately not here: - MOVE. It would be copy-then-delete with no transaction and no undo, so a partial failure loses data. It deserves its own design rather than riding in on a drag gesture. - Objects above 5 GiB, where S3 requires a multipart copy. The single-part limit is stated on the port so a caller keeps it rather than discovers it. An object row is not a destination: dragging across one on the way to the background falls through to the listed prefix rather than refusing the drop. The destination highlight is an outline rather than a fill, because the row beneath may already be selected or striped. A drag of files from outside the browser remains an upload and stays visually distinct — different gesture, different outcome. Depends on the row-drag change that publishes what is being dragged. Signed-off-by: Islam Eldemery <islam.eldemery@gmail.com>
01434f8 to
81f10ff
Compare
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }) => { | ||
| const { sourceS3Uri, destinationS3Uri } = params; | ||
|
|
||
| const cmdId = Date.now() + Math.random(); |
![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: 01434f82b2
ℹ️ 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".
| }) | ||
| ); | ||
|
|
||
| await s3Client.copyObject({ sourceS3Uri, destinationS3Uri }); |
There was a problem hiding this comment.
When the computed destination key already exists—particularly when copying a folder into an existing folder—CopyObject replaces that object immediately. Unlike the upload flow, this path performs no existence check or overwrite confirmation, so copying can silently destroy data in an unversioned destination bucket; check each destination and obtain confirmation before issuing the copy.
Useful? React with 👍 / 👎.
| await awsS3Client.send( | ||
| new (await import("@aws-sdk/client-s3")).CopyObjectCommand({ |
There was a problem hiding this comment.
For any source larger than 5 GiB, or a dragged prefix containing such an object, this unconditional single CopyObjectCommand is rejected by S3. Because sibling copies run concurrently, smaller objects may already have been copied before the operation rejects, leaving a partially copied tree; either implement multipart copy or reject oversized sources before starting the batch.
Useful? React with 👍 / 👎.
| return item !== undefined && item.type === "prefix segment" | ||
| ? item.s3Uri | ||
| : undefined; |
There was a problem hiding this comment.
When a prefix row has isDeleting === true, this function still returns it as a valid copy destination. A user can therefore drop objects onto a folder while its delete thunk is crawling and removing keys, causing the new copies either to be deleted by that crawl or to survive unpredictably depending on timing; exclude deleting prefix items from drop targets.
Useful? React with 👍 / 👎.
| return [ | ||
| ...result.objects.map(({ s3Uri }) => s3Uri), | ||
| ...( | ||
| await Promise.all( | ||
| result.prefixes.map(s3Uri => crawl({ s3UriPrefix: s3Uri })) | ||
| ) | ||
| ).flat() | ||
| ]; |
There was a problem hiding this comment.
When a dragged prefix is represented only by a zero-byte key ending in the delimiter, or contains nested empty prefixes represented that way, this crawl returns no objects to copy because S3Client.listObjects deliberately filters delimiter-terminated keys. The operation then reports success but creates no destination folder, and nested empty folders disappear from otherwise nonempty copies; include those marker objects or explicitly create equivalent destination markers.
Useful? React with 👍 / 👎.
| if (getIsEventFromInteractiveRowElement(event)) { | ||
| event.preventDefault(); | ||
| return; |
There was a problem hiding this comment.
When the user starts dragging on the displayed filename or folder name, this generic interactive-element check cancels the drag because that label is rendered as a <button>. The name is the most natural part of a row to grab, and the component specification only excludes checkboxes and row action buttons, so restrict this guard to those controls rather than making drags work only from the icon or unused row space.
Useful? React with 👍 / 👎.
3 participants
Just to test #1090