-
Notifications
You must be signed in to change notification settings - Fork 3
feat: upload folders on drag and drop #34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
gbirman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterchinman can you add the data transfer logic we have in the filedrop directive into filefolder drop (see what i added here https://github.com/macro-inc/app-monorepo/pull/6430)
gbirman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please address and re-request review
js/app/packages/core/component/LexicalMarkdown/plugins/file-paste/filePastePlugin.ts
Outdated
Show resolved
Hide resolved
gbirman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
almost there
39f9982 to
96b52a4
Compare
6ca0007 to
d484247
Compare
3c7a8f6 to
2d7c9e4
Compare
2d7c9e4 to
7261dbb
Compare
|
@peterchinman should we close this ? |
no its just a WIP, i think @peterchinman will address the comments i left |
| ) { | ||
| const onFilesReady = (uploadEntries: UploadInput[]) => { | ||
| handleFileUpload(uploadEntries, props.inputAttachments, (_attachment) => { | ||
| props.onChange(markdownState()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it ok to do this after every file upload vs. at the very end like we had before? assume its not a problem and makes sense to me if the markdown state is indeed changing
| }); | ||
| use:fileFolderDrop={{ | ||
| onDrop: (files, folders) => { | ||
| setIsDraggingOverChannel(false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i moved this setter out before the file upload whereas before we had it after upload, figure that's fine/correct behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes seems fine
| } | ||
| return false; | ||
|
|
||
| props.onPasteFilesAndDirs(fileEntries, directoryEntries); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterchinman you had code that took directories only to prevent "duplicate phantom files". what does that mean? i got rid of it as i think the api makes more sense this way but i'd like to understand what you meant so i dont accidentally introduce bugs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was a bug, I think the way it worked was that, you would upload a folder and the files within it would get uploaded twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to test if it's working, but I can't get any folder uploads to work anywhere... websocket issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok let's circle back to this once the backend perms are fixed:
User: arn:aws:sts::569036502058:assumed-role/upload_extractor_lambda_handler-role-dev/upload_extractor_lambda_handler-dev is not authorized to perform: s3:GetObject on resource: \"arn:aws:s3:::bulk-upload-staging-dev/extract/164cb2f8-861c-4c16-b4db-0a5a66091bf7\" because no identity-based policy allows the s3:GetObject action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I fixed, @peterchinman can you test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok yes bug still exists. the way to trigger it is to make a selection that includes both an open folder and a sibling file.
CleanShot.2025-12-03.at.15.00.09.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can take a look when I'm back
gbirman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterchinman so you're not blocked for this entire workflow I'll approve this under the condition that you assign me a ticket for the duplicate upload bug which can be addressed separately
Co-authored-by: Gabriel Birman <25272206+gbirman@users.noreply.github.com>
Co-authored-by: Gabriel Birman <25272206+gbirman@users.noreply.github.com>
Co-authored-by: Gabriel Birman <25272206+gbirman@users.noreply.github.com>
Co-authored-by: Gabriel Birman <25272206+gbirman@users.noreply.github.com>
Summary