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

fix(chronik): sentinel-based title split + drop duplicate comment preview

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
Marcel
2026-04-20 18:12:16 +02:00
committed by marcel
parent f0b21e226e
commit f68c892170
2 changed files with 55 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

36
frontend/src/lib/components/chronik/ChronikRow.svelte
View File

@@ -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 <span> inside the <a> 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)
};
});
</script>
@@ -133,8 +137,9 @@ const verbParts: { before: string; after: string } = $derived.by(() => {
<!-- Body -->
<div class="min-w-0 flex-1">
<p class="font-sans text-sm leading-snug text-ink">
{verbParts.before}<span class="underline decoration-accent underline-offset-2"
>{docTitle}</span
{verbParts.before}<span
data-testid="chronik-doc-title"
class="underline decoration-accent underline-offset-2">{docTitle}</span
>{verbParts.after}
{#if variant === 'rollup'}
<span
@@ -149,15 +154,18 @@ const verbParts: { before: string; after: string } = $derived.by(() => {
{#if variant === 'comment'}
<!--
TODO: the backend does not yet expose a comment body preview on
ActivityFeedItemDTO. For now we render the document title as a placeholder
so the comment variant structure is already in place; swap this to
item.commentPreview (or similar) once the DTO gains the field.
ActivityFeedItemDTO. Render an ellipsis placeholder until it does —
duplicating the document title here looks like the comment is
quoting itself (Leonie, PR #288 review).
SECURITY: once item.commentPreview lands, render via {text}, never
{@html}. The backend must truncate and strip tags server-side (Nora,
issue #285 comment #3552).
-->
<p
data-testid="chronik-comment-preview"
class="mt-1 line-clamp-1 font-serif text-sm text-ink-2 italic sm:line-clamp-2"
>
&bdquo;{docTitle}&ldquo;
&bdquo;&hellip;&ldquo;
</p>
{/if}

33
frontend/src/lib/components/chronik/ChronikRow.svelte.spec.ts
View File

@@ -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');
});
});