Implement global exception handling with custom error responses and e…#3
Merged
Merged
Conversation
…nhance error management in chat service
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ 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: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds validation dependency; introduces ChangesStructured error handling DAG
Sequence DiagramsequenceDiagram
participant Client as Frontend
participant Service as ChatService
participant LLM as LLM Provider
participant Handler as GlobalExceptionHandler
Client->>Service: POST /chat (prompt, sessionId, model)
Service->>LLM: RestClient POST (body)
alt LLM returns error status
LLM-->>Service: HTTP 4xx/5xx
Service->>Service: onStatus(isError) -> throw LLMException
else LLM returns empty/invalid body
LLM-->>Service: 200 with empty/invalid body
Service->>Service: throw LLMException
else LLM returns success
LLM-->>Service: 200 + content
Service-->>Client: 200 + content
end
opt Exception path
Service->>Handler: LLMException/other exception
Handler->>Handler: map to HttpStatus, build ApiErrorResponse
Handler-->>Client: ResponseEntity<ApiErrorResponse>
Client->>Client: parse JSON error and display message
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (1)
src/main/java/org/example/projectbifrost/exception/ApiErrorResponse.java (1)
5-7: ⚡ Quick winPrefer
InstantoverLocalDateTimefor the timestamp field
LocalDateTimehas no timezone information, so the value is ambiguous for any client not co-located with the server, and useless for cross-region log correlation.♻️ Proposed change
-import java.time.LocalDateTime; +import java.time.Instant; public record ApiErrorResponse(String message, int status, - LocalDateTime timestamp) { + Instant timestamp) { }All call sites in
GlobalExceptionHandlerthen becomeInstant.now()instead ofLocalDateTime.now().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/example/projectbifrost/exception/ApiErrorResponse.java` around lines 5 - 7, ApiErrorResponse currently uses LocalDateTime for timestamp; change the type to java.time.Instant (replace LocalDateTime with Instant in the ApiErrorResponse record declaration) and update all call sites in GlobalExceptionHandler to supply Instant.now() instead of LocalDateTime.now(); ensure imports are adjusted (import java.time.Instant) and any JSON serialization/configuration still supports Instant (or add a serializer if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java`:
- Around line 45-53: The handler method handleBadRequestException in
GlobalExceptionHandler currently returns ex.getLocalizedMessage() for both
MethodArgumentNotValidException and HttpMessageNotReadableException which can
leak parser/internal class info; change it to differentiate the two cases —
either split into two `@ExceptionHandler` methods (one for
MethodArgumentNotValidException that includes the detailed localized message,
and one for HttpMessageNotReadableException that returns a generic parsing error
string) or keep the single method but instance-check ex (using instanceof
MethodArgumentNotValidException) and only use ex.getLocalizedMessage() for
validation errors while returning a safe generic message like "Malformed request
body" for HttpMessageNotReadableException before building ApiErrorResponse and
returning BAD_REQUEST.
In `@src/main/java/org/example/projectbifrost/service/ChatService.java`:
- Around line 58-60: The current onStatus handler in ChatService throws
LLMException with only a message and model, discarding the HTTP status and
breaking rate-limit handling; update LLMException to include an int/status Code
field (e.g., add a statusCode property, constructor, and getter), change
ChatService.where .onStatus(...) to throw the new LLMException carrying the
response.statusCode().value(), and update
GlobalExceptionHandler.handleLLMException to inspect
LLMException.getStatusCode() (use a switch or if-chain) to map 429 ->
TOO_MANY_REQUESTS, 401/403 -> UNAUTHORIZED/FORBIDDEN, 500 ->
SERVICE_UNAVAILABLE, etc., instead of relying on fragile string matching.
In `@src/main/resources/static/app.js`:
- Around line 67-75: The current try/catch around response.json() encloses the
throw new Error(errorData.message || 'The Gods are Silent') so that the inner
catch(jsonError) always intercepts it and replaces the backend message with
"Divine connection lost"; fix by separating JSON parsing from throwing: call
await response.json() inside a try, store errorData, and then outside that try
(or in a separate block) throw new Error(errorData.message || 'The Gods are
Silent'); keep the catch(jsonError) only for JSON parse failures and still throw
new Error("Divine connection lost", { cause: jsonError }) there so the backend
message isn't swallowed (identify the change around response.ok handling,
response.json(), jsonError, and the two throw sites).
---
Nitpick comments:
In `@src/main/java/org/example/projectbifrost/exception/ApiErrorResponse.java`:
- Around line 5-7: ApiErrorResponse currently uses LocalDateTime for timestamp;
change the type to java.time.Instant (replace LocalDateTime with Instant in the
ApiErrorResponse record declaration) and update all call sites in
GlobalExceptionHandler to supply Instant.now() instead of LocalDateTime.now();
ensure imports are adjusted (import java.time.Instant) and any JSON
serialization/configuration still supports Instant (or add a serializer if
needed).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c8ffb96-dd75-4f72-a5be-8d824ebd69b4
📒 Files selected for processing (6)
pom.xmlsrc/main/java/org/example/projectbifrost/exception/ApiErrorResponse.javasrc/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.javasrc/main/java/org/example/projectbifrost/exception/LLMException.javasrc/main/java/org/example/projectbifrost/service/ChatService.javasrc/main/resources/static/app.js
… improved logging; update ApiErrorResponse and LLMException for better error management
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java (1)
53-61:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a dedicated handler for
HttpMessageNotReadableException— malformed JSON currently surfaces as 500.With
spring-boot-starter-validationnow wired in, requests with malformed/unparseable JSON bodies (e.g., the frontend ever posts a syntactically invalid payload) throwHttpMessageNotReadableException, which is not a subtype ofMethodArgumentNotValidException. As a result it falls through tohandleGeneralExceptionand the client receives500 INTERNAL_SERVER_ERRORwith the generic "An unexpected error occurred" — but this is a client-side input fault and should be400 BAD_REQUEST.🛠 Suggested handler (mirrors existing style; uses generic message to avoid leaking Jackson internals)
+ `@ExceptionHandler`(org.springframework.http.converter.HttpMessageNotReadableException.class) + public ResponseEntity<ApiErrorResponse> handleUnreadableMessage(org.springframework.http.converter.HttpMessageNotReadableException ex) { + log.warn("Malformed request body: {}", ex.getMostSpecificCause().getMessage()); + return new ResponseEntity<>( + new ApiErrorResponse(LocalDateTime.now(), HttpStatus.BAD_REQUEST.value(), "Malformed request body"), + HttpStatus.BAD_REQUEST + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java` around lines 53 - 61, Add a specific exception handler for HttpMessageNotReadableException in the GlobalExceptionHandler class so malformed/unparseable JSON returns 400 instead of falling through to handleGeneralException; implement a method like public ResponseEntity<ApiErrorResponse> handleHttpMessageNotReadable(HttpMessageNotReadableException ex) that logs the error (e.g., log.warn or log.error with context), and returns new ResponseEntity<>(new ApiErrorResponse(LocalDateTime.now(), 400, "Malformed request body"), HttpStatus.BAD_REQUEST) to mirror the existing ApiErrorResponse usage and style while keeping the message generic.
🧹 Nitpick comments (3)
src/main/java/org/example/projectbifrost/exception/ApiErrorResponse.java (1)
5-10: 💤 Low valueLGTM — record shape matches all call sites in
GlobalExceptionHandler.The component order
(timestamp, status, message)is consistent with every constructor invocation inGlobalExceptionHandler. Two very minor optional nits if you happen to revisit this file:
- The blank line between the parameter list and the body block (line 8) is unusual; most formatters/IDEs will keep them adjacent.
- Consider documenting the record at type level (Javadoc) rather than via a trailing inline comment on a single component, so all three fields are described uniformly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/example/projectbifrost/exception/ApiErrorResponse.java` around lines 5 - 10, The ApiErrorResponse record currently has an unusual blank line between the component list and its body and a trailing inline comment on the message component; update the declaration of ApiErrorResponse to remove the blank line so the parameter list and the body brace are adjacent, and add a Javadoc comment on the ApiErrorResponse type describing the three components (timestamp, status, message) instead of the inline comment; verify GlobalExceptionHandler usages remain unchanged.src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java (2)
38-58: ⚡ Quick winReplace hardcoded status numbers with
HttpStatus.*.value()for consistency.Lines 38, 48 and 58 hardcode
504,400and500numerically insideApiErrorResponse, whilehandleLLMException(line 29) usesstatus.value(). Using the enum throughout removes the risk of the body status drifting from the response status if anyone edits one but not the other.♻️ Proposed cleanup
- new ApiErrorResponse(LocalDateTime.now(), 504, "Connection timeout. The Gods are taking too long to respond."), + new ApiErrorResponse(LocalDateTime.now(), HttpStatus.GATEWAY_TIMEOUT.value(), "Connection timeout. The Gods are taking too long to respond."), HttpStatus.GATEWAY_TIMEOUT @@ - new ApiErrorResponse(LocalDateTime.now(), 400, "Invalid request - check your input"), + new ApiErrorResponse(LocalDateTime.now(), HttpStatus.BAD_REQUEST.value(), "Invalid request - check your input"), HttpStatus.BAD_REQUEST @@ - new ApiErrorResponse(LocalDateTime.now(), 500, "An unexpected error occurred"), + new ApiErrorResponse(LocalDateTime.now(), HttpStatus.INTERNAL_SERVER_ERROR.value(), "An unexpected error occurred"), HttpStatus.INTERNAL_SERVER_ERROR🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java` around lines 38 - 58, Replace the hardcoded numeric status codes in the ApiErrorResponse instances with the corresponding HttpStatus enum values to keep body and response consistent: in handleValidationException use HttpStatus.BAD_REQUEST.value(), in handleGeneralException use HttpStatus.INTERNAL_SERVER_ERROR.value(), and where the gateway timeout is constructed use HttpStatus.GATEWAY_TIMEOUT.value(); update the ApiErrorResponse constructor invocations in these methods (refer to handleValidationException, handleGeneralException and the gateway timeout handling) so the numeric status in the response body is derived from the HttpStatus enum instead of literal integers.
17-32: 💤 Low valueLGTM — dynamic status mapping replaces fragile string inspection.
Mapping
ex.getStatusCode()throughHttpStatus.valueOf(...)with anIllegalArgumentExceptionfallback to 500 is a clean replacement for the previousmessage.contains("429")approach. Thelog.warnline gives operators the upstream status for triage without leaking it through the response.One minor defensive consideration:
HttpStatus.valueOf(int)will happily accept any registered status code, including 1xx/2xx/3xx. IfLLMExceptionis ever constructed with a non-error status (or an upstream returns an unusual code like 200 alongside an "isError" classifier in a future change), the handler would echo that status with an error-shaped body. Optionally, you could clamp to 4xx/5xx:🛡️ Optional defensive clamp
try { status = HttpStatus.valueOf(ex.getStatusCode()); //Get the error from OpenRouter + if (!status.isError()) { + status = HttpStatus.INTERNAL_SERVER_ERROR; + } } catch (IllegalArgumentException e) { status = HttpStatus.INTERNAL_SERVER_ERROR; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java` around lines 17 - 32, The handler handleLLMException in GlobalExceptionHandler currently uses HttpStatus.valueOf(ex.getStatusCode()) which may return non-error codes; after resolving the status, enforce a defensive clamp so only 4xx/5xx are returned to clients: call HttpStatus.valueOf(...), then if(!status.is4xxClientError() && !status.is5xxServerError()) set status = HttpStatus.INTERNAL_SERVER_ERROR (or another chosen 4xx/5xx) before logging and building the ApiErrorResponse; keep log.warn and ApiErrorResponse usage but ensure the ResponseEntity always uses the clamped status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/resources/static/app.js`:
- Around line 67-85: The current catch in the async fetch flow (where you call
appendMessage('assistant', 'System', error.message)) displays raw
Abort/DOMException text; update the catch to detect an abort by checking
error.name === 'AbortError' (or error instanceof DOMException) and replace
error.message with a friendlier timeout message like "Request timed out — the
Gods will return shortly" before calling appendMessage('assistant', 'System',
...); ensure this logic sits in the same function handling the
fetch/AbortController (the block that reads response.json(), response.text(),
and calls setLoading(false)) so normal errors still render their messages and
aborts render the themed timeout text.
---
Duplicate comments:
In
`@src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java`:
- Around line 53-61: Add a specific exception handler for
HttpMessageNotReadableException in the GlobalExceptionHandler class so
malformed/unparseable JSON returns 400 instead of falling through to
handleGeneralException; implement a method like public
ResponseEntity<ApiErrorResponse>
handleHttpMessageNotReadable(HttpMessageNotReadableException ex) that logs the
error (e.g., log.warn or log.error with context), and returns new
ResponseEntity<>(new ApiErrorResponse(LocalDateTime.now(), 400, "Malformed
request body"), HttpStatus.BAD_REQUEST) to mirror the existing ApiErrorResponse
usage and style while keeping the message generic.
---
Nitpick comments:
In `@src/main/java/org/example/projectbifrost/exception/ApiErrorResponse.java`:
- Around line 5-10: The ApiErrorResponse record currently has an unusual blank
line between the component list and its body and a trailing inline comment on
the message component; update the declaration of ApiErrorResponse to remove the
blank line so the parameter list and the body brace are adjacent, and add a
Javadoc comment on the ApiErrorResponse type describing the three components
(timestamp, status, message) instead of the inline comment; verify
GlobalExceptionHandler usages remain unchanged.
In
`@src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java`:
- Around line 38-58: Replace the hardcoded numeric status codes in the
ApiErrorResponse instances with the corresponding HttpStatus enum values to keep
body and response consistent: in handleValidationException use
HttpStatus.BAD_REQUEST.value(), in handleGeneralException use
HttpStatus.INTERNAL_SERVER_ERROR.value(), and where the gateway timeout is
constructed use HttpStatus.GATEWAY_TIMEOUT.value(); update the ApiErrorResponse
constructor invocations in these methods (refer to handleValidationException,
handleGeneralException and the gateway timeout handling) so the numeric status
in the response body is derived from the HttpStatus enum instead of literal
integers.
- Around line 17-32: The handler handleLLMException in GlobalExceptionHandler
currently uses HttpStatus.valueOf(ex.getStatusCode()) which may return non-error
codes; after resolving the status, enforce a defensive clamp so only 4xx/5xx are
returned to clients: call HttpStatus.valueOf(...), then
if(!status.is4xxClientError() && !status.is5xxServerError()) set status =
HttpStatus.INTERNAL_SERVER_ERROR (or another chosen 4xx/5xx) before logging and
building the ApiErrorResponse; keep log.warn and ApiErrorResponse usage but
ensure the ResponseEntity always uses the clamped status.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0c884e1b-c736-4347-9cc5-bfccefbf8d1c
📒 Files selected for processing (5)
src/main/java/org/example/projectbifrost/exception/ApiErrorResponse.javasrc/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.javasrc/main/java/org/example/projectbifrost/exception/LLMException.javasrc/main/java/org/example/projectbifrost/service/ChatService.javasrc/main/resources/static/app.js
…outs and add handling for malformed request bodies
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java (1)
34-51: 💤 Low valueOptional: use
HttpStatus.*.value()consistently instead of magic ints.
handleLLMExceptionusesstatus.value()(line 29) but the timeout/validation/general handlers hardcode504,400,500. Aligning on the enum makes the code self-documenting and future-proof if status codes are ever bumped.♻️ Proposed tweak
- new ApiErrorResponse(LocalDateTime.now(), 504, "Connection timeout. The Gods are taking too long to respond."), + new ApiErrorResponse(LocalDateTime.now(), HttpStatus.GATEWAY_TIMEOUT.value(), "Connection timeout. The Gods are taking too long to respond."), HttpStatus.GATEWAY_TIMEOUT ... - new ApiErrorResponse(LocalDateTime.now(), 400, "Invalid request - check your input"), + new ApiErrorResponse(LocalDateTime.now(), HttpStatus.BAD_REQUEST.value(), "Invalid request - check your input"), HttpStatus.BAD_REQUEST🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java` around lines 34 - 51, In GlobalExceptionHandler, several handlers (handleTimeoutException, handleValidationException and any other handlers that construct ApiErrorResponse with hardcoded integers) use magic ints like 504/400/500; change those to use the corresponding HttpStatus enum values (e.g., HttpStatus.GATEWAY_TIMEOUT.value(), HttpStatus.BAD_REQUEST.value(), HttpStatus.INTERNAL_SERVER_ERROR.value()) so that the ApiErrorResponse is created with HttpStatus.*.value() consistently (match the existing handleLLMException pattern that uses status.value()) and keep the ResponseEntity status using the HttpStatus enum as already done.src/main/resources/static/app.js (1)
132-137: 💤 Low valueOptional: simplify the keydown handler.
sendMessagealready swallows errors in its owntry/catch/finally, so the outer.then(r => {}).catch(err => console.error(err))will never fire on a normal flow and the empty.thenis just noise. A bare call (orvoid sendMessage()) is equivalent and clearer.♻️ Proposed tweak
- sendMessage().then(r => { - }).catch(err => console.error(err)); + void sendMessage();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/static/app.js` around lines 132 - 137, The keydown handler calls sendMessage().then(...).catch(...) which is redundant because sendMessage handles its own errors and the empty .then is noise; replace the chained promise usage with a simple call (e.g., void sendMessage() or just sendMessage()) inside the if block to simplify the handler while preserving e.preventDefault() and the reference to dom.input event handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java`:
- Around line 63-70: The catch‑all handler in
GlobalExceptionHandler.handleGeneralException currently converts every Exception
into a 500 response and masks Spring’s framework status codes; change
GlobalExceptionHandler to extend ResponseEntityExceptionHandler so Spring’s
resolvers run first, and update handleGeneralException to respect
framework-provided statuses by checking for ResponseStatusException (or
inspecting ErrorResponse/HttpStatus from exceptions) and using that status when
present before defaulting to 500; keep logging via log.error("Unexpected error:
", ex) and construct ApiErrorResponse with the resolved status code and message.
---
Nitpick comments:
In
`@src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java`:
- Around line 34-51: In GlobalExceptionHandler, several handlers
(handleTimeoutException, handleValidationException and any other handlers that
construct ApiErrorResponse with hardcoded integers) use magic ints like
504/400/500; change those to use the corresponding HttpStatus enum values (e.g.,
HttpStatus.GATEWAY_TIMEOUT.value(), HttpStatus.BAD_REQUEST.value(),
HttpStatus.INTERNAL_SERVER_ERROR.value()) so that the ApiErrorResponse is
created with HttpStatus.*.value() consistently (match the existing
handleLLMException pattern that uses status.value()) and keep the ResponseEntity
status using the HttpStatus enum as already done.
In `@src/main/resources/static/app.js`:
- Around line 132-137: The keydown handler calls
sendMessage().then(...).catch(...) which is redundant because sendMessage
handles its own errors and the empty .then is noise; replace the chained promise
usage with a simple call (e.g., void sendMessage() or just sendMessage()) inside
the if block to simplify the handler while preserving e.preventDefault() and the
reference to dom.input event handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21a474ea-ffb4-498b-96f4-40956d66d61c
📒 Files selected for processing (2)
src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.javasrc/main/resources/static/app.js
…outs and add handling for malformed request bodies
…Handler inheritance and adding validation to chat request DTO
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/example/projectbifrost/BifrostController.java`:
- Around line 28-29: The handler in BifrostController is logging raw user
messages and session identifiers (logger.info in the request path) which is a
privacy/compliance risk; change the log to avoid dto.message() and to redact or
hash dto.sessionId() (e.g., implement a helper like maskSessionId or compute a
stable hash of sessionId) and log only non-sensitive metadata such as
personality and the redacted/hashed session id. Locate the logger.info call in
BifrostController and replace the arguments to exclude dto.message(), call your
maskSessionId(String) or hashSessionId(String) helper to produce the session
token shown in logs, and ensure the helper handles null/short values safely.
In
`@src/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java`:
- Around line 68-73: The handler in GlobalExceptionHandler converts
ErrorResponse er.getStatusCode() to HttpStatus which throws
IllegalArgumentException for non-standard codes; instead read the existing
HttpStatusCode from ErrorResponse (use er.getStatusCode()), extract its int
value for ApiErrorResponse, and build the ResponseEntity using the
HttpStatusCode (or ResponseEntity.status(code).body(...)) so non-standard status
codes (e.g., 499) are handled without throwing; update the branch handling
ErrorResponse and use HttpStatusCode, ApiErrorResponse, and ResponseEntity
constructs accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0e807854-854a-404a-9183-3ebc40b793cf
📒 Files selected for processing (2)
src/main/java/org/example/projectbifrost/BifrostController.javasrc/main/java/org/example/projectbifrost/exception/GlobalExceptionHandler.java
… and adding session ID masking in chat request logging
This was referenced May 7, 2026
Merged
Merged
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
1 participant
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.
This pull request introduces a robust global exception handling mechanism for the backend, improves error reporting to the frontend, and enhances code clarity with comments and minor refactoring. The main focus is on better error management and user feedback for both developers and end-users.
Backend: Exception Handling and Validation
GlobalExceptionHandler) to standardize error responses for different exception types, including custom LLM errors, timeouts, bad requests, and unexpected errors. This ensures clients receive clear, structured error messages.LLMExceptionto encapsulate errors from LLM providers, and refactoredChatServiceto throw this exception for LLM-related issues instead of generic runtime exceptions. [1] [2]ApiErrorResponse) to unify the error response format sent to clients.spring-boot-starter-validationdependency inpom.xmlto support request validation.Frontend: Improved Error Handling and Code Clarity
app.jsby parsing JSON error responses from the backend and displaying user-friendly error messages. Added explanatory comments and minor code clarifications for maintainability. [1] [2] [3] [4]…nhance error management in chat serviceSummary by CodeRabbit
New Features
Bug Fixes
Chores