From a92961118193ed09138f1eb5a9b2e465bd004021 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 9 Sep 2026 18:06:30 +0000 Subject: [PATCH] fix(routing): let a reader come back from a legacy link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial pass over #172 and #182. **Following a legacy link trapped the reader.** From the news archive, opening `/PathwayBrowser/#1280218` and pressing Back stayed on the pathway -- six times over. The rewrite is the cause: the app writes state into the URL while the fragment is still there, so `/PathwayBrowser#1280218` and `?tab=details#1280218` both became history entries carrying it, and going back to such an entry rewrites it forward again. Both PRs shipped this; the `#R-HSA-…` links have had it since #172. Two changes. A fragment being consumed is no longer carried forward by the navigations that write state into the URL, so no entry retains it. And a dbId is navigated on immediately rather than after its lookup -- going first means the fragment is consumed in the same turn as a stable-id one, before anything else can record it -- with the URL then corrected to the stable id by a replace, which adds no entry. Measured from /about/news: a dbId link now takes 3 Backs to leave, a stable-id link 4, and a direct load 3. It was never. **I had also written a resolver that already existed.** `dbIdToStId` has been on this service all along, unused, and it is the better one: it asks `/data/query//stId` and gets 13 bytes, where mine fetched the whole object for one field -- 12,682. Mine is gone and the existing one has its first caller. Not fixed, and not mine: a pathway load adds two history entries of its own before any of this, which is why leaving still takes three Backs rather than one. Every navigation in this service pushes, including the ones that are only normalising the URL. Worth a look, but making them replace would also stop Back working between pathways, so it needs deciding rather than just changing. The escape is now a test, because it broke twice without anything noticing. Co-Authored-By: Claude Opus 5 --- e2e/legacy-links.spec.ts | 72 ++++++++++++++++++ .../src/app/services/url-state.service.ts | 75 ++++++++++++------- 2 files changed, 122 insertions(+), 25 deletions(-) create mode 100644 e2e/legacy-links.spec.ts diff --git a/e2e/legacy-links.spec.ts b/e2e/legacy-links.spec.ts new file mode 100644 index 0000000..e0f277e --- /dev/null +++ b/e2e/legacy-links.spec.ts @@ -0,0 +1,72 @@ +import { test, expect, type Page } from '@playwright/test'; + +/** + * A link out of the news archive takes you somewhere, and lets you come back. + * + * The news carries 278 pathway links written the way the old browser addressed + * one: a bare dbId in the fragment, `/PathwayBrowser/#1280218`, plus 86 more as + * `#R-HSA-…`. Both are rewritten into proper routes now (#172, #182). + * + * Rewriting a URL under the reader is easy to get wrong in a way nothing else + * catches. Twice it left a history entry that still carried the fragment, and + * going back to such an entry rewrites it forward again -- so someone who + * followed a link out of the news archive could not get back to the news + * archive at all. That is what this holds down. + * + * The count of Backs is deliberately loose: the browser already adds a couple of + * entries of its own while a pathway settles, which a direct load does too. What + * matters is that the reader can leave. + */ + +const NEWS = '/about/news'; +const BOOT_TIMEOUT = 90_000; + +async function loadPathway(page: Page, target: string) { + await page.goto(`/PathwayBrowser/${target}`, { waitUntil: 'domcontentloaded' }); + await page.waitForFunction( + () => { + const container = document.querySelector('#cytoscape') as + (HTMLElement & { _cyreg?: { cy?: { elements(): { length: number } } } }) | null; + const drawn = container?._cyreg?.cy?.elements().length ?? 0; + return drawn > 0 || Boolean(document.querySelector('cr-ehld svg')); + }, + { timeout: BOOT_TIMEOUT } + ); +} + +/** Press Back until we are out of the pathway browser, or give up. */ +async function backOutOf(page: Page, limit = 6) { + for (let step = 1; step <= limit; step++) { + await page.goBack({ waitUntil: 'domcontentloaded' }).catch(() => undefined); + await page.waitForTimeout(2500); + if (new URL(page.url()).pathname === NEWS) return step; + } + return null; +} + +test.describe('Legacy pathway links', () => { + test.describe.configure({ timeout: 5 * 60 * 1000 }); + + // Both spellings that appear in the content, and a direct load to compare + // against -- if the direct load needs as many Backs, the entries are the + // browser's own doing rather than the rewrite's. + for (const [target, label] of [ + ['#1280218', 'a dbId fragment, as the release announcements write it'], + ['#R-HSA-202733', 'a stable id fragment'], + ['R-HSA-1280218', 'a direct stable id, for comparison'], + ]) { + test(`${label}: opens, and you can go back`, async ({ page }) => { + await page.goto(NEWS); + await page.waitForTimeout(1500); + + await loadPathway(page, target); + + // Whatever it was written as, the reader ends up on a stable id: a dbId + // is not stable across releases, so it is not a URL to leave them with. + expect(new URL(page.url()).pathname).toMatch(/\/PathwayBrowser\/R-[A-Z]{3}-\d+/); + + const steps = await backOutOf(page); + expect(steps, 'the reader could not get back to the news archive').not.toBeNull(); + }); + } +}); 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 18f5955..856a5ef 100644 --- a/projects/pathway-browser/src/app/services/url-state.service.ts +++ b/projects/pathway-browser/src/app/services/url-state.service.ts @@ -71,20 +71,6 @@ export class UrlStateService implements State { private router: Router = inject(Router); private http: HttpClient = inject(HttpClient); - /** - * The stable id for a dbId. - * - * Falls back to the dbId when the lookup fails, and says so: a page that - * loads on a less-good URL beats a link that goes nowhere, and the alternative - * -- refusing to navigate -- would turn a working legacy link into a dead one. - */ - private stableIdFor(dbId: string) { - return this.http.get<{ stId?: string }>(`${CONTENT_SERVICE}/data/query/${dbId}`).pipe( - map((object) => object?.stId), - catchError(() => of(undefined)) - ); - } - private readonly tabsCompatibility: [string | null, string][] = [ ['ST', 'details'], [null, 'details'], @@ -183,7 +169,7 @@ export class UrlStateService implements State { void this.navigateTo(this.pathwayId() ?? null, { queryParamsHandling: 'preserve', - preserveFragment: true, + preserveFragment: !this.carriesLegacyPathway(), }); }); @@ -214,18 +200,33 @@ export class UrlStateService implements State { 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. The browser can load one, - // but the reader would then be left on a dbId URL to copy and share, - // and a dbId is not stable across releases. Resolve it and navigate to - // the stable id instead, so an old link hands over a good one. + // 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)) { - this.stableIdFor(id) - .pipe(untilDestroyed(this)) - .subscribe((stId) => go(stId ?? id)); - } else { - go(id); + void this.dbIdToStId(Number(id)).then((stId) => { + if (stId && stId !== id) { + void this.navigateTo(stId, { + queryParamsHandling: 'preserve', + preserveFragment: false, + replaceUrl: true, + }); + } + }); } } }); @@ -291,7 +292,10 @@ export class UrlStateService implements State { // console.log('In content or search route, not navigating on state change'); return; } - void this.navigateTo(this.pathwayId() ?? null, { queryParams, preserveFragment: true }); + void this.navigateTo(this.pathwayId() ?? null, { + queryParams, + preserveFragment: !this.carriesLegacyPathway(), + }); }); } @@ -305,6 +309,20 @@ export class UrlStateService implements State { * 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 ?? ''); + } + navigateTo(pathwayId: string | null, extras: NavigationExtras = {}): Promise { let route = this.router.routerState.root; while (route.firstChild) route = route.firstChild; @@ -321,6 +339,13 @@ export class UrlStateService implements State { return isNumber(id) ? this.dbIdToStId(id) : id; } + /** + * The stable id for a dbId, asked for as text rather than as an object. + * + * `/stId` answers 13 bytes; fetching the object to read one field off it is + * 12,682. Falls back to the dbId, which the browser can still load: a page on + * a worse URL beats a link that goes nowhere. + */ async dbIdToStId(dbId: number): Promise { return firstValueFrom( this.http