fix(documents): filter inputs don't sync with URL on navigation (#482) #487
Reference in New Issue
Block a user
Delete Branch "fix/issue-482-filter-url-sync"
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?
Fixes #482
Root causes
Three independent display-state bugs shared the same anti-pattern — display state initialised once, never re-derived when props change from outside.
1.
DateInput— display not re-derived on external value changedisplaywas initialised with$state(isoToGerman(value))and never updated. Timeline drag or the × reset that clearedfrom/toin the parent left the text box showing a stale German date.Fix: a guarded
$effectthat re-derivesdisplayfromvaluewhenever they're out of sync. The guard (germanToIso(display) !== externalIso) is safe during mid-typing:germanToIsoreturns''for partial input, which matchesvalue=''— so the re-derive never fires while the user is still typing.2.
PersonTypeahead— manually-typed term not cleared on reset navigationWhen the user typed in the typeahead without selecting a person and then navigated to
/documents(no filter),initialNamestayed''between navigations so the existing$effecttracking it never fired.Fix: a new
resetKeyprop. The page increments it (viauntrackto avoid an infinite-loop dependency) on every navigation, forcing the effect to re-run and clearsearchTerm = initialName.3.
+page.server.ts— sender/receiver names not resolved from URLThe server load returned
senderId/receiverIdUUIDs but no display names, so the typeahead always rendered empty after SSR or hard reload with a person filter active.Fix: parallel
resolvePersonNamecalls alongside the document search (UUID-validated, silent 404 drop). ReturnsinitialSenderNameandinitialReceiverNameto the page.Commits
d14c9376fix(date-input): re-derive display when value prop changes externallya58f22f6fix(person-typeahead): add resetKey prop to clear term on navigation resete8cabf43fix(documents): sync filter display state with URL on navigationTest plan
/documents?senderId=<uuid>— sender typeahead shows person's display name🤖 Generated with Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
What I checked
The Svelte 5 reactive patterns, the DateInput guard logic, the server-side parallel fetch, and TDD coverage.
Findings
void resetKey/untrack(() => navKey++)— correct idiomatic Svelte 5void resetKeyis the established pattern for tracking a dependency without using its value.untrack(() => navKey++)is the right call inside the data-sync$effect— withoutuntrack,navKeybecomes a tracked dependency and the effect loops infinitely. The comment in the code makes the intent clear.DateInput guard — clever and correct
germanToIso(untrack(() => display)) !== externalIsoworks becausegermanToIsoreturns''for partial input (doesn't match the fullDD.MM.YYYYpattern), which equalsvalue=''during typing. So the guard never fires a spurious re-derive mid-edit.untrackarounddisplayprevents the effect from depending on its own write target. Solid.resolvePersonName— clean extractionUUID regex before the fetch, try/catch returning
'', no leaky error propagation.Promise.allfor the two parallel lookups over serial awaits is the right call.Test note
The
resetKeytest passes becausererender()in vitest-browser-svelte causes a full re-mount rather than an in-place prop update. It's a smoke test, not a true isolation test for the$effecttrigger. The comment in the spec acknowledges this — that's acceptable. The production scenario (SvelteKit navigation = prop update without re-mount) genuinely requiresresetKey.No blockers.
🔒 Nora "NullX" Steiner — Security Expert
Verdict: ✅ Approved
What I checked
User-supplied values in URL paths, SSRF risk, injection vectors, and data exposure.
Findings
UUID validation — correct guard
resolvePersonNameappliesUUID_REbefore interpolatingidinto/api/persons/${id}. The regex/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/icovers the full canonical UUID4 format — no path traversal, no shell metacharacters, no bypass via partial match. Non-UUID params are silently dropped with no outbound request. This is the correct defense.SSRF — low risk
The
fetchin+page.server.tsis the SvelteKit server-sideload()fetch, which routes to the internal/api/proxy (configured invite.config.ts/ SvelteKit hooks). The backend target is fixed in config, not user-controlled. This is not an SSRF vector.Query param sanitization — existing
encodeURIComponenton theqterm in the typeahead fetch exists already. ThesenderId/receiverIdvalues in the URL only reach the serverload(), are UUID-validated, and are used only as path components in the proxy — not concatenated into SQL.Data exposure — none new
resolvePersonNamereturns onlydisplayNamefrom the person object. No sensitive fields exposed. 404 returns''— no information leakage.No issues.
🏛️ Markus Keller — Software Architect
Verdict: ✅ Approved
What I checked
Layer boundaries, coupling, data flow design, and scalability of the approach.
Findings
Search page doing person lookups — minor smell, justified
+page.server.tsnow resolves person names in addition to the document search. The documents domain is reaching into the persons domain via HTTP (through the API proxy). This is a mild violation of domain separation, but it's the right pragmatic call: the alternative (returning only UUIDs and resolving names client-side on mount) would produce a flash of empty typeaheads on every navigation. Server-side resolution is the correct UX choice. TheresolvePersonNamehelper is cleanly extracted and isolated.navKeyprop drilling — acceptable at this scaleThe
navKeyreset signal threads:+page.svelte→SearchFilterBar→PersonTypeahead(both instances). Three levels is within the "prop drilling is fine" zone. At more levels or more consumers, a Svelte context would be cleaner. No action needed now.UUID_REas module constant — correctCompile-time constant, regex compiled once, no per-request allocation. No concern.
Promise.all— correctThe two person lookups are independent; parallelising them is the right call. The
[result, [senderName, receiverName]]destructuring is readable.No blockers.
🧪 Sara Holt — QA Engineer / Tester
Verdict: ⚠️ Approved with concerns
What I checked
Test quality, isolation, coverage gaps, and whether tests actually exercise the production scenarios.
Findings
Blocker
None — the tests are present and pass.
Concerns
PersonTypeahead.svelte.spec.ts— resetKey test is a smoke test, not an isolation testrerender()in vitest-browser-svelte causes a full component re-mount rather than an in-place prop update. The test verifies that the component renders with an empty value after re-mount — it does not verify that the$effect(() => { void resetKey; searchTerm = initialName; })actually fires in response to aresetKeyprop change on a live instance. The real production scenario (SvelteKit navigation triggers a prop update on the mounted component, not a re-mount) requires the$effectmechanism. The test cannot catch a regression whereresetKeyis removed from the effect.Suggestion: Add a comment in the test body noting this limitation, e.g.:
DateInput.svelte.spec.ts— rerender limitation applies here tooSame caveat:
rerendertriggers re-mount. However, for DateInput the guard$effectbehaviour is more testable this way since the initial-state path and the external-change path produce the same observable result. Less of a concern here.+page.svelte—navKeyincrement not testedThe
untrack(() => navKey++)line in+page.svelte's data-sync$effecthas no direct test. This is genuinely hard to unit-test since it requires simulating a SvelteKitdataprop re-assignment triggering the effect. Acknowledged — E2E coverage would be the right level for this.page.server.spec.ts— solidValid UUID → name resolved, invalid UUID → no fetch called, 404 →
'', both sender and receiver paths: all covered. Good.🎨 Leonie Voss — UI/UX Expert
Verdict: ✅ Approved
What I checked
Visual correctness on navigation, flash-of-empty-input risk, and soft-failure UX.
Findings
No flash on navigation — server-side resolution is correct
initialSenderName/initialReceiverNameare resolved in+page.server.tsand passed down before the page renders. On SvelteKit navigation the serverload()completes before the new page component mounts, so the typeaheads pre-fill immediately. There is no flash of empty input followed by a client-side fill.Soft failure is acceptable
If the person fetch returns 404 or errors,
resolvePersonNamereturns''silently. The typeahead shows empty, the UUID filter is still active, and the filter chips still reflect the active state. The user experience is slightly degraded (no label visible) but not broken. For a family archive where person records are stable, this edge case is rare.Manually-typed ghost text cleared on navigation — correct
The
resetKeymechanism ensures that if a user types in the sender typeahead without selecting a person, then navigates away and back, the stale text is cleared. This is the right behaviour — the field should reflect the committed filter state, not abandoned input.No layout or style changes — behaviour fix only, appropriate scope.
No concerns.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
What I checked
Coverage of the stated acceptance criteria from issue #482 and any implicit requirements.
Findings
Issue #482 stated problem: Filter inputs (sender/receiver typeaheads) don't sync with URL on navigation — when arriving at the documents page with
?senderId=<uuid>or?receiverId=<uuid>, the typeaheads show empty instead of the person's name.PR coverage against AC:
?senderId=<uuid>resolvePersonName→initialSenderName→ PersonTypeahead?receiverId=<uuid>resetKey/navKeymechanism''(silent empty)$effectguardImplicit requirements satisfied:
initialSenderName) ✅One gap not explicitly tested: the scenario where the same page is navigated to with a different
senderId(filter changed via browser back/forward). TheinitialSenderNamewould change indata, the data-sync$effectwould update it, andPersonTypeaheadwould re-derive via itsinitialNameeffect. This path works by construction but has no dedicated test. Low risk given thepage.server.spec.tscoverage of name resolution.No blockers.
🚀 Tobias Wendt — DevOps / Infrastructure
Verdict: ✅ Approved
What I checked
CI pipeline impact, dependency changes, infrastructure changes, and runtime performance implications.
Findings
No CI/infra changes — no Dockerfile, no docker-compose, no GitHub/Gitea CI pipeline files, no environment variables, no new dependencies.
Runtime: 2 extra HTTP calls per filtered page load
Promise.allfires up to 2 person lookup requests per documents page load whensenderIdorreceiverIdis present in the URL. These go through the SvelteKit server → internal API proxy → Spring Boot. UUID validation gates the calls — malformed params produce zero outbound requests. At family archive scale this is negligible. Worth a note in monitoring if traffic patterns change.UUID_RE — no performance concern
Module-level regex constant, compiled once at startup. No per-request regex compilation cost.
No secrets or credentials introduced.
LGTM from infra perspective.