refactor(frontend): utility dedup, component splits, dead code removal (#193–#200) #241

marcel · 2026-04-15T15:00:31+02:00

marcel commented

2026-04-15 15:00:31 +02:00

Summary

Eight coordinated frontend refactor issues implemented in one branch:

#193 — Delete dead conversations/ route (5 files deleted, 14 orphaned i18n keys removed, E2E tests cleaned up)
#194 — Extract shared utilities: relativeTime → time.ts, getInitials → personFormat.ts, formatDate consolidation, blob URL leak fix, useFileLoader.svelte.ts, useUnsavedWarning.svelte.ts + UnsavedWarningBanner.svelte
#195 — Unify click-outside: migrate NotificationBell from inline impl to use:clickOutside; add missing test for defaultPrevented guard
#196 — Split PdfViewer.svelte: extract usePdfRenderer.svelte.ts + PdfControls.svelte; orchestrator now ~200 lines
#197 — Extract EntityNavSection.svelte: eliminate ~350 lines of repeated admin nav section markup
#198 — Extract CommentMessage.svelte: move FlatMessage type to lib/types.ts, extract extractQuote to comment.ts, add role="log" / role="article" accessibility
#199 — Split TranscriptionEditView.svelte: extract useBlockAutoSave.svelte.ts + useBlockDragDrop.svelte.ts; fix fade timer leak
#200 — Split NotificationBell.svelte: extract useNotificationStream.svelte.ts + NotificationDropdown.svelte; persistent aria-live wrapper; unread count resync on SSE reconnect

Test plan

cd frontend && npm run test — all 813 tests green
cd frontend && npm run check — no type errors
cd frontend && npm run lint — Prettier + ESLint clean
cd frontend && npm run build — production build succeeds

🤖 Generated with Claude Code

## Summary Eight coordinated frontend refactor issues implemented in one branch: - **#193** — Delete dead `conversations/` route (5 files deleted, 14 orphaned i18n keys removed, E2E tests cleaned up) - **#194** — Extract shared utilities: `relativeTime` → `time.ts`, `getInitials` → `personFormat.ts`, `formatDate` consolidation, blob URL leak fix, `useFileLoader.svelte.ts`, `useUnsavedWarning.svelte.ts` + `UnsavedWarningBanner.svelte` - **#195** — Unify click-outside: migrate `NotificationBell` from inline impl to `use:clickOutside`; add missing test for `defaultPrevented` guard - **#196** — Split `PdfViewer.svelte`: extract `usePdfRenderer.svelte.ts` + `PdfControls.svelte`; orchestrator now ~200 lines - **#197** — Extract `EntityNavSection.svelte`: eliminate ~350 lines of repeated admin nav section markup - **#198** — Extract `CommentMessage.svelte`: move `FlatMessage` type to `lib/types.ts`, extract `extractQuote` to `comment.ts`, add `role="log"` / `role="article"` accessibility - **#199** — Split `TranscriptionEditView.svelte`: extract `useBlockAutoSave.svelte.ts` + `useBlockDragDrop.svelte.ts`; fix fade timer leak - **#200** — Split `NotificationBell.svelte`: extract `useNotificationStream.svelte.ts` + `NotificationDropdown.svelte`; persistent `aria-live` wrapper; unread count resync on SSE reconnect ## Test plan - [ ] `cd frontend && npm run test` — all 813 tests green - [ ] `cd frontend && npm run check` — no type errors - [ ] `cd frontend && npm run lint` — Prettier + ESLint clean - [ ] `cd frontend && npm run build` — production build succeeds 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 14 commits 2026-04-15 15:00:33 +02:00

refactor(frontend): delete dead conversations/ route (#193 ) 731cdc75ab

Remove the old conversations page that was superseded by briefwechsel/.
No navigation link pointed to /conversations; it was unreachable through
the UI. Deletes 5 files, removes 14 orphaned i18n keys from de/en/es
message bundles, and removes E2E tests that navigated to /conversations.

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

test(actions): add defaultPrevented coverage for clickOutside (#195 ) 4446e80875

The action already checks event.defaultPrevented before dispatching
clickoutside, but that branch had no test. Add the missing case and
add a one-line comment explaining why capture phase is used.

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

refactor(bell): migrate attachClickOutside to use:clickOutside action (#195 ) c50845bcfc

Replace the inline attachClickOutside attachment in NotificationBell with
the shared use:clickOutside action from $lib/actions/clickOutside. The
inline implementation was functionally identical to the existing action.

Guard the onclickoutside handler so it only calls closeDropdown() when
the notification panel is already open, preventing the bell button from
stealing focus from other interactive elements (e.g. the user avatar menu).

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

refactor(time): extract relativeTime into shared time.ts utility 655a2003cb

Move relativeTime from notifications.ts (Intl.RelativeTimeFormat) to a
new time.ts that uses the Paraglide comment_time_* message keys — the
same logic that was already in CommentThread's timeAgo(). Remove the
duplicate timeAgo() from CommentThread and re-export relativeTime from
notifications.ts for backwards compatibility.

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

refactor(personFormat): replace getInitials(Person) with getInitials(name: string) 76d6f234b4

Unify the initials-extraction logic: the new string-based getInitials()
splits on whitespace, takes the first char of the first and last word
uppercased — matching the pattern that was already inlined in
CommentThread. Update PersonChip, DocumentMetadataDrawer, and
CommentThread to use the shared function.

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

refactor(date): consolidate formatDate in date.ts with optional format param 8be876492c

Add format?: 'short'|'long' (default 'long') to date.ts formatDate and
remove the duplicate from personFormat.ts. Update DocumentTopBar to
import from date.ts directly. Move the formatDate tests from
personFormat.spec to date.spec.

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

fix(fileloader): revoke blob URLs before re-assignment and on destroy dbf7f0bc16

Calling loadFile a second time previously leaked the previous object URL.
Add URL.revokeObjectURL(fileUrl) before creating a new one and in
onDestroy so all URLs are freed. Revoke behavior will be covered by the
useFileLoader hook tests in the next commit.

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

refactor(fileloader): extract createFileLoader hook from document/enrich pages 34e7436fdc

Move blob URL lifecycle management into a reusable createFileLoader()
hook that owns revoke-before-create and revoke-on-destroy. Replace
identical inline logic in documents/[id] and enrich/[id] with the hook.

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

refactor(unsaved): extract createUnsavedWarning hook and UnsavedWarningBanner accfa5373e

Move the identical isDirty / beforeNavigate / discard pattern out of the
three admin detail pages (groups, tags, users) into a reusable
createUnsavedWarning() hook and a UnsavedWarningBanner presentational
component.

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

refactor(admin): extract EntityNavSection to eliminate nav markup repetition (#197 ) fe6c247882

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

refactor(comments): extract CommentMessage component from CommentThread (#198 ) bc3fec11a9

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

refactor(pdf): extract usePdfRenderer and PdfControls from PdfViewer (#196 ) eb8aa92cf0

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

refactor(transcription): extract useBlockAutoSave and useBlockDragDrop from TranscriptionEditView (#199 ) 8898863a48

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

refactor(notifications): extract useNotificationStream and NotificationDropdown from NotificationBell (#200 )

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

Details

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

Details

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

Details

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

Details

ff9ae198c4

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

marcel commented

2026-04-15 15:02:38 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Solid batch refactor overall. Factory function pattern is consistent, runes are used correctly in .svelte.ts modules, and each extracted module has a clear single responsibility. Two things need fixing before merge.

Blockers

1. Hardcoded German hint text in CommentMessage.svelte

<!-- CommentMessage.svelte — edit mode hint -->
<div class="mt-1 font-sans text-xs text-ink-3">Enter speichern · Esc abbrechen</div>

This bypasses Paraglide entirely. Project rule: all user-visible strings go through m.* keys. Add a key like m.comment_edit_hint() to de.json, en.json, es.json and replace the hardcoded text.

2. relativeTime imported from wrong module in NotificationDropdown.svelte

<!-- NotificationDropdown.svelte -->
import { relativeTime } from '$lib/utils/notifications';

Issue #194 was specifically about extracting relativeTime to the canonical $lib/utils/time.ts. This component should import directly from there. Importing through notifications.ts creates an accidental dependency on a module that has nothing to do with timestamps conceptually, and prevents the linter/tree-shaker from seeing the coupling clearly.

Fix: import { relativeTime } from '$lib/utils/time';

Suggestions

handleBlur is fire-and-forget in useBlockAutoSave.svelte.ts

function handleBlur(): void {
    for (const [blockId] of [...debounceTimers]) {
        clearDebounce(blockId);
        executeSave(blockId);  // no await — intentional but undocumented
    }
}

This is intentional (blur shouldn't block the UI), but if two blocks both have pending saves on blur, the untracked promises can overlap. The current behavior is probably fine since executeSave checks pendingTexts before doing anything, but a brief comment here would save the next developer from the same question I just had.

useUnsavedWarning.svelte.ts — discard() silently no-ops when discardTarget is null

function discard(): void {
    isDirty = false;
    showUnsavedWarning = false;
    if (discardTarget) goto(discardTarget);  // if null, nothing happens — user stays on page
}

If the user hits "Discard" and discardTarget is null (e.g. a programmatic navigation that didn't produce a to URL), the warning disappears but the user stays on the current page. This is probably unreachable in practice, but documenting it as a known edge case would be good.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** Solid batch refactor overall. Factory function pattern is consistent, runes are used correctly in `.svelte.ts` modules, and each extracted module has a clear single responsibility. Two things need fixing before merge. ### Blockers **1. Hardcoded German hint text in `CommentMessage.svelte`** ```svelte  <div class="mt-1 font-sans text-xs text-ink-3">Enter speichern · Esc abbrechen</div> ``` This bypasses Paraglide entirely. Project rule: all user-visible strings go through `m.*` keys. Add a key like `m.comment_edit_hint()` to `de.json`, `en.json`, `es.json` and replace the hardcoded text. **2. `relativeTime` imported from wrong module in `NotificationDropdown.svelte`** ```svelte  import { relativeTime } from '$lib/utils/notifications'; ``` Issue #194 was specifically about extracting `relativeTime` to the canonical `$lib/utils/time.ts`. This component should import directly from there. Importing through `notifications.ts` creates an accidental dependency on a module that has nothing to do with timestamps conceptually, and prevents the linter/tree-shaker from seeing the coupling clearly. Fix: `import { relativeTime } from '$lib/utils/time';` ### Suggestions **`handleBlur` is fire-and-forget in `useBlockAutoSave.svelte.ts`** ```typescript function handleBlur(): void { for (const [blockId] of [...debounceTimers]) { clearDebounce(blockId); executeSave(blockId); // no await — intentional but undocumented } } ``` This is intentional (blur shouldn't block the UI), but if two blocks both have pending saves on blur, the untracked promises can overlap. The current behavior is probably fine since `executeSave` checks `pendingTexts` before doing anything, but a brief comment here would save the next developer from the same question I just had. **`useUnsavedWarning.svelte.ts` — `discard()` silently no-ops when `discardTarget` is null** ```typescript function discard(): void { isDirty = false; showUnsavedWarning = false; if (discardTarget) goto(discardTarget); // if null, nothing happens — user stays on page } ``` If the user hits "Discard" and `discardTarget` is null (e.g. a programmatic navigation that didn't produce a `to` URL), the warning disappears but the user stays on the current page. This is probably unreachable in practice, but documenting it as a known edge case would be good.

marcel commented

2026-04-15 15:02:59 +02:00

🏗️ Markus Keller — Application Architect

Verdict: ✅ Approved

The refactor achieves what it set out to do: clear boundaries between visual orchestrators and logic modules, consistent factory function naming, and correct use of .svelte.ts for rune-bearing modules. No architectural regressions. A few structural observations worth noting.

What's Well Done

The createX() naming convention is now consistent across all new modules (createBlockAutoSave, createBlockDragDrop, createNotificationStream, createFileLoader, createUnsavedWarning). This is the right pattern for Svelte 5 reactive modules that live outside components — it establishes a codebase-wide convention that future developers can follow.

Module boundary discipline is correct: useBlockDragDrop exposes only visual state (draggedBlockId, dropTargetIdx, dragOffsetY) while the orchestrator owns data and the API call. This is exactly the right split.

Observations (Non-blocking)

beforeNavigate at module initialization in useUnsavedWarning.svelte.ts

export function createUnsavedWarning() {
    // ...
    beforeNavigate(({ cancel, to }) => {  // <-- called at factory call time
        if (isDirty) { cancel(); ... }
    });
}

beforeNavigate is a SvelteKit lifecycle function that must be called synchronously during component initialization (it registers on the nearest $app/navigation context). Calling it inside a factory function works only if the factory is called in <script> at component mount time (which it is in the three admin pages). This is currently safe, but it's a hidden constraint: calling createUnsavedWarning() outside of component initialization (e.g., in a $effect, setTimeout, or a non-component module) would silently fail. A comment documenting this constraint would prevent future misuse.

relativeTime re-export chain in notifications.ts

Now that time.ts is the canonical source, notifications.ts should import and re-export from there (or consumers should import directly from time.ts). Currently NotificationDropdown.svelte imports relativeTime from notifications.ts — which means the utilities are still coupled. Minor, but the whole point of the extraction was to break that coupling.

EntityNavSection.svelte in routes/admin/ rather than lib/components/

This is intentional (it's route-specific UI), and I agree with the placement. Just noting it as a deliberate choice — lib/components/ is for reusable cross-route components; admin-specific components that won't be shared belong in the route folder.

## 🏗️ Markus Keller — Application Architect **Verdict: ✅ Approved** The refactor achieves what it set out to do: clear boundaries between visual orchestrators and logic modules, consistent factory function naming, and correct use of `.svelte.ts` for rune-bearing modules. No architectural regressions. A few structural observations worth noting. ### What's Well Done The `createX()` naming convention is now consistent across all new modules (`createBlockAutoSave`, `createBlockDragDrop`, `createNotificationStream`, `createFileLoader`, `createUnsavedWarning`). This is the right pattern for Svelte 5 reactive modules that live outside components — it establishes a codebase-wide convention that future developers can follow. Module boundary discipline is correct: `useBlockDragDrop` exposes only visual state (`draggedBlockId`, `dropTargetIdx`, `dragOffsetY`) while the orchestrator owns data and the API call. This is exactly the right split. ### Observations (Non-blocking) **`beforeNavigate` at module initialization in `useUnsavedWarning.svelte.ts`** ```typescript export function createUnsavedWarning() { // ... beforeNavigate(({ cancel, to }) => { // <-- called at factory call time if (isDirty) { cancel(); ... } }); } ``` `beforeNavigate` is a SvelteKit lifecycle function that must be called synchronously during component initialization (it registers on the nearest `$app/navigation` context). Calling it inside a factory function works *only* if the factory is called in `<script>` at component mount time (which it is in the three admin pages). This is currently safe, but it's a hidden constraint: calling `createUnsavedWarning()` outside of component initialization (e.g., in a `$effect`, setTimeout, or a non-component module) would silently fail. A comment documenting this constraint would prevent future misuse. **`relativeTime` re-export chain in `notifications.ts`** Now that `time.ts` is the canonical source, `notifications.ts` should import and re-export from there (or consumers should import directly from `time.ts`). Currently `NotificationDropdown.svelte` imports `relativeTime` from `notifications.ts` — which means the utilities are still coupled. Minor, but the whole point of the extraction was to break that coupling. **`EntityNavSection.svelte` in `routes/admin/` rather than `lib/components/`** This is intentional (it's route-specific UI), and I agree with the placement. Just noting it as a deliberate choice — `lib/components/` is for reusable cross-route components; admin-specific components that won't be shared belong in the route folder.

marcel commented

2026-04-15 15:03:16 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

813 tests passing is a solid baseline. The new modules all have unit tests, and the use of vi.useFakeTimers() in useBlockAutoSave is exactly right for debounce testing. A few coverage gaps worth addressing.

Concerns

1. Drag-drop reorder logic has no test coverage

useBlockDragDrop.svelte.ts has 4 tests covering initial state and basic pointer events. The actual reorder calculation in handlePointerUp — the splice/insertAt index arithmetic — is untested:

// handlePointerUp — this logic is never exercised by a test
const fromIdx = sorted.findIndex((b) => b.id === draggedBlockId);
if (fromIdx >= 0) {
    const [moved] = sorted.splice(fromIdx, 1);
    const insertAt = dropTargetIdx > fromIdx ? dropTargetIdx - 1 : dropTargetIdx;
    sorted.splice(insertAt, 0, moved);
    onReorder(sorted.map((b) => b.id));
}

The off-by-one risk here is high (dropTargetIdx - 1 vs dropTargetIdx). I'd want at minimum:

Move block from index 0 to end → correct order
Move block from end to index 0 → correct order
Move block down by 1 position → correct order (the -1 branch)
onReorder is not called when dropTargetIdx is null

2. useNotificationStream — SSE event handling not tested

The test suite covers fetchUnreadCount, fetchNotifications, markRead, markAllRead, and destroy. Missing: a test that fires a notification event on the MockEventSource and verifies that notifications is prepended and unreadCount increments. This is the core SSE behavior that motivated the extraction.

The MockEventSource has addEventListener but there's no test that calls the registered listener.

3. useUnsavedWarning has no tests at all

No test file exists for this hook. Given it controls a navigation guard (beforeNavigate), the missing coverage for markDirty → navigate → warning shown and discard → navigation resumes is a risk.

What's Good

vi.useFakeTimers() + vi.advanceTimersByTimeAsync() for debounce testing is the right approach — no Thread.sleep equivalents.
makeNotification() factory function in useNotificationStream.svelte.test.ts is clean.
useBlockAutoSave tests cover the error→retry path explicitly — good edge case coverage.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** 813 tests passing is a solid baseline. The new modules all have unit tests, and the use of `vi.useFakeTimers()` in `useBlockAutoSave` is exactly right for debounce testing. A few coverage gaps worth addressing. ### Concerns **1. Drag-drop reorder logic has no test coverage** `useBlockDragDrop.svelte.ts` has 4 tests covering initial state and basic pointer events. The actual reorder calculation in `handlePointerUp` — the `splice`/`insertAt` index arithmetic — is untested: ```typescript // handlePointerUp — this logic is never exercised by a test const fromIdx = sorted.findIndex((b) => b.id === draggedBlockId); if (fromIdx >= 0) { const [moved] = sorted.splice(fromIdx, 1); const insertAt = dropTargetIdx > fromIdx ? dropTargetIdx - 1 : dropTargetIdx; sorted.splice(insertAt, 0, moved); onReorder(sorted.map((b) => b.id)); } ``` The `off-by-one` risk here is high (`dropTargetIdx - 1` vs `dropTargetIdx`). I'd want at minimum: - Move block from index 0 to end → correct order - Move block from end to index 0 → correct order - Move block down by 1 position → correct order (the `-1` branch) - `onReorder` is not called when `dropTargetIdx` is null **2. `useNotificationStream` — SSE event handling not tested** The test suite covers `fetchUnreadCount`, `fetchNotifications`, `markRead`, `markAllRead`, and `destroy`. Missing: a test that fires a `notification` event on the `MockEventSource` and verifies that `notifications` is prepended and `unreadCount` increments. This is the core SSE behavior that motivated the extraction. The `MockEventSource` has `addEventListener` but there's no test that calls the registered listener. **3. `useUnsavedWarning` has no tests at all** No test file exists for this hook. Given it controls a navigation guard (`beforeNavigate`), the missing coverage for `markDirty → navigate → warning shown` and `discard → navigation resumes` is a risk. ### What's Good - `vi.useFakeTimers()` + `vi.advanceTimersByTimeAsync()` for debounce testing is the right approach — no `Thread.sleep` equivalents. - `makeNotification()` factory function in `useNotificationStream.svelte.test.ts` is clean. - `useBlockAutoSave` tests cover the error→retry path explicitly — good edge case coverage.

marcel commented

2026-04-15 15:03:32 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

Pure frontend structural refactor — no new API endpoints, no new authentication surfaces, no new data flows. The security posture is unchanged. I audited the two spots that warranted close attention.

What I Checked

{@html renderBody(...)} in CommentMessage.svelte

<!-- eslint-disable-next-line svelte/no-at-html-tags -- renderBody escapes all HTML before injecting mention links -->
{@html renderBody(parsed.body, message.mentionDTOs ?? [])}

The comment claims renderBody escapes HTML before injecting mention links. This is the right pattern if the implementation actually does what it claims — sanitize user content, then inject trusted markup for mention anchors. The ESLint suppression with an inline justification is correct practice. Verify: confirm renderBody in $lib/utils/mention.ts escapes the raw body string before wrapping mentions in <a> tags. If it uses innerHTML or string interpolation without escaping, that's a stored XSS via comment content.

URL construction in useNotificationStream.svelte.ts

await fetch(`/api/notifications/${notification.id}/read`, { method: 'PATCH' });

notification.id is a TypeScript string sourced from the API response. It is not user-entered text and won't contain path traversal or injection payloads from the frontend. Safe as-is.

navigator.sendBeacon in useBlockAutoSave.svelte.ts

The plan's comment that sendBeacon silently fails in production because the app uses Authorization headers (not cookies) and sendBeacon cannot send custom headers is accurately preserved. The code works in the browser for unload flushing but the data won't reach the server in production. This is a pre-existing bug, correctly out of scope for this refactor. File the bug separately.

No New Issues Found

No XSS vectors, no IDOR risks, no auth bypass surface changes introduced by this PR.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** Pure frontend structural refactor — no new API endpoints, no new authentication surfaces, no new data flows. The security posture is unchanged. I audited the two spots that warranted close attention. ### What I Checked **`{@html renderBody(...)}` in `CommentMessage.svelte`** ```svelte  {@html renderBody(parsed.body, message.mentionDTOs ?? [])} ``` The comment claims `renderBody` escapes HTML before injecting mention links. This is the right pattern if the implementation actually does what it claims — sanitize user content, then inject trusted markup for mention anchors. The ESLint suppression with an inline justification is correct practice. **Verify**: confirm `renderBody` in `$lib/utils/mention.ts` escapes the raw `body` string before wrapping mentions in `<a>` tags. If it uses `innerHTML` or string interpolation without escaping, that's a stored XSS via comment content. **URL construction in `useNotificationStream.svelte.ts`** ```typescript await fetch(`/api/notifications/${notification.id}/read`, { method: 'PATCH' }); ``` `notification.id` is a TypeScript `string` sourced from the API response. It is not user-entered text and won't contain path traversal or injection payloads from the frontend. Safe as-is. **`navigator.sendBeacon` in `useBlockAutoSave.svelte.ts`** The plan's comment that `sendBeacon` silently fails in production because the app uses `Authorization` headers (not cookies) and `sendBeacon` cannot send custom headers is accurately preserved. The code works in the browser for unload flushing but the data won't reach the server in production. This is a pre-existing bug, correctly out of scope for this refactor. File the bug separately. ### No New Issues Found No XSS vectors, no IDOR risks, no auth bypass surface changes introduced by this PR.

marcel commented

2026-04-15 15:03:50 +02:00

🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist

Verdict: ⚠️ Approved with concerns

The accessibility improvements in this PR are real: role="log" on the message list, role="article" on individual messages, persistent aria-live wrapper for notification count, and delete button touch target increased to p-2. Good progress. But two things need fixing before merge.

Blockers

1. text-[9px] in EntityNavSection.svelte — below 12px minimum

This appears in both the tablet button and the flyout/desktop link:

<!-- tablet button count badge -->
<span class="text-[9px] font-bold ...">

<!-- flyout link label -->
<span class="text-[9px] font-extrabold tracking-[0.5px] uppercase ...">

9px is unreadable for the senior (60+) audience and fails WCAG's practical accessibility threshold. The desktop link uses text-[13px] which is marginally acceptable, but 9px for any visible text is a blocker. These labels are existing values being preserved from the original EntityNav.svelte — this refactor is the right time to fix them.

Fix: Use text-[11px] at minimum (matches other compact nav labels in the codebase), preferably text-xs (12px).

2. Hardcoded German in CommentMessage.svelte edit hint

<div class="mt-1 font-sans text-xs text-ink-3">Enter speichern · Esc abbrechen</div>

This is not an i18n concern only — it's a UX issue. English-speaking users see German instructions in an otherwise translated interface. This hint is visible every time someone edits a comment. Needs a Paraglide key.

Suggestions

UnsavedWarningBanner.svelte uses raw Tailwind amber values

class="... border-amber-200 bg-amber-50 text-amber-800 dark:border-amber-800 dark:bg-amber-950/40 dark:text-amber-300"

The project's design system uses semantic tokens (bg-surface, text-ink, border-line). The amber colors are raw Tailwind values that won't automatically participate in theming. This is preserved from the original admin pages, so it's a pre-existing issue — but worth flagging for a future cleanup ticket.

NotificationDropdown close button has no explicit label

The "View all" link at the bottom of the dropdown closes the dropdown via onclick={onClose}. The link itself has text content (m.notification_view_all()), so screen readers will announce it correctly. But the overall dropdown has role="dialog" with an aria-label pointing to m.notification_bell_label() — make sure that key returns something meaningful like "Benachrichtigungen" rather than a generic label.

## 🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist **Verdict: ⚠️ Approved with concerns** The accessibility improvements in this PR are real: `role="log"` on the message list, `role="article"` on individual messages, persistent `aria-live` wrapper for notification count, and delete button touch target increased to `p-2`. Good progress. But two things need fixing before merge. ### Blockers **1. `text-[9px]` in `EntityNavSection.svelte` — below 12px minimum** This appears in both the tablet button and the flyout/desktop link: ```svelte  <span class="text-[9px] font-bold ...">  <span class="text-[9px] font-extrabold tracking-[0.5px] uppercase ..."> ``` 9px is unreadable for the senior (60+) audience and fails WCAG's practical accessibility threshold. The desktop link uses `text-[13px]` which is marginally acceptable, but `9px` for any visible text is a blocker. These labels are existing values being preserved from the original `EntityNav.svelte` — this refactor is the right time to fix them. Fix: Use `text-[11px]` at minimum (matches other compact nav labels in the codebase), preferably `text-xs` (12px). **2. Hardcoded German in `CommentMessage.svelte` edit hint** ```svelte <div class="mt-1 font-sans text-xs text-ink-3">Enter speichern · Esc abbrechen</div> ``` This is not an i18n concern only — it's a UX issue. English-speaking users see German instructions in an otherwise translated interface. This hint is visible every time someone edits a comment. Needs a Paraglide key. ### Suggestions **`UnsavedWarningBanner.svelte` uses raw Tailwind amber values** ```svelte class="... border-amber-200 bg-amber-50 text-amber-800 dark:border-amber-800 dark:bg-amber-950/40 dark:text-amber-300" ``` The project's design system uses semantic tokens (`bg-surface`, `text-ink`, `border-line`). The amber colors are raw Tailwind values that won't automatically participate in theming. This is preserved from the original admin pages, so it's a pre-existing issue — but worth flagging for a future cleanup ticket. **`NotificationDropdown` close button has no explicit label** The "View all" link at the bottom of the dropdown closes the dropdown via `onclick={onClose}`. The link itself has text content (`m.notification_view_all()`), so screen readers will announce it correctly. But the overall dropdown has `role="dialog"` with an `aria-label` pointing to `m.notification_bell_label()` — make sure that key returns something meaningful like "Benachrichtigungen" rather than a generic label.

marcel commented

2026-04-15 15:03:59 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

Pure frontend refactor — no Docker Compose changes, no CI workflow changes, no new npm dependencies. Nothing here touches infrastructure.

What I Checked

No new packages added to package.json or package-lock.json
No changes to .gitea/workflows/ or docker-compose.yml
No new environment variables or secrets required
npm run build is listed in the PR test plan — good, confirms no bundle regressions

The extraction of usePdfRenderer with the dynamic import('pdfjs-dist') pattern preserves the existing code-split behavior — PDF.js is still loaded lazily and won't bloat the initial bundle.

813 tests green, build passes. Ship it (after the blockers from Felix and Leonie are addressed).

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** Pure frontend refactor — no Docker Compose changes, no CI workflow changes, no new npm dependencies. Nothing here touches infrastructure. ### What I Checked - No new packages added to `package.json` or `package-lock.json` - No changes to `.gitea/workflows/` or `docker-compose.yml` - No new environment variables or secrets required - `npm run build` is listed in the PR test plan — good, confirms no bundle regressions The extraction of `usePdfRenderer` with the dynamic `import('pdfjs-dist')` pattern preserves the existing code-split behavior — PDF.js is still loaded lazily and won't bloat the initial bundle. 813 tests green, build passes. Ship it (after the blockers from Felix and Leonie are addressed).

marcel added 5 commits 2026-04-15 15:22:51 +02:00

refactor(notifications): import relativeTime from canonical time.ts 2b93ccf92d

NotificationDropdown was importing relativeTime through notifications.ts,
creating an accidental coupling to a module unrelated to timestamp formatting.
Now imports directly from the canonical \$lib/utils/time module.

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

test(notifications): add SSE event handling tests for useNotificationStream 8739511058

Adds MockEventSource.simulate() helper and two tests covering:
- unread notification via SSE prepends to list and increments unreadCount
- read notification via SSE adds to list but does not increment unreadCount

Fixes @saraholt: "SSE event handling not tested"

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

fix(i18n): replace hardcoded German edit hint in CommentMessage with Paraglide key 7fb6ec04ab

Adds comment_edit_hint key to de/en/es message files and replaces the
hardcoded "Enter speichern · Esc abbrechen" string in CommentMessage.svelte.

Fixes @felixbrandt + @leonievoss: "hardcoded German bypasses Paraglide"

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

fix(a11y): increase nav label font size from 9px to 11px in EntityNavSection 45490ebaac

text-[9px] is below WCAG practical minimum and unreadable for senior users.
Changed all three occurrences (tablet button count, desktop link label,
flyout link label) to text-[11px].

Fixes @leonievoss: "text-[9px] is below 12px minimum"

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

test(drag-drop): add reorder logic tests for useBlockDragDrop

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

Details

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

Details

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

Details

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

Details

ed2c0231db

Adds simulateDragDrop helper and three tests covering the splice/insertAt
index arithmetic in handlePointerUp:
- move-to-end (insertAt path where target > fromIdx)
- move-to-start (insertAt path where target <= fromIdx)
- move-down-by-one (verifies the off-by-one dropTargetIdx - 1 branch)

Fixes @saraholt: "reorder calculation in handlePointerUp is untested"

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

marcel commented

2026-04-15 15:23:06 +02:00

Review concerns addressed

All blockers and test coverage concerns from the review have been resolved. 818 tests pass (up from 813).

Changes made

Blockers fixed:

2b93ccf — relativeTime in NotificationDropdown.svelte now imports from the canonical $lib/utils/time instead of $lib/utils/notifications — fixes @felixbrandt
7fb6ec0 — Added comment_edit_hint Paraglide key to de/en/es message files; replaced hardcoded "Enter speichern · Esc abbrechen" in CommentMessage.svelte with m.comment_edit_hint() — fixes @felixbrandt + @leonievoss
45490eb — Changed all three text-[9px] occurrences in EntityNavSection.svelte to text-[11px] (tablet button count badge, desktop link label, flyout link label) — fixes @leonievoss

Test coverage:

8739511 — Added MockEventSource.simulate(type, data) helper and two SSE event tests: unread notification prepends to list and increments unreadCount; read notification adds to list without incrementing count — fixes @saraholt
ed2c023 — Added simulateDragDrop helper with proper getBoundingClientRect mocks and three reorder tests covering move-to-end, move-to-start, and move-down-one-position (verifies the dropTargetIdx - 1 branch) — fixes @saraholt

Not acted on:

@saraholt: "useUnsavedWarning has no tests" — The test file useUnsavedWarning.svelte.test.ts already exists with 8 tests covering dirty state, navigation cancellation, and discard flow. The file was missed during the review scan.
@felixbrandt suggestion: add comment to handleBlur fire-and-forget — noted but not changed; the behavior is intentional and documented by the existing tests.
@markus suggestion: add comment about beforeNavigate constraint in useUnsavedWarning — good point for a follow-up but not blocking.

## Review concerns addressed All blockers and test coverage concerns from the review have been resolved. 818 tests pass (up from 813). ### Changes made **Blockers fixed:** - **2b93ccf** — `relativeTime` in `NotificationDropdown.svelte` now imports from the canonical `$lib/utils/time` instead of `$lib/utils/notifications` — fixes @felixbrandt - **7fb6ec0** — Added `comment_edit_hint` Paraglide key to de/en/es message files; replaced hardcoded `"Enter speichern · Esc abbrechen"` in `CommentMessage.svelte` with `m.comment_edit_hint()` — fixes @felixbrandt + @leonievoss - **45490eb** — Changed all three `text-[9px]` occurrences in `EntityNavSection.svelte` to `text-[11px]` (tablet button count badge, desktop link label, flyout link label) — fixes @leonievoss **Test coverage:** - **8739511** — Added `MockEventSource.simulate(type, data)` helper and two SSE event tests: unread notification prepends to list and increments `unreadCount`; read notification adds to list without incrementing count — fixes @saraholt - **ed2c023** — Added `simulateDragDrop` helper with proper `getBoundingClientRect` mocks and three reorder tests covering move-to-end, move-to-start, and move-down-one-position (verifies the `dropTargetIdx - 1` branch) — fixes @saraholt **Not acted on:** - @saraholt: "useUnsavedWarning has no tests" — The test file `useUnsavedWarning.svelte.test.ts` already exists with 8 tests covering dirty state, navigation cancellation, and discard flow. The file was missed during the review scan. - @felixbrandt suggestion: add comment to `handleBlur` fire-and-forget — noted but not changed; the behavior is intentional and documented by the existing tests. - @markus suggestion: add comment about `beforeNavigate` constraint in `useUnsavedWarning` — good point for a follow-up but not blocking.

marcel merged commit 4b8da0024f into main

2026-04-15 15:23:16 +02:00

marcel deleted branch refactor/issues-193-200

2026-04-15 15:23:16 +02:00

marcel referenced this issue from a commit

2026-04-15 15:23:20 +02:00

Merge pull request 'refactor(frontend): utility dedup, component splits, dead code removal (#193–#200)' (#241) from refactor/issues-193-200 into main

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#241