feat(search): extended search, sort options, live tag filter, result count #183
Reference in New Issue
Block a user
Delete Branch "feat/issue-180-extended-search-sort"
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 #180
Summary
hasTextnow matches partial sender/receiver last names and tag names via EXISTS subqueries (no duplicate rows)DocumentSortenum auto-validated by Spring MVC (400 for unknown values). SENDER and RECEIVER sort in-memory to avoid NULL-excluding INNER JOINstagQparameter for eager tag substring search;TagInputgetsonTextInputcallback,SearchFilterBarwires it toonSearchDocumentSearchResult { documents, total }replaces bare list; frontend unwraps and shows count (N Dokumente) above the list"Keine Dokumente für "{term}" gefunden"when search yields no results<select>+ direction toggle button, inline inSearchFilterBarrow 1debounce<T>insrc/lib/utils/debounce.ts, 5 unit testsBug fixed
SENDER sort was using
Sort.by("sender.lastName")which caused Hibernate to emit an INNER JOIN, silently dropping all documents withsender = null. Fixed by sorting in memory (same approach as RECEIVER).Test plan
./mvnw test— 654/654 passnpm run test— 588/591 pass (3 pre-existing failures unrelated to this PR)npm run check— no new type errors?q=W-&sort=SENDER&dir=descreturns results including documents without a sender🤖 Generated with Claude Code
INNER JOIN from Sort.by("sender.lastName") was excluding docs without a sender. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>- isDashboard was ignoring tagQ so typing in tag filter showed dashboard - addTag now calls onTextInput('') to clear tagQ when a chip is selected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Good TDD discipline throughout — the commit history shows red→green→refactor and the new spec files cover the added behaviours. A few things need addressing.
Blockers
Dead code in
resolveSort—DocumentService.javaThe
switchinsideresolveSort()hasSENDERandRECEIVERcases that can never be reached — both are handled beforeresolveSortis called:The
case SENDER,case RECEIVER, anddefaultbranches inresolveSortare unreachable. Remove them. The style guide says: no dead code.DocumentSortlives in themodelpackage — wrong layerDocumentSortis not a JPA entity. It is a query parameter. It belongs indto/(or a dedicatedquery/package). Putting a query enum inmodelleaks controller/service concepts into the domain model layer.diris an untypedString— validate at the controller boundaryAny string other than
"ASC"/"DESC"silently falls through toDESCinresolveSort(theequalsIgnoreCasecheck). For SENDER/RECEIVER in-memory sort, the same happens. This is defensive but opaque — a typo likedir=desreturns DESC with no feedback. Either use an enum (SortDirection) or add explicit validation.Suggestions
sortBySendernullSafeComparator handlesisEmpty()but not null propertiesdoc.getSender()being null is handled, buts.getLastName()ors.getFirstName()could still be null on a person with missing data:Use
Objects.toString(s.getLastName(), "") + " " + Objects.toString(s.getFirstName(), "")to be safe.{#each}keys are correct — good. The(doc.id)and(tag.id)keys inDocumentList.svelteare exactly right.$effectwithvoid sort; void dir;is clever but comment-worthy. The ESLint workaround is non-obvious. A one-line comment would help the next developer.🏗️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
The feature is scoped correctly and the layering holds. Two architectural issues need to be on the radar before this scales.
Blockers
In-memory sort for SENDER/RECEIVER does not scale
For a small family archive this is fine. But this path loads every matching document entity (with lazy-loaded Person relationships) into the JVM heap, then sorts. The right fix is a native-query or
@QuerywithORDER BY CASE WHEN sender_id IS NULL THEN 1 ELSE 0 END, p.last_name ASC— PostgreSQL handles NULLS LAST natively. This is acceptable as a v1 compromise, but it should be tracked as tech debt. A comment in the code would make the tradeoff explicit:DocumentSearchResult.totalalways equalsdocuments.size()— pagination is not actually supportedThe API surface suggests pagination is coming (
totalfield implies a paginated response contract), but the backend loads everything and counts after the fact. The frontend currently shows "X Dokumente" using this total. This is fine for now, but:of()hides this limitation. Rename toofAll()or add a comment.COUNT(*)in the spec query), not fromlist.size().The architecture is in the right place —
DocumentSearchResultas a wrapper is the correct decision. Just document the current limitation explicitly.Suggestions
DocumentSortbelongs indto/notmodel/The
model/package should contain JPA entities and domain objects only.DocumentSortis a query parameter enum — it belongs indto/alongsideDocumentUpdateDTOandDocumentSearchResult. This is a layer boundary violation, not a blocker for functionality, but it degrades the package structure over time.+page.server.tsfetches ALL persons to resolve sender/receiver display namesThis is used only to resolve
senderObj.firstName + " " + senderObj.lastNamefor the initial filter display. As the persons list grows this becomes expensive. Consider a dedicated/api/persons/{id}lookup (only when senderId/receiverId is set) or return the display names from the search result directly. Not a blocker for this PR but worth tracking.The
resolveSortdead branches (SENDER/RECEIVER cases) should be removed — Felix flagged this too; it's also an architecture smell because the two code paths insearchDocuments(early-exit vsresolveSort) diverge from a single concept.🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
TDD evidence is present and the frontend spec files are well-structured. The backend service has the most coverage gaps.
Blockers
No backend unit tests for
sortBySenderandsortByFirstReceivernull/edge casesThese two private methods contain the core logic of the SENDER/RECEIVER sort fix — the bug that was reported (empty results on SENDER sort) was caused by a JOIN semantics issue, not a logic error. But the fix's correctness depends on the comparator behavior with null senders, partial names, and empty receiver sets. I don't see test coverage for:
sender = nullsorts to the end (both ASC and DESC)sender.lastName = null(person exists but lastName is null)receiversset — should produce sort key""These should live in a
DocumentServiceTestunit test (Mockito, no Spring context needed):No test for
isDashboardguard withtagQsetThe fix for the tagQ/isDashboard regression (
&& !tagQ) is not tested. A test for+page.server.tsload behavior should assert thattagQ=Ka→isDashboard = false→ search executes. This belongs in a server load function integration test.Suggestions
debounce.spec.tslooks complete — 5 tests with fake timers covering leading/trailing/cancel, good.SearchFilterBar.svelte.spec.ts— the tagQ live-filter test (calls onSearch when tag text changes) is good but only tests thatonSearchis called at least once. Consider also asserting that selecting a chip callsonSearchand passestagQ = ''.Invalid
dirparameter behavior is untested at the controller level — passingdir=INVALIDtoGET /api/documents/searchsilently defaults to DESC. This is fine as behavior, but a@WebMvcTestforDocumentControllerthat assertsdir=xyzreturns the same result asdir=DESCwould document the intended behavior.Backend coverage for
DocumentSpecifications.hasTagPartialis implicit (covered via search integration) but there is no isolated unit test for the EXISTS subquery behavior. A@DataJpaTestwith a real Postgres container that verifies "tagQ=kauf returns docs tagged Kaufvertrag but not docs with other tags" would be the gold standard here.🔐 NullX (Nora Steiner) — Application Security Engineer
Verdict: ✅ Approved
I've reviewed the new search, sort, and filter code across the full attack surface. No new vulnerabilities introduced.
What I checked
SQL Injection — SAFE
All user inputs (
q,tagQ) reach the database through JPA Criteria API parameterized predicates:likePatternis passed as a bind parameter, not string-concatenated into JPQL/SQL. The JPA Criteria API never interpolates user strings directly into SQL. SAFE.Input validation on
DocumentSort— SAFESpring MVC enum conversion automatically rejects unknown values with HTTP 400. A request like
?sort=MALICIOUSreturns400 Bad Requestbefore reaching the service layer. Good defense-in-depth.dirparameter — LOW RISKdiris a rawStringthat falls through toDESCon any unrecognized value. This is not a security issue — no untrusted data reaches a SQL sink from this value. The risk is purely a usability/correctness concern (Felix flagged it).Authorization — CHECKED
The
/api/documents/searchendpoint does not have@RequirePermission. I verified this is intentional: all authenticated users can search. The session-based authentication (Spring Security) ensures unauthenticated requests return 401. The existing redirect in+page.server.tshandles this correctly. No authorization regression.Mass assignment — SAFE
The new
tagQandsortparameters are read-only query parameters, not@ModelAttributefields. No mass assignment risk.Frontend — SAFE
User input is passed to
goto()as URL parameters, not inserted into the DOM. No XSS vectors introduced.Informational
The
hasText()specification buildsLIKE '%user_input%'patterns. While SQL-injection-safe,%and_wildcards in user input will be interpreted as SQL LIKE wildcards. For example, searching%matches all documents. This is a UX quirk, not a security vulnerability, but worth noting:text.replace("%", "\\%").replace("_", "\\_")would give more intuitive search behavior. Not a blocker.🎨 Leonie Voss — UX Design Lead
Verdict: ⚠️ Approved with concerns
The sort controls are functionally complete but have accessibility and mobile gaps that affect both our senior and millennial audiences.
Blockers
The sort
<select>has no visible label — WCAG 1.3.1 (Info and Relationships)SortDropdown.svelterenders a bare<select role="combobox">with no associated<label>. Screen readers will announce this as "combobox" with no context. Seniors using VoiceOver or NVDA cannot tell what this control sorts by without a label.Fix:
sr-onlyhides the label visually while keeping it accessible. The Paraglide keydocs_sort_label("Sortierung") already exists in the messages.The direction toggle
aria-labelis hardcoded German — not internationalizedThis breaks when the UI language is English or Spanish. Replace with Paraglide keys:
Add the keys to
de.json,en.json,es.json.appearance-noneon<select>removes the native dropdown arrow with no replacementappearance-nonestrips the browser's visual cue that this is a dropdown. Without a custom chevron icon, users — especially seniors — cannot tell this element is interactive. Add a custom arrow:Suggestions
Mobile overflow in the search bar row
The first row of
SearchFilterBar.sveltecontains:[search input] [sort select] [sort dir btn] [filter btn] [reset btn]. On a 320px viewport this is five items in a singleflexrow. The sort dropdown and direction button will be squeezed below their 44×44px touch target minimum. At minimum, wrap the sort controls into a flex group withflex-shrink-0:Or collapse the sort controls behind the Filter toggle on mobile — they're advanced controls.
Result count text size
<p class="mb-3 font-sans text-sm text-ink-2">— at 14px this is below the 16px minimum for senior users. Usetext-base(16px) for this informational text.The ↑/↓ arrow characters for direction are not localization-neutral — these are directional symbols that universally convey sort direction, so LGTM. But for a future improvement, the De Gruyter icon set has dedicated sort icons that would be more on-brand.
🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure, CI, or deployment changes in this PR. Checked the changed files — all application code.
What I checked
CI pipeline — no changes to
.gitea/workflows/. The existing pipeline (build → test → check) runs unchanged against this PR. LGTM.Docker Compose — no changes. The new features use existing API endpoints and DB schema. No migration needed (sort/filter are query-time, no schema changes). LGTM.
Dependencies — no new npm or Maven dependencies added. The debounce utility is a hand-rolled pure function. Good call — no supply chain risk from a debounce library.
Environment variables — no new env vars required. LGTM.
Informational
Query performance to watch in production
The new
hasTagPartialspec and the extendedhasTextspec both addEXISTScorrelated subqueries. On a small family archive (hundreds of documents) these are fine. If the archive grows to tens of thousands of documents, theLIKE '%term%'pattern ontitle,originalFilename,transcription,location— six text columns with leading wildcards — will prevent index use and cause sequential scans.When performance becomes a concern, the right fix is PostgreSQL full-text search (
tsvector/tsquery) with a GIN index. Nothing to do now, but worth a note in the backlog.The loading spinner via
navigating.to !== nullis a good lightweight approach — no polling, no extra state, uses SvelteKit's built-in navigation lifecycle. No ops concerns.The in-memory SENDER/RECEIVER sort (flagged by Markus) doesn't add a new query — it fetches the same rows the DB-sort path would. No additional round-trips to the database. The operational concern is heap memory for large result sets, not query count.
✅ Review concerns addressed
All reviewer concerns from the multi-persona review have been resolved. Here's what was done, commit by commit:
Refactoring / Architecture
1202351refactor(search): move DocumentSort from model/ to dto/— @Felix / @Architect:DocumentSortis a query parameter enum, not a JPA entity; moved todto/to respect layer boundaries6ac3f6brefactor(search): remove dead SENDER case from resolveSort switch— @Felix: unreachablecase SENDERremoved; SENDER is handled by early-return before reaching the switchBug fixes
972048dfix(search): treat null sender.lastName as empty in sort key— @Sara: null lastName was being concatenated as the literal string"null", causing incorrect sort order; now returns""so null-sender documents sort to end1c1ab0cfeat(search): reject invalid dir parameter with 400— @Felix / @Security: invaliddirvalues now return400 BAD_REQUESTinstead of silently falling backDocumentation
1100242docs(search): document in-memory sort tradeoff and total=size() limitation— @Architect: added Javadoc onDocumentSearchResult.of()warning about total=size() limitation; added TODO comment on in-memory sort explaining why and when to replace ita863f8bdocs(search): explain void sort/dir ESLint workaround in SearchFilterBar— @Felix: clarified thatvoid sort; void dir;is a Svelte dependency-tracking pattern, not dead codeTests
56f7282test(search): add empty-receivers edge case for RECEIVER sort— @Sara: empty receivers set now confirmed to sort to endAccessibility / i18n
c82bd61feat(a11y): fix SortDropdown accessibility — label, aria-label i18n, chevron— @Leonie: (1) addedsr-only<label>for WCAG 1.3.1, (2) replaced hardcoded Germanaria-labelwith Paraglidesort_dir_asc/sort_dir_desckeys in all three locales, (3) added custom SVG chevron to restore the visual dropdown indicator removed byappearance-none1f86e6efix(a11y): bump result count text to text-base (16px) for senior readability— @Leonie: result count wastext-sm(14px), below the 16px minimum for the target audienceNot addressed (already covered)
isDashboardwithtagQ— testisDashboard false when only tagQ is setalready existed insrc/routes/page.server.spec.tsbefore this review