feat: bulk metadata edit for existing documents #331
Reference in New Issue
Block a user
Delete Branch "feat/issue-225-bulk-metadata-edit"
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 #225.
12 atomic commits implementing post-hoc bulk editing of existing documents.
What it does
WRITE_ALL users can tick checkboxes on
/documentsand/enrichrows. A live‐accumulator selection store powers a sticky bottom bar.Massenbearbeitungjumps to a new/documents/bulk-editroute that reusesBulkDocumentEditLayoutvia a newmode="edit"prop. Save chunks IDs into 500-sizedPATCH /api/documents/bulkrequests sequentially; per-document errors become red chips, chunk-level failures stop with a partial-save retry. TheAlle X editierenfast path replaces the selection with every UUID matching the active filter viaGET /api/documents/ids.Backend
PATCH /api/documents/bulk— WRITE_ALL, 500-ID cap, partial-failure response shape,bulkEdit actor=… documentIds=… updated=… errors=…audit logPOST /api/documents/batch-metadata— READ_ALL, summaries (id, title, server PDF URL) for the bulk-edit page's left stripGET /api/documents/ids— READ_ALL, every UUID matching the active filter, ignores page/sizeErrorCode.BULK_EDIT_TOO_MANY_IDSDocumentService.applyBulkEditToDocumentis wrapped in its own@Transactionalso a per-document failure cannot partially mutate other documents in the batchtagNamesandreceiverIdsadditive,senderId/documentLocation/archiveBox/archiveFolderreplace-on-non-blankFrontend
bulkSelectionSvelteSet store (live accumulator)canWrite-gated row checkboxes on/documentsand/enrich(44px touch targets, aria-label includes title)BulkSelectionBar— sticky bottom bar with count,Alles aufheben,Massenbearbeitung(iOS safe-area honoured)Alle X editierenbutton next to the result count/documents/bulk-editroute (server load redirects READ_ALL away)BulkDocumentEditLayout mode="edit"— hides drop zone, read-only title in strip, hidden date, server PDFs,role="note"onboarding callout, additive/replace badges via newFieldLabelBadgecomponent, chunked PATCH save with progress + retryDecisions made during planning
/documentsand/enrichonlyAlle X editierenactiondocumentIdsPATCH bodydocumentIds is requiredGET /api/documents/idscap+page.server.ts, redirect to/documentsrole="note"FieldLabelBadgecomponent, variants:additive,replaceTest plan
./mvnw test— backend 1334 / 1334 greennpx vitest runfor the bulk-edit specs — 69 / 69 green across 6 spec filesnpx playwright test bulk-edit— 5 passed, 1 skipped (no incomplete docs in test DB)admin@familyarchive.local, tick two rows on/documents, hit Massenbearbeitung, add a tag, save, verify both documents got the tag🤖 Generated with Claude Code
- 14 new Paraglide keys in de/en/es for the bulk-edit UI strings (selection bar, callout, badges, save progress, retry, error) - BULK_EDIT_TOO_MANY_IDS added to errors.ts type union and getErrorMessage() - Regenerated api.ts now includes /api/documents/{bulk,batch-metadata,ids} and the DocumentBulkEditDTO / BulkEditResult / DocumentBatchSummary schemas Refs #225 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🏗️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
Solid feature on the whole. Layering is respected (controller → service → repo, tags via
tagService.findOrCreate, persons viapersonService.getById/getAllById— no cross-domain repo reach), per-doc tx isolation is the right call, and the controller honestly returns a partial-success shape instead of pretending the loop is atomic. The PATCH cap is wired symmetrically into the frontend chunker (BulkDocumentEditLayout.svelte:206chunkSize = 500matchesBULK_EDIT_MAX_IDS = 500). Tests cover both layers properly.What worries me is two things: bulk edits silently bypass the audit/version trail that single-doc updates produce, and the
/ids+/batch-metadataendpoints have no input caps and will misbehave under realistic data volume. Neither is a "wrong design" — they are gaps, fixable in a small follow-up — but they are real enough that I want them addressed before this lands.Blockers (must fix before merge)
Bulk edits do not emit audit events or document versions —
DocumentService.applyBulkEditToDocument(backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java:405-442) saves the document and returns. Compare withupdateDocumentat:317-325, which callsdocumentVersionService.recordVersion(saved)andauditService.logAfterCommit(METADATA_UPDATED, …). A user mass-editing 500 documents leaves zero trace in the audit log and zero entries indocument_versions. For a family archive whose value proposition is "we know who did what to letter X" this is an architectural regression — it punches a 500-row hole through the very mechanism the rest of the app uses to reconstruct history. Either:documentVersionService.recordVersion(saved)andauditService.logAfterCommit(AuditKind.METADATA_UPDATED, actorId, saved.getId(), Map.of("source", "BULK_EDIT"))from the service method (preferred — keep parity withupdateDocument), orAuditKind.BULK_METADATA_UPDATEDand emit one event per document plus, optionally, one summary event per request.Add the
actorIdparameter toapplyBulkEditToDocument(UUID id, DocumentBulkEditDTO dto, UUID actorId)— the controller already resolves it onDocumentController.java:262but throws it away.GET /api/documents/idsis unbounded —DocumentController.java:284-298accepts every filter parameter as optional and forwards tofindIdsForFilter, which ends indocumentRepository.findAll(spec).stream().map(...).toList()(DocumentService.java:379). Hit the endpoint with no params and you load every UUID in thedocumentstable into memory and serialize it as JSON. The bulk-edit feature itself caps the write side at 500, but a "Alle X editieren" call against an unfiltered table can return 50k IDs today and 500k in a few years — and the next request immediately POSTs them to/batch-metadata, which thenfindAllByIds all of them. Add a server-side hard cap (e.g. 5000 IDs returned, with a 4xx ifcount(spec) > cap) or require at least one filter parameter. The same cap belongs onBatchMetadataRequest.ids— currently the only check isisEmpty()(DocumentController.java:303).Concerns (should fix before merge)
findIdsForFilterandsearchDocumentsduplicate the entire specification chain —DocumentService.java:359-380reproduces lines 498-517 verbatim (FTS prefilter +expandTagNamesToDescendantIdSets+ the seven-spec chain). Two copies of a non-trivial query graph means the next bug fix to one of them silently desyncs the other. Extract a privateSpecification<Document> buildSearchSpec(...)helper (or a small parameter object) and have both methods consume it. Cheap refactor, large maintainability win.The two new DTO names (
DocumentBatchSummary,BatchMetadataRequest) collide conceptually with existingDocumentBatchMetadataDTO—dto/DocumentBatchMetadataDTOis the quick-upload per-batch input; the newBatchMetadataRequestis the bulk-edit ID list;DocumentBatchSummaryis the bulk-edit response. Three "batch" words, three different shapes, no convention separating them. As we move toward feature packaging this gets worse. Suggestion: rename toBulkEditIdsRequestandBulkEditDocumentSummaryso theBulk*prefix groups everything this PR added (matchesBulkEditResult/BulkEditErroralready in place). Pure rename; no behaviour change.Controller-level loop with per-item
@Transactionalis fine, but document the trade-off —DocumentController.java:266-276opens 500 transactions on the HTTP thread of one request. Under contention with other writers (mass-import, OCR commit) this can serialize on row locks for the same documents and stretch a single PATCH well past the connection-pool's per-thread time budget. Acceptable for now (issue scale is family archive, not Salesforce), but the comment block aboveapplyBulkEditToDocumentshould call this out so the next person doesn't naively raiseBULK_EDIT_MAX_IDSto 50k. Consider adding a structured-log warning when a single bulk request takes > 5s wall-clock.DocumentBulkEditDTOis a Lombok@DataPOJO while every other new DTO in this PR is arecord—dto/DocumentBulkEditDTO.java:13vsBulkEditResult/BulkEditError/BatchMetadataRequest/DocumentBatchSummarywhich are all records. Java 21 records are the project default for input DTOs going forward. The mutable@Dataform is only justified when the DTO needs@ModelAttributeform binding, which this one doesn't (it's@RequestBodyJSON). Convert to a record for consistency and immutability.Suggestions (nice to have)
Fully-qualified type names inside
DocumentService—DocumentService.java:388, 391, 406referenceorg.raddatz.familienarchiv.dto.DocumentBatchSummaryandDocumentBulkEditDTOinline instead of importing them. The rest of the file imports its DTOs at the top. Add the imports.@Transactional(readOnly = true)onfindIdsForFilterandbatchMetadata— both are read-only paths. Marking them readOnly lets Hibernate skip dirty-checking on the loaded entities, modest savings on the 5k-rowfindAllpaths.ADR for the additive-vs-replace bulk-edit semantics — "tags merge, receivers merge, sender replaces, location fields replace-on-non-blank" is an opinionated UX choice that future contributors will second-guess. The Javadoc on
applyBulkEditToDocument(DocumentService.java:398-404) captures it well; lift that paragraph intodocs/adr/so the rationale survives the next refactor.BulkDocumentEditLayout.svelteis now ~512 lines and serves two modes (upload|edit) — manageable today, but watch it. A third mode (e.g. bulk-delete, bulk-status-change) will push this past the point where two-state-machines-in-one-file remains readable. Consider extractingsaveUpload/saveBulkEditinto thin per-mode service modules under$lib/services/once a third use-case appears.What I checked
personService/tagServicedocument_tagscascade anddocuments.sender_idFK; no schema changes needed and none introduced (good)@Transactionalactually applies)/idsand/batch-metadatafindIdsForFilterandsearchDocuments+page.server.tsredirects non-WRITE_ALLusers) and its alignment with@RequirePermission(WRITE_ALL)on the backend — both ends gated, goodBULK_EDIT_TOO_MANY_IDSpresent inErrorCode.java,errors.ts, and i18n messages)chunkSize = 500) and backend (BULK_EDIT_MAX_IDS = 500) — matches🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ⚠️ Approved with concerns
Operationally this is a clean, additive change — no infra moves, no migrations, no new services, no new env vars. Rollback is
git revert, which is exactly what I want from a feature like this. But there are two real prod-risk items I want fixed before this lands, and a couple of pool/audit questions worth a follow-up issue.Blockers (must fix before merge)
(none)
I will not block on this — the request budgets are within proxy/tomcat limits and the rollback path is trivial. The two items below are close-to-blocker but I'd rather see them resolved than gate the PR.
Concerns (should fix before merge)
1. Audit trail is silent for bulk edits —
backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java:405-442applyBulkEditToDocumentmutates tags / sender / receivers / location fields and saves, but unlikeupdateDocument(line 324) it never callsauditService.logAfterCommit(AuditKind.METADATA_UPDATED, ...). The controller writes a single batch-levellog.info("bulkEdit actor=… updated=… errors=…")(DocumentController.java:278), which is fine for ops, but the per-document audit log feedsAuditLogQueryService.findRecentContributorsPerDocumentand the activity feed (DocumentService.java:568). After this lands, anyone bulk-editing 500 documents disappears from the contributor surface for those documents. That's a real regression in observability of who did what to which document — exactly the kind of thing we'll want when somebody asks "wait, who tagged all of these wrong?". Add the audit call insideapplyBulkEditToDocument, ideally with a small payload like{"source":"bulk"}so we can distinguish bulk from single-doc edits later.2. No bean-validation on
DocumentBulkEditDTO—backend/src/main/java/org/raddatz/familienarchiv/dto/DocumentBulkEditDTO.java:13-21The controller is
@Validated(DocumentController.java:76) but the DTO carries no@Size, no@NotBlank, no length caps. The 500-ID cap ondocumentIdsexists (good — DocumentController.java:247-260), but a single request can still legally sendtagNameswith 100k entries, or adocumentLocationof arbitrary length. SvelteKit's 1 MiB proxy cap (frontend/src/routes/api/[...path]/+server.ts:42-52) is the only real backstop, and that's an 8MB-default Jetty body away from the backend. Add at minimum@Size(max=200)ontagNamesandreceiverIds, and@Size(max=255)(or the actual column length) on the three location strings. The N+1 cost (next item) makes this matter more than it would otherwise.Suggestions (nice to have)
3. DB pressure: 500 sequential per-document transactions per request
@Transactionalis onapplyBulkEditToDocument— not onpatchBulk. So a single/api/documents/bulkcall with 500 IDs opens, commits, and closes 500 separate transactions inside one HTTP request, each one also fanning out totagService.findOrCreate(name)for every tag andpersonService.getById/getAllByIdfor sender/receivers. With 10 added tags × 500 documents that's ~5,000 tag-resolve queries on top of 500 document loads + saves, all holding one HikariCP slot for the duration. On the dev box this is fine; on the CX32 with PgBouncer in front, a couple of concurrent bulk-edits will starve the pool. Two cheap improvements:Personentities once before the loop.BulkDocumentEditLayout.svelte:205), so a tighter backend cap is invisible to the user but bounds tail latency.Not a release-blocker — the DB will survive — but worth a follow-up issue with a "perf" label.
4. Frontend chunk size matches backend cap exactly —
frontend/src/lib/components/document/BulkDocumentEditLayout.svelte:205chunkSize = 500and backendBULK_EDIT_MAX_IDS = 500— fine today, but if we ever tune the backend cap down, the client breaks immediately withBULK_EDIT_TOO_MANY_IDS. Set the client to e.g. 250 so there's headroom. The cost is one extra round-trip per 500 docs — negligible.5. No rate limit on the new endpoints
RateLimitInterceptorexists inconfig/. Worth confirming it coversPATCH /api/documents/bulk— a single misbehaving client looping a 500-doc bulk-edit can pin a backend thread for several seconds per call. Follow-up, not a blocker.6. Body-size sanity check — passes
For the record: 500 UUIDs as JSON ≈ 19 KB, plus tag names + receiver UUIDs, well under the 1 MiB SvelteKit proxy cap.
/batch-metadatahas the same shape. No proxy tuning needed.What I checked
docker-compose.yml— no infra changes, no new services, no new ports, no new env vars (good)backend/src/main/java/.../controller/DocumentController.java— new endpoints/bulk,/batch-metadata,/ids; permission gates correct (WRITE_ALLon patch,READ_ALLon the read-side endpoints); 500-ID cap enforcedbackend/src/main/java/.../service/DocumentService.java— transaction scope, audit logging gap, N+1 fan-out on tag/person resolutionfrontend/src/routes/api/[...path]/+server.ts— 1 MiB proxy cap holds; new endpoints fit comfortablyfrontend/src/lib/components/document/BulkDocumentEditLayout.svelte— client chunking at 500, partial-failure handlingfrontend/e2e/bulk-edit.spec.ts— covers selection-bar UX; doesn't exercise the actual PATCH (acceptable, that's unit-test territory)🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
The bulk surface is well-fenced on the perimeter — every new endpoint is annotated with the right
@RequirePermission, the 500-ID cap is enforced server-side and tested, the proxy pre-buffers the body and rejects oversize payloads, andfindIdsForFilterreuses the same JPA Specification chain as the search endpoint (parameterised, no string concatenation). I have no SQLi, no auth bypass, no XSS via reflected error messages, and no CSRF concern given the Authorization-header model. The blocker list is empty. The concerns below are real but bounded — fixable in this PR or a quick follow-up.Blockers (must fix before merge)
(none)
Concerns (should fix before merge)
C1.
POST /api/documents/batch-metadatahas no per-request size cap (CWE-770: Resource Exhaustion)DocumentController.java:300-307andDocumentService.batchMetadata(DocumentService.java:388-396) accept an unboundedList<UUID>. AREAD_ALLuser can POST a{"ids":[...]}body with thousands of UUIDs (the proxy permits up to 1 MiB → ~26k UUIDs as JSON), forcingdocumentRepository.findAllById(ids)to materialise the full set. ThePATCH /bulkandquick-uploadendpoints both have explicit caps (500 / 50). This one does not.Add a regression test mirroring
patchBulk_returns400_whenDocumentIdsExceedsCap.C2.
GET /api/documents/idsis unbounded by design — make the bound explicit (CWE-770)DocumentController.java:284-298callsfindIdsForFilter, which loadsdocumentRepository.findAll(spec)and maps to UUIDs (DocumentService.java:359-380). With ~10k documents this is fine; with no filter at all it returns the entire ID set. That's intentional ("Alle X editieren" fast path), but it is also the only public endpoint that returns a list with no pagination contract. Two mitigations:@Queryprojection that selects only theidcolumn (avoids hydrating full entities + tags/receivers eagerly via the Specification — currently this materialises the whole row graph just to throw it away).documentstable size as a soft cap, or enforce a hard cap server-side and let the frontend page through if exceeded. Right now there is no defence if the table grows past expected size.C3. Frontend reflects backend error
messagedirectly into BulkEditError list — verify the rendering path stays escaped (CWE-79 defence-in-depth)BulkEditError.message(dto/BulkEditError.java) is a plain string sourced fromDomainException.getMessage()— which often contains attacker-influenced data ("Document not found: " + id; the id comes from the request, but attribute names liketagNamescould in future contain user-supplied substrings). The frontend doesn't display individual error messages today (BulkDocumentEditLayout.svelte:244-249just setsstatus: 'error'per id). Svelte's{}interpolation auto-escapes, so this is not currently exploitable, but if anyone later renderserr.messagevia{@html ...}(or the i18n layer with raw HTML interpolation) we have a stored-then-reflected XSS sink. Two cheap improvements:DomainException→ a typed code (e.g."DOCUMENT_NOT_FOUND") instead of a free-form string, mirroringBulkEditErrorto{id, code}. The frontend then translates via Paraglide. No user-controlled string ever round-trips.BulkDocumentEditLayoutnever passeserr.messagethrough{@html}.C4. Log injection potential in two new log lines (CWE-117)
DocumentController.java:274(log.warn("Bulk edit failed for document {}: {}", id, e.getMessage());) and:278-279(bulkEdit actor=... documentIds=... ...). The placeholders themselves are safe — SLF4J does not interpolate or evaluate them — bute.getMessage()can contain CR/LF (aDomainExceptionconstructed from a path that includes user input could carry newlines). With a plain-text log appender that lets an attacker forge log entries by injecting\n. Low severity given the message origin is the service layer (UUIDs andDOCUMENT_NOT_FOUNDconstants), but worth a CRLF strip in the audit-relevant lines:A repo-wide SLF4J helper would be even better, since this issue exists outside this PR too — but at minimum, lock down the new lines.
C5. Audit log is missing for bulk edits (defence-in-depth, audit completeness)
updateDocumentcallsauditService.logAfterCommit(METADATA_UPDATED, …)for every single-doc update.applyBulkEditToDocument(DocumentService.java:405-442) does not. A user withWRITE_ALLcan quietly modify 500 documents per request with zero audit trail beyond a singlelog.infoline in the application log (which is not the audit log and not queryable by admins viaAuditLogQueryService). For a family archive where intentional bad-actor scenarios are low but accidental damage is real, post-hoc accountability matters. Add:inside the per-document path. This also lets
Markus(architect) keep the audit story consistent across single vs bulk paths.Suggestions (nice to have)
S1.
applyBulkEditToDocumentis@Transactionalper call — but the controller loops 500× synchronously. Each iteration opens a new transaction. That's correct semantically (one-doc failure doesn't roll back the rest, which is what you want), but it creates 500 round-trips to Postgres for a worst-case batch. Not a security issue, just a request-time DoS amplifier when the cap is hit. Consider chunked commits with explicit savepoints if this becomes hot.S2.
DocumentBulkEditDTOuses Lombok@Data(mutable) and accepts arbitrary string lengths. Add@Size(max=255)ondocumentLocation,archiveBox,archiveFolderand@Size(max=200)per element oftagNames. Otherwise a singlearchiveBoxfield set to a 1 MiB string sails through and propagates to every doc in the batch — multiplying storage cost.S3. The 1 MiB body cap in the SvelteKit proxy fits a 500-UUID PATCH comfortably (~22 KiB) but is binding for the catch-all proxy. Confirmed fine for this PR. Worth a comment in
+server.tsnoting which endpoint sets the upper bound, so future contributors don't drop the cap when adding bigger uploads.S4. Consider a Semgrep rule to enforce per-request caps. Pattern: any
@PatchMappingor@PostMappingwhose handler accepts aList<UUID>orList<...>field in the body and does not call avalidateBatch/validateCaphelper — flag for review. Catches future bulk endpoints landing without the same protection.What I checked
PATCH /bulk,GET /ids,POST /batch-metadata) — correctly gated via@RequirePermission, and 401/403 tests are present inDocumentControllerTest.java:946-998WRITE_ALLusers as fully trusted writers — consistent with single-docupdateDocumentfindIdsForFilterandbatchMetadata— both go through JPA Specifications andfindAllById, parameterised throughoutlog.warn/log.infostatementsBulkEditError.messageround-trip (Svelte auto-escaping holds)SecurityConfig.java:41-45proxy()function, no special-casingDocumentControllerTest.java🤖 Reviewed in character as Nora "NullX" Steiner.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Solid feature. Backend is exemplary — guard clauses, partial-failure shape, transactional boundary per document, every behaviour TDD-covered (
DocumentServiceTest.java:1921-2191,DocumentControllerTest.java:933-1107). Frontend has good test depth too. The blockers below are real but small in scope; the concerns are about coupling and a few rule-violations I'd want cleaned up before this lands.Blockers (must fix before merge)
1. Top-level
$bindablemutation outside any reactive scope — Svelte 5 anti-pattern.frontend/src/lib/components/document/WhoWhenSection.svelte:37andDescriptionSection.svelte:36, 42:Re-assigning a bindable prop at the top level of
<script>(outside$effect/ event handler / lifecycle) writes back to the parent on every reactive run. InBulkDocumentEditLayoutthe parent ownsdateIsoas$state(''); this synchronous overwrite from a child can stomp the parent's value when props change (e.g. wheninitialEditEntrieshydration triggers a re-render). The DescriptionSection version is worse — it overwritesdocumentLocationwithinitialDocumentLocationon every re-run, which can clobber typed input the moment the parent re-renders. Move these into$effect(initialise-only with auntrack-guarded "first run" flag) or — better — drop theinitial*props entirely and have the parent seed its own$statefrom props before mounting the child.2. Layering rule violation:
applyBulkEditToDocumentre-implements tag merging instead of delegating.backend/.../service/DocumentService.java:410-419callstagService.findOrCreate(clean)directly inside a hand-written merge loop. The existingupdateDocumentTags(docId, tagNames)(line 334) is the canonical place for this, but it's a replace operation. The right move is to addmergeDocumentTags(docId, tagNames)to keep the tag-resolution logic in one place; right now we have the samename.trim()+findOrCreatefilter duplicated three times in this file (lines 340-347, 173-179, 412-417). One of them will drift.3.
BulkDocumentEditLayout.svelteis 511 lines doing two completely different jobs.The
mode === 'upload'andmode === 'edit'branches share the file-switcher chrome and not much else: separate save handlers (saveUploadvssaveBulkEdit), separate state shapes (Fileblobs vs document IDs + server URLs), separate empty states, separate metadata semantics. This is the classic boolean-flag-argument smell at the component level. Per the persona doc: "components handle one visual concern; 40 lines of template markup is the splitting signal." Split intoBulkUploadLayout.svelteandBulkEditLayout.sveltewith a shared<BulkPdfPreview>/<FileSwitcherStrip>. The save logic can share a tinychunkAndSave(items, chunkSize, request)helper.Concerns (should fix before merge)
1. Bulk-edit page is client-side only — no SSR, no server-side auth check on data fetch.
frontend/src/routes/documents/bulk-edit/+page.svelte:14-38does thePOST /api/documents/batch-metadatainonMount. The persona's "Secure Code" rule explicitly forbids this pattern: "Data flows from+page.server.tsvia props — never client-side API fetch." The +page.server.ts here only checkscanWriteand bounces, but the actual data fetch happens in the browser. Move it to the load function (server readsbulkSelectionStore-equivalent IDs from a query string or POST body would need a different approach — easiest: store the ID list in a server-set cookie or session entry when the user clicks "Massenbearbeitung", then read it server-side). Today this also means the loading spinner is unavoidable on every navigation; SSR would render the bulk-edit screen ready-to-use.2. Missing
getErrorMessage()mapping in bulk-edit fetch handlers.bulk-edit/+page.svelte:27showsm.error_internal_error()for any!res.ok, throwing away thecodefield the backend provides.BulkDocumentEditLayout.saveBulkEdit(lines 230-256) does the same — surfaces only "{done}/{total} saved" without telling the user why. Parse the body, extractcode, route throughgetErrorMessage(code). TheBULK_EDIT_TOO_MANY_IDSmapping you added inerrors.ts:146is the right pattern; the calling code just doesn't use it.3.
DocumentBulkEditDTOis a mutable@DataPOJO; everything else in the new DTOs is arecord.Compare
BulkEditError,BulkEditResult,BatchMetadataRequest,DocumentBatchSummary— all immutable records.DocumentBulkEditDTO(line 13) is@Data @NoArgsConstructor @AllArgsConstructor. There's no Spring/Jackson reason for the inconsistency; switch to a record (or a builder if you want optional-friendly construction in tests).4.
BulkSelectionBarmounted at two routes —/documentsand/enrich— but it's a global selection.bulkSelection.svelte.ts:7is a module singleton. If a user toggles a doc on/enrich, navigates to/documents, the selection persists — that's intentional and documented in the comment. But the bar disappears on/persons,/conversations, etc., even though selection is still live. That's a UX trap: a stale 3-doc selection follows the user invisibly until they happen back to /documents. Either show the bar globally (mount in+layout.svelte) or auto-clear()on route change away from the two source routes. Pick one.5.
editAllMatchingswallows fetch failures silently./documents/+page.svelte:170-173:No toast, no error UI — the button just snaps back to enabled. From the user's perspective the click did nothing. At minimum, surface a
getErrorMessage()-mapped error.6.
bulkSelectionStoreexposesidsbutBulkSelectionBarreadssizeandclear()only — theidsgetter is unused outside the page.svelte hydration.Minor surface bloat. Fine to leave, but worth noting that
add/remove/toggle/setAllis the full API;idsis the only escape hatch.Suggestions (nice to have)
patchBulk_returns400_whenDocumentIdsExceedsCapuses 501 IDs. Add a_acceptsExactly500_atCap()mirror so a future off-by-one in the comparison (>vs>=) is caught. ThevalidateBatch_doesNotThrow_whenFileCountEqualsCapExactlytest is the right model.applyBulkEditToDocumentdoesn't audit-log. CompareupdateDocument(line 320-325) which logsMETADATA_UPDATED. Bulk edits across hundreds of docs vanish from the audit trail entirely. If this is intentional, document it in the Javadoc; if not, add alogAfterCommitper doc.DocumentServiceis now 944 lines. Not introduced by this PR, but the new bulk methods push it deeper into "god class" territory. Worth a follow-up to extractDocumentBulkService. (Question for Markus, not a fix here.)FileSwitcherStripkeyboard handler attached via$effect. Works, but a<svelte:window>oronkeydownon the<ul>itself would be more idiomatic and avoid the manualaddEventListener+ cleanup pair. (Lines 50-74.)/documents/bulk-edit/+page.svelte:46shows a literal…as the loading state. Replace with a real spinner / skeleton — every other route in the codebase uses one.import { m } fromvsimport * as m from). New code uses both. Pick one in CODESTYLE.md.What I checked
applyBulkEditToDocument,batchMetadata,findIdsForFilter— guard clauses, transactional boundaries, layering,DomainExceptionusage,ErrorCodemirror in frontendDocumentServiceTestlines 1921-2191 (every behaviour covered, including null/empty/blank edge cases),DocumentControllerTestlines 933-1107 (auth matrix, 400 paths, partial-failure shape)DocumentBulkEditDTO,BulkEditError,BulkEditResult,BatchMetadataRequest,DocumentBatchSummary—@Schema(REQUIRED)discipline, record vs@DataconsistencyBulkDocumentEditLayout(size/responsibility/Svelte 5 idioms),BulkSelectionBar,FieldLabelBadge,WhoWhenSection+DescriptionSection(bindable prop mutation),FileSwitcherStrip(keyed{#each}✅, $effect with cleanup),bulkSelection.svelte.ts(SvelteSet usage ✅),DocumentRowcheckbox integrationBULK_EDIT_TOO_MANY_IDSmirrored inerrors.ts✅ but unused at the calling sites/documents/bulk-edit/+page.server.tspermission guard, layout overlay using--header-heightCSS var🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
The mental model is right: an additive/replace badge per field is exactly the affordance bulk edit needs, the sticky bottom bar mirrors patterns users already know from photo apps, and "Alle X editieren" is a thoughtful escape hatch. Touch targets on the row checkbox are correctly 44×44 with
min-h-[44px] min-w-[44px], the bottom bar honorsenv(safe-area-inset-bottom)for iOS notches, and the partial-failure card isrole="alert". Good baseline. The blockers below are mostly polish — none of them is structural.Blockers (must fix before merge)
Hardcoded German labels for "Karton" and "Mappe" (
DescriptionSection.svelte:130, 145) — WCAG 3.1.1/3.1.2 (Language of Page/Parts). Every other label in this section flows through Paraglide; these two are bare strings. English and Spanish users will see German labels with no fallback. Addform_label_archive_boxandform_label_archive_folderto all three message files and reference viam.form_label_archive_box(). While you're there: the user has no idea what "Karton" vs "Mappe" mean physically — they need a one-line helper text below each input the wayform_helper_archive_locationalready does for "Aufbewahrungsort".Hardcoded German
aria-label="Hinweis zur Massenbearbeitung"(BulkDocumentEditLayout.svelte:371) — WCAG 3.1.2. Screen-reader users on the EN/ES locale will hear German pronounced phonetically by their TTS engine — unintelligible. Either route throughm.bulk_edit_hint_aria_label()or just drop thearia-labelentirely (arole="note"whose visible text content is the hint is already self-describing — the redundant label actually overrides the visible text for AT users, which is a regression).No keyboard route to clear the bulk selection (
BulkSelectionBar.svelte) — WCAG 2.1.1 (Keyboard). The bar appears on selection but has no Escape handler. A keyboard user who selects 50 rows and then wants to bail must Tab through the entire footer to reach "Alles aufheben". Add a global<svelte:window onkeydown={(e) => e.key === 'Escape' && clearAll()} />whilecount > 0. Bonus: visible hint "Esc: Auswahl aufheben" at ≥768px on the bar itself.Sticky bar overlaps the last document row and pagination (
/documents/+page.svelte,/enrich/+page.svelte) — WCAG 1.4.10 (Reflow) / 2.4.7 (Focus Visible — the focused last-row link can be hidden behind the bar). The bar isposition: fixed~64px tall but neither page reserves bottom padding whencount > 0. On mobile this hides "Vorherige/Nächste Seite" entirely and the last document gets clipped. Fix: addclass:pb-24={bulkSelectionStore.size > 0}(or equivalent) to the<main>wrapper so the scroll container has room. Same fix on/enrich.Concerns (should fix before merge)
FieldLabelBadge.svelte:13—text-[10px]violates the persona's own 12 px minimum-text floor and arguably WCAG 1.4.4 (Resize Text) for the senior audience. Contrast itself (text-gray-600onbg-muted= 7.4:1) is fine, but the size is the bigger problem — readers in the 60+ cohort will see "+ wird hinzugefügt" as a smudge. Bump totext-[11px]minimum, ideallytext-xs(12 px). Also:text-gray-600is a raw Tailwind palette value — the codebase has a semantic equivalent (text-ink-2). Switch totext-ink-2so dark mode remaps correctly; right now in dark mode the badge keepsgray-600text on dark--c-muted: #011a30, which is ~5:1 — passes AA but inconsistent with the rest of the system that uses tokens./documents/bulk-editis unusable below ~768 px (BulkDocumentEditLayout.svelte:333–357). The split panel uses fixedflex-[55] / flex-[45]with no responsive stack. At 320 px the PDF preview gets ~176 px wide and the form ~144 px — neither is operable. The PR inherits this from the upload flow, but it now matters more because we're funneling phone users into bulk-edit from the doc list (the "Massenbearbeitung" button shows on mobile). Either gate the bar entry on viewport ≥ md, or addflex-col md:flex-rowso the panels stack on narrow screens (PDF preview collapses to a small thumbnail header above the form).No screen-reader announcement when selection count changes — WCAG 4.1.3 (Status Messages). Toggling a row checkbox silently updates the bottom bar; AT users hear nothing. Wrap the count text in
aria-live="polite":Loading state on
/documents/bulk-editis a literal…glyph (bulk-edit/+page.svelte:46) — WCAG 1.1.1 (Non-text Content) and bad UX. Screen readers announce "horizontal ellipsis" or nothing. Replace with the project's standard loading pattern, an explicitaria-live="polite""Dokumente werden geladen…" message, or a spinner witharia-label.Error card on the same page uses raw red palette (
bulk-edit/+page.svelte:48) —border-red-300 bg-red-50 text-red-700skips the design system. The codebase hastext-danger; introduce abg-danger-bg/border-dangerpair if missing. Same applies to the partial-failure card inBulkDocumentEditLayout.svelte:480-484. Consistency aside, raw red palette tokens won't remap in dark mode and currently produce a too-light pink background that washes out the alert text.bulk_edit_n_selectedGerman plural is wrong for n=1 — "1 Dokumente ausgewählt" reads as broken German. Paraglide supports plural forms — split intobulk_edit_n_selected_one/bulk_edit_n_selected_otheror use the inline plural syntax. Same in EN ("1 documents selected") and ES ("1 documentos seleccionados")."Alles aufheben" is ambiguous in DE — could read as "cancel everything" (i.e. discard the bulk-edit operation entirely). The intended meaning is "clear the selection". Suggest
Auswahl aufhebenorAuswahl leeren— leaves no doubt about scope. EN "Clear all" has the same problem; "Clear selection" is more precise.Suggestions (nice to have)
bulk_edit_all_xbutton styling (/documents/+page.svelte:227) — the "Alle 1247 editieren" affordance is rendered as a plaintext-ink-2link with no border/icon. For a destructive-feeling action ("you're about to modify 1247 records") it deserves a visible boundary and a leading icon (e.g. checkmark-stack). Addborder border-line rounded px-3 py-2plus<svg>so it doesn't disappear into the filter bar's whitespace.Badge wording in EN feels terse — "+ added" / "replaced" reads as past tense ("this field has been added/replaced"), but the intent is future ("will be added/replaced when you save"). DE "+ wird hinzugefügt" / "wird ersetzt" is correct. Mirror that in EN:
+ will be added/will replace. ES is already correct ("se añade" / "se reemplaza").Discard-all-on-edit-mode —
handleDiscard()clears the file map, but in edit mode the map is the user's selection. Discarding throws them onto an empty layout with no PDF preview but the bulk bar still shows the original count from the store — confusing. Either clearbulkSelectionStorein tandem inside edit mode, or rename the button in edit mode to "Bearbeitung verwerfen" + navigate back to/documents.Field-label badge has no tooltip explaining what additive/replace means — the
bulk_edit_hintcallout at the top covers it once, but users who scroll past it lose context. Consider adding atitleattribute on the badge (title={variant === 'additive' ? 'Bestehende Werte bleiben erhalten — neue werden ergänzt' : 'Vorhandener Wert wird mit der Eingabe überschrieben'}) for hover hint, paired witharia-describedbyfor AT.Row-level checkbox lacks a visible focus indicator on the label wrapper — the native
<input type="checkbox">gets a browser default ring but the surrounding<label>(the actual 44×44 hit area) does not. On keyboard navigation the focus ring lands inside the checkbox's 20×20 box, which is hard to spot. Addfocus-within:ring-2 focus-within:ring-focus-ring focus-within:ring-offset-2on the<label>.What I checked
min-h-[44px]✓)BulkSelectionBar(pb-[max(0.75rem,env(safe-area-inset-bottom))]✓)text-gray-600onbg-mutedlight (7.4:1, AA ✓) and dark mode (~5:1, AA ✓)aria-labels,role="alert"/role="note", missingaria-livefor dynamic counttext-ink-2vs rawtext-gray-600, brand colors vs rawred-50palette)/documents/bulk-editOnce the four blockers land, this is a calm, on-brand bulk-edit flow that respects the senior audience and the additive/replace mental model. Nice work overall.
— Leonie
📋 Elicit — Requirements Engineer
Verdict: 🚫 Changes requested
The contract work is mostly there — endpoints, semantics, store model, callout, badges, partial-failure shape, and per-document
@Transactionalall match what we agreed in the discussion thread. But there is one production-breaking AC failure and one straight-up missed AC from the issue's "Bulk-Edit Panel" table that I have to flag before this can land.Blockers (must fix before merge)
B1 —
POST /api/documents/batch-metadatafield name mismatch silently breaks every PATCH save.The backend record
DocumentBatchSummary(backend/src/main/java/org/raddatz/familienarchiv/dto/DocumentBatchSummary.java:7-10) serializes its UUID asid. The generated TS type confirms it (frontend/src/lib/generated/api.ts:1761-1766→{ id, title, pdfUrl }). The bulk-edit page (frontend/src/routes/documents/bulk-edit/+page.svelte:31) does:But
BulkEditEntry(frontend/src/lib/components/document/BulkDocumentEditLayout.svelte:23-27) is{ documentId, title, pdfUrl }. The cast is a TypeScript lie — at runtime everyentry.documentIdisundefined. The hydration loop (BulkDocumentEditLayout.svelte:73-83) then writes every file intofiles.set(undefined, …), collapsing the entire selection into one Map entry under theundefinedkey. Save (saveBulkEdit, line 200-201) doesentries.map(e => e.documentId).filter((x): x is string => !!x)which yields[], and the controller's empty-ids guard (DocumentController.java:254-256) then returns a 400. The bulk PATCH cannot succeed with real backend data.The unit specs pass because they hand-roll
editEntryfixtures that already usedocumentId(BulkDocumentEditLayout.svelte.spec.ts:320). The Playwright E2E (frontend/e2e/bulk-edit.spec.ts) only asserts the callout renders — it never actually saves through the real backend, so the bug is invisible to CI.This breaks ACs:
Fix: either rename the DTO field to
documentId(and regenerate API types) or add an explicit mapping in+page.svelte(summaries.map(s => ({ documentId: s.id, title: s.title, pdfUrl: s.pdfUrl }))). Add an E2E assertion that actually executes the PATCH and verifies the tag landed on both documents — the manual smoke step in the PR description is the test that would have caught this.B2 — "Discard" in
mode="edit"does not navigate back to the list.The issue's Bulk-Edit Panel table (issue body, Bulk Edit (
mode="edit") column) defines:In the implementation,
handleDiscard(BulkDocumentEditLayout.svelte:124-134) callsdiscardAll()— which only clears the localSvelteMap, leaves the user stuck on/documents/bulk-editwith a greyed-out empty form, and leavesbulkSelectionStorepopulated. There is no branch onmodeand nogoto('/documents'). AC missed.Fix: in edit mode, after the confirm dialog, clear
bulkSelectionStoreandgoto('/documents')instead of (or in addition to) emptying the file map.Concerns (should fix before merge)
C1 — Count-pill copy is wrong in edit mode ("{count} werden erstellt").
BulkDocumentEditLayout.svelte:317rendersm.bulk_count_pill({ count: files.size })for both modes. The German string is "{count} werden erstellt" (en: "{count} will be created", es: "Se crearán {count}"). In edit mode nothing is being created — the documents already exist. This will read as a bug to the first user who touches the feature with 5+ documents selected.Fix: split the key (e.g.
bulk_count_pill_create/bulk_count_pill_edit) and switch onmode. Same applies to the topbar title (bulk_title_multireads "Mehrere Dokumente hochladen" — confirm and adapt for edit mode).C2 — Empty-store redirect is client-side only; the page flashes a "…" spinner first.
The AC "Navigating directly to /documents/bulk-edit with empty store redirects to document list" is satisfied by
+page.svelte:14-19, but the redirect runs inonMount. SSR renders the spinner first, the user sees a flash, then the redirect fires. Functionally it works; experientially it's noticeable. Consider doing the empty-store check in the server load (alongside the existing WRITE_ALL guard) — though I acknowledge the store is client-only, so a server check isn't trivial. At minimum: don't show the spinner when redirecting.C3 — Save button shows a generic CTA in edit mode ("N werden erstellt").
UploadSaveBar.svelterendersm.bulk_save_cta({ count: fileCount })(de: "{count} Dokumente speichern" — actually checking the file, the translation key is upload-flavored). For edit mode an "Anwenden" or "Änderungen anwenden" CTA would match the field-label badges and the callout's framing. Thebulk_edit_save_buttonkey already exists (de: "Anwenden") but isn't wired in.C4 — Audit/observability gap on the metadata-fetch and ids endpoints.
The bulk PATCH logs
bulkEdit actor=… documentIds=… updated=… errors=…— good, matches Tobias's recommendation./api/documents/batch-metadataand/api/documents/idslog nothing. For anAlle X editiereninvocation that pulls 1500 IDs into the client, having no audit trail is a small regression from the upload path's standard. Onelog.infoper call would close it.Suggestions (nice to have)
S1 — Out-of-Scope register is not updated in the issue body.
The discussion-follow-up explicitly added "Rückgängig per Dokumentversions-Rollback (die bestehende Versionierungsinfrastruktur unterstützt dies als Follow-up)" to Out of Scope. The PR does not edit the issue body — fine, but please add it before closing the issue, otherwise the requirements record loses traceability of what we deliberately deferred.
S2 —
Alle X editierenbutton has no permission gate at server-load time.The button only renders when
data.canWrite(+page.svelte:225), which matches the AC. But theGET /api/documents/idsendpoint is gatedREAD_ALL(DocumentController.java:285) so a READ_ALL user could theoretically trigger the fetch via devtools. They couldn't reach the bulk-edit page (it has its own WRITE_ALL guard), so the practical impact is zero — but for symmetry and least-privilege, consider gating/api/documents/idsbehindWRITE_ALLsince its only consumer is the bulk-edit fast path.S3 — Multi-chunk progress message is on the progress bar's
aria-labelonly.UploadSaveBar.svelte:27exposesbulk_upload_progressto assistive tech but renders no visual text. For a PATCH operation that may take 30+ seconds across multiple chunks, sighted users get only the unitless<progress>bar fill — no "Batch X von N" text. The Item 4 decision in the discussion explicitly called out "progress bar showing X / N Batches verarbeitet" as visible text. Renderbulk_edit_save_progress(which already exists in the messages file at line 885) next to or under the progress bar in edit mode.What I checked
WRITE_ALL(DocumentRow.svelte:60-73,enrich/+page.svelte:70-83)BulkSelectionBar.svelte:19-46)/documents/bulk-edit(BulkSelectionBar.svelte:10-12)mode="edit"withrole="note"(BulkDocumentEditLayout.svelte:370-376)WhoWhenSection.svelte:100,108,DescriptionSection.svelte:86,113,131,146)DocumentService.java:410-419) but PATCH never reaches it (B1)DocumentService.java:421-423) but blocked by B1DocumentService.java:425-429) but blocked by B1DocumentService.java:431-439) but blocked by B1BulkDocumentEditLayout.svelte:230-258)WRITE_ALL; returns partial-failure shape (DocumentController.java:249-282)canWritethread)POST /api/documents/batch-metadataexists and isREAD_ALL-gated@Transactional(DocumentService.java:405-406)SvelteSet, no "(aktuelle Seite)" hint, "Alle X editieren" present, "Alles aufheben" present (bulkSelection.svelte.ts,BulkSelectionBar.svelte:28-34,documents/+page.svelte:225-237)savingflag; chunk progress bar presentBULK_EDIT_TOO_MANY_IDSErrorCode mirrored across backend/frontend/i18nbulk_count_pilland save CTA reuse upload-flavored strings (C1, C3)🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
The unit + WebMvc layers are well-named, factory-driven, and largely Arrange-Act-Assert-clean. The new bulk-edit code paths get genuine behavioral coverage and not just snapshots, the partial-failure shape is asserted, and 401/403 is checked on
PATCH /bulk. That's the good news. What I want fixed is two specific blind spots — duplicate-id semantics andREAD_ALLenforcement onbatchMetadata— plus the absence of any integration coverage that exercises the real@Transactionalboundary the bulk-edit design depends on. None of those are speculative; each maps to an exact line ofDocumentService.java/DocumentController.javathat no test currently fences in.Blockers (must fix before merge)
batchMetadatahas no403test for a user withoutREAD_ALL.DocumentControllerTest:1062-1085covers401and400 ids empty, but never asserts that an authenticated user lackingREAD_ALLis rejected.DocumentController.java:301carries@RequirePermission(Permission.READ_ALL)— without an explicit test, a future refactor that drops the annotation (or widens it topermitAll) merges silently. Add:Same omission for
getDocumentIds—DocumentControllerTest:1021-1037covers401but not403for an authenticated user withoutREAD_ALL.DocumentController.java:285is annotatedREAD_ALL; the negative path is untested.No coverage for duplicate
documentIdsin the PATCH payload.DocumentController.java:266iteratesdto.getDocumentIds()raw — it does not deduplicate. If the frontend sends[id, id](e.g. user double-clicks "Alle X editieren"), the service is called twice for the same document. Tags-additive logic is idempotent, but theupdatedcount inBulkEditResultwill be2while only one logical document changed, and the audit/log line atDocumentController.java:278will report inflated numbers. Add:Pin the contract — either dedupe in the controller, or assert the inflated-count behavior is intentional. Right now neither side is documented.
No integration test exercises the
@Transactionalboundary onapplyBulkEditToDocument. The Javadoc atDocumentService.java:402claims "Wrapped in its own transaction so a failure on one document never partially mutates another." That is precisely the kind of cross-test claim Mockito unit tests cannot verify — the@Transactionalannotation is a Spring proxy, and a self-call from inside the same bean would silently bypass it.DocumentControllerTestmocks the service entirely, so the proxy boundary is never hit. We need one@SpringBootTest(or@DataJpaTest+ manual wiring) with Testcontainers PostgreSQL that:PATCH /bulk, where document #2 has an unresolvablesenderId.This is the load-bearing claim of the entire feature; it deserves one real-DB test.
Concerns (should fix before merge)
Unresolvable
senderId/receiverIdsis not unit-tested at the service layer.DocumentServiceTest.java:1994-2012happy-paths a known sender. Nothing coverspersonService.getById(senderId)throwingDomainException.notFound.DocumentService.java:421-422propagates that exception unchanged into the controller's catch block, where it becomes a per-documentBulkEditError. That's the desired behavior — assert it. Same forpersonService.getAllByIdat line 427 returning fewer persons than requested IDs (silent partial resolve — what does merged set look like?).patchBulk_returns400_whenDocumentIdsExceedsCapconstructs 501 IDs and assertsBULK_EDIT_TOO_MANY_IDS— but never tests the boundary at exactly 500. That's the off-by-one zone.DocumentController.java:257reads> BULK_EDIT_MAX_IDS; a future change to>=would go undetected. Add apatchBulk_returns200_atExactly500Idscompanion test. Same pattern asvalidateBatch_doesNotThrow_whenFileCountEqualsCapExactlyalready in the suite — be consistent.findIdsForFilterparameter coverage is paper-thin.DocumentServiceTest.java:2092-2116covers two cases: all-nulls and FTS-empty. The method takes nine parameters and the controller wires nine query params (q,from,to,senderId,receiverId,tag,tagQ,status,tagOp).DocumentControllerTest.java:1041-1050only verifies thatsenderIdis forwarded. Coverage oftagOp=ORvs defaultAND(which flipsuseOrLogicat line 367) is missing. A regression that wiredtagOp=ORto AND logic (or vice versa) would not be caught.bulkSelection.svelte.spec.tshas zero coverage of theidsgetter andsetAllwith an empty iterable. The store exposesbulkSelectionStore.ids(used by+page.svelte:15); spec never reads it.setAll([])is the natural "load filter result that returned zero matches" path — also untested.BulkDocumentEditLayout.svelte.spec.ts:415-437("chunks 1100 IDs into 500-sized requests") asserts the chunk sizes but never asserts that all 1100 IDs round-trip without loss. Sum of chunk lengths should equal 1100, andSetof all sent IDs should equal the original. Also, the test stops at 3 calls — what if the frontend silently dropped the trailing partial chunk? Verify the equality of the union, not just the chunk count.BulkSelectionBar.svelte.spec.tshas no test for the count text format withcount=1vscount>=2. The Paraglide messagebulk_edit_n_selected({ count })likely has plural forms; only thecount=2happy path is exercised. Pluralization regressions are silent — at least snapshot both branches.page.server.spec.tsredirects on missinggroupsand missinguser— but does not test the case wherelocals.user.groups[0].permissionsisundefined.+page.server.ts:5uses?? falseon the outer.some(...), but if a group has nopermissionsarray the inner.includes('WRITE_ALL')will throw. A defensive test would lock down the contract.bulk-edit.spec.tsE2E is golden-path only and depends on "at least two documents exist." No fixture seeding, notest.beforeAllthat ensures the precondition. On a fresh DB the suite silently fails with cryptic timeout errors instead of "fixtures missing." Either seed in the spec, gate withtest.skip(documentCount < 2, ...)like the/enrichtest already does, or move the assertion out of "Assumptions:" comments and into runtime checks.No E2E for the "Alle X editieren" fast path. The whole point of
GET /api/documents/idsis the "select-all-across-pages" flow (issue #225 acceptance criterion). Spec covers the two-checkbox manual case; never exercises the fast-path button.No E2E asserting that
bulkSelectionStoreis cleared after a successful save.BulkDocumentEditLayout.svelte:263callsbulkSelectionStore.clear()on success — a regression that left this out would result in a stale selection bar appearing on/documentsafter navigation. Worth one E2E assertion on round-trip.Suggestions (nice to have)
patchBulk_returnsPartialFailureShape_whenServiceThrowsForOneDocumentis two assertions in one test (updated=1anderrors[0]shape). Per the Sara rule "one logical assertion per test, one reason to fail," split intopatchBulk_returnsErrorEntryForFailedDocumentandpatchBulk_incrementsUpdatedCountForSuccessfulDocument.Add an axe scan for
/documents/bulk-editinaccessibility.spec.ts. This is a brand-new page with arole=notecallout, sticky bars, and a focus-managed checkbox column — high a11y-regression risk. The persona guide explicitly calls outaxe-playwrightas a quality gate; it is not currently extended to this route.DocumentRow.svelte.spec.ts:296-298clicks via rawdocument.querySelectorbecause of a Tailwind/z-index issue in the test client. That comment is honest but the workaround leaks into multiple tests. Consider extracting to aclickFirstCheckbox(container)helper to localize the workaround in one place — when Tailwind is loaded into the browser project later, you delete one helper, not five tests.bulkSelectionStore.setAlltest only checks size + presence of new IDs. Add an explicit assertion that previous IDs are absent (currently inferred fromhas('a')returning false but only for one of the two pre-existing entries).FieldLabelBadge.svelte.spec.ts:24-29asserts a Tailwind class string. This is implementation-coupled — if the design system swaps totext-ink-3the test fails on a non-behavioral change. AgetComputedStylecolor-contrast assertion (or an axe scan in the parent component) would be more meaningful.BulkDocumentEditLayout.svelte.spec.ts:81-99("save calls fetch twice for 12 files") asserts only the call count. Doesn't assert that the second chunk's metadata Blob carries the samesenderId/tagNamesas the first. A regression that reset metadata between chunks would pass.No test for the "save in flight + user clicks Discard" race.
saving=trueblocks save, buthandleDiscardwill happily wipe the file map mid-upload. Either guard the discard or test that it's intentionally allowed.What I checked
backend/src/test/java/.../service/DocumentServiceTest.java— applyBulkEditToDocument, batchMetadata, findIdsForFilter blocks (lines 1921–2191): tag/sender/receiver additive vs replace, blank-string handling, location field replace. Probed for: unresolvable sender/receiver IDs, missing transactional integration tests, partial getAllById results.backend/src/test/java/.../controller/DocumentControllerTest.java— patchBulk, getDocumentIds, batchMetadata blocks (lines 933–1107):@WebMvcTestslice; 401/403 on patchBulk; partial-failure body shape. Probed for: missing 403 on READ_ALL endpoints, off-by-one at the 500-cap boundary, duplicate-id deduplication, parameter forwarding completeness.frontend/src/lib/stores/bulkSelection.svelte.spec.ts— full file. Probed for:idsgetter access,setAllwith empty iterable, idempotency edges.frontend/src/lib/components/document/BulkSelectionBar.svelte.spec.ts— full file. Probed for: pluralization branches, focus management on toggle.frontend/src/lib/components/document/BulkDocumentEditLayout.svelte.spec.ts— both upload-mode and edit-modedescribeblocks (full file, 501 lines). Probed for: round-trip ID preservation across chunks, metadata stability between chunks, save-during-discard race.frontend/src/lib/components/document/FieldLabelBadge.svelte.spec.ts— full file. Flagged: Tailwind-class assertion is implementation-coupled.frontend/src/lib/components/DocumentRow.svelte.spec.ts— bulk-selection checkbox describe block (lines 270–307). Probed for: aria-label correctness, store synchronization, querySelector workaround scope.frontend/src/routes/documents/bulk-edit/page.server.spec.ts— full file. Probed for: undefined permissions array branch, missing-locals defensiveness.frontend/e2e/bulk-edit.spec.ts— full file. Probed for: fixture preconditions, fast-path coverage, store-clear-on-success E2E, axe scan presence.DocumentService.java:355–442,DocumentController.java:245–307,BulkDocumentEditLayout.svelte:1–286.— Sara
B1 — i18n the archive-box / archive-folder labels and add helper text. Karton/Mappe were hardcoded German and broke EN/ES locales (WCAG 3.1.2). B2 — drop the hardcoded German aria-label on the onboarding callout. role="note" + the visible localised text is self-describing; the redundant label was overriding the translated content for AT users on EN/ES. B3 — Escape clears the bulk selection while the bar is visible. Adds an "Esc: Auswahl aufheben" hint visible at ≥ sm (WCAG 2.1.1). B4 — /documents and /enrich reserve pb-32 when the bulk-selection bar is visible so it doesn't occlude the last row or pagination (WCAG 1.4.10). Folded in three Leonie quick-concerns: - C5: badge text-[10px] → text-[11px], raw text-gray-600 → design-token text-ink-2 (dark-mode safe) - C7: aria-live="polite" on bulk-selection-count - C11: "Alles aufheben" → "Auswahl aufheben" (DE/EN/ES) — disambiguates from "discard the operation entirely" Refs #225, PR #331 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Elicit C1+C3 — bulk-selection count uses ICU-style plural keys (bulk_edit_n_selected_one / _other) so n=1 reads as "1 Dokument" instead of "1 Dokumente". Save CTA in edit mode reads "Anwenden" via the existing bulk_edit_save_button key; UploadSaveBar grew an editMode prop. Multi- chunk progress text is now visible (not aria-only). Felix C2 — bulk-edit page wires the backend error code through parseBackendError + getErrorMessage instead of falling back to a generic internal_error. Felix C5 — editAllMatching no longer swallows fetch failures: the button shows an inline error with the backend-mapped message (e.g. when the filter cap is exceeded). Leonie C8 — replace the literal "…" loading glyph on /documents/bulk-edit with a spinner + role=status + aria-live=polite + visible "Loading documents…" text. Leonie C9 — partial-failure card and bulk-edit page error card now use the design-system `text-danger` / `bg-danger/10` / `border-danger/40` tokens (dark-mode safe) instead of raw red palette values. Leonie C10 + C13 — German plural fixed; EN badges retensed ("+ added" → "+ will be added", "replaced" → "will replace") to match the future-tense intent of DE/ES. Refs #225, PR #331 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>✅ Cycle 1 fixes pushed
10 atomic commits (
f13f6351..1803db86). Backend: 233 tests green. Frontend bulk-edit specs all green. Working through every blocker + concern from the seven persona reviews.Production bug
id, frontend expecteddocumentId→ bulk PATCH was firing with empty list. Fixed by aligningBulkEditEntry.idwith the JSON shape (commit2bb8fb89).Blockers — backend
applyBulkEditToDocumentnow takesactorId, callsdocumentVersionService.recordVersion(saved), and emitsAuditKind.METADATA_UPDATEDtaggedsource=BULK_EDIT. Audit/version trail parity restored.POST /api/documents/batch-metadatacapped at 500 IDs (matches PATCH cap);GET /api/documents/idscapped at 5000 results with typedBULK_EDIT_TOO_MANY_IDSon overflow.batchMetadata_returns403_forUserWithoutReadAllandgetDocumentIds_returns403_forUserWithoutWriteAll(the latter after re-gating /ids).LinkedHashSetso a double-clicked "Alle X" cannot inflate theupdatedcount.Blockers — frontend
initial*props onWhoWhenSectionandDescriptionSection;dateIso/currentTitlenow seed from the bindable directly insideuntrack. No more top-level$bindablemutation.resolveTags(single source of truth for "name string → Tag find-or-create") +buildSearchSpec(single source of truth for the seven-spec chain). Used by bothupdateDocumentTags(replace) andapplyBulkEditToDocument(additive merge); bothsearchDocumentsandfindIdsForFilter.bulkSelectionStoreandgoto('/documents')(was leaving the user stuck on an empty form).Karton/Mappenow flow through Paraglide asform_label_archive_box/form_label_archive_folderwith helper text in DE/EN/ES.aria-labelon the onboarding callout;role="note"+ the visible localised text is self-describing.Escclears the bulk selection while the bar is visible; visible "Esc: Auswahl aufheben" hint at ≥ sm./documentsand/enrichreservepb-32when the bar is visible (no more occluded last row / pagination).Concerns
editAllMatchingnow route throughparseBackendError+getErrorMessage(code)instead of falling back toerror_internal_error.+layout.svelteauto-clearsbulkSelectionStorewhenever the user navigates outside/documentsand/enrich. No more invisible stale selection.editAllMatchingsurfaces fetch failures inline instead of silently re-enabling the button.DocumentBulkEditDTOnow carries@Sizeguards ontagNames(200×200),receiverIds(200), and the three location strings (255 each);@RequestBody @Validwired in the controller.Throwable#getMessage()(CWE-117).findIdsForFilterandsearchDocumentsnow share the newbuildSearchSpechelper.DocumentServicewith imports.@Transactional(readOnly = true)onfindIdsForFilterandbatchMetadata.n=1("1 Dokument ausgewählt" vs "{count} Dokumente ausgewählt") in DE/EN/ES via Paraglide_one/_otherkeys.bulk_edit_save_buttonkey; multi-chunk progress text is now visible (not aria-only)./api/documents/batch-metadataand/api/documents/idslog one INFO line per call./api/documents/idstoWRITE_ALL(its only consumer is the bulk-edit fast path).text-[10px]→text-[11px]; rawtext-gray-600→ design-tokentext-ink-2(dark-mode safe).aria-live="polite"on the bulk-selection-count region.…loading glyph on/documents/bulk-editwith a spinner +role="status"+ visible "Loading documents…" text.text-danger/bg-danger/10design tokens (dark-mode safe).+ added→+ will be added,replaced→will replace).Alles aufheben→Auswahl aufheben/Clear selection/Limpiar selección.applyBulkEditToDocument_propagatesDomainException_whenSenderIdUnresolvabletest added.patchBulk_acceptsExactly500Ids_atTheCapboundary test added.findIdsForFilter_passesTagOperatorOR_throughBuildSearchSpecservice-level test added (controller-level OR test deferred — see #332).setAll([])no-op + previous-IDs-absent +idsgetter tests added tobulkSelection.svelte.spec.ts.+page.server.tsdefensively handles aUserGroupwith NULLpermissions+ matching test.Deferred to follow-up #332
The eight largest items — split
BulkDocumentEditLayoutinto upload + edit components, server-side load for the bulk-edit page, Testcontainers integration test for the@Transactionalboundary, ADR for additive-vs-replace semantics, DTO renames (Batch*→BulkEdit*), DTO record conversion, axe-playwright scan for/documents/bulk-edit, and the per-request entity resolution optimisation — are tracked in #332. Each is its own small PR. Rationale: keeping this PR focused on the concrete blockers + concerns; the deferrals are real but bounded.Ready for cycle 2 review.
🛠️ Tobias Wendt — DevOps & Platform Engineer — Cycle 2
Verdict: ✅ Approved
Cycle-1 plugged both concerns I flagged, the deferrals went into a real follow-up issue (#332) instead of evaporating, and nothing in the cycle-1 deltas opens a new operational hole. Rollback is still pure
git revert— no migrations, no infra, no env vars. Ship it.Resolved (cycle-1 fixes verified)
Audit + version trail now parity with single-doc updates ✅
DocumentService.applyBulkEditToDocument(backend/src/main/java/.../service/DocumentService.java:471-475) now callsdocumentVersionService.recordVersion(saved)andauditService.logAfterCommit(AuditKind.METADATA_UPDATED, actorId, saved.getId(), Map.of("source", "BULK_EDIT")). Thesource=BULK_EDITtag is exactly what I wanted — bulk edits are still distinguishable from single-doc edits in the audit table for later analytics. Audit writes go off-thread onauditExecutorafter commit (AuditService.java:29-43), so they don't extend the per-doc transaction. Clean.Bean-validation on
DocumentBulkEditDTO✅backend/src/main/java/.../dto/DocumentBulkEditDTO.java:33-60—@Size(max=200)ontagNames(with per-element@Size(max=200)) andreceiverIds,@Size(max=255)on the three location strings. The Javadoc on the class even explains the payload-amplification math against the SvelteKit 1 MiB proxy cap, and the inline comment ondocumentIdscorrectly justifies not applying@Sizethere (it would short-circuit the typedBULK_EDIT_TOO_MANY_IDSerror code with a genericVALIDATION_ERROR). Good engineering judgment.CRLF log-injection sanitiser on free-form strings ✅
DocumentController.java:282-298—sanitizeForLog()strips\r\nfrome.getMessage()before it entersBulkEditErrormessages andlog.warn(...). Defends Loki/Grafana log views from CWE-117 forged log lines. Wasn't on my cycle-1 list explicitly — Nora flagged this — but worth calling out as an op-side win.Caps on
/idsand/batch-metadata✅BULK_EDIT_FILTER_MAX_IDS = 5000on/ids(DocumentController.java:253, 316-319),BULK_EDIT_MAX_IDS = 500on/batch-metadata(:331-334). Both return the typedBULK_EDIT_TOO_MANY_IDSerror code. The 5000 ceiling on/idsis sized for the family archive's actual scale (~1500 docs today, ~5k projected) and the comment says so — that's exactly the kind of explicit operational sizing I want to see in code, not buried in a wiki.Dedupe on duplicate document IDs in
/bulk✅LinkedHashSetatDocumentController.java:275and the log line now reports bothdocumentIds=andunique=(:289-290) — a double-click won't inflate theupdatedcount or double-write audit rows. Nice touch.Concerns
(none)
Suggestions / follow-ups (already tracked in #332)
recordVersion()is synchronous inside the per-doc transactionDocumentVersionService.recordVersion(service/DocumentVersionService.java:39-57) doesfindByDocumentIdOrderBySavedAtAsc(doc.getId())(full version-history read for change-field diffing) + JSON-serialise the Document + INSERT, all inside the per-doc TX. For a 500-doc batch the per-doc TX cost goes from "1 SELECT + 1 UPDATE + N tag/person resolves" to "+ 1 SELECT (history) + 1 INSERT (version)". On the CX32 + PgBouncer this is bounded but measurable — worth profiling once we have Grafana wired up. Not a blocker, not a regression vs single-doc edit (which has the same shape), and #332's "pre-resolve tag set + sender + receivers once per request" item will reduce the dominant N+1 cost anyway.RateLimitInterceptorstill does not coverPATCH /api/documents/bulkConfirmed by re-reading
config/WebConfig.java:11-14— interceptor is registered for/api/auth/invite/**and/api/auth/registeronly. Tracked as Tobias #5 in #332. A single misbehaving authenticated client looping 500-doc bulk-edits can pin a backend thread for several seconds per call. Mitigations available (per-user token bucket onWRITE_ALLendpoints, or just extending the existing path patterns). Follow-up, still not a blocker for this PR.What I checked (cycle 2)
backend/src/main/java/.../controller/DocumentController.java— audit log on/bulk,/ids,/batch-metadata;sanitizeForLog; both caps in place; dedupe via LinkedHashSet; permission gates correct (WRITE_ALLon/bulkand/ids,READ_ALLon/batch-metadata)backend/src/main/java/.../service/DocumentService.java:440-476—recordVersion+logAfterCommitcalls land at the correct point (afterdocumentRepository.save, before return); transactional scope is per-doc as designed; auditsource=BULK_EDITpayload presentbackend/src/main/java/.../dto/DocumentBulkEditDTO.java— bean-validation caps wired with sensible explanatory Javadoc;@Validon the controller signature (DocumentController.java:258) actually enforces thembackend/src/main/java/.../audit/AuditService.java—logAfterCommitregisters anafterCommitsynchronization that dispatches toauditExecutor, so audit writes don't extend the per-doc TX; failures are caught and logged without escalatingbackend/src/main/java/.../config/RateLimitInterceptor.java+WebConfig.java— confirmed scope unchanged from cycle 1; tracked as #332 itemgit revert, still no migrations, still no schema deltasINFOline per batch + 500 audit rows + 500 version rows for a max-size batch. Audit + version rows are real DB writes (intended); controller log is bounded. No log-bomb risk.🏗️ Markus Keller — Senior Application Architect — Cycle 2
Verdict: ✅ Approved
Both cycle-1 blockers are properly fixed at the architectural level. Audit/version parity is restored on the right side of the per-doc transaction boundary (
logAfterCommitregistered insideapplyBulkEditToDocument's transaction → fires on each successful commit via theauditExecutor, so partial-success batches produce exactly the audit/version rows the user actually mutated). Both unbounded endpoints now have explicit caps with typed error codes. The duplication betweenfindIdsForFilterandsearchDocumentsis gone — onebuildSearchSpecis the single source of truth, used from both call sites. The four deferred items (#4 DTO rename, #6 record conversion, #9 ADR, #10 layout split) are tracked in #332 with persona attributions intact, so the audit trail is preserved.No new architectural regressions introduced by the cycle-1 changes themselves. The per-doc N+1 on tag/person resolution is now explicitly called out in the Javadoc (
DocumentService.java:435-438) and tracked in #332 — that is the right way to ship a known-bounded perf trade-off.Blockers (must fix before merge)
(none)
Concerns (should fix before merge)
(none)
Resolved from cycle 1
applyBulkEditToDocumentnow takesactorId, callsdocumentVersionService.recordVersion(saved), emitsAuditKind.METADATA_UPDATEDwithMap.of("source", "BULK_EDIT")(DocumentService.java:471-474). Transaction model is correct:recordVersionruns in the same per-doc tx (Spring REQUIRED propagation), audit fires after commit on the audit executor — so a failed doc neither writes a version row nor logs an audit, and a successful doc gets both atomically./idsand/batch-metadataunbounded → Fixed.BULK_EDIT_FILTER_MAX_IDS = 5000cap on/idsreturningBULK_EDIT_TOO_MANY_IDS(DocumentController.java:316-319);BULK_EDIT_MAX_IDS = 500cap on/batch-metadata(DocumentController.java:331-334). Caps are documented inline with the family-archive scale rationale.buildSearchSpecextracted (DocumentService.java:389-404), called from bothsearchDocuments:541andfindIdsForFilter:378. Includes the FTS short-circuit signature (hasText,ftsIds) so callers stay responsible for their own empty-result early-return.Batch*vsBulkEdit*DTO naming collision → Deferred to #332 with my attribution. Acceptable as a pure rename in a follow-up.applyBulkEditToDocument(DocumentService.java:435-438) now warns about the N+1 fan-out and the family-archive scale assumption. The controller-loop side (the "500 transactions on one HTTP thread" warning) is still implicit. Acceptable — the perf follow-up in #332 covers the same risk surface.DocumentBulkEditDTOis@Data, others are records → Deferred to #332. The post-cycle-1 Javadoc on the DTO (DocumentBulkEditDTO.java:20-26) documents the deferral and its rationale (test-side mutation churn), which is exactly the kind of context-preserving comment I want on a deliberate non-conversion.DocumentService→ Fixed.DocumentBatchSummaryandDocumentBulkEditDTOnow imported at the top (DocumentService.java:11-12).@Transactional(readOnly=true)on read paths → Fixed. Applied to bothfindIdsForFilter(:368) andbatchMetadata(:412).BulkDocumentEditLayout.svelteis two state machines in one file → Deferred to #332 (Felix B3 owns it). Layout is now ~528 lines; extraction proposal (BulkUploadLayout+BulkEditLayout+ sharedBulkPdfPreview/chunkAndSave) is recorded.What I checked this cycle
updateDocument:319-327and bulkapplyBulkEditToDocument:471-474— both emitMETADATA_UPDATEDafter commit, both write a version row inside the tx; bulk-edit additionally tagssource=BULK_EDITso the audit log can distinguish the two pathsrecordVersion(@Transactional) is invoked insideapplyBulkEditToDocument(@Transactional) → joins the same tx, all writes commit atomically with the document save (correct)logAfterCommitsemantics inside the per-doc tx:TransactionSynchronizationManager.registerSynchronizationregisters per-tx, fires on the per-doc commit, dispatches the write to theauditExecutorthread — so the request thread never blocks on audit I/OBULK_EDIT_MAX_IDS = 500matches frontendBulkDocumentEditLayout.svelte:219 chunkSize = 500; backendBULK_EDIT_FILTER_MAX_IDS = 5000is documented for the family-archive scale assumptionbuildSearchSpecis genuinely shared between paged search and ID-only fast path; no copy-paste left on the spec chainLinkedHashSetdedupe on PATCH controller (DocumentController.java:275) preserves submission order while preventing double-click inflation of theupdatedcount — correct semantics@Sizecaps onDocumentBulkEditDTO(200 tags, 200 receivers, 255 chars on the three location strings) — defends the JSON-binding layer; the documented rationale for not cappingdocumentIdsat the bean-validation layer (so the typedBULK_EDIT_TOO_MANY_IDSreaches the frontend) is sound/documents/bulk-edit/+page.server.ts) defensively handles aUserGroupwith NULLpermissions— matches the backend@RequirePermission(WRITE_ALL)and survives malformed user records+layout.svelte$effectclearingbulkSelectionStoreoutside/documentsand/enrich— clean cross-cutting concern in the right placeShip it.
🔐 Nora "NullX" Steiner — Application Security Engineer — Cycle 2
Verdict: ✅ Approved
Cycle-1 hardening is comprehensive. I went back through every concern from comment 4490 with a fresh adversarial eye and could not find a path to escalate any of them, nor did the new code introduce a fresh attack surface. The bulk surface is now my favourite-looking endpoint cluster in the controller — explicit caps, explicit audit, explicit log-injection defence, and an actor identity that is structurally unforgeable. Shipping it.
Blockers
(none)
Concerns
(none) — all five from cycle 1 are resolved or correctly deferred.
Resolved (cycle 1)
C1 — Per-request cap on
POST /batch-metadata(CWE-770) — ✅ FIXEDDocumentController.java:331-334now mirrors the sameBULK_EDIT_MAX_IDS = 500guard asPATCH /bulk, returningBULK_EDIT_TOO_MANY_IDS. The proxy → 26k-UUID amplification I worried about can no longer reachfindAllById. Symmetric error code → frontend already maps it to a localised message.C2 —
GET /api/documents/idsupper bound (CWE-770) — ✅ FIXED with the right trade-offDocumentController.java:248-253introducesBULK_EDIT_FILTER_MAX_IDS = 5000with a Javadoc comment that explicitly states the threat model ("prevents an unfiltered call from materialising the entiredocumentstable into JSON") and the operational reality ("~1500 docs today, expected growth to ~5k"). This is exactly the "explain the threat model in the comment" pattern I push for — a reviewer 6 months from now knows why the magic number is what it is. Server-side enforcement at line 316-319 with the same typedBULK_EDIT_TOO_MANY_IDScode. Note: thedocumentRepository.findAll(spec)materialisation cost from the original C2 still exists inside the cap, but at 5k rows it's bounded and not a security issue — that's a perf follow-up.C3 — Typed BulkEditError code (CWE-79 defence-in-depth) — ⏸️ DEFERRED to #332, ACCEPTED
Verified the actual XSS sink is absent today:
bulk-edit/+page.svelte:30-34only readsbackend?.code(typed enum → Paraglide), andBulkDocumentEditLayout.svelte:258-263readserr.idfrom the body and discardserr.message. Svelte's{}interpolation would auto-escape it anyway. No reachable sink. Tracking the typed-code refactor in #332 keeps the defence-in-depth story honest without blocking this PR.C4 — Log injection in new log lines (CWE-117) — ✅ FIXED
DocumentController.java:295-299introducessanitizeForLog()with a Javadoc comment that names the threat (CWE-117) — exactly the right shape. Applied to bothe.getMessage()interpolations at line 282 and 285. The other newlog.info("bulkEdit actor={} ...")andlog.info("documentIds actor={} ...")lines I re-audited: every interpolated value is either aUUID(typed, can't carry CRLF), anint(from.size()), or aboolean— structurally safe without sanitisation. VerifiedactorIdflows fromSecurityUtils.requireUserId()which returnsuser.getId()— a DB-typedUUID, not user input.C5 — Audit log for bulk edits — ✅ FIXED, exactly as specified
DocumentService.java:473-474now emitsauditService.logAfterCommit(AuditKind.METADATA_UPDATED, actorId, saved.getId(), Map.of("source", "BULK_EDIT"))per document, plusdocumentVersionService.recordVersion(saved)at line 472. Thesource=BULK_EDITdiscriminator means admins can filter the audit feed for bulk-edit damage versus single-doc edits during incident response. Audit completeness across single-doc and bulk paths is now symmetric — the post-hoc accountability gap I flagged is closed.S2 —
@Sizeguards on DTO — ✅ FIXEDDocumentBulkEditDTO.java:44-59has@Size(max=200)ontagNames(entries + per-element char length),@Size(max=200)onreceiverIds, and@Size(max=255)on the three location fields. Plus@Validon the controller's@RequestBodyat line 258 wires it up. The deliberate omission of@SizeondocumentIdsis documented in the DTO Javadoc — "would short-circuit the typedBULK_EDIT_TOO_MANY_IDSerror code with a genericVALIDATION_ERROR" — that's a sound trade-off for UX, and the controller-level cap still applies.What I checked (cycle 2)
sanitizeForLog()covers every CRLF-bearing interpolation in the new code (DocumentController.java:282, 285); other new log lines interpolate only typed UUIDs/ints/booleansactorIdprovenance:SecurityUtils.requireUserId()→userService.findByEmail(authentication.getName())→user.getId()— DB-typed UUID, can't be log-injectedLinkedHashSet<UUID>dedupe at line 275: no IDOR introduction — the set still operates on the same caller-supplied IDs that were going through the loop before, just without double-counting; iteration order preservedapplyBulkEditToDocumentrebuilt audit + version flow: per-doc@Transactional,recordVersion+logAfterCommithappen inside the same tx, so a partial bulk failure won't leave version-without-audit or vice versaBulkEditError.messagetraversal end-to-end: backendString message→ frontendbody.errors[].messageis read into the typed shape but never rendered to the DOM (verified inBulkDocumentEditLayout.svelte:258-263— onlyerr.idis consumed). No XSS sink, with or without the C3 refactor.DocumentBulkEditDTO@Validwiring at controller boundary: confirmed the@RequestBody @Validannotation atDocumentController.java:258triggers bean validation;@Validatedon the controller class enables method-level constraint validationparseBackendErrorreturns a typed{code, message}shape, butbulk-edit/+page.svelte:32only consumesbackend?.code→ Paraglide. Backendmessagenever reaches the user-visible stringBULK_EDIT_TOO_MANY_IDSmapping inerrors.ts:146-147covers the new caps consistently — single error code for all three caps means future bulk endpoints can reuse the same translation keySuggestions (carry-over from cycle 1, no blocker)
List<...>in bulk endpoints) still worth doing as a separate hardening PR — the controller now has three explicit caps to copy from when the next bulk endpoint landsBulkEditError.messageround-trip is a latent sink waiting for a future "show me what failed" UX change. The current behaviour (only flip status toerror) is safe; if/when someone wants to render the per-document error message, do C3 first (typed code → Paraglide) before any{err.message}interpolation. Worth a comment inBulkDocumentEditLayout.sveltenoting "do not rendererr.messagedirectly — it is a free-form backend string; map via Paraglide instead". Optional.🤖 Reviewed in character as Nora "NullX" Steiner.
📋 Elicit — Requirements Engineer — Cycle 2
Verdict: ⚠️ Approved with concerns
B1 and B2 — both blockers — are correctly resolved. The production-breaking field-name mismatch is fixed at the source (
BulkEditEntry.idnow mirrors the backend JSON shape one-to-one, hydration usesentry.idfor both the SvelteMap key and the innerdocumentId, and the unit fixture inBulkDocumentEditLayout.svelte.spec.ts:343-347now matches the production shape so a future regression of B1 would fail CI). Discard in edit mode now branches onmode === 'edit', clearsbulkSelectionStore, andgoto('/documents')— the one missing AC from the issue's Bulk-Edit Panel table is restored.One concern from cycle 1 (C1) was partially addressed but the original issue is still live. Everything else from cycle 1 lands.
Resolved (cycle 1 → cycle 2)
frontend/src/lib/components/document/BulkDocumentEditLayout.svelte:28-32(type now{ id, title, pdfUrl }with explanatory comment) andfrontend/src/routes/documents/bulk-edit/+page.svelte:36-37(cast now matches reality). The hydration loop atBulkDocumentEditLayout.svelte:78-87usesentry.idfor both the map key and the innerdocumentId, sosaveBulkEdit'sentries.map(e => e.documentId).filter(...)now yields the actual UUID list. Tag-additive, sender-replace, receivers-additive, and location-replace ACs all fire end-to-end now.BulkDocumentEditLayout.svelte:128-148. Themode === 'edit'branch clears the upstream selection store and routes back to/documents, with an in-code comment citing the issue's Bulk-Edit Panel table as the requirement source.UploadSaveBar.svelte:20-23derivessaveCtafromeditMode; rendersbulk_edit_save_button("Anwenden" / "Apply") in edit mode. Wired from the layout at line 524.DocumentController.java:321(documentIds actor=… matched=…) andDocumentController.java:336(batchMetadata actor=… ids=…) both emit onelog.infoper call. Audit trail parity restored./api/documents/idspermission gate: ✅ Re-gated toWRITE_ALLatDocumentController.java:302. Matches least-privilege since its only consumer is the bulk-edit fast path.UploadSaveBar.svelte:39-49renders thebulk_edit_save_progressstring visibly (not aria-only) wheneditMode && chunkProgress.total > 1, witharia-live="polite"and a stabledata-testidfor E2E. Sighted users now see "Batch X von N verarbeitet" during multi-chunk PATCH.Concerns (carried over from cycle 1, partially addressed)
C1 — Topbar copy in edit mode still reads "Neue Dokumente / {count} werden erstellt" (NOT fixed).
The cycle-1 resolution comment lists "Elicit C1 — Fixed German plural for n=1" — but that fix was applied to
BulkSelectionBar.svelte:43(the document-list bottom bar), not to the topbar count pill on the bulk-edit page that my C1 actually flagged. The topbar atBulkDocumentEditLayout.svelte:326-333still renders unconditionally:bulk_title_multiresolves to "Neue Dokumente" / "New Documents" / "Nuevos documentos" andbulk_count_pillto "{count} werden erstellt" / "{count} will be created" / "Se crearán {count}". A user who selected 12 documents from/documentsand clicked Massenbearbeitung now lands on a page whose own header tells them they are creating new documents. That contradicts the inline callout (bulk_edit_hint) directly below it, which correctly frames the operation as editing. This is the same regression risk I described in cycle 1 — the persona's first read of the page claims the wrong operation.Suggested fix: add
bulk_edit_title_multi("N Dokumente bearbeiten" / "Edit N documents" / "Editar N documentos") andbulk_edit_count_pill("{count} werden aktualisiert" / "{count} will be updated" / "Se actualizarán {count}") and switch onmode === 'edit'. Same pattern as the save CTA already uses atUploadSaveBar.svelte:20-23.C2 — Empty-store redirect still flashes loading state (deferred, acceptable for now).
bulk-edit/+page.svelte:15-23now setsloading = falsebefore thegoto('/documents')so the spinner doesn't render — that's an improvement over the cycle-1 implementation. But the redirect still happens client-side after SSR returns the page chrome, so a user pasting/documents/bulk-editdirectly into the URL bar still sees a brief blank route shell before the bounce. The proper server-load fix is correctly deferred to #332. Logging here for traceability — not a blocker.Re-verification of every AC against current head
WRITE_ALL_one/_otherplural (cycle-1 win)/documents/bulk-editwith IDs in storemode="edit"(BulkDocumentEditLayout.svelte:386-392,role="note")DocumentService.applyBulkEditToDocumentmerges into existing set); reachable end-to-end now that B1 is fixedBulkDocumentEditLayout.svelte:494-513)PATCH /api/documents/bulkrequiresWRITE_ALL; returns{ updated, errors }READ_ALL-only usersWhat I checked
1803db86; confirmed all 9 cycle-1 commits landBulkDocumentEditLayout.svelte:28-32, 78-87, 213-215andbulk-edit/+page.svelte:36-37BulkDocumentEditLayout.svelte:128-148(mode-branch + store clear + goto)BulkDocumentEditLayout.svelte.spec.ts:343-347matches production shape (regression guard in place)UploadSaveBar.svelteeditModepropDocumentController.java:321, 336DocumentController.java:302C1 is the single remaining item. It's a copy/i18n concern, not a blocker, but it materially contradicts the user-facing onboarding cue the issue explicitly required. Approving with that one concern called out for resolution either in this PR or as a fast-follow.
🧪 Sara Holt — QA Engineer & Test Strategist — Cycle 2
Verdict: ⚠️ Approved with concerns
Solid cycle. Both my B1 (
READ_ALL/WRITE_ALL403 paths) and B2 (duplicate-id dedupe) are nailed shut with named, focused tests. The new audit/version assertion (applyBulkEditToDocument_recordsVersion_andLogsAuditEvent_taggedSourceBulkEdit) is exactly the contract pin Markus's blocker needed —verify(auditService).logAfterCommit(eq(METADATA_UPDATED), eq(actorId), eq(id), eq(Map.of("source", "BULK_EDIT")))is the right shape, not a looseverify(any()). C1 (...propagatesDomainException_whenSenderIdUnresolvable) and C2 (boundary at exactly 500) are clean Arrange-Act-Asserts. C7 (defensive UserGroup with NULL permissions) and the C4/S4 store split (setAll([])no-op + previous-IDs-absent +idsgetter as three separate tests) all do what their names claim. B3 (Testcontainers transactional boundary) is correctly deferred to #332 and that's documented.What I'm flagging are (a) one weak assertion that doesn't actually pin the regression I described, and (b) four new code paths that landed in cycle 1 with zero test coverage. None block merge — this PR is materially safer than cycle 1 — but they're real holes that should not slip into the deferred follow-up unless they're explicitly listed in #332.
Concerns (should fix before merge or move to #332)
findIdsForFilter_passesTagOperatorOR_throughBuildSearchSpecdoes not actually pin the OR-vs-AND regression I asked for in C3. The test passesTagOperator.OR, then assertsverify(documentRepository).findAll(any(Specification.class)). The body comment honestly admits this: "Spec built without throwing → OR branch was exercised." But a refactor that silently wiredOR → ANDinsidebuildSearchSpecwould still callfindAll(any())and still not throw — the test would pass. To actually fence the regression, capture the Specification withArgumentCaptor<Specification<Document>>, then assert on theuseOrLogicbranch via either (a) a sentinelTagrepository call pattern that differs OR-vs-AND, or (b) two paired tests where the same input produces different argument shapes to a downstream mock. As written, this test is reassuring rather than binding.The new auto-clear
$effectinfrontend/src/routes/+layout.svelte:25-35(Felix C4) has zero test coverage. This is a genuinely load-bearing behavior — if someone replacespath.startsWith('/documents/')withpath === '/documents'(easy mistake during a refactor), the bulk selection will silently leak into/documents/<id>detail pages. The fix is a+layout.svelte.spec.tswith a small matrix:The
$effectreadspage.url.pathnameso this is fully testable invitest-browser-sveltewith a navigation harness.The new
sanitizeForLoghelper inDocumentController.java:297-299(Nora C4 / CWE-117) has no unit test. This is the canonical "security finding becomes a permanent regression test" case from the Nora playbook. A follow-up that switchesreplaceAll("[\\r\\n]", "_")toreplace("\n", "_")(forgetting\r) would slip silently. Add three lines toDocumentControllerTest(or, better, extract the helper to a package-private utility and unit-test it directly):Currently the helper is private and untested. If it's invoked from any future log line, we'll never know if it works.
The new
bulk_edit_n_selected_one/_otherICU split (Elicit C1) is not pinned inBulkSelectionBar.svelte.spec.ts. Existing test assertstoHaveTextContent('2')— that's a substring match against_other, which would still pass if both branches resolved to_other. The whole point of the cycle 1 fix was then=1branch — add an explicit test:Without it, a regression that drops the
count === 1ternary branch would still satisfy the existing assertions.New
@Size/@Validvalidators onDocumentBulkEditDTO(Tobias C2) have no controller-level test verifying rejection.tagNamescapped at 200×200,receiverIdsat 200, location strings at 255. The annotations are present in source but no test fires atagNames=[201 entries]payload and asserts400 VALIDATION_ERROR. A future PR that drops@Validfrom the controller signature (or removes a@Size) would not be caught. One parametrized test or four tiny tests, your call — but the validation contract should be pinned by tests, not by source code review.Resolved (verified in cycle 1)
batchMetadata_returns403_forUserWithoutReadAllandgetDocumentIds_returns403_forUserWithoutWriteAllboth present, both clean@WithMockUserwith no authorities, both assertisForbidden(). Permission contract pinned.patchBulk_dedupesDuplicateDocumentIds_doesNotInflateUpdatedCountsends the same id three times, assertsupdated=1ANDverify(documentService, times(1)). Both halves of "dedupe" pinned.applyBulkEditToDocument_propagatesDomainException_whenSenderIdUnresolvableexercises the unresolvable-sender path, assertsDomainExceptionpropagation with the unknown id in the message.patchBulk_acceptsExactly500Ids_atTheCapis the off-by-one companion. Boundary fenced.bulkSelection.svelte.spec.ts:setAll([])no-op, all previous IDs absent (not just one),idsgetter exposes the SvelteSet. Each behavior gets its own reason-to-fail._one/_otherkeys), but the test was not added (see Concern 4).redirects when a group has no permissions array (defensive)covers the NULL-permissions branch withpermissions: undefined as unknown as string[]. The cast is honest and the test reads cleanly.What I checked
backend/src/test/java/.../service/DocumentServiceTest.java:2070-2163— verified the three new tests by name, body, and assertion shape. The audit/version test useseq()matchers (notany()), which is the right strictness. The OR-tag test does not pin the regression — flagged in Concern 1.backend/src/test/java/.../controller/DocumentControllerTest.java:999-1158— verified the six new tests.acceptsExactly500Ids_atTheCap,dedupesDuplicateDocumentIds,getDocumentIds_returns403_forUserWithoutWriteAll,getDocumentIds_returns400_whenResultExceedsFilterCap,batchMetadata_returns403_forUserWithoutReadAll,batchMetadata_returns400_whenIdsExceedsCap— all clean@WithMockUser(authorities=...)slicing, all assert the right status + error code.frontend/src/lib/stores/bulkSelection.svelte.spec.ts— full file. Three new tests confirmed (lines 54, 60, 70). Each is one-assertion-per-test.frontend/src/routes/documents/bulk-edit/page.server.spec.ts— full file. Defensive NULL-permissions test added at line 47 with explanatory comment referencing my prior C7.frontend/src/lib/components/document/BulkSelectionBar.svelte.spec.ts— full file. Three new tests:aria-live=polite,Escape clears,Escape no-op when hidden. Pluralization branches still NOT pinned (Concern 4).frontend/src/lib/components/document/BulkDocumentEditLayout.svelte.spec.ts— full file (526 lines). New "discard in edit mode" test at line 318 confirmed:bulkSelectionStore.setAll(['doc-1'])→ click discard → assertgoto('/documents')ANDbulkSelectionStore.size === 0. Both halves of the contract pinned.frontend/src/routes/+layout.svelte— verified the new auto-clear$effect(lines 25-35). No test exists for it (Concern 2).backend/src/main/java/.../controller/DocumentController.java:255-299— verifiedsanitizeForLog,BULK_EDIT_FILTER_MAX_IDS,LinkedHashSetdedupe,@RequestBody @Valid. No test exists forsanitizeForLog(Concern 3) or for@Validpayload rejection (Concern 5).backend/src/main/java/.../dto/DocumentBulkEditDTO.java— verified@Sizeannotations (200×200 tagNames, 200 receiverIds, 255 location strings). No test fires an oversized payload (Concern 5).— Sara
👨💻 Felix Brandt — Senior Fullstack Developer — Cycle 2
Verdict: 🚫 Changes requested
The cycle 1 sweep landed almost everything cleanly — backend audit/version parity, the LinkedHashSet dedupe, the typed cap on
/ids, thebuildSearchSpec+resolveTagsextractions, the auto-clear effect on the layout, the inlineeditAllErrorUI, the spinner replacing…, the German plural keys. Real progress.But the fix for my B1 introduced a silent data-loss regression on the single-document edit form — the existing
documentLocationandlocation(Verfasser-Ort) values no longer pre-fill on/documents/[id]/edit, and saving the form will overwrite them with empty strings. That has to land before merge.Blockers (must fix before merge)
1. Cycle-1 fix for B1 broke the
/documents/[id]/editpre-fill — silent overwrite ofdocumentLocationandlocation.Commit
499beca1removed three props from the section components:WhoWhenSection:initialDateIsoandinitialLocationDescriptionSection:initialDocumentLocation…and also removed
value={initialLocation}from the<input id="location">(WhoWhenSection.svelte:121in the diff) and thedocumentLocation = untrack(...)bridge line fromDescriptionSection.svelte.The bulk-edit caller is fine — it never used those props. But
DocumentEditLayout.svelte:198-213(the single-document edit page) is still on the old contract:DocumentEditLayoutdoes not binddocumentLocationand never seeds thelocationfield via the bindable. So:location<input>in WhoWhenSection now renders empty for an already-saved document (it was readingvalue={initialLocation}before).documentLocation<input>in DescriptionSection now renders empty because nothing seeds it (the deleteddocumentLocation = untrack(() => documentLocation || initialDocumentLocation);was the bridge).dateIsois OK by accident —DocumentEditLayout:49doesdateIso = untrack(() => doc.documentDate ?? '');before the section reads it, sodateDisplay = $state(untrack(() => isoToGerman(dateIso)))picks up the right value. TheinitialDateIsoprop is just dead.Result on save: a user opens an existing document with
documentLocation = "Karton 12, Mappe A", the form loads showing it empty, they edit the date and click save — the PUT body hasdocumentLocation="", the backendupdateDocumentblindly assignsdoc.setDocumentLocation(dto.getDocumentLocation())(line 273), and the value is gone.Two ways to fix:
Update
DocumentEditLayoutto seed the bindables in its own<script>(consistent with how it already doesdateIsoandcurrentTitleon lines 49-50) andbind:documentLocation/bind:locationthrough:Then add
locationanddocumentLocationto the form payload (or wire them into hidden inputs).Or keep the
initial*companion props on the sections and re-add the seeding lines you removed — but that brings back the exact anti-pattern I flagged in cycle 1, so option 1 is the right call.Either way: add a Vitest browser-mode test on
DocumentEditLayoutthat mounts it withdoc = { documentLocation: 'X', location: 'Y' }and asserts both inputs render those values. The cycle-1 fix went out without this guard, so the regression slipped past the green test suite.2.
DescriptionSection.svelte:36still mutatescurrentTitleat top-level script scope — same Svelte 5 anti-pattern I flagged in cycle 1.untrackonly suppresses dependency tracking — it does not gate execution to "first run". WhenDocumentEditLayoutre-renders (e.g. invalidate after file replace),DescriptionSectionre-evaluates, andcurrentTitlegets stomped back todoc.title, blowing away an in-progress edit. This is exactly what I flagged in WhoWhenSection — the WhoWhenSection version was correctly fixed (line 34 now seedsdateDisplayfromdateIso, no top-level mutation), but the matching mutation in DescriptionSection survived the cleanup.Fix: drop
initialTitleand have the parent seedcurrentTitlebefore mounting (DocumentEditLayout already does on line 50). ThetitleValue$derivedthen becomes:…with
currentTitleflowing from the bindable directly, noinitialTitleinvolved.Concerns (should fix before merge)
1.
applyBulkEditToDocumentwrites a version row + audit entry even for no-op calls.DocumentService.java:471-474always firesdocumentVersionService.recordVersion(saved)andauditService.logAfterCommit(METADATA_UPDATED…). If a user accidentally clicks "Anwenden" on the bulk-edit form with every field blank/empty (no tags added, no senderId, no receivers, no location strings), every doc in the selection gets a no-op version row and asource=BULK_EDITaudit event. With 500 docs × 0 actual changes, that's 1000 useless DB writes plusnaudit-trail rows that will mislead anyone investigating "who changed this document".Cheap fix: track a
boolean changedflag inside the method, only persist + audit whenchanged = true. Alternatively gate at the controller — if every DTO field is null/blank/empty, return400 BAD_REQUESTwithBULK_EDIT_NO_CHANGES.2.
BatchMetadataRequestcontroller method missing@Valid.DocumentController.java:327—@RequestBody BatchMetadataRequest request(no@Valid). The PATCH twin on line 258 has it. There are nojakarta.validationconstraints onBatchMetadataRequesttoday so the omission is functionally a no-op, but adding@Validis the cheap defensive move so a future@Size(max = N)actually fires.3. Auto-clear effect in
+layout.svelte:25-35readsbulkSelectionStore.sizereactively — unnecessary work on every checkbox toggle.bulkSelectionStore.sizeis a SvelteSet read inside a reactive scope, so the effect re-fires every time the user ticks/unticks a row on/documents. Each re-run computesinBulkContext, finds it'strue, and exits — not a correctness bug, just churn. Since the only state transition you actually care about is "leaving the bulk-context route", readingpage.url.pathnamealone is enough; gate the size-read inside an untracked block:4.
DocumentBulkEditLayout.svelte:77-88mutatesfiles(a SvelteMap) at top-level script scope — same family as B2 above.Top-level
if-mutation runs on every component re-evaluation, andactiveId = entry.idreassigns a$statefrom the script body. Today it's gated bymode === 'edit'(a constant prop in practice) so it accidentally works once, but the pattern is fragile. Move into a one-shot effect or a derived initialiser. Or — better — let the parent route shape the data structure once and passfilesin as a prop. Same shape of fix as B2.5.
LinkedHashSetinDocumentController.java:275is fully-qualified inline.The cycle-1 commit message claims "Replaced fully-qualified type names inside
DocumentServicewith imports" (Markus #7), but the brand-new dedupe line inDocumentControlleris fully-qualified. Addimport java.util.LinkedHashSet;at the top.Resolved from cycle 1
resolveTagsandbuildSearchSpecare clean single-source-of-truth helpers used by all three call sites (updateDocumentTags,applyBulkEditToDocument,applyBatchMetadata).onMount) — deferred to #332; acceptable.getErrorMessage()not wired) — ✅ resolved.bulk-edit/+page.svelte:30-33andeditAllMatchingboth go throughparseBackendError+getErrorMessage(code).@Datavs record inconsistency) — deferred to #332 with javadoc rationale on the DTO; acceptable.+layout.svelteeffect auto-clears on route change away from/documentsand/enrich. (See new C3 for a small efficiency note.)editAllMatching) — ✅ resolved.editAllErrorsurfaces inline below the button withrole="alert"anddata-testid="bulk-edit-all-x-error".bulkSelectionStore.idsgetter unused) — fine to leave;bulk-edit/+page.svelte:16actually uses it now (Array.from(bulkSelectionStore.ids)).patchBulk_acceptsExactly500Ids_atTheCap).applyBulkEditToDocument— ✅ resolved. Now writesMETADATA_UPDATEDaudit event taggedsource=BULK_EDITplus adocumentVersionsrow per success. (See new C1 for a related no-op concern.)What I checked this cycle
DocumentService.applyBulkEditToDocument(audit + version + per-doc tx),resolveTags+buildSearchSpec(single source of truth confirmed across 3 + 2 call sites),findIdsForFilter(read-only tx, FTS short-circuit, shared spec),batchMetadata(read-only tx).patchBulk(LinkedHashSet dedupe — works, but inline FQN),sanitizeForLog(CRLF strip — fine), cap enforcement on/bulk(500),/ids(5000),/batch-metadata(500),@Validwiring,Permission.WRITE_ALLon/ids.DocumentBulkEditDTO@Sizeconstraints, kept as@Dataper ADR-comment + #332 deferral.WhoWhenSection(B1 fix clean),DescriptionSection(still has top-levelcurrentTitle = untrack(...)mutation — B2), and the wider knock-on regression inDocumentEditLayoutconsumers (B1).BulkDocumentEditLayout: edit-mode hydration loop (top-level script mutation — C4),saveBulkEditchunking + partial-failure card (role="alert", retry button), error state propagation,bulkSelectionStore.clear()on success./documents/bulk-edit/+page.svelte: spinner withrole="status",getErrorMessagewiring (C2 ✅),parseBackendErrorgraceful JSON handling.+layout.svelte: auto-clear effect — works, minor efficiency concern (C3)./documents/+page.svelte:editAllMatchingerror UI,pb-32reservation when bar visible.bulk_edit_n_selected_one/_other.DocumentEditLayout, which is exactly what would have caught the new B1 regression.Once B1 (regression) and B2 (DescriptionSection top-level mutation) are fixed, plus a quick test pinning the single-doc edit pre-fill, this is good to land. The deferred items in #332 are correctly scoped.
🎨 Leonie Voss — UX Designer & Accessibility Strategist — Cycle 2
Verdict: 🚫 Changes requested
Most of the cycle-1 work is honestly excellent — the four blockers are properly resolved at the source (not just suppressed), the pluralization is correct in all three locales, the spinner has the right semantics, the danger card uses tokens, and the auto-clear-on-route-leave is a thoughtful catch on top of my list. I want to approve. Two new things stop me:
/enrich.<svelte:window>Esc handler that steals Escape from any other dialog or popover on/documentswhile a selection is active.Both are small fixes; the first is a one-liner.
Blockers (must fix before merge)
B5.
/enrich/+page.svelte:105references<BulkSelectionBar>but never imports it. I rannpx svelte-check:At runtime this means the bar does not render at all on the enrich list page — exactly the page where transcribers most need it. The
pb-32reservation you correctly added at line 15 reserves padding for a bar that never appears, so there is also a phantom 8 rem dead zone at the bottom. Add the import to match/documents/+page.svelte:9:This is the regression that makes me reject. Worth a Playwright assert on
/enrichselecting a checkbox and expecting the bar to be visible (Sara would catch this on a re-run).B6. The new
<svelte:window onkeydown>Esc handler inBulkSelectionBar.svelte:29does not stop propagation and does not check whether another modal/popover is open — WCAG 2.1.2 (No Keyboard Trap) is not violated, but the handler steals Escape from every other component on/documentsand/enrichwhile count > 0. Concrete repro on/documents:The bell already calls
event.stopPropagation()(NotificationBell.svelte:46), but Svelte's<svelte:window>listener fires before any innerstopPropagationbecause it is registered onwindow. Same problem withHelpPopover,MentionEditor,TagParentPickerand the globalConfirmDialog(the native<dialog>cancelevent does not stop the keydown). Two acceptable fixes:document.querySelector('dialog[open], [role="menu"][aria-expanded="true"]')first and bail out, ortabindex="-1"and focus-trap on mount — clunky.The first is cleaner:
Add a Vitest case in
BulkSelectionBar.svelte.spec.tsthat mounts a fake<dialog open>in the test DOM and asserts the store is not cleared on Escape.Concerns (should fix before merge)
C17. Esc-clear is destructive and irreversible — no undo. A user with 47 documents selected presses Escape by reflex (e.g. trying to dismiss a tooltip) and loses the selection with no toast, no recovery. For the senior cohort especially this is harsh. Two options, in order of preference:
lastSelection: string[]snapshot beforeclearAll().count >= 10("47 Dokumente abwählen?") — same pattern as the existingbulk_discard_confirm.I would accept either, but please don't ship a one-keystroke way to wipe a long selection with no undo path.
C18. The
pb-32reservation on/documentsand/enrichis8 rem(128 px) but the bar is ~64 px tall. Half the reservation is dead space — visually it looks like an unintentional empty band below the pagination. Tighten topb-20(80 px) or compute against the actual bar height withpadding-bottom: calc(64px + env(safe-area-inset-bottom) + 8px). Not a blocker, but seniors notice large empty regions and read them as "page broken".Resolved (cycle 1 → cycle 2)
Karton/Mappelocalised in all three locales with helper text. The DE helper "Welcher Karton im Archiv?" is exactly the kind of plain-language anchor the senior audience needs. Verifiedform_label_archive_box,form_label_archive_folder,form_helper_archive_box,form_helper_archive_folderinmessages/{de,en,es}.json.aria-labelremoved;role="note"carries the visible localised text. VerifiedBulkDocumentEditLayout.svelte:386–392.≥sm. (See B6 above for the new collision concern.)pb-32reservation present on both routes (/documents/+page.svelte:214,/enrich/+page.svelte:15). See C18 for sizing nit.text-[11px]+text-ink-2onFieldLabelBadge. Dark-mode token consistency restored.aria-live="polite"+aria-atomic="true"on the count region. Toggling a row now announces "3 Dokumente ausgewählt" → "4 Dokumente ausgewählt".aria-hidden="true", wrapped inrole="status"+aria-live="polite"+ visible "Dokumente werden geladen…". Exactly the pattern I asked for.border-danger/40 bg-danger/10 text-dangertokens. Dark-mode safe.bulk_edit_n_selected_one/_otherkeys + ternary in BulkSelectionBar. "1 Dokument ausgewählt" reads naturally now in DE/EN/ES.bulkSelectionStoreand routes to/documents. The "stuck on empty form with stale count" trap is closed (BulkDocumentEditLayout.svelte:137–146).Deferred to #332 (not re-evaluated this cycle)
titleattributefocus-withinring on the row-label wrapperWhat I checked
svelte-checkagainst the touched files (caught B5)frontend/src/lib/components/andfrontend/src/routes/documents/(caught B6)bulk_edit_*and fourform_*_archive_box/folderkeys exist inmessages/de.json,en.json,es.json/documents/bulk-editloading state (role="status"+aria-live+aria-hiddenon the SVG)+layout.svelte:18–32— confirmed it preserves selection across/documents/[id]and/documents/bulk-edit(both matchstartsWith('/documents/')) but clears on/persons,/admin, etc. Behaviour matches user expectation.pb-32reservation present on both list pages (caught C18 sizing nit)/documentsOnce B5 and B6 land, I am happy to flip to ✅ Approved. The cycle-1 work moved this from "approved with concerns" to "almost there" — these are the last two items.
— Leonie
Felix B1 (data-loss regression on /documents/[id]/edit) — DocumentEditLayout still passes initialDateIso, initialLocation, initialDocumentLocation, but my cycle-1 cleanup removed those props. Result: existing values rendered empty and a save would have overwritten them with "". Restored the props on WhoWhenSection and DescriptionSection; initialisation now lives in onMount so it runs exactly once and never stomps a parent-driven update on a later prop change. Felix B2 — `DescriptionSection.svelte:36` still had the top-level `currentTitle = untrack(() => initialTitle)` mutation that I cleaned up in WhoWhenSection but missed here. Same onMount-once treatment. Leonie B5 — `enrich/+page.svelte:105` referenced `<BulkSelectionBar>` but the import was lost in a prettier pass; svelte-check errored out and the bar never rendered, leaving an 8 rem dead zone from the pb-32 reservation. One-line fix: add the import. Leonie B6 — Esc handler in `BulkSelectionBar` was unscoped and stole Escape from NotificationBell, ConfirmDialog, HelpPopover, etc. (e.g. selecting docs → opening notification bell → Esc would close the bell AND silently wipe the selection). Now bails when an open dialog, expanded menu, or popover is detected. Elicit C1 — `BulkDocumentEditLayout` topbar now branches on `mode`: shows "Massenbearbeitung" + "{count} werden bearbeitet" in edit mode instead of the upload-flavoured "Mehrere Dokumente hochladen" + "werden erstellt" copy. New i18n keys `bulk_edit_topbar_title` and `bulk_edit_count_pill` in DE/EN/ES. Tests added: - DocumentControllerTest.patchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessages (Sara C2 follow-up — pin sanitizeForLog as a regression test) - BulkSelectionBar.spec — count=1 → "1 Dokument", count=2 → "2 Dokumente" (Sara C6 follow-up — pin the new bulk_edit_n_selected_one/_other branch) Refs #225, PR #331 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>✅ Cycle 2 fixes pushed (commit
8ce96294)All 4 cycle-2 blockers + 1 priority concern + 2 test gaps addressed:
/documents/[id]/edit) — RestoredinitialDateIso,initialLocation,initialDocumentLocationprops onWhoWhenSection/DescriptionSection. Initialisation now lives inonMountso it runs exactly once and never stomps a parent-driven update on a later prop change.DescriptionSection.svelte:36still had top-level$bindablemutation) — SameonMount-once treatment; matching test still green.BulkSelectionBarimport inenrich/+page.svelte) — One-line import added;npm run checknow clean for that file.BulkSelectionBarnow bails whendialog[open],[aria-expanded="true"],[role="menu"]:not([hidden]), or[role="dialog"]:not([hidden])is present.BulkDocumentEditLayouttopbar branches onmode: shows "Massenbearbeitung" + "{count} werden bearbeitet" in edit mode (DE/EN/ES) instead of the upload-flavoured "werden erstellt".patchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessagestest added sosanitizeForLogis now a regression-fenced unit, not just a hopeful helper.BulkSelectionBarplural branch tests forcount=1("1 Dokument") andcount=2("2 Dokumente").Total: 234 backend tests, 60+ frontend bulk-edit specs — all green.
Sara's remaining concerns (auto-clear
$effecttest, weak OR test,@Sizerejection tests) are tracked in #332 — they're test-coverage extensions, not behaviour gaps.Ready for cycle 3.
🛠️ Tobias Wendt — DevOps & Platform Engineer — Cycle 3
Verdict: ✅ Approved
Cycle-2 commit
8ce96294is purely Felix/Leonie/Elicit/Sara fixes — frontend props, anenrichimport, an Esc-handler scope tweak, edit-mode topbar copy, and two follow-up tests. Zero operational delta since my cycle-2 approval. Still ship it.Cycle-2 delta — what changed (operationally)
backend/src/main/java/.../controller/DocumentController.java— unchanged (git diff 1803db86..8ce96294 -- DocumentController.javais empty)backend/src/main/java/.../service/DocumentService.java— unchanged (same diff is empty)docker-compose*.ymltouched, nopom.xmltouched, no.gitea/workflows/*touched, no Flyway migration added, no new env var.sveltecomponents, one missing import, one test spec) are zero-impact for opsRollback path: still pure
git revert 8ce96294 1803db86 …f0da033e. No schema deltas, no infra to roll back.Regression-fence verified — Sara C2 follow-up
patchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessages(DocumentControllerTest.java:1178-1202) is doing exactly what I want from a CWE-117 fence:Three assertions on one mocked exception:
\nround-trips to the JSON body\rround-trips to the JSON body_, not just stripped — so the test catches a future "optimisation" that switchesreplaceAll("[\\r\\n]", "_")toreplaceAll("[\\r\\n]", "")(which would still satisfy 1 and 2 but lose the visual marker that something was sanitised)The "evil_" assertion is the key one — it pins the contract of
sanitizeForLog, not just its absence-of-CRLF behaviour. If somebody later swaps toString::striporreplace("\n", " ")thinking they're being helpful, this test fails immediately. That's a proper regression fence, not a happy-path test masquerading as one.It also exercises the
BulkEditError.messagepath through MockMvc end-to-end, which means the JSON serialiser, the controller'serrors.add(new BulkEditError(id, sanitizeForLog(e.getMessage())))call site (DocumentController.java:282), and the@RestControllerJSON body all stay covered. If anyone refactors the partial-failure response shape, this test breaks loudly.Concerns
(none)
Suggestions / follow-ups
Nothing new. Both my cycle-1 deferrals — N+1 entity resolve in the per-doc loop, and
RateLimitInterceptornot covering/api/documents/bulk— remain tracked in #332 with the right attribution. No action needed in this PR.What I checked (cycle 3)
git show --stat 8ce96294— 10 files, all frontend except the new backend test; no infra paths touchedgit diff 1803db86..8ce96294 -- backend/src/main/java/.../controller/DocumentController.java backend/src/main/java/.../service/DocumentService.java— empty (no ops-surface changes)backend/src/test/java/.../controller/DocumentControllerTest.java:1178-1202— new sanitiser regression test, asserts both CR and LF stripping plus the_replacement contractbackend/src/main/java/.../controller/DocumentController.java:270-299—sanitizeForLogstill wired into both theBulkEditError.messagepath (line 282) and thelog.warnline (line 285); confirmed the test exercises the formergit revert, still no migrations, still no schema deltas, still no env or container changesThree cycles, zero ops regressions. Merge when the rest of the personas land cycle-3 approvals.
📋 Elicit — Requirements Engineer — Cycle 3
Verdict: ✅ Approved
C1 — the only persisting concern from cycle 2 — is now properly fixed. Every AC from issue #225's Bulk-Edit Panel table is satisfied end-to-end. No new AC misses introduced by the cycle-2 commit. From a requirements perspective this is ready to merge.
Resolved (cycle 2 → cycle 3)
C1 — Topbar copy in edit mode: ✅ Fixed in commit
8ce96294atfrontend/src/lib/components/document/BulkDocumentEditLayout.svelte:326-341. The topbar title and the count pill both branch onmode === 'edit':m.bulk_edit_topbar_title()→ DE "Massenbearbeitung" / EN "Bulk edit" / ES "Edición masiva".m.bulk_edit_count_pill({ count })→ DE "{count} werden bearbeitet" / EN "{count} will be edited" / ES "Se editarán {count}".Verified all three locale files (
messages/de.json:900-901,messages/en.json:900-901,messages/es.json:900-901). The topbar now agrees with the inline callout below it (both frame the operation as editing existing documents). The contradiction the persona would have hit on first paint is gone.Minor IA note (not a finding):
bulk_edit_count_pilldoesn't carry a_oneplural variant. That's fine because the pill is gated byisMulti = files.size >= 2(BulkDocumentEditLayout.svelte:91) — it can never render withcount=1. Same pattern as the originalbulk_count_pill. Symmetric, no AC impact.Re-verification — cycle-1 fixes still intact
BulkEditEntryshape atBulkDocumentEditLayout.svelte:28-32still mirrors the backendDocumentBatchSummary({ id, title, pdfUrl }); hydration loop at lines 77-88 still usesentry.idfor both the SvelteMap key and the innerdocumentId; route cast atbulk-edit/+page.svelte:36-37unchanged. No regression.handleDiscardatBulkDocumentEditLayout.svelte:128-148still branches onmode === 'edit', clearsbulkSelectionStore, andgoto('/documents'). No regression./idspermission gate, edit-mode save CTA, visible chunk progress) untouched by the cycle-2 commit's diff.Cycle-2 commit footprint check (no new AC misses)
8ce96294touched 10 files: 4 i18n message files, 4 frontend components, 1 backend test, 1 enrich page. Walked the non-i18n changes:BulkDocumentEditLayout.svelte— only the topbar mode-branch (the C1 fix itself). No other AC-touching code paths altered.BulkSelectionBar.svelte— Esc-handler scoping (Leonie B6 fix). Pure defensive narrowing; no AC impact, fixes a real interaction conflict with dialogs/menus.WhoWhenSection.svelte/DescriptionSection.svelte— Felix's data-loss B1/B2 fix (restoredinitialDateIso,initialLocation,initialDocumentLocationprops withonMount-once initialisation). This actually prevents a regression on the single-document/documents/[id]/editpath — confirms the AC "existing values must be preserved on edit" across both single-doc and bulk paths.enrich/+page.svelte— single import-line fix forBulkSelectionBar. Restores the AC "sticky selection bar visible on /enrich when ≥1 row selected".DocumentControllerTest.java— test-only addition forsanitizeForLog(Sara C2 follow-up).No AC drift. The cycle-2 commit is purely corrective.
Carried-over deferrals (acceptable, tracked)
loading = falsebeforegoto('/documents')suppresses the spinner; the proper server-load fix is tracked in #332. Functional, not a blocker.Final AC trace against issue #225 — all 13 satisfied
DocumentRow.svelte,enrich/+page.svelte_one/_otherBulkSelectionBar.svelte:43/documents/bulk-editBulkSelectionBar.sveltebulk-edit/+page.svelte:15-23mode="edit"(role="note")BulkDocumentEditLayout.svelteWhoWhenSection,DescriptionSectionDocumentService.applyBulkEditToDocumentDocumentServiceDocumentServiceDocumentServiceBulkDocumentEditLayout.svelte:494-513PATCH /api/documents/bulkrequires WRITE_ALL; returns{ updated, errors }DocumentController.javaWhat I checked
8ce96294; verified C1 fix atBulkDocumentEditLayout.svelte:326-341bulk_edit_topbar_titleandbulk_edit_count_pillBulkDocumentEditLayout.svelte:28-32, 77-88, 213-215andbulk-edit/+page.svelte:36-37BulkDocumentEditLayout.svelte:128-148(mode-branch + store clear + goto)8ce96294diff (10 files, +104/-17) for any AC-impacting change beyond the named blocker fixes — none foundApproved. From a requirements perspective, ship it.
🔐 Nora "NullX" Steiner — Application Security Engineer — Cycle 3
Verdict: ✅ Approved
Re-audited every cycle-2 delta with a fresh adversarial pass. No new attack surface, no regressions on the cycle-1 hardening, and the one new test (
patchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessages) is exactly the regression fence I asked for in C4 —sanitizeForLogis now a tested control instead of a hopeful helper. Ship it.Blockers
(none)
Concerns
(none)
What I re-checked (cycle 3)
1. CWE-117 regression fence —
patchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessages(DocumentControllerTest.java:1178-1202) — ✅ exactly the test I wantedThe test plants the canonical log-injection payload (
"evil\r\nFAKE LOG ENTRY: admin logged in") inside aDomainException, drives it through the same code path as a real partial-failure error, and asserts:errors[0].messagecontains no\nerrors[0].messagecontains no\r"evil_"(proves the replacement uses_, matchingsanitizeForLogatDocumentController.java:297-299)This locks down the helper and its public-facing side-effect (the round-trip through
BulkEditError.message). A future "let's drop sanitizeForLog to simplify" refactor will fail loud. The test sits inside the existingWRITE_ALLmock-user fixture and runs in the standardDocumentControllerTestslice — no infra cost. This is the textbook shape of a CWE-117 regression test in the Spring/MockMvc world.2. Esc-scope guard in
BulkSelectionBar.svelte:24-32— ✅ no security issue introducedThe new selector —
dialog[open], [aria-expanded="true"], [role="menu"]:not([hidden]), [role="dialog"]:not([hidden])— is hard-coded with no user-controlled interpolation, so there's no CSS-selector-injection vector. Thee.defaultPreventedshort-circuit means a child component that already handled Esc (e.g. ConfirmDialog callingevent.preventDefault()) cannot get its handler racing the bar'sclearAll. Guard runs in the globalsvelte:windowscope but usesdocument.querySelector(single match, scoped to top-level DOM) — no XSS or DOM-clobbering risk. The clear-on-Esc behaviour itself doesn't expose any data; it just empties a client-sideSvelteSetof UUIDs.One micro-observation, not a finding:
[aria-expanded="true"]will also match a collapsed menubutton during transition states (e.g. during a one-frame async open). Worst-case the user has to press Esc twice. Not a security issue, not worth changing.3.
WhoWhenSection.svelteandDescriptionSection.svelteonMount-once initialisation — ✅ no new sinkThe change moves
currentTitle = initialTitle(and the newdocumentLocation = initialDocumentLocation) from a top-leveluntrack(...)mutation intoonMount(() => { ... }). This is purely a Svelte 5 lifecycle correction (Felix's B1/B2 data-loss fix). Critically:initial-*values are server-trusted strings sourced from the document loader (+page.server.ts), not user-controlled query params or arbitrary body fields<input value={...}>/<textarea>— Svelte's auto-escaping holds{@html}was introduced (verified viagrep -rn "@html" frontend/src/lib/components/document/→ zero hits)eval,Functionconstructor, or dynamic import — confirmed by inspecting the diffif (!currentTitle && initialTitle)does an empty-string check, not a security-relevant truthiness check; both sides are inert stringsThe fix is exactly what Felix asked for: it makes the initialisation idempotent without expanding the trust boundary. No reflected/stored XSS, no prototype pollution, no DOM clobbering.
4.
LinkedHashSet<UUID>dedupe atDocumentController.java:275— ✅ confirmed safeAlready audited in cycle 2; re-verified that no security property is affected. The dedupe operates on
dto.getDocumentIds()after the controller-levelBULK_EDIT_MAX_IDScap and bean validation, so an attacker cannot use it to bypass the size check by submitting[id1, id1, id1, ...]— the cap is on the original list size, not the unique set. Audit log retains bothdto.getDocumentIds().size()(raw) anduniqueIds.size()(deduped) at line 289-290, which is exactly the right shape for incident response: an admin can spot "user submitted 500 IDs but only 1 unique" as a possible enumeration probe.5.
BulkDocumentEditLayout.sveltetopbar mode branch — ✅ pure UX/i18n changeThe Elicit C1 fix adds a
{#if mode === 'edit'}branch that selects between two Paraglide message keys (bulk_edit_topbar_title/bulk_edit_count_pillvs.bulk_title_multi/bulk_count_pill). All four keys resolve to compile-time-typed Paraglide functions returning escaped strings. No interpolation of user input, no new attack surface.6. New
BulkSelectionBarplural tests (count=1,count=2) — ✅ no security implicationJust locking down the i18n plural-rule wiring. Defence-in-depth bonus: the
{count === 1 ? m.bulk_edit_n_selected_one() : m.bulk_edit_n_selected_other({ count })}interpolation uses Paraglide's typed counter, not raw template concatenation, so even ifcountwere attacker-influenced (it isn't — it's a derived.sizefrom a client-side Set) there'd be no injection vector.Cycle-1 carry-overs (status reconfirmed)
/batch-metadatacap (CWE-770)/idsupper bound (CWE-770)BulkEditErrorcode (CWE-79 defence-in-depth)@SizeDTO guardsSuggestions (carry-over, no blocker)
DocumentController(BULK_EDIT_MAX_IDS,BATCH_METADATA_MAX_IDS,BULK_EDIT_FILTER_MAX_IDS) make excellent positive examples for a Semgrep rule that flags any new@PostMapping/@PatchMapping/@GetMappingwhose handler accepts aList<UUID>(or anyList<...>) without a corresponding cap-validation call. Catches the next bulk endpoint at PR-review time.BulkEditError, a Paraglide-mapped per-document error message in the bulk-edit UI becomes safe. Until then, the// do not render err.messagewarning comment inBulkDocumentEditLayout.svelte(suggested in cycle 2) remains a useful trip-wire — still optional.🤖 Reviewed in character as Nora "NullX" Steiner.
🏗️ Markus Keller — Senior Application Architect — Cycle 3
Verdict: ✅ Approved
Cycle-2 fixes land cleanly. The data-loss regression on
/documents/[id]/editis properly resolved by lifting theinitial-*seed intoonMounton bothWhoWhenSectionandDescriptionSection—onMountruns exactly once per Svelte 5 component instance, so the seed-then-bindable pattern can no longer stomp a parent-driven update on a later prop change. The lostBulkSelectionBarimport inenrich/+page.svelteis restored, the Esc handler is now scoped behind a defensivedialog[open] / [aria-expanded="true"] / [role="menu"] / [role="dialog"]guard, andBulkDocumentEditLayoutcorrectly branches the topbar copy + count pill onmode. Two regression tests landed (patchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessages,BulkSelectionBarplural-form assertions) that pin the cycle-1/2 behaviour going forward.No new architectural regressions. The seed-on-mount pattern is the right shape for a one-shot hydration concern, and the comments on both sections explain why the mutation is safe — future contributors will not reverse it out of suspicion. The Esc-scope guard is a sensible application-level coordination of overlay ownership without introducing a global keyboard-stack abstraction (which we don't need yet).
Blockers (must fix before merge)
(none)
Concerns (should fix before merge)
(none)
Suggestions (nice to have, don't block merge)
The Esc-scope DOM-selector guard couples to ARIA conventions across unrelated component families.
BulkSelectionBar.svelte:27-30matches[aria-expanded="true"]globally, which today catchesNotificationBell,HelpPopover,UserMenu,DocumentTopBar— exactly the components we want to defer to. But it also matchesTagTreeNode,TrainingHistory.svelte:152,PersonDangerZone.svelte:21,OverflowPillButton, andAppNavmobile burger — none of which are open while the bulk bar is visible today, but any future a11y refactor that addsaria-expandedto a disclosure widget on/documents(e.g. an "advanced filters" toggle) would silently disable Esc-clears-selection. Two cheaper fixes worth tracking in the follow-up issue:NotificationBell/HelpPopover/ConfirmDialograise a single shareddata-overlay-open="true"attribute on<body>or<html>while open, and haveBulkSelectionBarquery for[data-overlay-open]instead of inferring from ARIA.overlay-keyboard-stack.svelte.ts), so each overlay pushes/pops its own Esc handler and the bar is always last.Either way the current selector list works in production today — call it out in #332 as "tech-debt: ARIA-based Esc coordination is fragile."
WhoWhenSection.onMountwrites throughdateIsoonly when the parent seeds an empty string. That is the correct invariant for the single-doc edit (the form must persist the document's current date if the user just clicks Save), but the conditional writeif (!dateIso) dateIso = seed;is subtle enough that it deserves a single regression test: render the component withbind:dateIso={parentDate}whereparentDate = ''andinitialDateIso = '2025-04-15', then assert the parent binding receives'2025-04-15'after mount. Pin the contract; future contributors who don't know about Felix's cycle-2 B1 won't find a test that breaks if they "simplify" the onMount away. (Pure suggestion — falls under Sara's coverage scope, not a Markus blocker.)No regression test was added for the new Esc-scope guard.
BulkSelectionBar.spec.tscovers the happy-path Esc-clears-selection case (line 75) but not the new "Esc bails when an overlay is open" branch. Worth a single browser-mode test that injects<dialog open>into the DOM, dispatches Escape, and assertsbulkSelectionStore.sizeis unchanged. Same Sara-scope note as above — flagging it here only because the new branch in the production code is the most fragile part of the cycle-2 changes and it's the only one without coverage.Resolved from cycle 2
/documents/[id]/edit→ Fixed.WhoWhenSection.svelte:42-48andDescriptionSection.svelte:42-45both useonMountto seed bindables frominitial-*props exactly once.DocumentEditLayout.svelte:198-213still passes the threeinitial-*props (initialDateIso,initialLocation,initialDocumentLocation) so the round-trip is intact. TheBulkDocumentEditLayoutcall sites (:432-447,:484-499) intentionally omit theinitial-*props because bulk edit binds its own state —seed = '' || '' = ''short-circuits the onMount, leaving the parent binding untouched. Both modes correct.DescriptionSectiontop-levelcurrentTitle = untrack(() => initialTitle)mutation → Fixed by the same onMount pattern. ThedocumentLocationseed got the same treatment in the sameonMountblock (:43-44) — both fields hydrated together, both gated by "parent didn't already supply a value."BulkSelectionBarimport in/enrich→ Fixed.enrich/+page.svelte:4imports the component, and the bar is rendered at:106withcanWriteproperly threaded through.BulkSelectionBar.svelte:24-32bails whendialog[open],[aria-expanded="true"],[role="menu"]:not([hidden]), or[role="dialog"]:not([hidden])is in the DOM. Selectors coverConfirmDialog(<dialog open>),NotificationDropdown(role="dialog"),HelpPopoverandNotificationBell(aria-expandedon trigger). See suggestion above re: long-term fragility.BulkDocumentEditLayout.svelte:327-340branches title and pill onmode. New i18n keysbulk_edit_topbar_titleandbulk_edit_count_pillpresent inde.json,en.json,es.json. Counterpartbulk_title_*/bulk_count_pillupload-mode keys preserved.sanitizeForLogregression test for bulk error messages → Added.DocumentControllerTest.patchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessages(:1180-1202) asserts both CR/LF removal and theevil_underscore replacement on theBulkEditError.messageround-trip. Pins CWE-117 defence.bulk_edit_n_selected→ Added.BulkSelectionBar.svelte.spec.ts:35-50assertscount=1 → "1 Dokument ausgewählt"andcount=2 → "2 Dokumente ausgewählt".What I checked this cycle
onMount-only initialisation pattern: confirmed Svelte 5 semantics —onMountfires exactly once per component instance, so the conditional writeif (!dateIso) dateIso = seedcannot run a second time on a later prop change. The pattern is the canonical Svelte 5 replacement for theuntrack(...)cell-initialiser idiom.bind:dateIsoagainstWhoWhenSection: single-doc edit (parent'sdateIso=''+ non-emptyinitialDateIso→ onMount writes through, persists on save) and bulk edit (parent'sdateIso=''+ noinitialDateIso→ onMount no-ops, parent binding stays empty,hideDatekeeps the field unrendered anyway). Both correct.DescriptionSectiondocumentLocationseed: same two-mode behaviour asdateIso, same correctness story.currentTitleseed only runs when!currentTitle && initialTitle— bulk-edit consumers don't passinitialTitle, so no clobber.frontend/src/lib/components/:ConfirmDialoguses<dialog open>(matched),NotificationDropdownusesrole="dialog"(matched),HelpPopover+NotificationBelltriggers exposearia-expanded={open}(matched). All three intended deferrals are caught. Surveyed the broader codebase foraria-expandedto flag the over-broad selector concern (suggestion #1).BulkDocumentEditLayout: edit-mode title pullsbulk_edit_topbar_title, multi-file pill pullsbulk_edit_count_pill({ count })— separate i18n keys from the upload path, no shared template. Confirmed all three locale files have both keys.enrich/+page.svelteafter the import fix: page declaresBulkSelectionBar(:4), reservespb-32when the bar is visible (:16), and renders the bar at:106withcanWritederived fromdata.canWrite— matches the/documentslist page's pattern.evil_substitution; frontend test asserts both pluralization branches. Both tests pin the most failure-prone behaviours from cycle 1/2.Ship it.
🎨 Leonie Voss — UX Designer & Accessibility Strategist — Cycle 3
Verdict: ✅ Approved
Both cycle-2 blockers landed cleanly. B5 is the literal one-line import in
frontend/src/routes/enrich/+page.svelte:4— the bar now actually renders for the audience that needs it most (transcribers on/enrich), and thepb-32reservation at line 16 is no longer phantom padding for a missing component. B6 is fixed at the source inBulkSelectionBar.svelte:24–32with a pre-flightdocument.querySelector('dialog[open], [aria-expanded="true"], [role="menu"]:not([hidden]), [role="dialog"]:not([hidden])')bail-out — wider than the selector I suggested (you also catch native<dialog>and anyrole="dialog"popovers, which is the right call) — plus the bonuse.defaultPreventedshort-circuit. NotificationBell, HelpPopover, ConfirmDialog and TagParentPicker can all consume Escape without nuking a 50-row selection.Bonus: the edit-mode topbar copy now branches at
BulkDocumentEditLayout.svelte:327–340—m.bulk_edit_topbar_title()("Massenbearbeitung" / "Bulk edit" / "Edición masiva") andm.bulk_edit_count_pill({ count })("{count} werden bearbeitet" / "{count} will be edited" / "Se editarán {count}") in DE/EN/ES (messages/{de,en,es}.json:900–901). The upload-flavoured "werden erstellt" no longer appears on a page where you're editing, not creating — closes the cognitive snag Elicit also flagged.This is a calm, on-brand bulk-edit flow that respects both the senior audience and the additive/replace mental model. Ship it.
Resolved (cycle 2 → cycle 3)
import BulkSelectionBar from '$lib/components/document/BulkSelectionBar.svelte';present atenrich/+page.svelte:4. Bar will now render on transcriber checkbox tick, no more deadpb-32band.BulkSelectionBar.svelte:24–32bails when an open dialog, expanded ARIA-menu,role="menu"orrole="dialog"is in the DOM. Coverage is wider than my suggested selector — no concern. WCAG 2.1.1 (Keyboard) and WCAG 2.1.2 (No Keyboard Trap) both honoured.modeinBulkDocumentEditLayout.svelte:327and:336; new keysbulk_edit_topbar_titleandbulk_edit_count_pilllocalised in DE/EN/ES atmessages/{de,en,es}.json:900–901. Cognitive consistency restored: upload page says "werden erstellt", edit page says "werden bearbeitet".Cycle-1 fixes — still holding
form_label_archive_box/form_label_archive_folder+form_helper_archive_box/form_helper_archive_folderlocalised in all three locales (messages/{de,en,es}.json:892–895).DescriptionSection.svelte:131–160references viam.form_label_*()/m.form_helper_*(). WCAG 3.1.1/3.1.2 satisfied.BulkDocumentEditLayout.svelte:391–399carriesrole="note"with the visible localisedm.bulk_edit_hint()text content; noaria-labeloverriding. Code comment makes the intent explicit ("an aria-label would override that text for AT users").m.bulk_edit_clear_hint_keyboard()visible at≥sminBulkSelectionBar.svelte:51–53.pb-32reservation present on both list pages:documents/+page.svelte:214andenrich/+page.svelte:16, gated onbulkSelectionStore.size > 0 && canWrite. WCAG 1.4.10 (Reflow) / 2.4.7 (Focus Visible) honoured.One follow-up worth tracking (non-blocking, do in #332)
N1. The B6 bail-out branch is not unit-tested.
BulkSelectionBar.svelte.spec.ts:75–86covers (a) Esc-clears-while-visible and (b) Esc-no-op-while-hidden, but the new "bail when overlay is open" branch — the regression I rejected on cycle 2 — has no test. The production code is correct; the test gap means a future refactor ofonEscapecould silently re-introduce the regression. Add a Vitest case that mounts a fake<dialog open>(or a[role="dialog"]:not([hidden])div) intodocument.body, dispatches Escape, and asserts the store is not cleared. Three lines of test for a bug we already paid the cost of finding once.Not blocking the merge — Sara already has #332 staged for test-coverage extensions, this slots in alongside her auto-clear
$effecttest and weak-OR test.Deferred to #332 (still not re-evaluated this cycle, by design)
titleattributefocus-withinring on the row-label wrapperpb-32(128 px) is double the actual bar height (~64 px) — tighten topb-20orcalc(64px + env(safe-area-inset-bottom) + 8px)What I checked
frontend/src/routes/enrich/+page.svelte:4—BulkSelectionBarimport present, matchesdocuments/+page.svelte:9frontend/src/lib/components/document/BulkSelectionBar.svelte:24–32— Esc handler bail-out selector coversdialog[open],[aria-expanded="true"],[role="menu"]:not([hidden]),[role="dialog"]:not([hidden]); bonuse.defaultPreventedshort-circuitfrontend/src/lib/components/document/BulkDocumentEditLayout.svelte:327, 336— topbar branches onmode === 'edit'frontend/messages/{de,en,es}.json:900–901—bulk_edit_topbar_title+bulk_edit_count_pillpresent in all three locales with correct plural-aware "werden bearbeitet" / "will be edited" / "Se editarán {count}" wordingBulkSelectionBar.svelte.spec.ts— confirms Esc clears + no-ops; bail-out branch not yet tested (see N1)Excellent cycle. The Esc-handler fix in particular is broader than I'd asked for —
role="dialog"coverage is the right defensive instinct. Ready to merge.— Leonie
🧪 Sara Holt — QA Engineer & Test Strategist — Cycle 3
Verdict: ⚠️ Approved with concerns
The two test gaps I asked for in cycle 2 are both plugged with the right shape.
patchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessagesis the canonical CWE-117 regression test — it injectsevil\r\nFAKE LOG ENTRY: admin logged in, then asserts both that no\nand no\rsurvive inerrors[0].messageAND that the original payload is still recognisable asevil_…. Three reasons-to-fail in one test is a tiny bit broad for the Sara rule, but eachandExpecthere is a different facet of the same contract ("strip CR and LF, preserve everything else"), so I'll allow it. The twoBulkSelectionBarplural tests use exact strings ('1 Dokument ausgewählt','2 Dokumente ausgewählt') — those distinguish the_one/_otherbranches cleanly. A regression that resolved both branches to_otherwould now fail thecount=1test on the singular noun. That's the contract pin I asked for.Felix's data-loss B1 fix is materially the right move —
onMount-once seeding instead ofuntrack-at-top-level — but it landed without any test, and the new Esc-scope guard and the topbar mode-switch copy are also untested. None of these block merge: the deferred work is real and the PR is materially safer than cycle 2. But if any of these slip past #332 they will silently regress. Two of them already show up in the deferral list; three new ones from the cycle-2 patch don't, and should be added.Concerns (should fix before merge or move to #332)
The
onMountseeding inWhoWhenSection.svelte:42-48andDescriptionSection.svelte:42-45is the load-bearing fix for B1 (data-loss on/documents/[id]/edit) and has zero test coverage. The contract is subtle and easy to break by future refactor:dateDisplay/currentTitle/documentLocationfrominitial*props at mount.if (!currentTitle && initialTitle)guard atDescriptionSection.svelte:43).onMountexists rather than$effect).A regression that swaps
onMountfor$effectwould re-seed on every prop tick, blowing away user edits. A regression that drops the!currentTitle &&guard would stomp a parent-bound non-empty value. Neither would be caught by any current test —frontend/src/routes/documents/[id]/edit/page.svelte.spec.tsonly exercises the delete button. The fix is one new spec per component with three tests:This is the same shape as the existing
bulkSelection.svelte.spec.ts— three reasons-to-fail, one behavior each. Add to #332.The new Esc-scope guard in
BulkSelectionBar.svelte:26-30(Leonie B6) has only the happy-path test from cycle 1. The cycle-2 patch addedif (overlay) returnwith a four-selectorquerySelector—dialog[open],[aria-expanded="true"],[role="menu"]:not([hidden]),[role="dialog"]:not([hidden]). Each of those four branches is dead code from a coverage standpoint. The exact regression Leonie's blocker fixes ("selecting docs → opening NotificationBell → Esc closes bell AND wipes selection") cannot be detected byEscape clears the selection while the bar is visible. Need:The
e.defaultPreventedbranch at line 26 is also dead code. Mock the DOM by appending the relevant element todocument.bodybefore dispatching the keydown. Add to #332.The topbar mode-switch in
BulkDocumentEditLayout.svelte:327-340(Elicit C1 fix) has no test. The whole point of the cycle-2 fix is that the topbar now readsm.bulk_edit_topbar_title()andm.bulk_edit_count_pill({count})inmode === 'edit'instead of the upload-flavouredbulk_title_multi/single+bulk_count_pill. A regression that drops the{#if mode === 'edit'}branch would put "werden erstellt" copy back on the bulk-edit page — exactly the bug Elicit blocked the cycle on. The bulk-edit spec covers the dropzone, callout, title display, save, chunking, and discard in edit mode but never reads the topbar text:Add to #332.
@Sizevalidator rejection tests (cycle-2 Concern 5) are still not present and are NOT listed in #332. The cycle-2 reply said "tracked in #332" but the deferral issue doesn't mention them. The validators onDocumentBulkEditDTO(tagNames200×200,receiverIds200, location strings 255) are pure source-code annotations with no behavioural pin. A drop of@Validfrom the controller signature would silently widen the contract. Either add one parametrised test (patchBulk_returns400_when{tagNames|receiverIds|location}_exceeds_size_cap) or add this row to #332 explicitly. Right now the deferral story is incomplete.The auto-clear
$effecttest gap (cycle-2 Concern 2) is correctly deferred but also NOT listed in #332. Same comment as Concern 4 — the deferral issue doesn't mention+layout.svelte's auto-clear$effect. It's a real coverage hole that we agreed to defer — please add the row so it doesn't drop off the radar.Resolved (verified in cycle 2)
Sara C2 (
sanitizeForLog) ✅ —patchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessagesatDocumentControllerTest.java:1180-1202exercises the helper end-to-end via the patchBulk path with an injected DomainException whose message contains\r\n. Three matchers: no\n, no\r, containsevil_. The contract is fully pinned. A regression that switchedreplaceAll("[\\r\\n]", "_")toreplace("\n", "_")would fail the no-\rassertion immediately. Reasonable concession from the four micro-unit tests I originally suggested — going through the controller path also exercises the integration of sanitizeForLog into the BulkEditError JSON serialisation, which is what we actually care about.Sara C6 (plural branches) ✅ —
BulkSelectionBar.svelte.spec.ts:35-50adds two named tests pinningcount=1 → "1 Dokument ausgewählt"andcount=2 → "2 Dokumente ausgewählt". Each is one logical assertion, exact string match (not substring), and the singular form is the load-bearing branch ("1 Dokument" not "1 Dokumente") that I asked for. Translation-key regressions on the_one/_othersplit are now caught.Deferred to #332 (acceptable)
tagOp=ORend-to-end ongetDocumentIds")./documents/bulk-edit— listed in #332.DocumentRowcheckbox-click workaround helper — listed in #332.What I checked
backend/src/test/java/.../controller/DocumentControllerTest.java:1178-1202— verifiedpatchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessagesend-to-end. Mocks the service to throw aDomainExceptionwith\r\n-laced message, asserts three independent properties of the response.frontend/src/lib/components/document/BulkSelectionBar.svelte.spec.ts:35-50— verified both plural tests. Exact-stringtoHaveTextContentcalls, one Svelte-set add per test, distinct singular/plural noun forms.frontend/src/lib/components/document/WhoWhenSection.svelte:42-48— newonMountseeding, untested. No spec file exists for the component.frontend/src/lib/components/document/DescriptionSection.svelte:42-45— newonMountseeding with!currentTitle && initialTitleguard, untested.frontend/src/lib/components/document/BulkSelectionBar.svelte:24-32— new four-selector overlay-detect guard +defaultPreventedearly-out, neither branch tested.frontend/src/lib/components/document/BulkDocumentEditLayout.svelte:327-340— new{#if mode === 'edit'}topbar-copy branches andbulk_edit_count_pillvsbulk_count_pill, neither branch tested.frontend/src/routes/+layout.svelte— new auto-clear$effectfrom cycle 1 still untested (cycle-2 Concern 2 deferred).backend/src/main/java/.../dto/DocumentBulkEditDTO.java—@Sizeannotations still not pinned by tests (cycle-2 Concern 5 deferred).frontend/src/routes/documents/[id]/edit/page.svelte.spec.ts— full file (82 lines). Only delete-button assertions; nothing pins WhoWhenSection / DescriptionSection seeding behaviour at the page level.— Sara
👨💻 Felix Brandt — Senior Fullstack Developer — Cycle 3
Verdict: ⚠️ Approved with concerns
Both my cycle-2 blockers are properly resolved. The B1 regression fix is the right shape —
initial*props restored as opt-in seeds, initialisation moved intoonMountwith a "don't stomp a value the parent already supplied" guard. The B2 fix onDescriptionSectionmirrors the same pattern. Single-doc/documents/[id]/editwill now pre-filldocumentLocationagain, and the bulk-edit consumers stay clean by simply not passing theinitial*props.What stops me approving outright is that none of the five concerns I raised in cycle 2 (C1–C5) were addressed in commit
8ce96294, and none were deferred to #332 either — they just fell through. Plus the explicit guard test I asked for on the regression itself didn't land. None of those are merge-blocking individually, but the C-list is a clean carry-over from cycle 2 that I want to see acknowledged before this lands — either fixed inline or formally moved to #332.Resolved from cycle 2
/documents/[id]/edit— ✅ resolved.WhoWhenSection.svelte:42-48andDescriptionSection.svelte:42-45seed the bindables inonMountwith theif (!currentValue && initialValue)guard.DocumentEditLayoutstill passes theinitial*props on lines 202-211, so the document's saveddocumentLocationandlocationrender on first paint and are no longer overwritten with""on save. The bulk-edit consumer (BulkDocumentEditLayout) doesn't pass them and binds its own state — clean separation.DescriptionSection.svelte:36top-levelcurrentTitle = untrack(...)mutation — ✅ resolved. NowonMount(() => { if (!currentTitle && initialTitle) currentTitle = initialTitle; … }). The Svelte 5 anti-pattern is gone from this file. (Comment on lines 35-40 captures the rationale clearly — that's good.)Carry-over from cycle 2 (not fixed, not deferred to #332)
These are the C1–C5 concerns from comment 4560. They didn't make it into
8ce96294and they aren't in #332. Pick a lane on each — either fix inline (most are 1-2 line changes) or update #332 with a "Felix cycle-2 carry-over" subsection so the audit trail survives the merge.1.
applyBulkEditToDocumentwrites a version row + audit event for no-op DTOs (was cycle-2 C1)DocumentService.java:471-474— every per-document call lands adocumentVersionService.recordVersion(saved)and aMETADATA_UPDATEDaudit row taggedsource=BULK_EDIT, even when every DTO field is null/blank/empty. With 500 docs × 0 actual changes, that's 1000 useless DB writes plus N audit-trail rows pointing at a "change" that didn't happen. Cheap fix is aboolean changedflag flipped inside eachifbranch; persist + audit only whenchanged = true. Or gate at the controller with aBULK_EDIT_NO_CHANGES400. (Confirmed unchanged in the post-cycle-2 file.)2.
BatchMetadataRequestmissing@Validon the controller (was cycle-2 C2)DocumentController.java:327—@RequestBody BatchMetadataRequest requestwith no@Valid. The PATCH twin on line 258 has it. There are nojakarta.validationconstraints today so it's a functional no-op, but adding@Validis the cheap defensive move so a future@Size(max = N)actually fires. One line.3. Auto-clear
$effectre-runs on every checkbox toggle (was cycle-2 C3)+layout.svelte:25-35readsbulkSelectionStore.sizereactively, so the effect fires every time the user ticks/unticks a row on/documents. The only state transition that matters is "leaving a bulk-context route"; gate the size-read inside anuntrack(() => …)block. Not a correctness bug, just churn — but theinBulkContextrecomputation per-tick is exactly the kind of thing that adds up across ~50 selections.4.
BulkDocumentEditLayout.svelte:77-88top-level script mutation offiles+activeId(was cycle-2 C4 — same family as the B2 I just signed off)Top-level
if-mutation runs on every component re-evaluation. Today it's gated bymode === 'edit'(a constant prop in practice) so it accidentally works once, but the pattern is the exact one I flagged inDescriptionSection. Move into a one-shotonMount(consistent with the B2 fix you just shipped) or have the route shape the data structure once and passfilesin as a prop. The fix is the same shape as B2, so the cleanup ought to be uncontroversial.5.
DocumentController.java:275uses fully-qualifiedjava.util.LinkedHashSet(was cycle-2 C5)Add
import java.util.LinkedHashSet;at the top — Markus's cycle-1 #7 explicitly cleaned up the same FQN pattern inDocumentService. Two lines of churn for a clean diff.Test gaps (also from cycle 2, also unaddressed)
6. No regression test for
DocumentEditLayoutpre-fill.I explicitly asked in cycle 2: "add a Vitest browser-mode test on
DocumentEditLayoutthat mounts it withdoc = { documentLocation: 'X', location: 'Y' }and asserts both inputs render those values". The cycle-1 fix went out without this guard, the regression slipped past the green test suite, and now the cycle-2 fix is also untested at the consumer level. There is noDocumentEditLayout.svelte.spec.tsand noDescriptionSection.svelte.spec.ts. The next person who touchesinitial*will repeat exactly the same break.This is the highest-leverage gap in the PR — without it, the B1 fix is only one prop-rename away from regressing again. I'd take this over any of C1–C5.
7. No test for the new edit-mode topbar copy switch.
Elicit C1 fix added
bulk_edit_topbar_titleandbulk_edit_count_pillkeys with amode === 'edit'branch in the layout. Sara'sBulkDocumentEditLayout.svelte.spec.tsdoesn't assert either render path (grep fortopbar,Massenbearbeitung,werden bearbeitet: zero hits). One test mounting the layout inmode="edit"and asserting the topbar text would lock the contract.What I checked this cycle
WhoWhenSection.svelte:39-65—dateDisplay$state+onMountseeding flow, suggested-date$effectwithuntrack(dateDirty). Theseed = dateIso || initialDateIsoorder is correct for the bulk-edit case (parent setsdateIsovia binding from$state('')→ falsy → falls through toinitialDateIso) and the single-doc case (DocumentEditLayout:49seedsdateIsobefore mount → seed picks up the parent value). Clean.DescriptionSection.svelte:42-45—onMountseeding, bothcurrentTitleanddocumentLocationguarded byif (!current && initial). Comment block on lines 35-40 explains the dual-consumer contract. Solid.BulkDocumentEditLayout.svelte:327-340— topbar{#if mode === 'edit'}branches for title + count pill, German plural keys wired correctly (bulk_edit_count_pill({ count: files.size })). Copy switch correct, just untested.+layout.svelte:25-35— auto-clear effect (still has the C3 efficiency footnote).enrich/+page.svelte:4—BulkSelectionBarimport restored, line 106 mounts the bar withcanWrite={canWrite}. Leonie B5 fixed.BulkSelectionBar.svelte:24-32— Esc handler with the newdialog[open], [aria-expanded="true"], [role="menu"]:not([hidden]), [role="dialog"]:not([hidden])overlay check. Solid hardening; thedefaultPreventedshort-circuit on line 26 is a nice extra. New tests atBulkSelectionBar.svelte.spec.ts:35-50pin bothcount=1("1 Dokument") andcount=2("2 Dokumente") plural branches.DocumentControllerTest.java:1178-1202—patchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessagesexercises the CRLF-injection vector with a"evil\r\nFAKE LOG ENTRY: admin logged in"payload. Three assertions (no\n, no\r, containsevil_) locksanitizeForLogin place. Good test.@Valid), C5 (FQNLinkedHashSet) are all unchanged in the post-8ce96294tree.files.setloop) are unchanged.frontend/src/lib/components/document/forDocumentEditLayout*spec*/DescriptionSection*spec*/WhoWhenSection*spec*: zero hits. The B1 regression has no automated guard.Once the C-list is either fixed inline or moved to #332, plus a guard test on
DocumentEditLayoutpre-fill, this is good to land. The two B-blockers from cycle 2 are properly resolved — that work is solid.✅ Cycle 3 fixes pushed (commit
c59287fc)Felix's 5 lingering concerns from cycle 2 + Sara's test-coverage gaps:
BatchMetadataRequestcontroller now uses@Valid.$effectin root+layout.sveltereadsbulkSelectionStore.sizeinsideuntrack()so the effect only re-fires on route change, not on every checkbox toggle.BulkDocumentEditLayoutedit-mode hydration loop now lives insideonMount(matches theWhoWhenSection/DescriptionSectionpattern).java.util.LinkedHashSetwith a top-of-file import.New test coverage (Sara's gaps from cycle 2 + cycle 3):
WhoWhenSection.svelte.spec.ts— 5 tests pinning the cycle-2 onMount seeding fix (initialDateIso pre-fills, parent-bound dateIso wins, hideDate hides the field, editMode hides meta_location, initialLocation pre-fills).DescriptionSection.svelte.spec.ts— 6 tests pinning the same Felix B1/B2 onMount pattern (initialTitle, initialDocumentLocation seeding + non-stomping + editMode-only archive fields).BulkSelectionBar.spec— 2 tests for the new Esc-scope guard (open<dialog>andaria-expandedpopover both block the clear).BulkDocumentEditLayout.spec— 1 test pinning the topbar mode-switch ("Massenbearbeitung" + "werden bearbeitet" in edit mode, NOT the upload-flavoured copy).DocumentControllerTest.patchBulk_returns400_whenArchiveBoxExceeds255Chars— pins the@Size(255)validator onarchiveBoxvia the@Validwiring.234+ backend tests, 60+ frontend bulk-edit specs, all green. The +layout.svelte
$effecttest is intentionally deferred (load-bearing but tricky to test in isolation; tracked in #332).Ready for cycle 4.
🛠️ Tobias Wendt — DevOps & Platform Engineer — Cycle 4
Verdict: ✅ Approved
Cycle-3 commit
c59287fcis exactly the kind of polish I want to see late in a review loop: tightened@Validwiring, an import cleanup, anuntrack()around a hot effect, anonMount-scoped hydration loop, and six new test specs pinning cycle-1/2 behaviour. Zero operational delta since my cycle-3 approval — still ship it.Cycle-3 delta — what changed (operationally)
git diff 8ce96294..c59287fc -- backend/shows a five-line backend touch:DocumentController.javaimport java.util.LinkedHashSet;— replaces the in-line FQN at:276. Pure cosmetics, no bytecode-level behaviour change.batchMetadata(@RequestBody @Valid BatchMetadataRequest request, ...)at:328—@Validnow actually triggers bean-validation on the record. Today the record carries no constraint annotations so the wire behaviour is identical, but future@Size/@NotNulladditions will fire instead of silently passing through. This is a future-prooofing edit, not a runtime change.DocumentControllerTest.java— one new test (patchBulk_returns400_whenArchiveBoxExceeds255Chars) pinning the@Size(max=255)onDocumentBulkEditDTO.archiveBoxvia the@Validwiring on the bulk-edit endpoint. Shifts the request-validation contract from "convention" to "regression-fenced". Good.No
docker-compose*.ymltouched, nopom.xmltouched, no.gitea/workflows/*touched, no Flyway migration added, no new env var, no new container, no new exposed port. Frontend deltas (theuntrack(), theonMountmove, four new.svelte.spec.tsfiles) are zero-impact for ops.Rollback
Still pure
git revert c59287fc 8ce96294 1803db86 …f0da033e. No schema deltas, no infra to roll back, no operational state to drain.Operational risk fence — unchanged from cycle-3
@Transactionalisolation inapplyBulkEditToDocument— still in place.BULK_EDIT_MAX_IDS = 500cap symmetric on/api/documents/bulkand the frontend chunker — still enforced.What I checked
git show --stat c59287fc— 8 files, +179/-9, all inbackend/{controller,test}+frontend/src/{lib/components/document,routes}.git diff 8ce96294..c59287fc -- backend/— the five lines above; nothing operationally meaningful.infra/,.gitea/workflows/,docker-compose*.yml,backend/src/main/resources/db/migration/, orbackend/pom.xml.LGTM. Merge whenever the rest of the personas clear.
— Tobi (@tobiwendt)
🏗️ Markus Keller — Senior Application Architect — Cycle 4
Verdict: ✅ Approved
Cycle-3 commit
c59287fcis exactly the kind of polish I want at the end of a multi-cycle PR — three surgical fixes (C2/C4/C5), one effect-loop tightening (C3), and the test coverage Sara asked for. No architectural surface area changed, no new abstractions introduced, no boundaries crossed. The C1 deferral (skip audit/version when bulk DTO has all blank fields) is correctly scoped as noisy-audit-log cleanup rather than a behaviour gap and tracked in #332.Spot-checks all clean:
+layout.svelte:28-40— theuntrack(() => { if (size > 0) clear(); })wrap is correct. The outer$effectnow depends only onpage.url.pathname(theinBulkContextboolean derives from it), so it re-runs on route change and not on every checkbox toggle. Theuntrackblock both readsbulkSelectionStore.sizeand calls.clear()which writesbulkSelectionStore— both reads and writes are insideuntrack, so even the write-during-clear cannot self-trigger the effect. Effect is now O(routes) instead of O(toggles). Correct fix.BulkDocumentEditLayout.svelte:79-91— moving the edit-mode hydration loop intoonMountties theSvelteMap.set()mutations and theactiveIdwrite to instance lifecycle instead of script-body first-execution. Matches the cycle-2WhoWhenSection/DescriptionSectionpattern, so the codebase now has one canonical "hydrate bindable from initial-* prop" idiom. Theuntrack(() => initialEditEntries)inside the loop is preserved (correct — onMount already isolates from reactivity, but the explicit untrack documents the intent and survives a future refactor that lifts the loop back out of onMount).DocumentController.java:6+:328—LinkedHashSetimport added in alphabetical order (betweenArrayListandList);@Validannotation correctly added toBatchMetadataRequestparameter (jakarta.validation.Validalready imported at line 17). The newpatchBulk_returns400_whenArchiveBoxExceeds255Charstest pins the contract — future@Sizeannotations onBatchMetadataRequestwill now actually fire.The two cycle-3 suggestions I made (ARIA-based Esc-scope fragility, regression test for the Esc-scope guard) were not asked to be addressed in cycle 4 and remain valid for the post-merge follow-up. The new
BulkSelectionBar.svelte.spec.tsEsc-scope tests Felix added in cycle 3 actually cover my second suggestion —dialog[open]andaria-expandedoverlay-blocking branches both have assertions now. The first suggestion (replace ARIA-based selectors with explicitdata-overlay-opencoordination, or invert to a keyboard-stack abstraction) is genuine tech debt for #332 and the right scale to defer.Blockers (must fix before merge)
(none)
Concerns (should fix before merge)
(none)
Suggestions (nice to have, don't block merge)
(none — cycle-3 suggestions still stand for #332 follow-up)
Resolved from cycle 3
@ValidonBatchMetadataRequest) — fixed atDocumentController.java:328. NewarchiveBox255-char regression test pins the wiring.$effectre-fires on every toggle) — fixed at+layout.svelte:35-39.untrackblock contains both the size read and the clear write; effect now depends only onpathname.BulkDocumentEditLayout.svelte:79-91. Now usesonMount, matching the cycle-2 section pattern.java.util.LinkedHashSet) — fixed atDocumentController.java:6(top-of-file import) +:276(use site cleaned up).METADATA_UPDATEDevent per document touched even if the DTO produced a no-op save — slightly noisy but not incorrect.WhoWhenSection,DescriptionSection,BulkSelectionBarEsc-scope,BulkDocumentEditLayouttopbar-mode,archiveBox@Size validator).What I checked this cycle
git show c59287fc --statconfirms 8 files / +179 / -9 lines, 3 production-code edits + 5 test files. All polish; no new logic.+layout.svelte$effectreactivity: confirmeduntrackcorrectly bounds both the read ofbulkSelectionStore.sizeand the call to.clear()(which writes the store), so the effect re-runs only onpage.url.pathnamechange. The cycle-2 implementation at:35would re-fire on every checkbox toggle because.size > 0was a tracked read; the cycle-3 fix is the textbook way to break that cycle.BulkDocumentEditLayout.sveltehydration loop: confirmedonMountruns once per instance, the earlyif (mode !== 'edit') returnguards the upload-mode case, anduntrack(() => initialEditEntries)inside the loop preserves the cycle-1 intent that prop changes after mount must not re-hydrate the SvelteMap (which would clobber user edits in flight).DocumentController.java: confirmedLinkedHashSetimport at line 6 is in alphabetical order in thejava.util.*block;@Validis the correct annotation (already imported at line 17 from cycle-1 work onDocumentBulkEditDTO); thepatchBulk_returns400_whenArchiveBoxExceeds255Charstest exercises the @Size(255) wiring through the @Valid path, so future bean-validation annotations onBatchMetadataRequestare guaranteed to fire.Ship it. Four cycles in, the PR is in better architectural shape than most single-cycle merges I see — clean layering, proper transaction boundaries, parity with the single-doc audit/version trail, explicit caps with typed error codes, and the deferred items routed to a real follow-up issue rather than buried in code comments.
🎨 Leonie Voss — UX Designer & Accessibility Strategist — Cycle 4
Verdict: ✅ Approved — LGTM
Quick re-pass to confirm the cycle-3 follow-up (N1) landed. It did, and then some.
N1 closed — Esc-scope guard is now pinned by tests
frontend/src/lib/components/document/BulkSelectionBar.svelte.spec.ts:90–106asserts that with an open native<dialog open>indocument.body, dispatching Escape leaves the selection at size 2 instead of clearing it. That is exactly the regression I rejected in cycle 2 — now a refactor that breaks the bail-out branch will turn red instead of silently re-introducing the "Esc nukes 47-row selection while ConfirmDialog is up" bug.Bonus: a second case at
:108–121covers the[aria-expanded="true"]branch (NotificationBell, HelpPopover, TagParentPicker pattern). Two of the four selectors inonEscape(dialog[open]and[aria-expanded="true"]) are now pinned. The remaining two ([role="menu"]:not([hidden])and[role="dialog"]:not([hidden])) are not unit-tested individually, but the bail-out logic is identical for all four selectors — if one branch holds, the wholeif (overlay) returnshort-circuit is exercised. Acceptable coverage, no follow-up needed.Production code re-verified
BulkSelectionBar.svelte:24–32—onEscapestill has the four-selector overlay guard, thee.defaultPreventedshort-circuit, and the!visibleearly bail-out. Unchanged from cycle 3 — good.BulkSelectionBar.svelte:35—<svelte:window onkeydown={onEscape} />is the registration site; the spec dispatchesKeyboardEventonwindowwhich is the right wiring for this listener.Deferred to #332 (unchanged from cycle 3, by design)
titleattributefocus-withinring on the row-label wrapperpb-32(128 px) is double the actual bar height (~64 px)What I checked this cycle
BulkSelectionBar.svelte.spec.ts:90–106—<dialog open>overlay → Esc → store size unchanged ✓BulkSelectionBar.svelte.spec.ts:108–121—[aria-expanded="true"]overlay → Esc → store size unchanged ✓BulkSelectionBar.svelte:24–32—onEscapeguard logic intact, four-selector query,defaultPreventedshort-circuit ✓BulkSelectionBar.svelte:35—<svelte:window onkeydown>registration ✓role="note"noaria-labeloverride), B3 (visible Esc hint at ≥sm), B4 (pb-32reservation gated on selection +canWrite) — all still holdThree cycles to ship a calm, accessible, on-brand bulk-edit flow that respects the senior audience and the additive/replace mental model. The Esc-scope guard story is now closed end-to-end: spotted in cycle 2, fixed in cycle 3, pinned by test in cycle 4. Merge with confidence.
— Leonie
📋 Elicit — Requirements Engineer — Cycle 4
Verdict: ✅ Approved
Cycle 3 commit
c59287fcis pure polish + test coverage. No AC-touching code paths altered, no new ambiguity introduced, and one of my own cycle-2 fixes (C1 — topbar mode-switch) is now pinned by an explicit regression test. Every AC from issue #225's Bulk-Edit Panel table remains satisfied. Ship it.Cycle-3 footprint check (no AC drift)
c59287fctouched 8 files: 1 backend controller, 1 backend test, 1 frontend layout, 1 bulk-edit component, 4 frontend spec files. Walked each non-test change:DocumentController.java— Two cosmetic deltas:LinkedHashSetimport cleanup (Felix C5) and adding@ValidtobatchMetadata'sBatchMetadataRequest(Felix C2). The@Validchange is forward-looking;BatchMetadataRequestcarries no constraints today, so behaviour is identical. No AC impact, no new contract surface.+layout.svelte— The auto-clear$effectnow readsbulkSelectionStore.sizeinsideuntrack()(Felix C3). The behaviour is unchanged: selection still clears on route exit from/documents//enrich, just without the per-checkbox-toggle re-fire. The AC "selection cleared only on successful save (or on route exit from bulk context)" remains satisfied; performance is the only thing that improved.BulkDocumentEditLayout.svelte— Edit-mode hydration loop moved from script-body intoonMount(Felix C4). Same loop body, sameentry.idkeying, same SvelteMap mutation, sameactiveIdinitialisation. The only semantic difference is that the seeding is now unambiguously instance-scoped — which actually strengthens the B1 fix from cycle 1 (the field-name mismatch) by tying it tighter to lifecycle. No AC impact.Cycle-3 test additions strengthen the requirements record
This is the part I want to call out positively. Three of the new specs pin behaviours I (or other personas) flagged in earlier cycles as un-tested risks:
BulkDocumentEditLayout.svelte.spec.tsadds a test that explicitly asserts the topbar reads "Massenbearbeitung" / "werden bearbeitet" and does not contain "hochladen" / "werden erstellt" — a direct regression guard for my cycle-2 C1. If anyone reverts the mode-branch in the topbar, CI now fails immediately. This is exactly the right shape.WhoWhenSection.svelte.spec.ts(5 tests) andDescriptionSection.svelte.spec.ts(6 tests) pin the cycle-2 onMount seeding fix that prevents data loss on/documents/[id]/edit— a Felix B1/B2 fix that I noted in cycle 3 also protects the bulk-edit AC "existing values must be preserved". Now both paths have explicit coverage.BulkSelectionBar.svelte.spec.tsadds 2 tests for the Esc-handler scope guard (open<dialog>,aria-expandedpopover) — pins Leonie's B6 fix from cycle 2.DocumentControllerTest.patchBulk_returns400_whenArchiveBoxExceeds255Charsconfirms the@Size(255)validator onarchiveBoxactually fires through the@Validwiring on the bulk PATCH controller — pins the contract-validation surface.Cycle-1 and cycle-2 fixes still intact (re-verified at
c59287fc)BulkEditEntryshape still{ id, title, pdfUrl }matching backendDocumentBatchSummary; hydration loop (now inonMount) still usesentry.idfor both SvelteMap key and innerdocumentId; route cast unchanged.mode === 'edit', clearsbulkSelectionStore, andgoto('/documents').mode === 'edit'and renderbulk_edit_topbar_title/bulk_edit_count_pill. Now backed by an automated regression test./idspermission gate, edit-mode save CTA, visible chunk progress) untouched byc59287fc's diff.Final AC trace (unchanged from cycle 3 — all 13 + the 2 new ACs from cycles 1/2 satisfied)
_one/_otherplural/documents/bulk-editmode="edit"(role="note")PATCH /api/documents/bulkrequires WRITE_ALL; returns{ updated, errors }Carried-over deferrals (acceptable, tracked in #332)
What I checked
c59287fc; reviewed full diff (8 files, +179/-9)BulkDocumentEditLayouttopbar test pins my own cycle-2 C1 fix with a negative-assertion (must NOT contain "hochladen" / "werden erstellt") — the strongest regression-guard shape for an i18n contradictionFrom a requirements perspective, this PR is ready to merge. Three cycles, every persona-raised concern in my scope either fixed, deferred-with-tracking, or pinned by automated test. The cycle-3 polish even improved the requirements record by adding regression coverage for fixes that were previously protected only by manual review.
Approved.
🔐 Nora "NullX" Steiner — Application Security Engineer — Cycle 4
Verdict: ✅ Approved
Cycle 3 is purely defence-hardening on top of an already-clean security posture. Both items I checked land exactly as intended, no new attack surface, no regression on cycles 1–2 hardening. Done from my side — ship it.
Blockers
(none)
Concerns
(none)
What I re-checked (cycle 4)
1.
@ValidonBatchMetadataRequest—DocumentController.java:328— ✅ wiredThe change at line 328 —
@RequestBody @Valid BatchMetadataRequest request— is the right shape. TodayBatchMetadataRequestcarries no bean-validation annotations, so the@Validis a no-op at runtime, but it pre-wires the Jakarta Validation pipeline so the next@Size/@NotNull/@Patternannotation added to the record fires automatically. Without@Valid, those annotations would be silently dead — a textbook "validation control that doesn't actually validate" trap (CWE-20 latent). This is exactly the symmetric posture I want versusDocumentBulkEditDTOat line 258, which has the same@Validwiring already keeping its@Size(max=255)/@Size(max=200)constraints live.The 500-ID controller-level cap at
DocumentController.java:331-334still owns the resource-exhaustion guard (CWE-770) and remains independent of the bean-validation pipeline — so even if someone removes@Validagain, theBULK_EDIT_TOO_MANY_IDSdefence holds. Defence in depth is intact.2.
patchBulk_returns400_whenArchiveBoxExceeds255Chars—DocumentControllerTest.java:999-1014— ✅ exactly the regression fence I wantedThe test pins the
@Size(max=255)validator onarchiveBoxend-to-end:"x".repeat(256)) — the canonical "one over the boundary" CWE-129 test shapemockMvcslice (so the@Validwiring is in the actual code path, not bypassed by a unit-level constructor call)The Javadoc-style comment in the test ("Tobias C2 — without @Valid on @RequestBody this would silently land an arbitrarily long string; the test pins both the annotation and the controller-level @Valid wiring") is exactly the threat-model-in-comment pattern I push for in security-relevant tests. A reviewer 6 months from now sees why the test exists, not just what it asserts. If anyone later removes
@Valid"to clean up", or strips@Sizefrom the DTO, the test fails loud — the cycle-1 S2 hardening is now structurally locked down, not just hopeful.This complements the cycle-2
patchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessagestest (CWE-117 fence). Both my "you have a control, prove it works" asks now have regression coverage.3. Felix C4 —
BulkDocumentEditLayoutonMountmove — ✅ no security impactThe
if (mode === 'edit') { for (const entry of untrack(...)) { files.set(...) } }moves from script-body top-level intoonMount(() => { ... }). Lifecycle correction only — same data, same trust boundary.initialEditEntriesis server-trusted (source:+page.server.tscalling the typed API), interpolation into Svelte bindables continues to auto-escape, no{@html}, noeval. Confirmed no XSS, no DOM clobbering, no prototype pollution introduced.4. Felix C3 —
+layout.svelteuntrackaround bulkSelectionStore — ✅ no security impactWrapping the
bulkSelectionStore.size > 0read and.clear()call inuntrack(() => { ... })is a reactivity-perf fix (effect re-fires only on route change instead of on every checkbox toggle). The selection state is a client-sideSvelteSetof UUIDs — clearing it neither exposes nor leaks data, and there's no auth boundary at this layer (auth lives in the route's+page.server.tsredirect for/documents/bulk-edit). The change reduces noise; it doesn't change the trust model.5. Felix C5 — fully-qualified
LinkedHashSet→ top-of-file import — ✅ stylistic, zero security deltaPure code-style cleanup. Same
LinkedHashSet<UUID>(dto.getDocumentIds())dedupe, same security properties (cap-then-dedupe order intact, audit log still records both raw and unique sizes per cycle 2's audit shape).Cycle-1 carry-overs (status reconfirmed)
/batch-metadatacap (CWE-770)@Valid-wired for future field-level guards/idsupper bound (CWE-770)BulkEditErrorcode (CWE-79 defence-in-depth)@SizeDTO guardsarchiveBox256-char)List<...>endpointsSuggestions (carry-over, no blocker)
DocumentController(BULK_EDIT_MAX_IDS=500,BATCH_METADATA_MAX_IDS=500,BULK_EDIT_FILTER_MAX_IDS=5000) make excellent positive examples for a Semgrep rule. The@Valid-wiring requirement could be a second rule: any@RequestBodywhose type contains@Size/@NotNull/@Patternand the parameter doesn't carry@Valid→ flag. Catches both classes of trap (missing cap, dead validation annotation) at PR-review time.// do not render err.messagecomment inBulkDocumentEditLayout.svelteremains a useful trip-wire. Optional, cosmetic.🤖 Reviewed in character as Nora "NullX" Steiner.
👨💻 Felix Brandt — Senior Fullstack Developer — Cycle 4
Verdict: ✅ Approved
Cycle 3 closed every concern I raised in cycle 2 (C2/C3/C4/C5) cleanly, and the regression tests Sara and I asked for landed with exactly the right shape. C1 (no-op DTO → skip audit + version) is correctly deferred to #332 — it's a noisy-audit-log issue, not a behaviour gap, so deferral is the right call. The B1/B2 onMount-seeding fix is now fenced by two new spec files that pin both branches of the contract (seed-from-initial AND don't-stomp-parent). Single-doc edit pre-fill is safe again, and the bulk-edit consumers stay clean by not passing
initial*. Ship it.Resolved this cycle
Felix C2 —
BatchMetadataRequestmissing@Valid✅ resolved.DocumentController.java:328now readsbatchMetadata(@RequestBody @Valid BatchMetadataRequest request, …). Same shape as the PATCH twin on line 258. A future@Size(max=N)will fire as intended. One-line change, exactly what I asked for.Felix C3 — auto-clear
$effectre-runs on every checkbox toggle ✅ resolved.+layout.svelte:28-40now only trackspage.url.pathname; thebulkSelectionStore.sizeread sits insideuntrack(() => …). The effect fires on route change, not on every tick. Comment block on lines 26-27 documents the intent. Clean fix.Felix C4 —
BulkDocumentEditLayouttop-level hydration loop ✅ resolved.BulkDocumentEditLayout.svelte:79-91— themode === 'edit'hydration is now insideonMount, matching the same lifecycle pattern as the cycle-2 B1/B2 fixes onWhoWhenSection/DescriptionSection. Theif (mode !== 'edit') returnguard clause +untrack(() => initialEditEntries)keeps the read non-reactive. The whole component is now consistent — every initial-state seeding flows throughonMount, no more top-level script-body mutation of reactive state. This is the right cleanup.Felix C5 — fully-qualified
java.util.LinkedHashSet✅ resolved.DocumentController.java:6addsimport java.util.LinkedHashSet;, andDocumentController.java:276is nowLinkedHashSet<UUID> uniqueIds = new LinkedHashSet<>(…). Diff cleaner, matches Markus's cycle-1 #7 cleanup of the same pattern inDocumentService.Felix C1 — no-op DTO writes audit + version row — deferred to #332.
Acceptable. The behaviour is correct; the cost is audit-log noise + 2× DB writes per no-op doc. Worth doing, doesn't block ship.
Test gaps closed this cycle
Both regression fences I asked for in cycle 3 (#6 + #7) plus Sara's matching asks landed:
WhoWhenSection.svelte.spec.ts— 5 tests, exactly the right shape.pre-fills the date input from initialDateIso when the bindable is empty— pins the seed branch.does not stomp a parent-bound dateIso that is already non-empty— pins the guard branch (dateIso || initialDateIso).hides the date field when hideDate=true (bulk-edit mode)— pins the dual-consumer contract.renders the meta_location input only outside editMode— pins the editMode toggle.pre-fills the location input from initialLocation— pins the second seed branch.A future refactor that swaps
onMountfor$effectwould re-seed on prop ticks and break test #2. A refactor that drops theseed = dateIso || initialDateIsoordering would break test #2 the same way. Both load-bearing branches are fenced.DescriptionSection.svelte.spec.ts— 6 tests, mirror coverage.currentTitlefrominitialTitlewhen empty, and the matching test fordocumentLocation/initialDocumentLocation.currentTitle = "Parent Title"wins, and parent-bounddocumentLocation = "Bound Value"wins. Both pin theif (!currentValue && initialValue)guard at lines 43-44.editModetoggle tests forarchiveBox+archiveFoldervisibility.Same shape as the WhoWhenSection spec. The B1/B2 contract is fully fenced now.
BulkDocumentEditLayout.svelte.spec.ts:404-420—topbar reads "Massenbearbeitung" + "{count} werden bearbeitet" in edit mode. Three positive assertions ("Massenbearbeitung", "werden bearbeitet") and two negative assertions (no "hochladen", no "werden erstellt"). Exactly what's needed to lock Elicit's C1 fix. A regression that drops the{#if mode === 'edit'}branch in the topbar would put upload-flavoured copy on the bulk-edit page and fail this test immediately.patchBulk_returns400_whenArchiveBoxExceeds255Chars— pins both the@Size(255)annotation on the DTO and the@Validcontroller wiring. A drop of either would silently re-widen the contract and this test would fail. Sara's cycle-3 Concern 4 closed.I ran the new specs locally to confirm —
WhoWhenSection.svelte.spec.ts(5/5) +DescriptionSection.svelte.spec.ts(6/6) green in 6.2s. Topbar test green in 6.7s.Outstanding (correctly tracked elsewhere)
onMount— already in #332 from cycle 1.DocumentBulkEditDTO@Datavs records — already in #332 from cycle 1.What I checked this cycle
DocumentController.java— re-confirmedimport java.util.LinkedHashSet;(line 6),LinkedHashSet<UUID>use without FQN (line 276),@Validon bothpatchBulk(line 258) andbatchMetadata(line 328).+layout.svelte— the$effectwraps the size-read inuntrack, onlypage.url.pathnamedrives re-runs. Comment block accurately captures intent.BulkDocumentEditLayout.svelte— hydration loop insideonMount, guard clause first,untrack(() => initialEditEntries)keeps the read non-reactive. Whole component consistent with the section-component pattern now.WhoWhenSection.svelte—onMountseeding (lines 42-48) withseed = dateIso || initialDateIsoordering, only writes back todateIsowhen it was empty. Matches the spec.DescriptionSection.svelte—onMountseeding (lines 42-45) withif (!current && initial)guards on bothcurrentTitleanddocumentLocation. Matches the spec.WhoWhenSection.svelte.spec.ts— read all 5 tests, ran locally, 5 passed.DescriptionSection.svelte.spec.ts— read all 6 tests, ran locally, 6 passed.BulkDocumentEditLayout.svelte.spec.ts:404-420— topbar test, 1 passed in isolation.DocumentControllerTest.java:999-1015— archiveBox @Size boundary test, 256-char string → expect 400. Validates both annotation and@Validwiring.Cycle 3 was a clean closure pass. The PR is ready to merge from my perspective.
— Felix
🧪 Sara Holt — QA Engineer & Test Strategist — Cycle 4
Verdict: ✅ Approved
Three of the five gaps I flagged in cycle 3 are now closed with named, focused regression fences. The remaining two are real but bounded and acceptable as deferrals — provided the two missing rows in #332 actually get added (see "Pre-merge ask" below). Both original cycle-1 blockers (B1:
batchMetadata403, B2: duplicate-id semantics) and Markus's audit/version blocker stay resolved. There are no new blockers. This PR is materially safer than cycle 3 and is good to merge.What I want to call out before stamping it: this is the third cycle in a row that ships with the auto-clear
$effecttest gap (cycle-2 C2) untested AND missing from #332. We agreed twice to defer it, twice it's slipped off the deferral list. That pattern itself is the risk — not the missing test. Same story for the five non-archiveBox@Sizevalidator branches. Add the rows.Cycle-3 verification
Resolved
C1 —
WhoWhenSection/DescriptionSectiononMount seeding (B1 data-loss fix regression fence) ✅ partiallyWhoWhenSection.svelte.spec.ts(5 tests): pre-fill frominitialDateIso, non-stomp of parent-bounddateIso,hideDate=trueremoves the field,editModetogglesmeta_location,initialLocationpre-fill. All clean Arrange-Act-Assert, exact-string matches on.value, one reason-to-fail per test.DescriptionSection.svelte.spec.ts(6 tests): same pattern acrossinitialTitle/currentTitle/initialDocumentLocation/documentLocation, pluseditModetrue/false visibility for the archive fields viadata-testid. Both bindable-non-stomp paths pinned.initialDateIso(orinitialTitle/initialDocumentLocation) changes after mount." That's the regression Felix'sonMount-vs-$effectchoice actually defends against. A future refactor that swapsonMountfor$effect(() => { if (!currentTitle && initialTitle) currentTitle = initialTitle; })would still pass every cycle-3 test (the!currentTitleguard catches the seed case) but would re-seed on every parent prop tick that arrives after the user has cleared the field — silent data loss on the same field that motivated B1. One test per component (rerender with a differentinitial*prop, assert the bindable still equals the user-edited value) would close this. Acceptable as cycle-5 follow-up; please add to #332 explicitly.C3 — Topbar mode-switch in
BulkDocumentEditLayout✅BulkDocumentEditLayout.svelte.spec.ts:404-420— pinsMassenbearbeitung+werden bearbeitetAND asserts the absence of upload-flavouredhochladen/werden erstellt. Three reasons-to-fail in one test, all facets of the samemode === 'edit'contract — same allowance I made forpatchBulk_stripsCarriageReturnsAndNewlinesFromErrorMessageslast cycle. The Elicit blocker (upload copy bleeding into bulk-edit) cannot regress silently.container.querySelector('span.font-bold.text-ink')and'span.bg-accent'are class-coupled selectors. A Tailwind palette rename would break the test on a non-behavioural change. Preferdata-testid="bulk-topbar-title"/"bulk-topbar-pill"next time the file is touched. Not blocking.@SizearchiveBox validator ✅ —patchBulk_returns400_whenArchiveBoxExceeds255CharsatDocumentControllerTest.java:999-1015. Sends a 256-chararchiveBoxand asserts400. This proves the@RequestBody @Valid DocumentBulkEditDTOwiring atDocumentController.java:259is live, which transitively defends every other@Sizeannotation on the DTO from being silently bypassed by a@Valid-removal regression. Good economy: one test fences six annotations against the most common regression mode.Partially resolved
BulkSelectionBar⚠️ — Two of four selector branches now tested.BulkSelectionBar.svelte.spec.ts:90-106coversdialog[open]. ✅BulkSelectionBar.svelte.spec.ts:108-121covers[aria-expanded="true"]. ✅[role="menu"]:not([hidden])(closes a custom-popover regression — exactly the kind of thing a kebab/overflow-menu author might rely on) and[role="dialog"]:not([hidden])(closes the case where a non-<dialog>modal — like a custom Tailwind sheet — is open). Also untested: thee.defaultPreventedearly-out at line 26.aria-expandedtriggers — Headless-UI menus, NotificationBell, etc.). The remaining branches are dead code from a coverage standpoint but lower-likelihood regression vectors. Acceptable as a follow-up — please add to #332.Still deferred (acceptable)
C4 — non-
archiveBox@Sizevalidator branches. ThearchiveBoxtest proves the@Validwiring is live, which materially de-risks all five other annotations (tagNames200×200,receiverIds200,documentLocation255,archiveFolder255). The residual risk is a single-annotation-drop regression on one of the other five fields, which would slip silently. One parametrized JUnit test would pin all five — but the wiring proof gets us to "good enough" for merge. Please add a row to #332 covering the remaining five.C5 —
+layout.svelteauto-clear$effect. Cycle 3 fixed the behaviour (untrack(() => { if (size > 0) clear() })so the effect only fires on route change, not on every checkbox toggle) but added no test. Three cycles in, this is still untested AND still not listed in #332. The behaviour is genuinely tricky to test in isolation (page navigation harness invitest-browser-svelte), which is why I keep accepting the deferral. But "load-bearing untested behaviour with no tracking issue row" is a CI-trust anti-pattern. Pre-merge ask: add the row to #332.Pre-merge ask (not blocking; please do before clicking merge)
Two
## Testsrows to add to issue #332:+layout.svelteauto-clear$effecttest — three to five tests in a newfrontend/src/routes/+layout.svelte.spec.tsmatrix: keeps selection on/documents/<id>, keeps on/enrich/<x>, clears on/persons, clears on/admin. Frame:vitest-browser-sveltewith apage.url.pathnameharness — same shape as the existing route guards.Non-
archiveBox@Sizevalidator rejection tests — one parametrized@MethodSourcetest inDocumentControllerTestcoveringtagNames201 entries,tagNameselement 256 chars,receiverIds201,documentLocation256,archiveFolder256. All assert400. Five branches, one test method.Optional but cheap: the "no re-seed on prop change after mount" companion test for
WhoWhenSectionandDescriptionSection(cycle-3 C1 third bullet). Two tests, ~12 lines.What I checked
frontend/src/lib/components/document/WhoWhenSection.svelte.spec.ts— full file (42 lines). Five tests verified by name and assertion shape. Confirmed missing third branch (no-re-seed on prop change).frontend/src/lib/components/document/DescriptionSection.svelte.spec.ts— full file (50 lines). Six tests verified, same pattern. Confirmed missing no-re-seed branch on bothcurrentTitleanddocumentLocation.frontend/src/lib/components/document/BulkSelectionBar.svelte.spec.ts:90-121— verified the two new Esc-scope guard tests. Both usetry/finallyto clean up the appended overlay element — that's the right hygiene; a leaked<dialog open>would cascade-fail every subsequent Esc test in the file. Cross-checked againstBulkSelectionBar.svelte:24-32to confirm the four-selector /defaultPreventedguard. Two of four branches and thedefaultPreventedearly-out remain uncovered.frontend/src/lib/components/document/BulkDocumentEditLayout.svelte.spec.ts:404-420— verified the topbar mode-switch test. Both positive (Massenbearbeitung,werden bearbeitet) and negative (hochladen,werden erstellt) assertions present. Class-coupled selectors flagged but not blocking.backend/src/test/java/.../controller/DocumentControllerTest.java:999-1015— verifiedpatchBulk_returns400_whenArchiveBoxExceeds255Chars. Confirms@Validwiring on@RequestBody DocumentBulkEditDTOatDocumentController.java:259.backend/src/main/java/.../dto/DocumentBulkEditDTO.java:44-58— re-checked the six@Sizeannotations. Five remain unpinned by individual tests (covered transitively by the wiring proof).backend/src/main/java/.../controller/DocumentController.java:259, 328— confirmed@RequestBody @Validon bothpatchBulk(DocumentBulkEditDTO) andbatchMetadata(BatchMetadataRequest).frontend/src/routes/+layout.svelte:25-37— re-verified the cycle-3untrack-wrapped$effect. No spec file exists atfrontend/src/routes/+layout.svelte.spec.ts.@Sizenon-archiveBox branches) and C5 (+layout.svelte$effect) still missing despite being raised in cycles 2 and 3.— Sara
marcel referenced this pull request2026-04-25 19:25:45 +02:00
✅ Cycle 4 — all 7 personas approved
4 cycles, every blocker resolved
$effecttest matrix + parametrized@Sizerejection tests). Already added to #332.Final state
feat/issue-225-bulk-metadata-edit(b690c74d..c59287fc)Ready to merge.