From 4ece7adf32bbb84a5d23e2295bf13a9183ec5784 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 9 Sep 2026 18:50:19 +0000 Subject: [PATCH] fix(routing): let Back leave the pathway browser Opening a pathway cost three presses of Back to get out of, and the first two did something nobody asked for. The app writes its own defaults into the URL, so `?tab=info` and then `?tab=details` each became a history entry: pressing Back stepped through tab changes the reader never made before it would leave the page. The URL still has to carry the tab -- a shared link has to open on the same thing -- so the fix is not to stop writing it. Being *given* a tab now replaces the entry; *choosing* one still adds to it. `details.component` already knew the difference, in `tabCameFromUrl`; there was just no way to tell the URL writer, so `settle()` is that way. From the news archive: one press of Back leaves, where it was three. Choosing the Molecule tab and pressing Back still returns to Details. Found while reviewing #183, where I called this pre-existing and left it. It was pre-existing, and it turned out to be one flag rather than the rework of navigation semantics I assumed -- the reason I gave for not doing it was wrong. Both halves are tested, because keeping the second is what stops the first turning into "Back does nothing in this app". Co-Authored-By: Claude Opus 5 --- e2e/back-button.spec.ts | 82 +++++++++++++++++++ .../src/app/details/details.component.ts | 13 ++- .../src/app/services/url-state.service.ts | 22 +++++ 3 files changed, 113 insertions(+), 4 deletions(-) create mode 100644 e2e/back-button.spec.ts diff --git a/e2e/back-button.spec.ts b/e2e/back-button.spec.ts new file mode 100644 index 00000000..6123a594 --- /dev/null +++ b/e2e/back-button.spec.ts @@ -0,0 +1,82 @@ +import { test, expect, type Page } from '@playwright/test'; + +/** + * Back leaves the pathway browser, and undoes what the reader actually did. + * + * Opening a pathway used to cost three presses of Back to get out of, and the + * first two did something nobody asked for: the app writes its own defaults into + * the URL, so `?tab=info` and then `?tab=details` each became a history entry, + * and going back stepped through tab changes the reader never made before it + * would leave the page. + * + * The URL still has to carry the tab -- a shared link has to open on the same + * thing -- so the fix is not to stop writing it. It is that being *given* a tab + * replaces the entry while *choosing* one adds to it. Both halves are here, + * because keeping the second is what stops the first turning into "Back does + * nothing in this app". + */ + +const NEWS = '/about/news'; +const PATHWAY = 'R-HSA-109606'; +const BOOT_TIMEOUT = 90_000; + +async function openPathway(page: Page) { + await page.goto(`/PathwayBrowser/${PATHWAY}`, { waitUntil: 'domcontentloaded' }); + 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: BOOT_TIMEOUT } + ); + // The tab settles a moment after the diagram. + await page.waitForTimeout(2500); +} + +test.describe('The back button', () => { + test.describe.configure({ timeout: 4 * 60 * 1000 }); + + test('leaves the pathway browser without stepping through tabs first', async ({ page }) => { + await page.goto(NEWS); + await page.waitForTimeout(1500); + + await openPathway(page); + // The tab is still in the URL: a shared link must open on the same thing. + expect(new URL(page.url()).search).toContain('tab='); + + let steps = 0; + let where = ''; + while (steps < 4) { + await page.goBack({ waitUntil: 'domcontentloaded' }).catch(() => undefined); + await page.waitForTimeout(2500); + steps++; + where = new URL(page.url()).pathname; + if (where === NEWS) break; + } + + expect(where, `still in the pathway browser after ${steps} presses`).toBe(NEWS); + // It was three, two of them spent changing tabs nobody chose. + expect(steps, 'presses of Back needed to leave').toBeLessThanOrEqual(2); + }); + + test('still undoes a tab the reader chose', async ({ page }) => { + await openPathway(page); + const settled = new URL(page.url()).search; + + 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); + + const chosen = new URL(page.url()).search; + expect(chosen, 'choosing a tab changes the URL').not.toBe(settled); + + await page.goBack({ waitUntil: 'domcontentloaded' }); + await page.waitForTimeout(2500); + expect(new URL(page.url()).search, 'a choice the reader made is undoable').toBe(settled); + }); +}); diff --git a/projects/pathway-browser/src/app/details/details.component.ts b/projects/pathway-browser/src/app/details/details.component.ts index f24f9215..cc5a281e 100644 --- a/projects/pathway-browser/src/app/details/details.component.ts +++ b/projects/pathway-browser/src/app/details/details.component.ts @@ -90,10 +90,15 @@ export class DetailsComponent { // when an analysis finishes, which is the point of running one. if (tabCameFromUrl) return; - if (this.state.section()) this.state.tab.set('details'); - else if (this.hasResult()) this.state.tab.set('results'); - else if (this.hasDetail()) this.state.tab.set('details'); - else this.state.tab.set('info'); + // A default, not a choice. The URL still has to say what is on screen so a + // link is shareable, but being given a tab should not cost the reader a + // press of Back the way choosing one does. + this.state.settle(() => { + if (this.state.section()) this.state.tab.set('details'); + else if (this.hasResult()) this.state.tab.set('results'); + else if (this.hasDetail()) this.state.tab.set('details'); + else this.state.tab.set('info'); + }); }); } } 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 856a5efa..91284e58 100644 --- a/projects/pathway-browser/src/app/services/url-state.service.ts +++ b/projects/pathway-browser/src/app/services/url-state.service.ts @@ -292,9 +292,16 @@ export class UrlStateService implements State { // console.log('In content or search route, not navigating on state change'); return; } + // Settling is not a step the reader took, so it replaces rather than + // adds. Opening a pathway used to write ?tab=info and then ?tab=details, + // two entries for a choice nobody made -- so pressing Back changed the + // tab twice before it left the page. + const settling = this.settling; + this.settling = false; void this.navigateTo(this.pathwayId() ?? null, { queryParams, preserveFragment: !this.carriesLegacyPathway(), + replaceUrl: settling, }); }); } @@ -323,6 +330,21 @@ export class UrlStateService implements State { return FRAGMENT_PATTERN.test(this.route.snapshot.fragment ?? ''); } + /** Whether the URL write now pending is the app settling its own defaults. */ + private settling = false; + + /** + * Make a state change that 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 { + this.settling = true; + change(); + } + navigateTo(pathwayId: string | null, extras: NavigationExtras = {}): Promise { let route = this.router.routerState.root; while (route.firstChild) route = route.firstChild;