Feature/view incident#71
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughConverted comment creation to an MVC redirect, renamed Comment timestamp/lifecycle fields, added a secured GET /api/incidents/{id} with RBAC in the service, replaced a server-rendered incident view with a client-driven /incidents/{id} page, and added a new viewincident.html that fetches incident and comment data client-side. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser as Client (Browser)
participant PageCtrl as PageController (MVC)
participant IncidentCtrl as IncidentController (REST)
participant IncidentSvc as IncidentService (Service)
participant Repo as IncidentRepository (DB)
participant CommentCtrl as CommentController (Controller)
Browser->>PageCtrl: GET /incidents/{id}
PageCtrl-->>Browser: viewincident.html (model: incidentId, role, userId)
Browser->>IncidentCtrl: GET /api/incidents/{id}
IncidentCtrl->>IncidentSvc: getById(id, user)
IncidentSvc->>Repo: findById(id)
Repo-->>IncidentSvc: Incident or not found
IncidentSvc-->>IncidentCtrl: IncidentResponse or throws (404/403)
IncidentCtrl-->>Browser: 200 OK JSON or 4xx
Browser->>CommentCtrl: GET /comments/incident/{id}
CommentCtrl-->>Browser: 200 OK List<Comment> JSON
Browser->>CommentCtrl: POST /comments (form submit)
CommentCtrl-->>Browser: Redirect 302 -> /incidents/{incidentId}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 (4)
src/main/resources/templates/incidents.html (1)
148-148: Empty div serves no purpose.This flex container div is empty and doesn't contribute to the layout. It appears to be leftover from restructuring or an incomplete implementation.
🧹 Remove empty div
<div class="incident-footer"> <span>ID: ${incident.id}</span> - <div style="display: flex; gap: 0.5rem;"></div> <button class="btn" onclick="viewIncident(${incident.id})">View</button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/incidents.html` at line 148, Remove the empty flex container div (<div style="display: flex; gap: 0.5rem;"></div>) from the incidents.html template since it serves no purpose; simply delete that element so the DOM has no unused empty container left behind.src/main/java/org/example/team6backend/page/PageController.java (1)
73-80: Consider validating incident existence server-side.The endpoint renders the view without verifying the incident exists. While the template handles API errors gracefully on the client side, a server-side check would provide a better UX (e.g., redirect to
/incidentswith an error message) instead of rendering a page that shows "Failed to load incident."Also note that unlike
/create-incidentand/admin, this endpoint has no role-based restriction. If this is intentional (all authenticated users can view any incident), it's fine—just confirming the design choice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/page/PageController.java` around lines 73 - 80, The viewIncident method in PageController renders the view without verifying the incident exists; inject and use the existing IncidentService or IncidentRepository to check presence inside viewIncident(Long id, ...), and if the incident is missing redirect to "/incidents" with a flash attribute or model error message (e.g., addRedirectAttribute("error","Incident not found")) instead of returning "viewincident". Also consider adding role-based access if desired by annotating viewIncident with a security annotation (e.g., `@PreAuthorize` or `@Secured`) or otherwise documenting that all authenticated users may view incidents.src/main/resources/templates/viewincident.html (2)
140-152: Use server-providedincidentIdinstead of URL parsing.The controller already passes
incidentIdto the model, but the form populates it via JavaScript from the URL. Use Thymeleaf directly for consistency and to avoid potential mismatch if the URL is manipulated:♻️ Suggested change
- <input type="hidden" name="incidentId" id="incidentIdInput"/> + <input type="hidden" name="incidentId" th:value="${incidentId}"/>This also makes
setIncidentIdInForm()unnecessary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/viewincident.html` around lines 140 - 152, The form currently fills incidentId via client-side URL parsing (and makes setIncidentIdInForm() unnecessary); instead bind the server-provided model value directly: set the hidden input with id "incidentIdInput" to use Thymeleaf value binding (th:value="${incidentId}") and remove the JS function setIncidentIdInForm() and any script that parses the URL for incidentId so the form uses the controller-provided incidentId consistently.
197-200: Minor: Consider escapingincident.idfor consistency.While
incident.idis expected to be numeric from the API, applyingescapeHtmlto all dynamic values maintains consistent defense-in-depth:♻️ Suggested change
<div class="incident-footer"> - <span>ID: ${incident.id}</span> + <span>ID: ${escapeHtml(incident.id)}</span> <span>Status: ${escapeHtml(incident.incidentStatus || "N/A")}</span> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/viewincident.html` around lines 197 - 200, The ID span currently outputs ${incident.id} unescaped; update the incident-footer rendering to pass the ID through the existing escapeHtml helper (ensuring you convert the numeric id to a string if needed) so the span uses escapeHtml(incident.id) for consistent escaping with the other fields.
🤖 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/team6backend/comment/controller/CommentController.java`:
- Around line 30-34: The createComment method should handle validation failures
and guard against a null incidentId: change the signature of
CommentController.createComment to accept a
org.springframework.validation.BindingResult immediately after the `@Valid`
CommentRequest, check bindingResult.hasErrors() and return the appropriate view
(e.g., the incident detail page or the form view) so validation errors render
instead of throwing MethodArgumentNotValidException; also
validate/request.getIncidentId() != null before building the redirect, and if
null, handle gracefully (render an error view or redirect to a safe list page)
instead of concatenating "null"; ensure you still call
commentService.createComment(request.getIncidentId(), request.getUserId(),
request.getMessage()) only when validation passes and incidentId is present.
In
`@src/main/java/org/example/team6backend/incident/service/IncidentService.java`:
- Around line 71-73: Replace the generic RuntimeException thrown from
IncidentService.getById with a specific 404-mapped exception: create
IncidentNotFoundException extends RuntimeException and throw it from
IncidentService.getById when incidentRepository.findById(id) is empty, or
alternatively throw a ResponseStatusException(HttpStatus.NOT_FOUND, "...") from
getById; then add or update a `@ControllerAdvice` (e.g., GlobalExceptionHandler)
with an `@ExceptionHandler`(IncidentNotFoundException.class) that returns
ResponseEntity.status(HttpStatus.NOT_FOUND) so the service returns HTTP 404 for
missing incidents.
In `@src/main/resources/templates/incidents.html`:
- Around line 136-144: The template uses card.innerHTML to inject unescaped user
fields (incident.subject, incident.incidentCategory, incident.description),
creating an XSS risk; fix by avoiding direct innerHTML: build DOM nodes with
document.createElement and set textContent for title/category/description or
implement and call an escapeHtml(text) helper before inserting into the
template, replacing the inline `${...}` insertions; update the code that
populates the card (reference card.innerHTML usage) to use safe DOM manipulation
or escaped strings for all three fields.
---
Nitpick comments:
In `@src/main/java/org/example/team6backend/page/PageController.java`:
- Around line 73-80: The viewIncident method in PageController renders the view
without verifying the incident exists; inject and use the existing
IncidentService or IncidentRepository to check presence inside viewIncident(Long
id, ...), and if the incident is missing redirect to "/incidents" with a flash
attribute or model error message (e.g., addRedirectAttribute("error","Incident
not found")) instead of returning "viewincident". Also consider adding
role-based access if desired by annotating viewIncident with a security
annotation (e.g., `@PreAuthorize` or `@Secured`) or otherwise documenting that all
authenticated users may view incidents.
In `@src/main/resources/templates/incidents.html`:
- Line 148: Remove the empty flex container div (<div style="display: flex; gap:
0.5rem;"></div>) from the incidents.html template since it serves no purpose;
simply delete that element so the DOM has no unused empty container left behind.
In `@src/main/resources/templates/viewincident.html`:
- Around line 140-152: The form currently fills incidentId via client-side URL
parsing (and makes setIncidentIdInForm() unnecessary); instead bind the
server-provided model value directly: set the hidden input with id
"incidentIdInput" to use Thymeleaf value binding (th:value="${incidentId}") and
remove the JS function setIncidentIdInForm() and any script that parses the URL
for incidentId so the form uses the controller-provided incidentId consistently.
- Around line 197-200: The ID span currently outputs ${incident.id} unescaped;
update the incident-footer rendering to pass the ID through the existing
escapeHtml helper (ensuring you convert the numeric id to a string if needed) so
the span uses escapeHtml(incident.id) for consistent escaping with the other
fields.
🪄 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: 7c60184a-93bb-40d6-84d2-4b33a46df8df
📒 Files selected for processing (7)
src/main/java/org/example/team6backend/comment/controller/CommentController.javasrc/main/java/org/example/team6backend/comment/entity/Comment.javasrc/main/java/org/example/team6backend/incident/controller/IncidentController.javasrc/main/java/org/example/team6backend/incident/service/IncidentService.javasrc/main/java/org/example/team6backend/page/PageController.javasrc/main/resources/templates/incidents.htmlsrc/main/resources/templates/viewincident.html
Comment on lines
30
to
34
| @PostMapping | ||
| public ResponseEntity<Comment> createComment(@Valid @RequestBody CommentRequest request) { | ||
| Comment saveComment = commentService.createComment(request.getIncidentId(), request.getUserId(), | ||
| request.getMessage()); | ||
| return ResponseEntity.ok(saveComment); | ||
| public String createComment(@Valid CommentRequest request) { | ||
| commentService.createComment(request.getIncidentId(), request.getUserId(), request.getMessage()); | ||
| return "redirect:/incidents/" + request.getIncidentId(); | ||
| } |
There was a problem hiding this comment.
Handle validation errors and null incidentId gracefully.
Two concerns with the redirect flow:
-
Validation errors: If
@Validfails (e.g., empty message), Spring throwsMethodArgumentNotValidException, likely showing a Whitelabel error page without a global handler. -
Null safety: If
request.getIncidentId()is null (malformed form submission), the string concatenation on line 33 would produceredirect:/incidents/null.
🛡️ Suggested improvement
`@PostMapping`
-public String createComment(`@Valid` CommentRequest request) {
+public String createComment(`@Valid` CommentRequest request, BindingResult bindingResult) {
+ if (bindingResult.hasErrors() || request.getIncidentId() == null) {
+ // Redirect back with error indicator, or to incidents list
+ return "redirect:/incidents";
+ }
commentService.createComment(request.getIncidentId(), request.getUserId(), request.getMessage());
return "redirect:/incidents/" + request.getIncidentId();
}You'd also need to import org.springframework.validation.BindingResult.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/team6backend/comment/controller/CommentController.java`
around lines 30 - 34, The createComment method should handle validation failures
and guard against a null incidentId: change the signature of
CommentController.createComment to accept a
org.springframework.validation.BindingResult immediately after the `@Valid`
CommentRequest, check bindingResult.hasErrors() and return the appropriate view
(e.g., the incident detail page or the form view) so validation errors render
instead of throwing MethodArgumentNotValidException; also
validate/request.getIncidentId() != null before building the redirect, and if
null, handle gracefully (render an error view or redirect to a safe list page)
instead of concatenating "null"; ensure you still call
commentService.createComment(request.getIncidentId(), request.getUserId(),
request.getMessage()) only when validation passes and incidentId is present.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/team6backend/incident/service/IncidentService.java`:
- Around line 63-64: The service removed the original findById API causing
callers to break; restore compatibility by adding a compat shim method named
findById(Long id) in IncidentService that simply delegates to the new
getById(Long id) (and mark it `@Deprecated` if desired) so existing callers
compile, while leaving the new getById implementation using
incidentRepository.findById(...). Ensure the shim throws the same exception
behavior by forwarding the result/exception from getById.
In `@src/main/java/org/example/team6backend/page/PageController.java`:
- Around line 162-169: There are two identical handlers named viewIncident(Long,
CustomUserDetails, Model) causing a duplicate-method compile error; remove one
of the duplicate methods or consolidate them into a single handler, and if both
routes are required keep one method and rename the other (e.g., viewIncidentList
or viewIncidentAlternate) and update its `@GetMapping` accordingly (/incident/{id}
vs /incidents/{id}) so method names and signatures are unique while preserving
the desired route behavior and model attributes (role, incidentId, userId) set
in the surviving handler.
🪄 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: 0253eb11-a181-488a-b3ee-aee7f0f0b65f
📒 Files selected for processing (3)
src/main/java/org/example/team6backend/incident/controller/IncidentController.javasrc/main/java/org/example/team6backend/incident/service/IncidentService.javasrc/main/java/org/example/team6backend/page/PageController.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/team6backend/incident/controller/IncidentController.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/team6backend/page/PageController.java`:
- Around line 134-141: The viewIncident GET in PageController (viewIncident)
currently always renders the page shell and skips the old per-incident
authorization (admin || creator || assignee), allowing deep-link access; restore
that check in the shared backend path by moving the ownership/assignment logic
into the incident service (e.g., add
IncidentService.isAuthorizedForIncident(User user, Long incidentId) or perform
the check inside IncidentService.getIncident) and call it from both
IncidentController (where /api/incidents/{id} lives) and
PageController.viewIncident so both endpoints enforce the same semantics and
return 403 or 404 as before when unauthorized or not found. Ensure the check
uses the same role/ownership rules (admin || creator || assignee) and that
PageController does not render the page unless the service authorizes the
request.
🪄 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: b7b2893f-6b11-42ff-a000-fa78549d1b7b
📒 Files selected for processing (1)
src/main/java/org/example/team6backend/page/PageController.java
… check here or in a shared backend path.
This was referenced Apr 9, 2026
Merged
Merged
This was referenced Apr 17, 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
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.
Added view commetns for a specific incident, add a new comment directly the incident page, see who wrote the comment and when it was created
Summary by CodeRabbit
New Features
Behavior Changes