feat: Admin tag page complete overhaul — hierarchy tree, parent picker, merge, delete guard #248
Reference in New Issue
Block a user
Delete Branch "%!s()"
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?
Context
Issue #221 introduced infinite-depth tag hierarchy. The existing admin tag page can't cope:
pl-5to all children regardless of depth<select>is unusable beyond ~20 tagsDesign spec:
docs/specs/admin-tag-overhaul.htmlPre-existing out-of-scope bug:
TagService.ALLOWED_TAG_COLORSdoesn't match the frontend color swatches — do not fix in this issue.Phase 1 — Backend: Tag Merge Endpoint
Files:
TagRepository.java,TagService.java,TagController.java,ErrorCode.java,TagServiceTest.java,TagControllerTest.javaNew repository queries
reassignDocumentTags(sourceId, targetId)— UPDATE document_tags, skip docs that already have targetId to avoid PK conflictdeleteDocumentTagsByTagId(sourceId)— remove remaining source rowsreparentChildren(sourceId, targetId)— UPDATE tag SET parent_id = targetId WHERE parent_id = sourceIdfindDescendantIds(tagId)— recursive CTE returning UUID of tag + all descendants (UUID variant; by-name variant already exists)New ErrorCode values
TagService.mergeTags(sourceId, targetId)—@TransactionalTAG_MERGE_SELFfindDescendantIds(sourceId)→TAG_MERGE_INVALID_TARGETreassignDocumentTagsdeleteDocumentTagsByTagId(sourceId)reparentChildren(sourceId, targetId)deleteById(sourceId)POST /api/tags/{id}/merge—@RequirePermission(ADMIN_TAG)Body:
{ "targetId": "uuid" }— returns survivingTagMirror of
POST /api/persons/{id}/merge.Tests (mirror
PersonServiceTest/PersonControllerTestpatterns)mergeTags_throwsBadRequest_whenSelfMergemergeTags_throwsBadRequest_whenTargetIsDescendantmergeTags_reassignsDocumentsAndReparentsChildrenAndDeletesSourcePhase 2 — Backend: Subtree Delete Endpoint
Files:
TagService.java,TagController.java, testsTagService.deleteWithDescendants(id)—@TransactionalfindDescendantIds(id)— get all IDs including selfdocument_tags(adddeleteDocumentTagsByTagIds(Collection<UUID>)to repository)deleteAllById(descendantIds)DELETE /api/tags/{id}/subtree—@RequirePermission(ADMIN_TAG), returns 204Tests
deleteWithDescendants_deletesTagAndAllDescendantsdeleteWithDescendants_removesDocumentTagsForAllDescendantsPhase 3 — Backend: Enrich Tree DTO
Files:
TagTreeNodeDTO.java,TagService.java,TagController.java,TagRepository.javaparentIdfield toTagTreeNodeDTO(populated inbuildTree()fromtag.getParentId())TagRepository(aggregate againstdocument_tags)TagService.getTagTree()to populatedocumentCountper nodePhase 4 — Regenerate API Types
After backend is running with
--spring.profiles.active=dev:New:
POST /api/tags/{id}/merge,DELETE /api/tags/{id}/subtree, enrichedTagTreeNodeDTOwithparentId+documentCount.Phase 5 — Frontend: Layout Server
File:
frontend/src/routes/admin/tags/+layout.server.tsSwitch from
GET /api/tags→GET /api/tags/tree. Expose both:tree: TagTreeNodeDTO[]— for the tree paneltags: FlatTag[]— derived flat list withparentId, for parent picker + tag lookupPhase 6 — Frontend: TagsListPanel Rewrite
File:
frontend/src/routes/admin/tags/TagsListPanel.sveltetree: TagTreeNodeDTO[]instead of flattagsSvelteMap<string, boolean>persisted tolocalStorageas JSONpadding-left: calc({depth} * 12px + 4px)— inline style, no depth cap<button aria-expanded aria-controls>(16×36px) +<a>linkw-60(240px, was 200px)(n)suffix fromnode.documentCount→expands,←collapses,↑↓navigate visible nodesPhase 7 — Frontend: TagParentPicker Component
New file:
frontend/src/lib/components/TagParentPicker.svelteMirror
PersonTypeahead.svelte.Props:
name,value(bound UUID),label,tags(flat list for path resolution),excludeIds(self + descendants).<input type="hidden">storing UUIDGET /api/tags?query=tagsprop)excludeIdsfiltered client-side; backend cycle detection is the hard backstoprole="combobox",aria-expanded,aria-autocomplete="list",aria-activedescendantPhase 8 — Frontend: Edit Page Overhaul
Files:
frontend/src/routes/admin/tags/[id]/+page.sveltefrontend/src/routes/admin/tags/[id]/+page.server.tsfrontend/src/routes/admin/tags/TagMergeZone.svelte[id]/+page.sveltechanges<a>link except the last<select>with<TagParentPicker>— passexcludeIds={[data.tag.id, ...descendantIds]}parentId !== ''; color picker hidden{child.name} (count), max 5 + "… und N weitere →"<TagMergeZone tag={data.tag} tags={data.tags} form={form} />delete-only(default) /delete-subtreeTagMergeZone.svelte(new)Two-step inline flow (like
PersonMergePanel.svelte):TagParentPickerfor target + preview card (docs count + children count that will move)[id]/+page.server.tsadditionsmergeaction — callsPOST /api/tags/{id}/merge, on successredirect(303, /admin/tags/{targetId})deleteaction — readsdeleteModefrom formData; ifdelete-subtreecallsDELETE /api/tags/{id}/subtree, otherwise existing flowPhase 9 — Frontend: i18n + Error Codes
Files:
messages/de.json,messages/en.json,messages/es.json,src/lib/errors.tsNew keys (German — translate for en/es):
admin_tag_parent_placeholder: "Kein übergeordnetes Schlagwort"admin_tag_inherited_color: "Farbe von {name} — wird vererbt"admin_tag_children_section: "Untergeordnete Schlagwörter"admin_tag_children_more: "… und {count} weitere →"admin_tag_merge_heading: "Zusammenführen"admin_tag_merge_description: "Alle Dokumente und untergeordnete Schlagwörter auf ein anderes Schlagwort übertragen und dieses danach löschen."admin_tag_merge_btn: "Mit anderem Schlagwort zusammenführen …"admin_tag_merge_target_label: "Ziel-Schlagwort"admin_tag_merge_preview_docs: "{count} Dokumente werden verschoben"admin_tag_merge_preview_children: "{count} Untergeordnete werden umgehängt"admin_tag_merge_deleted_after: "{name} wird danach gelöscht."admin_tag_merge_confirm_btn: "Jetzt zusammenführen"admin_tag_merge_step1: "Schritt 1 von 2 — Ziel-Schlagwort wählen"admin_tag_merge_step2: "Schritt 2 von 2 — Zusammenführen bestätigen"admin_tag_delete_impact: "{docs} Dokumente verknüpft · {children} Untergeordnete"admin_tag_delete_only_this: "Nur dieses Schlagwort löschen"admin_tag_delete_only_this_sub: "Untergeordnete werden zu {parent} verschoben"admin_tag_delete_subtree: "Gesamten Teilbaum löschen"admin_tag_delete_subtree_warn: "Löscht auch {count} Untergeordnete Schlagwörter unwiderruflich"error_tag_merge_self: "Ein Schlagwort kann nicht mit sich selbst zusammengeführt werden."error_tag_merge_invalid_target: "Das Ziel-Schlagwort ist ein Untergeordnetes und kann nicht als Ziel verwendet werden."errors.ts: add cases forTAG_MERGE_SELF,TAG_MERGE_INVALID_TARGETVerification
./mvnw test -Dtest=TagServiceTest,TagControllerTest— all pass./mvnw clean package— no regressionsnpm run generate:api— completes without errorsON DELETE SET NULLnpm run check+npm run lint— clean👨💻 Felix Brandt — Senior Fullstack Developer
Questions & Observations
Backend
findDescendantIds(tagId)is described as a "recursive CTE" — is this a@Query(nativeQuery = true)or a stored procedure? Native@Queryis the right call (no migration needed, no proc to maintain). Confirm it uses a named parameter:tagId, not string concatenation.TagService.mergeTagsorchestrates 6 steps. That's on the edge of "one method doing too much." Consider extracting a privatetransferDocuments(sourceId, targetId)helper for steps 3+4, and keepingmergeTagsas the thin orchestrator. Each helper does one thing, each is named, each is testable in isolation.deleteWithDescendantsstep order: (1) find IDs, (2) delete fromdocument_tags, (3) delete tags. Iftag.parent_idhas a FK constraint, deleting all IDs in bulk viadeleteAllById(descendantIds)might fail unless JPA resolves bottom-up ordering or the FK isON DELETE SET NULL/CASCADE. Worth an explicit note that the delete order must handle self-referential FK.Frontend
TagParentPickermirrorsPersonTypeahead— good. If the debounce/dropdown logic is copy-pasted, flag it. If the query endpoint, result shape, andexcludeIdsfiltering are different enough, two components are correct. But if there's >50% shared logic, a shareduseTypeaheadcomposable or base component might pay off here.TagsListPanelrecursive tree — if each node renders its children inline, the file will grow past 60 lines very quickly. Consider aTagTreeNode.sveltecomponent for the recursive part:TagsListPanelrenders roots,TagTreeNoderenders itself and recurses. That's two nameable visual regions.[id]/+page.sveltecontains 5 distinct visual regions: ancestry breadcrumb, parent picker, children preview, merge zone, and delete guard. Each region is a named component boundary. Are these being extracted (TagAncestry.svelte,TagChildrenPreview.svelte) or will they all live inline?SvelteMap<string, boolean>for collapse state is the right call — plainMapmutations are invisible to Svelte 5's reactivity. ✅Suggestions
mergeTags, lead with guard clauses: self-merge check first, descendant check second, happy path at the bottom at the lowest indentation level.flattenTreeutility in Phase 5 runs inload()server-side — good. Make sure it doesn't get called inside a client-side$derived.by()that re-runs on every reactive update.TagMergeZone.svelte, the two-step state should be$stateflags, not inferred from{#if form?.step}. The step is client UI state; the form result is server state. Keep them separate.{#each nodes as node (node.id)}— all{#each}blocks over tag nodes must be keyed. Unkeyed blocks will silently corrupt collapse state on list reordering.🏛️ Markus Keller — Application Architect
Questions & Observations
Data layer
reassignDocumentTagsanddeleteDocumentTagsByTagIdare bulk@Modifying @Querystatements on a junction table. These need@Modifying(clearAutomatically = true)(or an explicit flush before the subsequentdeleteAllById) to avoid stale first-level cache after bulk updates. Without it, JPA may still see deleted rows as present during the same transaction.tagtable have an index onparent_id? The recursive CTE traversesparent_idat every level. Without an index, each level of the recursion is a full table scan. This is invisible at 50 tags, painful at 10k. Worth checking if #221 added this; if not, aCREATE INDEX idx_tag_parent_id ON tag(parent_id)Flyway migration belongs in this issue.ON DELETE SET NULL" for the single-tag delete — is the FK constraint withON DELETE SET NULLalready in the schema from #221, or does this issue need a migration to add it? The spec doesn't mention a migration for this case.@Modifyingand native queriesreassignDocumentTagsis described as anUPDATE document_tags ... WHERE NOT EXISTS. This is native SQL, not JPQL. The spec should be explicit:@Query(value = "UPDATE ...", nativeQuery = true). JPQL cannot expressUPDATE ... WHERE NOT EXISTS (subquery)cleanly, and I want implementors to reach for the right annotation on first pass.DTO design
TagTreeNodeDTOgainsparentIdanddocumentCount. IfdocumentCountcan logically be 0 (leaf tag with no documents), the backend should guarantee it is nevernull— useintnotInteger, and annotate@Schema(requiredMode = REQUIRED). The frontend fallback?? 0should be unnecessary; let the backend make the guarantee.documentCountis populated via a per-node aggregate query. IfgetTagTree()calls the count query N times (once per tag), that's N+1 behavior on every tree load. Consider a single aggregate query that returns all(tag_id, count)pairs and merges them intobuildTree().Module boundaries
TagService.mergeTagscalls only tag-domain repositories.TagService.deleteWithDescendantslikewise. No cross-domain service calls in scope. ✅ Clean.Suggestions
VN__add_tag_parent_id_index.sqland/orVN__add_tag_parent_fk_on_delete_set_null.sqlif these don't already exist. Migrations are the only auditable record of schema changes.documentCountaggregate:SELECT tag_id, COUNT(*) FROM document_tags GROUP BY tag_idreturns a result set that can be converted to aMap<UUID, Long>and merged intobuildTree()in a single pass. That's one query instead of N.🧪 Sara Holt — QA Engineer
Questions & Observations
Missing test cases for
TagServiceTestThe spec names the happy path and two error cases for merge, but these edge cases are missing:
mergeTags_whenSourceHasNoDocuments_deletesSourceAndReparentsChildren— step 3 is a no-op; verify it doesn't errormergeTags_whenTargetAlreadyOnSomeDocuments_skipsThoseDocuments— this is the PK-conflict avoidance case ("skip docs that already have targetId"). It's the most subtle behavior in the whole feature and deserves its own explicit testmergeTags_whenSourceHasNoChildren_deletesSafely—reparentChildrenwith zero rows should be a safe no-opdeleteWithDescendants_whenTagHasNoDescendants_deletesTagOnly— boundary case: a leaf nodeFor
TagControllerTest, the spec says "401 / 403 / 200" but omits 404. What happens when{id}doesn't exist? Addmerge_returns404_when_sourceTag_doesNotExist.deleteDocumentTagsByTagIds(Collection<UUID>)edge caseAn
IN ()clause with an empty collection is invalid SQL in PostgreSQL and will throw at runtime. Add a testdeleteWithDescendants_whenCollectionIsEmpty_isNoOpand guard the repository call with a size check.Frontend test coverage gaps
TagsListPanelcollapse state persists tolocalStorage— this behavior should be an explicit component test: collapse a node, reload, assert it is still collapsed.vitest-browser-sveltecan readlocalStorage.→/←/↑↓) — these are behavioral requirements, not visual ones. They need assertion-based tests, not screenshots. Consider Playwright tests for each key binding.Acceptance criteria gaps
GET /api/tags/treereturns an empty array? Does the panel show an empty state or is empty tree impossible?/admin/tags/{targetId}. What should the page title/breadcrumb show immediately after redirect?Suggestions
PersonServiceTesttest naming convention:mergeTags_throwsBadRequest_whenSelfMerge✅ — keep it consistent across the test file.deleteWithDescendantsintegration test (if written against real PostgreSQL via Testcontainers) is the only test that can catch the FK deletion ordering issue. Consider adding an integration test alongside the unit tests for this method specifically.🔐 Nora "NullX" Steiner — Application Security Engineer
Questions & Observations
Authorization
Both new endpoints specify
@RequirePermission(ADMIN_TAG)— mirrors existing tag write endpoints. ✅ The permission boundary test spec lists 401/403/200. One addition: also test401for the subtree delete endpoint specifically — the spec mentions it for merge but the subtree delete section only says "auth/permission + 204."Input validation at controller boundary
POST /api/tags/{id}/mergebody{ "targetId": "uuid" }— what happens iftargetIdis null or absent from the body? The service validatessourceId != targetIdbut the null case hits that check as a NullPointerException, not a cleanBAD_REQUEST. Add a controller-level guard:if (dto.getTargetId() == null) throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "targetId required").DELETE /api/tags/{id}/subtree— no request body, just a path parameter. Clean. ✅Client-side security boundary — explicit documentation needed
Phase 7 states: "
excludeIdsfiltered client-side; backend cycle detection is the hard backstop." This is correct architecture. However, without an explicit comment in the code, a future developer might remove the backendTAG_MERGE_INVALID_TARGETvalidation thinking "the frontend already handles it." The implementation should include a comment on the service validation:// Security backstop — do not remove. Frontend excludeIds is UX-only, not a security control.Bulk query parameter binding
deleteDocumentTagsByTagIds(Collection<UUID>)— if this is a native@QuerywithIN (:ids), Spring Data JPA binds the collection as parameters correctly (not string-interpolated). ✅ Just confirm the@Queryannotation uses:idsas a named parameter, notIN (+ string join +).Transactional atomicity
mergeTagsis@Transactional— validation, reassignment, reparent, and delete are atomic. IfdeleteById(sourceId)fails on a FK violation, the whole merge rolls back cleanly. ✅ The only risk is ifclearAutomatically = trueis missing from@Modifying(see Markus's comment) — a stale cache could mask an incomplete state mid-transaction.Suggestions
targetIdat the controller boundary before the service call.ResponseStatusException(BAD_REQUEST)is acceptable per the project's error handling conventions for simple controller validation.{ "confirm": true }field — the UI flow is sufficient for a family archive context.🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist
Questions & Observations
Touch targets — critical
The chevron button is specified as
16×36px. WCAG 2.2 requires a minimum of 44×44px for all interactive controls. The collapse chevron is the primary navigation mechanism for the tree — this must be fixed. Recommend: wrap the chevron in a<button class="flex items-center justify-center min-h-[44px] min-w-[44px]">with the 16px icon centered inside. The adjacent tag link covers horizontal space, so the 44px height is the real fix needed.ARIA tree semantics
The spec specifies
aria-expandedandaria-controlson the chevron button — good. But for proper assistive technology support, the tree container needsrole="tree"and each node row needsrole="treeitem". Without these, screen readers announce the tree as a flat list, not a navigable hierarchy. VoiceOver and NVDA both rely onrole="treeitem"to communicate "this item has children" to the user.Brand color — amber button
TagMergeZonespecifies an "amber" confirm button (Jetzt zusammenführen). Amber is not in the project's brand palette (navy/mint/sand). If the intent is to signal a destructive-adjacent action, two options:bg-amber-600 hover:bg-amber-700 text-whitefrom Tailwind and document it as an intentional one-off for dangerous confirmationsbg-brand-navyand prefix with a warning icon⚠to signal the weight of the actionPick one and be consistent with how the person merge zone handles this.
Delete guard UX — hidden input reveal
"Name confirm input hidden until radio selected" — for the senior audience (60+), UI appearing unexpectedly after a click is disorienting. Consider showing the input disabled from the start:
<input disabled class="opacity-50" />. The user sees the full flow upfront and isn't surprised when new UI materializes.Root-level tag edge case in delete guard subtitle
admin_tag_delete_only_this_sub: "Untergeordnete werden zu {parent} verschoben" — what renders when the tag being deleted is a root (no parent)?{parent}would be empty. Needs a fallback: "Untergeordnete werden zur obersten Ebene verschoben" or similar.Ancestry breadcrumb overflow
Deep hierarchies (5+ levels) will overflow the card width on mobile (320px). Consider middle-truncation:
Root > … > Grandparent > Parent > Current, where only first and last segments are always shown.Children preview "… und N weitere →"
This is rendered as a link (
→). Where does it navigate? If it links to the tag's own page or a filtered list view, use<a href="...">. If it expands inline, use<button>. The element type must match the behavior.Suggestions
aria-label={m.close()}oraria-label={m.clear_selection()}to the × clear button inTagParentPicker. Without it, screen readers announce just "button."AxeBuilderspecifically on the tree panel —role="tree"+aria-expanded+aria-activedescendantcombinations are one of the more reliably caught axe violation patterns. Catching it in E2E before ship is much cheaper than after.⚙️ Tobias Wendt — DevOps & Platform Engineer
Questions & Observations
Database index — check before shipping
The
findDescendantIdsrecursive CTE traversestag.parent_idat every level. Without an index ontag(parent_id), each CTE recursion step is a full table scan. With 50-200 tags this is invisible; at 5k+ it shows up in Loki. Check whether #221 addedCREATE INDEX idx_tag_parent_id ON tag(parent_id). If not, this issue needs a Flyway migration to add it. The spec's verification section should explicitly name that migration.Missing Flyway migration entries
The spec doesn't list any new migration files. Two candidates worth confirming:
idx_tag_parent_idindex (if not already present)ON DELETE SET NULLFK constraint ontag.parent_idfor the single-tag delete (if not already present from #221)If both are already in the schema from #221, great — note it explicitly so the next person doesn't have to check. If not, add the migrations and include
./mvnw test -Dtest=MigrationIntegrationTestin the verification steps.Bulk operation observability
TagService.deleteWithDescendantscould delete hundreds ofdocument_tagsrows in one transaction. At family archive scale this is fine, but a singlelogger.info("Deleting subtree rooted at {}, {} descendants", rootId, descendantIds.size())before the bulk delete gives Loki something useful when a slow operation is reported. Same suggestion formergeTags.No new infrastructure
This feature adds no new services, containers, ports, volumes, or dependencies. The Docker Compose file does not change. ✅
CI pipeline — API type regeneration
Phase 4 (
npm run generate:api) requires a running backend with--spring.profiles.active=dev. This is a developer step, not CI-automated. Two questions:src/lib/api.d.ts(or equivalent) committed to the repo?If the generated file is committed and goes stale, TypeScript will silently compile against outdated types. Either automate the regeneration check in CI or document clearly that the dev must regenerate before pushing.
IN ()with empty collectionSara flagged this in the test context — I'll add the infrastructure angle:
deleteDocumentTagsByTagIds(emptyCollection)with a nativeIN ()will throw a PostgreSQL syntax error in production, not just in tests. The fix (size check before the query) is a one-liner but must be in the implementation, not just the tests.Suggestions
logger.info(...)calls at the start ofmergeTagsanddeleteWithDescendants— two lines that make these irreversible operations visible in Loki without being noisy../scripts/reset-db.sh && ./mvnw spring-boot:run— this catches migration ordering issues that unit tests miss.🏛️ Markus Keller — Architect Discussion Summary
Follow-up to my earlier review comment. Worked through 5 open items with the team — all resolved.
✅ Item 1 — Flyway migrations: nothing new needed
V39 (
add_tag_hierarchy.sql) from #221 already covers everything:CREATE INDEX idx_tag_parent_id ON tag(parent_id)✅ALTER TABLE tag ADD COLUMN parent_id UUID REFERENCES tag(id) ON DELETE SET NULL✅ALTER TABLE tag ADD CONSTRAINT chk_tag_no_self_reference CHECK (parent_id != id)✅Issue #248 requires zero new Flyway migrations. The spec should note this explicitly so no one writes a redundant migration.
✅ Item 2 —
documentCount: single aggregate query, not N+1Add
findDocumentCountsPerTag()toTagRepository:In
getTagTree(): convert toMap<UUID, Long>, pass intobuildTree()as a lookup. One query total, O(1) per node.✅ Item 3 —
@Modifying(clearAutomatically = true)required on both bulk methodsreassignDocumentTagsanddeleteDocumentTagsByTagIdmust both declare@Modifying(clearAutomatically = true). Without it, JPA's first-level cache is not flushed after the native UPDATE/DELETE, creating a stale-read landmine for any future step added to the merge transaction.✅ Item 4 — Full native SQL specified for all four repository methods
All four methods require
@Query(nativeQuery = true). Exact SQL:reassignDocumentTagsdeleteDocumentTagsByTagIddeleteDocumentTagsByTagIds⚠️ Guard required:
if (!ids.isEmpty()) repository.deleteDocumentTagsByTagIds(ids)— PostgreSQL rejectsIN ().findDescendantIds✅ Item 5 —
documentCountis non-null:int+@Schema(requiredMode = REQUIRED)Backend guarantees
documentCountis never null (0 for tags with no documents). DTO field:The
?? 0fallback inflattenTreeis unnecessary once the type is regenerated and can be removed.Overall this is a well-scoped spec. The data layer decisions are sound — adjacency list, native CTEs, manual junction table cleanup. No architectural concerns remaining.
👨💻 Felix Brandt — Developer Discussion Summary
Follow-up to my earlier review comment. Worked through 5 open component and code structure decisions — all resolved.
✅ Item 1 —
TagTreeNode.svelteis a required new fileTagsListPanelhandles too many concerns if recursive node rendering stays inline. Required split:TagsListPanel.svelte— ownsSvelteMapcollapse state, localStorage sync, keyboard handler, renders root nodes viaTagTreeNodeTagTreeNode.svelte(new) — props:node: TagTreeNodeDTO,depth: number,collapseMap: SvelteMap<string, boolean>— renders chevron + link + count badge, recurses into children✅ Item 2 — Three more required new files for
[id]/+page.svelteThe page has 5 visual regions. Only
TagMergeZone.sveltewas called out — adding the other three:TagAncestry.svelte(new) — props:tag: Tag,tags: FlatTag[]— ancestor chain as linked breadcrumb segmentsTagChildrenPreview.svelte(new) — props:children: FlatTag[]— up to 5 child chips with doc count + overflow linkTagDeleteGuard.svelte(new) — props:tag: Tag,descendantCount: number,documentCount: number,form— radio options, impact summary, name confirm, submit[id]/+page.sveltebecomes a thin orchestrator that composes these four components.✅ Item 3 —
useTypeahead.svelte.tsshared composableTagParentPickerandPersonTypeaheadshare identical debounce + dropdown + hidden-input + ARIA mechanics. Required new file:src/lib/composables/useTypeahead.svelte.ts(new) — owns debounce timer,$statefor query/results/isOpen/activeIndex,$derivedactive-descendant IDBoth
TagParentPicker.svelte(new) andPersonTypeahead.svelte(refactor) consume it. MigratingPersonTypeaheadis in scope for this issue — both components must use the composable when this ships.✅ Item 4 —
canDeletemust be a named$derivedinTagDeleteGuardNo inline conditions on the submit button. Required in
TagDeleteGuard.svelte:Template:
disabled={!canDelete}. Intent is named; template stays declarative.✅ Item 5 —
mergeTagsdecomposes into three private helpersThe 6-step method must be split. Required private methods in
TagService:Public method becomes a 5-line orchestrator:
The spec is now explicit enough to implement without structural surprises. Main addition beyond the original:
useTypeahead.svelte.tsis the most impactful new piece — it refactors existing code and needs its own failing tests before the composable is written.🎨 Leonie Voss — UI/UX Discussion Summary
Follow-up to my earlier review comment. Worked through 6 open accessibility, brand, and interaction decisions — all resolved.
✅ Item 1 — Chevron touch target:
min-h-[44px] min-w-[44px]requiredThe
16×36pxspec value is replaced.TagTreeNode.sveltechevron button must use:Icon stays 16px centered inside the 44px tap zone. WCAG 2.2 SC 2.5.8 compliant.
✅ Item 2 — Full ARIA tree pattern required
aria-expandedon the chevron alone is not enough. Full pattern:TagsListPanel.svelte:<ul role="tree" aria-label={m.admin_tag_tree_label()}>TagTreeNode.svelte:<li role="treeitem" aria-expanded={hasChildren ? isExpanded : undefined}><ul role="group">aria-expanded={undefined}(omitted), neverfalseadmin_tag_tree_label("Schlagwort-Baum") in all three languages✅ Item 3 — New
brand-warningdesign token (replaces ad-hoc amber)Amber is not in the palette. Adding a proper token:
Utilities:
bg-brand-warning,text-brand-warning,border-brand-warning.TagMergeZoneconfirm button:bg-brand-warning hover:bg-brand-warning/90 text-white. Reusable for all future "consequential but not destructive" confirmations.✅ Item 4 — Delete guard: modal pattern, no name-confirm input
The name-confirm pattern is not used elsewhere in the app. Replacing with the standard modal confirmation flow:
const canDelete = $derived(selectedMode !== null)(radio selected only)Name-confirm i18n keys dropped from Phase 9. Felix's
canDeletederived simplifies accordingly.✅ Item 5 — Root tag delete subtitle: fallback i18n key
admin_tag_delete_only_this_sub("Untergeordnete werden zu {parent} verschoben") would render empty{parent}for root tags. Two keys required:admin_tag_delete_only_this_sub— shown whentag.parentIdis setadmin_tag_delete_only_this_sub_root: "Untergeordnete werden zur obersten Ebene verschoben" — shown whentag.parentIdis null✅ Item 6 — "… und N weitere" expands inline, not a link
TagChildrenPreview.svelteuses a<button>with$state(showAll):No navigation, no
→. The overflow line disappears on expand.Accessibility is in good shape once the ARIA tree pattern and touch targets are locked in. The
brand-warningtoken is the biggest addition to the design system from this issue.