Split EntityNav.svelte (516 lines) into section + item subcomponents #197

New Issue

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

marcel commented

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

Context

EntityNav.svelte (516 lines) in routes/admin/ is the largest route-local component. It renders the entire admin sidebar: section headers with count badges, searchable entity lists, active-state highlighting, and mobile drawer behavior.

Proposed Split

1. `EntityNavSection.svelte`

Props: title, count, href, isActive, icon (slot or snippet)
Renders: section header row with count badge and active styling
Eliminates repeated section header markup

2. `EntityNavItem.svelte`

Props: label, href, isActive, subtitle?
Renders: single navigation item row with active highlight
Eliminates repeated item row markup across users/groups/tags lists

3. `EntityNav.svelte` becomes orchestrator

Imports EntityNavSection + EntityNavItem
Holds search state, mobile drawer toggle
Maps data to section/item components

Acceptance Criteria

EntityNav.svelte under ~150 lines
Navigation, search, active highlighting, mobile drawer all work identically
npm run check passes

## Context `EntityNav.svelte` (516 lines) in `routes/admin/` is the largest route-local component. It renders the entire admin sidebar: section headers with count badges, searchable entity lists, active-state highlighting, and mobile drawer behavior. ## Proposed Split ### 1. `EntityNavSection.svelte` - Props: `title`, `count`, `href`, `isActive`, `icon` (slot or snippet) - Renders: section header row with count badge and active styling - Eliminates repeated section header markup ### 2. `EntityNavItem.svelte` - Props: `label`, `href`, `isActive`, `subtitle?` - Renders: single navigation item row with active highlight - Eliminates repeated item row markup across users/groups/tags lists ### 3. `EntityNav.svelte` becomes orchestrator - Imports EntityNavSection + EntityNavItem - Holds search state, mobile drawer toggle - Maps data to section/item components ## Acceptance Criteria - [ ] EntityNav.svelte under ~150 lines - [ ] Navigation, search, active highlighting, mobile drawer all work identically - [ ] `npm run check` passes

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

marcel commented

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

👨‍💻 Felix Brandt -- Senior Fullstack Developer

Good refactor target. 516 lines is well past the splitting threshold. A few things I'd want nailed down before implementation:

Props Design

The issue proposes icon as "slot or snippet" on EntityNavSection. In Svelte 5, snippets are the idiomatic choice over slots. I'd commit to snippets now and skip the ambiguity.
EntityNavItem has subtitle? -- what type? A string, or a snippet for richer content (e.g., email under a user name)? This affects whether the component stays simple or needs a render delegation pattern.

Keyed `{#each}` Blocks

The current 516-line file -- does it key its {#each} loops? If not, this refactor is the right moment to fix that. Every {#each} over users/groups/tags must use (item.id) keys. I'd add this as an explicit acceptance criterion.

KISS vs. DRY Judgment Call

The issue says "eliminates repeated section header markup" and "eliminates repeated item row markup." Before extracting, I'd verify this is genuinely 3+ repetitions of the same structure, not 2 similar-looking things that might diverge. If sections for users, groups, and tags have meaningfully different internal structure, forcing them into one EntityNavSection component could make future changes harder.
The ~150-line target for the orchestrator is reasonable, but I'd measure it by template lines specifically -- a 150-line file with 120 lines of script and 30 lines of template is fine; 30 lines of script and 120 lines of template means the split didn't go deep enough.

Testing Strategy

Are there existing Vitest tests for EntityNav? If not, this refactor should add component tests for EntityNavSection and EntityNavItem in isolation before wiring them together. Writing tests for the current monolith first would also serve as a regression safety net.

`$derived` vs. `$effect`

Search state and filtering logic in the orchestrator -- confirm these use $derived for computed filtered lists, not $state + $effect. The refactor is a good opportunity to audit this.

Concrete Suggestion

Add to acceptance criteria:

All {#each} blocks use identity-based keys (item.id)
Snippets used for icon/custom content, not slots
Filtered lists computed via $derived, not $effect

## 👨‍💻 Felix Brandt -- Senior Fullstack Developer Good refactor target. 516 lines is well past the splitting threshold. A few things I'd want nailed down before implementation: ### Props Design - The issue proposes `icon` as "slot or snippet" on `EntityNavSection`. In Svelte 5, snippets are the idiomatic choice over slots. I'd commit to snippets now and skip the ambiguity. - `EntityNavItem` has `subtitle?` -- what type? A string, or a snippet for richer content (e.g., email under a user name)? This affects whether the component stays simple or needs a render delegation pattern. ### Keyed `{#each}` Blocks - The current 516-line file -- does it key its `{#each}` loops? If not, this refactor is the right moment to fix that. Every `{#each}` over users/groups/tags must use `(item.id)` keys. I'd add this as an explicit acceptance criterion. ### KISS vs. DRY Judgment Call - The issue says "eliminates repeated section header markup" and "eliminates repeated item row markup." Before extracting, I'd verify this is genuinely 3+ repetitions of the same structure, not 2 similar-looking things that might diverge. If sections for users, groups, and tags have meaningfully different internal structure, forcing them into one `EntityNavSection` component could make future changes harder. - The ~150-line target for the orchestrator is reasonable, but I'd measure it by template lines specifically -- a 150-line file with 120 lines of script and 30 lines of template is fine; 30 lines of script and 120 lines of template means the split didn't go deep enough. ### Testing Strategy - Are there existing Vitest tests for `EntityNav`? If not, this refactor should add component tests for `EntityNavSection` and `EntityNavItem` in isolation before wiring them together. Writing tests for the current monolith first would also serve as a regression safety net. ### `$derived` vs. `$effect` - Search state and filtering logic in the orchestrator -- confirm these use `$derived` for computed filtered lists, not `$state` + `$effect`. The refactor is a good opportunity to audit this. ### Concrete Suggestion Add to acceptance criteria: - [ ] All `{#each}` blocks use identity-based keys `(item.id)` - [ ] Snippets used for icon/custom content, not slots - [ ] Filtered lists computed via `$derived`, not `$effect`

marcel commented

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

🏗️ Markus Keller -- Senior Application Architect

Clean decomposition proposal. The three-component split (Section, Item, orchestrator) maps well to the visual hierarchy. Architectural considerations:

Component Boundary Clarity

The issue says EntityNav.svelte "holds search state, mobile drawer toggle." That's two distinct concerns -- data filtering and layout mode. If the mobile drawer logic is non-trivial (media queries, swipe gestures, focus trapping), consider whether the drawer shell itself should be a separate component, or whether the orchestrator simply toggles a CSS class. Keep the orchestrator as a data flow coordinator, not a layout engine.

State Ownership

Where does the "active" state originate? From the URL (SvelteKit $page.url)? From a parent layout? This determines whether isActive is a prop passed down or derived inside each component. If it's URL-based, each EntityNavItem could derive its own active state from $page.url.pathname, which would remove the need for the orchestrator to track and distribute it. That's simpler -- fewer props, less coordination.
The count on EntityNavSection -- is this loaded in the admin layout's +layout.server.ts and passed down, or fetched separately? If it comes from the layout load, the data flow is clean. If each section fetches its own count, that's a pattern to avoid.

Module Boundary

EntityNavSection and EntityNavItem live in routes/admin/ as route-local components, correct? They should not migrate to $lib/components/ unless another route needs them. Route-local is the right default -- it keeps the admin module self-contained.

Naming

EntityNavSection and EntityNavItem are descriptive and domain-appropriate. No objection. They pass the "one-word nameable region" test.

Suggestion

Clarify in the issue whether isActive is computed from URL or passed as a prop. This decision affects the component API surface and should be explicit before implementation starts.

## 🏗️ Markus Keller -- Senior Application Architect Clean decomposition proposal. The three-component split (Section, Item, orchestrator) maps well to the visual hierarchy. Architectural considerations: ### Component Boundary Clarity - The issue says `EntityNav.svelte` "holds search state, mobile drawer toggle." That's two distinct concerns -- data filtering and layout mode. If the mobile drawer logic is non-trivial (media queries, swipe gestures, focus trapping), consider whether the drawer shell itself should be a separate component, or whether the orchestrator simply toggles a CSS class. Keep the orchestrator as a data flow coordinator, not a layout engine. ### State Ownership - Where does the "active" state originate? From the URL (SvelteKit `$page.url`)? From a parent layout? This determines whether `isActive` is a prop passed down or derived inside each component. If it's URL-based, each `EntityNavItem` could derive its own active state from `$page.url.pathname`, which would remove the need for the orchestrator to track and distribute it. That's simpler -- fewer props, less coordination. - The `count` on `EntityNavSection` -- is this loaded in the admin layout's `+layout.server.ts` and passed down, or fetched separately? If it comes from the layout load, the data flow is clean. If each section fetches its own count, that's a pattern to avoid. ### Module Boundary - `EntityNavSection` and `EntityNavItem` live in `routes/admin/` as route-local components, correct? They should not migrate to `$lib/components/` unless another route needs them. Route-local is the right default -- it keeps the admin module self-contained. ### Naming - `EntityNavSection` and `EntityNavItem` are descriptive and domain-appropriate. No objection. They pass the "one-word nameable region" test. ### Suggestion - Clarify in the issue whether `isActive` is computed from URL or passed as a prop. This decision affects the component API surface and should be explicit before implementation starts.

marcel commented

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

🧪 Sara Holt -- Senior QA Engineer

Refactoring a 516-line component into three is exactly the kind of change that needs a test safety net. Here's my test strategy thinking:

Regression Risk

The acceptance criterion says "navigation, search, active highlighting, mobile drawer all work identically." How do we verify "identically"? Without existing tests, we're relying on manual verification, which is risky for a refactor of this scope. I'd want to know:
- Are there existing E2E tests covering admin sidebar navigation?
- Are there Vitest component tests for the current EntityNav?
- If neither exists, writing a baseline E2E test before the refactor gives us a regression gate.

Test Plan for the New Components

Unit layer (Vitest + @testing-library/svelte):

EntityNavSection: renders title, count badge, icon; applies active styling when isActive is true; omits count badge when count is 0 or undefined
EntityNavItem: renders label and href; applies active highlight when isActive is true; renders optional subtitle when provided; omits subtitle element when not provided
Orchestrator: filtered list updates when search input changes; all sections render with correct items

E2E layer (Playwright):

Admin sidebar navigation: click through users, groups, tags sections; verify active state changes
Search filtering: type a query, verify list narrows
Mobile drawer: at 375px viewport, verify drawer opens/closes and focus is managed

Edge Cases to Cover

Empty sections (no users, no groups, no tags) -- does the section still render with count "0", or is it hidden?
Search with no results -- is there an empty state message?
Long entity names -- does text truncate or wrap? This affects both the item component and the section header.

Acceptance Criteria Addition

Vitest component tests exist for EntityNavSection and EntityNavItem
Existing E2E admin navigation tests (if any) still pass
npm run check passes (already listed, good)

## 🧪 Sara Holt -- Senior QA Engineer Refactoring a 516-line component into three is exactly the kind of change that needs a test safety net. Here's my test strategy thinking: ### Regression Risk - The acceptance criterion says "navigation, search, active highlighting, mobile drawer all work identically." How do we verify "identically"? Without existing tests, we're relying on manual verification, which is risky for a refactor of this scope. I'd want to know: - Are there existing E2E tests covering admin sidebar navigation? - Are there Vitest component tests for the current `EntityNav`? - If neither exists, writing a baseline E2E test *before* the refactor gives us a regression gate. ### Test Plan for the New Components **Unit layer (Vitest + @testing-library/svelte):** - `EntityNavSection`: renders title, count badge, icon; applies active styling when `isActive` is true; omits count badge when count is 0 or undefined - `EntityNavItem`: renders label and href; applies active highlight when `isActive` is true; renders optional subtitle when provided; omits subtitle element when not provided - Orchestrator: filtered list updates when search input changes; all sections render with correct items **E2E layer (Playwright):** - Admin sidebar navigation: click through users, groups, tags sections; verify active state changes - Search filtering: type a query, verify list narrows - Mobile drawer: at 375px viewport, verify drawer opens/closes and focus is managed ### Edge Cases to Cover - Empty sections (no users, no groups, no tags) -- does the section still render with count "0", or is it hidden? - Search with no results -- is there an empty state message? - Long entity names -- does text truncate or wrap? This affects both the item component and the section header. ### Acceptance Criteria Addition - [ ] Vitest component tests exist for `EntityNavSection` and `EntityNavItem` - [ ] Existing E2E admin navigation tests (if any) still pass - [ ] `npm run check` passes (already listed, good)

marcel commented

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

🔒 Nora "NullX" Steiner -- Application Security Engineer

Pure frontend refactor with no new endpoints or data flows, so the attack surface doesn't change. Still, a few things worth checking during implementation:

Authorization Display Consistency

The admin sidebar shows entities (users, groups, tags) that require specific permissions (ADMIN_USER, ADMIN_TAG, etc.). When splitting into EntityNavSection and EntityNavItem, confirm that permission-based visibility logic doesn't get lost or duplicated. If the current component conditionally hides sections based on the user's permissions, that logic should live in exactly one place -- the orchestrator -- not be scattered across subcomponents.

`href` Injection

EntityNavItem takes an href prop. In the current implementation, these are presumably hardcoded admin routes. But as a general discipline: if href values ever come from user-controlled data (e.g., a person name used in a URL slug), ensure they're properly encoded. This is low risk here since admin routes are static, but worth a glance during review.

Mobile Drawer Focus Trapping

The issue mentions "mobile drawer behavior." If the drawer is a modal-like overlay on mobile, it needs focus trapping -- tab/shift-tab should cycle within the drawer, not escape to the page behind it. This is both an accessibility and a security concern (clickjacking-adjacent UI confusion). Verify this behavior survives the refactor.

No New Security Concerns

No new API calls, no new data flows, no new user inputs. This is a clean structural refactor. My review at PR time will be lightweight -- just confirming the points above.

Suggestion

Add a note in the issue or PR description confirming that permission-based section visibility is preserved and tested.

## 🔒 Nora "NullX" Steiner -- Application Security Engineer Pure frontend refactor with no new endpoints or data flows, so the attack surface doesn't change. Still, a few things worth checking during implementation: ### Authorization Display Consistency - The admin sidebar shows entities (users, groups, tags) that require specific permissions (`ADMIN_USER`, `ADMIN_TAG`, etc.). When splitting into `EntityNavSection` and `EntityNavItem`, confirm that permission-based visibility logic doesn't get lost or duplicated. If the current component conditionally hides sections based on the user's permissions, that logic should live in exactly one place -- the orchestrator -- not be scattered across subcomponents. ### `href` Injection - `EntityNavItem` takes an `href` prop. In the current implementation, these are presumably hardcoded admin routes. But as a general discipline: if `href` values ever come from user-controlled data (e.g., a person name used in a URL slug), ensure they're properly encoded. This is low risk here since admin routes are static, but worth a glance during review. ### Mobile Drawer Focus Trapping - The issue mentions "mobile drawer behavior." If the drawer is a modal-like overlay on mobile, it needs focus trapping -- tab/shift-tab should cycle within the drawer, not escape to the page behind it. This is both an accessibility and a security concern (clickjacking-adjacent UI confusion). Verify this behavior survives the refactor. ### No New Security Concerns - No new API calls, no new data flows, no new user inputs. This is a clean structural refactor. My review at PR time will be lightweight -- just confirming the points above. ### Suggestion - Add a note in the issue or PR description confirming that permission-based section visibility is preserved and tested.

marcel commented

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

🎨 Leonie Voss -- UI/UX Design Lead

Component splitting is great for maintainability, but the visual contract must survive intact. Here's what I'd verify:

Active State Styling

The current EntityNav handles active-state highlighting for both sections and items. When splitting, confirm:
- Active section header uses the same brand-navy/brand-mint treatment as today
- Active item row has the same background, font-weight, and left-border (or whatever the current indicator is)
- Hover and focus states on items are preserved -- these are often lost in refactors when styles move between files

Touch Targets on Mobile

EntityNavItem rows need to maintain 44x44px minimum touch targets. When extracting the component, verify the clickable area covers the full row width, not just the text label. If the current implementation relies on padding from a parent container, the extracted component might lose that spacing.

Count Badge Consistency

EntityNavSection renders a count badge. Confirm:
- Badge uses the project's established pattern (small, rounded, brand-sand background or similar)
- Badge is not the only cue for "this section has content" -- a screen reader should announce "Users (12)" or equivalent via an aria-label, not rely on visual badge alone

Mobile Drawer

The drawer is a critical interaction on small screens. When extracting:
- Does the drawer overlay use the correct z-index stacking?
- Is there a visible backdrop/scrim?
- Does close-on-outside-click still work?
- Is the close button's touch target at least 44x44px?
- Does focus return to the trigger button when the drawer closes?

Responsive Breakpoint

At what breakpoint does the sidebar collapse into a drawer? This threshold logic should stay in the orchestrator, not leak into subcomponents. The subcomponents should be layout-agnostic -- they render identically whether in a sidebar or a drawer.

Suggestion

Add to acceptance criteria: "Visual regression screenshot comparison at 375px and 1440px before and after the refactor."

## 🎨 Leonie Voss -- UI/UX Design Lead Component splitting is great for maintainability, but the visual contract must survive intact. Here's what I'd verify: ### Active State Styling - The current `EntityNav` handles active-state highlighting for both sections and items. When splitting, confirm: - Active section header uses the same brand-navy/brand-mint treatment as today - Active item row has the same background, font-weight, and left-border (or whatever the current indicator is) - Hover and focus states on items are preserved -- these are often lost in refactors when styles move between files ### Touch Targets on Mobile - `EntityNavItem` rows need to maintain 44x44px minimum touch targets. When extracting the component, verify the clickable area covers the full row width, not just the text label. If the current implementation relies on padding from a parent container, the extracted component might lose that spacing. ### Count Badge Consistency - `EntityNavSection` renders a count badge. Confirm: - Badge uses the project's established pattern (small, rounded, brand-sand background or similar) - Badge is not the only cue for "this section has content" -- a screen reader should announce "Users (12)" or equivalent via an aria-label, not rely on visual badge alone ### Mobile Drawer - The drawer is a critical interaction on small screens. When extracting: - Does the drawer overlay use the correct z-index stacking? - Is there a visible backdrop/scrim? - Does close-on-outside-click still work? - Is the close button's touch target at least 44x44px? - Does focus return to the trigger button when the drawer closes? ### Responsive Breakpoint - At what breakpoint does the sidebar collapse into a drawer? This threshold logic should stay in the orchestrator, not leak into subcomponents. The subcomponents should be layout-agnostic -- they render identically whether in a sidebar or a drawer. ### Suggestion - Add to acceptance criteria: "Visual regression screenshot comparison at 375px and 1440px before and after the refactor."

marcel commented

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

⚙️ Tobias Wendt -- DevOps & Platform Engineer

Pure frontend refactor -- no infrastructure impact. Quick notes from the build/CI perspective:

Build & Bundle Size

Splitting one .svelte file into three should not affect bundle size meaningfully (Svelte compiles components, and the code is the same logic reorganized). But worth confirming npm run build still produces a clean output with no new warnings. The acceptance criteria already include npm run check, which is good.

CI Pipeline Impact

No new dependencies, no new env vars, no docker changes. The existing CI workflow should handle this without modification. The Vitest and Playwright steps will pick up any new test files automatically.

File Location

The issue implies these stay as route-local components in routes/admin/. Confirm they don't accidentally end up in $lib/components/ -- that would change the import graph and could affect code splitting boundaries in the SvelteKit build.

Dev Server Hot Reload

One thing to watch: Vite's HMR with Svelte sometimes has edge cases when a large component is split into multiple files mid-session. If devs experience stale state after the refactor lands, a full page reload clears it. Not a blocker, just awareness.

Suggestion

After the refactor, run npm run build and confirm no new warnings in the build output. A quick du -sh .svelte-kit/output comparison before and after would confirm no unexpected bundle growth.

## ⚙️ Tobias Wendt -- DevOps & Platform Engineer Pure frontend refactor -- no infrastructure impact. Quick notes from the build/CI perspective: ### Build & Bundle Size - Splitting one .svelte file into three should not affect bundle size meaningfully (Svelte compiles components, and the code is the same logic reorganized). But worth confirming `npm run build` still produces a clean output with no new warnings. The acceptance criteria already include `npm run check`, which is good. ### CI Pipeline Impact - No new dependencies, no new env vars, no docker changes. The existing CI workflow should handle this without modification. The Vitest and Playwright steps will pick up any new test files automatically. ### File Location - The issue implies these stay as route-local components in `routes/admin/`. Confirm they don't accidentally end up in `$lib/components/` -- that would change the import graph and could affect code splitting boundaries in the SvelteKit build. ### Dev Server Hot Reload - One thing to watch: Vite's HMR with Svelte sometimes has edge cases when a large component is split into multiple files mid-session. If devs experience stale state after the refactor lands, a full page reload clears it. Not a blocker, just awareness. ### Suggestion - After the refactor, run `npm run build` and confirm no new warnings in the build output. A quick `du -sh .svelte-kit/output` comparison before and after would confirm no unexpected bundle growth.

marcel commented

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

Code Review Response

I read the full 516-line EntityNav.svelte and the surrounding files (+layout.svelte, +layout.server.ts, entity-nav.svelte.spec.ts, admin.spec.ts). Here are concrete findings addressing every concern raised.

Felix Brandt - Senior Fullstack Developer

Snippets vs. slots: Agreed. Svelte 5 snippets are the right choice. The current component uses neither slots nor snippets -- each section's SVG icon is hardcoded inline. The extracted EntityNavSection should accept an icon snippet prop.

subtitle? type on EntityNavItem: This concern is based on a misunderstanding of the component's scope. EntityNav is a section-level navigation bar (4 sections: users, groups, tags, system), not an entity list with individual item rows. There are no user/group/tag list items rendered here. The issue description's mention of "searchable entity lists" and EntityNavItem for "item row markup across users/groups/tags lists" is misleading -- this component has zero {#each} blocks and no search functionality. The proposed EntityNavItem subcomponent does not map to anything in the current code. I recommend dropping EntityNavItem from the plan and focusing on EntityNavSection only.

Keyed {#each} blocks: Not applicable. The component contains no {#each} loops. All four sections are statically defined with {#if} permission guards. No keying concern exists.

KISS vs. DRY -- repetition count: The structure repeats 3 times per section (tablet trigger button, desktop link, flyout link) across 4 sections = 12 near-identical blocks. Each block contains: the same SVG icon, the same isActive() class toggling, and the same text styling. That is genuine 3x repetition of identical structure per section -- well past the DRY threshold. An EntityNavSection that renders all three variants (tablet button, desktop link, flyout link) from a single declaration would eliminate ~350 lines of duplication.

$derived vs. $effect: Already correct. currentPath at line 25 uses $derived(page.url.pathname). isActive at line 26 is a plain function over that derived value. No $effect is used for computed state. No audit needed.

Markus Keller - Senior Application Architect

State ownership for isActive: isActive is derived from the URL via $derived on page.url.pathname (line 25-26). It is a function (section: string) => boolean that checks currentPath.startsWith('/admin/${section}'). This is self-contained within EntityNav -- not passed from a parent, not a prop. During the split, this function should stay in the orchestrator and be passed as a prop (or each subcomponent can import page from $app/state and derive its own). I'd keep it in the orchestrator to avoid each subcomponent importing $app/state independently -- single source of truth.

Count data origin: Counts come from +layout.server.ts (lines 48-50), which fetches full entity lists and takes .length. They flow through +layout.svelte as props to EntityNav. Clean top-down data flow, no per-section fetching.

Drawer as separate component: The flyout (lines 340-515) is a full navigation panel overlay, not a context-specific drawer. It duplicates the entire nav structure. The logic is modest: flyoutOpen state toggle, backdrop click, Escape key, focus management (lines 28-51). This is ~30 lines of script + a template block. Extracting it into a separate EntityNavFlyout.svelte would be reasonable but not required -- the main win comes from extracting the repeated section markup that the flyout also consumes. If each section is an EntityNavSection, the flyout template shrinks automatically.

Module boundary: Confirmed route-local in routes/admin/. Should stay there.

Sara Holt - Senior QA Engineer

Existing tests:

Vitest: entity-nav.svelte.spec.ts exists with 7 tests covering flyout behavior: initial hidden state, open on click, aria-modal, aria-label, link presence, Escape close, backdrop close, link click close. No tests for permission-based visibility, active state styling, or count rendering.
E2E (Playwright): admin.spec.ts covers admin navigation including clicking "Benutzer", "Gruppen", "Schlagworte" buttons (line 28-31) and full CRUD lifecycles. permissions.spec.ts also touches admin nav.

Gaps to fill before refactoring:

Add Vitest cases for: section hidden when canManageUsers=false, count badge renders correct number, active state applies aria-current="page", system section separated by spacer
The existing E2E suite is a solid regression gate -- it exercises real navigation through the admin sections

Edge cases: Empty sections (count=0) currently still render the section with "0" displayed. This is correct behavior and should be preserved. No "empty state" or "hidden when zero" logic exists.

Nora Steiner - Application Security Engineer

Permission-based visibility: Permission checks are cleanly implemented via {#if} guards at the template level:

canManageUsers guards users section (line 71, 362)
canManagePermissions guards groups section (line 138, 400)
canManageTags guards tags section (line 205, 438)
canRunMaintenance guards system section (line 276, 477)

These boolean props originate from +layout.server.ts lines 52-55, which checks hasPerm() against the authenticated user's group permissions. The permission logic lives in exactly one place (the layout loader). The component just receives booleans. During the split, these {#if} guards should stay in the orchestrator wrapping each <EntityNavSection> -- not move into the subcomponent.

href injection: All hrefs are hardcoded static strings: /admin/users, /admin/groups, /admin/tags, /admin/system (e.g., lines 103, 170, 239, 307). No user-controlled data flows into any href. No encoding concern.

Focus trapping in flyout: The current flyout (lines 340-514) does not implement focus trapping. It sets initial focus (line 38-39: firstLink?.focus()) and returns focus on close (line 44: flyoutTriggerElement?.focus()), but tab/shift-tab can escape the dialog into the page behind it. This is a pre-existing gap, not introduced by the refactor. Worth noting as a follow-up issue but out of scope for this split.

Leonie Voss - UI/UX Design Lead

Active state styling: Consistent across all three rendering contexts. Active items get:

Left border: border-brand-mint (3px)
Background: bg-white/10
Icon color: text-brand-mint (vs text-white/40)
Text: text-white for labels, text-white/65 for counts (vs text-white/55 and text-white/50)
aria-current="page" on desktop and flyout links (lines 109, 176, 245, etc.)

The tablet trigger buttons do not set aria-current (they're buttons, not links), which is correct.

Touch targets: All tablet trigger buttons have min-h-[44px] explicitly set (lines 79, 146, 213, 284). Full-width via w-full. Touch target requirement is met.

Count badge accessibility: Counts are rendered as plain <span> elements inside the link/button. They are not visually isolated badges -- they're inline text within the clickable element. A screen reader reading the button gets: icon (hidden via aria-hidden) + count text + label text. For example, the desktop users link reads as "5 Benutzer" which is clear. No separate aria-label needed since the count and label are both text children of the same <a>.

Mobile drawer specifics:

Z-index: backdrop z-40, panel z-50 (lines 345, 354) -- correct stacking
Backdrop: bg-black/40 with click-to-close (line 346)
Close button: no explicit close button exists; close is via backdrop click, Escape, or link click. The flyout trigger buttons themselves are 44px touch targets.
Focus return: yes, flyoutTriggerElement?.focus() on close (line 44)

Responsive breakpoint: The sidebar is hidden below md, icon-only strip at md (md:w-12), full labels at lg (lg:w-30). This is controlled by the parent +layout.svelte line 17 (hidden md:flex) and the nav element itself (line 61). Breakpoint logic will stay in the orchestrator.

Tobias Wendt - DevOps & Platform Engineer

CI impact: None. No new dependencies, no docker changes, no env vars. Existing npm run check, npm run test, and Playwright steps will cover the refactored files automatically.

File location: Confirmed route-local in routes/admin/. Will not move to $lib/components/.

Bundle size: Splitting a single Svelte component into subcomponents produces identical compiled output (same DOM operations, just organized differently). npm run build should show no meaningful change.

HMR edge case: Acknowledged. Splitting a large file mid-session can cause stale HMR state in Vite. A full page reload resolves it. Not a blocker.

Summary of Revised Plan

Based on the code analysis, the issue description needs correction:

Drop EntityNavItem -- there are no item-level rows or {#each} loops in this component. The "entity lists" are rendered in separate route pages, not here.
Extract EntityNavSection -- this is the real win. Each section currently renders 3 variants (tablet button, desktop link, flyout link) with identical structure. Props: section (string key), href, count?, label (message function), icon (snippet), isActive (boolean), onFlyoutOpen, onFlyoutClose, plus optional styling overrides for the system section's border-top.
Orchestrator keeps: permission {#if} guards, flyoutOpen state, isActive() derivation, flyout backdrop/dialog wrapper, Escape handler.
Add Vitest tests for permission-based visibility and count rendering before refactoring.
Follow-up issue for focus trapping in the flyout (pre-existing gap).

## Code Review Response I read the full 516-line `EntityNav.svelte` and the surrounding files (`+layout.svelte`, `+layout.server.ts`, `entity-nav.svelte.spec.ts`, `admin.spec.ts`). Here are concrete findings addressing every concern raised. --- ### Felix Brandt - Senior Fullstack Developer **Snippets vs. slots:** Agreed. Svelte 5 snippets are the right choice. The current component uses neither slots nor snippets -- each section's SVG icon is hardcoded inline. The extracted `EntityNavSection` should accept an `icon` snippet prop. **`subtitle?` type on EntityNavItem:** This concern is based on a misunderstanding of the component's scope. `EntityNav` is a **section-level navigation bar** (4 sections: users, groups, tags, system), not an entity list with individual item rows. There are no user/group/tag list items rendered here. The issue description's mention of "searchable entity lists" and `EntityNavItem` for "item row markup across users/groups/tags lists" is misleading -- this component has **zero `{#each}` blocks** and **no search functionality**. The proposed `EntityNavItem` subcomponent does not map to anything in the current code. I recommend dropping `EntityNavItem` from the plan and focusing on `EntityNavSection` only. **Keyed `{#each}` blocks:** Not applicable. The component contains no `{#each}` loops. All four sections are statically defined with `{#if}` permission guards. No keying concern exists. **KISS vs. DRY -- repetition count:** The structure repeats **3 times per section** (tablet trigger button, desktop link, flyout link) across **4 sections** = 12 near-identical blocks. Each block contains: the same SVG icon, the same `isActive()` class toggling, and the same text styling. That is genuine 3x repetition of identical structure per section -- well past the DRY threshold. An `EntityNavSection` that renders all three variants (tablet button, desktop link, flyout link) from a single declaration would eliminate ~350 lines of duplication. **`$derived` vs. `$effect`:** Already correct. `currentPath` at line 25 uses `$derived(page.url.pathname)`. `isActive` at line 26 is a plain function over that derived value. No `$effect` is used for computed state. No audit needed. --- ### Markus Keller - Senior Application Architect **State ownership for `isActive`:** `isActive` is derived from the URL via `$derived` on `page.url.pathname` (line 25-26). It is a function `(section: string) => boolean` that checks `currentPath.startsWith('/admin/${section}')`. This is self-contained within `EntityNav` -- not passed from a parent, not a prop. During the split, this function should stay in the orchestrator and be passed as a prop (or each subcomponent can import `page` from `$app/state` and derive its own). I'd keep it in the orchestrator to avoid each subcomponent importing `$app/state` independently -- single source of truth. **Count data origin:** Counts come from `+layout.server.ts` (lines 48-50), which fetches full entity lists and takes `.length`. They flow through `+layout.svelte` as props to `EntityNav`. Clean top-down data flow, no per-section fetching. **Drawer as separate component:** The flyout (lines 340-515) is a full navigation panel overlay, not a context-specific drawer. It duplicates the entire nav structure. The logic is modest: `flyoutOpen` state toggle, backdrop click, Escape key, focus management (lines 28-51). This is ~30 lines of script + a template block. Extracting it into a separate `EntityNavFlyout.svelte` would be reasonable but not required -- the main win comes from extracting the repeated section markup that the flyout also consumes. If each section is an `EntityNavSection`, the flyout template shrinks automatically. **Module boundary:** Confirmed route-local in `routes/admin/`. Should stay there. --- ### Sara Holt - Senior QA Engineer **Existing tests:** - **Vitest:** `entity-nav.svelte.spec.ts` exists with 7 tests covering flyout behavior: initial hidden state, open on click, `aria-modal`, `aria-label`, link presence, Escape close, backdrop close, link click close. No tests for permission-based visibility, active state styling, or count rendering. - **E2E (Playwright):** `admin.spec.ts` covers admin navigation including clicking "Benutzer", "Gruppen", "Schlagworte" buttons (line 28-31) and full CRUD lifecycles. `permissions.spec.ts` also touches admin nav. **Gaps to fill before refactoring:** - Add Vitest cases for: section hidden when `canManageUsers=false`, count badge renders correct number, active state applies `aria-current="page"`, system section separated by spacer - The existing E2E suite is a solid regression gate -- it exercises real navigation through the admin sections **Edge cases:** Empty sections (count=0) currently still render the section with "0" displayed. This is correct behavior and should be preserved. No "empty state" or "hidden when zero" logic exists. --- ### Nora Steiner - Application Security Engineer **Permission-based visibility:** Permission checks are cleanly implemented via `{#if}` guards at the template level: - `canManageUsers` guards users section (line 71, 362) - `canManagePermissions` guards groups section (line 138, 400) - `canManageTags` guards tags section (line 205, 438) - `canRunMaintenance` guards system section (line 276, 477) These boolean props originate from `+layout.server.ts` lines 52-55, which checks `hasPerm()` against the authenticated user's group permissions. The permission logic lives in exactly one place (the layout loader). The component just receives booleans. During the split, these `{#if}` guards should stay in the orchestrator wrapping each `<EntityNavSection>` -- not move into the subcomponent. **`href` injection:** All hrefs are hardcoded static strings: `/admin/users`, `/admin/groups`, `/admin/tags`, `/admin/system` (e.g., lines 103, 170, 239, 307). No user-controlled data flows into any href. No encoding concern. **Focus trapping in flyout:** The current flyout (lines 340-514) does **not** implement focus trapping. It sets initial focus (line 38-39: `firstLink?.focus()`) and returns focus on close (line 44: `flyoutTriggerElement?.focus()`), but tab/shift-tab can escape the dialog into the page behind it. This is a pre-existing gap, not introduced by the refactor. Worth noting as a follow-up issue but out of scope for this split. --- ### Leonie Voss - UI/UX Design Lead **Active state styling:** Consistent across all three rendering contexts. Active items get: - Left border: `border-brand-mint` (3px) - Background: `bg-white/10` - Icon color: `text-brand-mint` (vs `text-white/40`) - Text: `text-white` for labels, `text-white/65` for counts (vs `text-white/55` and `text-white/50`) - `aria-current="page"` on desktop and flyout links (lines 109, 176, 245, etc.) The tablet trigger buttons do not set `aria-current` (they're buttons, not links), which is correct. **Touch targets:** All tablet trigger buttons have `min-h-[44px]` explicitly set (lines 79, 146, 213, 284). Full-width via `w-full`. Touch target requirement is met. **Count badge accessibility:** Counts are rendered as plain `<span>` elements inside the link/button. They are not visually isolated badges -- they're inline text within the clickable element. A screen reader reading the button gets: icon (hidden via `aria-hidden`) + count text + label text. For example, the desktop users link reads as "5 Benutzer" which is clear. No separate `aria-label` needed since the count and label are both text children of the same `<a>`. **Mobile drawer specifics:** - Z-index: backdrop `z-40`, panel `z-50` (lines 345, 354) -- correct stacking - Backdrop: `bg-black/40` with click-to-close (line 346) - Close button: no explicit close button exists; close is via backdrop click, Escape, or link click. The flyout trigger buttons themselves are 44px touch targets. - Focus return: yes, `flyoutTriggerElement?.focus()` on close (line 44) **Responsive breakpoint:** The sidebar is `hidden` below `md`, icon-only strip at `md` (`md:w-12`), full labels at `lg` (`lg:w-30`). This is controlled by the parent `+layout.svelte` line 17 (`hidden md:flex`) and the nav element itself (line 61). Breakpoint logic will stay in the orchestrator. --- ### Tobias Wendt - DevOps & Platform Engineer **CI impact:** None. No new dependencies, no docker changes, no env vars. Existing `npm run check`, `npm run test`, and Playwright steps will cover the refactored files automatically. **File location:** Confirmed route-local in `routes/admin/`. Will not move to `$lib/components/`. **Bundle size:** Splitting a single Svelte component into subcomponents produces identical compiled output (same DOM operations, just organized differently). `npm run build` should show no meaningful change. **HMR edge case:** Acknowledged. Splitting a large file mid-session can cause stale HMR state in Vite. A full page reload resolves it. Not a blocker. --- ### Summary of Revised Plan Based on the code analysis, the issue description needs correction: 1. **Drop `EntityNavItem`** -- there are no item-level rows or `{#each}` loops in this component. The "entity lists" are rendered in separate route pages, not here. 2. **Extract `EntityNavSection`** -- this is the real win. Each section currently renders 3 variants (tablet button, desktop link, flyout link) with identical structure. Props: `section` (string key), `href`, `count?`, `label` (message function), `icon` (snippet), `isActive` (boolean), `onFlyoutOpen`, `onFlyoutClose`, plus optional styling overrides for the system section's border-top. 3. **Orchestrator** keeps: permission `{#if}` guards, `flyoutOpen` state, `isActive()` derivation, flyout backdrop/dialog wrapper, Escape handler. 4. **Add Vitest tests** for permission-based visibility and count rendering before refactoring. 5. **Follow-up issue** for focus trapping in the flyout (pre-existing gap).

marcel referenced this issue from a commit

2026-04-15 14:58:02 +02:00

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

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 closed this issue

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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#197

Split EntityNav.svelte (516 lines) into section + item subcomponents #197

Context

Proposed Split

1. EntityNavSection.svelte

2. EntityNavItem.svelte

3. EntityNav.svelte becomes orchestrator

Acceptance Criteria

👨‍💻 Felix Brandt -- Senior Fullstack Developer

Props Design

Keyed {#each} Blocks

KISS vs. DRY Judgment Call

Testing Strategy

$derived vs. $effect

Concrete Suggestion

🏗️ Markus Keller -- Senior Application Architect

Component Boundary Clarity

State Ownership

Module Boundary

Naming

Suggestion

🧪 Sara Holt -- Senior QA Engineer

Regression Risk

Test Plan for the New Components

Edge Cases to Cover

Acceptance Criteria Addition

🔒 Nora "NullX" Steiner -- Application Security Engineer

Authorization Display Consistency

href Injection

Mobile Drawer Focus Trapping

No New Security Concerns

Suggestion

🎨 Leonie Voss -- UI/UX Design Lead

Active State Styling

Touch Targets on Mobile

Count Badge Consistency

Mobile Drawer

Responsive Breakpoint

Suggestion

⚙️ Tobias Wendt -- DevOps & Platform Engineer

Build & Bundle Size

CI Pipeline Impact

File Location

Dev Server Hot Reload

Suggestion

Code Review Response

Felix Brandt - Senior Fullstack Developer

Markus Keller - Senior Application Architect

Sara Holt - Senior QA Engineer

Nora Steiner - Application Security Engineer

Leonie Voss - UI/UX Design Lead

Tobias Wendt - DevOps & Platform Engineer

Summary of Revised Plan

Split `EntityNav.svelte` (516 lines) into section + item subcomponents #197

1. `EntityNavSection.svelte`

2. `EntityNavItem.svelte`

3. `EntityNav.svelte` becomes orchestrator

Keyed `{#each}` Blocks

`$derived` vs. `$effect`

`href` Injection