From b0585808743abe141feb2fe6e6bb521c5d7fe2d3 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Fri, 4 Sep 2026 21:43:54 +0000 Subject: [PATCH] fix(diagram): keep trivial molecules visible while something is flagged Curators: "H2O and H+ disappear with zooming out." Separately, #150: a molecule's chemical structure drawn with no molecule underneath it. Same fault from two sides -- the node was hidden while a different handler carried on drawing its structure. Flagging pins trivial molecules visible by marking them `.always-visible`, a class rather than an inline style precisely so nothing can lose it. But the zoom handler writes an *inline* opacity across every `.trivial` element, and in cytoscape an inline style beats any stylesheet rule, so it painted straight over the class. Detaching it from the zoom event -- what the previous fix did -- was not enough: `triggerZoom()` calls it directly, on every restyle and whenever an interactor opens. The class is now the authority: the handler leaves anything wearing it alone. Measured on beta, with a flag active, opacity is 1 at every zoom and across a restyle; with no flag, molecules still fade out and back as before. Also, the reason this took two attempts to land: `build:libs` did not build reactome-cytoscape-style. CI compensated with a step of its own, so a fix to that library passed CI and never reached the dev server, which builds the app without building the library. `build:libs` now covers every workspace library and `build` depends on it, so any path that produces the app produces what it links against -- and CI's two special-case steps are gone, because it now runs the same command a developer does. The regression test is end-to-end rather than a unit test: the library cannot be constructed headlessly (its layers plugin refuses), and the bug lives in what a real renderer computes. It reads opacity through the registration cytoscape keeps on its own container, which beats sampling canvas pixels -- those cannot say which element was faded. Verified to fail without the fix: both flagging tests go red, the unflagged one stays green. Fixes #150 Fixes #143 Co-Authored-By: Claude Opus 5 --- .github/workflows/tests.yml | 19 ++- e2e/flagging-trivial.spec.ts | 134 ++++++++++++++++++ package.json | 6 +- .../src/lib/interactivity.ts | 23 ++- 4 files changed, 164 insertions(+), 18 deletions(-) create mode 100644 e2e/flagging-trivial.spec.ts diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 98d47d2f..0f1d1190 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -48,16 +48,17 @@ jobs: # and passed every test. Without this step the only build was the deploy # job, so a broken template reached main and was found by a person. # - # reactome-cytoscape-style is built separately because build:libs does not - # cover it and styles.scss @use's it; content-dist is a gitignored asset - # input, so stage:content has to run or ng build cannot find it. + # content-dist is a gitignored asset input, so stage:content has to run or + # ng build cannot find it. The libraries no longer need a step of their + # own: `npm run build` builds them, because a build that does not produce + # what it links against is a build that can pass here and ship something + # else -- a fix to reactome-cytoscape-style reached CI and never reached + # the dev server, which builds the app without building the library. - name: Compile CMS content run: npm run stage:content - name: Build the app - run: | - npx ng build reactome-cytoscape-style - npm run build + run: npm run build # Lint errors must be zero -- eslint.config.js only marks a rule "error" # once the codebase is clean of it, so an error is always a new violation. @@ -110,11 +111,7 @@ jobs: # pathway-browser build copies reactome-cytoscape-style's assets, so the # app cannot be served until they exist. - name: Build workspace libraries - run: | - npx ng build reactome-cytoscape-style - npx ng build ngx-reactome-style - npx ng build reactome-table - npx ng build reactome-gsa-form + run: npm run build:libs # site-search-index.json is generated, not committed, and the search # specs depend on it. diff --git a/e2e/flagging-trivial.spec.ts b/e2e/flagging-trivial.spec.ts new file mode 100644 index 00000000..50f9b1a1 --- /dev/null +++ b/e2e/flagging-trivial.spec.ts @@ -0,0 +1,134 @@ +import { test, expect, type Page } from '@playwright/test'; + +/** + * Trivial molecules stay visible while something is flagged. + * + * Water and protons are drawn faintly and fade out as you zoom away, because a + * diagram carrying hundreds of them is unreadable. Flagging turns that off: a + * search has to be able to point at a molecule, and a molecule you cannot see + * is not an answer. + * + * Curators reported the opposite — "H2O and H+ disappear with zooming out" — + * and, separately, chemical structures drawn with no molecule underneath them, + * which is the same fault seen from the other side: the node was hidden while a + * different handler carried on drawing its structure. + * + * The cause was that the zoom handler writes an *inline* opacity, which in + * cytoscape beats any stylesheet rule, so it silently overrode the + * `.trivial.always-visible` rule flagging relies on. Detaching it from the zoom + * event was not enough: `triggerZoom()` calls it directly on every restyle and + * whenever an interactor opens. + */ + +const BOOT_TIMEOUT = 60_000; +// Glycolysis: 41 trivial elements, H+ and H2O among them. +const PATHWAY = 'R-HSA-70171'; +const FLAGGED = 'PKM'; + +/** The few cytoscape members these checks touch, so none of this needs `any`. */ +interface DiagramGraph { + zoom(level?: number): number; + emit(event: string): void; + elements(selector: string): { + length: number; + map(fn: (element: { numericStyle(property: string): number }) => T): T[]; + }; + data(key: string): { update(graph: DiagramGraph): void }; +} + +/** + * Cytoscape registers itself on its own container so a second `cytoscape()` + * call cannot clobber it, and that registration is the only handle a test has. + * The alternative is reading opacities out of canvas pixels, which cannot say + * which element was faded. + */ +type CytoscapeHost = Element & { _cyreg?: { cy?: DiagramGraph } }; + +async function trivialOpacities(page: Page): Promise { + return page.evaluate(() => { + const host = document.querySelector('#cytoscape') as CytoscapeHost | null; + const cy = host?._cyreg?.cy; + if (!cy) throw new Error('no cytoscape instance on #cytoscape'); + return [ + ...new Set( + cy.elements('.trivial').map((element) => Number(element.numericStyle('opacity').toFixed(2))) + ), + ]; + }); +} + +async function zoomTo(page: Page, zoom: number) { + await page.evaluate((level) => { + const cy = (document.querySelector('#cytoscape') as CytoscapeHost | null)?._cyreg?.cy; + if (!cy) throw new Error('no cytoscape instance on #cytoscape'); + cy.zoom(level); + cy.emit('zoom'); + }, zoom); + await page.waitForTimeout(400); +} + +/** The flag arrives from its own request, after the diagram has drawn. */ +async function waitForFlag(page: Page) { + await page.waitForFunction( + () => { + const cy = (document.querySelector('#cytoscape') as CytoscapeHost | null)?._cyreg?.cy; + return cy ? cy.elements('.flag').length > 0 : false; + }, + { timeout: BOOT_TIMEOUT } + ); +} + +async function openDiagram(page: Page, query = '') { + await page.goto(`/PathwayBrowser/${PATHWAY}${query}`); + await page.waitForSelector('#cytoscape canvas', { timeout: BOOT_TIMEOUT }); + // The diagram keeps drawing after its first canvas, so wait for the elements + // themselves rather than for a delay. + await page.waitForFunction( + () => { + const cy = (document.querySelector('#cytoscape') as CytoscapeHost | null)?._cyreg?.cy; + return cy ? cy.elements('.trivial').length > 0 : false; + }, + { timeout: BOOT_TIMEOUT } + ); +} + +test.describe('Trivial molecules and flagging', () => { + test.describe.configure({ timeout: 3 * 60 * 1000 }); + + test('fade with zoom when nothing is flagged', async ({ page }) => { + await openDiagram(page); + + await zoomTo(page, 1.5); + expect(await trivialOpacities(page), 'visible close up').toEqual([1]); + + await zoomTo(page, 0.14); + expect(Math.max(...(await trivialOpacities(page))), 'faint far out').toBeLessThan(1); + }); + + test('stay visible at every zoom once something is flagged', async ({ page }) => { + await openDiagram(page, `?FLG=${FLAGGED}`); + await waitForFlag(page); + + expect(await trivialOpacities(page), 'as the diagram loads').toEqual([1]); + + for (const zoom of [0.05, 1.4, 0.12]) { + await zoomTo(page, zoom); + expect(await trivialOpacities(page), `at zoom ${zoom}`).toEqual([1]); + } + }); + + test('survive a restyle, which runs the zoom handler directly', async ({ page }) => { + await openDiagram(page, `?FLG=${FLAGGED}`); + await waitForFlag(page); + + // What a theme change or a loading analysis does. + await page.evaluate(() => { + const cy = (document.querySelector('#cytoscape') as CytoscapeHost | null)?._cyreg?.cy; + if (!cy) throw new Error('no cytoscape instance on #cytoscape'); + cy.data('reactome').update(cy); + }); + await page.waitForTimeout(500); + + expect(await trivialOpacities(page), 'a restyle must not fade a flagged molecule').toEqual([1]); + }); +}); diff --git a/package.json b/package.json index a611acfa..83c4719f 100644 --- a/package.json +++ b/package.json @@ -11,7 +11,7 @@ "dev:serve": "wait-on dist/reactome-cytoscape-style/assets/index.scss && tinacms dev --rootPath projects/website-angular -c \"ng serve\"", "dev:serve:local": "wait-on dist/reactome-cytoscape-style/assets/index.scss && tinacms dev --rootPath projects/website-angular -c \"ng serve --configuration development\"", "build:reactome-cytoscape-style": "ng build reactome-cytoscape-style", - "build:libs": "ng build ngx-reactome-style && ng build reactome-table && ng build reactome-gsa-form", + "build:libs": "ng build reactome-cytoscape-style && ng build ngx-reactome-style && ng build reactome-table && ng build reactome-gsa-form", "start": "npm run generate:indices && npm run stage:content && npm run build:libs && run-p dev:reactome-cytoscape-style dev:serve", "start:local": "npm run generate:indices && run-p dev:reactome-cytoscape-style dev:serve:local", "start:simple": "ng serve", @@ -20,8 +20,8 @@ "start:simple:local": "ng serve --configuration development", "start:website": "cd projects/website-angular && npm run start", "start:pathway": "cd projects/pathway-browser && npm run start-with-deps", - "build": "npm run generate:indices && npm run stage:content && ng build --configuration production", - "build:curator": "npm run generate:indices && npm run stage:content && npm run build:reactome-cytoscape-style && npm run build:libs && ng build --configuration production,curator", + "build": "npm run build:libs && npm run generate:indices && npm run stage:content && ng build --configuration production", + "build:curator": "npm run build:libs && npm run generate:indices && npm run stage:content && ng build --configuration production,curator", "build:website": "npm run build:curator && cd dist/reactome/browser/ && tar czvf browser.tar * && scp browser.tgz curator:~/browser.tgz && rm browser.tar", "build:pathway": "cd projects/pathway-browser && npm run build", "watch": "ng build --watch --configuration development", diff --git a/projects/reactome-cytoscape-style/src/lib/interactivity.ts b/projects/reactome-cytoscape-style/src/lib/interactivity.ts index 273b3152..53305490 100644 --- a/projects/reactome-cytoscape-style/src/lib/interactivity.ts +++ b/projects/reactome-cytoscape-style/src/lib/interactivity.ts @@ -548,10 +548,25 @@ export class Interactivity { shadowLabels.style({ 'text-opacity': shadowLabelOpacity, }); - trivial.style({ - opacity: trivialOpacity, - 'underlay-opacity': Math.min(shadowOpacity, trivialOpacity), - }); + // Not the ones flagging has pinned visible. + // + // This writes an inline opacity, and in cytoscape an inline style beats + // any stylesheet rule -- so it silently overrode the `.trivial + // .always-visible` rule that flagging relies on. Detaching this handler + // from the zoom event was not enough to stop it: `triggerZoom()` calls it + // directly, and that runs on every restyle (a theme change, an analysis + // loading) and whenever an interactor is opened. Curators saw H2O and H+ + // vanish as they zoomed out with something flagged, and saw chemical + // structures drawn with no molecule underneath -- the same fault, since + // the structures are drawn by a different handler that was still running. + // + // The class is the authority. Anything wearing it is left alone. + trivial + .filter((element) => !element.hasClass('always-visible')) + .style({ + opacity: trivialOpacity, + 'underlay-opacity': Math.min(shadowOpacity, trivialOpacity), + }); }; const updateDecorationPosition = (node: cytoscape.NodeSingular) => { if (!this.structureContainers.has(node)) return;