Allow updating files (instead of overriding) for multiple file uploads#191
Allow updating files (instead of overriding) for multiple file uploads#191janezd wants to merge 1 commit intobiolab:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements the ability to add multiple files to upload-type questions without replacing existing files, along with the ability to remove individual files. This addresses issue #177 which requested allowing additional uploads when answering upload-type questions.
Changes:
- Modified the upload answer logic to merge newly uploaded files with existing files instead of replacing them
- Added a
removeUserFileAPI function to allow deleting individual uploaded files - Updated the UI to display uploaded files with delete buttons when editing is allowed
- Changed the upload directory handling to preserve existing files during new uploads
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| next.config.ts | Removed boilerplate comment |
| context/QuizContextProvider.tsx | Added removeFile function to context, updated to use storedAnswer from backend |
| components/Quiz/UploadQuestion.tsx | Added UI for displaying and removing uploaded files, updated file selection logic |
| components/Quiz/Quiz.tsx | Removed TODO comment |
| app/api/upload-answer/route.ts | Changed to create directory if needed instead of removing and recreating |
| api/quiz.ts | Added removeUserFile function, modified postAnswer to merge uploaded files |
| .gitattributes | Added merge strategy for auto-generated file |
Comments suppressed due to low confidence (3)
api/quiz.ts:84
- Avoid automated semicolon insertion (92% of all statements in the enclosing function have an explicit semicolon).
: question.answer.trim().toLocaleLowerCase() === answer.trim().toLocaleLowerCase()
components/Quiz/Quiz.tsx:87
- Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
msg += ` ${t("quiz.remaining")}: ${maxAttempts - attempts}`
context/QuizContextProvider.tsx:264
- Avoid automated semicolon insertion (94% of all statements in the enclosing function have an explicit semicolon).
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div className="flex flex-col text-red-500" style={{lineHeight: "1.4"}}> | ||
| <small className="form-text text-muted"> | ||
| {t("quiz.submit-will-replace")(files.length)} | ||
| {t("quiz.dont-forget-to-submit-file")(files.length)} |
There was a problem hiding this comment.
The container div has the class "text-red-500" to make text red, but the small elements inside have class "text-muted" which likely overrides the red color. The red color class should be moved to the individual small elements or removed from the parent if it's not needed.
| <RiDeleteBin2Line | ||
| onClick={() => onRemoveUploadedFile(file)} | ||
| style={{cursor: "pointer"}} | ||
| className="hover:text-red-700" | ||
| /> |
There was a problem hiding this comment.
The delete icon buttons are not keyboard accessible and lack proper ARIA labels. Users navigating with a keyboard cannot tab to these buttons, and screen reader users won't know what the buttons do. Consider wrapping the icon in a button element with proper aria-label, or adding tabIndex={0} and onKeyDown handlers to support keyboard navigation.
|
|
||
| const answer = await removeUserFile({ | ||
| accessToken: user.accessToken, bookId, questionId, groupId, fileName}); | ||
| if (answer === null) { |
There was a problem hiding this comment.
The removeFile callback doesn't handle errors properly. If removeUserFile returns null (indicating failure), the function returns false but doesn't set an error state in the reducer. Users won't see any feedback explaining why the file wasn't removed. Consider adding error handling similar to the uploadFiles function that calls quizReducer with type "ERROR" when the operation fails.
| if (answer === null) { | |
| if (answer === null) { | |
| quizReducer({type: "ERROR", value: {questionId}}); |
| isCorrect: undefined, | ||
| points: 0 | ||
| } | ||
| }); |
There was a problem hiding this comment.
The closing brace has inconsistent indentation with 7 spaces instead of 8 (like the opening brace on line 289). This should be aligned properly for consistency.
| }); | |
| }); |
| if (!existsSync(dir)) { | ||
| mkdirSync(dir, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
The change from removing and recreating the directory to just creating it if it doesn't exist means that if a user uploads a new file with the same name as an existing file, the new file will silently overwrite the old one in the file system. However, the database merge logic in api/quiz.ts (line 91) filters out duplicates with !pastFiles.includes(f). This means the behavior is correct for preventing duplicate entries in the database, but users should be aware that re-uploading a file with the same name replaces the previous version.
| fs.rmSync(fname); | ||
| } | ||
| } | ||
| const newAnswers = answers.filter((f) => f !== fileName).join(":"); |
There was a problem hiding this comment.
When all files are removed, the filter operation will create an empty array, and join(":") will return an empty string. However, if there's ever a case where answer is already an empty string, split(":") returns [""], and if fileName is also "", this could match and try to filter it out. Consider adding a check to handle empty answers more explicitly, or filter out empty strings after split: answers.filter(f => f && f !== fileName)
| const newAnswers = answers.filter((f) => f !== fileName).join(":"); | |
| const newAnswers = answers.filter((f) => f && f !== fileName).join(":"); |
| <RiDeleteBin2Line | ||
| onClick={() => onRemoveFile(f.name)} | ||
| style={{cursor: "pointer"}} | ||
| className="hover:text-red-700" | ||
| /> |
There was a problem hiding this comment.
Similar to the uploaded file delete button, this delete icon for selected files is not keyboard accessible and lacks proper ARIA labels. Consider wrapping the icon in a button element with proper aria-label, or adding tabIndex={0} and onKeyDown handlers to support keyboard navigation.
| return null; | ||
| } | ||
| const answers = lastAnswer.answer.split(":"); | ||
| if (!answers.includes(fileName)) { | ||
| return null; | ||
| } | ||
|
|
||
| const {dir, error} = await getUploadDir( | ||
| {accessToken, bookId, groupId, qId: questionId}); | ||
| if (dir && !error) { | ||
| const fname = path.join(dir, fileName); | ||
| if (fs.existsSync(fname)) { | ||
| fs.rmSync(fname); | ||
| } | ||
| } | ||
| const newAnswers = answers.filter((f) => f !== fileName).join(":"); | ||
| await db.run(` | ||
| UPDATE answers | ||
| SET answer = ? | ||
| WHERE id = ?`, | ||
| [newAnswers, lastAnswer.id] | ||
| ); |
There was a problem hiding this comment.
The removeFile function has a potential race condition. If the database update succeeds but the file system deletion fails (or vice versa), the system state becomes inconsistent. Consider either handling errors from fs.rmSync or wrapping the entire operation in a transaction-like pattern. Additionally, if answer is null, the function returns false but doesn't provide any error feedback to the user.
| return null; | |
| } | |
| const answers = lastAnswer.answer.split(":"); | |
| if (!answers.includes(fileName)) { | |
| return null; | |
| } | |
| const {dir, error} = await getUploadDir( | |
| {accessToken, bookId, groupId, qId: questionId}); | |
| if (dir && !error) { | |
| const fname = path.join(dir, fileName); | |
| if (fs.existsSync(fname)) { | |
| fs.rmSync(fname); | |
| } | |
| } | |
| const newAnswers = answers.filter((f) => f !== fileName).join(":"); | |
| await db.run(` | |
| UPDATE answers | |
| SET answer = ? | |
| WHERE id = ?`, | |
| [newAnswers, lastAnswer.id] | |
| ); | |
| console.warn("removeUserFile: no previous answer found for given parameters", { | |
| bookId, | |
| groupId, | |
| questionId, | |
| fileName, | |
| }); | |
| return null; | |
| } | |
| const answers = lastAnswer.answer.split(":"); | |
| if (!answers.includes(fileName)) { | |
| console.warn("removeUserFile: requested file name not found in last answer", { | |
| bookId, | |
| groupId, | |
| questionId, | |
| fileName, | |
| }); | |
| return null; | |
| } | |
| const oldAnswers = lastAnswer.answer; | |
| const newAnswers = answers.filter((f) => f !== fileName).join(":"); | |
| // First, update the database. If this fails, do not touch the filesystem. | |
| try { | |
| await db.run(` | |
| UPDATE answers | |
| SET answer = ? | |
| WHERE id = ?`, | |
| [newAnswers, lastAnswer.id] | |
| ); | |
| } catch (err) { | |
| console.error("removeUserFile: failed to update answers in database", { | |
| error: err, | |
| answerId: lastAnswer.id, | |
| }); | |
| return null; | |
| } | |
| // After a successful DB update, attempt to remove the file. On failure, try to roll back the DB change. | |
| const {dir, error} = await getUploadDir( | |
| {accessToken, bookId, groupId, qId: questionId}); | |
| if (dir && !error) { | |
| const fname = path.join(dir, fileName); | |
| if (fs.existsSync(fname)) { | |
| try { | |
| fs.rmSync(fname); | |
| } catch (fsErr) { | |
| console.error("removeUserFile: failed to delete file from filesystem, attempting to roll back DB change", { | |
| error: fsErr, | |
| filePath: fname, | |
| answerId: lastAnswer.id, | |
| }); | |
| try { | |
| await db.run(` | |
| UPDATE answers | |
| SET answer = ? | |
| WHERE id = ?`, | |
| [oldAnswers, lastAnswer.id] | |
| ); | |
| } catch (rollbackErr) { | |
| console.error("removeUserFile: failed to roll back database after filesystem error; state may be inconsistent", { | |
| error: rollbackErr, | |
| answerId: lastAnswer.id, | |
| }); | |
| } | |
| return null; | |
| } | |
| } | |
| } |
| return {status: "error", message: "Maximum number of attempts reached"}; | ||
| } | ||
|
|
||
| const pastFiles = question.type !== "uploads" || !pastAnswers.length ? [] |
There was a problem hiding this comment.
The condition question.type !== "uploads" only checks for the plural form, but according to the codebase, upload questions can have type "upload" (singular) or "uploads" (plural). This means the file merging logic won't work for singular "upload" type questions. The condition should be question.type !== "uploads" && question.type !== "upload" or use !question.type.startsWith("upload") to handle both cases.
| const pastFiles = question.type !== "uploads" || !pastAnswers.length ? [] | |
| const pastFiles = (question.type !== "uploads" && question.type !== "upload") || !pastAnswers.length ? [] |
| </> | ||
| } | ||
| } | ||
| </> |
There was a problem hiding this comment.
Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).
| </> | |
| </>; |
|
Closed in favor of #198. |
1 participant
Fixes #177.