As a user I want the dashboard resume strip to show the actual document thumbnail so I recognize what I was working on at a glance #314

marcel · 2026-04-23T22:33:55+02:00

marcel commented

2026-04-23 22:33:55 +02:00

Closes #309

Summary

Replaces the generic parchment SVG in the dashboard resume strip with the real document thumbnail, so the "pick up where you left off" card actually shows the user what they were last working on.

Per the Discussion Resolutions, this also unifies thumbnail URL construction on the backend as the single source of truth:

Document.getThumbnailUrl() is now a @JsonProperty computed getter on the entity, so every Document JSON response automatically carries the URL.
The frontend $lib/thumbnails.ts helper has been retired; DocumentThumbnail.svelte reads doc.thumbnailUrl directly.
DashboardService.getResume() populates the resume DTO's thumbnailUrl via the same getter, replacing the previous null placeholder.

Design decisions (from the resolutions comment)

Fallback: document-text heroicon (matching DocumentThumbnail) when the document has no thumbnail yet — one "no thumbnail" vocabulary across the app. The parchment SVG survives only in the fully-empty state (no resume document at all).
Dimensions: 180×252 (exact 5:7 A4 ratio, matching DocumentThumbnail everywhere else in the app). Issue said "~180×246"; six pixels aligns the card with the rest of the thumbnail surfaces.
Accessibility: alt="" (decorative — title carries the semantics), sized container to prevent CLS, dark:mix-blend-multiply for paper-scan glare, aria-hidden="true" on the fallback wrapper.
Cache-buster safety: a code comment on Document.getThumbnailUrl() documents that the ?v={thumbnailGeneratedAt} query param is load-bearing for the thumbnail endpoint's immutable header — dropping it would create a CWE-525 cross-user cache poisoning risk.

Coverage

Backend: new DocumentTest.java with 3 cases on the getter (null, no-cache-buster, with-cache-buster including exact URLEncoder encoding), plus one wiring test in DashboardServiceTest proving getResume populates the DTO field from the Document.
Frontend: two new cases in DashboardResumeStrip.svelte.spec.ts (thumbnail <img> renders with correct attrs when URL set; fallback heroicon renders when URL null), using stable data-testid selectors on both branches.

Final status

Backend: 1256/1256 tests green, ./mvnw clean package -DskipTests BUILD SUCCESS.
Frontend: 994/994 tests green, zero new svelte-check errors (242 pre-existing ones untouched).

Closes #309 ## Summary Replaces the generic parchment SVG in the dashboard resume strip with the real document thumbnail, so the "pick up where you left off" card actually shows the user what they were last working on. Per the [Discussion Resolutions](http://heim-nas:3005/marcel/familienarchiv/issues/309#issuecomment-4111), this also **unifies thumbnail URL construction on the backend as the single source of truth**: - `Document.getThumbnailUrl()` is now a `@JsonProperty` computed getter on the entity, so every `Document` JSON response automatically carries the URL. - The frontend `$lib/thumbnails.ts` helper has been retired; `DocumentThumbnail.svelte` reads `doc.thumbnailUrl` directly. - `DashboardService.getResume()` populates the resume DTO's `thumbnailUrl` via the same getter, replacing the previous `null` placeholder. ## Design decisions (from the resolutions comment) - **Fallback**: document-text heroicon (matching `DocumentThumbnail`) when the document has no thumbnail yet — one "no thumbnail" vocabulary across the app. The parchment SVG survives only in the fully-empty state (no resume document at all). - **Dimensions**: 180×252 (exact 5:7 A4 ratio, matching `DocumentThumbnail` everywhere else in the app). Issue said "~180×246"; six pixels aligns the card with the rest of the thumbnail surfaces. - **Accessibility**: `alt=""` (decorative — title carries the semantics), sized container to prevent CLS, `dark:mix-blend-multiply` for paper-scan glare, `aria-hidden="true"` on the fallback wrapper. - **Cache-buster safety**: a code comment on `Document.getThumbnailUrl()` documents that the `?v={thumbnailGeneratedAt}` query param is load-bearing for the thumbnail endpoint's `immutable` header — dropping it would create a CWE-525 cross-user cache poisoning risk. ## Coverage - **Backend**: new `DocumentTest.java` with 3 cases on the getter (null, no-cache-buster, with-cache-buster including exact `URLEncoder` encoding), plus one wiring test in `DashboardServiceTest` proving `getResume` populates the DTO field from the Document. - **Frontend**: two new cases in `DashboardResumeStrip.svelte.spec.ts` (thumbnail `<img>` renders with correct attrs when URL set; fallback heroicon renders when URL null), using stable `data-testid` selectors on both branches. ## Final status - Backend: 1256/1256 tests green, `./mvnw clean package -DskipTests` BUILD SUCCESS. - Frontend: 994/994 tests green, zero new `svelte-check` errors (242 pre-existing ones untouched).

marcel commented

2026-04-23 22:35:15 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

What I checked

TDD evidence in commit history — every task has a red test before the green code. ✓
Guard clause in Document.getThumbnailUrl() keeps the happy path at the lowest indentation. ✓
Names reveal intent: getThumbnailUrl, mockResumeWithThumbnail, resume-thumbnail-img, resume-thumbnail-fallback. ✓
DocumentThumbnail.svelte Pick type trimmed to what it actually uses (id | thumbnailUrl | contentType) instead of dragging along retired fields. ✓
$derived(doc.thumbnailUrl ?? null) — correct Svelte 5 runes usage, not $state + $effect. ✓

Suggestions (non-blocking)

DashboardService.java:85 — the inline doc.getThumbnailUrl() in the constructor call is fine as-is at two args, but if a next field joins it I'd pull it to a local. Not worth touching now.
The URL path string "/api/documents/" is duplicated between Document.getThumbnailUrl() and DocumentController — if the controller's @RequestMapping("/api/documents") ever changes, the getter won't follow. A tiny suggestion for a follow-up: either extract a DocumentEndpoints.BASE_PATH constant both sides reference, or wire the base URL via ServletUriComponentsBuilder at serialisation time. Not a blocker — the issue's out-of-scope clause on generalisation applies here too.

LGTM

Clean, small, and the commit history reads as a TDD textbook. Ship it.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** ### What I checked - TDD evidence in commit history — every task has a red test before the green code. ✓ - Guard clause in `Document.getThumbnailUrl()` keeps the happy path at the lowest indentation. ✓ - Names reveal intent: `getThumbnailUrl`, `mockResumeWithThumbnail`, `resume-thumbnail-img`, `resume-thumbnail-fallback`. ✓ - `DocumentThumbnail.svelte` Pick type trimmed to what it actually uses (`id | thumbnailUrl | contentType`) instead of dragging along retired fields. ✓ - `$derived(doc.thumbnailUrl ?? null)` — correct Svelte 5 runes usage, not `$state` + `$effect`. ✓ ### Suggestions (non-blocking) - `DashboardService.java:85` — the inline `doc.getThumbnailUrl()` in the constructor call is fine as-is at two args, but if a next field joins it I'd pull it to a local. Not worth touching now. - The URL path string `"/api/documents/"` is duplicated between `Document.getThumbnailUrl()` and `DocumentController` — if the controller's `@RequestMapping("/api/documents")` ever changes, the getter won't follow. A tiny suggestion for a follow-up: either extract a `DocumentEndpoints.BASE_PATH` constant both sides reference, or wire the base URL via `ServletUriComponentsBuilder` at serialisation time. Not a blocker — the issue's out-of-scope clause on generalisation applies here too. ### LGTM Clean, small, and the commit history reads as a TDD textbook. Ship it.

marcel commented

2026-04-23 22:35:25 +02:00

🏛️ Markus Keller — Senior Application Architect

Verdict: ✅ Approved

Structure

Change stays inside the dashboard and model feature packages. No cross-boundary repository access, no new service dependencies, no new transport protocol. ✓
DashboardService.getResume() still routes through DocumentService.getDocumentById() — the layering rule is intact. ✓
Zero extra queries: the new thumbnailUrl is computed from fields on the already-loaded Document. No N+1 risk. ✓
No migration needed — the Document entity's thumbnailKey and thumbnailGeneratedAt columns are already present. ✓

One caveat I want on the record

The Document entity now carries URL-shape knowledge (/api/documents/{id}/thumbnail). This was the design decision from the Resolutions comment and I signed off there, but calling it out explicitly: the entity model and the controller's @RequestMapping are now coupled through this string. If the controller path ever moves (e.g. to /v2/documents), two places need to stay in sync. The counter-argument — that this keeps every Document JSON response carrying the URL with zero per-controller plumbing — was the winning one, and I still agree with it. Just be aware of the trade for future maintainers.

No ADR needed

One computed getter on one entity. The Resolutions comment on #309 is the decision log.

LGTM

Boring, small, and the architectural posture is consistent with the monolith-first philosophy. Merge when the loop clears.

## 🏛️ Markus Keller — Senior Application Architect **Verdict: ✅ Approved** ### Structure - Change stays inside the `dashboard` and `model` feature packages. No cross-boundary repository access, no new service dependencies, no new transport protocol. ✓ - `DashboardService.getResume()` still routes through `DocumentService.getDocumentById()` — the layering rule is intact. ✓ - Zero extra queries: the new `thumbnailUrl` is computed from fields on the already-loaded `Document`. No N+1 risk. ✓ - No migration needed — the `Document` entity's `thumbnailKey` and `thumbnailGeneratedAt` columns are already present. ✓ ### One caveat I want on the record - The `Document` entity now carries URL-shape knowledge (`/api/documents/{id}/thumbnail`). This was the design decision from the Resolutions comment and I signed off there, but calling it out explicitly: the entity model and the controller's `@RequestMapping` are now coupled through this string. If the controller path ever moves (e.g. to `/v2/documents`), two places need to stay in sync. The counter-argument — that this keeps every `Document` JSON response carrying the URL with zero per-controller plumbing — was the winning one, and I still agree with it. Just be aware of the trade for future maintainers. ### No ADR needed One computed getter on one entity. The Resolutions comment on #309 is the decision log. ### LGTM Boring, small, and the architectural posture is consistent with the monolith-first philosophy. Merge when the loop clears.

marcel commented

2026-04-23 22:35:39 +02:00

🔒 Nora Steiner — Application Security Engineer

Verdict: ✅ Approved

Surface review

No injection. The URL is composed from id (UUID), a hardcoded path literal, and a LocalDateTime (backend-controlled). None of it comes from user input. No SQLi/XSS/SSRF/path-traversal vector.
No new auth surface. The thumbnail endpoint GET /api/documents/{id}/thumbnail is unchanged; this PR just embeds its relative URL in a DTO the user is already authorized to receive. Follow-up GETs carry the user's cookie through the standard app chain.
Cache-Control invariant preserved. private, max-age=31536000, immutable at DocumentController.java:114 is still safe because the ?v={thumbnailGeneratedAt} cache-buster changes when the file does. The load-bearing comment on Document.getThumbnailUrl() (lines 131-136) documents this for future readers — exactly what I asked for. ✓
CWE-525 (cross-user cache leak) guard documented. The code comment names the CVE class so the next developer tempted to drop the ?v= query "for cleanup" sees the risk inline. ✓

Encoding parity

URLEncoder.encode(timestamp, StandardCharsets.UTF_8) vs. the frontend's old encodeURIComponent produces identical output for the characters that appear in LocalDateTime.toString() (digits, T, -, :, .). For any hypothetical future input containing spaces the two would diverge (+ vs %20), but LocalDateTime.toString() never emits spaces — and the frontend builder is gone now anyway, so the concern is moot. ✓

Audit trail

DocumentTest.java locks the exact encoded URL shape including the %3A for the : separator — any future change that breaks the convention fails at test time, not at "users report stale thumbnails" time.

LGTM

No new vulnerabilities introduced and the security-relevant comment on the getter is a meaningful addition to the defensive commentary.

## 🔒 Nora Steiner — Application Security Engineer **Verdict: ✅ Approved** ### Surface review - **No injection.** The URL is composed from `id` (UUID), a hardcoded path literal, and a `LocalDateTime` (backend-controlled). None of it comes from user input. No SQLi/XSS/SSRF/path-traversal vector. - **No new auth surface.** The thumbnail endpoint `GET /api/documents/{id}/thumbnail` is unchanged; this PR just embeds its relative URL in a DTO the user is already authorized to receive. Follow-up GETs carry the user's cookie through the standard app chain. - **Cache-Control invariant preserved.** `private, max-age=31536000, immutable` at `DocumentController.java:114` is still safe because the `?v={thumbnailGeneratedAt}` cache-buster changes when the file does. The load-bearing comment on `Document.getThumbnailUrl()` (lines 131-136) documents this for future readers — exactly what I asked for. ✓ - **CWE-525 (cross-user cache leak) guard documented.** The code comment names the CVE class so the next developer tempted to drop the `?v=` query "for cleanup" sees the risk inline. ✓ ### Encoding parity - `URLEncoder.encode(timestamp, StandardCharsets.UTF_8)` vs. the frontend's old `encodeURIComponent` produces identical output for the characters that appear in `LocalDateTime.toString()` (digits, `T`, `-`, `:`, `.`). For any hypothetical future input containing spaces the two would diverge (`+` vs `%20`), but `LocalDateTime.toString()` never emits spaces — and the frontend builder is gone now anyway, so the concern is moot. ✓ ### Audit trail - `DocumentTest.java` locks the exact encoded URL shape including the `%3A` for the `:` separator — any future change that breaks the convention fails at test time, not at "users report stale thumbnails" time. ### LGTM No new vulnerabilities introduced and the security-relevant comment on the getter is a meaningful addition to the defensive commentary.

marcel commented

2026-04-23 22:35:54 +02:00

🧪 Sara Holt — QA Engineer

Verdict: ⚠️ Approved with concerns

What's solid

DocumentTest.java covers the three behavioural branches of the getter and locks the exact encoded URL shape — %3A for : is asserted literally. A future developer who accidentally broke the encoding convention would fail at test time. ✓
DashboardServiceTest.getResume_populatesThumbnailUrl_fromDocument correctly tests the wiring (DTO field populated from the Document) without re-testing the URL shape. Right division of responsibility. ✓
Frontend spec additions use data-testid selectors on both branches (resume-thumbnail-img, resume-thumbnail-fallback) — stable against future DOM refactors. ✓
One-line-per-thing setup via the spread-with-override (...mockResume, thumbnailUrl: '...'). ✓

Concerns (non-blocking, worth considering)

No test covers the Jackson serialisation path itself. The backend assertion is result.thumbnailUrl() — a method call on the DTO record, not "the JSON that ships to the browser has thumbnailUrl as a field". If someone removed @JsonProperty("thumbnailUrl") tomorrow, all current tests would still pass, but the frontend would see undefined. A single ObjectMapper test — "serialising a Document with a thumbnailKey produces JSON with a thumbnailUrl property" — would close that gap. Low cost, high signal.
Frontend empty-state branch coverage is implicit, not explicit. The existing tests that use mockResume (which has no thumbnailUrl) now exercise the fallback icon path — that's fine coverage, but the dedicated renders fallback icon when thumbnailUrl is null test is the one I'd rely on if something broke. Just noting: the implicit coverage is a happy accident, not the intended test strategy.
No Playwright/E2E coverage for "thumbnail actually loads on the dashboard." This is intentional per the issue's test strategy (unit-level coverage only), but worth flagging that the end-to-end path MinIO → thumbnail endpoint → DTO serialisation → SSR → browser <img> has no automated proof. A smoke test in the existing dashboard E2E spec (if one exists) would be a nice future addition.

LGTM overall

The concerns are improvements, not blockers — the existing coverage is solid enough for a merge. Flagging #1 as the one I'd most like to see addressed: one small test, one big invariant protected.

## 🧪 Sara Holt — QA Engineer **Verdict: ⚠️ Approved with concerns** ### What's solid - `DocumentTest.java` covers the three behavioural branches of the getter and locks the exact encoded URL shape — `%3A` for `:` is asserted literally. A future developer who accidentally broke the encoding convention would fail at test time. ✓ - `DashboardServiceTest.getResume_populatesThumbnailUrl_fromDocument` correctly tests the wiring (DTO field populated from the Document) without re-testing the URL shape. Right division of responsibility. ✓ - Frontend spec additions use `data-testid` selectors on both branches (`resume-thumbnail-img`, `resume-thumbnail-fallback`) — stable against future DOM refactors. ✓ - One-line-per-thing setup via the spread-with-override (`...mockResume, thumbnailUrl: '...'`). ✓ ### Concerns (non-blocking, worth considering) 1. **No test covers the Jackson serialisation path itself.** The backend assertion is `result.thumbnailUrl()` — a method call on the DTO record, not "the JSON that ships to the browser has `thumbnailUrl` as a field". If someone removed `@JsonProperty("thumbnailUrl")` tomorrow, all current tests would still pass, but the frontend would see `undefined`. A single `ObjectMapper` test — "serialising a Document with a thumbnailKey produces JSON with a `thumbnailUrl` property" — would close that gap. Low cost, high signal. 2. **Frontend empty-state branch coverage is implicit, not explicit.** The existing tests that use `mockResume` (which has no `thumbnailUrl`) now exercise the fallback icon path — that's fine coverage, but the **dedicated** `renders fallback icon when thumbnailUrl is null` test is the one I'd rely on if something broke. Just noting: the implicit coverage is a happy accident, not the intended test strategy. 3. **No Playwright/E2E coverage** for "thumbnail actually loads on the dashboard." This is intentional per the issue's test strategy (unit-level coverage only), but worth flagging that the end-to-end path `MinIO → thumbnail endpoint → DTO serialisation → SSR → browser <img>` has no automated proof. A smoke test in the existing dashboard E2E spec (if one exists) would be a nice future addition. ### LGTM overall The concerns are improvements, not blockers — the existing coverage is solid enough for a merge. Flagging #1 as the one I'd most like to see addressed: one small test, one big invariant protected.

marcel commented

2026-04-23 22:36:06 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

What I checked

No Compose changes. docker-compose.yml untouched, minio service handles thumbnail objects as before, archive-documents bucket bootstrap unchanged. ✓
No new env vars, ports, volumes. ✓
No CI workflow touch. Backend unit tests run in the Mockito job; frontend tests run in the Vitest browser-mode job. The existing pipeline handles this PR without adjustment. ✓
Traffic profile is fine. One extra thumbnail request per dashboard load, cacheable immutable for a year — zero cost on repeat visits. First-load proxies through the JVM to MinIO, same pattern as the document list. ✓

One operational note

The commit message for chore(api): regenerate Document type with thumbnailUrl field is honest about being a manual edit rather than a real npm run generate:api pass. That's correct given the worktree constraint, and the comment is explicit so a future maintainer knows to regen properly after merge. I'd just add to the post-merge checklist: run npm run generate:api against a rebuilt dev backend and confirm the diff is empty. If it's not, we learn something.

Monitoring suggestion (future work)

Watch /api/documents/{id}/thumbnail p95 in Grafana as dashboard usage grows. If it ever becomes a hot path, pre-signed S3 URLs take MinIO's bytes off the JVM heap. Not needed today — the dashboard resume card is one image per page, not a grid.

LGTM

Zero infrastructure surface change, zero runbook impact. Ship it.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** ### What I checked - **No Compose changes.** `docker-compose.yml` untouched, `minio` service handles thumbnail objects as before, `archive-documents` bucket bootstrap unchanged. ✓ - **No new env vars, ports, volumes.** ✓ - **No CI workflow touch.** Backend unit tests run in the Mockito job; frontend tests run in the Vitest browser-mode job. The existing pipeline handles this PR without adjustment. ✓ - **Traffic profile is fine.** One extra thumbnail request per dashboard load, cacheable `immutable` for a year — zero cost on repeat visits. First-load proxies through the JVM to MinIO, same pattern as the document list. ✓ ### One operational note - The commit message for `chore(api): regenerate Document type with thumbnailUrl field` is honest about being a manual edit rather than a real `npm run generate:api` pass. That's correct given the worktree constraint, and the comment is explicit so a future maintainer knows to regen properly after merge. I'd just add to the post-merge checklist: run `npm run generate:api` against a rebuilt dev backend and confirm the diff is empty. If it's not, we learn something. ### Monitoring suggestion (future work) - Watch `/api/documents/{id}/thumbnail` p95 in Grafana as dashboard usage grows. If it ever becomes a hot path, pre-signed S3 URLs take MinIO's bytes off the JVM heap. Not needed today — the dashboard resume card is one image per page, not a grid. ### LGTM Zero infrastructure surface change, zero runbook impact. Ship it.

marcel commented

2026-04-23 22:36:23 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Advocate

Verdict: ⚠️ Approved with concerns

What's right

180×252, exact 5:7 A4 ratio. Matches DocumentThumbnail everywhere else in the app. The strip no longer looks squatter than the rest of the thumbnail surfaces. ✓
Sized container reserves space before the img decodes (h-[252px] w-[180px] on the wrapper). No CLS. ✓
alt="" — correct, decorative image, the <h2> title + caption already label the card. Screen readers won't double-announce. ✓
dark:mix-blend-multiply on the img — paper scans won't glare on a dark page background. ✓
aria-hidden="true" on the fallback icon wrapper — decorative-only. ✓
loading="lazy" + decoding="async" — the img doesn't block first paint. ✓
Consistent "no thumbnail" vocabulary — same heroicon as DocumentThumbnail.svelte:44-55. Users learn one shape for one meaning. ✓

One concern (non-blocking)

Fallback icon size on the larger container. The fallback SVG uses class="h-16 w-16" (64×64). That's the same size DocumentThumbnail uses for its lg container (120×168). On the resume strip's 180×252 container — significantly bigger — a 64×64 icon looks visually undersized: it's only ~25% of the height, compared to ~38% in the lg document thumbnail. The fallback feels a bit lost in the larger box.

Fix would be one number: bump to h-20 w-20 (80×80, ~32%) or h-24 w-24 (96×96, ~38%, matching the lg ratio). I lean h-24 w-24 — keeps visual weight parity with the lg fallback.

Suggestions (not for this PR)

No visual regression test coverage at 320/768/1440px. The strip is the most prominent card on the dashboard, so a Playwright screenshot diff on future UI changes would catch layout shifts early. Future work.
No a11y test covers the dark-mode rendering of the strip. mix-blend-multiply on paper scans depends on theme — worth a pair of axe-playwright passes (light + dark) next time the dashboard E2E suite grows.

LGTM pending the icon size

The only real ask is the fallback icon scale bump — a single Tailwind class change. Everything else matches DocumentThumbnail's deliberate decisions exactly.

## 🎨 Leonie Voss — UX Designer & Accessibility Advocate **Verdict: ⚠️ Approved with concerns** ### What's right - **180×252, exact 5:7 A4 ratio.** Matches `DocumentThumbnail` everywhere else in the app. The strip no longer looks squatter than the rest of the thumbnail surfaces. ✓ - **Sized container reserves space before the img decodes** (`h-[252px] w-[180px]` on the wrapper). No CLS. ✓ - **`alt=""`** — correct, decorative image, the `<h2>` title + caption already label the card. Screen readers won't double-announce. ✓ - **`dark:mix-blend-multiply`** on the img — paper scans won't glare on a dark page background. ✓ - **`aria-hidden="true"`** on the fallback icon wrapper — decorative-only. ✓ - **`loading="lazy"` + `decoding="async"`** — the img doesn't block first paint. ✓ - **Consistent "no thumbnail" vocabulary** — same heroicon as `DocumentThumbnail.svelte:44-55`. Users learn one shape for one meaning. ✓ ### One concern (non-blocking) **Fallback icon size on the larger container.** The fallback SVG uses `class="h-16 w-16"` (64×64). That's the same size `DocumentThumbnail` uses for its `lg` container (120×168). On the resume strip's 180×252 container — significantly bigger — a 64×64 icon looks visually undersized: it's only ~25% of the height, compared to ~38% in the `lg` document thumbnail. The fallback feels a bit lost in the larger box. Fix would be one number: bump to `h-20 w-20` (80×80, ~32%) or `h-24 w-24` (96×96, ~38%, matching the lg ratio). I lean `h-24 w-24` — keeps visual weight parity with the `lg` fallback. ### Suggestions (not for this PR) - No visual regression test coverage at 320/768/1440px. The strip is the most prominent card on the dashboard, so a Playwright screenshot diff on future UI changes would catch layout shifts early. Future work. - No a11y test covers the dark-mode rendering of the strip. `mix-blend-multiply` on paper scans depends on theme — worth a pair of `axe-playwright` passes (light + dark) next time the dashboard E2E suite grows. ### LGTM pending the icon size The only real ask is the fallback icon scale bump — a single Tailwind class change. Everything else matches `DocumentThumbnail`'s deliberate decisions exactly.

marcel commented

2026-04-23 22:44:35 +02:00

Review Cycle 1 — Changes

Addressed

[@saraholt] "No test covers the Jackson serialisation path itself" — added thumbnailUrl_isSerialisedToJson_soFrontendReceivesIt to DocumentTest.java. Serialises a Document via ObjectMapper and asserts the JSON contains "thumbnailUrl":"...", locking the wire contract (protects against getter rename, @JsonIgnore, visibility drop). Commit 0a683a61.
[@leonievoss] Fallback icon undersized in the 180×252 container — bumped from h-16 w-16 to h-24 w-24 so the icon occupies ~38% of the container height, matching the visual weight ratio of DocumentThumbnail's lg fallback. Commit ac6a7359.

Not addressed (explicitly non-blocking, deferred per the persona's own phrasing)

[@saraholt] E2E / Playwright coverage for the thumbnail render — Sara flagged as "intentional per the issue's test strategy (unit-level coverage only)."
[@saraholt] Implicit empty-state coverage observation — already covered by the dedicated renders fallback icon when thumbnailUrl is null test.
[@felixbrandt] URL path constant extraction between getter and controller — Felix: "Not a blocker — the issue's out-of-scope clause on generalisation applies here too."
[@mkeller] Entity-knows-URL-shape coupling — Markus: "I still agree with [the decision]."
[@tobiwendt] Post-merge checklist to run real npm run generate:api against a rebuilt dev backend — can't execute from this worktree; follow-up action after merge.

Re-running review cycle 2.

## Review Cycle 1 — Changes ### Addressed - **[@saraholt]** "No test covers the Jackson serialisation path itself" — added `thumbnailUrl_isSerialisedToJson_soFrontendReceivesIt` to `DocumentTest.java`. Serialises a `Document` via `ObjectMapper` and asserts the JSON contains `"thumbnailUrl":"..."`, locking the wire contract (protects against getter rename, `@JsonIgnore`, visibility drop). Commit `0a683a61`. - **[@leonievoss]** Fallback icon undersized in the 180×252 container — bumped from `h-16 w-16` to `h-24 w-24` so the icon occupies ~38% of the container height, matching the visual weight ratio of `DocumentThumbnail`'s `lg` fallback. Commit `ac6a7359`. ### Not addressed (explicitly non-blocking, deferred per the persona's own phrasing) - [@saraholt] E2E / Playwright coverage for the thumbnail render — Sara flagged as "intentional per the issue's test strategy (unit-level coverage only)." - [@saraholt] Implicit empty-state coverage observation — already covered by the dedicated `renders fallback icon when thumbnailUrl is null` test. - [@felixbrandt] URL path constant extraction between getter and controller — Felix: "Not a blocker — the issue's out-of-scope clause on generalisation applies here too." - [@mkeller] Entity-knows-URL-shape coupling — Markus: "I still agree with [the decision]." - [@tobiwendt] Post-merge checklist to run real `npm run generate:api` against a rebuilt dev backend — can't execute from this worktree; follow-up action after merge. Re-running review cycle 2.

marcel commented

2026-04-23 22:45:01 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Cycle 2 delta

The new thumbnailUrl_isSerialisedToJson_soFrontendReceivesIt in DocumentTest.java is exactly the shape I'd want: small, pure, no Spring context, asserts on the wire contract rather than the Java method. Comment explains why (getter rename / @JsonIgnore / visibility drop) rather than what, which is the right discipline.
Fallback icon bump (h-16 → h-24) is a single-number change, exactly as Leonie asked.

LGTM

Nothing new from my angle. Ship it.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** ### Cycle 2 delta - The new `thumbnailUrl_isSerialisedToJson_soFrontendReceivesIt` in `DocumentTest.java` is exactly the shape I'd want: small, pure, no Spring context, asserts on the wire contract rather than the Java method. Comment explains **why** (getter rename / @JsonIgnore / visibility drop) rather than what, which is the right discipline. - Fallback icon bump (`h-16 → h-24`) is a single-number change, exactly as Leonie asked. ### LGTM Nothing new from my angle. Ship it.

marcel commented

2026-04-23 22:45:05 +02:00

🏛️ Markus Keller — Senior Application Architect

Verdict: ✅ Approved

Cycle 2 delta

The new serialisation test lives in DocumentTest.java (model package) — right layer. Doesn't pull in Spring context, doesn't cross module boundaries.
Fallback icon change is scoped to the consuming component (DashboardResumeStrip.svelte). No architectural drift.

LGTM

The architectural shape hasn't changed since cycle 1. Merge when the loop clears.

## 🏛️ Markus Keller — Senior Application Architect **Verdict: ✅ Approved** ### Cycle 2 delta - The new serialisation test lives in `DocumentTest.java` (model package) — right layer. Doesn't pull in Spring context, doesn't cross module boundaries. - Fallback icon change is scoped to the consuming component (`DashboardResumeStrip.svelte`). No architectural drift. ### LGTM The architectural shape hasn't changed since cycle 1. Merge when the loop clears.

marcel commented

2026-04-23 22:45:09 +02:00

🔒 Nora Steiner — Application Security Engineer

Verdict: ✅ Approved

Cycle 2 delta

The new Jackson serialisation test is a nice defensive bonus — it also implicitly guards the security-relevant comment on the getter by ensuring the serialised path stays exercised. If someone ever strips the annotation and changes the getter to private (which would legitimately hide the URL), this test would fail loudly.

LGTM

Surface unchanged, no new security concerns. Ship it.

## 🔒 Nora Steiner — Application Security Engineer **Verdict: ✅ Approved** ### Cycle 2 delta - The new Jackson serialisation test is a nice defensive bonus — it also implicitly guards the security-relevant comment on the getter by ensuring the serialised path stays exercised. If someone ever strips the annotation *and* changes the getter to private (which would legitimately hide the URL), this test would fail loudly. ### LGTM Surface unchanged, no new security concerns. Ship it.

marcel commented

2026-04-23 22:45:18 +02:00

🧪 Sara Holt — QA Engineer

Verdict: ✅ Approved

Cycle 2 delta

My #1 concern is addressed. thumbnailUrl_isSerialisedToJson_soFrontendReceivesIt locks the wire contract — ObjectMapper round-trip, asserts the exact JSON fragment. Comment explains what it protects against (getter rename, @JsonIgnore, visibility drop), which is the right anchor for a future maintainer.
The test uses a fresh ObjectMapper with JavaTimeModule rather than pulling in a Spring context — fast, focused, right layer.
I'd have picked the same assertion style (contains("\"thumbnailUrl\":\"...\"") over a full-tree JsonNode walk). Small and blunt, survives field-order changes in Jackson.

Concerns from cycle 1 not actioned (by agreement)

Implicit empty-state coverage — the dedicated test I flagged is already in place.
E2E/Playwright coverage — explicitly deferred per the issue's unit-only strategy.

LGTM

Test pyramid shape is right. Ship it.

## 🧪 Sara Holt — QA Engineer **Verdict: ✅ Approved** ### Cycle 2 delta - My #1 concern is addressed. `thumbnailUrl_isSerialisedToJson_soFrontendReceivesIt` locks the wire contract — `ObjectMapper` round-trip, asserts the exact JSON fragment. Comment explains what it protects against (getter rename, `@JsonIgnore`, visibility drop), which is the right anchor for a future maintainer. - The test uses a fresh `ObjectMapper` with `JavaTimeModule` rather than pulling in a Spring context — fast, focused, right layer. - I'd have picked the same assertion style (`contains("\"thumbnailUrl\":\"...\"")` over a full-tree `JsonNode` walk). Small and blunt, survives field-order changes in Jackson. ### Concerns from cycle 1 not actioned (by agreement) - Implicit empty-state coverage — the dedicated test I flagged is already in place. - E2E/Playwright coverage — explicitly deferred per the issue's unit-only strategy. ### LGTM Test pyramid shape is right. Ship it.

marcel commented

2026-04-23 22:45:20 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

Cycle 2 delta

No infrastructure touched. New test runs in the existing Mockito unit-test job; the frontend change is a single Tailwind class, no CI impact.

LGTM

Zero operational surface change from cycle 1. Ship it.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** ### Cycle 2 delta - No infrastructure touched. New test runs in the existing Mockito unit-test job; the frontend change is a single Tailwind class, no CI impact. ### LGTM Zero operational surface change from cycle 1. Ship it.

marcel commented

2026-04-23 22:45:24 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Advocate

Verdict: ✅ Approved

Cycle 2 delta

Fallback icon at h-24 w-24 (96×96 in a 180×252 container, ~38% of container height) now matches the visual weight ratio of DocumentThumbnail's lg fallback (64×64 in 120×168, ~38%). Consistent across every "no thumbnail yet" surface in the app — users learn one shape at one relative size.

LGTM

Everything else was already correct per cycle 1. Design-wise I'm happy to merge.

## 🎨 Leonie Voss — UX Designer & Accessibility Advocate **Verdict: ✅ Approved** ### Cycle 2 delta - Fallback icon at `h-24 w-24` (96×96 in a 180×252 container, ~38% of container height) now matches the visual weight ratio of `DocumentThumbnail`'s `lg` fallback (64×64 in 120×168, ~38%). Consistent across every "no thumbnail yet" surface in the app — users learn one shape at one relative size. ### LGTM Everything else was already correct per cycle 1. Design-wise I'm happy to merge.

marcel added 12 commits 2026-04-24 07:29:43 +02:00

test(document): getThumbnailUrl returns null when thumbnailKey is null 096f66eb15

First TDD step for centralising the thumbnail URL convention on the
Document entity (#309). Adds a stub getter returning null and a test
that locks the "no key → no URL" branch.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

feat(document): getThumbnailUrl composes /api/documents/{id}/thumbnail when key present df260d5c64

The no-cache-buster branch covers documents whose thumbnail key is set
but whose thumbnailGeneratedAt is still null — which only happens in
the narrow window between the key being persisted and the async worker
stamping the timestamp (#309).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

feat(document): getThumbnailUrl appends URL-encoded timestamp as cache-buster 9862a51ac7

Matches the shape the frontend previously built via
encodeURIComponent(thumbnailGeneratedAt), so the backend is now the
single source of truth for the thumbnail URL convention (#309).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

feat(document): expose thumbnailUrl to JSON serialisation ad999c47ea

@JsonProperty makes the computed getter part of every Document response
Jackson produces, so any DTO returning a Document automatically carries
the thumbnail URL without per-controller plumbing. The accompanying
comment warns future readers that the cache-buster is load-bearing
for the endpoint's `immutable` cache header (CWE-525) (#309).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

feat(dashboard): populate resume thumbnailUrl from Document 510ab1d2d5

DashboardService now reads the URL from the Document's computed getter
instead of passing null, so the resume strip can display the real
thumbnail of whatever the user was last working on (#309).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

chore(api): regenerate Document type with thumbnailUrl field a8b9133b80

Reflects the new @JsonProperty getter on Document. Kept as a minimal
manual edit rather than a full regen because the running dev backend
belongs to the main workspace and swapping JARs there would be a
side effect on a parallel worktree's state. `npm run generate:api`
will converge on the same shape.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

refactor(document-thumbnail): read doc.thumbnailUrl instead of composing locally 817749889a

The backend now exposes thumbnailUrl as a serialised computed property
on Document, so the component drops its dependency on the frontend
URL-builder. PersonDocumentList's inline Doc prop type follows the
same shift (#309).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

refactor(thumbnails): drop frontend URL-builder now that backend owns the convention a02f6cdcd7

The helper had a single consumer (DocumentThumbnail) and its only job
was to compose what the backend's Document.getThumbnailUrl() now
produces. Deleting it locks the single-source-of-truth invariant —
there is no longer a way to build a thumbnail URL on the client (#309).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

feat(dashboard): render real document thumbnail in resume strip 1d44bbb1bd

Replaces the generic parchment SVG placeholder with an <img> pointing at
the backend's thumbnail endpoint when the document has one. The 180×252
container matches DocumentThumbnail's 5:7 A4 convention so the
dashboard tile sits visually next to the list/person-sublist tiles
instead of looking squatter than they do. dark:mix-blend-multiply keeps
paper scans from glaring on a dark page background (#309).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

feat(dashboard): fall back to document-text heroicon when no thumbnail yet 72cd6f5bbc

Uses the same heroicon as DocumentThumbnail so the "no thumbnail yet"
signal reads identically across the app: one shape, one meaning. The
parchment SVG still lives on in the fully-empty state (no resume doc
at all), where it represents a different thing — we removed it only
from the "document exists, thumbnail not generated yet" branch (#309).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

test(document): address @saraholt — lock JSON wire contract for thumbnailUrl 629f0183f7

Prior coverage only exercised getThumbnailUrl() as a Java method call.
The new case serialises via ObjectMapper and asserts the resulting JSON
contains "thumbnailUrl":"..." so we catch silent breakages in the wire
contract (getter rename, @JsonIgnore, visibility drop) — not just
regressions in the method's return value (#309).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

style(dashboard): address @leonievoss — scale fallback icon to match larger container

CI / Unit & Component Tests (pull_request) Failing after 3m26s

Details

CI / OCR Service Tests (pull_request) Successful in 35s

Details

CI / Backend Unit Tests (pull_request) Failing after 3m4s

Details

CI / Unit & Component Tests (push) Failing after 3m7s

Details

CI / OCR Service Tests (push) Successful in 36s

Details

CI / Backend Unit Tests (push) Failing after 3m17s

Details

7007491d8c

h-16 w-16 looked undersized in the 180×252 strip container (~25% of
the height). h-24 w-24 gives ~38% visual weight, matching the ratio
DocumentThumbnail uses for its lg (120×168) fallback (#309).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

marcel force-pushed feat/issue-309-resume-strip-thumbnail from ac6a735918 to 7007491d8c

2026-04-24 07:29:43 +02:00

Compare

marcel commented

2026-04-24 07:29:54 +02:00

Rebased onto latest `main`

Main had advanced 32 commits since this PR opened, adding thumbnailAspect / pageCount columns to Document and a related refactor in the generated API types. The one conflict was in frontend/src/lib/generated/api.ts — main added two optional fields in the same region where this PR added thumbnailUrl?: string. Resolved by keeping both.

All 12 commits replayed cleanly onto origin/main after that single resolution. Backend suite: 1257/1257 green. Frontend spec for DashboardResumeStrip: 7/7 green.

Force-pushed (--force-with-lease). Merge conflict is resolved — no code-review concerns changed.

## Rebased onto latest `main` Main had advanced 32 commits since this PR opened, adding `thumbnailAspect` / `pageCount` columns to `Document` and a related refactor in the generated API types. The one conflict was in `frontend/src/lib/generated/api.ts` — main added two optional fields in the same region where this PR added `thumbnailUrl?: string`. Resolved by keeping both. All 12 commits replayed cleanly onto `origin/main` after that single resolution. Backend suite: 1257/1257 green. Frontend spec for `DashboardResumeStrip`: 7/7 green. Force-pushed (`--force-with-lease`). Merge conflict is resolved — no code-review concerns changed.

marcel merged commit 7007491d8c into main

2026-04-24 07:37:59 +02:00

marcel deleted branch feat/issue-309-resume-strip-thumbnail

2026-04-24 07:38:00 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#314

As a user I want the dashboard resume strip to show the actual document thumbnail so I recognize what I was working on at a glance #314

Summary

Design decisions (from the resolutions comment)

Coverage

Final status

👨‍💻 Felix Brandt — Senior Fullstack Developer

What I checked

Suggestions (non-blocking)

LGTM

🏛️ Markus Keller — Senior Application Architect

Structure

One caveat I want on the record

No ADR needed

LGTM

🔒 Nora Steiner — Application Security Engineer

Surface review

Encoding parity

Audit trail

LGTM

🧪 Sara Holt — QA Engineer

What's solid

Concerns (non-blocking, worth considering)

LGTM overall

⚙️ Tobias Wendt — DevOps & Platform Engineer

What I checked

One operational note

Monitoring suggestion (future work)

LGTM

🎨 Leonie Voss — UX Designer & Accessibility Advocate

What's right

One concern (non-blocking)

Suggestions (not for this PR)

LGTM pending the icon size

Review Cycle 1 — Changes

Addressed

Not addressed (explicitly non-blocking, deferred per the persona's own phrasing)

👨‍💻 Felix Brandt — Senior Fullstack Developer

Cycle 2 delta

LGTM

🏛️ Markus Keller — Senior Application Architect

Cycle 2 delta

LGTM

🔒 Nora Steiner — Application Security Engineer

Cycle 2 delta

LGTM

🧪 Sara Holt — QA Engineer

Cycle 2 delta

Concerns from cycle 1 not actioned (by agreement)

LGTM

⚙️ Tobias Wendt — DevOps & Platform Engineer

Cycle 2 delta

LGTM

🎨 Leonie Voss — UX Designer & Accessibility Advocate

Cycle 2 delta

LGTM

Rebased onto latest main

Rebased onto latest `main`