marcel/familienarchiv
1
0
Fork 0
Code Issues 117 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

refactor: bulk-edit follow-ups deferred from PR #331 #332

New Issue
Open
opened 2026-04-25 17:00:44 +02:00 by marcel · 9 comments
marcel commented 2026-04-25 17:00:44 +02:00
Owner
Copy Link

Context

Deferred work from the cycle-1 multi-persona review on PR #331. Items are ordered Must → Should → Could. Tackle Musts before Shoulds. Each item is a standalone, atomic change — implement in a separate commit.

Note: pluralization branch tests for BulkSelectionBar (count=1 / count=2) were found to already exist in BulkSelectionBar.svelte.spec.ts lines 35–49 and are removed from this list.

MUST

M1 — Structured error code in BulkEditError

File: backend/src/main/java/org/raddatz/familienarchiv/dto/BulkEditError.java

Current message: String field is a free-form string that round-trips user-controlled data through the response body. The frontend also cannot localize it.

Changes:

  1. Rename field message → code, change type from String to ErrorCode: 
    public record BulkEditError(
    @Schema(requiredMode = Schema.RequiredMode.REQUIRED) UUID id,
    @Schema(requiredMode = Schema.RequiredMode.REQUIRED) ErrorCode code) {}
  2. Update all new BulkEditError(id, message) call sites to pass an ErrorCode value.
  3. Add a new ErrorCode entry if needed (e.g. BULK_EDIT_DOCUMENT_FAILED).
  4. Mirror in frontend/src/lib/errors.ts and add a translation key in messages/de.json, en.json, es.json.
  5. Regenerate TS types: npm run generate:api.

Done when: grep -r "BulkEditError" backend/ shows no String message field; the frontend renders a localized string when a per-document failure occurs.

M2 — WCAG focus ring on DocumentRow bulk-select checkbox

File: frontend/src/lib/components/DocumentRow.svelte

The <label data-testid="bulk-select-checkbox"> is the 44×44 hit target, but focus-within:ring-2 is not on it — only the inner 20×20 <input> gets the ring. Violates WCAG 2.1 AA SC 2.4.7.

Change: Add focus-within:ring-2 focus-within:ring-brand-mint focus-within:ring-offset-1 to the <label> wrapping the checkbox input.

Done when: Keyboard tab to the checkbox shows the ring on the full label area; DocumentRow.svelte.spec.ts has a test asserting focus-within is present on the label wrapper.

M3 — Guard handleDiscard against mid-save race

File: frontend/src/lib/components/document/BulkDocumentEditLayout.svelte (line ~130 handleDiscard, saving state at line 60)

handleDiscard has no guard against running while saving is true. handleSave (line 284) has if (saving) return; but handleDiscard does not.

Decision required: Is mid-save discard intentional?

  • If NO (bug): add if (saving) return; at top of handleDiscard.
  • If YES (intentional): add a brief comment and pin it in a test.

Done when: A test in BulkDocumentEditLayout.svelte.spec.ts covers the mid-save discard path and asserts the intended behaviour. The code either has a guard or a comment making intent explicit.

M4 — @SpringBootTest integration test for applyBulkEditToDocument transactional boundary

File to create: backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceBulkEditIntegrationTest.java

Method under test: DocumentService.applyBulkEditToDocument (line 443 in DocumentService.java)

The claim "a per-document failure cannot partially mutate other documents in the batch" is unverifiable by the existing Mockito unit suite because Mockito cannot exercise the Spring @Transactional proxy boundary.

Test scenario (use Testcontainers PostgreSQL — same setup as any existing @SpringBootTest):

  1. Persist three documents: doc1, doc2, doc3.
  2. Build a DocumentBulkEditDTO with senderId = a UUID that does not exist in the DB (forces a DomainException on doc2).
  3. Call patchBulk with IDs [doc1.id, doc2.id, doc3.id].
  4. Assert:
    • doc1 and doc3 have the new sender in the DB.
    • doc2 is unchanged in the DB.
    • Response body contains exactly one BulkEditError with id = doc2.id.

Done when: Test passes against a real PostgreSQL container and runs as part of ./mvnw test.

M5 — Round-trip ID preservation in chunking test

File: frontend/src/lib/components/document/BulkDocumentEditLayout.svelte.spec.ts

Test to update: Find the test for chunked saving with 1100 IDs (search for 1100 or chunk in the file).

Current test asserts call counts only. A regression dropping or duplicating IDs would pass silently.

Change: After asserting call counts, also assert the union of all chunk IDs equals the original input exactly:

const allSentIds = fetchCalls.flatMap(call => extractIdsFromCall(call));
expect(allSentIds.length).toBe(1100);               // no drop, no duplicate
expect(new Set(allSentIds).size).toBe(1100);

Done when: The test fails if any chunk sends a different set of IDs than the input.

M6 — +layout.svelte auto-clear $effect test matrix

File: frontend/src/routes/layout.svelte.spec.ts
Effect under test: Lines 28–40 in frontend/src/routes/+layout.svelte

The auto-clear effect has zero test coverage. A Svelte reactivity regression silently breaks it.

Add these 5 test cases:

  • (a) Navigate /documents → /persons: store is cleared.
  • (b) Navigate /enrich → /persons: store is cleared.
  • (c) Navigate /documents → /documents/123: store is NOT cleared.
  • (d) Navigate /documents → /documents/bulk-edit: store is NOT cleared.
  • (e) Toggle bulkSelectionStore (add/remove an ID) while on /documents: the clear does NOT fire (the untrack guarantee from line 36).

Done when: All 5 cases pass; a comment in the test references lines 28–40 in +layout.svelte.

SHOULD

S1 — Pre-resolve tags/sender/receivers once per bulk-edit request (N+1 fix)

File: backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java

tagService.findOrCreate() is called inside applyBulkEditToDocument, which is called per document. 500 docs × 10 tags = ~5000 tag-lookup queries per request.

Change: In the method that loops over document IDs (search for the loop calling applyBulkEditToDocument), resolve tags, sender, and receivers once before the loop. Pass resolved objects into applyBulkEditToDocument (update signature or add overload).

Done when: A unit test asserts tagService.findOrCreate is called exactly dto.getTagNames().size() times regardless of document count.

S2 — Add RateLimitInterceptor to PATCH /api/documents/bulk

Files:

  • backend/src/main/java/org/raddatz/familienarchiv/config/WebConfig.java (lines 12–13)
  • backend/src/test/java/org/raddatz/familienarchiv/config/RateLimitInterceptorTest.java

Confirmed: interceptor currently only covers /api/auth/invite/** and /api/auth/register. PATCH /api/documents/bulk can process 500 documents per call with no rate cap.

Change:

registry.addInterceptor(new RateLimitInterceptor())
    .addPathPatterns("/api/auth/invite/**", "/api/auth/register", "/api/documents/bulk");

Also add a test in RateLimitInterceptorTest.java asserting HTTP 429 after 10 calls on /api/documents/bulk.

Done when: The test passes; WebConfig.java includes the bulk path.

S3 — Move /documents/bulk-edit data fetch from onMount to server load

Files:

  • frontend/src/routes/documents/bulk-edit/+page.svelte (remove onMount at lines 2 and 15–16)
  • Create/update: frontend/src/routes/documents/bulk-edit/+page.server.ts

The onMount reads bulkSelectionStore.ids and calls the batch-metadata endpoint client-side, causing a loading flash and breaking the project rule that data flows from server load via props.

Challenge: The store is client-side state. One approach: persist selected IDs into a server-side session (cookie) before navigating, so the server load can read them. Confirm the approach before implementing.

Done when: Page renders with document data on first paint; onMount data-fetch is removed from +page.svelte; empty-store redirect is in the server load.

S4 — Responsive stacking for BulkDocumentEditLayout below 768px

File: frontend/src/lib/components/document/BulkDocumentEditLayout.svelte

The 55/45 panel split renders unusably narrow on phones. The Massenbearbeitung CTA in BulkSelectionBar is reachable from mobile.

Two options (choose one):

  • A (stack panels): Replace side-by-side with flex-col below md:. PDF preview on top, form below.
  • B (gate CTA): Hide the Massenbearbeitung button in BulkSelectionBar.svelte below md: breakpoint. Simpler but removes mobile access entirely.

Done when: At 375px viewport width, the chosen option is implemented and the page is fully usable (no horizontal overflow, no panel narrower than the viewport).

S5 — Extend accessibility.spec.ts to cover /documents/bulk-edit

File: frontend/e2e/accessibility.spec.ts

New page with role="note" callouts, sticky bars, and focus-managed checkboxes — high a11y-regression risk.

Change: Add a test that logs in, selects 2+ documents, navigates to /documents/bulk-edit, and runs checkA11y(). Assert zero critical/serious axe violations.

Done when: Test passes; any violations found during implementation are fixed or explicitly suppressed with a comment.

S6 — Cover tagOp=OR end-to-end in getDocumentIds controller test

File: Find the test containing getDocumentIds tests — likely backend/src/test/java/org/raddatz/familienarchiv/controller/DocumentControllerTest.java

Current test only forwards senderId; tagOp is untested.

Change: Add a test case passing tagOp=OR and verifying TagOperator.OR is forwarded to DocumentService.getDocumentIds. Add a case for the default (tagOp omitted → TagOperator.AND).

S7 — Snapshot save-flow metadata across chunks

File: frontend/src/lib/components/document/BulkDocumentEditLayout.svelte.spec.ts

Chunked-save test counts calls but doesn't verify FormData content. A regression resetting senderId or tagNames between chunks would pass silently.

Change: Capture the FormData body of each chunk call and assert senderId and tagNames are identical across all chunks.

S8 — ADR for bulk-edit additive-vs-replace semantics

File to create: docs/adr/ — increment from the last ADR number in that directory.

Lift the semantics documented in the DocumentBulkEditDTO Javadoc (lines 11–19) into a standalone ADR:

  • Tags + receivers: additive (merged, never replaced)
  • Sender, location fields: replace-on-non-blank (null/blank = skip)
  • Rationale: additive prevents accidentally clearing partially-reviewed documents; replace-on-non-blank mirrors single-document edit UX.

After writing the ADR, add a @see reference to it in the DocumentBulkEditDTO Javadoc.

S9 — Parametrize @Size validator rejection tests on DocumentBulkEditDTO

File: Find the test containing patchBulk_returns400_whenArchiveBoxExceeds255Chars — search in backend/src/test/.

Replace individual test methods with a @ParameterizedTest @CsvSource covering all 6 @Size constraints:

Field Limit Over-limit value
tagNames list length 200 201 entries
tagNames item length 200 chars 201-char string
receiverIds list length 200 201 UUIDs
documentLocation 255 chars 256-char string
archiveBox 255 chars 256-char string
archiveFolder 255 chars 256-char string

Each row must assert HTTP 400.

COULD

C1 — Rename BatchMetadataRequest → BulkEditIdsRequest, DocumentBatchSummary → BulkEditDocumentSummary

Files:

  • backend/src/main/java/org/raddatz/familienarchiv/dto/BatchMetadataRequest.java
  • backend/src/main/java/org/raddatz/familienarchiv/dto/DocumentBatchSummary.java
  • All references in controller, service, tests
  • Regenerate TS types after rename

Done when: grep -r "BatchMetadataRequest\|DocumentBatchSummary" backend/ returns zero results.

C2 — Refactor DocumentBulkEditDTO from @Data to record

File: backend/src/main/java/org/raddatz/familienarchiv/dto/DocumentBulkEditDTO.java

Blocker: Tests that mutate via setters must first be rewritten to use constructor args (see Javadoc lines 23–26). Do S9 first — it touches the same tests.

Done when: Class is a record; all @Size annotations preserved on record components; @Data, @NoArgsConstructor, @AllArgsConstructor removed.

C3 — Split BulkDocumentEditLayout.svelte into upload and edit components

File: frontend/src/lib/components/document/BulkDocumentEditLayout.svelte (~520 lines)

Extract:

  • BulkUploadLayout.svelte — upload mode (mode='upload' branch)
  • BulkEditLayout.svelte — edit mode (mode='edit' branch)
  • chunkAndSave() helper — shared chunking concern
  • BulkPdfPreview / FileSwitcherStrip — shared sub-components

Done when: The mode flag is no longer needed inside any single component; existing BulkDocumentEditLayout.svelte.spec.ts tests pass against the new structure.

C4 — Add tooltip to FieldLabelBadge

File: frontend/src/lib/components/document/FieldLabelBadge.svelte

Add a title attribute explaining the behaviour on hover:

  • additive: title="Wird zu bestehenden Werten hinzugefügt"
  • replace: title="Überschreibt bestehende Werte, wenn ausgefüllt"

Done when: Both variants have title; FieldLabelBadge.svelte.spec.ts asserts the value for each.

C5 — Pin checkbox-click workaround in DocumentRow.svelte.spec.ts into one helper

File: frontend/src/lib/components/DocumentRow.svelte.spec.ts

The Tailwind-in-browser checkbox-click workaround is duplicated across ~5 test cases. Extract into a single clickCheckbox(element) helper.

Done when: The workaround logic appears exactly once in the file.

C6 — Cover remaining Esc-scope guard selectors in BulkSelectionBar.svelte.spec.ts

File: frontend/src/lib/components/document/BulkSelectionBar.svelte.spec.ts

onEscape checks 4 selectors (lines 24–31 in BulkSelectionBar.svelte). Current tests cover selectors 1 and 2 only:

  1. dialog[open] ✅ line 90
  2. [aria-expanded="true"] ✅ line 108
  3. [role="menu"]:not([hidden]) ❌ missing
  4. [role="dialog"]:not([hidden]) ❌ missing

Also missing: e.defaultPrevented short-circuit (line 26 in BulkSelectionBar.svelte).

Add 3 test cases: Escape does not clear when selector 3 matches; does not clear when selector 4 matches; does not clear when e.defaultPrevented is true.

WON'T (this cycle)

  • Add to issue #225's Out of Scope register — verify #225 is still open before acting. If closed, include the rollback/versioning note in the S8 ADR instead.
## Context Deferred work from the cycle-1 multi-persona review on PR #331. Items are ordered Must → Should → Could. Tackle Musts before Shoulds. Each item is a standalone, atomic change — implement in a separate commit. > Note: pluralization branch tests for `BulkSelectionBar` (`count=1` / `count=2`) were found to already exist in `BulkSelectionBar.svelte.spec.ts` lines 35–49 and are removed from this list. --- ## MUST ### M1 — Structured error code in `BulkEditError` **File:** `backend/src/main/java/org/raddatz/familienarchiv/dto/BulkEditError.java` Current `message: String` field is a free-form string that round-trips user-controlled data through the response body. The frontend also cannot localize it. **Changes:** 1. Rename field `message` → `code`, change type from `String` to `ErrorCode`: ```java public record BulkEditError( @Schema(requiredMode = Schema.RequiredMode.REQUIRED) UUID id, @Schema(requiredMode = Schema.RequiredMode.REQUIRED) ErrorCode code) {} ``` 2. Update all `new BulkEditError(id, message)` call sites to pass an `ErrorCode` value. 3. Add a new `ErrorCode` entry if needed (e.g. `BULK_EDIT_DOCUMENT_FAILED`). 4. Mirror in `frontend/src/lib/errors.ts` and add a translation key in `messages/de.json`, `en.json`, `es.json`. 5. Regenerate TS types: `npm run generate:api`. **Done when:** `grep -r "BulkEditError" backend/` shows no `String message` field; the frontend renders a localized string when a per-document failure occurs. --- ### M2 — WCAG focus ring on `DocumentRow` bulk-select checkbox **File:** `frontend/src/lib/components/DocumentRow.svelte` The `<label data-testid="bulk-select-checkbox">` is the 44×44 hit target, but `focus-within:ring-2` is not on it — only the inner 20×20 `<input>` gets the ring. Violates WCAG 2.1 AA SC 2.4.7. **Change:** Add `focus-within:ring-2 focus-within:ring-brand-mint focus-within:ring-offset-1` to the `<label>` wrapping the checkbox input. **Done when:** Keyboard tab to the checkbox shows the ring on the full label area; `DocumentRow.svelte.spec.ts` has a test asserting `focus-within` is present on the label wrapper. --- ### M3 — Guard `handleDiscard` against mid-save race **File:** `frontend/src/lib/components/document/BulkDocumentEditLayout.svelte` (line ~130 `handleDiscard`, `saving` state at line 60) `handleDiscard` has no guard against running while `saving` is `true`. `handleSave` (line 284) has `if (saving) return;` but `handleDiscard` does not. **Decision required:** Is mid-save discard intentional? - **If NO (bug):** add `if (saving) return;` at top of `handleDiscard`. - **If YES (intentional):** add a brief comment and pin it in a test. **Done when:** A test in `BulkDocumentEditLayout.svelte.spec.ts` covers the mid-save discard path and asserts the intended behaviour. The code either has a guard or a comment making intent explicit. --- ### M4 — `@SpringBootTest` integration test for `applyBulkEditToDocument` transactional boundary **File to create:** `backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceBulkEditIntegrationTest.java` **Method under test:** `DocumentService.applyBulkEditToDocument` (line 443 in `DocumentService.java`) The claim "a per-document failure cannot partially mutate other documents in the batch" is unverifiable by the existing Mockito unit suite because Mockito cannot exercise the Spring `@Transactional` proxy boundary. **Test scenario (use Testcontainers PostgreSQL — same setup as any existing `@SpringBootTest`):** 1. Persist three documents: doc1, doc2, doc3. 2. Build a `DocumentBulkEditDTO` with `senderId` = a UUID that does not exist in the DB (forces a `DomainException` on doc2). 3. Call `patchBulk` with IDs `[doc1.id, doc2.id, doc3.id]`. 4. Assert: - doc1 and doc3 have the new sender in the DB. - doc2 is unchanged in the DB. - Response body contains exactly one `BulkEditError` with `id = doc2.id`. **Done when:** Test passes against a real PostgreSQL container and runs as part of `./mvnw test`. --- ### M5 — Round-trip ID preservation in chunking test **File:** `frontend/src/lib/components/document/BulkDocumentEditLayout.svelte.spec.ts` **Test to update:** Find the test for chunked saving with 1100 IDs (search for `1100` or `chunk` in the file). Current test asserts call counts only. A regression dropping or duplicating IDs would pass silently. **Change:** After asserting call counts, also assert the union of all chunk IDs equals the original input exactly: ```typescript const allSentIds = fetchCalls.flatMap(call => extractIdsFromCall(call)); expect(allSentIds.length).toBe(1100); // no drop, no duplicate expect(new Set(allSentIds).size).toBe(1100); ``` **Done when:** The test fails if any chunk sends a different set of IDs than the input. --- ### M6 — `+layout.svelte` auto-clear `$effect` test matrix **File:** `frontend/src/routes/layout.svelte.spec.ts` **Effect under test:** Lines 28–40 in `frontend/src/routes/+layout.svelte` The auto-clear effect has zero test coverage. A Svelte reactivity regression silently breaks it. **Add these 5 test cases:** - (a) Navigate `/documents` → `/persons`: store is cleared. - (b) Navigate `/enrich` → `/persons`: store is cleared. - (c) Navigate `/documents` → `/documents/123`: store is NOT cleared. - (d) Navigate `/documents` → `/documents/bulk-edit`: store is NOT cleared. - (e) Toggle `bulkSelectionStore` (add/remove an ID) while on `/documents`: the clear does NOT fire (the `untrack` guarantee from line 36). **Done when:** All 5 cases pass; a comment in the test references lines 28–40 in `+layout.svelte`. --- ## SHOULD ### S1 — Pre-resolve tags/sender/receivers once per bulk-edit request (N+1 fix) **File:** `backend/src/main/java/org/raddatz/familienarchiv/service/DocumentService.java` `tagService.findOrCreate()` is called inside `applyBulkEditToDocument`, which is called per document. 500 docs × 10 tags = ~5000 tag-lookup queries per request. **Change:** In the method that loops over document IDs (search for the loop calling `applyBulkEditToDocument`), resolve tags, sender, and receivers once before the loop. Pass resolved objects into `applyBulkEditToDocument` (update signature or add overload). **Done when:** A unit test asserts `tagService.findOrCreate` is called exactly `dto.getTagNames().size()` times regardless of document count. --- ### S2 — Add `RateLimitInterceptor` to `PATCH /api/documents/bulk` **Files:** - `backend/src/main/java/org/raddatz/familienarchiv/config/WebConfig.java` (lines 12–13) - `backend/src/test/java/org/raddatz/familienarchiv/config/RateLimitInterceptorTest.java` Confirmed: interceptor currently only covers `/api/auth/invite/**` and `/api/auth/register`. `PATCH /api/documents/bulk` can process 500 documents per call with no rate cap. **Change:** ```java registry.addInterceptor(new RateLimitInterceptor()) .addPathPatterns("/api/auth/invite/**", "/api/auth/register", "/api/documents/bulk"); ``` Also add a test in `RateLimitInterceptorTest.java` asserting HTTP 429 after 10 calls on `/api/documents/bulk`. **Done when:** The test passes; `WebConfig.java` includes the bulk path. --- ### S3 — Move `/documents/bulk-edit` data fetch from `onMount` to server load **Files:** - `frontend/src/routes/documents/bulk-edit/+page.svelte` (remove `onMount` at lines 2 and 15–16) - Create/update: `frontend/src/routes/documents/bulk-edit/+page.server.ts` The `onMount` reads `bulkSelectionStore.ids` and calls the batch-metadata endpoint client-side, causing a loading flash and breaking the project rule that data flows from server load via props. **Challenge:** The store is client-side state. One approach: persist selected IDs into a server-side session (cookie) before navigating, so the server load can read them. Confirm the approach before implementing. **Done when:** Page renders with document data on first paint; `onMount` data-fetch is removed from `+page.svelte`; empty-store redirect is in the server load. --- ### S4 — Responsive stacking for `BulkDocumentEditLayout` below 768px **File:** `frontend/src/lib/components/document/BulkDocumentEditLayout.svelte` The 55/45 panel split renders unusably narrow on phones. The `Massenbearbeitung` CTA in `BulkSelectionBar` is reachable from mobile. **Two options (choose one):** - **A (stack panels):** Replace side-by-side with `flex-col` below `md:`. PDF preview on top, form below. - **B (gate CTA):** Hide the `Massenbearbeitung` button in `BulkSelectionBar.svelte` below `md:` breakpoint. Simpler but removes mobile access entirely. **Done when:** At 375px viewport width, the chosen option is implemented and the page is fully usable (no horizontal overflow, no panel narrower than the viewport). --- ### S5 — Extend `accessibility.spec.ts` to cover `/documents/bulk-edit` **File:** `frontend/e2e/accessibility.spec.ts` New page with `role="note"` callouts, sticky bars, and focus-managed checkboxes — high a11y-regression risk. **Change:** Add a test that logs in, selects 2+ documents, navigates to `/documents/bulk-edit`, and runs `checkA11y()`. Assert zero critical/serious axe violations. **Done when:** Test passes; any violations found during implementation are fixed or explicitly suppressed with a comment. --- ### S6 — Cover `tagOp=OR` end-to-end in `getDocumentIds` controller test **File:** Find the test containing `getDocumentIds` tests — likely `backend/src/test/java/org/raddatz/familienarchiv/controller/DocumentControllerTest.java` Current test only forwards `senderId`; `tagOp` is untested. **Change:** Add a test case passing `tagOp=OR` and verifying `TagOperator.OR` is forwarded to `DocumentService.getDocumentIds`. Add a case for the default (`tagOp` omitted → `TagOperator.AND`). --- ### S7 — Snapshot save-flow metadata across chunks **File:** `frontend/src/lib/components/document/BulkDocumentEditLayout.svelte.spec.ts` Chunked-save test counts calls but doesn't verify `FormData` content. A regression resetting `senderId` or `tagNames` between chunks would pass silently. **Change:** Capture the `FormData` body of each chunk call and assert `senderId` and `tagNames` are identical across all chunks. --- ### S8 — ADR for bulk-edit additive-vs-replace semantics **File to create:** `docs/adr/` — increment from the last ADR number in that directory. Lift the semantics documented in the `DocumentBulkEditDTO` Javadoc (lines 11–19) into a standalone ADR: - Tags + receivers: additive (merged, never replaced) - Sender, location fields: replace-on-non-blank (null/blank = skip) - Rationale: additive prevents accidentally clearing partially-reviewed documents; replace-on-non-blank mirrors single-document edit UX. After writing the ADR, add a `@see` reference to it in the `DocumentBulkEditDTO` Javadoc. --- ### S9 — Parametrize `@Size` validator rejection tests on `DocumentBulkEditDTO` **File:** Find the test containing `patchBulk_returns400_whenArchiveBoxExceeds255Chars` — search in `backend/src/test/`. Replace individual test methods with a `@ParameterizedTest @CsvSource` covering all 6 `@Size` constraints: | Field | Limit | Over-limit value | |---|---|---| | `tagNames` list length | 200 | 201 entries | | `tagNames` item length | 200 chars | 201-char string | | `receiverIds` list length | 200 | 201 UUIDs | | `documentLocation` | 255 chars | 256-char string | | `archiveBox` | 255 chars | 256-char string | | `archiveFolder` | 255 chars | 256-char string | Each row must assert HTTP 400. --- ## COULD ### C1 — Rename `BatchMetadataRequest` → `BulkEditIdsRequest`, `DocumentBatchSummary` → `BulkEditDocumentSummary` **Files:** - `backend/src/main/java/org/raddatz/familienarchiv/dto/BatchMetadataRequest.java` - `backend/src/main/java/org/raddatz/familienarchiv/dto/DocumentBatchSummary.java` - All references in controller, service, tests - Regenerate TS types after rename **Done when:** `grep -r "BatchMetadataRequest\|DocumentBatchSummary" backend/` returns zero results. --- ### C2 — Refactor `DocumentBulkEditDTO` from `@Data` to `record` **File:** `backend/src/main/java/org/raddatz/familienarchiv/dto/DocumentBulkEditDTO.java` **Blocker:** Tests that mutate via setters must first be rewritten to use constructor args (see Javadoc lines 23–26). Do S9 first — it touches the same tests. **Done when:** Class is a `record`; all `@Size` annotations preserved on record components; `@Data`, `@NoArgsConstructor`, `@AllArgsConstructor` removed. --- ### C3 — Split `BulkDocumentEditLayout.svelte` into upload and edit components **File:** `frontend/src/lib/components/document/BulkDocumentEditLayout.svelte` (~520 lines) Extract: - `BulkUploadLayout.svelte` — upload mode (`mode='upload'` branch) - `BulkEditLayout.svelte` — edit mode (`mode='edit'` branch) - `chunkAndSave()` helper — shared chunking concern - `BulkPdfPreview` / `FileSwitcherStrip` — shared sub-components **Done when:** The `mode` flag is no longer needed inside any single component; existing `BulkDocumentEditLayout.svelte.spec.ts` tests pass against the new structure. --- ### C4 — Add tooltip to `FieldLabelBadge` **File:** `frontend/src/lib/components/document/FieldLabelBadge.svelte` Add a `title` attribute explaining the behaviour on hover: - `additive`: `title="Wird zu bestehenden Werten hinzugefügt"` - `replace`: `title="Überschreibt bestehende Werte, wenn ausgefüllt"` **Done when:** Both variants have `title`; `FieldLabelBadge.svelte.spec.ts` asserts the value for each. --- ### C5 — Pin checkbox-click workaround in `DocumentRow.svelte.spec.ts` into one helper **File:** `frontend/src/lib/components/DocumentRow.svelte.spec.ts` The Tailwind-in-browser checkbox-click workaround is duplicated across ~5 test cases. Extract into a single `clickCheckbox(element)` helper. **Done when:** The workaround logic appears exactly once in the file. --- ### C6 — Cover remaining Esc-scope guard selectors in `BulkSelectionBar.svelte.spec.ts` **File:** `frontend/src/lib/components/document/BulkSelectionBar.svelte.spec.ts` `onEscape` checks 4 selectors (lines 24–31 in `BulkSelectionBar.svelte`). Current tests cover selectors 1 and 2 only: 1. `dialog[open]` ✅ line 90 2. `[aria-expanded="true"]` ✅ line 108 3. `[role="menu"]:not([hidden])` ❌ missing 4. `[role="dialog"]:not([hidden])` ❌ missing Also missing: `e.defaultPrevented` short-circuit (line 26 in `BulkSelectionBar.svelte`). **Add 3 test cases:** Escape does not clear when selector 3 matches; does not clear when selector 4 matches; does not clear when `e.defaultPrevented` is true. --- ## WON'T (this cycle) - **Add to issue #225's Out of Scope register** — verify #225 is still open before acting. If closed, include the rollback/versioning note in the S8 ADR instead.
marcel commented 2026-04-26 15:46:41 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

M1 — BulkEditError.message is confirmed free-form. BulkEditError.java is a one-liner record with String message. The controller catches DomainException and passes e.getMessage() directly, and catches generic Exception and passes the hardcoded string "Internal error". Both paths need to change: the DomainException branch should use e.getErrorCode() (the ErrorCode enum value), and the fallback branch should use a new ErrorCode.BULK_EDIT_DOCUMENT_FAILED constant.

M3 — handleDiscard guard is a bug, not a design choice. I looked at handleDiscard (line 130) vs handleSave (line 284). handleSave has if (saving) return; as its first line. handleDiscard has nothing. There's no comment, no test, no documented intent for this asymmetry. Given that handleDiscard in edit mode calls bulkSelectionStore.clear() and goto('/documents') — both state mutations — running it mid-save is almost certainly destructive. Treat this as a bug: add if (saving) return; at the top of handleDiscard.

M5 — Chunking test counts calls but doesn't verify ID completeness. This is a classic off-by-one trap. The flatMap + Set size assertion pattern in the issue is correct. The extractIdsFromCall helper will need to deserialize the FormData body — make sure to account for however the chunk payload encodes IDs (likely a JSON array field inside FormData, or URL-encoded array params).

M6 — Auto-clear $effect has zero test coverage. The effect at lines 28–40 of +layout.svelte reads page.url.pathname reactively and bulkSelectionStore.size inside untrack. The five test cases in the issue are exactly right. Case (e) — untrack prevents the effect from re-firing on store mutations — is the trickiest to write: you'll need to trigger the effect once by navigating into a non-bulk route, verify it fires, then mutate the store while still on that route and assert the effect does NOT fire a second time.

C3 — BulkDocumentEditLayout.svelte at 535 lines is well past the split threshold. The issue's decomposition is sensible. The mode='upload' and mode='edit' branches share very little beyond the file switcher and save logic. The chunkAndSave helper is already a pure function and extracts cleanly.

Recommendations

  • M1: In DocumentController.patchBulk, change new BulkEditError(id, sanitizeForLog(e.getMessage())) to new BulkEditError(id, e.getErrorCode()) for the DomainException branch. Add ErrorCode.BULK_EDIT_DOCUMENT_FAILED for the generic Exception branch. Update the BulkEditError record last — after all call sites are migrated — to catch compilation errors.
  • M3: Add if (saving) return; as the first line of handleDiscard. Then write the test — a test that calls handleDiscard while saving === true and asserts neither bulkSelectionStore.clear() nor goto() was called.
  • M6/e: Use vi.spyOn on bulkSelectionStore.clear to count invocations across the two triggers (route change vs. store mutation). The spy count should be 1 after a route change, and still 1 after a subsequent store mutation on the same non-bulk route.
## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations **M1 — BulkEditError.message is confirmed free-form.** `BulkEditError.java` is a one-liner record with `String message`. The controller catches `DomainException` and passes `e.getMessage()` directly, and catches generic `Exception` and passes the hardcoded string `"Internal error"`. Both paths need to change: the `DomainException` branch should use `e.getErrorCode()` (the ErrorCode enum value), and the fallback branch should use a new `ErrorCode.BULK_EDIT_DOCUMENT_FAILED` constant. **M3 — handleDiscard guard is a bug, not a design choice.** I looked at `handleDiscard` (line 130) vs `handleSave` (line 284). `handleSave` has `if (saving) return;` as its first line. `handleDiscard` has nothing. There's no comment, no test, no documented intent for this asymmetry. Given that `handleDiscard` in `edit` mode calls `bulkSelectionStore.clear()` and `goto('/documents')` — both state mutations — running it mid-save is almost certainly destructive. Treat this as a bug: add `if (saving) return;` at the top of `handleDiscard`. **M5 — Chunking test counts calls but doesn't verify ID completeness.** This is a classic off-by-one trap. The `flatMap` + Set size assertion pattern in the issue is correct. The `extractIdsFromCall` helper will need to deserialize the FormData body — make sure to account for however the chunk payload encodes IDs (likely a JSON array field inside FormData, or URL-encoded array params). **M6 — Auto-clear $effect has zero test coverage.** The effect at lines 28–40 of `+layout.svelte` reads `page.url.pathname` reactively and `bulkSelectionStore.size` inside `untrack`. The five test cases in the issue are exactly right. Case (e) — `untrack` prevents the effect from re-firing on store mutations — is the trickiest to write: you'll need to trigger the effect once by navigating into a non-bulk route, verify it fires, then mutate the store while still on that route and assert the effect does NOT fire a second time. **C3 — BulkDocumentEditLayout.svelte at 535 lines is well past the split threshold.** The issue's decomposition is sensible. The `mode='upload'` and `mode='edit'` branches share very little beyond the file switcher and save logic. The `chunkAndSave` helper is already a pure function and extracts cleanly. ### Recommendations - **M1**: In `DocumentController.patchBulk`, change `new BulkEditError(id, sanitizeForLog(e.getMessage()))` to `new BulkEditError(id, e.getErrorCode())` for the `DomainException` branch. Add `ErrorCode.BULK_EDIT_DOCUMENT_FAILED` for the generic `Exception` branch. Update the `BulkEditError` record last — after all call sites are migrated — to catch compilation errors. - **M3**: Add `if (saving) return;` as the first line of `handleDiscard`. Then write the test — a test that calls `handleDiscard` while `saving === true` and asserts neither `bulkSelectionStore.clear()` nor `goto()` was called. - **M6/e**: Use `vi.spyOn` on `bulkSelectionStore.clear` to count invocations across the two triggers (route change vs. store mutation). The spy count should be 1 after a route change, and still 1 after a subsequent store mutation on the same non-bulk route.
marcel commented 2026-04-26 15:47:07 +02:00
Author
Owner
Copy Link

🏗️ Markus Keller — Application Architect

Observations

S3 — onMount data fetch is a confirmed SSR violation. I verified it: +page.svelte imports onMount from svelte and calls the batch-metadata endpoint client-side (lines 2 and 15–16). This exposes the API route to the browser, bypasses server-side auth cookie forwarding, and causes a load flash. The challenge the issue names — "store is client-side state, so the server load can't read it" — is real. The proposed session-cookie approach is the right call for SvelteKit. Before implementing: persist the selected IDs into a short-lived cookie (e.g. bulk_ids, HTTP-only is not needed here since it's just IDs, not credentials) immediately before navigating to /documents/bulk-edit. The +page.server.ts load function reads the cookie, fetches batch metadata, then clears the cookie. This matches how SvelteKit handles client-state-to-server-load handoff in the standard checkout flow pattern.

S8 — The ADR is overdue. The additive-vs-replace semantics are buried in a Javadoc on DocumentBulkEditDTO lines 11–19. This is an API contract decision — if someone changes applyBulkEditToDocument six months from now and removes the additive behavior for tags, nothing in the codebase will warn them that this was intentional. The next ADR number is 006. The Javadoc @see back-reference is a good touch; make sure the ADR also captures the "why additive for tags" rationale (partially-reviewed documents must not lose tags accumulated over time).

C1 — Name mismatch in the issue. The issue says rename BatchMetadataRequest → BulkEditIdsRequest. Codebase has BatchMetadataRequest.java (confirmed) and DocumentBatchSummary.java (confirmed). But there is also a separate DocumentBatchMetadataDTO.java which is used for the upload-batch flow — do NOT rename that one. The rename target is specifically BatchMetadataRequest (used for the POST /api/documents/batch-metadata IDs payload). Confirm the rename scope before starting to avoid breaking the upload path.

S1 — N+1 is confirmed. resolveTags in DocumentService calls tagService.findOrCreate(cleanName) per tag name inside a private method that is called per document inside applyBulkEditToDocument. The controller loops 500 documents. With 10 tags, that is ~5000 tag-lookup queries. The fix — resolve tags once before the loop and pass the resolved Set<Tag> into applyBulkEditToDocument — is correct. Consider also pre-resolving sender and receivers in the same way; all three suffer from the same pattern.

Recommendations

  • S3: Implement the session-cookie handoff pattern. Use document.cookie on the client before goto('/documents/bulk-edit'), parse it in +page.server.ts. Keep the cookie lifetime to one navigation (max-age: 60 seconds is sufficient). If the cookie is absent in the server load (user navigated directly), redirect to /documents — same behavior as the current empty-store redirect.
  • S8: Write ADR-006 before implementing S1 or any semantic change to applyBulkEditToDocument. The ADR anchors the "why" so future refactors don't accidentally break the contract.
  • C1: Scope the rename to BatchMetadataRequest and DocumentBatchSummary only. Add grep -r "BatchMetadataRequest\|DocumentBatchSummary" backend/src to the done-when check to verify no references remain.

Open Decisions

  • S3 cookie vs. URL param: An alternative to the session cookie is to encode the IDs as a URL query param (e.g., /documents/bulk-edit?ids=uuid1,uuid2,...). With 500 UUIDs × 36 chars = ~18kb URL — too long for browsers (4KB limit). The cookie approach is the right one. No decision needed here; just confirming the issue's instinct is correct.
## 🏗️ Markus Keller — Application Architect ### Observations **S3 — onMount data fetch is a confirmed SSR violation.** I verified it: `+page.svelte` imports `onMount` from svelte and calls the batch-metadata endpoint client-side (lines 2 and 15–16). This exposes the API route to the browser, bypasses server-side auth cookie forwarding, and causes a load flash. The challenge the issue names — "store is client-side state, so the server load can't read it" — is real. The proposed session-cookie approach is the right call for SvelteKit. Before implementing: persist the selected IDs into a short-lived cookie (e.g. `bulk_ids`, HTTP-only is not needed here since it's just IDs, not credentials) immediately before navigating to `/documents/bulk-edit`. The `+page.server.ts` load function reads the cookie, fetches batch metadata, then clears the cookie. This matches how SvelteKit handles client-state-to-server-load handoff in the standard checkout flow pattern. **S8 — The ADR is overdue.** The additive-vs-replace semantics are buried in a Javadoc on `DocumentBulkEditDTO` lines 11–19. This is an API contract decision — if someone changes `applyBulkEditToDocument` six months from now and removes the additive behavior for tags, nothing in the codebase will warn them that this was intentional. The next ADR number is `006`. The Javadoc `@see` back-reference is a good touch; make sure the ADR also captures the "why additive for tags" rationale (partially-reviewed documents must not lose tags accumulated over time). **C1 — Name mismatch in the issue.** The issue says rename `BatchMetadataRequest` → `BulkEditIdsRequest`. Codebase has `BatchMetadataRequest.java` (confirmed) and `DocumentBatchSummary.java` (confirmed). But there is also a separate `DocumentBatchMetadataDTO.java` which is used for the upload-batch flow — do NOT rename that one. The rename target is specifically `BatchMetadataRequest` (used for the `POST /api/documents/batch-metadata` IDs payload). Confirm the rename scope before starting to avoid breaking the upload path. **S1 — N+1 is confirmed.** `resolveTags` in `DocumentService` calls `tagService.findOrCreate(cleanName)` per tag name inside a private method that is called per document inside `applyBulkEditToDocument`. The controller loops 500 documents. With 10 tags, that is ~5000 tag-lookup queries. The fix — resolve tags once before the loop and pass the resolved `Set<Tag>` into `applyBulkEditToDocument` — is correct. Consider also pre-resolving sender and receivers in the same way; all three suffer from the same pattern. ### Recommendations - **S3**: Implement the session-cookie handoff pattern. Use `document.cookie` on the client before `goto('/documents/bulk-edit')`, parse it in `+page.server.ts`. Keep the cookie lifetime to one navigation (max-age: 60 seconds is sufficient). If the cookie is absent in the server load (user navigated directly), redirect to `/documents` — same behavior as the current empty-store redirect. - **S8**: Write ADR-006 before implementing S1 or any semantic change to `applyBulkEditToDocument`. The ADR anchors the "why" so future refactors don't accidentally break the contract. - **C1**: Scope the rename to `BatchMetadataRequest` and `DocumentBatchSummary` only. Add `grep -r "BatchMetadataRequest\|DocumentBatchSummary" backend/src` to the done-when check to verify no references remain. ### Open Decisions - **S3 cookie vs. URL param**: An alternative to the session cookie is to encode the IDs as a URL query param (e.g., `/documents/bulk-edit?ids=uuid1,uuid2,...`). With 500 UUIDs × 36 chars = ~18kb URL — too long for browsers (4KB limit). The cookie approach is the right one. No decision needed here; just confirming the issue's instinct is correct.
marcel commented 2026-04-26 15:47:28 +02:00
Author
Owner
Copy Link

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

M1 — CWE-209: Information Exposure via BulkEditError.message. The current code in DocumentController.patchBulk does:

errors.add(new BulkEditError(id, sanitizeForLog(e.getMessage())));

sanitizeForLog strips \r\n only — it is a log-injection defense (CWE-117), not an information-exposure defense. The full DomainException.getMessage() — which can contain internal document IDs, person names, or DB-level entity details — reaches the API response body and therefore the browser. Switching to ErrorCode enum eliminates this: the enum value carries no internal state, is safe to serialize, and lets the frontend render a localized string via getErrorMessage().

Additionally, the generic Exception catch branch passes the hardcoded literal "Internal error" — this is fine as-is for exception safety, but once the type changes to ErrorCode, this branch must use a real enum value (BULK_EDIT_DOCUMENT_FAILED) rather than a string literal.

S2 — No rate limit on PATCH /api/documents/bulk. Confirmed: WebConfig.addInterceptors only registers the RateLimitInterceptor for /api/auth/invite/** and /api/auth/register. PATCH /api/documents/bulk is completely uncapped. An authenticated user with WRITE_ALL can loop calls at maximum throughput: each call processes up to 500 documents, each of which performs multiple DB writes (tags, sender, receivers, audit log). Without a cap, this is a potential self-DoS or resource amplification vector. The fix in the issue (addPathPatterns to include /api/documents/bulk) is correct.

M3 — Mid-save discard: security consequence is data integrity. This is less a confidentiality concern and more an integrity concern. If handleDiscard runs mid-save, it clears bulkSelectionStore and navigates away, while the in-flight PATCH request continues applying edits. The user sees an empty selection and thinks nothing happened — but the edits were applied to some subset of documents. The if (saving) return; guard is the right defense.

M1 — Test coverage for the security fix. Per the persona's methodology: the fix must start with a failing test. Before changing BulkEditError, add a @WebMvcTest test asserting that when patchBulk catches a DomainException, the response body contains a field code (enum value) and NOT a field message (string). The test proves the old behavior and will go red; the fix makes it green.

Recommendations

  • M1: The two-step migration order matters: (1) Add the new ErrorCode.BULK_EDIT_DOCUMENT_FAILED constant, (2) Change call sites in DocumentController, (3) Change the BulkEditError record signature last. This keeps compilation green throughout. Do not try to change the record and call sites simultaneously.
  • S2: The rate limit applies per-IP. Verify that RateLimitInterceptorTest covers the 429 case on the new path. The existing test pattern for /api/auth/register should be copy-extensible in under 10 lines.
  • Consider: The PATCH /api/documents/bulk endpoint has no per-user-session concurrency guard either — two simultaneous calls from the same session would both apply. This is lower risk (it requires coordination) and I'd park it as a future hardening item rather than blocking this cycle.
## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations **M1 — CWE-209: Information Exposure via BulkEditError.message.** The current code in `DocumentController.patchBulk` does: ```java errors.add(new BulkEditError(id, sanitizeForLog(e.getMessage()))); ``` `sanitizeForLog` strips `\r\n` only — it is a log-injection defense (CWE-117), not an information-exposure defense. The full `DomainException.getMessage()` — which can contain internal document IDs, person names, or DB-level entity details — reaches the API response body and therefore the browser. Switching to `ErrorCode` enum eliminates this: the enum value carries no internal state, is safe to serialize, and lets the frontend render a localized string via `getErrorMessage()`. Additionally, the generic `Exception` catch branch passes the hardcoded literal `"Internal error"` — this is fine as-is for exception safety, but once the type changes to `ErrorCode`, this branch must use a real enum value (`BULK_EDIT_DOCUMENT_FAILED`) rather than a string literal. **S2 — No rate limit on PATCH /api/documents/bulk.** Confirmed: `WebConfig.addInterceptors` only registers the `RateLimitInterceptor` for `/api/auth/invite/**` and `/api/auth/register`. `PATCH /api/documents/bulk` is completely uncapped. An authenticated user with `WRITE_ALL` can loop calls at maximum throughput: each call processes up to 500 documents, each of which performs multiple DB writes (tags, sender, receivers, audit log). Without a cap, this is a potential self-DoS or resource amplification vector. The fix in the issue (`addPathPatterns` to include `/api/documents/bulk`) is correct. **M3 — Mid-save discard: security consequence is data integrity.** This is less a confidentiality concern and more an integrity concern. If `handleDiscard` runs mid-save, it clears `bulkSelectionStore` and navigates away, while the in-flight PATCH request continues applying edits. The user sees an empty selection and thinks nothing happened — but the edits were applied to some subset of documents. The `if (saving) return;` guard is the right defense. **M1 — Test coverage for the security fix.** Per the persona's methodology: the fix must start with a failing test. Before changing `BulkEditError`, add a `@WebMvcTest` test asserting that when `patchBulk` catches a `DomainException`, the response body contains a field `code` (enum value) and NOT a field `message` (string). The test proves the old behavior and will go red; the fix makes it green. ### Recommendations - **M1**: The two-step migration order matters: (1) Add the new `ErrorCode.BULK_EDIT_DOCUMENT_FAILED` constant, (2) Change call sites in `DocumentController`, (3) Change the `BulkEditError` record signature last. This keeps compilation green throughout. Do not try to change the record and call sites simultaneously. - **S2**: The rate limit applies per-IP. Verify that `RateLimitInterceptorTest` covers the 429 case on the new path. The existing test pattern for `/api/auth/register` should be copy-extensible in under 10 lines. - **Consider**: The `PATCH /api/documents/bulk` endpoint has no per-user-session concurrency guard either — two simultaneous calls from the same session would both apply. This is lower risk (it requires coordination) and I'd park it as a future hardening item rather than blocking this cycle.
marcel commented 2026-04-26 15:47:53 +02:00
Author
Owner
Copy Link

🧪 Sara Holt — Senior QA Engineer

Observations

M4 — The integration test is the right call; the Mockito suite is structurally blind here. applyBulkEditToDocument is annotated @Transactional. The controller calls it in a loop with individual try/catch per document. Mockito cannot exercise the @Transactional proxy, so the claim "a per-document failure is isolated" is untestable at unit level. The test scenario in the issue is precise and correct. Two implementation notes:

  1. Use @Transactional on the test method itself for automatic rollback — BUT for this specific test, you need to actually verify DB state after the call. If you use @Transactional on the test, the transaction spans the test and the save, and rollback happens together — making it impossible to read back the committed state. Use @Sql(executionPhase = AFTER_TEST_METHOD, scripts = "cleanup.sql") or a manual @AfterEach delete instead of @Transactional on the test method.
  2. The existing PersonServiceIntegrationTest and DocumentSearchPagedIntegrationTest show the Testcontainers setup pattern (postgres:16-alpine, @DynamicPropertySource). Mirror that exactly — don't introduce a new Postgres config class if one already exists.

M5 — ID round-trip assertion is a legitimate regression guard. The flatMap + Set.size pattern catches both duplicates and drops. One addition: also assert that new Set(allSentIds) contains no IDs that weren't in the original 1100 — i.e., check for IDs appearing in chunks that weren't in the input (though this is less likely with a straightforward slice implementation).

M6 — Five test cases are the right coverage. Case (e) — untrack guarantee — is the highest-value case because it's the one most likely to regress silently. If someone changes the $effect to read bulkSelectionStore.size outside untrack, 500 checkbox toggle events will fire 500 clears. The test should trigger 10 rapid store mutations and assert clear was called 0 times (not 10, not 1).

S6 — tagOp=OR is untested. Confirmed: looking at the DocumentControllerTest, the patchBulk_* tests cover auth, validation, deduplication, and the happy path, but getDocumentIds tests only pass senderId. tagOp=OR is a code path branch that is completely unexercised at the controller layer.

S9 — Parametrizing the @Size tests is correct. I found patchBulk_returns400_whenArchiveBoxExceeds255Chars in DocumentControllerTest. The current approach has separate test methods per field. A @ParameterizedTest @MethodSource (rather than @CsvSource, since you need to construct lists of UUIDs for receiverIds) is cleaner for the list-type fields. Use @CsvSource for the string fields, and a separate @ParameterizedTest @MethodSource for the list fields.

S7 — Snapshot FormData across chunks. Without this, you can't distinguish "bug: sender resets between chunks" from "feature: sender is consistently applied." The test is cheap to write and catches a whole class of regression.

Recommendations

  • M4: Place the test in backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceBulkEditIntegrationTest.java. Use @SpringBootTest + Testcontainers (follow PersonServiceIntegrationTest pattern). Do NOT annotate the test method @Transactional — you need to read committed DB state after patchBulk. Clean up with @AfterEach repository deletes.
  • S9: Split into two parameterized methods: one @CsvSource for string fields, one @MethodSource for list fields (since @CsvSource can't construct a List<UUID> of 201 entries inline).
  • M6/e: Use vi.spyOn(bulkSelectionStore, 'clear') before the test; assert clearSpy.callCount === 0 after 10 store mutations while on a non-bulk route.
## 🧪 Sara Holt — Senior QA Engineer ### Observations **M4 — The integration test is the right call; the Mockito suite is structurally blind here.** `applyBulkEditToDocument` is annotated `@Transactional`. The controller calls it in a loop with individual try/catch per document. Mockito cannot exercise the `@Transactional` proxy, so the claim "a per-document failure is isolated" is untestable at unit level. The test scenario in the issue is precise and correct. Two implementation notes: 1. Use `@Transactional` on the test method itself for automatic rollback — BUT for this specific test, you need to actually verify DB state after the call. If you use `@Transactional` on the test, the transaction spans the test and the save, and rollback happens together — making it impossible to read back the committed state. Use `@Sql(executionPhase = AFTER_TEST_METHOD, scripts = "cleanup.sql")` or a manual `@AfterEach` delete instead of `@Transactional` on the test method. 2. The existing `PersonServiceIntegrationTest` and `DocumentSearchPagedIntegrationTest` show the Testcontainers setup pattern (`postgres:16-alpine`, `@DynamicPropertySource`). Mirror that exactly — don't introduce a new Postgres config class if one already exists. **M5 — ID round-trip assertion is a legitimate regression guard.** The `flatMap + Set.size` pattern catches both duplicates and drops. One addition: also assert that `new Set(allSentIds)` contains no IDs that weren't in the original 1100 — i.e., check for IDs appearing in chunks that weren't in the input (though this is less likely with a straightforward slice implementation). **M6 — Five test cases are the right coverage.** Case (e) — `untrack` guarantee — is the highest-value case because it's the one most likely to regress silently. If someone changes the `$effect` to read `bulkSelectionStore.size` outside `untrack`, 500 `checkbox toggle` events will fire 500 clears. The test should trigger 10 rapid store mutations and assert `clear` was called 0 times (not 10, not 1). **S6 — tagOp=OR is untested.** Confirmed: looking at the `DocumentControllerTest`, the `patchBulk_*` tests cover auth, validation, deduplication, and the happy path, but `getDocumentIds` tests only pass `senderId`. `tagOp=OR` is a code path branch that is completely unexercised at the controller layer. **S9 — Parametrizing the @Size tests is correct.** I found `patchBulk_returns400_whenArchiveBoxExceeds255Chars` in `DocumentControllerTest`. The current approach has separate test methods per field. A `@ParameterizedTest @MethodSource` (rather than `@CsvSource`, since you need to construct lists of UUIDs for `receiverIds`) is cleaner for the list-type fields. Use `@CsvSource` for the string fields, and a separate `@ParameterizedTest @MethodSource` for the list fields. **S7 — Snapshot FormData across chunks.** Without this, you can't distinguish "bug: sender resets between chunks" from "feature: sender is consistently applied." The test is cheap to write and catches a whole class of regression. ### Recommendations - **M4**: Place the test in `backend/src/test/java/org/raddatz/familienarchiv/service/DocumentServiceBulkEditIntegrationTest.java`. Use `@SpringBootTest` + Testcontainers (follow `PersonServiceIntegrationTest` pattern). Do NOT annotate the test method `@Transactional` — you need to read committed DB state after `patchBulk`. Clean up with `@AfterEach` repository deletes. - **S9**: Split into two parameterized methods: one `@CsvSource` for string fields, one `@MethodSource` for list fields (since `@CsvSource` can't construct a `List<UUID>` of 201 entries inline). - **M6/e**: Use `vi.spyOn(bulkSelectionStore, 'clear')` before the test; assert `clearSpy.callCount === 0` after 10 store mutations while on a non-bulk route.
marcel commented 2026-04-26 15:48:17 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Observations

M2 — Focus ring on DocumentRow checkbox is a confirmed WCAG 2.1 AA SC 2.4.7 failure. The <label data-testid="bulk-select-checkbox"> is the 44×44 hit target. From the grep output, the focus-within ring is not on the label — only the inner <input> receives it. The fix is exactly as specified: focus-within:ring-2 focus-within:ring-brand-mint focus-within:ring-offset-1 on the <label> element. Add outline-none to the <input> so the browser's native ring doesn't render inside the custom ring area. Do not use focus-visible:ring-2 here — focus-within on the label is the right selector because the <input> inside receives focus, not the label itself.

S4 — Responsive stacking for BulkDocumentEditLayout. The 55/45 panel split at mobile viewport is unusable. I recommend Option A (stack panels) — hide the PDF preview by default on mobile, show only the form, and allow the user to expand the preview with a toggle. This preserves mobile access to the bulk-edit feature (Option B, hiding the CTA entirely, removes it). The stacking order should be: form on top, PDF preview below, with a "Vorschau einblenden" disclosure button. This follows the project's established pattern of "content before chrome" at small viewports. Avoid the horizontal overflow issue by using overflow-hidden on the outer container.

S5 — /documents/bulk-edit is high a11y-regression risk. It has:

  • role="note" callouts (the FieldLabelBadge additive/replace indicators)
  • A sticky save bar
  • Focus-managed checkboxes
  • A two-panel layout that reshapes at breakpoints

All four are common axe violation sources. The Playwright test should: (1) log in, (2) select 2 documents via checkbox, (3) navigate to /documents/bulk-edit, (4) run checkA11y() targeting wcag2a and wcag2aa. Expected zero critical/serious violations.

C4 — FieldLabelBadge tooltips are a polish improvement but meaningful for seniors. Our 60+ audience reading "Additiv" or "Ersetzt" badge labels without context will not understand the semantics. A title attribute is the minimum viable improvement. A more robust solution would use a <details>/<summary> pattern or an accessible tooltip (with role="tooltip", aria-describedby), but title is acceptable for this cycle given the badge is supplementary UI chrome.

Recommendations

  • M2: Apply focus-within:ring-2 focus-within:ring-brand-mint focus-within:ring-offset-1 rounded-sm to the <label> wrapper. Add focus:outline-none to the <input> inside. Test by tabbing through a document list — the ring should appear on the full 44×44 label area, not the inner 20×20 checkbox.
  • S4: Choose Option A. Use flex-col md:flex-row on the outer panel container. On mobile, add a <button class="md:hidden ...">Vorschau {showPreview ? 'ausblenden' : 'einblenden'}</button> toggle. The PDF panel has class="hidden md:block" unless showPreview is true. Keep the form always visible.
  • M2 test: DocumentRow.svelte.spec.ts should assert label[data-testid="bulk-select-checkbox"] has class focus-within:ring-2 (or query computed styles after programmatic focus on the input). The existing checkbox-click workaround helper (C5) should be extracted first to keep the new test clean.

Open Decisions

  • S4 — Mobile PDF preview interaction: Once stacked, should the PDF preview remember its expanded/collapsed state as the user tabs through documents in the file switcher strip, or reset to collapsed on each document switch? Collapsed-on-switch is simpler and avoids layout jumps, but a senior user who expanded the preview once may be confused when it collapses. Recommendation: reset to collapsed on switch (simpler, prevents accidental full-screen PDF state). Override if user testing contradicts this.
## 🎨 Leonie Voss — UX Designer & Accessibility Strategist ### Observations **M2 — Focus ring on DocumentRow checkbox is a confirmed WCAG 2.1 AA SC 2.4.7 failure.** The `<label data-testid="bulk-select-checkbox">` is the 44×44 hit target. From the grep output, the `focus-within` ring is not on the label — only the inner `<input>` receives it. The fix is exactly as specified: `focus-within:ring-2 focus-within:ring-brand-mint focus-within:ring-offset-1` on the `<label>` element. Add `outline-none` to the `<input>` so the browser's native ring doesn't render inside the custom ring area. Do not use `focus-visible:ring-2` here — `focus-within` on the label is the right selector because the `<input>` inside receives focus, not the label itself. **S4 — Responsive stacking for BulkDocumentEditLayout.** The 55/45 panel split at mobile viewport is unusable. I recommend **Option A (stack panels)** — hide the PDF preview by default on mobile, show only the form, and allow the user to expand the preview with a toggle. This preserves mobile access to the bulk-edit feature (Option B, hiding the CTA entirely, removes it). The stacking order should be: form on top, PDF preview below, with a "Vorschau einblenden" disclosure button. This follows the project's established pattern of "content before chrome" at small viewports. Avoid the horizontal overflow issue by using `overflow-hidden` on the outer container. **S5 — /documents/bulk-edit is high a11y-regression risk.** It has: - `role="note"` callouts (the FieldLabelBadge additive/replace indicators) - A sticky save bar - Focus-managed checkboxes - A two-panel layout that reshapes at breakpoints All four are common axe violation sources. The Playwright test should: (1) log in, (2) select 2 documents via checkbox, (3) navigate to `/documents/bulk-edit`, (4) run `checkA11y()` targeting `wcag2a` and `wcag2aa`. Expected zero critical/serious violations. **C4 — FieldLabelBadge tooltips are a polish improvement but meaningful for seniors.** Our 60+ audience reading "Additiv" or "Ersetzt" badge labels without context will not understand the semantics. A `title` attribute is the minimum viable improvement. A more robust solution would use a `<details>`/`<summary>` pattern or an accessible tooltip (with `role="tooltip"`, `aria-describedby`), but `title` is acceptable for this cycle given the badge is supplementary UI chrome. ### Recommendations - **M2**: Apply `focus-within:ring-2 focus-within:ring-brand-mint focus-within:ring-offset-1 rounded-sm` to the `<label>` wrapper. Add `focus:outline-none` to the `<input>` inside. Test by tabbing through a document list — the ring should appear on the full 44×44 label area, not the inner 20×20 checkbox. - **S4**: Choose Option A. Use `flex-col md:flex-row` on the outer panel container. On mobile, add a `<button class="md:hidden ...">Vorschau {showPreview ? 'ausblenden' : 'einblenden'}</button>` toggle. The PDF panel has `class="hidden md:block"` unless `showPreview` is true. Keep the form always visible. - **M2 test**: `DocumentRow.svelte.spec.ts` should assert `label[data-testid="bulk-select-checkbox"]` has class `focus-within:ring-2` (or query computed styles after programmatic focus on the input). The existing checkbox-click workaround helper (C5) should be extracted first to keep the new test clean. ### Open Decisions - **S4 — Mobile PDF preview interaction**: Once stacked, should the PDF preview remember its expanded/collapsed state as the user tabs through documents in the file switcher strip, or reset to collapsed on each document switch? Collapsed-on-switch is simpler and avoids layout jumps, but a senior user who expanded the preview once may be confused when it collapses. Recommendation: reset to collapsed on switch (simpler, prevents accidental full-screen PDF state). Override if user testing contradicts this.
marcel commented 2026-04-26 15:48:37 +02:00
Author
Owner
Copy Link

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

M4 — @SpringBootTest with Testcontainers: standard and correct. The existing integration tests (PersonServiceIntegrationTest, DocumentSearchPagedIntegrationTest, UserManagementAuditIntegrationTest) all use the same Testcontainers setup with postgres:16-alpine. Confirm there's a shared PostgresContainerConfig or static container pattern before writing a new one — avoid spinning up a second Postgres container per test class. If a shared @Container static PostgreSQLContainer<?> already exists in a base class or config, use it.

S2 — Rate limiter on /api/documents/bulk: infra impact is negligible. Adding /api/documents/bulk to WebConfig.addInterceptors is a one-liner config change. The RateLimitInterceptor is an in-memory per-IP counter — no external dependency, no new infrastructure. The CI pipeline picks this up automatically on the next backend build. No deployment changes needed.

S3 — onMount data fetch: no infra impact. The switch from onMount client-fetch to +page.server.ts load is a pure frontend refactor. The session-cookie approach Markus recommended uses the existing SvelteKit cookie API — no server-side session store, no Redis, no new infrastructure. This is correct for a solo-hosted family project where adding infrastructure is the last resort.

S8 — ADR in docs/adr/: confirm numbering before writing. Last ADR is 005-thumbnail-aspect-and-page-count.md. Next is 006. Write to docs/adr/006-bulk-edit-additive-replace-semantics.md. The WON'T entry at the bottom of the issue mentions cross-referencing issue #225 — verify that issue is still open before adding the reference (a reference to a closed issue in an ADR is confusing but not blocking).

CI timing impact of M4: A new @SpringBootTest integration test with Testcontainers adds roughly 10–20 seconds to the backend test run (cold Postgres container pull is cached after first run). This keeps total CI well within the 2-minute integration test budget. No concern here.

Recommendations

  • M4: Check for an existing Testcontainers config class (look for PostgresContainerConfig.java or a @Container static field in a base test class) before writing new setup code. Reuse it.
  • C1 (if tackled): After renaming BatchMetadataRequest → BulkEditIdsRequest, regenerate TS types and run npm run check in frontend/ before the commit. Type errors from the rename show up there first.
  • No new infra concerns across all items. This issue is entirely application-layer changes. The Docker Compose, CI pipeline, and production Compose file are unaffected.

No concerns from my angle on observability either — the existing log.info("bulkEdit actor=... errors=...") in DocumentController already captures per-request error counts. If S2 adds the rate limiter, confirm RateLimitInterceptor logs the 429 response (or at minimum doesn't swallow it silently) so we can spot abuse in Loki.

## 🚀 Tobias Wendt — DevOps & Platform Engineer ### Observations **M4 — @SpringBootTest with Testcontainers: standard and correct.** The existing integration tests (`PersonServiceIntegrationTest`, `DocumentSearchPagedIntegrationTest`, `UserManagementAuditIntegrationTest`) all use the same Testcontainers setup with `postgres:16-alpine`. Confirm there's a shared `PostgresContainerConfig` or static container pattern before writing a new one — avoid spinning up a second Postgres container per test class. If a shared `@Container static PostgreSQLContainer<?>` already exists in a base class or config, use it. **S2 — Rate limiter on /api/documents/bulk: infra impact is negligible.** Adding `/api/documents/bulk` to `WebConfig.addInterceptors` is a one-liner config change. The `RateLimitInterceptor` is an in-memory per-IP counter — no external dependency, no new infrastructure. The CI pipeline picks this up automatically on the next backend build. No deployment changes needed. **S3 — onMount data fetch: no infra impact.** The switch from `onMount` client-fetch to `+page.server.ts` load is a pure frontend refactor. The session-cookie approach Markus recommended uses the existing SvelteKit cookie API — no server-side session store, no Redis, no new infrastructure. This is correct for a solo-hosted family project where adding infrastructure is the last resort. **S8 — ADR in docs/adr/: confirm numbering before writing.** Last ADR is `005-thumbnail-aspect-and-page-count.md`. Next is `006`. Write to `docs/adr/006-bulk-edit-additive-replace-semantics.md`. The WON'T entry at the bottom of the issue mentions cross-referencing issue #225 — verify that issue is still open before adding the reference (a reference to a closed issue in an ADR is confusing but not blocking). **CI timing impact of M4:** A new `@SpringBootTest` integration test with Testcontainers adds roughly 10–20 seconds to the backend test run (cold Postgres container pull is cached after first run). This keeps total CI well within the 2-minute integration test budget. No concern here. ### Recommendations - **M4**: Check for an existing Testcontainers config class (look for `PostgresContainerConfig.java` or a `@Container static` field in a base test class) before writing new setup code. Reuse it. - **C1 (if tackled)**: After renaming `BatchMetadataRequest` → `BulkEditIdsRequest`, regenerate TS types and run `npm run check` in `frontend/` before the commit. Type errors from the rename show up there first. - **No new infra concerns across all items.** This issue is entirely application-layer changes. The Docker Compose, CI pipeline, and production Compose file are unaffected. No concerns from my angle on observability either — the existing `log.info("bulkEdit actor=... errors=...")` in `DocumentController` already captures per-request error counts. If S2 adds the rate limiter, confirm `RateLimitInterceptor` logs the 429 response (or at minimum doesn't swallow it silently) so we can spot abuse in Loki.
marcel commented 2026-04-26 15:48:58 +02:00
Author
Owner
Copy Link

📋 Elicit — Requirements Engineer

Observations

Issue structure is exemplary. Must → Should → Could ordering with explicit "Done when" conditions on every item is exactly the Definition of Ready pattern that prevents implementation ambiguity. Each item is atomic and independently committable. No concerns on the overall structure.

M3 — "Decision required" framing is correct but the answer is evident from the code. The issue asks "Is mid-save discard intentional?" Looking at the code: handleSave has an explicit guard (if (saving) return;), handleDiscard does not. That asymmetry in code written in the same feature is the strongest signal that the missing guard is a bug, not a deliberate design choice. The requirement can be tightened: remove the "Decision required" framing entirely and specify "Add if (saving) return; guard — the asymmetry with handleSave is a defect." No ambiguity remains.

S3 — "Confirm the approach before implementing" is an open requirement. This is the only item in the issue where the implementation path is genuinely uncertain. The acceptance criterion ("Page renders with document data on first paint; onMount data-fetch is removed") is clear, but the mechanism (session cookie vs. store serialization vs. URL param) is unresolved. Markus's analysis above eliminates the URL param option (URL length limit with 500 UUIDs). The cookie approach is recommended. This decision should be locked before S3 is started — it affects +page.server.ts structure and the store clear timing.

S8 ADR references issue #225. The WON'T entry says "Add to issue #225's Out of Scope register — verify #225 is still open before acting." This is a cross-issue dependency that should be tracked. If #225 is closed, the ADR absorbs the rollback/versioning note directly. Recommend: check issue #225 state before writing ADR-006, and record the outcome explicitly in the ADR's "Alternatives considered" section.

Missing acceptance criterion on M1 (frontend rendering). The "Done when" for M1 says "the frontend renders a localized string when a per-document failure occurs." This implies a change to getErrorMessage() in errors.ts and Paraglide translation keys — but the issue does not specify what the localized string should say. For BULK_EDIT_DOCUMENT_FAILED, a reasonable default is "Dieses Dokument konnte nicht bearbeitet werden." / "This document could not be edited." / "Este documento no pudo editarse." Pin these strings in the issue or in the translation files directly; don't leave it to implementation judgment.

Recommendations

  • M3: Remove the "Decision required" block. Replace with a direct requirement: "Add if (saving) return; at top of handleDiscard (parity with handleSave guard at line 284)."
  • S3: Lock the cookie implementation approach before starting. Update the "Done when" to include: "Cookie bulk_ids is written before navigation and cleared by the server load after reading." This makes the acceptance criterion testable.
  • M1 strings: Add the three translation strings to the issue body so the implementer does not have to invent copy. Suggest: "Dieses Dokument konnte nicht bearbeitet werden." for de.json.

Open Decisions

  • S3 — Cookie security attributes: Should the bulk_ids cookie be HttpOnly? It contains only UUIDs (no credentials), so JavaScript read access is harmless. Recommend: not HttpOnly, since the store also holds the IDs client-side anyway. But document this choice explicitly in the cookie write so a future security auditor doesn't flag it.
## 📋 Elicit — Requirements Engineer ### Observations **Issue structure is exemplary.** Must → Should → Could ordering with explicit "Done when" conditions on every item is exactly the Definition of Ready pattern that prevents implementation ambiguity. Each item is atomic and independently committable. No concerns on the overall structure. **M3 — "Decision required" framing is correct but the answer is evident from the code.** The issue asks "Is mid-save discard intentional?" Looking at the code: `handleSave` has an explicit guard (`if (saving) return;`), `handleDiscard` does not. That asymmetry in code written in the same feature is the strongest signal that the missing guard is a bug, not a deliberate design choice. The requirement can be tightened: remove the "Decision required" framing entirely and specify "Add `if (saving) return;` guard — the asymmetry with handleSave is a defect." No ambiguity remains. **S3 — "Confirm the approach before implementing" is an open requirement.** This is the only item in the issue where the implementation path is genuinely uncertain. The acceptance criterion ("Page renders with document data on first paint; `onMount` data-fetch is removed") is clear, but the mechanism (session cookie vs. store serialization vs. URL param) is unresolved. Markus's analysis above eliminates the URL param option (URL length limit with 500 UUIDs). The cookie approach is recommended. This decision should be locked before S3 is started — it affects `+page.server.ts` structure and the store clear timing. **S8 ADR references issue #225.** The WON'T entry says "Add to issue #225's Out of Scope register — verify #225 is still open before acting." This is a cross-issue dependency that should be tracked. If #225 is closed, the ADR absorbs the rollback/versioning note directly. Recommend: check issue #225 state before writing ADR-006, and record the outcome explicitly in the ADR's "Alternatives considered" section. **Missing acceptance criterion on M1 (frontend rendering).** The "Done when" for M1 says "the frontend renders a localized string when a per-document failure occurs." This implies a change to `getErrorMessage()` in `errors.ts` and Paraglide translation keys — but the issue does not specify what the localized string should say. For `BULK_EDIT_DOCUMENT_FAILED`, a reasonable default is `"Dieses Dokument konnte nicht bearbeitet werden."` / `"This document could not be edited."` / `"Este documento no pudo editarse."` Pin these strings in the issue or in the translation files directly; don't leave it to implementation judgment. ### Recommendations - **M3**: Remove the "Decision required" block. Replace with a direct requirement: "Add `if (saving) return;` at top of `handleDiscard` (parity with `handleSave` guard at line 284)." - **S3**: Lock the cookie implementation approach before starting. Update the "Done when" to include: "Cookie `bulk_ids` is written before navigation and cleared by the server load after reading." This makes the acceptance criterion testable. - **M1 strings**: Add the three translation strings to the issue body so the implementer does not have to invent copy. Suggest: `"Dieses Dokument konnte nicht bearbeitet werden."` for `de.json`. ### Open Decisions - **S3 — Cookie security attributes**: Should the `bulk_ids` cookie be `HttpOnly`? It contains only UUIDs (no credentials), so JavaScript read access is harmless. Recommend: not `HttpOnly`, since the store also holds the IDs client-side anyway. But document this choice explicitly in the cookie write so a future security auditor doesn't flag it.
marcel commented 2026-04-26 15:49:15 +02:00
Author
Owner
Copy Link

🗳️ Decision Queue — Action Required

2 decisions need your input before implementation starts.

Frontend / UX

  • S4 — Mobile PDF preview expanded/collapsed state on document switch: Once panels are stacked on mobile, should the preview panel reset to collapsed each time the user switches to a different document in the file switcher strip, or remember its state? Options: (A) reset to collapsed on every switch — simpler, no layout jump risk; (B) remember state — consistent but risks leaving a full-screen PDF up when the user expected the form. Leonie recommends (A). (Raised by: Leonie)

Security / Architecture

  • S3 — Cookie HttpOnly attribute for bulk_ids: The session cookie holding selected UUIDs before navigating to /documents/bulk-edit contains no credentials, only document IDs. Should it be HttpOnly: false (allows the store to read it back client-side, simpler) or HttpOnly: true (slightly more defensive, but client JS cannot read it — the store already holds the IDs anyway so this is redundant). Recommendation: HttpOnly: false, but add a comment in the code so a future auditor doesn't flag it. Confirm before implementing S3. (Raised by: Elicit)
## 🗳️ Decision Queue — Action Required _2 decisions need your input before implementation starts._ ### Frontend / UX - **S4 — Mobile PDF preview expanded/collapsed state on document switch**: Once panels are stacked on mobile, should the preview panel reset to collapsed each time the user switches to a different document in the file switcher strip, or remember its state? Options: (A) reset to collapsed on every switch — simpler, no layout jump risk; (B) remember state — consistent but risks leaving a full-screen PDF up when the user expected the form. Leonie recommends (A). _(Raised by: Leonie)_ ### Security / Architecture - **S3 — Cookie HttpOnly attribute for `bulk_ids`**: The session cookie holding selected UUIDs before navigating to `/documents/bulk-edit` contains no credentials, only document IDs. Should it be `HttpOnly: false` (allows the store to read it back client-side, simpler) or `HttpOnly: true` (slightly more defensive, but client JS cannot read it — the store already holds the IDs anyway so this is redundant). Recommendation: `HttpOnly: false`, but add a comment in the code so a future auditor doesn't flag it. Confirm before implementing S3. _(Raised by: Elicit)_
marcel commented 2026-04-28 11:28:54 +02:00
Author
Owner
Copy Link

S4: A
S3: Is the cookie needed anyway? If it's needed, we make HttpOnly false, if its not needed, remove it. Please verify beforehand.

S4: A S3: Is the cookie needed anyway? If it's needed, we make HttpOnly false, if its not needed, remove it. Please verify beforehand.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
P0-critical
Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.
 P1-high
Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.
 P2-medium
Noticeable improvement; doesn't block anything; schedule opportunistically.
 P3-later
Cosmetic or parking-lot work; fine to stay open indefinitely.
 audit
Read-only audit / assessment work; produces a report rather than changing code
 bug
Something isn't working
 cleanup
Removal of dead code, vague comments, naming violations, and scratch artifacts
 collaboration
We want to extend the family archive to have more collaboration tools
 conversation
descoped
We will do that later
 devops
documentation
README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs
 epic
Marker for epic-level issues that group multiple child issues
 feature
file-upload
legibility
Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction
 notification
person
phase-1: security
Security hygiene — must be done first
 phase-2: container-images
Production-ready multi-stage Docker images
 phase-3: prod-compose
Production compose overlay + Caddy reverse proxy
 phase-4: spring-prod-profile
Spring Boot production configuration profile
 phase-5: backups
Database and object storage backup strategy
 phase-6: deployment-docs
.env.example, DEPLOYMENT.md, runbook
 phase-7: monitoring
Prometheus, Loki, Grafana, Alertmanager
 refactor
Code restructuring without behaviour change
 security
test
ui
UI/UX and accessibility issues
 user
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Jens marcel
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/familienarchiv#332
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?