feat(search): scope a search to multiple vaults - #245
Merged
Conversation
Search could only target one vault or all accessible vaults. This adds a multi-vault scope: pick a subset and search just those. Backend: GET /search and GET /grep accept a repeatable `vault` query param (?vault=a&vault=b). The chosen names are intersected with the caller's accessible set (a vault you can't read drops out — no leak) via `v.name = ANY()` in the metadata pre-filter, the _accessible_vault_ids vector-ACL path, and the grep SQL (which now always applies the access predicate alongside the name filter). `vault` still accepts a single string for MCP/legacy callers (normalized to a one-element list), so nothing else changes. Frontend: the /search page replaces the single-vault scope dropdown with a VaultScopePicker — a Radix checkbox dropdown (search-filterable, same pattern as SelectMenu) plus removable chips for the selection; empty = "All vaults (N)". Deep-linkable via ?v=a,b,c. searchDocs/grepDocs take string[]. Verified locally: grep ?vault=a&vault=b returns only a,b (3-vault fixture); single ?vault=a still works; backend ruff/mypy clean + 22 search unit tests; frontend design/typecheck/lint + 347 tests (incl. 3 new scope tests); visual check of the picker (trigger + chip). backend 0.9.4. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…idates + tests
PR review (code-reviewer + silent-failure-hunter both flagged) found a CRITICAL:
the table and file candidate queries in `search()` still bound the RAW `vault`
param (now a `list[str]` from the REST route) to a scalar `v.name = $1`, so a
multi-vault search that takes the source-id path (any collection/type/tags
filter, or backfill-not-ready) raised on `text = text[]`. Only the documents
branch had been switched to `v.name = ANY()`.
- Fix: tables/files branches now use `vaults` + `v.name = ANY($1)` (mirrors the
documents branch). Verified: `?q=…&type=note&vault=a&vault=b` → 200 (was 500).
- Extract `_normalize_vault_scope()` so search()/grep() share one normalization
(kills the duplicated ternary + the `vault` vs `vaults` footgun).
- Comment fixes: `_accessible_vault_ids` docstring + call-site said a named-vault
anon/admin scope returns None ("unscoped") — it returns ids (scoped); and
search.tsx no longer calls `?v=` "legacy" (it's the active scope channel).
- Don't swallow a `listVaults()` failure silently (console.error so a broken
/my/vaults is diagnosable).
Tests (the gap that let the CRITICAL through):
- backend `test_search_multi_vault_unit.py`: `_normalize_vault_scope` (str/list/
empty/blank → canonical) + `_accessible_vault_ids` ACL INTERSECTION (a vault
you can't read drops out — no leak; all-unreadable → []; admin named → ids).
- frontend interaction tests: check a vault → scope adds + re-search; remove a
chip → drops; Clear → all vaults / searchDocs(undefined). (Radix dropdown
driven via userEvent, mirroring role-select.test.tsx.)
Gates: backend ruff/mypy clean + multi-vault unit tests; frontend design/
typecheck/lint + 350 tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kwoo24-oss
added a commit
that referenced
this pull request
Jun 24, 2026
…p) (#246) No behavior change — three cosmetic cleanups surfaced by the /simplify review of PR #245's diff (4 cleanup angles: reuse, simplification, efficiency, altitude): - api.ts: drop the gratuitous IIFE in `grepDocs`; build the params with a plain block body, identical in shape to its sibling `searchDocs`. - search.tsx: drop the `vs.length ? vs : undefined` ternary at both the searchDocs/grepDocs call sites. `vaultScopeParams` already collapses `[]` to "emit zero vault params" (== undefined on the wire), so the "empty = all vaults" transport rule now lives in exactly one place (api.ts) instead of being restated at every call site. - search_service.py: collapse the duplicated 2-line shim-call comment at both `search()`/`grep()` sites to one line — `_normalize_vault_scope`'s docstring already documents the str|list|None → canonical contract. Tests updated to assert the now-array-native no-scope call (`[]` instead of `undefined`); behavior is identical (both produce an unscoped query). Deliberately NOT done (agents agreed these are right as-is / out of scope for a cleanup pass): generalizing VaultScopePicker into a ui/ multi-select primitive (N=1 caller), extracting the SelectMenu menu-shell fork (defer until a 3rd dropdown), factoring the 3-place `v.name = ANY()` filter (independent param lists), memoizing the picker's per-render Set (negligible), and the VaultChip-in-chips visual change (behavior, not cleanup). Verified: frontend design:check + typecheck + lint (0 errors) + the two search suites (10/10); backend ruff + test_search_multi_vault_unit (5/5). Co-authored-by: kwoo24-oss <279600312+kwoo24-oss@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Search could only target one vault or all accessible vaults. This adds a multi-vault scope — pick a subset and search just those.
Asked for via
/ui-ux-pro-max; design chosen: Popover + checkbox list + removable chips (Option A).Backend
GET /searchandGET /grepnow accept a repeatablevaultquery param (?vault=a&vault=b). The chosen names are intersected with the caller's accessible set — a vault you can't read simply drops out (no leak) — viav.name = ANY($)in:search_service.search),_accessible_vault_ids, nowWHERE name = ANY($) AND <acl>),grepSQL, which now always applies the access predicate alongside the name filter (closing the old "named vault skips ACL" gap).vaultstill accepts a single string for MCP / legacy callers (normalized to a one-element list internally), so nothing else changes.Frontend
The
/searchpage replaces the single-vault scope dropdown with aVaultScopePicker— a Radix DropdownMenu checkbox list (search-filterable, the same pattern asSelectMenubutCheckboxItem+ stay-open) plus removable chips for the current selection. Empty = "All vaults (N)" (the calm default). Deep-linkable via?v=a,b,c.searchDocs/grepDocstakestring[].Applied UX rules: progressive-disclosure (popover), autocomplete/search-accessible (filter), color-not-only + visual-hierarchy (chips), deep-linking + state-preservation (URL), informative
aria-label(purpose + current value).Verified locally
?vault=a&vault=b→ onlya,b;?vault=a→ onlya; no param → all. ✓ruffclean ·mypyclean · 22 search unit tests pass.design:check·typecheck·lint· 347 tests (incl. 3 new scope tests:?v=a,b→searchDocs(q,[a,b],25), "All vaults (N)", count label + removable chips).backend 0.9.4.
🤖 Generated with Claude Code