Skip to content
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

[UI] File Attachment Added #804

Merged
merged 33 commits into from
Feb 2, 2025
Merged

[UI] File Attachment Added #804

merged 33 commits into from
Feb 2, 2025

Conversation

mulla028
Copy link
Collaborator

@mulla028 mulla028 commented Jan 27, 2025

Closes #794

Description (UPDATED)

This PR improves UX in terms of file management. Previously user was only able to see the file in the PromptForm, but now he is able to use paperclip icon to see all files attached in modal window, and manage them:

  • Attach files... (Same as in OptionsButton)
  • Delete (using removeFile from src/lib/fs.ts)
  • Download (using downloadFile from src/lib/fs.ts)

If user never attached the file(s) paperclip will trigger file attachment process without opening modal window.

Preview (UPDATED)

ScreenRecording2025-01-27at7 36 33PM-ezgif com-video-to-gif-converter

@mulla028 mulla028 requested a review from humphd January 27, 2025 17:16
Copy link

cloudflare-workers-and-pages bot commented Jan 27, 2025

Deploying chatcraft-org with  Cloudflare Pages  Cloudflare Pages

Latest commit: 12653ae
Status: ✅  Deploy successful!
Preview URL: https://e3431ed0.console-overthinker-dev.pages.dev
Branch Preview URL: https://mulla028-file-attachment.console-overthinker-dev.pages.dev

View logs

@mulla028 mulla028 changed the title File Attachment Added [RAW] File Attachment Added Jan 27, 2025
@mulla028
Copy link
Collaborator Author

@humphd @tarasglek opened a draft PR to hear about UI adjustments, also I'm having trouble with + button functionality while adding files. Should I copy the code from OptionsButton for +?

@mulla028 mulla028 requested a review from tarasglek January 27, 2025 17:40
Copy link
Collaborator

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial feedback:

  • thanks for doing something so quickly!
  • the paperclip icon should automatically open the file chooser if there are no existing attachments vs being disabled, and do the same flow as the "Attach Files..." menu item
  • don't do the animated + button only the paperclip
  • In the model, have the first icon in the top-left be an empty file icon with a + sign, and "Add Files..." so the user can add more files.

@mulla028 mulla028 requested a review from humphd January 27, 2025 23:49
@mulla028
Copy link
Collaborator Author

mulla028 commented Jan 27, 2025

@humphd, made all changes based on review/feedback. I still consider it as a draft-PR, I feel that UI changes required here. Please let me know what UI changes for this PR do we need.

@mulla028
Copy link
Collaborator Author

@humphd , I found a bug in my code, but hitting my head off the wall, I don't understand how to fix it, tried different ways.
How to reproduce it:

  • If you delete all the files using modal, and then close modal.
  • Try to click on paperclip, it doesn't open file selector. However, debugging shows that it enters handleAttachFiles.

Copy link
Collaborator

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is getting better and better. Some things I notice while testing this:

  • When the app loads, there is weird flash when the paperclip seems to change state and get repositioned, or hidden/revealed? It should always be visible. Is it due to the mic icon changing state and causing reflow? Maybe we should put the paperclip to the right of the mic?
  • When you attach a lot of files, the modal grows in height forever. The body of the modal should be scrollable
  • the contrast on the blue and red icons for download and delete isn't great. Use high contrast (e.g., white on black, or dark gray, etc).
  • let's get rid of the "download" icon and make the filename a link that does download
  • let's change the trash can to an 'X' icon
  • change the tooltip for the paperclip to "Attach Files..."

src/components/PromptForm/PaperclipIcon.tsx Outdated Show resolved Hide resolved
src/components/PromptForm/PaperclipIcon.tsx Outdated Show resolved Hide resolved
src/components/PromptForm/PaperclipIcon.tsx Outdated Show resolved Hide resolved
src/components/PromptForm/PaperclipIcon.tsx Outdated Show resolved Hide resolved
@mulla028 mulla028 requested a review from humphd January 28, 2025 16:14
@mulla028
Copy link
Collaborator Author

mulla028 commented Jan 28, 2025

@humphd I decided to get rid of hover background, since I couldn't find colour that fits both themes, while border colour still makes user feel interaction with the file!
By the way, I think that I didn't utilize refreshFiles properly

@uday-rana
Copy link
Collaborator

@humphd I decided to get rid of hover background, since I couldn't find colour that fits both themes, while border colour still makes user feel interaction with the file! By the way, I think that I didn't utilize refreshFiles properly

You can set different colours based on the current theme with useColorMode: https://v2.chakra-ui.com/docs/styled-system/color-mode#usecolormode

@mulla028
Copy link
Collaborator Author

@humphd I decided to get rid of hover background, since I couldn't find colour that fits both themes, while border colour still makes user feel interaction with the file! By the way, I think that I didn't utilize refreshFiles properly

You can set different colours based on the current theme with useColorMode: https://v2.chakra-ui.com/docs/styled-system/color-mode#usecolormode

Thank you! Will try it out!

@mulla028 mulla028 marked this pull request as ready for review January 28, 2025 19:39
@mulla028 mulla028 changed the title [RAW] File Attachment Added File Attachment Added Jan 28, 2025
@mulla028 mulla028 changed the title File Attachment Added [UI] File Attachment Added Jan 28, 2025
Copy link
Collaborator

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some odd state issue with the text pop-up on the mic and paperclip icons:

Screenshot 2025-01-28 at 3 29 38 PM

It doesn't always vanish when I move my mouse. Could the text popups go above vs. below these so they don't cover the Ask button?

Let's not animate the x icon in the file icons in the modal, and let's not use a colour

Let's put the file count at the top of the modal:

8 Attached Files

Should we get rid of the Add Files icon, and instead make it a button in the footer of the modal? If so we could add a "Remove All" option too. Not sure if this is good or not, just thinking...

Another idea for a follow-up, should hovering over the icon show you the contents instead of the icon? Something to play with later, since we have the URL for each item or its text content, so we could display an image or first few lines of text, etc.

@mulla028 mulla028 marked this pull request as draft January 28, 2025 20:38
@mulla028
Copy link
Collaborator Author

mulla028 commented Jan 28, 2025

@humphd may I also change mic's tooltip label placement, since it closes the ask button?
UPD: You've already mentioned that :D

@humphd
Copy link
Collaborator

humphd commented Jan 28, 2025

Do it in a follow-up PR. The more we add to this, the hard it is to land.

At this point you're very close to a workable first version we can land. I'd get the outstanding issues done here, and file follow-up issues then tackle them next in subsequent PRs.

Small PRs are the way you build big software projects.

@mulla028
Copy link
Collaborator Author

Last one question before I begin. By saying:

Let's not use colour and x animation

Do you mean that I may get rid of hover effect for FileIcons at this point?

@humphd
Copy link
Collaborator

humphd commented Jan 28, 2025

Do you mean that I may get rid of hover effect for FileIcons at this point?

I like the x icon showing up when you hover over the icon. I don't like how it animates and slides down. Just show/hide it.

@mulla028 mulla028 requested a review from humphd January 28, 2025 21:37
Copy link
Collaborator

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Don't use red for the x icon when deleting
  • Make the buttons in the footer look more like this with a single button as the "Call to Action" (e.g., "Add files...") and the secondary button being a "ghost" variant with red for "Remove All". Get rid of the icons on the buttons. This way it's easier not to accidentally click the wrong thing:
Screenshot 2025-01-28 at 4 41 36 PM

@mulla028 mulla028 requested a review from humphd January 28, 2025 21:57
@mulla028 mulla028 marked this pull request as ready for review January 28, 2025 21:59
@tarasglek
Copy link
Owner

tarasglek commented Jan 29, 2025

This is cool, we can download stuff out of duckdb now =D

type the following command:

/duck copy (select 1) to 'foo.json'
  1. click attachments and click download!

  2. Notice UI bug that the file is seemingly of 0 bytes, but it downloads correctly.

image

Thoughts

I find the download UI to be unclear. I would prefer some permanent https://react-icons.github.io/react-icons/search/#q=download icon on every attachment

In general I would expect adding an attachment to popup a sidebar with list of attachments, not to have hidden modal...then we could per-file options like [include in chat, include as RAG, exclude, delete]

I think we can address these in follow up, this is good to land, awesome feature

@tarasglek
Copy link
Owner

Again, this is amazing seeing all this infrastructure code culminate in user-visible UI!

@mulla028 mulla028 force-pushed the mulla028/file-attachment branch from c1a4995 to 553a576 Compare January 30, 2025 17:38
@mulla028
Copy link
Collaborator Author

I'm sorry for doing that, hope now everything looks correct. The only thing left is to catch an error and implement UI for it

@mulla028 mulla028 requested a review from humphd January 30, 2025 18:04
@tarasglek
Copy link
Owner

Can we land this? I love this for uploading json into duckdb

Copy link
Collaborator

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small fix and this is good to land.

Great work @mulla028

src/lib/utils.ts Outdated Show resolved Hide resolved
@tarasglek
Copy link
Owner

I noticed we can't upload .jsonl files is there a reason we don't allow all files?

@mulla028
Copy link
Collaborator Author

mulla028 commented Jan 31, 2025

Input element accepts what is defined in hooks/use-file-import acceptableFileFormats, may be the cause is there

@mulla028
Copy link
Collaborator Author

@tarasglek , I made a research, and added .jsonl files upload support.

@mulla028
Copy link
Collaborator Author

It didn't work tho, this problem isn't related to current pr. We have to provide a support of .jsonl files, I will file an issue.

@humphd
Copy link
Collaborator

humphd commented Feb 2, 2025

Land this and do follow-ups, please.

@mulla028 mulla028 merged commit bcb726d into main Feb 2, 2025
4 checks passed
@tarasglek
Copy link
Owner

awesomeness!

@mulla028 mulla028 deleted the mulla028/file-attachment branch February 3, 2025 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add file attachment UI
4 participants