From 358a5283de1b8346170f4e9a81eea0a156c322c4 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 24 Aug 2026 11:19:00 -0500 Subject: [PATCH 1/7] Changed theme state and tooltip provisioning to shared providers (#30222) no ref Two provider hoists. --- apps/admin/src/app-root.tsx | 9 +- apps/admin/src/hooks/use-theme.ts | 2 + .../src/layout/app-sidebar/user-menu.tsx | 5 +- apps/admin/src/providers/theme-context.ts | 14 ++ .../src/providers/theme-provider.test.tsx | 182 ++++++++++++++++++ apps/admin/src/providers/theme-provider.tsx | 11 ++ apps/shade/src/providers/shade-provider.tsx | 9 +- 7 files changed, 225 insertions(+), 7 deletions(-) create mode 100644 apps/admin/src/providers/theme-context.ts create mode 100644 apps/admin/src/providers/theme-provider.test.tsx create mode 100644 apps/admin/src/providers/theme-provider.tsx diff --git a/apps/admin/src/app-root.tsx b/apps/admin/src/app-root.tsx index ca8dc580f393..8f604b0e7f4c 100644 --- a/apps/admin/src/app-root.tsx +++ b/apps/admin/src/app-root.tsx @@ -8,12 +8,13 @@ import { ShadeApp } from '@tryghost/shade/app'; import App from './app.tsx'; import { routes } from './routes.tsx'; -import { useTheme } from './hooks/use-theme'; import { AppProvider } from './providers/app-provider'; +import { useThemeContext } from './providers/theme-context'; +import { ThemeProvider } from './providers/theme-provider'; import { fetchKoenigLexical } from './utils/fetch-koenig-lexical'; function ThemedAdminApp() { - const { resolvedTheme } = useTheme(); + const { resolvedTheme } = useThemeContext(); return ( - + + + diff --git a/apps/admin/src/hooks/use-theme.ts b/apps/admin/src/hooks/use-theme.ts index 8a9dfc10d85b..907e7fcee424 100644 --- a/apps/admin/src/hooks/use-theme.ts +++ b/apps/admin/src/hooks/use-theme.ts @@ -47,6 +47,8 @@ function applyAdminTheme(mode: ThemeMode, resolvedTheme: ResolvedThemeMode) { } } +// App code must consume this via ThemeProvider/useThemeContext (src/providers): +// each extra instance forks the optimistic state and re-runs the DOM effects. export function useTheme() { const { data: preferences } = useUserPreferences(); const { mutateAsync: editPreferences, isPending: isEditingPreferences } = diff --git a/apps/admin/src/layout/app-sidebar/user-menu.tsx b/apps/admin/src/layout/app-sidebar/user-menu.tsx index a13774bc4c81..9680063b48f8 100644 --- a/apps/admin/src/layout/app-sidebar/user-menu.tsx +++ b/apps/admin/src/layout/app-sidebar/user-menu.tsx @@ -17,7 +17,8 @@ import { useCurrentUser } from '@tryghost/admin-x-framework/api/current-user'; import { useDeleteSession } from '@tryghost/admin-x-framework/api/session'; import { getGhostPaths } from '@tryghost/admin-x-framework/helpers'; import { toast } from 'sonner'; -import { useTheme, type ThemeMode } from '@/hooks/use-theme'; +import { type ThemeMode } from '@/hooks/use-theme'; +import { useThemeContext } from '@/providers/theme-context'; import { useWhatsNew } from '@/whats-new/hooks/use-whats-new'; import { useUpgradeStatus } from './hooks/use-upgrade-status'; import { useBrowseSite } from '@tryghost/admin-x-framework/api/site'; @@ -51,7 +52,7 @@ const THEME_LABELS = Object.fromEntries( ) as Record; function UserMenuAppearance() { - const { theme, setTheme, isSettingTheme } = useTheme(); + const { theme, setTheme, isSettingTheme } = useThemeContext(); return ( diff --git a/apps/admin/src/providers/theme-context.ts b/apps/admin/src/providers/theme-context.ts new file mode 100644 index 000000000000..03f4d1ef01a9 --- /dev/null +++ b/apps/admin/src/providers/theme-context.ts @@ -0,0 +1,14 @@ +import { createContext, useContext } from 'react'; +import type { useTheme } from '@/hooks/use-theme'; + +export type ThemeContextValue = ReturnType; + +export const ThemeContext = createContext(undefined); + +export const useThemeContext = () => { + const context = useContext(ThemeContext); + if (context === undefined) { + throw new Error('useThemeContext must be used within a ThemeProvider'); + } + return context; +}; diff --git a/apps/admin/src/providers/theme-provider.test.tsx b/apps/admin/src/providers/theme-provider.test.tsx new file mode 100644 index 000000000000..be7525cd191f --- /dev/null +++ b/apps/admin/src/providers/theme-provider.test.tsx @@ -0,0 +1,182 @@ +import { test as baseTest, afterEach, describe, expect } from 'vitest'; +import { render, renderHook, screen, waitFor, act } from '@testing-library/react'; +import type { ReactNode } from 'react'; +import type { QueryClient } from '@tanstack/react-query'; +import { HttpResponse, http } from 'msw'; +import type { SetupServer } from 'msw/node'; +import type { + UpdateUserRequestBody, + UsersResponseType, +} from '@tryghost/admin-x-framework/api/users'; +import { mockUser } from '@test-utils/factories'; +import { serverFixture } from '@test-utils/fixtures/msw'; +import { queryClientFixtures, type TestWrapperComponent } from '@test-utils/fixtures/query-client'; +import { useUserPreferences } from '@/hooks/user-preferences'; +import { ThemeProvider } from './theme-provider'; +import { useThemeContext } from './theme-context'; + +const USERS_API_URL = '/ghost/api/admin/users/me/'; +const USER_UPDATE_API_URL = '/ghost/api/admin/users/:id/'; + +const themeContextTest = baseTest.extend<{ + server: SetupServer; + queryClient: QueryClient; + wrapper: TestWrapperComponent; +}>({ + ...serverFixture, + ...queryClientFixtures, +}); + +function mockPreferences(server: SetupServer, nightShift: string) { + server.use( + http.get(USERS_API_URL, () => { + return HttpResponse.json({ + users: [ + { + ...mockUser, + accessibility: JSON.stringify({ nightShift }), + }, + ], + }); + }), + http.put<{ id: string }, UpdateUserRequestBody, UsersResponseType>( + USER_UPDATE_API_URL, + async ({ request }) => { + const body = await request.json(); + return HttpResponse.json({ + users: [ + { + ...mockUser, + accessibility: body.users[0]?.accessibility ?? '', + }, + ], + }); + }, + ), + ); +} + +afterEach(() => { + document.documentElement.classList.remove('dark', 'theme-switching'); +}); + +describe('ThemeProvider', () => { + themeContextTest( + 'shares one optimistic theme state across separate consumers mid-save', + async ({ server, wrapper: Wrapper }) => { + mockPreferences(server, 'light'); + + // Hold the persistence PUT open so the optimistic window is observable. + let releasePut: () => void = () => {}; + const putGate = new Promise((resolve) => { + releasePut = resolve; + }); + server.use( + http.put<{ id: string }, UpdateUserRequestBody, UsersResponseType>( + USER_UPDATE_API_URL, + async ({ request }) => { + const body = await request.json(); + await putGate; + return HttpResponse.json({ + users: [ + { + ...mockUser, + accessibility: body.users[0]?.accessibility ?? '', + }, + ], + }); + }, + ), + ); + + // Two SEPARATE components: `Shell` mirrors ThemedAdminApp, `Menu` + // mirrors the appearance menu. The pre-provider architecture gave each + // its own useTheme instance with independent optimistic state; this + // asserts they now share one. + function Shell() { + const { resolvedTheme } = useThemeContext(); + return
{resolvedTheme}
; + } + let setThemeFromMenu: (mode: 'dark' | 'light' | 'system') => Promise = async () => {}; + function Menu() { + const { setTheme, isSettingTheme } = useThemeContext(); + setThemeFromMenu = setTheme; + return
{String(isSettingTheme)}
; + } + + render( + + + + + + , + ); + + await waitFor(() => { + expect(screen.getByTestId('shell-theme').textContent).toBe('light'); + }); + + let pendingSet: Promise; + act(() => { + pendingSet = setThemeFromMenu('dark'); + }); + + // Mid-flight (PUT still held open): the shell consumer already shows the + // menu's optimistic selection, and the shared saving flag is visible to + // the menu. The old two-instance architecture fails here: the shell's + // instance knew nothing about the menu's pendingTheme. + await waitFor(() => { + expect(screen.getByTestId('shell-theme').textContent).toBe('dark'); + }); + expect(screen.getByTestId('menu-saving').textContent).toBe('true'); + + releasePut(); + await act(async () => { + await pendingSet; + }); + + expect(screen.getByTestId('shell-theme').textContent).toBe('dark'); + await waitFor(() => { + expect(screen.getByTestId('menu-saving').textContent).toBe('false'); + }); + }, + ); + + themeContextTest( + 'shares one theme state: a menu setTheme flips every consumer', + async ({ server, wrapper: Wrapper }) => { + mockPreferences(server, 'light'); + + const contextWrapper = ({ children }: { children: ReactNode }) => ( + + {children} + + ); + + // Two consumers of the shared provider: `shell` mirrors ThemedAdminApp, + // `menu` mirrors the appearance menu in the user menu. + const { result } = renderHook( + () => ({ + shell: useThemeContext(), + menu: useThemeContext(), + preferences: useUserPreferences(), + }), + { wrapper: contextWrapper }, + ); + + // Wait for preferences to load so setTheme can persist the change + await waitFor(() => { + expect(result.current.preferences.data).toBeDefined(); + }); + expect(result.current.shell.resolvedTheme).toBe('light'); + + await act(async () => { + await result.current.menu.setTheme('dark'); + }); + + expect(result.current.shell.theme).toBe('dark'); + expect(result.current.shell.resolvedTheme).toBe('dark'); + }, + ); +}); diff --git a/apps/admin/src/providers/theme-provider.tsx b/apps/admin/src/providers/theme-provider.tsx new file mode 100644 index 000000000000..0162c3e95be4 --- /dev/null +++ b/apps/admin/src/providers/theme-provider.tsx @@ -0,0 +1,11 @@ +import type { ReactNode } from 'react'; +import { ThemeContext } from '@/providers/theme-context'; +import { useTheme } from '@/hooks/use-theme'; + +// Instantiates useTheme exactly once so every consumer shares the same +// optimistic theme state and a single set of bridge/DOM effects. +export function ThemeProvider({ children }: { children: ReactNode }) { + const theme = useTheme(); + + return {children}; +} diff --git a/apps/shade/src/providers/shade-provider.tsx b/apps/shade/src/providers/shade-provider.tsx index ac2034708b41..a5412c68d12c 100644 --- a/apps/shade/src/providers/shade-provider.tsx +++ b/apps/shade/src/providers/shade-provider.tsx @@ -3,6 +3,7 @@ import { Toaster } from '../components/ui/sonner'; import { createPortal } from 'react-dom'; import { GlobalDirtyStateProvider } from '../hooks/use-global-dirty-state'; import Icon from '../components/ui/icon'; +import { TooltipProvider } from '@/components/ui/tooltip'; import { SHADE_APP_NAMESPACES } from '@/shade-app'; export type FetchKoenigLexical = () => Promise; @@ -91,8 +92,12 @@ const ShadeProvider: React.FC = ({ value={{ isAnyTextFieldFocused, setFocusState, fetchKoenigLexical, darkMode }} > - {children} - + {/* Default Radix tooltip timing for any Tooltip without a nearer + provider; inner providers still win via nearest-provider scoping. */} + + {children} + + ); From a93e765b8311db6f95b882fa1f9f4ab41a4a6a73 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 24 Aug 2026 11:34:24 -0500 Subject: [PATCH 2/7] Removed duplicated admin helpers (#30223) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no ref Five small duplications/oddities: - **Newsletter reorder**: the optimistic-reorder block (mirrored local state, active/archived derivation, `setQueriesData` + sequential `editNewsletter`) was duplicated verbatim across `newsletters.tsx` and `newsletters-tab-content.tsx` (both live, reached from different settings surfaces). Extracted into `use-newsletter-reorder.ts`, used by both. - **Front-end preview fetches**: the theme and announcement-bar previews hand-rolled byte-identical POSTs (`x-ghost-preview`, cors, credentials); one `fetch-frontend-preview.ts` helper now owns it — deliberately a plain fetch, it targets the site front-end, not the Admin API. - **`use-whats-new`**: a `useQuery` whose queryFn was a synchronous boolean over already-loaded data → `useMemo`; the synthetic query key is gone. - **Email validation**: the two hand-rolled regexes (member edit, import mapping) → `validator.isEmail`. Looseness audited: every valid fixture passes both, every invalid fixture fails both; nothing relied on the looser match — and the server's member validation is validator-based, so the client now matches the server exactly. - **`@xyflow/react` CSS**: imported once, in the only module that mounts `` (`automation-canvas.tsx`); verified the styles still land in the lazy editor chunk in a production build. --- .../components/canvas/controls.tsx | 1 - .../automations/components/canvas/nodes.tsx | 1 - .../components/canvas/step-sidebar.tsx | 1 - .../app-sidebar/hooks/use-whats-new-status.ts | 4 +- .../src/layout/app-sidebar/user-menu.tsx | 6 +- .../import-members/mapping.ts | 5 +- .../members/detail/member-detail-edit.test.ts | 12 +++ .../src/members/detail/member-detail-edit.ts | 28 +++++-- .../src/members/detail/member-detail.tsx | 6 +- apps/admin/src/settings/email/newsletters.tsx | 77 +---------------- .../newsletters/newsletters-tab-content.tsx | 78 +---------------- .../settings/email/use-newsletter-reorder.ts | 83 +++++++++++++++++++ .../announcement-bar-preview.tsx | 23 ++--- .../design-and-branding/theme-preview.tsx | 62 ++++++-------- .../settings/utils/fetch-frontend-preview.ts | 18 ++++ .../whats-new/components/whats-new-banner.tsx | 4 +- .../whats-new/hooks/use-whats-new.test.tsx | 68 +++++++++------ .../src/whats-new/hooks/use-whats-new.ts | 51 +++--------- 18 files changed, 236 insertions(+), 292 deletions(-) create mode 100644 apps/admin/src/settings/email/use-newsletter-reorder.ts create mode 100644 apps/admin/src/settings/utils/fetch-frontend-preview.ts diff --git a/apps/admin/src/automations/components/canvas/controls.tsx b/apps/admin/src/automations/components/canvas/controls.tsx index 4d6587564d69..f5464dda4b66 100644 --- a/apps/admin/src/automations/components/canvas/controls.tsx +++ b/apps/admin/src/automations/components/canvas/controls.tsx @@ -1,4 +1,3 @@ -import '@xyflow/react/dist/style.css'; import React, { useState } from 'react'; import { Button, diff --git a/apps/admin/src/automations/components/canvas/nodes.tsx b/apps/admin/src/automations/components/canvas/nodes.tsx index b8a7c097ee38..ca9dfd40b975 100644 --- a/apps/admin/src/automations/components/canvas/nodes.tsx +++ b/apps/admin/src/automations/components/canvas/nodes.tsx @@ -1,4 +1,3 @@ -import '@xyflow/react/dist/style.css'; import React, { useRef, useState } from 'react'; import StepPicker, { type StepPickerType } from './step-picker'; import { useEmailTrackingSettings } from '@/automations/hooks/use-email-tracking-settings'; diff --git a/apps/admin/src/automations/components/canvas/step-sidebar.tsx b/apps/admin/src/automations/components/canvas/step-sidebar.tsx index 3216a140ba40..dc131e520076 100644 --- a/apps/admin/src/automations/components/canvas/step-sidebar.tsx +++ b/apps/admin/src/automations/components/canvas/step-sidebar.tsx @@ -1,4 +1,3 @@ -import '@xyflow/react/dist/style.css'; import React, { useEffect, useRef, useState } from 'react'; import type { AutomationDetail, diff --git a/apps/admin/src/layout/app-sidebar/hooks/use-whats-new-status.ts b/apps/admin/src/layout/app-sidebar/hooks/use-whats-new-status.ts index edb1b3928083..ac41731a68d1 100644 --- a/apps/admin/src/layout/app-sidebar/hooks/use-whats-new-status.ts +++ b/apps/admin/src/layout/app-sidebar/hooks/use-whats-new-status.ts @@ -6,11 +6,11 @@ export interface WhatsNewStatus { } export function useWhatsNewStatus(): WhatsNewStatus { - const { data: whatsNewData } = useWhatsNew(); + const { hasNew } = useWhatsNew(); const { data: changelog } = useChangelog(); const latestEntry = changelog?.entries[0]; return { - showWhatsNewBanner: !!whatsNewData?.hasNew && !!latestEntry, + showWhatsNewBanner: hasNew && !!latestEntry, }; } diff --git a/apps/admin/src/layout/app-sidebar/user-menu.tsx b/apps/admin/src/layout/app-sidebar/user-menu.tsx index 9680063b48f8..c97ca60e6ed6 100644 --- a/apps/admin/src/layout/app-sidebar/user-menu.tsx +++ b/apps/admin/src/layout/app-sidebar/user-menu.tsx @@ -119,7 +119,7 @@ interface UserMenuProps extends React.ComponentProps { } function UserMenu(props: UserMenuProps) { const currentUser = useCurrentUser(); - const { data: whatsNewData } = useWhatsNew(); + const { hasNew } = useWhatsNew(); const { showUpgradeBanner } = useUpgradeStatus(); return ( @@ -132,7 +132,7 @@ function UserMenu(props: UserMenuProps) { >
- {whatsNewData?.hasNew && ( + {hasNew && ( What’s new? - {whatsNewData?.hasNew && ( + {hasNew && (
; @@ -220,7 +219,7 @@ export function detectFieldTypes( const entry = sampledData[i]; for (const [key, value] of Object.entries(entry)) { - if (!mapping.email && value && EMAIL_REGEX.test(value) && !isCustomFieldColumn(key)) { + if (!mapping.email && value && validator.isEmail(value) && !isCustomFieldColumn(key)) { mapping.email = key; } } diff --git a/apps/admin/src/members/detail/member-detail-edit.test.ts b/apps/admin/src/members/detail/member-detail-edit.test.ts index 230ad8903f20..5cdc2279f040 100644 --- a/apps/admin/src/members/detail/member-detail-edit.test.ts +++ b/apps/admin/src/members/detail/member-detail-edit.test.ts @@ -297,6 +297,18 @@ describe('isValidMemberEmail', () => { expect(isValidMemberEmail('a@b.')).toBe(false); expect(isValidMemberEmail('a b@example.com')).toBe(false); }); + + it('grandfathers a stored email that no longer passes validation when unchanged', () => { + expect(isValidMemberEmail('legacy@mail_host.com', 'legacy@mail_host.com')).toBe(true); + // Trim-insensitive: the draft field may carry surrounding whitespace + expect(isValidMemberEmail(' legacy@mail_host.com ', 'legacy@mail_host.com')).toBe(true); + // Changing away from the stored value re-applies strict validation + expect(isValidMemberEmail('other@mail_host.com', 'legacy@mail_host.com')).toBe(false); + // A stored email is no excuse for a different invalid value + expect(isValidMemberEmail('nope', 'legacy@mail_host.com')).toBe(false); + // Create mode (no stored email) stays strict + expect(isValidMemberEmail('legacy@mail_host.com')).toBe(false); + }); }); describe('getDefaultNewsletterIdsForNewMember', () => { diff --git a/apps/admin/src/members/detail/member-detail-edit.ts b/apps/admin/src/members/detail/member-detail-edit.ts index 03fb29a7b9d4..684aeebf9ba3 100644 --- a/apps/admin/src/members/detail/member-detail-edit.ts +++ b/apps/admin/src/members/detail/member-detail-edit.ts @@ -1,4 +1,5 @@ import moment from 'moment-timezone'; +import validator from 'validator'; import { MEMBER_CUSTOM_FIELD_TYPES, memberCustomFieldParts, @@ -46,9 +47,6 @@ interface MemberFieldSource { newsletters?: Array<{ id: string }> | null; } -// Same shape as the import-members validator already used in this app. -const MEMBER_EMAIL_REGEX = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; - // Soft limit shown as a countdown (Ember imposes no hard maxlength; the DB column // allows 2000). The counter may go negative, matching the Ember behaviour. export const NOTE_MAX_LENGTH = 500; @@ -143,9 +141,19 @@ export function toggleMemberNewsletter(subscribedIds: string[], newsletterId: st return [...subscribedIds, newsletterId].sort(); } -/** Client-side email sanity check for the save gate; the server remains authoritative. */ -export function isValidMemberEmail(email: string): boolean { - return MEMBER_EMAIL_REGEX.test(email.trim()); +/** + * Client-side email sanity check for the save gate; the server remains + * authoritative. Mirrors the server's update semantics: an email is validated + * only when it differs from the stored one, because the server deliberately + * grandfathers stored emails that predate stricter validation + * (member-repository.js validates the email only when it changed). + */ +export function isValidMemberEmail(email: string, storedEmail?: string): boolean { + const trimmed = email.trim(); + if (storedEmail !== undefined && trimmed === storedEmail.trim()) { + return true; + } + return validator.isEmail(trimmed); } /** @@ -155,14 +163,18 @@ export function isValidMemberEmail(email: string): boolean { * validator runs on save-attempt for the same reason * (`ghost/admin/app/validators/member.js:15`). */ -export function getEmailErrorMessage(email: string, touched: boolean): string | null { +export function getEmailErrorMessage( + email: string, + touched: boolean, + storedEmail?: string, +): string | null { if (!touched) { return null; } if (email.trim() === '') { return 'Email is required.'; } - if (!isValidMemberEmail(email)) { + if (!isValidMemberEmail(email, storedEmail)) { return 'Invalid email.'; } return null; diff --git a/apps/admin/src/members/detail/member-detail.tsx b/apps/admin/src/members/detail/member-detail.tsx index 5f57aa6a4837..ea3f1b952d40 100644 --- a/apps/admin/src/members/detail/member-detail.tsx +++ b/apps/admin/src/members/detail/member-detail.tsx @@ -201,7 +201,7 @@ const MemberDetailPage: React.FC = ({ when: hasUnsavedChanges, confirmUnloadWhen: activeMutation.isPending || hasUnsavedChanges, }); - const emailValid = !!draft && isValidMemberEmail(draft.email); + const emailValid = !!draft && isValidMemberEmail(draft.email, member?.email ?? undefined); // `touched` is set on the email field's first blur. That keeps the New // member screen from painting an "Email is required." error before the // user has done anything, matching Ember's save-time-only validator @@ -211,7 +211,9 @@ const MemberDetailPage: React.FC = ({ React.useEffect(() => { setEmailTouched(false); }, [member?.id, isCreating]); - const emailError = draft ? getEmailErrorMessage(draft.email, emailTouched) : null; + const emailError = draft + ? getEmailErrorMessage(draft.email, emailTouched, member?.email ?? undefined) + : null; // The sidebar's identity block (avatar + heading) reads from a "committed" // copy of name/email that only advances on blur, not per keystroke. diff --git a/apps/admin/src/settings/email/newsletters.tsx b/apps/admin/src/settings/email/newsletters.tsx index 7dc7603f0487..b3a4ea3b8e4e 100644 --- a/apps/admin/src/settings/email/newsletters.tsx +++ b/apps/admin/src/settings/email/newsletters.tsx @@ -4,17 +4,12 @@ import TopLevelGroup from '@/settings/components/top-level-group'; import useQueryParams from '@/settings/hooks/use-query-params'; import { APIError } from '@tryghost/admin-x-framework/errors'; import { Button, Tabs, TabsContent, TabsList, TabsTrigger } from '@tryghost/shade/components'; -import { type InfiniteData, useQueryClient } from '@tryghost/admin-x-framework'; import { - type Newsletter, - type NewslettersResponseType, - newslettersDataType, useBrowseNewsletters, - useEditNewsletter, useVerifyNewsletterEmail, } from '@tryghost/admin-x-framework/api/newsletters'; -import { arrayMove } from '@dnd-kit/sortable'; import { formatNumber } from '@tryghost/shade/utils'; +import { useNewsletterReorder } from './use-newsletter-reorder'; import { type ConfirmationHandle, useConfirmation, @@ -60,19 +55,14 @@ const Newsletters: React.FC<{ keywords: string[] }> = ({ keywords }) => { isLoading, fetchNextPage, } = useBrowseNewsletters(); - const { mutateAsync: editNewsletter } = useEditNewsletter(); - const queryClient = useQueryClient(); const verifyEmailToken = useQueryParams().getParam('verifyEmail'); const { mutateAsync: verifyEmail } = useVerifyNewsletterEmail(); const handleError = useHandleError(); const { confirm } = useConfirmation(); - const [newsletters, setNewsletters] = useState(apiNewsletters || []); - - useEffect(() => { - setNewsletters(apiNewsletters || []); - }, [apiNewsletters]); + const { newsletters, sortedActiveNewsletters, archivedNewsletters, onSort } = + useNewsletterReorder(apiNewsletters); useEffect(() => { if (!verifyEmailToken || !window.location.href.includes('newsletters')) { @@ -166,67 +156,6 @@ const Newsletters: React.FC<{ keywords: string[] }> = ({ keywords }) => { ); - const sortedActiveNewsletters = - newsletters.filter((n) => n.status === 'active').sort((a, b) => a.sort_order - b.sort_order) || - []; - const archivedNewsletters = newsletters.filter((newsletter) => newsletter.status !== 'active'); - - const onSort = async (id: string, overId?: string) => { - const fromIndex = sortedActiveNewsletters.findIndex((newsletter) => newsletter.id === id); - const toIndex = - sortedActiveNewsletters.findIndex((newsletter) => newsletter.id === overId) || 0; - const newSortOrder = arrayMove(sortedActiveNewsletters, fromIndex, toIndex); - - const updatedActiveNewsletters = newSortOrder - .map((newsletter, index) => - newsletter.sort_order === index ? null : { ...newsletter, sort_order: index }, - ) - .filter((newsletter): newsletter is Newsletter => !!newsletter); - - const updatedArchivedNewsletters = archivedNewsletters - .map((newsletter, index) => - newsletter.sort_order === index + sortedActiveNewsletters.length - ? null - : { ...newsletter, sort_order: index }, - ) - .filter((newsletter): newsletter is Newsletter => !!newsletter); - - const orderUpdatedNewsletters = [ - ...updatedActiveNewsletters, - ...updatedArchivedNewsletters, - ].sort((a, b) => a.sort_order - b.sort_order); - - // Set the new order in local state and cache first so that the UI updates immediately - setNewsletters( - newsletters.map( - (newsletter) => orderUpdatedNewsletters.find((n) => n.id === newsletter.id) || newsletter, - ), - ); - queryClient.setQueriesData>( - { queryKey: [newslettersDataType] }, - (currentData) => { - if (!currentData) { - return; - } - - return { - ...currentData, - pages: currentData.pages.map((page) => ({ - ...page, - newsletters: page.newsletters.map( - (newsletter) => - orderUpdatedNewsletters.find((n) => n.id === newsletter.id) || newsletter, - ), - })), - }; - }, - ); - - for (const newsletter of orderUpdatedNewsletters) { - await editNewsletter(newsletter); - } - }; - return ( = ({ filter }) isLoading, fetchNextPage, } = useBrowseNewsletters(); - const { mutateAsync: editNewsletter } = useEditNewsletter(); - const queryClient = useQueryClient(); const verifyEmailToken = useQueryParams().getParam('verifyEmail'); const { mutateAsync: verifyEmail } = useVerifyNewsletterEmail(); const handleError = useHandleError(); const { confirm } = useConfirmation(); - const [newsletters, setNewsletters] = useState(apiNewsletters || []); - - useEffect(() => { - setNewsletters(apiNewsletters || []); - }, [apiNewsletters]); + const { newsletters, sortedActiveNewsletters, archivedNewsletters, onSort } = + useNewsletterReorder(apiNewsletters); useEffect(() => { if (!verifyEmailToken || !isNewsletterVerificationRoute()) { @@ -159,66 +149,6 @@ const NewslettersTabContent: React.FC = ({ filter }) void verify(); }, [verifyEmailToken, handleError, verifyEmail, confirm]); - const sortedActiveNewsletters = - newsletters.filter((n) => n.status === 'active').sort((a, b) => a.sort_order - b.sort_order) || - []; - const archivedNewsletters = newsletters.filter((newsletter) => newsletter.status !== 'active'); - - const onSort = async (id: string, overId?: string) => { - const fromIndex = sortedActiveNewsletters.findIndex((newsletter) => newsletter.id === id); - const toIndex = - sortedActiveNewsletters.findIndex((newsletter) => newsletter.id === overId) || 0; - const newSortOrder = arrayMove(sortedActiveNewsletters, fromIndex, toIndex); - - const updatedActiveNewsletters = newSortOrder - .map((newsletter, index) => - newsletter.sort_order === index ? null : { ...newsletter, sort_order: index }, - ) - .filter((newsletter): newsletter is Newsletter => !!newsletter); - - const updatedArchivedNewsletters = archivedNewsletters - .map((newsletter, index) => - newsletter.sort_order === index + sortedActiveNewsletters.length - ? null - : { ...newsletter, sort_order: index }, - ) - .filter((newsletter): newsletter is Newsletter => !!newsletter); - - const orderUpdatedNewsletters = [ - ...updatedActiveNewsletters, - ...updatedArchivedNewsletters, - ].sort((a, b) => a.sort_order - b.sort_order); - - setNewsletters( - newsletters.map( - (newsletter) => orderUpdatedNewsletters.find((n) => n.id === newsletter.id) || newsletter, - ), - ); - queryClient.setQueriesData>( - { queryKey: [newslettersDataType] }, - (currentData) => { - if (!currentData) { - return; - } - - return { - ...currentData, - pages: currentData.pages.map((page) => ({ - ...page, - newsletters: page.newsletters.map( - (newsletter) => - orderUpdatedNewsletters.find((n) => n.id === newsletter.id) || newsletter, - ), - })), - }; - }, - ); - - for (const newsletter of orderUpdatedNewsletters) { - await editNewsletter(newsletter); - } - }; - const showingActive = filter === 'active'; return ( diff --git a/apps/admin/src/settings/email/use-newsletter-reorder.ts b/apps/admin/src/settings/email/use-newsletter-reorder.ts new file mode 100644 index 000000000000..e1e6d0588ae0 --- /dev/null +++ b/apps/admin/src/settings/email/use-newsletter-reorder.ts @@ -0,0 +1,83 @@ +import { type InfiniteData, useQueryClient } from '@tryghost/admin-x-framework'; +import { + type Newsletter, + type NewslettersResponseType, + newslettersDataType, + useEditNewsletter, +} from '@tryghost/admin-x-framework/api/newsletters'; +import { arrayMove } from '@dnd-kit/sortable'; +import { useEffect, useState } from 'react'; + +export const useNewsletterReorder = (apiNewsletters: Newsletter[] | undefined) => { + const { mutateAsync: editNewsletter } = useEditNewsletter(); + const queryClient = useQueryClient(); + + const [newsletters, setNewsletters] = useState(apiNewsletters || []); + + useEffect(() => { + setNewsletters(apiNewsletters || []); + }, [apiNewsletters]); + + const sortedActiveNewsletters = + newsletters.filter((n) => n.status === 'active').sort((a, b) => a.sort_order - b.sort_order) || + []; + const archivedNewsletters = newsletters.filter((newsletter) => newsletter.status !== 'active'); + + const onSort = async (id: string, overId?: string) => { + const fromIndex = sortedActiveNewsletters.findIndex((newsletter) => newsletter.id === id); + const toIndex = + sortedActiveNewsletters.findIndex((newsletter) => newsletter.id === overId) || 0; + const newSortOrder = arrayMove(sortedActiveNewsletters, fromIndex, toIndex); + + const updatedActiveNewsletters = newSortOrder + .map((newsletter, index) => + newsletter.sort_order === index ? null : { ...newsletter, sort_order: index }, + ) + .filter((newsletter): newsletter is Newsletter => !!newsletter); + + const updatedArchivedNewsletters = archivedNewsletters + .map((newsletter, index) => + newsletter.sort_order === index + sortedActiveNewsletters.length + ? null + : { ...newsletter, sort_order: index }, + ) + .filter((newsletter): newsletter is Newsletter => !!newsletter); + + const orderUpdatedNewsletters = [ + ...updatedActiveNewsletters, + ...updatedArchivedNewsletters, + ].sort((a, b) => a.sort_order - b.sort_order); + + // Set the new order in local state and cache first so that the UI updates immediately + setNewsletters( + newsletters.map( + (newsletter) => orderUpdatedNewsletters.find((n) => n.id === newsletter.id) || newsletter, + ), + ); + queryClient.setQueriesData>( + { queryKey: [newslettersDataType] }, + (currentData) => { + if (!currentData) { + return; + } + + return { + ...currentData, + pages: currentData.pages.map((page) => ({ + ...page, + newsletters: page.newsletters.map( + (newsletter) => + orderUpdatedNewsletters.find((n) => n.id === newsletter.id) || newsletter, + ), + })), + }; + }, + ); + + for (const newsletter of orderUpdatedNewsletters) { + await editNewsletter(newsletter); + } + }; + + return { newsletters, sortedActiveNewsletters, archivedNewsletters, onSort }; +}; diff --git a/apps/admin/src/settings/site/announcement-bar/announcement-bar-preview.tsx b/apps/admin/src/settings/site/announcement-bar/announcement-bar-preview.tsx index 2528323ee902..5fb3ddf702d4 100644 --- a/apps/admin/src/settings/site/announcement-bar/announcement-bar-preview.tsx +++ b/apps/admin/src/settings/site/announcement-bar/announcement-bar-preview.tsx @@ -1,5 +1,6 @@ import IframeBuffering from '@/settings/utils/iframe-buffering'; import React, { useCallback, useMemo } from 'react'; +import { fetchFrontendPreview } from '@/settings/utils/fetch-frontend-preview'; const getPreviewData = ( announcementBackgroundColor?: string, @@ -37,24 +38,10 @@ const AnnouncementBarPreview: React.FC = ({ return; } - const previewUrl = new URL(url); - previewUrl.searchParams.set('admin_toolbar', '0'); - - fetch(previewUrl.toString(), { - method: 'POST', - headers: { - 'Content-Type': 'text/html;charset=utf-8', - 'x-ghost-preview': getPreviewData( - announcementBackgroundColor, - announcementContent, - visibilityMemo, - ), - Accept: 'text/html', - }, - mode: 'cors', - credentials: 'include', - }) - .then((response) => response.text()) + fetchFrontendPreview( + url, + getPreviewData(announcementBackgroundColor, announcementContent, visibilityMemo), + ) .then((data) => { // inject extra CSS to disable navigation and prevent clicks const injectedCss = `html { pointer-events: none; }`; diff --git a/apps/admin/src/settings/site/design-and-branding/theme-preview.tsx b/apps/admin/src/settings/site/design-and-branding/theme-preview.tsx index 2f045b5bb0a3..008b2654276d 100644 --- a/apps/admin/src/settings/site/design-and-branding/theme-preview.tsx +++ b/apps/admin/src/settings/site/design-and-branding/theme-preview.tsx @@ -4,6 +4,7 @@ import { type CustomThemeSetting, hiddenCustomThemeSettingValue, } from '@tryghost/admin-x-framework/api/custom-theme-settings'; +import { fetchFrontendPreview } from '@/settings/utils/fetch-frontend-preview'; import { isCustomThemeSettingVisible } from '@/settings/utils/is-custom-theme-settings-visible'; type GlobalSettings = { @@ -80,48 +81,33 @@ const ThemePreview: React.FC = ({ settings, url }) => { return; } - // Fetch theme preview HTML (suppress admin toolbar in preview) - const previewUrl = new URL(url); - previewUrl.searchParams.set('admin_toolbar', '0'); + void fetchFrontendPreview(url, previewData).then((data) => { + // inject extra CSS to disable navigation and prevent clicks + const injectedCss = `html { pointer-events: none; }`; - void fetch(previewUrl.toString(), { - method: 'POST', - headers: { - 'Content-Type': 'text/html;charset=utf-8', - 'x-ghost-preview': previewData, - Accept: 'text/html', - }, - mode: 'cors', - credentials: 'include', - }) - .then((response) => response.text()) - .then((data) => { - // inject extra CSS to disable navigation and prevent clicks - const injectedCss = `html { pointer-events: none; }`; + const domParser = new DOMParser(); + const htmlDoc = domParser.parseFromString(data, 'text/html'); - const domParser = new DOMParser(); - const htmlDoc = domParser.parseFromString(data, 'text/html'); + const stylesheet = htmlDoc.querySelector('style') as HTMLStyleElement; + const originalCSS = stylesheet?.innerHTML; + if (originalCSS) { + stylesheet.innerHTML = `${originalCSS}\n\n${injectedCss}`; + } else { + htmlDoc.head.innerHTML += ``; + } - const stylesheet = htmlDoc.querySelector('style') as HTMLStyleElement; - const originalCSS = stylesheet?.innerHTML; - if (originalCSS) { - stylesheet.innerHTML = `${originalCSS}\n\n${injectedCss}`; - } else { - htmlDoc.head.innerHTML += ``; - } + // replace the iframe contents with the doctored preview html + const doctype = htmlDoc.doctype + ? new XMLSerializer().serializeToString(htmlDoc.doctype) + : ''; + const finalDoc = doctype + htmlDoc.documentElement.outerHTML; - // replace the iframe contents with the doctored preview html - const doctype = htmlDoc.doctype - ? new XMLSerializer().serializeToString(htmlDoc.doctype) - : ''; - const finalDoc = doctype + htmlDoc.documentElement.outerHTML; - - // Send the data to the iframe's window using postMessage - // Inject the received content into the iframe - iframe.contentDocument?.open(); - iframe.contentDocument?.write(finalDoc); - iframe.contentDocument?.close(); - }); + // Send the data to the iframe's window using postMessage + // Inject the received content into the iframe + iframe.contentDocument?.open(); + iframe.contentDocument?.write(finalDoc); + iframe.contentDocument?.close(); + }); }, [previewData, url], ); diff --git a/apps/admin/src/settings/utils/fetch-frontend-preview.ts b/apps/admin/src/settings/utils/fetch-frontend-preview.ts new file mode 100644 index 000000000000..756313032eb7 --- /dev/null +++ b/apps/admin/src/settings/utils/fetch-frontend-preview.ts @@ -0,0 +1,18 @@ +// Fetches a site front-end page rendered with the given `x-ghost-preview` data. +// This targets the front-end, not the Admin API, so it stays a plain fetch. +export function fetchFrontendPreview(url: string, previewData: string): Promise { + // Suppress the admin toolbar in previews + const previewUrl = new URL(url); + previewUrl.searchParams.set('admin_toolbar', '0'); + + return fetch(previewUrl.toString(), { + method: 'POST', + headers: { + 'Content-Type': 'text/html;charset=utf-8', + 'x-ghost-preview': previewData, + Accept: 'text/html', + }, + mode: 'cors', + credentials: 'include', + }).then((response) => response.text()); +} diff --git a/apps/admin/src/whats-new/components/whats-new-banner.tsx b/apps/admin/src/whats-new/components/whats-new-banner.tsx index fc3baa4fae56..5a55d416e917 100644 --- a/apps/admin/src/whats-new/components/whats-new-banner.tsx +++ b/apps/admin/src/whats-new/components/whats-new-banner.tsx @@ -5,13 +5,13 @@ import { useWhatsNew, useDismissWhatsNew } from '@/whats-new/hooks/use-whats-new import { useChangelog } from '@/whats-new/hooks/use-changelog'; function WhatsNewBanner() { - const { data: whatsNewData } = useWhatsNew(); + const { hasNew } = useWhatsNew(); const { data: changelog } = useChangelog(); const { mutate: dismissWhatsNew } = useDismissWhatsNew(); const [isDismissed, setIsDismissed] = useState(false); // Don't show if dismissed or no new content - if (isDismissed || !whatsNewData?.hasNew) { + if (isDismissed || !hasNew) { return null; } diff --git a/apps/admin/src/whats-new/hooks/use-whats-new.test.tsx b/apps/admin/src/whats-new/hooks/use-whats-new.test.tsx index f628a7b21abc..e90bc35a0b3f 100644 --- a/apps/admin/src/whats-new/hooks/use-whats-new.test.tsx +++ b/apps/admin/src/whats-new/hooks/use-whats-new.test.tsx @@ -4,7 +4,6 @@ import type { QueryClient } from '@tanstack/react-query'; import { useWhatsNew, useDismissWhatsNew } from './use-whats-new'; import { HttpResponse, http } from 'msw'; import { mockUser, createRawChangelogEntry } from '@test-utils/factories'; -import { waitForQuerySettled } from '@test-utils/test-helpers'; import type { UpdateUserRequestBody, UsersResponseType, @@ -66,6 +65,25 @@ const fixtures = { }; // Setup functions + +/** + * `useWhatsNew` derives its value from the user-preferences and changelog + * queries (plus the preference-initializing mutation), so "settled" means + * all cached queries have resolved and no mutation is in flight. + */ +async function waitForBackingQueriesSettled(queryClient: QueryClient) { + await waitFor(() => { + const queries = queryClient.getQueryCache().getAll(); + expect(queries.length).toBeGreaterThan(0); + expect( + queries.every( + (query) => query.state.status !== 'pending' && query.state.fetchStatus === 'idle', + ), + ).toBe(true); + expect(queryClient.isMutating()).toBe(0); + }); +} + /** * Setup function for testing `useWhatsNew`. * @@ -74,23 +92,21 @@ const fixtures = { * 2. Mocking the user mutation endpoint (for initializing preferences) * 3. Mocking the changelog API endpoint with customizable changelog data * 4. Rendering the hook with the necessary React Query wrapper - * 5. Waiting for the query to settle (success or error state) - * - * Note: The query is disabled until preferences exist (hasWhatsNewPreferences = true). - * This means the query naturally stays in loading state during initialization, then - * enables and settles once the mutation completes. Tests only need to wait once. + * 5. Waiting for the backing queries and mutations to settle * * This allows tests to focus on asserting behavior rather than setup logic, * making test code more ergonomic and readable. * * @param server - MSW server instance for mocking API endpoints * @param wrapper - React Query wrapper component for hook testing + * @param queryClient - Query client backing the wrapper, used to await settling * @param options - Test configuration options (accessibility, changelog data) - * @returns The renderHook result with the query in a settled state + * @returns The renderHook result with the backing data loaded */ async function setupQuery( server: SetupServer, wrapper: TestWrapperComponent, + queryClient: QueryClient, options: SetupQueryOptions = {}, ) { const { accessibility = null, changelog = {} } = options; @@ -136,9 +152,7 @@ async function setupQuery( wrapper, }); - // Wait for query to settle. The query is disabled until preferences are initialized, - // so it stays in loading state during the mutation, then enables and settles naturally. - await waitForQuerySettled(result); + await waitForBackingQueriesSettled(queryClient); return result; } @@ -150,20 +164,22 @@ async function setupQuery( * 1. Mocking the user API endpoint with customizable lastSeenDate * 2. Mocking the user mutation endpoint (for updating preferences) * 3. Mocking the changelog API endpoint with customizable posts - * 4. Rendering both the query hook (to read state) and mutation hook (to update state) - * 5. Waiting for the initial query to settle before tests run + * 4. Rendering both the `useWhatsNew` hook (to read state) and mutation hook (to update state) + * 5. Waiting for the backing queries to settle before tests run * * This allows mutation tests to immediately call `mutation.mutateAsync()` without worrying * about setup, making test code more ergonomic and focused on the mutation behavior. * * @param server - MSW server instance for mocking API endpoints * @param wrapper - React Query wrapper component for hook testing + * @param queryClient - Query client backing the wrapper, used to await settling * @param options - Test configuration options (lastSeenDate, posts) - * @returns Both query and mutation hook results, ready for testing mutations + * @returns Both `useWhatsNew` and mutation hook results, ready for testing mutations */ async function setupMutation( server: SetupServer, wrapper: TestWrapperComponent, + queryClient: QueryClient, options: SetupMutationOptions = {}, ) { const { lastSeenDate = dates.past, posts = [fixtures.entries.newEntry()] } = options; @@ -211,7 +227,7 @@ async function setupMutation( wrapper, }); - await waitForQuerySettled(query.result); + await waitForBackingQueriesSettled(queryClient); return { query: query.result, @@ -228,8 +244,8 @@ const queryTest = baseTest.extend<{ }>({ ...serverFixture, ...queryClientFixtures, - setup: async ({ server, wrapper }, provide) => { - await provide((options) => setupQuery(server, wrapper, options)); + setup: async ({ server, wrapper, queryClient }, provide) => { + await provide((options) => setupQuery(server, wrapper, queryClient, options)); }, }); @@ -241,8 +257,8 @@ const mutationTest = baseTest.extend<{ }>({ ...serverFixture, ...queryClientFixtures, - setup: async ({ server, wrapper }, provide) => { - await provide((options) => setupMutation(server, wrapper, options)); + setup: async ({ server, wrapper, queryClient }, provide) => { + await provide((options) => setupMutation(server, wrapper, queryClient, options)); }, }); @@ -257,7 +273,7 @@ describe('useWhatsNew', () => { accessibility: JSON.stringify(fixtures.preferences.withLastSeen(dates.past)), }); - expect(result.current.data?.hasNew).toBe(true); + expect(result.current.hasNew).toBe(true); }); }); @@ -327,7 +343,7 @@ describe('useWhatsNew', () => { ].forEach(({ scenario, input }) => { queryTest(scenario, async ({ setup }) => { const result = await setup(input); - expect(result.current.data?.hasNew).toBe(false); + expect(result.current.hasNew).toBe(false); }); }); }); @@ -338,14 +354,14 @@ describe('useDismissWhatsNew', () => { mutationTest('changes hasNew from true to false', async ({ setup }) => { const { query, mutation } = await setup(); - expect(query.current.data?.hasNew).toBe(true); + expect(query.current.hasNew).toBe(true); await act(async () => { await mutation.current.mutateAsync(); }); await waitFor(() => { - expect(query.current.data?.hasNew).toBe(false); + expect(query.current.hasNew).toBe(false); }); }); @@ -364,14 +380,14 @@ describe('useDismissWhatsNew', () => { ], }); - expect(query.current.data?.hasNew).toBe(true); + expect(query.current.hasNew).toBe(true); await act(async () => { await mutation.current.mutateAsync(); }); await waitFor(() => { - expect(query.current.data?.hasNew).toBe(false); + expect(query.current.hasNew).toBe(false); }); }); @@ -380,14 +396,14 @@ describe('useDismissWhatsNew', () => { posts: [], }); - expect(query.current.data?.hasNew).toBe(false); + expect(query.current.hasNew).toBe(false); await act(async () => { await mutation.current.mutateAsync(); }); await waitFor(() => { - expect(query.current.data?.hasNew).toBe(false); + expect(query.current.hasNew).toBe(false); }); }); }); diff --git a/apps/admin/src/whats-new/hooks/use-whats-new.ts b/apps/admin/src/whats-new/hooks/use-whats-new.ts index fdfb1db8cce3..41845b718dda 100644 --- a/apps/admin/src/whats-new/hooks/use-whats-new.ts +++ b/apps/admin/src/whats-new/hooks/use-whats-new.ts @@ -1,18 +1,12 @@ -import { useEffect } from 'react'; -import { - useQuery, - useMutation, - type UseQueryResult, - type UseMutationResult, -} from '@tanstack/react-query'; +import { useEffect, useMemo } from 'react'; +import { useMutation, type UseMutationResult } from '@tanstack/react-query'; import { useUserPreferences, useEditUserPreferences, - type Preferences, type WhatsNewPreferences, } from '@/hooks/user-preferences'; -import { useChangelog, type ChangelogEntry } from './use-changelog'; +import { useChangelog } from './use-changelog'; function getDefaultWhatsNewPreferences(): WhatsNewPreferences { return { @@ -24,22 +18,13 @@ interface WhatsNewData { hasNew: boolean; } -const whatsNewQueryKey = ( - preferences: Preferences | undefined, - latestEntry: ChangelogEntry | undefined, -) => - [ - 'whatsNew', - preferences?.whatsNew?.lastSeenDate?.toISOString(), - latestEntry?.publishedAt.toISOString(), - ] as const; - -export const useWhatsNew = (): UseQueryResult => { +export const useWhatsNew = (): WhatsNewData => { const { data: preferences, isSuccess: isPreferencesLoaded } = useUserPreferences(); const { data: changelog, isSuccess: isChangelogLoaded } = useChangelog(); const { mutateAsync: updatePreferences } = useEditUserPreferences(); - const hasWhatsNewPreferences = !!preferences?.whatsNew?.lastSeenDate; + const lastSeenDate = preferences?.whatsNew?.lastSeenDate; + const hasWhatsNewPreferences = !!lastSeenDate; // Initialize default whatsNewPreferences if missing or invalid useEffect(() => { @@ -52,25 +37,13 @@ export const useWhatsNew = (): UseQueryResult => { const latestEntry = changelog?.entries[0]; - return useQuery({ - queryKey: whatsNewQueryKey(preferences, latestEntry), - queryFn: () => { - if (!latestEntry) { - return { hasNew: false }; - } - - // Safe to assert non-null because query is only enabled when hasWhatsNewPreferences is true, - // and useEffect ensures whatsNew is initialized with a valid lastSeenDate - const lastSeenDate = preferences!.whatsNew!.lastSeenDate!; - - const hasNew = latestEntry.publishedAt > lastSeenDate; + return useMemo(() => { + if (!isChangelogLoaded || !lastSeenDate || !latestEntry) { + return { hasNew: false }; + } - return { hasNew }; - }, - enabled: isChangelogLoaded && hasWhatsNewPreferences, - staleTime: Infinity, - gcTime: 0, - }); + return { hasNew: latestEntry.publishedAt > lastSeenDate }; + }, [isChangelogLoaded, lastSeenDate, latestEntry]); }; export const useDismissWhatsNew = (): UseMutationResult => { From 222667b20ee272beeb2bf768c124593dcd1cb534 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 24 Aug 2026 11:39:08 -0500 Subject: [PATCH 3/7] Moved the limiter to admin-x-framework (#30226) no ref Unified on one limiter instead of multiple implementations. --- apps/admin-x-framework/package.json | 1 + apps/admin-x-framework/src/hooks.ts | 4 + .../src/hooks/use-host-limits.ts | 12 +++ .../src/hooks/use-limiter.ts} | 81 +++++++++---------- apps/admin-x-framework/src/limit-service.d.ts | 19 +++++ apps/admin-x-framework/src/utils/errors.ts | 37 ++++++++- .../test/unit/hooks/use-host-limits.test.ts | 46 +++++++++++ .../test/unit/utils/errors.test.ts | 20 ++++- apps/admin/package.json | 1 - .../src/analytics/hooks/use-limiter.test.ts | 75 ----------------- apps/admin/src/analytics/hooks/use-limiter.ts | 21 ----- .../overview/components/overview-kpis.tsx | 6 +- .../src/settings/advanced/integrations.tsx | 5 +- .../integrations/add-integration-modal.tsx | 4 +- .../integrations/transistor-modal.tsx | 6 +- .../advanced/integrations/zapier-modal.tsx | 6 +- .../advanced/labs/private-features.tsx | 3 +- .../newsletters/add-newsletter-modal.tsx | 9 ++- .../newsletters/newsletter-detail-modal.tsx | 3 +- .../settings/general/invite-user-modal.tsx | 5 +- .../settings/general/user-detail-modal.tsx | 10 ++- apps/admin/src/settings/growth/network.tsx | 3 +- .../hooks/use-check-theme-limit-error.tsx | 7 +- apps/admin/src/settings/membership/access.tsx | 6 +- .../src/settings/membership/analytics.tsx | 3 +- .../stripe/stripe-connect-modal.tsx | 5 +- apps/admin/src/settings/membership/tiers.tsx | 4 +- apps/admin/src/vite-env.d.ts | 19 ----- pnpm-lock.yaml | 6 +- 29 files changed, 221 insertions(+), 206 deletions(-) create mode 100644 apps/admin-x-framework/src/hooks/use-host-limits.ts rename apps/{admin/src/settings/hooks/use-limiter.tsx => admin-x-framework/src/hooks/use-limiter.ts} (61%) create mode 100644 apps/admin-x-framework/src/limit-service.d.ts create mode 100644 apps/admin-x-framework/test/unit/hooks/use-host-limits.test.ts delete mode 100644 apps/admin/src/analytics/hooks/use-limiter.test.ts delete mode 100644 apps/admin/src/analytics/hooks/use-limiter.ts diff --git a/apps/admin-x-framework/package.json b/apps/admin-x-framework/package.json index 4db6cf3b5318..80a580c645a6 100644 --- a/apps/admin-x-framework/package.json +++ b/apps/admin-x-framework/package.json @@ -77,6 +77,7 @@ "@tanstack/react-query": "catalog:", "@tinybirdco/charts": "0.3.0", "@tryghost/custom-field-types": "workspace:*", + "@tryghost/limit-service": "catalog:", "@tryghost/nql-string": "workspace:*", "@tryghost/shade": "workspace:*", "bson-objectid": "catalog:", diff --git a/apps/admin-x-framework/src/hooks.ts b/apps/admin-x-framework/src/hooks.ts index ea3bd91c2193..9fae880e301b 100644 --- a/apps/admin-x-framework/src/hooks.ts +++ b/apps/admin-x-framework/src/hooks.ts @@ -11,6 +11,10 @@ export type { } from './hooks/use-form'; export { default as useHandleError } from './hooks/use-handle-error'; export { useFeatureFlag } from './hooks/use-feature-flag'; +export { useHostLimits } from './hooks/use-host-limits'; +export type { HostLimits } from './hooks/use-host-limits'; +export { useLimiter } from './hooks/use-limiter'; +export type { Limiter } from './hooks/use-limiter'; export { useKoenigFileUpload, koenigFileUploadTypes } from './hooks/use-koenig-file-upload'; export { useKoenigFetchEmbed } from './hooks/use-koenig-fetch-embed'; export type { KoenigFileUploadType } from './hooks/use-koenig-file-upload'; diff --git a/apps/admin-x-framework/src/hooks/use-host-limits.ts b/apps/admin-x-framework/src/hooks/use-host-limits.ts new file mode 100644 index 000000000000..b57e511b41ec --- /dev/null +++ b/apps/admin-x-framework/src/hooks/use-host-limits.ts @@ -0,0 +1,12 @@ +import { type Config, useBrowseConfig } from '../api/config'; + +export type HostLimits = NonNullable['limits']>; + +/** + * The site's host plan limits from config — undefined while config is loading + * or when the site has none. + */ +export const useHostLimits = (): HostLimits | undefined => { + const { data } = useBrowseConfig({ refetchOnMount: false }); + return data?.config.hostSettings?.limits; +}; diff --git a/apps/admin/src/settings/hooks/use-limiter.tsx b/apps/admin-x-framework/src/hooks/use-limiter.ts similarity index 61% rename from apps/admin/src/settings/hooks/use-limiter.tsx rename to apps/admin-x-framework/src/hooks/use-limiter.ts index d4f57744faa7..bbaeffefd932 100644 --- a/apps/admin/src/settings/hooks/use-limiter.tsx +++ b/apps/admin-x-framework/src/hooks/use-limiter.ts @@ -1,39 +1,18 @@ -import useStaffUsers from './use-staff-users'; -import { useBrowseMembers } from '@tryghost/admin-x-framework/api/members'; -import { useBrowseNewsletters } from '@tryghost/admin-x-framework/api/newsletters'; import { useEffect, useMemo, useState } from 'react'; -import { useGlobalData } from '@/settings/providers/global-data-context'; +import { useBrowseConfig } from '../api/config'; +import { useBrowseInvites } from '../api/invites'; +import { useBrowseMembers } from '../api/members'; +import { useBrowseNewsletters } from '../api/newsletters'; +import { useBrowseRoles } from '../api/roles'; +import { useBrowseUsers } from '../api/users'; +import { HostLimitError } from '../utils/errors'; const limitServiceImport = import('@tryghost/limit-service'); -export class LimitError extends Error { - public readonly errorType: string; - public readonly errorDetails: string; - - constructor({ - errorType, - errorDetails, - message, - }: { - errorType: string; - errorDetails: string; - message: string; - }) { +// limit-service constructs its misconfiguration error with a single options object +class IncorrectUsageError extends Error { + constructor({ message }: { message: string }) { super(message); - this.errorType = errorType; - this.errorDetails = errorDetails; - } -} - -export class IncorrectUsageError extends LimitError { - constructor(options: { errorDetails: string; message: string }) { - super(Object.assign({ errorType: 'IncorrectUsageError' }, options)); - } -} - -export class HostLimitError extends LimitError { - constructor(options: { errorDetails: string; message: string }) { - super(Object.assign({ errorType: 'HostLimitError' }, options)); } } @@ -55,8 +34,17 @@ interface LimiterLimits { }; } -export const useLimiter = () => { - const { config } = useGlobalData(); +export interface Limiter { + isLimited: (limitName: string) => boolean; + isDisabled: (limitName: string) => boolean; + checkWouldGoOverLimit: (limitName: string) => Promise; + errorIfWouldGoOverLimit: (limitName: string, metadata?: Record) => Promise; + errorIfIsOverLimit: (limitName: string) => Promise; +} + +export const useLimiter = (): Limiter => { + const { data: configData } = useBrowseConfig({ refetchOnMount: false }); + const config = configData?.config; const [LimitService, setLimitService] = useState< typeof import('@tryghost/limit-service').default | null >(null); @@ -65,7 +53,10 @@ export const useLimiter = () => { void limitServiceImport.then((exports) => setLimitService(() => exports.default)); }, []); - const { users, contributorUsers, invites, isLoading } = useStaffUsers(); + const { data: { users } = { users: [] }, isLoading: usersLoading } = useBrowseUsers(); + const { data: { invites } = { invites: [] }, isLoading: invitesLoading } = useBrowseInvites(); + const { data: { roles } = {}, isLoading: rolesLoading } = useBrowseRoles(); + const isStaffLoading = usersLoading || invitesLoading || rolesLoading; const { refetch: fetchMembers } = useBrowseMembers({ searchParams: { limit: '1' }, enabled: false, @@ -76,12 +67,12 @@ export const useLimiter = () => { }); const helpLink = useMemo(() => { - if (config.hostSettings?.billing?.enabled === true && config.hostSettings?.billing?.url) { + if (config?.hostSettings?.billing?.enabled === true && config.hostSettings.billing.url) { return config.hostSettings.billing.url; } else { return 'https://ghost.org/help/'; } - }, [config.hostSettings?.billing]); + }, [config?.hostSettings?.billing]); return useMemo(() => { // Return a stable no-op API when the limiter isn't ready @@ -94,7 +85,7 @@ export const useLimiter = () => { errorIfIsOverLimit: (): Promise => Promise.resolve(), }; - if (!LimitService || !config.hostSettings?.limits || isLoading) { + if (!LimitService || !config?.hostSettings?.limits || isStaffLoading) { return noOpLimiter; } @@ -103,12 +94,16 @@ export const useLimiter = () => { if (limits.staff) { limits.staff.currentCountQuery = () => { - // useStaffUsers will only return the first 100 users by default, but we can assume - // that either there's no limit or the limit is <100 + // Keep the existing first-page behavior for this move. Full pagination is tracked in + // PLA-369 because excluded users/invites can push countable staff onto later pages. const staffUsers = users.filter( - (u) => u.status !== 'inactive' && !contributorUsers.includes(u), + (user) => + user.status !== 'inactive' && !user.roles.some((role) => role.name === 'Contributor'), ); - const staffInvites = invites.filter((i) => i.role !== 'Contributor'); + const staffInvites = invites.filter((invite) => { + const role = roles?.find(({ id }) => id === invite.role_id); + return role?.name !== 'Contributor'; + }); return Promise.resolve(staffUsers.length + staffInvites.length); }; @@ -152,12 +147,12 @@ export const useLimiter = () => { }, [ LimitService, config, - contributorUsers, fetchMembers, fetchNewsletters, helpLink, invites, - isLoading, + isStaffLoading, + roles, users, ]); }; diff --git a/apps/admin-x-framework/src/limit-service.d.ts b/apps/admin-x-framework/src/limit-service.d.ts new file mode 100644 index 000000000000..0e873beedcbb --- /dev/null +++ b/apps/admin-x-framework/src/limit-service.d.ts @@ -0,0 +1,19 @@ +declare module '@tryghost/limit-service' { + type LimitOptions = Record; + export default class LimitService { + loadLimits(config: { + limits: object; + subscription?: unknown; + helpLink?: string; + db?: unknown; + errors: Record; + }): void; + isLimited(limitName: string): boolean; + isDisabled(limitName: string): boolean; + checkIsOverLimit(limitName: string, options?: LimitOptions): Promise; + checkWouldGoOverLimit(limitName: string, options?: LimitOptions): Promise; + errorIfIsOverLimit(limitName: string, options?: LimitOptions): Promise; + errorIfWouldGoOverLimit(limitName: string, options?: LimitOptions): Promise; + checkIfAnyOverLimit(options?: LimitOptions): Promise; + } +} diff --git a/apps/admin-x-framework/src/utils/errors.ts b/apps/admin-x-framework/src/utils/errors.ts index 952cfa2260c9..0839ec290876 100644 --- a/apps/admin-x-framework/src/utils/errors.ts +++ b/apps/admin-x-framework/src/utils/errors.ts @@ -36,7 +36,7 @@ export class JSONError extends APIError { public readonly data?: ErrorResponse; constructor( - response: Response, + response: Response | undefined, data?: ErrorResponse, message?: string, errorOptions?: ErrorOptions, @@ -111,9 +111,40 @@ export class ThemeValidationError extends JSONError { } } +export interface HostLimitErrorDetails { + name?: string; + limit?: number; + total?: number; +} + +interface HostLimitOptions { + message?: string; + errorDetails?: HostLimitErrorDetails; + help?: string; +} + +// Constructed two ways: from an API error response, and by @tryghost/limit-service +// (via useLimiter), which news the registered class with a single options object. export class HostLimitError extends JSONError { - constructor(response: Response, data: ErrorResponse, errorOptions?: ErrorOptions) { - super(response, data, 'A hosting plan limit was reached or exceeded.', errorOptions); + public readonly errorDetails?: HostLimitErrorDetails; + + constructor(response: Response, data: ErrorResponse, errorOptions?: ErrorOptions); + constructor(limit: HostLimitOptions); + constructor( + responseOrLimit: Response | HostLimitOptions, + data?: ErrorResponse, + errorOptions?: ErrorOptions, + ) { + if (responseOrLimit instanceof Response) { + super(responseOrLimit, data, 'A hosting plan limit was reached or exceeded.', errorOptions); + } else { + super( + undefined, + undefined, + responseOrLimit.message || 'A hosting plan limit was reached or exceeded.', + ); + this.errorDetails = responseOrLimit.errorDetails; + } } } diff --git a/apps/admin-x-framework/test/unit/hooks/use-host-limits.test.ts b/apps/admin-x-framework/test/unit/hooks/use-host-limits.test.ts new file mode 100644 index 000000000000..fcae95d63a20 --- /dev/null +++ b/apps/admin-x-framework/test/unit/hooks/use-host-limits.test.ts @@ -0,0 +1,46 @@ +import { renderHook } from '@testing-library/react'; +import { useHostLimits } from '../../../src/hooks/use-host-limits'; + +vi.mock('../../../src/api/config', () => ({ + useBrowseConfig: vi.fn(), +})); + +import { useBrowseConfig } from '../../../src/api/config'; + +const mockUseBrowseConfig = vi.mocked(useBrowseConfig); + +const withConfig = (config: unknown) => { + mockUseBrowseConfig.mockReturnValue({ data: config && { config } } as ReturnType< + typeof useBrowseConfig + >); +}; + +describe('useHostLimits', () => { + afterEach(() => { + vi.clearAllMocks(); + }); + + it('returns the host limits from config', () => { + withConfig({ hostSettings: { limits: { limitAnalytics: { disabled: true } } } }); + + const { result } = renderHook(() => useHostLimits()); + + expect(result.current?.limitAnalytics?.disabled).toBe(true); + }); + + it('returns undefined when the site has no host limits', () => { + withConfig({ hostSettings: {} }); + + const { result } = renderHook(() => useHostLimits()); + + expect(result.current).toBeUndefined(); + }); + + it('returns undefined before config has loaded', () => { + withConfig(undefined); + + const { result } = renderHook(() => useHostLimits()); + + expect(result.current).toBeUndefined(); + }); +}); diff --git a/apps/admin-x-framework/test/unit/utils/errors.test.ts b/apps/admin-x-framework/test/unit/utils/errors.test.ts index 13be63c91312..ded567568c40 100644 --- a/apps/admin-x-framework/test/unit/utils/errors.test.ts +++ b/apps/admin-x-framework/test/unit/utils/errors.test.ts @@ -243,6 +243,24 @@ describe('errors utils', () => { expect(error.data).toBe(mockErrorResponse); }); + // The shape @tryghost/limit-service passes when it constructs the registered class + it('creates error from a limit-service options object', () => { + const error = new HostLimitError({ + message: 'Your plan supports up to 5 staff users.', + errorDetails: { name: 'staff', limit: 5, total: 6 }, + help: 'https://ghost.org/help/', + }); + expect(error.message).toBe('Your plan supports up to 5 staff users.'); + expect(error.errorDetails).toEqual({ name: 'staff', limit: 5, total: 6 }); + expect(error.response).toBeUndefined(); + expect(error.data).toBeUndefined(); + }); + + it('falls back to the generic message when a limit-service error has none', () => { + const error = new HostLimitError({ errorDetails: { name: 'customThemes' } }); + expect(error.message).toBe('A hosting plan limit was reached or exceeded.'); + }); + it('is included in errorsWithMessage', () => { expect(errorsWithMessage).toContain(HostLimitError); }); @@ -365,7 +383,7 @@ describe('errors utils', () => { }, ], }; - return new HostLimitError({} as Response, data); + return new HostLimitError(new Response(), data); }; it('returns the context of any error carrying an API body, not only a validation one', () => { diff --git a/apps/admin/package.json b/apps/admin/package.json index 34e01d9f519e..0ec2f4432c4d 100644 --- a/apps/admin/package.json +++ b/apps/admin/package.json @@ -36,7 +36,6 @@ "@tryghost/i18n": "workspace:*", "@tryghost/kg-unsplash-selector": "workspace:*", "@tryghost/koenig-lexical": "workspace:*", - "@tryghost/limit-service": "catalog:", "@tryghost/nql": "catalog:", "@tryghost/nql-lang": "catalog:", "@tryghost/nql-string": "workspace:*", diff --git a/apps/admin/src/analytics/hooks/use-limiter.test.ts b/apps/admin/src/analytics/hooks/use-limiter.test.ts deleted file mode 100644 index a9de807a6367..000000000000 --- a/apps/admin/src/analytics/hooks/use-limiter.test.ts +++ /dev/null @@ -1,75 +0,0 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { renderHook } from '@testing-library/react'; -import { useLimiter } from '@/analytics/hooks/use-limiter'; - -vi.mock('@/shared/analytics/use-analytics-data', () => ({ - useAnalyticsData: vi.fn(), -})); - -const mockUseAnalyticsData = vi.mocked( - await import('@/shared/analytics/use-analytics-data'), -).useAnalyticsData; - -type AnalyticsData = ReturnType; - -const withConfig = (config: unknown) => { - mockUseAnalyticsData.mockReturnValue({ config } as AnalyticsData); -}; - -describe('useLimiter', () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - it('reports the limit when limitAnalytics is disabled', () => { - withConfig({ hostSettings: { limits: { limitAnalytics: { disabled: true } } } }); - - const { result } = renderHook(() => useLimiter()); - - expect(result.current.isLimited('limitAnalytics')).toBe(true); - }); - - it('does not report the limit when limitAnalytics is not disabled', () => { - withConfig({ hostSettings: { limits: { limitAnalytics: { disabled: false } } } }); - - const { result } = renderHook(() => useLimiter()); - - expect(result.current.isLimited('limitAnalytics')).toBe(false); - }); - - it('does not report the limit when the site has no host limits', () => { - withConfig({ hostSettings: {} }); - - const { result } = renderHook(() => useLimiter()); - - expect(result.current.isLimited('limitAnalytics')).toBe(false); - }); - - it('does not report the limit before config has loaded', () => { - withConfig(undefined); - - const { result } = renderHook(() => useLimiter()); - - expect(result.current.isLimited('limitAnalytics')).toBe(false); - }); - - it('does not report unknown limits even when host limits exist', () => { - withConfig({ hostSettings: { limits: { limitAnalytics: { disabled: true } } } }); - - const { result } = renderHook(() => useLimiter()); - - expect(result.current.isLimited('limitMembers')).toBe(false); - }); - - // Regression guard: `data` used to be the raw config response, so this hook - // read `data.config.hostSettings`. When the provider started handing over the - // unwrapped Config, the extra hop silently resolved to undefined — and kept - // typechecking, because Config ends in an index signature. - it('reads hostSettings off the config itself, not a nested config property', () => { - withConfig({ config: { hostSettings: { limits: { limitAnalytics: { disabled: true } } } } }); - - const { result } = renderHook(() => useLimiter()); - - expect(result.current.isLimited('limitAnalytics')).toBe(false); - }); -}); diff --git a/apps/admin/src/analytics/hooks/use-limiter.ts b/apps/admin/src/analytics/hooks/use-limiter.ts deleted file mode 100644 index 01a4132bbde9..000000000000 --- a/apps/admin/src/analytics/hooks/use-limiter.ts +++ /dev/null @@ -1,21 +0,0 @@ -import { useAnalyticsData } from '@/shared/analytics/use-analytics-data'; - -export const useLimiter = () => { - const { config } = useAnalyticsData(); - - const isLimited = (limitName: string): boolean => { - if (!config?.hostSettings?.limits) { - return false; - } - - if (limitName === 'limitAnalytics') { - return config.hostSettings.limits.limitAnalytics?.disabled === true; - } - - return false; - }; - - return { - isLimited, - }; -}; diff --git a/apps/admin/src/analytics/views/stats/overview/components/overview-kpis.tsx b/apps/admin/src/analytics/views/stats/overview/components/overview-kpis.tsx index a90746872a69..04df67f52391 100644 --- a/apps/admin/src/analytics/views/stats/overview/components/overview-kpis.tsx +++ b/apps/admin/src/analytics/views/stats/overview/components/overview-kpis.tsx @@ -25,7 +25,7 @@ import { useAppContext } from '@tryghost/admin-x-framework'; import { useAnalytics } from '@/analytics/providers/analytics-context'; import { useAnalyticsData } from '@/shared/analytics/use-analytics-data'; import { upgradeRoute } from '@tryghost/admin-x-framework/api/config'; -import { useLimiter } from '@/analytics/hooks/use-limiter'; +import { useHostLimits } from '@tryghost/admin-x-framework/hooks'; import { useNavigate } from '@tryghost/admin-x-framework'; interface OverviewKPICardProps { @@ -173,9 +173,9 @@ const OverviewKPIs: React.FC = ({ }) => { const navigate = useNavigate(); const { appSettings } = useAppContext(); - const limiter = useLimiter(); + const hostLimits = useHostLimits(); const { config } = useAnalyticsData(); - const isWebAnalyticsLimited = limiter.isLimited('limitAnalytics'); + const isWebAnalyticsLimited = hostLimits?.limitAnalytics?.disabled === true; const areaChartClassName = '-mb-3 h-[10vw] max-h-[200px] min-h-[100px] hover:cursor-pointer!'; diff --git a/apps/admin/src/settings/advanced/integrations.tsx b/apps/admin/src/settings/advanced/integrations.tsx index 49172fe76e8c..2f19d36e0899 100644 --- a/apps/admin/src/settings/advanced/integrations.tsx +++ b/apps/admin/src/settings/advanced/integrations.tsx @@ -27,7 +27,7 @@ import { getSettingValues } from '@tryghost/admin-x-framework/api/settings'; import { toast } from 'sonner'; import { useConfirmation } from '@/settings/providers/confirmation-context'; import { useGlobalData } from '@/settings/providers/global-data-context'; -import { useHandleError } from '@tryghost/admin-x-framework/hooks'; +import { useHandleError, useHostLimits } from '@tryghost/admin-x-framework/hooks'; import { useSettingsNavigation } from '@/settings/hooks/use-settings-navigation'; import { DEFAULT_UPGRADE_ROUTE } from '@tryghost/admin-x-framework/api/config'; import { useUpgradeRoute } from '@/settings/hooks/use-upgrade-route'; @@ -143,7 +143,6 @@ const IntegrationItem: React.FC = ({ }; const BuiltInIntegrations: React.FC = () => { - const { config } = useGlobalData(); const upgradeRoute = useUpgradeRoute(); const { updateRoute } = useSettingsNavigation(); @@ -151,7 +150,7 @@ const BuiltInIntegrations: React.FC = () => { updateRoute(modal); }; - const builtInApiIntegrationsDisabled = config.hostSettings?.limits?.customIntegrations?.disabled; + const builtInApiIntegrationsDisabled = useHostLimits()?.customIntegrations?.disabled; const pinturaEditor = usePinturaEditor(); diff --git a/apps/admin/src/settings/advanced/integrations/add-integration-modal.tsx b/apps/admin/src/settings/advanced/integrations/add-integration-modal.tsx index f1ceb1abf68f..bdb4b6112b34 100644 --- a/apps/admin/src/settings/advanced/integrations/add-integration-modal.tsx +++ b/apps/admin/src/settings/advanced/integrations/add-integration-modal.tsx @@ -1,12 +1,12 @@ import { useEffect, useState } from 'react'; import { Field, FieldError, FieldGroup, FieldLabel, Input } from '@tryghost/shade/components'; -import { HostLimitError, useLimiter } from '@/settings/hooks/use-limiter'; +import { HostLimitError } from '@tryghost/admin-x-framework/errors'; import { useConfirmation } from '@/settings/providers/confirmation-context'; import { useSettingsNavigation } from '@/settings/hooks/use-settings-navigation'; import { useUpgradeRoute } from '@/settings/hooks/use-upgrade-route'; import { SettingsModal } from '@tryghost/shade/patterns'; import { useCreateIntegration } from '@tryghost/admin-x-framework/api/integrations'; -import { useHandleError } from '@tryghost/admin-x-framework/hooks'; +import { useHandleError, useLimiter } from '@tryghost/admin-x-framework/hooks'; function AddIntegrationModal() { const { updateRoute } = useSettingsNavigation(); diff --git a/apps/admin/src/settings/advanced/integrations/transistor-modal.tsx b/apps/admin/src/settings/advanced/integrations/transistor-modal.tsx index 9d381b604438..2b1035866f4c 100644 --- a/apps/admin/src/settings/advanced/integrations/transistor-modal.tsx +++ b/apps/admin/src/settings/advanced/integrations/transistor-modal.tsx @@ -22,13 +22,13 @@ import { useBrowseIntegrations } from '@tryghost/admin-x-framework/api/integrati import { useConfirmation } from '@/settings/providers/confirmation-context'; import { useEffect, useState } from 'react'; import { useGlobalData } from '@/settings/providers/global-data-context'; -import { useHandleError } from '@tryghost/admin-x-framework/hooks'; +import { useHandleError, useHostLimits } from '@tryghost/admin-x-framework/hooks'; import { useRefreshAPIKey } from '@tryghost/admin-x-framework/api/api-keys'; import { useSettingsNavigation } from '@/settings/hooks/use-settings-navigation'; function TransistorModal() { const { updateRoute } = useSettingsNavigation(); - const { config, settings } = useGlobalData(); + const { settings } = useGlobalData(); const { mutateAsync: editSettings } = useEditSettings(); const { data: { integrations } = { integrations: [] } } = useBrowseIntegrations(); @@ -37,7 +37,7 @@ function TransistorModal() { const { confirm } = useConfirmation(); const [regenerated, setRegenerated] = useState(false); - const builtInApiIntegrationsDisabled = config.hostSettings?.limits?.customIntegrations?.disabled; + const builtInApiIntegrationsDisabled = useHostLimits()?.customIntegrations?.disabled; const [transistorEnabled] = getSettingValues(settings, ['transistor']); const [enabled, setEnabled] = useState(!!transistorEnabled); const [okLabel, setOkLabel] = useState('Save'); diff --git a/apps/admin/src/settings/advanced/integrations/zapier-modal.tsx b/apps/admin/src/settings/advanced/integrations/zapier-modal.tsx index c5bdbcb4bb7a..3b18dc919147 100644 --- a/apps/admin/src/settings/advanced/integrations/zapier-modal.tsx +++ b/apps/admin/src/settings/advanced/integrations/zapier-modal.tsx @@ -15,8 +15,7 @@ import { getGhostPaths } from '@tryghost/admin-x-framework/helpers'; import { useBrowseIntegrations } from '@tryghost/admin-x-framework/api/integrations'; import { useConfirmation } from '@/settings/providers/confirmation-context'; import { useEffect, useState } from 'react'; -import { useGlobalData } from '@/settings/providers/global-data-context'; -import { useHandleError } from '@tryghost/admin-x-framework/hooks'; +import { useHandleError, useHostLimits } from '@tryghost/admin-x-framework/hooks'; import { useRefreshAPIKey } from '@tryghost/admin-x-framework/api/api-keys'; import { useSettingsNavigation } from '@/settings/hooks/use-settings-navigation'; import { useSettingsApp } from '@/settings/providers/settings-app-context'; @@ -32,14 +31,13 @@ function ZapierModal() { const { updateRoute } = useSettingsNavigation(); const { zapierTemplates } = useSettingsApp(); const { data: { integrations } = { integrations: [] } } = useBrowseIntegrations(); - const { config } = useGlobalData(); const { mutateAsync: refreshAPIKey } = useRefreshAPIKey(); const handleError = useHandleError(); const { confirm } = useConfirmation(); const [regenerated, setRegenerated] = useState(false); - const zapierDisabled = config.hostSettings?.limits?.customIntegrations?.disabled; + const zapierDisabled = useHostLimits()?.customIntegrations?.disabled; const integration = integrations.find(({ slug }) => slug === 'zapier'); const adminApiKey = integration?.api_keys?.find((key) => key.type === 'admin'); diff --git a/apps/admin/src/settings/advanced/labs/private-features.tsx b/apps/admin/src/settings/advanced/labs/private-features.tsx index 841c7bc5d0a8..7276ff5b1d02 100644 --- a/apps/admin/src/settings/advanced/labs/private-features.tsx +++ b/apps/admin/src/settings/advanced/labs/private-features.tsx @@ -2,7 +2,8 @@ import FeatureToggle from './feature-toggle'; import LabItem from './lab-item'; import React, { useEffect, useState } from 'react'; import { ActionList } from '@tryghost/shade/components'; -import { HostLimitError, useLimiter } from '@/settings/hooks/use-limiter'; +import { HostLimitError } from '@tryghost/admin-x-framework/errors'; +import { useLimiter } from '@tryghost/admin-x-framework/hooks'; type Feature = { title: string; diff --git a/apps/admin/src/settings/email/newsletters/add-newsletter-modal.tsx b/apps/admin/src/settings/email/newsletters/add-newsletter-modal.tsx index b12ea3caf6e0..90b023cf8384 100644 --- a/apps/admin/src/settings/email/newsletters/add-newsletter-modal.tsx +++ b/apps/admin/src/settings/email/newsletters/add-newsletter-modal.tsx @@ -10,7 +10,7 @@ import { Switch, Textarea, } from '@tryghost/shade/components'; -import { HostLimitError, useLimiter } from '@/settings/hooks/use-limiter'; +import { HostLimitError } from '@tryghost/admin-x-framework/errors'; import { useConfirmation } from '@/settings/providers/confirmation-context'; import { useSettingsNavigation } from '@/settings/hooks/use-settings-navigation'; import { useUpgradeRoute } from '@/settings/hooks/use-upgrade-route'; @@ -18,7 +18,12 @@ import { SettingsModal } from '@tryghost/shade/patterns'; import { formatNumber } from '@tryghost/shade/utils'; import { useAddNewsletter } from '@tryghost/admin-x-framework/api/newsletters'; import { useBrowseMembers } from '@tryghost/admin-x-framework/api/members'; -import { useFeatureFlag, useForm, useHandleError } from '@tryghost/admin-x-framework/hooks'; +import { + useFeatureFlag, + useForm, + useHandleError, + useLimiter, +} from '@tryghost/admin-x-framework/hooks'; const AddNewsletterModal: React.FC = () => { const { updateRoute } = useSettingsNavigation(); diff --git a/apps/admin/src/settings/email/newsletters/newsletter-detail-modal.tsx b/apps/admin/src/settings/email/newsletters/newsletter-detail-modal.tsx index f19ea1152f0e..b12188eb8ccf 100644 --- a/apps/admin/src/settings/email/newsletters/newsletter-detail-modal.tsx +++ b/apps/admin/src/settings/email/newsletters/newsletter-detail-modal.tsx @@ -39,8 +39,9 @@ import { useFeatureFlag, useForm, useHandleError, + useLimiter, } from '@tryghost/admin-x-framework/hooks'; -import { HostLimitError, useLimiter } from '@/settings/hooks/use-limiter'; +import { HostLimitError } from '@tryghost/admin-x-framework/errors'; import { Inline, Stack } from '@tryghost/shade/primitives'; import { LucideIcon } from '@tryghost/shade/utils'; import { diff --git a/apps/admin/src/settings/general/invite-user-modal.tsx b/apps/admin/src/settings/general/invite-user-modal.tsx index ff7ebe63f8a7..5b27561414de 100644 --- a/apps/admin/src/settings/general/invite-user-modal.tsx +++ b/apps/admin/src/settings/general/invite-user-modal.tsx @@ -1,5 +1,5 @@ import validator from 'validator'; -import { APIError, ValidationError } from '@tryghost/admin-x-framework/errors'; +import { APIError, HostLimitError, ValidationError } from '@tryghost/admin-x-framework/errors'; import { Field, FieldContent, @@ -12,7 +12,6 @@ import { RadioGroup, RadioGroupItem, } from '@tryghost/shade/components'; -import { HostLimitError, useLimiter } from '@/settings/hooks/use-limiter'; import { SettingsModal } from '@tryghost/shade/patterns'; import { Stack } from '@tryghost/shade/primitives'; import { toast } from 'sonner'; @@ -20,7 +19,7 @@ import { useAddInvite, useBrowseInvites } from '@tryghost/admin-x-framework/api/ import { useBrowseRoles } from '@tryghost/admin-x-framework/api/roles'; import { useBrowseUsers } from '@tryghost/admin-x-framework/api/users'; import { useEffect, useState } from 'react'; -import { useFeatureFlag, useHandleError } from '@tryghost/admin-x-framework/hooks'; +import { useFeatureFlag, useHandleError, useLimiter } from '@tryghost/admin-x-framework/hooks'; import { useSettingsNavigation } from '@/settings/hooks/use-settings-navigation'; type RoleType = 'administrator' | 'editor' | 'author' | 'contributor' | 'super editor'; diff --git a/apps/admin/src/settings/general/user-detail-modal.tsx b/apps/admin/src/settings/general/user-detail-modal.tsx index 90a622323c3f..6433876d4c7d 100644 --- a/apps/admin/src/settings/general/user-detail-modal.tsx +++ b/apps/admin/src/settings/general/user-detail-modal.tsx @@ -6,7 +6,7 @@ import clsx from 'clsx'; import usePinturaEditor from '@/settings/hooks/use-pintura-editor'; import useStaffUsers from '@/settings/hooks/use-staff-users'; import validator from 'validator'; -import { APIError } from '@tryghost/admin-x-framework/errors'; +import { APIError, HostLimitError } from '@tryghost/admin-x-framework/errors'; import { Button, DropdownMenu, @@ -19,8 +19,12 @@ import { TabsList, TabsTrigger, } from '@tryghost/shade/components'; -import { type ErrorMessages, useForm, useHandleError } from '@tryghost/admin-x-framework/hooks'; -import { HostLimitError, useLimiter } from '@/settings/hooks/use-limiter'; +import { + type ErrorMessages, + useForm, + useHandleError, + useLimiter, +} from '@tryghost/admin-x-framework/hooks'; import { ImageUpload, ImageUploadAction, diff --git a/apps/admin/src/settings/growth/network.tsx b/apps/admin/src/settings/growth/network.tsx index c7df0819db33..d8e9f696dd1f 100644 --- a/apps/admin/src/settings/growth/network.tsx +++ b/apps/admin/src/settings/growth/network.tsx @@ -12,8 +12,7 @@ import { SettingGroupContent } from '@tryghost/shade/patterns'; import { Switch } from '@tryghost/shade/components'; import { getGhostPaths } from '@tryghost/admin-x-framework/helpers'; import { useGlobalData } from '@/settings/providers/global-data-context'; -import { useHandleError } from '@tryghost/admin-x-framework/hooks'; -import { useLimiter } from '@/settings/hooks/use-limiter'; +import { useHandleError, useLimiter } from '@tryghost/admin-x-framework/hooks'; import { useSettingsNavigation } from '@/settings/hooks/use-settings-navigation'; import { withErrorBoundary } from '@/settings/components/with-error-boundary'; diff --git a/apps/admin/src/settings/hooks/use-check-theme-limit-error.tsx b/apps/admin/src/settings/hooks/use-check-theme-limit-error.tsx index 9c25b13e973f..45d14467f760 100644 --- a/apps/admin/src/settings/hooks/use-check-theme-limit-error.tsx +++ b/apps/admin/src/settings/hooks/use-check-theme-limit-error.tsx @@ -1,6 +1,6 @@ -import { HostLimitError, useLimiter } from './use-limiter'; +import { HostLimitError } from '@tryghost/admin-x-framework/errors'; import { useCallback } from 'react'; -import { useGlobalData } from '@/settings/providers/global-data-context'; +import { useHostLimits, useLimiter } from '@tryghost/admin-x-framework/hooks'; interface UseCheckThemeLimitErrorReturn { checkThemeLimitError: (themeName?: string) => Promise; @@ -12,9 +12,8 @@ interface UseCheckThemeLimitErrorReturn { export const useCheckThemeLimitError = (): UseCheckThemeLimitErrorReturn => { const limiter = useLimiter(); - const { config } = useGlobalData(); - const allowedThemesList = config.hostSettings?.limits?.customThemes?.allowlist; + const allowedThemesList = useHostLimits()?.customThemes?.allowlist; // Single theme: always error const noThemeChangesAllowed = allowedThemesList?.length === 1 || false; diff --git a/apps/admin/src/settings/membership/access.tsx b/apps/admin/src/settings/membership/access.tsx index a903a461664e..da430b787d4d 100644 --- a/apps/admin/src/settings/membership/access.tsx +++ b/apps/admin/src/settings/membership/access.tsx @@ -34,7 +34,7 @@ import { import { toast } from 'sonner'; import { useBrowseTiers } from '@tryghost/admin-x-framework/api/tiers'; import { useGlobalData } from '@/settings/providers/global-data-context'; -import { useLimiter } from '@/settings/hooks/use-limiter'; +import { useHostLimits, useLimiter } from '@tryghost/admin-x-framework/hooks'; import { withErrorBoundary } from '@/settings/components/with-error-boundary'; const SITE_VISIBILITY_OPTIONS = [ @@ -134,10 +134,10 @@ const getAccessOptionLabel = (options: Array<{ value: string; label: string }>, const Access: React.FC<{ keywords: string[] }> = ({ keywords }) => { const [tiersOpen, setTiersOpen] = React.useState(false); - const { settings, config } = useGlobalData(); + const { settings } = useGlobalData(); const limiter = useLimiter(); const isTrialMode = limiter?.isDisabled('publicSiteAccess'); - const publicSiteAccessLimit = config.hostSettings?.limits?.publicSiteAccess; + const publicSiteAccessLimit = useHostLimits()?.publicSiteAccess; const preLaunchTitle = publicSiteAccessLimit?.title || DEFAULT_PRELAUNCH_TITLE; const preLaunchMessage = publicSiteAccessLimit?.error || DEFAULT_PRELAUNCH_MESSAGE; const preLaunchUpgradeUrl = publicSiteAccessLimit?.upgradeUrl || DEFAULT_PRELAUNCH_UPGRADE_URL; diff --git a/apps/admin/src/settings/membership/analytics.tsx b/apps/admin/src/settings/membership/analytics.tsx index 74c043007c0e..e49e3690bff2 100644 --- a/apps/admin/src/settings/membership/analytics.tsx +++ b/apps/admin/src/settings/membership/analytics.tsx @@ -9,9 +9,10 @@ import { Separator, Switch, } from '@tryghost/shade/components'; -import { HostLimitError, useLimiter } from '@/settings/hooks/use-limiter'; +import { HostLimitError } from '@tryghost/admin-x-framework/errors'; import { SettingGroupContent } from '@tryghost/shade/patterns'; import { getSettingValues, isSettingReadOnly } from '@tryghost/admin-x-framework/api/settings'; +import { useLimiter } from '@tryghost/admin-x-framework/hooks'; import { useSettingsNavigation } from '@/settings/hooks/use-settings-navigation'; import { useUpgradeRoute } from '@/settings/hooks/use-upgrade-route'; import { withErrorBoundary } from '@/settings/components/with-error-boundary'; diff --git a/apps/admin/src/settings/membership/stripe/stripe-connect-modal.tsx b/apps/admin/src/settings/membership/stripe/stripe-connect-modal.tsx index eb3f7eba1dd1..8f67075c7ee1 100644 --- a/apps/admin/src/settings/membership/stripe/stripe-connect-modal.tsx +++ b/apps/admin/src/settings/membership/stripe/stripe-connect-modal.tsx @@ -16,8 +16,7 @@ import { Switch, Textarea, } from '@tryghost/shade/components'; -import { HostLimitError, useLimiter } from '@/settings/hooks/use-limiter'; -import { JSONError } from '@tryghost/admin-x-framework/errors'; +import { HostLimitError, JSONError } from '@tryghost/admin-x-framework/errors'; import { LucideIcon } from '@tryghost/shade/utils'; import { SettingsModal } from '@tryghost/shade/patterns'; import { Text } from '@tryghost/shade/primitives'; @@ -34,7 +33,7 @@ import { useBrowseMembers } from '@tryghost/admin-x-framework/api/members'; import { useBrowseTiers, useEditTier } from '@tryghost/admin-x-framework/api/tiers'; import { useConfirmation } from '@/settings/providers/confirmation-context'; import { useGlobalData } from '@/settings/providers/global-data-context'; -import { useHandleError } from '@tryghost/admin-x-framework/hooks'; +import { useHandleError, useLimiter } from '@tryghost/admin-x-framework/hooks'; import { useSettingsNavigation } from '@/settings/hooks/use-settings-navigation'; import { useUpgradeRoute } from '@/settings/hooks/use-upgrade-route'; diff --git a/apps/admin/src/settings/membership/tiers.tsx b/apps/admin/src/settings/membership/tiers.tsx index d962988f6f93..e72d13c6c302 100644 --- a/apps/admin/src/settings/membership/tiers.tsx +++ b/apps/admin/src/settings/membership/tiers.tsx @@ -26,7 +26,7 @@ import { TabsTrigger, } from '@tryghost/shade/components'; import { ChevronDown } from 'lucide-react'; -import { HostLimitError, useLimiter } from '@/settings/hooks/use-limiter'; +import { HostLimitError } from '@tryghost/admin-x-framework/errors'; import { SettingGroupContent } from '@tryghost/shade/patterns'; import { type Setting, @@ -44,7 +44,7 @@ import { currencySelectGroups, validateCurrencyAmount } from '@tryghost/admin-x- import { formatNumber } from '@tryghost/shade/utils'; import { useConfirmation } from '@/settings/providers/confirmation-context'; import { useGlobalData } from '@/settings/providers/global-data-context'; -import { useFeatureFlag, useHandleError } from '@tryghost/admin-x-framework/hooks'; +import { useFeatureFlag, useHandleError, useLimiter } from '@tryghost/admin-x-framework/hooks'; import { useSettingsNavigation } from '@/settings/hooks/use-settings-navigation'; import { useUpgradeRoute } from '@/settings/hooks/use-upgrade-route'; import { withErrorBoundary } from '@/settings/components/with-error-boundary'; diff --git a/apps/admin/src/vite-env.d.ts b/apps/admin/src/vite-env.d.ts index b5a1aa34f2b3..6d1e63307bae 100644 --- a/apps/admin/src/vite-env.d.ts +++ b/apps/admin/src/vite-env.d.ts @@ -4,25 +4,6 @@ interface ImportMetaEnv { readonly GHOST_BUILD_VERSION?: string; } -declare module '@tryghost/limit-service' { - type LimitOptions = Record; - export default class LimitService { - loadLimits(config: { - limits: object; - subscription?: unknown; - helpLink?: string; - db?: unknown; - errors: Record; - }): void; - isLimited(limitName: string): boolean; - isDisabled(limitName: string): boolean; - checkIsOverLimit(limitName: string, options?: LimitOptions): Promise; - checkWouldGoOverLimit(limitName: string, options?: LimitOptions): Promise; - errorIfIsOverLimit(limitName: string, options?: LimitOptions): Promise; - errorIfWouldGoOverLimit(limitName: string, options?: LimitOptions): Promise; - checkIfAnyOverLimit(options?: LimitOptions): Promise; - } -} declare module '@tryghost/nql' { export default function nql(query: string): { queryJSON: (data: unknown) => boolean }; } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0702678aa452..82bdc9806283 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -796,9 +796,6 @@ importers: '@tryghost/koenig-lexical': specifier: workspace:* version: link:../../koenig/koenig-lexical - '@tryghost/limit-service': - specifier: 'catalog:' - version: 1.5.6 '@tryghost/nql': specifier: 0.13.4 version: 0.13.4(supports-color@10.2.2) @@ -1011,6 +1008,9 @@ importers: '@tryghost/custom-field-types': specifier: workspace:* version: link:../../packages/custom-field-types + '@tryghost/limit-service': + specifier: 'catalog:' + version: 1.5.6 '@tryghost/nql-string': specifier: workspace:* version: link:../../packages/nql-string From 8736be91a98cabe3d4dedf6cce150aee75f70157 Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 24 Aug 2026 11:52:10 -0500 Subject: [PATCH 4/7] Added lint guardrails for the admin boundaries (#30224) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no ref The recent cleanup established several invariants by hand; this makes them self-enforcing. Every rule was proven to fire (deliberate violation → error → revert), and product code carries **zero** eslint-disable except four documented ones (see PR). --- .dependency-cruiser.cjs | 24 ++++ .lintstagedrc.cjs | 2 +- apps/admin-x-framework/src/index.ts | 2 + apps/admin/eslint.config.js | 107 ++++++++++++++++++ apps/admin/package.json | 1 + .../src/hooks/use-unsaved-changes-guard.ts | 10 +- .../growth/explore/testimonials-modal.tsx | 1 + .../layout/dirty-navigation-guard.tsx | 2 +- .../announcement-bar-preview.tsx | 24 +++- .../design-and-branding/theme-preview.tsx | 63 +++++++---- .../settings/utils/fetch-frontend-preview.ts | 1 + .../src/whats-new/hooks/use-changelog.ts | 1 + pnpm-lock.yaml | 3 + 13 files changed, 207 insertions(+), 34 deletions(-) diff --git a/.dependency-cruiser.cjs b/.dependency-cruiser.cjs index 8eee566c9283..7e81ab178e0e 100644 --- a/.dependency-cruiser.cjs +++ b/.dependency-cruiser.cjs @@ -129,6 +129,30 @@ module.exports = { }, to: { path: '^@tryghost/(shade|admin-x-framework)' }, }, + // ============================================================ + // apps/ — admin is an app, not a library + // ============================================================ + { + name: 'admin-is-app', + comment: + 'No sibling app or library may depend on @tryghost/admin - whether by package specifier or by relative reach-in. Admin sits at the top of the layer stack.', + severity: 'error', + from: { path: '^apps/', pathNot: '^apps/admin/' }, + to: { path: '^@tryghost/admin($|/)|^apps/admin/' }, + }, + // ============================================================ + // apps/admin — shared/ must stay domain-free + // ============================================================ + { + name: 'admin-shared-no-domains', + comment: + 'apps/admin/src/shared must not import from feature domains. Move code used by a single domain into that domain; keep shared/ generic. In-app imports use the @/ alias, which the cruiser sees as an unresolved @/-prefixed specifier.', + severity: 'error', + from: { path: '^apps/admin/src/shared/' }, + to: { + path: '^(@/|apps/admin/src/)(members|settings|analytics|posts|tags|comments|automations|onboarding|whats-new|layout)($|/)', + }, + }, ], options: { doNotFollow: { path: 'node_modules' }, diff --git a/.lintstagedrc.cjs b/.lintstagedrc.cjs index c202977353d5..5acefc30f187 100644 --- a/.lintstagedrc.cjs +++ b/.lintstagedrc.cjs @@ -130,7 +130,7 @@ module.exports = { ]; }, 'ghost/core/core/{server,shared,frontend}/**/*.{js,ts}': (files) => buildBoundaryCommand(files), - 'apps/{shade,admin-x-framework,activitypub,portal,comments-ui,signup-form,sodo-search,announcement-bar,admin-toolbar}/src/**/*.{js,ts,tsx,jsx}': + 'apps/{admin,shade,admin-x-framework,activitypub,portal,comments-ui,signup-form,sodo-search,announcement-bar,admin-toolbar}/src/**/*.{js,ts,tsx,jsx}': (files) => buildBoundaryCommand(files), '*.{mjs,mts,cts,json,jsonc,json5,yml,yaml,css,mdx}': (files) => buildOxfmtCommand(files), '**/*.md': (files) => [buildOxfmtCommand(files), ...buildMarkdownCommands(files)], diff --git a/apps/admin-x-framework/src/index.ts b/apps/admin-x-framework/src/index.ts index 7c1c3d47bc26..2acd8a7509c9 100644 --- a/apps/admin-x-framework/src/index.ts +++ b/apps/admin-x-framework/src/index.ts @@ -84,6 +84,7 @@ export { export { useNavigationStack } from './providers/navigation-stack-provider'; export { Link, + NavigationType, Outlet, useBlocker, useLocation, @@ -95,6 +96,7 @@ export { useMatch, useMatches, } from 'react-router'; +export type { BlockerFunction } from 'react-router'; // Lazy component loader export { lazyComponent } from './utils/lazy-component'; diff --git a/apps/admin/eslint.config.js b/apps/admin/eslint.config.js index 08249ae4454d..d9f3d2f9f5d0 100644 --- a/apps/admin/eslint.config.js +++ b/apps/admin/eslint.config.js @@ -1,6 +1,18 @@ import noRelativeImportPaths from 'eslint-plugin-no-relative-import-paths'; import * as tseslint from 'typescript-eslint'; import { reactAppConfig } from '@internal/cfg-eslint-react'; +import { shadeLayeredImportsRule } from '@internal/cfg-eslint'; + +// The factory's shade restriction and this file's boundary bans share the +// `no-restricted-imports` rule slot, so the boundary blocks must re-include it. +const shadeRestrictedPaths = shadeLayeredImportsRule['no-restricted-imports'][1].paths; + +const emberBridgeImportPatterns = [ + { + group: ['@/ember-bridge/*', '**/ember-bridge/ember-bridge'], + message: 'Import bridge helpers from the @/ember-bridge barrel, not the implementation module.', + }, +]; const noHardcodedGhostPaths = { meta: { @@ -93,4 +105,99 @@ export default tseslint.config( 'no-relative-import-paths/no-relative-import-paths': ['error', { allowSameFolder: true }], }, }, + // Boundary guardrails. Product code must reach react-router, the Ember + // bridge, and the Admin API through their owning layers. + { + files: ['src/**/*.{ts,tsx}'], + ignores: ['src/**/*.test.*', 'src/ember-bridge/**'], + rules: { + 'no-restricted-imports': [ + 'error', + { + paths: [ + ...shadeRestrictedPaths, + { + name: 'react-router', + message: + 'Import routing APIs (and their types) from @tryghost/admin-x-framework instead of react-router directly.', + }, + ], + patterns: [ + ...emberBridgeImportPatterns, + { + group: ['react-router/*'], + message: + 'Import routing APIs (and their types) from @tryghost/admin-x-framework instead of react-router directly.', + }, + ], + }, + ], + 'no-restricted-syntax': [ + 'error', + { + selector: "MemberExpression[property.name='EmberBridge']", + message: + 'Access Ember through the @/ember-bridge helpers, not window.EmberBridge directly.', + }, + { + selector: "MemberExpression[property.value='EmberBridge']", + message: + 'Access Ember through the @/ember-bridge helpers, not window.EmberBridge directly.', + }, + { + selector: + "VariableDeclarator[init.name=/^(window|globalThis)$/] Property[key.name='EmberBridge']", + message: + 'Access Ember through the @/ember-bridge helpers, not window.EmberBridge directly.', + }, + { + selector: "CallExpression[callee.name='fetch']", + message: + 'Admin API requests belong in the @tryghost/admin-x-framework API layer. For non-Ghost URLs (external services, front-end previews), disable this rule for the line with a reason.', + }, + { + selector: "CallExpression[callee.object.name='window'][callee.property.name='fetch']", + message: + 'Admin API requests belong in the @tryghost/admin-x-framework API layer. For non-Ghost URLs (external services, front-end previews), disable this rule for the line with a reason.', + }, + { + selector: "CallExpression[callee.object.name='globalThis'][callee.property.name='fetch']", + message: + 'Admin API requests belong in the @tryghost/admin-x-framework API layer. For non-Ghost URLs (external services, front-end previews), disable this rule for the line with a reason.', + }, + ], + }, + }, + // Test files keep react-router scaffolding (MemoryRouter, createMemoryRouter) + // and window.EmberBridge stubs, but must still import the bridge barrel. + { + files: ['src/**/*.test.*'], + ignores: ['src/ember-bridge/**'], + rules: { + 'no-restricted-imports': [ + 'error', + { paths: [...shadeRestrictedPaths], patterns: emberBridgeImportPatterns }, + ], + }, + }, + // Advisory only — warnings do not fail CI (`eslint .` without --max-warnings). + // Steers new code to the shade utilities without forcing a bulk conversion. + { + files: ['src/**/*.{ts,tsx}'], + ignores: ['src/**/*.test.*'], + rules: { + '@typescript-eslint/no-restricted-imports': [ + 'warn', + { + paths: [ + { name: 'clsx', message: 'Use cn from @tryghost/shade/utils.' }, + { + name: 'lucide-react', + message: 'Use the LucideIcon namespace from @tryghost/shade/utils.', + }, + ], + }, + ], + }, + }, ); diff --git a/apps/admin/package.json b/apps/admin/package.json index 0ec2f4432c4d..eaa776a3b359 100644 --- a/apps/admin/package.json +++ b/apps/admin/package.json @@ -65,6 +65,7 @@ "zod": "catalog:" }, "devDependencies": { + "@internal/cfg-eslint": "workspace:*", "@internal/cfg-eslint-react": "workspace:*", "@tailwindcss/vite": "catalog:", "@testing-library/jest-dom": "catalog:", diff --git a/apps/admin/src/hooks/use-unsaved-changes-guard.ts b/apps/admin/src/hooks/use-unsaved-changes-guard.ts index 4b6d1217eb3e..e698b1c5e002 100644 --- a/apps/admin/src/hooks/use-unsaved-changes-guard.ts +++ b/apps/admin/src/hooks/use-unsaved-changes-guard.ts @@ -1,9 +1,13 @@ import React from 'react'; -import { NavigationType, useBlocker } from 'react-router'; import { isOnRouterHistoryEntry } from '@/hooks/use-router-history-entry'; -import { useConfirmUnload, useLocation } from '@tryghost/admin-x-framework'; +import { + NavigationType, + useBlocker, + useConfirmUnload, + useLocation, +} from '@tryghost/admin-x-framework'; import { useHashLinkNavigationGuard } from '@/hooks/use-hash-link-navigation-guard'; -import type { BlockerFunction } from 'react-router'; +import type { BlockerFunction } from '@tryghost/admin-x-framework'; type BlockerFunctionArgs = Parameters[0]; diff --git a/apps/admin/src/settings/growth/explore/testimonials-modal.tsx b/apps/admin/src/settings/growth/explore/testimonials-modal.tsx index 846a3c439b7b..de2a60a77f74 100644 --- a/apps/admin/src/settings/growth/explore/testimonials-modal.tsx +++ b/apps/admin/src/settings/growth/explore/testimonials-modal.tsx @@ -63,6 +63,7 @@ const TestimonialsModal = () => { throw new Error('Something went wrong, please try again later.'); } + // eslint-disable-next-line no-restricted-syntax -- external Ghost Explore service, not the Admin API const response = await fetch(exploreTestimonialsUrl, { method: 'POST', headers: { diff --git a/apps/admin/src/settings/layout/dirty-navigation-guard.tsx b/apps/admin/src/settings/layout/dirty-navigation-guard.tsx index ea356d38c4e5..990a93a4edac 100644 --- a/apps/admin/src/settings/layout/dirty-navigation-guard.tsx +++ b/apps/admin/src/settings/layout/dirty-navigation-guard.tsx @@ -1,5 +1,5 @@ import { DirtyConfirmDialog } from '@tryghost/shade/patterns'; -import { NavigationType, useBlocker } from 'react-router'; +import { NavigationType, useBlocker } from '@tryghost/admin-x-framework'; import { useConfirmUnload } from '@tryghost/admin-x-framework/hooks'; import { useGlobalDirtyState } from '@tryghost/shade/utils'; import { isOnRouterHistoryEntry } from '@/hooks/use-router-history-entry'; diff --git a/apps/admin/src/settings/site/announcement-bar/announcement-bar-preview.tsx b/apps/admin/src/settings/site/announcement-bar/announcement-bar-preview.tsx index 5fb3ddf702d4..8b7290e0d0f6 100644 --- a/apps/admin/src/settings/site/announcement-bar/announcement-bar-preview.tsx +++ b/apps/admin/src/settings/site/announcement-bar/announcement-bar-preview.tsx @@ -1,6 +1,5 @@ import IframeBuffering from '@/settings/utils/iframe-buffering'; import React, { useCallback, useMemo } from 'react'; -import { fetchFrontendPreview } from '@/settings/utils/fetch-frontend-preview'; const getPreviewData = ( announcementBackgroundColor?: string, @@ -38,10 +37,25 @@ const AnnouncementBarPreview: React.FC = ({ return; } - fetchFrontendPreview( - url, - getPreviewData(announcementBackgroundColor, announcementContent, visibilityMemo), - ) + const previewUrl = new URL(url); + previewUrl.searchParams.set('admin_toolbar', '0'); + + // eslint-disable-next-line no-restricted-syntax -- posts preview data to the site front-end, not the Admin API + fetch(previewUrl.toString(), { + method: 'POST', + headers: { + 'Content-Type': 'text/html;charset=utf-8', + 'x-ghost-preview': getPreviewData( + announcementBackgroundColor, + announcementContent, + visibilityMemo, + ), + Accept: 'text/html', + }, + mode: 'cors', + credentials: 'include', + }) + .then((response) => response.text()) .then((data) => { // inject extra CSS to disable navigation and prevent clicks const injectedCss = `html { pointer-events: none; }`; diff --git a/apps/admin/src/settings/site/design-and-branding/theme-preview.tsx b/apps/admin/src/settings/site/design-and-branding/theme-preview.tsx index 008b2654276d..27aef5f17fe5 100644 --- a/apps/admin/src/settings/site/design-and-branding/theme-preview.tsx +++ b/apps/admin/src/settings/site/design-and-branding/theme-preview.tsx @@ -4,7 +4,6 @@ import { type CustomThemeSetting, hiddenCustomThemeSettingValue, } from '@tryghost/admin-x-framework/api/custom-theme-settings'; -import { fetchFrontendPreview } from '@/settings/utils/fetch-frontend-preview'; import { isCustomThemeSettingVisible } from '@/settings/utils/is-custom-theme-settings-visible'; type GlobalSettings = { @@ -81,33 +80,49 @@ const ThemePreview: React.FC = ({ settings, url }) => { return; } - void fetchFrontendPreview(url, previewData).then((data) => { - // inject extra CSS to disable navigation and prevent clicks - const injectedCss = `html { pointer-events: none; }`; + // Fetch theme preview HTML (suppress admin toolbar in preview) + const previewUrl = new URL(url); + previewUrl.searchParams.set('admin_toolbar', '0'); - const domParser = new DOMParser(); - const htmlDoc = domParser.parseFromString(data, 'text/html'); + // eslint-disable-next-line no-restricted-syntax -- posts preview data to the site front-end, not the Admin API + void fetch(previewUrl.toString(), { + method: 'POST', + headers: { + 'Content-Type': 'text/html;charset=utf-8', + 'x-ghost-preview': previewData, + Accept: 'text/html', + }, + mode: 'cors', + credentials: 'include', + }) + .then((response) => response.text()) + .then((data) => { + // inject extra CSS to disable navigation and prevent clicks + const injectedCss = `html { pointer-events: none; }`; - const stylesheet = htmlDoc.querySelector('style') as HTMLStyleElement; - const originalCSS = stylesheet?.innerHTML; - if (originalCSS) { - stylesheet.innerHTML = `${originalCSS}\n\n${injectedCss}`; - } else { - htmlDoc.head.innerHTML += ``; - } + const domParser = new DOMParser(); + const htmlDoc = domParser.parseFromString(data, 'text/html'); - // replace the iframe contents with the doctored preview html - const doctype = htmlDoc.doctype - ? new XMLSerializer().serializeToString(htmlDoc.doctype) - : ''; - const finalDoc = doctype + htmlDoc.documentElement.outerHTML; + const stylesheet = htmlDoc.querySelector('style') as HTMLStyleElement; + const originalCSS = stylesheet?.innerHTML; + if (originalCSS) { + stylesheet.innerHTML = `${originalCSS}\n\n${injectedCss}`; + } else { + htmlDoc.head.innerHTML += ``; + } - // Send the data to the iframe's window using postMessage - // Inject the received content into the iframe - iframe.contentDocument?.open(); - iframe.contentDocument?.write(finalDoc); - iframe.contentDocument?.close(); - }); + // replace the iframe contents with the doctored preview html + const doctype = htmlDoc.doctype + ? new XMLSerializer().serializeToString(htmlDoc.doctype) + : ''; + const finalDoc = doctype + htmlDoc.documentElement.outerHTML; + + // Send the data to the iframe's window using postMessage + // Inject the received content into the iframe + iframe.contentDocument?.open(); + iframe.contentDocument?.write(finalDoc); + iframe.contentDocument?.close(); + }); }, [previewData, url], ); diff --git a/apps/admin/src/settings/utils/fetch-frontend-preview.ts b/apps/admin/src/settings/utils/fetch-frontend-preview.ts index 756313032eb7..e3d75109f869 100644 --- a/apps/admin/src/settings/utils/fetch-frontend-preview.ts +++ b/apps/admin/src/settings/utils/fetch-frontend-preview.ts @@ -5,6 +5,7 @@ export function fetchFrontendPreview(url: string, previewData: string): Promise< const previewUrl = new URL(url); previewUrl.searchParams.set('admin_toolbar', '0'); + // eslint-disable-next-line no-restricted-syntax -- targets the site front-end, not the Admin API return fetch(previewUrl.toString(), { method: 'POST', headers: { diff --git a/apps/admin/src/whats-new/hooks/use-changelog.ts b/apps/admin/src/whats-new/hooks/use-changelog.ts index c0359afa82e8..8723f657f0b8 100644 --- a/apps/admin/src/whats-new/hooks/use-changelog.ts +++ b/apps/admin/src/whats-new/hooks/use-changelog.ts @@ -45,6 +45,7 @@ export const useChangelog = () => useQuery({ queryKey: ['changelog'], queryFn: async () => { + // eslint-disable-next-line no-restricted-syntax -- external ghost.org changelog feed, not the Admin API const response = await fetch('https://ghost.org/changelog.json'); if (!response.ok) { diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 82bdc9806283..2d169703b0aa 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -878,6 +878,9 @@ importers: specifier: 'catalog:' version: 4.4.3 devDependencies: + '@internal/cfg-eslint': + specifier: workspace:* + version: link:../../configs/eslint '@internal/cfg-eslint-react': specifier: workspace:* version: link:../../configs/eslint-react From e24e6c0b92c9a572a013bf9df973c5ad03a7926a Mon Sep 17 00:00:00 2001 From: Austin Burdine Date: Mon, 24 Aug 2026 16:18:00 -0400 Subject: [PATCH 5/7] =?UTF-8?q?=F0=9F=90=9B=20=20Fixed=20dropped=20newslet?= =?UTF-8?q?ter=20recipients=20when=20batch=20creation=20is=20interrupted?= =?UTF-8?q?=20(#30230)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit no ref If a container restarted while an email's batches were still being built, the next run treated the partial batch set as complete: sendEmail skipped createBatches whenever any batches already existed, silently abandoning the un-built tail of recipients. The email was then marked submitted with an email_count reflecting the pre-send estimate, so recipients below the interruption point were never batched or sent. createBatches is now idempotent. It reads the coverage a prior run already built (per segment: recipient count and the lowest built member id) and resumes each segment below that watermark, so re-running builds only the un-built tail and never duplicates. sendEmail always reconciles instead of skipping, and totalCount is seeded from existing coverage so the domain-warmup split and the email_count correction stay accurate. Batch creation also now aborts at a batch boundary on shutdown, mirroring the send loop, leaving a consistent partial that resumes on the next boot. --- .../email-service/batch-sending-service.js | 114 ++++-- .../batch-sending-service.test.js | 341 ++++++++++++++++-- .../services/email-service/utils/index.ts | 12 + 3 files changed, 419 insertions(+), 48 deletions(-) diff --git a/ghost/core/core/server/services/email-service/batch-sending-service.js b/ghost/core/core/server/services/email-service/batch-sending-service.js index ddb970c94b06..56c0e097b659 100644 --- a/ghost/core/core/server/services/email-service/batch-sending-service.js +++ b/ghost/core/core/server/services/email-service/batch-sending-service.js @@ -322,6 +322,24 @@ class BatchSendingService { * @throws {errors.EmailError} If one of the batches fails */ async sendEmail(email) { + // Track the whole operation (batch creation + sending) so onShutdown awaits it + // before ghost-server schedules process.exit. Covers both creating batches and the + // Mailgun POST + EmailBatch status write, so neither is killed mid-flight. + const work = this.#sendEmailInner(email); + this.#inFlight.add(work); + try { + return await work; + } finally { + this.#inFlight.delete(work); + } + } + + /** + * @private + * @param {Email} email + * @throws {errors.EmailError} If one of the batches fails + */ + async #sendEmailInner(email) { logging.info(`Sending email ${email.id}`); // Load required relations @@ -349,16 +367,16 @@ class BatchSendingService { }, ); - let batches = await this.retryDb( + const existingBatches = await this.retryDb( async () => { return await this.getBatches(email); }, { ...this.#getBeforeRetryConfig(email), description: `getBatches for email ${email.id}` }, ); - if (batches.length === 0) { - batches = await this.createBatches({ email, newsletter, post }); - } + // Always reconcile: createBatches is idempotent and resumes an interrupted creation + // (e.g. a container restart) instead of treating any existing batches as complete. + const batches = await this.createBatches({ email, newsletter, post, existingBatches }); await this.sendBatches({ email, batches, post, newsletter }); } @@ -378,11 +396,13 @@ class BatchSendingService { } /** + * Idempotent: builds only the batches not already present, resuming an interrupted + * creation from each segment's watermark. Returns the full set (existing + created). * @private - * @param {{email: Email, newsletter: Newsletter, post: Post}} data + * @param {{email: Email, newsletter: Newsletter, post: Post, existingBatches?: EmailBatch[]}} data * @returns {Promise} */ - async createBatches({ email, post, newsletter }) { + async createBatches({ email, post, newsletter, existingBatches = [] }) { logging.info(`Creating batches for email ${email.id}`); // Infinity implies all emails should be sent from the primary domain @@ -393,10 +413,32 @@ class BatchSendingService { : Infinity; } + // What a prior run already built, grouped by segment. We resume from each segment's + // watermark rather than rebuilding, which keeps this idempotent. + const coverage = await this.retryDb( + async () => { + return await this.#getExistingCoverage(email.id); + }, + { + ...this.#getBeforeRetryConfig(email), + description: `getExistingCoverage for email ${email.id}`, + }, + ); + const segments = await this.#emailRenderer.getSegments(post); - const batches = []; + // Seed with existing batches so the returned set and the domain-warmup accounting + // below span existing + newly created. + const batches = [...existingBatches]; const BATCH_SIZE = this.#sendingService.getMaximumRecipients(); let totalCount = 0; + for (const { count } of coverage.values()) { + totalCount += count; + } + if (totalCount > 0) { + logging.info( + `Resuming batch creation for email ${email.id}: ${totalCount} recipient(s) across ${coverage.size} segment(s) already built`, + ); + } for (const segment of segments) { logging.info(`Creating batches for email ${email.id} segment ${segment}`); @@ -412,9 +454,22 @@ class BatchSendingService { // Start with the id of the email, which is an objectId. We'll only fetch members that are created before the email. This is a special property of ObjectIds. // Note: we use ID and not created_at, because imported members could set a created_at in the future or past and avoid limit checking. - let lastId = email.id; + // On a resume, start below this segment's watermark so we only build the un-built + // tail (coverage is a contiguous id-descending prefix, so nothing is skipped). + const segmentCoverage = coverage.get(segment ?? null); + let lastId = segmentCoverage ? segmentCoverage.minMemberId : email.id; while (!members || lastId) { + // Stop claiming new creation work on shutdown. Bailing at a batch boundary + // (createBatch is atomic) leaves a consistent partial that resumes next boot; + // SHUTDOWN_CODE keeps the email in `submitting` rather than sending it incomplete. + if (this.#shuttingDown) { + throw new errors.InternalServerError({ + code: SHUTDOWN_CODE, + message: 'Email batch creation stopped because the container is shutting down', + }); + } + logging.info( `Fetching members batch for email ${email.id} segment ${segment}, lastId: ${lastId}`, ); @@ -507,6 +562,35 @@ class BatchSendingService { return batches; } + /** + * Coverage already built for an email, grouped by member segment. Batches are created + * atomically in descending member-id order, so MIN(member_id) is the watermark to + * resume below. + * @private + * @param {string} emailId + * @returns {Promise>} + */ + async #getExistingCoverage(emailId) { + const rows = + (await this.#db + .knex('email_recipients as r') + .join('email_batches as b', 'r.batch_id', 'b.id') + .where('r.email_id', emailId) + .groupBy('b.member_segment') + .select('b.member_segment as member_segment') + .count('r.id as count') + .min('r.member_id as min_member_id')) || []; + + const coverage = new Map(); + for (const row of rows) { + coverage.set(row.member_segment ?? null, { + count: Number(row.count), + minMemberId: row.min_member_id, + }); + } + return coverage; + } + /** * Creates a batch with retry logic and adds it to the batches array * @param {object} params @@ -603,20 +687,6 @@ class BatchSendingService { } async sendBatches({ email, batches, post, newsletter }) { - // Track the in-flight call so onShutdown can await it. The cleanup task - // must wait for the Mailgun POST + EmailBatch DB write to settle before - // ghost-server schedules process.exit, otherwise mid-flight requests get - // killed and EmailBatch rows never record what Mailgun actually accepted. - const work = this.#sendBatchesInner({ email, batches, post, newsletter }); - this.#inFlight.add(work); - try { - return await work; - } finally { - this.#inFlight.delete(work); - } - } - - async #sendBatchesInner({ email, batches, post, newsletter }) { logging.info(`Sending ${batches.length} batches for email ${email.id}`); const deadline = this.getDeliveryDeadline(email); diff --git a/ghost/core/test/unit/server/services/email-service/batch-sending-service.test.js b/ghost/core/test/unit/server/services/email-service/batch-sending-service.test.js index cda0fb8868c9..c67cb569ff88 100644 --- a/ghost/core/test/unit/server/services/email-service/batch-sending-service.test.js +++ b/ghost/core/test/unit/server/services/email-service/batch-sending-service.test.js @@ -256,9 +256,14 @@ describe('Batch Sending Service', function () { }); describe('sendEmail', function () { - it('does not create batches if already created', async function () { + it('always reconciles via createBatches, passing existing batches (idempotent resume)', async function () { + // Existing batches from a prior run are handed to createBatches, which is + // idempotent: it resumes any un-built tail rather than being skipped. The old + // behaviour skipped creation entirely when batches existed, silently abandoning + // the tail of a creation interrupted by a container restart. + const existingBatches = [createModel({}), createModel({})]; const EmailBatch = createModelClass({ - findAll: [{}, {}], + findAll: existingBatches, }); const service = new BatchSendingService({ models: { EmailBatch }, @@ -275,13 +280,17 @@ describe('Batch Sending Service', function () { }); const sendBatches = sinon.stub(service, 'sendBatches').resolves(); - const createBatches = sinon.stub(service, 'createBatches').resolves(); + // Reconcile finds nothing new to build, so it returns just the existing set. + const createBatches = sinon.stub(service, 'createBatches').resolves(existingBatches); const result = await service.sendEmail(email); assert.equal(result, undefined); sinon.assert.calledOnce(sendBatches); - sinon.assert.notCalled(createBatches); + sinon.assert.calledOnce(createBatches); + + // createBatches receives the existing batches so it can resume from their watermark + assert.equal(createBatches.firstCall.args[0].existingBatches.length, 2); - // Check called with batches + // sendBatches gets the reconciled set const argument = sendBatches.firstCall.args[0]; assert.equal(argument.batches.length, 2); }); @@ -736,6 +745,221 @@ describe('Batch Sending Service', function () { assert.equal(email.get('email_count'), 3); }); + describe('resume (idempotent creation)', function () { + // Builds a setup where `builtCount` recipients (the highest member ids) were + // already created by a prior run, leaving `watermark` as the lowest built id. + // createBatches must resume below the watermark and build only the un-built tail. + function createResumeSetup({ + builtCount, + watermark, + totalMembers, + email_count, + maxRecipients = 5, + }) { + const newsletter = createModel({}); + const domainWarmingService = { isEnabled: () => false }; + + // Intended members id00..id{N-1}, all subscribed to the newsletter + const members = new Array(totalMembers).fill(0).map((_, i) => { + const idx = String(i).padStart(2, '0'); + return createModel({ + id: `id${idx}`, + email: `example${idx}@example.com`, + uuid: `member${idx}`, + name: `Member ${idx}`, + newsletters: [newsletter], + }); + }); + + const seenFilters = []; + const Member = createModelClass({}); + Member.getFilteredCollectionQuery = ({ filter }) => { + seenFilters.push(filter); + const q = nql(filter); + const all = members + .filter((m) => q.queryJSON(m.toJSON())) + .sort((a, b) => b.id.localeCompare(a.id)); + return createDb({ all: all.map((m) => m.toJSON()) }); + }; + + const EmailBatch = createModelClass({}); + + // The service #db resolves the existing coverage for #getExistingCoverage + const coverageRows = + builtCount > 0 + ? [{ member_segment: null, count: builtCount, min_member_id: watermark }] + : []; + const db = createDb({ all: coverageRows }); + const insert = sinon.spy(db, 'insert'); + + const service = new BatchSendingService({ + models: { Member, EmailBatch }, + domainWarmingService, + emailRenderer: { + getSegments() { + return [null]; + }, + }, + sendingService: { + getMaximumRecipients() { + return maxRecipients; + }, + }, + emailSegmenter: { + getMemberFilterForSegment(n) { + return `newsletters.id:'${n.id}'`; + }, + }, + db, + }); + + // Email id sorts above every member id, so a fresh cursor includes them all + const email = createModel({ id: 'idZZ', email_count }); + return { service, email, newsletter, insert, seenFilters }; + } + + it('resumes from the segment watermark and only builds the un-built tail', async function () { + // Top 10 (id10..id19) already built; watermark = id10, tail = id00..id09 + const { service, email, newsletter, insert, seenFilters } = createResumeSetup({ + builtCount: 10, + watermark: 'id10', + totalMembers: 20, + email_count: 20, + }); + + const batches = await service.createBatches({ email, post: createModel({}), newsletter }); + + // Only the tail is built: 10 members / 5 per batch = 2 new batches + assert.equal(batches.length, 2); + const inserted = insert.getCalls().flatMap((c) => c.args[0]); + assert.equal(inserted.length, 10); + assert.deepEqual(inserted.map((r) => r.member_id).sort(), [ + 'id00', + 'id01', + 'id02', + 'id03', + 'id04', + 'id05', + 'id06', + 'id07', + 'id08', + 'id09', + ]); + + // First member fetch started below the watermark, not the email id + assert.match(seenFilters[0], /id:<'id10'/); + + // email_count reconciled to existing (10) + built (10) + assert.equal(email.get('email_count'), 20); + }); + + it('creates nothing when coverage is already complete (idempotent re-run)', async function () { + // All 20 built; watermark = id00 (lowest), nothing below it to build + const { service, email, newsletter, insert, seenFilters } = createResumeSetup({ + builtCount: 20, + watermark: 'id00', + totalMembers: 20, + email_count: 20, + }); + + const existingBatches = [createModel({}), createModel({})]; + const batches = await service.createBatches({ + email, + post: createModel({}), + newsletter, + existingBatches, + }); + + // No new recipients inserted; returned set is exactly the existing batches + assert.equal(insert.getCalls().length, 0); + assert.equal(batches.length, 2); + assert.match(seenFilters[0], /id:<'id00'/); + assert.equal(email.get('email_count'), 20); + }); + + it('builds the full set on a fresh send (no coverage), starting from the email id', async function () { + const { service, email, newsletter, insert, seenFilters } = createResumeSetup({ + builtCount: 0, + watermark: null, + totalMembers: 20, + email_count: 20, + }); + + const batches = await service.createBatches({ email, post: createModel({}), newsletter }); + + assert.equal(batches.length, 4); // 20 / 5 + assert.equal(insert.getCalls().flatMap((c) => c.args[0]).length, 20); + // Fresh send starts the cursor at the email id + assert.match(seenFilters[0], /id:<'idZZ'/); + assert.equal(email.get('email_count'), 20); + }); + + it('aborts at a batch boundary during shutdown (SHUTDOWN_CODE), leaving a resumable partial', async function () { + const newsletter = createModel({}); + const members = new Array(20).fill(0).map((_, i) => { + const idx = String(i).padStart(2, '0'); + return createModel({ + id: `id${idx}`, + email: `example${idx}@example.com`, + uuid: `member${idx}`, + name: `Member ${idx}`, + newsletters: [newsletter], + }); + }); + + let service; + let fetchCount = 0; + const Member = createModelClass({}); + Member.getFilteredCollectionQuery = ({ filter }) => { + fetchCount += 1; + // Signal shutdown after the first page is fetched and built, so the next + // loop iteration bails at the batch boundary rather than mid-batch. + if (fetchCount === 1) { + service.onPreStop(); + } + const q = nql(filter); + const all = members + .filter((m) => q.queryJSON(m.toJSON())) + .sort((a, b) => b.id.localeCompare(a.id)); + return createDb({ all: all.map((m) => m.toJSON()) }); + }; + + const db = createDb({ all: [] }); + const insert = sinon.spy(db, 'insert'); + service = new BatchSendingService({ + models: { Member, EmailBatch: createModelClass({}) }, + domainWarmingService: { isEnabled: () => false }, + emailRenderer: { + getSegments() { + return [null]; + }, + }, + sendingService: { + getMaximumRecipients() { + return 5; + }, + }, + emailSegmenter: { + getMemberFilterForSegment(n) { + return `newsletters.id:'${n.id}'`; + }, + }, + db, + }); + const email = createModel({ id: 'idZZ', email_count: 20 }); + + await assert.rejects( + service.createBatches({ email, post: createModel({}), newsletter }), + (err) => err.code === BatchSendingService.SHUTDOWN_CODE, + ); + + // Only the first batch (5 recipients) was committed before the abort; the + // remaining tail is left un-built for the next boot to resume. + const inserted = insert.getCalls().flatMap((c) => c.args[0]); + assert.equal(inserted.length, 5); + }); + }); + describe('Domain warming', function () { // Helper function to create test setup with minimal boilerplate function createDomainWarmingTestSetup({ @@ -2214,64 +2438,129 @@ describe('Batch Sending Service', function () { ); }); - it('onShutdown does not resolve until in-flight sendBatches settles', async function () { + it('onShutdown does not resolve until an in-flight send settles', async function () { + const EmailBatch = createModelClass({ findAll: [] }); const service = new BatchSendingService({ + models: { EmailBatch }, sendingService: { getTargetDeliveryWindow() { return 0; }, }, }); - // Gate sendBatch behind a manual promise so we can observe the order - // in which onShutdown and the in-flight sendBatches resolve. + // Reach the real send loop (with its worker fan-out) through sendEmail, the single + // in-flight tracker. sendBatch is gated so a send stays in flight; `reachedSend` + // lets us hold off calling onShutdown until a worker is already past the shutdown + // check — otherwise the synchronous flag flip would make the workers bail before + // sending, and we'd be testing the wrong path. + sinon.stub(service, 'createBatches').resolves([createModel({}), createModel({})]); + let markReached; + const reachedSend = new Promise((resolve) => { + markReached = resolve; + }); let releaseGate; const gate = new Promise((resolve) => { releaseGate = resolve; }); const sendBatch = sinon.stub(service, 'sendBatch').callsFake(async () => { + markReached(); await gate; return true; }); - // Kick off sendBatches; do NOT await — it must remain in flight. - const sendPromise = service.sendBatches({ - email: createModel({}), - batches: [createModel({}), createModel({})], - post: createModel({}), + const email = createModel({ + status: 'submitting', newsletter: createModel({}), + post: createModel({}), }); - // Tag each promise so we can observe resolution order. - let onShutdownDone = false; - let sendBatchesDone = false; - const onShutdownPromise = service.onShutdown().then(() => { - onShutdownDone = true; - }); + // Kick off sendEmail; do NOT await — it must remain in flight. + const sendPromise = service.sendEmail(email); + let sendEmailDone = false; sendPromise .then(() => { - sendBatchesDone = true; + sendEmailDone = true; }) .catch(() => { - sendBatchesDone = true; + sendEmailDone = true; }); - // Yield the microtask queue. Neither sendBatches nor onShutdown can have settled + // Wait until a batch send is actually in flight, then start the drain. + await reachedSend; + let onShutdownDone = false; + const onShutdownPromise = service.onShutdown().then(() => { + onShutdownDone = true; + }); + + // Yield the microtask queue. Neither the send nor onShutdown can have settled // because sendBatch is still awaiting the gate. await new Promise((resolve) => { setImmediate(resolve); }); - assert.equal(sendBatchesDone, false, 'sendBatches should still be in flight'); - assert.equal(onShutdownDone, false, 'onShutdown must wait for in-flight sendBatches'); + assert.equal(sendEmailDone, false, 'sendEmail should still be in flight'); + assert.equal(onShutdownDone, false, 'onShutdown must wait for the in-flight send'); - // Release the gate; sendBatches finishes, then onShutdown resolves. + // Release the gate; the send finishes, then onShutdown resolves. releaseGate(); await onShutdownPromise; await sendPromise; assert.equal(onShutdownDone, true); - assert.equal(sendBatchesDone, true); + assert.equal(sendEmailDone, true); sinon.assert.called(sendBatch); }); + it('onShutdown does not resolve until in-flight batch creation settles', async function () { + const EmailBatch = createModelClass({ findAll: [] }); + const service = new BatchSendingService({ + models: { EmailBatch }, + sendingService: { + getTargetDeliveryWindow() { + return 0; + }, + }, + }); + + // Gate batch creation so it stays in flight while we observe the drain. + let releaseCreation; + const creationGate = new Promise((resolve) => { + releaseCreation = resolve; + }); + sinon.stub(service, 'createBatches').callsFake(async () => { + await creationGate; + return []; + }); + const sendBatches = sinon.stub(service, 'sendBatches').resolves(); + + const email = createModel({ + status: 'submitting', + newsletter: createModel({}), + post: createModel({}), + }); + + // Kick off sendEmail; do NOT await — creation must remain in flight. + const sendPromise = service.sendEmail(email); + + let onShutdownDone = false; + const onShutdownPromise = service.onShutdown().then(() => { + onShutdownDone = true; + }); + + // Yield the microtask queue. onShutdown must not settle: createBatches is gated + // and sendBatches has not been reached yet. + await new Promise((resolve) => { + setImmediate(resolve); + }); + assert.equal(onShutdownDone, false, 'onShutdown must wait for in-flight batch creation'); + sinon.assert.notCalled(sendBatches); + + // Release creation; sending proceeds, then onShutdown resolves. + releaseCreation(); + await onShutdownPromise; + await sendPromise; + assert.equal(onShutdownDone, true); + sinon.assert.calledOnce(sendBatches); + }); + it('emailJob leaves email in submitting status when sendEmail rejects with SHUTDOWN_CODE', async function () { const captureException = sinon.stub(); const Email = createModelClass({ diff --git a/ghost/core/test/unit/server/services/email-service/utils/index.ts b/ghost/core/test/unit/server/services/email-service/utils/index.ts index fe794fe97952..c18cc7f12c7f 100644 --- a/ghost/core/test/unit/server/services/email-service/utils/index.ts +++ b/ghost/core/test/unit/server/services/email-service/utils/index.ts @@ -157,6 +157,18 @@ const createDb = ({ first, all }: DbOptions = {}) => { whereNull: function () { return this; }, + join: function () { + return this; + }, + groupBy: function () { + return this; + }, + count: function () { + return this; + }, + min: function () { + return this; + }, select: function () { return this; }, From 7ab1acf73818f588708615f2f5c613af8d032b80 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Mon, 24 Aug 2026 16:41:10 -0500 Subject: [PATCH 6/7] Fixed Admin build permissions after cache restores (#30237) ref https://github.com/TryGhost/Ghost/actions/runs/32774072137/job/97591747358 Cached Admin builds could retain restrictive file permissions, leaving the Docker runtime unable to serve Ember Admin assets. We'd get errors like this in CI: ``` EACCES: permission denied, open '/home/ghost/core/built/admin/index.html' ``` This normalizes permissions while producing the legacy Ember output so restored builds remain readable. This is only for Ember Admin, which we plan to remove soon. --- apps/admin/vite-ember-assets.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/apps/admin/vite-ember-assets.ts b/apps/admin/vite-ember-assets.ts index e78f7d589b6d..9bd36bed3aef 100644 --- a/apps/admin/vite-ember-assets.ts +++ b/apps/admin/vite-ember-assets.ts @@ -18,6 +18,19 @@ function prefixUrl(url: string, base: string): string { return `${normalizedBase}/${url}`; } +function normalizeBuildPermissions(directory: string): void { + fs.chmodSync(directory, 0o755); + + for (const entry of fs.readdirSync(directory, { withFileTypes: true })) { + const entryPath = path.join(directory, entry.name); + if (entry.isDirectory()) { + normalizeBuildPermissions(entryPath); + } else if (entry.isFile()) { + fs.chmodSync(entryPath, 0o644); + } + } +} + // Vite plugin to extract styles and scripts from Ghost admin index.html export function emberAssetsPlugin() { let config: ResolvedConfig; @@ -138,6 +151,12 @@ export function emberAssetsPlugin() { // Copy React index.html, overwriting the existing index.html const forwardIndexFile = path.resolve(GHOST_ADMIN_PATH, 'index.html'); fs.copyFileSync(reactIndexFile, forwardIndexFile); + + // Nx preserves output modes in its local and remote caches. Normalize + // both declared outputs so a cache entry created with a restrictive + // umask remains readable when restored by another user or in Docker. + normalizeBuildPermissions(config.build.outDir); + normalizeBuildPermissions(GHOST_ADMIN_PATH); } catch (error) { throw new Error( `Failed to copy admin assets: ${error instanceof Error ? error.message : String(error)}`, From 825943f999c71e87f9c4921697da928f4522f842 Mon Sep 17 00:00:00 2001 From: Evan Hahn Date: Mon, 24 Aug 2026 16:41:10 -0500 Subject: [PATCH 7/7] =?UTF-8?q?=E2=9C=A8=20Added=20new=20analytics=20field?= =?UTF-8?q?s=20to=20automations=20list=20(#30155)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref df5535b80aac8ded08e672839bb1e0196965aabc # Manual test Disabled the flag: Screenshot 1 And I can see the new columns: Screenshot 2 --- .../automations.acceptance.test.tsx | 5 +- .../src/automations/automations.test.tsx | 24 ---- apps/admin/src/automations/automations.tsx | 8 +- .../components/automations-list.test.tsx | 20 +--- .../components/automations-list.tsx | 112 ++++++++---------- 5 files changed, 55 insertions(+), 114 deletions(-) diff --git a/apps/admin/src/automations/automations.acceptance.test.tsx b/apps/admin/src/automations/automations.acceptance.test.tsx index 9174d5501110..d49d53aaa4c5 100644 --- a/apps/admin/src/automations/automations.acceptance.test.tsx +++ b/apps/admin/src/automations/automations.acceptance.test.tsx @@ -5,7 +5,6 @@ import { automationsScreen } from './automations.screen'; // Automations ships behind the `automations` beta labs flag. const AUTOMATIONS_ENABLED = { labs: { automations: true } }; -const RUN_ANALYTICS_ENABLED = { labs: { automations: true, automationRunAnalytics: true } }; describe('Automations list', () => { it('renders the automations page', async () => { @@ -13,7 +12,7 @@ describe('Automations list', () => { await renderAdminApp('/automations', AUTOMATIONS_ENABLED); await expect.element(automationsScreen.heading()).toBeVisible(); - await expect.element(automationsScreen.columnHeader('Last entry')).not.toBeInTheDocument(); + await expect.element(automationsScreen.columnHeader('Last entry')).toBeVisible(); }); it('lists the welcome automations', async () => { @@ -39,7 +38,7 @@ describe('Automations list', () => { }, }), ]); - await renderAdminApp('/automations', RUN_ANALYTICS_ENABLED); + await renderAdminApp('/automations', AUTOMATIONS_ENABLED); await expect.element(automationsScreen.link('Free member welcome flow')).toBeVisible(); await expect.element(automationsScreen.columnHeader('Last entry')).toBeVisible(); diff --git a/apps/admin/src/automations/automations.test.tsx b/apps/admin/src/automations/automations.test.tsx index a1e8c1322877..5860a3375bf9 100644 --- a/apps/admin/src/automations/automations.test.tsx +++ b/apps/admin/src/automations/automations.test.tsx @@ -3,18 +3,6 @@ import { MemoryRouter } from 'react-router'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { render, screen } from '@testing-library/react'; -const mockRunAnalyticsFlag = vi.hoisted(() => ({ enabled: true })); - -vi.mock('@tryghost/admin-x-framework/hooks', async () => { - const actual = await vi.importActual( - '@tryghost/admin-x-framework/hooks', - ); - return { - ...actual, - useFeatureFlag: () => mockRunAnalyticsFlag.enabled, - }; -}); - const { mockUseBrowseAutomations, mockUseBrowseSettings, mockUseBrowseConfig, mockUseCurrentUser } = vi.hoisted(() => ({ mockUseBrowseAutomations: vi.fn(), @@ -120,7 +108,6 @@ const renderPage = () => describe('Automations', () => { beforeEach(() => { vi.clearAllMocks(); - mockRunAnalyticsFlag.enabled = true; mockUseBrowseAutomations.mockReturnValue({ data: { automations }, isError: false, @@ -131,17 +118,6 @@ describe('Automations', () => { mockUseCurrentUser.mockReturnValue({ data: { id: 'user-1', roles: [{ name: 'Owner' }] } }); }); - it('hides run analytics when the private feature is disabled', () => { - mockRunAnalyticsFlag.enabled = false; - - renderPage(); - - expect(screen.queryByRole('columnheader', { name: 'Last entry' })).not.toBeInTheDocument(); - expect(screen.queryByRole('columnheader', { name: 'Total entries' })).not.toBeInTheDocument(); - expect(screen.queryByRole('columnheader', { name: 'In progress' })).not.toBeInTheDocument(); - expect(screen.queryByText('1,432')).not.toBeInTheDocument(); - }); - it('shows free and paid sequences when Stripe is connected', () => { renderPage(); diff --git a/apps/admin/src/automations/automations.tsx b/apps/admin/src/automations/automations.tsx index d594eeeb7ff9..d33409f596a3 100644 --- a/apps/admin/src/automations/automations.tsx +++ b/apps/admin/src/automations/automations.tsx @@ -6,11 +6,9 @@ import { Box, Container } from '@tryghost/shade/primitives'; import { ListPage } from '@tryghost/shade/page-templates'; import { PageHeader } from '@tryghost/shade/patterns'; import { useVisibleAutomations } from './hooks/use-visible-automations'; -import { useFeatureFlag } from '@tryghost/admin-x-framework/hooks'; const Automations: React.FC = () => { const { automations, error, isError, isLoading } = useVisibleAutomations(); - const showRunAnalytics = useFeatureFlag('automationRunAnalytics'); if (isError) { throw error instanceof Error ? error : new Error('Failed to load automations'); @@ -38,11 +36,7 @@ const Automations: React.FC = () => { - + diff --git a/apps/admin/src/automations/components/automations-list.test.tsx b/apps/admin/src/automations/components/automations-list.test.tsx index c67b8a4048d0..d183f0d81002 100644 --- a/apps/admin/src/automations/components/automations-list.test.tsx +++ b/apps/admin/src/automations/components/automations-list.test.tsx @@ -41,10 +41,7 @@ const renderWithRoutes = () => render( - } - path="/automations" - /> + } path="/automations" /> } path="/automations/:id" /> , @@ -61,7 +58,7 @@ describe('AutomationsList', () => { }); it('renders fetched automations with private beta copy and status labels', () => { - renderWithRouter(); + renderWithRouter(); expect(screen.getAllByTestId('automation-list-row')).toHaveLength(2); expect(screen.getByText('Free member welcome flow')).toBeInTheDocument(); @@ -81,20 +78,11 @@ describe('AutomationsList', () => { }); it('renders Never when an automation has no last entry', () => { - renderWithRouter(); + renderWithRouter(); expect(screen.getByText('Never')).toBeInTheDocument(); }); - it('hides run analytics when the feature is disabled', () => { - renderWithRouter(); - - expect(screen.queryByRole('columnheader', { name: 'Last entry' })).not.toBeInTheDocument(); - expect(screen.queryByRole('columnheader', { name: 'Total entries' })).not.toBeInTheDocument(); - expect(screen.queryByRole('columnheader', { name: 'In progress' })).not.toBeInTheDocument(); - expect(screen.queryByText('1,432')).not.toBeInTheDocument(); - }); - it('links each row to the automation sequence by id', () => { renderWithRouter(); @@ -138,7 +126,7 @@ describe('AutomationsList', () => { }); it('renders a table skeleton while loading', () => { - renderWithRouter(); + renderWithRouter(); expect(screen.getByTestId('automations-list-loading')).toBeInTheDocument(); }); diff --git a/apps/admin/src/automations/components/automations-list.tsx b/apps/admin/src/automations/components/automations-list.tsx index 270cf50cfd6e..a4ea6862051b 100644 --- a/apps/admin/src/automations/components/automations-list.tsx +++ b/apps/admin/src/automations/components/automations-list.tsx @@ -50,15 +50,14 @@ const handleRowClick = (event: React.MouseEvent) => { interface AutomationsListProps { automations?: AutomationBrowseItem[]; isLoading?: boolean; - showRunAnalytics?: boolean; } -const AutomationsListSkeleton: React.FC<{ showRunAnalytics: boolean }> = ({ showRunAnalytics }) => { +const AutomationsListSkeleton: React.FC = () => { return ( @@ -72,22 +71,16 @@ const AutomationsListSkeleton: React.FC<{ showRunAnalytics: boolean }> = ({ show - {showRunAnalytics && - AUTOMATION_STAT_COLUMNS.map((column) => ( - - - - ))} - - + {AUTOMATION_STAT_COLUMNS.map((column) => ( + + + + ))} + + ))} @@ -99,39 +92,36 @@ const AutomationsListSkeleton: React.FC<{ showRunAnalytics: boolean }> = ({ show const AutomationsList: React.FC = ({ automations = [], isLoading = false, - showRunAnalytics = false, }) => { if (isLoading) { - return ; + return ; } return (
- {showRunAnalytics && ( - - - - Name - - {AUTOMATION_STAT_COLUMNS.map((column) => ( - - {column.label} - - ))} - - Status + + + + Name + + {AUTOMATION_STAT_COLUMNS.map((column) => ( + + {column.label} - - - )} + ))} + + Status + + + {automations.map((automation) => { const description = AUTOMATION_DESCRIPTIONS[automation.slug]; @@ -176,29 +166,23 @@ const AutomationsList: React.FC = ({ {description && {description}} - {showRunAnalytics && - AUTOMATION_STAT_COLUMNS.map((column) => { - const cell = statCells[column.key]; + {AUTOMATION_STAT_COLUMNS.map((column) => { + const cell = statCells[column.key]; - return ( - - {cell.content} - - ); - })} - + return ( + + {cell.content} + + ); + })} +