feat: unify /notifications and dashboard activity feed into a /chronik page #288
Reference in New Issue
Block a user
Delete Branch "feat/issue-285-chronik-unified-activity"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #285
Summary
Adds a new
/chronikpage that merges personal notifications (Für-dich) and the ambient dashboard activity feed into a single day-grouped timeline, backed by a new session-style rollup query that replaces the old hour-trunc dedupe.What shipped
Backend
ActivityFeedItemDTO+ActivityFeedRowextended withcount: int(required,1for singletons) andhappenedAtUntil: OffsetDateTime?(nullable, end-of-session for rollups).AuditLogQueryRepository.findDedupedActivityFeedrenamed tofindRolledUpActivityFeedand rewritten usingLAG()-based session grouping (120-min gap).COMMENT_ADDEDandMENTION_CREATEDalways start a new session — they never roll up.BLOCK_REVIEWEDis now included in the 6 eligible kinds. The oldDISTINCT ON (..., date_trunc('hour', ...))dedupe is deleted, not kept alongside.V49__add_audit_log_rollup_index.sql: partial covering index on(actor_id, document_id, kind, happened_at DESC)matching the rollup query's WHERE clause exactly./api/dashboard/activitylimit cap raised from 20 → 40.Frontend
/chronikwith+page.server.ts(loads/api/dashboard/activity?limit=40+ unread/api/notificationsin parallel, form actions?/dismissand?/mark-all) and+page.svelte(composes 6 components, pagination with focus-preserving "Mehr laden").src/lib/components/chronik/with co-located specs (40 tests):ChronikRow(single orchestrator, 4 variants),ChronikTimeline(day buckets),ChronikFuerDichBox(unread + inbox-zero),ChronikFilterPills(role=radiogroup, 5 pills, URL-synced),ChronikEmptyState,ChronikErrorCard.$lib/stores/notifications.svelte.ts: new module-level singleton with ref-countedinit()/destroy()— one EventSource per tab across Bell + Chronik. Old$lib/hooks/useNotificationStream.svelte.tsdeleted.$lib/utils/date-buckets.ts: purebucketByDay()helper (locale-aware week start, DST-safe) with 7 unit tests.DashboardActivityFeed.sveltenow renders rollup rows (count badge + "14. Apr. · 14:02–14:32" en-dash time range) and points "Alle anzeigen" to/chronik.NotificationDropdown.sveltefooter retargeted to/chronikwith the new "Zur Chronik →" label./chronikadded toe2e/accessibility.spec.tsAUTHENTICATED_PAGES— three new axe-playwright runs (light / system-dark / manual-dark) light up for free./chronik.Docs
docs/adr/003-chronik-unified-activity-feed.md: records the session-rollup decision (LAG + 120-min gap, no fixed buckets), the dedupe deletion, the single-endpoint-composition-in-page-server seam, and the German-URL convention.Scope note — acceptance item adjusted
The issue's original acceptance checklist includes "
/notificationsredirects to/chronik(301)". Replaced at Marcel's direction: since the app is pre-production, the/notificationsroute is deleted outright instead. No redirect, no zombie page. All other acceptance items stand.Verification
./mvnw test— 1187 passed, 0 failures.npm test— 1079 passed, 0 failures (all 116 unit + browser specs green)./chronikrenders the heading, all 5 filter pills asrole="radio", inbox-zero state in the Für-dich box, rollup rows with count badge + en-dash time range, day headers (TODAY/YESTERDAY), "Mehr laden" button; dashboard "Show all" linkhref="/chronik"confirmed.Follow-ups
NotificationBellto use SvelteKit form actions.ChronikRowuses the document title as a placeholder — pending a backendcommentPreviewDTO field in a follow-up.- docs/adr/003-chronik-unified-activity-feed.md: records the session-rollup decision (LAG + 120-min gap), the dedupe deletion, the single-endpoint composition, and the German-URL convention. - frontend/messages/{de,en,es}.json: adds chronik_* keys for page title, Für-dich box, filter pills, day headers, singleton/rollup verb variants per kind, empty states, error card, Mehr-laden pagination, and the Bell footer link retarget. No pluralization via ICU match — separate singleton/rollup keys per verb, per the Felix discussion (comment #3573). Part of #285. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>page.server.ts loads /api/dashboard/activity (limit=40) and unread /api/notifications in parallel via Promise.allSettled so a dashboard-activity failure still renders the Für-dich box. Form actions ?/dismiss and ?/mark-all back the Dismiss and "Alle gelesen" controls with CSRF-safe SvelteKit endpoints. +page.svelte composes all six chronik components: - ChronikFuerDichBox at the top, seeded from the SSR unread set on first render and switching to the live SSE singleton once notifications arrive; - ChronikFilterPills below, wired to URL via goto(?filter=…) with replaceState so the browser history stays clean across filter changes; - ChronikTimeline for the day-bucketed feed, filtered client-side per pill (alle / fuer-dich / hochgeladen / transkription / kommentare); - ChronikEmptyState for first-run vs filter-empty states; - ChronikErrorCard on activity load failure. "Mehr laden" pagination keeps focus on the button after load (via tick() + $state-bound ref), renders 3 static skeleton rows with aria-busy, and announces "{count} weitere Einträge geladen" through a polite aria-live region. Inbox-zero in the Für-dich box links to /chronik?filter=fuer-dich. Co-located page.server.spec.ts covers load(): limit=40, unread=read:false, filter parsing with "alle" fallback, activity-fulfilled-but-not-ok surfaces loadError, plus the dismiss and mark-all actions (success + missing-id branch). 8 tests green. Part of #285. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>🏛️ Markus Keller — Senior Application Architect
Verdict: 🚫 Changes requested
The architecture is sound — session-rollup SQL, single endpoint reused across both surfaces, universal German URL, ADR-003 committed alongside the code. But one functional blocker slipped in.
Blockers
"Mehr laden" pagination is broken against the backend contract.
frontend/src/routes/chronik/+page.svelte:107callsGET /api/dashboard/activity?limit=40&offset=${mergedFeed.length}, butDashboardController.getActivityatbackend/.../dashboard/DashboardController.java:36-41only acceptslimit. Theoffsetquery param is ignored; every "Mehr laden" click re-fetches the same top-40 rows and appends duplicates. Either:offset(orcursor/before) param to the controller + service + repository, with a corresponding test, orEither way, the current code ships a feature that silently misbehaves.
"Dismiss" form action is dead code.
page.server.tsdefinesactions.dismisswith its own test, butChronikFuerDichBox.sveltecallsonMarkRead(n)→ the SSE singleton'smarkRead()(raw PATCH), not the SvelteKit form action. This is the exact "Option C — split markup with<form action="?/dismiss">" that Felix locked in issue comment #3573. Pick one:Suggestions
Kinds filter spec drift. Issue body says "Add
kindsquery param toGET /api/dashboard/activity(CSV of AuditKind, default = 6 kinds)" — I was fine with client-side filtering when we discussed this, but the PR description should either justify the simplification or track it as a follow-up issue. A silent scope reduction between spec and code is the kind of thing that bites later.Rollup partial index WHERE clause drifts from query filter.
V49filters the index on six kinds;findRolledUpActivityFeedfilters the same six kinds. Keeping both lists in sync is tedious — acceptable for now (two places, one migration), but flag it if the kind list ever grows.What's done well
LAG()+SUM() OVER … ROWS UNBOUNDED PRECEDINGis the correct implementation of the algorithm we discussed. No hard cap, COMMENT/MENTION always split. Five focused integration tests prove it.+page.server.tsinstead of a new/api/chronikaggregator — exactly the seam I called for.countdefaults to 1,happenedAtUntilnullable) — backward compatible with existing dashboard consumers.The two blockers are local and easy to fix. Once pagination is honest and the dismiss path is consistent, this ships.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: 🚫 Changes requested
TDD evidence is solid across the board — every commit has a red-phase proof in the history (
feat(audit): replace hour-trunc dedupe …adds the red tests first, the SQL rewrite lands in the same commit after; the chronik components each have co-located specs). But two concrete issues need fixing before this ships.Blockers
<button>nested inside<a>inChronikFuerDichBox.sveltelines 82–123. The Dismiss<button>is a descendant of the outer<a href={href(n)}>. HTML spec forbids interactive content descendants of<a>. Browsers tolerate it inconsistently — some propagate the nested click to the anchor despitestopPropagation+preventDefault, some don't focus the button reliably with keyboard nav. The spec we locked in issue comment #3573 was explicit:The current markup ships the anti-pattern Leonie called out. Fix by splitting to the sibling structure with the form-action Dismiss. The spec tests that cover
onMarkReadstill pass after the refactor because they render the component in isolation — update them to render the form-action outcome instead.verbPartsstring split inChronikRow.svelte:87-94is fragile.verbText.indexOf(docTitle)assumes the document title appears verbatim in the compiled Paraglide message exactly once. That breaks when:docTitleis an empty string (indexOf('') === 0→ everything renders as "after", the span is empty).docTitlehappens to match a substring of the verb (e.g., a German translation of "annotated" contains the word that is also the document title). Unlikely but not impossible for short titles like "Brief".{doc}placeholder moves to the start, wherebeforebecomes "".Safer: render the message with
{doc}replaced by a sentinel (e.g.\u0001), then split on the sentinel. Or drop the split-and-style pattern entirely and emit the title as plain bold text before/after the verb — translators can keep the natural sentence, and the UI contract of "underlined accent = document link" is preserved by the fact that the entire row is the link.At minimum, add a red test: "ChronikRow renders an empty-title item without the before/after collapse." It'll fail.
Suggestions
let activeFilter = $state('alle')+$effect(() => activeFilter = data.filter)with eslint-disable. I get the reasoning (optimistic UI beforegoto()resolves), but there's a cleaner path: use$derived(data.filter)for the visual state and keeponFilterChangeas a fire-and-forgetgoto()— Svelte handles optimism becausedata.filterreflects the URL immediately afterreplaceState. One less magic comment, one fewer reactive cycle.Singleton seed-vs-live switch in
+page.svelte:70—notificationStore.notifications.length > 0 ? liveUnread : seedUnread. If the singleton has been initialized but fetched zero notifications,seedUnreadwins, showing stale SSR data. Fix: gate the switch on an explicitinitializedflag from the store rather than notification count, or just always useliveUnreadafter mount and accept one frame of flicker.What's done well
$derived(not$state + $effect) forvariant,actorName,timeLabel,verbTextin ChronikRow.{#each ... (item.kind + item.happenedAt + item.documentId)}keyed — no reconciliation corruption.findRolledUpActivityFeed_returnsAnnotationEntrytest strengthened withcount == 1+happenedAtUntil == nullassertions, not replaced. Preserves historical coverage.Fix the two blockers and this is green from my side.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
No SQL injection, no XSS, no SSRF, no unsanitized logging, no broken auth. The rollup SQL uses
:currentUserIdas a named parameter — injection-proof by design. The new/chronikroute inherits the session-cookie auth gate fromhooks.server.ts— no endpoint exposure regression. But three items need attention.Blockers
Missing ownership test on
?/dismissaction.page.server.ts:47-58takesidfrom form data and callsPATCH /api/notifications/{id}/readwith no check that the id belongs to the current user. The backendNotificationController.markOneRead(UUID, auth)does enforce ownership at the service layer (that's the existing trust boundary — see issue #285 comment #3552). That's fine. What's missing is a regression test that proves this holds:Without this test, a future refactor that blindly forwards the client's id (or worse, pulls ownership info from the form) won't be caught. The current
dismisstest only covers 400-missing-id and 200-happy-path.(Related to Felix's finding — if we wire the Dismiss button through the form action, this test becomes doubly load-bearing.)
Suggestions
Comment preview placeholder in
ChronikRow.svelte:149-161rendersdocTitleinside German quotes as if it were user-generated content. Today that's fine becausedocTitlecomes from the server and Svelte's{text}escapes it. But the TODO says "swap this toitem.commentPreview" — the moment that DTO field lands, it will contain user-typed content. Two precautions for the follow-up:{text}, never{@html}in this path.Worth a
// SECURITY:comment on the TODO so the next person knows the constraint before they add the field.SSE singleton
markReadvia raw PATCH.notifications.svelte.ts:40sendsPATCH /api/notifications/{id}/readfrom the browser. Spring Security currently accepts this because the same-origin session cookie travels with it — that's consistent with the existing NotificationBell pattern. But it means the CSRF protection story is "we trust SameSite=Lax on the session cookie." Worth documenting in a comment or in the upcoming #286 refactor (which is tracked). Not a blocker today.What's done well
V49ships the rollup index through the migration pipeline — no side-channel DDL, testable in CI via Testcontainers.:currentUserIdand:limitpassed as named parameters to the native query. No string concatenation anywhere in the new SQL.role="radiogroup"+aria-checked+ single tab stop onChronikFilterPills— solid semantics, nothing a screen reader can be tricked into misinterpreting.The blocker is a test, not a vulnerability — but the test is load-bearing. Add it and I'm green.
🧪 Sara Holt — Senior QA Engineer
Verdict: 🚫 Changes requested
65 new tests (5 backend rollup + 8 page.server + 40 chronik components + 7 date-buckets + 5 SSE singleton + 6 dashboard activity feed) — that's real coverage, not line padding. The rollup SQL is tested against a real Postgres 16 container, which is the only way to catch
LAG()/ window-function bugs. But the test strategy has three gaps that shouldn't ship.Blockers
loadMorepagination in+page.svelteis untested, and it's broken (see Markus). The UX involves optimistic aria-busy, 3 skeleton rows, focus preservation aftertick(), and thearia-liveannouncement. None of that has a test. A brokenoffsetparam regressing silently is exactly the bug class unit tests prevent. Once the backend honorsoffset(or the feature is removed), add:?/dismissaction has no end-to-end proof of triggering. The action is tested in isolation (success + missing-id), but the UI never actually invokes it — the Dismiss button callsonMarkRead→ singleton → raw PATCH instead. Either:Right now we have a tested action that no user can trigger. That's dead test weight.
Suggestions
No test exercises the rollup query filter clause against non-eligible kinds (
STATUS_CHANGED,METADATA_UPDATED). The existing 5 rollup tests only insert rollable kinds. Add one test that inserts, e.g., aSTATUS_CHANGEDevent and asserts it does not appear in the feed. Cheap insurance for the day someone widens theWHERE kind IN (...)clause.AuditLogQueryRepositoryRolledUpTestdepends onUSER_IDbeing a fresh UUID per run. It is (dddddddd-…-ddddhardcoded) — but@DataJpaTest+@Transactionalauto-rolls back, so cross-test contamination is unlikely. Still, preferringrandomUUID()in test bodies eliminates the class of bugs where a second test class picks the same sentinel and runs in parallel. Low priority.ChronikFuerDichBox.svelte.spec.tsmocksonMarkRead— can't tell from the spec file list whether it asserts "clicking Dismiss does NOT navigate." That assertion is security-adjacent (see Nora) and UX-critical (see Leonie's discussion outcome). Confirm it's there; if not, add:No test covers the filter-empty → "Nichts in dieser Ansicht" transition in
+page.svelte. The componentChronikEmptyStatehas its own variant test, but the page-level orchestration (activity has 3 items, filter=hochgeladen matches 0, empty state renders withvariant='filter-empty') isn't verified end-to-end.What's done well
postgres:16-alpineviaPostgresContainerConfig— no H2, no in-memory approximation. The partial index, theLAG()window, and theBOOL_ORall run against real Postgres.rolledUpFeed_splits_at_2h_boundaryuse an explicitsessionOneLastintermediate variable after I flagged the earlier off-by-one. Readable, correct, and the assertion layout shows intent..svelte.spec.tsfor every new component, factory helpers,getByRole/getByTextfor user-visible assertions rather than internals.findDedupedActivityFeed_returnsAnnotationEntrywas renamed + strengthened withcount/happenedAtUntilassertions, not deleted.Fix the two missing test layers and we're green. And for the record: I still agree VR is a follow-up, not part of this PR.
🏗️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure surface area changes — no new containers, volumes, env vars, ports, or secrets. This is pure app code shipping through the existing pipeline. What's there is done right.
Suggestions (not blockers)
Grafana panel for
/api/dashboard/activityp95 latency. I flagged this in the issue review (comment #3555). The rollup query +V49index combine to make this a new hot path — both/chronikand the dashboard side-rail consume it. A missing index or a query regression will manifest as a slow feed before anything else breaks. Worth an issue:Out of scope for this PR. Flag it so it doesn't fall off the board.
Partial covering index needs verification on the target Postgres.
V49__add_audit_log_rollup_index.sqluses a partial index (WHERE kind IN (...)). That's standard Postgres 12+, perfectly supported by thepostgres:16-alpineimage the devcontainer / Compose / CI / Testcontainers all use — confirmed by the integration tests passing. Keep in mind for any future prod upgrade that Postgres 11 and earlier would not accept this without modification. Noted for Renovate bumps.What's done well
V49goes through Flyway, notCREATE INDEX IF NOT EXISTSat app boot. Single source of truth, replayable on any clean DB. Matches my recommendation in the issue review exactly.:latesttags, no bind mounts for persistent data, no hardcoded secrets. I checked the diff fordocker-compose,.env, and CI workflow changes — zero hits. This is app-code-only.notifications/+page.server.ts+ spec + page.svelte deleted as one commit — no zombie files left for CI to accidentally run..gitea/changes in the diff..svelte-kit/andsrc/lib/paraglide/ownership recovery handled during dev. The test run ended clean afterrm -rf .svelte-kit && svelte-kit sync. That's standard dev hygiene — not something I expect the PR to fix, but worth noting that the frontend container's paraglide write pattern should use named volumes or run as the host UID to avoid root-owned regeneration artifacts long-term. File separately as a devcontainer fix.No infrastructure gates to hold this PR on. Ship it.
🎨 Leonie Voss — UX/Design Lead
Verdict: ⚠️ Approved with concerns
The page ships the spec faithfully — all 5 filter pills, day-bucketed timeline, Für-dich box with inbox-zero, accent-bordered for-you rows, en-dash rollup time ranges,
@/↩markers hidden at 320 px per the A2 decision. The screenshot proof confirms it renders. Two UX concerns plus one copy drift need attention before release.Blockers (UX)
Comment preview renders the document title inside German quotes — it's a visible regression.
ChronikRow.svelte:156-161shows„{docTitle}"forCOMMENT_ADDEDrows. The user sees the document title twice in the same row: once as the underlined link, once as the italic "quoted preview." That's confusing — it looks like the quote is the comment body, but it's the title again.TODO comments in code do not help the user. Two acceptable options:
Hide the preview variant entirely until the backend exposes
item.commentPreview. Collapse the comment row to{actor} kommentierte {doc}with no second line. Ship the preview row as a follow-up once the DTO has the field.Emit an ellipsis placeholder like
„…"(italic, muted) — clearly signals "preview pending" rather than "this is the content."Either is fine. The current state implies the comment is about itself.
Für-dich Dismiss is a
<button>nested inside<a>. Felix and Nora flagged this as invalid HTML / CSRF-architecture drift. From the UX side: the senior audience (60+) often tap-selects with a slight drag — when the OS treats the interaction as anchor click vs. button click inconsistently, they end up navigating into the document instead of dismissing. I've watched that exact failure pattern in user testing before. The split-markup layout (Option C, row<div>with anchor + form as siblings) avoids it.Suggestions
Inbox-zero link copy at 320 px uses the full
„Ältere Erwähnungen ansehen →"phrasing. The spec's A2 decision (comment #3568) said tighten to„Ältere ansehen →"at 320 px only. Either add asm:hidden/hidden sm:inlinevariant, or accept the longer copy if it fits — worth checking in the browser at 320 px.Rollup count badge (
ChronikRow.svelte:140-146): the badge reads just a number ("9") with no unit noun. The verb sentence supplies the plural noun ("9 Blöcke"), but at a scanning glance the bare "9" is ambiguous. Not a blocker — the verb carries the meaning for screen readers. Consider addingaria-label="{count} Einträge zusammengefasst"on the badge so keyboard/screen-reader users don't lose the grouping context.Dark-mode axe run.
/chronikis now inAUTHENTICATED_PAGES→ that fires the three-mode sweep (light, system-dark, manual-dark). Good. But the screenshot in the PR description is light mode only. Can you confirm thebg-accent-bg/10tint on the for-you row hits the 4.5:1 contrast threshold against the dark canvas? The accent token remaps navy → mint in dark mode; the 10% tint on mint on dark canvas is the one combination I'd want to see with eyes before trusting axe.<style>block inChronikFuerDichBox.svelte(lines 130-151) defines.fade-inwithprefers-reduced-motionfallback — clean. One detail: the animation runs on every render, so a re-render that preserves the sameunreadlist (e.g. a filter pill click elsewhere on the page) could flash the rows. Check with DevTools if it's noticeable. If it is, gate on an$effectthat triggers only whenunreadlength increases.What's done well
role="radiogroup", arrow-key navigation wrapping, 44 px min-height — dual-audience constraint met for both seniors and keyboard-first users.text-ink-3,tracking-widest) — will remap cleanly to dark mode.@marker (≥640 px only) — three redundant non-color cues. Color-blind users still read the structure.focus-visible:ring-2 focus-visible:ring-focus-ring focus-visible:outline-noneon every interactive element I checked. Keyboard users see where they are.font-serifon the italic comment preview,font-sanson everything else — brand typography discipline preserved.Fix the comment-preview duplication and the Dismiss nesting and I'm green.
🔄 Review cycle 1 — summary
Every blocker addressed. 4 new commits pushed to
feat/issue-285-chronik-unified-activity.Blockers → commits
<button>nested inside<a>in Für-dich row7035c5d7<li>flex,<a>as child,<button>as sibling. New spec assertsdismiss.closest('a')is null.?offset=Npagination48e5adf9?/dismissand?/mark-allform actions48e5adf9verbText.indexOf(docTitle)split1add91b0{doc}placeholder). Red tests lock the invariant for empty titles and short substring-matching titles.1add91b0„…". Red test asserts preview no longer contains the title verbatim. SECURITY comment added for the forthcomingcommentPreviewDTO field.d5d9021dSTATUS_CHANGED+METADATA_UPDATEDalongsideTEXT_SAVED, asserts only the eligible row is returned.Deferred as follow-up issues
/api/dashboard/activity+ wire "Mehr laden" (Markus, Sara)./api/dashboard/activity(Tobias).kindsCSV query param for server-side filter-pill filtering (Markus).Concerns not addressed (suggestions only, not blockers)
activeFilter = $state(...) + $effect(...)pattern — left as-is with the eslint-disable comment. Converting to pure$derived(data.filter)loses the brief optimistic flicker duringgoto(), not worth the trade on a filter control.+page.svelteuses.length > 0sentinel — acceptable for initial render, flagged for a futureinitializedflag on the store.?/dismissaction — the action no longer exists, so the test is moot. Backend ownership check atNotificationController.markOneReadstill enforces and is covered by existing controller-layer tests.aria-label, 320 px copy variant, dark-mode contrast spot-check — all tracked for the follow-up VR/a11y polish pass.Verification
./mvnw test→ 1188 passed, 0 failures (+1 new non-eligible-kinds test).npm test→ 1080 passed, 0 failures (123/123 test files after fresh.svelte-kitsync).Ready for cycle 2 review.
🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Both blockers I raised in cycle 1 are resolved honestly, not papered over.
Checked
?offset=Npath was removed rather than shipped with a silent bug. Backend pagination support tracked as #290 with a clear scope (cursor vs offset, integration test, loading micro-state). Deferring is the right call — adding cursor-based pagination cleanly needs a design decision and its own test surface.?/dismissand?/mark-allgone, tests with them. The decision to stay on the callback pattern (matching NotificationBell) is consistent — form-action migration across both surfaces belongs in #286 as planned.Architectural posture holds
+page.server.ts— still the seam.Ship it.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Both blockers resolved with the textbook red/green/commit pattern — the tests exist before the fix and they prove the new behavior.
Checked
ChronikFuerDichBox.svelte): Outer<li>flex container,<a>+<button>as siblings. New specexpect(dismiss.closest('a')).toBeNull()locks the invariant. No more invalid HTML.indexOfsplit (ChronikRow.svelte): Sentinel-based split with U+0001 is the right choice — robust against empty titles, substring-matching titles, and translator sentence reordering. Two new red tests exercise the edge cases:still renders the row link when documentTitle is an empty stringandrenders a short document title that could substring-match the verb(+ uniquedata-testid="chronik-doc-title"so the assertion counts exactly one span, not zero or two).Suggestions left as-is (deferred, not blocked)
activeFilter = $state + $effectwith eslint-disable — your justification on the briefgoto()flicker is fair. Keep it..length > 0sentinel — pragmatic; can tighten to aninitializedflag later if it matters.Bonus
The
SECURITY:comment next to the comment-preview TODO is exactly the kind of future-reader note that pays for itself the first time someone reaches for{@html}. That's two paragraphs of ADR material compressed into a five-line comment.Green from my side.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
The blocker I raised — ownership-enforcement test on
?/dismiss— is resolved because the action was deleted, removing the attack surface it would have protected. Backend ownership enforcement atNotificationController.markOneRead(UUID, auth)remains the single source of truth for the "dismiss" path, and that code/test is unchanged.Checked
PATCH /api/notifications/{id}/readandPOST /api/notifications/read-all(both CSRF-safe via SameSite session cookie, consistent with the existing NotificationBell pattern). Trust boundary unchanged.ChronikRow.svelte:154-168). The„…"is a static string — no user input, no XSS vector. TheSECURITY:comment on the TODO is explicit about the constraints for the follow-up ({text}only, backend must sanitize + truncate). That's the kind of hand-off note that prevents the next developer from reaching for{@html}.ChronikRow.svelte). U+0001 is a control character; Paraglide compiles it into the message, we split on it, and the original document title renders via Svelte's escape-by-default{text}interpolation. No injection surface.page.server.tsno longer has user-controlled input paths. One less place for a regression to creep in.Suggestions (non-blocking)
?/dismissaction for the #286 form-action migration, carry the ownership-403 regression test I drafted in cycle 1 across. Paste-able scaffold still in my review.npm audit+ an OWASP Dependency-Check pass before the next deploy — unrelated to this PR, just routine hygiene sinceopenapi-typescriptand Svelte changed this sprint.Clean.
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
Both blockers resolved cleanly.
Checked
?/dismiss+?/mark-alltests deleted alongside their actions. No orphan tests pretending to cover code the UI never triggers. That's healthier than what I asked for.AuditLogQueryRepositoryRolledUpTest.rolledUpFeed_excludes_non_eligible_kinds) insertsSTATUS_CHANGED+METADATA_UPDATEDalongsideTEXT_SAVEDand asserts only the eligible row returns. Exactly the invariant I flagged.ChronikFuerDichBoxspec line 117-127) assertsdismiss.closest('a')is null. That's a good example of a locked-in structural test — invisible in the UI, loud in CI if someone refactors the markup back to the nested form.Suggestions (non-blocking)
ChronikEmptyState+ChronikTimelinespecs cover the subcomponents. File it if you ever add routing-level tests for/chronik.@DataJpaTest + @Transactionalrollback covers cross-test contamination in the new rollup tests. Hardcoded sentinel UUIDs are fine given that.VR stays deferred — on the same page as cycle 1.
Ship it.
🏗️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No change from cycle 1 — still pure app code, still no infrastructure surface touched. Cycle 1's suggestion (Grafana p95 panel) is now tracked as #291, which is the right place for it.
Checked
V49migration unchanged — applied cleanly in Testcontainers CI during the 1188-test backend run.Clean ship.
🎨 Leonie Voss — UX/Design Lead
Verdict: ✅ Approved
Both UX blockers resolved with the correct interpretation of the spec.
Checked
ChronikFuerDichBox.svelte). Outer<li>hosts<a>+<button>as independent interactive elements. The senior-audience tap-drag failure mode I flagged is gone — no browser routing ambiguity between anchor and button. Test lock prevents regression.ChronikRow.svelte:153-168). The placeholder is now„…"— distinct from the document title, signals "preview pending" without implying the comment is quoting itself. TheSECURITY:addendum on the TODO is a nice touch: it tells the next person exactly what to do when the backend DTO gainscommentPreview.<span>regardless of where the translator places it in the sentence. That's a real i18n improvement, not just a bug fix.Deferred (confirmed with you in the cycle 1 summary)
„Ältere ansehen →").aria-label.bg-accent-bg/10on dark canvas.All three are polish items, not release-blockers, and consistent with the VR follow-up posture we agreed on.
New note (non-blocking)
The
„…"ellipsis at current font scale (~13 px) is visually light — on bright mobile screens in sunlight (the 67-year-old on a park bench test), it may render almost invisible. If you watch it in the wild for a week and it feels too subtle, nudge totext-ink-3or bump totext-ink-2/80. Not a change I'd ask for today.Ship it.