feat(ui): Korrespondenz redesign — compact strip, log cards, single-person mode #164
Reference in New Issue
Block a user
Delete Branch "feat/issue-162-korrespondenz-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 #162
Summary
/conversations→/korrespondenz(301 redirect from old URL), updates nav label in all three languagesCorrespondenzEmptyStatewith localStorage recent-persons chipsreceiverIdDateInputcomponent anddateutility functionsTest plan
npm run checkpasses with no new type errorsnpm run test— all Vitest unit tests pass (including new DateInput and date util specs)/conversationsredirects to/korrespondenzwith 301🤖 Generated with Claude Code
Blockers resolved: - localStorage key collision: UsersListPanel/GroupsListPanel/TagsListPanel now each use their own key (admin_*_list_collapsed) - $effect autocollapse replaced with $derived(autocollapse || manualCollapse) across all three list panels (Felix — Svelte 5 rule violation) - groups/new: add READ_ALL and ANNOTATE_ALL to available standard permissions - Mobile back-to-list links added to all five detail panel headers (md:hidden) so users landing directly on a detail URL on mobile can navigate back - onDestroy(() => stopPolling()) added to system/+page.svelte (Tobias) High priority resolved: - Permission labels in groups/[id] and groups/new now use Paraglide i18n keys (admin_perm_read_all, admin_perm_annotate_all, etc.) across de/en/es - $derived used for permission arrays (reactive i18n) — Felix Svelte 5 rule - UserGroup type in +layout.server.ts now uses generated API type (Markus/Felix) - discardTarget annotation changed to variable-level type annotation Accessibility (Leonie): - EntityNav tablet icon strip buttons: min-h-[44px] for WCAG 2.5.8 compliance - Flyout focus management: openFlyout() focuses first link, closeFlyout() returns focus to the trigger button that opened it - Flyout animation replaced: broken inline style -> transition:fly={{ x: -160 }} Tests (Sara/Felix): - localStorage key assertion tests added per panel - localStorage.removeItem calls updated to use the panel-specific keys - page.server.spec.ts added for groups/[id] and tags/[id] delete actions - Polling lifecycle tests added to system/page.svelte.spec.ts Note: Paraglide types for new admin_perm_* keys regenerate automatically on next npm run dev (Vite plugin). No manual compilation step needed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Update empty-state, swap-button, and new-doc-link tests to match redesigned components. Add new tests for: single-person hint bar visibility, recent-persons chips from localStorage, corrupt localStorage graceful handling, Row 2 aria-disabled state, and strip letter count in single-person and bilateral modes. Fix CorrespondenzEmptyState to use {id, name} storage format matching persistRecentPerson in +page.svelte. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>- Icon: 36px (was 24px), heading text-xl font-black (was text-sm) - Subtext: text-base (was text-xs), max-w-sm (was max-w-[280px]) - Search button: h-10, text-sm, full max-w-sm width (was fixed 260px) - Recent persons divider and chip block moved inside the {#if recentPersons.length > 0} block so no blank "oder" section renders when localStorage is empty - Chips: text-sm px-4 py-2 (was text-xs), avatar h-5 w-5 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
This is a substantial, well-structured feature. The component decomposition is clean —
CorrespondenzPersonBar,CorrespondenzFilterControls,ConversationTimeline,CorrespondenzEmptyState,SinglePersonHintBar,CorrespondentSuggestionsDropdown— each named for a single visible region. TDD evidence is solid: unit tests precede the implementation structure and test meaningful behaviors. A few issues need attention before merging.Blockers
1.
$effectused for state synchronization where$derivedwould be correct —+page.svelteThe sync block in
+page.sveltebreaks command-query separation and creates an extra reactive cycle:The state variables
senderId,receiverId, etc. are initialized withuntrack(() => data.filters.senderId)and then re-synced in an effect. The persistence side-effect inside the effect is the real issue: mixing a side-effect (localStorage write) with state synchronization. Split it:Note: if
senderIdandreceiverIdneed to be locally writable for immediate UI feedback before navigation, then$state+untrackinit is valid — but then the$effectsync is wrong because it overrides local writes on every navigation. Choose one model.2.
messages-extra.tsshim is a production debt, not just a dev convenienceThe file ships with a
TODO: Remove this filecomment and is German-only (no locale switching). Given the app supports de/en/es, any UI path importing frommessages-extrasilently ignores the user's locale. This is a regression from the current behavior. The keys are already inde.json,en.json,es.json— the blocker is the Paraglide compile step. Either run it before merging, or at minimum import the generated functions from$lib/paraglide/messages.jsdirectly (they should exist after a compile).3.
konversationenlabel not updated in+page.svelteheading areaCorrespondenzEmptyStatehas the heading hardcoded in German:Korrespondenz durchsuchenandWähle eine Person aus dem Archiv...— not using them.conv_empty_heading()andm.conv_empty_text()message functions. This is inconsistent with the rest of the codebase and breaks i18n for EN/ES users.4. Missing
{#each}key oncountsByYeariteration —ConversationTimeline.sveltecountsByYearusesnew Map<number, number>()which is not aSvelteMap. In a reactive derived context this is fine since it's rebuilt each time, but the year divider section iteratesenrichedDocumentswhich is keyed correctly. No issue here — but the plainMapused as a reactive source inside$derivedis fine since$derivedre-runs on dependency change. This is actually correct usage. No action needed.Suggestions
S1.
CorrespondentSuggestionsDropdownfetches inonMount— violates the DI ruleThe pattern violates the rule: "data flows in via props from server load". For a suggestions dropdown that opens on user interaction this is a pragmatic exception, but worth calling out. If this endpoint ever moves or needs auth headers, the component will break silently. Consider passing the fetch result in as a prop, or at least using the SvelteKit
fetchfrom the page context rather than a raw browserfetch.S2.
PersonTypeaheadeslint-disable comment is defensive, not explanatoryThe comment above it explains the reason well. But the rule being disabled suggests the linter has a point — this is a pattern worth filing as a known exception with a reference to why writable
$deriveddoesn't work here (it's read-only in Svelte 5). The comment is good, the disable is acceptable here.S3.
statusDotClassuses raw Tailwind color names — no design tokenThese don't map to the project's brand token palette (
brand-navy,brand-mint,brand-sand). APLACEHOLDERstatus usingbg-yellow-400may clash with the brand. Suggest aligning with Leonie on the correct semantic colors for document status dots.S4. No redirect from
/conversationsto/korrespondenzThe PR title mentions "301 redirect" but there is no
frontend/src/routes/conversations/+server.tsor hooks-based redirect in the diff. Anyone with a bookmarked/conversationsURL or an external link will hit a 404. Add:S5.
EntityNavtablet flyout: all four trigger buttons call the sameopenFlyoutfunctionThe flyout opens a generic panel but all four section buttons (Users, Groups, Tags, System) call the same handler. The flyout's content is always the full nav repeated — it doesn't show the clicked section. This is probably intentional (it shows all sections in a wider panel), but the UX is confusing: clicking "Tags" opens a panel showing all sections. If intentional, add a comment; if not, this is a bug.
🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
The architectural direction is sound. Moving from a two-person bilateral-only model to a flexible single/bilateral query mode is the right call — the old model was too restrictive for a family archive where you often want "show me everything involving this person." The admin section refactor from a single monolithic page to routed sub-sections with
+layout.server.tsis correct SvelteKit architecture. Two concerns need resolution before merge.Blockers
1. Admin
+layout.server.tsmakes three parallel API calls on every admin subroute load — including data the caller doesn't needThis fetches the full user list, full group list, and full tag list on every admin page load — including the System page, which doesn't render any of those entities. The layout only needs the counts for the EntityNav. This should either be a dedicated
/api/admin/statsendpoint returning counts, or deferred to the sub-route load functions. As written, every admin page hit loads all users and all tags regardless of what the user is viewing. On a large archive this will be slow.Proposed architecture: move entity-specific data fetching to the individual sub-route
+layout.server.tsfiles (which are already present in the diff:admin/users/+layout.server.ts,admin/tags/+layout.server.ts,admin/groups/+layout.server.ts). The top-level layout should only fetch what the layout itself renders (nav counts), not what every sub-page might need.2.
korrespondenz/+page.server.tsswallows API errors silentlyThere is no check on
result.response.ok. If the backend returns a 500,datawill be undefined,documentssilently becomes[], and the user sees an empty result instead of an error. The CLAUDE.md pattern is explicit:This should throw, not silently degrade. A user searching for correspondence and seeing zero results when there's a backend error will assume the archive is empty, not that something failed.
Suggestions
S1. The
messages-extra.tsshim is the wrong approach for a locale-supporting appI understand the ownership issue (Paraglide generated files are owned by the compile step), but shipping a German-only shim that bypasses the locale system is architectural debt that will quietly break EN/ES users. The right fix is to run the Paraglide compile as part of the CI pipeline before the frontend build, not to shim around it. This is a pipeline concern (see Tobias), not a code concern — but the architectural consequence is real.
S2. The
findSinglePersonCorrespondenceJPQL query may produce duplicate resultsA document with multiple receivers will appear once per receiver due to the JOIN, but
DISTINCTshould handle this. Worth adding a backend integration test (not just a mock-based unit test) that verifies a document with 3 receivers matchingpersonIdreturns exactly one result. This is the kind of edge case that passes unit tests and fails in production against real Postgres.S3. Admin permission model:
canManageGroupsmaps toADMIN_PERMISSION, notADMIN_GROUPThe variable name
canManageGroupsimplies group management, but the permission being checked isADMIN_PERMISSION. IfADMIN_PERMISSIONis the correct permission for group management, the variable should be namedcanManagePermissionsor the permission should beADMIN_GROUP. Naming mismatch between the UI flag and the backend permission creates confusion during future audits.S4. The
/conversations→/korrespondenzrename has no redirectAny bookmarked URL, shared link, or external link to
/conversationsnow 404s. A SvelteKit+server.tsGET handler that issues a 301 with query string passthrough should be added. This is not optional for a URL that was previously in production.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
The test coverage in this PR is genuinely impressive for a feature of this size. Unit tests for
DateInput,date.spec.ts,page.svelte.spec.tsfor the korrespondenz route, and specs for every admin sub-route layout — that's a real pyramid being built. The E2E spec is thoughtfully structured with graceful skips for data-dependent scenarios. A few gaps remain that I won't let slide.Blockers
1.
korrespondenz/+page.server.tsload function has zero test coverageThe load function handles the core data-fetching logic for the entire feature — it branches on
senderId,receiverId, fetches persons and documents in parallel, and computescanWrite. There is nopage.server.spec.ts(or equivalent) for this file. The pattern for testing SvelteKit load functions by importing them directly is established in the admin sub-routes (e.g.admin/layout.server.spec.ts) — apply it here.Minimum required tests:
load returns empty documents when no senderId is setload calls conversation endpoint when senderId is setload calls single-person endpoint when receiverId is absentload derives canWrite correctly from user groupsload does not throw when backend returns non-ok (graceful degradation or propagated error — either way, test the behavior)2. E2E tests do not run
checkA11yon any pageEvery E2E test should include an accessibility check per the project standard. The
korrespondenz.spec.tstakes screenshots but never callscheckA11y. This is a quality gate violation:The empty state, single-person mode, and bilateral mode all need axe checks. The SinglePersonHintBar uses an emoji (
📋) as a visible character — axe will flag this because the emoji is not wrapped in anaria-hiddenspan.3.
findSinglePersonCorrespondenceJPQL query has no integration test against real PostgresThe service unit test mocks the repository and verifies routing — that's correct and present. But
DISTINCT+LEFT JOINbehavior on multi-receiver documents needs verification against a real database. H2 is not acceptable as a substitute. A@DataJpaTest+ Testcontainers test that:personIdfindSinglePersonCorrespondenceThis is exactly the class of bug that passes mocks and fails in Postgres.
Suggestions
S1.
DateInput.svelte.spec.ts— thedomFlushhelper is a timing smellA 50ms
setTimeoutis a hardcoded wait. In CI under load this may not be enough. Replace withwaitForfrom@testing-library/svelteor Vitest'sexpect.poll():This is more reliable and doesn't add 50ms to every test that uses it.
S2.
PersonServicewhitespace behavior change has a test but the old test is simply deletedThe old test
findAll_returnsAll_whenQueryIsBlankwas deleted and replaced withfindAll_returnsEmpty_whenQueryIsWhitespaceOnly. The behavior change is intentional and documented, but the deletion of the old test means there's no test verifying the previous contract was intentionally broken. At minimum, a comment in the new test explaining that this is a deliberate behavior change (and a reference to the PR/issue) helps future maintainers understand why whitespace now returns empty instead of all.S3. E2E bilateral tests use
test.skipwith runtime conditions — fragile against empty databasesA test that silently skips because of missing data is a false green. In CI with a seeded database this should never skip. Ensure the E2E database fixture has at least one bilateral correspondence pair, and make the test unconditional. The skip guard is acceptable only for local dev where data may vary.
S4. Missing coverage:
CorrespondenzFilterControlsaria-disabledstateThe filter strip uses
aria-disabled={!senderId}but there's no component test asserting this attribute is set correctly whensenderIdis empty. This is a WCAG-relevant attribute and should be in the spec.🔐 NullX (Nora Steiner) — Application Security Engineer
Verdict: ⚠️ Approved with concerns
One confirmed security finding, one high-severity concern, and several smells that deserve documentation. No RCE or auth bypass — but the test endpoint in the API spec is a real risk in a family archive context.
Blockers
1.
/api/auth/reset-token-for-testis exposed in the generated API spec — CWE-200 (Information Exposure)The generated
api.tsincludes:with parameters:
This endpoint returns a reset token for a given email address via a GET request. Even if the backend endpoint is guarded by a profile flag (
@Profile("dev")or similar), it is now documented in the frontend API client that ships to all users. The immediate concern:GET /api/auth/reset-token-for-test?email=...endpoint exists. This narrows their attack surface significantly.Required action: Verify the backend controller is annotated with
@Profile("dev")and that the dev profile is never active in production. Additionally, exclude this path from OpenAPI spec generation in non-dev profiles so it does not appear in the generated client. In Spring:Or in
application.properties:Detection note: Add a Semgrep rule or CI check that fails if any path matching
*test*or*debug*appears in the generatedapi.tson merge to main.2.
UserController.getUserwas previously unauthenticated — confirmed fix, but regression test needed — CWE-862 (Missing Authorization)This is the right fix. The
@RequirePermissionannotation is now present and the controller test covers both the 403 and 200 cases. However, the@RequirePermissionaspect applies at runtime — if the Spring AOP proxy is ever misconfigured (e.g.@EnableAspectJAutoProxy(proxyTargetClass=false)and a final class), the annotation silently stops working. Recommend adding an@WebMvcTestintegration-slice test (not just a unit test with@WithMockUser) that verifies the endpoint actually returns 403 when called without theADMIN_USERauthority — using Spring Security's real filter chain.Informational / Smells
I1.
localStoragestores person IDs and names without validation on readThe
as RecentPerson[]cast does no runtime validation. If an attacker can write tolocalStorage(via XSS on any other page), they could inject{ id: "...", name: "<script>..." }entries. The name is rendered as text content (Svelte auto-escapes), so there is no XSS risk here — but the lack of schema validation means corrupted or crafted entries could cause unexpected behavior. The corrupt JSON test exists and passes, which is good. This is informational, not a blocker.I2.
CorrespondentSuggestionsDropdownuses rawfetchwithout credentials checkThe raw
fetchcall does not explicitly setcredentials: 'include'(same-origin cookies are sent by default in modern browsers, so this is fine). Not a vulnerability, but worth noting that if the auth cookie ever becomes SameSite=None for cross-origin SSR, this call may silently fail authentication. Noted for future cross-origin deployment planning.I3. Admin
+layout.server.tspermission check runs on the frontend but the actual enforcement is backendThis is the correct pattern for user-facing access control in SvelteKit SSR. The backend APIs still enforce permissions independently via
@RequirePermission. No issue here — defense in depth is working as intended. Recording this explicitly so future reviewers don't "simplify" the frontend check assuming the backend is sufficient.🎨 Leonie Voss — UI/UX Design Lead
Verdict: ⚠️ Approved with concerns
The compact filter strip is a genuine improvement over the old card layout — it frees vertical space for the actual content, which is what matters on a family archive. The log card timeline reads clearly. The asymmetry bar is a lovely archival insight — seeing at a glance who wrote more in a correspondence is genuinely useful. A few accessibility and brand issues need to be addressed before this goes to users.
Blockers
1.
SinglePersonHintBaruses hardcoded amber colors — not brand tokens, and contrast is unverifiedThese are Tailwind's amber palette (
amber-300,orange-50,amber-900roughly), not the project's brand token system (brand-navy,brand-mint,brand-sand). The contrast of#92400Eon#FFF7EDis approximately 6.4:1 — that passes WCAG AA, so it's not a contrast failure. The issue is design consistency: this component uses a completely different color vocabulary from the rest of the app. The hint bar should use semantic tokens from the existing theme, not raw hex values.Proposed replacement using existing brand tokens:
This brings it into the brand vocabulary while preserving the "informational banner" feeling.
2.
SinglePersonHintBaruses a 📋 emoji as UI chrome — WCAG 1.1.1 failureThe emoji is marked
aria-hidden="true"which is correct — it won't be read by screen readers. However, it renders as a Unicode emoji whose appearance varies wildly between operating systems and browsers. On some platforms it renders as a clipboard icon, on others as a colorful cartoon. For consistent UI chrome, use an SVG icon (matching the style used everywhere else in the codebase). The project already uses Heroicons stroke style consistently.3.
CorrespondenzFilterControlsuses<input type="date">— not the project'sDateInputcomponentThe project has a
DateInputcomponent (DateInput.svelte) that formats dates in German style (dd.mm.yyyy) and includes validation. The filter strip uses<input type="date">which renders the browser's native date picker — appearance varies significantly between browsers and OSes. On mobile (Safari/iOS) it shows a native picker wheel that is inconsistent with the rest of the form. TheDateInputcomponent exists specifically to solve this. Even in a compact strip, using the existingDateInputcomponent (withcompactmode, similar to howPersonTypeaheadreceived acompactprop in this PR) maintains visual consistency.Suggestions
S1. Touch targets in the filter strip fall below 44px on desktop
On
sm:(≥640px), themin-h-[44px]override is removed, leaving the input ath-8= 32px. The WCAG 2.2 target size minimum is 24×24px (SC 2.5.8 minimum), but the recommended target is 44×44px. At 32px height the date inputs are technically compliant but below the comfortable touch target on tablet viewports where fingers are in use. Removingsm:min-h-0would keep the 44px height across all sizes, which at 8px per cell in the strip adds only 12px to the strip height on small screens.S2. The asymmetry bar is color-only — direction not conveyed without color
The
→and←arrows do convey direction via text, which is good. The primary/accent color distinction additionally reinforces which person is which. For colorblind users (particularly deuteranopia),text-primaryandtext-accentmay not be distinguishable depending on the exact color values. The arrows are the crucial differentiator here — they carry the meaning without color. This is acceptable, but worth a quick check thataria-labelon therole="img"container is readable: the current label"Briefverteilung in diesem Zeitraum: {outCount} von {senderName}, {inCount} von {receiverName}"is excellent.S3.
CorrespondenzEmptyStateheading and subtext are hardcoded German stringsThese strings are not using the
m.conv_empty_heading()andm.conv_empty_text()message functions. EN/ES users will see German. The translation keys are defined in all three locale files.S4.
EntityNavat tablet width (md) shows icon-only navigation with no visible label on the buttonsThe button shows only the icon and a count number (not the label) at tablet width. Screen readers get the
aria-label— that's correct. But sighted users who don't recognize the icon will not know what the button does without hovering (and no tooltip is shown on click). Add atitleattribute to match thearia-label, which provides a native browser tooltip on hover without added complexity.S5.
w-30is not a standard Tailwind utility — verify custom config existsw-30is not in Tailwind's default scale (w-28= 7rem,w-32= 8rem). Either this is a custom value defined intailwind.config.js, or it silently does nothing (the nav would use auto width). Verify this renders at the intended width.⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ⚠️ Approved with concerns
This PR is primarily a frontend feature — it doesn't touch
docker-compose.yml, CI pipeline, or infrastructure configuration, which is the right scope for a UI redesign. However, there are two operational concerns I need to flag: one is a data issue on the build pipeline, and one is a deployment concern for the URL rename.Blockers
1. The Paraglide compile step is not enforced in CI before the frontend build
messages-extra.tsexists specifically because the Paraglide-generated files insrc/lib/paraglide/couldn't be regenerated (file ownership issue). The workaround ships German-only strings to EN/ES users. The root fix is a pipeline change:The CI
unit-testsjob should runnpm run dev(or a dedicatednpm run generate:i18n/paraglide compilestep) before runningnpm run checkandnpm run test. Paraglide generates from the JSON message files — those files are in the repo and up to date. The compile step is deterministic and fast (milliseconds). There is no reason to skip it in CI.Concrete CI change needed (in whatever job runs the frontend build):
This eliminates the need for
messages-extra.tsentirely and ensures any future message key additions are caught in CI before they ship as German-only strings.2. The
/conversations→/korrespondenzURL rename has no redirect — operational impactIf this app is in production and family members have bookmarked
/conversations?senderId=...&receiverId=...links (which the old UI would have encouraged via the "Häufige Korrespondenten" section), those links will 404 after this deploy.A SvelteKit server route handles this at zero runtime cost:
This is a one-file, six-line change. There is no reason to skip it. The 301 cache means browsers and search engines will update their indexes. Add a note to the deployment runbook (if one exists) that old
/conversationslinks will redirect.Suggestions
S1. The admin section refactor more than doubles the number of frontend route files — consider the maintenance surface
The old admin page was one route with tabs. The new structure has:
admin/+layout.server.ts,+layout.svelte,+page.svelteadmin/users/+layout.server.ts,+layout.svelte,+page.svelte,UsersListPanel.svelte,[id]/+page.server.ts,[id]/+page.svelte,new/+page.server.ts,new/+page.svelteThis is architecturally correct (each sub-entity has its own route), and for a growing feature this is the right call. Just noting that the maintenance surface grew significantly in one PR. Ensure the CI test run time for the admin section specs is profiled — 15+ spec files with
vitest-browser-sveltecan add meaningful time to the unit test job.S2. Screenshot test artifacts in
test-results/e2e/— ensure CI cleans these upThese screenshots are useful for debugging failed runs but will accumulate across CI runs if not cleaned up. Ensure the CI job either uses a fresh workspace each run (standard for containerized CI) or has a
rm -rf test-results/step before the E2E job. If using Gitea Actions artifact upload, set a retention policy (7 days is typically enough for debugging).