feature(briefwechsel): thumbnail rows with summary quote and bilateral distribution bar #305

New Issue

marcel · 2026-04-22T20:18:51+02:00

marcel commented

2026-04-22 20:18:51 +02:00

Summary

Redesign the /briefwechsel row so each entry feels like a letter worth opening, not a database record. Adds a PDF first-page thumbnail on the left (portrait for letters, landscape for postcards), renders the summary as a quote next to it, and restores the bilateral distribution bar above the list when both sender and receiver are selected. Removes the status-lifecycle chip and script-type indicator — both are unreliable / slated for removal.

Spec

Final HTML spec (scaled wireframes × 3 viewports + impl-ref tables with exact Tailwind classes and pixel values + WCAG contrast verification):

docs/specs/briefwechsel-thumbnail-rows-spec.html

Exploration context (how we got here — 5 concepts):

docs/specs/briefwechsel-fill/index.html

What changes visually

Thumbnail cell 104×120 px at desktop · portrait 82×106, landscape 104×72
Summary rendered serif italic with mint-coloured quote marks; omitted entirely when empty (no placeholder)
Meta line keeps direction arrow + counterpart + location + up to 2 tags
Right column is only the date (serif bold) + "vor N Jahren". Archive box removed.
Row height 128 px min — comfortable, not dense (this page is fun discovery, not scanning)
Bilateral view adds a distribution bar above the list (pattern lifted from production ConversationTimeline)
Multi-page letters get a "4 S." badge on the thumbnail
Postcards get a stamp + postmark corner via the thumbnail service

Removed from earlier drafts

Status dot / "Hochgeladen / Transkribiert / Geprüft / Archiviert" label — lifecycle will be dropped from the product
Handwriting / typewriter indicator — only reliably set after OCR, not DB-backed

Breakdown

Phase 1 — UI without thumbnails

New ThumbnailRow.svelte component (replaces row markup in ConversationTimeline.svelte)
New DistributionBar.svelte component (lift out of ConversationTimeline.svelte — also needed by the person-dashboard issue)
New YearDivider.svelte wrapper (or keep inline — see spec)
Summary renders with quote pseudo-elements when present; omit element when empty
Right column: date (de-DE long format) + relative "vor N Jahren"
Remove status dot + status label + script-type meta from row output
Paraglide keys for new labels (conv_direction_out, conv_direction_in, doc_kind_postcard, doc_pages_count, rel_years_ago)
Thumbnail cell renders the paper-coloured skeleton (placeholder for Phase 2)
axe-playwright tests at 320 / 768 / 1440 in light + dark
Visual regression snapshots at the 3 viewports
prefers-reduced-motion disables the hover lift

Phase 2 — wire real thumbnails (depends on thumbnail-generation issue)

Extend Document with thumbnailUrl, thumbnailAspect (PORTRAIT | LANDSCAPE), pageCount
/api/documents/conversation returns the new fields
DocumentThumbnail.svelte with loading="lazy", intersection-observer, skeleton → real image swap
Multi-page badge renders when pageCount > 1
Fallback placeholder (paper gradient + document icon) when thumbnail missing

Phase 3 — observe + tune

Measure performance on 851-letter lists
Review a11y with a real screen reader (VoiceOver / NVDA)

Accessibility contract

All contrast ratios verified in the spec — light and dark mode both pass WCAG AA, most pass AAA. Row is a native <a href>, keyboard-reachable, focus-visible ring, distribution bar has role="img" + descriptive aria-label, direction uses arrow shape + colour (redundant cue), no text below 12 px, 128 px row >> 44 px WCAG minimum.

Depends on the existing "PDF thumbnail generation" backend issue for Phase 2
Shares the DistributionBar.svelte component with #PERSON-DASHBOARD (linked below once opened)

## Summary Redesign the `/briefwechsel` row so each entry feels like a letter worth opening, not a database record. Adds a PDF first-page thumbnail on the left (portrait for letters, landscape for postcards), renders the summary as a quote next to it, and restores the bilateral distribution bar above the list when both sender and receiver are selected. Removes the status-lifecycle chip and script-type indicator — both are unreliable / slated for removal. ## Spec Final HTML spec (scaled wireframes × 3 viewports + impl-ref tables with exact Tailwind classes and pixel values + WCAG contrast verification): - [`docs/specs/briefwechsel-thumbnail-rows-spec.html`](../raw/branch/main/docs/specs/briefwechsel-thumbnail-rows-spec.html) Exploration context (how we got here — 5 concepts): - [`docs/specs/briefwechsel-fill/index.html`](../raw/branch/main/docs/specs/briefwechsel-fill/index.html) ## What changes visually - Thumbnail cell 104×120 px at desktop · portrait 82×106, landscape 104×72 - Summary rendered serif italic with mint-coloured quote marks; omitted entirely when empty (no placeholder) - Meta line keeps direction arrow + counterpart + location + up to 2 tags - Right column is only the date (serif bold) + "vor N Jahren". Archive box removed. - Row height 128 px min — comfortable, not dense (this page is fun discovery, not scanning) - Bilateral view adds a distribution bar above the list (pattern lifted from production `ConversationTimeline`) - Multi-page letters get a "4 S." badge on the thumbnail - Postcards get a stamp + postmark corner via the thumbnail service ## Removed from earlier drafts - Status dot / "Hochgeladen / Transkribiert / Geprüft / Archiviert" label — lifecycle will be dropped from the product - Handwriting / typewriter indicator — only reliably set after OCR, not DB-backed ## Breakdown ### Phase 1 — UI without thumbnails - [ ] New `ThumbnailRow.svelte` component (replaces row markup in `ConversationTimeline.svelte`) - [ ] New `DistributionBar.svelte` component (lift out of `ConversationTimeline.svelte` — also needed by the person-dashboard issue) - [ ] New `YearDivider.svelte` wrapper (or keep inline — see spec) - [ ] Summary renders with quote pseudo-elements when present; omit element when empty - [ ] Right column: date (de-DE long format) + relative "vor N Jahren" - [ ] Remove status dot + status label + script-type meta from row output - [ ] Paraglide keys for new labels (`conv_direction_out`, `conv_direction_in`, `doc_kind_postcard`, `doc_pages_count`, `rel_years_ago`) - [ ] Thumbnail cell renders the paper-coloured skeleton (placeholder for Phase 2) - [ ] axe-playwright tests at 320 / 768 / 1440 in light + dark - [ ] Visual regression snapshots at the 3 viewports - [ ] `prefers-reduced-motion` disables the hover lift ### Phase 2 — wire real thumbnails (depends on thumbnail-generation issue) - [ ] Extend `Document` with `thumbnailUrl`, `thumbnailAspect` (`PORTRAIT` | `LANDSCAPE`), `pageCount` - [ ] `/api/documents/conversation` returns the new fields - [ ] `DocumentThumbnail.svelte` with `loading="lazy"`, intersection-observer, skeleton → real image swap - [ ] Multi-page badge renders when `pageCount > 1` - [ ] Fallback placeholder (paper gradient + document icon) when thumbnail missing ### Phase 3 — observe + tune - [ ] Measure performance on 851-letter lists - [ ] Review a11y with a real screen reader (VoiceOver / NVDA) ## Accessibility contract All contrast ratios verified in the spec — light and dark mode both pass WCAG AA, most pass AAA. Row is a native `<a href>`, keyboard-reachable, focus-visible ring, distribution bar has `role="img"` + descriptive `aria-label`, direction uses arrow shape + colour (redundant cue), no text below 12 px, 128 px row >> 44 px WCAG minimum. ## Related - Depends on the existing "PDF thumbnail generation" backend issue for Phase 2 - Shares the `DistributionBar.svelte` component with #PERSON-DASHBOARD (linked below once opened)

marcel referenced this issue

2026-04-22 20:19:27 +02:00

feature(persons): Korrespondenz-Überblick dashboard on /persons/[id] #306

marcel added the conversation feature ui labels 2026-04-22 20:19:35 +02:00

marcel commented

2026-04-23 11:18:30 +02:00

🏛️ Markus Keller — Senior Application Architect

Observations

The "depends on the existing PDF thumbnail generation backend issue" note is stale. ThumbnailService, ThumbnailBackfillService, ThumbnailAsyncRunner, AdminController#generateThumbnails, Document.thumbnailKey / thumbnailGeneratedAt, and GET /api/documents/{id}/thumbnail are all already shipped. Phase 2 is not blocked — it needs to extend the existing service, not wait for it.
The spec's data contract introduces thumbnailUrl, thumbnailAspect, pageCount as "new fields from the thumbnail service". Two of them (thumbnailAspect, pageCount) need schema changes. thumbnailUrl is a client-side derivation from thumbnailKey + thumbnailGeneratedAt — it should not be added to the API surface.
GET /api/documents/conversation currently returns List<Document> — the entity is the API contract. This has been tolerable because eager-loaded collections (tags, receivers, sender) are already small, but the list can be 851 rows for one person. Adding two more fields is fine; the shape hasn't changed.
The spec claims WEBP storage "at 2× target resolution". ThumbnailService writes JPEG at a fixed 240-px width. That is a spec/implementation inconsistency that will surface the moment someone tries to follow the spec literally.
DistributionBar is shared with person-dashboard-spec.html. Same props, same aria-label pattern. Extraction location matters for reuse.

Recommendations

Drop thumbnailUrl from the backend contract. Derive the URL in $lib/thumbnails from thumbnailKey + thumbnailGeneratedAt — this is already how DocumentThumbnail.svelte works today. Sending a URL field muddies the API and invites cache-key drift.
Add thumbnailAspect (enum PORTRAIT | LANDSCAPE) and pageCount (integer) on Document in Phase 2. Compute both inside ThumbnailService#generate:
- thumbnailAspect = source.getWidth() / (float) source.getHeight() > 1.1f ? LANDSCAPE : PORTRAIT, persisted alongside thumbnailKey in persistThumbnailMetadata.
- pageCount = pdf.getNumberOfPages() from the already-loaded PDDocument in renderPdfFirstPage (return it up to generate). For image uploads: pageCount = 1.
- The existing ThumbnailBackfillService gets this data populated the next time it runs — no separate backfill job needed. Trigger the admin backfill once after migration.
Keep the spec honest about storage format. Either change the spec to "JPEG at 240 px width" (match reality) or open a separate issue for a WEBP rewrite with backfill cost estimate. Do not bundle a format change into this visual redesign.
Place DistributionBar.svelte in $lib/components/, not under $lib/components/conversation/ or inside the briefwechsel/ route. It's genuinely shared between two domains (conversation + person dashboard); cross-domain components belong at the library root so neither route directory "owns" it.

Migration shape (Phase 2 only):

-- V{n}__add_thumbnail_aspect_and_page_count.sql
ALTER TABLE documents
    ADD COLUMN thumbnail_aspect VARCHAR(16),
    ADD COLUMN page_count INTEGER;
-- no default, no NOT NULL — null means "not yet computed"
-- backfill runs via existing ThumbnailBackfillService after deployment

Document the decision as an ADR. Not the Phase-1 UI split — that's clean-code, no ADR needed — but the choice to compute thumbnailAspect/pageCount inside the existing ThumbnailService (vs. a separate metadata-extraction service). One paragraph. Context, decision, alternatives, consequences.

Open Decisions

None. The architecture is straightforward once the spec's staleness is corrected.

## 🏛️ Markus Keller — Senior Application Architect ### Observations - The "depends on the existing PDF thumbnail generation backend issue" note is stale. `ThumbnailService`, `ThumbnailBackfillService`, `ThumbnailAsyncRunner`, `AdminController#generateThumbnails`, `Document.thumbnailKey` / `thumbnailGeneratedAt`, and `GET /api/documents/{id}/thumbnail` are all already shipped. Phase 2 is not blocked — it needs to *extend* the existing service, not wait for it. - The spec's data contract introduces `thumbnailUrl`, `thumbnailAspect`, `pageCount` as "new fields from the thumbnail service". Two of them (`thumbnailAspect`, `pageCount`) need schema changes. `thumbnailUrl` is a client-side derivation from `thumbnailKey` + `thumbnailGeneratedAt` — it should **not** be added to the API surface. - `GET /api/documents/conversation` currently returns `List<Document>` — the entity is the API contract. This has been tolerable because eager-loaded collections (tags, receivers, sender) are already small, but the list can be 851 rows for one person. Adding two more fields is fine; the shape hasn't changed. - The spec claims WEBP storage "at 2× target resolution". `ThumbnailService` writes JPEG at a fixed 240-px width. That is a spec/implementation inconsistency that will surface the moment someone tries to follow the spec literally. - `DistributionBar` is shared with `person-dashboard-spec.html`. Same props, same aria-label pattern. Extraction location matters for reuse. ### Recommendations - **Drop `thumbnailUrl` from the backend contract.** Derive the URL in `$lib/thumbnails` from `thumbnailKey` + `thumbnailGeneratedAt` — this is already how `DocumentThumbnail.svelte` works today. Sending a URL field muddies the API and invites cache-key drift. - **Add `thumbnailAspect` (enum `PORTRAIT | LANDSCAPE`) and `pageCount` (integer) on `Document` in Phase 2.** Compute both inside `ThumbnailService#generate`: - `thumbnailAspect` = `source.getWidth() / (float) source.getHeight() > 1.1f ? LANDSCAPE : PORTRAIT`, persisted alongside `thumbnailKey` in `persistThumbnailMetadata`. - `pageCount` = `pdf.getNumberOfPages()` from the already-loaded `PDDocument` in `renderPdfFirstPage` (return it up to `generate`). For image uploads: `pageCount = 1`. - The existing `ThumbnailBackfillService` gets this data populated the next time it runs — no separate backfill job needed. Trigger the admin backfill once after migration. - **Keep the spec honest about storage format.** Either change the spec to "JPEG at 240 px width" (match reality) or open a separate issue for a WEBP rewrite with backfill cost estimate. Do not bundle a format change into this visual redesign. - **Place `DistributionBar.svelte` in `$lib/components/`**, not under `$lib/components/conversation/` or inside the `briefwechsel/` route. It's genuinely shared between two domains (conversation + person dashboard); cross-domain components belong at the library root so neither route directory "owns" it. - **Migration shape** (Phase 2 only): ```sql -- V{n}__add_thumbnail_aspect_and_page_count.sql ALTER TABLE documents ADD COLUMN thumbnail_aspect VARCHAR(16), ADD COLUMN page_count INTEGER; -- no default, no NOT NULL — null means "not yet computed" -- backfill runs via existing ThumbnailBackfillService after deployment ``` - **Document the decision as an ADR.** Not the Phase-1 UI split — that's clean-code, no ADR needed — but the choice to compute `thumbnailAspect`/`pageCount` inside the existing `ThumbnailService` (vs. a separate metadata-extraction service). One paragraph. Context, decision, alternatives, consequences. ### Open Decisions - None. The architecture is straightforward once the spec's staleness is corrected.

marcel commented

2026-04-23 11:18:56 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

ConversationTimeline.svelte is currently 202 lines doing three jobs: the bilateral distribution bar (lines 85–115), the year divider (lines 120–128), and the row markup (lines 130–173). Each is a nameable visual region — classic split trigger. The file already uses {#each ... (doc.id)} correctly.
DocumentThumbnail.svelte already exists at src/lib/components/DocumentThumbnail.svelte. It is not a match for what the spec needs:
- Fixed 5:7 portrait aspect (h-[84px] w-[60px] for sm, h-[168px] w-[120px] for lg). The spec needs portrait (82×106) and landscape (104×72) centered in a 104×120 cell.
- Uses object-cover object-top (crops). Spec implies the real aspect is shown — no cropping.
- No multi-page badge, no page-count awareness.
- No kind chip for postcards.
Today the component reads thumbnailKey + thumbnailGeneratedAt through $lib/thumbnails and derives the URL client-side. That's the right pattern; keep it.
summary on Document is a TEXT column. Spec says omit the element when empty. Today ConversationTimeline renders nothing for summary at all — new territory.
canWrite is already passed into ConversationTimeline and used for the "new doc" link at the bottom. The row itself has no write actions, so the new ThumbnailRow doesn't need it.

Recommendations

Component split (Phase 1) — TDD in this order:
1. Red: extract DistributionBar.svelte with props { outCount, inCount, outLabel, inLabel } and a snapshot/behavior test — assert role="img", the composed aria-label, segment widths from counts (not client percentages — Markus' point). No rendering change to ConversationTimeline yet; just introduce the component with a failing test that it renders the three elements (two labels + one bar). Green: move the existing JSX into the new component. Keep the old ConversationTimeline unchanged.
2. Red: failing test on ConversationTimeline that it uses <DistributionBar> when bilateral. Green: swap the inline markup for the component.
3. Red: ThumbnailRow.svelte receiving { doc, senderId, receiverId }, asserting title, summary (when non-empty), meta line, date, relative age, aria-label on the <a>. Green: write the row. No thumbnail yet — cell is a static skeleton div.
4. Red: assert the row renders no summary element when doc.summary is null/empty. Green: {#if doc.summary?.trim()}.
5. Red: assert year divider still appears on group boundaries. Green: orchestrate in ConversationTimeline.
6. Delete the old row markup from ConversationTimeline once every test is green on the new components.
Don't extend DocumentThumbnail.svelte — write a new ConversationThumbnail.svelte (or similar name). The existing component is used on /documents list pages with fixed aspect crop. Adding variable-aspect, postcard kind chips, and multi-page badges to it risks regressions on the document list. Two components is cheaper than one doing two visual jobs. The current DocumentThumbnail.svelte stays exactly as is.
Don't add thumbnailUrl as a data field — derive it in $lib/thumbnails the same way DocumentThumbnail already does. The spec's "Data contract" table is wrong on this point.
Summary guard clause: $derived(doc.summary?.trim() || null) then {#if computedSummary}. This handles both null and empty-after-trim.
Year divider key: when extracting YearDivider.svelte, the {#each} key must still be (doc.id) — the divider is rendered conditionally inside the loop, not as a sibling array. The spec's "YearDivider.svelte wrapper" is unnecessary — the current inline <div> at lines 120–128 is 8 lines, nameable, and couples tightly to enrichedDocuments. Leaving it inline is the cleaner call; don't extract for the sake of the spec.
Relative time ("vor N Jahren"): $lib/relativeTime.ts already exists in the codebase. Use it — don't compute inline.

Open Decisions

None. The test order is clean, the component split is obvious, and the data model extension is small.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations - `ConversationTimeline.svelte` is currently 202 lines doing three jobs: the bilateral distribution bar (lines 85–115), the year divider (lines 120–128), and the row markup (lines 130–173). Each is a nameable visual region — classic split trigger. The file already uses `{#each ... (doc.id)}` correctly. - `DocumentThumbnail.svelte` already exists at `src/lib/components/DocumentThumbnail.svelte`. It is **not a match** for what the spec needs: - Fixed 5:7 portrait aspect (`h-[84px] w-[60px]` for `sm`, `h-[168px] w-[120px]` for `lg`). The spec needs portrait (82×106) and landscape (104×72) centered in a 104×120 cell. - Uses `object-cover object-top` (crops). Spec implies the real aspect is shown — no cropping. - No multi-page badge, no page-count awareness. - No kind chip for postcards. - Today the component reads `thumbnailKey` + `thumbnailGeneratedAt` through `$lib/thumbnails` and derives the URL client-side. That's the right pattern; keep it. - `summary` on `Document` is a `TEXT` column. Spec says omit the element when empty. Today `ConversationTimeline` renders nothing for summary at all — new territory. - `canWrite` is already passed into `ConversationTimeline` and used for the "new doc" link at the bottom. The row itself has no write actions, so the new `ThumbnailRow` doesn't need it. ### Recommendations - **Component split (Phase 1) — TDD in this order:** 1. **Red**: extract `DistributionBar.svelte` with props `{ outCount, inCount, outLabel, inLabel }` and a snapshot/behavior test — assert `role="img"`, the composed `aria-label`, segment widths from counts (not client percentages — Markus' point). No rendering change to `ConversationTimeline` yet; just introduce the component with a failing test that it renders the three elements (two labels + one bar). Green: move the existing JSX into the new component. Keep the old `ConversationTimeline` unchanged. 2. **Red**: failing test on `ConversationTimeline` that it *uses* `<DistributionBar>` when bilateral. Green: swap the inline markup for the component. 3. **Red**: `ThumbnailRow.svelte` receiving `{ doc, senderId, receiverId }`, asserting title, summary (when non-empty), meta line, date, relative age, `aria-label` on the `<a>`. Green: write the row. No thumbnail yet — cell is a static skeleton div. 4. **Red**: assert the row renders *no* summary element when `doc.summary` is null/empty. Green: `{#if doc.summary?.trim()}`. 5. **Red**: assert year divider still appears on group boundaries. Green: orchestrate in `ConversationTimeline`. 6. Delete the old row markup from `ConversationTimeline` once every test is green on the new components. - **Don't extend `DocumentThumbnail.svelte` — write a new `ConversationThumbnail.svelte` (or similar name).** The existing component is used on `/documents` list pages with fixed aspect crop. Adding variable-aspect, postcard kind chips, and multi-page badges to it risks regressions on the document list. Two components is cheaper than one doing two visual jobs. The current `DocumentThumbnail.svelte` stays exactly as is. - **Don't add `thumbnailUrl` as a data field** — derive it in `$lib/thumbnails` the same way `DocumentThumbnail` already does. The spec's "Data contract" table is wrong on this point. - **Summary guard clause**: `$derived(doc.summary?.trim() || null)` then `{#if computedSummary}`. This handles both `null` and empty-after-trim. - **Year divider key**: when extracting `YearDivider.svelte`, the `{#each}` key must still be `(doc.id)` — the divider is rendered conditionally inside the loop, not as a sibling array. The spec's "YearDivider.svelte wrapper" is unnecessary — the current inline `<div>` at lines 120–128 is 8 lines, nameable, and couples tightly to `enrichedDocuments`. Leaving it inline is the cleaner call; don't extract for the sake of the spec. - **Relative time ("vor N Jahren")**: `$lib/relativeTime.ts` already exists in the codebase. Use it — don't compute inline. ### Open Decisions - None. The test order is clean, the component split is obvious, and the data model extension is small.

marcel commented

2026-04-23 11:19:15 +02:00

🔒 Nora Steiner — Application Security Engineer

Observations

GET /api/documents/{id}/thumbnail already sits behind the project's @RequirePermission / session auth — no new endpoint is introduced by this issue's Phase 2.
summary and location are user-controlled free text on Document. Rendered via standard Svelte interpolation ({doc.summary}, {doc.location}), Svelte HTML-escapes by default — safe.
Tag names are already sanitized at write-time on the backend; rendering them as text in the meta line is not a new injection vector.
thumbnailUrl as proposed in the spec does not exist as a backend field and should remain derived client-side (same-origin API path with an internal cache buster). No external URL ever lands in an <img src>.
Distribution bar uses role="img" + aria-label — the aria-label is composed from counts (integers) and names (already-escaped text). No XSS risk.
@GetMapping("/conversation") on DocumentController — inspected: returns List<Document> via DocumentService#getConversationFiltered. The query is parameterized. No SSRF, no injection surface added by this redesign.

Recommendations

Write a regression test for the summary rendering path as part of ThumbnailRow.svelte tests:

it('escapes HTML in summary text', async () => {
    const { getByText } = render(ThumbnailRow, {
        props: { doc: makeDoc({ summary: '<img src=x onerror="alert(1)">' }) }
    });
    await expect.element(getByText(/onerror/)).toBeVisible();  // text, not executed
});

Svelte auto-escapes, but new component + new render path = new test. Cheap to write, stays forever.

Verify Cache-Control on /api/documents/{id}/thumbnail matches the spec's 30-day immutable claim. Looking at DocumentController.java:98–112, this is already wired with ?v=<thumbnailGeneratedAt> as a cache-buster — good. If the spec's 30-day value isn't currently set, add it; otherwise the rolling cache across users is a CWE-525 concern when a thumbnail is replaced.
Do not log summary or originalFilename in the new row-rendering path or in the thumbnail service's pageCount/aspect extraction. Filenames can contain user-supplied data. Use SLF4J parameterized form if any logging is added: log.debug("Rendered row for doc={}", doc.getId()) — IDs only.
When adding thumbnailAspect persistence, validate the PDFBox / ImageIO output width/height are positive integers before dividing. A corrupt image with height=0 would cause a /0 that bubbles up — guard with a sanity check in ThumbnailService#persistThumbnailMetadata:
```
if (source.getWidth() <= 0 || source.getHeight() <= 0) {
    return Outcome.FAILED;  // already the pattern elsewhere in this service
}
```

Open Decisions

None. This is a primarily-UI change with a small, low-risk data-model extension.

## 🔒 Nora Steiner — Application Security Engineer ### Observations - `GET /api/documents/{id}/thumbnail` already sits behind the project's `@RequirePermission` / session auth — no new endpoint is introduced by this issue's Phase 2. - `summary` and `location` are user-controlled free text on `Document`. Rendered via standard Svelte interpolation (`{doc.summary}`, `{doc.location}`), Svelte HTML-escapes by default — safe. - Tag names are already sanitized at write-time on the backend; rendering them as text in the meta line is not a new injection vector. - `thumbnailUrl` as proposed in the spec does not exist as a backend field and should remain derived client-side (same-origin API path with an internal cache buster). No external URL ever lands in an `<img src>`. - Distribution bar uses `role="img"` + aria-label — the aria-label is composed from counts (integers) and names (already-escaped text). No XSS risk. - `@GetMapping("/conversation")` on `DocumentController` — inspected: returns `List<Document>` via `DocumentService#getConversationFiltered`. The query is parameterized. No SSRF, no injection surface added by this redesign. ### Recommendations - **Write a regression test for the `summary` rendering path** as part of `ThumbnailRow.svelte` tests: ```typescript it('escapes HTML in summary text', async () => { const { getByText } = render(ThumbnailRow, { props: { doc: makeDoc({ summary: '<img src=x onerror="alert(1)">' }) } }); await expect.element(getByText(/onerror/)).toBeVisible(); // text, not executed }); ``` Svelte auto-escapes, but new component + new render path = new test. Cheap to write, stays forever. - **Verify `Cache-Control` on `/api/documents/{id}/thumbnail`** matches the spec's 30-day immutable claim. Looking at `DocumentController.java:98–112`, this is already wired with `?v=<thumbnailGeneratedAt>` as a cache-buster — good. If the spec's 30-day value isn't currently set, add it; otherwise the rolling cache across users is a CWE-525 concern when a thumbnail is replaced. - **Do not log `summary` or `originalFilename`** in the new row-rendering path or in the thumbnail service's pageCount/aspect extraction. Filenames can contain user-supplied data. Use SLF4J parameterized form if any logging is added: `log.debug("Rendered row for doc={}", doc.getId())` — IDs only. - **When adding `thumbnailAspect` persistence**, validate the PDFBox / ImageIO output width/height are positive integers before dividing. A corrupt image with `height=0` would cause a `/0` that bubbles up — guard with a sanity check in `ThumbnailService#persistThumbnailMetadata`: ```java if (source.getWidth() <= 0 || source.getHeight() <= 0) { return Outcome.FAILED; // already the pattern elsewhere in this service } ``` ### Open Decisions - None. This is a primarily-UI change with a small, low-risk data-model extension.

marcel commented

2026-04-23 11:19:48 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

Phase 1 is primarily a structural component refactor (extract distribution bar, extract row, add summary line, add skeleton thumbnail) — the visible behavior changes are additive. This gives us a clean TDD path.
Current coverage that must not regress:
- frontend/src/routes/briefwechsel/page.svelte.spec.ts
- frontend/src/routes/briefwechsel/page.server.spec.ts
- E2E tests for /briefwechsel (check frontend/e2e/ — the bilateral flow is user-critical).
The spec asks for axe-playwright at 3 viewports × 2 themes = 6 runs per page, plus visual regression snapshots at 3 viewports. That's 6 axe assertions + 3 snapshots = 9 new Playwright checks for one page. Not excessive; fits the project's existing pattern.
Removal of the status dot and script-type chip means the statusDotClass() helper (lines 61–70 of ConversationTimeline.svelte) goes away. Any existing test that asserts bg-brand-mint, bg-brand-navy etc. on rows needs updating, not just deleting.

Recommendations

Phase 1 test pyramid:

Layer	File / scope	Assertions
Unit	`DistributionBar.svelte.spec.ts` (new)	renders two labels + bar; aria-label composed correctly; segment widths from backend counts (not `outCount / total * 100` on the client); empty state (0 / 0) doesn't divide by zero
Unit	`ThumbnailRow.svelte.spec.ts` (new)	title fallback to `originalFilename`; summary omitted when null/empty/whitespace; meta line tags cap at 2 (desktop) / 1 (tablet) / 0 (mobile) — test the visible-tag slicing logic; `aria-label` on the `<a>` carries date + title; border-l color matches `isOut`
Unit	`YearDivider.svelte.spec.ts` (only if extracted)	numeric year + pluralized `Briefe` count
Unit	`page.svelte.spec.ts` (existing)	updated to assert `<DistributionBar>` present when bilateral, absent otherwise; no longer asserts status dot class
Integration	`page.server.spec.ts` (existing)	no changes in Phase 1 (API surface unchanged)
Visual regression	`frontend/e2e/briefwechsel-rows.visual.spec.ts` (new)	320, 768, 1440 × light + dark = 6 snapshots. Use a fixed seed of ~6 documents covering: incoming typed, outgoing handwritten, postcard (landscape, Phase 2 only), multi-page (Phase 2 only), no-summary, long-summary
A11y	`frontend/e2e/briefwechsel-a11y.spec.ts` (new or extended)	axe-playwright at 3 viewports × 2 themes; assert `role="img"` on bar, aria-label on rows, 44-px touch target on rows

Phase 2 test pyramid (when adding thumbnailAspect + pageCount):

Layer	Scope	Assertions
Unit (Java)	`ThumbnailServiceTest` extension	aspect=PORTRAIT when w/h ≤ 1.1; aspect=LANDSCAPE when w/h > 1.1; pageCount=N for N-page PDF; pageCount=1 for image upload; both persisted in `persistThumbnailMetadata`
Integration (Java)	`MigrationTest` (if not already present)	new Flyway migration applies cleanly; `thumbnailAspect` and `page_count` columns exist with correct types; no data loss
Integration (Java)	`DocumentControllerTest#getConversation`	response body includes `thumbnailAspect` and `pageCount` fields
Unit (Svelte)	`ThumbnailRow` (update)	page badge rendered when `pageCount > 1`; not rendered when `pageCount ∈ {null, 1}`; kind chip rendered when `thumbnailAspect === 'LANDSCAPE'`; landscape and portrait thumbnails use different CSS classes/sizes

Deterministic seed data for visual regression. Don't rely on database state. Mock /api/documents/conversation to return a fixed list in the E2E visual test so snapshots are reproducible across runs and machines.
Do not regress the existing conv-new-doc-link flow (tested via data-testid="conv-new-doc-link" — line 180). The new ThumbnailRow doesn't host it, but ConversationTimeline must still render the canWrite footer link at the bottom of the list.
Reject any @Disabled, .skip, or it.only that sneaks in during the loop. Cycle-review catches them.

Open Decisions

Visual regression tolerance. Svelte 5 + subpixel rendering + Font loading can produce 1–3 px diffs that still "look the same." Either accept a Playwright maxDiffPixels threshold of ~100 per snapshot (common in this project) or make snapshots strict and accept occasional flake-fixes. Options & cost:
- Strict (0 px diff): catches real regressions instantly but every font tweak or Tailwind upgrade = 9 snapshots to re-baseline. Mild ongoing tax.
- Threshold 100 px: stable, but a 10-px label shift could sneak through. Mitigated by axe-playwright catching the WCAG-relevant cases.
- My default: threshold 100 px, revisited if any real regression slips through.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations - Phase 1 is primarily a structural component refactor (extract distribution bar, extract row, add summary line, add skeleton thumbnail) — the *visible* behavior changes are additive. This gives us a clean TDD path. - Current coverage that must not regress: - `frontend/src/routes/briefwechsel/page.svelte.spec.ts` - `frontend/src/routes/briefwechsel/page.server.spec.ts` - E2E tests for `/briefwechsel` (check `frontend/e2e/` — the bilateral flow is user-critical). - The spec asks for axe-playwright at **3 viewports × 2 themes = 6 runs** per page, plus **visual regression snapshots at 3 viewports**. That's 6 axe assertions + 3 snapshots = 9 new Playwright checks for one page. Not excessive; fits the project's existing pattern. - Removal of the status dot and script-type chip means the `statusDotClass()` helper (lines 61–70 of `ConversationTimeline.svelte`) goes away. Any existing test that asserts `bg-brand-mint`, `bg-brand-navy` etc. on rows needs updating, not just deleting. ### Recommendations - **Phase 1 test pyramid**: | Layer | File / scope | Assertions | |---|---|---| | Unit | `DistributionBar.svelte.spec.ts` (new) | renders two labels + bar; aria-label composed correctly; segment widths from backend counts (not `outCount / total * 100` on the client); empty state (0 / 0) doesn't divide by zero | | Unit | `ThumbnailRow.svelte.spec.ts` (new) | title fallback to `originalFilename`; summary omitted when null/empty/whitespace; meta line tags cap at 2 (desktop) / 1 (tablet) / 0 (mobile) — test the visible-tag slicing logic; `aria-label` on the `<a>` carries date + title; border-l color matches `isOut` | | Unit | `YearDivider.svelte.spec.ts` (only if extracted) | numeric year + pluralized `Briefe` count | | Unit | `page.svelte.spec.ts` (existing) | updated to assert `<DistributionBar>` present when bilateral, absent otherwise; no longer asserts status dot class | | Integration | `page.server.spec.ts` (existing) | no changes in Phase 1 (API surface unchanged) | | Visual regression | `frontend/e2e/briefwechsel-rows.visual.spec.ts` (new) | 320, 768, 1440 × light + dark = 6 snapshots. Use a fixed seed of ~6 documents covering: incoming typed, outgoing handwritten, postcard (landscape, Phase 2 only), multi-page (Phase 2 only), no-summary, long-summary | | A11y | `frontend/e2e/briefwechsel-a11y.spec.ts` (new or extended) | axe-playwright at 3 viewports × 2 themes; assert `role="img"` on bar, aria-label on rows, 44-px touch target on rows | - **Phase 2 test pyramid (when adding `thumbnailAspect` + `pageCount`)**: | Layer | Scope | Assertions | |---|---|---| | Unit (Java) | `ThumbnailServiceTest` extension | aspect=PORTRAIT when w/h ≤ 1.1; aspect=LANDSCAPE when w/h > 1.1; pageCount=N for N-page PDF; pageCount=1 for image upload; both persisted in `persistThumbnailMetadata` | | Integration (Java) | `MigrationTest` (if not already present) | new Flyway migration applies cleanly; `thumbnailAspect` and `page_count` columns exist with correct types; no data loss | | Integration (Java) | `DocumentControllerTest#getConversation` | response body includes `thumbnailAspect` and `pageCount` fields | | Unit (Svelte) | `ThumbnailRow` (update) | page badge rendered when `pageCount > 1`; not rendered when `pageCount ∈ {null, 1}`; kind chip rendered when `thumbnailAspect === 'LANDSCAPE'`; landscape and portrait thumbnails use different CSS classes/sizes | - **Deterministic seed data for visual regression.** Don't rely on database state. Mock `/api/documents/conversation` to return a fixed list in the E2E visual test so snapshots are reproducible across runs and machines. - **Do not regress the existing `conv-new-doc-link` flow** (tested via `data-testid="conv-new-doc-link"` — line 180). The new `ThumbnailRow` doesn't host it, but `ConversationTimeline` must still render the canWrite footer link at the bottom of the list. - **Reject any `@Disabled`, `.skip`, or `it.only` that sneaks in during the loop.** Cycle-review catches them. ### Open Decisions - **Visual regression tolerance.** Svelte 5 + subpixel rendering + Font loading can produce 1–3 px diffs that still "look the same." Either accept a Playwright `maxDiffPixels` threshold of ~100 per snapshot (common in this project) or make snapshots strict and accept occasional flake-fixes. **Options & cost:** - Strict (0 px diff): catches real regressions instantly but every font tweak or Tailwind upgrade = 9 snapshots to re-baseline. Mild ongoing tax. - Threshold 100 px: stable, but a 10-px label shift could sneak through. Mitigated by axe-playwright catching the WCAG-relevant cases. - My default: **threshold 100 px**, revisited if any real regression slips through.

marcel commented

2026-04-23 11:20:11 +02:00

🛠️ Tobias Wendt — DevOps & Platform Engineer

Observations

No new infrastructure required. Thumbnail storage already lives in the archive-documents MinIO bucket under the thumbnails/ prefix (see ThumbnailService.THUMBNAIL_KEY_PREFIX). No new bucket, no new service, no new docker-compose additions.
The spec says the thumbnail bucket is called thumbnails — it isn't; they live as a prefix inside the existing archive bucket. This is actually fine for the current volume; don't split buckets without a reason.
ThumbnailBackfillService is already wired, already has an admin endpoint (POST /api/admin/generate-thumbnails), already uses a dedicated thumbnailExecutor pool. When Phase 2 adds thumbnailAspect + pageCount, the backfill rerun gives us the data on existing documents — no migration-time compute needed.
CI impact is small: +1 Svelte component spec file per extraction, + ~9 Playwright E2E assertions. Probably +30–60 seconds on the frontend CI job. Well within budget.
Renovate config doesn't need touching — no new dependencies.
The spec mentions Cache-Control: public, max-age=2592000 (30 days) on the thumbnail redirect. Verify this is actually set on the 302 response from DocumentController#getThumbnail — if it's only on the presigned MinIO URL, browsers won't cache the redirect itself.

Recommendations

One Flyway migration for Phase 2 — don't split thumbnailAspect and pageCount across two migrations. Both come from the same place (ThumbnailService#generate); ship them together:

-- V{n}__add_thumbnail_aspect_and_page_count.sql
ALTER TABLE documents
    ADD COLUMN thumbnail_aspect VARCHAR(16),
    ADD COLUMN page_count INTEGER;

CREATE INDEX idx_documents_thumbnail_aspect
    ON documents (thumbnail_aspect)
    WHERE thumbnail_aspect IS NOT NULL;
-- Index only useful if we ever filter by aspect; drop it if we don't.

Skip the partial index unless the product plans to filter by aspect. "Might need it" isn't a reason to add it.

Deployment sequence for Phase 2:
1. Deploy backend with new columns + ThumbnailService populating them on new uploads.
2. Trigger POST /api/admin/generate-thumbnails to backfill existing rows. Thumbnails regenerate idempotently (same S3 key, PutObject overwrite).
3. Deploy frontend reading the new fields. Until backfill finishes, rows fall back to PORTRAIT + no page badge — handled by the spec's fallback contract.
Do NOT change the thumbnail format from JPEG to WEBP in this issue. The spec mentions WEBP; the code writes JPEG. If WEBP is actually wanted:
- It's a separate issue.
- Bundling it here means re-running backfill on every existing document + cache invalidation on every browser that has a 30-day Cache-Control hit.
- Raise it as its own feature issue with a cost estimate (pdfbox → TwelveMonkeys or similar); don't smuggle it into a UI redesign.
If visual regression snapshots are flaky in CI, pin the font loading. frontend/e2e/ should already use page.waitForLoadState('networkidle') or an equivalent font-ready assertion before toHaveScreenshot. Flag for Sara to verify during Phase 1.
Monitoring: no new metrics needed. ThumbnailBackfillService already logs counts. If you want, add a Grafana panel after this ships: "documents with thumbnail_aspect NULL" as a backfill-completeness gauge — but that's a nice-to-have, not a gate.

Open Decisions

None. Infrastructure side is small and additive.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer ### Observations - No new infrastructure required. Thumbnail storage already lives in the `archive-documents` MinIO bucket under the `thumbnails/` prefix (see `ThumbnailService.THUMBNAIL_KEY_PREFIX`). No new bucket, no new service, no new docker-compose additions. - The spec says the thumbnail bucket is called `thumbnails` — it isn't; they live as a prefix inside the existing archive bucket. This is actually **fine** for the current volume; don't split buckets without a reason. - `ThumbnailBackfillService` is already wired, already has an admin endpoint (`POST /api/admin/generate-thumbnails`), already uses a dedicated `thumbnailExecutor` pool. When Phase 2 adds `thumbnailAspect` + `pageCount`, the backfill rerun gives us the data on existing documents — no migration-time compute needed. - CI impact is small: +1 Svelte component spec file per extraction, + ~9 Playwright E2E assertions. Probably +30–60 seconds on the frontend CI job. Well within budget. - Renovate config doesn't need touching — no new dependencies. - The spec mentions `Cache-Control: public, max-age=2592000` (30 days) on the thumbnail redirect. Verify this is actually set on the 302 response from `DocumentController#getThumbnail` — if it's only on the presigned MinIO URL, browsers won't cache the redirect itself. ### Recommendations - **One Flyway migration for Phase 2** — don't split `thumbnailAspect` and `pageCount` across two migrations. Both come from the same place (`ThumbnailService#generate`); ship them together: ```sql -- V{n}__add_thumbnail_aspect_and_page_count.sql ALTER TABLE documents ADD COLUMN thumbnail_aspect VARCHAR(16), ADD COLUMN page_count INTEGER; CREATE INDEX idx_documents_thumbnail_aspect ON documents (thumbnail_aspect) WHERE thumbnail_aspect IS NOT NULL; -- Index only useful if we ever filter by aspect; drop it if we don't. ``` Skip the partial index unless the product plans to filter by aspect. "Might need it" isn't a reason to add it. - **Deployment sequence for Phase 2:** 1. Deploy backend with new columns + `ThumbnailService` populating them on new uploads. 2. Trigger `POST /api/admin/generate-thumbnails` to backfill existing rows. Thumbnails regenerate idempotently (same S3 key, PutObject overwrite). 3. Deploy frontend reading the new fields. Until backfill finishes, rows fall back to `PORTRAIT` + no page badge — handled by the spec's fallback contract. - **Do NOT change the thumbnail format from JPEG to WEBP** in this issue. The spec mentions WEBP; the code writes JPEG. If WEBP is actually wanted: - It's a separate issue. - Bundling it here means re-running backfill on every existing document + cache invalidation on every browser that has a 30-day Cache-Control hit. - Raise it as its own feature issue with a cost estimate (`pdfbox` → `TwelveMonkeys` or similar); don't smuggle it into a UI redesign. - **If visual regression snapshots are flaky in CI**, pin the font loading. `frontend/e2e/` should already use `page.waitForLoadState('networkidle')` or an equivalent font-ready assertion before `toHaveScreenshot`. Flag for Sara to verify during Phase 1. - **Monitoring**: no new metrics needed. `ThumbnailBackfillService` already logs counts. If you want, add a Grafana panel after this ships: "documents with thumbnail_aspect NULL" as a backfill-completeness gauge — but that's a nice-to-have, not a gate. ### Open Decisions - None. Infrastructure side is small and additive.

marcel commented

2026-04-23 11:20:48 +02:00

🎨 Leonie Voss — UI/UX & Accessibility

Observations

The spec's accessibility contract is strong: 128 px row >> WCAG 44 px, native <a> focus-visible ring, role="img" on distribution bar, prefers-reduced-motion on hover lift, redundant cues (arrow colour + glyph for direction). Contrast ratios verified for both modes in the spec's Section 05.
Two concerns are not flagged in the spec but show up on inspection:
1. The postcard kind chip and multi-page badge are 10 px uppercase (text-[10px]). The project's own style guide (CLAUDE.md: "Font sizes below 12 px" "DON'T") and my own floor say 12 px minimum for any visible text. Uppercase + 700 weight doesn't exempt it — small caps are harder for the senior audience, not easier.
2. "vor 102 Jahren" relative time sits in text-ink-3 on bg-surface. In dark mode, ink-3 is a muted gray — needs a concrete contrast measurement. The spec lists light-mode ratios but I want to see the dark-mode number for this specific token combination before signing off.
The spec keeps summary in italic serif with mint quote glyphs. Lovely for discovery UX. But summaries in this archive are user-authored free text: I've seen them run to 2–3 sentences. Clamping behavior isn't specified.
Border-left 3 px in border-l-primary (out) vs border-l-accent (in): redundant with the direction arrow + label in the meta line, so color-alone is not a WCAG violation. But the 3-px left border needs contrast ≥ 3:1 against the row background for non-text UI components (WCAG 1.4.11). brand-mint on bg-surface is close to that line; the spec claims AAA but 1.4.11 at 3 px accent is often where well-intentioned specs fail.
At 320 px, the thumbnail cell shrinks. The spec shows it still present. For a senior holding the phone single-handed in portrait, the row then has: thumbnail + title + summary + meta + date + relative time all within 296 px of usable width. That's a lot.

Recommendations

Bump the kind chip and page-count badge to text-[11px] minimum, prefer text-xs (12 px). Tailwind's text-[10px] is explicitly called out as a "don't" in the project's CLAUDE.md. Uppercase + bold doesn't earn you a smaller size — it earns you a same-size label that reads as structural.
Clamp the summary at 2 lines using line-clamp-2 with a fade or ellipsis. 3-sentence summaries break the 128-px row cadence and destroy the "quote" visual rhythm:
```
<p class="font-serif italic text-ink-2 line-clamp-2 overflow-hidden">
    {doc.summary}
</p>
```
The full summary is visible on the document detail page; the row is a teaser, not the payload.
At 320 px, drop the relative time ("vor N Jahren") and the counterpart name from the meta line. Keep: thumbnail skeleton → title → summary (if present) → direction arrow + date. Everything else is noise at that width. The meta line at 320 should read: → Berlin · Verlag (direction glyph, location, one tag max). The spec's current 320-px frame shows direction arrow + name + short date on one line — tight but OK. Just verify overflow-hidden + truncate on the counterpart name so a long name doesn't push the date off-screen.
Verify text-ink-3 contrast in dark mode specifically. The spec's dark-mode verification table should explicitly list text-ink-3 on bg-surface (regular) and on bg-muted (year divider background). If any combination is <4.5:1 for normal text, promote it to text-ink-2 for the "vor N Jahren" line.
Add a focus-within: ring on the row, not just focus-visible:. The row is a single <a>, so focus-visible: on the <a> is sufficient today — but if a tag chip becomes interactive later (click to filter), you'll want the outer row ring to activate on any child focus. Write this in now; it's free:
```
<a class="... focus-visible:ring-2 focus-visible:ring-primary focus-within:ring-2 focus-within:ring-primary/60">
```
Skeleton animation respects prefers-reduced-motion: the spec specifies this for hover lift but the 1.4-second shimmer on the skeleton also needs the same guard. Add:
```
@media (prefers-reduced-motion: reduce) {
    .thumb-skeleton { animation: none; }
}
```
Or use Tailwind's motion-safe:animate-pulse.
Postcards getting a "stamp + postmark corner via the thumbnail service" (spec Section 03, Type C) — flag this as Phase 2-plus, not Phase 2. Generating decorative stamps on server-rendered thumbnails is expensive, non-accessible (decorative image must have empty alt, which the current setup already does), and is a "nice flourish" that can ship after the core rows work. Split it into its own issue; don't let it block the main redesign.

Open Decisions

Summary clamp line count (2 vs 3). 2 lines keeps the row at 128 px and feels like a quote. 3 lines surfaces more content on the discovery page but pushes the row to ~150 px and loses rhythm on a long list. My recommendation: 2 lines, verified against the 851-letter list in Phase 3. Flip to 3 only if user feedback says the teaser feels too short.

## 🎨 Leonie Voss — UI/UX & Accessibility ### Observations - The spec's accessibility contract is strong: 128 px row >> WCAG 44 px, native `<a>` focus-visible ring, `role="img"` on distribution bar, `prefers-reduced-motion` on hover lift, redundant cues (arrow colour + glyph for direction). Contrast ratios verified for both modes in the spec's Section 05. - Two concerns are not flagged in the spec but show up on inspection: 1. **The postcard kind chip and multi-page badge are 10 px uppercase (`text-[10px]`).** The project's own style guide (CLAUDE.md: "Font sizes below 12 px" "DON'T") and my own floor say 12 px minimum for any visible text. Uppercase + 700 weight doesn't exempt it — small caps are harder for the senior audience, not easier. 2. **"vor 102 Jahren" relative time sits in `text-ink-3`** on `bg-surface`. In dark mode, ink-3 is a muted gray — needs a concrete contrast measurement. The spec lists light-mode ratios but I want to see the dark-mode number for this specific token combination before signing off. - The spec keeps summary in **italic serif with mint quote glyphs**. Lovely for discovery UX. But summaries in this archive are user-authored free text: I've seen them run to 2–3 sentences. Clamping behavior isn't specified. - Border-left 3 px in `border-l-primary` (out) vs `border-l-accent` (in): redundant with the direction arrow + label in the meta line, so color-alone is not a WCAG violation. But the 3-px left border needs contrast ≥ 3:1 against the row background for **non-text UI components** (WCAG 1.4.11). `brand-mint` on `bg-surface` is close to that line; the spec claims AAA but 1.4.11 at 3 px accent is often where well-intentioned specs fail. - At 320 px, the thumbnail cell shrinks. The spec shows it still present. For a senior holding the phone single-handed in portrait, the row then has: thumbnail + title + summary + meta + date + relative time all within 296 px of usable width. That's a lot. ### Recommendations - **Bump the kind chip and page-count badge to `text-[11px]` minimum, prefer `text-xs` (12 px).** Tailwind's `text-[10px]` is explicitly called out as a "don't" in the project's CLAUDE.md. Uppercase + bold doesn't earn you a smaller size — it earns you a same-size label that reads as structural. - **Clamp the summary at 2 lines** using `line-clamp-2` with a fade or ellipsis. 3-sentence summaries break the 128-px row cadence and destroy the "quote" visual rhythm: ```svelte <p class="font-serif italic text-ink-2 line-clamp-2 overflow-hidden"> {doc.summary} </p> ``` The full summary is visible on the document detail page; the row is a teaser, not the payload. - **At 320 px, drop the relative time ("vor N Jahren") and the counterpart name from the meta line.** Keep: thumbnail skeleton → title → summary (if present) → direction arrow + date. Everything else is noise at that width. The meta line at 320 should read: `→ Berlin · Verlag` (direction glyph, location, one tag max). The spec's current 320-px frame shows direction arrow + name + short date on one line — tight but OK. Just verify `overflow-hidden` + `truncate` on the counterpart name so a long name doesn't push the date off-screen. - **Verify `text-ink-3` contrast in dark mode specifically.** The spec's dark-mode verification table should explicitly list `text-ink-3` on `bg-surface` (regular) and on `bg-muted` (year divider background). If any combination is <4.5:1 for normal text, promote it to `text-ink-2` for the "vor N Jahren" line. - **Add a `focus-within:` ring on the row**, not just `focus-visible:`. The row is a single `<a>`, so `focus-visible:` on the `<a>` is sufficient *today* — but if a tag chip becomes interactive later (click to filter), you'll want the outer row ring to activate on any child focus. Write this in now; it's free: ```svelte <a class="... focus-visible:ring-2 focus-visible:ring-primary focus-within:ring-2 focus-within:ring-primary/60"> ``` - **Skeleton animation respects `prefers-reduced-motion`:** the spec specifies this for hover lift but the 1.4-second shimmer on the skeleton also needs the same guard. Add: ```css @media (prefers-reduced-motion: reduce) { .thumb-skeleton { animation: none; } } ``` Or use Tailwind's `motion-safe:animate-pulse`. - **Postcards getting a "stamp + postmark corner via the thumbnail service"** (spec Section 03, Type C) — flag this as **Phase 2-plus, not Phase 2**. Generating decorative stamps on server-rendered thumbnails is expensive, non-accessible (decorative image must have empty `alt`, which the current setup already does), and is a "nice flourish" that can ship after the core rows work. Split it into its own issue; don't let it block the main redesign. ### Open Decisions - **Summary clamp line count (2 vs 3).** 2 lines keeps the row at 128 px and feels like a quote. 3 lines surfaces more content on the discovery page but pushes the row to ~150 px and loses rhythm on a long list. **My recommendation: 2 lines**, verified against the 851-letter list in Phase 3. Flip to 3 only if user feedback says the teaser feels too short.

marcel commented

2026-04-23 11:21:11 +02:00

🗳️ Decision Queue — Action Required

2 decisions need your input before implementation starts.

UX / Content

Summary clamp — 2 lines vs 3 lines. 2 lines holds the 128 px row rhythm and reads like a quote; 3 lines surfaces more content at the cost of row cadence and a ~150 px row on long summaries. Leonie recommends 2 lines, revisit in Phase 3 based on feedback. (Raised by: Leonie)

Testing

Visual regression tolerance — strict 0 px vs maxDiffPixels: 100. Strict catches every pixel regression but forces a re-baseline of 9 snapshots on any font tweak or Tailwind bump. Threshold 100 is stable but can hide ~10 px label shifts (mitigated by axe-playwright catching WCAG cases). Sara recommends 100 px threshold. (Raised by: Sara)

Cross-cutting observations (not decisions — noted for implementation)

Multiple personas independently flagged the same items; recording them here so they don't get lost between review and plan:

The issue's "depends on the PDF thumbnail generation backend issue" is stale. Thumbnail infrastructure (ThumbnailService, ThumbnailBackfillService, /api/documents/{id}/thumbnail, thumbnailKey/thumbnailGeneratedAt) is already shipped. Phase 2 extends it, not waits for it. (Markus, Felix, Tobias)
Spec says WEBP, code writes JPEG. Do not smuggle a format change into this redesign. Either correct the spec or raise a separate WEBP issue with backfill cost. (Markus, Tobias)
Drop thumbnailUrl from the backend data contract. Derive it client-side via $lib/thumbnails as DocumentThumbnail.svelte already does. Only thumbnailAspect and pageCount are genuinely new backend fields. (Markus, Felix)
Existing DocumentThumbnail.svelte is fixed-aspect and portrait-only. Write a new component for the conversation row — don't extend the old one and risk regressions on /documents list pages. (Felix)
Kind chip and multi-page badge at text-[10px] violate the project's own 12 px minimum. Bump to text-xs. (Leonie)

## 🗳️ Decision Queue — Action Required _2 decisions need your input before implementation starts._ ### UX / Content - **Summary clamp — 2 lines vs 3 lines.** 2 lines holds the 128 px row rhythm and reads like a quote; 3 lines surfaces more content at the cost of row cadence and a ~150 px row on long summaries. _Leonie recommends 2 lines, revisit in Phase 3 based on feedback._ _(Raised by: Leonie)_ ### Testing - **Visual regression tolerance — strict 0 px vs `maxDiffPixels: 100`.** Strict catches every pixel regression but forces a re-baseline of 9 snapshots on any font tweak or Tailwind bump. Threshold 100 is stable but can hide ~10 px label shifts (mitigated by axe-playwright catching WCAG cases). _Sara recommends 100 px threshold._ _(Raised by: Sara)_ --- ### Cross-cutting observations (not decisions — noted for implementation) Multiple personas independently flagged the same items; recording them here so they don't get lost between review and plan: - **The issue's "depends on the PDF thumbnail generation backend issue" is stale.** Thumbnail infrastructure (`ThumbnailService`, `ThumbnailBackfillService`, `/api/documents/{id}/thumbnail`, `thumbnailKey`/`thumbnailGeneratedAt`) is already shipped. Phase 2 *extends* it, not waits for it. _(Markus, Felix, Tobias)_ - **Spec says WEBP, code writes JPEG.** Do not smuggle a format change into this redesign. Either correct the spec or raise a separate WEBP issue with backfill cost. _(Markus, Tobias)_ - **Drop `thumbnailUrl` from the backend data contract.** Derive it client-side via `$lib/thumbnails` as `DocumentThumbnail.svelte` already does. Only `thumbnailAspect` and `pageCount` are genuinely new backend fields. _(Markus, Felix)_ - **Existing `DocumentThumbnail.svelte` is fixed-aspect and portrait-only.** Write a new component for the conversation row — don't extend the old one and risk regressions on `/documents` list pages. _(Felix)_ - **Kind chip and multi-page badge at `text-[10px]` violate the project's own 12 px minimum.** Bump to `text-xs`. _(Leonie)_

marcel commented

2026-04-23 11:26:21 +02:00

Summary clamp: 2 lines it is
Visuall regression tolerance: maxDiffPixels: 100

Summary clamp: 2 lines it is Visuall regression tolerance: maxDiffPixels: 100

marcel commented

2026-04-23 11:32:43 +02:00

🎯 Discussion Resolutions

Walked through every point from the six persona reviews with the user. These resolutions are the authoritative design for implementation.

Theme 1 — Scope

Ship in one PR, not phased. The "depends on PDF thumbnail generation backend issue" reference was stale — that infrastructure is already in main. Phase 1 (UI refactor) and Phase 2 (backend fields) ship together.
Stay on JPEG/240 px. The spec's mention of WEBP/2× is dropped; the code-reality wins. If WEBP is ever wanted, it goes in its own issue with a cost estimate.
Stamp + postmark decoration dropped entirely. Not deferred — out of scope for this redesign.

Theme 2 — Data model & API

Drop thumbnailUrl from the backend contract. The client derives it via $lib/thumbnails from thumbnailKey + thumbnailGeneratedAt (already how DocumentThumbnail.svelte works).
Add two new fields on Document:
- thumbnailAspect — enum PORTRAIT | LANDSCAPE, threshold w/h > 1.1 → LANDSCAPE
- pageCount — integer
Compute both inside the existing ThumbnailService#generate: pageCount from the already-loaded PDDocument#getNumberOfPages() (1 for image uploads); thumbnailAspect from the BufferedImage dimensions. Persisted alongside thumbnailKey in persistThumbnailMetadata.
Guard width/height ≤ 0 before aspect computation to prevent division by zero on corrupt images. Returns Outcome.FAILED.
/api/documents/conversation stays List<Document>. Adding two fields doesn't justify introducing a DTO.
Single Flyway migration, nullable columns, no partial index.

Theme 3 — Component split (Phase 1)

Extract DistributionBar.svelte to src/lib/components/ (shared with the person dashboard issue).
Extract ThumbnailRow.svelte for the row markup.
Write a new ConversationThumbnail.svelte. Do not touch the existing DocumentThumbnail.svelte — it's fixed-aspect portrait, used on /documents, has different requirements. Two components is cheaper than one doing two jobs.
YearDivider stays inline (reject the spec's "new wrapper" — the 8-line block is clean as-is).
Summary guard: $derived(doc.summary?.trim() || null) then {#if computedSummary}.
Use the existing $lib/relativeTime.ts for "vor N Jahren".
TDD order: extract bar (red/green) → wire into timeline → build row → summary-omit test → year divider orchestration → delete old markup.

Summary clamp at 2 lines via line-clamp-2.
Kind chip and page badge bump from text-[10px] to text-xs (12 px). The project's own floor is 12 px; uppercase + bold doesn't earn a smaller size.
Dark-mode contrast verification for text-ink-3 on bg-surface and bg-muted. If either is below 4.5:1, promote "vor N Jahren" to text-ink-2.
Border-left 3 px mint accent: verify ≥ 3:1 contrast (WCAG 1.4.11 for non-text UI). Darken or widen if it fails.
320 px layout: follow the spec — keep direction arrow + counterpart name + relative time — but add truncate on the counterpart name so long names don't push the date off-screen.
Add focus-within:ring-* alongside focus-visible:ring-* on the row — free future-proofing for when child elements become interactive.
Skeleton shimmer respects prefers-reduced-motion (motion-safe:animate-pulse or equivalent). The spec covered this for hover lift only.

Theme 5 — Testing

Visual regression tolerance: maxDiffPixels: 100.
Phase 1 pyramid:
- Unit: new DistributionBar.svelte.spec.ts, new ThumbnailRow.svelte.spec.ts, updated page.svelte.spec.ts
- Visual regression: new briefwechsel-rows.visual.spec.ts — 320/768/1440 × light + dark = 6 snapshots
- A11y: axe-playwright at 3 viewports × 2 themes
Phase 2 pyramid:
- Unit (Java): ThumbnailServiceTest extended — PORTRAIT/LANDSCAPE threshold, pageCount for N-page PDF and image, persistence
- Integration: MigrationTest applies cleanly, new columns with correct types; DocumentControllerTest#getConversation response-shape check
Deterministic seed: mock /api/documents/conversation in visual E2E with a fixed ~6-doc fixture.
Preserve conv-new-doc-link — the canWrite footer link must still render from ConversationTimeline.
Update (not delete) existing statusDotClass assertions — they become border-l colour assertions matching isOut.

Theme 6 — Security hardening

XSS regression test for summary rendering in ThumbnailRow.svelte.spec.ts.
Verify Cache-Control: public, max-age=2592000 on the 302 response from DocumentController#getThumbnail. If it's missing or lives only on the presigned URL, add it to the redirect itself.
Do not log summary or originalFilename in new code paths. SLF4J parameterized form, IDs only.

Theme 7 — Deployment, backfill, ADR

Deployment sequence (documented in PR body): merge → deploy backend → trigger POST /api/admin/generate-thumbnails to backfill thumbnailAspect + pageCount on existing rows → deploy frontend. Until backfill finishes, frontend falls back to PORTRAIT + no page badge.
Short ADR under docs/adr/: one paragraph documenting the decision to compute aspect/pageCount inside the existing ThumbnailService vs. introducing a separate metadata-extraction service.
Grafana backfill-completeness panel skipped — nice-to-have, not a gate for this issue.

Open / Skipped

Stamp + postmark corner on postcards — dropped entirely, not backlogged.
WEBP format change — not opened as a follow-up issue; raise only if bandwidth problems actually surface.

The implement skill will read this comment alongside the original issue.

# 🎯 Discussion Resolutions Walked through every point from the six persona reviews with the user. These resolutions are the authoritative design for implementation. ## Theme 1 — Scope - **Ship in one PR, not phased.** The "depends on PDF thumbnail generation backend issue" reference was stale — that infrastructure is already in `main`. Phase 1 (UI refactor) and Phase 2 (backend fields) ship together. - **Stay on JPEG/240 px.** The spec's mention of WEBP/2× is dropped; the code-reality wins. If WEBP is ever wanted, it goes in its own issue with a cost estimate. - **Stamp + postmark decoration dropped entirely.** Not deferred — out of scope for this redesign. ## Theme 2 — Data model & API - **Drop `thumbnailUrl` from the backend contract.** The client derives it via `$lib/thumbnails` from `thumbnailKey` + `thumbnailGeneratedAt` (already how `DocumentThumbnail.svelte` works). - **Add two new fields on `Document`:** - `thumbnailAspect` — enum `PORTRAIT | LANDSCAPE`, threshold `w/h > 1.1 → LANDSCAPE` - `pageCount` — integer - **Compute both inside the existing `ThumbnailService#generate`:** `pageCount` from the already-loaded `PDDocument#getNumberOfPages()` (1 for image uploads); `thumbnailAspect` from the BufferedImage dimensions. Persisted alongside `thumbnailKey` in `persistThumbnailMetadata`. - **Guard width/height ≤ 0** before aspect computation to prevent division by zero on corrupt images. Returns `Outcome.FAILED`. - **`/api/documents/conversation` stays `List<Document>`.** Adding two fields doesn't justify introducing a DTO. - **Single Flyway migration, nullable columns, no partial index.** ## Theme 3 — Component split (Phase 1) - **Extract `DistributionBar.svelte`** to `src/lib/components/` (shared with the person dashboard issue). - **Extract `ThumbnailRow.svelte`** for the row markup. - **Write a new `ConversationThumbnail.svelte`.** Do not touch the existing `DocumentThumbnail.svelte` — it's fixed-aspect portrait, used on `/documents`, has different requirements. Two components is cheaper than one doing two jobs. - **`YearDivider` stays inline** (reject the spec's "new wrapper" — the 8-line block is clean as-is). - **Summary guard**: `$derived(doc.summary?.trim() || null)` then `{#if computedSummary}`. - **Use the existing `$lib/relativeTime.ts`** for "vor N Jahren". - **TDD order**: extract bar (red/green) → wire into timeline → build row → summary-omit test → year divider orchestration → delete old markup. ## Theme 4 — UI details & a11y - **Summary clamp at 2 lines** via `line-clamp-2`. - **Kind chip and page badge bump from `text-[10px]` to `text-xs`** (12 px). The project's own floor is 12 px; uppercase + bold doesn't earn a smaller size. - **Dark-mode contrast verification** for `text-ink-3` on `bg-surface` and `bg-muted`. If either is below 4.5:1, promote "vor N Jahren" to `text-ink-2`. - **Border-left 3 px mint accent**: verify ≥ 3:1 contrast (WCAG 1.4.11 for non-text UI). Darken or widen if it fails. - **320 px layout**: follow the spec — keep direction arrow + counterpart name + relative time — but add `truncate` on the counterpart name so long names don't push the date off-screen. - **Add `focus-within:ring-*` alongside `focus-visible:ring-*`** on the row — free future-proofing for when child elements become interactive. - **Skeleton shimmer respects `prefers-reduced-motion`** (`motion-safe:animate-pulse` or equivalent). The spec covered this for hover lift only. ## Theme 5 — Testing - **Visual regression tolerance: `maxDiffPixels: 100`.** - **Phase 1 pyramid:** - Unit: new `DistributionBar.svelte.spec.ts`, new `ThumbnailRow.svelte.spec.ts`, updated `page.svelte.spec.ts` - Visual regression: new `briefwechsel-rows.visual.spec.ts` — 320/768/1440 × light + dark = 6 snapshots - A11y: axe-playwright at 3 viewports × 2 themes - **Phase 2 pyramid:** - Unit (Java): `ThumbnailServiceTest` extended — PORTRAIT/LANDSCAPE threshold, pageCount for N-page PDF and image, persistence - Integration: `MigrationTest` applies cleanly, new columns with correct types; `DocumentControllerTest#getConversation` response-shape check - **Deterministic seed**: mock `/api/documents/conversation` in visual E2E with a fixed ~6-doc fixture. - **Preserve `conv-new-doc-link`** — the canWrite footer link must still render from `ConversationTimeline`. - **Update (not delete)** existing `statusDotClass` assertions — they become border-l colour assertions matching `isOut`. ## Theme 6 — Security hardening - **XSS regression test for summary rendering** in `ThumbnailRow.svelte.spec.ts`. - **Verify `Cache-Control: public, max-age=2592000` on the 302 response** from `DocumentController#getThumbnail`. If it's missing or lives only on the presigned URL, add it to the redirect itself. - **Do not log `summary` or `originalFilename`** in new code paths. SLF4J parameterized form, IDs only. ## Theme 7 — Deployment, backfill, ADR - **Deployment sequence** (documented in PR body): merge → deploy backend → trigger `POST /api/admin/generate-thumbnails` to backfill `thumbnailAspect` + `pageCount` on existing rows → deploy frontend. Until backfill finishes, frontend falls back to PORTRAIT + no page badge. - **Short ADR under `docs/adr/`**: one paragraph documenting the decision to compute aspect/pageCount inside the existing `ThumbnailService` vs. introducing a separate metadata-extraction service. - **Grafana backfill-completeness panel skipped** — nice-to-have, not a gate for this issue. ## Open / Skipped - **Stamp + postmark corner on postcards** — dropped entirely, not backlogged. - **WEBP format change** — not opened as a follow-up issue; raise only if bandwidth problems actually surface. --- The `implement` skill will read this comment alongside the original issue.

marcel referenced this issue from a commit

2026-04-23 16:27:40 +02:00

refactor(briefwechsel): extract bilateral DistributionBar component

marcel referenced this issue from a commit

2026-04-23 16:27:40 +02:00

refactor(briefwechsel): ConversationTimeline consumes DistributionBar

marcel referenced this issue from a commit

2026-04-23 16:27:40 +02:00

feat(relativeTime): add relativeYearsDe helper for historical letter dates

marcel referenced this issue from a commit

2026-04-23 16:27:40 +02:00

feat(briefwechsel): add ConversationThumbnail with aspect + page badge