Reference in New Issue
Block a user
Delete Branch "feat/issue-175-176-metadata-drawer-transcription"
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
Implements two related features for the document detail page:
#175 — Expandable metadata header with labeled "Details" toggle
aria-expanded, Svelteslidetransition#176 — Annotation-backed collaborative transcription system (backend + frontend shell)
transcription_blockstable (with@Versionoptimistic locking),transcription_block_versionsfor edit history,block_idFK ondocument_comments/api/documents/{docId}/transcription-blockswith@RequirePermissionbeforeunloadflush, empty state CTANot yet implemented (future PRs)
Test plan
/persons/{id}, tags link to filtered searchGET /api/documents/{id}/transcription-blocksreturns empty listPOSTcreates a block with annotation,PUTupdates with version historyCloses #175, closes #176
🤖 Generated with Claude Code
TranscriptionBlock entity with @Version optimistic locking TranscriptionBlockVersion for edit history TranscriptionService facade: CRUD, reorder, version history TranscriptionBlockController: REST endpoints under /api/documents/{docId}/transcription-blocks DTOs: Create, Update, Reorder ErrorCode: TRANSCRIPTION_BLOCK_NOT_FOUND, TRANSCRIPTION_BLOCK_CONFLICT DocumentComment: add block_id field for block-level comment threads Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>- DocumentMetadataDrawer: 3-column grid (≥1024px), single-column mobile Shows document date, location, status, person cards, tag chips Person names link to /persons/{id}, tags link to filtered search Empty states for missing persons/tags, receiver cap with expand button - DocumentTopBar: "Details" toggle button with animated SVG chevron 44×44px tap target, aria-expanded, Svelte slide transition Semantic color tokens for dark mode compatibility - Remove DocumentBottomPanel from document detail page Bottom panel replaced by topbar drawer for metadata access Simplify +page.server.ts (remove comments loading) Update page.server.spec.ts for new load signature Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: 🚫 Changes requested
Blockers
No tests anywhere in this PR. This is a TDD violation — every new behavior must have a failing test before any production code is written. The PR introduces:
TranscriptionServicewith 7 public methods — no unit testsTranscriptionBlockControllerwith 7 endpoints — no integration tests+page.server.ts— the existing spec was updated to remove test cases, but no new cases were added for the new load behaviorAt minimum, I need to see:
TranscriptionServiceTest.java— create, update, delete, reorder, version history creation, text sanitization, max-length enforcementTranscriptionBlockControllerTest.java— endpoint routing, permission checks, 404 on missing blockDocumentMetadataDrawer.svelte.spec.ts— renders person links, tag chips, empty statesTranscriptionBlock.svelte.spec.ts— renders text, fires onTextChange, shows save statesTranscriptionService.deleteBlock()double-deletes the annotation (TranscriptionService.java:91-92). The block hasannotation_idwithON DELETE CASCADEon the annotation table, so deleting the annotation viaannotationService.deleteAnnotation()first would cascade-delete the block. But the code deletes the block first viablockRepository.delete(block), then tries to delete the annotation. The annotation delete will also fail becauseAnnotationService.deleteAnnotation()checks thatuserId.equals(annotation.getCreatedBy())— but the block creator and annotation creator might differ in theory. This needs a test to expose the issue, then a fix.TranscriptionService.deleteBlock()callsannotationService.deleteAnnotation()which requires the user to be the annotation author (AnnotationService.java:58). SinceTranscriptionServicecreates annotations on behalf of users, thecreatedByfield should match — but this is an assumption with no test. If a user with WRITE_ALL permission tries to delete another user's block, the annotation delete will throwDomainException.forbidden. The block is the aggregate root per the architecture discussion — block deletion should not be gated by annotation ownership.Mode exclusivity
$effectcreates an infinite loop risk (+page.svelte:65-68):This effect mutates
transcribeModewhich is a dependency. Svelte 5 guards against infinite loops, but this is still a code smell. The correct approach is to handle exclusivity at the toggle site — whenannotateModeis set to true, explicitly settranscribeMode = falseat the same call site (already done in the topbar snippets). This$effectis redundant and should be removed.DocumentBottomPanel.svelteis still in the codebase — only its spec was deleted, and it's no longer imported. But the file itself still exists. The issue says "remove entirely." Delete the file.Suggestions
handleCommentClickinTranscriptionEditView.svelteis an empty function — this is dead code. Either wire it up or remove the prop fromTranscriptionBlockand the button. An empty handler with a// Placeholder for futurecomment violates the "no dead code" rule.TranscriptionBlock.svelteusesconfirm()for delete — browserconfirm()is not styleable and breaks the De Gruyter brand. Consider a custom confirmation component in a follow-up, but for now document this as a known limitation.personAvatarColorinDocumentMetadataDrawer.svelte— verify this function exists and is exported. The import path$lib/utils/personFormatshould be checked (I confirmed it exists, but the function signature should be verified against usage — it takes anid: stringparameter).Command-query separation:
TranscriptionService.reorderBlocks()both mutates sort order AND returns the reordered list. Consider splitting intoreorderBlocks()(void) and letting the controller calllistBlocks()separately. Minor — not a blocker.🏗️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
Blockers
TranscriptionServicedirectly injectsAnnotationServiceANDDocumentService— this creates a cross-domain coupling chain:TranscriptionService → AnnotationService → AnnotationRepositoryandTranscriptionService → DocumentService → DocumentRepository. Per the architecture discussion,TranscriptionServiceis its own bounded context. InjectingDocumentServicejust to get the file hash for annotation creation is a leaky abstraction. Consider: pass thefileHashas a parameter from the controller (which already has access to it via the document), or add a dedicated method toDocumentServicethat only returns the hash.Delete cascade direction is backwards. The architecture discussion (comment #1620) established: "delete block → CASCADE annotation + CASCADE comments." But the migration has
annotation_idFK ontranscription_blockswithON DELETE CASCADE, meaning deleting the annotation cascades to the block. The block should be the aggregate root — the cascade should flow from block deletion to annotation, not the other way. This requires either:ON DELETE CASCADEfrom theannotation_idFK ontranscription_blocksand handling annotation cleanup in application code, orON DELETE CASCADEfromtranscription_blockstodocument_annotations(which requires the FK to be on the annotation side, not the block side — a schema redesign)For now, the pragmatic fix: remove
ON DELETE CASCADEfrom theannotation_idFK ontranscription_blocks(useON DELETE RESTRICT) and letTranscriptionService.deleteBlock()handle the cleanup in application code within the same transaction.Suggestions
Package placement: The new classes live in the existing layer-based packages (
controller/,service/,model/,dto/,repository/). Per our conventions, feature-based packaging is preferred. Considertranscription/as a package, but this is not a blocker for this PR — it can be addressed in a follow-up refactor.TranscriptionServiceis not@Slf4j— butTranscriptionBlockControlleris. The service should also have logging for operations like create, delete, and version history writes. This aids debugging in production.API path for reorder:
PUT /api/documents/{docId}/transcription-blocks/reorder— the/reordersuffix on a resource path is not strictly RESTful. ConsiderPATCH /api/documents/{docId}/transcription-blockswith a body indicating the operation. Minor — not a blocker for MVP.No
@TransactionalonreorderBlocks— wait, it is annotated. Good. ButgetBlockHistorycallsgetBlockwhich does a DB read inside a method that's not@Transactional— this is fine since it's read-only, but the two queries (find block, find versions) are not atomic. Acceptable for MVP.Frontend: direct
fetchto backend in+page.svelte— the transcription block loading, saving, and deleting all use rawfetch('/api/documents/...')from the client side. This bypasses the SvelteKit server-side API client pattern established in the project. For read operations, data should flow through+page.server.tsload functions. For mutations, form actions or at minimum server-side API routes are preferred. This works for MVP but should be refactored to use the typed API client once the OpenAPI spec is regenerated.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: 🚫 Changes requested
Blockers
Zero new tests in a 1,281-line addition PR. This is the single biggest quality gap I've seen in this project. The PR adds:
@DataJpaTestverifying mapping)@WebMvcTest)page.server.spec.ts(the comments-related tests) without replacing themRequired test coverage before merge:
Backend unit (Mockito):
TranscriptionServiceTest: create block saves version, update block saves version, delete block removes annotation, reorder updates sort order, sanitizeText strips HTML, sanitizeText enforces max length, getBlock throws NOT_FOUND for missing IDBackend integration (Testcontainers):
TranscriptionBlockRepositoryCRUD operationsFrontend unit (Vitest):
DocumentMetadataDrawer: renders date, location, status; renders person cards as links; renders tag chips; shows empty statesTranscriptionBlock: renders block number and text; fires onTextChange on input; shows save states (saving, saved, error); shows delete confirmationThe existing
page.server.spec.tswas gutted — 3 of 6 test cases were removed (the ones testing comments loading). The remaining tests still pass, but the deleted tests covered real behavior that existed before this PR. The comments loading was removed from the server load, which is correct, but the test removal should be in the same commit as the behavior removal, and the commit message should mention it.TranscriptionEditView.sveltedebounce logic is untested — the auto-save debounce (1.5s delay, flush on blur, flush on beforeunload) is critical user-facing behavior. A debounce bug means lost transcription work on irreplaceable family documents. This needs Vitest tests withvi.useFakeTimers().Suggestions
TranscriptionService.sanitizeText()uses regex for HTML stripping (text.replaceAll("<[^>]*>", "")) — this is fragile. Test with edge cases:<script>alert(1)</script>,<img onerror=alert(1)>, incomplete tags like<div, nested<<div>>. Regex-based HTML stripping is a known antipattern; consider a proper sanitizer library. At minimum, write tests that prove the regex handles the known attack vectors.Optimistic locking (
@Version) is declared but never tested — theversionfield exists on the entity, but there's no test that two concurrent updates produce a conflict. Write an integration test that loads a block, modifies it in two separate transactions, and verifies one gets anOptimisticLockException.The
beforeunloadhandler inTranscriptionEditViewfiresexecuteSavewhich is async — butbeforeunloadhandlers cannot reliably await async operations. The browser may close before the fetch completes. Consider usingnavigator.sendBeacon()for the unload save, or at minimum test this edge case.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: 🚫 Changes requested
Blockers
HTML sanitization via regex is insufficient (
TranscriptionService.java:109):This regex fails for:
<img src=x onerror=alert(1)(no closing>)<<script>alert(1)</script>><script>The spec and UX discussion explicitly decided on
<textarea>to ensure plain text — the backend should enforce this. Replace the regex with a proper approach: either use a whitelist-based sanitizer (OWASP Java HTML Sanitizer) that strips ALL HTML, or simply reject any input containing<characters with a validation error. Since we explicitly decided "no HTML is persisted," rejecting<is the simplest correct solution.No input validation on
CreateTranscriptionBlockDTO— the controller accepts annotation coordinates (x,y,width,height,pageNumber) without any validation. A malicious client could send:width: -1000, height: -1000x: 999999, y: 999999pageNumber: -1width: 0, height: 0Add
@Min,@Max, or@Positiveannotations on the DTO fields, or validate in the service layer.No rate limiting on the auto-save endpoint —
PUT /api/documents/{docId}/transcription-blocks/{blockId}is called by the frontend every 1.5 seconds during typing. A malicious client can bypass the frontend debounce and flood this endpoint. Each save also writes a version row, so this is a DB amplification vector. Add at minimum a note in the controller that rate limiting should be added, or implement a simple check (e.g., reject updates within 500ms of the last update for the same block).TranscriptionBlockController.resolveUserId()silently returnsnullon failure — if user resolution fails (line 93-96), the method returnsnull, and thisnullis passed toTranscriptionService.createBlock()asuserId. The service then storesnullincreated_by. A block withnullcreator is an orphaned record with no audit trail. The controller should throwDomainException.unauthorized()instead of silently proceeding.Suggestions
textcolumn isTEXT(unlimited) — the service enforcesMAX_TEXT_LENGTH = 10_000, but only via application code. Add aCHECKconstraint in the migration:CHECK (length(text) <= 10000). Defense in depth — the DB constraint catches anything the application misses.Version history exposes
changed_byUUID — theGET /{blockId}/historyendpoint returnsTranscriptionBlockVersionentities directly, includingchangedBy(a user UUID). This is an internal identifier. Consider returning only the display name (like comments do withauthorName) to avoid exposing user IDs. Not a blocker for MVP.The
@RequirePermissionon the controller usesPermission.READ_ALLfor GET endpoints — this means any user with read access can see transcription blocks and history. Verify this is intentional — the issue doesn't specify whether transcription content should be visible to read-only users during active editing.🎨 Leonie Voss — UI/UX Design Lead
Verdict: ⚠️ Approved with concerns
Blockers
None — the implementation follows the spec and discussion resolutions well.
Concerns
Details toggle button lacks visual weight — the toggle uses
text-xs font-semiboldwhich is very small in the topbar context. The spec mockup showed the toggle at a comparable size to the action buttons (Annotieren, Bearbeiten). Consider bumping totext-smto match the visual hierarchy. The 44×44px tap target is correct viamin-h-[44px], which is good.Turquoise transcribe button uses hardcoded color
#00C7B1instead of a semantic token. The annotate button usesborder-primary/bg-primary/text-primary-fg. For consistency and dark mode support, the transcribe button should use a semantic token (e.g.,border-turquoise/bg-turquoise/text-turquoise-fg). If the token doesn't exist yet, define it inlayout.css. Hardcoded hex values in Tailwind classes are a maintenance risk.Block card missing the turquoise numbered badge overlapping style — the spec (item 8 in the UX discussion) specifies the badge should be "24px turquoise circle, top-left, slightly overlapping." The current implementation uses
bg-[#002850](navy) and the badge is inside the card padding, not overlapping. The badge should:bg-[#00C7B1]) to match the annotation rectangles on the PDFThis is important for the visual link between the numbered badge on the PDF annotation and the block card in the right panel.
No "Gespeichert ✓" fade animation — the UX discussion (item 2) specified the saved indicator should "fade after 2s." The implementation uses a 2s timeout to set state back to
idle, which abruptly removes the text. Add a CSStransition: opacity 300ms easeand setopacity-0before removing, so the "Gespeichert ✓" fades out smoothly instead of disappearing.Empty state CTA uses a generic document icon — the UX discussion (item 10) specified an illustration in the empty state. The current implementation uses a basic SVG outline icon. Consider replacing with a more descriptive illustration or at minimum a larger, more prominent icon with the crosshair cursor mentioned in the spec to reinforce the "draw on the scan" interaction.
Suggestions
Person card hover in the metadata drawer — the UX discussion (item 8 from issue #175) specified
hover:bg-mutedon the entire card withcursor-pointer. The current implementation useshover:bg-mutedwhich is correct, but the card is an<a>tag without explicitcursor-pointer(links default to pointer, so this is fine). Good.Drawer border-bottom — the spec discussion specified
border-bottomusingborder-lineon the drawer when open. The implementation hasborder-b border-lineon the drawer container. Correct.Responsive breakpoint — the drawer uses
lg:grid-cols-3which is 1024px. This matches the resolved UX discussion (item 3: "Single column below 1024px, 3-column grid at ≥1024px"). Correct.🔧 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
What I checked
ON DELETEbehavior (CASCADE where appropriate, SET NULL for user references). Migration numbering is sequential and won't conflict. The migrations referenceapp_users(id)for FK constraints — verified this matches the actual table name. Wait — the migration referencesapp_users(id)but the original V10 migration referencesusers(id). Let me check... The V18 migration usesREFERENCES app_users(id)while V10 usesREFERENCES users(id). The table is actuallyuserswith a JPA mapping toAppUser(see@Table(name = "users")). This is a bug — the FK in V18/V19 should referenceusers(id), notapp_users(id). This will cause a migration failure on startup.Blockers
V18__add_transcription_blocks.sqlreferencesapp_users(id)but the users table is actually namedusers(seeV10__add_document_annotations.sqlwhich correctly usesREFERENCES users(id)). Same issue inV19__add_transcription_block_versions.sql. Fix: changeREFERENCES app_users(id)toREFERENCES users(id)in both V18 and V19 migrations.Suggestions
pom.xmlorpackage.json— the PR uses only existing framework capabilities.idx_tb_document_sorton(document_id, sort_order)andidx_tb_annotationon(annotation_id)are good for the expected query patterns. Theidx_tbv_blockon(block_id, changed_at DESC)is a covering index for the history query. Well done.transcription_block_versionstable grows with every save (every 1.5s of typing). For the expected user base (small family, 1-3 editors), this is negligible. But consider adding a note to the backlog about version compaction for long-term health.🔄 Review Feedback Addressed — Round 1
Pushed 2 commits addressing the blockers from all six personas:
6463a32— fix: address PR review feedback — security, architecture, dead codeTobias (DevOps) — blocker fixed:
REFERENCES app_users(id)→REFERENCES users(id)Markus (Architect) — blockers fixed:
annotation_idFK fromON DELETE CASCADEtoON DELETE RESTRICT(block is aggregate root)TranscriptionService.deleteBlock(): now deletes block first (cascades to versions + comments), then deletes annotation directly via repository — no ownership check neededNora (Security) — blockers fixed:
sanitizeText()now only enforces max length (textarea content is plain text by design)requireUserId(): now throwsDomainException.unauthorized()instead of silently returning null@Min/@Positivevalidation onCreateTranscriptionBlockDTOcoordinatesFelix (Developer) — blockers fixed:
DocumentBottomPanel.svelteentirely$effecthandleCommentClickfunction andonCommentClickprop51c799e— test(transcription): add TranscriptionServiceTest with 13 unit testsFelix + Sara (Developer + QA) — blocker fixed:
Remaining items (suggestions, not blockers — deferred to follow-up)
vi.useFakeTimers()👨💻 Felix Brandt — Senior Fullstack Developer (Round 2)
Verdict: ⚠️ Approved with concerns
Previous blockers — status
TranscriptionServiceTestcovering all public methods. Good coverage of the service layer.deleteBlockno longer callsannotationService.deleteAnnotation()— usesannotationRepository.deleteById()directly.onCommentClickprop.Remaining concerns (not blockers)
Frontend component tests still missing —
DocumentMetadataDrawer,TranscriptionBlock,TranscriptionEditViewhave no Vitest specs. The backend is now covered, but the frontend components are untested. I'll accept this for merge given the backend coverage, but these should be added in a fast follow-up.sanitizeTextis now package-private (no access modifier) — this is fine for testing, but note it's an intentional visibility relaxation. The method only enforces max length now, which is correct since textarea content is plain text.TranscriptionServiceinjectsAnnotationRepositorydirectly — this crosses the domain boundary (service → another domain's repository). Per the architecture discussion, the block owns the annotation, so this is arguably within the transcription bounded context. Acceptable for now.Suggestions
TranscriptionBlockDatatype inTranscriptionEditView.svelteinto$lib/types.tsso it can be shared with the page component (which duplicates the same type definition).🏗️ Markus Keller — Application Architect (Round 2)
Verdict: ✅ Approved
Previous blockers — status
✅ Cross-domain coupling → The service now injects
AnnotationRepositorydirectly for the delete path. Since the block is the aggregate root and the annotation is a dependent, this is within the transcription bounded context. TheAnnotationServiceis still used for creation (which handles overlap checking), which is the correct delegation.✅ Delete cascade direction → Fixed.
annotation_idFK now usesON DELETE RESTRICT. Block deletion handled in application code: delete block (cascades to versions + comments via DB), flush, then delete annotation.Assessment
The architecture is sound for MVP. The data flow is clean:
@Transactionalensures atomicityThe remaining suggestion about package-by-feature is a refactoring task, not a merge blocker. The direct
fetchcalls in the frontend page component are also acceptable for MVP — they should be migrated to the typed API client when the OpenAPI spec is regenerated.🧪 Sara Holt — QA Engineer & Test Strategist (Round 2)
Verdict: ⚠️ Approved with concerns
Previous blockers — status
✅ Zero tests → Fixed.
TranscriptionServiceTesthas 13 well-structured unit tests using Mockito + AssertJ. Coverage includes: CRUD operations, version history creation, sanitization (null, max length, plain text), error paths (not found). All pass.⚠️ Deleted test cases from page.server.spec.ts → The 3 comments-related tests were removed because the comments loading was removed from the server load. The test removal is correct but was committed separately from the behavior removal. Acceptable.
⚠️ Frontend component tests still missing → No Vitest specs for the 3 new Svelte components. This is a gap, but the backend — which is where data integrity matters most — is now covered.
Assessment
The backend test coverage is sufficient for merge. The test names read as sentences, assertions are specific, and error paths are covered. The
sanitizeTextmethod being package-private for testability is a pragmatic choice.Remaining suggestions (for follow-up, not blockers)
TranscriptionBlock(save state rendering, delete confirmation) andTranscriptionEditView(debounce behavior withvi.useFakeTimers())🔒 Nora "NullX" Steiner — Application Security Engineer (Round 2)
Verdict: ⚠️ Approved with concerns
Previous blockers — status
✅ HTML sanitization via regex → Fixed. The flawed regex was removed entirely.
sanitizeText()now only enforces max length (10,000 chars). Since the frontend uses<textarea>(which produces plain text by design), and the backend never renders content as HTML, this is the correct approach. The text is rendered in Svelte via{text}interpolation (not{@html}), so no XSS vector exists.✅ No input validation on DTO → Fixed.
CreateTranscriptionBlockDTOnow has@Min(0)onpageNumber,x,yand@Positiveonwidth,height.⚠️ Rate limiting on auto-save → Not addressed, but acknowledged in the PR summary as a follow-up item. The current user base (small family, 1-3 editors) makes this low risk. Acceptable for MVP.
✅
resolveUserId()silently returns null → Fixed. Renamed torequireUserId(), now throwsDomainException.unauthorized()on null auth or missing user. Clean.Remaining concerns (not blockers)
DB CHECK constraint on text length — the 10,000 char limit is only enforced in application code. Adding
CHECK (length(text) <= 10000)to V18 would be defense in depth. Low priority.Version history exposes
changedByUUID — consider a DTO wrapper for the history endpoint to avoid exposing internal user IDs. Low priority for a family-internal app.Assessment
The critical security issues from round 1 are resolved. The remaining items are defense-in-depth suggestions, not blockers. No new attack surface concerns.
🎨 Leonie Voss — UI/UX Design Lead (Round 2)
Verdict: ⚠️ Approved with concerns
Changes since round 1
The round 2 changes were backend/architecture fixes and dead code removal. The UI components (
DocumentMetadataDrawer,TranscriptionBlock,TranscriptionEditView,DocumentTopBar) are unchanged from round 1.Previous concerns — status
All UI concerns from round 1 remain as suggestions for a follow-up polish PR. None are blockers:
text-xs, could be bumped totext-sm. Polish item.#00C7B1— should use a semantic token. Maintenance risk, not a blocker.Assessment
The UI implementation is functional and follows the spec's layout rules correctly (3-column grid at ≥1024px, semantic color tokens in the drawer, proper person cards with links). The polish items above should be addressed in a follow-up before the feature goes to end users, but they're not merge blockers.
🔧 Tobias Wendt — DevOps & Platform Engineer (Round 2)
Verdict: ✅ Approved
Previous blocker — status
users(id)instead ofapp_users(id). Verified in the updated migration files.Additional observations
ON DELETE RESTRICTon theannotation_idFK — correct, since the application code handles cascade.TranscriptionServiceTest.java) adds ~7 seconds to the test suite. Within budget.Assessment
All infrastructure concerns from round 1 are resolved. Ship it.
🔄 Review Concerns Addressed — Round 3
Pushed 5 additional commits addressing all remaining reviewer concerns:
Commits
refactor(types): extract TranscriptionBlockData to shared typesfix(ui): semantic turquoise tokens, badge styling, saved fade animationfix(transcription): use navigator.sendBeacon for beforeunload savefix(migration): add CHECK constraint on text lengthrefactor(transcription): split reorderBlocks for CQSConcern resolution status
All actionable concerns resolved. Remaining deferrals are acknowledged non-blockers.
DocumentMetadataDrawer (10 tests): - Renders formatted date, dash for null date - Renders location, dash for null location - Renders translated status label - Person cards as links to /persons/{id} - Receiver links, empty state for no persons - Tag chips as links, empty state for no tags TranscriptionBlock (12 tests): - Renders block number, text, optional label - Save states: idle (nothing), saving (pulse), saved (checkmark), error (retry) - Active turquoise border, error red border - onTextChange fires on typing, onFocus fires on click Fixes @Felix/@Sara: "Frontend component tests still missing" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>RED/GREEN for CommentService: - getCommentsForBlock(blockId): returns root comments filtered by blockId - postBlockComment(documentId, blockId, content, mentions, author): creates comment with block_id set RED/GREEN for CommentController: - GET /api/documents/{docId}/transcription-blocks/{blockId}/comments - POST /api/documents/{docId}/transcription-blocks/{blockId}/comments - POST .../comments/{commentId}/replies (reuses existing replyToComment) 4 new tests: 2 service unit tests + 2 controller integration tests All 25 CommentServiceTest + 24 CommentControllerTest green Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer (Round 3)
Verdict: ⚠️ Approved with concerns
What improved since Round 2
Massive progress. The PR now has 30 commits addressing every blocker and most suggestions from rounds 1 and 2. Key wins:
Remaining concerns (not blockers)
No test for the inline comment edit flow —
CommentThread.sveltenow has click-to-edit, Enter-to-save, Escape-to-cancel, and trash-to-delete. None of these have frontend tests. The backend edit/delete endpoints are tested viaCommentControllerTest, but the UI interaction paths are untested. Suggestion: add aCommentThread.svelte.spec.tsin a follow-up.TranscriptionBlock.svelteis growing — it now handles: textarea editing, auto-resize, local state sync, quote capture on mouseup, comment open/close, comment count tracking, save state display, delete confirmation. That's 8+ responsibilities. The component is ~210 lines, approaching the 40-line template threshold from the style guide. Consider extractingBlockFooterorBlockCommentSectionin a follow-up.Raw
fetchcalls in+page.svelte— transcription block CRUD uses client-sidefetchdirectly. This works but bypasses the typed API client pattern. Acceptable for MVP given the OpenAPI spec hasn't been regenerated.Suggestions
extractQuoteregex in CommentThread (/^>\s*"(.+?)"\s*\n\n?([\s\S]*)$/) doesn't handle multi-line quotes. If a user selects text spanning multiple lines, the quote will contain newlines that break the regex. Consider using a non-greedy[\s\S]+?instead of.+?for the quote capture group.🏗️ Markus Keller — Application Architect (Round 3)
Verdict: ✅ Approved
The architectural decisions from rounds 1-2 are cleanly implemented:
canDraw+color+onDraw/api/documents/{docId}/transcription-blocks/{blockId}/commentsreorderBlocks()returns void, controller callslistBlocks()separatelyNo architectural concerns remaining. Ship it.
🧪 Sara Holt — QA Engineer & Test Strategist (Round 3)
Verdict: ⚠️ Approved with concerns
Test coverage — current state
Backend: solid
TranscriptionServiceTest— 13 unit tests covering all public methodsCommentServiceTest— 2 new tests for block-level comments (getCommentsForBlock, postBlockComment)CommentControllerTest— 2 new integration tests for block comment endpointsFrontend: partial
DocumentMetadataDrawer.svelte.spec.ts— 10 tests ✅TranscriptionBlock.svelte.spec.ts— 13 tests ✅AnnotationLayer.svelte.spec.ts— 4 tests ✅Remaining gaps (not blockers for merge)
CommentThread.svelte has no tests — the reworked flat comment thread with inline edit, delete, quote extraction, and Enter-to-send has zero frontend tests. This is the highest-risk untested component.
TranscriptionEditView.svelte has no tests — debounce logic, sendBeacon on unload, flush on blur. These are critical for data integrity on irreplaceable family documents.
Integration test gap — no test verifying Flyway migrations V18/V19/V20 run cleanly, or that FK cascades work end-to-end. The
@DataJpaTestlayer is missing for the new entities.Assessment
Backend test coverage is sufficient for merge. The frontend test gaps are real but the components are iterating fast — writing tests for a UI that's still being refined would produce tests that need constant rewriting. Suggest stabilizing the UI first, then adding
CommentThread.svelte.spec.tsandTranscriptionEditView.svelte.spec.ts.🔒 Nora "NullX" Steiner — Application Security Engineer (Round 3)
Verdict: ✅ Approved
All blockers from rounds 1-2 resolved. New changes reviewed:
What improved
sanitizeText()now only enforces max length. Correct approach since textarea produces plain text.CHECK (length(text) <= 10000)in V18 migration. Defense in depth ✓requireUserId()throws — no more silent null on auth failure ✓@Min/@Positiveon annotation coordinates ✓New code reviewed
GET/POST /api/documents/{docId}/transcription-blocks/{blockId}/commentswith@RequirePermission. Follows existing pattern. Clean.PATCH /api/documents/{docId}/comments/{commentId}endpoint which already checks author ownership inCommentService.editComment(). No new auth bypass risk.DELETEendpoint with author/admin check. Clean.navigator.sendBeaconfor unload saves — sends JSON blob to the PUT endpoint. The endpoint already has@RequirePermission(WRITE_ALL), so unauthenticated beacons will be rejected. Good.renderBodywith{@html}— still uses the mention renderer which escapes HTML before injecting mention links. The eslint-disable comment documents this. Acceptable.No security concerns. Ship it.
🎨 Leonie Voss — UI/UX Design Lead (Round 3)
Verdict: ✅ Approved
Significant UI improvements since round 2. All my previous concerns have been addressed:
Resolved concerns
text-smwith more padding. Good.border-turquoise/bg-turquoise/text-turquoise-fgdefined in layout.css for light+dark mode. No more hardcoded hex in components.fadingstate. Smooth.border-accent,bg-muted,text-ink-2) instead of hardcoded orange. Adapts correctly.text-xstotext-sm. Appropriate for 60+ users.New UI elements reviewed
cursor-text+ subtle hover bg signals editability. Enter saves, Escape cancels. Intuitive.h-4 w-4matches block delete. Good.No UI concerns remaining. The transcription experience is clean and focused.
🔧 Tobias Wendt — DevOps & Platform Engineer (Round 3)
Verdict: ✅ Approved
What changed since Round 2
navigator.sendBeaconusage for unload saves is a browser API, no infra impactCommentControllerrouting — no new Spring beansMigrations
V18/V19/V20 unchanged from round 2 fix (correct
users(id)FK,ON DELETE RESTRICTon annotation FK,CHECKconstraint on text length). All verified.Build impact
The PR removes more code than it adds in dead component cleanup. Net deletion of ~1971 lines means the frontend bundle likely got smaller. No CI concerns.
Ship it.