feat/issue-283-sender-receiver-grouping #284

Merged

marcel merged 8 commits from feat/issue-283-sender-receiver-grouping into main 2026-04-20 11:29:34 +02:00

Conversation 13 Commits 8 Files Changed 7 +211 -32

marcel commented 2026-04-20 10:55:12 +02:00

Owner

Copy Link

No description provided.

marcel added 3 commits 2026-04-20 10:55:12 +02:00

feat(i18n): add unknown sender/receiver fallback labels for document grouping ... 06ba11a743

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

feat(documents): restore sender/receiver grouping in document list ... ca3d8098d3

When sort=SENDER, documents group under the sender's display name card.
When sort=RECEIVER, a document appears under each receiver's card
(with multi-receiver duplication). Falls back to i18n labels for unknown
sender/receiver. Passes sort prop from /documents page to DocumentList.

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

fix(document-row): align contributor circles with progress ring ... ac50b353b8

The ProgressRing renders SVG + percentage label as a flex column (~52px
total). With items-center, the contributor circles aligned to the middle
of the full block, placing them 8px below the ring center. Changed to
items-start on the container and wrapped ContributorStack in h-9 (36px =
SVG height) flex items-center so both circles center at the same 18px.

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

marcel commented 2026-04-20 10:56:41 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Blockers

sort prop typed as string instead of a union literal type — DocumentList.svelte, props declaration:

sort?: string;

A typo like sort="SNEDER" silently falls through to year grouping — no compiler error, no runtime error, just wrong behaviour. Fix:

sort?: 'DATE' | 'SENDER' | 'RECEIVER';

'Ohne Datum' is a hardcoded German string in groupByYear() while groupBySender and groupByReceiver both use m.docs_group_unknown_sender() / m.docs_group_unknown_receiver(). The inconsistency is in DocumentList.svelte — either add an m.docs_group_no_date() key or at least add the German, English, and Spanish keys to be consistent with the pattern used directly below.

Suggestions

• year-card data-testid is semantically wrong now — the sender and receiver grouping tests also check page.getByTestId('year-card') to count cards. Renaming it to group-card in the template and tests would make the intention clear for future readers.
• The three groupBy* functions share a nearly identical SvelteMap pattern — I considered suggesting extraction, but the receiver case is genuinely different (multi-bucket fan-out per item), so KISS wins. Good call keeping them separate.
• TDD discipline: the tests are comprehensive and clearly written alongside the implementation. The makeItem factory function pattern is used consistently throughout. The sender: null → undefined fix ensures type alignment. Well done.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** ### Blockers **`sort` prop typed as `string` instead of a union literal type** — `DocumentList.svelte`, props declaration: ```svelte sort?: string; ``` A typo like `sort="SNEDER"` silently falls through to year grouping — no compiler error, no runtime error, just wrong behaviour. Fix: ```typescript sort?: 'DATE' | 'SENDER' | 'RECEIVER'; ``` **`'Ohne Datum'` is a hardcoded German string** in `groupByYear()` while `groupBySender` and `groupByReceiver` both use `m.docs_group_unknown_sender()` / `m.docs_group_unknown_receiver()`. The inconsistency is in `DocumentList.svelte` — either add an `m.docs_group_no_date()` key or at least add the German, English, and Spanish keys to be consistent with the pattern used directly below. ### Suggestions - **`year-card` data-testid is semantically wrong now** — the sender and receiver grouping tests also check `page.getByTestId('year-card')` to count cards. Renaming it to `group-card` in the template and tests would make the intention clear for future readers. - The three `groupBy*` functions share a nearly identical `SvelteMap` pattern — I considered suggesting extraction, but the receiver case is genuinely different (multi-bucket fan-out per item), so KISS wins. Good call keeping them separate. - TDD discipline: the tests are comprehensive and clearly written alongside the implementation. The `makeItem` factory function pattern is used consistently throughout. The `sender: null → undefined` fix ensures type alignment. Well done.

marcel commented 2026-04-20 10:56:48 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

This is a clean, purely presentational change. All grouping logic runs client-side on already-loaded data — no new API surface, no backend coupling, no database queries added. The architecture is solid:

• DocumentList remains a pure component: it takes items: DocumentSearchItem[] and sort as props and derives its view. No side effects, no external calls.
• The sort dispatch happens in the parent page (+page.svelte), which is the right orchestration layer.
• The SvelteMap used in the group functions is correct — plain Map mutations are invisible to Svelte's reactivity system.
• The composite key group.label + '-' + item.document.id in the {#each} block is a smart move: it handles the receiver fan-out case where the same document ID appears under multiple groups.

Minor note: The sort prop is typed as string in the props interface. For an architectural view this is a weak contract — Felix flagged it too. A union literal type ('DATE' | 'SENDER' | 'RECEIVER') would make the component's contract explicit and eliminate the silent fallback risk. No structural consequence, but worth tightening.

No layer boundary violations, no accidental complexity, no premature abstractions. Good scope control on this feature.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** This is a clean, purely presentational change. All grouping logic runs client-side on already-loaded data — no new API surface, no backend coupling, no database queries added. The architecture is solid: - `DocumentList` remains a pure component: it takes `items: DocumentSearchItem[]` and `sort` as props and derives its view. No side effects, no external calls. - The `sort` dispatch happens in the parent page (`+page.svelte`), which is the right orchestration layer. - The `SvelteMap` used in the group functions is correct — plain `Map` mutations are invisible to Svelte's reactivity system. - The composite key `group.label + '-' + item.document.id` in the `{#each}` block is a smart move: it handles the receiver fan-out case where the same document ID appears under multiple groups. **Minor note:** The `sort` prop is typed as `string` in the props interface. For an architectural view this is a weak contract — Felix flagged it too. A union literal type (`'DATE' | 'SENDER' | 'RECEIVER'`) would make the component's contract explicit and eliminate the silent fallback risk. No structural consequence, but worth tightening. No layer boundary violations, no accidental complexity, no premature abstractions. Good scope control on this feature.

marcel commented 2026-04-20 10:56:55 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

No security concerns in this PR. A clean LGTM from me — here's what I checked:

• No new API surface — this is purely client-side rendering logic. No new endpoints, no new server-side data flows, no new CORS surface.
• No user input reaches the database — the grouping is computed from already-loaded DocumentSearchItem[] data. The sort prop is a UI state variable, not a database query parameter.
• displayName rendered in group headers comes from the backend — it's pre-stored data that passed through the existing persistence layer, not a raw user-controlled string injected into the current request. No XSS vector here.
• i18n keys for unknown sender/receiver are static translations, not interpolated user content.
• sender: null → undefined change is a TypeScript alignment fix with no security implication.

The one thing I'd note for future reference: if the sort URL parameter ever gets proxied into a backend query (e.g., a future server-side sorting feature), validate and whitelist it strictly at the server boundary before it touches any query logic. Right now it's safe because the sort is purely frontend-computed.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** No security concerns in this PR. A clean LGTM from me — here's what I checked: - **No new API surface** — this is purely client-side rendering logic. No new endpoints, no new server-side data flows, no new CORS surface. - **No user input reaches the database** — the grouping is computed from already-loaded `DocumentSearchItem[]` data. The `sort` prop is a UI state variable, not a database query parameter. - **`displayName` rendered in group headers comes from the backend** — it's pre-stored data that passed through the existing persistence layer, not a raw user-controlled string injected into the current request. No XSS vector here. - **i18n keys for unknown sender/receiver** are static translations, not interpolated user content. - **`sender: null → undefined` change** is a TypeScript alignment fix with no security implication. The one thing I'd note for future reference: if the `sort` URL parameter ever gets proxied into a backend query (e.g., a future server-side sorting feature), validate and whitelist it strictly at the server boundary before it touches any query logic. Right now it's safe because the sort is purely frontend-computed.

marcel commented 2026-04-20 10:57:06 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

The new test coverage is solid overall — three well-structured describe blocks, consistent use of the makeItem factory, and tests for both the happy path and fallback labels. The sender: null → undefined fix aligns test data with the actual TypeScript types.

Concerns

year-card data-testid is semantically misleading in sender/receiver tests — the sender and receiver grouping tests both use page.getByTestId('year-card') to count group cards:

// In "groups documents with the same sender into one card"
const cards = page.getByTestId('year-card');
await expect.element(cards.first()).toBeInTheDocument();
await expect.element(cards.nth(1)).not.toBeInTheDocument();

This works today but will confuse future test readers: "why is a sender grouping test looking for year-card?" Rename the testid in the template to group-card and update all three test suites for consistency.

Missing edge case: unknown sort value — the $derived.by() dispatch silently falls through to year grouping for any unrecognised sort value. This is reasonable behaviour, but it's untested. If a stale bookmark or URL parameter sends sort=SUBJECT, the user silently sees year groups with no indication of the fallback. A single test case would document the intent:

it('falls back to year grouping for unknown sort values', async () => {
    render(DocumentList, { ...baseProps, items: yearItems, sort: 'UNKNOWN' });
    await expect.element(page.getByTestId('group-header').filter({ hasText: '2024' })).toBeInTheDocument();
});

What's Good

• Factory function pattern used consistently — zero ad-hoc object construction in test bodies.
• The "duplicates into each receiver group" test explicitly checks for two separate group-header cards — covers the fan-out logic precisely.
• Fallback label tests use exact translated strings ('Unbekannter Absender', 'Unbekannter Empfänger'), which will catch i18n regressions.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** The new test coverage is solid overall — three well-structured `describe` blocks, consistent use of the `makeItem` factory, and tests for both the happy path and fallback labels. The `sender: null → undefined` fix aligns test data with the actual TypeScript types. ### Concerns **`year-card` data-testid is semantically misleading in sender/receiver tests** — the sender and receiver grouping tests both use `page.getByTestId('year-card')` to count group cards: ```typescript // In "groups documents with the same sender into one card" const cards = page.getByTestId('year-card'); await expect.element(cards.first()).toBeInTheDocument(); await expect.element(cards.nth(1)).not.toBeInTheDocument(); ``` This works today but will confuse future test readers: "why is a sender grouping test looking for `year-card`?" Rename the testid in the template to `group-card` and update all three test suites for consistency. **Missing edge case: unknown `sort` value** — the `$derived.by()` dispatch silently falls through to year grouping for any unrecognised `sort` value. This is reasonable behaviour, but it's untested. If a stale bookmark or URL parameter sends `sort=SUBJECT`, the user silently sees year groups with no indication of the fallback. A single test case would document the intent: ```typescript it('falls back to year grouping for unknown sort values', async () => { render(DocumentList, { ...baseProps, items: yearItems, sort: 'UNKNOWN' }); await expect.element(page.getByTestId('group-header').filter({ hasText: '2024' })).toBeInTheDocument(); }); ``` ### What's Good - Factory function pattern used consistently — zero ad-hoc object construction in test bodies. - The "duplicates into each receiver group" test explicitly checks for two separate group-header cards — covers the fan-out logic precisely. - Fallback label tests use exact translated strings (`'Unbekannter Absender'`, `'Unbekannter Empfänger'`), which will catch i18n regressions.

marcel commented 2026-04-20 10:57:14 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ⚠️ Approved with concerns

Concern — uppercase transform on person names in group headers

The group header uses:

<span class="font-sans text-xs font-bold tracking-widest text-ink-3 uppercase">
    {group.label}
</span>

For year grouping, uppercase on "2024" is fine — it renders the same. But for sender/receiver grouping, "Max Mustermann" becomes "MAX MUSTERMANN", which:

1. Feels harsh and impersonal — the archive is about family members, and all-caps names are harder to read and lose the proper-noun casing.
2. Is harder to scan for the senior audience (60+) — all-caps strings have reduced letter-shape differentiation.

Fix: Conditionally suppress the transform for name-based group modes, or apply CSS font-variant: small-caps which preserves case while maintaining the visual weight:

<span
    data-testid="group-header"
    class="font-sans text-xs font-bold tracking-widest text-ink-3"
    class:uppercase={sort === 'DATE'}
    class:tracking-wider={sort !== 'DATE'}
>{group.label}</span>

What's Good

• The items-start + h-9 items-center wrapper in DocumentRow.svelte correctly fixes the vertical alignment — the ProgressRing was stretching to match the ContributorStack height before. Clean fix.
• data-testid="group-header" added to the span — good for both testing and accessibility tooling.
• The i18n keys are added for all three languages (de/en/es) simultaneously — no missing translation gaps.

Minor

The data-testid="year-card" on the group container is now a misnomer. Renaming to group-card would improve future readability (Sara flagged this too from a testing angle).

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ⚠️ Approved with concerns** ### Concern — `uppercase` transform on person names in group headers The group header uses: ```svelte <span class="font-sans text-xs font-bold tracking-widest text-ink-3 uppercase"> {group.label} </span> ``` For year grouping, `uppercase` on "2024" is fine — it renders the same. But for sender/receiver grouping, "Max Mustermann" becomes **"MAX MUSTERMANN"**, which: 1. Feels harsh and impersonal — the archive is about family members, and all-caps names are harder to read and lose the proper-noun casing. 2. Is harder to scan for the senior audience (60+) — all-caps strings have reduced letter-shape differentiation. **Fix:** Conditionally suppress the transform for name-based group modes, or apply CSS `font-variant: small-caps` which preserves case while maintaining the visual weight: ```svelte <span data-testid="group-header" class="font-sans text-xs font-bold tracking-widest text-ink-3" class:uppercase={sort === 'DATE'} class:tracking-wider={sort !== 'DATE'} >{group.label}</span> ``` ### What's Good - The `items-start` + `h-9 items-center` wrapper in `DocumentRow.svelte` correctly fixes the vertical alignment — the ProgressRing was stretching to match the ContributorStack height before. Clean fix. - `data-testid="group-header"` added to the span — good for both testing and accessibility tooling. - The i18n keys are added for all three languages (de/en/es) simultaneously — no missing translation gaps. ### Minor The `data-testid="year-card"` on the group container is now a misnomer. Renaming to `group-card` would improve future readability (Sara flagged this too from a testing angle).

marcel commented 2026-04-20 10:57:19 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

Nothing here touches infrastructure. This is a pure frontend feature — Svelte component logic, i18n strings, and Vitest tests. No changes to docker-compose.yml, CI workflows, environment variables, image tags, or deployment configuration.

LGTM from a platform perspective. No action needed on my end.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** Nothing here touches infrastructure. This is a pure frontend feature — Svelte component logic, i18n strings, and Vitest tests. No changes to `docker-compose.yml`, CI workflows, environment variables, image tags, or deployment configuration. LGTM from a platform perspective. No action needed on my end.

marcel added 5 commits 2026-04-20 11:25:42 +02:00

refactor(documents): rename year-card testid to group-card ... 5c2378a6fe

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

fix(documents): use i18n key for undated group label instead of hardcoded German string ... a62ccd428b

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

refactor(documents): narrow sort prop type to full SortMode union ... 4cdec7ec71

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

test(documents): add regression test for sort fallback to year grouping ... 506a220ad2

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

fix(documents): suppress uppercase on person name group headers for SENDER/RECEIVER sort ... 1ac2918835

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

marcel commented 2026-04-20 11:25:56 +02:00

Author

Owner

Copy Link

✅ Review concerns addressed

All review concerns from this round have been resolved. Here's what was done:

5c2378a6 — data-testid="year-card" → group-card

Renamed in both DocumentList.svelte and all four spec assertions. Addresses: @felixbrandt, @saraholt, @leonievoss.

a62ccd42 — Hardcoded 'Ohne Datum' → m.docs_group_undated()

groupByYear() now uses the pre-existing docs_group_undated i18n key ("Undatiert" / "Undated" / "Sin fecha"). The corresponding spec updated from 'Ohne Datum' to 'Undatiert'. Addresses: @felixbrandt.

4cdec7ec — sort prop type narrowed from string to SortMode

SortMode = 'DATE' | 'TITLE' | 'SENDER' | 'RECEIVER' | 'UPLOAD_DATE' | 'RELEVANCE' — matches the full API enum so +page.svelte passes without a cast. Addresses: @felixbrandt, @mkeller.

506a220a — Regression test for sort fallback behaviour

Added a test asserting that sort='TITLE' (and any non-SENDER/non-RECEIVER value) falls back to year grouping. Addresses: @saraholt.

1ac29188 — Suppress uppercase on person name group headers

uppercase and tracking-widest are now conditional on sort !== 'SENDER' && sort !== 'RECEIVER'; person name headers get tracking-wide instead. Addresses: @leonievoss.

---

17 tests passing, no new type check errors, lint clean.

## ✅ Review concerns addressed All review concerns from this round have been resolved. Here's what was done: ### 5c2378a6 — `data-testid="year-card"` → `group-card` Renamed in both `DocumentList.svelte` and all four spec assertions. Addresses: @felixbrandt, @saraholt, @leonievoss. ### a62ccd42 — Hardcoded `'Ohne Datum'` → `m.docs_group_undated()` `groupByYear()` now uses the pre-existing `docs_group_undated` i18n key ("Undatiert" / "Undated" / "Sin fecha"). The corresponding spec updated from `'Ohne Datum'` to `'Undatiert'`. Addresses: @felixbrandt. ### 4cdec7ec — `sort` prop type narrowed from `string` to `SortMode` `SortMode = 'DATE' | 'TITLE' | 'SENDER' | 'RECEIVER' | 'UPLOAD_DATE' | 'RELEVANCE'` — matches the full API enum so `+page.svelte` passes without a cast. Addresses: @felixbrandt, @mkeller. ### 506a220a — Regression test for sort fallback behaviour Added a test asserting that `sort='TITLE'` (and any non-SENDER/non-RECEIVER value) falls back to year grouping. Addresses: @saraholt. ### 1ac29188 — Suppress `uppercase` on person name group headers `uppercase` and `tracking-widest` are now conditional on `sort !== 'SENDER' && sort !== 'RECEIVER'`; person name headers get `tracking-wide` instead. Addresses: @leonievoss. --- 17 tests passing, no new type check errors, lint clean.

marcel commented 2026-04-20 11:27:42 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Every concern from round 1 has been resolved cleanly.

What I checked:

• SortMode = 'DATE' | 'TITLE' | 'SENDER' | 'RECEIVER' | 'UPLOAD_DATE' | 'RELEVANCE' — the right call. Matches the full backend enum so +page.svelte needs no cast. Typos are now caught at compile time.
• m.docs_group_undated() replaces 'Ohne Datum' — all three groupBy functions now use i18n consistently.
• group-card testid is consistent everywhere — template and all four spec assertions.
• Fallback test added: sort='TITLE' → year grouping verified as a regression guard. Passes immediately (existing behaviour) — appropriate for a guard test.
• Conditional uppercase / tracking-widest vs tracking-wide for person name headers — correct and clean.

TDD discipline: 17 tests covering year grouping, sort fallback, sender grouping (3 cases), receiver grouping (3 cases), and DocumentRow delegation. Each describe block tests one behaviour axis. Factory function used consistently throughout. Red/green sequence was clean.

Code quality: Three separate groupBy* functions — the receiver fan-out case genuinely differs from the sender/year patterns, so keeping them separate is the right KISS call. No abstraction for its own sake.

Ready to merge.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** Every concern from round 1 has been resolved cleanly. **What I checked:** - `SortMode = 'DATE' | 'TITLE' | 'SENDER' | 'RECEIVER' | 'UPLOAD_DATE' | 'RELEVANCE'` — the right call. Matches the full backend enum so `+page.svelte` needs no cast. Typos are now caught at compile time. - `m.docs_group_undated()` replaces `'Ohne Datum'` — all three groupBy functions now use i18n consistently. - `group-card` testid is consistent everywhere — template and all four spec assertions. - Fallback test added: `sort='TITLE'` → year grouping verified as a regression guard. Passes immediately (existing behaviour) — appropriate for a guard test. - Conditional `uppercase` / `tracking-widest` vs `tracking-wide` for person name headers — correct and clean. **TDD discipline:** 17 tests covering year grouping, sort fallback, sender grouping (3 cases), receiver grouping (3 cases), and DocumentRow delegation. Each describe block tests one behaviour axis. Factory function used consistently throughout. Red/green sequence was clean. **Code quality:** Three separate `groupBy*` functions — the receiver fan-out case genuinely differs from the sender/year patterns, so keeping them separate is the right KISS call. No abstraction for its own sake. Ready to merge.

marcel commented 2026-04-20 11:27:48 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

All architectural concerns from round 1 are resolved.

SortMode now correctly spans the full backend enum — the component's prop interface is honest about what it accepts, and the implementation handles the three grouping strategies while gracefully falling through to year-grouping for anything else. That's the right layering: the component knows about grouping modes, not about what the backend sorts by.

The composite {#each} key group.label + '-' + item.document.id correctly handles the receiver fan-out case where the same document ID appears in multiple groups. Subtle, but correct.

Architecture remains clean: DocumentList is a pure presentational component. No new backend coupling, no side effects. The sort prop flows down from +page.svelte which is the right orchestration layer.

This is ready to merge.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** All architectural concerns from round 1 are resolved. `SortMode` now correctly spans the full backend enum — the component's prop interface is honest about what it accepts, and the implementation handles the three grouping strategies while gracefully falling through to year-grouping for anything else. That's the right layering: the component knows about grouping modes, not about what the backend sorts by. The composite `{#each}` key `group.label + '-' + item.document.id` correctly handles the receiver fan-out case where the same document ID appears in multiple groups. Subtle, but correct. Architecture remains clean: `DocumentList` is a pure presentational component. No new backend coupling, no side effects. The `sort` prop flows down from `+page.svelte` which is the right orchestration layer. This is ready to merge.

marcel commented 2026-04-20 11:27:52 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

Nothing changed from a security perspective. Still a pure client-side rendering feature — no new API surface, no user input reaching any query, no new server-side data flows. The displayName values in group headers come from pre-stored backend data, not from the current request.

The i18n key additions are static translations with no interpolated user content.

Clean bill of health. Ready to merge.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** Nothing changed from a security perspective. Still a pure client-side rendering feature — no new API surface, no user input reaching any query, no new server-side data flows. The `displayName` values in group headers come from pre-stored backend data, not from the current request. The i18n key additions are static translations with no interpolated user content. Clean bill of health. Ready to merge.

marcel commented 2026-04-20 11:27:56 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ✅ Approved

All round-1 concerns addressed.

• group-card testid is now consistent across all four test call sites — the semantic confusion is gone.
• The sort fallback test (sort='TITLE' → shows '2024' group header) documents the existing behaviour as a regression guard. Correct approach for an already-implemented fallback.
• 17 tests across 5 describe blocks: result count, empty state, year grouping (3), sort fallback (1), sender grouping (3), receiver grouping (3), DocumentRow delegation (3).
• Factory function makeItem() used consistently — no ad-hoc construction in test bodies.
• Test names read as sentences describing behaviour.

The test suite now covers every code path in DocumentList.svelte — year, SENDER, RECEIVER, and the fallback case. This is what a complete unit test layer looks like.

Ready to merge.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ✅ Approved** All round-1 concerns addressed. - `group-card` testid is now consistent across all four test call sites — the semantic confusion is gone. - The sort fallback test (`sort='TITLE'` → shows `'2024'` group header) documents the existing behaviour as a regression guard. Correct approach for an already-implemented fallback. - 17 tests across 5 describe blocks: result count, empty state, year grouping (3), sort fallback (1), sender grouping (3), receiver grouping (3), DocumentRow delegation (3). - Factory function `makeItem()` used consistently — no ad-hoc construction in test bodies. - Test names read as sentences describing behaviour. The test suite now covers every code path in `DocumentList.svelte` — year, SENDER, RECEIVER, and the fallback case. This is what a complete unit test layer looks like. Ready to merge.

marcel commented 2026-04-20 11:28:01 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

Both concerns from round 1 are resolved.

The conditional CSS is clean:

class:uppercase={sort !== 'SENDER' && sort !== 'RECEIVER'}
class:tracking-widest={sort !== 'SENDER' && sort !== 'RECEIVER'}
class:tracking-wide={sort === 'SENDER' || sort === 'RECEIVER'}

Person names like "Max Mustermann" now render in normal case with tracking-wide — readable, respectful, appropriately weighted. Year labels like "2024" keep the uppercase + tracking-widest style that gives them the archival heading feel. The distinction is deliberate and correct.

The group-card / group-header testid rename is a nice side effect for anyone doing visual regression testing later.

The items-start alignment fix in DocumentRow.svelte is solid — prevents the ProgressRing from stretching when the ContributorStack is taller than expected.

Everything I would have flagged is resolved. Ready to merge.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** Both concerns from round 1 are resolved. The conditional CSS is clean: ```svelte class:uppercase={sort !== 'SENDER' && sort !== 'RECEIVER'} class:tracking-widest={sort !== 'SENDER' && sort !== 'RECEIVER'} class:tracking-wide={sort === 'SENDER' || sort === 'RECEIVER'} ``` Person names like "Max Mustermann" now render in normal case with `tracking-wide` — readable, respectful, appropriately weighted. Year labels like "2024" keep the `uppercase` + `tracking-widest` style that gives them the archival heading feel. The distinction is deliberate and correct. The `group-card` / `group-header` testid rename is a nice side effect for anyone doing visual regression testing later. The `items-start` alignment fix in `DocumentRow.svelte` is solid — prevents the ProgressRing from stretching when the ContributorStack is taller than expected. Everything I would have flagged is resolved. Ready to merge.

marcel commented 2026-04-20 11:28:05 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

Still no infrastructure changes. Pure frontend feature — Svelte components, i18n strings, and Vitest tests. Nothing in docker-compose.yml, CI workflows, environment variables, or deployment config.

LGTM. Ready to merge.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** Still no infrastructure changes. Pure frontend feature — Svelte components, i18n strings, and Vitest tests. Nothing in `docker-compose.yml`, CI workflows, environment variables, or deployment config. LGTM. Ready to merge.

marcel merged commit 2873d8646b into main 2026-04-20 11:29:34 +02:00

marcel deleted branch feat/issue-283-sender-receiver-grouping 2026-04-20 11:29:34 +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

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#284