From f68c8921709705b6a9f3abc412f5d4def420d002 Mon Sep 17 00:00:00 2001 From: Marcel Date: Mon, 20 Apr 2026 18:12:16 +0200 Subject: [PATCH] fix(chronik): sentinel-based title split + drop duplicate comment preview MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two PR #288 blockers from Felix and Leonie: Felix: verbText.indexOf(docTitle) broke when the title was empty (indexOf returned 0, the before/after slices both emptied) or when the title substring-matched any word in the compiled Paraglide message (e.g. "Brief" appearing inside a translated verb). Swap to a sentinel approach: interpolate {doc} with U+0001, then split the compiled text on that sentinel — robust regardless of title content or translator sentence order. Two new red tests lock the invariant: empty title still renders the row link; short titles that could substring-match render exactly once as a single chronik-doc-title span. Leonie: the comment variant rendered „{documentTitle}" as a placeholder, which made the row show the same title twice — once as the underlined link, once as the italic "preview quote" — implying the comment was quoting itself. Replace with an italic ellipsis „…". A new red test asserts the preview no longer contains the document title text verbatim. While here, add a SECURITY comment next to the TODO so the next person who wires item.commentPreview knows the backend must truncate/strip server-side and the frontend must use {text}, never {@html} (Nora, issue #285 #3552). Part of #285, address PR #288 review. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../lib/components/chronik/ChronikRow.svelte | 36 +++++++++++-------- .../chronik/ChronikRow.svelte.spec.ts | 33 +++++++++++++++++ 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/frontend/src/lib/components/chronik/ChronikRow.svelte b/frontend/src/lib/components/chronik/ChronikRow.svelte index a739f6db..1df3c75e 100644 --- a/frontend/src/lib/components/chronik/ChronikRow.svelte +++ b/frontend/src/lib/components/chronik/ChronikRow.svelte @@ -68,13 +68,18 @@ function formatTimeHHMM(iso: string): string { const actorName: string = $derived(item.actor?.name ?? item.actor?.initials ?? '?'); const docTitle: string = $derived(item.documentTitle); -// The visible verb phrase (split so we can render the document title as a styled span). -// We compute the text once and then split it on the document title string — a pragmatic -// choice that lets translators keep the sentence natural while we still style the doc link. +// We split the translated verb around the document title so the title can be +// rendered as a styled inside the without nesting anchors. Using a +// non-printable sentinel (U+0001) as the {doc} interpolation value lets us +// split the compiled message regardless of what the actual title contains — +// empty strings, short substrings that also appear in the verb, and any +// translator sentence order all work without special cases. +const SENTINEL = '\u0001'; + const verbText: string = $derived( variant === 'rollup' - ? verbRollup(item.kind, actorName, docTitle, item.count) - : verbSingleton(item.kind, actorName, docTitle) + ? verbRollup(item.kind, actorName, SENTINEL, item.count) + : verbSingleton(item.kind, actorName, SENTINEL) ); const timeLabel: string = $derived( @@ -83,13 +88,12 @@ const timeLabel: string = $derived( : relativeTime(item.happenedAt) ); -// Split the verb text around the document title so the title can be styled without nesting anchors. const verbParts: { before: string; after: string } = $derived.by(() => { - const idx = verbText.indexOf(docTitle); + const idx = verbText.indexOf(SENTINEL); if (idx === -1) return { before: verbText, after: '' }; return { before: verbText.slice(0, idx), - after: verbText.slice(idx + docTitle.length) + after: verbText.slice(idx + SENTINEL.length) }; }); @@ -133,8 +137,9 @@ const verbParts: { before: string; after: string } = $derived.by(() => {

- {verbParts.before}{docTitle}{docTitle}{verbParts.after} {#if variant === 'rollup'} { {#if variant === 'comment'}

- „{docTitle}“ + „…“

{/if} diff --git a/frontend/src/lib/components/chronik/ChronikRow.svelte.spec.ts b/frontend/src/lib/components/chronik/ChronikRow.svelte.spec.ts index 41800bf8..a4234361 100644 --- a/frontend/src/lib/components/chronik/ChronikRow.svelte.spec.ts +++ b/frontend/src/lib/components/chronik/ChronikRow.svelte.spec.ts @@ -118,4 +118,37 @@ describe('ChronikRow', () => { const preview = document.querySelector('[data-testid="chronik-comment-preview"]'); expect(preview).not.toBeNull(); }); + + it('comment preview does NOT duplicate the document title verbatim', async () => { + // Leonie: user sees the title twice otherwise — looks like the comment is quoting itself. + // Until the backend exposes item.commentPreview, the placeholder must be distinct. + const item: ActivityFeedItemDTO = { + ...baseItem, + kind: 'COMMENT_ADDED', + documentTitle: 'Brief vom 12. Juli 1920' + }; + render(ChronikRow, { item }); + const preview = document.querySelector('[data-testid="chronik-comment-preview"]'); + expect(preview).not.toBeNull(); + expect(preview?.textContent).not.toContain('Brief vom 12. Juli 1920'); + }); + + // --- robustness: title rendering for edge cases --- + it('still renders the row link when documentTitle is an empty string', async () => { + // Felix: verbText.indexOf(docTitle) returned 0 for empty titles — the span + // collapsed and before/after both emptied out. Swap to a sentinel-based + // approach so this case renders like every other row. + const empty: ActivityFeedItemDTO = { ...baseItem, documentTitle: '' }; + render(ChronikRow, { item: empty }); + const link = document.querySelector('a[href="/documents/doc-1"]'); + expect(link).not.toBeNull(); + }); + + it('renders a short document title that could substring-match the verb', async () => { + const short: ActivityFeedItemDTO = { ...baseItem, documentTitle: 'Brief' }; + render(ChronikRow, { item: short }); + const titleEls = document.querySelectorAll('[data-testid="chronik-doc-title"]'); + expect(titleEls.length).toBe(1); + expect(titleEls[0].textContent).toBe('Brief'); + }); });