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

fix: address PR review feedback — security, architecture, dead code

Browse Source 
Fixes from PR #178 review:

Migration fixes:
- V18/V19: fix FK references from app_users to users (correct table name)
- V18: change annotation_id FK from ON DELETE CASCADE to ON DELETE RESTRICT
  (block is aggregate root, cascade flows from block, not annotation)

Backend fixes:
- TranscriptionService.deleteBlock(): remove userId param, delete block first
  then annotation directly via repository (no ownership check — block owns annotation)
- TranscriptionService.sanitizeText(): remove flawed regex HTML stripping,
  textarea content is plain text by design — just enforce max length
- TranscriptionBlockController.requireUserId(): throw DomainException.unauthorized()
  instead of silently returning null on auth failure
- CreateTranscriptionBlockDTO: add @Min/@Positive validation on coordinates
- Add @Slf4j logging to TranscriptionService for create/delete operations

Frontend fixes:
- Delete DocumentBottomPanel.svelte entirely (issue #175 requirement)
- Remove redundant mode exclusivity $effect (handled at toggle call sites)
- Remove dead handleCommentClick + onCommentClick prop (comments are future work)
- Remove quote hint UI (depends on comment feature)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Marcel
2026-04-05 11:43:35 +02:00
parent 1efd3d8e23
commit 6463a32dfc
9 changed files with 41 additions and 250 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
backend/src/main/java/org/raddatz/familienarchiv/controller/TranscriptionBlockController.java
View File

@@ -5,6 +5,7 @@ import lombok.extern.slf4j.Slf4j;
import org.raddatz.familienarchiv.dto.CreateTranscriptionBlockDTO;
import org.raddatz.familienarchiv.dto.ReorderTranscriptionBlocksDTO;
import org.raddatz.familienarchiv.dto.UpdateTranscriptionBlockDTO;
import org.raddatz.familienarchiv.exception.DomainException;
import org.raddatz.familienarchiv.model.AppUser;
import org.raddatz.familienarchiv.model.TranscriptionBlock;
import org.raddatz.familienarchiv.model.TranscriptionBlockVersion;
@@ -47,7 +48,7 @@ public class TranscriptionBlockController {
@PathVariable UUID documentId,
@RequestBody CreateTranscriptionBlockDTO dto,
Authentication authentication) {
UUID userId = resolveUserId(authentication);
UUID userId = requireUserId(authentication);
return transcriptionService.createBlock(documentId, dto, userId);
}
@@ -58,7 +59,7 @@ public class TranscriptionBlockController {
@PathVariable UUID blockId,
@RequestBody UpdateTranscriptionBlockDTO dto,
Authentication authentication) {
UUID userId = resolveUserId(authentication);
UUID userId = requireUserId(authentication);
return transcriptionService.updateBlock(documentId, blockId, dto, userId);
}
@@ -67,10 +68,8 @@ public class TranscriptionBlockController {
@RequirePermission(Permission.WRITE_ALL)
public void deleteBlock(
@PathVariable UUID documentId,
@PathVariable UUID blockId,
Authentication authentication) {
UUID userId = resolveUserId(authentication);
transcriptionService.deleteBlock(documentId, blockId, userId);
@PathVariable UUID blockId) {
transcriptionService.deleteBlock(documentId, blockId);
}
@PutMapping("/reorder")
@@ -89,14 +88,14 @@ public class TranscriptionBlockController {
return transcriptionService.getBlockHistory(documentId, blockId);
}
private UUID resolveUserId(Authentication authentication) {
if (authentication == null || !authentication.isAuthenticated()) return null;
try {
AppUser user = userService.findByUsername(authentication.getName());
return user != null ? user.getId() : null;
} catch (Exception e) {
log.warn("Could not resolve user for transcription: {}", e.getMessage());
return null;
private UUID requireUserId(Authentication authentication) {
if (authentication == null || !authentication.isAuthenticated()) {
throw DomainException.unauthorized("Authentication required");
}
AppUser user = userService.findByUsername(authentication.getName());
if (user == null) {
throw DomainException.unauthorized("User not found");
}
return user.getId();
}
}

7
backend/src/main/java/org/raddatz/familienarchiv/dto/CreateTranscriptionBlockDTO.java
View File

@@ -1,5 +1,7 @@
package org.raddatz.familienarchiv.dto;
import jakarta.validation.constraints.Min;
import jakarta.validation.constraints.Positive;
import lombok.AllArgsConstructor;
import lombok.Data;
import lombok.NoArgsConstructor;
@@ -8,10 +10,15 @@ import lombok.NoArgsConstructor;
@NoArgsConstructor
@AllArgsConstructor
public class CreateTranscriptionBlockDTO {
@Min(0)
private int pageNumber;
@Min(0)
private double x;
@Min(0)
private double y;
@Positive
private double width;
@Positive
private double height;
private String text;
private String label;

25
backend/src/main/java/org/raddatz/familienarchiv/service/TranscriptionService.java
View File

@@ -12,6 +12,7 @@ import org.raddatz.familienarchiv.model.Document;
import org.raddatz.familienarchiv.model.DocumentAnnotation;
import org.raddatz.familienarchiv.model.TranscriptionBlock;
import org.raddatz.familienarchiv.model.TranscriptionBlockVersion;
import org.raddatz.familienarchiv.repository.AnnotationRepository;
import org.raddatz.familienarchiv.repository.TranscriptionBlockRepository;
import org.raddatz.familienarchiv.repository.TranscriptionBlockVersionRepository;
import org.springframework.stereotype.Service;
@@ -30,6 +31,7 @@ public class TranscriptionService {
private final TranscriptionBlockRepository blockRepository;
private final TranscriptionBlockVersionRepository versionRepository;
private final AnnotationRepository annotationRepository;
private final AnnotationService annotationService;
private final DocumentService documentService;
@@ -69,6 +71,7 @@ public class TranscriptionService {
TranscriptionBlock saved = blockRepository.save(block);
saveVersion(saved, userId);
log.info("Created transcription block {} for document {}", saved.getId(), documentId);
return saved;
}
@@ -90,11 +93,17 @@ public class TranscriptionService {
}
@Transactional
public void deleteBlock(UUID documentId, UUID blockId, UUID userId) {
public void deleteBlock(UUID documentId, UUID blockId) {
TranscriptionBlock block = getBlock(documentId, blockId);
// CASCADE deletes annotation, versions, and comments via DB constraints
UUID annotationId = block.getAnnotationId();
// Block is the aggregate root — delete block first (cascades to versions + comments),
// then delete the dependent annotation directly (no ownership check needed)
blockRepository.delete(block);
annotationService.deleteAnnotation(documentId, block.getAnnotationId(), userId);
blockRepository.flush();
annotationRepository.deleteById(annotationId);
log.info("Deleted transcription block {} and annotation {} for document {}",
blockId, annotationId, documentId);
}
@Transactional
@@ -122,13 +131,11 @@ public class TranscriptionService {
versionRepository.save(version);
}
private String sanitizeText(String text) {
String sanitizeText(String text) {
if (text == null) return "";
// Strip any HTML tags — textarea content should be plain text only
String cleaned = text.replaceAll("<[^>]*>", "");
if (cleaned.length() > MAX_TEXT_LENGTH) {
cleaned = cleaned.substring(0, MAX_TEXT_LENGTH);
if (text.length() > MAX_TEXT_LENGTH) {
text = text.substring(0, MAX_TEXT_LENGTH);
}
return cleaned;
return text;
}
}

6
backend/src/main/resources/db/migration/V18__add_transcription_blocks.sql
View File

@@ -1,13 +1,13 @@
CREATE TABLE transcription_blocks (
id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
annotation_id UUID NOT NULL REFERENCES document_annotations(id) ON DELETE CASCADE,
annotation_id UUID NOT NULL REFERENCES document_annotations(id) ON DELETE RESTRICT,
document_id UUID NOT NULL REFERENCES documents(id) ON DELETE CASCADE,
text TEXT NOT NULL DEFAULT '',
label VARCHAR(200),
sort_order INTEGER NOT NULL DEFAULT 0,
version INTEGER NOT NULL DEFAULT 0,
created_by UUID REFERENCES app_users(id) ON DELETE SET NULL,
updated_by UUID REFERENCES app_users(id) ON DELETE SET NULL,
created_by UUID REFERENCES users(id) ON DELETE SET NULL,
updated_by UUID REFERENCES users(id) ON DELETE SET NULL,
created_at TIMESTAMP NOT NULL DEFAULT now(),
updated_at TIMESTAMP NOT NULL DEFAULT now()
);

2
backend/src/main/resources/db/migration/V19__add_transcription_block_versions.sql
View File

@@ -2,7 +2,7 @@ CREATE TABLE transcription_block_versions (
id UUID PRIMARY KEY DEFAULT gen_random_uuid(),
block_id UUID NOT NULL REFERENCES transcription_blocks(id) ON DELETE CASCADE,
text TEXT NOT NULL,
changed_by UUID REFERENCES app_users(id) ON DELETE SET NULL,
changed_by UUID REFERENCES users(id) ON DELETE SET NULL,
changed_at TIMESTAMP NOT NULL DEFAULT now()
);

193
frontend/src/lib/components/DocumentBottomPanel.svelte
View File

@@ -1,193 +0,0 @@
<script lang="ts">
import { m } from '$lib/paraglide/messages.js';
import PanelMetadata from './PanelMetadata.svelte';
import PanelTranscription from './PanelTranscription.svelte';
import PanelDiscussion from './PanelDiscussion.svelte';
import PanelHistory from './PanelHistory.svelte';
import type { Comment, DocumentPanelTab } from '$lib/types';
type Doc = {
id: string;
title?: string | null;
documentDate?: string | null;
location?: string | null;
documentLocation?: string | null;
tags?: { id: string; name: string }[] | null;
sender?: { id: string; firstName: string; lastName: string; alias?: string | null } | null;
receivers?: { id: string; firstName: string; lastName: string }[] | null;
summary?: string | null;
transcription?: string | null;
};
type Props = {
doc: Doc;
comments: Comment[];
canComment: boolean;
currentUserId: string | null;
canAdmin: boolean;
open: boolean;
height: number;
activeTab: DocumentPanelTab;
targetCommentId?: string | null;
};
let {
doc,
comments,
canComment,
currentUserId,
canAdmin,
open = $bindable(),
height = $bindable(),
activeTab = $bindable(),
targetCommentId = null
}: Props = $props();
const MIN_HEIGHT = 52; // drag handle (8px) + tab bar (~44px)
let isDragging = $state(false);
let dragStartY = 0;
let dragStartHeight = 0;
function fullHeight() {
const topbar = document.querySelector('[data-topbar]');
return window.innerHeight - (topbar?.getBoundingClientRect().bottom ?? 0);
}
function openTab(tab: DocumentPanelTab) {
activeTab = tab;
if (!open) {
open = true;
if (height <= MIN_HEIGHT) height = fullHeight();
}
}
function closePanel() {
open = false;
}
function onDragStart(e: PointerEvent) {
isDragging = true;
dragStartY = e.clientY;
dragStartHeight = open ? height : MIN_HEIGHT;
(e.currentTarget as HTMLElement).setPointerCapture(e.pointerId);
}
function onDragMove(e: PointerEvent) {
if (!isDragging) return;
const delta = dragStartY - e.clientY; // positive = dragging up = bigger panel
const newHeight = dragStartHeight + delta;
const maxHeight = fullHeight();
if (newHeight <= MIN_HEIGHT + 20) {
// collapsed past threshold → close
open = false;
} else {
open = true;
height = Math.max(80, Math.min(newHeight, maxHeight));
}
}
function onDragEnd() {
isDragging = false;
}
const tabs: { id: DocumentPanelTab; label: () => string }[] = [
{ id: 'metadata', label: m.doc_panel_tab_metadata },
{ id: 'transcription', label: m.doc_panel_tab_transcription },
{ id: 'discussion', label: m.doc_panel_tab_discussion },
{ id: 'history', label: m.doc_panel_tab_history }
];
const panelHeight = $derived(open ? height : MIN_HEIGHT);
let discussionCount = $state((() => comments.reduce((s, c) => s + 1 + c.replies.length, 0))());
function handleCountChange(count: number) {
discussionCount = count;
}
</script>
<div
class="z-30 flex shrink-0 flex-col border-t border-line bg-surface shadow-[0_-4px_16px_rgba(0,0,0,0.08)]"
style="height: {panelHeight}px"
data-testid="bottom-panel"
>
<!-- Drag handle -->
<div
class="flex h-2 shrink-0 cursor-ns-resize items-center justify-center bg-surface"
style="touch-action: none"
role="separator"
aria-orientation="horizontal"
aria-label="Panel resize"
onpointerdown={onDragStart}
onpointermove={onDragMove}
onpointerup={onDragEnd}
onpointercancel={onDragEnd}
>
<div class="h-1 w-12 rounded-full bg-line"></div>
</div>
<!-- Tab bar -->
<div class="flex shrink-0 items-center border-b border-line bg-surface">
<!-- Scrollable tabs area — hides scrollbar visually -->
<div
class="flex flex-1 items-center overflow-x-auto px-2 [scrollbar-width:none] [&::-webkit-scrollbar]:hidden"
>
{#each tabs as tab (tab.id)}
<button
onclick={() => openTab(tab.id)}
class="mr-1 shrink-0 px-3 py-2.5 font-sans text-xs font-medium transition-colors {activeTab === tab.id && open
? 'border-b-2 border-primary text-ink'
: 'text-ink-3 hover:text-ink'}"
aria-pressed={activeTab === tab.id && open}
>
{tab.label()}
{#if tab.id === 'discussion'}
<span
data-testid="discussion-count-badge"
class="ml-1.5 inline-flex h-4 min-w-4 items-center justify-center rounded-full bg-primary px-1 font-sans text-[10px] font-bold text-primary-fg"
>{discussionCount}</span
>
{/if}
</button>
{/each}
</div>
{#if open}
<button
onclick={closePanel}
data-testid="panel-close-btn"
aria-label="Panel schließen"
class="mr-2 shrink-0 rounded p-1.5 text-ink-3 transition-colors hover:bg-muted hover:text-ink"
>
<svg class="h-4 w-4" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2">
<path stroke-linecap="round" stroke-linejoin="round" d="M6 18L18 6M6 6l12 12" />
</svg>
</button>
{/if}
</div>
<!-- Tab content -->
{#if open}
<div class="flex-1 overflow-y-auto" data-testid="bottom-panel-content">
{#if activeTab === 'metadata'}
<PanelMetadata doc={doc} />
{:else if activeTab === 'transcription'}
<PanelTranscription doc={doc} />
{:else if activeTab === 'discussion'}
<PanelDiscussion
documentId={doc.id}
initialComments={comments}
canComment={canComment}
currentUserId={currentUserId}
canAdmin={canAdmin}
targetCommentId={targetCommentId}
onCountChange={handleCountChange}
/>
{:else if activeTab === 'history'}
<PanelHistory documentId={doc.id} />
{/if}
</div>
{/if}
</div>

19
frontend/src/lib/components/TranscriptionBlock.svelte
View File

@@ -12,7 +12,6 @@ type Props = {
saveState: SaveState;
onTextChange: (text: string) => void;
onFocus: () => void;
onCommentClick: () => void;
onDeleteClick: () => void;
onRetry: () => void;
};
@@ -26,7 +25,6 @@ let {
saveState,
onTextChange,
onFocus,
onCommentClick,
onDeleteClick,
onRetry
}: Props = $props();
@@ -91,22 +89,7 @@ function handleDelete() {
></textarea>
<!-- Footer -->
<div class="flex items-center justify-between border-t border-line pt-2">
<div class="flex flex-col gap-1">
<button
type="button"
class="text-xs font-medium text-ink-2 transition-colors hover:text-ink"
onclick={onCommentClick}
>
{m.transcription_block_comment_btn()}
</button>
{#if active}
<span class="text-xs text-ink-3">
{m.transcription_block_quote_hint()}
</span>
{/if}
</div>
<div class="flex items-center justify-end border-t border-line pt-2">
<div class="flex items-center gap-2">
<!-- Save state indicator -->
{#if saveState === 'saving'}

5
frontend/src/lib/components/TranscriptionEditView.svelte
View File

@@ -119,10 +119,6 @@ function handleDelete(blockId: string) {
onDeleteBlock(blockId);
}
function handleCommentClick() {
// Placeholder for future comment functionality
}
$effect(() => {
function onBeforeUnload() {
flushAllPending();
@@ -153,7 +149,6 @@ $effect(() => {
saveState={getSaveState(block.id)}
onTextChange={(text) => handleTextChange(block.id, text)}
onFocus={() => handleFocus(block.id)}
onCommentClick={handleCommentClick}
onDeleteClick={() => handleDelete(block.id)}
onRetry={() => handleRetry(block.id)}
/>

7
frontend/src/routes/documents/[id]/+page.svelte
View File

@@ -63,13 +63,6 @@ let transcribeMode = $state(false);
let activeAnnotationId = $state<string | null>(null);
let activeAnnotationPage = $state<number | null>(null);
// Mode exclusivity: entering one mode exits the other
$effect(() => {
if (annotateMode && transcribeMode) {
transcribeMode = false;
}
});
// ── Transcription blocks ─────────────────────────────────────────────────────
type TranscriptionBlockData = {