From 386e9660681113819ff573703cb4024e2b79272b Mon Sep 17 00:00:00 2001 From: 4gray Date: Tue, 6 Oct 2026 23:16:02 +0200 Subject: [PATCH] fix(workspace): adopt the router navigation the Back depth tracker missed Review follow-up (Codex, Greptile): the lazy workspace shell creates the Back service after the first NavigationStart, so the tracker saw only its NavigationEnd, left the depth unknown and counted the next push as the first entry. It now adopts the router's current or last successful navigation when it starts: a first navigation is depth 0, a later one leaves the depth unknown (browser history Back), and a late start of the adopted navigation is not counted again. Co-Authored-By: Claude Opus 5.5 --- docs/architecture/workspace-shell.md | 5 +- .../src/lib/router-history-depth.ts | 23 ++++- .../workspace-back-navigation.service.spec.ts | 89 ++++++++++++++++++- 3 files changed, 113 insertions(+), 4 deletions(-) diff --git a/docs/architecture/workspace-shell.md b/docs/architecture/workspace-shell.md index 655e9ed0f..829e6e5f9 100644 --- a/docs/architecture/workspace-shell.md +++ b/docs/architecture/workspace-shell.md @@ -174,7 +174,10 @@ entry is an in-app one, by the same Navigation API test as the history fallback. Without the API (older Safari and Firefox) the router's history depth decides (`trackRouterHistoryDepth`): the document's first navigation is depth 0, a push adds one, a replacement keeps it and a traversal restores the -depth recorded for its entry. A traversal to an entry from before a reload +depth recorded for its entry. The lazy workspace shell creates the service +after the first navigation began, so the tracker adopts the router's current +or last navigation: a first one is depth 0, a later one leaves the depth +unknown. A traversal to an entry from before a reload leaves the depth unknown and keeps `Location.back()`, which then has a previous entry. Otherwise the page opened the session (a deep link, a reload or a restored view), where diff --git a/libs/portal/shared/data-access/src/lib/router-history-depth.ts b/libs/portal/shared/data-access/src/lib/router-history-depth.ts index 46368a4ef..a41a4786d 100644 --- a/libs/portal/shared/data-access/src/lib/router-history-depth.ts +++ b/libs/portal/shared/data-access/src/lib/router-history-depth.ts @@ -19,7 +19,10 @@ import { * leaves the depth unknown (null). */ export function trackRouterHistoryDepth( - router: Pick, + router: Pick< + Router, + 'events' | 'currentNavigation' | 'lastSuccessfulNavigation' + >, destroyRef: Pick ): () => number | null { let depth: number | null = null; @@ -30,8 +33,26 @@ export function trackRouterHistoryDepth( // unknown, which keeps browser history Back. if (!router.events) return () => null; + // The tracker can start after the first navigation began: the workspace + // shell that creates it is lazy. Adopt the router's state for the events + // it missed. Only a first navigation has a known depth (0); a later one + // leaves it unknown, which keeps browser history Back. + const inFlight = router.currentNavigation?.() ?? null; + const last = router.lastSuccessfulNavigation?.() ?? null; + const adoptedId = inFlight?.id ?? null; + if (inFlight) { + started = true; + pending = inFlight.previousNavigation === null ? 0 : null; + } else if (last) { + started = true; + depth = last.previousNavigation === null ? 0 : null; + if (depth !== null) depthByNavigationId.set(last.id, depth); + } + const subscription = router.events.subscribe((event) => { if (event instanceof NavigationStart) { + // Already adopted above; its start may still be on its way. + if (event.id === adoptedId) return; if (!started) { pending = 0; } else if (event.navigationTrigger === 'popstate') { diff --git a/libs/portal/shared/data-access/src/lib/workspace-back-navigation.service.spec.ts b/libs/portal/shared/data-access/src/lib/workspace-back-navigation.service.spec.ts index d371d61d3..2a5222dbd 100644 --- a/libs/portal/shared/data-access/src/lib/workspace-back-navigation.service.spec.ts +++ b/libs/portal/shared/data-access/src/lib/workspace-back-navigation.service.spec.ts @@ -52,9 +52,14 @@ describe('WorkspaceBackNavigationService', () => { const navigate = jest.fn().mockResolvedValue(true); const navigateByUrl = jest.fn().mockResolvedValue(true); let routerEvents = new Subject(); - const currentNavigation = signal<{ + /** The parts of a router Navigation the depth tracker reads. */ + type NavigationLike = { + id?: number; + previousNavigation?: unknown; extras: { replaceUrl?: boolean }; - } | null>(null); + }; + const currentNavigation = signal(null); + const lastSuccessfulNavigation = signal(null); let navigationId = 0; /** A router navigation as the history depth tracker sees it. */ @@ -100,6 +105,7 @@ describe('WorkspaceBackNavigationService', () => { navigateByUrl, events: routerEvents, currentNavigation, + lastSuccessfulNavigation, }, }, { @@ -111,6 +117,11 @@ describe('WorkspaceBackNavigationService', () => { return TestBed.inject(WorkspaceBackNavigationService); } + afterEach(() => { + currentNavigation.set(null); + lastSuccessfulNavigation.set(null); + }); + function createTarget(run = jest.fn()): WorkspaceBackTarget { return { label: signal(null), @@ -374,6 +385,80 @@ describe('WorkspaceBackNavigationService', () => { expect(navigateByUrl).toHaveBeenCalledTimes(1); }); + // The lazy workspace shell creates the service after the first + // navigation started, so the tracker adopts the router's state. + it('adopts a first navigation that ended before the service existed', async () => { + lastSuccessfulNavigation.set({ + id: 1, + previousNavigation: null, + extras: {}, + }); + const service = createService(null); + + service.back(() => '/workspace/dashboard'); + await Promise.resolve(); + + expect(back).not.toHaveBeenCalled(); + expect(navigateByUrl).toHaveBeenCalledTimes(1); + }); + + it('adopts a first navigation whose start fired before the service existed', async () => { + currentNavigation.set({ + id: 1, + previousNavigation: null, + extras: {}, + }); + const service = createService(null); + navigationId = 1; + // Only the end reaches the tracker. + routerEvents.next(new NavigationEnd(1, '/page-1', '/page-1')); + currentNavigation.set(null); + + service.back(() => '/workspace/dashboard'); + await Promise.resolve(); + + expect(back).not.toHaveBeenCalled(); + expect(navigateByUrl).toHaveBeenCalledTimes(1); + }); + + it('adopts a first navigation still in flight and counts the next push', async () => { + currentNavigation.set({ + id: 1, + previousNavigation: null, + extras: {}, + }); + const service = createService(null); + navigationId = 1; + // Its start arrives late; the end commits depth 0. + routerEvents.next(new NavigationStart(1, '/page-1')); + routerEvents.next(new NavigationEnd(1, '/page-1', '/page-1')); + currentNavigation.set(null); + + service.back(() => '/workspace/dashboard'); + await Promise.resolve(); + expect(back).not.toHaveBeenCalled(); + expect(navigateByUrl).toHaveBeenCalledTimes(1); + + routerNavigation(); + service.back(() => '/workspace/dashboard'); + expect(back).toHaveBeenCalledTimes(1); + }); + + it('keeps browser history when created after several navigations', () => { + lastSuccessfulNavigation.set({ + id: 3, + previousNavigation: { id: 2 }, + extras: {}, + }); + const service = createService(null); + const parent = jest.fn(() => '/workspace/dashboard'); + + service.back(parent); + + expect(back).toHaveBeenCalledTimes(1); + expect(parent).not.toHaveBeenCalled(); + }); + it('keeps browser history on an entry from before a reload', () => { const service = createService(null); routerNavigation();