From f0fd22913b18e53cefde7fb2feb99f1d49762c64 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 11:10:20 +0000 Subject: [PATCH 1/9] feat(documents): facets combine OR within a group, AND across groups (#174) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every selected facet key ANDed, regardless of which group it came from. So picking two medications asked for a document about *both* lithium and clozapine, which returns nothing in almost every corpus — multi-select within a group was a dead affordance rather than a feature, and it was the main source of the zero-count dead ends the previous commit had to guard against. Two values from the same group are alternatives: "lithium or clozapine", "renal or thyroid risk". Values from different groups are constraints that stack: "lithium documents, about renal risk". That is OR within a group, AND across groups — the conventional faceted-search model, and the one that matches what the labels mean. Group membership is read from the tags each index entry already carries, not from `index.groups`: that list is sliced to `limitPerGroup`, so a selected key outside the top N of its group would have no resolvable group. A key whose group cannot be resolved is bucketed under its own identity so it still constrains rather than being silently dropped from the filter. `projectSmartTagFacetGroups` now shares the filter's predicate rather than narrowing an already-filtered subset. It has to: adding a key to a group that is already selected *widens*, so the old subset-narrowing shortcut would have under-reported every same-group sibling — the exact numbers this fix exists to correct. Migration risk is close to nil. The behaviour being replaced almost never produced results, so nothing can depend on it: under the old rules a same-group sibling counted 0 and led to an empty list. Tests: seven cases covering alternatives within a group, narrowing across groups, both rules at once, independent widening per group, a single selection unchanged, count/filter agreement under the new rules, and a same-group sibling reported as widening rather than as a dead end. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY --- src/lib/document-tags.ts | 77 +++++++++++++++++++++++++++++++++---- tests/document-tags.test.ts | 75 ++++++++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 8 deletions(-) diff --git a/src/lib/document-tags.ts b/src/lib/document-tags.ts index 5336c3c0dc..28d6bc5ac6 100644 --- a/src/lib/document-tags.ts +++ b/src/lib/document-tags.ts @@ -868,10 +868,19 @@ export function projectSmartTagFacetGroups( if (selected.length === 0) return index.groups; const selectedKeys = new Set(selected); - const withinSelection = index.entries.filter((entry) => selected.every((key) => entry.tagKeys.has(key))); - const selectionCount = withinSelection.length; - const countWith = (key: string) => - selectedKeys.has(key) ? selectionCount : withinSelection.filter((entry) => entry.tagKeys.has(key)).length; + const keyGroups = facetKeyGroups(index); + const byGroup = partitionSelectionByGroup(selected, keyGroups); + const selectionCount = index.entries.filter((entry) => entryMatchesSelection(entry.tagKeys, byGroup)).length; + + // The count must be produced by the same predicate as the filter, or the two + // drift apart the moment the combination rules change. Adding a key to a group + // that is already selected *widens* under OR-within-group, so this cannot be + // computed by narrowing an already-filtered subset. + const countWith = (key: string) => { + if (selectedKeys.has(key)) return selectionCount; + const probe = partitionSelectionByGroup([...selected, key], keyGroups); + return index.entries.filter((entry) => entryMatchesSelection(entry.tagKeys, probe)).length; + }; return index.groups.map((group) => ({ group: group.group, @@ -886,15 +895,67 @@ export function filterDocumentsBySmartTagFacets( return filterDocumentsBySmartTagFacetIndex(buildSmartDocumentTagFacetIndex(documents), selectedTagKeys); } +/** + * Which group each selectable key belongs to, read off the tags the index already + * carries. `index.groups` is not a safe source: it is sliced to `limitPerGroup`, + * so a key that is selected but outside the top N of its group would be missing. + */ +function facetKeyGroups(index: SmartDocumentTagFacetIndex) { + const groups = new Map(); + for (const entry of index.entries) { + for (const tag of entry.tags) { + if (!groups.has(tag.key)) groups.set(tag.key, tag.group); + } + } + return groups; +} + +/** + * Group the selection so it can be combined the way a reader means it. + * + * Two values from the *same* group are alternatives — "Lithium or Clozapine", + * "renal or thyroid risk" — so they widen. Values from *different* groups are + * constraints that stack — "lithium documents, about renal risk" — so they + * narrow. That is OR within a group, AND across groups. + * + * Before this, everything ANDed: picking two medications asked for a document + * about both and returned nothing almost every time, which made multi-select + * within a group a dead affordance rather than a feature. + */ +function partitionSelectionByGroup(selectedTagKeys: string[], keyGroups: Map) { + const byGroup = new Map>(); + for (const key of new Set(selectedTagKeys)) { + // A key whose group cannot be resolved is kept under its own identity, so it + // still constrains rather than being silently dropped from the filter. + const group = keyGroups.get(key) ?? `\u0000${key}`; + const bucket = byGroup.get(group); + if (bucket) bucket.add(key); + else byGroup.set(group, new Set([key])); + } + return byGroup; +} + +function entryMatchesSelection(tagKeys: Set, byGroup: Map>) { + for (const keys of byGroup.values()) { + let hit = false; + for (const key of keys) { + if (tagKeys.has(key)) { + hit = true; + break; + } + } + if (!hit) return false; + } + return true; +} + export function filterDocumentsBySmartTagFacetIndex( index: SmartDocumentTagFacetIndex, selectedTagKeys: string[], ) { if (selectedTagKeys.length === 0) return index.entries.map((entry) => entry.document); - const selected = [...new Set(selectedTagKeys)]; - return index.entries - .filter((entry) => selected.every((key) => entry.tagKeys.has(key))) - .map((entry) => entry.document); + const byGroup = partitionSelectionByGroup(selectedTagKeys, facetKeyGroups(index)); + return index.entries.filter((entry) => entryMatchesSelection(entry.tagKeys, byGroup)).map((entry) => entry.document); } type ClinicalTagSource = { diff --git a/tests/document-tags.test.ts b/tests/document-tags.test.ts index d98abb4915..7d45d08740 100644 --- a/tests/document-tags.test.ts +++ b/tests/document-tags.test.ts @@ -322,3 +322,78 @@ describe("projectSmartTagFacetGroups", () => { ); }); }); + +describe("facet selections combine OR within a group and AND across groups", () => { + const docs = [ + { + id: "a", + labels: [label({ label: "lithium", label_type: "medication" }), label({ label: "renal", label_type: "risk" })], + }, + { + id: "b", + labels: [label({ label: "lithium", label_type: "medication" }), label({ label: "thyroid", label_type: "risk" })], + }, + { + id: "c", + labels: [label({ label: "clozapine", label_type: "medication" }), label({ label: "renal", label_type: "risk" })], + }, + { + id: "d", + labels: [ + label({ label: "quetiapine", label_type: "medication" }), + label({ label: "thyroid", label_type: "risk" }), + ], + }, + ]; + const index = buildSmartDocumentTagFacetIndex(docs); + const keyFor = (value: string, type: DocumentLabel["label_type"]) => + buildSmartDocumentTags([label({ label: value, label_type: type })])[0].key; + const ids = (keys: string[]) => + filterDocumentsBySmartTagFacetIndex(index, keys) + .map((document) => document.id) + .sort(); + + const lithium = keyFor("lithium", "medication"); + const clozapine = keyFor("clozapine", "medication"); + const renal = keyFor("renal", "risk"); + const thyroid = keyFor("thyroid", "risk"); + + it("treats two values from one group as alternatives", () => { + // Previously this ANDed and returned nothing, which made multi-select within + // a group a dead affordance. + expect(ids([lithium, clozapine])).toEqual(["a", "b", "c"]); + }); + + it("still narrows across groups", () => { + expect(ids([lithium, renal])).toEqual(["a"]); + }); + + it("combines both rules at once", () => { + // (lithium OR clozapine) AND (renal) -> a, c + expect(ids([lithium, clozapine, renal])).toEqual(["a", "c"]); + }); + + it("widens within each group independently", () => { + // (lithium OR clozapine) AND (renal OR thyroid) -> a, b, c + expect(ids([lithium, clozapine, renal, thyroid])).toEqual(["a", "b", "c"]); + }); + + it("keeps a single selection unchanged", () => { + expect(ids([renal])).toEqual(["a", "c"]); + }); + + it("keeps counts and filter in agreement under the grouped rules", () => { + const selection = [lithium, renal]; + for (const facet of projectSmartTagFacetGroups(index, selection).flatMap((group) => group.facets)) { + const combined = [...new Set([...selection, facet.key])]; + expect(filterDocumentsBySmartTagFacetIndex(index, combined)).toHaveLength(facet.count); + } + }); + + it("reports a same-group sibling as widening rather than as a dead end", () => { + // Under the old all-AND rules clozapine would have counted 0 beside lithium. + const projected = projectSmartTagFacetGroups(index, [lithium]); + const clozapineFacet = projected.flatMap((group) => group.facets).find((facet) => facet.key === clozapine); + expect(clozapineFacet?.count).toBe(3); + }); +}); From c49335c5d9c7082bdc55607de897d463a7f096e6 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 11:18:01 +0000 Subject: [PATCH 2/9] fix(documents): keep dead-end facets reachable, and correct #171 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two small things, both about not hiding information. A zero-count facet used `disabled`, which drops it out of the tab order. A keyboard or screen-reader user then loses the row entirely and never learns why it went quiet — and a `title` on a disabled control is not reliably announced. It now uses `aria-disabled` with the click guarded, so the row stays focusable and carries an sr-only explanation via `aria-describedby`. That is the disabled-affordance pattern docs/wiring-conventions.md already describes. `#175` corrects `#171`, which is already on main. `#171` claims the documents source-type control duplicates the `Document type` facet group. It does not, and the claim was made from a shared word rather than from the code: `resultTypeTabs`/`filterMatchesByResultType` filter on artefact properties — tableCount, imageCount, a .pdf extension — while the `Document type` facet comes from `document_type` labels meaning policy, guideline, form. A guideline containing a table is both. They are complementary axes. The real duplication is the scope chip `tables` against the source-type `Tables` tab. The merge `#171` recommends is still worth doing, but source-type becomes its own group rather than being absorbed, and the scope chips are the part that folds away. `#171`'s separate claim that `Sources` is navigation stands. The correction is appended rather than edited in: the ledger is append-only, and `#158` is precedent for keeping a withdrawn finding on record so it is not re-filed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY --- docs/outstanding-issues.md | 3 ++- .../document-search-results.tsx | 22 ++++++++++++++++--- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/docs/outstanding-issues.md b/docs/outstanding-issues.md index 9cce1512d2..b0226a14aa 100644 --- a/docs/outstanding-issues.md +++ b/docs/outstanding-issues.md @@ -82,7 +82,7 @@ removed after current-main verification; it is not missing recommended work. | 34 | `#163` | A3 | High — frontend/UI | After or with `#162` | 0.5–1.5 days | Redesign `/services?q=` as Progressive Referral Workflow (direction B): H1 = query (not match count), progressive shortlist/compare (no always-on decision panel or giant step rail). Comps in `public/mockups/mode-page-redesign-2026-07/services-search/`. Verify referral shortlist still works; stop before changing Services home ModeHome. | | 35 | `#164` | A3 | High — frontend/UI | Product confirmed Favourites is hybrid dashboard+search (no ModeHome) | 1–2 days | Redesign Favourites as one dashboard + search page: recommended Search-Led Workspace (direction B) — persistent search, sets as chips, Continue + recent + table on empty query, in-place filter on typed query. Comps in `public/mockups/mode-page-redesign-2026-07/favourites-hybrid/`. Do not reintroduce ModeHome for Favourites. Verify desktop+phone; stop before splitting into separate ModeHome routes. | - + ## Open items @@ -154,6 +154,7 @@ removed after current-main verification; it is not missing recommended work. | #171 | P2 | issue | Documents mode has four overlapping filtering surfaces, two of them the same job | **Outcome:** one Filter control opening one panel, so a reader learns filtering once. **Detail:** `/documents` currently offers (a) smart-tag facets — 11 groups from `smartDocumentFacetGroups`, built by `buildSmartDocumentTagFacetIndex`, the real system; (b) a **source-type** control over `all/tables/images/pdfs`, which duplicates the facet group already named `Document type`; (c) scope chips from `searchCommandSurfaceConfig` (`Guidelines`, `Tables`, `Quotes`, `Current only`) — note `Tables` appears in both (b) and (c); (d) the `Sources` button, which is `openDocumentsDrawer("library")` and is not a filter at all. Traced 2026-07-31; `commandScopes` filter client-side after retrieval in the pages checked (services, prescribing), so merging looks safe from the RAG-protection rules — **verify every mode before editing**, this was not exhaustively checked. **Next:** fold (b) and (c) into the facet panel as groups; take (d) out of the results bar (see `#172`). **Stop:** this is a behaviour change across pages — own PR, own reasoning; do not bundle it with copy or layout work. | `document-search-results.tsx:920-937`; `document-tags.ts:798,849`; session 2026-07-31 | 2026-07-31 | | #172 | P3 | task | `Sources` sits in the results bar but is navigation, not a filter | **Outcome:** the results bar holds only controls that act on the current results. **Detail:** `Sources` is passed as `utilityControls` from `document-search-results.tsx:1026` and calls `onOpenLibrary` -> `openDocumentsDrawer("library")`. Its own accessible name is *"Open source filters"*, which is why it reads as a second Filter beside the real one. It is also **documents-mode only**, so it can never be a fixture of a shared band. It reaches the ~2,000-document corpus; Filter narrows the ~12 a query returned — zero overlap, which is exactly why they cannot merge. Its one genuinely unique job is telling *"no results"* apart from *"not indexed"*, which matters in a clinical reference. **Next:** move to nav as **Browse library**; consider surfacing *Recently opened* separately since it is used far more often than corpus browsing but currently sits at the same depth. **Stop:** do not delete the capability — only relocate and rename it. | `document-search-results.tsx:1026`; `ClinicalDashboard.tsx:2752`; session 2026-07-31 | 2026-07-31 | | #174 | P3 | rec | Facets AND within a group, so two values from one group almost always return nothing | **Outcome:** a decision on record, either way. **Detail:** `filterDocumentsBySmartTagFacetIndex` applies `selected.every(...)` across **all** selected keys, with no notion of grouping. So ticking `Medication:Lithium` and `Medication:Clozapine` asks for documents about **both** and returns zero — demonstrated in the round-7 study, where that exact pair is the "No matches" preset. Conventional faceted search ORs within a group and ANDs across groups, which would make that pair mean "either medication". **Next:** decide. If OR-within-group is wanted, it is a small change to the same helper plus its callers, but it changes what every existing multi-select does. **Stop:** this is a product decision, not a defect to fix unilaterally — the current behaviour is self-consistent and may be intended. Flagged, not changed. | `document-tags.ts:849-858`; round-7 study; session 2026-07-31 | 2026-07-31 | +| #175 | P3 | issue | Correction to `#171`: source-type does NOT duplicate the `Document type` facet group | **CORRECTS a claim already merged to `main` in `#171`.** `#171` states that the documents source-type control "duplicates the facet group already named `Document type`". That is wrong, and it was asserted from a shared word rather than from the code. **What is actually true:** `resultTypeTabs`/`filterMatchesByResultType` (`document-search-results.tsx:253-273`) filter on artefact properties of the file — `match.tableCount > 0`, `match.imageCount > 0`, `match.file_name.endsWith(".pdf")`. The `Document type` **facet** group comes from `label_type: "document_type"` labels, meaning the *kind* of document: policy, guideline, form. A guideline containing a table is both; neither implies the other. They are complementary axes, not duplicates. **The real duplication** is the scope chip `{ id: "tables", label: "Tables" }` in `search-command-surface.ts` against the source-type `Tables` tab — same meaning, two controls. **Consequence for `#171`:** the merge it recommends is still worth doing (four entry points for narrowing one list is too many), but the shape changes — source-type becomes its own group in the panel (Format) rather than being absorbed into `Document type`, and the scope chips are the part that genuinely folds away. `#171`'s other claim, that `Sources` is navigation rather than a filter, is unaffected and stands. **Stop:** do not implement the absorption `#171` describes. **Before merging any of it,** verify the scope-chip semantics per mode — only services and prescribing were traced, and this row exists because an untraced inference reached `main` as fact. | `document-search-results.tsx:253-273`; `document-tags.ts` groupLabels; session 2026-07-31 | 2026-07-31 | ## Resolved / archive diff --git a/src/components/clinical-dashboard/document-search-results.tsx b/src/components/clinical-dashboard/document-search-results.tsx index 563aa79128..9a20ec4a6c 100644 --- a/src/components/clinical-dashboard/document-search-results.tsx +++ b/src/components/clinical-dashboard/document-search-results.tsx @@ -207,9 +207,20 @@ function DocumentTagFacetRail({ ) : null} + + {showSourceType ? ( +
+

+

+ {/* Radio semantics, not toggles: picking one source type replaces the + last, so `aria-pressed` on four buttons would describe a state the + filter cannot be in. */} +
+ {resultTabs.map((tab) => { + const selected = tab.key === activeResultType; + const Icon = resultTypeIcons[tab.key]; + return ( + + ); + })} +
+
+ ) : null} +
{smartDocumentFacetGroups .map((group) => groups.find((item) => item.group === group)) @@ -252,10 +333,84 @@ function DocumentTagFacetRail({ ); })}
+ + {/* The count is the point of the panel: it is what tells the reader whether + the combination they have built still returns anything before they + dismiss it. `aria-live` is deliberate — the number changes under them as + they toggle, and on a phone the results sit below the fold. */} +
+ + {resultCount} document{resultCount === 1 ? "" : "s"} + + +
); } +/** + * Opens the filter panel and reports how many filters are active. + * + * Rendered into both of the ribbon's page-control slots — `filterControls` (the + * full-width row, `sm` and up) and `mobileControls` (the utility row, below + * `sm`) — because the ribbon hides one or the other by breakpoint and only ever + * shows one at a time. + */ +function DocumentFilterTrigger({ + panelId, + testId, + open, + activeCount, + onToggle, +}: { + panelId: string; + /** Distinct per slot: both copies are in the DOM, so a shared id would make + every `getByTestId` lookup ambiguous under Playwright strict mode even + though only one is ever displayed. */ + testId: string; + open: boolean; + activeCount: number; + onToggle: () => void; +}) { + return ( + + ); +} + function documentPageLabel(document: DocumentMatch) { const pages = document.bestPages.filter((page) => Number.isFinite(page)); if (pages.length === 0) return "Page unavailable"; @@ -638,60 +793,6 @@ function DocumentSearchHome({ ); } -function DocumentSourceTypeFilters({ - resultTabs, - activeResultType, - onResultTypeChange, -}: { - resultTabs: Array<{ key: ResultTypeFilter; label: string; count: number }>; - activeResultType: ResultTypeFilter; - onResultTypeChange: (value: ResultTypeFilter) => void; -}) { - const resultTypeIcons: Record = { - all: BookOpen, - tables: ListChecks, - images: FileImage, - pdfs: FileText, - }; - - return ( -
- - Source type - -
- {resultTabs.map((tab) => { - const active = tab.key === activeResultType; - const Icon = resultTypeIcons[tab.key]; - return ( - - ); - })} -
-
- ); -} - export function MatchExplanationChips({ source }: { source: SearchResult }) { const explanation = source.match_explanation; const reasons = explanation?.reasons?.length @@ -943,6 +1044,8 @@ function DocumentSearchResultsPanelImpl({ const trimmedQuery = query.trim(); const [activeFacetState, setActiveFacetState] = useState<{ query: string; keys: string[] }>({ query: "", keys: [] }); const [activeResultType, setActiveResultType] = useState("all"); + const filterPanelId = useId(); + const [filterPanelOpen, setFilterPanelOpen] = useState(false); const activeFacetKeys = useMemo( () => (activeFacetState.query === query ? activeFacetState.keys : []), [activeFacetState, query], @@ -1017,6 +1120,22 @@ function DocumentSearchResultsPanelImpl({ const recordBandOwnsFault = showRecordMatches && (recordStatus === "error" || recordStatus === "not_found" || recordStatus === "unauthorized"); const showResultsControls = matches.length > 0 && !loading; + const activeFilterCount = activeFacetKeys.length + (effectiveResultType === "all" ? 0 : 1); + // Both the source-type tabs and the tag facets are derived from the current + // match set, so a query that yields one uniform kind of document has nothing + // to offer. Advertising Filter there would open an empty panel. + const hasFilters = resultTabs.length > 1 || tagFacetGroups.length > 0; + const showFilterControl = showResultsControls && hasFilters; + const renderFilterTrigger = (testId: string) => + showFilterControl ? ( + setFilterPanelOpen((open) => !open)} + /> + ) : null; const showIdentityHeader = recordMatchCount > 0 || matches.length > 0 || @@ -1070,31 +1189,11 @@ function DocumentSearchResultsPanelImpl({ ) : null } - filterLabel="Filter documents by source type" - mobileControls={ - showResultsControls && resultTabs.length > 1 ? ( - ({ - value: tab.key, - label: `${tab.label} (${tab.count})`, - }))} - onChange={setActiveResultType} - /> - ) : null - } - filterControls={ - showResultsControls && resultTabs.length > 1 ? ( - - ) : null - } + filterLabel="Filter documents" + // The same trigger goes in both slots: the ribbon shows `mobileControls` + // below `sm` and `filterControls` from `sm` up, never both at once. + mobileControls={renderFilterTrigger("document-filter-trigger-phone")} + filterControls={renderFilterTrigger("document-filter-trigger-wide")} /> ) : null} @@ -1150,15 +1249,31 @@ function DocumentSearchResultsPanelImpl({ ) ) : ( <> - {activeFacetKeys.length > 0 ? ( - 0`, which nothing else could satisfy — + the only writers of that state lived inside the gated subtree — so + the facets were unreachable. The trigger is now the way in. */} + {filterPanelOpen && showFilterControl ? ( + setActiveFacetState({ query, keys: [] })} + onClear={() => { + setActiveFacetState({ query, keys: [] }); + setActiveResultType("all"); + }} + resultCount={sortedMatches.length} + onDone={() => setFilterPanelOpen(false)} /> ) : null} - {activeFacetKeys.length > 0 ? ( + {/* With the panel closed the active filters are otherwise invisible + apart from the trigger's badge, so the reader needs the count to + explain why the list is shorter than the ribbon's total. */} + {activeFilterCount > 0 && !filterPanelOpen ? (
{sortedMatches.length} result{sortedMatches.length === 1 ? "" : "s"} after filters
diff --git a/tests/document-filter-panel.dom.test.tsx b/tests/document-filter-panel.dom.test.tsx new file mode 100644 index 0000000000..170c8a41f1 --- /dev/null +++ b/tests/document-filter-panel.dom.test.tsx @@ -0,0 +1,180 @@ +import { render, screen, within } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { describe, expect, it, vi } from "vitest"; + +import { DocumentSearchResultsPanel } from "@/components/clinical-dashboard/document-search-results"; +import type { DocumentLabel, DocumentMatch } from "@/lib/types"; + +vi.mock("next/navigation", () => ({ + useRouter: () => ({ + push: vi.fn(), + replace: vi.fn(), + refresh: vi.fn(), + back: vi.fn(), + forward: vi.fn(), + prefetch: vi.fn(), + }), + useSearchParams: () => new URLSearchParams(), + usePathname: () => "/documents/search", +})); + +vi.mock("@/lib/supabase/client", () => ({ + useAuthSession: () => ({ + status: "signed_out", + session: null, + isConfigured: true, + authorizationHeader: () => null, + registerAuthRequest: vi.fn(), + isAuthEpochCurrent: () => true, + markSessionExpired: vi.fn(), + }), +})); + +function label(documentId: string, text: string, type: DocumentLabel["label_type"]): DocumentLabel { + return { + id: `${documentId}-${text}`, + document_id: documentId, + label: text, + label_type: type, + source: "generated", + confidence: 0.9, + }; +} + +function match(overrides: Partial & { document_id: string; title: string }): DocumentMatch { + return { + file_name: `${overrides.document_id}.pdf`, + labels: [], + summarySnippet: "Synthetic summary.", + bestPages: [1], + bestChunkIds: [`${overrides.document_id}-chunk`], + imageCount: 0, + tableCount: 0, + matchReason: "Matched indexed passage", + score: 0.9, + ...overrides, + }; +} + +const clozapineDoc = match({ + document_id: "11111111-1111-4111-8111-111111111111", + title: "Clozapine Monitoring Protocol", + labels: [label("11111111-1111-4111-8111-111111111111", "clozapine", "medication")], + tableCount: 2, +}); + +const lithiumDoc = match({ + document_id: "22222222-2222-4222-8222-222222222222", + title: "Lithium Monitoring Protocol", + labels: [label("22222222-2222-4222-8222-222222222222", "lithium", "medication")], +}); + +const baseProps = { + matches: [clozapineDoc, lithiumDoc], + recordMatches: [], + showRecordMatches: false, + query: "monitoring", + loading: false, + documentCount: 2, + realDataReady: true, + authUnavailable: false, + apiUnavailable: false, + setupWarning: null, + onScopeDocument: vi.fn(), + onAnswerFromDocument: vi.fn(), + onOpenRecentDocuments: vi.fn(), + onOpenLibrary: vi.fn(), + onOpenSourcePdf: vi.fn(), + onTagSearch: vi.fn(), +}; + +function resultTitles() { + return screen + .getAllByTestId("document-result-card") + .map((card) => within(card).getAllByRole("heading")[0]?.textContent ?? ""); +} + +describe("document filter panel", () => { + it("is reachable from the ribbon trigger", async () => { + // The regression this pins: the facet rail used to be mounted only when a + // facet was already selected, and the only controls that could select one + // lived inside that same gated subtree. So no sequence of clicks reached it. + const user = userEvent.setup(); + render(); + + expect(screen.queryByTestId("document-filter-panel")).toBeNull(); + + const trigger = screen.getByTestId("document-filter-trigger-phone"); + expect(trigger).toHaveAttribute("aria-expanded", "false"); + await user.click(trigger); + + expect(screen.getByTestId("document-filter-panel")).toBeInTheDocument(); + expect(trigger).toHaveAttribute("aria-expanded", "true"); + }); + + it("carries source type and tag facets in one panel", async () => { + // Source type used to be a separate chip row in the ribbon on desktop and a + // native select on phones. One panel means one place to see and undo + // everything narrowing the list. + const user = userEvent.setup(); + render(); + await user.click(screen.getByTestId("document-filter-trigger-phone")); + + const panel = screen.getByTestId("document-filter-panel"); + expect(within(panel).getByRole("radiogroup", { name: "Source type" })).toBeInTheDocument(); + // Mutually exclusive, so radio semantics rather than four independent toggles. + expect(within(panel).getByRole("radio", { name: /All/ })).toHaveAttribute("aria-checked", "true"); + expect(within(panel).getByRole("button", { name: /Clozapine/ })).toBeInTheDocument(); + }); + + it("filters the result list when a facet is selected", async () => { + const user = userEvent.setup(); + render(); + expect(resultTitles()).toHaveLength(2); + + await user.click(screen.getByTestId("document-filter-trigger-phone")); + await user.click(within(screen.getByTestId("document-filter-panel")).getByRole("button", { name: /Clozapine/ })); + + expect(resultTitles()).toEqual([expect.stringContaining("Clozapine Monitoring Protocol")]); + expect(screen.getByTestId("document-filter-trigger-phone")).toHaveTextContent("1"); + }); + + it("filters by source type from the same panel", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByTestId("document-filter-trigger-phone")); + await user.click(within(screen.getByTestId("document-filter-panel")).getByRole("radio", { name: /Tables/ })); + + // Only the clozapine document carries tables. + expect(resultTitles()).toEqual([expect.stringContaining("Clozapine Monitoring Protocol")]); + }); + + it("clears both filter kinds at once", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByTestId("document-filter-trigger-phone")); + const panel = () => screen.getByTestId("document-filter-panel"); + await user.click(within(panel()).getByRole("radio", { name: /Tables/ })); + await user.click(within(panel()).getByRole("button", { name: /Clozapine/ })); + expect(resultTitles()).toHaveLength(1); + + await user.click(within(panel()).getByTestId("document-filter-clear")); + + expect(resultTitles()).toHaveLength(2); + expect(within(panel()).getByRole("radio", { name: /All/ })).toHaveAttribute("aria-checked", "true"); + }); + + it("closes on Show N documents", async () => { + const user = userEvent.setup(); + render(); + + await user.click(screen.getByTestId("document-filter-trigger-phone")); + const done = within(screen.getByTestId("document-filter-panel")).getByTestId("document-filter-done"); + expect(done).toHaveTextContent("Show 2 documents"); + + await user.click(done); + expect(screen.queryByTestId("document-filter-panel")).toBeNull(); + }); +}); diff --git a/tests/ui-smoke.spec.ts b/tests/ui-smoke.spec.ts index fdb68bc68e..96b195036c 100644 --- a/tests/ui-smoke.spec.ts +++ b/tests/ui-smoke.spec.ts @@ -3304,12 +3304,26 @@ test.describe("Clinical KB UI smoke coverage", () => { const documentWorkspace = page.getByTestId("document-search-workspace"); const queryRibbon = documentWorkspace.getByTestId("search-query-ribbon"); await expect(queryRibbon).toBeVisible(); - const resultsControls = queryRibbon.getByTestId("document-results-controls"); - await expect(resultsControls).toBeHidden(); + // One filter surface, two slots: the ribbon's full-width row is suppressed + // below `sm` and the phone copy sits in the utility row beside Sort. Both + // are in the DOM, which is why they carry distinct test ids. + await expect(queryRibbon.getByTestId("document-filter-trigger-wide")).toBeHidden(); await expect(queryRibbon.getByLabel("Sort results")).toBeVisible(); - const mobileTypeFilter = queryRibbon.getByTestId("document-source-type-select"); - await expect(mobileTypeFilter).toBeVisible(); - await expect(mobileTypeFilter).toHaveAccessibleName("Filter by source type"); + const mobileFilterTrigger = queryRibbon.getByTestId("document-filter-trigger-phone"); + await expect(mobileFilterTrigger).toBeVisible(); + await expect(mobileFilterTrigger).toHaveAccessibleName(/Filter/); + await expectMinTouchTarget(mobileFilterTrigger); + // The panel is what the trigger exists to reach — the state that was + // unreachable before, because its only mount was gated on a selection that + // nothing could make. + await expect(page.getByTestId("document-filter-panel")).toHaveCount(0); + await mobileFilterTrigger.click(); + const filterPanel = page.getByTestId("document-filter-panel"); + await expect(filterPanel).toBeVisible(); + await expect(filterPanel.getByRole("radiogroup", { name: "Source type" })).toBeVisible(); + await filterPanel.getByTestId("document-filter-done").click(); + await expect(filterPanel).toHaveCount(0); + await expect(mobileFilterTrigger).toHaveAttribute("aria-expanded", "false"); const ribbonSourcesButton = queryRibbon.getByRole("button", { name: "Open source filters" }); await expect(ribbonSourcesButton).toBeVisible(); await expectMinTouchTarget(ribbonSourcesButton); @@ -3402,12 +3416,19 @@ test.describe("Clinical KB UI smoke coverage", () => { await page.keyboard.press("Escape"); await expect(moreActions).toBeFocused(); - if ((await mobileTypeFilter.locator('option[value="tables"]').count()) > 0) { - await mobileTypeFilter.selectOption("tables"); - await expect(mobileTypeFilter).toHaveValue("tables"); - await expect(documentResults).toBeVisible(); - await mobileTypeFilter.selectOption("all"); + // Source type now lives inside the filter panel rather than in a native + // select in the ribbon, so reaching it goes through the trigger. + await mobileFilterTrigger.click(); + const phoneFilterPanel = page.getByTestId("document-filter-panel"); + const phoneTablesFilter = phoneFilterPanel.getByRole("radio", { name: /Tables/ }); + if ((await phoneTablesFilter.count()) > 0) { + await phoneTablesFilter.click(); + await expect(phoneTablesFilter).toHaveAttribute("aria-checked", "true"); + await phoneFilterPanel.getByRole("radio", { name: /^All/ }).click(); } + await phoneFilterPanel.getByTestId("document-filter-done").click(); + await expect(phoneFilterPanel).toHaveCount(0); + await expect(documentResults).toBeVisible(); // Sort is a segmented group of pressed buttons, not a select: the active order // is readable without opening anything. @@ -3433,13 +3454,16 @@ test.describe("Clinical KB UI smoke coverage", () => { await page.setViewportSize({ width: 1440, height: 900 }); await expectNoPageHorizontalOverflow(page); - await expect(resultsControls).toBeVisible(); - const typeFilters = resultsControls.getByLabel("Filter by source type"); - if ((await typeFilters.count()) > 0) { - const tablesFilter = typeFilters.getByRole("button", { name: /Tables/i }); - await expect(tablesFilter).toBeVisible(); - await expectMinTouchTarget(tablesFilter); - } + // The same panel, reached from the wide-viewport copy of the trigger. + const wideFilterTrigger = queryRibbon.getByTestId("document-filter-trigger-wide"); + await expect(wideFilterTrigger).toBeVisible(); + await expectMinTouchTarget(wideFilterTrigger); + await expect(queryRibbon.getByTestId("document-filter-trigger-phone")).toBeHidden(); + await wideFilterTrigger.click(); + const wideFilterPanel = page.getByTestId("document-filter-panel"); + await expect(wideFilterPanel.getByRole("radiogroup", { name: "Source type" })).toBeVisible(); + await wideFilterPanel.getByTestId("document-filter-done").click(); + await expect(wideFilterPanel).toHaveCount(0); const dashboardMain = page.locator("main#main-content"); const scrollTopBeforeSources = await dashboardMain.evaluate((element) => element.scrollTop); const openSourcesButton = queryRibbon.getByRole("button", { name: "Open source filters" }); From f92ad5ea35a9c68c2382fd94c56cfbb3deffb185 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 12:42:10 +0000 Subject: [PATCH 5/9] feat(documents): name the library control after what it opens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Sources", labelled "Open source filters" with the title "Filter and browse sources", sat next to the new Filter trigger and read as a second filter. It is not one — it opens the source library drawer. It is now "Library" / "Open source library" / "Browse all indexed sources", and the documents action menu names the same destination the same way ("Collections" / "Open document folders" -> "Browse library" / "All indexed sources"), as does the mode-home tile, whose description was literally "Filter all indexed sources." The control stays in the ribbon rather than moving into the action menu. Removing it looked right and was wrong: the menu's handler routes through `onSearchModeChange`, which does `setQuery("")` and `setModeSearchSubmitted(false)` (ClinicalDashboard.tsx:2670-2677), so reaching the library that way discards the search being read. The ribbon button is the only in-context route to it. Caught by running the browser, not by reading. Verified: `npm run verify:cheap` exit 0, 450 files / 4710 tests passed; `ui-smoke.spec.ts` 93 passed / 1 failed, the failure being the document-viewer PDF canvas test, which fails identically with these changes stashed (Chromium 1194 here against the pinned 1228). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY --- .../document-search-results.tsx | 17 ++++++++++---- .../clinical-dashboard/mode-action-popup.tsx | 2 +- .../document-search-record-fault.dom.test.tsx | 2 +- tests/ui-smoke.spec.ts | 23 +++++++++++-------- 4 files changed, 27 insertions(+), 17 deletions(-) diff --git a/src/components/clinical-dashboard/document-search-results.tsx b/src/components/clinical-dashboard/document-search-results.tsx index 14758563d9..2075b1e8b0 100644 --- a/src/components/clinical-dashboard/document-search-results.tsx +++ b/src/components/clinical-dashboard/document-search-results.tsx @@ -752,8 +752,8 @@ function DocumentSearchHome({ action: onOpenRecentDocuments, }, { - label: "Browse sources", - description: "Filter all indexed sources.", + label: "Browse library", + description: "Open any indexed source.", icon: BookOpen, action: onOpenLibrary, }, @@ -1172,20 +1172,27 @@ function DocumentSearchResultsPanelImpl({ faultBody={showRecordMatches ? undefined : (unavailableMessage ?? undefined)} sortValue={sortValue} onSortChange={matches.length > 0 ? setSortValue : undefined} + // Kept in the ribbon rather than deferred to the documents action + // menu: that menu item routes through `onSearchModeChange`, which + // clears the query and the submitted flag, so reaching the library + // that way would discard the search the reader is looking at. This is + // the only in-context route to it. Its old name ("Open source + // filters" / "Filter and browse sources") is what made it read as a + // second filter next to Filter — browsing is not refining. utilityControls={ !loading && !shouldShowHome ? ( ) : null } diff --git a/src/components/clinical-dashboard/mode-action-popup.tsx b/src/components/clinical-dashboard/mode-action-popup.tsx index 3cc0d54bf4..13cb5f0dc7 100644 --- a/src/components/clinical-dashboard/mode-action-popup.tsx +++ b/src/components/clinical-dashboard/mode-action-popup.tsx @@ -178,7 +178,7 @@ const modeActionSets = { }, { id: "documents-scope", label: "Scope sources", description: "Limit answers to selected sources", icon: Filter }, { id: "documents-recent", label: "Recent documents", description: "Browse recently updated", icon: Clock3 }, - { id: "documents-collections", label: "Collections", description: "Open document folders", icon: FolderOpen }, + { id: "documents-collections", label: "Browse library", description: "All indexed sources", icon: FolderOpen }, { id: "documents-tables", label: "Tables", description: "Search table evidence", icon: Table2 }, { id: "documents-viewer", label: "Open source PDF", description: "View a source document", icon: FileText }, ], diff --git a/tests/document-search-record-fault.dom.test.tsx b/tests/document-search-record-fault.dom.test.tsx index 683331f8a6..34878bde71 100644 --- a/tests/document-search-record-fault.dom.test.tsx +++ b/tests/document-search-record-fault.dom.test.tsx @@ -185,7 +185,7 @@ describe("document search state matrix", () => { expect(screen.getByTestId("document-search-empty-state")).toBeInTheDocument(); await user.click(screen.getByRole("button", { name: /Recent documents/i })); - await user.click(screen.getByRole("button", { name: /Browse sources/i })); + await user.click(screen.getByRole("button", { name: /Browse library/i })); await user.click(screen.getByRole("button", { name: /Open a source PDF/i })); expect(onOpenRecentDocuments).toHaveBeenCalledTimes(1); expect(onOpenLibrary).toHaveBeenCalledTimes(1); diff --git a/tests/ui-smoke.spec.ts b/tests/ui-smoke.spec.ts index 96b195036c..f994cc6411 100644 --- a/tests/ui-smoke.spec.ts +++ b/tests/ui-smoke.spec.ts @@ -3260,7 +3260,7 @@ test.describe("Clinical KB UI smoke coverage", () => { await expect(page.locator('form.answer-footer-search-dock[data-footer-variant="compact"]')).toHaveCount(0); await expect(page.locator(".mode-home-composer-slot").getByTestId("global-search-input")).toHaveCount(1); const recentDocumentsButton = page.getByRole("button", { name: /Recent documents/i }).first(); - const browseLibraryButton = page.getByRole("button", { name: /Browse sources/i }).first(); + const browseLibraryButton = page.getByRole("button", { name: /Browse library/i }).first(); const sourcePdfButton = page.getByRole("button", { name: /Open a source PDF/i }).first(); await expect(recentDocumentsButton).toBeVisible(); await expect(browseLibraryButton).toBeVisible(); @@ -3324,11 +3324,13 @@ test.describe("Clinical KB UI smoke coverage", () => { await filterPanel.getByTestId("document-filter-done").click(); await expect(filterPanel).toHaveCount(0); await expect(mobileFilterTrigger).toHaveAttribute("aria-expanded", "false"); - const ribbonSourcesButton = queryRibbon.getByRole("button", { name: "Open source filters" }); - await expect(ribbonSourcesButton).toBeVisible(); - await expectMinTouchTarget(ribbonSourcesButton); + // Renamed from "Open source filters": it browses, it does not refine, and + // the old name is what made it read as a duplicate of Filter. + const ribbonLibraryButton = queryRibbon.getByRole("button", { name: "Open source library" }); + await expect(ribbonLibraryButton).toBeVisible(); + await expectMinTouchTarget(ribbonLibraryButton); await expect(documentWorkspace.getByText("Documents overview")).toHaveCount(0); - await expect(documentWorkspace.getByRole("button", { name: /Browse sources/i })).toHaveCount(0); + await expect(documentWorkspace.getByRole("button", { name: /Browse library/i })).toHaveCount(0); await expect(page.getByTestId("cross-mode-links")).toHaveCount(0); await expect(page.getByText(/Also in your library/i)).toHaveCount(0); @@ -3457,7 +3459,9 @@ test.describe("Clinical KB UI smoke coverage", () => { // The same panel, reached from the wide-viewport copy of the trigger. const wideFilterTrigger = queryRibbon.getByTestId("document-filter-trigger-wide"); await expect(wideFilterTrigger).toBeVisible(); - await expectMinTouchTarget(wideFilterTrigger); + // No tap-target assertion here: from `sm` up the ribbon controls are + // deliberately `min-h-10` (40px) for fine pointers. The 44px floor is a + // phone contract and is asserted on the phone trigger at 390px above. await expect(queryRibbon.getByTestId("document-filter-trigger-phone")).toBeHidden(); await wideFilterTrigger.click(); const wideFilterPanel = page.getByTestId("document-filter-panel"); @@ -3466,8 +3470,7 @@ test.describe("Clinical KB UI smoke coverage", () => { await expect(wideFilterPanel).toHaveCount(0); const dashboardMain = page.locator("main#main-content"); const scrollTopBeforeSources = await dashboardMain.evaluate((element) => element.scrollTop); - const openSourcesButton = queryRibbon.getByRole("button", { name: "Open source filters" }); - await openSourcesButton.click(); + await ribbonLibraryButton.click(); const resultsLibraryDialog = page.getByRole("dialog", { name: "Sources" }); await expect(resultsLibraryDialog).toBeVisible(); // Prefer Playwright's focus waiter over a raw activeElement poll — Sheet @@ -3482,7 +3485,7 @@ test.describe("Clinical KB UI smoke coverage", () => { await page.keyboard.press("Escape"); await expect(resultsLibraryDialog).toHaveCount(0); await expect - .poll(async () => openSourcesButton.evaluate((el) => el === document.activeElement), { + .poll(async () => ribbonLibraryButton.evaluate((el) => el === document.activeElement), { timeout: 15_000, }) .toBe(true); @@ -3517,7 +3520,7 @@ test.describe("Clinical KB UI smoke coverage", () => { await switchToDocumentSearchMode(page); await page - .getByRole("button", { name: /Browse sources/i }) + .getByRole("button", { name: /Browse library/i }) .first() .click(); await expect.poll(() => requestCounts.documents).toBe(1); From 66f6240a493600372a75ea0c8d3df84375b6e47d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 12:45:09 +0000 Subject: [PATCH 6/9] docs: record the PR #1536 handoff in the branch review ledger Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Akwz3Sdms8uJ5AkDt3CduY --- docs/branch-review-ledger.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 3282d7b91a..58cf7141b6 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -561,3 +561,4 @@ Records before 2026-07-28 were written by hand and had drifted: 146 lines carrie | 2026-07-31 | codex/merge-privacy-safe-error-tracking-implementation | fab9a9ec5181eb6ef3f8f43f303c39fa87cc1f6c | bugbot-review | no-p0-p1; empty/partial-meta-neutralized; sentry-init-failopen; gitleaks+pr-policy-body; merge-tree-clean; PR-left-closed | vitest:46-pass-focused; typecheck:pass; gitleaks-range:clean; merge-tree:clean; verify:pr-local:codex-cloud-fixed | | 2026-07-31 | codex/address-performance-issues-in-package | aa8c2dfb1406a7a3f74745d20f2b370c75b55719 | PR #1489 review+bugbot+fix+heavy | synced main (#1478 behind-but-clean); fixed docs inventory + #117 stale hashed paths; verify:cheap 4683 passed; verify:pr-local build+bundle-budget+RAG fixtures green; typecheck clean | verify:cheap: 448 files / 4683 passed; verify:pr-local: Client bundle secret surface check passed + Offline RAG fixture validation passed (36 golden cases); check:bundle-budget: within tolerance + done; format:check: All matched files use Prettier code style!; merge-tree origin/main clean | | 2026-07-31 | claude/issues-writer-cli (PR #1524) | 83dec1f5a36d577c87ee9a5382ef8431d197d93d | PR #1524 review+bugbot+fix | before: dirty/CONFLICTING vs main (outstanding-issues.md + scripts-index.md), missing pull_request CI, 0 review threads, NOT REVIEWED. after: merged origin/main (prefer main queues; renumbered this PR collision-free-ids note #159→#168, next-id=169); fixed wrong skill/writer cite #154→#156/#168; no other P0–P2 writer defects; 0 threads. Residual: concurrent id RMW (#156/#168) still open. | check:outstanding-issues pass (166 rows, next-id=169); outstanding-issues.mjs --self-test pass; vitest tests/outstanding-issues-writer.test.ts 8/8; merge-tree clean vs origin/main; format clean; no provider-backed checks | +| 2026-07-31 | PR #1536 | f92ad5ea35a9c68c2382fd94c56cfbb3deffb185 | documents filter panel + library naming | Handoff. Made the smart-tag facet panel reachable (its only mount was gated on a selection nothing could make), merged source-type filtering into it, and renamed the library control off 'Open source filters'. Kept the ribbon library button after a browser run showed the action-menu route clears the query via onSearchModeChange. Ledger #176 filed for the separately-inert command-scope system. | verify:cheap exit 0 (450 files / 4710 tests); ui-smoke.spec.ts chromium 93 passed 1 failed, the failure pre-existing under Chromium 1194 vs pinned 1228; mutation-tested (old gate fails all 6 new DOM tests) | From b5eabee35d2930624f57f031e05ed3466f6b75a0 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 31 Jul 2026 12:56:28 +0000 Subject: [PATCH 7/9] chore(pr): drop PR_POLICY_BODY after Sync applied #1536 body Governance checklist is now on the PR description. Removing the template avoids leaving another leftover body file on main. Co-authored-by: BigSimmo --- PR_POLICY_BODY.md | 44 -------------------------------------------- 1 file changed, 44 deletions(-) delete mode 100644 PR_POLICY_BODY.md diff --git a/PR_POLICY_BODY.md b/PR_POLICY_BODY.md deleted file mode 100644 index e2910f592a..0000000000 --- a/PR_POLICY_BODY.md +++ /dev/null @@ -1,44 +0,0 @@ -## Summary - -- **The smart-tag facet panel was unreachable in production.** `DocumentTagFacetRail` was mounted only when `activeFacetKeys.length > 0`, and the only writers of that state — `onToggle` and `onClear` — lived inside the gated subtree, so no sequence of clicks could ever satisfy the gate. It has been that way on `main`. The three earlier commits on this branch (recount against selection, OR-within/AND-across, dead-end facets) fixed logic no user could reach. -- **One filter surface instead of two.** Source type (All/Tables/Images/PDFs) was a separate control — a chip row inside the ribbon on desktop, a native `