Bugfix/bai 1297 fix issue with file uploads #1338
Conversation
ARADDCC002
commented
Jun 19, 2024
ARADDCC002
commented
…oads will be stored so they are not re-uploaded.
| const failedFileList = useMemo( | ||
| () => | ||
| failedFileUploads.map((file) => ( | ||
| <div key={file.fileName}> |
There was a problem hiding this comment.
File names are unique although If three files of the same name were uploaded two of them would fail. Would it be better using the id here instead of the fileName
There was a problem hiding this comment.
We don't have access to the ID for failed uploads, so name is the next best thing. Worst case it'll cause a minor performance issue as react won't be able to track each item in the list (unless someone uploads a large amount of files with the same name).
| successfulFiles.forEach((file) => { | ||
| if (!successfulFileUploads.find((successfulFile) => successfulFile.fileName === file.fileName)) { | ||
| setSuccessfulFileUploads([...successfulFileUploads, file]) | ||
| } | ||
| }) |
There was a problem hiding this comment.
Would setSuccessfulFileUpload([...new Set([...successfulFiles, ...successfulFileUploads))]) be a little more performant?
There was a problem hiding this comment.
Due to us creating the objects in successfulFiles each time this function runs, they will have a different reference to the ones in successfulFileUploads even if the properties are the same. That being said, I think this logic can be improved, so I'll do something with it to ensure we're not calling setSuccessfulFileUpload multiple times unnecessarily.
| const failedFileList = useMemo( | ||
| () => | ||
| failedFileUploads.map((file) => ( | ||
| <div key={file.fileName}> |
There was a problem hiding this comment.
Same with using id instead of fileName here
| @@ -49,7 +49,7 @@ export async function postRelease(release: CreateReleaseParams) { | |||
|
|
|||
| export type UpdateReleaseParams = Pick< | |||
| ReleaseInterface, | |||
| 'modelId' | 'modelCardVersion' | 'semver' | 'notes' | 'minor' | 'draft' | 'fileIds' | 'images' | 'comments' | |||
| 'modelId' | 'modelCardVersion' | 'semver' | 'notes' | 'minor' | 'draft' | 'fileIds' | 'images' | |||
There was a problem hiding this comment.
Comment's also seem to be optional on creation. Could I suggest adding it as optional to the ReleaseInterface. There's some conditional rendering on the Releases page that seems to break when there is no comment attribute. This could just be a result of switching branches.
There was a problem hiding this comment.
I would agree, but comments are now stored in a separate collection and are no longer a part of the ReleaseInterface
| setLoading(false) | ||
| } | ||
| } catch (e) { | ||
| if (e instanceof Error) { |
There was a problem hiding this comment.
Why is this assertion necessary and does setLoading(false) inside it impact if this is not carried out?
There was a problem hiding this comment.
Good catch on the setLoading(false). I've removed it from this section of the logic as we shouldn't set it until we've finished processing all files. I've ensured we set it to false at the end of this process.
As for the assertion, errors are unknown so this is a simple hack to handle it as an error. Realistically it'll always be an error, but it's not the best piece of code at all.
