From 91df7681b14f185972dbc57c4fd2a07b31a685ed Mon Sep 17 00:00:00 2001 From: tsenoner Date: Tue, 16 Jun 2026 20:28:33 +0200 Subject: [PATCH 1/7] fix(legend): persist per-annotation category visibility across switches Switching annotations called clearPersistedLegendHiddenValues, which rewrote the previous annotation's legend localStorage entry to an empty hiddenValues array, so hidden categories reappeared on switch-back. The core legend already persists and restores hiddenValues per datasetHash + annotation, so remove the clearing call and its now-dead plumbing (the persisted-legend controller, its runtime wiring, and the lastKnownAnnotation tracking it required). The demo still resets on a full reload via the separate clearForNewDataset path, so it always returns to its original state. Closes #281 Co-Authored-By: Claude Opus 4.8 (1M context) --- app/src/explore/control-bar-events.ts | 13 +---- app/src/explore/dataset-controller.ts | 6 +-- app/src/explore/interaction-controller.ts | 21 +++----- app/src/explore/persisted-legend.ts | 66 ----------------------- app/src/explore/runtime.ts | 8 --- app/tests/url-view-state.spec.ts | 35 +++++++++++- 6 files changed, 42 insertions(+), 107 deletions(-) delete mode 100644 app/src/explore/persisted-legend.ts diff --git a/app/src/explore/control-bar-events.ts b/app/src/explore/control-bar-events.ts index 5b1940b3..d558d3bc 100644 --- a/app/src/explore/control-bar-events.ts +++ b/app/src/explore/control-bar-events.ts @@ -1,8 +1,4 @@ -import type { - ProtspaceControlBar, - ProtspaceScatterplot, - SelectionDisabledNotificationDetail, -} from '@protspace/core'; +import type { SelectionDisabledNotificationDetail } from '@protspace/core'; import { notify } from '../lib/notify'; import { getSelectionDisabledNotification } from './notifications'; import type { DatasetController } from './dataset-controller'; @@ -15,26 +11,21 @@ interface ControlBarEventsOptions { listener: EventListenerOrEventListenerObject, options?: boolean | AddEventListenerOptions, ): void; - controlBar: ProtspaceControlBar; datasetController: DatasetController; handleExport(event: Event): Promise; interactionController: InteractionController; - plotElement: ProtspaceScatterplot; viewController: ViewController; } export function bindControlBarEvents({ addControlBarListener, - controlBar, datasetController, handleExport, interactionController, - plotElement, viewController, }: ControlBarEventsOptions): void { addControlBarListener('annotation-change', () => { - const nextAnnotation = controlBar.selectedAnnotation || plotElement.selectedAnnotation || ''; - interactionController.handleAnnotationChange(nextAnnotation); + interactionController.handleAnnotationChange(); viewController.handleUserAnnotationChange(); }); diff --git a/app/src/explore/dataset-controller.ts b/app/src/explore/dataset-controller.ts index 93dd8e7a..0cbc5668 100644 --- a/app/src/explore/dataset-controller.ts +++ b/app/src/explore/dataset-controller.ts @@ -138,11 +138,7 @@ export function createDatasetController({ legendElement.clearForNewDataset(datasetHash, shouldClearPersistedState); controlBar.clearForNewDataset(datasetHash, shouldClearPersistedState); - const initialViewFallback = Object.keys(data.annotations)[0] ?? ''; - interactionController.setLastKnownAnnotation(initialViewFallback); - - const initialView = await loadData(data); - interactionController.setLastKnownAnnotation(initialView?.annotation ?? initialViewFallback); + await loadData(data); const shouldApplyEmbeddedFileSettings = settings && loadMeta.kind !== 'opfs'; if (shouldApplyEmbeddedFileSettings) { diff --git a/app/src/explore/interaction-controller.ts b/app/src/explore/interaction-controller.ts index a91b1300..f113480c 100644 --- a/app/src/explore/interaction-controller.ts +++ b/app/src/explore/interaction-controller.ts @@ -6,7 +6,6 @@ import type { StructureErrorEventDetail, StructureLoadEvent, } from '@protspace/core'; -import type { VisualizationData } from '@protspace/utils'; import { getProteinAnnotationIndices } from '@protspace/utils'; import { notify } from '../lib/notify'; import { getLegendErrorNotification } from './notifications'; @@ -16,14 +15,12 @@ interface InteractionControllerOptions { plotElement: ProtspaceScatterplot; selectedProteinElement: HTMLElement | null; structureViewer: ProtspaceStructureViewer; - clearPersistedLegendHiddenValues: (annotation: string, dataOverride?: VisualizationData) => void; } export interface InteractionController { updateLegend(): void; updateSelectedProteinDisplay(proteinId: string | null): void; getSelectedProteins(): string[]; - setLastKnownAnnotation(annotation: string): void; handleSelectionChange(event: Event): void; handlePlotDataChange(): void; handleLegendItemClick(event: Event): void; @@ -31,7 +28,7 @@ export interface InteractionController { handleStructureLoad(event: Event): void; handleStructureError(event: Event): void; handleStructureClose(event: Event): void; - handleAnnotationChange(nextAnnotation: string): void; + handleAnnotationChange(): void; } export function createInteractionController({ @@ -39,11 +36,9 @@ export function createInteractionController({ plotElement, selectedProteinElement, structureViewer, - clearPersistedLegendHiddenValues, }: InteractionControllerOptions): InteractionController { let hiddenValues: string[] = []; let selectedProteins: string[] = []; - let lastKnownAnnotation = ''; const updateSelectedProteinDisplay = (proteinId: string | null) => { if (!selectedProteinElement) { @@ -96,9 +91,6 @@ export function createInteractionController({ getSelectedProteins() { return selectedProteins; }, - setLastKnownAnnotation(annotation) { - lastKnownAnnotation = annotation; - }, handleSelectionChange(event) { const customEvent = event as CustomEvent<{ proteinIds?: string[] }>; selectedProteins = Array.isArray(customEvent.detail.proteinIds) @@ -152,15 +144,14 @@ export function createInteractionController({ console.log('Structure viewer should now be hidden'); updateSelectedProteinDisplay(null); }, - handleAnnotationChange(nextAnnotation) { - if (lastKnownAnnotation && lastKnownAnnotation !== nextAnnotation) { - clearPersistedLegendHiddenValues(lastKnownAnnotation); - } - + handleAnnotationChange() { + // Reset only the in-memory hidden set for the newly selected annotation. + // Per-annotation visibility is persisted by the core legend + // (saveSettings/loadSettings, keyed by datasetHash + annotation), so + // switching away and back restores the previously hidden categories. hiddenValues = []; plotElement.hiddenAnnotationValues = hiddenValues; updateLegend(); - lastKnownAnnotation = nextAnnotation; }, }; } diff --git a/app/src/explore/persisted-legend.ts b/app/src/explore/persisted-legend.ts deleted file mode 100644 index 2566f1dc..00000000 --- a/app/src/explore/persisted-legend.ts +++ /dev/null @@ -1,66 +0,0 @@ -import type { ProtspaceScatterplot } from '@protspace/core'; -import { - buildStorageKey, - generateDatasetHash, - getStorageItem, - setStorageItem, - type VisualizationData, -} from '@protspace/utils'; - -interface PersistedLegendOptions { - plotElement: ProtspaceScatterplot; -} - -interface PersistedLegendController { - clearPersistedLegendHiddenValues(annotation: string, dataOverride?: VisualizationData): void; -} - -export function createPersistedLegendController({ - plotElement, -}: PersistedLegendOptions): PersistedLegendController { - const getCurrentDatasetForPersistence = ( - dataOverride?: VisualizationData, - ): VisualizationData | null => { - if (dataOverride) { - return dataOverride; - } - - const plotWithUnfilteredGetter = plotElement as ProtspaceScatterplot & { - getCurrentData?: (options?: { - includeFilteredProteinIds?: boolean; - }) => VisualizationData | undefined; - }; - - return ( - plotWithUnfilteredGetter.getCurrentData?.({ includeFilteredProteinIds: false }) ?? - plotElement.getCurrentData?.() ?? - plotElement.data ?? - null - ); - }; - - return { - clearPersistedLegendHiddenValues(annotation, dataOverride) { - if (!annotation) { - return; - } - - const dataset = getCurrentDatasetForPersistence(dataOverride); - if (!dataset) { - return; - } - - const storageKey = buildStorageKey('legend', generateDatasetHash(dataset), annotation); - const persistedSettings = getStorageItem | null>(storageKey, null); - - if (!persistedSettings || !Array.isArray(persistedSettings.hiddenValues)) { - return; - } - - setStorageItem(storageKey, { - ...persistedSettings, - hiddenValues: [], - }); - }, - }; -} diff --git a/app/src/explore/runtime.ts b/app/src/explore/runtime.ts index a50c3e1d..42627c63 100644 --- a/app/src/explore/runtime.ts +++ b/app/src/explore/runtime.ts @@ -21,7 +21,6 @@ import { } from './fasta-prep-limits'; import { createLoadQueue } from './load-queue'; import { createLoadingOverlayController } from './loading-overlay'; -import { createPersistedLegendController } from './persisted-legend'; import { startInitialExploreLoad } from './startup'; import { NOOP_CONTROLLER, type ExploreController } from './types'; import { createViewController } from './view-controller'; @@ -217,16 +216,11 @@ export async function initializeExploreRuntime(): Promise { }); lifecycle.addCleanup(() => viewController.dispose()); - const persistedLegendController = createPersistedLegendController({ - plotElement, - }); - const interactionController = createInteractionController({ legendElement, plotElement, selectedProteinElement, structureViewer, - clearPersistedLegendHiddenValues: persistedLegendController.clearPersistedLegendHiddenValues, }); const datasetController = createDatasetController({ @@ -321,11 +315,9 @@ export async function initializeExploreRuntime(): Promise { addControlBarListener(type, listener, options) { addTrackedEventListener(lifecycle, controlBar, type, listener, options); }, - controlBar, datasetController, handleExport, interactionController, - plotElement, viewController, }); diff --git a/app/tests/url-view-state.spec.ts b/app/tests/url-view-state.spec.ts index c3ab6c29..2f144102 100644 --- a/app/tests/url-view-state.spec.ts +++ b/app/tests/url-view-state.spec.ts @@ -813,7 +813,7 @@ test.describe('URL-backed explore view state', () => { await expect(page).toHaveURL(/foo=1/); }); - test('resets hidden legend state when back navigation changes annotation via URL', async ({ + test('restores hidden legend state when back navigation returns to a previous annotation via URL', async ({ page, }) => { await page.goto( @@ -838,6 +838,37 @@ test.describe('URL-backed explore view state', () => { await page.goBack(); await waitForView(page, { annotation: targetAnnotation, projection: targetProjection }); - await expect.poll(() => isLegendItemHidden(page, firstLegendValue)).toBe(false); + // Per-annotation legend visibility is persisted (datasetHash + annotation), so + // returning to the original annotation restores the previously hidden category. + await expect.poll(() => isLegendItemHidden(page, firstLegendValue)).toBe(true); + }); + + test('keeps hidden legend categories when switching annotation away and back via the control bar', async ({ + page, + }) => { + await page.goto( + `/explore?annotation=${encodeURIComponent(targetAnnotation)}&projection=${encodeURIComponent(targetProjection)}`, + ); + await dismissTourIfPresent(page); + await waitForExploreDataLoad(page); + await waitForView(page, { annotation: targetAnnotation, projection: targetProjection }); + + const currentView = await getCurrentView(page); + const nextAnnotation = currentView.annotations.find( + (annotation) => annotation !== targetAnnotation, + ); + expect(nextAnnotation).toBeTruthy(); + + const firstLegendValue = await getFirstLegendItemValue(page); + await clickLegendItem(page, firstLegendValue); + await expect.poll(() => isLegendItemHidden(page, firstLegendValue)).toBe(true); + + // Switch to another annotation and back, both via the control bar (no URL navigation). + await selectAnnotation(page, nextAnnotation!); + await waitForView(page, { annotation: nextAnnotation! }); + await selectAnnotation(page, targetAnnotation); + await waitForView(page, { annotation: targetAnnotation }); + + await expect.poll(() => isLegendItemHidden(page, firstLegendValue)).toBe(true); }); }); From 5db65f2c6c1b4cffe16d6248447dc7bb2ee10200 Mon Sep 17 00:00:00 2001 From: tsenoner Date: Tue, 16 Jun 2026 20:29:04 +0200 Subject: [PATCH 2/7] fix(data-loader): split categorical values only on top-level ';' Categorical annotation cells encode multiple hits as "accession (name)|score" joined by ';', but a name can itself contain ';' (e.g. CATH-Gene3D "Ribosomal Protein L15; Chain: K; domain 2"). The naive split(';') shattered a single hit into bogus categories such as "domain 2)" and "Chain: K". Split only on ';' at parenthesis depth 0 so each (name) stays intact, falling back to a plain split for the rare name with an unbalanced '(' so distinct hits are not merged. This repairs already-distributed bundles; sanitizing the names at the source is tracked in tsenoner/protspace#56. Closes #282 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../data-loader/utils/conversion.test.ts | 59 ++++++++++++++++++- .../data-loader/utils/conversion.ts | 45 ++++++++++++-- 2 files changed, 99 insertions(+), 5 deletions(-) diff --git a/packages/core/src/components/data-loader/utils/conversion.test.ts b/packages/core/src/components/data-loader/utils/conversion.test.ts index 290b3b87..2a3c9ddd 100644 --- a/packages/core/src/components/data-loader/utils/conversion.test.ts +++ b/packages/core/src/components/data-loader/utils/conversion.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { parseAnnotationValue } from './conversion'; +import { parseAnnotationValue, splitCategoricalAnnotationValues } from './conversion'; describe('parseAnnotationValue', () => { it('parses label without pipe as full string with empty scores', () => { @@ -166,3 +166,60 @@ describe('parseAnnotationValue', () => { expect(result.evidence).toBeNull(); }); }); + +describe('splitCategoricalAnnotationValues', () => { + it('splits distinct hits on the top-level ; separator', () => { + expect(splitCategoricalAnnotationValues('PF00001 (7tm_1)|1.5e-10;PF00002 (Foo)|30.0')).toEqual([ + 'PF00001 (7tm_1)|1.5e-10', + 'PF00002 (Foo)|30.0', + ]); + }); + + it('keeps a CATH-Gene3D name containing ";" intact as a single category', () => { + // Real-world shape: the name itself contains semicolons inside the parentheses. + expect( + splitCategoricalAnnotationValues( + 'G3DSA:3.100 (Ribosomal Protein L15; Chain: K; domain 2)|45.2', + ), + ).toEqual(['G3DSA:3.100 (Ribosomal Protein L15; Chain: K; domain 2)|45.2']); + }); + + it('splits two CATH hits whose names both contain ";"', () => { + expect( + splitCategoricalAnnotationValues( + 'G3DSA:3.100 (Ribosomal Protein L15; Chain: K; domain 2)|45.2;' + + 'G3DSA:2.40 (Acid Proteases; Chain A)|30.0', + ), + ).toEqual([ + 'G3DSA:3.100 (Ribosomal Protein L15; Chain: K; domain 2)|45.2', + 'G3DSA:2.40 (Acid Proteases; Chain A)|30.0', + ]); + }); + + it('handles nested balanced parentheses in a name', () => { + expect( + splitCategoricalAnnotationValues('G3DSA:2.60 (3-Layer(aba) Sandwich; domain 1)|12.0'), + ).toEqual(['G3DSA:2.60 (3-Layer(aba) Sandwich; domain 1)|12.0']); + }); + + it('still splits plain multi-value cells without parentheses', () => { + expect(splitCategoricalAnnotationValues('Cytoplasm;Nucleus;Membrane')).toEqual([ + 'Cytoplasm', + 'Nucleus', + 'Membrane', + ]); + }); + + it('falls back to a plain split when a name has an unbalanced "(" so distinct hits are not merged', () => { + // The name "YojJ-like (1" never closes its paren; depth never returns to 0, so the + // paren-aware scan would swallow the inter-hit ";". The fallback keeps the two hits apart. + expect( + splitCategoricalAnnotationValues('G3DSA:1.10 (YojJ-like (1)|9.0;G3DSA:3.40 (Bar)|8.0'), + ).toEqual(['G3DSA:1.10 (YojJ-like (1)|9.0', 'G3DSA:3.40 (Bar)|8.0']); + }); + + it('returns an empty array for missing cells', () => { + expect(splitCategoricalAnnotationValues(null)).toEqual([]); + expect(splitCategoricalAnnotationValues('')).toEqual([]); + }); +}); diff --git a/packages/core/src/components/data-loader/utils/conversion.ts b/packages/core/src/components/data-loader/utils/conversion.ts index e3587502..8c658d20 100644 --- a/packages/core/src/components/data-loader/utils/conversion.ts +++ b/packages/core/src/components/data-loader/utils/conversion.ts @@ -219,14 +219,51 @@ export const parseAnnotationValue = ( return { label, scores, evidence: null }; }; -function splitCategoricalAnnotationValues(rawValue: unknown): string[] { +/** + * Split a categorical annotation cell on the top-level hit separator ';'. + * + * Multi-hit values are encoded as `accession (name)|score;accession2 (name2)|score`, + * but a name can legitimately contain ';' — e.g. CATH-Gene3D names such as + * "Ribosomal Protein L15; Chain: K; domain 2". A naive `split(';')` shatters one hit + * into bogus categories ("Chain: K", "domain 2)"). Splitting only on ';' at parenthesis + * depth 0 keeps each `(name)` intact while still separating distinct hits. + * + * If a name contains an unbalanced '(' (depth never returns to 0), fall back to a plain + * split so two distinct hits are not merged — at the cost of re-splitting that one rare + * value. Sanitizing names at the source is tracked in tsenoner/protspace#56. + */ +function splitOnTopLevelSemicolons(value: string): string[] { + const parts: string[] = []; + let current = ''; + let depth = 0; + + for (const ch of value) { + if (ch === '(') { + depth += 1; + } else if (ch === ')') { + if (depth > 0) depth -= 1; // clamp at 0 so a stray ')' can't go negative + } else if (ch === ';' && depth === 0) { + parts.push(current); + current = ''; + continue; + } + current += ch; + } + parts.push(current); + + if (depth !== 0) { + return value.split(';'); + } + return parts; +} + +export function splitCategoricalAnnotationValues(rawValue: unknown): string[] { // First-level: normalize the whole cell. Returns null if the entire cell is missing. const cellNormalized = normalizeMissingValue(rawValue); if (cellNormalized == null) return []; - // Split, trim, drop empty tokens, and drop tokens that normalize to missing. - return String(cellNormalized) - .split(';') + // Split on top-level ';' (paren-aware), trim, drop empty/missing tokens. + return splitOnTopLevelSemicolons(String(cellNormalized)) .map((part) => part.trim()) .filter((part) => part !== '' && normalizeMissingValue(part) !== null); } From b7c9fa8bb376115f153af488ce887253a81fdd26 Mon Sep 17 00:00:00 2001 From: tsenoner Date: Tue, 16 Jun 2026 20:29:25 +0200 Subject: [PATCH 3/7] fix(header): restyle Feedback button as an on-brand ghost The Feedback button rendered with the shadcn default variant (solid bg-primary #3c83f6), a different and more prominent blue than the canonical ProtSpace #00a3e0 used in-canvas. Switch both the desktop and mobile call sites to a no-border ghost: transparent, #00a3e0 text/icon, faint blue hover. De-emphasized and consistent on the light and dark headers. Closes #283 Co-Authored-By: Claude Opus 4.8 (1M context) --- app/src/components/Header.tsx | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/app/src/components/Header.tsx b/app/src/components/Header.tsx index fc02c5c7..19074d60 100644 --- a/app/src/components/Header.tsx +++ b/app/src/components/Header.tsx @@ -9,6 +9,11 @@ import { getNavigation } from '../../../config/navigation'; const FEEDBACK_HREF = buildMailto({ subject: 'ProtSpace feedback' }); +// Ghost-style Feedback CTA: transparent, canonical ProtSpace blue (#00a3e0) text/icon, +// faint blue hover. De-emphasized and on-brand vs. the app-shell primary (#3c83f6). +const FEEDBACK_BUTTON_CLASS = + 'text-[#00a3e0] hover:bg-[#00a3e0]/10 hover:text-[#00a3e0] focus-visible:ring-[#00a3e0]'; + const mode = import.meta.env.MODE === 'production' ? 'production' : 'development'; const navItems = getNavigation(mode); @@ -127,7 +132,7 @@ const Header = ({ variant = 'default', className }: HeaderProps) => { })} {/* Feedback CTA */} -