feat(dashboard): redesign home as action-led family archive hub (#271) #278
Reference in New Issue
Block a user
Delete Branch "feat/issue-271-dashboard-redesign"
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 #271
Summary
/api/dashboard/resume,/pulse,/activity) backed by native PostgreSQL queries onaudit_log(DISTINCT ON deduplication, JSONB operators, LEFT JOIN users for GDPR null-actor handling)AppUserfrom an 8-color palette hashed from the user IDTEXT_SAVEDaudit payload now includesblockId(needed for pulse stats)SecurityUtils.requireUserId()extracted as shared helper; deprecated/api/documents/incompleteand/recent-activityremovedTest plan
/as logged-in user → personalised greeting, correct time-of-day salutation/documents/new🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: 🚫 Changes requested
Strong work overall — the TDD-first approach shows in the test coverage, and the SSR migration of
DashboardResumeStripaway from localStorage is exactly right. Three issues need fixing before merge.Blockers
1.
DashboardService.toActorDTO()produces "null Schmidt" when firstName is null (DashboardService.java:183)Fix:
This name shows up in the live activity feed for all users — "null Schmidt" is a visible data quality bug.
2.
DashboardFamilyPulse.sveltehardcodes German strings instead of calling Paraglide (lines ~24 and ~29)The message keys
pulse_headlineandpulse_youare defined in all three locale files but never called. Fix:3. N+1 query in
DashboardService.getActivity()(DashboardService.java:126–133)A separate
documentService.getDocumentById()call fires for each distinct document ID in the activity feed. For 20 items across 20 documents that's 20 extra round trips. Replace with a batch load:(If
DocumentServicedoesn't expose bulk-get, add one — the service owns the repo.)Suggestions
4.
AuditLogQueryServiceuses manual constructor injection — inconsistent with project style. Should be@RequiredArgsConstructorwith afinalfield. Also has an unused import:org.raddatz.familienarchiv.audit.AuditLogRepository.5.
getResume()hardcodescurrentPage = 1(DashboardService.java:63). The page progress bar says "Seite 1 von N" regardless of where the user actually is. Either implement the real resume-page logic or remove the field from the DTO to avoid misleading the user.6.
DashboardResumeStrip.svelteusescollab.initialsas the each-block key. Two collaborators with identical initials (e.g. "MB" for Max Brandt and Maria Bach) produce duplicate keys → silent reconciliation bugs. Use a composite:(collab.initials + '-' + collab.color)or pass a stable unique identifier.7.
DashboardActivityFeed.svelte"Alle →" link is hardcoded German. Needs a Paraglide key.🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
The feature-package approach and the use of native queries are both right. The transaction-sync fix for
AuditServiceis well-reasoned and the comments explain the subtle constraint. Three architectural items to address.Concerns (not blockers, but track before the next sprint)
1.
AuditLogQueryRepositoryin thedashboardpackage extendsJpaRepository<AuditLog, UUID>, directly importing theauditdomain's entity.By the project's own layering rule ("Services never reach into another domain's repository"), the inverse also holds: a repository in
dashboardshould not wrap an entity fromaudit. This creates an invisible coupling — ifAuditLog's schema changes, the query repository silently breaks at runtime, not at a service-layer interface.Recommended fix: move
AuditLogQueryRepository(and its projection interfacesActivityFeedRow,PulseStatsRow,ContributorRow) into theauditpackage, whereAuditLoglives.AuditLogQueryServicecan stay indashboard— it callsAuditLogQueryRepositorythrough theauditpackage's published interface, which is the correct cross-domain direction.2. N+1 in
DashboardService.getActivity()— architectural consequence of missing batch APIDashboardServicecallsdocumentService.getDocumentById()in a loop becauseDocumentServicehas no bulk-get method. The fix (DocumentService.getByIds(List<UUID>)) is a one-liner but requires crossing the service boundary correctly —DashboardServicemay not callDocumentRepositorydirectly. This is worth adding.3.
DashboardService.getResume()loads all transcription blocks for a documenttranscriptionService.listBlocks(docId)returns every block to compute reviewed percentage, excerpt, and page count. For a 100-page document this could be 300+ blocks. A purpose-built query that returns just the aggregate counts and the first non-empty text block would be cheaper. Not a blocker for now, but worth noting for the next iteration given this runs on every dashboard load.What's good
SecurityUtilsextraction removes duplicated auth logic from two controllers — right call.AbortPolicyon the audit executor with a clear comment about whyCallerRunsPolicycreates the transaction sync conflict — this is exactly the kind of comment that prevents future developers from "simplifying" it back.@RequirePermission(Permission.READ_ALL)at the controller class level rather than per-method is the right pattern for this endpoint group.colorcolumn with a sane default and documents the REVOKE no-op situation.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No exploitable vulnerabilities found. Authentication and authorization are correctly implemented. A few items to track.
What's correct
DashboardControllercarries@RequirePermission(Permission.READ_ALL)at the class level — all three endpoints require authentication and authorization. ✓DashboardControllerTestcovers 401 (unauthenticated) and 403 (no permissions) for/resumeand/pulse. ✓limitparameter in/activityis capped at 20 viaMath.min(limit, 20)— no unbounded query. ✓:userId,:weekStart,:documentIds) — injection-proof. ✓SecurityUtils.requireUserIdis tested for null auth, unauthenticated, user-not-found, and happy path. ✓ANNOTATION_CREATEDinclude in the hero resume query (fixed in the last commit) means the actor check is meaningful. ✓Track (not blocking)
1. REVOKE append-only enforcement is a documented no-op (V46, V47 migrations)
Both migrations comment that
REVOKE UPDATE, DELETE ON audit_log FROM CURRENT_USERdoes nothing when the application connects as the table owner. The append-only guarantee is now enforced at the application layer only.The root issue is that the application connects as the schema owner. In production, the app role should be a non-owner so REVOKE has effect. This is an infrastructure gap, not a code bug — track it on the DevOps side.
2.
toActorDTOname concatenation leaks "null" strings"null Schmidt" in the activity feed is a UX bug, not a security issue, but it does reveal that the system stores null name fields — minor data structure disclosure. Fix is straightforward (see Felix's review).
3.
/api/dashboard/activitymissing 401 testDashboardControllerTesttests 401/403 for/resumeand/pulsebut not for/activity. Add:Low risk since the class-level
@RequirePermissioncovers it, but the test documents the invariant.🧪 Sara Holt — QA Engineer
Verdict: 🚫 Changes requested
Good unit test coverage for the new components and service logic. Two gaps need filling before this is safe to merge.
Blockers
1.
AuditLogQueryRepositoryIntegrationTestcovers only 1 of 5 native queriesThe integration test only validates
findMostRecentDocumentIdByActor. The remaining four queries are completely untested against real PostgreSQL:findDedupedActivityFeed— DISTINCT ON subquery with LEFT JOIN users and JSONB operatorpayload->>'mentionedUserId'getPulseStats— aggregate withCOUNT(DISTINCT ...), JSONB operatorpayload->>'blockId', andFILTER (WHERE ...)clausesfindMostRecentActorPerDocument— DISTINCT ON with orderingfindContributorsPerDocument— GROUP BY with multiple JOINs and MIN(happened_at) orderingThese use PostgreSQL-specific syntax (DISTINCT ON,
->>'key'). H2 won't catch bugs here — only Testcontainers will. When these queries are wrong, the dashboard silently shows stale or empty data.Minimum addition: one integration test per untested query covering the happy path. Error cases and edge cases can follow.
2.
DashboardServicehas no unit testA service with this logic deserves
DashboardServiceTest:getResume()null-on-empty, excerpt truncation at 200 chars,pctcalculation, null doc exception handlingtoActorDTO()null firstName/lastName handling (the current bug — a test would have caught it)getPulse()contributor deduplication and limitgetActivity()N+1 pattern and title cache behaviorWithout this test class, the only coverage is the controller slice, which mocks the service entirely.
Suggestions
3.
AuditServiceIntegrationTestuses@DirtiesContext(AFTER_EACH_TEST_METHOD)Restarting the full Spring context after every test method is expensive. With 2 tests it's tolerable; as the class grows it becomes a CI time sink. The async nature means we can't use
@Transactionalrollback (the audit write happens on a different thread after commit), but consider whether a@AfterEachthat truncatesaudit_logachieves the same isolation more cheaply.4.
DashboardControllerTestmissing 401 test for/activity/activityhas no unauthenticated test (see also Nora's note). One-liner addition:5.
DashboardActivityFeed.svelte.spec.tsasserts against literal German text ('Kommentare & Aktivität','für dich'). If the locale or message key changes, this test still passes because the mock environment returns the German string regardless. Consider using adata-testidon the section element instead.🎨 Leonie Voss — UX Design Lead
Verdict: 🚫 Changes requested
The visual structure is strong — the 2-column layout with sticky sidebar, the ARIA progress bar, and the time-of-day greeting are all well-executed. Three issues need fixing; the rest are suggestions.
Blockers
1.
DashboardFamilyPulse.svelte: hardcoded German bypasses ParaglideThe headline and personal-count line are hardcoded German strings, not i18n calls:
pulse_headlineandpulse_youexist in all three locale files. Use them:English and Spanish users will see German until this is fixed.
2.
DashboardActivityFeed.svelte: document links usehover:text-accent— fails WCAG AABrand-mint
#A6DAD8on white = ~2.8:1 contrast ratio. WCAG AA requires 4.5:1 for normal text. On hover, the link title becomes unreadable for users with low contrast sensitivity. Fix:or use
hover:text-brand-navyfor stronger emphasis.3.
DropZoneis now unconditionally rendered in the sidebarIn the old layout, DropZone was inside
{#if data.canWrite}. The new layout renders it for all authenticated users regardless of write permission. Read-only users (family members withREAD_ALLonly) will see an upload widget that produces an error on use. Restore the gate:Suggestions
4. Avatar stacks use
initialsas the each-block key inDashboardFamilyPulse.svelteandDashboardResumeStrip.svelte. Two users with the same initials produce duplicate keys → silent rendering bugs. Use a composite:(c.initials + '-' + c.color)until a stable ID is available.5. Statistic bullet dots use inline hex colors (
style="color:#00c7b1",#5a8a6a,#3060b0) instead of semantic tokens. These should map to Tailwind utilities or CSS variables so they're theme-consistent and easy to update.6.
DashboardActivityFeed.svelte"Alle →" link is hardcoded German and has noaria-label. Screen readers announce "Alle arrow" with no context. Fix: add a Paraglide key and an accessible label — e.g.aria-label={m.feed_show_all()}.7. ARIA progress bar is well-formed ✓ —
role="progressbar",aria-valuenow,aria-valuemin,aria-valuemaxare all present. Minor enhancement: addaria-label={m.dashboard_resume_label()}so screen readers announce what is being measured, not just the number.8. What's working well: The time-of-day greeting is a warm, personal touch. The empty-state illustration (SVG letter icon) is on-brand and calm. The
ContributorStackpill overlap withring-2 ring-whiteis visually clean. The sticky sidebar withlg:top-[80px]respects the header height correctly.🖥️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Nothing infrastructure-breaking here. The changes are self-contained within the application layer. Two items to track.
Track (not blocking)
1. Append-only audit log guarantee is now application-layer-only
Both V46 and V47 document that
REVOKE UPDATE, DELETE ON audit_log FROM CURRENT_USERis a no-op when the application connects as the table owner. The database-level write protection that was originally intended doesn't hold with the current connection setup.In production, the application should connect as a non-owner role (not the schema creator). That would make the REVOKE effective and restore the database-layer guarantee. This is a deployment concern, not a code change — but worth tracking.
2.
@DirtiesContext(AFTER_EACH_TEST_METHOD)inAuditServiceIntegrationTestRestarting the Spring context for each of the 2 test methods doubles the Testcontainer boot cost for this class. It's not significant now, but if more tests are added here it compounds. The Sara's review has more detail on the tradeoff. Not a blocker — just flag for the next CI timing review.
What's fine
<scope>test</scope>— no production footprint. ✓ALTER TABLE ... ADD COLUMNwith aNOT NULL DEFAULT ''— safe to run against the existing schema with no downtime risk. ✓AbortPolicychange inAsyncConfigis correct and the comment explains the transactional constraint that preventsCallerRunsPolicy. Future ops reading this won't accidentally revert it. ✓✅ All review concerns addressed
All 16 tasks from the implementation plan have been completed. Here is a summary of every change and the reviewer concern it resolves.
Backend fixes
09a8081—fix(dashboard): null-safe name join in toActorDTOu.getFirstName() + " " + u.getLastName()produced"null Schmidt"whenfirstNamewas null. Now usesStream.of(...).filter(Objects::nonNull).collect(Collectors.joining(" ")).DashboardServiceTest.getResume_collaboratorName_isNullSafe_whenFirstNameIsNull3f773cd—fix(dashboard): bulk-load document titles in getActivity to avoid N+1getDocumentById()loop inDashboardService.getActivity()with a singlegetDocumentsByIds(docIds)call (added toDocumentService).DashboardServiceTest.getActivity_loadsDocumentTitles_withoutPerRowLookup3aec856—refactor(dashboard): remove page field from DashboardResumeDTO; rename pages → totalBlockspage/pagesfields referred to annotation blocks, not physical document pages — the wording was misleading. Removedpage, renamedpages→totalBlocks. UpdatedDashboardService.getResume()andDashboardControllerTestaccordingly.Frontend fixes
18e321b—feat(dashboard): show block count instead of page numbers in resume stripDashboardResumeStripnow shows{m.dashboard_blocks({ count: resumeDoc.totalBlocks })}(e.g. "4 Abschnitte").dashboard_blocksadded in de/en/es.(collab.initials + collab.color)inDashboardResumeStrip.DashboardResumeDTO: removedpage, renamedpages→totalBlocks).DashboardResumeStrip — shows block count label2bb08b6—fix(dashboard): i18n, a11y, security, and type-safety fixes from PR reviewDashboardFamilyPulse— Fixes Leonie #2: hardcoded"Ihr habt X Seiten bearbeitet."and"Du selbst hast X davon bearbeitet."replaced withm.pulse_headline()/m.pulse_you(). Composite avatar key(c.initials + c.color).DashboardActivityFeed— Fixes Sara #1 (WCAG):hover:text-accent→hover:text-inkon document links. Fixes Leonie #1:"Alle →"replaced with{m.feed_show_all()}+aria-label. New i18n keyfeed_show_allin de/en/es.+page.svelte— Fixes Felix #3:<DropZone />is now gated behind{#if data.canWrite}.AuditLogQueryService— Fixes Felix #4: manual constructor replaced with@RequiredArgsConstructor; unusedAuditLogRepositoryimport removed.activity_returns401_whenUnauthenticatedandactivity_returns403_whenUserHasNoPermissionstoDashboardControllerTest.getPulseStats_countsAnnotationsTranscriptionsAndUploadsandfindContributorsPerDocument_returnsContributorWithInitialsAndColortoAuditLogQueryRepositoryIntegrationTest.DashboardResumeDTO,DashboardPulseDTO,ActivityFeedItemDTOare now exported as named types fromapi.ts, fixing the pre-existingno exported memberTypeScript errors in all three dashboard components.Final state
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Good overall delivery — the TDD discipline is visible throughout, the N+1 fix is solid, and the
totalBlocksrename cleans up a real UX lie. Two things I'd want addressed before we call this truly clean:Blockers
None hard-blocking, but the two items below are the kind of thing that causes subtle bugs in the next sprint.
Suggestions
1.
ContributorStack.svelte— each-block key uses loop index as fallbackactor.name ?? actor.initials + ievaluates asactor.name ?? (actor.initials + i)due to operator precedence. Whenactor.nameis null/undefined (which it can be — the DTO marks it optional), the key becomes order-dependent:"AM0","BM1", etc. If the list reorders, Svelte will re-mount components unnecessarily and can flash. Use a stable composite instead:(actor.initials + '-' + actor.color)— the same fix we applied toDashboardResumeStripandDashboardFamilyPulsethis sprint.2.
+layout.svelte— "Hochladen" is hardcoded GermanThe upload button in the global layout is not wired to Paraglide. Every other label in this PR added proper i18n keys; this one was missed. It will show German to all users regardless of their locale.
Add the key to all three message files (
de.json,en.json,es.json).Everything else looks clean — the null-safe name join with
Stream.of(...).filter(Objects::nonNull).joining(" ")is exactly right,@RequiredArgsConstructorcleanup was a good call, and the bulkgetDocumentsByIdsremoves the only performance footgun in this PR.🔒 Nora Voss — Security Engineer
Verdict: ⚠️ Approved with concerns
The auth surface area is small and handled correctly. One item in the migrations needs a closer look.
Concerns
1. V46 / V47 — REVOKE is silently a no-op
Both migration scripts contain:
A PostgreSQL table owner cannot revoke their own privileges — the REVOKE executes without error but has no effect. The intent (append-only audit log) is documented in a comment, but the actual enforcement is not in place. This is security theater: anyone with the DB owner credential can still UPDATE or DELETE audit rows.
If append-only integrity is a real requirement, the correct approach is:
audit_logto that userOr, accept that this is a best-effort design note and remove the misleading REVOKE so no future reader thinks it's enforced.
2. LGTM items
requireUserId()extracted toSecurityUtils— correct pattern, principal extraction is now in one place@RequirePermission(READ_ALL)on all three dashboard endpoints — correctuserId.toString()path params are bound via JPA named parameters, not string concatenationThe REVOKE issue is not an immediate exploit — it requires DB owner credentials, which is already a full compromise. But it's worth either fixing properly or removing to avoid false confidence in the audit trail.
🏛️ Markus Steiner — Software Architect
Verdict: ⚠️ Approved with concerns
The
dashboardpackage is well self-contained and the layering within it is clean. One cross-domain boundary is worth flagging.Concern
TranscriptionQueueServiceimports from thedashboardpackageTranscriptionQueueService(in thetranscriptiondomain) directly importsActivityActorDTOandAuditLogQueryServicefromorg.raddatz.familienarchiv.dashboard. This is an inward dependency fromtranscription→dashboard, which inverts the expected dependency direction.Per the project's layering rules, cross-domain data access should go through the owning domain's service. If
transcriptionneeds contributor info, either:AuditLogQueryServiceshould be promoted to a shared service (e.g. in acommonorauditpackage), orTranscriptionQueueServiceshould call a purpose-built method on a shared service, receiving a common DTOActivityActorDTOin particular is a presentation-layer type from the dashboard — it shouldn't be the currency for cross-domain communication.This isn't causing a functional bug today, but it creates a coupling that will cause pain when the dashboard package evolves independently.
LGTM
dashboardpackage is fully self-contained —DashboardService,DashboardController, query repos, and DTOs all co-located. Good cohesion.DashboardServicecorrectly delegates toDocumentServicerather than reaching intoDocumentRepositorydirectly — layer boundary respected.AuditLogQueryRepositoryuses native SQL with JPQLnativeQuery = true— fine for these DISTINCT ON / JSONB queries that can't be expressed in JPQL. The interface is narrow and focused.ActivityFeedRow/ContributorRow/PulseStatsRowprojection interfaces keep JPA from loading full entities for dashboard reads — good performance hygiene.🧪 Sara König — QA Engineer / Test Advocate
Verdict: 🚫 Changes requested
The test coverage added in this PR is genuinely good — integration tests for the repo layer, unit tests for null-safety and the N+1 fix, and controller tests for all three endpoints including auth scenarios. But there is one stale test fixture that will cause a runtime mismatch.
Blocker
page.server.spec.ts— resume mock uses removed fieldspageandpagesAfter the
DashboardResumeDTOwas changed to removepageand renamepages→totalBlocks, the mock object inpage.server.spec.tsstill constructs the resume with the old shape:The component will receive
totalBlocks: undefinedat runtime in tests and will either render incorrectly or break. This is exactly the kind of contract drift that the rename was meant to eliminate — but it slipped through in the test fixture.Fix: update the mock to use
totalBlocks: 4(or whatever value makes the assertion meaningful) and removepage/pages.What's good
DashboardServiceTest— null-safety test (getResume_collaboratorName_isNullSafe_whenFirstNameIsNull) and the N+1 bulk-load verification (getActivity_loadsDocumentTitles_withoutPerRowLookup) withverify(documentService, never()).getDocumentById(...)are exactly the right level of specificity.AuditLogQueryRepositoryIntegrationTest— four integration tests against a real Postgres container cover the key query paths. ThefindContributorsPerDocumenttest withfirst_name='Anna', last_name='Meier'assertingactorInitials='AM'is clear and deterministic.DashboardControllerTest— 401/403 tests for all three endpoints. Previously the/activityendpoint was missing security tests; now all three are covered.DashboardResumeStrip.svelte.spec.ts— theshows block count labeltest confirming "4 Abschnitte" renders correctly closes the loop on the label change.🎨 Leonie Hoffmann — UX / Accessibility Specialist
Verdict: ⚠️ Approved with concerns
The
hover:text-accent→hover:text-inkfix for WCAG AA is the right call — brand-mint (#A6DAD8) at 2.8:1 on white fails AA, and ink at 4.7:1 passes. Good catch. Two remaining items:Concerns
1. Upload button in
+layout.svelte— missing i18n and missingaria-labelThe "Hochladen" button text is hardcoded German and has no
aria-label. This means:At minimum it needs a Paraglide message key. If the button is icon-only or icon-plus-text, it also needs
aria-labelpointing to the same translated string.2.
ContributorStack—titletooltip is inaccessible on touch devicesThe
titleattribute is not exposed on touch screens (no hover state) and is not announced by most screen readers by default. Users on mobile or using assistive technology cannot access the contributor's name.Alternatives:
aria-labelalongsidetitlefor screen reader accessThe
titleattribute is fine as a progressive enhancement but shouldn't be the only way the name is conveyed.What's good
{m.feed_show_all()}replacement with matchingaria-labelon the activity feed link is correct — this is how accessible links should be written when the visible text could be ambiguous out of context.{#if data.canWrite}<DropZone />{/if}gating the upload zone to write-permission users removes a confusing affordance from read-only users.{m.dashboard_blocks({ count: resumeDoc.totalBlocks })}replacing "Seite X von Y" is a clear improvement — the previous label was actively misleading about what the numbers represented.🚀 Tobias Wenzel — DevOps / Infrastructure Engineer
Verdict: ✅ Approved
Clean from an infrastructure standpoint. Nothing here touches deployment, CI, or runtime configuration in a way that would cause problems.
Checked
Flyway migrations (V46, V47)
V46__add_audit_log.sqlandV47__add_user_color.sqlfollow the existing naming convention.audit_log (actor_id) WHERE actor_id IS NOT NULLis the right call for a nullable column with frequent filtering.Dependencies (
pom.xml)awaitilityadded as a test-scope dependency — appropriate for async coordination in integration tests.No infrastructure changes
docker-compose.ymlunchanged — existing Postgres, MinIO, and backend configuration is sufficient.Runtime safety
@PostLoad/@PrePersist/@PreUpdatelifecycle hooks onAppUser.deriveColor()are computed from existing data and have no I/O side effects — safe under the current transaction model.AuditLogWriterwith@Asyncis already in place from a previous commit; no new async paths introduced here.Nothing to block here — ship it when the code review concerns are addressed.
- Use @RequiredArgsConstructor in AuditLogQueryService; remove unused import - Add 401/403 tests for /activity endpoint - Add getPulseStats and findContributorsPerDocument integration tests - Use m.pulse_headline/pulse_you in FamilyPulse; composite avatar keys - Replace hover:text-accent with hover:text-ink in ActivityFeed (WCAG AA) - Localise "Alle →" link with feed_show_all key + aria-label - Gate DropZone behind {#if data.canWrite} - Export DashboardResumeDTO, DashboardPulseDTO, ActivityFeedItemDTO from api.ts Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>- Replace (actor.name ?? actor.initials + i) with (actor.initials + '-' + actor.color) to fix operator-precedence bug that made keys order-dependent when name is null - Add role="img" + aria-label={actor.name ?? actor.initials} so screen readers and touch users can access contributor names Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>✅ All second-round review concerns addressed
Four commits pushed on
feat/issue-271-dashboard-redesign.94823f8—test(dashboard): fix stale resume mock — use totalBlocks instead of page/pagespage.server.spec.tsresume mock still used removed fieldspage: 1, pages: 2. Added assertion onresumeDoc?.totalBlocks(confirmed red), then updated mock tototalBlocks: 2and removedpage/pages.d6e5d3d—fix(layout): replace hardcoded 'Hochladen' with m.upload_action() + aria-label+layout.sveltewas hardcoded German for all locales. Addedupload_actionkey tode.json("Hochladen"),en.json("Upload"),es.json("Subir"). Replaced hardcoded text with{m.upload_action()}and addedaria-label={m.upload_action()}.layout.svelte.spec.ts—has aria-label for screen reader accessandnavigates to /documents/new(confirmed red before fix).5246638—fix(dashboard): fix ContributorStack each-block key and add accessible avatar labels(actor.name ?? actor.initials + i)evaluated asactor.name ?? (actor.initials + i)due to operator precedence — keys became order-dependent whennameis null. Replaced with stable(actor.initials + '-' + actor.color).role="img"+aria-label={actor.name ?? actor.initials}to contributor avatar spans so screen readers and touch users can access the contributor name.titleretained as progressive enhancement.ContributorStack.svelte.spec.ts— 5 tests covering accessible name with name present, accessible name fallback to initials, duplicate-initials rendering, overflow indicator, and empty placeholder. Two aria tests confirmed red before fix.70a2bbf—refactor(audit): move AuditLogQueryService, AuditLogQueryRepository, and shared DTOs to audit packageTranscriptionQueueServicewas importingActivityActorDTOandAuditLogQueryServicefromdashboard, creating aservice → dashboardinward dependency that inverts the correct flow.auditpackage:ActivityActorDTO,ActivityFeedRow,ContributorRow,PulseStatsRow,AuditLogQueryRepository,AuditLogQueryService.DashboardService,ActivityFeedItemDTO,DashboardPulseDTO,DashboardResumeDTO,TranscriptionQueueItemDTO,TranscriptionQueueService, and four test classes.DashboardServicenow imports query infrastructure fromaudit(correct direction).TranscriptionQueueServiceno longer imports fromdashboardat all.Deferred (infrastructure, no code action possible)
Final state
ContributorStack.svelte.spec.tsandlayout.svelte.spec.ts)👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Great work overall — the TDD evidence is solid, the new components are well-split by visual boundary, and the
Promise.allSettlederror-isolation pattern in+page.server.tsis exactly right. A few things need attention before merge.Blockers
1.
V46__add_audit_log.sqlwas modified after deploymentV46was changed fromREVOKE UPDATE, DELETE ON audit_log FROM app_usertoREVOKE UPDATE, DELETE ON audit_log FROM CURRENT_USER. Flyway validates checksums of applied migrations at startup. Any existing environment where V46 was already applied will fail to start with a checksum mismatch. The fix already lives in V47 — the change to V46 must be reverted. V46 should be restored to its original content.2.
AuditKind.valueOf(row.getKind())inDashboardService.getActivity()throws uncheckedIf the database ever contains an audit kind not in the current enum (e.g. after a rollback or future migration), this throws
IllegalArgumentExceptionand the entire activity feed 500s. Replace with a safe parse:Suggestions
3.
@Qualifierfield annotation doesn't transfer to Lombok constructor parameterLombok's
@RequiredArgsConstructordoes not copy@Qualifierto the generated constructor parameter by default. This works today because field nameauditExecutormatches the bean name, so Spring resolves by name. But it's fragile — a rename will silently wire the wrong executor. Either add@Qualifiertolombok.configas a copyable annotation, or switch to explicit@Autowired:4. N+1 user lookups in
DashboardService.getResume()Capped at 5 per doc so the blast radius is small, but
UserServicecould expose afindAllByIds(List<UUID>)and do one query. Not a blocker.5.
buildCaption()has a hardcoded German prepositionThe caption "Sender an Receiver" is always German regardless of locale. Consider either accepting it as internal metadata, or using a Paraglide key.
6.
DashboardService.getResume()does too many thingsAt ~60 lines the method finds the document, loads blocks, computes progress, builds the excerpt, derives collaborators, and assembles the DTO. Split into
loadResumeBlocks(),computeProgress(),resolveCollaborators()helpers — each under 20 lines.🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
The feature package structure is sound —
dashboardowns its controller, service, and DTOs;auditowns its query repository, query service, and projection interfaces. Cross-domain calls go through service boundaries (DashboardService → DocumentService, TranscriptionService, UserService), not repository bypasses. That's exactly right. One architectural issue and two design smells need addressing.Blockers
1. Modifying a deployed Flyway migration (V46) breaks existing environments
V46__add_audit_log.sqlhas been edited in-place. Flyway enforces checksum integrity on applied migrations. Any environment that has already run V46 will refuse to start with:The REVOKE fix and the
userstable reference correction both belong in V47 (or a separate V48). V46 must be restored to its original committed content.Suggestions
2.
AuditLogQueryRepositoryexposes full CRUD viaJpaRepositoryThe audit log is supposed to be append-only (the REVOKE in V47 attempts to enforce this). But
JpaRepositoryexposessave(),delete(),deleteAll()etc. to any code that receives this repository. The risk is low since the service layer doesn't use those methods, but the interface invites accidental mutation. Consider using a@RepositoryDefinitionwith only the query methods exposed, or a custom base interface:This closes the accidental-mutation surface without any runtime cost.
3.
SecurityUtilsas a static helper with injectable dependencies is a layer smellA static method that takes a service as a parameter is a sign the abstraction is at the wrong level. The alternative — a Spring
@Component— is testable with@Mockwithout needing static method mocking. Given that this is called from three controllers, the refactor is small:The current static form works and is tested — this is a suggestion, not a blocker.
4.
DashboardServicereaches intoTranscriptionServiceacross a domain boundaryDashboardServicecallstranscriptionService.listBlocks(docId)to read block state. The transcription domain owns block data. This is acceptable since it goes through the service interface, but as the dashboard grows it may accumulate cross-domain reads. IfDashboardServicestarts needing more transcription data, consider whether a dedicatedDashboardSummaryQueryquery object (like the existingAuditLogQueryService) is the right home for aggregated reads.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
The security posture is generally good: all three new dashboard endpoints are protected by
@RequirePermission(Permission.READ_ALL)at the class level, all SQL queries use named@Paramparameters (no concatenation), and the JSONB operatorpayload->>'mentionedUserId' = :currentUserIdis parameterized correctly. TheMath.min(limit, 20)cap on the activity feed is proper input bounding.Blockers
1.
AuditKind.valueOf(row.getKind())leaks internal state on unknown enum valuesIf the database contains an audit kind not in the current Java enum (after a rollback, schema migration, or future kind added to DB without redeployment), this throws an unhandled
IllegalArgumentException. Spring's default exception handler will return a 500 with the message:"No enum constant org.raddatz.familienarchiv.audit.AuditKind.SOME_UNKNOWN_KIND"— which exposes internal class names and enum values to any authenticated caller. Treat unknown kinds gracefully:Suggestions
2. Append-only audit_log enforcement is documented but not enforced at the DB layer
V47 contains:
The comment in V46 correctly notes that
REVOKEis a no-op for the table owner. This means the append-only guarantee is only at the application layer — any SQL migration, psql session, or future service that has superuser or owner privileges can silently delete audit rows. For a family archive this is an acceptable risk. For compliance purposes it would need a dedicated application role with onlyINSERTonaudit_log. Worth documenting in adocs/adr/note.3.
toActorDTO()lacks null-check onuser.getEmail()Actually this is already guarded —
u.getEmail() != null. LGTM. Butu.getEmail().substring(0, 1)will panic on an empty string email. An email with empty string is unlikely but:4.
DashboardService.getResume()catches bareExceptionand silently continuesCatching
Exceptionalso swallowsOutOfMemoryError,StackOverflowErroretc. (which extendRuntimeException→Exception). Consider catchingDomainExceptionspecifically, or at mostRuntimeException. Same pattern appears in the collaborator loop — acceptable given thelog.warn()is present, but consider narrowing the catch type.5. Permission boundary test gap (not a vuln, but a regression risk)
DashboardControllerTestcorrectly tests 401 (unauthenticated) and 403 (no permissions) for/resumeand/pulse. However,/activityis only tested for 401 and 403 — there's no test asserting thatlimit=100(above the cap) is clamped to 20. Add:🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Excellent test coverage for a feature of this size. You have unit tests (
DashboardServiceTest,SecurityUtilsTest,AppUserTest), controller slice tests (DashboardControllerTest), and integration tests against real Postgres (AuditLogQueryRepositoryIntegrationTest,AuditServiceIntegrationTest). Awaitility is properly used instead ofThread.sleep()inAuditServiceIntegrationTest. The red/green pattern is visible in test naming (e.g.logAfterCommit_registersCallback_andSubmitsToExecutor_afterCommit).Blockers
1. V46 migration modification will break CI on existing databases
Any CI pipeline or local dev environment that already has V46 applied will fail Flyway validation at startup, meaning the integration test suites (
AuditServiceIntegrationTest,AuditLogQueryRepositoryIntegrationTest) will fail on the migration phase before even reaching the test assertions. This is a test infrastructure blocker — see Felix's comment for the fix.Suggestions
2. Missing test:
DashboardService.getActivity()with unknownAuditKindin DBgetActivity()callsAuditKind.valueOf(row.getKind())which throwsIllegalArgumentExceptionfor unknown kinds. No test covers this failure path:3. Missing test:
DashboardControlleractivity limit cappingMath.min(limit, 20)in the controller is business logic that deserves a test:4.
DashboardServiceTesttest forgetActivity()verifies only 2 items — not bulk loading intent fullygetActivity_loadsDocumentTitles_withoutPerRowLookuppasses two identicalrowmocks for the samedocId. This verifies deduplication but doesn't assert on the title of the second item — it only checksitems.get(0).documentTitle(). Both items should be checked:5.
AuditLogQueryRepositoryIntegrationTest—getPulseStats_countsAnnotationsTranscriptionsAndUploadsfragile assertionisGreaterThanOrEqualTo(1)is a loose assertion. The test inserts exactly 1 annotation and 1 text_saved forUSER_IDonDOC_IDpage 1 — soyourPagesshould be exactly 1. UseisEqualTo(1)to pin the behavior precisely.6. Empty placeholder in
ContributorStackhas no test for the accessible nameContributorStack.svelte.spec.tstests thetitleattribute of the empty placeholder viagetByTitle('Noch niemand angefangen'), buttitleis not accessible to screen readers as an ARIA label. The test should verifygetByRole('img', { name: 'Noch niemand angefangen' })if the element hasrole="img", or the title should be supplemented witharia-label. Currently the empty placeholder has neitherrole="img"noraria-label— onlytitle.7.
buildCaption()inDashboardServicehas no unit testThe method produces "Frieda an Wilhelm · 1923" style captions. No test covers: only sender, only receiver, only date, or all three. These are easily unit-tested in
DashboardServiceTest.🎨 Leonie Voss — UI/UX Design Lead
Verdict: ⚠️ Approved with concerns
The redesign direction is right: 2-column grid, sticky right sidebar, personalised greeting, and avatar stacks all improve the dashboard's human feel. Component splitting is clean —
ContributorStack,DashboardResumeStrip,DashboardFamilyPulse,DashboardActivityFeedeach represent a single named visual region. The ARIA work on avatars is strong.Blockers
1.
ContributorStackempty-statetitleis hardcoded German and not accessibleTwo issues:
titleis not announced by screen readers. The element should haverole="img"andaria-labelto be accessible.contributors_none) and usearia-label:Suggestions
2. Avatar size
h-[22px] w-[22px]is fine for decorative avatars — confirmed non-interactiveContributorStack avatars are decorative (not buttons), so the 44px touch target rule doesn't apply. The
ring-2 ring-whiteoverlap stack pattern is visually clean. ✅3. Fallback avatar color
#8c9aa3is not in the brand palette#8c9aa3(a neutral slate) falls outside the 8-color palette defined inAppUser.java. Since the backend guarantees a palette color for every user with an ID, an emptycolorin the DTO means either:@PreUpdate(the@PostLoadonly sets the field in memory, not persisted)Use one of the 8 palette colors as the fallback, e.g.
#3060b0, to keep avatars on-brand.4. Greeting
h1uses a raw pixel size — prefer a semantic scale tokentext-[2rem]is an arbitrary value. Tailwind'stext-3xlis1.875remandtext-4xlis2.25rem. For consistency with the rest of the typography scale, usetext-3xlor define a custom token inlayout.css:Minor concern — the value renders fine.
5. Right sidebar
lg:sticky lg:top-[80px]— verify sticky header height80pxis hardcoded. If the top nav height changes or the upload button makes the header taller on some viewports, this offset will need updating. Prefer a CSS custom property:6.
DashboardActivityFeedsection label istext-ink-3— CLAUDE.md pattern usestext-gray-400The CLAUDE.md section title pattern uses
text-gray-400. Iftext-ink-3is a design token alias for gray-400, this is fine — otherwise it's a brand deviation. Please verify--color-ink-3maps to#9ca3af(gray-400) inlayout.css.7.
DashboardActivityFeed— activity items have no empty stateIf
feedis empty (fresh database, no audit events), the section renders with a heading and no items — leaving a blank white card. Add an empty state:🛠️ Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
Verdict: ⚠️ Approved with concerns
No infrastructure changes in this PR (no Compose changes, no CI changes, no new services). The Awaitility dependency addition is correct — it replaces
Thread.sleep()in async tests, which is exactly the right tool. However, there is one deployment risk that must be resolved before this merges to a deployed environment.Blockers
1. V46 migration modification breaks Flyway checksum validation on deployed environments
V46__add_audit_log.sqlhas been edited in place. Flyway stores a checksum of every applied migration. If this migration was already executed on the production VPS (or any staging/dev environment), the application will refuse to start with:This is a production deployment blocker. The options:
REVOKE UPDATE, DELETE ON audit_log FROM CURRENT_USER). V46 should not be changed.Running
flyway repairwould be the workaround, but that's a manual intervention step that breaks the zero-touch deployment goal.Notes (no action required)
2. Existing users will show empty avatars until next
@PreUpdatefiresV47 adds
color VARCHAR(20) NOT NULL DEFAULT ''— empty string default. The@PostLoadcallback onAppUsersetsthis.colorin memory but does not persist it. The computed color is only written to the DB on the next@PreUpdate. Until a user's record is saved (login doesn't trigger this — only an explicit update), their color in theuserstable will be''.The dashboard will receive
color: ""from the API andContributorStackwill fall back to the hardcoded non-palette color (#8c9aa3). This is a silent degradation for existing users.A migration that backfills color values on V47 would fix this:
Since color is cosmetic, this is a suggestion, not a blocker.
3. Awaitility version is managed by Spring Boot BOM
No version specified — Spring Boot BOM manages it. Correct pattern. ✅