feat: Persons section redesign — Concept A (Enriched Directory) #157
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?
Overview
Redesign of the Persons section (
/persons,/persons/[id],/persons/new) based on a UX audit across mobile, tablet, and desktop. Concept A ("Enriched Directory") takes an evolutionary approach — enrich what exists without a structural overhaul.📄 Full wireframe spec → docs/specs/persons-concept-a-enriched-directory.html
UX Audit — Problems being fixed
PLACEHOLDER,UPLOADED)Proposed changes
Routes
/persons/persons/new/persons/[id]/persons/[id]/editPerson list (
/persons)* 1882 – † 1944), document count chipPerson detail (
/persons/[id]) — 2-column layout on desktop (≥1024px)Edit person (
/persons/[id]/edit) — new dedicated routeDiscard changes|Save changesPersonMergePanel.sveltemoves here from the detail pageNew person (
/persons/new)Breakpoints
<640pxMobile640–1023pxTablet≥1024pxDesktopFiles to create / modify
src/routes/persons/+page.sveltesrc/routes/persons/+page.server.tsdocumentCountto PersonDTOsrc/routes/persons/new/+page.sveltesrc/routes/persons/new/+page.server.tssrc/routes/persons/[id]/+page.sveltesrc/routes/persons/[id]/PersonCard.sveltesrc/routes/persons/[id]/PersonMergePanel.sveltesrc/routes/persons/[id]/edit/+page.sveltesrc/routes/persons/[id]/edit/+page.server.tssrc/lib/i18n/messages/*.jsonAcceptance criteria
/persons/newhas all fields: first name, last name, alias, birth year, death year, notes/persons/[id]shows 2-column layout on desktop (≥1024px), stacked on smaller/persons/[id]/edit(no in-place toggle)/persons/[id]/edithas sticky save bar with Discard and Save changesSpec authored by Leonie Voss (UI/UX persona) · Concept A · v1.0 · 2026-03
Wireframe spec: docs/specs/persons-concept-a-enriched-directory.html
Architectural Review — @mkeller
The UX spec is sound and the proposed file changes are appropriate. The route split (
[id]→ view-only,[id]/edit→ form + danger zone) is the right call — it removes the awkward inline-edit toggle and gives the merge panel a natural home. No structural concerns with the SvelteKit side.There are three backend gaps that need resolving before implementation starts. None are large, but two will require deliberate query design and one is an N+1 trap if done naively.
Gap 1:
POST /api/personscannot accept the new fieldsCurrent state:
PersonController.createPerson()takesMap<String, String>and passes three positional strings toPersonService.createPerson(String, String, String).birthYearanddeathYearare integers — they cannot be added to this endpoint without parsing them from strings in the controller, which is controller logic leaking into the wrong layer.What the spec needs: The
/persons/newform will submitbirthYear,deathYear, andnoteson creation.Recommendation:
PersonCreateDTO(or reusePersonUpdateDTO— the fields are identical) as the request body forPOST /api/persons.PersonService.createPerson()to accept the full DTO, same pattern asupdatePerson().npm run generate:apiafter — the frontend create action currently sendsfirstName/lastName/aliasas a raw JSON body; it needs to match the new DTO.This is migration-free —
birthYear/deathYear/notesalready exist on the entity.Gap 2: Document count on person cards — N+1 risk
What the spec asks for: Each person card on
/personsshows a document count chip. The spec notes this as "Optionally adddocumentCountto PersonDTO".Current state:
GET /api/personsreturns barePersonentities. IfdocumentCountis added naively (a@Formulasubquery per row, or a loop callingdocumentServiceper person), you fire one extra SQL query per person row.Recommendation: Add a single native query that returns the count in the same round-trip:
Surface this via a
PersonSummaryDTO(a record with allPersonfields plusdocumentCount) returned only byGET /api/persons. TheGET /api/persons/{id}detail endpoint keeps returning the rawPersonentity — no document count needed there.Trade-off: you add a DTO and a new repository method. Cost is low. N+1 on a list page is not acceptable.
Gap 3: Stats bar totals require a new API surface
What the spec asks for:
"4 persons · 12 documents"on the list page.Current state: No endpoint returns aggregate counts. The frontend could derive
persons.lengthclient-side buttotalDocumentCountis not available anywhere.Options:
{ persons: [...], totalDocuments: N }generate:apiGET /api/statsreturning{ totalPersons, totalDocuments }totalPersonsfrompersons.lengthclient-side, add onlytotalDocumentsto the list responseRecommendation: Add
GET /api/stats— a single endpoint, twoCOUNT(*)queries. Keep the persons list response shape unchanged. The frontend load function calls both inPromise.all(). Useful beyond this feature (dashboard widgets, admin page).What needs no backend changes
/persons/[id]/editroute — moveupdateandmergeactions from[id]/+page.server.tsto[id]/edit/+page.server.ts. Backend endpoints unchanged. Theredirect(303, '/persons/${targetPersonId}')after merge still works.One existing issue worth fixing while you're here
PersonService.getById()andupdatePerson()throwResponseStatusExceptiondirectly. The rest of the codebase usesDomainException. Not a blocker, but if you touchPersonServiceto extendcreatePerson(), standardise the error throwing in the same commit.Summary of backend work required
PersonCreateDTO+ updatePOST /api/personsPersonController,PersonService,PersonCreateDTO(new)PersonSummaryDTOwithdocumentCount+ native queryPersonRepository,PersonService,PersonController,PersonSummaryDTO(new)GET /api/statsendpointStatsControlleror add to existingDo the backend changes first, regenerate the API types, then implement the frontend. Do not start the SvelteKit work against stale types.
Test Scope — Concept A Persons Redesign
Sara Holt (@saraholt) · QA
Layer 1 — Static Analysis
svelte-check+ TypeScript strict + ESLint run on every commit as usual. Any new$libimport path that isn't typed is a CI blocker before anything else runs.Layer 2 — Unit Tests
src/lib/utils/personStatus.test.ts(new)maps PLACEHOLDER to human-readable labelmaps UPLOADED to human-readable labelmaps TRANSCRIBED, REVIEWED, ARCHIVEDreturns fallback for unknown statusformats life date range with both dates* 1882 – † 1944formats life date range with birth year only* 1882— no death yearformats life date range with neither* –src/lib/utils/personStats.test.ts(if stats bar logic is extracted)formats stats bar with plural personsformats stats bar with singularhandles zero countsLayer 3 — Integration Tests
src/routes/persons/[id]/edit/+page.server.test.ts(new — critical)load returns person data for valid idload throws 404 for unknown person idload throws 403 for unauthenticated userupdate action saves all fields including birth year, death year, notesupdate action returns validation error for invalid birth yeardiscard action redirects to /persons/[id] without savingmerge action delegates to PersonService and redirectsmerge action returns 404 when target person does not existsrc/routes/persons/new/+page.server.test.ts(modify existing)create action accepts birth year, death year, notescreate action succeeds with only required fields (firstName, lastName)src/routes/persons/+page.server.test.ts(modify existing, only ifdocumentCountadded to PersonDTO)load returns documentCount per personload returns total document count for stats barLayer 4 — E2E Tests (Playwright)
checkA11y()runs on every page entry and every navigation target.e2e/persons/persons-list.spec.ts(modify)stats bar shows person count and document countperson card shows alias when setperson card shows life date range when setperson card shows document count chipempty state shown when no persons existsearch filters persons and shows result countaccessibility: /persons passes axe at all breakpointse2e/persons/person-detail.spec.ts(modify)2-column layout renders on desktop (1440px)stacked single-column on tablet (768px)stacked single-column on mobile (375px)Full Name label is not visible in detail viewEdit person button links to /persons/[id]/editco-correspondent chips show tooltip or icondocument status shows human-readable string not raw enumaccessibility: /persons/[id] passes axe at all breakpointse2e/persons/person-edit.spec.ts(new)edit page loads with all fields pre-filledsticky save bar remains visible while scrolling long notesSave changes persists all fields and redirects to detailDiscard changes returns to detail without savingDanger Zone accordion is collapsed on page loadDanger Zone expands on click and shows merge panelmerge flow completes from Danger Zoneaccessibility: /persons/[id]/edit passes axee2e/persons/person-new.spec.ts(modify)new person form has birth year, death year, notes fieldscreate with all fields succeeds and redirects to detailcreate with only required fields succeedsVisual regression — run at 375px, 768px, 1440px in light and dark mode for all four routes (
/persons,/persons/[id],/persons/[id]/edit,/persons/new). That's 24 snapshots. All diffs block merge pending Leonie's review.Testability Concerns
1.
documentCounton PersonDTO — needs a@DataJpaTestslice if computed via JOIN/subquery. A mock would let a broken query ship silently.2. Danger Zone accordion — assert behavior, not class names. Test that the merge panel is hidden/visible, not that a CSS class is present. The test must survive implementation changes.
3. Merge moved to edit route — explicit regression required. The E2E merge test must use
/persons/[id]/edit. Verify no stale action handler remains on the old/persons/[id]path.4. Status label mapping — must be extracted and unit-tested. If the enum → display string mapping lives inline in the template, it's untestable. Extract it to a utility function. Any future
DocumentStatusaddition will otherwise regress silently.Estimated CI impact
Total stays well within the 8-minute E2E budget.
Blockers before first test can be written
documentCountadded toPersonDTOor computed in the frontend load function? Determines which layer owns that test.Security Review — Concept A Implementation
Reviewed by: Nora "NullX" Steiner · Application Security Engineer
Scope: Existing persons surface + all proposed changes in this issue
🔴 Finding 1 — AUTHORIZATION BYPASS: Write endpoints have no permission check
Severity: High | CWE-285 (Improper Authorization)
PersonController.javahas no@RequirePermissionat class or method level on any mutating endpoint. Spring Security requires authentication (anyRequest().authenticated()) but that only gates identity — it does not gate write access. Any authenticated user with onlyREAD_ALLcan call these directly:Fix — add
@RequirePermissionto each write endpoint:🔴 Finding 2 — AUTHORIZATION BYPASS: Form actions have no permission guard
Severity: High | CWE-285
/persons/new/+page.server.tscorrectly guards with aWRITE_ALLcheck inload(). The existing/persons/[id]/+page.server.tshas no equivalent guard on itsupdateandmergeactions — any authenticated user can POST to?/updateor?/mergedirectly, bypassing the UI.Critical for this issue: the spec moves these actions to the new
/persons/[id]/edit/+page.server.tsbut does not mention adding a permission guard. It will be missing unless explicitly added.Fix — add to
load()in both the existing[id]/+page.server.tsand the new/edit/+page.server.ts:🟠 Finding 3 — No input length limits on string fields
Severity: Medium | CWE-400 (Uncontrolled Resource Consumption)
PersonUpdateDTOhas no@Sizeconstraints. Thenotesfield being added to the new/edit forms is especially at risk — a write-access user could store a very large value, bloating every API response that returns aPerson(search results, correspondent lists, document sender/receiver data).Fix — add
@Size+@Valid:Add
@Validto theupdatePersoncontroller parameter to activate validation.🟡 Finding 4 — No absolute range bounds on year fields
Severity: Low | Data integrity
birthYearanddeathYearare cross-validated against each other but accept any integer (e.g.-9999,99999). Both new forms surface these fields to users.Fix — add a range check in
PersonService.updatePerson:✅ Things that are fine
PersonRepositoryuse named@Parambindings — no injection risk in the search or merge helpersSecurityConfig.javamergePersonscorrectly rejectssourceId.equals(targetId)before touching the DBPersonMergePanel.svelteis a solid UX safeguard; moving it behind the Danger Zone accordion makes it even better/persons/<targetPersonId>— not an open redirectImplementation checklist
@RequirePermission(WRITE_ALL)to write endpointsPersonController.javaWRITE_ALLguard to existing[id]/+page.server.tsand the new/edit/+page.server.ts@Sizeconstraints +@ValidPersonUpdateDTO.java,PersonController.javaPersonService.javaFindings 1 and 2 should be fixed before or during implementation — not as follow-up issues. Finding 2 in particular is easy to introduce when creating the new edit route if the spec doesn't call it out explicitly.