Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions apps/admin/src/layout/app-sidebar/member-sidebar-views.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ function isMemberSidebarView(view: SharedView): view is MemberSidebarView {
}

function getMemberViewUrl(filter: string) {
return `members-forward?${new URLSearchParams({filter}).toString()}`;
return `members?${new URLSearchParams({filter}).toString()}`;
}

function isMemberViewActive(currentSearch: string, filter: string) {
Expand All @@ -25,7 +25,7 @@ function isMemberViewActive(currentSearch: string, filter: string) {
export function useMemberSidebarViews() {
const location = useLocation();
const sharedViews = useSharedViews('members');
const isOnMembersForward = location.pathname === '/members-forward';
const isOnMembersListRoute = location.pathname === '/members';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Normalize members path when computing saved-view active state

The members route gate now explicitly treats /members/ as the members list, but this check only matches '/members', so a trailing-slash URL leaves all member saved views inactive even when the filter matches. This creates inconsistent sidebar state for bookmarked/manual /members/ URLs; normalizing trailing slashes here (as done in the gate) avoids that mismatch.

Useful? React with 👍 / 👎.


return useMemo<NavSavedView[]>(() => {
return sharedViews
Expand All @@ -34,7 +34,7 @@ export function useMemberSidebarViews() {
key: `${view.name}:${view.filter.filter}`,
name: view.name,
to: getMemberViewUrl(view.filter.filter),
isActive: isOnMembersForward && isMemberViewActive(location.search, view.filter.filter)
isActive: isOnMembersListRoute && isMemberViewActive(location.search, view.filter.filter)
}));
}, [isOnMembersForward, location.search, sharedViews]);
}, [isOnMembersListRoute, location.search, sharedViews]);
}
25 changes: 7 additions & 18 deletions apps/admin/src/layout/app-sidebar/nav-content.helpers.test.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,11 @@
import {describe, expect, it} from 'vitest';
import {getMembersNavActiveRoutes, isMembersNavActive} from './nav-content.helpers';

describe('getMembersNavActiveRoutes', () => {
it('always includes members-forward alongside the legacy members routes', () => {
expect(getMembersNavActiveRoutes()).toEqual([
'members-forward',
'members',
'member',
'member.new'
]);
});
});
import {isMembersNavActive} from './nav-content.helpers';

describe('isMembersNavActive', () => {
it('uses the legacy route active state when members forward is disabled', () => {
it('uses the legacy route active state when the feature flag is disabled', () => {
expect(isMembersNavActive({
membersForwardEnabled: false,
isOnMembersForward: false,
isOnMembersRoute: false,
hasActiveMemberView: false,
isMembersExpanded: false,
isLegacyMembersRouteActive: true
Expand All @@ -26,7 +15,7 @@ describe('isMembersNavActive', () => {
it('marks the base Members link active when a saved member view is active but collapsed', () => {
expect(isMembersNavActive({
membersForwardEnabled: true,
isOnMembersForward: true,
isOnMembersRoute: true,
hasActiveMemberView: true,
isMembersExpanded: false,
isLegacyMembersRouteActive: false
Expand All @@ -36,17 +25,17 @@ describe('isMembersNavActive', () => {
it('marks the base Members link inactive when a saved member view is active and expanded', () => {
expect(isMembersNavActive({
membersForwardEnabled: true,
isOnMembersForward: true,
isOnMembersRoute: true,
hasActiveMemberView: true,
isMembersExpanded: true,
isLegacyMembersRouteActive: false
})).toBe(false);
});

it('falls back to the base Members link when no saved member view is active', () => {
it('marks the base Members link active on React-owned members routes when no saved member view is active', () => {
expect(isMembersNavActive({
membersForwardEnabled: true,
isOnMembersForward: true,
isOnMembersRoute: true,
hasActiveMemberView: false,
isMembersExpanded: false,
isLegacyMembersRouteActive: false
Expand Down
11 changes: 3 additions & 8 deletions apps/admin/src/layout/app-sidebar/nav-content.helpers.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,12 @@
export function getMembersNavActiveRoutes(): string[] {
// TODO: Remove members-forward once the membersForward flag and legacy route split are gone.
return ['members-forward', 'members', 'member', 'member.new'];
}

export function isMembersNavActive({
membersForwardEnabled,
isOnMembersForward,
isOnMembersRoute,
hasActiveMemberView,
isMembersExpanded,
isLegacyMembersRouteActive
}: {
membersForwardEnabled: boolean;
isOnMembersForward: boolean;
isOnMembersRoute: boolean;
hasActiveMemberView: boolean;
isMembersExpanded: boolean;
isLegacyMembersRouteActive: boolean;
Expand All @@ -20,7 +15,7 @@ export function isMembersNavActive({
return isLegacyMembersRouteActive;
}

if (isOnMembersForward) {
if (isOnMembersRoute) {
if (!hasActiveMemberView) {
return true;
}
Expand Down
14 changes: 8 additions & 6 deletions apps/admin/src/layout/app-sidebar/nav-content.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@ import { useNavigationExpanded } from "./hooks/use-navigation-preferences";
import { NavCustomViews } from "./nav-custom-views";
import { NavMemberViews } from "./nav-member-views";
import { useMemberSidebarViews } from "./member-sidebar-views";
import { getMembersNavActiveRoutes, isMembersNavActive } from "./nav-content.helpers";
import { isMembersNavActive } from "./nav-content.helpers";
import { useCustomSidebarViews } from "./use-custom-sidebar-views";
import { useEmberRouting } from "@/ember-bridge";
import { useFeatureFlag } from "@/hooks/use-feature-flag";

const LEGACY_MEMBERS_ACTIVE_ROUTES = ['members', 'member', 'member.new'];

function PostsNavItemContent({isActive, to}: {isActive: boolean; to: string}) {
return (
<>
Expand Down Expand Up @@ -92,20 +94,20 @@ function NavContent({ ...props }: React.ComponentProps<typeof SidebarGroup>) {
const isPublishedPostsRouteActive = routing.isRouteActive('posts', {type: 'published'});
const hasActivePostChild = isDraftPostsRouteActive || isScheduledPostsRouteActive || isPublishedPostsRouteActive || postCustomViews.some(view => view.isActive);
const postsExpanded = savedPostsExpanded;
const isOnMembersForward = location.pathname === '/members-forward';
const hasActiveMemberView = isOnMembersForward && memberViews.some(view => view.isActive);
const isOnMembersRoute = location.pathname === '/members' || location.pathname === '/members/import';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Normalize members pathname before computing nav active state

The new check only treats '/members' and '/members/import' as React-owned members routes, so a valid trailing-slash URL like /members/ is treated as off-route and isMembersNavActive falls back to Ember route state, leaving the Members nav item inactive. This is a regression for bookmarked/manual URLs (and members-route-gate.test.tsx now explicitly treats /members/ as a members route), so the pathname should be normalized (for example by trimming a trailing slash) before this comparison.

Useful? React with 👍 / 👎.

const hasActiveMemberView = memberViews.some(view => view.isActive);
const membersExpanded = savedMembersExpanded;
const membersNavActive = isMembersNavActive({
membersForwardEnabled,
isOnMembersForward,
isOnMembersRoute,
hasActiveMemberView,
isMembersExpanded: membersExpanded,
isLegacyMembersRouteActive: routing.isRouteActive(getMembersNavActiveRoutes())
isLegacyMembersRouteActive: routing.isRouteActive(LEGACY_MEMBERS_ACTIVE_ROUTES)
});
const postsRoute = routing.getRouteUrl('posts');
const isPostsRouteActive = routing.isRouteActive('posts');
const postsNavActive = isPostsRouteActive || (!postsExpanded && hasActivePostChild);
const membersRoute = membersForwardEnabled ? 'members-forward' : routing.getRouteUrl('members');
const membersRoute = routing.getRouteUrl('members');

return (
<SidebarGroup {...props}>
Expand Down
54 changes: 54 additions & 0 deletions apps/admin/src/members-route-gate.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import {render, screen} from '@testing-library/react';
import React from 'react';
import {beforeEach, describe, expect, it, vi} from 'vitest';
import {MembersRouteGate} from './members-route-gate';

const {mockUseLocation, mockUseFeatureFlag} = vi.hoisted(() => ({
mockUseLocation: vi.fn(),
mockUseFeatureFlag: vi.fn()
}));

vi.mock('@tryghost/admin-x-framework', () => ({
Outlet: () => React.createElement('div', {'data-testid': 'outlet'}),
useLocation: mockUseLocation
}));

vi.mock('./ember-bridge', () => ({
EmberFallback: () => React.createElement('div', {'data-testid': 'ember-fallback'})
}));

vi.mock('./hooks/use-feature-flag', () => ({
useFeatureFlag: mockUseFeatureFlag
}));

describe('MembersRouteGate', () => {
beforeEach(() => {
mockUseFeatureFlag.mockReturnValue(false);
mockUseLocation.mockReturnValue({pathname: '/members'});
});

it('delegates /members/ to Ember when the flag is disabled', () => {
mockUseLocation.mockReturnValue({pathname: '/members/'});

render(<MembersRouteGate />);

expect(screen.getByTestId('ember-fallback')).toBeInTheDocument();
});

it('delegates /members/import to Ember when the flag is disabled', () => {
mockUseLocation.mockReturnValue({pathname: '/members/import'});

render(<MembersRouteGate />);

expect(screen.getByTestId('ember-fallback')).toBeInTheDocument();
});

it('renders React routes when the flag is enabled', () => {
mockUseFeatureFlag.mockReturnValue(true);
mockUseLocation.mockReturnValue({pathname: '/members/import'});

render(<MembersRouteGate />);

expect(screen.getByTestId('outlet')).toBeInTheDocument();
});
});
13 changes: 13 additions & 0 deletions apps/admin/src/members-route-gate.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import {Outlet} from "@tryghost/admin-x-framework";
import {EmberFallback} from "./ember-bridge";
import {useFeatureFlag} from "./hooks/use-feature-flag";

export function MembersRouteGate() {
const membersForwardEnabled = useFeatureFlag("membersForward");

if (!membersForwardEnabled) {
return <EmberFallback />;
}

return <Outlet />;
}
33 changes: 32 additions & 1 deletion apps/admin/src/routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
// Ember
import { EmberFallback, ForceUpgradeGuard } from "./ember-bridge";
import type { RouteHandle } from "./ember-bridge";
import { MembersRouteGate } from "./members-route-gate";

import { NotFound } from "./not-found";

Expand All @@ -40,7 +41,8 @@
"/tags/new",
"/explore/*",
"/migrate/*",
"/members/*",
"/members/new",
"/members/:member_id",
"/members-activity",
"/designsandbox",
"/mentions",
Expand All @@ -53,6 +55,33 @@
Component: EmberFallback,
handle: emberFallbackHandle,
}));

const membersRoute: RouteObject = {
path: "/members",
element: <MembersRouteGate />,
handle: emberFallbackHandle,
children: [
{
index: true,
lazy: lazyComponent(() => import("@tryghost/posts/src/views/members/members"))
},
{
path: "import",
lazy: lazyComponent(() => import("@tryghost/posts/src/views/members/members"))
}
]
};

const membersForwardRedirectRoute: RouteObject = {
path: "/members-forward",
// TODO: Remove once the legacy Ember members list is deleted.

Check warning on line 77 in apps/admin/src/routes.tsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Complete the task associated to this "TODO" comment.

See more on https://sonarcloud.io/project/issues?id=TryGhost_Ghost&issues=AZ1If0oNrwhSJOCgFKsF&open=AZ1If0oNrwhSJOCgFKsF&pullRequest=27040
handle: emberFallbackHandle,
loader: ({request}) => {
const url = new URL(request.url);
return redirect(`/members${url.search}`);
}
};

export const routes: RouteObject[] = [
{
// ForceUpgradeGuard wraps all routes to redirect to /pro when in force upgrade mode.
Expand All @@ -69,6 +98,8 @@
Component: EmberFallback,
handle: emberFallbackHandle,
},
membersRoute,
membersForwardRedirectRoute,
{
element: (
<PostsAppContextProvider value={{ fromAnalytics: true }}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ describe('virtual-list-window', () => {
it('restores the unlocked window from the current history entry on remount', () => {
window.history.replaceState({
ghostVirtualListWindow: {
'/members-forward::?filter=members': 2000
'/members::?filter=members': 2000
}
}, '');

const wrapper = createWrapper('/members-forward?filter=members');
const wrapper = createWrapper('/members?filter=members');
const {result, unmount} = renderHook(() => useVirtualListWindow(5000), {wrapper});

expect(result.current.visibleItemCount).toBe(2000);
Expand All @@ -44,7 +44,7 @@ describe('virtual-list-window', () => {
window.history.replaceState({}, '');

const {result} = renderHook(() => useVirtualListWindow(5000), {
wrapper: createWrapper('/members-forward?filter=members')
wrapper: createWrapper('/members?filter=members')
});

act(() => {
Expand All @@ -53,14 +53,14 @@ describe('virtual-list-window', () => {

expect(window.history.state).toMatchObject({
ghostVirtualListWindow: {
'/members-forward::?filter=members': 2000
'/members::?filter=members': 2000
}
});
});

it('caps the visible window at 1,000 rows by default', () => {
const {result} = renderHook(() => useVirtualListWindow(1500), {
wrapper: createWrapper('/members-forward?filter=members')
wrapper: createWrapper('/members?filter=members')
});

expect(result.current).toMatchObject({
Expand All @@ -71,7 +71,7 @@ describe('virtual-list-window', () => {

it('shows all items when the total is below the cap', () => {
const {result} = renderHook(() => useVirtualListWindow(125), {
wrapper: createWrapper('/members-forward?filter=members')
wrapper: createWrapper('/members?filter=members')
});

expect(result.current).toMatchObject({
Expand All @@ -82,7 +82,7 @@ describe('virtual-list-window', () => {

it('unlocks the next 1,000 rows each time load more is requested', () => {
const {result} = renderHook(() => useVirtualListWindow(5000), {
wrapper: createWrapper('/members-forward?filter=members')
wrapper: createWrapper('/members?filter=members')
});

expect(result.current.visibleItemCount).toBe(1000);
Expand All @@ -97,12 +97,12 @@ describe('virtual-list-window', () => {
it('ignores invalid persisted unlocked counts', () => {
window.history.replaceState({
ghostVirtualListWindow: {
'/members-forward::?filter=members': Number.NaN
'/members::?filter=members': Number.NaN
}
}, '');

const {result} = renderHook(() => useVirtualListWindow(5000), {
wrapper: createWrapper('/members-forward?filter=members')
wrapper: createWrapper('/members?filter=members')
});

expect(result.current.visibleItemCount).toBe(1000);
Expand Down
4 changes: 0 additions & 4 deletions apps/posts/src/routes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,6 @@ export const routes: RouteObject[] = [
path: 'comments',
lazy: lazyComponent(() => import('@views/comments/comments'))
},
{
path: 'members-forward',
lazy: lazyComponent(() => import('@views/members/members'))
},

// Error handling
{
Expand Down
Loading
Loading