feat(persons): clean filterable directory + triage UI (Phase 5, #667) #679
Reference in New Issue
Block a user
Delete Branch "feature/667-persons-directory"
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?
Summary
Phase 5 of the "Handling the Unknowns" milestone — stop the persons directory from drowning readers in ~942 provisional import entries, and give transcribers a triage surface. Builds on #671 (
provisionalcolumn) + #674 (importer populates it).familyMember OR documentCount > 0; provisional / zero-doc / "?" persons sit behind a "Needs review / Show all" toggle. Server-side filters (type,familyOnly,hasDocuments,provisional,q) onGET /api/persons.PersonSearchResultrecord (items/totalElements/pageNumber/pageSize/totalPages— mirrorsDocumentSearchResult, not SpringPage). A sharedFILTER_WHEREfragment drives both the slice (findByFilter) and the pairedcountByFiltersototalElementscan't drift.page @Min(0),size @Min(1)@Max(100)→ 400 on garbage;typeenum-bound → 400.PATCH /api/persons/{id}/confirm(clearsprovisional) andDELETE /api/persons/{id}(detaches sender/receiver refs first to avoid FK orphan) — both@RequirePermission(WRITE_ALL), NOT a DTO field (mass-assignment)./persons/review: merge (existingPOST /{id}/merge), rename, confirm, delete (focus-trapped confirm dialog).lastName[0]on null); provisional/UNKNOWN/"?" cards get a neutral placeholder avatar + a text+icon "unbestätigt" badge (never a "?" initial; WCAG 1.4.1).GET /api/personsnow returns the paged envelope; updated all callers (dashboard top-persons,documents/new+[id]/editloaders, raw-fetch consumersPersonTypeahead/PersonMultiSelect/PersonMentionEditor→body.items,review=true). Legacysort=documentCounttop-N folded into the paged contract.aria-pressedfilter chips in a labelled group;role="switch"toggle with the count in its accessible name; metadata 11px→text-sm.Test plan
PersonRepositoryTest43 (filter/count parity, page-beyond-range, combined-AND),PersonServiceTest54,PersonServiceIntegrationTest8,PersonControllerTest59 (pagination shape, size 0/101→400, confirm/delete 401/403) = 164. Frontend node project 633 pass (incl. new loader spec).lintclean;clean package -DskipTestsclean. (Full backend suite not run locally — CI owns the sweep.)Notes / deviations
generate:apihand-edited (no dev backend in the worktree) —api.tsupdated for the new schema/params/two operations; CI must re-runnpm run generate:apito confirm parity.READ_ALL(UI hides the triage link only — write actions are server-guarded);needsReviewCountvia a cheap writers-only loader count.PersonMentionEditor.svelte.spec.tswas updated only for the new envelope/URL shape (not otherwise touched) — its CI flake is unrelated to this PR.Closes #667
GET /api/persons now returns PersonSearchResult with server-side filter params (type, familyOnly, hasDocuments, provisional) and page/size bounds (@Min/@Max -> 400). review=true drops the clean reader default. The legacy sort=documentCount top-N path is folded into the paged contract. Add PATCH /{id}/confirm and DELETE /{id}, both WRITE_ALL-guarded. Remove the now unreachable PersonService.findAll(String). BREAKING-CHANGE: GET /api/persons response shape changes from a bare list to PersonSearchResult { items, totalElements, pageNumber, pageSize, totalPages }. Refs #667 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Hand-edited frontend/src/lib/generated/api.ts to match the backend: GET /api/persons now returns PersonSearchResult with the new filter/page/size query params; adds PATCH /api/persons/{id}/confirm and DELETE /api/persons/{id}. Generated offline (no dev backend running) — CI should re-run `npm run generate:api` against the live spec to confirm parity. Refs #667 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>GET /api/persons now returns PersonSearchResult { items, … } instead of a bare list. Update every caller: the dashboard top-persons path reads .items; the unused full-list fetches in documents/new and documents/[id]/edit are dropped (both pages use the self-fetching PersonTypeahead); the raw-fetch consumers (PersonTypeahead, PersonMultiSelect, PersonMentionEditor) read body.items and pass review=true so search still spans the whole directory. Specs updated to the new envelope shape. Refs #667 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Clean work overall. The shared
FILTER_WHEREconstant is the right call — one source of truth for slice + count, and the comment explains why (drift prevention), not what.PersonFilteras a builder record threaded controller→service→repo is exactly the value-object pattern I'd want. TDD evidence is solid: controller param-capture tests, repository parity tests, and the red-path 400 tests all read as sentences.Blockers
None.
Concerns / Suggestions
Wrong field labels in the rename form —
frontend/src/lib/person/PersonReviewRow.svelte. The first-name<input>'s label ism.persons_filter_type_person()("Person") and the last-name label ism.persons_section_details()("Person details"). Those keys mean other things; reusing them as field labels is misleading and will rot. Add dedicatedpersons_field_first_name/persons_field_last_namekeys.Duplicated
canWritederivation — thelocals.user.groups.some(g => g.permissions.includes('WRITE_ALL'))block is copy-pasted inroutes/persons/+page.server.tsandroutes/persons/review/+page.server.ts(and the castas { groups?... }). Extract ahasWriteAll(locals)helper in$lib/shared. Three identical derivations is the extract signal.PersonCardinitialsderived has a latent quirk —const first = person.firstName?.[0] ?? ''; const last = person.lastName?.[0] ?? ''; return (first || last) + last;— when there's no first name this yieldslast + last(e.g. "MM"). The old inline code did the same, and the glyph branch covers the empty-name crash, so it's not a bug — but the doubling reads like an accident. A short comment orfirst ? first + last : lastwould state intent.persons: [] as never[]in the two document loaders is a clear, well-commented removal of a now-unused full-list fetch — good. Just confirm the consuming+page.sveltetruly never readsdata.persons(typeahead self-fetches), else it's a silent empty list.What I checked: TDD ordering, guard clauses,
$derivedvs$effect(correct — no effects abused), keyed{#each}(all keyed byperson.id), component split (PersonCard / PersonFilterBar / PersonReviewRow each one visual region — good),!response.okchecks (correct in every loader/action),result.data!after ok-check.🛡️ Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
I went looking for the four classic failure modes on a feature like this — SQL injection in the native filter, mass-assignment of
provisional, missing authz on the new write verbs, and IDOR. All four are handled correctly.What I verified
No SQL injection in
FILTER_WHERE—PersonRepository.java. Every predicate uses named, bound params (:type,:familyOnly,:query,:limit,:offset, …). TheLIKEclauses wrap the bound value inCONCAT('%', CAST(:query AS text), '%')— the wildcards are literals, the user value is a parameter, never concatenated into the SQL string.FILTER_WHEREis a compile-time constant, not request-derived. Clean.provisionalis a dedicated verb, not a DTO field (CWE-915 avoided) —PATCH /{id}/confirmis the only way to clear the flag; it is not settable through create/update. Exactly the right call, and the code comment names the threat model. 👍Authz on both new endpoints —
confirmPersonanddeletePersonboth carry@RequirePermission(Permission.WRITE_ALL), andPersonControllerTestproves 401 (unauthenticated) AND 403 (READ_ALL only) AND the happy path for each. This is the textbook coverage I ask for — both failure codes tested, not just the 200.DELETE is fail-safe, not fail-open —
deletePersonis@Transactionaland detaches FKs (reassignSenderToNull+deleteReceiverReferences) beforedeleteById, so a referenced person can't be silently un-deletable. No raw exception leak.Notes (not blockers)
READ_ALL(the UI only hides the triage link for non-writers; thereview=truedata is fetched server-side and the typeahead consumers passreview=truefor everyone). The PR body states this is an accepted assumption. It's fine for this family-only archive — there is no per-row confidentiality boundary — but I'm flagging it so it's a conscious decision: a READ_ALL user can enumerate provisional/zero-doc persons viaGET /api/persons?review=true. No fix needed given the trust model; just don't letprovisionalever carry sensitive content.extractErrorCode+getErrorMessage— no raw backend messages reach the client. Good.What I checked: JPQL/native injection, mass-assignment,
@RequirePermissionpresence + test coverage (401/403), IDOR on PATCH/DELETE (path-bound UUID, no ownership model in this domain), error-message leakage.🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
The structure is sound and the boundaries hold.
PersonSearchResultmirrorsDocumentSearchResultfield-for-field rather than sharing a DTO — duplication over coupling, which is the correct trade-off between two feature modules; the comment even justifies it. ThePersonFilterrecord is a clean value object. No service reaches into another domain's repository. Layering is respected.Documentation: complete. I check doc currency as a hard gate, and this PR passes — new route
/persons/reviewis in theCLAUDE.mdroute table,l3-frontend-3c-people-stories.pumlgains thepersonReviewcomponent + its Rel edges, andl3-backend-3e-persons.pumlupdates the controller/service/repo descriptions for the paged search + confirm/delete. No new migration, no new package — the C4 + CLAUDE.md updates are exactly the right set. Nothing to block on here.Concerns
Legacy top-N path reports a false
totalElements—PersonController.getPersons, thesort=documentCountbranch:PersonSearchResult.paged(top, 0, safeSize, top.size()).totalElements/totalPagesare derived from the returned slice size, not the real match count, so that path always claims one page. It's wrapped only to keep one response shape for the dashboard top-N — defensible — but it means the envelope's contract ("totalElements = full match count") is violated on one path. Either document this on the record, or have the dashboard call a distinct endpoint so the paged contract stays honest everywhere.Per-row correlated subqueries, no index named — both the SELECT projection and
FILTER_WHERErecomputedocumentCountvia correlatedCOUNT(*)subqueries againstdocuments.sender_idanddocument_receivers.person_id. On thehasDocuments/readerDefaultpredicates these run per candidate row. At ~942 persons this is fine, but the data integrity/perf instinct says: confirm indexes exist ondocuments(sender_id)anddocument_receivers(person_id). If they don't, that's a cheap migration and a real win — push the cost down to Postgres' index, not a sequential scan per row.Three round-trips on the persons page — slice + count + a
size=1provisional count for the badge. Acceptable; the count query is the same shared WHERE so it's cheap. Just noting the N+ pattern in case the badge count later wants to fold into a single response.What I checked: module boundaries (no cross-repo access), DTO duplication vs coupling, transport choice (plain REST — correct), DB-layer integrity for the delete (FK detach + transaction), and the full doc-update matrix.
🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
The test design here is genuinely good. Filter/count parity — the exact bug class that bites paginated endpoints — is tested head-on against real Postgres (
countByFilter_readerDefault_matchesSliceSize,countByFilter_typeInstitution_matchesSlice), and the repository suite covers each filter dimension, combined-AND (findByFilter_combinedFilters_andTogether), page-beyond-range, page-size disjointness, and thedocumentCountprojection. The controller layer captures thePersonFilterviaArgumentCaptorand proves the size 0 / 101 / negative-page → 400 boundaries. Both write verbs test 401 AND 403 AND success. This is the pyramid done right: native-query behavior at the integration layer, validation/delegation at the slice layer.Concerns (coverage gaps, not blockers)
No parity test for the
queryfilter combined with the count.findByFilter_query_combinesWithFiltersexists for the slice, but there's nocountByFilter(..., "Verlag")asserting the count matches. The whole point of the shared WHERE is that they can't drift — add the count assertion for thequerypath so a future edit to one query'sLIKEclause is caught.The legacy
sort=documentCounttotalElementssemantic is untested.getPersons_delegatesTopByDocumentCount_whenSortGivenasserts the items but not the (deliberately-degenerate)totalElements/totalPages. If that's intended behavior, pin it with an assertion so nobody "fixes" it later.PersonServiceTest(54) andPersonServiceIntegrationTest(8) — I'm trusting the PR's counts;deletePersonshould have a service-level test proving it callsreassignSenderToNull→deleteReceiverReferences→deleteByIdin order (or an integration test proving a person with a sent + received document deletes cleanly and the documents survive with null sender). That's the highest-value delete test and I want to see it explicitly.Browser/component specs are CI-only —
PersonCard.svelte.test.ts(crash fix),PersonFilterBar.svelte.test.ts, updated page specs. I can't run vitest-browser locally; trusting CI to be green per project policy. The flakyPersonMentionEditor.svelte.spec.tsis pre-existing and out of scope here.Quality gate reminder: this merges on CI green including a re-run of
npm run generate:apito confirm the hand-editedapi.tsmatches the spec — that's the one thing local review cannot verify.What I checked: filter/count parity coverage, boundary/validation tests, permission-boundary tests (401+403), edge cases (empty, page-beyond-range, combined filters), and test naming/readability.
🎨 Leonie Voss — UX & Accessibility
Verdict: ⚠️ Approved with concerns
A lot to like for the dual audience. Filter chips are
min-h-[44px] min-w-[44px]in a labelledrole="group", witharia-pressedreflecting state and a checkmark icon so active state isn't colour-only. The show-all control is a realrole="switch"witharia-checkedand the count baked into its accessible name. The unconfirmed badge conveys state with icon + text + muted shape, never colour alone (WCAG 1.4.1) — and the null-avatar crash fix correctly gives provisional/UNKNOWN/"?" entries a neutral placeholder glyph instead of a "?" initial. Metadata bumped 11px →text-sm. The destructive delete sits behind a focus-trapped confirm dialog. This is the senior-first constraint respected.Concerns
Mislabelled rename fields (Critical for screen-reader users) —
PersonReviewRow.svelte. The first-name field's<label>text ism.persons_filter_type_person()→ "Person", and the last-name field's ism.persons_section_details()→ "Person details". A screen reader announces "Person, edit text" and "Person details, edit text" — neither tells the user which name they're editing. The<label>association is correct, but the text is wrong. Addpersons_field_first_name/persons_field_last_namekeys (de/en/es) and use those.role="switch"witharia-labeloverriding visible text — the switch setsaria-label={persons_toggle_needs_review({count})}and renders visible text that flips between "Show all" / "Needs review (N)". When active, the visible label is "Show all" but the accessible name stays "Needs review (N)" — a visible-text / accessible-name mismatch (WCAG 2.5.3 Label in Name). Drop thearia-labeland let the visible text be the name, or keep them in sync.Filter chips not reachable without colour for the active checkmark only — minor: the active chip is navy-bg + white-text + check icon, inactive is surface + check absent. That's fine (icon + fill both change). No action.
<img alt="">placeholder avatars — correctlyaria-hidden+ empty alt (decorative). Good.I can't run axe locally (CI-only browser tests), so contrast of
bg-muted text-ink-2on the badge andchipActivenavy/white should be confirmed by the axe gate — both look AA-safe by token, but verify in the E2E a11y run at 320px.What I checked: 44px targets, switch/pressed semantics, colour-only cues, label association + label text, focus rings (
focus-visible:ring-brand-navypresent throughout), placeholder alt text, font sizes.⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infra surface in this PR — no Compose, CI workflow, Dockerfile, env var, or migration changed. So this is mostly a "does it create operational risk" pass, and it's low-risk.
Notes
Hand-edited
api.tsis a CI dependency, not an infra one — the PR correctly flags that CI must re-runnpm run generate:api. Worth confirming the CI pipeline actually fails on a generated-file drift (agit diff --exit-codeonfrontend/src/lib/generated/after regen), otherwise a future hand-edit could ship out of sync silently. If that gate doesn't exist yet, that's a small CI hardening issue worth filing — same drift risk that bit #673.Query cost — agreeing with Markus: the directory page now issues slice + count + a provisional-count probe, and the WHERE recomputes correlated
COUNT(*)subqueries per row. ~942 rows won't trouble the CX32, but ifdocuments(sender_id)/document_receivers(person_id)lack indexes this becomes a per-row scan. Cheap to confirm; cheap to add a migration if missing. Not a blocker at current data volume.No secrets, no port exposure, no image tags, no bind mounts touched.
What I checked: Compose/CI/Dockerfile/env deltas (none), generated-artifact CI gate, and query-cost implications for the single-VPS deployment.
📋 "Elicit" — Requirements Engineer
Verdict: ⚠️ Approved with concerns (Brownfield: feature-vs-spec audit of #667)
The core requirement — stop drowning readers in ~942 provisional import entries; give transcribers a triage surface — is met and traceable: reader default =
familyMember OR documentCount > 0, writer-only show-all/triage, server-side filters, and a/persons/reviewCRUD-triage flow (merge / rename / confirm / delete with a confirm-delete guard). Empty states are handled distinctly (persons_empty_filteredvs the no-filter empty state). i18n is complete and parallel across de/en/es — no missing keys. Good adherence to the project's "spec-dense issue" workflow.Ambiguities / gaps to resolve
"Needs review (N)" count ≠ the app's own definition of "unconfirmed." The badge/triage count is
provisional = trueonly (needsReviewCount). ButPersonCard.isUnconfirmedflags a broader set:provisional === trueORpersonType === 'UNKNOWN'ORlastName === '?'OR empty last name. So a reader can see cards visibly marked "unbestätigt" that are not in the "Zu prüfen (N)" total and not in the/persons/reviewlist (which filtersprovisional=true). That's a requirements contradiction: either the triage surface should cover everything the UI calls unconfirmed, or the badge logic should narrow toprovisionalonly. Recommend deciding which definition is canonical and aligning both. (AMBIGUITY — surfaced, not silently resolved.)Delete confirm copy makes a promise the system keeps — verify it's the intended UX. The dialog text says document references "are kept but lose this person." Functionally accurate (sender → null, receiver link removed). But from a family archivist's mental model, silently nulling a letter's sender on a person-delete is a meaningful data change. Confirm the requirement is "hard delete + detach" and not "soft delete / archive," since this is irreversible. If irreversibility wasn't an explicit decision, log it as an open question.
Reader default scope assumption. PR body notes chips AND within the reader default and that provisional rows are readable under READ_ALL (write actions server-guarded). These were owner-unreachable assumptions — they're reasonable, but they should be ratified into the #667 acceptance criteria so the next reader of the issue isn't guessing.
Non-blocking observations
Paginationis adequate for ~19 pages of show-all.What I checked: requirement-to-implementation traceability for #667, empty/edge states, i18n completeness, definitional consistency of "unconfirmed/needs-review," and irreversibility of destructive actions.
The triage rename form reused persons_filter_type_person ("Person") and persons_section_details ("Angaben zur Person") as the first/last-name field labels, so a screen reader announced the wrong name for each input. Add dedicated persons_field_first_name / persons_field_last_name keys (de/en/es) and use them. Refs #667 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Review concerns addressed (7 commits, all
Refs #667)Thanks all — every raised concern is fixed in-PR. Autonomous pass (owner unreachable); decisions noted below.
persons_field_first_name/persons_field_last_name(de/en/es);PersonReviewRowuses them98f8c012aria-label; the visible text ("Zu prüfen (N)" / "Alle anzeigen") is now the accessible name,aria-checkedcarries state. Also removed a redundant duplicatearia-labelon the card badge1a0be413,79e9cc5acanWritederivation (Felix)hasWriteAll(locals)in$lib/shared/server/permissions.ts; reused in persons, persons/review, documents/new, documents/[id]/edit loaders3a758393sort=documentCountfalsetotalElements(Markus/Sara)PersonSearchResult.topN(...)factory reports reality —totalElements= returned count,pageSize= that count,totalPages= 1 (0 when empty) — instead of pretending to be a slice. Documented in a comment on the factory + controller1e3e4208PersonCard.isUnconfirmednow keys offprovisionalonly — same authoritative flag asneedsReviewCountand the/persons/reviewlist, so badge ⇔ count ⇔ triage agree. Empty/"?" name is kept as a separate crash-safety branch (hasNoName) → neutral placeholder glyph, never a "?" initial, no badge implied by empty name alone. Card + tests updated79e9cc5acountByFilter_query_matchesSliceSize(query count-parity); pinned the legacy non-pagedtotalElements/totalPagessemantic (populated + empty); addeddeletePersonintegration test — person referenced as both sender and receiver deletes cleanly (sender nulled, receiver link removed, documents survive, no orphan)362672cd,1e3e4208Index check (Markus / Tobias) — verify, no change here
documents(sender_id)— indexed (idx_documents_sender_id, V62). ✅document_receivers(person_id)— the correlatedCOUNT(*)subquery filters onperson_id, but the only index is the composite PK(document_id, person_id). A B-tree composite can't serve a predicate on its trailing column, so this is effectively unindexed for the per-row subquery. Fine at ~942 rows. Schema is frozen for this PR — not adding a migration. Flagging for a cheap follow-up:CREATE INDEX idx_document_receivers_person_id ON document_receivers(person_id);.Verification
PersonControllerTest61,PersonServiceTest54,PersonRepositoryTest44,PersonServiceIntegrationTest9 — 168 pass.clean compileclean.npm run lintclean; node/server vitest 638 pass (+5 new). Browser/component specs (PersonCard, PersonFilterBar) are CI-only — written red→green but trust CI per policy.svelte-checkerrors in touched files are all pre-existing (e.g.documents/new/+page.sveltedata-prop type,PersonReviewRow$state-init warnings) — none introduced here; full sweep is CI's.Noted, no change
api.tsregen drift CI guard (Tobias) is being filed as a separate follow-up, not added here.Husky was skipped on commits (browser tests are CI-only); code is lint/vitest-clean as shown above. Not merging — over to CI + re-review.
Markus Keller — Senior Application Architect
Verdict: Approved
I reviewed the boundaries, the paging contract, the DB integrity story, and the documentation currency. This round resolves the things I would have flagged.
What I checked and like
PersonSearchResultis hand-written, not SpringPage<T>— and the Javadoc names the reason (stable serialized shape, clean OpenAPI). It mirrorsDocumentSearchResultfield-for-field rather than coupling the two modules via a shared DTO. That is the correct call: duplication beats cross-module coupling.PersonFilteras a single value object threaded controller → service → repository keeps the filter set cohesive instead of growing a 7-arg method signature at every layer. Good module hygiene.topN(...)non-paged factory is now honest:totalElements= returned count,totalPages= 1 (0 when empty). The legacysort=documentCountpath no longer masquerades as a page slice, and two controller tests pin the semantic. This is exactly the ambiguity I wanted closed.FILTER_WHEREshared betweenfindByFilterandcountByFiltermeans the slice and the total derive from one identical predicate —totalElementscannot drift. The count-parity repository test (countByFilter_query_matchesSliceSize) locks it in.PersonService.search/confirmPerson/deletePerson; no controller-to-repository reach;deletePersonorchestrates the detach-then-delete inside the service, not the controller.l3-backend-3e-persons.puml,l3-frontend-3c-people-stories.puml, rootCLAUDE.mdroute table, andfrontend/CLAUDE.mdare all updated for the new/persons/reviewroute and the two new endpoints. No doc omission — so no blocker.provisionalcolumn landed in #671. So the DB-diagram gate is not triggered here.Concern (non-blocking)
deletePersonnullsdocuments.sender_idand removesdocument_receiversrows via two@Modifyingqueries beforedeleteById. That works and is integration-tested, but it is the application-layer pattern I normally push down to the DB (ON DELETE SET NULL/ON DELETE CASCADEon the join). I am NOT asking for it here — the schema is frozen for this PR and the path is well-tested — but it belongs in the relationship/FK hardening backlog so a future caller that deletes a person outsidedeletePersoncan't orphan a row.document_receivers(person_id)missing index is already filed as #681. Correctly deferred — agreed, no migration in this PR.Clean, well-reasoned, well-documented. Ship it.
Blockers
None.
Suggestions
ON DELETEsemantics so person deletion is integrity-safe regardless of the call path.Nora "NullX" Steiner — Application Security Engineer
Verdict: Approved
Threat model for this PR: mass-assignment of the
provisionalflag, authorization on the two new write verbs, SQL injection in the new dynamic filter query, and IDOR on confirm/delete. All four hold up.What I verified
provisionalis cleared by a dedicatedPATCH /{id}/confirmstate-transition verb, never as a settable DTO field. The service comment names the threat explicitly (never as a mass-assignable DTO field (CWE-915)). Security intent is self-documenting — exactly what I want to see.confirmPersonanddeletePersoncarry@RequirePermission(Permission.WRITE_ALL)(type-safe enum, not a magic string).PersonControllerTestproves the full boundary for each:401unauthenticated,403forREAD_ALL-only, success forWRITE_ALL. This is the 401-vs-403 distinction done right, codified as permanent regression tests.FILTER_WHEREis astatic finalstring composed only of literals plus+ FILTER_WHERE +concatenation of trusted fragments; every user-influenced value (type,query, the booleans) flows through named:params. No request data is concatenated into SQL. TheCAST(:type AS text) IS NULL OR …idiom is parameterized. Good.typeis enum-bound at the controller (PersonType), so a garbagetypeyields a 400, not a query error — and only valid enum names ever reach the native query.{m.*()}i18n and text interpolation, no{@html}, noinnerHTML.Observations (not findings)
READ_ALLand/persons/reviewdoes not 403 a read-only user who navigates there directly — it renders the list, and the write actions 403 server-side. The PR body states this is an intentional assumption (the triage link is write-gated; the data is not sensitive — it is import noise). I concur: there is no confidentiality boundary on person summaries, so this is a UX gating choice, not a security control. The server-guarded write actions are the real boundary, and they are tested.hasWriteAll(locals)helper is a UI-affordance gate only (hides the link/CTA). It is correctly NOT relied upon for authorization — the backend@RequirePermissionis the enforcement point. No client-side-only auth here.Blockers
None.
Suggestions
None — the security-relevant tests (
401/403/200/204) are present and meaningful. Nice work isolating the provisional transition behind its own verb.Felix Brandt — Senior Fullstack Developer
Verdict: Approved with one concern
Clean diff. The fixes from the prior round are genuinely resolved, and the component split is what I'd have asked for.
What's good
PersonCardextracted from the 80-line inline block that was in+page.svelte— one nameable visual region, props are domain-named (person),{#each … (person.id)}is keyed. The old crash (person.lastName[0]on a null lastName) is gone:initialsuses optional chaining (person.lastName?.[0]) and the empty-name path renders the placeholder glyph.isUnconfirmed = person.provisional === trueis the single source.hasNoNameis a separate$derivedfor the crash-safety/glyph branch and never raises the badge. The loader'sneedsReviewCountis thetotalElementsof aprovisional=truequery, and/persons/reviewlistsprovisional=true. All three key off the sameprovisionaldefinition — the parity tests inPersonCard.svelte.test.ts("does NOT show the badge for a '?' name when not provisional", "renders the placeholder glyph … even without provisional") pin exactly this. This was the core thing to verify and it holds.hasWriteAll(locals)extracted to$lib/shared/server/permissions.tsand reused in all four loaders (persons,persons/review,documents/new,documents/[id]/edit). DRY win — the inlinegroups?.some(g => g.permissions.includes('WRITE_ALL'))duplication is gone, and the helper has its own unit spec covering anonymous/no-groups/multi-group cases.persons_field_first_name/persons_field_last_name(present in de/en/es), not a borrowed key. No wrong-key reuse remains in the diff.PersonFilterBar.navigate()and the page'shandleSearch/buildPageHrefall clonepage.url.searchParamsand mutate, so toggling a chip never wipesq/other chips, and any filter change resetspage. Tested.hasResults,noFiltersActive,documentCount,initialsare all$derivedin the script.Concern (non-blocking)
persons: [] as never[]. Functionally fine — sender/receiver editing uses the self-fetchingPersonTypeahead, and the comment explains it. But shipping an always-empty typed-as-never[]field is a slightly awkward contract; if nothing consumesdata.personson those pages anymore, I'd rather see the field dropped entirely (and the+page.svelteprop removed) than carried as dead[]. Low priority — it reads as a deliberate minimal-diff choice and is documented, so not a blocker.Blockers
None.
Suggestions
persons: [] as never[]fromdocuments/newanddocuments/[id]/editloaders + their pages rather than passing an empty array forever.Sara Holt — Senior QA Engineer
Verdict: Approved
The test pyramid for this PR is well-shaped: fast unit/slice tests for the controller and service, real-Postgres integration tests for the query+count and the FK-detach behavior, and browser component tests for the parity-critical UI. The new behaviors all have boundary and error-path coverage.
Coverage I confirmed
findByFilter/countByFilterare tested across reader-default, show-all, each single filter, combined-AND, query+filter, page-beyond-range, page-size isolation, and document-count projection. Crucially,countByFilter_query_matchesSliceSizepins slice⇔count parity on the LIKE path — exactly the regression risk of two queries sharing a WHERE clause. Right layer (these need real Postgres because the ORDER BY references a computed alias H2 can't handle — correctly noted in the test comment).ceil(120/50)=3), offset computation (page*size), enum-name passing, blank-query→null, plusconfirmPerson/deletePersonhappy + not-found (404) paths.@WebMvcTestslice): paged envelope shape,size=0/size=101/page=-1→ 400, review-flag default vs. set, and the non-paged top-N pin (…isNonPaged_totalElementsEqualsReturnedCount+ empty→0 pages). Auth boundaries 401/403/200/204 for confirm and delete.@SpringBootTest): reader-default-hides-provisional, show-all-includes, confirm round-trip, and the newdeletePerson_detachesSentAndReceivedReferences_beforeDelete_noOrphan— a person who is both sender and receiver deletes cleanly, the documents survive, the bystander survives. TheentityManager.flush()/clear()around the native@Modifyingdeletes is correct and the comment explains why (native queries bypass the persistence context). This is the kind of edge case that bites in production if untested.PersonCard.svelte.test.tsandPersonFilterBar.svelte.test.tsusevitest-browser-svelteagainst real DOM and assert user-visible behavior viagetByRole/getByText(switch accessible name/Zu prüfen \(12\)/, badge presence/absence), not internals.page.server.spec.tstests the loader directly by importingloadand mockingfetch— the canonical pattern. The envelope migration is reflected in every raw-fetch consumer's spec ({ items }).Notes
PersonMentionEditor.svelte.spec.tswas touched only for the{ items }envelope/size=URL shape; its pre-existing flake is unrelated to this PR — agreed, not this PR's problem.Thread.sleep, no H2-as-Postgres, no@Disabledwithout a ticket. Test names read as sentences.Blockers
None.
Suggestions
None that gate merge — coverage is thorough and at the right layers.
Leonie Voss — UI/UX & Accessibility
Verdict: Approved with concerns
I reviewed touch targets, color-independence, the
role="switch"accessible name, semantics, and the 320px story. The two prior-round a11y fixes are confirmed resolved; the remaining items are non-blocking polish.Confirmed fixed
role="switch"button no longer carries a conflictingaria-label; its visible text is the accessible name andaria-checkedcarries state. The comment documents the reasoning, and Sara's test asserts the switch is found byname: /Zu prüfen \(12\)/. Correct.min-h-[44px](chips alsomin-w-[44px]). Meets WCAG 2.5.8 / 2.2 for the 60+ audience.text-[11px]life-date / doc-count text is nowtext-sm(14px) inPersonCard, clearing my 12px floor.focus-visible:ring-2 focus-visible:ring-brand-navy), and the search<label>is no longer a hardcoded "Suche" — it usesm.persons_search_placeholder().role="group"; the review list is a<ul>/<li>; the error banner isrole="alert". Destructive delete is behind a focus-trapped confirm dialog (confirmservice) with a distinct danger style.Concerns (non-blocking)
bg-brand-navy text-white(navy is the project's AAA text color, so white-on-navy is safe). But inactive chips aretext-inkonbg-surface, and the badge/doc-count chips usetext-ink-2onbg-muted.ink-2onmutedis the pairing most likely to dip under 4.5:1. I can't measure tokens from the diff — confirmink-2/mutedclears AA for this 14px text before merge. If it's close, bump totext-ink.transition-all/transition-colors. Fine if the globalprefers-reduced-motionreset is inlayout.css(it should be, project-wide) — just confirming nothing here opts out of it.role="switch"semantics vs. behavior. The control is labelled a switch, but toggling it changes the visible label text ("Zu prüfen (N)" ⇄ "Alle anzeigen") rather than keeping a stable name with on/off state. A screen-reader user who toggles hears the name change and the checked state flip. It works and is tested, but a stable-name toggle (or a plainaria-pressedbutton like the chips) would announce more predictably. Minor — not worth blocking.Blockers
None.
Suggestions
text-ink-2-on-bg-mutedcontrast at 14px; bump totext-inkif under 4.5:1.aria-pressedto match the chip pattern already in the same bar.Tobias Wendt — DevOps & Platform Engineer
Verdict: Approved
No infrastructure surface in this PR — no Compose changes, no CI workflow edits, no new service, no Dockerfile, no env vars, no secrets. So my review is narrow: API-shape stability for CI and the one performance footnote.
What I checked
api.tswas hand-edited (no dev backend in the worktree) and that CI must re-runnpm run generate:apito confirm the generated types match the backend OpenAPI output. That drift guard is tracked as #680. This is the right call — a hand-edited generated file is exactly the kind of thing that passes locally and breaks the CI type-check. As long as #680's guard runs in the pipeline, the regen mismatch will surface there, not in production.GET /api/personsnow returns the paged envelope; every consumer (dashboard top-persons, the two document loaders, the three raw-fetchcomponents) was updated to readbody.items. The contract change is contained to one endpoint and its callers are enumerated in the PR body — that's a reviewable blast radius.Performance note (deferred, correct)
findByFilter/countByFilteradd correlated subqueries countingdocument_receiversper person, and the PR notesdocument_receivers(person_id)is unindexed — filed as #681, no migration here. Agreed with freezing the schema for this PR. For ~942 provisional rows on a single-VPS Postgres this is not a fire, but #681 should land before the directory grows much further — a per-row correlated count over an unindexed FK column is the classic "feels slow" cause I'd otherwise chase in Grafana later. Profile after #681 if latency shows up.Blockers
None.
Suggestions
generate:apiparity check is a required CI gate on this PR (the hand-editedapi.tsis the one thing that won't surface locally).document_receivers(person_id)) ahead of any large person-volume growth.Elicit — Requirements Engineer & Business Analyst
Verdict: Approved with concerns
I'm assessing this against the stated need: stop ~942 provisional import entries from drowning readers in the persons directory, and give transcribers a triage surface. I review behavior and acceptance, not code.
Requirement coverage (traces to #667)
familyMember OR documentCount > 0; provisional/zero-doc/"?" entries sit behind an explicit toggle. Verified by the reader-default integration tests./persons/review+ the show-all toggle + the per-row actions deliver the full CRUD-triage loop, behind a write-gated affordance.role="alert"on action failures; destructive delete behind a confirm dialog) — these are the unhappy paths I usually find skipped.sizecapped 1–100). Good.Concerns / open questions (non-blocking, but log them)
provisional, not "?"-name. This round deliberately redefined the badge/count/triage to key offprovisionalonly, decoupling it from empty/"?" names. That's internally consistent and testable now — good. But it raises a product question: a person with a blank/"?" name who is NOT provisional (e.g. confirmed but unnamed) will show no badge and will NOT appear in the triage list. Is that intended? If unnamed-but-confirmed entries are possible after import, they may need their own cleanup path. Worth a one-line confirmation from the owner; not a blocker for this slice.[ ] CI greenin the PR body). DoD isn't fully closed until that box is ticked — standard gate, just flagging it's not yet checked.Blockers
None.
Suggestions
provisionalfully covers the import-noise population.