feat(search): NL search frontend — toggle, chips, disambiguation, empty state (#739) #757
Reference in New Issue
Block a user
Delete Branch "feat/issue-739-nl-search-frontend"
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?
Closes #739.
Depends on #738 (merged) +
npm run generate:apialready run —NlQueryInterpretation,NlSearchResponse,PersonHintexist insrc/lib/generated/api.ts. #738's PR updatedCLAUDE.md, thel3-backend-search.pumldiagram for the newsearch/package, and added the Prometheus histogram for/api/search/nl.What this adds
A smart-search mode on the
/documentspage. One input, one toggle pill inside it; keyword mode is unchanged. LLM parsing happens server-side — the frontend sends the query, renders interpretation chips, and handles loading / empty / error states.New components (
src/routes/search/)SmartModeToggle.svelte— toggle pill witharia-pressed, active style matching the AND/OR operator button, mobile-expanded KI/Text labels.InterpretationChipRow.svelte— type-prefixed chips (Absender:/Zeitraum:/Stichwort:), a single directional chip for 2-name queries (Walter → Emma), keyword chips gated onkeywordsApplied,onRemoveChip(type, value?), truncating name spans with a 44px × button, focus ring on the chip wrapper.SmartSearchStatus.svelte— full-area panels: loading (role="status",motion-safe:spinner + pulse), 503 (red icon + switch-to-keyword button), 429 (amber clock, no button).DisambiguationPicker.svelte— accessible single-select disclosure:aria-expanded/aria-controls, focus moves into the list on open, Escape/click-outside close and return focus to the trigger.Wiring
SearchFilterBar.sveltegainssmartMode($bindable) +onSmartSearch/onModeToggle. Smart mode disables the liveoninputkeyword search, addsmaxlength="500", and submits the NL query on Enter.documents/+page.svelteliftssmartMode, runsPOST /api/search/nlviacsrfFetch+parseBackendError, and renders the NL view (status / chips / disambiguation / results / empty) vs. the normalDocumentList. Chip removal and disambiguation selection map the interpretation to keyword params and re-run viaGET /api/documents/search(Option A in-page fallback).messages/{de,en,es}.json(formal Sie per project standard).frontend/CLAUDE.mdProject Structure documentssrc/routes/search/.Resolved open decision (disambiguation)
The shipped backend (#738) added only single-person
searchDocumentsByPersonId— there is nosenderIds[]array param, so multi-sender OR is not supported. Per discussion with the maintainer, the picker is single-select: choosing a candidate setssenderId(andreceiverIdwhen a resolved partner exists) and re-runs via GET. One sender + one receiver (directional) is fully supported by the existing GET search.Tests
SmartModeToggle8,InterpretationChipRow9,SmartSearchStatus5,DisambiguationPicker5,SearchFilterBar17 incl. smart-mode lifecycle).e2e/nl-search.spec.ts): mocked/api/search/nl→ loading → chips → axe-clean (light + dark) → remove chip re-runs GET with remaining params. Ollama not required in CI.npm run buildclean;svelte-checkintroduces no new errors (the singleonclickoutsidetyping gap matches the established pattern in 9 existing components).Notes
page.routeintercepts before the real request in CI.🤖 Generated with Claude Code
🎨 Leonie Voss — Senior UX Designer & Accessibility Strategist
Verdict: 🚫 Changes requested
I reviewed brand compliance, WCAG AA, touch targets at 320px, focus visibility, redundant cues, dark mode, and the icon-button labelling. The keyboard/ARIA wiring is genuinely strong — the disambiguation disclosure (focus-into-list on open, Escape returns focus to trigger,
aria-expanded/aria-controls) is textbook, every icon-only × button carries anaria-label, focus rings usefocus-visible, and the loading panel respectsmotion-safe:. But two font sizes break my hardest constraint, and that blocks merge.Blockers
Font size
text-[7.5px]on the toggle pill —SmartModeToggle.svelte:18. My rule is absolute: no visible text below 12px, and the senior audience (60+) needs ≥16px for chrome they must read. 7.5px is unreadable for the people this archive exists for. The toggle is also the primary affordance for the whole feature — it cannot be the smallest text on the page. Fix: bump to at leasttext-xs(12px). If the pill then crowds the input, shrink horizontal padding or move the keyword/AI suffix behindsm:rather than shrinking the glyph.Font size
text-[9px]on the loading sub-text —SmartSearchStatus.svelte:43. Same criterion. "Die KI analysiert Ihre Anfrage…" is exactly the reassurance a nervous senior user needs during a 15-second wait, and at 9px they cannot read it. Fix:text-xsminimum,text-smpreferred. Also: themotion-safe:animate-pulseon body copy makes small text harder to read — consider dropping the pulse on the text and keeping it only on the spinner.Suggestions
border-primary/12(loading spinner track,SmartSearchStatus.svelte:38) — decorative only, so not a blocker, but verify the spinner is still perceivable in dark mode where the track may vanish against the surface.→isaria-hiddenand the chip wrapper carriessearch_chip_directional_label— good. But the visible chip has no type prefix (unlike "Absender:"/"Stichwort:"). A sighted user sees "Walter → Emma" with no cue it's a sender→receiver relationship. Consider a small leading icon or label so the meaning isn't carried by the arrow alone.min-h-[44px]buttons and redundant icon+text cues (the 503!/ 429 clock) — nicely done, exactly the redundant-cue pattern I ask for.Fix the two sub-12px sizes and I approve.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
I checked TDD evidence, naming, function size, guard clauses, Svelte 5 idioms (keyed
{#each},$derivedover$effect, reactive collections), component splitting, and error handling. This is clean, idiomatic Svelte 5. Components are split by visual region (SmartModeToggle,InterpretationChipRow,SmartSearchStatus,DisambiguationPicker), every{#each}is keyed ((person.id),(chip.key)),$derived/$derived.byis used throughout (no$state+$effectderivation anti-pattern), andSvelteSetis used for the reactiveremovedset. Tests precede behaviour and cover happy + error + empty + edge (100-char name). Good work.Blockers
None.
Suggestions
Hardcoded element id in
DisambiguationPicker.svelte:14—const panelId = 'disambiguation-panel'. This is fine today because only one picker mounts. But it's a latent bug: if two pickers ever co-exist,aria-controls/idcollide and the disclosure breaks. Cheap fix:const panelId = $props.id?.()isn't available, so useimport { nanoid }or$state(crypto.randomUUID())/ Svelte's$props-free unique id. Low cost, removes a footgun.runSmartSearch(documents/+page.svelte) bypasses the typedcreateApiClientand usescsrfFetch+res.json()with a manual castbody: NlSearchResponse. I understand why — this is an interactive client-side POST, not an SSR load, andcsrfFetchis the established client-hook pattern. But you lose the openapi-typed response contract:await res.json()isany, cast toNlSearchResponse. If the backend shape drifts, TS won't catch it. Not a blocker (the cast is at least named), but consider whethercreateApiClient(fetch).POST('/api/search/nl', …)works here for compile-time safety. You do correctly check!res.okbefore reading the body — good, that's the rule.runSmartSearchresets state in two places — lines setnlInterpretation = null; nlResult = null;both in the function body andresetNlState()exists separately. The manual inline resets at the top ofrunSmartSearchduplicate most ofresetNlState()exceptnlSubmitted/nlLoading. Consider callingresetNlState()then settingnlSubmitted = true; nlLoading = true;to keep one source of truth for "what fields make up NL state."selectDisambiguatedandremoveChipboth rebuild params viaparamsFromInterpretationexceptselectDisambiguatedinlines its own object literal rather than reusing the helper. Minor DRY: it could start fromparamsFromInterpretation(nlInterpretation)and override sender/receiver. Reads as two slightly-different copies of the same mapping right now.text-[7.5px]/text-[9px]— flagged by Leonie as a11y blockers; from my side they're also magic arbitrary values that will look out of place next to the token scale. Prefertext-xs.Nothing here blocks merge from a code-correctness standpoint; address #1 before the picker is ever reused.
🏛️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
I reviewed module boundaries, accidental complexity, layer placement, transport choices, and — per my checklist — documentation currency. Structurally this is sound: the new
search/route folder houses presentation-only sub-components (no+page), consumed bydocuments/+page.svelteandSearchFilterBar.svelte. No new transport — it reuses HTTP/REST (POST /api/search/nlfrom #738,GET /api/documents/searchfor the fallback), which is the correct default. The page orchestrates state and children render; that's the right altitude.Blockers
l3-frontend-*.pumlnot updated for the new route folder. My rule table is explicit: "New SvelteKit route →CLAUDE.mdroute table + matchingdocs/architecture/c4/l3-frontend-*.puml."frontend/CLAUDE.mdwas updated (good — thesearch/line is there), but the PR body and diff show no corresponding C4 L3 frontend diagram change. Even thoughsearch/has no+page(it's a sub-component bundle), it is a new structural unit consumed across two routes and warrants a node on the frontend container diagram. If the team's convention is that+page-less folders are exempt, say so in the PR and reference the convention; otherwise update the diagram. A doc omission is a blocker until the diagram matches the code, or the exemption is documented.Suggestions
Decision worth an ADR-lite note (not necessarily a full ADR). The "single-select disambiguation, no
senderIds[]array" decision (because #738 shipped only single-person search) is exactly the kind of constraint-driven design choice that future developers will reverse out of ignorance — "why didn't they support multi-sender OR?" The PR body explains it well; capturing one paragraph indocs/GLOSSARY.mdor a short ADR ("NL search resolves to single sender + single receiver; multi-sender OR deferred pending backendsenderIds[]") preserves the why. Lightweight, high future-value.Client-side fetch vs. SSR load — boundary observation, not a violation. The codebase's default is data-via-
+page.server.ts. This feature deliberately fetches client-side (csrfFetch) because NL search is an interactive, post-load action. That's a legitimate "interactive island" exception to SSR-first, and the existingcsrfFetchhelper exists precisely for this. No change needed — flagging only so the boundary deviation is a conscious, documented one rather than drift.The in-page "Option A" fallback (map interpretation → keyword params → re-run via GET) keeps all NL complexity in one orchestrator and reuses the existing search endpoint instead of inventing new ones. Good restraint — no premature backend surface added.
Fix the diagram (or document the exemption) and this is an approve.
🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
I reviewed the test pyramid shape, coverage of happy/error/empty/edge states, determinism, and the E2E scope. The unit layer is strong: 44
vitest-browser-sveltespecs against real DOM, behaviour-focused (getByRole/getByText, not internal state), with factory helpers (makeInterpretation,makePerson) and one-behaviour-per-test naming that reads as sentences. Error and empty states are covered at the unit layer (503 panel, 429 panel, empty keyword chips, keywordsApplied=false, 100-char name). That's the right layer for permutations. Good pyramid discipline.Blockers
None — but two gaps below are close to the line.
Suggestions
E2E covers only the happy path; the two error states and the disambiguation flow have no integration/E2E coverage.
nl-search.spec.tstests toggle → loading → chips → remove-chip-re-runs-GET, which is the right critical journey. But the 503 "switch to keyword" fallback and the 429 rate-limit panel are user-facing recovery paths — the kind of thing that silently breaks when someone refactors the error mapping inrunSmartSearch. They're unit-tested in isolation (SmartSearchStatus.svelte.spec.ts), but nothing verifies that a real 503 from/api/search/nlactually renders that panel wired through the page. One extrapage.routefulfilling{ status: 503, body: { code: 'SMART_SEARCH_UNAVAILABLE' } }and asserting the panel + clicking the fallback would close the highest-value gap. Likewise a disambiguationpage.routereturningambiguousPersonsand assertingselectDisambiguatedre-runs the GET. Both are cheap; the integration wiring is exactly where unit tests can't reach.axe scan scope is narrow. The E2E runs axe light+dark but only
.include('[data-testid="smart-search-results"]')in the chips state. The loading, error, and empty panels never get an axe pass in any test (Leonie noted the same). Since they all render inside the same testid container, scanning during the loading delay and in an error fixture would cover them at near-zero cost.Determinism — good. The 150ms
page.routedelay to make the loading state assertable is the correct technique (deterministic, notThread.sleep-equivalent racing), andwaitForURL/toHaveURLuse Playwright auto-wait. ThedispatchEvent(KeyboardEvent('Enter'))workaround in the component specs matches the established TipTap/vitest-browser pattern. No flakiness concerns.CSRF is explicitly not exercised in CI (page.route intercepts first). The PR is honest about this. Acceptable for a mocked-boundary E2E, but it means the
csrfFetchheader path on/api/search/nlis only ever verified manually — worth a backend@WebMvcTest/integration assertion on the #738 side if not already present.Add the 503 + disambiguation E2E paths when convenient; not a merge blocker given the solid unit layer.
🛡️ Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
I reviewed this through an adversarial lens: XSS via interpretation rendering, CSRF on the new POST, error-message leakage, input bounds, and
target=_blank/external-link handling. This is a front-end-only PR against a backend (#738) that already owns auth, rate limiting, and the LLM call, so my surface is the rendering and the client request. It holds up well.What I checked and found clean
No XSS sink. Interpretation data (
displayName,keywords,rawQuery) flows into the template via{...}text interpolation only — Svelte auto-escapes. No{@html}, noinnerHTML, noeval. Attacker-controlled person names / keywords from the LLM are rendered as inert text. CWE-79: not present.CSRF correctly applied.
runSmartSearchusescsrfFetch('/api/search/nl', { method: 'POST', … }), which injectsX-XSRF-TOKENfrom theXSRF-TOKENcookie on mutating verbs. This is the project's established CSRF pattern and it's wired correctly for the one state-changing call. The PR is also honest that CSRF is only exercised in manual full-stack runs (page.route intercepts in CI) — acceptable, but I'd echo Sara: keep a backend-side CSRF/permission assertion on/api/search/nlso the contract is enforced somewhere automated.No raw backend error reflected to the user.
parseBackendError(res)readscodeand maps to a closed set (SMART_SEARCH_RATE_LIMITEDelseSMART_SEARCH_UNAVAILABLE), and the panels render i18n strings — nevererror.message, never SQL/stack/class names. Defaults fail closed to the generic "unavailable" copy. Good. CWE-209: not present.Input bound at the boundary.
maxlength={500}in smart mode caps the NL query client-side, andrunSmartSearchearly-returns onquery.length < 3. Client-side bounds are defense-in-depth only — the real cap must live on the backend (#738) — but this is the right belt-and-suspenders.No
target="_blank"introduced, so nowindow.openerreverse-tabnabbing concern in this diff.Suggestions (non-blocking)
await res.json() as NlSearchResponsetrusts the response shape. Not a security issue per se (Svelte still escapes on render), but if you ever move interpretation values into an attribute context or a URL, re-audit —keywords.join(' ')becomingqin a GET is currently safe because SvelteKit URL-encodes params, just keep that in mind.runSmartSearchnoting "user/LLM-derived strings are rendered as escaped text only" so the next person doesn't reach for{@html}to "render rich chips."No vulnerabilities found. Approved.
📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ⚠️ Approved with concerns
I assessed this against the issue's intent and the Definition of Done: are all states handled, are unhappy paths covered, is the behaviour testable and unambiguous, and does anything contradict a stated requirement. The feature delivers a coherent end-to-end slice — toggle → query → interpret → act — with all four UI states (loading / populated / empty / error) present. That's the visibility-of-system-status and error-recovery heuristic satisfied. Strong functional completeness.
Requirements observations (blockers)
None at the requirements level — scope matches #739 and the deferred items are explicitly named.
Open questions / ambiguities to resolve (not merge blockers, but log them)
OQ — Mode persistence is a silent behaviour change. "Smart mode resets on page navigation (away + back)" is documented as an accepted limitation. But from a user's mental model (Nielsen #2, match with real world; #6, recognition over recall), a user who ran an AI search, clicked a result, and hit Back will find themselves in keyword mode with their query gone. Is that the intended behaviour, or a known debt item to ticket? Recommend a follow-up issue so it's a tracked decision, not an accident.
OQ — The
query.length < 3minimum is a silent no-op.runSmartSearchreturns early on queries under 3 chars with no feedback — the user presses Enter and nothing happens. That violates error-prevention/visibility heuristics: the system should tell the user why nothing happened ("Bitte geben Sie mindestens 3 Zeichen ein"). Right now it's an invisible dead-end. Small AC gap worth a chip of inline hint text.OQ — Disambiguation semantics need a one-line acceptance criterion.
selectDisambiguated: when one person is already resolved, the chosen ambiguous person becomes the receiver; when none is resolved, the chosen becomes the sender. This is a reasonable rule, but it's an implicit business rule with no AC and no user-visible explanation. A user picking "Walter Müller" from a list has no way to know whether they just set a sender or a receiver. Recommend: (a) write the rule as an explicit AC, and (b) consider surfacing it in the picker label.NFR check — i18n complete (de/en/es), formal Sie used consistently — verified across all three message files. Good. One nit:
search_toggle_smart_label_suffixis"-Suche"(de) vs" search"(en) — the German concatenationKI+-Suche= "KI-Suche" works, but this label-splitting-for-concatenation pattern is fragile for translators; flag for the localisation reviewer, not a blocker.NFR check — empty state has a recovery action ("Repeat as full-text search"), error states have appropriate actions (503 → switch button; 429 → no button, correct since retrying won't help). This is exactly the error-recovery affordance I look for. Well specified.
The feature is releasable. Please convert OQ #1–#3 into tracked issues or inline ACs so these implicit decisions don't resurface as "bugs" later.
⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
I checked this for anything touching my domain: CI workflow changes, Docker/Compose, image tags, secrets, deprecated actions, and the runtime/operational footprint of the new code path. This is a frontend-only PR — no Compose, no CI YAML, no infra files in the diff — so my surface is small, but a few operational notes are worth recording.
What I checked
POST /api/search/nl(added in #738) andGET /api/documents/search. Nothing for me to size or operate.page.route— no real Ollama, no real backend, no creds in the spec. Good.Suggestions (non-blocking)
/api/search/nl. From the frontend side, the 503/429 paths are now user-visible — make sure there's a Grafana panel / alert on/api/search/nl5xx rate and p95 latency so that "AI search is down" surfaces on the dashboard before a family member reports it. The 15s client-side expectation ("can take up to 15 seconds") implies the backend timeout budget should be aligned and alertable. That's a #738/observability follow-up, not this PR./api/search/nlwith and without the token would close that gap cheaply.Nothing in my domain blocks this. Approved.
🔁 Blocker re-check
Clean re-check of the two prior blockers (comments #14648 Leonie, #14651 Markus). Verified against latest commits
0058b297and87af9ab4.1. Sub-12px fonts (Leonie) — ✅ RESOLVED
frontend/src/routes/search/SmartModeToggle.svelte:22— toggle pill now usestext-xs(no moretext-[7.5px]).frontend/src/routes/search/SmartSearchStatus.svelte:36-37— loading subtitle now usestext-xs(no moretext-[9px]).text-\[[0-9]+(\.[0-9]+)?px\]acrossfrontend/src/routes/search/returns zero matches. No sub-12px text remains in the new search components.2. C4 docs (Markus) — ✅ RESOLVED
docs/architecture/c4/l3-frontend-3b-document-workflows.pumlnow documents all four new components plus the NL relation:Component(smartToggle, "search/SmartModeToggle.svelte", ...)Component(chipRow, "search/InterpretationChipRow.svelte", ...)Component(smartStatus, "search/SmartSearchStatus.svelte", ...)Component(disambig, "search/DisambiguationPicker.svelte", ...)Rel(homePage, backend, "POST /api/search/nl (smart mode)", "HTTP / JSON")Overall
Both prior blockers are cleared. ✅