marcel/familienarchiv
1
0
Fork 0
Code Issues 102 Pull Requests Actions Packages Projects Releases Wiki Activity

feat: notifications, @mentions, and comment deep-links (#71 #72 #73) #127

Merged
marcel merged 19 commits from feat/71-72-73-notifications-mentions-deeplinks into main 2026-03-28 16:06:59 +01:00
Conversation 12 Commits 19 Files Changed 54 +47 -13
7 changed files with 47 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit d13422c65a - Show all commits

1
backend/src/main/java/org/raddatz/familienarchiv/controller/NotificationController.java
View File

@@ -19,7 +19,6 @@ import java.util.UUID;
@RestController @RestController
@RequiredArgsConstructor @RequiredArgsConstructor
@RequirePermission(Permission.READ_ALL)
public class NotificationController { public class NotificationController {
private final NotificationService notificationService; private final NotificationService notificationService;

1
backend/src/main/java/org/raddatz/familienarchiv/dto/NotificationDTO.java
View File

@@ -10,6 +10,7 @@ public record NotificationDTO(
NotificationType type, NotificationType type,
UUID documentId, UUID documentId,
UUID referenceId, UUID referenceId,
UUID annotationId,
boolean read, boolean read,
LocalDateTime createdAt, LocalDateTime createdAt,
String actorName String actorName

3
backend/src/main/java/org/raddatz/familienarchiv/model/Notification.java
View File

@@ -37,6 +37,9 @@ public class Notification {
@Column(name = "reference_id") @Column(name = "reference_id")
private UUID referenceId; private UUID referenceId;
@Column(name = "annotation_id")
private UUID annotationId;
@Column(nullable = false) @Column(nullable = false)
@Builder.Default @Builder.Default
@Schema(requiredMode = Schema.RequiredMode.REQUIRED) @Schema(requiredMode = Schema.RequiredMode.REQUIRED)

3
backend/src/main/java/org/raddatz/familienarchiv/service/NotificationService.java
View File

@@ -55,6 +55,7 @@ public class NotificationService {
.type(NotificationType.REPLY) .type(NotificationType.REPLY)
.documentId(reply.getDocumentId()) .documentId(reply.getDocumentId())
.referenceId(reply.getId()) .referenceId(reply.getId())
.annotationId(reply.getAnnotationId())
marcel commented 2026-03-27 22:11:31 +01:00
Outdated
Review
Copy Link

N+1: findById inside a loop.

For every participant ID you're issuing a separate SELECT. If a thread has 10 participants that's 10 round-trips to the database before a single notification is written.

Replace the loop body with a single bulk fetch:

public void notifyReply(DocumentComment reply, DocumentComment root) {
    Set<UUID> participantIds = collectParticipantIds(root);
    participantIds.remove(reply.getAuthorId());
    if (participantIds.isEmpty()) return;

    List<AppUser> recipients = userRepository.findAllById(participantIds);
    for (AppUser recipient : recipients) {
        Notification notification = Notification.builder()
                .recipient(recipient)
                .type(NotificationType.REPLY)
                .documentId(reply.getDocumentId())
                .referenceId(reply.getId())
                .actorName(reply.getAuthorName())
                .build();
        notificationRepository.save(notification);
        if (recipient.isNotifyOnReply()) {
            sendNotificationEmail(recipient, reply, NotificationType.REPLY);
        }
    }
}

Same pattern applies to notifyMentions below.

**N+1: `findById` inside a loop.** For every participant ID you're issuing a separate SELECT. If a thread has 10 participants that's 10 round-trips to the database before a single notification is written. Replace the loop body with a single bulk fetch: ```java public void notifyReply(DocumentComment reply, DocumentComment root) { Set<UUID> participantIds = collectParticipantIds(root); participantIds.remove(reply.getAuthorId()); if (participantIds.isEmpty()) return; List<AppUser> recipients = userRepository.findAllById(participantIds); for (AppUser recipient : recipients) { Notification notification = Notification.builder() .recipient(recipient) .type(NotificationType.REPLY) .documentId(reply.getDocumentId()) .referenceId(reply.getId()) .actorName(reply.getAuthorName()) .build(); notificationRepository.save(notification); if (recipient.isNotifyOnReply()) { sendNotificationEmail(recipient, reply, NotificationType.REPLY); } } } ``` Same pattern applies to `notifyMentions` below.
.actorName(reply.getAuthorName()) .actorName(reply.getAuthorName())
.build(); .build();
notificationRepository.save(notification); notificationRepository.save(notification);
@@ -80,6 +81,7 @@ public class NotificationService {
.type(NotificationType.MENTION) .type(NotificationType.MENTION)
.documentId(comment.getDocumentId()) .documentId(comment.getDocumentId())
marcel commented 2026-03-27 22:11:32 +01:00
Review
Copy Link

N+1: same issue as notifyReply — findById in a loop.

For notifyMentions:

List<AppUser> recipients = userRepository.findAllById(mentionedUserIds);
for (AppUser recipient : recipients) { ... }

findAllById emits a single SELECT … WHERE id IN (…) instead of one query per user.

**N+1: same issue as `notifyReply` — `findById` in a loop.** For `notifyMentions`: ```java List<AppUser> recipients = userRepository.findAllById(mentionedUserIds); for (AppUser recipient : recipients) { ... } ``` `findAllById` emits a single `SELECT … WHERE id IN (…)` instead of one query per user.
.referenceId(comment.getId()) .referenceId(comment.getId())
.annotationId(comment.getAnnotationId())
.actorName(comment.getAuthorName()) .actorName(comment.getAuthorName())
.build(); .build();
notificationRepository.save(notification); notificationRepository.save(notification);
@@ -129,6 +131,7 @@ public class NotificationService {
n.getType(), n.getType(),
n.getDocumentId(), n.getDocumentId(),
n.getReferenceId(), n.getReferenceId(),
n.getAnnotationId(),
n.isRead(), n.isRead(),
n.getCreatedAt(), n.getCreatedAt(),
n.getActorName() n.getActorName()

1
backend/src/main/resources/db/migration/V19__add_annotation_id_to_notifications.sql Normal file
View File

@@ -0,0 +1 @@
ALTER TABLE notifications ADD COLUMN annotation_id UUID;

20
backend/src/test/java/org/raddatz/familienarchiv/controller/NotificationControllerTest.java
View File

@@ -53,9 +53,14 @@ class NotificationControllerTest {
@Test @Test
@WithMockUser(username = "testuser") @WithMockUser(username = "testuser")
void getNotifications_returns403_whenUserLacksPermission() throws Exception { void getNotifications_returns200_whenAuthenticatedWithNoPermissions() throws Exception {
AppUser user = AppUser.builder().id(USER_ID).username("testuser").build();
when(userService.findByUsername("testuser")).thenReturn(user);
when(notificationService.getNotifications(eq(USER_ID), any()))
.thenReturn(new PageImpl<>(List.of()));
mockMvc.perform(get("/api/notifications")) mockMvc.perform(get("/api/notifications"))
.andExpect(status().isForbidden()); .andExpect(status().isOk());
} }
@Test @Test
@@ -64,7 +69,7 @@ class NotificationControllerTest {
AppUser user = AppUser.builder().id(USER_ID).username("testuser").build(); AppUser user = AppUser.builder().id(USER_ID).username("testuser").build();
NotificationDTO dto = new NotificationDTO( NotificationDTO dto = new NotificationDTO(
UUID.randomUUID(), NotificationType.REPLY, UUID.randomUUID(), UUID.randomUUID(), NotificationType.REPLY, UUID.randomUUID(),
UUID.randomUUID(), false, LocalDateTime.now(), "Anna Smith"); UUID.randomUUID(), null, false, LocalDateTime.now(), "Anna Smith");
when(userService.findByUsername("testuser")).thenReturn(user); when(userService.findByUsername("testuser")).thenReturn(user);
when(notificationService.getNotifications(eq(USER_ID), any())) when(notificationService.getNotifications(eq(USER_ID), any()))
@@ -185,9 +190,14 @@ class NotificationControllerTest {
@Test @Test
@WithMockUser(username = "testuser", authorities = {"WRITE_ALL"}) @WithMockUser(username = "testuser", authorities = {"WRITE_ALL"})
void getNotifications_returns403_whenUserHasOnlyWriteAll() throws Exception { void getNotifications_returns200_whenUserHasOnlyWriteAll() throws Exception {
AppUser user = AppUser.builder().id(USER_ID).username("testuser").build();
when(userService.findByUsername("testuser")).thenReturn(user);
when(notificationService.getNotifications(eq(USER_ID), any()))
.thenReturn(new PageImpl<>(List.of()));
mockMvc.perform(get("/api/notifications")) mockMvc.perform(get("/api/notifications"))
.andExpect(status().isForbidden()); .andExpect(status().isOk());
} }
// ─── PUT /api/users/me/notification-preferences ────────────────────────── // ─── PUT /api/users/me/notification-preferences ──────────────────────────

31
frontend/src/routes/profile/+page.svelte
View File

@@ -1,14 +1,14 @@
<script lang="ts"> <script lang="ts">
import { enhance } from '$app/forms'; import { enhance } from '$app/forms';
import { untrack } from 'svelte';
import { m } from '$lib/paraglide/messages.js'; import { m } from '$lib/paraglide/messages.js';
import PersonalInfoForm from './PersonalInfoForm.svelte'; import PersonalInfoForm from './PersonalInfoForm.svelte';
import PasswordChangeForm from './PasswordChangeForm.svelte'; import PasswordChangeForm from './PasswordChangeForm.svelte';
let { data, form } = $props(); let { data, form } = $props();
let notifyOnReply = $state(untrack(() => data.notificationPrefs?.notifyOnReply ?? false)); let notifyOnReply = $derived(data.notificationPrefs?.notifyOnReply ?? false);
let notifyOnMention = $state(untrack(() => data.notificationPrefs?.notifyOnMention ?? false)); let notifyOnMention = $derived(data.notificationPrefs?.notifyOnMention ?? false);
const hasEmail = $derived(!!data.user?.email);
</script> </script>
<div class="mx-auto max-w-7xl px-4 py-8 sm:px-6 lg:px-8"> <div class="mx-auto max-w-7xl px-4 py-8 sm:px-6 lg:px-8">
@@ -53,32 +53,49 @@ let notifyOnMention = $state(untrack(() => data.notificationPrefs?.notifyOnMenti
</div> </div>
{/if} {/if}
<form method="POST" action="?/updateNotificationPrefs" use:enhance> <form
method="POST"
action="?/updateNotificationPrefs"
use:enhance={() => async ({ update }) => update({ reset: false })}
>
<div class="space-y-4"> <div class="space-y-4">
<label class="flex cursor-pointer items-start gap-3"> <label
class="flex items-start gap-3 {hasEmail ? 'cursor-pointer' : 'cursor-not-allowed opacity-40'}"
>
<input <input
type="checkbox" type="checkbox"
name="notifyOnReply" name="notifyOnReply"
bind:checked={notifyOnReply} bind:checked={notifyOnReply}
disabled={!hasEmail}
class="mt-0.5 h-4 w-4 rounded border-line accent-primary" class="mt-0.5 h-4 w-4 rounded border-line accent-primary"
/> />
<span class="text-sm text-ink">{m.notification_pref_reply()}</span> <span class="text-sm text-ink">{m.notification_pref_reply()}</span>
</label> </label>
<label class="flex cursor-pointer items-start gap-3"> <label
class="flex items-start gap-3 {hasEmail ? 'cursor-pointer' : 'cursor-not-allowed opacity-40'}"
>
<input <input
type="checkbox" type="checkbox"
name="notifyOnMention" name="notifyOnMention"
bind:checked={notifyOnMention} bind:checked={notifyOnMention}
disabled={!hasEmail}
class="mt-0.5 h-4 w-4 rounded border-line accent-primary" class="mt-0.5 h-4 w-4 rounded border-line accent-primary"
/> />
<span class="text-sm text-ink">{m.notification_pref_mention()}</span> <span class="text-sm text-ink">{m.notification_pref_mention()}</span>
</label> </label>
</div> </div>
{#if !hasEmail}
<p class="mt-3 text-xs text-ink-3">
{m.notification_prefs_no_email()}
</p>
{/if}
<button <button
type="submit" type="submit"
class="mt-5 rounded-sm bg-primary px-5 py-2 font-sans text-xs font-bold tracking-widest text-primary-fg uppercase transition-opacity hover:opacity-80" disabled={!hasEmail}
class="mt-5 rounded-sm bg-primary px-5 py-2 font-sans text-xs font-bold tracking-widest text-primary-fg uppercase transition-opacity {hasEmail ? 'hover:opacity-80' : 'cursor-not-allowed opacity-40'}"
> >
{m.btn_save()} {m.btn_save()}
</button> </button>