Issue/71 (DONT MERGE)#73
Conversation
📝 WalkthroughWalkthroughThis PR introduces a real-time notifications system for case updates using Server-Sent Events (SSE). It adds backend infrastructure to manage SSE subscriptions with role-based access control, publishes events from case and document operations, exposes HTTP endpoints for client subscriptions, and implements client-side event listeners that trigger page reloads when updates are detected. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client<br/>(Browser)
participant Controller as CaseRealtimeController
participant Service as CaseRealtimeService
participant Emitter as SseEmitter
participant DB as CaseService<br/>(Mutation)
Client->>Controller: GET /ui/realtime/cases/{caseId}/stream
Controller->>Service: subscribeToCase(actor, caseId)
Service->>Service: validateActor & checkAccess
Service->>Emitter: create SseEmitter
Service->>Emitter: send 'connected' event
Emitter-->>Client: EventSource opens
DB->>Service: publishCaseEvent(caseId, eventType, message)
Service->>Service: findSubscribers(caseId)
Service->>Service: filterByReadablePermissions
Service->>Emitter: send SSE event
Emitter-->>Client: receives event<br/>(note-added, etc.)
Client->>Client: throttled reload<br/>(min 800ms)
Service->>Service: keepAlive (every 25s)
Service->>Emitter: send 'keepalive' event
Emitter-->>Client: connection maintained
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 docstrings
🧪 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
start-local.ps1 (1)
187-197:⚠️ Potential issue | 🟡 MinorBrowser opens before Spring Boot is ready, showing a connection-refused page.
The previous implementation polled
http://localhost:8080for readiness before opening the browser. The new flow callsOpen-BrowserUrlsynchronously on line 195 and then starts./mvnw spring-boot:runon line 197. Since Spring Boot takes tens of seconds to boot (especially on a cold JVM), users will almost always land on a "connection refused" / "site can't be reached" page and have to manually refresh.Consider launching the browser after Spring Boot is reachable (e.g., from a background job that polls the port) or at least warning the user that a manual refresh will be required once the app is up.
🔧 One possible fix: background readiness poller
-Pause-ForEnjoyment -Open-BrowserUrl -Url "http://localhost:8080" - -$mavenExitCode = Invoke-CommandWithMode -Command { .\mvnw spring-boot:run "-Dspring-boot.run.profiles=local" } +Pause-ForEnjoyment + +$browserJob = Start-Job -ScriptBlock { + param($url) + for ($i = 0; $i -lt 60; $i++) { + try { + Invoke-WebRequest -Uri $url -UseBasicParsing -TimeoutSec 2 | Out-Null + return $true + } catch { + Start-Sleep -Seconds 2 + } + } + return $false +} -ArgumentList "http://localhost:8080" + +# Open browser once the app responds (fire-and-forget handler) +Register-ObjectEvent -InputObject $browserJob -EventName StateChanged -Action { + if ($Event.Sender.State -eq 'Completed') { + Start-Process "http://localhost:8080" + } +} | Out-Null + +$mavenExitCode = Invoke-CommandWithMode -Command { .\mvnw spring-boot:run "-Dspring-boot.run.profiles=local" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@start-local.ps1` around lines 187 - 197, Move the browser open logic out of the direct startup flow and start a background readiness poll that opens the browser only when Spring Boot is reachable: create a small background job (using Start-Job) that repeatedly tries Invoke-WebRequest (or Test-NetConnection) against "http://localhost:8080" with a short sleep/retry loop and a reasonable timeout, and when it receives a successful response call Open-BrowserUrl; start that job before calling Invoke-CommandWithMode (which runs .\mvnw spring-boot:run) so the browser opens automatically once the app is ready; reference the existing Open-BrowserUrl and Invoke-CommandWithMode symbols and add an optional warning/log if the poll times out.
🧹 Nitpick comments (2)
src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java (1)
47-48: Consider adding assertions that realtime events are published.The mock is wired in but no test verifies that
caseRealtimeServiceactually receives"document-added"on upload or"document-deleted"on delete. Adding averify(caseRealtimeService).publishCaseEvent(...)inuploadDocument_shouldAllowOwneranddeleteDocument_shouldAllowOwnerwould lock in the new behavior described in the PR summary and prevent silent regressions if the publish call is accidentally removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java` around lines 47 - 48, Add assertions in the tests uploadDocument_shouldAllowOwner and deleteDocument_shouldAllowOwner to verify that the mocked CaseRealtimeService receives the publishCaseEvent call: use verify(caseRealtimeService).publishCaseEvent(...) (or verify(caseRealtimeService, times(1)).publishCaseEvent(...)) to assert the event name parameter equals "document-added" for uploadDocument_shouldAllowOwner and "document-deleted" for deleteDocument_shouldAllowOwner, passing the expected case identifier (or appropriate matcher) so the tests lock in the realtime publish behavior.src/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.java (1)
47-48: No tests assert that realtime events are published.Given that
CaseServicenow emitscase-created,case-updated,status-changed,note-added, andassignment-updatedevents, consider addingverify(caseRealtimeService).publishCaseEvent(...)assertions in at least one test per event type (e.g.,createCase_shouldAllowDoctor,updateCase_shouldAllowOwnerDoctor,assignUsers_shouldSetStatusToHandlerAssigned). Without these, the SSE publishing behavior is untested and can silently regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.java` around lines 47 - 48, Add assertions that CaseService publishes realtime events by verifying interactions with the mocked CaseRealtimeService: in tests like createCase_shouldAllowDoctor, updateCase_shouldAllowOwnerDoctor, and assignUsers_shouldSetStatusToHandlerAssigned add Mockito.verify(caseRealtimeService).publishCaseEvent(...) calls (or verify with times(1)) for the corresponding event names "case-created", "case-updated", "status-changed", "note-added", and "assignment-updated"; if you need to inspect payloads, use an ArgumentCaptor to capture the published event and assert its contents, otherwise use argument matchers (e.g., any()) to ensure the publishCaseEvent call occurred.
🤖 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/projektarendehantering/application/service/CaseRealtimeService.java`:
- Around line 98-138: canRead currently omits patients and only checks the
post-update CaseEntity, so patients who should see their own cases can't
subscribe and subscribers who lose access (on CLOSE or reassignment) are simply
skipped; update canRead(Actor, CaseEntity) to treat Role.PATIENT as allowed when
actor.userId() equals entity.getPatientId() (or appropriate patient id field),
and modify the subscriber loops (listSubscribers and caseSubscribers handling in
CaseRealtimeService) to compute and use the previous readable audience or
explicitly publish a minimal revocation/update event via sendEvent before
removing/closing emitters (use closeEmitter) so clients that lose access receive
a final event to refresh/remove the stale case.
- Around line 155-163: The sendEvent method currently only catches IOException,
but SseEmitter.send(...) can also throw IllegalStateException for
completed/stale emitters; update sendEvent (and any similar send wrappers) to
catch IllegalStateException as well (either add a separate
catch(IllegalStateException ex) or broaden to catch(IOException |
IllegalStateException ex)), log the eventName and exception message similarly to
the existing log.debug line, call closeEmitter(emitter) in that branch, and
return false so stale emitters don't propagate exceptions to callers.
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`:
- Line 68: In CaseService replace direct calls to
caseRealtimeService.publishCaseEvent(...) (e.g., the calls in CaseService where
notes/case updates are published) with an after-commit scheduling helper so
events are emitted only when the transaction successfully commits; implement a
small helper (e.g., runAfterCommit or use
TransactionSynchronizationManager.registerSynchronization / publish an
ApplicationEvent consumed by `@TransactionalEventListener`(phase = AFTER_COMMIT))
and use it to invoke caseRealtimeService.publishCaseEvent(caseEntity.getId(),
"note-added", ...) (and the other occurrences) instead of calling
publishCaseEvent synchronously.
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Line 117: uploadDocument currently publishes the SSE event via
caseRealtimeService.publishCaseEvent(caseEntity.getId(), "document-added",
"Document uploaded") while still inside the transaction; move this publish into
a transaction after-commit hook so the event is emitted only if the transaction
successfully commits. Locate the uploadDocument method and replace the direct
publish call with registering a TransactionSynchronization (via
TransactionSynchronizationManager.registerSynchronization or equivalent) that
calls caseRealtimeService.publishCaseEvent(...) in its afterCommit()
implementation, ensuring any variables used (caseEntity.getId(), event name,
message) are captured or final/effectively final for use in the callback.
In `@src/main/resources/templates/cases/list.html`:
- Around line 53-101: The page currently reloads on any unnamed SSE (remove the
footgun), manually overrides EventSource backoff (stop pounding the server), and
reloads too frequently (throttle too short); fix by removing the catch-all
assignment source.onmessage = refreshPage and rely only on the explicit
addEventListener(...) handlers, change source.onerror to not close the
EventSource or schedule a manual reconnect (let the browser's EventSource handle
retries), and increase the refresh throttle in refreshPage by raising
lastRefreshAt threshold from 800 to ~3000 ms (or implement a short debounce) so
bursts of events collapse into a single reload; reference symbols:
source.onmessage, addEventListener(...), source.onerror, reconnectTimer,
connect(), refreshPage, lastRefreshAt.
---
Outside diff comments:
In `@start-local.ps1`:
- Around line 187-197: Move the browser open logic out of the direct startup
flow and start a background readiness poll that opens the browser only when
Spring Boot is reachable: create a small background job (using Start-Job) that
repeatedly tries Invoke-WebRequest (or Test-NetConnection) against
"http://localhost:8080" with a short sleep/retry loop and a reasonable timeout,
and when it receives a successful response call Open-BrowserUrl; start that job
before calling Invoke-CommandWithMode (which runs .\mvnw spring-boot:run) so the
browser opens automatically once the app is ready; reference the existing
Open-BrowserUrl and Invoke-CommandWithMode symbols and add an optional
warning/log if the poll times out.
---
Nitpick comments:
In
`@src/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.java`:
- Around line 47-48: Add assertions that CaseService publishes realtime events
by verifying interactions with the mocked CaseRealtimeService: in tests like
createCase_shouldAllowDoctor, updateCase_shouldAllowOwnerDoctor, and
assignUsers_shouldSetStatusToHandlerAssigned add
Mockito.verify(caseRealtimeService).publishCaseEvent(...) calls (or verify with
times(1)) for the corresponding event names "case-created", "case-updated",
"status-changed", "note-added", and "assignment-updated"; if you need to inspect
payloads, use an ArgumentCaptor to capture the published event and assert its
contents, otherwise use argument matchers (e.g., any()) to ensure the
publishCaseEvent call occurred.
In
`@src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java`:
- Around line 47-48: Add assertions in the tests uploadDocument_shouldAllowOwner
and deleteDocument_shouldAllowOwner to verify that the mocked
CaseRealtimeService receives the publishCaseEvent call: use
verify(caseRealtimeService).publishCaseEvent(...) (or
verify(caseRealtimeService, times(1)).publishCaseEvent(...)) to assert the event
name parameter equals "document-added" for uploadDocument_shouldAllowOwner and
"document-deleted" for deleteDocument_shouldAllowOwner, passing the expected
case identifier (or appropriate matcher) so the tests lock in the realtime
publish behavior.
🪄 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: 71f31482-4243-4cb0-b3ec-b21e2ad27801
📒 Files selected for processing (12)
README.mdsrc/main/java/org/example/projektarendehantering/ProjektArendehanteringApplication.javasrc/main/java/org/example/projektarendehantering/application/service/CaseRealtimeEvent.javasrc/main/java/org/example/projektarendehantering/application/service/CaseRealtimeService.javasrc/main/java/org/example/projektarendehantering/application/service/CaseService.javasrc/main/java/org/example/projektarendehantering/application/service/DocumentService.javasrc/main/java/org/example/projektarendehantering/presentation/web/CaseRealtimeController.javasrc/main/resources/templates/cases/detail.htmlsrc/main/resources/templates/cases/list.htmlsrc/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.javasrc/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.javastart-local.ps1
| listSubscribers.forEach((id, sub) -> { | ||
| if (!canRead(sub.actor(), caseEntity)) { | ||
| return; | ||
| } | ||
| if (!sendEvent(sub.emitter(), event.eventType(), event)) { | ||
| listSubscribers.remove(id); | ||
| } | ||
| }); | ||
|
|
||
| caseSubscribers.forEach((id, sub) -> { | ||
| if (!event.caseId().equals(sub.caseId())) { | ||
| return; | ||
| } | ||
| if (!canRead(sub.actor(), caseEntity)) { | ||
| caseSubscribers.remove(id); | ||
| closeEmitter(sub.emitter()); | ||
| return; | ||
| } | ||
| if (!sendEvent(sub.emitter(), event.eventType(), event)) { | ||
| caseSubscribers.remove(id); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| private boolean canRead(Actor actor, CaseEntity entity) { | ||
| if (actor == null || actor.role() == null) { | ||
| return false; | ||
| } | ||
| if (actor.role() == Role.MANAGER) { | ||
| return true; | ||
| } | ||
| if (entity.getStatus() == CaseStatus.CLOSED) { | ||
| return false; | ||
| } | ||
| if (actor.role() == Role.DOCTOR && actor.userId().equals(entity.getOwnerId())) { | ||
| return true; | ||
| } | ||
| if (actor.role() == Role.NURSE && actor.userId().equals(entity.getHandlerId())) { | ||
| return true; | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Align realtime access with case access rules and access-loss events.
canRead omits patients, so patients who can read their own cases through CaseService cannot subscribe or receive updates. Also, filtering only against the post-update state means users who lose access on close/reassignment are silently skipped instead of receiving the event needed to refresh/remove the stale case.
Add patient visibility and handle revocation events explicitly
if (actor.role() == Role.NURSE && actor.userId().equals(entity.getHandlerId())) {
return true;
}
+ if (actor.role() == Role.PATIENT
+ && entity.getPatient() != null
+ && actor.userId().equals(entity.getPatient().getId())) {
+ return true;
+ }
return false;For closures/reassignments, consider publishing with the previous readable audience or sending a minimal revocation/update event before removing case subscribers, so affected clients can reload cleanly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseRealtimeService.java`
around lines 98 - 138, canRead currently omits patients and only checks the
post-update CaseEntity, so patients who should see their own cases can't
subscribe and subscribers who lose access (on CLOSE or reassignment) are simply
skipped; update canRead(Actor, CaseEntity) to treat Role.PATIENT as allowed when
actor.userId() equals entity.getPatientId() (or appropriate patient id field),
and modify the subscriber loops (listSubscribers and caseSubscribers handling in
CaseRealtimeService) to compute and use the previous readable audience or
explicitly publish a minimal revocation/update event via sendEvent before
removing/closing emitters (use closeEmitter) so clients that lose access receive
a final event to refresh/remove the stale case.
| private boolean sendEvent(SseEmitter emitter, String eventName, CaseRealtimeEvent payload) { | ||
| try { | ||
| emitter.send(SseEmitter.event().name(eventName).data(payload)); | ||
| return true; | ||
| } catch (IOException ex) { | ||
| log.debug("Failed to send SSE event '{}': {}", eventName, ex.getMessage()); | ||
| closeEmitter(emitter); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Spring Framework SseEmitter, can SseEmitter.send(...) throw IllegalStateException or another RuntimeException when the emitter is already completed or disconnected?
💡 Result:
Yes, SseEmitter.send(...) can throw IllegalStateException when the emitter is already completed. The SseEmitter.send method explicitly checks if it is already complete and throws IllegalStateException if so, as shown in the source code: Assert.state(!this.complete, "ResponseBodyEmitter has already completed" + (this.failure != null ? " with error: " + this.failure : ""));. Multiple Stack Overflow examples confirm this behavior in practice, such as when sending after timeout or completion, resulting in "ResponseBodyEmitter is already set complete". For disconnection, it typically throws IOException rather than IllegalStateException.
Citations:
- 1: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/servlet/mvc/method/annotation/SseEmitter.html
- 2: https://docs.spring.vmware.com/spring-framework/docs/6.0.25/javadoc-api/org/springframework/web/servlet/mvc/method/annotation/SseEmitter.html
- 3: https://stackoverflow.com/questions/78042347/spring-boot-3-sseemitter-complete-triggers-cannot-forward-to-error-page
- 4: https://stackoverflow.com/questions/42835888/spring-server-sent-events-responsebodyemitter-is-already-set-complete
- 5: https://docs.spring.io/spring-framework/docs/current/javadoc-api/org/springframework/web/servlet/mvc/method/annotation/ResponseBodyEmitter.html
- 6: https://www.seaxiang.com/blog/1dc2c92bb8404031b62512ba7c30a200
Catch IllegalStateException when sending to stale SSE emitters.
SseEmitter.send(...) throws IllegalStateException when the emitter is already completed, not just IOException for disconnections. The current catch clause only handles IOException, allowing IllegalStateException to escape and potentially break transactional operations in the caller.
Handle completed/disconnected emitters defensively
- } catch (IOException ex) {
+ } catch (IOException | IllegalStateException ex) {
log.debug("Failed to send SSE event '{}': {}", eventName, ex.getMessage());
closeEmitter(emitter);
return false;
}📝 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.
| private boolean sendEvent(SseEmitter emitter, String eventName, CaseRealtimeEvent payload) { | |
| try { | |
| emitter.send(SseEmitter.event().name(eventName).data(payload)); | |
| return true; | |
| } catch (IOException ex) { | |
| log.debug("Failed to send SSE event '{}': {}", eventName, ex.getMessage()); | |
| closeEmitter(emitter); | |
| return false; | |
| } | |
| private boolean sendEvent(SseEmitter emitter, String eventName, CaseRealtimeEvent payload) { | |
| try { | |
| emitter.send(SseEmitter.event().name(eventName).data(payload)); | |
| return true; | |
| } catch (IOException | IllegalStateException ex) { | |
| log.debug("Failed to send SSE event '{}': {}", eventName, ex.getMessage()); | |
| closeEmitter(emitter); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseRealtimeService.java`
around lines 155 - 163, The sendEvent method currently only catches IOException,
but SseEmitter.send(...) can also throw IllegalStateException for
completed/stale emitters; update sendEvent (and any similar send wrappers) to
catch IllegalStateException as well (either add a separate
catch(IllegalStateException ex) or broaden to catch(IOException |
IllegalStateException ex)), log the eventName and exception message similarly to
the existing log.debug line, call closeEmitter(emitter) in that branch, and
return false so stale emitters don't propagate exceptions to callers.
| caseEntity.setStatus(CaseStatus.COMMUNICATION); | ||
| caseRepository.save(caseEntity); | ||
| recordStatusChange(actor, caseEntity.getId(), previousStatus, CaseStatus.COMMUNICATION); | ||
| caseRealtimeService.publishCaseEvent(caseEntity.getId(), "note-added", "A note was added"); |
There was a problem hiding this comment.
Defer case realtime events until after commit.
These methods are transactional, but the SSE events are emitted before commit. A failed commit can produce phantom notifications, and a fast client reload can observe stale state.
Use an after-commit helper for all case events
import org.springframework.transaction.annotation.Transactional;
+import org.springframework.transaction.support.TransactionSynchronization;
+import org.springframework.transaction.support.TransactionSynchronizationManager;- caseRealtimeService.publishCaseEvent(caseEntity.getId(), "note-added", "A note was added");
+ publishCaseEventAfterCommit(caseEntity.getId(), "note-added", "A note was added");- caseRealtimeService.publishCaseEvent(savedEntity.getId(), "case-created", "Case created");
+ publishCaseEventAfterCommit(savedEntity.getId(), "case-created", "Case created");- caseRealtimeService.publishCaseEvent(savedEntity.getId(), "case-updated", "Case updated");
+ publishCaseEventAfterCommit(savedEntity.getId(), "case-updated", "Case updated");- caseRealtimeService.publishCaseEvent(entity.getId(), "status-changed", "Case closed");
+ publishCaseEventAfterCommit(entity.getId(), "status-changed", "Case closed");- caseRealtimeService.publishCaseEvent(savedEntity.getId(), "assignment-updated", "Case assignment updated");
+ publishCaseEventAfterCommit(savedEntity.getId(), "assignment-updated", "Case assignment updated");
return caseMapper.toDTO(savedEntity);
}
CaseEntity savedEntity = caseRepository.save(entity);
- caseRealtimeService.publishCaseEvent(savedEntity.getId(), "assignment-updated", "Case assignment updated");
+ publishCaseEventAfterCommit(savedEntity.getId(), "assignment-updated", "Case assignment updated");
return caseMapper.toDTO(savedEntity);+ private void publishCaseEventAfterCommit(UUID caseId, String eventType, String message) {
+ if (TransactionSynchronizationManager.isSynchronizationActive()) {
+ TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
+ `@Override`
+ public void afterCommit() {
+ caseRealtimeService.publishCaseEvent(caseId, eventType, message);
+ }
+ });
+ return;
+ }
+ caseRealtimeService.publishCaseEvent(caseId, eventType, message);
+ }
+
private void recordStatusChange(Actor actor, UUID caseId, CaseStatus from, CaseStatus to) {Also applies to: 110-110, 147-147, 164-164, 258-263
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`
at line 68, In CaseService replace direct calls to
caseRealtimeService.publishCaseEvent(...) (e.g., the calls in CaseService where
notes/case updates are published) with an after-commit scheduling helper so
events are emitted only when the transaction successfully commits; implement a
small helper (e.g., runAfterCommit or use
TransactionSynchronizationManager.registerSynchronization / publish an
ApplicationEvent consumed by `@TransactionalEventListener`(phase = AFTER_COMMIT))
and use it to invoke caseRealtimeService.publishCaseEvent(caseEntity.getId(),
"note-added", ...) (and the other occurrences) instead of calling
publishCaseEvent synchronously.
| .actorRole(actor.role() != null ? actor.role().name() : null) | ||
| .build()); | ||
| } | ||
| caseRealtimeService.publishCaseEvent(caseEntity.getId(), "document-added", "Document uploaded"); |
There was a problem hiding this comment.
Publish document-added only after the transaction commits.
Line 117 sends the SSE event while uploadDocument is still transactional. A later commit failure would still notify clients, and clients may reload before the document/status/audit changes are visible.
Defer the notification to `afterCommit()`
- caseRealtimeService.publishCaseEvent(caseEntity.getId(), "document-added", "Document uploaded");
+ if (syncActive) {
+ TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
+ `@Override`
+ public void afterCommit() {
+ caseRealtimeService.publishCaseEvent(caseEntity.getId(), "document-added", "Document uploaded");
+ }
+ });
+ } else {
+ caseRealtimeService.publishCaseEvent(caseEntity.getId(), "document-added", "Document uploaded");
+ }
return documentMapper.toDTO(saved);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`
at line 117, uploadDocument currently publishes the SSE event via
caseRealtimeService.publishCaseEvent(caseEntity.getId(), "document-added",
"Document uploaded") while still inside the transaction; move this publish into
a transaction after-commit hook so the event is emitted only if the transaction
successfully commits. Locate the uploadDocument method and replace the direct
publish call with registering a TransactionSynchronization (via
TransactionSynchronizationManager.registerSynchronization or equivalent) that
calls caseRealtimeService.publishCaseEvent(...) in its afterCommit()
implementation, ensuring any variables used (caseEntity.getId(), event name,
message) are captured or final/effectively final for use in the callback.
| <script> | ||
| (function () { | ||
| let source = null; | ||
| let reconnectTimer = null; | ||
| let lastRefreshAt = 0; | ||
|
|
||
| function refreshPage() { | ||
| const now = Date.now(); | ||
| if (now - lastRefreshAt < 800) return; | ||
| lastRefreshAt = now; | ||
| window.location.reload(); | ||
| } | ||
|
|
||
| function connect() { | ||
| if (source) { | ||
| source.close(); | ||
| } | ||
|
|
||
| source = new EventSource('/ui/realtime/cases/stream'); | ||
| source.onmessage = refreshPage; | ||
| source.addEventListener('keepalive', function () {}); | ||
| source.addEventListener('connected', function () {}); | ||
| source.addEventListener('case-created', refreshPage); | ||
| source.addEventListener('case-updated', refreshPage); | ||
| source.addEventListener('assignment-updated', refreshPage); | ||
| source.addEventListener('status-changed', refreshPage); | ||
| source.addEventListener('note-added', refreshPage); | ||
| source.addEventListener('document-added', refreshPage); | ||
| source.addEventListener('document-deleted', refreshPage); | ||
|
|
||
| source.onerror = function () { | ||
| if (source) { | ||
| source.close(); | ||
| source = null; | ||
| } | ||
| if (reconnectTimer) return; | ||
| reconnectTimer = setTimeout(function () { | ||
| reconnectTimer = null; | ||
| connect(); | ||
| }, 2000); | ||
| }; | ||
| } | ||
|
|
||
| connect(); | ||
| window.addEventListener('beforeunload', function () { | ||
| if (source) source.close(); | ||
| }); | ||
| })(); | ||
| </script> |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does the browser EventSource API automatically reconnect on error, and how does it choose the retry interval?
💡 Result:
Yes, the browser EventSource API automatically reconnects on errors such as network failures. It continues attempting reconnection indefinitely until explicitly closed via EventSource.close or the page is unloaded, unless it receives an HTTP 204 No Content response from the server. The retry interval starts as an implementation-defined value (typically around 3 seconds, e.g., Chrome's defaultReconnectDelay = 3000ms). Servers can override this by including a "retry:" field in an event message (e.g., "retry: 10000" for 10 seconds), parsed as an integer in milliseconds. Invalid retry values are ignored, reverting to the previous or default value. The WHATWG HTML spec confirms reconnection on network errors and retry field parsing.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events/Using_server-sent_events
- 2: https://developer.mozilla.org/en-US/docs/Web/API/EventSource
- 3: https://html.spec.whatwg.org/multipage/server-sent-events.html
- 4: https://html.spec.whatwg.org/dev/server-sent-events.html
- 5: https://chromium.googlesource.com/chromium/blink/+/HEAD/Source/core/page/EventSource.cpp?autodive=0%2F%2F%2F
- 6: https://stackoverflow.com/questions/24564030/is-an-eventsource-sse-supposed-to-try-to-reconnect-indefinitely
- 7: https://stackoverflow.com/questions/21831206/eventsource-permanent-auto-reconnection
- 8: https://www.bookamed.com
SSE-triggered full-page reloads are heavy-handed and may thrash the server/UX.
A few concerns with this strategy:
-
Catch-all
onmessagewill reload on any un-named event. Since named handlers are already registered for every event type the backend emits,source.onmessage = refreshPageis a footgun — if the server ever sends a generic/default event (including default SpringSseEmitter.send(data)without a name), the page reloads for no semantic reason. Safer to make it a no-op and rely only on explicitaddEventListenercalls. -
Manual reconnect disables EventSource's native retry/backoff.
EventSourceautomatically reconnects on network errors with the browser's built-in backoff (defaulting to ~3 seconds, or whatever the server specifies via theretry:field). Closing the source inonerrorand scheduling a fixed 2000 ms reconnect replaces that with a tight fixed loop — on a sustained backend outage this pounds the server every 2 s forever, per open tab. Either let EventSource handle reconnection (drop the manual logic) or add exponential backoff with a cap. -
800 ms throttle is very short for full reloads. On a busy case-list page with multiple concurrent editors, each mutation triggers a reload; users will experience near-continuous page refreshes that destroy scroll position and any transient UI state. Consider re-fetching just the table fragment (e.g., Thymeleaf fragment +
fetch+ DOM swap) or at minimum increasing the throttle to 2–3 s and debouncing (collapsing a burst of events into one reload).
🔧 Minimal mitigation (safer onmessage + let EventSource reconnect)
- source = new EventSource('/ui/realtime/cases/stream');
- source.onmessage = refreshPage;
- source.addEventListener('keepalive', function () {});
+ source = new EventSource('/ui/realtime/cases/stream');
+ source.onmessage = function () {}; // ignore default/unnamed events
+ source.addEventListener('keepalive', function () {});
source.addEventListener('connected', function () {});
source.addEventListener('case-created', refreshPage);
...
- source.onerror = function () {
- if (source) {
- source.close();
- source = null;
- }
- if (reconnectTimer) return;
- reconnectTimer = setTimeout(function () {
- reconnectTimer = null;
- connect();
- }, 2000);
- };
+ // Rely on EventSource's built-in reconnect/backoff.📝 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.
| <script> | |
| (function () { | |
| let source = null; | |
| let reconnectTimer = null; | |
| let lastRefreshAt = 0; | |
| function refreshPage() { | |
| const now = Date.now(); | |
| if (now - lastRefreshAt < 800) return; | |
| lastRefreshAt = now; | |
| window.location.reload(); | |
| } | |
| function connect() { | |
| if (source) { | |
| source.close(); | |
| } | |
| source = new EventSource('/ui/realtime/cases/stream'); | |
| source.onmessage = refreshPage; | |
| source.addEventListener('keepalive', function () {}); | |
| source.addEventListener('connected', function () {}); | |
| source.addEventListener('case-created', refreshPage); | |
| source.addEventListener('case-updated', refreshPage); | |
| source.addEventListener('assignment-updated', refreshPage); | |
| source.addEventListener('status-changed', refreshPage); | |
| source.addEventListener('note-added', refreshPage); | |
| source.addEventListener('document-added', refreshPage); | |
| source.addEventListener('document-deleted', refreshPage); | |
| source.onerror = function () { | |
| if (source) { | |
| source.close(); | |
| source = null; | |
| } | |
| if (reconnectTimer) return; | |
| reconnectTimer = setTimeout(function () { | |
| reconnectTimer = null; | |
| connect(); | |
| }, 2000); | |
| }; | |
| } | |
| connect(); | |
| window.addEventListener('beforeunload', function () { | |
| if (source) source.close(); | |
| }); | |
| })(); | |
| </script> | |
| <script> | |
| (function () { | |
| let source = null; | |
| let reconnectTimer = null; | |
| let lastRefreshAt = 0; | |
| function refreshPage() { | |
| const now = Date.now(); | |
| if (now - lastRefreshAt < 800) return; | |
| lastRefreshAt = now; | |
| window.location.reload(); | |
| } | |
| function connect() { | |
| if (source) { | |
| source.close(); | |
| } | |
| source = new EventSource('/ui/realtime/cases/stream'); | |
| source.onmessage = function () {}; // ignore default/unnamed events | |
| source.addEventListener('keepalive', function () {}); | |
| source.addEventListener('connected', function () {}); | |
| source.addEventListener('case-created', refreshPage); | |
| source.addEventListener('case-updated', refreshPage); | |
| source.addEventListener('assignment-updated', refreshPage); | |
| source.addEventListener('status-changed', refreshPage); | |
| source.addEventListener('note-added', refreshPage); | |
| source.addEventListener('document-added', refreshPage); | |
| source.addEventListener('document-deleted', refreshPage); | |
| // Rely on EventSource's built-in reconnect/backoff. | |
| } | |
| connect(); | |
| window.addEventListener('beforeunload', function () { | |
| if (source) source.close(); | |
| }); | |
| })(); | |
| </script> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/templates/cases/list.html` around lines 53 - 101, The page
currently reloads on any unnamed SSE (remove the footgun), manually overrides
EventSource backoff (stop pounding the server), and reloads too frequently
(throttle too short); fix by removing the catch-all assignment source.onmessage
= refreshPage and rely only on the explicit addEventListener(...) handlers,
change source.onerror to not close the EventSource or schedule a manual
reconnect (let the browser's EventSource handle retries), and increase the
refresh throttle in refreshPage by raising lastRefreshAt threshold from 800 to
~3000 ms (or implement a short debounce) so bursts of events collapse into a
single reload; reference symbols: source.onmessage, addEventListener(...),
source.onerror, reconnectTimer, connect(), refreshPage, lastRefreshAt.
2 participants
Summary by CodeRabbit
Release Notes
New Features
Chores