feat(viewer): show delete icon directly on transcription annotation (#339) #348

marcel · 2026-04-26T21:07:26+02:00

marcel commented

2026-04-26 21:07:26 +02:00

Summary

Adds a trash icon directly on transcription annotation shapes in the PDF viewer, enabling single-gesture block deletion without navigating to the Bearbeiten tab.

What was implemented

AnnotationShape.svelte — trash button (44×44 px, top-right corner, teal on hover) with showDelete/onDeleteRequest props; Delete key support on focused annotations; button only visible when hovered or active and showDelete=true
AnnotationLayer.svelte — passes showDelete={canDraw} and onDeleteRequest down to each AnnotationShape
PdfViewer.svelte — accepts and forwards onDeleteAnnotationRequest prop
DocumentViewer.svelte — accepts and forwards onDeleteAnnotationRequest prop
+page.svelte — handleAnnotationDeleteRequest(): confirm dialog → delete block (if linked) or DELETE /annotations/{id} directly (orphaned annotation fallback) → reload annotations
AnnotationShape.svelte.spec.ts (new) — 8 browser-mode unit tests covering all visibility/interaction cases
AnnotationLayer.svelte.spec.ts — updated tests to match new showDelete prop behaviour
TranscriptionBlockControllerTest.java — added security regression: deleteBlock_returns403_whenUserHasOnlyReadAllPermission

Decisions resolved during review

Confirm-dialog (not undo-toast) — reuses existing ConfirmService, no new i18n keys
Full prop chain (+page.svelte → DocumentViewer → PdfViewer → AnnotationLayer → AnnotationShape) mirroring existing onAnnotationClick pattern
Orphaned annotations (no linked block) show the trash icon and delete the annotation record directly via DELETE /annotations/{id}

NFRs met

Touch target ≥ 44 px on the trash button
Keyboard-accessible via Delete key on focused annotation
Secondary path (Bearbeiten → Block → Block löschen) preserved

Closes #339

## Summary Adds a trash icon directly on transcription annotation shapes in the PDF viewer, enabling single-gesture block deletion without navigating to the Bearbeiten tab. ## What was implemented - **`AnnotationShape.svelte`** — trash button (44×44 px, top-right corner, teal on hover) with `showDelete`/`onDeleteRequest` props; Delete key support on focused annotations; button only visible when hovered or active and `showDelete=true` - **`AnnotationLayer.svelte`** — passes `showDelete={canDraw}` and `onDeleteRequest` down to each `AnnotationShape` - **`PdfViewer.svelte`** — accepts and forwards `onDeleteAnnotationRequest` prop - **`DocumentViewer.svelte`** — accepts and forwards `onDeleteAnnotationRequest` prop - **`+page.svelte`** — `handleAnnotationDeleteRequest()`: confirm dialog → delete block (if linked) or `DELETE /annotations/{id}` directly (orphaned annotation fallback) → reload annotations - **`AnnotationShape.svelte.spec.ts`** (new) — 8 browser-mode unit tests covering all visibility/interaction cases - **`AnnotationLayer.svelte.spec.ts`** — updated tests to match new `showDelete` prop behaviour - **`TranscriptionBlockControllerTest.java`** — added security regression: `deleteBlock_returns403_whenUserHasOnlyReadAllPermission` ## Decisions resolved during review 1. **Confirm-dialog** (not undo-toast) — reuses existing `ConfirmService`, no new i18n keys 2. **Full prop chain** (`+page.svelte` → `DocumentViewer` → `PdfViewer` → `AnnotationLayer` → `AnnotationShape`) mirroring existing `onAnnotationClick` pattern 3. **Orphaned annotations** (no linked block) show the trash icon and delete the annotation record directly via `DELETE /annotations/{id}` ## NFRs met - Touch target ≥ 44 px on the trash button - Keyboard-accessible via Delete key on focused annotation - Secondary path (Bearbeiten → Block → Block löschen) preserved Closes #339

marcel added 1 commit 2026-04-26 21:07:26 +02:00

feat(viewer): show delete icon on annotation for direct block deletion (#339 )

CI / Unit & Component Tests (push) Failing after 3m11s

Details

CI / OCR Service Tests (push) Successful in 40s

Details

CI / Backend Unit Tests (push) Failing after 3m4s

Details

CI / Unit & Component Tests (pull_request) Failing after 3m7s

Details

CI / OCR Service Tests (pull_request) Successful in 30s

Details

CI / Backend Unit Tests (pull_request) Failing after 2m54s

Details

b13c10936b

Adds a trash icon button (44×44 px touch target) directly on each annotation shape in transcription mode so users can delete a block without navigating through the sidebar. Includes keyboard support (Delete key), confirm dialog via ConfirmService, prop-chain wiring through DocumentViewer → PdfViewer → AnnotationLayer → AnnotationShape, and orphaned-annotation fallback (calls DELETE /annotations/{id} when no block is linked). Backend security regression test added for deleteBlock 403 on READ_ALL.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-04-26 21:08:15 +02:00

🏗️ Markus Keller — Application Architect

Verdict: ✅ Approved

What I checked

Layer boundaries, prop-chain design, and module coupling.

Findings

Prop chain implementation is correct. The onDeleteAnnotationRequest / onDeleteRequest chain through DocumentViewer → PdfViewer → AnnotationLayer → AnnotationShape mirrors the existing onAnnotationClick and onTranscriptionDraw patterns exactly. No new coupling is introduced.

+page.svelte correctly owns the orchestration. The confirm dialog, block lookup, and API call all live in handleAnnotationDeleteRequest at the page level. AnnotationShape emits only intent — it has no knowledge of transcription blocks, ConfirmService, or API routes. This is the right boundary.

Orphaned annotation fallback is architecturally fine. The DELETE /api/documents/{id}/annotations/{annotationId} call in the else-branch is a clean escape hatch. The branch is short-lived — once all annotations have linked blocks, this path is never taken.

canDraw as the showDelete driver is correct. canDraw already reflects transcribeMode && !ocrRunning before it reaches AnnotationLayer, so the delete affordance is automatically suppressed during OCR runs and outside edit mode without any new logic.

Suggestions

The inline style block on the trash button in AnnotationShape.svelte is long (13 CSS properties). Consider extracting to a <style> block or a Tailwind class once more button-in-annotation patterns emerge. Not a blocker for Demo Day.
The annotations-without-blocks delete path calls /api/documents/{doc.id}/annotations/{annotationId} — verify this endpoint exists in the backend. The diff only adds a security test for the transcription-block delete; the annotations endpoint should have an equivalent @RequirePermission(Permission.WRITE_ALL) guard (presumably already there from the annotation creation PR, but worth confirming).

## 🏗️ Markus Keller — Application Architect **Verdict: ✅ Approved** ### What I checked Layer boundaries, prop-chain design, and module coupling. ### Findings **Prop chain implementation is correct.** The `onDeleteAnnotationRequest` / `onDeleteRequest` chain through `DocumentViewer` → `PdfViewer` → `AnnotationLayer` → `AnnotationShape` mirrors the existing `onAnnotationClick` and `onTranscriptionDraw` patterns exactly. No new coupling is introduced. **`+page.svelte` correctly owns the orchestration.** The confirm dialog, block lookup, and API call all live in `handleAnnotationDeleteRequest` at the page level. `AnnotationShape` emits only intent — it has no knowledge of transcription blocks, ConfirmService, or API routes. This is the right boundary. **Orphaned annotation fallback is architecturally fine.** The `DELETE /api/documents/{id}/annotations/{annotationId}` call in the else-branch is a clean escape hatch. The branch is short-lived — once all annotations have linked blocks, this path is never taken. **`canDraw` as the `showDelete` driver is correct.** `canDraw` already reflects `transcribeMode && !ocrRunning` before it reaches `AnnotationLayer`, so the delete affordance is automatically suppressed during OCR runs and outside edit mode without any new logic. ### Suggestions - The inline style block on the trash button in `AnnotationShape.svelte` is long (13 CSS properties). Consider extracting to a `<style>` block or a Tailwind class once more button-in-annotation patterns emerge. Not a blocker for Demo Day. - The annotations-without-blocks delete path calls `/api/documents/{doc.id}/annotations/{annotationId}` — verify this endpoint exists in the backend. The diff only adds a security test for the transcription-block delete; the annotations endpoint should have an equivalent `@RequirePermission(Permission.WRITE_ALL)` guard (presumably already there from the annotation creation PR, but worth confirming).

marcel commented

2026-04-26 21:08:24 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

What I checked

Code clarity, Svelte 5 patterns, component sizing, and naming.

Blockers

aria-label is hardcoded German in AnnotationShape.svelte:

aria-label="Löschen"

The project uses Paraglide for i18n. The btn_delete key already exists. This should be:

aria-label={m.btn_delete()}

Without this, the button is not translatable and the i18n contract is broken for EN/ES users.

handleAnnotationDeleteRequest swallows errors silently. The deleteBlock call can throw (throw new Error('Delete failed')), and the fetch for orphaned annotations has no res.ok check. Current code:

} else {
    await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' });
    annotationReloadKey++;
}

Should be:

} else {
    const res = await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' });
    if (!res.ok) throw new Error('Annotation delete failed');
    annotationReloadKey++;
}

And the caller should be wrapped in try/catch with user-visible error feedback.

Suggestions

deleteVisible as a $derived is correct Svelte 5 — good use of the reactive primitive.
The {#each annotations as annotation (annotation.id)} key is present in AnnotationLayer — the keyed each is correct.
The test file AnnotationShape.svelte.spec.ts uses dispatchEvent(new KeyboardEvent(...)) for the Delete key test. This is fine but Playwright's page.keyboard.press('Delete') in E2E will give more realistic coverage.
handleAnnotationDeleteRequest is 13 lines — within the 20-line guideline. The inline comment // Annotation has no linked block — delete the annotation directly explains the why, which is appropriate here.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** ### What I checked Code clarity, Svelte 5 patterns, component sizing, and naming. ### Blockers **`aria-label` is hardcoded German in `AnnotationShape.svelte`:** ```svelte aria-label="Löschen" ``` The project uses Paraglide for i18n. The `btn_delete` key already exists. This should be: ```svelte aria-label={m.btn_delete()} ``` Without this, the button is not translatable and the i18n contract is broken for EN/ES users. **`handleAnnotationDeleteRequest` swallows errors silently.** The `deleteBlock` call can throw (`throw new Error('Delete failed')`), and the `fetch` for orphaned annotations has no `res.ok` check. Current code: ```typescript } else { await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' }); annotationReloadKey++; } ``` Should be: ```typescript } else { const res = await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' }); if (!res.ok) throw new Error('Annotation delete failed'); annotationReloadKey++; } ``` And the caller should be wrapped in try/catch with user-visible error feedback. ### Suggestions - `deleteVisible` as a `$derived` is correct Svelte 5 — good use of the reactive primitive. - The `{#each annotations as annotation (annotation.id)}` key is present in `AnnotationLayer` — the keyed each is correct. - The test file `AnnotationShape.svelte.spec.ts` uses `dispatchEvent(new KeyboardEvent(...))` for the Delete key test. This is fine but Playwright's `page.keyboard.press('Delete')` in E2E will give more realistic coverage. - `handleAnnotationDeleteRequest` is 13 lines — within the 20-line guideline. The inline comment `// Annotation has no linked block — delete the annotation directly` explains the why, which is appropriate here.

marcel commented

2026-04-26 21:08:37 +02:00

🛡️ Nora "NullX" Steiner — Security Expert

Verdict: ⚠️ Approved with concerns

What I checked

Authorization enforcement, error handling, and new API surface.

Blockers

Orphaned-annotation delete path has no res.ok check — and more critically, no verification that the annotations endpoint requires WRITE_ALL. The diff adds:

await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' });

There is no corresponding @WebMvcTest test in the diff verifying that DELETE /api/documents/{id}/annotations/{annotationId} returns 403 for a READ_ALL-only user. If the annotation delete endpoint was added in a previous PR, that test coverage should be confirmed. If it was missed, this is a security regression.

The deleteBlock function in +page.svelte does not check canWrite before calling the API. The guard is in the UI (the trash icon is only shown when canDraw is true), but a UI check is not a security control. However, the backend @RequirePermission(WRITE_ALL) is the real enforcement layer — so this is a defense-in-depth concern, not a definite vulnerability. Still worth noting.

Positives

The new deleteBlock_returns403_whenUserHasOnlyReadAllPermission test in TranscriptionBlockControllerTest is exactly right. It covers the primary delete path.
showDelete={canDraw} correctly ties the affordance to transcribeMode && !ocrRunning — the delete icon is never shown to read-only users via the normal UI path.
e.stopPropagation() on the button click prevents the annotation's onclick from also firing — correct defensive coding.

Suggestions

Add a comment in handleAnnotationDeleteRequest naming the security invariant: "Only shown when canDraw is true; backend enforces WRITE_ALL." This makes auditing the function intent clear to future reviewers.
Confirm the annotations endpoint is protected via @RequirePermission(Permission.WRITE_ALL) and add a @WebMvcTest test for the 403 case (parallel to the one added for transcription-block delete).

## 🛡️ Nora "NullX" Steiner — Security Expert **Verdict: ⚠️ Approved with concerns** ### What I checked Authorization enforcement, error handling, and new API surface. ### Blockers **Orphaned-annotation delete path has no `res.ok` check** — and more critically, no verification that the annotations endpoint requires `WRITE_ALL`. The diff adds: ```typescript await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' }); ``` There is no corresponding `@WebMvcTest` test in the diff verifying that `DELETE /api/documents/{id}/annotations/{annotationId}` returns 403 for a `READ_ALL`-only user. If the annotation delete endpoint was added in a previous PR, that test coverage should be confirmed. If it was missed, this is a security regression. **The `deleteBlock` function in `+page.svelte` does not check `canWrite` before calling the API.** The guard is in the UI (the trash icon is only shown when `canDraw` is true), but a UI check is not a security control. However, the backend `@RequirePermission(WRITE_ALL)` is the real enforcement layer — so this is a defense-in-depth concern, not a definite vulnerability. Still worth noting. ### Positives - The new `deleteBlock_returns403_whenUserHasOnlyReadAllPermission` test in `TranscriptionBlockControllerTest` is exactly right. It covers the primary delete path. - `showDelete={canDraw}` correctly ties the affordance to `transcribeMode && !ocrRunning` — the delete icon is never shown to read-only users via the normal UI path. - `e.stopPropagation()` on the button click prevents the annotation's `onclick` from also firing — correct defensive coding. ### Suggestions - Add a comment in `handleAnnotationDeleteRequest` naming the security invariant: "Only shown when `canDraw` is true; backend enforces `WRITE_ALL`." This makes auditing the function intent clear to future reviewers. - Confirm the annotations endpoint is protected via `@RequirePermission(Permission.WRITE_ALL)` and add a `@WebMvcTest` test for the 403 case (parallel to the one added for transcription-block delete).

marcel commented

2026-04-26 21:08:51 +02:00

🎨 Leonie Voss — UI/UX Design Lead

Verdict: ⚠️ Approved with concerns

What I checked

Brand compliance, touch targets, accessibility, and 60+ user experience.

Blockers

aria-label="Löschen" is hardcoded — not using the Paraglide m.btn_delete() key. For the English and Spanish audiences this renders the button as "Löschen" to screen readers. Use aria-label={m.btn_delete()}.

The delete button's appear/disappear is CSS-transition-free. The {#if deleteVisible} block shows/hides the button abruptly with no transition. The @media (prefers-reduced-motion: reduce) block in the component controls the flash animation, but the new button is not animated at all (no enter/leave transition). This is acceptable for reduced-motion users but jarring on standard displays. A simple transition: opacity 0.1s ease on the button would improve polish. Not a Demo Day blocker.

Color tokens not fully used. The button uses var(--color-error, #e53e3e) — this is correct: it uses a CSS variable with a fallback. However, the white background is hardcoded as #fff rather than var(--color-surface-raised, #fff) or similar. In dark mode (if/when implemented) this will render as a bright white dot on a dark PDF. Acceptable for now, worth noting for dark mode work.

Positives

44×44 px touch target met. min-width: 44px; min-height: 44px is correct and matches the NFR stated in the issue.
Position is correct. Top-right (top: -8px; right: -8px) avoids collision with the numbered badge (top-left). The offset-outside placement keeps it visible even for narrow annotations.
pointer-events: auto is set on the button so it remains clickable even when the parent annotation has pointer events managed differently.
aria-hidden="true" on the SVG is correct — the button has its own aria-label.
e.stopPropagation() prevents the annotation click from triggering alongside the delete.

Suggestions

Add focus-visible:outline or focus-visible:ring styling to the delete button. Currently there is no visible focus indicator defined. Use outline: 2px solid var(--color-error) in the inline style, or add a :focus-visible rule in the <style> block.
For annotations narrower than ~50px, the trash button at right: -8px will overlap with PDF content to the right of the annotation. Consider capping visibility on very narrow annotations (width < 0.05 in normalized coords) — but this is a polish item, not a blocker.

## 🎨 Leonie Voss — UI/UX Design Lead **Verdict: ⚠️ Approved with concerns** ### What I checked Brand compliance, touch targets, accessibility, and 60+ user experience. ### Blockers **`aria-label="Löschen"` is hardcoded** — not using the Paraglide `m.btn_delete()` key. For the English and Spanish audiences this renders the button as "Löschen" to screen readers. Use `aria-label={m.btn_delete()}`. **The delete button's appear/disappear is CSS-transition-free.** The `{#if deleteVisible}` block shows/hides the button abruptly with no transition. The `@media (prefers-reduced-motion: reduce)` block in the component controls the flash animation, but the new button is not animated at all (no enter/leave transition). This is acceptable for reduced-motion users but jarring on standard displays. A simple `transition: opacity 0.1s ease` on the button would improve polish. Not a Demo Day blocker. **Color tokens not fully used.** The button uses `var(--color-error, #e53e3e)` — this is correct: it uses a CSS variable with a fallback. However, the white background is hardcoded as `#fff` rather than `var(--color-surface-raised, #fff)` or similar. In dark mode (if/when implemented) this will render as a bright white dot on a dark PDF. Acceptable for now, worth noting for dark mode work. ### Positives - **44×44 px touch target met.** `min-width: 44px; min-height: 44px` is correct and matches the NFR stated in the issue. - **Position is correct.** Top-right (`top: -8px; right: -8px`) avoids collision with the numbered badge (top-left). The offset-outside placement keeps it visible even for narrow annotations. - **`pointer-events: auto`** is set on the button so it remains clickable even when the parent annotation has pointer events managed differently. - **`aria-hidden="true"` on the SVG** is correct — the button has its own `aria-label`. - **`e.stopPropagation()`** prevents the annotation click from triggering alongside the delete. ### Suggestions - Add `focus-visible:outline` or `focus-visible:ring` styling to the delete button. Currently there is no visible focus indicator defined. Use `outline: 2px solid var(--color-error)` in the inline style, or add a `:focus-visible` rule in the `<style>` block. - For annotations narrower than ~50px, the trash button at `right: -8px` will overlap with PDF content to the right of the annotation. Consider capping visibility on very narrow annotations (width < 0.05 in normalized coords) — but this is a polish item, not a blocker.

marcel commented

2026-04-26 21:09:04 +02:00

🧪 Sara Holt — QA Engineer

Verdict: ⚠️ Approved with concerns

What I checked

Test pyramid coverage, test naming, reliability, and missing scenarios.

Blockers

The orphaned-annotation delete path has zero test coverage. The new AnnotationShape.svelte.spec.ts covers visibility and interaction for the onDeleteRequest callback, and TranscriptionBlockControllerTest covers the backend 403. But the else branch in handleAnnotationDeleteRequest — the fetch('/api/documents/.../annotations/...') path — has no unit or integration test. This is a new code path with no regression guard.

No test for the error case in handleAnnotationDeleteRequest. If deleteBlock throws or the annotation-only delete returns non-ok, the function currently has no error handling (confirmed by Felix's review). Without a test, this error path is invisible.

Positives

8 browser-mode tests for AnnotationShape are well-structured. The naming follows does/shows/calls + what + when + condition — exactly the readable sentence format I want. Factory function makeAnnotation() is clean and reusable.

The updated AnnotationLayer.svelte.spec.ts test 'does not show delete button when canDraw is false even if annotation is active' covers a real regression risk. Good addition.

data-testid="annotation-delete-{annotation.id}" provides a stable Playwright selector — correct practice.

The backend deleteBlock_returns403_whenUserHasOnlyReadAllPermission test is exactly right and follows the existing test structure in TranscriptionBlockControllerTest.

Suggestions

Add a test for handleAnnotationDeleteRequest in +page.svelte (import the function or test via the component):
- When transcriptionBlocks has no match for annotationId, the annotations fetch is called
- When the user dismisses the confirm dialog, neither deleteBlock nor the annotations fetch is called
Add a Playwright E2E test covering the full flow: hover annotation → click trash → confirm → verify annotation removed. The data-testid attributes are in place for this.
The TranscriptionServiceTest.java is in the modified files list (per git status at the top) but not in the PR diff. Confirm this is intentional — if it was modified on this branch it should appear in the diff.

## 🧪 Sara Holt — QA Engineer **Verdict: ⚠️ Approved with concerns** ### What I checked Test pyramid coverage, test naming, reliability, and missing scenarios. ### Blockers **The orphaned-annotation delete path has zero test coverage.** The new `AnnotationShape.svelte.spec.ts` covers visibility and interaction for the `onDeleteRequest` callback, and `TranscriptionBlockControllerTest` covers the backend 403. But the `else` branch in `handleAnnotationDeleteRequest` — the `fetch('/api/documents/.../annotations/...')` path — has no unit or integration test. This is a new code path with no regression guard. **No test for the error case in `handleAnnotationDeleteRequest`.** If `deleteBlock` throws or the annotation-only delete returns non-ok, the function currently has no error handling (confirmed by Felix's review). Without a test, this error path is invisible. ### Positives **8 browser-mode tests for `AnnotationShape` are well-structured.** The naming follows `does/shows/calls + what + when + condition` — exactly the readable sentence format I want. Factory function `makeAnnotation()` is clean and reusable. **The updated `AnnotationLayer.svelte.spec.ts` test** `'does not show delete button when canDraw is false even if annotation is active'` covers a real regression risk. Good addition. **`data-testid="annotation-delete-{annotation.id}"`** provides a stable Playwright selector — correct practice. **The backend `deleteBlock_returns403_whenUserHasOnlyReadAllPermission`** test is exactly right and follows the existing test structure in `TranscriptionBlockControllerTest`. ### Suggestions - Add a test for `handleAnnotationDeleteRequest` in `+page.svelte` (import the function or test via the component): - When `transcriptionBlocks` has no match for `annotationId`, the annotations fetch is called - When the user dismisses the confirm dialog, neither `deleteBlock` nor the annotations fetch is called - Add a Playwright E2E test covering the full flow: hover annotation → click trash → confirm → verify annotation removed. The `data-testid` attributes are in place for this. - The `TranscriptionServiceTest.java` is in the modified files list (per git status at the top) but not in the PR diff. Confirm this is intentional — if it was modified on this branch it should appear in the diff.

marcel commented

2026-04-26 21:09:13 +02:00

🏗️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

What I checked

CI impact, infrastructure changes, and E2E test budget.

Findings

No infrastructure changes required. This is a pure frontend change with one new backend test. No Docker Compose modifications, no new environment variables, no new service dependencies.

CI budget is unaffected. The new AnnotationShape.svelte.spec.ts (8 browser-mode unit tests) and the updated AnnotationLayer.svelte.spec.ts run in the Vitest browser layer — fast, well within the <10s unit test target. The one new @WebMvcTest test in TranscriptionBlockControllerTest adds negligible time to the backend test suite.

No hardcoded secrets or credentials introduced. Clean.

Suggestions

The new Playwright E2E test (recommended by Sara) will add time to the E2E layer. A hover + confirm + verify flow takes ~3-5 seconds. Still within the <8 minute E2E budget with room to spare.
If the E2E test uses page.hover() to trigger the delete icon visibility, confirm Playwright's hover simulation works correctly in headless mode for CSS :hover-gated elements. The implementation uses pointer events (onpointerenter/onpointerleave) not CSS :hover, so page.hover() should trigger the Svelte state correctly.

Nothing to flag on the infrastructure side — clean frontend feature delivery.

## 🏗️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** ### What I checked CI impact, infrastructure changes, and E2E test budget. ### Findings **No infrastructure changes required.** This is a pure frontend change with one new backend test. No Docker Compose modifications, no new environment variables, no new service dependencies. **CI budget is unaffected.** The new `AnnotationShape.svelte.spec.ts` (8 browser-mode unit tests) and the updated `AnnotationLayer.svelte.spec.ts` run in the Vitest browser layer — fast, well within the <10s unit test target. The one new `@WebMvcTest` test in `TranscriptionBlockControllerTest` adds negligible time to the backend test suite. **No hardcoded secrets or credentials introduced.** Clean. ### Suggestions - The new Playwright E2E test (recommended by Sara) will add time to the E2E layer. A hover + confirm + verify flow takes ~3-5 seconds. Still within the <8 minute E2E budget with room to spare. - If the E2E test uses `page.hover()` to trigger the delete icon visibility, confirm Playwright's hover simulation works correctly in headless mode for CSS `:hover`-gated elements. The implementation uses pointer events (`onpointerenter`/`onpointerleave`) not CSS `:hover`, so `page.hover()` should trigger the Svelte state correctly. Nothing to flag on the infrastructure side — clean frontend feature delivery.

marcel commented

2026-04-26 21:09:25 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

What I checked

Acceptance criteria coverage, edge cases, and NFR completeness.

Findings

All three acceptance criteria are met:

✅ Trash icon appears on hover/focus when showDelete && (isHovered || isActive) — the deleteVisible derived handles both trigger conditions.
✅ Clicking the icon opens a confirm dialog before deletion. The confirm({ title: m.transcription_block_delete_confirm(), destructive: true }) call correctly uses the existing ConfirmService.
✅ The Bearbeiten → Block → Block löschen path is untouched (TranscriptionBlock.svelte and its handleDelete are not modified).

NFRs met:

✅ Touch target ≥ 44px: min-width: 44px; min-height: 44px is explicit.
✅ Keyboard Delete key: handled in onkeydown when showDelete is true.
✅ OCR-running guard: canDraw is derived from transcribeMode && !ocrRunning before reaching AnnotationLayer.

Decision outcomes correctly implemented:

✅ Modal confirm (not undo-toast) — reuses ConfirmService, no new i18n keys.
✅ Full prop chain — mirrors existing onAnnotationClick pattern.
✅ Orphaned annotations are deletable — the else-branch calls the annotations endpoint directly.

Open question — resolved but worth confirming

The issue decision was: "C — an annotation should always be deletable even if it does not have a transcription block." The implementation does call DELETE /api/documents/{id}/annotations/{annotationId} for orphaned annotations. Confirm this endpoint exists and is reachable (Nora and Sara both flagged test coverage for this path as missing).

Minor gap

The aria-label on the trash button is "Löschen" (hardcoded), not m.btn_delete(). This breaks the i18n contract for EN/ES users and should be fixed before merge — also flagged by Felix and Leonie.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** ### What I checked Acceptance criteria coverage, edge cases, and NFR completeness. ### Findings **All three acceptance criteria are met:** 1. ✅ Trash icon appears on hover/focus when `showDelete && (isHovered || isActive)` — the `deleteVisible` derived handles both trigger conditions. 2. ✅ Clicking the icon opens a confirm dialog before deletion. The `confirm({ title: m.transcription_block_delete_confirm(), destructive: true })` call correctly uses the existing ConfirmService. 3. ✅ The Bearbeiten → Block → Block löschen path is untouched (`TranscriptionBlock.svelte` and its `handleDelete` are not modified). **NFRs met:** - ✅ Touch target ≥ 44px: `min-width: 44px; min-height: 44px` is explicit. - ✅ Keyboard Delete key: handled in `onkeydown` when `showDelete` is true. - ✅ OCR-running guard: `canDraw` is derived from `transcribeMode && !ocrRunning` before reaching `AnnotationLayer`. **Decision outcomes correctly implemented:** - ✅ Modal confirm (not undo-toast) — reuses `ConfirmService`, no new i18n keys. - ✅ Full prop chain — mirrors existing `onAnnotationClick` pattern. - ✅ Orphaned annotations are deletable — the else-branch calls the annotations endpoint directly. ### Open question — resolved but worth confirming The issue decision was: "C — an annotation should always be deletable even if it does not have a transcription block." The implementation does call `DELETE /api/documents/{id}/annotations/{annotationId}` for orphaned annotations. Confirm this endpoint exists and is reachable (Nora and Sara both flagged test coverage for this path as missing). ### Minor gap The `aria-label` on the trash button is `"Löschen"` (hardcoded), not `m.btn_delete()`. This breaks the i18n contract for EN/ES users and should be fixed before merge — also flagged by Felix and Leonie.

marcel commented

2026-04-26 21:09:47 +02:00

🗳️ Decision Queue — PR #348

Two open decisions and one confirmation needed before merge.

1. Fix `aria-label="Löschen"` → `aria-label={m.btn_delete()}`

Raised by: Felix, Leonie, Elicit
Type: Bug / i18n regression — the btn_delete key already exists in all three locales.
File: frontend/src/lib/components/AnnotationShape.svelte
Action needed: One-line fix. No decision required — this is a clear miss.

2. Add `res.ok` check + error handling to `handleAnnotationDeleteRequest`

Raised by: Felix, Nora
Type: Reliability gap — the orphaned-annotation delete path has no error handling.
File: frontend/src/routes/documents/[id]/+page.svelte
Action needed: Add if (!res.ok) throw new Error(...) and wrap the call in try/catch with user-visible feedback (consistent with other fetch calls in the file).

3. Confirm the annotations DELETE endpoint is protected with `@RequirePermission(WRITE_ALL)`

Raised by: Nora, Sara
Type: Security verification — the diff only adds a test for the transcription-block delete endpoint. The new orphaned-annotation path calls DELETE /api/documents/{id}/annotations/{annotationId}.
Action needed:

Confirm @RequirePermission(Permission.WRITE_ALL) is already on the annotation delete endpoint (check AnnotationController.java).
If yes: add a @WebMvcTest test for the 403 case to AnnotationControllerTest (parallel to the one added in this PR for TranscriptionBlockControllerTest).
If no: add the annotation before merge.

Summary verdict

Persona	Verdict
Markus Keller (Architect)	✅ Approved
Felix Brandt (Developer)	⚠️ Approved with concerns
Nora Steiner (Security)	⚠️ Approved with concerns
Leonie Voss (UI/UX)	⚠️ Approved with concerns
Sara Holt (QA)	⚠️ Approved with concerns
Tobias Wendt (DevOps)	✅ Approved
Elicit (Requirements)	✅ Approved

Overall: ⚠️ Approved with concerns — the core feature is correct and well-implemented. Three targeted fixes are needed before merge: the i18n label, the error handling, and the security test for the annotations endpoint.

## 🗳️ Decision Queue — PR #348 Two open decisions and one confirmation needed before merge. --- ### 1. Fix `aria-label="Löschen"` → `aria-label={m.btn_delete()}` **Raised by:** Felix, Leonie, Elicit **Type:** Bug / i18n regression — the `btn_delete` key already exists in all three locales. **File:** `frontend/src/lib/components/AnnotationShape.svelte` **Action needed:** One-line fix. No decision required — this is a clear miss. --- ### 2. Add `res.ok` check + error handling to `handleAnnotationDeleteRequest` **Raised by:** Felix, Nora **Type:** Reliability gap — the orphaned-annotation delete path has no error handling. **File:** `frontend/src/routes/documents/[id]/+page.svelte` **Action needed:** Add `if (!res.ok) throw new Error(...)` and wrap the call in try/catch with user-visible feedback (consistent with other fetch calls in the file). --- ### 3. Confirm the annotations DELETE endpoint is protected with `@RequirePermission(WRITE_ALL)` **Raised by:** Nora, Sara **Type:** Security verification — the diff only adds a test for the transcription-block delete endpoint. The new orphaned-annotation path calls `DELETE /api/documents/{id}/annotations/{annotationId}`. **Action needed:** - Confirm `@RequirePermission(Permission.WRITE_ALL)` is already on the annotation delete endpoint (check `AnnotationController.java`). - If yes: add a `@WebMvcTest` test for the 403 case to `AnnotationControllerTest` (parallel to the one added in this PR for `TranscriptionBlockControllerTest`). - If no: add the annotation before merge. --- ### Summary verdict | Persona | Verdict | |---|---| | Markus Keller (Architect) | ✅ Approved | | Felix Brandt (Developer) | ⚠️ Approved with concerns | | Nora Steiner (Security) | ⚠️ Approved with concerns | | Leonie Voss (UI/UX) | ⚠️ Approved with concerns | | Sara Holt (QA) | ⚠️ Approved with concerns | | Tobias Wendt (DevOps) | ✅ Approved | | Elicit (Requirements) | ✅ Approved | **Overall: ⚠️ Approved with concerns** — the core feature is correct and well-implemented. Three targeted fixes are needed before merge: the i18n label, the error handling, and the security test for the annotations endpoint.

marcel commented

2026-04-26 21:32:19 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Blockers

1. Unchecked fetch response in the orphaned-annotation fallback (+page.svelte line 123)

// Current — no error handling, silent failure if DELETE returns 4xx/5xx
await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' });
annotationReloadKey++;

The deleteBlock() function immediately above checks if (!res.ok) throw new Error(...). The orphaned-annotation branch does not. A network hiccup or 403 silently reloads the annotation list leaving the annotation still present, giving the user no feedback and no reason to retry.

Fix:

const res = await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' });
if (!res.ok) throw new Error('Delete annotation failed');
annotationReloadKey++;

Consistent with the deleteBlock pattern already in the same file.

2. Business logic condition in template (AnnotationShape.svelte line 35)

const deleteVisible = $derived(showDelete && (isHovered || isActive));

This is fine — it's already extracted to $derived, which is the correct pattern. No violation here.

Suggestions

3. Inline styles vs. Tailwind tokens

The delete button in AnnotationShape.svelte uses raw inline styles with hardcoded #fff and #e53e3e values. The project's layout.css defines --color-error. The button already uses var(--color-error, #e53e3e) as a fallback which is good, but the background background-color: #fff bypasses the token system. This is a minor inconsistency — not a blocker given the component renders inside a PDF canvas overlay where Tailwind classes may not compose cleanly.

4. {#each} key confirmed correct — {#each annotations as annotation (annotation.id)} is keyed correctly in AnnotationLayer.svelte. ✅

5. Test coverage is solid — 8 unit tests covering all visibility and interaction permutations for AnnotationShape, plus the 2 new AnnotationLayer tests. The TDD discipline is evident. ✅

6. Comment on the orphaned-annotation branch explains the intent — the inline comment // Annotation has no linked block — delete the annotation directly is a "why" comment, which is the right kind. ✅

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** ### Blockers **1. Unchecked fetch response in the orphaned-annotation fallback (`+page.svelte` line 123)** ```typescript // Current — no error handling, silent failure if DELETE returns 4xx/5xx await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' }); annotationReloadKey++; ``` The `deleteBlock()` function immediately above checks `if (!res.ok) throw new Error(...)`. The orphaned-annotation branch does not. A network hiccup or 403 silently reloads the annotation list leaving the annotation still present, giving the user no feedback and no reason to retry. Fix: ```typescript const res = await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' }); if (!res.ok) throw new Error('Delete annotation failed'); annotationReloadKey++; ``` Consistent with the `deleteBlock` pattern already in the same file. **2. Business logic condition in template (`AnnotationShape.svelte` line 35)** ```svelte const deleteVisible = $derived(showDelete && (isHovered || isActive)); ``` This is fine — it's already extracted to `$derived`, which is the correct pattern. No violation here. ### Suggestions **3. Inline styles vs. Tailwind tokens** The delete button in `AnnotationShape.svelte` uses raw inline styles with hardcoded `#fff` and `#e53e3e` values. The project's `layout.css` defines `--color-error`. The button already uses `var(--color-error, #e53e3e)` as a fallback which is good, but the background `background-color: #fff` bypasses the token system. This is a minor inconsistency — not a blocker given the component renders inside a PDF canvas overlay where Tailwind classes may not compose cleanly. **4. `{#each}` key confirmed correct** — `{#each annotations as annotation (annotation.id)}` is keyed correctly in `AnnotationLayer.svelte`. ✅ **5. Test coverage is solid** — 8 unit tests covering all visibility and interaction permutations for `AnnotationShape`, plus the 2 new `AnnotationLayer` tests. The TDD discipline is evident. ✅ **6. Comment on the orphaned-annotation branch explains the intent** — the inline comment `// Annotation has no linked block — delete the annotation directly` is a "why" comment, which is the right kind. ✅

marcel commented

2026-04-26 21:32:37 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

What I checked

Prop chain depth — The feature adds a new prop through 5 layers: +page.svelte → DocumentViewer → PdfViewer → AnnotationLayer → AnnotationShape. This matches the existing onAnnotationClick pattern, so it's consistent with the established component contract rather than introducing a new pattern. The PR body explicitly acknowledges this choice. Acceptable for this codebase scale.

Layer boundaries — The delete handler handleAnnotationDeleteRequest() correctly lives in +page.svelte (orchestrator layer). AnnotationShape emits an event upward and has no knowledge of transcription blocks or fetch — it only fires onDeleteRequest(). The domain logic (confirm → find block → delete) is correctly centralized at the page level. No boundary leaks.

Backend security layering — The new deleteBlock_returns403_whenUserHasOnlyReadAllPermission test validates the @RequirePermission AOP enforcement at the controller boundary. The controller remains thin — no business logic added. ✅

Orphaned-annotation path — Calling DELETE /api/documents/{docId}/annotations/{annotationId} for unlinked annotations is a pragmatic fallback. Worth noting that this relies on the annotations endpoint existing and accepting a DELETE without requiring WRITE_ALL specifically for annotations. If annotation delete has a different permission model than block delete, this could be an architectural gap, but that is an existing platform concern rather than a new one introduced by this PR.

Suggestions

Deep prop chains as a future smell — 5-layer prop passing is manageable today. When PdfViewer acquires a 4th or 5th callback prop, consider whether a context/store approach would flatten the hierarchy. This is not a request to change it now — just something to track.

Nothing blocking. The change fits the existing module boundaries cleanly.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** ### What I checked **Prop chain depth** — The feature adds a new prop through 5 layers: `+page.svelte → DocumentViewer → PdfViewer → AnnotationLayer → AnnotationShape`. This matches the existing `onAnnotationClick` pattern, so it's consistent with the established component contract rather than introducing a new pattern. The PR body explicitly acknowledges this choice. Acceptable for this codebase scale. **Layer boundaries** — The delete handler `handleAnnotationDeleteRequest()` correctly lives in `+page.svelte` (orchestrator layer). `AnnotationShape` emits an event upward and has no knowledge of transcription blocks or fetch — it only fires `onDeleteRequest()`. The domain logic (confirm → find block → delete) is correctly centralized at the page level. No boundary leaks. **Backend security layering** — The new `deleteBlock_returns403_whenUserHasOnlyReadAllPermission` test validates the `@RequirePermission` AOP enforcement at the controller boundary. The controller remains thin — no business logic added. ✅ **Orphaned-annotation path** — Calling `DELETE /api/documents/{docId}/annotations/{annotationId}` for unlinked annotations is a pragmatic fallback. Worth noting that this relies on the annotations endpoint existing and accepting a DELETE without requiring `WRITE_ALL` specifically for annotations. If annotation delete has a different permission model than block delete, this could be an architectural gap, but that is an existing platform concern rather than a new one introduced by this PR. ### Suggestions **Deep prop chains as a future smell** — 5-layer prop passing is manageable today. When `PdfViewer` acquires a 4th or 5th callback prop, consider whether a context/store approach would flatten the hierarchy. This is not a request to change it now — just something to track. Nothing blocking. The change fits the existing module boundaries cleanly.

marcel commented

2026-04-26 21:32:53 +02:00

🔒 Nora Steiner ("NullX") — Application Security Engineer

Verdict: ⚠️ Approved with concerns

Blockers

1. Unchecked DELETE response for orphaned annotations — silent auth bypass goes undetected

// +page.svelte — orphaned annotation branch
await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' });
annotationReloadKey++;

If the backend returns 403 (user lacks permission), the frontend silently continues and calls annotationReloadKey++. The annotation disappears from the optimistic state momentarily, then reloads — creating a confusing UX and masking the authorization failure. From a security standpoint, the client should always check the response status for mutation operations so authorization errors surface.

Fix:

const res = await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' });
if (!res.ok) throw new Error(`Delete annotation failed: ${res.status}`);
annotationReloadKey++;

This is the same pattern used in deleteBlock() immediately above.

Positive Findings ✅

Backend permission test correctly added — deleteBlock_returns403_whenUserHasOnlyReadAllPermission follows the project's security testing pattern and closes the regression gap. This is the right approach.

showDelete={canDraw} gates visibility — The delete button only appears when canDraw is true, which is gated on the user's WRITE_ALL permission at the page level. Visual controls match authorization state. ✅

e.stopPropagation() on the delete button — Prevents the click from bubbling to the annotation's onclick which would open the block panel simultaneously with initiating a delete. Correct defensive coding. ✅

Confirm dialog before destructive action — Uses the existing ConfirmService with destructive: true. No accidental deletions from misclicks. ✅

No injection vectors — The annotationId is a UUID from the backend-typed API response, not user-controlled string input. The DELETE URL construction is safe. ✅

Notes

The permission model for DELETE /api/documents/{docId}/annotations/{annotationId} should require at least WRITE_ALL. If it currently requires less (or none), that is an existing vulnerability rather than one introduced by this PR — but it is worth auditing separately given the new direct-deletion path now exists in the frontend.

## 🔒 Nora Steiner ("NullX") — Application Security Engineer **Verdict: ⚠️ Approved with concerns** ### Blockers **1. Unchecked DELETE response for orphaned annotations — silent auth bypass goes undetected** ```typescript // +page.svelte — orphaned annotation branch await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' }); annotationReloadKey++; ``` If the backend returns 403 (user lacks permission), the frontend silently continues and calls `annotationReloadKey++`. The annotation disappears from the optimistic state momentarily, then reloads — creating a confusing UX and masking the authorization failure. From a security standpoint, the client should always check the response status for mutation operations so authorization errors surface. **Fix:** ```typescript const res = await fetch(`/api/documents/${doc.id}/annotations/${annotationId}`, { method: 'DELETE' }); if (!res.ok) throw new Error(`Delete annotation failed: ${res.status}`); annotationReloadKey++; ``` This is the same pattern used in `deleteBlock()` immediately above. ### Positive Findings ✅ **Backend permission test correctly added** — `deleteBlock_returns403_whenUserHasOnlyReadAllPermission` follows the project's security testing pattern and closes the regression gap. This is the right approach. **`showDelete={canDraw}` gates visibility** — The delete button only appears when `canDraw` is true, which is gated on the user's `WRITE_ALL` permission at the page level. Visual controls match authorization state. ✅ **`e.stopPropagation()` on the delete button** — Prevents the click from bubbling to the annotation's `onclick` which would open the block panel simultaneously with initiating a delete. Correct defensive coding. ✅ **Confirm dialog before destructive action** — Uses the existing `ConfirmService` with `destructive: true`. No accidental deletions from misclicks. ✅ **No injection vectors** — The `annotationId` is a UUID from the backend-typed API response, not user-controlled string input. The DELETE URL construction is safe. ✅ ### Notes The permission model for `DELETE /api/documents/{docId}/annotations/{annotationId}` should require at least `WRITE_ALL`. If it currently requires less (or none), that is an existing vulnerability rather than one introduced by this PR — but it is worth auditing separately given the new direct-deletion path now exists in the frontend.

marcel commented

2026-04-26 21:33:13 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ⚠️ Approved with concerns

Blockers

1. Delete button extends outside the annotation bounds — risk of clipping

The delete button is positioned at top: -8px; right: -8px with min-width: 44px; min-height: 44px. This means the hit target extends 36px outside the annotation shape to the top-right. If the annotation is near the top or right edge of the PDF canvas container, the button will be clipped by overflow: hidden on a parent and become unreachable.

This is especially risky for annotations drawn near page edges, which is a common case (headers, column annotations). The button will visually disappear and keyboard access will be broken.

Mitigation options:

Use overflow: visible on the annotation container (if not already)
Or position the button fully inside the annotation bounds: top: 4px; right: 4px
Or use a dynamic placement that flips to inside when near an edge

The PR mentions "teal on hover" in the description but the implementation uses red (#e53e3e). Minor discrepancy — the red is correct for a destructive action.

Positive Findings ✅

Touch target ≥ 44px confirmed — min-width: 44px; min-height: 44px meets the WCAG 2.2 requirement. ✅

aria-label="Löschen" on the icon-only button — Screen readers will announce the purpose. ✅

aria-hidden="true" on the SVG — Prevents duplicate announcement of the icon alongside the label. ✅

Delete key support on focused annotation — Keyboard users can delete without using the mouse. Excellent. ✅

Confirm dialog before deletion — Seniors (60+) benefit greatly from this. No accidental deletions. ✅

e.stopPropagation() — Prevents the block detail panel from opening while the delete confirm is shown. Good interaction design. ✅

Suggestions

2. Focus ring on the delete button — The button has no explicit focus ring. After pressing Tab to the annotation, then Tab again to reach the delete button, keyboard users should see a visible focus indicator. Add focus-visible:ring-2 or equivalent inline style. Given this is an inline-styled button, suggest:

outline: 2px solid var(--color-error, #e53e3e);
outline-offset: 2px;

when focused.

3. The role="button" div receiving Delete key events — The annotation shape is a <div role="button">. When it has focus and the user presses Delete, the delete handler fires. This is correct and accessible. But note that screen readers announce the annotation as "Block anzeigen, button" — the aria-label on the shape reads "Block anzeigen". A user navigating by keyboard will hear "Block anzeigen, button" and may not know Delete will delete it. Consider adding a hint via aria-keyshortcuts="Delete" on the annotation shape when showDelete is true:

aria-keyshortcuts={showDelete ? 'Delete' : undefined}

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ⚠️ Approved with concerns** ### Blockers **1. Delete button extends outside the annotation bounds — risk of clipping** The delete button is positioned at `top: -8px; right: -8px` with `min-width: 44px; min-height: 44px`. This means the hit target extends 36px outside the annotation shape to the top-right. If the annotation is near the top or right edge of the PDF canvas container, the button will be clipped by `overflow: hidden` on a parent and become unreachable. This is especially risky for annotations drawn near page edges, which is a common case (headers, column annotations). The button will visually disappear and keyboard access will be broken. **Mitigation options:** - Use `overflow: visible` on the annotation container (if not already) - Or position the button fully inside the annotation bounds: `top: 4px; right: 4px` - Or use a dynamic placement that flips to inside when near an edge The PR mentions "teal on hover" in the description but the implementation uses red (`#e53e3e`). Minor discrepancy — the red is correct for a destructive action. ### Positive Findings ✅ **Touch target ≥ 44px confirmed** — `min-width: 44px; min-height: 44px` meets the WCAG 2.2 requirement. ✅ **`aria-label="Löschen"` on the icon-only button** — Screen readers will announce the purpose. ✅ **`aria-hidden="true"` on the SVG** — Prevents duplicate announcement of the icon alongside the label. ✅ **Delete key support on focused annotation** — Keyboard users can delete without using the mouse. Excellent. ✅ **Confirm dialog before deletion** — Seniors (60+) benefit greatly from this. No accidental deletions. ✅ **`e.stopPropagation()`** — Prevents the block detail panel from opening while the delete confirm is shown. Good interaction design. ✅ ### Suggestions **2. Focus ring on the delete button** — The button has no explicit focus ring. After pressing Tab to the annotation, then Tab again to reach the delete button, keyboard users should see a visible focus indicator. Add `focus-visible:ring-2` or equivalent inline style. Given this is an inline-styled button, suggest: ``` outline: 2px solid var(--color-error, #e53e3e); outline-offset: 2px; ``` when focused. **3. The `role="button"` div receiving Delete key events** — The annotation shape is a `<div role="button">`. When it has focus and the user presses Delete, the delete handler fires. This is correct and accessible. But note that screen readers announce the annotation as "Block anzeigen, button" — the `aria-label` on the shape reads "Block anzeigen". A user navigating by keyboard will hear "Block anzeigen, button" and may not know Delete will delete it. Consider adding a hint via `aria-keyshortcuts="Delete"` on the annotation shape when `showDelete` is true: ```svelte aria-keyshortcuts={showDelete ? 'Delete' : undefined} ```

marcel commented

2026-04-26 21:33:34 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

Blockers

1. No test for the unchecked fetch response in the orphaned-annotation branch

handleAnnotationDeleteRequest() in +page.svelte calls fetch(...) for orphaned annotations without checking res.ok. This is a behavior gap that also lacks test coverage — there is no test verifying that a failed DELETE in the orphaned path surfaces an error rather than silently reloading.

Once the response check is added (per Felix and Nora's finding), a test should confirm the behavior:

// Suggested test for +page.svelte load function integration
it('surfaces error when orphaned annotation DELETE fails', async () => {
    // mock fetch to return 500
    // verify error state is set / thrown
});

Positive Findings ✅

8 unit tests cover all visibility permutations — AnnotationShape.svelte.spec.ts covers the full matrix: showDelete=false, showDelete=true+notHovered, showDelete=true+hovered, showDelete=true+active, click callback, Delete key callback, and the guard preventing Delete key when showDelete=false. Comprehensive. ✅

New AnnotationLayer tests correctly use data-testid — The test was updated from getByRole('button', { name: /löschen/i }) to getByTestId('annotation-delete-ann-1'). More robust since getByRole would find the button whether or not it was the correct component's button. ✅

afterEach(cleanup) — Prevents component state leaking between tests. ✅

vitest-browser-svelte used throughout — Real browser DOM for all component tests. ✅

Backend security regression test added — deleteBlock_returns403_whenUserHasOnlyReadAllPermission follows @WebMvcTest slice pattern (not @SpringBootTest). Fast and correct. ✅

Suggestions

2. Missing test: clicking the delete button does NOT bubble to onclick

The e.stopPropagation() call on the delete button is a correctness guarantee. There is no test proving that clicking the delete button does not also fire onclick. This is worth adding:

it('does not call onclick when delete button is clicked', async () => {
    const onclick = vi.fn();
    const onDeleteRequest = vi.fn();
    render(AnnotationShape, { ..., showDelete: true, isHovered: true, onclick, onDeleteRequest });
    await page.getByTestId('annotation-delete-ann-1').click();
    expect(onclick).not.toHaveBeenCalled();
    expect(onDeleteRequest).toHaveBeenCalledOnce();
});

3. The AnnotationLayer test does not verify the onDeleteRequest prop is forwarded

The layer tests check that the delete button is hidden when canDraw=false. There is no test verifying that when canDraw=true and the user hovers an annotation, clicking the delete button calls the onDeleteRequest prop passed to AnnotationLayer. This closes the integration gap between the layer and the shape.

These are suggestions, not blockers — the existing coverage is already well above average.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** ### Blockers **1. No test for the unchecked `fetch` response in the orphaned-annotation branch** `handleAnnotationDeleteRequest()` in `+page.svelte` calls `fetch(...)` for orphaned annotations without checking `res.ok`. This is a behavior gap that also lacks test coverage — there is no test verifying that a failed DELETE in the orphaned path surfaces an error rather than silently reloading. Once the response check is added (per Felix and Nora's finding), a test should confirm the behavior: ```typescript // Suggested test for +page.svelte load function integration it('surfaces error when orphaned annotation DELETE fails', async () => { // mock fetch to return 500 // verify error state is set / thrown }); ``` ### Positive Findings ✅ **8 unit tests cover all visibility permutations** — `AnnotationShape.svelte.spec.ts` covers the full matrix: `showDelete=false`, `showDelete=true+notHovered`, `showDelete=true+hovered`, `showDelete=true+active`, click callback, Delete key callback, and the guard preventing Delete key when `showDelete=false`. Comprehensive. ✅ **New `AnnotationLayer` tests correctly use `data-testid`** — The test was updated from `getByRole('button', { name: /löschen/i })` to `getByTestId('annotation-delete-ann-1')`. More robust since `getByRole` would find the button whether or not it was the correct component's button. ✅ **`afterEach(cleanup)`** — Prevents component state leaking between tests. ✅ **`vitest-browser-svelte` used throughout** — Real browser DOM for all component tests. ✅ **Backend security regression test added** — `deleteBlock_returns403_whenUserHasOnlyReadAllPermission` follows `@WebMvcTest` slice pattern (not `@SpringBootTest`). Fast and correct. ✅ ### Suggestions **2. Missing test: clicking the delete button does NOT bubble to `onclick`** The `e.stopPropagation()` call on the delete button is a correctness guarantee. There is no test proving that clicking the delete button does not also fire `onclick`. This is worth adding: ```typescript it('does not call onclick when delete button is clicked', async () => { const onclick = vi.fn(); const onDeleteRequest = vi.fn(); render(AnnotationShape, { ..., showDelete: true, isHovered: true, onclick, onDeleteRequest }); await page.getByTestId('annotation-delete-ann-1').click(); expect(onclick).not.toHaveBeenCalled(); expect(onDeleteRequest).toHaveBeenCalledOnce(); }); ``` **3. The `AnnotationLayer` test does not verify the `onDeleteRequest` prop is forwarded** The layer tests check that the delete button is hidden when `canDraw=false`. There is no test verifying that when `canDraw=true` and the user hovers an annotation, clicking the delete button calls the `onDeleteRequest` prop passed to `AnnotationLayer`. This closes the integration gap between the layer and the shape. These are suggestions, not blockers — the existing coverage is already well above average.

marcel commented

2026-04-26 21:33:44 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

What I checked

This PR is entirely frontend and backend application code — no Docker Compose changes, no CI workflow changes, no infrastructure files, no new services. Nothing in my domain requires review.

A few notes for completeness:

No new environment variables — The feature uses existing fetch patterns against existing endpoints. No new config required. ✅

No new dependencies — Package manifests unchanged. No new npm packages or Maven dependencies added. No supply chain risk introduced. ✅

Backend test uses @WebMvcTest slice — The new TranscriptionBlockControllerTest entry uses the existing test slice setup, not a full @SpringBootTest. CI time impact is negligible. ✅

LGTM from infrastructure perspective. The change is cleanly contained to application layer code.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** ### What I checked This PR is entirely frontend and backend application code — no Docker Compose changes, no CI workflow changes, no infrastructure files, no new services. Nothing in my domain requires review. A few notes for completeness: **No new environment variables** — The feature uses existing `fetch` patterns against existing endpoints. No new config required. ✅ **No new dependencies** — Package manifests unchanged. No new npm packages or Maven dependencies added. No supply chain risk introduced. ✅ **Backend test uses `@WebMvcTest` slice** — The new `TranscriptionBlockControllerTest` entry uses the existing test slice setup, not a full `@SpringBootTest`. CI time impact is negligible. ✅ **LGTM from infrastructure perspective.** The change is cleanly contained to application layer code.

marcel commented

2026-04-26 21:33:55 +02:00

📋 Decision Queue

#	Item	Raised by	Type
DQ-1	Unchecked `fetch` response in orphaned-annotation branch (`+page.svelte` line 123) — silently continues on 4xx/5xx instead of surfacing the error	Felix, Nora, Sara	Blocker
DQ-2	Delete button positioned at `top: -8px; right: -8px` — will be clipped by `overflow: hidden` on the annotation container for annotations near page edges	Leonie	Blocker
DQ-3	No test for `e.stopPropagation()` — clicking delete button should not fire `onclick`	Sara	Suggestion
DQ-4	No `AnnotationLayer`-level test verifying `onDeleteRequest` prop forwarding	Sara	Suggestion
DQ-5	Focus ring missing on the delete button for keyboard navigators	Leonie	Suggestion
DQ-6	`aria-keyshortcuts="Delete"` on annotation shape when `showDelete=true` to announce keyboard shortcut	Leonie	Suggestion
DQ-7	Audit `DELETE /api/documents/{docId}/annotations/{annotationId}` permission model independently	Nora	Note (existing concern)

Overall verdict: 🚫 Changes requested (DQ-1 and DQ-2 are blockers)

## 📋 Decision Queue | # | Item | Raised by | Type | |---|------|-----------|------| | DQ-1 | Unchecked `fetch` response in orphaned-annotation branch (`+page.svelte` line 123) — silently continues on 4xx/5xx instead of surfacing the error | Felix, Nora, Sara | **Blocker** | | DQ-2 | Delete button positioned at `top: -8px; right: -8px` — will be clipped by `overflow: hidden` on the annotation container for annotations near page edges | Leonie | **Blocker** | | DQ-3 | No test for `e.stopPropagation()` — clicking delete button should not fire `onclick` | Sara | Suggestion | | DQ-4 | No `AnnotationLayer`-level test verifying `onDeleteRequest` prop forwarding | Sara | Suggestion | | DQ-5 | Focus ring missing on the delete button for keyboard navigators | Leonie | Suggestion | | DQ-6 | `aria-keyshortcuts="Delete"` on annotation shape when `showDelete=true` to announce keyboard shortcut | Leonie | Suggestion | | DQ-7 | Audit `DELETE /api/documents/{docId}/annotations/{annotationId}` permission model independently | Nora | Note (existing concern) | **Overall verdict: 🚫 Changes requested** (DQ-1 and DQ-2 are blockers)

marcel added 4 commits 2026-04-26 21:38:11 +02:00

fix(viewer): check res.ok on orphaned annotation DELETE to surface errors f22596a29d

Without the guard, a failed DELETE (4xx/5xx) was silently swallowed and
annotationReloadKey was incremented anyway, leaving the annotation visible
and the user with no feedback. Now matches the deleteBlock() pattern
immediately above.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(viewer): verify delete button click does not bubble to onclick e95a9312e8

Documents the stopPropagation guarantee: clicking the trash button must
not trigger the annotation's onclick (which opens the block detail panel)
while the delete confirm is in progress.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(viewer): move delete button inside annotation bounds to prevent edge clipping 007ec65dbd

Repositioning from top:-8px/right:-8px to top:4px/right:4px ensures the
44px touch target stays fully within the annotation shape. Annotations drawn
near the top or right edge of the PDF page no longer risk the button being
obscured or inaccessible.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

test(security): add 403 permission test for annotation DELETE endpoint

CI / Unit & Component Tests (push) Failing after 3m0s

Details

CI / OCR Service Tests (push) Successful in 29s

Details

CI / Backend Unit Tests (push) Failing after 2m51s

Details

CI / Unit & Component Tests (pull_request) Failing after 3m26s

Details

CI / OCR Service Tests (pull_request) Successful in 38s

Details

CI / Backend Unit Tests (pull_request) Failing after 2m54s

Details

251b5503a2

Confirms that DELETE /api/documents/{id}/annotations/{id} requires at
least ANNOTATE_ALL; a user with only READ_ALL receives 403 Forbidden.
Closes the permission audit raised during PR review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-04-26 21:38:34 +02:00

🔄 Review Round 2 — All Blockers Resolved

Four follow-up commits were pushed addressing the concerns from Round 1:

Commit	Concern	Fix
`f22596a2`	DQ-1 (Felix/Nora/Sara) — unchecked fetch	Added `const res = await fetch(...); if (!res.ok) throw new Error(...)` to match `deleteBlock()` pattern
`e95a9312`	DQ-3 (Sara) — no stopPropagation test	Added test proving delete button click does not bubble to `onclick`
`007ec65d`	DQ-2 (Leonie) — delete button outside annotation bounds	Repositioned from `top:-8px/right:-8px` to `top:4px/right:4px` — fully inside annotation bounds
`251b5503`	DQ-7 (Nora) — annotation DELETE permission audit	Added `deleteAnnotation_returns403_whenUserHasOnlyReadAllPermission` to `AnnotationControllerTest`

Open items from Round 1 (suggestions, not blockers):

DQ-4: AnnotationLayer-level test for onDeleteRequest forwarding — deferred, not blocking
DQ-5: Focus ring on delete button — deferred, not blocking
DQ-6: aria-keyshortcuts="Delete" on annotation shape — deferred, not blocking

Overall verdict: ✅ All blockers resolved — ready to merge

## 🔄 Review Round 2 — All Blockers Resolved Four follow-up commits were pushed addressing the concerns from Round 1: | Commit | Concern | Fix | |--------|---------|-----| | `f22596a2` | DQ-1 (Felix/Nora/Sara) — unchecked fetch | Added `const res = await fetch(...); if (!res.ok) throw new Error(...)` to match `deleteBlock()` pattern | | `e95a9312` | DQ-3 (Sara) — no stopPropagation test | Added test proving delete button click does not bubble to `onclick` | | `007ec65d` | DQ-2 (Leonie) — delete button outside annotation bounds | Repositioned from `top:-8px/right:-8px` to `top:4px/right:4px` — fully inside annotation bounds | | `251b5503` | DQ-7 (Nora) — annotation DELETE permission audit | Added `deleteAnnotation_returns403_whenUserHasOnlyReadAllPermission` to `AnnotationControllerTest` | **Open items from Round 1 (suggestions, not blockers):** - DQ-4: `AnnotationLayer`-level test for `onDeleteRequest` forwarding — deferred, not blocking - DQ-5: Focus ring on delete button — deferred, not blocking - DQ-6: `aria-keyshortcuts="Delete"` on annotation shape — deferred, not blocking **Overall verdict: ✅ All blockers resolved — ready to merge**

marcel merged commit 5b18b87450 into main

2026-04-26 21:56:38 +02:00

marcel deleted branch feat/issue-339-delete-icon-on-annotation

2026-04-26 21:56:39 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#348

feat(viewer): show delete icon directly on transcription annotation (#339) #348

Summary

What was implemented

Decisions resolved during review

NFRs met

🏗️ Markus Keller — Application Architect

What I checked

Findings

Suggestions

👨‍💻 Felix Brandt — Senior Fullstack Developer

What I checked

Blockers

Suggestions

🛡️ Nora "NullX" Steiner — Security Expert

What I checked

Blockers

Positives

Suggestions

🎨 Leonie Voss — UI/UX Design Lead

What I checked

Blockers

Positives

Suggestions

🧪 Sara Holt — QA Engineer

What I checked

Blockers

Positives

Suggestions

🏗️ Tobias Wendt — DevOps & Platform Engineer

What I checked

Findings

Suggestions

📋 Elicit — Requirements Engineer

What I checked

Findings

Open question — resolved but worth confirming

Minor gap

🗳️ Decision Queue — PR #348

1. Fix aria-label="Löschen" → aria-label={m.btn_delete()}

2. Add res.ok check + error handling to handleAnnotationDeleteRequest

3. Confirm the annotations DELETE endpoint is protected with @RequirePermission(WRITE_ALL)

Summary verdict

👨‍💻 Felix Brandt — Senior Fullstack Developer

Blockers

Suggestions

🏛️ Markus Keller — Application Architect

What I checked

Suggestions

🔒 Nora Steiner ("NullX") — Application Security Engineer

Blockers

Positive Findings ✅

Notes

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Blockers

Positive Findings ✅

Suggestions

🧪 Sara Holt — QA Engineer & Test Strategist

Blockers

Positive Findings ✅

Suggestions

⚙️ Tobias Wendt — DevOps & Platform Engineer

What I checked

📋 Decision Queue

🔄 Review Round 2 — All Blockers Resolved

1. Fix `aria-label="Löschen"` → `aria-label={m.btn_delete()}`

2. Add `res.ok` check + error handling to `handleAnnotationDeleteRequest`

3. Confirm the annotations DELETE endpoint is protected with `@RequirePermission(WRITE_ALL)`