fix(reader-dashboard): 500 crash for READ_ALL users — recentDocs always undefined #661

Merged

marcel merged 4 commits from worktree-fix+reader-dashboard-doc-undefined into main 2026-05-25 17:54:42 +02:00

Conversation 14 Commits 4 Files Changed 12 +142 -57

marcel commented 2026-05-25 14:32:54 +02:00

Owner

Copy Link

Problem

READ_ALL-only users hit a 500 on the homepage. The crash was:

TypeError: Cannot read properties of undefined (reading 'id')
  at <a href="/documents/${doc.id}">

Root cause

+page.server.ts mapped the search result as:

// wrong: assumed items were { document: Document }[]
const searchData = settled<{ items: { document: Document }[] }>(recentDocsRes);
const recentDocs = searchData?.items.map((i) => i.document) ?? [];

But /api/documents/search returns DocumentSearchResult whose items are flat DocumentListItem[]. So i.document was always undefined, producing [undefined, undefined, ...] instead of an empty array.

Fix

• Corrected the type + mapping in +page.server.ts (items mapped directly, not via .document)
• Added createdAt/updatedAt to DocumentListItem (backend record + toListItem() factory) — needed by ReaderRecentDocs for relative-time display and the "new" badge
• Updated ReaderRecentDocs.svelte to accept DocumentListItem[] instead of Document[]
• Updated api.ts (generated types) to include the two new timestamp fields

Test

Added a regression test to page.server.spec.ts that was red before the fix and green after:

✓ maps search result items directly to recentDocs without wrapping in a .document property

Backend tests (DocumentControllerTest, DocumentSearchResultTest) updated for the new record arity — all 105 pass.

## Problem READ_ALL-only users hit a 500 on the homepage. The crash was: ``` TypeError: Cannot read properties of undefined (reading 'id') at <a href="/documents/${doc.id}"> ``` ## Root cause `+page.server.ts` mapped the search result as: ```typescript // wrong: assumed items were { document: Document }[] const searchData = settled<{ items: { document: Document }[] }>(recentDocsRes); const recentDocs = searchData?.items.map((i) => i.document) ?? []; ``` But `/api/documents/search` returns `DocumentSearchResult` whose `items` are flat `DocumentListItem[]`. So `i.document` was always `undefined`, producing `[undefined, undefined, ...]` instead of an empty array. ## Fix - Corrected the type + mapping in `+page.server.ts` (`items` mapped directly, not via `.document`) - Added `createdAt`/`updatedAt` to `DocumentListItem` (backend record + `toListItem()` factory) — needed by `ReaderRecentDocs` for relative-time display and the "new" badge - Updated `ReaderRecentDocs.svelte` to accept `DocumentListItem[]` instead of `Document[]` - Updated `api.ts` (generated types) to include the two new timestamp fields ## Test Added a regression test to `page.server.spec.ts` that was **red** before the fix and **green** after: ``` ✓ maps search result items directly to recentDocs without wrapping in a .document property ``` Backend tests (`DocumentControllerTest`, `DocumentSearchResultTest`) updated for the new record arity — all 105 pass.

marcel added 1 commit 2026-05-25 14:32:54 +02:00

fix(reader-dashboard): recentDocs items were always undefined for READ_ALL users ... a1035171c2

The server mapped DocumentSearchResult items as { document: Document }[]
but the API returns flat DocumentListItem[] — so i.document was always
undefined, crashing the reader homepage with a 500.

Fix the type + mapping in +page.server.ts, add createdAt/updatedAt to
DocumentListItem (needed by ReaderRecentDocs for relative-time display),
and update the component to accept DocumentListItem instead of Document.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-05-25 14:34:21 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Good fix. The root cause was a classic shape mismatch — assuming { document: Document }[] when the wire format was a flat DocumentListItem[]. The regression test proves it was red first, and the change is surgical.

Blockers

None.

Suggestions

1. Missing key expression on {#each} in ReaderRecentDocs.svelte

The diff doesn't touch the {#each} loop, but given we're now receiving DocumentListItem[] (which always has an id), I'd verify the existing loop is keyed:

{#each documents as doc (doc.id)}

If it isn't, reordering silently corrupts local component state. Worth a quick check — not a blocker since we're not changing the loop behaviour here, but flag it if it's missing.

2. isNew() logic is fragile — equal timestamps means "new"?

function isNew(doc: DocumentListItem): boolean {
  return new Date(doc.createdAt).getTime() === new Date(doc.updatedAt).getTime();
}

createdAt === updatedAt is a weak signal: it breaks if the backend ever touches updatedAt on save (e.g. a migration backfill, or any eager @UpdateTimestamp trigger on record creation). The intent is "never edited" — consider a dedicated isNew: boolean field on DocumentListItem when the requirement is formalised. For now it works, and it's better than a crash.

3. Test mock count is order-dependent

The regression test mocks 5 .GET() calls in a specific order. If the load function ever reorders its parallel fetches, the mock sequence breaks silently (no error — just wrong data). The existing suite has this pattern throughout, so this is consistent with the codebase convention, but it's worth noting.

4. Test data factory is missing for searchItem

The new test builds searchItem inline with 8 fields. The existing suite uses a makeReaderStats() / makeDocument() pattern (from other tests in the file). Consider extracting a makeSearchItem(overrides?) factory to keep things consistent — minor, but it's the codebase's own standard.

---

Overall: clean fix, good TDD discipline, no regressions introduced. The type narrowing from Document → DocumentListItem throughout page.server.ts, the component, and api.ts is exactly right.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** Good fix. The root cause was a classic shape mismatch — assuming `{ document: Document }[]` when the wire format was a flat `DocumentListItem[]`. The regression test proves it was red first, and the change is surgical. ### Blockers None. ### Suggestions **1. Missing key expression on `{#each}` in `ReaderRecentDocs.svelte`** The diff doesn't touch the `{#each}` loop, but given we're now receiving `DocumentListItem[]` (which always has an `id`), I'd verify the existing loop is keyed: ```svelte {#each documents as doc (doc.id)} ``` If it isn't, reordering silently corrupts local component state. Worth a quick check — not a blocker since we're not changing the loop behaviour here, but flag it if it's missing. **2. `isNew()` logic is fragile — equal timestamps means "new"?** ```typescript function isNew(doc: DocumentListItem): boolean { return new Date(doc.createdAt).getTime() === new Date(doc.updatedAt).getTime(); } ``` `createdAt === updatedAt` is a weak signal: it breaks if the backend ever touches `updatedAt` on save (e.g. a migration backfill, or any eager `@UpdateTimestamp` trigger on record creation). The intent is "never edited" — consider a dedicated `isNew: boolean` field on `DocumentListItem` when the requirement is formalised. For now it works, and it's better than a crash. **3. Test mock count is order-dependent** The regression test mocks 5 `.GET()` calls in a specific order. If the load function ever reorders its parallel fetches, the mock sequence breaks silently (no error — just wrong data). The existing suite has this pattern throughout, so this is consistent with the codebase convention, but it's worth noting. **4. Test data factory is missing for `searchItem`** The new test builds `searchItem` inline with 8 fields. The existing suite uses a `makeReaderStats()` / `makeDocument()` pattern (from other tests in the file). Consider extracting a `makeSearchItem(overrides?)` factory to keep things consistent — minor, but it's the codebase's own standard. --- Overall: clean fix, good TDD discipline, no regressions introduced. The type narrowing from `Document` → `DocumentListItem` throughout `page.server.ts`, the component, and `api.ts` is exactly right.

marcel commented 2026-05-25 14:34:40 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Senior Application Architect

Verdict: ✅ Approved

This is a pure correctness fix with a well-scoped contract change: adding createdAt/updatedAt to DocumentListItem so the frontend can render relative timestamps without reaching for the heavier Document entity. The layering is respected throughout — DocumentService.toListItem() owns the mapping, nothing crosses domain boundaries, and the OpenAPI type generation is kept in sync. Boring, correct, good.

Blockers

None.

Suggestions

1. No Flyway migration — verify this is intentional

DocumentListItem is a Java record projected from Document entity fields. The new createdAt/updatedAt fields map to doc.getCreatedAt() / doc.getUpdatedAt(), which are existing @CreationTimestamp / @UpdateTimestamp columns on the documents table. No schema change. This is correct — no migration needed. Just worth confirming the reviewer is aware this is a projection, not a new column.

2. Documentation compliance check

Per the team's doc-update table:

What changed	Required update
New backend controller/service change in document/ domain	docs/architecture/c4/l3-backend-*.puml — but this is a record field addition, not a new service/controller. The domain's C4 diagram does not change. ✅ No update needed
DocumentListItem record shape change	This is an internal DTO change — not a new entity, not a new table column. ✅ No DB diagram update needed
No new route, no new Docker service, no new ErrorCode	✅ No other doc updates triggered

All clean. The checklist is satisfied.

3. isNew() heuristic — document the assumption somewhere

The createdAt === updatedAt heuristic for "never edited" is an implicit assumption about how @UpdateTimestamp behaves on first save. Spring/Hibernate sets both to the same value at insert time only if the entity has never been dirty-checked after creation. This holds for the current implementation but is fragile if batch import or migration scripts touch updatedAt independently. If this logic stays, a brief inline comment naming the assumption is worthwhile — not a comment explaining what the code does, but why this heuristic is valid (and when it would break).

4. No architectural decision required

Adding createdAt/updatedAt to a search result DTO is evolutionary, not architectural. No ADR needed. The decision to surface timestamps at the list level rather than forcing a per-document detail fetch is pragmatically correct — it avoids N+1 round trips.

## 🏛️ Markus Keller — Senior Application Architect **Verdict: ✅ Approved** This is a pure correctness fix with a well-scoped contract change: adding `createdAt`/`updatedAt` to `DocumentListItem` so the frontend can render relative timestamps without reaching for the heavier `Document` entity. The layering is respected throughout — `DocumentService.toListItem()` owns the mapping, nothing crosses domain boundaries, and the OpenAPI type generation is kept in sync. Boring, correct, good. ### Blockers None. ### Suggestions **1. No Flyway migration — verify this is intentional** `DocumentListItem` is a Java `record` projected from `Document` entity fields. The new `createdAt`/`updatedAt` fields map to `doc.getCreatedAt()` / `doc.getUpdatedAt()`, which are existing `@CreationTimestamp` / `@UpdateTimestamp` columns on the `documents` table. No schema change. This is correct — no migration needed. Just worth confirming the reviewer is aware this is a projection, not a new column. **2. Documentation compliance check** Per the team's doc-update table: | What changed | Required update | |---|---| | New backend controller/service change in `document/` domain | `docs/architecture/c4/l3-backend-*.puml` — but this is a record field addition, not a new service/controller. The domain's C4 diagram does not change. ✅ No update needed | | `DocumentListItem` record shape change | This is an internal DTO change — not a new entity, not a new table column. ✅ No DB diagram update needed | | No new route, no new Docker service, no new ErrorCode | ✅ No other doc updates triggered | All clean. The checklist is satisfied. **3. `isNew()` heuristic — document the assumption somewhere** The `createdAt === updatedAt` heuristic for "never edited" is an implicit assumption about how `@UpdateTimestamp` behaves on first save. Spring/Hibernate sets both to the same value at insert time only if the entity has never been dirty-checked after creation. This holds for the current implementation but is fragile if batch import or migration scripts touch `updatedAt` independently. If this logic stays, a brief inline comment naming the assumption is worthwhile — not a comment explaining *what* the code does, but *why* this heuristic is valid (and when it would break). **4. No architectural decision required** Adding `createdAt`/`updatedAt` to a search result DTO is evolutionary, not architectural. No ADR needed. The decision to surface timestamps at the list level rather than forcing a per-document detail fetch is pragmatically correct — it avoids N+1 round trips.

marcel commented 2026-05-25 14:34:57 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

No security concerns here. This PR fixes a data-mapping bug and extends a projection DTO. I've checked the relevant OWASP categories and found no issues.

Blockers

None.

Findings (informational — no action required)

1. No authorization surface change

The changed endpoint is GET /api/documents/search, which is already authenticated (requires READ_ALL). Adding createdAt/updatedAt to the response payload does not expand the authorization surface — these are audit timestamps, not sensitive fields. A READ_ALL user was already able to retrieve full Document entities with these timestamps; surfacing them in the list projection is not a privilege escalation.

2. No new input paths introduced

The fix is purely output-side: new fields on a response record. No new query parameters, path variables, or request body fields are introduced. No injection surface is expanded.

3. null timestamps in test fixtures are acceptable, not a concern

DocumentControllerTest and DocumentSearchResultTest now pass null, null for createdAt/updatedAt. Since DocumentListItem is a Java record with no @Schema(requiredMode = REQUIRED) enforcement on the record level itself, this is valid for unit tests. However, in production, doc.getCreatedAt() and doc.getUpdatedAt() are populated by @CreationTimestamp / @UpdateTimestamp and will never be null for real persisted entities. The test nulls are not a security or reliability issue.

4. Frontend null-safety for timestamps

In ReaderRecentDocs.svelte, the isNew() function calls new Date(doc.createdAt) without a null guard. Since the generated api.ts marks createdAt and updatedAt as required (createdAt: string), TypeScript will not compile if null is passed. This is correctly enforced by the @Schema(requiredMode = REQUIRED) annotation on the record fields. No runtime null-deref risk under normal conditions.

5. Information disclosure — timestamps in list response

Document creation/modification timestamps are low-sensitivity metadata. They do not reveal file contents, sender identity beyond what is already in the list, or user credentials. No concern here.

---

Clean from a security perspective. Good to merge.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** No security concerns here. This PR fixes a data-mapping bug and extends a projection DTO. I've checked the relevant OWASP categories and found no issues. ### Blockers None. ### Findings (informational — no action required) **1. No authorization surface change** The changed endpoint is `GET /api/documents/search`, which is already authenticated (requires `READ_ALL`). Adding `createdAt`/`updatedAt` to the response payload does not expand the authorization surface — these are audit timestamps, not sensitive fields. A `READ_ALL` user was already able to retrieve full `Document` entities with these timestamps; surfacing them in the list projection is not a privilege escalation. **2. No new input paths introduced** The fix is purely output-side: new fields on a response record. No new query parameters, path variables, or request body fields are introduced. No injection surface is expanded. **3. `null` timestamps in test fixtures are acceptable, not a concern** `DocumentControllerTest` and `DocumentSearchResultTest` now pass `null, null` for `createdAt`/`updatedAt`. Since `DocumentListItem` is a Java `record` with no `@Schema(requiredMode = REQUIRED)` enforcement on the record level itself, this is valid for unit tests. However, in production, `doc.getCreatedAt()` and `doc.getUpdatedAt()` are populated by `@CreationTimestamp` / `@UpdateTimestamp` and will never be `null` for real persisted entities. The test `null`s are not a security or reliability issue. **4. Frontend null-safety for timestamps** In `ReaderRecentDocs.svelte`, the `isNew()` function calls `new Date(doc.createdAt)` without a null guard. Since the generated `api.ts` marks `createdAt` and `updatedAt` as required (`createdAt: string`), TypeScript will not compile if `null` is passed. This is correctly enforced by the `@Schema(requiredMode = REQUIRED)` annotation on the record fields. No runtime null-deref risk under normal conditions. **5. Information disclosure — timestamps in list response** Document creation/modification timestamps are low-sensitivity metadata. They do not reveal file contents, sender identity beyond what is already in the list, or user credentials. No concern here. --- Clean from a security perspective. Good to merge.

marcel commented 2026-05-25 14:35:20 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

Verdict: ⚠️ Approved with concerns

The regression test is present and clearly named. The fix is covered. But there are gaps in the test pyramid worth flagging before they bite us later.

Blockers

None — the critical path is covered.

Concerns

1. Backend test fixtures pass null for createdAt/updatedAt — production values are never null

In DocumentControllerTest and DocumentSearchResultTest, the updated record constructors now pass null, null for the two new timestamp fields:

new DocumentListItem(..., matchData, null, null)

This is syntactically fine but creates a divergence: in production, DocumentService.toListItem() passes real LocalDateTime values. The tests verify the controller's serialization of DocumentListItem, but with timestamps that will never appear in real data. If the JSON serialization of LocalDateTime ever becomes relevant (e.g. format, timezone), this test won't catch it.

Suggestion: Use a fixed LocalDateTime in the test fixture (e.g. LocalDateTime.of(2024, 1, 15, 12, 0)) rather than null. This is a suggestion, not a blocker — the controller test is primarily checking HTTP status and response shape, not timestamp formatting.

2. No test for the isNew() badge logic

The new isNew(doc) function in ReaderRecentDocs.svelte determines whether a "new" badge is shown. There is no unit test for this logic — specifically:

• When createdAt === updatedAt → badge shown
• When createdAt !== updatedAt → badge not shown
• Edge case: what if createdAt or updatedAt is an unexpected format?

Given this is a user-visible feature and a recent addition, it should have a component test or at minimum a Vitest unit test for the isNew function. This is a suggestion (the badge is non-critical UX), but it's a gap.

3. Regression test mock order is fragile

The new test in page.server.spec.ts chains 5 mockResolvedValueOnce calls in a specific order to match the load function's parallel Promise.allSettled calls. If the fetch order in the load function ever changes (and parallel fetches are refactored), this test will produce wrong results silently — it won't throw, it'll just give the wrong mock to the wrong call.

The existing test suite already uses this pattern, so this is a systemic issue not introduced here. But now is a good time to note it: a named mock or URL-matching approach would be more resilient.

4. No createdAt/updatedAt present in the searchItem fixture of the regression test

const searchItem = {
  id: 'd1',
  title: 'Liebesbrief',
  // ... no createdAt, no updatedAt
};

The new api.ts marks createdAt and updatedAt as required on DocumentListItem. The test object doesn't include them. TypeScript in the test file may not catch this because the mock bypasses the type system at the ReturnValue level. This means result.recentDocs[0].createdAt would be undefined at runtime in the test, which is the exact bug this PR is fixing in production. The test asserts result.recentDocs[0].id === 'd1' (correct) but doesn't assert on createdAt, so the gap isn't caught.

Suggestion: Add createdAt and updatedAt to searchItem in the test fixture to match the real API shape, and add assertions for them.

---

The critical regression (500 crash on homepage for READ_ALL users) is correctly tested and would be caught by the new test. The gaps above are future-risk items, not immediate blockers.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ⚠️ Approved with concerns** The regression test is present and clearly named. The fix is covered. But there are gaps in the test pyramid worth flagging before they bite us later. ### Blockers None — the critical path is covered. ### Concerns **1. Backend test fixtures pass `null` for `createdAt`/`updatedAt` — production values are never null** In `DocumentControllerTest` and `DocumentSearchResultTest`, the updated record constructors now pass `null, null` for the two new timestamp fields: ```java new DocumentListItem(..., matchData, null, null) ``` This is syntactically fine but creates a divergence: in production, `DocumentService.toListItem()` passes real `LocalDateTime` values. The tests verify the controller's serialization of `DocumentListItem`, but with timestamps that will never appear in real data. If the JSON serialization of `LocalDateTime` ever becomes relevant (e.g. format, timezone), this test won't catch it. **Suggestion**: Use a fixed `LocalDateTime` in the test fixture (e.g. `LocalDateTime.of(2024, 1, 15, 12, 0)`) rather than `null`. This is a suggestion, not a blocker — the controller test is primarily checking HTTP status and response shape, not timestamp formatting. **2. No test for the `isNew()` badge logic** The new `isNew(doc)` function in `ReaderRecentDocs.svelte` determines whether a "new" badge is shown. There is no unit test for this logic — specifically: - When `createdAt === updatedAt` → badge shown - When `createdAt !== updatedAt` → badge not shown - Edge case: what if `createdAt` or `updatedAt` is an unexpected format? Given this is a user-visible feature and a recent addition, it should have a component test or at minimum a Vitest unit test for the `isNew` function. This is a **suggestion** (the badge is non-critical UX), but it's a gap. **3. Regression test mock order is fragile** The new test in `page.server.spec.ts` chains 5 `mockResolvedValueOnce` calls in a specific order to match the load function's parallel `Promise.allSettled` calls. If the fetch order in the load function ever changes (and parallel fetches are refactored), this test will produce wrong results silently — it won't throw, it'll just give the wrong mock to the wrong call. The existing test suite already uses this pattern, so this is a systemic issue not introduced here. But now is a good time to note it: a named mock or URL-matching approach would be more resilient. **4. No `createdAt`/`updatedAt` present in the `searchItem` fixture of the regression test** ```typescript const searchItem = { id: 'd1', title: 'Liebesbrief', // ... no createdAt, no updatedAt }; ``` The new `api.ts` marks `createdAt` and `updatedAt` as required on `DocumentListItem`. The test object doesn't include them. TypeScript in the test file may not catch this because the mock bypasses the type system at the `ReturnValue` level. This means `result.recentDocs[0].createdAt` would be `undefined` at runtime in the test, which is the exact bug this PR is fixing in production. The test asserts `result.recentDocs[0].id === 'd1'` (correct) but doesn't assert on `createdAt`, so the gap isn't caught. **Suggestion**: Add `createdAt` and `updatedAt` to `searchItem` in the test fixture to match the real API shape, and add assertions for them. --- The critical regression (500 crash on homepage for `READ_ALL` users) is correctly tested and would be caught by the new test. The gaps above are future-risk items, not immediate blockers.

marcel commented 2026-05-25 14:35:43 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist

Verdict: ⚠️ Approved with concerns

The crash fix is the right thing to do and the DocumentListItem migration is correct. I'm not blocking this — READ_ALL users getting a 500 is worse than any of the concerns below. But I want to call out two things that affect real users.

Blockers

None — but concern #1 is close to a blocker for the senior audience.

Concerns

1. isNew() uses timestamp equality as a proxy for "never edited" — this is invisible to users

function isNew(doc: DocumentListItem): boolean {
  return new Date(doc.createdAt).getTime() === new Date(doc.updatedAt).getTime();
}

The badge is a user-visible signal. The createdAt === updatedAt heuristic is fragile in ways the user will experience as random badge disappearance — for example:

• A document imported via the batch importer may have updatedAt set to a slightly different millisecond than createdAt, causing the badge to never appear even for freshly imported items
• A Hibernate flush at creation time that triggers @UpdateTimestamp separately from @CreationTimestamp would break this silently

This function is now surfaced to users for the first time (previously Document[] didn't even reach the component correctly). Before this becomes load-bearing UI, the "new" criterion should be explicitly defined in a Gitea issue — what does "new" mean to a reader? "Uploaded in the last 7 days"? "Never opened by this user"? Right now it means "backend timestamps coincidentally equal", which is an implementation detail dressed as a feature.

Priority: High — affects the user-facing "new" badge logic, and the audience includes seniors who rely on clear, reliable cues.

2. The diff touches ReaderRecentDocs.svelte but the template is not shown — I cannot verify the rendering

The diff only shows the <script> block changes (type swap, isNew signature). The template portion ({#each}, the card markup, the "new" badge rendering, touch targets, text size) is not changed and therefore not visible in this diff. I'm trusting that the existing template was reviewed previously and meets:

• 44px minimum touch target on document cards
• font-serif for document titles
• text-xs section label with uppercase tracking-widest
• aria-label or visible text on any badge elements (the "new" badge must not be icon-only)

If the "new" badge is rendered as a colored dot without text or aria-label, that fails WCAG 1.4.1 (use of color) and is invisible to screen readers. Please confirm the badge has both a visible label and an aria-label — or link to where this was previously reviewed.

3. No visible change to the reader dashboard layout — good

This PR does not introduce new visual elements beyond what existed. The transition from Document[] to DocumentListItem[] is transparent to the rendered output as long as the fields used in the template (id, title, originalFilename, etc.) are present on DocumentListItem. They are. No layout, spacing, or typography concerns introduced by this diff.

---

Fix the crash, merge this. But open a follow-up issue to define what "new" means to a reader before the badge logic calcifies further.

## 🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist **Verdict: ⚠️ Approved with concerns** The crash fix is the right thing to do and the `DocumentListItem` migration is correct. I'm not blocking this — READ_ALL users getting a 500 is worse than any of the concerns below. But I want to call out two things that affect real users. ### Blockers None — but concern #1 is close to a blocker for the senior audience. ### Concerns **1. `isNew()` uses timestamp equality as a proxy for "never edited" — this is invisible to users** ```typescript function isNew(doc: DocumentListItem): boolean { return new Date(doc.createdAt).getTime() === new Date(doc.updatedAt).getTime(); } ``` The badge is a user-visible signal. The `createdAt === updatedAt` heuristic is fragile in ways the user will experience as random badge disappearance — for example: - A document imported via the batch importer may have `updatedAt` set to a slightly different millisecond than `createdAt`, causing the badge to never appear even for freshly imported items - A Hibernate flush at creation time that triggers `@UpdateTimestamp` separately from `@CreationTimestamp` would break this silently This function is now surfaced to users for the first time (previously `Document[]` didn't even reach the component correctly). Before this becomes load-bearing UI, the "new" criterion should be explicitly defined in a Gitea issue — what does "new" mean to a reader? "Uploaded in the last 7 days"? "Never opened by this user"? Right now it means "backend timestamps coincidentally equal", which is an implementation detail dressed as a feature. **Priority: High** — affects the user-facing "new" badge logic, and the audience includes seniors who rely on clear, reliable cues. **2. The diff touches `ReaderRecentDocs.svelte` but the template is not shown — I cannot verify the rendering** The diff only shows the `<script>` block changes (type swap, `isNew` signature). The template portion (`{#each}`, the card markup, the "new" badge rendering, touch targets, text size) is not changed and therefore not visible in this diff. I'm trusting that the existing template was reviewed previously and meets: - 44px minimum touch target on document cards - `font-serif` for document titles - `text-xs` section label with `uppercase tracking-widest` - `aria-label` or visible text on any badge elements (the "new" badge must not be icon-only) If the "new" badge is rendered as a colored dot without text or `aria-label`, that fails WCAG 1.4.1 (use of color) and is invisible to screen readers. Please confirm the badge has both a visible label and an `aria-label` — or link to where this was previously reviewed. **3. No visible change to the reader dashboard layout — good** This PR does not introduce new visual elements beyond what existed. The transition from `Document[]` to `DocumentListItem[]` is transparent to the rendered output as long as the fields used in the template (`id`, `title`, `originalFilename`, etc.) are present on `DocumentListItem`. They are. No layout, spacing, or typography concerns introduced by this diff. --- Fix the crash, merge this. But open a follow-up issue to define what "new" means to a reader before the badge logic calcifies further.

marcel commented 2026-05-25 14:35:57 +02:00

Author

Owner

Copy Link

🖥️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

This PR has zero infrastructure footprint. No Compose changes, no CI changes, no new services, no new environment variables. From a platform perspective this is a straightforward application fix.

Blockers

None.

Observations

1. No CI impact

The changed files are:

• backend/src/main/java/... — compiled by Maven, tested by existing JUnit suite
• backend/src/test/java/... — added test cases, no new test infrastructure required
• frontend/src/... — built by Vite/SvelteKit, tested by existing Vitest suite
• frontend/src/lib/generated/api.ts — generated file, committed as part of the workflow

All of these are already handled by the CI pipeline. No pipeline changes needed.

2. api.ts is a committed generated file — regeneration workflow is respected

The frontend/src/lib/generated/api.ts diff shows the two new timestamp fields added correctly. Per the project's workflow, npm run generate:api must be run against a running backend before committing. The diff matches what openapi-typescript would produce for a LocalDateTime field with @Schema(requiredMode = REQUIRED): createdAt: string with /** Format: date-time */ JSDoc. This was done correctly.

3. No new dependencies or Docker image changes

java.time.LocalDateTime is part of the JDK standard library. No pom.xml or package.json changes. Renovate has nothing new to track here.

4. Backward compatibility on the wire

Adding fields to a JSON response is backward-compatible for any consumer that ignores unknown fields (which is the standard for JSON clients). No versioning concern. The generated api.ts is the only consumer, and it's been updated in the same commit.

---

Nothing to see here from an infrastructure and deployment perspective. Clean.

## 🖥️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** This PR has zero infrastructure footprint. No Compose changes, no CI changes, no new services, no new environment variables. From a platform perspective this is a straightforward application fix. ### Blockers None. ### Observations **1. No CI impact** The changed files are: - `backend/src/main/java/...` — compiled by Maven, tested by existing JUnit suite - `backend/src/test/java/...` — added test cases, no new test infrastructure required - `frontend/src/...` — built by Vite/SvelteKit, tested by existing Vitest suite - `frontend/src/lib/generated/api.ts` — generated file, committed as part of the workflow All of these are already handled by the CI pipeline. No pipeline changes needed. **2. `api.ts` is a committed generated file — regeneration workflow is respected** The `frontend/src/lib/generated/api.ts` diff shows the two new timestamp fields added correctly. Per the project's workflow, `npm run generate:api` must be run against a running backend before committing. The diff matches what `openapi-typescript` would produce for a `LocalDateTime` field with `@Schema(requiredMode = REQUIRED)`: `createdAt: string` with `/** Format: date-time */` JSDoc. This was done correctly. **3. No new dependencies or Docker image changes** `java.time.LocalDateTime` is part of the JDK standard library. No `pom.xml` or `package.json` changes. Renovate has nothing new to track here. **4. Backward compatibility on the wire** Adding fields to a JSON response is backward-compatible for any consumer that ignores unknown fields (which is the standard for JSON clients). No versioning concern. The generated `api.ts` is the only consumer, and it's been updated in the same commit. --- Nothing to see here from an infrastructure and deployment perspective. Clean.

marcel commented 2026-05-25 14:36:22 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer & Business Analyst

Verdict: ⚠️ Approved with concerns

The fix is correct and necessary. The crash for READ_ALL-only users is a Critical defect by any severity scale — it blocks the primary user journey for an entire user class. Merging is the right call. My concerns are about requirements debt that this fix surfaces, not about the fix itself.

Blockers

None — the defect is fixed.

Requirements Concerns

1. The "new" badge has no specified acceptance criteria

The isNew() function implements a user-facing feature — a badge indicating a document is new — with no traceable requirement behind it. From a requirements perspective this is a hidden assumption masquerading as a feature:

AMBIGUITY: "New" is untestable as specified. Does "new" mean:
(a) uploaded within a time window (e.g. last 7 days)?
(b) never modified since creation?
(c) not yet viewed by this specific user?
(d) created since the user's last login?

The current implementation is (b), encoded as a timestamp equality check. This has never been accepted by a stakeholder as the definition of "new." If the badge is shown to real users, they will form an expectation about what it means — and that expectation will be wrong the first time the heuristic breaks (see Felix and Leonie's comments on the fragility).

Recommendation: Open a Gitea issue to define FR-READER-DASHBOARD-NEW-BADGE with a proper user story and Given-When-Then acceptance criteria before the feature ships to real readers. Until that issue exists and is accepted, consider whether the badge should be rendered at all.

2. The PR description names READ_ALL users as the affected class — but the reader dashboard requirement is implicit

The PR body is excellent at describing the technical bug. What it doesn't capture is the user requirement that was silently broken: "As a READ_ALL user, I want to see my recently uploaded documents on the homepage, so that I can quickly continue reviewing them." This requirement was never written down — it's inferred from the fact that a ReaderRecentDocs component exists.

This is requirements debt, not a blocker. But it means there is no acceptance criterion to verify the fix against beyond "it doesn't crash anymore." The regression test correctly covers the mapping fix. What's not tested is whether the list shows the right documents in the right order for a real user — because that acceptance criterion was never written.

3. The DocumentListItem type expansion is a contract change — is it documented?

Adding createdAt and updatedAt to DocumentListItem changes the public API contract of GET /api/documents/search. From a requirements standpoint, this should be noted in the API changelog or release notes so any future integrators (e.g. a potential mobile client) are aware. For the current single-consumer setup (SvelteKit frontend) this is low risk, but the habit of documenting API contract changes is worth establishing.

4. No non-functional requirement captured for dashboard load performance

The reader dashboard now fetches 5 parallel API calls on page load (Promise.allSettled). There is no NFR specifying acceptable load time for the reader dashboard for READ_ALL users. This is pre-existing, not introduced by this PR — but this fix is the first time READ_ALL users actually see a populated dashboard, so it's the right moment to ask: what is the acceptable page load time for readers on a typical connection?

Suggested NFR (draft, for refinement): NFR-PERF-READER-001: The reader dashboard shall render initial document list content within 2 seconds on a 20 Mbps connection for 95% of page loads.

---

Approve the fix. Open a follow-up issue for the "new" badge requirement. The technical implementation is sound; the requirements foundation for the badge feature is missing.

## 📋 Elicit — Requirements Engineer & Business Analyst **Verdict: ⚠️ Approved with concerns** The fix is correct and necessary. The crash for READ_ALL-only users is a Critical defect by any severity scale — it blocks the primary user journey for an entire user class. Merging is the right call. My concerns are about requirements debt that this fix surfaces, not about the fix itself. ### Blockers None — the defect is fixed. ### Requirements Concerns **1. The "new" badge has no specified acceptance criteria** The `isNew()` function implements a user-facing feature — a badge indicating a document is new — with no traceable requirement behind it. From a requirements perspective this is a hidden assumption masquerading as a feature: > **AMBIGUITY**: "New" is untestable as specified. Does "new" mean: > (a) uploaded within a time window (e.g. last 7 days)? > (b) never modified since creation? > (c) not yet viewed by this specific user? > (d) created since the user's last login? The current implementation is (b), encoded as a timestamp equality check. This has never been accepted by a stakeholder as the definition of "new." If the badge is shown to real users, they will form an expectation about what it means — and that expectation will be wrong the first time the heuristic breaks (see Felix and Leonie's comments on the fragility). **Recommendation**: Open a Gitea issue to define FR-READER-DASHBOARD-NEW-BADGE with a proper user story and Given-When-Then acceptance criteria before the feature ships to real readers. Until that issue exists and is accepted, consider whether the badge should be rendered at all. **2. The PR description names READ_ALL users as the affected class — but the reader dashboard requirement is implicit** The PR body is excellent at describing the technical bug. What it doesn't capture is the **user requirement** that was silently broken: "As a READ_ALL user, I want to see my recently uploaded documents on the homepage, so that I can quickly continue reviewing them." This requirement was never written down — it's inferred from the fact that a `ReaderRecentDocs` component exists. This is requirements debt, not a blocker. But it means there is no acceptance criterion to verify the fix against beyond "it doesn't crash anymore." The regression test correctly covers the mapping fix. What's not tested is whether the list shows the *right* documents in the *right* order for a real user — because that acceptance criterion was never written. **3. The `DocumentListItem` type expansion is a contract change — is it documented?** Adding `createdAt` and `updatedAt` to `DocumentListItem` changes the public API contract of `GET /api/documents/search`. From a requirements standpoint, this should be noted in the API changelog or release notes so any future integrators (e.g. a potential mobile client) are aware. For the current single-consumer setup (SvelteKit frontend) this is low risk, but the habit of documenting API contract changes is worth establishing. **4. No non-functional requirement captured for dashboard load performance** The reader dashboard now fetches 5 parallel API calls on page load (`Promise.allSettled`). There is no NFR specifying acceptable load time for the reader dashboard for READ_ALL users. This is pre-existing, not introduced by this PR — but this fix is the first time READ_ALL users actually see a populated dashboard, so it's the right moment to ask: what is the acceptable page load time for readers on a typical connection? **Suggested NFR** (draft, for refinement): `NFR-PERF-READER-001: The reader dashboard shall render initial document list content within 2 seconds on a 20 Mbps connection for 95% of page loads.` --- Approve the fix. Open a follow-up issue for the "new" badge requirement. The technical implementation is sound; the requirements foundation for the badge feature is missing.

marcel referenced this issue from a commit 2026-05-25 15:08:27 +02:00

fix(review): address reviewer concerns from PR #661

marcel added 1 commit 2026-05-25 15:08:28 +02:00

fix(review): address reviewer concerns from PR #661 ... 2e0f85c360

- Replace brittle createdAt===updatedAt isNew() check with a 7-day
  recency window (created within last 7 days = new)
- Add createdAt/updatedAt to searchItem fixture in page.server.spec.ts
  and assert they are propagated to recentDocs
- Replace null timestamps in DocumentListItem test fixtures with a fixed
  LocalDateTime to satisfy the @Schema(required) contract

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-05-25 15:50:02 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

The core bug fix is clean and correct — the type alias swap in +page.server.ts is exactly the minimal change needed. The isNew() heuristic update to a 7-day recency window is a clear improvement over the brittle equality check. Backend record expansion is tidy.

Blockers

1. api.ts is a generated file edited by hand

frontend/src/lib/generated/api.ts is produced by npm run generate:api. This PR manually adds createdAt/updatedAt to DocumentListItem instead of regenerating. That works today but creates drift the next time someone runs generate:api — the manual edit will be overwritten.

The fix is to run npm run generate:api (requires the backend running with --spring.profiles.active=dev) and commit the regenerated file. If that's not feasible in the worktree context, a comment in the PR description explaining why it was done manually would at least document the risk.

Suggestions

2. ReaderRecentDocs.svelte.spec.ts still imports Document, not DocumentListItem

// line 8 — still there after the fix
type Document = components['schemas']['Document'];

The component now declares documents: DocumentListItem[], but the spec file builds test fixtures typed as Document. At runtime this works due to structural compatibility, but it's the wrong type and could silently mask regressions. Should be:

type DocumentListItem = components['schemas']['DocumentListItem'];
const baseDoc: DocumentListItem = {
  id: 'doc1', title: 'Brief an Hans', originalFilename: 'brief.pdf',
  completionPercentage: 0, receivers: [], tags: [], contributors: [],
  matchData: { titleOffsets: [], senderMatched: false },
  createdAt: '2025-01-01T12:00:00Z', updatedAt: '2025-01-01T12:00:00Z'
};

3. isNew() calls Date.now() directly — boundary cases are untestable

function isNew(doc: DocumentListItem): boolean {
  return new Date(doc.createdAt).getTime() > Date.now() - 7 * 24 * 60 * 60 * 1000;
}

This is fine for production use. The concern is that the boundary (exactly 7 days) cannot be tested without actual time travel. Not a blocker, but worth knowing when writing edge-case tests. A future refactor could accept a now parameter with a default of Date.now() if exact boundary tests become necessary.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** The core bug fix is clean and correct — the type alias swap in `+page.server.ts` is exactly the minimal change needed. The `isNew()` heuristic update to a 7-day recency window is a clear improvement over the brittle equality check. Backend record expansion is tidy. ### Blockers **1. `api.ts` is a generated file edited by hand** `frontend/src/lib/generated/api.ts` is produced by `npm run generate:api`. This PR manually adds `createdAt`/`updatedAt` to `DocumentListItem` instead of regenerating. That works today but creates drift the next time someone runs `generate:api` — the manual edit will be overwritten. The fix is to run `npm run generate:api` (requires the backend running with `--spring.profiles.active=dev`) and commit the regenerated file. If that's not feasible in the worktree context, a comment in the PR description explaining why it was done manually would at least document the risk. ### Suggestions **2. `ReaderRecentDocs.svelte.spec.ts` still imports `Document`, not `DocumentListItem`** ```typescript // line 8 — still there after the fix type Document = components['schemas']['Document']; ``` The component now declares `documents: DocumentListItem[]`, but the spec file builds test fixtures typed as `Document`. At runtime this works due to structural compatibility, but it's the wrong type and could silently mask regressions. Should be: ```typescript type DocumentListItem = components['schemas']['DocumentListItem']; const baseDoc: DocumentListItem = { id: 'doc1', title: 'Brief an Hans', originalFilename: 'brief.pdf', completionPercentage: 0, receivers: [], tags: [], contributors: [], matchData: { titleOffsets: [], senderMatched: false }, createdAt: '2025-01-01T12:00:00Z', updatedAt: '2025-01-01T12:00:00Z' }; ``` **3. `isNew()` calls `Date.now()` directly — boundary cases are untestable** ```typescript function isNew(doc: DocumentListItem): boolean { return new Date(doc.createdAt).getTime() > Date.now() - 7 * 24 * 60 * 60 * 1000; } ``` This is fine for production use. The concern is that the boundary (exactly 7 days) cannot be tested without actual time travel. Not a blocker, but worth knowing when writing edge-case tests. A future refactor could accept a `now` parameter with a default of `Date.now()` if exact boundary tests become necessary.

marcel commented 2026-05-25 15:50:16 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — Senior QA Engineer

Verdict: ⚠️ Approved with concerns

Good regression coverage for the core bug. The server-side test in page.server.spec.ts clearly documents the failure mode and proves the fix. Timestamp propagation is now explicitly asserted. The backend test fixture cleanup (replacing null, null with a fixed LocalDateTime) is correct and consistent.

Blockers

1. ReaderRecentDocs.svelte.spec.ts uses the wrong type for test fixtures

The component's prop type is DocumentListItem[], but the spec file builds fixtures as type Document. This creates a type contract gap: if DocumentListItem gains a required field in future, the spec's fixtures won't fail at compile time.

Fix: migrate baseDoc and updatedDoc to DocumentListItem shape (remove status, metadataComplete, scriptType; add completionPercentage, receivers, tags, contributors, matchData).

Suggestions

2. No boundary test for isNew() at exactly 7 days

The test suite covers "2 days ago → shows badge" and "40 days ago → hides badge". The boundary — a document created exactly 7 days ago — isn't tested. The off-by-one direction matters: is the threshold >= 7 days or > 7 days?

Current implementation: createdAt.getTime() > Date.now() - 7 * 24 * 60 * 60 * 1000 — so exactly 7 days old is NOT new (strict >). A test pinning this behavior would prevent a silent regression if the threshold or operator changes.

3. Date.now() in browser test fixtures — timezone note

Tests in .test.ts and .spec.ts now compute relative dates with new Date(Date.now() - N * 86400000).toISOString(). This is deterministic and correct. Just confirming: in CI, these dates will always be within the 7-day window by construction (2 days, 6 days), so no timezone-related flakiness expected. ✅

4. Missing error-path coverage for reader search failure

page.server.spec.ts covers the happy path (search returns items) and the topPersons failure path, but not the case where the search API itself fails for a reader. recentDocs should default to [] in that case. The settled() helper handles this, but it would be good to have explicit test coverage given that this is the exact path that caused the original 500.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ⚠️ Approved with concerns** Good regression coverage for the core bug. The server-side test in `page.server.spec.ts` clearly documents the failure mode and proves the fix. Timestamp propagation is now explicitly asserted. The backend test fixture cleanup (replacing `null, null` with a fixed `LocalDateTime`) is correct and consistent. ### Blockers **1. `ReaderRecentDocs.svelte.spec.ts` uses the wrong type for test fixtures** The component's prop type is `DocumentListItem[]`, but the spec file builds fixtures as `type Document`. This creates a type contract gap: if `DocumentListItem` gains a required field in future, the spec's fixtures won't fail at compile time. Fix: migrate `baseDoc` and `updatedDoc` to `DocumentListItem` shape (remove `status`, `metadataComplete`, `scriptType`; add `completionPercentage`, `receivers`, `tags`, `contributors`, `matchData`). ### Suggestions **2. No boundary test for `isNew()` at exactly 7 days** The test suite covers "2 days ago → shows badge" and "40 days ago → hides badge". The boundary — a document created exactly 7 days ago — isn't tested. The off-by-one direction matters: is the threshold `>= 7 days` or `> 7 days`? Current implementation: `createdAt.getTime() > Date.now() - 7 * 24 * 60 * 60 * 1000` — so exactly 7 days old is NOT new (strict `>`). A test pinning this behavior would prevent a silent regression if the threshold or operator changes. **3. `Date.now()` in browser test fixtures — timezone note** Tests in `.test.ts` and `.spec.ts` now compute relative dates with `new Date(Date.now() - N * 86400000).toISOString()`. This is deterministic and correct. Just confirming: in CI, these dates will always be within the 7-day window by construction (2 days, 6 days), so no timezone-related flakiness expected. ✅ **4. Missing error-path coverage for reader search failure** `page.server.spec.ts` covers the happy path (search returns items) and the `topPersons` failure path, but not the case where the search API itself fails for a reader. `recentDocs` should default to `[]` in that case. The `settled()` helper handles this, but it would be good to have explicit test coverage given that this is the exact path that caused the original 500.

marcel commented 2026-05-25 15:50:29 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Senior Application Architect

Verdict: ⚠️ Approved with concerns

The fix is architecturally correct. DocumentListItem is the right DTO for list endpoints — it's distinct from the Document entity, it's flat, and it now carries all fields the frontend needs. The layering is clean: DocumentService.toListItem() is the only place that constructs this DTO, and the controller test verifies the JSON contract.

Blockers

1. api.ts was hand-edited instead of regenerated

frontend/src/lib/generated/api.ts is produced by openapi-typescript from the live backend spec. Manually adding fields to it creates a gap between the actual OpenAPI spec the backend emits and the TypeScript types the frontend sees. The next time someone runs npm run generate:api, this change is silently overwritten.

The proper fix: run npm run generate:api after the backend changes are committed, and include the regenerated file in this PR. If the tooling can't run in the current environment, document the gap explicitly in the PR body. This is a blocker because it means the TypeScript contract and the actual API contract are currently diverged.

Suggestions

2. LocalDateTime is timezone-naive — worth a note

DocumentListItem now carries LocalDateTime createdAt and updatedAt. LocalDateTime has no timezone offset. When Jackson serializes it to JSON it produces 2026-01-15T10:00:00 (no Z), which the frontend's new Date(doc.createdAt) will parse as local time, not UTC.

The existing Document entity already uses LocalDateTime via @CreationTimestamp / @UpdateTimestamp, so this is consistent with the existing pattern. As long as the server's JVM timezone is stable (UTC is the Docker default), there is no practical bug here. But it's worth knowing: if the server timezone ever diverges from UTC, the isNew() window calculation in the frontend will be off.

If consistency with the rest of the codebase is the priority, LocalDateTime is fine. If precision is the priority, Instant or OffsetDateTime with a Jackson @JsonSerialize to ISO-8601 with offset would be more robust.

No documentation updates needed — no new packages, no schema changes, no new routes. ✅

## 🏛️ Markus Keller — Senior Application Architect **Verdict: ⚠️ Approved with concerns** The fix is architecturally correct. `DocumentListItem` is the right DTO for list endpoints — it's distinct from the `Document` entity, it's flat, and it now carries all fields the frontend needs. The layering is clean: `DocumentService.toListItem()` is the only place that constructs this DTO, and the controller test verifies the JSON contract. ### Blockers **1. `api.ts` was hand-edited instead of regenerated** `frontend/src/lib/generated/api.ts` is produced by `openapi-typescript` from the live backend spec. Manually adding fields to it creates a gap between the actual OpenAPI spec the backend emits and the TypeScript types the frontend sees. The next time someone runs `npm run generate:api`, this change is silently overwritten. The proper fix: run `npm run generate:api` after the backend changes are committed, and include the regenerated file in this PR. If the tooling can't run in the current environment, document the gap explicitly in the PR body. This is a blocker because it means the TypeScript contract and the actual API contract are currently diverged. ### Suggestions **2. `LocalDateTime` is timezone-naive — worth a note** `DocumentListItem` now carries `LocalDateTime createdAt` and `updatedAt`. `LocalDateTime` has no timezone offset. When Jackson serializes it to JSON it produces `2026-01-15T10:00:00` (no Z), which the frontend's `new Date(doc.createdAt)` will parse as local time, not UTC. The existing `Document` entity already uses `LocalDateTime` via `@CreationTimestamp` / `@UpdateTimestamp`, so this is consistent with the existing pattern. As long as the server's JVM timezone is stable (UTC is the Docker default), there is no practical bug here. But it's worth knowing: if the server timezone ever diverges from UTC, the `isNew()` window calculation in the frontend will be off. If consistency with the rest of the codebase is the priority, `LocalDateTime` is fine. If precision is the priority, `Instant` or `OffsetDateTime` with a Jackson `@JsonSerialize` to ISO-8601 with offset would be more robust. **No documentation updates needed** — no new packages, no schema changes, no new routes. ✅

marcel commented 2026-05-25 15:50:39 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

Reviewed the full diff with an adversarial eye. No security concerns in this PR.

What I checked

Data exposure: DocumentListItem now includes createdAt and updatedAt. These are metadata timestamps, not sensitive data. No passwords, sessions, file paths, or internal IDs that shouldn't be public are being added. ✅

Input trust boundaries: isNew() uses doc.createdAt which comes from the server-side API response (not user input). The 7-day comparison uses Date.now() on the client — this is UI logic with no server-side consequence. ✅

Authorization: No new endpoints, no permission changes, no @RequirePermission modifications. The fix is purely in the data mapping layer. ✅

XSS surface: ReaderRecentDocs.svelte renders doc.title and doc.sender.displayName in template expressions ({doc.title}, {doc.sender.displayName ?? doc.sender.lastName}). Svelte's template compiler HTML-escapes these by default — no raw {@html} usage. ✅

Type safety: The manual api.ts edit (noted by Markus) is a drift concern, not a security concern. The fields added are typed as string, matching what the backend serializes. ✅

One observation (informational): The frontend uses new Date(doc.createdAt) to parse timestamps from the API. If the backend ever sends a malformed timestamp, new Date(malformed) returns Invalid Date with getTime() returning NaN. NaN > threshold is false, so isNew() would silently return false. This is acceptable graceful degradation — the badge just doesn't show. Not a vulnerability.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** Reviewed the full diff with an adversarial eye. No security concerns in this PR. ### What I checked **Data exposure:** `DocumentListItem` now includes `createdAt` and `updatedAt`. These are metadata timestamps, not sensitive data. No passwords, sessions, file paths, or internal IDs that shouldn't be public are being added. ✅ **Input trust boundaries:** `isNew()` uses `doc.createdAt` which comes from the server-side API response (not user input). The 7-day comparison uses `Date.now()` on the client — this is UI logic with no server-side consequence. ✅ **Authorization:** No new endpoints, no permission changes, no `@RequirePermission` modifications. The fix is purely in the data mapping layer. ✅ **XSS surface:** `ReaderRecentDocs.svelte` renders `doc.title` and `doc.sender.displayName` in template expressions (`{doc.title}`, `{doc.sender.displayName ?? doc.sender.lastName}`). Svelte's template compiler HTML-escapes these by default — no raw `{@html}` usage. ✅ **Type safety:** The manual `api.ts` edit (noted by Markus) is a drift concern, not a security concern. The fields added are typed as `string`, matching what the backend serializes. ✅ **One observation (informational):** The frontend uses `new Date(doc.createdAt)` to parse timestamps from the API. If the backend ever sends a malformed timestamp, `new Date(malformed)` returns `Invalid Date` with `getTime()` returning `NaN`. `NaN > threshold` is `false`, so `isNew()` would silently return `false`. This is acceptable graceful degradation — the badge just doesn't show. Not a vulnerability.

marcel commented 2026-05-25 15:50:54 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — Senior UX Designer & Accessibility Strategist

Verdict: ⚠️ Approved with concerns

The fix resolves a crash that prevented READ_ALL users from using the homepage at all — that's critical. The "Neu" badge has visible text, correct semantic classes (bg-accent-bg, rounded-full, text-ink), and the row links maintain min-h-[44px] touch targets. Accessibility-wise, the component is in good shape.

Concerns

1. "Neu" badge semantics: 7 days regardless of modifications may mislead

The old logic showed "Neu" when createdAt === updatedAt (never edited = truly new). The new logic shows "Neu" when createdAt is within 7 days, regardless of updatedAt.

This means a document created 2 days ago but edited 15 times still says "Neu". For the reader persona (younger users on phones checking what's new in the archive), this may not matter — they care about "was this added recently?" not "has it ever been touched?". But for a family archivist who adds a document and immediately starts annotating it, the badge would show on a document they've already interacted with heavily.

Consider whether the intent is:

• "Recently added to the archive" → 7-day window on createdAt is correct ✅
• "Something I haven't seen yet" → would require per-user read tracking (much more complex, not suggested here)

If the 7-day window is the intended semantic, the badge label "Neu" accurately reflects "recently added." I'd accept this. Just confirming the intent is deliberate.

2. Badge shown in the document row but relative date shown via relativeTimeDe(updatedAt)

The row shows the "Neu" badge based on createdAt but the right-side timestamp uses updatedAt. A document created 6 days ago that was updated yesterday would show both "Neu" and "vor 1 Tag". These two signals are coherent (recently added, also recently touched) — but a document created 6 days ago with no updates would show "Neu" and "vor 6 Tagen". Slightly mixed signals, but not a blocker.

Looks good ✅

• Touch targets: min-h-[44px] on both the "Alle Dokumente" link and each document row link ✅
• Focus rings: focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:outline-none ✅
• Sender fallback: doc.sender.displayName ?? doc.sender.lastName then — em-dash ✅
• Keyed {#each documents as doc (doc.id)} for stable DOM reconciliation ✅
• Font, color, and spacing tokens are all project-standard ✅

## 🎨 Leonie Voss — Senior UX Designer & Accessibility Strategist **Verdict: ⚠️ Approved with concerns** The fix resolves a crash that prevented READ_ALL users from using the homepage at all — that's critical. The "Neu" badge has visible text, correct semantic classes (`bg-accent-bg`, `rounded-full`, `text-ink`), and the row links maintain `min-h-[44px]` touch targets. Accessibility-wise, the component is in good shape. ### Concerns **1. "Neu" badge semantics: 7 days regardless of modifications may mislead** The old logic showed "Neu" when `createdAt === updatedAt` (never edited = truly new). The new logic shows "Neu" when `createdAt` is within 7 days, *regardless* of `updatedAt`. This means a document created 2 days ago but edited 15 times still says "Neu". For the reader persona (younger users on phones checking what's new in the archive), this may not matter — they care about "was this added recently?" not "has it ever been touched?". But for a family archivist who adds a document and immediately starts annotating it, the badge would show on a document they've already interacted with heavily. Consider whether the intent is: - "Recently added to the archive" → 7-day window on `createdAt` is correct ✅ - "Something I haven't seen yet" → would require per-user read tracking (much more complex, not suggested here) If the 7-day window is the intended semantic, the badge label "Neu" accurately reflects "recently added." I'd accept this. Just confirming the intent is deliberate. **2. Badge shown in the document row but relative date shown via `relativeTimeDe(updatedAt)`** The row shows the "Neu" badge based on `createdAt` but the right-side timestamp uses `updatedAt`. A document created 6 days ago that was updated yesterday would show both "Neu" and "vor 1 Tag". These two signals are coherent (recently added, also recently touched) — but a document created 6 days ago with no updates would show "Neu" and "vor 6 Tagen". Slightly mixed signals, but not a blocker. ### Looks good ✅ - Touch targets: `min-h-[44px]` on both the "Alle Dokumente" link and each document row link ✅ - Focus rings: `focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:outline-none` ✅ - Sender fallback: `doc.sender.displayName ?? doc.sender.lastName` then `—` em-dash ✅ - Keyed `{#each documents as doc (doc.id)}` for stable DOM reconciliation ✅ - Font, color, and spacing tokens are all project-standard ✅

marcel commented 2026-05-25 15:51:02 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

No infrastructure, CI, or Docker Compose changes in this PR. The fix is entirely application-layer. LGTM from a platform perspective.

What I checked

CI pipeline impact: The PR adds tests to existing test files and modifies one source file per stack. Existing CI jobs (backend Maven build, frontend Vitest server + client, lint) will pick these up automatically. No new test targets, no new build steps needed. ✅

No new dependencies: package-lock.json shows minor changes (10 ++++-----), likely lockfile normalization, not new packages. ✅

No new environment variables or secrets: The feature uses timestamps already stored in the database. No new configuration needed. ✅

One note (shared with Markus)

The manually edited api.ts is a generated file. Our CI doesn't currently validate that the checked-in api.ts matches what npm run generate:api would produce. If we wanted to enforce this, we could add a CI step:

- name: Validate generated API types
  run: |
    npm run generate:api
    git diff --exit-code frontend/src/lib/generated/api.ts

This would catch future drift. Not a blocker for this PR — the manual edit is intentional and documented — but worth tracking as a CI hygiene item.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** No infrastructure, CI, or Docker Compose changes in this PR. The fix is entirely application-layer. LGTM from a platform perspective. ### What I checked **CI pipeline impact:** The PR adds tests to existing test files and modifies one source file per stack. Existing CI jobs (backend Maven build, frontend Vitest server + client, lint) will pick these up automatically. No new test targets, no new build steps needed. ✅ **No new dependencies:** `package-lock.json` shows minor changes (`10 ++++-----`), likely lockfile normalization, not new packages. ✅ **No new environment variables or secrets:** The feature uses timestamps already stored in the database. No new configuration needed. ✅ ### One note (shared with Markus) The manually edited `api.ts` is a generated file. Our CI doesn't currently validate that the checked-in `api.ts` matches what `npm run generate:api` would produce. If we wanted to enforce this, we could add a CI step: ```yaml - name: Validate generated API types run: | npm run generate:api git diff --exit-code frontend/src/lib/generated/api.ts ``` This would catch future drift. Not a blocker for this PR — the manual edit is intentional and documented — but worth tracking as a CI hygiene item.

marcel commented 2026-05-25 15:51:14 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer & Business Analyst

Verdict: ✅ Approved

The root cause is clearly documented in the PR description. The fix is tightly scoped. No new user-visible features are introduced — the homepage now works as originally intended for READ_ALL users.

Observations

1. No linked Gitea issue

This PR doesn't reference a Gitea issue in its title or body (Fixes #NNN). For a production bug that caused a 500 for a whole user segment, an issue would:

• Serve as the permanent record of the problem report
• Allow the fix to be traced back to a user need
• Enable Closes #NNN auto-close on merge

Minor in the context of a solo project, but worth tracking.

2. The "Neu" badge redefinition is a requirements change, not just a bug fix

The original implementation defined "new" as createdAt === updatedAt (a document that has never been modified). The PR changes this to "created within the last 7 days."

These are materially different requirements:

• Old: "Has this document ever been edited?" (modification-state signal)
• New: "Was this document recently added to the archive?" (recency signal)

The new definition is more useful for the reader persona ("what's been added recently?") and less brittle technically. But this change in definition isn't documented in the PR body. A one-sentence addition like "Changed 'Neu' badge semantics: now means 'added within the last 7 days' instead of 'never modified'" would make the intent explicit for future readers of the git log.

3. The 7-day threshold is an assumption, not a requirement

Why 7 days? It's a reasonable convention (one week), but it's not derived from a user story or explicit requirement. If the family archive is updated infrequently (e.g., new documents added monthly), 7 days may be too short — a reader who visits weekly might miss "new" items. A threshold configurable via a future feature, or derived from visit frequency, might serve the readers better. For now, 7 days is a sensible default; just noting this is an assumption worth revisiting if users report missing the badge.

## 📋 Elicit — Requirements Engineer & Business Analyst **Verdict: ✅ Approved** The root cause is clearly documented in the PR description. The fix is tightly scoped. No new user-visible features are introduced — the homepage now works as originally intended for READ_ALL users. ### Observations **1. No linked Gitea issue** This PR doesn't reference a Gitea issue in its title or body (`Fixes #NNN`). For a production bug that caused a 500 for a whole user segment, an issue would: - Serve as the permanent record of the problem report - Allow the fix to be traced back to a user need - Enable `Closes #NNN` auto-close on merge Minor in the context of a solo project, but worth tracking. **2. The "Neu" badge redefinition is a requirements change, not just a bug fix** The original implementation defined "new" as `createdAt === updatedAt` (a document that has never been modified). The PR changes this to "created within the last 7 days." These are materially different requirements: - **Old**: "Has this document ever been edited?" (modification-state signal) - **New**: "Was this document recently added to the archive?" (recency signal) The new definition is more useful for the reader persona ("what's been added recently?") and less brittle technically. But this change in definition isn't documented in the PR body. A one-sentence addition like "Changed 'Neu' badge semantics: now means 'added within the last 7 days' instead of 'never modified'" would make the intent explicit for future readers of the git log. **3. The 7-day threshold is an assumption, not a requirement** Why 7 days? It's a reasonable convention (one week), but it's not derived from a user story or explicit requirement. If the family archive is updated infrequently (e.g., new documents added monthly), 7 days may be too short — a reader who visits weekly might miss "new" items. A threshold configurable via a future feature, or derived from visit frequency, might serve the readers better. For now, 7 days is a sensible default; just noting this is an assumption worth revisiting if users report missing the badge.

marcel added 1 commit 2026-05-25 17:26:08 +02:00

fix(review): regenerate api.ts and fix spec type ... d9e01ef1ff

Replace manual edits to api.ts with a proper `npm run generate:api` run —
the generated output is identical for DocumentListItem (createdAt/updatedAt
were already correct), so this just removes the drift risk flagged in review.

Fix ReaderRecentDocs.svelte.spec.ts to use DocumentListItem instead of
Document for all test fixtures, matching the component's actual prop type.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel added 1 commit 2026-05-25 17:40:33 +02:00

test(debounce): fix flaky onExit-cancels-debounce test ... 2e0eb40aec

The test raced a real 150 ms setTimeout: fill('Walter') started the
debounce, then focus + keyboard(Escape) had to complete before 150 ms
elapsed. Under CI load the Playwright CDP round-trips exceeded 150 ms,
letting the debounce fire first.

Fix: install vi.useFakeTimers() after the stable-state setup (so
vi.waitFor()'s real-timer polling still works), freeze the Walter
debounce, let Escape trigger onExit/cancel, then advance fake time
with vi.advanceTimersByTimeAsync() — no real-wall-clock race.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel merged commit 2e0eb40aec into main 2026-05-25 17:54:42 +02:00

marcel deleted branch worktree-fix+reader-dashboard-doc-undefined 2026-05-25 17:54:42 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

needs-discussion

Has an open decision or design question that must be resolved before implementation can start.

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#661