Minor structural improvements — notification rows, admin list panels, search filters, icons #201

New Issue

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

marcel commented

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

Context

Lower-priority structural improvements identified in the frontend complexity audit. Each is a small improvement to readability, not a critical duplication fix.

Scope

1. Extract `NotificationRow.svelte` + `NotificationFilterPills.svelte`

notifications/+page.svelte (280 lines) has a 57-line notification row in {#each} and a 68-line filter pill radiogroup
Extract both as focused components

2. Generic `AdminListPanel.svelte`

GroupsListPanel.svelte (110 lines), TagsListPanel.svelte (87 lines), UsersListPanel.svelte (150 lines) follow the same pattern: collapsible list, "new" button, active item highlighting, optional search
Create a generic component that accepts items, href builder, optional search, and "new" action

3. Extract `AdvancedFilters.svelte` from `SearchFilterBar.svelte`

SearchFilterBar.svelte (218 lines) — the advanced filter panel (date inputs, sender/receiver typeaheads, tag input) is a distinct visual region from the main search input
Split into main search bar + collapsible advanced filters component

4. SVG icon extraction

Inline SVGs repeated across components (bell, chevrons, document, edit, trash, etc.)
Extract frequently used ones as individual components: IconBell.svelte, IconChevronLeft.svelte, etc.
Tree-shakeable, reduces markup noise

5. Extract `TopBarMobileMenu.svelte` from `DocumentTopBar.svelte`

The mobile kebab menu dropdown (~40 lines) is a self-contained visual region

Acceptance Criteria

Each extraction is one atomic commit
No behavior changes
npm run check passes
npm run build passes

## Context Lower-priority structural improvements identified in the frontend complexity audit. Each is a small improvement to readability, not a critical duplication fix. ## Scope ### 1. Extract `NotificationRow.svelte` + `NotificationFilterPills.svelte` - `notifications/+page.svelte` (280 lines) has a 57-line notification row in `{#each}` and a 68-line filter pill radiogroup - Extract both as focused components ### 2. Generic `AdminListPanel.svelte` - `GroupsListPanel.svelte` (110 lines), `TagsListPanel.svelte` (87 lines), `UsersListPanel.svelte` (150 lines) follow the same pattern: collapsible list, "new" button, active item highlighting, optional search - Create a generic component that accepts items, href builder, optional search, and "new" action ### 3. Extract `AdvancedFilters.svelte` from `SearchFilterBar.svelte` - `SearchFilterBar.svelte` (218 lines) — the advanced filter panel (date inputs, sender/receiver typeaheads, tag input) is a distinct visual region from the main search input - Split into main search bar + collapsible advanced filters component ### 4. SVG icon extraction - Inline SVGs repeated across components (bell, chevrons, document, edit, trash, etc.) - Extract frequently used ones as individual components: `IconBell.svelte`, `IconChevronLeft.svelte`, etc. - Tree-shakeable, reduces markup noise ### 5. Extract `TopBarMobileMenu.svelte` from `DocumentTopBar.svelte` - The mobile kebab menu dropdown (~40 lines) is a self-contained visual region ## Acceptance Criteria - [ ] Each extraction is one atomic commit - [ ] No behavior changes - [ ] `npm run check` passes - [ ] `npm run build` passes

marcel added the refactor label 2026-04-07 10:49:06 +02:00

marcel commented

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

👨‍💻 Felix Brandt -- Senior Fullstack Developer

Good structural cleanup scope. A few things I'd want clarified before implementation:

1. NotificationRow + NotificationFilterPills

The 57-line row and 68-line filter pill region are clear splitting candidates by the visual boundary rule -- each is a nameable, distinct UI region.
Question: Does NotificationRow receive the full notification object or only the fields it renders? Props discipline says: pass exactly what is needed. I'd want to see the prop interface defined up front.
Question: Does NotificationFilterPills own any local state (active filter), or is it a controlled component driven by the parent via a bound prop or callback?

2. Generic AdminListPanel

This is the riskiest item. The three panels (Groups 110 lines, Tags 87 lines, Users 150 lines) "follow the same pattern" -- but KISS beats DRY. The question is whether the abstraction has a clear, stable name and genuinely reduces cognitive load at the call site.
Risk: UsersListPanel at 150 lines is almost twice the size of TagsListPanel at 87 lines. The "optional search" and the 63-line delta suggest these panels may diverge more than they converge. Forcing them into one generic component could create a prop explosion (showSearch?, showNewButton?, customHeader?) that makes the call site harder to read than the original.
Suggestion: Before writing the generic component, list the exact props it would need. If it's more than 5-6, the abstraction is not earning its keep. Consider extracting just the shared collapsible-section chrome as a CollapsibleSection.svelte and keeping the list content in each panel.

3. AdvancedFilters extraction

Clean split -- the advanced filter region is visually distinct from the search bar. This follows the visual boundary rule directly.
Question: Will AdvancedFilters own the expand/collapse state, or does the parent SearchFilterBar control it? The answer affects whether the toggle button lives in the parent or the child.

4. SVG icon extraction

Strongly agree. Inline SVGs repeated across components are markup noise.
Suggestion: Use a consistent naming convention: Icon{Name}.svelte with a single class prop for sizing. No width/height props -- let the consumer set size via Tailwind classes on the wrapper or the class prop.
Question: How many icons are we talking about? If it's more than ~10, consider a $lib/icons/ directory rather than dumping them all in $lib/components/.

5. TopBarMobileMenu

40 lines, self-contained visual region -- textbook extraction candidate. No concerns.

General

The acceptance criteria say "no behavior changes" -- this is pure refactor, so every extraction should be verifiable by npm run check + npm run build passing with zero diff in rendered output. I'd add npm run test to the acceptance criteria if any existing tests reference the components being split.
One atomic commit per extraction is correct. Five commits total (or six if AdminListPanel is two steps).

## 👨‍💻 Felix Brandt -- Senior Fullstack Developer Good structural cleanup scope. A few things I'd want clarified before implementation: ### 1. NotificationRow + NotificationFilterPills - The 57-line row and 68-line filter pill region are clear splitting candidates by the visual boundary rule -- each is a nameable, distinct UI region. - **Question:** Does `NotificationRow` receive the full notification object or only the fields it renders? Props discipline says: pass exactly what is needed. I'd want to see the prop interface defined up front. - **Question:** Does `NotificationFilterPills` own any local state (active filter), or is it a controlled component driven by the parent via a bound prop or callback? ### 2. Generic AdminListPanel - This is the riskiest item. The three panels (Groups 110 lines, Tags 87 lines, Users 150 lines) "follow the same pattern" -- but KISS beats DRY. The question is whether the abstraction has a **clear, stable name** and genuinely reduces cognitive load at the call site. - **Risk:** `UsersListPanel` at 150 lines is almost twice the size of `TagsListPanel` at 87 lines. The "optional search" and the 63-line delta suggest these panels may diverge more than they converge. Forcing them into one generic component could create a prop explosion (`showSearch?`, `showNewButton?`, `customHeader?`) that makes the call site harder to read than the original. - **Suggestion:** Before writing the generic component, list the exact props it would need. If it's more than 5-6, the abstraction is not earning its keep. Consider extracting just the shared collapsible-section chrome as a `CollapsibleSection.svelte` and keeping the list content in each panel. ### 3. AdvancedFilters extraction - Clean split -- the advanced filter region is visually distinct from the search bar. This follows the visual boundary rule directly. - **Question:** Will `AdvancedFilters` own the expand/collapse state, or does the parent `SearchFilterBar` control it? The answer affects whether the toggle button lives in the parent or the child. ### 4. SVG icon extraction - Strongly agree. Inline SVGs repeated across components are markup noise. - **Suggestion:** Use a consistent naming convention: `Icon{Name}.svelte` with a single `class` prop for sizing. No `width`/`height` props -- let the consumer set size via Tailwind classes on the wrapper or the class prop. - **Question:** How many icons are we talking about? If it's more than ~10, consider a `$lib/icons/` directory rather than dumping them all in `$lib/components/`. ### 5. TopBarMobileMenu - 40 lines, self-contained visual region -- textbook extraction candidate. No concerns. ### General - The acceptance criteria say "no behavior changes" -- this is pure refactor, so every extraction should be verifiable by `npm run check` + `npm run build` passing with zero diff in rendered output. I'd add `npm run test` to the acceptance criteria if any existing tests reference the components being split. - One atomic commit per extraction is correct. Five commits total (or six if AdminListPanel is two steps).

marcel commented

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

🏗️ Markus Keller -- Senior Application Architect

Structurally sound refactoring scope. My comments focus on module boundaries and abstraction durability.

AdminListPanel -- the one I'd scrutinize most

Generic UI components that accept "items, href builder, optional search, and new action" tend to accumulate configuration props over time until they become mini-frameworks. I've seen this pattern collapse under its own weight in multiple projects.
Concrete question: Do all three panels share the same data shape? GroupsListPanel works with groups (name + permissions), TagsListPanel with tags (name + color?), UsersListPanel with users (name + email + groups). If the item rendering differs significantly, the generic component will need a slot or render prop for the item row -- at which point you're passing back most of the complexity you extracted.
Suggestion: A lighter-weight approach is to extract the layout shell (collapsible container with header, "new" button slot, and list area) and let each panel own its item rendering. This gives you DRY on the chrome without coupling the content. Something like:

<AdminSection title="Tags" href="/admin/tags/new" newLabel="Neuer Tag">
  {#each tags as tag (tag.id)}
    <TagRow {tag} active={tag.id === activeId} />
  {/each}
</AdminSection>

This keeps the module boundary clean: the shell owns layout, each domain panel owns its items.

SearchFilterBar split

Good separation of concerns. The main search input is a stable, simple component. The advanced filters region is a distinct feature that could evolve independently (new filter types, saved filter presets, etc.).
Question: After splitting, does SearchFilterBar become a thin orchestrator that composes SearchInput + AdvancedFilters? Or does AdvancedFilters become a sibling in the page? The answer determines whether the page layout or the component tree owns the filter visibility toggle.

SVG icons

No architectural concern with extraction. One practical note: ensure icons are pure presentational components with no internal state. They should accept class and nothing else. This keeps them leaf nodes in the component tree with zero runtime overhead.

Ordering of commits

I'd recommend this sequence to minimize merge conflicts:
1. SVG icons first (leaf nodes, no structural changes to parents)
2. TopBarMobileMenu (small, isolated)
3. NotificationRow + NotificationFilterPills (isolated page)
4. AdvancedFilters (touches search, which is a high-traffic area)
5. AdminListPanel last (highest risk, benefits from seeing the pattern in the other extractions)

Missing from scope

Question: Are there any shared utilities being duplicated across these components (e.g., click-outside handlers, keyboard navigation, collapse/expand logic)? If so, extracting those as reusable actions or utilities before the component extractions would prevent duplicating them in the new components.

## 🏗️ Markus Keller -- Senior Application Architect Structurally sound refactoring scope. My comments focus on module boundaries and abstraction durability. ### AdminListPanel -- the one I'd scrutinize most - Generic UI components that accept "items, href builder, optional search, and new action" tend to accumulate configuration props over time until they become mini-frameworks. I've seen this pattern collapse under its own weight in multiple projects. - **Concrete question:** Do all three panels share the same data shape? `GroupsListPanel` works with groups (name + permissions), `TagsListPanel` with tags (name + color?), `UsersListPanel` with users (name + email + groups). If the item rendering differs significantly, the generic component will need a slot or render prop for the item row -- at which point you're passing back most of the complexity you extracted. - **Suggestion:** A lighter-weight approach is to extract the **layout shell** (collapsible container with header, "new" button slot, and list area) and let each panel own its item rendering. This gives you DRY on the chrome without coupling the content. Something like: ```svelte <AdminSection title="Tags" href="/admin/tags/new" newLabel="Neuer Tag"> {#each tags as tag (tag.id)} <TagRow {tag} active={tag.id === activeId} /> {/each} </AdminSection> ``` This keeps the module boundary clean: the shell owns layout, each domain panel owns its items. ### SearchFilterBar split - Good separation of concerns. The main search input is a stable, simple component. The advanced filters region is a distinct feature that could evolve independently (new filter types, saved filter presets, etc.). - **Question:** After splitting, does `SearchFilterBar` become a thin orchestrator that composes `SearchInput` + `AdvancedFilters`? Or does `AdvancedFilters` become a sibling in the page? The answer determines whether the page layout or the component tree owns the filter visibility toggle. ### SVG icons - No architectural concern with extraction. One practical note: ensure icons are pure presentational components with no internal state. They should accept `class` and nothing else. This keeps them leaf nodes in the component tree with zero runtime overhead. ### Ordering of commits - I'd recommend this sequence to minimize merge conflicts: 1. SVG icons first (leaf nodes, no structural changes to parents) 2. TopBarMobileMenu (small, isolated) 3. NotificationRow + NotificationFilterPills (isolated page) 4. AdvancedFilters (touches search, which is a high-traffic area) 5. AdminListPanel last (highest risk, benefits from seeing the pattern in the other extractions) ### Missing from scope - **Question:** Are there any shared utilities being duplicated across these components (e.g., click-outside handlers, keyboard navigation, collapse/expand logic)? If so, extracting those as reusable actions or utilities before the component extractions would prevent duplicating them in the new components.

marcel commented

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

🧪 Sara Holt -- Senior QA Engineer

Pure refactor with "no behavior changes" -- this is exactly the scenario where test coverage proves its value. My focus is on how we verify nothing breaks.

Test coverage before refactoring

Question: What is the current test coverage for the components being split? Specifically:
- Are there Vitest component tests for notifications/+page.svelte?
- Are there tests for SearchFilterBar.svelte?
- Are there tests for any of the three admin list panels?
- Are there E2E tests covering the notification page, search filters, or admin panels?
If coverage is thin, I'd recommend writing characterization tests for the current behavior before extracting. These tests document what the component does today and catch any accidental behavior changes during extraction.

Acceptance criteria gaps

The current acceptance criteria are:
- Each extraction is one atomic commit
- No behavior changes
- npm run check passes
- npm run build passes
Missing: npm run test should be in the acceptance criteria. Type checking and build success do not guarantee behavioral correctness.
Missing: If E2E tests exist for these pages, they should pass after each commit, not just at the end.

Specific test concerns per item

NotificationRow + NotificationFilterPills:

Filter pills involve user interaction (selecting a filter). If there are no tests for filter state changes, we should add them to the extracted NotificationFilterPills component -- this is the ideal time since the component will have a clean, focused API.

AdminListPanel (generic):

If this becomes a generic component, it needs its own test suite verifying: rendering items, active item highlighting, search filtering (when enabled), and the "new" button link.
Each concrete usage (groups, tags, users) also needs a smoke test confirming props are wired correctly.
Risk: Regression in keyboard navigation or focus management if the generic component handles things differently than the original panels.

AdvancedFilters:

The expand/collapse interaction needs a test: click toggle, verify filters are visible/hidden. This is the kind of thing that breaks silently if the state ownership changes during extraction.

SVG icons:

Low risk. No tests needed for pure presentational icon components. A snapshot test would be overkill.

TopBarMobileMenu:

If there's an existing E2E test for the document top bar on mobile viewports, verify it still passes. The kebab menu open/close interaction is a common regression point.

Suggestion

Add npm run test to the acceptance criteria.
For each extraction commit, run the full frontend test suite (unit + E2E) as verification, not just type checking.

## 🧪 Sara Holt -- Senior QA Engineer Pure refactor with "no behavior changes" -- this is exactly the scenario where test coverage proves its value. My focus is on how we verify nothing breaks. ### Test coverage before refactoring - **Question:** What is the current test coverage for the components being split? Specifically: - Are there Vitest component tests for `notifications/+page.svelte`? - Are there tests for `SearchFilterBar.svelte`? - Are there tests for any of the three admin list panels? - Are there E2E tests covering the notification page, search filters, or admin panels? - If coverage is thin, I'd recommend writing **characterization tests** for the current behavior before extracting. These tests document what the component does today and catch any accidental behavior changes during extraction. ### Acceptance criteria gaps - The current acceptance criteria are: - Each extraction is one atomic commit - No behavior changes - `npm run check` passes - `npm run build` passes - **Missing:** `npm run test` should be in the acceptance criteria. Type checking and build success do not guarantee behavioral correctness. - **Missing:** If E2E tests exist for these pages, they should pass after each commit, not just at the end. ### Specific test concerns per item **NotificationRow + NotificationFilterPills:** - Filter pills involve user interaction (selecting a filter). If there are no tests for filter state changes, we should add them to the extracted `NotificationFilterPills` component -- this is the ideal time since the component will have a clean, focused API. **AdminListPanel (generic):** - If this becomes a generic component, it needs its own test suite verifying: rendering items, active item highlighting, search filtering (when enabled), and the "new" button link. - Each concrete usage (groups, tags, users) also needs a smoke test confirming props are wired correctly. - **Risk:** Regression in keyboard navigation or focus management if the generic component handles things differently than the original panels. **AdvancedFilters:** - The expand/collapse interaction needs a test: click toggle, verify filters are visible/hidden. This is the kind of thing that breaks silently if the state ownership changes during extraction. **SVG icons:** - Low risk. No tests needed for pure presentational icon components. A snapshot test would be overkill. **TopBarMobileMenu:** - If there's an existing E2E test for the document top bar on mobile viewports, verify it still passes. The kebab menu open/close interaction is a common regression point. ### Suggestion - Add `npm run test` to the acceptance criteria. - For each extraction commit, run the full frontend test suite (unit + E2E) as verification, not just type checking.

marcel commented

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

🔒 Nora "NullX" Steiner -- Application Security Engineer

This is a pure frontend refactoring issue -- no new endpoints, no auth changes, no data flow changes. My security exposure assessment is low, but there are a few things worth noting.

SVG icon extraction -- the one I care about

Inline SVGs are safe because they're hardcoded in the template. Extracted icon components remain safe as long as they are static markup with no dynamic attributes from user input.
Verify: None of the extracted icon components accept props that get interpolated into SVG attributes (e.g., fill={userInput} or href={dynamicUrl}). SVG href, xlink:href, and use elements can be XSS vectors if they accept untrusted input. Pure class prop for styling is fine.
Suggestion: If any icons use <use href="..."> to reference external SVG sprites, ensure the href is a static path, not user-controlled.

TopBarMobileMenu

Mobile menus sometimes introduce click-outside-to-close handlers that use document.addEventListener. These are fine, but verify the handler is cleaned up on component destroy to prevent memory leaks and stale event listeners.
Question: Does the mobile menu contain any links with dynamic href values derived from URL params or user data? If so, ensure they're still sanitized after extraction (SvelteKit handles this by default for href attributes, but worth confirming).

AdminListPanel (generic)

If the generic component accepts an href builder function (as described: "accepts items, href builder"), ensure the generated hrefs are not constructed from raw user input. Admin panels typically use UUIDs from the database, which is safe. Just confirming this stays the case.

NotificationFilterPills / AdvancedFilters

No security concerns. Filter state is client-side UI state with no injection surface.

General

Since the acceptance criteria state "no behavior changes," the security posture should remain identical. The risk is purely in accidental introduction of new patterns during extraction. A quick grep for innerHTML, {@html}, and dynamic href construction in the new components after extraction would be a sufficient sanity check.

## 🔒 Nora "NullX" Steiner -- Application Security Engineer This is a pure frontend refactoring issue -- no new endpoints, no auth changes, no data flow changes. My security exposure assessment is low, but there are a few things worth noting. ### SVG icon extraction -- the one I care about - Inline SVGs are safe because they're hardcoded in the template. Extracted icon components remain safe as long as they are **static markup with no dynamic attributes from user input**. - **Verify:** None of the extracted icon components accept props that get interpolated into SVG attributes (e.g., `fill={userInput}` or `href={dynamicUrl}`). SVG `href`, `xlink:href`, and `use` elements can be XSS vectors if they accept untrusted input. Pure `class` prop for styling is fine. - **Suggestion:** If any icons use `<use href="...">` to reference external SVG sprites, ensure the href is a static path, not user-controlled. ### TopBarMobileMenu - Mobile menus sometimes introduce click-outside-to-close handlers that use `document.addEventListener`. These are fine, but verify the handler is cleaned up on component destroy to prevent memory leaks and stale event listeners. - **Question:** Does the mobile menu contain any links with dynamic `href` values derived from URL params or user data? If so, ensure they're still sanitized after extraction (SvelteKit handles this by default for `href` attributes, but worth confirming). ### AdminListPanel (generic) - If the generic component accepts an `href` builder function (as described: "accepts items, href builder"), ensure the generated hrefs are not constructed from raw user input. Admin panels typically use UUIDs from the database, which is safe. Just confirming this stays the case. ### NotificationFilterPills / AdvancedFilters - No security concerns. Filter state is client-side UI state with no injection surface. ### General - Since the acceptance criteria state "no behavior changes," the security posture should remain identical. The risk is purely in accidental introduction of new patterns during extraction. A quick grep for `innerHTML`, `{@html}`, and dynamic `href` construction in the new components after extraction would be a sufficient sanity check.

marcel commented

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

🎨 Leonie Voss -- UI/UX Design Lead

Structural refactoring that doesn't change visual output -- I'm supportive. My focus is on preserving design fidelity and accessibility during extraction.

NotificationRow

Critical: The notification row likely has hover, focus, and active states. When extracting, ensure all interaction states are preserved -- especially the :focus-visible outline. It's common for focus styles to get lost when a component boundary moves.
Question: Does the notification row have different visual states (read vs. unread)? If so, the extracted component needs to accept a state prop and apply the correct styling. Verify the contrast ratio of the "read" state (often gray-on-white, often failing WCAG AA).

NotificationFilterPills

Filter pills are interactive elements. Each pill must be a minimum 44x44px touch target.
Verify after extraction: ARIA attributes are preserved. If the pills use role="radiogroup" and role="radio" (as implied by "radiogroup" in the issue), those roles must stay on the correct elements after extraction. Splitting a radiogroup across component boundaries can break the ARIA tree if the role="radiogroup" container ends up in the parent while the role="radio" items are in the child.
Suggestion: Keep the role="radiogroup" wrapper inside NotificationFilterPills.svelte, not in the parent.

AdminListPanel (generic)

The three panels likely have slightly different visual densities. Users panel with email + groups is denser than tags panel with just a name.
Risk: A generic component might homogenize the spacing and typography, making the denser panels feel cramped or the sparser ones feel empty.
Suggestion: If going generic, ensure the item slot allows full control over the row layout. The shell should provide consistent vertical rhythm (gap/padding) but not constrain the item content height.

AdvancedFilters

The collapse/expand animation (if any) must be preserved. A sudden layout shift when toggling filters is jarring, especially on mobile where the content below jumps.
Question: Is the expand/collapse animated with a height transition? If so, ensure the animation stays with the component that owns the collapsible region.

SVG icons

Verify: Extracted icons must preserve aria-hidden="true" (if decorative) or have an appropriate aria-label (if semantic). Inline SVGs often have this set correctly in context but it gets dropped during extraction.
Suggestion: Default to aria-hidden="true" on all icon components. If an icon is the sole content of a button, the button itself should have the aria-label, not the icon.

TopBarMobileMenu

Mobile menu extraction is clean. Ensure the kebab trigger button retains its aria-expanded attribute that reflects menu state, and that aria-controls points to the correct element ID after the menu moves into its own component.

General principle

For every extraction: do a quick a11y check. Run npm run check, yes, but also verify ARIA attributes, focus order, and touch targets in the browser. Type checking doesn't catch accessibility regressions.

## 🎨 Leonie Voss -- UI/UX Design Lead Structural refactoring that doesn't change visual output -- I'm supportive. My focus is on preserving design fidelity and accessibility during extraction. ### NotificationRow - **Critical:** The notification row likely has hover, focus, and active states. When extracting, ensure all interaction states are preserved -- especially the `:focus-visible` outline. It's common for focus styles to get lost when a component boundary moves. - **Question:** Does the notification row have different visual states (read vs. unread)? If so, the extracted component needs to accept a state prop and apply the correct styling. Verify the contrast ratio of the "read" state (often gray-on-white, often failing WCAG AA). ### NotificationFilterPills - Filter pills are interactive elements. Each pill must be a minimum 44x44px touch target. - **Verify after extraction:** ARIA attributes are preserved. If the pills use `role="radiogroup"` and `role="radio"` (as implied by "radiogroup" in the issue), those roles must stay on the correct elements after extraction. Splitting a radiogroup across component boundaries can break the ARIA tree if the `role="radiogroup"` container ends up in the parent while the `role="radio"` items are in the child. - **Suggestion:** Keep the `role="radiogroup"` wrapper inside `NotificationFilterPills.svelte`, not in the parent. ### AdminListPanel (generic) - The three panels likely have slightly different visual densities. Users panel with email + groups is denser than tags panel with just a name. - **Risk:** A generic component might homogenize the spacing and typography, making the denser panels feel cramped or the sparser ones feel empty. - **Suggestion:** If going generic, ensure the item slot allows full control over the row layout. The shell should provide consistent vertical rhythm (gap/padding) but not constrain the item content height. ### AdvancedFilters - The collapse/expand animation (if any) must be preserved. A sudden layout shift when toggling filters is jarring, especially on mobile where the content below jumps. - **Question:** Is the expand/collapse animated with a height transition? If so, ensure the animation stays with the component that owns the collapsible region. ### SVG icons - **Verify:** Extracted icons must preserve `aria-hidden="true"` (if decorative) or have an appropriate `aria-label` (if semantic). Inline SVGs often have this set correctly in context but it gets dropped during extraction. - **Suggestion:** Default to `aria-hidden="true"` on all icon components. If an icon is the sole content of a button, the button itself should have the `aria-label`, not the icon. ### TopBarMobileMenu - Mobile menu extraction is clean. Ensure the kebab trigger button retains its `aria-expanded` attribute that reflects menu state, and that `aria-controls` points to the correct element ID after the menu moves into its own component. ### General principle - For every extraction: do a quick a11y check. Run `npm run check`, yes, but also verify ARIA attributes, focus order, and touch targets in the browser. Type checking doesn't catch accessibility regressions.

marcel commented

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

⚙️ Tobias Wendt -- DevOps & Platform Engineer

Pure frontend refactoring -- no infrastructure changes, no new dependencies, no build pipeline modifications. My exposure is minimal, but a few operational notes:

Build impact

Five to six atomic commits, each touching only frontend Svelte files. The CI pipeline (npm run check, npm run build, npm run test) should handle this without any changes.
Verify: Each commit independently passes npm run build. If a mid-series commit breaks the build (e.g., extracting a component but not updating all import paths), the git history becomes non-bisectable. This matters for git bisect when tracking down future regressions.

SVG icon extraction -- bundle size

Extracting inline SVGs into individual .svelte components is a net neutral or slight improvement for bundle size. Vite/SvelteKit will tree-shake unused icon components, whereas inline SVGs are always included in the parent component's bundle.
Question: Are any of these icons used in server-rendered pages where they'd be part of the SSR HTML payload? If so, extraction doesn't change the payload size -- just the source organization. No concern either way, just noting it.

AdminListPanel -- no infra concern

A generic component vs. three specific ones has no impact on build output or CI. Vite handles both identically.

What I'd watch for

New dependencies: The issue doesn't mention any new npm packages. Confirm that no icon library (like lucide-svelte or heroicons) is being introduced. If icons are extracted as plain .svelte files with inline SVG markup, that's zero new dependencies -- ideal.
File count: Extracting ~8-12 new component files is fine. It won't meaningfully affect build times. SvelteKit handles hundreds of components without issue.

Suggestion

If the team is doing 5+ atomic commits on main for this issue, consider doing it on a short-lived branch with a single merge commit or squash merge. This keeps main bisectable at a coarser granularity while preserving the atomic commit history on the branch. But this is a workflow preference, not a blocker.

## ⚙️ Tobias Wendt -- DevOps & Platform Engineer Pure frontend refactoring -- no infrastructure changes, no new dependencies, no build pipeline modifications. My exposure is minimal, but a few operational notes: ### Build impact - Five to six atomic commits, each touching only frontend Svelte files. The CI pipeline (`npm run check`, `npm run build`, `npm run test`) should handle this without any changes. - **Verify:** Each commit independently passes `npm run build`. If a mid-series commit breaks the build (e.g., extracting a component but not updating all import paths), the git history becomes non-bisectable. This matters for `git bisect` when tracking down future regressions. ### SVG icon extraction -- bundle size - Extracting inline SVGs into individual `.svelte` components is a net neutral or slight improvement for bundle size. Vite/SvelteKit will tree-shake unused icon components, whereas inline SVGs are always included in the parent component's bundle. - **Question:** Are any of these icons used in server-rendered pages where they'd be part of the SSR HTML payload? If so, extraction doesn't change the payload size -- just the source organization. No concern either way, just noting it. ### AdminListPanel -- no infra concern - A generic component vs. three specific ones has no impact on build output or CI. Vite handles both identically. ### What I'd watch for - **New dependencies:** The issue doesn't mention any new npm packages. Confirm that no icon library (like `lucide-svelte` or `heroicons`) is being introduced. If icons are extracted as plain `.svelte` files with inline SVG markup, that's zero new dependencies -- ideal. - **File count:** Extracting ~8-12 new component files is fine. It won't meaningfully affect build times. SvelteKit handles hundreds of components without issue. ### Suggestion - If the team is doing 5+ atomic commits on main for this issue, consider doing it on a short-lived branch with a single merge commit or squash merge. This keeps `main` bisectable at a coarser granularity while preserving the atomic commit history on the branch. But this is a workflow preference, not a blocker.

marcel commented

2026-04-07 11:06:03 +02:00

Comprehensive Response to Review Comments

I read every file referenced in this issue. Here are concrete answers organized by persona.

Felix Brandt

NotificationRow props: The {#each} block at notifications/+page.svelte:207 iterates over NotificationItem objects. The row renders n.actorName, n.type, n.documentTitle, n.createdAt, n.read, n.documentId, n.referenceId, n.annotationId. The extracted NotificationRow should accept the full NotificationItem object -- it uses 8 of its fields, which is effectively all of them. Splitting into individual props would create noise for no benefit. Prop interface:

{ notification: NotificationItem; onnavigate: (n: NotificationItem) => void }

The typeBadgeLabel() helper (line 58) moves into the child component since it's only used there.

NotificationFilterPills state ownership: The filter pills at lines 107-175 are stateless -- they read activeType and activeReadFilter (derived from URL search params) and call setFilter() which does a goto(). The extracted component should be controlled: it receives activeType, activeReadFilter, unreadCount as props and emits a filter change event. The parent owns setFilter().

AdminListPanel -- KISS vs DRY decision: I compared all three panels line by line:

Feature	Groups (110 lines)	Tags (87 lines)	Users (150 lines)
Collapse/expand + localStorage	Yes	Yes	Yes
"New" button	Yes	No	Yes
Search input	No	No	Yes
Item subtitle	permissions count	none	full name + group chips
Active highlight	identical	identical	identical

The collapsed state (lines 32-46 in each file) is character-for-character identical except for the label text. The expanded panel header (lines 48-82 in Groups) differs: Tags has no "New" button, Users adds a search bar.

Verdict: Extract the layout shell only, not a fully generic panel. A CollapsibleAdminPanel.svelte that owns the collapse/expand chrome, localStorage persistence, header with title + optional action slot, and scrollable list area. Each domain panel keeps its item rendering. This avoids the prop explosion Felix warned about. The shell needs ~4 props: title, storageKey, autocollapse, plus a default slot and an optional actions snippet slot. Well under the 5-6 threshold.

Icon count: 71 inline <svg> elements across 39 files, plus 42 De Gruyter icon <img> references across 21 files. The plus icon (M12 4v16m8-8H4) appears in 4 files. The chevron-left (M15.75 19.5L8.25 12l7.5-7.5) appears only in notifications/+page.svelte. Most inline SVGs are unique to their component. I'd extract only the plus icon and the chevron-down (used in details toggles and sort) -- maybe 3-4 icons total, into $lib/icons/. Not worth a dedicated directory for 3-4 files; $lib/icons/ is fine but $lib/components/ would also work at this scale.

Markus Keller

Layout shell vs full generic: Agreed -- the shell-only approach is the right call. See the detailed comparison table above. The item rendering diverges too much (Tags: 1 line of text, Users: username + full name + group chip badges with nested {#each}). A render slot gives each panel full control.

SearchFilterBar split -- orchestrator or sibling: After the split, SearchFilterBar.svelte stays as the orchestrator composing search input + toggle button + AdvancedFilters. The toggle button (line 109) lives in the parent because it's visually part of the main search row. AdvancedFilters receives the bound filter values and onSearch callback. The showAdvanced state stays in the parent (it controls the toggle button's icon rotation at line 117). AdvancedFilters is a pure content component rendered conditionally by the parent.

Commit ordering: Agree with the proposed sequence. SVG icons first (leaf), then TopBarMobileMenu, then NotificationRow/FilterPills, then AdvancedFilters, then CollapsibleAdminPanel last.

Shared utilities: Checked $lib/actions/ and $lib/ -- the only shared action is clickOutside.ts (used in DocumentTopBar.svelte:237). The collapse/expand logic in the admin panels is not extracted as a shared utility; each panel has its own manualCollapse + localStorage $effect. The CollapsibleAdminPanel extraction will naturally deduplicate this. No other duplicated utilities (keyboard nav, collapse logic) found that need pre-extraction.

Sara Holt

Current test coverage for affected components:

notifications/+page.svelte -- zero unit or component tests
SearchFilterBar.svelte -- zero tests
GroupsListPanel.svelte, TagsListPanel.svelte, UsersListPanel.svelte -- zero tests
DocumentTopBar.svelte -- zero tests
E2E: only e2e/password-reset.spec.ts exists. No E2E tests for notifications, search, or admin panels.
The only Vitest component test in the repo is AnnotationLayer.svelte.test.ts.

Coverage is thin. Writing characterization tests before extraction would be ideal but significantly expands the scope. Pragmatic approach: Add npm run test to acceptance criteria (agreed). For the higher-risk extractions (NotificationFilterPills with its radiogroup, CollapsibleAdminPanel with localStorage persistence), write focused component tests for the extracted components as part of the extraction commit. This tests the new interface directly rather than trying to test the monolithic component first.

For the low-risk extractions (SVG icons, TopBarMobileMenu), tests are unnecessary -- these are pure presentational components.

Nora Steiner

SVG icon props safety: Confirmed -- the plan is class as the only prop. No fill, href, or xlink:href props. The inline SVGs in the codebase are all static markup with hardcoded d paths. No <use href="..."> patterns found anywhere. No {@html} usage in any affected component (only in PdfViewer.svelte which is out of scope). The extraction will preserve this -- icons are pure static SVG with a class prop for sizing.

TopBarMobileMenu event listener cleanup: The mobile menu at DocumentTopBar.svelte:233-270 uses use:clickOutside which is a Svelte action. Checked clickOutside.ts (lines 1-15) -- it registers document.addEventListener('click', ...) in the action and removes it in destroy(). Svelte's action lifecycle guarantees destroy() runs when the element is removed from the DOM. No leak. After extraction into TopBarMobileMenu.svelte, the use:clickOutside stays on the wrapper <div>, so cleanup behavior is unchanged.

Dynamic hrefs in mobile menu: The only dynamic hrefs are fileUrl (from server load, not user input) and /documents/${doc.id} (UUID from database). Both are safe.

Admin panel href builder: If we go with the shell-only approach (slot-based), each domain panel constructs its own href strings from database UUIDs. No raw user input in href construction.

Leonie Voss

ARIA radiogroup on filter pills: The current implementation at notifications/+page.svelte:107-175 is correct: role="radiogroup" on the container <div>, role="radio" + aria-checked on each <button>. When extracting NotificationFilterPills.svelte, the role="radiogroup" wrapper must stay inside the child component, not in the parent. The current code structure makes this natural -- the entire <div role="radiogroup"> block (lines 107-175) moves as one unit.

Focus-visible outlines: Each pill button has focus-visible:ring-2 focus-visible:ring-focus-ring focus-visible:ring-offset-2 (lines 114, 129, 152, 163). These will be preserved in the extraction since they're on the individual buttons, not the container.

Touch targets: The pills use px-3 py-2 which gives adequate height. Worth verifying the 44px minimum in browser but the padding looks sufficient.

NotificationRow interaction states: The row at lines 208-263 has hover:bg-accent-bg (line 215), read/unread border styling (line 216-217), and a full aria-label (lines 219-225). The :focus-visible outline comes from the <a> element's default browser styling. No explicit focus-visible:ring-* class on the row link -- this is a gap worth adding during extraction: focus-visible:ring-2 focus-visible:ring-focus-ring on the <a> at line 209.

Read state contrast: The read state uses border-l-transparent (line 217) and the text colors remain text-ink / text-ink-2 / text-ink-3. No reduced opacity or grayed-out text on read items -- contrast should be fine, but worth a spot-check.

aria-hidden on icons: The existing inline SVGs in notifications already have aria-hidden="true" (line 81, line 188). Extracted icon components should default to aria-hidden="true" as Leonie suggests. All icons in the affected files are decorative (adjacent to text labels or inside labeled buttons).

AdvancedFilters animation: The collapse uses transition:slide at SearchFilterBar.svelte:140. This stays with the {#if showAdvanced} block. After extraction, AdvancedFilters.svelte will be rendered inside the {#if} in the parent, so the transition:slide stays on the wrapper inside AdvancedFilters or on a wrapper in the parent. Either works; keeping it on the child's root element is cleaner.

Tobias Wendt

Tree-shaking benefit: Minimal in practice. The inline SVGs are already only included in the components that use them. Extraction into .svelte icon components means Vite can tree-shake unused icons, but since we're only extracting icons that are actually used, the bundle size stays the same. The real benefit is source readability, not bundle size.

SSR payload: Icons are used in both SSR and client-rendered pages. Extraction doesn't change the SSR HTML payload -- the SVG markup is still emitted server-side. Purely organizational.

New dependencies: Zero. All icons are plain .svelte files with inline SVG markup. No lucide-svelte, heroicons, or any icon library.

Branch vs direct-to-main: Per project convention, UI bug fixes go direct-to-main, but this is a refactor issue with 5+ commits. A short-lived branch with a merge (not squash -- preserve atomic commits for bisectability) is the right call. This keeps main stable between commits while preserving the individual extraction history.

Build bisectability: Each commit will be verified with npm run check && npm run build && npm run test before committing. Import paths are the main risk -- the IDE rename refactoring handles this but we'll verify explicitly.

Updated Acceptance Criteria

Based on all feedback, the acceptance criteria should be:

Each extraction is one atomic commit
No behavior changes
npm run check passes per commit
npm run build passes per commit
npm run test passes per commit (added per Sara's feedback)
Add focus-visible:ring-2 focus-visible:ring-focus-ring to NotificationRow link (per Leonie's finding)
All extracted icon components default to aria-hidden="true"
role="radiogroup" stays inside NotificationFilterPills, not in parent
AdminListPanel approach: shell-only (CollapsibleAdminPanel) with slots, not fully generic
Work on a short-lived branch, merge to main when complete
Commit order: icons -> TopBarMobileMenu -> NotificationRow/FilterPills -> AdvancedFilters -> CollapsibleAdminPanel

## Comprehensive Response to Review Comments I read every file referenced in this issue. Here are concrete answers organized by persona. --- ### Felix Brandt **NotificationRow props:** The `{#each}` block at `notifications/+page.svelte:207` iterates over `NotificationItem` objects. The row renders `n.actorName`, `n.type`, `n.documentTitle`, `n.createdAt`, `n.read`, `n.documentId`, `n.referenceId`, `n.annotationId`. The extracted `NotificationRow` should accept the full `NotificationItem` object -- it uses 8 of its fields, which is effectively all of them. Splitting into individual props would create noise for no benefit. Prop interface: ```typescript { notification: NotificationItem; onnavigate: (n: NotificationItem) => void } ``` The `typeBadgeLabel()` helper (line 58) moves into the child component since it's only used there. **NotificationFilterPills state ownership:** The filter pills at lines 107-175 are stateless -- they read `activeType` and `activeReadFilter` (derived from URL search params) and call `setFilter()` which does a `goto()`. The extracted component should be controlled: it receives `activeType`, `activeReadFilter`, `unreadCount` as props and emits a filter change event. The parent owns `setFilter()`. **AdminListPanel -- KISS vs DRY decision:** I compared all three panels line by line: | Feature | Groups (110 lines) | Tags (87 lines) | Users (150 lines) | |---|---|---|---| | Collapse/expand + localStorage | Yes | Yes | Yes | | "New" button | Yes | No | Yes | | Search input | No | No | Yes | | Item subtitle | permissions count | none | full name + group chips | | Active highlight | identical | identical | identical | The collapsed state (lines 32-46 in each file) is character-for-character identical except for the label text. The expanded panel header (lines 48-82 in Groups) differs: Tags has no "New" button, Users adds a search bar. **Verdict: Extract the layout shell only, not a fully generic panel.** A `CollapsibleAdminPanel.svelte` that owns the collapse/expand chrome, localStorage persistence, header with title + optional action slot, and scrollable list area. Each domain panel keeps its item rendering. This avoids the prop explosion Felix warned about. The shell needs ~4 props: `title`, `storageKey`, `autocollapse`, plus a default slot and an optional `actions` snippet slot. Well under the 5-6 threshold. **Icon count:** 71 inline `<svg>` elements across 39 files, plus 42 De Gruyter icon `<img>` references across 21 files. The plus icon (`M12 4v16m8-8H4`) appears in 4 files. The chevron-left (`M15.75 19.5L8.25 12l7.5-7.5`) appears only in `notifications/+page.svelte`. Most inline SVGs are unique to their component. I'd extract only the plus icon and the chevron-down (used in details toggles and sort) -- maybe 3-4 icons total, into `$lib/icons/`. Not worth a dedicated directory for 3-4 files; `$lib/icons/` is fine but `$lib/components/` would also work at this scale. --- ### Markus Keller **Layout shell vs full generic:** Agreed -- the shell-only approach is the right call. See the detailed comparison table above. The item rendering diverges too much (Tags: 1 line of text, Users: username + full name + group chip badges with nested `{#each}`). A render slot gives each panel full control. **SearchFilterBar split -- orchestrator or sibling:** After the split, `SearchFilterBar.svelte` stays as the orchestrator composing search input + toggle button + `AdvancedFilters`. The toggle button (line 109) lives in the parent because it's visually part of the main search row. `AdvancedFilters` receives the bound filter values and `onSearch` callback. The `showAdvanced` state stays in the parent (it controls the toggle button's icon rotation at line 117). `AdvancedFilters` is a pure content component rendered conditionally by the parent. **Commit ordering:** Agree with the proposed sequence. SVG icons first (leaf), then TopBarMobileMenu, then NotificationRow/FilterPills, then AdvancedFilters, then CollapsibleAdminPanel last. **Shared utilities:** Checked `$lib/actions/` and `$lib/` -- the only shared action is `clickOutside.ts` (used in `DocumentTopBar.svelte:237`). The collapse/expand logic in the admin panels is not extracted as a shared utility; each panel has its own `manualCollapse` + localStorage `$effect`. The `CollapsibleAdminPanel` extraction will naturally deduplicate this. No other duplicated utilities (keyboard nav, collapse logic) found that need pre-extraction. --- ### Sara Holt **Current test coverage for affected components:** - `notifications/+page.svelte` -- **zero** unit or component tests - `SearchFilterBar.svelte` -- **zero** tests - `GroupsListPanel.svelte`, `TagsListPanel.svelte`, `UsersListPanel.svelte` -- **zero** tests - `DocumentTopBar.svelte` -- **zero** tests - E2E: only `e2e/password-reset.spec.ts` exists. No E2E tests for notifications, search, or admin panels. - The only Vitest component test in the repo is `AnnotationLayer.svelte.test.ts`. Coverage is thin. Writing characterization tests before extraction would be ideal but significantly expands the scope. **Pragmatic approach:** Add `npm run test` to acceptance criteria (agreed). For the higher-risk extractions (NotificationFilterPills with its radiogroup, CollapsibleAdminPanel with localStorage persistence), write focused component tests for the extracted components *as part of* the extraction commit. This tests the new interface directly rather than trying to test the monolithic component first. For the low-risk extractions (SVG icons, TopBarMobileMenu), tests are unnecessary -- these are pure presentational components. --- ### Nora Steiner **SVG icon props safety:** Confirmed -- the plan is `class` as the only prop. No `fill`, `href`, or `xlink:href` props. The inline SVGs in the codebase are all static markup with hardcoded `d` paths. No `<use href="...">` patterns found anywhere. No `{@html}` usage in any affected component (only in `PdfViewer.svelte` which is out of scope). The extraction will preserve this -- icons are pure static SVG with a `class` prop for sizing. **TopBarMobileMenu event listener cleanup:** The mobile menu at `DocumentTopBar.svelte:233-270` uses `use:clickOutside` which is a Svelte action. Checked `clickOutside.ts` (lines 1-15) -- it registers `document.addEventListener('click', ...)` in the action and removes it in `destroy()`. Svelte's action lifecycle guarantees `destroy()` runs when the element is removed from the DOM. No leak. After extraction into `TopBarMobileMenu.svelte`, the `use:clickOutside` stays on the wrapper `<div>`, so cleanup behavior is unchanged. **Dynamic hrefs in mobile menu:** The only dynamic hrefs are `fileUrl` (from server load, not user input) and `/documents/${doc.id}` (UUID from database). Both are safe. **Admin panel href builder:** If we go with the shell-only approach (slot-based), each domain panel constructs its own `href` strings from database UUIDs. No raw user input in href construction. --- ### Leonie Voss **ARIA radiogroup on filter pills:** The current implementation at `notifications/+page.svelte:107-175` is correct: `role="radiogroup"` on the container `<div>`, `role="radio"` + `aria-checked` on each `<button>`. When extracting `NotificationFilterPills.svelte`, the `role="radiogroup"` wrapper **must** stay inside the child component, not in the parent. The current code structure makes this natural -- the entire `<div role="radiogroup">` block (lines 107-175) moves as one unit. **Focus-visible outlines:** Each pill button has `focus-visible:ring-2 focus-visible:ring-focus-ring focus-visible:ring-offset-2` (lines 114, 129, 152, 163). These will be preserved in the extraction since they're on the individual buttons, not the container. **Touch targets:** The pills use `px-3 py-2` which gives adequate height. Worth verifying the 44px minimum in browser but the padding looks sufficient. **NotificationRow interaction states:** The row at lines 208-263 has `hover:bg-accent-bg` (line 215), read/unread border styling (line 216-217), and a full `aria-label` (lines 219-225). The `:focus-visible` outline comes from the `<a>` element's default browser styling. No explicit `focus-visible:ring-*` class on the row link -- **this is a gap** worth adding during extraction: `focus-visible:ring-2 focus-visible:ring-focus-ring` on the `<a>` at line 209. **Read state contrast:** The read state uses `border-l-transparent` (line 217) and the text colors remain `text-ink` / `text-ink-2` / `text-ink-3`. No reduced opacity or grayed-out text on read items -- contrast should be fine, but worth a spot-check. **aria-hidden on icons:** The existing inline SVGs in notifications already have `aria-hidden="true"` (line 81, line 188). Extracted icon components should default to `aria-hidden="true"` as Leonie suggests. All icons in the affected files are decorative (adjacent to text labels or inside labeled buttons). **AdvancedFilters animation:** The collapse uses `transition:slide` at `SearchFilterBar.svelte:140`. This stays with the `{#if showAdvanced}` block. After extraction, `AdvancedFilters.svelte` will be rendered inside the `{#if}` in the parent, so the `transition:slide` stays on the wrapper inside `AdvancedFilters` or on a wrapper in the parent. Either works; keeping it on the child's root element is cleaner. --- ### Tobias Wendt **Tree-shaking benefit:** Minimal in practice. The inline SVGs are already only included in the components that use them. Extraction into `.svelte` icon components means Vite can tree-shake unused icons, but since we're only extracting icons that are actually used, the bundle size stays the same. The real benefit is source readability, not bundle size. **SSR payload:** Icons are used in both SSR and client-rendered pages. Extraction doesn't change the SSR HTML payload -- the SVG markup is still emitted server-side. Purely organizational. **New dependencies:** Zero. All icons are plain `.svelte` files with inline SVG markup. No `lucide-svelte`, `heroicons`, or any icon library. **Branch vs direct-to-main:** Per project convention, UI bug fixes go direct-to-main, but this is a refactor issue with 5+ commits. A short-lived branch with a merge (not squash -- preserve atomic commits for bisectability) is the right call. This keeps main stable between commits while preserving the individual extraction history. **Build bisectability:** Each commit will be verified with `npm run check && npm run build && npm run test` before committing. Import paths are the main risk -- the IDE rename refactoring handles this but we'll verify explicitly. --- ### Updated Acceptance Criteria Based on all feedback, the acceptance criteria should be: - [ ] Each extraction is one atomic commit - [ ] No behavior changes - [ ] `npm run check` passes per commit - [ ] `npm run build` passes per commit - [ ] `npm run test` passes per commit (added per Sara's feedback) - [ ] Add `focus-visible:ring-2 focus-visible:ring-focus-ring` to NotificationRow link (per Leonie's finding) - [ ] All extracted icon components default to `aria-hidden="true"` - [ ] `role="radiogroup"` stays inside `NotificationFilterPills`, not in parent - [ ] AdminListPanel approach: shell-only (`CollapsibleAdminPanel`) with slots, not fully generic - [ ] Work on a short-lived branch, merge to main when complete - [ ] Commit order: icons -> TopBarMobileMenu -> NotificationRow/FilterPills -> AdvancedFilters -> CollapsibleAdminPanel

marcel closed this issue

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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#201

Minor structural improvements — notification rows, admin list panels, search filters, icons #201

Context

Scope

1. Extract NotificationRow.svelte + NotificationFilterPills.svelte

2. Generic AdminListPanel.svelte

3. Extract AdvancedFilters.svelte from SearchFilterBar.svelte

4. SVG icon extraction

5. Extract TopBarMobileMenu.svelte from DocumentTopBar.svelte

Acceptance Criteria

👨‍💻 Felix Brandt -- Senior Fullstack Developer

1. NotificationRow + NotificationFilterPills

2. Generic AdminListPanel

3. AdvancedFilters extraction

4. SVG icon extraction

5. TopBarMobileMenu

General

🏗️ Markus Keller -- Senior Application Architect

AdminListPanel -- the one I'd scrutinize most

SearchFilterBar split

SVG icons

Ordering of commits

Missing from scope

🧪 Sara Holt -- Senior QA Engineer

Test coverage before refactoring

Acceptance criteria gaps

Specific test concerns per item

Suggestion

🔒 Nora "NullX" Steiner -- Application Security Engineer

SVG icon extraction -- the one I care about

TopBarMobileMenu

AdminListPanel (generic)

NotificationFilterPills / AdvancedFilters

General

🎨 Leonie Voss -- UI/UX Design Lead

NotificationRow

NotificationFilterPills

AdminListPanel (generic)

AdvancedFilters

SVG icons

TopBarMobileMenu

General principle

⚙️ Tobias Wendt -- DevOps & Platform Engineer

Build impact

SVG icon extraction -- bundle size

AdminListPanel -- no infra concern

What I'd watch for

Suggestion

Comprehensive Response to Review Comments

Felix Brandt

Markus Keller

Sara Holt

Nora Steiner

Leonie Voss

Tobias Wendt

Updated Acceptance Criteria

1. Extract `NotificationRow.svelte` + `NotificationFilterPills.svelte`

2. Generic `AdminListPanel.svelte`

3. Extract `AdvancedFilters.svelte` from `SearchFilterBar.svelte`

5. Extract `TopBarMobileMenu.svelte` from `DocumentTopBar.svelte`