From 38f2462519aad57787251bb571dc294506d125a0 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 9 Sep 2026 19:17:12 +0000 Subject: [PATCH 1/2] fix(routing): decide "replace or add" from the URL, not from a flag #184 put a `settling` boolean on the service: settle() raised it, the writer effect read it and lowered it. That only works if the writer always runs, once, right after. It does not: - the writer returns early on the content and search routes, before it ever lowers the flag; - a default that already matches the current value changes no signal, so the writer never runs at all. Either way the flag outlives its turn, and the reader's next write -- a real choice -- silently replaces their history instead of adding to it. So the writer now decides from the values. settle() records the params the URL should say once its change has been applied; the writer replaces only if it is about to write exactly those, and clears the record either way. Both sides go through one currentQueryParams(), so they agree by construction rather than by a comment. The record is taken untracked. settle() is called from inside the details defaulting effect, so reading every param made that effect depend on every param: choosing the Molecule tab re-ran the default, which set the tab straight back to details. The URL never changed and the click did nothing -- caught by e2e/back-button.spec.ts, which now passes both ways again. Co-Authored-By: Claude Opus 5 --- lint-baseline.json | 2 +- .../src/app/services/url-state.service.ts | 78 +++++++++++-------- 2 files changed, 48 insertions(+), 32 deletions(-) diff --git a/lint-baseline.json b/lint-baseline.json index e0ad8d5c..ac58d760 100644 --- a/lint-baseline.json +++ b/lint-baseline.json @@ -1,3 +1,3 @@ { - "warnings": 654 + "warnings": 653 } 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 91284e58..27e1b82f 100644 --- a/projects/pathway-browser/src/app/services/url-state.service.ts +++ b/projects/pathway-browser/src/app/services/url-state.service.ts @@ -1,4 +1,4 @@ -import { effect, inject, Injectable, signal, WritableSignal } from '@angular/core'; +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'; @@ -273,35 +273,25 @@ export class UrlStateService implements State { })().catch((error) => console.error('Could not apply URL parameters', error)); }); effect(() => { - const queryParams = {} as any; - for (const key in this.values) { - const param = this.values[key as keyof State]; - let paramValue = 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; - } - // console.log('Updating URL from state', queryParams) - if (this.router.url.includes('content') || this.router.url.includes('query')) { - // 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; + const queryParams = this.currentQueryParams(); + if (this.router.url.includes('content') || this.router.url.includes('query')) 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: settling, + replaceUrl: settled !== null && settled === JSON.stringify(queryParams), }); }); } @@ -330,19 +320,45 @@ 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; + /** The params settle() last wrote, so the writer can recognise its own work. */ + private settledParams: string | null = null; /** - * Make a state change that the reader did not ask for. + * 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 { - this.settling = true; 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 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; } navigateTo(pathwayId: string | null, extras: NavigationExtras = {}): Promise { From d77b1bf67e721e9a80a8e34c65d273846c9a2af3 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 9 Sep 2026 19:21:42 +0000 Subject: [PATCH 2/2] docs(hierarchy): write down why the scroll restore takes one frame #181 restores the reader's scroll position on frame one; RevealDirective brings a selected row into view on frame two. That order is what makes `block: 'nearest'` compare against where the reader actually was, so a row already on screen is left alone. Reverse it and clicking a visible row scrolls twice again -- the bug #181 fixed. Neither side said so, and both comments read as if their frame count were a local choice. Noted in both, because either one being "simplified" breaks the other. Co-Authored-By: Claude Opus 5 --- .../src/app/event-hierarchy/event-hierarchy.component.ts | 6 ++++++ projects/pathway-browser/src/app/utils/reveal.directive.ts | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.ts b/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.ts index 22182e83..df9cf394 100644 --- a/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.ts +++ b/projects/pathway-browser/src/app/event-hierarchy/event-hierarchy.component.ts @@ -241,6 +241,12 @@ export class EventHierarchyComponent implements AfterViewInit, OnDestroy { // Again once the rows have been laid out: the height is not final until // the restored branches have rendered, and the browser clamps a // scrollTop set against a container that is still short. + // + // One frame, and it has to stay one: RevealDirective brings a selected + // row into view on its *second* frame, so restoring first is what lets + // its `block: 'nearest'` see the reader's real position and decide the + // row is already visible. Restore on a later frame than reveal and every + // click scrolls twice again. requestAnimationFrame(() => { if (scroller.scrollTop !== scrollTop) scroller.scrollTop = scrollTop; }); diff --git a/projects/pathway-browser/src/app/utils/reveal.directive.ts b/projects/pathway-browser/src/app/utils/reveal.directive.ts index b4775c9b..ec3e1dc2 100644 --- a/projects/pathway-browser/src/app/utils/reveal.directive.ts +++ b/projects/pathway-browser/src/app/utils/reveal.directive.ts @@ -47,6 +47,13 @@ export class RevealDirective { // table that is still expanding rows, or paging to a different page, moves // after the first frame. Revealing then left it 28px past the edge of its // container. The second frame is after that layout has been painted. + // + // It is also what keeps the event hierarchy still. That tree rebuilds + // itself from scratch on every click and restores the reader's scroll + // position on frame one; arriving on frame two means `nearest` below is + // comparing against where the reader actually was, and so does nothing + // when the row was already on screen. Cut this to one frame and clicking + // a visible row jumps the hierarchy again. let frame = requestAnimationFrame(() => { frame = requestAnimationFrame(() => { this.element.nativeElement.scrollIntoView({