From 575d301e19b19e9e4ff6622498e2fa936a24145f Mon Sep 17 00:00:00 2001 From: Peter Zimon Date: Thu, 27 Aug 2026 11:44:02 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20stale=20sidebar=20select?= =?UTF-8?q?ions=20across=20Admin=20routes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fixes https://linear.app/ghost/issue/DES-1491/posts-and-pages-selection-gets-stuck-in-nav-sidebar React-owned navigation uses pushState, which leaves Ember's last route active. Trust bridged route state only while an Ember fallback is rendered so the sidebar follows the router that owns the current screen. --- .../src/ember-bridge/ember-bridge.test.tsx | 11 +++-- apps/admin/src/ember-bridge/ember-bridge.tsx | 11 ++++- apps/admin/src/ember-bridge/index.ts | 1 + .../admin/src/layout/app-sidebar/nav-main.tsx | 14 ++++-- .../src/layout/sidebar.acceptance.test.tsx | 45 ++++++++++++++++++- .../post-analytics.acceptance.test.tsx | 5 +++ 6 files changed, 78 insertions(+), 9 deletions(-) diff --git a/apps/admin/src/ember-bridge/ember-bridge.test.tsx b/apps/admin/src/ember-bridge/ember-bridge.test.tsx index 9344ab1e7e1..6a10f58e39f 100644 --- a/apps/admin/src/ember-bridge/ember-bridge.test.tsx +++ b/apps/admin/src/ember-bridge/ember-bridge.test.tsx @@ -416,6 +416,7 @@ describe('useEmberRouting', () => { baseTest('returns bridge routing methods when bridge is available', () => { const mock = createMockStateBridge(); + mock.stateBridge.isRouteActive = vi.fn(() => true); window.EmberBridge = { state: mock.stateBridge }; const { result } = renderHook(() => useEmberRouting()); @@ -423,9 +424,11 @@ describe('useEmberRouting', () => { expect(result.current).toHaveProperty('getRouteUrl'); expect(result.current).toHaveProperty('isRouteActive'); - // Should be using bridge methods, not defaults + // Should be using bridge methods, not defaults. The active-state method is + // wrapped so the app can ignore stale Ember state on React-owned routes. expect(result.current.getRouteUrl).toBe(mock.stateBridge.getRouteUrl); - expect(result.current.isRouteActive).toBe(mock.stateBridge.isRouteActive); + expect(result.current.isRouteActive('posts')).toBe(true); + expect(mock.stateBridge.isRouteActive).toHaveBeenCalledWith('posts'); }); baseTest('switches to bridge methods when bridge becomes available', async () => { @@ -439,6 +442,7 @@ describe('useEmberRouting', () => { // Bridge becomes available const mock = createMockStateBridge(); + mock.stateBridge.isRouteActive = vi.fn(() => true); window.EmberBridge = { state: mock.stateBridge }; // Wait for the subscription interval to fire @@ -448,7 +452,8 @@ describe('useEmberRouting', () => { // Now should be using bridge methods expect(result.current.getRouteUrl).toBe(mock.stateBridge.getRouteUrl); - expect(result.current.isRouteActive).toBe(mock.stateBridge.isRouteActive); + expect(result.current.isRouteActive('posts')).toBe(true); + expect(mock.stateBridge.isRouteActive).toHaveBeenCalledWith('posts'); }); baseTest('re-renders when route changes', async () => { diff --git a/apps/admin/src/ember-bridge/ember-bridge.tsx b/apps/admin/src/ember-bridge/ember-bridge.tsx index 57f44ef8769..d94d8a7d02b 100644 --- a/apps/admin/src/ember-bridge/ember-bridge.tsx +++ b/apps/admin/src/ember-bridge/ember-bridge.tsx @@ -1,6 +1,7 @@ -import { useCallback, useEffect, useState, useSyncExternalStore } from 'react'; +import { useCallback, useContext, useEffect, useState, useSyncExternalStore } from 'react'; import { useQueryClient } from '@tanstack/react-query'; import { useBrowseConfig } from '@tryghost/admin-x-framework/api/config'; +import { EmberContext } from './ember-context'; export interface EmberBridge { state: StateBridge; @@ -360,6 +361,7 @@ const defaultRouting: EmberRouting = { * ``` */ export function useEmberRouting(): EmberRouting { + const emberContext = useContext(EmberContext); const [bridge, setBridge] = useState(() => window.EmberBridge?.state ?? null); const [, forceUpdate] = useState(0); @@ -385,7 +387,12 @@ export function useEmberRouting(): EmberRouting { return { getRouteUrl: bridge.getRouteUrl, - isRouteActive: bridge.isRouteActive, + // React-owned navigations use pushState, which Ember does not observe. + // Only trust Ember's route state while the current route is actually + // rendering an Ember fallback. Outside EmberProvider (mainly unit tests + // and standalone consumers), preserve the bridge's original behaviour. + isRouteActive: (...args) => + (emberContext?.isFallbackPresent ?? true) && bridge.isRouteActive(...args), }; } diff --git a/apps/admin/src/ember-bridge/index.ts b/apps/admin/src/ember-bridge/index.ts index 81d7466bd40..9a411fd7dde 100644 --- a/apps/admin/src/ember-bridge/index.ts +++ b/apps/admin/src/ember-bridge/index.ts @@ -22,4 +22,5 @@ export type { EmberDataChangeEvent, EmberRouting, OpenGiftLinkModalEvent, + StateBridge, } from './ember-bridge'; diff --git a/apps/admin/src/layout/app-sidebar/nav-main.tsx b/apps/admin/src/layout/app-sidebar/nav-main.tsx index 83ed898f883..0dc58c1f269 100644 --- a/apps/admin/src/layout/app-sidebar/nav-main.tsx +++ b/apps/admin/src/layout/app-sidebar/nav-main.tsx @@ -6,7 +6,7 @@ import { SidebarMenu, SidebarMenuBadge, } from '@tryghost/shade/components'; -import { LucideIcon } from '@tryghost/shade/utils'; +import { formatNumber, LucideIcon } from '@tryghost/shade/utils'; import { useBrowseSite } from '@tryghost/admin-x-framework/api/site'; import { useCurrentUser } from '@tryghost/admin-x-framework/api/current-user'; import { useBrowseSettings } from '@tryghost/admin-x-framework/api/settings'; @@ -34,6 +34,11 @@ function NavMain({ ...props }: React.ComponentProps) { ); const isNetworkRouteActive = useIsActiveLink({ path: 'network', activeOnSubpath: true }); const isActivitypubRouteActive = useIsActiveLink({ path: 'activitypub', activeOnSubpath: true }); + const isAnalyticsRouteActive = useIsActiveLink({ path: 'analytics', activeOnSubpath: true }); + const isPostAnalyticsRouteActive = useIsActiveLink({ + path: 'posts/analytics', + activeOnSubpath: true, + }); const showNetworkBadge = networkNotificationCount > 0 && !isNetworkRouteActive && !isActivitypubRouteActive; @@ -46,7 +51,10 @@ function NavMain({ ...props }: React.ComponentProps) { - + Analytics @@ -62,7 +70,7 @@ function NavMain({ ...props }: React.ComponentProps) { {showNetworkBadge && ( - {networkNotificationCount} + {formatNumber(networkNotificationCount)} )} diff --git a/apps/admin/src/layout/sidebar.acceptance.test.tsx b/apps/admin/src/layout/sidebar.acceptance.test.tsx index 88ad0c9082b..bdcf8a38be9 100644 --- a/apps/admin/src/layout/sidebar.acceptance.test.tsx +++ b/apps/admin/src/layout/sidebar.acceptance.test.tsx @@ -1,4 +1,5 @@ -import { describe, expect, it } from 'vitest'; +import { afterEach, describe, expect, it } from 'vitest'; +import type { StateBridge } from '@/ember-bridge'; import { activeThemeResponse, @@ -30,6 +31,29 @@ function fakeUnreadNotifications(count: number): void { fakeEndpoint('GET', UNREAD_COUNT_URL, { count }); } +function installStaleEmberRoute(activeRoute: 'members-activity' | 'pages' | 'posts'): void { + window.EmberBridge = { + state: { + onUpdate: () => {}, + onInvalidate: () => {}, + onDelete: () => {}, + isFeatureEnabled: () => false, + on: () => {}, + off: () => {}, + sidebarVisible: true, + getRouteUrl: (routeName) => routeName, + isRouteActive: (routeNames) => { + const routes = Array.isArray(routeNames) ? routeNames : routeNames.split(' '); + return routes.includes(activeRoute); + }, + } satisfies StateBridge, + }; +} + +afterEach(() => { + delete window.EmberBridge; +}); + describe('Sidebar navigation', () => { it('renders the navigation for the current user', async () => { await renderAdminApp('/site'); @@ -89,6 +113,25 @@ describe('Sidebar navigation', () => { await expect.poll(currentRoute).toBe('/pages'); }); + it.each([ + { label: 'Posts', route: '/posts', emberRoute: 'posts' }, + { label: 'Pages', route: '/pages', emberRoute: 'pages' }, + { label: 'Members', route: '/members-activity', emberRoute: 'members-activity' }, + ] as const)( + 'clears the $label active state after leaving its Ember route', + async ({ label, route, emberRoute }) => { + fakeTags([]); + installStaleEmberRoute(emberRoute); + await renderAdminApp(route); + + await expect.element(sidebarScreen.navLink(label)).toHaveAttribute('aria-current', 'page'); + + await sidebarScreen.navLink('Tags').click(); + await expect.poll(currentRoute).toBe('/tags'); + await expect.element(sidebarScreen.navLink(label)).not.toHaveAttribute('aria-current'); + }, + ); + it('shows the default post views and collapses them with the toggle', async () => { await renderAdminApp('/posts'); diff --git a/apps/admin/src/posts/analytics/post-analytics.acceptance.test.tsx b/apps/admin/src/posts/analytics/post-analytics.acceptance.test.tsx index 5853b681935..974d5fd7243 100644 --- a/apps/admin/src/posts/analytics/post-analytics.acceptance.test.tsx +++ b/apps/admin/src/posts/analytics/post-analytics.acceptance.test.tsx @@ -15,6 +15,7 @@ import { webAnalyticsBootOverrides, } from '@test-utils/acceptance'; import { membersScreen } from '@/members/members.screen'; +import { sidebarScreen } from '@/layout/sidebar.screen'; import { postAnalyticsScreen } from './post-analytics.screen'; const POST_ID = '64d623b64676110001e897d9'; @@ -133,6 +134,10 @@ describe('Post analytics overview', () => { await expect.element(postAnalyticsScreen.postTitle('Attack of the Clones')).toBeVisible(); await expect(postsApi).toHaveSentFilter(`id:${POST_ID}`); + await expect + .element(sidebarScreen.navLink('Analytics')) + .toHaveAttribute('aria-current', 'page'); + await expect.element(sidebarScreen.navLink('Posts')).not.toHaveAttribute('aria-current'); // Web performance: visitors summed from the Tinybird rows. await expect.element(postAnalyticsScreen.webPerformanceCard()).toBeVisible();