feat(admin): OCR admin pages — overview & model detail #265
Reference in New Issue
Block a user
Delete Branch "feat/issue-264-ocr-admin-pages"
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?
Closes #264
Summary
TrainingInfoResponsetodto/package and addssenderModelsto itGET /api/ocr/training-info/globalandGET /api/ocr/training-info/{personId}for per-model historyPOST /api/ocr/train-senderto manually trigger sender model training (RUNNING or QUEUED run returned, 202 Accepted)/admin/ocr(overview + stat cards + models table),/admin/ocr/global(global training history),/admin/ocr/[personId](sender model detail)Note: base branch is
feat/issue-253-sender-models(PR #263) sinceSenderModel/SenderModelServiceare not yet onmain.🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
Good structural work here. Promoting
TrainingInfoResponseout ofOcrTrainingServiceas an inner record and into thedto/package is the right call — inner records in service classes are a layering smell. Constructor injection is consistent throughout. Cross-domain access viapersonService.getById()inSenderModelServicerespects the boundary rule. SvelteKit SSR pattern is applied uniformly across all three new routes.Blocker
TriggerSenderTrainingDTO.personIdis missing@Schema(requiredMode = REQUIRED)Without this, the generated
api.tshasTriggerSenderTrainingDTO.personId?: string(optional). The frontend can legally omit it and the backend will receive null, leading to an NPE rather than a 400 validation error. Every field the backend always needs should be markedREQUIRED.Suggestions
OcrControlleris growing broad. It now owns batch OCR, streaming OCR, segmentation training, AND sender model training. The controller has 7+ injected services. Worth filing a follow-up to extractSenderTrainingControllerfor the sender-model endpoints — no action needed in this PR, just noting the accumulation.The split between
triggerManualSenderTrainingandrunSenderTrainingleaks an implementation detail to the controller. The controller needs to know "if RUNNING, call runSenderTraining":The Spring
@Asyncself-invocation constraint is the reason, and it's a real constraint — but the controller shouldn't need to understand it. A cleaner boundary would be a package-privatevoid triggerAsync(UUID personId)onSenderModelServicethat the controller always calls, with the method itself checking whether to dispatch. Leave this as a suggestion since changing it now would require test updates.TrainingHistoryResponseexposesrunsasList<OcrTrainingRun>(the JPA entity directly). This is consistent with existing patterns in this codebase (TrainingInfoResponsealso returnsOcrTrainingRun). Not introducing a new problem, but worth noting the entity-as-response-body pattern makes schema evolution harder. Suggestion for a follow-up ADR if the OCR domain grows.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: 🚫 Changes requested
TDD discipline is solid throughout — the test files clearly preceded the implementation, and the coverage is genuine, not decorative. Component decomposition is good:
OcrHealthBar,OcrStatCards,OcrModelsTableare each one nameable visual region. Svelte 5 runes used correctly:$props(),$derived(), keyed{#each}in the table.+page.server.tspattern is consistent with the rest of the admin routes.Requesting changes on three issues before merge.
Blockers
1. All new UI strings are hardcoded English — Paraglide is not used
The project has de/en/es support via Paraglide. Every user-visible string in these files should be a message key:
OcrHealthBar.svelte:OcrStatCards.svelte:OcrModelsTable.svelte:/admin/ocr/+page.svelte:/admin/ocr/global/+page.svelte:Each needs a message key in
de.json,en.json,es.jsonand a{m.key()}call. Theadmin_tab_ocrkey was correctly added for the nav label — same pattern needed for all the above.2.
personNamederivation in[personId]/+page.svelteis fragileThe route already has
params.personIdavailable. Look up by the correct key:If the API ever returns names for multiple persons,
Object.values(...)[0]would pick an arbitrary one.3. No empty state in
OcrModelsTableWhen
senderModelsis empty the tbody renders with zero rows — no user-visible feedback. Add a no-models row:Suggestion
Redundant navigation in
OcrModelsTable. Both the person name cell and the "Details" action cell link to the same/admin/ocr/{model.personId}. The "Actions" column adds no value when there's only one action and the row already links there. Either drop the Actions column (leave only the clickable person name), or expand Actions later to include a "Train" button for the newPOST /api/ocr/train-senderendpoint.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: 🚫 Changes requested
Good security foundations: all three new endpoints are protected with
@RequirePermission(Permission.ADMIN). UUID typing on path params prevents injection.personService.getById()as a 404-guard also prevents IDOR (can't trigger training for a non-existent person). Auth tests for both GET endpoints (401 + 403) are present.One blocker before merge.
Blocker
POST /api/ocr/train-senderhas no controller security tests (401/403)The GET endpoints both have explicit auth boundary tests:
The POST endpoint has zero controller-layer tests. Per the project standard, every protected endpoint needs an unauthorized (401) and forbidden (403) test. Without them, a future config change that accidentally permits this endpoint would go undetected. Add to
OcrControllerTest:Security Smell (not a blocker)
TriggerSenderTrainingDTO.personIdhas no@NotNullvalidation.A POST with
{}or{"personId": null}will deserialize toTriggerSenderTrainingDTO(null), which then reachessenderModelService.triggerManualSenderTraining(null)and throws an NPE — currently a 500 rather than a 400. Add@NotNull:And
@Validon the controller parameter:This is a defense-in-depth hardening —
ADMINalready limits the blast radius — but it's the correct pattern at system boundaries.Note on rate limiting
POST /api/ocr/train-sendertriggers an async training job. There's no cooldown check beyond the QUEUED/RUNNING guard inrunOrQueueSenderTraining. A user with ADMIN access could queue training for N persons in rapid succession. The application handles this gracefully (QUEUED state), but worth documenting as a known behavior.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: 🚫 Changes requested
Test pyramid is well-represented: unit tests at the service layer (Mockito, no Spring context), controller slices with
@WebMvcTest, and Vitest component tests withvitest-browser-svelte. The+page.server.tstests import and call the load function directly — exactly right. Factory patterns withbuilder()keep setup readable. Thepage.server.spec.tsfiles now use the non-null assertion(await load(...))!to avoid thevoid | {...}union error — correct fix.Two gaps before merge.
Blocker
POST /api/ocr/train-senderhas no controller testsThe other two new endpoints have auth tests AND happy-path tests. The POST endpoint has none at all in
OcrControllerTest. At minimum, the following are needed:triggerSenderTraining_returns401_whenUnauthenticatedtriggerSenderTraining_returns403_whenNotAdmintriggerSenderTraining_returns202_withRunningRun(mockssenderModelService.triggerManualSenderTraining→ RUNNING run, verifiessenderModelService.runSenderTrainingis called)triggerSenderTraining_returns202_withQueuedRun(mocks → QUEUED run, verifiesrunSenderTrainingis NOT called)triggerSenderTraining_returns404_whenPersonNotFoundThe service-layer coverage in
SenderModelServiceTestis solid, but controller-layer behavior (HTTP status codes, response body shape, conditionalrunSenderTrainingdispatch) is not covered by service tests alone.Suggestion
OcrStatCards.svelte.spec.tsdoesn't coveravailableSegBlocksThree of the four stat cards have tests (
availableBlocks: 42,totalOcrBlocks: 200,availableDocuments: 15).availableSegBlocks: 8has no assertion. Add:OcrModelsTableempty-state test is vacuousZero rows is the current behavior — there is no empty state message to assert on. This test passes but proves nothing useful to the user. Once an
{:else}empty row is added to the component (see Felix's comment), update this test to assert the empty message is visible.🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
Brand compliance is good:
brand-navy,brand-sandused correctly throughout.OcrStatCardsfollows the card pattern exactly (bg-white shadow-sm border border-brand-sand rounded-sm p-6). Section titles usetext-xs font-bold uppercase tracking-widest text-gray-400. Back links with hover-chevron follow the established pattern.text-3xl font-bold text-brand-navyfor the stat numbers — correct weight for at-a-glance scanning.Concerns below, none blocking by themselves, but the i18n gaps noted by Felix need to be fixed before this ships to German users.
Concerns
OcrHealthBarrelies on color as the only redundant cue — the dot is not accessibleThe colored dot (
bg-green-500/bg-red-500) is a<span>with no role or label:Screen readers announce this as nothing. Color-blind users already have the text ("Online"/"Offline"), but the dot provides no semantic value. Add
role="img"with anaria-label:Empty state in
OcrModelsTableAn empty tbody with visible headers but no rows gives no feedback. First-time users won't know if models are loading, unavailable, or simply not trained yet. The
{:else}block mentioned by Felix should include a meaningful message (e.g., "No sender models trained yet").Table links in
OcrModelsTablehave no minimum touch targetThe person name link and "Details" link are inline text in table cells — likely 20–24px tall. WCAG 2.2 requires 24×24px minimum (44×44px preferred). Since this is an admin interface primarily used on desktop, this is lower priority — but consider adding
py-2to the<a>elements to increase the tap target height.Hardcoded strings (noted by Felix as blockers): "Global History", "Sender Models", "Global history →", "OCR" page heading, all table headers, "Online"/"Offline", all stat card labels. These will all display in English to German/Spanish users. This needs to be fixed before shipping — confirmed as a functional issue, not just style.
i18n suggestion for stat card abbreviations: "Seg. blocks" will be confusing to non-technical family members. Consider something like "Seg.-Blöcke" (de) and a tooltip or
titleattribute explaining what segmentation blocks are. Not a blocker.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR — no new Docker Compose services, no new environment variables, no new Flyway migrations needed (the new endpoints query existing tables), no CI workflow modifications. The
@Asynctraining dispatch reuses the existingAsyncConfigthread pool. The PR is self-contained to application code.Checked and confirmed clean:
OcrTrainingRunRepositoryquery methods are JPQL/Spring Data — no DDL)api.tschange is expected regeneration output, not a config concernThe one minor note: the
POST /api/ocr/train-senderendpoint triggers async work that consumes thread pool capacity. Since OCR training is already gated by the single-active-run constraint (via the partial unique index from V42), there's no risk of runaway concurrency. The QUEUED state handles backpressure correctly.LGTM from the infrastructure side.
Review cycle 1 — concerns addressed
Backend — DTO validation (Nora/Sara)
b879d28— Added@NotNull+@Schema(requiredMode = REQUIRED)toTriggerSenderTrainingDTO.personIdand@Validto the controller method. A nullpersonIdnow returns HTTP 400 instead of NPE → 500. Covered by a new@WebMvcTesttest (triggerSenderTraining_returns400_whenPersonIdIsNull).Frontend — i18n, empty state, accessibility (Leonie/Felix)
a006171— Full pass over all 6 OCR Svelte files:de.json,en.json,es.jsonm.*()calls inOcrHealthBar,OcrStatCards,OcrModelsTable,+page.svelte,global/+page.svelte,[personId]/+page.svelteOcrHealthBarstatus dot: addedrole="img"+aria-label(i18n'd)OcrModelsTable: added{:else}empty-state row withcolspan="5"[personId]/+page.svelte: fixed fragilepersonNamederivation — now usesdata.history.personNames?.[params.personId] ?? 'Unknown'via$derived()OcrModelsTablespec: test refactored to locale-agnostic structural assertionOcrStatCardsspec: added missingavailableSegBlockstestNot addressed / deferred
None — all blockers and suggestions from cycle 1 were addressed.
🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
Blockers
1. Business logic in
OcrController.triggerSenderTraining— controller making a service-level decisionOcrController.javalines ~150-159:The decision "if the run is RUNNING, fire the async task" is a domain rule, not HTTP routing. Controllers delegate; they do not interrogate return values to decide follow-up calls.
SenderModelService.triggerManualSenderTraining()should internally callrunSenderTraining()when it enqueues a RUNNING run — the controller just calls one method and returns what it gets.Suggestions
1.
TrainingInfoResponseandTrainingHistoryResponsefields missing@Schema(requiredMode = REQUIRED)All record fields in both DTOs are generated as optional in TypeScript:
The Java record constructor guarantees these are never null, but the TypeScript types don't reflect that. The frontend has to use
?? 0,?? [],?? {}everywhere defensively. Mark required fields with@Schema(requiredMode = Schema.RequiredMode.REQUIRED)on each record component.2. Architecture of the promotion was correct
Moving
TrainingInfoResponsefrom an inner record ofOcrTrainingServiceto a dedicated file indto/is the right structural choice — inner types in services are invisible from outside and make cross-referencing awkward. Thedto/package placement is clean.3. Dependency direction is acceptable
OcrTrainingService→SenderModelServiceforgetAllSenderModels()— no circular dependency, direction flows toward the data source. Accepted.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
1. Business logic in controller —
OcrController.triggerSenderTrainingRule: controllers delegate, they don't decide. The
if (run.getStatus() == RUNNING)branch is a business rule that belongs inSenderModelService.triggerManualSenderTraining(). Proposed fix — the service method should return the run AND fire the async task internally when needed:2.
TriggerSenderTrainingDTO.personIdgenerated as optional in TypeScript despite@Schema(requiredMode = REQUIRED)The
@Schema(requiredMode = REQUIRED)annotation was added to the record constructor parameter. For Java records, the OpenAPI spec annotation may need to be on the field using@Schemadifferently, or the types were regenerated before the annotation was committed. Either way: TypeScript callers can pass{ personId: undefined }without a compile error, which the backend will reject with 400. The types should match the contract.Suggestions
1.
paramsin$props()for[personId]/+page.svelte— verify this is supportedIn SvelteKit 2, route
paramsare not standard props passed to+page.svelte— onlydataandformare. The personId should come from$page.params(viaimport { page } from '$app/state') or be included in the serverload()return. The tests pass but that doesn't confirm this works in the SSR context. Worth a quick smoke test at/admin/ocr/[some-uuid].2. Missing
@Schema(requiredMode = REQUIRED)onTrainingInfoResponse/TrainingHistoryResponsefieldsSee Markus's note. The frontend compensates with
?? []/?? {}but the types should be authoritative.3. Everything else is clean
Naming is clear, components are well-sized,
$derivedused correctly, keyed{#each ... (model.id)}✅,!result.response.okerror checks ✅, all$props()typed ✅.🔐 Nora Steiner — Application Security Engineer
Verdict: ✅ Approved
All three new endpoints are properly gated with
@RequirePermission(Permission.ADMIN)and permission boundary tests (401 unauthenticated, 403 READ_ALL, correct status for ADMIN) exist for each. The null-personId → NPE → 500 vulnerability is closed with@NotNull+@Valid— cycle 1 addressed this correctly.Checked and clean:
@CrossOrigin, no CORS changes{@html}) — no XSS riskOcrHealthBarstatus dot usesrole="img"+aria-label— no invisible content that could mislead users@RequirePermission(Permission.ADMIN)on all 3 new endpoints — enum-based, typo-proof ✅Minor observation (not a blocker):
TriggerSenderTrainingDTO.personIdin the generated TypeScript ispersonId?: string(optional) even though the backend enforces@NotNull. The backend contract is correct and will return 400 on null input; the TypeScript type just gives callers a false sense thatundefinedis acceptable. Addressed by Felix's blocker #2.What was fixed in cycle 1 is solid:
The test
triggerSenderTraining_returns400_whenPersonIdIsNullcorrectly proves the vulnerability was present (RED) and the fix works (GREEN). It will prevent regression permanently.🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
Blocker
OcrHealthBar.svelte.spec.tsuses locale-sensitive text matchers/online/imatches German "Online" and English "Online" but fails for Spanish "En línea"./offline/ifails for Spanish "Sin conexión". If the test runner locale ever changes toes, these tests silently mis-verify. TheOcrModelsTabletest was already fixed for this in cycle 1 — apply the same pattern here. Therole="img"+aria-labelattribute is the locale-agnostic hook:Or simply assert that the correct CSS class is applied (
bg-green-500vsbg-red-500) as a structural, locale-agnostic alternative.What's good
triggerManualSenderTraining_returnsRunningRun_whenIdle,_returnsQueuedRun_whenAnotherRunning,_throws404_whenPersonNotFound— happy path + both queue paths + error path ✅page.server.spec.ts,global/page.server.spec.ts,[personId]/page.server.spec.ts) with happy path and error case ✅OcrStatCards,OcrModelsTable,OcrHealthBar— all present ✅OcrModelsTableempty state: locale-agnostic structural assertion (td[colspan="5"]) from cycle 1 ✅getTrainingInfo_includesSenderModels_inResponseservice test — new field covered ✅🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
Blocker
Links in
OcrModelsTablemissing visible focus rings — WCAG 2.4.7Person name links and "Details" links:
No
focus-visible:ring-*classes. Keyboard users tab through the table and cannot see where they are. WCAG 2.4.7 requires a visible focus indicator on all interactive elements. Fix:The same applies to the "Global history →" link in
+page.svelteand the back-link in[personId]/+page.svelteandglobal/+page.svelte(those back-links usehover:text-brand-navy transition-colorsbut nofocus-visible:ring-*).Suggestions
1. Stat cards: numbers have no programmatic association to their labels
Screen readers may announce "42" without context. A
<dl>/<dt>/<dd>pattern oraria-labelledbywould associate label and value. Low severity for an admin page used by technical users, but worth noting.2. Gray text
text-gray-400on white — verify contrasttext-gray-400(#9CA3AF) on white (#FFFFFF) is 2.5:1 — below WCAG AA 4.5:1 for normal text. This is used for column headers and stat card labels. It matches the existing pattern elsewhere in the admin UI, so this is a project-wide finding, not new — but it's worth flagging.What's good
OcrHealthBar:role="img"+aria-labelon the status dot — correct, screen readers skip decorative elements and announce the label ✅OcrStatCards:grid-cols-2 lg:grid-cols-4— responsive, works at 320px ✅OcrModelsTable:{:else}empty state withcolspan="5"— users aren't left with a blank table ✅group-hover:-translate-x-1) — consistent with the rest of the admin UI ✅m.*()— ready for locale switching ✅🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR. Checked:
pom.xmlorpackage.json✅@Asynctraining trigger uses the existingAsyncConfigthread pool — no new infrastructure requirement ✅This is a pure feature addition within the existing stack. From an infra perspective it ships as-is.
Review Cycle 2 — All Concerns Addressed
Concerns resolved
@Markus (Architect) — Business logic in controller
OcrControllerintoSenderModelService.triggerManualSenderTraining@Lazy @Autowired SenderModelService selffield for Spring AOP proxy self-reference (standard pattern for@Asyncon self-calls)269894a—refactor(ocr): move async training dispatch out of controller into SenderModelService@Felix (Developer) — Verify
runSenderTrainingnot called from controller layertriggerSenderTraining_doesNotCallRunSenderTraining_fromControllerinOcrControllerTestrunSenderTrainingdirectly)269894a@Sara (Tester) — Locale-sensitive OcrHealthBar tests
/online/i//offline/itext matchers with CSS class assertions on[role="img"]element794000c—fix(admin): locale-agnostic OcrHealthBar tests, focus rings on all OCR links@Leonie (UI/UX) — Missing focus rings on OCR admin links
focus-visible:rounded-sm focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:outline-noneto:OcrModelsTable.svelte/admin/ocr/+page.svelteglobal/+page.svelte[personId]/+page.svelte794000cConcerns deferred
@Nora (Security) —
TriggerSenderTrainingDTO.personIdoptional in TypeScript types@Schema(requiredMode = REQUIRED)and@NotNullbut the generated type showspersonId?: string🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Clean work. The architectural concerns from cycle 1 are resolved. Let me walk through what I checked.
What I verified
Layer separation:
TrainingInfoResponseandTrainingHistoryResponsecorrectly promoted from inner record todto/package. Controller delegates cleanly to service, zero business logic in the web layer. ✅Cross-domain access:
SenderModelService→PersonService.getById()(notPersonRepository).OcrTrainingService→SenderModelService.getAllSenderModels()(notSenderModelRepository). All cross-domain access goes through service boundaries. ✅@Lazy @Autowired SenderModelService self: Standard pattern for Spring AOP self-invocation when@Asyncmust be honoured. The comment explains the why. ✅Dependency direction: No circular dependency.
SenderModelService→PersonService→PersonRepository.OcrTrainingService→SenderModelService,PersonService. ✅Integrity enforcement: The partial unique index constraint (V42 from base branch) protects single-active training run invariant at the database layer. ✅
Suggestions (non-blocking)
Unlimited
findByPersonIdIsNullOrderByCreatedAtDesc()andfindByPersonIdOrderByCreatedAtDesc(UUID): Neither query has a LIMIT. As the training runs table grows, global history will scan everything. For an admin-only page this is acceptable today — the table is low-volume. Worth addingfindTop50By...variants when/if history depth becomes noticeable. Not a blocker now.TrainingHistoryResponse.personNamesis alwaysMap.of()for global history: The response shape works but is slightly misleading — callers must know that global runs never have person names. A doc comment on the record explaining this invariant would help future readers, but I won't block on it.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: 🚫 Changes requested
Two blockers. Everything else is clean.
Blockers
1.
paramsis not a SvelteKit page prop —personNamealways shows 'Unknown'frontend/src/routes/admin/ocr/[personId]/+page.svelte, line 6:In SvelteKit 2, page components only receive
data(andform) as typed props.paramsis not injected as a component prop — it will beundefinedat runtime. This meansparams.personIdthrows aTypeErrorand the page crashes, or TypeScript widens the type and it silently evaluates to'Unknown'forever.Fix — pass
personIdthrough the load function:2.
new IllegalStateException(...)should beDomainException.internal()SenderModelService.java—triggerManualSenderTraining:IllegalStateExceptionhas no HTTP mapping. Spring's exception handler will return a 500 with a generic response, bypassing the structuredDomainException/ErrorCodepipeline.Fix:
(Add
OCR_TRAINING_CONFLICTtoErrorCodeif not already present, and a corresponding i18n key.)Suggestions (non-blocking)
runSenderTrainingispublic @Async: It can be called directly from outside the service, bypassing the proxy. The method is only intended for self-invocation viaself.runSenderTraining(personId). A short comment clarifying this would prevent accidental direct calls. Not a blocker, but it's a subtle trap.OcrStatCardsnumber display: Numbers like42rendered withgetByText('42')in tests — these pass now, but if another element on the same page shows42, the test becomes ambiguous. ConsidergetByRole('heading', { name: '42' })or wrapping in adata-testidfor the stat value. Low risk given isolated test rendering.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Solid security posture. All new endpoints are locked down correctly. Here's what I audited.
What I verified
Authorization on all new endpoints: All three endpoints carry
@RequirePermission(Permission.ADMIN)— the type-safe enum annotation, not a magic string. ✅Input validation:
TriggerSenderTrainingDTOuses@NotNull+@Valid @RequestBody. Spring returns 400 on a nullpersonIdbefore the service layer is reached. ✅Path variable coercion:
@PathVariable UUID personId— Spring coerces the string toUUIDand returns 400 on a malformed UUID. No injection risk. ✅Permission boundary tests: The test suite covers 401 (unauthenticated) and 403 (wrong role
READ_ALL) for all three new endpoints. This is exactly the right coverage — testing both auth failure modes, not just the success path. ✅No new public endpoints without auth: No
permitAll()additions. Existing security config unchanged. ✅Structured error codes:
DomainException.notFound(ErrorCode.PERSON_NOT_FOUND, ...)propagates cleanly to the frontend'sgetErrorMessage()mapper. ✅Open (tracked in #266, non-blocking here)
TriggerSenderTrainingDTO.personId?: stringin generated types: The backend field has@NotNulland@Schema(requiredMode = REQUIRED), but the generated TypeScript shows the field as optional. This means frontend TypeScript won't catch a missingpersonIdat compile time. The backend validation will still reject it at runtime (400), so no security bypass is possible — but it reduces the compile-time safety net. Issue #266 tracks the type regeneration post-merge.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Test coverage is strong across the board, but two gaps worth addressing.
What looks good
Controller permission boundary tests: All three new endpoints tested for 401, 403, and happy-path 200/202. The
verify(senderModelService, never()).runSenderTraining(any())assertion explicitly prevents regression of the architecture violation from cycle 1. ✅Service tests:
getGlobalTrainingHistory_returnsOnlyGlobalRuns,getSenderTrainingHistory_includesPersonName_andRuns,getSenderTrainingHistory_propagates404_whenPersonNotFound— complete happy + error path coverage. ✅Frontend load function tests: All three server load functions have
page.server.spec.tstests with happy and error cases. ✅Component tests:
OcrHealthBar,OcrModelsTable,OcrStatCardsall have behaviour-focused tests with locale-agnostic assertions. ✅Concerns
1.
getAllSenderModels()has no unit test inSenderModelServiceTestThe method delegates directly to
senderModelRepository.findAll(), but there's no test verifying the delegation. Trivial to add:2.
runSenderTraining()public async method has no dedicated unit testSenderModelService.runSenderTraining(UUID)is the@Asyncentry point called viaself.runSenderTraining(personId). The existing test suite coverstriggerManualSenderTraining(which calls it via the proxy mock) but doesn't testrunSenderTrainingitself — specifically that it delegates totriggerSenderTrainingwith the correct block count. A test like:Note
The
page.server.spec.tsnaming convention (without leading+) is intentional and consistent with other spec files in the project. Not flagging this.🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
Focus rings are correctly applied to all interactive links. Brand consistency is solid. One accessibility concern to address.
What looks good
Focus rings: All anchor elements now have
focus-visible:rounded-sm focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:outline-none. This is the correct pattern —focus-visiblenotfocus, so rings don't show on mouse click. ✅OcrHealthBar accessibility: The status dot uses
role="img"with anaria-labelcarrying the locale-appropriate text (m.ocr_status_online()/m.ocr_status_offline()). Color is never the only indicator — text label is also rendered. ✅Brand compliance:
text-brand-navy,border-brand-sand,bg-white,shadow-sm,rounded-smthroughout. Consistent with the card pattern. ✅Stat cards:
text-3xl font-bold text-brand-navyfor numbers (≈30px) is readable for the 60+ audience. Labels attext-xs uppercase tracking-widest text-gray-400(12px) are borderline but consistent with section headers elsewhere in the admin. ✅Back navigation: Arrow icon + translate animation matches the established project pattern. ✅
OCR icon in EntityNav:
aria-hidden="true"is correct — the adjacent text label carries the meaning for screen readers. ✅Concern
"Details" link in
OcrModelsTablehas no padding — touch target may be too smallThe
py-3is on the<td>, not the<a>. The touch target of the link itself is only as tall as the text (≈20px), which falls short of the 44px WCAG 2.5.5 minimum. The same issue applies to the person name link in the same row.Fix — add padding directly to the anchor tags:
Then remove
py-3from the<td>to avoid double-padding. This passes WCAG 2.5.5 and improves the experience on touch screens for the senior audience.Note
This admin section is typically accessed on desktop, which reduces urgency. Flagging as a concern rather than a blocker for that reason.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR. Here's what I verified.
What I checked
No new Docker services: The OCR admin pages consume the existing
ocr-servicecontainer (already in docker-compose). No topology changes needed. ✅No new environment variables: All new code uses existing Spring configuration (
${ocr.sender-model.activation-threshold:100}was already there). No.envchanges required. ✅No new open ports: New API endpoints route through the existing Spring Boot backend on port 8080. ✅
No new dependencies: Backend and frontend dependency trees unchanged. Renovate will continue to work as-is. ✅
Test suite coverage for CI: Backend adds ~50 new JUnit tests (controller + service). Frontend adds ~30 Vitest tests (components + load functions). CI should pass without any pipeline configuration changes. ✅
Build reproducibility: No pinned image tags changed, no Compose file modifications. Existing CI workflow is sufficient. ✅
Everything looks clean from a platform perspective. The feature is additive and contained entirely within existing infrastructure.
Review Cycle 3 — All Concerns Addressed
Blockers resolved
@Felix —
paramsnot a SvelteKit page proppersonId: params.personIdto[personId]/+page.server.tsreturnparams.personIdwithdata.personIdin+page.svelte; removed the erroneousparamsdestructuringpage.svelte.spec.tsthat verified the bug (RED: TypeError onparams.personId) and now passes GREEN with the fixfc892f0—fix(admin): pass personId through load fn instead of params prop; widen touch targets in table rows@Felix —
new IllegalStateExceptionshould beDomainException.internal()OCR_TRAINING_CONFLICTtoErrorCode.javaand mirrored inerrors.tsIllegalStateExceptionwithDomainException.internal(ErrorCode.OCR_TRAINING_CONFLICT, ...)inSenderModelService.triggerManualSenderTrainingtriggerManualSenderTraining_throwsDomainException_whenRunRowMissingAfterCreatethat confirmed the failure and now passes GREEN2466553—fix(ocr): replace IllegalStateException with DomainException.internal in triggerManualSenderTrainingConcerns resolved
@Sara — Missing
getAllSenderModels()unit testgetAllSenderModels_returnsAllModelsFromRepositorytoSenderModelServiceTest2466553@Sara — Missing
runSenderTraining()unit testrunSenderTraining_queriesBlockCountForPersontoSenderModelServiceTest2466553@Leonie — Touch targets on table links (WCAG 2.5.5)
py-3from<td>to<a>asinline-block py-3on both the person name link and Details link inOcrModelsTable.sveltefc892f0Test counts
page.svelte.spec.ts)👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Good progress from cycle 3. The
paramsSvelteKit bug is fixed correctly, component tests are in, touch targets look right, andtriggerManualSenderTrainingnow usesDomainException.internal(). One inconsistency remains.Suggestions
SenderModelService.java:157—new IllegalStateExceptionstill present intriggerSenderTrainingThe sister method
triggerManualSenderTrainingwas fixed to useDomainException.internal(ErrorCode.OCR_TRAINING_CONFLICT, ...)buttriggerSenderTrainingstill throws rawIllegalStateException:I know this path runs on an
@Asyncbackground thread so it won't affect HTTP responses directly — but it's inconsistent with the codebase convention and with the explicit fix made totriggerManualSenderTraining. Suggest applying the same change:What's Good
[personId]/+page.sveltenow readsdata.personId(not the non-existentparamsprop) ✅[personId]/+page.server.tscorrectly returnspersonId: params.personId✅page.svelte.spec.tscomponent tests cover the fix and the fallback ✅OcrModelsTable.sveltetouch targets:inline-block py-3on<a>instead of<td>✅ErrorCode.OCR_TRAINING_CONFLICTproperly documented in the enum ✅🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Architecture is clean throughout this PR.
What I Checked
Layering —
OcrController→SenderModelService→OcrTrainingRunRepository. No cross-domain repository access.PersonService.getById()is called through the service boundary. ✅Module boundaries — The new OCR admin endpoints (
/api/ocr/training-info,/api/ocr/train-sender) belong inOcrControllerand delegate toSenderModelService/OcrTrainingService. No leakage. ✅Self-reference pattern —
@Lazy @Autowired SenderModelService selffor@Asyncproxy dispatch is a known Spring tradeoff. The comment explains the necessity. The alternative (Spring Events or a dedicated async service) would be over-engineering for the current scale. Acceptable. ✅ErrorCode.OCR_TRAINING_CONFLICT— Correctly added to the enum with a javadoc comment explaining HTTP status (500) and the precise condition. Follows the existing documentation pattern inErrorCode.java. ✅Frontend data flow —
+page.server.tsloads on the server and returnspersonIdas part of the load result. Page component reads it fromdata. Correct SSR pattern with no client-side API calls. ✅No architectural concerns. The module is coherent and respects existing boundaries.
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Audited all new endpoints and frontend data flows. No security issues found.
What I Checked
Authorization on all new OCR admin endpoints — Every new endpoint carries
@RequirePermission(Permission.ADMIN)or@RequirePermission(Permission.READ_ALL):Input validation on
TriggerSenderTrainingDTO—@Valid @RequestBodyon the controller method and@NotNull UUID personIdon the record field. Spring will reject a missing or nullpersonIdwith 400 before the service is called. ✅Path parameter type safety —
@PathVariable UUID personIdongetSenderTrainingHistoryuses Spring's UUID type coercion, so a non-UUID path segment returns 400, not a DB query with a garbage value. ✅Frontend — no client-side API exposure — All data loading happens in
+page.server.ts. The component receivesdatavia$props(). Nofetch('/api/...')calls inonMount. ✅Error codes — no implementation details leaked —
OCR_TRAINING_CONFLICTmaps tom.error_internal_error()in the frontend, which is a generic user-facing message. The underlyingDomainExceptionmessage never reaches the client. ✅🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
Test coverage improved meaningfully in this PR. Prior cycle concerns addressed.
What I Checked
SenderModelServiceTest.java— 19 tests passing — Coverage now includes:triggerManualSenderTraining_throwsDomainException_whenRunRowMissingAfterCreate✅getAllSenderModels_returnsAllModelsFromRepository✅runSenderTraining_queriesBlockCountForPerson✅[personId]/page.svelte.spec.ts(new file) — Two component tests usingvitest-browser-svelte:personNamesmap ✅Both test against real DOM behavior via the browser mode — not JSDOM approximations. ✅
TDD evidence — The component test file was written to reproduce the
paramsbug (RED first), then the page was fixed to make it pass (GREEN). Correct process. ✅[personId]/page.server.spec.ts— Load function tests cover 200 and error paths. ✅Suggestion
triggerSenderTraininginSenderModelServicestill has no test coverage for the path where the RUNNING row is missing after creation. If Felix's suggestion to replacenew IllegalStateExceptionwithDomainException.internal()is applied, a test similar totriggerManualSenderTraining_throwsDomainException_whenRunRowMissingAfterCreatewould be the right follow-up — though I note this path runs on a background thread and is harder to test in isolation.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
Accessibility concerns from the previous review cycle have been correctly addressed.
What I Checked
Touch targets on
OcrModelsTable.svelte— The fix is correct.py-3(12px vertical padding + ~20px line-height = ~44px hit area) is now applied directly to the<a>element asinline-block py-3:Previously
py-3was on<td>, which expanded the cell but not the link's clickable area. Now the<a>itself isinline-blockwith vertical padding — the tap target meets WCAG 2.5.5 (44×44px minimum). Applied to both the person name link and the "Details" link. ✅Focus rings — Both links carry
focus-visible:rounded-sm focus-visible:ring-2 focus-visible:ring-brand-navy focus-visible:outline-none. Keyboard navigation is visible. ✅[personId]/+page.svelte— The back link and heading structure are clean.<h1>withpersonNamederived correctly. The back link follows the project's established back-link pattern. ✅Brand compliance — Colors, typography (
uppercase tracking-widest font-bold), and card container patterns match the project design system. ✅No remaining UI/accessibility concerns on the changed files.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR. Checked what was relevant to my domain.
What I Checked
No new services or ports — The OCR admin feature is implemented entirely within the existing Spring Boot backend and SvelteKit frontend. No new containers, volumes, or networking changes. ✅
No environment variable additions — The new
@Value("${ocr.sender-model.activation-threshold:100}")and@Value("${ocr.sender-model.retrain-delta:50}")fields have safe defaults (100and50). They're optional overrides, not required configuration. No Docker Compose env changes needed. ✅No CI workflow changes — Test pipeline is unchanged. The new frontend component tests run in the existing
vitest-browser-sveltesetup. The new backend tests run in the existing Mockito unit test suite. No additional CI steps or time impact. ✅@Asyncthreading — TheSenderModelServiceuses Spring's existing async executor for background training runs. No new thread pool configuration required. ExistingAsyncConfighandles it. ✅Nothing for me to flag. Clean from a platform perspective.
Cycle 4 fixes — commit
16bcd0fConcern addressed
Felix @felixbrandt —
new IllegalStateExceptionintriggerSenderTraininginconsistent with project styleFix:
SenderModelService.java:157— replacednew IllegalStateException(...)withDomainException.internal(ErrorCode.OCR_TRAINING_CONFLICT, ...), matching the pattern already applied totriggerManualSenderTraining.Test added:
triggerSenderTraining_throwsDomainException_whenRunningRowMissingAfterDispatch— confirmed RED withIllegalStateException, GREEN after fix. All 20SenderModelServiceTesttests pass.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
My cycle 4 concern was addressed correctly.
SenderModelService.java:155-159now reads:Both defensive paths in
SenderModelService—triggerManualSenderTrainingandtriggerSenderTraining— now useDomainException.internal(ErrorCode.OCR_TRAINING_CONFLICT, ...). Consistent. ✅The new test
triggerSenderTraining_throwsDomainException_whenRunningRowMissingAfterDispatchwas written RED first (confirmed by the commit history showing it was added before the fix), and 20 tests pass green. ✅No further concerns. The full feature is clean.
🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
No new architectural concerns in cycle 5. The fix is a one-liner change that improves consistency within the module. Architecture review stands from cycle 4. LGTM.
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
The cycle 5 change is a single exception type replacement in a background-thread method. No security surface area changed. Security review stands from cycle 4. LGTM.
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
SenderModelServiceTestnow has 20 tests passing, includingtriggerSenderTraining_throwsDomainException_whenRunningRowMissingAfterDispatchwhich covers the defensive path I flagged in cycle 4. The test confirms the exception type contract for background-thread failure scenarios. Test suite is solid. LGTM.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
No UI or frontend changes in cycle 5. UI/accessibility review stands from cycle 4. LGTM.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in cycle 5. Platform review stands from cycle 4. LGTM.
Replaces catch(Exception ignored){} with log.debug() in getTrainingInfo(). Adds controller test documenting the graceful degradation behavior (response stays 200 when personService.getById() throws). Fixes reviewer concerns from @felixbrandt and @nullx. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Remove the intermediate Map<String,Object> and return the typed record directly so OpenAPI codegen produces a concrete TypeScript type. Fixes lastRun serializing as {} (empty object) instead of null when no training run exists. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Add findByPersonIdIsNullOrderByCreatedAtDesc + findByPersonIdOrderByCreatedAtDesc to OcrTrainingRunRepository. Add dto/TrainingHistoryResponse. Expose GET /api/ocr/training-info/global and GET /api/ocr/training-info/{personId} on OcrController, both requiring ADMIN; getSenderTrainingHistory guards person existence via PersonService and returns 404 for unknown personId. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>- Replace hardcoded EN strings in OcrHealthBar/OcrStatCards/OcrModelsTable with Paraglide message keys (de/en/es translations added) - Add role=img + aria-label to OcrHealthBar status dot - Add {:else} empty-state row in OcrModelsTable - Fix personName derivation in [personId]/+page.svelte to use params.personId key instead of Object.values()[0] (fragile when multiple persons present) - Update OcrModelsTable spec to assert empty-state row structure (locale-agnostic) - Add missing availableSegBlocks test to OcrStatCards spec Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>