Feature/error page#38
Merged
Merged
Conversation
- Add Thymeleaf error template - Update GlobalExceptionHandler to render error view instead of @responsebody - Fix validation logic in visa application form - Add TODOs for dynamic dashboard routing based on user roles
- update processUpdate_Unauthorized_ShouldReturnForbiddenStatus to return new error page instead of earlier @responsebody
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 34 seconds. ⌛ 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 (2)
📝 WalkthroughWalkthroughGlobal exception handling was split: controller-layer errors now render a Thymeleaf error view populated with Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller
participant RestHandler as RestExceptionHandler
participant CtrlHandler as GlobalExceptionHandler
participant Model
participant ViewRenderer as Thymeleaf
participant Browser
Client->>Controller: HTTP request (web or API)
Controller->>Controller: process / encounter exception
alt API request
Controller-->>RestHandler: Exception thrown
RestHandler->>Client: HTTP JSON ErrorResponse (status + message)
else Controller (MVC) request
Controller-->>CtrlHandler: Exception thrown
CtrlHandler->>Model: set errorTitle / errorMessage
CtrlHandler->>ViewRenderer: return "error/error" view
ViewRenderer->>Browser: Rendered HTML error page
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 4
🧹 Nitpick comments (1)
src/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.java (1)
22-23: Repeated TODO across all handlers — extract shared setup.All four handlers duplicate the same three model attributes and the same TODO about
dashboardUrl. Extract a private helper likepopulateErrorModel(Model model, String title, String message)to reduce duplication and ensure the futuredashboardUrlchange lands in a single place.Also applies to: 34-35, 46-47, 58-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.java` around lines 22 - 23, Multiple exception handlers in GlobalExceptionHandler duplicate setting the same Model attributes and the TODO for dashboardUrl; create a private helper method (e.g., populateErrorModel(Model model, String title, String message)) and have each handler call it instead of repeating the three model.addAttribute calls and the dashboardUrl TODO. Update handlers like handleNotFoundException, handleBadRequestException, handleUnauthorizedException, and handleServerError (use actual method names present) to call populateErrorModel(model, "Title", ex.getMessage()) and then return the view; implement dashboardUrl resolution inside populateErrorModel by fetching the role from SecurityContext when ready so future changes are centralized.
🤖 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/visacasemanagementsystem/exception/GlobalExceptionHandler.java`:
- Around line 18-26: Update the exception handlers (notably
handleUnauthorizedException, the handlers for
EntityNotFoundException/ResourceNotFoundException and IllegalArgumentException)
to stop putting raw exception.getMessage() into the model for user-facing pages;
instead set a sanitized/static message (e.g. "Access denied", "Resource not
found", "Invalid request") in the model and add the detailed exception message
and stacktrace to server logs at warn/info level; also ensure handlers other
than handleAllUncaughtErrors (the 403/404/400 handlers) log the exception with
context so diagnostics are possible, and keep any detailed messages out of the
rendered model to prevent info leakage.
- Line 12: GlobalExceptionHandler is currently annotated with
`@ControllerAdvice`(annotations = Controller.class) and returns view names/Model,
which will render HTML for exceptions thrown by REST endpoints like
CommentService's IllegalArgumentException and ResourceNotFoundException; add a
separate REST-focused handler (e.g., annotate a new class with
`@RestControllerAdvice` or add a `@ControllerAdvice` targeting REST controllers)
that catches IllegalArgumentException and ResourceNotFoundException and returns
ResponseEntity<JsonError> (JSON payload with fields like timestamp, status,
error, message, path) instead of view names; locate GlobalExceptionHandler and
create a new class (or overload) named e.g., RestGlobalExceptionHandler with
methods handleIllegalArgumentException and handleResourceNotFoundException that
produce ResponseEntity with proper HTTP status codes and JSON bodies for REST
clients.
In `@src/main/resources/templates/error/error.html`:
- Around line 82-85: The dashboard link in the error.html template points to the
wrong path; update the anchor href from "/visa/dashboard" to "/visas/dashboard"
to match the VisaViewController mapping (VisaViewController -> /visas) and
remove or update the TODO comment accordingly; ensure the <a class="btn-back">
element's href is corrected so the link doesn't 404 (later, when wiring
authentication, pull currentUserId from the security context as noted).
- Around line 87-89: The template currently shows a hardcoded Reference ID;
update the exception handlers in GlobalExceptionHandler (e.g., in each
`@ExceptionHandler` method) to generate a correlation id
(UUID.randomUUID().toString()), log it with the exception (e.g., log.error("Ref
{}: ", refId, ex)), and add it to the model as "referenceId"; then change the
template's span to render that model attribute (or remove the entire footer-note
if you prefer not to show IDs). Use the model attribute name "referenceId" so
the template and GlobalExceptionHandler align.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.java`:
- Around line 22-23: Multiple exception handlers in GlobalExceptionHandler
duplicate setting the same Model attributes and the TODO for dashboardUrl;
create a private helper method (e.g., populateErrorModel(Model model, String
title, String message)) and have each handler call it instead of repeating the
three model.addAttribute calls and the dashboardUrl TODO. Update handlers like
handleNotFoundException, handleBadRequestException, handleUnauthorizedException,
and handleServerError (use actual method names present) to call
populateErrorModel(model, "Title", ex.getMessage()) and then return the view;
implement dashboardUrl resolution inside populateErrorModel by fetching the role
from SecurityContext when ready so future changes are centralized.
🪄 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: 1fc70d5e-7a76-4a9e-b1d2-4a10b47ce664
📒 Files selected for processing (4)
src/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.javasrc/main/resources/templates/error/error.htmlsrc/main/resources/templates/visa/apply-form.htmlsrc/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
Comment on lines
+18
to
26
| public String handleUnauthorizedException(UnauthorizedException exception, Model model) { | ||
| model.addAttribute("errorMessage", exception.getMessage()); | ||
| model.addAttribute("errorTitle", "⚠️Access Denied."); | ||
|
|
||
| // TODO: Fetch user role from SecurityContext and add correct dashboard URL to model | ||
| // model.addAttribute("dashboardUrl", determineDashboardUrl()); | ||
|
|
||
| return "error/error"; | ||
| } |
There was a problem hiding this comment.
Potential info leakage via exception.getMessage() on the error page.
For UnauthorizedException, EntityNotFoundException/ResourceNotFoundException, and IllegalArgumentException, the raw exception message is rendered to end users. This is fine when messages are curated (as in VisaService.updateVisa), but EntityNotFoundException from JPA can contain internal details (entity class, id lookups, etc.). Consider sanitizing or using a static message for the framework-thrown variants, keeping the detailed message only in logs.
Also note: none of these handlers log the exception except handleAllUncaughtErrors. A 403/404/400 with no server-side log makes diagnosis harder — at minimum log at warn/info.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.java`
around lines 18 - 26, Update the exception handlers (notably
handleUnauthorizedException, the handlers for
EntityNotFoundException/ResourceNotFoundException and IllegalArgumentException)
to stop putting raw exception.getMessage() into the model for user-facing pages;
instead set a sanitized/static message (e.g. "Access denied", "Resource not
found", "Invalid request") in the model and add the detailed exception message
and stacktrace to server logs at warn/info level; also ensure handlers other
than handleAllUncaughtErrors (the 403/404/400 handlers) log the exception with
context so diagnostics are possible, and keep any detailed messages out of the
rendered model to prevent info leakage.
Contributor
Author
There was a problem hiding this comment.
Do you have any suggestions how I can implement this?
There was a problem hiding this comment.
@eeebbaandersson, here's a concrete implementation that addresses both concerns — sanitizing user-facing messages and adding logging:
`@ControllerAdvice`(annotations = Controller.class)
`@Slf4j`
public class GlobalExceptionHandler {
`@ResponseStatus`(value = HttpStatus.FORBIDDEN)
`@ExceptionHandler`(UnauthorizedException.class)
public String handleUnauthorizedException(UnauthorizedException exception, Model model) {
log.warn("Access denied: {}", exception.getMessage());
model.addAttribute("errorMessage", "You do not have permission to perform this action.");
model.addAttribute("errorTitle", "⚠️Access Denied.");
return "error/error";
}
`@ResponseStatus`(value = HttpStatus.NOT_FOUND)
`@ExceptionHandler`({EntityNotFoundException.class, ResourceNotFoundException.class})
public String handleNotFoundException(RuntimeException exception, Model model) {
log.warn("Resource not found: {}", exception.getMessage());
model.addAttribute("errorMessage", "The requested resource could not be found.");
model.addAttribute("errorTitle", "⚠️Not Found.");
return "error/error";
}
`@ResponseStatus`(value = HttpStatus.BAD_REQUEST)
`@ExceptionHandler`(IllegalArgumentException.class)
public String handleIllegalArgumentException(IllegalArgumentException exception, Model model) {
log.warn("Bad request: {}", exception.getMessage());
// Keep the curated message from services like VisaService (they are safe),
// but fall back to a generic message if getMessage() returns null
String userMessage = exception.getMessage() != null
? exception.getMessage()
: "The request contained invalid data.";
model.addAttribute("errorMessage", userMessage);
model.addAttribute("errorTitle", "⚠️Bad Request.");
return "error/error";
}
`@ResponseStatus`(value = HttpStatus.INTERNAL_SERVER_ERROR)
`@ExceptionHandler`(Exception.class)
public String handleAllUncaughtErrors(Exception exception, Model model) {
log.error("Unexpected Error: ", exception);
model.addAttribute("errorMessage", "Something went wrong. Try again later.");
model.addAttribute("errorTitle", "⚠️Internal Server Error.");
return "error/error";
}
}Key changes explained:
-
handleUnauthorizedException&handleNotFoundException— replacedexception.getMessage()in the model with a static, safe string. The raw message is still captured in the server log vialog.warn(...)so you can diagnose issues without exposing internals (e.g. JPA entity class names or IDs) to the browser. -
handleIllegalArgumentException— kept the dynamic message here since yourVisaServicethrowsIllegalArgumentExceptionwith curated, user-friendly messages (e.g."Travel date cannot be in the past."). Added a null-safe fallback and alog.warn(...)for traceability. If you later have sources ofIllegalArgumentExceptionoutside your control, you may want to switch this to a static message too. -
All handlers now log — using
log.warn(...)for 4xx errors (they're client-side issues, not server faults) and retaininglog.error(...)for the catch-all 500 handler.
🧠 Learnings used
Learnt from: eeebbaandersson
Repo: ithsjava25/visa-case-management-system PR: 27
File: src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java:38-41
Timestamp: 2026-04-14T13:13:34.322Z
Learning: In `visa-case-management-system`, the `processUpdate()` method in `VisaViewController` (`src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`, around lines 148–152) is missing `model.addAttribute("statusInformation", ...)` in the `IllegalArgumentException` catch block. When `visaService.updateVisa()` throws this exception, the edit form (`edit-form.html`) re-renders without `statusInformation`, causing the "Feedback from Handler" banner to display incorrectly. This is a UI/model bug independent of Spring Security and should be fixed by fetching the current visa's `statusInformation` (e.g., via `visaService.findVisaDtoById(id).statusInformation()`) and adding it to the model before returning the edit view.
Learnt from: eeebbaandersson
Repo: ithsjava25/visa-case-management-system PR: 16
File: src/main/java/org/example/visacasemanagementsystem/comment/service/CommentService.java:42-43
Timestamp: 2026-04-09T11:13:00.080Z
Learning: In the `visa-case-management-system` repository, the `authorId` field in `CreateCommentDTO` is currently trusted directly in `CommentService.createComment()` (file: `src/main/java/org/example/visacasemanagementsystem/comment/service/CommentService.java`, around line 42). This is a known security gap intentionally deferred until Spring Security is integrated. Once Spring Security is added, the author should be resolved from the authenticated principal (e.g., via `SecurityContextHolder`) rather than from `dto.authorId()`, and the `authorId` field should be removed from `CreateCommentDTO` or restricted to admin use only.
Learnt from: eeebbaandersson
Repo: ithsjava25/visa-case-management-system PR: 27
File: src/main/resources/templates/visa/details.html:237-244
Timestamp: 2026-04-14T12:40:29.108Z
Learning: In `visa-case-management-system`, the comment form in `src/main/resources/templates/visa/details.html` (around lines 237-244) currently submits to `/api/comments` with form fields `visaId`, `authorId`, and `content` (wrong field name). The intended fix is to replace the HTML form with JavaScript/fetch to POST JSON to the existing REST endpoint `/api/comments`. The correct JSON body field for the comment text is `text` (matching `CreateCommentDTO`), not `content`. This is a known deferred issue to be addressed in a future PR.
Comment on lines
+82
to
+85
| <!-- Todo: update button wih correct redirect URL--> | ||
| <a href="/visa/dashboard" class="btn-back"> | ||
| Return to Dashboard | ||
| </a> |
There was a problem hiding this comment.
Dashboard link points to wrong path.
VisaViewController is mapped to /visas (plural), so /visa/dashboard will 404. The existing TODO already flags this — fix before merge.
🔧 Proposed fix
-<!-- Todo: update button wih correct redirect URL-->
- <a href="/visa/dashboard" class="btn-back">
+ <a th:href="@{/visas/dashboard}" class="btn-back">
Return to Dashboard
</a>Note: after authentication is wired in, currentUserId will also need to come from the security context (see prior deferred work on VisaViewController).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <!-- Todo: update button wih correct redirect URL--> | |
| <a href="/visa/dashboard" class="btn-back"> | |
| Return to Dashboard | |
| </a> | |
| <a th:href="@{/visas/dashboard}" class="btn-back"> | |
| Return to Dashboard | |
| </a> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/templates/error/error.html` around lines 82 - 85, The
dashboard link in the error.html template points to the wrong path; update the
anchor href from "/visa/dashboard" to "/visas/dashboard" to match the
VisaViewController mapping (VisaViewController -> /visas) and remove or update
the TODO comment accordingly; ensure the <a class="btn-back"> element's href is
corrected so the link doesn't 404 (later, when wiring authentication, pull
currentUserId from the security context as noted).
Contributor
Author
There was a problem hiding this comment.
@coderabbitai can you make an issue out of this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo ithsjava25/visa-case-management-system \
--title "Fix: error page dashboard link points to wrong path (/visa/dashboard → /visas/dashboard)" \
--assignee eeebbaandersson \
--body "## Summary
The 'Return to Dashboard' button in \`src/main/resources/templates/error/error.html\` uses a hardcoded href of \`/visa/dashboard\`, which does not match the actual controller mapping. \`VisaViewController\` is mapped to \`/visas\` (plural), so the link results in a 404.
## Expected behaviour
Clicking 'Return to Dashboard' on the error page should navigate the user back to the dashboard without error.
## Steps to reproduce
1. Trigger any handled exception (e.g. \`UnauthorizedException\`) to land on the \`error/error\` view.
2. Click the 'Return to Dashboard' button.
3. Observe a 404 response because \`/visa/dashboard\` does not exist.
## Suggested fix
Replace the hardcoded href with a Thymeleaf expression that resolves to the correct path:
\`\`\`html
<a th:href=\"@{/visas/dashboard}\" class=\"btn-back\">
Return to Dashboard
</a>
\`\`\`
Also remove the stale TODO comment on the line above.
## Additional context
- Raised in PR `#38` review: https://github.com/ithsjava25/visa-case-management-system/pull/38#discussion_r3111434899
- Once Spring Security is wired in, \`currentUserId\` will also need to be resolved from the security context rather than passed as a request parameter (see existing deferred work on \`VisaViewController\`).
Requested by `@eeebbaandersson`."
Length of output: 67
@eeebbaandersson, done! I've opened #40 – Fix: error page dashboard link points to wrong path (/visa/dashboard → /visas/dashboard) and assigned it to you. It includes a description of the bug, reproduction steps, and the suggested fix (th:href="@{/visas/dashboard}").
🧠 Learnings used
Learnt from: eeebbaandersson
Repo: ithsjava25/visa-case-management-system PR: 27
File: src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java:38-41
Timestamp: 2026-04-14T13:13:34.322Z
Learning: In `visa-case-management-system`, the `processUpdate()` method in `VisaViewController` (`src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`, around lines 148–152) is missing `model.addAttribute("statusInformation", ...)` in the `IllegalArgumentException` catch block. When `visaService.updateVisa()` throws this exception, the edit form (`edit-form.html`) re-renders without `statusInformation`, causing the "Feedback from Handler" banner to display incorrectly. This is a UI/model bug independent of Spring Security and should be fixed by fetching the current visa's `statusInformation` (e.g., via `visaService.findVisaDtoById(id).statusInformation()`) and adding it to the model before returning the edit view.
Learnt from: eeebbaandersson
Repo: ithsjava25/visa-case-management-system PR: 27
File: src/main/resources/templates/visa/dashboard.html:125-126
Timestamp: 2026-04-14T12:26:06.631Z
Learning: In the `visa-case-management-system` repository, the "Sign Out" link in `src/main/resources/templates/visa/dashboard.html` (line ~126) points to `/visas/logout`, which is not yet implemented. This is intentionally deferred until Spring Security is fully configured. When implemented, the link should either use Spring Security's default `/logout` endpoint (with CSRF token handling via a POST form) or a dedicated `VisaViewController` method that delegates to `SecurityContextLogoutHandler`.
Learnt from: eeebbaandersson
Repo: ithsjava25/visa-case-management-system PR: 27
File: src/main/resources/templates/visa/details.html:237-244
Timestamp: 2026-04-14T12:40:29.108Z
Learning: In `visa-case-management-system`, the comment form in `src/main/resources/templates/visa/details.html` (around lines 237-244) currently submits to `/api/comments` with form fields `visaId`, `authorId`, and `content` (wrong field name). The intended fix is to replace the HTML form with JavaScript/fetch to POST JSON to the existing REST endpoint `/api/comments`. The correct JSON body field for the comment text is `text` (matching `CreateCommentDTO`), not `content`. This is a known deferred issue to be addressed in a future PR.
Learnt from: eeebbaandersson
Repo: ithsjava25/visa-case-management-system PR: 27
File: src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java:38-41
Timestamp: 2026-04-14T13:13:34.322Z
Learning: In `visa-case-management-system`, `VisaViewController` (`src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`) currently accepts `currentUserId` as a `RequestParam` in all handler methods (showDashboard, showApplyForm, submitApplication, showEditForm, processUpdate, approveVisa, requestMoreInformation, rejectVisa, assignCaseToHandler, viewDetails) instead of resolving the authenticated user from the Spring Security context. This is intentionally deferred until Spring Security is fully wired into the web layer. When implemented, `RequestParam Long currentUserId` should be removed from all these methods and replaced with resolution via `SecurityContextHolder.getContext().getAuthentication()` or `AuthenticationPrincipal`, followed by a `userService` lookup to obtain the `UserDTO`.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/resources/templates/visa/details.html (1)
298-306: LGTM — aligns well with the newErrorResponseshape.Reading
errorData.messageand surfacing it viaSystem Message:correctly consumes the structured payload fromRestExceptionHandler. One small caveat: if an endpoint ever returns a non-JSON error (e.g., a CSRF 403 from Spring Security, or a proxy error page),await response.json()will throw and be caught by the outer.catch, which is acceptable — just be aware the user will seeUnexpected token ...rather than a clean message. Wrapping the parse in a try/catch with a fallback string would make this more robust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/visa/details.html` around lines 298 - 306, The current fetch error handling reads errorData via await response.json() and throws new Error(errorData.message || 'Could not save comment'), but if the server returns a non-JSON error body the await response.json() will itself throw and produce an ugly parse error; update the promise chain in the .then handler (the code referencing response.ok, response.json(), errorData.message, and throw new Error(...)) to attempt parsing JSON inside a try/catch and, on parse failure, use a fallback error string (e.g., response.status + ' ' + response.statusText or a generic 'Could not save comment') and then throw the Error with that fallback so consumers always get a clean message.
🤖 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/visacasemanagementsystem/exception/RestExceptionHandler.java`:
- Around line 37-46: Add a dedicated handler for ResourceNotFoundException in
RestExceptionHandler so 404s are returned instead of the generic 500: implement
a method annotated with `@ExceptionHandler`(ResourceNotFoundException.class) and
`@ResponseStatus`(HttpStatus.NOT_FOUND) (or merge this logic into the existing
handleNotFound if present) that returns an ErrorResponse constructed with
LocalDateTime.now(), HttpStatus.NOT_FOUND.value(), a "Not Found" title, and
exception.getMessage() so the real cause surfaces to the client; ensure the
method signature accepts ResourceNotFoundException and replaces the generic
fallback for this specific exception.
- Around line 37-46: The catch-all REST handler RestExceptionHandler.handleAll
currently returns a generic ErrorResponse but never logs the original exception;
add logging of the exception (e.g., log.error("Unexpected Error in handleAll: ",
exception)) before returning the ErrorResponse so the stacktrace is available in
server logs; if the class lacks a logger, declare and use a logger (e.g.,
LoggerFactory.getLogger(RestExceptionHandler.class)) and ensure the log call
includes the exception object to record stack/causes.
---
Nitpick comments:
In `@src/main/resources/templates/visa/details.html`:
- Around line 298-306: The current fetch error handling reads errorData via
await response.json() and throws new Error(errorData.message || 'Could not save
comment'), but if the server returns a non-JSON error body the await
response.json() will itself throw and produce an ugly parse error; update the
promise chain in the .then handler (the code referencing response.ok,
response.json(), errorData.message, and throw new Error(...)) to attempt parsing
JSON inside a try/catch and, on parse failure, use a fallback error string
(e.g., response.status + ' ' + response.statusText or a generic 'Could not save
comment') and then throw the Error with that fallback so consumers always get a
clean message.
🪄 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: fc71f129-9be1-408d-9b85-23249ed39f26
📒 Files selected for processing (6)
src/main/java/org/example/visacasemanagementsystem/exception/ErrorResponse.javasrc/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.javasrc/main/java/org/example/visacasemanagementsystem/exception/RestExceptionHandler.javasrc/main/resources/templates/error/error.htmlsrc/main/resources/templates/visa/details.htmlsrc/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/templates/error/error.html
Closed
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/example/visacasemanagementsystem/exception/RestExceptionHandler.java (1)
30-47:⚠️ Potential issue | 🟡 MinorMove error-level stacktrace logging to the catch-all handler.
Line 31 logs expected 400s as
"Unhandled REST exception", while Lines 42-48 still return a generic 500 without logging the real exception. Lower the bad-request log level and log the catch-all exception with the stacktrace.Proposed logging adjustment
`@ExceptionHandler`(IllegalArgumentException.class) `@ResponseStatus`(HttpStatus.BAD_REQUEST) public ErrorResponse handleBadRequest(IllegalArgumentException exception) { - log.error("Unhandled REST exception: ", exception); + log.warn("Bad REST request: {}", exception.getMessage()); return new ErrorResponse( LocalDateTime.now(), HttpStatus.BAD_REQUEST.value(), "Bad Request", exception.getMessage() @@ `@ExceptionHandler`(Exception.class) `@ResponseStatus`(HttpStatus.INTERNAL_SERVER_ERROR) public ErrorResponse handleAll(Exception exception) { + log.error("Unhandled REST exception: ", exception); return new ErrorResponse( LocalDateTime.now(), HttpStatus.INTERNAL_SERVER_ERROR.value(), "Internal Server Error",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/exception/RestExceptionHandler.java` around lines 30 - 47, Reduce logging level in handleBadRequest(IllegalArgumentException) by replacing the error-level stacktrace log with a lower-level log (e.g., warn or info) that records the message only, and add an error-level log with the full exception (including stacktrace) inside handleAll(Exception) before returning the ErrorResponse; update references to the methods handleBadRequest and handleAll so handleBadRequest logs exception.getMessage() at warn/info and handleAll calls log.error("Unhandled REST exception: ", exception) to capture the stacktrace.
🤖 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/visacasemanagementsystem/exception/GlobalExceptionHandler.java`:
- Line 12: GlobalExceptionHandler is annotated with
`@ControllerAdvice`(annotations = Controller.class) which also matches
`@RestController` (because it's meta-annotated), causing REST controllers to be
handled by the view-based handler; update the `@ControllerAdvice` on
GlobalExceptionHandler to target only MVC controllers (use assignableTypes to
list the specific controller classes or a common MVC base class) so REST
controllers are excluded, or alternatively add an explicit `@Order` on
RestExceptionHandler to ensure it runs before GlobalExceptionHandler; reference
GlobalExceptionHandler and RestExceptionHandler when making the change.
---
Duplicate comments:
In
`@src/main/java/org/example/visacasemanagementsystem/exception/RestExceptionHandler.java`:
- Around line 30-47: Reduce logging level in
handleBadRequest(IllegalArgumentException) by replacing the error-level
stacktrace log with a lower-level log (e.g., warn or info) that records the
message only, and add an error-level log with the full exception (including
stacktrace) inside handleAll(Exception) before returning the ErrorResponse;
update references to the methods handleBadRequest and handleAll so
handleBadRequest logs exception.getMessage() at warn/info and handleAll calls
log.error("Unhandled REST exception: ", exception) to capture the stacktrace.
🪄 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: cb55ee11-f04e-4f60-857e-4e611c32527e
📒 Files selected for processing (2)
src/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.javasrc/main/java/org/example/visacasemanagementsystem/exception/RestExceptionHandler.java
Martin-E-Karlsson
approved these changes
Apr 21, 2026
Martin-E-Karlsson
left a comment
Martin-E-Karlsson
left a comment
Contributor
There was a problem hiding this comment.
Ser bra ut! Snyggt fixat med exception handlern för Rest. Vi löser eventuella problem med User tester i en ny PR
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
UI/UX Improvements
Tests