diff --git a/app/src/explore/data-renderer.ts b/app/src/explore/data-renderer.ts index 462dd12f..5d7d37f4 100644 --- a/app/src/explore/data-renderer.ts +++ b/app/src/explore/data-renderer.ts @@ -87,6 +87,10 @@ function applyPlotState( plotElement.selectedProteinIds = []; plotElement.selectionMode = false; plotElement.hiddenAnnotationValues = []; + // A query filter is scoped to the previous dataset — clear it so it can't carry + // stale protein ids onto the new dataset. + plotElement.filteredProteinIds = []; + plotElement.filtersActive = false; plotElement.requestUpdate('data', previousData); } diff --git a/docs/explore/control-bar.md b/docs/explore/control-bar.md index 2e5f7bde..a3032904 100644 --- a/docs/explore/control-bar.md +++ b/docs/explore/control-bar.md @@ -25,7 +25,7 @@ Your current projection is reflected in the page URL, so refresh, browser back/f ::: ::: tip 3D Projections -When a 3D projection is available, a **plane selector** (XY / XZ / YZ) appears, letting you view different 2D slices of the 3D space. +3D projections load normally and are shown as their X/Y view; the third (Z) dimension is not rendered in the web viewer. ::: ## 2. Annotation Selector diff --git a/packages/core/src/components/control-bar/control-bar-helpers.test.ts b/packages/core/src/components/control-bar/control-bar-helpers.test.ts index 9b6c1435..d8b6a4ac 100644 --- a/packages/core/src/components/control-bar/control-bar-helpers.test.ts +++ b/packages/core/src/components/control-bar/control-bar-helpers.test.ts @@ -1,8 +1,6 @@ import { describe, it, expect } from 'vitest'; import { EXPORT_DEFAULTS, - isProjection3D, - getProjectionPlane, shouldDisableSelection, getSelectionDisabledMessage, toggleProteinSelection, @@ -92,48 +90,6 @@ describe('control-bar-helpers', () => { }); }); - describe('isProjection3D', () => { - const projectionsMeta = [ - { name: 'projection2D', metadata: { dimension: 2 as const } }, - { name: 'projection3D', metadata: { dimension: 3 as const } }, - { name: 'projectionNoMeta' }, - ]; - - it('returns true for 3D projection', () => { - expect(isProjection3D('projection3D', projectionsMeta)).toBe(true); - }); - - it('returns false for 2D projection', () => { - expect(isProjection3D('projection2D', projectionsMeta)).toBe(false); - }); - - it('returns false for projection without metadata', () => { - expect(isProjection3D('projectionNoMeta', projectionsMeta)).toBe(false); - }); - - it('returns false for non-existent projection', () => { - expect(isProjection3D('nonExistent', projectionsMeta)).toBe(false); - }); - - it('returns false for empty projections array', () => { - expect(isProjection3D('projection3D', [])).toBe(false); - }); - }); - - describe('getProjectionPlane', () => { - it('returns current plane for 3D projection', () => { - expect(getProjectionPlane(true, 'xz')).toBe('xz'); - expect(getProjectionPlane(true, 'yz')).toBe('yz'); - expect(getProjectionPlane(true, 'xy')).toBe('xy'); - }); - - it('returns xy for 2D projection regardless of current plane', () => { - expect(getProjectionPlane(false, 'xz')).toBe('xy'); - expect(getProjectionPlane(false, 'yz')).toBe('xy'); - expect(getProjectionPlane(false, 'xy')).toBe('xy'); - }); - }); - describe('shouldDisableSelection', () => { it('returns true when data size is 0', () => { expect(shouldDisableSelection(0)).toBe(true); diff --git a/packages/core/src/components/control-bar/control-bar-helpers.ts b/packages/core/src/components/control-bar/control-bar-helpers.ts index c94a88d6..dd53d2f9 100644 --- a/packages/core/src/components/control-bar/control-bar-helpers.ts +++ b/packages/core/src/components/control-bar/control-bar-helpers.ts @@ -19,27 +19,6 @@ export const EXPORT_DEFAULTS = { INCLUDE_LEGEND: true, }; -/** - * Check if projection is 3D based on metadata - */ -export function isProjection3D( - projectionName: string, - projectionsMeta: Array<{ name: string; metadata?: { dimension?: 2 | 3 } }>, -): boolean { - const meta = projectionsMeta.find((p) => p.name === projectionName); - return meta?.metadata?.dimension === 3; -} - -/** - * Get appropriate plane for projection - */ -export function getProjectionPlane( - is3D: boolean, - currentPlane: 'xy' | 'xz' | 'yz', -): 'xy' | 'xz' | 'yz' { - return is3D ? currentPlane : 'xy'; -} - /** * Validate selection mode based on data size */ diff --git a/packages/core/src/components/control-bar/control-bar.query-apply.test.ts b/packages/core/src/components/control-bar/control-bar.query-apply.test.ts new file mode 100644 index 00000000..05f3a521 --- /dev/null +++ b/packages/core/src/components/control-bar/control-bar.query-apply.test.ts @@ -0,0 +1,146 @@ +/** + * @vitest-environment jsdom + * + * Behavioural contract for applying a filter query from the control bar. + * + * Regression coverage for the "re-apply shrinks the result" bug (issue #257 and + * the PR #259 report): applying `protein_family = phospholipase A2` matched 546 + * proteins, re-applying the unchanged query matched 19, and a third apply only + * faded points. Root cause: the query was evaluated against the full materialized + * dataset but the matched indices were translated back through the *isolated* + * subset returned by `getCurrentData()`, and every apply stacked another + * isolation layer. + * + * The fix routes a filter query through the dedicated, idempotent + * `filteredProteinIds` / `filtersActive` channel on the scatter plot — a filter + * is not a selection and is not an isolation. These tests pin that contract. + * + * The control bar is created via document.createElement (no WebGL scatter plot + * is mounted); a lightweight stub stands in for the scatter plot so we can + * assert exactly what the apply/reset handlers write. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import './control-bar'; +import type { FilterQuery } from './query-types'; +import type { ProtspaceData } from './types'; + +interface StubScatterplot { + filteredProteinIds?: string[]; + filtersActive?: boolean; + selectedProteinIds?: string[]; + isolateSelection: ReturnType; + resetIsolation: ReturnType; + getCurrentData: ReturnType; + getMaterializedData: ReturnType; + // The control bar treats the scatter plot as an Element (it (de)registers DOM + // listeners on it), so the stub must answer these even though we don't use them. + addEventListener: ReturnType; + removeEventListener: ReturnType; +} + +interface ControlBarInternals extends HTMLElement { + _scatterplotElement: StubScatterplot | null; + _currentData: ProtspaceData | undefined; + filterActive: boolean; + filterQuery: FilterQuery; + _handleQueryApply(event: CustomEvent<{ matchedIndices: Set }>): void; + _handleQueryReset(): void; + updateComplete: Promise; +} + +/** Build a full dataset of `count` proteins: p0, p1, … p{count-1}. */ +function makeFullData(count: number): ProtspaceData { + return { + protein_ids: Array.from({ length: count }, (_, i) => `p${i}`), + }; +} + +function applyEvent(matchedIndices: Set): CustomEvent<{ matchedIndices: Set }> { + return new CustomEvent('query-apply', { detail: { matchedIndices } }); +} + +describe('control-bar filter query apply', () => { + let controlBar: ControlBarInternals; + let scatter: StubScatterplot; + + beforeEach(async () => { + document.body.innerHTML = ''; + controlBar = document.createElement('protspace-control-bar') as ControlBarInternals; + controlBar.autoSync = false; + document.body.appendChild(controlBar); + await controlBar.updateComplete; + + scatter = { + // sentinel selection — must survive a filter apply untouched + selectedProteinIds: ['sentinel'], + isolateSelection: vi.fn(), + resetIsolation: vi.fn(), + // getCurrentData returns the *isolated subset*. The old buggy code used this + // to translate matched indices; the fix must never read it for translation. + getCurrentData: vi.fn(() => ({ protein_ids: ['p0', 'p1'] })), + getMaterializedData: vi.fn(() => makeFullData(100)), + addEventListener: vi.fn(), + removeEventListener: vi.fn(), + }; + + controlBar._scatterplotElement = scatter; + // The query builder evaluates against the full materialized data, exposed as + // _currentData. Matched indices are positions in THIS array. + controlBar._currentData = makeFullData(100); + }); + + it('applies a query via the filter channel without selecting or isolating', () => { + // family "A" = first 30 proteins + const matched = new Set(Array.from({ length: 30 }, (_, i) => i)); + + controlBar._handleQueryApply(applyEvent(matched)); + + const expectedIds = Array.from({ length: 30 }, (_, i) => `p${i}`); + expect(scatter.filteredProteinIds).toEqual(expectedIds); + expect(scatter.filtersActive).toBe(true); + expect(controlBar.filterActive).toBe(true); + + // A filter is not an isolation and not a selection. + expect(scatter.isolateSelection).not.toHaveBeenCalled(); + expect(scatter.selectedProteinIds).toEqual(['sentinel']); + }); + + it('is idempotent: re-applying the same query yields the same matches', () => { + const matched = new Set(Array.from({ length: 30 }, (_, i) => i)); + const expectedIds = Array.from({ length: 30 }, (_, i) => `p${i}`); + + controlBar._handleQueryApply(applyEvent(matched)); + expect(scatter.filteredProteinIds).toEqual(expectedIds); + + // Second apply with the unchanged query — must NOT shrink (was 30 → 19 → fade). + controlBar._handleQueryApply(applyEvent(new Set(matched))); + expect(scatter.filteredProteinIds).toEqual(expectedIds); + expect(scatter.filtersActive).toBe(true); + + // Third apply — still stable, still no isolation stacking. + controlBar._handleQueryApply(applyEvent(new Set(matched))); + expect(scatter.filteredProteinIds).toEqual(expectedIds); + expect(scatter.isolateSelection).not.toHaveBeenCalled(); + }); + + it('replaces (does not stack) when a narrower query is applied next', () => { + controlBar._handleQueryApply(applyEvent(new Set(Array.from({ length: 30 }, (_, i) => i)))); + expect(scatter.filteredProteinIds).toHaveLength(30); + + controlBar._handleQueryApply(applyEvent(new Set(Array.from({ length: 12 }, (_, i) => i)))); + expect(scatter.filteredProteinIds).toEqual(Array.from({ length: 12 }, (_, i) => `p${i}`)); + }); + + it('clears the filter channel on reset, leaving manual isolation alone', () => { + controlBar._handleQueryApply(applyEvent(new Set([0, 1, 2]))); + expect(scatter.filtersActive).toBe(true); + + controlBar._handleQueryReset(); + + expect(scatter.filteredProteinIds).toEqual([]); + expect(scatter.filtersActive).toBe(false); + expect(controlBar.filterActive).toBe(false); + // Reset re-seeds an empty condition row so the builder shows a fresh query. + expect(controlBar.filterQuery).toHaveLength(1); + }); +}); diff --git a/packages/core/src/components/control-bar/control-bar.ts b/packages/core/src/components/control-bar/control-bar.ts index 5d9a1683..6f7cc64a 100644 --- a/packages/core/src/components/control-bar/control-bar.ts +++ b/packages/core/src/components/control-bar/control-bar.ts @@ -12,8 +12,6 @@ import type { import { handleDropdownEscape, isAnyDropdownOpen } from '../../utils/dropdown-helpers'; import { EXPORT_DEFAULTS, - isProjection3D, - getProjectionPlane, toggleProteinSelection, mergeProteinSelections, } from './control-bar-helpers'; @@ -43,8 +41,6 @@ export class ProtspaceControlBar extends LitElement { @property({ type: String, attribute: 'selected-annotation' }) selectedAnnotation: string = ''; @property({ type: Array }) tooltipAnnotations: string[] = []; - @property({ type: String, attribute: 'projection-plane' }) - projectionPlane: 'xy' | 'xz' | 'yz' = 'xy'; @property({ type: Boolean, attribute: 'selection-mode' }) selectionMode: boolean = false; @property({ type: String, attribute: 'selection-tool' }) @@ -142,13 +138,6 @@ export class ProtspaceControlBar extends LitElement { if (projectionIndex !== -1 && 'selectedProjectionIndex' in this._scatterplotElement) { (this._scatterplotElement as ScatterplotElementLike).selectedProjectionIndex = projectionIndex; - - const is3D = isProjection3D(this.selectedProjection, this.projectionsMeta); - const nextPlane = getProjectionPlane(is3D, this.projectionPlane); - if ('projectionPlane' in this._scatterplotElement) { - (this._scatterplotElement as ScatterplotElementLike).projectionPlane = nextPlane; - } - this.projectionPlane = nextPlane; } } @@ -267,25 +256,6 @@ export class ProtspaceControlBar extends LitElement { }); } - private handlePlaneChange(event: Event) { - const target = event.target as HTMLSelectElement; - const plane = target.value as 'xy' | 'xz' | 'yz'; - if ( - this.autoSync && - this._scatterplotElement && - 'projectionPlane' in this._scatterplotElement - ) { - (this._scatterplotElement as ScatterplotElementLike).projectionPlane = plane; - this.projectionPlane = plane; - } - const customEvent = new CustomEvent('projection-plane-change', { - detail: { plane }, - bubbles: true, - composed: true, - }); - this.dispatchEvent(customEvent); - } - applyAnnotationSelection(annotation: string) { this.selectedAnnotation = annotation; @@ -537,6 +507,11 @@ export class ProtspaceControlBar extends LitElement { public clearForNewDataset(_datasetHash: string, _clearPersistedState: boolean = true): void { this.exportFormat = EXPORT_DEFAULTS.FORMAT; + // A new dataset has different protein ids, so any active filter is stale. Clear + // the badge/query here (the canonical per-dataset reset hook); the scatter plot's + // filter channel is reset in parallel by applyPlotState. + this.filterQuery = []; + this.filterActive = false; } private openFileDialog() { @@ -617,25 +592,6 @@ export class ProtspaceControlBar extends LitElement { - ${(() => { - return isProjection3D(this.selectedProjection, this.projectionsMeta) - ? html` -
- - -
- ` - : null; - })()} -
@@ -1613,19 +1569,22 @@ export class ProtspaceControlBar extends LitElement { private _handleQueryApply(e: CustomEvent<{ matchedIndices: Set }>) { if (!this._scatterplotElement) return; const sp = this._scatterplotElement as ScatterplotElementLike; - const data = sp.getCurrentData?.(); - const proteinIds = data?.protein_ids; + // matchedIndices are positions in the exact array the query was evaluated + // against — `_currentData`, the full materialized snapshot handed to the query + // builder. Mapping through any other array (e.g. getCurrentData(), the isolated + // subset) mis-resolves the indices and shrank the result on every re-apply (#257). + const proteinIds = this._currentData?.protein_ids; if (!proteinIds) return; - // Convert indices to protein IDs - const matchedIds = Array.from(e.detail.matchedIndices).map((i) => proteinIds[i]); + const matchedIds = Array.from(e.detail.matchedIndices) + .map((i) => proteinIds[i]) + .filter((id): id is string => id !== undefined); - // Set selection and isolate (dispatch event for downstream listeners) - sp.selectedProteinIds = matchedIds; - this.dispatchEvent( - new CustomEvent('isolate-data', { detail: {}, bubbles: true, composed: true }), - ); - sp.isolateSelection?.(); + // A filter is not a selection and not an isolation: route it through the + // dedicated, idempotent filteredProteinIds channel on the scatter plot so that + // re-applying the same query is a no-op rather than stacking isolation layers. + sp.filteredProteinIds = matchedIds; + sp.filtersActive = true; this.filterActive = true; this.showFilterMenu = false; @@ -1634,15 +1593,13 @@ export class ProtspaceControlBar extends LitElement { private _handleQueryReset() { if (!this._scatterplotElement) return; const sp = this._scatterplotElement as ScatterplotElementLike; - this.dispatchEvent( - new CustomEvent('reset-isolation', { detail: {}, bubbles: true, composed: true }), - ); - sp.resetIsolation?.(); + // Clear only the filter channel; any manual isolation the user created + // independently of the filter is left untouched. + sp.filteredProteinIds = []; + sp.filtersActive = false; this.filterQuery = [createCondition()]; this.filterActive = false; - this.isolationMode = false; - this.isolationHistory = []; // Do NOT close popover -- user may want to start a new query } } diff --git a/packages/core/src/components/control-bar/query-builder.styles.ts b/packages/core/src/components/control-bar/query-builder.styles.ts index 01700201..65be8966 100644 --- a/packages/core/src/components/control-bar/query-builder.styles.ts +++ b/packages/core/src/components/control-bar/query-builder.styles.ts @@ -484,6 +484,62 @@ export const queryBuilderStyles = css` overflow: visible; } + /* ========================================== + NUMERIC INPUT + ========================================== */ + + .numeric-input { + display: flex; + align-items: center; + gap: var(--spacing-xs); + flex-wrap: wrap; + } + + .numeric-operator-select { + padding: var(--input-padding-y) 18px var(--input-padding-y) var(--spacing-xs); + border: var(--border-width) solid var(--border); + border-radius: var(--radius); + background-color: var(--surface); + background-image: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' width='12' height='12' viewBox='0 0 24 24' fill='none' stroke='%235b6b7a' stroke-width='2' stroke-linecap='round' stroke-linejoin='round'%3E%3Cpath d='M6 9l6 6 6-6'/%3E%3C/svg%3E"); + background-repeat: no-repeat; + background-position: right 4px center; + background-size: 12px; + font-size: var(--text-sm); + color: var(--text-primary); + cursor: pointer; + appearance: none; + -webkit-appearance: none; + } + + .numeric-field { + width: 90px; + padding: var(--input-padding-y) var(--input-padding-x); + border: var(--border-width) solid var(--border); + border-radius: var(--radius); + background: var(--surface); + font-size: var(--text-base); + color: var(--text-primary); + box-sizing: border-box; + } + + .numeric-field:focus-visible { + outline: none; + border-color: var(--primary); + box-shadow: + 0 0 0 1px var(--primary), + 0 0 0 3px var(--focus-ring-bg); + } + + .numeric-dash { + color: var(--text-secondary); + } + + .numeric-match-count { + font-size: var(--text-sm); + color: var(--text-secondary); + white-space: nowrap; + } + /* ========================================== RESPONSIVE ========================================== */ diff --git a/packages/core/src/components/control-bar/query-builder.ts b/packages/core/src/components/control-bar/query-builder.ts index c1272cdd..9b4cbe34 100644 --- a/packages/core/src/components/control-bar/query-builder.ts +++ b/packages/core/src/components/control-bar/query-builder.ts @@ -15,7 +15,7 @@ import './query-condition-row'; * * Events: * - `query-changed` — dispatched whenever the query changes, detail: `{ query: FilterQuery }` - * - `query-apply` — dispatched when "Apply & Isolate" is clicked, detail: `{ matchedIndices: Set }` + * - `query-apply` — dispatched when "Apply Filter" is clicked, detail: `{ matchedIndices: Set }` * - `query-reset` — dispatched when "Reset All" is clicked */ @customElement('protspace-query-builder') @@ -322,7 +322,7 @@ class ProtspaceQueryBuilder extends LitElement { ?disabled=${this._matchedIndices.size === 0 || this.query.length === 0} @click=${this._handleApply} > - Apply & Isolate + Apply Filter
diff --git a/packages/core/src/components/control-bar/query-condition-row.test.ts b/packages/core/src/components/control-bar/query-condition-row.test.ts new file mode 100644 index 00000000..1fe0ac92 --- /dev/null +++ b/packages/core/src/components/control-bar/query-condition-row.test.ts @@ -0,0 +1,80 @@ +/** + * @vitest-environment jsdom + * + * Picking an annotation must produce a condition whose kind matches the + * annotation's type. The tricky case is a numeric annotation that has been + * materialized for the legend: it becomes kind:'categorical' but keeps + * sourceKind:'numeric', and must still be filtered with the numeric range input. + */ +import { beforeEach, describe, expect, it } from 'vitest'; +import './query-condition-row'; +import type { FilterCondition } from './query-types'; +import type { ProtspaceData } from './types'; + +interface ConditionRowEl extends HTMLElement { + condition: FilterCondition; + annotations: string[]; + data: ProtspaceData; + updateComplete: Promise; + _selectAnnotation(annotation: string): void; +} + +const data: ProtspaceData = { + protein_ids: ['P1', 'P2'], + annotations: { + organism: { kind: 'categorical', values: ['Human', 'Mouse'] }, + // A genuine, unmaterialized numeric annotation. + length: { kind: 'numeric', values: [] }, + // A numeric annotation after legend materialization: binned to categorical, + // but still numeric at heart (sourceKind). + mass: { kind: 'categorical', sourceKind: 'numeric', values: ['0–10', '10–20'] }, + }, + annotation_data: { organism: [[0], [1]] }, + numeric_annotation_data: { length: [100, 200], mass: [5, 15] }, +}; + +async function mount(): Promise { + document.body.innerHTML = ''; + const el = document.createElement('protspace-query-condition-row') as ConditionRowEl; + el.condition = { id: 'c1', kind: 'categorical', annotation: '', values: [] }; + el.annotations = ['organism', 'length', 'mass']; + el.data = data; + document.body.appendChild(el); + await el.updateComplete; + return el; +} + +async function kindAfterSelecting(el: ConditionRowEl, annotation: string): Promise { + let kind = ''; + el.addEventListener( + 'condition-changed', + (e) => { + kind = (e as CustomEvent<{ condition: FilterCondition }>).detail.condition.kind; + }, + { once: true }, + ); + el._selectAnnotation(annotation); + await el.updateComplete; + return kind; +} + +describe('query-condition-row annotation kind detection', () => { + beforeEach(() => { + document.body.innerHTML = ''; + }); + + it('builds a categorical condition for a categorical annotation', async () => { + const el = await mount(); + expect(await kindAfterSelecting(el, 'organism')).toBe('categorical'); + }); + + it('builds a numeric condition for a numeric annotation', async () => { + const el = await mount(); + expect(await kindAfterSelecting(el, 'length')).toBe('numeric'); + }); + + it('builds a numeric condition for a materialized (sourceKind:numeric) annotation', async () => { + const el = await mount(); + expect(await kindAfterSelecting(el, 'mass')).toBe('numeric'); + }); +}); 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 8072dee2..f9cc0ef2 100644 --- a/packages/core/src/components/control-bar/query-condition-row.ts +++ b/packages/core/src/components/control-bar/query-condition-row.ts @@ -1,12 +1,14 @@ import { LitElement, html, nothing } from 'lit'; import { property, state, query as litQuery } from 'lit/decorators.js'; import { customElement } from '../../utils/safe-custom-element'; -import type { FilterCondition, LogicalOp } from './query-types'; +import type { FilterCondition, LogicalOp, NumericCondition } from './query-types'; +import { createCondition, createNumericCondition } from './query-types'; import type { ProtspaceData } from './types'; import { groupAnnotations } from './annotation-categories'; -import { NA_VALUE, NA_DISPLAY } from '@protspace/utils'; +import { NA_VALUE, NA_DISPLAY, isNumericAnnotation } from '@protspace/utils'; import { queryBuilderStyles } from './query-builder.styles'; import './query-value-picker'; +import './query-numeric-input'; /** * Renders a single query condition row. @@ -111,8 +113,19 @@ class ProtspaceQueryConditionRow extends LitElement { private _selectAnnotation(annotation: string) { this._showAnnotationPicker = false; this._annotationSearch = ''; - // Clear values when annotation changes - this._dispatchChanged({ ...this.condition, annotation, values: [] }); + this._showValuePicker = false; + // Replace the whole condition object so its kind matches the annotation. + const base = { + id: this.condition.id, + logicalOp: this.condition.logicalOp, + annotation, + }; + // Use the shared numeric check so that a numeric annotation is still treated as + // numeric after materialization bins it to kind:'categorical' (it keeps + // sourceKind:'numeric'); otherwise the currently-coloured numeric annotation + // would offer a categorical bin picker instead of the numeric range input. + const isNumeric = isNumericAnnotation(this.data?.annotations?.[annotation]); + this._dispatchChanged(isNumeric ? createNumericCondition(base) : createCondition(base)); } private _handleLogicalOpChange(e: Event) { @@ -122,11 +135,13 @@ class ProtspaceQueryConditionRow extends LitElement { } private _removeValue(value: string) { + if (this.condition.kind !== 'categorical') return; const values = this.condition.values.filter((v) => v !== value); this._dispatchChanged({ ...this.condition, values }); } private _handleValueSelected(e: CustomEvent<{ value: string }>) { + if (this.condition.kind !== 'categorical') return; const value = e.detail.value; if (!this.condition.values.includes(value)) { this._dispatchChanged({ ...this.condition, values: [...this.condition.values, value] }); @@ -199,6 +214,7 @@ class ProtspaceQueryConditionRow extends LitElement { } private _renderValues() { + if (this.condition.kind !== 'categorical') return nothing; return html`
${this.condition.values.map( @@ -245,6 +261,21 @@ class ProtspaceQueryConditionRow extends LitElement { `; } + private _handleNumericChanged(e: CustomEvent<{ condition: NumericCondition }>) { + this._dispatchChanged(e.detail.condition); + } + + private _renderNumericInput() { + if (this.condition.kind !== 'numeric') return nothing; + return html` + + `; + } + // ─── Render ─────────────────────────────────────────────────────────────── render() { @@ -278,7 +309,7 @@ class ProtspaceQueryConditionRow extends LitElement { ${this._showAnnotationPicker ? this._renderAnnotationPicker() : nothing} - ${this._renderValues()} + ${this.condition.kind === 'numeric' ? this._renderNumericInput() : this._renderValues()}
+ `; + } +} + +declare global { + interface HTMLElementTagNameMap { + 'protspace-query-numeric-input': ProtspaceQueryNumericInput; + } +} diff --git a/packages/core/src/components/control-bar/query-types.ts b/packages/core/src/components/control-bar/query-types.ts index 58972c80..47e9a290 100644 --- a/packages/core/src/components/control-bar/query-types.ts +++ b/packages/core/src/components/control-bar/query-types.ts @@ -1,12 +1,26 @@ export type LogicalOp = 'AND' | 'OR' | 'NOT'; +export type NumericOperator = 'gt' | 'lt' | 'between'; -export interface FilterCondition { +interface BaseCondition { id: string; logicalOp?: LogicalOp; annotation: string; +} + +export interface CategoricalCondition extends BaseCondition { + kind: 'categorical'; values: string[]; } +export interface NumericCondition extends BaseCondition { + kind: 'numeric'; + operator: NumericOperator; + min: number | null; + max: number | null; +} + +export type FilterCondition = CategoricalCondition | NumericCondition; + export interface FilterGroup { id: string; logicalOp?: LogicalOp; @@ -22,19 +36,33 @@ function generateId(): string { return `q-${Date.now()}-${nextId++}`; } -export function createCondition(overrides?: Partial): FilterCondition { +export function createCondition(overrides?: Partial): CategoricalCondition { return { id: generateId(), + kind: 'categorical', annotation: '', values: [], ...overrides, }; } +export function createNumericCondition(overrides?: Partial): NumericCondition { + return { + id: generateId(), + kind: 'numeric', + annotation: '', + operator: 'gt', + min: null, + max: null, + ...overrides, + }; +} + export function createGroup(overrides?: Partial): FilterGroup { return { id: generateId(), - logicalOp: 'AND', + // No leading logicalOp by default, mirroring createCondition — the builder + // sets one only for a group that is not first (query-builder._addGroup). conditions: [createCondition()], ...overrides, }; diff --git a/packages/core/src/components/control-bar/types.ts b/packages/core/src/components/control-bar/types.ts index 834a469f..a7ec1d3c 100644 --- a/packages/core/src/components/control-bar/types.ts +++ b/packages/core/src/components/control-bar/types.ts @@ -55,7 +55,6 @@ export interface ScatterplotElementLike extends Element { selectionMode?: boolean; selectionTool?: 'rectangle' | 'lasso'; selectedProteinIds?: string[]; - projectionPlane?: 'xy' | 'xz' | 'yz'; data?: ProtspaceData; filteredProteinIds?: string[]; filtersActive?: boolean; diff --git a/packages/core/src/components/scatter-plot/scatter-plot.filter-render.test.ts b/packages/core/src/components/scatter-plot/scatter-plot.filter-render.test.ts new file mode 100644 index 00000000..d36feca5 --- /dev/null +++ b/packages/core/src/components/scatter-plot/scatter-plot.filter-render.test.ts @@ -0,0 +1,117 @@ +/** + * @vitest-environment jsdom + * + * Rendering-integrity regression for the query-filter channel (#257 follow-up). + * + * When a query filter is active, the scatter plot builds `_plotData` from the + * matched subset. Each PlotDataPoint.originalIndex MUST still address the full + * dataset, because the style getters (colors/shape/opacity) and the tooltip + * path resolve annotation values against the full data by that index — exactly + * as the isolation path already does. If a filter renumbers originalIndex to a + * slice-local 0..N-1, then a non-prefix filter (one that drops earlier proteins) + * paints kept points with the WRONG protein's colour and can even hide them. + * + * Construct the element via createElement without appending it (so Lit's + * connectedCallback / WebGL init never runs — same approach as + * scatter-plot.isolation.test.ts) and drive _processData directly. + */ +import { vi, describe, it, expect } from 'vitest'; +import type { PlotDataPoint, VisualizationData } from '@protspace/utils'; + +vi.hoisted(() => { + if (!('ResizeObserver' in globalThis)) { + (globalThis as unknown as { ResizeObserver: unknown }).ResizeObserver = class { + observe() {} + unobserve() {} + disconnect() {} + }; + } +}); + +import './scatter-plot'; + +const RED = '#ff0000'; +const GREEN = '#00ff00'; + +type ScatterplotInternals = HTMLElement & { + data: VisualizationData; + selectedAnnotation: string; + filteredProteinIds: string[]; + filtersActive: boolean; + _plotData: PlotDataPoint[]; + _processData(): void; + _buildStyleGetters(): { getColors(point: PlotDataPoint): string[] }; +}; + +/** + * Six proteins. p0–p2 are family "A" (red), p3–p5 are family "B" (green). + * annotation_data rows hold an index into `annotations.fam.values`, and + * valueToColor is derived from values↔colors positionally, so A→red, B→green. + */ +function makeFamilyData(): VisualizationData { + const families = ['A', 'A', 'A', 'B', 'B', 'B']; + const colorFor = (v: string) => (v === 'A' ? RED : GREEN); + return { + protein_ids: families.map((_, i) => `p${i}`), + projections: [{ name: 'umap', data: families.map((_, i) => [i, i]) }], + annotations: { + fam: { + values: families, + colors: families.map(colorFor), + shapes: families.map(() => 'circle'), + }, + }, + annotation_data: { + // each row points at the first index of its family value in `values` + fam: families.map((v) => [families.indexOf(v)]), + }, + } as unknown as VisualizationData; +} + +function makeScatter(): ScatterplotInternals { + const sp = document.createElement('protspace-scatterplot') as ScatterplotInternals; + sp.data = makeFamilyData(); + sp.selectedAnnotation = 'fam'; + return sp; +} + +describe('scatter-plot query-filter rendering integrity', () => { + it('colours a non-prefix filtered subset by each point’s OWN value', () => { + const sp = makeScatter(); + // Keep only family B — a non-prefix subset (drops p0–p2). + sp.filteredProteinIds = ['p3', 'p4', 'p5']; + sp.filtersActive = true; + + sp._processData(); + const getters = sp._buildStyleGetters(); + + expect(sp._plotData.map((p) => p.id).sort()).toEqual(['p3', 'p4', 'p5']); + + // Every kept point is family B → must be green, not the colour of the + // protein sitting at the same slice-local position in the full dataset. + for (const point of sp._plotData) { + expect(getters.getColors(point)).toEqual([GREEN]); + } + }); + + it('still colours a prefix filter and the unfiltered plot correctly', () => { + // Prefix subset (p0–p2, family A) — happens to work even with the bug, so + // this guards against an over-correction that breaks the easy case. + const prefix = makeScatter(); + prefix.filteredProteinIds = ['p0', 'p1', 'p2']; + prefix.filtersActive = true; + prefix._processData(); + const prefixGetters = prefix._buildStyleGetters(); + for (const point of prefix._plotData) { + expect(prefixGetters.getColors(point)).toEqual([RED]); + } + + // No filter at all — full plot, both families correct. + const full = makeScatter(); + full._processData(); + const fullGetters = full._buildStyleGetters(); + const colorById = new Map(full._plotData.map((p) => [p.id, fullGetters.getColors(p)])); + expect(colorById.get('p0')).toEqual([RED]); + expect(colorById.get('p5')).toEqual([GREEN]); + }); +}); diff --git a/packages/core/src/components/scatter-plot/scatter-plot.ts b/packages/core/src/components/scatter-plot/scatter-plot.ts index 93b3ba12..86709311 100644 --- a/packages/core/src/components/scatter-plot/scatter-plot.ts +++ b/packages/core/src/components/scatter-plot/scatter-plot.ts @@ -62,7 +62,6 @@ export class ProtspaceScatterplot extends LitElement { // Properties @property({ type: Object }) data: VisualizationData | null = null; @property({ type: Number }) selectedProjectionIndex = 0; - @property({ type: String }) projectionPlane: 'xy' | 'xz' | 'yz' = 'xy'; @property({ type: String }) selectedAnnotation = 'family'; @property({ type: Array }) tooltipAnnotations: string[] = []; @property({ type: Array }) highlightedProteinIds: string[] = []; @@ -448,20 +447,27 @@ export class ProtspaceScatterplot extends LitElement { } } + // A query filter is scoped to the current dataset. On a dataset swap, drop the + // filtered-id set before _processData runs below — otherwise a stale set (ids + // from the previous dataset) would match nothing and blank the new plot. Set + // here (not via a getter) so the synchronous _processData read sees it cleared. + if (changedProperties.has('data') && this.filtersActive) { + this.filteredProteinIds = []; + this.filtersActive = false; + } + const numericSettingsChangedOnly = changedProperties.has('numericAnnotationSettings') && !changedProperties.has('data') && !changedProperties.has('filteredProteinIds') && !changedProperties.has('filtersActive') && - !changedProperties.has('selectedProjectionIndex') && - !changedProperties.has('projectionPlane'); + !changedProperties.has('selectedProjectionIndex'); if ( changedProperties.has('data') || changedProperties.has('filteredProteinIds') || changedProperties.has('filtersActive') || - changedProperties.has('selectedProjectionIndex') || - changedProperties.has('projectionPlane') + changedProperties.has('selectedProjectionIndex') ) { this._processData(); this._scheduleQuadtreeRebuild(); @@ -642,18 +648,29 @@ export class ProtspaceScatterplot extends LitElement { } private _processData() { - const dataToUse = this._getCurrentDisplayData(); + // Build _plotData from the FULL materialized data and apply the query filter + // as an id-membership filter (see processVisualizationData) rather than from a + // pre-sliced display array. This keeps each point's originalIndex a GLOBAL + // index into the full dataset, which the style getters and tooltip path + // require — a slice-local index would mis-resolve colours/values under any + // non-prefix filter. Isolation already worked this way; filtering now matches. + const dataToUse = this._getMaterializedData(); if (!dataToUse) return; - // Detect whether only the projection changed (same data reference, not in isolation mode). - // In this case we take the fast path: update coordinates in-place - // instead of rebuilding all PlotDataPoint objects with annotation data (~700MB for 500k proteins). + const visibleProteinIds = this._getVisibleProteinIdsSet(); + + // Fast path applies only to a projection change on the plain, unfiltered, + // non-isolated plot. Whenever a filter or isolation is active we rebuild so + // the kept set is recomputed (and originalIndex stays global). const onlyProjectionChanged = - this._plotData.length > 0 && this._lastDataRef === dataToUse && !this._isolationMode; + this._plotData.length > 0 && + this._lastDataRef === dataToUse && + !this._isolationMode && + !this.filtersActive; if (onlyProjectionChanged) { // Fast path: update coordinates in-place from the new projection data. - // No new object allocation — just overwrite x/y/z on existing PlotDataPoints. + // No new object allocation — just overwrite x/y on existing PlotDataPoints. this._updatePlotDataCoordinates(dataToUse); } else { // Release old data references before allocating the new dataset. @@ -669,7 +686,7 @@ export class ProtspaceScatterplot extends LitElement { this.selectedProjectionIndex, this._isolationMode, this._isolationHistory, - this.projectionPlane, + visibleProteinIds, ); } @@ -844,18 +861,8 @@ export class ProtspaceScatterplot extends LitElement { | [number, number] | [number, number, number]; - let xVal = coords[0]; - let yVal = coords[1]; - - if (coords.length === 3) { - point.z = coords[2]; - if (this.projectionPlane === 'xz') { - yVal = coords[2]; - } else if (this.projectionPlane === 'yz') { - xVal = coords[1]; - yVal = coords[2]; - } - } + const xVal = coords[0]; + const yVal = coords[1]; point.x = xVal; point.y = yVal; diff --git a/packages/core/src/components/scatter-plot/style-getters.test.ts b/packages/core/src/components/scatter-plot/style-getters.test.ts index becc64c6..9ddec109 100644 --- a/packages/core/src/components/scatter-plot/style-getters.test.ts +++ b/packages/core/src/components/scatter-plot/style-getters.test.ts @@ -32,7 +32,6 @@ describe('style-getters', () => { id, x: 0, y: 0, - z: 0, originalIndex, }); @@ -182,7 +181,6 @@ describe('style-getters', () => { id, x: 0, y: 0, - z: 0, originalIndex, }); @@ -321,7 +319,6 @@ describe('style-getters', () => { id: 'test_protein', x: 0, y: 0, - z: 0, originalIndex, }); diff --git a/packages/utils/src/types.ts b/packages/utils/src/types.ts index c84edf04..e6591c99 100644 --- a/packages/utils/src/types.ts +++ b/packages/utils/src/types.ts @@ -63,7 +63,6 @@ export interface PlotDataPoint { id: string; x: number; y: number; - z?: number; originalIndex: number; } diff --git a/packages/utils/src/visualization/data-processor.test.ts b/packages/utils/src/visualization/data-processor.test.ts index 581677d0..c388979b 100644 --- a/packages/utils/src/visualization/data-processor.test.ts +++ b/packages/utils/src/visualization/data-processor.test.ts @@ -24,7 +24,7 @@ describe('DataProcessor.processVisualizationData', () => { expect(result[1]).toEqual({ id: 'p1', x: 3, y: 4, originalIndex: 1 }); }); - it('preserves z coordinate for 3D projections', () => { + it('drops the z coordinate for 3D projections (rendered as 2D)', () => { const data: VisualizationData = { protein_ids: ['p0'], projections: [{ name: 't', data: [[1, 2, 3]] }], @@ -32,29 +32,8 @@ describe('DataProcessor.processVisualizationData', () => { annotation_data: {}, }; const result = DataProcessor.processVisualizationData(data, 0); - expect(result[0]).toEqual({ id: 'p0', x: 1, y: 2, z: 3, originalIndex: 0 }); - }); - - it('maps coordinates to xz plane when projectionPlane is "xz"', () => { - const data: VisualizationData = { - protein_ids: ['p0'], - projections: [{ name: 't', data: [[10, 20, 30]] }], - annotations: {}, - annotation_data: {}, - }; - const result = DataProcessor.processVisualizationData(data, 0, false, undefined, 'xz'); - expect(result[0]).toEqual({ id: 'p0', x: 10, y: 30, z: 30, originalIndex: 0 }); - }); - - it('maps coordinates to yz plane when projectionPlane is "yz"', () => { - const data: VisualizationData = { - protein_ids: ['p0'], - projections: [{ name: 't', data: [[10, 20, 30]] }], - annotations: {}, - annotation_data: {}, - }; - const result = DataProcessor.processVisualizationData(data, 0, false, undefined, 'yz'); - expect(result[0]).toEqual({ id: 'p0', x: 20, y: 30, z: 30, originalIndex: 0 }); + expect(result[0]).toEqual({ id: 'p0', x: 1, y: 2, originalIndex: 0 }); + expect(result[0]).not.toHaveProperty('z'); }); it('returns empty array when projection index is out of range', () => { diff --git a/packages/utils/src/visualization/data-processor.ts b/packages/utils/src/visualization/data-processor.ts index c34ae7f7..94cd290e 100644 --- a/packages/utils/src/visualization/data-processor.ts +++ b/packages/utils/src/visualization/data-processor.ts @@ -7,7 +7,7 @@ export class DataProcessor { projectionIndex: number, isolationMode: boolean = false, isolationHistory?: string[][], - projectionPlane: 'xy' | 'xz' | 'yz' = 'xy', + visibleProteinIds?: Set | null, ): PlotDataPoint[] { if (!data.projections[projectionIndex]) return []; @@ -16,30 +16,29 @@ export class DataProcessor { | [number, number] | [number, number, number]; - let xVal = coordinates[0]; - let yVal = coordinates[1]; - if (coordinates.length === 3) { - if (projectionPlane === 'xz') { - yVal = coordinates[2]; - } else if (projectionPlane === 'yz') { - xVal = coordinates[1]; - yVal = coordinates[2]; - } - return { id, x: xVal, y: yVal, z: coordinates[2], originalIndex: index }; - } - return { id, x: xVal, y: yVal, originalIndex: index }; + return { id, x: coordinates[0], y: coordinates[1], originalIndex: index }; }); + // Apply the query filter (and then isolation) as id-membership filters AFTER + // the map, so every kept point keeps its GLOBAL `originalIndex`. Style getters + // and tooltips resolve annotation values against the full dataset by that + // index, so a slice-local index would mis-resolve points under a non-prefix + // filter. This mirrors how isolation has always preserved the global index. + let result = processedData; + + if (visibleProteinIds) { + result = result.filter((p) => visibleProteinIds.has(p.id)); + } + if (isolationMode && isolationHistory && isolationHistory.length > 0) { - let filteredData = processedData.filter((p) => isolationHistory[0].includes(p.id)); + result = result.filter((p) => isolationHistory[0].includes(p.id)); for (let i = 1; i < isolationHistory.length; i++) { const splitIds = isolationHistory[i]; - filteredData = filteredData.filter((p) => splitIds.includes(p.id)); + result = result.filter((p) => splitIds.includes(p.id)); } - return filteredData; } - return processedData; + return result; } static createScales(