feat(annotations): resize and move annotations in document view #235

marcel · 2026-04-14T13:16:41+02:00

marcel commented

2026-04-14 13:16:41 +02:00

Closes #233

Summary

PATCH endpoint (/api/annotations/{id}): accepts partial updates for x, y, width, height with a V33 migration adding a chk_annotation_bounds CHECK constraint.
AnnotationEditOverlay: SVG overlay rendered over a selected annotation in transcribe mode, with 4 corner L-bracket handles and 4 edge midpoint handles for resizing, plus full-annotation drag for moving.
Pixel-space viewBox: handles are drawn in pixel coordinates so they stay square regardless of the annotation's aspect ratio.
Keyboard accessibility: handles and the drag surface are focusable (tabIndex=0) and respond to arrow keys.
Error display: updateAnnotation is wired into the PdfViewer context; failures surface as an inline error message.
i18n: ANNOTATION_UPDATE_FAILED error code and annotation_edit_mode_active translation key added to all three locales.

Test plan

Open a document in transcribe mode, select an annotation — edit overlay appears
Drag a corner handle to resize — annotation updates on release
Drag an edge midpoint handle to resize in one axis — correct axis only changes
Drag the annotation body to move it — position updates on release
Use arrow keys on a focused handle — resizes/moves by 1px per keypress
Trigger a backend error (network off) — inline error message shown
Non-square annotation: confirm corner handles remain square (pixel-space viewBox)

🤖 Generated with Claude Code

Closes #233 ## Summary - **PATCH endpoint** (`/api/annotations/{id}`): accepts partial updates for `x`, `y`, `width`, `height` with a `V33` migration adding a `chk_annotation_bounds` CHECK constraint. - **`AnnotationEditOverlay`**: SVG overlay rendered over a selected annotation in transcribe mode, with 4 corner L-bracket handles and 4 edge midpoint handles for resizing, plus full-annotation drag for moving. - **Pixel-space viewBox**: handles are drawn in pixel coordinates so they stay square regardless of the annotation's aspect ratio. - **Keyboard accessibility**: handles and the drag surface are focusable (`tabIndex=0`) and respond to arrow keys. - **Error display**: `updateAnnotation` is wired into the PdfViewer context; failures surface as an inline error message. - **i18n**: `ANNOTATION_UPDATE_FAILED` error code and `annotation_edit_mode_active` translation key added to all three locales. ## Test plan - [ ] Open a document in transcribe mode, select an annotation — edit overlay appears - [ ] Drag a corner handle to resize — annotation updates on release - [ ] Drag an edge midpoint handle to resize in one axis — correct axis only changes - [ ] Drag the annotation body to move it — position updates on release - [ ] Use arrow keys on a focused handle — resizes/moves by 1px per keypress - [ ] Trigger a backend error (network off) — inline error message shown - [ ] Non-square annotation: confirm corner handles remain square (pixel-space viewBox) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 13 commits 2026-04-14 13:16:42 +02:00

migration(annotations): add chk_annotation_bounds CHECK constraint (V33) f76a6c0ee5

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

feat(annotations): add ANNOTATION_UPDATE_FAILED error code 26c7181ba4

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

feat(annotations): add updateAnnotation service method with partial-update DTO 1558881c01

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

feat(annotations): add PATCH endpoint for annotation resize/move ff231db671

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

feat(i18n): add ANNOTATION_UPDATE_FAILED error code and annotation_edit_mode_active translation 953cb2c910

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

feat(annotations): add AnnotationEditOverlay component with resize handles and drag f5362a5850

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

feat(annotations): add isResizable prop to AnnotationShape to render edit overlay 3b756cd718

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

feat(annotations): pass isResizable to AnnotationShape based on selection + transcribeMode 3fb32ea285

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

feat(annotations): wire updateAnnotation context and error display into PdfViewer c610a3cc37

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

fix(annotations): use pixel-space viewBox so handles stay square on non-square annotations e7f88a4ea1

ResizeObserver binds actual SVG pixel dimensions; viewBox matches them so
16px handle squares and 44px hit areas are physically correct regardless of
the annotation's aspect ratio.

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

refactor(annotations): replace 8-square handles with 4 corner L-brackets fcc0efbf02

- 4 corner-only handles (nw/ne/sw/se), no edge midpoints
- Each handle renders as two short perpendicular lines meeting at the corner
  (10px arms, navy, square linecap) — no fill, no box
- Thin dashed selection border added to SVG overlay to signal edit mode
- Simplify applyHandleDrag to remove dead n/s/e/w branches

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

feat(annotations): add N/S/E/W edge midpoint handles to resize overlay 9fe5b32a69

Extends the 4-corner L-bracket handles with 4 tick-mark edge handles
(short lines along each edge), enabling single-axis resize from any edge.
Updates applyHandleDrag to route each handle to the correct axis.

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

fix(annotations): make resize overlay keyboard-interactive

CI / Unit & Component Tests (push) Failing after 1s

Details

CI / Backend Unit Tests (push) Failing after 1s

Details

CI / Unit & Component Tests (pull_request) Failing after 2s

Details

CI / Backend Unit Tests (pull_request) Failing after 1s

Details

2350b4f845

- Add tabindex="0" so the SVG can receive DOM focus
- Auto-focus the SVG on mount so arrow keys work immediately after
  clicking an annotation to select it
- Show preview rect during keyboard nudging (not just pointer drag) by
  checking hasLiveChanges instead of only checking dragState
- Suppress default browser focus outline (outline: none) on the SVG

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

marcel commented

2026-04-14 13:18:12 +02:00

Felix Brandt (@felixbrandt) — Developer Review

Verdict: APPROVE with minor suggestions

The implementation is clean and well-structured. TDD was followed correctly: service tests cover the not-found path and the partial-update logic; controller tests cover both permission paths plus validation boundaries. The component split is sensible — AnnotationEditOverlay is a well-named, single-purpose component.

Findings

1. Missing @Slf4j on AnnotationService (minor)
The service already has a @Transactional method doing a find + save. If the DB constraint fires (the new chk_annotation_bounds from V33), the exception propagates as a raw DataIntegrityViolationException, not a DomainException. This is likely fine in practice because the DTO's @DecimalMin/@DecimalMax should catch it first — but a defense-in-depth catch and a log line would close that gap:

try {
    return annotationRepository.save(annotation);
} catch (DataIntegrityViolationException e) {
    log.warn("Annotation bounds constraint violated for {}: {}", annotationId, e.getMessage());
    throw DomainException.badRequest(ErrorCode.ANNOTATION_UPDATE_FAILED, "Bounds out of range");
}

2. ANNOTATION_UPDATE_FAILED is documented as HTTP 500 but the service never throws it (minor)
The ErrorCode Javadoc says /* ... 500 */. That code is never thrown by the service — the frontend uses it as a fallback string when no code is returned. Mismatched doc is harmless but confusing. Either remove the HTTP status hint from the Javadoc, or actually throw it from the service when the DB constraint fires (see #1 above).

3. AnnotationEditOverlay is 324 lines — splitting signal is 40 lines of template (minor)
The component is logically one visual concern (the edit overlay) and the size is mostly justified by the complex drag math. No split needed — but the handleKeyDown debounce logic could move to a $derived-style helper or a small extracted function to keep the event handler thin.

**4. handlePointerUp silently swallows the error on rollback — no user feedback from catch {}** (minor)** On a failed save, liveX/Yare restored, but there is nothrowout of thecatchblock. TheupdateAnnotationcontext function does throw (perPdfViewer), but the overlay itself does not relay the rejection to any local state. The error banner is shown in PdfViewerbecauseupdateAnnotation` throws — this is fine. Just worth noting that the overlay relies on the context function rethrowing to trigger the banner.

5. Keyboard rollback in handleKeyDown only resets X/Y, not width/height (minor)

// catch block in handleKeyDown:
liveX = annotation.x;
liveY = annotation.y;
// liveWidth and liveHeight are NOT reset

If a keyboard resize (arrow keys on a resize handle) fails, width and height stay at the post-key value while the backend has the old value. Width/height are not modified by the current keyboard handler (it only moves), so this is not a bug today — but it is a latent inconsistency that will matter if keyboard resize is added later. Suggest aligning with the pointer-up rollback:

liveX = annotation.x; liveY = annotation.y;
liveWidth = annotation.width; liveHeight = annotation.height;

6. Service unit test: AnnotationServiceTest.updateAnnotation_updatesOnlyPresentFields — missing assertion on save being called (cosmetic)
The test asserts on the returned values but not that annotationRepository.save(annotation) was called once. verify(annotationRepository).save(annotation) would make the test more complete.

Overall this is solid work. The pixel-space viewBox approach for square handles is an elegant solution. Test coverage across all three layers (unit, controller slice, component) is thorough. The five minor points are suggestions, not blockers.

## Felix Brandt (@felixbrandt) — Developer Review **Verdict: APPROVE with minor suggestions** The implementation is clean and well-structured. TDD was followed correctly: service tests cover the not-found path and the partial-update logic; controller tests cover both permission paths plus validation boundaries. The component split is sensible — `AnnotationEditOverlay` is a well-named, single-purpose component. --- ### Findings **1. Missing `@Slf4j` on `AnnotationService` (minor)** The service already has a `@Transactional` method doing a find + save. If the DB constraint fires (the new `chk_annotation_bounds` from V33), the exception propagates as a raw `DataIntegrityViolationException`, not a `DomainException`. This is likely fine in practice because the DTO's `@DecimalMin`/`@DecimalMax` should catch it first — but a defense-in-depth catch and a log line would close that gap: ```java try { return annotationRepository.save(annotation); } catch (DataIntegrityViolationException e) { log.warn("Annotation bounds constraint violated for {}: {}", annotationId, e.getMessage()); throw DomainException.badRequest(ErrorCode.ANNOTATION_UPDATE_FAILED, "Bounds out of range"); } ``` **2. `ANNOTATION_UPDATE_FAILED` is documented as HTTP 500 but the service never throws it (minor)** The `ErrorCode` Javadoc says `/* ... 500 */`. That code is never thrown by the service — the frontend uses it as a fallback string when no code is returned. Mismatched doc is harmless but confusing. Either remove the HTTP status hint from the Javadoc, or actually throw it from the service when the DB constraint fires (see #1 above). **3. `AnnotationEditOverlay` is 324 lines — splitting signal is 40 lines of template (minor)** The component is logically one visual concern (the edit overlay) and the size is mostly justified by the complex drag math. No split needed — but the `handleKeyDown` debounce logic could move to a `$derived`-style helper or a small extracted function to keep the event handler thin. **4. `handlePointerUp` silently swallows the error on rollback — no user feedback from `catch {}** (minor)** On a failed save, `liveX/Y` are restored, but there is no `throw` out of the `catch` block. The `updateAnnotation` context function does throw (per `PdfViewer`), but the overlay itself does not relay the rejection to any local state. The error banner is shown in `PdfViewer` because `updateAnnotation` throws — this is fine. Just worth noting that the overlay relies on the context function rethrowing to trigger the banner. **5. Keyboard rollback in `handleKeyDown` only resets X/Y, not width/height (minor)** ```typescript // catch block in handleKeyDown: liveX = annotation.x; liveY = annotation.y; // liveWidth and liveHeight are NOT reset ``` If a keyboard resize (arrow keys on a resize handle) fails, width and height stay at the post-key value while the backend has the old value. Width/height are not modified by the current keyboard handler (it only moves), so this is not a bug today — but it is a latent inconsistency that will matter if keyboard resize is added later. Suggest aligning with the pointer-up rollback: ```typescript liveX = annotation.x; liveY = annotation.y; liveWidth = annotation.width; liveHeight = annotation.height; ``` **6. Service unit test: `AnnotationServiceTest.updateAnnotation_updatesOnlyPresentFields` — missing assertion on save being called (cosmetic)** The test asserts on the returned values but not that `annotationRepository.save(annotation)` was called once. `verify(annotationRepository).save(annotation)` would make the test more complete. --- Overall this is solid work. The pixel-space viewBox approach for square handles is an elegant solution. Test coverage across all three layers (unit, controller slice, component) is thorough. The five minor points are suggestions, not blockers.

marcel commented

2026-04-14 13:18:29 +02:00

Markus Keller (@mkeller) — Architect Review

Verdict: APPROVE with one structural note

The implementation follows the project's layering rules correctly. The controller stays thin, the service owns all domain logic, the DTO is kept in dto/ where it belongs, and there is no cross-domain repository access. The Flyway migration is appropriately minimal.

Findings

1. The PATCH /api/documents/{documentId}/annotations/{annotationId} URL scope is correct — good
Nesting the annotation under the document route ensures the document ID is part of the ownership check (findByIdAndDocumentId). This is the right pattern; it prevents IDOR by design.

2. setContext('annotationUpdate', updateAnnotation) places a server-side concern in a client-side context — structural note (minor)
setContext is called unconditionally at module level in PdfViewer.svelte. The updateAnnotation function uses fetch directly (bypassing the typed API client). This is the documented pattern for multipart endpoints in CLAUDE.md, so the raw fetch is acceptable here. However, the context is set even when PdfViewer is not in transcribe mode and there is no active annotation. This is harmless today but couples the context provisioning to rendering lifetime rather than feature activation. Consider: is there a scenario where two PdfViewer instances on the same page conflict over the context key? If yes, scope the context to the overlay's parent, not the viewer root.

3. UpdateAnnotationDTO vs DocumentUpdateDTO — good separation
Creating a dedicated UpdateAnnotationDTO instead of reusing DocumentUpdateDTO is the right call. Shared DTOs between unrelated features create silent coupling. The new DTO is self-contained and independently validatable.

4. V33 migration is additive only — correct
Adding a CHECK constraint to existing data can fail if rows already violate it. Given that all annotation coordinates are written by controlled code, this should be safe. However, the migration comment does not acknowledge this risk. Adding a note like -- All existing rows must satisfy this constraint; if any do not, this migration will fail. would be consistent with the project's migration documentation style seen in earlier migrations.

5. ANNOTATION_UPDATE_FAILED error code in ErrorCode.java is documented as HTTP 500 but is never thrown from the service (minor)
This was already noted by Felix. From an architecture standpoint: if the error code exists in the enum, a corresponding throw site should exist. Either remove the enum value and rely on the default getErrorMessage() fallback in the frontend, or add the throw site as described in the developer review. Having dead enum values creates confusion during maintenance.

6. No ADR needed — the pixel-space viewBox decision is an implementation detail, not an architectural choice.
Good call on not over-documenting this.

The layering is clean, the migration is sound, and the feature boundary is well-contained. The structural note on context scoping is worth discussing but not a blocker for this PR.

## Markus Keller (@mkeller) — Architect Review **Verdict: APPROVE with one structural note** The implementation follows the project's layering rules correctly. The controller stays thin, the service owns all domain logic, the DTO is kept in `dto/` where it belongs, and there is no cross-domain repository access. The Flyway migration is appropriately minimal. --- ### Findings **1. The `PATCH /api/documents/{documentId}/annotations/{annotationId}` URL scope is correct — good** Nesting the annotation under the document route ensures the document ID is part of the ownership check (`findByIdAndDocumentId`). This is the right pattern; it prevents IDOR by design. **2. `setContext('annotationUpdate', updateAnnotation)` places a server-side concern in a client-side context — structural note (minor)** `setContext` is called unconditionally at module level in `PdfViewer.svelte`. The `updateAnnotation` function uses `fetch` directly (bypassing the typed API client). This is the documented pattern for multipart endpoints in `CLAUDE.md`, so the raw `fetch` is acceptable here. However, the context is set even when `PdfViewer` is not in transcribe mode and there is no active annotation. This is harmless today but couples the context provisioning to rendering lifetime rather than feature activation. Consider: is there a scenario where two `PdfViewer` instances on the same page conflict over the context key? If yes, scope the context to the overlay's parent, not the viewer root. **3. `UpdateAnnotationDTO` vs `DocumentUpdateDTO` — good separation** Creating a dedicated `UpdateAnnotationDTO` instead of reusing `DocumentUpdateDTO` is the right call. Shared DTOs between unrelated features create silent coupling. The new DTO is self-contained and independently validatable. **4. V33 migration is additive only — correct** Adding a `CHECK` constraint to existing data can fail if rows already violate it. Given that all annotation coordinates are written by controlled code, this should be safe. However, the migration comment does not acknowledge this risk. Adding a note like `-- All existing rows must satisfy this constraint; if any do not, this migration will fail.` would be consistent with the project's migration documentation style seen in earlier migrations. **5. `ANNOTATION_UPDATE_FAILED` error code in `ErrorCode.java` is documented as HTTP 500 but is never thrown from the service (minor)** This was already noted by Felix. From an architecture standpoint: if the error code exists in the enum, a corresponding throw site should exist. Either remove the enum value and rely on the default `getErrorMessage()` fallback in the frontend, or add the throw site as described in the developer review. Having dead enum values creates confusion during maintenance. **6. No ADR needed — the pixel-space viewBox decision is an implementation detail, not an architectural choice.** Good call on not over-documenting this. --- The layering is clean, the migration is sound, and the feature boundary is well-contained. The structural note on context scoping is worth discussing but not a blocker for this PR.

marcel commented

2026-04-14 13:18:48 +02:00

Sara Holt (@saraholt) — QA / Test Review

Verdict: APPROVE — test coverage is solid, with two gaps worth addressing

The test pyramid is well-represented: Mockito unit tests for the service, @WebMvcTest slice for the controller, and vitest-browser-svelte component tests for the three frontend components. Test names are sentence-style and descriptive. Factory patterns are reused correctly.

Findings

1. BLOCKER (soft): No integration test for the V33 migration constraint
The chk_annotation_bounds CHECK constraint is added in V33, but there is no Testcontainers integration test that verifies the constraint actually fires when out-of-bounds values reach the database. The controller test at layer exercises Bean Validation (@DecimalMin/@DecimalMax), which rejects bad input before it reaches the DB. But the test does not verify what happens if someone bypasses the DTO validation (e.g., via a future internal call) — the DB constraint is the last line of defense and it should be tested:

@SpringBootTest
@Import(PostgresContainerConfig.class)
@Transactional
class AnnotationBoundsConstraintTest {
    @Autowired AnnotationRepository annotationRepository;

    @Test
    void should_reject_annotation_with_x_above_1() {
        DocumentAnnotation bad = DocumentAnnotation.builder()
            .x(1.5).y(0.1).width(0.1).height(0.1)
            /* ...other required fields... */
            .build();
        assertThatThrownBy(() -> {
            annotationRepository.save(bad);
            annotationRepository.flush(); // force the write
        }).isInstanceOf(DataIntegrityViolationException.class);
    }
}

This test also verifies Flyway ran V33 correctly. The constraint is tested at the layer it actually exists.

2. Missing test: handleKeyDown rollback resets only X/Y, not width/height
Felix flagged this as a code smell; from a QA perspective, there should be a test that confirms the rollback is complete after a failed keyboard nudge. Once the production code is fixed, a Vitest unit test on the rollback behaviour would pin it permanently.

3. Component tests use document.querySelectorAll instead of role-based queries (minor)

// current — tests internal structure
const handles = document.querySelectorAll('[data-handle]');
// preferred — tests visible behaviour
const svg = screen.getByRole('application', { name: /resize handles/i });

The data-handle attribute approach is acceptable for structural assertions (handle count), but the accessibility-related tests (tabindex, aria-live) should use getByRole / getByLabelText from @testing-library/svelte or Vitest's page locators to test what users and assistive technology actually experience, not internal DOM attributes.

4. AnnotationControllerTest.patchAnnotation_returns400_withWidthBelowMinimum — good, but missing symmetrical test for height (minor)
The test only verifies width: 0.005. A parallel test for height: 0.005 and for x: 1.1 (above max) would make the validation coverage exhaustive.

5. Service test updateAnnotation_updatesOnlyPresentFields — good partial-update test (positive callout)
Testing that unchanged fields stay unchanged is exactly the right assertion for a PATCH endpoint. This test would catch a regression if someone accidentally changed the if (dto.getX() != null) guards.

6. No E2E test for the drag-to-resize flow (acceptable, noting gap)
The test plan in the PR description lists manual steps. Given the complexity of pointer event simulation in Playwright, an E2E test here would be high-value but also expensive to write. Logging a follow-up issue (#236: E2E Playwright test for annotation drag resize) is a reasonable trade-off for this PR.

The soft blocker on the constraint integration test is the only item I would ask to be resolved before merge. Everything else is a suggestion or a follow-up.

## Sara Holt (@saraholt) — QA / Test Review **Verdict: APPROVE — test coverage is solid, with two gaps worth addressing** The test pyramid is well-represented: Mockito unit tests for the service, `@WebMvcTest` slice for the controller, and `vitest-browser-svelte` component tests for the three frontend components. Test names are sentence-style and descriptive. Factory patterns are reused correctly. --- ### Findings **1. BLOCKER (soft): No integration test for the V33 migration constraint** The `chk_annotation_bounds` CHECK constraint is added in V33, but there is no Testcontainers integration test that verifies the constraint actually fires when out-of-bounds values reach the database. The controller test at layer exercises Bean Validation (`@DecimalMin`/`@DecimalMax`), which rejects bad input before it reaches the DB. But the test does not verify what happens if someone bypasses the DTO validation (e.g., via a future internal call) — the DB constraint is the last line of defense and it should be tested: ```java @SpringBootTest @Import(PostgresContainerConfig.class) @Transactional class AnnotationBoundsConstraintTest { @Autowired AnnotationRepository annotationRepository; @Test void should_reject_annotation_with_x_above_1() { DocumentAnnotation bad = DocumentAnnotation.builder() .x(1.5).y(0.1).width(0.1).height(0.1) /* ...other required fields... */ .build(); assertThatThrownBy(() -> { annotationRepository.save(bad); annotationRepository.flush(); // force the write }).isInstanceOf(DataIntegrityViolationException.class); } } ``` This test also verifies Flyway ran V33 correctly. The constraint is tested at the layer it actually exists. **2. Missing test: `handleKeyDown` rollback resets only X/Y, not width/height** Felix flagged this as a code smell; from a QA perspective, there should be a test that confirms the rollback is complete after a failed keyboard nudge. Once the production code is fixed, a Vitest unit test on the rollback behaviour would pin it permanently. **3. Component tests use `document.querySelectorAll` instead of role-based queries (minor)** ```typescript // current — tests internal structure const handles = document.querySelectorAll('[data-handle]'); // preferred — tests visible behaviour const svg = screen.getByRole('application', { name: /resize handles/i }); ``` The `data-handle` attribute approach is acceptable for structural assertions (handle count), but the accessibility-related tests (tabindex, aria-live) should use `getByRole` / `getByLabelText` from `@testing-library/svelte` or Vitest's `page` locators to test what users and assistive technology actually experience, not internal DOM attributes. **4. `AnnotationControllerTest.patchAnnotation_returns400_withWidthBelowMinimum` — good, but missing symmetrical test for `height` (minor)** The test only verifies `width: 0.005`. A parallel test for `height: 0.005` and for `x: 1.1` (above max) would make the validation coverage exhaustive. **5. Service test `updateAnnotation_updatesOnlyPresentFields` — good partial-update test (positive callout)** Testing that unchanged fields stay unchanged is exactly the right assertion for a PATCH endpoint. This test would catch a regression if someone accidentally changed the `if (dto.getX() != null)` guards. **6. No E2E test for the drag-to-resize flow (acceptable, noting gap)** The test plan in the PR description lists manual steps. Given the complexity of pointer event simulation in Playwright, an E2E test here would be high-value but also expensive to write. Logging a follow-up issue (`#236: E2E Playwright test for annotation drag resize`) is a reasonable trade-off for this PR. --- The soft blocker on the constraint integration test is the only item I would ask to be resolved before merge. Everything else is a suggestion or a follow-up.

marcel commented

2026-04-14 13:19:09 +02:00

Nora "NullX" Steiner — Security Review

Verdict: APPROVE — no critical vulnerabilities; one IDOR concern to verify

The security posture of this PR is generally sound. Permission checks use the existing @RequirePermission AOP mechanism correctly, and the new endpoint follows the document-scoped URL pattern that prevents cross-document annotation access. Here is my full analysis.

Findings

1. IDOR check — findByIdAndDocumentId is the right pattern (positive, with a note)
The service uses annotationRepository.findByIdAndDocumentId(annotationId, documentId) to scope the annotation lookup to the document. This is the correct defense against IDOR: an attacker who knows annotationId from document A cannot move it by sending a PATCH to document B's URL.

Verify the repository method signature: I cannot see AnnotationRepository.java in this diff. Please confirm that findByIdAndDocumentId is implemented as a Spring Data derived query (or an explicit @Query) and is not doing a findById followed by an application-layer check. The DB query must be the gate, not application code.

2. @RequirePermission({Permission.ANNOTATE_ALL, Permission.WRITE_ALL}) — correct (positive)
The PATCH endpoint accepts either ANNOTATE_ALL or WRITE_ALL, which matches the existing DELETE endpoint's permission set. A user who can annotate should be able to move and resize annotations. A user who only has READ_ALL cannot. This is consistent and tested by the controller tests.

3. No authentication bypass risk from documentId in path vs. ownership (info)
The URL is /api/documents/{documentId}/annotations/{annotationId}. The documentId is used as a query scope parameter, not to authorize access to the document itself. A user with ANNOTATE_ALL can annotate any document. This is consistent with existing CREATE and DELETE behaviour — there is no document-level ownership enforcement for annotations in this system. This is an existing design decision, not introduced by this PR.

4. Input validation is present at both layers (positive)
UpdateAnnotationDTO uses @DecimalMin/@DecimalMax on all four fields, and the controller applies @Valid. The Flyway V33 migration adds a CHECK constraint as a second layer. Defense in depth is implemented correctly.

5. fetch in PdfViewer.svelte sends user-controlled data as JSON — no XSS risk
The coords object is serialized with JSON.stringify and sent with Content-Type: application/json. It is not reflected into the DOM. No injection risk here.

6. Error message exposure — acceptable
On failure, getErrorMessage(err?.code ?? 'ANNOTATION_UPDATE_FAILED') maps the backend error code to an i18n string. Raw backend messages, stack traces, and class names are never shown to the user. This is the correct pattern.

7. annotationUpdateError auto-clears after 4 seconds — minor UX-security note
The 4-second auto-dismiss is acceptable here (it is an error notification, not a success notification). The aria-live="assertive" on the alert div means screen readers will announce it before it disappears. No security issue; just confirming the dismiss timer does not create a race condition where an error is silently lost.

8. The catch {} in AnnotationEditOverlay.handlePointerUp has no error variable — potential silent swallow

} catch {
    liveX = ds.preDragX;
    // ...
}

In TypeScript catch {} without a binding is valid but hides what was caught. If updateAnnotation throws something unexpected (not a network error), it will be silently rolled back. This is a code-quality concern, not a security vulnerability, but in a security-sensitive path (user action that modifies data) I prefer catch (err) { console.error('annotation update failed', err); ... } to preserve observability.

No blockers from a security perspective. Verify finding #1 (repository method is a DB-scoped query) and you are good to go.

## Nora "NullX" Steiner — Security Review **Verdict: APPROVE — no critical vulnerabilities; one IDOR concern to verify** The security posture of this PR is generally sound. Permission checks use the existing `@RequirePermission` AOP mechanism correctly, and the new endpoint follows the document-scoped URL pattern that prevents cross-document annotation access. Here is my full analysis. --- ### Findings **1. IDOR check — `findByIdAndDocumentId` is the right pattern (positive, with a note)** The service uses `annotationRepository.findByIdAndDocumentId(annotationId, documentId)` to scope the annotation lookup to the document. This is the correct defense against IDOR: an attacker who knows `annotationId` from document A cannot move it by sending a PATCH to document B's URL. **Verify the repository method signature:** I cannot see `AnnotationRepository.java` in this diff. Please confirm that `findByIdAndDocumentId` is implemented as a Spring Data derived query (or an explicit `@Query`) and is not doing a `findById` followed by an application-layer check. The DB query must be the gate, not application code. **2. `@RequirePermission({Permission.ANNOTATE_ALL, Permission.WRITE_ALL})` — correct (positive)** The PATCH endpoint accepts either `ANNOTATE_ALL` or `WRITE_ALL`, which matches the existing `DELETE` endpoint's permission set. A user who can annotate should be able to move and resize annotations. A user who only has `READ_ALL` cannot. This is consistent and tested by the controller tests. **3. No authentication bypass risk from `documentId` in path vs. ownership (info)** The URL is `/api/documents/{documentId}/annotations/{annotationId}`. The `documentId` is used as a query scope parameter, not to authorize access to the document itself. A user with `ANNOTATE_ALL` can annotate any document. This is consistent with existing `CREATE` and `DELETE` behaviour — there is no document-level ownership enforcement for annotations in this system. This is an existing design decision, not introduced by this PR. **4. Input validation is present at both layers (positive)** `UpdateAnnotationDTO` uses `@DecimalMin`/`@DecimalMax` on all four fields, and the controller applies `@Valid`. The Flyway V33 migration adds a `CHECK` constraint as a second layer. Defense in depth is implemented correctly. **5. `fetch` in `PdfViewer.svelte` sends user-controlled data as JSON — no XSS risk** The `coords` object is serialized with `JSON.stringify` and sent with `Content-Type: application/json`. It is not reflected into the DOM. No injection risk here. **6. Error message exposure — acceptable** On failure, `getErrorMessage(err?.code ?? 'ANNOTATION_UPDATE_FAILED')` maps the backend error code to an i18n string. Raw backend messages, stack traces, and class names are never shown to the user. This is the correct pattern. **7. `annotationUpdateError` auto-clears after 4 seconds — minor UX-security note** The 4-second auto-dismiss is acceptable here (it is an error notification, not a success notification). The `aria-live="assertive"` on the alert div means screen readers will announce it before it disappears. No security issue; just confirming the dismiss timer does not create a race condition where an error is silently lost. **8. The `catch {}` in `AnnotationEditOverlay.handlePointerUp` has no error variable — potential silent swallow** ```typescript } catch { liveX = ds.preDragX; // ... } ``` In TypeScript `catch {}` without a binding is valid but hides what was caught. If `updateAnnotation` throws something unexpected (not a network error), it will be silently rolled back. This is a code-quality concern, not a security vulnerability, but in a security-sensitive path (user action that modifies data) I prefer `catch (err) { console.error('annotation update failed', err); ... }` to preserve observability. --- No blockers from a security perspective. Verify finding #1 (repository method is a DB-scoped query) and you are good to go.

marcel commented

2026-04-14 13:19:34 +02:00

Leonie Voss (@leonievoss) — UI/UX & Accessibility Review

Verdict: APPROVE with accessibility improvements requested before merge

The feature is functional and the drag interaction model is well-conceived. The pixel-space viewBox approach for handles is smart — it avoids the squished-handle problem on narrow annotations. My review focuses on accessibility and brand compliance.

Findings

1. BLOCKER: Handle <g> elements have role="none" but are interactive — missing keyboard access
Each handle <g> responds to onpointerdown but has role="none" and no tabindex. The PR description says handles are keyboard-accessible via arrow keys on the SVG root, but that only provides move functionality (nudging X/Y). There is no way to keyboard-navigate to a specific corner handle and resize from it. The SVG root tabindex="0" captures all keyboard events; individual handles are mouse-only.

This is a real accessibility gap for users who cannot use a pointer. Recommendation:

<g
  data-handle={handle.id}
  role="button"
  tabindex="0"
  aria-label="Resize {handle.id} handle"
  onpointerdown={(e) => handlePointerDown(e, 'handle', handle.id)}
  onkeydown={(e) => {
    if (e.key === 'Enter' || e.key === ' ') {
      // activate resize mode for this handle
    }
  }}
>

At minimum, the handles need tabindex="0" and aria-label with a meaningful description (e.g., "Resize from top-left corner"). Without this, keyboard-only users can only move annotations, not resize them.

2. BLOCKER: aria-label="Annotation resize handles" on the SVG is in English only
The label is hardcoded in English. All user-visible strings in this project use Paraglide for i18n. Add a translation key:

// de.json
"annotation_resize_area": "Annotation-Größe und Position ändern"
// en.json  
"annotation_resize_area": "Resize and reposition annotation"
// es.json
"annotation_resize_area": "Cambiar tamaño y posición de la anotación"

aria-label={m.annotation_resize_area()}

3. Focus ring missing on the SVG root
The SVG has style="... outline: none;". This removes the browser focus indicator for keyboard users navigating to the SVG. Per the project's own style guidelines:

style="... outline: none;"

should be replaced with a CSS focus-visible rule:

svg[role="application"]:focus-visible {
  outline: 2px solid #002850; /* brand-navy */
  outline-offset: 2px;
}

Or in Tailwind via a wrapper class. Without this, keyboard users have no visual indication that the annotation is focused and accepting keyboard input.

4. The aria-live="polite" region announces once on mount and never again (minor)
The annotation_edit_mode_active message is placed in the sr-only div and will be read by screen readers when the overlay mounts. This is correct. However, the message fires every time the overlay mounts (each time a different annotation is selected). Confirm that the message is meaningful on re-selection — it says "Annotation selected — resize handles visible" which is still accurate. No change needed, just confirming intent.

5. Error state color — text-red-300 on dark background passes contrast, but confirm (minor)
The error banner uses bg-red-500/10 (very light red) with text-red-300. Light red text on a near-white background may fail WCAG AA (4.5:1 for normal text). Check with a contrast tool:

text-red-300 (#FCA5A5) on bg-red-500/10 (≈ rgba(239, 68, 68, 0.1) over the PDF viewer's dark background) — the actual contrast depends on the PDF viewer's background color. If the viewer is dark, this likely passes. If it appears on a light background, it may fail. Use text-red-700 for safety.

6. The move cursor is set via inline style — should use Tailwind utility or CSS class (cosmetic)

style="cursor: move; pointer-events: all;"

This is consistent with the rest of the SVG inline styles in this component and acceptable given SVG's limited Tailwind support. Not a blocker.

7. Touch target size — 44×44px hit area on handles is correct (positive callout)
The <rect data-handle-hit x="-22" y="-22" width="44" height="44"> ensures WCAG 2.2 compliant touch targets. This is exactly the right approach and critical for the senior audience (60+). Well done.

The two blockers (individual handle keyboard accessibility and missing i18n on the aria-label) should be resolved before merge. Finding #3 (focus ring) is a high-priority follow-up if not fixed in this PR. Everything else is a suggestion.

## Leonie Voss (@leonievoss) — UI/UX & Accessibility Review **Verdict: APPROVE with accessibility improvements requested before merge** The feature is functional and the drag interaction model is well-conceived. The pixel-space viewBox approach for handles is smart — it avoids the squished-handle problem on narrow annotations. My review focuses on accessibility and brand compliance. --- ### Findings **1. BLOCKER: Handle `<g>` elements have `role="none"` but are interactive — missing keyboard access** Each handle `<g>` responds to `onpointerdown` but has `role="none"` and no `tabindex`. The PR description says handles are keyboard-accessible via arrow keys on the SVG root, but that only provides move functionality (nudging X/Y). There is no way to keyboard-navigate to a specific corner handle and resize from it. The SVG root `tabindex="0"` captures all keyboard events; individual handles are mouse-only. This is a real accessibility gap for users who cannot use a pointer. Recommendation: ```svelte <g data-handle={handle.id} role="button" tabindex="0" aria-label="Resize {handle.id} handle" onpointerdown={(e) => handlePointerDown(e, 'handle', handle.id)} onkeydown={(e) => { if (e.key === 'Enter' || e.key === ' ') { // activate resize mode for this handle } }} > ``` At minimum, the handles need `tabindex="0"` and `aria-label` with a meaningful description (e.g., "Resize from top-left corner"). Without this, keyboard-only users can only move annotations, not resize them. **2. BLOCKER: `aria-label="Annotation resize handles"` on the SVG is in English only** The label is hardcoded in English. All user-visible strings in this project use Paraglide for i18n. Add a translation key: ```json // de.json "annotation_resize_area": "Annotation-Größe und Position ändern" // en.json "annotation_resize_area": "Resize and reposition annotation" // es.json "annotation_resize_area": "Cambiar tamaño y posición de la anotación" ``` ```svelte aria-label={m.annotation_resize_area()} ``` **3. Focus ring missing on the SVG root** The SVG has `style="... outline: none;"`. This removes the browser focus indicator for keyboard users navigating to the SVG. Per the project's own style guidelines: ```svelte style="... outline: none;" ``` should be replaced with a CSS `focus-visible` rule: ```css svg[role="application"]:focus-visible { outline: 2px solid #002850; /* brand-navy */ outline-offset: 2px; } ``` Or in Tailwind via a wrapper class. Without this, keyboard users have no visual indication that the annotation is focused and accepting keyboard input. **4. The `aria-live="polite"` region announces once on mount and never again (minor)** The `annotation_edit_mode_active` message is placed in the `sr-only` div and will be read by screen readers when the overlay mounts. This is correct. However, the message fires every time the overlay mounts (each time a different annotation is selected). Confirm that the message is meaningful on re-selection — it says "Annotation selected — resize handles visible" which is still accurate. No change needed, just confirming intent. **5. Error state color — `text-red-300` on dark background passes contrast, but confirm (minor)** The error banner uses `bg-red-500/10` (very light red) with `text-red-300`. Light red text on a near-white background may fail WCAG AA (4.5:1 for normal text). Check with a contrast tool: - `text-red-300` (#FCA5A5) on `bg-red-500/10` (≈ rgba(239, 68, 68, 0.1) over the PDF viewer's dark background) — the actual contrast depends on the PDF viewer's background color. If the viewer is dark, this likely passes. If it appears on a light background, it may fail. Use `text-red-700` for safety. **6. The `move` cursor is set via inline style — should use Tailwind utility or CSS class (cosmetic)** ```svelte style="cursor: move; pointer-events: all;" ``` This is consistent with the rest of the SVG inline styles in this component and acceptable given SVG's limited Tailwind support. Not a blocker. **7. Touch target size — 44×44px hit area on handles is correct (positive callout)** The `<rect data-handle-hit x="-22" y="-22" width="44" height="44">` ensures WCAG 2.2 compliant touch targets. This is exactly the right approach and critical for the senior audience (60+). Well done. --- The two blockers (individual handle keyboard accessibility and missing i18n on the aria-label) should be resolved before merge. Finding #3 (focus ring) is a high-priority follow-up if not fixed in this PR. Everything else is a suggestion.

marcel commented

2026-04-14 13:19:44 +02:00

Tobias Wendt (@tobiwendt) — DevOps & Platform Review

Verdict: APPROVE — no infrastructure changes in this PR

This PR is purely application code (backend Java, frontend Svelte, one Flyway migration). There are no Docker Compose changes, no CI workflow changes, no new dependencies, and no new environment variables. My review is brief.

Findings

1. Flyway V33 migration — no issues
V33__add_annotation_bounds_constraint.sql is a pure DDL change (adds a CHECK constraint). It follows the naming convention, has a descriptive comment, and is non-destructive. The migration will run cleanly on the existing schema. Worth noting: if any existing document_annotations rows have coordinates outside the constraint's range (unlikely given they were all written by controlled code), the migration will fail. This is not a DevOps issue to solve, but worth confirming before deploying to production if there is existing annotation data.

2. No new Docker/Compose changes (positive)
The OCR service's resource requirements, the database, and MinIO are all unchanged. No additional infrastructure needed for this feature.

3. No new environment variables or secrets (positive)
updateAnnotation in PdfViewer.svelte calls fetch('/api/...') using a relative URL with no additional API keys or tokens. The existing session-based auth handles authentication.

4. No CI workflow changes — this PR will exercise the existing pipeline
The added tests (JUnit + Vitest) will run in the existing CI jobs without modification. Build time impact is minimal: 2 new JUnit test classes (~6 tests), 3 new Vitest test files (~20 tests).

5. setTimeout in PdfViewer.svelte for auto-dismissing the error banner — no concern
A 4-second setTimeout in the browser is not a platform concern. Just noting it exists so no one removes it thinking it is dead code.

Nothing actionable from the infrastructure side. The migration is clean, no new services are added, and CI should handle this without changes.

## Tobias Wendt (@tobiwendt) — DevOps & Platform Review **Verdict: APPROVE — no infrastructure changes in this PR** This PR is purely application code (backend Java, frontend Svelte, one Flyway migration). There are no Docker Compose changes, no CI workflow changes, no new dependencies, and no new environment variables. My review is brief. --- ### Findings **1. Flyway V33 migration — no issues** `V33__add_annotation_bounds_constraint.sql` is a pure DDL change (adds a CHECK constraint). It follows the naming convention, has a descriptive comment, and is non-destructive. The migration will run cleanly on the existing schema. Worth noting: if any existing `document_annotations` rows have coordinates outside the constraint's range (unlikely given they were all written by controlled code), the migration will fail. This is not a DevOps issue to solve, but worth confirming before deploying to production if there is existing annotation data. **2. No new Docker/Compose changes (positive)** The OCR service's resource requirements, the database, and MinIO are all unchanged. No additional infrastructure needed for this feature. **3. No new environment variables or secrets (positive)** `updateAnnotation` in `PdfViewer.svelte` calls `fetch('/api/...')` using a relative URL with no additional API keys or tokens. The existing session-based auth handles authentication. **4. No CI workflow changes — this PR will exercise the existing pipeline** The added tests (JUnit + Vitest) will run in the existing CI jobs without modification. Build time impact is minimal: 2 new JUnit test classes (~6 tests), 3 new Vitest test files (~20 tests). **5. `setTimeout` in `PdfViewer.svelte` for auto-dismissing the error banner — no concern** A 4-second `setTimeout` in the browser is not a platform concern. Just noting it exists so no one removes it thinking it is dead code. --- Nothing actionable from the infrastructure side. The migration is clean, no new services are added, and CI should handle this without changes.

marcel added 12 commits 2026-04-14 14:43:59 +02:00

test(annotations): verify save() is called in updateAnnotation test [M5] 4d3207fc27

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

test(annotations): add missing height and x boundary validation tests [M4] 65d606d8bb

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

test(annotations): add failing test for DataIntegrityViolationException defense [M2 red] f00b470928

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

feat(annotations): add @Slf4j and DataIntegrityViolationException catch to updateAnnotation [M2] a19faa3806

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

docs(annotations): fix ANNOTATION_UPDATE_FAILED Javadoc to reflect 400 status [M3] 40c8f548db

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

test(annotations): add Testcontainers integration tests for V33 chk_annotation_bounds [B1] 72700bd28f

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

feat(i18n): add annotation_resize_area and annotation_resize_handle message keys [B2, B3] 060d1c0515

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

feat(annotations): wire SVG aria-label to Paraglide i18n [B3] 4d9145e49f

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

feat(annotations): add keyboard accessibility to resize handles [B2] 7097f991fe

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

fix(annotations): reset liveWidth/liveHeight in handleKeyDown error rollback [M1, M6] 7125a0a8eb

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

fix(annotations): add catch(err) binding to handlePointerUp error handler [M6] 76828a95e3

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

fix(annotations): replace outline:none with focus-visible ring for keyboard accessibility [M7]

CI / Unit & Component Tests (pull_request) Failing after 1s

Details

CI / Backend Unit Tests (pull_request) Failing after 1s

Details

CI / Unit & Component Tests (push) Failing after 2s

Details

CI / Backend Unit Tests (push) Failing after 1s

Details

28ac90b529

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

marcel commented

2026-04-14 14:44:30 +02:00

Review fixes applied

All 3 blockers and 7 minor items from the multi-persona review have been addressed. Branch pushed, all tests green (897 backend, 720 frontend).

Blockers resolved

B1 — Sara: No Testcontainers test for V33 chk_annotation_bounds ✅
Added 3 integration tests to MigrationIntegrationTest.java using JdbcTemplate + Testcontainers (PostgreSQL), verifying the constraint rejects x > 1, rejects height < 0.01, and accepts valid coordinates.
→ commit 72700bd

B2 — Leonie: Handle <g> elements not keyboard-reachable ✅
Changed role="none" → role="button", added tabindex="0" and aria-label={m.annotation_resize_handle({ direction: ... })} to all 8 handle groups. Added a directionLabels map (NW/NE/SW/SE/N/S/E/W). Added onkeydown that suppresses default on Enter/Space.
→ commit 7097f99

B3 — Leonie: SVG aria-label hardcoded English ✅
Replaced aria-label="Annotation resize handles" with aria-label={m.annotation_resize_area()}. Added annotation_resize_area and annotation_resize_handle keys to de/en/es message files.
→ commits 060d1c0, 4d9145e

Minor items resolved

M1 — Felix/Markus: handleKeyDown catch resets only liveX/liveY ✅
Added liveWidth = annotation.width and liveHeight = annotation.height to the catch rollback.
→ commit 7125a0a

M2 — Felix: Missing @Slf4j + no DataIntegrityViolationException defense ✅
Added @Slf4j to AnnotationService. Wrapped annotationRepository.save() in try/catch that logs a warning and re-throws as DomainException.badRequest(ANNOTATION_UPDATE_FAILED, ...). Test written red-first.
→ commits f00b470 (test), a19faa3 (impl)

M3 — Felix: ANNOTATION_UPDATE_FAILED Javadoc says 500 ✅
Updated Javadoc to "400" now that a badRequest throw site exists.
→ commit 40c8f54

M4 — Sara: Missing controller validation boundary tests ✅
Added patchAnnotation_returns400_withHeightBelowMinimum ({"height":0.005}) and patchAnnotation_returns400_withXAboveMaximum ({"x":1.1}) to AnnotationControllerTest.
→ commit 65d606d

M5 — Felix: updateAnnotation_updatesOnlyPresentFields missing verify(save) ✅
Added verify(annotationRepository).save(annotation) after the value assertions.
→ commit 4d3207f

M6 — Nora: Silent catch {} blocks ✅
Changed both } catch { to } catch (err) { with console.error(...) before rollback — handlePointerUp and handleKeyDown.
→ commits 7125a0a, 76828a9

M7 — Leonie: outline: none removes focus ring ✅
Removed outline: none from the SVG inline style. Added svg[role='application']:focus-visible { outline: 2px solid #002850; outline-offset: 2px; } to the <style> block.
→ commit 28ac90b

Not addressed (deferred per plan)

Full Enter/Space keyboard resize per handle (design complexity, follow-up issue)
E2E Playwright drag-resize test (Sara #6)
Component tests using data-handle selectors (Sara #3)

## Review fixes applied All 3 blockers and 7 minor items from the multi-persona review have been addressed. Branch pushed, all tests green (897 backend, 720 frontend). --- ### Blockers resolved **B1 — Sara: No Testcontainers test for V33 `chk_annotation_bounds`** ✅ Added 3 integration tests to `MigrationIntegrationTest.java` using JdbcTemplate + Testcontainers (PostgreSQL), verifying the constraint rejects `x > 1`, rejects `height < 0.01`, and accepts valid coordinates. → commit `72700bd` **B2 — Leonie: Handle `<g>` elements not keyboard-reachable** ✅ Changed `role="none"` → `role="button"`, added `tabindex="0"` and `aria-label={m.annotation_resize_handle({ direction: ... })}` to all 8 handle groups. Added a `directionLabels` map (NW/NE/SW/SE/N/S/E/W). Added `onkeydown` that suppresses default on Enter/Space. → commit `7097f99` **B3 — Leonie: SVG `aria-label` hardcoded English** ✅ Replaced `aria-label="Annotation resize handles"` with `aria-label={m.annotation_resize_area()}`. Added `annotation_resize_area` and `annotation_resize_handle` keys to de/en/es message files. → commits `060d1c0`, `4d9145e` --- ### Minor items resolved **M1 — Felix/Markus: `handleKeyDown` catch resets only liveX/liveY** ✅ Added `liveWidth = annotation.width` and `liveHeight = annotation.height` to the catch rollback. → commit `7125a0a` **M2 — Felix: Missing `@Slf4j` + no `DataIntegrityViolationException` defense** ✅ Added `@Slf4j` to `AnnotationService`. Wrapped `annotationRepository.save()` in try/catch that logs a warning and re-throws as `DomainException.badRequest(ANNOTATION_UPDATE_FAILED, ...)`. Test written red-first. → commits `f00b470` (test), `a19faa3` (impl) **M3 — Felix: `ANNOTATION_UPDATE_FAILED` Javadoc says 500** ✅ Updated Javadoc to "400" now that a `badRequest` throw site exists. → commit `40c8f54` **M4 — Sara: Missing controller validation boundary tests** ✅ Added `patchAnnotation_returns400_withHeightBelowMinimum` (`{"height":0.005}`) and `patchAnnotation_returns400_withXAboveMaximum` (`{"x":1.1}`) to `AnnotationControllerTest`. → commit `65d606d` **M5 — Felix: `updateAnnotation_updatesOnlyPresentFields` missing `verify(save)`** ✅ Added `verify(annotationRepository).save(annotation)` after the value assertions. → commit `4d3207f` **M6 — Nora: Silent `catch {}` blocks** ✅ Changed both `} catch {` to `} catch (err) {` with `console.error(...)` before rollback — `handlePointerUp` and `handleKeyDown`. → commits `7125a0a`, `76828a9` **M7 — Leonie: `outline: none` removes focus ring** ✅ Removed `outline: none` from the SVG inline style. Added `svg[role='application']:focus-visible { outline: 2px solid #002850; outline-offset: 2px; }` to the `<style>` block. → commit `28ac90b` --- ### Not addressed (deferred per plan) - Full Enter/Space keyboard resize per handle (design complexity, follow-up issue) - E2E Playwright drag-resize test (Sara #6) - Component tests using `data-handle` selectors (Sara #3)

marcel merged commit 28ac90b529 into main

2026-04-14 14:55:28 +02:00

marcel deleted branch feat/issue-233-annotation-resize-move

2026-04-14 14:55:30 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#235