fix(routing): three ways URL state stopped reaching the URL

lint-baseline.json

@@ -1,3 +1,3 @@
 {
-  "warnings": 654
+  "warnings": 653
 }

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;
         });

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<string, unknown> {
+    const queryParams: Record<string, unknown> = {};
+    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<boolean> {

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({