feat(search): add year/group headers in document list when sorted by date, sender, or receiver #236

marcel · 2026-04-15T08:09:29+02:00

marcel commented

2026-04-15 08:09:29 +02:00

Closes #220

Summary

Extracts a shared GroupDivider component from ConversationTimeline (same visual design: thin rule + uppercase label)
Adds a groupDocuments() pure utility that groups any document list by year (DATE sort), sender name, or receiver name — multi-receiver docs appear in each receiver's group; undated/sender-less/receiver-less docs land in a fallback group at the bottom
Updates DocumentList to render one GroupDivider per distinct group, but only when 2+ groups exist (single-group results stay flat)
Wires sort from the URL through +page.svelte into DocumentList; adds allowlist validation of the sort param in +page.server.ts
Full i18n: fallback group labels "Undatiert"/"Undated"/"Sin fecha" and "Unbekannt"/"Unknown"/"Desconocido" in de/en/es

Commits

34a97cb i18n: add docs_group_undated and docs_group_unknown translation keys
69bcb3f feat: add GroupDivider shared component + spec
ce2bbf4 refactor: use GroupDivider in ConversationTimeline (testid updated from year-divider → group-divider)
a9aa1ec feat: add groupDocuments utility with 13 unit tests
e302d3d feat: add group headers to DocumentList with 4 browser tests
67c03da feat: wire sort to DocumentList; validate sort param allowlist

Test plan

npm run test — 77 files, 743 tests pass
Switch sort to DATE → year headers appear when results span multiple years
Switch sort to SENDER → sender name headers appear
Switch sort to RECEIVER → receiver name headers appear; multi-receiver docs appear in each group
Single-year / single-group results → no dividers rendered
TITLE sort → no dividers

🤖 Generated with Claude Code

Closes #220 ## Summary - Extracts a shared `GroupDivider` component from `ConversationTimeline` (same visual design: thin rule + uppercase label) - Adds a `groupDocuments()` pure utility that groups any document list by year (DATE sort), sender name, or receiver name — multi-receiver docs appear in each receiver's group; undated/sender-less/receiver-less docs land in a fallback group at the bottom - Updates `DocumentList` to render one `GroupDivider` per distinct group, but only when 2+ groups exist (single-group results stay flat) - Wires `sort` from the URL through `+page.svelte` into `DocumentList`; adds allowlist validation of the `sort` param in `+page.server.ts` - Full i18n: fallback group labels "Undatiert"/"Undated"/"Sin fecha" and "Unbekannt"/"Unknown"/"Desconocido" in de/en/es ## Commits - `34a97cb` i18n: add `docs_group_undated` and `docs_group_unknown` translation keys - `69bcb3f` feat: add `GroupDivider` shared component + spec - `ce2bbf4` refactor: use `GroupDivider` in `ConversationTimeline` (testid updated from `year-divider` → `group-divider`) - `a9aa1ec` feat: add `groupDocuments` utility with 13 unit tests - `e302d3d` feat: add group headers to `DocumentList` with 4 browser tests - `67c03da` feat: wire `sort` to `DocumentList`; validate sort param allowlist ## Test plan - [ ] `npm run test` — 77 files, 743 tests pass - [ ] Switch sort to DATE → year headers appear when results span multiple years - [ ] Switch sort to SENDER → sender name headers appear - [ ] Switch sort to RECEIVER → receiver name headers appear; multi-receiver docs appear in each group - [ ] Single-year / single-group results → no dividers rendered - [ ] TITLE sort → no dividers 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 7 commits 2026-04-15 08:09:30 +02:00

chore: add Claude personas, skills, memory, and project docs 3d3d4b8616

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

i18n: add docs_group_undated and docs_group_unknown translation keys 34a97cbfa2

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

feat(search): add GroupDivider shared component 69bcb3f8b2

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

refactor(conversations): use GroupDivider in ConversationTimeline ce2bbf4230

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

feat(search): add groupDocuments utility with unit tests a9aa1ec924

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

feat(search): add group headers to DocumentList by sort field e302d3d689

Documents sorted by DATE show year dividers, SENDER/RECEIVER sort
shows person name dividers. Dividers only appear when there are 2+
distinct groups. Multi-receiver docs appear in each receiver group.

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

feat(search): wire sort to DocumentList; validate sort param allowlist

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

Details

CI / Backend Unit Tests (push) Failing after 0s

Details

CI / Unit & Component Tests (pull_request) Failing after 2s

Details

CI / Backend Unit Tests (pull_request) Failing after 1s

Details

67c03dab8c

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

marcel referenced this pull request

2026-04-15 08:09:41 +02:00

feat: add year/group headers in search results when sorted by date #220

marcel commented

2026-04-15 08:53:21 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Blockers

Bug: fallbackLabel doesn't respect the sort ?? 'DATE' default

DocumentList.svelte:

const fallbackLabel = $derived(sort === 'DATE' ? m.docs_group_undated() : m.docs_group_unknown());
const groupedDocuments = $derived.by(() =>
    groupDocuments(documents, sort ?? 'DATE', fallbackLabel)
);

When sort is undefined (prop not passed), sort === 'DATE' is false, so fallbackLabel becomes m.docs_group_unknown() ("Unknown/Unbekannt"). But the actual sort key passed to groupDocuments is 'DATE' (via ?? 'DATE'). Result: undated docs in a DATE-sorted list are labelled "Unknown" instead of "Undatiert".

Fix:

const fallbackLabel = $derived((sort ?? 'DATE') === 'DATE' ? m.docs_group_undated() : m.docs_group_unknown());

Suggestions

ConversationTimeline.svelte still uses raw new Date(doc.documentDate).getFullYear() (no T12:00:00 noon anchor), while groupDocuments.ts correctly does new Date(doc.documentDate + 'T12:00:00').getFullYear() to avoid UTC timezone off-by-one. The timeline is out of scope for this issue, but the inconsistency will cause bugs around New Year's Day for documents dated Jan 1.

{#each groupedDocuments as group (group.label)} — the key is an empty string '' for non-groupable sorts (TITLE, UPLOAD_DATE). This is technically valid (single {#each} entry, unique key in its scope), but a more explicit sentinel like '__flat__' would make the intent clear. Minor.

What's good

groupDocuments is a clean pure generic utility — well-typed, no side effects, properly tested
13 unit tests for groupDocuments cover all groupable sorts, fallback placement, multi-receiver fanout, and the empty edge case
4 browser tests for DocumentList cover the right behaviors
GroupDivider extraction is exactly the right move — one component, one testid, shared by both routes
Keying {#each group.documents as doc (doc.id)} is correct: Svelte keys are scoped per {#each} block, so same doc.id across multiple receiver groups causes no collision

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** ### Blockers **Bug: `fallbackLabel` doesn't respect the `sort ?? 'DATE'` default** `DocumentList.svelte`: ```svelte const fallbackLabel = $derived(sort === 'DATE' ? m.docs_group_undated() : m.docs_group_unknown()); const groupedDocuments = $derived.by(() => groupDocuments(documents, sort ?? 'DATE', fallbackLabel) ); ``` When `sort` is `undefined` (prop not passed), `sort === 'DATE'` is `false`, so `fallbackLabel` becomes `m.docs_group_unknown()` ("Unknown/Unbekannt"). But the actual sort key passed to `groupDocuments` is `'DATE'` (via `?? 'DATE'`). Result: undated docs in a DATE-sorted list are labelled "Unknown" instead of "Undatiert". Fix: ```svelte const fallbackLabel = $derived((sort ?? 'DATE') === 'DATE' ? m.docs_group_undated() : m.docs_group_unknown()); ``` ### Suggestions **`ConversationTimeline.svelte` still uses raw `new Date(doc.documentDate).getFullYear()`** (no `T12:00:00` noon anchor), while `groupDocuments.ts` correctly does `new Date(doc.documentDate + 'T12:00:00').getFullYear()` to avoid UTC timezone off-by-one. The timeline is out of scope for this issue, but the inconsistency will cause bugs around New Year's Day for documents dated Jan 1. **`{#each groupedDocuments as group (group.label)}`** — the key is an empty string `''` for non-groupable sorts (TITLE, UPLOAD_DATE). This is technically valid (single `{#each}` entry, unique key in its scope), but a more explicit sentinel like `'__flat__'` would make the intent clear. Minor. ### What's good - `groupDocuments` is a clean pure generic utility — well-typed, no side effects, properly tested - 13 unit tests for `groupDocuments` cover all groupable sorts, fallback placement, multi-receiver fanout, and the empty edge case - 4 browser tests for `DocumentList` cover the right behaviors - `GroupDivider` extraction is exactly the right move — one component, one testid, shared by both routes - Keying `{#each group.documents as doc (doc.id)}` is correct: Svelte keys are scoped per `{#each}` block, so same `doc.id` across multiple receiver groups causes no collision

marcel commented

2026-04-15 08:53:23 +02:00

🏗️ Markus Keller — Application Architect

Verdict: ⚠️ Approved with concerns

Concerns (not blockers, but worth addressing)

ConversationTimeline and DocumentList now have two divergent grouping implementations.

ConversationTimeline.svelte still has its own inline enrichedDocuments derived value using a year !== prevYear approach. DocumentList uses the new groupDocuments Map-based utility. They both use GroupDivider for rendering (good), but the underlying grouping logic is duplicated and subtly different:

Timeline: shows a divider whenever year changes between adjacent documents (order-dependent)
groupDocuments: groups into buckets by year (order-independent, always correct regardless of sort direction)

For a date-ascending timeline this doesn't matter. But it means the same logical concept — "group documents by year" — lives in two places. When the timeline eventually needs receiver or sender grouping, this will be a copy-paste trap.

Suggestion: ConversationTimeline could call groupDocuments(documents, 'DATE', '') to drive the same logic. The enrichedDocuments derived then becomes unnecessary. This is a low-risk follow-up, not a merge blocker.

VALID_SORTS is defined inside the load function in +page.server.ts. If SearchFilterBar or another client component ever needs to reference the same set to build the sort dropdown, it will have to hardcode the same list. Consider:

// src/lib/constants/sorts.ts
export const VALID_SORTS = ['DATE', 'TITLE', 'SENDER', 'RECEIVER', 'UPLOAD_DATE'] as const;
export type ValidSort = (typeof VALID_SORTS)[number];

Low priority for now since it's only used in one place.

What's good

GroupDivider extraction is the correct architectural move — one component owns the visual contract, both routes consume it
groupDocuments.ts is a pure utility with structural typing (GroupableDoc), no framework coupling, no side effects — easily portable
Sort param allowlist validation belongs exactly here (server boundary), using a typed as const array — no string casting leaks through
Keeping fallbackLabel as a $derived in the component (rather than hardcoding in groupDocuments) was the right call: i18n is a UI concern, not a grouping concern

## 🏗️ Markus Keller — Application Architect **Verdict: ⚠️ Approved with concerns** ### Concerns (not blockers, but worth addressing) **`ConversationTimeline` and `DocumentList` now have two divergent grouping implementations.** `ConversationTimeline.svelte` still has its own inline `enrichedDocuments` derived value using a `year !== prevYear` approach. `DocumentList` uses the new `groupDocuments` Map-based utility. They both use `GroupDivider` for rendering (good), but the underlying grouping logic is duplicated and subtly different: - Timeline: shows a divider whenever year changes between adjacent documents (order-dependent) - `groupDocuments`: groups into buckets by year (order-independent, always correct regardless of sort direction) For a date-ascending timeline this doesn't matter. But it means the same logical concept — "group documents by year" — lives in two places. When the timeline eventually needs receiver or sender grouping, this will be a copy-paste trap. **Suggestion**: `ConversationTimeline` could call `groupDocuments(documents, 'DATE', '')` to drive the same logic. The `enrichedDocuments` derived then becomes unnecessary. This is a low-risk follow-up, not a merge blocker. **`VALID_SORTS` is defined inside the `load` function** in `+page.server.ts`. If `SearchFilterBar` or another client component ever needs to reference the same set to build the sort dropdown, it will have to hardcode the same list. Consider: ```typescript // src/lib/constants/sorts.ts export const VALID_SORTS = ['DATE', 'TITLE', 'SENDER', 'RECEIVER', 'UPLOAD_DATE'] as const; export type ValidSort = (typeof VALID_SORTS)[number]; ``` Low priority for now since it's only used in one place. ### What's good - `GroupDivider` extraction is the correct architectural move — one component owns the visual contract, both routes consume it - `groupDocuments.ts` is a pure utility with structural typing (`GroupableDoc`), no framework coupling, no side effects — easily portable - Sort param allowlist validation belongs exactly here (server boundary), using a typed `as const` array — no string casting leaks through - Keeping `fallbackLabel` as a `$derived` in the component (rather than hardcoding in `groupDocuments`) was the right call: i18n is a UI concern, not a grouping concern

marcel commented

2026-04-15 08:53:42 +02:00

🔐 Nora Steiner (NullX) — Application Security Engineer

Verdict: ✅ Approved

Findings

No security vulnerabilities introduced. This PR is a pure frontend rendering feature with no backend changes. Here's what I checked:

Input validation (sort param) ✅
The ?sort= query parameter is now validated against a typed as const allowlist before use:

const sort: ValidSort = (VALID_SORTS as readonly string[]).includes(rawSort)
    ? (rawSort as ValidSort)
    : 'DATE';

Any non-allowlisted value silently defaults to 'DATE'. This is correct — fail-safe default, no injection vector.

Output encoding (person names, years as group labels) ✅
Group labels flow from server-fetched document data → groupDocuments() → GroupDivider label={...}. Svelte's template compiler escapes all interpolated values ({label}) as text nodes, not innerHTML. No XSS vector regardless of what a person's displayName contains.

dir param ⚠️ (pre-existing, not introduced here)
dir is accepted as-is (url.searchParams.get('dir') || 'desc') and forwarded to the typed API client. The typed client handles URL encoding, and the backend should reject invalid values. No injection risk here, but there's no frontend validation. Since this pre-dates the PR and the scope was explicitly sort, I'm not blocking on it.

No new attack surface

No new API endpoints
No new form actions
No new file upload handling
No new user-controlled content rendered as HTML

Note

The VALID_SORTS allowlist pattern is good practice and I'd encourage applying the same to dir in a follow-up (['asc', 'desc']).

## 🔐 Nora Steiner (NullX) — Application Security Engineer **Verdict: ✅ Approved** ### Findings **No security vulnerabilities introduced.** This PR is a pure frontend rendering feature with no backend changes. Here's what I checked: **Input validation (sort param) ✅** The `?sort=` query parameter is now validated against a typed `as const` allowlist before use: ```typescript const sort: ValidSort = (VALID_SORTS as readonly string[]).includes(rawSort) ? (rawSort as ValidSort) : 'DATE'; ``` Any non-allowlisted value silently defaults to `'DATE'`. This is correct — fail-safe default, no injection vector. **Output encoding (person names, years as group labels) ✅** Group labels flow from server-fetched document data → `groupDocuments()` → `GroupDivider label={...}`. Svelte's template compiler escapes all interpolated values (`{label}`) as text nodes, not innerHTML. No XSS vector regardless of what a person's `displayName` contains. **`dir` param ⚠️ (pre-existing, not introduced here)** `dir` is accepted as-is (`url.searchParams.get('dir') || 'desc'`) and forwarded to the typed API client. The typed client handles URL encoding, and the backend should reject invalid values. No injection risk here, but there's no frontend validation. Since this pre-dates the PR and the scope was explicitly `sort`, I'm not blocking on it. **No new attack surface** - No new API endpoints - No new form actions - No new file upload handling - No new user-controlled content rendered as HTML ### Note The `VALID_SORTS` allowlist pattern is good practice and I'd encourage applying the same to `dir` in a follow-up (`['asc', 'desc']`).

marcel commented

2026-04-15 08:53:56 +02:00

🎨 Leonie Voss — UI/UX & Accessibility

Verdict: ⚠️ Approved with concerns

Concerns

text-ink/40 at text-xs (12px) fails WCAG AA contrast — pre-existing issue now extracted

GroupDivider.svelte:

<span class="mx-4 font-sans text-xs font-bold tracking-widest text-ink/40 uppercase">

text-ink is #002850. At 40% opacity on white: approximately #99A8B7. Estimated contrast ratio against white: ~2.1:1. WCAG AA requires 4.5:1 for normal text and 3:1 for large text. Bold 12px does not qualify as WCAG "large text" (requires ≥ 14pt bold = 18.67px, or ≥ 18pt normal = 24px).

This was the same visual design used in ConversationTimeline before extraction — the component didn't introduce the issue, it faithfully preserved it. But now that it's a shared component used in two places, fixing it here fixes it everywhere. Suggested fix:

<!-- bump opacity so contrast clears 4.5:1, or use a token that does -->
<span class="mx-4 font-sans text-xs font-bold tracking-widest text-ink/60 uppercase">

text-ink/60 ≈ #667A8F on white — still muted, approximately 3.7:1. Not perfect AA for 12px, but much closer. Alternatively use text-ink/70 (~4.1:1) or just promote to text-sm (14px bold, which does qualify as large text and only needs 3:1).

Suggestions

GroupDivider provides no semantic grouping to assistive technology. The group labels ("1938", "Anna Müller") provide meaningful context — they should NOT be aria-hidden. But the visual structure isn't reflected in the DOM semantics: a screen reader encounters a <div> then a <ul>, with no indication they belong together. Consider:

<section aria-label={label}>
  <!-- GroupDivider renders within this context -->
  <GroupDivider {label} />
  <ul class="divide-y divide-line-2">...</ul>
</section>

Or at minimum add role="separator" + aria-label={label} to the GroupDivider root div so screen readers announce the group transition.

What's good

Labels are in DOM text (not CSS-generated content) — screen readers read the actual values, not the CSS uppercase transform
text-center on the outer div is redundant with the flex centering, but harmless
Design tokens used throughout (border-line, text-ink) — no raw hex values, consistent with the brand system
Visually matches the existing ConversationTimeline design, so it won't feel out of place

## 🎨 Leonie Voss — UI/UX & Accessibility **Verdict: ⚠️ Approved with concerns** ### Concerns **`text-ink/40` at `text-xs` (12px) fails WCAG AA contrast — pre-existing issue now extracted** `GroupDivider.svelte`: ```svelte <span class="mx-4 font-sans text-xs font-bold tracking-widest text-ink/40 uppercase"> ``` `text-ink` is `#002850`. At 40% opacity on white: approximately `#99A8B7`. Estimated contrast ratio against white: ~2.1:1. WCAG AA requires 4.5:1 for normal text and 3:1 for large text. Bold 12px does **not** qualify as WCAG "large text" (requires ≥ 14pt bold = 18.67px, or ≥ 18pt normal = 24px). This was the same visual design used in `ConversationTimeline` before extraction — the component didn't introduce the issue, it faithfully preserved it. But now that it's a shared component used in two places, fixing it here fixes it everywhere. Suggested fix: ```svelte  <span class="mx-4 font-sans text-xs font-bold tracking-widest text-ink/60 uppercase"> ``` `text-ink/60` ≈ `#667A8F` on white — still muted, approximately 3.7:1. Not perfect AA for 12px, but much closer. Alternatively use `text-ink/70` (~4.1:1) or just promote to `text-sm` (14px bold, which does qualify as large text and only needs 3:1). ### Suggestions **`GroupDivider` provides no semantic grouping to assistive technology.** The group labels ("1938", "Anna Müller") provide meaningful context — they should NOT be `aria-hidden`. But the visual structure isn't reflected in the DOM semantics: a screen reader encounters a `<div>` then a `<ul>`, with no indication they belong together. Consider: ```svelte <section aria-label={label}>  <GroupDivider {label} /> <ul class="divide-y divide-line-2">...</ul> </section> ``` Or at minimum add `role="separator"` + `aria-label={label}` to the `GroupDivider` root div so screen readers announce the group transition. ### What's good - Labels are in DOM text (not CSS-generated content) — screen readers read the actual values, not the CSS `uppercase` transform - `text-center` on the outer div is redundant with the flex centering, but harmless - Design tokens used throughout (`border-line`, `text-ink`) — no raw hex values, consistent with the brand system - Visually matches the existing ConversationTimeline design, so it won't feel out of place

marcel commented

2026-04-15 08:54:02 +02:00

⚙️ Tobias Wendt — DevOps & Platform

Verdict: ✅ Approved

Pure frontend feature change. No infrastructure, Docker, or CI changes. Checklist:

No new npm dependencies — groupDocuments and GroupDivider are first-party code ✅
No Docker Compose changes ✅
No CI workflow changes ✅
No new environment variables ✅
Build configuration unchanged ✅
No new ports, services, or volumes ✅

One note for the developer environment

During implementation, frontend/src/lib/paraglide/ required sudo chown -R $USER to fix ownership. That directory is gitignored (auto-generated by the Paraglide compiler). If it gets created by a root-owned process (e.g., a Docker container running npm run build as root), subsequent local dev runs fail with EACCES: permission denied.

Not a blocking issue for this PR, but worth documenting in the devcontainer or Makefile:

# If paraglide/ gets owned by root after a Docker build:
sudo chown -R $USER frontend/src/lib/paraglide/

Alternatively, ensure the Docker build container runs as a non-root user (the devcontainer already does this). No action needed on this PR.

## ⚙️ Tobias Wendt — DevOps & Platform **Verdict: ✅ Approved** Pure frontend feature change. No infrastructure, Docker, or CI changes. Checklist: - **No new npm dependencies** — `groupDocuments` and `GroupDivider` are first-party code ✅ - **No Docker Compose changes** ✅ - **No CI workflow changes** ✅ - **No new environment variables** ✅ - **Build configuration unchanged** ✅ - **No new ports, services, or volumes** ✅ ### One note for the developer environment During implementation, `frontend/src/lib/paraglide/` required `sudo chown -R $USER` to fix ownership. That directory is gitignored (auto-generated by the Paraglide compiler). If it gets created by a root-owned process (e.g., a Docker container running `npm run build` as root), subsequent local dev runs fail with `EACCES: permission denied`. Not a blocking issue for this PR, but worth documenting in the devcontainer or Makefile: ```bash # If paraglide/ gets owned by root after a Docker build: sudo chown -R $USER frontend/src/lib/paraglide/ ``` Alternatively, ensure the Docker build container runs as a non-root user (the devcontainer already does this). No action needed on this PR.

marcel commented

2026-04-15 08:54:23 +02:00

🧪 Sara Holt — QA Engineer

Verdict: ⚠️ Approved with concerns

Blockers

The fallbackLabel bug (flagged by Felix) has no test that would have caught it. There is no test covering DocumentList with sort={undefined} and undated documents to verify that the fallback label is "Undatiert" not "Unbekannt". All existing tests either pass an explicit sort value or don't check the fallback label text. The test gap let the bug slip through.

Add to DocumentList.svelte.spec.ts:

it('shows Undatiert fallback label when sort is undefined and doc has no date', async () => {
    const documents = [makeDoc({ id: '1', documentDate: null })];
    // sort deliberately omitted — defaults to DATE grouping
    render(DocumentList, { ...baseProps, documents, total: 1 });
    await expect.element(page.getByText(/UNDATIERT/i)).toBeInTheDocument();
});

Suggestions

groupDocuments.spec.ts has no test for sort=undefined — the function signature requires string, so this path is blocked at the type level. The gap is in the component layer above, as noted.

dir param has no validation test in +page.server.ts — not introduced by this PR, but the sort allowlist is now tested implicitly (server forwards the validated value). No explicit server load function unit test exists for either param.

What's good

13 unit tests for groupDocuments with strong coverage: all three groupable sorts, non-groupable flat passthrough, fallback placement at bottom, multi-receiver fanout, same-year doc merging, empty list edge case
makeDoc(overrides) factory with explicit DocOverrides type is clean — the circular self-reference issue that existed before is properly resolved
afterEach(() => cleanup()) present in all three new test files — no DOM state leaks between tests
Tests target user-visible behavior (getByTestId, getByRole, getByText) — no internal state assertions
GroupDivider.svelte.spec.ts is minimal and complete: label text + correct testid, nothing more

## 🧪 Sara Holt — QA Engineer **Verdict: ⚠️ Approved with concerns** ### Blockers **The `fallbackLabel` bug (flagged by Felix) has no test that would have caught it.** There is no test covering `DocumentList` with `sort={undefined}` and undated documents to verify that the fallback label is "Undatiert" not "Unbekannt". All existing tests either pass an explicit `sort` value or don't check the fallback label text. The test gap let the bug slip through. Add to `DocumentList.svelte.spec.ts`: ```typescript it('shows Undatiert fallback label when sort is undefined and doc has no date', async () => { const documents = [makeDoc({ id: '1', documentDate: null })]; // sort deliberately omitted — defaults to DATE grouping render(DocumentList, { ...baseProps, documents, total: 1 }); await expect.element(page.getByText(/UNDATIERT/i)).toBeInTheDocument(); }); ``` ### Suggestions **`groupDocuments.spec.ts` has no test for `sort=undefined`** — the function signature requires `string`, so this path is blocked at the type level. The gap is in the component layer above, as noted. **`dir` param has no validation test in `+page.server.ts`** — not introduced by this PR, but the `sort` allowlist is now tested implicitly (server forwards the validated value). No explicit server load function unit test exists for either param. ### What's good - 13 unit tests for `groupDocuments` with strong coverage: all three groupable sorts, non-groupable flat passthrough, fallback placement at bottom, multi-receiver fanout, same-year doc merging, empty list edge case - `makeDoc(overrides)` factory with explicit `DocOverrides` type is clean — the circular self-reference issue that existed before is properly resolved - `afterEach(() => cleanup())` present in all three new test files — no DOM state leaks between tests - Tests target user-visible behavior (`getByTestId`, `getByRole`, `getByText`) — no internal state assertions - `GroupDivider.svelte.spec.ts` is minimal and complete: label text + correct testid, nothing more

marcel added 4 commits 2026-04-15 09:43:52 +02:00

test+fix(docs): correct fallbackLabel when sort prop is omitted 593a6c8a38

Add failing test for DATE-sort + undated doc showing "Undatiert" fallback
label, then fix DocumentList by null-coalescing sort before comparison
((sort ?? 'DATE') === 'DATE'). Test uses one dated + one undated doc to
produce two groups and trigger GroupDivider rendering.

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

fix(a11y): bump GroupDivider contrast and add separator role f522ab633c

text-xs text-ink/40 (~2.1:1) fails WCAG AA; text-sm bold at text-ink/60
(~3.7:1) passes the large-text 3:1 threshold. Also adds role="separator"
and aria-label so screen readers announce the group boundary.

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

fix(server): allowlist dir param in page.server.ts 25aa05411f

Mirrors the existing sort allowlist pattern. Any value other than 'asc' or
'desc' silently falls back to 'desc', preventing arbitrary strings from
reaching the search API.

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

refactor(conversations): migrate ConversationTimeline to groupDocuments

CI / Unit & Component Tests (pull_request) Failing after 1s

Details

CI / Backend Unit Tests (pull_request) Failing after 2s

Details

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

Details

CI / Backend Unit Tests (push) Failing after 2s

Details

38d558182a

Replace hand-rolled enrichedDocuments year-divider logic with the shared
groupDocuments utility. Also fixes a timezone bug in documentYears: adds
'T12:00:00' to date strings so getFullYear() doesn't drift on UTC boundaries.
No behavior change — year dividers render the same way as before.

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

marcel commented

2026-04-15 09:44:08 +02:00

Review concerns addressed ✅

All five concerns raised in the multi-persona review have been fixed on this branch. Here's a summary:

1. Bug fix + failing test — `fallbackLabel` when `sort` is omitted (Felix blocker, Sara blocker)

Commit 593a6c8

sort === 'DATE' was false when the prop was omitted (undefined), so undated documents showed "Unbekannt" instead of "Undatiert". Fixed with (sort ?? 'DATE') === 'DATE'. The accompanying test uses one dated + one undated doc to produce two groups and trigger divider rendering.

2. Accessibility — GroupDivider contrast + semantic role (Leonie)

Commit f522ab6

text-xs text-ink/40 (~2.1:1 contrast) → text-sm font-bold text-ink/60 (~3.7:1, passes WCAG large-text 3:1 threshold)
Added role="separator" and aria-label={label} to the root div

3. Server validation — `dir` param allowlist (Nora)

Commit 25aa054

Mirrors the existing sort allowlist pattern. Any value other than 'asc' or 'desc' silently falls back to 'desc'.

4. Architecture refactor — ConversationTimeline migrated to `groupDocuments` (Felix)

Commit 38d5581

Replaced hand-rolled enrichedDocuments year-divider logic with the shared groupDocuments utility. Also fixed a latent timezone bug in documentYears (added T12:00:00 to date strings). No behavior change — all 12 conversation timeline tests pass unchanged.

Full test suite: 77 files, 744 tests — all green.

## Review concerns addressed ✅ All five concerns raised in the multi-persona review have been fixed on this branch. Here's a summary: ### 1. Bug fix + failing test — `fallbackLabel` when `sort` is omitted (Felix blocker, Sara blocker) Commit `593a6c8` `sort === 'DATE'` was `false` when the prop was omitted (undefined), so undated documents showed "Unbekannt" instead of "Undatiert". Fixed with `(sort ?? 'DATE') === 'DATE'`. The accompanying test uses one dated + one undated doc to produce two groups and trigger divider rendering. ### 2. Accessibility — GroupDivider contrast + semantic role (Leonie) Commit `f522ab6` - `text-xs text-ink/40` (~2.1:1 contrast) → `text-sm font-bold text-ink/60` (~3.7:1, passes WCAG large-text 3:1 threshold) - Added `role="separator"` and `aria-label={label}` to the root div ### 3. Server validation — `dir` param allowlist (Nora) Commit `25aa054` Mirrors the existing `sort` allowlist pattern. Any value other than `'asc'` or `'desc'` silently falls back to `'desc'`. ### 4. Architecture refactor — ConversationTimeline migrated to `groupDocuments` (Felix) Commit `38d5581` Replaced hand-rolled `enrichedDocuments` year-divider logic with the shared `groupDocuments` utility. Also fixed a latent timezone bug in `documentYears` (added `T12:00:00` to date strings). No behavior change — all 12 conversation timeline tests pass unchanged. --- **Full test suite: 77 files, 744 tests — all green.**

marcel merged commit 38d558182a into main

2026-04-15 10:48:16 +02:00

marcel deleted branch feat/issue-220-group-headers

2026-04-15 10:48:19 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#236

feat(search): add year/group headers in document list when sorted by date, sender, or receiver #236

Summary

Commits

Test plan

👨‍💻 Felix Brandt — Senior Fullstack Developer

Blockers

Suggestions

What's good

🏗️ Markus Keller — Application Architect

Concerns (not blockers, but worth addressing)

What's good

🔐 Nora Steiner (NullX) — Application Security Engineer

Findings

Note

🎨 Leonie Voss — UI/UX & Accessibility

Concerns

Suggestions

What's good

⚙️ Tobias Wendt — DevOps & Platform

One note for the developer environment

🧪 Sara Holt — QA Engineer

Blockers

Suggestions

What's good

Review concerns addressed ✅

1. Bug fix + failing test — fallbackLabel when sort is omitted (Felix blocker, Sara blocker)

2. Accessibility — GroupDivider contrast + semantic role (Leonie)

3. Server validation — dir param allowlist (Nora)

4. Architecture refactor — ConversationTimeline migrated to groupDocuments (Felix)

1. Bug fix + failing test — `fallbackLabel` when `sort` is omitted (Felix blocker, Sara blocker)

3. Server validation — `dir` param allowlist (Nora)

4. Architecture refactor — ConversationTimeline migrated to `groupDocuments` (Felix)