fix(#71-#73): address all review findings from Markus and Sara
BLOCKERs: - Remove direct AppUserRepository/CommentRepository access from CommentService and NotificationService — replaced with UserService.findAllById() and UserService (fixes layering contract from CLAUDE.md) - Switch Optional<JavaMailSender> constructor injection — removes @Autowired(required=false) field and ReflectionTestUtils hack in tests - Add @RequirePermission(READ_ALL) to UserSearchController — prevents user enumeration without read access Data bug: - Promote actorName from @Transient to persisted VARCHAR column (V18 migration) - Set actorName in notifyReply and notifyMentions from comment.getAuthorName() Architecture: - Add @RequirePermission(READ_ALL) to NotificationController - Introduce NotificationDTO — controller returns DTO instead of Notification entity, eliminating lazy-load N+1 and AppUser field leakage - Change mentions FetchType to EAGER — fixes LazyInitializationException outside transaction - Add @Transactional(propagation=REQUIRES_NEW) to notifyReply/notifyMentions so a notification failure cannot roll back the parent comment - N+1 fix: replace per-ID findById loops with single findAllById bulk fetch - Move collectParticipantIds to CommentService; notifyReply accepts Set<UUID> directly Security: - Escape displayName before injecting into renderBody HTML span - Replace <a href="#"> with <span class="mention"> — no profile page to link to, and the anchor's scroll-to-top behaviour is harmful Tests added/fixed: - markRead_throwsNotFound, markAllRead_delegatesToRepository, countUnread_delegatesToRepository - markOneRead_returns401, @RequirePermission 403 coverage for both controllers - postComment/replyToComment_triggersNotifyMentions_whenMentionedUserIdsProvided - search_returnsAtMostTenResults now asserts $.length() <= 10 - XSS regression test for escaped displayName in mention.spec.ts Frontend minors: - relativeTime() uses Intl.RelativeTimeFormat (locale-aware, not German-hardcoded) - aria-label uses m.notification_unread() Paraglide key (de/en/es added) - <div role="button"> replaced with <button> (native Enter+Space handling) - onDestroy clears debounceTimer in MentionEditor - setTimeout(100) replaced with await tick() + requestAnimationFrame in CommentThread - Notification prefs form uses checkbox name attributes + formData.has() pattern Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -304,6 +304,7 @@
|
||||
"notification_prefs_heading": "Benachrichtigungen",
|
||||
"notification_pref_reply": "E-Mail, wenn jemand auf meinen Kommentar antwortet",
|
||||
"notification_pref_mention": "E-Mail, wenn jemand mich in einem Kommentar erwähnt",
|
||||
"notification_unread": "ungelesen",
|
||||
"mention_btn_label": "Person erwähnen",
|
||||
"mention_popup_empty": "Keine Nutzer gefunden"
|
||||
}
|
||||
|
||||
@@ -304,6 +304,7 @@
|
||||
"notification_prefs_heading": "Notifications",
|
||||
"notification_pref_reply": "Email when someone replies to my comment",
|
||||
"notification_pref_mention": "Email when someone mentions me in a comment",
|
||||
"notification_unread": "unread",
|
||||
"mention_btn_label": "Mention person",
|
||||
"mention_popup_empty": "No users found"
|
||||
}
|
||||
|
||||
@@ -304,6 +304,7 @@
|
||||
"notification_prefs_heading": "Notificaciones",
|
||||
"notification_pref_reply": "Correo cuando alguien responde a mi comentario",
|
||||
"notification_pref_mention": "Correo cuando alguien me menciona en un comentario",
|
||||
"notification_unread": "no leído",
|
||||
"mention_btn_label": "Mencionar persona",
|
||||
"mention_popup_empty": "No se encontraron usuarios"
|
||||
}
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
<script lang="ts">
|
||||
import { onMount, untrack } from 'svelte';
|
||||
import { onMount, tick, untrack } from 'svelte';
|
||||
import { m } from '$lib/paraglide/messages.js';
|
||||
import type { Comment, CommentReply } from '$lib/types';
|
||||
import MentionEditor from '$lib/components/MentionEditor.svelte';
|
||||
@@ -180,7 +180,7 @@ function cancelReply() {
|
||||
replyText = '';
|
||||
}
|
||||
|
||||
onMount(() => {
|
||||
onMount(async () => {
|
||||
if (loadOnMount) {
|
||||
reload();
|
||||
} else {
|
||||
@@ -189,11 +189,11 @@ onMount(() => {
|
||||
}
|
||||
|
||||
if (targetCommentId) {
|
||||
// Scroll to target after a tick so the DOM is settled
|
||||
setTimeout(() => {
|
||||
await tick();
|
||||
requestAnimationFrame(() => {
|
||||
const el = document.querySelector(`[data-comment-id="${targetCommentId}"]`);
|
||||
el?.scrollIntoView({ behavior: 'smooth', block: 'center' });
|
||||
}, 100);
|
||||
});
|
||||
|
||||
// Remove highlight on first user interaction
|
||||
const clearHighlight = () => {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
<script lang="ts">
|
||||
import { onDestroy } from 'svelte';
|
||||
import { detectMention } from '$lib/utils/mention';
|
||||
import type { MentionDTO } from '$lib/types';
|
||||
import { m } from '$lib/paraglide/messages.js';
|
||||
@@ -180,6 +181,8 @@ function handleAtButtonClick() {
|
||||
}, 0);
|
||||
}
|
||||
|
||||
onDestroy(() => clearTimeout(debounceTimer));
|
||||
|
||||
const popupOpen = $derived(query !== null);
|
||||
</script>
|
||||
|
||||
|
||||
@@ -117,16 +117,15 @@ function attachClickOutside(node: HTMLElement) {
|
||||
}
|
||||
|
||||
function relativeTime(isoString: string): string {
|
||||
const now = Date.now();
|
||||
const then = new Date(isoString).getTime();
|
||||
const diffMs = now - then;
|
||||
const diffMs = Date.now() - new Date(isoString).getTime();
|
||||
const diffMin = Math.floor(diffMs / 60000);
|
||||
if (diffMin < 1) return 'gerade eben';
|
||||
if (diffMin < 60) return `vor ${diffMin} Min.`;
|
||||
const rtf = new Intl.RelativeTimeFormat(undefined, { numeric: 'auto' });
|
||||
if (diffMin < 1) return rtf.format(0, 'minute');
|
||||
if (diffMin < 60) return rtf.format(-diffMin, 'minute');
|
||||
const diffH = Math.floor(diffMin / 60);
|
||||
if (diffH < 24) return `vor ${diffH} Std.`;
|
||||
if (diffH < 24) return rtf.format(-diffH, 'hour');
|
||||
const diffD = Math.floor(diffH / 24);
|
||||
return `vor ${diffD} Tag${diffD !== 1 ? 'en' : ''}`;
|
||||
return rtf.format(-diffD, 'day');
|
||||
}
|
||||
|
||||
onMount(() => {
|
||||
@@ -232,12 +231,10 @@ onDestroy(() => {
|
||||
<ul role="list">
|
||||
{#each notifications as notification (notification.id)}
|
||||
<li>
|
||||
<div
|
||||
role="button"
|
||||
tabindex="0"
|
||||
<button
|
||||
type="button"
|
||||
onclick={() => markRead(notification)}
|
||||
onkeydown={(e) => e.key === 'Enter' && markRead(notification)}
|
||||
class="flex cursor-pointer items-start gap-3 border-b border-line px-4 py-3 last:border-b-0 hover:bg-canvas
|
||||
class="flex w-full cursor-pointer items-start gap-3 border-b border-line px-4 py-3 text-left last:border-b-0 hover:bg-canvas
|
||||
{!notification.read ? 'bg-accent-bg/20' : ''}"
|
||||
>
|
||||
<!-- Type icon -->
|
||||
@@ -291,10 +288,10 @@ onDestroy(() => {
|
||||
{#if !notification.read}
|
||||
<span
|
||||
class="mt-1.5 h-2 w-2 shrink-0 rounded-full bg-primary"
|
||||
aria-label="ungelesen"
|
||||
aria-label={m.notification_unread()}
|
||||
></span>
|
||||
{/if}
|
||||
</div>
|
||||
</button>
|
||||
</li>
|
||||
{/each}
|
||||
</ul>
|
||||
|
||||
@@ -92,10 +92,10 @@ describe('renderBody', () => {
|
||||
expect(result).toContain('AT&T');
|
||||
});
|
||||
|
||||
it('wraps @mention in an anchor tag', () => {
|
||||
it('wraps @mention in a mention span', () => {
|
||||
const mentions: MentionDTO[] = [{ id: 'uuid-1', firstName: 'Hans', lastName: 'Müller' }];
|
||||
const result = renderBody('Hey @Hans Müller!', mentions);
|
||||
expect(result).toContain('<a');
|
||||
expect(result).toContain('<span');
|
||||
expect(result).toContain('Hans Müller');
|
||||
});
|
||||
|
||||
@@ -108,8 +108,15 @@ describe('renderBody', () => {
|
||||
it('replaces all occurrences of the same mention', () => {
|
||||
const mentions: MentionDTO[] = [{ id: 'uuid-1', firstName: 'Hans', lastName: 'Müller' }];
|
||||
const result = renderBody('@Hans Müller and @Hans Müller', mentions);
|
||||
const linkCount = (result.match(/<a /g) ?? []).length;
|
||||
expect(linkCount).toBe(2);
|
||||
const spanCount = (result.match(/<span /g) ?? []).length;
|
||||
expect(spanCount).toBe(2);
|
||||
});
|
||||
|
||||
it('escapes HTML special chars in mention display names', () => {
|
||||
const mentions: MentionDTO[] = [{ id: 'u1', firstName: '<script>', lastName: 'alert(1)' }];
|
||||
const result = renderBody('@<script> alert(1)', mentions);
|
||||
expect(result).not.toContain('<script>');
|
||||
expect(result).toContain('<script>');
|
||||
});
|
||||
|
||||
it('converts newlines to <br>', () => {
|
||||
|
||||
@@ -59,8 +59,13 @@ export function renderBody(content: string, mentions: MentionDTO[]): string {
|
||||
|
||||
for (const mention of mentions) {
|
||||
const displayName = `${mention.firstName} ${mention.lastName}`.trim();
|
||||
const link = `<a class="mention" data-user-id="${mention.id}" href="#">@${displayName}</a>`;
|
||||
escaped = escaped.replaceAll(`@${displayName}`, link);
|
||||
const escapedDisplayName = displayName
|
||||
.replaceAll('&', '&')
|
||||
.replaceAll('<', '<')
|
||||
.replaceAll('>', '>')
|
||||
.replaceAll('"', '"');
|
||||
const span = `<span class="mention" data-user-id="${mention.id}">@${escapedDisplayName}</span>`;
|
||||
escaped = escaped.replaceAll(`@${escapedDisplayName}`, span);
|
||||
}
|
||||
|
||||
return escaped.replaceAll('\n', '<br>');
|
||||
|
||||
@@ -60,8 +60,8 @@ export const actions: Actions = {
|
||||
updateNotificationPrefs: async ({ request, fetch }) => {
|
||||
const formData = await request.formData();
|
||||
const body = {
|
||||
notifyOnReply: formData.get('notifyOnReply') === 'true',
|
||||
notifyOnMention: formData.get('notifyOnMention') === 'true'
|
||||
notifyOnReply: formData.has('notifyOnReply'),
|
||||
notifyOnMention: formData.has('notifyOnMention')
|
||||
};
|
||||
|
||||
const res = await fetch(`${apiBase()}/api/users/me/notification-preferences`, {
|
||||
|
||||
@@ -54,13 +54,11 @@ let notifyOnMention = $state(untrack(() => data.notificationPrefs?.notifyOnMenti
|
||||
{/if}
|
||||
|
||||
<form method="POST" action="?/updateNotificationPrefs" use:enhance>
|
||||
<input type="hidden" name="notifyOnReply" value={notifyOnReply} />
|
||||
<input type="hidden" name="notifyOnMention" value={notifyOnMention} />
|
||||
|
||||
<div class="space-y-4">
|
||||
<label class="flex cursor-pointer items-start gap-3">
|
||||
<input
|
||||
type="checkbox"
|
||||
name="notifyOnReply"
|
||||
bind:checked={notifyOnReply}
|
||||
class="mt-0.5 h-4 w-4 rounded border-line accent-primary"
|
||||
/>
|
||||
@@ -70,6 +68,7 @@ let notifyOnMention = $state(untrack(() => data.notificationPrefs?.notifyOnMenti
|
||||
<label class="flex cursor-pointer items-start gap-3">
|
||||
<input
|
||||
type="checkbox"
|
||||
name="notifyOnMention"
|
||||
bind:checked={notifyOnMention}
|
||||
class="mt-0.5 h-4 w-4 rounded border-line accent-primary"
|
||||
/>
|
||||
|
||||
Reference in New Issue
Block a user