As a reader I want a clean persons directory so I'm not overwhelmed by unconfirmed import entries #667
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Context
After the spreadsheet import,
/personsis a flat grid of ~1100 cards: ~163 curated register persons plus ~942 auto-generated provisional persons (single first-names, group labels, literal?placeholders,UNKNOWN-type rows, zero-document entries). Everything renders in one undifferentiated grid with no type filter, no pagination, and no separation between "real" register entries and import noise. Readers — who just want to browse — are buried under question-mark and zero-document rows.Two audiences (see device-split context):
The fix: make the default directory view clean for readers (family members and people with documents), push the messy triage behind an explicit toggle, paginate the grid server-side, and give transcribers a dedicated triage surface to clean up the import noise.
The reader-facing filter data is already on the wire —
PersonSummaryDTOexposesgetPersonType(),isFamilyMember(),getDocumentCount(). No new summary fields are required for the reader filters; onlyprovisionalis added by the dependency chain.Current behaviour (verified against code)
frontend/src/routes/persons/+page.server.ts:14-15— loadsGET /api/personspassing onlyq; line 30 returns the fullpersonsarray to the page. No type filter, no pagination, no slicing.frontend/src/routes/persons/+page.svelte:93— flat gridgrid-cols-1 sm:grid-cols-2 lg:grid-cols-4iterating every person ({#each data.persons …}, keyed onperson.id— correct, keep). Cards show avatar,PersonTypeBadge, alias, life dates, doc-count chip (only whendocumentCount > 0, line 158).frontend/src/routes/persons/+page.svelte:132— latent null-avatar crash (Felix). The fallback is{person.firstName ? person.firstName[0] : person.lastName[0]}{person.lastName[0]}. IflastNameisnull/empty (a malformed provisional row),person.lastName[0]throws at render. The placeholder-avatar fix below also removes this crash — it is a bug, not polish.frontend/src/routes/persons/+page.svelte:21-26— the debounced searchgoto(\/persons?q=${q}`)**drops every other param** (Felix). Reusing it naively for chips would wipepage/type/reviewon each keystroke. All navigation must compose params viaURLSearchParams/page.url`, never string concatenation.backend/.../person/PersonController.java:36-47—getPersons(q, size, sort). Has a top-NdocumentCountpath (sort=documentCount,size>0,q==null→findTopByDocumentCount); otherwisepersonService.findAll(q). No filter params. The existingsizeparam means "top-N limit" — this collides with the new paginationsizeand must be reconciled to one meaning (Markus, Elicit — see Open Decisions).backend/.../person/PersonService.java:34-46—findAll(q)returnsfindAllWithDocumentCount()(q null),List.of()whenq.isBlank()(line 38-40 — the new filter path must NOT inherit this quirk; emptyq+ active filters returns filtered results, not empty), orsearchWithDocumentCount(q).backend/.../person/PersonSummaryDTO.java:10-22— interface projection (getPersonType()returnsString, not the enum — Felix). Backend filter comparisons must use thePersonTypeenum in the query; the codegen frontend type will bestring.backend/.../person/PersonRepository.java:40-86— native queries use named parameters (no injection surface).findTopByDocumentCountORDER BYs a computed alias and would "silently fail on H2" (note at :72-73). These native queries have noCOUNT(*)companion.backend/.../person/PersonController.java:102-111—POST /api/persons/{id}/mergeexists ({ "targetPersonId": "<uuid>" }, 204).mergePersonsvalidates both ids withfindById…orElseThrow(:184-186) and runs 4 native mutations. The triage view reuses this.backend/.../person/Person.java:55-58— hasfamily_member; noprovisionalcolumn today (comes from #671).PersonTypeincludesUNKNOWN.DELETE /api/persons/{id}— confirmed absent (review finding). The triage "Löschen" action needs it. Confirmed absent — addingDELETE /api/persons/{id}(@RequirePermission(WRITE_ALL)) is in scope here.Dependency
Consumes
Person.provisional(boolean) created by #671 and populated by #669. The "Needs review" toggle and the triage view key off that flag. Until #669 lands, fall back to the heuristic: default/show-all split usesfamilyMember || documentCount > 0; triage approximates provisional asdocumentCount == 0 && !familyMember(pluspersonType == UNKNOWNor a?-only name). Interim caveat (Elicit): the "163 register + 942 provisional" split is only exact once the flag exists — pre-flag, a confirmed register person with zero documents is wrongly pushed into triage. Ship filtering + pagination + UI on the heuristic; wire theprovisionalparam the moment the flag exists.User journey
Reader (phone):
/persons. Sees only family members and people with at least one document — clean, paginated, ≤50 per page.?initial or a wall of empty unconfirmed cards.Transcriber (laptop/tablet):
/persons, flips the "Alle anzeigen / Zu prüfen" toggle to reveal provisional / zero-document / unconfirmed entries inline, orBackend changes
PersonController.getPersons(PersonController.java:36-47)Add filter params (all optional, backward-compatible). Bind
typeto the enum so Spring rejects garbage with 400 (Nora):PersonService.findAll(...). Reconcile the legacysize/sort=documentCounttop-N branch into the new paged contract (it is effectivelypage=0&size=N&sort=documentCount) — do not leave two meanings ofsize(Markus, Elicit).PersonService.findAll(PersonService.java:34-46)PersonFilterrecord (type,familyOnly,hasDocuments,provisional) rather than threading 4 params (Felix). Keeps the controller→service mapping one line.q+ filters into the projection query in the repository (filter-aware native variants or a Specification/CriteriaBuilder approach) — not in-memory.q+ active filters must return filtered results (notList.of()).provisionalmaps toPerson.provisionalonce the flag lands; until then, the documented heuristic.Pagination — hand-written
PersonSearchResultrecord (CORRECTED)PersonSearchResultrecord in thepersonpackage, mirroringDocumentSearchResult's field shape exactly (items,totalElements,pageNumber,pageSize,totalPages). Do NOT import thedocumentDTO (would couple two feature modules — duplication beats coupling) and do NOT use SpringPage<T>.List<PersonSummaryDTO>toPersonSearchResult.sizecap (@Max(100)) also caps a trivial resource-exhaustion vector (Nora). Frontend resetspageto 0 on any filter change.Server-side COUNT query — MANDATORY (CORRECTED)
postgres:16-alpine), NOT H2 — the native queries use Postgres-specific behavior (computed-alias ORDER BY silently fails on H2). A mocked-repo unit test cannot catch a slice/count mismatch.Confirm endpoint — dedicated
PATCH /api/persons/{id}/confirm(RESOLVED)@RequirePermission(Permission.WRITE_ALL).Triage endpoints
GET /api/persons?provisional=true&page=…&size=…— no new list endpoint.POST /api/persons/{id}/merge({ targetPersonId }).PATCH /{id}/confirmabove.DELETE /api/persons/{id}— confirmed absent — addingDELETE /api/persons/{id}(@RequirePermission(WRITE_ALL)) is in scope here.@RequirePermission@RequirePermission(Permission.READ_ALL).@RequirePermission(Permission.WRITE_ALL).OpenAPI regeneration
npm run generate:apiinfrontend/after the response shape changes toPersonSearchResultand after any new param/endpoint. Commit the regeneratedfrontend/src/lib/generated/artifact, or CInpm run checkfails on type drift (Tobias).Frontend changes
frontend/src/routes/persons/+page.server.tspage(clamp ≥ 0),type,familyOnly,hasDocuments, and areview(show-all) flag from the URL.review): restrict the load tofamilyMember || documentCount > 0. Whenreview=1, drop the restriction (and setprovisionalappropriately once the flag exists).page+size=50toapi.GET('/api/persons', …).{ persons, totalElements, totalPages, pageNumber, pageSize, filters, review, needsReviewCount, q, canWrite }.frontend/src/routes/persons/+page.svelteComponent boundaries (one nameable region each — Felix):
PersonFilterBar.svelte— filter chips (Person / Group / Institution, family-only, has-documents) + the "Alle anzeigen / Zu prüfen" toggle. Chips compose URL params viaURLSearchParams, resetpage=0on change, use$derivedfor the active-filter set (never$effectto mirror URL into local$state).PersonCard.svelte— extract the inline card (~75 lines, about to grow). Card fix: for provisional /UNKNOWN-type /?-name persons render a neutral placeholder avatar (theAccount-MD.svgglyph already used inPersonsEmptyState, on a muted non-primary background — visually distinct so "unverified" is pre-attentive) and an "unbestätigt" badge. Replace the line-132 initial fallback so a?name never becomes a?avatar (also fixes the null crash). Bump undersized metadata — life dates and doc-count chip aretext-[11px]; raise to ≥12px, prefertext-smfor the 60+ audience (Leonie).$lib/shared/primitives/Pagination.svelteas-is (already 44px controls,aria-current, sr-only "Seite X von Y" — do NOT rebuild). Import the canonical path, not a worktree copy (Tobias).Triage view (transcriber path)
frontend/src/routes/persons/review/+page.svelte++page.server.ts, loadingGET /api/persons?provisional=true&page=…&size=….PersonReviewRow.svelte(do not grow merge/rename/confirm/delete inline). Each row: placeholder avatar, name (rename), doc count, actions Merge · Umbenennen · Bestätigen · Löschen:POST /api/persons/{id}/merge{ targetPersonId }.PATCH /api/persons/{id}/confirm.canWriteUI gating is presentation only, NOT access control (Nora). The triage data is fetched underREAD_ALL; readers can hitGET /api/persons?provisional=truedirectly. That is acceptable (writers are trusted), but every write action stays permission-guarded server-side — hiding the link is not security./persons, shown only whencanWrite.Accessibility (Leonie)
<button aria-pressed>(orrole="group"of toggles), ≥44px min height, visiblefocus-visible:ring-2 ring-brand-navy, keyboard-operable.role="switch"+aria-checked) or pairedaria-pressedbuttons; the(N)count inside the accessible name, e.g.aria-label="Zu prüfen, 942".aria-label, never color alone.aria-label, ≥44px targets.persons_empty_filteredandpersons_review_emptyas calm centered messages (matchPersonsEmptyState); add a grid skeleton during navigation. Verify chips + toggle with axe in light AND dark mode before merge.i18n —
frontend/messages/{de,en,es}.jsonAdd keys (de shown; mirror for en/es):
Reuse:
persons_search_placeholder,persons_btn_new,person_type_*,person_card_doc_count_*,persons_empty_*, and #315's pagination keys (pagination_prev/pagination_next/pagination_page_of).Doc updates this PR must carry (Markus — blockers)
persons/review/route →CLAUDE.mdroute table anddocs/architecture/c4/l3-frontend-*-people-stories.puml.PersonSearchResult+ confirm (and any delete) endpoint →docs/architecture/c4/l3-backend-*.puml(person domain).Person.provisionalcolumn diagram update (db-orm.puml,db-relationships.puml) belongs to #671, NOT here — do not duplicate.Acceptance criteria
Tests
Backend (integration-first for query logic — Sara)
@DataJpaTest+ Testcontainerspostgres:16-alpine(NEVER H2): filter-aware slice + paired COUNT queries;totalElementsmatches the filtered count exactly;type/familyOnly/hasDocuments/provisionaleach return only matching rows; combined filters AND; default =familyMember || documentCount > 0;page=999→ empty slice + correcttotalPages(not 500); merge round-trip (merged-away person gone, document refs survive — extend existing coverage, don't duplicate); confirm flipsprovisionaland the person then appears in the default reader query.PersonControllerTest(@WebMvcTest):page=0&size=50→ ≤50 items; filter params reach the service (argument captor);size=0/size=101→ 400; confirm endpoint → 403 (READ_ALL only) AND 401 (unauthenticated) as permanent fixtures.Frontend
+page.server.tsloader: default returns onlyfamilyMember || documentCount > 0;review=1drops the restriction; filter +pageparams forwarded to the API client.PersonFilterBarcomponent: activating a chip composes the URL viaURLSearchParams, resets page to 0, preserves other params.PersonCardbadge: a?-name /UNKNOWN/ provisional / null-lastName person renders the placeholder avatar + "unbestätigt" badge, never a?initial, never crashes.POST /api/persons/{id}/mergewith the chosen target.Open Decisions
PersonSearchResultusesitems/totalElements/pageNumber/pageSize/totalPages(matchDocumentSearchResultfor one paged shape across the app). Confirm before implementation; the alternative (Spring-stylecontent/number) leaves two divergent shapes forever. (Markus){count}thetotalElementsof aprovisional=truequery, or a new field on/api/stats? This determines whether the/personsload fires an extra request. Tentative: surfaceneedsReviewCountfrom the loader (cheap count call) rather than overloading/api/stats. (Elicit)familyMember || documentCount > 0default lets a zero-document provisional institution surface in the reader view — re-introducing the noise this feature removes. Alternative: chips AND within the clean default unlessreview=1is set. Decide deliberately; it changes what a reader can stumble into. (Elicit, Markus)READ_ALLusers at all? Tentative: leave readable (matches "all family members are trusted," simplest); the UI just hides the link. Alternative: requireWRITE_ALLto passreview=1/provisional=true(a second check on a read path, divergence from "readers see everything, just can't edit"). (Nora)Out of scope
Person.provisionalschema/flag + migration itself (#671) — this issue only consumes the flag.marcel referenced this issue2026-05-26 21:35:53 +02:00
Architect — Markus Keller (@mkeller)
Observations
document/DocumentSearchResult.javais exactly the hand-written record this issue cites (items,totalElements,pageNumber,pageSize,totalPages) and thatDocumentController.searchalready binds@RequestParam @Min(0) @Max(100_000) int page/@Min(1) @Max(100) int size. Mirroring that shape into a newPersonSearchResult(not importing the document DTO, not SpringPage<T>) is the correct, boundary-respecting choice. Duplication beats coupling here — agreed.DocumentSpecifications+JpaSpecificationExecutor(findAll(spec, pageable)gives you the slice ANDcount(spec)fortotalElementsfrom one Specification).PersonSummaryDTOis an interface projection over a native query, so the Specification path isn't a drop-in — but the issue's "Specification/CriteriaBuilder approach" alternative is the architecturally cleaner one because it gives the paired COUNT for free and eliminates the "native query with no COUNT companion" risk the issue correctly flags. If you stay native, the slice query and the COUNT query MUST share one WHERE-clause builder so they can never drift.size/sort=documentCounttop-N branch inPersonController.getPersons(lines 42-45,Math.min(size, 50)) genuinely collides with the new pagedsize. Verified — this is real, not theoretical. Two meanings of one param is exactly the kind of silent coupling that rots a controller.PATCH /{id}/confirmover a DTO field is the right state-transition modeling. Confirm/merge/delete reusing existing verbs keeps the triage view from spawning a parallel API surface.Recommendations
sizeto one meaning. The legacy top-N isfindTopByDocumentCount(safeSize). Express it aspage=0, size=N, sort=documentCountwithin the new paged contract and delete the special-case branch. One param, one meaning. If the dashboard is the only caller of the old top-N path, confirm that and migrate it in the same PR so you don't leave a dead branch.persons/review/route →CLAUDE.mdroute table +docs/architecture/c4/l3-frontend-*-people-stories.puml; newPersonSearchResultrecord +PATCH /confirm(+DELETEif added) →docs/architecture/c4/l3-backend-*.pumlperson domain. The issue already lists these — good. ThePerson.provisionalcolumn diagram belongs to #671, NOT here; do not duplicate it.familyMember || documentCount > 0for the default split) is the right interim until #669 populates the flag. Keep that logic in the service, not the controller.Open Decisions
items/totalElements/pageNumber/pageSize/totalPagesto matchDocumentSearchResult. I strongly endorse this; the only reason to revisit is if you ever intend to expose SpringPageshapes elsewhere (you don't). Confirm and move on — one paged shape across the app is worth defending.type=INSTITUTIONchip replaces thefamilyMember || documentCount > 0default, a zero-document provisional institution leaks back into the reader view — re-introducing the exact noise this feature removes. ANDing chips within the clean default (unlessreview=1) preserves the invariant. State the intended semantics in the AC before Felix implements, because the Gherkin currently reads ambiguously.Developer — Felix Brandt (@felixbrandt)
Observations
+page.svelte:132is{person.firstName ? person.firstName[0] : person.lastName[0]}{person.lastName[0]}.lastNameis@Column(nullable = false)on the entity, but a provisional/import row or a future bad state makesperson.lastName[0]throw at render. The placeholder-avatar fix removes the crash AND the?-initial problem in one change. Good — treat it as a bug with a failing test, not polish.+page.svelte:24doesgoto(\/persons?q=${q}`). Confirmed. Reusing this for chips would wipepage/type/review. Every navigation must compose viaURLSearchParamsoverpage.url.searchParams, never template-string concatenation. Also note line 12-17 uses$state+$effectto mirrordata.qinto local state — that's the pattern to NOT replicate for filter state. Filter chips read from the URL via$derived; only the debounced text input needs local$state`.PersonSummaryDTOis an interface projection returningString getPersonType()(not the enum). So in the codegen frontend typepersonTypeisstring, and the backend filter comparison must use thePersonTypeenum bound on the controller param. The issue states this correctly.+page.svelteis already ~172 lines with the card inline (~70 lines of it). It is over the splitting threshold now and about to grow. The component breakdown in the issue (PersonFilterBar,PersonCard,PersonReviewRow) is the correct set of one-nameable-region splits.Recommendations
@DataJpaTest(Testcontainerspostgres:16-alpine) — write the failing filter+count test before the repository query exists. The mismatch between slice and COUNT only shows up against real Postgres.PersonControllerTest(@WebMvcTest) —size=0/size=101→ 400; confirm endpoint 403 (READ_ALL) AND 401 (unauthenticated). Argument-captor that filter params reach the service.+page.svelte:132crash test (vitest-browser-svelte) — render a person withlastName: null/ name"?", assert placeholder avatar +unbestätigtbadge, no throw. Watch it go red against current code.PersonFilterBar— activating a chip composes the URL viaURLSearchParams, resetspage=0, preserves other params.PersonFilterrecord (type,familyOnly,hasDocuments,provisional) for the controller→service hop. Threading fourBooleanparams plus an enum is a smell; a record keepsfindAll(...)to one parameter and is@Builder-friendly for tests.Pagination.svelteverbatim. I read it — 44px controls, sliding window,aria-current, sr-only "Seite X von Y". It takespage,totalPages,makeHref.makeHrefis where you compose the URL preserving filters. Do NOT rebuild it; import$lib/shared/primitives/Pagination.svelte.dataobject.<PersonCard person={...} />,<PersonReviewRow person={...} canWrite={...} />. Keep{#each ... (person.id)}keyed (the current code already does — keep it).!result.response.ok, map errors viagetErrorMessage(). Delete behind a focus-trapped confirm dialog.Open Decisions
None. The component boundaries, the
PersonSearchResultshape, and the confirm-endpoint approach are all decided correctly in the body. Naming and test layering are mine to execute, not decisions for the PO.Security — Nora Steiner / "NullX" (@nullx)
Observations
PATCH /{id}/confirmdecision correctly closes a mass-assignment vector (CWE-915). Ifprovisionalwere a settable field onPersonUpdateDTO, it would be assignable on bothPUT /{id}(update) andPOST(create) viapersonService.createPerson(dto)/updatePerson(id, dto)— verified those methods copy DTO fields straight onto the entity. A dedicated verb is the right state transition and cannot be smuggled in through the edit form. Endorsed.canWriteUI gating is presentation, not access control — the body says this and it's correct. The triage data is fetched underREAD_ALL(GET /api/personscarries@RequirePermission(READ_ALL)). AREAD_ALLuser CAN hit?provisional=truedirectly. That's acceptable here (writers/readers are all trusted family members) but the security boundary is the server-side@RequirePermission(WRITE_ALL)on every write verb — hiding the link is UX, not a control.typeto thePersonTypeenum makes Spring reject garbage with 400 automatically — good input-validation-at-the-boundary. The new filter params feed into either a Specification or native query with named parameters; the existing person native queries already use:querynamed params (no injection surface). Keep it that way — no string concatenation into the new filter SQL.@Max(100)size cap also bounds a trivial resource-exhaustion vector (nosize=100000returning the whole table). Correct.Recommendations
PATCH /{id}/confirm→ 403 when caller has onlyREAD_ALL, 401 when unauthenticated. These are different security failures; assert both.DELETE /api/persons/{id}is added (see below), it needs the identical 403/401 pair plus@RequirePermission(WRITE_ALL).DELETE /api/persons/{id}does not exist today — I grepped the person package; only/{id}/aliases/{aliasId}andrelationships/{relId}deletes exist. The triage "Löschen" action requires a new person-delete endpoint. When you add it:@RequirePermission(WRITE_ALL),DomainException.notFound(...)for a missing id (neverOptional.get()), and decide cascade behaviour for a person that is still a document sender/receiver — a hard delete that orphans an FK is a data-integrity bug, not just a 500. Prefer a guard that refuses to delete a person with live document references, or document the cascade explicitly.getErrorMessage(code)on the frontend — no raw backend strings in the triage UI.READ_ALLendpoint do not expand the auth surface, but confirm no filter value can be reflected into a log line unsanitized (use SLF4J{}placeholders if you log the filter).Open Decisions
provisional=true/review=1be reachable byREAD_ALLusers at all? Tentative (body): leave readable — consistent with "all family members are trusted; readers just can't edit," and the simplest model (no second check on a read path). Alternative: gatereview=1/provisional=truebehindWRITE_ALL, which diverges from "readers see everything, just can't change it" and adds a permission check on a GET. This is a trust-model decision, not a vulnerability — pick the model deliberately and write the AC to match. The current AC ("the read API may still serve provisional rows on direct request") encodes the leave-readable choice; confirm that's intended.QA / Test Strategy — Sara Holt (@saraholt)
Observations
postgres:16-alpine(never H2), and the explicit "a mocked-repo unit test cannot catch a slice/count mismatch" reasoning. That's the correct instinct — the existing person native queries even carry a comment that computed-aliasORDER BY documentCount"would silently fail on H2." H2 here would be false confidence.totalElementsdrivingtotalPagesdriving the AC "does not return all ~1100 rows" — if the COUNT doesn't match the filtered slice's WHERE clause exactly, pagination lies and no UI test will catch it. Make the assertiontotalElements == filtered-fixture-countfor each filter permutation.@DataJpaTest), not Playwright.backend/CLAUDE.md), not 80%. The new filter branches (each chip on/off, default vsreview=1,pagebeyond range) are branch-heavy — budget tests for every branch or the gate blocks the merge.Recommendations
page=999(beyond last page) → empty slice + correcttotalPages, not 500.size=0andsize=101→ 400 (@Min(1) @Max(100)).familyMember || documentCount > 0; a zero-document non-family person is absent.review=1drops the restriction and the same zero-document person now appears.provisionaland the person then appears in the default reader query (round-trip, one test).+page.server.tsloader tests (plain TS, mock onlyfetch): default restriction applied;review=1removes it;page+filter params forwarded to the API client.PersonCardcrash regression test is non-negotiable —lastName: null/ name"?"must render the placeholder +unbestätigt, never throw. This stays in the suite forever; it's a real production crash.@DataJpaTest. Controller contract (status codes, param binding) →@WebMvcTestwith argument captor. User-visible behaviour →vitest-browser-svelte. Only the two journeys → Playwright./personsand/persons/reviewin both light and dark mode is a gate (coordinate with Leonie). The filter chips and the show-all switch are new interactive surfaces — they get axe coverage before merge.Open Decisions
None. The test strategy is well-specified. My only ask is that the COUNT/slice parity assertion and the null-lastName crash test are treated as must-have permanent fixtures, not optional — and that's an execution standard, not a decision for the PO.
UX / Accessibility — Leonie Voss (@leonievoss)
Observations
?cards. That framing is right.+page.svelte:152life dates aretext-[11px]and:160the doc-count chip istext-[11px]. 11px is below my 12px hard floor and is genuinely hard to read for the senior audience. Raise to ≥12px; prefertext-sm(14px) for life dates and the chip.Pagination.svelteis solid — I read it:min-h-[44px] min-w-[44px]controls,focus-visible:ring-2 focus-visible:ring-brand-navy,aria-current="page", an sr-only "Seite X von Y" that stays in the AT tree at every breakpoint, and ellipsis spans arearia-hidden. Reuse it verbatim; rebuilding would regress all of this.Account-MD.svgglyph on a muted non-primary background +unbestätigtbadge) gives the redundant cues I require: shape + text, never color alone. Good — and it doubles as the crash fix.Recommendations (exact values)
<button aria-pressed={active}>,min-h-[44px] min-w-[44px] px-4 py-2,focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:ring-offset-2 outline-none. Active state must differ by more than color — add a check glyph or a filled background plus thearia-pressedstate. Group them in arole="group"with anaria-label("Filter").role="switch"+aria-checked, OR pairedaria-pressedbuttons. Put the count inside the accessible name:aria-label="Zu prüfen, 942"— a bare "(942)" is announced as a meaningless number out of context.unbestätigtbadge: placeholder avatar (shape) + visible "unbestätigt" text +aria-labelon the card. Use a token, not raw color, and verify contrast in dark mode — a muted badge background that passes AA in light mode can fail in dark.persons_empty_filteredandpersons_review_emptyas calm centered messages matchingPersonsEmptyState; add a grid skeleton during navigation so the reader isn't staring at a blank page on a slow phone.bg-primarywithtext-primary-fginitials — the provisional placeholder must be visually distinct (muted, non-primary) so "unverified" is pre-attentive, not something you have to read to discover./personsand/persons/reviewin light AND dark mode; visual regression at 320 / 768 / 1440px before merge. Chips and toggle wrap, not overflow, at 320px.Open Decisions
None from my side. The accessibility requirements are concrete and already in the body; these are exact-value clarifications for implementation, not tradeoffs needing your input.
DevOps / Platform — Tobias Wendt (@tobiwendt)
Observations
List<PersonSummaryDTO>toPersonSearchResult, plus new params and a new endpoint. Iffrontend/src/lib/generated/isn't regenerated and committed, CInpm run checkfails on type drift — that's a red pipeline, not a runtime issue, but it blocks the merge and wastes a CI cycle.npm run generate:apirequires the backend running with--spring.profiles.active=dev.Recommendations
@DataJpaTestcount-query tests don't need a new CI capability. They don't.Pagination.sveltefrom the canonical path$lib/shared/primitives/Pagination.svelte— not a worktree copy. A divergent copy is a maintenance trap that silently rots when the original gets a fix.Open Decisions
None. No infrastructure decision is required for this issue. The only operational must-do — regenerate and commit the API types — is a checklist item, not a tradeoff.
Requirements / Business Analysis — Elicit
Observations
?/null-lastName card, server-side filtering, the full triage CRUD, confirm round-trip, the permission boundary, the "hiding the link is not access control" case, empty states, and i18n coverage. INVEST-wise the story is Valuable, Testable, and Estimable. My concern is Independent/Small: it depends on #671→#669 for the exact split and bundles reader directory + transcriber triage in one issue.provisional-flag, the heuristicdocumentCount == 0 && !familyMemberwrongly pushes a confirmed register person with zero documents into triage and out of the reader default. The "163 + 942" split is only exact once #669 populates the flag. That is acceptable to ship on the heuristic — but it should be stated as a known interim limitation in the PR, not discovered later as a "bug."{count}comes from a separateprovisional=truecount query fired on every/personsload, that's an extra request on the reader's hot path — most of whom never triage. This is a measurable NFR question (one extra round-trip per page load), not cosmetic.Recommendations
review=1. One sentence in the AC removes a whole class of "is this a bug?" later. (Markus raises the same.)needsReviewCount. Tentative recommendation from the body — surface it from the loader as a cheap count rather than overloading/api/stats— is the right call: it keeps/api/statsfrom accreting feature-specific fields and keeps the cost on the page that needs it. Confirm and write it down.?-name or zero-doc card; a transcriber's triage list should contain exactly the import noise. Until #669, the heuristic is the contract — document it so QA's fixtures and the PO's expectations match.Open Decisions
review=1. Decide based on what a reader should be allowed to stumble into.needsReviewCountsource: loader count call vs/api/statsfield. Loader call = one extra request per/personsload, no cross-feature coupling./api/statsfield = no extra request but/api/statsgrows a triage-specific field used by one page. Tradeoff is request-count vs endpoint cohesion.Decision Queue — Action Required
5 decisions need your input before implementation starts. Everything else in the body is resolved; personas made concrete recommendations elsewhere.
Behaviour / UX
type=INSTITUTIONchip replaces thefamilyMember || documentCount > 0default, a zero-document provisional institution leaks back into the reader view — re-introducing the exact noise this feature removes. ANDing chips within the clean default (unlessreview=1is set) preserves the invariant, at the cost that a reader can't browse ALL institutions (incl. empty ones) without flipping the toggle. The current Gherkin reads as "replace" but the feature intent implies "AND". Pick one and pin it in the AC. (Raised by: Markus, Elicit)Architecture
items/totalElements/pageNumber/pageSize/totalPages. The body resolves toward matchingDocumentSearchResult(verified to use exactly this shape). Strongly endorsed — one paged shape across the app. Only revisit if you ever intend to expose SpringPage(content/number) shapes; the alternative leaves two divergent shapes forever. Likely a rubber-stamp. (Raised by: Markus)needsReviewCountsource: loader count call vs a new/api/statsfield. Loader call = one extra request per/personsload, no cross-feature coupling./api/statsfield = no extra request, but/api/statsgrows a triage-specific field used by one page. Tradeoff is request-count vs endpoint cohesion. Tentative lean: loader count. (Raised by: Elicit)Security / Trust model
READ_ALLusers at all? Tentative (body + AC): leave readable — consistent with "all family members are trusted; readers just can't edit," simplest, no second check on a GET. Alternative: gatereview=1/provisional=truebehindWRITE_ALL, diverging from "readers see everything, just can't change it." This is a trust-model choice, not a vulnerability. Confirm the leave-readable AC is intended. (Raised by: Nora)Scope / sequencing
provisional? Ship-now delivers pagination + filtering + the null-avatar crash fix immediately, but the reader/triage split is approximate until #669 (a confirmed register person with zero documents is wrongly pushed into triage in the interim). Waiting gives an exact split but blocks all user-visible value behind the full #671→#669 chain. The body leans ship-now with the heuristic documented as a known interim limitation — confirm that's the intent. (Raised by: Elicit)