From 6151a3720135533bd4c589b12825cdc92dcf2e5a Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 9 Sep 2026 19:34:46 +0000 Subject: [PATCH] fix(routing): ask the path whether this is a content page, not the whole URL Both effects that write pathway browser state into the URL stood down with `router.url.includes('content') || router.url.includes('query')`. The query string answers that as readily as the path does. `sample` holds a column name taken verbatim from the reader's own expression file, and analysis.service.ts sets it automatically to the first column. So an expression file whose first column is called "GC content" puts `?sample=GC__content` in the URL -- and from that moment nothing the reader did was written to the URL again. Not selecting a node, not flagging, not changing tab. None of it survived a reload, and none of it was in any link they shared. Now one exported predicate, testing whole segments of the path only. "query" is gone rather than fixed: the search page is `content/query`, already covered by the first test, and of the two words it is the likelier to turn up in somebody's data. Verified by putting the substring test back: the new e2e case fails on it and passes on this, so it is a regression test rather than a description. The three content pages keep their own addresses either way, which is what the guard is actually for and had no coverage at all. Co-Authored-By: Claude Opus 5 --- e2e/content-page-urls.spec.ts | 65 +++++++++++++++++++ .../app/services/url-state.service.spec.ts | 40 +++++++++++- .../src/app/services/url-state.service.ts | 36 ++++++++-- 3 files changed, 134 insertions(+), 7 deletions(-) create mode 100644 e2e/content-page-urls.spec.ts diff --git a/e2e/content-page-urls.spec.ts b/e2e/content-page-urls.spec.ts new file mode 100644 index 0000000..a542054 --- /dev/null +++ b/e2e/content-page-urls.spec.ts @@ -0,0 +1,65 @@ +/** + * The content pages own their own addresses. + * + * They render some of the same panels as the pathway browser, from the same + * UrlStateService, and that service has two effects that write pathway browser + * state into the URL. On a content page those have to stand down, or opening a + * detail page rewrites its address into a pathway browser one. + * + * The guard that does this used to be a substring test over the whole URL, so a + * query parameter carrying the word "content" or "query" decided it too -- and + * `sample` is a column name taken verbatim from the reader's own expression file. + * Both halves are covered here: the content pages still stand down, and the + * pathway browser still writes its URL when a parameter happens to say "content". + */ +import { expect, test } from '@playwright/test'; + +test.describe.configure({ timeout: 4 * 60 * 1000 }); + +const contentPages = [ + '/content/detail/R-HSA-1430728', + '/content/query?q=kinase', + '/content/schema/Pathway', +]; + +for (const path of contentPages) { + test(`${path} keeps its own address`, async ({ page }) => { + await page.goto(path, { waitUntil: 'domcontentloaded' }); + await page.waitForTimeout(4000); + + const landed = new URL(page.url()); + expect(landed.pathname, 'the page rewrote its own address').toContain(path.split('?')[0]); + }); +} + +test('the pathway browser still writes its URL when a parameter says "content"', async ({ + page, +}) => { + // `?sample=GC__content` is what an expression file with a column called + // "GC content" produces. Under the old guard this URL silenced every write + // that followed it: no selection, no flag, no tab, nothing to share. + await page.goto('/PathwayBrowser/R-HSA-109606?sample=GC__content', { + waitUntil: 'domcontentloaded', + }); + // The tabs are not there until the diagram is, and the default tab settles a + // moment after that. + await page.waitForFunction( + () => { + const container = document.querySelector('#cytoscape') as + (HTMLElement & { _cyreg?: { cy?: { elements(): { length: number } } } }) | null; + return (container?._cyreg?.cy?.elements().length ?? 0) > 0; + }, + { timeout: 90_000 } + ); + await page.waitForTimeout(2500); + + const molecule = page + .locator('[role="tab"]') + .filter({ hasText: /Molecule/i }) + .first(); + test.skip((await molecule.count()) === 0, 'this pathway offers no Molecule tab'); + await molecule.click(); + await page.waitForTimeout(2500); + + expect(new URL(page.url()).search, 'choosing a tab reached the URL').toContain('tab=molecule'); +}); diff --git a/projects/pathway-browser/src/app/services/url-state.service.spec.ts b/projects/pathway-browser/src/app/services/url-state.service.spec.ts index 3678c9f..0946cbc 100644 --- a/projects/pathway-browser/src/app/services/url-state.service.spec.ts +++ b/projects/pathway-browser/src/app/services/url-state.service.spec.ts @@ -9,7 +9,7 @@ * The cases below are taken from what is actually in the content, not invented. */ import { describe, expect, it } from 'vitest'; -import { FRAGMENT_PATTERN } from './url-state.service'; +import { FRAGMENT_PATTERN, isContentRoute } from './url-state.service'; /** What the subscriber does with a fragment, reduced to its decisions. */ function route(fragment: string) { @@ -88,3 +88,41 @@ describe('a legacy pathway link in the fragment', () => { } }); }); + +/** + * Standing down on the content pages, without standing down anywhere else. + * + * The guard used to be a substring test over the whole URL, so a query parameter + * carrying either word decided it. `sample` is a column name out of the reader's + * own expression file, set automatically to the first column, and "GC content" is + * an ordinary thing for a column to be called. + */ +describe('recognising a content page', () => { + it('stands down on the content pages', () => { + expect(isContentRoute('/content/detail/R-HSA-1430728')).toBe(true); + expect(isContentRoute('/content/schema/Pathway')).toBe(true); + }); + + it('still stands down on the search page, which is a content page', () => { + expect(isContentRoute('/content/query')).toBe(true); + expect(isContentRoute('/content/query?q=kinase')).toBe(true); + }); + + it('does not stand down in the pathway browser', () => { + expect(isContentRoute('/PathwayBrowser/R-HSA-1430728')).toBe(false); + expect(isContentRoute('/PathwayBrowser/R-HSA-1430728?tab=details&sel=R-HSA-9')).toBe(false); + }); + + it('does not stand down because the reader named a column "GC content"', () => { + expect(isContentRoute('/PathwayBrowser/R-HSA-1430728?sample=GC__content')).toBe(false); + }); + + it('does not stand down for a value that merely contains the word', () => { + expect(isContentRoute('/PathwayBrowser/R-HSA-1?overlay=contents')).toBe(false); + expect(isContentRoute('/PathwayBrowser/R-HSA-1#content')).toBe(false); + }); + + it('does not stand down for the word "query" anywhere but a content path', () => { + expect(isContentRoute('/PathwayBrowser/R-HSA-1?sample=query__1')).toBe(false); + }); +}); diff --git a/projects/pathway-browser/src/app/services/url-state.service.ts b/projects/pathway-browser/src/app/services/url-state.service.ts index 27e1b82..b98d46f 100644 --- a/projects/pathway-browser/src/app/services/url-state.service.ts +++ b/projects/pathway-browser/src/app/services/url-state.service.ts @@ -36,6 +36,34 @@ import { toSignal } from '@angular/core/rxjs-interop'; */ export const FRAGMENT_PATTERN = /^\/?(?R-[A-Z]{3}-\d+|\d{4,})(?:\.\d+)?(?:&(?.*))?$/; +/** + * Whether this URL is one of the content pages rather than the pathway browser. + * + * The content pages render some of the same panels from this same service, but + * they own their own addresses -- so the two effects that write pathway browser + * state into the URL have to stand down there, or opening a detail page rewrites + * its address into a pathway browser one. + * + * The path, and whole segments of it. This used to ask whether the whole URL + * *contained* "content" or "query", and the query string answers that just as + * readily as the path does: `sample` holds a column name out of the reader's own + * expression file -- set automatically to the first column of it -- so a file + * with a column called "GC content" puts `?sample=GC__content` in the URL, and + * from that moment nothing the reader did was written to the URL again. Not + * selecting a node, not flagging, not changing tab. None of it survived a + * reload, and none of it was in a link they shared. + * + * "query" is gone rather than fixed: the search page is `content/query`, which + * the first test already covers, and of the two words it is the likelier to turn + * up in somebody's data. + */ +export function isContentRoute(url: string): boolean { + return url + .split(/[?#]/, 1)[0] + .split('/') + .some((segment) => segment === 'content'); +} + export type UrlParam = WritableSignal & { otherTokens?: string[]; initialValue: T; @@ -161,11 +189,7 @@ export class UrlStateService implements State { effect(() => { // console.log('Updating patwhayId to ', this.pathwayId()) - //If in content/search do not navigate - if (this.router.url.includes('content') || this.router.url.includes('query')) { - // console.log('In content or search route, not navigating on pathwayId change'); - return; - } + if (isContentRoute(this.router.url)) return; void this.navigateTo(this.pathwayId() ?? null, { queryParamsHandling: 'preserve', @@ -274,7 +298,7 @@ export class UrlStateService implements State { }); effect(() => { const queryParams = this.currentQueryParams(); - if (this.router.url.includes('content') || this.router.url.includes('query')) return; + if (isContentRoute(this.router.url)) return; // Settling is not a step the reader took, so it replaces rather than adds: // opening a pathway wrote ?tab=info and then ?tab=details, two entries for a