feat(briefwechsel): thumbnail rows with summary quote and bilateral distribution bar (#305) #311
Reference in New Issue
Block a user
Delete Branch "feat/issue-305-briefwechsel-thumbnail-rows"
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 #305.
Rebalances the
/briefwechselcorrespondence list into PDF-thumbnail rows and ships the backend metadata the new layout needs in a single PR.Phase 1 — frontend layout
DistributionBar.svelte($lib/components) — extracted from the inline markup inConversationTimeline, now reusable on any bilateral surface.ConversationThumbnail.svelte— portrait (120×168) / landscape (168×120) tile, page-count badge top-right, motion-safe pulsing skeleton while the async thumbnail job is still running.ThumbnailRow.svelte— thumbnail + title + italic summary quote (line-clamp-2) + meta line + up to three tag chips with+Noverflow. Colored left border encodes direction.aria-labelpairs title with formatted date.ConversationTimeline.sveltenow composes<DistributionBar>and<ThumbnailRow>— the old status dot, script-type icon, and arrow glyphs are gone.relativeYearsDehelper (calendar-field math, not ms/365.25) with full unit coverage.Phase 2 — backend metadata
thumbnail_aspect VARCHAR(16)andpage_count INTEGER, both nullable.ThumbnailAspectenum (PORTRAIT/LANDSCAPE) on theDocumententity.ThumbnailServicecomputes both values during the existing JPEG pipeline:w / h > 1.1 → LANDSCAPE, elsePORTRAIT(near-square A4 scans stay portrait).PDDocument.getNumberOfPages()for PDFs,1for image uploads.SourcePreviewrecord so the first-page image and page count travel through the pipeline together instead of reopening the PDF.width <= 0 || height <= 0→FAILEDbefore aspect/scale logic runs.Decision records
docs/adr/005-thumbnail-aspect-and-page-count.md— captures the 1.1 threshold, the choice to persist (not derive client-side), and the ordering invariants insideThumbnailService.generate().Deployment sequence
thumbnailAspect+pageCount, fall back to'PORTRAIT'/1for any document the backfill hasn't visited yet.POST /api/admin/generate-thumbnails— idempotent, regenerates thumbnails AND populates the two new columns for every pre-existing document. Safe to re-run.Frontend renders sensibly throughout the window because both new columns are nullable and the components default to portrait / single-page.
Test plan
./mvnw test— 100% green including 4 new migration tests, 2 new entity round-trip tests, 6 newThumbnailServiceTestcases (corrupt JPEG, corrupt PDF, PORTRAIT at 600×800 / 500×500 / 1099×1000, LANDSCAPE at 800×400, pageCount for 1 vs 3).DistributionBar(3 cases),ConversationThumbnail(7 cases),ThumbnailRow(10 cases including XSS regression),relativeYearsDe(5 cases). Updatedpage.svelte.spec.tscoversDistributionBarvisibility in bilateral vs single-person mode.briefwechsel-rows.visual.spec.ts) — scaffold lands with this PR; baselines to be captured viaplaywright test --update-snapshots briefwechsel-rowsagainst a seeded backend during review.briefwechsel-a11y.spec.ts) — axe-core at 3 viewports × 2 color schemes, runs in CI.🤖 Generated with Claude Code
The correspondence timeline labels each row with its distance from today ("vor 86 Jahren"). Uses calendar-field math so the anniversary day flips exactly — an ms-based 365.25d average misses by a day on leap years. Invalid / future dates return "" so the caller can hide the label rather than print "vor 0 Jahren". Refs #305 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Combines ConversationThumbnail with a quote-styled summary, truncated meta line, and up to three tag chips (the rest collapsed into "+N"). The colored left border tells a reader at a glance whether this letter left or entered the perspective person's mailbox — replacing the previous status dot + script-type icons that were too busy for the list view. Relative-year label ("vor 76 Jahren") is derived from documentDate so the list carries temporal context without a full date column. Rendering rules: - title falls back to originalFilename when empty - summary uses a text expression, never {@html}, so inline markup in the summary field is escaped (XSS regression test locks this) - focus-visible outline + focus-within hover keep keyboard-only users in sync with mouse hover feedback - aria-label always pairs title with the formatted date so screen readers hear both identifiers Refs #305 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
TDD evidence is strong end-to-end: every commit has a matching red test — V53 migration tests (4), Document entity round-trip (2), ThumbnailService aspect + pageCount + corrupt-input cases (6), DistributionBar (3), ConversationThumbnail (7), ThumbnailRow (10), relativeYearsDe (5). Svelte 5 rules followed (
$derived, no$effect, keyed{#each}). Guard clauses inThumbnailService.generate()keep the happy path flat. Nothing@Transactionalon a read. Lombok@Builder/@Dataper project style.Blockers
None.
Suggestions
ThumbnailRow.svelteis 110 lines — past the 60-line splitting signal. The tag chip row is a self-contained visual region:Not a blocker — the logic is straightforward and the file stays readable — but a future change to tag styling will touch a 110-line file instead of a 30-line one.
persistThumbnailMetadatasignature is at the boundary (ThumbnailService.java:159):Four params is where a param object starts earning its keep. You already have a
SourcePreviewrecord — consider widening it (or adding a peerThumbnailResult(String key, ThumbnailAspect aspect, int pageCount)) sogenerate()can pass a single value downstream.now?: DateinThumbnailRowis injected for tests but the callsite inConversationTimelinenever passes it, so production always hitsnew Date()inside a$derived— which re-runs on every reactivity cycle. In practice the timeline doesn't re-render frequently enough to matter, but a staticnow = new Date()captured in the parent would be cleaner and more testable in Storybook-style tools.All three are "nice to have" at the refactor stage. Clean work — merge-ready from the TDD-discipline side.
— Felix (@felixbrandt)
🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
Structural choices are conservative and well-documented. No new infrastructure, no new domain module, no boundary leaks beyond the one intentional exception already captured in ADR-004 (ThumbnailService injecting DocumentRepository directly). The new ADR-005 captures context / decision / alternatives / consequences in the house format — exactly what I want to see land alongside a schema change.
What I checked
V53__add_thumbnail_aspect_and_page_count.sql— correct naming, nullable columns, no default backfill. Safe to deploy ahead of the frontend, and idempotent re-run via the existing/api/admin/generate-thumbnailsendpoint.ThumbnailServicestill owns its exception-to-the-rule access toDocumentRepository. No new cross-domain leaks. Frontend components land in$lib/components/(shared) vsroutes/briefwechsel/(route-local) on the correct side of that line —DistributionBaris reusable,ConversationTimelinestays route-scoped.ThumbnailService.SourcePreviewrecord). The frontend consumes the result rather than recomputing — correct direction.Suggestions (not blockers)
When Document grows a 5th thumbnail-related column, extract. Today the entity has
thumbnailKey,thumbnailGeneratedAt,thumbnailAspect,pageCount— four. The 5th (OCR text hash? DPI override? content-addressed hash?) is the signal to extract a@Embeddable ThumbnailMeta. Not now — two-field additions don't justify it and ADR-004's "thumbnails are a cross-cutting aspect of Document" still holds. Flag it for yourself when the next request lands.LANDSCAPE_THRESHOLD = 1.1fis an application constant, not a configuration value. ADR-005 notes this is intentional. Fine — single-operator archive, no tuning-per-tenant pressure. If that ever changes, this is the first place to revisit.The frontend Document type shape is now 25 fields after the two additions. That's fine for now but approaching the size where a per-page DTO (e.g.
DocumentTimelineCard) would reduce wire payload for the briefwechsel list view. Not urgent — the endpoint already returns the full entity for other reasons — but worth noting as the feature surface grows.No layering violations, no unjustified complexity, no hype adoption. Merge.
— Markus (@mkeller)
🛡️ Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
The PR widens the data surface (two new columns, a new frontend row layout rendering summary + tags) but does not add new authentication paths, controller endpoints, query strings, or file-handling primitives. Threat model stays within existing boundaries.
What I audited
XSS on the summary field —
ThumbnailRow.svelte:79uses a single text expression{doc.summary}inside an<div>. Svelte auto-escapes. No{@html}anywhere in the PR. A regression test (ThumbnailRow.svelte.spec.ts:144-159) asserts that a payload like<img src=x onerror="alert(1)">stays as literal text and does NOT become a live DOM element — that's the permanent lock I want to see. ✅aria-label string construction —
DistributionBar.svelte:20interpolatessenderNameandreceiverNameinto the aria-label. These come fromPerson.displayNamewhich is built server-side fromPerson.firstName+Person.lastName— already sanitized at the domain boundary. Bound via Svelte expression so they're escaped. Not exploitable. ✅No new user-controlled server input —
thumbnailAspectis derived fromBufferedImage.getWidth() / getHeight(),pageCountfromPDDocument.getNumberOfPages(). Both are server-computed from the PDF bytes we already loaded for rendering. Never trusts a request param. No IDOR / mass-assignment risk. ✅PDFBox parser attack surface — unchanged. The existing
Loader.loadPDF(...)call is the only entry and it's already behind theThumbnailAsyncRunnerwatchdog per ADR-004.No new JPQL, no string concatenation, no new actuator exposure, no new CORS.
Corrupt-PDF / corrupt-JPEG regression tests (
ThumbnailServiceTest.java:142-170) lock in the FAILED-outcome behavior. If someone "simplifies" the catch block later, tests break first. ✅Non-blocking observations
private, max-age=31536000, immutable(pre-existing, not touched by this PR). Stricter than ADR-005 mentions but correct given cache-busting via?v=<timestamp>. No change needed.line-clamp-2hides it but the full value travels over the wire and into the DOM. Pre-existing — not introduced by this PR. Worth a V54CHECK (length(summary) <= N)in a future migration; flag for your backlog, not a merge blocker.Clean pass from me.
— Nora (@NullX)
🧪 Sara Holt — QA & Test Strategist
Verdict: 🚫 Changes requested
The unit layer is excellent — I counted 37 new assertions across 6 files (migration, entity, service, three components, helper). Factory fixtures, AAA structure, descriptive names, one-logical-assertion-per-test, Testcontainers-backed integration.
@Transactionalis used correctly.MockitoExtensionkeeps the unit tests under a second. All of that is merge-ready.The E2E layer is where I'm blocking.
Blockers
briefwechsel-a11y.spec.tsnever exercises the new row layout. It navigates to/briefwechselwith no query params, which renders the hero state (conv-hero) — not a bilateral pair. axe-core runs against the hero, not ThumbnailRow, ConversationThumbnail, or DistributionBar. The sweep is 6 tests × 2 themes × 3 viewports = 36 runs that all check the wrong thing.Fix: seed two persons with correspondence in CI (or use existing seed data), then drive the URL with
?senderId=X&receiverId=Ybefore running axe. Example:briefwechsel-rows.visual.spec.tshas the same problem — the structural test just assertsconv-herois visible. That's a 5-line smoke test for the hero, not the row layout. And the snapshot block is gated onVISUAL=1so CI never runs it.Fix: same seeding approach. Additionally, I'd rather see the snapshots run by default with a pixel tolerance (
maxDiffPixels: 100is fine) and fail if baselines don't exist, so the missing-baseline state is visible as a red CI run and not silent.Concerns (not blocking)
page.svelte.spec.tsDistributionBar tests render the page but never assert thatThumbnailRowis rendered for each document. Oneexpect(getByTestId('conv-thumb-tile')).toBeInTheDocument()per bilateral-with-docs fixture would catch a broken{#each}wiring.Test fixture pollution:
hansPerson/annaPersonat module scope inpage.svelte.spec.tsis fine but they're also reused in the new DistributionBardescribe. If one test mutates them (none do today), it cascades. Convention here is usuallymakePerson({ displayName: 'Hans Müller' })— matchesmakeDoc. Nitpick.No test for the relative-year label hiding on future/invalid dates at the ThumbnailRow layer. The
relativeYearsDehelper has its own coverage, but the integration ("if the helper returns '' we don't render the label element") isn't asserted. One additionaldoc: { documentDate: '2030-01-01' }case would cover it.The unit layer is solid. The E2E layer needs to actually touch the feature before I can sign off.
— Sara (@saraholt)
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
This PR is infrastructure-quiet by design — no changes to
docker-compose.yml,.github/workflows,Caddyfile, or the devcontainer. That's the right shape for a feature that only adds a schema column and a frontend refactor.What I checked
V53__add_thumbnail_aspect_and_page_count.sql— correct format, correct sequence. NoV52.1or gaps. ✅ADD COLUMNs, noNOT NULLdefault, no constraint churn. Non-blocking on a table with 1000s of rows. Rolls forward in ~milliseconds. Zero-downtime compatible with the existing backend running the pre-V53 schema until restart. ✅POST /api/admin/generate-thumbnailsoverwrites the same S3 keys (deterministicthumbnails/{docId}.jpg) and now populates the two new columns. Safe to rerun if a deploy is interrupted.PDDocument, so no additional PDFBox cost beyond what ADR-004 already covers.Follow-ups (not blockers)
Visual baseline capture is a manual step (
VISUAL=1 playwright test --update-snapshots briefwechsel-rows). File that as a one-shot Gitea issue after this merges so the baselines are captured against the deployed frontend and committed. Otherwise the spec is dead code — either CI ignores it or it stays local-only forever.The a11y spec lands unconditionally in CI. If Sara's seeding concern (see her review) isn't addressed, expect ~1min of CI time spent axe-scanning the hero. Not broken, just wasted cycles — worth fixing before merge to keep the pipeline lean.
No new
actions/dependencies. Good — keeps the supply-chain surface unchanged. If the visual-regression pipeline later needs a baseline artifact upload,actions/upload-artifact@v4is the right version; the older@v3is deprecated.Ship it — deployment plan is clean and the blast radius is a single ALTER TABLE that every Postgres can handle during a rolling restart.
— Tobias (@tobiwendt)
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: 🚫 Changes requested
The structural intent is right — thumbnails as the primary visual anchor, quote-styled summary, direction encoded on the left border, page badge for multi-page letters. Tailwind tokens (
text-ink,bg-muted,border-line,text-primary,text-accent) are used correctly. Focus ring (focus-visible:outline-2 focus-visible:outline-offset-[-2px] focus-visible:outline-primary) is properly gated withfocus-visibleso mouse users don't see it on click. Touch target at 120px min-height is well above the 44px WCAG 2.2 floor.But there are three issues I need addressed before this lands in front of users.
Blockers
Direction is encoded by color alone — WCAG 1.4.1 failure
The previous layout had
<img>arrow icons (Long-Arrow-Right-MD.svg/Long-Arrow-Left-MD.svg) inside each row indicating send-vs-receive. This PR removes them and leaves onlyborder-l-primaryvsborder-l-accent. A color-blind user (deuteranopia: 8% of men) cannot distinguish brand-navy from brand-mint reliably at a 3px border width. The aria-label only says"{title}, {date}"— no directional indicator for screen readers either.Fix — either restore the arrow inside
ConversationThumbnailtop-left corner (keeps the tile visual, doesn't bloat the row), or add a visually-hidden direction text toThumbnailRowaria-label:Keep the colored border (it's a great redundant cue), but don't rely on it alone.
DistributionBar.svelte:20hardcodes German for EN/ES usersAnd the visible text (
{outCount} von {shortSenderName}) has "von" baked in. The project uses Paraglide and supportsde/en/es. An English-speaking user gets German prose in their screen reader and visible UI.Fix: add Paraglide keys, e.g.
and consume them via
m.dist_bar_from({ count: outCount, name: shortSenderName }).ConversationThumbnail.svelte:23hardcodesbg-whiteIn dark mode the tile becomes a bright-white rectangle with a dark paper scan on it, held together only by
dark:mix-blend-multiply. The mix-blend works for light-colored scans but produces muddy results on high-contrast handwriting. The existingDocumentThumbnail.sveltehas the identical pattern, so this is the team's convention — but the right fix isbg-surface(dark-mode-aware) with the blend mode reserved for cases where the scan's own background clashes.This is actually a pre-existing smell inherited from
DocumentThumbnail, so if you'd rather address it in a dedicated follow-up PR (affecting both components) I'll accept that. But please file the issue before merge.Concerns
Page badge text size is text-xs (12px) on a 168×120 tile. Meets the 12px floor but marginal for seniors on a 320px phone where the tile may be zoomed out. Bumping to
text-sm(14px) and increasing padding frompx-1.5 py-0.5topx-2 py-1gives senior users a readable affordance without crowding the thumbnail:Summary uses italic serif with
line-clamp-2. Merriweather italic attext-sm(14px) renders a bit thin at low DPI — legible but not strong. Considertext-[15px]or dropping italic in favor of a leading“on the left margin. Taste call, not a blocker.data-aspectattribute is in English CAPS (data-aspect="PORTRAIT"). That's fine for a testing hook but the attribute leaks the backend enum into markup. Minor — purely cosmetic, happy to live with it ifdata-testidelsewhere follows the same convention.What I loved
“/”entities) reads like correspondence, not metadata. Strong editorial choice.motion-safe:animate-pulseon the skeleton — respectsprefers-reduced-motioncorrectly.Fix 1, 2, and file 3. Resubmit and I approve.
— Leonie (@leonievoss)
Drops the hardcoded German strings ("Briefverteilung in diesem Zeitraum", "{n} von {name}") and routes every visible + assistive-tech string through dist_bar_aria and dist_bar_segment message keys. An English or Spanish user now sees "from" / "de" instead of "von" both on screen and in the aria-label their screen reader announces. Refs #305 Fixes @leonievoss i18n concern from PR review Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Consolidates the hansPerson / annaPerson fixture into a makePerson() factory matching the makeDoc convention, adds an assertion that the bilateral list renders one ConversationThumbnail tile per document (catches a broken {#each} keying wired around the DistributionBar), and decouples the DistributionBar aria-label assertion from the German locale now that i18n lands via Paraglide. Refs #305 Fixes @saraholt concerns 3 + 4 from PR review Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🔧 Review fixes — round 1
Pushed 10 commits addressing every blocker + concern from the first review cycle.
@leonievoss blockers
2d7fac6b) —ThumbnailRowaria-label now leads withGesendet:/Empfangen:so screen-reader users hear direction before the title. Colored border stays as a redundant cue.858c9f05) — both the visible segment text and aria-label route through newdist_bar_segment+dist_bar_ariaParaglide keys in de / en / es.bg-whitedark-mode — filed as #312 (ui, refactor labels) since it pre-existed this PR and affectsDocumentThumbnailtoo; scope kept focused on #305.@leonievoss concern
0a3e5d62) — bumpedtext-xs→text-smwithpx-2 py-1for senior users on small phones.@saraholt blockers
d1a69e0b) —beforeAllcreates sender + receiver + one document via/api/persons+/api/documents; each axe run now drives?senderId=X&receiverId=Yand asserts theconv-person-baris visible so a hero-state regression fails loudly.a92644d6) — same seeding pattern, structural test assertsconv-thumb-tile+DistributionBarrender. Snapshot block stays gated onVISUAL=1for the baseline-capture step.@saraholt concerns
3b83141d) —page.svelte.spec.tsnow assertsconv-thumb-tilecount matches the document count.makePersonfactory (3b83141d, same commit) — replaces module-scopehansPerson/annaPersonwith a single factory matching themakeDocconvention.54ef1643) — regression test fordocumentDatein the future → novor N Jahrenlabel.@felixbrandt suggestions
3b3b551d) —ThumbnailRowdrops from 110 to ~90 lines; new component has its own 3-case spec.78caac8d) —ThumbnailService.persistThumbnailMetadata(Document, ThumbnailResult)replaces the 4-arg signature, mirroring the existingSourcePreviewrecord.nowdefault at prop binding (514f0174) —ThumbnailRownow usesnow = new Date()in$props()destructure instead of inside$derived.Test status
./mvnw test -Dtest=ThumbnailServiceTest→ 16/16 green.ChronikErrorCardcase unrelated to this PR, verified by running the same test against the untouched baseline).makePerson+ per-row tile (+1), TagChipList (+3).Deferred (not blockers, by author consensus)
@Embeddable ThumbnailMeta, Nora summary-length CHECK, Tobias baseline-capture follow-up — all post-merge items.Ready for re-review.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Round-2 check — every suggestion I raised in round 1 is resolved, each with its own atomic commit and a matching test pass or refactor-under-green.
What I verified
TagChipList extraction (
3b3b551d) — pulled cleanly into$lib/components/TagChipList.svelte. 22-line component, 3 spec cases (under-cap / at-cap / empty), samedata-testid="thumb-row-tag"so the existing ThumbnailRow tag-cap assertion keeps working without change.ThumbnailRowdropped thedisplayedTags/hiddenTagCountderivations — clean delete.ThumbnailResult record (
78caac8d) —persistThumbnailMetadata(Document, ThumbnailResult)is two args, mirrors theSourcePreviewpattern one step up the pipeline, and the orphan-thumbnail log message correctly readsresult.key(). The private record lives next toSourcePreviewwith a clarifying comment about what it carries.nowcaptured at prop binding (514f0174) — default in$props()destructure, the$derivedforrelativeYearLabelno longer has a?? new Date()fallback. Stable across reactivity ticks now.Also noticed (not blocking, props)
Fixes @leonievoss concern Xpointer is a nice audit trail.ChronikErrorCardfailure verified against baseline.Clean pass. No new concerns.
— Felix (@felixbrandt)
🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
No structural changes in round 2 — the fixes stay inside existing boundaries. ADR-005 remains the single source of truth for the thumbnail aspect/pageCount decision. Follow-up issue #312 for the
bg-whitedark-mode pattern was filed rather than widening this PR's scope — correct call.Re-verified
TagChipList.sveltelands in$lib/components/(shared) not inroutes/briefwechsel/— right side of the reusability line. Next consumer will likely be the document detail header.ThumbnailResultis a private nested record — correctly not promoted to a public domain type. That's right: this is internal plumbing forThumbnailService.generate(), not something the rest of the app should see.Merge when Leonie and Sara sign off.
— Markus (@mkeller)
🛡️ Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Re-audited the delta. No security surface added, no existing control weakened.
What changed and what I re-checked
Paraglide message interpolation (
858c9f05) —m.dist_bar_aria(...)andm.dist_bar_segment(...)receivesenderName/receiverName/ counts. Paraglide auto-escapes variables into its output via ICU message format; the Sveltearia-label={ariaLabel}binding auto-escapes the resulting string. No DOM or attribute injection vector. The three locale JSON files (de.json,en.json,es.json) have the same placeholder shape — no missing-substitution gap.ThumbnailResult record (
78caac8d) — purely internal refactor. The aspect + pageCount source values still come fromBufferedImage.getWidth/getHeightandPDDocument.getNumberOfPages, never from the request. No change to the trust boundary.E2E seeding via API (
d1a69e0b,a92644d6) —request.post('/api/persons', ...)runs with the authenticated admin session fromauth.setup.ts. This is test infrastructure, not production code, and it stays behind the existing admin storage state — no credentials leaked into the spec file.TagChipList (
3b3b551d) — tag names render as text expressions (Svelte auto-escapes). No{@html}. Existing XSS regression test onThumbnailRowstill passes against the new composition.No new JPQL, no new endpoint, no new auth surface. All good.
— Nora (@NullX)
🧪 Sara Holt — QA & Test Strategist
Verdict: ⚠️ Approved with concerns
Both blockers addressed, both concerns addressed. What's left is a handful of test-hygiene follow-ups — they're suggestions, not blockers, so this is a sign-off from me.
Blockers — resolved
briefwechsel-a11y.spec.tsseeds + targets the row layout (d1a69e0b) —beforeAllcreates sender + receiver + doc via API, each test drives?senderId=X&receiverId=Y, andexpect(page.getByTestId('conv-person-bar')).toBeVisible()guards that we actually reached the row state before axe runs. That's the exact shape I wanted. ✅briefwechsel-rows.visual.spec.tsgets the same seeding + a real structural assertion (a92644d6) —conv-thumb-tilevisibility + non-emptyaria-labelon[role="img"]both checked. The snapshot block staying gated onVISUAL=1is the right trade-off until baselines are captured against a seeded backend. ✅Concerns — resolved
3b83141d) —expect(tiles).toHaveLength(3)against a 3-doc fixture. ✅makePersonfactory (3b83141d) — matches themakeDocconvention. ✅54ef1643) — regression test locked. ✅New round-2 follow-ups (non-blocking)
These are things I noticed while re-reviewing — don't hold the merge on these, but file or address as a follow-up:
Test isolation in the E2E seeding —
beforeAllleaves the seeded persons + document in the DB after the suite completes. The test runner is sequential and scopes are unique viaDate.now(), so this isn't a conflict risk today. But the test DB will accrueA11y Sender-<ts>/Visual Sender-<ts>rows forever. A per-specafterAllthatDELETEs the seeded IDs would be tidier. Alternative: a single shared fixture module with a cleanup helper.Both specs duplicate the seeding logic (a11y + visual). Each
beforeAlldoes the same 3 API calls with slightly different names. Extract toe2e/fixtures/bilateral-correspondence.tsexporting aseedBilateralPair()helper. Nothing is broken; it's drift prevention.conv-person-barvisibility gate — good safety net, but it's only present in the a11y spec. Adding it to the visual spec'sopenBilateral()helper would make both specs fail identically on a hero-state regression.Snapshot baselines still need capturing. This is the Tobias follow-up. Once baselines are committed, flip the default of the
VISUALenv flag totrueso CI runs the sweep without an opt-in. That's a one-line change when the baselines land.What I loved
Date.now()suffix on the seeded names — deterministic enough for a single run, unique across runs, no schema change needed. Nice touch.All blockers clear. Happy to see this merge.
— Sara (@saraholt)
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Round 2 stays infrastructure-quiet: no Compose, no CI workflow, no env var change. The two E2E specs now hit the real backend via
/api/persons+/api/documents— same auth pattern asnotification-deep-link.spec.ts, same admin storage state. CI already has the backend + MinIO running for E2E, so no pipeline work to do on my side.What I checked
@axe-core/playwrightand@playwright/testalready in package.json.workers: 1.frontend/messages/{de,en,es}.jsonand the generatedsrc/lib/paraglide/is regenerated per-build. No generated code committed. ✅bg-whitefollow-up withui+refactorlabels — correct pattern for non-blocking tech debt.Single follow-up (not blocking merge)
Ship it.
— Tobias (@tobiwendt)
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
All three of my round-1 blockers are resolved in a way I'm comfortable shipping.
Blockers — resolved
WCAG 1.4.1 direction indicator (
2d7fac6b) —ThumbnailRow.svelteline 53:Screen-reader users hear "Gesendet: Liebe Anna, 1. Juni 1950" — direction first, then title, then date. The colored border stays as a visual redundant cue. Clean. ✅
Minor follow-up for later (not a blocker): "Gesendet" / "Empfangen" are German-only. When we i18n the aria-label text itself, route them through Paraglide like the DistributionBar segments. I'd file it as a small cleanup ticket, but given 95% of users are on German I'm happy to merge today.
DistributionBar i18n (
858c9f05) — three new keys (dist_bar_segment,dist_bar_aria) inde.json/en.json/es.json. The Spanish translation "de" works correctly for both "from X" ("de Juan") and numerical "3 de Hans" contexts since Spanish doesn't have the German article tangle. English "3 from Hans, 7 from Anna" reads naturally. The ICU placeholder{count}+{name}pattern matches howcomment_time_minuteswas structured — consistent with the project's existing i18n style. ✅bg-whitedark-mode — filed as #312 withui+refactorlabels and a clear acceptance-criteria checklist. That's the right scope split — touchingDocumentThumbnail.sveltein this PR would have widened the diff unnecessarily. ✅Concern — resolved
0a3e5d62) —text-sm+px-2 py-1. Verified against the tile dimensions: 14px font fits comfortably in the 120×168 portrait tile with breathing room, and the newleading-none(already there) keeps the vertical rhythm tight. Senior users on a 320px phone zoom will see this clearly. ✅Round-2 tiny things (not blockers — purely preference)
The new page-count badge (
px-2 py-1 text-sm) on abg-primary/90pill is a strong visual anchor. On a very busy scan (handwriting with dark ink), the badge could optionally add a subtle white ring (ring-1 ring-surface/50) to float above any ink underneath it. I'll leave that to a visual review of real thumbnails.TagChipList.sveltereuses the exact styling from the inline version — no regression. If it ever gets used on a different surface (document detail header, Chronik feed), consider makingmaxdefault to3so the prop can be omitted for the common case.What I loved
Approved. Merge when Sara's test concerns are either addressed or queued.
— Leonie (@leonievoss)
Makes `max` an optional prop with default 3 — the common row-layout case doesn't need to name the cap explicitly. ThumbnailRow's callsite drops to `<TagChipList tags={doc.tags ?? []} />`, consistent with how other shared components in $lib/components expose sensible defaults. Refs #305 Fixes @leonievoss round-2 follow-up from PR review Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🔧 Review fixes — round 2
Pushed 4 commits addressing every non-blocking follow-up from the round-2 reviews.
@saraholt follow-ups
seedBilateralPairfixture (00fa7674) — extracted intoe2e/fixtures/bilateral-correspondence.tswith matchingcleanupBilateralPair. Both specs now call one function instead of duplicating three API calls.afterAllcleanup (00fa7674,b7083d42) — both specs purge the seeded document on teardown. Persons remain (backend has no person-delete endpoint) but theDate.now()suffix guarantees no collisions between runs.conv-person-bargate in visual spec (b7083d42) — folded intoopenBilateral()so every test path (structural + snapshot block) fails loudly on a hero-state regression, matching the a11y spec's safety net.@leonievoss follow-ups
0da34d06) —row_direction_sent/row_direction_receivedkeys in de (Gesendet/Empfangen), en (Sent/Received), es (Enviada/Recibida). Screen-reader users on EN/ES now hear their language.TagChipListmaxdefaults to 3 (fba685e7) — common case drops to<TagChipList tags={doc.tags ?? []} />; ThumbnailRow simplified accordingly.@tobiwendt follow-up
test+devopslabels. Step-by-step reproduction included, one-line flip documented so the followup PR is a low-friction change.Deferred (for a reason)
VISUALdefault) — depends on #313 landing first; flagged explicitly in that issue.Test status
ChronikErrorCardfailure is still the only non-green test, confirmed unrelated.Commits this round
00fa7674refactor(e2e)b7083d42refactor(e2e)0da34d06i18n(briefwechsel)fba685e7refactor(briefwechsel)Ready for re-review / merge.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Round-3 check — nothing on my side left to raise.
What I verified
TagChipList'smaxdefaulting to 3 (fba685e7) — clean Svelte 5 destructure with optional prop typing.ThumbnailRowcallsite dropped the explicitmax={3}which is the signal that the default is doing its job.e2e/fixtures/bilateral-correspondence.ts— shape matches the existinge2e/fixtures/conventions (only PDF fixtures lived there before; typed helpers fit fine alongside).0da34d06) is the consistent next step after the earlier DistributionBar i18n — same pattern, same test-style update viam.row_direction_sent().All 4 round-2 commits are atomic, TDD-driven where a test existed, and clean refactors-under-green where a test already covered the behavior. Shipit.
— Felix (@felixbrandt)
🏛️ Markus Keller — Application Architect
Verdict: ✅ Approved
Round 3 was pure hygiene — no structural changes. The new fixture module (
e2e/fixtures/bilateral-correspondence.ts) is the right shape: a focused helper that doesn't grow into a "test framework", no cross-test global state, typed return value (BilateralPair). That's a fixture, not a framework.Issue #313 for baseline capture is well-scoped: concrete steps, acceptance criteria, one-line flip documented. That's how follow-up work should be filed — the next person picking it up doesn't need to reconstruct context.
No new ADRs warranted for round-2/3 changes — these are all internal polish within boundaries ADR-004 and ADR-005 already capture.
— Markus (@mkeller)
🛡️ Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
No security surface added in round 3.
What I re-audited
e2e/fixtures/bilateral-correspondence.ts) — test-only code, runs behind the authenticated admin storage state.request.delete('/api/documents/...')uses the same admin context as the POSTs; no privilege escalation path.0da34d06) — static string returns (no variable interpolation), auto-escaped by Svelte onaria-label={ariaLabel}. Nothing to attack.Clean pass.
— Nora (@NullX)
🧪 Sara Holt — QA & Test Strategist
Verdict: ✅ Approved
All four of my round-2 follow-ups resolved in the way I asked for.
Verified
seedBilateralPairhelper (00fa7674) — typed return (BilateralPair), clear prefix param to disambiguate the two specs ("A11y" vs "Visual"), docstring documents the person-delete caveat up-front. Exactly the shape I wanted.afterAllcleanup in both specs (00fa7674,b7083d42) — purges the document via the existing delete endpoint. The orphaned-persons caveat is called out honestly;Date.now()suffix keeps them unique so they don't accumulate into collisions. Acceptable trade-off for a test DB.conv-person-bargate in the visual spec (b7083d42) — folded intoopenBilateral()so every path uses it. Safety net at the right layer.VISUALdefault flip all documented. Whoever picks this up has zero friction.What I loved
e2e/fixtures/next to the existingminimal.pdf/minimal2.pdf— same directory, consistent convention.fba685e7(TagChipListmax=3default) got a dedicated test case for the default, not just a refactor commit. That's the TDD discipline showing.Clean sign-off from me.
— Sara (@saraholt)
🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Round 3 is still infrastructure-quiet. Issue #313 filed with the right labels (
test,devops), scoped to one clear deliverable, one-line flip documented. That's the right artifact to hand off.No change to Compose, CI workflow, or secrets. No new dependencies. The seeding goes through the existing authenticated test context — same auth pattern, zero CI pipeline changes needed.
Ship it.
— Tobias (@tobiwendt)
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
Both round-2 follow-ups I flagged are landed.
Direction label i18n (
0da34d06)Three locales, idiomatic translations:
The
$derivedwiring usesm.row_direction_sent()/m.row_direction_received()— consistent with the DistributionBar i18n pattern from round 1. A Spanish-speaking user hearing "Enviada: Querida Ana, 1 de junio de 1950" now gets a locally-correct announcement. That's the bar.TagChipList default (
fba685e7)max = 3default with optional prop type. Callsite simplified. Future consumers (document detail header, Chronik) get the sensible default for free — matches thesize = 'sm'default inDocumentThumbnail. Consistent.Nothing left on my side
No more UX/a11y follow-ups pending on this PR. The white-ring-on-badge nit is awaiting the baseline PNGs from #313 — correct deferral, I explicitly said that.
Merge.
— Leonie (@leonievoss)