feat(tag-typeahead): tree-aware results — expand children & surface ancestor path #250

New Issue

marcel · 2026-04-17T11:10:49+02:00

marcel commented

2026-04-17 11:10:49 +02:00

Summary

The tag typeahead currently only returns tags whose names match the search query. Two gaps result:

Parent match, no children shown — if the user types "Brief" and "Briefe" matches, its children ("Familienbriefe", "Weihnachtsbriefe") are invisible unless the user already knows their exact names.
Child match, no ancestor context — if the user types "Hochzeit", the result appears without hierarchy context. The user cannot tell where "Hochzeit" sits in the tree.

Design spec

→ docs/specs/tag-typeahead-tree-aware.html

The spec covers all cases with mockups at ~55% scale and a full implementation reference table.

Behaviour

When a root tag matches: all descendants are appended to the result set (via existing findDescendantIds recursive CTE).
When a child tag matches: all ancestors are prepended to the result set (via existing findAncestorIds recursive CTE).
Every item in the dropdown is selectable — context nodes (ancestors) are valid tags, not decorations.
Direct matches render in full color + font-medium. Context nodes render in text-ink-3 with a › prefix for root-level ancestors.

Scope

Backend — `TagService.java`

Modify search(String query) only. No new endpoint, no new DTO, no API type regeneration.

// For each matched tag:
if (parentId == null) {
    extraIds.addAll(tagRepository.findDescendantIds(id));  // root → expand children
} else {
    extraIds.addAll(tagRepository.findAncestorIds(id));    // child → walk up to root
}
// batch-fetch extras, deduplicate, resolveEffectiveColors, return

Reuses: TagRepository.findDescendantIds(), TagRepository.findAncestorIds(), TagRepository.findAllById(), TagService.resolveEffectiveColors() — all already exist.

Frontend — `TagInput.svelte`

Replace the current 1-level-deep ordering logic with a recursive DFS that computes depth per entry:

type SuggestionEntry = { tag: Tag; depth: number; isDirectMatch: boolean };

Depth indentation: depth * 16 + 12 px (inline style — dynamic Tailwind classes not viable)
Root context nodes get a › prefix
Color dot opacity: 100% for direct matches, 50% for context nodes
Keyboard navigation unchanged — arrow keys traverse all entries regardless of depth

Tests

TagServiceTest: 3 new unit tests (children included, ancestors included, deduplication)
TagInput.svelte.spec.ts: 4 new/updated tests (ancestor above match, children below match, 3-level DFS order, Enter on context node adds tag)

Files to change

File	Change
`backend/…/service/TagService.java`	Modify `search()`
`backend/…/service/TagServiceTest.java`	Add 3 unit tests
`frontend/src/lib/components/TagInput.svelte`	Rewrite `orderedSuggestions` + template
`frontend/src/lib/components/TagInput.svelte.spec.ts`	Add/update 4 tests

## Summary The tag typeahead currently only returns tags whose **names** match the search query. Two gaps result: 1. **Parent match, no children shown** — if the user types "Brief" and "Briefe" matches, its children ("Familienbriefe", "Weihnachtsbriefe") are invisible unless the user already knows their exact names. 2. **Child match, no ancestor context** — if the user types "Hochzeit", the result appears without hierarchy context. The user cannot tell where "Hochzeit" sits in the tree. ## Design spec → **[docs/specs/tag-typeahead-tree-aware.html](../blob/main/docs/specs/tag-typeahead-tree-aware.html)** The spec covers all cases with mockups at ~55% scale and a full implementation reference table. --- ## Behaviour - When a **root tag matches**: all descendants are appended to the result set (via existing `findDescendantIds` recursive CTE). - When a **child tag matches**: all ancestors are prepended to the result set (via existing `findAncestorIds` recursive CTE). - Every item in the dropdown is selectable — context nodes (ancestors) are valid tags, not decorations. - **Direct matches** render in full color + `font-medium`. **Context nodes** render in `text-ink-3` with a `›` prefix for root-level ancestors. --- ## Scope ### Backend — `TagService.java` Modify `search(String query)` only. No new endpoint, no new DTO, no API type regeneration. ```java // For each matched tag: if (parentId == null) { extraIds.addAll(tagRepository.findDescendantIds(id)); // root → expand children } else { extraIds.addAll(tagRepository.findAncestorIds(id)); // child → walk up to root } // batch-fetch extras, deduplicate, resolveEffectiveColors, return ``` Reuses: `TagRepository.findDescendantIds()`, `TagRepository.findAncestorIds()`, `TagRepository.findAllById()`, `TagService.resolveEffectiveColors()` — all already exist. ### Frontend — `TagInput.svelte` Replace the current 1-level-deep ordering logic with a recursive DFS that computes depth per entry: ```typescript type SuggestionEntry = { tag: Tag; depth: number; isDirectMatch: boolean }; ``` - Depth indentation: `depth * 16 + 12` px (inline style — dynamic Tailwind classes not viable) - Root context nodes get a `›` prefix - Color dot opacity: 100% for direct matches, 50% for context nodes - Keyboard navigation unchanged — arrow keys traverse all entries regardless of depth ### Tests - `TagServiceTest`: 3 new unit tests (children included, ancestors included, deduplication) - `TagInput.svelte.spec.ts`: 4 new/updated tests (ancestor above match, children below match, 3-level DFS order, Enter on context node adds tag) --- ## Files to change | File | Change | |---|---| | `backend/…/service/TagService.java` | Modify `search()` | | `backend/…/service/TagServiceTest.java` | Add 3 unit tests | | `frontend/src/lib/components/TagInput.svelte` | Rewrite `orderedSuggestions` + template | | `frontend/src/lib/components/TagInput.svelte.spec.ts` | Add/update 4 tests |

marcel added the feature ui labels 2026-04-17 11:10:54 +02:00

marcel commented

2026-04-17 11:18:35 +02:00

🏗️ Markus Keller — Senior Application Architect

Observations

Scope is correctly bounded. No new endpoint, no DTO change, no generate:api run. The Tag entity already carries parentId — the frontend already has what it needs. This is the minimum invasive change.
N CTE queries scale linearly with match count. For a typical query matching 1–3 tags at depth ≤ 4, that's 3–5 queries total — negligible. The existing depth guard (50 levels) on both CTEs is the right protection against pathological trees. No additional guard needed, but worth a code comment on search() that documents this cost profile.
findAncestorIds does NOT include the seed ID — contrary to the spec note which says "Both return List<UUID> including the seed ID." Looking at the actual query: SELECT parent_id FROM ancestors WHERE id = :tagId — it returns the parent's ID, not the queried tag's own ID. findDescendantIds does include the seed. The deduplication extraIds.removeAll(matchedIds) is still correct (it catches the findDescendantIds case), but it's effectively a no-op for the ancestor path. No bug — but the deduplication test should target the descendant path to be meaningful, not the ancestor path.
resolveEffectiveColors is single-level by design. It only loads immediate parents. This is correct because colors are stored only at root level (per the code comment). Verify this assumption still holds when implementing — if a multi-level intermediate tag ever gets a color set directly, the enrichment may show wrong colors on grandchildren. Not blocking, just worth a verify-before-merge check.
search() lacks @Transactional — this is correct. Read methods in this codebase are deliberately non-transactional per the CLAUDE.md convention. Do not add @Transactional to search() even though it now issues multiple queries.

Recommendations

Extract the enrichment into a private helper to keep search() under 20 lines and the logic independently readable:

public List<Tag> search(String query) {
    List<Tag> matched = tagRepository.findByNameContainingIgnoreCase(query);
    if (matched.isEmpty()) return matched;
    return enrichWithRelatives(matched);
}

private List<Tag> enrichWithRelatives(List<Tag> matched) {
    // collect extra IDs via CTEs...
    // batch-fetch, deduplicate, resolveEffectiveColors, return
}

This also makes enrichWithRelatives testable in isolation if needed later.

Confirm the early-exit guard if (!extraIds.isEmpty()) is in place before findAllById — the spec's pseudocode shows it. If extraIds is empty (all matched tags are leaves with no hierarchy), skip the batch fetch entirely.
No ADR required. The change stays entirely within TagService's domain, reuses existing CTE infrastructure, and doesn't alter the API contract. Too small to warrant documentation beyond a code comment.

## 🏗️ Markus Keller — Senior Application Architect ### Observations - **Scope is correctly bounded.** No new endpoint, no DTO change, no `generate:api` run. The `Tag` entity already carries `parentId` — the frontend already has what it needs. This is the minimum invasive change. - **N CTE queries scale linearly with match count.** For a typical query matching 1–3 tags at depth ≤ 4, that's 3–5 queries total — negligible. The existing depth guard (50 levels) on both CTEs is the right protection against pathological trees. No additional guard needed, but worth a code comment on `search()` that documents this cost profile. - **`findAncestorIds` does NOT include the seed ID** — contrary to the spec note which says "Both return `List<UUID>` including the seed ID." Looking at the actual query: `SELECT parent_id FROM ancestors WHERE id = :tagId` — it returns the parent's ID, not the queried tag's own ID. `findDescendantIds` *does* include the seed. The deduplication `extraIds.removeAll(matchedIds)` is still correct (it catches the `findDescendantIds` case), but it's effectively a no-op for the ancestor path. No bug — but the deduplication test should target the descendant path to be meaningful, not the ancestor path. - **`resolveEffectiveColors` is single-level by design.** It only loads immediate parents. This is correct because colors are stored only at root level (per the code comment). Verify this assumption still holds when implementing — if a multi-level intermediate tag ever gets a color set directly, the enrichment may show wrong colors on grandchildren. Not blocking, just worth a verify-before-merge check. - **`search()` lacks `@Transactional`** — this is correct. Read methods in this codebase are deliberately non-transactional per the CLAUDE.md convention. Do not add `@Transactional` to `search()` even though it now issues multiple queries. ### Recommendations - **Extract the enrichment into a private helper** to keep `search()` under 20 lines and the logic independently readable: ```java public List<Tag> search(String query) { List<Tag> matched = tagRepository.findByNameContainingIgnoreCase(query); if (matched.isEmpty()) return matched; return enrichWithRelatives(matched); } private List<Tag> enrichWithRelatives(List<Tag> matched) { // collect extra IDs via CTEs... // batch-fetch, deduplicate, resolveEffectiveColors, return } ``` This also makes `enrichWithRelatives` testable in isolation if needed later. - **Confirm the early-exit guard** `if (!extraIds.isEmpty())` is in place before `findAllById` — the spec's pseudocode shows it. If `extraIds` is empty (all matched tags are leaves with no hierarchy), skip the batch fetch entirely. - **No ADR required.** The change stays entirely within `TagService`'s domain, reuses existing CTE infrastructure, and doesn't alter the API contract. Too small to warrant documentation beyond a code comment.

marcel commented

2026-04-17 11:18:59 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

Backend — TagService.java

The current search() is 1 line. The proposed expansion adds ~15 lines. With Markus's enrichWithRelatives() extraction, the public method stays at 4 lines — good.
findAncestorIds seed discrepancy: The spec states both CTEs include the seed ID but findAncestorIds does not — it returns parent_id values starting from the queried tag. The deduplication extraIds.removeAll(matchedIds) is correct for findDescendantIds (which includes the seed), but fires on an empty intersection for the ancestor path. Write the deduplication test (search_deduplicates_when_ancestor_also_matches) by mocking a root tag that also matches — the descendant CTE returns [rootId, childId], so rootId appears in both matchedIds and extraIds. That's the path where deduplication actually fires.
Missing test: None of the 3 specified tests verify that resolveEffectiveColors is called on the merged result list. This is a side-effectful call that drives color display. Add: search_callsResolveEffectiveColors_onAllResults() — mock a root+child result, call search(), verify tagRepository.findAllById(...) is invoked (which is what resolveEffectiveColors triggers for colorless children).

Frontend — TagInput.svelte

Tailwind dynamic class interpolation is a problem. The spec template uses:
```
class="... opacity-{isDirectMatch ? '100' : '50'}"
```
Tailwind's content scanner will not detect opacity-100 / opacity-50 inside a template literal interpolation like this. Use a ternary with full class names instead:
```
class="... {isDirectMatch ? 'opacity-100' : 'opacity-50'}"
```
This is the same reason inline style is used for padding-left — dynamic values can't be reliably scanned. Apply the same discipline to opacity.
suggestionsById derived map becomes dead code after the rewrite. The spec explicitly calls this out. Delete it entirely — the new orderedSuggestions builds its own byId map internally.
handleKeydown breaking change: addTag(orderedSuggestions[activeIndex]) must become addTag(orderedSuggestions[activeIndex].tag). The existing test navigates suggestions with ArrowDown and selects with Enter will catch this regression if it's missed, since it verifies a tag chip is added after keyboard selection.
Existing test at line 223 needs a careful review. shows child suggestion after its parent when both are in results mocks { id: 'p1', name: 'Eltern' } and { id: 'c1', name: 'Kind', parentId: 'p1' }. The DOM-order assertion via [role="option"] will still pass (DFS produces the same order), but the addTag call path now receives a SuggestionEntry, not a Tag. If addTag(suggestion) in the onclick handler isn't updated to addTag(s) (per the spec's template), clicking a suggestion will pass a SuggestionEntry object to addTag, which expects a Tag. This is the key regression risk — the test checks ordering but not selection behavior.

Recommendations

TDD order: write the 3 backend tests first (@Test void search_includes_children_when_root_matches(), etc.), confirm they fail, then implement enrichWithRelatives. Never touch search() before the tests are red.
Add mockFetchWithTagObjects(tags: Tag[]) helper in the spec file to replace the ad-hoc vi.stubGlobal calls the new tests will need — the existing mockFetchWithTags(tagNames: string[]) only creates nameless objects and won't support id/parentId for tree tests.
Remove SvelteMap from orderedSuggestions internals in the new implementation — a plain Map is correct inside $derived.by() since it's recomputed fresh each time and doesn't need reactive mutations.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations **Backend — `TagService.java`** - The current `search()` is 1 line. The proposed expansion adds ~15 lines. With Markus's `enrichWithRelatives()` extraction, the public method stays at 4 lines — good. - **`findAncestorIds` seed discrepancy:** The spec states both CTEs include the seed ID but `findAncestorIds` does not — it returns `parent_id` values starting from the queried tag. The deduplication `extraIds.removeAll(matchedIds)` is correct for `findDescendantIds` (which includes the seed), but fires on an empty intersection for the ancestor path. Write the deduplication test (`search_deduplicates_when_ancestor_also_matches`) by mocking a root tag that also matches — the descendant CTE returns `[rootId, childId]`, so `rootId` appears in both `matchedIds` and `extraIds`. That's the path where deduplication actually fires. - **Missing test:** None of the 3 specified tests verify that `resolveEffectiveColors` is called on the merged result list. This is a side-effectful call that drives color display. Add: `search_callsResolveEffectiveColors_onAllResults()` — mock a root+child result, call `search()`, verify `tagRepository.findAllById(...)` is invoked (which is what `resolveEffectiveColors` triggers for colorless children). **Frontend — `TagInput.svelte`** - **Tailwind dynamic class interpolation is a problem.** The spec template uses: ```svelte class="... opacity-{isDirectMatch ? '100' : '50'}" ``` Tailwind's content scanner will not detect `opacity-100` / `opacity-50` inside a template literal interpolation like this. Use a ternary with full class names instead: ```svelte class="... {isDirectMatch ? 'opacity-100' : 'opacity-50'}" ``` This is the same reason inline style is used for `padding-left` — dynamic values can't be reliably scanned. Apply the same discipline to opacity. - **`suggestionsById` derived map becomes dead code** after the rewrite. The spec explicitly calls this out. Delete it entirely — the new `orderedSuggestions` builds its own `byId` map internally. - **`handleKeydown` breaking change:** `addTag(orderedSuggestions[activeIndex])` must become `addTag(orderedSuggestions[activeIndex].tag)`. The existing test `navigates suggestions with ArrowDown and selects with Enter` will catch this regression if it's missed, since it verifies a tag chip is added after keyboard selection. - **Existing test at line 223 needs a careful review.** `shows child suggestion after its parent when both are in results` mocks `{ id: 'p1', name: 'Eltern' }` and `{ id: 'c1', name: 'Kind', parentId: 'p1' }`. The DOM-order assertion via `[role="option"]` will still pass (DFS produces the same order), but the `addTag` call path now receives a `SuggestionEntry`, not a `Tag`. If `addTag(suggestion)` in the `onclick` handler isn't updated to `addTag(s)` (per the spec's template), clicking a suggestion will pass a `SuggestionEntry` object to `addTag`, which expects a `Tag`. This is the key regression risk — the test checks ordering but not selection behavior. ### Recommendations - TDD order: write the 3 backend tests first (`@Test void search_includes_children_when_root_matches()`, etc.), confirm they fail, then implement `enrichWithRelatives`. Never touch `search()` before the tests are red. - Add `mockFetchWithTagObjects(tags: Tag[])` helper in the spec file to replace the ad-hoc `vi.stubGlobal` calls the new tests will need — the existing `mockFetchWithTags(tagNames: string[])` only creates nameless objects and won't support `id`/`parentId` for tree tests. - Remove `SvelteMap` from `orderedSuggestions` internals in the new implementation — a plain `Map` is correct inside `$derived.by()` since it's recomputed fresh each time and doesn't need reactive mutations.

marcel commented

2026-04-17 11:19:03 +02:00

🔒 Nora Steiner (@NullX) — Application Security Engineer

Observations

Attack surface unchanged. No new endpoints, no new input parameters, no authorization model changes. The enrichment happens server-side on a read path that is already behind authentication.
Input encoding already correct. The fetch in TagInput.svelte uses encodeURIComponent(query) before constructing the URL. No injection risk in the query parameter.
CTE depth guard already in place. findDescendantIds and findAncestorIds both include AND a.depth < 50 — a crafted deeply-nested tag tree cannot cause a runaway recursive query.
resolveEffectiveColors is safe. It works on detached in-memory entities and issues only SELECT queries via findAllById. No unintended write path is opened.
Deduplication via Set is correct. Using Set<UUID> for extraIds and matchedIds prevents the same tag appearing twice in the result without any application-layer race conditions. UUID identity is stable.
No IDOR surface introduced. The enrichment returns ancestors/descendants of tags the user already found by searching — it does not expose new tag IDs that weren't reachable via the existing GET /api/tags?query=... endpoint.

Recommendations

No security changes required. This is a clean additive enrichment on an existing authenticated read path. No concerns from my angle.

## 🔒 Nora Steiner (@NullX) — Application Security Engineer ### Observations - **Attack surface unchanged.** No new endpoints, no new input parameters, no authorization model changes. The enrichment happens server-side on a read path that is already behind authentication. - **Input encoding already correct.** The fetch in `TagInput.svelte` uses `encodeURIComponent(query)` before constructing the URL. No injection risk in the query parameter. - **CTE depth guard already in place.** `findDescendantIds` and `findAncestorIds` both include `AND a.depth < 50` — a crafted deeply-nested tag tree cannot cause a runaway recursive query. - **`resolveEffectiveColors` is safe.** It works on detached in-memory entities and issues only `SELECT` queries via `findAllById`. No unintended write path is opened. - **Deduplication via `Set` is correct.** Using `Set<UUID>` for `extraIds` and `matchedIds` prevents the same tag appearing twice in the result without any application-layer race conditions. UUID identity is stable. - **No IDOR surface introduced.** The enrichment returns ancestors/descendants of tags the user already found by searching — it does not expose new tag IDs that weren't reachable via the existing `GET /api/tags?query=...` endpoint. ### Recommendations No security changes required. This is a clean additive enrichment on an existing authenticated read path. No concerns from my angle.

marcel commented

2026-04-17 11:19:19 +02:00

🧪 Sara Holt — QA Engineer

Observations

Test specification is well-structured — 3 backend unit tests and 4 frontend component tests are named and described in the spec. This is the right level of upfront planning.
Missing test: "orphaned children of filtered parent" (spec Section 3). When a tag is already selected (filtered out as a chip), its children appear in the dropdown as depth-0 roots. None of the 4 specified frontend tests cover this edge case. The DFS "orphan becomes root" path is one of the more surprising behaviors in the spec — it needs an explicit test.
mockFetchWithTags(tagNames: string[]) is insufficient for new tests. The existing helper creates tag objects with name only (no id, no parentId). All 4 new frontend tests need hierarchy data. Each test currently falls back to inlining vi.stubGlobal(...) (as the existing shows child suggestion after its parent test already does at line 223). Extract a mockFetchWithTagObjects(tags: Tag[]) helper to avoid 4 copies of the same vi.stubGlobal boilerplate.
Existing test update scope. The shows child suggestion after its parent when both are in results test (line 223) checks DOM ordering via [role="option"] index — this will still pass after the change. However, the spec says to "update" it, meaning it should also assert isDirectMatch styling (e.g., the parent renders with font-medium and the child renders without). Consider checking computed padding-left or a data-testid attribute to verify depth indentation.
Test 4 (keyboard Enter on context node adds it as a tag) is the highest-value test — it exercises the handleKeydown breaking change (.tag unwrap) and confirms that context ancestors are truly selectable. Write this one first; if it passes without the .tag fix, something is wrong.
Backend test naming should follow the existing snake_case convention in TagServiceTest.java. The spec names are already in this style (search_includes_children_when_root_matches) — use them verbatim.

Recommendations

Add a 5th frontend test:

it('shows orphaned children at depth 0 when their parent is already selected', async () => {
    vi.stubGlobal('fetch', vi.fn().mockResolvedValue({
        ok: true,
        json: vi.fn().mockResolvedValue([
            { id: 'p1', name: 'Briefe' },
            { id: 'c1', name: 'Familienbriefe', parentId: 'p1' }
        ])
    }));
    render(TagInput, {
        tags: [{ id: 'p1', name: 'Briefe' }],  // already selected
        allowCreation: true
    });
    const input = page.getByRole('textbox');
    await input.fill('Fa');
    await waitForDebounce();
    await expect.element(page.getByRole('option', { name: 'Familienbriefe' })).toBeInTheDocument();
    await expect.element(page.getByRole('option', { name: 'Briefe' })).not.toBeInTheDocument();
});

Backend deduplication test should mock a root tag appearing in both matched and findDescendantIds result — that's the actual deduplication path (see Felix's note on findAncestorIds seed discrepancy).
Keep backend tests at @ExtendWith(MockitoExtension.class) level — no @SpringBootTest needed. The existing TagServiceTest setup is correct and should be followed exactly.

## 🧪 Sara Holt — QA Engineer ### Observations - **Test specification is well-structured** — 3 backend unit tests and 4 frontend component tests are named and described in the spec. This is the right level of upfront planning. - **Missing test: "orphaned children of filtered parent" (spec Section 3).** When a tag is already selected (filtered out as a chip), its children appear in the dropdown as depth-0 roots. None of the 4 specified frontend tests cover this edge case. The DFS "orphan becomes root" path is one of the more surprising behaviors in the spec — it needs an explicit test. - **`mockFetchWithTags(tagNames: string[])` is insufficient for new tests.** The existing helper creates tag objects with `name` only (no `id`, no `parentId`). All 4 new frontend tests need hierarchy data. Each test currently falls back to inlining `vi.stubGlobal(...)` (as the existing `shows child suggestion after its parent` test already does at line 223). Extract a `mockFetchWithTagObjects(tags: Tag[])` helper to avoid 4 copies of the same `vi.stubGlobal` boilerplate. - **Existing test update scope.** The `shows child suggestion after its parent when both are in results` test (line 223) checks DOM ordering via `[role="option"]` index — this will still pass after the change. However, the spec says to "update" it, meaning it should also assert `isDirectMatch` styling (e.g., the parent renders with `font-medium` and the child renders without). Consider checking computed `padding-left` or a `data-testid` attribute to verify depth indentation. - **Test 4 (`keyboard Enter on context node adds it as a tag`) is the highest-value test** — it exercises the `handleKeydown` breaking change (`.tag` unwrap) and confirms that context ancestors are truly selectable. Write this one first; if it passes without the `.tag` fix, something is wrong. - **Backend test naming** should follow the existing `snake_case` convention in `TagServiceTest.java`. The spec names are already in this style (`search_includes_children_when_root_matches`) — use them verbatim. ### Recommendations - **Add a 5th frontend test:** ```typescript it('shows orphaned children at depth 0 when their parent is already selected', async () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: true, json: vi.fn().mockResolvedValue([ { id: 'p1', name: 'Briefe' }, { id: 'c1', name: 'Familienbriefe', parentId: 'p1' } ]) })); render(TagInput, { tags: [{ id: 'p1', name: 'Briefe' }], // already selected allowCreation: true }); const input = page.getByRole('textbox'); await input.fill('Fa'); await waitForDebounce(); await expect.element(page.getByRole('option', { name: 'Familienbriefe' })).toBeInTheDocument(); await expect.element(page.getByRole('option', { name: 'Briefe' })).not.toBeInTheDocument(); }); ``` - **Backend deduplication test** should mock a root tag appearing in both `matched` and `findDescendantIds` result — that's the actual deduplication path (see Felix's note on `findAncestorIds` seed discrepancy). - Keep backend tests at `@ExtendWith(MockitoExtension.class)` level — no `@SpringBootTest` needed. The existing `TagServiceTest` setup is correct and should be followed exactly.

marcel commented

2026-04-17 11:19:36 +02:00

🎨 Leonie Voss — UI/UX Designer

Observations

Spec quality is high. The 55%-scale mockups with before/after states and an implementation reference table (§5) are well-formed. The visual decisions — › prefix, 50% dot opacity for context nodes, depth * 16 + 12 px indentation formula — are all clearly reasoned.
text-ink-3 token needs verification. The spec calls for text-ink-3 for context nodes, but the existing TagInput.svelte uses text-ink-2 for non-active suggestions. I don't see text-ink-3 used anywhere in the current TagInput. Verify it's defined in layout.css before implementing — if it's missing, add it as: --color-ink-3: #6b7280 (matching the hex shown in the spec's impl-ref table). Using an undefined Tailwind class silently falls back to the default color, which would make context nodes visually indistinguishable from direct matches.
role="listbox" is missing from the <ul>. The dropdown uses role="option" on each <li> but the parent <ul> has no role="listbox". This violates WCAG 1.3.1 (Info and Relationships) — screen readers won't understand the list structure. This is an existing bug; any touch of this file is the right moment to fix it:
```
<ul role="listbox" aria-label={m.comp_taginput_suggestions_label()}
 class="absolute top-full ...">
```
(Add a new i18n key for the aria-label, or use a generic "Suggestions" label.)
The › prefix glyph will be announced by screen readers. The spec renders it as a plain text character inside a . Screen readers will read it as "right-pointing angle quotation mark" or similar. Add aria-hidden="true" to that span:
```
{#if !isDirectMatch && depth === 0}
 ›
{/if}
```
The hierarchy context is already conveyed by indentation and position — the › is a decorative visual cue, not semantic content.
Color dot opacity at 50% for context nodes. At h-2 w-2 (8×8px), a 50%-opacity dot on a white background may be hard to distinguish for users with low vision. The spec accepts this tradeoff — noted but not blocking. If it causes complaints from the senior audience (60+), consider opacity-75 as a compromise.
Touch targets on dropdown items. The existing py-2 class gives ~32px item height, below the 44px WCAG 2.2 minimum. This is an existing limitation of the dropdown density — acceptable for a desktop-focused input. Not introduced by this change.

Recommendations

Before implementing: Confirm text-ink-3 is in layout.css. If not, add --color-ink-3: #6b7280 and register it as a Tailwind utility.
Add role="listbox" to the <ul> — this is a one-line fix for a real accessibility violation.
Add aria-hidden="true" to the › prefix span — prevents screen readers from announcing decorative punctuation.
No new i18n keys required — the spec explicitly confirms this. All existing comp_taginput_* keys remain applicable.

## 🎨 Leonie Voss — UI/UX Designer ### Observations - **Spec quality is high.** The 55%-scale mockups with before/after states and an implementation reference table (§5) are well-formed. The visual decisions — `›` prefix, 50% dot opacity for context nodes, `depth * 16 + 12` px indentation formula — are all clearly reasoned. - **`text-ink-3` token needs verification.** The spec calls for `text-ink-3` for context nodes, but the existing `TagInput.svelte` uses `text-ink-2` for non-active suggestions. I don't see `text-ink-3` used anywhere in the current TagInput. Verify it's defined in `layout.css` before implementing — if it's missing, add it as: `--color-ink-3: #6b7280` (matching the hex shown in the spec's impl-ref table). Using an undefined Tailwind class silently falls back to the default color, which would make context nodes visually indistinguishable from direct matches. - **`role="listbox"` is missing from the `<ul>`.** The dropdown uses `role="option"` on each `<li>` but the parent `<ul>` has no `role="listbox"`. This violates WCAG 1.3.1 (Info and Relationships) — screen readers won't understand the list structure. This is an existing bug; any touch of this file is the right moment to fix it: ```svelte <ul role="listbox" aria-label={m.comp_taginput_suggestions_label()} class="absolute top-full ..."> ``` (Add a new i18n key for the `aria-label`, or use a generic "Suggestions" label.) - **The `›` prefix glyph will be announced by screen readers.** The spec renders it as a plain text character inside a ``. Screen readers will read it as "right-pointing angle quotation mark" or similar. Add `aria-hidden="true"` to that span: ```svelte {#if !isDirectMatch && depth === 0} › {/if} ``` The hierarchy context is already conveyed by indentation and position — the `›` is a decorative visual cue, not semantic content. - **Color dot opacity at 50% for context nodes.** At `h-2 w-2` (8×8px), a 50%-opacity dot on a white background may be hard to distinguish for users with low vision. The spec accepts this tradeoff — noted but not blocking. If it causes complaints from the senior audience (60+), consider `opacity-75` as a compromise. - **Touch targets on dropdown items.** The existing `py-2` class gives ~32px item height, below the 44px WCAG 2.2 minimum. This is an existing limitation of the dropdown density — acceptable for a desktop-focused input. Not introduced by this change. ### Recommendations 1. **Before implementing:** Confirm `text-ink-3` is in `layout.css`. If not, add `--color-ink-3: #6b7280` and register it as a Tailwind utility. 2. **Add `role="listbox"`** to the `<ul>` — this is a one-line fix for a real accessibility violation. 3. **Add `aria-hidden="true"`** to the `›` prefix span — prevents screen readers from announcing decorative punctuation. 4. **No new i18n keys required** — the spec explicitly confirms this. All existing `comp_taginput_*` keys remain applicable.

marcel commented

2026-04-17 11:19:42 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

No infrastructure changes required. Pure application feature — no new services, no new Docker Compose entries, no environment variables, no CI pipeline changes.
N CTE queries per search stay server-side. No new external service calls, no MinIO interactions, no async jobs. The extra 2–4 queries per search are PostgreSQL CTEs that run inside the existing connection pool.
No observability gap introduced. The existing request latency metrics on GET /api/tags will naturally capture any slowdown if this becomes a bottleneck at scale. No new instrumentation needed for this feature.
CI is fully unaffected. Backend unit tests (@ExtendWith(MockitoExtension.class)) and frontend component tests (vitest-browser-svelte) both run in the existing CI jobs without changes.

No concerns from my angle. Clean implementation that fits squarely within the existing deployment footprint.

## 🚀 Tobias Wendt — DevOps & Platform Engineer ### Observations - **No infrastructure changes required.** Pure application feature — no new services, no new Docker Compose entries, no environment variables, no CI pipeline changes. - **N CTE queries per search stay server-side.** No new external service calls, no MinIO interactions, no async jobs. The extra 2–4 queries per search are PostgreSQL CTEs that run inside the existing connection pool. - **No observability gap introduced.** The existing request latency metrics on `GET /api/tags` will naturally capture any slowdown if this becomes a bottleneck at scale. No new instrumentation needed for this feature. - **CI is fully unaffected.** Backend unit tests (`@ExtendWith(MockitoExtension.class)`) and frontend component tests (`vitest-browser-svelte`) both run in the existing CI jobs without changes. No concerns from my angle. Clean implementation that fits squarely within the existing deployment footprint.

marcel commented

2026-04-17 11:59:06 +02:00

✅ Implementation complete — branch `feat/issue-250-tag-typeahead-tree-aware`

What was implemented

Backend — TagService.java

Extracted private enrichWithRelatives(List<Tag> matched) helper; search() now calls it when results are non-empty
Root matches expand descendants via findDescendantIds; child matches prepend ancestors via findAncestorIds
Deduplication via extraIds.removeAll(matchedIds) (handles findDescendantIds seed inclusion)
resolveEffectiveColors called on the merged result list
4 new unit tests in TagServiceTest: search_includes_children_when_root_matches, search_includes_ancestors_when_child_matches, search_deduplicates_when_root_also_in_descendant_result, search_callsResolveEffectiveColors_onAllResults

Frontend — TagInput.svelte

Introduced SuggestionEntry type ({ tag, depth, isDirectMatch })
Replaced orderedSuggestions with recursive DFS that computes depth per entry; isDirectMatch derived from fetchedForQuery state
Removed dead suggestionsById derived
Depth indentation via inline style: depth * 16 + 12 px
Direct matches: font-medium text-ink-2; context nodes: text-ink-3
Root-level context ancestors get › prefix with aria-hidden="true"
Color dot in dropdown: 100% opacity for direct matches, 50% for context nodes
Fixed handleKeydown and onclick to unwrap .tag from SuggestionEntry
Added role="listbox" to the <ul> (Leonie's accessibility fix)

Frontend tests — TagInput.svelte.spec.ts

Added mockFetchWithTagObjects helper
5 new tests: 3-level DFS order, ancestor above child match, Enter on context node, orphaned children at depth 0 (Sara's 5th test)
Updated existing shows child suggestion after its parent to also verify font-medium on direct match

Commits

d075bf3 — feat(tag-search): expand children and surface ancestor path in search results
5120dd1 — feat(tag-input): tree-aware DFS ordering, depth indentation, and direct-match styling

All 1013 backend tests and 947 frontend tests pass ✅

## ✅ Implementation complete — branch `feat/issue-250-tag-typeahead-tree-aware` ### What was implemented **Backend — `TagService.java`** - Extracted private `enrichWithRelatives(List<Tag> matched)` helper; `search()` now calls it when results are non-empty - Root matches expand descendants via `findDescendantIds`; child matches prepend ancestors via `findAncestorIds` - Deduplication via `extraIds.removeAll(matchedIds)` (handles `findDescendantIds` seed inclusion) - `resolveEffectiveColors` called on the merged result list - 4 new unit tests in `TagServiceTest`: `search_includes_children_when_root_matches`, `search_includes_ancestors_when_child_matches`, `search_deduplicates_when_root_also_in_descendant_result`, `search_callsResolveEffectiveColors_onAllResults` **Frontend — `TagInput.svelte`** - Introduced `SuggestionEntry` type (`{ tag, depth, isDirectMatch }`) - Replaced `orderedSuggestions` with recursive DFS that computes depth per entry; `isDirectMatch` derived from `fetchedForQuery` state - Removed dead `suggestionsById` derived - Depth indentation via inline style: `depth * 16 + 12` px - Direct matches: `font-medium text-ink-2`; context nodes: `text-ink-3` - Root-level context ancestors get `›` prefix with `aria-hidden="true"` - Color dot in dropdown: 100% opacity for direct matches, 50% for context nodes - Fixed `handleKeydown` and `onclick` to unwrap `.tag` from `SuggestionEntry` - Added `role="listbox"` to the `<ul>` (Leonie's accessibility fix) **Frontend tests — `TagInput.svelte.spec.ts`** - Added `mockFetchWithTagObjects` helper - 5 new tests: 3-level DFS order, ancestor above child match, Enter on context node, orphaned children at depth 0 (Sara's 5th test) - Updated existing `shows child suggestion after its parent` to also verify `font-medium` on direct match ### Commits - `d075bf3` — feat(tag-search): expand children and surface ancestor path in search results - `5120dd1` — feat(tag-input): tree-aware DFS ordering, depth indentation, and direct-match styling All 1013 backend tests and 947 frontend tests pass ✅

marcel referenced a pull request that will close this issue

2026-04-17 11:59:26 +02:00

feat(tag-typeahead): tree-aware results — expand children & surface ancestor path #251

marcel closed this issue

2026-04-17 12:25:56 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#250

feat(tag-typeahead): tree-aware results — expand children & surface ancestor path #250

Summary

Design spec

Behaviour

Scope

Backend — TagService.java

Frontend — TagInput.svelte

Tests

Files to change

🏗️ Markus Keller — Senior Application Architect

Observations

Recommendations

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

Recommendations

🔒 Nora Steiner (@NullX) — Application Security Engineer

Observations

Recommendations

🧪 Sara Holt — QA Engineer

Observations

Recommendations

🎨 Leonie Voss — UI/UX Designer

Observations

Recommendations

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

✅ Implementation complete — branch feat/issue-250-tag-typeahead-tree-aware

What was implemented

Commits

Backend — `TagService.java`

Frontend — `TagInput.svelte`

✅ Implementation complete — branch `feat/issue-250-tag-typeahead-tree-aware`