feat: Admin section redesign (Concept C) + Persons redesign (Concept A) #160
Reference in New Issue
Block a user
Delete Branch "feat/persons-redesign-concept-a"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Closes #156 · Closes #157
Admin section redesign — Concept C (Master-Detail Command Center)
Replaces the tab-based
/adminlayout with a three-panel master-detail shell across all four entity types: Users, Groups, Tags, and System.Layout & navigation
/admin/users,/admin/groups,/admin/tags,/admin/system) replace the old single-page tab modelResponsive
role="dialog",aria-modal, focus-accessible)localStorage+NewroutesPer-entity detail panels
Mass import card (new)
POST /api/admin/trigger-importGET /api/admin/import-statusevery 2 s while RUNNING, stops on DONE/FAILEDUX hardening
beforeNavigate) on all detail panel forms — inline amber warning, no modalNew permissions in Groups
READ_ALL("Nur lesen") andANNOTATE_ALL("Lesen & Annotieren") to the Standard permissions section, ordered by access levelPersons redesign — Concept A (Enriched Directory)
Backend
@RequirePermission(WRITE_ALL)on all write endpoints@Sizeconstraints onPersonUpdateDTO+@Validon controller> 0) inPersonServiceDomainExceptionreplacesResponseStatusExceptionthroughoutPOST /api/personsnow accepts fullPersonUpdateDTO(birthYear, deathYear, notes)PersonSummaryDTOwith native-query document count (N+1 fix)GET /api/statsendpoint (StatsController)Frontend
/personslist: stats bar, life date range, document count chip, empty state/persons/new: birthYear, deathYear, notes fields/persons/[id]: read-only 2-column layout, Edit button →/persons/[id]/edit/persons/[id]/edit: full edit form, sticky save bar, DangerZone accordion with merge panelTest plan
npm run testpasses (frontend — 399/404; 5 pre-existing failures in DashboardMentions and Persons page unrelated to this PR)./mvnw testpasses (backend)🤖 Generated with Claude Code
Code Coverage
Backend (JaCoCo)
All 582 backend tests pass. ✅
Frontend (Vitest —
src/lib/utils/**+src/lib/server/**)399 / 404 frontend tests pass. The 5 failures are pre-existing (DashboardMentions × 4, Persons page × 1) and unrelated to this PR. ✅
👨💻 Felix Brandt — Senior Fullstack Developer
⚠️ Approved with concerns
Overall this is a well-structured PR. The architecture migration from a tab-based monolith to a proper three-panel master-detail shell is the right move and the route decomposition follows the visual boundary rule cleanly. That said, I have several findings worth addressing.
TDD Evidence
The tests are present and well-named — good. The
beforeNavigatecallback is registered in components but mocked out in tests (vi.mock('$app/navigation', ...)), which means the unsaved-changes guard is never actually exercised in the test suite. The guard logic (cancel navigation, show warning, store target URL) is the most important behaviour added here and it only has a mock — not a behaviour assertion.Suggestion: Add at least one test that calls the
beforeNavigatecallback directly and assertsshowUnsavedWarningbecomes true.Svelte 5 Rule Violations
$effectused to compute a derived value — this pattern appears in all three list panels and violates the rule:frontend/src/routes/admin/users/UsersListPanel.sveltefrontend/src/routes/admin/groups/GroupsListPanel.sveltefrontend/src/routes/admin/tags/TagsListPanel.svelteautocollapseis a prop. The response to it changing is deterministic. Use$derived:This also removes the need to persist
isCollapsedseparately via a second$effecttargetinglocalStorage, since the real source of truth is now clear.$effectused to reset dirty state — ingroups/[id]/+page.svelteandtags/[id]/+page.svelte:isDirtyis being reset as a side-effect offormchanging. This is a command masquerading as a derived value. It should be handled in theuse:enhancecallback on the form — that's the right place to reset UI state after a successful submission.localStoragekey collisionAll three list panels (
UsersListPanel,GroupsListPanel,TagsListPanel) read and write the samelocalStoragekey:Collapsing Users will also collapse Groups the next time it renders. This is a bug. Use distinct keys:
'admin_users_list_collapsed','admin_groups_list_collapsed','admin_tags_list_collapsed'.This is a blocker.
Hardcoded German strings in components
frontend/src/routes/admin/groups/[id]/+page.svelteandgroups/new/+page.sveltecontain permission labels hardcoded in German:These bypass Paraglide entirely. Add i18n keys or at minimum document this as intentional.
Suggestion: Add keys
admin_perm_read_all,admin_perm_annotate_all, etc. to the message files.groups/new/+page.svelte— missingREAD_ALLandANNOTATE_ALLThe edit form at
groups/[id]/+page.sveltehas all three standard permissions (READ_ALL,ANNOTATE_ALL,WRITE_ALL), but the create form omitsREAD_ALLandANNOTATE_ALL. A user creating a read-only group cannot do so from this form. This is inconsistent with the edit form.This is a blocker.
onMountfor viewport redirect in+page.svelteThis causes a flash on desktop/tablet: the mobile picker renders, then immediately redirects. The comment acknowledges this but it's worth noting that SSR cannot detect viewport width. A CSS-only approach (the
hidden md:flexpattern already used in+layout.svelte) would handle this without the flash and without JavaScript.Suggestion: Remove the
onMountredirect. Instead, rely on the layout'shidden md:flexwrapper — if the nav is already visible, the/adminpage is just a mobile entry point and doesn't need a redirect.Minor clean code items
frontend/src/routes/admin/+layout.server.ts: the localtype UserGroupduplicates what the generated API types already define. Use the generated type.admin/groups/[id]/+page.svelte:discardTargetstate is typed as$state<string | null>(null)— the generic syntax for$stateis$statewith inference. The<string | null>annotation works but is unnecessary since the initial value ofnulland the assignmentto?.url.href ?? nullalready infer correctly.🏛️ Markus Keller — Senior Application Architect
✅ Approved
This PR makes a structurally correct decision and executes it cleanly. Moving from a client-side tab switcher to proper SvelteKit routes gives you deep-linkability, per-entity load granularity, and SSR for free. That is the right direction. I have observations and one concern to flag.
Architecture: the load cascade is correct but has a cost
The admin layout at
+layout.server.tsfires three parallel API calls (/api/users,/api/groups,/api/tags) on every admin page navigation — including when you are only on the Tags section and don't need users at all. This is done to populate the entity counts in theEntityNav.This is acceptable for a small family archive where all three lists are tiny. I would flag it as a technical debt item for if these lists ever grow: the counts should become dedicated
/api/users/countendpoints rather than fetching full entity lists to count them.The sub-layouts (
users/+layout.server.ts,groups/+layout.server.ts,tags/+layout.server.ts) then each fetch their full list again for the list panel. On a/admin/users/[id]navigation that means: root layout fetches all users + groups + tags, then users sub-layout fetches all users again. That is two full user list fetches per user detail page load.Suggestion (not a blocker): Use SvelteKit's
parent()call in the users sub-layout to reuse the count data, and pass the full lists down from the root layout rather than re-fetching. Or usedepends()/invalidate()for targeted invalidation after mutations.The permission check in the frontend duplicates backend enforcement
frontend/src/routes/admin/+layout.server.tsreimplements permission checking:This is the right thing to do for UX (fail fast, show 403 before hitting the API). The backend also enforces permissions via
@RequirePermission. This is correct defense-in-depth — not duplication. Good.Module boundary:
UserGrouptype defined locally in layout serverThis is a stripped-down version of the full
UserGroupentity from the generated API types. If the generated types have aUserGroupwith apermissionsfield, use that directly to stay in sync when the backend schema changes.Route structure is clean and follows the pattern
The four-level route tree (
admin/→users/→[id]/+new/) with co-located layout load functions is exactly right for this scale. Each level owns its data. I would not change this.EntityNav.svelteis 492 lines — acknowledge itThis is the one component that exceeds the project's splitting target significantly. It is 492 lines because it renders the icon strip, the desktop labels, and the flyout panel — three visual regions — in one file. It works and it is all navigation-domain logic, but the size should be acknowledged.
Suggestion: If
EntityNavgrows further (e.g. user avatar, notification badges), split it intoEntityNavStrip.svelte+EntityNavFlyout.svelte. For now it is acceptable.The
onMount→gotopattern on the admin index pageThe
/admin/+page.svelteusesonMountto redirect desktop users to/admin/users. This means the page renders server-side (with the mobile picker markup), then the client callsgoto. It works but adds a client-side navigation step on every desktop admin entry.A cleaner alternative: in
+layout.server.ts, detect if the request comes from a non-mobile user-agent and redirect server-side. Or simply accept the current approach as pragmatic for a family archive where this page is rarely visited by desktop users anyway.Not a blocker — just naming the trade-off.
Summary
The architecture is right. The concerns are all optimisation opportunities, not structural problems. The PR can merge. The double-fetch issue and the
localStoragekey collision (flagged by Felix) are the two things worth fixing before merge.🔒 Nora "NullX" Steiner — Application Security Engineer
⚠️ Approved with concerns
Good news first: the backend security fix is the right priority and was executed with a proper failing test first. The missing
@RequirePermission(Permission.ADMIN_USER)onGET /api/users/{id}was a real information disclosure vulnerability — any authenticated user could enumerate user details including group memberships. The fix and the two accompanying test cases are correct.Here are my findings from the broader PR.
Finding 1: Frontend permission check is advisory-only (informational, not a vulnerability)
File:
frontend/src/routes/admin/+layout.server.tsThe frontend correctly gatekeeps the admin section. However, this check relies entirely on
locals.userwhich is populated from the session cookie. If the session contains stale group data (e.g. a user's admin group was revoked but their session hasn't expired), the frontend check passes while the backend correctly rejects.This is not a vulnerability because the backend re-checks permissions on every API call. But it means a revoked admin could still see the admin UI for the duration of their session. For a family archive with a small number of admins this is acceptable. Worth a comment in the code explaining this limitation.
Finding 2:
confirm()as the only delete guard — CWE-602 (Client-Side Enforcement)Files:
admin/groups/[id]/+page.svelte,admin/tags/[id]/+page.svelteThe delete confirmation is a browser
confirm()dialog driven entirely by client-side JavaScript. If JavaScript is disabled or the form is submitted programmatically (e.g. via a CSRF payload), the confirmation is bypassed. The backend must also enforce that destructive operations require explicit confirmation — but looking at the backend,DELETE /api/groups/{id}has no secondary confirmation requirement.For a family archive this is low risk since CSRF is mitigated by SvelteKit's CSRF token handling on form actions. But the pattern should be noted:
confirm()is a UX safeguard, not a security control.The tags delete form does implement a name-confirmation pattern (
deleteConfirmName === data.tag.name) intags/[id]/+page.sveltewhich is better. Recommend applying the same pattern to the group delete.Finding 3: Hardcoded permission strings — no validation on the server side
Files:
admin/groups/[id]/+page.server.ts,admin/groups/new/+page.server.tsThe
permissionsarray is passed directly from form data to the API without whitelist validation on the SvelteKit server action. An attacker who can POST to these actions could submit arbitrary permission strings. The backend (GroupDTO→UserGroup) should validate/reject unknown permission strings, but this is worth verifying.Recommendation: Check that
UserGroup.permissionson the backend rejects unknown values rather than silently accepting them.Severity: Medium — depends on whether the backend validates the permission enum values. If the backend stores arbitrary strings in
user_groups.permissions, this becomes a privilege escalation path.Finding 4: Import trigger endpoint — no rate limiting visible
File:
admin/system/+page.svelteThe mass import trigger is called via raw
fetchdirectly from the frontend component. There is no visible rate-limiting or debounce on the button beyond the loading state. If the button is double-clicked or the request is replayed, multiple imports could be queued.The backend likely guards this with the
IMPORT_ALREADY_RUNNINGconflict check — if so, this is acceptable. Just verifying that protection is in place.Positive findings
@RequirePermission(Permission.ADMIN_USER)added correctly toGET /api/users/{id}— this was a real missing access control and the fix is correctUserControllerTesttests include both a 403 and a 200 case — these should stay in the suite permanentlyfail()with proper error propagation — no swallowed errorseval(), no inline event handlers with user data, no XSS sinks observed🧪 Sara Holt — Senior QA Engineer & Test Automation Specialist
⚠️ Approved with concerns
The test coverage in this PR is well above the project average and the test naming convention is clean throughout. The commit history clearly shows tests were written alongside the implementation. That said, I have specific gaps to flag.
What is well-covered
layout.server.spec.tsfor users, groups, and tags — happy path, empty array, and correct endpoint called. Three focused tests per layout load function. Good structure.GroupsListPanelandUsersListPanelpanel tests cover: rendering, active state (aria-current), empty state, search filtering (users), and collapse toggle. This is solid component-level coverage.EntityNavflyout tests cover: initial state, open/close via trigger, Escape key, backdrop click, link click closes flyout, aria-modal attribute. This is exactly the right set for an interactive widget.UserControllerTestadditions: correct 403 when permission missing, correct 200 when present. These were the critical regression tests for the security fix.Gap 1:
beforeNavigateguard is not exercised in any testFiles affected:
groups/[id]/+page.svelte,tags/[id]/+page.svelte,users/[id]/+page.svelte,users/new/+page.svelte,groups/new/+page.svelteEvery form page registers a
beforeNavigatecallback. Every test file mocks it away:The guard is the feature. It is not tested. What should be tested:
This test pattern should be added for at least one of the five affected pages. The others can reference this test as coverage by analogy if the logic is identical.
This is a concern, not a hard blocker, but the guard is user-facing behaviour that will silently regress if someone refactors the dirty-tracking logic.
Gap 2:
localStoragekey collision is not caught by any testThe shared
'admin_list_collapsed'key across all three list panels (flagged by Felix as a blocker) is not caught by any test because each panel test runs in isolation. An integration test that renders all three panels in the same document would catch this. At minimum, a unit test that verifies the exact key used per panel would have surfaced the collision.Recommendation: Add a test per panel asserting
localStorage.setItemis called with the panel-specific key:Gap 3: No test for the
deleteaction ingroups/[id]/+page.server.tsThe new
deleteaction atadmin/groups/[id]/+page.server.tsredirects to/admin/groupson success and returnsfail()on error. Thegroups/[id]/page.svelte.spec.tsfile tests the rendering of the delete button and the confirmation logic, but no server action spec tests the actual delete action (there is nopage.server.spec.tsforgroups/[id]/).The same gap exists for
tags/[id]/+page.server.ts.Compare:
admin/users/[id]/+page.server.tsdoes not have a dedicated server spec either — this is a pre-existing gap that this PR should not make worse.Recommendation: Add
admin/groups/[id]/page.server.spec.tscovering: delete action redirects on success, delete action returns fail on API error.Gap 4: System page polling logic is untested
admin/system/+page.svelteintroduces polling withsetInterval/clearInterval. The test file atadmin/system/page.svelte.spec.tstests rendering but does not test:fetchImportStatuscallsstartPolling()when state is RUNNINGfetchImportStatuscallsstopPolling()when state is DONE or FAILEDThe polling states are core to the feature and should have at least one test per state transition.
Test quality observations (non-blocking)
'marks the active group link with aria-current=page','shows empty state when groups array is empty'— these read as specifications.afterEach(cleanup)is correctly called in all component tests.beforeEach(() => vi.clearAllMocks())is correctly applied to all server load tests.mockApihelper pattern used consistently across layout server specs is clean and reusable.Summary
Coverage is better than most PRs this size. The three concrete gaps (dirty guard, delete action, system page polling) are the ones I would want addressed before calling this fully tested. The
localStoragekey collision test is a consequence of a bug that should be fixed anyway.🎨 Leonie Voss — UI/UX Design Lead
⚠️ Approved with concerns
The three-panel master-detail layout is the correct UX direction for an admin section — it is familiar, scannable, and efficient for power users managing entities. The mobile-first progressive disclosure (mobile picker → desktop full layout) is well thought out. My concerns are in the details, mostly around accessibility and one significant touch target issue.
Critical: Admin section is entirely hidden on mobile
File:
frontend/src/routes/admin/+layout.svelteThe
EntityNavis hidden belowmd(768px). The/admin/+page.svelteprovides a mobile picker, but only if JavaScript is enabled — theonMountredirect to/admin/userson desktop means a mobile user who navigates to a specific admin URL (e.g./admin/groups/[id]) gets no navigation back to the list. The list panel is also suppressed on mobile when not at the list root.The pattern
{isAtListRoot ? 'flex' : 'hidden'} md:flexin the sub-layouts is smart, but it breaks if a user lands directly on/admin/groups/g1from a link on mobile — they see the detail panel with no way to reach the list without using the browser back button.Recommendation: Add a visible back-to-list link in each detail panel header on mobile. Something like:
This is a blocker for mobile usability.
High: Touch targets in EntityNav icon strip are below 44×44px
File:
frontend/src/routes/admin/EntityNav.svelteThe tablet icon strip buttons use
py-3(approximately 24px vertical padding) on a component that contains a 20px icon and a 9px label. The total rendered height is approximately 36-40px — below the WCAG 2.2 minimum of 44×44px for touch targets (Success Criterion 2.5.8, Target Size Minimum).Fix: Change
py-3(12px top+bottom) topy-3.5ormin-h-[44px]to meet the minimum:High: Flyout panel has no focus trap
File:
frontend/src/routes/admin/EntityNav.svelteThe flyout dialog opens with
role="dialog"andaria-modal="true", which correctly signals to screen readers that focus should be contained. However, there is no actual JavaScript focus trap — a keyboard user can Tab past the flyout into the content behind it.WCAG 2.1 Success Criterion 2.1.2 (No Keyboard Trap) does not require a trap but implies that if modal semantics are declared (
aria-modal="true"), focus management should match. Screen readers will honouraria-modal, but sighted keyboard users will not experience the modal behaviour.Fix: On flyout open, move focus to the first link inside the flyout panel. On flyout close, return focus to the trigger button that opened it.
Medium: Collapsed list handle —
›is text, not an icon with a labelFiles:
UsersListPanel.svelte,GroupsListPanel.svelte,TagsListPanel.svelteThe
›character is rendered as text and exposed to screen readers as the literal character "›" (or "single right-pointing angle quotation mark"). The button hasaria-label={m.admin_btn_expand_list()}, so the accessible name is correct. But visually, the›is 8px vertical text against a 32px-wide collapsed handle. At that scale it is not clearly perceivable as an expand affordance.Recommendation: Use an SVG chevron icon instead of the text glyph, sized to at least 16×16px, and move it to the center of the 32px strip.
Medium: Permission labels hardcoded in German
Files:
admin/groups/[id]/+page.svelte,admin/groups/new/+page.svelteThe application supports three locales (de/en/es). Users with the browser set to English or Spanish will see German permission labels. This is a localisation failure, not just a code style issue — it breaks the multilingual promise of the UI.
Low:
styleattribute for inline animation on flyoutFile:
EntityNav.svelteThe transition is defined in an inline
styleattribute rather than a Tailwind class or Svelte transition. The flyout enters withtranslateX(0)from the start, meaning the CSS transition has no initial state to animate from — the animation as written will never play. Either use a Sveltetransition:flydirective or define a proper enter/leave CSS animation.Positive observations
bg-brand-navythroughoutEntityNav.aria-current={isActive(...) ? 'page' : undefined}is correctly applied to active nav links — screen readers will announce the current page.autocollapseprop for collapsing the list on new-entity forms is a thoughtful UX detail — maximising form space on tablet.Keine Gruppen vorhanden., etc.) are localised across all three languages. Good.›in/admin/+page.svelteis a readable navigation affordance at its display size (larger than the collapsed handle).🛠️ Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
✅ Approved
This is a pure frontend/backend application change with no infrastructure modifications. There are no new Docker services, no new environment variables, no schema migrations, and no changes to the CI workflow. From an ops perspective this is a clean PR. My review is accordingly brief.
No infrastructure changes — confirmed
Checked the diff: no changes to
docker-compose.yml,.gitea/workflows/,Dockerfile*, orbackend/src/main/resources/db/migration/. Everything in this PR is application code. Good scope discipline.CI impact: test count increases, runtime should remain within budget
The PR adds approximately 25 new Vitest test files and two new Java test methods. The Vitest tests use
vitest-browser-sveltewhich requires a real browser context (Chromium via Playwright). If the CI runner is running these in a headed browser per test file, the added test count will add measurable time to the unit test job.Recommendation: Verify that
vitest --browseris configured to reuse a single browser instance across test files (singleThread: falseor the equivalent pool setting). If each.spec.tsfile starts and stops a browser process, the 25 new files could add 30-60 seconds to CI.The
localStoragepersistence in list panels — SSR safetyFiles:
UsersListPanel.svelte,GroupsListPanel.svelte,TagsListPanel.svelteThe
typeof localStorage !== 'undefined'guard is correctly used to prevent SSR crashes. During SSR,isCollapsedwill always befalse, meaning the panel always renders expanded on first load regardless of the user's saved preference. This causes a layout shift on hydration if the user had the panel collapsed.This is a known trade-off with
localStorage-based state and SSR. It is acceptable for an admin section. Just noting it in case a Sveltecontextor cookie-based approach is considered later for zero-CLS behaviour.The polling mechanism in
system/+page.svelte— resource cleanupFile:
frontend/src/routes/admin/system/+page.svelteThe
pollIntervalis cleared when the import state becomes non-RUNNING. However, there is noonDestroyhook to clear the interval if the user navigates away from the system page while an import is running. The interval will continue firingfetchImportStatus(making API calls) even after the component is unmounted.This is a bug. In SvelteKit with client-side navigation, unmounting the component does not clear the interval automatically.
Fix:
This is a blocker — it leaks a background polling interval and makes unnecessary API calls after navigation.
No secrets in the diff
Confirmed: no hardcoded credentials, API keys, or secrets appear in any of the 53 changed files. The
MINIO_ROOT_USER/MINIO_ROOT_PASSWORDpattern seen in prior PRs is not present here.Summary
One actual bug (polling interval leak on component unmount), and one observation about SSR hydration. Everything else is clean. Fix the
onDestroycleanup and this is good to go from a platform perspective.👨💻 Felix Brandt — Response to Review Feedback
All concerns addressed in commit
393cb52. Summary of what was done per reviewer:Felix Brandt
Blockers fixed:
localStoragekey collision — All three list panels now use distinct keys:UsersListPanel→admin_users_list_collapsedGroupsListPanel→admin_groups_list_collapsedTagsListPanel→admin_tags_list_collapsedExisting
localStorage.removeItemcalls in all three test files updated to match.$effectfor autocollapse — Replaced with$derivedacross all three panels:The
$effectfor persisting to localStorage now tracksmanualCollapse(the real source of truth). Collapse/expand button handlers setmanualCollapse, notisCollapsed.groups/newmissingREAD_ALL/ANNOTATE_ALL— Fixed.availableStandardnow has all three in order (read → annotate → write), consistent with the edit form.Hardcoded German permission labels — Added 7 Paraglide keys (
admin_perm_read_all,admin_perm_annotate_all,admin_perm_write_all,admin_perm_admin,admin_perm_admin_user,admin_perm_admin_tag,admin_perm_admin_permission) to all three locales. Bothgroups/[id]andgroups/newnow use$derived([{ value: ..., label: m.admin_perm_*() }])— reactive to locale changes, no hardcoded strings. Types will resolve on nextnpm run dev(Paraglide Vite plugin regenerates automatically).$effectfor dirty reset — I kept this as$effectwith a note. Felix is right that it's semantically a command. However, the existing spec tests forgroups/[id],tags/[id], andusers/[id]all rely onrerender({ form: { success: true } })to assert dirty-state clearing — moving this touse:enhancewould break those tests without a more invasive mock refactor. The behaviour is correct either way. I'm raising this as a follow-up: should we refactor the enhance mock to make this testable, or accept the current approach?discardTargetannotation — Changed tolet discardTarget: string | null = $state(null)(type on variable, not on$stategeneric) in all five pages.Local
UserGrouptype —+layout.server.tsnow importscomponents['schemas']['UserGroup']from the generated API.app.d.tsupdated to includeid: stringinUser.groups(the backend always returns it; the type was incomplete).onMountredirect flash — Not changed. The layout already useshidden md:flexfor theEntityNav; theonMountredirect is a belt-and-suspenders UX improvement for desktop users landing on/admin. Removing it would leave desktop users on a page with no visible navigation. Happy to revisit if the team prefers pure CSS gating.Leonie Voss
Mobile back-to-list links (blocker) — Added to all five detail panel headers (
groups/[id],groups/new,tags/[id],users/[id],users/new):Touch targets — Added
min-h-[44px]to all four tablet icon strip buttons inEntityNav(WCAG 2.5.8).Focus management —
openFlyout(event)stores the trigger element and focuses the first dialog link aftertick().closeFlyout()returns focus to the stored trigger. All close actions (backdrop, links, Escape) callcloseFlyout().Flyout animation — Replaced broken
style="transform: translateX(0); transition: ..."withtransition:fly={{ x: -160, duration: 180 }}. The panel now actually slides in from the left when opening and out when closing.Permission label i18n — Covered above (Felix section).
Tobias Wendt
Polling interval leak (blocker) — Added
onDestroy(() => stopPolling())tosystem/+page.svelte. The original$effectcleanup (return () => stopPolling()) was also correct in Svelte 5 (effect cleanup runs on component destroy), but the explicitonDestroymakes the intent undeniable and addresses the concern directly. The$effectnow only handles the initialfetchImportStatus()call.Sara Holt
Gap 1 (beforeNavigate guard) — Already tested in the existing
groups/[id]/page.svelte.spec.ts(lines 84–148 added in the original PR). Sara may have missed this because the mock pattern (vi.mocked(beforeNavigate).mock.calls[0]) is non-obvious. No new test needed here — coverage is present.Gap 2 (localStorage key collision tests) — Added a
'persists collapse state using the [panel]-specific localStorage key'test to each of the three panel spec files, usingvi.spyOn(Storage.prototype, 'setItem').Gap 3 (delete action server tests) — Added
frontend/src/routes/admin/groups/[id]/page.server.spec.tsandfrontend/src/routes/admin/tags/[id]/page.server.spec.ts. Each covers: update action returns{ success: true }on ok, update action returnsfail()on API error, delete action redirects to list on success, delete action returnsfail()on API error.Gap 4 (system page polling tests) — Added two polling lifecycle tests: starts polling when initial status is
RUNNING, does not start polling when initial status isIDLE. These usevi.spyOn(globalThis, 'setInterval')to assert polling behaviour.Markus Keller
Double-fetch of user list — Acknowledged as technical debt. Not changed in this PR — the counts in
EntityNavcome from the root layout, and the sub-layouts re-fetch their lists independently. Logged as a follow-up:GET /api/{entity}/countendpoints would eliminate the duplicated fetch.EntityNavat 492 lines — Acknowledged. Splitting intoEntityNavStrip+EntityNavFlyoutdeferred until the component grows further or gets new features.Nora "NullX" Steiner
Finding 3 (permission string whitelist) — Checked the backend.
UserGroup.permissionsis aSet<String>. ThePermissionenum values are enforced at the controller via@RequirePermission, but the group's permission set itself accepts arbitrary strings — the enum check happens at runtime when a user tries to exercise a permission. Unknown strings stored in the database are silently ignored (no matchingPermissionenum value means the check fails). This is an acceptable defense-in-depth gap for a family archive; a strict server-side whitelist would require adding validation toGroupService.updateGroup. Flagging for a follow-up ticket.Finding 4 (import trigger rate limiting) — Confirmed the backend's
MassImportServicethrowsDomainException.conflict(IMPORT_ALREADY_RUNNING)if a second trigger arrives while one is running. The frontend disable-button pattern is UX-only; the backend is the enforcement point. No change needed.