Enhancement/file upload additions#141
Merged
Merged
Conversation
…tickets and edit tickets. Added repository with logic to get files from db and connect it to the S3 storage
# Conflicts: # src/main/java/org/example/untitled/usercase/controller/CaseController.java # src/main/resources/templates/ticket.html
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR refactors the file upload workflow by migrating the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/untitled/usercase/controller/CaseController.java (1)
42-46: 🛠️ Refactor suggestion | 🟠 Major
S3Serviceis injected but never stored or used — dead parameter.The constructor accepts
S3Service s3Servicebut the body doesn't assign it to a field, and noS3Servicefield exists in the class. Spring will still instantiateS3Serviceand pass it in, but the controller does nothing with it. Either remove the parameter (and the import on line 4) or wire it through if the new files flow was meant to call it directly.♻️ If S3Service isn't needed here, remove it
-import org.example.untitled.s3.S3Service; ... - public CaseController(CaseService caseService, CommentService commentService, S3Service s3Service, AuditLogService auditLogService) { + public CaseController(CaseService caseService, CommentService commentService, AuditLogService auditLogService) { this.caseService = caseService; this.commentService = commentService; this.auditLogService = auditLogService; }Otherwise, add
private final S3Service s3Service;andthis.s3Service = s3Service;.🤖 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/controller/CaseController.java` around lines 42 - 46, The CaseController constructor declares an unused S3Service parameter (S3Service s3Service) which is never stored or used; either remove the S3Service parameter (and its import) from the CaseController constructor signature if it's not needed, or add a private final S3Service s3Service field to the CaseController class and assign it in the constructor with this.s3Service = s3Service so the injected service is wired for later use.
🧹 Nitpick comments (3)
src/main/java/org/example/untitled/s3/S3Service.java (1)
103-112: Method name now overstates what it does — consider renaming.
createFileno longer persists; it just constructs anUploadedFile.buildUploadedFile(ortoUploadedFile) would more accurately reflect the contract and prevent future callers from assuming the entity is saved. Optional but improves clarity.🤖 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 103 - 112, The method createFile in S3Service constructs but does not persist an UploadedFile, so rename it to a clearer name like buildUploadedFile or toUploadedFile to reflect that it only builds the entity; update the method signature/name (public UploadedFile createFile(...) -> public UploadedFile buildUploadedFile(...) or toUploadedFile(...)) and any call sites that reference createFile, and adjust related JavaDoc/comments to indicate the method only constructs the UploadedFile (reference symbols: S3Service.createFile, UploadedFile).src/main/java/org/example/untitled/usercase/service/CaseService.java (1)
187-188: Redundant.stream().toList().Both
getTicketFiles(line 187–188) andgetUserFiles(line 202) call.stream().toList()on a value that is already aList<UploadedFile>. You can return the list directly. Also, sinceCaseEntityalready exposes itsfilescollection (you populate it increateTicket/updateTicket),getTicketFilescould simplyreturn caseEntity.getFiles()(subject to fetch/lazy-init within the transactional boundary), avoiding an extra query.♻️ Minimal cleanup
- return uploadedFileRepository.associatedCaseEntity(caseEntity).stream() - .toList(); + return uploadedFileRepository.findByAssociatedCase(caseEntity); @@ - return uploadedFileRepository.getUploadedFilesByUploadedBy(owner).stream().toList(); + return uploadedFileRepository.getUploadedFilesByUploadedBy(owner);🤖 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 187 - 188, The code is calling .stream().toList() redundantly on a List<UploadedFile>; update getTicketFiles and getUserFiles to return the existing List directly instead of streaming it. Specifically, replace the uploadedFileRepository.associatedCaseEntity(caseEntity).stream().toList() return with the direct List return (or better, return caseEntity.getFiles() from getTicketFiles since createTicket/updateTicket already populate CaseEntity.files and this avoids an extra repository query), keeping transactional boundaries to ensure lazy-loaded collections are initialized.src/main/resources/templates/upload.html (1)
27-37: Consider an empty-state row for when${files}is empty.The previous template had an empty-state conditional; this version unconditionally renders the table, so when a user has no uploads they see only headers with no body. Consider adding a
th:if/th:unlessrow to communicate the empty state.♻️ Example
<tbody> + <tr th:if="${`#lists.isEmpty`(files)}"> + <td colspan="2">No files uploaded yet.</td> + </tr> <tr th:each="file : ${files}">🤖 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 27 - 37, The table body currently only renders rows for entries in th:each="file : ${files}" so when files is empty users see only headers; add an empty-state row inside the same <tbody> after the th:each loop that uses a Thymeleaf conditional (e.g. th:if="${`#lists.isEmpty`(files)}" or th:unless="${files}") to render a single <tr> with a descriptive message like "No uploads yet" and a colspan matching the table columns (so it spans the filename and actions columns) — locate the existing th:each="file : ${files}" block and add this conditional row next to it, leaving the downloadFile(...) button and file rows unchanged.
🤖 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 27-30: The S3Service constructor takes an UploadedFileRepository
parameter that's never stored or used; either remove that parameter and its
import from the S3Service constructor, or assign it to a private field and use
it inside createFile to persist the uploaded metadata so callers (e.g.,
CaseService) no longer need to call uploadedFileRepository.save(...) themselves;
locate the S3Service(S3Client s3Client, S3Presigner s3Presigner,
UploadedFileRepository uploadedFileRepository) constructor and either drop the
UploadedFileRepository parameter and import, or add a private final
UploadedFileRepository uploadedFileRepository field, set
this.uploadedFileRepository = uploadedFileRepository in the constructor, and
update createFile to call uploadedFileRepository.save(...) with the created
UploadedFile entity before returning.
In `@src/main/java/org/example/untitled/user/controller/UserController.java`:
- Around line 91-95: The getUserTickets method (UserController.getUserTickets)
lacks the defensive null check and role restriction present in sibling
userLanding; add a null check for the `@AuthenticationPrincipal` UserDetails (same
behavior as userLanding, e.g., return the same redirect when null) and annotate
the method with `@PreAuthorize`("hasRole('USER')") so only USER-role principals
can access it; keep the rest of the method logic (model.addAttribute(...),
return "upload") unchanged.
In
`@src/main/java/org/example/untitled/usercase/repository/UploadedFileRepository.java`:
- Line 15: The repository method
UploadedFileRepository#getUploadedFilesByFilename currently returns a single
UploadedFile but queries on UploadedFile.filename which is not unique; change
the signature to return List<UploadedFile> (or Optional<UploadedFile> only if
you enforce uniqueness) and update any callers to handle a collection (e.g.,
List<UploadedFile> getUploadedFilesByFilename(String filename)); also review
usages of getUploadedFilesByFilename and remove the method if it's unused.
- Line 14: The repository method name associatedCaseEntity(CaseEntity
caseEntity) is not a valid Spring Data derived query; rename it to
findByAssociatedCaseEntity(CaseEntity caseEntity) (or annotate the existing
method with an appropriate `@Query`) so Spring can resolve the query, and update
the call site in CaseService.getTicketFiles to invoke
findByAssociatedCaseEntity(...) instead of associatedCaseEntity(...); ensure the
method signature still returns List<UploadedFile> and uses the existing
associatedCaseEntity entity field.
In `@src/main/java/org/example/untitled/usercase/service/CaseService.java`:
- Around line 68-75: The code double-persists UploadedFile: you call
uploadedFileRepository.save(uploadFile) and then add the file to
caseEntity.getFiles() and later call caseRepository.save(caseEntity), while the
CaseEntity `@OneToMany` mapping uses cascade = CascadeType.ALL; remove the
explicit repository save calls (uploadedFileRepository.save(uploadFile)) in the
file-adding loop in CaseService (the shown block and the similar loop in
updateTicket) so new UploadedFile instances are persisted via the caseEntity
cascade when caseRepository.save(caseEntity) is called.
In `@src/main/java/org/example/untitled/usercase/UploadedFile.java`:
- Around line 18-19: The `@Column` on the s3Key field in class UploadedFile
currently omits the nullability constraint; update the annotation on the s3Key
field (private String s3Key) to include nullable = false so the JPA mapping
matches the migration V4__create_uploaded_file_table.sql (s3key VARCHAR(255) NOT
NULL) and prevents a schema validation mismatch and allows JPA to enforce
non-null at the ORM level.
In `@src/main/resources/templates/edit_ticket.html`:
- Line 32: The Edit Ticket page's Save Changes button is being grabbed by
uploadNewFile() via id="submitBtn" and its label gets overwritten; fix by either
removing/renaming the id on the button in edit_ticket.html (so uploadNewFile()
no longer selects it) or by adding a data-default-label attribute to the button
and updating uploadNewFile() in src/main/resources/static/js/script.js to read
submitBtn.dataset.defaultLabel (or capture submitBtn.innerText at function
start) and use that value instead of the hardcoded "Create Ticket" when
restoring the label; update references to submitBtn and uploadNewFile()
accordingly.
In `@src/main/resources/templates/ticket.html`:
- Around line 48-58: The markup uses <tr>/<td> inside a <div>, which is invalid;
wrap the iteration in a proper table structure (e.g., <table> with <tbody>) and
move the th:each="file : ${files}" onto the <tr> inside that tbody so table rows
render correctly; update Thymeleaf expressions from
file.getFilename()/file.getS3Key() to property-style file.filename and
file.s3Key for idiomatic access, keep th:data-file on the button, and add
type="button" to the Download button to avoid accidental form submission; verify
the downloadFile(...) call remains unchanged.
In `@src/main/resources/templates/upload.html`:
- Around line 39-42: Remove the empty anchor and the stray closing main tag to
fix the invalid markup: delete the orphaned <p><a th:href="@{/user}"></a></p>
element (or replace it with visible content only if you intend a second link)
and remove the stray </main> closing tag so the template’s tags properly match;
target the fragment containing the empty anchor and the lone </main> near the
end of the file (lines showing the <p><a th:href="@{/user}"> and the </main>).
---
Outside diff comments:
In `@src/main/java/org/example/untitled/usercase/controller/CaseController.java`:
- Around line 42-46: The CaseController constructor declares an unused S3Service
parameter (S3Service s3Service) which is never stored or used; either remove the
S3Service parameter (and its import) from the CaseController constructor
signature if it's not needed, or add a private final S3Service s3Service field
to the CaseController class and assign it in the constructor with this.s3Service
= s3Service so the injected service is wired for later use.
---
Nitpick comments:
In `@src/main/java/org/example/untitled/s3/S3Service.java`:
- Around line 103-112: The method createFile in S3Service constructs but does
not persist an UploadedFile, so rename it to a clearer name like
buildUploadedFile or toUploadedFile to reflect that it only builds the entity;
update the method signature/name (public UploadedFile createFile(...) -> public
UploadedFile buildUploadedFile(...) or toUploadedFile(...)) and any call sites
that reference createFile, and adjust related JavaDoc/comments to indicate the
method only constructs the UploadedFile (reference symbols:
S3Service.createFile, UploadedFile).
In `@src/main/java/org/example/untitled/usercase/service/CaseService.java`:
- Around line 187-188: The code is calling .stream().toList() redundantly on a
List<UploadedFile>; update getTicketFiles and getUserFiles to return the
existing List directly instead of streaming it. Specifically, replace the
uploadedFileRepository.associatedCaseEntity(caseEntity).stream().toList() return
with the direct List return (or better, return caseEntity.getFiles() from
getTicketFiles since createTicket/updateTicket already populate CaseEntity.files
and this avoids an extra repository query), keeping transactional boundaries to
ensure lazy-loaded collections are initialized.
In `@src/main/resources/templates/upload.html`:
- Around line 27-37: The table body currently only renders rows for entries in
th:each="file : ${files}" so when files is empty users see only headers; add an
empty-state row inside the same <tbody> after the th:each loop that uses a
Thymeleaf conditional (e.g. th:if="${`#lists.isEmpty`(files)}" or
th:unless="${files}") to render a single <tr> with a descriptive message like
"No uploads yet" and a colspan matching the table columns (so it spans the
filename and actions columns) — locate the existing th:each="file : ${files}"
block and add this conditional row next to it, leaving the downloadFile(...)
button and file rows unchanged.
🪄 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: fe6b2942-b8be-4a8b-a536-ff5d82c4fd0b
📒 Files selected for processing (13)
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/user/controller/UserController.javasrc/main/java/org/example/untitled/usercase/UploadedFile.javasrc/main/java/org/example/untitled/usercase/controller/CaseController.javasrc/main/java/org/example/untitled/usercase/repository/UploadedFileRepository.javasrc/main/java/org/example/untitled/usercase/service/CaseService.javasrc/main/resources/static/js/script.jssrc/main/resources/templates/edit_ticket.htmlsrc/main/resources/templates/ticket.htmlsrc/main/resources/templates/upload.htmlsrc/main/resources/templates/userpage.html
💤 Files with no reviewable changes (1)
- src/main/java/org/example/untitled/s3/S3Controller.java
# Conflicts: # src/main/java/org/example/untitled/usercase/controller/CaseController.java # src/main/java/org/example/untitled/usercase/service/CaseService.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/untitled/usercase/controller/CaseController.java (1)
278-288:⚠️ Potential issue | 🟡 MinorMissing
filesmodel attribute in error path.When
addCommenthas validation errors, the "ticket" view is rendered without the"files"attribute. SinceshowTicketDetails(line 116) adds this attribute and the template expects it, the error path will cause inconsistent rendering or a template error.🐛 Proposed fix
if (bindingResult.hasErrors()) { List<AuditLog> auditLogs = auditLogService.getLogsForCase(id); Map<Long, String> auditUserMap = buildAuditUserMap(auditLogs); + List<UploadedFile> files = caseService.getTicketFiles(id); model.addAttribute("ticket", ticket); model.addAttribute("comments", commentService.getCommentsByTicketId(id)); model.addAttribute("canClose", perms.canClose()); model.addAttribute("canComment", perms.canComment()); model.addAttribute("auditLogs", auditLogs); model.addAttribute("auditUserMap", auditUserMap); + model.addAttribute("files", files); return "ticket"; }🤖 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/controller/CaseController.java` around lines 278 - 288, The error branch in addComment (the bindingResult.hasErrors() block) fails to add the "files" model attribute expected by the "ticket" view; update that block to populate the same "files" attribute as showTicketDetails does (e.g., call the same file retrieval used by showTicketDetails or a FileService method like getFilesByTicketId(id)) before returning "ticket", so the template has a consistent model (refer to bindingResult.hasErrors(), showTicketDetails, and buildAuditUserMap to locate the code).
🧹 Nitpick comments (1)
src/main/java/org/example/untitled/usercase/service/CaseService.java (1)
182-189: Simplify: remove redundant.stream().toList().If
uploadedFileRepository.associatedCaseEntity()already returns aList<UploadedFile>, the.stream().toList()call is unnecessary.♻️ Suggested simplification
public List<UploadedFile> getTicketFiles(long id) { CaseEntity caseEntity = caseRepository.findById(id) .orElseThrow( () -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found: " + id)); - return uploadedFileRepository.associatedCaseEntity(caseEntity).stream() - .toList(); + return uploadedFileRepository.associatedCaseEntity(caseEntity); }🤖 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 182 - 189, The getTicketFiles method uses uploadedFileRepository.associatedCaseEntity(caseEntity).stream().toList() redundantly; if associatedCaseEntity already returns a List<UploadedFile>, remove the unnecessary stream() call and return the list directly from getTicketFiles (update the return statement referencing getTicketFiles, uploadedFileRepository.associatedCaseEntity, and CaseEntity accordingly), leaving the existing caseRepository.findById(...) exception behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/org/example/untitled/usercase/controller/CaseController.java`:
- Around line 278-288: The error branch in addComment (the
bindingResult.hasErrors() block) fails to add the "files" model attribute
expected by the "ticket" view; update that block to populate the same "files"
attribute as showTicketDetails does (e.g., call the same file retrieval used by
showTicketDetails or a FileService method like getFilesByTicketId(id)) before
returning "ticket", so the template has a consistent model (refer to
bindingResult.hasErrors(), showTicketDetails, and buildAuditUserMap to locate
the code).
---
Nitpick comments:
In `@src/main/java/org/example/untitled/usercase/service/CaseService.java`:
- Around line 182-189: The getTicketFiles method uses
uploadedFileRepository.associatedCaseEntity(caseEntity).stream().toList()
redundantly; if associatedCaseEntity already returns a List<UploadedFile>,
remove the unnecessary stream() call and return the list directly from
getTicketFiles (update the return statement referencing getTicketFiles,
uploadedFileRepository.associatedCaseEntity, and CaseEntity accordingly),
leaving the existing caseRepository.findById(...) exception behavior intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a87622b-4c55-4f95-abd3-573e39ec26a9
📒 Files selected for processing (4)
src/main/java/org/example/untitled/user/controller/UserController.javasrc/main/java/org/example/untitled/usercase/controller/CaseController.javasrc/main/java/org/example/untitled/usercase/service/CaseService.javasrc/main/resources/templates/ticket.html
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/templates/ticket.html
- src/main/java/org/example/untitled/user/controller/UserController.java
apaegs
approved these changes
Apr 28, 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
Release Notes
New Features
Bug Fixes
UI/UX Improvements