Enhancement/file upload connection - #110
Merged
Merged
Conversation
…files with correct ids
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR refactors file upload functionality from single-file to multi-file support across controllers, services, and UI layers. Endpoint mappings shift to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant JavaScript as JS (script.js)
participant Controller as CaseViewController
participant CaseService
participant S3Service
participant S3 as AWS S3
participant Database
Client->>JavaScript: Upload multiple files
JavaScript->>S3Service: GET /tickets/upload/api/files/upload-url<br/>(for each file, with caseId)
S3Service->>S3: Generate presigned URL
S3->>S3Service: Return presigned URL
S3Service->>JavaScript: Return upload URL
JavaScript->>S3: POST file (with presigned URL)
S3->>JavaScript: Upload complete
JavaScript->>Controller: POST /create with fileNames[]
Controller->>CaseService: createTicket(request, username)
CaseService->>Database: Save CaseEntity
CaseService->>S3Service: generateS3PreUploadUrl(caseId, fileName, contentType)
S3Service->>S3: Create structured S3 key<br/>tickets/{caseId}/uploads/{uuid}/{fileName}
S3->>S3Service: Return S3 key
CaseService->>Database: Create UploadedFile with s3Key
CaseService->>Database: Save updated CaseEntity
CaseService->>Client: Return CaseEntityDto
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/org/example/untitled/s3/S3Service.java (1)
64-76:⚠️ Potential issue | 🟠 MajorPresigned upload key does not match persisted metadata key, breaking round-trip consistency.
The presigned PUT request at line 70 signs for key
fileIn(UUID prefix + fileName), and the client uploads to this key. However,createFile()at line 97 persists a completely different key (tickets/{caseId}/uploads/{fileName}) in the database. Downstream download and delete operations retrieve this stored key and will fail to find the uploaded object.Fix: Either pass
fileInfromgenerateS3PreUploadUrl()tocreateFile()and persist that, or generate the presigned URL with the ticket-scoped path directly. If the ticket-scoped structure is required, copy the uploaded object to that key before returning fromcreateFile().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/s3/S3Service.java` around lines 64 - 76, The presigned PUT signs the key stored elsewhere, causing mismatch: modify generateS3PreUploadUrl and/or createFile so they use the same S3 key (fileIn) — either (A) construct the presigned key as the ticket-scoped path (tickets/{caseId}/uploads/{fileName}) inside generateS3PreUploadUrl (use that string for fileIn) so the client uploads directly to the persisted key, or (B) keep the UUID-prefixed fileIn produced by generateS3PreUploadUrl and change createFile to accept and persist that fileIn (pass fileIn through the call chain) so stored metadata matches the uploaded object; reference generateS3PreUploadUrl, createFile, and the fileIn variable and ensure the chosen path is used both when creating the PresignedPutObjectRequest and when persisting the key to the database.src/main/resources/templates/edit_ticket.html (1)
7-31:⚠️ Potential issue | 🟠 MajorFix edit form to match multi-file contract: use
fileNamesbinding and add missing file container.The
edit_ticket.htmlform has three issues preventing uploaded files from binding correctly:
- Line 25 binds to
th:field="*{fileName}"which no longer exists onCreateCaseRequest(now usesfileNames: List<String>)- Missing the
<div id="hidden-file-inputs"></div>container thatscript.jsrequires to append uploaded file entries- File input lacks
multipleattribute, restricting to single fileAlign with
create_ticket.html:Required changes
- <input type="hidden" id="fileNameHidden" th:field="*{fileName}"> + <div id="hidden-file-inputs"></div> - <input type="file" id="fileInput"> + <input type="file" id="fileInput" multiple>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/edit_ticket.html` around lines 7 - 31, The edit form binds to a removed single-file field and is missing DOM elements required by script.js; update the form to bind to the new List<String> property by replacing th:field="*{fileName}" with th:field="*{fileNames}", add the missing container <div id="hidden-file-inputs"></div> (the element script.js uses to append hidden inputs for uploaded files), and make the file input support multiple files by adding the multiple attribute to the element with id="fileInput" so uploadNewFile() and the multi-file contract work as in create_ticket.html.src/main/resources/static/js/script.js (1)
79-89:⚠️ Potential issue | 🟠 MajorPre-existing: delete-confirm condition is inverted.
Outside this PR's diff surface, but worth flagging since the path was changed here:
if (!window.confirm(...))means the DELETE request fires only when the user clicks Cancel; clicking OK skips the deletion. The branch appears logically inverted.- if (!window.confirm(fileName + " will be deleted! Are you sure?")) { + if (window.confirm(fileName + " will be deleted! Are you sure?")) { const res = await apiReq(...);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/script.js` around lines 79 - 89, The deleteFile function contains an inverted confirmation check so the DELETE request runs when the user clicks Cancel; in deleteFile, invert the condition to run the apiReq call only when window.confirm(...) returns true (i.e., user confirmed), and keep the error/status handling and fetchAFile() logic inside that confirmed branch; ensure you reference deleteFile, apiReq(`/tickets/upload/api/files/delete-url?...`), fetchAFile, and status.innerText when making the change.
🧹 Nitpick comments (6)
src/main/java/org/example/untitled/usercase/dto/CreateCaseRequest.java (1)
5-27: Consider normalizingfileNamesto an empty list for cleaner separation of concerns.The field currently defaults to
null, but both call sites inCaseServicealready guard against null before iteration. While the current code is safe, normalizingfileNamesto an empty list would eliminate the need for repeated null checks downstream.Suggested improvement
+import java.util.ArrayList; import java.util.List; @@ - private List<String> fileNames; + private List<String> fileNames = new ArrayList<>(); @@ public void setFileNames(List<String> fileNames) { - this.fileNames = fileNames; + this.fileNames = fileNames == null ? new ArrayList<>() : fileNames; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/usercase/dto/CreateCaseRequest.java` around lines 5 - 27, CreateCaseRequest.fileNames defaults to null which forces callers to null-check; initialize and normalize it to an empty list to remove downstream null checks: set fileNames = Collections.emptyList() (or new ArrayList<>()) in the no-arg constructor, update getFileNames() to return an empty list (never null), and make setFileNames(List<String>) treat a null input as an empty list so the field is always non-null; reference CreateCaseRequest, the no-arg constructor, getFileNames, and setFileNames when applying the change.src/main/java/org/example/untitled/s3/S3RestController.java (1)
21-21: Minor: drop empty parentheses in@GetMapping().Idiomatic Spring style is
@GetMapping(no parentheses) when no attributes are set. Purely cosmetic.- `@GetMapping`() + `@GetMapping` `@ResponseBody` public List<String> listFiles(){🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/s3/S3RestController.java` at line 21, In S3RestController, remove the empty parentheses from the annotation at the handler method so it uses the idiomatic form `@GetMapping` (instead of `@GetMapping`()) — locate the annotation above the controller method in class S3RestController and replace the current `@GetMapping`() with `@GetMapping`.src/main/resources/templates/create_ticket.html (1)
31-31: Submit path bypasses browser-level validation and error handling.The flow
type="button"→uploadNewFile()→document.querySelector('form').submit()(inscript.js) triggers form submission unconditionally — even when the user didn't select any files (input.files.length === 0returns early and never submits), but also when some/all uploads failed (the loop only updates status text and still falls through toform.submit()at line 75 ofscript.js). Programmaticform.submit()also does not run HTML5required/constraint validation.Consider:
- Calling
form.requestSubmit()sonovalidaterules and submit events fire, or manually invokingform.reportValidity()before submitting.- Tracking per-file success and aborting / asking for confirmation if any upload failed, so you don't create a ticket that references files that never made it to S3.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/create_ticket.html` at line 31, The Create Ticket button uses type="button" and calls uploadNewFile(), which ultimately calls document.querySelector('form').submit() and bypasses HTML5 validation and proper error handling; update uploadNewFile() in script.js to use form.requestSubmit() (or call form.reportValidity() and only submit when true) instead of form.submit(), and add per-file upload success tracking so that if any upload fails you abort submission or prompt the user for confirmation before submitting (ensure the code paths reference uploadNewFile(), the per-file upload loop, and the final submit call to enforce validation and check a success flag/count before calling requestSubmit()).src/main/resources/static/js/script.js (1)
55-60: Avoid shadowing the outerinputvariable.The outer
input(line 28) is thefileInputelement; here a newconst inputis declared for a dynamically-created hidden<input>. It's block-scoped so not broken, but the collision is easy to misread during future edits. Rename tohiddenInput:- const hiddenContainer = document.getElementById("hidden-file-inputs"); - const input = document.createElement('input'); - input.type = 'hidden'; - input.name = 'fileNames'; - input.value = uploadedFileName; - hiddenContainer.appendChild(input); + const hiddenContainer = document.getElementById("hidden-file-inputs"); + const hiddenInput = document.createElement('input'); + hiddenInput.type = 'hidden'; + hiddenInput.name = 'fileNames'; + hiddenInput.value = uploadedFileName; + hiddenContainer.appendChild(hiddenInput);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/script.js` around lines 55 - 60, The dynamically-created DOM element currently declared as const input inside the block shadows the outer file input variable named input; rename this inner variable to hiddenInput (or similar) where it is created/used in the block that references hiddenContainer and uploadedFileName, i.e., replace const input = document.createElement('input') and all subsequent uses of input.append/value/appendChild with hiddenInput so there is no name collision with the outer input element.src/main/resources/templates/upload.html (2)
7-7: Preferdeferfor the head-loaded script, for consistency withcreate_ticket.html.
create_ticket.htmlloads the same script withdefer(<script th:src="@{/js/script.js}" defer></script>). Using a plain<script src=...>in<head>here blocks HTML parsing. Since none of the handlers run at parse time, adddefer(and consider usingth:src="@{/js/script.js}"for consistent URL resolution under a context path).Proposed change
- <script src="/js/script.js" type="text/javascript"></script> + <script th:src="@{/js/script.js}" defer></script>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/upload.html` at line 7, Update the head-loaded script tag in upload.html to use the defer attribute and the Thymeleaf URL form to match create_ticket.html: change the plain <script src="/js/script.js"> reference to use defer and th:src (i.e., the same pattern used in create_ticket.html) so the browser won't block HTML parsing and the URL resolves correctly under a context path; specifically modify the script tag that loads script.js to include defer and use th:src="@{/js/script.js}".
23-44: Remove the commented-out Thymeleaf block (dead markup).Leaving this large block commented out adds noise and will drift from the real implementation. If the intent is to replace it with client-rendered content (as suggested by
fetchAFile()targeting#fileTableBodyinscript.js), either restore the table skeleton with a<tbody id="fileTableBody">and callfetchAFile()on load, or delete this block entirely and rely on version control for history.As-is, this page no longer contains
#fileTableBody, sofetchAFile()(the client-side replacement for the removed server rendering) would fail silently if wired up from here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/upload.html` around lines 23 - 44, The commented-out Thymeleaf table is dead markup and breaks the client-side loader; either delete that entire commented block or restore a minimal table skeleton containing a tbody with id "fileTableBody" and ensure fetchAFile() (from script.js) is invoked on page load; locate the commented block in upload.html, remove it if you want client-rendered content only, or re-add the <tbody id="fileTableBody"> and hook fetchAFile() so the client-side download list can populate correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/untitled/s3/S3RestController.java`:
- Around line 35-41: The client is calling the wrong download URL path; align
the client call in src/main/resources/static/js/script.js (around the fetch at
line ~19) with the controller endpoint exposed by S3RestController.downloadFile
(GET mapping "/download-url" under the /tickets/upload/api/files base path).
Update the path used in script.js from "/tickets/upload/files/download-url" to
"/tickets/upload/api/files/download-url" so the fetch matches the controller
route.
In `@src/main/java/org/example/untitled/usercase/service/CaseService.java`:
- Around line 60-64: The loop that calls s3Service.createFile using entries from
request.getFileNames() must skip null or blank names to avoid creating
UploadedFile with empty filename/s3Key; update the checks in the createTicket
logic (the block that iterates request.getFileNames() and calls
caseEntity.getFiles().addAll(s3Service.createFile(...))) and the symmetric
updateTicket block to filter each fName with a guard like not-null and
not-empty-after-trim (or StringUtils.hasText) before invoking
s3Service.createFile(caseEntity, fName).
- Around line 91-95: The updateTicket logic currently appends UploadedFile
entities for every name in request.getFileNames() without checking existing
associations (caseEntity.getFiles()), causing duplicate rows referencing the
same s3Key; update the code in updateTicket to first build a Set of existing
s3Keys/file names from caseEntity.getFiles() (UploadedFile.s3Key or fileName)
and then only call s3Service.createFile(caseEntity, fName) for names not already
present; do not create or add duplicates and ensure you operate on
caseEntity.getFiles() (which uses CascadeType.ALL + orphanRemoval) so that
existing associations remain unchanged unless explicitly removed by a separate
delete/diff flow.
In `@src/main/resources/static/js/script.js`:
- Around line 26-33: The submit button is being disabled unconditionally
(submitBtn) and the handler returns early when no files are present
(input.files.length === 0), leaving the button disabled and preventing form
submission; fix by only disabling submitBtn and changing its innerText when
there are files to upload (check input.files.length > 0 before setting
submitBtn.disabled = true and submitBtn.innerText = "Uploading..."), and if you
keep the early return path ensure you re-enable submitBtn (or proceed to submit
the form) so a ticket can be created with no attachments; update the logic
around submitBtn, input, and filesToUpload to reflect attachments being
optional.
- Around line 61-76: The upload loop currently continues on upload failure
(putRes.ok === false) and in the catch, causing
document.querySelector('form').submit() to run even when uploads failed and the
final status message is written after navigation; fix by introducing a failure
flag (e.g., uploadFailed boolean or failedFiles array) updated inside the
putRes.ok === false branch and the catch block, immediately stop further
processing for that file (return from the enclosing async function or break the
loop) and do not call form.submit() if uploadFailed is true, re-enable submitBtn
and leave its label for the user to retry; also move or set status.innerText =
"All uploads completed." before calling form.submit() (or remove it) so it is
visible prior to navigation.
In `@src/main/resources/templates/create_ticket.html`:
- Line 25: Remove the static hidden input (<input type="hidden"
th:field="*{fileNames}" id="s3FileName">) from the create_ticket.html template
because it renders an empty value and causes a spurious "" entry to be
submitted; instead rely on the JS that appends hidden inputs into
`#hidden-file-inputs`. Alternatively (or additionally) add defensive filtering in
CaseService.createTicket and CaseService.updateTicket to skip null/blank entries
from request.getFileNames() before calling S3Service.createFile so empty strings
are not passed to S3.
---
Outside diff comments:
In `@src/main/java/org/example/untitled/s3/S3Service.java`:
- Around line 64-76: The presigned PUT signs the key stored elsewhere, causing
mismatch: modify generateS3PreUploadUrl and/or createFile so they use the same
S3 key (fileIn) — either (A) construct the presigned key as the ticket-scoped
path (tickets/{caseId}/uploads/{fileName}) inside generateS3PreUploadUrl (use
that string for fileIn) so the client uploads directly to the persisted key, or
(B) keep the UUID-prefixed fileIn produced by generateS3PreUploadUrl and change
createFile to accept and persist that fileIn (pass fileIn through the call
chain) so stored metadata matches the uploaded object; reference
generateS3PreUploadUrl, createFile, and the fileIn variable and ensure the
chosen path is used both when creating the PresignedPutObjectRequest and when
persisting the key to the database.
In `@src/main/resources/static/js/script.js`:
- Around line 79-89: The deleteFile function contains an inverted confirmation
check so the DELETE request runs when the user clicks Cancel; in deleteFile,
invert the condition to run the apiReq call only when window.confirm(...)
returns true (i.e., user confirmed), and keep the error/status handling and
fetchAFile() logic inside that confirmed branch; ensure you reference
deleteFile, apiReq(`/tickets/upload/api/files/delete-url?...`), fetchAFile, and
status.innerText when making the change.
In `@src/main/resources/templates/edit_ticket.html`:
- Around line 7-31: The edit form binds to a removed single-file field and is
missing DOM elements required by script.js; update the form to bind to the new
List<String> property by replacing th:field="*{fileName}" with
th:field="*{fileNames}", add the missing container <div
id="hidden-file-inputs"></div> (the element script.js uses to append hidden
inputs for uploaded files), and make the file input support multiple files by
adding the multiple attribute to the element with id="fileInput" so
uploadNewFile() and the multi-file contract work as in create_ticket.html.
---
Nitpick comments:
In `@src/main/java/org/example/untitled/s3/S3RestController.java`:
- Line 21: In S3RestController, remove the empty parentheses from the annotation
at the handler method so it uses the idiomatic form `@GetMapping` (instead of
`@GetMapping`()) — locate the annotation above the controller method in class
S3RestController and replace the current `@GetMapping`() with `@GetMapping`.
In `@src/main/java/org/example/untitled/usercase/dto/CreateCaseRequest.java`:
- Around line 5-27: CreateCaseRequest.fileNames defaults to null which forces
callers to null-check; initialize and normalize it to an empty list to remove
downstream null checks: set fileNames = Collections.emptyList() (or new
ArrayList<>()) in the no-arg constructor, update getFileNames() to return an
empty list (never null), and make setFileNames(List<String>) treat a null input
as an empty list so the field is always non-null; reference CreateCaseRequest,
the no-arg constructor, getFileNames, and setFileNames when applying the change.
In `@src/main/resources/static/js/script.js`:
- Around line 55-60: The dynamically-created DOM element currently declared as
const input inside the block shadows the outer file input variable named input;
rename this inner variable to hiddenInput (or similar) where it is created/used
in the block that references hiddenContainer and uploadedFileName, i.e., replace
const input = document.createElement('input') and all subsequent uses of
input.append/value/appendChild with hiddenInput so there is no name collision
with the outer input element.
In `@src/main/resources/templates/create_ticket.html`:
- Line 31: The Create Ticket button uses type="button" and calls
uploadNewFile(), which ultimately calls document.querySelector('form').submit()
and bypasses HTML5 validation and proper error handling; update uploadNewFile()
in script.js to use form.requestSubmit() (or call form.reportValidity() and only
submit when true) instead of form.submit(), and add per-file upload success
tracking so that if any upload fails you abort submission or prompt the user for
confirmation before submitting (ensure the code paths reference uploadNewFile(),
the per-file upload loop, and the final submit call to enforce validation and
check a success flag/count before calling requestSubmit()).
In `@src/main/resources/templates/upload.html`:
- Line 7: Update the head-loaded script tag in upload.html to use the defer
attribute and the Thymeleaf URL form to match create_ticket.html: change the
plain <script src="/js/script.js"> reference to use defer and th:src (i.e., the
same pattern used in create_ticket.html) so the browser won't block HTML parsing
and the URL resolves correctly under a context path; specifically modify the
script tag that loads script.js to include defer and use
th:src="@{/js/script.js}".
- Around line 23-44: The commented-out Thymeleaf table is dead markup and breaks
the client-side loader; either delete that entire commented block or restore a
minimal table skeleton containing a tbody with id "fileTableBody" and ensure
fetchAFile() (from script.js) is invoked on page load; locate the commented
block in upload.html, remove it if you want client-rendered content only, or
re-add the <tbody id="fileTableBody"> and hook fetchAFile() so the client-side
download list can populate correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7affae8-a0c9-43e6-994a-7f3439dc2729
📒 Files selected for processing (14)
src/main/java/org/example/untitled/s3/S3Controller.javasrc/main/java/org/example/untitled/s3/S3RestController.javasrc/main/java/org/example/untitled/s3/S3Service.javasrc/main/java/org/example/untitled/usercase/CaseEntity.javasrc/main/java/org/example/untitled/usercase/UploadedFile.javasrc/main/java/org/example/untitled/usercase/controller/CaseController.javasrc/main/java/org/example/untitled/usercase/controller/CaseViewController.javasrc/main/java/org/example/untitled/usercase/dto/CreateCaseRequest.javasrc/main/java/org/example/untitled/usercase/service/CaseService.javasrc/main/resources/static/js/script.jssrc/main/resources/templates/create_ticket.htmlsrc/main/resources/templates/edit_ticket.htmlsrc/main/resources/templates/upload.htmlsrc/main/resources/templates/userpage.html
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/example/untitled/usercase/service/CaseService.java (1)
64-68:⚠️ Potential issue | 🟡 MinorDeduplicate filenames within the same upload batch.
Line 98 only seeds
existingfrom already-persisted files, andcreateTickethas no batch-level set. IffileNamescontains the same name twice, both paths can persist duplicateUploadedFilerows with the sames3Key.Proposed fix
caseEntity = caseRepository.save(caseEntity); if (request.getFileNames() != null){ + Set<String> seen = new java.util.HashSet<>(); for(String fName : request.getFileNames()){ - if (fName == null || fName.isBlank()) continue; + if (fName == null || fName.isBlank() || !seen.add(fName)) continue; caseEntity.getFiles().addAll(s3Service.createFile(caseEntity, fName)); } } @@ Set<String> existing = caseEntity.getFiles().stream() .map(UploadedFile::getFilename) .collect(Collectors.toSet()); for(String fName : request.getFileNames()){ - if (fName == null || fName.isBlank() || existing.contains(fName)) continue; + if (fName == null || fName.isBlank() || !existing.add(fName)) continue; caseEntity.getFiles().addAll(s3Service.createFile(caseEntity, fName)); } }Also applies to: 97-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/usercase/service/CaseService.java` around lines 64 - 68, The batch can contain duplicate file names so modify CaseService (in the createTicket flow) to deduplicate request.getFileNames() before calling s3Service.createFile: build a batch-level Set (e.g., LinkedHashSet to preserve order) from request.getFileNames(), skip null/blank entries, and then iterate only the unique names; also skip names already present in the existing persisted set (the same `existing` used to seed persisted files) and only call caseEntity.getFiles().addAll(s3Service.createFile(caseEntity, fName)) once per unique, non-duplicated name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/example/untitled/usercase/service/CaseService.java`:
- Around line 64-68: The batch can contain duplicate file names so modify
CaseService (in the createTicket flow) to deduplicate request.getFileNames()
before calling s3Service.createFile: build a batch-level Set (e.g.,
LinkedHashSet to preserve order) from request.getFileNames(), skip null/blank
entries, and then iterate only the unique names; also skip names already present
in the existing persisted set (the same `existing` used to seed persisted files)
and only call caseEntity.getFiles().addAll(s3Service.createFile(caseEntity,
fName)) once per unique, non-duplicated name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1995062-0ec7-4c70-8e29-9f85932a6bf1
📒 Files selected for processing (4)
src/main/java/org/example/untitled/s3/S3Service.javasrc/main/java/org/example/untitled/usercase/service/CaseService.javasrc/main/resources/static/js/script.jssrc/main/resources/templates/create_ticket.html
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/example/untitled/s3/S3Service.java
- src/main/resources/templates/create_ticket.html
- src/main/resources/static/js/script.js
# Conflicts: # src/main/resources/templates/userpage.html
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/org/example/untitled/usercase/service/CaseServiceTest.java (1)
145-235: Call-site updates LGTM; consider adding coverage for the newfileNamesbranches.The signature alignment is correct. As a follow-up, it would be worth adding tests for the newly introduced behavior that isn't currently exercised:
createTicketwith afileNameslist containingnull,""," "→ these entries are skipped andS3Service.createFileis not invoked for them.updateTicketre-submitting a filename already attached to the case →S3Service.createFileis not called for that entry (dedup againstcaseEntity.getFiles()).updateTicketwith a brand-new filename →s3Service.createFileis invoked and the newUploadedFileis appended.This would lock in the guards added in commits 36b4613…bbf7de9 against future regression.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/untitled/usercase/service/CaseServiceTest.java` around lines 145 - 235, Add unit tests covering the new fileNames branches: for CaseService.createTicket add a test that passes fileNames containing null, empty string, and whitespace-only entries and assert S3Service.createFile is never called for those; for CaseService.updateTicket add tests for (1) re-submitting an existing filename (ensure S3Service.createFile is not invoked and the existing UploadedFile list is unchanged) and (2) submitting a new filename (ensure S3Service.createFile is invoked and the returned UploadedFile is appended to caseEntity.getFiles()); reference the methods caseService.createTicket and caseService.updateTicket, the repository mocks used in existing tests (userRepository, caseRepository), and the S3Service.createFile and CaseEntity.getFiles behavior when asserting expectations.src/main/java/org/example/untitled/usercase/service/CaseService.java (1)
52-108: Createticket/updateTicket changes look good.The earlier concerns about blank/null filename entries and duplicate re-association on re-submit have been addressed (guards at lines 66 and 101, existing-filename set at 97-99). Saving the entity before the upload loop (line 63) is the right order since
S3Service.createFilenow depends oncaseEntity.getId().Note: once the
s3Keymismatch inS3Service.createFileis fixed (see the review onS3Service.java), consider deduping byUploadedFile::getS3Keyinstead ofgetFilenameat line 98 — two distinct uploads with the same display name are still distinct objects in S3.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/usercase/service/CaseService.java` around lines 52 - 108, Update the dedupe logic in updateTicket (and mirror in createTicket if applicable) to compare UploadedFile::getS3Key instead of UploadedFile::getFilename once you fix the s3Key generation in S3Service.createFile; specifically, after S3Service.createFile correctly sets UploadedFile.getS3Key, change the existing set construction that now collects UploadedFile::getFilename (in updateTicket around existing = caseEntity.getFiles()...) to collect UploadedFile::getS3Key and compare incoming file S3 keys rather than filenames before calling s3Service.createFile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/untitled/s3/S3Service.java`:
- Around line 95-105: The persisted s3Key in createFile doesn't match the actual
key used by generateS3PreUploadUrl; pick a single canonical key layout (prefer
Option A): modify generateS3PreUploadUrl to construct its fileIn with the case
prefix (e.g. "tickets/{caseId}/uploads/{UUID[:8]}-{fileName}") and ensure
createFile sets uploadedFile.setS3Key(...) to that exact same value (or pass
fileIn into createFile), update generateS3DownloadUrl/deleteFile to use
UploadedFile.getS3Key(), and sanitize/reject fileName values containing "/" or
".." before composing the key so clients cannot inject path segments.
---
Nitpick comments:
In `@src/main/java/org/example/untitled/usercase/service/CaseService.java`:
- Around line 52-108: Update the dedupe logic in updateTicket (and mirror in
createTicket if applicable) to compare UploadedFile::getS3Key instead of
UploadedFile::getFilename once you fix the s3Key generation in
S3Service.createFile; specifically, after S3Service.createFile correctly sets
UploadedFile.getS3Key, change the existing set construction that now collects
UploadedFile::getFilename (in updateTicket around existing =
caseEntity.getFiles()...) to collect UploadedFile::getS3Key and compare incoming
file S3 keys rather than filenames before calling s3Service.createFile.
In `@src/test/java/org/example/untitled/usercase/service/CaseServiceTest.java`:
- Around line 145-235: Add unit tests covering the new fileNames branches: for
CaseService.createTicket add a test that passes fileNames containing null, empty
string, and whitespace-only entries and assert S3Service.createFile is never
called for those; for CaseService.updateTicket add tests for (1) re-submitting
an existing filename (ensure S3Service.createFile is not invoked and the
existing UploadedFile list is unchanged) and (2) submitting a new filename
(ensure S3Service.createFile is invoked and the returned UploadedFile is
appended to caseEntity.getFiles()); reference the methods
caseService.createTicket and caseService.updateTicket, the repository mocks used
in existing tests (userRepository, caseRepository), and the S3Service.createFile
and CaseEntity.getFiles behavior when asserting expectations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d72c3c3-0808-454f-bbbe-65ac21099693
📒 Files selected for processing (6)
src/main/java/org/example/untitled/s3/S3Service.javasrc/main/java/org/example/untitled/usercase/controller/CaseController.javasrc/main/java/org/example/untitled/usercase/service/CaseService.javasrc/main/resources/templates/login.htmlsrc/main/resources/templates/userpage.htmlsrc/test/java/org/example/untitled/usercase/service/CaseServiceTest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/resources/templates/login.html
- src/main/resources/templates/userpage.html
# Conflicts: # src/main/resources/static/js/script.js # src/main/resources/templates/upload.html
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/org/example/untitled/usercase/service/CaseService.java (1)
70-75:⚠️ Potential issue | 🔴 CriticalBuild is broken:
fileNameis undefined increateTicket(and again inupdateTicket).The
fileNameparameter was removed from both method signatures, but the audit-log branch at lines 72–74 (and the symmetric one at lines 110–112) still references it. This produces the compile errors reported byci.yml:COMPILATION ERROR: cannot find symbol. Variable 'fileName' is not defined in class org.example.untitled.usercase.service.CaseService.Drive the FILE_UPLOADED audit event off
request.getFileNames()(ideally per successfully attached file, inside the loop) so the audit trail reflects every uploaded attachment.🛠️ Proposed fix for both methods
CaseEntity saved = caseRepository.save(caseEntity); auditLogService.log(AuditAction.CASE_CREATED, owner.getId(), saved.getId()); - if (fileName != null && !fileName.isBlank()) { - auditLogService.log(AuditAction.FILE_UPLOADED, owner.getId(), saved.getId()); + if (!saved.getFiles().isEmpty()) { + auditLogService.log(AuditAction.FILE_UPLOADED, owner.getId(), saved.getId()); } return CaseMapper.toDto(saved); }CaseEntity saved = caseRepository.save(caseEntity); auditLogService.log(AuditAction.CASE_UPDATED, caseEntity.getOwner().getId(), saved.getId()); - if (fileName != null && !fileName.isBlank()) { - auditLogService.log(AuditAction.FILE_UPLOADED, caseEntity.getOwner().getId(), saved.getId()); + if (!saved.getFiles().isEmpty()) { + auditLogService.log(AuditAction.FILE_UPLOADED, caseEntity.getOwner().getId(), saved.getId()); } return CaseMapper.toDto(saved); }Even better, log once per attached file inside the
forloop so the audit count is accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/usercase/service/CaseService.java` around lines 70 - 75, The code references an undefined variable fileName in createTicket and updateTicket; replace that branch with logic that iterates request.getFileNames() after saving (e.g., in createTicket and updateTicket where CaseEntity saved = caseRepository.save(caseEntity) and auditLogService.log(...) are used), and call auditLogService.log(AuditAction.FILE_UPLOADED, owner.getId(), saved.getId()) once per filename inside the loop so each attached file produces a FILE_UPLOADED audit entry rather than referencing the removed fileName parameter.src/main/java/org/example/untitled/s3/S3RestController.java (1)
10-33:⚠️ Potential issue | 🟡 MinorEndpoint restructuring looks clean; consider ownership check for
caseId.Class-level mapping plus shortened per-method paths are consistent with the updated frontend calls. One thing worth considering:
caseIdis accepted as an unauthenticated request parameter and pushed straight into the S3 key, so any authenticated user can request a presigned PUT undertickets/{anyCaseId}/uploads/.... The uploaded object will never actually be linked to that other case (attachment happens inCaseServicewith owner checks), but it still lets users pollute the S3 path namespace of cases they don't own.If that's undesirable, validate in the controller that
caseId == nullorcaseIdbelongs to the current user before callings3Service.generateS3PreUploadUrl(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/s3/S3RestController.java` around lines 10 - 33, The getUploadUrl endpoint in S3RestController accepts a caseId and calls s3Service.generateS3PreUploadUrl(...) without verifying ownership; change getUploadUrl to validate that caseId is either null or owned by the current user before calling S3Service: inject or use an existing CaseService (or SecurityContext principal) to check ownership via a method like CaseService.isOwner(caseId, currentUserId) and return an HTTP 403 (or appropriate error) when the caller is not the owner; keep the call to s3Service.generateS3PreUploadUrl(...) only after the ownership check passes.src/main/resources/static/js/script.js (1)
82-92:⚠️ Potential issue | 🔴 CriticalInverted confirm logic — delete only runs when the user clicks Cancel.
window.confirm(...)returnstruefor OK andfalsefor Cancel.if (!window.confirm(...))therefore fires the DELETE only when the user cancels, and does nothing when they actually confirm. Drop the!:🛠️ Proposed fix
- if (!window.confirm(fileName + " will be deleted! Are you sure?")) { + if (window.confirm(fileName + " will be deleted! Are you sure?")) { const res = await apiReq(`/tickets/upload/api/files/delete-url?fileName=${encodeURIComponent(fileName)}`, {method: 'DELETE'}); if(res.ok){ await fetchAFile(); } else { status.innerText= 'Error: ' + res.status; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/script.js` around lines 82 - 92, The confirm logic in deleteFile is inverted so the DELETE runs only when the user cancels; change the condition to run the delete when window.confirm(...) returns true (i.e., remove the negation) so that the apiReq DELETE call and subsequent fetchAFile() happen only after user confirms; keep the same API call (apiReq(`/tickets/upload/api/files/delete-url?fileName=${encodeURIComponent(fileName)}`, {method: 'DELETE'})), error handling (status.innerText = 'Error: ' + res.status) and references to status and fetchAFile unchanged.
♻️ Duplicate comments (1)
src/main/resources/static/js/script.js (1)
51-79:⚠️ Potential issue | 🟠 MajorUpload failures still let the form submit, and the success-status line runs after navigation.
Flagged previously and partially addressed (button is now re-enabled), but the core problem remains:
- On
putRes.ok === false(L64–68) and in thecatch(L69–74), the loop continues and the terminaldocument.querySelector('form').submit()at L79 still fires. The ticket is created referencing only the files that succeeded, and per-file upload errors are silently dropped — S3 and DB rows desync with no signal to the user.status.innerText = "All uploads completed."at L78 runs unconditionally (even when some uploads failed), and because L79 navigates away, the user never sees it anyway.Track a failure flag and bail out of
form.submit()when set; also set the status before submit.♻️ Proposed fix
async function uploadNewFile(){ const submitBtn = document.getElementById('submitBtn'); submitBtn.disabled = true; const input = document.getElementById('fileInput'); const status = document.getElementById('status'); if (input.files.length === 0){ document.querySelector('form').submit(); return; } const filesToUpload = Array.from(input.files); input.value = null; submitBtn.innerText = "Uploading..."; + let uploadFailed = false; for (let i = 0; i < filesToUpload.length; i++) { const file = filesToUpload[i]; try { ... if (putRes.ok) { ... } else { status.innerText = `Failed to upload ${file.name}. Status: ${putRes.status}`; - submitBtn.disabled = false; - submitBtn.innerText = "Create Ticket"; + uploadFailed = true; + break; } } catch (error) { console.error(error); status.innerText = `Error uploading ${file.name}: ${error.message}`; - submitBtn.disabled = false; - submitBtn.innerText = "Create Ticket"; + uploadFailed = true; + break; } } submitBtn.disabled = false; submitBtn.innerText = "Create Ticket"; + if (uploadFailed) return; status.innerText = "All uploads completed."; document.querySelector('form').submit(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/script.js` around lines 51 - 79, The upload loop currently continues on per-file failures (putRes.ok false or catch) and always executes status.innerText = "All uploads completed." and document.querySelector('form').submit(), causing silent failures; modify the handler that performs uploads (the block using putRes, the catch, submitBtn, and status) to track a boolean failure flag (e.g., uploadFailed), set it to true when putRes.ok is false or an exception occurs, stop further uploads or break out of the loop when failure is detected, ensure status.innerText reflects failure before any navigation, and only call document.querySelector('form').submit() when uploadFailed is false; also keep submitBtn state consistent (re-enable on failure and disable while uploading) so the user sees the error and the form is not submitted on partial failures.
🧹 Nitpick comments (1)
src/main/java/org/example/untitled/s3/S3RestController.java (1)
30-52: Nit: prefer parameterized logging over string concatenation for user input.Lines 30, 38, 45 concatenate user-controlled
fileNameinto the log message; line 52 already uses SLF4J's{}placeholder, which is the preferred pattern and avoids CRLF log-injection from attacker-controlled input.- log.info("uploading file " + fileName); + log.info("uploading file {}", fileName);…and similarly for
downloadFileanddeleteFile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/untitled/s3/S3RestController.java` around lines 30 - 52, Replace the string-concatenation logging in S3RestController with parameterized SLF4J logging to avoid log-injection: change log.info("uploading file " + fileName) in the upload method to log.info("uploading file {}", fileName), change log.info("Trying to download file " + fileName) in downloadFile to log.info("Trying to download file {}", fileName), and change log.info("Deleting file " + fileName) in deleteFile to log.info("Deleting file {}", fileName); keep the existing parameterized log in uploadCallback as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/untitled/usercase/service/CaseService.java`:
- Around line 99-107: The deduplication never matches because existing is built
from UploadedFile::getFilename (just the last path segment) while
request.getFileNames() contains full S3 keys; update the dedupe to compare S3
keys instead — i.e., when constructing existing from caseEntity.getFiles() use
UploadedFile.getS3Key (or otherwise normalize both sides to full S3 keys), then
keep the rest of the loop (calling s3Service.createFile) unchanged; ensure
references to request.getFileNames(), caseEntity.getFiles(), UploadedFile (s3Key
vs filename) and S3Service.createFile are the points you change.
---
Outside diff comments:
In `@src/main/java/org/example/untitled/s3/S3RestController.java`:
- Around line 10-33: The getUploadUrl endpoint in S3RestController accepts a
caseId and calls s3Service.generateS3PreUploadUrl(...) without verifying
ownership; change getUploadUrl to validate that caseId is either null or owned
by the current user before calling S3Service: inject or use an existing
CaseService (or SecurityContext principal) to check ownership via a method like
CaseService.isOwner(caseId, currentUserId) and return an HTTP 403 (or
appropriate error) when the caller is not the owner; keep the call to
s3Service.generateS3PreUploadUrl(...) only after the ownership check passes.
In `@src/main/java/org/example/untitled/usercase/service/CaseService.java`:
- Around line 70-75: The code references an undefined variable fileName in
createTicket and updateTicket; replace that branch with logic that iterates
request.getFileNames() after saving (e.g., in createTicket and updateTicket
where CaseEntity saved = caseRepository.save(caseEntity) and
auditLogService.log(...) are used), and call
auditLogService.log(AuditAction.FILE_UPLOADED, owner.getId(), saved.getId())
once per filename inside the loop so each attached file produces a FILE_UPLOADED
audit entry rather than referencing the removed fileName parameter.
In `@src/main/resources/static/js/script.js`:
- Around line 82-92: The confirm logic in deleteFile is inverted so the DELETE
runs only when the user cancels; change the condition to run the delete when
window.confirm(...) returns true (i.e., remove the negation) so that the apiReq
DELETE call and subsequent fetchAFile() happen only after user confirms; keep
the same API call
(apiReq(`/tickets/upload/api/files/delete-url?fileName=${encodeURIComponent(fileName)}`,
{method: 'DELETE'})), error handling (status.innerText = 'Error: ' + res.status)
and references to status and fetchAFile unchanged.
---
Duplicate comments:
In `@src/main/resources/static/js/script.js`:
- Around line 51-79: The upload loop currently continues on per-file failures
(putRes.ok false or catch) and always executes status.innerText = "All uploads
completed." and document.querySelector('form').submit(), causing silent
failures; modify the handler that performs uploads (the block using putRes, the
catch, submitBtn, and status) to track a boolean failure flag (e.g.,
uploadFailed), set it to true when putRes.ok is false or an exception occurs,
stop further uploads or break out of the loop when failure is detected, ensure
status.innerText reflects failure before any navigation, and only call
document.querySelector('form').submit() when uploadFailed is false; also keep
submitBtn state consistent (re-enable on failure and disable while uploading) so
the user sees the error and the form is not submitted on partial failures.
---
Nitpick comments:
In `@src/main/java/org/example/untitled/s3/S3RestController.java`:
- Around line 30-52: Replace the string-concatenation logging in
S3RestController with parameterized SLF4J logging to avoid log-injection: change
log.info("uploading file " + fileName) in the upload method to
log.info("uploading file {}", fileName), change log.info("Trying to download
file " + fileName) in downloadFile to log.info("Trying to download file {}",
fileName), and change log.info("Deleting file " + fileName) in deleteFile to
log.info("Deleting file {}", fileName); keep the existing parameterized log in
uploadCallback as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c0c8fdf-e437-4fb7-aa5d-7bbd05feaa08
📒 Files selected for processing (6)
src/main/java/org/example/untitled/s3/S3RestController.javasrc/main/java/org/example/untitled/s3/S3Service.javasrc/main/java/org/example/untitled/usercase/service/CaseService.javasrc/main/resources/static/js/script.jssrc/main/resources/templates/create_ticket.htmlsrc/main/resources/templates/upload.html
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/templates/upload.html
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/resources/templates/create_ticket.html
apaegs
approved these changes
Apr 27, 2026
This was referenced Apr 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Chores
/tickets/upload/api/files/namespace