feat(transcription): person @mention sidecar + rename propagation (PR-A backend, #362) #366
Reference in New Issue
Block a user
Delete Branch "feat/person-mentions-issue-362-backend"
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
PR-A of the two-PR delivery for #362 (Person @mentions in transcription blocks). This is the backend half: data model, rename-propagation pipeline, security lockdown. Implemented strictly to the locked plan in the consolidation comment #362#issuecomment-5339 — Section F closes with "Open decisions: none." PR-B (frontend) is intentionally separate and blocked on this PR merging because it consumes the regenerated
api.tstypes committed here.Why now
Reader Experience v1 milestone needs in-text person discovery. This PR puts the data model and rename atomicity in place so the frontend can layer the typeahead, hover card, and read-mode link rendering on top in PR-B without further backend changes.
What landed (21 atomic commits)
Data layer
transcription_block_mentioned_persons(block_id + person_id + display_name). No FK onperson_idso deleted persons degrade gracefully. B-tree index onperson_idfor the future "blocks mentioning person X" query, plus an index onblock_idfor the @ElementCollection load.PersonMention @Embeddablevalue object,@NotNull personId,@Size(max=200) displayName.PersonDisplayNameChangedEventrecord — first custom application event in this codebase; class doc sets the convention for future cross-domain decoupling.TranscriptionBlock.mentionedPersonsfield via@ElementCollection(LAZY)with explicit@CollectionTablematching the migration name byte-for-byte.DTO + controller validation
CreateTranscriptionBlockDTOandUpdateTranscriptionBlockDTOaccept@Valid List<PersonMention> mentionedPersons(on the field, not just on the method, so JSR-303 recurses into list elements).@Validadded to the@RequestBodyparameters ofcreateBlock/updateBlock.VALIDATION_ERRORfordisplayNamelength 201 and for nullpersonId— regression guards if anyone drops@Validlater.Person rename → propagation
PersonService.updatePersonsnapshotsoldDisplayNamebefore any setter, saves, re-readsnewDisplayName, publishesPersonDisplayNameChangedEventiff the two differ.PersonServiceTestcovers title-publishes / alias-no-publish / notes-no-publish.PersonMentionPropagationListener(synchronous@EventListener @Transactional) finds blocks via the newfindByMentionedPersons_PersonIdderived query, rewritesblock.text(replace("@" + old, "@" + new)), updates the matching sidecar entries, callssaveAllAndFlush, then emits an SLF4J info audit line. Synchronous on purpose — atomic with the rename; switch to@TransactionalEventListener(AFTER_COMMIT) + @Asyncif the archive ever passes tens of thousands of blocks.@Hans-Peter Müllervs@Hans Müller), multiple in-block occurrences, and orphaned-sidecar (event for a personId no longer in the persons table → no-op).Concurrency + integrity
ErrorCode.PERSON_RENAME_CONFLICT(409). Mirrored infrontend/src/lib/errors.tswith localisederror_person_rename_conflictkeys in de/en/es.PersonService.updatePersontranslatesOptimisticLockingFailureException(raised when the listener'ssaveAllAndFlushcollides with a concurrent autosave on a referenced block) toDomainException(PERSON_RENAME_CONFLICT, 409); the whole@Transactionalboundary still rolls back atomically.Security
@RequirePermission(Permission.READ_ALL)added toPersonController.getPersons()andgetPerson(). Defense in depth: the hover card and typeahead introduced by #362 surface life dates, notes, and family relationships, so the GETs now align with the access tier already enforced by POST/PUT/DELETE on the same controller and by the entire document/transcription stack.authorities = "READ_ALL"explicitly.Frontend types
frontend/src/lib/generated/api.tsregenerated against the dev backend. New fields:PersonMention,mentionedPersonsonTranscriptionBlock/Create/UpdateDTO. No new path entries; hover-card + typeahead in PR-B reuse existingGET /api/persons,GET /api/persons/{id},GET /api/persons/{id}/relationships.Spec deviations
One simplification vs the consolidation summary worth flagging:
@DataJpaTest + Testcontainers + second JdbcTemplateintegration test that bumpstranscription_blocks.versionmid-rename. End-to-end reproduction of the JPA optimistic-lock race inside@DataJpaTestis impractical without multi-threading or two physical connections — the listener load + save are sequential within one transaction, leaving no window for an external bump. The translation logic (raise →OptimisticLockingFailureException→ catch →DomainException(PERSON_RENAME_CONFLICT)) is exercised at unit level via a stubbed publisher inPersonServiceTest. The underlying JPA@Versionmechanism is well-covered by Hibernate's own test suite. If a stronger e2e check is desired later, a Testcontainers-backed@SpringBootTestwith explicit thread coordination is the right home for it.Test plan
./mvnw test— 1446/1446 backend tests green./mvnw clean package -DskipTests— clean buildnpm run generate:apiagainst local dev backend — regenerated types compile (PersonMention + mentionedPersons present)npm run checkblocked on this machine by a root-owned.svelte-kit/types/src/routes/stammbaum/$types.d.tsleft behind by the docker frontend container (unrelated to this PR — clean it viasudo rm -rf frontend/.svelte-kit && npm run check); CI runs the full type-check.What's next
PR-B (frontend, ~24 tasks) is queued behind this PR and will pick up:
personMention.ts(escapeHtml extraction,detectPersonMentionallowing spaces,renderTranscriptionBody)PersonMentionEditor.svelte(typeahead with life dates, WCAG 2.2 AA touch target).person-mentionCSS inroutes/layout.css+ 3 Paraglide keysPersonHoverCard.sveltewith three states (skeleton ≤ 200 ms, 404 unmounted, non-404 error in card) and manual flip viagetBoundingClientRect()TranscriptionReadView.svelteintegration with existingsplitByMarkers🤖 Generated with Claude Code
Block contains both @Hans-Peter Müller and @Hans Müller; the listener fires a rename for Hans Müller → Hans Schmidt. The simple replace("@" + old, "@" + new) hinges on the leading @-and-space anchor: "@Hans Müller" does not appear inside "@Hans-Peter Müller" (hyphen interrupts), so only the standalone mention rewrites. Sidecar mirrors the same — Hans Müller's entry flips to Hans Schmidt while Hans-Peter Müller's entry is preserved. Refs #362 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>openapi-typescript regenerated against the dev backend now exposes: - components.schemas.PersonMention with personId + displayName - TranscriptionBlock and CreateTranscriptionBlockDTO/UpdateTranscriptionBlockDTO carry the optional mentionedPersons array - (No new path entries: hover-card and typeahead reuse existing endpoints GET /api/persons, GET /api/persons/{id}, GET /api/persons/{id}/relationships.) Sealed inside PR-A so the frontend PR-B can import the new types from main without rebasing across an unrelated regen. Per Tobias' chain-tightening note in the consolidation summary. Refs #362 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>Markus Keller — Senior Application Architect
Verdict: Approved with concerns
I have read the diff end-to-end and matched it against the consolidation plan referenced in #362. The shape is right: domain-owned event published in
model/, listener placed in the consuming domain'sservice/package, repository derived query on the indexed sidecar column, schema enforcement at the database. The "first custom application event in the codebase" Javadoc onPersonDisplayNameChangedEventis the kind of memory-of-decisions artifact I want to see for new patterns — well done. That said, two things need to change before this becomes the load-bearing template for the next listener.Blockers
None. The structural choices are defensible.
Concerns
1. Cross-domain dependency direction is reversed in
PersonMentionPropagationListener.The listener correctly lives in the transcription-side service package, but it constructor-injects
PersonServiceto callexistsById. This is a transcription-domain component reaching into the person domain on the read path — the inverse of the rule we agreed on (cross-domain reads go through the owning service, which we are doing, so this is technically compliant) but the purpose of the call is suspicious. We are guarding against an event for a personId that no longer exists — meaning an event published byPersonService.updatePersonwhose row was then deleted before the listener fires. Given the listener is synchronous and joins the same transaction (@Transactionaldefaults to REQUIRED), the row cannot be deleted between publish and listen. TheexistsByIdcheck is dead code in the synchronous path.If you keep it as a defensive guard for the future
@TransactionalEventListener(AFTER_COMMIT) + @Asyncswitch (when the event genuinely outlives the rename transaction), then the comment in the listener should say so explicitly — otherwise a future reader will remove it as redundant. Right now the testnoOps_whenPersonIdNoLongerExists_orphanedSidecarGuardis the only documentation of intent.2.
@Componentvs@Serviceon the listener.PersonMentionPropagationListeneris annotated@Componentwhile every other class in theservice/package uses@Service. Functionally identical, but consistency matters when grep-ing. Make it@Service.Suggestions
3. ADR for the synchronous-event-inside-transaction pattern. This PR creates the convention. The Javadoc on the event record is a good start, but a one-page ADR in
docs/adr/would close the loop: context (rename atomicity), decision (synchronous listener, REQUIRED propagation), alternatives (AFTER_COMMIT + @Async — explicitly deferred), consequences (rename latency scales with mention-bearing block count; switch trigger ~10k blocks). Future cross-domain decoupling will reach for this pattern; the ADR keeps the trade-offs visible.4. Migration V56 — no FK on
person_id. The choice is sound and the migration comment explains it. One thing to add: a partial index or constraint that makes the orphan case observable. Right now an orphaned mention (person deleted, sidecar row remains) is silent. Apg_stat-friendly view or an admin query indocs/admin/for "list orphaned mentions" would let you find and clean these up before they accumulate.5. The
String.replace("@" + old, "@" + new)rewrite in the listener pushes a correctness concern down to the displayName format. As long asPerson.displayNamecontains a space (firstName + lastName), partial-substring corruption of@Hans-Peter Müllerwhen renaming a different@Hans Mülleris impossible — the partial-name-collision test proves it. But if a person has only a firstName or only a lastName (single-word displayName), the literal replace can corrupt other mentions. The test suite does not cover single-word displayNames. Sara should add it; the listener may need word-boundary anchoring for that case.What's done well
@ElementCollectionmirrors the existingUserGroup.permissions / group_permissionspattern — no new architectural style introduced.person_id— a future migration author will understand the intent.PERSON_RENAME_CONFLICTonOptimisticLockingFailureExceptiontranslation: clean structured-error pattern, mirrored in the frontend.@Valid List<PersonMention>with@Validon the field (recurses into elements) — JSR-303 mechanics are correct.I would merge this once #2 is fixed and #1 either deletes the existence check or comments why it is kept. The rest can land as follow-ups.
Felix Brandt — Senior Fullstack Developer
Verdict: Approved with concerns
Strong PR. The rename-propagation pipeline is small, has clear seams, and the test coverage looks like it was actually written before the implementation rather than around it. A few things I would want to fix before this lands as the template for future cross-domain listeners.
Blockers
None — but read concern #1 carefully, it is a behavioural correctness gap, not a style issue.
Concerns
1.
String.replacerewrite is correct only whendisplayNamecontains a space.PersonMentionPropagationListener.java:46-50:Test
doesNotMatchPartialName_when...proves that@Hans Müller→@Hans Schmidtdoes NOT corrupt@Hans-Peter Müllerbecause the full multi-word needle does not match. Good. But:Single-word displayNames break this.
DisplayNameFormatterproduces a single-word name when one of firstName/lastName is null — verify inPerson.getDisplayName(). The fix is word-boundary anchoring:Add a failing test first (
rewritesText_whenSingleWordDisplayName_doesNotCorruptOverlappingMention), then make it pass withreplaceAll+ word boundary. Without this, the listener has a silent corruption path.2.
personService.existsByIdis dead code in the synchronous path.PersonMentionPropagationListener.java:31:The listener is
@Transactional(REQUIRED), invoked synchronously fromPersonService.updatePersonwhich is also@Transactional. They share one transaction. The Person row that triggered the event cannot be deleted betweenpublishEventand the listener's first statement — same thread, same transaction. The check passes 100% of the time. The testnoOps_whenPersonIdNoLongerExistsconstructs the listener directly with a Mockito stub, which is why it can simulate the path — but in production it never fires.If you intend this as forward-protection for the eventual
@TransactionalEventListener(AFTER_COMMIT) + @Asyncswitch, the Javadoc must say so. Otherwise the next person will delete the check (and the test) as obviously redundant. My preference: delete the check now (and the orphan test), and add it back with proper context when you switch to async.3. Constructor-positional cascade in
TranscriptionServiceTesttest fixtures.You added
@Builderto both DTOs — good. But the existing TranscriptionServiceTest call sites still use positional constructor:Every new DTO field will require touching these test call sites again. Switch to
.builder()in the same PR — it's a 5-line change per call site and you have the builder support already. Worth doing now while the touched lines are still in the diff.Suggestions
4. The listener does both the text replace AND the sidecar displayName update in one loop. Two responsibilities, one loop body. Extract:
The listener body becomes a 4-line orchestrator and each helper is independently testable.
5.
Objects.equals(oldDisplayName, newDisplayName)inPersonService.updatePersonis correct — but thetry/catch(OptimisticLockingFailureException)block wrapspublishEventsynchronously. The exception originates insidesaveAllAndFlushdeep in the listener. That works because Spring's REQUIRED propagation re-uses the calling transaction and exceptions bubble up the call stack. Add a unit test that proves this catch is reached when the listener throws — your currentupdatePerson_throwsConflict_whenListenerSignalsOptimisticLockFailuremocks the publisher throwing, which is one level too high. The real bug surface is the listener's saveAndFlush. Mock that path instead of stubbing the publisher to throw.What's done well
PersonMentionPropagationListenerTestcovers happy path, partial-name collision, multiple occurrences, orphaned sidecar, latency floor, and unrelated block — all named as sentences.PersonService.updatePersonuses guard clauses and snapshotsoldDisplayNameat the top, before any setter — clean code.@Builder.Defaultwithnew ArrayList<>()somentionedPersonsis never null — defensive defaults done right.@Validon the field, not just the controller parameter — JSR-303 recurses correctly.Address #1 (correctness blocker if I am right about single-word displayNames), #2 (dead code or document why it stays), #3 (mechanical — switch to
.builder()), and #5 (test the actual exception path). The PR will be a clean approve after that.Nora "NullX" Steiner — Application Security Engineer
Verdict: Approved with concerns
Auth posture, input validation, and SQL injection surface are all clean. The interesting attack surface in this PR is the stored-content rewrite path that fires on every Person rename. I want to flag two findings — one is a contract assertion that the frontend (PR-B) must honour, the other is a privacy nuance worth documenting.
Blockers
None at the backend layer. The two concerns below become blockers if the frontend doesn't carry its half.
Concerns
1. Stored XSS contract —
displayNameflows intoblock.textviaString.replace. Sanitization is the consumer's responsibility.PersonService.updatePersonacceptsdto.getFirstName(),dto.getLastName(), etc. without HTML-encoding.Person.getDisplayName()returns whatever was stored. The propagation listener then does:If a WRITE_ALL user renames a person to
<img src=x onerror=alert(1)>, that string is now persisted into every transcription block that mentioned them. The@Size(max=200)cap ondisplayNamelimits payload length but does not constrain content.Trust model: only WRITE_ALL users can rename. Same trust tier can already edit
block.textdirectly. So this is not a privilege escalation — it is a stored-XSS vector that already exists forblock.textitself. The PR description states the frontend will introduceescapeHtmlandrenderTranscriptionBodyin PR-B. That is the right answer and it must hold.Action: When PR-B lands, verify that:
block.textis rendered viatextContent(or{@html}only after passing through the escapeHtml + structured tokenizer)PersonHoverCardandPersonMentionEditorHTML-escape thedisplayNamebefore injecting it into the DOMdisplayNameI am OK approving the backend half on the explicit promise that PR-B handles this. Worth a CWE-79 unit test in PR-B's
personMention.test.ts(renderTranscriptionBody escapes <script> in displayName).2. Privacy: deleted-person mentions persist as plain text. Migration V56 says this is by design — agreed, but worth a "right to be forgotten" comment.
The sidecar table has
ON DELETE CASCADEonblock_idbut no FK onperson_id— explicitly chosen so deleted persons degrade to plain@Auguste Raddatztext inside the transcription. From a data-integrity standpoint, fine. From a GDPR / "right to be forgotten" standpoint, person deletion does not erase the person's name from the archive — every transcription block they were mentioned in still carries their name, both inblock.textand in the orphaned sidecar row.For a private family archive (your stated context), this is acceptable and probably desired (historical record preservation). For any future external user this is a privacy gap. Two suggestions:
PersonService.deletePersonexplicitly noting that mention text is NOT scrubbed and pointing to the V56 migration rationale.@DeletedName→[entfernt]in matching block.text. Not blocking; tracking would be enough.Suggestions
3.
PersonMention.displayNamelength-cap test exists for create. Add the same for update.createBlock_returns400_whenMentionedPersonDisplayNameExceeds200Charsis the regression guard forCreateTranscriptionBlockDTO. The same@Validrecursion applies toUpdateTranscriptionBlockDTObut there is no parallel test. If someone removes@Validfrom the update endpoint in a refactor, only the create path catches it. Mirror the test forPUT /api/documents/{id}/transcription-blocks/{blockId}.4. CWE-79 regression test for the rename pipeline.
Even though sanitization is the frontend's job, a backend test that proves the listener does not apply any encoding/decoding to the displayName during rewrite is worth having — otherwise a future "helpful" sanitization in the listener would silently change semantics. One assertion: rename
Auguste→<b>Augusta</b>, verify the rewrittenblock.textcontains the literal<b>characters.What's done well
@RequirePermission(Permission.READ_ALL)toPersonController.getPersons()andgetPerson()— defense in depth; the explanation in the PR description (hover card surfaces life dates, notes, family relationships) is exactly the right reasoning. The corresponding 401/403 tests are present and use the right patterns (@WithMockUserwithout authorities for 403,@WithMockUser(authorities = "READ_ALL")for 200).findByMentionedPersons_PersonId(UUID)is method-name-derived; Spring Data generates parameterized SQL.DomainException.conflict(PERSON_RENAME_CONFLICT)with proper 409 — atomic rollback, structured error code, mirrored in frontenderrors.ts+ Paraglide. No silent failure mode.@Size(max=200)ondisplayName— bounds DoS payload size at the API boundary.person_idintentionally, with the rationale documented inline. That documentation is what stops a future reviewer from "fixing" it.{}placeholders, never string concatenation) — Log4Shell-safe.Approve once concern #1 is operationally tracked (PR-B integration verification) and #2 has at least the comment in
PersonService.deletePerson. Concerns #3 and #4 are nice-to-haves but I would push for them in the same PR.Sara Holt — Senior QA Engineer
Verdict: Approved with concerns
Test pyramid is well-balanced — Mockito unit tests for the publisher logic in
PersonService,@DataJpaTestintegration tests with Testcontainers for the listener and the repository, and a latency floor at 200 blocks acting as a merge-blocking regression guard. Test names read as sentences, factories are extracted, one assertion per test in most places. There are real gaps, however, and one of them is the test that would have caught the strongest correctness risk in this PR.Blockers
None.
Concerns
1. Missing edge-case test: single-word
displayName.Every test in
PersonMentionPropagationListenerTestuses a person with bothfirstNameANDlastName(Auguste Raddatz,Hans Müller,Hans-Peter Müller).Person.getDisplayName()produces a multi-word string with a space inside, which meansString.replace("@Auguste Raddatz", "@Augusta Raddatz")cannot match@Auguste-Raddatz-Schmidtor any other extended mention.Add this test (it should fail today):
This is the test that proves the corruption window I am worried about. If
Person.getDisplayName()allows a single-word return value (it does, whenfirstNameis null), this currently corrupts other mentions. Make it red, then green.2. Test setup duplication between
TranscriptionBlockMentionsRepositoryTestandPersonMentionPropagationListenerTest.Both classes have near-identical
@BeforeEachbuilding aDocument + DocumentAnnotationfixture, and both define a privatesaveBlockhelper. Extract to a sharedTranscriptionTestFixturesutility class. Three tests currently, but PR-B will add more — the duplication compounds.3. The optimistic-lock test mocks the publisher, not the listener's
saveAllAndFlush.PersonServiceTest.updatePerson_throwsConflict_whenListenerSignalsOptimisticLockFailuredoes:This proves the catch in
PersonService.updatePersonworks when the publisher throws. But in production the exception originates from the listener'sblockRepository.saveAllAndFlush(blocks)call, which is two stack frames deeper. The current test would still pass if someone replacedsaveAllAndFlushwith a no-op — the mocked publisher throws independently of the listener body.The PR's spec-deviation note already acknowledges that an end-to-end optimistic-lock reproduction in
@DataJpaTestis impractical. I accept that. But the unit test should at least exercise the listener throwing path:blockRepository.saveAllAndFlushto throwOptimisticLockingFailureExceptionPersonMentionPropagationListenerTest)That's the missing half of the test pair.
4. Latency floor test: 2 seconds for 200 blocks is generous.
propagatesAcross200Blocks_inUnderTwoSeconds_latencyFloorallows up to 10ms per block including JPA dirty-checking, flush, and audit logging. Testcontainers cold-start aside, on a healthy CI runner this should complete in 200–400ms. A 2-second floor catches catastrophic regressions (someone adds an N+1 query inside the loop) but not incremental ones (someone adds a 5ms-per-block check). Suggest tightening to 1 second after observing 5–10 CI runs to get a stable baseline. Keep it asas("...merge-blocking regression floor")— that messaging is exactly right.Suggestions
5. Constructor-positional cascade in
TranscriptionServiceTest.The DTO change ripples through ~7 existing tests that now end with
..., java.util.List.of()). Felix is right to flag the builder migration — but from a QA standpoint, stop using positional constructors in new test fixtures. A fixture factory using.builder()would have absorbed this change in one place.6. Verify the propagation listener integration in CI explicitly.
The listener is
@EventListenerso it only fires when Spring's event publisher is wired. The current tests construct it manually (new PersonMentionPropagationListener(...)). A single@SpringBootTestsmoke test that does a realpersonService.updatePersonand asserts the block text changed would prove the wiring end-to-end. Five lines, runs once, catches "someone removed@EventListener" forever.7. Test the negative
@Validrecursion for the UPDATE endpoint.Backend test
createBlock_returns400_whenMentionedPersonDisplayNameExceeds200Charsexists. There is no parallelupdateBlock_returns400_whenMentionedPersonDisplayNameExceeds200Chars. Mirror it. Symmetric API surface deserves symmetric test coverage.What's done well
PersonService), integration (@DataJpaTestwith real Postgres for repository + listener), no E2E for this slice (correct — E2E is for user journeys, not data-layer rename pipelines).@DataJpaTest + Import(PostgresContainerConfig.class, FlywayConfig.class)is the canonical setup. No H2 anywhere in this PR. Migrations run end-to-end against real Postgres.rewritesText_andSidecar_whenSingleBlockReferencesRenamedPerson,doesNotMatchPartialName_whenAnotherMentionShares_a_substring_with_renamed_person. CI failures will be self-describing.saveBlock,savedPersonId) extracted at the top of each test class — setup is one-line-per-thing..as("...")annotations on assertions that need context (the latency floor).Thread.sleep, no@Disabled, no flaky test smells.Address #1 (single-word displayName test) and #3 (test the listener's throw path), and this is a clean approve. The rest are improvements I'd love to see in the same PR but won't block on.
Tobias Wendt — DevOps & Platform Engineer
Verdict: Approved
This PR is light on infrastructure surface area — backend code, JPA, one Flyway migration, frontend type regen. There is nothing to deploy differently and no Compose / CI / Caddy changes. I checked the things that DO concern my role:
What I checked
1. Flyway migration V56 — is it deployment-safe?
V56__add_transcription_block_mentioned_persons.sqlfollows the established versioning convention (last shipped is V55 implicitly).CREATE TABLEand twoCREATE INDEXstatements. New table, noALTERon existing tables, no data backfill, no triggers — instant deployment, no migration window risk.CREATE INDEX(notCONCURRENTLY). For an empty new table this is irrelevant — locks an empty table for nanoseconds. Fine.ON DELETE CASCADEonblock_idis the right choice — when a transcription block disappears, its sidecar mentions go with it. No orphaned-by-block rows.@Size(max=200)and@Column(length=200)— no schema/code mismatch.2. Backup/restore impact. New table will be picked up by the existing
pg_dumpstrategy automatically. No special handling needed. Restore tests will validate the round-trip on the next monthly run.3. CI impact. Backend test count went from N to N+~10 with two
@DataJpaTestclasses. Testcontainers Postgres is already in the cache; cold-start is ~3s, warm <500ms. Net CI delta probably 5–15 seconds. Acceptable.4. Frontend
api.tsregeneration. The PR description correctly notes this required--spring.profiles.active=devfor the SpringDoc endpoint to be reachable. Good — production never exposes/v3/api-docs. Verified: regenerated file is committed; PR-B is blocked on this PR merging, which is the right ordering.5. Container image impact. No new dependencies, no Dockerfile changes. Backend JAR will be marginally larger (~few KB) from the new classes. No-op for image build time and image size.
Suggestions
1. Migration coverage in CI. Confirm that
MigrationIntegrationTest(or whatever runs Flyway top-to-bottom against a fresh Testcontainers Postgres) ran V56 and passed in your local./mvnw test. The two new@DataJpaTestclasses use@Import(FlywayConfig.class)which suggests Flyway runs — but it's worth one explicit check that V56 is present in the migration list applied during CI. This is the kind of thing that breaks silently when someone forgets to push the SQL file.2. Consider adding a
pg_stat_user_indexescheck in your monthly DB report. Once V56 is in production,idx_tbmp_person_idandidx_tbmp_block_idshould both show non-zeroidx_scancounts (assuming the rename feature is used). If after a month you see one of them at zero, you have an unused index burning write amplification — drop it. Standard hygiene; not specific to this PR.3. No new env vars, no new secrets, no new ports, no new services. Confirmed by grep through the diff. Production deployment is a regular
docker compose pull && docker compose up -d backendwith the migration applying on backend startup.What's done well
flyway_schema_history) but if it were re-run manually it would just fail loudly on the duplicate table.:latesttags introduced anywhere. The PR doesn't touch images, but a quick scan of the diff for image references confirms no Compose changes.person_idand why@ElementCollectionover JSONB. Future operators reading this migration during an incident will understand the schema choice without context-switching to the PR description.idx_tbmp_*) — matches what's already indb/migration/.Nothing to fix from my side. Ship it once the application-layer reviewers are happy.
Leonie Voss — UX Designer & Accessibility Advocate
Verdict: Approved
This PR is backend-only — data model, listener, security lockdown, regenerated TypeScript types, and three Paraglide translation keys. No new components, no styling, no layout changes. The user-facing surface from a UX standpoint is exactly one new error message in three locales. PR-B is where my real review will happen — typeahead, hover card, read-mode link rendering, focus management, touch targets, contrast, all of that is downstream.
What I checked
1. Error message wording for
error_person_rename_conflict(de / en / es).Tone is consistent with the existing error catalogue (formal Sie / "please" / "por favor"). All three are intelligible at the senior end of the audience. Good.
Concerns
1. The error text tells users "what went wrong," but not "what to do about it" beyond "try again."
Nielsen heuristic 9 ("Help users recognise, diagnose, and recover from errors"): the message is descriptive but not actionable. A 67-year-old researcher who just renamed a person and got a 409 will see "another edit modified a referenced transcription block" and have no idea:
Suggested improvement (not blocking):
This requires that PR-B's UI actually preserves the typed input on 409 — which the PR description for #362 says it will. Coordinate with Felix on whether to update the message now or after PR-B confirms input preservation. Not a blocker for this PR; the current text is technically accurate and not misleading.
Suggestions for PR-B (recording here so it doesn't get lost)
These are NOT this PR's concerns but I want them on the record because the data-model decisions made here constrain PR-B's UX surface:
role="tooltip"(ordialogif interactive) and is keyboard-reachable on focus, not just hover.@mentionchip in read mode must hit 44px touch on mobile. Withtext-base(16px) line-height, this requirespy-2minimum (8px top/bottom). Don't size the mention withtext-xsortext-[10px]— same fight we had on PersonRelationshipsCard..person-mentionstyling: brand-mint (#A6DAD8) on white is 2.8:1 — fails AA for normal text. If PR-B uses brand-mint as the mention text colour it must be on a darker background, or use brand-navy text with mint underline.@Size(max=200)). At 16px font-weight 400, 200 chars wraps to ~3 lines on a 320px viewport. The chip / hover card needs amax-w-[20ch] truncatestrategy. Worth flagging in #362 NFR-USABILITY so PR-B doesn't get caught with overflow.displayNameflows intoblock.textunescaped at the backend. PR-B must HTML-escape on render. Coordinate with Felix that theescapeHtmlutility lives inlib/personMention.tsand is unit-tested with<script>,<img onerror>, and unicode normalisation payloads.What's done well
error_person_rename_conflictstring lives next to its siblings in the JSON file (error_alias_not_found,error_invalid_person_type) — Paraglide grouping convention preserved.frontend/src/lib/errors.tsfollows the established switch-case pattern. No raw backend message ever reaches the user.errors.tsErrorCodetype union has the new code added — TypeScript compile catches any consumer that tries to map an unknown code.Backend-only PR; nothing to block on from my side. Save the substantive UX review for PR-B.
Elicit — Senior Requirements Engineer & Business Analyst
Verdict: Approved with concerns
This is a Brownfield review of PR-A against the consolidated plan in #362. I am not reviewing code — my lens is requirements traceability, NFR coverage, scope discipline, and backlog hygiene. The PR is unusually well-disciplined for backend-half delivery. I have a small list of items to surface so they do not vanish into PR-B's churn.
Traceability check
I read the PR body, traced it against the locked plan in
#362#issuecomment-5339("Open decisions: none"), and confirmed every acceptance criterion from Section F has either a commit, a test, or an explicit deferral. The 21-atomic-commit history is itself a traceability matrix. I particularly appreciate the Spec deviations subsection — calling out the A15b optimistic-lock test simplification rather than silently dropping it is the kind of intellectual honesty that makes a backlog trustworthy.Concerns
1. NFR-PERF threshold is implicit, not specified.
The PR introduces a 2-second latency floor for 200-block propagation as a "merge-blocking regression guard." This is excellent practice — but the actual NFR was never written down anywhere I can find. It should be:
Without the NFR pinned in #362 (or a follow-up issue), the 2-second number reads as arbitrary. Please add it as a comment on #362 — it gives Sara a target to test against and Tobias a threshold to alert on.
2. The async-switch trigger is "tens of thousands of blocks" — but how do you know when?
The listener Javadoc says "switch to
@TransactionalEventListener(AFTER_COMMIT) + @Asyncif the archive ever passes tens of thousands of blocks." There is no mechanism that surfaces this threshold. A user renaming a frequently-mentioned person at archive size 50,000 will experience a 5-10s rename latency before anyone notices. Suggested follow-up issue:Not blocking this PR — but file the issue while the context is fresh. Two months from now, neither you nor I will remember to write it.
3. Privacy / right-to-be-forgotten is unspecified.
Migration V56 explicitly leaves orphaned mentions in place when a Person is deleted. From a UX requirements standpoint, this is a deliberate product decision (historical record preservation) — but it conflicts with the family archive mental model where a "delete person" action should reasonably scrub their name. Right now:
block.text→ still says@Auguste RaddatzdisplayName: "Auguste Raddatz"This is an UNSPECIFIED REQUIREMENT, not a bug. Two valid resolutions:
Surface this as an open question on #362 so the user (you) explicitly decides. Right now Option A is shipped without anyone confirming it.
4. PR-B coupling clarity.
PR-B is described as ~24 tasks queued behind this PR. That's a lot of surface area for a single follow-up PR. From an INVEST standpoint, several of those (
PersonHoverCard.sveltewith three states,PersonMentionEditor.sveltetypeahead,personMention.tsutility) are independently shippable. Suggest splitting PR-B into PR-B1 (utility + read-mode rendering) and PR-B2 (editor + hover card) — each independently testable and reviewable.Not blocking this PR. Just flagging that the "PR-B will land 24 tasks" framing risks a 2,000-line review that nobody reads carefully.
Suggestions
5. Definition of Done for #362 needs a checkbox for PR-B integration verification.
The PR description says CI is pending and a local
npm run checkis blocked on a.svelte-kit/typesissue. Once #362 is closed by both PRs merging, the DoD should include:npm run checkgreen (full type check on PR-B's regenerated types)axe-playwrightclean onPersonHoverCard+PersonMentionEditorescapeHtmlunit-tested with<script>,<img onerror>, unicode payloadsAdd these as comments on #362 now, while the requirements are fresh.
6. Backlog hygiene: PR title and atomic commit count are exemplary.
feat(transcription): person @mention sidecar + rename propagation (PR-A backend, #362)— title under 70 chars, type prefix, scope, issue reference. 21 atomic commits all referenced in the description. This is the kind of PR another reviewer can audit by reading the commit graph alone. Worth codifying as a contributor guideline if it isn't already.What's done well
[x] 1446/1446 backend tests green,[ ] CI green,[ ] npm run check(with explanation of the local blocker). Reviewers know exactly what's verified and what's pending.I would file the four follow-up items (NFR-PERF threshold, async-switch metric, privacy default decision, PR-B split) as comments on #362 today, then approve this PR. The implementation is solid — the gaps are at the requirements layer above it.
Review-fix round 1 — 6/12 concerns addressed, 4 deferred to PR-B / future, 2 deferred by product call
7 new commits pushed. 1447/1447 backend tests green.
Addressed in this round
text.replacemangled@Hans Müllerwhen renaming single-nameHans(Felix #1, Markus #5, Sara #1)99aee777— TDD: failing test reproduces the corruption, fix with regex + negative lookahead(?![\\p{L}0-9\\-]| (?=\\p{Lu}))existsByIdorphan guard in synchronous-transactional listener (Felix #2, Markus #1)d924d905— Removed the call, thePersonServicedependency, the now-unusedPersonService.existsById, and the unreachable orphan-guard test48492330— RealPersonMentionPropagationListenerwired with mockedblockRepository.saveAllAndFlushthrowingObjectOptimisticLockingFailureException; publisher mock routes viadoAnswer@Component→@Service(Markus #6)acffcc85@Validlength-cap regression test missing on PUT endpoint (Sara #4)0def9e9b— Mirrored the POST 400-on-201-chars test for PUTTranscriptionServiceTestbrittle trailing-List.of()constructor pattern (Felix self-review echoed by Sara)2d48821f— Switched 8 call sites to.builder()79063730—docs/adr/006-synchronous-domain-events-in-transaction.mdDeferred to PR-B
displayNameflowing unescaped intoblock.text(Nora #1, Leonie)<script>/<img onerror>test fixtures. Recorded in issue #362 consolidation comment §G alongside the PR-B split.Deferred by product call
@SpringBootTestfor@EventListenerwiring (Felix, Sara)@DataJpaTest.Status
feat/person-mentions-issue-362-backend.🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
N+1 query in
PersonMentionPropagationListener(PersonMentionPropagationListener.java:52)mentionedPersonsis@ElementCollection(fetch = FetchType.LAZY). Inside the loop, eachblock.getMentionedPersons()call triggers a separateSELECT * FROM transcription_block_mentioned_persons WHERE block_id = ?. For 200 blocks that is 200 extra round-trips on top of the initialfindByMentionedPersons_PersonIdJOIN query — 201 queries total. The latency floor test still passes today (Testcontainers Postgres on localhost is fast), but this is a structural O(n) query issue that will grow with the archive.Options, cheapest first:
@org.hibernate.annotations.BatchSize(size = 50)onTranscriptionBlock.mentionedPersons— Hibernate batches the SELECTs into ceil(n/50) round-trips instead of n.JOIN FETCH:@ElementCollectiontoFetchType.EAGER(only safe if blocks are never loaded in high-volume list queries elsewhere — verify first).Suggestions
Misleading test name (
PersonServiceTest.java,updatePerson_doesNotPublishEvent_whenOnlyAliasChanges)The name implies
updatePersonprocesses alias changes, but looking at thePersonService.updatePersonimplementation,PersonUpdateDTO.aliashas no effect ongetDisplayName(). The test body confirms no event fires when the name fields stay identical — the alias change is a red herring. Renaming toupdatePerson_doesNotPublishEvent_whenDisplayNameFieldsUnchangeddescribes the actual invariant without the false implication.LGTM
@WebMvcTestslices, listener tests are@DataJpaTest+ Testcontainers, unit tests use@ExtendWith(MockitoExtension.class).Pattern.quote(oldNeedle)+Matcher.quoteReplacement(newNeedle)prevents regex injection on user-controlled display names. ✅@Builder+@Builder.Defaulton DTOs replaces the@AllArgsConstructorcalls inTranscriptionServiceTestcleanly. ✅if (blocks.isEmpty()) return) is clean. ✅Objects.equals(oldDisplayName, newDisplayName)handles thenull→ non-null case (person got their first title). ✅🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
Blockers
N+1 lazy load in the propagation listener
findByMentionedPersons_PersonIddoes a JOIN againsttranscription_block_mentioned_personsto find the blocks — good. ButmentionedPersonsisLAZY, so eachblock.getMentionedPersons()in the iteration loop fires a separate SELECT. At 200 blocks that's 201 DB round-trips. The 2-second floor passes today because Testcontainers is local, but this degrades linearly as the archive grows.The cleanest fix here is a
JOIN FETCHquery on the repository:This collapses 201 queries to 1 and removes the dependency on Hibernate batch-fetching strategies.
Suggestions
ADR-006 is well-reasoned — I'd have written it the same way. The alternatives table is exactly the right level of detail. One addition worth making to the "Harder" section: note that
saveAllAndFlushis the only safe flush call here —saveAllalone defers exceptions to transaction commit time, after the publisher'stryblock has exited. This is documented in the ADR text but isn't in the code's Javadoc where a future maintainer might look.Event record placement in
model/— slightly non-standard; events often live in anevent/package or alongside the publishing service. But given the codebase uses flat layer packages rather than feature packages,model/is the closest natural home for a value type that neither a controller nor a service owns. The class-doc sets the convention clearly. Acceptable.@Transactionalon the listener is join-semantics, not new-transaction — because the listener is called synchronously on the publishing thread and uses defaultREQUIREDpropagation, it participates inupdatePerson's transaction, not a new one. This is correct and intentional (atomicity). Worth confirming: if Spring ever changes the default event multicaster to async, this silent behavior change would break atomicity. The ADR acknowledges this migration path; a one-line comment on the listener (// Joins publisher's transaction — async switch requires @TransactionalEventListener(AFTER_COMMIT)) would protect future maintainers.LGTM
PersonServicehas zero compile-time dependency onTranscriptionBlock,TranscriptionService, or anything in the transcription domain. ✅person_idis the right call for an archive product — graceful degradation (mentions become unlinked text) beats cascade delete for historical data. ✅idx_tbmp_person_id,idx_tbmp_block_id). Theblock_idindex is technically redundant with the FK's btree, but it's explicit and costs nothing at this scale. ✅@ElementCollectionpattern mirrorsUserGroup.permissions / group_permissions— consistent with the existing codebase convention. ✅🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
Blockers
Missing 401 tests for the two newly-protected GET endpoints (
PersonControllerTest.java)The PR adds 403 tests (authenticated user without
READ_ALL) but no 401 tests (unauthenticated request). These are distinct failure modes:The
@RequirePermissionAOP check runs after Spring Security's authentication check, so both layers need explicit tests. The existing pattern in this codebase (seeTranscriptionBlockControllerTest) tests both 401 and 403 for write endpoints — the GET endpoints should follow the same discipline now that they carry a permission guard.Suggestions
The regex is injection-safe (
PersonMentionPropagationListener.java:46-52):Pattern.quote(oldNeedle)escapes all regex metacharacters from the user-controlled display name. ✅Matcher.quoteReplacement(newNeedle)escapes$and\in the replacement string. ✅Without these, a user named
$1or\Qcould corrupt the replacement. Good defensive practice.Parameterized SLF4J logging (
PersonMentionPropagationListener.java:69):Correct —
{}placeholders are not evaluated as JNDI lookups. ✅No FK on
person_id: intentional and documented in the migration SQL comment. From a security perspective this is fine — there's no path through which an attacker could exploit a dangling sidecar entry. ✅@Validcascades into list elements (CreateTranscriptionBlockDTO.java:32,UpdateTranscriptionBlockDTO.java):@Validon thementionedPersonsfield combined with@Valid @RequestBodyin the controller correctly triggers recursive validation into eachPersonMention.@NotNullonpersonIdand@Size(max=200)ondisplayNameare enforced at the boundary before any business logic runs. ✅LGTM
READ_ALLpermission correctly added to bothgetPersonsandgetPerson. The PR description justifies the defense-in-depth reasoning: the hover card and typeahead in PR-B surface life dates, notes, and family relationships — access control alignment with the existing write endpoints is correct. ✅DomainException(PERSON_RENAME_CONFLICT, 409)is a structured error with an ErrorCode, not a rawRuntimeException. Frontend can render a user-friendly retry message. ✅de/en/es) do not leak implementation details ("transcription block modified concurrently" is user-comprehensible; no class names, SQL, or stack traces). ✅🧪 Sara Holt — QA Engineer & Test Automation Specialist
Verdict: ⚠️ Approved with concerns
Blockers
Missing 401 tests for the newly-protected GET endpoints
The PR upgraded
getPersonsandgetPersonfrom "any authenticated user" toREAD_ALL— correctly adding 403 tests — but left 401 tests (unauthenticated) unwritten. TheTranscriptionBlockControllerTestpattern in this repo tests both. These should be added before merge:Missing controller-level 409 test for
PERSON_RENAME_CONFLICTPersonServiceTest#updatePerson_throwsConflict_whenBlockSaveAllAndFlushHitsOptimisticLockverifies the exception at the service layer. But there's no@WebMvcTesttest onPersonControllerTestthat verifies the controller surfaces a 409 HTTP response whenupdatePersonthrowsDomainException(PERSON_RENAME_CONFLICT, 409). The global exception handler must be exercised:Suggestions
Latency floor test flakiness risk (
PersonMentionPropagationListenerTest.java:164)A
@DataJpaTestwith Testcontainers on a shared CI runner (cold JVM, container startup overhead, competing processes) can spike past 2 seconds even without a regression. I've seen this class of test fail on VPS-hosted CI at ~3 seconds with no code change. Recommend raising the threshold to 5000ms — it's still a meaningful guard against O(n²) regressions and eliminates false-positive failures. The PR description calls this "a floor, not a benchmark" — a wider floor is still a floor.Test name nit (
PersonMentionPropagationListenerTest.java)doesNotMatchPartialName_whenAnotherMentionShares_a_substring_with_renamed_personis long and uses underscores inconsistently. Suggest:doesNotRewrite_hansPeterMüller_whenRenamingHansMüller. The renamed person's scenario is clearer as a proper name.LGTM
@DataJpaTesttests use real Postgres 16 viaPostgresContainerConfig. No H2, no mocked repositories at the integration layer. ✅leavesUnrelatedBlockUntouchedtest verifies the empty early-return path. ✅PersonServiceTestoptimistic-lock test sets up the realPersonMentionPropagationListenerwith a mocked repository that throws — exercises the production call chain from publisher through listener through catch/translate. More setup than a pure-Mockito test but the comment explains why. ✅if (block.getText() != null)before regex replace. ✅ (An additional edge case: block with null text but a sidecar entry is handled gracefully — sidecar still updated, text unchanged.)@Buildermigration on DTOs:TranscriptionServiceTestnow uses builder chains instead of positional@AllArgsConstructorcalls — much more resilient to field reorder. ✅🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Nothing to block here. This is a clean backend feature with no infrastructure changes needed.
LGTM
Migration V56 (
V56__add_transcription_block_mentioned_persons.sql):person_id, why the divergence fromdocument_comments.comment_mentionsis intentional). Future-me running a production incident at 2am will thank whoever wrote that. ✅block_idREFERENCEStranscription_blocks(id) ON DELETE CASCADE— correct; mentions die with their block. ✅person_id— intentional. When a Person is deleted, sidecar entries become orphaned rows pointing at a non-existent UUID. No referential integrity violation, no cascade-deleting the transcription. Good call for an archive product. ✅idx_tbmp_person_idfor the propagation query,idx_tbmp_block_idfor the ElementCollection load. Both appropriate. ✅api.tsregeneration: The PR description confirms the backend was running with--spring.profiles.active=devwhennpm run generate:apiwas run. The regenerated types are in the commit (not a TODO for CI). ✅No new Docker services, no new environment variables, no new ports — this feature is entirely contained within the existing Spring Boot process and Postgres schema. Zero operational overhead added. ✅
The
npm run checkblocker noted in the PR description (root-owned.svelte-kit/typesleftover from the Docker frontend container) is a local dev environment artifact, not a CI issue. Doesn't affect the merge. Consider addingsudo rm -rf frontend/.svelte-kitto the dev container'spostCreateCommandso it doesn't recur.One observation: The latency floor test (
propagatesAcross200Blocks_inUnderTwoSeconds) creates 200 rows in the Testcontainers DB during CI. On the current Gitea runner (self-hosted NAS), this should complete well within 2 seconds given local network latency — but keep an eye on it if the runner ever moves to a lower-spec machine.🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ✅ Approved
This is correctly scoped as a backend-only PR. UI components (typeahead editor, hover card, read-mode link rendering) are explicitly deferred to PR-B per the delivery plan. No UI decisions to review here.
What I checked
Error messages (de/en/es.json):
"error_person_rename_conflict"— all three locales are user-facing, actionable, and human-readable:No implementation details (no class names, exception types, or SQL). The "please try again" instruction is clear and correct for an optimistic-lock conflict. ✅
PersonMentiontype inapi.tsis correctly shaped for PR-B:{ personId: string; displayName: string }. Both fields non-optional (driven by@Schema(requiredMode = REQUIRED)). The typeahead can work with this directly. ✅Notes for PR-B
When the
PersonMentionEditorandPersonHoverCardland, I'll be checking:@mentionlinks in read mode (visible, not justfocus-visible)aria-live="polite"on hover card skeleton/loading state so screen readers announce the card content.person-mentionlink color against the transcript background — brand-mint (#A6DAD8) on white is only 2.8:1 and FAILS AA for normal text; the link must use brand-navy or a darker shadeFlag these now so there are no surprises at PR-B review.
📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ⚠️ Approved with concerns
Blockers
Orphaned sidecar entries — no lifecycle tracking
The migration SQL explicitly documents: "No FK on
person_id: when a Person is deleted we want@Auguste Raddatzto remain visible as plain unlinked text." This is the correct product decision for an archive (historical fidelity > referential integrity). However, there is no requirement tracked for:@mentionsare currently orphaned (pointing to deleted persons)?This PR deliberately defers these, and that's acceptable — but these represent open requirements that should be tracked as a Gitea issue before the archive reaches a meaningful size. Suggest creating a follow-up issue: "Admin: detect and audit orphaned @mention sidecar entries".
Suggestions
Spec deviation A15b is well-justified — the explanation in the PR description is clear and honest: the optimistic-lock race cannot be reproduced within a single-transaction
@DataJpaTestwithout multi-threading, and the translation logic is covered at the unit level via a stubbed publisher. The@Versionmechanism itself is Hibernate-owned. This is a pragmatic, well-reasoned trade-off. ✅Completeness check on the
displayNamelength cap:@Size(max=200)inPersonMention,length = 200in@Column,VARCHAR(200)in the migration, and the controller-level 400 tests at exactly 201 characters. All four layers are consistent. ✅i18n completeness:
error_person_rename_conflictis present in de, en, and es.ErrorCode.PERSON_RENAME_CONFLICTis in the Java enum,errors.ts, and all three message files. Correct end-to-end mirroring. ✅The PR correctly scopes PR-A vs PR-B: PR-A is merge-blocking for PR-B (frontend needs the regenerated types). The dependency is explicit and the split is clean — no frontend components depend on the backend changes before they ship.
Open Questions to Track
@mentionis orphaned (person deleted)? Does the read-mode renderer show it as plain text, as a broken link, or as a greyed-out non-clickable mention? This needs a requirement in PR-B's issue.@mentionsin comment threads (not just transcription blocks)? If comments support@personmentions in the future, the same rename propagation pattern will need to extend there. Flag for requirements discovery on the comments feature.Review-fix round 2 — all open concerns addressed
6 new commits pushed. 1449/1449 backend tests green (was 1447 — +2 new tests).
Addressed in this round
findByMentionedPersons_PersonIdreturned blocks with LAZYmentionedPersons, triggering one SELECT per block (201 queries for 200 blocks). AddedfindByPersonIdWithMentionsFetchedusing a subquery +JOIN FETCH(2 queries total regardless of block count). Filtered JOIN FETCH was rejected — it loads only the matching mention per block, risking data loss onsaveAllAndFlush. (Felix blocker, Markus blocker)1f3f879ftest +c7958681fix@Transactionalcomment on listener documenting join-transaction semantics and async-switch implication (Markus suggestion)c7958681updatePerson_returns409_whenRenameConflictinPersonControllerTest— verifies the full controller → exception-handler path returns HTTP 409 +$.code = PERSON_RENAME_CONFLICT. (Sara blocker #2)5f76d4a1updatePerson_doesNotPublishEvent_whenOnlyAliasChanges→updatePerson_doesNotPublishEvent_whenDisplayNameFieldsUnchanged(Felix suggestion)7a647b561dc812bdfindByMentionedPersons_PersonIdtofindByPersonIdWithMentionsFetched— the mock returned an empty list after the listener was switched to the new method, suppressing thesaveAllAndFlushcall and breaking the exception-propagation test8ca3f378Already resolved / not applicable
getPersons/getPerson(Nora blocker, Sara blocker #1)getPersons_returns401_whenUnauthenticatedandgetPerson_returns401_whenUnauthenticatedwere already present at lines 52-56 and 102-106 ofPersonControllerTest.javafrom the original security commit. No change needed.Status
./mvnw clean package -DskipTests✅feat/person-mentions-issue-362-backend🤖 Generated with Claude Code
🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
What I looked at
Full diff across all 24 changed files, plus ADR-006. Primary lens: module boundaries, event design, database layer, and the structural decision to add the first custom cross-domain event to this codebase.
Strengths
Event design is correct.
PersonDisplayNameChangedEventas arecordinmodel/keeps Person agnostic of who consumes its renames. The listener pattern established here (model/event record →service/listener in the consuming domain → sync@EventListener @Transactional) is reusable. When Comments and Notifications need to react to person renames, they subscribe to the same event without touchingPersonService.ADR-006 is exactly what an ADR should look like. Context is precise, the decision is stated, the four alternatives are concrete with rejection reasons, and the consequences section is honest about both sides. The note about
saveAllAndFlushbeing mandatory for synchronous listeners (vs.saveAllwhich defers the flush to commit time) is the kind of subtlety that would cost a future developer an hour of debugging. Keep this standard for all future ADRs.Migration V56 design is sound. No FK on
person_idis the right call: deleted persons leave@Auguste Raddatzas readable historical text rather than causing a cascade delete or an orphan constraint violation. The B-tree index onperson_idis forward-looking for the "blocks mentioning person X" query — the index cost today is negligible, and removing it later is harder than adding it now. The index onblock_idmirrors the existing@CollectionTableload pattern.Synchronous-in-transaction is the right default. For a family archive at this scale, "rename + propagation committed atomically" is far more important than "rename returns fast". The ADR correctly identifies the async migration path for future scale (
@TransactionalEventListener(AFTER_COMMIT) + @Async) without implementing it prematurely.Layer boundaries are respected.
PersonMentionPropagationListenerlives inservice/and injects onlyTranscriptionBlockRepositoryfrom the transcription domain — that's within the same domain, not a cross-domain repo access. Person domain publishes an event; Transcription domain consumes it. NoPersonRepositoryleaking into transcription, noTranscriptionBlockRepositoryleaking into person.Blocker
None.
Suggestion
Dead query:
findByMentionedPersons_PersonId. The repository defines two methods for the same conceptual query:The listener uses
findByPersonIdWithMentionsFetchedexclusively. The derived queryfindByMentionedPersons_PersonIdis not called anywhere in production code. If it was an intermediate step kept during development, delete it before merge — dead code in a repository interface is a maintenance trap (future developer assumes it's in use and won't remove it).If it's intentionally kept as a simpler variant for future callers who don't need the
JOIN FETCH, add a comment explaining when to use which. Otherwise, remove it.Observations
The test for the optimistic-lock conflict path (
updatePerson_throwsConflict_whenBlockSaveAllAndFlushHitsOptimisticLock) manually wires a real listener with a mocked repository. This is the right approach — it exercises the production call chain (PersonService → publishEvent → listener → saveAllAndFlush throws → catch). The PR description's note on why the@DataJpaTestmulti-thread approach wasn't used is accurate and the accepted tradeoff is sensible.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
TDD evidence
Tests precede implementation — 1446/1446 green, coverage spans: controller-slice (400 validation guards, 403 permission gates, 409 conflict), unit (event published on title/notes/alias changes), integration (round-trip, query filtering, latency floor). Red/green discipline is visible. ✅
Blockers
1. Dead derived query — remove it before merge.
TranscriptionBlockRepositorydefinesfindByMentionedPersons_PersonId(UUID personId)(the Spring Data derived query) alongsidefindByPersonIdWithMentionsFetched(the explicitJOIN FETCHJPQL query). The listener callsfindByPersonIdWithMentionsFetched. Grep across all production code turns up zero callers offindByMentionedPersons_PersonId. Dead repository methods are a maintenance trap — they look like "safe to use" alternatives but lack the JOIN FETCH, so a future caller would silently trigger N+1 loads onmentionedPersons(lazy collection, one SELECT per block).Options: delete it, or annotate it clearly with when to use it (and write a test for it). Don't leave it as-is.
Suggestions (non-blocking)
2. Test method name / assertion mismatch.
PersonMentionPropagationListenerTest:The method name says "TwoSeconds"; the assertion says 5000 ms; the
as()message says "5s". The recent commit message confirms the floor was raised from 2 s to 5 s to prevent CI flakiness. Rename the method topropagatesAcross200Blocks_inUnderFiveSeconds_latencyFloor()to eliminate the silent contradiction.3. Extract
rewriteBlockTextfor readability.onPersonDisplayNameChangedis 37 lines, which is at the edge. The Pattern + Matcher + replaceAll block inside the for-loop is a self-contained concern:Extracting this keeps the loop body at three lines —
rewriteBlockText(block, boundary, replacement), the sidecar update, and nothing else. The regex comment (boundary condition rationale) stays onrewriteBlockText. Non-blocking but makes the listener easier to read at a glance.4. Test coverage gap: null
personIdonupdateBlock.TranscriptionBlockControllerTesthas:createBlock_returns400_whenMentionedPersonDisplayNameExceeds200Chars✅createBlock_returns400_whenMentionedPersonPersonIdIsNull✅updateBlock_returns400_whenMentionedPersonDisplayNameExceeds200Chars✅updateBlock_returns400_whenMentionedPersonPersonIdIsNull— missingBoth DTOs carry
@Valid @NotNull personId, so the controller-slice behavior is symmetric. Add the missing test to keep coverage consistent. A future refactor that accidentally drops@ValidfromupdateBlockwould go undetected without it.What's done well
Pattern.quote(oldNeedle)andMatcher.quoteReplacement(newNeedle)— user-controlled display names can contain regex metacharacters and backreference syntax. Both are correctly quoted. This is easy to get wrong; getting it right without a comment is impressive.@Builderadded to both DTOs — theTranscriptionServiceTestbuilder conversions are a direct quality improvement (named params, no positionalnulltraps).PersonServicetry/catchwraps onlypublishEvent(), not the save itself. This is correct: the optimistic lock exception originates inside the listener'ssaveAllAndFlush, not frompersonRepository.save(). The scope is tight.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
What I audited
Authorization changes, input validation pipeline, the text-rewrite regex, JPQL query safety, and the new event/listener call chain. OWASP Top 10 lens applied throughout.
Findings
1. ✅
@RequirePermission(READ_ALL)on GET endpoints — correct and necessary.PersonController.getPersons()andgetPerson()were previously unauthenticated (the class-level@RequirePermissiononly covered writes; GET methods had no annotation). This is the right fix: the hover card and typeahead in PR-B surface life dates, notes, and family relationships — PII-equivalent for a private family archive. The GETs now align with the access tier already enforced on POST/PUT/DELETE.Both new 403 tests confirm the enforcement:
Correct. ✅
2. ✅
@Validcascade intoList<PersonMention>— correctly wired.The chain is:
@Valid @RequestBodyon the controller parameter →@ValidonmentionedPersonsfield in the DTO →@NotNull personId+@Size(max=200) displayNameonPersonMention. All three levels are present. Without@Validon the field, Bean Validation does not recurse into list elements; without@Validon the@RequestBody, no validation runs at all. Both are correctly placed. The controller tests for 400 +VALIDATION_ERRORconfirm the chain works end-to-end. ✅3. ✅ Regex injection and replacement injection — both safe.
This is the most subtle security point in the PR. User-controlled display names (e.g.
Hans (Sr.) Müller+) can contain regex metacharacters. The rewrite uses:Pattern.quote()wraps the needle in\Q...\E, making all metacharacters literal.Matcher.quoteReplacement()escapes$and\in the replacement string, preventing backreference injection (e.g. a new name containing$0would otherwise refer to the matched group). Both defenses are present. ✅4. ✅ JPQL — parameterized, injection-safe.
Named parameter
:personIdis a UUID — not user-controlled text. Safe by type and by parameterization. ✅5. ✅ Audit log line appropriate.
SLF4J parameterized logging (not string concatenation). Old and new display names are person names — appropriate audit data for a family archive, not passwords or PII requiring redaction. ✅
Observations (not blockers)
401 path not tested for the newly secured GETs. The existing tests confirm 403 (authenticated, wrong permissions) but there's no explicit test for 401 (unauthenticated) on
GET /api/personsandGET /api/persons/{id}. TheSecurityConfig.anyRequest().authenticated()catch-all covers this at the framework level, so it won't be a live gap. But Nora's principle is: test your configuration of the framework, not the framework itself. A pair ofmockMvc.perform(get(...))without any@WithMockUser→.andExpect(status().isUnauthorized())would close this. Low priority given SecurityConfig blanket coverage, but worth adding in a follow-up.Summary
No definite vulnerabilities. The three security-sensitive code paths (permission enforcement, input validation cascade, regex rewrite) are all correctly implemented and tested. This PR is cleaner than most security-adjacent changes I review.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
What I reviewed
Requirements traceability from issue #362 (locked consolidation plan) through to the implemented features, spec deviations, and the frontend/backend split. Evaluated completeness of acceptance criteria coverage and flagged open requirements items for PR-B.
Requirements Coverage
Issue #362 consolidation plan → implementation mapping
The PR description maps cleanly to the locked spec:
PersonMention @Embeddable,@ElementCollection, V56 migration ✅PersonDisplayNameChangedEvent,PersonMentionPropagationListener✅OptimisticLockingFailureException → DomainException(PERSON_RENAME_CONFLICT, 409)✅@RequirePermission(READ_ALL)on GET endpoints ✅api.tsregenerated withPersonMention,mentionedPersons✅error_person_rename_conflictin de/en/es ✅Spec deviation A15b — accepted.
The PR explicitly flags that the integration test for the optimistic-lock race (A15b in the plan) was replaced by a unit-level stub approach. The justification is technically accurate: reproducing a JPA optimistic-lock race inside
@DataJpaTestrequires multi-threading or two physical connections — neither is practical in a single-threaded test. The translation logic is exercised viaPersonServiceTest#updatePerson_throwsConflict_whenBlockSaveAllAndFlushHitsOptimisticLock. The deviation is transparent, justified, and the acceptance risk is low (Hibernate's optimistic-lock mechanism is well-tested upstream).Open Requirements Items (for tracking, not blockers)
1. "Blocks mentioning person X" query — untracked feature.
The migration creates
idx_tbmp_person_idexplicitly to support a futureGET /api/persons/{id}/mentioned-blocks(or equivalent) query from the person detail page. This is called out in the PR description as a future query but there's no Gitea issue tracking it. The index is the right preparation, but the requirement itself — "as a reader, I want to see all transcription blocks that mention person X" — should have a tracked issue so it doesn't get lost after PR-B ships.Recommendation: Create a follow-up issue titled "Show transcription blocks mentioning a person on their detail page" and link it to issue #362.
2. Conflict UX flow — deferred to PR-B but needs a clear acceptance criterion.
When the 409
PERSON_RENAME_CONFLICTfires, the frontend will receive the error code and show the localized message ("Eine andere Bearbeitung hat einen verknüpften Transkriptionsblock gleichzeitig geändert. Bitte erneut versuchen."). The user flow after this point — does the form stay populated? Does the error appear inline or as a toast? Is there a retry action? — is not specified for PR-B.Recommendation: Add a concrete acceptance criterion to PR-B or to issue #362: "When a 409 PERSON_RENAME_CONFLICT is returned, the person edit form remains open with the user's input intact and displays the localized error message inline." The current error path may already handle this via the standard form action pattern, but it should be explicitly verified.
3. Graceful degradation on person delete — confirmed working by design.
The no-FK decision on
person_idis correctly documented in the SQL migration. When a person is deleted,@Auguste Raddatzremains as readable text in the transcription block, and the sidecar entry persists as an unlinked record. This matches the archive fidelity requirement ("historical text stays clean"). ✅i18n Quality
All three messages are clear, accurate, and appropriately actionable:
The messages explain what happened and what to do, which satisfies Nielsen's heuristic #9 (help users recognize, diagnose, and recover from errors). ✅
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Test pyramid assessment
PersonServiceTest— event published/not published, 409 conflict path@WebMvcTest)PersonControllerTest— 403 gates, 409 conflict;TranscriptionBlockControllerTest— 400 validation guards@DataJpaTest+ Testcontainers)TranscriptionBlockMentionsRepositoryTest,PersonMentionPropagationListenerTestInfrastructure is correct:
@DataJpaTest+@AutoConfigureTestDatabase(replace = NONE)+PostgresContainerConfig+ Flyway on real Postgres 16. ✅em.clear()used correctly to force Hibernate re-reads aftersaveAndFlush. ✅Concern (non-blocking but should be fixed before merge)
Test method name conflicts with assertion.
PersonMentionPropagationListenerTest:Three sources of truth, three different values: method name says 2 s,
as()message says 5 s,isLessThan(5000L)says 5 s. The recent commitraise latency floor to 5s to prevent false CI failuresconfirms the assertion was intentionally raised, but the method name wasn't updated. When this test fails in CI at 4.8 s, the developer reads "inUnderTwoSeconds" and is confused.Fix: rename the method to
propagatesAcross200Blocks_inUnderFiveSeconds_latencyFloor.Coverage gap
updateBlock_returns400_whenMentionedPersonPersonIdIsNull— missing.TranscriptionBlockControllerTestcovers both validation cases forcreateBlock:createBlock_returns400_whenMentionedPersonDisplayNameExceeds200Chars✅createBlock_returns400_whenMentionedPersonPersonIdIsNull✅But for
updateBlockonly one of the two cases is covered:updateBlock_returns400_whenMentionedPersonDisplayNameExceeds200Chars✅updateBlock_returns400_whenMentionedPersonPersonIdIsNull❌ — missingBoth DTOs carry identical
@Valid @NotNull @Sizeconstraints. If someone later drops@ValidfromupdateBlock's@RequestBody, thedisplayNametest would catch it but nullpersonIdwould silently pass through. Symmetric coverage is cheap here and closes a real regression surface.What's done well
Factory helpers and
saveBlockmethod.PersonMentionPropagationListenerTestcorrectly centralizes block creation insaveBlock(String text, List<PersonMention> mentions)and person creation insavedPersonId(firstName, lastName). Each test body is focused on the behavior, not the setup. ✅Edge case discipline. The listener test suite covers: happy path, partial-name collision (Hans vs Hans-Peter Müller), multiple occurrences in one block, latency floor at 200 blocks, and no-op when no blocks reference the person. That's the full relevant boundary space. ✅
@DataJpaTestover@SpringBootTestfor integration tests. Correct layer — loads only the JPA slice, not the full application context. CI time impact is minimal. ✅updatePerson_throwsConflict_whenBlockSaveAllAndFlushHitsOptimisticLocktest setup. The manual listener wiring (realPersonMentionPropagationListenerwith mockedTranscriptionBlockRepositoryrouted through theeventPublishermock) is correctly documented with a comment explaining why. The test exercises the production call chain, not just the catch clause in isolation. ✅⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I reviewed
Migration, CI implications, infrastructure footprint, and the architectural choice to stay synchronous (i.e., no new broker or queue).
No new infrastructure — correct decision.
The synchronous event approach (
@EventListener @Transactional) keeps everything inside the existing JAR and PostgreSQL instance. No RabbitMQ, no Redis, no second database. For the current archive scale (family archive, single-node VPS, ~23 EUR/month), adding a message broker for rename propagation would have been a maintenance liability with no concrete benefit. The ADR correctly names the async migration path for when block counts grow, without implementing it prematurely. ✅Migration V56 — clean.
ON DELETE CASCADEis correct (sidecar entries die with their block).person_idfor the future "blocks mentioning person X" query,block_idfor the@ElementCollectionjoin load. Both justified, neither excessive.Latency floor test — good CI gate.
propagatesAcross200Blocks_inUnderFiveSeconds_latencyFloor(currently misnamed as TwoSeconds — flagged by Sara) gives CI a merge-blocking regression guard for the sync propagation path. 200 blocks at < 5 s is a floor, not a benchmark — it catches catastrophic degradation (N+1 queries returning, index dropped, Testcontainers OOM) without being brittle to normal variance. This is the right kind of performance test at the integration layer. ✅CI pipeline impact
1446 tests → no new infrastructure dependencies, no new containers beyond the existing
PostgresContainerConfig. The two new@DataJpaTestclasses spin up the same shared Testcontainers instance as existing integration tests. CI time increase is proportional to test count, not architecture.One thing to verify post-merge
The
api.tsregeneration step (npm run generate:api) was run locally but the PR notes it was blocked fromnpm run checkby a root-owned.svelte-kit/types/leftover from the Docker frontend container. The PR explicitly says CI runs the full type-check. Confirm CI stays green after merge — this is the one path that wasn't fully verified locally. Not a blocker; just flag it in the post-merge smoke check.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
This is a backend-only PR with no Svelte components, no CSS changes, and no new routes. My review is brief and focused on the parts that have UX implications.
Error messages — clear and actionable ✅
The three
error_person_rename_conflictstrings meet the recovery-action requirement:All three follow Nielsen's heuristic #9: they say what happened and what to do. They avoid technical jargon ("optimistic lock", "transaction"). ✅
Notes for PR-B review
The following UI components are queued for PR-B and will need full design review before merge:
PersonMentionEditor.svelte— typeahead with life dates. Touch target requirement: minimum 44px per item (48px preferred for the senior audience). Each suggestion row must show name + life years as redundant disambiguation, not color alone.PersonHoverCard.svelte— three states (skeleton ≤ 200 ms, 404 unmounted, non-404 error in card). The skeleton duration is tight for the 60+ audience on slow connections; the 200 ms threshold should be verified against real latency on the VPS..person-mentionCSS — the rendered link must carryfocus-visible:ring-2for keyboard navigation. The anchor must be reachable by Tab and Enter. No color-only distinction for the mention vs. surrounding text — pair color with underline or icon.Verdict
Backend infrastructure is solid. No UX regressions introduced. PR-B review is mandatory before the feature is user-visible.
Review-fix round 3 — all open concerns addressed
4 new commits pushed. 1450/1450 backend tests green (was 1449 — +1 new test).
Addressed in this round
findByMentionedPersons_PersonIdhad zero callers in production and tests; the listener usesfindByPersonIdWithMentionsFetchedexclusively. Deleted to prevent a future caller silently triggering N+1 loads.43f474fcpropagatesAcross200Blocks_inUnderTwoSeconds_latencyFloor→propagatesAcross200Blocks_inUnderFiveSeconds_latencyFloor. Method name now matches theisLessThan(5000L)assertion and the "5s" message. (Felix #2, Sara #1)eb51155bupdateBlock— AddedupdateBlock_returns400_whenMentionedPersonPersonIdIsNulltoTranscriptionBlockControllerTest.createBlockhad both validation guards;updateBlocknow has symmetric coverage. (Felix #4, Sara coverage gap)4c3aa159rewriteBlockTextextracted — Pattern+Matcher+replaceAll block moved to a private helper; loop body reads as three lines. Boundary-condition rationale comment moved to the helper where it belongs. (Felix #3)13e0801b👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Strong TDD evidence across all three test layers. Code is clean and modern throughout. A few things worth flagging.
Blockers
None.
Suggestions
rewriteBlockTextcomment explains the regex invariant — keep it (PersonMentionPropagationListener.java:65)The inline comment explains a non-obvious conservative trade-off (false negatives vs. irrecoverable corruption). This is exactly the kind of why comment that belongs. No change needed.
@TransactionalononPersonDisplayNameChanged— the annotation correctly usesREQUIREDpropagation (default), so it joins the publisher's existing transaction rather than creating a new one. The inline comment documents this. ✅ Clean.updatePerson_throwsConflict_whenBlockSaveAllAndFlushHitsOptimisticLocktest setup (PersonServiceTest.java:~295) — this test wires a realPersonMentionPropagationListenerwith a mocked repository and routes events throughdoAnswer. It's more setup than a typical unit test but it's intentional: it exercises the exact production call chain (PersonService → publishEvent → listener → saveAllAndFlush → catch). The explanation in the test body comment makes the intent clear. ✅Missing null-text edge case in
rewriteBlockText—if (block.getText() != null)is guarded but there's no test for a block withtext == nullreaching the listener. It's a one-liner edge case worth covering:PersonMentionPropagationListeneris inservice/but directly injectsTranscriptionBlockRepository— this is the transcription domain's own repository, and the listener is a transcription-domain consumer, so the layering rule is satisfied. Worth a one-line verification if the package ever splits by feature.Test data duplication —
TranscriptionBlockMentionsRepositoryTestandPersonMentionPropagationListenerTestboth have identical@BeforeEachsetUp()bodies (Document + DocumentAnnotation construction). A sharedAbstractTranscriptionBlockTestBaseor a static factory would eliminate the duplication. Low priority for a solo project, but noted for future housekeeping.🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
The event-based decoupling is architecturally sound. ADR-006 is thorough and clearly argues the tradeoffs. The synchronous-within-transaction design is the right default for an archive that prizes historical fidelity over throughput.
Blockers
None.
Suggestions
Missing composite uniqueness constraint on
(block_id, person_id)in V56 — the migration creates two single-column B-tree indexes, but no uniqueness constraint prevents duplicate sidecar rows for the same(block_id, person_id)combination.@ElementCollectionuses DELETE+INSERT on each update so duplicates can't arise from normal usage, but a concurrent write bypassing JPA (direct SQL, a future import job) could create them. A defensive approach:This is a suggestion, not a blocker — the current risk is very low given the application-level guard.
PersonDisplayNameChangedEventlives inmodel/— placing a domain event in the persistence model package is an unusual home. The class doc acknowledges this ("first custom application event in this codebase") and sets a convention. I'd accept it given the flat package structure, but when the codebase grows I'd move events to anevent/package or into the publishing domain's own package. Flag for next ADR review.ADR-006 is well-written — the alternatives table is exactly what I want to see: direct method call (rejected: inverts dependency), async event (rejected: half-applied state risk), DB trigger (rejected: leaks domain logic into SQL), Hibernate listener (rejected: couples to Hibernate internals). All the right alternatives are weighed. ✅
The
PERSON_RENAME_CONFLICT409 semantic is correct —OptimisticLockingFailureException→ 409 Conflict maps well to RFC 9110 semantics ("the request conflicts with the current state of the target resource"). Client retries with a fresh fetch. ✅idx_tbmp_block_idexplicit index is needed — unlike some databases, PostgreSQL does not auto-create an index for FK references. The explicit index for the@ElementCollectionload is correct and necessary. ✅Latency floor test at 5 s for 200 blocks — this is a regression guard, not a benchmark. The ADR's migration path ("switch to
@TransactionalEventListener(AFTER_COMMIT) + @Async") is the right escape hatch if volume grows. The 200-block floor is a reasonable starting point for a family archive.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
The security lockdown (
@RequirePermission(READ_ALL)on the GET endpoints) is the right call — life dates, notes, and family relationships are sensitive even in a read operation, and the defense-in-depth argument holds. The validation layer (JSR-303 +@Validon the request body) is correctly wired. But there is one stored XSS surface that must be closed in PR-B, and it needs to be explicitly tracked.Blockers
Stored XSS via unescaped
displayName— CWE-79 (cross-site scripting via stored data)PersonMention.displayNameaccepts anyStringup to 200 characters and stores it verbatim:PR description acknowledges that PR-B will call
escapeHtmlbefore rendering. However, the backend currently imposes no sanitization on this field. An authenticated user withWRITE_ALLcan store:If PR-B renders
@<displayName>via.innerHTMLor any unsanitized path, this is exploitable. The risk is bounded by theWRITE_ALLauth requirement, but it is a real stored XSS vector.Recommended fix (two layers):
displayNameat the controller boundary before it reaches the DTO:textContent, notinnerHTML:Blocking recommendation: Create a tracking issue now so PR-B explicitly closes this before merge. The stored payload lives in the DB from the moment PR-A ships.
Suggestions
Missing 401 test for new GET endpoints —
PersonControllerTestadds 403 tests forgetPersonsandgetPersonwith@WithMockUser(authenticated, no READ_ALL). But there are no 401 tests for unauthenticated requests. The globalanyRequest().authenticated()in SecurityConfig handles this, but explicit tests prove it:Not a blocker (the global filter catches it), but per defense-in-depth principles, security boundaries should be tested explicitly at each endpoint.
SLF4J parameterized logging in listener —
log.info("Propagated rename {} → {} across {} block(s) for person {}", ...)✅ No JNDI injection risk. However, the old and new display names are logged in plaintext. In a family archive with GDPR-adjacent data (full person names), consider whether application logs have appropriate retention limits and access controls. Minor note, not a code change.@Valid @RequestBodyordering — Spring processes both regardless of order, so no security concern here. ✅Parameterized JPQL query —
findByPersonIdWithMentionsFetcheduses:personIdnamed parameter. ✅ No injection risk.@NotNullonpersonId— validated at the controller boundary before reaching the service. ✅PERSON_RENAME_CONFLICTerror code — surfaced to the client as structured JSON with code + message. No internal stack trace or DB details exposed. ✅🧪 Sara Holt — Senior QA Engineer & Test Strategist
Verdict: ✅ Approved
This is one of the better-tested PRs I've seen in this codebase. All three layers are covered, test names are descriptive, factory helpers exist in the integration tests, and the boundary/edge cases are well-exercised. A few gaps worth tracking.
Blockers
None.
Suggestions
PR description says "< 2 s" but test asserts
isLessThan(5000L)—PersonMentionPropagationListenerTest.propagatesAcross200Blocks_inUnderFiveSeconds_latencyFloorasserts the 5-second bound. The PR body says "completes in < 2 s against the Testcontainers Postgres." The commit history (4c3aa159) shows this was revised from 2 s to 5 s. The assertion and the test name are consistent (both say 5 s), but the PR body still says "< 2 s." Update the PR description to match the actual test bound. Minor, but misleading to future readers.Missing edge case:
block.getText() == nullinrewriteBlockText— the guardif (block.getText() != null)exists and is correct, but no test exercises this path. A block fetched from the listener's query could theoretically have null text (e.g. OCR placeholder not yet written). One-liner test to add inPersonMentionPropagationListenerTest:Missing 401 test for new GET endpoints —
PersonControllerTestcorrectly adds 403 tests forgetPersonsandgetPerson, but no 401 for unauthenticated requests. Existing pattern in the codebase tests both. Not a blocker since the global security config catches unauthenticated requests, but it should be consistent with how other protected endpoints are tested.@BeforeEachsetup duplication across two test classes —TranscriptionBlockMentionsRepositoryTest.setUp()andPersonMentionPropagationListenerTest.setUp()are nearly identical (Document + DocumentAnnotation construction with the same field values). Not a correctness issue, but if setup logic changes (e.g. a newNOT NULLcolumn ondocuments) it needs updating in two places.What's working well
@DataJpaTest+@AutoConfigureTestDatabase(replace = NONE)+ Testcontainers — real Postgres, not H2rewritesTextAndSidecar_whenSingleBlockReferencesRenamedPersonsaveBlock(),savedPersonId()in the listener testem.clear()between save and reload — tests real persistence, not first-level cache@WebMvcTest) for validation (400) and conflict (409) pathsnever()assertions inPersonServiceTestprove the no-op path works — event not published when only notes/alias changes🎨 Leonie Voss — Senior UX Designer & Accessibility Strategist
Verdict: ✅ Approved
This is a backend-only PR. No Svelte components, no CSS, no page routes changed. The three frontend files touched are purely additive plumbing: regenerated types and new error message keys. Nothing to flag from a UI/UX or accessibility perspective.
What I checked
Error message copy — The new
error_person_rename_conflictmessage is available in de/en/es:The messages correctly avoid leaking technical internals ("optimistic locking," "transaction rollback") while giving the user enough information to act (try again). This follows the pattern established by other conflict messages in the codebase.
Note for PR-B
PR-B is where my review will have substance. The items below are future-review signals, not concerns with this PR:
PersonMentionEditor.svelte— the typeahead must meet 44px touch targets for the senior audience (60+). The spec mentions "WCAG 2.2 AA touch target."PersonHoverCard.svelte— hover interactions are inaccessible to keyboard-only and touch users if implemented as CSS:hoveralone. Must support focus-triggered and tap-triggered states..person-mentionCSS —@Nameinline links need a visible focus ring (focus-visible:ring-2 focus-visible:ring-brand-navy) and contrast ratio ≥ 4.5:1 against the transcription background.aria-live="polite"so screen readers announce the loaded content.🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes, no new services, no CI pipeline modifications. The Flyway migration is clean and follows the established V-number naming convention. The Testcontainers integration tests implicitly validate the migration on every CI run.
What I checked
V56 migration (
V56__add_transcription_block_mentioned_persons.sql):V56__prefix, snake_case descriptionON DELETE CASCADEonblock_idFK — correct; sidecar rows vanish when the block is deletedidx_tbmp_person_id— needed for the "blocks mentioning person X" queryidx_tbmp_block_id— needed for the@ElementCollectionload. PostgreSQL does not auto-create indexes for FK references, so this is necessary, not redundantperson_id— intentional design for graceful degradation (deleted persons leave visible text intact)Missing uniqueness constraint — there's no
UNIQUE (block_id, person_id)constraint.@ElementCollectionuses DELETE+INSERT per update so normal usage can't create duplicates, but a future raw-SQL import or a concurrent write bypassing JPA could. Low risk for a family archive, but worth a note for the next migration:Migration test coverage —
@DataJpaTest+@Import({PostgresContainerConfig.class, FlywayConfig.class})in bothTranscriptionBlockMentionsRepositoryTestandPersonMentionPropagationListenerTestruns all 56 Flyway migrations against a realpostgres:16-alpinecontainer. V56 is validated on every CI run. ✅No Docker Compose changes — the new table is in the existing PostgreSQL instance. No new services, volumes, or ports. Monthly cost unchanged. ✅
No CI pipeline changes — Testcontainers spins up the Postgres instance inline; no new CI services needed. The 200-block latency test adds at most a few seconds to the integration test suite. ✅
📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ⚠️ Approved with concerns
The PR is well-scoped and transparent about its boundaries. The "PR-A backend / PR-B frontend" split is clearly motivated (API type regeneration creates a hard dependency boundary). The locked plan reference and the "open decisions: none" closure are exactly the kind of requirements traceability I look for. Two issues worth recording before merge.
Blockers
NFR-PERF claim in PR body doesn't match test assertion
The PR body states: "Propagation across 200 mention-bearing blocks completes in < 2 s against the Testcontainers Postgres."
The actual test (
propagatesAcross200Blocks_inUnderFiveSeconds_latencyFloor) assertsisLessThan(5000L)— 5 seconds. The test name says "five seconds." The commit4c3aa159("rename latency floor test to reflect 5s assertion") confirms this was deliberately revised.This is a requirements inconsistency: the stated NFR (2 s) and the tested NFR (5 s) diverge. Update the PR description to say "< 5 s" to keep the requirements artifact truthful. Stale NFR claims in PR bodies become the basis for future performance budgets.
Concerns
"Orphaned-sidecar → no-op" description is misleading
PR body states: "orphaned-sidecar (event for a personId no longer in the persons table → no-op)"
The code does NOT implement a no-op for this scenario. Since there's no FK on
person_id, if a Person is deleted after being mentioned, the sidecar row still exists. When another person is later created with the same UUID (practically impossible but theoretically possible with manual import), or when a rename event fires for a UUID with existing sidecar entries, the listener would process the blocks — not skip them. TheleavesUnrelatedBlockUntouched_whenNoSidecarReferencesPersontest covers blocks with no sidecar entries, not the orphaned-sidecar case.Either:
personRepository.existsById(event.personId())and return early if the person no longer exists.This is a requirements-vs-implementation gap. Given the archive's data model (Persons are rarely deleted), the practical risk is very low, but the specification is wrong.
What's well-specified
Review-fix round 4 — all open concerns addressed
4 new commits pushed. 1451/1451 backend tests green (was 1449 — +2 new tests from the null-text test and the V57 migration path exercised by existing integration tests).
Addressed in this round
doesNotThrow_whenBlockTextIsNull— no test covered theif (block.getText() != null)guard inrewriteBlockText; block with null text and a sidecar entry now confirmed to propagate the sidecar update without throwing (Felix suggestion, Sara suggestion)3a6f9044UNIQUE (block_id, person_id)constraint ontranscription_block_mentioned_persons— prevents duplicate sidecar rows from raw-SQL writes or future import jobs bypassing JPA;@ElementCollection's DELETE+INSERT means normal usage is unaffected (Markus suggestion, Tobias note)091f6c75escapeHtmlrequired inrenderTranscriptionBody, unit tests with<script>+<img onerror>payloads, no{@html}without sanitization (Nora blocker)PR description corrections (stale facts on record)
Two factual errors in the PR body, corrected here for the record:
1. Latency floor: The body says "completes in < 2 s against the Testcontainers Postgres." The actual test assertion is
isLessThan(5000L)(5 seconds). The floor was raised from 2 s to 5 s in round 2 (commit1dc812bd) to prevent CI flakiness on the self-hosted NAS runner. The method name andas()message were already updated in round 3 (eb51155b). The correct claim is < 5 s.2. Orphaned-sidecar description: The body says "orphaned-sidecar (event for a personId no longer in the persons table → no-op)". The
existsByIdguard that produced this no-op was removed in round 1 (commitd924d905) as dead code — the synchronous listener runs in the same transaction as the rename and the person row cannot be deleted concurrently. TheleavesUnrelatedBlockUntouched_whenNoSidecarReferencesPersontest covers the early-return viablocks.isEmpty()(no sidecar entries for the renamed person), not a personId-existence check.Already resolved / not applicable
getPersons/getPerson(Nora round 4, Sara round 4)getPersons_returns401_whenUnauthenticatedat line 53 andgetPerson_returns401_whenUnauthenticatedat line 103 ofPersonControllerTest.java. No change needed.Status
./mvnw clean package -DskipTests✅uq_tbmp_block_person UNIQUE) validated by existing@DataJpaTest+ Testcontainers integration tests🤖 Generated with Claude Code