From f681dd7cb67cf8e8ea119aacdeb668d266cab86e Mon Sep 17 00:00:00 2001 From: tsenoner Date: Tue, 15 Sep 2026 22:17:30 +0200 Subject: [PATCH 1/3] fix(control-bar): stop annotation search matching the middle of a column name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Searching `ted` returned the four Biocentral predictions above the one annotation the reader wanted, because every one of them is keyed `predicted_*` and the filter matched any substring of the raw column name: MATCH predicted_subcellular_location key:Y label("Subcellular location"):n MATCH predicted_membrane key:Y label("Membrane"):n MATCH predicted_signal_peptide key:Y label("Signal peptide"):n MATCH predicted_transmembrane key:Y label("Transmembrane"):n MATCH ted_domains key:Y label("TED domains"):Y Every Biocentral hit was on the key and none on the label, so the matched text was never on screen. A query now matches a substring of the displayed label, or the start of a word in the column name. The asymmetry is the point: a column name is a machine identifier built by joining words, so a mid-word hit in it is almost always an accident, while a label is prose the reader is looking at, so `cellular` should still find Subcellular location. The rule lives in one helper beside annotationLabel/annotationSource, and both pickers call it. They disagreed before — query-condition-row matched column names only and never labels at all, so searching for a label you could see found nothing there. Verified in the running app against the reported column set: ted → ted_domains alone; predicted → all four; membrane → membrane and transmembrane; cellular and loc → subcellular location. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J26u3dxSKYGb8fRxchpJVa --- .../.openspec.yaml | 2 + .../fix-annotation-search-matching/README.md | 4 + .../fix-annotation-search-matching/design.md | 77 +++++++++++++++++++ .../proposal.md | 43 +++++++++++ .../specs/annotation-presentation/spec.md | 32 ++++++++ .../fix-annotation-search-matching/tasks.md | 17 ++++ .../control-bar/annotation-select.ts | 14 ++-- .../control-bar/query-condition-row.ts | 4 +- .../visualization/annotation-metadata.test.ts | 56 +++++++++++++- .../src/visualization/annotation-metadata.ts | 28 +++++++ 10 files changed, 264 insertions(+), 13 deletions(-) create mode 100644 openspec/changes/fix-annotation-search-matching/.openspec.yaml create mode 100644 openspec/changes/fix-annotation-search-matching/README.md create mode 100644 openspec/changes/fix-annotation-search-matching/design.md create mode 100644 openspec/changes/fix-annotation-search-matching/proposal.md create mode 100644 openspec/changes/fix-annotation-search-matching/specs/annotation-presentation/spec.md create mode 100644 openspec/changes/fix-annotation-search-matching/tasks.md diff --git a/openspec/changes/fix-annotation-search-matching/.openspec.yaml b/openspec/changes/fix-annotation-search-matching/.openspec.yaml new file mode 100644 index 00000000..96db9a43 --- /dev/null +++ b/openspec/changes/fix-annotation-search-matching/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-09-15 diff --git a/openspec/changes/fix-annotation-search-matching/README.md b/openspec/changes/fix-annotation-search-matching/README.md new file mode 100644 index 00000000..3649c991 --- /dev/null +++ b/openspec/changes/fix-annotation-search-matching/README.md @@ -0,0 +1,4 @@ +# fix-annotation-search-matching + +Stop annotation search from matching the middle of a column name, and give the +query builder the same label-aware matching the annotation dropdown has. diff --git a/openspec/changes/fix-annotation-search-matching/design.md b/openspec/changes/fix-annotation-search-matching/design.md new file mode 100644 index 00000000..c2d870c6 --- /dev/null +++ b/openspec/changes/fix-annotation-search-matching/design.md @@ -0,0 +1,77 @@ +## Context + +Two pickers filter the same annotation list with two different, hand-written rules: +`annotation-select.ts` matches `column.includes(q) || label(column).includes(q)`, and +`query-condition-row.ts` matches `column.includes(q)`. Neither is tested, and the first is +what produces the `ted` → four-Biocentral-rows result. + +## Goals / Non-Goals + +**Goals:** + +- A query only matches text the reader can account for. +- Searching by raw column name keeps working. +- One rule, one implementation, both pickers. + +**Non-Goals:** + +- Fuzzy matching, ranking, or scoring. The list is short and grouped by source. +- Highlighting the matched substring. Worth doing, but it is a rendering change in two + components and does not depend on this. +- Touching the value picker (`query-value-picker.ts`), which searches annotation _values_, + not column names. + +## Decisions + +### Match column names by word, labels by substring + +A query matches a column when it is a prefix of one of the words in the column name +(split on `_` and `-`), or a substring of the friendly label. + +The asymmetry is the point. Column names are machine identifiers built by joining words, +so a word boundary is meaningful in them and a mid-word hit is almost always an accident — +`ted` inside `predicted` is the whole bug. Labels are prose the reader is looking at, so a +mid-word hit there is exactly what they meant: `cellular` should still find +`Subcellular location`, and `membrane` should still find `Transmembrane`. + +Worked through against the real registry: + +| query | matches | via | +| ----------- | ----------------------------------------------- | ------------------------- | +| `ted` | `ted_domains` only | label and first word | +| `predicted` | all four Biocentral | first word of each column | +| `membrane` | `predicted_membrane`, `predicted_transmembrane` | word; label substring | +| `cellular` | `predicted_subcellular_location` | label substring | +| `loc` | `predicted_subcellular_location` | word `location` | + +Alternatives considered: + +- **Match the label only.** Kills the `predicted` case and every other search by real + column name, which is how anyone reading a bundle's schema looks things up. +- **Match labels first, fall back to column names when nothing matched.** Fixes `ted` too, + but the result set then depends on whether some _other_ annotation happened to match, + so the same query behaves differently in different datasets. +- **Substring on both, and mark why a row matched.** The better end state, but it is a + rendering change in two components; the matching rule should be right regardless. + +### One helper, in the registry module + +`annotationMatchesQuery` goes beside `annotationLabel` and `annotationSource` in +`packages/utils`, which already own per-column presentation knowledge. Both pickers call +it. Today they disagree — the query builder never matches labels at all — and that is only +possible because the rule is written twice. + +## Risks / Trade-offs + +- **A mid-word column search stops working** — `cellular` no longer matches + `predicted_subcellular_location` _by column name_. It still matches by label, and every + registry column has a label. A column with no registry entry falls back to a label + derived from its own name, so the word rule still reaches it. + +## Migration Plan + +None. Pure filtering behaviour, no stored state. + +## Open Questions + +None. diff --git a/openspec/changes/fix-annotation-search-matching/proposal.md b/openspec/changes/fix-annotation-search-matching/proposal.md new file mode 100644 index 00000000..4e1ac02f --- /dev/null +++ b/openspec/changes/fix-annotation-search-matching/proposal.md @@ -0,0 +1,43 @@ +## Why + +Typing `ted` into the annotation dropdown returns the four Biocentral predictions — +Membrane, Signal peptide, Subcellular location, Transmembrane — above the one annotation +the reader was looking for. Every one of them matches on `predic**ted**_*`, the raw column +name, and none of them matches on anything the dropdown actually displays: + +``` +MATCH predicted_subcellular_location key:Y label("Subcellular location"):n +MATCH predicted_membrane key:Y label("Membrane"):n +MATCH predicted_signal_peptide key:Y label("Signal peptide"):n +MATCH predicted_transmembrane key:Y label("Transmembrane"):n +MATCH ted_domains key:Y label("TED domains"):Y +``` + +Searching the column name is deliberate and worth keeping — it is how someone who knows +`predicted_membrane` finds it. Matching an arbitrary substring of it is what makes the +result unexplainable, because the matched text is never on screen. + +The query builder's annotation picker has the same flaw and one more: it matches the raw +column name _only_, so a reader who searches for the label they can see finds nothing. + +## What Changes + +- Match a query against a column name by word, not by arbitrary substring: the query must + begin one of the words in the column name. +- Keep matching the friendly label as a substring, so partial words the reader can actually + see still work. +- Move the rule into one shared helper and have both the annotation dropdown and the query + builder's annotation picker use it, so the two pickers stop disagreeing. + +## Capabilities + +### Modified Capabilities + +- `annotation-presentation`: annotation search matches the displayed label or a word of the + column name, and behaves identically in both pickers. + +## Impact + +- Adds one exported helper beside `annotationLabel`/`annotationSource` in `packages/utils`. +- Changes the filter in `annotation-select.ts` and `query-condition-row.ts` to call it. +- No data, storage, URL or API change. Only which rows a query shows. diff --git a/openspec/changes/fix-annotation-search-matching/specs/annotation-presentation/spec.md b/openspec/changes/fix-annotation-search-matching/specs/annotation-presentation/spec.md new file mode 100644 index 00000000..d6ea5df9 --- /dev/null +++ b/openspec/changes/fix-annotation-search-matching/specs/annotation-presentation/spec.md @@ -0,0 +1,32 @@ +## ADDED Requirements + +### Requirement: Annotation search matches displayed text or a column-name word + +Every annotation picker SHALL match a search query against an annotation when the query is a +substring of that annotation's displayed label, or a prefix of one of the words in its column +name, and SHALL NOT match on an arbitrary substring of the column name. All annotation pickers +SHALL apply this same rule. + +#### Scenario: A query does not match the middle of a column name + +- **WHEN** the reader searches for `ted` +- **THEN** `ted_domains` is offered +- **AND** `predicted_membrane`, `predicted_signal_peptide`, `predicted_subcellular_location` + and `predicted_transmembrane` are not offered, because `ted` appears only inside the word + `predicted` + +#### Scenario: A column name is still searchable by word + +- **WHEN** the reader searches for `predicted` +- **THEN** every `predicted_*` annotation is offered + +#### Scenario: A partial word of a label still matches + +- **WHEN** the reader searches for `cellular` +- **THEN** the annotation labelled `Subcellular location` is offered + +#### Scenario: Both pickers agree + +- **WHEN** the same query is entered in the annotation dropdown and in the query builder's + annotation picker +- **THEN** both offer the same annotations diff --git a/openspec/changes/fix-annotation-search-matching/tasks.md b/openspec/changes/fix-annotation-search-matching/tasks.md new file mode 100644 index 00000000..986ed2d4 --- /dev/null +++ b/openspec/changes/fix-annotation-search-matching/tasks.md @@ -0,0 +1,17 @@ +## 1. Regression Coverage + +- [x] 1.1 Cover the reported case: `ted` matches `ted_domains` and none of `predicted_*`. +- [x] 1.2 Cover that `predicted`, `membrane`, `cellular` and `loc` still match. +- [x] 1.3 Cover that both pickers filter identically. + +## 2. Implementation + +- [x] 2.1 Add `annotationMatchesQuery` beside the other per-column helpers in `packages/utils`. +- [x] 2.2 Call it from the annotation dropdown. +- [x] 2.3 Call it from the query builder's annotation picker, which matched no labels before. + +## 3. Verification + +- [x] 3.1 Run the affected package tests and the `pnpm precommit` gate. +- [x] 3.2 Confirm the reported case in the running app. +- [x] 3.3 Archive this change before the merge. diff --git a/packages/core/src/components/control-bar/annotation-select.ts b/packages/core/src/components/control-bar/annotation-select.ts index a90e5713..a2011d3e 100644 --- a/packages/core/src/components/control-bar/annotation-select.ts +++ b/packages/core/src/components/control-bar/annotation-select.ts @@ -7,6 +7,7 @@ import { groupAnnotations, type GroupedAnnotation } from './annotation-categorie import { annotationLabel, annotationStatSummary, + annotationMatchesQuery, clusterAgreement, getAnnotationMeta, hasAnnotationStats, @@ -174,22 +175,17 @@ class ProtspaceAnnotationSelect extends LitElement { */ private getFilteredGroupedAnnotations(): GroupedAnnotation[] { const grouped = this.categorizeAnnotations(this.annotations); - const queryLower = this.query.trim().toLowerCase(); + const query = this.query.trim(); - if (!queryLower) { + if (!query) { return grouped; } - // Filter each category's annotations by column name or friendly label return grouped .map((group) => ({ ...group, - annotations: group.annotations.filter( - (annotation) => - annotation.toLowerCase().includes(queryLower) || - annotationLabel(annotation, this.annotationDefinitions[annotation]) - .toLowerCase() - .includes(queryLower), + annotations: group.annotations.filter((annotation) => + annotationMatchesQuery(annotation, query, this.annotationDefinitions[annotation]), ), })) .filter((group) => group.annotations.length > 0); // Remove empty categories diff --git a/packages/core/src/components/control-bar/query-condition-row.ts b/packages/core/src/components/control-bar/query-condition-row.ts index 0f727a5d..a145800f 100644 --- a/packages/core/src/components/control-bar/query-condition-row.ts +++ b/packages/core/src/components/control-bar/query-condition-row.ts @@ -6,7 +6,7 @@ import { ANY_VALUE, createCondition, createNumericCondition } from './query-type import type { ProtspaceData } from './types'; import { groupAnnotations } from './annotation-categories'; import { handleListboxKeydown, scrollHighlightedIntoView } from '../../utils/dropdown-helpers'; -import { isNumericAnnotation } from '@protspace/utils'; +import { annotationMatchesQuery, isNumericAnnotation } from '@protspace/utils'; import { queryBuilderStyles } from './query-builder.styles'; import { renderValueChip } from './query-presence'; import './query-value-picker'; @@ -100,7 +100,7 @@ class ProtspaceQueryConditionRow extends LitElement { .map((g) => ({ category: g.category, items: g.annotations - .filter((a) => !queryLower || a.toLowerCase().includes(queryLower)) + .filter((a) => annotationMatchesQuery(a, queryLower, this.data?.annotations?.[a])) .map((name) => ({ name, index: flatIndex++ })), })) .filter((g) => g.items.length > 0); diff --git a/packages/utils/src/visualization/annotation-metadata.test.ts b/packages/utils/src/visualization/annotation-metadata.test.ts index 9bc912f7..7463cc8a 100644 --- a/packages/utils/src/visualization/annotation-metadata.test.ts +++ b/packages/utils/src/visualization/annotation-metadata.test.ts @@ -3,11 +3,12 @@ import { ANNOTATION_METADATA, PREDICTED_PREFIX, TAXONOMY_RANK_ORDER, + annotationLabel, + annotationMatchesQuery, + annotationSource, compareTaxonomyRank, getAnnotationMeta, isPredictedAnnotation, - annotationLabel, - annotationSource, prettifyAnnotationName, } from './annotation-metadata'; @@ -163,3 +164,54 @@ describe('prettifyAnnotationName', () => { expect(prettifyAnnotationName('')).toBe(''); }); }); + +describe('annotationMatchesQuery', () => { + const BIOCENTRAL = [ + 'predicted_membrane', + 'predicted_signal_peptide', + 'predicted_subcellular_location', + 'predicted_transmembrane', + ]; + + it('does not match the middle of a column name', () => { + // `ted` lives inside `predicted`, and none of these columns display it. + expect(BIOCENTRAL.filter((c) => annotationMatchesQuery(c, 'ted'))).toEqual([]); + }); + + it('matches the TED column the reader was looking for', () => { + expect(annotationMatchesQuery('ted_domains', 'ted')).toBe(true); + }); + + it('still matches a column name by whole word', () => { + expect(BIOCENTRAL.every((c) => annotationMatchesQuery(c, 'predicted'))).toBe(true); + expect(annotationMatchesQuery('predicted_subcellular_location', 'loc')).toBe(true); + }); + + it('matches a partial word of the displayed label', () => { + // `Subcellular location` / `Transmembrane` — mid-word, but on screen. + expect(annotationMatchesQuery('predicted_subcellular_location', 'cellular')).toBe(true); + expect(annotationMatchesQuery('predicted_transmembrane', 'membrane')).toBe(true); + }); + + it('treats an empty query as matching everything', () => { + expect(annotationMatchesQuery('predicted_membrane', ' ')).toBe(true); + }); + + it('is case-insensitive', () => { + expect(annotationMatchesQuery('ted_domains', 'TED')).toBe(true); + }); + + it('reaches a column outside the registry through its derived label', () => { + expect(annotationMatchesQuery('my_custom_score', 'custom')).toBe(true); + expect(annotationMatchesQuery('my_custom_score', 'zzz')).toBe(false); + }); + + it('matches mid-word when the column name IS the displayed label', () => { + // An unregistered column's label is its own prettified name, so a mid-word + // query still reaches it. That is the rule working, not a leak: the reader + // is looking at "Predicted foo". Registered columns like predicted_membrane + // display "Membrane" instead, which is why `ted` no longer reaches them. + expect(annotationMatchesQuery('predicted_foo', 'ted')).toBe(true); + expect(annotationMatchesQuery('predicted_membrane', 'ted')).toBe(false); + }); +}); diff --git a/packages/utils/src/visualization/annotation-metadata.ts b/packages/utils/src/visualization/annotation-metadata.ts index 8f2344c3..6333a123 100644 --- a/packages/utils/src/visualization/annotation-metadata.ts +++ b/packages/utils/src/visualization/annotation-metadata.ts @@ -419,6 +419,34 @@ export function annotationLabel(column: string, annotation?: Pick, +): boolean { + const needle = query.trim().toLowerCase(); + if (!needle) return true; + + if (annotationLabel(column, annotation).toLowerCase().includes(needle)) return true; + + return column + .toLowerCase() + .split(/[_-]+/) + .some((word) => word.startsWith(needle)); +} + /** Source/group for an annotation (registry source, else `Other`). */ export function annotationSource( column: string, From f08f0bf79e08c44781d3bcb8902b740b6f25f0dc Mon Sep 17 00:00:00 2001 From: tsenoner Date: Tue, 15 Sep 2026 22:17:50 +0200 Subject: [PATCH 2/3] chore(openspec): archive fix-annotation-search-matching Run as the last commit before the merge, per AGENTS.md. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J26u3dxSKYGb8fRxchpJVa --- .../.openspec.yaml | 0 .../README.md | 0 .../design.md | 0 .../proposal.md | 0 .../specs/annotation-presentation/spec.md | 0 .../tasks.md | 0 .../specs/annotation-presentation/spec.md | 31 +++++++++++++++++++ 7 files changed, 31 insertions(+) rename openspec/changes/{fix-annotation-search-matching => archive/2026-09-15-fix-annotation-search-matching}/.openspec.yaml (100%) rename openspec/changes/{fix-annotation-search-matching => archive/2026-09-15-fix-annotation-search-matching}/README.md (100%) rename openspec/changes/{fix-annotation-search-matching => archive/2026-09-15-fix-annotation-search-matching}/design.md (100%) rename openspec/changes/{fix-annotation-search-matching => archive/2026-09-15-fix-annotation-search-matching}/proposal.md (100%) rename openspec/changes/{fix-annotation-search-matching => archive/2026-09-15-fix-annotation-search-matching}/specs/annotation-presentation/spec.md (100%) rename openspec/changes/{fix-annotation-search-matching => archive/2026-09-15-fix-annotation-search-matching}/tasks.md (100%) diff --git a/openspec/changes/fix-annotation-search-matching/.openspec.yaml b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/.openspec.yaml similarity index 100% rename from openspec/changes/fix-annotation-search-matching/.openspec.yaml rename to openspec/changes/archive/2026-09-15-fix-annotation-search-matching/.openspec.yaml diff --git a/openspec/changes/fix-annotation-search-matching/README.md b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/README.md similarity index 100% rename from openspec/changes/fix-annotation-search-matching/README.md rename to openspec/changes/archive/2026-09-15-fix-annotation-search-matching/README.md diff --git a/openspec/changes/fix-annotation-search-matching/design.md b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/design.md similarity index 100% rename from openspec/changes/fix-annotation-search-matching/design.md rename to openspec/changes/archive/2026-09-15-fix-annotation-search-matching/design.md diff --git a/openspec/changes/fix-annotation-search-matching/proposal.md b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/proposal.md similarity index 100% rename from openspec/changes/fix-annotation-search-matching/proposal.md rename to openspec/changes/archive/2026-09-15-fix-annotation-search-matching/proposal.md diff --git a/openspec/changes/fix-annotation-search-matching/specs/annotation-presentation/spec.md b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/specs/annotation-presentation/spec.md similarity index 100% rename from openspec/changes/fix-annotation-search-matching/specs/annotation-presentation/spec.md rename to openspec/changes/archive/2026-09-15-fix-annotation-search-matching/specs/annotation-presentation/spec.md diff --git a/openspec/changes/fix-annotation-search-matching/tasks.md b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/tasks.md similarity index 100% rename from openspec/changes/fix-annotation-search-matching/tasks.md rename to openspec/changes/archive/2026-09-15-fix-annotation-search-matching/tasks.md diff --git a/openspec/specs/annotation-presentation/spec.md b/openspec/specs/annotation-presentation/spec.md index 7c8b02a8..52dabacf 100644 --- a/openspec/specs/annotation-presentation/spec.md +++ b/openspec/specs/annotation-presentation/spec.md @@ -75,3 +75,34 @@ accessible and dismissable, and SHALL be absent when there is no description and - **WHEN** a documentation popover is open and the user presses Escape or clicks outside it - **THEN** the popover closes + +### Requirement: Annotation search matches displayed text or a column-name word + +Every annotation picker SHALL match a search query against an annotation when the query is a +substring of that annotation's displayed label, or a prefix of one of the words in its column +name, and SHALL NOT match on an arbitrary substring of the column name. All annotation pickers +SHALL apply this same rule. + +#### Scenario: A query does not match the middle of a column name + +- **WHEN** the reader searches for `ted` +- **THEN** `ted_domains` is offered +- **AND** `predicted_membrane`, `predicted_signal_peptide`, `predicted_subcellular_location` + and `predicted_transmembrane` are not offered, because `ted` appears only inside the word + `predicted` + +#### Scenario: A column name is still searchable by word + +- **WHEN** the reader searches for `predicted` +- **THEN** every `predicted_*` annotation is offered + +#### Scenario: A partial word of a label still matches + +- **WHEN** the reader searches for `cellular` +- **THEN** the annotation labelled `Subcellular location` is offered + +#### Scenario: Both pickers agree + +- **WHEN** the same query is entered in the annotation dropdown and in the query builder's + annotation picker +- **THEN** both offer the same annotations From aa4180cb712f8ba074213430098a813945485ac9 Mon Sep 17 00:00:00 2001 From: tsenoner Date: Tue, 15 Sep 2026 22:32:24 +0200 Subject: [PATCH 3/3] fix(control-bar): match the query word-wise, and share one filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review caught two things wrong with the first cut. The rule split the column but not the query, then asked whether any word started with the needle. No word can start with a needle that still holds a separator, so every multi-word column stopped matching its own name — 16 of 38 in the registry. Typing the name you know went fine to `predicted`, went empty at `predicted_`, and never recovered. That directly contradicted this change's own goal that searching by raw column name keeps working. Both sides are normalised on `_`/`-` now, and the query has to begin where a word does. `ted` still misses every `predicted_*`; `predicted_membrane`, `ted_domains` and `go_bp` find themselves again. Verified across the whole registry: no column fails to match its own name. The other miss was worse for being self-inflicted: this change claimed "one rule, one implementation" while annotation-select.test.ts kept its own exported copy on the OLD semantics, with eight tests asserting it. The suite was green for both rules at once, and two of those tests encoded results the shipped code no longer produced. The whole pipeline — group, filter, drop empty sections — now lives in annotation-categories.ts, which already said "Shared by annotation-select and query-condition-row" in its own doc comment. Both pickers call it, the mirror is deleted, and its tests exercise the real function. That also restores the empty-query short circuit query-condition-row lost, and removes the double normalisation at both call sites. Recorded in design.md, because it is not fixed here: a match can still land on text a picker does not draw. The dropdown renders only the label, so `predicted` returns four rows reading "Membrane", "Signal peptide", … The query builder renders only the column name, so `swiss` returns `reviewed`. This removes the worst case — a hit on neither, which is what `ted` was — but making every match accountable needs the pickers to show what matched. query-value-picker already highlights matched substrings; that is the pattern to copy. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01J26u3dxSKYGb8fRxchpJVa --- .../design.md | 18 +++- .../proposal.md | 3 +- .../specs/annotation-presentation/spec.md | 14 ++- .../tasks.md | 2 +- .../specs/annotation-presentation/spec.md | 14 ++- .../control-bar/annotation-categories.ts | 26 +++++ .../control-bar/annotation-select.test.ts | 101 ++++++++---------- .../control-bar/annotation-select.ts | 29 +---- .../control-bar/query-condition-row.ts | 21 ++-- .../visualization/annotation-metadata.test.ts | 70 ++++++------ .../src/visualization/annotation-metadata.ts | 20 ++-- 11 files changed, 162 insertions(+), 156 deletions(-) diff --git a/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/design.md b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/design.md index c2d870c6..e90fd79e 100644 --- a/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/design.md +++ b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/design.md @@ -25,8 +25,14 @@ what produces the `ted` → four-Biocentral-rows result. ### Match column names by word, labels by substring -A query matches a column when it is a prefix of one of the words in the column name -(split on `_` and `-`), or a substring of the friendly label. +A query matches a column when it begins at a word boundary of the column name, or is a +substring of the friendly label. Query and column are both normalised on `_`/`-` first. + +Normalising _both_ sides is the part that is easy to get wrong. A rule that only splits the +column and then asks for `word.startsWith(query)` looks equivalent and is not: no word can +ever start with a needle that still contains a separator, so every multi-word column stops +matching its own name. On this registry that was 16 of 38 columns — typing `predicted_` +emptied the list and never recovered. The asymmetry is the point. Column names are machine identifiers built by joining words, so a word boundary is meaningful in them and a mid-word hit is almost always an accident — @@ -67,6 +73,14 @@ possible because the rule is written twice. `predicted_subcellular_location` _by column name_. It still matches by label, and every registry column has a label. A column with no registry entry falls back to a label derived from its own name, so the word rule still reaches it. +- **A match can still be on text the picker does not draw, and this change does not fix + that.** The dropdown renders only the label, so a column-name hit (`predicted` → four rows + reading "Membrane", "Signal peptide", …) is unexplained. The query builder renders only the + column name, so a label hit (`swiss` → `reviewed`) is unexplained in the other direction. + This removes the worst case — a hit on neither, which is what `ted` was — but "every match + is accountable" needs the pickers to show what matched. `query-value-picker` already + highlights matched substrings via `_highlightMatch`; copying that, and rendering both label + and column name, is the real end state. Deliberately out of scope here. ## Migration Plan diff --git a/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/proposal.md b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/proposal.md index 4e1ac02f..7cc563a3 100644 --- a/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/proposal.md +++ b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/proposal.md @@ -23,7 +23,8 @@ column name _only_, so a reader who searches for the label they can see finds no ## What Changes - Match a query against a column name by word, not by arbitrary substring: the query must - begin one of the words in the column name. + begin where a word does. Query and column are both split on `_`/`-`, so a column is still + findable by its own full name. - Keep matching the friendly label as a substring, so partial words the reader can actually see still work. - Move the rule into one shared helper and have both the annotation dropdown and the query diff --git a/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/specs/annotation-presentation/spec.md b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/specs/annotation-presentation/spec.md index d6ea5df9..7a8c318a 100644 --- a/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/specs/annotation-presentation/spec.md +++ b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/specs/annotation-presentation/spec.md @@ -1,11 +1,12 @@ ## ADDED Requirements -### Requirement: Annotation search matches displayed text or a column-name word +### Requirement: Annotation search matches a label substring or a column-name word Every annotation picker SHALL match a search query against an annotation when the query is a -substring of that annotation's displayed label, or a prefix of one of the words in its column -name, and SHALL NOT match on an arbitrary substring of the column name. All annotation pickers -SHALL apply this same rule. +substring of that annotation's label, or begins at a word boundary of its column name, and +SHALL NOT match on an arbitrary substring of the column name. A column SHALL remain findable +by its own full name, separators included. All annotation pickers SHALL apply this same rule, +through one shared implementation. #### Scenario: A query does not match the middle of a column name @@ -20,6 +21,11 @@ SHALL apply this same rule. - **WHEN** the reader searches for `predicted` - **THEN** every `predicted_*` annotation is offered +#### Scenario: A column is findable by its own full name + +- **WHEN** the reader types a complete column name such as `predicted_membrane` +- **THEN** that annotation is offered, the separator notwithstanding + #### Scenario: A partial word of a label still matches - **WHEN** the reader searches for `cellular` diff --git a/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/tasks.md b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/tasks.md index 986ed2d4..56ea28d7 100644 --- a/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/tasks.md +++ b/openspec/changes/archive/2026-09-15-fix-annotation-search-matching/tasks.md @@ -2,7 +2,7 @@ - [x] 1.1 Cover the reported case: `ted` matches `ted_domains` and none of `predicted_*`. - [x] 1.2 Cover that `predicted`, `membrane`, `cellular` and `loc` still match. -- [x] 1.3 Cover that both pickers filter identically. +- [x] 1.3 Make both pickers filter through one shared function, and test that function. ## 2. Implementation diff --git a/openspec/specs/annotation-presentation/spec.md b/openspec/specs/annotation-presentation/spec.md index 52dabacf..02d652d0 100644 --- a/openspec/specs/annotation-presentation/spec.md +++ b/openspec/specs/annotation-presentation/spec.md @@ -76,12 +76,13 @@ accessible and dismissable, and SHALL be absent when there is no description and - **WHEN** a documentation popover is open and the user presses Escape or clicks outside it - **THEN** the popover closes -### Requirement: Annotation search matches displayed text or a column-name word +### Requirement: Annotation search matches a label substring or a column-name word Every annotation picker SHALL match a search query against an annotation when the query is a -substring of that annotation's displayed label, or a prefix of one of the words in its column -name, and SHALL NOT match on an arbitrary substring of the column name. All annotation pickers -SHALL apply this same rule. +substring of that annotation's label, or begins at a word boundary of its column name, and +SHALL NOT match on an arbitrary substring of the column name. A column SHALL remain findable +by its own full name, separators included. All annotation pickers SHALL apply this same rule, +through one shared implementation. #### Scenario: A query does not match the middle of a column name @@ -96,6 +97,11 @@ SHALL apply this same rule. - **WHEN** the reader searches for `predicted` - **THEN** every `predicted_*` annotation is offered +#### Scenario: A column is findable by its own full name + +- **WHEN** the reader types a complete column name such as `predicted_membrane` +- **THEN** that annotation is offered, the separator notwithstanding + #### Scenario: A partial word of a label still matches - **WHEN** the reader searches for `cellular` diff --git a/packages/core/src/components/control-bar/annotation-categories.ts b/packages/core/src/components/control-bar/annotation-categories.ts index 06f448ae..87e8265c 100644 --- a/packages/core/src/components/control-bar/annotation-categories.ts +++ b/packages/core/src/components/control-bar/annotation-categories.ts @@ -1,4 +1,5 @@ import { + annotationMatchesQuery, annotationSource, compareTaxonomyRank, type Annotation, @@ -86,3 +87,28 @@ export function groupAnnotations( return groups; } + +/** + * Group annotations and keep only those matching a search query, dropping any + * section left empty. + * + * Both pickers filter through here, so "the dropdown and the query builder + * agree" is structural rather than a convention each has to keep honouring. + */ +export function filterGroupedAnnotations( + annotations: string[], + query: string, + definitions?: Readonly>>, +): GroupedAnnotation[] { + const grouped = groupAnnotations(annotations, definitions); + if (!query.trim()) return grouped; + + return grouped + .map((group) => ({ + ...group, + annotations: group.annotations.filter((annotation) => + annotationMatchesQuery(annotation, query, definitions?.[annotation]), + ), + })) + .filter((group) => group.annotations.length > 0); +} diff --git a/packages/core/src/components/control-bar/annotation-select.test.ts b/packages/core/src/components/control-bar/annotation-select.test.ts index cb5a1a83..53dfe8ff 100644 --- a/packages/core/src/components/control-bar/annotation-select.test.ts +++ b/packages/core/src/components/control-bar/annotation-select.test.ts @@ -1,28 +1,9 @@ import { describe, it, expect } from 'vitest'; -import { groupAnnotations, type GroupedAnnotation } from './annotation-categories'; - -/** - * Filter annotations based on search query (mirrors the component's filtering). - */ -export function filterGroupedAnnotations( - grouped: GroupedAnnotation[], - query: string, -): GroupedAnnotation[] { - const queryLower = query.trim().toLowerCase(); - - if (!queryLower) { - return grouped; - } - - return grouped - .map((group) => ({ - ...group, - annotations: group.annotations.filter((annotation) => - annotation.toLowerCase().includes(queryLower), - ), - })) - .filter((group) => group.annotations.length > 0); // Remove empty categories -} +import { + filterGroupedAnnotations, + groupAnnotations, + type GroupedAnnotation, +} from './annotation-categories'; /** * Flatten grouped annotations into a single array for keyboard navigation. @@ -192,63 +173,71 @@ describe('annotation-select', () => { }); describe('filterGroupedAnnotations', () => { - const grouped: GroupedAnnotation[] = [ - { category: 'UniProt', annotations: ['gene_name', 'reviewed', 'protein_families'] }, - { category: 'InterPro', annotations: ['pfam', 'cath'] }, - { category: 'Taxonomy', annotations: ['species', 'genus'] }, - { category: 'Other', annotations: ['custom_field'] }, + const columns = [ + 'gene_name', + 'reviewed', + 'protein_families', + 'pfam', + 'cath', + 'species', + 'genus', + 'custom_field', ]; + const grouped = groupAnnotations(columns); + const filter = (query: string) => filterGroupedAnnotations(columns, query); + const names = (groups: GroupedAnnotation[]) => groups.flatMap((g) => g.annotations); it('returns all annotations when query is empty', () => { - const result = filterGroupedAnnotations(grouped, ''); - expect(result).toEqual(grouped); + expect(filter('')).toEqual(grouped); }); - it('filters annotations by substring match (case insensitive)', () => { - const result = filterGroupedAnnotations(grouped, 'gene'); - expect(result.length).toBe(1); - expect(result[0].category).toBe('UniProt'); - expect(result[0].annotations).toEqual(['gene_name']); + it('matches a column name by word, and a label by substring', () => { + // `cath` comes along because its label is "CATH-Gene3D" — visible text. + expect(names(filter('gene'))).toEqual(['cath', 'gene_name']); }); it('filters across multiple categories', () => { - const result = filterGroupedAnnotations(grouped, 'e'); - const allAnnotations = result.flatMap((g) => g.annotations); - expect(allAnnotations).toContain('gene_name'); - expect(allAnnotations).toContain('reviewed'); - expect(allAnnotations).toContain('species'); - expect(allAnnotations).toContain('genus'); + const all = names(filter('e')); + expect(all).toContain('gene_name'); + expect(all).toContain('reviewed'); + expect(all).toContain('species'); + expect(all).toContain('genus'); }); it('removes categories with no matching annotations', () => { - const result = filterGroupedAnnotations(grouped, 'pfam'); + const result = filter('pfam'); expect(result.length).toBe(1); expect(result[0].category).toBe('InterPro'); }); it('handles case insensitive search', () => { - const result = filterGroupedAnnotations(grouped, 'GENE'); - expect(result.length).toBe(1); - expect(result[0].annotations).toEqual(['gene_name']); + expect(names(filter('GENE'))).toEqual(names(filter('gene'))); }); it('trims whitespace from query', () => { - const result = filterGroupedAnnotations(grouped, ' gene '); - expect(result.length).toBe(1); - expect(result[0].annotations).toEqual(['gene_name']); + expect(names(filter(' gene '))).toEqual(names(filter('gene'))); }); it('returns empty array when no matches found', () => { - const result = filterGroupedAnnotations(grouped, 'xyz123'); - expect(result).toEqual([]); + expect(filter('xyz123')).toEqual([]); }); it('handles partial matches', () => { - const result = filterGroupedAnnotations(grouped, 'fam'); - expect(result.length).toBe(2); // pfam and protein_families - const allAnnotations = result.flatMap((g) => g.annotations); - expect(allAnnotations).toContain('pfam'); - expect(allAnnotations).toContain('protein_families'); + // both via their labels, "Pfam" and "Protein family" + expect(names(filter('fam')).sort()).toEqual(['pfam', 'protein_families']); + }); + + it('does not match the middle of a column name', () => { + // the reported bug: `ted` inside `predicted_*`, which display no "ted" + expect(names(filterGroupedAnnotations(['predicted_membrane', 'ted_domains'], 'ted'))).toEqual( + ['ted_domains'], + ); + }); + + it('still finds a column by its full name', () => { + expect(names(filterGroupedAnnotations(['predicted_membrane'], 'predicted_membrane'))).toEqual( + ['predicted_membrane'], + ); }); }); diff --git a/packages/core/src/components/control-bar/annotation-select.ts b/packages/core/src/components/control-bar/annotation-select.ts index a2011d3e..17dde64f 100644 --- a/packages/core/src/components/control-bar/annotation-select.ts +++ b/packages/core/src/components/control-bar/annotation-select.ts @@ -3,11 +3,10 @@ import { property, state } from 'lit/decorators.js'; import { customElement } from '../../utils/safe-custom-element'; import { annotationSelectStyles } from './annotation-select.styles'; import { handleListboxKeydown, scrollHighlightedIntoView } from '../../utils/dropdown-helpers'; -import { groupAnnotations, type GroupedAnnotation } from './annotation-categories'; +import { filterGroupedAnnotations, type GroupedAnnotation } from './annotation-categories'; import { annotationLabel, annotationStatSummary, - annotationMatchesQuery, clusterAgreement, getAnnotationMeta, hasAnnotationStats, @@ -164,31 +163,11 @@ class ProtspaceAnnotationSelect extends LitElement { } /** - * Categorize annotations using the shared utility. - */ - private categorizeAnnotations(annotations: string[]): GroupedAnnotation[] { - return groupAnnotations(annotations, this.annotationDefinitions); - } - - /** - * Filter annotations based on search query + * Group and filter annotations using the shared utility, so this picker and + * the query builder's cannot drift apart. */ private getFilteredGroupedAnnotations(): GroupedAnnotation[] { - const grouped = this.categorizeAnnotations(this.annotations); - const query = this.query.trim(); - - if (!query) { - return grouped; - } - - return grouped - .map((group) => ({ - ...group, - annotations: group.annotations.filter((annotation) => - annotationMatchesQuery(annotation, query, this.annotationDefinitions[annotation]), - ), - })) - .filter((group) => group.annotations.length > 0); // Remove empty categories + return filterGroupedAnnotations(this.annotations, this.query, this.annotationDefinitions); } /** diff --git a/packages/core/src/components/control-bar/query-condition-row.ts b/packages/core/src/components/control-bar/query-condition-row.ts index a145800f..4d6d70d5 100644 --- a/packages/core/src/components/control-bar/query-condition-row.ts +++ b/packages/core/src/components/control-bar/query-condition-row.ts @@ -4,9 +4,9 @@ import { customElement } from '../../utils/safe-custom-element'; import type { FilterCondition, LogicalOp, NumericCondition } from './query-types'; import { ANY_VALUE, createCondition, createNumericCondition } from './query-types'; import type { ProtspaceData } from './types'; -import { groupAnnotations } from './annotation-categories'; +import { filterGroupedAnnotations } from './annotation-categories'; import { handleListboxKeydown, scrollHighlightedIntoView } from '../../utils/dropdown-helpers'; -import { annotationMatchesQuery, isNumericAnnotation } from '@protspace/utils'; +import { isNumericAnnotation } from '@protspace/utils'; import { queryBuilderStyles } from './query-builder.styles'; import { renderValueChip } from './query-presence'; import './query-value-picker'; @@ -94,16 +94,15 @@ class ProtspaceQueryConditionRow extends LitElement { category: string; items: { name: string; index: number }[]; }[] { - const queryLower = this._annotationSearch.trim().toLowerCase(); let flatIndex = 0; - return groupAnnotations(this.annotations, this.data?.annotations) - .map((g) => ({ - category: g.category, - items: g.annotations - .filter((a) => annotationMatchesQuery(a, queryLower, this.data?.annotations?.[a])) - .map((name) => ({ name, index: flatIndex++ })), - })) - .filter((g) => g.items.length > 0); + return filterGroupedAnnotations( + this.annotations, + this._annotationSearch, + this.data?.annotations, + ).map((g) => ({ + category: g.category, + items: g.annotations.map((name) => ({ name, index: flatIndex++ })), + })); } /** Flattened filtered list — the sequence keyboard navigation walks. */ diff --git a/packages/utils/src/visualization/annotation-metadata.test.ts b/packages/utils/src/visualization/annotation-metadata.test.ts index 7463cc8a..c779c06d 100644 --- a/packages/utils/src/visualization/annotation-metadata.test.ts +++ b/packages/utils/src/visualization/annotation-metadata.test.ts @@ -12,6 +12,14 @@ import { prettifyAnnotationName, } from './annotation-metadata'; +/** The Biocentral ML columns, spelled out once. */ +const BIOCENTRAL_COLUMNS = [ + 'predicted_membrane', + 'predicted_signal_peptide', + 'predicted_subcellular_location', + 'predicted_transmembrane', +]; + describe('annotation-metadata registry', () => { it('resolves known annotations to their metadata', () => { const ec = getAnnotationMeta('ec'); @@ -22,12 +30,7 @@ describe('annotation-metadata registry', () => { }); it('flags the Biocentral predictions as predicted', () => { - for (const column of [ - 'predicted_subcellular_location', - 'predicted_membrane', - 'predicted_signal_peptide', - 'predicted_transmembrane', - ]) { + for (const column of BIOCENTRAL_COLUMNS) { expect(ANNOTATION_METADATA[column]?.isPredicted).toBe(true); expect(ANNOTATION_METADATA[column]?.source).toBe('Biocentral'); } @@ -62,12 +65,7 @@ describe('annotation-metadata registry', () => { }); it('keeps the predicted_ prefix on the Biocentral ML columns', () => { - for (const column of [ - 'predicted_subcellular_location', - 'predicted_membrane', - 'predicted_signal_peptide', - 'predicted_transmembrane', - ]) { + for (const column of BIOCENTRAL_COLUMNS) { expect(column.startsWith(PREDICTED_PREFIX)).toBe(true); } }); @@ -166,52 +164,44 @@ describe('prettifyAnnotationName', () => { }); describe('annotationMatchesQuery', () => { - const BIOCENTRAL = [ - 'predicted_membrane', - 'predicted_signal_peptide', - 'predicted_subcellular_location', - 'predicted_transmembrane', - ]; - it('does not match the middle of a column name', () => { - // `ted` lives inside `predicted`, and none of these columns display it. - expect(BIOCENTRAL.filter((c) => annotationMatchesQuery(c, 'ted'))).toEqual([]); + // `ted` lives inside `predicted`, and these columns display "Membrane", + // "Signal peptide", "Subcellular location", "Transmembrane" — no "ted". + expect(BIOCENTRAL_COLUMNS.filter((c) => annotationMatchesQuery(c, 'ted'))).toEqual([]); + expect(annotationMatchesQuery('ted_domains', 'ted')).toBe(true); }); - it('matches the TED column the reader was looking for', () => { - expect(annotationMatchesQuery('ted_domains', 'ted')).toBe(true); + it('finds every column by its own full name, separators and all', () => { + // The word rule has to split the query too, or typing the name you know + // empties the list at the underscore. + for (const column of Object.keys(ANNOTATION_METADATA)) { + expect(annotationMatchesQuery(column, column)).toBe(true); + } }); - it('still matches a column name by whole word', () => { - expect(BIOCENTRAL.every((c) => annotationMatchesQuery(c, 'predicted'))).toBe(true); + it('matches a column name by word', () => { + expect(BIOCENTRAL_COLUMNS.every((c) => annotationMatchesQuery(c, 'predicted'))).toBe(true); expect(annotationMatchesQuery('predicted_subcellular_location', 'loc')).toBe(true); }); it('matches a partial word of the displayed label', () => { - // `Subcellular location` / `Transmembrane` — mid-word, but on screen. + // mid-word, but on screen: "Subcellular location", "Transmembrane" expect(annotationMatchesQuery('predicted_subcellular_location', 'cellular')).toBe(true); expect(annotationMatchesQuery('predicted_transmembrane', 'membrane')).toBe(true); + // a hyphenated label is matched as typed, not word-split: "CATH-Gene3D" + expect(annotationMatchesQuery('cath', 'cath-gene3d')).toBe(true); }); - it('treats an empty query as matching everything', () => { + it('treats an empty query as matching everything, case-insensitively', () => { expect(annotationMatchesQuery('predicted_membrane', ' ')).toBe(true); - }); - - it('is case-insensitive', () => { expect(annotationMatchesQuery('ted_domains', 'TED')).toBe(true); }); it('reaches a column outside the registry through its derived label', () => { - expect(annotationMatchesQuery('my_custom_score', 'custom')).toBe(true); + // No registry entry, so the label is the prettified column name — which + // means a mid-word query does reach it. That is the rule working: the + // reader is looking at "My custom score". + expect(annotationMatchesQuery('my_custom_score', 'core')).toBe(true); expect(annotationMatchesQuery('my_custom_score', 'zzz')).toBe(false); }); - - it('matches mid-word when the column name IS the displayed label', () => { - // An unregistered column's label is its own prettified name, so a mid-word - // query still reaches it. That is the rule working, not a leak: the reader - // is looking at "Predicted foo". Registered columns like predicted_membrane - // display "Membrane" instead, which is why `ted` no longer reaches them. - expect(annotationMatchesQuery('predicted_foo', 'ted')).toBe(true); - expect(annotationMatchesQuery('predicted_membrane', 'ted')).toBe(false); - }); }); diff --git a/packages/utils/src/visualization/annotation-metadata.ts b/packages/utils/src/visualization/annotation-metadata.ts index 6333a123..9cbdbbb2 100644 --- a/packages/utils/src/visualization/annotation-metadata.ts +++ b/packages/utils/src/visualization/annotation-metadata.ts @@ -422,14 +422,10 @@ export function annotationLabel(column: string, annotation?: Pick word.startsWith(needle)); + // Compare word-wise so a separator in the query still lines up, and so a + // match can only begin where a word does. + const words = (value: string) => value.toLowerCase().replace(/[_-]+/g, ' '); + return ` ${words(column)}`.includes(` ${words(needle)}`); } /** Source/group for an annotation (registry source, else `Other`). */