feat: Briefwechsel hero redesign — discovery framing + padding #186
Reference in New Issue
Block a user
Delete Branch "feat/issue-179-briefwechsel-hero"
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?
Summary
/korrespondenz→/briefwechseland all labels from "Korrespondenz" to "Briefwechsel" (de/en/es)max-w-7xl px-4 sm:px-6 lg:px-8)h-12for better touch targetsKey changes
CorrespondenzHerocomponent: centred headline "Wessen Briefe möchten Sie lesen?", cross-link to document search,h-14PersonTypeahead (auto-focused), recent persons chipsPersonTypeaheadlargeprop: newh-14size variant for the hero inputCorrespondenzEmptyState(fully replaced), focus delegation hackTest plan
svelte-check— 0 new errors/briefwechsel?senderId=Xskips hero and renders results directlyCloses #179
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
What I checked
TDD evidence, clean code, Svelte 5 patterns, component sizing, naming.
Positives
CorrespondenzHero.svelte.spec.tscovers headline, cross-link, typeahead presence, recent persons rendering, and chip click behaviour. Good.CorrespondenzHeromaps to exactly one visual region. The oldCorrespondenzEmptyState+ focus-delegation hack is gone — replaced by a proper component that owns its own typeahead. Correct split.{#each}blocks are keyed (person.id) — Svelte 5 rule followed.$derivedfor computed values (isSinglePerson,senderName,receiverName) — no$effectabuse.CorrespondenzEmptyState.sveltedeleted entirely, focus delegation hack removed fromselectPerson.Concerns
PersonTypeaheadclass ternary is getting unwieldy (CorrespondenzPersonBar.svelteuses[&_input]:h-12to override thecompactinput'sh-9). This works but is a CSS specificity hack. Thelarge/compact/default ternary inPersonTypeahead.svelte:142-148is already three branches of long class strings. Consider extracting asize: 'compact' | 'default' | 'large'prop with a lookup map instead of nested ternaries. Not a blocker — the current code works — but it'll be harder to extend next time.selectPersonno longer guards against emptyid. The old code hadif (!id) { focus(); return; }. The new code unconditionally setssenderId = idand callsapplyFilters(). IfonSelectPerson('')is somehow called (e.g. from a future code path), it will navigate to/briefwechsel?dir=DESCwith an empty senderId, which works but wastes a navigation. Low risk sinceCorrespondenzHero.handlePersonChangeguardsif (id), but the parent function is exposed.Hardcoded German text:
CorrespondenzHero.svelte:63hasoderhardcoded in the divider. This should use a Paraglide message key for consistency with the i18n approach used everywhere else.Suggestions
onMount+await tick()+focus()pattern inCorrespondenzHerois fine but consider usingbind:thison the input directly (via a newautofocusprop onPersonTypeahead) to avoid the DOM query. Not blocking.🏗️ Markus Keller — Application Architect
Verdict: ✅ Approved
What I checked
Layer boundaries, state management, SSR behaviour, coupling, module structure.
Assessment
!senderId→ hero,senderId→ results) driven by$derivedbooleans. No$effectchains, no cycles. The{#if}/{:else}branching in+page.sveltemaps directly to the state machine. This is exactly the pattern I'd recommend./briefwechsel?senderId=Xwill hydrate directly into the results state because the server load function populatesfilters.senderId. The hero component is never mounted server-side for these URLs. Correct behaviour, no special handling needed.CorrespondenzHeroreceivesrecentPersonsas a prop from the parent, which reads localStorage inonMount. Data flows down, events flow up. No child component fetching its own data./conversationslegacy route is unaffected — it was already separate.RecentPersoninterface duplicated in+page.svelteandCorrespondenzHero.svelte. Minor — could be extracted to a shared type, but at two usages it doesn't warrant a shared module yet. KISS applies.🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
What I checked
Test coverage, test quality, edge cases, missing scenarios.
Positives
conv-heronot present whensenderIdset, and vice versa). Good.data-testid+document.querySelector— reliable approach that matches the existing test patterns in this project.Concerns
Removed test:
aria-disabledwhen no senderId. The old suite had a test verifying filter controls render witharia-disabled="true"when no person is selected. Since filter controls are now hidden in the hero state (not disabled), this test was correctly removed — but there's no replacement test verifying that the hero state is accessible. Consider adding: hero landmarks, heading level, cross-link discoverability for screen readers.No test for auto-focus behaviour. The hero's
onMountcallsfocus()on the typeahead input. This is a key UX requirement from the issue (users shouldn't have to click). No test verifies this. A test likeexpect(document.activeElement).toBe(input)after render would lock this behaviour in.Missing edge case:
recentPersonswith empty array vs undefined. The prop defaults to[], but what if someone passesundefinedexplicitly? The{#if recentPersons.length > 0}would throw. Low risk since TypeScript catches this, but a defensive test would be nice.Suggestions
page.server.spec.tswas only renamed (no content changes). Consider adding a test that verifies the load function returns empty data when no query params are present, confirming the hero state is the default server-side.🔒 Nora "NullX" Steiner — Security Engineer
Verdict: ✅ Approved
What I checked
XSS vectors, injection points, auth boundaries, data exposure, localStorage handling.
Assessment
JSON.parseis wrapped in try/catch in both the read (onMount) and write (persistRecentPerson) paths. Corrupt data doesn't crash the app. The parsed data is used for display only (person names rendered via Svelte's auto-escaping), not for navigation or API calls with the name — only theidis used in URLs, which is a UUID.{@html}usage: all dynamic content is rendered through Svelte's auto-escaping. The person names from localStorage go through normal text interpolation. Good.href="/"is static — no user-controlled redirect target. Safe.PersonTypeaheadsearch term goes throughencodeURIComponentin the fetch URL (existing code, not changed in this PR). No injection vector there.data-testidattributes added to components are benign — they expose no sensitive information and are standard practice for test hooks.No findings. LGTM from a security perspective.
🎨 Leonie Voss — UX Design Lead
Verdict: ⚠️ Approved with concerns
What I checked
Design fidelity to the agreed spec, accessibility, responsive behaviour, touch targets, typography, brand consistency.
Positives
h-14(56px) comfortably exceeds the 44px minimum for seniors and mobile users. Good.text-2xl, body text attext-base(16px minimum), helper text attext-sm(14px). Meets the dual-audience requirement.Concerns
Hardcoded "oder" in the divider (
CorrespondenzHero.svelte:63). This must be a Paraglide key — English users will see a German word in the middle of an otherwise English interface. This is a blocker for i18n correctness.Cross-link placement: the cross-link sits above the headline. In the design discussion, I specified it should be above the typeahead, not above the headline. The current order is: cross-link → headline → typeahead. Consider: headline → cross-link → typeahead. This keeps the headline as the first thing users see, with the escape hatch just below it. Not blocking — visual hierarchy still works — but worth iterating on.
py-20on the hero is generous vertical padding (~80px top and bottom). On short viewports (e.g. landscape tablet), this might push the typeahead below the fold. Considerpy-12 sm:py-20for a responsive approach.Suggestions
h-12(48px) in the results state are correct per spec. The[&_input]:h-12override works but would be cleaner as a dedicated prop. Cosmetic — not blocking.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
Infrastructure impact, CI pipeline, bundle size, environment variables, Docker Compose changes.
Assessment
PersonTypeaheadlargeprop is a pure CSS change. No new npm packages, no animation libraries. Bundle size impact is negligible.korrespondenz/tobriefwechsel/is handled cleanly by git rename detection. SvelteKit regenerates route types automatically onsvelte-kit sync. No manual steps needed.npm run checkandnpm run buildon every push. The new components will be type-checked and built automatically. No workflow changes needed.No findings. Clean change from an ops perspective.
Review concerns addressed
All reviewer concerns resolved in 2 new commits:
e2af9f9— fix(i18n): replace hardcoded "oder" with conv_hero_divider message keyconv_hero_dividerParaglide key (de: "oder", en: "or", es: "o")py-12 sm:py-20for short viewports93be648— fix(ui): guard selectPerson against empty idif (!id) return;guardDeferred
All 38 tests green.
👨💻 Felix Brandt — Senior Fullstack Developer (re-review)
Verdict: ✅ Approved
Previous concerns — resolved
m.conv_hero_divider()Paraglide key. All three languages have the key.selectPersonnow hasif (!id) return;— clean guard clause.Code quality check
{#each}keyed:(person.id)— correct.$derivedfor computed values:isSinglePerson,senderName,receiverName— no$effectabuse.CorrespondenzEmptyStatefully removed, no leftover imports.CorrespondenzHero.svelteis 92 lines (30 script + 62 template) — reasonable for a hero with recent persons section. No split needed.Remaining note (non-blocking)
The
PersonTypeaheadclass ternary (large ? ... : compact ? ... : ...) is getting long. Asizeprop with a class lookup map would be cleaner. Not blocking — tracked for future cleanup.LGTM. Ship it.
🏗️ Markus Keller — Application Architect (re-review)
Verdict: ✅ Approved
No architectural concerns added or changed since the first review. The review fixes are cosmetic (i18n key, guard clause, layout reorder, responsive padding) — no state management, layer boundary, or coupling changes.
The two-state model (
!senderId→ hero,senderId→ results) remains clean. SSR behaviour unchanged. LGTM.🧪 Sara Holt — QA Engineer (re-review)
Verdict: ✅ Approved
Previous concerns — status
aria-disabledtest removal: correctly handled — filter controls are now hidden in hero state (not disabled), so the old test was semantically wrong. The new tests verify mutual exclusivity of hero/results states instead.Test coverage assessment
LGTM. Ready to merge.
🔒 Nora "NullX" Steiner — Security Engineer (re-review)
Verdict: ✅ Approved
No security-relevant changes in the review fix commits. The
conv_hero_dividermessage key is a static i18n string — no user input involved. Theif (!id) returnguard is a defensive check, not a security boundary. No new attack surface.LGTM. Same assessment as first review — no findings.
🎨 Leonie Voss — UX Design Lead (re-review)
Verdict: ✅ Approved
Previous concerns — all resolved
m.conv_hero_divider()— de: "oder", en: "or", es: "o". i18n blocker cleared.py-12 sm:py-20— typeahead stays above the fold on short viewports (landscape tablet, small laptops).Final design check
h-14(56px) — exceeds 44px touch target minimum. ✓text-2xlheadline,text-smcross-link,text-basebody. All meet dual-audience requirements. ✓h-12(48px) in results state via[&_input]:h-12. ✓Design implementation matches the agreed spec. Ready to ship.
🚀 Tobias Wendt — DevOps & Platform Engineer (re-review)
Verdict: ✅ Approved
No infrastructure changes in the review fix commits. Three message key additions and two Svelte component edits — zero CI/CD impact. Same assessment as first review.
LGTM.
👨💻 Felix Brandt — Senior Fullstack Developer (final review)
Verdict: ✅ Approved
Previous concerns — all resolved
m.conv_hero_divider()Paraglide keyselectPersonCode quality
$derivedforisSinglePerson,showHero,senderName,receiverName. No$effectabuse — the single$effectforpersistRecentPersonis a legitimate side-effect.showHerologic is clean:$derived(!senderId && !data.filters.senderId)— prevents hero flicker when sender is cleared locally. The guard inCorrespondenzPersonBar(if (id) onapplyFilters()) prevents navigation on clear. Two-layer defence, both necessary.{#each}keyed everywhere:(person.id),(doc.id),(c.id).CorrespondenzHero92 lines,CorrespondenzPersonBar183 lines (heavier due to sort/filter row + suggestions dropdown, but all one visual region),CorrespondenzFilterControls49 lines. All within bounds.CorrespondenzEmptyStatefully deleted, no orphan imports.Long-Arrow-*-MD.svg.showAdvancedtoggle withtransition:slide, chevron rotation, same button styling.One note (non-blocking)
The
CorrespondenzPersonBarnow handles person inputs, swap, sort, filter toggle, count, and correspondent suggestions. It's still one visual region (the search card's main row), but at 183 lines it's approaching the split threshold. If more controls are added later, consider extracting the sort/filter/count row into its own component.LGTM. Ship it.
🏗️ Markus Keller — Application Architect (final review)
Verdict: ✅ Approved
Architecture assessment
showHero(fresh page, no context) and results (senderId set or previously set). No cycles, no intermediate states.$derivedbooleans drive{#if}branches cleanly.showHero = !senderId && !data.filters.senderIdchecks both local state and server-loaded state. TheCorrespondenzPersonBaronly triggers navigation when a person is actually selected (if (id) onapplyFilters()). This prevents the page component from being replaced with empty server data when the user is just clearing an input to retype.showAdvancedtoggle +transition:slide+ chevron rotation mirrorsSearchFilterBarexactly. Good pattern reuse.SearchFilterBarnow usesDateInputcomponent instead of native<input type="date">, matching Briefwechsel.SortDropdownuses De Gruyter long arrow icons, matching Briefwechsel. Consistency improved across both pages.CorrespondenzHeroreceivesrecentPersonsfrom parent, doesn't fetch its own data.No concerns. LGTM.
🧪 Sara Holt — QA Engineer (final review)
Verdict: ✅ Approved
Test coverage
SortDropdown.svelte.spec.tstests updated to check forLong-Arrow-Up/Long-Arrow-Downimg src instead of↑/↓text.Deferred items (by agreement)
Edge cases covered
showHeroprevents flicker when clearing sender locallyLGTM.
🔒 Nora "NullX" Steiner — Security Engineer (final review)
Verdict: ✅ Approved
No new attack surface. The additional changes since the last review are purely visual (icon swaps, padding, collapsible filter, date input replacement). No new data flows, no new endpoints, no auth changes.
DateInputcomponent replacing<input type="date">on the document search page: the component usesoninputwith German format parsing — all client-side, no server-side injection vector.<img src="...">paths are all static strings pointing to files in/static/— no user-controlled paths.showHero/showAdvancedare local UI state — no security implications.LGTM. No findings.
🎨 Leonie Voss — UX Design Lead (final review)
Verdict: ✅ Approved
Design assessment — significant improvement since first review
Search card unification — the Briefwechsel search bar now matches the document search page:
rounded-sm border p-6 shadow-sm— identicalborder-line py-2.5label overrides — identicalDateInputcomponent withrounded-md border bg-surface px-3 py-2.5— identical to all other inputs. Placeholder showsTT.MM.JJJJformat hint instead of repeating labels.border bg-muted px-4 py-2.5 text-sm font-bold uppercase) — matches document searchIcon consistency:
Long-Arrow-Up/Down-MD.svgon both pagesLong-Arrow-Right/Leftath-3.5, compact and clearLong-Arrow-Right/Leftinstead of text→/←Layout:
py-12 sm:py-20py-8matching document search pageconv_hero_dividerkey, all text localizedAnti-flicker: clearing sender keeps the search bar visible — no disorienting jump back to hero. Excellent UX decision.
Receiver input: dashed border / canvas background removed — both inputs now look identical, with "Alle Korrespondenten" placeholder communicating optionality clearly.
No concerns. The two search pages now feel like siblings in the same design system. Ready to ship.
🚀 Tobias Wendt — DevOps & Platform Engineer (final review)
Verdict: ✅ Approved
No infrastructure impact. The additional changes are frontend-only: CSS adjustments, icon references (static files already in
/static/), component restructuring, andDateInputreplacing native date inputs. No new dependencies, no new env vars, no Docker/CI changes.The De Gruyter icon
<img>references point to files already in the repo underfrontend/static/degruyter-icons/— no external CDN or new asset pipeline needed.LGTM.