feat: add sorting options to person list page #223

New Issue

marcel · 2026-04-12T08:38:20+02:00

marcel commented

2026-04-12 08:38:20 +02:00

Problem

The person list page only supports searching by name. There's no way to sort persons by relevance metrics, making it hard to discover the most prominent or recently active people in the archive.

Proposal

Add a sort dropdown to the person list page (similar to the document search sort). Possible sort options:

Name (A–Z / Z–A) — current default
Most documents — persons with the most sent + received documents first
Last document date — persons whose most recent document is newest first
Recently updated — persons whose records were edited most recently

Open questions

Does the backend need new query support, or can PersonSummaryDTO (which already has document count) cover most of this?
Should document count be shown on the person card when sorting by that field?
Should this also support the existing search filter (sort + search combined)?

## Problem The person list page only supports searching by name. There's no way to sort persons by relevance metrics, making it hard to discover the most prominent or recently active people in the archive. ## Proposal Add a sort dropdown to the person list page (similar to the document search sort). Possible sort options: - **Name** (A–Z / Z–A) — current default - **Most documents** — persons with the most sent + received documents first - **Last document date** — persons whose most recent document is newest first - **Recently updated** — persons whose records were edited most recently ### Open questions - Does the backend need new query support, or can `PersonSummaryDTO` (which already has document count) cover most of this? - Should document count be shown on the person card when sorting by that field? - Should this also support the existing search filter (sort + search combined)?

marcel added the feature person labels 2026-04-12 08:38:26 +02:00

marcel commented

2026-04-14 21:05:44 +02:00

🧑‍💻 Felix Brandt — Fullstack Developer

Implementation decisions for the person list sort feature — scope, approach, and API contract.

Resolved

1. Sort implementation approach
In-memory sort in PersonService.findAll() using a Comparator. The repository stays untouched — both native queries already return all required data, and the person list is small enough that DB-level sorting is unnecessary. No new query methods needed.

2. "Last document date" — cut from v1
Would require a new lastDocumentDate field on PersonSummaryDTO and a non-trivial MAX(document_date) subquery across both native queries, plus a TypeScript type regen. Not worth the complexity for v1.

3. "Recently updated" — cut from v1
Cut alongside item 2. Can be revisited in a follow-up issue.

4. Sort + search combined
Sort and search always work together — ?q=opa&sort=DOCUMENT_COUNT&dir=DESC is fully supported. With in-memory sorting this is free: findAll(q) returns the filtered list, the comparator is applied after regardless of whether q is set. Sort dropdown stays visible and active while a search query is in use.

5. API contract
GET /api/persons?sort=NAME|DOCUMENT_COUNT&dir=ASC|DESC

Default when params absent: sort=NAME&dir=ASC
Mirrors the existing briefwechsel dir pattern exactly
Maps to a Java enum in the controller and a TypeScript string literal union in the frontend

v1 scope summary

Name A–Z / Z–A ✓
Most documents (desc/asc) ✓
Last document date ✗ (follow-up)
Recently updated ✗ (follow-up)

Clean, minimal change: one new service comparator, one new sort/dir query param pair, frontend sort dropdown wired to URL params.

## 🧑‍💻 Felix Brandt — Fullstack Developer Implementation decisions for the person list sort feature — scope, approach, and API contract. --- ### Resolved **1. Sort implementation approach** In-memory sort in `PersonService.findAll()` using a `Comparator`. The repository stays untouched — both native queries already return all required data, and the person list is small enough that DB-level sorting is unnecessary. No new query methods needed. **2. "Last document date" — cut from v1** Would require a new `lastDocumentDate` field on `PersonSummaryDTO` and a non-trivial `MAX(document_date)` subquery across both native queries, plus a TypeScript type regen. Not worth the complexity for v1. **3. "Recently updated" — cut from v1** Cut alongside item 2. Can be revisited in a follow-up issue. **4. Sort + search combined** Sort and search always work together — `?q=opa&sort=DOCUMENT_COUNT&dir=DESC` is fully supported. With in-memory sorting this is free: `findAll(q)` returns the filtered list, the comparator is applied after regardless of whether `q` is set. Sort dropdown stays visible and active while a search query is in use. **5. API contract** `GET /api/persons?sort=NAME|DOCUMENT_COUNT&dir=ASC|DESC` - Default when params absent: `sort=NAME&dir=ASC` - Mirrors the existing briefwechsel `dir` pattern exactly - Maps to a Java enum in the controller and a TypeScript string literal union in the frontend --- ### v1 scope summary - Name A–Z / Z–A ✓ - Most documents (desc/asc) ✓ - Last document date ✗ (follow-up) - Recently updated ✗ (follow-up) Clean, minimal change: one new service comparator, one new `sort`/`dir` query param pair, frontend sort dropdown wired to URL params.

marcel commented

2026-04-14 21:09:13 +02:00

🧑‍💻 Felix Brandt — Senior Fullstack Developer

Questions & Observations

Comparator placement — The sort logic shouldn't live inline in PersonService.findAll(). A switch over 4 cases inside a service method pushes it past 20 lines fast. Suggest extracting to a package-private PersonSortComparator class or a static factory: PersonSortComparator.by(sort, dir). The service method stays thin.
Enum location — PersonSort { NAME, DOCUMENT_COUNT } is part of the API contract. It belongs in dto/PersonSort.java, not defined inside the controller. Same for a SortDirection enum if one doesn't already exist.
Service signature — PersonService.findAll(String q, PersonSort sort, SortDirection dir) is clear. The default (NAME ASC) should be expressed in the service (or the enum itself), not only via required = false in the controller — keeps the default in one place.
Frontend init pattern — The sort and dir state should use untrack(() => data.filters.sort) on init, same as senderId and sortDir do in briefwechsel. Don't bind directly to data.
TypeScript types — After regen, sort and dir will be string in the generated client. Consider narrowing to a literal union ('NAME' | 'DOCUMENT_COUNT') in a local type alias for the frontend, so the compiler catches typos in goto() calls.

Suggestions

Unit test the comparator in complete isolation — four tests, one per combination: should_sort_by_name_ascending, should_sort_by_name_descending, should_sort_by_document_count_descending, should_sort_by_document_count_ascending. No Spring context needed.
Add a tiebreaker to the DOCUMENT_COUNT comparator — when two persons have equal counts, fall back to name alphabetically. Otherwise sort is non-deterministic on re-renders.

## 🧑‍💻 Felix Brandt — Senior Fullstack Developer ### Questions & Observations - **Comparator placement** — The sort logic shouldn't live inline in `PersonService.findAll()`. A switch over 4 cases inside a service method pushes it past 20 lines fast. Suggest extracting to a package-private `PersonSortComparator` class or a static factory: `PersonSortComparator.by(sort, dir)`. The service method stays thin. - **Enum location** — `PersonSort { NAME, DOCUMENT_COUNT }` is part of the API contract. It belongs in `dto/PersonSort.java`, not defined inside the controller. Same for a `SortDirection` enum if one doesn't already exist. - **Service signature** — `PersonService.findAll(String q, PersonSort sort, SortDirection dir)` is clear. The default (`NAME ASC`) should be expressed in the service (or the enum itself), not only via `required = false` in the controller — keeps the default in one place. - **Frontend init pattern** — The sort and dir state should use `untrack(() => data.filters.sort)` on init, same as `senderId` and `sortDir` do in briefwechsel. Don't bind directly to `data`. - **TypeScript types** — After regen, `sort` and `dir` will be `string` in the generated client. Consider narrowing to a literal union (`'NAME' | 'DOCUMENT_COUNT'`) in a local type alias for the frontend, so the compiler catches typos in `goto()` calls. ### Suggestions - Unit test the comparator in complete isolation — four tests, one per combination: `should_sort_by_name_ascending`, `should_sort_by_name_descending`, `should_sort_by_document_count_descending`, `should_sort_by_document_count_ascending`. No Spring context needed. - Add a tiebreaker to the `DOCUMENT_COUNT` comparator — when two persons have equal counts, fall back to name alphabetically. Otherwise sort is non-deterministic on re-renders.

marcel commented

2026-04-14 21:09:26 +02:00

🏛️ Markus Keller — Application Architect

Questions & Observations

Layer compliance — looks good — Controller accepts params → passes to service → service fetches from repo → applies comparator in memory. Nothing bleeds across boundaries. The sort decision in the discussion comment is architecturally correct.
Enum ownership — PersonSort is an API contract type. It belongs in dto/, not anonymous inside the controller or service. If a future second endpoint (e.g. a stats endpoint) also wants to sort persons, the enum is already in a shared, importable location.
In-memory sort ceiling — The discussion notes this is acceptable for the expected data volume. For the record: a family archive reaching even 2,000 persons would still sort in well under a millisecond in Java. Revisiting the approach would only become relevant above ~50,000 rows, which is not a realistic concern. The simplicity payoff is worth it.
Default sort as a named constant — The NAME ASC default is currently implicit (controller required = false, service fallback). Consider making it an explicit constant:
```
public static final PersonSort DEFAULT_SORT = PersonSort.NAME;
public static final SortDirection DEFAULT_DIR = SortDirection.ASC;
```
This prevents silent divergence if the controller default is updated but the service fallback isn't (or vice versa).

Suggestions

No ADR needed — the discussion comment already documents the in-memory decision and the rationale. That's sufficient for this scope.
The PersonController.getPersons() signature growing to three params (q, sort, dir) is still thin. No concern there — it delegates immediately, which is correct.

## 🏛️ Markus Keller — Application Architect ### Questions & Observations - **Layer compliance — looks good** — Controller accepts params → passes to service → service fetches from repo → applies comparator in memory. Nothing bleeds across boundaries. The sort decision in the discussion comment is architecturally correct. - **Enum ownership** — `PersonSort` is an API contract type. It belongs in `dto/`, not anonymous inside the controller or service. If a future second endpoint (e.g. a stats endpoint) also wants to sort persons, the enum is already in a shared, importable location. - **In-memory sort ceiling** — The discussion notes this is acceptable for the expected data volume. For the record: a family archive reaching even 2,000 persons would still sort in well under a millisecond in Java. Revisiting the approach would only become relevant above ~50,000 rows, which is not a realistic concern. The simplicity payoff is worth it. - **Default sort as a named constant** — The `NAME ASC` default is currently implicit (controller `required = false`, service fallback). Consider making it an explicit constant: ```java public static final PersonSort DEFAULT_SORT = PersonSort.NAME; public static final SortDirection DEFAULT_DIR = SortDirection.ASC; ``` This prevents silent divergence if the controller default is updated but the service fallback isn't (or vice versa). ### Suggestions - No ADR needed — the discussion comment already documents the in-memory decision and the rationale. That's sufficient for this scope. - The `PersonController.getPersons()` signature growing to three params (`q`, `sort`, `dir`) is still thin. No concern there — it delegates immediately, which is correct.

marcel commented

2026-04-14 21:09:32 +02:00

🧪 Sara Holt — QA Engineer

Questions & Observations

The issue has no explicit acceptance criteria. Before implementation starts, I'd want these defined:

Given persons exist, when sort=DOCUMENT_COUNT&dir=DESC, the person with the highest document count appears first
Given q=opa&sort=DOCUMENT_COUNT&dir=DESC, results are filtered AND sorted (search + sort combined)
Given two persons have equal document counts, results are stable with a defined tiebreaker (name A–Z)
Given the sort param is absent, results default to NAME ASC — identical to current behaviour

Edge cases not addressed in the issue:

Tiebreaker — No mention of what happens when multiple persons share the same documentCount. The sort is non-deterministic without a secondary key. Should be name A–Z.
Zero-document persons — When sorting DOCUMENT_COUNT DESC, persons with 0 documents should appear last (not mixed in arbitrarily). This should be an explicit test case.
Invalid param value — What does GET /api/persons?sort=INVALID return? If PersonSort is a Java enum, Spring returns 400 automatically. That behaviour should be tested and documented.
Empty search + sort — q=zzzznothing&sort=DOCUMENT_COUNT should return an empty list, not an error.

Suggestions

Unit test the PersonSortComparator in isolation for all four combinations — no Spring context, fast feedback.
Add a Vitest component test for the sort dropdown: renders with default value, changing selection triggers goto() with the correct params.
Integration test: GET /api/persons?sort=DOCUMENT_COUNT&dir=DESC with seeded data — assert the response order, not just the status code.

## 🧪 Sara Holt — QA Engineer ### Questions & Observations The issue has no explicit acceptance criteria. Before implementation starts, I'd want these defined: - Given persons exist, when `sort=DOCUMENT_COUNT&dir=DESC`, the person with the highest document count appears first - Given `q=opa&sort=DOCUMENT_COUNT&dir=DESC`, results are filtered AND sorted (search + sort combined) - Given two persons have equal document counts, results are stable with a defined tiebreaker (name A–Z) - Given the `sort` param is absent, results default to `NAME ASC` — identical to current behaviour **Edge cases not addressed in the issue:** - **Tiebreaker** — No mention of what happens when multiple persons share the same `documentCount`. The sort is non-deterministic without a secondary key. Should be name A–Z. - **Zero-document persons** — When sorting `DOCUMENT_COUNT DESC`, persons with 0 documents should appear last (not mixed in arbitrarily). This should be an explicit test case. - **Invalid param value** — What does `GET /api/persons?sort=INVALID` return? If `PersonSort` is a Java enum, Spring returns 400 automatically. That behaviour should be tested and documented. - **Empty search + sort** — `q=zzzznothing&sort=DOCUMENT_COUNT` should return an empty list, not an error. ### Suggestions - Unit test the `PersonSortComparator` in isolation for all four combinations — no Spring context, fast feedback. - Add a Vitest component test for the sort dropdown: renders with default value, changing selection triggers `goto()` with the correct params. - Integration test: `GET /api/persons?sort=DOCUMENT_COUNT&dir=DESC` with seeded data — assert the response order, not just the status code.

marcel commented

2026-04-14 21:09:41 +02:00

🔐 Nora Steiner — Application Security

Questions & Observations

This feature is low-risk — read-only, no new data exposure, no auth model changes. A few points worth confirming during implementation:

Enum validation on sort param — If PersonSort is a Java enum and Spring binds it directly via @RequestParam, Spring returns a 400 Bad Request on unrecognized values automatically. This is the correct behaviour and should be tested. If the binding is done manually (e.g. String sort → manual lookup), add explicit validation and a 400 response before the service call.
dir param validation — Same principle. Accept only ASC and DIR (or use a SortDirection enum). Don't let arbitrary strings pass through, even if they're not used in SQL. The in-memory sort means there's no SQL injection vector here, but accepting unvalidated input is a pattern that erodes over time.
No new data exposure — PersonSummaryDTO is unchanged. The sort changes the order, not the content. documentCount is already in the response — no new sensitive fields.
Endpoint authorization — GET /api/persons has no @RequirePermission today, relying on global authentication. That's consistent with the rest of the read endpoints. No change needed for this feature.

Suggestions

Add a test case for GET /api/persons?sort=GARBAGE — assert 400, not 500 or a silently defaulted 200. This confirms the enum binding is working as expected and doesn't silently swallow bad input.

## 🔐 Nora Steiner — Application Security ### Questions & Observations This feature is low-risk — read-only, no new data exposure, no auth model changes. A few points worth confirming during implementation: - **Enum validation on `sort` param** — If `PersonSort` is a Java enum and Spring binds it directly via `@RequestParam`, Spring returns a `400 Bad Request` on unrecognized values automatically. This is the correct behaviour and should be tested. If the binding is done manually (e.g. `String sort` → manual lookup), add explicit validation and a `400` response before the service call. - **`dir` param validation** — Same principle. Accept only `ASC` and `DIR` (or use a `SortDirection` enum). Don't let arbitrary strings pass through, even if they're not used in SQL. The in-memory sort means there's no SQL injection vector here, but accepting unvalidated input is a pattern that erodes over time. - **No new data exposure** — `PersonSummaryDTO` is unchanged. The sort changes the order, not the content. `documentCount` is already in the response — no new sensitive fields. - **Endpoint authorization** — `GET /api/persons` has no `@RequirePermission` today, relying on global authentication. That's consistent with the rest of the read endpoints. No change needed for this feature. ### Suggestions - Add a test case for `GET /api/persons?sort=GARBAGE` — assert `400`, not `500` or a silently defaulted `200`. This confirms the enum binding is working as expected and doesn't silently swallow bad input.

marcel commented

2026-04-14 21:09:53 +02:00

🎨 Leonie Voss — UI/UX Design Lead

Questions & Observations

Header crowding on mobile (320px) — The current header row already has a search input (w-56) and a "New person" button. Adding a sort dropdown as a third control on the same row will overflow on narrow screens. Proposed layout:
- Desktop: [Sort ▼] [Dir ↑↓] [Search ________] [+ New Person] — all in one row
- Mobile: search full-width on top, sort + direction toggle + button in a row below
Sort direction control — The briefwechsel page uses a small toggle button for direction. Reuse that pattern here for consistency — a sort dropdown for the field, an icon button (↑/↓ or A–Z/Z–A label) for direction. Don't put both in a single <select> with options like "Name A–Z" and "Name Z–A" separately — that doubles the option count and hides the relationship.
Accessibility — The sort <select> needs a visible <label> or at minimum aria-label="Sortierung". An icon-only direction toggle needs aria-label with the current direction state, e.g. aria-label="Aufsteigend sortieren".
i18n keys needed — At minimum:
- persons_sort_label ("Sortieren nach")
- persons_sort_name ("Name")
- persons_sort_document_count ("Anzahl Dokumente")
- Direction labels for all three languages (de/en/es)
URL persistence — Sort and dir in the URL (?sort=DOCUMENT_COUNT&dir=DESC) means deep links and browser back button work correctly. Good.

Suggestions

The document count chip is already shown on every person card — no need to conditionally show/hide it based on the active sort. The chip is useful context regardless of sort order.
Consider a subtle visual cue on the card when sorted by document count — e.g. the chip styled slightly more prominently — to reinforce the connection between the sort selection and what the user sees. Low priority, but it aids orientation.

## 🎨 Leonie Voss — UI/UX Design Lead ### Questions & Observations - **Header crowding on mobile (320px)** — The current header row already has a search input (`w-56`) and a "New person" button. Adding a sort dropdown as a third control on the same row will overflow on narrow screens. Proposed layout: - **Desktop**: `[Sort ▼] [Dir ↑↓] [Search ________] [+ New Person]` — all in one row - **Mobile**: search full-width on top, sort + direction toggle + button in a row below - **Sort direction control** — The briefwechsel page uses a small toggle button for direction. Reuse that pattern here for consistency — a sort dropdown for the field, an icon button (↑/↓ or A–Z/Z–A label) for direction. Don't put both in a single `<select>` with options like "Name A–Z" and "Name Z–A" separately — that doubles the option count and hides the relationship. - **Accessibility** — The sort `<select>` needs a visible `<label>` or at minimum `aria-label="Sortierung"`. An icon-only direction toggle needs `aria-label` with the current direction state, e.g. `aria-label="Aufsteigend sortieren"`. - **i18n keys needed** — At minimum: - `persons_sort_label` ("Sortieren nach") - `persons_sort_name` ("Name") - `persons_sort_document_count` ("Anzahl Dokumente") - Direction labels for all three languages (de/en/es) - **URL persistence** — Sort and dir in the URL (`?sort=DOCUMENT_COUNT&dir=DESC`) means deep links and browser back button work correctly. Good. ### Suggestions - The document count chip is already shown on every person card — no need to conditionally show/hide it based on the active sort. The chip is useful context regardless of sort order. - Consider a subtle visual cue on the card when sorted by document count — e.g. the chip styled slightly more prominently — to reinforce the connection between the sort selection and what the user sees. Low priority, but it aids orientation.

marcel commented

2026-04-14 21:09:58 +02:00

⚙️ Tobias Wendt — DevOps & Platform

No infrastructure concerns from my angle. Confirming what I checked:

No schema changes — The in-memory sort decision means no new DB index, no Flyway migration, nothing in db/migration/.
No new env vars or config — sort and dir are query params handled entirely in application code.
No new services or containers — Docker Compose is untouched.
CI pipeline — Existing test jobs cover the new code. No new workflow steps needed.
Observability — Sort params appear in access logs as query string values automatically. No additional logging needed.

One forward-looking note: the in-memory sort approach is the right call for the current data volume. If the archive ever grew to tens of thousands of persons, a DB-side ORDER BY would be more efficient. Not a concern for a family archive, but worth keeping in mind if the codebase is ever adapted for a different use case.

## ⚙️ Tobias Wendt — DevOps & Platform No infrastructure concerns from my angle. Confirming what I checked: - **No schema changes** — The in-memory sort decision means no new DB index, no Flyway migration, nothing in `db/migration/`. - **No new env vars or config** — `sort` and `dir` are query params handled entirely in application code. - **No new services or containers** — Docker Compose is untouched. - **CI pipeline** — Existing test jobs cover the new code. No new workflow steps needed. - **Observability** — Sort params appear in access logs as query string values automatically. No additional logging needed. One forward-looking note: the in-memory sort approach is the right call for the current data volume. If the archive ever grew to tens of thousands of persons, a DB-side `ORDER BY` would be more efficient. Not a concern for a family archive, but worth keeping in mind if the codebase is ever adapted for a different use case.

marcel referenced this issue

2026-04-24 13:24:55 +02:00

feat(persons): visually distinguish incomplete placeholder persons + filter by completeness on /persons #323

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#223