From 3beba983ea0647305b6ccd5e999f73a0cfe3a9c3 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Tue, 13 Oct 2026 04:50:14 +0000 Subject: [PATCH 1/4] Keep the router out of the embeddable diagram (#356) The embed never routes, but carried @angular/router (67 KB): everything that draws a pathway injects UrlStateService, and that class -- the token itself -- imported the Router to keep the state in the address bar. MemoryState, the embed's in-memory state, also imported from that file. - UrlStateService is now an abstract, router-free base: the state signals (built by createStateValues, as before), the tab maps and the dbId lookup, with section, settle, navigateTo and two new members abstract. It imports only router *types*. - RouterUrlStateService (router-url-state.service.ts) is the address-bar implementation, moved unchanged apart from the base taking the shared parts; the site provides it in src/app/app.config.ts. - MemoryState extends the base, so its copy of every field is gone. - The diagram's "first page opened" check and the species switch's query parameters were the last direct router reads; they are now state.isFirstNavigation() and state.queryParams() (the embed answers true and {}, which is what it got before with no router injected). - lodash's isArray/isNumber in the state service are now Array.isArray and typeof. The embed's initial JavaScript goes from 1.58 MB to 1.50 MB (all its JavaScript: 1,650 KB -> 1,570 KB). The full e2e suite passes (401, 6 skipped). Co-Authored-By: Claude Opus 5.5 --- .../src/app/diagram/diagram.component.ts | 4 +- .../src/app/services/analysis.service.ts | 2 +- .../src/app/services/entity.service.spec.ts | 9 +- .../app/services/router-url-state.service.ts | 282 ++++++++++++++ .../src/app/services/species.service.ts | 6 +- .../src/app/services/url-state.service.ts | 363 ++++-------------- .../src/memory-state.ts | 72 +--- src/app/app.config.ts | 6 + 8 files changed, 382 insertions(+), 362 deletions(-) create mode 100644 projects/pathway-browser/src/app/services/router-url-state.service.ts diff --git a/projects/pathway-browser/src/app/diagram/diagram.component.ts b/projects/pathway-browser/src/app/diagram/diagram.component.ts index 0098d2d6..67da0677 100644 --- a/projects/pathway-browser/src/app/diagram/diagram.component.ts +++ b/projects/pathway-browser/src/app/diagram/diagram.component.ts @@ -49,7 +49,6 @@ import { AnalysisService } from '../services/analysis.service'; import { Graph } from '../model/graph.model'; import { average, isDefined, isPathwayWithDiagram, isReferenceEntityStId } from '../services/utils'; import type { Analysis } from '../model/analysis.model'; -import { Router } from '@angular/router'; import { InteractorsComponent } from '../interactors/interactors.component'; import { EventService } from '../services/event.service'; import { Event as EventModel } from '../model/graph/event/event.model'; @@ -115,7 +114,6 @@ export class DiagramComponent implements AfterViewInit, OnDestroy { private event = inject(EventService); // Optional: the embeddable diagram (projects/reactome-diagram-element) has no // router -- it must never read or write its host page's address. - private router = inject(Router, { optional: true }); private download = inject(DownloadService); private data = inject(DataStateService); private deltaSignal = inject(DeltaSignalService); @@ -186,7 +184,7 @@ export class DiagramComponent implements AfterViewInit, OnDestroy { private readonly hierarchyHover = inject(HierarchyHoverService); constructor() { - this.isInitialLoad = Boolean(!this.router?.currentNavigation()?.previousNavigation); + this.isInitialLoad = this.state.isFirstNavigation(); effect(() => this.pathwayId() && this.loadDiagram()); // A reaction or sub-pathway pointed at in the hierarchy stands out here, // as the old browser drew it in yellow; nothing reached the diagram before. diff --git a/projects/pathway-browser/src/app/services/analysis.service.ts b/projects/pathway-browser/src/app/services/analysis.service.ts index 322c5ed6..4f450246 100644 --- a/projects/pathway-browser/src/app/services/analysis.service.ts +++ b/projects/pathway-browser/src/app/services/analysis.service.ts @@ -18,7 +18,7 @@ import chroma, { Color, Scale } from 'chroma-js'; import { extract, Style } from 'ngx-reactome-cytoscape-style'; import { rxResource, toObservable } from '@angular/core/rxjs-interop'; import { DarkService } from './dark.service'; -import { Params } from '@angular/router'; +import type { Params } from '@angular/router'; import { httpParams } from '../utils/http-params'; import { isDefined, shouldBeScientificFormat } from './utils'; import { Report } from 'reactome-gsa-form'; diff --git a/projects/pathway-browser/src/app/services/entity.service.spec.ts b/projects/pathway-browser/src/app/services/entity.service.spec.ts index 4fa244cc..bce71599 100644 --- a/projects/pathway-browser/src/app/services/entity.service.spec.ts +++ b/projects/pathway-browser/src/app/services/entity.service.spec.ts @@ -3,6 +3,8 @@ import { TestBed } from '@angular/core/testing'; import { provideHttpClient } from '@angular/common/http'; import { HttpTestingController, provideHttpClientTesting } from '@angular/common/http/testing'; import { provideRouter } from '@angular/router'; +import { UrlStateService } from './url-state.service'; +import { RouterUrlStateService } from './router-url-state.service'; import { firstValueFrom } from 'rxjs'; import { EntityService } from './entity.service'; @@ -16,7 +18,12 @@ describe('EntityService.getEntityInDepth', () => { beforeEach(() => { TestBed.configureTestingModule({ - providers: [provideHttpClient(), provideHttpClientTesting(), provideRouter([])], + providers: [ + provideHttpClient(), + provideHttpClientTesting(), + provideRouter([]), + { provide: UrlStateService, useClass: RouterUrlStateService }, + ], }); service = TestBed.inject(EntityService); http = TestBed.inject(HttpTestingController); diff --git a/projects/pathway-browser/src/app/services/router-url-state.service.ts b/projects/pathway-browser/src/app/services/router-url-state.service.ts new file mode 100644 index 00000000..58c08280 --- /dev/null +++ b/projects/pathway-browser/src/app/services/router-url-state.service.ts @@ -0,0 +1,282 @@ +import { effect, inject, Injectable, untracked } from '@angular/core'; +import { ActivatedRoute, NavigationEnd, NavigationExtras, Params, Router } from '@angular/router'; +import { filter, map, switchMap } from 'rxjs'; +import { UntilDestroy, untilDestroyed } from '@ngneat/until-destroy'; +import { toSignal } from '@angular/core/rxjs-interop'; +import { + FRAGMENT_PATTERN, + fragmentParams, + isContentRoute, + StateValues, + UrlParam, + UrlStateService, +} from './url-state.service'; + +@UntilDestroy() +/** + * The Pathway Browser's state, kept in the address bar. + * + * What the site provides for UrlStateService (src/app/app.config.ts). It is a + * class of its own, rather than the token everything injects, so that the + * embeddable diagram -- which keeps the same state in memory and never routes -- + * does not carry the router in its bundle (#356). + */ +@Injectable() +export class RouterUrlStateService extends UrlStateService { + private route: ActivatedRoute = inject(ActivatedRoute); + private router: Router = inject(Router); + + override readonly section = toSignal(this.route.fragment); + + constructor() { + super(); + this.router.events + .pipe( + filter((event) => event instanceof NavigationEnd), + switchMap(() => { + let route = this.router.routerState.root; + while (route.firstChild) route = route.firstChild; + return route.params; + }), + map((params) => params['pathwayId']) + ) + .subscribe((id) => { + this.pathwayId.set(id); + }); + + effect(() => { + // console.log('Updating patwhayId to ', this.pathwayId()) + + if (isContentRoute(this.router.url)) return; + + void this.navigateTo(this.pathwayId() ?? null, { + queryParamsHandling: 'preserve', + preserveFragment: !this.carriesLegacyPathway(), + }); + }); + + this.route.fragment.pipe(untilDestroyed(this)).subscribe((fragment) => { + if (fragment) { + // Convert fragments to params + const params: Params = {}; + let id = undefined; // Default routing + + const match = fragment.match(FRAGMENT_PATTERN); + if (match?.groups) { + if (match.groups['id']) { + id = match.groups['id']; + } + if (match.groups['params']) { + Object.assign(params, fragmentParams(match.groups['params'])); + } + + // `DIAGRAM` is the old browser's name for the pathway to open, so it is + // the route rather than a setting: `#DIAGRAM=9006934&PATH=162582` in the + // v64 announcement means the same as `#9006934&PATH=162582`. It is only + // ever a dbId, which the resolution below then swaps for the stable id. + if (!id && typeof params['DIAGRAM'] === 'string') { + id = params['DIAGRAM']; + delete params['DIAGRAM']; + } + + // `TOOL=AT` opened the analysis tool. Rewritten into the parameter that + // does that now, rather than carried through as a legacy token, because + // a legacy token does not survive the trip: the writer replaces the + // whole query string as soon as any state settles, so `?tab=info` + // arriving first dropped `?TOOL=AT` before the reader saw it. Setting + // the signal instead does not work either -- the reader resets every + // param the URL does not mention, so it would be undone in the same + // turn. The URL is the source of truth, so the URL is where it goes. + if (params['TOOL'] === 'AT') { + params['analysisTab'] = 'qualitative'; + delete params['TOOL']; + } + } + + const go = (resolved: string | undefined) => + void this.navigateTo(resolved ?? null, { + queryParamsHandling: 'merge', + fragment: fragment.replace(FRAGMENT_PATTERN, ''), + preserveFragment: false, + queryParams: params, + // Replace, do not add. Rewriting a legacy fragment into a proper + // route is a correction, not a step the reader took: pushing it + // left the old URL one Back away, and going back to it rewrote it + // again -- so someone who followed a link out of the news archive + // could not get back to the news archive. + replaceUrl: true, + }); + + // A legacy link may name a pathway by dbId. Navigate on it straight + // away -- the browser resolves one, and going first means the fragment + // is consumed in the same turn as an `#R-HSA-…` one, before anything + // else writes a history entry that still carries it. + // + // Then swap the URL for the stable id, replacing rather than pushing: a + // dbId is not stable across releases, so it is not a URL to leave a + // reader holding, but correcting it is not a step they took. + go(id); + if (id && /^\d+$/.test(id)) { + void this.dbIdToStId(Number(id)).then((stId) => { + if (stId && stId !== id) { + void this.navigateTo(stId, { + queryParamsHandling: 'preserve', + preserveFragment: false, + replaceUrl: true, + }); + } + }); + } + } + }); + + this.route.queryParams.pipe(untilDestroyed(this)).subscribe((params) => { + // Kept synchronous: subscribe ignores what the callback returns, so a + // rejection in here would go unreported. + void (async () => { + for (const mainToken in this.values) { + const param = this.values[mainToken as keyof StateValues] as UrlParam; + const tokens: string[] = [mainToken, ...(param.otherTokens || [])]; + const token = tokens.find((token) => params[token] !== undefined); + if (token) { + const initialValue = param.initialValue; + let value = params[token]; + if (value === undefined || value === null) param.set(value); + if (param.otherTransform && token !== mainToken) value = param.otherTransform(value); + else { + if (Array.isArray(initialValue)) { + let values: any[] = value.split(';'); + if (param.type === 'id') { + const mixedValues = values.map((v) => + v.charAt(0).match(/\d/) ? parseInt(v) : v + ); + values = await Promise.all(mixedValues.map((v) => this.ensureStId(v))); + } + if (param.type === 'number') values = values.map((v) => +v); + if (param.type === 'boolean') values = values.map((v) => v === 'true'); + param.set(values); + } else if (param.type === 'boolean') { + param.set(value === 'true'); + } else if (param.type === 'id') { + param.set(await this.ensureStId(value)); + } else if (param.type === 'number') { + param.set(parseFloat(value)); + } else { + param.set(value.replaceAll('__', ' ')); + } + } + } else { + param.set(param.initialValue); + } + } + })().catch((error) => console.error('Could not apply URL parameters', error)); + }); + effect(() => { + const queryParams = this.currentQueryParams(); + 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 + // choice nobody made, so Back changed the tab twice before it left. + // + // Decided from the values rather than from a flag. A flag raised by + // settle() and read here outlives its turn whenever this does not run -- + // the early return above, or a default that was already the current value, + // so no signal changed at all -- and the reader's next write, a real one, + // would then quietly replace their history instead of adding to it. + const settled = this.settledParams; + this.settledParams = null; + + void this.navigateTo(this.pathwayId() ?? null, { + queryParams, + preserveFragment: !this.carriesLegacyPathway(), + replaceUrl: settled !== null && settled === JSON.stringify(queryParams), + }); + }); + } + + /** + * Navigate to a pathway within the PathwayBrowser route context. + * Resolves the correct base path whether running standalone or inside the umbrella app. + * + * Router.navigate rejects when navigation fails -- a guard throwing, a + * resolver erroring -- and almost every caller here ignores the result, so + * those failures went nowhere. A rejection handler is attached here so they + * are always reported; the promise is still returned for the one caller that + * legitimately chains on navigation having finished. + */ + /** + * Whether the URL still carries a legacy pathway reference in its fragment. + * + * Such a fragment is on its way out: it is being rewritten into a proper + * route. Until then the other navigations here -- writing state into the URL, + * following a pathway change -- must not carry it along, because every entry + * they carry it into is one that rewrites itself forward when the reader goes + * back to it. Someone who followed a link out of the news archive could not + * get back to the news archive: three Backs, still on the pathway. + */ + private carriesLegacyPathway(): boolean { + return FRAGMENT_PATTERN.test(this.route.snapshot.fragment ?? ''); + } + + /** The params settle() last wrote, so the writer can recognise its own work. */ + private settledParams: string | null = null; + + /** + * Make a state change the reader did not ask for. + * + * The URL still has to say what is on screen -- a link has to be shareable -- + * but writing a default nobody chose should not cost them a press of Back. + * Choosing a tab pushes; being given one replaces. + */ + override settle(change: () => void): void { + change(); + // What the URL should now say. The writer replaces only if it is about to + // write exactly this, which is what keeps the decision out of a flag. + // + // Untracked, and that is the whole of it: settle() is called from inside an + // effect, so reading every param here made that effect depend on every + // param. Choosing a tab then re-ran the defaulting effect, which set the + // tab back to its default -- the reader's click did nothing at all. + this.settledParams = JSON.stringify(untracked(() => this.currentQueryParams())); + } + + /** The query params the current state should put in the URL. */ + private currentQueryParams(): Record { + const queryParams: Record = {}; + for (const key in this.values) { + const param = this.values[key as keyof StateValues]; + let paramValue: unknown = param(); + if ( + paramValue === undefined || + paramValue === null || + (Array.isArray(paramValue) && paramValue.length === 0) || + paramValue === param.initialValue + ) + continue; + if (typeof paramValue === 'string') paramValue = paramValue.replaceAll(' ', '__'); + queryParams[key] = Array.isArray(paramValue) ? paramValue.join(';') : paramValue; + } + return queryParams; + } + + override navigateTo(pathwayId: string | null, extras: NavigationExtras = {}): Promise { + let route = this.router.routerState.root; + while (route.firstChild) route = route.firstChild; + const segments = pathwayId ? [pathwayId] : []; + const navigation = this.router.navigate(segments, { + relativeTo: route.parent, + ...extras, + }); + navigation.catch((error) => console.error('Navigation failed', segments, error)); + return navigation; + } + + override isFirstNavigation(): boolean { + return !this.router.currentNavigation()?.previousNavigation; + } + + override queryParams(): Params { + return this.route.snapshot.queryParams; + } +} diff --git a/projects/pathway-browser/src/app/services/species.service.ts b/projects/pathway-browser/src/app/services/species.service.ts index 2b35563e..5472af10 100644 --- a/projects/pathway-browser/src/app/services/species.service.ts +++ b/projects/pathway-browser/src/app/services/species.service.ts @@ -4,7 +4,6 @@ import { catchError, map, Observable, of } from 'rxjs'; import { CONTENT_SERVICE } from '../../environments/environment'; import { OrthologousMap, Species } from '../model/graph/species.model'; import { Event } from '../model/graph/event/event.model'; -import { ActivatedRoute } from '@angular/router'; import { UrlStateService } from './url-state.service'; import { rxResource } from '@angular/core/rxjs-interop'; import { isDefined } from './utils'; @@ -16,9 +15,6 @@ import { SelectableObject } from './event.service'; export class SpeciesService { private http = inject(HttpClient); private state = inject(UrlStateService); - // Optional: the embeddable diagram has no router. It is read only when the - // reader switches species in the browser, which the embed does not offer. - private route = inject(ActivatedRoute, { optional: true }); private readonly _MAIN_SPECIES = `${CONTENT_SERVICE}/data/species/main`; private readonly _ORTHOLOGIES = `${CONTENT_SERVICE}/data/orthologies/ids/species/`; @@ -133,7 +129,7 @@ export class SpeciesService { } updateQueryParams(map: OrthologousMap, pathwayId: string | undefined) { - const params = { ...this.route?.snapshot.queryParams }; + const params = { ...this.state.queryParams() }; for (const [key, value] of Object.entries(params)) { let newValue = JSON.stringify(value); for (const [initial, replacement] of map) { 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 94f11d8f..cb9dd670 100644 --- a/projects/pathway-browser/src/app/services/url-state.service.ts +++ b/projects/pathway-browser/src/app/services/url-state.service.ts @@ -1,14 +1,11 @@ -import { effect, inject, Injectable, signal, untracked, WritableSignal } from '@angular/core'; -import { ActivatedRoute, NavigationEnd, NavigationExtras, Params, Router } from '@angular/router'; -import { catchError, filter, firstValueFrom, map, of, switchMap } from 'rxjs'; -import { isArray, isNumber } from 'lodash'; +import { inject, Signal, signal, WritableSignal } from '@angular/core'; +import type { NavigationExtras, Params } from '@angular/router'; +import { catchError, firstValueFrom, of } from 'rxjs'; import { HttpClient } from '@angular/common/http'; import { CONTENT_SERVICE } from '../../environments/environment'; -import { PaletteName } from './analysis.service'; +import type { PaletteName } from './analysis.service'; import type { Analysis } from '../model/analysis.model'; import { DEFAULT_INTERACTOR_SCORE } from '../interactors/interactor-threshold'; -import { UntilDestroy, untilDestroyed } from '@ngneat/until-destroy'; -import { toSignal } from '@angular/core/rxjs-interop'; /** * A legacy pathway link, as the old browser addressed one in the URL fragment. @@ -205,307 +202,83 @@ export function createStateValues(oldToNewTab: ReadonlyMap; -@UntilDestroy() -@Injectable({ - providedIn: 'root', -}) -export class UrlStateService implements State { - private route: ActivatedRoute = inject(ActivatedRoute); - private router: Router = inject(Router); - private http: HttpClient = inject(HttpClient); +/** The old browser's tab codes, and the tabs they are now. */ +const TABS_COMPATIBILITY: [string | null, string][] = [ + ['ST', 'details'], + [null, 'details'], + ['MT', 'molecule'], + ['AN', 'results'], + ['EX', 'expression'], + ['DT', 'download'], +]; - private readonly tabsCompatibility: [string | null, string][] = [ - ['ST', 'details'], - [null, 'details'], - ['MT', 'molecule'], - ['AN', 'results'], - ['EX', 'expression'], - ['DT', 'download'], - ]; - readonly oldToNewTab = new Map(this.tabsCompatibility); - readonly newToOldTab = new Map( - this.tabsCompatibility.map(([newTab, oldTab]) => [oldTab, newTab]) - ); - - readonly values = createStateValues(this.oldToNewTab); - - public readonly select = this.values.select; - public readonly flag = this.values.flag; - public readonly path = this.values.path; - public readonly flagInteractors = this.values.flagInteractors; - public readonly overlay = this.values.overlay; - public readonly analysis = this.values.analysis; - public readonly analysisTab = this.values.analysisTab; - public readonly tab = this.values.tab; - public readonly significance = this.values.significance; - public readonly sample = this.values.sample; - public readonly interactorScore = this.values.interactorScore; - public readonly palette = this.values.palette; - public readonly filterViewMode = this.values.filterViewMode; - public readonly speciesFilter = this.values.speciesFilter; - public readonly resourceFilter = this.values.resourceFilter; - public readonly includeDisease = this.values.includeDisease; - public readonly includeGrouping = this.values.includeGrouping; - public readonly pathwayMinSizeFilter = this.values.pathwayMinSizeFilter; - public readonly pathwayMaxSizeFilter = this.values.pathwayMaxSizeFilter; - public readonly minExpressionFilter = this.values.minExpressionFilter; - public readonly maxExpressionFilter = this.values.maxExpressionFilter; - public readonly fdrFilter = this.values.fdrFilter; - public readonly gsaFilter = this.values.gsaFilter; - public readonly summariseDisease = this.values.summariseDisease; - public readonly example = this.values.example; - - public readonly pathwayId = signal(undefined); - - readonly section = toSignal(this.route.fragment); - - constructor() { - this.router.events - .pipe( - filter((event) => event instanceof NavigationEnd), - switchMap(() => { - let route = this.router.routerState.root; - while (route.firstChild) route = route.firstChild; - return route.params; - }), - map((params) => params['pathwayId']) - ) - .subscribe((id) => { - this.pathwayId.set(id); - }); - - effect(() => { - // console.log('Updating patwhayId to ', this.pathwayId()) - - if (isContentRoute(this.router.url)) return; - - void this.navigateTo(this.pathwayId() ?? null, { - queryParamsHandling: 'preserve', - preserveFragment: !this.carriesLegacyPathway(), - }); - }); - - this.route.fragment.pipe(untilDestroyed(this)).subscribe((fragment) => { - if (fragment) { - // Convert fragments to params - const params: Params = {}; - let id = undefined; // Default routing - - const match = fragment.match(FRAGMENT_PATTERN); - if (match?.groups) { - if (match.groups['id']) { - id = match.groups['id']; - } - if (match.groups['params']) { - Object.assign(params, fragmentParams(match.groups['params'])); - } - - // `DIAGRAM` is the old browser's name for the pathway to open, so it is - // the route rather than a setting: `#DIAGRAM=9006934&PATH=162582` in the - // v64 announcement means the same as `#9006934&PATH=162582`. It is only - // ever a dbId, which the resolution below then swaps for the stable id. - if (!id && typeof params['DIAGRAM'] === 'string') { - id = params['DIAGRAM']; - delete params['DIAGRAM']; - } - - // `TOOL=AT` opened the analysis tool. Rewritten into the parameter that - // does that now, rather than carried through as a legacy token, because - // a legacy token does not survive the trip: the writer replaces the - // whole query string as soon as any state settles, so `?tab=info` - // arriving first dropped `?TOOL=AT` before the reader saw it. Setting - // the signal instead does not work either -- the reader resets every - // param the URL does not mention, so it would be undone in the same - // turn. The URL is the source of truth, so the URL is where it goes. - if (params['TOOL'] === 'AT') { - params['analysisTab'] = 'qualitative'; - delete params['TOOL']; - } - } - - const go = (resolved: string | undefined) => - void this.navigateTo(resolved ?? null, { - queryParamsHandling: 'merge', - fragment: fragment.replace(FRAGMENT_PATTERN, ''), - preserveFragment: false, - queryParams: params, - // Replace, do not add. Rewriting a legacy fragment into a proper - // route is a correction, not a step the reader took: pushing it - // left the old URL one Back away, and going back to it rewrote it - // again -- so someone who followed a link out of the news archive - // could not get back to the news archive. - replaceUrl: true, - }); - - // A legacy link may name a pathway by dbId. Navigate on it straight - // away -- the browser resolves one, and going first means the fragment - // is consumed in the same turn as an `#R-HSA-…` one, before anything - // else writes a history entry that still carries it. - // - // Then swap the URL for the stable id, replacing rather than pushing: a - // dbId is not stable across releases, so it is not a URL to leave a - // reader holding, but correcting it is not a step they took. - go(id); - if (id && /^\d+$/.test(id)) { - void this.dbIdToStId(Number(id)).then((stId) => { - if (stId && stId !== id) { - void this.navigateTo(stId, { - queryParamsHandling: 'preserve', - preserveFragment: false, - replaceUrl: true, - }); - } - }); - } - } - }); +/** + * The Pathway Browser's state, as everything that draws a pathway injects it. + * + * Where it is kept is up to the subclass. The site keeps it in the address bar + * (RouterUrlStateService, provided in src/app/app.config.ts); the embeddable + * diagram keeps it in memory (projects/reactome-diagram-element's MemoryState). + * This class imports nothing from the router -- only its types -- so the embed, + * which never routes, does not carry the router in its bundle (#356). + */ +export abstract class UrlStateService implements StateValues { + private readonly http = inject(HttpClient); - this.route.queryParams.pipe(untilDestroyed(this)).subscribe((params) => { - // Kept synchronous: subscribe ignores what the callback returns, so a - // rejection in here would go unreported. - void (async () => { - for (const mainToken in this.values) { - const param = this.values[mainToken as keyof State] as UrlParam; - const tokens: string[] = [mainToken, ...(param.otherTokens || [])]; - const token = tokens.find((token) => params[token] !== undefined); - if (token) { - const initialValue = param.initialValue; - let value = params[token]; - if (value === undefined || value === null) param.set(value); - if (param.otherTransform && token !== mainToken) value = param.otherTransform(value); - else { - if (isArray(initialValue)) { - let values: any[] = value.split(';'); - if (param.type === 'id') { - const mixedValues = values.map((v) => - v.charAt(0).match(/\d/) ? parseInt(v) : v - ); - values = await Promise.all(mixedValues.map((v) => this.ensureStId(v))); - } - if (param.type === 'number') values = values.map((v) => +v); - if (param.type === 'boolean') values = values.map((v) => v === 'true'); - param.set(values); - } else if (param.type === 'boolean') { - param.set(value === 'true'); - } else if (param.type === 'id') { - param.set(await this.ensureStId(value)); - } else if (param.type === 'number') { - param.set(parseFloat(value)); - } else { - param.set(value.replaceAll('__', ' ')); - } - } - } else { - param.set(param.initialValue); - } - } - })().catch((error) => console.error('Could not apply URL parameters', error)); - }); - effect(() => { - const queryParams = this.currentQueryParams(); - if (isContentRoute(this.router.url)) return; + readonly oldToNewTab = new Map(TABS_COMPATIBILITY); + readonly newToOldTab = new Map(TABS_COMPATIBILITY.map(([newTab, oldTab]) => [oldTab, newTab])); - // 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 - // choice nobody made, so Back changed the tab twice before it left. - // - // Decided from the values rather than from a flag. A flag raised by - // settle() and read here outlives its turn whenever this does not run -- - // the early return above, or a default that was already the current value, - // so no signal changed at all -- and the reader's next write, a real one, - // would then quietly replace their history instead of adding to it. - const settled = this.settledParams; - this.settledParams = null; + readonly values = createStateValues(this.oldToNewTab); - void this.navigateTo(this.pathwayId() ?? null, { - queryParams, - preserveFragment: !this.carriesLegacyPathway(), - replaceUrl: settled !== null && settled === JSON.stringify(queryParams), - }); - }); - } + readonly select = this.values.select; + readonly flag = this.values.flag; + readonly path = this.values.path; + readonly flagInteractors = this.values.flagInteractors; + readonly overlay = this.values.overlay; + readonly analysis = this.values.analysis; + readonly analysisTab = this.values.analysisTab; + readonly tab = this.values.tab; + readonly significance = this.values.significance; + readonly sample = this.values.sample; + readonly interactorScore = this.values.interactorScore; + readonly palette = this.values.palette; + readonly filterViewMode = this.values.filterViewMode; + readonly speciesFilter = this.values.speciesFilter; + readonly resourceFilter = this.values.resourceFilter; + readonly includeDisease = this.values.includeDisease; + readonly includeGrouping = this.values.includeGrouping; + readonly pathwayMinSizeFilter = this.values.pathwayMinSizeFilter; + readonly pathwayMaxSizeFilter = this.values.pathwayMaxSizeFilter; + readonly minExpressionFilter = this.values.minExpressionFilter; + readonly maxExpressionFilter = this.values.maxExpressionFilter; + readonly fdrFilter = this.values.fdrFilter; + readonly gsaFilter = this.values.gsaFilter; + readonly summariseDisease = this.values.summariseDisease; + readonly example = this.values.example; + + readonly pathwayId: WritableSignal = signal(undefined); + + /** The section of the page named in the address, if there is one. */ + abstract readonly section: Signal; /** - * Navigate to a pathway within the PathwayBrowser route context. - * Resolves the correct base path whether running standalone or inside the umbrella app. - * - * Router.navigate rejects when navigation fails -- a guard throwing, a - * resolver erroring -- and almost every caller here ignores the result, so - * those failures went nowhere. A rejection handler is attached here so they - * are always reported; the promise is still returned for the one caller that - * legitimately chains on navigation having finished. - */ - /** - * Whether the URL still carries a legacy pathway reference in its fragment. - * - * Such a fragment is on its way out: it is being rewritten into a proper - * route. Until then the other navigations here -- writing state into the URL, - * following a pathway change -- must not carry it along, because every entry - * they carry it into is one that rewrites itself forward when the reader goes - * back to it. Someone who followed a link out of the news archive could not - * get back to the news archive: three Backs, still on the pathway. + * Make a state change the reader did not ask for: where there is a history, + * it does not add a step to it. */ - private carriesLegacyPathway(): boolean { - return FRAGMENT_PATTERN.test(this.route.snapshot.fragment ?? ''); - } - - /** The params settle() last wrote, so the writer can recognise its own work. */ - private settledParams: string | null = null; + abstract settle(change: () => void): void; - /** - * Make a state change the reader did not ask for. - * - * The URL still has to say what is on screen -- a link has to be shareable -- - * but writing a default nobody chose should not cost them a press of Back. - * Choosing a tab pushes; being given one replaces. - */ - settle(change: () => void): void { - change(); - // What the URL should now say. The writer replaces only if it is about to - // write exactly this, which is what keeps the decision out of a flag. - // - // Untracked, and that is the whole of it: settle() is called from inside an - // effect, so reading every param here made that effect depend on every - // param. Choosing a tab then re-ran the defaulting effect, which set the - // tab back to its default -- the reader's click did nothing at all. - this.settledParams = JSON.stringify(untracked(() => this.currentQueryParams())); - } + /** Open a pathway, or none. */ + abstract navigateTo(pathwayId: string | null, extras?: NavigationExtras): Promise; - /** The query params the current state should put in the URL. */ - private currentQueryParams(): Record { - const queryParams: Record = {}; - for (const key in this.values) { - const param = this.values[key as keyof State]; - let paramValue: unknown = param(); - if ( - paramValue === undefined || - paramValue === null || - (isArray(paramValue) && paramValue.length === 0) || - paramValue === param.initialValue - ) - continue; - if (typeof paramValue === 'string') paramValue = paramValue.replaceAll(' ', '__'); - queryParams[key] = isArray(paramValue) ? paramValue.join(';') : paramValue; - } - return queryParams; - } + /** Whether this is the first page opened here: nothing came before it. */ + abstract isFirstNavigation(): boolean; - navigateTo(pathwayId: string | null, extras: NavigationExtras = {}): Promise { - let route = this.router.routerState.root; - while (route.firstChild) route = route.firstChild; - const segments = pathwayId ? [pathwayId] : []; - const navigation = this.router.navigate(segments, { - relativeTo: route.parent, - ...extras, - }); - navigation.catch((error) => console.error('Navigation failed', segments, error)); - return navigation; - } + /** The query parameters in the address now; none where there is no address. */ + abstract queryParams(): Params; async ensureStId(id: string | number): Promise { - return isNumber(id) ? this.dbIdToStId(id) : id; + return typeof id === 'number' ? this.dbIdToStId(id) : id; } /** diff --git a/projects/reactome-diagram-element/src/memory-state.ts b/projects/reactome-diagram-element/src/memory-state.ts index 89c1069a..13828bfd 100644 --- a/projects/reactome-diagram-element/src/memory-state.ts +++ b/projects/reactome-diagram-element/src/memory-state.ts @@ -1,11 +1,6 @@ -import { inject, Injectable, signal } from '@angular/core'; -import { HttpClient } from '@angular/common/http'; -import { catchError, firstValueFrom, of } from 'rxjs'; -import { - createStateValues, - UrlStateService, -} from '../../pathway-browser/src/app/services/url-state.service'; -import { CONTENT_SERVICE } from '../../pathway-browser/src/environments/environment'; +import { Injectable, signal } from '@angular/core'; +import type { Params } from '@angular/router'; +import { UrlStateService } from '../../pathway-browser/src/app/services/url-state.service'; /** * The Pathway Browser's state, held in memory for one embedded diagram. @@ -13,69 +8,32 @@ import { CONTENT_SERVICE } from '../../pathway-browser/src/environments/environm * The browser keeps its state in the address bar, through the Router. On a * partner's page that would read and rewrite *their* address -- every select and * flag pushing history onto a page that is not ours. This offers the same public - * surface (the compiler holds it to UrlStateService's, member for member) and - * touches nothing outside itself. + * state, from the same definitions (it extends UrlStateService), and touches + * nothing outside itself. */ @Injectable() -export class MemoryState implements Pick { - private readonly http = inject(HttpClient); - - // The browser's legacy tab names; an embedded diagram has no tabs. - readonly oldToNewTab = new Map(); - readonly newToOldTab = new Map(); - - readonly values = createStateValues(this.oldToNewTab); - - readonly select = this.values.select; - readonly flag = this.values.flag; - readonly path = this.values.path; - readonly flagInteractors = this.values.flagInteractors; - readonly overlay = this.values.overlay; - readonly analysis = this.values.analysis; - readonly analysisTab = this.values.analysisTab; - readonly tab = this.values.tab; - readonly significance = this.values.significance; - readonly sample = this.values.sample; - readonly interactorScore = this.values.interactorScore; - readonly palette = this.values.palette; - readonly filterViewMode = this.values.filterViewMode; - readonly speciesFilter = this.values.speciesFilter; - readonly resourceFilter = this.values.resourceFilter; - readonly includeDisease = this.values.includeDisease; - readonly includeGrouping = this.values.includeGrouping; - readonly pathwayMinSizeFilter = this.values.pathwayMinSizeFilter; - readonly pathwayMaxSizeFilter = this.values.pathwayMaxSizeFilter; - readonly minExpressionFilter = this.values.minExpressionFilter; - readonly maxExpressionFilter = this.values.maxExpressionFilter; - readonly fdrFilter = this.values.fdrFilter; - readonly gsaFilter = this.values.gsaFilter; - readonly summariseDisease = this.values.summariseDisease; - readonly example = this.values.example; - - readonly pathwayId = signal(undefined); - readonly section = signal(undefined); +export class MemoryState extends UrlStateService { + override readonly section = signal(undefined); /** Nothing to keep off a history stack: there is none. */ - settle(change: () => void): void { + override settle(change: () => void): void { change(); } /** A double-click into a sub-pathway: the diagram changes, the page does not. */ - navigateTo(pathwayId: string | null): Promise { + override navigateTo(pathwayId: string | null): Promise { this.pathwayId.set(pathwayId ?? undefined); this.select.set(null); return Promise.resolve(true); } - async ensureStId(id: string | number): Promise { - return typeof id === 'number' ? this.dbIdToStId(id) : id; + /** There is no history here, so every diagram is the first. */ + override isFirstNavigation(): boolean { + return true; } - async dbIdToStId(dbId: number): Promise { - return firstValueFrom( - this.http - .get(`${CONTENT_SERVICE}/data/query/${dbId}/stId`, { responseType: 'text' }) - .pipe(catchError(() => of(dbId + ''))) - ); + /** There is no address to read. */ + override queryParams(): Params { + return {}; } } diff --git a/src/app/app.config.ts b/src/app/app.config.ts index 36711ccb..dca2b3b5 100644 --- a/src/app/app.config.ts +++ b/src/app/app.config.ts @@ -15,6 +15,8 @@ import { import { environment } from '../../projects/pathway-browser/src/environments/environment'; import { routes } from './app.routes'; +import { UrlStateService } from '../../projects/pathway-browser/src/app/services/url-state.service'; +import { RouterUrlStateService } from '../../projects/pathway-browser/src/app/services/router-url-state.service'; /** * Analytics, when the deployment has a property to report to. @@ -40,6 +42,10 @@ export const appConfig: ApplicationConfig = { // template event bindings and explicit markForCheck instead. provideZonelessChangeDetection(), provideRouter(routes), + // The Pathway Browser's state lives in the address bar on the site. The + // token is router-free so the embeddable diagram can keep it in memory + // without carrying the router (#356). + { provide: UrlStateService, useClass: RouterUrlStateService }, provideHttpClient(withFetch()), // Loaded on demand rather than eagerly: the only animations in the app // are in the lazily routed pathway browser, so the animations engine has From cf6f23b4febbca17d3585594fdda96f3d797461b Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Tue, 13 Oct 2026 04:58:41 +0000 Subject: [PATCH 2/4] createStateValues is the base class's own now; unexport it Co-Authored-By: Claude Opus 5.5 --- .../src/app/services/url-state.service.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 cb9dd670..ade0c10d 100644 --- a/projects/pathway-browser/src/app/services/url-state.service.ts +++ b/projects/pathway-browser/src/app/services/url-state.service.ts @@ -144,12 +144,12 @@ export function urlParam( /** * Every piece of Pathway Browser state, as signals. * - * A factory so the embeddable diagram's in-memory state - * (projects/reactome-diagram-element) is built from exactly these definitions: - * two copies of this list would drift, and a param added here but not there is - * a param the embedded diagram silently ignores. + * Built once per state, in UrlStateService below, so the site's address-bar + * state and the embeddable diagram's in-memory state are made from exactly + * these definitions: two copies of this list would drift, and a param added + * here but not there is a param the embedded diagram silently ignores. */ -export function createStateValues(oldToNewTab: ReadonlyMap) { +function createStateValues(oldToNewTab: ReadonlyMap) { return { select: urlParam(null, 'id', ['SEL']), flag: urlParam([], 'id', ['FLG']), From 7ae376d7f1f3509e68b8ec6dc80ee764933d4d1d Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Tue, 13 Oct 2026 05:10:35 +0000 Subject: [PATCH 3/4] Review: a copy of the query params, a stale comment gone, DetailUrlState says it is unchecked Co-Authored-By: Claude Opus 5.5 --- .../src/app/diagram/diagram.component.ts | 2 -- .../src/app/services/router-url-state.service.ts | 3 ++- .../src/app/services/species.service.ts | 2 +- .../detail/providers/detail-url-state.provider.ts | 13 ++++++++++--- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/projects/pathway-browser/src/app/diagram/diagram.component.ts b/projects/pathway-browser/src/app/diagram/diagram.component.ts index 67da0677..ee624f3e 100644 --- a/projects/pathway-browser/src/app/diagram/diagram.component.ts +++ b/projects/pathway-browser/src/app/diagram/diagram.component.ts @@ -112,8 +112,6 @@ export class DiagramComponent implements AfterViewInit, OnDestroy { protected state = inject(UrlStateService); analysis = inject(AnalysisService); private event = inject(EventService); - // Optional: the embeddable diagram (projects/reactome-diagram-element) has no - // router -- it must never read or write its host page's address. private download = inject(DownloadService); private data = inject(DataStateService); private deltaSignal = inject(DeltaSignalService); diff --git a/projects/pathway-browser/src/app/services/router-url-state.service.ts b/projects/pathway-browser/src/app/services/router-url-state.service.ts index 58c08280..7a27590b 100644 --- a/projects/pathway-browser/src/app/services/router-url-state.service.ts +++ b/projects/pathway-browser/src/app/services/router-url-state.service.ts @@ -277,6 +277,7 @@ export class RouterUrlStateService extends UrlStateService { } override queryParams(): Params { - return this.route.snapshot.queryParams; + // A copy: the snapshot's object is the router's own. + return { ...this.route.snapshot.queryParams }; } } diff --git a/projects/pathway-browser/src/app/services/species.service.ts b/projects/pathway-browser/src/app/services/species.service.ts index 5472af10..331e1b10 100644 --- a/projects/pathway-browser/src/app/services/species.service.ts +++ b/projects/pathway-browser/src/app/services/species.service.ts @@ -129,7 +129,7 @@ export class SpeciesService { } updateQueryParams(map: OrthologousMap, pathwayId: string | undefined) { - const params = { ...this.state.queryParams() }; + const params = this.state.queryParams(); for (const [key, value] of Object.entries(params)) { let newValue = JSON.stringify(value); for (const [initial, replacement] of map) { diff --git a/projects/website-angular/src/app/content/detail/providers/detail-url-state.provider.ts b/projects/website-angular/src/app/content/detail/providers/detail-url-state.provider.ts index aa9a48ac..fafb4f05 100644 --- a/projects/website-angular/src/app/content/detail/providers/detail-url-state.provider.ts +++ b/projects/website-angular/src/app/content/detail/providers/detail-url-state.provider.ts @@ -7,6 +7,13 @@ import { } from '../../../../../../pathway-browser/src/app/services/url-state.service'; import { DEFAULT_INTERACTOR_SCORE } from '../../../../../../pathway-browser/src/app/interactors/interactor-threshold'; +/** + * The state the content pages read, for the Pathway Browser components they + * reuse. Partial on purpose, and so unchecked: nothing makes it carry the rest + * of UrlStateService (settle, navigateTo, isFirstNavigation, queryParams), and + * nothing on these pages calls them -- their diagram is an image, not + * cr-diagram. Reuse a component that does, and it will need them. + */ @Injectable() export class DetailUrlState implements Partial { private route = inject(ActivatedRoute); @@ -28,9 +35,9 @@ export class DetailUrlState implements Partial { ); significance = urlParam(0.05, 'number'); sample = urlParam(null, 'string'); - // Mirrors UrlStateService. The content pages do not draw interactors, but - // this class declares the same shape, so a param added there has to appear - // here or the build fails -- which is how this one was found. + // Mirrors UrlStateService. The content pages do not draw interactors, but a + // component they reuse read this param, and finding it missing here is how it + // was added. interactorScore = urlParam(DEFAULT_INTERACTOR_SCORE, 'number'); palette = urlParam(null, 'string'); filterViewMode = urlParam(undefined, 'string'); From 094cb2e588f0cf06d3e38e457cf4749176adeb0f Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Tue, 13 Oct 2026 05:18:57 +0000 Subject: [PATCH 4/4] DetailUrlState: say only what is known about interactorScore Co-Authored-By: Claude Opus 5.5 --- .../content/detail/providers/detail-url-state.provider.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/projects/website-angular/src/app/content/detail/providers/detail-url-state.provider.ts b/projects/website-angular/src/app/content/detail/providers/detail-url-state.provider.ts index fafb4f05..83dd70b2 100644 --- a/projects/website-angular/src/app/content/detail/providers/detail-url-state.provider.ts +++ b/projects/website-angular/src/app/content/detail/providers/detail-url-state.provider.ts @@ -35,9 +35,8 @@ export class DetailUrlState implements Partial { ); significance = urlParam(0.05, 'number'); sample = urlParam(null, 'string'); - // Mirrors UrlStateService. The content pages do not draw interactors, but a - // component they reuse read this param, and finding it missing here is how it - // was added. + // Mirrors UrlStateService, though the content pages do not draw interactors. + // Nothing checks the two stay in step (see the class comment), so keep them so. interactorScore = urlParam(DEFAULT_INTERACTOR_SCORE, 'number'); palette = urlParam(null, 'string'); filterViewMode = urlParam(undefined, 'string');