feat: improved file upload frontend and add attachment validation#981
feat: improved file upload frontend and add attachment validation#981puzzle-solver wants to merge 6 commits into
Conversation
dazy-ds
left a comment
dazy-ds
left a comment
There was a problem hiding this comment.
Overall looks good, I would like to have some additional context before approving. I just want to fully understand the rationale behind the implementation C:
Also left some food for thoughts :D
| export type FileDropHandler = (files: File[]) => void; | ||
|
|
||
| let handler: FileDropHandler | null = null; | ||
|
|
||
| export function registerFileDropHandler(h: FileDropHandler): () => void { | ||
| handler = h; | ||
| return () => { | ||
| if (handler === h) handler = null; | ||
| }; | ||
| } | ||
|
|
||
| export function dispatchDroppedFiles(files: File[]): boolean { | ||
| if (!handler || files.length === 0) return false; | ||
| handler(files); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I assume that this was added as a way of allowing plugins to register for file events, right?
If so, this is kind of a big architecture change/new feature and I think it should be discussed.
The implementation of this seems fine, as it somewhat mimics how we handle slots, etc. but I would like to know rationale behind that implementation.
On the side note, if we decide to keep it, I would move it to a directory called registires or events or something similar. That way we can communicate that those methods are used to register some handlers of global events emitted by the UI (just like in an even driven application, e.g. CMSes have similar mechanisms).
| export interface PreSendContribution { | ||
| context?: Record<string, unknown>; | ||
| userMessageExtra?: Record<string, unknown>; | ||
| files?: File[]; | ||
| } | ||
|
|
||
| type Contributor = () => PreSendContribution | null; | ||
|
|
||
| const contributors = new Set<Contributor>(); | ||
|
|
||
| export function registerPreSendContributor(c: Contributor): () => void { | ||
| contributors.add(c); | ||
| return () => { | ||
| contributors.delete(c); | ||
| }; | ||
| } | ||
|
|
||
| export function collectPreSendContext(): PreSendContribution { | ||
| let context: Record<string, unknown> | undefined; | ||
| let userMessageExtra: Record<string, unknown> | undefined; | ||
| const files: File[] = []; | ||
| for (const c of contributors) { | ||
| const r = c(); | ||
| if (!r) continue; | ||
| if (r.context) context = { ...(context ?? {}), ...r.context }; | ||
| if (r.userMessageExtra) { | ||
| userMessageExtra = { ...(userMessageExtra ?? {}), ...r.userMessageExtra }; | ||
| } | ||
| if (r.files && r.files.length > 0) files.push(...r.files); | ||
| } | ||
| return { | ||
| context, | ||
| userMessageExtra, | ||
| files: files.length > 0 ? files : undefined, | ||
| }; | ||
| } |
| add: (file: File) => { | ||
| const id = crypto.randomUUID(); | ||
| const entry: UploadingAttachment = { | ||
| id, | ||
| filename: file.name, | ||
| mimeType: file.type || "application/octet-stream", | ||
| blobUrl: URL.createObjectURL(file), | ||
| status: "uploading", | ||
| }; | ||
| set((state) => ({ pending: [...state.pending, entry] })); | ||
| return id; | ||
| }, | ||
| addFailed: (file: File, error: string, retryFile?: File) => { | ||
| const entry: FailedAttachment = { | ||
| id: crypto.randomUUID(), | ||
| filename: file.name, | ||
| mimeType: file.type || "application/octet-stream", | ||
| blobUrl: URL.createObjectURL(file), | ||
| status: "failed", | ||
| error, | ||
| retryFile, | ||
| }; | ||
| set((state) => ({ pending: [...state.pending, entry] })); | ||
| }, |
There was a problem hiding this comment.
This seems like it could be merged into one method. We could shift the setting of "status" to consumer of the store and expect objects of a specified structure as arguments (to enforce the required arguments).
E.g.
interface FailedFile {
file: File,
status: "failed",
error: string,
retryFile?: File
}
interface UploadingFile {
file: File,
status: "uploading"
}
add(fileObject: UploadingFile | FailedFile) {}I want to note that this is a pure cosmetic change coming from my preference :D
We can also keep this as is, but abstract the common entry creation process (it would give the id, extract filename, etc), and keep the case specific code in the existing methods.
| unregisterContributor = registerPreSendContributor(() => { | ||
| const { files, previews } = useUploadAttachmentsStore | ||
| .getState() | ||
| .consumeReady(); | ||
| if (files.length === 0) return null; | ||
| return { | ||
| files, | ||
| userMessageExtra: { attachments: previews }, | ||
| }; | ||
| }); | ||
| unregisterDropHandler = registerFileDropHandler((files) => { | ||
| files.forEach(processFile); | ||
| }); |
There was a problem hiding this comment.
Registering this way is okay (although I am not a fan of usage of global variables for such purposes).
We can also consider expanding the Plugin to support a list of callbacks to be returned from onActivate that would be automatically called when plugin is deactivated. It can simplify the plugin registration process while also keeping onDeactivate for some general stuff.
Either way I am fine with that approach, just my thought.
| } | ||
| requestBody = formData; | ||
| attachmentsStore.getState().clear(); | ||
| } |
There was a problem hiding this comment.
This is the part that got most of my attention in this PR.
Previous version negated whole purpose of having plugins, because it directly imported a file from a plugin into one of the core entities. The fixed version is better, but I am still not sure about splitting files and userMessageExtra objects.
One can also wonder why would we add a utility to add files into the core, other that for uploading them. I can't see other use case for files dropped into the chat, other than them being uploaded to be processed (or processed on the client). Having that in mind, we could consider adding whole UploadPlugin into the core.
To summarize I think this is fine with a small nit about naming and separating files from userMessageExtra, because for me, files are extra additions to user's message.
| } | ||
| case "wrong-type": | ||
| return `File type not supported. Allowed: ${error.allowed.join(", ")}.`; | ||
| } |
There was a problem hiding this comment.
nit: We can add exhaustive check to always make sure that we have all cases handled in case a new kind is added (https://stackoverflow.com/questions/39419170/how-do-i-check-that-a-switch-block-is-exhaustive-in-typescript)
2 participants
Added additional frontend+backend validation of uploads (configurable from the backend). Improved UI showing the attached files.