Extract shared utility functions to eliminate duplication #194

New Issue

marcel · 2026-04-07T10:47:45+02:00

marcel commented

2026-04-07 10:47:45 +02:00

Context

Frontend complexity audit identified several utility functions duplicated across components. These are pure-logic extractions that don't change any component markup.

Scope

Each item is one atomic commit:

1. Deduplicate `timeAgo` / `relativeTime`

CommentThread.svelte has its own timeAgo() (lines 70-79)
$lib/utils/notifications.ts exports relativeTime() — same logic, different name
Action: Delete timeAgo from CommentThread, import relativeTime (or move it to a shared $lib/utils/time.ts if we want to decouple it from notifications)

2. Consolidate dual `formatDate`

$lib/utils/date.ts exports formatDate() with fixed long format
$lib/utils/personFormat.ts exports its own formatDate() with 'short' | 'long' options
Action: Consolidate into one formatDate(iso, format) in date.ts. Have personFormat.ts re-export or call through.

3. Move `getInitials` to `personFormat.ts`

CommentThread.svelte has getInitials() (lines 89-95)
users/[id]/+page.svelte derives initials inline
PersonChip.svelte computes initials inline
Action: Add getInitials(name: string): string to $lib/utils/personFormat.ts, import everywhere

4. Extract `useUnsavedWarning.svelte.ts` hook

Identical pattern in admin/groups/[id], admin/tags/[id], admin/users/[id]
State: isDirty, showUnsavedWarning, discardTarget
Logic: beforeNavigate guard, form success reset
Action: Create $lib/hooks/useUnsavedWarning.svelte.ts returning { isDirty, showUnsavedWarning, discardTarget, markDirty, discard }
Also extract UnsavedWarningBanner.svelte for the shared amber alert markup

5. Extract `useFileLoader.svelte.ts` hook

documents/[id]/+page.svelte (lines 26-47) and enrich/[id]/+page.svelte have identical blob-URL file loading
Action: Create $lib/hooks/useFileLoader.svelte.ts returning { fileUrl, isLoading, fileError, loadFile }

6. Extract `useConversationFilters.svelte.ts` hook

briefwechsel/+page.svelte has filter state + applyFilters() + toggleSort() + swapPersons()
After conversations/ deletion (issue above), this becomes future-proofing for the briefwechsel route
Action: Create $lib/hooks/useConversationFilters.svelte.ts encapsulating filter state and URL sync

Acceptance Criteria

No duplicated utility functions remain
All existing tests still pass
New utility functions have unit tests
npm run check passes
Each extraction is one atomic commit

## Context Frontend complexity audit identified several utility functions duplicated across components. These are pure-logic extractions that don't change any component markup. ## Scope Each item is one atomic commit: ### 1. Deduplicate `timeAgo` / `relativeTime` - `CommentThread.svelte` has its own `timeAgo()` (lines 70-79) - `$lib/utils/notifications.ts` exports `relativeTime()` — same logic, different name - **Action:** Delete `timeAgo` from CommentThread, import `relativeTime` (or move it to a shared `$lib/utils/time.ts` if we want to decouple it from notifications) ### 2. Consolidate dual `formatDate` - `$lib/utils/date.ts` exports `formatDate()` with fixed long format - `$lib/utils/personFormat.ts` exports its own `formatDate()` with `'short'` | `'long'` options - **Action:** Consolidate into one `formatDate(iso, format)` in `date.ts`. Have `personFormat.ts` re-export or call through. ### 3. Move `getInitials` to `personFormat.ts` - `CommentThread.svelte` has `getInitials()` (lines 89-95) - `users/[id]/+page.svelte` derives initials inline - `PersonChip.svelte` computes initials inline - **Action:** Add `getInitials(name: string): string` to `$lib/utils/personFormat.ts`, import everywhere ### 4. Extract `useUnsavedWarning.svelte.ts` hook - Identical pattern in `admin/groups/[id]`, `admin/tags/[id]`, `admin/users/[id]` - State: `isDirty`, `showUnsavedWarning`, `discardTarget` - Logic: `beforeNavigate` guard, form success reset - **Action:** Create `$lib/hooks/useUnsavedWarning.svelte.ts` returning `{ isDirty, showUnsavedWarning, discardTarget, markDirty, discard }` - Also extract `UnsavedWarningBanner.svelte` for the shared amber alert markup ### 5. Extract `useFileLoader.svelte.ts` hook - `documents/[id]/+page.svelte` (lines 26-47) and `enrich/[id]/+page.svelte` have identical blob-URL file loading - **Action:** Create `$lib/hooks/useFileLoader.svelte.ts` returning `{ fileUrl, isLoading, fileError, loadFile }` ### 6. Extract `useConversationFilters.svelte.ts` hook - `briefwechsel/+page.svelte` has filter state + `applyFilters()` + `toggleSort()` + `swapPersons()` - After conversations/ deletion (issue above), this becomes future-proofing for the briefwechsel route - **Action:** Create `$lib/hooks/useConversationFilters.svelte.ts` encapsulating filter state and URL sync ## Acceptance Criteria - [ ] No duplicated utility functions remain - [ ] All existing tests still pass - [ ] New utility functions have unit tests - [ ] `npm run check` passes - [ ] Each extraction is one atomic commit

marcel added the refactor label 2026-04-07 10:48:54 +02:00

marcel commented

2026-04-07 10:51:16 +02:00

👨‍💻 Felix Brandt -- Senior Fullstack Developer

Good refactoring scope. A few things I want to nail down before implementation:

Item 1 -- `timeAgo` / `relativeTime`

Before deleting timeAgo from CommentThread.svelte, I'd want to diff the two implementations line by line. "Same logic" can hide edge cases (e.g., does one handle null dates? Different rounding for "just now"?). Have we actually confirmed they produce identical output for all inputs?
Moving to $lib/utils/time.ts is the right call -- relativeTime has nothing to do with notifications conceptually. The name relativeTime is better than timeAgo since it describes the return value, not the direction.

Item 2 -- Consolidate `formatDate`

The personFormat.ts version accepts 'short' | 'long' -- the date.ts version is fixed long. Consolidating means the new signature is formatDate(iso: string, format?: 'short' | 'long') with 'long' as default. That preserves backward compatibility at every existing call site. Just want to confirm that's the plan.
Which module owns it? date.ts is the natural home. personFormat.ts should import from date.ts, not re-export -- re-exports create import confusion when two paths resolve to the same function.

Item 3 -- `getInitials`

Three duplicates (CommentThread, users/[id], PersonChip) -- meets the 3+ rule for extraction. Clean case.
Edge cases to cover: single-word names, empty strings, names with multiple spaces, hyphenated names (e.g., "Anna-Maria Raddatz"). The test suite should define behavior for all of these before the function is written.

Item 4 -- `useUnsavedWarning` hook

This is the most complex extraction. The beforeNavigate guard interacts with SvelteKit's navigation lifecycle -- the hook needs to properly call navigation.cancel() and clean up on component destroy.
The return type { isDirty, showUnsavedWarning, discardTarget, markDirty, discard } mixes state and commands. That's fine for a hook, but isDirty should be a $derived from form state comparison, not a manually toggled boolean. Is the current implementation doing manual tracking or actual dirty-checking?
UnsavedWarningBanner.svelte extraction makes sense -- it's a distinct visual region with its own markup.

Item 5 -- `useFileLoader` hook

The blob URL lifecycle is the tricky part. The hook must call URL.revokeObjectURL() on cleanup. Is the current code in documents/[id]/+page.svelte doing this correctly? If not, this extraction is also a bug fix -- and per project rules, the bug fix should be a separate commit with its own test.
Return type looks right. loadFile should accept the document ID and fetch path, not hardcode the endpoint.

Item 6 -- `useConversationFilters` hook

The issue says "after conversations/ deletion, this becomes future-proofing." That's a yellow flag per our KISS-beats-DRY rule: extracting a hook used in exactly one place is premature. If briefwechsel/+page.svelte is the only consumer, I'd defer this until a second consumer appears.
If we proceed anyway, the URL sync logic (goto with search params) couples this hook to SvelteKit's router. That's acceptable but should be explicit in the hook's contract.

General

The $lib/hooks/ directory doesn't exist yet. Establishing the convention with items 4 and 5 is fine, but let's keep the naming pattern consistent: use[Feature].svelte.ts for hooks that use Svelte runes, plain .ts for pure logic.
Each commit needs its own unit test file. For items 1-3, tests go alongside the utility in $lib/utils/__tests__/. For items 4-5, tests go in $lib/hooks/__tests__/.

## 👨‍💻 Felix Brandt -- Senior Fullstack Developer Good refactoring scope. A few things I want to nail down before implementation: ### Item 1 -- `timeAgo` / `relativeTime` - Before deleting `timeAgo` from `CommentThread.svelte`, I'd want to diff the two implementations line by line. "Same logic" can hide edge cases (e.g., does one handle `null` dates? Different rounding for "just now"?). Have we actually confirmed they produce identical output for all inputs? - Moving to `$lib/utils/time.ts` is the right call -- `relativeTime` has nothing to do with notifications conceptually. The name `relativeTime` is better than `timeAgo` since it describes the return value, not the direction. ### Item 2 -- Consolidate `formatDate` - The `personFormat.ts` version accepts `'short' | 'long'` -- the `date.ts` version is fixed `long`. Consolidating means the new signature is `formatDate(iso: string, format?: 'short' | 'long')` with `'long'` as default. That preserves backward compatibility at every existing call site. Just want to confirm that's the plan. - Which module owns it? `date.ts` is the natural home. `personFormat.ts` should import from `date.ts`, not re-export -- re-exports create import confusion when two paths resolve to the same function. ### Item 3 -- `getInitials` - Three duplicates (CommentThread, users/[id], PersonChip) -- meets the 3+ rule for extraction. Clean case. - Edge cases to cover: single-word names, empty strings, names with multiple spaces, hyphenated names (e.g., "Anna-Maria Raddatz"). The test suite should define behavior for all of these before the function is written. ### Item 4 -- `useUnsavedWarning` hook - This is the most complex extraction. The `beforeNavigate` guard interacts with SvelteKit's navigation lifecycle -- the hook needs to properly call `navigation.cancel()` and clean up on component destroy. - The return type `{ isDirty, showUnsavedWarning, discardTarget, markDirty, discard }` mixes state and commands. That's fine for a hook, but `isDirty` should be a `$derived` from form state comparison, not a manually toggled boolean. Is the current implementation doing manual tracking or actual dirty-checking? - `UnsavedWarningBanner.svelte` extraction makes sense -- it's a distinct visual region with its own markup. ### Item 5 -- `useFileLoader` hook - The blob URL lifecycle is the tricky part. The hook must call `URL.revokeObjectURL()` on cleanup. Is the current code in `documents/[id]/+page.svelte` doing this correctly? If not, this extraction is also a bug fix -- and per project rules, the bug fix should be a separate commit with its own test. - Return type looks right. `loadFile` should accept the document ID and fetch path, not hardcode the endpoint. ### Item 6 -- `useConversationFilters` hook - The issue says "after conversations/ deletion, this becomes future-proofing." That's a yellow flag per our KISS-beats-DRY rule: extracting a hook used in exactly one place is premature. If `briefwechsel/+page.svelte` is the only consumer, I'd defer this until a second consumer appears. - If we proceed anyway, the URL sync logic (`goto` with search params) couples this hook to SvelteKit's router. That's acceptable but should be explicit in the hook's contract. ### General - The `$lib/hooks/` directory doesn't exist yet. Establishing the convention with items 4 and 5 is fine, but let's keep the naming pattern consistent: `use[Feature].svelte.ts` for hooks that use Svelte runes, plain `.ts` for pure logic. - Each commit needs its own unit test file. For items 1-3, tests go alongside the utility in `$lib/utils/__tests__/`. For items 4-5, tests go in `$lib/hooks/__tests__/`.

marcel commented

2026-04-07 10:51:38 +02:00

🏗️ Markus Keller -- Senior Application Architect

This is a well-scoped refactoring issue. The module boundaries and file locations matter here -- a few observations:

Module structure decisions

$lib/utils/ vs $lib/hooks/: The issue implicitly introduces a new hooks/ directory. This is the right separation -- utils/ for pure functions (no framework dependency), hooks/ for Svelte-rune-dependent state capsules. Worth documenting this convention once so it doesn't erode.
Import direction: personFormat.ts should depend on date.ts, never the reverse. Similarly, components import from utils/ and hooks/, never the other way. This is a one-way dependency graph: components -> hooks -> utils. Is this the intended layering?

Item-by-item

Item 1 (timeAgo/relativeTime): Moving to $lib/utils/time.ts is cleaner than keeping it in notifications.ts. notifications.ts is a domain module; relative time formatting is a general utility. Correct separation of concerns.

Item 2 (formatDate): One formatDate in date.ts with an options parameter is the right consolidation. But be careful about the Intl.DateTimeFormat instantiation -- if formatDate is called in a tight loop (e.g., rendering 100 documents), creating a new formatter per call is wasteful. Consider caching the formatter instances per locale+format combination, or at minimum, note that this is a known optimization point.

Item 3 (getInitials): Clean extraction. personFormat.ts is the right home -- it already owns person display logic. No architectural concern here.

Item 4 (useUnsavedWarning): This is the highest-value extraction. Three identical implementations in admin pages is exactly the kind of accidental complexity that grows quietly. Two questions:

Does the hook encapsulate the beforeNavigate registration and teardown, or does the consuming component still need to wire that up? The hook should own the full lifecycle.
The UnsavedWarningBanner.svelte component -- does it receive state via props from the hook, or does it internally use the hook? Props from the parent is the correct pattern (data flows down).

Item 5 (useFileLoader): The blob URL pattern appears in two places, and the lifecycle management (create/revoke) is easy to get wrong. Centralizing this is justified even at two occurrences because the failure mode (memory leak via unreleased blob URLs) is silent.

Item 6 (useConversationFilters): I share the concern about single-consumer extraction. The issue acknowledges this is "future-proofing" -- that's usually a signal to defer. The filter state + URL sync pattern in briefwechsel/+page.svelte is page-specific today. I'd recommend: keep this as item 6, implement it last, and only if the first five items reveal that the pattern genuinely generalizes.

Commit ordering

The issue says "each item is one atomic commit." Suggested dependency order:

Item 1 (time.ts) -- no dependencies
Item 3 (getInitials) -- no dependencies
Item 2 (formatDate consolidation) -- depends on understanding current call sites
Item 5 (useFileLoader) -- establishes hooks/ directory
Item 4 (useUnsavedWarning) -- most complex, benefits from hooks/ existing
Item 6 (useConversationFilters) -- defer or implement last

This ordering minimizes merge conflicts and lets each commit build on the previous directory structure.

## 🏗️ Markus Keller -- Senior Application Architect This is a well-scoped refactoring issue. The module boundaries and file locations matter here -- a few observations: ### Module structure decisions - **`$lib/utils/` vs `$lib/hooks/`**: The issue implicitly introduces a new `hooks/` directory. This is the right separation -- `utils/` for pure functions (no framework dependency), `hooks/` for Svelte-rune-dependent state capsules. Worth documenting this convention once so it doesn't erode. - **Import direction**: `personFormat.ts` should depend on `date.ts`, never the reverse. Similarly, components import from `utils/` and `hooks/`, never the other way. This is a one-way dependency graph: `components -> hooks -> utils`. Is this the intended layering? ### Item-by-item **Item 1 (timeAgo/relativeTime):** Moving to `$lib/utils/time.ts` is cleaner than keeping it in `notifications.ts`. `notifications.ts` is a domain module; relative time formatting is a general utility. Correct separation of concerns. **Item 2 (formatDate):** One `formatDate` in `date.ts` with an options parameter is the right consolidation. But be careful about the Intl.DateTimeFormat instantiation -- if `formatDate` is called in a tight loop (e.g., rendering 100 documents), creating a new formatter per call is wasteful. Consider caching the formatter instances per locale+format combination, or at minimum, note that this is a known optimization point. **Item 3 (getInitials):** Clean extraction. `personFormat.ts` is the right home -- it already owns person display logic. No architectural concern here. **Item 4 (useUnsavedWarning):** This is the highest-value extraction. Three identical implementations in admin pages is exactly the kind of accidental complexity that grows quietly. Two questions: - Does the hook encapsulate the `beforeNavigate` registration and teardown, or does the consuming component still need to wire that up? The hook should own the full lifecycle. - The `UnsavedWarningBanner.svelte` component -- does it receive state via props from the hook, or does it internally use the hook? Props from the parent is the correct pattern (data flows down). **Item 5 (useFileLoader):** The blob URL pattern appears in two places, and the lifecycle management (create/revoke) is easy to get wrong. Centralizing this is justified even at two occurrences because the failure mode (memory leak via unreleased blob URLs) is silent. **Item 6 (useConversationFilters):** I share the concern about single-consumer extraction. The issue acknowledges this is "future-proofing" -- that's usually a signal to defer. The filter state + URL sync pattern in `briefwechsel/+page.svelte` is page-specific today. I'd recommend: keep this as item 6, implement it last, and only if the first five items reveal that the pattern genuinely generalizes. ### Commit ordering The issue says "each item is one atomic commit." Suggested dependency order: 1. Item 1 (time.ts) -- no dependencies 2. Item 3 (getInitials) -- no dependencies 3. Item 2 (formatDate consolidation) -- depends on understanding current call sites 4. Item 5 (useFileLoader) -- establishes hooks/ directory 5. Item 4 (useUnsavedWarning) -- most complex, benefits from hooks/ existing 6. Item 6 (useConversationFilters) -- defer or implement last This ordering minimizes merge conflicts and lets each commit build on the previous directory structure.

marcel commented

2026-04-07 10:51:59 +02:00

🧪 Sara Holt -- Senior QA Engineer

The acceptance criteria mention "new utility functions have unit tests" -- here's what I'd want to see for each item:

Test strategy per item

Item 1 -- relativeTime (moved to time.ts)

Already tested in its current location? If notifications.ts has tests, we need to migrate those tests to match the new path. If it's untested today, this extraction is the right moment to add coverage.
Edge cases to test: null/undefined input, future dates, dates exactly on boundary values (59 seconds vs 1 minute, 23 hours vs 1 day), very old dates.
Time-dependent tests are flaky by nature. Use a fixed reference date (vi.useFakeTimers() with vi.setSystemTime()) -- never new Date() in assertions.

Item 2 -- consolidated formatDate

Test matrix: 'short' format, 'long' format, default (no format arg), invalid ISO string, empty string, null.
Locale sensitivity: the function uses Intl.DateTimeFormat('de-DE'). Tests should verify German output format explicitly. If we ever add locale as a parameter, the test suite is already ready.
The T12:00:00 suffix to prevent timezone off-by-one (per CLAUDE.md) -- there should be a test that catches the off-by-one if the suffix is removed. This is a regression guard.

Item 3 -- getInitials

Input variants: "Marcel Raddatz" -> "MR", "Marcel" -> "M", "" -> "", "Anna-Maria Raddatz" -> "AR" or "AMR"?, names with extra whitespace, names with unicode characters (umlauts: "Muller" vs "Mueller").
The three current implementations may handle edge cases differently. Before extracting, I'd write a characterization test for each existing implementation to document current behavior, then decide which behavior the shared function should adopt.

Item 4 -- useUnsavedWarning

This needs integration-level testing because it depends on SvelteKit's beforeNavigate. Pure unit testing won't catch navigation lifecycle issues.
Test cases: navigating away when dirty shows warning, navigating away when clean proceeds, discarding resets state and navigates, form submission success resets dirty state.
The beforeNavigate callback receives a navigation object -- mock it carefully. Verify navigation.cancel() is called when dirty, and not called when clean.

Item 5 -- useFileLoader

Test: loading triggers isLoading = true, successful load sets fileUrl and isLoading = false, failed load sets fileError, cleanup calls URL.revokeObjectURL.
Mock fetch and URL.createObjectURL / URL.revokeObjectURL -- verify the revoke call happens on both cleanup and re-load (replacing a previous URL).

Item 6 -- useConversationFilters

If implemented: test applyFilters updates URL params, toggleSort flips direction, swapPersons exchanges sender/receiver, filter reset clears all state.
URL sync testing requires mocking goto from $app/navigation.

Regression risk

The biggest risk is subtle behavioral differences between the duplicated implementations. Item 3 (getInitials) is the most likely candidate -- three independent implementations rarely handle all edge cases identically.
Recommendation: before each extraction, add a snapshot/characterization test at the original location, then verify the extracted version passes the same assertions.

CI impact

Six new test files, all unit-level. Estimated CI impact: < 1 second added. No concern.

## 🧪 Sara Holt -- Senior QA Engineer The acceptance criteria mention "new utility functions have unit tests" -- here's what I'd want to see for each item: ### Test strategy per item **Item 1 -- `relativeTime` (moved to `time.ts`)** - Already tested in its current location? If `notifications.ts` has tests, we need to migrate those tests to match the new path. If it's untested today, this extraction is the right moment to add coverage. - Edge cases to test: `null`/`undefined` input, future dates, dates exactly on boundary values (59 seconds vs 1 minute, 23 hours vs 1 day), very old dates. - Time-dependent tests are flaky by nature. Use a fixed reference date (`vi.useFakeTimers()` with `vi.setSystemTime()`) -- never `new Date()` in assertions. **Item 2 -- consolidated `formatDate`** - Test matrix: `'short'` format, `'long'` format, default (no format arg), invalid ISO string, empty string, `null`. - Locale sensitivity: the function uses `Intl.DateTimeFormat('de-DE')`. Tests should verify German output format explicitly. If we ever add locale as a parameter, the test suite is already ready. - The `T12:00:00` suffix to prevent timezone off-by-one (per CLAUDE.md) -- there should be a test that catches the off-by-one if the suffix is removed. This is a regression guard. **Item 3 -- `getInitials`** - Input variants: "Marcel Raddatz" -> "MR", "Marcel" -> "M", "" -> "", "Anna-Maria Raddatz" -> "AR" or "AMR"?, names with extra whitespace, names with unicode characters (umlauts: "Muller" vs "Mueller"). - The three current implementations may handle edge cases differently. Before extracting, I'd write a characterization test for each existing implementation to document current behavior, then decide which behavior the shared function should adopt. **Item 4 -- `useUnsavedWarning`** - This needs integration-level testing because it depends on SvelteKit's `beforeNavigate`. Pure unit testing won't catch navigation lifecycle issues. - Test cases: navigating away when dirty shows warning, navigating away when clean proceeds, discarding resets state and navigates, form submission success resets dirty state. - The `beforeNavigate` callback receives a navigation object -- mock it carefully. Verify `navigation.cancel()` is called when dirty, and not called when clean. **Item 5 -- `useFileLoader`** - Test: loading triggers `isLoading = true`, successful load sets `fileUrl` and `isLoading = false`, failed load sets `fileError`, cleanup calls `URL.revokeObjectURL`. - Mock `fetch` and `URL.createObjectURL` / `URL.revokeObjectURL` -- verify the revoke call happens on both cleanup and re-load (replacing a previous URL). **Item 6 -- `useConversationFilters`** - If implemented: test `applyFilters` updates URL params, `toggleSort` flips direction, `swapPersons` exchanges sender/receiver, filter reset clears all state. - URL sync testing requires mocking `goto` from `$app/navigation`. ### Regression risk - The biggest risk is subtle behavioral differences between the duplicated implementations. Item 3 (getInitials) is the most likely candidate -- three independent implementations rarely handle all edge cases identically. - Recommendation: before each extraction, add a snapshot/characterization test at the original location, then verify the extracted version passes the same assertions. ### CI impact - Six new test files, all unit-level. Estimated CI impact: < 1 second added. No concern.

marcel commented

2026-04-07 10:52:17 +02:00

🔒 Nora "NullX" Steiner -- Application Security Engineer

This is a pure frontend refactoring issue with no new endpoints or auth changes. Low security risk overall, but a few items warrant attention:

Item 5 -- `useFileLoader` hook (primary concern)

This is the only item that touches a security-relevant boundary -- it creates blob URLs from server-fetched file content.

Blob URL lifecycle: Unreleased blob URLs are a memory leak, but more importantly, blob URLs bypass CSP connect-src restrictions in some browsers. The hook must call URL.revokeObjectURL() on component teardown and when replacing a previous URL. Verify the current implementations do this correctly -- if they don't, the extraction should fix it.
Content-Type trust: When the hook calls URL.createObjectURL(blob), the blob's MIME type comes from the server's Content-Type header. If the server ever returns text/html for an uploaded file, the blob URL becomes an XSS vector when rendered in an <iframe> or <object>. Confirm that the file preview component sets sandbox on any iframe displaying blob URLs, or that the backend forces safe Content-Types (e.g., application/pdf, image/*).
Error disclosure: The fileError state should not leak internal server details (stack traces, file paths). Ensure the error message is sanitized to a user-friendly string.

Item 4 -- `useUnsavedWarning` hook (minor)

The discardTarget stores a URL that the user intended to navigate to. If this URL comes from user-controlled input (e.g., a query parameter), ensure it's validated before passing to goto(). Open redirect via discardTarget is low-risk in a SvelteKit app (since goto is client-side), but worth a defensive check.

Items 1-3 -- Pure utility functions (no concern)

relativeTime, formatDate, and getInitials are pure string transformations with no user input flowing to security-sensitive sinks. No issues.

Item 6 -- `useConversationFilters` (minor)

Filter values written to URL search params via goto() -- ensure these are encoded properly. SvelteKit's goto with URLSearchParams handles encoding automatically, but if anyone manually constructs the URL string, that's an injection point. Prefer the URLSearchParams API.

General

This refactoring doesn't change the attack surface. No new endpoints, no auth changes, no data flow changes. The main value from a security perspective is that centralizing blob URL management (item 5) makes it easier to audit and enforce correct cleanup in one place rather than hunting for it across multiple components.

## 🔒 Nora "NullX" Steiner -- Application Security Engineer This is a pure frontend refactoring issue with no new endpoints or auth changes. Low security risk overall, but a few items warrant attention: ### Item 5 -- `useFileLoader` hook (primary concern) This is the only item that touches a security-relevant boundary -- it creates blob URLs from server-fetched file content. - **Blob URL lifecycle**: Unreleased blob URLs are a memory leak, but more importantly, blob URLs bypass CSP `connect-src` restrictions in some browsers. The hook must call `URL.revokeObjectURL()` on component teardown and when replacing a previous URL. Verify the current implementations do this correctly -- if they don't, the extraction should fix it. - **Content-Type trust**: When the hook calls `URL.createObjectURL(blob)`, the blob's MIME type comes from the server's `Content-Type` header. If the server ever returns `text/html` for an uploaded file, the blob URL becomes an XSS vector when rendered in an `<iframe>` or `<object>`. Confirm that the file preview component sets `sandbox` on any iframe displaying blob URLs, or that the backend forces safe Content-Types (e.g., `application/pdf`, `image/*`). - **Error disclosure**: The `fileError` state should not leak internal server details (stack traces, file paths). Ensure the error message is sanitized to a user-friendly string. ### Item 4 -- `useUnsavedWarning` hook (minor) - The `discardTarget` stores a URL that the user intended to navigate to. If this URL comes from user-controlled input (e.g., a query parameter), ensure it's validated before passing to `goto()`. Open redirect via `discardTarget` is low-risk in a SvelteKit app (since `goto` is client-side), but worth a defensive check. ### Items 1-3 -- Pure utility functions (no concern) - `relativeTime`, `formatDate`, and `getInitials` are pure string transformations with no user input flowing to security-sensitive sinks. No issues. ### Item 6 -- `useConversationFilters` (minor) - Filter values written to URL search params via `goto()` -- ensure these are encoded properly. SvelteKit's `goto` with `URLSearchParams` handles encoding automatically, but if anyone manually constructs the URL string, that's an injection point. Prefer the `URLSearchParams` API. ### General - This refactoring doesn't change the attack surface. No new endpoints, no auth changes, no data flow changes. The main value from a security perspective is that centralizing blob URL management (item 5) makes it easier to audit and enforce correct cleanup in one place rather than hunting for it across multiple components.

marcel commented

2026-04-07 10:52:35 +02:00

🎨 Leonie Voss -- UI/UX Design Lead

This issue is explicitly "pure-logic extractions that don't change any component markup" -- so no visual regressions expected. Still, a few items touch user-facing behavior worth watching:

Item 4 -- `UnsavedWarningBanner.svelte`

The issue mentions extracting a shared amber alert component for the unsaved warning. This is a visual component extraction, not just logic. Before implementing:
- Are the three existing amber banners (groups, tags, users) visually identical today? If they've drifted (different padding, different icon, different text), the extraction should normalize them to one consistent design.
- The banner should follow our card pattern: bg-amber-50 border border-amber-200 rounded-sm with consistent padding. Confirm it meets the 4.5:1 contrast ratio for the warning text against the amber background.
- Touch target on any dismiss or discard button: minimum 44x44px.
- The banner text should be a Paraglide translation key, not hardcoded German. Are the current implementations using i18n?

Item 1 -- `relativeTime` display

relativeTime output appears in the conversation thread UI. If the function signature or output format changes during consolidation (e.g., "vor 2 Stunden" vs "2h ago"), that's a visible UI change. Ensure the consolidated function produces the same German-language output the user currently sees.

Item 2 -- `formatDate` consolidation

The 'short' vs 'long' format distinction matters for the UI. Short format is typically used in dense lists (person detail, document cards), long format in detail views. After consolidation, verify that every call site still passes the correct format argument -- a missing argument defaulting to 'long' in a place that currently shows 'short' would be a visual regression.

Item 3 -- `getInitials` in avatars

getInitials output is displayed in avatar circles (CommentThread, PersonChip). The visual space is tight -- typically a 32-40px circle. If the extracted function returns 3 characters for hyphenated names (e.g., "AMR" for "Anna-Maria Raddatz"), the text may overflow the circle. Define maximum 2 characters as the contract, truncating to first and last initials.
Ensure the initials are uppercase -- the three current implementations may or may not normalize case.

No visual spec needed

Since this doesn't introduce new UI, no spec file is required. But I'd recommend a quick visual check (manual or screenshot test) of the conversation thread and admin pages after implementation to confirm nothing shifted.

## 🎨 Leonie Voss -- UI/UX Design Lead This issue is explicitly "pure-logic extractions that don't change any component markup" -- so no visual regressions expected. Still, a few items touch user-facing behavior worth watching: ### Item 4 -- `UnsavedWarningBanner.svelte` - The issue mentions extracting a shared amber alert component for the unsaved warning. This is a visual component extraction, not just logic. Before implementing: - Are the three existing amber banners (groups, tags, users) visually identical today? If they've drifted (different padding, different icon, different text), the extraction should normalize them to one consistent design. - The banner should follow our card pattern: `bg-amber-50 border border-amber-200 rounded-sm` with consistent padding. Confirm it meets the 4.5:1 contrast ratio for the warning text against the amber background. - Touch target on any dismiss or discard button: minimum 44x44px. - The banner text should be a Paraglide translation key, not hardcoded German. Are the current implementations using i18n? ### Item 1 -- `relativeTime` display - `relativeTime` output appears in the conversation thread UI. If the function signature or output format changes during consolidation (e.g., "vor 2 Stunden" vs "2h ago"), that's a visible UI change. Ensure the consolidated function produces the same German-language output the user currently sees. ### Item 2 -- `formatDate` consolidation - The `'short'` vs `'long'` format distinction matters for the UI. Short format is typically used in dense lists (person detail, document cards), long format in detail views. After consolidation, verify that every call site still passes the correct format argument -- a missing argument defaulting to `'long'` in a place that currently shows `'short'` would be a visual regression. ### Item 3 -- `getInitials` in avatars - `getInitials` output is displayed in avatar circles (CommentThread, PersonChip). The visual space is tight -- typically a 32-40px circle. If the extracted function returns 3 characters for hyphenated names (e.g., "AMR" for "Anna-Maria Raddatz"), the text may overflow the circle. Define maximum 2 characters as the contract, truncating to first and last initials. - Ensure the initials are uppercase -- the three current implementations may or may not normalize case. ### No visual spec needed Since this doesn't introduce new UI, no spec file is required. But I'd recommend a quick visual check (manual or screenshot test) of the conversation thread and admin pages after implementation to confirm nothing shifted.

marcel commented

2026-04-07 10:52:52 +02:00

⚙️ Tobias Wendt -- DevOps & Platform Engineer

Pure frontend refactoring -- no infrastructure changes needed. A few CI and build considerations:

New directory `$lib/hooks/`

Vite/SvelteKit will pick up .svelte.ts files in $lib/hooks/ automatically via the $lib alias. No build config changes needed.
Vitest should also discover $lib/hooks/__tests__/*.test.ts without configuration changes, assuming the existing vitest.config.ts includes $lib in the test paths. Worth a quick check before starting.

CI pipeline impact

Six atomic commits, each with new or moved test files. The npm run check and npm run test steps in CI already cover this. No pipeline changes required.
The E2E suite should be run after the full set of changes lands (not per-commit) to catch any navigation or display regressions. The existing Playwright suite covers the admin pages (where useUnsavedWarning lives) and the document detail page (where useFileLoader lives).

Tree-shaking / bundle size

Moving utilities from component-level inline functions to $lib/ imports should not increase bundle size -- Vite's tree-shaking handles this. But if time.ts or date.ts imports grow (e.g., pulling in a date library), that could affect the client bundle.
Recommendation: after all six commits, run npm run build and compare the output size. If any chunk grows by more than 1KB, investigate.

Dependency changes

This issue introduces no new npm dependencies. Good -- no node_modules or lockfile changes to worry about.

Suggested validation before merge

npm run check && npm run test && npm run build && npm run lint

If all four pass, the refactoring is safe from a build/CI perspective. The existing E2E suite provides the integration safety net.

## ⚙️ Tobias Wendt -- DevOps & Platform Engineer Pure frontend refactoring -- no infrastructure changes needed. A few CI and build considerations: ### New directory `$lib/hooks/` - Vite/SvelteKit will pick up `.svelte.ts` files in `$lib/hooks/` automatically via the `$lib` alias. No build config changes needed. - Vitest should also discover `$lib/hooks/__tests__/*.test.ts` without configuration changes, assuming the existing `vitest.config.ts` includes `$lib` in the test paths. Worth a quick check before starting. ### CI pipeline impact - Six atomic commits, each with new or moved test files. The `npm run check` and `npm run test` steps in CI already cover this. No pipeline changes required. - The E2E suite should be run after the full set of changes lands (not per-commit) to catch any navigation or display regressions. The existing Playwright suite covers the admin pages (where `useUnsavedWarning` lives) and the document detail page (where `useFileLoader` lives). ### Tree-shaking / bundle size - Moving utilities from component-level inline functions to `$lib/` imports should not increase bundle size -- Vite's tree-shaking handles this. But if `time.ts` or `date.ts` imports grow (e.g., pulling in a date library), that could affect the client bundle. - Recommendation: after all six commits, run `npm run build` and compare the output size. If any chunk grows by more than 1KB, investigate. ### Dependency changes - This issue introduces no new npm dependencies. Good -- no `node_modules` or lockfile changes to worry about. ### Suggested validation before merge ```bash npm run check && npm run test && npm run build && npm run lint ``` If all four pass, the refactoring is safe from a build/CI perspective. The existing E2E suite provides the integration safety net.

marcel commented

2026-04-07 11:05:30 +02:00

Addressing Review Feedback

All concerns verified against source code. Responses organized by persona.

Re: Felix Brandt

Item 1 -- timeAgo vs relativeTime are NOT identical. timeAgo (CommentThread.svelte:70-79) uses Paraglide translation keys (m.comment_time_just_now(), m.comment_time_minutes(), etc.) and returns translated German strings. relativeTime (notifications.ts:15-24) uses Intl.RelativeTimeFormat with the browser locale. Different output for the same input. The consolidation must use the Paraglide approach (CommentThread's logic) since that's i18n-correct. Moving to $lib/utils/time.ts and using Paraglide keys is the plan.
Item 2 -- Consolidation plan confirmed. formatDate(iso, format?: 'short' | 'long') with 'long' as default in date.ts. personFormat.ts will import from date.ts, not re-export. Current consumers: date.ts formatDate (no format arg, always long) is used by ConversationTimeline.svelte, DocumentList.svelte, PersonDocumentList.svelte, DocumentMetadataDrawer.svelte. personFormat.ts formatDate (requires format arg) is used by DocumentTopBar.svelte and internally by formatXsMeta. All call sites will work with the consolidated signature. Note: DashboardRecentDocuments.svelte:22 has a third local formatDate that handles full ISO datetimes (not date-only) with getLocale() -- this one stays separate since it serves a different purpose (datetime vs date).
Item 3 -- Three implementations confirmed, but actually FIVE exist. CommentThread.svelte:89 (splits on whitespace, takes first 2 words), PersonChip.svelte:17 (firstName.charAt(0) + lastName.charAt(0)), users/[id]/+page.svelte:12 (handles first-only, last-only, both, returns null for neither), DocumentMetadataDrawer.svelte:35 (same as PersonChip), CorrespondentSuggestionsDropdown.svelte:43 (same as PersonChip). Edge case differences: CommentThread handles single-word names and multi-word names but NOT hyphenated first names (e.g., "Anna-Maria Raddatz" -> "AR", takes first char of each whitespace-separated word, max 2). PersonChip/MetadataDrawer/CorrespondentSuggestionsDropdown always take first char of firstName + first char of lastName -- will break on empty firstName or lastName. users/[id] gracefully handles missing names and returns null. The shared getInitials needs two signatures: one for (name: string) (CommentThread use case -- full name string) and one for (person: {firstName, lastName}) (structured person). Or a single getInitials(name: string) and callers construct the string. Max 2 characters as Leonie suggests.
Item 4 -- isDirty is manual tracking, not derived. All three admin pages (groups/[id]:8-9, tags/[id]:11-12, users/[id]:13-14) use let isDirty = $state(false) toggled by oninput on the form. No dirty-checking against original values. This is acceptable for the hook extraction -- the hook will expose markDirty() (called from oninput) and the form success effect resets it. Actual value-comparison dirty checking would be a separate enhancement.
Item 5 -- Blob URL cleanup is MISSING in both implementations. Neither documents/[id]/+page.svelte nor enrich/[id]/+page.svelte calls URL.revokeObjectURL(). This is a confirmed memory leak. Per project rules, the bug fix (adding revoke on cleanup and re-load) will be a separate commit before the hook extraction. The hook will own the full lifecycle: revoke previous URL before creating a new one, and revoke on component teardown.
Item 6 -- Agree to defer. briefwechsel/+page.svelte is the only consumer. KISS beats DRY. Will remove from this issue's scope.
$lib/hooks/ naming convention confirmed. .svelte.ts for hooks using Svelte runes, plain .ts for pure logic. Tests in __tests__/ subdirectories.

Re: Markus Keller

Layering rule confirmed: components -> hooks -> utils. Hooks may import from utils, never the reverse. Components import from both.
Item 2 -- Intl.DateTimeFormat caching: Current code creates a new formatter per call. For document lists rendering 50-100 items, this is measurable but not critical. Will note as optimization point but not block this issue. If we cache, a simple module-level Map<string, Intl.DateTimeFormat> keyed by ${locale}-${format} suffices.
Commit ordering accepted. Will follow: 1 (time.ts) -> 3 (getInitials) -> 2 (formatDate) -> 5 (useFileLoader, establishes hooks/) -> 4 (useUnsavedWarning) -> 6 (deferred).

Re: Sara Holt

Item 1 -- relativeTime already has tests in notifications.spec.ts (8 test cases covering boundary values, uses fixed now parameter). These tests will move to time.spec.ts. The timeAgo variant from CommentThread is untested -- new tests needed for the Paraglide-based version.
Item 2 -- test matrix accepted. Will include: short, long, default (no arg), and the T12:00:00 regression guard test.
Item 3 -- characterization tests first, agreed. The five implementations have different behavior for edge cases. Will document current behavior before choosing the canonical one.
Item 4 -- beforeNavigate mock: Vitest test config has a browser project (svelte.{test,spec}.ts pattern, runs in Chromium via Playwright). Hook tests with .svelte.test.ts extension will run in browser context where SvelteKit navigation can be mocked. The navigation.cancel() verification is testable.
Item 5 -- Mock strategy confirmed. Will mock fetch, URL.createObjectURL, URL.revokeObjectURL. Will verify revoke on both cleanup and re-load.

Re: Nora "NullX" Steiner

Item 5 -- Blob URL lifecycle: confirmed bug. Neither implementation calls revokeObjectURL. Will fix as separate commit. On the Content-Type concern: the DocumentViewer (DocumentViewer.svelte:85-104) renders blob URLs only as <img> tags or passes them to PdfViewer (canvas-based rendering via pdfjs-dist). No <iframe> or <object> tags are used, so the XSS-via-text/html vector does not apply. Error messages are hardcoded user-friendly strings ("Nicht eingeloggt", "Fehler beim Laden der Datei", m.doc_file_error_preview()) -- no server details leak.
Item 4 -- discardTarget open redirect: discardTarget is set from to?.url.href in the beforeNavigate callback (groups/[id]:13, tags/[id]:16, users/[id]:18). The to object comes from SvelteKit's navigation lifecycle -- it represents the URL the user clicked within the app. It is not user-controlled input from query params. SvelteKit's goto() is client-side only. No open redirect risk.
Item 6 -- URL encoding: briefwechsel/+page.svelte:70 uses new SvelteURLSearchParams() with .set() calls -- encoding is handled by the URLSearchParams API. No manual string construction. Safe.

Re: Leonie Voss

Item 4 -- Amber banners are visually identical across all three admin pages. Same classes: mb-5 flex items-center justify-between rounded border border-amber-200 bg-amber-50 p-3 text-sm text-amber-800 dark:border-amber-800 dark:bg-amber-950/40 dark:text-amber-300. Same i18n keys: m.admin_unsaved_warning() for text, m.person_discard_changes() for discard button. Extraction will produce a pixel-identical component. All text uses Paraglide translation keys, not hardcoded German.
Item 1 -- relativeTime output format: As noted above, the two implementations produce different output. The consolidated version will keep the Paraglide-based output (translated strings), matching what CommentThread users currently see. Notification consumers currently see Intl.RelativeTimeFormat output -- this will change to the Paraglide format. This is a minor visual change in notifications that improves i18n consistency.
Item 2 -- Format argument audit: Every current consumer of date.ts formatDate passes no format arg (= long). Every consumer of personFormat.ts formatDate passes an explicit format arg. After consolidation with 'long' as default, all existing call sites produce identical output. No visual regression.
Item 3 -- Max 2 characters agreed. The shared function will return at most 2 uppercase characters. For "Anna-Maria Raddatz" the result will be "AR" (first char of first word + first char of last word), not "AMR". All implementations already produce uppercase output.

Decisions

Item	Decision	Reasoning
1 (timeAgo/relativeTime)	Proceed -- use Paraglide-based logic, move to `$lib/utils/time.ts`	Two implementations with different output; Paraglide version is i18n-correct
2 (formatDate)	Proceed -- consolidate in `date.ts` with optional format param	Clear dedup, backward-compatible signature
3 (getInitials)	Proceed -- add to `personFormat.ts`, max 2 chars	Five duplicates found (not three), clear extraction case
4 (useUnsavedWarning)	Proceed -- extract hook + banner component	Three identical implementations, highest-value extraction
5 (useFileLoader)	Proceed -- fix blob leak as separate commit first, then extract	Bug fix + dedup, two consumers
6 (useConversationFilters)	Defer -- single consumer, KISS wins	Remove from this issue's scope

Updated Acceptance Criteria

No duplicated utility functions remain (items 1-5)
Blob URL memory leak fixed as standalone bug-fix commit before hook extraction
All existing tests still pass
New utility functions have unit tests (characterization tests for getInitials edge cases)
npm run check passes
Each extraction is one atomic commit
$lib/hooks/ convention: .svelte.ts for rune-dependent hooks, .ts for pure logic
Item 6 (useConversationFilters) deferred -- removed from scope
relativeTime consolidation uses Paraglide keys (visual change in notifications acknowledged)
getInitials returns max 2 uppercase characters

## Addressing Review Feedback All concerns verified against source code. Responses organized by persona. --- ### Re: Felix Brandt - **Item 1 -- `timeAgo` vs `relativeTime` are NOT identical.** `timeAgo` (CommentThread.svelte:70-79) uses Paraglide translation keys (`m.comment_time_just_now()`, `m.comment_time_minutes()`, etc.) and returns translated German strings. `relativeTime` (notifications.ts:15-24) uses `Intl.RelativeTimeFormat` with the browser locale. Different output for the same input. The consolidation must use the Paraglide approach (CommentThread's logic) since that's i18n-correct. Moving to `$lib/utils/time.ts` and using Paraglide keys is the plan. - **Item 2 -- Consolidation plan confirmed.** `formatDate(iso, format?: 'short' | 'long')` with `'long'` as default in `date.ts`. `personFormat.ts` will import from `date.ts`, not re-export. Current consumers: `date.ts` `formatDate` (no format arg, always long) is used by `ConversationTimeline.svelte`, `DocumentList.svelte`, `PersonDocumentList.svelte`, `DocumentMetadataDrawer.svelte`. `personFormat.ts` `formatDate` (requires format arg) is used by `DocumentTopBar.svelte` and internally by `formatXsMeta`. All call sites will work with the consolidated signature. Note: `DashboardRecentDocuments.svelte:22` has a third local `formatDate` that handles full ISO datetimes (not date-only) with `getLocale()` -- this one stays separate since it serves a different purpose (datetime vs date). - **Item 3 -- Three implementations confirmed, but actually FIVE exist.** CommentThread.svelte:89 (splits on whitespace, takes first 2 words), PersonChip.svelte:17 (`firstName.charAt(0) + lastName.charAt(0)`), users/[id]/+page.svelte:12 (handles first-only, last-only, both, returns null for neither), DocumentMetadataDrawer.svelte:35 (same as PersonChip), CorrespondentSuggestionsDropdown.svelte:43 (same as PersonChip). Edge case differences: CommentThread handles single-word names and multi-word names but NOT hyphenated first names (e.g., "Anna-Maria Raddatz" -> "AR", takes first char of each whitespace-separated word, max 2). PersonChip/MetadataDrawer/CorrespondentSuggestionsDropdown always take first char of firstName + first char of lastName -- will break on empty firstName or lastName. users/[id] gracefully handles missing names and returns null. The shared `getInitials` needs two signatures: one for `(name: string)` (CommentThread use case -- full name string) and one for `(person: {firstName, lastName})` (structured person). Or a single `getInitials(name: string)` and callers construct the string. Max 2 characters as Leonie suggests. - **Item 4 -- `isDirty` is manual tracking, not derived.** All three admin pages (groups/[id]:8-9, tags/[id]:11-12, users/[id]:13-14) use `let isDirty = $state(false)` toggled by `oninput` on the form. No dirty-checking against original values. This is acceptable for the hook extraction -- the hook will expose `markDirty()` (called from `oninput`) and the form success effect resets it. Actual value-comparison dirty checking would be a separate enhancement. - **Item 5 -- Blob URL cleanup is MISSING in both implementations.** Neither `documents/[id]/+page.svelte` nor `enrich/[id]/+page.svelte` calls `URL.revokeObjectURL()`. This is a confirmed memory leak. Per project rules, the bug fix (adding revoke on cleanup and re-load) will be a separate commit before the hook extraction. The hook will own the full lifecycle: revoke previous URL before creating a new one, and revoke on component teardown. - **Item 6 -- Agree to defer.** `briefwechsel/+page.svelte` is the only consumer. KISS beats DRY. Will remove from this issue's scope. - **`$lib/hooks/` naming convention confirmed.** `.svelte.ts` for hooks using Svelte runes, plain `.ts` for pure logic. Tests in `__tests__/` subdirectories. --- ### Re: Markus Keller - **Layering rule confirmed:** `components -> hooks -> utils`. Hooks may import from utils, never the reverse. Components import from both. - **Item 2 -- `Intl.DateTimeFormat` caching:** Current code creates a new formatter per call. For document lists rendering 50-100 items, this is measurable but not critical. Will note as optimization point but not block this issue. If we cache, a simple module-level `Map<string, Intl.DateTimeFormat>` keyed by `${locale}-${format}` suffices. - **Commit ordering accepted.** Will follow: 1 (time.ts) -> 3 (getInitials) -> 2 (formatDate) -> 5 (useFileLoader, establishes hooks/) -> 4 (useUnsavedWarning) -> 6 (deferred). --- ### Re: Sara Holt - **Item 1 -- `relativeTime` already has tests** in `notifications.spec.ts` (8 test cases covering boundary values, uses fixed `now` parameter). These tests will move to `time.spec.ts`. The `timeAgo` variant from CommentThread is untested -- new tests needed for the Paraglide-based version. - **Item 2 -- test matrix accepted.** Will include: short, long, default (no arg), and the T12:00:00 regression guard test. - **Item 3 -- characterization tests first, agreed.** The five implementations have different behavior for edge cases. Will document current behavior before choosing the canonical one. - **Item 4 -- `beforeNavigate` mock:** Vitest test config has a browser project (`svelte.{test,spec}.ts` pattern, runs in Chromium via Playwright). Hook tests with `.svelte.test.ts` extension will run in browser context where SvelteKit navigation can be mocked. The `navigation.cancel()` verification is testable. - **Item 5 -- Mock strategy confirmed.** Will mock `fetch`, `URL.createObjectURL`, `URL.revokeObjectURL`. Will verify revoke on both cleanup and re-load. --- ### Re: Nora "NullX" Steiner - **Item 5 -- Blob URL lifecycle: confirmed bug.** Neither implementation calls `revokeObjectURL`. Will fix as separate commit. On the Content-Type concern: the `DocumentViewer` (DocumentViewer.svelte:85-104) renders blob URLs only as `<img>` tags or passes them to `PdfViewer` (canvas-based rendering via pdfjs-dist). No `<iframe>` or `<object>` tags are used, so the XSS-via-text/html vector does not apply. Error messages are hardcoded user-friendly strings ("Nicht eingeloggt", "Fehler beim Laden der Datei", `m.doc_file_error_preview()`) -- no server details leak. - **Item 4 -- `discardTarget` open redirect:** `discardTarget` is set from `to?.url.href` in the `beforeNavigate` callback (groups/[id]:13, tags/[id]:16, users/[id]:18). The `to` object comes from SvelteKit's navigation lifecycle -- it represents the URL the user clicked within the app. It is not user-controlled input from query params. SvelteKit's `goto()` is client-side only. No open redirect risk. - **Item 6 -- URL encoding:** `briefwechsel/+page.svelte:70` uses `new SvelteURLSearchParams()` with `.set()` calls -- encoding is handled by the URLSearchParams API. No manual string construction. Safe. --- ### Re: Leonie Voss - **Item 4 -- Amber banners are visually identical** across all three admin pages. Same classes: `mb-5 flex items-center justify-between rounded border border-amber-200 bg-amber-50 p-3 text-sm text-amber-800 dark:border-amber-800 dark:bg-amber-950/40 dark:text-amber-300`. Same i18n keys: `m.admin_unsaved_warning()` for text, `m.person_discard_changes()` for discard button. Extraction will produce a pixel-identical component. All text uses Paraglide translation keys, not hardcoded German. - **Item 1 -- `relativeTime` output format:** As noted above, the two implementations produce different output. The consolidated version will keep the Paraglide-based output (translated strings), matching what CommentThread users currently see. Notification consumers currently see `Intl.RelativeTimeFormat` output -- this will change to the Paraglide format. This is a minor visual change in notifications that improves i18n consistency. - **Item 2 -- Format argument audit:** Every current consumer of `date.ts` `formatDate` passes no format arg (= long). Every consumer of `personFormat.ts` `formatDate` passes an explicit format arg. After consolidation with `'long'` as default, all existing call sites produce identical output. No visual regression. - **Item 3 -- Max 2 characters agreed.** The shared function will return at most 2 uppercase characters. For "Anna-Maria Raddatz" the result will be "AR" (first char of first word + first char of last word), not "AMR". All implementations already produce uppercase output. --- ### Decisions | Item | Decision | Reasoning | |------|----------|-----------| | 1 (timeAgo/relativeTime) | **Proceed** -- use Paraglide-based logic, move to `$lib/utils/time.ts` | Two implementations with different output; Paraglide version is i18n-correct | | 2 (formatDate) | **Proceed** -- consolidate in `date.ts` with optional format param | Clear dedup, backward-compatible signature | | 3 (getInitials) | **Proceed** -- add to `personFormat.ts`, max 2 chars | Five duplicates found (not three), clear extraction case | | 4 (useUnsavedWarning) | **Proceed** -- extract hook + banner component | Three identical implementations, highest-value extraction | | 5 (useFileLoader) | **Proceed** -- fix blob leak as separate commit first, then extract | Bug fix + dedup, two consumers | | 6 (useConversationFilters) | **Defer** -- single consumer, KISS wins | Remove from this issue's scope | ### Updated Acceptance Criteria - [ ] No duplicated utility functions remain (items 1-5) - [ ] Blob URL memory leak fixed as standalone bug-fix commit before hook extraction - [ ] All existing tests still pass - [ ] New utility functions have unit tests (characterization tests for getInitials edge cases) - [ ] `npm run check` passes - [ ] Each extraction is one atomic commit - [ ] `$lib/hooks/` convention: `.svelte.ts` for rune-dependent hooks, `.ts` for pure logic - [ ] Item 6 (useConversationFilters) deferred -- removed from scope - [ ] relativeTime consolidation uses Paraglide keys (visual change in notifications acknowledged) - [ ] getInitials returns max 2 uppercase characters

marcel referenced this issue

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

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

marcel referenced this issue

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

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

marcel closed this issue

2026-04-15 21:02:35 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#194

Extract shared utility functions to eliminate duplication #194

Context

Scope

1. Deduplicate timeAgo / relativeTime

2. Consolidate dual formatDate

3. Move getInitials to personFormat.ts

4. Extract useUnsavedWarning.svelte.ts hook

5. Extract useFileLoader.svelte.ts hook

6. Extract useConversationFilters.svelte.ts hook

Acceptance Criteria

👨‍💻 Felix Brandt -- Senior Fullstack Developer

Item 1 -- timeAgo / relativeTime

Item 2 -- Consolidate formatDate

Item 3 -- getInitials

Item 4 -- useUnsavedWarning hook

Item 5 -- useFileLoader hook

Item 6 -- useConversationFilters hook

General

🏗️ Markus Keller -- Senior Application Architect

Module structure decisions

Item-by-item

Commit ordering

🧪 Sara Holt -- Senior QA Engineer

Test strategy per item

Regression risk

CI impact

🔒 Nora "NullX" Steiner -- Application Security Engineer

Item 5 -- useFileLoader hook (primary concern)

Item 4 -- useUnsavedWarning hook (minor)

Items 1-3 -- Pure utility functions (no concern)

Item 6 -- useConversationFilters (minor)

General

🎨 Leonie Voss -- UI/UX Design Lead

Item 4 -- UnsavedWarningBanner.svelte

Item 1 -- relativeTime display

Item 2 -- formatDate consolidation

Item 3 -- getInitials in avatars

No visual spec needed

⚙️ Tobias Wendt -- DevOps & Platform Engineer

New directory $lib/hooks/

CI pipeline impact

Tree-shaking / bundle size

Dependency changes

Suggested validation before merge

Addressing Review Feedback

Re: Felix Brandt

Re: Markus Keller

Re: Sara Holt

Re: Nora "NullX" Steiner

Re: Leonie Voss

Decisions

Updated Acceptance Criteria

1. Deduplicate `timeAgo` / `relativeTime`

2. Consolidate dual `formatDate`

3. Move `getInitials` to `personFormat.ts`

4. Extract `useUnsavedWarning.svelte.ts` hook

5. Extract `useFileLoader.svelte.ts` hook

6. Extract `useConversationFilters.svelte.ts` hook

Item 1 -- `timeAgo` / `relativeTime`

Item 2 -- Consolidate `formatDate`

Item 3 -- `getInitials`

Item 4 -- `useUnsavedWarning` hook

Item 5 -- `useFileLoader` hook

Item 6 -- `useConversationFilters` hook

Item 5 -- `useFileLoader` hook (primary concern)

Item 4 -- `useUnsavedWarning` hook (minor)

Item 6 -- `useConversationFilters` (minor)

Item 4 -- `UnsavedWarningBanner.svelte`

Item 1 -- `relativeTime` display

Item 2 -- `formatDate` consolidation

Item 3 -- `getInitials` in avatars

New directory `$lib/hooks/`